Skip to content

Commit

Permalink
Report 401 and remove OIDC session cookie if it is malformed
Browse files Browse the repository at this point in the history
  • Loading branch information
@sberyozkin
sberyozkin committed Aug 24, 2023
1 parent b9b12d0 commit f3c08b3
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 23 deletions.
11 changes: 10 additions & 1 deletion ...sions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
}

Expand Down
46 changes: 26 additions & 20 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/DefaultTokenStateManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand Down
4 changes: 2 additions & 2 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
17 changes: 17 additions & 0 deletions integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand All @@ -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();
}
}

Expand Down

0 comments on commit f3c08b3

Please sign in to comment.