-
Notifications
You must be signed in to change notification settings - Fork 324
Adds Argon2 support for password hashing #5441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds Argon2 support for password hashing #5441
Conversation
Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @aidenlindsay ! The PR mostly looks good to me. Please address the JSM related comments and fix the integration tests.
src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java
Show resolved
Hide resolved
src/test/java/org/opensearch/security/hasher/Argon2PasswordHasherTests.java
Show resolved
Hide resolved
…, manually tested and works the same Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
…ame change can work for the other algs Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
…password hashing algorithms Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
…instead of hard coding the values Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
…e is returned Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
will fix the test failures after lunch |
…rect type Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me. I'll wait for CI to be green before my final review.
…s, this change seems to create a new error which may be improved Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
… this to rerun Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
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 :) |
Added the relevant documentation for this PR: |
Signed-off-by: Aiden Lindsay <87037572+aidenlindsay@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5441 +/- ##
==========================================
+ Coverage 72.41% 72.57% +0.16%
==========================================
Files 395 397 +2
Lines 24325 24580 +255
Branches 3714 3735 +21
==========================================
+ Hits 17614 17840 +226
- Misses 4882 4905 +23
- Partials 1829 1835 +6
🚀 New features to boost your workflow:
|
@DarshitChanpura @willyborankin @shikharj05 Can this get another maintainer to review? |
Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com> Signed-off-by: Aiden Lindsay <87037572+aidenlindsay@users.noreply.github.com> Co-authored-by: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>
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.