Update to Quarkus 3.2.8

[abstractj]

Closes #24638
Closes #24160

[jsorah]

Based on my testing, this should also fix #24160

Not sure of the etiquette if we should add another closes for that one to also link the PR or if the mention is enough.

[ahus1]

@abstractj / @vmuzikar - I've updated some version in Keycloak's POM that should match Quarkus' parent POM in a second commit. Please squash the commits before merging.

[vmuzikar]

@abstractj Thanks for the PR.

Seems there's some issue with the Operator OLM tests. I'll look into it.

[vmuzikar]

@abstractj @shawkins Created abstractj#2

[abstractj]

Based on my testing, this should also fix #24160

Not sure of the etiquette if we should add another closes for that one to also link the PR or if the mention is enough.

Thanks for pointing that out @jsorah. Updated the commit message to reflect that.

[mabartos]

LGTM

[ahus1]

@abstractj - there have been some expired certificates, which have just been updated in main. You might need another CI run before all your tests are green. See #24657.

[shawkins]

@abstractj @vmuzikar @vietj the issue with the javascript adapter tests appear to be due to changes in the vertx forwarding logic.

The problematic request is explicitly setting an empty host:

sec-ch-ua="Not/A)Brand";v="99", "HeadlessChrome";v="115", "Chromium";v="115"
sec-ch-ua-mobile=?0
sec-ch-ua-platform="Linux"
upgrade-insecure-requests=1
user-agent=Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/115.0.5790.110 Safari/537.36
accept=text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7
sec-fetch-site=none
sec-fetch-mode=navigate
sec-fetch-user=?1
sec-fetch-dest=document
accept-encoding=gzip, deflate, br
host=

The forwarding logic then creates an empty authority and will calculate absolute uris that lack a host https:/auth/admin...

I'm not sure if this this intended to be a protection mechanism when the host isn't populated, or if it should still infer the host - someone else will need to weigh in on a fix.

I see the same condition being hit during the failing cors tests, so it's possible this could be the root cause for all the failures.

[vmuzikar]

@shawkins Thanks for investigating. I think we need someone from @keycloak/ui to chime in. I don't think a request without Host header is really valid, seems like a potential bug in the JS adapter to me. The header should be driven by the Hostname SPI.

[vmuzikar]

@shawkins Actually, which request exactly do you mean is missing the header? A JS Adapter created request?

[shawkins]

@shawkins Actually, which request exactly do you mean is missing the header? A JS Adapter created request?

I would presume it is coming from there yes.

[jonkoops]

I did some testing with @vmuzikar, and to me it doesn't look like this is an error caused by Keycloak JS or any other client-side code for that matter. I was able to run the test and get it to fail using the following command:

mvn test -Dtest=JavascriptAdapterTest#testLocationHeaderInResponse -Pauth-server-quarkus

The test only seems to fail when the Quarkus server is enabled using -Pauth-server-quarkus. The failure here is as follows:

JavascriptAdapterTest.testLocationHeaderInResponse:654->lambda$testLocationHeaderInResponse$2a7ea37a$1:658 
Expected: a string containing "location: https://localhost:8543/auth/admin/realms/test/users/044f3ca4-af90-4ecb-b6b6-63d36a9cc2fd"
     but: was "access-control-allow-credentials: true
access-control-allow-origin: https://localhost:8543
access-control-expose-headers: location
content-length: 0
location: https:/auth/admin/realms/test/users/044f3ca4-af90-4ecb-b6b6-63d36a9cc2fd
referrer-policy: no-referrer
strict-transport-security: max-age=31536000; includesubdomains
x-content-type-options: nosniff
x-frame-options: sameorigin
x-xss-protection: 1; mode=block
"

Specifically, the value of the location header returned here is malformed, causing the test to fail:

-https://localhost:8543/auth/admin/realms/test/users/044f3ca4-af90-4ecb-b6b6-63d36a9cc2fd
+https:/auth/admin/realms/test/users/044f3ca4-af90-4ecb-b6b6-63d36a9cc2fd

