Separate password and OTP brute force protection to prevent OTP bypass attacks by default by pskopek · Pull Request #46168 · keycloak/keycloak

pskopek · 2026-02-10T12:06:42Z

Separate password and OTP brute force protection to prevent OTP bypass attacks by default

...ispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/LoginFailureEntity.java

...ak/models/sessions/infinispan/changes/remote/updater/loginfailures/LoginFailuresUpdater.java

…s attacks by default Closes keycloak#46164 Signed-off-by: Peter Skopek <peter.skopek@ibm.com> Update model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/changes/remote/updater/loginfailures/LoginFailuresUpdater.java Co-authored-by: Pedro Ruivo <pruivo@users.noreply.github.com> Signed-off-by: Peter Skopek <peter.skopek@ibm.com> Add recovery codes to the list of brute force checked authenticators. Closes keycloak#46164 Signed-off-by: Peter Skopek <peter.skopek@ibm.com>

pskopek · 2026-02-12T05:54:55Z

Fixed the DCO issue (no other changes).

pruivo · 2026-02-12T09:06:05Z

services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java

+            if (realm.getRequiredCredentialsStream().anyMatch(r -> Objects.equals(r.getType(), RequiredCredentialModel.PASSWORD.getType()))) {
+                return RequiredCredentialModel.PASSWORD.getType();
+            } else if (realm.getRequiredCredentialsStream().anyMatch(r -> Objects.equals(r.getType(), RequiredCredentialModel.TOTP.getType()))) {
+                return RequiredCredentialModel.TOTP.getType();


Is it possible the realm requires both a password and TOTP? If so, this method will never return the TOTP type.

pruivo · 2026-02-12T09:07:10Z

...gration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java

    }

+    @Test
+    public void testGrantPermamentOtpAbsolut() {


typo

Suggested change

public void testGrantPermamentOtpAbsolut() {

public void testGrantPermanentOtpAbsolut() {

pruivo · 2026-02-12T09:07:38Z

...gration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java

+            AccessTokenResponse response = getTestToken(getPassword("test-user@localhost"), totpSecret);
+            assertTokenNull(response);
+            Assert.assertNotNull(response.getError());
+            Assert.assertEquals(response.getError(), "invalid_grant");


parameters in the wrong order:

Suggested change

Assert.assertEquals(response.getError(), "invalid_grant");

Assert.assertEquals("invalid_grant", response.getError());

pruivo · 2026-02-12T09:08:04Z

...gration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java

+                events.clear();
+            }
+        }
+        {


I'm not sure why you need this block

pruivo · 2026-02-12T09:08:42Z

...gration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java

+                Assert.assertEquals(response.getError(), "invalid_grant");
+                Assert.assertEquals(response.getErrorDescription(), "Invalid user credentials");


Suggested change

Assert.assertEquals(response.getError(), "invalid_grant");

Assert.assertEquals(response.getErrorDescription(), "Invalid user credentials");

Assert.assertEquals("invalid_grant", response.getError());

Assert.assertEquals("Invalid user credentials", response.getErrorDescription());

rmartinc

Thanks @pskopek!

I think the test is not testing the feature and, checking the test, I detected the category is not correctly detected in direct access grants. I did a quick fix in branch https://github.com/rmartinc/keycloak/tree/pr-46168. But probably something will fail. 😄

rmartinc · 2026-02-12T15:34:22Z

services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java

+    );
+
+    public static final String OTP_CATEGORY = OTPCredentialModel.TYPE;
+    public static final int MAX_OTP_FAILURES = 100;


I would have allowed this as configurable in the brute force, but we can do this later if it is requested.

rmartinc · 2026-02-12T17:26:41Z

...gration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java

+            Assert.assertNotNull(response.getError());
+            Assert.assertEquals(response.getError(), "invalid_grant");
+            Assert.assertEquals("Invalid user credentials", response.getErrorDescription());
+            assertUserDisabledEvent(Errors.USER_TEMPORARILY_DISABLED);


@pskopek I think that this test is not testing that the the user is permanently locked after 100 failed OTP attempts. Probably you need to set in the realm failure factor to more than 100 and quick login check millis to 0. This way the user is not temporarily locked out and you can check the OTP is really permanently locking the user.

rmartinc · 2026-02-12T18:06:08Z

services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java

+                                : AuthenticationProcessor.LAST_PROCESSED_EXECUTION
+                )
+        );
+        if (execution == null) return null;


