From cb0ee07b5152b9312ad3aa0f5a3ca97aeb8ca501 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 21 Jul 2021 13:24:54 -0700 Subject: [PATCH] Fetch AD search base from defaultNamingContext when not specified --- .../active_directory_upstream_watcher.go | 39 ++ .../active_directory_upstream_watcher_test.go | 519 +++++++++++++++++- .../ldap_upstream_watcher.go | 6 + .../ldap_upstream_watcher_test.go | 86 ++- .../upstreamwatchers/upstream_watchers.go | 70 ++- internal/upstreamldap/upstreamldap.go | 43 ++ test/integration/supervisor_login_test.go | 13 +- 7 files changed, 712 insertions(+), 64 deletions(-) diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index 27357bfe..6f83699b 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -81,6 +81,45 @@ func (s *activeDirectoryUpstreamGenericLDAPSpec) GroupSearch() upstreamwatchers. return &activeDirectoryUpstreamGenericLDAPGroupSearch{s.activeDirectoryIdentityProvider.Spec.GroupSearch} } +func (s *activeDirectoryUpstreamGenericLDAPSpec) DetectAndSetSearchBase(ctx context.Context, config *upstreamldap.ProviderConfig) *v1alpha1.Condition { + config.GroupSearch.Base = s.activeDirectoryIdentityProvider.Spec.GroupSearch.Base + config.UserSearch.Base = s.activeDirectoryIdentityProvider.Spec.UserSearch.Base + if config.GroupSearch.Base != "" && config.UserSearch.Base != "" { + // Both were already set in spec so just return; no need to query the RootDSE + return &v1alpha1.Condition{ + Type: "SearchBaseFound", + Status: v1alpha1.ConditionTrue, + Reason: "Success", + Message: "Using search base from ActiveDirectoryIdentityProvider config.", + } + } + ldapProvider := upstreamldap.New(*config) + // Query your AD server for the defaultNamingContext to get a DN to use as the search base + // when it isn't specified. + // https://ldapwiki.com/wiki/DefaultNamingContext + defaultNamingContext, err := ldapProvider.SearchForDefaultNamingContext(ctx) + if err != nil { + return &v1alpha1.Condition{ + Type: upstreamwatchers.TypeSearchBaseFound, + Status: v1alpha1.ConditionFalse, + Reason: "Error", + Message: fmt.Sprintf(`Error finding search base: %s`, err.Error()), + } + } + if config.UserSearch.Base == "" { + config.UserSearch.Base = defaultNamingContext + } + if config.GroupSearch.Base == "" { + config.GroupSearch.Base = defaultNamingContext + } + return &v1alpha1.Condition{ + Type: upstreamwatchers.TypeSearchBaseFound, + Status: v1alpha1.ConditionTrue, + Reason: "Success", + Message: "Successfully fetched defaultNamingContext to use as default search base from RootDSE.", + } +} + type activeDirectoryUpstreamGenericLDAPUserSearch struct { userSearch v1alpha1.ActiveDirectoryIdentityProviderUserSearch } diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go index f944c710..20014656 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go @@ -256,11 +256,51 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { ObservedGeneration: gen, } } + + searchBaseFoundInRootDSECondition := func(gen int64) v1alpha1.Condition { + return v1alpha1.Condition{ + Type: "SearchBaseFound", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "Successfully fetched defaultNamingContext to use as default search base from RootDSE.", + ObservedGeneration: gen, + } + } + + searchBaseFoundInConfigCondition := func(gen int64) v1alpha1.Condition { + return v1alpha1.Condition{ + Type: "SearchBaseFound", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "Using search base from ActiveDirectoryIdentityProvider config.", + ObservedGeneration: gen, + } + } + + searchBaseFoundErrorCondition := func(gen int64, message string) v1alpha1.Condition { + return v1alpha1.Condition{ + Type: "SearchBaseFound", + Status: "False", + LastTransitionTime: now, + Reason: "Error", + Message: message, + ObservedGeneration: gen, + } + } + allConditionsTrue := func(gen int64, secretVersion string) []v1alpha1.Condition { return []v1alpha1.Condition{ bindSecretValidTrueCondition(gen), activeDirectoryConnectionValidTrueCondition(gen, secretVersion), + searchBaseFoundInConfigCondition(gen), tlsConfigurationValidLoadedTrueCondition(gen), + // TODO should there be a condition when you just get it from the config? is that worth reporting? + // I'm thinking maybe no since it's not a network call or anything... it's just like any other field in the + // spec that we don't bother to report on. + // Although perhaps it would be weirder to have a condition that only sometimes exists? And it's a useful + // way to communicate internally. } } @@ -272,6 +312,34 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { } } + expectedDefaultNamingContextSearch := func() *ldap.SearchRequest { + request := &ldap.SearchRequest{ + BaseDN: "", + Scope: ldap.ScopeBaseObject, + DerefAliases: ldap.NeverDerefAliases, + SizeLimit: 2, + TimeLimit: 90, + TypesOnly: false, + Filter: "(objectClass=*)", + Attributes: []string{"defaultNamingContext"}, + Controls: nil, // don't need paging because we set the SizeLimit so small + } + return request + } + + exampleDefaultNamingContext := "dc=default,dc=naming,dc=context,dc=example,dc=com" + + exampleDefaultNamingContextSearchResult := &ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: "", + Attributes: []*ldap.EntryAttribute{ + ldap.NewEntryAttribute("defaultNamingContext", []string{exampleDefaultNamingContext}), + }, + }, + }, + } + tests := []struct { name string initialValidatedSettings map[string]upstreamwatchers.ValidatedSettings @@ -305,7 +373,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, }, { name: "missing secret", @@ -477,6 +545,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: []v1alpha1.Condition{ bindSecretValidTrueCondition(1234), activeDirectoryConnectionValidTrueCondition(1234, "4242"), + searchBaseFoundInConfigCondition(1234), { Type: "TLSConfigurationValid", Status: "True", @@ -488,7 +557,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, }, { name: "when TLS connection fails it tries to use StartTLS instead: without a specified port it automatically switches ports", @@ -542,11 +611,12 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { "ldap.example.com", testBindUsername, testSecretName, "4242"), ObservedGeneration: 1234, }, + searchBaseFoundInConfigCondition(1234), tlsConfigurationValidLoadedTrueCondition(1234), }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, }, { name: "when TLS connection fails it tries to use StartTLS instead: with a specified port it does not automatically switch ports", @@ -599,10 +669,12 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { "ldap.example.com:5678", testBindUsername, "ldap.example.com:5678"), ObservedGeneration: 1234, }, + searchBaseFoundInConfigCondition(1234), tlsConfigurationValidLoadedTrueCondition(1234), }, }, }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, }, { name: "non-nil TLS configuration with empty CertificateAuthorityData is valid", @@ -643,7 +715,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, }, { name: "one valid upstream and one invalid upstream updates the cache to include only the valid upstream", @@ -686,7 +758,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, }, { name: "when testing the connection to the LDAP server fails then the upstream is still added to the cache anyway (treated like a warning)", @@ -716,10 +788,107 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { testHost, testBindUsername, testBindUsername), ObservedGeneration: 1234, }, + searchBaseFoundInConfigCondition(1234), tlsConfigurationValidLoadedTrueCondition(1234), }, }, }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, + }, + { + name: "when testing the connection to the LDAP server fails, but later querying defaultsearchbase succeeds, then the upstream is still added to the cache anyway (treated like a warning)", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Spec.UserSearch.Base = "" + })}, + inputSecrets: []runtime.Object{validBindUserSecret("")}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Should perform a test dial and bind. + // Expect three calls bind: once for trying TLS and once for trying StartTLS (both fail), and one before querying for defaultNamingContext (succeeds) + first := conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2).Return(errors.New("some bind error")) + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1).After(first) + conn.EXPECT().Search(expectedDefaultNamingContextSearch()).Times(1).Return(exampleDefaultNamingContextSearchResult, nil) + conn.EXPECT().Close().Times(3) + }, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantResultingCache: []*upstreamldap.ProviderConfig{ + { + Name: testName, + Host: testHost, + ConnectionProtocol: upstreamldap.TLS, + CABundle: testCABundle, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: upstreamldap.UserSearchConfig{ + Base: exampleDefaultNamingContext, + Filter: testUserSearchFilter, + UsernameAttribute: testUsernameAttrName, + UIDAttribute: testUIDAttrName, + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: testGroupSearchBase, + Filter: testGroupSearchFilter, + GroupNameAttribute: testGroupNameAttrName, + }, + }, + }, + wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ + 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 binding as "%s": some bind error`, + testHost, testBindUsername, testBindUsername), + ObservedGeneration: 1234, + }, + searchBaseFoundInRootDSECondition(1234), + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase}}, + }, + { + name: "when testing the connection to the LDAP server fails, and querying defaultsearchbase fails, then the upstream is not added to the cache", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Spec.UserSearch.Base = "" + })}, + inputSecrets: []runtime.Object{validBindUserSecret("")}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Should perform a test dial and bind. + // Expect 3 calls to each of these: once for trying TLS and once for trying StartTLS, one before querying + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(3).Return(errors.New("some bind error")) + conn.EXPECT().Close().Times(3) + }, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ + 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 binding as "%s": some bind error`, + testHost, testBindUsername, testBindUsername), + ObservedGeneration: 1234, + }, + searchBaseFoundErrorCondition(1234, "Error finding search base: error binding as \"test-bind-username\" before user search: some bind error"), + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {GroupSearchBase: testGroupSearchBase}}, }, { 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", @@ -730,7 +899,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { } })}, inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, 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. }, @@ -742,7 +911,112 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, + }, + { + name: "when the LDAP server connection was already validated using TLS, but the search base wasn't, load TLS into the config and try again for the search base", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Generation = 1234 + upstream.Status.Conditions = []v1alpha1.Condition{ + activeDirectoryConnectionValidTrueCondition(1234, "4242"), + } + upstream.Spec.UserSearch.Base = "" + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Close().Times(1) + conn.EXPECT().Search(expectedDefaultNamingContextSearch()).Return(exampleDefaultNamingContextSearchResult, nil).Times(1) + }, + wantResultingCache: []*upstreamldap.ProviderConfig{ + { + Name: testName, + Host: testHost, + ConnectionProtocol: upstreamldap.TLS, + CABundle: testCABundle, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: upstreamldap.UserSearchConfig{ + Base: exampleDefaultNamingContext, + Filter: testUserSearchFilter, + UsernameAttribute: testUsernameAttrName, + UIDAttribute: testUIDAttrName, + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: testGroupSearchBase, + Filter: testGroupSearchFilter, + GroupNameAttribute: testGroupNameAttrName, + }, + }, + }, + wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + bindSecretValidTrueCondition(1234), + activeDirectoryConnectionValidTrueCondition(1234, "4242"), + searchBaseFoundInRootDSECondition(1234), + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase}}, + }, + { + name: "when the LDAP server connection was already validated using TLS, and the search base was found, load TLS and search base info into the cache", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Generation = 1234 + upstream.Status.Conditions = []v1alpha1.Condition{ + activeDirectoryConnectionValidTrueCondition(1234, "4242"), + searchBaseFoundInRootDSECondition(1234), + } + upstream.Spec.UserSearch.Base = "" + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase}}, + setupMocks: func(conn *mockldapconn.MockConn) { + }, + wantResultingCache: []*upstreamldap.ProviderConfig{ + { + Name: testName, + Host: testHost, + ConnectionProtocol: upstreamldap.TLS, + CABundle: testCABundle, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: upstreamldap.UserSearchConfig{ + Base: exampleDefaultNamingContext, + Filter: testUserSearchFilter, + UsernameAttribute: testUsernameAttrName, + UIDAttribute: testUIDAttrName, + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: testGroupSearchBase, + Filter: testGroupSearchFilter, + GroupNameAttribute: testGroupNameAttrName, + }, + }, + }, + wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + bindSecretValidTrueCondition(1234), + activeDirectoryConnectionValidTrueCondition(1234, "4242"), + searchBaseFoundInRootDSECondition(1234), + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: exampleDefaultNamingContext, + GroupSearchBase: testGroupSearchBase, + }}, }, { 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", @@ -765,7 +1039,12 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.StartTLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}, }, { name: "when the LDAP server connection was validated for an older resource generation, then try to validate it again", @@ -775,8 +1054,13 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { activeDirectoryConnectionValidTrueCondition(1233, "4242"), // older spec generation! } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -790,7 +1074,12 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}, }, { name: "when the LDAP server connection validation previously failed for this resource generation, then try to validate it again", @@ -822,7 +1111,12 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}, }, { 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", @@ -832,8 +1126,13 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { activeDirectoryConnectionValidTrueCondition(1234, "4241"), // same spec generation, old secret version } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, // newer secret version! - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4241", LDAPConnectionProtocol: upstreamldap.TLS}}, // old version was validated + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, // newer secret version! + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4241", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}, // old version was validated setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -847,7 +1146,12 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{ + testName: {BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}, }, { name: "when the input activedirectoryidentityprovider leaves user attributes blank, provide default values", @@ -888,7 +1192,190 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}, + }, + { + name: "when the input activedirectoryidentityprovider leaves user and group search base blank, query for defaultNamingContext", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{} + upstream.Spec.UserSearch.Base = "" + upstream.Spec.GroupSearch.Base = "" + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Should perform a test dial and bind. + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2) + conn.EXPECT().Close().Times(2) + conn.EXPECT().Search(expectedDefaultNamingContextSearch()).Return(exampleDefaultNamingContextSearchResult, nil).Times(1) + }, + wantResultingCache: []*upstreamldap.ProviderConfig{ + { + Name: testName, + Host: testHost, + ConnectionProtocol: upstreamldap.TLS, + CABundle: testCABundle, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: upstreamldap.UserSearchConfig{ + Base: exampleDefaultNamingContext, + Filter: testUserSearchFilter, + UsernameAttribute: "sAMAccountName", + UIDAttribute: "objectGUID", + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: exampleDefaultNamingContext, + Filter: testGroupSearchFilter, + GroupNameAttribute: testGroupNameAttrName, + }, + }, + }, + wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + bindSecretValidTrueCondition(1234), + activeDirectoryConnectionValidTrueCondition(1234, "4242"), + searchBaseFoundInRootDSECondition(1234), + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: exampleDefaultNamingContext}}, + }, + { + name: "when the input activedirectoryidentityprovider leaves user search base blank but provides group search base, query for defaultNamingContext", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{} + upstream.Spec.UserSearch.Base = "" + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Should perform a test dial and bind. + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2) + conn.EXPECT().Close().Times(2) + conn.EXPECT().Search(expectedDefaultNamingContextSearch()).Return(exampleDefaultNamingContextSearchResult, nil).Times(1) + }, + wantResultingCache: []*upstreamldap.ProviderConfig{ + { + Name: testName, + Host: testHost, + ConnectionProtocol: upstreamldap.TLS, + CABundle: testCABundle, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: upstreamldap.UserSearchConfig{ + Base: exampleDefaultNamingContext, + Filter: testUserSearchFilter, + UsernameAttribute: "sAMAccountName", + UIDAttribute: "objectGUID", + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: testGroupSearchBase, + Filter: testGroupSearchFilter, + GroupNameAttribute: testGroupNameAttrName, + }, + }, + }, + wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + bindSecretValidTrueCondition(1234), + activeDirectoryConnectionValidTrueCondition(1234, "4242"), + searchBaseFoundInRootDSECondition(1234), + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase}}, + }, + { + name: "when the input activedirectoryidentityprovider leaves group search base blank but provides user search base, query for defaultNamingContext", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{} + upstream.Spec.GroupSearch.Base = "" + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Should perform a test dial and bind. + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2) + conn.EXPECT().Close().Times(2) + conn.EXPECT().Search(expectedDefaultNamingContextSearch()).Return(exampleDefaultNamingContextSearchResult, nil).Times(1) + }, + wantResultingCache: []*upstreamldap.ProviderConfig{ + { + Name: testName, + Host: testHost, + ConnectionProtocol: upstreamldap.TLS, + CABundle: testCABundle, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: upstreamldap.UserSearchConfig{ + Base: testUserSearchBase, + Filter: testUserSearchFilter, + UsernameAttribute: "sAMAccountName", + UIDAttribute: "objectGUID", + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: exampleDefaultNamingContext, + Filter: testGroupSearchFilter, + GroupNameAttribute: testGroupNameAttrName, + }, + }, + }, + wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + bindSecretValidTrueCondition(1234), + activeDirectoryConnectionValidTrueCondition(1234, "4242"), + searchBaseFoundInRootDSECondition(1234), + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: exampleDefaultNamingContext}}, + }, + { + name: "when the input activedirectoryidentityprovider leaves group search base blank and query for defaultNamingContext fails", + // TODO is this a fatal error? I think so because leaving the search base blank and trying anyway does not seem expected. + // it could potentially succeed but return something unexpected... + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{} + upstream.Spec.GroupSearch.Base = "" + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Should perform a test dial and bind. + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2) + conn.EXPECT().Close().Times(2) + conn.EXPECT().Search(expectedDefaultNamingContextSearch()).Return(nil, errors.New("some error")).Times(1) + }, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + bindSecretValidTrueCondition(1234), + activeDirectoryConnectionValidTrueCondition(1234, "4242"), + searchBaseFoundErrorCondition(1234, "Error finding search base: error querying RootDSE for defaultNamingContext: some error"), + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{ + testName: {BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase}}, }, } diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go index 838f5574..90d172b1 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go @@ -77,6 +77,12 @@ func (s *ldapUpstreamGenericLDAPSpec) GroupSearch() upstreamwatchers.UpstreamGen return &ldapUpstreamGenericLDAPGroupSearch{s.ldapIdentityProvider.Spec.GroupSearch} } +func (s *ldapUpstreamGenericLDAPSpec) DetectAndSetSearchBase(_ context.Context, config *upstreamldap.ProviderConfig) *v1alpha1.Condition { + config.GroupSearch.Base = s.ldapIdentityProvider.Spec.GroupSearch.Base + config.UserSearch.Base = s.ldapIdentityProvider.Spec.UserSearch.Base + return nil +} + type ldapUpstreamGenericLDAPUserSearch struct { userSearch v1alpha1.LDAPIdentityProviderUserSearch } diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go index 9d475292..16993640 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go @@ -305,7 +305,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}, }, { name: "missing secret", @@ -488,8 +493,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, - }, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}}, { 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) { @@ -546,8 +555,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}}, - }, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.StartTLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}}, { 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) { @@ -603,6 +616,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}, }, { name: "non-nil TLS configuration with empty CertificateAuthorityData is valid", @@ -643,7 +660,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}, }, { name: "one valid upstream and one invalid upstream updates the cache to include only the valid upstream", @@ -686,8 +708,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, }, }, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, - }, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}}, { name: "when testing the connection to the LDAP server fails then the upstream is still added to the cache anyway (treated like a warning)", inputUpstreams: []runtime.Object{validUpstream}, @@ -720,6 +746,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}, }, { 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", @@ -742,8 +772,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, - }, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}}, { 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) { @@ -765,8 +799,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}}, - }, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.StartTLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}}, { name: "when the LDAP server connection was validated for an older resource generation, then try to validate it again", inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { @@ -790,8 +828,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, - }, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}}, { name: "when the LDAP server connection validation previously failed for this resource generation, then try to validate it again", inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { @@ -822,8 +864,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, - }, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}}, { 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", inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { @@ -847,8 +893,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, - }, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + }}}, } for _, tt := range tests { diff --git a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go index 65b34eea..58522d60 100644 --- a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go +++ b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go @@ -36,13 +36,14 @@ const ( typeBindSecretValid = "BindSecretValid" typeTLSConfigurationValid = "TLSConfigurationValid" typeLDAPConnectionValid = "LDAPConnectionValid" + TypeSearchBaseFound = "SearchBaseFound" reasonLDAPConnectionError = "LDAPConnectionError" noTLSConfigurationMessage = "no TLS configuration provided" loadedTLSConfigurationMessage = "loaded TLS configuration" ) // An in-memory cache with an entry for each ActiveDirectoryIdentityProvider, to keep track of which ResourceVersion -// of the bind Secret and which TLS/StartTLS setting was used during the most recent successful validation. +// of the bind Secret, which TLS/StartTLS setting was used and which search base was found during the most recent successful validation. type SecretVersionCache struct { ValidatedSettingsByName map[string]ValidatedSettings } @@ -50,6 +51,8 @@ type SecretVersionCache struct { type ValidatedSettings struct { BindSecretResourceVersion string LDAPConnectionProtocol upstreamldap.LDAPConnectionProtocol + UserSearchBase string + GroupSearchBase string } func NewSecretVersionCache() *SecretVersionCache { @@ -71,6 +74,7 @@ type UpstreamGenericLDAPSpec interface { BindSecretName() string UserSearch() UpstreamGenericLDAPUserSearch GroupSearch() UpstreamGenericLDAPGroupSearch + DetectAndSetSearchBase(ctx context.Context, config *upstreamldap.ProviderConfig) *v1alpha1.Condition } type UpstreamGenericLDAPUserSearch interface { @@ -161,7 +165,7 @@ func TestConnection( } } -func HasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(secretVersionCache *SecretVersionCache, currentGeneration int64, upstreamStatusConditions []v1alpha1.Condition, upstreamName string, currentSecretVersion string, config *upstreamldap.ProviderConfig) bool { +func HasPreviousSuccessfulTLSConnectionConditionForCurrentSpecGenerationAndSecretVersion(secretVersionCache *SecretVersionCache, currentGeneration int64, upstreamStatusConditions []v1alpha1.Condition, upstreamName string, currentSecretVersion string, config *upstreamldap.ProviderConfig) bool { for _, cond := range upstreamStatusConditions { if cond.Type == typeLDAPConnectionValid && cond.Status == v1alpha1.ConditionTrue && cond.ObservedGeneration == currentGeneration { // Found a previously successful condition for the current spec generation. @@ -177,6 +181,21 @@ func HasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(secr return false } +func HasPreviousSuccessfulSearchBaseConditionForCurrentGeneration(secretVersionCache *SecretVersionCache, currentGeneration int64, upstreamStatusConditions []v1alpha1.Condition, upstreamName string, currentSecretVersion string, config *upstreamldap.ProviderConfig) bool { + for _, cond := range upstreamStatusConditions { + if cond.Type == TypeSearchBaseFound && 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. + validatedSettings := secretVersionCache.ValidatedSettingsByName[upstreamName] + // Reload the TLS vs StartTLS setting that was previously validated. + config.UserSearch.Base = validatedSettings.UserSearchBase + config.GroupSearch.Base = validatedSettings.GroupSearchBase + return true + } + } + return false +} + func validTLSCondition(message string) *v1alpha1.Condition { return &v1alpha1.Condition{ Type: typeTLSConfigurationValid, @@ -267,38 +286,47 @@ func ValidateGenericLDAP(ctx context.Context, upstream UpstreamGenericLDAPIDP, s // No point in trying to connect to the server if the config was already determined to be invalid. var ldapConnectionValidCondition *v1alpha1.Condition + var searchBaseFoundCondition *v1alpha1.Condition if secretValidCondition.Status == v1alpha1.ConditionTrue && tlsValidCondition.Status == v1alpha1.ConditionTrue { - ldapConnectionValidCondition = validateAndSetLDAPServerConnectivity(ctx, validatedSecretVersionsCache, upstream, config, currentSecretVersion) + ldapConnectionValidCondition, searchBaseFoundCondition = validateAndSetLDAPServerConnectivity(ctx, validatedSecretVersionsCache, upstream, config, currentSecretVersion) if ldapConnectionValidCondition != nil { conditions.Append(ldapConnectionValidCondition, false) } + if searchBaseFoundCondition != nil { + conditions.Append(searchBaseFoundCondition, true) + } } return conditions } -func validateAndSetLDAPServerConnectivity(ctx context.Context, validatedSecretVersionsCache *SecretVersionCache, upstream UpstreamGenericLDAPIDP, config *upstreamldap.ProviderConfig, currentSecretVersion string) *v1alpha1.Condition { - // TODO refactor validateAndSetLDAPServerConnectivity to be shared and take a helper function for the defaultNamingContext stuff - // so that can be shared. - if HasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(validatedSecretVersionsCache, upstream.Generation(), upstream.Status().Conditions(), upstream.Name(), currentSecretVersion, config) { - return nil - } +func validateAndSetLDAPServerConnectivity(ctx context.Context, validatedSecretVersionsCache *SecretVersionCache, upstream UpstreamGenericLDAPIDP, config *upstreamldap.ProviderConfig, currentSecretVersion string) (*v1alpha1.Condition, *v1alpha1.Condition) { + var ldapConnectionValidCondition *v1alpha1.Condition + if !HasPreviousSuccessfulTLSConnectionConditionForCurrentSpecGenerationAndSecretVersion(validatedSecretVersionsCache, upstream.Generation(), upstream.Status().Conditions(), upstream.Name(), currentSecretVersion, config) { + testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, TestLDAPConnectionTimeout) + defer cancelFunc() - testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, TestLDAPConnectionTimeout) - defer cancelFunc() + ldapConnectionValidCondition = TestConnection(testConnectionTimeout, upstream.Spec().BindSecretName(), config, currentSecretVersion) - condition := TestConnection(testConnectionTimeout, upstream.Spec().BindSecretName(), 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. - validatedSecretVersionsCache.ValidatedSettingsByName[upstream.Name()] = ValidatedSettings{ - BindSecretResourceVersion: currentSecretVersion, - LDAPConnectionProtocol: config.ConnectionProtocol, + if ldapConnectionValidCondition.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. + validatedSecretVersionsCache.ValidatedSettingsByName[upstream.Name()] = ValidatedSettings{ + BindSecretResourceVersion: currentSecretVersion, + LDAPConnectionProtocol: config.ConnectionProtocol, + } } } + var searchBaseFoundCondition *v1alpha1.Condition + if !HasPreviousSuccessfulSearchBaseConditionForCurrentGeneration(validatedSecretVersionsCache, upstream.Generation(), upstream.Status().Conditions(), upstream.Name(), currentSecretVersion, config) { + searchBaseFoundCondition = upstream.Spec().DetectAndSetSearchBase(ctx, config) + validatedSettings := validatedSecretVersionsCache.ValidatedSettingsByName[upstream.Name()] + validatedSettings.GroupSearchBase = config.GroupSearch.Base + validatedSettings.UserSearchBase = config.UserSearch.Base + validatedSecretVersionsCache.ValidatedSettingsByName[upstream.Name()] = validatedSettings + } - return condition + return ldapConnectionValidCondition, searchBaseFoundCondition } func EvaluateConditions(conditions GradatedConditions, config *upstreamldap.ProviderConfig) (provider.UpstreamLDAPIdentityProviderI, bool) { diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index e58e76b4..f6bee32f 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -392,6 +392,35 @@ func (p *Provider) validateConfig() error { return nil } +func (p *Provider) SearchForDefaultNamingContext(ctx context.Context) (string, error) { + t := trace.FromContext(ctx).Nest("slow ldap authenticate user attempt", trace.Field{Key: "providerName", Value: p.GetName()}) + defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches + + conn, err := p.dial(ctx) + if err != nil { + p.traceAuthFailure(t, err) + return "", fmt.Errorf(`error dialing host "%s": %w`, p.c.Host, err) + } + defer conn.Close() + + err = conn.Bind(p.c.BindUsername, p.c.BindPassword) + if err != nil { + p.traceAuthFailure(t, err) + return "", fmt.Errorf(`error binding as "%s" before user search: %w`, p.c.BindUsername, err) + } + + searchResult, err := conn.Search(p.defaultNamingContextRequest()) + if err != nil { + return "", fmt.Errorf(`error querying RootDSE for defaultNamingContext: %w`, err) + } + // TODO handle getting empty entry back-- I think this is possible but we might want to + // treat it as an error + // TODO handle getting no entries back + // TODO handle getting more than 1 result back + // TODO handle getting no values for defaultNamingContext attribute back in entry + return searchResult.Entries[0].GetAttributeValue("defaultNamingContext"), nil +} + func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(conn Conn, foundUserDN string) error) (string, string, []string, error) { searchResult, err := conn.Search(p.userSearchRequest(username)) if err != nil { @@ -462,6 +491,20 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c return mappedUsername, mappedUID, mappedGroupNames, nil } +func (p *Provider) defaultNamingContextRequest() *ldap.SearchRequest { + return &ldap.SearchRequest{ + BaseDN: "", + Scope: ldap.ScopeBaseObject, + DerefAliases: ldap.NeverDerefAliases, + SizeLimit: 2, + TimeLimit: 90, + TypesOnly: false, + Filter: "(objectClass=*)", + Attributes: []string{"defaultNamingContext"}, + Controls: nil, // don't need paging because we set the SizeLimit so small + } +} + func (p *Provider) userSearchRequest(username string) *ldap.SearchRequest { // See https://ldap.com/the-ldap-search-operation for general documentation of LDAP search options. return &ldap.SearchRequest{ diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 17c4ec55..6a556a29 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -254,12 +254,6 @@ func TestSupervisorLogin(t *testing.T) { TLS: &idpv1alpha1.TLSSpec{ CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamActiveDirectory.CABundle)), }, - UserSearch: idpv1alpha1.ActiveDirectoryIdentityProviderUserSearch{ - Base: "dc=activedirectory,dc=test,dc=pinniped,dc=dev", - }, - GroupSearch: idpv1alpha1.ActiveDirectoryIdentityProviderGroupSearch{ - Base: "dc=activedirectory,dc=test,dc=pinniped,dc=dev", - }, Bind: idpv1alpha1.ActiveDirectoryIdentityProviderBind{ SecretName: secret.Name, }, @@ -269,7 +263,7 @@ func TestSupervisorLogin(t *testing.T) { env.SupervisorUpstreamActiveDirectory.Host, env.SupervisorUpstreamActiveDirectory.BindUsername, secret.Name, secret.ResourceVersion, ) - requireSuccessfulActiveDirectoryIdentityProviderConditions(t, adIDP, expectedMsg) // TODO refactor to be same as LDAP func + requireSuccessfulActiveDirectoryIdentityProviderConditions(t, adIDP, expectedMsg) }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingLDAPIdentityProvider(t, @@ -282,7 +276,7 @@ func TestSupervisorLogin(t *testing.T) { // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: regexp.QuoteMeta( "ldaps://" + env.SupervisorUpstreamActiveDirectory.Host + - "?base=" + url.QueryEscape("dc=activedirectory,dc=test,dc=pinniped,dc=dev") + + "?base=" + url.QueryEscape("DC=activedirectory,DC=test,DC=pinniped,DC=dev") + "&sub=" + env.SupervisorUpstreamActiveDirectory.TestUserUniqueIDAttributeValue, ), // the ID token Username should have been pulled from the requested UserSearch.Attributes.Username attribute @@ -331,7 +325,7 @@ func requireSuccessfulLDAPIdentityProviderConditions(t *testing.T, ldapIDP *idpv }, conditionsSummary) } func requireSuccessfulActiveDirectoryIdentityProviderConditions(t *testing.T, adIDP *idpv1alpha1.ActiveDirectoryIdentityProvider, expectedActiveDirectoryConnectionValidMessage string) { - require.Len(t, adIDP.Status.Conditions, 3) + require.Len(t, adIDP.Status.Conditions, 4) conditionsSummary := [][]string{} for _, condition := range adIDP.Status.Conditions { @@ -352,6 +346,7 @@ func requireSuccessfulActiveDirectoryIdentityProviderConditions(t *testing.T, ad {"BindSecretValid", "True", "Success"}, {"TLSConfigurationValid", "True", "Success"}, {"LDAPConnectionValid", "True", "Success"}, + {"SearchBaseFound", "True", "Success"}, }, conditionsSummary) }