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

Don't force TLS bind if not using SSL. #61

Merged
merged 1 commit into from
Jun 7, 2018

Conversation

GrahamDumpleton
Copy link
Contributor

@GrahamDumpleton GrahamDumpleton commented Feb 28, 2018

The current code in the authenticate() method does:

            conn = ldap3.Connection(
                server,
                user=self.escape_userdn_if_needed(userdn),
                password=password,
                auto_bind=ldap3.AUTO_BIND_TLS_BEFORE_BIND,
            )

The explicit setting of auto_bind to be ldap3.AUTO_BIND_TLS_BEFORE_BIND appears to be wrong because it is forcing TLS negotiation even if use_ssl option was False.

When not using SSL, this results in the error:

    Traceback (most recent call last):
      File "/opt/app-root/lib/python3.5/site-packages/tornado/web.py", line 1512, in _execute
        result = yield result
      File "/opt/app-root/lib/python3.5/site-packages/jupyterhub/handlers/login.py", line 83, in post
        user = yield self.login_user(data)
      File "/opt/app-root/lib/python3.5/site-packages/jupyterhub/handlers/base.py", line 328, in login_user
        authenticated = yield self.authenticate(data)
      File "/opt/app-root/lib/python3.5/site-packages/jupyterhub/auth.py", line 227, in get_authenticated_user
        authenticated = yield self.authenticate(handler, data)
      File "/opt/app-root/lib64/python3.5/types.py", line 243, in wrapped
        coro = func(*args, **kwargs)
      File "/opt/app-root/lib/python3.5/site-packages/ldapauthenticator/ldapauthenticator.py", line 314, in authenticate
        conn = getConnection(userdn, username, password)
      File "/opt/app-root/lib/python3.5/site-packages/ldapauthenticator/ldapauthenticator.py", line 284, in getConnection
        auto_bind=ldap3.AUTO_BIND_TLS_BEFORE_BIND,
      File "/opt/app-root/lib/python3.5/site-packages/ldap3/core/connection.py", line 321, in __init__
        self.do_auto_bind()
      File "/opt/app-root/lib/python3.5/site-packages/ldap3/core/connection.py", line 336, in do_auto_bind
        self.open(read_server_info=False)
      File "/opt/app-root/lib/python3.5/site-packages/ldap3/strategy/sync.py", line 56, in open
        BaseStrategy.open(self, reset_usage, read_server_info)
      File "/opt/app-root/lib/python3.5/site-packages/ldap3/strategy/base.py", line 153, in open
        self.connection.do_auto_bind()
      File "/opt/app-root/lib/python3.5/site-packages/ldap3/core/connection.py", line 340, in do_auto_bind
        self.start_tls(read_server_info=False)
      File "/opt/app-root/lib/python3.5/site-packages/ldap3/core/connection.py", line 1215, in start_tls
        if self.server.tls.start_tls(self) and self.strategy.sync:  # for asynchronous connections _start_tls is run by the strategy
      File "/opt/app-root/lib/python3.5/site-packages/ldap3/core/tls.py", line 271, in start_tls
        raise LDAPStartTLSError(connection.last_error)
    ldap3.core.exceptions.LDAPStartTLSError: startTLS failed - unavailable

because it is perhaps triggering TLS connection against LDAP port 389, or may be because LDAPS port 636 is not working.

This PR changes the code to:

            conn = ldap3.Connection(
                server,
                user=self.escape_userdn_if_needed(userdn),
                password=password,
                auto_bind=self.use_ssl and ldap3.AUTO_BIND_TLS_BEFORE_BIND or ldap3.AUTO_BIND_NO_TLS,
            )

so that TLS binding is only requested if use_ssl is True.

This eliminates the exception and is believed not to affect SSL behaviour, although didn't have a LDAP with working LDAPS port to test.

It is amending what was originally added for this in #46

@GrahamDumpleton GrahamDumpleton changed the title Don't force TLS bind if not using TLS. Don't force TLS bind if not using SSL. Feb 28, 2018
@GrahamDumpleton
Copy link
Contributor Author

I will add that maybe you intended it to try and upgrade from LDAP to LDAPS automatically even when use_ssl is False. The problem with that in scenario encountered was that the LDAPS port was non functional because of certificate issues or something. In attempting LDAPS first, even though actually wanted only LDAP, the TLS failure would result in the whole thing failing and wasn't falling back to LDAP. So it was required to use LDAP and didn't want it trying LDAPS. The current setting interpretation means you can't say only try LDAP.

If you still want automatic upgrade, then maybe use_ssl should default to None meaning auto upgrade. Be able to be set to False to say no TLS and only try LDAP. And True means only LDAPS.

@GrahamDumpleton
Copy link
Contributor Author

FWIW, basing change made on what has been done in other packages, such as:

@haobibo
Copy link

haobibo commented Mar 14, 2018

Thank you @GrahamDumpleton ! This helped me!

@dhirschfeld
Copy link
Collaborator

Thanks for your contribution @GrahamDumpleton! I'm doing some PR triage in the hope of getting a release out the door shortly.

Also, thank you for the detailed description - it makes me a lot happier that I understand the change and the reasons for it so I'm happy to merge.

@dhirschfeld dhirschfeld merged commit 6cca4b8 into jupyterhub:master Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants