Skip to content

Replace Argon2idPasswordEncoder by SodiumPasswordEncoder #31016

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
Closed
nicolas-grekas opened this issue Apr 8, 2019 · 3 comments
Closed

Replace Argon2idPasswordEncoder by SodiumPasswordEncoder #31016

nicolas-grekas opened this issue Apr 8, 2019 · 3 comments
Labels
Feature Security

Comments

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 8, 2019 •
edited
Loading

#31014 makes no sense to me: we added a class for which we cannot guarantee that it will work depending on a default that is under control of libsodium only.
To me this is the sign that we should adopt the approach of libsodium instead: we should replace Argon2idPasswordEncoder by SodiumPasswordEncoder and align to its recommendation: trust them to always select the best default in the future. It's not like we have the choice: there is no other ways permitted by the extension (and I trust them on that it's the best).

Similarly, I would add a new NativePasswordEncoder that would always use PASSWORD_DEFAULT, and deprecate Argon2iPasswordEncoder and BCryptPasswordEncoder.
@nicolas-grekas nicolas-grekas mentioned this issue Apr 8, 2019
Merged
@chalasr chalasr self-assigned this Apr 8, 2019
@chalasr chalasr changed the title Replace Argon2idPasswordEncoder by LibsodiumPasswordEncoder Replace Argon2idPasswordEncoder by SodiumPasswordEncoder Apr 8, 2019
@chalasr chalasr added the Feature label Apr 8, 2019
@chalasr chalasr mentioned this issue Apr 8, 2019
Merged
@chalasr
Copy link
Member

chalasr commented Apr 8, 2019

See #31019

@chalasr chalasr removed their assignment Apr 8, 2019
@fabpot fabpot closed this as completed Apr 9, 2019
fabpot added a commit that referenced this issue Apr 9, 2019
@fabpot
feature #31019 [Security] Replace Argon2*PasswordEncoder by SodiumPas…
ace49c1 
…swordEncoder (chalasr)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Security] Replace Argon2*PasswordEncoder by SodiumPasswordEncoder

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      |  no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #31016
| License       | MIT
| Doc PR        | todo symfony/symfony-docs#11368

See fixed ticket, much simpler/secure/maintainable.

Commits
-------

529211d [Security] Replace Argon2*PasswordEncoder by SodiumPasswordEncoder
@ro0NL
Copy link
Contributor

ro0NL commented Apr 12, 2019

Similarly, I would add a new NativePasswordEncoder that would always use PASSWORD_DEFAULT, and deprecate Argon2iPasswordEncoder and BCryptPasswordEncoder.

didnt happen (yet?)

@chalasr
Copy link
Member

chalasr commented Apr 12, 2019

On it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature Security
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@fabpot @nicolas-grekas @ro0NL @chalasr