Repository: jupyterhub/ldapauthenticator (language: python)

Issue:
Add a new option: use_tls
Hello !

Thank you for the work done on this plugin ! I am currently using it for accessing JupyterHub in our data science team at my company, but I needed to add an option "use_tls" and set it to False to actually make it work. That is because we need to connect to a LDAP server that needs this configuration.

For information, such option exists in the LDAP connector used by Airflow, another tool that we are also using. There is distincts "USE_SSL" and "USE_TLS" options for addressing our use case, so I just applied the same logic.

I hope that my PR is correct, I simply added the option "use_tls" and a small test for testing the option. I also needed to update the black pre-commit hook because it didn't work on my computer.

Obviously, the default value of this option does not change anything on the current behavior. 

Thank you ! 
Add `auto_bind` config option
<!-- Thank you for contributing. These HTML commments will not render in the issue, but you can delete them once you've read them if you prefer! -->

### Bug description
Make ldap3 library `auto_bind` config settable.

#### Expected behaviour
Currently, if `use_ssl` is set to `False`, `auto_bind` becomes `ldap3.AUTO_BIND_TLS_BEFORE_BIND`. This means that the ldap server must support ssl or starttls. There is no way to use a ldap server which doesn't have either. This is the case for a local LDAP server i.e runs in the same server as jupyterhub and communicates via the internal network.

#### Actual behaviour
Add a new config variable `auto_bind` to match upstream ldap3 library configuration.

I opened this as a bug because it used to because this was the behavior in 1.3.0 but it seems behavior changed because of the discussion in https://github.com/jupyterhub/ldapauthenticator/issues/171


