Bug 57830 - Add support for ProxyProtocol
Summary: Add support for ProxyProtocol
Status: NEW
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Connectors (show other bugs)
Version: 8.0.21
Hardware: PC Mac OS X 10.1
: P2 enhancement with 30 votes (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-18 19:58 UTC by Frank Kline
Modified: 2023-01-23 08:28 UTC (History)
5 users (show)



Attachments
SVN patch, source code, and jar file (693.73 KB, application/octet-stream)
2016-04-07 21:15 UTC, kycrow32
Details
Source and JAR for proxy protocol support for 8.5.23 (774.83 KB, application/x-zip-compressed)
2017-11-17 14:21 UTC, kycrow32
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Frank Kline 2015-04-18 19:58:50 UTC
When using TCP load balancing to avoid terminating HTTPS at a load balancer, there are few options for passing client information to the backend server from the load balancer.

AWS uses ProxyProtocol (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) as the mechanism to pass client information to servers behind a load balancer. http://docs.aws.amazon.com/ElasticLoadBalancing/latest/DeveloperGuide/enable-proxy-protocol.html

When ProxyProtocol is enabled for TCP load balancing over HTTPS, Tomcat cannot serve the request.
Comment 1 Mark Thomas 2015-04-19 07:52:29 UTC
Moving this to an enhancement request.

I can see the benefit of this but is would be non-trivial to implement - particularly for HTTPS.

For NIO and NIO2 (and much like SNI support) it would require the server to process some data before passing it to the SSL engine. Unlike SNI, the data would not be left in the bufffer for the SSL engine to process.

APR/native needs further investigation to determine what changes would be required to implement this.
Comment 2 Christopher Schultz 2015-04-19 13:17:57 UTC
+1 to adding this enhancement.

Without this feature, AWS ELB won't send anything about an ELB-terminated TLS connection through to the server except for the protocol (e.g. TLSv1) and the remote client's IP address. That means you can't do client certificates, further-validate cipher strength, etc.

As for the difference between the recently-added SNI code, is it not possible to read a block of data and then discard some of it (after taking-out the relevant data, of course) before passing it down the chain to the SSL handshake processor?
Comment 3 Bill Barker 2015-04-19 21:48:58 UTC
(In reply to Mark Thomas from comment #1)
> Moving this to an enhancement request.
> 
> I can see the benefit of this but is would be non-trivial to implement -
> particularly for HTTPS.
> 
> For NIO and NIO2 (and much like SNI support) it would require the server to
> process some data before passing it to the SSL engine. Unlike SNI, the data
> would not be left in the bufffer for the SSL engine to process.
> 
> APR/native needs further investigation to determine what changes would be
> required to implement this.

My reading of the ProxyProtocol spec linked above states that it is agnostic to the transport protocol between the proxy server and the receiver. Otherwise it can't work if you are using SSH tunneling which is one of the target configurations. That means that the "PROXY ..." line is encrypted over SSL/TSL just like everything else in the payload.  This in turn means that all of the code that is required to support this comes at the point of reading the initial request line and no special handling is required for SSL/TSL.

It would be a huge security hole if the proxy was allowed to inject a plain text (for version 1) payload in front of the encrypted payload. Both of them would have to be encrypted if you are using an encrypted connection between the proxy and the receiver.
Comment 4 Christopher Schultz 2015-04-20 13:55:37 UTC
(In reply to Bill Barker from comment #3)
> That means that the "PROXY ..." line is encrypted
> over SSL/TSL just like everything else in the payload.  This in turn means
> that all of the code that is required to support this comes at the point of
> reading the initial request line and no special handling is required for
> SSL/TSL.

That's not my reading of this spec. The proxy prepends the header information to the front of whatever the client is sending. The fact that the original connection is encrypted is irrelevant. The PROXY protocol allows anything to be tunneled, including non-encrypted configurations.

> It would be a huge security hole if the proxy was allowed to inject a plain
> text (for version 1) payload in front of the encrypted payload.

Why?

> Both of them would have to be encrypted if you are using an encrypted
> connection between the proxy and the receiver.

There is no requirement to encrypt the traffic between the proxy and the receiver.
Comment 5 Bill Barker 2015-04-21 07:09:02 UTC
Ok, so I miss read the spec.  After reading the spec again, I have lost all interest in this issue.
Comment 6 Christopher Schultz 2015-04-27 16:02:47 UTC
(In reply to Bill Barker from comment #5)
> Ok, so I miss read the spec.  After reading the spec again, I have lost all
> interest in this issue.

I'm curious: does this simply not interest you, or do you actively think this is a bad idea, in general, or a bad idea to implement in Tomcat?
Comment 7 Bill Barker 2015-04-28 04:10:46 UTC
(In reply to Christopher Schultz from comment #6)
> (In reply to Bill Barker from comment #5)
> > Ok, so I miss read the spec.  After reading the spec again, I have lost all
> > interest in this issue.
> 
> I'm curious: does this simply not interest you, or do you actively think
> this is a bad idea, in general, or a bad idea to implement in Tomcat?

It just doesn't interest me to support such low-grade proxy servers. Other then that they are popular on a certain well-known hosting site, it doesn't seem worth my effort. 

If I thought it was a bad idea then I would have expressed a vote. But if this is an itch that somebody wants to scratch, by all means scratch away :).

I haven't been able to see how to theoretically break this if it is implemented properly according to the spec, as long as you also have a competent network admin (which presumably applies to the above-mentioned well-known hosting site). If I can sit as man-in-the-middle before the proxy and inject my own "PROXY ..." line, then Tomcat gets two such lines (assuming the proxy is working properly) and mine is rejected just like Tomcat does now.
Comment 8 Axel Fontaine 2016-02-22 18:07:44 UTC
+1
Comment 9 Christopher Schultz 2016-02-22 23:49:52 UTC
I think Daniel Ruggeri did some work on this. He's been waiting for some feedback from me. Maybe I should get on that!
Comment 10 Christopher Schultz 2016-02-23 17:51:45 UTC
Whoops, sorry. I confused this with a similar httpd enhancement request. Please ignore comment #9 (and this one, too).
Comment 11 kycrow32 2016-04-07 21:15:01 UTC
Created attachment 33738 [details]
SVN patch, source code, and jar file

+1

Also....

I've attached a zip file containing:
tomcat-coyote-pp-src.zip (a maven project extending tomcat-coyote.jar)
tomcat-coyote-pp.patch   (an SVN patch for tomcat-coyote's source)
tomcat-coyote-pp.jar     (the jar built from tomcat-coyote-pp-src)

This extension is based on tomcat-coyote.jar version 8.0.32, but applying the changes to 8.0.33 works as well.  It enables support for human readable Proxy Protocol (according to the spec at http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) in the Http11NioProtocol connector.

Simply add 'proxyProtocol="off|on|optional"' to server.xml, eg:
<Connector port="8443" protocol="org.apache.coyote.http11.Http11NioProtocol" proxyProtocol="optional" />

This will cause the connector to add two attributes to an incoming request:
"ProxyProtocolInfo" -> org.apache.tomcat.util.net.ProxyProtocolInfo   a class that contains the source and destination addresses and ports extracted from the proxy protocol header
"ProxyProtocolSourceAddress" -> String containing the source IP address



I've tested this using our own deployment+webapps+integration tests, and it appears to work.  I'd love to see this (or any other support) added to tomcat.  If anyone on the apache team wants to look at this, I'm receptive to feedback, criticism, further instructions, and hate mail.

This is an extension to tomcat code, so it is licensed under the Apache License v2.0.  If you feel like using this jar provided by "some guy on the internet", feel free :)
Comment 12 Christopher Schultz 2016-04-07 21:40:04 UTC
Awesome. I should be able to do some testing using Amazon ELB, which I expect was the impetus for this work.
Comment 13 Christopher Schultz 2016-08-31 16:02:36 UTC
Since I'm working with Daniel Ruggeri @ httpd, I've got an AWS lb set up for testing. I should be able to apply your patch and test it... soon. Sorry for the delay on this. Lots to do at $work.
Comment 14 Igor Cicimov 2016-12-28 04:22:20 UTC
Hi,

Just wonder about the status on this?

Thanks,
Igor
Comment 15 Christopher Schultz 2016-12-28 13:29:46 UTC
My work on this stalled while I worked with Dan @ httpd on his patches. Ironically, as he was trying to merge his patch set, he discovered that someone else had built the same capabilities and committed them a few hours earlier :(

So I'm going to get back involved, here, first working with httpd and then it should be easy to validate that the Tomcat patch is working as well.
Comment 16 Axel U 2017-03-08 03:46:35 UTC
Is there an update on adding PROXY protocol support?
Comment 17 Rob 2017-04-11 20:41:54 UTC
Would it make sense to create a new protocol object either containing or derived from Http11NioProtocol (e.g. ProxyHttp11NioProtocol)?  That would avoid put the proxy protocol code in a class by itself.  I have no idea if this would, work for all configurations.
Comment 18 Christopher Schultz 2017-04-12 16:20:11 UTC
The PROXY protocol should be "easy" to roll into an existing class: just have a look at the first few bytes of a request to see if it's got a valid "PROXY" command in there. If so, validate the source IP (the proxy), set all of the source IP information (the client's actual IP, port, etc.), and consume the "PROXY" command so that the following code just sees the raw request.

I'm not familiar enough with the channel-unwrapping internals of Tomcat to know how best to implement it in the current pipeline. The best I would be able to do is validate that the patch actually works. Which I have not yet done :(
Comment 19 kycrow32 2017-11-17 14:21:53 UTC
Created attachment 35535 [details]
Source and JAR for proxy protocol support for 8.5.23

I've attached a new tomcat-coyote jar, with source code, that supports proxy protocol in tomcat 8.5.23.  Test/use/ignore as you like.

(And yes, this work was done for support with AWS ELB.)
Comment 20 Emanuel 2019-06-30 11:20:23 UTC
hi, are there any plans to commit these changes into new tomcat releases?
Comment 21 Mark Thomas 2019-06-30 20:14:07 UTC
In its current form? Unlikely. A quick look identifies several issues:

- The patch no longer applies cleanly. It needs to be updated to work with the latest 9.0.x code.
- The optional configuration is inherently insecure. I'd consider vetoing a commit that implemented this.
- The IP address parsing is not robust. Tomcat provides robust parsers in org.apache.tomcat.util.http.parser
- This looks to be significantly more complex to implement for APR/Native. That may not be an issue on the basis APR/Native may be dropped in Tomcat 10 onwards.
- No documentation
- I haven't looked at the overall design because the patch didn't apply. There may be additional concerns.

Generally, when there appears to be a low level of interest in an enhancement the closer a patch is to being able to be simply reviewed and committed without additional changes the more likely it is to be applied.
Comment 22 Tim 2021-06-30 04:32:04 UTC
Mark Thomas: Why do you object to the optional config?

I'm asking because we have our servers behind a load balancer but we often send test requests to individual servers. It would make sense to me to accept proxy protocol requests from the load balancer and also accept requests without the proxy protocol. This would seem to be the case that the optional config handles.
Comment 23 Mark Thomas 2021-06-30 09:15:54 UTC
(In reply to Tim from comment #22)
> Mark Thomas: Why do you object to the optional config?

Because it is insecure. It is for this reason that the PROXY spec explicitly states that "The receiver ... MUST not try to guess whether the protocol header is present or not."

Looking at a diff against the 8.5.23, I'm also wondering if the implementation has been made at the right point. That the broadly the same implementation would need to be repeated across multiple connectors suggests it should be better implemented further up the stack.
Comment 24 Tim 2021-06-30 21:00:54 UTC
I just re-read the spec and I see that the Proxy Protocol header must be required on every request.

I'm not convinced requiring the Proxy Protocol header on every request increases security, especially not in the narrow context of HTTP and HTTPS connections. As I understood the argument, requiring the header always be present would somehow prevent spoofing. I don't see any way it prevents a malicious actor from sending a request with a falsified header. I think the correct guard against malicious actors sending falsified headers is to limit the range of IP's that can send requests with the Proxy Protocol header.

In the context of supporting Proxy Protocol in Tomcat, I wouldn't want an implementation unless it provided the ability to limit the allowed senders of the Proxy Protocol header with an allow list that specified either a list of allowed IP addresses or one or more CIDRs. It is probably better to support the spec as written, but I would support an 'optional' argument to autodetect presence of the Proxy Protocol header if it didn't meet fierce resistance from the Tomcat team and if it rejected requests with a Proxy Protocol header that did not come from an address in the allow list.

I think logs should show both the "claimed" IP as well as the actual IP a request came from. This is the approach I've taken when processing X-Request-For headers; log both the source of tcp connectoin and the X-Request-For address so anybody investigating a breach can verify if the request actually came from an authorized load balancer.

Lastly, I think the implementation should make the "claimed" IP and port from the Proxy Protocol header available in the X-Forwarded-For and X-Forwarded-Port headers when handing the request off to the Servlet. This would preserve the data necessary to log both the "claimed" ip and the actual IP. In the tomcat access log, I think the %a and %h flags should print both the actual source of the connection (e.g. load balancer), and the "claimed" IP and port.

I'm not strongly invested in any of my positions I layed out here. I'm just trying to get a better handle on what sort of patch would be acceptable.