Playing with your test, I think that direct access grants return null execution here so OTP is not detected here for that grant. Maybe we have to pass the authenticator on failure and use the credentials used in success. I did a quick tets in branch: https://github.com/rmartinc/keycloak/tree/pr-46168

mposolda

I did not checked the details in the codebase as it is already reviewed by others. Just some high level thoughts:

It seems to me that the behavior of automatically doing permanent lockout for OTP is not backwards compatible. Existing deployments may not expect this behavior as there is not permanent lockout as of now. I suppose we may need a switch on the brute-force protector (disabled by default) and make this behavior enabled just with the switch?
It will be good to document this also in the brute-force protector documentation (probably together with the new switch added from above if we add this). As upgrading guide is something, which people read just when updating to the particular release.
I think we should also document (in both brute-force docs and upgrading guide) the behavior around custom authenticators (x509, passkeys, IDP login etc) and about the fact that they do not count anymore to brute-force and successful login with those do not reset brute-force

mposolda · 2026-02-13T07:08:28Z

docs/documentation/server_admin/topics/threat/brute-force.adoc


+[NOTE]
+====
+OTP type authenticators will cause permanent user account lockout even in case of temporary lockout brute force mode is set. This will mitigate probabilistic attack. The OTP oauthenticator type threshold is hardcoded to 100. This counter is incremented only in case of using OTP type authenticator.


Suggested change

OTP type authenticators will cause permanent user account lockout even in case of temporary lockout brute force mode is set. This will mitigate probabilistic attack. The OTP oauthenticator type threshold is hardcoded to 100. This counter is incremented only in case of using OTP type authenticator.

OTP type authenticators will cause permanent user account lockout even in case of temporary lockout brute force mode is set. This will mitigate probabilistic attack. The OTP authenticator type threshold is hardcoded to 100. This counter is incremented only in case of using OTP type authenticator.

mposolda · 2026-02-13T07:13:39Z

...ak/models/sessions/infinispan/changes/remote/updater/loginfailures/LoginFailuresUpdater.java

    }

+    @Override
+    public int getNumOtpFailures() {


Is it possible to rename this method (and other related methods and constants) to something more generic, so that brute-force protector API do not reference "OTP" as concrete authentication method in those methods? Perhaps something like getNumSecondaryAuthFailures()

pskopek requested review from a team as code owners February 10, 2026 12:06

keycloak-github-bot bot added team/cloud-native team/core-clients team/core-iam team/sre labels Feb 10, 2026

pskopek mentioned this pull request Feb 10, 2026

Fix: Prevent brute force counter increment on session limit denial #45541

Open

mabartos reviewed Feb 10, 2026

View reviewed changes

...ispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/LoginFailureEntity.java Show resolved Hide resolved

mposolda self-assigned this Feb 10, 2026

pruivo requested changes Feb 10, 2026

View reviewed changes

...ak/models/sessions/infinispan/changes/remote/updater/loginfailures/LoginFailuresUpdater.java Outdated Show resolved Hide resolved

pskopek force-pushed the issues/otpAbsolutLockout branch from 3a51e50 to c695a03 Compare February 11, 2026 12:12

pskopek force-pushed the issues/otpAbsolutLockout branch from c695a03 to 6e32ba3 Compare February 12, 2026 05:52

pruivo requested changes Feb 12, 2026

View reviewed changes

mposolda requested a review from rmartinc February 12, 2026 14:48

rmartinc suggested changes Feb 12, 2026

View reviewed changes

mposolda reviewed Feb 13, 2026

View reviewed changes

	public void testGrantPermamentOtpAbsolut() {
	public void testGrantPermanentOtpAbsolut() {

	Assert.assertEquals(response.getError(), "invalid_grant");
	Assert.assertEquals("invalid_grant", response.getError());

		Assert.assertEquals(response.getError(), "invalid_grant");
		Assert.assertEquals(response.getErrorDescription(), "Invalid user credentials");

	OTP type authenticators will cause permanent user account lockout even in case of temporary lockout brute force mode is set. This will mitigate probabilistic attack. The OTP oauthenticator type threshold is hardcoded to 100. This counter is incremented only in case of using OTP type authenticator.
	OTP type authenticators will cause permanent user account lockout even in case of temporary lockout brute force mode is set. This will mitigate probabilistic attack. The OTP authenticator type threshold is hardcoded to 100. This counter is incremented only in case of using OTP type authenticator.

Conversation

pskopek commented Feb 10, 2026

Uh oh!

Uh oh!

Uh oh!

pskopek commented Feb 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmartinc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants