TRUNK-6693: Upgrade secrets to modern password encoding#6345
Conversation
| /** | ||
| * Global property name for Argon2 memory cost in KB | ||
| */ | ||
| public static final String GP_ARGON2_MEMORY = "security.argon2.memory"; | ||
|
|
||
| /** | ||
| * Global property name for Argon2 parallelism factor | ||
| */ | ||
| public static final String GP_ARGON2_PARALLELISM = "security.argon2.parallelism"; | ||
|
|
||
| /** | ||
| * Global property name for Argon2 iterations | ||
| */ | ||
| public static final String GP_ARGON2_ITERATIONS = "security.argon2.iterations"; | ||
|
|
||
| /** | ||
| * Global property name for Argon2 hash length in bytes | ||
| */ | ||
| public static final String GP_ARGON2_HASH_LENGTH = "security.argon2.hashLength"; | ||
|
|
||
| /** | ||
| * Global property name for Argon2 salt length in bytes | ||
| */ | ||
| public static final String GP_ARGON2_SALT_LENGTH = "security.argon2.saltLength"; |
There was a problem hiding this comment.
We shouldn't make these global properties, I think. Rather, we should prefer to get these from the runtime properties (which is where we traditionally store the more security-sensitive stuff).
| String token = RandomStringUtils.randomAlphanumeric(20); | ||
| long time = System.currentTimeMillis() + getValidTime(); | ||
| String hashedKey = Security.encodeString(token); | ||
| String hashedKey = Security.encodeStringSHA512(token); |
| String answer = rs.getString("secret_answer"); | ||
| String salt = rs.getString("salt"); | ||
| String encryptedAnswer = Security.encodeString(answer.toLowerCase() + salt); | ||
| String encryptedAnswer = Security.encodeStringSHA512(answer.toLowerCase() + salt); |
…into TRUNK-6693
|
@claude review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return hPrime(longsToBytes(finalBlock, 1024), desiredLength); | ||
| } | ||
|
|
||
| private int getReferenceIndex(int pass, int slice, int lane, int index, int segmentLength, int parallelism, |
There was a problem hiding this comment.
This isn't RFC 9106-conformant Argon2id, which matters because both passwords and, with this PR, secret answers will depend on it for their security guarantees.
Two concrete deviations from the spec:
getReferenceIndexpicks the reference block with a plain double-modulo of an XOR ofpass/lane/index. The spec instead derives J1/J2 from the previous block's first 64-bit word and maps J1 into the reference area withz = W - 1 - ((W * (J1*J1 >> 32)) >> 32)(RFC 9106 §3.4.1.3) — a deliberately non-uniform mapping that is part of what gives Argon2 its resistance to time-memory tradeoff attacks. A uniform modulo here changes the memory-access pattern the whole algorithm is built around.f()only runsapplyCompressionover each 16-word row once. The real Argon2 block function G applies the mixing permutation to the 8 rows and then to the 8 columns of the resulting 8x16 matrix (RFC 9106 §3.6). The column pass is missing entirely here.
If merged as-is, every Argon2id hash this project produces is a proprietary, non-standard construction dressed up with the $argon2id$ tag — so it will silently fail to interoperate with any real Argon2id verifier, and there's no way to know whether it's still as memory-hard/attack-resistant as the RFC algorithm, since that's exactly the property these two omissions affect. SecurityTest/UserDAOTest only round-trip hashes against this same code, so none of this is caught — nothing here is checked against a published Argon2id test vector.
Rolling a from-scratch Argon2/BLAKE2b implementation for a security-critical path is risky even when byte-for-byte correct; given the two verified deviations above, I don't think this should replace the deterministic SHA-512 hashing without either fixing it to match the RFC exactly and validating against known test vectors, or using a vetted library (e.g. Bouncy Castle's Argon2BytesGenerator, or Spring Security's Argon2PasswordEncoder if the dependency is acceptable).
| } | ||
|
|
||
| @Test | ||
| public void setUserActivationKey_shouldStillWorkAfterSecretAnswerMigration() { |
There was a problem hiding this comment.
This test doesn't exercise what its name and assertion claim. dao.setUserActivationKey(lc) (HibernateUserDAO.java:769) just calls session.merge(credentials) — it never hashes anything. The test manually assigns the literal string "$argon2id$test-activation-key-12345" to lc.setActivationKey(...) and then asserts the same string comes back after a merge, so it will pass regardless of what hashing algorithm activation keys actually use.
Worse, the assertion is factually backwards: activation keys are hashed with Security.encodeStringSHA512(...) (UserServiceImpl.setUserActivationKey, and looked up the same way in HibernateUserDAO.getLoginCredentialByActivationKey), specifically because Argon2's random per-call salt makes it useless for a deterministic lookup key. A reader relying on this test's name ("activation key should use deterministic Argon2id") would draw the wrong conclusion about how activation keys work.
Worth replacing with a test that actually calls UserServiceImpl.setUserActivationKey(User) (or getLoginCredentialByActivationKey) and asserts the stored/looked-up key is SHA-512, not Argon2id.
| if (matches && !answerOnRecord.startsWith("$argon2id$")) { | ||
| String rehashedAnswer = Security.encodeString(answer.toLowerCase() + credentials.getSalt()); | ||
| credentials.setSecretAnswer(rehashedAnswer); | ||
| credentials.setDateChanged(new Date()); |
There was a problem hiding this comment.
Was it intended for a plain secret-answer check (e.g. during a forgot-password flow) to update DateChanged/ChangedBy on the credential as a side effect of the lazy rehash? Those fields normally reflect an explicit credential change made by the user, and changeQuestionAnswer already sets them when the answer is actually changed — here they'll also get bumped just because someone successfully answered their security question, which could be surprising to anything that audits DateChanged as "the user changed their credentials on this date".
|
2 participants
Description of what I changed
I extended the new Argon2id password encoding mechanism to support secret answers, which are human-controlled, low-entropy secrets that benefit from the same secure hashing approach as passwords. Activation keys remain on SHA-512 as they are short-lived and randomly generated tokens requiring deterministic lookup.
Issue I worked on
https://openmrs.atlassian.net/browse/TRUNK-6693
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