Implement recovery key support for user storage providers

[ShashankFC]

Test 9nn---nReplicated from ai-code-review-evaluation/keycloak-greptile#9

[GitHoobar]

@claude

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 11m 48s —— View job

---

Code Review Complete

I've reviewed the PR that adds recovery key support for user storage providers. Here are my findings:

Overview

This PR extends the recovery authentication codes feature to work with custom user storage providers, following the same pattern as OTP credentials. The implementation allows recovery codes to be stored either in user storage or local Keycloak storage.

---

Critical Issues

1. Potential NullPointerException in RecoveryAuthnCodeInputLoginBean (High Severity)

File: services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java:19

RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = 
    RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());

This code calls .get() on an Optional without checking if it's present first. If the credential doesn't exist, this will throw a NoSuchElementException.

Fix: Add null/empty check:

if (credentialModelOpt.isEmpty()) {
    throw new IllegalStateException("Recovery codes credential not found");
}
RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = 
    RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());

Fix this →

---

2. Missing Null Check on getNextRecoveryAuthnCode() (High Severity)

File: services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java:21

this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber();

Double .get() call without checking if the Optional is present. This will fail if all recovery codes have been used.

Fix: Add proper Optional handling:

this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode()
    .orElseThrow(() -> new IllegalStateException("No recovery codes available"))
    .getNumber();

Fix this →

---

3. Inconsistent Error Handling in User Storage Provider (Medium Severity)

File: testsuite/.../BackwardsCompatibilityUserStorage.java:243-244

} catch (IOException e) {
    log.error("Could not deserialize  credential of type: recovery-codes");
}

The exception is logged but swallowed. The method continues as if nothing happened, which could lead to silent failures.

Fix: Either rethrow as a runtime exception or return an error indicator:

} catch (IOException e) {
    log.error("Could not deserialize credential of type: recovery-codes", e);
    throw new RuntimeException("Failed to deserialize recovery codes credential", e);
}

---

Security Concerns

4. Weak Hashing Algorithm Choice (Medium Severity)

File: server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java:26

public static final String NOM_ALGORITHM_TO_HASH = Algorithm.RS512;

Using RS512 (an RSA signature algorithm) for hashing recovery codes is unusual and potentially inappropriate. RS512 is an asymmetric signature algorithm, not a hashing algorithm. This should use a proper hashing algorithm like SHA-256 or SHA-512.

Additionally: The variable name has a typo: NOM_ALGORITHM_TO_HASH should be HASH_ALGORITHM or similar.

Recommendation: Use a proper cryptographic hash algorithm. Consider using the same approach as password hashing with salt and iterations.

---

5. Only One Hash Iteration (Low-Medium Severity)

File: server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java:27

public static final int NUM_HASH_ITERATIONS = 1;

Single iteration hashing makes the codes more vulnerable to brute-force attacks if the hashed values are compromised. While recovery codes are typically single-use, multiple iterations would provide better security.

Recommendation: Increase to at least 10,000+ iterations or align with Keycloak's password hashing policy.

---

Code Quality Issues

6. Unnecessary Local Variable (Low Severity)

File: server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java:116

var recoveryCodeCredentialProvider = session.getProvider(CredentialProvider.class, "keycloak-recovery-authn-codes");

The var usage here is fine, but the variable name is verbose. Consider shortening to provider.

---

7. Inconsistent Credential Removal Logic (Medium Severity)

File: services/.../RecoveryAuthnCodesFormAuthenticator.java:86-87

if (recoveryCodeCredentialModel.allCodesUsed()) {
    authenticatedUser.credentialManager().removeStoredCredentialById(
        recoveryCodeCredentialModel.getId());
}

This only removes credentials from local storage (removeStoredCredentialById), but doesn't handle user storage credentials. This creates an asymmetry where user storage credentials aren't cleaned up when all codes are used.

Issue: If credentials are in user storage, they won't be removed, leading to inconsistent behavior.

---

8. Missing Input Validation (Low Severity)

File: server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java:115-131

The createRecoveryCodesCredential method doesn't validate:

• generatedCodes is not null or empty
• credentialModel is not null
• user and realm are not null

Recommendation: Add input validation with meaningful error messages.

---

Test Coverage Observations

9. Good Test Coverage ✓

The test file BackwardsCompatibilityUserStorageTest.java includes:

• Setup and login tests
• Credential storage verification
• Integration with authentication flows

Positive: The test properly validates that recovery codes work in user storage, following the same pattern as OTP tests.

---

10. Test Cleanup Consideration

File: testsuite/.../BackwardsCompatibilityUserStorageTest.java:267

// Revert copy of browser flow to original to keep clean slate after this test
BrowserFlowTest.revertFlows(testRealm(), BROWSER_FLOW_WITH_RECOVERY_AUTHN_CODES);

This cleanup is in a finally block, which is good practice. However, it would be safer to use @After annotation to ensure cleanup happens even if the test fails early.

---

Positive Aspects ✓

1. Consistent Pattern: The implementation follows the same pattern as OTP credentials, making it maintainable
2. Proper Abstraction: The RecoveryAuthnCodesUtils.getCredential() method properly checks both user storage and local storage
3. Logging: Good use of debug logging for tracking where credentials are created
4. Backwards Compatibility: The test suite ensures the feature works with legacy user storage implementations

---

Recommendations Summary

Must Fix (High Priority):

1. Fix NullPointerExceptions in RecoveryAuthnCodeInputLoginBean (lines 19, 21)
2. Review and fix the hashing algorithm choice (RS512 → proper hash function)
3. Handle IOException properly in BackwardsCompatibilityUserStorage.java

Should Fix (Medium Priority):
4. Fix inconsistent credential removal for user storage
5. Increase hash iterations for better security
6. Add input validation to createRecoveryCodesCredential

Nice to Have (Low Priority):
7. Fix typo: NOM_ALGORITHM_TO_HASH → HASH_ALGORITHM
8. Simplify variable names where appropriate

---