From 56e536746d1e89c896361cde24fa9a6a67df2856 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Wed, 31 Jan 2024 23:25:34 +0000 Subject: [PATCH 1/3] Fixing remap acl rules for @action=allow If a user specifies an @action=allow remap.config ACL rule, then the implication is that requests with methods not in the allow list would be denied. Before this patch, allow ACL rules would just never deny. This fixes that behavior so that allow ACL rules that match on IP but not on method deny. --- doc/admin-guide/files/remap.config.en.rst | 3 + include/proxy/http/HttpTransact.h | 2 +- include/proxy/http/remap/AclFiltering.h | 23 ++- include/proxy/http/remap/UrlMapping.h | 2 +- src/proxy/http/HttpSM.cc | 4 +- src/proxy/http/HttpTransact.cc | 6 +- src/proxy/http/remap/RemapConfig.cc | 26 ++- src/proxy/http/remap/UrlRewrite.cc | 80 +++++---- tests/gold_tests/remap/remap_acl.test.py | 160 ++++++++++++++++++ .../remap/remap_acl_all_denied.replay.yaml | 126 ++++++++++++++ .../remap_acl_get_post_allowed.replay.yaml | 130 ++++++++++++++ .../remap_acl_get_post_denied.replay.yaml | 124 ++++++++++++++ 12 files changed, 635 insertions(+), 51 deletions(-) create mode 100644 tests/gold_tests/remap/remap_acl.test.py create mode 100644 tests/gold_tests/remap/remap_acl_all_denied.replay.yaml create mode 100644 tests/gold_tests/remap/remap_acl_get_post_allowed.replay.yaml create mode 100644 tests/gold_tests/remap/remap_acl_get_post_denied.replay.yaml diff --git a/doc/admin-guide/files/remap.config.en.rst b/doc/admin-guide/files/remap.config.en.rst index b1c7d0e1dbb..d7dc1131e97 100644 --- a/doc/admin-guide/files/remap.config.en.rst +++ b/doc/admin-guide/files/remap.config.en.rst @@ -469,6 +469,9 @@ Note that these Acl filters will return a 403 response if the resource is restri The difference between ``@src_ip`` and ``@in_ip`` is that the ``@src_ip`` is the client ip and the ``in_ip`` is the ip address the client is connecting to (the incoming address). ``@src_ip_category`` functions like ``ip_category`` described in :file:`ip_allow.yaml`. +If no IP address is specified for ``@src_ip``, ``@src_ip_category``, or +``@in_ip``, the filter will implicitly apply to all incoming IP addresses. This +can be explicitly stated with ``@src_ip=all``. Named Filters ============= diff --git a/include/proxy/http/HttpTransact.h b/include/proxy/http/HttpTransact.h index f4d4fe31639..01845b196ce 100644 --- a/include/proxy/http/HttpTransact.h +++ b/include/proxy/http/HttpTransact.h @@ -682,7 +682,7 @@ class HttpTransact bool is_upgrade_request = false; bool is_websocket = false; bool did_upgrade_succeed = false; - bool client_connection_enabled = true; + bool client_connection_allowed = true; bool acl_filtering_performed = false; bool api_cleanup_cache_read = false; bool api_server_response_no_store = false; diff --git a/include/proxy/http/remap/AclFiltering.h b/include/proxy/http/remap/AclFiltering.h index a19287b1073..a0c4a26c7f3 100644 --- a/include/proxy/http/remap/AclFiltering.h +++ b/include/proxy/http/remap/AclFiltering.h @@ -40,22 +40,27 @@ static int const ACL_FILTER_MAX_IN_IP = 8; static int const ACL_FILTER_MAX_ARGV = 512; struct src_ip_info_t { - IpAddr start; ///< Minimum value in range. - IpAddr end; ///< Maximum value in range. - bool invert; ///< Should we "invert" the meaning of this IP range ("not in range") + IpAddr start; ///< Minimum value in range. + IpAddr end; ///< Maximum value in range. + bool invert; ///< Should we "invert" the meaning of this IP range ("not in range") + bool match_all_addresses; ///< This rule should match all IP addresses. void reset() { start.invalidate(); end.invalidate(); - invert = false; + invert = false; + match_all_addresses = false; } /// @return @c true if @a ip is inside @a this range. bool contains(IpEndpoint const &ip) const { + if (match_all_addresses) { + return true; + } IpAddr addr{ip}; return addr.cmp(start) >= 0 && addr.cmp(end) <= 0; } @@ -95,11 +100,11 @@ class acl_filter_rule acl_filter_rule *next = nullptr; char *filter_name = nullptr; // optional filter name unsigned int allow_flag : 1, // action allow deny - src_ip_valid : 1, // src_ip range valid - src_ip_category_valid : 1, // src_ip range valid - in_ip_valid : 1, - active_queue_flag : 1, // filter is in active state (used by .useflt directive) - internal : 1; // filter internal HTTP requests + src_ip_valid : 1, // src_ip (client's src IP) range is specified and valid + src_ip_category_valid : 1, // src_ip_category (client's src IP category) is specified and valid + in_ip_valid : 1, // in_ip (client's dest IP) range is specified and valid + active_queue_flag : 1, // filter is in active state (used by .useflt directive) + internal : 1; // filter internal HTTP requests // we need arguments as string array for directive processing int argc = 0; // argument counter (only for filter defs) diff --git a/include/proxy/http/remap/UrlMapping.h b/include/proxy/http/remap/UrlMapping.h index 511e949f424..2779ac9563d 100644 --- a/include/proxy/http/remap/UrlMapping.h +++ b/include/proxy/http/remap/UrlMapping.h @@ -116,7 +116,7 @@ class url_mapping referer_info *referer_list = nullptr; redirect_tag_str *redir_chunk_list = nullptr; bool ip_allow_check_enabled_p = false; - acl_filter_rule *filter = nullptr; // acl filtering (list of rules) + acl_filter_rule *filter = nullptr; // acl filtering (linked list of rules) LINK(url_mapping, link); // For use with the main Queue linked list holding all the mapping std::shared_ptr strategy = nullptr; std::string remapKey; diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 3a196961fcf..ed0024a7aa7 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -4303,7 +4303,7 @@ HttpSM::check_sni_host() swoc::bwprint(error_bw_buffer, "No SNI for TLS request: connecting to {} for host='{}', returning a 403", t_state.client_info.dst_addr, std::string_view{host_name, static_cast(host_len)}); Log::error("%s", error_bw_buffer.c_str()); - this->t_state.client_connection_enabled = false; + this->t_state.client_connection_allowed = false; } } else if (strncasecmp(host_name, sni_value, host_len) != 0) { // Name mismatch Warning("SNI/hostname mismatch sni=%s host=%.*s action=%s", sni_value, host_len, host_name, action_value); @@ -4312,7 +4312,7 @@ HttpSM::check_sni_host() swoc::bwprint(error_bw_buffer, "SNI/hostname mismatch: connecting to {} for host='{}' sni='{}', returning a 403", t_state.client_info.dst_addr, std::string_view{host_name, static_cast(host_len)}, sni_value); Log::error("%s", error_bw_buffer.c_str()); - this->t_state.client_connection_enabled = false; + this->t_state.client_connection_allowed = false; } } else { SMDbg(dbg_ctl_ssl_sni, "SNI/hostname successfully match sni=%s host=%.*s", sni_value, host_len, host_name); diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index a57aed0a719..2931e4d1988 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -1107,7 +1107,7 @@ HttpTransact::EndRemapRequest(State *s) ///////////////////////////////////////////////////////////////////////// // We must close this connection if client_connection_enabled == false // ///////////////////////////////////////////////////////////////////////// - if (!s->client_connection_enabled) { + if (!s->client_connection_allowed) { build_error_response(s, HTTP_STATUS_FORBIDDEN, "Access Denied", "access#denied"); s->reverse_proxy = false; goto done; @@ -6489,7 +6489,7 @@ void HttpTransact::process_quick_http_filter(State *s, int method) { // connection already disabled by previous ACL filtering, don't modify it. - if (!s->client_connection_enabled) { + if (!s->client_connection_allowed) { return; } @@ -6525,7 +6525,7 @@ HttpTransact::process_quick_http_filter(State *s, int method) TxnDbg(dbg_ctl_ip_allow, "Line %d denial for '%.*s' from %s", acl.source_line(), method_str_len, method_str, ats_ip_ntop(&s->client_info.src_addr.sa, ipb, sizeof(ipb))); } - s->client_connection_enabled = false; + s->client_connection_allowed = false; } } } diff --git a/src/proxy/http/remap/RemapConfig.cc b/src/proxy/http/remap/RemapConfig.cc index 2736802a145..f5fbb18a684 100644 --- a/src/proxy/http/remap/RemapConfig.cc +++ b/src/proxy/http/remap/RemapConfig.cc @@ -21,6 +21,7 @@ * limitations under the License. */ +#include "proxy/http/remap/AclFiltering.h" #include "swoc/swoc_file.h" #include "proxy/http/remap/RemapConfig.h" @@ -449,6 +450,7 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const char **argv, int arg Debug("url_rewrite", "[validate_filter_args] new acl_filter_rule class was created during remap rule processing"); } + bool ip_is_listed = false; for (i = 0; i < argc; i++) { unsigned long ul; bool hasarg; @@ -508,7 +510,10 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const char **argv, int arg if (ul & REMAP_OPTFLG_INVERT) { ipi->invert = true; } - if (ats_ip_range_parse(argptr, ipi->start, ipi->end) != 0) { + std::string_view arg{argptr}; + if (arg == "all") { + ipi->match_all_addresses = true; + } else if (ats_ip_range_parse(argptr, ipi->start, ipi->end) != 0) { Debug("url_rewrite", "[validate_filter_args] Unable to parse IP value in %s", argv[i]); snprintf(errStrBuf, errStrBufSize, "Unable to parse IP value in %s", argv[i]); errStrBuf[errStrBufSize - 1] = 0; @@ -528,6 +533,7 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const char **argv, int arg if (ipi) { rule->src_ip_cnt++; rule->src_ip_valid = 1; + ip_is_listed = true; } } @@ -556,10 +562,11 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const char **argv, int arg if (ipi) { rule->src_ip_category_cnt++; rule->src_ip_category_valid = 1; + ip_is_listed = true; } } - if (ul & REMAP_OPTFLG_IN_IP) { /* "dest_ip=" option */ + if (ul & REMAP_OPTFLG_IN_IP) { /* "in_ip=" option */ if (rule->in_ip_cnt >= ACL_FILTER_MAX_IN_IP) { Debug("url_rewrite", "[validate_filter_args] Too many \"in_ip=\" filters"); snprintf(errStrBuf, errStrBufSize, "Defined more than %d \"in_ip=\" filters!", ACL_FILTER_MAX_IN_IP); @@ -575,7 +582,10 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const char **argv, int arg ipi->invert = true; } // important! use copy of argument - if (ats_ip_range_parse(argptr, ipi->start, ipi->end) != 0) { + std::string_view arg{argptr}; + if (arg == "all") { + ipi->match_all_addresses = true; + } else if (ats_ip_range_parse(argptr, ipi->start, ipi->end) != 0) { Debug("url_rewrite", "[validate_filter_args] Unable to parse IP value in %s", argv[i]); snprintf(errStrBuf, errStrBufSize, "Unable to parse IP value in %s", argv[i]); errStrBuf[errStrBufSize - 1] = 0; @@ -595,6 +605,7 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const char **argv, int arg if (ipi) { rule->in_ip_cnt++; rule->in_ip_valid = 1; + ip_is_listed = true; } } @@ -620,6 +631,15 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const char **argv, int arg } } + if (!ip_is_listed) { + // If no IP addresses are listed, treat that like `@src_ip=all`. + ink_release_assert(rule->src_ip_valid == 0 && rule->src_ip_cnt == 0); + src_ip_info_t *ipi = &rule->src_ip_array[rule->src_ip_cnt]; + ipi->match_all_addresses = true; + rule->src_ip_cnt++; + rule->src_ip_valid = 1; + } + if (is_debug_tag_set("url_rewrite")) { rule->print(); } diff --git a/src/proxy/http/remap/UrlRewrite.cc b/src/proxy/http/remap/UrlRewrite.cc index 04bf8a3c1d0..4b1f114e341 100644 --- a/src/proxy/http/remap/UrlRewrite.cc +++ b/src/proxy/http/remap/UrlRewrite.cc @@ -401,7 +401,7 @@ UrlRewrite::ReverseMap(HTTPHdr *response_header) void UrlRewrite::PerformACLFiltering(HttpTransact::State *s, url_mapping *map) { - if (unlikely(!s || s->acl_filtering_performed || !s->client_connection_enabled)) { + if (unlikely(!s || s->acl_filtering_performed || !s->client_connection_allowed)) { return; } @@ -413,60 +413,68 @@ UrlRewrite::PerformACLFiltering(HttpTransact::State *s, url_mapping *map) ink_release_assert(ats_is_ip(&s->client_info.src_addr)); - s->client_connection_enabled = true; // Default is that we allow things unless some filter matches + s->client_connection_allowed = true; // Default is that we allow things unless some filter matches - for (acl_filter_rule *rp = map->filter; rp; rp = rp->next) { - bool match = true; + int rule_index = 0; + for (acl_filter_rule *rp = map->filter; rp; rp = rp->next, ++rule_index) { + bool method_matches = true; if (rp->method_restriction_enabled) { if (method_wksidx >= 0 && method_wksidx < HTTP_WKSIDX_METHODS_CNT) { - match = rp->standard_method_lookup[method_wksidx]; + method_matches = rp->standard_method_lookup[method_wksidx]; } else if (!rp->nonstandard_methods.empty()) { - match = false; + method_matches = false; } else { int method_str_len; const char *method_str = s->hdr_info.client_request.method_get(&method_str_len); - match = rp->nonstandard_methods.count(std::string(method_str, method_str_len)); + method_matches = rp->nonstandard_methods.count(std::string(method_str, method_str_len)); } + } else { + // No method specified, therefore all match. + method_matches = true; } - if (match && rp->src_ip_valid) { - match = false; - for (int j = 0; j < rp->src_ip_cnt && !match; j++) { + // Is there a @src_ip specified? If so, check it. + bool ip_matches = false; + if (rp->src_ip_valid) { + ip_matches = false; + for (int j = 0; j < rp->src_ip_cnt && !ip_matches; j++) { bool in_range = rp->src_ip_array[j].contains(s->client_info.src_addr); if (rp->src_ip_array[j].invert) { if (!in_range) { - match = true; + ip_matches = true; } } else { if (in_range) { - match = true; + ip_matches = true; } } } } - if (match && rp->src_ip_category_valid) { - Debug("url_rewrite", "match was true and we have specified an src_ip_category field"); - match = false; - for (int j = 0; j < rp->src_ip_category_cnt && !match; j++) { + // Is there a @src_ip_category specified? If so, check it. + if (ip_matches && rp->src_ip_category_valid) { + Debug("url_rewrite", "src_ip match was true and we have specified an src_ip_category field"); + ip_matches = false; + for (int j = 0; j < rp->src_ip_category_cnt && !ip_matches; j++) { bool in_category = rp->src_ip_category_array[j].contains(s->client_info.src_addr); if (rp->src_ip_category_array[j].invert) { if (!in_category) { - match = true; + ip_matches = true; } } else { if (in_category) { - match = true; + ip_matches = true; } } } } - if (match && rp->in_ip_valid) { - Debug("url_rewrite", "match was true and we have specified an in_ip field"); - match = false; - for (int j = 0; j < rp->in_ip_cnt && !match; j++) { + // Is there an @in_ip specified? If so, check it. + if (ip_matches && rp->in_ip_valid) { + Debug("url_rewrite", "src_ip or src_ip_category match was true, checking the specified in_ip range."); + ip_matches = false; + for (int j = 0; j < rp->in_ip_cnt && !ip_matches; j++) { IpEndpoint incoming_addr; incoming_addr.assign(s->state_machine->get_ua_txn()->get_netvc()->get_local_addr()); if (is_debug_tag_set("url_rewrite")) { @@ -479,28 +487,36 @@ UrlRewrite::PerformACLFiltering(HttpTransact::State *s, url_mapping *map) bool in_range = rp->in_ip_array[j].contains(incoming_addr); if (rp->in_ip_array[j].invert) { if (!in_range) { - match = true; + ip_matches = true; } } else { if (in_range) { - match = true; + ip_matches = true; } } } } if (rp->internal) { - match = s->state_machine->get_ua_txn()->get_netvc()->get_is_internal_request(); - Debug("url_rewrite", "%s an internal request", match ? "matched" : "didn't match"); + ip_matches = s->state_machine->get_ua_txn()->get_netvc()->get_is_internal_request(); + Debug("url_rewrite", "%s an internal request", ip_matches ? "matched" : "didn't match"); } - if (match) { - // We have a match, stop evaluating filters - Debug("url_rewrite", "matched ACL filter rule, %s request", rp->allow_flag ? "allowing" : "denying"); - s->client_connection_enabled = rp->allow_flag; + Debug("url_rewrite", "%d: ACL filter %s rule matches by ip: %s, by method: %s", rule_index, + (rp->allow_flag ? "allow" : "deny"), (ip_matches ? "true" : "false"), (method_matches ? "true" : "false")); + + if (ip_matches) { + // The rule matches. Handle the method according to the rule. + if (method_matches) { + // Did they specify allowing the listed methods, or denying them? + Debug("url_rewrite", "matched ACL filter rule, %s request", rp->allow_flag ? "allowing" : "denying"); + s->client_connection_allowed = rp->allow_flag; + } else { + Debug("url_rewrite", "ACL rule matched on IP but not on method, action: %s, %s the request", + (rp->allow_flag ? "allow" : "deny"), (rp->allow_flag ? "denying" : "allowing")); + s->client_connection_allowed = !rp->allow_flag; + } break; - } else { - Debug("url_rewrite", "did NOT match ACL filter rule, %s request", rp->allow_flag ? "denying" : "allowing"); } } } /* end of for(rp = map->filter;rp;rp = rp->next) */ diff --git a/tests/gold_tests/remap/remap_acl.test.py b/tests/gold_tests/remap/remap_acl.test.py new file mode 100644 index 00000000000..62e058b23d2 --- /dev/null +++ b/tests/gold_tests/remap/remap_acl.test.py @@ -0,0 +1,160 @@ +''' +Verify remap.config acl behavior. +''' +# 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 os +import re +from typing import List, Tuple + +Test.Summary = ''' +Verify remap.config acl behavior. +''' + + +class Test_remap_acl: + """Configure a test to verify remap.config acl behavior.""" + + _ts_counter: int = 0 + _server_counter: int = 0 + _client_counter: int = 0 + + def __init__( + self, name: str, replay_file: str, ip_allow_content: str, deactivate_ip_allow: bool, acl_configuration: str, + named_acls: List[Tuple[str, str]], expected_responses: list[int]): + """Initialize the test. + + :param name: The name of the test. + :param replay_file: The replay file to be used. + :param ip_allow_content: The ip_allow configuration to be used. + :param deactivate_ip_allow: Whether to deactivate the ip_allow filter. + :param acl_configuration: The ACL configuration to be used. + :param named_acls: The set of named ACLs to configure and use. + :param expect_responses: The in-order expected responses from the proxy. + """ + self._replay_file = replay_file + self._ip_allow_content = ip_allow_content + self._deactivate_ip_allow = deactivate_ip_allow + self._acl_configuration = acl_configuration + self._named_acls = named_acls + self._expected_responses = expected_responses + + tr = Test.AddTestRun(name) + self._configure_server(tr) + self._configure_traffic_server(tr) + self._configure_client(tr) + + def _configure_server(self, tr: 'TestRun') -> None: + """Configure the server. + + :param tr: The TestRun object to associate the server process with. + """ + name = f"server-{Test_remap_acl._server_counter}" + server = tr.AddVerifierServerProcess(name, self._replay_file) + Test_remap_acl._server_counter += 1 + self._server = server + + def _configure_traffic_server(self, tr: 'TestRun') -> None: + """Configure Traffic Server. + + :param tr: The TestRun object to associate the Traffic Server process with. + """ + + name = f"ts-{Test_remap_acl._ts_counter}" + ts = tr.MakeATSProcess(name, enable_cache=False, enable_tls=True) + Test_remap_acl._ts_counter += 1 + self._ts = ts + + ts.addDefaultSSLFiles() + ts.Disk.ssl_multicert_config.AddLine('dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key') + ts.Disk.records_config.update( + { + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'http|url|remap', + 'proxy.config.http.push_method_enabled': 1, + 'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir, + 'proxy.config.quic.no_activity_timeout_in': 0, + 'proxy.config.ssl.server.private_key.path': ts.Variables.SSLDir, + 'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE', + 'proxy.config.http.connect_ports': self._server.Variables.http_port, + }) + + remap_config_lines = [] + if self._deactivate_ip_allow: + remap_config_lines.append('.deactivatefilter ip_allow') + + # First, define the name ACLs (filters). + for name, definition in self._named_acls: + remap_config_lines.append(f'.definefilter {name} {definition}') + # Now activate them. + for name, _ in self._named_acls: + remap_config_lines.append(f'.activatefilter {name}') + + remap_config_lines.append(f'map / http://127.0.0.1:{self._server.Variables.http_port} {self._acl_configuration}') + ts.Disk.remap_config.AddLines(remap_config_lines) + ts.Disk.ip_allow_yaml.AddLines(self._ip_allow_content.split("\n")) + + def _configure_client(self, tr: 'TestRun') -> None: + """Run the test. + + :param tr: The TestRun object to associate the client process with. + """ + + name = f"client-{Test_remap_acl._client_counter}" + p = tr.AddVerifierClientProcess(name, self._replay_file, https_ports=[self._ts.Variables.ssl_port]) + Test_remap_acl._client_counter += 1 + p.StartBefore(self._server) + p.StartBefore(self._ts) + + codes = [str(code) for code in self._expected_responses] + p.Streams.stdout += Testers.ContainsExpression( + '.*'.join(codes), "Verifying the expected order of responses", reflags=re.DOTALL | re.MULTILINE) + + +IP_ALLOW_CONTENT = f''' +ip_allow: + - apply: in + ip_addrs: 0/0 + action: allow +''' + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify non-allowed methods are blocked.", + replay_file='remap_acl_get_post_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=True, + acl_configuration='@action=allow @src_ip=127.0.0.1 @method=GET @method=POST', + named_acls=[], + expected_responses=[200, 200, 403, 403, 403]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify denied methods are blocked.", + replay_file='remap_acl_get_post_denied.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=True, + acl_configuration='@action=deny @src_ip=127.0.0.1 @method=GET @method=POST', + named_acls=[], + expected_responses=[403, 403, 200, 200, 400]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify a default deny filter rule works.", + replay_file='remap_acl_all_denied.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=True, + acl_configuration='@action=allow @src_ip=1.2.3.4 @method=GET @method=POST', + named_acls=[('deny', '@action=deny')], + expected_responses=[403, 403, 403, 403, 403]) diff --git a/tests/gold_tests/remap/remap_acl_all_denied.replay.yaml b/tests/gold_tests/remap/remap_acl_all_denied.replay.yaml new file mode 100644 index 00000000000..d15782b0463 --- /dev/null +++ b/tests/gold_tests/remap/remap_acl_all_denied.replay.yaml @@ -0,0 +1,126 @@ +# 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. + +# This expects a remap.config that denies all request methods. + +meta: + version: "1.0" + + blocks: + - standard_response: &standard_response + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 20 ] + +sessions: +- protocol: + - name: http + version: 1 + - name: tls + sni: test_sni + transactions: + + - client-request: + method: "GET" + version: "1.1" + url: /test/ip_allow/test_get + headers: + fields: + - [ Content-Length, 0 ] + - [ uuid, get ] + - [ X-Request, get ] + + # Not received. + <<: *standard_response + + proxy-response: + status: 403 + + - client-request: + method: "POST" + version: "1.1" + url: /test/ip_allow/test_post + headers: + fields: + - [Content-Length, 10] + - [ uuid, post ] + - [ X-Request, post ] + + # Not received. + <<: *standard_response + + proxy-response: + status: 403 + + - client-request: + method: "PUT" + version: "1.1" + url: /test/ip_allow/test_put + headers: + fields: + - [ Host, example.com ] + - [ uuid, put ] + - [ X-Request, put ] + - [ Content-Length, 113 ] + content: + encoding: plain + data: "HTTP/1.1 200 OK\nServer: ATS/10.0.0\nAccept-Ranges: bytes\nContent-Length: 6\nCache-Control: public,max-age=2\n\nCACHED" + + # Not received. + <<: *standard_response + + proxy-response: + status: 403 + + # DELETE rejected + - client-request: + method: "DELETE" + version: "1.1" + url: /test/ip_allow/test_delete + headers: + fields: + - [ Host, example.com ] + - [ uuid, delete ] + - [ X-Request, delete ] + - [ Content-Length, 0 ] + + <<: *standard_response + + proxy-response: + status: 403 + + # PUSH rejected + - client-request: + method: "PUSH" + version: "1.1" + url: /test/ip_allow/test_push + headers: + fields: + - [ Host, example.com ] + - [ uuid, push ] + - [ X-Request, push ] + - [ Content-Length, 113 ] + content: + encoding: plain + data: "HTTP/1.1 200 OK\nServer: ATS/10.0.0\nAccept-Ranges: bytes\nContent-Length: 6\nCache-Control: public,max-age=2\n\nCACHED" + + <<: *standard_response + + proxy-response: + status: 403 diff --git a/tests/gold_tests/remap/remap_acl_get_post_allowed.replay.yaml b/tests/gold_tests/remap/remap_acl_get_post_allowed.replay.yaml new file mode 100644 index 00000000000..cf08a22a8ae --- /dev/null +++ b/tests/gold_tests/remap/remap_acl_get_post_allowed.replay.yaml @@ -0,0 +1,130 @@ +# 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. + +# This expects a remap.config that allows GET and POST, but denies all other +# methods. + +meta: + version: "1.0" + + blocks: + - standard_response: &standard_response + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 20 ] + +sessions: +- protocol: + - name: http + version: 1 + - name: tls + sni: test_sni + transactions: + + - client-request: + method: "GET" + version: "1.1" + url: /test/ip_allow/test_get + headers: + fields: + - [ Content-Length, 0 ] + - [ uuid, get ] + - [ X-Request, get ] + + <<: *standard_response + + proxy-response: + status: 200 + + # POST also is in the allow list. + - client-request: + method: "POST" + version: "1.1" + url: /test/ip_allow/test_post + headers: + fields: + - [Content-Length, 10] + - [ uuid, post ] + - [ X-Request, post ] + + <<: *standard_response + + proxy-response: + status: 200 + + # PUT rejected + - client-request: + method: "PUT" + version: "1.1" + url: /test/ip_allow/test_put + headers: + fields: + - [ Host, example.com ] + - [ uuid, put ] + - [ X-Request, put ] + - [ Content-Length, 113 ] + content: + encoding: plain + data: "HTTP/1.1 200 OK\nServer: ATS/10.0.0\nAccept-Ranges: bytes\nContent-Length: 6\nCache-Control: public,max-age=2\n\nCACHED" + + # Not received. + <<: *standard_response + + # Verify that ATS rejected the PUSH. + proxy-response: + status: 403 + + # DELETE rejected + - client-request: + method: "DELETE" + version: "1.1" + url: /test/ip_allow/test_delete + headers: + fields: + - [ Host, example.com ] + - [ uuid, delete ] + - [ X-Request, delete ] + - [ Content-Length, 0 ] + + <<: *standard_response + + # Verify that ATS rejects the DELETE. + proxy-response: + status: 403 + + # PUSH rejected + - client-request: + method: "PUSH" + version: "1.1" + url: /test/ip_allow/test_push + headers: + fields: + - [ Host, example.com ] + - [ uuid, push ] + - [ X-Request, push ] + - [ Content-Length, 113 ] + content: + encoding: plain + data: "HTTP/1.1 200 OK\nServer: ATS/10.0.0\nAccept-Ranges: bytes\nContent-Length: 6\nCache-Control: public,max-age=2\n\nCACHED" + + <<: *standard_response + + # Verify that ATS rejected the PUSH. + proxy-response: + status: 403 diff --git a/tests/gold_tests/remap/remap_acl_get_post_denied.replay.yaml b/tests/gold_tests/remap/remap_acl_get_post_denied.replay.yaml new file mode 100644 index 00000000000..2a824456481 --- /dev/null +++ b/tests/gold_tests/remap/remap_acl_get_post_denied.replay.yaml @@ -0,0 +1,124 @@ +# 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. + +# This expects a remap.config that denies GET and POST, but allows all other +# methods. + +meta: + version: "1.0" + + blocks: + - standard_response: &standard_response + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 20 ] + +sessions: +- protocol: + - name: http + version: 1 + - name: tls + sni: test_sni + transactions: + + - client-request: + method: "GET" + version: "1.1" + url: /test/ip_allow/test_get + headers: + fields: + - [ Content-Length, 0 ] + - [ uuid, get ] + - [ X-Request, get ] + + <<: *standard_response + + proxy-response: + status: 403 + + - client-request: + method: "POST" + version: "1.1" + url: /test/ip_allow/test_post + headers: + fields: + - [Content-Length, 10] + - [ uuid, post ] + - [ X-Request, post ] + + <<: *standard_response + + proxy-response: + status: 403 + + # All other methods are allowed. + - client-request: + method: "PUT" + version: "1.1" + url: /test/ip_allow/test_put + headers: + fields: + - [ Host, example.com ] + - [ uuid, put ] + - [ X-Request, put ] + - [ Content-Length, 113 ] + content: + encoding: plain + data: "HTTP/1.1 200 OK\nServer: ATS/10.0.0\nAccept-Ranges: bytes\nContent-Length: 6\nCache-Control: public,max-age=2\n\nCACHED" + + <<: *standard_response + + proxy-response: + status: 200 + + - client-request: + method: "DELETE" + version: "1.1" + url: /test/ip_allow/test_delete + headers: + fields: + - [ Host, example.com ] + - [ uuid, delete ] + - [ X-Request, delete ] + - [ Content-Length, 0 ] + + <<: *standard_response + + proxy-response: + status: 200 + + - client-request: + method: "PUSH" + version: "1.1" + url: /test/ip_allow/test_push + headers: + fields: + - [ Host, example.com ] + - [ uuid, push ] + - [ X-Request, push ] + - [ Content-Length, 113 ] + content: + encoding: plain + data: "HTTP/1.1 200 OK\nServer: ATS/10.0.0\nAccept-Ranges: bytes\nContent-Length: 6\nCache-Control: public,max-age=2\n\nCACHED" + + <<: *standard_response + + # ATS will allow the PUSH, but issue a 400 saying that it can't cache it. + proxy-response: + status: 400 From bf64022c4a237d82995c85aead04cd0747409810 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Thu, 1 Feb 2024 22:56:34 +0000 Subject: [PATCH 2/3] remap.config acl: remap acl, then defined, then ip_allow This switches the order of how ACls are processed. Before this patch, defined rules were matched before remap ACL lines. Then ip_allow.yaml rules were run. This does two things: 1. Make remap.config ACL lines run before defined names. 2. If an ACL rule matches, it turns off ip_allow.yaml processing since a rule was already matched. This will likely obviate the need for a lot of uses of `.deactivatefilter ip_allow` since a matched rule will turn off ip_allow.yaml processing. --- doc/admin-guide/files/remap.config.en.rst | 12 +- src/proxy/http/remap/RemapConfig.cc | 31 +++-- src/proxy/http/remap/UrlRewrite.cc | 2 + tests/gold_tests/remap/remap_acl.test.py | 55 +++++++- .../remap/remap_acl_all_allowed.replay.yaml | 122 ++++++++++++++++++ 5 files changed, 202 insertions(+), 20 deletions(-) create mode 100644 tests/gold_tests/remap/remap_acl_all_allowed.replay.yaml diff --git a/doc/admin-guide/files/remap.config.en.rst b/doc/admin-guide/files/remap.config.en.rst index d7dc1131e97..7988220b307 100644 --- a/doc/admin-guide/files/remap.config.en.rst +++ b/doc/admin-guide/files/remap.config.en.rst @@ -435,17 +435,18 @@ You may configure Nexthop or Parent hierarchical caching rules by remap using th **@strategy** tag. See :doc:`../configuration/hierarchical-caching.en` and :doc:`strategies.yaml.en` for configuration details and examples. -Acl Filters +ACL Filters =========== -Acl filters can be created to control access of specific remap lines. The markup +ACL filters can be created to control access of specific remap lines. The markup is very similar to that of :file:`ip_allow.yaml`, with slight changes to accommodate remap markup. **Note:** As of ATS v10.x, these filters are applied just as :file:`ip_allow.yaml`, -this means once a filter matches the request, the action for that rule takes effect. +meaning once a filter matches the request, the action for that rule takes effect. In previous versions, all filters for a remap rule were evaluated, and the ``deny`` -action took priority. +action took priority. Also, if an ACL filter matches, then :file:`ip_allow.yaml` rules +will not not apply to the request because the matched rule is the ACL filter. Examples -------- @@ -464,7 +465,7 @@ Examples map http://foo.example.com/ http://foo.example.com/ @action=allow @src_ip_category=ACME_INTERNAL @method=post @method=get @method=head -Note that these Acl filters will return a 403 response if the resource is restricted. +Note that these ACL filters will return a 403 response if the resource is restricted. The difference between ``@src_ip`` and ``@in_ip`` is that the ``@src_ip`` is the client ip and the ``in_ip`` is the ip address the client is connecting to (the incoming address). @@ -489,6 +490,7 @@ is helpful for remapping internal requests without allowing access to external users. By default both internal and external requests are allowed. +In-line ACL filters take priority over named active ACL filters. Examples -------- diff --git a/src/proxy/http/remap/RemapConfig.cc b/src/proxy/http/remap/RemapConfig.cc index f5fbb18a684..8f5c6e436c8 100644 --- a/src/proxy/http/remap/RemapConfig.cc +++ b/src/proxy/http/remap/RemapConfig.cc @@ -112,24 +112,31 @@ process_filter_opt(url_mapping *mp, const BUILD_TABLE_INFO *bti, char *errStrBuf Debug("url_rewrite", "[process_filter_opt] Invalid argument(s)"); return (const char *)"[process_filter_opt] Invalid argument(s)"; } - for (rp = bti->rules_list; rp; rp = rp->next) { - if (rp->active_queue_flag) { - Debug("url_rewrite", "[process_filter_opt] Add active main filter \"%s\" (argc=%d)", - rp->filter_name ? rp->filter_name : "", rp->argc); - for (rpp = &mp->filter; *rpp; rpp = &((*rpp)->next)) { - ; - } - if ((errStr = remap_validate_filter_args(rpp, (const char **)rp->argv, rp->argc, errStrBuf, errStrBufSize)) != nullptr) { - break; - } - } - } + // ACLs are processed in this order: + // 1. A remap.config ACL line for an individual remap rule. + // 2. All named ACLs in remap.config. + // 3. Rules as specified in ip_allow.yaml. if (!errStr && (bti->remap_optflg & REMAP_OPTFLG_ALL_FILTERS) != 0) { Debug("url_rewrite", "[process_filter_opt] Add per remap filter"); for (rpp = &mp->filter; *rpp; rpp = &((*rpp)->next)) { ; } errStr = remap_validate_filter_args(rpp, (const char **)bti->argv, bti->argc, errStrBuf, errStrBufSize); + for (rp = bti->rules_list; rp; rp = rp->next) { + for (rpp = &mp->filter; *rpp; rpp = &((*rpp)->next)) { + ; + } + if (rp->active_queue_flag) { + Debug("url_rewrite", "[process_filter_opt] Add active main filter \"%s\" (argc=%d)", + rp->filter_name ? rp->filter_name : "", rp->argc); + for (rpp = &mp->filter; *rpp; rpp = &((*rpp)->next)) { + ; + } + if ((errStr = remap_validate_filter_args(rpp, (const char **)rp->argv, rp->argc, errStrBuf, errStrBufSize)) != nullptr) { + break; + } + } + } } // Set the ip allow flag for this rule to the current ip allow flag state mp->ip_allow_check_enabled_p = bti->ip_allow_check_enabled_p; diff --git a/src/proxy/http/remap/UrlRewrite.cc b/src/proxy/http/remap/UrlRewrite.cc index 4b1f114e341..611eaa09310 100644 --- a/src/proxy/http/remap/UrlRewrite.cc +++ b/src/proxy/http/remap/UrlRewrite.cc @@ -516,6 +516,8 @@ UrlRewrite::PerformACLFiltering(HttpTransact::State *s, url_mapping *map) (rp->allow_flag ? "allow" : "deny"), (rp->allow_flag ? "denying" : "allowing")); s->client_connection_allowed = !rp->allow_flag; } + // Since we have a matching ACL, no need to process ip_allow.yaml rules. + map->ip_allow_check_enabled_p = false; break; } } diff --git a/tests/gold_tests/remap/remap_acl.test.py b/tests/gold_tests/remap/remap_acl.test.py index 62e058b23d2..30e710591e9 100644 --- a/tests/gold_tests/remap/remap_acl.test.py +++ b/tests/gold_tests/remap/remap_acl.test.py @@ -130,22 +130,42 @@ def _configure_client(self, tr: 'TestRun') -> None: - apply: in ip_addrs: 0/0 action: allow + methods: + - GET ''' test_ip_allow_optional_methods = Test_remap_acl( "Verify non-allowed methods are blocked.", replay_file='remap_acl_get_post_allowed.replay.yaml', ip_allow_content=IP_ALLOW_CONTENT, - deactivate_ip_allow=True, + deactivate_ip_allow=False, acl_configuration='@action=allow @src_ip=127.0.0.1 @method=GET @method=POST', named_acls=[], expected_responses=[200, 200, 403, 403, 403]) +test_ip_allow_optional_methods = Test_remap_acl( + "Verify @src_ip=all works.", + replay_file='remap_acl_get_post_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @src_ip=all @method=GET @method=POST', + named_acls=[], + expected_responses=[200, 200, 403, 403, 403]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify no @src_ip implies all IP addresses.", + replay_file='remap_acl_get_post_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @method=GET @method=POST', + named_acls=[], + expected_responses=[200, 200, 403, 403, 403]) + test_ip_allow_optional_methods = Test_remap_acl( "Verify denied methods are blocked.", replay_file='remap_acl_get_post_denied.replay.yaml', ip_allow_content=IP_ALLOW_CONTENT, - deactivate_ip_allow=True, + deactivate_ip_allow=False, acl_configuration='@action=deny @src_ip=127.0.0.1 @method=GET @method=POST', named_acls=[], expected_responses=[403, 403, 200, 200, 400]) @@ -154,7 +174,36 @@ def _configure_client(self, tr: 'TestRun') -> None: "Verify a default deny filter rule works.", replay_file='remap_acl_all_denied.replay.yaml', ip_allow_content=IP_ALLOW_CONTENT, - deactivate_ip_allow=True, + deactivate_ip_allow=False, acl_configuration='@action=allow @src_ip=1.2.3.4 @method=GET @method=POST', named_acls=[('deny', '@action=deny')], expected_responses=[403, 403, 403, 403, 403]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify defined in-line ACLS are evaluated before named ones.", + replay_file='remap_acl_get_post_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @src_ip=127.0.0.1 @method=GET @method=POST', + named_acls=[('deny', '@action=deny')], + expected_responses=[200, 200, 403, 403, 403]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify remap.config line overrides ip_allow rule.", + replay_file='remap_acl_get_post_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @src_ip=127.0.0.1 @method=GET @method=POST', + named_acls=[], + expected_responses=[200, 200, 403, 403, 403]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify we can deactivate the ip_allow filter.", + replay_file='remap_acl_all_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=True, + # This won't match, so nothing will match since ip_allow.yaml is off. + acl_configuration='@action=allow @src_ip=1.2.3.4 @method=GET @method=POST', + named_acls=[], + # Nothing will block the request since ip_allow.yaml is off. + expected_responses=[200, 200, 200, 200, 400]) diff --git a/tests/gold_tests/remap/remap_acl_all_allowed.replay.yaml b/tests/gold_tests/remap/remap_acl_all_allowed.replay.yaml new file mode 100644 index 00000000000..52d9517d91a --- /dev/null +++ b/tests/gold_tests/remap/remap_acl_all_allowed.replay.yaml @@ -0,0 +1,122 @@ +# 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. + +# This expects a remap.config to allow all requests. + +meta: + version: "1.0" + + blocks: + - standard_response: &standard_response + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 20 ] + +sessions: +- protocol: + - name: http + version: 1 + - name: tls + sni: test_sni + transactions: + + - client-request: + method: "GET" + version: "1.1" + url: /test/ip_allow/test_get + headers: + fields: + - [ Content-Length, 0 ] + - [ uuid, get ] + - [ X-Request, get ] + + <<: *standard_response + + proxy-response: + status: 200 + + - client-request: + method: "POST" + version: "1.1" + url: /test/ip_allow/test_post + headers: + fields: + - [Content-Length, 10] + - [ uuid, post ] + - [ X-Request, post ] + + <<: *standard_response + + proxy-response: + status: 200 + + - client-request: + method: "PUT" + version: "1.1" + url: /test/ip_allow/test_put + headers: + fields: + - [ Host, example.com ] + - [ uuid, put ] + - [ X-Request, put ] + - [ Content-Length, 113 ] + content: + encoding: plain + data: "HTTP/1.1 200 OK\nServer: ATS/10.0.0\nAccept-Ranges: bytes\nContent-Length: 6\nCache-Control: public,max-age=2\n\nCACHED" + + # Not received. + <<: *standard_response + + proxy-response: + status: 200 + + - client-request: + method: "DELETE" + version: "1.1" + url: /test/ip_allow/test_delete + headers: + fields: + - [ Host, example.com ] + - [ uuid, delete ] + - [ X-Request, delete ] + - [ Content-Length, 0 ] + + <<: *standard_response + + proxy-response: + status: 200 + + - client-request: + method: "PUSH" + version: "1.1" + url: /test/ip_allow/test_push + headers: + fields: + - [ Host, example.com ] + - [ uuid, push ] + - [ X-Request, push ] + - [ Content-Length, 113 ] + content: + encoding: plain + data: "HTTP/1.1 200 OK\nServer: ATS/10.0.0\nAccept-Ranges: bytes\nContent-Length: 6\nCache-Control: public,max-age=2\n\nCACHED" + + <<: *standard_response + + proxy-response: + status: 400 From f4b98d767089d246e76c8227aaaa425f65ee0d9f Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Thu, 8 Feb 2024 21:33:41 +0000 Subject: [PATCH 3/3] src_ip_category merge fixes --- src/proxy/http/remap/AclFiltering.cc | 17 ++- src/proxy/http/remap/RemapConfig.cc | 15 +- src/proxy/http/remap/UrlRewrite.cc | 34 +++-- tests/gold_tests/ip_allow/ip_category.test.py | 6 +- ...ttps_categories_external_remap.replay.yaml | 13 +- tests/gold_tests/remap/remap_acl.test.py | 91 +++++++++++- .../remap/remap_acl_get_allowed.replay.yaml | 130 ++++++++++++++++++ 7 files changed, 271 insertions(+), 35 deletions(-) create mode 100644 tests/gold_tests/remap/remap_acl_get_allowed.replay.yaml diff --git a/src/proxy/http/remap/AclFiltering.cc b/src/proxy/http/remap/AclFiltering.cc index 259972768cb..106690cdf44 100644 --- a/src/proxy/http/remap/AclFiltering.cc +++ b/src/proxy/http/remap/AclFiltering.cc @@ -55,6 +55,7 @@ acl_filter_rule::reset() for (i = (src_ip_cnt = 0); i < ACL_FILTER_MAX_SRC_IP; i++) { src_ip_array[i].reset(); } + src_ip_category_valid = 0; for (i = (src_ip_category_cnt = 0); i < ACL_FILTER_MAX_SRC_IP; i++) { src_ip_category_array[i].reset(); } @@ -66,7 +67,7 @@ acl_filter_rule::reset() internal = 0; } -acl_filter_rule::acl_filter_rule() : allow_flag(1), src_ip_valid(0), active_queue_flag(0), internal(0) +acl_filter_rule::acl_filter_rule() : allow_flag(1), src_ip_valid(0), src_ip_category_valid(0), active_queue_flag(0), internal(0) { standard_method_lookup.resize(HTTP_WKSIDX_METHODS_CNT); ink_zero(argv); @@ -108,9 +109,11 @@ acl_filter_rule::print() { int i; printf("-----------------------------------------------------------------------------------------\n"); - printf("Filter \"%s\" status: allow_flag=%s, src_ip_valid=%s, in_ip_valid=%s, internal=%s, active_queue_flag=%d\n", + printf("Filter \"%s\" status: allow_flag=%s, src_ip_valid=%s, src_ip_category_valid=%s, in_ip_valid=%s, internal=%s, " + "active_queue_flag=%d\n", filter_name ? filter_name : "", allow_flag ? "true" : "false", src_ip_valid ? "true" : "false", - in_ip_valid ? "true" : "false", internal ? "true" : "false", static_cast(active_queue_flag)); + src_ip_category_valid ? "true" : "false", in_ip_valid ? "true" : "false", internal ? "true" : "false", + static_cast(active_queue_flag)); printf("standard methods="); for (i = 0; i < HTTP_WKSIDX_METHODS_CNT; i++) { if (standard_method_lookup[i]) { @@ -126,18 +129,20 @@ acl_filter_rule::print() printf("src_ip_cnt=%d\n", src_ip_cnt); for (i = 0; i < src_ip_cnt; i++) { ip_text_buffer b1, b2; - printf("%s - %s, ", src_ip_array[i].start.toString(b1, sizeof(b1)), src_ip_array[i].end.toString(b2, sizeof(b2))); + printf("%s - %s/invert=%s, ", src_ip_array[i].start.toString(b1, sizeof(b1)), src_ip_array[i].end.toString(b2, sizeof(b2)), + src_ip_array[i].invert ? "true" : "false"); } printf("\n"); printf("src_ip_category_cnt=%d\n", src_ip_category_cnt); for (i = 0; i < src_ip_category_cnt; i++) { - printf("%s, ", src_ip_category_array[i].category.c_str()); + printf("%s/invert=%s, ", src_ip_category_array[i].category.c_str(), src_ip_category_array[i].invert ? "true" : "false"); } printf("\n"); printf("in_ip_cnt=%d\n", in_ip_cnt); for (i = 0; i < in_ip_cnt; i++) { ip_text_buffer b1, b2; - printf("%s - %s, ", in_ip_array[i].start.toString(b1, sizeof(b1)), in_ip_array[i].end.toString(b2, sizeof(b2))); + printf("%s - %s/invert=%s, ", in_ip_array[i].start.toString(b1, sizeof(b1)), in_ip_array[i].end.toString(b2, sizeof(b2)), + in_ip_array[i].invert ? "true" : "false"); } printf("\n"); for (i = 0; i < argc; i++) { diff --git a/src/proxy/http/remap/RemapConfig.cc b/src/proxy/http/remap/RemapConfig.cc index 8f5c6e436c8..2bbc6300608 100644 --- a/src/proxy/http/remap/RemapConfig.cc +++ b/src/proxy/http/remap/RemapConfig.cc @@ -520,7 +520,7 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const char **argv, int arg std::string_view arg{argptr}; if (arg == "all") { ipi->match_all_addresses = true; - } else if (ats_ip_range_parse(argptr, ipi->start, ipi->end) != 0) { + } else if (ats_ip_range_parse(arg, ipi->start, ipi->end) != 0) { Debug("url_rewrite", "[validate_filter_args] Unable to parse IP value in %s", argv[i]); snprintf(errStrBuf, errStrBufSize, "Unable to parse IP value in %s", argv[i]); errStrBuf[errStrBufSize - 1] = 0; @@ -556,6 +556,7 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const char **argv, int arg return (const char *)errStrBuf; } src_ip_category_info_t *ipi = &rule->src_ip_category_array[rule->src_ip_category_cnt]; + ipi->category.assign(argptr); if (ul & REMAP_OPTFLG_INVERT) { ipi->invert = true; } @@ -592,7 +593,7 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const char **argv, int arg std::string_view arg{argptr}; if (arg == "all") { ipi->match_all_addresses = true; - } else if (ats_ip_range_parse(argptr, ipi->start, ipi->end) != 0) { + } else if (ats_ip_range_parse(arg, ipi->start, ipi->end) != 0) { Debug("url_rewrite", "[validate_filter_args] Unable to parse IP value in %s", argv[i]); snprintf(errStrBuf, errStrBufSize, "Unable to parse IP value in %s", argv[i]); errStrBuf[errStrBufSize - 1] = 0; @@ -702,7 +703,7 @@ remap_check_option(const char **argv, int argc, unsigned long findmode, int *_re *argptr = &argv[i][8]; } ret_flags |= (REMAP_OPTFLG_SRC_IP | REMAP_OPTFLG_INVERT); - } else if (!strncasecmp(argv[i], "src_ip_category=~", 8)) { + } else if (!strncasecmp(argv[i], "src_ip_category=~", 17)) { if ((findmode & REMAP_OPTFLG_SRC_IP_CATEGORY) != 0) { idx = i; } @@ -718,6 +719,14 @@ remap_check_option(const char **argv, int argc, unsigned long findmode, int *_re *argptr = &argv[i][7]; } ret_flags |= REMAP_OPTFLG_SRC_IP; + } else if (!strncasecmp(argv[i], "src_ip_category=", 16)) { + if ((findmode & REMAP_OPTFLG_SRC_IP_CATEGORY) != 0) { + idx = i; + } + if (argptr) { + *argptr = &argv[i][16]; + } + ret_flags |= REMAP_OPTFLG_SRC_IP_CATEGORY; } else if (!strncasecmp(argv[i], "in_ip=~", 7)) { if ((findmode & REMAP_OPTFLG_IN_IP) != 0) { idx = i; diff --git a/src/proxy/http/remap/UrlRewrite.cc b/src/proxy/http/remap/UrlRewrite.cc index 611eaa09310..097fcefc592 100644 --- a/src/proxy/http/remap/UrlRewrite.cc +++ b/src/proxy/http/remap/UrlRewrite.cc @@ -434,47 +434,49 @@ UrlRewrite::PerformACLFiltering(HttpTransact::State *s, url_mapping *map) method_matches = true; } + bool ip_matches = true; // Is there a @src_ip specified? If so, check it. - bool ip_matches = false; if (rp->src_ip_valid) { - ip_matches = false; - for (int j = 0; j < rp->src_ip_cnt && !ip_matches; j++) { + bool src_ip_matches = false; + for (int j = 0; j < rp->src_ip_cnt && !src_ip_matches; j++) { bool in_range = rp->src_ip_array[j].contains(s->client_info.src_addr); if (rp->src_ip_array[j].invert) { if (!in_range) { - ip_matches = true; + src_ip_matches = true; } } else { if (in_range) { - ip_matches = true; + src_ip_matches = true; } } } + Debug("url_rewrite", "Checked the specified src_ip, result: %s", src_ip_matches ? "true" : "false"); + ip_matches &= src_ip_matches; } // Is there a @src_ip_category specified? If so, check it. if (ip_matches && rp->src_ip_category_valid) { - Debug("url_rewrite", "src_ip match was true and we have specified an src_ip_category field"); - ip_matches = false; - for (int j = 0; j < rp->src_ip_category_cnt && !ip_matches; j++) { + bool category_ip_matches = false; + for (int j = 0; j < rp->src_ip_category_cnt && !category_ip_matches; j++) { bool in_category = rp->src_ip_category_array[j].contains(s->client_info.src_addr); if (rp->src_ip_category_array[j].invert) { if (!in_category) { - ip_matches = true; + category_ip_matches = true; } } else { if (in_category) { - ip_matches = true; + category_ip_matches = true; } } } + Debug("url_rewrite", "Checked the specified src_ip_category, result: %s", category_ip_matches ? "true" : "false"); + ip_matches &= category_ip_matches; } // Is there an @in_ip specified? If so, check it. if (ip_matches && rp->in_ip_valid) { - Debug("url_rewrite", "src_ip or src_ip_category match was true, checking the specified in_ip range."); - ip_matches = false; - for (int j = 0; j < rp->in_ip_cnt && !ip_matches; j++) { + bool in_ip_matches = false; + for (int j = 0; j < rp->in_ip_cnt && !in_ip_matches; j++) { IpEndpoint incoming_addr; incoming_addr.assign(s->state_machine->get_ua_txn()->get_netvc()->get_local_addr()); if (is_debug_tag_set("url_rewrite")) { @@ -487,14 +489,16 @@ UrlRewrite::PerformACLFiltering(HttpTransact::State *s, url_mapping *map) bool in_range = rp->in_ip_array[j].contains(incoming_addr); if (rp->in_ip_array[j].invert) { if (!in_range) { - ip_matches = true; + in_ip_matches = true; } } else { if (in_range) { - ip_matches = true; + in_ip_matches = true; } } } + Debug("url_rewrite", "Checked the specified in_ip, result: %s", in_ip_matches ? "true" : "false"); + ip_matches &= in_ip_matches; } if (rp->internal) { diff --git a/tests/gold_tests/ip_allow/ip_category.test.py b/tests/gold_tests/ip_allow/ip_category.test.py index e5044ecc627..9c04df20d21 100644 --- a/tests/gold_tests/ip_allow/ip_category.test.py +++ b/tests/gold_tests/ip_allow/ip_category.test.py @@ -316,11 +316,11 @@ def _configure_client(self, tr: 'TestRun') -> None: acl_configuration='', expected_responses=None) -# Deny GET as well via remap.config ACL. +# Deny GET via remap.config ACL. test_ip_allow_optional_methods = Test_ip_category( - "IP Category: INTERNAL", + "IP Category: EXTERNAL", replay_file='replays/https_categories_external_remap.replay.yaml', ip_allow_config=IP_ALLOW_CONTENT, ip_category_config=localhost_is_external, acl_configuration='@action=deny @src_ip_category=ACME_REMAP_EXTERNAL @method=GET', - expected_responses=[403, 403, 403]) + expected_responses=[403, 200, 200]) diff --git a/tests/gold_tests/ip_allow/replays/https_categories_external_remap.replay.yaml b/tests/gold_tests/ip_allow/replays/https_categories_external_remap.replay.yaml index 16ca64ddf78..2ead15ebb7d 100644 --- a/tests/gold_tests/ip_allow/replays/https_categories_external_remap.replay.yaml +++ b/tests/gold_tests/ip_allow/replays/https_categories_external_remap.replay.yaml @@ -14,8 +14,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -# The replay file executes various HTTP requests to verify the ip_allow policy -# applies by default to all methods. +# The replay file executes various HTTP requests to verify that GET +# is denied while all other requests are allowed. meta: version: "1.0" @@ -53,7 +53,7 @@ sessions: proxy-response: status: 403 - # POST rejected + # POST accepted - client-request: method: "POST" version: "1.1" @@ -64,13 +64,12 @@ sessions: - [ uuid, post ] - [ X-Request, post ] - # Shouldn't be used. <<: *standard_response proxy-response: - status: 403 + status: 200 - # PUSH rejected + # PUSH accepted - client-request: method: "PUSH" version: "1.1" @@ -89,4 +88,4 @@ sessions: # Verify that ATS rejected the PUSH. proxy-response: - status: 403 + status: 400 diff --git a/tests/gold_tests/remap/remap_acl.test.py b/tests/gold_tests/remap/remap_acl.test.py index 30e710591e9..b4bb1c7e85f 100644 --- a/tests/gold_tests/remap/remap_acl.test.py +++ b/tests/gold_tests/remap/remap_acl.test.py @@ -84,7 +84,7 @@ def _configure_traffic_server(self, tr: 'TestRun') -> None: ts.Disk.records_config.update( { 'proxy.config.diags.debug.enabled': 1, - 'proxy.config.diags.debug.tags': 'http|url|remap', + 'proxy.config.diags.debug.tags': 'http|url|remap|ip_allow', 'proxy.config.http.push_method_enabled': 1, 'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir, 'proxy.config.quic.no_activity_timeout_in': 0, @@ -126,6 +126,12 @@ def _configure_client(self, tr: 'TestRun') -> None: IP_ALLOW_CONTENT = f''' +ip_categories: + - name: ACME_LOCAL + ip_addrs: 127.0.0.1 + - name: ACME_EXTERNAL + ip_addrs: 5.6.7.8 + ip_allow: - apply: in ip_addrs: 0/0 @@ -143,6 +149,15 @@ def _configure_client(self, tr: 'TestRun') -> None: named_acls=[], expected_responses=[200, 200, 403, 403, 403]) +test_ip_allow_optional_methods = Test_remap_acl( + "Verify if no ACLs match, ip_allow.yaml is used.", + replay_file='remap_acl_get_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @src_ip=1.2.3.4 @method=GET @method=POST', + named_acls=[], + expected_responses=[200, 403, 403, 403, 403]) + test_ip_allow_optional_methods = Test_remap_acl( "Verify @src_ip=all works.", replay_file='remap_acl_get_post_allowed.replay.yaml', @@ -152,6 +167,15 @@ def _configure_client(self, tr: 'TestRun') -> None: named_acls=[], expected_responses=[200, 200, 403, 403, 403]) +test_ip_allow_optional_methods = Test_remap_acl( + "Verify @src_ip_category works.", + replay_file='remap_acl_get_post_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @src_ip_category=ACME_LOCAL @method=GET @method=POST', + named_acls=[], + expected_responses=[200, 200, 403, 403, 403]) + test_ip_allow_optional_methods = Test_remap_acl( "Verify no @src_ip implies all IP addresses.", replay_file='remap_acl_get_post_allowed.replay.yaml', @@ -179,6 +203,53 @@ def _configure_client(self, tr: 'TestRun') -> None: named_acls=[('deny', '@action=deny')], expected_responses=[403, 403, 403, 403, 403]) +test_ip_allow_optional_methods = Test_remap_acl( + "Verify inverting @src_ip works.", + replay_file='remap_acl_all_denied.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @src_ip=~127.0.0.1 @method=GET @method=POST', + named_acls=[('deny', '@action=deny')], + expected_responses=[403, 403, 403, 403, 403]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify inverting @src_ip works with the rule matching.", + replay_file='remap_acl_get_post_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @src_ip=~3.4.5.6 @method=GET @method=POST', + named_acls=[('deny', '@action=deny')], + expected_responses=[200, 200, 403, 403, 403]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify inverting @src_ip_category works.", + replay_file='remap_acl_all_denied.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @src_ip_category=~ACME_LOCAL @method=GET @method=POST', + named_acls=[('deny', '@action=deny')], + expected_responses=[403, 403, 403, 403, 403]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify inverting @src_ip_category works with the rule matching.", + replay_file='remap_acl_get_post_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @src_ip_category=~ACME_EXTERNAL @method=GET @method=POST', + named_acls=[('deny', '@action=deny')], + expected_responses=[200, 200, 403, 403, 403]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify @src_ip and @src_ip_category AND together.", + replay_file='remap_acl_all_denied.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + # The rule will not match because, while @src_ip matches, @src_ip_category does not. + acl_configuration='@action=allow @src_ip=127.0.0.1 @src_ip_category=ACME_EXTERNAL @method=GET @method=POST', + # Therefore, this named deny filter will block. + named_acls=[('deny', '@action=deny')], + expected_responses=[403, 403, 403, 403, 403]) + test_ip_allow_optional_methods = Test_remap_acl( "Verify defined in-line ACLS are evaluated before named ones.", replay_file='remap_acl_get_post_allowed.replay.yaml', @@ -207,3 +278,21 @@ def _configure_client(self, tr: 'TestRun') -> None: named_acls=[], # Nothing will block the request since ip_allow.yaml is off. expected_responses=[200, 200, 200, 200, 400]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify in_ip matches on IP as expected.", + replay_file='remap_acl_get_post_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @in_ip=127.0.0.1 @method=GET @method=POST', + named_acls=[], + expected_responses=[200, 200, 403, 403, 403]) + +test_ip_allow_optional_methods = Test_remap_acl( + "Verify in_ip rules do not match on other IPs.", + replay_file='remap_acl_get_allowed.replay.yaml', + ip_allow_content=IP_ALLOW_CONTENT, + deactivate_ip_allow=False, + acl_configuration='@action=allow @in_ip=3.4.5.6 @method=GET @method=POST', + named_acls=[], + expected_responses=[200, 403, 403, 403, 403]) diff --git a/tests/gold_tests/remap/remap_acl_get_allowed.replay.yaml b/tests/gold_tests/remap/remap_acl_get_allowed.replay.yaml new file mode 100644 index 00000000000..3e20822fda5 --- /dev/null +++ b/tests/gold_tests/remap/remap_acl_get_allowed.replay.yaml @@ -0,0 +1,130 @@ +# 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. + +# This expects a remap.config that allows GET, but denies all other +# methods. + +meta: + version: "1.0" + + blocks: + - standard_response: &standard_response + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 20 ] + +sessions: +- protocol: + - name: http + version: 1 + - name: tls + sni: test_sni + transactions: + + - client-request: + method: "GET" + version: "1.1" + url: /test/ip_allow/test_get + headers: + fields: + - [ Content-Length, 0 ] + - [ uuid, get ] + - [ X-Request, get ] + + <<: *standard_response + + proxy-response: + status: 200 + + # POST rejected + - client-request: + method: "POST" + version: "1.1" + url: /test/ip_allow/test_post + headers: + fields: + - [Content-Length, 10] + - [ uuid, post ] + - [ X-Request, post ] + + <<: *standard_response + + proxy-response: + status: 403 + + # PUT rejected + - client-request: + method: "PUT" + version: "1.1" + url: /test/ip_allow/test_put + headers: + fields: + - [ Host, example.com ] + - [ uuid, put ] + - [ X-Request, put ] + - [ Content-Length, 113 ] + content: + encoding: plain + data: "HTTP/1.1 200 OK\nServer: ATS/10.0.0\nAccept-Ranges: bytes\nContent-Length: 6\nCache-Control: public,max-age=2\n\nCACHED" + + # Not received. + <<: *standard_response + + # Verify that ATS rejected the PUSH. + proxy-response: + status: 403 + + # DELETE rejected + - client-request: + method: "DELETE" + version: "1.1" + url: /test/ip_allow/test_delete + headers: + fields: + - [ Host, example.com ] + - [ uuid, delete ] + - [ X-Request, delete ] + - [ Content-Length, 0 ] + + <<: *standard_response + + # Verify that ATS rejects the DELETE. + proxy-response: + status: 403 + + # PUSH rejected + - client-request: + method: "PUSH" + version: "1.1" + url: /test/ip_allow/test_push + headers: + fields: + - [ Host, example.com ] + - [ uuid, push ] + - [ X-Request, push ] + - [ Content-Length, 113 ] + content: + encoding: plain + data: "HTTP/1.1 200 OK\nServer: ATS/10.0.0\nAccept-Ranges: bytes\nContent-Length: 6\nCache-Control: public,max-age=2\n\nCACHED" + + <<: *standard_response + + # Verify that ATS rejected the PUSH. + proxy-response: + status: 403