diff --git a/doc/admin-guide/configuration/proxy-protocol.en.rst b/doc/admin-guide/configuration/proxy-protocol.en.rst index d7fca8ac512..a2cdfcd6f39 100644 --- a/doc/admin-guide/configuration/proxy-protocol.en.rst +++ b/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 diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index b319c15b1b0..dad7f47b5da 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/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 `````` - 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 diff --git a/doc/release-notes/upgrading.en.rst b/doc/release-notes/upgrading.en.rst index 4fde3d3c7fa..c55f4a18df6 100644 --- a/doc/release-notes/upgrading.en.rst +++ b/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 diff --git a/include/iocore/net/NetVConnection.h b/include/iocore/net/NetVConnection.h index 4da60535390..41b2d80c094 100644 --- a/include/iocore/net/NetVConnection.h +++ b/include/iocore/net/NetVConnection.h @@ -505,6 +505,8 @@ class NetVConnection : public VConnection, public PluginUserArgsmemcpy(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(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; } diff --git a/src/iocore/net/SSLNetVConnection.cc b/src/iocore/net/SSLNetVConnection.cc index ab5eb9d32bd..309e16006da 100644 --- a/src/iocore/net/SSLNetVConnection.cc +++ b/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(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(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 diff --git a/src/proxy/ProtocolProbeSessionAccept.cc b/src/proxy/ProtocolProbeSessionAccept.cc index e88926456da..b8b3b2c8db2 100644 --- a/src/proxy/ProtocolProbeSessionAccept.cc +++ b/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"); } diff --git a/tests/gold_tests/proxy_protocol/proxy_protocol.test.py b/tests/gold_tests/proxy_protocol/proxy_protocol.test.py index e2abc9f7138..47f39729fea 100644 --- a/tests/gold_tests/proxy_protocol/proxy_protocol.test.py +++ b/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() diff --git a/tests/gold_tests/proxy_protocol/replay/proxy_protocol_allowlist.replay.yaml b/tests/gold_tests/proxy_protocol/replay/proxy_protocol_allowlist.replay.yaml new file mode 100644 index 00000000000..6614fa06c51 --- /dev/null +++ b/tests/gold_tests/proxy_protocol/replay/proxy_protocol_allowlist.replay.yaml @@ -0,0 +1,53 @@ +# 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. + +meta: + version: "1.0" + +sessions: + - protocol: + stack: http + transactions: + - client-request: + method: GET + version: "1.1" + url: /get + headers: + fields: + - [uuid, 1] + + server-response: + status: 200 + + proxy-response: + status: 200 + + - protocol: + stack: https + transactions: + - client-request: + method: GET + version: "1.1" + url: /get + headers: + fields: + - [uuid, 2] + + server-response: + status: 200 + + proxy-response: + status: 200