Require existing OTP on default Reset Credentials flow#50144
Conversation
0988d37 to
72e4b80
Compare
There was a problem hiding this comment.
Pull request overview
Updates Keycloak’s default realm bootstrap for the Reset Credentials flow to require proof-of-possession of an already-enrolled OTP during password reset, mitigating account takeover when an attacker can access the password-reset email. This aligns the shipped defaults for newly created realms with a stronger SSPR baseline, while documenting the behavior change in the upgrading guide.
Changes:
- Switch the Reset - Conditional OTP execution from
reset-otptoauth-otp-formin the default reset-credentials flow bootstrap. - Add an upgrading guide entry describing the new default behavior and how to retain/achieve legacy behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| server-spi-private/src/main/java/org/keycloak/models/utils/DefaultAuthenticationFlows.java | Changes the default reset-credentials conditional OTP execution to require OTP validation via auth-otp-form and adds explanatory comments. |
| docs/documentation/upgrading/topics/changes/changes-26_7_0.adoc | Documents the new default reset-credentials behavior and guidance for admins. |
| execution = new AuthenticationExecutionModel(); | ||
| execution.setParentFlow(conditionalOTP.getId()); | ||
| execution.setRequirement(AuthenticationExecutionModel.Requirement.REQUIRED); | ||
| execution.setAuthenticator("reset-otp"); | ||
| execution.setAuthenticator("auth-otp-form"); | ||
| execution.setPriority(20); |
| // control (see https://github.com/keycloak/keycloak/issues/12759). | ||
| // The previous "reset-otp" authenticator (which silently deletes or shadows | ||
| // the existing OTP credential and queues CONFIGURE_TOTP) can still be added | ||
| // by realm administrators who explicitly prefer that recovery model; existing | ||
| // realms keep their persisted flow unchanged. |
| execution.setAuthenticatorFlow(false); | ||
| realm.addAuthenticatorExecution(execution); | ||
|
|
||
| // For users with an OTP credential, require possession of the existing OTP | ||
| // before allowing the password reset to complete. This prevents an attacker |
| When a realm is created with the built-in default authentication flows, the *Reset - Conditional OTP* subflow inside the *Reset Credentials* flow now contains the `auth-otp-form` authenticator instead of `reset-otp`. The new default requires a user who has an OTP credential enrolled to enter that OTP before completing a password reset, which prevents an attacker who can read the user's password-reset email from rotating the OTP under their control. See link:https://github.com/keycloak/keycloak/issues/12759[GitHub issue #12759] for the prior discussion. | ||
|
|
||
| Only realms created in this release or later use the new default. Existing realms keep their persisted Reset Credentials flow unchanged, so this is not an automatic upgrade. Realm administrators who want the new behavior on an existing realm can edit the Reset Credentials flow in the admin console and replace the `Reset OTP` execution with `OTP Form`. Realm administrators who explicitly prefer the previous recovery model can configure their flow to use the `reset-otp` authenticator, which is still shipped and unchanged. |
72e4b80 to
1bcac45
Compare
|
Thanks for the Copilot review. All four comments addressed in the amended commit (
Force-pushed to |
| // The previous "reset-otp" authenticator (which always queues CONFIGURE_TOTP | ||
| // as a required action, and additionally removes one or all of the user's | ||
| // existing OTP credentials depending on the configured "otpReset" mode) |
Replace the "reset-otp" authenticator with "auth-otp-form" in the default Reset Credentials flow's conditional OTP subflow. For users with an OTP credential enrolled, the password reset now requires possession of the existing OTP before completing, which prevents an attacker who can read the password-reset email from rotating the OTP under their control. The previous "reset-otp" authenticator is still shipped unchanged. Realm administrators who explicitly prefer the legacy recovery model can configure their flow to use it. Existing realms keep their persisted Reset Credentials flow unchanged; this change only affects realms created on this release or later. Closes keycloak#12759 Signed-off-by: Kript0r3x <kript0r3x@gmail.com>
|
Fifth Copilot comment addressed in the same single commit (force-pushed):
DCO still green; commit shape unchanged (one commit). |
1bcac45 to
a1787c6
Compare
Closes
Closes #12759
Motivation
For users with an OTP credential enrolled, the shipped default Reset Credentials flow currently accepts an email-link click as the sole proof of identity, then queues
CONFIGURE_TOTPso the requester binds a brand-new OTP under their control. The original OTP secret is silently deleted or shadowed depending on the realm'sotpResetpolicy. An attacker who can read the user's password-reset email therefore completes a password reset, binds a new OTP under their control, and ends up authenticated as the victim with no possession check against the existing OTP.This issue has been discussed since 2022 in #12759. The maintainer-suggested workaround in that thread is to "include the OTP form as part of the reset credentials flow" by replacing the
reset-otpexecution withOTP Form. This PR makes that the default for new realms, matching the industry baseline (Auth0, Okta, Entra ID all require MFA on SSPR for MFA-enrolled accounts per their published documentation).A more recent thread comment (NikolaNemes) noted the original workaround is no longer applicable to default flows once #15536 made built-in flows non-editable, which is an additional reason to ship the safer default rather than rely on operator opt-in.
Change
One-line change in
server-spi-private/src/main/java/org/keycloak/models/utils/DefaultAuthenticationFlows.java: theReset - Conditional OTPsubflow's second execution now setssetAuthenticator("auth-otp-form")instead ofsetAuthenticator("reset-otp"). Theconditional-user-configuredpredicate is unchanged, so the subflow still skips entirely for users with no OTP enrolled (no regression for non-MFA users).Accompanying upgrading-guide entry added under
docs/documentation/upgrading/topics/changes/changes-26_7_0.adocas a Breaking change, explaining the new default and how to keep the legacy behavior.Scope
reset-otpauthenticator itself is unchanged and remains available. Realm administrators who explicitly prefer the legacy recovery model can still configure it.auth-otp-formauthenticator and theconditional-user-configuredpredicate already power the equivalent gate in the browser flow.Tests
No new test file is added in this commit. The change replaces one string literal in the realm-bootstrap path; the existing
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/suite (ResetPasswordTest,ResetOtpTest,AltSubflowForCredentialResetTest,ResetCredentialsAlternativeFlowsTest) exercises the reset-credentials path comprehensively and will catch any regression.ResetOtpTest's scenarios configure their own custom flow viacreateOrChangeResetOtpFlowConfigand theotpResetTestFlowalias, so they exercise thereset-otpauthenticator explicitly and are not affected by the default-flow change. If reviewers prefer an additional focused assertion that the fresh-realm default flow now containsauth-otp-formrather thanreset-otp, I am happy to add it in a follow-up commit or amend this one.Verification
Manual reproduction of the original vulnerability against
quay.io/keycloak/keycloak:latestonmain(commit before this PR): with a TOTP-enrolled user, attacker reads the reset email, lands inUPDATE_PASSWORD, thenCONFIGURE_TOTP, completes both, and obtains a fully authenticated session with attacker-controlled password and attacker-controlled OTP, original OTP silently replaced, no notification to the previously verified email.After this change: with the same setup, the reset flow presents
OTP Formto the user after the password update step. An attacker who does not hold the victim's existing OTP cannot complete the reset.Notes
I am happy to address review feedback on test additions, migration handling for existing realms (if the team prefers an explicit
MigrationProviderstep instead of leaving existing realms unchanged), or any wording in the upgrading-guide entry.