PROXY Protocol transformed to Forwarded HTTP header. #3123

reveller · 2018-02-14T22:04:03Z

This update automatically detects a PROXY protocol header that precludes either http or https incoming requests. If the header is available, it is parsed and used internally by ATS as the client IP. If the proxy.config.http.insert_forwarded if configured with the 'for:' field, the Forwarded: HTTP header sent to the origin servers will be populated with the client IP passed through the PROXY protocol header. This will facilitate logging of the proper client IP versus load balancer IP. In addition, the origin servers can now use this IP address for tasks such as authz and authn.

mlibbey · 2018-02-15T05:07:12Z

FWIW, https://httpoxy.org/ describes a vulnerability with origins around the Proxy header. As a result, in our setup, we always remove the Proxy: header

shukitchan · 2018-02-15T08:09:17Z

Proxy protocol and the Proxy header is two different thing.
This is a really long-awaited feature. Thanks.

This should solve #1985

shinrich · 2018-02-22T14:59:51Z

proxy/ProxyProtocol.cc

@@ -0,0 +1,149 @@
+/** @file


Do you mean to put ProxyProtocol.cc in the tree twice? Both in proxy and iocore/net.

I had moved the ProxyProtocol.cc from proxy/ into iocore/net, then rebased and sqquash my commits, but somehow it is still in both places. I will fix it now.

shinrich · 2018-02-22T15:08:31Z

proxy/http/HttpTransact.h

      }
      internal_msg_buffer_size = 0;
    }
+


I am unclear on why you need to keep a copy of the proxy protocol information in the HttpState object. Is this for future growth. I don't see where this PR makes use of that data.

The Proxy Protocol info is detected, parsed and stashed into the NetVC object. It is then copied into the State object in HttpTransact::initialize_state_variables_from_request() so that it can be accessed in HttpTransactHeaders::add_forwarded_field_to_request to write the Forwarded: header. Somehow, the changes in HttpTransactHeaders.cc didn't get committed into this PR. Will fix this.

Sorry, I had forgotten about a late change in the code. Instead of waiting until it's time to write the headers out, I use the address received in the Proxy Protocol info and set it as the remote address in NetVConnection::get_remote_addr(). This way all internal references to the remote address use the info from Proxy Protocol.
I do store all of the data received in the Proxy Protocol header, and hope that someday, we can use that data in further enhancements, like a full Proxy Protocol implementation.

shinrich · 2018-02-22T15:10:46Z

Generally looks reasonable beyond my two comments above. Would be good to have an autest to exercise this code.

zwoop · 2018-03-03T03:23:49Z

[approve ci autest]

reveller · 2018-03-05T21:38:31Z

[approve ci centos]

bryancall · 2018-03-05T22:50:49Z

[approve ci debian]

zwoop · 2018-04-03T20:39:10Z

iocore/net/Makefile.am

  $(top_builddir)/lib/ts/libtsutil.la \
  $(top_builddir)/iocore/eventsystem/libinkevent.a \
-  $(top_builddir)/proxy/ParentSelectionStrategy.o
+  $(top_builddir)/proxy/ParentSelectionStrategy.o 


Why is this additional white space needed ?

Removed the extranseous space

zwoop · 2018-04-03T20:41:59Z

iocore/net/I_NetVConnection.h


+  enum ProxyProtocolVersion_t {
+    PROXY_VERSION_UNDEFINED,
+    PROXY_V1,


PROXY_ here seems a little ambiguous in this flat name space. Can we make the symbols e.g. PROXY_PROTO_VERSION_UNDEF, PROXY_PROTO_VERSION_1 and PROXY_PROTO_VERSION_2 ? Similar to your name spacing further below (in ProxyProtocolData_t).

That, or make ProxyProtocolVersion an enum class and change the names to UNDEFINED and V1.

zwoop · 2018-04-03T20:46:58Z

iocore/net/I_NetVConnection.h

+  int
+  set_proxy_protocol_addr(ProxyProtocolData_t src_or_dst, ts::string_view &ip_port_str)
+  {
+    int ret = -1;


Nitpicking, but hell, it's what I do. I much prefer an empty line between these local scope variables, and the code ;-).

zwoop · 2018-04-03T20:56:38Z

iocore/net/I_NetVConnection.h

+
+  enum ProxyProtocolData_t {
+    PROXY_PROTO_UNDEFINED,
+    PROXY_PROTO_SRC,


Hmmm, this looked a little odd to me when reading the code. You are really treating this like a boolean in (almost?) all places where it's used? Why not just get rid of this, and have a boolean parameter where needed, like proxo_proto_is_src or some such.

I don't care particularly strongly on it though.

I am converting both of these enums to enum classes, and reducing the names to UNDEFINED, VI, etc. As for the ProxyProtocolData enum, I use this as a sort of switch for the get_ and set_proxy_protocol_addr(). E.g. the intent is to call set_proxy_protocol_src_addr() which sets the SRC or DST flag before calling set_proxy_protocol_src_addr(). If this doesn't work for you guys, I can change it to a simple bool. I was just looking for something I could expand on down the road.

zwoop · 2018-04-03T20:57:20Z

iocore/net/P_NetVConnection.h

+TS_INLINE sockaddr const *
+NetVConnection::get_proxy_protocol_addr(ProxyProtocolData_t src_or_dst)
+{
+  if (src_or_dst == PROXY_PROTO_SRC) {


Like, here you are treating this really like a boolean, i.e. you don't check any other possible values of src_or_dst).

zwoop · 2018-04-03T20:57:48Z

iocore/net/I_NetVConnection.h

+  set_proxy_protocol_addr(ProxyProtocolData_t src_or_dst, ts::string_view &ip_port_str)
+  {
+    int ret = -1;
+    if (src_or_dst == PROXY_PROTO_SRC) {


Same here, treating it as a boolean, not checking any other values in the enum).

zwoop · 2018-04-03T21:00:58Z

iocore/net/ProxyProtocol.cc

+bool
+proxy_protov1_parse(NetVConnection *netvc, char *buf)
+{
+  std::string src_addr_port;


Not convinced this is a good use of std::string. Is this in the critical path? I.e. if this happens on every request, makes this a sufficiently large char[] on the stack, and use that. You can use snprintf() into that safely (and I assume the size of these port strings are of known, limited size.

I do feel strongly about this on the critical path, where we avoid using std::string intentionally. If it's not in the critical path, e.g. in some configuration code that only happens infrequent, it's fine.

Heh. This is a great place for TextView. Let me rip out some code for it.

I got a bit carried away, but here a suggested version of ProxyProtocol.cc which addresses @zwoop 's concern. This doesn't update ssl_has_proxy_v1 but the same sort of changes would be reasonable (e.g., not copying out of the IOBuffer twice).

I'm implementing amc's TextView with some minor modifications.

SolidWallOfCode · 2018-04-08T18:29:36Z

iocore/net/ProxyProtocol.h

+const char *const PROXY_V2_CONNECTION_PREFACE = "\x0D\x0A\x0D\x0A\x00\x0D\x0A\x51\x55\x49\x54\x0A\x02";
+
+const size_t PROXY_V1_CONNECTION_PREFACE_LEN = strlen(PROXY_V1_CONNECTION_PREFACE); // 5
+const size_t PROXY_V2_CONNECTION_PREFACE_LEN = strlen(PROXY_V2_CONNECTION_PREFACE); // 13


I think this will fail, because there is a nul byte in the source string.

Hard coded the value for V2 since it is part of the "spec".

SolidWallOfCode · 2018-04-13T13:07:12Z

iocore/net/I_NetVConnection.h

+
+  typedef struct _ProxyProtocol {
+    ProxyProtocolVersion proxy_protocol_version = ProxyProtocolVersion::UNDEFINED;
+    uint16_t ip_family;


If @zwoop is going to nitpick, I'll note these should be in_port_t.

A more serious question - you have the port values broken out here, but later you have things like

(ats_is_ip(&pp_info.src_addr) && ats_ip_port_cast(&pp_info.src_addr)

which treats the port as being in the IpEndpoint. Why have src_port and dst_port at all? Are these values different from the port in the IpEndpoint?

SolidWallOfCode · 2018-04-13T13:12:44Z

iocore/net/P_NetVConnection.h

+NetVConnection::get_proxy_protocol_addr(ProxyProtocolData_t src_or_dst)
+{
+  if (src_or_dst == PROXY_PROTO_SRC) {
+    if ((ats_is_ip(&pp_info.src_addr) && ats_ip_port_cast(&pp_info.src_addr)) ||


Becuause src_addr is an IpEndpoint, this can be done as

if ((pp_info.src_addr.isValid() && pp_info.src_addr.port() != 0) ||

New submission

This allows make -j to build all of these things much efficiently, since it's seen as a single build unit. This is similar to how we already build the plugins/ directory. (cherry picked from commit 09e25db)

(cherry picked from commit 882faa0)

(cherry picked from commit 3b07f26)

… new/delete" This reverts commit 79682c4. (cherry picked from commit 3430a69)

(cherry picked from commit d859302)

(cherry picked from commit 77882a6)

(cherry picked from commit 5a759ed)

(cherry picked from commit 652f821)

This was broke by: 4e62375 Remove Congestion Control Feature (cherry picked from commit ff85139)

This change imports yaml-cpp 0.6.2 (into lib/yamlcpp) NOTE: I've removed gtest from it's tree (cherry picked from commit 96592e5)

(cherry picked from commit aa908d1)

(cherry picked from commit b0c1653)

…onversion This is still a WIP (cherry picked from commit 1c189cd)

Fixes coredump when testing in production (cherry picked from commit 77be135)

(cherry picked from commit dff7ded)

(cherry picked from commit aafd3a2)

(cherry picked from commit 7fb12c0)

(cherry picked from commit 8ff51f8)

(cherry picked from commit c9d6099)

calavera · 2018-06-19T20:39:32Z

@reveller is there any progress on this? I'd really like to give it a try.

…trafficserver into proxy-protocol-submit

SolidWallOfCode · 2018-07-13T13:58:22Z

@calavera I think this one is dead, check out #3958.

reveller · 2018-07-16T17:57:33Z

@calavera and @zonestc, please see new PRs for this functionality:

#3958
#3956

I am closing this old PR.

zwoop · 2018-07-28T19:15:05Z

Please remember to remove Milestone and Projects etc. when you close a PR (and not merging it).

reveller added the Core label Feb 14, 2018

reveller added this to the 8.0.0 milestone Feb 14, 2018

reveller self-assigned this Feb 14, 2018

reveller force-pushed the proxy-protocol-submit branch 3 times, most recently from 4693d4a to ba03be0 Compare February 15, 2018 02:15

shinrich reviewed Feb 22, 2018

View reviewed changes

reveller force-pushed the proxy-protocol-submit branch 3 times, most recently from 5c540eb to a9fd370 Compare February 22, 2018 23:30

apache deleted a comment from reveller Mar 3, 2018

shinrich previously approved these changes Mar 12, 2018

View reviewed changes

zwoop previously requested changes Apr 3, 2018

View reviewed changes

SolidWallOfCode reviewed Apr 8, 2018

View reviewed changes

reveller dismissed shinrich’s stale review via 84e089d April 12, 2018 20:40

SolidWallOfCode reviewed Apr 13, 2018

View reviewed changes

SolidWallOfCode changed the title ~~PROXY Protocol transformed to Forwarded HTTP heder.~~ PROXY Protocol transformed to Forwarded HTTP header. Apr 13, 2018

reveller force-pushed the proxy-protocol-submit branch 2 times, most recently from fba3b8c to a028fc4 Compare April 20, 2018 21:09

reveller force-pushed the proxy-protocol-submit branch from a028fc4 to f2e4d72 Compare April 20, 2018 23:43

zwoop and others added 21 commits June 12, 2018 14:14

Collapses the cmd/ build into one single build unit

5ff5fad

This allows make -j to build all of these things much efficiently, since it's seen as a single build unit. This is similar to how we already build the plugins/ directory. (cherry picked from commit 09e25db)

fix broken out of tree build

a4be8f9

(cherry picked from commit 882faa0)

set the LDFLAGS when jemalloc is being used

083f7ff

(cherry picked from commit 3b07f26)

Revert "force the binary to link with libstdc++ so it uses the proper…

4dc34b9

… new/delete" This reverts commit 79682c4. (cherry picked from commit 3430a69)

Renames the cmd/ tree to src/

408aaa2

(cherry picked from commit d859302)

Fix off-by-one access in header_rewrite config parser

8f232c4

(cherry picked from commit 77882a6)

Clear server_entry in case of ERROR as well as NO_ACTIVITY_TIMEOUT

884e888

(cherry picked from commit 5a759ed)

redirection_tries should no be incremented twice

17f8f79

(cherry picked from commit 652f821)

Fixed coredump with origin throttling

c677d67

This was broke by: 4e62375 Remove Congestion Control Feature (cherry picked from commit ff85139)

Importing yaml-cpp into tree

39fedef

This change imports yaml-cpp 0.6.2 (into lib/yamlcpp) NOTE: I've removed gtest from it's tree (cherry picked from commit 96592e5)

Converts logging.config and ssl_server_name.config to YAML-based files

6b45069

(cherry picked from commit aa908d1)

Removes luajit lib and custom Lua bindings

9e4eacb

(cherry picked from commit b0c1653)

doc: Updates for the logging.config and ssl_server_name.config yaml c…

2e8724d

…onversion This is still a WIP (cherry picked from commit 1c189cd)

Handle inactivity timeout event in state_http_server_open

e0a236c

Fixes coredump when testing in production (cherry picked from commit 77be135)

Removes the TSUrlDestroy API, which has been a no-op for a long time

145eeda

(cherry picked from commit dff7ded)

Removes the old TSRedirectUrlGet API, deprecated

9e0cff8

(cherry picked from commit aafd3a2)

set verify locations in the initializer function only

14dbccc

(cherry picked from commit 7fb12c0)

Handle various kinds of errors in cache scan

567e227

(cherry picked from commit 8ff51f8)

Update installation instruction for lua plugin

6f3a600

(cherry picked from commit c9d6099)

Proxy Protocol progress check in before merging to syng with 8.0.x

8eafb04

PROXY Protocol transformed to Forwarded HTTP heder.

21850da

sfeltner-godaddy added 3 commits July 2, 2018 15:06

Proxy Protocol check point

7da5f2c

Proxy Protocol check point merge

69f5343

Merge branch 'proxy-protocol-port-descriptor' of github.com:reveller/…

e1091aa

…trafficserver into proxy-protocol-submit

reveller closed this Jul 16, 2018

zwoop removed this from the 9.0.0 milestone Jul 31, 2018

PROXY Protocol transformed to Forwarded HTTP header. #3123

PROXY Protocol transformed to Forwarded HTTP header. #3123

Uh oh!

Conversation

reveller commented Feb 14, 2018

Uh oh!

mlibbey commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shukitchan commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shinrich commented Feb 22, 2018

Uh oh!

zwoop commented Mar 3, 2018

Uh oh!

reveller commented Mar 5, 2018

Uh oh!

bryancall commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zwoop Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calavera commented Jun 19, 2018

Uh oh!

SolidWallOfCode commented Jul 13, 2018

Uh oh!

reveller commented Jul 16, 2018

Uh oh!

zwoop commented Jul 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlibbey commented Feb 15, 2018 •

edited

Loading

zwoop Apr 3, 2018 •

edited

Loading

SolidWallOfCode Apr 9, 2018 •

edited

Loading

zwoop commented Jul 28, 2018 •

edited

Loading