From c64ad8aca0ba4344e570d1e7c249e30a9234c376 Mon Sep 17 00:00:00 2001 From: Sergey Beryozkin Date: Mon, 3 Jul 2023 18:50:52 +0100 Subject: [PATCH] Simplify and improve OIDC PKCE secret initialization --- .../security-openid-connect-providers.adoc | 10 +++ .../oidc/test/CodeFlowDevModeTestCase.java | 49 ++++++++++++- .../resources/application-dev-mode.properties | 5 +- .../io/quarkus/oidc/OidcTenantConfig.java | 13 +++- .../oidc/runtime/TenantConfigContext.java | 68 ++++++++++++++----- .../io/quarkus/it/keycloak/CodeFlowTest.java | 6 +- 6 files changed, 127 insertions(+), 24 deletions(-) diff --git a/docs/src/main/asciidoc/security-openid-connect-providers.adoc b/docs/src/main/asciidoc/security-openid-connect-providers.adoc index d4ce993016c23..bd3ade4344cc8 100644 --- a/docs/src/main/asciidoc/security-openid-connect-providers.adoc +++ b/docs/src/main/asciidoc/security-openid-connect-providers.adoc @@ -343,6 +343,16 @@ quarkus.oidc.client-id= quarkus.oidc.credentials.secret= ---- +[NOTE] +==== +Twitter provider requires Proof Key for Code Exchange (PKCE) which is supported by the `quarkus.oidc.provider=twitter` declaration. +Quarkus has to encrypt the current PKCE code verifier in a state cookie while the authorization code flow with Twitter is in progress and it will +generate a secure random secret key for encrypting it. + +You can provide your own secret key for encrypting the PKCE code verifier if you prefer with the `quarkus.oidc.authentication.pkce-secret` property but +note that this secret should be 32 characters long, and an error will be reported if it is less than 16 characters long. +==== + === Spotify Create a https://developer.spotify.com/documentation/general/guides/authorization/app-settings/[Spotify application]: 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 1aafda2c92309..976e555325ebe 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 @@ -1,12 +1,24 @@ package io.quarkus.oidc.test; +import static org.awaitility.Awaitility.given; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; +import java.io.BufferedReader; +import java.io.ByteArrayInputStream; import java.io.IOException; +import java.io.InputStreamReader; import java.net.URI; - +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.awaitility.core.ThrowingRunnable; import org.eclipse.microprofile.config.spi.ConfigSource; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -107,6 +119,8 @@ public void testAccessAndRefreshTokenInjectionDevMode() throws IOException, Inte webClient.getCookieManager().clearCookies(); } + checkPkceSecretGenerated(); + } private void useTenantConfigResolver() throws IOException, InterruptedException { @@ -144,4 +158,37 @@ private WebClient createWebClient() { webClient.setCssErrorHandler(new SilentCssErrorHandler()); return webClient; } + + protected static void checkPkceSecretGenerated() { + AtomicBoolean checkPassed = new AtomicBoolean(); + given().pollInterval(100, TimeUnit.MILLISECONDS) + .atMost(10, TimeUnit.SECONDS) + .untilAsserted(new ThrowingRunnable() { + @Override + public void run() throws Throwable { + final Path logDirectory = Paths.get(".", "target"); + Path accessLogFilePath = logDirectory.resolve("quarkus.log"); + boolean fileExists = Files.exists(accessLogFilePath); + if (!fileExists) { + accessLogFilePath = logDirectory.resolve("target/quarkus.log"); + fileExists = Files.exists(accessLogFilePath); + } + assertTrue(fileExists, "quarkus log is missing"); + + try (BufferedReader reader = new BufferedReader( + new InputStreamReader(new ByteArrayInputStream(Files.readAllBytes(accessLogFilePath)), + StandardCharsets.UTF_8))) { + String line = null; + while ((line = reader.readLine()) != null) { + if (line.contains( + "Secret key for encrypting PKCE code verifier is missing, auto-generating it")) { + checkPassed.set(true); + } + } + } + } + }); + assertTrue(checkPassed.get(), "Can not confirm Secret key for encrypting PKCE code verifier has been generated"); + } + } diff --git a/extensions/oidc/deployment/src/test/resources/application-dev-mode.properties b/extensions/oidc/deployment/src/test/resources/application-dev-mode.properties index fad0040fdfdbb..26c7ff4e26bd1 100644 --- a/extensions/oidc/deployment/src/test/resources/application-dev-mode.properties +++ b/extensions/oidc/deployment/src/test/resources/application-dev-mode.properties @@ -6,5 +6,8 @@ quarkus.oidc.credentials.client-secret.provider.name=vault-secret-provider quarkus.oidc.credentials.client-secret.provider.key=secret-from-vault-typo quarkus.oidc.application-type=web-app quarkus.oidc.logout.path=/protected/logout - +quarkus.oidc.authentication.pkce-required=true quarkus.log.category."com.gargoylesoftware.htmlunit.javascript.host.css.CSSStyleSheet".level=FATAL +quarkus.log.category."io.quarkus.oidc.runtime.TenantConfigContext".level=DEBUG +quarkus.log.file.enable=true + 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 fc83fabbc2c7c..35dcae0e411df 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 @@ -915,8 +915,17 @@ public enum ResponseMode { /** * Secret which will be used to encrypt a Proof Key for Code Exchange (PKCE) code verifier in the code flow state. - * This secret must be set if PKCE is required but no client secret is set. - * The length of the secret which will be used to encrypt the code verifier must be 32 characters long. + * This secret should be at least 32 characters long. + *

