Ensure Forwarded and X-Forwarded values are the same

[sberyozkin]

Fixes #35751

Enabling both X-Forwarded and Forwarded for the proxy address forwarding may pose some risks - it is very clearly mentioned in JavaDocs for both the allowForwarded and allowXforwarded properties and a warning is logged when both are enabled. This PR adds a hardening check to make sure that if both Forwarded and XForwarded headers are enabled on the proxy address forwarding path, then the header values must match.

I've been thinking that if the mismatch happens then an error must be reported and it should be 400, as opposed to 500. If it is agreed upon then the remaining puzzle for me is where to intercept non-JAX-RS BadClientRequestException in order to end the request with 401, which I added in this PR, since the forwarded handler delegates to Vert.x HttpServerRequest.

Also CC @luneo7

[sberyozkin]

I've figured out how to end it with 400, setting the response status and doing delegate.end() seems to work, though happy to improve it

[luneo7]

Will see if I can test this in our infra, we have one use case that our service is served through AWS API GW externally (which we also filter the exposed endpoints), and internally through VPN we hit the service hitting one AWS ALB... so each one adds the header differently, ALB adds the Forwarded, API GW adds X-Forwarded... so need to see in both entry points if this change would be a breaking change and would also render our service somewhat useless if we were to upgrade to a version with this change.
Thanks @sberyozkin for pinging me =)

[michalvavrik]

I don't really have time until Saturday, I'll re-check then, but please don't wait for my review. Thanks

[sberyozkin]

Thanks @luneo7 If it proves a breaking change that we can add a strict check property that you'd be able to turn off...

But the main reason I turned it to draft is that I'm not certain the solution is correct, because right now it will cause a failure if, for ex, Forwarded headers impact scheme, host only and XForwarded only host.

I believe only those properties which have been impacted by both headers must be compared. For example, in the previous example, it should be checked that both Forwarded and XForwarded set exactly the same host, and not trying to check the scheme.

Is it a correct assumption ?

[sberyozkin]

I've updated it to make sure that only those properties are checked that are set at both Forwarded and X-Forwarded level, to avoid some forwarded property value being overridden by the x-forwarded one

[cescoffier]

Thanks @sberyozkin for addressing this! It makes sense. Honestly, I would fail if the values are not the same (and add a flag to disable this check).

[sberyozkin]

Hi @cescoffier

Yes, it is HTTP 400 right now if the values does not match.

Right, I can look at adding a property, I'd like to hear from @luneo7 as well when he gets a chance to test it in his setup

[cescoffier]

@sberyozkin We have related issues about the precedence between the headers. So, I believe we would need a flag to disable the "safe way".

See #35751 (which I would consider closed one this is in).

[cescoffier]

LGTM - I would just add the flag as discussed.

[sberyozkin]

Hi @cescoffier @gsmet, I had this feeling when looking at the code, but I was confused by several things, by the #35751 issue, so I've realized only now that Forwarded and X-Forwarded are not supported at the same time on main, in released versions.
X-Forwarded is only checked in the else branch here, if Forwarded has not been processed above here.

So when the user says allowForwarded=true, then allowXForwarded=true is ignored. So it is always only Forwarded or XForwarded.

The test which was added to prove that both Forwarded and X-Forwarded can be used simply confirms that only Forwarded is taken into consideration.

The original issue, #25077, remains unresolved, while #35751 is basically invalid because Forwarded can not override X-Forwarded, because the latter is ignored altogether.

So I've moved it to draft again because it actually changes the main behavior and properly fixes #25077, applies a strict control by default where the same header values must match but allows X-Forwarded override some or all of Forwarded values which would actually address #35751 - essentially it disabling a strict mode indirectly changes the precedence - XForwarded headers will decide what can be overridden if Forwarded is present.

Comments are welcome, I'll reopen for review once everyone is happy with what is proposed here.

[github-actions]

🎊 PR Preview 1e7e874 has been successfully built and deployed to https://quarkus-pr-main-44601-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[luneo7]

@sberyozkin in our case it wouldn't work, just checked... forwarded header has the right user IP address, and X-Forwarded-For has internal IP from AWS that hit the ALB, since the path is API GW -> ALB... whereas hitting the service directly only has X-Forwarded-For.... so if we have this rule to match both headers, it would make our use case not work... it would be awesome to have a disable flag

[luneo7]

And yeah, when we have both headers, forwarded has precedence over X-Forwarded-For, so forwarded needs to be considered and X-Forwarded-For discarded

[sberyozkin]

Thanks @luneo7.
So you have 2 types of requests, direct with X-Forwarded only, and indirect with Forwarded and X-Forwarded combined but Forwarded being preferred.
But the current solution for Forwarded and X-Forwarded combined does not allow X-Forwarded being preferred.

I think we need one more property, the precedence enum.

[sberyozkin]

Now, when both types of forwarded headers are enabled, and no strict control is enabled, the precedence decides which headers make final impact. For example, if Forwarded has a precedence then they are run after X-Forwarded are run to be able to overwrite whatever they set, and vice versa.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit a4d1906.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.