From a79761a447b0a9b5b19c116cb1d15c01f5b34b7f Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Fri, 17 May 2024 11:16:04 -0300 Subject: [PATCH] Support for blocking concurrent requests when brute force is enabled Closes #31726 Signed-off-by: Pedro Igor Signed-off-by: Douglas Palmer Signed-off-by: mposolda --- .../DefaultBlockingBruteForceProtector.java | 118 ++++++++++++++++++ .../DefaultBruteForceProtectorFactory.java | 7 +- .../testsuite/forms/BruteForceTest.java | 106 ++++++++++++++-- 3 files changed, 217 insertions(+), 14 deletions(-) create mode 100644 services/src/main/java/org/keycloak/services/managers/DefaultBlockingBruteForceProtector.java diff --git a/services/src/main/java/org/keycloak/services/managers/DefaultBlockingBruteForceProtector.java b/services/src/main/java/org/keycloak/services/managers/DefaultBlockingBruteForceProtector.java new file mode 100644 index 0000000000..5d6f0dc254 --- /dev/null +++ b/services/src/main/java/org/keycloak/services/managers/DefaultBlockingBruteForceProtector.java @@ -0,0 +1,118 @@ +/* + * Copyright 2016 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.keycloak.services.managers; + +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Map.Entry; + +import org.keycloak.models.AbstractKeycloakTransaction; +import org.keycloak.models.KeycloakSession; +import org.keycloak.models.KeycloakSessionFactory; +import org.keycloak.models.RealmModel; +import org.keycloak.models.UserModel; +import org.keycloak.sessions.AuthenticationSessionModel; + +public class DefaultBlockingBruteForceProtector extends DefaultBruteForceProtector { + + // make this configurable? + private static final int DEFAULT_MAX_CONCURRENT_ATTEMPTS = 1000; + private static final float DEFAULT_LOAD_FACTOR = 0.75f; + + private final Map loginAttempts = Collections.synchronizedMap(new LinkedHashMap<>(100, DEFAULT_LOAD_FACTOR) { + @Override + protected boolean removeEldestEntry(Entry eldest) { + return loginAttempts.size() > DEFAULT_MAX_CONCURRENT_ATTEMPTS; + } + }); + + DefaultBlockingBruteForceProtector(KeycloakSessionFactory factory) { + super(factory); + } + + @Override + public boolean isPermanentlyLockedOut(KeycloakSession session, RealmModel realm, UserModel user) { + if (super.isPermanentlyLockedOut(session, realm, user)) { + return true; + } + + if (!realm.isPermanentLockout()) return false; + + return isLoginInProgress(session, user); + } + + @Override + public boolean isTemporarilyDisabled(KeycloakSession session, RealmModel realm, UserModel user) { + if (super.isTemporarilyDisabled(session, realm, user)) { + return true; + } + + return isLoginInProgress(session, user); + } + + private boolean isLoginInProgress(KeycloakSession session, UserModel user) { + AuthenticationSessionModel authSession = session.getContext().getAuthenticationSession(); + + if (authSession == null) { + // not authenticating as there is no auth session bound to the session + return false; + } + + if (isCurrentLoginAttempt(user)) { + return !tryEnlistBlockingTransaction(session, user); + } + + return true; + } + + // Return true if this thread successfully enlisted itself + private boolean tryEnlistBlockingTransaction(KeycloakSession session, UserModel user) { + String threadInProgress = loginAttempts.computeIfAbsent(user.getId(), k -> getThreadName()); + + // This means that this thread successfully added itself into the map. We can enlist transaction just in that case + if (threadInProgress.equals(getThreadName())) { + session.getTransactionManager().enlistAfterCompletion(new AbstractKeycloakTransaction() { + @Override + protected void commitImpl() { + unblock(); + } + + @Override + protected void rollbackImpl() { + unblock(); + } + + private void unblock() { + loginAttempts.remove(user.getId()); + } + }); + + return true; + } else { + return false; + } + } + + private boolean isCurrentLoginAttempt(UserModel user) { + return loginAttempts.getOrDefault(user.getId(), getThreadName()).equals(getThreadName()); + } + + private String getThreadName() { + return Thread.currentThread().getName(); + } +} diff --git a/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtectorFactory.java b/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtectorFactory.java index 0769c7b506..53a8ad6d46 100755 --- a/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtectorFactory.java +++ b/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtectorFactory.java @@ -28,6 +28,8 @@ import org.keycloak.models.KeycloakSessionFactory; public class DefaultBruteForceProtectorFactory implements BruteForceProtectorFactory { DefaultBruteForceProtector protector; + private boolean allowConcurrentRequests; + @Override public BruteForceProtector create(KeycloakSession session) { return protector; @@ -35,12 +37,13 @@ public class DefaultBruteForceProtectorFactory implements BruteForceProtectorFac @Override public void init(Config.Scope config) { - + // this can be a brute force setting? + this.allowConcurrentRequests = config.getBoolean("allowConcurrentRequests", Boolean.FALSE); } @Override public void postInit(KeycloakSessionFactory factory) { - protector = new DefaultBruteForceProtector(factory); + protector = allowConcurrentRequests ? new DefaultBruteForceProtector(factory) : new DefaultBlockingBruteForceProtector(factory); } @Override diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java index 431b12a7cf..811761df53 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java @@ -100,7 +100,7 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { private int lifespan; - private static final Integer failureFactor= 2; + private static final Integer failureFactor = 2; @Override public void configureTestRealm(RealmRepresentation testRealm) { @@ -380,13 +380,13 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { assertUserNumberOfFailures(user.getId(), failureFactor); events.clear(); - } finally { - realm.setPermanentLockout(false); - testRealm().update(realm); - UserRepresentation user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); - user.setEnabled(true); - updateUser(user); - } + } finally { + realm.setPermanentLockout(false); + testRealm().update(realm); + UserRepresentation user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); + user.setEnabled(true); + updateUser(user); + } } @Test @@ -416,7 +416,7 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { loginInvalidPassword(); //Wait for brute force executor to process the login and then wait for delta time - waitForExecutors(numExecutors+1); + waitForExecutors(numExecutors + 1); testingClient.testing().setTimeOffset(Collections.singletonMap("offset", String.valueOf(5))); loginInvalidPassword(); @@ -438,7 +438,7 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { loginInvalidPassword(); //Wait for brute force executor to process the login and then wait for delta time - waitForExecutors(numExecutors+1); + waitForExecutors(numExecutors + 1); testingClient.testing().setTimeOffset(Collections.singletonMap("offset", String.valueOf(5))); loginInvalidPassword(); @@ -468,10 +468,11 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { ExecutorsProvider provider = session.getProvider(ExecutorsProvider.class); ExecutorService executor = provider.getExecutor("bruteforce"); ThreadPoolExecutor threadPoolExecutor = (ThreadPoolExecutor) executor; - while(!threadPoolExecutor.getQueue().isEmpty()) { + while (!threadPoolExecutor.getQueue().isEmpty()) { try { Thread.sleep(1000); - } catch (Exception e) {} + } catch (Exception e) { + } } assertEquals(numExecutors, threadPoolExecutor.getCompletedTaskCount()); }); @@ -753,6 +754,87 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { events.clear(); } + @Test + public void testRaceAttackTemporaryLockout() throws Exception { + RealmRepresentation realm = testRealm().toRepresentation(); + UserRepresentation user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); + try { + realm.setWaitIncrementSeconds(120); + realm.setQuickLoginCheckMilliSeconds(120000L); + testRealm().update(realm); + clearUserFailures(); + clearAllUserFailures(); + user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); + user.setEnabled(true); + testRealm().users().get(user.getId()).update(user); + String totpSecret = totp.generateTOTP("totpSecret"); + OAuthClient.AccessTokenResponse response = getTestToken("password", totpSecret); + Assert.assertNotNull(response.getAccessToken()); + raceAttack(user); + } finally { + realm.setWaitIncrementSeconds(5); + realm.setQuickLoginCheckMilliSeconds(100L); + testRealm().update(realm); + user.setEnabled(true); + updateUser(user); + } + } + + @Test + public void testRaceAttackPermanentLockout() throws Exception { + RealmRepresentation realm = testRealm().toRepresentation(); + UserRepresentation user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); + try { + realm.setPermanentLockout(true); + testRealm().update(realm); + raceAttack(user); + clearUserFailures(); + clearAllUserFailures(); + user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); + user.setEnabled(true); + testRealm().users().get(user.getId()).update(user); + String totpSecret = totp.generateTOTP("totpSecret"); + OAuthClient.AccessTokenResponse response = getTestToken("password", totpSecret); + Assert.assertNotNull(response.getAccessToken()); + } finally { + realm.setPermanentLockout(false); + testRealm().update(realm); + user.setEnabled(true); + updateUser(user); + } + } + + private void raceAttack(UserRepresentation user) throws Exception { + int num = 100; + LoginThread[] threads = new LoginThread[num]; + for (int i = 0; i < num; ++i) { + threads[i] = new LoginThread(); + } + for (int i = 0; i < num; ++i) { + threads[i].start(); + } + for (int i = 0; i < num; ++i) { + threads[i].join(); + } + int invalidCount = (int) adminClient.realm("test").attackDetection().bruteForceUserStatus(user.getId()).get("numFailures"); + assertTrue("Invalid count should be less than or equal 2 but was: " + invalidCount, invalidCount <= 2); + } + + public class LoginThread extends Thread { + + public void run() { + try { + String totpSecret = totp.generateTOTP("totpSecret"); + OAuthClient.AccessTokenResponse response = getTestToken("invalid", totpSecret); + Assert.assertNull(response.getAccessToken()); + Assert.assertEquals(response.getError(), "invalid_grant"); + Assert.assertEquals(response.getErrorDescription(), "Invalid user credentials"); + } catch (Exception ex) { + ex.printStackTrace(); + } + } + } + public void expectTemporarilyDisabled() { expectTemporarilyDisabled("test-user@localhost", null, "password"); }