From fee33e29bab6d31be9c3ad38d6ebfded124c47bb Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Mon, 16 Sep 2024 23:02:05 +0200 Subject: [PATCH] Escape username within DN correctly and remove `escape_userdn` --- README.md | 7 ---- ldapauthenticator/ldapauthenticator.py | 46 +++++++++++--------------- ldapauthenticator/tests/conftest.py | 1 - 3 files changed, 20 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index ca96f12..bc8d54a 100644 --- a/README.md +++ b/README.md @@ -235,12 +235,6 @@ Attribute containing user's name needed for building DN string, if `lookup_dn` i See `user_search_base` for info on how this attribute is used. For most LDAP servers, this is username. For Active Directory, it is cn. -#### `LDAPAuthenticator.escape_userdn` - -If set to True, escape special chars in userdn when authenticating in LDAP. -On some LDAP servers, when userdn contains chars like '(', ')', '\' authentication may fail when those chars -are not escaped. - #### `LDAPAuthenticator.auth_state_attributes` An optional list of attributes to be fetched for a user after login. @@ -274,7 +268,6 @@ c.LDAPAuthenticator.lookup_dn_search_password = 'secret' c.LDAPAuthenticator.user_search_base = 'ou=people,dc=wikimedia,dc=org' c.LDAPAuthenticator.user_attribute = 'sAMAccountName' c.LDAPAuthenticator.lookup_dn_user_dn_attribute = 'cn' -c.LDAPAuthenticator.escape_userdn = False ``` In setup above, first LDAP will be searched (with account ldap_search_user_technical_account) for users that have sAMAccountName=login diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 7326228..97adcc9 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -5,6 +5,7 @@ import ldap3 from jupyterhub.auth import Authenticator 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 @@ -166,20 +167,17 @@ def _validate_bind_dn_template(self, proposal): help="List of attributes to be searched", ) - # FIXME: Use something other than this? THIS IS LAME, akin to websites restricting things you - # can use in usernames / passwords to protect from SQL injection! valid_username_regex = Unicode( r"^[a-z][.a-z0-9_-]*$", config=True, help=""" - Regex for validating usernames - those that do not match this regex will be rejected. - - This is primarily used as a measure against LDAP injection, which has fatal security - considerations. The default works for most LDAP installations, but some users might need - to modify it to fit their custom installs. If you are modifying it, be sure to understand - the implications of allowing additional characters in usernames and what that means for - LDAP injection issues. See https://www.owasp.org/index.php/LDAP_injection for an overview - of LDAP injection. + Regex for validating usernames - those that do not match this regex will + be rejected. + + This config was primarily introduced to prevent LDAP injection + (https://www.owasp.org/index.php/LDAP_injection), but that is since 2.0 + being mitigated by escaping all sensitive characters when interacting + with the LDAP server. """, ) @@ -250,7 +248,8 @@ def _validate_bind_dn_template(self, proposal): default_value=None, allow_none=True, help=""" - Technical account for user lookup, if `lookup_dn` is set to True. + DN for a technical user account allowed to search for information about + provided username, if `lookup_dn` is set to True. If both lookup_dn_search_user and lookup_dn_search_password are None, then anonymous LDAP query will be done. """, @@ -282,13 +281,16 @@ def _validate_bind_dn_template(self, proposal): False, config=True, help=""" - If set to True, escape special chars in userdn when authenticating in LDAP. - - On some LDAP servers, when userdn contains chars like '(', ')', '\' authentication may fail when those chars - are not escaped. + Removed in 2.0, configuring this no longer has any effect. """, ) + @observe("escape_userdn") + def _observe_escape_userdn(self, change): + self.log.warning( + "LDAPAuthenticator.escape_userdn was removed in 2.0 and no longer has any effect." + ) + search_filter = Unicode( config=True, help=""" @@ -329,16 +331,13 @@ def resolve_username(self, username_supplied_by_user): Resolves a username supplied by a user to the a user DN when lookup_dn is True. """ - search_dn = self.lookup_dn_search_user - if self.escape_userdn: - search_dn = escape_filter_chars(search_dn) conn = self.get_connection( - userdn=search_dn, + 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 '{search_dn}'" + f"Failed to connect to LDAP server with search user '{self.lookup_dn_search_user}'" ) return (None, None) @@ -463,17 +462,12 @@ async def authenticate(self, handler, data): username, resolved_dn = self.resolve_username(username) if not username: return None - if str(self.lookup_dn_user_dn_attribute).upper() == "CN": - # Only escape commas if the lookup attribute is CN - username = re.subn(r"([^\\]),", r"\1\,", username)[0] if not bind_dn_template: bind_dn_template = [resolved_dn] is_bound = False for dn in bind_dn_template: - userdn = dn.format(username=username) - if self.escape_userdn: - userdn = escape_filter_chars(userdn) + 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: diff --git a/ldapauthenticator/tests/conftest.py b/ldapauthenticator/tests/conftest.py index 5204a35..c59c62c 100644 --- a/ldapauthenticator/tests/conftest.py +++ b/ldapauthenticator/tests/conftest.py @@ -14,7 +14,6 @@ def authenticator(): authenticator.user_search_base = "ou=people,dc=planetexpress,dc=com" authenticator.user_attribute = "uid" authenticator.lookup_dn_user_dn_attribute = "cn" - authenticator.escape_userdn = True authenticator.attributes = ["uid", "cn", "mail", "ou"] authenticator.use_lookup_dn_username = False