Closed lokesh-wise closed 3 years ago
Hi, you can create a pull request for this. The changes need to be done in these locations:
https://github.com/LDAPAccountManager/lam/blob/11cca1212893d636ddf83cb1a27f701e78ed012b/lam/lib/account.inc#L225 https://github.com/LDAPAccountManager/lam/blob/11cca1212893d636ddf83cb1a27f701e78ed012b/lam/lib/account.inc#L165 https://github.com/LDAPAccountManager/lam/blob/11cca1212893d636ddf83cb1a27f701e78ed012b/lam/lib/account.inc#L385
Please also add unit tests here:
https://github.com/LDAPAccountManager/lam/blob/develop/lam/tests/lib/accountTest.php
I merged the branch to feature/113-argon2. But it seems not to work. The value in LDAP is {ARGON2ID} and a base64-encoded value. But it should be something like {ARGON2}$argon2i$v=19$m=4096,t=3,p=1$uJyf0UfB25SQTfX7oCyK2w$U45DJqEFwD0yFaLvTVyACHLvGMwzNGf19dvzPR8XvGc
https://git.openldap.org/openldap/openldap/-/tree/master/contrib/slapd-modules/passwd/argon2
So it seems the hash type is wrong and base64 encoding should be removed. Can you check?
@gruberroland I've make those changes here https://github.com/LDAPAccountManager/lam/pull/116
So, hash looks like this now.
{ARGON2}$argon2i$v=19$m=65536,t=4,p=1$VEpUMFoxaGZyc3ZiLnhLMw$46Yu/Wcvo6FfpUs1kpP8MtPqPLw8dRYanenqzbFeQUQ
Hopefully, it would work now. Could you please check again?
Thanks for the update. Just one question about the algorithm - you changed from Argon2id to Argon2i. Is there any specific reason for this? Seems that the "id" variant is the recommended one. (7.4 in https://datatracker.ietf.org/doc/draft-irtf-cfrg-argon2/?include_text=1)
Also, PHP version requirement can stay on 7.3.
Maybe you switched because the link above only shows agon2i in the examples? But it seems the module supports all 3 variants:
@gruberroland You are right, I changed it to argon2i because I thought it isn't supported in LDAP. Now, that you have mentioned it is supported, I've reverted it to argon2id. Now the hash looks as follow-
{ARGON2}$argon2id$v=19$m=65536,t=4,p=1$SElucXA0NUNualViUkNZWA$BSox1Bz0B8e7nx6N3F34FnfrkNAerccHT+QMB6ZX62c
Thanks a lot for your update @lokesh-wise. I just merged it to develop. The change will be part of the next LAM release in a few days.
@gruberroland Great! Thank you for verifying the fix.
I'm willing to add support for this in LAM. What do I need to do in order to add its support?