Do not throw on initial LDAP connection failure

cert-ldap-login-module-common/src/main/java/org/esbtools/auth/ldap/LdapConfiguration.java

@@ -34,6 +34,7 @@ public class LdapConfiguration {
     private Integer responseTimeoutMS = 3000; // time to wait until receiving response from ldap
     private boolean debug = false;
     private boolean keepAlive = true;
+    private Integer retryIntervalSeconds = 5;
 
     public LdapConfiguration server (String server) {
         this.server = server;
@@ -151,4 +152,31 @@ public LdapConfiguration poolMaxConnectionAgeMS(Integer poolMaxConnectionAgeMS)
         this.poolMaxConnectionAgeMS = poolMaxConnectionAgeMS;
         return this;
     }
+
+    public Integer getRetryIntervalSeconds() {
+        return retryIntervalSeconds;
+    }
+
+    public LdapConfiguration retryIntervalSeconds(Integer retryIntervalSeconds) {
+        this.retryIntervalSeconds = retryIntervalSeconds;
+        return this;
+    }
+
+    @Override
+    public String toString() {
+        return "LdapConfiguration{" +
+            "server='" + server + '\'' +
+            ", port=" + port +
+            ", bindDn='" + bindDn + '\'' +
+            ", useSSL=" + useSSL +
+            ", trustStore='" + trustStore + '\'' +
+            ", poolSize=" + poolSize +
+            ", poolMaxConnectionAgeMS=" + poolMaxConnectionAgeMS +
+            ", connectionTimeoutMS=" + connectionTimeoutMS +
+            ", responseTimeoutMS=" + responseTimeoutMS +
+            ", debug=" + debug +
+            ", keepAlive=" + keepAlive +
+            ", retryIntervalSeconds=" + retryIntervalSeconds +
+            '}';
+    }
 }

cert-ldap-login-module-common/src/main/java/org/esbtools/auth/ldap/LdapRolesProvider.java

@@ -19,7 +19,6 @@
 package org.esbtools.auth.ldap;
 
 import org.esbtools.auth.util.RolesProvider;
-import com.unboundid.ldap.sdk.BindRequest;
 import com.unboundid.ldap.sdk.BindResult;
 import com.unboundid.ldap.sdk.DN;
 import com.unboundid.ldap.sdk.LDAPConnection;
@@ -32,19 +31,22 @@
 import com.unboundid.ldap.sdk.SearchResult;
 import com.unboundid.ldap.sdk.SearchResultEntry;
 import com.unboundid.ldap.sdk.SearchScope;
-import com.unboundid.ldap.sdk.SimpleBindRequest;
 import com.unboundid.util.DebugType;
 import com.unboundid.util.ssl.SSLUtil;
 import com.unboundid.util.ssl.TrustStoreTrustManager;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.net.ssl.SSLSocketFactory;
+import java.security.GeneralSecurityException;
+import java.time.Duration;
+import java.time.Instant;
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 
 /**
@@ -59,29 +61,51 @@
  *
  */
 public class LdapRolesProvider implements RolesProvider {
-    private final Logger LOGGER = LoggerFactory.getLogger(LdapRolesProvider.class);
+    private static final Logger LOGGER = LoggerFactory.getLogger(LdapRolesProvider.class);
 
-    private String searchBase;
+    private final String searchBase;
 
-    private LdapConfiguration ldapConfiguration;
+    private final LdapConfiguration ldapConfiguration;
+
+    private final LDAPConnection ldapConnection;
 
     // Connection pool needs to be a singleton
+    /**
+     * @{code null} until {@link #connectIfNeeded()} is called and successful.
+     */
     private LDAPConnectionPool connectionPool;
+    private volatile LDAPException connectionException;
+    private volatile Instant lastConnectionAttempt;
+    private final AtomicBoolean attemptingConnect = new AtomicBoolean(false);
 
     public LdapRolesProvider(String searchBase, LdapConfiguration ldapConfiguration) throws Exception {
+        this(searchBase, ldapConfiguration, true);
+    }
+
+    public LdapRolesProvider(String searchBase, LdapConfiguration ldapConfiguration,
+            boolean failFast) throws Exception {
         LOGGER.debug("Creating esbtoolsLdapRoleProvider");
 
         Objects.requireNonNull(searchBase);
         Objects.requireNonNull(ldapConfiguration);
 
         this.searchBase = searchBase;
         this.ldapConfiguration = ldapConfiguration;
-
-        initializeFromConfiguration();
+        this.ldapConnection = getLdapConnection(ldapConfiguration);
+
+        try {
+            connectIfNeeded();
+        } catch (LDAPException e) {
+            if (failFast) {
+                throw e;
+            } else {
+                LOGGER.warn("Failed to connect to LDAP server, will retry on next lookup after " +
+                    "{} seconds.", ldapConfiguration.getRetryIntervalSeconds(), e);
+            }
+        }
     }
 
-    private void initializeFromConfiguration() throws Exception {
-
+    private static LDAPConnection getLdapConnection(LdapConfiguration ldapConfiguration) throws GeneralSecurityException {
         if (ldapConfiguration.isDebug()) {
             // bridge java.util.Logger output to log4j
             System.setProperty("java.util.logging.manager", "org.apache.logging.log4j.jul.LogManager");
@@ -103,49 +127,86 @@ private void initializeFromConfiguration() throws Exception {
         // A flag that indicates whether to use the SO_KEEPALIVE socket option to attempt to more quickly detect when idle TCP connections have been lost or to prevent them from being unexpectedly closed by intermediate network hardware. By default, the SO_KEEPALIVE socket option will be used.
         options.setUseKeepAlive(ldapConfiguration.isKeepAlive());
 
-
-        if(ldapConfiguration.getUseSSL()) {
+        if (ldapConfiguration.getUseSSL()) {
             TrustStoreTrustManager trustStoreTrustManager = new TrustStoreTrustManager(
-                    ldapConfiguration.getTrustStore(),
-                    ldapConfiguration.getTrustStorePassword().toCharArray(),
-                    "JKS",
-                    true);
+                ldapConfiguration.getTrustStore(),
+                ldapConfiguration.getTrustStorePassword().toCharArray(),
+                "JKS",
+                true);
             SSLSocketFactory socketFactory = new SSLUtil(trustStoreTrustManager).createSSLSocketFactory();
 
-            ldapConnection = new LDAPConnection(
-                    socketFactory,
-                    options,
-                    ldapConfiguration.getServer(),
-                    ldapConfiguration.getPort(),
-                    ldapConfiguration.getBindDn(),
-                    ldapConfiguration.getBindDNPwd()
-            );
+            ldapConnection = new LDAPConnection(socketFactory, options);
         } else {
             LOGGER.warn("Not using SSL to connect to ldap. This is very insecure - do not use in prod environments!");
 
-            ldapConnection = new LDAPConnection(
-                    options,
-                    ldapConfiguration.getServer(),
-                    ldapConfiguration.getPort(),
-                    ldapConfiguration.getBindDn(),
-                    ldapConfiguration.getBindDNPwd()
-            );
+            ldapConnection = new LDAPConnection(options);
+        }
+
+        return ldapConnection;
+    }
+
+    private void connectIfNeeded() throws LDAPException {
+        if (connectionPool != null) {
+            return;
         }
 
-        BindRequest bindRequest = new SimpleBindRequest(ldapConfiguration.getBindDn(), ldapConfiguration.getBindDNPwd());
-        BindResult bindResult = ldapConnection.bind(bindRequest);
+        if (!readyForConnectionAttempt()) {
+            // It's too early to retry connecting again.
+            throw lastSeenConnectionException();
+        }
 
-        if (bindResult.getResultCode() != ResultCode.SUCCESS) {
-            LOGGER.error("Error binding to LDAP" + bindResult.getResultCode());
-            throw new LDAPException(bindResult.getResultCode(), "Error binding to LDAP");
+        // Multiple threads can get to this point, but establishing a connection is not thread safe
+        // (see LDAPConnection#connect), so we must synchronize access. Instead of blocking all
+        // threads except the one that makes it to this point and wins the race to connect, we fail
+        // the "loser" threads fast like when the connection retry interval has not been met, to
+        // prevent potentially many requests from waiting potentially a long wait. We do this via
+        // atomic compare and swap: see if attemptingConnect flag is false, and if so set it as one
+        // atomic operation. If attemptingConnect is true, we fail fast.
+        if (!attemptingConnect.compareAndSet(/*expect*/ false, /*update*/ true)) {
+            // attemptingConnect was true, which means another thread is connecting. Fail fast now
+            // instead of blocking.
+            throw lastSeenConnectionException();
         }
 
-        connectionPool = new LDAPConnectionPool(ldapConnection, ldapConfiguration.getPoolSize());
-        connectionPool.setMaxConnectionAgeMillis(ldapConfiguration.getPoolMaxConnectionAgeMS());
-        LOGGER.info("Initialized LDAPConnectionPool: poolSize={}, poolMaxAge={}, connectionTimeout={}, responseTimeout={}, debug={}, keepAlive={}.",
+        try {
+            if (lastConnectionAttempt != null) {
+                LOGGER.info("Connection retry interval ({} seconds) passed, " +
+                    "attempting connection recovery to LDAP at {}:{}",
+                    ldapConfiguration.getRetryIntervalSeconds(),
+                    ldapConfiguration.getServer(),
+                    ldapConfiguration.getPort());
+            }
+
+            lastConnectionAttempt = Instant.now();
+            BindResult bindResult = null;
+
+            if (!ldapConnection.isConnected()) {
+                ldapConnection.connect(
+                    ldapConfiguration.getServer(), ldapConfiguration.getPort());
+                bindResult = ldapConnection.bind(
+                    ldapConfiguration.getBindDn(), ldapConfiguration.getBindDNPwd());
+            } else if (ldapConnection.getLastBindRequest() == null) {
+                bindResult = ldapConnection.bind(
+                    ldapConfiguration.getBindDn(), ldapConfiguration.getBindDNPwd());
+            }
+
+            if (bindResult != null && bindResult.getResultCode() != ResultCode.SUCCESS) {
+                LOGGER.error("Error binding to LDAP" + bindResult.getResultCode());
+                throw new LDAPException(bindResult.getResultCode(), "Error binding to LDAP");
+            }
+
+            connectionPool = new LDAPConnectionPool(ldapConnection, ldapConfiguration.getPoolSize());
+            connectionPool.setMaxConnectionAgeMillis(ldapConfiguration.getPoolMaxConnectionAgeMS());
+
+            LOGGER.info("Initialized LDAPConnectionPool: poolSize={}, poolMaxAge={}, connectionTimeout={}, responseTimeout={}, debug={}, keepAlive={}.",
                 ldapConfiguration.getPoolSize(), ldapConfiguration.getPoolMaxConnectionAgeMS(), ldapConfiguration.getConnectionTimeoutMS(), ldapConfiguration.getResponseTimeoutMS(),
                 ldapConfiguration.isDebug(), ldapConfiguration.isKeepAlive());
-
+        } catch (LDAPException e) {
+            connectionException = e;
+            throw e;
+        } finally {
+            attemptingConnect.set(false);
+        }
     }
 
     @Override
@@ -154,6 +215,8 @@ public Set<String> getUserRoles(String username) throws Exception {
 
         Objects.requireNonNull(username);
 
+        connectIfNeeded();
+
         Set<String> roles = new HashSet<>();
 
         String filter = "(uid=" + username + ")";
@@ -187,4 +250,25 @@ public Set<String> getUserRoles(String username) throws Exception {
         return roles;
     }
 
+    private LDAPException lastSeenConnectionException() {
+        if (connectionException == null) {
+            throw new IllegalStateException("Expected connection exception, but was null. There " +
+                "was probably a problem connecting but the exception is unknown. Please report " +
+                "this bug to maintainers: " +
+                "https://github.com/esbtools/cert-ldap-login-module/issues/new");
+        }
+
+        return connectionException;
+    }
+
+    private boolean readyForConnectionAttempt() {
+        if (lastConnectionAttempt == null) {
+            return true;
+        }
+
+        Duration timeSinceLastAttempt = Duration.between(lastConnectionAttempt, Instant.now());
+        Duration retryInterval = Duration.ofSeconds(ldapConfiguration.getRetryIntervalSeconds());
+
+        return timeSinceLastAttempt.compareTo(retryInterval) >= 0;
+    }
 }

cert-ldap-login-module-common/src/test/java/org/esbtools/auth/ldap/LdapRoleProviderRecoveryTest.java

@@ -0,0 +1,83 @@
+package org.esbtools.auth.ldap;
+
+import static org.junit.Assert.fail;
+
+import com.redhat.lightblue.ldap.test.LdapServerExternalResource;
+
+import com.unboundid.ldap.listener.InMemoryDirectoryServer;
+import com.unboundid.ldap.listener.InMemoryDirectoryServerConfig;
+import com.unboundid.ldap.listener.InMemoryListenerConfig;
+import com.unboundid.ldap.sdk.Attribute;
+import com.unboundid.ldap.sdk.LDAPException;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+public class LdapRoleProviderRecoveryTest {
+  private final LdapConfiguration ldapConfiguration = new LdapConfiguration()
+      .bindDn("uid=admin,dc=com")
+      .bindDNPwd("password")
+      .server("localhost")
+      .port(LdapServerExternalResource.DEFAULT_PORT)
+      .retryIntervalSeconds(1);
+
+  private InMemoryDirectoryServer ldapServer;
+
+  @Before
+  public void createButDoNotStartLdapServer() throws LDAPException {
+    InMemoryDirectoryServerConfig config = new InMemoryDirectoryServerConfig("dc=com");
+    config.addAdditionalBindCredentials("uid=admin,dc=com", "password");
+
+    InMemoryListenerConfig listenerConfig = new InMemoryListenerConfig(
+        "test", null, LdapServerExternalResource.DEFAULT_PORT, null, null, null);
+    config.setListenerConfigs(listenerConfig);
+    config.setSchema(null);
+
+    ldapServer = new InMemoryDirectoryServer(config);
+
+    ldapServer.add("dc=com", new Attribute("objectClass", "top"),
+        new Attribute("objectClass", "domain"),
+        new Attribute("dc", "com"));
+  }
+
+  @After
+  public void stopLdapServer() {
+    ldapServer.shutDown(true);
+  }
+
+  @Test
+  public void eventuallyConnectsToLdapIfNotFailFast() throws Exception {
+    LdapRolesProvider provider = null;
+
+    try {
+      provider = new LdapRolesProvider(
+          LdapServerExternalResource.DEFAULT_BASE_DN,
+          ldapConfiguration, false);
+    } catch (LDAPException e) {
+      fail("Expected not to fail fast if failFast is false but got: " + e);
+    }
+
+    try {
+      provider.getUserRoles("test");
+      fail("Expected exception since LDAP server is not yet started, but no exception was thrown");
+    } catch (LDAPException expected) {
+      // fall through
+    }
+
+    // Above should fail even if server already started due to retry interval, however it's possible
+    // (if unlikely) the test could take a second before calling getUserRoles which would mean it
+    // would attempt the connection, succeed, then fail the test. Thus to avoid a flakey test,
+    // not testing that case exactly.
+    ldapServer.startListening();
+
+    // Wait retry interval...
+    Thread.sleep(1000);
+
+    try {
+      provider.getUserRoles("test");
+      // pass!
+    } catch (LDAPException e) {
+      fail("Expected to recover after retry interval but got: " + e);
+    }
+  }
+}

jboss-cert-ldap-login-module/src/main/java/org/esbtools/auth/jboss/CertLdapLoginModule.java

@@ -63,12 +63,13 @@ public class CertLdapLoginModule extends BaseCertLoginModule {
     public static final String ROLES_CACHE_EXPIRY_MS = "rolesCacheExpiryMS";
     public static final String ENVIRONMENT = "environment";
     public static final String ALL_ACCESS_OU = "allAccessOu";
+    public static final String RETRY_INTERVAL_SECONDS = "retryIntervalSeconds";
 
     private static final String[] ALL_VALID_OPTIONS = {
             AUTH_ROLE_NAME, SERVER, PORT, SEARCH_BASE, BIND_DN, BIND_PWD, USE_SSL,
             TRUST_STORE, TRUST_STORE_PASSWORD, POOL_SIZE, POOL_MAX_CONNECTION_AGE_MS,
             CONNECTION_TIMEOUT_MS,RESPONSE_TIMEOUT_MS,DEBUG,KEEP_ALIVE,
-            ROLES_CACHE_EXPIRY_MS, ENVIRONMENT, ALL_ACCESS_OU};
+            ROLES_CACHE_EXPIRY_MS, ENVIRONMENT, ALL_ACCESS_OU, RETRY_INTERVAL_SECONDS};
 
     public static final String UID = "uid";
     public static final String CN = "cn";
@@ -119,13 +120,16 @@ public void initializeRolesProvider() throws Exception {
                     if (options.containsKey(POOL_MAX_CONNECTION_AGE_MS)) {
                         ldapConf.poolMaxConnectionAgeMS(Integer.parseInt((String)options.get(POOL_MAX_CONNECTION_AGE_MS)));
                     }
+                    if (options.containsKey(RETRY_INTERVAL_SECONDS)) {
+                        ldapConf.retryIntervalSeconds(Integer.parseInt((String) options.get(RETRY_INTERVAL_SECONDS)));
+                    }
 
                     int rolesCacheExpiry = 5*60*1000; // default 5 minutes
                     if (options.containsKey(ROLES_CACHE_EXPIRY_MS)) {
                         rolesCacheExpiry = Integer.parseInt((String)options.get(ROLES_CACHE_EXPIRY_MS));
                     }
 
-                    rolesProvider = new CachedRolesProvider(new LdapRolesProvider(searchBase, ldapConf), new RolesCache(rolesCacheExpiry));
+                    rolesProvider = new CachedRolesProvider(new LdapRolesProvider(searchBase, ldapConf, false), new RolesCache(rolesCacheExpiry));
                 }
             }
         }