Harden LDAP authentication against empty credentials

src/authentication/rfl/authentication/ldap.py

@@ -4,20 +4,19 @@
 #
 # SPDX-License-Identifier: LGPL-3.0-or-later
 
-from typing import Optional, List, Tuple
-from pathlib import Path
 import logging
 import os
+from pathlib import Path
+from typing import List, Optional, Tuple
 
 try:
     import ldap
     import ldap.filter
 except ImportError as err:
     raise ImportError("python-ldap is required for RFL LDAP Authentication") from err
 
-from .user import AuthenticatedUser
 from .errors import LDAPAuthenticationError
-
+from .user import AuthenticatedUser
 
 logger = logging.getLogger(__name__)
 
@@ -367,8 +366,16 @@ def login(self, user: str, password: str) -> AuthenticatedUser:
         and the user in not member of any of these groups."""
         fullname = None
         groups = None
-        if user is None or password is None:
+
+        # Reject missing, non-string or empty credentials before any LDAP
+        # operation. An empty password in particular must never reach
+        # simple_bind_s(): per RFC 4513 section 5.1.2 it triggers an
+        # "unauthenticated bind" that some servers accept as a success,
+        # allowing authentication as any existing user.
+        if not isinstance(user, str) or not isinstance(password, str):
             raise LDAPAuthenticationError("Invalid authentication request")
+        if not user.strip() or not password:
+            raise LDAPAuthenticationError("Invalid user or password")
 
         # Lookup user DN in user base.
         user_dn = self._lookup_user_dn(user)

src/authentication/rfl/tests/test_ldap.py

@@ -5,17 +5,16 @@
 # SPDX-License-Identifier: LGPL-3.0-or-later
 
 import os
+import ssl
 import unittest
-from unittest.mock import patch, Mock
-from pathlib import Path
 import urllib
-import ssl
+from pathlib import Path
+from unittest.mock import Mock, patch
 
 import ldap
 import ldap.filter
-
-from rfl.authentication.ldap import LDAPAuthentifier
 from rfl.authentication.errors import LDAPAuthenticationError
+from rfl.authentication.ldap import LDAPAuthentifier
 
 
 class MockLDAPObject:
@@ -387,12 +386,75 @@ def test_login_not_in_restricted_group(
         ):
             self.authentifier.login("john", "SECR3T")
 
-    def test_login_missing_user_or_password(self):
+    @patch.object(LDAPAuthentifier, "_lookup_user_dn")
+    def test_login_missing_user_or_password(self, mock_lookup_user_dn):
+        for user, password in [
+            ("john", None),
+            (None, "SECR3T"),
+            (None, None),
+        ]:
+            with self.assertRaisesRegex(
+                LDAPAuthenticationError, "^Invalid authentication request$"
+            ):
+                self.authentifier.login(user, password)
+
+        mock_lookup_user_dn.assert_not_called()
+
+    @patch.object(LDAPAuthentifier, "_lookup_user_dn")
+    def test_login_non_string_credentials(self, mock_lookup_user_dn):
+        for user, password in [
+            (["john"], "SECR3T"),
+            ("john", 1234),
+            (42, 42),
+            ({}, "SECR3T"),
+        ]:
+            with self.assertRaisesRegex(
+                LDAPAuthenticationError, "^Invalid authentication request$"
+            ):
+                self.authentifier.login(user, password)
+
+        mock_lookup_user_dn.assert_not_called()
+
+    @patch.object(ldap.ldapobject.LDAPObject, "simple_bind_s")
+    @patch.object(LDAPAuthentifier, "_lookup_user_dn")
+    def test_login_empty_password_rejected(
+        self, mock_lookup_user_dn, mock_simple_bind_s
+    ):
+        mock_lookup_user_dn.return_value = "uid=john,ou=people,dc=corp,dc=org"
         with self.assertRaisesRegex(
-            LDAPAuthenticationError, "Invalid authentication request"
+            LDAPAuthenticationError, "^Invalid user or password$"
         ):
-            self.authentifier.login("john", None)
-            self.authentifier.login(None, "SECR3T")
+            self.authentifier.login("john", "")
+
+        mock_simple_bind_s.assert_not_called()
+
+    @patch.object(ldap.ldapobject.LDAPObject, "simple_bind_s")
+    @patch.object(LDAPAuthentifier, "_lookup_user_dn")
+    def test_login_empty_or_blank_user_rejected(
+        self, mock_lookup_user_dn, mock_simple_bind_s
+    ):
+        mock_lookup_user_dn.return_value = "uid=john,ou=people,dc=corp,dc=org"
+        for user in ["", "   "]:
+            with self.assertRaisesRegex(
+                LDAPAuthenticationError, "^Invalid user or password$"
+            ):
+                self.authentifier.login(user, "SECR3T")
+
+        mock_simple_bind_s.assert_not_called()
+
+    @patch.object(ldap.ldapobject.LDAPObject, "simple_bind_s")
+    @patch.object(LDAPAuthentifier, "_lookup_user_dn")
+    def test_login_password_with_spaces_allowed(
+        self, mock_lookup_user_dn, mock_simple_bind_s
+    ):
+        mock_lookup_user_dn.return_value = "uid=john,ou=people,dc=corp,dc=org"
+        try:
+            self.authentifier.login("john", " s p a c e ")
+        except LDAPAuthenticationError as err:
+            self.assertNotIn("Invalid user or password", str(err))
+            self.assertNotIn("Invalid authentication request", str(err))
+
+        mock_simple_bind_s.assert_called_once()
 
     @patch.object(LDAPAuthentifier, "_lookup_user_dn")
     @patch.object(ldap.ldapobject.LDAPObject, "simple_bind_s")