TRUNK-6666: Automatic Password Upgrade on Successful Login#6230
TRUNK-6666: Automatic Password Upgrade on Successful Login#6230solomonfortune wants to merge 12 commits into
Conversation
|
Is this still in draft? Or ready for review? |
I had a few tasks i was finalising yesterday and hopefully finished them today. It's now ready for review |
|
Can you check the build failures? |
…e salt for secret answer
@dkayiwa |
|
@solomonfortune did you see the above build failure? |
|
@dkayiwa i think this is a different cause of failure than the first one according to the explanantions that I tried to look through. So let me also address this one too. |
|
@dkayiwa The changeset count in DatabaseUpdaterDatabaseIT has been updated from 904 to 905 to account for the new Liquibase migration added in this PR. That was the cause of the build failures |
|
@claude review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Lazy rehash: if password is legacy, upgrade to Argon2id transparently | ||
| if (Security.isLegacyHash(passwordOnRecord)) { | ||
| try { | ||
| upgradePasswordHash(candidateUser, password); |
There was a problem hiding this comment.
This needs fixing before merge: upgradePasswordHash is called here as a plain this-scoped call, from authenticate() in the very same class. Spring's transaction advice is applied through a proxy, and self-invocation bypasses that proxy entirely, so the @Transactional(propagation = Propagation.REQUIRES_NEW) on upgradePasswordHash (line 374) is silently ignored. In practice the upgrade write runs inside the exact same transaction as authenticate(), not an isolated one, which contradicts the method's own javadoc ("only the upgrade is rolled back — the authentication that already succeeded is never affected") and defeats the point of REQUIRES_NEW. Because the call is still wrapped in a try/catch here, the existing test (authenticate_shouldUpgradeLegacyPasswordToArgon2idOnSuccessfulLogin) passes either way, so this doesn't show up in CI. To actually get a separate transaction, the call needs to go through the Spring-managed proxy, e.g. inject ContextDAO/self-reference (@Autowired self-injection or AopContext.currentProxy() with exposeProxy) and invoke the annotated method through that, or move upgradePasswordHash to a different bean that this class calls through its injected reference.
There was a problem hiding this comment.
upgradePasswordHash() is now called through AopContext.currentProxy() instead of this, ensuring the @transactional(propagation = Propagation.REQUIRES_NEW) annotation is honoured by the Spring proxy. expose-proxy="true" has been added to the AOP configuration in applicationContext-service.xml to enable this.
|
|
||
| /** | ||
| * The modern password encoder using Argon2id algorithm. | ||
| * Used for all new password hashing and lazy rehash upgrades. |
There was a problem hiding this comment.
Is this claim actually true? Tracing where password hashes get written outside the lazy-rehash path: HibernateUserDAO.changePassword(User, String) and HibernateUserDAO.changePassword(String, String) (both unchanged by this PR) still hash new passwords with Security.encodeString(pw + salt), never with this Argon2id encoder. Concretely that means a brand-new user's initial password is never Argon2id, and, more importantly, a user who was already upgraded to Argon2id via the login-triggered lazy rehash gets silently downgraded back to a legacy SHA-512-style hash the very next time they change their password through the normal self-service or admin-reset flow. There's no test exercising changePassword() that checks the resulting hash format, so this regression wouldn't be caught. If the intent of this PR is scoped to only the login-rehash path, that's a significant gap worth calling out explicitly (e.g. in the PR description) since it undermines the migration for any account whose password is changed proactively.
There was a problem hiding this comment.
Both changePassword(User, String) and changePassword(String, String) in HibernateUserDAO now hash new passwords using Security.getArgon2Encoder().encode() instead of Security.encodeString(). The existing salt is preserved after the hash upgrade so secret answer verification continues to work correctly.
There was a problem hiding this comment.
changePassword() now using Argon2id closes the downgrade-on-password-change gap, but HibernateUserDAO.saveUser() (lines 105-106 in the current head) still hashes a brand-new user's initial password with Security.encodeString(password + salt), not Security.getArgon2Encoder(). That's the method UserService.createUser() calls (and HibernateContextDAO.createUser() via Daemon.createUser()), so every newly created account still starts out on the legacy SHA-512-style hash and only gets upgraded once it logs in and hits the lazy-rehash path.
| UserDAO userDAO = (UserDAO) applicationContext.getBean("userDAO"); | ||
| LoginCredential credential = userDAO.getLoginCredential(user); | ||
| assertNotNull(credential.getHashedPassword()); | ||
| assertTrue(credential.getHashedPassword().startsWith("$argon2id$"), |
There was a problem hiding this comment.
This asserts the stored hash flips to Argon2id, but nothing here or elsewhere in the suite then authenticates against an Argon2id hash, so the modern verification path is never actually run. Every authenticate test starts from the admin account, whose seeded password is a legacy hash (in fact the 39 character buggy SHA-1 from the #1178 dataset, not SHA-512 as the comment says), so isPasswordMatch and LoginCredential.checkPassword only ever take their legacy branch. Security.getArgon2Encoder().matches(rawPassword, storedHash) has no coverage.
That branch is the endpoint of the whole migration: once an account is upgraded, every later login depends on it. If it regresses (argument order, an encoder parameter change, a salt accidentally appended to the input) then every already-upgraded user is locked out on their next login and CI stays green, because nothing logs in against an Argon2id hash. The current argument order is correct, so this is about guarding the path rather than a bug that is live today.
A reliable way to cover it is to seed a user whose stored hash is Security.getArgon2Encoder().encode("somePassword") and assert dao.authenticate succeeds with the right password and fails with a wrong one. Re-authenticating admin a second time in this test will not exercise it, because the native select password, salt query plus the session's MANUAL flush mode re-reads the original legacy hash. A small direct LoginCredential.checkPassword test with an Argon2id hash would cover the other copy. Worth adding before this merges, given the failure mode is a silent lockout.
There was a problem hiding this comment.
A new user with a pre-seeded Argon2id hash has been added to contextDAOTest.xml. Two new tests have been added to ContextDAOTest. One asserting successful authentication against an Argon2id hash and one asserting correct rejection of a wrong password. A direct LoginCredential.checkPassword() test with an Argon2id hash has also been added to SecurityTest to cover that verification path independently.
| * @param salt the legacy salt from the database (may be null for Argon2id) | ||
| * @return true if the password matches | ||
| */ | ||
| private boolean isPasswordMatch(String storedHash, String rawPassword, String salt) { |
There was a problem hiding this comment.
This is the same legacy-versus-Argon2 decision that LoginCredential.checkPassword now also implements (LoginCredential line 149), copied almost verbatim. Two independent copies of the password-verification rule tend to drift: the day someone teaches one branch about another stored format, swaps the encoder, or changes how the salt is applied, the other copy quietly keeps the old behavior and authentication starts depending on which entry point you came through (login through here, change-password through checkPassword). For verification code that is an unpleasant way to get an inconsistency.
I would pull this into one helper, for example Security.passwordMatches(storedHash, rawPassword, salt), and have both callers use it. Not blocking, but cheap to do now while both copies are fresh.
There was a problem hiding this comment.
The verification logic has been consolidated into a single Security.passwordMatches(storedHash, rawPassword, salt) helper method. Both isPasswordMatch() in HibernateContextDAO and checkPassword() in LoginCredential now delegate to this single method, eliminating the duplicate implementation.
…ity.passwordMatches(), fix Argon2id verification in checkPassword() and changePassword(), add Argon2id authentication tests, fix AopContext proxy for REQUIRES_NEW transaction isolation, update Liquibase changeset count
|
|
@dkayiwa I went through the comments and tried my best to fix the issues |
|
@claude review |
|
|
||
| @Autowired | ||
| private SearchSessionFactory searchSessionFactory; | ||
| private HibernateContextDAO self; |
There was a problem hiding this comment.
Nit: this field is never read, never assigned, and has no setter or Spring wiring for it — looks like a leftover from an earlier self-injection approach that got replaced by AopContext.currentProxy() in authenticate(). Worth deleting.
| */ | ||
| public static Argon2PasswordEncoder getArgon2Encoder() { | ||
| return ARGON2_ENCODER; | ||
| } |
There was a problem hiding this comment.
This does not feel like a method that exists. More exactly, callers of the Security class should not need to know what encoding or encoders we are using. Instead of creating new APIs, the focus here should be updating the existing APIs so that existing flows still continue to work.
There was a problem hiding this comment.
In fact, this PR should have zero new public API space.
There was a problem hiding this comment.
In fact, this PR should have zero new public API space.
thanks @ibacher for this feedback. let me work on this immediately. getArgon2Encoder() really leaks the implementation detail and creates unnecessary public API surface.
There was a problem hiding this comment.
@ibacher getArgon2Encoder() has been made private and upgradePasswordHash() reduced to package-private. passwordMatches() and isLegacyHash() are used by LoginCredential.checkPassword() which lives in a different package and making them package-private would break that. Would you prefer we move the verification logic entirely into Security.java and have LoginCredential.checkPassword() call Security.passwordMatches() keeping it public, or is there another approach you have in mind?
There was a problem hiding this comment.
Checking this against the current head (01ffbff, which is still the PR's tip): none of the three changes described above are actually in the code. getArgon2Encoder() is still public (Security.java:119), passwordMatches() is still public (Security.java:98), and upgradePasswordHash() is still public (HibernateContextDAO.java:374) — none show as private or package-private. There's no commit after 01ffbff (2026-07-08) touching either file, so this looks like the follow-up commit described here didn't actually get pushed. Worth confirming before treating this as addressed, since @ibacher's "zero new public API space" ask is otherwise still open.
|
@claude review |
4 participants
Description of what I changed
spring-security-cryptodependency for Argon2id password hashing supportusers.passwordcolumn length from 128 to 255 via Liquibase migration to accommodate Argon2id hash outputisLegacyHash()toSecurity.javato detect all three legacy hash formats (SHA-512, SHA-1 correct, SHA-1 buggy from ticket Changed String constructor to valueOf #1178)Argon2PasswordEncoderinstance toSecurity.javawith production-ready default parametersisLegacyHash()covering all legacy formats and edge casesNote: This is a draft PR. Tasks remaining — upgrade hook in
authenticate(), transaction isolation, and thread safety — will be added in subsequent commits.Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-6666
Checklist: I completed these to help reviewers :)
mvn clean packageright before creating this pull request and added all formatting changes to my commit.