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 a2326ac2c..f3acde8d5 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go @@ -729,7 +729,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{}, }, { name: "non-nil TLS configuration with empty CertificateAuthorityData is valid", @@ -1558,6 +1558,69 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase}}, }, + { + name: "when search base was previously found but the bind secret has changed", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Generation = 1234 + upstream.Status.Conditions = []v1alpha1.Condition{ + searchBaseFoundInRootDSECondition(1234), + } + upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{} + upstream.Spec.GroupSearch.Base = "" + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4241", + 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(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: "userPrincipalName", + UIDAttribute: "objectGUID", + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: exampleDefaultNamingContext, + Filter: testGroupSearchFilter, + GroupNameAttribute: testGroupNameAttrName, + }, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, + }, + }, + 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, + GroupSearchBase: exampleDefaultNamingContext, + UserSearchBase: testUserSearchBase}}, + }, } for _, tt := range tests { diff --git a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go index 3801cb21e..daf24e756 100644 --- a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go +++ b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go @@ -183,16 +183,18 @@ func HasPreviousSuccessfulTLSConnectionConditionForCurrentSpecGenerationAndSecre return false } -func HasPreviousSuccessfulSearchBaseConditionForCurrentGeneration(secretVersionCache *SecretVersionCache, currentGeneration int64, upstreamStatusConditions []v1alpha1.Condition, upstreamName string, currentSecretVersion string, config *upstreamldap.ProviderConfig) bool { +func HasPreviousSuccessfulSearchBaseConditionForCurrentGeneration(secretVersionCache *SecretVersionCache, currentGeneration int64, upstreamStatusConditions []v1alpha1.Condition, upstreamName string, currentSecretVersion string, previouslyValidatedSecretVersion 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 user search and group search base settings that were previously validated. - config.UserSearch.Base = validatedSettings.UserSearchBase - config.GroupSearch.Base = validatedSettings.GroupSearchBase - return true + if previouslyValidatedSecretVersion == currentSecretVersion { + // Reload the user search and group search base settings that were previously validated. + config.UserSearch.Base = validatedSettings.UserSearchBase + config.GroupSearch.Base = validatedSettings.GroupSearchBase + return true + } } } return false @@ -302,6 +304,7 @@ func ValidateGenericLDAP(ctx context.Context, upstream UpstreamGenericLDAPIDP, s } func validateAndSetLDAPServerConnectivityAndSearchBase(ctx context.Context, validatedSecretVersionsCache *SecretVersionCache, upstream UpstreamGenericLDAPIDP, config *upstreamldap.ProviderConfig, currentSecretVersion string) (*v1alpha1.Condition, *v1alpha1.Condition) { + previouslyValidatedSecretVersion := validatedSecretVersionsCache.ValidatedSettingsByName[upstream.Name()].BindSecretResourceVersion var ldapConnectionValidCondition *v1alpha1.Condition if !HasPreviousSuccessfulTLSConnectionConditionForCurrentSpecGenerationAndSecretVersion(validatedSecretVersionsCache, upstream.Generation(), upstream.Status().Conditions(), upstream.Name(), currentSecretVersion, config) { testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, probeLDAPTimeout) @@ -320,16 +323,19 @@ func validateAndSetLDAPServerConnectivityAndSearchBase(ctx context.Context, vali } } var searchBaseFoundCondition *v1alpha1.Condition - if !HasPreviousSuccessfulSearchBaseConditionForCurrentGeneration(validatedSecretVersionsCache, upstream.Generation(), upstream.Status().Conditions(), upstream.Name(), currentSecretVersion, config) { + if !HasPreviousSuccessfulSearchBaseConditionForCurrentGeneration(validatedSecretVersionsCache, upstream.Generation(), upstream.Status().Conditions(), upstream.Name(), currentSecretVersion, previouslyValidatedSecretVersion, config) { searchBaseTimeout, cancelFunc := context.WithTimeout(ctx, probeLDAPTimeout) defer cancelFunc() searchBaseFoundCondition = upstream.Spec().DetectAndSetSearchBase(searchBaseTimeout, config) validatedSettings := validatedSecretVersionsCache.ValidatedSettingsByName[upstream.Name()] - validatedSettings.GroupSearchBase = config.GroupSearch.Base - validatedSettings.UserSearchBase = config.UserSearch.Base - validatedSecretVersionsCache.ValidatedSettingsByName[upstream.Name()] = validatedSettings + if validatedSettings.BindSecretResourceVersion == currentSecretVersion { + // only update the rest of the cached settings if the secret versions match. + validatedSettings.GroupSearchBase = config.GroupSearch.Base + validatedSettings.UserSearchBase = config.UserSearch.Base + validatedSecretVersionsCache.ValidatedSettingsByName[upstream.Name()] = validatedSettings + } } return ldapConnectionValidCondition, searchBaseFoundCondition diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 504f4fef5..b6939f2eb 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/oauth2" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" idpv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" @@ -448,6 +449,78 @@ func TestSupervisorLogin(t *testing.T) { wantDownstreamIDTokenUsernameToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamActiveDirectory.TestUserMailAttributeValue) + "$", wantDownstreamIDTokenGroups: env.SupervisorUpstreamActiveDirectory.TestUserDirectGroupsDNs, }, + { + name: "active directory login still works after deleting and recreating bind secret", + maybeSkip: func(t *testing.T) { + t.Helper() + if len(env.ToolsNamespace) == 0 && !env.HasCapability(testlib.CanReachInternetLDAPPorts) { + t.Skip("LDAP integration test requires connectivity to an LDAP server") + } + if env.SupervisorUpstreamActiveDirectory.Host == "" { + t.Skip("Active Directory hostname not specified") + } + }, + createIDP: func(t *testing.T) { + t.Helper() + + secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ad-service-account", v1.SecretTypeBasicAuth, + map[string]string{ + v1.BasicAuthUsernameKey: env.SupervisorUpstreamActiveDirectory.BindUsername, + v1.BasicAuthPasswordKey: env.SupervisorUpstreamActiveDirectory.BindPassword, + }, + ) + secretName := secret.Name + adIDP := testlib.CreateTestActiveDirectoryIdentityProvider(t, idpv1alpha1.ActiveDirectoryIdentityProviderSpec{ + Host: env.SupervisorUpstreamActiveDirectory.Host, + TLS: &idpv1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamActiveDirectory.CABundle)), + }, + Bind: idpv1alpha1.ActiveDirectoryIdentityProviderBind{ + SecretName: secretName, + }, + }, idpv1alpha1.ActiveDirectoryPhaseReady) + + secret.Annotations = map[string]string{"pinniped.dev/test": "", "another-label": "another-key"} + // update that secret, which will cause the cache to + client := testlib.NewKubernetesClientset(t) + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + updatedSecret, err := client.CoreV1().Secrets(env.SupervisorNamespace).Update(ctx, secret, metav1.UpdateOptions{}) + require.NoError(t, err) + + expectedMsg := fmt.Sprintf( + `successfully able to connect to "%s" and bind as user "%s" [validated with Secret "%s" at version "%s"]`, + env.SupervisorUpstreamActiveDirectory.Host, env.SupervisorUpstreamActiveDirectory.BindUsername, + updatedSecret.Name, updatedSecret.ResourceVersion, + ) + supervisorClient := testlib.NewSupervisorClientset(t) + testlib.RequireEventually(t, func(requireEventually *require.Assertions) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + adIDP, err = supervisorClient.IDPV1alpha1().ActiveDirectoryIdentityProviders(env.SupervisorNamespace).Get(ctx, adIDP.Name, metav1.GetOptions{}) + requireEventually.NoError(err) + requireEventuallySuccessfulActiveDirectoryIdentityProviderConditions(t, requireEventually, adIDP, expectedMsg) + }, time.Minute, 500*time.Millisecond) + }, + requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { + requestAuthorizationUsingCLIPasswordFlow(t, + downstreamAuthorizeURL, + env.SupervisorUpstreamActiveDirectory.TestUserPrincipalNameValue, // username to present to server during login + env.SupervisorUpstreamActiveDirectory.TestUserPassword, // password to present to server during login + httpClient, + false, + ) + }, + // 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(env.SupervisorUpstreamActiveDirectory.DefaultNamingContextSearchBase)+ + "&sub="+env.SupervisorUpstreamActiveDirectory.TestUserUniqueIDAttributeValue, + ) + "$", + // the ID token Username should have been pulled from the requested UserSearch.Attributes.Username attribute + wantDownstreamIDTokenUsernameToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamActiveDirectory.TestUserPrincipalNameValue) + "$", + wantDownstreamIDTokenGroups: env.SupervisorUpstreamActiveDirectory.TestUserIndirectGroupsSAMAccountPlusDomainNames, + }, { name: "logging in to activedirectory with a deactivated user fails", maybeSkip: func(t *testing.T) { @@ -570,6 +643,40 @@ func requireSuccessfulActiveDirectoryIdentityProviderConditions(t *testing.T, ad }, conditionsSummary) } +func requireEventuallySuccessfulActiveDirectoryIdentityProviderConditions(t *testing.T, requireEventually *require.Assertions, adIDP *idpv1alpha1.ActiveDirectoryIdentityProvider, expectedActiveDirectoryConnectionValidMessage string) { + t.Helper() + requireEventually.Len(adIDP.Status.Conditions, 4) + + conditionsSummary := [][]string{} + for _, condition := range adIDP.Status.Conditions { + conditionsSummary = append(conditionsSummary, []string{condition.Type, string(condition.Status), condition.Reason}) + t.Logf("Saw ActiveDirectoryIdentityProvider Status.Condition Type=%s Status=%s Reason=%s Message=%s", + condition.Type, string(condition.Status), condition.Reason, condition.Message) + switch condition.Type { + case "BindSecretValid": + requireEventually.Equal("loaded bind secret", condition.Message) + case "TLSConfigurationValid": + requireEventually.Equal("loaded TLS configuration", condition.Message) + case "LDAPConnectionValid": + requireEventually.Equal(expectedActiveDirectoryConnectionValidMessage, condition.Message) + } + } + + expectedUserSearchReason := "" + if adIDP.Spec.UserSearch.Base == "" || adIDP.Spec.GroupSearch.Base == "" { + expectedUserSearchReason = "Success" + } else { + expectedUserSearchReason = "UsingConfigurationFromSpec" + } + + requireEventually.ElementsMatch([][]string{ + {"BindSecretValid", "True", "Success"}, + {"TLSConfigurationValid", "True", "Success"}, + {"LDAPConnectionValid", "True", "Success"}, + {"SearchBaseFound", "True", expectedUserSearchReason}, + }, conditionsSummary) +} + func testSupervisorLogin( t *testing.T, createIDP func(t *testing.T), diff --git a/test/testlib/client.go b/test/testlib/client.go index 9345ffa32..d75f37d9b 100644 --- a/test/testlib/client.go +++ b/test/testlib/client.go @@ -464,7 +464,7 @@ func CreateTestActiveDirectoryIdentityProvider(t *testing.T, spec idpv1alpha1.Ac }) t.Logf("created test ActiveDirectoryIdentityProvider %s", created.Name) - // Wait for the LDAPIdentityProvider to enter the expected phase (or time out). + // Wait for the ActiveDirectoryIdentityProvider to enter the expected phase (or time out). var result *idpv1alpha1.ActiveDirectoryIdentityProvider RequireEventuallyf(t, func(requireEventually *require.Assertions) {