Escape username within DN correctly and remove `escape_userdn`

consideRatio commented 2 months ago

Background

Why `escape_userdn` was introduced

escape_userdn was introduced after a non-conclusive discussion in https://github.com/jupyterhub/ldapauthenticator/pull/32#pullrequestreview-26048588, 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 #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".

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.

jupyterhub / ldapauthenticator