TRUNK-6662: Get unchanged password hashing working using Spring's interfaces#6219
TRUNK-6662: Get unchanged password hashing working using Spring's interfaces#6219jwnasambu wants to merge 5 commits into
Conversation
2cbe51f to
0d6ff3f
Compare
jwnasambu
changed the title
ibacher
left a comment
ibacher
left a comment
There was a problem hiding this comment.
Thanks @jwnasambu! I want to suggest basically a completely different implementation.
| String saltOnRecord = (String) passwordAndSalt[1]; | ||
|
|
||
| // if the username and password match, hydrate the user and return it | ||
| if (passwordOnRecord != null && Security.hashMatches(passwordOnRecord, password + saltOnRecord)) { |
There was a problem hiding this comment.
I think for most of these changes instead of updating all of these classes, we instead keep these implementations the same and make the appropriate changes in the Security class itself.
| * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||
| * graphic logo is a trademark of OpenMRS Inc. | ||
| */ | ||
| package org.openmrs.api.util; |
There was a problem hiding this comment.
This doesn't seem like the place we store similar classes at all does it?
| * <strong>Should</strong> match strings hashed with sha1 algorithm | ||
| * <strong>Should</strong> match strings hashed with sha512 algorithm and 128 characters salt | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
These definitely shouldn't be deprecated. That's not part of the implementation requested in the ticket (or, at least, it shouldn't be).
There was a problem hiding this comment.
Security is our class for doing security-related things.
There was a problem hiding this comment.
@ibacher Thanks for the review. I have fixed the proposed chnages.
ad27032 to
57f3219
Compare
|
@dkayiwa, @mseaton, @rkorytkowski kindly help me review this PR at your convenient time please! |
| public static String[] encodePassword(String rawPassword) { | ||
| String encoded = getPasswordEncoder().encode(rawPassword); | ||
| String[] parts = encoded.split(":", 2); | ||
| return new String[] { parts[0], parts.length > 1 ? parts[1] : "" }; | ||
| } |
There was a problem hiding this comment.
Right now nothing calls this method, and nothing calls the passwordEncoder bean, getPasswordEncoder(), or either encoder class. The real hashing still runs through Security.encodeString + getRandomToken directly in HibernateUserDAO, and verification still calls Security.hashMatches directly in LoginCredential.checkPassword and HibernateContextDAO.authenticate. So if this merges as-is, the Spring PasswordEncoder is created but never exercised: password hashing is unchanged, but it is not going through Spring's interface, which is what TRUNK-6662 is asking for, and spring-security-crypto ships unused.
There is also a format mismatch behind this: the encoder works with a combined hash:salt string, but OpenMRS stores the hash and salt in two separate columns, so encode/matches don't line up with the existing storage without the split-and-rejoin that encodePassword is doing here.
To actually meet the goal the live paths need to call through the encoder. Per @ibacher's earlier suggestion, that probably means Security (and/or the DAO) delegating to the injected PasswordEncoder instead of calling encodeString/getRandomToken directly, with a test that sets a password and then authenticates it so the round trip is shown to go through the encoder. If the plan is to add the API now and wire it later, I would hold these classes and the dependency until that wiring lands in the same change rather than merge code nothing reaches.
| String storedPassword = getHashedPassword(); | ||
| String storedSalt = getSalt(); | ||
| return storedPassword != null && Security.hashMatches(storedPassword, pw + (storedSalt != null ? storedSalt : "")); |
There was a problem hiding this comment.
Is this null-handling meant to be part of TRUNK-6662? It is the one change in the PR that actually runs at login time, and it shifts behavior in two ways: a null stored hash now returns false instead of throwing password.cannot.be.null, and a null salt now hashes pw instead of pw + "null" (the old pw + getSalt() appended the literal string "null"). I don't think it breaks existing logins, since the DAO always sets a real salt when a password is stored, so no stored hash was ever computed over pw + "null". Both look like reasonable hardening, but they are untested and tangential to the encoder work. Could we pull them into their own change, or if they stay, add a small test covering the null-hash and null-salt cases?
There was a problem hiding this comment.
Thanks @dkayiwa for the review let me fix it right away.
There was a problem hiding this comment.
Now that this moved into Security.checkPassword, the two differences from the base branch are still here: a null or blank salt hashes pw instead of pw + "null", and a null stored hash returns false instead of throwing password.cannot.be.null. Both look fine to keep (the null-salt case is arguably more correct for an old saltless row), so this does not block. The loose end from this thread is that neither null path has its own test. Could you add a small one covering checkPassword with a null or blank salt, and with a null stored hash?
There was a problem hiding this comment.
These don't actually exercise Security.checkPassword yet. Both new tests build a LegacyOpenmrsPasswordEncoder and call matches(...) with an already-joined string ("null:" and hash + ":"), so the null construction inside Security.checkPassword (storedHash + ":" + (storedSalt != null ? storedSalt : "")) never runs. If that line regressed, both tests would stay green, and the @see Security#checkPassword on each points at a method the test never calls.
The reason it's awkward to hit directly is that Security.checkPassword resolves the encoder through Context.getRegisteredComponent, which a plain SecurityTest can't do (that's also why these two pass without a Spring context). I'd move them into a context sensitive test and call Security.checkPassword("anyPassword", null, "") and Security.checkPassword("password", hash, null) so they cover the null hash and null salt paths through the real method. If you'd rather keep exercising the encoder from SecurityTest, then renaming them and pointing @see at LegacyOpenmrsPasswordEncoder#matches would at least stop them claiming checkPassword coverage they don't have. My preference is the context sensitive version.
There was a problem hiding this comment.
@dkayiwa Kindly I have pushed some changes feel free to review my PR at your convenient time please!
335c6bc to
c566e9c
Compare
|
@dkayiwa do you mind looking at my changes again? |
dkayiwa
left a comment
dkayiwa
left a comment
There was a problem hiding this comment.
Reading this against TRUNK-6662, which asks for Spring's PasswordEncoder to be the single entry point for all password operations (encoding and matching), I think it needs one more change before it meets that goal. There is still a password matching path that bypasses the encoder.
UpdateFilter.authenticateAsSuperUser (called at line 157, defined at line 318) verifies the login by calling Security.hashMatches(storedPassword, password + salt) directly, at line 340 and again in the pre-1.6 fallback at line 390. This is the superuser gate for the database upgrade wizard, so it is a real password match, and right now it does not go through the encoder. As long as it stands, the encoder is not the single entry point the ticket requires. The concrete cost is that the point of this abstraction is to let us change password handling in one place, and this login would not follow any such change, so the upgrade-wizard superuser login would drift onto a different code path from every other login in the system.
It is not a simple swap to Security.checkPassword, which is worth knowing before attempting it. UpdateFilter runs on a raw JDBC connection (DatabaseUpdater.getConnection()) without a started Context, while Security.checkPassword reaches the encoder through Context.getRegisteredComponent, which needs the context to be up. So this path needs a way to reach the encoder without a started context, for example holding a direct encoder instance rather than a registered-component lookup. That same limitation applies to the new encode and check methods in general: resolving the encoder through getRegisteredComponent means they cannot serve any password operation that runs before the context starts.
Could you route authenticateAsSuperUser through the encoder in a context-safe way, so all matching goes through one place? UpdateFilterTest already covers this method, so the change stays testable. If instead the team intends this path as a later step, let's note that on TRUNK-6662 so the scope is explicit, since as written the ticket reads as covering it.
|
@dkayiwa Thanks for the review. I have fixed the proposed changes. |
|
Thanks, that resolves it. authenticateAsSuperUser now matches through Security.checkPassword at both sites, so all password verification goes through the encoder, and the getPasswordEncoder fallback handles the case where the update filter runs before the context is up. I checked the head: UpdateFilterTest and ContextDAOTest both pass, so the upgrade-wizard superuser login and the normal login still authenticate. One optional thing, not a blocker. The fallback catches Exception broadly and returns the legacy encoder silently. That is fine today because the legacy encoder is the only one, so the fallback hands back the same encoder the bean would create. Once this is used to plug in a different encoder, a broad silent catch could mask a genuinely misconfigured bean and quietly fall back to legacy hashing with nothing in the logs. Narrowing the catch to the context-not-available case, or logging at debug when it falls back, would keep that from hiding later. |
|
@claude review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * @param logLevel the comma-separated log level string | ||
| * @param applier callback invoked with (className, level) for each parsed entry | ||
| */ | ||
| static void parseLogLevels(String logLevel, BiConsumer<String, String> applier) { |
There was a problem hiding this comment.
Optional: the last few commits ("Eliminate duplicated code across password and logging utilities", "Eliminate code duplication", "Code cleaning") pull parseLogLevels, applyLogLevelToConfiguration and resolveLogLevelProperty out of OpenmrsConfigurationFactory/OpenmrsLoggingUtil and touch ModuleUtil, Listener, ObsServiceImpl and HibernateContextDAO too, none of which relate to TRUNK-6662's password-encoder goal. I compared the extracted logic against what it replaced and it looks behavior-preserving, but none of the three new methods here have their own test, and this is already a large, heavily-reviewed PR (a maintainer flagged unrelated whitespace churn elsewhere in it earlier). Worth splitting this cleanup into its own PR so the password-encoder change stays reviewable on its own.
|
@claude review |
| } | ||
| } | ||
| } | ||
| return Boolean.FALSE; |
There was a problem hiding this comment.
Nit, not blocking: authenticateFromStatement's doc says it returns null when no row is found, and both call sites gate on if (result != null) before returning, but the method's only paths are return Security.checkPassword(...) && isSuperUser(...) or the trailing return Boolean.FALSE;, so it can never actually return null. That makes the null check at both call sites dead code and the javadoc inaccurate. I traced both call sites and the net behavior still matches the base branch (falling through used to reach the method's final return false; either way), so this isn't a functional bug, but in an authentication code path a reader could reasonably infer from the signature/doc that null is reachable and go looking for it. Worth changing the trailing return to false and dropping the dead null checks, or fixing the doc if null actually is meant to be reachable somewhere I'm missing.
|
@claude review |
1 similar comment
|
@claude review |
1aeb22a to
fe9b4b2
Compare
|
@dkayiwa Kindly Feel free to review my PR please! I supposed I have fixed the mess. |
| <dependency> | ||
| <groupId>org.springframework.security</groupId> | ||
| <artifactId>spring-security-crypto</artifactId> | ||
| <version>5.8.16</version> |
There was a problem hiding this comment.
This explicit version makes the dependencyManagement entry this PR adds to the root pom inert for the api module, since a direct version always wins. A future upgrade that bumps only the root pom would silently leave api on 5.8.16. The sibling dependencies here all inherit their versions from the root, and I confirmed the build still resolves 5.8.16 with this line removed, so it can just go:
| <version>5.8.16</version> |
| * @return String[] where [0] is the hash and [1] is the salt (empty string if absent) | ||
| * @since 2.8.8 | ||
| */ | ||
| public static String[] parseEncodedPassword(String encodedPassword) { |
There was a problem hiding this comment.
Does parseEncodedPassword need to be public? Its only callers are this class and LegacyOpenmrsPasswordEncoder.matches, all in org.openmrs.util, so package private works today. Once it ships in 2.8.8 as public with an @SInCE tag, the hash:salt packing becomes a contract modules can start depending on, which is exactly the seam TRUNK-6672 is supposed to redesign. Same for LegacyOpenmrsPasswordEncoder.encodeWithSalt, which only Security calls. I would make both package private now and widen later if a real caller appears (encodePasswordWithSalt does have to stay public, since HibernateUserDAO lives in another package). Not blocking merge.
|
@claude review |
|
@jwnasambu isn't it supposed to run again when you commit? |
|
@dkayiwa That is what I thought but its the reverse. |
|
@jwnasambu SonarCloud Code Analysis |
|
@dkayiwa Kindly how did you trigger it? At one point I had to push an empty commit which work but this time I didn't because I wished to learn something new. I tried using the Action button but I didn't have permission. |
|
@jwnasambu i did not trigger it at all. I just found when it had run. |
3 participants
Description of what I changed
I integrated Spring Security's
PasswordEncoderinterfaces to manage password hashing within the authentication flow and ensure that unchanged password validation and hashing mechanisms remain functional using Spring's native interfaces.Issue I worked on
https://openmrs.atlassian.net/browse/TRUNK-6662
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
mvn 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