Gate PP allowlist by header preface

doc/admin-guide/configuration/proxy-protocol.en.rst

@@ -49,10 +49,13 @@ configured with :ts:cv:`proxy.config.http.proxy_protocol_allowlist`.
 
    .. important::
 
-       If the allowlist is configured, requests will only be accepted from these
-       IP addresses for all ports designated for Proxy Protocol in the
-       :ts:cv:`proxy.config.http.server_ports` configuration, regardless of whether
-       the connections have the Proxy Protocol header.
+       If the allowlist is configured, connections that begin with a Proxy
+       Protocol header preface will only be accepted from these IP addresses on
+       ports designated for Proxy Protocol in the
+       :ts:cv:`proxy.config.http.server_ports` configuration. Connections
+       without a Proxy Protocol header preface are not restricted by this
+       allowlist; use :file:`ip_allow.yaml` for general source-IP access
+       control.
 
 By default, |TS| uses client's IP address that is from the peer when it applies ACL. If you configure a port to
 enable PROXY protocol and want to apply ACL against the IP address delivered by PROXY protocol, you need to have ``PROXY`` in

doc/admin-guide/files/records.yaml.en.rst

@@ -2179,10 +2179,13 @@ Proxy User Variables
 
 .. ts:cv:: CONFIG proxy.config.http.proxy_protocol_allowlist STRING ```<ip list>```
 
-   This defines a allowlist of server IPs that are trusted to provide
-   connections with Proxy Protocol information.  This is a comma delimited list
-   of IP addresses.  Addressed may be listed individually, in a range separated
-   by a dash or by using CIDR notation.
+   This defines an allowlist of server IPs that are trusted to provide
+   connections with Proxy Protocol information. This allowlist is enforced only
+   for connections that begin with a Proxy Protocol header preface; non-Proxy
+   Protocol traffic on flexible Proxy Protocol ports is not restricted by this
+   setting. Use :file:`ip_allow.yaml` for general source-IP access control. This
+   is a comma delimited list of IP addresses. Addresses may be listed
+   individually, in a range separated by a dash, or by using CIDR notation.
 
    ======================= ===========================================================
    Example                 Effect

doc/release-notes/upgrading.en.rst

@@ -163,6 +163,10 @@ The following :file:`records.yaml` changes have been made:
   :ts:cv:`proxy.config.http.header_field_max_size` have been changed to 32KB.
 - The records.yaml entry :ts:cv:`proxy.config.http.server_ports` now also accepts the
   ``allow-plain`` option
+- The records.yaml entry :ts:cv:`proxy.config.http.proxy_protocol_allowlist` is now enforced
+  only for connections on Proxy Protocol-enabled ports that begin with a Proxy Protocol
+  header preface. Non-Proxy Protocol traffic on flexible Proxy Protocol ports is no longer
+  restricted by this setting; use :file:`ip_allow.yaml` for general source-IP access control.
 - The records.yaml entry :ts:cv:`proxy.config.http.cache.max_open_write_retry_timeout` has been added to specify a timeout for starting a write to cache
 - The records.yaml entry :ts:cv:`proxy.config.net.per_client.max_connections_in` has
   been added to limit the number of connections from a client IP. This works the

include/iocore/net/NetVConnection.h

@@ -505,6 +505,8 @@ class NetVConnection : public VConnection, public PluginUserArgs<TS_USER_ARGS_VC
   void                 set_proxy_protocol_info(const ProxyProtocol &src);
   const ProxyProtocol &get_proxy_protocol_info() const;
 
+  bool has_proxy_protocol_preface(IOBufferReader *) const;
+  bool has_proxy_protocol_preface(const char *, int64_t) const;
   bool has_proxy_protocol(IOBufferReader *, int max_header_size);
   bool has_proxy_protocol(char *, int64_t *);
 

src/iocore/net/NetVConnection.cc

@@ -45,6 +45,38 @@ DbgCtl dbg_ctl_ssl{"ssl"};
 // NetVConnection
 //
 
