Add support for PBKDF2 for password hashing & add support for configuring BCrypt and PBKDF2 #4524

dancristiancecoi · 2024-07-04T10:12:46Z

Description

This change adds support for PBKDF2 as an alternative, FIPS compliant, password hashing algorithm.
Furthermore, it adds support for configuring the following parameters for these hashing algorithms:

BCrypt:

Rounds (defaults to 12)
Minor (defaults to Y)

PBKDF2:

Iterations (defaults to 600000)
Length (defaults to 256)
Function (defaults to SHA256)

Additionally, the hash.sh script has been modified to support the new algorithm and configuration.

usage: hash.sh [-h] [-p <password>] [-env <Environment variable name>] [-a
       <hashing algorithm>] [-r <rounds (BCrypt)>] [-min <minor (BCrypt)>]
       [-l <length (PBKDF2)>] [-f <function (PBDKF2)>] [-i <iterations
       (PBKDF2)>]
 -h,--help                               Display the help information
 -p,--password <password>                Cleartext password to hash
 -env <Environment variable name>        Environment variable name to read
                                         password from
 -a,--algorithm <hashing algorithm>      Algorithm to use for password
                                         hashing. Valid values are: BCrypt
                                         | PBKDF2. Default: BCrypt
 -r,--rounds <rounds (BCrypt)>           Number of rounds to use in
                                         logarithmic form. Valid values
                                         are: 4 to 31. Default: 12
 -min,--minor <minor (BCrypt)>           Version of BCrypt algorithm to
                                         use. Valid values are: A | B | Y.
                                         Default: Y
 -l,--length <length (PBKDF2)>           Desired length of the final
                                         derived key. Default: 256
 -f,--function <function (PBDKF2)>       Pseudo-random function applied to
                                         the password. Valid values are
                                         SHA1 | SHA224 | SHA256 | SHA384 |
                                         SHA512. Default: SHA256
 -i,--iterations <iterations (PBKDF2)>   Number of times the pseudo-random
                                         function is applied to the
                                         password. Default: 600000

install_demo_configuration.sh was left unchanged and currently only creates demo configurations with BCrypt hashes.

Issues Resolved

#4590

Related issues:

#4381
#3420

Testing

Added unit tests, integration tests and done manual tests.

Deploy without any hashing configuration specified:

curl -k https://localhost:9200 -u admin:$PASSWORD
{
  "name" : "SAS-1B16WV3",
  "cluster_name" : "opensearch",
  "cluster_uuid" : "zeB4OjWjRACHFQikYd34Fg",
  "version" : {
    "distribution" : "opensearch",
    "number" : "3.0.0-SNAPSHOT",
    "build_type" : "tar",
    "build_hash" : "58d1164f74e921a26b7a73b6185b38cc87bbc7a9",
    "build_date" : "2024-07-04T14:13:09.807311713Z",
    "build_snapshot" : true,
    "lucene_version" : "9.12.0",
    "minimum_wire_compatibility_version" : "2.16.0",
    "minimum_index_compatibility_version" : "2.0.0"
  },
  "tagline" : "The OpenSearch Project: https://opensearch.org/"
}


curl -k https://localhost:9200 -u admin:$WRONG_PASSWORD
Unauthorized

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"}}' "value2"}}'
{"status":"CREATED","message":"'dancecoi' created."}

curl -k -s "https://localhost:9200/_cat/indices?v" -u dancecoi:strongpassword1996!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.07.04 NeRiqXzSSIKHxWnhLBOmaw   1   1          3            0     44.4kb         44.4kb
green  open   .opendistro_security         pRLb5T_8SyObPEUWlu7wug   1   0         10            0     89.6kb         89.6kb

curl -k -s "https://localhost:9200/_cat/indices?v" -u dancecoi:wrong_password
Unauthorized


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"}}' "value2"}}'
{"status":"CREATED","message":"'dancecoihash' created."}

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.07.04 NeRiqXzSSIKHxWnhLBOmaw   1   1          5            0     74.4kb         74.4kb
green  open   .opendistro_security         pRLb5T_8SyObPEUWlu7wug   1   0         10            1    115.6kb        115.6kb

Deploy with PBKDF2:

opensearch.yml config:

.....
plugins.security.password.hashing.algorithm: pbkdf2
plugins.security.password.hashing.pbkdf2.iterations: 650000
plugins.security.password.hashing.pbkdf2.length: 512
plugins.security.password.hashing.pbkdf2.function: SHA512
.....

internal_users.yml config:

