deltachat / chatmail

chatmail service deployment scripts and docs
https://delta.chat/en/2023-12-13-chatmail
MIT License
126 stars 11 forks source link

Replace crypt with passlib #319

Closed hagenest closed 3 months ago

hagenest commented 3 months ago

fix #318

hagenest commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

Edit: Or not, this does seem overly complicated

missytake commented 3 months ago

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 commented 3 months ago

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

hagenest commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

fyi in #318 is more context

hagenest commented 3 months ago

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 commented 3 months ago

linter fails, will fix tomorrow

missytake commented 3 months ago

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 commented 3 months ago

@missytake you sure that the CI is working now?

missytake commented 3 months ago

@missytake you sure that the CI is working now?

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

missytake commented 3 months ago

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 :)

hagenest commented 3 months ago

at least the CI works :)