[ELY-2903] skip nonce check on oidc client refresh. Check nonce on oi… by rsearls · Pull Request #2277 · wildfly-security/wildfly-elytron

rsearls · 2025-04-22T19:47:54Z

…dc request authentication

https://issues.redhat.com/browse/ELY-2903

fjuma · 2025-04-22T20:00:57Z

            try {
                TokenValidator tokenValidator = TokenValidator.builder(clientConfiguration).build();
-                TokenValidator.VerifiedTokens verifiedTokens = tokenValidator.parseAndVerifyToken(idTokenString, accessTokenString, cookie);
+                TokenValidator.VerifiedTokens verifiedTokens = tokenValidator.parseAndVerifyToken(idTokenString, accessTokenString, cookie, false);


With the approach I mentioned below, we could leave this unchanged (and the nonce won't get validated).

fjuma

We should add a test case for the refresh token case to validate this fix.

fjuma · 2025-04-22T20:01:56Z

@rsearls Just to check, were you able to reproduce the issue with Keycloak?

fjuma · 2025-04-22T20:42:53Z

    }

+    public VerifiedTokens parseAndVerifyToken(final String idToken, final String accessToken,
+                                              OidcHttpFacade.Cookie cookie) throws OidcException {


Just realized, I guess the cookie is something that is specifically used for the nonce validation case, right? So perhaps there's some more cleanup that would be good to do so this method would just need to take the idToken and accessToken.

Looking back at the original PR, RefreshableOidcSecurityContext was updated so that it was possible to pass it the session random value cookie but that's no longer needed now.

rsearls

requested changes pushed

rsearls · 2025-04-22T21:26:52Z

    }

+    public VerifiedTokens parseAndVerifyToken(final String idToken, final String accessToken,
+                                              OidcHttpFacade.Cookie cookie) throws OidcException {


rsearls · 2025-04-25T20:32:03Z

test case pushded

fjuma · 2025-04-29T16:44:51Z

    private static final int ACCESS_TOKEN_LIFESPAN = 120;
    private static final int SESSION_MAX_LIFESPAN = 120;
+    private static final int REFRESH_ACCESS_TOKEN_LIFESPAN = 10;
+    private static final int REFRESH_SESSION_MAX_LIFESPAN = 10;


Just curious, is there a reason these low numbers are needed here if we are forcing the refresh to happen regardless?

Git CI was failing on every test platform due to time over run for this test.
With a lifespan of 120. testRefreshToken was taking 35sec to run and all the
tests in the file 2+mins. Reducing the lifespan for this test enabled the test file
to run in about 1.5 min.

Do we know what was causing the test to take longer to run in that case? Because we're calling refreshToken with checkActive set to false, I'm just wondering why changing the access token lifespan would impact the test duration.

I tried using a lifespan of 120 locally but I don't seem to see a significant difference in the overall time the test takes to run compared to when the lifespan is set to 10.

I think we could try setting the lifespan back to 120 and see if the CI issue occurs. If it does, we can then take a closer look to see what's happening. I can't think of why the token lifespan would be affecting the test duration here.

another job has failed.
https://github.com/wildfly-security/wildfly-elytron/actions/runs/14781730898
This job just failed with 1 Bind Address already in use: NET_Bind (window-11)
All other jogs canceled (I'm assuming because none responsive)
I have restarted the job.

Thanks, Rebecca. Yes, I see the bind address in use message in the tests. Thanks for re-triggering. We should create an ELY issue for this intermittent bind address problem on Windows. Looks like the tests that I've seen fail with this intermitent issue are ElytronAuthenticatorTest, MaskedPasswordSaslAuthenticationTest, JwtSecurityRealmTest, OAuth2SaslClientV10Test, and OAuth2SaslClientV11Test.

I don't see this as an elytron issue. It's an issue with the github CI env. We see this type of failure in the middle of the (U.S.) day not in the morning or evening. Most likely the system is under heavy load at that time which causes communication delays. Perhaps we should discuss this with the github CI maintenance team first.

I'm wondering if it could be related to something happening in a previous test where something doesn't get stopped/cleaned up in time before these tests start or something like that.

This is always a possibility. Most likely it would be very hard to find as the failures are random and not reproducable locally. Perhaps asking the performance team to evaluate elytron.

fjuma · 2025-04-29T16:45:41Z

+                "    \"" + REALM + "\" : \"" + TEST_REFRESH_REALM + "\",\n" +
+                "    \"" + RESOURCE + "\" : \"" + CLIENT_ID + "\",\n" +
+                "    \"" + PUBLIC_CLIENT + "\" : \"false\",\n" +
+                "    \"" + AUTH_SERVER_URL + "\" : \"" + authServerUrl + "\",\n" +


It would be good to use the client-id, provider-url configuration instead in this config.

fjuma · 2025-04-29T16:46:07Z

    }

-    public RefreshableOidcSecurityContext(OidcClientConfiguration clientConfiguration, OidcHttpFacade.Cookie cookie, OidcTokenStore tokenStore, String tokenString,
+    public RefreshableOidcSecurityContext(OidcClientConfiguration clientConfiguration, OidcTokenStore tokenStore, String tokenString,


Just an FYI for other reviewers, this isn't public API.

rsearls · 2025-05-01T13:15:22Z

Requested changes complete

fjuma · 2025-05-01T14:07:04Z

+                            OidcAccount account = (OidcAccount) session.getAttachment(OidcAccount.class.getName());
+                            assertNotNull("No OidcAccount for token refresh", account);
+                            RefreshableOidcSecurityContext refreshableContext = account.getOidcSecurityContext();
+                            assertNotNull("No RefreshableOidcSecurityContext for token refresh", account);


s/account/refreshableContext

fjuma · 2025-05-01T14:08:22Z

+    }
+    private InputStream getOidcRefreshConfigurationInputStream(String clientSecret, String authServerUrl) {
+        String oidcConfig = "{\n" +
+                "    \"" + REALM + "\" : \"" + TEST_REALM + "\",\n" +


The realm isn't needed here (since the provider-url is being specified and that contains the realm)

fjuma · 2025-05-01T18:29:27Z

Thanks @rsearls! This is looking good. You can go ahead and squash the commits. Thanks.

fjuma · 2025-05-01T19:04:41Z

    public static final String CLIENT_SECRET = "longerclientsecretthatisstleast256bitslong";
    public static KeycloakContainer KEYCLOAK_CONTAINER;
    public static final String TEST_REALM = "WildFly";
+    public static final String TEST_REFRESH_REALM = "WildFlyRefresh";


Just one last small comment - if this is unused, it can be removed.

…dc request authentication

fjuma

Thanks very much @rsearls!

darranl

Hi @rsearls thank you for getting to the end of this one, as we need to include the fix in a couple of streams can you please send a PR to the 2.2.x branch and I will take it from there.

rsearls · 2025-05-05T12:31:41Z

branch 2.2.x pr #2282

skyllarr · 2025-07-08T08:52:18Z

Hi @rsearls , just checking if this PR can be closed? Since it is resolved in 2.6.4.Final

rsearls · 2025-07-08T14:10:14Z

closing as this was resolved in 2.6.4.Final

rsearls · 2025-07-08T14:11:06Z

Yes. I closed it. Thanks.

…

On Tue, Jul 8, 2025 at 4:52 AM Diana Křepinská ***@***.***> wrote: *Skyllarr* left a comment (wildfly-security/wildfly-elytron#2277) <#2277 (comment)> Hi @rsearls <https://github.com/rsearls> , just checking if this PR can be closed? Since it is resolved in 2.6.4.Final — Reply to this email directly, view it on GitHub <#2277 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD3X45EFK4WQPQVEOSVBQL3HOBFRAVCNFSM6AAAAAB3URPSVSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTANBXHE3DKMBQGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

rsearls requested review from fjuma and skyllarr as code owners April 22, 2025 19:47

fjuma reviewed Apr 22, 2025

View reviewed changes

Comment thread http/oidc/src/main/java/org/wildfly/security/http/oidc/TokenValidator.java

fjuma reviewed Apr 22, 2025

View reviewed changes

fjuma requested changes Apr 22, 2025

View reviewed changes

rsearls force-pushed the ELY-2903-refresh-tokens-skip-nonce branch from 504e217 to ddcdf0e Compare April 22, 2025 20:06

fjuma reviewed Apr 22, 2025

View reviewed changes

rsearls force-pushed the ELY-2903-refresh-tokens-skip-nonce branch from ddcdf0e to f1167c9 Compare April 22, 2025 21:26

rsearls commented Apr 22, 2025

View reviewed changes

rsearls force-pushed the ELY-2903-refresh-tokens-skip-nonce branch from f1167c9 to 0157e01 Compare April 25, 2025 20:29

rsearls force-pushed the ELY-2903-refresh-tokens-skip-nonce branch from 0157e01 to 71fa1b4 Compare April 28, 2025 20:50

fjuma reviewed Apr 29, 2025

View reviewed changes

fjuma reviewed May 1, 2025

View reviewed changes

rsearls force-pushed the ELY-2903-refresh-tokens-skip-nonce branch from 8890357 to 681cfcc Compare May 1, 2025 18:33

fjuma reviewed May 1, 2025

View reviewed changes

[ELY-2903] skip nonce check on oidc client refresh. Check nonce on oi…

dab06c1

…dc request authentication

rsearls force-pushed the ELY-2903-refresh-tokens-skip-nonce branch from 681cfcc to dab06c1 Compare May 1, 2025 19:11

fjuma approved these changes May 1, 2025

View reviewed changes

fjuma added the +1 FJ label May 1, 2025

fjuma requested a review from darranl May 1, 2025 19:20

darranl requested changes May 3, 2025

View reviewed changes

rsearls closed this Jul 8, 2025

Conversation

rsearls commented Apr 22, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjuma left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjuma commented Apr 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsearls left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rsearls commented Apr 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsearls commented May 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjuma commented May 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjuma left a comment

Choose a reason for hiding this comment

Uh oh!

darranl left a comment

Choose a reason for hiding this comment

Uh oh!

rsearls commented May 5, 2025

Uh oh!

skyllarr commented Jul 8, 2025

Uh oh!

rsearls commented Jul 8, 2025

Uh oh!

rsearls commented Jul 8, 2025 via email

fjuma left a comment •

edited

Loading

rsearls left a comment •

edited

Loading