TRUNK-6664: Implement deterministic bootstrap password mechanism#6355
Conversation
- Added PBKDF2-based deterministic password generation - Added bootstrap password methods to UserService - Added global properties for system salt and iterations - Added unit tests (15/15 passing)
…p-password-mechanism
|
@ibacher , @dkayiwa , @jwnasambu kindly check on this PR. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| KeySpec spec = new PBEKeySpec( | ||
| input.toCharArray(), | ||
| PBKDF2_SALT, // Non-empty fixed salt | ||
| actualIterations, | ||
| PBKDF2_KEY_LENGTH | ||
| ); |
There was a problem hiding this comment.
intentionally a fixed salt was used because bootstrap passwords must be deterministic (same input → same output). The system pepper, stored in runtime.properties, provides the unpredictability per installation. This is a deliberate design trade-off.
|
@claude review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| try { | ||
| String expected = generateBootstrapPassword(user); | ||
| return expected.equals(password); |
There was a problem hiding this comment.
If this ships as-is, a bootstrap password never actually stops working. validateBootstrapPassword only recomputes generateBootstrapPassword(user) and compares strings here - it never consults isBootstrapPasswordExpired. So once forcePasswordChange is called (per its own Javadoc, that's meant to happen right after a successful bootstrap login), the same password keeps validating successfully forever, since it's a pure function of the user's UUID and the system salt with no mutable state involved. The "expired" flag only affects whatever caller checks isBootstrapPasswordExpired directly; it does nothing to stop re-authentication with the same bootstrap password through this method. Worth having this return false once isBootstrapPasswordExpired(user) is true, or otherwise tying expiry to something that actually blocks validation.
There was a problem hiding this comment.
fixed, validateBootstrapPassword now returns false if isBootstrapPasswordExpired(user) is true. thank you
| throw new APIException("bootstrap.system.salt.missing", (Object[]) null); | ||
| } | ||
|
|
||
| String input = uuid + salt; |
There was a problem hiding this comment.
Beyond the salt being a compile-time constant (already flagged by SonarCloud on this PR), the derivation itself concentrates every user's login credential behind one shared secret: input = uuid + salt with a single system-wide salt. If security.bootstrap.systemSalt is ever exposed - a DB backup, a support dump, or simply anyone holding the "Get Global Properties" privilege, since that's all getGlobalProperty requires - every current and future user's bootstrap password can be computed from nothing but their UUID, and UUIDs aren't treated as secret anywhere else in this codebase. There's also no way to revoke one compromised bootstrap password without regenerating the shared salt, which simultaneously invalidates every other account's bootstrap password at once. Combined with the missing expiry check on validateBootstrapPassword, a leaked salt plus a known UUID is a full account takeover with no revocation path short of a system-wide salt rotation. Is a single global secret really the intended trust model, or should this derive from something scoped per-user that can be rotated independently?
| * @see org.openmrs.api.UserService#generateBootstrapPassword(User) | ||
| */ | ||
| @Override | ||
| public String generateBootstrapPassword(User user) { |
There was a problem hiding this comment.
I couldn't find any caller of generateBootstrapPassword, validateBootstrapPassword, forcePasswordChange, or isBootstrapPasswordExpired anywhere outside this class, Security, and the new test - including DaoAuthenticationScheme / UsernamePasswordAuthenticationScheme, which is where I'd expect a bootstrap password to actually get checked during login. As merged, nothing in openmrs-core itself can authenticate a user with a bootstrap password. Is wiring this into the login flow planned as a follow-up PR, or are these methods meant to be called directly by downstream modules?
There was a problem hiding this comment.
Wiring the bootstrap password check into the authentication flow is planned as a follow-up ticket. This PR focuses on the backend service layer. We'll coordinate with the auth framework team and see how to integrate it.
…untime properties
|
"Addressed feedback: renamed 'system salt' → 'pepper' and moved to runtime properties. All 15 tests passing. Ready for review." |
|
There was a problem hiding this comment.
@praisekavuma4 could you please change your PR title to "TRUNK-6664" rather than "TRUNK-6666" which belongs to me
praisekavuma4
changed the title
4 participants
Summary
This PR implements a deterministic bootstrap password mechanism for OpenMRS, allowing administrators to generate secure, temporary passwords for user provisioning and controlled resets. The password is derived from the user's UUID and a system-wide salt using PBKDF2, ensuring the same user always gets the same password without storing it in the database.
What This PR Adds
Core PBKDF2 Logic (Security.java)
generateDeterministicHash(String input, int iterations) – PBKDF2-based derivation
getBootstrapSystemSalt() – Retrieves system salt from global properties
getBootstrapIterations() – Retrieves iteration count from global properties
generateBootstrapPassword(User user) – Generates deterministic password from UUID + salt
validateBootstrapPassword(User user, String password) – Validates a password against the user's bootstrap password
forcePasswordChange(User user) – Sets the forcePassword user property to force password change on next login
isBootstrapPasswordExpired(User user) – Checks if a bootstrap password has expired
Service Layer Integration (UserService.java / UserServiceImpl.java)
Added 5 new methods to the UserService interface:
generateBootstrapPassword(User user)
validateBootstrapPassword(User user, String password)
forcePasswordChange(User user)
isBootstrapPassword(User user, String password)
isBootstrapPasswordExpired(User user)
Global Properties (OpenmrsConstants.java)
security.bootstrap.systemSalt – System salt for password derivation (must be kept secret)
security.bootstrap.iterations – PBKDF2 iteration count (default: 600,000)
Unit Tests (BootstrapPasswordTest.java)
15 unit tests covering:
Deterministic hash generation
Bootstrap password generation and validation
Error handling (null user, missing UUID, missing salt)
Expiry detection via forcePassword property
All tests passing: Tests run: 15, Failures: 0, Errors: 0