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.

Sign up for GitHub

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

Jump to bottom

Badly configured HttpConfiguration.securePort can lead to wrong port produced by ForwardedHeader #5417

Closed
stuban opened this issue Oct 9, 2020 · 2 comments · Fixed by #5419
Closed

Badly configured HttpConfiguration.securePort can lead to wrong port produced by ForwardedHeader #5417

stuban opened this issue Oct 9, 2020 · 2 comments · Fixed by #5419
Assignees
@joakime

Comments

@stuban
Copy link

stuban commented Oct 9, 2020

Jetty version
9.4.32

Java version
AdoptOpenJDK build 1.8.0_262-b10

OS type/version
Linux CentOS 7.8 Kernel 3.10.0-1062.el7.x86_64

Description
For us the changes related to #5224 or #5247 have some surprising regression with 9.4.32
Our reverse-proxy (haproxy) always sends a RFC7239 style forwarded header towards jetty.
This always worked fine with 9.4.31 but with 9.4.32 it suddenly adds the (wrong) port to the requestURL
A simple example just printing out request.getRequestURL()
Request forwarded from upstream haproxy:

GET /sdpVab/test-forwarded.jsp HTTP/1.1
user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:81.0) Gecko/20100101 Firefox/81.0
accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8
accept-language: en,en-US;q=0.95,de;q=0.89,fr-FR;q=0.84,nl;q=0.79,de-DE;q=0.74,en-GB;q=0.68,de-US;q=0.63,en-EN;q=0.58,fr;q=0.53,nl-NL;q=0.47,de-formal;q=0.42,en-CA;q=0.37,en-AU;q=0.32,es-ES;q=0.26,es;q=0.21,en-DE;q=0.16,eng-US;q=0.11,eng;q=0.05
accept-encoding: gzip, deflate, br
dnt: 1
upgrade-insecure-requests: 1
cache-control: max-age=0
te: trailers
host: web-144-237.ect-telecoms.de
cookie: JSESSIONID=oj47pppk3nxc1k1cyy156nqlx0;
forwarded: for=192.168.253.64;host=web-144-237.ect-telecoms.de;proto=https;proto-version=h2

Answer from 9.4.31:

HTTP/1.1 200 OK
Content-Type: text/html;charset=utf-8
Set-Cookie: JSESSIONID=nxeetp3wb734e9udszzebmvs0; Path=/sdpVab; Secure; HttpOnly
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Transfer-Encoding: chunked


<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
<title>Forward test</title>
</head>
<body>
<p>https://web-144-237.ect-telecoms.de/sdpVab/test-forwarded.jsp</p>
</body>
</html>

Answer from 9.4.32:

HTTP/1.1 200 OK
Content-Type: text/html;charset=utf-8
Set-Cookie: JSESSIONID=oj47pppk3nxc1k1cyy156nqlx0; Path=/sdpVab; Secure; HttpOnly
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Transfer-Encoding: chunked


<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
<title>Forward test</title>
</head>
<body>
<p>https://web-144-237.ect-telecoms.de:8443/sdpVab/test-forwarded.jsp</p>
</body>
</html>

Looks like the port 8443 is added in error here.
joakime added a commit that referenced this issue Oct 9, 2020
@joakime
Issue #5417 - Honoring implied ports on ForwardedRequestCustomizer be…
149f389 
…tter

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime linked a pull request Oct 9, 2020 that will close this issue
Better support for implied ports on ForwardedRequestCustomizer #5419
Merged
@joakime
Copy link
Contributor

joakime commented Oct 9, 2020 •
edited
Loading

Opened PR #5419

The port in the implied case, like yours, is being pulled from the HttpConfiguration you have.
The HttpConfiguration.securePort on your system is set to 8443.
If you change HttpConfiguration.securePort on your Jetty server side it to 443 it should operate properly.

The HttpConfiguration.securePort is the logical secure port for your server, it's not the one that the https/ssl/tls connector is listening on.
In your scenario, where you have a Proxy, with Forwarding headers, your server side HttpConfiguratoin.securePort should be the port of the proxy public facing side https port.

To say this differently, take this setup.

  • Browser : makes request for resource https://demo.machine.com/test/foo.jsp
  • Proxy : serving demo.machine.com and listening on port 443 with SSL/TLS
  • Server : handling requests on internal.machine.com on port 8443

The Server in this scenario, should have a HttpConfiguration.securePort set to 443 (the default for SSL/TLS) for the connector on port 8443.

If this was a bit more complex it might make more sense this way ...

  • Browser : makes request for resource https://demo.machine.com:7443/test/foo.jsp
  • Proxy : serving demo.machine.com and listening on port 7443 with SSL/TLS
  • Server : handling requests on internal.machine.com on port 9443

In this scenario, the browser is talking to demo.machine.com:7443 so all redirects and URLs it sees coming back from the server should be consistent to that expectation.

The forwarding headers from the Proxy to the Server would look like this ...

GET /test/foo.jsp HTTP/1.1
host: demo.machine.com:7443
forwarded: for=192.168.253.64;host=demo.machine.com:7443;proto=https

So that means the server's HttpConfiguration.securePort should be 7443 even though it's bound/listening on 9443

@joakime joakime self-assigned this Oct 9, 2020
joakime referenced this issue Oct 11, 2020
@joakime
Issue #5247 - Improve testing of ForwardedRequestCustomizer
ac42cbd 
+ Merge ProxyPass tests from CheckReverseProxyHeadersTest into
  ForwardedRequestCustomizerTest
+ Deleted CheckReverseProxyHeadersTest.java
+ Add more tests for ForcedHost configuration
+ Updated ForwardedRequestCustomizer to conform to expectations

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime changed the title wrong port in ForwardedHeader Badly configured HttpConfiguration.securePort can lead to wrong port produced by ForwardedHeader Oct 11, 2020
joakime added a commit that referenced this issue Oct 12, 2020
@joakime
Issue #5417 - Cleanup of PR from Review
c2266fd 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Oct 13, 2020
@joakime
Merge pull request #5419 from eclipse/jetty-9.4.x-5417-forwarded-prot…
c37c2c5 
…o-port

Issue #5417 - Honoring implied ports on ForwardedRequestCustomizer better
@gregw gregw mentioned this issue Oct 13, 2020
Merged
@gregw
Copy link
Contributor

gregw commented Oct 13, 2020

Fixed by #5419

@gregw gregw closed this as completed Oct 13, 2020
@jglick jglick mentioned this issue Oct 19, 2020
Closed
3 tasks
oleg-nenashev added a commit to jenkinsci/winstone that referenced this issue Oct 19, 2020
@oleg-nenashev
[JENKINS-63958] - Update Jetty from 9.4.32.v20200930 to 9.4.33.v20201019
d7833ca 
Picks up https://github.com/eclipse/jetty.project/releases/tag/jetty-9.4.33.v20201019 which includes a fix for jetty/jetty.project#5417 fix is out (PR: jetty/jetty.project#5419 )
This was referenced Oct 19, 2020
Merged
Merged
@joakime joakime mentioned this issue Oct 21, 2020
Closed
olamy pushed a commit to jenkinsci/winstone that referenced this issue Oct 23, 2020
@oleg-nenashev @olamy
[JENKINS-63958] - Update Jetty from 9.4.32.v20200930 to 9.4.33.v20201019
734bc77 
Picks up https://github.com/eclipse/jetty.project/releases/tag/jetty-9.4.33.v20201019 which includes a fix for jetty/jetty.project#5417 fix is out (PR: jetty/jetty.project#5419 )
olamy added a commit to jenkinsci/winstone that referenced this issue Oct 26, 2020
@oleg-nenashev @olamy
[JENKINS-63958] - Update Jetty from 9.4.32.v20200930 to 9.4.33.v20201…
a7e3258 
…020 (#124)

* [JENKINS-63958] - Update Jetty from 9.4.32.v20200930 to 9.4.33.v20201019

Picks up https://github.com/eclipse/jetty.project/releases/tag/jetty-9.4.33.v20201019 which includes a fix for jetty/jetty.project#5417 fix is out (PR: jetty/jetty.project#5419 )

* Update parent POM

* jetty version is 9.4.33.v20201020

Signed-off-by: olivier lamy <olamy@apache.org>

* parent 1.59 is broken argLine

Signed-off-by: olivier lamy <olamy@apache.org>

Co-authored-by: olivier lamy <olamy@apache.org>
@oleg-nenashev oleg-nenashev mentioned this issue Oct 26, 2020
Merged
10 tasks
@dependabot dependabot bot mentioned this issue Mar 15, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@joakime joakime

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Better support for implied ports on ForwardedRequestCustomizer jetty/jetty.project
3 participants
@gregw @joakime @stuban