Skip to content

Add support for argon2#808

Open
Ph0tonic wants to merge 3 commits into
DirectoryTree:masterfrom
Ph0tonic:master
Open

Add support for argon2#808
Ph0tonic wants to merge 3 commits into
DirectoryTree:masterfrom
Ph0tonic:master

Conversation

@Ph0tonic

@Ph0tonic Ph0tonic commented Jun 9, 2026

Copy link
Copy Markdown

Hey, here is a PR to add support for the argon2 hashing algorithm 🎉

Add argon2 password hashing support ({ARGON2} scheme)

Adds Password::argon2i() and Password::argon2id(), and implements password
changes for argon2 users via the RFC 3062 Password Modify extended operation
(ldap_exop_passwd) — argon2 hashes embed a random salt, so the stored hash
cannot be reproduced to build the usual REMOVE/ADD batch modification.

How it works

  • Reset ($user->password = 'new'): hashes locally with password_hash()
    and queues a single REPLACE modification, using the {ARGON2} scheme
    prefix registered by OpenLDAP's argon2 module. The variant is carried by the
    PHC string (e.g. {ARGON2}$argon2id$v=19$...), matching what slapd verifies
    on bind.
  • Change ($user->password = ['old', 'new']): defers a self-service
    RFC 3062 extended operation until save(). It runs on an isolated
    connection, 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 olcPasswordHash setting.

Changes

  • New LdapInterface::exopPasswd() / Ldap::exopPasswd(), with LdapFake
    support for testing
  • New Connection::changePassword($dn, $oldPassword, $newPassword)
  • Pending password changes are held as plain serializable data on the model;
    they are cleared when the password is reassigned, on discardChanges() /
    setRawAttributes(), and only after the operation succeeds — so a
    transiently failed change is re-attempted on a retried save()
  • Exop-only changes fire the updating/updated model events, and deferred
    operations run before batch modifications to fail early and avoid partial
    updates
  • Password::hashMethodRequiresExop() centralizes the capability next to the
    other hash-method helpers; models can override
    passwordChangeRequiresExop() to route any scheme through the extended
    operation (letting the server do the hashing)
  • Deferring a change requires an existing model with a DN (throws a clear
    exception otherwise)
  • ConnectionFake::replicate() shares the underlying fake so isolated
    operations can be asserted through the original fake's expectations

Requirements

  • PHP compiled with argon2 support (libargon2 or libsodium) — the argon2
    methods throw a catchable LdapRecordException otherwise
  • OpenLDAP with the argon2 module loaded server-side (built-in module in
    2.5+, pw-argon2 contrib in 2.4), which registers the {ARGON2} scheme

@stevebauman

Copy link
Copy Markdown
Member

Thanks for the PR @Ph0tonic!

In regards to this:

Password reset ($user->password = 'new') works as expected; only the password change (array) flow is unsupported — this can be addressed in a follow-up by implementing the LDAP Password Modify extended operation (ldap_exop_passwd)

According to the docs for this method, it supports being called with $old_password here:

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|bool

So 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?

@stevebauman

stevebauman commented Jun 9, 2026

Copy link
Copy Markdown
Member

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.

@Ph0tonic

Copy link
Copy Markdown
Author

I agree — supporting this directly is the right approach.

The challenge is that setChangedPassword works by queuing a BatchModification that gets executed asynchronously when save() is called. ldap_exop_passwd doesn't fit into that model — it's a separate LDAP protocol operation that can't be expressed as a batch modification, so it can't participate in the existing modification queue.

If we call exopPasswd directly in the setter it executes immediately, breaking the deferred-write contract that the rest of the password flow follows.

To defer it to save() time, we'd need an extension point in Model::performUpdate() between the "no modifications, bail early" guard and the event dispatching — something like hasPendingUpdates() / executePendingUpdates() hooks that traits could override. Without that hook, we're forced to either override performUpdate() entirely (which duplicates the dispatch/sync sequence) or execute eagerly in the setter (which breaks the save contract).

Would you be open to adding such hooks to Model, or is there a pattern in the codebase I'm missing that already handles this kind of deferred non-batch operation?

@stevebauman

Copy link
Copy Markdown
Member

Yea you're right, will be tricky to maintain that queued behaviour. I think we could register a one-time event listener on the updating event to bail/throw an exception if the password change fails. Let me take a look into this and see what I can do 👍

@Ph0tonic

Copy link
Copy Markdown
Author

@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 argon2

The ['old', 'new'] flow normally emits REMOVE(old-hash) + ADD(new-hash), reproducing the stored old hash by extracting its salt and re-hashing. Argon2 embeds a random salt and password_hash() won't accept a caller-supplied one, so the stored hash can't be reproduced — the REMOVE can never match. So argon2 changes have to go through the RFC 3062 Password Modify extended operation (ldap_exop_passwd), which is a separate protocol op, not a batch modification.

