From 425cda956706b7ed3c67f38dc7d9b1bb9ab74a29 Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Fri, 12 Apr 2024 17:37:53 +0800 Subject: [PATCH] ldap: add timeout and retry-backoff for ldap (#51927) (#51943) close pingcap/tidb#51883 --- privilege/privileges/ldap/BUILD.bazel | 3 + privilege/privileges/ldap/ldap_common.go | 42 +++++++++-- privilege/privileges/ldap/ldap_common_test.go | 71 +++++++++++++++++++ privilege/privileges/ldap/test/ca.crt | 31 ++++++++ 4 files changed, 140 insertions(+), 7 deletions(-) create mode 100644 privilege/privileges/ldap/test/ca.crt diff --git a/privilege/privileges/ldap/BUILD.bazel b/privilege/privileges/ldap/BUILD.bazel index a807291caf073..fb822094db1a3 100644 --- a/privilege/privileges/ldap/BUILD.bazel +++ b/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"], ) diff --git a/privilege/privileges/ldap/ldap_common.go b/privilege/privileges/ldap/ldap_common.go index e28f15d4ef447..9d450680f9522 100644 --- a/privilege/privileges/ldap/ldap_common.go +++ b/privilege/privileges/ldap/ldap_common.go @@ -22,12 +22,24 @@ import ( "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 @@ func (impl *ldapAuthImpl) connectionFactory() (pools.Resource, error) { // 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 @@ func (impl *ldapAuthImpl) connectionFactory() (pools.Resource, error) { } 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,6 +182,9 @@ func (impl *ldapAuthImpl) getConnection() (*ldap.Conn, error) { 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) @@ -170,6 +192,9 @@ func (impl *ldapAuthImpl) getConnection() (*ldap.Conn, error) { 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) putConnection(conn *ldap.Conn) { } 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 @@ func (impl *ldapAuthImpl) SetLDAPServerHost(ldapServerHost string) { if ldapServerHost != impl.ldapServerHost { impl.ldapServerHost = ldapServerHost + impl.initializePool() } } @@ -242,6 +268,7 @@ func (impl *ldapAuthImpl) SetLDAPServerPort(ldapServerPort int) { if ldapServerPort != impl.ldapServerPort { impl.ldapServerPort = ldapServerPort + impl.initializePool() } } @@ -252,6 +279,7 @@ func (impl *ldapAuthImpl) SetEnableTLS(enableTLS bool) { if enableTLS != impl.enableTLS { impl.enableTLS = enableTLS + impl.initializePool() } } diff --git a/privilege/privileges/ldap/ldap_common_test.go b/privilege/privileges/ldap/ldap_common_test.go index 7794b5ab5f3d4..d8e8a870015c3 100644 --- a/privilege/privileges/ldap/ldap_common_test.go +++ b/privilege/privileges/ldap/ldap_common_test.go @@ -15,11 +15,21 @@ 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", @@ -27,3 +37,64 @@ func TestCanonicalizeDN(t *testing.T) { 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") +} diff --git a/privilege/privileges/ldap/test/ca.crt b/privilege/privileges/ldap/test/ca.crt new file mode 100644 index 0000000000000..cbef4f3fb2bc4 --- /dev/null +++ b/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----- \ No newline at end of file