Repository navigation
Allow identity providers to reset brute force login failures on login - #53724
JBoogieman wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Security-sensitive cookie-SSO behavior with the provider setting enabled lacks regression coverage.
1 open finding
What changed in this PR
Adds opt-in brute-force failure resets for successful identity-provider logins.
Changes:
- Adds per-provider configuration and Admin UI control.
- Integrates brokered logins with brute-force protection.
- Adds broker tests and administrator documentation.
| File | Description |
|---|---|
AbstractAdvancedBrokerTest.java |
Tests default and enabled behavior. |
DefaultBruteForceProtector.java |
Adds the identity-provider category. |
AuthenticationManager.java |
Detects eligible brokered logins. |
IdentityProviderModel.java |
Defines the new configuration property. |
AdvancedSettings.tsx |
Adds the Admin UI switch. |
messages_en.properties |
Adds UI labels and guidance. |
brute-force.adoc |
Documents reset behavior. |
configuration.adoc |
Documents provider configuration. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
There was a problem hiding this comment.
🟡 Changes recommended
The new regression tests should be migrated from the deprecated Arquillian suite to the current Test Framework.
1 open finding
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Closes keycloak#53249 Signed-off-by: Justin Brueggemann <167224505+JBoogieman@users.noreply.github.com>
9303428 to
beecf92
Compare
|
Rebased on main and squashed to one commit. The only conflict was the admin UI's On moving the new tests to the new Test Framework: they add test cases to the existing |
Closes #53249
Since #49996, the brute force failure count is only reset by a successful login that used a password, OTP, or recovery code. That fixed cookie SSO re-authentication resetting the count (#49960), but it also means a successful brokered login (SAML, OIDC, social) never resets it. Users who mostly sign in through an identity provider keep accumulating failures from occasional mistyped local passwords until they are locked out.
Resetting on every brokered login would reopen #49960, because an identity provider can complete a login without the user entering anything (from an existing session at the provider). This PR makes the reset opt-in per identity provider:
resetLoginFailures(admin console: Advanced settings > "Reset brute force failures on login"). Off by default, so existing behavior is unchanged.AuthenticationManager.logSuccess()passes anidentity-providercategory to the brute force protector for logins completed through that provider, and the failure count is reset the same way as after a password login.IDENTITY_PROVIDERuser session note set infinishBrokerAuthentication(), so later cookie-based SSO re-authentication still does not reset the count, even with the setting on.I went with a per-provider setting since trust differs between providers, and it needs no schema changes. Happy to move it to a realm-level setting if you'd prefer.
Tests: three new tests in
AbstractAdvancedBrokerTest(runs for SAML, OIDC, and OAuth2 brokering):loginWithBrokerDoesNotResetBruteForceFailureCountByDefault: count stays at 1 after a brokered login with the setting offloginWithBrokerResetsBruteForceFailureCountWhenEnabled: count resets to 0 with the setting onloginWithBrokerThenCookieSsoDoesNotResetBruteForceFailureCount: with the setting on, cookie SSO re-authentication on a session created through the identity provider does not reset the count (Brute-force detection: Authenticated session token refresh resets lockout counter #49960)Ran locally for
KcSamlBrokerTestandKcOidcBrokerTest.Docs: identity provider general configuration table and the brute force detection page.
AI disclosure: I used Claude to help test, and review old PRs. I reviewed, built, and tested all changes.