One correction on the updating-listener idea

A one-time listener on updating won't fire in the common case. An argon2 change touches neither $this->modifications nor any dirty attribute, so getModifications() is empty — and performUpdate() early-returns on empty modifications before dispatching updating (Model.php). So when the password change is the only change, updating/updated never fire. (listenForModelEvent also registers on the shared container dispatcher, which would leak across instances.)

What I did instead

  • The setter no longer throws for argon2; it queues a deferred closure ($pendingPasswordChange).
  • Added a generic Model::performDeferredOperations() hook (default no-op) called in save() right after performUpdate()/performInsert() and before dispatch('saved'). HasPassword overrides it to flush the pending change. This guarantees it runs even when there are no batch modifications, keeps the "nothing hits the wire until save()" contract, and aborts the save if the operation fails.
  • Kept the strategy selection minimal — a passwordChangeRequiresExop() predicate (match on argon2i/argon2id) rather than a full PasswordChanger hierarchy. Easy to promote to a strategy object later if more exop-only schemes show up.
  • Added exopPasswd() to LdapInterface + Ldap (guarded on function_exists, routed through executeFailableOperation so a failure throws) + LdapFake.

Self-service vs. admin reset

The actual change lives in a new Connection::changePassword($dn, $old, $new) that runs on an isolated connection bound as the user, then performs the exop. The bind is deliberate: the Password Modify exop runs as whoever is currently bound, and our primary connection is bound as the configured admin — so a bare exop there would be an admin reset that most servers authorize without verifying the old password (a wrong old password could silently succeed). Binding as the user with the current password is what actually enforces the "must know the old password" guarantee and runs the change in the user's own context, without disturbing the primary connection's bind state.

Tests

OpenLDAP\User tests now cover: the change is deferred (no batch mods queued), reset still emits a single REPLACE, the self-service exop fires on save() even with no other modifications (asserted via LdapFake + assertMinimumExpectationCounts), and a rejected current password throws. The existing SSHA/CRYPT change flow and AD's unicodePwd flow are untouched.

BC note

The only real surface change is adding exopPasswd() to LdapInterface — a third party implementing the interface directly (rather than extending Ldap) would need to add it. Both shipped implementations (Ldap, LdapFake) do.

Happy to adjust any of the naming or move changePassword somewhere you'd prefer.

- 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.
@Ph0tonic

Ph0tonic commented Jul 7, 2026

Copy link
Copy Markdown
Author

I've pushed an update addressing several issues found while reviewing the implementation more deeply:

Wrong scheme prefix (breaking): hashes were written as {ARGON2I}/{ARGON2ID}, but OpenLDAP's argon2 module only registers the {ARGON2} scheme — both the 2.4 contrib module and the 2.5+ built-in one (BER_BVC("{ARGON2}") in contrib/slapd-modules/passwd/argon2/pw-argon2.c and servers/slapd/pwmods/argon2.c). The variant lives in the PHC string following the prefix, and slapd rejects unrecognized {...} schemes with no fallback — so a password reset would have stored a value that no subsequent bind could ever match. Password::argon2i()/argon2id() now emit {ARGON2}, and the variant is detected back from the PHC string when determining the hash method.

Missing StartTLS upgrade: Connection::changePassword() initialized and bound directly, bypassing Guard::bind() where the StartTLS upgrade happens — on use_starttls configurations, the user's current password would have been transmitted in cleartext. It now uses $connection->connect($dn, $oldPassword) on the isolated connection, which also brings SASL support, detailed BindExceptions, and multi-host retry.

Hardening of the deferred password change:

  • The pending change is stored as a plain [old, new] pair instead of a closure, keeping models serializable (e.g. queued jobs) and the state inspectable
  • It is cleared when the password is reassigned, and on discardChanges()/setRawAttributes(), so a superseded or discarded change can no longer fire on a later save()
  • It is cleared only after the operation succeeds, so a transiently failed change is re-attempted when save() is retried
  • Exop-only changes now fire the updating/updated events: deferred operations run inside performUpdate(), before batch modifications, so failures happen before any partial update
  • Deferring a change requires an existing model with a DN (clear exception otherwise)
  • PASSWORD_ARGON2I/PASSWORD_ARGON2ID are conditionally defined by PHP (only with libargon2/libsodium), so the argon2 methods now guard with defined() and throw a catchable LdapRecordException instead of a fatal Error
  • Password::hashMethodRequiresExop() centralizes the capability next to the other hash-method helpers; overriding passwordChangeRequiresExop() on a model routes any scheme through the extended operation, should server-side hashing be preferred more broadly
  • Replicated ConnectionFakes no longer reset the shared fake's state on disconnect

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

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.

2 participants