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

Improve ForwardRequestCustomizer authority priority #5247

Closed
joakime opened this issue Sep 8, 2020 · 3 comments · Fixed by #5251
Closed

Improve ForwardRequestCustomizer authority priority #5247

joakime opened this issue Sep 8, 2020 · 3 comments · Fixed by #5251

Comments

@joakime
Copy link
Contributor

joakime commented Sep 8, 2020

Jetty version
9.4.31

Description
The ForwardedRequestCustomizer has a torturous implementation of priority for how the authority (server name:port) is calculated.

We should improve this to implement a clear priority of what header fields are interrogated, and under what order they resolve to an authority.
Bonus is an improved javadoc that documents this behavior.
On a brief discussion with @gregw the following priority for the Authority Host and Port were proposed.

Authority Host

  1. Forwarded (RFC7239) (left-most)
  2. X-Forwarded-Host (left-most)
  3. X-Forwarded-For (left-most)
  4. Host
  5. default to what? or ServerConnector localAddress? do we need this? No Host header is an error, right?

Authority Port

  1. Forwarded (RFC7239) (left-most)
  2. X-Forwarded-Host (left-most)
  3. X-Forwarded-For (left-most)
  4. X-Forwarded-Port (left-most)
  5. Host
  6. X-Forwarded-Proto (left-most) [https=443, http=80]
  7. X-Proxied-Https (on=443, off=80)
  8. default to 80? or ServerConnector localPort? (don't think we need to interrogate the ServerConnector)

I propose we track the authority host and port separately (not in a HostPort field).
Each with a priority int, that indicates where the last setting of those fields came from.
Then, we could change the MethodHandle calls to include a priority for when that specific field is encountered, ala handlePort(int priority, HttpField field), and if the field (port in this case) was set, and from a lower priority then this call, then override it's value.
When the full set of headers is done parsing, finally form the HostPort field from the separate host and port fields.

@peterlynch
Copy link

peterlynch commented Sep 9, 2020

I noticed X-Forwarded-Server header was omitted from the opening issue description. It might need to be considered as well for Authority Host?

org.eclipse.jetty.server.ForwardedRequestCustomizer.setHostHeader(String) seems also designed to force the authority to be whatever an admin configures it to be, overriding inbound host inspection?

joakime added a commit that referenced this issue Sep 9, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Sep 9, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime linked a pull request Sep 9, 2020 that will close this issue
@joakime
Copy link
Contributor Author

joakime commented Sep 9, 2020

Opened PR #5247 to address this.

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

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)

Key:

  • "Required" means that if the origin exists, then the value (host or port) must exist and have a valid value.
  • "Optional" means that if the origin exists, then the value (host or port) can be missing / empty and will result in no change to the authority
  • "Authoritative" means that if the origin exists, then the value provided is taken as-is, even if it's missing / empty. (only relevant for port)
  • "standard" means that the port comes from standards defined names (see notes).
  • "boolean" means that the port comes from "on" and "off" values (see notes).

joakime added a commit that referenced this issue Sep 11, 2020
+ 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 added a commit that referenced this issue Sep 11, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Sep 11, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Sep 11, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Sep 21, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Sep 23, 2020
…er-priority

Issue #5247 ForwardedRequestCustomizer authority order rework
@joakime
Copy link
Contributor Author

joakime commented Sep 23, 2020

Merged #5251

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

Successfully merging a pull request may close this issue.

2 participants