diff --git a/crypto/default/src/test/java/org/keycloak/crypto/def/test/CryptoPerfTest.java b/crypto/default/src/test/java/org/keycloak/crypto/def/test/CryptoPerfTest.java index 2dad3fe46a..da5b6e4108 100644 --- a/crypto/default/src/test/java/org/keycloak/crypto/def/test/CryptoPerfTest.java +++ b/crypto/default/src/test/java/org/keycloak/crypto/def/test/CryptoPerfTest.java @@ -138,7 +138,7 @@ public class CryptoPerfTest { perfTest(new Runnable() { @Override public void run() { - provider.encode("password", -1); + provider.encodedCredential("password", -1); } }, "testPbkdf512", 1); } diff --git a/docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc b/docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc index 29ccd69790..e15cf17b2c 100644 --- a/docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc +++ b/docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc @@ -302,6 +302,11 @@ the `SingleUseObjectKeyModel` also changed to keep consistency with the method n The previous `getExpiration` method is now deprecated and you should prefer using new newly introduced `getExp` method to avoid overflow after 2038. += Method encode deprecated on PasswordHashProvider + +Method `String encode(String rawPassword, int iterations)` on the interface `org.keycloak.credential.hash.PasswordHashProvider` is deprecated. The method will be removed in +one of the future {project_name} releases. It might be {project_name} 27 release. + = Resteasy util class is deprecated `org.keycloak.common.util.Resteasy` has been deprecated. You should use the `org.keycloak.util.KeycloakSessionUtil` to obtain the `KeycloakSession` instead. diff --git a/server-spi/src/main/java/org/keycloak/credential/hash/PasswordHashProvider.java b/server-spi/src/main/java/org/keycloak/credential/hash/PasswordHashProvider.java index 2e44777ce4..7ae7185a2d 100644 --- a/server-spi/src/main/java/org/keycloak/credential/hash/PasswordHashProvider.java +++ b/server-spi/src/main/java/org/keycloak/credential/hash/PasswordHashProvider.java @@ -30,6 +30,10 @@ public interface PasswordHashProvider extends Provider { PasswordCredentialModel encodedCredential(String rawPassword, int iterations); + /** + * Exists due the backwards compatibility. It is recommended to use {@link #encodedCredential(String, int)} + */ + @Deprecated default String encode(String rawPassword, int iterations) { return rawPassword; diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java index feb7bc4738..66da3971a5 100755 --- a/services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java @@ -21,12 +21,11 @@ import org.jboss.logging.Logger; import org.keycloak.authentication.AbstractFormAuthenticator; import org.keycloak.authentication.AuthenticationFlowContext; import org.keycloak.authentication.AuthenticationFlowError; -import org.keycloak.credential.hash.PasswordHashProvider; +import org.keycloak.authentication.authenticators.util.AuthenticatorUtils; import org.keycloak.events.Details; import org.keycloak.events.Errors; import org.keycloak.forms.login.LoginFormsProvider; import org.keycloak.models.ModelDuplicateException; -import org.keycloak.models.PasswordPolicy; import org.keycloak.models.UserCredentialModel; import org.keycloak.models.UserModel; import org.keycloak.models.utils.FormMessage; @@ -100,21 +99,9 @@ public abstract class AbstractUsernameFormAuthenticator extends AbstractFormAuth return challengeResponse; } - protected void dummyHash(AuthenticationFlowContext context) { - PasswordPolicy passwordPolicy = context.getRealm().getPasswordPolicy(); - PasswordHashProvider provider; - if (passwordPolicy != null && passwordPolicy.getHashAlgorithm() != null) { - provider = context.getSession().getProvider(PasswordHashProvider.class, passwordPolicy.getHashAlgorithm()); - } else { - provider = context.getSession().getProvider(PasswordHashProvider.class); - } - int iterations = passwordPolicy != null ? passwordPolicy.getHashIterations() : -1; - provider.encode("SlightlyLongerDummyPassword", iterations); - } - public void testInvalidUser(AuthenticationFlowContext context, UserModel user) { if (user == null) { - dummyHash(context); + AuthenticatorUtils.dummyHash(context); context.getEvent().error(Errors.USER_NOT_FOUND); Response challengeResponse = challenge(context, getDefaultChallengeMessage(context), FIELD_USERNAME); context.failureChallenge(AuthenticationFlowError.INVALID_USER, challengeResponse); diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/directgrant/ValidateUsername.java b/services/src/main/java/org/keycloak/authentication/authenticators/directgrant/ValidateUsername.java index 35feb3b93c..39530d359c 100755 --- a/services/src/main/java/org/keycloak/authentication/authenticators/directgrant/ValidateUsername.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/directgrant/ValidateUsername.java @@ -20,6 +20,7 @@ package org.keycloak.authentication.authenticators.directgrant; import org.keycloak.authentication.AuthenticationFlowContext; import org.keycloak.authentication.AuthenticationFlowError; import org.keycloak.authentication.authenticators.browser.AbstractUsernameFormAuthenticator; +import org.keycloak.authentication.authenticators.util.AuthenticatorUtils; import org.keycloak.events.Details; import org.keycloak.events.Errors; import org.keycloak.models.AuthenticationExecutionModel; @@ -71,6 +72,7 @@ public class ValidateUsername extends AbstractDirectGrantAuthenticator { if (user == null) { + AuthenticatorUtils.dummyHash(context); context.getEvent().error(Errors.USER_NOT_FOUND); Response challengeResponse = errorResponse(Response.Status.UNAUTHORIZED.getStatusCode(), "invalid_grant", "Invalid user credentials"); context.failure(AuthenticationFlowError.INVALID_USER, challengeResponse); @@ -79,6 +81,7 @@ public class ValidateUsername extends AbstractDirectGrantAuthenticator { String bruteForceError = getDisabledByBruteForceEventError(context, user); if (bruteForceError != null) { + AuthenticatorUtils.dummyHash(context); context.getEvent().user(user); context.getEvent().error(bruteForceError); Response challengeResponse = errorResponse(Response.Status.UNAUTHORIZED.getStatusCode(), "invalid_grant", "Invalid user credentials"); diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.java b/services/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.java index 03c01a28b9..0d140f1a32 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.core.type.TypeReference; import org.jboss.logging.Logger; import org.keycloak.authentication.AuthenticationFlowContext; import org.keycloak.common.util.Time; +import org.keycloak.credential.hash.PasswordHashProvider; import org.keycloak.events.Errors; import org.keycloak.models.*; import org.keycloak.services.managers.BruteForceProtector; @@ -53,6 +54,24 @@ public final class AuthenticatorUtils { return AuthenticatorUtils.getDisabledByBruteForceEventError(authnFlowContext.getProtector(), authnFlowContext.getSession(), authnFlowContext.getRealm(), authenticatedUser); } + /** + * This method exists to simulate hashing of some "dummy" password. The purpose is to make the user enumeration harder, so the authentication request with non-existing username also need + * to simulate the password hashing overhead and takes same time like the request with existing username, but incorrect password. + * + * @param context + */ + public static void dummyHash(AuthenticationFlowContext context) { + PasswordPolicy passwordPolicy = context.getRealm().getPasswordPolicy(); + PasswordHashProvider provider; + if (passwordPolicy != null && passwordPolicy.getHashAlgorithm() != null) { + provider = context.getSession().getProvider(PasswordHashProvider.class, passwordPolicy.getHashAlgorithm()); + } else { + provider = context.getSession().getProvider(PasswordHashProvider.class); + } + int iterations = passwordPolicy != null ? passwordPolicy.getHashIterations() : -1; + provider.encodedCredential("SlightlyLongerDummyPassword", iterations); + } + /** * Get all completed authenticator executions from the user session notes. * @param note The serialized note value to parse diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java index 3885251c26..7c2b0b62aa 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java @@ -213,7 +213,7 @@ public class PasswordHashingTest extends AbstractTestRealmKeycloakTest { Pbkdf2Sha512PasswordHashProviderFactory.DEFAULT_ITERATIONS, 0, 256); - String encodedPassword = specificKeySizeHashProvider.encode(password, -1); + String encodedPassword = specificKeySizeHashProvider.encodedCredential(password, -1).getPasswordSecretData().getValue(); // Create a user with the encoded password, simulating a user import from a different system using a specific key size UserRepresentation user = UserBuilder.create().username(username).password(encodedPassword).build(); @@ -416,7 +416,7 @@ public class PasswordHashingTest extends AbstractTestRealmKeycloakTest { BiFunction hasher = (provider, iterations) -> { long result = 0L; for (String password : plainTextPasswords) { - String encoded = provider.encode(password, iterations); + String encoded = provider.encodedCredential(password, iterations).getPasswordSecretData().getValue(); result += encoded.hashCode(); } return result; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java index a4dbbd3a30..6f812ae468 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java @@ -652,6 +652,38 @@ public class ResourceOwnerPasswordCredentialsGrantTest extends AbstractKeycloakT } } + @Test + public void grantAccessTokenInvalidUserCredentialsPerf() throws Exception { + int count = 5; + + // Measure the times when username exists, but password is invalid + long sumInvalidPasswordMs = perfTest(count, "Invalid password", this::grantAccessTokenInvalidUserCredentials); + + // Measure the times when username does not exists + long sumInvalidUsernameMs = perfTest(count, "User not found", this::grantAccessTokenUserNotFound); + + String errorMessage = String.format("Times in ms of %d attempts: For invalid password: %d. For invalid username: %d", count, sumInvalidPasswordMs, sumInvalidUsernameMs); + + // The times should be very similar. Using the bigger difference just to avoid flakiness (Before the fix, the difference was like 3 times shorter time for invalid-username, which allowed quite accurate username enumeration) + Assert.assertTrue(errorMessage, sumInvalidUsernameMs * 2 > sumInvalidPasswordMs); + } + + private long perfTest(int actionsCount, String actionMessage, RunnableWithException action) throws Exception { + long sumTimeMs = 0; + for (int i = 0 ; i < actionsCount ; i++) { + long start = System.currentTimeMillis(); + action.run(); + long took = System.currentTimeMillis() - start; + getLogger().infof("%s %d: %d ms", actionMessage, i + 1, took); + sumTimeMs = sumTimeMs + took; + } + return sumTimeMs; + } + + private interface RunnableWithException { + void run() throws Exception; + } + @Test public void grantAccessTokenInvalidUserCredentials() throws Exception { oauth.clientId("resource-owner");