diff --git a/doc/admin-guide/plugins/rate_limit.en.rst b/doc/admin-guide/plugins/rate_limit.en.rst index c96f6a3684b..57c9d96c4c2 100644 --- a/doc/admin-guide/plugins/rate_limit.en.rst +++ b/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 diff --git a/doc/release-notes/upgrading.en.rst b/doc/release-notes/upgrading.en.rst index bec15027abc..06026470074 100644 --- a/doc/release-notes/upgrading.en.rst +++ b/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 ====================== diff --git a/plugins/experimental/rate_limit/ip_reputation.cc b/plugins/experimental/rate_limit/ip_reputation.cc index 427806630c0..1313a791fa5 100644 --- a/plugins/experimental/rate_limit/ip_reputation.cc +++ b/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(); } @@ -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(); - } + if (!validate_yaml_keys(perma, "perma-block", {"limit", "threshold", "max_age"})) { + return false; + } - if (perma["threshold"]) { - _permablock_threshold = perma["threshold"].as(); - } + if (perma["limit"]) { + _permablock_limit = perma["limit"].as(); + } - if (perma["max_age"]) { - _permablock_max_age = std::chrono::seconds(perma["max_age"].as()); - } - } else { - TSError("[%s] The perma-block node must be a map", PLUGIN_NAME); - return false; + if (perma["threshold"]) { + _permablock_threshold = perma["threshold"].as(); + } + + if (perma["max_age"]) { + _permablock_max_age = std::chrono::seconds(perma["max_age"].as()); } } diff --git a/plugins/experimental/rate_limit/limiter.h b/plugins/experimental/rate_limit/limiter.h index 715cb6316e6..791c7a578bd 100644 --- a/plugins/experimental/rate_limit/limiter.h +++ b/plugins/experimental/rate_limit/limiter.h @@ -202,7 +202,7 @@ template class RateLimiter } if (node["rate"]) { - _limit = node["rate"].as(); + _rate = node["rate"].as(); } // ToDo: One or both of these should be required @@ -211,6 +211,10 @@ template 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_MAX; if (queue["max_age"]) { @@ -221,6 +225,10 @@ template 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() : RATE_LIMITER_METRIC_PREFIX; std::string tag = metrics["tag"] ? metrics["tag"].as() : name(); diff --git a/plugins/experimental/rate_limit/lists.cc b/plugins/experimental/rate_limit/lists.cc index 83daa08b5f1..afa40b1e938 100644 --- a/plugins/experimental/rate_limit/lists.cc +++ b/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()) { diff --git a/plugins/experimental/rate_limit/sni_limiter.cc b/plugins/experimental/rate_limit/sni_limiter.cc index 36bca010045..93486c9437a 100644 --- a/plugins/experimental/rate_limit/sni_limiter.cc +++ b/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(); diff --git a/plugins/experimental/rate_limit/sni_selector.cc b/plugins/experimental/rate_limit/sni_selector.cc index 02fc08f15b9..fad36001737 100644 --- a/plugins/experimental/rate_limit/sni_selector.cc +++ b/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() 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(); 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; } diff --git a/plugins/experimental/rate_limit/sni_selector.h b/plugins/experimental/rate_limit/sni_selector.h index b25c913f62b..bbb1f0ce460 100644 --- a/plugins/experimental/rate_limit/sni_selector.h +++ b/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 diff --git a/plugins/experimental/rate_limit/utilities.cc b/plugins/experimental/rate_limit/utilities.cc index 37b682dbcb8..1e21fcc0cdf 100644 --- a/plugins/experimental/rate_limit/utilities.cc +++ b/plugins/experimental/rate_limit/utilities.cc @@ -21,6 +21,9 @@ #include "ts/remap.h" #include "utilities.h" +#include +#include + 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 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; +} diff --git a/plugins/experimental/rate_limit/utilities.h b/plugins/experimental/rate_limit/utilities.h index 6069fb171d8..4a4611d733e 100644 --- a/plugins/experimental/rate_limit/utilities.h +++ b/plugins/experimental/rate_limit/utilities.h @@ -17,13 +17,23 @@ */ #pragma once -#include #include +#include +#include +#include #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 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); diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_keys.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_keys.test.py new file mode 100644 index 00000000000..458fb65cb11 --- /dev/null +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_keys.test.py @@ -0,0 +1,128 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import copy +import yaml + +Test.Summary = 'rate_limit rejects unknown YAML keys at every configuration level.' +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) + + +class TestYamlKeys: + """Exercise configuration loading without sending traffic.""" + + def __init__(self) -> None: + config = { + 'lists': [{ + 'name': 'local', + 'cidr': ['127.0.0.1/32'] + }], + 'ip-rep': + [ + { + 'name': 'reputation', + 'buckets': 2, + 'size': 4, + 'percentage': 90, + 'max_age': 300, + 'perma-block': { + 'limit': 100, + 'threshold': 1, + 'max_age': 1800 + }, + } + ], + 'selector': + [ + { + 'sni': 'test.example.com', + 'aliases': ['alias.example.com'], + 'limit': 10, + 'rate': 0, + 'queue': { + 'size': 5, + 'max_age': 30 + }, + 'metrics': { + 'prefix': 'plugin.rate_limit', + 'tag': 'valid' + }, + 'ip-rep': 'reputation', + 'exclude': 'local', + } + ], + } + self._configure('valid', config) + for name, path, key, context in [ + ('root', (), 'selecter', 'configuration'), + ('list', ('lists', 0), 'cidrs', 'lists'), + ('selector', ('selector', 0), 'limti', 'selector'), + ('queue', ('selector', 0, 'queue'), 'max-age', 'queue'), + ('metrics', ('selector', 0, 'metrics'), 'prefxi', 'metrics'), + ('iprep', ('ip-rep', 0), 'max-age', 'ip-rep'), + ('perma', ('ip-rep', 0, 'perma-block'), 'max-age', 'perma-block'), + ]: + invalid = copy.deepcopy(config) + node = invalid + for part in path: + node = node[part] + node[key] = 1 + self._configure(name, invalid, f"Unknown key '{key}' in {context} node at line [0-9]+") + for name, config, error in [ + ('misspelled-sni', {'selector': [{'sin': 'test'}]}, "Unknown key 'sin' in selector node at line [0-9]+"), + ('no-sni', {'selector': [{'limit': 10}]}, 'selector node is not a map or without a name'), + ('bad-queue', {'selector': [{'sni': 'test', 'queue': []}]}, 'The queue node must be a map'), + ('bad-metrics', {'selector': [{'sni': 'test', 'metrics': []}]}, 'The metrics node must be a map'), + ('bad-selector', {'selector': {'sni': 'test'}}, 'The selector node must be a sequence'), + ('non-scalar-key', {'selector': [{'sni': 'test', 'queue': {('bad', 'key'): 1}}]}, + 'The queue node has a non-scalar key at line [0-9]+'), + ('bad-value', {'selector': [{'sni': 'test', 'limit': 'abc'}]}, 'Invalid value in configuration file'), + ('empty-config', None, 'The configuration file is empty'), + ]: + self._configure(name, config, error) + + @staticmethod + def _configure(name: str, config: dict | None, error: str | None = None) -> None: + ts = Test.MakeATSProcess(name, disable_log_checks=error is not None) + ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rate_limit', + }) + # A None config stands for a file that declares no rules at all. + lines = yaml.safe_dump(config).splitlines() if config is not None else ['# no rate limiting rules'] + ts.Disk.File(f'{ts.Variables.CONFIGDIR}/rate_limit.yaml', typename='ats:config').AddLines(lines) + ts.Disk.plugin_config.AddLine(f'rate_limit.so {ts.Variables.CONFIGDIR}/rate_limit.yaml') + tr = Test.AddTestRun(f'{name}: rate_limit YAML configuration') + tr.Processes.Default.Command = 'echo configuration checked' + tr.Processes.Default.ReturnCode = 0 + if error: + ts.ReturnCode = 70 # EX_SOFTWARE from TSFatal. + ts.Ready = 0 + ts.Disk.diags_log.Content = Testers.ContainsExpression(error, 'Report the invalid configuration') + ts.Disk.diags_log.Content += Testers.ExcludesExpression( + 'Traffic Server is fully initialized', 'Invalid configuration prevents startup') + watcher = Test.Processes.Process(f'{name}-watcher') + watcher.Command = 'sleep 10' + watcher.Ready = When.FileContains(ts.Disk.diags_log.Name, 'Failed to parse YAML file') + watcher.StartBefore(ts) + tr.TimeOut = 5 + tr.Processes.Default.StartBefore(watcher) + else: + ts.Disk.traffic_out.Content += Testers.ContainsExpression('Successfully loaded YAML file', 'Accept all supported keys') + tr.Processes.Default.StartBefore(ts) + + +TestYamlKeys()