Allow configuration of read/connect timeout in LDAP client

docs/src/main/sphinx/security/ldap.rst

@@ -109,6 +109,8 @@ Property                           Description
 ``ldap.ignore-referrals``          Ignore referrals to other LDAP servers while
                                    performing search queries. Defaults to ``false``.
 ``ldap.cache-ttl``                 LDAP cache duration. Defaults to ``1h``.
+``ldap.timeout.connection``        Timeout for establishing an LDAP connection.
+``ldap.timeout.read``              Timeout for reading data from an LDAP connection.
 ================================== ======================================================
 
 Based on the LDAP server implementation type, the property

plugin/trino-password-authenticators/pom.xml

@@ -122,6 +122,13 @@
             <scope>test</scope>
         </dependency>
 
+        <dependency>
+            <groupId>eu.rekawek.toxiproxy</groupId>
+            <artifactId>toxiproxy-java</artifactId>
+            <version>2.1.5</version>
+            <scope>test</scope>
+        </dependency>
+
         <dependency>
             <groupId>org.assertj</groupId>
             <artifactId>assertj-core</artifactId>
@@ -134,6 +141,12 @@
             <scope>test</scope>
         </dependency>
 
+        <dependency>
+            <groupId>org.testcontainers</groupId>
+            <artifactId>toxiproxy</artifactId>
+            <scope>test</scope>
+        </dependency>
+
         <dependency>
             <groupId>org.testng</groupId>
             <artifactId>testng</artifactId>

plugin/trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/JdkLdapAuthenticatorClient.java

@@ -17,6 +17,7 @@
 import com.google.common.collect.ImmutableSet;
 import io.airlift.log.Logger;
 import io.airlift.security.pem.PemReader;
+import io.airlift.units.Duration;
 import io.trino.spi.security.AccessDeniedException;
 
 import javax.inject.Inject;
@@ -65,11 +66,23 @@ public JdkLdapAuthenticatorClient(LdapConfig ldapConfig)
             log.warn("Passwords will be sent in the clear to the LDAP server. Please consider using SSL to connect.");
         }
 
-        this.basicEnvironment = ImmutableMap.<String, String>builder()
-                .put(INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory")
+        ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+        builder.put(INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory")
                 .put(PROVIDER_URL, ldapUrl)
-                .put(REFERRAL, ldapConfig.isIgnoreReferrals() ? "ignore" : "follow")
-                .buildOrThrow();
+                .put(REFERRAL, ldapConfig.isIgnoreReferrals() ? "ignore" : "follow");
+
+        ldapConfig.getLdapConnectionTimeout()
+                .map(Duration::toMillis)
+                .map(String::valueOf)
+                .ifPresent(timeout -> builder.put("com.sun.jndi.ldap.connect.timeout", timeout));
+
+        ldapConfig.getLdapReadTimeout()
+                .map(Duration::toMillis)
+                .map(String::valueOf)
+                .ifPresent(timeout -> builder.put("com.sun.jndi.ldap.read.timeout", timeout));
+
+        this.basicEnvironment = builder.buildOrThrow();
 
         this.sslContext = Optional.ofNullable(ldapConfig.getTrustCertificate())
                 .map(JdkLdapAuthenticatorClient::createSslContext);

plugin/trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapConfig.java

@@ -27,6 +27,7 @@
 
 import java.io.File;
 import java.util.List;
+import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 
 import static com.google.common.base.Strings.nullToEmpty;
@@ -44,6 +45,8 @@ public class LdapConfig
     private String bindPassword;
     private boolean ignoreReferrals;
     private Duration ldapCacheTtl = new Duration(1, TimeUnit.HOURS);
+    private Optional<Duration> ldapConnectionTimeout = Optional.empty();
+    private Optional<Duration> ldapReadTimeout = Optional.empty();
 
     @NotNull
     @Pattern(regexp = "^ldaps?://.*", message = "Invalid LDAP server URL. Expected ldap:// or ldaps://")
@@ -194,4 +197,30 @@ public LdapConfig setLdapCacheTtl(Duration ldapCacheTtl)
         this.ldapCacheTtl = ldapCacheTtl;
         return this;
     }
+
+    public Optional<Duration> getLdapConnectionTimeout()
+    {
+        return ldapConnectionTimeout;
+    }
+
+    @Config("ldap.timeout.connect")
+    @ConfigDescription("Timeout for establishing a connection")
+    public LdapConfig setLdapConnectionTimeout(Duration ldapConnectionTimeout)
+    {
+        this.ldapConnectionTimeout = Optional.ofNullable(ldapConnectionTimeout);
+        return this;
+    }
+
+    public Optional<Duration> getLdapReadTimeout()
+    {
+        return ldapReadTimeout;
+    }
+
+    @Config("ldap.timeout.read")
+    @ConfigDescription("Timeout for reading data from LDAP")
+    public LdapConfig setLdapReadTimeout(Duration ldapReadTimeout)
+    {
+        this.ldapReadTimeout = Optional.ofNullable(ldapReadTimeout);
+        return this;
+    }
 }

plugin/trino-password-authenticators/src/test/java/io/trino/plugin/password/ldap/TestLdapAuthenticator.java

@@ -17,6 +17,7 @@
 import io.trino.plugin.password.ldap.TestingOpenLdapServer.DisposableSubContext;
 import io.trino.spi.security.AccessDeniedException;
 import io.trino.spi.security.BasicPrincipal;
+import org.testcontainers.containers.Network;
 import org.testng.annotations.AfterClass;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
@@ -37,7 +38,10 @@ public class TestLdapAuthenticator
     public void setup()
             throws Exception
     {
-        openLdapServer = new TestingOpenLdapServer();
+        Network network = Network.newNetwork();
+        closer.register(network::close);
+
+        openLdapServer = new TestingOpenLdapServer(network);
         closer.register(openLdapServer);
         openLdapServer.start();
 

plugin/trino-password-authenticators/src/test/java/io/trino/plugin/password/ldap/TestLdapAuthenticatorWithTimeouts.java

@@ -0,0 +1,122 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.plugin.password.ldap;
+
+import com.google.common.io.Closer;
+import io.airlift.units.Duration;
+import io.trino.plugin.password.ldap.TestingOpenLdapServer.DisposableSubContext;
+import io.trino.spi.security.BasicPrincipal;
+import org.testcontainers.containers.Network;
+import org.testcontainers.containers.ToxiproxyContainer;
+import org.testcontainers.containers.ToxiproxyContainer.ContainerProxy;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import static eu.rekawek.toxiproxy.model.ToxicDirection.DOWNSTREAM;
+import static io.trino.plugin.password.ldap.TestingOpenLdapServer.LDAP_PORT;
+import static java.lang.String.format;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.testng.Assert.assertEquals;
+
+public class TestLdapAuthenticatorWithTimeouts
+{
+    private final Closer closer = Closer.create();
+
+    private TestingOpenLdapServer openLdapServer;
+    private String proxyLdapUrl;
+
+    @BeforeClass
+    public void setup()
+            throws Exception
+    {
+        Network network = Network.newNetwork();
+        closer.register(network::close);
+
+        ToxiproxyContainer proxyServer = new ToxiproxyContainer("shopify/toxiproxy:2.1.0")
+                .withNetwork(network);
+        closer.register(proxyServer::close);
+        proxyServer.start();
+
+        openLdapServer = new TestingOpenLdapServer(network);
+        closer.register(openLdapServer);
+        openLdapServer.start();
+
+        ContainerProxy proxy = proxyServer.getProxy(openLdapServer.getNetworkAlias(), LDAP_PORT);
+        proxy.toxics()
+                .latency("latency", DOWNSTREAM, 5_000);
+        proxyLdapUrl = format("ldap://%s:%s", proxy.getContainerIpAddress(), proxy.getProxyPort());
+    }
+
+    @AfterClass(alwaysRun = true)
+    public void close()
+            throws Exception
+    {
+        closer.close();
+    }
+
+    @Test
+    public void testConnectTimeout()
+            throws Exception
+    {
+        try (DisposableSubContext organization = openLdapServer.createOrganization();
+                DisposableSubContext ignored = openLdapServer.createUser(organization, "alice", "alice-pass")) {
+            LdapConfig ldapConfig = new LdapConfig()
+                    .setLdapUrl(proxyLdapUrl)
+                    .setLdapConnectionTimeout(new Duration(1, SECONDS))
+                    .setUserBindSearchPatterns("uid=${USER}," + organization.getDistinguishedName());
+
+            LdapAuthenticator ldapAuthenticator = new LdapAuthenticator(new JdkLdapAuthenticatorClient(ldapConfig), ldapConfig);
+            assertThatThrownBy(() -> ldapAuthenticator.createAuthenticatedPrincipal("alice", "alice-pass"))
+                    .isInstanceOf(RuntimeException.class)
+                    .hasMessageMatching(".*Authentication error.*");
+
+            LdapConfig withIncreasedTimeout = ldapConfig.setLdapConnectionTimeout(new Duration(30, SECONDS));
+            assertEquals(
+                    new LdapAuthenticator(new JdkLdapAuthenticatorClient(withIncreasedTimeout), withIncreasedTimeout)
+                            .createAuthenticatedPrincipal("alice", "alice-pass"),
+                    new BasicPrincipal("alice"));
+        }
+    }
+
+    @Test
+    public void testReadTimeout()
+            throws Exception
+    {
+        try (DisposableSubContext organization = openLdapServer.createOrganization();
+                DisposableSubContext group = openLdapServer.createGroup(organization);
+                DisposableSubContext alice = openLdapServer.createUser(organization, "alice", "alice-pass")) {
+            openLdapServer.addUserToGroup(alice, group);
+
+            LdapConfig ldapConfig = new LdapConfig()
+                    .setLdapUrl(proxyLdapUrl)
+                    .setLdapReadTimeout(new Duration(1, SECONDS))
+                    .setUserBindSearchPatterns("uid=${USER}," + organization.getDistinguishedName())
+                    .setUserBaseDistinguishedName(organization.getDistinguishedName())
+                    .setGroupAuthorizationSearchPattern(format("(&(objectClass=groupOfNames)(cn=group_*)(member=uid=${USER},%s))", organization.getDistinguishedName()));
+
+            LdapAuthenticator ldapAuthenticator = new LdapAuthenticator(new JdkLdapAuthenticatorClient(ldapConfig), ldapConfig);
+            assertThatThrownBy(() -> ldapAuthenticator.createAuthenticatedPrincipal("alice", "alice-pass"))
+                    .isInstanceOf(RuntimeException.class)
+                    .hasMessageMatching(".*Authentication error.*");
+
+            LdapConfig withIncreasedTimeout = ldapConfig.setLdapReadTimeout(new Duration(30, SECONDS));
+            assertEquals(
+                    new LdapAuthenticator(new JdkLdapAuthenticatorClient(withIncreasedTimeout), withIncreasedTimeout)
+                            .createAuthenticatedPrincipal("alice", "alice-pass"),
+                    new BasicPrincipal("alice"));
+        }
+    }
+}

plugin/trino-password-authenticators/src/test/java/io/trino/plugin/password/ldap/TestLdapConfig.java

@@ -50,7 +50,9 @@ public void testDefault()
                 .setBindDistingushedName(null)
                 .setBindPassword(null)
                 .setIgnoreReferrals(false)
-                .setLdapCacheTtl(new Duration(1, TimeUnit.HOURS)));
+                .setLdapCacheTtl(new Duration(1, TimeUnit.HOURS))
+                .setLdapConnectionTimeout(null)
+                .setLdapReadTimeout(null));
     }
 
     @Test
