Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Argon2PasswordEncoder #7045

Merged
merged 1 commit into from
Aug 5, 2019
Merged

Add Argon2PasswordEncoder #7045

merged 1 commit into from
Aug 5, 2019

Conversation

simmac
Copy link
Contributor

@simmac simmac commented Jun 27, 2019

Add PasswordEncoder for the Argon2 hashing algorithm (Password Hashing
Competition (PHC) winner).
This implementation uses the BouncyCastle-implementation of Argon2.

Fixes gh-5354

@pivotal-issuemaster
Copy link

@simmac Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@simmac Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 27, 2019
@simmac simmac closed this Jun 27, 2019
@simmac simmac reopened this Jun 27, 2019
rwinch
rwinch requested changes Jun 27, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @simmac! I have provided some minor comments inline.

crypto/src/main/java/org/springframework/security/crypto/argon2/Argon2EncodingUtils.java Outdated Show resolved Hide resolved
crypto/src/main/java/org/springframework/security/crypto/argon2/Argon2PasswordEncoder.java Outdated Show resolved Hide resolved
crypto/src/test/java/org/springframework/security/crypto/argon2/Argon2EncodingUtilsTests.java Outdated Show resolved Hide resolved
crypto/src/test/java/org/springframework/security/crypto/argon2/Argon2EncodingUtilsTests.java Outdated Show resolved Hide resolved
crypto/src/test/java/org/springframework/security/crypto/argon2/Argon2PasswordEncoderTests.java Outdated Show resolved Hide resolved
@larsgrefer
Copy link
Contributor

How about also implementing upgradeEncoding in the spirit of #7057 and #7042 ?

@rwinch rwinch added status: waiting-for-feedback We need additional information before we can continue in: crypto An issue in spring-security-crypto type: enhancement A general enhancement status: duplicate A duplicate of another issue and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 3, 2019
@simmac
Copy link
Contributor Author

simmac commented Jul 21, 2019

sorry for the delay.
I worked in all your feedback and implemented @larsgrefer's suggestion (thank you!) :)

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 21, 2019
@rwinch
Copy link
Member

rwinch commented Jul 26, 2019

The code looks great now. One last request. Can you please add some documentation in https://github.com/spring-projects/spring-security/blob/c05b0765c1741eaaa237ed58cb72cd57385a173a/docs/manual/src/docs/asciidoc/_includes/servlet/architecture/password-encoder.adoc#bcryptpasswordencoder Please:

  • place below bcrypt
  • follow the same outline as bcrypt
  • mention the password hashing competition
  • mention that it requires bouncy castle.

@simmac
Add Argon2PasswordEncoder
b9a081d 
Add PasswordEncoder for the Argon2 hashing algorithm (Password Hashing
Competition (PHC) winner).
This implementation uses the BouncyCastle-implementation of Argon2.

Fixes gh-5354
@simmac
Copy link
Contributor Author

simmac commented Jul 28, 2019

Done @rwinch, hope this is sufficient

@rwinch rwinch removed the status: feedback-provided Feedback has been provided label Aug 5, 2019
@rwinch rwinch self-assigned this Aug 5, 2019
@rwinch rwinch added this to the 5.2.0.RC1 milestone Aug 5, 2019
@rwinch rwinch merged commit b3da1e4 into spring-projects:master Aug 5, 2019
@rwinch
Copy link
Member

rwinch commented Aug 5, 2019

Thanks for all your hard work on this @simmac! This is now in master

@relsayed8205
Copy link

Hi,
I have a small question:
Why is the BouncyCastle dependency not added to the maven central repository by default? One have to state it explicitly in their pom.xml, which seems strange, as using maven should automate retrieving all dependencies.
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwinch rwinch rwinch approved these changes

Assignees

@rwinch rwinch

Labels
in: crypto An issue in spring-security-crypto status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.RC1
Development

Successfully merging this pull request may close these issues.

Add Argon2PasswordEncoder
6 participants
@simmac @pivotal-issuemaster @larsgrefer @rwinch @relsayed8205 @spring-projects-issues