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

Reconsider the precedence of Forwarded vs X-Forwarded-For #35751

Open
tsaarni opened this issue Sep 5, 2023 · 6 comments · May be fixed by #44601
Open

Reconsider the precedence of Forwarded vs X-Forwarded-For #35751

tsaarni opened this issue Sep 5, 2023 · 6 comments · May be fixed by #44601
Assignees
@sberyozkin
Labels
area/security area/vertx kind/enhancement New feature or request

Comments

@tsaarni
Copy link
Contributor

tsaarni commented Sep 5, 2023 •
edited
Loading

Description

Previously change #25117 introduced the option to set both quarkus.http.proxy.allow-forwarded and quarkus.http.proxy.allow-x-forwarded at the same time. It is rightly highlighted in the docs that this combination is not recommended and there can be security implications if used (doc link).

When both are enabled, Quarkus currently uses Forwarded over X-Forwarded-* if both headers are present. However many popular proxies like NGINX and Envoy do not seem to support Forwarded - they do support X-Forwarded-*. As a result, application that enabled both will be subject to header forging (untrusted client setting Forwarded), unless user of the application realizes this and explicitly re-configures the proxy to overwrite Forwarded. If the order Quarkus parses the headers would be the other way around (X-Forwarded-* over Forwarded) this would not be the case. Even if user missed reconfiguring the proxy and uses the defaults, they would be safe(r).

Another reasoning for the idea: often the backwards compatibility design principles say that old functionality should take precedence over new functionality.

If you consider this idea of swapping the priority something you'd take into Quarkus, I can contribute it.

Implementation ideas

No response
@tsaarni tsaarni added the kind/enhancement New feature or request label Sep 5, 2023
@sberyozkin
Copy link
Member

sberyozkin commented Sep 5, 2023 •
edited
Loading

@tsaarni Hi,

However many popular proxies like NGINX and Envoy do not seem to support Forwarded - they do support X-Forwarded-*. As a result, application that enabled both will be subject to header forging (untrusted client setting Forwarded), unless user of the application realizes this and explicitly re-configures the proxy to overwrite Forwarded

Why then enable both if it is a concern in such cases ?

@tsaarni
Copy link
Contributor Author

tsaarni commented Sep 5, 2023 •
edited
Loading

I have an example use case, where the end-user of a Quarkus-based application started to see the application suddenly act on Forwarded after upgrade, since the application had enabled both settings at the same time. It can be argued that application could/should have provided at least opt-out - as an alternative to requiring end-user to make changes in proxy. On the other hand, if the priority of X-Forwarded was first, it would match with what the popular proxies do by default.

@tsaarni tsaarni mentioned this issue Sep 21, 2023
Closed
@cescoffier
Copy link
Member

While X-Forwarded-For is widely used, it is not a standard.
Conversely, Forwarded is defined in the RFC 7239 (https://tools.ietf.org/html/rfc7239) from 2014. It's kind of sad to still have usages of X-Forwarded-For as Forwarded was defined almost 10 years ago. It is also richer than the bare X-Forwarded-For.

So, I think the current behavior of Quarkus is correct. We prefer a standardized mechanism.

We could imagine an option to switch back the precedence, but it would need to be explicitly set.

@sschu
Copy link
Contributor

sschu commented Oct 16, 2023

I don't think an option to switch precedence would really help here. The problem is that people who don't know/don't care about these headers suddenly have an open attack surface if an attacker sends the headers with higher precedence. I am wondering if it would be better to to just not process requests if both headers are present and the values are not consistent...

@tsaarni
Copy link
Contributor Author

tsaarni commented Oct 16, 2023

I am wondering if it would be better to to just not process requests if both headers are present and the values are not consistent...

Oh, that's right! That would be clever solution to the problem.

@cescoffier
Copy link
Member

I totally agree with @sschu idea.

@sschu sschu mentioned this issue Dec 5, 2023
Closed
2 tasks
@sberyozkin sberyozkin self-assigned this Nov 20, 2024
@sberyozkin sberyozkin linked a pull request Nov 20, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sberyozkin sberyozkin

Labels
area/security area/vertx kind/enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Ensure Forwarded and X-Forwarded values are the same sberyozkin/quarkus
5 participants
@tsaarni @cescoffier @sberyozkin @sschu @geoand