admin:
  hash: "$5$2576980377600512$CobxDePVXnGapo+pU3xo7hCZElhfgsnxtRh+Tox4nsgEP/3/McAWatRQz74eczIPYtbMCZbDFc4Bpd2GUZ56aw==$9aG6wcMv2ztLddg4UJN21dPtVzK2AihllX5p5w1OSY3C8je2ryFDsgXhb8Mg//YcClTTTK5Q6ZCtc1M0vQeLCA=="
  reserved: true
  backend_roles:
  - "admin"
  description: "Demo admin user"
....

Testing:

curl -k https://localhost:9200 -u admin:$PASSWORD
{
  "name" : "SAS-1B16WV3",
  "cluster_name" : "opensearch",
  "cluster_uuid" : "cnmnETP_R--rNPCV2csXcQ",
  "version" : {
    "distribution" : "opensearch",
    "number" : "3.0.0-SNAPSHOT",
    "build_type" : "tar",
    "build_hash" : "58d1164f74e921a26b7a73b6185b38cc87bbc7a9",
    "build_date" : "2024-07-04T14:13:09.807311713Z",
    "build_snapshot" : true,
    "lucene_version" : "9.12.0",
    "minimum_wire_compatibility_version" : "2.16.0",
    "minimum_index_compatibility_version" : "2.0.0"
  },
  "tagline" : "The OpenSearch Project: https://opensearch.org/"
}

curl -k https://localhost:9200 -u admin:$WRONG_PASSWORD
Unauthorized

./tools/hash.sh -a pbkdf2 -f SHA512 -l 512 -p testPassword
**************************************************************************
** 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
$5$2576980377600512$Z5Sk45FrRk1UR2HZWMrbBiWmd63qN37Vc2pwJQIEjs0103aG8OFCYgkZJG+FQ533XAZCw5me9ilxkjPwVtm1eQ==$3jQnsKteu/2Jq3aR/C0ME6CI58T4N/dHtGfimYWBdVFnRVSFZ91GkDC7KJgesG989zW/UpvHqLCtT+o1zkKROw==

curl -X PUT -k -s -H "Content-Type: application/json" "https://localhost:9200/_opendistro/_security/api/internalusers/dancecoihash" -u admin:$PASSWORD -d $'{"hash": "$5$2576980377600512$Z5Sk45FrRk1UR2HZWMrbBiWmd63qN37Vc2pwJQIEjs0103aG8OFCYgkZJG+FQ533XAZCw5me9ilxkjPwVtm1eQ==$3jQnsKteu/2Jq3aR/C0ME6CI58T4N/dHtGfimYWBdVFnRVSFZ91GkDC7KJgesG989zW/UpvHqLCtT+o1zkKROw==","opendistro_security_roles": ["all_access"],"backend_roles": ["all_access"],"attributes": {"attribute1": "value1","attribute2": "value2"}}'
{"status":"CREATED","message":"'dancecoihash' created."}

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.07.04 pdViqtaxSx6fM4LYlxIypQ   1   1          1            0     13.9kb         13.9kb
green  open   .opendistro_security         o7CmWxd6S6GSOQw9W2enEA   1   0         10            0     90.6kb         90.6kb


curl -k -s "https://localhost:9200/_cat/indices?v" -u dancecoihash:wrongTestPassword
Unauthorized

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."}

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.07.04 pdViqtaxSx6fM4LYlxIypQ   1   1          5            0     75.3kb         75.3kb
green  open   .opendistro_security         o7CmWxd6S6GSOQw9W2enEA   1   0         10            0     93.9kb         93.9kb

curl -k -s "https://localhost:9200/_cat/indices?v" -u dancecoi:wrongpassword
Unauthorized

Documentation

This requires changes to the OpenSearch documentation.

[DOC] Add documentation for configuring the password hashing algorithm documentation-website#7669

Check List

New functionality includes testing
New functionality has been documented
New Roles/Permissions have a corresponding security dashboards plugin PR
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.

…and verification This change introduces support for PBKDF2 as an alternative password hashing algorithm to BCrypt. Furthermore, it adds support for configuring the parameters of BCrypt (rounds, minor) & PBKDF2 (iterations, length, function). Signed-off-by: Dan Cecoi <[email protected]>

… fix for integration tests failures Signed-off-by: Dan Cecoi <[email protected]>

Signed-off-by: Dan Cecoi <[email protected]>

…recommendations; refactored hash.sh help messages; modified an exception message Signed-off-by: Dan Cecoi <[email protected]>

Signed-off-by: Dan Cecoi <[email protected]>

…e Hasher script Signed-off-by: Dan Cecoi <[email protected]>

dancristiancecoi · 2024-07-04T10:30:12Z