@@ -70,6 +72,8 @@ public void testExplicitConfig()
                 .put("ldap.bind-password", "password1234")
                 .put("ldap.ignore-referrals", "true")
                 .put("ldap.cache-ttl", "2m")
+                .put("ldap.timeout.connect", "3m")
+                .put("ldap.timeout.read", "4m")
                 .buildOrThrow();
 
         LdapConfig expected = new LdapConfig()
@@ -82,7 +86,9 @@ public void testExplicitConfig()
                 .setBindDistingushedName("CN=User Name,OU=CITY_OU,OU=STATE_OU,DC=domain,DC=domain_root")
                 .setBindPassword("password1234")
                 .setIgnoreReferrals(true)
-                .setLdapCacheTtl(new Duration(2, TimeUnit.MINUTES));
+                .setLdapCacheTtl(new Duration(2, TimeUnit.MINUTES))
+                .setLdapConnectionTimeout(new Duration(3, TimeUnit.MINUTES))
+                .setLdapReadTimeout(new Duration(4, TimeUnit.MINUTES));
 
         assertFullMapping(properties, expected);
     }

plugin/trino-password-authenticators/src/test/java/io/trino/plugin/password/ldap/TestingOpenLdapServer.java

@@ -18,6 +18,7 @@
 import com.google.common.io.Closer;
 import io.trino.testing.TestingProperties;
 import org.testcontainers.containers.GenericContainer;
+import org.testcontainers.containers.Network;
 import org.testcontainers.containers.startupcheck.IsRunningStartupCheckStrategy;
 import org.testcontainers.containers.wait.strategy.HostPortWaitStrategy;
 
@@ -46,14 +47,15 @@ public class TestingOpenLdapServer
 {
     private static final String BASE_DISTINGUISED_NAME = "dc=trino,dc=testldap,dc=com";
 
-    private static final int LDAP_PORT = 389;
+    public static final int LDAP_PORT = 389;
 
     private final Closer closer = Closer.create();
     private final GenericContainer<?> openLdapServer;
 
-    public TestingOpenLdapServer()
+    public TestingOpenLdapServer(Network network)
     {
         openLdapServer = new GenericContainer<>("ghcr.io/trinodb/testing/centos7-oj11-openldap:" + TestingProperties.getDockerImagesVersion())
+                .withNetwork(network)
                 .withExposedPorts(LDAP_PORT)
                 .withStartupCheckStrategy(new IsRunningStartupCheckStrategy())
                 .waitingFor(new HostPortWaitStrategy())
@@ -67,6 +69,11 @@ public void start()
         openLdapServer.start();
     }
 
+    public String getNetworkAlias()
+    {
+        return openLdapServer.getNetworkAliases().get(0);
+    }
+
     public String getLdapUrl()
     {
         return format("ldap://%s:%s", openLdapServer.getContainerIpAddress(), openLdapServer.getMappedPort(LDAP_PORT));