Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional support for the PROXY protocol #126

Merged
merged 1 commit into from
Jun 14, 2016

Conversation

Jonty
Copy link
Contributor

@Jonty Jonty commented Mar 31, 2016

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
Copy link

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
Copy link

cf-gitbot commented Mar 31, 2016

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
Copy link
Contributor Author

Jonty commented Mar 31, 2016

This should close #125

@Jonty Jonty force-pushed the add_proxy_protocol_support branch 4 times, most recently from 160bec3 to 62b127b Compare March 31, 2016 14:20
@Jonty
Copy link
Contributor Author

Jonty commented Mar 31, 2016

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

@Jonty Jonty force-pushed the add_proxy_protocol_support branch from 62b127b to cfed0a2 Compare April 4, 2016 15:52
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.
@Jonty Jonty force-pushed the add_proxy_protocol_support branch from cfed0a2 to 2c1f04d Compare April 5, 2016 16:13
@shalako
Copy link
Contributor

shalako commented Apr 26, 2016

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
Copy link
Contributor Author

Jonty commented Apr 26, 2016

@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
Copy link
Contributor Author

Jonty commented May 3, 2016

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
Copy link
Contributor

keymon commented May 4, 2016

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
Copy link
Contributor

shalako commented May 10, 2016

@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
Copy link
Contributor

keymon commented May 11, 2016

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
Copy link
Contributor

crhino commented May 23, 2016

@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
Copy link
Contributor Author

Jonty commented May 24, 2016

@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
Copy link
Contributor

flawedmatrix commented Jun 13, 2016

@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
Copy link
Contributor Author

Jonty commented Jun 13, 2016

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

@flawedmatrix
Copy link
Contributor

@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 shashwathi merged commit 2c1f04d into cloudfoundry:master Jun 14, 2016
@shashwathi
Copy link
Contributor

@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

@shashwathi shashwathi added invalid and removed invalid labels Jul 8, 2016
@bleach
Copy link

bleach commented Jul 12, 2016

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
Copy link
Contributor Author

Jonty commented Jul 12, 2016

@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
Copy link
Contributor

keymon commented Jul 12, 2016

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
Copy link
Contributor

keymon commented Jul 12, 2016

As an update:

@keymon
Copy link
Contributor

keymon commented Jul 13, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants