Add `auto_bind` config option

[gramakri]

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 #171

[welcome]

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively.

You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[gramakri]

Currently, the code reads like this

        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.

[gramakri]

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
    )

[1kastner]

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?

[skpeters]

Having the same issue - has this solution been merge or any workaround?

[nylocx]

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.

[1kastner]

@nylocx this is an open source project so anybody is free to create pull requests.

[nylocx]

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

[1kastner]

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

[manics]

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.

[nylocx]

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.

[1kastner]

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.

[manics]

@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. 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!

[nylocx]

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.