From 9bf1cc8ce4de6d4e13ce5bf060587ebe20e1096b Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 17 Sep 2024 16:14:17 +0200 Subject: [PATCH 1/5] cleanup: cleanup of unreachable code Either we would have bound the connection thanks to auto_bind, or we would have had an unhandled error. Due to this, we can remove this. --- ldapauthenticator/ldapauthenticator.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 97adcc9..8c41b52 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -335,11 +335,6 @@ def resolve_username(self, username_supplied_by_user): userdn=self.lookup_dn_search_user, password=self.lookup_dn_search_password, ) - if not conn.bind(): - self.log.warning( - f"Failed to connect to LDAP server with search user '{self.lookup_dn_search_user}'" - ) - return (None, None) search_filter = self.lookup_dn_search_filter.format( login_attr=self.user_attribute, From a57aaf804e23761655b9d6f95e1c5bcdc023e4ef Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 17 Sep 2024 16:51:51 +0200 Subject: [PATCH 2/5] Fix duplicated bind operation, only one is needed --- ldapauthenticator/ldapauthenticator.py | 66 +++++++++++++++----------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 8c41b52..7715e26 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -4,6 +4,7 @@ import ldap3 from jupyterhub.auth import Authenticator +from ldap3.core.exceptions import LDAPBindError from ldap3.utils.conv import escape_filter_chars from ldap3.utils.dn import escape_rdn from traitlets import Bool, Int, List, Unicode, Union, UseEnum, observe, validate @@ -335,6 +336,8 @@ def resolve_username(self, username_supplied_by_user): userdn=self.lookup_dn_search_user, password=self.lookup_dn_search_password, ) + if not conn: + return (None, None) search_filter = self.lookup_dn_search_filter.format( login_attr=self.user_attribute, @@ -379,9 +382,14 @@ def resolve_username(self, username_supplied_by_user): def get_connection(self, userdn, password): """ - Returns a ldap3 Connection object automatically bound to the user. + Returns either a ldap3 Connection object automatically bound to the + user, or None if the bind operation failed for some reason. + + Raises errors on connectivity or TLS issues. - ldap3 Connection ref: https://ldap3.readthedocs.io/en/latest/connection.html + ldap3 Connection ref: + - docs: https://ldap3.readthedocs.io/en/latest/connection.html + - code: https://github.com/cannatag/ldap3/blob/dev/ldap3/core/connection.py """ if self.tls_strategy == TlsStrategy.on_connect: use_ssl = True @@ -398,13 +406,26 @@ def get_connection(self, userdn, password): port=self.server_port, use_ssl=use_ssl, ) - conn = ldap3.Connection( - server, - user=userdn, - password=password, - auto_bind=auto_bind, - ) - return conn + try: + self.log.debug(f"Attempting to bind {userdn}") + conn = ldap3.Connection( + server, + user=userdn, + password=password, + auto_bind=auto_bind, + ) + except LDAPBindError as e: + self.log.warning( + "Failed to bind {userdn}\n{e_type}: {e_msg}".format( + userdn=userdn, + e_type=e.__class__.__name__, + e_msg=e.args[0] if e.args else "", + ) + ) + return None + else: + self.log.debug(f"Successfully bound {userdn}") + return conn def get_user_attributes(self, conn, userdn): attrs = {} @@ -460,28 +481,17 @@ async def authenticate(self, handler, data): if not bind_dn_template: bind_dn_template = [resolved_dn] - is_bound = False + # bind to ldap user + conn = None for dn in bind_dn_template: + # DN's attribute values should be escaped with escape_rdn to respect + # https://datatracker.ietf.org/doc/html/rfc4514#section-2.4 userdn = dn.format(username=escape_rdn(username)) - self.log.debug(f"Attempting to bind {username} with {userdn}") - msg = "Status of user bind {username} with {userdn} : {is_bound}" - try: - conn = self.get_connection(userdn, password) - except ldap3.core.exceptions.LDAPBindError as exc: - is_bound = False - msg += "\n{exc_type}: {exc_msg}".format( - exc_type=exc.__class__.__name__, - exc_msg=exc.args[0] if exc.args else "", - ) - else: - is_bound = True if conn.bound else conn.bind() - msg = msg.format(username=username, userdn=userdn, is_bound=is_bound) - self.log.debug(msg) - if is_bound: + conn = self.get_connection(userdn, password) + if conn: break - - if not is_bound: - self.log.warning(f"Invalid password for user '{username}'") + if not conn: + self.log.warning(f"Failed to bind user '{username}' to a LDAP user.") return None if self.search_filter: From 5c5ccbeae2d8a7aaf2595f023246c74a6c5232ea Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Thu, 19 Sep 2024 09:30:30 +0200 Subject: [PATCH 3/5] Use non-deprecated log.warning instead of log.warn --- ldapauthenticator/ldapauthenticator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 7715e26..05f2bdd 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -370,7 +370,7 @@ def resolve_username(self, username_supplied_by_user): elif len(user_dn) == 1: user_dn = user_dn[0] else: - self.log.warn( + self.log.warning( f"A lookup of the username '{username_supplied_by_user}' returned a list " f"of entries for the attribute '{self.lookup_dn_user_dn_attribute}'. Only " f"the first among these ('{user_dn[0]}') was used. The other entries " From 406b7236129b357141ca4b9f7655751357f215a6 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Thu, 19 Sep 2024 09:44:10 +0200 Subject: [PATCH 4/5] Fix grammar --- ldapauthenticator/ldapauthenticator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 05f2bdd..c8575d9 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -382,7 +382,7 @@ def resolve_username(self, username_supplied_by_user): def get_connection(self, userdn, password): """ - Returns either a ldap3 Connection object automatically bound to the + Returns either an ldap3 Connection object automatically bound to the user, or None if the bind operation failed for some reason. Raises errors on connectivity or TLS issues. @@ -491,7 +491,7 @@ async def authenticate(self, handler, data): if conn: break if not conn: - self.log.warning(f"Failed to bind user '{username}' to a LDAP user.") + self.log.warning(f"Failed to bind user '{username}' to an LDAP user.") return None if self.search_filter: From 1ba6e0fe58a8461652a08870b5e446ca1fb04b8d Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Thu, 19 Sep 2024 09:45:00 +0200 Subject: [PATCH 5/5] Fix logging details --- ldapauthenticator/ldapauthenticator.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index c8575d9..f40df16 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -337,6 +337,9 @@ def resolve_username(self, username_supplied_by_user): password=self.lookup_dn_search_password, ) if not conn: + self.log.error( + f"Failed to bind lookup_dn_search_user '{self.lookup_dn_search_user}'" + ) return (None, None) search_filter = self.lookup_dn_search_filter.format( @@ -415,7 +418,7 @@ def get_connection(self, userdn, password): auto_bind=auto_bind, ) except LDAPBindError as e: - self.log.warning( + self.log.debug( "Failed to bind {userdn}\n{e_type}: {e_msg}".format( userdn=userdn, e_type=e.__class__.__name__,