From 5b4c56d6c49577a6fff869b78e25635076f1bfc9 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Fri, 6 Jul 2018 13:40:49 +1000 Subject: [PATCH 1/2] Fix building AD URL from domain name The steps to read the settings and build URLs happen in a non-obvious order, which meant that we would build the default URL (from the domain name, and port) before we'd actually read the port settings. This would cause the URL to always have a port of `0`. Relates: bccf988 --- .../ldap/ActiveDirectorySessionFactory.java | 8 +++- .../authc/ldap/ActiveDirectoryRealmTests.java | 47 +++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java index 295e00e19a8a9..0c2ffd501b570 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java @@ -11,6 +11,7 @@ import com.unboundid.ldap.sdk.LDAPException; import com.unboundid.ldap.sdk.LDAPInterface; import com.unboundid.ldap.sdk.SearchResultEntry; +import com.unboundid.ldap.sdk.ServerSet; import com.unboundid.ldap.sdk.SimpleBindRequest; import com.unboundid.ldap.sdk.controls.AuthorizationIdentityRequestControl; import org.apache.logging.log4j.Logger; @@ -102,7 +103,7 @@ class ActiveDirectorySessionFactory extends PoolingSessionFactory { @Override protected List getDefaultLdapUrls(Settings settings) { return Collections.singletonList("ldap://" + settings.get(ActiveDirectorySessionFactorySettings.AD_DOMAIN_NAME_SETTING) + - ":" + ldapPort); + ":" + ActiveDirectorySessionFactorySettings.AD_LDAP_PORT_SETTING.get(settings)); } @Override @@ -197,6 +198,11 @@ static String getBindDN(Settings settings) { return bindDN; } + // Exposed for testing + ServerSet getServerSet() { + return super.serverSet; + } + ADAuthenticator getADAuthenticator(String username) { if (username.indexOf('\\') > 0) { return downLevelADAuthenticator; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectoryRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectoryRealmTests.java index bcd7996e32a8c..739523795e7c5 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectoryRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectoryRealmTests.java @@ -8,8 +8,10 @@ import com.unboundid.ldap.listener.InMemoryDirectoryServer; import com.unboundid.ldap.listener.InMemoryDirectoryServerConfig; import com.unboundid.ldap.sdk.Attribute; +import com.unboundid.ldap.sdk.FailoverServerSet; import com.unboundid.ldap.sdk.LDAPException; import com.unboundid.ldap.sdk.LDAPURL; +import com.unboundid.ldap.sdk.SingleServerSet; import com.unboundid.ldap.sdk.schema.Schema; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.PlainActionFuture; @@ -28,6 +30,7 @@ import org.elasticsearch.xpack.core.security.authc.ldap.ActiveDirectorySessionFactorySettings; import org.elasticsearch.xpack.core.security.authc.ldap.LdapRealmSettings; import org.elasticsearch.xpack.core.security.authc.ldap.PoolingSessionFactorySettings; +import org.elasticsearch.xpack.core.security.authc.ldap.support.SessionFactorySettings; import org.elasticsearch.xpack.core.security.authc.support.CachingUsernamePasswordRealmSettings; import org.elasticsearch.xpack.core.security.authc.support.DnRoleMapperSettings; import org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken; @@ -51,9 +54,11 @@ import static org.elasticsearch.xpack.core.security.authc.ldap.support.SessionFactorySettings.URLS_SETTING; import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.arrayContainingInAnyOrder; +import static org.hamcrest.Matchers.arrayWithSize; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasEntry; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.mockito.Matchers.any; @@ -355,6 +360,48 @@ public void testCustomSearchFilters() throws Exception { assertEquals("(objectClass=down level)", sessionFactory.downLevelADAuthenticator.getUserSearchFilter()); } + public void testBuildUrlFromDomainNameAndDefaultPort() throws Exception { + Settings settings = Settings.builder() + .put(ActiveDirectorySessionFactorySettings.AD_DOMAIN_NAME_SETTING, "ad.test.elasticsearch.com") + .build(); + RealmConfig config = new RealmConfig("testBuildUrlFromDomainNameAndDefaultPort", settings, globalSettings, + TestEnvironment.newEnvironment(globalSettings), new ThreadContext(globalSettings)); + ActiveDirectorySessionFactory sessionFactory = new ActiveDirectorySessionFactory(config, sslService, threadPool); + assertSingleLdapServer(sessionFactory, "ad.test.elasticsearch.com", 389); + } + + public void testBuildUrlFromDomainNameAndCustomPort() throws Exception { + Settings settings = Settings.builder() + .put(ActiveDirectorySessionFactorySettings.AD_DOMAIN_NAME_SETTING, "ad.test.elasticsearch.com") + .put(ActiveDirectorySessionFactorySettings.AD_LDAP_PORT_SETTING.getKey(), 10389) + .build(); + RealmConfig config = new RealmConfig("testBuildUrlFromDomainNameAndCustomPort", settings, globalSettings, + TestEnvironment.newEnvironment(globalSettings), new ThreadContext(globalSettings)); + ActiveDirectorySessionFactory sessionFactory = new ActiveDirectorySessionFactory(config, sslService, threadPool); + assertSingleLdapServer(sessionFactory, "ad.test.elasticsearch.com", 10389); + } + + public void testUrlConfiguredInSettings() throws Exception { + Settings settings = Settings.builder() + .put(ActiveDirectorySessionFactorySettings.AD_DOMAIN_NAME_SETTING, "ad.test.elasticsearch.com") + .put(SessionFactorySettings.URLS_SETTING, "ldap://ad01.testing.elastic.co:20389/") + .build(); + RealmConfig config = new RealmConfig("testBuildUrlFromDomainNameAndCustomPort", settings, globalSettings, + TestEnvironment.newEnvironment(globalSettings), new ThreadContext(globalSettings)); + ActiveDirectorySessionFactory sessionFactory = new ActiveDirectorySessionFactory(config, sslService, threadPool); + assertSingleLdapServer(sessionFactory, "ad01.testing.elastic.co", 20389); + } + + private void assertSingleLdapServer(ActiveDirectorySessionFactory sessionFactory, String hostname, int port) { + assertThat(sessionFactory.getServerSet(), instanceOf(FailoverServerSet.class)); + FailoverServerSet fss = (FailoverServerSet) sessionFactory.getServerSet(); + assertThat(fss.getServerSets(), arrayWithSize(1)); + assertThat(fss.getServerSets()[0], instanceOf(SingleServerSet.class)); + SingleServerSet sss = (SingleServerSet) fss.getServerSets()[0]; + assertThat(sss.getAddress(), equalTo(hostname)); + assertThat(sss.getPort(), equalTo(port)); + } + private Settings settings() throws Exception { return settings(Settings.EMPTY); } From b971b7262c1dd57b7661b620c903921f2d960752 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Mon, 9 Jul 2018 13:45:16 +1000 Subject: [PATCH 2/2] Remove unnecessary field --- .../security/authc/ldap/ActiveDirectorySessionFactory.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java index 0c2ffd501b570..d175e1b229312 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java @@ -63,8 +63,6 @@ class ActiveDirectorySessionFactory extends PoolingSessionFactory { final DownLevelADAuthenticator downLevelADAuthenticator; final UpnADAuthenticator upnADAuthenticator; - private final int ldapPort; - ActiveDirectorySessionFactory(RealmConfig config, SSLService sslService, ThreadPool threadPool) throws LDAPException { super(config, sslService, new ActiveDirectoryGroupsResolver(config.settings()), ActiveDirectorySessionFactorySettings.POOL_ENABLED, @@ -86,7 +84,7 @@ class ActiveDirectorySessionFactory extends PoolingSessionFactory { + "] setting for active directory"); } String domainDN = buildDnFromDomain(domainName); - ldapPort = ActiveDirectorySessionFactorySettings.AD_LDAP_PORT_SETTING.get(settings); + final int ldapPort = ActiveDirectorySessionFactorySettings.AD_LDAP_PORT_SETTING.get(settings); final int ldapsPort = ActiveDirectorySessionFactorySettings.AD_LDAPS_PORT_SETTING.get(settings); final int gcLdapPort = ActiveDirectorySessionFactorySettings.AD_GC_LDAP_PORT_SETTING.get(settings); final int gcLdapsPort = ActiveDirectorySessionFactorySettings.AD_GC_LDAPS_PORT_SETTING.get(settings);