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

[quarkus-bot[bot]]

Status for workflow Quarkus CI

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

:white_check_mark: 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.

---

Flaky tests - Develocity

:gear: Native Tests - HTTP

:package: integration-tests/rest-client-reactive

✖ io.quarkus.it.rest.client.BasicTestIT.shouldCreateClientSpans - History

• expected: <1> but was: <2> - org.opentest4j.AssertionFailedError

``` org.opentest4j.AssertionFailedError: expected: <1> but was: <2> at io.quarkus.it.rest.client.BasicTest.shouldCreateClientSpans(BasicTest.java:216) at java.base/java.lang.reflect.Method.invoke(Method.java:569) at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:806) at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) ```

[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).