From 7e76b666390bc481493d7d3c7a26a2c07b6b83b0 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 20 May 2021 12:46:33 -0700 Subject: [PATCH] LDAP upstream watcher controller tries using both TLS and StartTLS - Automatically try to fall back to using StartTLS when using TLS doesn't work. Only complain when both don't work. - Remember (in-memory) which one worked and keeping using that one in the future (unless the pod restarts). --- .../ldap_upstream_watcher.go | 57 ++-- .../ldap_upstream_watcher_test.go | 243 ++++++++++++++---- internal/upstreamldap/upstreamldap.go | 18 +- internal/upstreamldap/upstreamldap_test.go | 24 +- 4 files changed, 263 insertions(+), 79 deletions(-) diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go index 47cb404d..180b9ef4 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go @@ -58,13 +58,18 @@ type ldapWatcherController struct { } // An in-memory cache with an entry for each LDAPIdentityProvider, to keep track of which ResourceVersion -// of the bind Secret was used during the most recent successful validation. +// of the bind Secret and which TLS/StartTLS setting was used during the most recent successful validation. type secretVersionCache struct { - ResourceVersionsByName map[string]string + ValidatedSettingsByName map[string]validatedSettings +} + +type validatedSettings struct { + BindSecretResourceVersion string + LDAPConnectionProtocol upstreamldap.LDAPConnectionProtocol } func newSecretVersionCache() *secretVersionCache { - return &secretVersionCache{ResourceVersionsByName: map[string]string{}} + return &secretVersionCache{ValidatedSettingsByName: map[string]validatedSettings{}} } // New instantiates a new controllerlib.Controller which will populate the provided UpstreamLDAPIdentityProviderICache. @@ -152,9 +157,8 @@ func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream * spec := upstream.Spec config := &upstreamldap.ProviderConfig{ - Name: upstream.Name, - Host: spec.Host, - ConnectionProtocol: upstreamldap.TLS, + Name: upstream.Name, + Host: spec.Host, UserSearch: upstreamldap.UserSearchConfig{ Base: spec.UserSearch.Base, Filter: spec.UserSearch.Filter, @@ -229,22 +233,23 @@ func (c *ldapWatcherController) validateTLSConfig(upstream *v1alpha1.LDAPIdentit } func (c *ldapWatcherController) validateFinishedConfig(ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider, config *upstreamldap.ProviderConfig, currentSecretVersion string) *v1alpha1.Condition { - ldapProvider := upstreamldap.New(*config) - - if c.hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream, currentSecretVersion) { + if c.hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream, currentSecretVersion, config) { return nil } testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, testLDAPConnectionTimeout) defer cancelFunc() - condition := c.testConnection(testConnectionTimeout, upstream, config, ldapProvider, currentSecretVersion) + condition := c.testConnection(testConnectionTimeout, upstream, config, currentSecretVersion) if condition.Status == v1alpha1.ConditionTrue { // Remember (in-memory for this pod) that the controller has successfully validated the LDAP provider // using this version of the Secret. This is for performance reasons, to avoid attempting to connect to // the LDAP server more than is needed. If the pod restarts, it will attempt this validation again. - c.validatedSecretVersionsCache.ResourceVersionsByName[upstream.GetName()] = currentSecretVersion + c.validatedSecretVersionsCache.ValidatedSettingsByName[upstream.GetName()] = validatedSettings{ + BindSecretResourceVersion: currentSecretVersion, + LDAPConnectionProtocol: config.ConnectionProtocol, + } } return condition @@ -254,10 +259,28 @@ func (c *ldapWatcherController) testConnection( ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider, config *upstreamldap.ProviderConfig, - ldapProvider *upstreamldap.Provider, currentSecretVersion string, ) *v1alpha1.Condition { - err := ldapProvider.TestConnection(ctx) + // First try using TLS. + config.ConnectionProtocol = upstreamldap.TLS + tlsLDAPProvider := upstreamldap.New(*config) + err := tlsLDAPProvider.TestConnection(ctx) + if err != nil { + // If there was any error, try again with StartTLS instead. + config.ConnectionProtocol = upstreamldap.StartTLS + startTLSLDAPProvider := upstreamldap.New(*config) + startTLSErr := startTLSLDAPProvider.TestConnection(ctx) + if startTLSErr == nil { + // Successfully able to fall back to using StartTLS, so clear the original + // error and consider the connection test to be successful. + err = nil + } else { + // Falling back to StartTLS also failed, so put TLS back into the config + // and consider the connection test to be failed. + config.ConnectionProtocol = upstreamldap.TLS + } + } + if err != nil { return &v1alpha1.Condition{ Type: typeLDAPConnectionValid, @@ -277,14 +300,16 @@ func (c *ldapWatcherController) testConnection( } } -func (c *ldapWatcherController) hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream *v1alpha1.LDAPIdentityProvider, currentSecretVersion string) bool { +func (c *ldapWatcherController) hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream *v1alpha1.LDAPIdentityProvider, currentSecretVersion string, config *upstreamldap.ProviderConfig) bool { currentGeneration := upstream.Generation for _, cond := range upstream.Status.Conditions { if cond.Type == typeLDAPConnectionValid && cond.Status == v1alpha1.ConditionTrue && cond.ObservedGeneration == currentGeneration { // Found a previously successful condition for the current spec generation. // Now figure out which version of the bind Secret was used during that previous validation, if any. - validatedSecretVersion := c.validatedSecretVersionsCache.ResourceVersionsByName[upstream.GetName()] - if validatedSecretVersion == currentSecretVersion { + validatedSecretVersion := c.validatedSecretVersionsCache.ValidatedSettingsByName[upstream.GetName()] + if validatedSecretVersion.BindSecretResourceVersion == currentSecretVersion { + // Reload the TLS vs StartTLS setting that was previously validated. + config.ConnectionProtocol = validatedSecretVersion.LDAPConnectionProtocol return true } } diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go index ae9346f5..6eaab2f3 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/go-ldap/ldap/v3" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -196,7 +197,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { return deepCopy } - providerConfigForValidUpstream := &upstreamldap.ProviderConfig{ + providerConfigForValidUpstreamWithTLS := &upstreamldap.ProviderConfig{ Name: testName, Host: testHost, ConnectionProtocol: upstreamldap.TLS, @@ -216,6 +217,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, } + // Make a copy with targeted changes. + copyOfProviderConfigForValidUpstreamWithTLS := *providerConfigForValidUpstreamWithTLS + providerConfigForValidUpstreamWithStartTLS := ©OfProviderConfigForValidUpstreamWithTLS + providerConfigForValidUpstreamWithStartTLS.ConnectionProtocol = upstreamldap.StartTLS + bindSecretValidTrueCondition := func(gen int64) v1alpha1.Condition { return v1alpha1.Condition{ Type: "BindSecretValid", @@ -265,16 +271,16 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { } tests := []struct { - name string - initialValidatedSecretVersions map[string]string - inputUpstreams []runtime.Object - inputSecrets []runtime.Object - setupMocks func(conn *mockldapconn.MockConn) - dialError error - wantErr string - wantResultingCache []*upstreamldap.ProviderConfig - wantResultingUpstreams []v1alpha1.LDAPIdentityProvider - wantValidatedSecretVersions map[string]string + name string + initialValidatedSettings map[string]validatedSettings + inputUpstreams []runtime.Object + inputSecrets []runtime.Object + setupMocks func(conn *mockldapconn.MockConn) + dialErrors map[string]error + wantErr string + wantResultingCache []*upstreamldap.ProviderConfig + wantResultingUpstreams []v1alpha1.LDAPIdentityProvider + wantValidatedSettings map[string]validatedSettings }{ { name: "no LDAPIdentityProvider upstreams clears the cache", @@ -289,7 +295,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) conn.EXPECT().Close().Times(1) }, - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -297,7 +303,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, { name: "missing secret", @@ -480,7 +486,121 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + }, + { + name: "when TLS connection fails it tries to use StartTLS instead: without a specified port it automatically switches ports", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { + upstream.Spec.Host = "ldap.example.com" // when the port is not specified, automatically switch ports for StartTLS + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Should perform a test dial and bind. + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Close().Times(1) + }, + dialErrors: map[string]error{ + "ldap.example.com:" + ldap.DefaultLdapsPort: fmt.Errorf("some ldaps dial error"), + "ldap.example.com:" + ldap.DefaultLdapPort: nil, // no error on the regular ldap:// port + }, + wantResultingCache: []*upstreamldap.ProviderConfig{ + { + Name: testName, + Host: "ldap.example.com", + ConnectionProtocol: upstreamldap.StartTLS, // successfully fell back to using StartTLS + CABundle: testCABundle, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: upstreamldap.UserSearchConfig{ + Base: testUserSearchBase, + Filter: testUserSearchFilter, + UsernameAttribute: testUsernameAttrName, + UIDAttribute: testUIDAttrName, + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: testGroupSearchBase, + Filter: testGroupSearchFilter, + GroupNameAttribute: testGroupNameAttrName, + }, + }, + }, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + bindSecretValidTrueCondition(1234), + { + Type: "LDAPConnectionValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: fmt.Sprintf( + `successfully able to connect to "%s" and bind as user "%s" [validated with Secret "%s" at version "%s"]`, + "ldap.example.com", testBindUsername, testSecretName, "4242"), + ObservedGeneration: 1234, + }, + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}}, + }, + { + name: "when TLS connection fails it tries to use StartTLS instead: with a specified port it does not automatically switch ports", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { + upstream.Spec.Host = "ldap.example.com:5678" // when the port is specified, do not automatically switch ports for StartTLS + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Both dials fail, so there should be no bind. + }, + dialErrors: map[string]error{ + "ldap.example.com:5678": fmt.Errorf("some dial error"), // both TLS and StartTLS should try the same port and both fail + }, + wantResultingCache: []*upstreamldap.ProviderConfig{ + // even though the connection test failed, still loads into the cache because it is treated like a warning + { + Name: testName, + Host: "ldap.example.com:5678", + ConnectionProtocol: upstreamldap.TLS, // need to pick TLS or StartTLS to load into the cache when both fail, so choose TLS + CABundle: testCABundle, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: upstreamldap.UserSearchConfig{ + Base: testUserSearchBase, + Filter: testUserSearchFilter, + UsernameAttribute: testUsernameAttrName, + UIDAttribute: testUIDAttrName, + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: testGroupSearchBase, + Filter: testGroupSearchFilter, + GroupNameAttribute: testGroupNameAttrName, + }, + }, + }, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + bindSecretValidTrueCondition(1234), + { + Type: "LDAPConnectionValid", + Status: "False", + LastTransitionTime: now, + Reason: "LDAPConnectionError", + Message: fmt.Sprintf( + `could not successfully connect to "%s" and bind as user "%s": error dialing host "%s": some dial error`, + "ldap.example.com:5678", testBindUsername, "ldap.example.com:5678"), + ObservedGeneration: 1234, + }, + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, }, { name: "non-nil TLS configuration with empty CertificateAuthorityData is valid", @@ -521,7 +641,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, { name: "one valid upstream and one invalid upstream updates the cache to include only the valid upstream", @@ -537,7 +657,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { conn.EXPECT().Close().Times(1) }, wantErr: controllerlib.ErrSyntheticRequeue.Error(), - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{ { ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: "other-upstream", Generation: 42}, @@ -564,7 +684,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, }, }, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, { name: "when testing the connection to the LDAP server fails then the upstream is still added to the cache anyway (treated like a warning)", @@ -572,11 +692,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { inputSecrets: []runtime.Object{validBindUserSecret("")}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. - conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1).Return(errors.New("some bind error")) - conn.EXPECT().Close().Times(1) + // Expect two calls to each of these: once for trying TLS and once for trying StartTLS. + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2).Return(errors.New("some bind error")) + conn.EXPECT().Close().Times(2) }, wantErr: controllerlib.ErrSyntheticRequeue.Error(), - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -599,19 +720,19 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }}, }, { - name: "when the LDAP server connection was already validated for the current resource generation and secret version, then do not validate it again", + name: "when the LDAP server connection was already validated using TLS for the current resource generation and secret version, then do not validate it again and keep using TLS", inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { upstream.Generation = 1234 upstream.Status.Conditions = []v1alpha1.Condition{ ldapConnectionValidTrueCondition(1234, "4242"), } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSecretVersions: map[string]string{testName: "4242"}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, setupMocks: func(conn *mockldapconn.MockConn) { // Should not perform a test dial and bind. No mocking here means the test will fail if Bind() or Close() are called. }, - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -619,7 +740,30 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + }, + { + name: "when the LDAP server connection was already validated using StartTLS for the current resource generation and secret version, then do not validate it again and keep using StartTLS", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { + upstream.Generation = 1234 + upstream.Status.Conditions = []v1alpha1.Condition{ + ldapConnectionValidTrueCondition(1234, "4242"), + } + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Should not perform a test dial and bind. No mocking here means the test will fail if Bind() or Close() are called. + }, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithStartTLS}, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Ready", + Conditions: allConditionsTrue(1234, "4242"), + }, + }}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}}, }, { name: "when the LDAP server connection was validated for an older resource generation, then try to validate it again", @@ -629,14 +773,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ldapConnectionValidTrueCondition(1233, "4242"), // older spec generation! } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSecretVersions: map[string]string{testName: "4242"}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) conn.EXPECT().Close().Times(1) }, - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -644,7 +788,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, { name: "when the LDAP server connection validation previously failed for this resource generation, then try to validate it again", @@ -661,14 +805,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSecretVersions: map[string]string{testName: "1"}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "1", LDAPConnectionProtocol: upstreamldap.TLS}}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) conn.EXPECT().Close().Times(1) }, - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -676,7 +820,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, { name: "when the LDAP server connection was already validated for this resource generation but the bind secret has changed, then try to validate it again", @@ -686,14 +830,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ldapConnectionValidTrueCondition(1234, "4241"), // same spec generation, old secret version } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, // newer secret version! - initialValidatedSecretVersions: map[string]string{testName: "4241"}, // old version was validated + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, // newer secret version! + initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4241", LDAPConnectionProtocol: upstreamldap.TLS}}, // old version was validated setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) conn.EXPECT().Close().Times(1) }, - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -701,7 +845,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, } @@ -727,16 +871,19 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { tt.setupMocks(conn) } - dialer := &comparableDialer{upstreamldap.LDAPDialerFunc(func(ctx context.Context, _ string) (upstreamldap.Conn, error) { - if tt.dialError != nil { - return nil, tt.dialError + dialer := &comparableDialer{upstreamldap.LDAPDialerFunc(func(ctx context.Context, hostAndPort string) (upstreamldap.Conn, error) { + if tt.dialErrors != nil { + dialErr := tt.dialErrors[hostAndPort] + if dialErr != nil { + return nil, dialErr + } } return conn, nil })} validatedSecretVersionCache := newSecretVersionCache() - if tt.initialValidatedSecretVersions != nil { - validatedSecretVersionCache.ResourceVersionsByName = tt.initialValidatedSecretVersions + if tt.initialValidatedSettings != nil { + validatedSecretVersionCache.ValidatedSettingsByName = tt.initialValidatedSettings } controller := newInternal( @@ -768,11 +915,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { require.Equal(t, len(tt.wantResultingCache), len(actualIDPList)) for i := range actualIDPList { actualIDP := actualIDPList[i].(*upstreamldap.Provider) - copyOfExpectedValue := *tt.wantResultingCache[i] // copy before edit to avoid race because these tests are run in parallel + copyOfExpectedValueForResultingCache := *tt.wantResultingCache[i] // copy before edit to avoid race because these tests are run in parallel // The dialer that was passed in to the controller's constructor should always have been // passed through to the provider. - copyOfExpectedValue.Dialer = dialer - require.Equal(t, copyOfExpectedValue, actualIDP.GetConfig()) + copyOfExpectedValueForResultingCache.Dialer = dialer + require.Equal(t, copyOfExpectedValueForResultingCache, actualIDP.GetConfig()) } actualUpstreams, err := fakePinnipedClient.IDPV1alpha1().LDAPIdentityProviders(testNamespace).List(ctx, metav1.ListOptions{}) @@ -787,10 +934,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { } // Check that the controller remembered which version of the secret it most recently validated successfully with. - if tt.wantValidatedSecretVersions == nil { - tt.wantValidatedSecretVersions = map[string]string{} + if tt.wantValidatedSettings == nil { + tt.wantValidatedSettings = map[string]validatedSettings{} } - require.Equal(t, tt.wantValidatedSecretVersions, validatedSecretVersionCache.ResourceVersionsByName) + require.Equal(t, tt.wantValidatedSettings, validatedSecretVersionCache.ValidatedSettingsByName) }) } } diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 39d51e8c..54c51293 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -157,16 +157,26 @@ func (p *Provider) dial(ctx context.Context) (Conn, error) { return nil, ldap.NewError(ldap.ErrorNetwork, err) } + // Choose how and where to dial based on TLS vs. StartTLS config option. + var dialFunc LDAPDialerFunc + var hostAndPort string switch { - case p.c.Dialer != nil: - return p.c.Dialer.Dial(ctx, tlsHostAndPort) case p.c.ConnectionProtocol == TLS: - return p.dialTLS(ctx, tlsHostAndPort) + dialFunc = p.dialTLS + hostAndPort = tlsHostAndPort case p.c.ConnectionProtocol == StartTLS: - return p.dialStartTLS(ctx, startTLSHostAndPort) + dialFunc = p.dialStartTLS + hostAndPort = startTLSHostAndPort default: return nil, ldap.NewError(ldap.ErrorNetwork, fmt.Errorf("did not specify valid ConnectionProtocol")) } + + // Override the real dialer for testing purposes sometimes. + if p.c.Dialer != nil { + dialFunc = p.c.Dialer.Dial + } + + return dialFunc(ctx, hostAndPort) } // dialTLS is a default implementation of the Dialer, used when Dialer is nil and ConnectionProtocol is TLS. diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 56203fbb..f66d69ce 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -55,11 +55,12 @@ var ( func TestEndUserAuthentication(t *testing.T) { providerConfig := func(editFunc func(p *ProviderConfig)) *ProviderConfig { config := &ProviderConfig{ - Name: "some-provider-name", - Host: testHost, - CABundle: nil, // this field is only used by the production dialer, which is replaced by a mock for this test - BindUsername: testBindUsername, - BindPassword: testBindPassword, + Name: "some-provider-name", + Host: testHost, + CABundle: nil, // this field is only used by the production dialer, which is replaced by a mock for this test + ConnectionProtocol: TLS, + BindUsername: testBindUsername, + BindPassword: testBindPassword, UserSearch: UserSearchConfig{ Base: testUserSearchBase, Filter: testUserSearchFilter, @@ -989,12 +990,13 @@ func TestEndUserAuthentication(t *testing.T) { func TestTestConnection(t *testing.T) { providerConfig := func(editFunc func(p *ProviderConfig)) *ProviderConfig { config := &ProviderConfig{ - Name: "some-provider-name", - Host: testHost, - CABundle: nil, // this field is only used by the production dialer, which is replaced by a mock for this test - BindUsername: testBindUsername, - BindPassword: testBindPassword, - UserSearch: UserSearchConfig{}, // not used by TestConnection + Name: "some-provider-name", + Host: testHost, + CABundle: nil, // this field is only used by the production dialer, which is replaced by a mock for this test + ConnectionProtocol: TLS, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: UserSearchConfig{}, // not used by TestConnection } if editFunc != nil { editFunc(config)