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.

Author: bneradt

doc/admin-guide/files/remap.config.en.rst

@@ -459,6 +459,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).
+If no IP address is specified for either ``@src_ip`` or ``@in_ip``, the filter will
+implicitly apply to all incoming IP addresses. This can be explicitly stated with
+``@src_ip=all``.
 
 Named Filters
 =============

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;

include/proxy/http/remap/AclFiltering.h

@@ -37,22 +37,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)
   {
+    if (match_all_addresses) {
+      return true;
+    }
     IpAddr addr{ip};
     return addr.cmp(start) >= 0 && addr.cmp(end) <= 0;
   }
@@ -70,10 +75,10 @@ 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
-    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
+    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)

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<NextHopSelectionStrategy> strategy = nullptr;
   std::string remapKey;

src/proxy/http/HttpSM.cc

@@ -4301,7 +4301,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<size_t>(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);
@@ -4310,7 +4310,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<size_t>(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);

src/proxy/http/HttpTransact.cc

@@ -1106,7 +1106,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;
@@ -6491,7 +6491,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;
   }
 
@@ -6527,7 +6527,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;
     }
   }
 }

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"
@@ -450,6 +451,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;
@@ -509,7 +511,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;
@@ -529,6 +534,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;
       }
     }
 
@@ -568,6 +574,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;
       }
     }
 
@@ -593,6 +600,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();
   }

src/proxy/http/remap/UrlRewrite.cc

@@ -399,7 +399,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;
   }
 
@@ -411,43 +411,50 @@ 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->in_ip_valid) {
-        Debug("url_rewrite", "match was true and we have specified a 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 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")) {
@@ -460,28 +467,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) */