8000
Stop the deferred password re-hash from overwriting a password changed after validation by d53dave · Pull Request #53742 · keycloak/keycloak · GitHub
You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Stop the deferred password re-hash from overwriting a password changed after validation - #53742
1. The deferred re-hash no longer overwrites a password that changed after it was validated (PasswordCredentialProvider)
rehashPasswordIfRequired now passes the secret data of the credential it validated to the re-hash job. refreshPassword then works in this order:
Compute the new hash.
Read the credential from local storage (UserStoragePrivateUtil.userLocalStorage), not through the user cache.
If the credential is gone or its secret data changed, skip the write.
Otherwise update it through the user model, as before.
Step 2 loads the CredentialEntity into the job's persistence context. The update in step 4 then works on that managed entity. If a change commits between the read and the write, the @Version check on CredentialEntity rejects the update. The ModelException is already caught and logged.
Ordering is important. Comparing before computing the hash, or fetching from the user cache, will still let concurrent changes through: argon2 hashing is potentially slow, and the cached credential can be stale. In our tests that version reverted about 22% of the concurrent rounds.
2. Argon2 policyCheck uses the same iterations as encodedCredential (Argon2PasswordHashProvider)
encodedCredential uses the realm policy hashIterations(n) when it is set and ≤ 100. policyCheck always compared against the provider's configured iterations. With e.g. hashIterations(2), a hash that was just created was considered outdated, so every successful login queued a re-hash. Both methods now get the expected iterations from one helper. The previous Javadoc said the policy iterations were ignored, which encodedCredential didn't do; I updated it to describe the actual behaviour.
Change 2 alone removes the "every login" trigger, but change 1 is still needed. A hash that is legitimately outdated (after a policy or algorithm change) can still overwrite a concurrent or same-transaction password change.
Tests
I added four tests to PasswordHashingTest. They go into the existing Arquillian test because #51644 (migrating it to the new test framework) is still open. All four are deterministic and don't depend on timing.
testPasswordNotRehashedWithArgon2PolicyIterations: with hashIterations(2) and Argon2, a login doesn't re-hash a fresh hash (the salt is unchanged).
testPasswordChangedAfterValidationInSameTransactionNotReverted: with an outdated hash, isValid(old) and then updateCredential(new) run in one transaction. After the commit, the new password must be valid and the old one invalid.
testPasswordChangedConcurrentlyAfterValidationNotReverted: with an outdated hash, isValid(old) queues a re-hash. Then a separate transaction (standing in for a concurrent request, e.g. an admin reset) sets the new password and commits before the validating transaction does.
testPasswordChangedWhileRehashingNotReverted: the new password is committed by a separate transaction while the re-hash job computes the hash. This uses a test-only PasswordHashProvider in testsuite-providers (PasswordChangingHashProviderFactory): PBKDF2-SHA512 that, when configured through realm attributes, changes the user password in a separate transaction during encodedCredential. This test pins the order inside refreshPassword: hash first, then read and compare.
Results:
With the fix, all 17 tests in PasswordHashingTest pass, and spotless:check passes for the changed modules.
On stock main, all four new tests fail. The re-hash tests report "New password must be valid", and the argon2 test finds a changed salt.
With only the PasswordCredentialProvider change reverted, the re-hash tests still fail. The argon2 change alone doesn't prevent the revert.
testPasswordChangedWhileRehashingNotReverted also fails with a variant of the fix that compares, through the user cache, before hashing. That variant reads the credential from the cache, the password change commits during hashing, and the write then reverts it.
A change that commits between the jobs read and its write is not covered by a dedicated test. That window is guarded by the existing @Version optimistic locking on CredentialEntity, because the job's read loads the entity the update then uses. I saw this guard work in one of the control runs above (a variant that reads through local storage before hashing): Hibernates OptimisticLockException was converted to a ModelIllegalStateException, caught, and logged at INFO ("Error re-hashing the password in a different transaction"). The validating request didn't fail, and the new password was kept.
AI disclosure: parts of this PR (investigation, scripts to reproduce, some of the test code) were done using Claude Code.
I've addressed the federated user case in e5b16a5.
FederatedUserCredentialEntity now has @Version, with a VERSION column on FED_USER_CREDENTIAL (changeset 27.0.0-53633, the same pattern as 26.2.0-26106 for CREDENTIAL). The job's read loads the entity into its persistence context, and the update works on that entity. So the write is now UPDATE ... WHERE ID = ? AND VERSION = ?. If a change commits after the read, the update matches 0 rows, and the resulting ModelException is caught and logged like for local credentials. Comparing SECRET_DATA in the UPDATE itself isn't portable AFAICT, because it's a CLOB.
I chose this over pessimistic locking for 2 reasons:
services only sees the UserFederatedUserCredentialStore SPI, so a locking read would need a new SPI method.
A lock would only protect the re-hash. The version also protects other concurrent writers of federated credentials, which is why CREDENTIAL got one in Recovery Code Validation Race Possible #26106.
New tests cover this interval for both storages: testPasswordChangedBeforeRehashCommitNotReverted and testFederatedStoragePasswordChangedBeforeRehashCommitNotReverted.
The log shows the 0-row update on CREDENTIAL and FED_USER_CREDENTIAL respectively
Manually removing @Versionmakes both fail with "New password must be valid".
The guard only compares secret data, but the subsequent update still writes the label captured before hashing. If a credential-label update commits during hashing, this check passes and the re-hash reverts that newer label; copy the label from current after the comparison (or include it in the guard).
@d53dave I've just added one commit on top of it to be safe for backports with the DB migration + keeping the metadata on the password credential.
As we want to have also the tests for the new testsuite, we can make some changes there - I think we can simplify it. I'll try to produce the new tests and let you know if it's ok to you. Thanks!
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR. This tries to fix two problems at the same time, and I appreciate it. Still I think the "avoid overwriting with a rehashed password" needs some more as it is a hack. Possibly we would need a new method updateStoredCredential(oldCred, newCred) to make this a good abstraction.
As a way forward, I suggest to have 2-3 PRs, handled one after the other:
Fix the Argon2 parameter problem, which will also be backported to 26.6.x
Rework the interfaces, and add the new method. It should not add the new column so it can be backported to 26.6. Instead of using optimistic locking, it can use pessimistic locking.
For 27.x only, we can add the new column and change the backend from pessimistic locking to optimistic locking for the federated credentials.
1 and 2 could possibly be combined, but it makes it harder to discuss so we would actually be slower.
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
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.
Closes #53633
Scope
1. The deferred re-hash no longer overwrites a password that changed after it was validated (
PasswordCredentialProvider)rehashPasswordIfRequirednow passes the secret data of the credential it validated to the re-hash job.refreshPasswordthen works in this order:UserStoragePrivateUtil.userLocalStorage), not through the user cache.Step 2 loads the
CredentialEntityinto the job's persistence context. The update in step 4 then works on that managed entity. If a change commits between the read and the write, the@Versioncheck onCredentialEntityrejects the update. TheModelExceptionis already caught and logged.Ordering is important. Comparing before computing the hash, or fetching from the user cache, will still let concurrent changes through: argon2 hashing is potentially slow, and the cached credential can be stale. In our tests that version reverted about 22% of the concurrent rounds.
2. Argon2
policyCheckuses the same iterations asencodedCredential(Argon2PasswordHashProvider)encodedCredentialuses the realm policyhashIterations(n)when it is set and ≤ 100.policyCheckalways compared against the provider's configured iterations. With e.g.hashIterations(2), a hash that was just created was considered outdated, so every successful login queued a re-hash. Both methods now get the expected iterations from one helper. The previous Javadoc said the policy iterations were ignored, whichencodedCredentialdidn't do; I updated it to describe the actual behaviour.Change 2 alone removes the "every login" trigger, but change 1 is still needed. A hash that is legitimately outdated (after a policy or algorithm change) can still overwrite a concurrent or same-transaction password change.
Tests
I added four tests to
PasswordHashingTest. They go into the existing Arquillian test because #51644 (migrating it to the new test framework) is still open. All four are deterministic and don't depend on timing.testPasswordNotRehashedWithArgon2PolicyIterations: withhashIterations(2)and Argon2, a login doesn't re-hash a fresh hash (the salt is unchanged).testPasswordChangedAfterValidationInSameTransactionNotReverted: with an outdated hash,isValid(old)and thenupdateCredential(new)run in one transaction. After the commit, the new password must be valid and the old one invalid.testPasswordChangedConcurrentlyAfterValidationNotReverted: with an outdated hash,isValid(old)queues a re-hash. Then a separate transaction (standing in for a concurrent request, e.g. an admin reset) sets the new password and commits before the validating transaction does.testPasswordChangedWhileRehashingNotReverted: the new password is committed by a separate transaction while the re-hash job computes the hash. This uses a test-onlyPasswordHashProviderintestsuite-providers(PasswordChangingHashProviderFactory): PBKDF2-SHA512 that, when configured through realm attributes, changes the user password in a separate transaction duringencodedCredential. This test pins the order insiderefreshPassword: hash first, then read and compare.Results:
PasswordHashingTestpass, and spotless:check passes for the changed modules.main, all four new tests fail. The re-hash tests report "New password must be valid", and the argon2 test finds a changed salt.PasswordCredentialProviderchange reverted, the re-hash tests still fail. The argon2 change alone doesn't prevent the revert.testPasswordChangedWhileRehashingNotRevertedalso fails with a variant of the fix that compares, through the user cache, before hashing. That variant reads the credential from the cache, the password change commits during hashing, and the write then reverts it.A change that commits between the jobs read and its write is not covered by a dedicated test. That window is guarded by the existing
@Versionoptimistic locking onCredentialEntity, because the job's read loads the entity the update then uses. I saw this guard work in one of the control runs above (a variant that reads through local storage before hashing): HibernatesOptimisticLockExceptionwas converted to aModelIllegalStateException, caught, and logged at INFO ("Error re-hashing the password in a different transaction"). The validating request didn't fail, and the new password was kept.AI disclosure: parts of this PR (investigation, scripts to reproduce, some of the test code) were done using Claude Code.