[JBEAP-28701] added use of nonce #2253

rsearls · 2025-02-11T14:06:21Z

https://issues.redhat.com/browse/JBEAP-28701

fjuma · 2025-02-11T18:07:54Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/Oidc.java

@@ -390,7 +395,11 @@ public enum AuthOutcome {
    }

    public static String generateId() {
-        return UUID.randomUUID().toString();


The generateId method is also used for generating a unique identifier for the JWT ID claim and for the state code. I think we should leave this existing method unchanged and introduce a new method for the nonce generation.

fjuma · 2025-02-11T18:10:07Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

@@ -222,7 +223,8 @@ protected String getRedirectUri(String state) {
            String redirectUri = rewrittenRedirectUri(url);
            URIBuilder redirectUriBuilder = new URIBuilder(deployment.getAuthUrl());
            redirectUriBuilder.addParameter(RESPONSE_TYPE, CODE)
-                    .addParameter(CLIENT_ID, deployment.getResourceName());
+                    .addParameter(CLIENT_ID, deployment.getResourceName())
+                    .addParameter(NONCE, String.valueOf(state.hashCode()));


We shouldn't be re-using the state value here. We should be using a separate value for the nonce.

fjuma · 2025-02-11T18:12:23Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

@@ -456,6 +459,14 @@ protected AuthChallenge resolveCode(String code) {
            log.error("Stale token");
            return challenge(HttpStatus.SC_FORBIDDEN, AuthenticationError.Reason.STALE_TOKEN, null);
        }
+
+        String stateCookieValue = getCookieValue(deployment.getStateCookieName());


A separate cookie should be used for nonce purposes.

fjuma · 2025-02-11T18:15:38Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+        String stateCookieValue = getCookieValue(deployment.getStateCookieName());
+        String nonceValue = token.getClaimValueAsString(NONCE);
+        if (nonceValue == null || !nonceValue.equals(String.valueOf(stateCookieValue.hashCode()))) {
+            log.error("Invalid nonce");


I think it would be better to validate the nonce in the TokenValidator#parseAndVerifyToken method here:

https://github.com/wildfly-security/wildfly-elytron/blob/2.x/http/oidc/src/main/java/org/wildfly/security/http/oidc/TokenValidator.java#L85

Its more straight forward to take the value from idToken which has been validated

fjuma · 2025-02-11T18:18:03Z

@rsearls Thanks for working on this! It would also be good to add some unit tests for the case where the ID token contains the correct nonce and the case where the ID token contains the wrong nonce.

fjuma · 2025-02-11T18:25:03Z

@rsearls Please update the base branch for this PR to the 2.6.x branch, thanks.

rsearls · 2025-02-17T14:11:01Z

All existing tests use the valid nonce case. I have add a test for the invalid case.

Once you are happy this the changes in this branch I will port them to 2.6.x

fjuma

Thanks @rsearls! I've added some comments, let me know if you have any questions.

fjuma · 2025-02-20T19:55:06Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

@@ -96,6 +100,9 @@ public class OidcRequestAuthenticator {
    protected String refreshToken;
    protected String strippedOauthParametersRequestUri;

+    private int NONCE_SIZE = 36;
+    private final String sessionId = generateSessionId();


Maybe we could name this sessionRandomValue or something like that?

The random value can be generated when creating the authentication request.

Variable renamed.

There are several reasons I made sessionId private final in class scope
rather than a method local variable. I wanted to keep the needed code
changes to this class to a minimum. The variable is used in 3 different
methods, getRedirectUri, createRequestWithRequestParameter, convertToRequestParameter.
Each method would require a signature change. The methods are called
in 5 different places also requiring a change.
I also thought that in the future some calling class might need access to the
value of sessionId. Having a class scope private final variable would enable
a consistent accurate reference.
However I can make the change you requested if you prefer.

fjuma · 2025-02-20T20:15:23Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

@@ -222,7 +229,8 @@ protected String getRedirectUri(String state) {
            String redirectUri = rewrittenRedirectUri(url);
            URIBuilder redirectUriBuilder = new URIBuilder(deployment.getAuthUrl());
            redirectUriBuilder.addParameter(RESPONSE_TYPE, CODE)
-                    .addParameter(CLIENT_ID, deployment.getResourceName());
+                    .addParameter(CLIENT_ID, deployment.getResourceName())
+                    .addParameter(NONCE, String.valueOf(sessionId.hashCode()));


Here, we need to use a cryptographic hash of the random value. We can use MessageDigest for this.

fjuma · 2025-02-20T20:16:17Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/Oidc.java

@@ -111,6 +111,7 @@ public class Oidc {
    public static final String REDIRECT_URI = "redirect_uri";
    public static final String REFRESH_TOKEN = "refresh_token";
    public static final String RESPONSE_TYPE = "response_type";
+    public static final String SESSION_ID = "session_id";


Maybe we can rename this to something like SESSION_RANDOM_VALUE="session_random_value"

fjuma · 2025-02-20T20:28:01Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

@@ -336,9 +346,16 @@ protected AuthChallenge checkStateCookie() {
            log.warn("state parameter was null");
            return challenge(HttpStatus.SC_BAD_REQUEST, AuthenticationError.Reason.INVALID_STATE_COOKIE, null);
        }
-        if (!state.equals(stateCookieValue)) {
+
+        String sCookieValue = stateCookieValue;


Why are these changes needed?

The checkStateCookie() method currently deals specifically with the deployment.getStateCookieName() (i.e., the OAuth_Token_Request_State cookie).

fjuma · 2025-02-20T20:29:39Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

@@ -444,6 +461,22 @@ protected AuthChallenge resolveCode(String code) {
            TokenValidator.VerifiedTokens verifiedTokens = tokenValidator.parseAndVerifyToken(idTokenString, tokenString);
            idToken = verifiedTokens.getIdToken();
            token = verifiedTokens.getAccessToken();
+
+            String stateCookieValue = getCookieValue(deployment.getStateCookieName());


Won't this get the value of the OAuth_Token_Request_State cookie instead of the session random value cookie?

This is related to the issue described for line 350.

fjuma · 2025-02-20T20:33:56Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+            String sessionIdValue = "";
+            for(int i=0; i < cookieArr.length; i++) {
+                String cookie = cookieArr[i].trim();
+                if (cookie.startsWith(SESSION_ID+"=")) {


Just to check, is this needed? I don't see a similar check being done in the code that obtains the state cookie value.

Cookie state is performed by the challenge returned at line 428 (i.e checkStateCookie().
This is needed to get at the value fo the session_id as described for line 350

fjuma · 2025-02-20T20:39:01Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+                }
+            }
+            String nonceValue = idToken.getNonce();
+            if (nonceValue == null || !nonceValue.equals(String.valueOf(sessionIdValue.hashCode()))) {


Here, we should use the cryptographic hash as mentioned above.

Because the nonce validation is part of verifying the ID token, I think it makes sense for this check to be done in the OIDC TokenValidator#parseAndVerifyToken method. From the TokenValidator we have access to the oidcClientConfiguration from which we can get the value of the session random value cookie and we can get the nonce from idJwtClaims.

fjuma · 2025-02-20T20:44:44Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

        jwtClaims.setClaim(STATE, state);
        jwtClaims.setClaim(REDIRECT_URI, redirectUri);
        jwtClaims.setClaim(RESPONSE_TYPE, CODE);
        jwtClaims.setClaim(CLIENT_ID, deployment.getResourceName());
+        jwtClaims.setClaim(NONCE, String.valueOf(sessionId.hashCode()));


Note, same comment as above regarding using a cryptographic hash.

fjuma · 2025-02-20T20:46:34Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+        SecureRandom random = new SecureRandom();
+        byte[] nonceData = new byte[NONCE_SIZE];
+        random.nextBytes(nonceData);
+        return String.valueOf(ByteIterator.ofBytes(nonceData)


I think this can be return ByteIterator.ofBytes(nonceData).base64Encode().drainToString();

fjuma · 2025-02-20T20:49:40Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java

+    // Generate an invalid sessionId so that the nonce check fails
+    public void testSessionIdNonceMismatch() throws Exception {
+        performAuthentication(
+                getOidcConfigurationInputStreamWithRequestParameter(REQUEST.getValue(), NONE, "", ""),


This test uses the request parameter configuration. It would be good to have an additional test case that doesn't use this configuration to test the standard authentication request case.

rsearls requested review from fjuma and Skyllarr as code owners February 11, 2025 14:06

fjuma requested changes Feb 11, 2025

View reviewed changes

rsearls force-pushed the JBEAP-28701-OIDC-Auth-Code branch from c4aec6e to 3f02a79 Compare February 17, 2025 13:59

fjuma requested changes Feb 20, 2025

View reviewed changes

[JBEAP-28701] added use of nonce

d6f32ce

rsearls force-pushed the JBEAP-28701-OIDC-Auth-Code branch from 3f02a79 to d6f32ce Compare February 25, 2025 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JBEAP-28701] added use of nonce #2253

[JBEAP-28701] added use of nonce #2253

rsearls commented Feb 11, 2025

fjuma Feb 11, 2025

rsearls Feb 17, 2025

fjuma Feb 11, 2025

rsearls Feb 17, 2025

fjuma Feb 11, 2025

rsearls Feb 17, 2025

fjuma Feb 11, 2025

rsearls Feb 17, 2025

fjuma commented Feb 11, 2025 •

edited

Loading

fjuma commented Feb 11, 2025

rsearls commented Feb 17, 2025

fjuma left a comment

fjuma Feb 20, 2025

fjuma Feb 20, 2025

rsearls Feb 24, 2025

fjuma Feb 20, 2025

rsearls Feb 25, 2025

fjuma Feb 20, 2025

rsearls Feb 24, 2025

fjuma Feb 20, 2025

fjuma Feb 20, 2025 •

edited

Loading

rsearls Feb 24, 2025

fjuma Feb 20, 2025 •

edited

Loading

rsearls Feb 24, 2025 •

edited

Loading

fjuma Feb 20, 2025

fjuma Feb 20, 2025 •

edited

Loading

rsearls Feb 25, 2025

fjuma Feb 20, 2025

rsearls Feb 25, 2025

fjuma Feb 20, 2025

rsearls Feb 25, 2025

fjuma Feb 20, 2025

rsearls Feb 25, 2025

[JBEAP-28701] added use of nonce #2253

Are you sure you want to change the base?

[JBEAP-28701] added use of nonce #2253

Conversation

rsearls commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma commented Feb 11, 2025 • edited Loading

fjuma commented Feb 11, 2025

rsearls commented Feb 17, 2025

fjuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

rsearls Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma commented Feb 11, 2025 •

edited

Loading

fjuma Feb 20, 2025 •

edited

Loading

fjuma Feb 20, 2025 •

edited

Loading

rsearls Feb 24, 2025 •

edited

Loading

fjuma Feb 20, 2025 •

edited

Loading