Discussion / hints:
Thanks for submitting your first pull request! You are awesome! :hugs:
<br>If you haven't done so already, check out [Jupyter's Code of Conduct](https://github.com/jupyter/governance/blob/master/conduct/code_of_conduct.md). Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
![welcome](https://raw.githubusercontent.com/jupyterhub/.github/master/images/welcome.jpg)
You can meet the other [Jovyans](https://jupyter.readthedocs.io/en/latest/community/content-community.html?highlight=jovyan#what-is-a-jovyan) by joining our [Discourse forum](http://discourse.jupyter.org/). There is also a intro thread there where you can stop by and say Hi! :wave:
<br>Welcome to the Jupyter community! :tada:

I think this makes sense, it clarifies that actually TLS is used by default as well, but I'm struggling still a bit as there seem to be two ways of handling TLS.

(From googling `ldap port tls`)

![image](https://github.com/user-attachments/assets/4319ee18-d0e2-4a7c-ac48-89cedbf3addc)

This config seems related to `StartTLS` rather than encrypted from the start, right? Hmmm... Overall learning about things still and can't decide yet on pressing merge or not.
SSL and TLS --- its confusing to say "use_ssl=False" and "use_tls=True" at the same time, and its also confusing to have TLS when use_ssl is false as we do now.

Argh.
Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! :hugs:
<br>If you haven't done so already, check out [Jupyter's Code of Conduct](https://github.com/jupyter/governance/blob/master/conduct/code_of_conduct.md). Also, please try to follow the issue template as it helps other other community members to contribute more effectively.
![welcome](https://raw.githubusercontent.com/jupyterhub/.github/master/images/welcome.jpg)
You can meet the other [Jovyans](https://jupyter.readthedocs.io/en/latest/community/content-community.html?highlight=jovyan#what-is-a-jovyan) by joining our [Discourse forum](http://discourse.jupyter.org/). There is also an intro thread there where you can stop by and say Hi! :wave:
<br>Welcome to the Jupyter community! :tada:

Currently, the code reads like [this](https://github.com/jupyterhub/ldapauthenticator/blob/master/ldapauthenticator/ldapauthenticator.py#L307)

```
        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
        )
        conn = ldap3.Connection(
            server, user=userdn, password=password, auto_bind=auto_bind
        )
```

In the above code, if use_ssl is false, then auto_bind becomes `AUTO_BIND_TLS_BEFORE_BIND`. Are you open to adding a separate auto_bind config option which does not derive it's value from use_ssl ? This will be useful for servers which are local intranet/same server which don't have TLS enabled.


How about something like:

```
if self.auto_bind:
    auto_bind = self.auto_bind
else:
    auto_bind = (
        ldap3.AUTO_BIND_NO_TLS if self.use_ssl else ldap3.AUTO_BIND_TLS_BEFORE_BIND
    )
```

Well, I would argue that even in an intranet encryption is desirable as in most cases we should not trust all participants of the intranet to the full extent. If you believe that your suggestion is looking good, I guess you can create a pull request?
Having the same issue - has this solution been merge or any workaround?
Same here all internal networking and I really don't need encryption between the AD and jupyterhub. So for now I patched the auto_bind in the code but that is far from a solution. I really hope this will be addressed in the future.
@nylocx this is an open source project so anybody is free to create pull requests.
@1kastner Yeah I know, and after rereading my comment it may sound way harsher than intended (Not a native speaker) I really love the project and the work that is done on it. Just my current "patch" is nothing that will benefit the community, it's just getting our internal hub back to work before my colleagues start complaining :). I always have to get a signed permission to contribute to open source during working hours and with my current workload it will be hard to squeeze it in.

I see the benefits in the solution provided by @gramakri, but I think a more elegant solution might be to try to connect with the STARTTLS stuff and catch the exception to than retry with `ldap3.AUTO_BIND_NO_TLS`. This way users would not have to change anything and it might just continue to work. Maybe I find some free time to read into it and write a small pull request.
@nylocx I support your idea that security is always context-dependent. However, even in an internal network it seems wrong to me to transfer credentials in plaintext. If such an option is included, it should be obvious to anybody that this is *not* the go-to solution. 
Unfortunately one problem with this repo is it's very difficult to do a full test without a real LDAP system, though there are some CI tests which certainly help! To help with review it's helpful if tests can be added for any change, and ideally if you could solicit reviews from existing users (feel free to `@mention` people who are using it and could review it!) so we know it works.
> However, even in an internal network it seems wrong to me to transfer credentials in plaintext.

Totally agree on that and I have already field a request to our admin team to enable TLS encrypted LDAP on our Active Directory Server. But I would have liked to get a big red warning in the logs and a note that it will stop working in the next major release. To give me the time to update my infrastructure before running into trouble with a no longer working server.
Well, the version number of the ldap3 dependency was usually not pinned in this project. Previously, that library was very forgiving (many try/catches) which they stopped after one of their updates. This project shortly pinned the ldap3 version to a lower number as an intermediate solution before we reworked the `auto_bind` option to be like it is now - a very small change. If you want to go back to the previous behavior of the LDAPAuthenticator, I believe just pinning the ldap3 library to a lower version number is sufficient. So there couldn't have been any information in the logs of the LDAPAuthenticator because it was not the LDAPAuthenticator which changed.
@nylocx We definitely didn't intend to introduce any breaking changes, sorry if it caused you problems.

Regardless of whether the change was due to an upstream change or a change in this repo, the general problem is a lack of testing as I mentioned in my [previous comment](https://github.com/jupyterhub/ldapauthenticator/issues/179#issuecomment-829136905). A lack of tests for this configuration, coupled with a lack of people able/willing to test PRs, unfortunately led to this.

If anyone is able to contribute more tests, or is wiling to help review future PRs, that would be great! Note that _anyone_ on GitHub can review PRs and help out on issues, you don't have to be a maintainer!
I should really work on my written social skills ;) I wasn't under the impression that you actively broke something to state the point that I should fix my infrastructure (which I really should do, so I'm thankful for that too ;)).

The point above was just meant as another argument for the try except + logging a warning solution until the next major release instead of introducing another configuration variable.

But the more I think about it the best solution in my opinion would be to do both, add "old" default behavior by catching the exception and falling back to a non encrypted communication (which might be fine if both servers live on the same machine and communicate via the loopback interface) and stating in the warning that the user can silence this warning if he is sure that he wants an unencrypted connection by setting `force_unencrypted` to `True`. Or something like this.

If someone else finds the time to fix this before I do I would be happy to review and test it.
I somehow missed @1kastner 's [comment](https://github.com/jupyterhub/ldapauthenticator/issues/179#issuecomment-808873363). I will submit a PR. We use JupyterHub in the Cloudron app, so I have a test setup handy.

Fix patch:
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..761ff72 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 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.
         """,
     )
 
@@ -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
 


Test patch:
diff --git a/ldapauthenticator/tests/test_ldapauthenticator.py b/ldapauthenticator/tests/test_ldapauthenticator.py
index f8fc057..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,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
+
+    # 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(