From f471a110cd092c066adbc71aa7a5aeede859e839 Mon Sep 17 00:00:00 2001 From: stianst Date: Tue, 28 Sep 2021 09:01:02 +0200 Subject: [PATCH] KEYCLOAK-19408 Better client secrets --- .../adapters/installed/KeycloakInstalled.java | 5 +- .../keycloak/common/util/RandomString.java | 66 ----------------- .../keycloak/common/util/SecretGenerator.java | 71 +++++++++++++++++++ ...finispanAuthenticationSessionProvider.java | 4 +- .../MapRootAuthenticationSessionAdapter.java | 4 +- .../MapDeploymentStateProviderFactory.java | 4 +- .../resources/QuarkusWelcomeResource.java | 4 +- .../DefaultOAuth2DeviceUserCodeProvider.java | 6 +- .../models/utils/KeycloakModelUtils.java | 19 ++--- .../keycloak/models/UserCredentialModel.java | 4 +- .../requiredactions/ConsoleVerifyEmail.java | 4 +- .../broker/oidc/OIDCIdentityProvider.java | 3 +- ...ractGeneratedSecretKeyProviderFactory.java | 3 +- .../device/endpoints/DeviceEndpoint.java | 4 +- .../protocol/oidc/utils/PkceUtils.java | 4 +- .../managers/AuthenticationManager.java | 3 +- .../services/managers/CodeGenerateUtil.java | 3 +- .../services/resources/WelcomeResource.java | 4 +- .../resources/admin/ClientResource.java | 8 ++- .../WebAuthnRegisterAndLoginTest.java | 4 +- 20 files changed, 113 insertions(+), 114 deletions(-) delete mode 100644 common/src/main/java/org/keycloak/common/util/RandomString.java create mode 100644 common/src/main/java/org/keycloak/common/util/SecretGenerator.java diff --git a/adapters/oidc/installed/src/main/java/org/keycloak/adapters/installed/KeycloakInstalled.java b/adapters/oidc/installed/src/main/java/org/keycloak/adapters/installed/KeycloakInstalled.java index 8632e7ec3a..e188f6ea75 100644 --- a/adapters/oidc/installed/src/main/java/org/keycloak/adapters/installed/KeycloakInstalled.java +++ b/adapters/oidc/installed/src/main/java/org/keycloak/adapters/installed/KeycloakInstalled.java @@ -30,7 +30,6 @@ import java.net.URI; import java.net.URISyntaxException; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; -import java.security.SecureRandom; import java.util.Deque; import java.util.Locale; import java.util.Map; @@ -57,7 +56,7 @@ import org.keycloak.adapters.rotation.AdapterTokenVerifier; import org.keycloak.common.VerificationException; import org.keycloak.common.util.Base64Url; import org.keycloak.common.util.KeycloakUriBuilder; -import org.keycloak.common.util.RandomString; +import org.keycloak.common.util.SecretGenerator; import org.keycloak.representations.AccessToken; import org.keycloak.representations.AccessTokenResponse; import org.keycloak.representations.IDToken; @@ -770,7 +769,7 @@ public class KeycloakInstalled { public static Pkce generatePkce() { try { - String codeVerifier = new RandomString(PKCE_CODE_VERIFIER_MAX_LENGTH, new SecureRandom()).nextString(); + String codeVerifier = SecretGenerator.getInstance().randomString(PKCE_CODE_VERIFIER_MAX_LENGTH); String codeChallenge = generateS256CodeChallenge(codeVerifier); return new Pkce(codeVerifier, codeChallenge); } catch (Exception ex){ diff --git a/common/src/main/java/org/keycloak/common/util/RandomString.java b/common/src/main/java/org/keycloak/common/util/RandomString.java deleted file mode 100644 index 70ce02d794..0000000000 --- a/common/src/main/java/org/keycloak/common/util/RandomString.java +++ /dev/null @@ -1,66 +0,0 @@ -package org.keycloak.common.util; - -import java.security.SecureRandom; -import java.util.Locale; -import java.util.Objects; -import java.util.Random; - -public class RandomString { - - /** - * Generate a random string. - */ - public String nextString() { - for (int idx = 0; idx < buf.length; ++idx) - buf[idx] = symbols[random.nextInt(symbols.length)]; - return new String(buf); - } - - public static final String upper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; - - public static final String lower = upper.toLowerCase(Locale.ROOT); - - public static final String digits = "0123456789"; - - public static final String alphanum = upper + lower + digits; - - private final Random random; - - private final char[] symbols; - - private final char[] buf; - - public RandomString(int length, Random random, String symbols) { - if (length < 1) throw new IllegalArgumentException(); - if (symbols.length() < 2) throw new IllegalArgumentException(); - this.random = Objects.requireNonNull(random); - this.symbols = symbols.toCharArray(); - this.buf = new char[length]; - } - - /** - * Create an alphanumeric string generator. - */ - public RandomString(int length, Random random) { - this(length, random, alphanum); - } - - /** - * Create an alphanumeric strings from a secure generator. - */ - public RandomString(int length) { - this(length, new SecureRandom()); - } - - /** - * Create session identifiers. - */ - public RandomString() { - this(21); - } - - public static String randomCode(int length) { - return new RandomString(length).nextString(); - } - -} \ No newline at end of file diff --git a/common/src/main/java/org/keycloak/common/util/SecretGenerator.java b/common/src/main/java/org/keycloak/common/util/SecretGenerator.java new file mode 100644 index 0000000000..a20259e81f --- /dev/null +++ b/common/src/main/java/org/keycloak/common/util/SecretGenerator.java @@ -0,0 +1,71 @@ +package org.keycloak.common.util; + +import java.security.SecureRandom; +import java.util.Random; + +public class SecretGenerator { + + public static final int DEFAULT_LENGTH = 32; + + public static final char[] UPPER = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".toCharArray(); + + public static final char[] DIGITS = "0123456789".toCharArray(); + + public static final char[] ALPHANUM = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789".toCharArray(); + + private static final SecretGenerator instance = new SecretGenerator(); + + private ThreadLocal random = new ThreadLocal() { + @Override + protected Random initialValue() { + return new SecureRandom(); + } + }; + + private SecretGenerator() { + } + + public static SecretGenerator getInstance() { + return instance; + } + + public String randomString() { + return randomString(DEFAULT_LENGTH, ALPHANUM); + } + + public String randomString(int length) { + return randomString(length, ALPHANUM); + } + + public String randomString(int length, char[] symbols) { + if (length < 1) { + throw new IllegalArgumentException(); + } + if (symbols == null || symbols.length < 2) { + throw new IllegalArgumentException(); + } + + Random r = random.get(); + char[] buf = new char[length]; + + for (int idx = 0; idx < buf.length; ++idx) { + buf[idx] = symbols[r.nextInt(symbols.length)]; + } + + return new String(buf); + } + public byte[] randomBytes() { + return randomBytes(DEFAULT_LENGTH); + } + + public byte[] randomBytes(int length) { + if (length < 1) { + throw new IllegalArgumentException(); + } + + byte[] buf = new byte[length]; + random.get().nextBytes(buf); + return buf; + } + +} \ No newline at end of file diff --git a/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanAuthenticationSessionProvider.java b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanAuthenticationSessionProvider.java index 9e370c108b..f30ffe7ebb 100644 --- a/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanAuthenticationSessionProvider.java +++ b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanAuthenticationSessionProvider.java @@ -25,6 +25,7 @@ import java.util.concurrent.TimeUnit; import org.infinispan.Cache; import org.jboss.logging.Logger; import org.keycloak.common.util.Base64Url; +import org.keycloak.common.util.SecretGenerator; import org.keycloak.common.util.Time; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; @@ -35,7 +36,6 @@ import org.keycloak.models.sessions.infinispan.events.RealmRemovedSessionEvent; import org.keycloak.models.sessions.infinispan.events.SessionEventsSenderTransaction; import org.keycloak.models.sessions.infinispan.stream.RootAuthenticationSessionPredicate; import org.keycloak.models.sessions.infinispan.util.InfinispanKeyGenerator; -import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.models.utils.RealmInfoUtil; import org.keycloak.sessions.AuthenticationSessionCompoundId; import org.keycloak.sessions.AuthenticationSessionProvider; @@ -186,6 +186,6 @@ public class InfinispanAuthenticationSessionProvider implements AuthenticationSe protected String generateTabId() { - return Base64Url.encode(KeycloakModelUtils.generateSecret(8)); + return Base64Url.encode(SecretGenerator.getInstance().randomBytes(8)); } } diff --git a/model/map/src/main/java/org/keycloak/models/map/authSession/MapRootAuthenticationSessionAdapter.java b/model/map/src/main/java/org/keycloak/models/map/authSession/MapRootAuthenticationSessionAdapter.java index 5c919287f9..81b06a7ad8 100644 --- a/model/map/src/main/java/org/keycloak/models/map/authSession/MapRootAuthenticationSessionAdapter.java +++ b/model/map/src/main/java/org/keycloak/models/map/authSession/MapRootAuthenticationSessionAdapter.java @@ -17,11 +17,11 @@ package org.keycloak.models.map.authSession; import org.keycloak.common.util.Base64Url; +import org.keycloak.common.util.SecretGenerator; import org.keycloak.common.util.Time; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; -import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.sessions.AuthenticationSessionModel; import java.util.Map; @@ -123,6 +123,6 @@ public class MapRootAuthenticationSessionAdapter extends AbstractRootAuthenticat } private String generateTabId() { - return Base64Url.encode(KeycloakModelUtils.generateSecret(8)); + return Base64Url.encode(SecretGenerator.getInstance().randomBytes(8)); } } diff --git a/model/map/src/main/java/org/keycloak/models/map/deploymentState/MapDeploymentStateProviderFactory.java b/model/map/src/main/java/org/keycloak/models/map/deploymentState/MapDeploymentStateProviderFactory.java index 5f54728a90..c9e8936ec3 100644 --- a/model/map/src/main/java/org/keycloak/models/map/deploymentState/MapDeploymentStateProviderFactory.java +++ b/model/map/src/main/java/org/keycloak/models/map/deploymentState/MapDeploymentStateProviderFactory.java @@ -24,7 +24,7 @@ import org.keycloak.Config; import org.keycloak.common.Profile; import org.keycloak.common.Version; import org.keycloak.common.util.Base64Url; -import org.keycloak.common.util.RandomString; +import org.keycloak.common.util.SecretGenerator; import org.keycloak.migration.MigrationModel; import org.keycloak.migration.ModelVersion; import org.keycloak.models.KeycloakSession; @@ -52,7 +52,7 @@ public class MapDeploymentStateProviderFactory implements DeploymentStateProvide Logger.getLogger(DeploymentStateProviderFactory.class) .warnf("It is recommended to set '%s' property in the %s provider config of %s SPI", RESOURCES_VERSION_SEED, PROVIDER_ID, DeploymentStateSpi.NAME); //generate random string for this installation - seed = RandomString.randomCode(10); + seed = SecretGenerator.getInstance().randomString(10); } try { Version.RESOURCES_VERSION = Base64Url.encode(MessageDigest.getInstance("MD5") diff --git a/quarkus/runtime/src/main/java/org/keycloak/services/resources/QuarkusWelcomeResource.java b/quarkus/runtime/src/main/java/org/keycloak/services/resources/QuarkusWelcomeResource.java index 434d184ca9..b208ebe0be 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/services/resources/QuarkusWelcomeResource.java +++ b/quarkus/runtime/src/main/java/org/keycloak/services/resources/QuarkusWelcomeResource.java @@ -22,8 +22,8 @@ import org.keycloak.common.ClientConnection; import org.keycloak.common.Version; import org.keycloak.common.util.Base64Url; import org.keycloak.common.util.MimeTypeUtil; +import org.keycloak.common.util.SecretGenerator; import org.keycloak.models.KeycloakSession; -import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.services.ForbiddenException; import org.keycloak.services.ServicesLogger; import org.keycloak.services.managers.ApplianceBootstrap; @@ -251,7 +251,7 @@ public class QuarkusWelcomeResource { } private String setCsrfCookie() { - String stateChecker = Base64Url.encode(KeycloakModelUtils.generateSecret()); + String stateChecker = Base64Url.encode(SecretGenerator.getInstance().randomBytes()); String cookiePath = session.getContext().getUri().getPath(); boolean secureOnly = session.getContext().getUri().getRequestUri().getScheme().equalsIgnoreCase("https"); CookieHelper.addCookie(KEYCLOAK_STATE_CHECKER, stateChecker, cookiePath, null, null, 300, secureOnly, true); diff --git a/server-spi-private/src/main/java/org/keycloak/models/DefaultOAuth2DeviceUserCodeProvider.java b/server-spi-private/src/main/java/org/keycloak/models/DefaultOAuth2DeviceUserCodeProvider.java index 34cc4c8ae6..cc02cd0e77 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/DefaultOAuth2DeviceUserCodeProvider.java +++ b/server-spi-private/src/main/java/org/keycloak/models/DefaultOAuth2DeviceUserCodeProvider.java @@ -17,9 +17,7 @@ package org.keycloak.models; -import org.keycloak.common.util.RandomString; - -import java.security.SecureRandom; +import org.keycloak.common.util.SecretGenerator; /** * The default implementation for generating/formatting user code of OAuth 2.0 Device Authorization Grant. @@ -36,7 +34,7 @@ public class DefaultOAuth2DeviceUserCodeProvider implements OAuth2DeviceUserCode @Override public String generate() { // For case-insensitive, use uppercase - return new RandomString(LENGTH, new SecureRandom(), RandomString.upper).nextString(); + return SecretGenerator.getInstance().randomString(LENGTH, SecretGenerator.UPPER); } @Override diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java index cf6410bc86..482500ca23 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java @@ -24,6 +24,7 @@ import org.keycloak.broker.social.SocialIdentityProviderFactory; import org.keycloak.common.util.CertificateUtils; import org.keycloak.common.util.KeyUtils; import org.keycloak.common.util.PemUtils; +import org.keycloak.common.util.SecretGenerator; import org.keycloak.component.ComponentModel; import org.keycloak.models.AuthenticationExecutionModel; import org.keycloak.models.AuthenticationFlowModel; @@ -40,7 +41,6 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.RealmProvider; import org.keycloak.models.RoleModel; import org.keycloak.models.ScopeContainerModel; -import org.keycloak.models.UserCredentialModel; import org.keycloak.models.UserModel; import org.keycloak.representations.idm.CertificateRepresentation; import org.keycloak.storage.UserStorageProviderModel; @@ -54,7 +54,6 @@ import java.security.Key; import java.security.KeyPair; import java.security.PrivateKey; import java.security.PublicKey; -import java.security.SecureRandom; import java.security.cert.X509Certificate; import java.util.Collection; import java.util.HashSet; @@ -85,16 +84,6 @@ public final class KeycloakModelUtils { return UUID.randomUUID().toString(); } - public static byte[] generateSecret() { - return generateSecret(32); - } - - public static byte[] generateSecret(int bytes) { - byte[] buf = new byte[bytes]; - new SecureRandom().nextBytes(buf); - return buf; - } - public static PublicKey getPublicKey(String publicKeyPem) { if (publicKeyPem != null) { try { @@ -156,9 +145,9 @@ public final class KeycloakModelUtils { return rep; } - public static UserCredentialModel generateSecret(ClientModel client) { - UserCredentialModel secret = UserCredentialModel.generateSecret(); - client.setSecret(secret.getChallengeResponse()); + public static String generateSecret(ClientModel client) { + String secret = SecretGenerator.getInstance().randomString(); + client.setSecret(secret); return secret; } diff --git a/server-spi/src/main/java/org/keycloak/models/UserCredentialModel.java b/server-spi/src/main/java/org/keycloak/models/UserCredentialModel.java index 87af69316f..f642a4af45 100755 --- a/server-spi/src/main/java/org/keycloak/models/UserCredentialModel.java +++ b/server-spi/src/main/java/org/keycloak/models/UserCredentialModel.java @@ -17,6 +17,7 @@ package org.keycloak.models; +import org.keycloak.common.util.SecretGenerator; import org.keycloak.credential.CredentialInput; import org.keycloak.credential.CredentialModel; import org.keycloak.models.credential.OTPCredentialModel; @@ -25,7 +26,6 @@ import org.keycloak.models.credential.PasswordUserCredentialModel; import java.util.HashMap; import java.util.Map; -import java.util.UUID; /** * @author Bill Burke @@ -123,7 +123,7 @@ public class UserCredentialModel implements CredentialInput { } public static UserCredentialModel generateSecret() { - return new UserCredentialModel("", SECRET, UUID.randomUUID().toString()); + return new UserCredentialModel("", SECRET, SecretGenerator.getInstance().randomString()); } @Override diff --git a/services/src/main/java/org/keycloak/authentication/requiredactions/ConsoleVerifyEmail.java b/services/src/main/java/org/keycloak/authentication/requiredactions/ConsoleVerifyEmail.java index e3c6ec5a30..a4187b3a54 100755 --- a/services/src/main/java/org/keycloak/authentication/requiredactions/ConsoleVerifyEmail.java +++ b/services/src/main/java/org/keycloak/authentication/requiredactions/ConsoleVerifyEmail.java @@ -21,7 +21,7 @@ import org.jboss.logging.Logger; import org.keycloak.authentication.RequiredActionContext; import org.keycloak.authentication.RequiredActionProvider; import org.keycloak.authentication.ConsoleDisplayMode; -import org.keycloak.common.util.RandomString; +import org.keycloak.common.util.SecretGenerator; import org.keycloak.email.EmailException; import org.keycloak.email.EmailTemplateProvider; import org.keycloak.events.Details; @@ -116,7 +116,7 @@ public class ConsoleVerifyEmail implements RequiredActionProvider { UserModel user = context.getUser(); AuthenticationSessionModel authSession = context.getAuthenticationSession(); EventBuilder event = context.getEvent().clone().event(EventType.SEND_VERIFY_EMAIL).detail(Details.EMAIL, user.getEmail()); - String code = RandomString.randomCode(8); + String code = SecretGenerator.getInstance().randomString(8); authSession.setAuthNote(Constants.VERIFY_EMAIL_CODE, code); RealmModel realm = session.getContext().getRealm(); diff --git a/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java b/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java index 7d781807ec..5d3015fc00 100755 --- a/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java +++ b/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java @@ -28,6 +28,7 @@ import org.keycloak.broker.provider.ExchangeExternalToken; import org.keycloak.broker.provider.IdentityBrokerException; import org.keycloak.broker.provider.util.SimpleHttp; import org.keycloak.common.util.Base64Url; +import org.keycloak.common.util.SecretGenerator; import org.keycloak.common.util.Time; import org.keycloak.events.Details; import org.keycloak.events.Errors; @@ -778,7 +779,7 @@ public class OIDCIdentityProvider extends AbstractOAuth2IdentityProvider