ldap: Changed LdapSslSocketFactory method of setting the static factory, plus miscellaneous cleanup

Original commit: elastic/x-pack-elasticsearch@1e1ba2aa7d

src/main/java/org/elasticsearch/shield/authc/ldap/LdapModule.java

@@ -27,8 +27,12 @@ public class LdapModule extends AbstractShieldModule.Node {
     @Override
     protected void configureNode() {
         if (enabled) {
+            /* This socket factory needs to be configured before any LDAP connections are created.  LDAP configuration
+            for JNDI invokes a static getSocketFactory method from LdapSslSocketFactory.  This doesn't mesh well with
+            guice so we set the factory here during startup.  See LdapSslSocketFactory for more details. */
+            LdapSslSocketFactory.init(settings);
+
             bind(Realm.class).annotatedWith(named(LdapRealm.TYPE)).to(LdapRealm.class).asEagerSingleton();
-            bind(LdapSslSocketFactory.class).asEagerSingleton();
             bind(LdapGroupToRoleMapper.class).asEagerSingleton();
             String mode = settings.getComponentSettings(LdapModule.class).get("mode", "ldap");
             if ("ldap".equals(mode)) {

src/main/java/org/elasticsearch/shield/authc/ldap/LdapSslSocketFactory.java

@@ -5,9 +5,7 @@
  */
 package org.elasticsearch.shield.authc.ldap;
 
-import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.common.collect.ImmutableMap;
-import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.logging.ESLogger;
 import org.elasticsearch.common.logging.ESLoggerFactory;
 import org.elasticsearch.common.settings.Settings;
@@ -22,23 +20,28 @@ import java.util.Locale;
 
 /**
  * This factory is needed for JNDI configuration for LDAP connections.  It wraps a single instance of a static
- * factory that is initiated by the settings constructor
+ * factory that is initiated by the settings constructor.  JNDI uses reflection to call the getDefault() static method
+ * then checks to make sure that the factory returned is an LdapSslSocketFactory.  Because of this we have to wrap
+ * the socket factory
+ *
+ * http://docs.oracle.com/javase/tutorial/jndi/ldap/ssl.html
  */
 public class LdapSslSocketFactory extends SocketFactory {
-    private static SocketFactory socketFactory;
+
     private static ESLogger logger = ESLoggerFactory.getLogger(LdapSslSocketFactory.class.getName());
+    private static LdapSslSocketFactory instance;
 
     /**
-     * This should only be invoked once to establish a static instance.
+     * This should only be invoked once to establish a static instance that will be used for each constructor.
      */
-    @Inject
+    public static void init(Settings settings) {
-    public LdapSslSocketFactory(Settings settings) {
+        if (instance != null) {
-        if (socketFactory == null) {
             logger.error("LdapSslSocketFactory already configured, this change could lead to threading issues");
         }
-        Settings componentSettings = settings.getComponentSettings(getClass());
+
+        Settings componentSettings = settings.getComponentSettings(LdapSslSocketFactory.class);
         SSLTrustConfig sslConfig = new SSLTrustConfig(componentSettings, settings.getByPrefix("shield.ssl."));
-        socketFactory = sslConfig.createSSLSocketFactory();
+        instance = new LdapSslSocketFactory(sslConfig.createSSLSocketFactory());
     }
 
     /**
@@ -46,20 +49,21 @@ public class LdapSslSocketFactory extends SocketFactory {
      * @return
      */
     public static SocketFactory getDefault() {
-        return new LdapSslSocketFactory();
+        assert instance != null;
+        return instance;
     }
 
     public static boolean initialized() {
-        return socketFactory != null;
+        return instance != null;
     }
 
-    LdapSslSocketFactory(){
+    final private SocketFactory socketFactory;
-        if (socketFactory == null){
+
-            throw new ElasticsearchException("Attempt to construct an uninitialized LdapSslSocketFactory");
+    private LdapSslSocketFactory(SocketFactory wrappedSocketFactory){
-        }
+        socketFactory = wrappedSocketFactory;
     }
 
-    //The following methods are all wrappers around the static instance of socketFactory
+    //The following methods are all wrappers around the instance of socketFactory
 
     @Override
     public Socket createSocket(String s, int i) throws IOException {
@@ -82,11 +86,11 @@ public class LdapSslSocketFactory extends SocketFactory {
     }
 
     /**
-     * If one of the ldapUrls are SSL this will set the LdapSslSocketFactory as a socket provider on the
+     * If one of the ldapUrls are SSL this will set the LdapSslSocketFactory as a socket provider on the builder
      * @param ldapUrls
      * @param builder set of jndi properties, that will
      */
-    public static ImmutableMap.Builder<String, Serializable> configureJndiSSL(String[] ldapUrls, ImmutableMap.Builder<String, Serializable> builder) {
+    public static void configureJndiSSL(String[] ldapUrls, ImmutableMap.Builder<String, Serializable> builder) {
         boolean needsSSL = false;
         for(String url: ldapUrls){
             if (url.toLowerCase(Locale.getDefault()).startsWith("ldaps://")) {
@@ -95,14 +99,10 @@ public class LdapSslSocketFactory extends SocketFactory {
             }
         }
         if (needsSSL) {
-            if (socketFactory != null) {
+            assert instance != null : "LdapSslSocketFactory not initialized and won't be used for LDAP connections";
-                builder.put("java.naming.ldap.factory.socket", LdapSslSocketFactory.class.getName());
+            builder.put("java.naming.ldap.factory.socket", LdapSslSocketFactory.class.getName());
-            } else {
-                logger.warn("LdapSslSocketFactory not initialized and won't be used for LDAP connections");
-            }
         } else {
             logger.debug("LdapSslSocketFactory not used for LDAP connections");
         }
-        return builder;
     }
 }

src/test/java/org/elasticsearch/shield/authc/ldap/ActiveDirectoryFactoryTests.java

@@ -7,7 +7,6 @@ package org.elasticsearch.shield.authc.ldap;
 
 import org.elasticsearch.common.settings.ImmutableSettings;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.shield.authc.support.SecuredString;
 import org.elasticsearch.shield.authc.support.SecuredStringTests;
 import org.elasticsearch.test.ElasticsearchTestCase;
 import org.elasticsearch.test.junit.annotations.Network;
@@ -19,10 +18,9 @@ import java.io.File;
 import java.net.URISyntaxException;
 import java.util.List;
 
-import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.Matchers.*;
-import static org.hamcrest.Matchers.containsInAnyOrder;
-import static org.hamcrest.core.IsCollectionContaining.hasItem;
 
+@Network
 public class ActiveDirectoryFactoryTests extends ElasticsearchTestCase {
     public static final String AD_LDAP_URL = "ldaps://54.213.145.20:636";
     public static final String PASSWORD = "NickFuryHeartsES";
@@ -31,13 +29,13 @@ public class ActiveDirectoryFactoryTests extends ElasticsearchTestCase {
 
     @BeforeClass
     public static void setTrustStore() throws URISyntaxException {
-        new LdapSslSocketFactory(ImmutableSettings.builder()
+        LdapSslSocketFactory.init(ImmutableSettings.builder()
                 .put(SETTINGS_PREFIX + "truststore", new File(LdapConnectionTests.class.getResource("ldaptrust.jks").toURI()))
                 .build());
     }
 
 
-    @Test @Network
+    @Test
     public void testAdAuth() {
         ActiveDirectoryConnectionFactory connectionFactory = new ActiveDirectoryConnectionFactory(
                 buildAdSettings(AD_LDAP_URL, AD_DOMAIN));
@@ -54,10 +52,9 @@ public class ActiveDirectoryFactoryTests extends ElasticsearchTestCase {
                 containsString("Philanthropists"),
                 containsString("Avengers"),
                 containsString("SHIELD")));
-
     }
 
-    @Test @Network
+    @Test
     public void testAdAuth_avengers() {
         ActiveDirectoryConnectionFactory connectionFactory = new ActiveDirectoryConnectionFactory(
                 buildAdSettings(AD_LDAP_URL, AD_DOMAIN));
@@ -70,7 +67,7 @@ public class ActiveDirectoryFactoryTests extends ElasticsearchTestCase {
         }
     }
 
-    @Test @Network
+    @Test
     public void testAdAuth_specificUserSearch() {
         ActiveDirectoryConnectionFactory connectionFactory = new ActiveDirectoryConnectionFactory(
                 buildAdSettings(AD_LDAP_URL, AD_DOMAIN,
@@ -81,17 +78,15 @@ public class ActiveDirectoryFactoryTests extends ElasticsearchTestCase {
         String userDN = ldap.getAuthenticatedUserDn();
 
         List<String> groups = ldap.getGroupsFromUserAttrs(userDN);
-        System.out.println("groups: "+groups);
 
         assertThat(groups, containsInAnyOrder(
                 containsString("Avengers"),
                 containsString("SHIELD"),
                 containsString("Geniuses"),
                 containsString("Philanthropists")));
-
     }
 
-    @Test @Network
+    @Test
     public void testAD_standardLdapConnection(){
         String groupSearchBase = "DC=ad,DC=test,DC=elasticsearch,DC=com";
         String userTemplate = "CN={0},CN=Users,DC=ad,DC=test,DC=elasticsearch,DC=com";
@@ -105,8 +100,6 @@ public class ActiveDirectoryFactoryTests extends ElasticsearchTestCase {
         List<String> groups = ldap.getGroupsFromUserAttrs(ldap.getAuthenticatedUserDn());
         List<String> groups2 = ldap.getGroupsFromSearch(ldap.getAuthenticatedUserDn());
 
-        System.out.println(groups);
-        System.out.println(groups2);
         assertThat(groups, containsInAnyOrder(
                 containsString("Avengers"),
                 containsString("SHIELD"),

src/test/java/org/elasticsearch/shield/authc/ldap/LdapConnectionTests.java

@@ -12,8 +12,6 @@ import org.elasticsearch.test.ElasticsearchTestCase;
 import org.junit.Rule;
 import org.junit.Test;
 
-import java.io.File;
-import java.net.URISyntaxException;
 import java.util.List;
 import java.util.Map;
 
@@ -76,7 +74,6 @@ public class LdapConnectionTests extends LdapTest {
 
         LdapConnection ldap = ldapFac.bind(user, userPass);
         List<String> groups = ldap.getGroupsFromSearch(ldap.getAuthenticatedUserDn());
-        System.out.println("groups:"+groups);
         assertThat(groups, contains("cn=HMS Lydia,ou=crews,ou=groups,o=sevenSeas"));
     }
 
@@ -92,7 +89,6 @@ public class LdapConnectionTests extends LdapTest {
         LdapConnection ldap = ldapFac.bind(user, SecuredStringTests.build("pass"));
 
         List<String> groups = ldap.getGroupsFromSearch(ldap.getAuthenticatedUserDn());
-        System.out.println("groups:"+groups);
         assertThat(groups, contains("cn=HMS Lydia,ou=crews,ou=groups,o=sevenSeas"));
     }
 }

src/test/java/org/elasticsearch/shield/authc/ldap/OpenLdapTests.java

@@ -6,6 +6,7 @@
 package org.elasticsearch.shield.authc.ldap;
 
 import org.elasticsearch.common.settings.ImmutableSettings;
+import org.elasticsearch.shield.authc.support.SecuredStringTests;
 import org.elasticsearch.test.ElasticsearchTestCase;
 import org.elasticsearch.test.junit.annotations.Network;
 import org.junit.BeforeClass;
@@ -17,6 +18,7 @@ import java.net.URISyntaxException;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.hasItem;
 
+@Network
 public class OpenLdapTests extends ElasticsearchTestCase {
     public static final String OPEN_LDAP_URL = "ldaps://54.200.235.244:636";
     public static final String PASSWORD = "NickFuryHeartsES";
@@ -25,12 +27,12 @@ public class OpenLdapTests extends ElasticsearchTestCase {
     @BeforeClass
     public static void setTrustStore() throws URISyntaxException {
         //LdapModule will set this up as a singleton normally
-        new LdapSslSocketFactory(ImmutableSettings.builder()
+        LdapSslSocketFactory.init(ImmutableSettings.builder()
                 .put(SETTINGS_PREFIX + "truststore", new File(LdapConnectionTests.class.getResource("ldaptrust.jks").toURI()))
                 .build());
     }
 
-    @Test @Network
+    @Test
     public void test_standardLdapConnection_uid(){
         //openldap does not use cn as naming attributes by default
 
@@ -42,7 +44,7 @@ public class OpenLdapTests extends ElasticsearchTestCase {
 
         String[] users = new String[]{"blackwidow", "cap", "hawkeye", "hulk", "ironman", "thor"};
         for(String user: users) {
-            LdapConnection ldap = connectionFactory.bind(user, PASSWORD.toCharArray());
+            LdapConnection ldap = connectionFactory.bind(user, SecuredStringTests.build(PASSWORD));
             assertThat(ldap.getGroups(), hasItem(containsString("Avengers")));
             ldap.close();
         }

tests.policy

@@ -39,6 +39,4 @@ grant {
   //this shouldn't be in a production environment, just to run tests:
   permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
   permission java.lang.RuntimePermission "setDefaultUncaughtExceptionHandler";
-
-  permission javax.net.ssl.SSLPermission "setDefaultSSLContext";
 };