Merge pull request #35542 from sberyozkin/oidc_malformed_session_cookie

Report 401 and remove OIDC session cookie if it is malformed
quarkusio · Aug 24, 2023 · 8d924ba · 8d924ba
2 parents 4abf865 + f3c08b3
commit 8d924ba
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 23 deletions.
diff --git a/...sions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java b/...sions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java
@@ -304,6 +304,15 @@ private Uni<SecurityIdentity> reAuthenticate(Cookie sessionCookie,
         context.put(TenantConfigContext.class.getName(), configContext);
         return resolver.getTokenStateManager().getTokens(context, configContext.oidcConfig,
                 sessionCookie.getValue(), getTokenStateRequestContext)
+                .onFailure(AuthenticationCompletionException.class)
+                .recoverWithUni(
+                        new Function<Throwable, Uni<? extends AuthorizationCodeTokens>>() {
+                            @Override
+                            public Uni<AuthorizationCodeTokens> apply(Throwable t) {
+                                return removeSessionCookie(context, configContext.oidcConfig)
+                                        .replaceWith(Uni.createFrom().failure(t));
+                            }
+                        })
                 .chain(new Function<AuthorizationCodeTokens, Uni<? extends SecurityIdentity>>() {
                     @Override
                     public Uni<? extends SecurityIdentity> apply(AuthorizationCodeTokens session) {
@@ -846,7 +855,7 @@ private static boolean prepareNonceForVerification(RoutingContext context, OidcT
         }
     }
 
-    private static Object errorMessage(Throwable t) {
+    private static String errorMessage(Throwable t) {
         return t.getCause() != null ? t.getCause().getMessage() : t.getMessage();
     }
 

diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/DefaultTokenStateManager.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/DefaultTokenStateManager.java
@@ -6,6 +6,7 @@
 import io.quarkus.oidc.OidcRequestContext;
 import io.quarkus.oidc.OidcTenantConfig;
 import io.quarkus.oidc.TokenStateManager;
+import io.quarkus.security.AuthenticationCompletionException;
 import io.quarkus.security.AuthenticationFailedException;
 import io.smallrye.mutiny.Uni;
 import io.vertx.core.http.Cookie;
@@ -77,33 +78,38 @@ public Uni<AuthorizationCodeTokens> getTokens(RoutingContext routingContext, Oid
         tokenState = decryptAll ? decryptToken(tokenState, routingContext, oidcConfig) : tokenState;
 
         String[] tokens = CodeAuthenticationMechanism.COOKIE_PATTERN.split(tokenState);
+
         String idToken = decryptAll ? tokens[0] : decryptToken(tokens[0], routingContext, oidcConfig);
 
         String accessToken = null;
         String refreshToken = null;
-        if (oidcConfig.tokenStateManager.strategy == OidcTenantConfig.TokenStateManager.Strategy.KEEP_ALL_TOKENS) {
-            if (!oidcConfig.tokenStateManager.splitTokens) {
-                accessToken = decryptAll ? tokens[1] : decryptToken(tokens[1], routingContext, oidcConfig);
-                refreshToken = decryptAll ? tokens[2] : decryptToken(tokens[2], routingContext, oidcConfig);
-            } else {
-                Cookie atCookie = getAccessTokenCookie(routingContext, oidcConfig);
-                if (atCookie != null) {
-                    accessToken = decryptToken(atCookie.getValue(), routingContext, oidcConfig);
-                }
-                Cookie rtCookie = getRefreshTokenCookie(routingContext, oidcConfig);
-                if (rtCookie != null) {
-                    refreshToken = decryptToken(rtCookie.getValue(), routingContext, oidcConfig);
+        try {
+            if (oidcConfig.tokenStateManager.strategy == OidcTenantConfig.TokenStateManager.Strategy.KEEP_ALL_TOKENS) {
+                if (!oidcConfig.tokenStateManager.splitTokens) {
+                    accessToken = decryptAll ? tokens[1] : decryptToken(tokens[1], routingContext, oidcConfig);
+                    refreshToken = decryptAll ? tokens[2] : decryptToken(tokens[2], routingContext, oidcConfig);
+                } else {
+                    Cookie atCookie = getAccessTokenCookie(routingContext, oidcConfig);
+                    if (atCookie != null) {
+                        accessToken = decryptToken(atCookie.getValue(), routingContext, oidcConfig);
+                    }
+                    Cookie rtCookie = getRefreshTokenCookie(routingContext, oidcConfig);
+                    if (rtCookie != null) {
+                        refreshToken = decryptToken(rtCookie.getValue(), routingContext, oidcConfig);
+                    }
                 }
-            }
-        } else if (oidcConfig.tokenStateManager.strategy == OidcTenantConfig.TokenStateManager.Strategy.ID_REFRESH_TOKENS) {
-            if (!oidcConfig.tokenStateManager.splitTokens) {
-                refreshToken = decryptAll ? tokens[2] : decryptToken(tokens[2], routingContext, oidcConfig);
-            } else {
-                Cookie rtCookie = getRefreshTokenCookie(routingContext, oidcConfig);
-                if (rtCookie != null) {
-                    refreshToken = decryptToken(rtCookie.getValue(), routingContext, oidcConfig);
+            } else if (oidcConfig.tokenStateManager.strategy == OidcTenantConfig.TokenStateManager.Strategy.ID_REFRESH_TOKENS) {
+                if (!oidcConfig.tokenStateManager.splitTokens) {
+                    refreshToken = decryptAll ? tokens[2] : decryptToken(tokens[2], routingContext, oidcConfig);
+                } else {
+                    Cookie rtCookie = getRefreshTokenCookie(routingContext, oidcConfig);
+                    if (rtCookie != null) {
+                        refreshToken = decryptToken(rtCookie.getValue(), routingContext, oidcConfig);
+                    }
                 }
             }
+        } catch (ArrayIndexOutOfBoundsException ex) {
+            return Uni.createFrom().failure(new AuthenticationCompletionException("Session cookie is malformed"));
         }
 
         return Uni.createFrom().item(new AuthorizationCodeTokens(idToken, accessToken, refreshToken));

diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcProvider.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcProvider.java
@@ -301,9 +301,9 @@ public TokenIntrospection apply(TokenIntrospection introspectionResult, Throwabl
                             throw new AuthenticationFailedException(t);
                         }
                         if (!introspectionResult.isActive()) {
-                            LOG.debugf("Token issued to client %s is not active", oidcConfig.clientId.get());
                             verifyTokenExpiry(introspectionResult.getLong(OidcConstants.INTROSPECTION_TOKEN_EXP));
-                            throw new AuthenticationFailedException();
+                            throw new AuthenticationFailedException(
+                                    String.format("Token issued to client %s is not active", oidcConfig.clientId.get()));
                         }
                         verifyTokenExpiry(introspectionResult.getLong(OidcConstants.INTROSPECTION_TOKEN_EXP));
                         try {

diff --git a/integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java b/integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java
@@ -733,6 +733,7 @@ public void testIdTokenInjection() throws IOException {
 
             Cookie sessionCookie = getSessionCookie(webClient, null);
             assertNotNull(sessionCookie);
+            // Replace the session cookie with the correctly formatted cookie but with invalid token values
             webClient.getCookieManager().clearCookies();
             webClient.getCookieManager().addCookie(new Cookie(sessionCookie.getDomain(), sessionCookie.getName(),
                     "1|2|3"));
@@ -747,6 +748,22 @@ public void testIdTokenInjection() throws IOException {
                 assertNull(getSessionCookie(webClient, null));
             }
             webClient.getCookieManager().clearCookies();
+
+            // Replace the session cookie with malformed cookie
+            webClient.getCookieManager().clearCookies();
+            webClient.getCookieManager().addCookie(new Cookie(sessionCookie.getDomain(), sessionCookie.getName(),
+                    "1"));
+            sessionCookie = getSessionCookie(webClient, null);
+            assertEquals("1", sessionCookie.getValue());
+
+            try {
+                webClient.getPage("http://localhost:8081/web-app");
+                fail("401 status error is expected");
+            } catch (FailingHttpStatusCodeException ex) {
+                assertEquals(401, ex.getStatusCode());
+                assertNull(getSessionCookie(webClient, null));
+            }
+            webClient.getCookieManager().clearCookies();
         }
     }