GUACAMOLE-938: Refactor LDAP connect/bind process such that the same code is used for all LDAP connection attempts, including referrals.

extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java

@@ -20,12 +20,10 @@
 package org.apache.guacamole.auth.ldap;
 
 import com.google.inject.Inject;
+import org.apache.directory.api.ldap.model.exception.LdapAuthenticationException;
 import org.apache.directory.api.ldap.model.exception.LdapException;
+import org.apache.directory.api.ldap.model.exception.LdapInvalidDnException;
 import org.apache.directory.api.ldap.model.filter.ExprNode;
-import org.apache.directory.api.ldap.model.message.BindRequest;
-import org.apache.directory.api.ldap.model.message.BindRequestImpl;
-import org.apache.directory.api.ldap.model.message.BindResponse;
-import org.apache.directory.api.ldap.model.message.ResultCodeEnum;
 import org.apache.directory.api.ldap.model.message.SearchRequest;
 import org.apache.directory.api.ldap.model.message.SearchRequestImpl;
 import org.apache.directory.api.ldap.model.message.SearchScope;
@@ -59,41 +57,58 @@ public class LDAPConnectionService {
 
     /**
      * Creates a new instance of LdapNetworkConnection, configured as required
-     * to use whichever encryption method is requested within
+     * to use the given encryption method to communicate with the LDAP server
-     * guacamole.properties.
+     * at the given hostname and port. The returned LdapNetworkConnection is
+     * configured for use but is not yet connected nor bound to the LDAP
+     * server. It will not be bound until it a bind operation is explicitly
+     * requested, and will not connected until it is used in an LDAP operation
+     * (such as a bind).
+     *
+     * @param host
+     *     The hostname or IP address of the LDAP server.
+     *
+     * @param port
+     *     The TCP port that the LDAP server is listening on.
+     *
+     * @param encryptionMethod
+     *     The encryption method that should be used to communicate with the
+     *     LDAP server.
      *
      * @return
-     *     A new LdapNetworkConnection instance which has already been 
+     *     A new instance of LdapNetworkConnection which uses the given
-     *     configured to use the encryption method requested within 
+     *     encryption method to communicate with the LDAP server at the given
-     *     guacamole.properties.
+     *     hostname and port.
      *
      * @throws GuacamoleException
-     *     If an error occurs while parsing guacamole.properties, or if the
+     *     If the requested encryption method is actually not implemented (a
-     *     requested encryption method is actually not implemented (a bug).
+     *     bug).
      */
-    private LdapNetworkConnection createLDAPConnection() throws GuacamoleException {
+    private LdapNetworkConnection createLDAPConnection(String host, int port,
+            EncryptionMethod encryptionMethod) throws GuacamoleException {
+
+        LdapConnectionConfig config = new LdapConnectionConfig();
+        config.setLdapHost(host);
+        config.setLdapPort(port);
 
-        String host = confService.getServerHostname();
-        int port = confService.getServerPort();
-        
         // Map encryption method to proper connection and socket factory
-        EncryptionMethod encryptionMethod = confService.getEncryptionMethod();
         switch (encryptionMethod) {
 
             // Unencrypted LDAP connection
             case NONE:
                 logger.debug("Connection to LDAP server without encryption.");
-                return new LdapNetworkConnection(host, port);
+                break;
 
             // LDAP over SSL (LDAPS)
             case SSL:
                 logger.debug("Connecting to LDAP server using SSL/TLS.");
-                return new LdapNetworkConnection(host, port, true);
+                config.setUseSsl(true);
+                break;
 
             // LDAP + STARTTLS
             case STARTTLS:
                 logger.debug("Connecting to LDAP server using STARTTLS.");
-                return new LdapNetworkConnection(host, port);
+                config.setUseTls(true);
+                break;
 
             // The encryption method, though known, is not actually
             // implemented. If encountered, this would be a bug.
@@ -102,10 +117,206 @@ public class LDAPConnectionService {
 
         }
 
+        return new LdapNetworkConnection(config);
+
     }
 
     /**
-     * Binds to the LDAP server using the provided user DN and password.
+     * Creates a new instance of LdapNetworkConnection, configured as required
+     * to use whichever encryption method, hostname, and port are requested
+     * within guacamole.properties. The returned LdapNetworkConnection is
+     * configured for use but is not yet connected nor bound to the LDAP
+     * server. It will not be bound until it a bind operation is explicitly
+     * requested, and will not connected until it is used in an LDAP operation
+     * (such as a bind).
+     *
+     * @return
+     *     A new LdapNetworkConnection instance which has already been
+     *     configured to use the encryption method, hostname, and port
+     *     requested within guacamole.properties.
+     *
+     * @throws GuacamoleException
+     *     If an error occurs while parsing guacamole.properties, or if the
+     *     requested encryption method is actually not implemented (a bug).
+     */
+    private LdapNetworkConnection createLDAPConnection()
+            throws GuacamoleException {
+        return createLDAPConnection(
+                confService.getServerHostname(),
+                confService.getServerPort(),
+                confService.getEncryptionMethod());
+    }
+
+    /**
+     * Creates a new instance of LdapNetworkConnection, configured as required
+     * to use whichever encryption method, hostname, and port are specified
+     * within the given LDAP URL. The returned LdapNetworkConnection is
+     * configured for use but is not yet connected nor bound to the LDAP
+     * server. It will not be bound until it a bind operation is explicitly
+     * requested, and will not connected until it is used in an LDAP operation
+     * (such as a bind).
+     *
+     * @param url
+     *     The LDAP URL containing the details which should be used to connect
+     *     to the LDAP server.
+     *
+     * @return
+     *     A new LdapNetworkConnection instance which has already been
+     *     configured to use the encryption method, hostname, and port
+     *     specified within the given LDAP URL.
+     *
+     * @throws GuacamoleException
+     *     If the given URL is not a valid LDAP URL, or if the encryption
+     *     method indicated by the URL is known but not actually implemented (a
+     *     bug).
+     */
+    private LdapNetworkConnection createLDAPConnection(String url)
+            throws GuacamoleException {
+
+        // Parse provided LDAP URL
+        LdapUrl ldapUrl;
+        try {
+            ldapUrl = new LdapUrl(url);
+        }
+        catch (LdapException e) {
+            logger.debug("Cannot connect to LDAP URL \"{}\": URL is invalid.", url, e);
+            throw new GuacamoleServerException("Invalid LDAP URL.", e);
+        }
+
+        // Retrieve hostname from URL, bailing out if no hostname is present
+        String host = ldapUrl.getHost();
+        if (host == null || host.isEmpty()) {
+            logger.debug("Cannot connect to LDAP URL \"{}\": no hostname is present.", url);
+            throw new GuacamoleServerException("LDAP URL contains no hostname.");
+        }
+
+        // Parse encryption method from URL scheme
+        EncryptionMethod encryptionMethod = EncryptionMethod.NONE;
+        if (LdapUrl.LDAPS_SCHEME.equals(ldapUrl.getScheme()))
+            encryptionMethod = EncryptionMethod.SSL;
+
+        // If no post is specified within the URL, use the default port
+        // dictated by the encryption method
+        int port = ldapUrl.getPort();
+        if (port < 1)
+            port = encryptionMethod.DEFAULT_PORT;
+
+        return createLDAPConnection(host, port, encryptionMethod);
+
+    }
+
+    /**
+     * Binds to the LDAP server indicated by the given LdapNetworkConnection
+     * using the given credentials. If the LdapNetworkConnection is not yet
+     * connected, an LDAP connection is first established. The provided
+     * credentials will be stored within the LdapConnectionConfig of the given
+     * LdapNetworkConnection. If the bind operation fails, the given
+     * LdapNetworkConnection is automatically closed.
+     *
+     * @param ldapConnection
+     *     The LdapNetworkConnection describing the connection to the LDAP
+     *     server. This LdapNetworkConnection is modified as a result of this
+     *     call and will be automatically closed if this call fails.
+     *
+     * @param userDN
+     *     The DN of the user to bind as, or null to bind anonymously.
+     *
+     * @param password
+     *     The password to use when binding as the specified user, or null to
+     *     attempt to bind without a password.
+     *
+     * @return
+     *     A bound LDAP connection, or null if the connection could not be
+     *     bound.
+     */
+    private LdapNetworkConnection bindAs(LdapNetworkConnection ldapConnection,
+            Dn userDN, String password) {
+
+        // Add credentials to existing config
+        LdapConnectionConfig config = ldapConnection.getConfig();
+        config.setName(userDN.getName());
+        config.setCredentials(password);
+
+        try {
+            // Connect and bind using provided credentials
+            ldapConnection.bind();
+        }
+
+        // Disconnect if an authentication error occurs, but log that failure
+        // only at the debug level (such failures are expected)
+        catch (LdapAuthenticationException e) {
+            ldapConnection.close();
+            logger.debug("Bind attempt with LDAP server as user \"{}\" failed.", userDN, e);
+            return null;
+        }
+
+        // Disconnect for all other bind failures, as well, logging those at
+        // the error level
+        catch (LdapException e) {
+            ldapConnection.close();
+            logger.error("Binding with the LDAP server at \"{}\" as user "
+                    + "\"{}\" failed: {}", config.getLdapHost(), userDN, e.getMessage());
+            logger.debug("Unable to bind to LDAP server.", e);
+            return null;
+        }
+
+        return ldapConnection;
+
+    }
+
+    /**
+     * Binds to the LDAP server indicated by a given LdapNetworkConnection
+     * using the credentials that were used to bind another
+     * LdapNetworkConnection. If the LdapNetworkConnection about to be bound is
+     * not yet connected, an LDAP connection is first established. The
+     * credentials from the other LdapNetworkConnection will be stored within
+     * the LdapConnectionConfig of the given LdapNetworkConnection. If the bind
+     * operation fails, the given LdapNetworkConnection is automatically
+     * closed.
+     *
+     * @param ldapConnection
+     *     The LdapNetworkConnection describing the connection to the LDAP
+     *     server. This LdapNetworkConnection is modified as a result of this
+     *     call and will be automatically closed if this call fails.
+     *
+     * @param useCredentialsFrom
+     *     A bound LdapNetworkConnection whose bind credentials should be
+     *     copied for use within this bind operation.
+     *
+     * @return
+     *     A bound LDAP connection, or null if the connection could not be
+     *     bound.
+     */
+    private LdapNetworkConnection bindAs(LdapNetworkConnection ldapConnection,
+            LdapNetworkConnection useCredentialsFrom) {
+
+        // Copy bind username and password from original config
+        LdapConnectionConfig ldapConfig = useCredentialsFrom.getConfig();
+        String username = ldapConfig.getName();
+        String password = ldapConfig.getCredentials();
+
+        // Parse bind username as an LDAP DN
+        Dn userDN;
+        try {
+            userDN = new Dn(username);
+        }
+        catch (LdapInvalidDnException e) {
+            logger.error("Credentials of existing connection cannot be used. "
+                    + "The username used (\"{}\") is not a valid DN.", username);
+            logger.debug("Cannot bind using invalid DN.", e);
+            ldapConnection.close();
+            return null;
+        }
+
+        // Bind using username/password from existing connection
+        return bindAs(ldapConnection, userDN, password);
+
+    }
+
+    /**
+     * Binds to the LDAP server using the provided user DN and password. The
+     * hostname, port, and encryption method of the LDAP server are determined
+     * from guacamole.properties.
      *
      * @param userDN
      *     The DN of the user to bind as, or null to bind anonymously.
@@ -119,110 +330,39 @@ public class LDAPConnectionService {
      *     bound.
      *
      * @throws GuacamoleException
-     *     If the configuration details relevant to binding to the LDAP server
+     *     If an error occurs while parsing guacamole.properties, or if the
-     *     cannot be read.
+     *     configured encryption method is actually not implemented (a bug).
      */
     public LdapNetworkConnection bindAs(Dn userDN, String password)
             throws GuacamoleException {
-
+        return bindAs(createLDAPConnection(), userDN, password);
-        // Get ldapConnection and try to connect and bind.
-        LdapNetworkConnection ldapConnection = createLDAPConnection();
-        try {
-
-            // Connect to LDAP server
-            ldapConnection.connect();
-
-            // Explicitly start TLS if requested
-            if (confService.getEncryptionMethod() == EncryptionMethod.STARTTLS)
-                ldapConnection.startTls();
-
-            // Bind using provided credentials
-            BindRequest bindRequest = new BindRequestImpl();
-            bindRequest.setDn(userDN);
-            bindRequest.setCredentials(password);
-            BindResponse bindResponse = ldapConnection.bind(bindRequest);
-
-            if (bindResponse.getLdapResult().getResultCode() != ResultCodeEnum.SUCCESS) {
-                ldapConnection.close();
-                logger.debug("LDAP bind attempt failed: {}", bindResponse.toString());
-                return null;
-            }
-
-        }
-
-        // Disconnect if an error occurs during bind
-        catch (LdapException e) {
-            ldapConnection.close();
-            logger.debug("Unable to bind to LDAP server.", e);
-            return null;
-        }
-
-        return ldapConnection;
-
     }
-    
+
     /**
-     * Establishes a new network connection to the LDAP server indicated by the
+     * Binds to the LDAP server indicated by the given LDAP URL using the
-     * given LDAP referral URL. The credentials used to bind with the referred
+     * credentials that were used to bind an existing LdapNetworkConnection.
-     * LDAP server will be the same as those used to bind with the original
-     * connection.
-     * 
-     * @param ldapConnection
-     *     The LDAP connection that bind credentials should be copied from.
      *
      * @param url
-     *     The URL of the referred LDAP server to which a new network
+     *     The LDAP URL containing the details which should be used to connect
-     *     connection should be established.
+     *     to the LDAP server.
+     *
+     * @param useCredentialsFrom
+     *     A bound LdapNetworkConnection whose bind credentials should be
+     *     copied for use within this bind operation.
      *
      * @return
-     *     A LdapNetworkConnection representing a network connection to the
+     *     A bound LDAP connection, or null if the connection could not be
-     *     LDAP server specified in the URL, or null if the specified URL is
+     *     bound.
-     *     invalid.
+     *
+     * @throws GuacamoleException
+     *     If the given URL is not a valid LDAP URL, or if the encryption
+     *     method indicated by the URL is known but not actually implemented (a
+     *     bug).
      */
-    public LdapNetworkConnection getReferralConnection(
+    public LdapNetworkConnection bindAs(String url,
-            LdapNetworkConnection ldapConnection, String url) {
+            LdapNetworkConnection useCredentialsFrom)
-       
+            throws GuacamoleException {
-        LdapConnectionConfig ldapConfig = ldapConnection.getConfig();
+        return bindAs(createLDAPConnection(url), useCredentialsFrom);
-        LdapConnectionConfig referralConfig = new LdapConnectionConfig();
-        
-        // Copy bind name and password from original config
-        referralConfig.setName(ldapConfig.getName());
-        referralConfig.setCredentials(ldapConfig.getCredentials());        
-
-        LdapUrl referralUrl;
-        try {
-            referralUrl = new LdapUrl(url);
-        }
-        catch (LdapException e) {
-            logger.debug("Referral URL \"{}\" is invalid.", url, e);
-            return null;
-        }
-
-        // Look for host - if not there, bail out.
-        String host = referralUrl.getHost();
-        if (host == null || host.isEmpty()) {
-            logger.debug("Referral URL \"{}\" is invalid as it contains "
-                    + "no hostname.", url );
-            return null;
-        }
-       
-        referralConfig.setLdapHost(host);
-       
-        // Look for port, or assign a default.
-        int port = referralUrl.getPort();
-        if (port < 1)
-            referralConfig.setLdapPort(389);
-        else
-            referralConfig.setLdapPort(port);
-        
-        // Deal with SSL connections
-        if (referralUrl.getScheme().equals(LdapUrl.LDAPS_SCHEME))
-            referralConfig.setUseSsl(true);
-        else
-            referralConfig.setUseSsl(false);
-        
-        return new LdapNetworkConnection(referralConfig);
-        
     }
 
     /**

extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ObjectQueryService.java

@@ -41,8 +41,6 @@ import org.apache.directory.api.ldap.model.filter.OrNode;
 import org.apache.directory.api.ldap.model.filter.PresenceNode;
 import org.apache.directory.api.ldap.model.message.SearchRequest;
 import org.apache.directory.api.ldap.model.name.Dn;
-import org.apache.directory.api.ldap.model.url.LdapUrl;
-import org.apache.directory.ldap.client.api.LdapConnectionConfig;
 import org.apache.directory.ldap.client.api.LdapNetworkConnection;
 import org.apache.guacamole.GuacamoleException;
 import org.apache.guacamole.GuacamoleServerException;
@@ -250,14 +248,15 @@ public class ObjectQueryService {
 
                             // Connect to referred LDAP server to retrieve further results, ensuring the network
                             // connection is always closed when it will no longer be used
-                            try (LdapNetworkConnection referralConnection = ldapService.getReferralConnection(ldapConnection, url)) {
+                            try (LdapNetworkConnection referralConnection = ldapService.bindAs(url, ldapConnection)) {
                                 if (referralConnection != null) {
                                     logger.debug("Following referral to \"{}\"...", url);
                                     entries.addAll(search(referralConnection, baseDN, query, searchHop + 1));
                                 }
                                 else
-                                    logger.debug("Could not follow referral to "
+                                    logger.debug("Could not bind with LDAP "
-                                            + "\"{}\" as the URL is invalid.", url);
+                                            + "server indicated by referral "
+                                            + "URL \"{}\".", url);
                             }
                             catch (GuacamoleException e) {
                                 logger.warn("Referral to \"{}\" could not be followed: {}", url, e.getMessage());