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.

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

Add tls_strategy and deprecate use_ssl #258

Merged
merged 4 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 26 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`

Expand Down
90 changes: 79 additions & 11 deletions ldapauthenticator/ldapauthenticator.py
Original file line number Diff line number Diff line change
@@ -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):
Expand All @@ -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 to communicate with the LDAP server.
`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.
""",
)

Deprecated in version 3 of LDAP. Your LDAP server must be configured to support this, however.
@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="""
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.
""",
)

Expand Down Expand Up @@ -314,14 +365,31 @@ 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
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

Expand Down
32 changes: 30 additions & 2 deletions ldapauthenticator/tests/test_ldapauthenticator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -56,9 +58,35 @@ async def test_ldap_auth_blank_template(authenticator):
assert authorized["name"] == "fry"


async def test_ldap_auth_ssl(authenticator):
async def test_ldap_use_ssl_deprecation(authenticator):
assert authenticator.tls_strategy == TlsStrategy.before_bind

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment to these two tests saying we're not actually verifying SSL or TLS? for example if someone were to incorrectly modify the tls_strategy logic these tests would still pass.

Copy link
Member Author

@consideRatio consideRatio Sep 15, 2024

Choose a reason for hiding this comment

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

Good point! I pushed df38bd1 and df67ba7

Copy link
Member Author

Choose a reason for hiding this comment

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

after rebase: f89de63 d6178bc

# setting use_ssl to True should result in tls_strategy being set to
# on_connect
authenticator.use_ssl = True
authenticator.server_port = 636
assert authenticator.tls_strategy == TlsStrategy.on_connect


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
authorized = await authenticator.get_authenticated_user(
None, {"username": "fry", "password": "fry"}
)
assert authorized["name"] == "fry"


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
authorized = await authenticator.get_authenticated_user(
Expand Down
Loading