Fixing remap acl rules for @action=allow

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) */