Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix duplicated bind operation, only one is needed #270

Merged
merged 5 commits into from
Sep 19, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 40 additions & 32 deletions ldapauthenticator/ldapauthenticator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -335,9 +336,9 @@ 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}'"
if not conn:
self.log.error(
f"Failed to bind lookup_dn_search_user '{self.lookup_dn_search_user}'"
)
return (None, None)

Expand Down Expand Up @@ -372,7 +373,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 "
Expand All @@ -384,9 +385,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 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.

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
Expand All @@ -403,13 +409,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.debug(
"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 = {}
Expand Down Expand Up @@ -465,28 +484,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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic moved to get_connection, but no longer including logging details about the specific username (but still includes the userdn which should be sufficient).


if not is_bound:
self.log.warning(f"Invalid password for user '{username}'")
Comment on lines -488 to -489
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warning about incorrect password wasn't always correct. It could say incorrect password even though it failed to bind for another reason, such as not finding a user with the userdn or similar.

I think there have been several issues about invalid password that really wasn't about invalid password, but about failing to bind because the user didn't exist - because the get_connection was passed a userdn that wasn't constructed correctly etc.

if not conn:
self.log.warning(f"Failed to bind user '{username}' to an LDAP user.")
return None

if self.search_filter:
Expand Down