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

Enable forwarded for host header configuration #10138

Merged
merged 1 commit into from
Jun 24, 2020
Merged

Enable forwarded for host header configuration #10138

merged 1 commit into from
Jun 24, 2020

Conversation

ejba
Copy link
Contributor

@ejba ejba commented Jun 21, 2020

This is the first part of #9622 that enable users to configure the header to be used to override the HTTP request's host.

@ejba
Copy link
Contributor Author

ejba commented Jun 21, 2020

@ejba Looks like a switch per header can be the most secure solution based on what you've discussed with @stuartwdouglas and @kraeftbraeu, please follow up with this update.

May be it can make sense to split the PR in 2 parts, the 1st part will only deal with X-Forwarded-Host and X-Forwarded-Server to accelerate it, X-Forwarded-Prefix related updated can be discussed further as needed in another (draft) PR...
Sounds good ?
Cheers, Sergey

/cc @sberyozkin @stuartwdouglas @kraeftbraeu

* Enable override the received request's host through a forwarded host header.
*/
@ConfigItem(defaultValue = "false")
public boolean enableForwardedForHostHeader;
Copy link
Member

Choose a reason for hiding this comment

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

This feels a bit long, maybe 'enableForwardedHost' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (hostHeader != null) {
setHostAndPort(hostHeader.split(",")[0], port);
if (proxyConfig.enableForwardedForHostHeader) {
AsciiString forwardedHeader = AsciiString.cached(proxyConfig.forwardedHostHeader);
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to do this in the recorder, and pass it into the parser, so it is only done once and not once per request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* Holds configuration related with proxy addressing forward.
*/
@ConfigGroup
public class ProxyConfig {
Copy link
Member

Choose a reason for hiding this comment

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

I think the existing proxy config should be moved to this class as well (proxyAddressForwarding and allowForwarded).

To do this mark the original ones deprecated, change them to Optional, and then add new ones here. If the old ones are set then log a warning about deprecation and use those values, otherwise use the new config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@kraeftbraeu
Copy link

My mail app didn't inform me about this new PR, time to look for a new mail app :/

I like what I see here!
👍 Clean header handling
👍 backward compatibility

Just one question: What about documentation? Some sentences in docs/src/main/asciidoc/vertx.adoc like in https://github.com/quarkusio/quarkus/pull/9809/files#diff-69f9c3e57ab7a8001e01e5d4683e057a would be helpful.

@kraeftbraeu
Copy link

Hi @stuartwdouglas, a short friendly reminder: Please check @ejba 's recent changes and update your review status. I'm really looking forward to that merge :)

@stuartwdouglas stuartwdouglas added this to the 1.6.0 - master milestone Jun 24, 2020
@stuartwdouglas stuartwdouglas merged commit 97ad4fe into quarkusio:master Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants