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.

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

Replace Argon2idPasswordEncoder by SodiumPasswordEncoder #31016

Closed
nicolas-grekas opened this Issue Apr 8, 2019 · 3 comments

Comments

Projects
None yet
4 participants
@nicolas-grekas
Copy link
Member

commented Apr 8, 2019

#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.

@chalasr chalasr self-assigned this Apr 8, 2019

@chalasr chalasr added the Security label 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

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

See #31019

@chalasr chalasr removed their assignment Apr 8, 2019

@fabpot fabpot closed this Apr 9, 2019

fabpot added a commit that referenced this issue Apr 9, 2019

feature #31019 [Security] Replace Argon2*PasswordEncoder by SodiumPas…
…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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

On it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.