You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by ma...@apache.org on 2022/07/05 22:42:06 UTC
[trafficserver] branch master updated: Fix % with PROXY Protocol (#8893)
This is an automated email from the ASF dual-hosted git repository.
masaori pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new cc2979d36 Fix %<chi> with PROXY Protocol (#8893)
cc2979d36 is described below
commit cc2979d36722fc40f0434be0cd7a431414371c6a
Author: Masaori Koshiba <ma...@apache.org>
AuthorDate: Wed Jul 6 07:42:00 2022 +0900
Fix %<chi> with PROXY Protocol (#8893)
---
doc/admin-guide/logging/formatting.en.rst | 3 +-
iocore/net/P_NetVConnection.h | 6 +---
iocore/net/SSLNetVConnection.cc | 4 +--
proxy/ProtocolProbeSessionAccept.cc | 3 +-
proxy/http/HttpTransactHeaders.cc | 15 ++++++--
tests/gold_tests/proxy_protocol/gold/access.gold | 3 ++
.../proxy_protocol/gold/test_case_0_stdout.gold | 2 +-
.../proxy_protocol/gold/test_case_1_stdout.gold | 2 +-
.../proxy_protocol/gold/test_case_2_stdout.gold | 14 ++++++++
.../proxy_protocol/proxy_protocol.test.py | 42 +++++++++++++++++++++-
10 files changed, 77 insertions(+), 17 deletions(-)
diff --git a/doc/admin-guide/logging/formatting.en.rst b/doc/admin-guide/logging/formatting.en.rst
index 2849694d0..e1ba9f3e5 100644
--- a/doc/admin-guide/logging/formatting.en.rst
+++ b/doc/admin-guide/logging/formatting.en.rst
@@ -488,7 +488,8 @@ incoming/outgoing ports, and network interfaces used during transactions.
===== ============== ==========================================================
Field Source Description
===== ============== ==========================================================
-chi Client IP address of the client's host.
+chi Client IP address of the client's host. If :ref:`Proxy Protocol <proxy-protocol>`
+ is used, this represents the IP address of the previous hop.
chih Client IP address of the client's host, in hexadecimal.
hii Proxy IP address for the proxy's incoming interface (to which
the client connected).
diff --git a/iocore/net/P_NetVConnection.h b/iocore/net/P_NetVConnection.h
index b878eabcb..b25a75078 100644
--- a/iocore/net/P_NetVConnection.h
+++ b/iocore/net/P_NetVConnection.h
@@ -28,11 +28,7 @@ inline sockaddr const *
NetVConnection::get_remote_addr()
{
if (!got_remote_addr) {
- if (pp_info.version != ProxyProtocolVersion::UNDEFINED) {
- set_remote_addr(get_proxy_protocol_src_addr());
- } else {
- set_remote_addr();
- }
+ set_remote_addr();
got_remote_addr = true;
}
return &remote_addr.sa;
diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc
index 87d5e09b4..bee4f5c07 100644
--- a/iocore/net/SSLNetVConnection.cc
+++ b/iocore/net/SSLNetVConnection.cc
@@ -439,8 +439,7 @@ SSLNetVConnection::read_raw_data()
// what we want now.
void *payload = nullptr;
if (!pp_ipmap->contains(get_remote_addr(), &payload)) {
- Debug("proxyprotocol", "proxy protocol src IP is NOT in the configured allowlist of trusted IPs - "
- "closing connection");
+ Debug("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 {
@@ -455,7 +454,6 @@ SSLNetVConnection::read_raw_data()
if (this->has_proxy_protocol(buffer, &r)) {
Debug("proxyprotocol", "ssl has proxy protocol header");
- set_remote_addr(get_proxy_protocol_src_addr());
if (is_debug_tag_set("proxyprotocol")) {
IpEndpoint dst;
dst.sa = *(this->get_proxy_protocol_dst_addr());
diff --git a/proxy/ProtocolProbeSessionAccept.cc b/proxy/ProtocolProbeSessionAccept.cc
index 36bba6856..dad9de9c9 100644
--- a/proxy/ProtocolProbeSessionAccept.cc
+++ b/proxy/ProtocolProbeSessionAccept.cc
@@ -108,7 +108,7 @@ struct ProtocolProbeTrampoline : public Continuation, public ProtocolProbeSessio
void *payload = nullptr;
if (!pp_ipmap->contains(netvc->get_remote_addr(), &payload)) {
Debug("proxyprotocol",
- "ioCompletionEvent: proxy protocol src IP is NOT in the configured allowlist of trusted IPs - closing connection");
+ "ioCompletionEvent: Source IP is NOT in the configured allowlist of trusted IPs - closing connection");
goto done;
} else {
char new_host[INET6_ADDRSTRLEN];
@@ -123,7 +123,6 @@ struct ProtocolProbeTrampoline : public Continuation, public ProtocolProbeSessio
if (netvc->has_proxy_protocol(reader)) {
Debug("proxyprotocol", "ioCompletionEvent: http has proxy protocol header");
- netvc->set_remote_addr(netvc->get_proxy_protocol_src_addr());
} else {
Debug("proxyprotocol",
"ioCompletionEvent: proxy protocol was enabled, but required header was not present in the transaction - "
diff --git a/proxy/http/HttpTransactHeaders.cc b/proxy/http/HttpTransactHeaders.cc
index 90b97951d..22796079d 100644
--- a/proxy/http/HttpTransactHeaders.cc
+++ b/proxy/http/HttpTransactHeaders.cc
@@ -960,18 +960,27 @@ HttpTransactHeaders::add_forwarded_field_to_request(HttpTransact::State *s, HTTP
ts::LocalBufferWriter<1024> hdr;
- if (optSet[HttpForwarded::FOR] and ats_is_ip(&s->client_info.src_addr.sa)) {
+ IpEndpoint src_addr = s->client_info.src_addr;
+ if (s->state_machine->ua_txn && s->state_machine->ua_txn->get_netvc()) {
+ const ProxyProtocol &pp = s->state_machine->ua_txn->get_netvc()->get_proxy_protocol_info();
+
+ if (pp.version != ProxyProtocolVersion::UNDEFINED) {
+ src_addr = pp.src_addr;
+ }
+ }
+
+ if (optSet[HttpForwarded::FOR] and ats_is_ip(&src_addr.sa)) {
// NOTE: The logic within this if statement assumes that hdr is empty at this point.
hdr << "for=";
- bool is_ipv6 = ats_is_ip6(&s->client_info.src_addr.sa);
+ bool is_ipv6 = ats_is_ip6(&src_addr.sa);
if (is_ipv6) {
hdr << "\"[";
}
- if (ats_ip_ntop(&s->client_info.src_addr.sa, hdr.auxBuffer(), hdr.remaining()) == nullptr) {
+ if (ats_ip_ntop(&src_addr.sa, hdr.auxBuffer(), hdr.remaining()) == nullptr) {
Debug("http_trans", "[add_forwarded_field_to_outgoing_request] ats_ip_ntop() call failed");
return;
}
diff --git a/tests/gold_tests/proxy_protocol/gold/access.gold b/tests/gold_tests/proxy_protocol/gold/access.gold
new file mode 100644
index 000000000..54d08479d
--- /dev/null
+++ b/tests/gold_tests/proxy_protocol/gold/access.gold
@@ -0,0 +1,3 @@
+127.0.0.1 127.0.0.1
+127.0.0.1 127.0.0.1
+127.0.0.1 198.51.100.1
diff --git a/tests/gold_tests/proxy_protocol/gold/test_case_0_stdout.gold b/tests/gold_tests/proxy_protocol/gold/test_case_0_stdout.gold
index 103929503..ad84f31d9 100644
--- a/tests/gold_tests/proxy_protocol/gold/test_case_0_stdout.gold
+++ b/tests/gold_tests/proxy_protocol/gold/test_case_0_stdout.gold
@@ -2,7 +2,7 @@
``
"headers": {
``
- "Forwarded": "for=127.0.0.1;proto=http",
+ "Forwarded": "for=127.0.0.1;by=127.0.0.1;proto=http",
``
},
``
diff --git a/tests/gold_tests/proxy_protocol/gold/test_case_1_stdout.gold b/tests/gold_tests/proxy_protocol/gold/test_case_1_stdout.gold
index b219208c7..cac9e4fea 100644
--- a/tests/gold_tests/proxy_protocol/gold/test_case_1_stdout.gold
+++ b/tests/gold_tests/proxy_protocol/gold/test_case_1_stdout.gold
@@ -2,7 +2,7 @@
``
"headers": {
``
- "Forwarded": "for=127.0.0.1;proto=https",
+ "Forwarded": "for=127.0.0.1;by=127.0.0.1;proto=https",
``
},
``
diff --git a/tests/gold_tests/proxy_protocol/gold/test_case_2_stdout.gold b/tests/gold_tests/proxy_protocol/gold/test_case_2_stdout.gold
new file mode 100644
index 000000000..e10a9aa55
--- /dev/null
+++ b/tests/gold_tests/proxy_protocol/gold/test_case_2_stdout.gold
@@ -0,0 +1,14 @@
+HTTP/1.1 200 OK
+Server: ATS/``
+Date: ``
+Age: ``
+
+{
+``
+ "headers":``{
+``
+ "Forwarded":``"for=198.51.100.1;by=127.0.0.1;proto=http",
+``
+ },
+``
+}
diff --git a/tests/gold_tests/proxy_protocol/proxy_protocol.test.py b/tests/gold_tests/proxy_protocol/proxy_protocol.test.py
index dfd07c920..f2440bfd6 100644
--- a/tests/gold_tests/proxy_protocol/proxy_protocol.test.py
+++ b/tests/gold_tests/proxy_protocol/proxy_protocol.test.py
@@ -16,6 +16,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+import os
import sys
Test.Summary = 'Test PROXY Protocol'
@@ -49,13 +50,25 @@ class ProxyProtocolTest:
self.ts.Disk.records_config.update({
"proxy.config.http.server_ports": f"{self.ts.Variables.port}:pp {self.ts.Variables.ssl_port}:ssl:pp",
"proxy.config.http.proxy_protocol_allowlist": "127.0.0.1",
- "proxy.config.http.insert_forwarded": "for|proto",
+ "proxy.config.http.insert_forwarded": "for|by=ip|proto",
"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.diags.debug.enabled": 1,
"proxy.config.diags.debug.tags": "proxyprotocol",
})
+ self.ts.Disk.logging_yaml.AddLines(
+ '''
+logging:
+ formats:
+ - name: access
+ format: '%<chi> %<pps>'
+
+ logs:
+ - filename: access
+ format: access
+'''.split("\n"))
+
def addTestCase0(self):
"""
Incoming PROXY Protocol v1 on TCP port
@@ -82,9 +95,36 @@ class ProxyProtocolTest:
tr.StillRunningAfter = self.httpbin
tr.StillRunningAfter = self.ts
+ def addTestCase2(self):
+ """
+ Test with netcat
+ """
+ tr = Test.AddTestRun()
+ tr.Processes.Default.Command = f"echo 'PROXY TCP4 198.51.100.1 198.51.100.2 51137 80\r\nGET /get HTTP/1.1\r\nHost: 127.0.0.1:80\r\n' | nc localhost {self.ts.Variables.port}"
+ tr.Processes.Default.ReturnCode = 0
+ tr.Processes.Default.Streams.stdout = "gold/test_case_2_stdout.gold"
+ tr.StillRunningAfter = self.httpbin
+ tr.StillRunningAfter = self.ts
+
+ def addTestCase99(self):
+ """
+ check access log
+ """
+ Test.Disk.File(os.path.join(self.ts.Variables.LOGDIR, 'access.log'), exists=True, content='gold/access.gold')
+
+ # Wait for log file to appear, then wait one extra second to make sure TS is done writing it.
+ tr = Test.AddTestRun()
+ tr.Processes.Default.Command = (
+ os.path.join(Test.Variables.AtsTestToolsDir, 'condwait') + ' 60 1 -f ' +
+ os.path.join(self.ts.Variables.LOGDIR, 'access.log')
+ )
+ tr.Processes.Default.ReturnCode = 0
+
def run(self):
self.addTestCase0()
self.addTestCase1()
+ self.addTestCase2()
+ self.addTestCase99()
ProxyProtocolTest().run()