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.

Sign up for GitHub

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

Jump to bottom

Escape username within DN correctly and remove escape_userdn #267

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Escape username within DN correctly and remove escape_userdn #267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 0 additions & 7 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
46 changes: 20 additions & 26 deletions ldapauthenticator/ldapauthenticator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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.
""",
)

Expand Down Expand Up @@ -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.
""",
Expand Down Expand Up @@ -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="""
Expand Down Expand Up @@ -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,
Copy link
Member Author

@consideRatio consideRatio Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note that the DN here isn't escaped with escape_rdn, because its a full DN rather than a part of it. The full DN include commas and equalsigns for example, and they shouldnt be escaped as they are describing structure of the DN itself, but an individual attributes value should be escaped.

We cant do that here though as we have a full DN here and not its parts.

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)

Expand Down Expand Up @@ -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]
Copy link

Choose a reason for hiding this comment

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

The later escape_rdn is a superset of this comma escape. So probably ok.

However the if ... logic is now missing. Is it important to only do this escape at some times?

Copy link
Member Author

Choose a reason for hiding this comment

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

The if logic didn't make sense to me, the username should reasonably always be escaped before being passed to the LDAP server.

Hmmm, i guess this could also have impacted the username chosen if use_lookup_dn_username is true as well though...

Are you a user of that?

Copy link

Choose a reason for hiding this comment

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

I think for my use case the if condition would always be true as my company uses Active Directory.

Copy link
Member Author

@consideRatio consideRatio Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm, the issue I'm seeing now, is that username is assigned with the escaping, and the username variable will, conditionally on use_lookup_dn_username end up being used to become the JupyterHub username, which otherwise can become whatever the user initially wrote in the login prompt.

This is because of this line of code, where resolved_username seen below was previously named username. In other words, the use_lookup_dn_username can be used or not and it doesn't really matter for anything except the resulting JupyterHub username. One doesn't want to change the username for one person over time typically though, as storage can be associated with the JupterHub username.

ldapauthenticator/ldapauthenticator/ldapauthenticator.py

Lines 642 to 647 in cea8616

username = resolved_username if self.use_lookup_dn_username else login_username
auth_state = {
"ldap_groups": ldap_groups,
"user_attributes": user_attributes,
}
return {"name": username, "auth_state": auth_state}

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the PR description with this:

EDIT: Breaking change

This PR introduced a breaking change in a edge case scenario if both...

  • the following LDAPAuthenticator configuration was used: lookup_dn = True, lookup_dn_user_dn_attribute = "cn", use_lookup_dn_username = True (previous default value)
  • one or more users, that had already signed in at any point in time before, had a comma in the cn attribute's value, as they would if for example the cn attribute values looks like "lastname, firstname"

Then, these users' resulting JupyterHub usernames would be changed from looking like "lastname\\, firstname" to "lastname, firstname".

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))
Copy link

Choose a reason for hiding this comment

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

escape_filter_chars and escape_rdn seem to escape for different things unless I'm missing something.

escape_filter_chars(r"\foo + , (N") -> '\\5cfoo + , \\28N'
escape_rdn(r"\foo + , (N") -> '\\\\foo \\+ \\, (N'

Is it ok to exchange them here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes i think that is right to do, because the rdn escape is for parts of a dn like here. Filter chars wasn't right to use here

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a comment to this in another PR merged after this, linking to an RFC

Copy link

Choose a reason for hiding this comment

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

I guess you are correct. You explained this at the beginning of the PR but first I didn't understand this.

self.log.debug(f"Attempting to bind {username} with {userdn}")
msg = "Status of user bind {username} with {userdn} : {is_bound}"
try:
Expand Down
1 change: 0 additions & 1 deletion ldapauthenticator/tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down