Update default configuration for PasswordEncoders

[jgrandja]

Update default configuration for Argon2PasswordEncoder, SCryptPasswordEncoder and Pbkdf2PasswordEncoder, as per recommended minimums outlined in OWASP Cheat Sheet Series .

Argon2PasswordEncoder

Recommendation:
Use Argon2id with a minimum configuration of 15 MiB of memory, an iteration count of 2, and 1 degree of parallelism.

Previous default configuration:
memory=4, iterations=3, parallelism=1

New default configuration:
memory=16, iterations=2, parallelism=1

SCryptPasswordEncoder

Recommendation:
Use scrypt with a minimum CPU/memory cost parameter of (2^16), a minimum block size of 8 (1024 bytes), and a parallelization parameter of 1.

Previous default configuration:
cpuCost=16384, memoryCost=8, parallelism=1

New default configuration:
cpuCost=65536, memoryCost=8, parallelism=1

The default salt length was also updated from 64 to 16.

Pbkdf2PasswordEncoder

Recommendation:
If FIPS-140 compliance is required, use PBKDF2 with a work factor of 310,000 or more and set with an internal hash function of HMAC-SHA-256.

Previous default configuration:
algorithm=SHA1, iterations=185000, hashLength=256

New default configuration:
algorithm=SHA256, iterations=310000, hashLength=256

The default salt length was also updated from 8 to 16.

Closes gh-10506, Closes gh-10489

[jgrandja]

@Sc00bz @larsgrefer Thank you for all your valuable feedback so far.

When you have a moment, I would appreciate it if you can take a look at the updates in this PR and provide any feedback. Thank you.

[Sc00bz]

cpuCost=65536, memoryCost=8, parallelism=1

Your names are incorrect "cpuCost" is "N" (number of blocks) and "memoryCost" is "r" or "rate" (block size). Note N*r is memory cost and you can think of parallelism as a time cost.

Pbkdf2PasswordEncoder.java, line 53:

private static final int DEFAULT_SALT_LENGTH = 8;

You might get complaints with 64 bits being low (128 to 256 bits is normal). Note 64 bits is fine because 99.99%+ of salts will be globally unique.

SCryptPasswordEncoder.java, line 69:

private static final int DEFAULT_SALT_LENGTH = 64;

512 bits for a salt is overkill.

[jgrandja]

@Sc00bz

Your names are incorrect "cpuCost" is "N" (number of blocks) and "memoryCost" is "r" or "rate" (block size)

Yeah, I realize the member names might be misleading but if you look at the javadoc:

/**
 * Creates a new instance
 *
 * @param cpuCost cpu cost of the algorithm (as defined in scrypt this is N). must be
 * power of 2 greater than 1. Default is currently 16,384 or 2^14)
 * @param memoryCost memory cost of the algorithm (as defined in scrypt this is r)
 * Default is currently 8.
 * ...
 */
public SCryptPasswordEncoder(int cpuCost, int memoryCost, int parallelization, int keyLength, int saltLength) {

cpuCost represents "N" and memoryCost represents "r".

I'll update the salt for pbkdf2 and scrypt.

Thanks for the feedback.

[rwinch]

I like your idea around the minimums based upon the Spring Security version. I'd like to propose we take it a little further to provide an easier migration path for our users by allowing users to easily opt into changes in 5.8. Rather than changing the default constructor, I'd like to propose we do a few things:

Starting in 5.8

• Introduce a static factory method that is named with the major and minor version of Spring Security that introduced the defaults for each encoder. For example defaultsForSecurity5_0()
• Deprecate default constructors
• Introduce a static factory method that is named with 6.0 version of Spring Security that introduces the defaults provided here.

In 6.0 remove the default constructor, but preserve the static factory methods as deprecated methods so that applications can continue to work.

Please also ensure there is a test for each instance of the new password encoder validating a password that was encoded from the older settings. Please hard code the encoded password.

[rwinch]

I think we should have an instance of Pbkdf2 added that uses the new defaults added because without it there is no way to encode passwords with pbkdf2 new defaults.

[jgrandja]

Added new encodingId pbkdf2-sha256 that is configured with the new defaults.

[jgrandja]

@rwinch I applied the static factory methods to Pbkdf2PasswordEncoder to help users with the migration to 5.8.

However, I did not apply the static factory methods to Argon2PasswordEncoder or SCryptPasswordEncoder since they both support upgradeEncoding(). I don't feel it makes sense here but if there are reasons I'm missing please let me know.

[rwinch]

I've left feedback inline. In addition, please update the other encoders to have static factory methods as we discussed offline.

[rwinch]

I think the -sha256 should be also related to v5_8 or all alg parameters since there are other parameters that could change. I realize that the recommendations are based on the algorithm, but these recommendations are for now and may change. That would make it difficult for us to provide additional settings later.

My quick thought is that I'd prefer the id to contain all parameters even if it is verbose as this is easier for users to understand and will be similar to other encodings work (I.e. phc, bcrypt, etc)

[jgrandja]

My quick thought is that I'd prefer the id to contain all parameters even if it is verbose

I still think it would be too verbose, for example, the encodingId for pbkdf2 would be something like pbkdf2[salt=16;iterations=310000;algorithm=sha256;hashWidth=256].

Instead, I decided to align the encodingId to the static factory method name, for example:

• argon2@SpringSecurity_v5_8 -> Argon2PasswordEncoder.defaultsForSpringSecurity_v5_8()
• scrypt@SpringSecurity_v5_8 -> SCryptPasswordEncoder.defaultsForSpringSecurity_v5_8()
• pbkdf2@SpringSecurity_v5_8 -> Pbkdf2PasswordEncoder.defaultsForSpringSecurity_v5_8()