[FEATURE] Add support for PBKDF2 for passwords hashing

[willyborankin]

What solution would you like?

Currently we support only bcrypt for passwords hashing which is resource consuming for slow CPUs.
It would be a good idea to give customers a possibility to select different famous hashing algorithms like: PBKDF2.

To archive that new properties should be added in the security plugin configuration:

• plugins.security.password.hashing.algorithm = ... - default is bcrypt, supported values are: pbkdf2.
• plugins.security.password.hashing.<algorithm>.<param_name> = ... - multiple parameters which are relevant to each algo

e.g.

plugins.security.password.hashing.algorithm='pbkdf2'
plugins.security.password.hashing.pbkdf2.iterations = 1000 
plugins.security.password.hashing.pbkdf2.hash_size_bytes = 128 
plugins.security.password.hashing.pbkdf2.salt_size_bytes = 12 

In case of PBKDF2 an additional security settings needs to be add:

[peternied]

@willyborankin Looks like this would this address the perf issues around PATCH on the user API [1], do I have that right?

• [1] [BUG] High load when updating internal users using PATCH #4008

[willyborankin]

@peternied, yes the main idea that users can try to turn hashing password algos. wdyt does it make sense?

[stephen-crawford]

[Triage] Hi @willyborankin, thanks for filing this issue. This change seems to have a clear direction forward and would be valuable in fixing some current performance issues.

[terryquigleysas]

There is some crossover with this request and #3420

[dancristiancecoi]

Hi, can I get assigned to this please?

[dancristiancecoi]

Hello!

I have some WIP changes for this feature. As I am somewhat unfamiliar with the code-base I'd like to discuss it before I proceed further in case I am taking a wrong approach.

The solution is based around the Password4J library. The main two considerations when choosing it were:

1. FIPS compliance
2. Built-in support of every algorithm that we want to support as part of this feature (BCrypt, PBKDF2, SCrypt, Argon2)

Password4j ticked both boxes while other solutions would have required pulling in multiple libraries for each algorithm & potential dependency on BouncyCastle.

In terms of configuration, this is what the current implementation supports:

BCrypt:

1. Rounds
2. Minor version

PBKDF2

1. Algorithm/Function (SHA-256 etc)
2. Iterations
3. Output Length

Argon2

1. Memory
2. Iterations
3. Parallelization
4. Output length
5. Type of the algorithm
6. Version of the algorithm

SCrypt

1. Work Factor
2. Resources
3. Parallelisation
4. Output length

If an administrator does not specify the default hashing algorithm it will default to BCrypt with a configuration that should make it compatible with the "legacy" hashes. On the other hand if they specify a different configuration for BCrypt or if they specify an entirely different algorithm they will have to rehash the passwords.

Here are the current changes: dancristiancecoi@2029526 ( I can raise a draft PR if it will make the process easier)

As I mentioned this is WIP so some things are not fully finished and will get refactored (and split into multiple commits when it is time to raise PR's). These changes do not currently cover redacting of the new hash formats in the AuditLog.

So my main questions:

1. Does this approach seem reasonable and am I missing anything ?
2. Will there be any issues bringing Password4j into this project?
3. Are we happy supporting all these configuration options? This will require quite an extensive documentation effort for all these config options + the equivalent command line arguments in the Hash.sh script

[peternied]

@dancristiancecoi Thanks for the thoughtful response and proposal. I'd recommend creating a pull request in draft mode for review, I think you'll get better specific feedback about the implementations details vs this issue.

Does this approach seem reasonable and am I missing anything ?

High level this seems reasonable. However, I would say as you transition into the design, working backward from what/how things will be configured might be very important. For example you shouldn't be able to define multiple hashing implementations at once, they should be mutually exclusive in an intuitive way.

Will there be any issues bringing Password4j into this project?

Let me invert the question, why do you trust password4j? By writing out why we use implementation vs other options can help guide the discussion. This feature will need to go through a security review [1] reachout to @kkhatua & @cwperks for more details.

Are we happy supporting all these configuration options? This will require quite an extensive documentation effort for all these config options + the equivalent command line arguments in the Hash.sh script

So long as there is good documentation and testing - I'm not dauted by a large number of options.

However, I recommend trying to simplify options where system admins only need to make an clear and informed choice. An example of this might be around output length - does it make sense to allow this option to be configured or could we use a sensible default? If we found that we needed to expose an option in the future that seems trivial whereas once we've shipped a config option we have to wait for a major release to remove it.

• [1] https://github.com/opensearch-project/.github/blob/main/RELEASING.md#security-reviews

[dancristiancecoi]

Thanks a lot for your thoughts & suggestions @peternied !

Let me invert the question, why do you trust password4j?

In my opinion these are the points in favor of Password4j:

• Open Source with an Apache License
• Actively maintained
• Supports all the algorithms we need
• Straightforward API
• Backwards compatible (it can verify hashes generated by the current BouncyCastle/OpenBSDBCrypt implementation)
• Supports updating of existing hashes (to a more secure configuration or to an entirely different algorithm).
• Relatively popular

Unfortunately I can't comment on the actual underlying implementation of each algorithm.

By writing out why we use implementation vs other options can help guide the discussion.

Outside of Password4j, these are the libraries that are generally used for each hashing algorithm:

BCrypt:

• BouncyCastle
• Spring Security
• patrickfav/bcrypt (based on jBCrypt)
• jBCrypt

SCrypt:

• BouncyCastle
• Spring Security
• lambdaworks/scrypt

Argon2:

• BouncyCastle
• Spring Security
• argon2-jvm

This is certainly not an exhaustive list but these are the libraries that came up during my research.

PBKDF2 is the only hashing algorithm out of these 4 that has native support in Java and as such does not require the use of a 3rd party library.

This feature will need to go through a security review

To facilitate the Security Review I plan to raise a smaller PR that simply replaces BouncyCastle's OpenBSDBCrypt with Password4J but without yet supporting any additional configuration or any other algorithms beside BCrypt. This change will also be useful for the ongoing FIPS work. How does this sound to you?

[peternied]

To facilitate the Security Review ...

Good news / bad news. Small PRs are great - and we can merge them into main without issue. However, we can't backport the PRs into the 2.x release branches until the security review process has been worked through. I'm not sure how long it will actually take - but it might be 4-8 weeks long, and there will be likely be extra artifacts needed to validate the design and risks. @kotwanikunal and @cwperks will be the points of contact so please follow up with them for specifics.

[dancristiancecoi]

Hi! I've raised the initial pull request:

#4381

[dancristiancecoi]

Hello!

Now that we've added support for PBKDF2 and for configuring BCrypt I propose we wait a few release cycles before introducing SCrypt and Argon2. This will allow us to sort out potential bugs with PBKDF2 and in general with the whole approach of changing the hashing algorithm (implementation and instructions) before we introduce 2 more algorithms.

Thoughts? We can modify this issue so its purely about PBKDF2 then raise 2 more issues for SCrypt and Argon2

[cwperks]

We can modify this issue so its purely about PBKDF2 then raise 2 more issues for SCrypt and Argon2

Yes please. Let's create an issue that's solely about PBKDF2 so that we can tag it for 2.16.0.

[dancristiancecoi]

Hi!

@cwperks I've raised the following issues:

• [FEATURE] Add support for configuring BCrypt password hashing #4590 (the work for this is merged but I thought it might be useful to be able to tag it for 2.16. Can just close it if you think its unnecessary)
• [FEATURE] Add support for Argon2 password hashing algorithm #4592
• [FEATURE] Add support for SCrypt password hashing algorithm. #4591

@willyborankin would you be able to modify this issue so its purely about PBKDF2 so we can close it?

Cheers

[DarshitChanpura]

@willyborankin I've modified original issue to be about PBKDF2.
cc: @dancristiancecoi