Skip to content

Fixed #19518 - added support for SASL LDAP - #19556

Open
snipe wants to merge 3 commits into
developfrom
#19518-sasl-without-credentials
Open

Fixed #19518 - added support for SASL LDAP#19556
snipe wants to merge 3 commits into
developfrom
#19518-sasl-without-credentials

Conversation

@snipe

@snipe snipe commented Aug 26, 2026

Copy link
Copy Markdown
Member

This adds an opt-in Use SASL EXTERNAL bind toggle to the LDAP wizard so Snipe-IT can authenticate to a directory with the client TLS cert / key already configured on step 1, instead of a bind DN + password. Fixes #19518.

This (should?) enable Google Workspace LDAP and other directories that use mTLS certificate authentication.

Summary

  • New checkbox on step 1 (Connection), next to the client TLS cert / key fields it depends on.
  • Hidden when Active Directory mode is on. AD cert-based auth is a niche within a niche and clutters the most common AD path.
  • Server-side gate: cannot enable without both cert and key populated. Toggling AD mode on auto-clears any stale SASL flag.
  • Bind DN/UPN and Bind Password on step 2 are locked and no longer required when SASL EXTERNAL is on. Values in those fields, if present, are ignored by the bind path.
  • ldap:troubleshoot artisan command routes to ldap_sasl_bind() in the same shape. --ldap-search preview emits -Y EXTERNAL (dropping -x / -D / -w) so operators can copy-paste the shell equivalent.
  • Off by default. Existing installs keep the simple / anonymous bind behavior they had before.
  • Read-only in demo mode, like the other LDAP/AD fields

(Note: The -Y flag specifies the SASL mechanism to be used for authentication. If it's not specified, the program will choose the best mechanism the server knows.)

Testing

  • Open the LDAP wizard step 1 and confirm the new checkbox renders below the client TLS cert field.
  • Toggle the checkbox with cert / key unpopulated and confirm the inline error points at the cert fields.
  • Populate cert + key, enable the checkbox, save step 1, advance to step 2, and confirm Bind DN/UPN + Bind Password are optional and read-only.
  • Run the step 2 test-bind against a real Google Workspace LDAP tenant and confirm SASL EXTERNAL bind succeeds.
  • Run php artisan ldap:troubleshoot --ldap-search with SASL EXTERNAL on and confirm the ldapsearch uses -Y EXTERNAL instead of -x -D … -w ….
  • Run php artisan ldap:troubleshoot and confirm Stage 4 admin bind + Stage 5 informational bind both succeed via SASL EXTERNAL.
  • Toggle Active Directory mode on and confirm the SASL EXTERNAL checkbox hides. If it was previously enabled, confirm the flag auto-clears.

@codacy-production

codacy-production Bot commented Aug 26, 2026

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 1 minor

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category Results
CodeStyle 1 minor

View in Codacy

🟢 Metrics 19 complexity

Metric Results
Complexity 19

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@uberbrady uberbrady left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a large amount of changed code for something ought to be as simple as "...and if you enter no username nor password, but you have a certificate, try using that." I don't even think you need a new field for that. And I definitely don't think we should be using the ldap_sasl_bind() function, which only has stub documentation here: https://www.php.net/manual/en/function.ldap-sasl-bind.php. I think you should be able to keep using ldap_bind() as it was without any modification. I do think that something probably should change in the LdapTroubleshooter to handle the null-username (and/or null-password) cases.

According to the PHP documentation for ldap_bind -

If password is not specified or is empty, an anonymous bind is attempted. The dn can also be left empty for an anonymous bind. This is defined in https://tools.ietf.org/html/rfc2251#section-4.2.2

Once we have a new take for this, I can test against our own Google Workspace setup trying a password-less certificate to make sure that it works as intended.

$identityLabel = 'the SASL EXTERNAL client certificate';
} else {
$bind_results = ldap_bind($conn, $username, $password);
$identityLabel = $username;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better as:

$identityLabel = " as $username";

And then pulling the as $identityLabel below. And changing the $identityLabel for when $username is null to " using the SASL EXTERNAL client certificate". Then it would end up saying:

WARNING: Failed to bind to $ldap_url as $username
or
WARNING: Failed to bind to $ldap_url using the SASL EXTERNAL client certificate

@snipe

snipe commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

Yeah, I can kill the checkbox, that's fair. Cert + key + blank uname/pword should be enough to auto-detect. 🤞

I don't think empty-cred ldap_bind() will cut it though. That path is anonymous auth per RFC 2251 §4.2.2 (for the nerdy nerds like you), and the server doesn't auto-promote the mTLS identity into the bind. That's exactly what SASL EXTERNAL is for (RFC 4513 §5.2). Google Workspace's own LDAP docs require it, OpenLDAP's admin guide too. The user requesting this feature ran into exactly the anonymous-session, queries-rejected problem.

Sparse PHP docs on ldap_sasl_bind() are annoying of course, but it wraps openldap's ldap_sasl_bind_s(), which is the same C API samba and sssd use, and it's not like the regular ldap_bind(), ldap_connect() stuff is super well documented. All of the PHP ldap functions have always just been thin wrappers, so... ¯_(ツ)_/¯

Here's my counter-proposal:

  • Delete the setting, migration and checkbox in the UI
  • Keep ldap_sasl_bind() in bindAdminToLdap(), auto-detect on cert + key populated + uname/pword blank.
  • Same in LdapTroubleshooter.
  • Tests stay unchanged, since they mock the bind call.

What say you?

@uberbrady

Copy link
Copy Markdown
Member

I'm grumpy about it, but if (and only if) I really cannot get it to hook up with a "regular" ldap_bind, then I'll be OK with it (-ish).

But once you have a re-worked PR, I can try and fiddle with it and see if we really do need that method for it to work. If we don't, then I'd love to not use it.

@snipe
snipe requested a review from uberbrady August 31, 2026 13:14
@snipe

snipe commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Okay Mr. Grumpy Pockets. Go ahead and empty those grumpy pockets and pull down this branch and give it a go when you get a moment.

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