For some reason the domain name is being stripped from the URL returned from the server. This seems to be a regression in Quarkus.

Looking at the changelog it looks like quarkusio/quarkus#36234 might have something to do with this, which leads to eclipse-vertx/vert.x#4879, which unfortunately does not have a commit or PR associated with it.

Either way, I think this is a Quarkus bug, or we're not passing something into it correctly on our side. It cannot be a client-side bug, as we're not allowed to set the Host header since it's a forbidden header name.

[shawkins]

@jonkoops that is correct, it is not coming directly from the client side.

For some reason the domain name is being stripped from the URL returned from the server. This seems to be a regression in Quarkus.

Working backwards more from https://github.com/vert-x3/vertx-web/blob/a76e681af8313035ab7a772481d840a81051ecc9/vertx-web/src/main/java/io/vertx/ext/web/impl/ForwardedParser.java#L200 the header is being set by https://github.com/quarkusio/quarkus/blob/b9766f547d2c5c02715b5e35fda1c5f2f15904d8/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/ForwardedParser.java#L202

The headers at this level show the host is missing, not empty:

sec-ch-ua="Not/A)Brand";v="99", "HeadlessChrome";v="115", "Chromium";v="115"
sec-ch-ua-mobile=?0
sec-ch-ua-platform="Linux"
upgrade-insecure-requests=1
user-agent=Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/115.0.5790.110 Safari/537.36
accept=text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7
sec-fetch-site=none
sec-fetch-mode=navigate
sec-fetch-user=?1
sec-fetch-dest=document
accept-encoding=gzip, deflate, br

Which then gets set as an explicit empty value on the delegate, which then gets turned into the uri with the missing host.

For reference the offending request is coming into the RealmsResource.getProtocol method - test/protocol/openid-connect/

[vmuzikar]

@jonkoops Thanks for the summary.

@shawkins I looked into this a bit more. AFAICT, it is completely valid that Host header is omitted if :authority HTTP/2 pseudo-header is present. That's what e.g. Chrome is doing and that's why JS related tests are failing. Looks to me like a Vert.x bug. WDYT?

[shawkins]

Looks to me like a Vert.x bug. WDYT?

Yes, vert.x or the vert.x extension logic in quarkus.

This change looks suspect: quarkusio/quarkus@d41c78b#diff-26b0d509bb91cfaf3c4160496ea25fc584e85d4e2cc1b708eb72ea3b25b74c4dR124 - the old logic just called host(), which would return localhost:8543 in this case.

[jonkoops]

So I guess this will have to be reported as a bug for Quarkus, perhaps we can even submit a fix? Anyone willing to jump on that? Either way, it looks like we will not be able to upgrade until this is fixed upstream.

[vmuzikar]

@jonkoops @shawkins Yeah, definitely seems like a blocker that prevents us from upgrading. Is there an issue for it on Quarkus side already?

[jonkoops]

Is there an issue for it on Quarkus side already?

Did a quick search, but I could not find any issues closed or open that report this problem.

[shawkins]

Captured as quarkusio/quarkus#37045

[vmuzikar]

jFTR – it was confirmed to be a Vert.x bug: vert-x3/vertx-web#2511

Found also another reproducer. Just access KC via https://127.0.0.1.nip.io:8443/ (needs to be TLS to make browser to use HTTP/2 and a real hostname, not localhost).

[vmuzikar]

Possible workaround would be to temporarily disable HTTP/2 in KC. But there'd be perf implications at the very least.

[abstractj]

Possible workaround would be to temporarily disable HTTP/2 in KC. But there'd be perf implications at the very least

@vmuzikar my 2 cents. I'd wait for the next release with the fix.

[vmuzikar]

eclipse-vertx/vert.x#4947 was resolved but it turned out to be another regression in 3.2.8. The original issue is still unresolved.

[vmuzikar]

It was confirmed the Quarkus issue will be resolved in 3.2.9. Since we'll skip 3.2.8, I think we can close this PR.

[vmuzikar]

Superseded by #24842.

@abstractj Let me know if you have any concerns with closing this.