8000 Update to Quarkus 3.2.8 by abstractj · Pull Request #24639 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Update to Quarkus 3.2.8 - #24639

Closed
abstractj wants to merge 1 commit into
keycloak:mainfrom
abstractj:issue-24638
Closed

Update to Quarkus 3.2.8#24639
abstractj wants to merge 1 commit into
keycloak:mainfrom
abstractj:issue-24638

Conversation

@abstractj
@abstractj abstractj commented Nov 8, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

Closes #24638
Closes #24160

@abstractj
abstractj requested a review from a team November 8, 2023 17:26
@abstractj
abstractj requested a review from a team as a code owner November 8, 2023 17:26
@ghost ghost added the team/cloud-native label Nov 8, 2023
shawkins
shawkins previously approved these changes Nov 8, 2023
View reviewed changes
@jsorah
jsorah commented Nov 8, 2023
Copy link
Copy Markdown
Contributor

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
ahus1 commented Nov 8, 2023
Copy link
Copy Markdown
Member

@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.

@ghost ghost added the flaky-test label Nov 9, 2023
@ghost ghost mentioned this pull request Nov 9, 2023
Closed
vmuzikar
vmuzikar previously approved these changes Nov 9, 2023
View reviewed changes
@vmuzikar
vmuzikar commented Nov 9, 2023
Copy link
Copy Markdown
Contributor

@abstractj Thanks for the PR.

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

@vmuzikar
vmuzikar commented Nov 9, 2023
Copy link
Copy Markdown
Contributor

@abstractj @shawkins Created abstractj#2

@abstractj
abstractj commented Nov 9, 2023
Copy link
Copy Markdown
Contributor Author

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
mabartos approved these changes Nov 9, 2023
View reviewed changes
@mabartos mabartos left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ahus1
ahus1 commented Nov 9, 2023
Copy link
Copy Markdown
Member

@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.

@abstractj
abstractj force-pushed the issue-24638 branch 2 times, most recently from ec4d204 to 6ace403 Compare November 10, 2023 14:35
@vmuzikar vmuzikar mentioned this pull request Nov 10, 2023
Merged
This was referenced Nov 10, 2023
Closed
Closed
@shawkins
shawkins commented Nov 10, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

@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
vmuzikar commented Nov 13, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

@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
vmuzikar commented Nov 13, 2023
Copy link
Copy Markdown
Contributor

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

@shawkins
shawkins commented Nov 13, 2023
Copy link
Copy Markdown
Contributor

@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
jonkoops commented Nov 13, 2023
Copy link
Copy Markdown
Contributor

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
shawkins commented Nov 13, 2023
Copy link
Copy Markdown
Contributor

@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
vmuzikar commented Nov 13, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

@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
shawkins commented Nov 13, 2023
Copy link
Copy Markdown
Contributor

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
jonkoops commented Nov 13, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

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
vmuzikar commented Nov 13, 2023
Copy link
Copy Markdown
Contributor

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

@jonkoops
jonkoops commented Nov 13, 2023
Copy link
Copy Markdown
Contributor

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 shawkins mentioned this pull request Nov 13, 2023
Closed
@shawkins
shawkins commented Nov 13, 2023
Copy link
Copy Markdown
Contributor

Captured as quarkusio/quarkus#37045

@vmuzikar
vmuzikar commented Nov 14, 2023
Copy link
Copy Markdown
Contributor

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
vmuzikar commented Nov 14, 2023
Copy link
Copy Markdown
Contributor

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

@abstractj
abstractj commented Nov 14, 2023
Copy link
Copy Markdown
Contributor Author

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
vmuzikar commented Nov 15, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

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
vmuzikar commented Nov 16, 2023
Copy link
Copy Markdown
Contributor

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.

@abstractj @ahus1 @vmuzikar
Update to Quarkus 3.2.8
c3a30db 
Closes keycloak#24638
Closes keycloak#24160

Signed-off-by: Bruno Oliveira da Silva <bruno@abstractj.com>
Co-authored-by: Alexander Schwartz <aschwart@redhat.com>
Co-authored-by: Václav Muzikář <vmuzikar@redhat.com>
@vmuzikar
vmuzikar commented Nov 20, 2023
Copy link
Copy Markdown
Contributor

Superseded by #24842.

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

@vmuzikar vmuzikar closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmuzikar vmuzikar vmuzikar approved these changes
@mabartos mabartos mabartos approved these changes
@shawkins shawkins Awaiting requested review from shawkins

Assignees

@stianst stianst

Labels

flaky-test team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update to Quarkus 3.2.8 HTTP/2 - Last parameter of POST form data contains 0x00 byte in some configurations.

8 participants

@abstractj @jsorah @ahus1 @vmuzikar @shawkins @jonkoops @mabartos @stianst
0