ldap: add timeout and retry-backoff for ldap (#51927)

privilege/privileges/ldap/BUILD.bazel

@@ -12,9 +12,11 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//privilege/conn",
+        "//util/logutil",
         "@com_github_go_ldap_ldap_v3//:ldap",
         "@com_github_ngaut_pools//:pools",
         "@com_github_pingcap_errors//:errors",
+        "@org_uber_go_zap//:zap",
     ],
 )
 
@@ -23,6 +25,7 @@ go_test(
     timeout = "short",
     srcs = ["ldap_common_test.go"],
     embed = [":ldap"],
+    embedsrcs = ["test/ca.crt"],
     flaky = True,
     deps = ["@com_github_stretchr_testify//require"],
 )

privilege/privileges/ldap/ldap_common.go

@@ -22,12 +22,24 @@
 	"os"
 	"strconv"
 	"sync"
+	"time"
 
 	"github.com/go-ldap/ldap/v3"
 	"github.com/ngaut/pools"
 	"github.com/pingcap/errors"
+	"github.com/pingcap/tidb/util/logutil"
+	"go.uber.org/zap"
 )
 
+// ldapTimeout is set to 10s. It works on both the TCP/TLS dialing timeout, and the LDAP request timeout. For connection with TLS, the
+// user may find that it fails after 2*ldapTimeout, because TiDB will try to connect through both `StartTLS` (from a normal TCP connection)
+// and `TLS`, therefore the total time is 2*ldapTimeout.
+var ldapTimeout = 10 * time.Second
+
+// skipTLSForTest is used to skip trying to connect with TLS directly in tests. If it's set to false, connection will only try to
+// use `StartTLS`
+var skipTLSForTest = false
+
 // ldapAuthImpl gives the internal utilities of authentication with LDAP.
 // The getter and setter methods will lock the mutex inside, while all other methods don't, so all other method call
 // should be protected by `impl.Lock()`.
@@ -120,10 +132,13 @@
 	// It's fine to load these two TLS configurations one-by-one (but not guarded by a single lock), because there isn't
 	// a way to set two variables atomically.
 	if impl.enableTLS {
-		ldapConnection, err := ldap.Dial("tcp", address)
+		ldapConnection, err := ldap.DialURL("ldap://"+address, ldap.DialWithDialer(&net.Dialer{
+			Timeout: ldapTimeout,
+		}))
 		if err != nil {
 			return nil, errors.Wrap(err, "create ldap connection")
 		}
+		ldapConnection.SetTimeout(ldapTimeout)
 
 		err = ldapConnection.StartTLS(&tls.Config{
 			RootCAs:    impl.caPool,
@@ -134,15 +149,19 @@
 		}
 		return ldapConnection, nil
 	}
-	ldapConnection, err := ldap.Dial("tcp", address)
+	ldapConnection, err := ldap.DialURL("ldap://"+address, ldap.DialWithDialer(&net.Dialer{
+		Timeout: ldapTimeout,
+	}))
 	if err != nil {
 		return nil, errors.Wrap(err, "create ldap connection")
 	}
+	ldapConnection.SetTimeout(ldapTimeout)
 
 	return ldapConnection, nil
 }
 
 const getConnectionMaxRetry = 10
+const getConnectionRetryInterval = 500 * time.Millisecond
 
 func (impl *ldapAuthImpl) getConnection() (*ldap.Conn, error) {
 	retryCount := 0
@@ -163,13 +182,19 @@
 			Password: impl.bindRootPWD,
 		})
 		if err != nil {
+			logutil.BgLogger().Warn("fail to use LDAP connection bind to anonymous user. Retrying", zap.Error(err),
+				zap.Duration("backoff", getConnectionRetryInterval))
+
 			// fail to bind to anonymous user, just release this connection and try to get a new one
 			impl.ldapConnectionPool.Put(nil)
 
 			retryCount++
 			if retryCount >= getConnectionMaxRetry {
 				return nil, errors.Wrap(err, "fail to bind to anonymous user")
 			}
+			// Be careful that it's still holding the lock of the system variables, so it's not good to sleep here.
+			// TODO: refactor the `RWLock` to avoid the problem of holding the lock.
+			time.Sleep(getConnectionRetryInterval)
 			continue
 		}
 
@@ -182,12 +207,12 @@
 }
 
 func (impl *ldapAuthImpl) initializePool() {
-	if impl.ldapConnectionPool != nil {
-		impl.ldapConnectionPool.Close()
-	}
-
-	// skip initialization when the variables are not correct
+	// skip re-initialization when the variables are not correct
 	if impl.initCapacity > 0 && impl.maxCapacity >= impl.initCapacity {
+		if impl.ldapConnectionPool != nil {
+			impl.ldapConnectionPool.Close()
+		}
+
 		impl.ldapConnectionPool = pools.NewResourcePool(impl.connectionFactory, impl.initCapacity, impl.maxCapacity, 0)
 	}
 }
@@ -232,6 +257,7 @@
 
 	if ldapServerHost != impl.ldapServerHost {
 		impl.ldapServerHost = ldapServerHost
+		impl.initializePool()
 	}
 }
 
@@ -242,6 +268,7 @@
 
 	if ldapServerPort != impl.ldapServerPort {
 		impl.ldapServerPort = ldapServerPort
+		impl.initializePool()
 	}
 }
 
@@ -252,6 +279,7 @@
 
 	if enableTLS != impl.enableTLS {
 		impl.enableTLS = enableTLS
+		impl.initializePool()
 	}
 }
 

privilege/privileges/ldap/ldap_common_test.go

@@ -15,15 +15,86 @@
 package ldap
 
 import (
+	"crypto/x509"
+	_ "embed"
+	"fmt"
+	"math/rand"
+	"net"
+	"sync"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 )
 
+//go:embed test/ca.crt
+var tlsCAStr []byte
+
 func TestCanonicalizeDN(t *testing.T) {
 	impl := &ldapAuthImpl{
 		searchAttr: "cn",
 	}
 	require.Equal(t, impl.canonicalizeDN("yka", "cn=y,dc=ping,dc=cap"), "cn=y,dc=ping,dc=cap")
 	require.Equal(t, impl.canonicalizeDN("yka", "+dc=ping,dc=cap"), "cn=yka,dc=ping,dc=cap")
 }
+
+func TestLDAPStartTLSTimeout(t *testing.T) {
+	originalTimeout := ldapTimeout
+	ldapTimeout = time.Second * 2
+	skipTLSForTest = true
+	defer func() {
+		ldapTimeout = originalTimeout
+		skipTLSForTest = false
+	}()
+
+	var ln net.Listener
+	startListen := make(chan struct{})
+	afterTimeout := make(chan struct{})
+	defer close(afterTimeout)
+
+	// this test only tests whether the LDAP with LTS enabled will fallback from StartTLS
+	randomTLSServicePort := rand.Int()%10000 + 10000
+	serverWg := &sync.WaitGroup{}
+	serverWg.Add(1)
+	go func() {
+		var err error
+		defer close(startListen)
+		defer serverWg.Done()
+
+		ln, err = net.Listen("tcp", fmt.Sprintf("localhost:%d", randomTLSServicePort))
+		require.NoError(t, err)
+		startListen <- struct{}{}
+
+		conn, err := ln.Accept()
+		require.NoError(t, err)
+
+		<-afterTimeout
+		require.NoError(t, conn.Close())
+
+		// close the server
+		require.NoError(t, ln.Close())
+	}()
+
+	<-startListen
+	defer func() {
+		serverWg.Wait()
+	}()
+
+	impl := &ldapAuthImpl{}
+	impl.SetEnableTLS(true)
+	impl.SetLDAPServerHost("localhost")
+	impl.SetLDAPServerPort(randomTLSServicePort)
+
+	impl.caPool = x509.NewCertPool()
+	require.True(t, impl.caPool.AppendCertsFromPEM(tlsCAStr))
+	impl.SetInitCapacity(1)
+	impl.SetMaxCapacity(1)
+
+	now := time.Now()
+	_, err := impl.connectionFactory()
+	afterTimeout <- struct{}{}
+	dur := time.Since(now)
+	require.Greater(t, dur, 2*time.Second)
+	require.Less(t, dur, 3*time.Second)
+	require.ErrorContains(t, err, "connection timed out")
+}

privilege/privileges/ldap/test/ca.crt

