search

opensearch-project / security

🔐 Secure your cluster with TLS, numerous authentication backends, data masking, audit logging as well as role-based access control on indices, documents, and fields
https://opensearch.org/docs/latest/security-plugin/index/
Apache License 2.0
181 stars 264 forks source link

Replace BouncyCastle's OpenBSDBCrypt use with password4j for password hashing and verification #4381

Closed dancristiancecoi closed 3 weeks ago

dancristiancecoi commented 1 month ago

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

(base) mtldce@SAS-1B16WV3:~$ curl -k https://localhost:9200 -u admin:$PASSWORD
{
  "name" : "SAS-1B16WV3",
  "cluster_name" : "opensearch",
  "cluster_uuid" : "XE_PodgCSZCaQQjHhWzlJA",
  "version" : {
    "distribution" : "opensearch",
    "number" : "3.0.0-SNAPSHOT",
    "build_type" : "tar",
    "build_hash" : "db5240e06dc08e7d7d03432595bb4d93b0e2e32d",
    "build_date" : "2024-05-30T10:46:41.778575385Z",
    "build_snapshot" : true,
    "lucene_version" : "9.11.0",
    "minimum_wire_compatibility_version" : "2.15.0",
    "minimum_index_compatibility_version" : "2.0.0"
  },
  "tagline" : "The OpenSearch Project: https://opensearch.org/"
}

(base) mtldce@SAS-1B16WV3:~$ curl -k https://localhost:9200 -u admin:$WRONG_PASSWORD
Unauthorized

(base) mtldce@SAS-1B16WV3:~$ curl -X PUT -k -s -H "Content-Type: application/json" "https://localhost:9200/_opendistro/_security/api/internalusers/dancecoi" -u admin:$PASSWORD -d $'{"password": "strongpassword1996!","opendistro_security_roles": ["all_access"],"backend_roles": ["all_access"],"attributes": {"attribute1": "value1","attribute2": "value2"}}'
{"status":"CREATED","message":"'dancecoi' created."}

(base) mtldce@SAS-1B16WV3:~$  curl -k -s "https://localhost:9200/_cat/indices?v" -u dancecoi:strongpassword1996!
health status index                        uuid                   pri rep docs.count docs.deleted store.size pri.store.size
yellow open   security-auditlog-2024.05.30 ihLqFsJQToS1Jih90b_2Dg   1   1          4            0     57.5kb         57.5kb
green  open   .opendistro_security         w9zyt-LAT3CcwEqsL1p2TQ   1   0         10            0     88.6kb         88.6kb

(base) mtldce@SAS-1B16WV3:~$ curl -k -s "https://localhost:9200/_cat/indices?v" -u dancecoi:wrong_password
Unauthorized(base)

(base) mtldce@SAS-1B16WV3:~$ curl -X PUT -k -s -H "Content-Type: application/json" "https://localhost:9200/_opendistro/_security/api/internalusers/dancecoihash" -u admin:$PASSWORD -d $'{"hash": "$2y$12$gdh2ecVBQmwpmcAeyReicuNtXyR6GMWSfXHxtcBBqFeFz2VQ8kDZe","opendistro_security_roles": ["all_access"],"backend_roles": ["all_access"],"attributes": {"attribute1": "value1","attribute2": "value2"}}'
{"status":"CREATED","message":"'dancecoihash' created."}

(base) mtldce@SAS-1B16WV3:~$ curl -k -s "https://localhost:9200/_cat/indices?v" -u dancecoihash:testPassword
health status index                        uuid                   pri rep docs.count docs.deleted store.size pri.store.size
yellow open   security-auditlog-2024.05.30 ihLqFsJQToS1Jih90b_2Dg   1   1          6            0     87.5kb         87.5kb
green  open   .opendistro_security         w9zyt-LAT3CcwEqsL1p2TQ   1   0         10            1    114.6kb        114.6kb

(base) mtldce@SAS-1B16WV3:~$ curl -k -s "https://localhost:9200/_cat/indices?v" -u dancecoihash:wrongTestPassword
Unauthorized(base)

(base) mtldce@SAS-1B16WV3:~/Projects/forks/OpenSearch/build/distribution/local/opensearch-3.0.0-SNAPSHOT/plugins/opensearch-security/tools$ ./hash.sh 
**************************************************************************
** This tool will be deprecated in the next major release of OpenSearch **
** https://github.com/opensearch-project/security/issues/1755           **
**************************************************************************
WARNING: nor OPENSEARCH_JAVA_HOME nor JAVA_HOME is set, will use /usr/bin/java
[Password:] 
$2y$12$2/lx4CpmaotLUtfa.vyiCOw3.1T6pgiPaql6l49pFF8m43ZAlJcRO

curl -X PUT -k -s -H "Content-Type: application/json" "https://localhost:9200/_opendistro/_security/api/internalusers/dancecoihash2" -u admin:$PASSWORD -d $'{"hash": "$2y$12$2/lx4CpmaotLUtfa.vyiCOw3.1T6pgiPaql6l49pFF8m43ZAlJcRO","opendistro_security_roles": ["all_access"],"backend_roles": ["all_access"],"attributes": {"attribute1": "value1","attribute2": "value2"}}' "value2"}}'
{"status":"CREATED","message":"'dancecoihash2' created."}

