rate_limit: reject unknown YAML configuration keys

doc/admin-guide/plugins/rate_limit.en.rst

@@ -142,7 +142,14 @@ configuration file. The basic use is as::
 
 
 The YAML configuration can have the following format, where the various sections
-and nodes are documented below.
+and nodes are documented below. Unknown keys at any level cause configuration
+loading to fail, with a diagnostic identifying the key, node, and line number.
+An invalid value, such as a non-numeric ``limit``, fails the load the same way.
+A failed reload keeps the previous configuration active. Use ``max_age`` (with
+an underscore) for the ``queue``, ``ip-rep``, and ``perma-block`` aging settings.
+
+The file must hold a YAML map. An empty file is an error. To load the plugin
+with no rules, write ``selector: []``.
 
    .. code-block:: yaml
 

doc/release-notes/upgrading.en.rst

@@ -49,6 +49,24 @@ Reaching a single metric by name is ``lookup()``.
 Spans handed out unnamed slots that only ``rename()`` could name, and
 ``rename()`` mutated a name that the lock free readers hand out views of.
 
+Plugins
+-------
+
+Changes to Features
+~~~~~~~~~~~~~~~~~~~
+The following plugins have been changed in this version of ATS.
+
+* rate_limit - The YAML configuration is now validated strictly:
+
+  * An unknown key at any level makes the configuration fail to load. Correct any
+    misspelled key, such as ``max-age`` in place of ``max_age``.
+  * An invalid value, such as a non-numeric ``limit``, also fails the load.
+  * An empty configuration file is an error. Write ``selector: []`` to load the
+    plugin with no rules.
+
+  A failed reload keeps the previous configuration active. For more details, please
+  check :ref:`admin-plugins-rate-limit`.
+
 Upgrading to ATS v10.x
 ======================
 

plugins/experimental/rate_limit/ip_reputation.cc

