From b25c28458a562abda2f84fc684e59cce8577e562 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Tue, 6 Aug 2024 12:58:50 +0200 Subject: [PATCH] Remove the attempt in brute force when the off-thread finishes Closes #31881 Signed-off-by: rmartinc --- .../DefaultBlockingBruteForceProtector.java | 71 ++++++++++++++----- .../managers/DefaultBruteForceProtector.java | 16 +++-- 2 files changed, 65 insertions(+), 22 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/managers/DefaultBlockingBruteForceProtector.java b/services/src/main/java/org/keycloak/services/managers/DefaultBlockingBruteForceProtector.java index e343578acc0..063a97c08d7 100644 --- a/services/src/main/java/org/keycloak/services/managers/DefaultBlockingBruteForceProtector.java +++ b/services/src/main/java/org/keycloak/services/managers/DefaultBlockingBruteForceProtector.java @@ -20,7 +20,9 @@ import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; import java.util.Map.Entry; +import java.util.concurrent.atomic.AtomicBoolean; +import org.keycloak.common.ClientConnection; import org.keycloak.models.AbstractKeycloakTransaction; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; @@ -33,6 +35,7 @@ public class DefaultBlockingBruteForceProtector extends DefaultBruteForceProtect // make this configurable? private static final int DEFAULT_MAX_CONCURRENT_ATTEMPTS = 1000; private static final float DEFAULT_LOAD_FACTOR = 0.75f; + private static final String OFF_THREAD_STARTED = "#brute_force_started"; private final Map loginAttempts = Collections.synchronizedMap(new LinkedHashMap(100, DEFAULT_LOAD_FACTOR) { @Override @@ -73,46 +76,80 @@ public class DefaultBlockingBruteForceProtector extends DefaultBruteForceProtect return false; } - if (isCurrentLoginAttempt(user)) { - return !tryEnlistBlockingTransaction(session, user); - } - - return true; + return !tryEnlistBlockingTransactionOrSameThread(session, user); } - // Return true if this thread successfully enlisted itself - private boolean tryEnlistBlockingTransaction(KeycloakSession session, UserModel user) { - String threadInProgress = loginAttempts.computeIfAbsent(user.getId(), k -> getThreadName()); + // Return true if this thread successfully enlisted itself or it was already done by the same thread + private boolean tryEnlistBlockingTransactionOrSameThread(KeycloakSession session, UserModel user) { + AtomicBoolean inserted = new AtomicBoolean(false); + String threadInProgress = loginAttempts.computeIfAbsent(user.getId(), k -> { + inserted.set(true); + return getThreadName(); + }); // This means that this thread successfully added itself into the map. We can enlist transaction just in that case - if (threadInProgress.equals(getThreadName())) { + if (inserted.get()) { session.getTransactionManager().enlistAfterCompletion(new AbstractKeycloakTransaction() { @Override protected void commitImpl() { - unblock(); + // remove or wait the brute force thread to finish + loginAttempts.computeIfPresent(user.getId(), (k, v) -> v.endsWith(OFF_THREAD_STARTED)? "" : null); } @Override protected void rollbackImpl() { - unblock(); - } - - private void unblock() { + // remove on rollback loginAttempts.remove(user.getId()); } }); return true; } else { - return false; + return isCurrentThread(threadInProgress); } } - private boolean isCurrentLoginAttempt(UserModel user) { - return loginAttempts.getOrDefault(user.getId(), getThreadName()).equals(getThreadName()); + private boolean isCurrentThread(String name) { + return name.equals(getThreadName()) || name.equals(getThreadName() + OFF_THREAD_STARTED); } private String getThreadName() { return Thread.currentThread().getName(); } + + private void enlistRemoval(KeycloakSession session, String userId) { + session.getTransactionManager().enlistAfterCompletion(new AbstractKeycloakTransaction() { + @Override + protected void commitImpl() { + // remove or wait the main thread to finish + loginAttempts.computeIfPresent(userId, (k, v) -> v.isEmpty()? null : v.substring(0, v.length() - OFF_THREAD_STARTED.length())); + } + + @Override + protected void rollbackImpl() { + loginAttempts.remove(userId); + } + }); + } + + @Override + protected void processLogin(KeycloakSession session, LoginEvent event) { + // mark the off-thread is started for this request + loginAttempts.computeIfPresent(event.userId, (k, v) -> v + OFF_THREAD_STARTED); + super.processLogin(session, event); + } + + @Override + protected void failure(KeycloakSession session, LoginEvent event) { + // remove the user from concurrent login attemps once it's processed + enlistRemoval(session, event.userId); + super.failure(session, event); + } + + @Override + protected void success(KeycloakSession session, LoginEvent event) { + // remove the user from concurrent login attemps once it's processed + enlistRemoval(session, event.userId); + super.success(session, event); + } } diff --git a/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java b/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java index b111d9aefd2..72d2d0ee83f 100644 --- a/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java +++ b/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java @@ -217,12 +217,10 @@ public class DefaultBruteForceProtector implements Runnable, BruteForceProtector session.getTransactionManager().begin(); try { for (LoginEvent event : events) { - if (event instanceof FailedLogin) { - failure(session, event); - } else if (event instanceof SuccessfulLogin) { - success(session, event); - } else if (event instanceof ShutdownEvent) { + if (event instanceof ShutdownEvent) { run = false; + } else { + processLogin(session, event); } } } catch (Exception e) { @@ -299,6 +297,14 @@ public class DefaultBruteForceProtector implements Runnable, BruteForceProtector logger.trace("sent success event"); } + protected void processLogin(KeycloakSession session, LoginEvent event) { + if (event instanceof SuccessfulLogin) { + success(session, event); + } else { + failure(session, event); + } + } + @Override public boolean isTemporarilyDisabled(KeycloakSession session, RealmModel realm, UserModel user) { UserLoginFailureModel failure = session.loginFailures().getUserLoginFailure(realm, user.getId());