@@ -0,0 +1,31 @@
+-----BEGIN CERTIFICATE-----
+MIIFZTCCA02gAwIBAgIUZ2hQOFVvjuAHrC8Tv+5dnwPGvF0wDQYJKoZIhvcNAQEL
+BQAwQjELMAkGA1UEBhMCWFgxFTATBgNVBAcMDERlZmF1bHQgQ2l0eTEcMBoGA1UE
+CgwTRGVmYXVsdCBDb21wYW55IEx0ZDAeFw0yMzA0MjQwNjM1MTRaFw0yODA0MjMw
+NjM1MTRaMEIxCzAJBgNVBAYTAlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAa
+BgNVBAoME0RlZmF1bHQgQ29tcGFueSBMdGQwggIiMA0GCSqGSIb3DQEBAQUAA4IC
+DwAwggIKAoICAQDFvQt3xupYFQxZsyQPr2byhR9ZHoAWBxxqNWxbvpqy7RzHeccJ
+Jg36dO1BNIBY8NjIy/JHV7eLDVGCh9FTGozn8dODQMOwDXTYqxFOiBHb2/M9wxVX
+ILafa1GlsOnUFxEws9T0XH7ZBqMLC/KlXdJ5xQD1C36K1eWHvphjD0AFhgUnqQ4N
+O3NT3tJjzcY7oXBoKpgSgQs7qeTdJLTKJE7pY02C/hJI2WvJDdIiEhZTi0UWqE06
+5aXp8Heag/H4VlZzRA+RzQuDXqgXC3Bt7mJwtnoym0HgyTvoKBKO/vzfAW1yQhGo
+6ikfSZkvIy3kyPAxD1FSWeSA0Xo8soGNDUsZjV6dQRtcnlWLPFA+7VfwivCPNiFh
+91csXhHNEkYPNq4yCe6ZsycydZ+GNdNygzIrMjQ+DjNnHXXmfdeiiLLJbyxYzwuu
+GaAT9eD98vXeJFhuWSbKyj4oXcMKnj3bTAQnudMCHIV8cMDe7Zuq1d4/gjXvk95Z
+s/OnxqRYYNTXECkdLrevAPfGI2Qg9d7IrhnAh6KqCDDiFkhDkS5TMbWeHA88ZPKZ
+6RvLYZmA+j3tjtKPpta9yPiTglExjBUDHIe+37K84G4p0C4bEo5RxEop5hHuX4EW
+QvwNb0254i+RCsdyt+tFHiAVzo3/mTg9EMkWlTzoy0381HytFNcLDGb37QIDAQAB
+o1MwUTAdBgNVHQ4EFgQUHs+YTH+x0YRNja11v18CF4iu5XowHwYDVR0jBBgwFoAU
+Hs+YTH+x0YRNja11v18CF4iu5XowDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0B
+AQsFAAOCAgEAqf2ukpuqXC7u0kTqWdrw+3x2R+mwoLSsy7QbpsjdPbJwVoEysC02
+WHiZcMj+w75wZQ2SS5Md1+1K4HiIWC3n+eRDodz+Di0Hg9lxtoMFuOIBpnYUsDuA
+Fooo/B7HadZkw9AbWFxPK5EGLyfCRuZF50981svSX5rnYqgCLLs0zGxr7Uswhzvh
+3fQMDd0OGLST0M4FW/pQkRYIWnQ4zn/n+wJaHBeaKXHJ7QfgNCtVXOLTXdzpreIL
+RvvydcOdVoPnjMgCs1jyhB6g226+/nOuQqic53pxnUTUTvHFJQ5B6/JlzMHeJS1m
+ycvSxmF+3RqhjePiwRAT/ui9FBXkhXSG3wp0n0w83rpq7Ne0uwPH/KE9hqFkiI5x
+PzobjKy6ahzoSbZrw/a4gDXfZe3fYGtm1EdyDYTh1HFCi7hkdoxY9iCIL1Gr+mpB
+YruELQZ+RpvZQ7V8JN7bPtzWfPybPkOSozP1EoLXhUAnXl4/DinoBZvum1MpvPWY
+sKF9qQfTP6cAqOuIL1LcVhJ7yHAjR+BK7tvhA2h4sIqxEjhlDmJjH0XMr9JpYQcb
+yBzNgkS0YycMPJru0zb2p7vodql5rxSWArQW13Pyqza6N803Qk3vP0/SCfYXeR/i
+hv/InNBQBwfHo79FBEv/T7UB8yS7CIu75f562jp23DFKUQD+doybmDg=
+-----END CERTIFICATE-----