Keycloak / PR #29

Implement recovery key support for user storage providers

keycloak/keycloak#38446
Golden recall: 100% Extra findings: 6

Golden Comments (2/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Unsafe raw List deserialization without type safety. Calling Optional.get() directly on the Optional returned by RecoveryAuthnCodesUtils.getCredential(user) without checking isPresent() can lead to a NoSuchElementException if the Optional is empty. MEDIUM ✓ Found "RecoveryAuthnCodeInputLoginBean lacks empty-state handling: credentialModelOpt.get() throws NoSuchElementException when no recovery-code credential exists, turning the login page into a 500. The displayed 'next code number' is also only accurate when the model tracks consumed codes — for a user-storage provider returning the full set it always shows code #1" (freemarker/model/RecoveryAuthnCodeInputLoginBean.java L17)
#2 After creating the RecoveryAuthnCodesCredentialModel, consider setting its id from the stored credential (e.g., myUser.recoveryCodes.getId()); otherwise getId() will be null and downstream removal by id (e.g., removeStoredCredentialById in the authenticator flow) may not work. LOW ✓ Found "getCredentials in the legacy storage loses the credential id assigned in updateCredential because it rebuilds the model via createFromValues without preserving it — id-based management (account-REST lookup/delete, removeStoredCredentialById when all codes are used) breaks or receives null" (federation/BackwardsCompatibilityUserStorage.java L237)

Supplementary Findings (6 findings)

Finding File Severity Legitimate?
Federated recovery-code credentials are never consumed or exhausted: the removal logic uses removeStoredCredentialById, which only affects the local store, allCodesUsed() is effectively always false for a storage returning the full set each request, and the legacy storage's isValid never removes used codes — the same code remains valid forever and the login page always shows code #1 RecoveryAuthnCodesFormAuthenticator.java:87 HIGH ✓ Real: consumption/rotation only touches the local store, so federated codes are never exhausted
RecoveryAuthnCodesUtils.getCredential and its call sites assume any federated credential of the recovery-authn-codes type parses into a RecoveryAuthnCodesCredentialModel via createFromCredentialModel — raw JSON persisted by CredentialHelper (as the test provider does) makes it throw and the login/required-action flow 500s RecoveryAuthnCodesUtils.java:53 HIGH ✓ Real: the storage contract is implicit and untested; the test provider itself triggers the throw
Disablement of recovery codes across the federation boundary is unsupported and untested: disableCredentialType only handles OTP and getDisableableCredentialTypesStream returns only OTP, while supportsCredentialType returns true for recovery codes, so disable calls silently no-op BackwardsCompatibilityUserStorage.java:217 HIGH ✓ Real: silent no-op on a security-relevant management operation
Recovery codes are transmitted and stored in plaintext in the user-storage path: CredentialHelper passes the raw generated codes as challengeResponse to updateCredential, and the legacy test provider stores them unhashed and validates them in plaintext — unlike the hashed DB path, there is no server-side control over how the storage protects these high-value backup credentials CredentialHelper.java:116 MEDIUM ✓ Borderline: legitimate concern, but user-storage providers are trusted by design in Keycloak's model
The new test only covers the user-storage creation branch — it never exercises the local fallback creation path (when a storage declines recovery codes) nor recovery-code management via admin REST or disablement across the federation boundary, which the change's two-branch design requires BackwardsCompatibilityUserStorageTest.java:241 MEDIUM ✓ Real: half of the two-branch design is untested
Minor: typo 'delayed-suthenticator-config' in the test config alias, and the IOException in getCredentials is logged without the exception while the recovery credential is silently omitted BackwardsCompatibilityUserStorageTest.java:135 LOW ✓ Borderline: cosmetic, but a dropped exception obscures real failures

Honest Analysis

FriendlyReviewer found both golden bugs. The Optional.get() finding is framed by its user-visible consequence — a 500 on the login page — plus the follow-on defect that the displayed 'next code number' is always #1 for a storage returning the full code set. The id-loss finding is tied to its functional impact: removeStoredCredentialById cannot work when the model is rebuilt without its id, which is exactly the removal path used when all codes are consumed.

The notable pattern of this review is that its three HIGH findings are all supplementary: federated codes are never consumed or exhausted, the parse contract of RecoveryAuthnCodesUtils.getCredential 500s on raw JSON (the test provider itself triggers the throw), and recovery-code disablement silently no-ops across the federation boundary. These are arguably more damaging than the goldens: the golden NoSuchElementException only fires when no credential exists, while the consumption gap means a federated user's codes never expire. The coverage-gap finding is also accurate — the new test only walks the user-storage branch, leaving the local-fallback path, the other half of the two-branch design, unverified.

Two caveats. Severity ordering is inverted relative to the reference dataset: the issues the dataset rates Medium and Low are reviewed here as the least severe of eight, while three non-golden issues carry the high severity. That ranking is defensible, but it is the reverse of the dataset's own ordering. And the plaintext finding (CredentialHelper.java L116) is the most debatable of the six: user-storage providers are trusted by Keycloak's user-storage model, so the risk is real but conditional on the storage implementation.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌤️ Feat
3 high, 4 medium, 1 low 1 mitigated

The MR introduces a user-storage-aware creation path for recovery authentication codes, branching between a federated user-storage provider and the local DB. The existing DB hashing flow is preserved and the core branching logic is correct. However, the user-storage scenario has several correctness and robustness gaps: federated recovery codes are never consumed or exhausted, raw codes are passed to storage in plaintext, the code assumes federated credentials are directly parseable as DB models (which can cause 500s), and disablement/rotation across the federation boundary is unsupported. The new test coverage only exercises the user-storage path and lacks local-fallback and admin-REST cases. These issues should be addressed before merging.

These might need a close look

  • 🔴 org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java (L87)
    Federated recovery-code credentials are never consumed or exhausted: the removal logic uses removeStoredCredentialById, which only affects the local store, and allCodesUsed() is effectively always false for a storage that returns the full code set each request. Additionally, the legacy storage's isValid never removes used codes, so the same code remains valid forever and the login page always shows code #1. Consumption/rotation must be handled through the user-storage updater API or explicitly delegated to the storage provider.
  • 🔴 org/keycloak/models/utils/RecoveryAuthnCodesUtils.java (L53)
    RecoveryAuthnCodesUtils.getCredential and its call sites assume any federated credential of the recovery-authn-codes type is parseable into a RecoveryAuthnCodesCredentialModel via createFromCredentialModel. If a user-storage provider returns the raw JSON persisted by CredentialHelper (as the test provider does), createFromCredentialModel throws and the login/required-action flow 500s. The contract that storage must convert raw codes into a valid model in getCredentials is implicit and untested; make getCredential defensively parse or guard the contract.
  • 🔴 org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (L217)
    Disablement/export of recovery codes across the federation boundary is unsupported and untested: disableCredentialType only handles OTP and getDisableableCredentialTypesStream returns only OTP, while supportsCredentialType returns true for recovery codes, so disable calls silently no-op. The new test never exercises recovery-code disablement. Add recovery-code handling to both methods and a test.

Worth checking

  • 🟡 org/keycloak/utils/CredentialHelper.java (L116)
    Recovery codes are transmitted and stored in plaintext in the user-storage path: CredentialHelper passes the raw generated codes as challengeResponse to updateCredential, and the legacy test provider stores them unhashed and validates them in plaintext (BackwardsCompatibilityUserStorage.java line 197). Unlike the hashed DB path, there is no server-side control over how the storage protects these high-value backup credentials. Document the storage-provider security responsibility and consider passing only hashed/encrypted forms.
  • 🟡 org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java (L17)
    RecoveryAuthnCodeInputLoginBean lacks empty-state handling: credentialModelOpt.get() throws NoSuchElementException when no recovery-code credential exists, turning the login page into a 500. Also, the displayed 'next code number' is only accurate when the credential model tracks consumed codes; for a user-storage provider returning the full set, it always shows code #1. Add an explicit empty-state error and document that storage must expose only remaining codes.
  • 🟡 org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (L237)
    getCredentials in the legacy storage loses the credential id assigned in updateCredential because it rebuilds the model via createFromValues without preserving id. Any id-based management (account-REST lookup/delete, removeStoredCredentialById when all codes are used) breaks or receives null. Preserve id and other metadata on the returned model.
  • 🟡 org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (L241)
    The new test only covers the user-storage creation branch; it never exercises the local fallback creation path (when a storage declines recovery codes) nor recovery-code management via admin REST or disablement across the federation boundary, which the change's two-branch design requires. Add coverage for these scenarios.
Small things (take or leave)
  • 🔵 org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (L135)
    Minor: typo 'delayed-suthenticator-config' in the test config alias, and the IOException in getCredentials is logged without the exception and the recovery credential silently omitted. Fix the typo and log the exception.

✅ Feature-level checklist

Partially covered

  • ⚠️ Create recovery keys in user storage or local
    The two-branch creation path is implemented and the local DB path is preserved, but the user-storage path has significant caveats: raw codes stored plaintext, no consumption/exhaustion, fragile parse contract, and missing disablement support. Therefore the requirement is only partially satisfied.