From f8a2f75aeb6a0c67cf4e9a248ba889b1168cf9f0 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Sun, 15 Sep 2024 11:55:30 +0200 Subject: [PATCH 1/4] Add tls_strategy and deprecate use_ssl With `tls_strategy` we have three choices on how to secure our connection to the LDAP server. - "on_connect", it is "use_ssl=True" implied - "before_bind", it is what "use_ssl=False" implied - "insecure", this wasn't an option before, but has been requested by users. --- README.md | 32 +++++-- ldapauthenticator/ldapauthenticator.py | 85 ++++++++++++++++--- .../tests/test_ldapauthenticator.py | 21 ++++- 3 files changed, 120 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 4ec50fe..172aeb7 100644 --- a/README.md +++ b/README.md @@ -167,15 +167,35 @@ is what most shell username validators do. #### `LDAPAuthenticator.use_ssl` -Boolean to specify whether to use SSL encryption when contacting -the LDAP server. If it is left to `False` (the default) -`LDAPAuthenticator` will try to upgrade connection with StartTLS. -Set this to be `True` to start SSL connection. +`use_ssl` is deprecated since 2.0. `use_ssl=True` translates to configuring +`tls_strategy="on_connect"`, but `use_ssl=False` (previous default) doesn't +translate to anything. + +#### `LDAPAuthenticator.tls_strategy` + +When LDAPAuthenticator connects to the LDAP server, it can establish a +SSL/TLS connection directly, or do it before binding, which is LDAP +terminology for authenticating and sending sensitive credentials. + +The LDAP v3 protocol deprecated establishing a SSL/TLS connection +directly (`tls_strategy="on_connect"`) in favor of upgrading the +connection to SSL/TLS before binding (`tls_strategy="before_bind"`). + +Supported `tls_strategy` values are: + +- "before_bind" (default) +- "on_connect" (deprecated in LDAP v3, associated with use of port 636) +- "insecure" + +When configuring `tls_strategy="on_connect"`, the default value of +`server_port` becomes 636. #### `LDAPAuthenticator.server_port` -Port to use to contact the LDAP server. Defaults to 389 if no SSL -is being used, and 636 is SSL is being used. +Port on which to contact the LDAP server. + +Defaults to `636` if `tls_strategy="on_connect"` is set, `389` +otherwise. #### `LDAPAuthenticator.user_search_base` diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 140b83b..6200ae4 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -1,9 +1,21 @@ +import enum import re import ldap3 from jupyterhub.auth import Authenticator from ldap3.utils.conv import escape_filter_chars -from traitlets import Bool, Int, List, Unicode, Union, validate +from traitlets import Bool, Int, List, Unicode, Union, UseEnum, observe, validate + + +class TlsStrategy(enum.Enum): + """ + Represents a SSL/TLS strategy for LDAPAuthenticator to use when interacting + with the LDAP server. + """ + + before_bind = 1 + on_connect = 2 + insecure = 3 class LDAPAuthenticator(Authenticator): @@ -20,23 +32,62 @@ class LDAPAuthenticator(Authenticator): help=""" Port on which to contact the LDAP server. - Defaults to `636` if `use_ssl` is set, `389` otherwise. + Defaults to `636` if `tls_strategy="on_connect"` is set, `389` + otherwise. """, ) def _server_port_default(self): - if self.use_ssl: + if self.tls_strategy == TlsStrategy.on_connect: return 636 # default SSL port for LDAP else: return 389 # default plaintext port for LDAP use_ssl = Bool( - False, + None, + allow_none=True, + config=True, + help=""" + `use_ssl` is deprecated since 2.0. `use_ssl=True` translates to configuring + `tls_strategy="on_connect"`, but `use_ssl=False` (previous default) doesn't + translate to anything. + """, + ) + + @observe("use_ssl") + def _observe_use_ssl(self, change): + if change.new: + self.tls_strategy = TlsStrategy.on_connect + self.log.warning( + "LDAPAuthenticator.use_ssl is deprecated in 2.0 in favor of LDAPAuthenticator.tls_strategy, " + 'instead of configuring use_ssl=True, configure use tls_strategy="on_connect" from now on.' + ) + else: + self.log.warning( + "LDAPAuthenticator.use_ssl is deprecated in 2.0 in favor of LDAPAuthenticator.tls_strategy, " + "you can stop configuring use_ssl=False from now on as doing so has no effect." + ) + + tls_strategy = UseEnum( + TlsStrategy, + default_value=TlsStrategy.before_bind, config=True, help=""" - Use SSL to communicate with the LDAP server. + When LDAPAuthenticator connects to the LDAP server, it can establish a + SSL/TLS connection directly, or do it before binding, which is LDAP + terminology for authenticating and sending sensitive credentials. + + The LDAP v3 protocol deprecated establishing a SSL/TLS connection + directly (`tls_strategy="on_connect"`) in favor of upgrading the + connection to SSL/TLS before binding (`tls_strategy="before_bind"`). + + Supported `tls_strategy` values are: + - "before_bind" (default) + - "on_connect" (deprecated in LDAP v3, associated with use of port 636) + - "insecure" - Deprecated in version 3 of LDAP. Your LDAP server must be configured to support this, however. + When configuring `tls_strategy="on_connect"`, the default value of + `server_port` becomes 636. """, ) @@ -314,14 +365,26 @@ def resolve_username(self, username_supplied_by_user): return (user_dn, response[0]["dn"]) def get_connection(self, userdn, password): + if self.tls_strategy == TlsStrategy.on_connect: + use_ssl = True + auto_bind = ldap3.AUTO_BIND_NO_TLS + elif self.tls_strategy == TlsStrategy.before_bind: + use_ssl = False + auto_bind = ldap3.AUTO_BIND_TLS_BEFORE_BIND + else: # TlsStrategy.insecure + use_ssl = False + auto_bind = ldap3.AUTO_BIND_NO_TLS + server = ldap3.Server( - self.server_address, port=self.server_port, use_ssl=self.use_ssl - ) - auto_bind = ( - ldap3.AUTO_BIND_NO_TLS if self.use_ssl else ldap3.AUTO_BIND_TLS_BEFORE_BIND + self.server_address, + port=self.server_port, + use_ssl=use_ssl, ) conn = ldap3.Connection( - server, user=userdn, password=password, auto_bind=auto_bind + server, + user=userdn, + password=password, + auto_bind=auto_bind, ) return conn diff --git a/ldapauthenticator/tests/test_ldapauthenticator.py b/ldapauthenticator/tests/test_ldapauthenticator.py index f8fc057..af2d0ac 100644 --- a/ldapauthenticator/tests/test_ldapauthenticator.py +++ b/ldapauthenticator/tests/test_ldapauthenticator.py @@ -58,7 +58,26 @@ async def test_ldap_auth_blank_template(authenticator): async def test_ldap_auth_ssl(authenticator): authenticator.use_ssl = True - authenticator.server_port = 636 + + # proper username and password in allowed group + authorized = await authenticator.get_authenticated_user( + None, {"username": "fry", "password": "fry"} + ) + assert authorized["name"] == "fry" + + +async def test_ldap_auth_tls_strategy_on_connect(authenticator): + authenticator.tls_strategy = "on_connect" + + # proper username and password in allowed group + authorized = await authenticator.get_authenticated_user( + None, {"username": "fry", "password": "fry"} + ) + assert authorized["name"] == "fry" + + +async def test_ldap_auth_tls_strategy_insecure(authenticator): + authenticator.tls_strategy = "insecure" # proper username and password in allowed group authorized = await authenticator.get_authenticated_user( From f89de634e841c04f273123522b90fb4adc355171 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Sun, 15 Sep 2024 22:32:19 +0200 Subject: [PATCH 2/4] Add docstrings to two tests about tls_strategy --- ldapauthenticator/tests/test_ldapauthenticator.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ldapauthenticator/tests/test_ldapauthenticator.py b/ldapauthenticator/tests/test_ldapauthenticator.py index af2d0ac..3cffee0 100644 --- a/ldapauthenticator/tests/test_ldapauthenticator.py +++ b/ldapauthenticator/tests/test_ldapauthenticator.py @@ -67,6 +67,10 @@ async def test_ldap_auth_ssl(authenticator): async def test_ldap_auth_tls_strategy_on_connect(authenticator): + """ + Verifies basic function of the authenticator with a given tls_strategy + without actually confirming use of that strategy. + """ authenticator.tls_strategy = "on_connect" # proper username and password in allowed group @@ -77,6 +81,10 @@ async def test_ldap_auth_tls_strategy_on_connect(authenticator): async def test_ldap_auth_tls_strategy_insecure(authenticator): + """ + Verifies basic function of the authenticator with a given tls_strategy + without actually confirming use of that strategy. + """ authenticator.tls_strategy = "insecure" # proper username and password in allowed group From d6178bc6a24f64b9afe5ae92a23cff1046d660eb Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Sun, 15 Sep 2024 22:32:59 +0200 Subject: [PATCH 3/4] Adjust use_ssl test to check deprecation logics function --- ldapauthenticator/tests/test_ldapauthenticator.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/ldapauthenticator/tests/test_ldapauthenticator.py b/ldapauthenticator/tests/test_ldapauthenticator.py index 3cffee0..87f5983 100644 --- a/ldapauthenticator/tests/test_ldapauthenticator.py +++ b/ldapauthenticator/tests/test_ldapauthenticator.py @@ -5,6 +5,8 @@ https://github.com/rroemhild/docker-test-openldap?tab=readme-ov-file#ldap-structure """ +from ..ldapauthenticator import TlsStrategy + async def test_ldap_auth_allowed(authenticator): # proper username and password in allowed group @@ -56,14 +58,13 @@ async def test_ldap_auth_blank_template(authenticator): assert authorized["name"] == "fry" -async def test_ldap_auth_ssl(authenticator): - authenticator.use_ssl = True +async def test_ldap_use_ssl_deprecation(authenticator): + assert authenticator.tls_strategy == TlsStrategy.before_bind - # proper username and password in allowed group - authorized = await authenticator.get_authenticated_user( - None, {"username": "fry", "password": "fry"} - ) - assert authorized["name"] == "fry" + # setting use_ssl to True should result in tls_strategy being set to + # on_connect + authenticator.use_ssl = True + assert authenticator.tls_strategy == TlsStrategy.on_connect async def test_ldap_auth_tls_strategy_on_connect(authenticator): From 537d0128c6de5d30cfe49fa4454121e094b0e68b Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Mon, 16 Sep 2024 13:33:56 +0200 Subject: [PATCH 4/4] docs: add get_connection docstring --- ldapauthenticator/ldapauthenticator.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 6200ae4..761ff72 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -365,6 +365,11 @@ def resolve_username(self, username_supplied_by_user): return (user_dn, response[0]["dn"]) def get_connection(self, userdn, password): + """ + Returns a ldap3 Connection object automatically bound to the user. + + ldap3 Connection ref: https://ldap3.readthedocs.io/en/latest/connection.html + """ if self.tls_strategy == TlsStrategy.on_connect: use_ssl = True auto_bind = ldap3.AUTO_BIND_NO_TLS