Escape username within DN correctly and remove `escape_userdn` #267

consideRatio · 2024-09-16T21:02:34Z

Background

Why `escape_userdn` was introduced

escape_userdn was introduced after a non-conclusive discussion in #32 (review), where it made things break for some, and fixed things for others.

I think that was a fluke related to other issues, such as using the userdn variable both in a binding operation and in other operations where it was part of a search filter. When a DN is part of a search filter as a value to find, it should be escaped with escape_filter_chars. With #238, we are good at using escape_filter_chars just-in-time where its needed.

Prevent LDAP injections and the `escape_rdn` function

The config valid_username_regex was this projects key defence for LDAP injection attacks, but since it was introduced there is now a escape_rdn function to help escaping usernames given to be part of a DN via bind_dn_template strings. This is stressed as important in the ldap3 Python package's docs:

What should be escaped?

A DN represented as a string should have its attribute values escaped in a certain way (escape_rdn).
This is described in https://datatracker.ietf.org/doc/html/rfc4514#section-2.4. Escaped characters are \,+"<>;= (and null).
Search filters matching against certain values, should have the values escaped in a certain way (escape_filter_chars).
This is described in https://datatracker.ietf.org/doc/html/rfc4515#section-3. Escaped characters are /()* (and null).
Recent fixes related to this are in Escape user- or ldap-provided strings in ldap search filters #238.

PR changes

escape_userdn is made to no longer have any effect and logs a warning if set
escape_rdn is applied on a username string when constructing DN(s) from bind_dn_template.
Removed a section that escaped , in a username, which was just a piece of what escape_rdn escapes.
Misc docs are updated

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".

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.

consideRatio · 2024-09-17T13:38:49Z

(Rebased on main)

consideRatio · 2024-09-17T13:39:30Z

Going for a merge and aiming for this to be available to test practically in a beta release

Nikolai-Hlubek · 2024-09-25T10:51:00Z

ldapauthenticator/ldapauthenticator.py

@@ -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]


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?

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?

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

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}

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".

Nikolai-Hlubek · 2024-09-25T10:53:58Z

ldapauthenticator/ldapauthenticator.py

-            userdn = dn.format(username=username)
-            if self.escape_userdn:
-                userdn = escape_filter_chars(userdn)
+            userdn = dn.format(username=escape_rdn(username))


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?

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

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

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

Nikolai-Hlubek · 2024-09-25T10:55:38Z

@consideRatio Sorry for the late reply.
I don't know about LDAP so I can only check the logic of the changes. I noticed two things which I wrote comments about, in the commit.

m-erhardt · 2024-10-10T15:31:41Z

I know, I'm a little late to the party.
Unfortunatley I've been quite busy for the past weeks and had no capacity to dig into this.

As more testing/validation certainly wont hurt before the release of 2.0.0 I've just validated this change by testing the current version from the main-branch (0c65788) on our production system against our ActiveDirectory with the following (unchanged) config.

c.JupyterHub.authenticator_class = 'ldapauthenticator.LDAPAuthenticator'

c.LDAPAuthenticator.server_address = 'our.companys.ldap.server'
c.LDAPAuthenticator.server_port = 636
c.LDAPAuthenticator.use_ssl = True

c.LDAPAuthenticator.lookup_dn = True
c.LDAPAuthenticator.lookup_dn_search_filter = '({login_attr}={login})'
c.LDAPAuthenticator.lookup_dn_search_user = 'CN=jupyter.bind,OU=Users,DC=our,DC=company'
c.LDAPAuthenticator.lookup_dn_search_password = '***'
c.LDAPAuthenticator.user_search_base = 'DC=our,DC=company'
c.LDAPAuthenticator.user_attribute = 'sAMAccountName'
c.LDAPAuthenticator.lookup_dn_user_dn_attribute = 'cn'
c.LDAPAuthenticator.escape_userdn = False
c.LDAPAuthenticator.use_lookup_dn_username = False

c.LDAPAuthenticator.allowed_groups = [
    "CN=JupyterHubGroup,OU=Groups,DC=our,DC=company",
]

I've also validated that the issue described in #237 is still fixed (which is the case).