+/**
+   PROXY Protocol preface check with IOBufferReader.
+ */
+bool
+NetVConnection::has_proxy_protocol_preface(IOBufferReader *reader) const
+{
+  if (reader == nullptr) {
+    return false;
+  }
+
+  swoc::TextView tv;
+
+  char preface[PPv2_CONNECTION_HEADER_LEN];
+  tv.assign(preface, reader->memcpy(preface, sizeof(preface), 0));
+  return proxy_protocol_detect(tv);
+}
+
+/**
+   PROXY Protocol preface check with a raw buffer.
+ */
+bool
+NetVConnection::has_proxy_protocol_preface(const char *buffer, int64_t bytes_r) const
+{
+  if (buffer == nullptr || bytes_r <= 0) {
+    return false;
+  }
+
+  swoc::TextView tv;
+  tv.assign(buffer, static_cast<size_t>(bytes_r));
+  return proxy_protocol_detect(tv);
+}
+
 /**
    PROXY Protocol check with IOBufferReader
 
@@ -55,9 +87,7 @@ NetVConnection::has_proxy_protocol(IOBufferReader *reader, int max_header_size)
 {
   swoc::TextView tv;
 
-  char preface[PPv2_CONNECTION_HEADER_LEN];
-  tv.assign(preface, reader->memcpy(preface, sizeof(preface), 0));
-  if (!proxy_protocol_detect(tv)) {
+  if (!this->has_proxy_protocol_preface(reader)) {
     return false;
   }
 

src/iocore/net/SSLNetVConnection.cc

@@ -353,45 +353,47 @@ SSLNetVConnection::read_raw_data()
 
     if (this->get_is_proxy_protocol() && this->get_proxy_protocol_version() == ProxyProtocolVersion::UNDEFINED) {
       Dbg(dbg_ctl_proxyprotocol, "proxy protocol is enabled on this port");
-      if (pp_ipmap->count() > 0) {
-        Dbg(dbg_ctl_proxyprotocol, "proxy protocol has a configured allowlist of trusted IPs - checking");
-
-        // Using get_remote_addr() will return the ip of the
-        // proxy source IP, not the Proxy Protocol client ip.
-        if (!pp_ipmap->contains(swoc::IPAddr(get_remote_addr()))) {
-          Dbg(dbg_ctl_proxyprotocol, "Source IP is NOT in the configured allowlist of trusted IPs - closing connection");
-          r = -ENOTCONN; // Need a quick close/exit here to refuse the connection!!!!!!!!!
-          goto proxy_protocol_bypass;
+      if (this->has_proxy_protocol_preface(buffer, r)) {
+        if (pp_ipmap->count() > 0) {
+          Dbg(dbg_ctl_proxyprotocol, "proxy protocol has a configured allowlist of trusted IPs - checking");
+
+          // Using get_remote_addr() will return the ip of the
+          // proxy source IP, not the Proxy Protocol client ip.
+          if (!pp_ipmap->contains(swoc::IPAddr(get_remote_addr()))) {
+            Dbg(dbg_ctl_proxyprotocol, "Source IP is NOT in the configured allowlist of trusted IPs - closing connection");
+            r = -ENOTCONN; // Need a quick close/exit here to refuse the connection!!!!!!!!!
+            goto proxy_protocol_bypass;
+          } else {
+            char new_host[INET6_ADDRSTRLEN];
+            Dbg(dbg_ctl_proxyprotocol, "Source IP [%s] is in the trusted allowlist for proxy protocol",
+                ats_ip_ntop(this->get_remote_addr(), new_host, sizeof(new_host)));
+          }
         } else {
-          char new_host[INET6_ADDRSTRLEN];
-          Dbg(dbg_ctl_proxyprotocol, "Source IP [%s] is in the trusted allowlist for proxy protocol",
-              ats_ip_ntop(this->get_remote_addr(), new_host, sizeof(new_host)));
+          Dbg(dbg_ctl_proxyprotocol, "proxy protocol DOES NOT have a configured allowlist of trusted IPs but "
+                                     "proxy protocol is enabled on this port - processing all connections with Proxy Protocol "
+                                     "headers");
         }
-      } else {
-        Dbg(dbg_ctl_proxyprotocol, "proxy protocol DOES NOT have a configured allowlist of trusted IPs but "
-                                   "proxy protocol is enabled on this port - processing all connections");
-      }
 
-      auto const stored_r = r;
-      if (this->has_proxy_protocol(buffer, &r)) {
-        Dbg(dbg_ctl_proxyprotocol, "ssl has proxy protocol header");
-        if (dbg_ctl_proxyprotocol.on()) {
-          IpEndpoint src;
-          src.sa = *(this->get_proxy_protocol_src_addr());
-          IpEndpoint dst;
-          dst.sa = *(this->get_proxy_protocol_dst_addr());
-          ip_port_text_buffer src_ipb, dst_ipb;
-          ats_ip_nptop(&src, src_ipb, sizeof(src_ipb));
-          ats_ip_nptop(&dst, dst_ipb, sizeof(dst_ipb));
-          DbgPrint(dbg_ctl_proxyprotocol, "ssl proxy protocol v%d header parsed: src=[%s] dst=[%s]",
-                   static_cast<int>(this->get_proxy_protocol_version()), src_ipb, dst_ipb);
+        auto const stored_r = r;
+        if (this->has_proxy_protocol(buffer, &r)) {
+          Dbg(dbg_ctl_proxyprotocol, "ssl has proxy protocol header");
+          if (dbg_ctl_proxyprotocol.on()) {
+            IpEndpoint src;
+            src.sa = *(this->get_proxy_protocol_src_addr());
+            IpEndpoint dst;
+            dst.sa = *(this->get_proxy_protocol_dst_addr());
+            ip_port_text_buffer src_ipb, dst_ipb;
+            ats_ip_nptop(&src, src_ipb, sizeof(src_ipb));
+            ats_ip_nptop(&dst, dst_ipb, sizeof(dst_ipb));
+            DbgPrint(dbg_ctl_proxyprotocol, "ssl proxy protocol v%d header parsed: src=[%s] dst=[%s]",
+                     static_cast<int>(this->get_proxy_protocol_version()), src_ipb, dst_ipb);
+          }
+        } else {
+          Dbg(dbg_ctl_proxyprotocol, "proxy protocol preface was present, but Proxy Protocol header could not be parsed");
+          r = stored_r;
         }
       } else {
         Dbg(dbg_ctl_proxyprotocol, "proxy protocol was enabled, but Proxy Protocol header was not present");
-        // We are flexible with the Proxy Protocol designation. Maybe not all
-        // connections include Proxy Protocol. Revert to the stored value of r so
-        // we can process the bytes that are on the wire (likely a CLIENT_HELLO).
-        r = stored_r;
       }
     }
   } // end of Proxy Protocol processing

src/proxy/ProtocolProbeSessionAccept.cc

@@ -111,27 +111,33 @@ struct ProtocolProbeTrampoline : public Continuation, public ProtocolProbeSessio
 
     if (netvc->get_is_proxy_protocol() && netvc->get_proxy_protocol_version() == ProxyProtocolVersion::UNDEFINED) {
       Dbg(dbg_ctl_proxyprotocol, "ioCompletionEvent: proxy protocol is enabled on this port");
-      if (pp_ipmap->count() > 0) {
-        Dbg(dbg_ctl_proxyprotocol, "ioCompletionEvent: proxy protocol has a configured allowlist of trusted IPs - checking");
-        if (!pp_ipmap->contains(swoc::IPAddr(netvc->get_remote_addr()))) {
+      if (netvc->has_proxy_protocol_preface(reader)) {
+        if (pp_ipmap->count() > 0) {
+          Dbg(dbg_ctl_proxyprotocol, "ioCompletionEvent: proxy protocol has a configured allowlist of trusted IPs - checking");
+          if (!pp_ipmap->contains(swoc::IPAddr(netvc->get_remote_addr()))) {
+            Dbg(dbg_ctl_proxyprotocol,
+                "ioCompletionEvent: Source IP is NOT in the configured allowlist of trusted IPs - closing connection");
+            goto done;
+          } else {
+            char new_host[INET6_ADDRSTRLEN];
+            Dbg(dbg_ctl_proxyprotocol, "ioCompletionEvent: Source IP [%s] is trusted in the allowlist for proxy protocol",
+                ats_ip_ntop(netvc->get_remote_addr(), new_host, sizeof(new_host)));
+          }
+        } else {
           Dbg(dbg_ctl_proxyprotocol,
-              "ioCompletionEvent: Source IP is NOT in the configured allowlist of trusted IPs - closing connection");
-          goto done;
+              "ioCompletionEvent: proxy protocol DOES NOT have a configured allowlist of trusted IPs but proxy protocol is "
+              "enabled on this port - processing all connections with Proxy Protocol headers");
+        }
+
+        HttpConfigParams *param           = HttpConfig::acquire();
+        int               max_header_size = param->pp_hdr_max_size;
+        HttpConfig::release(param);
+        if (netvc->has_proxy_protocol(reader, max_header_size)) {
+          Dbg(dbg_ctl_proxyprotocol, "ioCompletionEvent: http has proxy protocol header");
         } else {
-          char new_host[INET6_ADDRSTRLEN];
-          Dbg(dbg_ctl_proxyprotocol, "ioCompletionEvent: Source IP [%s] is trusted in the allowlist for proxy protocol",
-              ats_ip_ntop(netvc->get_remote_addr(), new_host, sizeof(new_host)));
+          Dbg(dbg_ctl_proxyprotocol,
+              "ioCompletionEvent: proxy protocol preface was present, but Proxy Protocol header could not be parsed");
         }
-      } else {
-        Dbg(dbg_ctl_proxyprotocol,
-            "ioCompletionEvent: proxy protocol DOES NOT have a configured allowlist of trusted IPs but proxy protocol is "
-            "enabled on this port - processing all connections");
-      }
-      HttpConfigParams *param           = HttpConfig::acquire();
-      int               max_header_size = param->pp_hdr_max_size;
-      HttpConfig::release(param);
-      if (netvc->has_proxy_protocol(reader, max_header_size)) {
-        Dbg(dbg_ctl_proxyprotocol, "ioCompletionEvent: http has proxy protocol header");
       } else {
         Dbg(dbg_ctl_proxyprotocol, "ioCompletionEvent: proxy protocol was enabled, but Proxy Protocol header was not present");
       }

tests/gold_tests/proxy_protocol/proxy_protocol.test.py

@@ -60,8 +60,8 @@ def setupTS(self, name, enable_cp):
                 "proxy.config.http.insert_forwarded": "for|by=ip|proto",
                 "proxy.config.http.insert_client_ip": 2,
                 "proxy.config.http.insert_squid_x_forwarded_for": 1,
-                "proxy.config.ssl.server.cert.path": f"{self.ts.Variables.SSLDir}",
-                "proxy.config.ssl.server.private_key.path": f"{self.ts.Variables.SSLDir}",
+                "proxy.config.ssl.server.cert.path": self.ts.Variables.SSLDir,
+                "proxy.config.ssl.server.private_key.path": self.ts.Variables.SSLDir,
                 "proxy.config.diags.debug.enabled": 1,
                 "proxy.config.diags.debug.tags": "proxyprotocol",
             })
@@ -108,6 +108,79 @@ def run(self):
         self.checkAccessLog()
 
 
+class ProxyProtocolAllowlistTest:
+    """Test that the PROXY Protocol allowlist applies only to PP-prefaced traffic."""
+
+    replay_file = "replay/proxy_protocol_allowlist.replay.yaml"
+
+    def __init__(self):
+        self.setupOriginServer()
+        self.setupTS()
+
+    def setupOriginServer(self):
+        self.server = Test.MakeVerifierServerProcess("pp-allowlist-server", self.replay_file)
+
+    def setupTS(self):
+        self.ts = Test.MakeATSProcess("ts_pp_allowlist", enable_tls=True, enable_cache=False, enable_proxy_protocol=True)
+
+        self.ts.addDefaultSSLFiles()
+        self.ts.Disk.ssl_multicert_yaml.AddLines(
+            """
+ssl_multicert:
+  - dest_ip: "*"
+    ssl_cert_name: server.pem
+    ssl_key_name: server.key
+""".split("\n"))
+
+        self.ts.Disk.remap_config.AddLine(f"map / http://127.0.0.1:{self.server.Variables.http_port}/")
+
+        self.ts.Disk.records_config.update(
+            {
+                "proxy.config.http.proxy_protocol_allowlist": "192.0.2.1",
+                "proxy.config.ssl.server.cert.path": self.ts.Variables.SSLDir,
+                "proxy.config.ssl.server.private_key.path": self.ts.Variables.SSLDir,
+                "proxy.config.diags.debug.enabled": 1,
+                "proxy.config.diags.debug.tags": "proxyprotocol",
+            })
+
+    def addCurlRun(self, name, args, return_code=0, expect_status=None, start_processes=False):
+        tr = Test.AddTestRun(name)
+        tr.TimeOut = 10
+        tr.MakeCurlCommand(args, ts=self.ts)
+        tr.Processes.Default.ReturnCode = return_code
+
+        if expect_status is not None:
+            tr.Processes.Default.Streams.stdout = Testers.ContainsExpression(expect_status, f"Expected HTTP {expect_status}")
+
+        if start_processes:
+            tr.Processes.Default.StartBefore(self.server)
+            tr.Processes.Default.StartBefore(self.ts)
+
+        tr.StillRunningAfter = self.server
+        tr.StillRunningAfter = self.ts
+
+    def run(self):
+        self.addCurlRun(
+            "Non-PP HTTP traffic bypasses proxy_protocol_allowlist",
+            f'-sS -o /dev/null -w "%{{http_code}}" -H "uuid: 1" http://127.0.0.1:{self.ts.Variables.proxy_protocol_port}/get',
+            expect_status="200",
+            start_processes=True)
+        self.addCurlRun(
+            "Non-PP TLS traffic bypasses proxy_protocol_allowlist", f'-k -sS -o /dev/null -w "%{{http_code}}" -H "uuid: 2" '
+            f'https://127.0.0.1:{self.ts.Variables.proxy_protocol_ssl_port}/get',
+            expect_status="200")
+        self.addCurlRun(
+            "PP-prefaced HTTP traffic is rejected when peer is not allowlisted",
+            f'-sS -o /dev/null --max-time 5 --haproxy-protocol '
+            f'http://127.0.0.1:{self.ts.Variables.proxy_protocol_port}/get',
+            return_code=Any(52, 56))
+        self.addCurlRun(
+            "PP-prefaced TLS traffic is rejected when peer is not allowlisted",
+            f'-k -sS -o /dev/null --max-time 5 --haproxy-protocol '
+            f'https://127.0.0.1:{self.ts.Variables.proxy_protocol_ssl_port}/get',
+            return_code=Any(35, 52, 56))
+
+
 class ProxyProtocolOutTest:
     """Test that ATS can send Proxy Protocol."""
 
@@ -166,8 +239,8 @@ def setupTS(self, tr: 'TestRun') -> None:
 
         self._ts.Disk.records_config.update(
             {
-                "proxy.config.ssl.server.cert.path": f"{self._ts.Variables.SSLDir}",
-                "proxy.config.ssl.server.private_key.path": f"{self._ts.Variables.SSLDir}",
+                "proxy.config.ssl.server.cert.path": self._ts.Variables.SSLDir,
+                "proxy.config.ssl.server.private_key.path": self._ts.Variables.SSLDir,
                 "proxy.config.diags.debug.enabled": 1,
                 "proxy.config.diags.debug.tags": "http|proxyprotocol",
                 "proxy.config.http.proxy_protocol_out": self._pp_version,
@@ -240,6 +313,7 @@ def run(self) -> None:
 
 ProxyProtocolInTest("nocp", False).run()
 ProxyProtocolInTest("cp", True).run()
+ProxyProtocolAllowlistTest().run()
 
 # non-tunnling HTTP to origin
 ProxyProtocolOutTest(pp_version=-1, is_tunnel=False, is_tls_to_origin=False).run()