Regression with ForwardedParser setting an empty host header

[shawkins]

Describe the bug

Keycloak has failing tests in https://github.com/keycloak/keycloak/pull/24639 with the upgrade to quarkus 3.2.8. The ForwardedParser is setting an empty host header eventually results in uris being returned to the client without a host.

Expected behavior

location uris should be fully formed.

Actual behavior

Location uris are missing the host "https:/auth/admin..."

The likely cause is this change https://github.com/quarkusio/quarkus/commit/d41c78bf553406c6026d28ff2795f096b9da184d#diff-26b0d509bb91cfaf3c4160496ea25fc584e85d4e2cc1b708eb72ea3b25b74c4dR124 - if there is no host header it will proceed to set an explicit empty value.

How to Reproduce?

See the pr https://github.com/keycloak/keycloak/pull/24639

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

3.2.8

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

[gsmet]

@cescoffier @aloubyansky for your awareness. If we release a 3.2.9, we probably need to fix that too.

[cescoffier]

It got fixed in Vertx. We would need to bump vertx to get the fix in.

It is not something we can fix in Quarkus only.

[vmuzikar]

This is blocking Keycloak from upgrading from 3.2.7. It'd be super nice to get this fixed. :)

[vmuzikar]

jFTR – this is a regression introduced by 3.2.8.

[cescoffier]

Unfortunately, it requires a vertx bump.

[aloubyansky]

4.4.6 @cescoffier?

[aloubyansky]

ah, it's already 4.4.6 in 3.2.8, so Vert.X with the fix hasn't even been released yet?

[cescoffier]

No 4.4.6 contains the fix.

[cescoffier]

I would need a reproducer. We cannot rollback that change as it is a fix for http/2. Note that empty host header is slightly odd.

[cescoffier]

I just read the issue again. So it's actually the opposite. You got an empty host. I may have been missing something (I do not touch to the security layer).

Note that while odd, it's valid.

[vmuzikar]

Modern browser, e.g. Chrome, seem to be omitting the Host header in HTTP/2 requests and replace it by the :authority pseudo-header.

I was able to somewhat reproduce it outside of Keycloak as simply as:

    @GET
    @Produces(MediaType.TEXT_PLAIN)
    public String get(@Context UriInfo uriInfo) {
        return uriInfo.getAbsolutePath().toString();
    }

which throws an NPE in 3.2.8 when accessed via a real hostname and TLS (e.g. https://127.0.0.1.nip.io:8443/), this is not reproducible with just localhost for some reason.

[cescoffier]

Can you give me the instructions to set up this kind of environment?

[vmuzikar]

@cescoffier No need to set up anything. :) Just use a self-signed TLS cert and access the endpoint via nip.io (it works as is out of the box, 127.0.0.1.nip.io points to 127.0.0.1).

[cescoffier]

Ok, got the NPE.

[cescoffier]

Unfortunately, we would need a fix in Vert.x. The stack trace is:

2023-11-14 10:44:39,026 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (executor-thread-1) HTTP Request to /hello failed, error id: e8875d63-d7aa-4a9b-8b26-6970f253e227-2: java.lang.NullPointerException
        at io.vertx.core.net.impl.HostAndPortImpl.<init>(HostAndPortImpl.java:141)
        at io.vertx.core.net.HostAndPort.create(HostAndPort.java:20)
        at io.vertx.ext.web.impl.ForwardedParser.calculate(ForwardedParser.java:137)
        at io.vertx.ext.web.impl.ForwardedParser.authority(ForwardedParser.java:82)
        at io.vertx.ext.web.impl.HttpServerRequestWrapper.authority(HttpServerRequestWrapper.java:188)
        at org.jboss.resteasy.reactive.server.vertx.VertxResteasyReactiveRequestContext.getRequestAbsoluteUri(VertxResteasyReactiveRequestContext.java:181)

[cescoffier]

Just opened https://github.com/vert-x3/vertx-web/issues/2511.

[cescoffier]

The issue is unrelated to the forward parsed or empty host header. It's because the parsing to the nip URL was considered as an IP (as it starts like an IP). It's fixed by https://github.com/eclipse-vertx/vert.x/pull/4948#event-10958579094.

[cescoffier]

See https://github.com/vert-x3/vertx-web/issues/2511#issuecomment-1811234841 for further explainations. As you will see, it's is not related to HTTP2 or the forward parser.

If it's not the root cause, please provide another standalone reproducer.

[vmuzikar]

@cescoffier Thanks for the update!

As it turned out, while the NPE was a real issue, it was unrelated to this one. Sorry for the confusion.

To reproduce the original issue, I forgot to mention you need to set quarkus.http.proxy.proxy-address-forwarding=true to engage the forwarded parser. Then you can use the reproducer I mentioned in a previous comment. Even with localhost you'll see it outputs https:///.

The root cause seems to be still present in the Quarkus 3.2 branch.

[cescoffier]

Ok, I will have another look, but not before Friday or next week.

[cescoffier]

BTW, I would need a standalone reproducer - not involving keycloak.

[vmuzikar]

Ok, I will have another look, but not before Friday or next week.

Thanks. Feels quite critical, TBH. Quarkus basically broken proxy headers handling in an LTS version micro release.

BTW, I would need a standalone reproducer - not involving keycloak.

Sure, the reproducer I provided in the comment is standalone.

[aloubyansky]

@vmuzikar would you be able to check whether https://github.com/quarkusio/quarkus/pull/37135 works for you?

[vmuzikar]

Looks it worked for the Keycloak use case. Thanks for the quick fix!

Can we expect this to be fixed in 3.2.9?

[aloubyansky]

Yes