Implement recovery key support for user storage providers #2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
closes #38445 Signed-off-by: rtufisi <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Author
Greptile OverviewConfidence Score: 0/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant RecoveryAuthnCodesAction
participant CredentialHelper
participant UserStorage
participant LocalDB
participant Authenticator
Note over User,LocalDB: Recovery Codes Setup Flow
User->>RecoveryAuthnCodesAction: Setup recovery codes
RecoveryAuthnCodesAction->>RecoveryAuthnCodesAction: Generate raw codes
RecoveryAuthnCodesAction->>RecoveryAuthnCodesAction: Create credential model
RecoveryAuthnCodesAction->>CredentialHelper: createRecoveryCodesCredential(model, rawCodes)
CredentialHelper->>CredentialHelper: Serialize raw codes to JSON
CredentialHelper->>UserStorage: updateCredential(rawCodesJson)
alt User Storage accepts credential
UserStorage-->>CredentialHelper: true (stored in UserStorage)
Note over UserStorage: SECURITY ISSUE: Stores unhashed codes
else User Storage rejects credential
UserStorage-->>CredentialHelper: false
CredentialHelper->>LocalDB: createCredential(hashedModel)
Note over LocalDB: Stores hashed codes correctly
LocalDB-->>CredentialHelper: Credential created
end
CredentialHelper-->>RecoveryAuthnCodesAction: Success
RecoveryAuthnCodesAction-->>User: Display codes to save
Note over User,LocalDB: Recovery Codes Authentication Flow
User->>Authenticator: Enter recovery code
Authenticator->>Authenticator: RecoveryAuthnCodesUtils.getCredential()
Authenticator->>UserStorage: getFederatedCredentialsStream()
alt Credential in UserStorage
UserStorage-->>Authenticator: Recovery codes credential
Authenticator->>UserStorage: isValid(inputCode)
UserStorage->>UserStorage: Plain text comparison
Note over UserStorage: SECURITY ISSUE: Compares unhashed
UserStorage-->>Authenticator: Valid/Invalid
else Credential in LocalDB
UserStorage-->>Authenticator: Not found
Authenticator->>LocalDB: getStoredCredentialsByType()
LocalDB-->>Authenticator: Recovery codes credential
Authenticator->>LocalDB: isValid(inputCode)
LocalDB->>LocalDB: Hash input and compare
Note over LocalDB: Secure: Uses hash verification
LocalDB-->>Authenticator: Valid/Invalid
end
Authenticator-->>User: Authentication result
|
Comment on lines
+119
to
+123
| recoveryCodesJson = JsonSerialization.writeValueAsString(generatedCodes); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| UserCredentialModel recoveryCodesCredential = new UserCredentialModel("", credentialModel.getType(), recoveryCodesJson); |
There was a problem hiding this comment.
logic: Storing unhashed recovery codes in user storage is a critical security vulnerability. The generatedCodes list contains plain text codes, but user storage providers may persist these to their backend systems. When stored in local DB via createCredential, codes are hashed (via RecoveryAuthnCodesCredentialModel.createFromValues), but when stored in user storage via updateCredential, they remain unhashed.
Suggested change
| recoveryCodesJson = JsonSerialization.writeValueAsString(generatedCodes); | |
| } catch (IOException e) { | |
| throw new RuntimeException(e); | |
| } | |
| UserCredentialModel recoveryCodesCredential = new UserCredentialModel("", credentialModel.getType(), recoveryCodesJson); | |
| String recoveryCodesJson; | |
| try { | |
| // Hash the codes before storing, consistent with local storage | |
| List<String> hashedCodes = generatedCodes.stream() | |
| .map(RecoveryAuthnCodesUtils::hashRawCode) | |
| .collect(Collectors.toList()); | |
| recoveryCodesJson = JsonSerialization.writeValueAsString(hashedCodes); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java
Line: 119:123
Comment:
**logic:** Storing unhashed recovery codes in user storage is a critical security vulnerability. The `generatedCodes` list contains plain text codes, but user storage providers may persist these to their backend systems. When stored in local DB via `createCredential`, codes are hashed (via `RecoveryAuthnCodesCredentialModel.createFromValues`), but when stored in user storage via `updateCredential`, they remain unhashed.
```suggestion
String recoveryCodesJson;
try {
// Hash the codes before storing, consistent with local storage
List<String> hashedCodes = generatedCodes.stream()
.map(RecoveryAuthnCodesUtils::hashRawCode)
.collect(Collectors.toList());
recoveryCodesJson = JsonSerialization.writeValueAsString(hashedCodes);
```
How can I resolve this? If you propose a fix, please make it concise.| return false; | ||
| } | ||
|
|
||
| return generatedKeys.stream().anyMatch(key -> key.equals(input.getChallengeResponse())); |
There was a problem hiding this comment.
logic: Plain text comparison used instead of hash verification. Should use RecoveryAuthnCodesUtils.verifyRecoveryCodeInput() to validate hashed codes consistently with the main credential provider.
Suggested change
| return generatedKeys.stream().anyMatch(key -> key.equals(input.getChallengeResponse())); | |
| return generatedKeys.stream().anyMatch(key -> RecoveryAuthnCodesUtils.verifyRecoveryCodeInput(input.getChallengeResponse(), key)); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java
Line: 340:340
Comment:
**logic:** Plain text comparison used instead of hash verification. Should use `RecoveryAuthnCodesUtils.verifyRecoveryCodeInput()` to validate hashed codes consistently with the main credential provider.
```suggestion
return generatedKeys.stream().anyMatch(key -> RecoveryAuthnCodesUtils.verifyRecoveryCodeInput(input.getChallengeResponse(), key));
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
13
to
15
|
|
||
| private final int codeNumber; | ||
|
|
There was a problem hiding this comment.
logic: Calling .get() on Optional without checking isPresent() first will throw NoSuchElementException if credential not found
Suggested change
| private final int codeNumber; | |
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | |
| if (!credentialModelOpt.isPresent()) { | |
| throw new RuntimeException("Recovery codes credential not found for user"); | |
| } | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java
Line: 13:15
Comment:
**logic:** Calling `.get()` on `Optional` without checking `isPresent()` first will throw `NoSuchElementException` if credential not found
```suggestion
Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);
if (!credentialModelOpt.isPresent()) {
throw new RuntimeException("Recovery codes credential not found for user");
}
RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());
```
How can I resolve this? If you propose a fix, please make it concise.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Test 9
Replicated from ai-code-review-evaluation/keycloak-greptile#9