From df305ff12386cf28b33567b8d9a18db164f019dd Mon Sep 17 00:00:00 2001 From: Sergey Beryozkin Date: Tue, 16 May 2023 14:41:39 +0100 Subject: [PATCH 1/3] Encrypt OIDC session cookie value by default --- .../io/quarkus/oidc/OidcTenantConfig.java | 8 +-- .../runtime/CodeAuthenticationMechanism.java | 4 +- .../runtime/DefaultTokenStateManager.java | 4 +- .../io/quarkus/oidc/runtime/OidcUtils.java | 4 +- .../oidc/runtime/TenantConfigContext.java | 34 +++++++++---- .../src/main/resources/application.properties | 2 +- .../io/quarkus/it/keycloak/CodeFlowTest.java | 51 +++++++++++++------ .../keycloak/CustomTenantConfigResolver.java | 1 + .../src/main/resources/application.properties | 2 +- .../keycloak/CodeFlowAuthorizationTest.java | 31 +++++++---- 10 files changed, 97 insertions(+), 44 deletions(-) diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java index f017eadf2ea99..5ae98a40d3fd3 100644 --- a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java +++ b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java @@ -340,8 +340,8 @@ public enum Strategy { /** * Requires that the tokens are encrypted before being stored in the cookies. */ - @ConfigItem(defaultValueDocumentation = "false") - public Optional encryptionRequired = Optional.empty(); + @ConfigItem(defaultValue = "true") + public boolean encryptionRequired = true; /** * Secret which will be used to encrypt the tokens. @@ -351,12 +351,12 @@ public enum Strategy { @ConfigItem public Optional encryptionSecret = Optional.empty(); - public Optional isEncryptionRequired() { + public boolean isEncryptionRequired() { return encryptionRequired; } public void setEncryptionRequired(boolean encryptionRequired) { - this.encryptionRequired = Optional.of(encryptionRequired); + this.encryptionRequired = encryptionRequired; } public Optional getEncryptionSecret() { 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 1e8880d62490a..e8d0481f975c4 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 @@ -648,7 +648,9 @@ public Throwable apply(Throwable tInner) { LOG.debugf("Starting the final redirect"); return tInner; } - LOG.errorf("ID token verification has failed: %s", tInner.getMessage()); + String message = tInner.getCause() != null ? tInner.getCause().getMessage() + : tInner.getMessage(); + LOG.errorf("ID token verification has failed: %s", message); return new AuthenticationCompletionException(tInner); } }); 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 index 4e51635c34d53..080632be484b5 100644 --- 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 @@ -129,7 +129,7 @@ private static String getRefreshTokenCookieName(OidcTenantConfig oidcConfig) { } private String encryptToken(String token, RoutingContext context, OidcTenantConfig oidcConfig) { - if (oidcConfig.tokenStateManager.encryptionRequired.orElse(false)) { + if (oidcConfig.tokenStateManager.encryptionRequired) { TenantConfigContext configContext = context.get(TenantConfigContext.class.getName()); try { return OidcUtils.encryptString(token, configContext.getTokenEncSecretKey()); @@ -141,7 +141,7 @@ private String encryptToken(String token, RoutingContext context, OidcTenantConf } private String decryptToken(String token, RoutingContext context, OidcTenantConfig oidcConfig) { - if (oidcConfig.tokenStateManager.encryptionRequired.orElse(false)) { + if (oidcConfig.tokenStateManager.encryptionRequired) { TenantConfigContext configContext = context.get(TenantConfigContext.class.getName()); try { return OidcUtils.decryptString(token, configContext.getTokenEncSecretKey()); 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 d9747dd05f547..4213bff15ebaa 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 @@ -422,7 +422,7 @@ public static String encryptJson(JsonObject json, SecretKey key) throws Exceptio public static String encryptString(String jweString, SecretKey key) throws Exception { JsonWebEncryption jwe = new JsonWebEncryption(); - jwe.setAlgorithmHeaderValue(KeyEncryptionAlgorithm.A256KW.getAlgorithm()); + jwe.setAlgorithmHeaderValue(KeyEncryptionAlgorithm.A256GCMKW.getAlgorithm()); jwe.setEncryptionMethodHeaderParameter(ContentEncryptionAlgorithm.A256GCM.getAlgorithm()); jwe.setKey(key); jwe.setPlaintext(jweString); @@ -434,7 +434,7 @@ public static JsonObject decryptJson(String jweString, Key key) throws Exception } public static String decryptString(String jweString, Key key) throws Exception { - return decryptString(jweString, key, KeyEncryptionAlgorithm.A256KW); + return decryptString(jweString, key, KeyEncryptionAlgorithm.A256GCMKW); } public static String decryptString(String jweString, Key key, KeyEncryptionAlgorithm algorithm) throws JoseException { diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/TenantConfigContext.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/TenantConfigContext.java index 014d4513ce0b4..68396fa958e53 100644 --- a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/TenantConfigContext.java +++ b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/TenantConfigContext.java @@ -1,12 +1,20 @@ package io.quarkus.oidc.runtime; +import java.nio.charset.StandardCharsets; + +import javax.crypto.KeyGenerator; import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; + +import org.jboss.logging.Logger; +import io.quarkus.oidc.OIDCException; import io.quarkus.oidc.OidcTenantConfig; import io.quarkus.oidc.common.runtime.OidcCommonUtils; import io.smallrye.jwt.util.KeyUtils; public class TenantConfigContext { + private static final Logger LOG = Logger.getLogger(TenantConfigContext.class); /** * OIDC Provider @@ -39,8 +47,8 @@ public TenantConfigContext(OidcProvider client, OidcTenantConfig config, boolean this.oidcConfig = config; this.ready = ready; - pkceSecretKey = createPkceSecretKey(config); - tokenEncSecretKey = createTokenEncSecretKey(config); + pkceSecretKey = provider != null && provider.client != null ? createPkceSecretKey(config) : null; + tokenEncSecretKey = provider != null && provider.client != null ? createTokenEncSecretKey(config) : null; } private static SecretKey createPkceSecretKey(OidcTenantConfig config) { @@ -59,16 +67,24 @@ private static SecretKey createPkceSecretKey(OidcTenantConfig config) { } private static SecretKey createTokenEncSecretKey(OidcTenantConfig config) { - if (config.tokenStateManager.encryptionRequired.orElse(false)) { + if (config.tokenStateManager.encryptionRequired) { String encSecret = config.tokenStateManager.encryptionSecret .orElse(OidcCommonUtils.clientSecret(config.credentials)); - if (encSecret == null) { - throw new RuntimeException("Secret key for encrypting tokens is missing"); - } - if (encSecret.length() < 32) { - throw new RuntimeException("Secret key for encrypting tokens must be at least 32 characters long"); + try { + if (encSecret == null) { + LOG.warn("Secret key for encrypting tokens is missing, auto-generating it"); + KeyGenerator keyGenerator = KeyGenerator.getInstance("AES"); + keyGenerator.init(256); + return keyGenerator.generateKey(); + } + byte[] secretBytes = encSecret.getBytes(StandardCharsets.UTF_8); + if (secretBytes.length < 32) { + LOG.warn("Secret key for encrypting tokens should be 32 characters long"); + } + return new SecretKeySpec(OidcUtils.getSha256Digest(secretBytes), "AES"); + } catch (Exception ex) { + throw new OIDCException(ex); } - return KeyUtils.createSecretKeyFromSecret(encSecret); } return null; } 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 c18b1c767ed39..a9023990b1580 100644 --- a/integration-tests/oidc-code-flow/src/main/resources/application.properties +++ b/integration-tests/oidc-code-flow/src/main/resources/application.properties @@ -10,6 +10,7 @@ quarkus.oidc.authentication.cookie-domain=localhost quarkus.oidc.authentication.extra-params.max-age=60 quarkus.oidc.application-type=web-app quarkus.oidc.authentication.cookie-suffix=test +quarkus.oidc.token-state-manager.encryption-required=false # OIDC client configuration quarkus.oidc-client.auth-server-url=${quarkus.oidc.auth-server-url} @@ -143,7 +144,6 @@ quarkus.oidc.tenant-split-tokens.auth-server-url=${keycloak.url}/realms/quarkus quarkus.oidc.tenant-split-tokens.client-id=quarkus-app quarkus.oidc.tenant-split-tokens.credentials.secret=secret quarkus.oidc.tenant-split-tokens.token-state-manager.split-tokens=true -quarkus.oidc.tenant-split-tokens.token-state-manager.encryption-required=true quarkus.oidc.tenant-split-tokens.token-state-manager.encryption-secret=eUk1p7UB3nFiXZGUXi0uph1Y9p34YhBU quarkus.oidc.tenant-split-tokens.application-type=web-app 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 3f8132dd97a2a..b6bcd610b3aaa 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 @@ -16,6 +16,9 @@ import java.util.concurrent.Callable; import java.util.concurrent.TimeUnit; +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; + import org.hamcrest.Matchers; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; @@ -268,7 +271,15 @@ public void testCodeFlowForceHttpsRedirectUriWithQueryAndPkce() throws Exception Cookie sessionCookie = getSessionCookie(webClient, "tenant-https_test"); assertNotNull(sessionCookie); - JsonObject idToken = OidcUtils.decodeJwtContent(sessionCookie.getValue().split("\\|")[0]); + + String encryptedIdToken = sessionCookie.getValue().split("\\|")[0]; + + SecretKey key = new SecretKeySpec(OidcUtils + .getSha256Digest("secret".getBytes(StandardCharsets.UTF_8)), + "AES"); + String encodedIdToken = OidcUtils.decryptString(encryptedIdToken, key); + + JsonObject idToken = OidcUtils.decodeJwtContent(encodedIdToken); String expiresAt = idToken.getInteger("exp").toString(); page = webClient.getPage(endpointLocationWithoutQueryUri.toURL()); String response = page.getBody().asText(); @@ -827,7 +838,7 @@ public void testDefaultSessionManagerIdTokenOnly() throws IOException, Interrupt assertEquals("tenant-idtoken-only:no refresh", page.getBody().asText()); Cookie idTokenCookie = getSessionCookie(page.getWebClient(), "tenant-idtoken-only"); - checkSingleTokenCookie(idTokenCookie, "ID"); + checkSingleTokenCookie(idTokenCookie, "ID", "secret"); assertNull(getSessionAtCookie(webClient, "tenant-idtoken-only")); assertNull(getSessionRtCookie(webClient, "tenant-idtoken-only")); @@ -837,7 +848,7 @@ public void testDefaultSessionManagerIdTokenOnly() throws IOException, Interrupt } @Test - public void testDefaultSessionManagerIdRefreshTokens() throws IOException, InterruptedException { + public void testDefaultSessionManagerIdRefreshTokens() throws Exception { try (final WebClient webClient = createWebClient()) { HtmlPage page = webClient.getPage("http://localhost:8081/web-app/tenant-id-refresh-token"); assertNotNull(getStateCookie(webClient, "tenant-id-refresh-token")); @@ -858,11 +869,16 @@ public void testDefaultSessionManagerIdRefreshTokens() throws IOException, Inter assertEquals("tenant-id-refresh-token:RT injected", page.getBody().asText()); Cookie idTokenCookie = getSessionCookie(page.getWebClient(), "tenant-id-refresh-token"); + + SecretKey key = new SecretKeySpec(OidcUtils + .getSha256Digest("secret".getBytes(StandardCharsets.UTF_8)), + "AES"); + String[] parts = idTokenCookie.getValue().split("\\|"); assertEquals(3, parts.length); - assertEquals("ID", OidcUtils.decodeJwtContent(parts[0]).getString("typ")); + assertEquals("ID", OidcUtils.decodeJwtContent(OidcUtils.decryptString(parts[0], key)).getString("typ")); assertEquals("", parts[1]); - assertEquals("Refresh", OidcUtils.decodeJwtContent(parts[2]).getString("typ")); + assertEquals("Refresh", OidcUtils.decodeJwtContent(OidcUtils.decryptString(parts[2], key)).getString("typ")); assertNull(getSessionAtCookie(webClient, "tenant-id-refresh-token")); assertNull(getSessionRtCookie(webClient, "tenant-id-refresh-token")); @@ -893,14 +909,15 @@ public void testDefaultSessionManagerSplitTokens() throws IOException, Interrupt page = webClient.getPage("http://localhost:8081/web-app/refresh/tenant-split-tokens"); assertEquals("tenant-split-tokens:RT injected", page.getBody().asText()); + final String decryptSecret = "eUk1p7UB3nFiXZGUXi0uph1Y9p34YhBU"; Cookie idTokenCookie = getSessionCookie(page.getWebClient(), "tenant-split-tokens"); - checkSingleTokenCookie(idTokenCookie, "ID", true); + checkSingleTokenCookie(idTokenCookie, "ID", decryptSecret); Cookie atTokenCookie = getSessionAtCookie(page.getWebClient(), "tenant-split-tokens"); - checkSingleTokenCookie(atTokenCookie, "Bearer", true); + checkSingleTokenCookie(atTokenCookie, "Bearer", decryptSecret); Cookie rtTokenCookie = getSessionRtCookie(page.getWebClient(), "tenant-split-tokens"); - checkSingleTokenCookie(rtTokenCookie, "Refresh", true); + checkSingleTokenCookie(rtTokenCookie, "Refresh", decryptSecret); // verify all the cookies are cleared after the session timeout webClient.getOptions().setRedirectEnabled(false); @@ -949,12 +966,12 @@ public void testDefaultSessionManagerIdRefreshSplitTokens() throws IOException, assertEquals("tenant-split-id-refresh-token:RT injected", page.getBody().asText()); Cookie idTokenCookie = getSessionCookie(page.getWebClient(), "tenant-split-id-refresh-token"); - checkSingleTokenCookie(idTokenCookie, "ID"); + checkSingleTokenCookie(idTokenCookie, "ID", "secret"); assertNull(getSessionAtCookie(page.getWebClient(), "tenant-split-id-refresh-token")); Cookie rtTokenCookie = getSessionRtCookie(page.getWebClient(), "tenant-split-id-refresh-token"); - checkSingleTokenCookie(rtTokenCookie, "Refresh"); + checkSingleTokenCookie(rtTokenCookie, "Refresh", "secret"); // verify all the cookies are cleared after the session timeout webClient.getOptions().setRedirectEnabled(false); @@ -982,26 +999,30 @@ public Boolean call() throws Exception { } private void checkSingleTokenCookie(Cookie tokenCookie, String type) { - checkSingleTokenCookie(tokenCookie, type, false); + checkSingleTokenCookie(tokenCookie, type, null); } - private void checkSingleTokenCookie(Cookie tokenCookie, String type, boolean decrypt) { + private void checkSingleTokenCookie(Cookie tokenCookie, String type, String decryptSecret) { String[] cookieParts = tokenCookie.getValue().split("\\|"); assertEquals(1, cookieParts.length); String token = cookieParts[0]; String[] tokenParts = token.split("\\."); - if (decrypt) { + if (decryptSecret != null) { assertEquals(5, tokenParts.length); try { - token = OidcUtils.decryptString(token, KeyUtils.createSecretKeyFromSecret("eUk1p7UB3nFiXZGUXi0uph1Y9p34YhBU")); + SecretKey key = new SecretKeySpec(OidcUtils + .getSha256Digest(decryptSecret.getBytes(StandardCharsets.UTF_8)), + "AES"); + token = OidcUtils.decryptString(token, key); tokenParts = token.split("\\."); } catch (Exception ex) { fail("Token decryption has failed"); } } assertEquals(3, tokenParts.length); - assertEquals(type, OidcUtils.decodeJwtContent(token).getString("typ")); + JsonObject json = OidcUtils.decodeJwtContent(token); + assertEquals(type, json.getString("typ")); } @Test diff --git a/integration-tests/oidc-tenancy/src/main/java/io/quarkus/it/keycloak/CustomTenantConfigResolver.java b/integration-tests/oidc-tenancy/src/main/java/io/quarkus/it/keycloak/CustomTenantConfigResolver.java index 2c3b1347f85a5..148f486cc73b5 100644 --- a/integration-tests/oidc-tenancy/src/main/java/io/quarkus/it/keycloak/CustomTenantConfigResolver.java +++ b/integration-tests/oidc-tenancy/src/main/java/io/quarkus/it/keycloak/CustomTenantConfigResolver.java @@ -136,6 +136,7 @@ public OidcTenantConfig get() { config.setJwksPath(jwksUri); config.getToken().setIssuer("any"); config.tokenStateManager.setSplitTokens(true); + config.tokenStateManager.setEncryptionRequired(false); config.getAuthentication().setSessionAgeExtension(Duration.ofMinutes(1)); return config; } else if ("tenant-web-app-dynamic".equals(tenantId)) { diff --git a/integration-tests/oidc-wiremock/src/main/resources/application.properties b/integration-tests/oidc-wiremock/src/main/resources/application.properties index eadda60bd9e56..dd69032d0c773 100644 --- a/integration-tests/oidc-wiremock/src/main/resources/application.properties +++ b/integration-tests/oidc-wiremock/src/main/resources/application.properties @@ -36,7 +36,7 @@ quarkus.oidc.code-flow-encrypted-id-token-pem.token.decryption-key-location=priv quarkus.oidc.code-flow-form-post.auth-server-url=${keycloak.url}/realms/quarkus-form-post/ quarkus.oidc.code-flow-form-post.client-id=quarkus-web-app -quarkus.oidc.code-flow-form-post.credentials.secret=secret +quarkus.oidc.code-flow-form-post.credentials.secret=AyM1SysPpbyDfgZld3umj1qzKObwVMkoqQ-EstJQLr_T-1qS0gZH75aKtMN3Yj0iPS4hcgUuTwjAzZr1Z9CAow quarkus.oidc.code-flow-form-post.application-type=web-app quarkus.oidc.code-flow-form-post.authentication.response-mode=form_post quarkus.oidc.code-flow-form-post.discovery-enabled=false diff --git a/integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/CodeFlowAuthorizationTest.java b/integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/CodeFlowAuthorizationTest.java index 9bba7529a88ec..5446cd41231ff 100644 --- a/integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/CodeFlowAuthorizationTest.java +++ b/integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/CodeFlowAuthorizationTest.java @@ -12,8 +12,12 @@ import java.io.IOException; import java.net.URI; +import java.nio.charset.StandardCharsets; import java.util.Set; +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; + import org.junit.jupiter.api.Test; import com.gargoylesoftware.htmlunit.SilentCssErrorHandler; @@ -140,7 +144,7 @@ public void testCodeFlowFormPost() throws IOException { } @Test - public void testCodeFlowUserInfo() throws IOException { + public void testCodeFlowUserInfo() throws Exception { defineCodeFlowAuthorizationOauth2TokenStub(); doTestCodeFlowUserInfo("code-flow-user-info-only"); @@ -150,7 +154,7 @@ public void testCodeFlowUserInfo() throws IOException { doTestCodeFlowUserInfoCashedInIdToken(); } - private void doTestCodeFlowUserInfo(String tenantId) throws IOException { + private void doTestCodeFlowUserInfo(String tenantId) throws Exception { try (final WebClient webClient = createWebClient()) { webClient.getOptions().setRedirectEnabled(true); HtmlPage page = webClient.getPage("http://localhost:8081/" + tenantId); @@ -163,15 +167,26 @@ private void doTestCodeFlowUserInfo(String tenantId) throws IOException { assertEquals("alice:alice, cache size: 1", page.getBody().asText()); - Cookie sessionCookie = getSessionCookie(webClient, tenantId); - assertNotNull(sessionCookie); - JsonObject idTokenClaims = OidcUtils.decodeJwtContent(sessionCookie.getValue().split("\\|")[0]); + JsonObject idTokenClaims = decryptIdToken(webClient, tenantId); assertNull(idTokenClaims.getJsonObject(OidcUtils.USER_INFO_ATTRIBUTE)); webClient.getCookieManager().clearCookies(); } } - private void doTestCodeFlowUserInfoCashedInIdToken() throws IOException { + private JsonObject decryptIdToken(WebClient webClient, String tenantId) throws Exception { + Cookie sessionCookie = getSessionCookie(webClient, tenantId); + assertNotNull(sessionCookie); + String encryptedIdToken = sessionCookie.getValue().split("\\|")[0]; + + SecretKey key = new SecretKeySpec(OidcUtils + .getSha256Digest("AyM1SysPpbyDfgZld3umj1qzKObwVMkoqQ-EstJQLr_T-1qS0gZH75aKtMN3Yj0iPS4hcgUuTwjAzZr1Z9CAow" + .getBytes(StandardCharsets.UTF_8)), + "AES"); + String encodedIdToken = OidcUtils.decryptString(encryptedIdToken, key); + return OidcUtils.decodeJwtContent(encodedIdToken); + } + + private void doTestCodeFlowUserInfoCashedInIdToken() throws Exception { try (final WebClient webClient = createWebClient()) { webClient.getOptions().setRedirectEnabled(true); HtmlPage page = webClient.getPage("http://localhost:8081/code-flow-user-info-github-cached-in-idtoken"); @@ -184,9 +199,7 @@ private void doTestCodeFlowUserInfoCashedInIdToken() throws IOException { assertEquals("alice:alice, cache size: 0", page.getBody().asText()); - Cookie sessionCookie = getSessionCookie(webClient, "code-flow-user-info-github-cached-in-idtoken"); - assertNotNull(sessionCookie); - JsonObject idTokenClaims = OidcUtils.decodeJwtContent(sessionCookie.getValue().split("\\|")[0]); + JsonObject idTokenClaims = decryptIdToken(webClient, "code-flow-user-info-github-cached-in-idtoken"); assertNotNull(idTokenClaims.getJsonObject(OidcUtils.USER_INFO_ATTRIBUTE)); webClient.getCookieManager().clearCookies(); } From ccbcfdef432f3122ca52c1cd3c077b6ff62fd13f Mon Sep 17 00:00:00 2001 From: Sergey Beryozkin Date: Wed, 29 Mar 2023 13:42:08 +0100 Subject: [PATCH 2/3] Create a single encrypted session cookie value --- ...ity-openid-connect-web-authentication.adoc | 10 ++++--- .../oidc/common/runtime/OidcCommonUtils.java | 6 +++- .../oidc/test/CodeFlowDevModeTestCase.java | 2 +- .../runtime/DefaultTokenStateManager.java | 29 +++++++++++++------ .../oidc/runtime/TenantConfigContext.java | 3 ++ .../io/quarkus/it/keycloak/CodeFlowTest.java | 16 +++++----- .../keycloak/CodeFlowAuthorizationTest.java | 7 +++-- 7 files changed, 49 insertions(+), 24 deletions(-) diff --git a/docs/src/main/asciidoc/security-openid-connect-web-authentication.adoc b/docs/src/main/asciidoc/security-openid-connect-web-authentication.adoc index cc1436778512f..b11090fb5d45b 100644 --- a/docs/src/main/asciidoc/security-openid-connect-web-authentication.adoc +++ b/docs/src/main/asciidoc/security-openid-connect-web-authentication.adoc @@ -571,7 +571,7 @@ Note that some endpoints do not require the access token. An access token is onl If the ID, access and refresh tokens are JWT tokens then combining all of them (if the strategy is the default `keep-all-tokens`) or only ID and refresh tokens (if the strategy is `id-refresh-token`) may produce a session cookie value larger than 4KB and the browsers may not be able to keep this cookie. In such cases, you can use `quarkus.oidc.token-state-manager.split-tokens=true` to have a unique session token per each of these tokens. -You can also configure the default `TokenStateManager` to encrypt the tokens before storing them as cookie values which may be necessary if the tokens contain sensitive claim values. +Note that `TokenStateManager` will encrypt the tokens before storing them in the session cookie. For example, here is how you configure it to split the tokens and encrypt them: [source, properties] @@ -581,12 +581,14 @@ quarkus.oidc.client-id=quarkus-app quarkus.oidc.credentials.secret=secret quarkus.oidc.application-type=web-app quarkus.oidc.token-state-manager.split-tokens=true -quarkus.oidc.token-state-manager.encryption-required=true quarkus.oidc.token-state-manager.encryption-secret=eUk1p7UB3nFiXZGUXi0uph1Y9p34YhBU ---- -The token encryption secret must be 32 characters long. Note that you only have to set `quarkus.oidc.token-state-manager.encryption-secret` if you prefer not to use -`quarkus.oidc.credentials.secret` for encrypting the tokens or if `quarkus.oidc.credentials.secret` length is less than 32 characters. +The token encryption secret must be at least 32 characters long. If this key is not configured then either `quarkus.oidc.credentials.secret` or `quarkus.oidc.credentials.jwt.secret` will be hashed to create an encryption key. + +`quarkus.oidc.token-state-manager.encryption-secret` should be configured if Quarkus authenticates to OpenId Connect Provider using either mTLS or `private_key_jwt` (where a private RSA or EC key is used to sign a JWT token) authentication methods, otherwise a random key will be generated which will be problematic if the Quarkus application is running in the cloud with multiple pods managing the requests. + +If you need you can disable encrypting the tokens in the session cookie with `quarkus.oidc.token-state-manager.encryption-required=false`. Register your own `io.quarkus.oidc.TokenStateManager' implementation as an `@ApplicationScoped` CDI bean if you need to customize the way the tokens are associated with the session cookie. For example, you may want to keep the tokens in a database and have only a database pointer stored in a session cookie. Note though that it may present some challenges in making the tokens available across multiple microservices nodes. diff --git a/extensions/oidc-common/runtime/src/main/java/io/quarkus/oidc/common/runtime/OidcCommonUtils.java b/extensions/oidc-common/runtime/src/main/java/io/quarkus/oidc/common/runtime/OidcCommonUtils.java index 6afd8a4664209..21a1321b95b00 100644 --- a/extensions/oidc-common/runtime/src/main/java/io/quarkus/oidc/common/runtime/OidcCommonUtils.java +++ b/extensions/oidc-common/runtime/src/main/java/io/quarkus/oidc/common/runtime/OidcCommonUtils.java @@ -276,6 +276,10 @@ public static String clientSecret(Credentials creds) { return creds.secret.orElse(creds.clientSecret.value.orElseGet(fromCredentialsProvider(creds.clientSecret.provider))); } + public static String jwtSecret(Credentials creds) { + return creds.jwt.secret.orElseGet(fromCredentialsProvider(creds.jwt.secretProvider)); + } + public static Secret.Method clientSecretMethod(Credentials creds) { return creds.clientSecret.method.orElseGet(() -> Secret.Method.BASIC); } @@ -300,7 +304,7 @@ public String get() { public static Key clientJwtKey(Credentials creds) { if (creds.jwt.secret.isPresent() || creds.jwt.secretProvider.key.isPresent()) { return KeyUtils - .createSecretKeyFromSecret(creds.jwt.secret.orElseGet(fromCredentialsProvider(creds.jwt.secretProvider))); + .createSecretKeyFromSecret(jwtSecret(creds)); } else { Key key = null; try { diff --git a/extensions/oidc/deployment/src/test/java/io/quarkus/oidc/test/CodeFlowDevModeTestCase.java b/extensions/oidc/deployment/src/test/java/io/quarkus/oidc/test/CodeFlowDevModeTestCase.java index 1684c3ee95b83..741953eecd1d8 100644 --- a/extensions/oidc/deployment/src/test/java/io/quarkus/oidc/test/CodeFlowDevModeTestCase.java +++ b/extensions/oidc/deployment/src/test/java/io/quarkus/oidc/test/CodeFlowDevModeTestCase.java @@ -97,7 +97,7 @@ public void testAccessAndRefreshTokenInjectionDevMode() throws IOException, Inte assertEquals("alice", page.getBody().asText()); - assertEquals("custom", page.getWebClient().getCookieManager().getCookie("q_session").getValue().split("\\|")[3]); + assertEquals("custom", page.getWebClient().getCookieManager().getCookie("q_session").getValue().split("\\|")[1]); webClient.getOptions().setRedirectEnabled(false); WebResponse webResponse = webClient 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 index 080632be484b5..13f18aab9bb80 100644 --- 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 @@ -21,14 +21,19 @@ public class DefaultTokenStateManager implements TokenStateManager { @Override public Uni createTokenState(RoutingContext routingContext, OidcTenantConfig oidcConfig, AuthorizationCodeTokens tokens, OidcRequestContext requestContext) { + + boolean encryptAll = !oidcConfig.tokenStateManager.splitTokens; + StringBuilder sb = new StringBuilder(); - sb.append(encryptToken(tokens.getIdToken(), routingContext, oidcConfig)); + sb.append(encryptAll ? tokens.getIdToken() : encryptToken(tokens.getIdToken(), routingContext, oidcConfig)); if (oidcConfig.tokenStateManager.strategy == OidcTenantConfig.TokenStateManager.Strategy.KEEP_ALL_TOKENS) { if (!oidcConfig.tokenStateManager.splitTokens) { sb.append(CodeAuthenticationMechanism.COOKIE_DELIM) - .append(encryptToken(tokens.getAccessToken(), routingContext, oidcConfig)) + .append(encryptAll ? tokens.getAccessToken() + : encryptToken(tokens.getAccessToken(), routingContext, oidcConfig)) .append(CodeAuthenticationMechanism.COOKIE_DELIM) - .append(encryptToken(tokens.getRefreshToken(), routingContext, oidcConfig)); + .append(encryptAll ? tokens.getRefreshToken() + : encryptToken(tokens.getRefreshToken(), routingContext, oidcConfig)); } else { CodeAuthenticationMechanism.createCookie(routingContext, oidcConfig, @@ -48,7 +53,8 @@ public Uni createTokenState(RoutingContext routingContext, OidcTenantCon sb.append(CodeAuthenticationMechanism.COOKIE_DELIM) .append("") .append(CodeAuthenticationMechanism.COOKIE_DELIM) - .append(encryptToken(tokens.getRefreshToken(), routingContext, oidcConfig)); + .append(encryptAll ? tokens.getRefreshToken() + : encryptToken(tokens.getRefreshToken(), routingContext, oidcConfig)); } else { if (tokens.getRefreshToken() != null) { CodeAuthenticationMechanism.createCookie(routingContext, @@ -59,21 +65,26 @@ public Uni createTokenState(RoutingContext routingContext, OidcTenantCon } } } - return Uni.createFrom().item(sb.toString()); + String state = encryptAll ? encryptToken(sb.toString(), routingContext, oidcConfig) : sb.toString(); + return Uni.createFrom().item(state); } @Override public Uni getTokens(RoutingContext routingContext, OidcTenantConfig oidcConfig, String tokenState, OidcRequestContext requestContext) { + boolean decryptAll = !oidcConfig.tokenStateManager.splitTokens; + + tokenState = decryptAll ? decryptToken(tokenState, routingContext, oidcConfig) : tokenState; + String[] tokens = CodeAuthenticationMechanism.COOKIE_PATTERN.split(tokenState); - String idToken = decryptToken(tokens[0], routingContext, oidcConfig); + 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 = decryptToken(tokens[1], routingContext, oidcConfig); - refreshToken = decryptToken(tokens[2], routingContext, oidcConfig); + 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) { @@ -86,7 +97,7 @@ public Uni getTokens(RoutingContext routingContext, Oid } } else if (oidcConfig.tokenStateManager.strategy == OidcTenantConfig.TokenStateManager.Strategy.ID_REFRESH_TOKENS) { if (!oidcConfig.tokenStateManager.splitTokens) { - refreshToken = decryptToken(tokens[2], routingContext, oidcConfig); + refreshToken = decryptAll ? tokens[2] : decryptToken(tokens[2], routingContext, oidcConfig); } else { Cookie rtCookie = getRefreshTokenCookie(routingContext, oidcConfig); if (rtCookie != null) { diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/TenantConfigContext.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/TenantConfigContext.java index 68396fa958e53..c25e570e38902 100644 --- a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/TenantConfigContext.java +++ b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/TenantConfigContext.java @@ -70,6 +70,9 @@ private static SecretKey createTokenEncSecretKey(OidcTenantConfig config) { if (config.tokenStateManager.encryptionRequired) { String encSecret = config.tokenStateManager.encryptionSecret .orElse(OidcCommonUtils.clientSecret(config.credentials)); + if (encSecret == null) { + encSecret = OidcCommonUtils.jwtSecret(config.credentials); + } try { if (encSecret == null) { LOG.warn("Secret key for encrypting tokens is missing, auto-generating it"); 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 b6bcd610b3aaa..4db9672f9b6e8 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 @@ -272,12 +272,12 @@ public void testCodeFlowForceHttpsRedirectUriWithQueryAndPkce() throws Exception Cookie sessionCookie = getSessionCookie(webClient, "tenant-https_test"); assertNotNull(sessionCookie); - String encryptedIdToken = sessionCookie.getValue().split("\\|")[0]; - SecretKey key = new SecretKeySpec(OidcUtils .getSha256Digest("secret".getBytes(StandardCharsets.UTF_8)), "AES"); - String encodedIdToken = OidcUtils.decryptString(encryptedIdToken, key); + String sessionCookieValue = OidcUtils.decryptString(sessionCookie.getValue(), key); + + String encodedIdToken = sessionCookieValue.split("\\|")[0]; JsonObject idToken = OidcUtils.decodeJwtContent(encodedIdToken); String expiresAt = idToken.getInteger("exp").toString(); @@ -868,17 +868,19 @@ public void testDefaultSessionManagerIdRefreshTokens() throws Exception { page = webClient.getPage("http://localhost:8081/web-app/refresh/tenant-id-refresh-token"); assertEquals("tenant-id-refresh-token:RT injected", page.getBody().asText()); - Cookie idTokenCookie = getSessionCookie(page.getWebClient(), "tenant-id-refresh-token"); + Cookie sessionCookie = getSessionCookie(page.getWebClient(), "tenant-id-refresh-token"); SecretKey key = new SecretKeySpec(OidcUtils .getSha256Digest("secret".getBytes(StandardCharsets.UTF_8)), "AES"); - String[] parts = idTokenCookie.getValue().split("\\|"); + String sessionCookieValue = OidcUtils.decryptString(sessionCookie.getValue(), key); + + String[] parts = sessionCookieValue.split("\\|"); assertEquals(3, parts.length); - assertEquals("ID", OidcUtils.decodeJwtContent(OidcUtils.decryptString(parts[0], key)).getString("typ")); + assertEquals("ID", OidcUtils.decodeJwtContent(parts[0]).getString("typ")); assertEquals("", parts[1]); - assertEquals("Refresh", OidcUtils.decodeJwtContent(OidcUtils.decryptString(parts[2], key)).getString("typ")); + assertEquals("Refresh", OidcUtils.decodeJwtContent(parts[2]).getString("typ")); assertNull(getSessionAtCookie(webClient, "tenant-id-refresh-token")); assertNull(getSessionRtCookie(webClient, "tenant-id-refresh-token")); diff --git a/integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/CodeFlowAuthorizationTest.java b/integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/CodeFlowAuthorizationTest.java index 5446cd41231ff..c377d827501be 100644 --- a/integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/CodeFlowAuthorizationTest.java +++ b/integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/CodeFlowAuthorizationTest.java @@ -176,13 +176,16 @@ private void doTestCodeFlowUserInfo(String tenantId) throws Exception { private JsonObject decryptIdToken(WebClient webClient, String tenantId) throws Exception { Cookie sessionCookie = getSessionCookie(webClient, tenantId); assertNotNull(sessionCookie); - String encryptedIdToken = sessionCookie.getValue().split("\\|")[0]; SecretKey key = new SecretKeySpec(OidcUtils .getSha256Digest("AyM1SysPpbyDfgZld3umj1qzKObwVMkoqQ-EstJQLr_T-1qS0gZH75aKtMN3Yj0iPS4hcgUuTwjAzZr1Z9CAow" .getBytes(StandardCharsets.UTF_8)), "AES"); - String encodedIdToken = OidcUtils.decryptString(encryptedIdToken, key); + + String decryptedSessionCookie = OidcUtils.decryptString(sessionCookie.getValue(), key); + + String encodedIdToken = decryptedSessionCookie.split("\\|")[0]; + return OidcUtils.decodeJwtContent(encodedIdToken); } From a56acfe55b675c24ef4de1f1887b2f722ebea888 Mon Sep 17 00:00:00 2001 From: Guillaume Smet Date: Wed, 17 May 2023 14:12:22 +0200 Subject: [PATCH 3/3] Higher timeout for Security1 native tests --- .github/native-tests.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/native-tests.json b/.github/native-tests.json index 9017057d9634c..7cb793157b4a5 100644 --- a/.github/native-tests.json +++ b/.github/native-tests.json @@ -68,7 +68,7 @@ }, { "category": "Security1", - "timeout": 50, + "timeout": 60, "test-modules": "elytron-security-oauth2, elytron-security, elytron-security-jdbc, elytron-undertow, elytron-security-ldap, bouncycastle, bouncycastle-jsse, bouncycastle-fips", "os-name": "ubuntu-latest" },