Add support for argon2#808
Conversation
|
Thanks for the PR @Ph0tonic! In regards to this:
According to the docs for this method, it supports being called with https://www.php.net/manual/en/function.ldap-exop-passwd.php function ldap_exop_passwd(
LDAP\Connection $ldap,
string $user = "",
#[\SensitiveParameter] string $old_password = "",
#[\SensitiveParameter] string $new_password = "",
array &$controls = null
): string|boolSo it looks like we may be able to support the same flow without throwing the exception here. We could extract the mechanism for changing passwords into some interface and classes depending on the method being used. Maybe something like (psuedo-code): $changer = match (strtolower($method)) {
'argon2i', 'argon2id' => new ArgonPasswordChanger(...),
default => new DefaultPasswordChanger(...),
};Thoughts? |
|
In other words I think we should implement the full operation here instead of in a follow up, because if the exception is no longer thrown then that's a major breaking change. |
|
I agree — supporting this directly is the right approach. The challenge is that If we call To defer it to Would you be open to adding such hooks to |
|
Yea you're right, will be tricky to maintain that queued behaviour. I think we could register a one-time event listener on the |
|
@stevebauman — pushed an update that implements the full change flow (no more exception). Summary of the approach and the design decisions behind it: Why the array syntax can't stay a batch modification for argon2The One correction on the
|
- Use the {ARGON2} scheme prefix for argon2i/argon2id hashes: OpenLDAP's
argon2 module registers only {ARGON2}, the variant being carried by
the PHC string that follows it. Values stored with the previous
{ARGON2I}/{ARGON2ID} prefixes were never recognized by slapd, failing
every subsequent bind. The variant is now detected back from stored
{ARGON2} values when determining the hash method.
- Guard the argon2 methods with defined() checks so PHP builds without
argon2 support throw a catchable LdapRecordException instead of a
fatal undefined-constant Error.
- Connect as the user in Connection::changePassword() instead of
initializing and binding directly, so the connection is upgraded with
StartTLS (when configured) before any credentials are transmitted.
- Store the pending password change as an [old, new] pair rather than a
closure, keeping models serializable and the deferred state
inspectable.
- Clear the pending change when the password is reassigned, on
discardChanges(), and on setRawAttributes(); clear it only after the
operation succeeds so a transiently failed change is re-attempted on
a retried save.
- Require an existing model with a distinguished name to defer a
password change.
- Run deferred operations inside performUpdate() so exop-only changes
fire the updating/updated events, and run them before batch
modifications to fail early and avoid partial directory updates.
- Add Password::hashMethodRequiresExop() alongside the existing
hash-method capability helpers; models may override
passwordChangeRequiresExop() to route other schemes through the
extended operation.
- Prevent replicated ConnectionFake instances from resetting the shared
fake's state on disconnect.
|
I've pushed an update addressing several issues found while reviewing the implementation more deeply: Wrong scheme prefix (breaking): hashes were written as Missing StartTLS upgrade: Hardening of the deferred password change:
Tests were added for each of these, and I've updated the PR description to reflect the actual implementation (the exop-based change flow, rather than the exception initially mentioned). |
Hey, here is a PR to add support for the argon2 hashing algorithm 🎉
Add argon2 password hashing support ({ARGON2} scheme)
Adds
Password::argon2i()andPassword::argon2id(), and implements passwordchanges for argon2 users via the RFC 3062 Password Modify extended operation
(
ldap_exop_passwd) — argon2 hashes embed a random salt, so the stored hashcannot be reproduced to build the usual
REMOVE/ADDbatch modification.How it works
$user->password = 'new'): hashes locally withpassword_hash()and queues a single
REPLACEmodification, using the{ARGON2}schemeprefix registered by OpenLDAP's argon2 module. The variant is carried by the
PHC string (e.g.
{ARGON2}$argon2id$v=19$...), matching what slapd verifieson bind.
$user->password = ['old', 'new']): defers a self-serviceRFC 3062 extended operation until
save(). It runs on an isolatedconnection, connected as the user themselves (with StartTLS upgrade when
configured), so the directory verifies the current password. The server
hashes the new password according to its
olcPasswordHashsetting.Changes
LdapInterface::exopPasswd()/Ldap::exopPasswd(), withLdapFakesupport for testing
Connection::changePassword($dn, $oldPassword, $newPassword)they are cleared when the password is reassigned, on
discardChanges()/setRawAttributes(), and only after the operation succeeds — so atransiently failed change is re-attempted on a retried
save()updating/updatedmodel events, and deferredoperations run before batch modifications to fail early and avoid partial
updates
Password::hashMethodRequiresExop()centralizes the capability next to theother hash-method helpers; models can override
passwordChangeRequiresExop()to route any scheme through the extendedoperation (letting the server do the hashing)
exception otherwise)
ConnectionFake::replicate()shares the underlying fake so isolatedoperations can be asserted through the original fake's expectations
Requirements
methods throw a catchable
LdapRecordExceptionotherwise2.5+,
pw-argon2contrib in 2.4), which registers the{ARGON2}scheme