Tests are failing due to the switch to JDK21 and SecurityManager no longer being supported.

I can try and add the @SuppressWarnings("removal") annotation but any advice on how to proceed will be appreciated!

Edit: added the SupressWarnings annotation.

…is used to fix compile errors with JDK21 Signed-off-by: Dan Cecoi <[email protected]>

cwperks · 2024-07-05T13:50:18Z

@dancristiancecoi some quick comments from the PR description. I think it would be helpful to expand on the help text for the hashing script to make it read more like a man page.

Without passing any params will it default to the same BCrypt hash as it does today?

For the default values I think we should include those in the help text of the hash script as well. For any option with multiple choices (like --minor) I think the help text should include the list of options (if its a small amount of options) or a reference to where you can find out more about the possible options.

src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java

src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java

dancristiancecoi · 2024-07-05T14:07:49Z

@dancristiancecoi some quick comments from the PR description. I think it would be helpful to expand on the help text for the hashing script to make it read more like a man page.

Without passing any params will it default to the same BCrypt hash as it does today?

For the default values I think we should include those in the help text of the hash script as well. For any option with multiple choices (like --minor) I think the help text should include the list of options (if its a small amount of options) or a reference to where you can find out more about the possible options.

Good points! I will document the default values in the Hasher script.

The script is backwards compatible, so if a user doesnt pass any algorithm related parameters it will default to the same BCrypt hash configuration

cwperks · 2024-07-05T14:09:31Z

Took a first pass and left a few comments. I'm curious to see the test coverage report when all CI checks pass. Thank you for adding extensive tests to accompany this change!

One thought crossed my mind while reviewing the code as well, I think it would be useful in a follow-up change to add an option to the install_demo_configuration script that can be used to install the demo configuration with FIPS compliance.

Signed-off-by: Dan Cecoi <[email protected]>

kumargu · 2024-07-08T08:09:08Z

1/ because PBKDF2 consumes higher CPU cycles (depending on iterations, which i see here is already very high) -- can a bad actor effect Opensearch cluster health by repeated password attempts?

2/ Have we benchmarked how many cpu cycles are consumed and if this will work with single core cloud instances?

dancristiancecoi · 2024-07-08T09:26:01Z

1/ because PBKDF2 consumes higher CPU cycles (depending on iterations, which i see here is already very high) -- can a bad actor effect Opensearch cluster health by repeated password attempts?

2/ Have we benchmarked how many cpu cycles are consumed and if this will work with single core cloud instances?

In theory that is possible. However, the high CPU cycles and slow hash computation are done on purpose to make brute-force attacks much harder so its about finding a balance between the risk of DoS attacks and brute-force attacks. OpenSearch has rate limiting so that should help with the former.
OWASP recommends 600000 iterations or higher when using SHA256 so this is pretty much the lower bound for their PBKDF2 recommendations
No benchmarking has been done yet

src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java

src/main/java/org/opensearch/security/support/ConfigConstants.java

src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java

src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java

src/test/java/org/opensearch/security/hasher/BCryptPasswordHasherTests.java

src/test/java/org/opensearch/security/hasher/PBKDF2PasswordHasherTests.java

kumargu

Thanks @dancristiancecoi for the PR. left a few comments.

dancristiancecoi · 2024-07-09T10:34:30Z

Thanks @dancristiancecoi for the PR. left a few comments.

thanks a lot for the in-depth review!!

- Added help option and improved the hash.sh help message - Added java docs to PasswordHasher & AbstractPasswordHasher - Changed the default PBKDF2 length from 512 to 256 - Improved the validation exception messages - Moved null check logic to AbstractPasswordHasher Signed-off-by: Dan Cecoi <[email protected]>

codecov · 2024-07-09T15:03:25Z

Codecov Report

Attention: Patch coverage is 89.50000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 65.28%. Comparing base (0a91626) to head (7823689).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4524      +/-   ##
==========================================
+ Coverage   64.95%   65.28%   +0.33%     
==========================================
  Files         313      317       +4     
  Lines       22064    22273     +209     
  Branches     3562     3582      +20     
==========================================
+ Hits        14331    14541     +210     
+ Misses       5952     5940      -12     
- Partials     1781     1792      +11

Files	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	`84.59% <100.00%> (+0.28%)`	⬆️
...pensearch/security/auditlog/impl/AuditMessage.java	`76.38% <100.00%> (ø)`
...ensearch/security/hasher/BCryptPasswordHasher.java	`90.47% <100.00%> (+4.76%)`	⬆️
...g/opensearch/security/support/ConfigConstants.java	`95.23% <100.00%> (+0.23%)`	⬆️
...y/tools/democonfig/SecuritySettingsConfigurer.java	`76.71% <100.00%> (ø)`
...search/security/hasher/AbstractPasswordHasher.java	`91.66% <91.66%> (ø)`
...nsearch/security/hasher/PasswordHasherFactory.java	`90.90% <90.90%> (ø)`
...ensearch/security/hasher/PBKDF2PasswordHasher.java	`78.57% <78.57%> (ø)`
...ain/java/org/opensearch/security/tools/Hasher.java	`81.89% <89.00%> (+75.83%)`	⬆️

... and 10 files with indirect coverage changes

cwperks

@dancristiancecoi The changes look good to me. Can you also create a PR on the documentation-website to add documentation about the settings being introduced?

dancristiancecoi · 2024-07-10T14:08:36Z

@dancristiancecoi The changes look good to me. Can you also create a PR on the documentation-website to add documentation about the settings being introduced?

Thank you!

I've created it here: opensearch-project/documentation-website#7669

kumargu · 2024-07-10T15:29:57Z

LGTM. thanks for taking care of the comments.

src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java

src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java

src/main/java/org/opensearch/security/hasher/AbstractPasswordHasher.java

src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java

src/main/java/org/opensearch/security/hasher/PasswordHasherFactory.java

src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java

- removed unnecessary instantiation of PasswordHasher in tests - improved the exception message when password hashing alg is not supported Signed-off-by: Dan Cecoi <[email protected]>

dancristiancecoi · 2024-07-11T12:28:42Z

Any suggestions on where to document these new settings?

One option is to document them in Configuring OpenSearch -> Security Settings > Expert-level settings

We could also add a new chapter under Password settings in Security in OpenSearch -> Configuration -> Modifying the YAML files

Which option is the most suitable one if at all?

DarshitChanpura · 2024-07-11T15:32:00Z

Which option is the most suitable one if at all?

IMO we should add it under Configuring OpenSearch -> Security Settings > Expert-level settings since it lists all available security settings and also highlights that this is an expert-level setting.

DarshitChanpura

Thanks @dancristiancecoi for this contribution!

…ring BCrypt and PBKDF2 (#4524) Signed-off-by: Dan Cecoi <[email protected]> Co-authored-by: Dan Cecoi <[email protected]> (cherry picked from commit 8d29b11) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

dancristiancecoi · 2024-07-11T17:12:10Z

Thanks a lot for the reviews everyone!

Dan Cecoi added 7 commits July 2, 2024 10:39

Added missing tests, fixed checkstyle test failures & added potential…

79e666d

… fix for integration tests failures Signed-off-by: Dan Cecoi <[email protected]>

fixed checkstyle test failures

cf65688

Signed-off-by: Dan Cecoi <[email protected]>

Made changes to the default PBKDF2 rounds value to comply with OWASP …

7f2f013

…recommendations; refactored hash.sh help messages; modified an exception message Signed-off-by: Dan Cecoi <[email protected]>

Fixed the expected rounds value in Hasher tests

c61edff

Signed-off-by: Dan Cecoi <[email protected]>

fixed spotlessCheck failure

e58828a

Signed-off-by: Dan Cecoi <[email protected]>

Fixed some test comments & added a long arg option for password in th…

4041271

…e Hasher script Signed-off-by: Dan Cecoi <[email protected]>

dancristiancecoi requested review from cliu123, cwperks, DarshitChanpura, peternied, RyanL1997, stephen-crawford, reta and willyborankin as code owners July 4, 2024 10:12

dancristiancecoi marked this pull request as draft July 4, 2024 10:12

dancristiancecoi and others added 2 commits July 4, 2024 11:36

Merge branch 'opensearch-project:main' into passwordHashingStage2

0543c61

added @SuppressWarnings("removal") annotations where SecurityManager …

5b658de

…is used to fix compile errors with JDK21 Signed-off-by: Dan Cecoi <[email protected]>

dancristiancecoi marked this pull request as ready for review July 4, 2024 11:04

cwperks reviewed Jul 5, 2024

View reviewed changes

src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java Outdated Show resolved Hide resolved

cwperks reviewed Jul 5, 2024

View reviewed changes

src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java Show resolved Hide resolved

cwperks reviewed Jul 5, 2024

View reviewed changes

src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java Show resolved Hide resolved

Dan Cecoi added 2 commits July 5, 2024 17:32

fixed test failure & removed wildcard import

951abe1

Signed-off-by: Dan Cecoi <[email protected]>

Merge branch 'main' into passwordHashingStage2-v2

68e1115