Adds Argon2 support for password hashing #5441
Conversation
Signed-off-by: Aiden Lindsay <[email protected]>
Signed-off-by: Aiden Lindsay <[email protected]>
Signed-off-by: Aiden Lindsay <[email protected]>
Signed-off-by: Aiden Lindsay <[email protected]>
aidenlindsay
requested review from
cwperks,
DarshitChanpura,
derek-ho,
nibix,
peternied,
RyanL1997,
reta,
shikharj05 and
willyborankin
as code owners
Signed-off-by: Aiden Lindsay <[email protected]>
Signed-off-by: Aiden Lindsay <[email protected]>
Signed-off-by: Aiden Lindsay <[email protected]>
src/test/java/org/opensearch/security/hasher/Argon2PasswordHasherTests.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/hasher/Argon2PasswordHasher.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/hasher/Argon2PasswordHasher.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/hasher/Argon2PasswordHasher.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thank you @aidenlindsay ! The PR mostly looks good to me. Please address the JSM related comments and fix the integration tests.
| }); | ||
|
|
||
| Assert.assertFalse(message1.toString().contains("$argon2id$")); | ||
| Assert.assertTrue(message1.toString().contains("__HASH__")); |
There was a problem hiding this comment.
is __HASH__ a standard redaction string? If it's possible can we check exact instead of contains, that way we will not need assertion on line 401
There was a problem hiding this comment.
For the 401 assert I can be more specific with the hash data instead of just saying .contains, but I don't really know what you mean by removing it? HASH is not a standard but it does seem to be the convention used in the security plugin for other algorithms to indicate the hash has been redacted. I think that for its use case, checking that this appears at all is sufficient to ensure hashes are being redacted as intended, but I'm open to other suggestions
There was a problem hiding this comment.
I see. I meant to say whether we should compare with equalTo instead of contains.
There was a problem hiding this comment.
I think in practice audit messages are usually JSON objects or logs, and so they will contain more than just the string HASH, and therefore it would be unrealistic to check it is exactly equal, and more helpful to check that the term appears to show redaction
src/test/java/org/opensearch/security/hasher/Argon2PasswordHasherTests.java
Show resolved
Hide resolved
DarshitChanpura
changed the title
…, manually tested and works the same Signed-off-by: Aiden Lindsay <[email protected]>
…ame change can work for the other algs Signed-off-by: Aiden Lindsay <[email protected]>
…password hashing algorithms Signed-off-by: Aiden Lindsay <[email protected]>
…instead of hard coding the values Signed-off-by: Aiden Lindsay <[email protected]>
Signed-off-by: Aiden Lindsay <[email protected]>
…e is returned Signed-off-by: Aiden Lindsay <[email protected]>
|
will fix the test failures after lunch |
…rect type Signed-off-by: Aiden Lindsay <[email protected]>
…s, this change seems to create a new error which may be improved Signed-off-by: Aiden Lindsay <[email protected]>
… this to rerun Signed-off-by: Aiden Lindsay <[email protected]>
|
All bulk integration tests now pass locally except a single unrequired one that seems to be related to the clusters and not the code, could somebody run these integration tests again, think everything should be good for merge now. Will begin working on an update to the documentation :) |
Description
This change adds support for Argon2 as another alternative password hashing algorithm. Which builds on the added support for configuration of parameters, by building support for the following parameters:
Argon2:
This includes adding the Argon2PasswordHasher class as well as amending all current classes that use the hashing algorithms to support the new algorithm (PasswordHasherFactory.java, Hasher.java, test scripts, etc.)
Previously there was only support for BCrypt and PBKDF2 with BCrypt being the default, this allows the option to use another alternative algorithm in password hashing
Issues Resolved
#4592
Related Issues
#4590
#4524
Testing
Added both integration tests and unit tests to cover the new code as well as some manual testing.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.