8000 Verification of iss at refresh token request by graziang · Pull Request #24212 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Verification of iss at refresh token request - #24212

Merged
mposolda merged 1 commit into
keycloak:mainfrom
graziang:issue-22191
Oct 24, 2023
Merged

Verification of iss at refresh token request#24212
mposolda merged 1 commit into
keycloak:mainfrom
graziang:issue-22191

Conversation

@graziang
@graziang graziang commented Oct 23, 2023
Copy link
Copy Markdown
Contributor

Added iss checking using the existing TokenVerifier.RealmUrlCheck in the verifyRefreshToken method.

Closes #22191

@graziang
graziang requested review from a team as code owners October 23, 2023 06:43
@graziang
graziang requested a review from a team October 23, 2023 06:43
@graziang
graziang requested a review from a team as a code owner October 23, 2023 06:43
@graziang
graziang commented Oct 23, 2023
Copy link
Copy Markdown
Contributor Author

HI, I've reproduced the issue, and indeed, the iss check is missing during token refresh and during logout with refresh token.

To fix the problem, I've added the iss check using the existing TokenVerifier.RealmUrlCheck, with its error message: "Invalid token issuer. Expected X, but was Y."
Is this message too explicit?

Additionally, I've added a test in the RefreshTokenTest using the ReverseProxy to simulate token refresh from another issuer. Please let me know if this test is unnecessary or invasive.

P.S. Using the https url of the ReverseProxy with the OAuthClient seems to be causing a certificate verification issue. I've added a get method in the ReverseProxy to get the proxy http url (to be analyzed, perhaps opening another issue).

mposolda
mposolda reviewed Oct 23, 2023
View reviewed changes
@mposolda mposolda left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Peps14 Nice, Thanks for the PR!

Added one minor inline comment regarding the test.

I think you have good point regarding the explicit error details message. IMO sending back the error response with some values, which were sent in the parameters itself can cause some XSS attacks or other issues in theory. For instance if client applications displays the errorDescription on the page, it can in theory cause trigger the javascript (in case that refresh token is something, which contains some XSS in the issuer field).

Maybe we can revisit this more thoroughly, but for this one, maybe it is OK to just update error message sent by the issuer check, so it contains just something like "Invalid token issuer. Expected X", but no sending back the value of the issuer from the provided refresh-token. WDYT?

Comment thread ...ation-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/RefreshTokenTest.java Outdated
@mposolda mposolda Oct 23, 2023
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to update this to ClassRule to avoid restart undertow server at every test method?

@ghost ghost added the flaky-test label Oct 23, 2023
This was referenced Oct 23, 2023
Closed
Closed
Closed
Closed
Closed
ghost
ghost reviewed Oct 23, 2023
View reviewed changes
@ghost ghost left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ghost
ghost commented Oct 23, 2023
Copy link
Copy Markdown

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

@graziang
graziang commented Oct 23, 2023
Copy link
Copy Markdown
Contributor Author

Hi @mposolda thanks for the review.
I totally agree with you, just committed the changes.

@abstractj
abstractj requested a review from mposolda October 23, 2023 16:49
@ghost ghost mentioned this pull request Oct 23, 2023
Closed
pedroigor
pedroigor previously approved these changes Oct 23, 2023
View reviewed changes
@mposolda
mposolda commented Oct 24, 2023
Copy link
Copy Markdown
Contributor

@Peps14 Thanks! Looks good, besides RefreshTokenTest failing. Hopefully you can figure it. In worst case, it might be possible to manually start/stop the proxy just during this particular method instead of using Rule. But maybe it is something different. Hopefully you are able to investigate.

@graziang
graziang commented Oct 24, 2023
Copy link
8000 Copy Markdown
Contributor Author

Thanks @mposolda, at first look the falling seems to happen when using the FetchOnServer, RunOnServer functions, causing problems in the ReverseProxy in getting system property or java.lang.NoClassDefFoundError: io/undertow/server/HttpHandler. Not yet thoroughly investigated to understand why.
As simplest solution I directly used the proxy only for the test method.

ggraziano ggraziano
Verification of iss at refresh token request
f061775 
Added iss checking using the existing TokenVerifier.RealmUrlCheck in the verifyRefreshToken method.

Closes keycloak#22191
@mposolda mposolda self-assigned this Oct 24, 2023
mposolda
mposolda approved these changes Oct 24, 2023
View reviewed changes
@mposolda mposolda left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Peps14 Looks good, hope tests are ok. Thanks!

@mposolda
mposolda merged commit 84112f5 into keycloak:main Oct 24, 2023
@cypress
cypress Bot commented Oct 25, 2023
Copy link
Copy Markdown

4 flaky tests on run #9567 ↗︎

0 1054 96 0 Flakiness 4

Details:

Verification of iss at refresh token request
Project: Keycloak Admin UI Commit: 84112f57b5
Status: Passed Duration: 13:05 💡
Started: Oct 25, 2023 12:27 AM Ended: Oct 25, 2023 12:40 AM
Flakiness  realm_settings_client_profiles_test.spec.ts • 1 flaky test • firefox

View Output Video

Test Artifacts
Realm settings client profiles tab tests > Should not create duplicate client profile Output Screenshots
Flakiness  clients_test.spec.ts • 2 flaky tests • chrome

View Output Video

Test Artifacts
Clients test > Keys tab test > Generate new keys Test Replay Output Screenshots
Clients test > Accessibility tests for clients > Check a11y violations on load/ clients list tab Test Replay Output Screenshots
Flakiness  realm_settings_general_tab_test.spec.ts • 1 flaky test • chrome

View Output Video

Test Artifacts
Realm settings general tab tests > Test all general tab switches Test Replay Output Screenshots

Review all test suite changes for PR #24212 ↗︎

@NilsEngelbach
NilsEngelbach commented Mar 7, 2024 •
edited
Loading
Copy link
Copy Markdown

This Change breaks refreshing tokens with dynamic hostnames:

#26017

Would it be possible to make it configurable, or enable it based on the hostname-strict option?

@graziang
graziang commented Mar 7, 2024
Copy link
Copy Markdown
Contributor Author

@NilsEngelbach Thanks, Can you open a new detailed issue?

@NilsEngelbach NilsEngelbach mentioned this pull request Mar 7, 2024
Closed
1 of 2 tasks
@graziang
graziang deleted the issue-22191 branch March 16, 2026 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mposolda mposolda mposolda approved these changes
@pedroigor pedroigor pedroigor left review comments
+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

@mposolda mposolda

Labels

flaky-test team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Verification of iss at refresh token request

4 participants

@graziang @mposolda @NilsEngelbach @pedroigor
0