Undocumented change in priority of X-Forwarded-* headers as of Quarkus distribution

[coro]

Before reporting an issue

• I have searched existing issues
• I have reproduced the issue with the latest nightly release

Area

oidc

Describe the bug

Upon upgrading from KC 10 to KC 21, we noticed a breaking change in how Keycloak prioritises X-Forwarded-* headers, when behind a reverse proxy and reporting OIDC config via the .well-known/openid-configuration endpoint. Specifically, if there is a conflict between the port specified in X-Forwarded-Port and X-Forwarded-Host.

In Wildfly, the X-Forwarded-Host header takes priority over the X-Forwarded-Port header (source)

$ curl -s -H 'X-Forwarded-Host: forwardhost:9090' -H 'X-Forwarded-Port: 80' http://localhost:8080/realms/master/.well-known/openid-configuration | jq | head -n 5
{
  "issuer": "http://forwardhost:9090/realms/master",
  "authorization_endpoint": "http://forwardhost:9090/realms/master/protocol/openid-connect/auth",
  "token_endpoint": "http://forwardhost:9090/realms/master/protocol/openid-connect/token",
  "token_introspection_endpoint": "http://forwardhost:9090/realms/master/protocol/openid-connect/token/introspect",

In Quarkus, the opposite is true (source)

$ curl -s -H 'X-Forwarded-Host: forwardhost:9090' -H 'X-Forwarded-Port: 80' http://localhost:8080/realms/master/.well-known/openid-configuration | jq | head -n 5
{
  "issuer": "http://forwardhost/realms/master",
  "authorization_endpoint": "http://forwardhost/realms/master/protocol/openid-connect/auth",
  "token_endpoint": "http://forwardhost/realms/master/protocol/openid-connect/token",
  "introspection_endpoint": "http://forwardhost/realms/master/protocol/openid-connect/token/introspect",

$ curl -s -H 'X-Forwarded-Host: forwardhost:9090' http://localhost:8080/realms/master/.well-known/openid-configuration | jq | head -n 5
{
  "issuer": "http://forwardhost:9090/realms/master",
  "authorization_endpoint": "http://forwardhost:9090/realms/master/protocol/openid-connect/auth",
  "token_endpoint": "http://forwardhost:9090/realms/master/protocol/openid-connect/token",
  "introspection_endpoint": "http://forwardhost:9090/realms/master/protocol/openid-connect/token/introspect",

You can see that the X-Forwarded-Port header has effectively removed the desired port 9090. In our case this was because our ALB was adding this header on requests to the OIDC endpoint, thus resulting in a token endpoint that was on an unreachable port as of Quarkus.

Version

21.1.2

Expected behavior

Priority to follow something like Jetty's ForwardedRequestCustomizer, i.e. in order:

1. Forwarded, as defined by rfc7239
2. X-Forwarded-Host
3. X-Forwarded-Port

Actual behavior

1. Forwarded, as defined by rfc7239
2. X-Forwarded-Port
3. X-Forwarded-Host

(omitting some for clarity)

How to Reproduce?

See 'Describe the bug'. I reproduced this on v10.0.2 for the Wildfly distribution, and v21.1.2 and nightly for the Quarkus distribution, with:

$ docker run -p 8080:8080 -e KEYCLOAK_ADMIN=admin -e KEYCLOAK_ADMIN_PASSWORD=admin quay.io/keycloak/keycloak:$KEYCLOAK_VERSION start-dev --proxy edge

Anything else?

I don't believe there's an established standard for the order of priority of the non-standard X-Forwarded-* headers, but I think it would be useful to clarify the current expected order of these in the proxy guides, as well as listed as a breaking change on the Quarkus migration guide.

[shawkins]

@vmuzikar @vietj I think this could be considered a regression. It does appear that most consider the port specified in the X-Forwarded-Host as taking prescedence, and since that was the prior behavior it likely should have remained.

@coro what is responsible for the conflict in the headers? It seems like if host is being populated with a port, then the forwarded port header should be omitted.

[sschu]

This is related if not a duplicate: #23431

[vmuzikar]

I would not consider this a duplicate of #23431. Sure, that issue will deprecate the original proxy option which this issue is about. However, the proxy option will still be around for a while and any change in behaviour should've been captured in upgrading guide. Or possibly it's really a bug in Vert.x.

[shawkins]

Also the new option does not resolve the of the priority between X-Forwarded-Host and X-Forwarded-Port, only the choice between forwarded and x-forwarded.

[coro]

@shawkins thanks for looking at this. In our case, we had an ALB exposing a route on port 80, adding the X-Forwarded-Port: 80. Behind this ALB was an Envoy proxy that added the X-Forwarded-Host: forwardhost:9090, but did not modify the X-Forwarded-Port header. We switched the Envoy to use the Forwarded header instead and it resolved the issue.

I agree it's arguably a regression, though we also arguably shouldn't have been relying on the implicit ordering of those X-Forwarded headers. I mainly raised this for documentation's sake - it was quite difficult to spot this change and it would be useful to have the expectation of ordering explicit in the upgrade docs.

[shawkins]

Moving over to the core to assess further.

[sschu]

There is also an issue on the Quarkus side about this: quarkusio/quarkus#35751

[vmuzikar]

Moving over to the core to assess further.

@shawkins Proxy headers handling belongs more to the Cloud Native team as it's based on Quarkus. :)

[shawkins]

@vmuzikar ok, my misunderstanding. Then we'll need @vietj or others from quarkus / vertx to weigh in on whether this can be treated as a regression.

@sschu I don't see x-forwarded-host nor port mentioned in quarkusio/quarkus#35751

[sschu]

@shawkins True. I just brought it up because it is related, not necessarily the same.

[shawkins]

To resolve this issue we agreed to document this in the migration guide and create an upstream discussion on the handling: quarkusio/quarkus#38032