TRUNK-6672: Add Password Hash Algorithm Detection#6311
Conversation
|
@claude review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
|
|
||
| if (PasswordHashDetector.isArgon2Hash(hashedPassword)) { | ||
| return new Argon2PasswordEncoder().verify(hashedPassword, passwordToHash); |
There was a problem hiding this comment.
This will break login for every Argon2-hashed account. Both real callers of hashMatches concatenate the OpenMRS salt onto the password before calling it: LoginCredential.checkPassword does Security.hashMatches(getHashedPassword(), pw + getSalt()), and HibernateContextDAO's DB-authentication path does Security.hashMatches(passwordOnRecord, password + saltOnRecord). Argon2's encoded hash format already embeds its own salt and Argon2.verify(hash, password) expects the bare password, not password+externalSalt. So new Argon2PasswordEncoder().verify(hashedPassword, passwordToHash) here receives password + salt instead of password, and verification will fail on the correct password whenever the account's salt column is non-empty, which is the normal case (nothing in this PR sets it to empty for Argon2 records). PasswordHashDetectorTest and the existing SecurityTest cases call hashMatches/isArgon2Hash directly without appending a salt, so they don't exercise this path and don't catch it. This needs to strip the OpenMRS salt before delegating to Argon2 verification (or the callers need a salt-free path for Argon2 records) before this can actually authenticate anyone.
| public boolean verify(String hashedPassword, String password) { | ||
| Argon2 argon2 = Argon2Factory.create(); | ||
| try { | ||
| return argon2.verify(hashedPassword, password.toCharArray()); |
There was a problem hiding this comment.
Worth wiping the password out of memory once verification is done. argon2-jvm's Argon2 interface documents wipeArray(char[]) as: "Use this method after hash creation / verification on the array which contains the user password." Here password.toCharArray() is created inline and never referenced again, so the plaintext password sits on the heap until GC reclaims it rather than being zeroed immediately, which is exactly what the library's own API is there to avoid. Capture the char[] in a local, pass it to verify, and call argon2.wipeArray(chars) in a finally block.
| </dependency> | ||
| <dependency> | ||
| <groupId>de.mkammerer</groupId> | ||
| <artifactId>argon2-jvm</artifactId> |
There was a problem hiding this comment.
Is the license on this dependency cleared? argon2-jvm 2.11's published POM lists it (and its argon2-jvm-nolibs runtime dependency) as LGPL-3.0-only, which is copyleft with distribution obligations (dynamic-linking/relinking rights, NOTICE, etc.). I don't see any other GPL/LGPL dependency anywhere else in this repo's poms, and OpenMRS itself ships under MPL 2.0 plus the healthcare disclaimer, so this looks like it'd be the project's first LGPL runtime dependency. Worth confirming this was a deliberate call before merge rather than an overlooked side effect of picking this Argon2 binding.
| * @param hashedPassword the stored password hash to analyze | ||
| * @return the detected algorithm, or {@link PasswordHashAlgorithm#UNKNOWN} if unrecognized | ||
| */ | ||
| public static PasswordHashAlgorithm detectAlgorithm(String hashedPassword) { |
There was a problem hiding this comment.
PasswordHashAlgorithm.SHA_512 and SHA_1 are defined but detectAlgorithm can never return them, it only ever distinguishes ARGON2 from UNKNOWN, and nothing outside this class's own test calls detectAlgorithm at all. If detecting those two formats isn't in scope for this PR, consider dropping the unused constants (or noting in their javadoc that they're reserved for follow-up work) so a caller doesn't assume SHA hashes are already being identified here.
There was a problem hiding this comment.
This is still true, and it's grown since this was posted: the "Remove verification" commit deleted Argon2PasswordEncoder along with the Security.hashMatches call site that used to invoke this class. So it isn't just SHA_512/SHA_1 that are unreachable now — detectAlgorithm and isArgon2Hash have zero callers anywhere outside PasswordHashDetectorTest (verified with a repo-wide grep for PasswordHashDetector). As merged, this adds a detection utility that never runs against a real stored password hash and doesn't route anything to any encoder, which is what the PR description says it does. Either wire Security.hashMatches back up to use it — handling the OpenMRS salt correctly this time, per the salt issue raised above — or scope this PR down to just the enum/cleanup pieces until that wiring actually lands.
|
@claude review |
solomonfortune
left a comment
solomonfortune
left a comment
There was a problem hiding this comment.
Hey @jwnasambu, this looks like the same salt issue I ran into on #6230 (TRUNK-6666) while building the Argon2id lazy-rehash path.
Both real callers of hashMatches append the OpenMRS salt before calling it (LoginCredential.checkPassword does pw + getSalt(), and the DB-auth path in HibernateContextDAO does password + saltOnRecord). Argon2's encoded hash already carries its own salt, so Argon2PasswordEncoder.verify() needs the bare password, not password+externalSalt. Passing the concatenated value in will fail verification for every real Argon2 account.
In #6230 I consolidated this into a single Security.passwordMatches(storedHash, rawPassword, salt) helper that branches on legacy vs. Argon2id and only applies the OpenMRS salt on the legacy branch. Worth noting though is that @ibacher flagged in review that this shouldn't be adding new public API surface, so the exact shape of that helper (and whether it stays public) is still being worked out. Treat it as a reference for the fix pattern rather than a finished API to copy. Happy to sync once it settles if that'd help.
|
@claude review |
| </dependency> | ||
| <dependency> | ||
| <groupId>com.password4j</groupId> | ||
| <artifactId>password4j</artifactId> |
There was a problem hiding this comment.
Now that Argon2PasswordEncoder (the only caller of password4j) has been deleted, this dependency has no import anywhere in the codebase (a repo-wide grep for password4j only turns up these two pom.xml entries). If this merges as-is it pulls in a new runtime dependency, and everything it drags in transitively, purely to back a detection class that also has no caller in production code (see the thread on PasswordHashDetector.java). Either finish wiring an Argon2 verifier through Security.hashMatches so this dependency is actually used, or drop it along with PasswordHashDetector until that's ready.
|
@claude review |
There was a problem hiding this comment.
@jwnasambu
Thanks for cleaning up the unused SHA_512/SHA_1 placeholders. I think that's resolved. The other three items from the last review still need attention though, most importantly the salt-handling bug in Security.java. hashMatches is still receiving password + salt from both real callers which are LoginCredential.checkPassword and HibernateContextDAO's DB-auth path, but Argon2PasswordEncoder.verify() needs the bare password since Argon2's hash already embeds its own salt. This will fail to authenticate any real Argon2 account. This is the same issue I hit on #6230.
Also, the argon2-jvm LGPL-3.0 license question from the earlier review. This is also not yet addressed and it may be the project's first LGPL runtime dependency. The wipeArray() suggestion on Argon2PasswordEncoder.verify() — that thread shows as "Outdated" but I don't think the fix actually landed, looks like it just got shifted by the merge commit. Worth confirming.
|
@solomonfortune Thank you for the feedback! You're absolutely right about the salt-handling issue—passing password + salt directly to Argon2 will break real logins. Because of this (and the licensing/dependency concerns), I ended up stripping out the verification logic (Argon2PasswordEncoder) in the latest commits to keep this PR strictly limited to the detection utility. However, as the bot pointed out, this leaves the detector with no production callers. As a way forward, I have kept this PR this PR strictly limited to adding the PasswordHashDetector and the enum, leaving the actual wiring/verification to be integrated alongside your work in #6230. Kindly let me know if the is something I am messing out based on your explanation and I will gladly fix it!" |
| } | ||
| String s = hashedPassword.trim(); | ||
| // Match standard Argon2 encoded hashes: $argon2id$..., $argon2i$..., $argon2d$... | ||
| return s.matches("^\\$argon2(?:id|i|d)\\$.*"); |
There was a problem hiding this comment.
All the regexes are static and should be statically compiled rather than defined inline. I'm not sure we should be building an Argon2Hash detector. There's got to be part of Spring's machinery (which is generating this) we can reuse for this and then fallback to the length-based detection.
|
3 participants
Description of what I changed
I introduced password hash format detection for Argon2 based passwords allowing the authentication system to identify modern password hashes and route them to the appropriate encoder. This is detection-only — no stored passwords are modified and no password migration occurs.
Issue I worked on
see https://openmrs.atlassian.net/browse/TRUNK-6672
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
./mvnw clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master