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

Set argon2 as default hashing algorithm for mix.gen.auth #4471

Conversation

goncalotomas
Copy link
Contributor

@goncalotomas goncalotomas commented Sep 10, 2021

Argon2 is now recommended by OWASP as being a modern and secure password hashing algorithm. The OWASP Password Storage page states that the first choice should be Argon2, listing bcrypt as a second option when Argon2 is not available.

This Pull Request sets Argon2 as a default algorithm used in mix.gen.auth and updates the test suite accordingly.

I decided not to update the default option for Windows as I am unaware of the stability of the Argon2 library on that Operating System.

I tried to follow the contribution guidelines but seeing as this is my first PR to Phoenix I will be happy to change anything that is not as expected.

@josevalim josevalim merged commit 1d5029f into phoenixframework:master Sep 10, 2021
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@goncalotomas goncalotomas deleted the fix/set-argon2-default-in-mix-gen-auth branch September 10, 2021 23:00
kgautreaux added a commit to kgautreaux/phoenix that referenced this pull request Sep 15, 2021
josevalim pushed a commit that referenced this pull request Sep 17, 2021
This just syncs the documentation with the PR from @goncalotomas in #4471
@chrismccord
Copy link
Member

We may need to revert this as some users are experiencing OOM failures on smaller instances. It seems argon2 is enough to OOM 256MB instances easily, so I think unfortunately it's a poor default when bcrypt remains a great option.

ref: https://elixirforum.com/t/staging-environment-how-to-debug-out-of-memory-errors-in-production-on-fly-io/42763

I will touch base with the team tomorrow and cut a new release if everything checks out.

josevalim added a commit that referenced this pull request Oct 1, 2021
@josevalim
Copy link
Member

I have reverted it on master but added notes that users should consider using argon2. 👍

chrismccord pushed a commit that referenced this pull request Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants