LdapUserSearch rebind with bind DN after user bind (elastic/x-pack-elasticsearch#4209)

Fixes an inconsistency bug in which `LdapSession`s built by
`LdapUserSearchSessionFactory` are different if the factory is
configured to use a connection pool or not. The bind status of the
connection, or the connection(s) from the pool, passed through to
the newly minted `LdapSession` are now identical. Connections are
bind to the bind_dn configuration entry in the realm config.

Original commit: elastic/x-pack-elasticsearch@094af063ea

plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java

@@ -285,7 +285,7 @@ class ActiveDirectorySessionFactory extends PoolingSessionFactory {
                                 ActionListener<LdapSession> listener) {
             final byte[] passwordBytes = CharArrays.toUtf8Bytes(password.getChars());
             final SimpleBindRequest bind = new SimpleBindRequest(bindUsername(username), passwordBytes);
-            LdapUtils.maybeForkThenBind(pool, bind, threadPool, new ActionRunnable<LdapSession>(listener) {
+            LdapUtils.maybeForkThenBindAndRevert(pool, bind, threadPool, new ActionRunnable<LdapSession>(listener) {
                 @Override
                 protected void doRun() throws Exception {
                     searchForDN(pool, username, password, Math.toIntExact(timeout.seconds()), ActionListener.wrap((entry) -> {

plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/LdapUserSearchSessionFactory.java

@@ -90,18 +90,13 @@ class LdapUserSearchSessionFactory extends PoolingSessionFactory {
             } else {
                 final String dn = entry.getDN();
                 final byte[] passwordBytes = CharArrays.toUtf8Bytes(password.getChars());
-                SimpleBindRequest bind = new SimpleBindRequest(dn, passwordBytes);
+                final SimpleBindRequest bind = new SimpleBindRequest(dn, passwordBytes);
-                LdapUtils.maybeForkThenBind(connectionPool, bind, threadPool, new ActionRunnable<LdapSession>(listener) {
+                LdapUtils.maybeForkThenBindAndRevert(connectionPool, bind, threadPool, new ActionRunnable<LdapSession>(listener) {
                     @Override
                     protected void doRun() throws Exception {
                         listener.onResponse(new LdapSession(logger, config, connectionPool, dn, groupResolver, metaDataResolver, timeout,
                                 entry.getAttributes()));
                     }
-
-                    @Override
-                    public void onFailure(Exception e) {
-                        listener.onFailure(e);
-                    }
                 });
             }
         }, listener::onFailure));
@@ -138,8 +133,20 @@ class LdapUserSearchSessionFactory extends PoolingSessionFactory {
                             LdapUtils.maybeForkThenBind(connection, userBind, threadPool, new AbstractRunnable() {
                                 @Override
                                 protected void doRun() throws Exception {
-                                    listener.onResponse(new LdapSession(logger, config, connection, dn, groupResolver, metaDataResolver,
+                                    LdapUtils.maybeForkThenBind(connection, bind, threadPool, new AbstractRunnable() {
-                                            timeout, entry.getAttributes()));
+
+                                        @Override
+                                        protected void doRun() throws Exception {
+                                            listener.onResponse(new LdapSession(logger, config, connection, dn, groupResolver,
+                                                    metaDataResolver, timeout, entry.getAttributes()));
+                                        }
+
+                                        @Override
+                                        public void onFailure(Exception e) {
+                                            IOUtils.closeWhileHandlingException(connection);
+                                            listener.onFailure(e);
+                                        }
+                                    });
                                 }
 
                                 @Override

plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/support/LdapUtils.java

@@ -88,22 +88,35 @@ public final class LdapUtils {
     }
 
     /**
-     * This method submits the {@code bind} request over the provided {@code ldap}
+     * If necessary, fork before executing the runnable. A deadlock will happen if
-     * connection or connection pool. The connection authentication status changes,
+     * the same thread which handles bind responses blocks on the bind call, waiting
-     * see {@code LDAPConnection#bind}; in case of a connection pool the bind
+     * for the response which he itself should handle.
-     * authentication status is reverted, so that the connection can be safely
+     */
-     * returned to the pool, see:
+    private static void maybeForkAndRun(ThreadPool threadPool, Runnable runnable) {
-     * {@code LDAPConnectionPool#bindAndRevertAuthentication}.
+        if (isLdapConnectionThread(Thread.currentThread())) {
+            // only fork if binding on the LDAPConnectionReader thread
+            threadPool.executor(ThreadPool.Names.GENERIC).execute(runnable);
+        } else {
+            // avoids repeated forking
+            runnable.run();
+        }
+    }
+
+    /**
+     * This method submits the {@code bind} request over one connection from the
+     * pool. The bind authentication is then reverted and the connection is returned
+     * to the pool, so that the connection can be safely reused, see
+     * {@code LDAPConnectionPool#bindAndRevertAuthentication}. This validates the
+     * bind credentials.
      *
      * Bind calls are blocking and if a bind is executed on the LDAP Connection
-     * Reader thread (as returned by {@code LdapUtils#isLdapConnectionThread}),
+     * Reader thread (as returned by {@code LdapUtils#isLdapConnectionThread}), the
-     * the thread will be blocked until it is interrupted by something else
+     * thread will be blocked until it is interrupted by something else such as a
-     * such as a timeout timer.
+     * timeout timer. <b>Do not call bind</b> outside this method or
-     * <b>Do not call bind</b> outside of this method.
+     * {@link LdapUtils#maybeForkThenBind(LDAPConnection, BindRequest, ThreadPool, AbstractRunnable)}
      *
-     * @param ldap
+     * @param ldapPool
-     *            The LDAP connection or connection pool on which to submit the bind
+     *            The LDAP connection pool on which to submit the bind operation.
-     *            operation.
      * @param bind
      *            The request object of the bind operation.
      * @param threadPool
@@ -114,19 +127,13 @@ public final class LdapUtils {
      *            The runnable that continues the program flow after the bind
      *            operation. It is executed on the same thread as the prior bind.
      */
-    public static void maybeForkThenBind(LDAPInterface ldap, BindRequest bind, ThreadPool threadPool,
+    public static void maybeForkThenBindAndRevert(LDAPConnectionPool ldapPool, BindRequest bind, ThreadPool threadPool,
                                                   AbstractRunnable runnable) {
-        Runnable bindRunnable = new AbstractRunnable() {
+        final Runnable bindRunnable = new AbstractRunnable() {
             @Override
             @SuppressForbidden(reason = "Bind allowed if forking of the LDAP Connection Reader Thread.")
             protected void doRun() throws Exception {
-                if (ldap instanceof LDAPConnectionPool) {
+                privilegedConnect(() -> ldapPool.bindAndRevertAuthentication(bind.duplicate()));
-                    privilegedConnect(() -> ((LDAPConnectionPool) ldap).bindAndRevertAuthentication(bind));
-                } else if (ldap instanceof LDAPConnection) {
-                    ((LDAPConnection) ldap).bind(bind);
-                } else {
-                    throw new IllegalArgumentException("unsupported LDAPInterface implementation: " + ldap);
-                }
                 runnable.run();
             }
 
@@ -140,14 +147,52 @@ public final class LdapUtils {
                 runnable.onAfter();
             }
         };
-
+        maybeForkAndRun(threadPool, bindRunnable);
-        if (isLdapConnectionThread(Thread.currentThread())) {
-            // only fork if binding on the LDAPConnectionReader thread
-            threadPool.executor(ThreadPool.Names.GENERIC).execute(bindRunnable);
-        } else {
-            // avoids repeated forking
-            bindRunnable.run();
     }
+
+    /**
+     * This method submits the {@code bind} request over the ldap connection. Its
+     * authentication status changes. The connection can be subsequently reused.
+     * This validates the bind credentials.
+     *
+     * Bind calls are blocking and if a bind is executed on the LDAP Connection
+     * Reader thread (as returned by {@code LdapUtils#isLdapConnectionThread}), the
+     * thread will be blocked until it is interrupted by something else such as a
+     * timeout timer. <b>Do not call bind</b> outside this method or
+     * {@link LdapUtils#maybeForkThenBind(LDAPConnection, BindRequest, ThreadPool, AbstractRunnable)}
+     *
+     * @param ldap
+     *            The LDAP connection on which to submit the bind operation.
+     * @param bind
+     *            The request object of the bind operation.
+     * @param threadPool
+     *            The threads that will call the blocking bind operation, in case
+     *            the calling thread is a connection reader, see:
+     *            {@code LdapUtils#isLdapConnectionThread}.
+     * @param runnable
+     *            The runnable that continues the program flow after the bind
+     *            operation. It is executed on the same thread as the prior bind.
+     */
+    public static void maybeForkThenBind(LDAPConnection ldap, BindRequest bind, ThreadPool threadPool, AbstractRunnable runnable) {
+        final Runnable bindRunnable = new AbstractRunnable() {
+            @Override
+            @SuppressForbidden(reason = "Bind allowed if forking of the LDAP Connection Reader Thread.")
+            protected void doRun() throws Exception {
+                privilegedConnect(() -> ldap.bind(bind.duplicate()));
+                runnable.run();
+            }
+
+            @Override
+            public void onFailure(Exception e) {
+                runnable.onFailure(e);
+            }
+
+            @Override
+            public void onAfter() {
+                runnable.onAfter();
+            }
+        };
+        maybeForkAndRun(threadPool, bindRunnable);
     }
 
     /**

plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/LdapSessionFactoryTests.java

@@ -6,8 +6,11 @@
 package org.elasticsearch.xpack.security.authc.ldap;
 
 import com.unboundid.ldap.listener.InMemoryDirectoryServer;
+import com.unboundid.ldap.sdk.BindRequest;
 import com.unboundid.ldap.sdk.LDAPException;
 import com.unboundid.ldap.sdk.LDAPURL;
+import com.unboundid.ldap.sdk.SimpleBindRequest;
+
 import org.elasticsearch.common.settings.SecureString;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.concurrent.ThreadContext;
@@ -92,9 +95,10 @@ public class LdapSessionFactoryTests extends LdapTestCase {
 
         String user = "Horatio Hornblower";
         SecureString userPass = new SecureString("pass");
+        final SimpleBindRequest bindRequest = new SimpleBindRequest("cn=Horatio Hornblower,ou=people,o=sevenSeas", "pass");
 
         try (LdapSession ldap = session(sessionFactory, user, userPass)) {
-            assertConnectionCanReconnect(ldap.getConnection());
+            assertConnectionValid(ldap.getConnection(), bindRequest);
             String dn = ldap.userDn();
             assertThat(dn, containsString(user));
         }
@@ -132,9 +136,10 @@ public class LdapSessionFactoryTests extends LdapTestCase {
 
         String user = "Horatio Hornblower";
         SecureString userPass = new SecureString("pass");
+        final SimpleBindRequest bindRequest = new SimpleBindRequest("cn=Horatio Hornblower,ou=people,o=sevenSeas", "pass");
 
         try (LdapSession ldap = session(ldapFac, user, userPass)) {
-            assertConnectionCanReconnect(ldap.getConnection());
+            assertConnectionValid(ldap.getConnection(), bindRequest);
             List<String> groups = groups(ldap);
             assertThat(groups, contains("cn=HMS Lydia,ou=crews,ou=groups,o=sevenSeas"));
         }
@@ -149,8 +154,10 @@ public class LdapSessionFactoryTests extends LdapTestCase {
         LdapSessionFactory ldapFac = new LdapSessionFactory(config, sslService, threadPool);
 
         String user = "Horatio Hornblower";
+        final SimpleBindRequest bindRequest = new SimpleBindRequest("cn=Horatio Hornblower,ou=people,o=sevenSeas", "pass");
+
         try (LdapSession ldap = session(ldapFac, user, new SecureString("pass"))) {
-            assertConnectionCanReconnect(ldap.getConnection());
+            assertConnectionValid(ldap.getConnection(), bindRequest);
             List<String> groups = groups(ldap);
             assertThat(groups, contains("cn=HMS Lydia,ou=crews,ou=groups,o=sevenSeas"));
         }
@@ -166,9 +173,10 @@ public class LdapSessionFactoryTests extends LdapTestCase {
 
         String user = "Horatio Hornblower";
         SecureString userPass = new SecureString("pass");
+        final SimpleBindRequest bindRequest = new SimpleBindRequest("cn=Horatio Hornblower,ou=people,o=sevenSeas", "pass");
 
         try (LdapSession ldap = session(ldapFac, user, userPass)) {
-            assertConnectionCanReconnect(ldap.getConnection());
+            assertConnectionValid(ldap.getConnection(), bindRequest);
             List<String> groups = groups(ldap);
             assertThat(groups.size(), is(1));
             assertThat(groups, contains("cn=HMS Lydia,ou=crews,ou=groups,o=sevenSeas"));

plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/LdapUserSearchSessionFactoryTests.java

@@ -24,7 +24,6 @@ import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.common.util.concurrent.ThreadContext;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.env.TestEnvironment;
-import org.elasticsearch.test.junit.annotations.Network;
 import org.elasticsearch.threadpool.TestThreadPool;
 import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.xpack.core.security.authc.RealmConfig;
@@ -40,9 +39,7 @@ import org.junit.After;
 import org.junit.Before;
 
 import java.nio.file.Path;
-import java.util.List;
 
-import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.isEmptyString;
@@ -137,18 +134,19 @@ public class LdapUserSearchSessionFactoryTests extends LdapTestCase {
 
         String user = "William Bush";
         SecureString userPass = new SecureString("pass");
+        final SimpleBindRequest bindDNBindRequest = LdapUserSearchSessionFactory.bindRequest(config.settings());
 
         try {
             // auth
             try (LdapSession ldap = session(sessionFactory, user, userPass)) {
-                assertConnectionCanReconnect(ldap.getConnection());
+                assertConnectionValid(ldap.getConnection(), bindDNBindRequest);
                 String dn = ldap.userDn();
                 assertThat(dn, containsString(user));
             }
 
             //lookup
             try (LdapSession ldap = unauthenticatedSession(sessionFactory, user)) {
-                assertConnectionCanReconnect(ldap.getConnection());
+                assertConnectionValid(ldap.getConnection(), bindDNBindRequest);
                 String dn = ldap.userDn();
                 assertThat(dn, containsString(user));
             }
@@ -222,18 +220,19 @@ public class LdapUserSearchSessionFactoryTests extends LdapTestCase {
 
         String user = "William Bush";
         SecureString userPass = new SecureString("pass");
+        final SimpleBindRequest bindDNBindRequest = LdapUserSearchSessionFactory.bindRequest(config.settings());
 
         try {
             // auth
             try (LdapSession ldap = session(sessionFactory, user, userPass)) {
-                assertConnectionCanReconnect(ldap.getConnection());
+                assertConnectionValid(ldap.getConnection(), bindDNBindRequest);
                 String dn = ldap.userDn();
                 assertThat(dn, containsString(user));
             }
 
             //lookup
             try (LdapSession ldap = unauthenticatedSession(sessionFactory, user)) {
-                assertConnectionCanReconnect(ldap.getConnection());
+                assertConnectionValid(ldap.getConnection(), bindDNBindRequest);
                 String dn = ldap.userDn();
                 assertThat(dn, containsString(user));
             }
@@ -307,18 +306,19 @@ public class LdapUserSearchSessionFactoryTests extends LdapTestCase {
 
         String user = "William Bush";
         SecureString userPass = new SecureString("pass");
+        final SimpleBindRequest bindDNBindRequest = LdapUserSearchSessionFactory.bindRequest(config.settings());
 
         try {
             //auth
             try (LdapSession ldap = session(sessionFactory, user, userPass)) {
-                assertConnectionCanReconnect(ldap.getConnection());
+                assertConnectionValid(ldap.getConnection(), bindDNBindRequest);
                 String dn = ldap.userDn();
                 assertThat(dn, containsString(user));
             }
 
             //lookup
             try (LdapSession ldap = unauthenticatedSession(sessionFactory, user)) {
-                assertConnectionCanReconnect(ldap.getConnection());
+                assertConnectionValid(ldap.getConnection(), bindDNBindRequest);
                 String dn = ldap.userDn();
                 assertThat(dn, containsString(user));
             }
@@ -383,18 +383,19 @@ public class LdapUserSearchSessionFactoryTests extends LdapTestCase {
 
         String user = "wbush";
         SecureString userPass = new SecureString("pass");
+        final SimpleBindRequest bindDNBindRequest = LdapUserSearchSessionFactory.bindRequest(config.settings());
 
         try {
             //auth
             try (LdapSession ldap = session(sessionFactory, user, userPass)) {
-                assertConnectionCanReconnect(ldap.getConnection());
+                assertConnectionValid(ldap.getConnection(), bindDNBindRequest);
                 String dn = ldap.userDn();
                 assertThat(dn, containsString("William Bush"));
             }
 
             //lookup
             try (LdapSession ldap = unauthenticatedSession(sessionFactory, user)) {
-                assertConnectionCanReconnect(ldap.getConnection());
+                assertConnectionValid(ldap.getConnection(), bindDNBindRequest);
                 String dn = ldap.userDn();
                 assertThat(dn, containsString("William Bush"));
             }

plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/support/LdapTestCase.java

@@ -7,11 +7,13 @@ package org.elasticsearch.xpack.security.authc.ldap.support;
 
 import com.unboundid.ldap.listener.InMemoryDirectoryServer;
 import com.unboundid.ldap.sdk.Attribute;
+import com.unboundid.ldap.sdk.BindRequest;
 import com.unboundid.ldap.sdk.LDAPConnection;
 import com.unboundid.ldap.sdk.LDAPConnectionPool;
 import com.unboundid.ldap.sdk.LDAPException;
 import com.unboundid.ldap.sdk.LDAPInterface;
 import com.unboundid.ldap.sdk.LDAPURL;
+import com.unboundid.ldap.sdk.SimpleBindRequest;
 
 import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.action.support.PlainActionFuture;
@@ -170,15 +172,20 @@ public abstract class LdapTestCase extends ESTestCase {
         return future.actionGet();
     }
 
-    protected static void assertConnectionCanReconnect(LDAPInterface conn) {
+    protected static void assertConnectionValid(LDAPInterface conn, SimpleBindRequest bindRequest) {
         AccessController.doPrivileged(new PrivilegedAction<Void>() {
             @Override
             public Void run() {
                 try {
                     if (conn instanceof LDAPConnection) {
+                        assertTrue(((LDAPConnection) conn).isConnected());
+                        assertEquals(bindRequest.getBindDN(),
+                                ((SimpleBindRequest)((LDAPConnection) conn).getLastBindRequest()).getBindDN());
                         ((LDAPConnection) conn).reconnect();
                     } else if (conn instanceof LDAPConnectionPool) {
                         try (LDAPConnection c = ((LDAPConnectionPool) conn).getConnection()) {
+                            assertTrue(c.isConnected());
+                            assertEquals(bindRequest.getBindDN(), ((SimpleBindRequest)c.getLastBindRequest()).getBindDN());
                             c.reconnect();
                         }
                     }