Adding dummyHash to DirectGrant request in case user does not exists. Fix dummyHash for normal login requests

closes #12298

Signed-off-by: mposolda <mposolda@gmail.com>

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);
     }

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.

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;

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);

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");

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

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<PasswordHashProvider, Integer, Long> 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;

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");