Improved find-and-bind for complex LDAP directory structures

Author: uberbrady

app/Models/Ldap.php

@@ -104,6 +104,78 @@ public static function connectToLdap()
         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,18 +219,29 @@ public static function findAndBindUserLdap($username, $password)
 
         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)) {
                 /*
                  * 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!!!!
                  *