Repository navigation
Replace a credential only if unchanged when re-hashing a password - #55030
Conversation
Add updateStoredCredential(oldCred, newCred) that replaces a stored credential only if it still has the content of the old version, using a pessimistic lock in the JPA stores. Use it in the deferred password re-hash, so that it no longer overwrites a credential that changed after it was validated. Closes keycloak#55028 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
There was a problem hiding this comment.
🟡 Changes recommended
The tests do not cover a stale persistence context after a concurrent password change, leaving rejection of the conditional update and preservation of the newer password unverified.
What changed in this PR
This PR addresses #55028 by making deferred password re-hashing conditional on the credential remaining unchanged, protecting intervening password and metadata updates.
Changes:
- Adds conditional-update APIs and credential-content comparison, with compatibility defaults for existing implementations.
- Implements locking in both JPA credential stores and routes conditional updates through credential managers and cache invalidation.
- Adds regression tests for intervening password changes, unchanged-password re-hashing, and conditional replacement.
| File | Description |
|---|---|
tests/base/src/test/java/org/keycloak/tests/model/PasswordRehashTest.java |
Tests re-hashing and conditional credential replacement. |
services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java |
Uses conditional replacement for deferred re-hashing. |
server-spi/src/main/java/org/keycloak/models/SubjectCredentialManager.java |
Adds the conditional-update manager API. |
server-spi/src/main/java/org/keycloak/credential/UserCredentialStore.java |
Adds the conditional-update storage API. |
server-spi/src/main/java/org/keycloak/credential/CredentialModel.java |
Adds credential-content comparison. |
server-spi-private/src/main/java/org/keycloak/models/light/EmptyCredentialManager.java |
Returns false for conditional updates. |
model/storage/src/main/java/org/keycloak/storage/federated/UserFederatedUserCredentialStore.java |
Adds conditional updates to the federated storage API. |
model/storage/src/main/java/org/keycloak/credential/UserCredentialManager.java |
Delegates conditional updates to the appropriate store. |
model/jpa/src/main/java/org/keycloak/storage/jpa/JpaUserFederatedStorageProvider.java |
Implements locked conditional updates for federated credentials. |
model/jpa/src/main/java/org/keycloak/models/jpa/JpaUserProvider.java |
Delegates conditional updates to the JPA credential store. |
model/jpa/src/main/java/org/keycloak/models/jpa/JpaUserCredentialStore.java |
Locks and refreshes local credentials before comparison. |
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/SubjectCredentialManagerCacheAdapter.java |
Invalidates cached state before conditional updates. |
Review effort: Balanced
Closes keycloak#55028 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive credential atomicity depends on Hibernate locking and refresh semantics that need database-backed validation, especially under MySQL/MariaDB REPEATABLE READ, while the federated JPA path remains untested.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
Closes keycloak#55028 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive credential-write atomicity depends on database-specific locking and refresh behavior, requiring maintainer validation across supported databases and the untested federated-store path.
No open issues 🎉
Review effort: Balanced
Closes keycloak#55028 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
… is usable Closes keycloak#55028 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
e27c2fa to
6451e56
Compare
|
@ahus1 I've just briefly reviewed and it seems that the approach could also be used to resolve the: #54414 And is there any specific reason to use pessimistic locking instead of optimistic? As we have the version column on the entity, there should not be a problem, right? Maybe for federated credentials? |
@mabartos - IMHO optimistic locking would not work here. By default, it works only if someone modifies the row concurrently between reading and writing it within the same transaction. Here, we're reading in one transaction (the password check), and later writing in another transaction (the rehashing). We could make it work by memorizing the version we saw in the first transaction and then compare it in the second transaction. For example, by encoding it the credential data. But this is something we don't do anywhere (yet) in Keycloak, so this is the approach I've chosen. If we would forward the version ID to the caller (for example in the Admin UI), we would also be able to recognize if someone has changed the data in the meantime. Again, nothing we do today, and outside of the scope. So I'd like to proceed with the PR as is. |
@ahus1 Thanks for the further explanation. Yep, let's stick with the common way we do in Keycloak, as pessimistic locking should not cause any problem for this. |
|
Can someone from the @keycloak/core-iam team help out in reviewing the JPA parts of this PR? Once that's in, it should be ready for merging. Thanks! |
Closes #55028
Second part of the split of #53742, after #55027.
Decisions and assumptions
updateStoredCredential(oldCred, newCred)is a compare-and-swap: it replaces the credential only if the stored one still has the content ofoldCred(type, label, created date, secret data, credential data), and returnsfalseotherwise. Comparing the whole credential also means the re-hash doesn't overwrite a concurrent change of the label or created date.PasswordHashingTeststill cover the happy path and are left in place, to be migrated in a separate PR.Based on the work of @d53dave in #53742.