Keycloak/PR #9

Implement recovery key support for user storage providers

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

Golden Comments (2/2 found)

#CommentSeverityStatusFriendlyReviewer Detail
#1Unsafe raw List deserialization without type safety. Optional.get() on RecoveryAuthnCodesUtils.getCredential(user) without isPresent() → NoSuchElementException if Optional is empty MEDIUM✓ Found FR found both aspects: (1) HIGH: credentialModelOpt.get() without isPresent() at L19 + getNextRecoveryAuthnCode().get() without isPresent() at L21 — login page crash if no credential. (2) MEDIUM: raw List.class deserialization without type safety at L238.
#2After creating RecoveryAuthnCodesCredentialModel, getId() will be null → removeStoredCredentialById() downstream will not work LOW✓ Found FR: "Missing credential ID in getCredentials(). createFromValues() does not set the ID, so downstream operations like removeStoredCredentialById() will receive null." Upgraded from Low to HIGH.

Supplementary Findings (10 findings)

FindingFileSeverityLegitimate?
CRITICAL SECURITY: Unhashed recovery codes serialized to JSON and passed to user storage providers via updateCredential(), bypassing RS512 hashingCredentialHelper.java:118HIGH✓ Security vulnerability — cleartext code leak
Post-validation credential cleanup ineffective for federated storage — isValid() does not remove used codes, allCodesUsed() never returns trueRecoveryAuthnCodesFormAuthenticator.java:86MEDIUM✓ Credential lifecycle blocked
Inconsistency between model reconstruction and validation — getCredential() returns hashed model, isValid() does raw comparison → allCodesUsed() always falseRecoveryAuthnCodesUtils.java:55MEDIUM✓ Broken lifecycle management
getMyUser() may return null but myUser.recoveryCodes accessed without null check → NPEBackwardsCompatibilityUserStorage.java:235HIGH✓ Potential crash
getDisableableCredentialTypesStream() does not include recovery codes even though storage supports themBackwardsCompatibilityUserStorage.java:270HIGH✓ Missing functionality
disableCredentialType() has no handler for RecoveryAuthnCodesCredentialModel.TYPE — disabling recovery codes silently ignoredBackwardsCompatibilityUserStorage.java:281HIGH✓ Silently incorrect behavior
Typo "delayed-suthenticator-config" → "authenticator"BackwardsCompatibilityUserStorageTest.java:163LOW✓ Typo
Hardcoded username "otp1" in assertUserDontHaveDBCredentials() prevents reuseBackwardsCompatibilityUserStorageTest.java:441LOW✓ Lack of parameterization
Potential NPE via equals() without null guard on form parametersRecoveryAuthnCodesAction.java:93LOW✓ Potential crash
Inconsistent use of var — explicit typing for recoveryCodeCredentialProvider would be more readableCredentialHelper.java:114LOW✓ Convention

Honest Analysis

FriendlyReviewer detected both golden bugs and went further. The Optional.get() finding (#1) was decomposed into two separate calls (L19 and L21) — both crash the login page. The missing credential ID (#2) was upgraded from LOW to HIGH. Both upgrades are justified: these crashes affect end users on the login page.

The 10 supplementary findings include one critical security vulnerability that no golden comment covered: unhashed recovery codes are serialized to JSON and passed to user storage providers, bypassing RS512 hashing. This is a cleartext credential leak.

Three additional HIGH-severity bugs in BackwardsCompatibilityUserStorage (NPE on getMyUser(), missing recovery codes in getDisableableCredentialTypesStream(), and disableCredentialType() silently ignoring recovery codes) show that FriendlyReviewer thoroughly evaluated the test storage provider implementation — code that the golden set did not examine at all.

The broken credential lifecycle management for federated storage (two MEDIUM findings) is a correct identification of a design flaw: post-validation cleanup is ineffective for user storage providers because isValid() never removes used codes.

FriendlyReviewer's full review

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

friendly-reviewerbotcommented now

Hi there 👋

🌥️ Tech
7 high, 4 medium, 4 low

This merge request introduces federated user storage support for recovery authentication codes, refactoring the flow to leverage a shared utility method. However, critical issues must be addressed: a security vulnerability leaks unhashed recovery codes to user storage providers, and multiple unchecked Optional.get() calls will crash the login page when credentials are missing or exhausted. Additional concerns include broken lifecycle management for federated credentials, potential null pointer exceptions, and incomplete implementations in the test user storage provider. The review recommends changes before merging.

These might need a close look

  • 🔴 server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java (L118)
    Critical security: Raw (unhashed) recovery codes are serialized to JSON and passed to user storage providers via updateCredential(), bypassing the RS512 hashing applied by RecoveryAuthnCodesCredentialModel.createFromValues(). Use hashed credential data instead.
  • 🔴 services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java (L19)
    Critical crash: credentialModelOpt.get() called without checking isPresent(). If no recovery credential exists, a NoSuchElementException is thrown during login page render. Add a guard and handle the absent case.
  • 🔴 services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java (L21)
    Critical crash: getNextRecoveryAuthnCode().get() called without checking isPresent(). When all codes are used, a NoSuchElementException crashes the login page. Add a guard and handle the all-codes-used case.
  • 🔴 testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (L230)
    Critical: Missing credential ID in getCredentials(). createFromValues() does not set the ID, so downstream operations like removeStoredCredentialById() will receive null. Set the ID after creation.
  • 🔴 testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (L235)
    High risk: getMyUser() may return null, but myUser.recoveryCodes is accessed without a null check, causing a NullPointerException. Add a null check early.
  • 🔴 testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (L270)
    High: getDisableableCredentialTypesStream() does not include recovery codes, even though the storage supports them. Add recovery codes to the returned types.
  • 🔴 testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (L281)
    High: disableCredentialType() has no handler for RecoveryAuthnCodesCredentialModel.TYPE. Disabling recovery codes is silently ignored. Add a handler to set recoveryCodes to null.

Worth checking

  • 🟡 services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java (L86)
    Medium: Post-validation credential cleanup is ineffective for federated storage. isValid() in user storage does not remove used codes, and getCredential() reconstructs models with all codes available, so allCodesUsed() never returns true. The credential is never cleaned up and re-generation is never triggered.
  • 🟡 server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java (L55)
    Medium: Inconsistency between model reconstruction and validation. getCredential() returns a model with hashed codes (via createFromValues), while isValid() in user storage does raw string comparison. This also causes allCodesUsed() to always return false, preventing proper lifecycle management.
  • 🟡 testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (L238)
    Medium: Unchecked type warning. JsonSerialization.readValue(..., List.class) returns raw List. Use new TypeReference<List<String>>() {} for type safety.
  • 🟡 testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (L320)
    Medium: Unchecked type warning. JsonSerialization.readValue(..., List.class) returns raw List. Use typed TypeReference for safe deserialization.

Small things (take or leave)

  • 🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (L163)
    Low: Typo in flow alias "delayed-suthenticator-config" (missing 'au'). Fix to "delayed-authenticator-config".
  • 🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (L441)
    Low: Hardcoded username "otp1" in assertUserDontHaveDBCredentials() prevents reuse. Make username a parameter.
  • 🔵 services/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java (L93)
    Low: Potential NullPointerException from equals() calls without null guard on form parameters. Use Objects.equals() for null-safe comparison.
  • 🔵 server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java (L114)
    Low: Inconsistent use of var for recoveryCodeCredentialProvider. Explicit typing improves readability and aligns with existing code.