Make salt length configurable in Pbkdf2PasswordEncoder

[rikles]

Add constructors with a salt length input parameter.
Default salt length is still 8-byte long like before when
saltGenerator was initialized with call to
KeyGenerators#secureRandom() which use
SecureRandomBytesKeyGenerator#DEFAULT_KEY_LENGTH.

Closes gh-4372

---

This PR is related to #4372 to make salt length configurable in Pbkdf2PasswordEncoder.

Like mention in the #4372 issue, NIST Special Publication 800-132 section 5.1 said that salt length shall be at least 128 bits (16 bytes). Current Pbkdf2PasswordEncoder use a 8-byte (64-bit) random salt with no possibility to configure its length.

Users that want to conform to NIST Special Publication 800-132 (or other rules requiring different that 8-byte random salt) for password storage with PBKDF2 algorithm, have no choice to develop their own PBKDF2 PasswordEncoder from scratch. They can't inherit from current Pbkdf2PasswordEncoder class because the saltGenerator attribute is private final and not initialized in a at least protected constructor.
This could be a security problem in case of bad implementation or if a new spring-security version fix a security issue in the Pbkdf2PasswordEncoder class an the user not apply this fix to its own PasswordEncoder.

PS : I've not added @since annotation yet because i don't know if constructors are concerned by this annotation and the target version.

PS : I've submitted the CLA

[rwinch]

Thanks for the PR! Can you please remove any unrelated changes. For example, the changes for matches test is not related https://github.com/spring-projects/spring-security/pull/9147/files#diff-b33c45ec3428094cc83f413049097a5d95d3cf54f43d7f13d372be66e82cdd0dR46-R52

[rikles]

I modified the tests to ensure that the "default" instance and the custom salt length instance pass same kind of tests with success. I think that simply adding encodedLengthSuccess test is not enough (for example, matches method could potentially fail if the salt extraction from the encoded password is not "custom salt length" aware).

But if you want, i can revert changes on matches test. Are there some other tests you want me to revert ?

[rwinch]

@rikles Thanks for the response. I'd like existing tests to stay the same, so let's leave matches test the same (don't make it allSatisifes. I think you are right that we should have additional tests for custom salt length, but they can be an entirely new test. For example, add a test named matchesWhenCustomSaltLengthThenMatchesTrue. Does that make sense?

[rikles]

Hello @rwinch.

It's my first PR on Spring Security, so i've to adjust my work to this project test philosophy...
If i understand your logic, i'd "duplicate" 9 of existing test methods (matches, matchesLengthChecked, notMatches, encodeSamePasswordMultipleTimesDiffers, passivity ...) to test the "custom salt length" initial configuration (ie encoderSalt16 instance) in isolated methods (see below) ?

My approach is to group tests by aims like before (ex: matches tests whether a raw password matches with the encode result of this raw password, notMatches tests a raw password doesn't matches with a different password encode result...) and those unitary test purposes are checked on different initial configurations (encoder instances).
To be more complete on initial conditions, i'd like to add some other password encoder instances to the encoders array : one instance with no secret, another with a custom hash size (but, i didn't because it's not related to my current pull request subject. I think i will submit another PR for this).
With my approach, no need to change (except to complete zipSatisfy tests) or add tests methods to tests those other configurations, we simply have to add them to the encoders array. This reduce duplicate code, make test purpose clear and same for all configurations. But the counter parts are a little more changes in test history and if a test fails for only one initial configuration, the entire test methods fails. I think this is not a big problem for precise tests that are proof of working for a code change and/or non-regression guards for future changes.

What do you think about this ?

If you prefer seperate methods, i plan to add those methods, is that OK ? :

• matchesWhenCustomSaltLengthThenSuccess
• matchesLengthCheckedWhenCustomSaltLengthThenSuccess
• notMatchesWhenCustomSaltLengthThenSuccess
• encodeSamePasswordMultipleTimesWhenCustomSaltLengthThenDiffers
• passivityWhenCustomSaltLengthThenSuccess
• encodeAndMatchWhenBase64AndCustomSaltLengthThenSuccess
• encodeWhenBase64AndCustomSaltLengthThenSuccess
• matchWhenBase64AndCustomSaltLengthThenSuccess
• encodeAndMatchWhenSha256AndCustomSaltLengthThenSuccess

[rwinch]

Thanks. I do prefer adding the additional methods. That ensures that we know exactly what broke just by seeing the name of the test failure. The test method names you have proposed look good to me. Thanks!

[rikles]

Hello @rwinch,
I've removed my changes on test methods (removing allSatisfy) and added the new test methods to test the "custom salt length" configuration.

I've rebased my branch on current master head. On my device, ./gradlew clean build integrationTest run with success, but the PR Build job failed with this error :

Execution failed for task ':spring-security-samples-xml-ldap:appBeforeIntegrationTest'.
> java.net.ConnectException: Connection refused (Connection refused)

Maybe a change on CI/tests configuration occured since my previous commit ?

[rwinch]

Thanks the changes look good to me. I think we should now consider implementing the upgradeEncoding method to ensure that a bigger salt means that the password is re-encoded. Thoughts?

[rikles]

For now, the salt length is not extracted from the encoded password in matches methods, but form saltGenerator attribute.
So, if the salt length in encoded password differs from salt length defined in the password encoder instance, matches will return false.
In consequence, if i understand the upgradeEncoding method purpose, i think we have two options :

• say that the salt length is fixed for the password encoder instance and not retrieve from the encoded password
  => so we leave matches method as this and upgradeEncoding always return false.
• say that the salt length is fixed for password encoding only but is extracted from the encoded password (passwordEncodedLength - hashSize / 8) on match test
  => so we have to change the matches methods and to implement upgradeEncoding. But doing this may have consequence on projects using the Pbkdf2PasswordEncoder. May this be done in a separate pull request ? And what about hash size or iterations count ?

But i never used the upgradeEncoding methods so perhaps i'm wrong.

What do you think ?

[rwinch]

In hindsight I don't think we can calculate the salt length based on the size of the output because that also changes based on the hash width parameter. Also it is not quite clear how/when to upgrade with iterations, hash width, and salt length. I agree with your your thoughts about revisiting upgradeEncoding on another issue.

[rikles]

Hello @rwinch,
Is this enhancement will be ported in 4.2.x and 5.x branches ?

[rwinch]

No. We follow semantic versioning. Generally, enhancements are only included in the latest branch. The other branches are for patches only to reduce risk.