From ca1152c3e520ddf0f3ab6bb63d48f1b9d1073557 Mon Sep 17 00:00:00 2001 From: Eriksson Fabian Date: Thu, 19 Jan 2017 17:20:03 +0100 Subject: [PATCH] KEYCLOAK-4204 Extend brute force protection with permanent lockout on failed attempts - Can still use temporary brute force protection. - After X-1 failed login attempt, if the user successfully logs in his/her fail login count is reset. --- .../idm/RealmRepresentation.java | 9 ++ .../models/cache/infinispan/RealmAdapter.java | 12 ++ .../infinispan/entities/CachedRealm.java | 6 + .../org/keycloak/models/jpa/RealmAdapter.java | 10 ++ .../models/utils/ModelToRepresentation.java | 1 + .../models/utils/RepresentationToModel.java | 2 + .../managers/BruteForceProtector.java | 2 + .../java/org/keycloak/models/RealmModel.java | 2 + .../AuthenticationProcessor.java | 20 ++- .../managers/DefaultBruteForceProtector.java | 116 +++++++++++++----- .../services/managers/RealmManager.java | 1 + .../testsuite/forms/BruteForceTest.java | 69 +++++++++++ .../messages/admin-messages_en.properties | 2 + .../admin/resources/partials/brute-force.html | 14 ++- 14 files changed, 233 insertions(+), 33 deletions(-) diff --git a/core/src/main/java/org/keycloak/representations/idm/RealmRepresentation.java b/core/src/main/java/org/keycloak/representations/idm/RealmRepresentation.java index e552c58954..b14e55bda1 100755 --- a/core/src/main/java/org/keycloak/representations/idm/RealmRepresentation.java +++ b/core/src/main/java/org/keycloak/representations/idm/RealmRepresentation.java @@ -66,6 +66,7 @@ public class RealmRepresentation { //--- brute force settings protected Boolean bruteForceProtected; + protected Boolean permanentLockout; protected Integer maxFailureWaitSeconds; protected Integer minimumQuickLoginWaitSeconds; protected Integer waitIncrementSeconds; @@ -558,6 +559,14 @@ public class RealmRepresentation { this.bruteForceProtected = bruteForceProtected; } + public Boolean isPermanentLockout() { + return permanentLockout; + } + + public void setPermanentLockout(Boolean permanentLockout) { + this.permanentLockout = permanentLockout; + } + public Integer getMaxFailureWaitSeconds() { return maxFailureWaitSeconds; } diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmAdapter.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmAdapter.java index 1e6109f6b1..8350f0dc30 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmAdapter.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmAdapter.java @@ -221,6 +221,18 @@ public class RealmAdapter implements CachedRealmModel { updated.setBruteForceProtected(value); } + @Override + public boolean isPermanentLockout() { + if(isUpdated()) return updated.isPermanentLockout(); + return cached.isPermanentLockout(); + } + + @Override + public void setPermanentLockout(final boolean val) { + getDelegateForUpdate(); + updated.setPermanentLockout(val); + } + @Override public int getMaxFailureWaitSeconds() { if (isUpdated()) return updated.getMaxFailureWaitSeconds(); diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRealm.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRealm.java index 5e4b5792b3..fef0486af1 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRealm.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRealm.java @@ -65,6 +65,7 @@ public class CachedRealm extends AbstractExtendableRevisioned { protected boolean editUsernameAllowed; //--- brute force settings protected boolean bruteForceProtected; + protected boolean permanentLockout; protected int maxFailureWaitSeconds; protected int minimumQuickLoginWaitSeconds; protected int waitIncrementSeconds; @@ -156,6 +157,7 @@ public class CachedRealm extends AbstractExtendableRevisioned { editUsernameAllowed = model.isEditUsernameAllowed(); //--- brute force settings bruteForceProtected = model.isBruteForceProtected(); + permanentLockout = model.isPermanentLockout(); maxFailureWaitSeconds = model.getMaxFailureWaitSeconds(); minimumQuickLoginWaitSeconds = model.getMinimumQuickLoginWaitSeconds(); waitIncrementSeconds = model.getWaitIncrementSeconds(); @@ -314,6 +316,10 @@ public class CachedRealm extends AbstractExtendableRevisioned { return bruteForceProtected; } + public boolean isPermanentLockout() { + return permanentLockout; + } + public int getMaxFailureWaitSeconds() { return this.maxFailureWaitSeconds; } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java index cce494f4ca..58aa4246c6 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java @@ -278,6 +278,16 @@ public class RealmAdapter implements RealmModel, JpaModel { setAttribute("bruteForceProtected", value); } + @Override + public boolean isPermanentLockout() { + return getAttribute("permanentLockout", false); + } + + @Override + public void setPermanentLockout(final boolean val) { + setAttribute("permanentLockout", val); + } + @Override public int getMaxFailureWaitSeconds() { return getAttribute("maxFailureWaitSeconds", 0); diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java b/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java index 360f287e96..e90192963f 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java @@ -267,6 +267,7 @@ public class ModelToRepresentation { rep.setRegistrationEmailAsUsername(realm.isRegistrationEmailAsUsername()); rep.setRememberMe(realm.isRememberMe()); rep.setBruteForceProtected(realm.isBruteForceProtected()); + rep.setPermanentLockout(realm.isPermanentLockout()); rep.setMaxFailureWaitSeconds(realm.getMaxFailureWaitSeconds()); rep.setMinimumQuickLoginWaitSeconds(realm.getMinimumQuickLoginWaitSeconds()); rep.setWaitIncrementSeconds(realm.getWaitIncrementSeconds()); diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java b/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java index 439134159b..b427477846 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java @@ -141,6 +141,7 @@ public class RepresentationToModel { if (rep.getDisplayNameHtml() != null) newRealm.setDisplayNameHtml(rep.getDisplayNameHtml()); if (rep.isEnabled() != null) newRealm.setEnabled(rep.isEnabled()); if (rep.isBruteForceProtected() != null) newRealm.setBruteForceProtected(rep.isBruteForceProtected()); + if (rep.isPermanentLockout() != null) newRealm.setPermanentLockout(rep.isPermanentLockout()); if (rep.getMaxFailureWaitSeconds() != null) newRealm.setMaxFailureWaitSeconds(rep.getMaxFailureWaitSeconds()); if (rep.getMinimumQuickLoginWaitSeconds() != null) newRealm.setMinimumQuickLoginWaitSeconds(rep.getMinimumQuickLoginWaitSeconds()); @@ -787,6 +788,7 @@ public class RepresentationToModel { if (rep.getDisplayNameHtml() != null) realm.setDisplayNameHtml(rep.getDisplayNameHtml()); if (rep.isEnabled() != null) realm.setEnabled(rep.isEnabled()); if (rep.isBruteForceProtected() != null) realm.setBruteForceProtected(rep.isBruteForceProtected()); + if (rep.isPermanentLockout() != null) realm.setPermanentLockout(rep.isPermanentLockout()); if (rep.getMaxFailureWaitSeconds() != null) realm.setMaxFailureWaitSeconds(rep.getMaxFailureWaitSeconds()); if (rep.getMinimumQuickLoginWaitSeconds() != null) realm.setMinimumQuickLoginWaitSeconds(rep.getMinimumQuickLoginWaitSeconds()); diff --git a/server-spi-private/src/main/java/org/keycloak/services/managers/BruteForceProtector.java b/server-spi-private/src/main/java/org/keycloak/services/managers/BruteForceProtector.java index e884b02cdf..02be48d5df 100755 --- a/server-spi-private/src/main/java/org/keycloak/services/managers/BruteForceProtector.java +++ b/server-spi-private/src/main/java/org/keycloak/services/managers/BruteForceProtector.java @@ -30,5 +30,7 @@ import org.keycloak.provider.Provider; public interface BruteForceProtector extends Provider { void failedLogin(RealmModel realm, UserModel user, ClientConnection clientConnection); + void successfulLogin(RealmModel realm, UserModel user, ClientConnection clientConnection); + boolean isTemporarilyDisabled(KeycloakSession session, RealmModel realm, UserModel user); } diff --git a/server-spi/src/main/java/org/keycloak/models/RealmModel.java b/server-spi/src/main/java/org/keycloak/models/RealmModel.java index 7640aad83c..dc8bff5333 100755 --- a/server-spi/src/main/java/org/keycloak/models/RealmModel.java +++ b/server-spi/src/main/java/org/keycloak/models/RealmModel.java @@ -127,6 +127,8 @@ public interface RealmModel extends RoleContainerModel { //--- brute force settings boolean isBruteForceProtected(); void setBruteForceProtected(boolean value); + boolean isPermanentLockout(); + void setPermanentLockout(boolean val); int getMaxFailureWaitSeconds(); void setMaxFailureWaitSeconds(int val); int getWaitIncrementSeconds(); diff --git a/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java b/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java index 2f23cb82a4..a34e4ee914 100755 --- a/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java +++ b/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java @@ -552,6 +552,22 @@ public class AuthenticationProcessor { } } + protected void logSuccess() { + if (realm.isBruteForceProtected()) { + String username = clientSession.getNote(AbstractUsernameFormAuthenticator.ATTEMPTED_USERNAME); + // TODO: as above, need to handle non form success + + if(username == null) { + return; + } + + UserModel user = KeycloakModelUtils.findUserByNameOrEmail(session, realm, username); + if (user != null) { + getBruteForceProtector().successfulLogin(realm, user, connection); + } + } + } + public boolean isSuccessful(AuthenticationExecutionModel model) { ClientSessionModel.ExecutionStatus status = clientSession.getExecutionStatus().get(model.getId()); if (status == null) return false; @@ -853,7 +869,7 @@ public class AuthenticationProcessor { public void validateUser(UserModel authenticatedUser) { if (authenticatedUser == null) return; if (!authenticatedUser.isEnabled()) throw new AuthenticationFlowException(AuthenticationFlowError.USER_DISABLED); - if (realm.isBruteForceProtected()) { + if (realm.isBruteForceProtected() && !realm.isPermanentLockout()) { if (getBruteForceProtector().isTemporarilyDisabled(session, realm, authenticatedUser)) { throw new AuthenticationFlowException(AuthenticationFlowError.USER_TEMPORARILY_DISABLED); } @@ -866,6 +882,8 @@ public class AuthenticationProcessor { return redirectToRequiredActions(session, realm, clientSession, uriInfo); } else { event.detail(Details.CODE_ID, clientSession.getId()); // todo This should be set elsewhere. find out why tests fail. Don't know where this is supposed to be set + // the user has successfully logged in and we can clear his/her previous login failure attempts. + logSuccess(); return AuthenticationManager.finishedRequiredActions(session, userSession, clientSession, connection, request, uriInfo, 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 76c9cc76af..c779f3eca8 100644 --- a/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java +++ b/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java @@ -85,6 +85,14 @@ public class DefaultBruteForceProtector implements Runnable, BruteForceProtector } } + protected class SuccessfulLogin extends LoginEvent { + protected final CountDownLatch latch = new CountDownLatch(1); + + public SuccessfulLogin(String realmId, String userId, String ip) { + super(realmId, userId, ip); + } + } + public DefaultBruteForceProtector(KeycloakSessionFactory factory) { this.factory = factory; } @@ -96,44 +104,67 @@ public class DefaultBruteForceProtector implements Runnable, BruteForceProtector String userId = event.userId; UserModel user = session.users().getUserById(userId, realm); + if (user == null) { + return; + } + UserLoginFailureModel userLoginFailure = getUserModel(session, event); - if (user != null) { - if (userLoginFailure == null) { - userLoginFailure = session.sessions().addUserLoginFailure(realm, userId); - } - userLoginFailure.setLastIPFailure(event.ip); - long currentTime = Time.currentTimeMillis(); - long last = userLoginFailure.getLastFailure(); - long deltaTime = 0; - if (last > 0) { - deltaTime = currentTime - last; - } - userLoginFailure.setLastFailure(currentTime); - if (deltaTime > 0) { - // if last failure was more than MAX_DELTA clear failures - if (deltaTime > (long) realm.getMaxDeltaTimeSeconds() * 1000L) { - userLoginFailure.clearFailures(); - } - } + if (userLoginFailure == null) { + userLoginFailure = session.sessions().addUserLoginFailure(realm, userId); + } + userLoginFailure.setLastIPFailure(event.ip); + long currentTime = Time.currentTimeMillis(); + long last = userLoginFailure.getLastFailure(); + long deltaTime = 0; + if (last > 0) { + deltaTime = currentTime - last; + } + userLoginFailure.setLastFailure(currentTime); + + if(realm.isPermanentLockout()) { userLoginFailure.incrementFailures(); logger.debugv("new num failures: {0}", userLoginFailure.getNumFailures()); - int waitSeconds = realm.getWaitIncrementSeconds() * (userLoginFailure.getNumFailures() / realm.getFailureFactor()); - logger.debugv("waitSeconds: {0}", waitSeconds); - logger.debugv("deltaTime: {0}", deltaTime); - - if (waitSeconds == 0) { - if (last > 0 && deltaTime < realm.getQuickLoginCheckMilliSeconds()) { - logger.debugv("quick login, set min wait seconds"); - waitSeconds = realm.getMinimumQuickLoginWaitSeconds(); - } + if(userLoginFailure.getNumFailures() == realm.getFailureFactor()) { + logger.debugv("user {0} locked permanently due to too many login attempts", user.getUsername()); + user.setEnabled(false); + return; } - if (waitSeconds > 0) { - waitSeconds = Math.min(realm.getMaxFailureWaitSeconds(), waitSeconds); + + if (last > 0 && deltaTime < realm.getQuickLoginCheckMilliSeconds()) { + logger.debugv("quick login, set min wait seconds"); + int waitSeconds = realm.getMinimumQuickLoginWaitSeconds(); int notBefore = (int) (currentTime / 1000) + waitSeconds; logger.debugv("set notBefore: {0}", notBefore); userLoginFailure.setFailedLoginNotBefore(notBefore); } + return; + } + + if (deltaTime > 0) { + // if last failure was more than MAX_DELTA clear failures + if (deltaTime > (long) realm.getMaxDeltaTimeSeconds() * 1000L) { + userLoginFailure.clearFailures(); + } + } + userLoginFailure.incrementFailures(); + logger.debugv("new num failures: {0}", userLoginFailure.getNumFailures()); + + int waitSeconds = realm.getWaitIncrementSeconds() * (userLoginFailure.getNumFailures() / realm.getFailureFactor()); + logger.debugv("waitSeconds: {0}", waitSeconds); + logger.debugv("deltaTime: {0}", deltaTime); + + if (waitSeconds == 0) { + if (last > 0 && deltaTime < realm.getQuickLoginCheckMilliSeconds()) { + logger.debugv("quick login, set min wait seconds"); + waitSeconds = realm.getMinimumQuickLoginWaitSeconds(); + } + } + if (waitSeconds > 0) { + waitSeconds = Math.min(realm.getMaxFailureWaitSeconds(), waitSeconds); + int notBefore = (int) (currentTime / 1000) + waitSeconds; + logger.debugv("set notBefore: {0}", notBefore); + userLoginFailure.setFailedLoginNotBefore(notBefore); } } @@ -185,6 +216,8 @@ public class DefaultBruteForceProtector implements Runnable, BruteForceProtector for (LoginEvent event : events) { if (event instanceof FailedLogin) { failure(session, event); + } else if (event instanceof SuccessfulLogin) { + success(session, event); } else if (event instanceof ShutdownEvent) { run = false; } @@ -197,6 +230,8 @@ public class DefaultBruteForceProtector implements Runnable, BruteForceProtector for (LoginEvent event : events) { if (event instanceof FailedLogin) { ((FailedLogin) event).latch.countDown(); + } else if (event instanceof SuccessfulLogin) { + ((SuccessfulLogin) event).latch.countDown(); } } events.clear(); @@ -214,6 +249,17 @@ public class DefaultBruteForceProtector implements Runnable, BruteForceProtector } } + private void success(KeycloakSession session, LoginEvent event) { + String userId = event.userId; + UserModel model = session.users().getUserById(userId, getRealmModel(session, event)); + + UserLoginFailureModel user = getUserModel(session, event); + if(user == null) return; + + logger.debugv("user {0} successfully logged in, clearing all failures", model.getUsername()); + user.clearFailures(); + } + protected void logFailure(LoginEvent event) { ServicesLogger.LOGGER.loginFailure(event.userId, event.ip); failures++; @@ -243,6 +289,18 @@ public class DefaultBruteForceProtector implements Runnable, BruteForceProtector logger.trace("sent failure event"); } + @Override + public void successfulLogin(final RealmModel realm, final UserModel user, final ClientConnection clientConnection) { + try { + SuccessfulLogin event = new SuccessfulLogin(realm.getId(), user.getId(), clientConnection.getRemoteAddr()); + queue.offer(event); + + event.latch.await(5, TimeUnit.SECONDS); + } catch (InterruptedException e) { + } + logger.trace("sent success event"); + } + @Override public boolean isTemporarilyDisabled(KeycloakSession session, RealmModel realm, UserModel user) { UserLoginFailureModel failure = session.sessions().getUserLoginFailure(realm, user.getId()); diff --git a/services/src/main/java/org/keycloak/services/managers/RealmManager.java b/services/src/main/java/org/keycloak/services/managers/RealmManager.java index b28cf2f218..3306bcdedb 100755 --- a/services/src/main/java/org/keycloak/services/managers/RealmManager.java +++ b/services/src/main/java/org/keycloak/services/managers/RealmManager.java @@ -213,6 +213,7 @@ public class RealmManager { // brute force realm.setBruteForceProtected(false); // default settings off for now todo set it on + realm.setPermanentLockout(false); realm.setMaxFailureWaitSeconds(900); realm.setMinimumQuickLoginWaitSeconds(60); realm.setWaitIncrementSeconds(60); 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 d87896e84d..884762b38e 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 @@ -325,6 +325,54 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { loginSuccess(); } + @Test + public void testPermanentLockout() throws Exception { + RealmRepresentation realm = testRealm().toRepresentation(); + + try { + // arrange + realm.setPermanentLockout(true); + testRealm().update(realm); + + // act + loginInvalidPassword(); + loginInvalidPassword(); + + // assert + expectPermanentlyDisabled(); + Assert.assertFalse(adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0).isEnabled()); + } 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 + public void testResetLoginFailureCount() throws Exception { + RealmRepresentation realm = testRealm().toRepresentation(); + + try { + // arrange + realm.setPermanentLockout(true); + testRealm().update(realm); + + // act + loginInvalidPassword(); + loginSuccess(); + loginInvalidPassword(); + loginSuccess(); + + // assert + Assert.assertTrue(adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0).isEnabled()); + } finally { + realm.setPermanentLockout(false); + testRealm().update(realm); + } + } + @Test public void testNonExistingAccounts() throws Exception { @@ -358,6 +406,27 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { event.assertEvent(); } + public void expectPermanentlyDisabled() throws Exception { + expectPermanentlyDisabled("test-user@localhost", null); + } + + public void expectPermanentlyDisabled(String username, String userId) throws Exception { + loginPage.open(); + loginPage.login(username, "password"); + + loginPage.assertCurrent(); + Assert.assertEquals("Account is disabled, contact admin.", loginPage.getError()); + ExpectedEvent event = events.expectLogin() + .session((String) null) + .error(Errors.USER_DISABLED) + .detail(Details.USERNAME, username) + .removeDetail(Details.CONSENT); + if (userId != null) { + event.user(userId); + } + event.assertEvent(); + } + public void loginSuccess() throws Exception { loginSuccess("test-user@localhost"); } diff --git a/themes/src/main/resources/theme/base/admin/messages/admin-messages_en.properties b/themes/src/main/resources/theme/base/admin/messages/admin-messages_en.properties index fff8bf2536..dc4ef935e7 100644 --- a/themes/src/main/resources/theme/base/admin/messages/admin-messages_en.properties +++ b/themes/src/main/resources/theme/base/admin/messages/admin-messages_en.properties @@ -126,6 +126,8 @@ robots-tag=X-Robots-Tag robots-tag-tooltip=Prevent pages from appearing in search engines (click label for more information) x-xss-protection=X-XSS-Protection x-xss-protection-tooltip=This header configures the Cross-site scripting (XSS) filter in your browser. Using the default behavior, the browser will prevent rendering of the page when a XSS attack is detected (click label for more information) +permanent-lockout=Permanent Lockout +permanent-lockout.tooltip=Lock the user permanently when the user exceeds the maximum login failures. max-login-failures=Max Login Failures max-login-failures.tooltip=How many failures before wait is triggered. wait-increment=Wait Increment diff --git a/themes/src/main/resources/theme/base/admin/resources/partials/brute-force.html b/themes/src/main/resources/theme/base/admin/resources/partials/brute-force.html index 33845c0b40..6ab0092391 100755 --- a/themes/src/main/resources/theme/base/admin/resources/partials/brute-force.html +++ b/themes/src/main/resources/theme/base/admin/resources/partials/brute-force.html @@ -14,6 +14,14 @@ +
+ +
+ +
+ {{:: 'permanent-lockout.tooltip' | translate}} +
+
@@ -23,7 +31,7 @@
{{:: 'max-login-failures.tooltip' | translate}} -
+
{{:: 'min-quick-login-wait.tooltip' | translate}}
-
+
{{:: 'max-wait.tooltip' | translate}}
-
+