Skip to content

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

Merged
merged 18 commits into from
Jul 2, 2025

Conversation

aidenlindsay
Copy link
Contributor

@aidenlindsay aidenlindsay commented Jun 25, 2025

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:

  • Memory (defaults to 64MiB/65536)
  • Iterations (defaults to 3)
  • Length (defaults to 32)
  • Parallelism (defaults to 1)
  • Type (defaults to Argon2ID)
  • Version (defaults to 19)

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

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • Commits are signed per the DCO using --signoff

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.

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>
Copy link
Member

@cwperks cwperks left a 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.

@DarshitChanpura DarshitChanpura changed the title Add support for Argon2 for password hashing (NEW + IMPROVED) Adds Argon2 support for password hashing Jun 25, 2025
…, 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>
@aidenlindsay
Copy link
Contributor Author

will fix the test failures after lunch

…rect type

Signed-off-by: Aiden Lindsay <aiden.o.lindsay@gmail.com>
Copy link
Member

@DarshitChanpura DarshitChanpura left a 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>
@aidenlindsay
Copy link
Contributor Author

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 :)

@aidenlindsay
Copy link
Contributor Author

Added the relevant documentation for this PR:

opensearch-project/documentation-website#10163

Signed-off-by: Aiden Lindsay <87037572+aidenlindsay@users.noreply.github.com>
Copy link

codecov bot commented Jun 30, 2025

Codecov Report

Attention: Patch coverage is 87.69231% with 16 lines in your changes missing coverage. Please review.

Project coverage is 72.57%. Comparing base (deeacf5) to head (07515fe).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...nsearch/security/hasher/PasswordHasherFactory.java 55.55% 6 Missing and 6 partials ⚠️
...ensearch/security/hasher/Argon2PasswordHasher.java 85.71% 2 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java 84.11% <100.00%> (+0.24%) ⬆️
...pensearch/security/auditlog/impl/AuditMessage.java 78.70% <100.00%> (ø)
...ensearch/security/hasher/BCryptPasswordHasher.java 100.00% <100.00%> (+19.04%) ⬆️
...ensearch/security/hasher/PBKDF2PasswordHasher.java 100.00% <100.00%> (+21.42%) ⬆️
...g/opensearch/security/support/ConfigConstants.java 95.23% <ø> (ø)
...ain/java/org/opensearch/security/tools/Hasher.java 87.50% <100.00%> (+5.60%) ⬆️
...ensearch/security/hasher/Argon2PasswordHasher.java 85.71% <85.71%> (ø)
...nsearch/security/hasher/PasswordHasherFactory.java 75.00% <55.55%> (-15.91%) ⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cwperks
Copy link
Member

cwperks commented Jul 1, 2025

@DarshitChanpura @willyborankin @shikharj05 Can this get another maintainer to review?

@cwperks cwperks merged commit 4ed5a07 into opensearch-project:main Jul 2, 2025
70 checks passed
markdboyd pushed a commit to cloud-gov/opensearch-security that referenced this pull request Jul 21, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants