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
184 stars 266 forks source link

[FEATURE] Add support for PBKDF2 and Scrypt/yescrypt for passwords hashing #4079

Open willyborankin opened 4 months ago

willyborankin commented 4 months ago

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 and Scrypt.

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

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 t be add:

peternied commented 4 months ago

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

willyborankin commented 4 months ago

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

scrawfor99 commented 4 months ago

[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 commented 4 months ago

There is some crossover with this request and https://github.com/opensearch-project/security/issues/3420

dancristiancecoi commented 3 months ago

Hi, can I get assigned to this please?

dancristiancecoi commented 2 months ago

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: https://github.com/dancristiancecoi/security/commit/20295268d5648a0b0be48d4b58ed0ac89de1cd6b ( 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 commented 2 months ago

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

dancristiancecoi commented 2 months ago

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:

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:

SCrypt:

Argon2:

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 commented 2 months ago

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 commented 1 month ago

Hi! I've raised the initial pull request:

https://github.com/opensearch-project/security/pull/4381

dancristiancecoi commented 1 week ago

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 commented 1 week ago

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.