So thanks @consideRatio for decluttering the whole escape_userdn/escape_rdn issue and improving the documentation.

So for most users using ldapauthenticator with ActiveDirectory this should not be a breaking change.

consideRatio force-pushed the pr/remove-escape-userdn branch 2 times, most recently from 4f7bd11 to 242ff07 Compare September 16, 2024 21:26

consideRatio changed the title ~~Escape DN attribute values according to LDAP spec and remove escape_userdn~~ Escape DN attribute values according to LDAP spec and remove escape_userdn Sep 16, 2024

consideRatio force-pushed the pr/remove-escape-userdn branch from 242ff07 to 1c254c3 Compare September 16, 2024 21:29

consideRatio changed the title ~~Escape DN attribute values according to LDAP spec and remove escape_userdn~~ Escape username within DN correctly and remove escape_userdn Sep 16, 2024

consideRatio added the bug label Sep 16, 2024

This was referenced Sep 16, 2024

Fix escaping issue when escape_userdn and allowed_groups are configured #253

Closed

Fix for escape_userdn #226

Closed

Coordination about escaping issues and fixes #243

Closed

consideRatio force-pushed the pr/remove-escape-userdn branch from 1c254c3 to 0cfd040 Compare September 16, 2024 22:16

consideRatio mentioned this pull request Sep 16, 2024

Release plan for 2.0.0 #220

Closed

12 tasks

minrk approved these changes Sep 17, 2024

View reviewed changes

consideRatio commented Sep 17, 2024

View reviewed changes

consideRatio closed this Sep 17, 2024

consideRatio reopened this Sep 17, 2024

Escape username within DN correctly and remove escape_userdn

fee33e2

consideRatio force-pushed the pr/remove-escape-userdn branch from 0cfd040 to fee33e2 Compare September 17, 2024 13:38

consideRatio merged commit fde899f into jupyterhub:main Sep 17, 2024
5 checks passed

consideRatio mentioned this pull request Sep 17, 2024

Escaping commas from CN #139

Closed

Nikolai-Hlubek reviewed Sep 25, 2024

View reviewed changes

consideRatio added the breaking label Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Escape username within DN correctly and remove `escape_userdn` #267

Escape username within DN correctly and remove `escape_userdn` #267

consideRatio commented Sep 16, 2024 •

edited

Loading

consideRatio commented Sep 16, 2024

minrk left a comment

consideRatio Sep 17, 2024 •

edited

Loading

consideRatio commented Sep 17, 2024

consideRatio commented Sep 17, 2024

Nikolai-Hlubek Sep 25, 2024

consideRatio Sep 25, 2024

Nikolai-Hlubek Sep 25, 2024

consideRatio Sep 25, 2024 •

edited

Loading

consideRatio Sep 26, 2024

Nikolai-Hlubek Sep 25, 2024

consideRatio Sep 25, 2024

consideRatio Sep 25, 2024

Nikolai-Hlubek Sep 25, 2024

Nikolai-Hlubek commented Sep 25, 2024

m-erhardt commented Oct 10, 2024

                       conn = self.get_connection(
-                          userdn=search_dn,
+                          userdn=self.lookup_dn_search_user,

	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}

Escape username within DN correctly and remove escape_userdn #267

Escape username within DN correctly and remove escape_userdn #267

Conversation

consideRatio commented Sep 16, 2024 • edited Loading

Background

Why escape_userdn was introduced

Prevent LDAP injections and the escape_rdn function

What should be escaped?

PR changes

EDIT: Breaking change

Related

consideRatio commented Sep 16, 2024

minrk left a comment

Choose a reason for hiding this comment

consideRatio Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

consideRatio commented Sep 17, 2024

consideRatio commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

consideRatio Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EDIT: Breaking change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nikolai-Hlubek commented Sep 25, 2024

m-erhardt commented Oct 10, 2024

Escape username within DN correctly and remove `escape_userdn` #267

Escape username within DN correctly and remove `escape_userdn` #267

consideRatio commented Sep 16, 2024 •

edited

Loading

Why `escape_userdn` was introduced

Prevent LDAP injections and the `escape_rdn` function

consideRatio Sep 17, 2024 •

edited

Loading

consideRatio Sep 25, 2024 •

edited

Loading