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 crypt with passlib #319

Closed
wants to merge 14 commits into from
Closed

Replace crypt with passlib #319

wants to merge 14 commits into from

Conversation

hagenest
Copy link
Contributor

@hagenest hagenest commented Jun 5, 2024

fix #318

@hagenest
Copy link
Contributor Author

hagenest commented Jun 5, 2024

Still not completely sure why this isn't working, but maybe it has something to do with the fact that our previous function created a string with pw + salt, even though for SHA512 Dovecot (and I suppose hashlib too) does salt + pw.

See https://doc.dovecot.org/configuration_manual/authentication/password_schemes/#salting

Gotta go to sleep now :D

@hagenest
Copy link
Contributor Author

hagenest commented Jun 5, 2024

also, with the new implementation, the hashed pw is longer than with the old one.

{SHA512-CRYPT}$6$bfda1eb3f32d03a608ceaba76b73356682c25929f03df7b46e2c0811aca0dd002749c1fe72fd4049355f409d6b6ae39752f44148a4840c6a48f45772930fefcb
---
{SHA512-CRYPT}$6$LSXKC072aV0MhmvR$YMy5pV6iiF.o5iWSxz3qUe/Kjat00fvnzlp5sf93kbI2a2Ol3QF8.ye1rQENkDGh3Q/CZnYhgW4tY/Bl4zIVX1

@missytake
Copy link
Contributor

Hm, didn't dive into it deeper. But will passphrases which were hashed with the old function still work? For that the new function needs to return exactly the same hash as the old function, or (a bit ugly) a migration function which detects on login if the passphrase is the old or new hash, uses the appropriate method to authenticate, and updates the hash in the database to the new method on successful login.

I'm also unsure where the salt is added; with the old method, it is randomly generated and stored in the database in the same string as the actual hash (i.e. {hash-method}$version$salt$hash). The new hash format doesn't seem to have this format.

@hagenest
Copy link
Contributor Author

hagenest commented Jun 6, 2024

Good point, maybe we should only replace crypt on new installations. But even then we are only kicking the bucket down the road, because as soon as they update to Python 3.13, it stops working anyways. So a migration function is our only real option... :/

Regarding the formatting of the hash, the correct way, as mentioned in the dovecot docs should be $6$salt$hash. You are right, the crypt function doesn't say how exactly it does this, and this may even be different on different platforms as crypt uses the systems cryptography functions, if I understand correctly.

Anyways, crypt is using a 16 character salt and 86 character hash, have to find out what hashlib does

@hagenest
Copy link
Contributor Author

hagenest commented Jun 6, 2024

Probably useful: https://eighty-twenty.org/2024/01/13/python-crypt-shacrypt

Edit: Or not, this does seem overly complicated

@missytake
Copy link
Contributor

Probably useful: https://eighty-twenty.org/2024/01/13/python-crypt-shacrypt

Edit: Or not, this does seem overly complicated

ugh. not sure how complicated it is, but I'd prefer to use a maintained library for our password hashing. hashlib should be able to do this, and to produce a hash exactly like crypt did in the past, no?

@missytake
Copy link
Contributor

I think this is how salting works with hashlib: https://docs.python.org/3/library/hashlib.html#randomized-hashing

@hagenest
Copy link
Contributor Author

hagenest commented Jun 6, 2024

Probably useful: https://eighty-twenty.org/2024/01/13/python-crypt-shacrypt
Edit: Or not, this does seem overly complicated

ugh. not sure how complicated it is, but I'd prefer to use a maintained library for our password hashing. hashlib should be able to do this, and to produce a hash exactly like crypt did in the past, no?

they are using only hashlib, it's just that their code to reproduce crypt's functionality 1 to 1 is so long that they put it in a seperate file.

@hagenest
Copy link
Contributor Author

hagenest commented Jun 6, 2024

I think this is how salting works with hashlib: https://docs.python.org/3/library/hashlib.html#randomized-hashing

duh :D

I've seen that, obviously. Sadly the docs don't explain their defaults for SHA512 salts.

@hagenest
Copy link
Contributor Author

hagenest commented Jun 6, 2024

Another option would be to use passlib (https://passlib.readthedocs.io/en/stable/), which while an additional dependency is recommended as a full replacement for crypt in the python docs (https://docs.python.org/3/library/crypt.html)

"The hashlib module is a potential replacement for certain use cases. The passlib package can replace all use cases of this module."

@hagenest
Copy link
Contributor Author

hagenest commented Jun 6, 2024

fyi in #318 is more context

@hagenest
Copy link
Contributor Author

hagenest commented Jun 16, 2024

So, passlib does it, but not in the correct format. This seems to work, but is ugly:

def encrypt_passlib(password: str):
    pw = passlib.hash.sha512_crypt.hash(password).split("$")
    return "{SHA512-CRYPT}$" + pw[1] + "$" + pw[3] + "§" + pw[4] 

Edit: haha, the § does not help here :D

@hagenest hagenest changed the title Replace crypt with hashlib Replace crypt with passlib Jun 16, 2024
@hagenest
Copy link
Contributor Author

linter fails, will fix tomorrow

@hagenest hagenest closed this Jun 18, 2024
@missytake
Copy link
Contributor

missytake commented Jun 18, 2024

The CI fail was very likely due to the staging.testrun.org cert failing, if we re-open this one day, we should re-check, maybe it simply works.

@hagenest hagenest reopened this Jun 18, 2024
@hagenest
Copy link
Contributor Author

@missytake you sure that the CI is working now?

@missytake
Copy link
Contributor

@missytake you sure that the CI is working now?

nope, there is still something wrong... :/

@missytake missytake force-pushed the hagi/#318-crypt=>hashlib branch from e5707b7 to 3eae165 Compare June 19, 2024 12:42
@missytake
Copy link
Contributor

Nope, it's still broken. Account creation works, but authenticating doesn't. Let's close and re-open as soon as we have to support python 3.13 :)

@missytake missytake closed this Jun 19, 2024
@hagenest
Copy link
Contributor Author

at least the CI works :)

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.

python: migrate away from crypt library
2 participants