Make sure search base in the validatedSettings cache is properly updated when the bind secret changes

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 {

internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go

@@ -183,18 +183,20 @@ 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]
+			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,17 +323,20 @@ 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()]
+		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
 }

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),

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) {