Fix building AD URL from domain name (#31849)

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

Author: tvernum

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;
@@ -62,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,
@@ -85,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);
@@ -102,7 +101,7 @@ class ActiveDirectorySessionFactory extends PoolingSessionFactory {
     @Override
     protected List<String> 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 +196,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;

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;
@@ -29,6 +31,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;
@@ -52,9 +55,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;
@@ -354,6 +359,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);
     }