Skip to content

Conversation

@reveller
Copy link
Contributor

This update adds a port descriptor option for enabling Proxy Protocol as well as a whitelist of trusted IP addresses as sources of incoming requests with Proxy Protocol headers. If the the Proxy Protocol is enabled and the PROXY header is prefaced on an incoming request, the remote address is changed internally to reflect the original client IP. This has the side effect of transforming the originating IP address into the Forwarded: header for: field, if proxy.config.http.insert_forwaded is configured with the for: field. This will also facilitate logging of the proper client IP versus the previous hop address, i.e. load balancer.

@SolidWallOfCode
Copy link
Member

I'm working with the code and one thing I wonder is, why is the proxy protocol map in HttpConfig? Why not have it only in SSLConfig in the same way as cipherSuite or clientCertLevel? Those are read directly out of records.config by SSLConfig. That would seem to simply the issue with reaching in to HttpConfig.

@SolidWallOfCode
Copy link
Member

A bit more work. Here's a diff that seems to work to have HttpConfig call back to the SSL component with the address of the global configuation value. After that I have a back trace from gdb demonstrating that it worked as expected. Overall, though, I think I'd pull the IpMap out of HttpConfig and have it a static member of SSLConfigParams, loaded along with other values from records.config in SSLConfigParams::initialize.

diff --git a/iocore/net/SSLConfig.cc b/iocore/net/SSLConfig.cc
index c7f6236fa..52bac7406 100644
--- a/iocore/net/SSLConfig.cc
+++ b/iocore/net/SSLConfig.cc
@@ -90,6 +90,10 @@ SSLConfigParams::~SSLConfigParams()
   ink_mutex_destroy(&ctxMapLock);
 }
 
+void SSLConfigCoreInit(IpMap* map) {
+  SSLConfigParams::proxy_protocol_ipmap = map;
+}
+
 void
 SSLConfigParams::reset()
 {
@@ -360,7 +364,7 @@ SSLConfigParams::initialize()
   }
 
   // Grab a reference to the Proxy Protocol IpMap whitelist
-  proxy_protocol_ipmap = &HttpConfig::m_master.config_proxy_protocol_ipmap;
+//  proxy_protocol_ipmap = &HttpConfig::m_master.config_proxy_protocol_ipmap;
 
   // Enable client regardless of config file settings as remap file
   // can cause HTTP layer to connect using SSL. But only if SSL
diff --git a/proxy/http/HttpConfig.cc b/proxy/http/HttpConfig.cc
index 518af9c86..b44633f52 100644
--- a/proxy/http/HttpConfig.cc
+++ b/proxy/http/HttpConfig.cc
@@ -908,6 +908,7 @@ register_stat_callbacks()
 void
 HttpConfig::startup()
 {
+  extern void SSLConfigCoreInit(IpMap* map);
   http_rsb = RecAllocateRawStatBlock((int)http_stat_count);
   register_stat_callbacks();
 
@@ -926,6 +927,7 @@ HttpConfig::startup()
   RecHttpLoadIp("proxy.local.incoming_ip_to_bind", c.inbound_ip4, c.inbound_ip6);
   RecHttpLoadIp("proxy.local.outgoing_ip_to_bind", c.outbound_ip4, c.outbound_ip6);
   RecHttpLoadIpMap("proxy.config.http.proxy_protocol_whitelist", c.config_proxy_protocol_ipmap);
+  SSLConfigCoreInit(&c.config_proxy_protocol_ipmap);
 
   HttpEstablishStaticConfigLongLong(c.server_max_connections, "proxy.config.http.server_max_connections");
   HttpEstablishStaticConfigLongLong(c.max_websocket_connections, "proxy.config.http.websocket.max_number_of_connections");
Thread 1 "traffic_server" hit Breakpoint 1, SSLConfigCoreInit (map=0xb6ce60 <HttpConfig::m_master+160>) at SSLConfig.cc:94
94	  SSLConfigParams::proxy_protocol_ipmap = map;
Missing separate debuginfos, use: dnf debuginfo-install libcap-2.25-7.fc27.x86_64 libgcc-7.3.1-5.fc27.x86_64 libstdc++-7.3.1-5.fc27.x86_64 openssl-libs-1.1.0h-3.fc27.x86_64 pcre-8.42-1.fc27.x86_64 tcl-8.6.8-1.fc27.x86_64 xz-libs-5.2.3-4.fc27.x86_64 zlib-1.2.11-4.fc27.x86_64
(gdb) p map
$1 = (IpMap *) 0xb6ce60 <HttpConfig::m_master+160>
(gdb) bt
#0  SSLConfigCoreInit (map=0xb6ce60 <HttpConfig::m_master+160>) at SSLConfig.cc:94
#1  0x00000000005be368 in HttpConfig::startup () at HttpConfig.cc:930
#2  0x000000000059417b in main (argv=0x7fffffffdf98) at traffic_server/traffic_server.cc:1727
(gdb) p *map
$2 = {_m4 = 0x0, _m6 = 0x0}
(gdb) n
95	}
(gdb) p SSLConfigParams::proxy_protocol_ipmap 
$3 = (IpMap *) 0xb6ce60 <HttpConfig::m_master+160>

@reveller
Copy link
Contributor Author

@SolidWallOfCode This is not only for SSL/TLS requests. It was written to work for both HTTP and HTTPS. If we only want it to work for HTTP, we can just config it in the SSLConfigParams and be done with it.

I have a working callback already written where HttpConfig calls into an SSLConfigParams config_init function, but when HttpConfig calls to set the IpMap reference the SSLConfigParams doesn't exist and there is nothing for HttpConfig to write into.

@SolidWallOfCode
Copy link
Member

Ah.

However, I don't see the problem concerning the SSLConfigParams structure - the target is a static member and will exist by the time main is called. This can be seen in the debugging trace above where it was successfully assigned out of a callback from HttpConfig::startup.

@reveller reveller closed this Jul 16, 2018
@reveller reveller force-pushed the 9.0.x-proxy-protocol branch from e2814f7 to ef785f2 Compare July 16, 2018 16:13
@bryancall
Copy link
Contributor

bryancall commented Oct 9, 2018

@reveller Please fix the merge conflicts and make sure it is same commit as #3956

bryancall
bryancall previously approved these changes Oct 9, 2018
@bryancall
Copy link
Contributor

@reveller Please fix the merge conflicts and make sure it is same commit as #3956

@reveller reveller dismissed stale reviews from bryancall and SolidWallOfCode via 654f179 January 9, 2019 03:00
@reveller reveller force-pushed the 9.0.x-proxy-protocol branch from 951bcb8 to 654f179 Compare January 9, 2019 03:00
@reveller reveller force-pushed the 9.0.x-proxy-protocol branch from 654f179 to a9ab891 Compare January 9, 2019 17:51
@reveller
Copy link
Contributor Author

reveller commented Jan 9, 2019

[approve ci autest]

@bryancall bryancall merged commit adf3299 into apache:master Jan 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants