From a07d83e58376913c122bed87e4be307ad2f517bc Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Tue, 9 Sep 2025 15:02:50 +0100 Subject: [PATCH 1/2] Improved find-and-bind for complex LDAP directory structures --- app/Models/Ldap.php | 87 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 85 insertions(+), 2 deletions(-) diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php index 168da8b571..e583cbf6a2 100644 --- a/app/Models/Ldap.php +++ b/app/Models/Ldap.php @@ -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!!!! * From 598612d4bfbfa2df6b653d907d36b7cfb4570426 Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Thu, 16 Oct 2025 19:03:24 +0100 Subject: [PATCH 2/2] Got tests to pass? Not very happy about it, tbh. --- tests/Unit/LdapTest.php | 47 +++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/tests/Unit/LdapTest.php b/tests/Unit/LdapTest.php index 3f01160976..358c97e96f 100644 --- a/tests/Unit/LdapTest.php +++ b/tests/Unit/LdapTest.php @@ -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");