Trunk-6663: Implement argon 2 id password encoder with configurable work factors#6301
Conversation
There was a problem hiding this comment.
Thanks, @EDSONZ-WASSWA, for the progress! Kindly note that we're pushing changes to the 2.8.x branch, not the master branch. Please follow these steps to change the base branch of your pull request on github please.
Open your pull request on GitHub.
Near the top of the PR, click Edit.
You will see something like base: master and compare: your-branch.
Change the base dropdown from master to 2.8.x.
Review any changes GitHub shows, then click Save or Update pull request.
…coder-with-configurable-work-factors
|
@claude review |
|
@dkayiwa please have a look at this PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…onfigurable-work-factors' of https://github.com/EDSONZ-WASSWA/openmrs-core into TRUNK-6663-implement-argon-2-id-password-encoder-with-configurable-work-factors
|
@claude review |
|
This needs fixing before merge — it's the current, reproducible CI failure on this exact commit (
Switching all four to |
There was a problem hiding this comment.
Kindly on this file api/src/test/java/org/openmrs/util/SecurityTest.java all the 3 new test methods use setAccessible(true) to manipulate static fields and ServiceContext. These tests are fragile any internal refactor breaks them. Consider adding a package private static void resetEncoder() method in Security.java specifically for testing, or use a test-specific subclass/factory pattern.
…coder-with-configurable-work-factors
|
@claude review |
…coder-with-configurable-work-factors
|
2 participants
Summary
This PR implements Argon2id as the default password encoder for OpenMRS while preserving backward compatibility with existing password hashes.
Argon2 work factors (memory, parallelism, iterations, hash length, and salt length) are configurable through Global Properties with documented defaults. The encoder automatically refreshes when configuration changes, allowing updated settings to take effect without requiring a server restart.
The implementation validates all configurable work factor values, ensuring they are positive and falling back to safe defaults with warning logs when invalid values are configured. Existing SHA-512 and SHA-1 password hashes continue to verify correctly, while newly encoded passwords use Argon2id.
To preserve existing deterministic hashing behaviour outside password storage, a dedicated
Security.encodeStringSHA512()helper was introduced. Existing consumers such as activation keys and secret-answer hashing continue using SHA-512, preventing regressions in features that rely on deterministic hashes while allowing password hashing to transition to Argon2id.The configuration-loading logic was also updated to safely fall back to the default Argon2 parameters when the application service layer is not yet available (for example during startup or database upgrades), while automatically using Global Property values once the service layer has been initialized.
Changes
spring-security-cryptoandargon2-jvmdependencies.Security.encodeString().Security.hashMatches()to verify Argon2 hashes while maintaining compatibility with legacy hashes.Security.encodeStringSHA512()for deterministic hashing use cases.Backward Compatibility
Issue I worked on
https://openmrs.atlassian.net/issues?filter=-1&selectedIssue=TRUNK-6663
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