-
Notifications
You must be signed in to change notification settings - Fork 275
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
Replace BouncyCastle's OpenBSDBCrypt use with password4j for password hashing and verification #4381
Replace BouncyCastle's OpenBSDBCrypt use with password4j for password hashing and verification #4381
Conversation
…fication Signed-off-by: Dan Cecoi <Dan.Cecoi@sas.com>
Signed-off-by: Dan Cecoi <Dan.Cecoi@sas.com>
There are some failing integration tests that I will look into ASAP. |
…on the hash Signed-off-by: Dan Cecoi <Dan.Cecoi@sas.com>
…asswordHashingStage1-v2
Signed-off-by: Dan Cecoi <Dan.Cecoi@sas.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4381 +/- ##
==========================================
- Coverage 65.44% 65.40% -0.04%
==========================================
Files 310 311 +1
Lines 21992 22020 +28
Branches 3554 3557 +3
==========================================
+ Hits 14392 14402 +10
- Misses 5830 5843 +13
- Partials 1770 1775 +5
|
Signed-off-by: Dan Cecoi <Dan.Cecoi@sas.com>
…asswordHashingStage1-v2
Integration tests were failing due to pre-set hashes in various internal_users.yml files used for integration tests having a different configuration to the default one (BCrypt.A with 4 log rounds vs BCrypt.Y with 12 log rounds). I've adapted the code so it verifies the passwords with a BCrypt configuration based on the hash. |
src/main/java/org/opensearch/security/hasher/BCryptPasswordHasher.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/auth/internal/InternalAuthenticationBackend.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/dlic/rest/api/AccountApiActionConfigValidationsTest.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Dan Cecoi <Dan.Cecoi@sas.com>
Signed-off-by: Dan Cecoi <Dan.Cecoi@sas.com>
src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java
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.
Thanks for the PR. Quick questions -
- How does this look like for end-users? Doesn't it break existing logins?
- Any performance numbers we can get to understand the difference?
I do not have any performance numbers but I can run some if required. What will be the best way to do it? Should I use the opensearch-benchmark tool locally or is there a github action we can run? |
@shikharj05 I've ran the benchmarking tool with the percolator workload. It was against a one node local cluster running on my laptop so I am not sure how representative the results are :D. For comparison I also attached a benchmark run in similar conditions but against the latest code in main. testRunAgainstChanges.txt Run 2: testRunAgainstMain-run2.txt To me it doesn't look like there is a significant variation either way in these bench-marking results. |
This change LGTM! I am also looking at the benchmarking tool to look at ways to measure the impact on performance for the change in library. There is a performance test suite that's in Draft and not unavailable yet unfortunately. |
src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java
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.
@dancristiancecoi Changes look good to me! I don't have any reservations against merging this as long as there is some sort of benchmarking done once the #4282 is merged. Once everything is implemented we can ship it in 2.x line.
src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java
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.
LGTM
src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java
Show resolved
Hide resolved
Thanks, looked at the results, Run-1 shows some drop in throughput for |
20c524a
into
opensearch-project:main
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-4381-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 20c524ad994a9cc7d8757999f92f6d2fec6cb8ca
# Push it to GitHub
git push --set-upstream origin backport/backport-4381-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x Then, create a pull request where the |
@dancristiancecoi could you please prepare a manual backport? |
…d hashing and verification (opensearch-project#4381) Signed-off-by: Dan Cecoi <Dan.Cecoi@sas.com> Co-authored-by: Dan Cecoi <dan.cecoi@sas.com> (cherry picked from commit 20c524a)
Sure! #4428 |
Thanks everyone for the reviews! |
Hi @dancristiancecoi I'm not able to start a new cluster with old I got error:
|
Hi @ruanyl. Are there any other errors in the logs to pinpoint to why the Security plugin was not initialised? When you mention "old internal-users.yml file" is it similar to the ones used in the integration tests / demo config ? Anything that's unique about them? There isn't documentation for this change as it SHOULD have worked seamlessly but it's very possible we've missed something |
Hi @dancristiancecoi Thanks, I think I figured it out, just need to regenerate the password hash, then it works :) |
Description
This change removes the usages of Bouncy Castle's OpenBSDBCrypt from the code and replaces it with a FIPS compliant library that supports additional hashing algorithms like PBKDF2, Argon2 and SCrypt.
Furthermore it consolidates the password hashing and verification logic in one place.
This change will require a security review.
Issues Resolved
Related Issues
Testing
Various authentication attempts against a local 3.x deploy
Upgrade 2.11 -> 2.13 (with these changes back-ported)
Pre-upgrade:
Post-upgrade:
Extra:
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.