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

Issue #5247 ForwardedRequestCustomizer authority order rework #5251

Merged
merged 7 commits into from
Sep 23, 2020

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Sep 9, 2020

This is an alternate approach for handling the search order for request.authority in ForwardedRequestCustomizer.
With javadoc.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from gregw September 9, 2020 20:22
@joakime joakime self-assigned this Sep 9, 2020
@joakime joakime added this to In progress in Jetty 9.4.32 via automation Sep 9, 2020
@joakime joakime linked an issue Sep 9, 2020 that may be closed by this pull request
@joakime joakime moved this from In progress to Review in progress in Jetty 9.4.32 Sep 9, 2020
@joakime
Copy link
Contributor Author

joakime commented Sep 9, 2020

This is the search order for request.authority in an incoming request now.
(this same table is in the javadoc for ForwardedRequestCustomizer in this PR)

num Value Origin Host Port Notes
1 Forwarded Header Required Authoritative From left-most host=[value] parameter (see rfc7239)
2 X-Forwarded-Host Header Required Optional left-most value
3 X-Forwarded-Port Header n/a Required left-most value (only if getForwardedPortAsAuthority() is true)
4 X-Forwarded-Server Header Required Optional left-most value
5 Request Metadata Optional Optional found in Request Line absolute path and/or Host client request header value as value host:port or host
6 X-Forwarded-Proto Header n/a standard left-most value as http (implied port 80) or https (implied port 443)
7 X-Proxied-Https Header n/a boolean left-most value as on (implied port 443) or off (implied port 80)

+ 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>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Copy link
Contributor Author

joakime commented Sep 11, 2020

Current javadoc says (it looks better formatted, and linked, in javadoc) ...

num Value Origin Host Port Protocol Notes
1 Forwarded Header "host=<host>" param (Required) "host=<host>:<port>" param (Implied) "proto=<value>" param (Optional) From left-most relevant parameter (see rfc7239)
2 X-Forwarded-Host Header Required Implied n/a left-most value
3 X-Forwarded-Port Header n/a Required n/a left-most value (only if getForwardedPortAsAuthority() is true)
4 X-Forwarded-Server Header Required Optional n/a left-most value
5 X-Forwarded-Proto Header n/a Implied from value Required left-most value becomes protocol.Value of "http" means port=80.Value of "HttpConfiguration.getSecureScheme()" means port=HttpConfiguration.getSecurePort().
6 X-Proxied-Https Header n/a Implied from value boolean left-most value determines protocol and port.Value of "on" means port=HttpConfiguration.getSecurePort(), and protocol=HttpConfiguration.getSecureScheme()).Value of "off" means port=80, and protocol=http.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Copy link
Contributor Author

joakime commented Sep 16, 2020

@gregw bump. review please.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of niggles, but otherwise LGTM.
I don't think the CI failure is related?

* Lowest priority first.
* </p>
*/
public enum Priority
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than "Priority" I think "Source" is a better name for this. Will make more sense in logging to see something like "source=XFORWARDED_FOR". It also matches a lot of our webapp config stuff where we record the source, which is then used to determine the priority.

Comment on lines 870 to 871
getAuthority().setHost(hostField.getHost(), Priority.FORWARDED);
getAuthority().setPort(hostField.getPort(), Priority.FORWARDED);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe:

Suggested change
getAuthority().setHost(hostField.getHost(), Priority.FORWARDED);
getAuthority().setPort(hostField.getPort(), Priority.FORWARDED);
getAuthority().setHostPort(hostField.getHost(), hostField.getPort(), Priority.FORWARDED);

Jetty 9.4.32 automation moved this from Review in progress to Reviewer approved Sep 21, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime merged commit 8fe32ce into jetty-9.4.x Sep 23, 2020
Jetty 9.4.32 automation moved this from Reviewer approved to Done Sep 23, 2020
@joakime joakime deleted the jetty-9.4.x-5247-forwarded-header-priority branch September 23, 2020 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Jetty 9.4.32
  
Done
Development

Successfully merging this pull request may close these issues.

Improve ForwardRequestCustomizer authority priority
3 participants