diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b17111..4d2a2f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [unreleased] + +### Fixed + +- auth: Reject missing, non-string, blank usernames and empty passwords in + `LDAPAuthentifier.login()` before any LDAP operation, so an empty password + cannot succeed as an unauthenticated bind (RFC 4513). Contribution from + @AugustoMagalhaes. + ## [1.9.0] - 2026-07-09 ### Added diff --git a/src/authentication/rfl/authentication/ldap.py b/src/authentication/rfl/authentication/ldap.py index c022ca0..d958514 100644 --- a/src/authentication/rfl/authentication/ldap.py +++ b/src/authentication/rfl/authentication/ldap.py @@ -4,10 +4,10 @@ # # 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 @@ -15,9 +15,8 @@ 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) diff --git a/src/authentication/rfl/tests/test_ldap.py b/src/authentication/rfl/tests/test_ldap.py index d11ee93..41ba1df 100644 --- a/src/authentication/rfl/tests/test_ldap.py +++ b/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: @@ -367,6 +366,24 @@ def test_login_ok( self.assertEqual(user.fullname, "John Doe") self.assertEqual(user.groups, ["group1", "group2"]) + @patch.object(LDAPAuthentifier, "_lookup_user_dn") + @patch.object(LDAPAuthentifier, "_get_user_info") + @patch.object(LDAPAuthentifier, "_get_groups") + @patch("rfl.authentication.ldap.ldap") + def test_login_ok_with_spaces_in_password( + self, mock_ldap, mock_get_groups, mock_get_user_info, mock_lookup_user_dn + ): + mock_get_groups.return_value = ["group1", "group2"] + mock_get_user_info.return_value = ("John Doe", 42) + mock_lookup_user_dn.return_value = "uid=john,ou=people,dc=corp,dc=org" + mock_ldap_object = mock_ldap.initialize.return_value + + self.authentifier.login("john", " s p a c e ") + + mock_ldap_object.simple_bind_s.assert_called_once_with( + "uid=john,ou=people,dc=corp,dc=org", " s p a c e " + ) + @patch.object(LDAPAuthentifier, "_lookup_user_dn") @patch.object(LDAPAuthentifier, "_get_user_info") @patch.object(LDAPAuthentifier, "_get_groups") @@ -387,12 +404,61 @@ 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(LDAPAuthentifier, "_lookup_user_dn") @patch.object(ldap.ldapobject.LDAPObject, "simple_bind_s")