@@ -81,6 +81,10 @@ SieveLru::hasher(const std::string &ip, u_short family) // Mostly a convenience
 bool
 SieveLru::parseYaml(const YAML::Node &node)
 {
+  if (!validate_yaml_keys(node, "ip-rep", {"name", "buckets", "size", "percentage", "max_age", "perma-block"})) {
+    return false;
+  }
+
   if (node["buckets"]) {
     _num_buckets = node["buckets"].as<uint32_t>();
   }
@@ -100,21 +104,20 @@ SieveLru::parseYaml(const YAML::Node &node)
   if (node["perma-block"]) {
     const YAML::Node &perma = node["perma-block"];
 
-    if (perma.IsMap()) {
-      if (perma["limit"]) {
-        _permablock_limit = perma["limit"].as<uint32_t>();
-      }
+    if (!validate_yaml_keys(perma, "perma-block", {"limit", "threshold", "max_age"})) {
+      return false;
+    }
 
-      if (perma["threshold"]) {
-        _permablock_threshold = perma["threshold"].as<uint32_t>();
-      }
+    if (perma["limit"]) {
+      _permablock_limit = perma["limit"].as<uint32_t>();
+    }
 
-      if (perma["max_age"]) {
-        _permablock_max_age = std::chrono::seconds(perma["max_age"].as<uint32_t>());
-      }
-    } else {
-      TSError("[%s] The perma-block node must be a map", PLUGIN_NAME);
-      return false;
+    if (perma["threshold"]) {
+      _permablock_threshold = perma["threshold"].as<uint32_t>();
+    }
+
+    if (perma["max_age"]) {
+      _permablock_max_age = std::chrono::seconds(perma["max_age"].as<uint32_t>());
     }
   }
 

plugins/experimental/rate_limit/limiter.h

@@ -202,7 +202,7 @@ template <class T> class RateLimiter
     }
 
     if (node["rate"]) {
-      _limit = node["rate"].as<uint32_t>();
+      _rate = node["rate"].as<uint32_t>();
     }
 
     // ToDo: One or both of these should be required
@@ -211,6 +211,10 @@ template <class T> class RateLimiter
 
     // If enabled, we default to UINT32_MAX, but the object default is still 0 (no queue)
     if (queue) {
+      if (!validate_yaml_keys(queue, "queue", {"size", "max_age"})) {
+        return false;
+      }
+
       _max_queue = queue["size"] ? queue["size"].as<uint32_t>() : UINT32_MAX;
 
       if (queue["max_age"]) {
@@ -221,6 +225,10 @@ template <class T> class RateLimiter
     const YAML::Node &metrics = node["metrics"];
 
     if (metrics) {
+      if (!validate_yaml_keys(metrics, "metrics", {"prefix", "tag"})) {
+        return false;
+      }
+
       std::string prefix = metrics["prefix"] ? metrics["prefix"].as<std::string>() : RATE_LIMITER_METRIC_PREFIX;
       std::string tag    = metrics["tag"] ? metrics["tag"].as<std::string>() : name();
 

plugins/experimental/rate_limit/lists.cc

@@ -22,6 +22,10 @@
 bool
 List::IP::parseYaml(const YAML::Node &node)
 {
+  if (!validate_yaml_keys(node, "lists", {"name", "cidr"})) {
+    return false;
+  }
+
   const YAML::Node &cidr = node["cidr"];
 
   if (cidr && cidr.IsSequence()) {

plugins/experimental/rate_limit/sni_limiter.cc

@@ -30,7 +30,9 @@ int gVCIdx = -1;
 bool
 SniRateLimiter::parseYaml(const YAML::Node &node)
 {
-  super_type::parseYaml(node);
+  if (!super_type::parseYaml(node)) {
+    return false;
+  }
 
   if (node["ip-rep"]) {
     auto ipr_name = node["ip-rep"].as<std::string>();

plugins/experimental/rate_limit/sni_selector.cc

@@ -42,6 +42,35 @@ SniSelector::yamlParser(const std::string &yaml_file)
     return false;
   }
 
+  // yaml-cpp throws out of as<T>() on a malformed value, e.g. "limit: abc". Contain it here so such
+  // a configuration fails the load rather than terminating the process during a reload.
+  try {
+    return parseConfig(config, yaml_file);
+  } catch (YAML::Exception const &e) {
+    TSError("[%s] Invalid value in configuration file: %s.", PLUGIN_NAME, e.what());
+    return false;
+  }
+}
+
+bool
+SniSelector::parseConfig(const YAML::Node &config, const std::string &yaml_file)
+{
+  if (config.IsNull()) {
+    TSError("[%s] The configuration file is empty, use 'selector: []' to configure no rules", PLUGIN_NAME);
+    return false;
+  }
+
+  if (!validate_yaml_keys(config, "configuration", {"lists", "ip-rep", "selector"})) {
+    return false;
+  }
+
+  for (const auto *key : {"lists", "ip-rep", "selector"}) {
+    if (config[key] && !config[key].IsSequence()) {
+      TSError("[%s] The %s node must be a sequence at line %d", PLUGIN_NAME, key, config[key].Mark().line + 1);
+      return false;
+    }
+  }
+
   _yaml_file = yaml_file;
 
   // First build the Lists, if any
@@ -113,7 +142,13 @@ SniSelector::yamlParser(const std::string &yaml_file)
     for (const auto &i : sel) {
       const YAML::Node &sni = i;
 
-      if (sni.IsMap() && !sni["sni"].IsSequence()) {
+      if (!validate_yaml_keys(sni, "selector", {"sni", "aliases", "limit", "rate", "queue", "metrics", "ip-rep", "exclude"})) {
+        return false;
+      }
+
+      // On a const node, operator[] yields a zombie for a missing key, and IsScalar() throws on it.
+      // The boolean test is safe, so it has to come first.
+      if (sni["sni"] && sni["sni"].IsScalar()) {
         auto name = sni["sni"].as<std::string>();
 
         if (nullptr != findLimiter(name)) {
@@ -167,7 +202,7 @@ SniSelector::yamlParser(const std::string &yaml_file)
     }
   }
 
-  Dbg(dbg_ctl, "Succesfully loaded YAML file: %s", yaml_file.c_str());
+  Dbg(dbg_ctl, "Successfully loaded YAML file: %s", yaml_file.c_str());
 
   return true;
 }

plugins/experimental/rate_limit/sni_selector.h

@@ -185,6 +185,8 @@ class SniSelector
   static void startup(const std::string &yaml_file);
 
 private:
+  bool parseConfig(const YAML::Node &config, const std::string &yaml_file);
+
   std::string _yaml_file;
   bool        _needs_queue_cont = false;
   TSCont      _queue_cont       = nullptr;            // Continuation processing the queue periodically

plugins/experimental/rate_limit/utilities.cc

@@ -21,6 +21,9 @@
 #include "ts/remap.h"
 #include "utilities.h"
 
+#include <algorithm>
+#include <yaml-cpp/yaml.h>
+
 namespace rate_limit_ns
 {
 DbgCtl dbg_ctl{PLUGIN_NAME};
@@ -122,3 +125,28 @@ getDescriptionFromUrl(const char *url)
 
   return description;
 }
+
+bool
+validate_yaml_keys(const YAML::Node &node, const char *context, std::initializer_list<std::string_view> keys)
+{
+  if (!node.IsMap()) {
+    TSError("[%s] The %s node must be a map", PLUGIN_NAME, context);
+    return false;
+  }
+
+  for (const auto &entry : node) {
+    if (!entry.first.IsScalar()) {
+      TSError("[%s] The %s node has a non-scalar key at line %d", PLUGIN_NAME, context, entry.first.Mark().line + 1);
+      return false;
+    }
+
+    const auto &key = entry.first.Scalar();
+
+    if (std::find(keys.begin(), keys.end(), key) == keys.end()) {
+      TSError("[%s] Unknown key '%s' in %s node at line %d", PLUGIN_NAME, key.c_str(), context, entry.first.Mark().line + 1);
+      return false;
+    }
+  }
+
+  return true;
+}

plugins/experimental/rate_limit/utilities.h

@@ -17,13 +17,23 @@
  */
 #pragma once
 
-#include <string>
 #include <chrono>
+#include <initializer_list>
+#include <string>
+#include <string_view>
 
 #include "ts/ts.h"
 
+namespace YAML
+{
+class Node;
+}
+
 constexpr char const PLUGIN_NAME[] = "rate_limit";
 
+/// Reject unknown keys and malformed YAML mappings with a configuration diagnostic.
+bool validate_yaml_keys(const YAML::Node &node, const char *context, std::initializer_list<std::string_view> keys);
+
 void        delayHeader(TSHttpTxn txnp, const std::string &header, std::chrono::milliseconds delay);
 void        retryAfter(TSHttpTxn txnp, unsigned retry);
 std::string getDescriptionFromUrl(const char *url);