diff --git a/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java b/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java index e4eb389c5b8..bf167fe768e 100644 --- a/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java +++ b/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java @@ -285,7 +285,7 @@ class ActiveDirectorySessionFactory extends PoolingSessionFactory { ActionListener listener) { final byte[] passwordBytes = CharArrays.toUtf8Bytes(password.getChars()); final SimpleBindRequest bind = new SimpleBindRequest(bindUsername(username), passwordBytes); - LdapUtils.maybeForkThenBind(pool, bind, threadPool, new ActionRunnable(listener) { + LdapUtils.maybeForkThenBindAndRevert(pool, bind, threadPool, new ActionRunnable(listener) { @Override protected void doRun() throws Exception { searchForDN(pool, username, password, Math.toIntExact(timeout.seconds()), ActionListener.wrap((entry) -> { diff --git a/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/LdapUserSearchSessionFactory.java b/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/LdapUserSearchSessionFactory.java index ebd0982a25e..59cf5fd99ca 100644 --- a/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/LdapUserSearchSessionFactory.java +++ b/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); - LdapUtils.maybeForkThenBind(connectionPool, bind, threadPool, new ActionRunnable(listener) { + final SimpleBindRequest bind = new SimpleBindRequest(dn, passwordBytes); + LdapUtils.maybeForkThenBindAndRevert(connectionPool, bind, threadPool, new ActionRunnable(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, - timeout, entry.getAttributes())); + LdapUtils.maybeForkThenBind(connection, bind, threadPool, new AbstractRunnable() { + + @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 diff --git a/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/support/LdapUtils.java b/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/support/LdapUtils.java index 373aa0eb801..90cecd1e48a 100644 --- a/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/support/LdapUtils.java +++ b/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} - * connection or connection pool. The connection authentication status changes, - * see {@code LDAPConnection#bind}; in case of a connection pool the bind - * authentication status is reverted, so that the connection can be safely - * returned to the pool, see: - * {@code LDAPConnectionPool#bindAndRevertAuthentication}. + * If necessary, fork before executing the runnable. A deadlock will happen if + * the same thread which handles bind responses blocks on the bind call, waiting + * for the response which he itself should handle. + */ + private static void maybeForkAndRun(ThreadPool threadPool, Runnable runnable) { + 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}), - * the thread will be blocked until it is interrupted by something else - * such as a timeout timer. - * Do not call bind outside of this method. + * 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. Do not call bind outside this method or + * {@link LdapUtils#maybeForkThenBind(LDAPConnection, BindRequest, ThreadPool, AbstractRunnable)} * - * @param ldap - * The LDAP connection or connection pool on which to submit the bind - * operation. + * @param ldapPool + * The LDAP connection pool on which to submit the bind 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, - AbstractRunnable runnable) { - Runnable bindRunnable = new AbstractRunnable() { + public static void maybeForkThenBindAndRevert(LDAPConnectionPool ldapPool, 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 { - if (ldap instanceof LDAPConnectionPool) { - privilegedConnect(() -> ((LDAPConnectionPool) ldap).bindAndRevertAuthentication(bind)); - } else if (ldap instanceof LDAPConnection) { - ((LDAPConnection) ldap).bind(bind); - } else { - throw new IllegalArgumentException("unsupported LDAPInterface implementation: " + ldap); - } + privilegedConnect(() -> ldapPool.bindAndRevertAuthentication(bind.duplicate())); 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. Do not call bind 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); } /** diff --git a/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/LdapSessionFactoryTests.java b/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/LdapSessionFactoryTests.java index e32fb4ead7d..b96da799e48 100644 --- a/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/LdapSessionFactoryTests.java +++ b/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 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 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 groups = groups(ldap); assertThat(groups.size(), is(1)); assertThat(groups, contains("cn=HMS Lydia,ou=crews,ou=groups,o=sevenSeas")); diff --git a/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/LdapUserSearchSessionFactoryTests.java b/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/LdapUserSearchSessionFactoryTests.java index d157a887c3f..67d080ccbae 100644 --- a/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/LdapUserSearchSessionFactoryTests.java +++ b/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")); } diff --git a/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/support/LdapTestCase.java b/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/support/LdapTestCase.java index b588381bece..c802812038c 100644 --- a/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/support/LdapTestCase.java +++ b/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() { @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(); } }