ouyang/snipe-it
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Improved find-and-bind for complex LDAP directory structures

Browse Source
This commit is contained in:
Brady Wetherington
2025-09-09 15:02:50 +01:00
parent 2dbec867d9
commit a07d83e583
1 changed files with 85 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/Models/Ldap.php
+85 -2
View File
@@ -104,6 +104,78 @@ class Ldap extends Model
return $connection;
}
/**
* Finds user via Admin search *first*, and _then_ try to bind as that user, returning the user attributes on success,
* or false on failure. This enables login when the DN is harder to programmatically 'guess' due to having users in
* various different OU's or other LDAP entities.
*/
public static function findAndBindMultiOU(string $baseDn, string $filterQuery, string $password, int $slow_failure = 3): array|false
{
/**
* If you *don't* set the slow_failure variable, do note that we might permit timing attacks in here - if
* your find results come back 'slow' when a user *does* exist, but fast if they *don't* exist, then you
* can use this to enumerate users.
*
* Even if that's *not* true, we still might have an issue: if we don't find the user, then we don't even _try_
* to bind as them. Again, that could permit a timing attack.
*
* Instead of checking every little thing, we just wrap everything in a try/catch in order to unify the
* 'slow_failure' treatment. All failures are re-raised as exceptions so that all failures exit from the
* same place.
*/
$connection = null;
$admin_conn = null;
try {
/**
* First we get an 'admin' connection, which will need search permissions. That was already a requirement
* here, so that's not a big lift. But it _is_ possible to configure LDAP to only login, and *not* to be
* able to import lists of users. In that case, this function *will not work* - and you should use the
* legacy 'findAndBindUserLdap' method, below. Otherwise, it looks like this would attempt an anonymous
* bind - which you might want, but you probably don't.
*
**/
$admin_conn = self::connectToLdap();
self::bindAdminToLdap($admin_conn);
$results = ldap_search($admin_conn, $baseDn, $filterQuery);
$entry_count = ldap_count_entries($admin_conn, $results);
if ($entry_count != 1) {
throw new \Exception('Wrong number of entries found: ' . $entry_count);
}
$entry = ldap_first_entry($admin_conn, $results);
$user = ldap_get_attributes($admin_conn, $entry);
$userDn = ldap_get_dn($admin_conn, $entry);
if (!$userDn) {
throw new \Exception("No user DN found");
}
\Log::debug("FOUND DN IS: $userDn");
// The temptation now is to do ldap_unbind on the $admin_conn, but that gets handled in the 'finally' below.
// I don't know if that means a separate 'connection' is maintained to the LDAP server or not, and would
// definitely prefer to not do that if we can avoid it. But I don't know enough about the LDAP protocol to
// be certain that that happens.
//now we try to log in (bind) as that found user
$connection = self::connectToLdap();
$bind_results = ldap_bind($connection, $userDn, $password);
if (!$bind_results) {
throw new \Exception("Unable to bind as user");
}
return array_change_key_case($user);
} catch (\Exception $e) {
\Log::debug("Exception on fast find-and-bind: " . $e->getMessage());
if ($slow_failure) {
sleep($slow_failure);
}
return false; //TODO - make this null instead for a slightly nicer type signature
} finally {
if ($admin_conn) {
ldap_unbind($admin_conn);
}
if ($connection) {
ldap_unbind($connection);
}
}
}
/**
* Binds/authenticates the user to LDAP, and returns their attributes.
@@ -147,6 +219,17 @@ class Ldap extends Model
Log::debug('Filter query: '.$filterQuery);
// only try this if we have an Admin username set; otherwise use the 'legacy' method
if ($settings->ldap_uname) {
// in the fallowing call, we pick a slow-failure of 0 because we might need to fall through to 'legacy'
$fast_bind = self::findAndBindMultiOU($baseDn, $filterQuery, $password, 0);
if ($fast_bind) {
\Log::debug("Fast bind worked");
return $fast_bind;
}
\Log::debug("Fast bind failed; falling through to legacy bind");
}
if (! $ldapbind = @ldap_bind($connection, $userDn, $password)) {
Log::debug("Status of binding user: $userDn to directory: (directly!) ".($ldapbind ? "success" : "FAILURE"));
if (! $ldapbind = self::bindAdminToLdap($connection)) {
@@ -154,11 +237,11 @@ class Ldap extends Model
* TODO PLEASE:
*
* this isn't very clear, so it's important to note: the $ldapbind value is never correctly returned - we never 'return true' from self::bindAdminToLdap() (the function
* just "falls off the end" without ever explictly returning 'true')
* just "falls off the end" without ever explicitly returning 'true')
*
* but it *does* have an interesting side-effect of checking for the LDAP password being incorrectly encrypted with the wrong APP_KEY, so I'm leaving it in for now.
*
* If it *did* correctly return 'true' on a succesful bind, it would _probably_ allow users to log in with an incorrect password. Which would be horrible!
* If it *did* correctly return 'true' on a successful bind, it would _probably_ allow users to log in with an incorrect password. Which would be horrible!
*
* Let's definitely fix this at the next refactor!!!!
*