-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Implement password hashing with argon2-cffi #3793
Conversation
Thank you - I agree that the current non-iterated use of SHA-1 is awful. I also think it is now clear that Argon2 is better than bcrypt, and I wonder if using it (and adding a dependency on it) instead of bcrypt might be more appropriate now. Or maybe Argon2 will come to the standard Python3 library soon? |
I'm happy to make further changes and look into stdlib facilities if/when a project member expresses interest in this. |
Hi @remram44, we're catching up on PR review. I'd support the use of |
I wonder if scrypt is appropriate by now. scrypt is significantly better than pbkdf2, since it is memory-hard, and is supported in Python 3.6 (scrypt), and requires OpenSSL 1.1+ I also note this interesting reflection on argon2: https://twitter.com/Sc00bzT/status/1149950559258140673 |
I'll defer to your expertise here @nealmcb. Given that argon2 is available on conda-forge I'd support taking it on as a dependency. |
I have some thoughts that I'll take to the main issue #2426. |
This can easily be updated to another method, if a consensus is reached. |
@remram44 if you make these changes I'll go ahead and merge: #2426 (comment) |
I rebased, and switched to argon2-cffi as requested. A point that was mentioned by @nealmcb was making the Argon2 parameters configurable via |
It looks like |
Any reason this was not also added to jupyter_server and ipython that have the same implementation of these functions? |
I don't think jupyter_server existed when I opened this PR. It took 2 years to merge. |
Thanks to everyone for getting this wrapped up last year. |
Fixes #2426
The
hashlib
hashing method could probably be removed frompasswd()
since nothing ever specifies thealgorithm
argument. Should probably be kept inpasswd_check()
until people update their configs.