mtldce@SAS-1B16WV3:~$ curl -k -s "https://localhost:9200/_cat/indices?v" -u dancecoihash2:geforce_rtx_3070!
health status index                        uuid                   pri rep docs.count docs.deleted store.size pri.store.size
yellow open   security-auditlog-2024.05.30 ihLqFsJQToS1Jih90b_2Dg   1   1          8            0    117.6kb        117.6kb
green  open   .opendistro_security         w9zyt-LAT3CcwEqsL1p2TQ   1   0         10            2    127.8kb        127.8kb

(base) mtldce@SAS-1B16WV3:~$ curl -k -s "https://localhost:9200/_cat/indices?v" -u dancecoihash2:wrong_password
Unauthorized

Upgrade 2.11 -> 2.13 (with these changes back-ported)

Pre-upgrade:
curl -X PUT -k -s -H "Content-Type: application/json" "https://localhost:9200/_opendistro/_security/api/internalusers/dancecoisas" -u $ADMINUSER:$PASSWORD -d $'
{"password": "strongpassword1996!","opendistro_security_roles": ["all_access"],"backend_roles": ["all_access"],"attributes": {"attribute1": "value1","attribute2": "value2"}}'
{"status":"CREATED","message":"'dancecoisas' created."}

curl -k -s "https://localhost:9200/" -u $ADMINUSER:$PASSWORD
{
  "name" : "sas-opendistro-default-0",
  "cluster_name" : "sas-opendistro",
  "cluster_uuid" : "arjz9067TKSANhk48Xg6JQ",
  "version" : {
    "distribution" : "opensearch",
    "number" : "2.11.0",
    "build_type" : "tar",
    "build_hash" : "4dcad6dd1fd45b6bd91f041a041829c8687278fa",
    "build_date" : "2023-10-13T02:55:55.511945994Z",
    "build_snapshot" : false,
    "lucene_version" : "9.7.0",
    "minimum_wire_compatibility_version" : "7.10.0",
    "minimum_index_compatibility_version" : "7.0.0"
  },
  "tagline" : "The OpenSearch Project: https://opensearch.org/"
}
Post-upgrade:
curl -k -s "https://localhost:9200/" -u $ADMINUSER:$PASSWORD
{
  "name" : "sas-opendistro-default-0",
  "cluster_name" : "sas-opendistro",
  "cluster_uuid" : "arjz9067TKSANhk48Xg6JQ",
  "version" : {
    "distribution" : "opensearch",
    "number" : "2.13.0",
    "build_type" : "tar",
    "build_hash" : "7ec678d1b7c87d6e779fdef94e33623e1f1e2647",
    "build_date" : "2024-03-26T00:02:39.659767978Z",
    "build_snapshot" : false,
    "lucene_version" : "9.10.0",
    "minimum_wire_compatibility_version" : "7.10.0",
    "minimum_index_compatibility_version" : "7.0.0"
  },
  "tagline" : "The OpenSearch Project: https://opensearch.org/"
}

curl -k -s "https://localhost:9200/_cat/indices" -u dancecoisas:strongpassword1996!
Defaulted container "sas-opendistro" out of: sas-opendistro, sysctl (init), sas-certframe (init)
green  open .plugins-ml-config               A3U0wNFnS5WOrC2yUrGtKg 1 0   1 0   3.9kb   3.9kb
green  open .opensearch-observability        SPlW4oIsQ5yDO7qT5xuFrA 1 0   0 0    208b    208b
yellow open security-auditlog-2024.05.30     VLtGQpMrS1en6CyEMj-VMA 1 1   3 0    47kb    47kb
green  open .opensearch-sap-log-types-config 7rNwKfsWRImllcPVTkbgbA 1 0 452 0 101.6kb 101.6kb
green  open .opendistro_security             3q1xOXU_SZGg1ud34p_K_Q 1 0   9 0  24.4kb  24.4kb

curl -k -s "https://localhost:9200/_cat/indices" -u dancecoisas:wrongpassword

(base) curl -k -s "https://localhost:9200/_cat/indices" -u dancecoisas:wrongpassword
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.

dancristiancecoi commented 1 month ago

There are some failing integration tests that I will look into ASAP.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 88.67925% with 6 lines in your changes missing coverage. Please review.

Project coverage is 65.40%. Comparing base (a1e5db3) to head (8e3b9cc). Report is 11 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/opensearch-project/security/pull/4381/graphs/tree.svg?width=650&height=150&src=pr&token=rBpySfQXMt&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project)](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project) ```diff @@ 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 ``` | [Files](https://app.codecov.io/gh/opensearch-project/security/pull/4381?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project) | Coverage Δ | | |---|---|---| | [.../opensearch/security/OpenSearchSecurityPlugin.java](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2FOpenSearchSecurityPlugin.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS9PcGVuU2VhcmNoU2VjdXJpdHlQbHVnaW4uamF2YQ==) | `84.35% <100.00%> (+0.04%)` | :arrow_up: | | [...y/auth/internal/InternalAuthenticationBackend.java](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2Fauth%2Finternal%2FInternalAuthenticationBackend.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS9hdXRoL2ludGVybmFsL0ludGVybmFsQXV0aGVudGljYXRpb25CYWNrZW5kLmphdmE=) | `72.30% <100.00%> (+0.87%)` | :arrow_up: | | [...security/dlic/rest/api/InternalUsersApiAction.java](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2Fdlic%2Frest%2Fapi%2FInternalUsersApiAction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS9kbGljL3Jlc3QvYXBpL0ludGVybmFsVXNlcnNBcGlBY3Rpb24uamF2YQ==) | `81.57% <100.00%> (+0.16%)` | :arrow_up: | | [...security/dlic/rest/api/SecurityRestApiActions.java](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2Fdlic%2Frest%2Fapi%2FSecurityRestApiActions.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS9kbGljL3Jlc3QvYXBpL1NlY3VyaXR5UmVzdEFwaUFjdGlvbnMuamF2YQ==) | `80.00% <ø> (ø)` | | | [...g/opensearch/security/dlic/rest/support/Utils.java](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2Fdlic%2Frest%2Fsupport%2FUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS9kbGljL3Jlc3Qvc3VwcG9ydC9VdGlscy5qYXZh) | `60.52% <ø> (-2.89%)` | :arrow_down: | | [...ch/security/securityconf/DynamicConfigFactory.java](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2Fsecurityconf%2FDynamicConfigFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS9zZWN1cml0eWNvbmYvRHluYW1pY0NvbmZpZ0ZhY3RvcnkuamF2YQ==) | `55.62% <100.00%> (ø)` | | | [...ain/java/org/opensearch/security/tools/Hasher.java](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2Ftools%2FHasher.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS90b29scy9IYXNoZXIuamF2YQ==) | `6.06% <100.00%> (-10.16%)` | :arrow_down: | | [...y/tools/democonfig/SecuritySettingsConfigurer.java](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2Ftools%2Fdemoconfig%2FSecuritySettingsConfigurer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS90b29scy9kZW1vY29uZmlnL1NlY3VyaXR5U2V0dGluZ3NDb25maWd1cmVyLmphdmE=) | `76.71% <100.00%> (+0.32%)` | :arrow_up: | | [...earch/security/dlic/rest/api/AccountApiAction.java](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2Fdlic%2Frest%2Fapi%2FAccountApiAction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS9kbGljL3Jlc3QvYXBpL0FjY291bnRBcGlBY3Rpb24uamF2YQ==) | `60.56% <66.66%> (+0.56%)` | :arrow_up: | | [...java/org/opensearch/security/user/UserService.java](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2Fuser%2FUserService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS91c2VyL1VzZXJTZXJ2aWNlLmphdmE=) | `57.03% <80.00%> (+0.32%)` | :arrow_up: | | ... and [1 more](https://app.codecov.io/gh/opensearch-project/security/pull/4381?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project) | | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/opensearch-project/security/pull/4381/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project)
dancristiancecoi commented 1 month ago

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.

dancristiancecoi commented 1 month ago

@shikharj05

Thanks for the PR. Quick questions -

  1. How does this look like for end-users? Doesn't it break existing logins?
  1. No difference for end-users as the existing hashes still get verified!
  1. 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?

dancristiancecoi commented 4 weeks ago

@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 testRunAgainstMain.txt

Run 2:

testRunAgainstMain-run2.txt testRunAgainstMyChanges-run2.txt

To me it doesn't look like there is a significant variation either way in these bench-marking results.

cwperks commented 4 weeks ago

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.

dancristiancecoi commented 4 weeks ago

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.

Awesome, thanks @cwperks!

shikharj05 commented 3 weeks ago

@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 testRunAgainstMain.txt

Run 2:

testRunAgainstMain-run2.txt testRunAgainstMyChanges-run2.txt

To me it doesn't look like there is a significant variation either way in these bench-marking results.

Thanks, looked at the results, Run-1 shows some drop in throughput for index, however in run-2 it matches up. LGTM for now!

opensearch-trigger-bot[bot] commented 3 weeks ago

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

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 base branch is 2.x and the compare/head branch is backport/backport-4381-to-2.x.

willyborankin commented 3 weeks ago

@dancristiancecoi could you please prepare a manual backport?

dancristiancecoi commented 3 weeks ago

@dancristiancecoi could you please prepare a manual backport?

Sure! https://github.com/opensearch-project/security/pull/4428

dancristiancecoi commented 3 weeks ago

Thanks everyone for the reviews!

ruanyl commented 2 weeks ago

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.

Hi @dancristiancecoi I'm not able to start a new cluster with old internal_users.yml, is there documentations that I can take a look on fix this?

I got error:

Not yet initialized (you may need to run securityadmin)
dancristiancecoi commented 2 weeks ago

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

ruanyl commented 2 weeks ago

Hi @dancristiancecoi Thanks, I think I figured it out, just need to regenerate the password hash, then it works :) I was using a internal_users.yml that's generated before.