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

Merge pull request #18058 from uberbrady/ldap_fetch_dn_before_login_FIXED

Browse Source 
FIXED - perform Ldap fetch for dn (Distinguished Name) before logging-in (fixed)
This commit is contained in:
snipe
2025-10-21 12:56:31 +01:00
committed by GitHub
parent 07f0e8a3be 598612d4bf
commit 485d343e0f
2 changed files with 118 additions and 16 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!!!!
*
tests/Unit/LdapTest.php
+33 -14
View File
@@ -81,18 +81,22 @@ class LdapTest extends TestCase
$this->settings->enableLdap();
$ldap_connect = $this->getFunctionMock("App\\Models", "ldap_connect");
$ldap_connect->expects($this->once())->willReturn('hello');
$ldap_connect->expects($this->exactly(3))->willReturn('hello');
$ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
$ldap_set_option->expects($this->exactly(4));
$ldap_set_option->expects($this->exactly(12));
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->once())->willReturn(true);
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(2))->willReturn(true);
$this->getFunctionMock("App\\Models", "ldap_search")->expects($this->once())->willReturn(true);
$this->getFunctionMock("App\\Models", "ldap_search")->expects($this->exactly(1))->willReturn(true);
$this->getFunctionMock("App\\Models", "ldap_first_entry")->expects($this->once())->willReturn(true);
$this->getFunctionMock("App\\Models", "ldap_first_entry")->expects($this->exactly(1))->willReturn(true);
$this->getFunctionMock("App\\Models", "ldap_unbind")->expects($this->exactly(2));
$this->getFunctionMock("App\\Models", "ldap_count_entries")->expects($this->once())->willReturn(1);
$this->getFunctionMock("App\\Models", "ldap_get_dn")->expects($this->once())->willReturn('dn=FirstName Surname,ou=Org,dc=example,dc=com');
$this->getFunctionMock("App\\Models", "ldap_get_attributes")->expects($this->once())->willReturn(
$this->getFunctionMock("App\\Models", "ldap_get_attributes")->expects($this->exactly(1))->willReturn(
[
"count" => 1,
0 => [
@@ -111,13 +115,25 @@ class LdapTest extends TestCase
$this->settings->enableLdap();
$ldap_connect = $this->getFunctionMock("App\\Models", "ldap_connect");
$ldap_connect->expects($this->once())->willReturn('hello');
$ldap_connect->expects($this->exactly(3))->willReturn('hello');
$ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
$ldap_set_option->expects($this->exactly(4));
$ldap_set_option->expects($this->exactly(12));
// note - we return FALSE first, to simulate a bad-bind, then TRUE the second time to simulate a successful admin bind
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(2))->willReturn(false, true);
//
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(4))->willReturn(
true, /* initial admin connection for 'fast path' */
false, /* the actual login for the user */
false, /* the direct login for the user binding-as-themselves in the legacy path */
true /* the admin login afterwards (which is weird and doesn't make sense) */
);
$this->getFunctionMock("App\\Models", "ldap_unbind")->expects($this->exactly(2));
$this->getFunctionMock("App\\Models", "ldap_error")->expects($this->never())->willReturn("exception");
$this->getFunctionMock("App\\Models", "ldap_search")->expects($this->exactly(1))->willReturn(false); //uhm?
$this->getFunctionMock("App\\Models", "ldap_count_entries")->expects($this->exactly(1))->willReturn(1);
$this->getFunctionMock("App\\Models", "ldap_first_entry")->expects($this->exactly(1))->willReturn(true);
$this->getFunctionMock("App\\Models", "ldap_get_attributes")->expects($this->exactly(1))->willReturn(1);
$this->getFunctionMock("App\\Models", "ldap_get_dn")->expects($this->exactly(1))->willReturn('dn=FirstName Surname,ou=Org,dc=example,dc=com');
// $this->getFunctionMock("App\\Models","ldap_error")->expects($this->once())->willReturn("exception");
@@ -132,14 +148,17 @@ class LdapTest extends TestCase
$this->settings->enableLdap();
$ldap_connect = $this->getFunctionMock("App\\Models", "ldap_connect");
$ldap_connect->expects($this->once())->willReturn('hello');
$ldap_connect->expects($this->exactly(2))->willReturn('hello');
$ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
$ldap_set_option->expects($this->exactly(4));
$ldap_set_option->expects($this->exactly(8));
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->once())->willReturn(true);
$this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(2))->willReturn(true); //I think this is OK
$this->getFunctionMock("App\\Models", "ldap_search")->expects($this->exactly(2))->willReturn(false); //uhm?
$this->getFunctionMock("App\\Models", "ldap_unbind")->expects($this->once());
$this->getFunctionMock("App\\Models", "ldap_count_entries")->expects($this->once())->willReturn(0);
$this->getFunctionMock("App\\Models", "ldap_search")->expects($this->once())->willReturn(false);
$this->expectExceptionMessage("Could not search LDAP:");
$results = Ldap::findAndBindUserLdap("username","password");