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

Fixed #30472 -- Added Argon2id support and made it the default variety of Argon2PasswordHasher. #11349

Closed
wants to merge 4 commits into from

Conversation

fengsi
Copy link
Contributor

@fengsi fengsi commented May 10, 2019

No description provided.

@ngnpope
Copy link
Member

ngnpope commented May 17, 2019

Please revert all of the unrelated changes - string formatting, quotes, etc. It makes it hard to review the changes intended to address the ticket.

@fengsi
Copy link
Contributor Author

fengsi commented May 17, 2019

@pope1ni sure, reverted PEP 8 stuff :)

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@fengsi Thanks for this patch.

Changes require dropping support for argon2-cffi < 16.3.0 please add a release notes, docs and setup.py, also auth_tests.test_hashers.TestUtilsHashPassArgon2.test_argon2_upgrade is falling for 18.2.0 >= argon2-cffi >= 16.3.0. Moreover a lot of tests in auth_tests.test_hashers is falling when argon2-cffi is not installed it is probably related with moving _load_library() to the _init__().

Please revert all unrelated changes e.g. mask_hash().

@apollo13
Copy link
Member

apollo13 commented Jun 2, 2019

This PR has many unrelated changes which is a nogo for security related changes. Also I am not sure if exposing the variety makes sense. The less options the better. A less minimal approach would be master...apollo13:argon

Also we should not rely on the argon defaults as tempting as it would be, but it would break our testsuites etc if argon ever updates those (+ causes rehashing on all sites using it). Better stick to the "known" Django updates can cause a rehash…

@fengsi
Copy link
Contributor Author

fengsi commented Jun 5, 2019

@apollo13 in general I agree that we could probably go for your approach (didn't see it before filing the ticket and also creating the PR). However, I'd highly recommend also updating the must_update() logic in your change set to perform a force rehash for any password not using Argon2id (I had mine here). Reason being if we already hard-coded a different algorithm, it's the same as if we change the default params, such as memory_cost and parallelism, which would cause rehashing as well.

@apollo13
Copy link
Member

apollo13 commented Jun 5, 2019

Ah yes, missed the rehashing on the first pass.

@felixxm
Copy link
Member

felixxm commented Jun 15, 2020

Superseded by #13066.

@felixxm felixxm closed this Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants