Add optional support for the PROXY protocol

[Jonty]

This adds support for the commonly implemented PROXY protocol, allowing TCP proxies to pass along upstream client information. When this is enabled gorouter will read the PROXY preamble and inject the upstream information into the X-Forwarded-For header.

http://blog.haproxy.com/haproxy/proxy-protocol/

It should be noted that when using PROXY on the HTTPS port the X-Forwarded-Proto header will not be set to "https" as expected because the X-Forwarded-Proto code checks for source.TLS, which is only set by the go http library if the conn is a tls.conn:

https://golang.org/src/net/http/server.go#L1398

This can only be fixed properly by patching the standard library. We plan to submit a separate gorouter patch to allow operators to override the autodetected X-Forwarded-Proto if they are terminating SSL at the load balancer before gorouter and using PROXY to communicate with gorouter.

[cfdreddbot]

Hey Jonty!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/116690921.

[Jonty]

This should close #125

[Jonty]

The build is failing due to Travis being slow and tests timing out, they pass when run locally.

[shalako]

It should be noted that when using PROXY on the HTTPS port the X-Forwarded-Proto header will not be set to "https" as expected

What is the behavior when using PROXY on the HTTP port? Your use case is an insecure private network. Since your SSL terminator is sending requests to port 80, would Gorouter detect the value of X-Forwarded-Proto using this PROXY protocol? If so, it seems like your "Force Proto" solution isn't necessary.

[Jonty]

@shalako When the inbound connection is on port 80 the X-Forwarded-Proto header will be set to http unless the header is already set, in which case gorouter will leave it unmodified.

The PROXY protocol does not communicate the incoming protocol used, and the upstream proxy itself is not HTTP-aware so will not insert the X-Forwarded-Proto header.

The PROXY protocol does communicate the incoming port number, so it would be possible to automatically set X-Forwarded-Proto to https if the upstream port is detected as being the same as SSLPort, but unfortunately go makes it extremely difficult to get the local address in an HTTP handler, so implementing this would be extremely intrusive.

[Jonty]

Yesterday upstream golang gained support for accessing the local address from a http handler, which would make my previous suggestion substantially easier to implement. Obviously this won't be possible until there's a new golang release though.

[keymon]

As commented here what is the decision for this PR?

Shall we go for the approach of an updated golang release which would allow implement logic to check if the upstream port the same as SSLPort?

[shalako]

@keymon @Jonty

Please confirm my understanding of the alternative you're suggesting

• requires support for PROXY protocol
• requires gorouter upgrade to newer version of golang (which version?)
• upstream TLS-terminating component does not support sending X-Forwarded-Proto http header, but can send the port it received the request on
• If gorouter receives a request that includes the port an upstream component received the request on, identified using the PROXY protocol, and this port is 443, then gorouter will set X-Forwarded-Proto: https in the request to backend

[keymon]

requires support for PROXY protocol

Yes, in order to get the original connection information from the first proxy (e.g. ELB in TCP mode)

requires gorouter upgrade to newer version of golang (which version?)

Yes, it is required to be able to check the port as described in @Jonty comment.

But as @Jonty pointed, it has been added and reviewed here, and you can see the github commit here. But it seems that the code has not been released yet.

upstream TLS-terminating component does not support sending X-Forwarded-Proto http header, but can send the port it received the request on

Yes, they send the port as part of the proxy protocol

If gorouter receives a request that includes the port an upstream component received the request on, identified using the PROXY protocol, and this port is 443, then gorouter will set X-Forwarded-Proto: https in the request to backend

exactly

[crhino]

@keymon @Jonty In terms of the X-Forwarded-Proto being set to https when the port is 443, I do not see how being able to find out the local address of the connection enables us to identify the upstream proxy was listening on port 443.

In the commit you mention, the context key is set to the l.Addr() value, which in the proxy protocol library you use in the PR will still return the local address of the gorouter, when one would want to check the destination port provided by the proxy header at the beginning of the connection. Let me know if I am missing something here.

Other than that, the PR looks fine code-wise.

@crhino && @flawedmatrix

[Jonty]

@crhino go-proxyproto wraps the connection and replaces the connection local port with the one communicated via the PROXY protocol, which will be 443 (PROXY communicates the protocol version, source port, destination port, and remote IP address).

[flawedmatrix]

@Jonty : Thank you for submitting PR. We were wondering if you could provide an integration test to verify the continued support for Proxy Protocol.

Regards
@shashwathi && @flawedmatrix

[Jonty]

@flawedmatrix @shashwathi Is the test we implemented not sufficient?

[flawedmatrix]

@Jonty :

We were thinking more along the lines of an end-to-end test. After looking at the PR again, the given tests should do the job.
We are trying to deploy this on an AWS environment and verify it works as expected. Nothing more should be required from your end.

Regards
@shashwathi && @flawedmatrix

[shashwathi]

@Jonty :

We noticed an issue when we were testing this PR, gorouter does not seem accept multiple connections anymore.

Steps to reproduce this issue:

1. Enable proxy protocol on gorouter
2. Use telnet to establish a long lived connection to GOROUTER_IP:PORT
3. You can use curl or telnet to establish another connection to GOROUTER_IP:PORT. If you are using curl you will notice your request just hangs.

When you disable proxy protocol and repeat the above steps, you will see that the gorouter accepts multiple connections on its port.

We have a failing test case on a branch in gorouter to demonstrate the same. We think the problem might be in the proxyproto library, where listener which is a wrapper around http listener only accepts one connection at a time.

To solve this immediately, we will add a Known Limitations section in Gorouter README to warn the operators. In the meantime, could you please take a look at this?

Let us know if you have any questions.

Regards
@shashwathi

[bleach]

Hi @shashwathi, I'm one of @Jonty's colleagues.

This is definitely not intended behaviour & we'll take a look in the next few days.

[Jonty]

@bleach There is an open PR that fixes this, however no work has been done to get it upstream: armon/go-proxyproto#2

I would suggest that someone should take that and work with @armon to get it into master, rather than forking it.

[keymon]

I think that this issue happens only if:

• you enable proxy protocol
• and you get ONE connection which sends no data, which will block the RemoteAddr() call until some data is sent.

If you have a proxy sending Proxy Protocol prefixes in front of go-router it should not happen.

But it must be fixed, indeed.

[keymon]

As an update:

• Indeed the http.Serve gets block in RemoteAddr() call until the Proxy Protocol or at least 6 bytes are sent (len('PROXY ')).
• On Go 1.6 this does not happen, I guess they changed the http server so it does not call RemoteAddr() immediately with the TCP connection. (link to change needed)
• The branch Check for proxy prefix early, so RemoteAddr doesn't block Accept armon/go-proxyproto#2 does NOT really solve this issue. Actually makes it worse because it blocks on Accept.

[keymon]

I opened a new issue #141 to track this. We propose a solution there.