yammer / dropwizard-auth-ldap

Dropwizard Authentication Module for LDAP using JNDI.
Apache License 2.0
36 stars 17 forks source link

Should LDAP authentication continue if TLS negotiation fails? #17

Closed EmmetRMS closed 7 years ago

EmmetRMS commented 7 years ago

With LDAPS being deprecated [1] in favour of negotiating TLS over port 386 I'm assuming that if the TLS negotiation fails then the attempt to authenticate against the LDAP server on port 386 should also fail.

What I'm experiencing with the dropwizard-auth-ldap module (v1.0.3) is that if the client is returned a LDAP server security certificate which is not found in the client trust store (that is, the TLS negotiate fails) the DW LdapAuthenticator method authenticate() logs the error as LOGGER.info("Could not negotiate TLS", err); and then continues to pass the user credentials as plain text to the LDAP server. I would have thought the AutoclosingLdapContext constructor should log the error [2] and also throw an exception.

What are your thoughts?

Thanks.

[1]

https://books.google.com.au/books?id=utsMgEfnPSEC&pg=PT38&lpg=PT38&dq=RFC+2830+ldaps+deprecated&source=bl&ots=LomwCLOS0g&sig=T9cFF26ahPbzJUg5Hh-gp2ybA-s&hl=en&sa=X&ved=0ahUKEwjf6r69sq_SAhVJG5QKHX8RDqEQ6AEIODAE#v=onepage&q=RFC%202830%20ldaps%20deprecated&f=false

[2]

` public class AutoclosingLdapContext extends InitialLdapContext implements AutoCloseable { private static final Logger LOGGER = LoggerFactory.getLogger(AutoclosingLdapContext.class); private StartTlsResponse tls = null;

protected AutoclosingLdapContext() throws NamingException {
    this(new Hashtable<>(), true);
}

public AutoclosingLdapContext(Hashtable<?, ?> environment, boolean negotiateTls) throws NamingException {
    super(environment, null);
    if (negotiateTls) {
        try {
            tls = (StartTlsResponse) this.extendedOperation(new StartTlsRequest());
            tls.negotiate();
        } catch (Exception err) {
            LOGGER.info("Could not negotiate TLS", err);
        }
    }
}`
EmmetRMS commented 7 years ago

Sorry this is a confusion on my part. What I'm actually seeing is:

  1. The connection is first made to the LDAP server without TLS
  2. Then an attempt is made to promote the connection to TLS which then fails (due to the certificate issue) 3. The LdapAuthenticator method: public boolean authenticate(BasicCredentials credentials) throws io.dropwizard.auth.AuthenticationException returns true.

So I think I need to use the deprecated LDAPS protocol so that user credentials in step 1 aren't sent as plain text. Is that right?

EmmetRMS commented 7 years ago

Sorry I didn't mean to close the issue.

chrisgray commented 7 years ago

If I'm understanding your issue is it that you want it to connect to LDAP then when attempting to promote to TLS and that fails you want it to fail the entire attempt? If so, then I would agree that the code doesn't currently support that flow. It instead tries best effort to negotiate TLS and if it works great, otherwise at least still try to do plain-text which is what you're seeing at the moment.

You're correct to assume that if you use LDAPS then it will either work or not work but at least your credentials aren't sent over plain-text.

This was originally added to help support some users that wanted the ability to negotiate TLS if it was available. However, it sounds like perhaps we need to add the ability to have strict negotiation TLS in your case and fail the entire attempt if TLS is unavailable.

I can create a PR for this.

chrisgray commented 7 years ago

I've fixed this with PR https://github.com/yammer/dropwizard-auth-ldap/pull/18

negotiateTls can be NONE, ATTEMPT, or STRICT. Where ATTEMPT tries to negotiate TLS if possible and STRICT fails the entire operation if TLS does not succeed in being established.