+ * If this secret is not set, the client secret configured with + * either `quarkus.oidc.credentials.secret` or `quarkus.oidc.credentials.client-secret.value` will be checked. + * Finally, `quarkus.oidc.credentials.jwt.secret` which can be used for `client_jwt_secret` authentication will be + * checked. Client secret will not be used as a PKCE code verifier encryption secret if it is less than 32 characters + * long. + *

+ * The secret will be auto-generated if it remains uninitialized after checking all of these properties. + *

+ * Error will be reported if the secret length is less than 16 characters. */ @ConfigItem public Optional pkceSecret = Optional.empty(); 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 42d8373b2a0d5..066f4f5e6fb4b 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 @@ -11,7 +11,6 @@ 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); @@ -54,15 +53,41 @@ public TenantConfigContext(OidcProvider client, OidcTenantConfig config, boolean private static SecretKey createPkceSecretKey(OidcTenantConfig config) { if (config.authentication.pkceRequired.orElse(false)) { - String pkceSecret = config.authentication.pkceSecret - .orElse(OidcCommonUtils.clientSecret(config.credentials)); - if (pkceSecret == null) { - throw new RuntimeException("Secret key for encrypting PKCE code verifier is missing"); + String pkceSecret = null; + if (config.authentication.pkceSecret.isPresent()) { + pkceSecret = config.authentication.pkceSecret.get(); + } else { + LOG.debug("'quarkus.oidc.token-state-manager.encryption-secret' is not configured, " + + "trying to use the configured client secret"); + String possiblePkceSecret = fallbackToClientSecret(config); + if (possiblePkceSecret != null && possiblePkceSecret.length() < 32) { + LOG.debug("Client secret is less than 32 characters long, the pkce secret will be generated"); + } else { + pkceSecret = possiblePkceSecret; + } } - if (pkceSecret.length() != 32) { - throw new RuntimeException("Secret key for encrypting PKCE code verifier must be 32 characters long"); + try { + if (pkceSecret == null) { + LOG.debug("Secret key for encrypting PKCE code verifier is missing, auto-generating it"); + SecretKey key = generateSecretKey(); + return key; + } + byte[] secretBytes = pkceSecret.getBytes(StandardCharsets.UTF_8); + if (secretBytes.length < 32) { + String errorMessage = "Secret key for encrypting PKCE code verifier in a state cookie should be at least 32 characters long" + + " for the strongest state cookie encryption to be produced." + + " Please update 'quarkus.oidc.authentication.pkce-secret' or update the configured client secret."; + if (secretBytes.length < 16) { + throw new RuntimeException( + "Secret key for encrypting PKCE code verifier is less than 32 characters long"); + } else { + LOG.debug(errorMessage); + } + } + return new SecretKeySpec(OidcUtils.getSha256Digest(secretBytes), "AES"); + } catch (Exception ex) { + throw new OIDCException(ex); } - return KeyUtils.createSecretKeyFromSecret(pkceSecret); } return null; } @@ -75,19 +100,12 @@ private static SecretKey createTokenEncSecretKey(OidcTenantConfig config) { } else { LOG.debug("'quarkus.oidc.token-state-manager.encryption-secret' is not configured, " + "trying to use the configured client secret"); - encSecret = OidcCommonUtils.clientSecret(config.credentials); - if (encSecret == null) { - LOG.debug("Client secret is not configured, " - + "trying to use the configured 'client_jwt_secret' secret"); - encSecret = OidcCommonUtils.jwtSecret(config.credentials); - } + encSecret = fallbackToClientSecret(config); } try { if (encSecret == null) { LOG.warn("Secret key for encrypting tokens in a session cookie is missing, auto-generating it"); - KeyGenerator keyGenerator = KeyGenerator.getInstance("AES"); - keyGenerator.init(256); - return keyGenerator.generateKey(); + return generateSecretKey(); } byte[] secretBytes = encSecret.getBytes(StandardCharsets.UTF_8); if (secretBytes.length < 32) { @@ -111,6 +129,22 @@ private static SecretKey createTokenEncSecretKey(OidcTenantConfig config) { return null; } + private static String fallbackToClientSecret(OidcTenantConfig config) { + String encSecret = OidcCommonUtils.clientSecret(config.credentials); + if (encSecret == null) { + LOG.debug("Client secret is not configured, " + + "trying to use the configured 'client_jwt_secret' secret"); + encSecret = OidcCommonUtils.jwtSecret(config.credentials); + } + return encSecret; + } + + private static SecretKey generateSecretKey() throws Exception { + KeyGenerator keyGenerator = KeyGenerator.getInstance("AES"); + keyGenerator.init(256); + return keyGenerator.generateKey(); + } + public OidcTenantConfig getOidcTenantConfig() { return oidcConfig; } 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 25bab959a85d6..085b1ac532f88 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 @@ -38,7 +38,6 @@ import io.quarkus.test.junit.QuarkusTest; import io.quarkus.test.keycloak.client.KeycloakTestClient; import io.restassured.RestAssured; -import io.smallrye.jwt.util.KeyUtils; import io.vertx.core.json.JsonObject; /** @@ -358,8 +357,9 @@ public void testCodeFlowForceHttpsRedirectUriAndPkceMissingCodeVerifier() throws private void verifyCodeVerifier(Cookie stateCookie, String keycloakUrl) throws Exception { String encodedState = stateCookie.getValue().split("\\|")[1]; - String codeVerifier = OidcUtils.decryptJson(encodedState, - KeyUtils.createSecretKeyFromSecret("eUk1p7UB3nFiXZGUXi0uph1Y9p34YhBU")).getString("code_verifier"); + byte[] secretBytes = "eUk1p7UB3nFiXZGUXi0uph1Y9p34YhBU".getBytes(StandardCharsets.UTF_8); + SecretKey key = new SecretKeySpec(OidcUtils.getSha256Digest(secretBytes), "AES"); + String codeVerifier = OidcUtils.decryptJson(encodedState, key).getString("code_verifier"); String codeChallenge = Base64.getUrlEncoder().withoutPadding() .encodeToString(OidcUtils.getSha256Digest(codeVerifier.getBytes(StandardCharsets.US_ASCII)));