-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add password4j implementation of PasswordEncoder #17825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add password4j implementation of PasswordEncoder #17825
Conversation
74cf846
to
ae43ab4
Compare
Here is my PR. I’d really appreciate any feedback or suggestions for improvements. |
ca45b53
to
d9494ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR @mehrdadbozorgmehr!
I've provided feedback inline. Once those get addressed, we will want to discuss updating the documentation.
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
crypto/src/main/java/org/springframework/security/crypto/factory/PasswordEncoderFactories.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
...test/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoderTests.java
Show resolved
Hide resolved
099f552
to
c4d71a5
Compare
f3b431c
to
04f994c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress! Thanks again for your PR.
I've added a few inline comments. Please also add documentation to docs/modules/ROOT/pages/features/authentication/password-storage.adoc
and docs/modules/ROOT/pages/features/whats-new.adoc
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
7d67c20
to
8bf697a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we can reuse it as a base class for most of the algorithms, it looks like a single Password4jPasswordEncoder
will not be possible. I've provided some feedback inline.
PS: Sorry for finding this late and reverting some of my previous feedback.
...test/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoderTests.java
Outdated
Show resolved
Hide resolved
8bf697a
to
fe8626b
Compare
441b4cd
to
7daccae
Compare
Thanks for pointing this out. I’ve updated the design by making Password4jPasswordEncoder abstract and introducing algorithm-specific subclasses for the working implementations. I’ve also implemented a PBKDF2-specific encoder and Balloon hashing in this PR with proper salt handling. @rwinch , please let me know if any additional adjustments are required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes. Overall they look good. Please:
- Rebase to resolve the conflict
- Add documentation including a whats-new.adoc entry that links to the newly added documentation
- Create two tickets:
- For the password4j implementations supporting upgradeEncoding method
- For the password4j implementations being able to match on the same algorithm when the algorithm contains different parameters. This can be done by using the static
getInstanceFromHash(String)
method on the implementation (e.g.BcryptFunction.getInstanceFromHash(String)
). Note that not all implementations will be able to support this because not all hashes include the parameters in the resulting hash.
7daccae
to
a4e6021
Compare
69bcb69
to
ea0a347
Compare
…hm selection and enhance documentation Closes spring-projectsgh-17706 Signed-off-by: M.Bozorgmehr <[email protected]> Add Password4jPasswordEncoder for enhanced password hashing support Signed-off-by: M.Bozorgmehr <[email protected]> Signed-off-by: M.Bozorgmehr <[email protected]> Add Password4jPasswordEncoder for enhanced password hashing support Signed-off-by: M.Bozorgmehr <[email protected]> Signed-off-by: Mehrdad <[email protected]> Signed-off-by: M.Bozorgmehr <[email protected]>
…hm selection and enhance documentation Closes spring-projectsgh-17706 Signed-off-by: M.Bozorgmehr <[email protected]> Signed-off-by: Mehrdad <[email protected]> Signed-off-by: M.Bozorgmehr <[email protected]>
…ibrary Closes spring-projectsgh-17706 Signed-off-by: Mehrdad <[email protected]> Signed-off-by: M.Bozorgmehr <[email protected]>
…rd encoders using Password4j library Closes spring-projectsgh-17706 Signed-off-by: Mehrdad <[email protected]> Signed-off-by: M.Bozorgmehr <[email protected]>
…BCrypt, Scrypt, PBKDF2, and Balloon hashing Closes spring-projectsgh-17706 Signed-off-by: M.Bozorgmehr <[email protected]>
ea0a347
to
b2d4c52
Compare
Closes gh-17706