From 3ffa6ecee00d1c6d030139434d63a3b3d5e2d973 Mon Sep 17 00:00:00 2001 From: Guillaume Smet Date: Wed, 21 Aug 2024 18:39:20 +0200 Subject: [PATCH 1/2] Encode URL in OIDC cookie Fix #31802 --- .../quarkus/oidc/runtime/CodeAuthenticationMechanism.java | 6 ++++-- .../src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java index 7ce98b9559606..2e982380f61fc 100644 --- a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java +++ b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java @@ -4,6 +4,8 @@ import static io.quarkus.oidc.runtime.OidcIdentityProvider.REFRESH_TOKEN_GRANT_RESPONSE; import java.net.URI; +import java.net.URLDecoder; +import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import java.security.PrivateKey; import java.security.SecureRandom; @@ -940,7 +942,7 @@ private CodeAuthenticationStateBean getCodeAuthenticationBean(String[] parsedSta Authentication authentication = configContext.oidcConfig.authentication; boolean pkceRequired = authentication.pkceRequired.orElse(false); if (!pkceRequired && !authentication.nonceRequired) { - bean.setRestorePath(parsedStateCookieValue[1]); + bean.setRestorePath(URLDecoder.decode(parsedStateCookieValue[1], StandardCharsets.UTF_8)); return bean; } @@ -1177,7 +1179,7 @@ private String encodeExtraStateValue(CodeAuthenticationStateBean extraStateValue throw new AuthenticationCompletionException(ex); } } else { - return extraStateValue.getRestorePath(); + return URLEncoder.encode(extraStateValue.getRestorePath(), StandardCharsets.UTF_8); } } 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 index 3b1e45505da7c..efc8f9016876e 100644 --- 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 @@ -11,6 +11,7 @@ import java.io.IOException; import java.net.URI; +import java.net.URLDecoder; import java.nio.charset.StandardCharsets; import java.time.Duration; import java.util.Base64; @@ -1561,12 +1562,12 @@ private String getStateCookieStateParam(Cookie stateCookie) { private String getStateCookieSavedPath(WebClient webClient, String tenantId) { String[] parts = getStateCookie(webClient, tenantId).getValue().split("\\|"); - return parts.length == 2 ? parts[1] : null; + return parts.length == 2 ? URLDecoder.decode(parts[1], StandardCharsets.UTF_8) : null; } private String getStateCookieSavedPath(Cookie stateCookie) { String[] parts = stateCookie.getValue().split("\\|"); - return parts.length == 2 ? parts[1] : null; + return parts.length == 2 ? URLDecoder.decode(parts[1], StandardCharsets.UTF_8) : null; } private Cookie getSessionCookie(WebClient webClient, String tenantId) { From 65c3611368c25650d4ab4fb8e1628085cf7d2386 Mon Sep 17 00:00:00 2001 From: Sergey Beryozkin Date: Thu, 29 Aug 2024 15:56:16 +0100 Subject: [PATCH 2/2] Use Base64URL encoded JSON to store unencrypted OIDC state cookie value --- .../runtime/CodeAuthenticationMechanism.java | 19 +++++---- .../io/quarkus/oidc/runtime/OidcUtils.java | 3 +- .../src/main/resources/application.properties | 2 +- .../io/quarkus/it/keycloak/CodeFlowTest.java | 40 ++++++++++++++++--- 4 files changed, 48 insertions(+), 16 deletions(-) diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java index 2e982380f61fc..a9c5dc6c4f86b 100644 --- a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java +++ b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java @@ -4,8 +4,6 @@ import static io.quarkus.oidc.runtime.OidcIdentityProvider.REFRESH_TOKEN_GRANT_RESPONSE; import java.net.URI; -import java.net.URLDecoder; -import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import java.security.PrivateKey; import java.security.SecureRandom; @@ -71,7 +69,6 @@ public class CodeAuthenticationMechanism extends AbstractOidcAuthenticationMecha static final String EQ = "="; static final String COOKIE_DELIM = "|"; static final Pattern COOKIE_PATTERN = Pattern.compile("\\" + COOKIE_DELIM); - static final String STATE_COOKIE_RESTORE_PATH = "restore-path"; static final Uni VOID_UNI = Uni.createFrom().voidItem(); static final String NO_OIDC_COOKIES_AVAILABLE = "no_oidc_cookies"; @@ -940,13 +937,16 @@ private CodeAuthenticationStateBean getCodeAuthenticationBean(String[] parsedSta if (parsedStateCookieValue.length == 2) { CodeAuthenticationStateBean bean = new CodeAuthenticationStateBean(); Authentication authentication = configContext.oidcConfig.authentication; + boolean pkceRequired = authentication.pkceRequired.orElse(false); if (!pkceRequired && !authentication.nonceRequired) { - bean.setRestorePath(URLDecoder.decode(parsedStateCookieValue[1], StandardCharsets.UTF_8)); + JsonObject json = new JsonObject(OidcUtils.base64UrlDecode(parsedStateCookieValue[1])); + bean.setRestorePath(json.getString(OidcUtils.STATE_COOKIE_RESTORE_PATH)); return bean; } JsonObject json = null; + try { json = OidcUtils.decryptJson(parsedStateCookieValue[1], configContext.getStateEncryptionKey()); } catch (Exception ex) { @@ -954,7 +954,7 @@ private CodeAuthenticationStateBean getCodeAuthenticationBean(String[] parsedSta configContext.oidcConfig.tenantId.get()); throw new AuthenticationCompletionException(ex); } - bean.setRestorePath(json.getString(STATE_COOKIE_RESTORE_PATH)); + bean.setRestorePath(json.getString(OidcUtils.STATE_COOKIE_RESTORE_PATH)); bean.setCodeVerifier(json.getString(OidcConstants.PKCE_CODE_VERIFIER)); bean.setNonce(json.getString(OidcConstants.NONCE)); return bean; @@ -1161,8 +1161,9 @@ private boolean isRestorePath(Authentication auth) { } private String encodeExtraStateValue(CodeAuthenticationStateBean extraStateValue, TenantConfigContext configContext) { + JsonObject json = new JsonObject(); + if (extraStateValue.getCodeVerifier() != null || extraStateValue.getNonce() != null) { - JsonObject json = new JsonObject(); if (extraStateValue.getCodeVerifier() != null) { json.put(OidcConstants.PKCE_CODE_VERIFIER, extraStateValue.getCodeVerifier()); } @@ -1170,7 +1171,7 @@ private String encodeExtraStateValue(CodeAuthenticationStateBean extraStateValue json.put(OidcConstants.NONCE, extraStateValue.getNonce()); } if (extraStateValue.getRestorePath() != null) { - json.put(STATE_COOKIE_RESTORE_PATH, extraStateValue.getRestorePath()); + json.put(OidcUtils.STATE_COOKIE_RESTORE_PATH, extraStateValue.getRestorePath()); } try { return OidcUtils.encryptJson(json, configContext.getStateEncryptionKey()); @@ -1179,7 +1180,9 @@ private String encodeExtraStateValue(CodeAuthenticationStateBean extraStateValue throw new AuthenticationCompletionException(ex); } } else { - return URLEncoder.encode(extraStateValue.getRestorePath(), StandardCharsets.UTF_8); + json.put(OidcUtils.STATE_COOKIE_RESTORE_PATH, extraStateValue.getRestorePath()); + + return Base64.getUrlEncoder().withoutPadding().encodeToString(json.encode().getBytes(StandardCharsets.UTF_8)); } } diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcUtils.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcUtils.java index db3b03456ab6f..6773643e284e8 100644 --- a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcUtils.java +++ b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcUtils.java @@ -75,6 +75,7 @@ public final class OidcUtils { private static final Logger LOG = Logger.getLogger(OidcUtils.class); + public static final String STATE_COOKIE_RESTORE_PATH = "restore-path"; public static final String CONFIG_METADATA_ATTRIBUTE = "configuration-metadata"; public static final String USER_INFO_ATTRIBUTE = "userinfo"; public static final String INTROSPECTION_ATTRIBUTE = "introspection"; @@ -252,7 +253,7 @@ private static JsonObject decodeAsJsonObject(String encodedContent) { } } - private static String base64UrlDecode(String encodedContent) { + public static String base64UrlDecode(String encodedContent) { return new String(Base64.getUrlDecoder().decode(encodedContent), StandardCharsets.UTF_8); } diff --git a/integration-tests/oidc-code-flow/src/main/resources/application.properties b/integration-tests/oidc-code-flow/src/main/resources/application.properties index 41372ae76857a..273e767de6583 100644 --- a/integration-tests/oidc-code-flow/src/main/resources/application.properties +++ b/integration-tests/oidc-code-flow/src/main/resources/application.properties @@ -173,7 +173,7 @@ quarkus.oidc.tenant-split-tokens.token-state-manager.encryption-secret=eUk1p7UB3 quarkus.oidc.tenant-split-tokens.application-type=web-app quarkus.oidc.tenant-split-tokens.authentication.cookie-same-site=strict -quarkus.http.auth.permission.roles1.paths=/index.html +quarkus.http.auth.permission.roles1.paths=/index.html,/index.html;/checktterer quarkus.http.auth.permission.roles1.policy=authenticated quarkus.http.auth.permission.logout.paths=/tenant-logout 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 index efc8f9016876e..886fd30414843 100644 --- 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 @@ -11,7 +11,6 @@ import java.io.IOException; import java.net.URI; -import java.net.URLDecoder; import java.nio.charset.StandardCharsets; import java.time.Duration; import java.util.Base64; @@ -1082,6 +1081,29 @@ public void testAccessTokenInjection() throws IOException { } } + @Test + public void testInvalidPath() throws IOException { + try (final WebClient webClient = createWebClient()) { + HtmlPage page = webClient.getPage("http://localhost:8081/index.html;/checktterer"); + assertEquals("/index.html;/checktterer", getStateCookieSavedPath(webClient, null)); + + assertEquals("Sign in to quarkus", page.getTitleText()); + + HtmlForm loginForm = page.getForms().get(0); + + loginForm.getInputByName("username").setValueAttribute("alice"); + loginForm.getInputByName("password").setValueAttribute("alice"); + + try { + page = loginForm.getInputByName("login").click(); + } catch (FailingHttpStatusCodeException ex) { + assertEquals(404, ex.getStatusCode()); + } + + webClient.getCookieManager().clearCookies(); + } + } + @Test public void testAccessAndRefreshTokenInjection() throws IOException { try (final WebClient webClient = createWebClient()) { @@ -1387,8 +1409,8 @@ public void testAccessAndRefreshTokenInjectionWithoutIndexHtmlAndListenerMultiTa @Test public void testAccessAndRefreshTokenInjectionWithQuery() throws Exception { try (final WebClient webClient = createWebClient()) { - HtmlPage page = webClient.getPage("http://localhost:8081/web-app/refresh-query?a=aValue"); - assertEquals("/web-app/refresh-query?a=aValue", getStateCookieSavedPath(webClient, null)); + HtmlPage page = webClient.getPage("http://localhost:8081/web-app/refresh-query?a=aValue%"); + assertEquals("/web-app/refresh-query?a=aValue%25", getStateCookieSavedPath(webClient, null)); assertEquals("Sign in to quarkus", page.getTitleText()); @@ -1399,7 +1421,8 @@ public void testAccessAndRefreshTokenInjectionWithQuery() throws Exception { page = loginForm.getInputByName("login").click(); - assertEquals("RT injected:aValue", page.getBody().asNormalizedText()); + // Query parameters are decoded by the time they reach the JAX-RS endpoint + assertEquals("RT injected:aValue%", page.getBody().asNormalizedText()); webClient.getCookieManager().clearCookies(); } } @@ -1562,12 +1585,17 @@ private String getStateCookieStateParam(Cookie stateCookie) { private String getStateCookieSavedPath(WebClient webClient, String tenantId) { String[] parts = getStateCookie(webClient, tenantId).getValue().split("\\|"); - return parts.length == 2 ? URLDecoder.decode(parts[1], StandardCharsets.UTF_8) : null; + return parts.length == 2 ? getSavedPathFromJson(parts[1]) : null; } private String getStateCookieSavedPath(Cookie stateCookie) { String[] parts = stateCookie.getValue().split("\\|"); - return parts.length == 2 ? URLDecoder.decode(parts[1], StandardCharsets.UTF_8) : null; + return parts.length == 2 ? getSavedPathFromJson(parts[1]) : null; + } + + private String getSavedPathFromJson(String value) { + JsonObject json = new JsonObject(OidcUtils.base64UrlDecode(value)); + return json.getString(OidcUtils.STATE_COOKIE_RESTORE_PATH); } private Cookie getSessionCookie(WebClient webClient, String tenantId) {