diff --git a/ROADMAP.md b/ROADMAP.md index 4cf5e5b4..6b7eb097 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -40,7 +40,9 @@ Last Updated: Sept 2021 |Improving Security Posture|Supervisor token refresh fails when the upstream user is in an invalid state for LDAP/AD |Jan 2022| |Improving Security Posture|Set stricter default TLS versions and Ciphers |Jan 2022| |Improving Security Posture|Support FIPS compliant Boring crypto libraries |Feb 2022| -|Improving Security Posture|Support Audit logging of security events related to Authentication |March 2022| +|Multiple IDP support|Support multiple IDPs configured on a single Supervisor|March/April 2022| +|Improving Security Posture|TLS hardening |March/April 2022| +|Improving Security Posture|Support Audit logging of security events related to Authentication |April/May 2022| |Improving Security Posture|mTLS for Supervisor sessions |Exploring/Ongoing| |Improving Security Posture|Key management/rotation for Pinniped components with minimal downtime |Exploring/Ongoing| |Improving Security Posture|Support for Session Logout |Exploring/Ongoing| @@ -52,7 +54,6 @@ Last Updated: Sept 2021 |Improving Security Posture|Client side use of Secure Enclaves for Session data |Exploring/Ongoing| |Improving Security Posture|Enforce the use of HTTP Strict Transport (HSTS) |Exploring/Ongoing| |Improving Security Posture|Assert that Pinniped runs under the restricted PSP version2 levels |Exploring/Ongoing| -|Multiple IDP support|Support multiple IDPs configured on a single Supervisor|Exploring/Ongoing| |Wider Concierge cluster support|Support for OpenShift cluster types in the Concierge|Exploring/Ongoing| |Identity transforms|Support prefixing, filtering, or performing coarse-grained checks on upstream users and groups|Exploring/Ongoing| |CLI SSO|Support Kerberos based authentication on CLI |Exploring/Ongoing| diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index 8cc13256..33d1557a 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -226,7 +226,7 @@ type UpstreamActiveDirectoryIdentityProviderICache interface { type activeDirectoryWatcherController struct { cache UpstreamActiveDirectoryIdentityProviderICache - validatedSecretVersionsCache upstreamwatchers.SecretVersionCacheI + validatedSettingsCache upstreamwatchers.ValidatedSettingsCacheI ldapDialer upstreamldap.LDAPDialer client pinnipedclientset.Interface activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer @@ -243,8 +243,8 @@ func New( ) controllerlib.Controller { return newInternal( idpCache, - // start with an empty secretVersionCache - upstreamwatchers.NewSecretVersionCache(), + // start with an empty cache + upstreamwatchers.NewValidatedSettingsCache(), // nil means to use a real production dialer when creating objects to add to the cache nil, client, @@ -257,7 +257,7 @@ func New( // For test dependency injection purposes. func newInternal( idpCache UpstreamActiveDirectoryIdentityProviderICache, - validatedSecretVersionsCache upstreamwatchers.SecretVersionCacheI, + validatedSettingsCache upstreamwatchers.ValidatedSettingsCacheI, ldapDialer upstreamldap.LDAPDialer, client pinnipedclientset.Interface, activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer, @@ -266,7 +266,7 @@ func newInternal( ) controllerlib.Controller { c := activeDirectoryWatcherController{ cache: idpCache, - validatedSecretVersionsCache: validatedSecretVersionsCache, + validatedSettingsCache: validatedSettingsCache, ldapDialer: ldapDialer, client: client, activeDirectoryIdentityProviderInformer: activeDirectoryIdentityProviderInformer, @@ -351,7 +351,7 @@ func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context, } } - conditions := upstreamwatchers.ValidateGenericLDAP(ctx, adUpstreamImpl, c.secretInformer, c.validatedSecretVersionsCache, config) + conditions := upstreamwatchers.ValidateGenericLDAP(ctx, adUpstreamImpl, c.secretInformer, c.validatedSettingsCache, config) c.updateStatus(ctx, upstream, conditions.Conditions()) 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 713e32f7..7777bfec 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go @@ -150,8 +150,8 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { const ( testNamespace = "test-namespace" - testResourceUID = "test-uid" testName = "test-name" + testResourceUID = "test-uid" testSecretName = "test-bind-secret" testBindUsername = "test-bind-username" testBindPassword = "test-bind-password" @@ -255,6 +255,19 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { ObservedGeneration: gen, } } + activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration := func(secretVersion string) v1alpha1.Condition { + c := activeDirectoryConnectionValidTrueCondition(0, secretVersion) + c.LastTransitionTime = metav1.Time{} + return c + } + condPtr := func(c v1alpha1.Condition) *v1alpha1.Condition { + return &c + } + withoutTime := func(c v1alpha1.Condition) v1alpha1.Condition { + c = *c.DeepCopy() + c.LastTransitionTime = metav1.Time{} + return c + } tlsConfigurationValidLoadedTrueCondition := func(gen int64) v1alpha1.Condition { return v1alpha1.Condition{ Type: "TLSConfigurationValid", @@ -377,7 +390,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "missing secret", @@ -568,7 +589,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "sAMAccountName explicitly provided as group name attribute does not add an override", @@ -629,7 +658,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "when TLS connection fails it tries to use StartTLS instead: without a specified port it automatically switches ports", @@ -695,7 +732,22 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.StartTLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: &v1alpha1.Condition{ + Type: "LDAPConnectionValid", + Status: "True", + 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"), + }, + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "when TLS connection fails it tries to use StartTLS instead: with a specified port it does not automatically switch ports", @@ -808,7 +860,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "one valid upstream and one invalid upstream updates the cache to include only the valid upstream", @@ -852,7 +912,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "when testing the connection to the LDAP server fails then the upstream is still added to the cache anyway but not to validatedsettings (treated like a warning)", @@ -1003,8 +1071,16 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { searchBaseFoundInConfigCondition(1234), } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, 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. }, @@ -1016,7 +1092,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "when the validated cache contains LDAP server info but the search base is empty, reload everything", @@ -1029,8 +1113,12 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { } upstream.Spec.UserSearch.Base = "" })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, Generation: 1234}}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + IDPSpecGeneration: 1234, + }}, setupMocks: func(conn *mockldapconn.MockConn) { conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2) conn.EXPECT().Close().Times(2) @@ -1075,7 +1163,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: exampleDefaultNamingContext, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), + }}, }, { 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", @@ -1087,8 +1183,16 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { } upstream.Spec.UserSearch.Base = "" })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: exampleDefaultNamingContext, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), + }}, setupMocks: func(conn *mockldapconn.MockConn) { }, wantResultingCache: []*upstreamldap.ProviderConfig{ @@ -1136,7 +1240,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), }}, }, { @@ -1148,8 +1254,16 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { searchBaseFoundInConfigCondition(1234), } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS, Generation: 1234, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.StartTLS, + IDPSpecGeneration: 1234, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, 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. }, @@ -1166,7 +1280,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, }, { @@ -1183,7 +1299,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1233, + IDPSpecGeneration: 1233, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. @@ -1203,7 +1321,49 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, + }, + { + name: "when the LDAP server connection condition failed to update previously, then write the cached condition from the previous connection validation", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Generation = 1234 // current generation + upstream.Status.Conditions = []v1alpha1.Condition{ + activeDirectoryConnectionValidTrueCondition(1234, "4200"), // old version of the condition, as if the previous update of conditions had failed + } + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + IDPSpecGeneration: 1234, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), // already previously validated with version 4242 + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, + setupMocks: func(conn *mockldapconn.MockConn) { + // The connection had already been validated previously and the result was cached, so don't probe the server again. + // 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{providerConfigForValidUpstreamWithTLS}, + wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, UID: testResourceUID, Generation: 1234}, + Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ + Phase: "Ready", + Conditions: allConditionsTrue(1234, "4242"), // updated version of the condition using the cached condition value + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, }, { @@ -1221,8 +1381,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "1", LDAPConnectionProtocol: upstreamldap.TLS, Generation: 1234}}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -1241,7 +1400,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, }, { @@ -1258,7 +1419,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4241")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, // old version was validated setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. @@ -1273,13 +1436,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{ - testName: {BindSecretResourceVersion: "4242", - LDAPConnectionProtocol: upstreamldap.TLS, - UserSearchBase: testUserSearchBase, - GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "when the input activedirectoryidentityprovider leaves user attributes blank, provide default values", @@ -1336,7 +1501,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, }, { @@ -1398,7 +1565,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: exampleDefaultNamingContext, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), }}, }, { @@ -1454,7 +1623,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: exampleDefaultNamingContext, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), + }}, }, { name: "when the input activedirectoryidentityprovider leaves group search base blank but provides user search base, query for defaultNamingContext", @@ -1509,7 +1686,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: exampleDefaultNamingContext, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: exampleDefaultNamingContext, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), + }}, }, { name: "when the input activedirectoryidentityprovider leaves group search base blank and query for defaultNamingContext fails", @@ -1662,7 +1847,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4241")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), }}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. @@ -1712,10 +1899,12 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }}, wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{ testName: {BindSecretResourceVersion: "4242", - LDAPConnectionProtocol: upstreamldap.TLS, - GroupSearchBase: exampleDefaultNamingContext, - UserSearchBase: testUserSearchBase, - Generation: 1234, + LDAPConnectionProtocol: upstreamldap.TLS, + GroupSearchBase: exampleDefaultNamingContext, + UserSearchBase: testUserSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), }}, }, } @@ -1752,20 +1941,20 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { return conn, nil })} - var validatedSecretVersionCache *upstreamwatchers.SecretVersionCache + var validatedSettingsCache *upstreamwatchers.ValidatedSettingsCache if tt.initialValidatedSettings != nil { - validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{ + validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{ ValidatedSettingsByName: tt.initialValidatedSettings, } } else { - validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{ + validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{ ValidatedSettingsByName: map[string]upstreamwatchers.ValidatedSettings{}, } } controller := newInternal( cache, - validatedSecretVersionCache, + validatedSettingsCache, dialer, fakePinnipedClient, pinnipedInformers.IDP().V1alpha1().ActiveDirectoryIdentityProviders(), @@ -1850,7 +2039,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { if tt.wantValidatedSettings == nil { tt.wantValidatedSettings = map[string]upstreamwatchers.ValidatedSettings{} } - require.Equal(t, tt.wantValidatedSettings, validatedSecretVersionCache.ValidatedSettingsByName) + require.Equal(t, tt.wantValidatedSettings, validatedSettingsCache.ValidatedSettingsByName) }) } } diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go index bcbd6d91..82a9fdec 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package ldapupstreamwatcher implements a controller which watches LDAPIdentityProviders. @@ -134,7 +134,7 @@ type UpstreamLDAPIdentityProviderICache interface { type ldapWatcherController struct { cache UpstreamLDAPIdentityProviderICache - validatedSecretVersionsCache upstreamwatchers.SecretVersionCacheI + validatedSettingsCache upstreamwatchers.ValidatedSettingsCacheI ldapDialer upstreamldap.LDAPDialer client pinnipedclientset.Interface ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer @@ -151,8 +151,8 @@ func New( ) controllerlib.Controller { return newInternal( idpCache, - // start with an empty secretVersionCache - upstreamwatchers.NewSecretVersionCache(), + // start with an empty cache + upstreamwatchers.NewValidatedSettingsCache(), // nil means to use a real production dialer when creating objects to add to the cache nil, client, @@ -165,7 +165,7 @@ func New( // For test dependency injection purposes. func newInternal( idpCache UpstreamLDAPIdentityProviderICache, - validatedSecretVersionsCache upstreamwatchers.SecretVersionCacheI, + validatedSettingsCache upstreamwatchers.ValidatedSettingsCacheI, ldapDialer upstreamldap.LDAPDialer, client pinnipedclientset.Interface, ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer, @@ -174,7 +174,7 @@ func newInternal( ) controllerlib.Controller { c := ldapWatcherController{ cache: idpCache, - validatedSecretVersionsCache: validatedSecretVersionsCache, + validatedSettingsCache: validatedSettingsCache, ldapDialer: ldapDialer, client: client, ldapIdentityProviderInformer: ldapIdentityProviderInformer, @@ -243,7 +243,7 @@ func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream * Dialer: c.ldapDialer, } - conditions := upstreamwatchers.ValidateGenericLDAP(ctx, &ldapUpstreamGenericLDAPImpl{*upstream}, c.secretInformer, c.validatedSecretVersionsCache, config) + conditions := upstreamwatchers.ValidateGenericLDAP(ctx, &ldapUpstreamGenericLDAPImpl{*upstream}, c.secretInformer, c.validatedSettingsCache, config) c.updateStatus(ctx, upstream, conditions.Conditions()) diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go index c562d969..d7f82edf 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package ldapupstreamwatcher @@ -253,6 +253,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObservedGeneration: gen, } } + ldapConnectionValidTrueConditionWithoutTimeOrGeneration := func(secretVersion string) v1alpha1.Condition { + c := ldapConnectionValidTrueCondition(0, secretVersion) + c.LastTransitionTime = metav1.Time{} + return c + } + condPtr := func(c v1alpha1.Condition) *v1alpha1.Condition { + return &c + } tlsConfigurationValidLoadedTrueCondition := func(gen int64) v1alpha1.Condition { return v1alpha1.Condition{ Type: "TLSConfigurationValid", @@ -317,7 +325,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), }}, }, { @@ -507,8 +516,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, { 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) { @@ -571,8 +582,17 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: &v1alpha1.Condition{ + Type: "LDAPConnectionValid", + Status: "True", + 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"), + }, + }}, + }, { 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) { @@ -676,7 +696,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), }}, }, { @@ -726,8 +747,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, { name: "when testing the connection to the LDAP server fails then the upstream is still added to the cache anyway (treated like a warning) but not the validated settings cache", inputUpstreams: []runtime.Object{validUpstream}, @@ -773,10 +796,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { inputSecrets: []runtime.Object{validBindUserSecret("4242")}, initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{ testName: {BindSecretResourceVersion: "4242", - LDAPConnectionProtocol: upstreamldap.TLS, - UserSearchBase: testUserSearchBase, - GroupSearchBase: testGroupSearchBase, - Generation: 1234, + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), }}, 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. @@ -794,8 +818,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, { 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) { @@ -810,7 +836,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), }}, 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. @@ -828,8 +855,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, { 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) { @@ -842,7 +871,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, - Generation: 1233, + IDPSpecGeneration: 1233, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, }}, @@ -864,8 +893,48 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, + { + name: "when the LDAP server connection condition failed to update previously, then write the cached condition from the previous connection validation", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { + upstream.Generation = 1234 // current generation + upstream.Status.Conditions = []v1alpha1.Condition{ + ldapConnectionValidTrueCondition(1234, "4200"), // old version of the condition, as if the previous update of conditions had failed + } + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + IDPSpecGeneration: 1234, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), // already previously validated with version 4242 + }}, + setupMocks: func(conn *mockldapconn.MockConn) { + // The connection had already been validated previously and the result was cached, so don't probe the server again. + // 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{providerConfigForValidUpstreamWithTLS}, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testResourceUID}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Ready", + Conditions: allConditionsTrue(1234, "4242"), // updated version of the condition using the cached condition value + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, { 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) { @@ -900,8 +969,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, { + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, + { name: "when the validated settings cache is incomplete, then try to validate it again", // this shouldn't happen, but if it does, just throw it out and try again. inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { @@ -940,7 +1012,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), }}, }, { @@ -957,7 +1030,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, }}, // old version was validated setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. @@ -977,7 +1050,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), }}}, } @@ -1013,20 +1087,20 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { return conn, nil })} - var validatedSecretVersionCache *upstreamwatchers.SecretVersionCache + var validatedSettingsCache *upstreamwatchers.ValidatedSettingsCache if tt.initialValidatedSettings != nil { - validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{ + validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{ ValidatedSettingsByName: tt.initialValidatedSettings, } } else { - validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{ + validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{ ValidatedSettingsByName: map[string]upstreamwatchers.ValidatedSettings{}, } } controller := newInternal( cache, - validatedSecretVersionCache, + validatedSettingsCache, dialer, fakePinnipedClient, pinnipedInformers.IDP().V1alpha1().LDAPIdentityProviders(), @@ -1075,7 +1149,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { if tt.wantValidatedSettings == nil { tt.wantValidatedSettings = map[string]upstreamwatchers.ValidatedSettings{} } - require.Equal(t, tt.wantValidatedSettings, validatedSecretVersionCache.ValidatedSettingsByName) + require.Equal(t, tt.wantValidatedSettings, validatedSettingsCache.ValidatedSettingsByName) }) } } diff --git a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go index 60d09eba..5c33ea2d 100644 --- a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go +++ b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package upstreamwatchers @@ -44,45 +44,61 @@ const ( ReasonErrorFetchingSearchBase = "ErrorFetchingSearchBase" ) -// An in-memory cache with an entry for each ActiveDirectoryIdentityProvider, to keep track of which ResourceVersion -// of the bind Secret, which TLS/StartTLS setting was used and which search base was found during the most recent successful validation. -type SecretVersionCacheI interface { - Get(upstreamName, resourceVersion string, generation int64) (ValidatedSettings, bool) - Set(upstreamName, resourceVersion string, generation int64, settings ValidatedSettings) +// ValidatedSettings is the struct which is cached by the ValidatedSettingsCacheI interface. +type ValidatedSettings struct { + IDPSpecGeneration int64 // which IDP spec was used during the validation + BindSecretResourceVersion string // which bind secret was used during the validation + + // Cache the setting for TLS vs StartTLS. This is always auto-discovered by probing the server. + LDAPConnectionProtocol upstreamldap.LDAPConnectionProtocol + + // Cache the settings for search bases. These could be configured by the IDP spec, or in the + // case of AD they can also be auto-discovered by probing the server. + UserSearchBase, GroupSearchBase string + + // Cache copies of the conditions that were computed when the above settings were cached, so we + // can keep writing them to the status in the future. This matters most when the first attempt + // to write them to the IDP's status fails. In this case, future Syncs calls will be able to + // use these cached values to try writing them again. + ConnectionValidCondition, SearchBaseFoundCondition *v1alpha1.Condition } -type SecretVersionCache struct { +// ValidatedSettingsCacheI is an interface for an in-memory cache with an entry for each upstream +// provider. It keeps track of settings that were already validated for a given IDP spec and bind +// secret for that upstream. +type ValidatedSettingsCacheI interface { + // Get the cached settings for a given upstream at a given generation which was previously + // validated using a given bind secret version. If no settings have been cached for the + // upstream, or if the settings were cached at a different generation of the upstream or + // using a different version of the bind secret, then return false to indicate that the + // desired settings were not cached yet for that combination of spec generation and secret version. + Get(upstreamName, resourceVersion string, idpSpecGeneration int64) (ValidatedSettings, bool) + + // Set some settings into the cache for a given upstream. + Set(upstreamName string, settings ValidatedSettings) +} + +type ValidatedSettingsCache struct { ValidatedSettingsByName map[string]ValidatedSettings } -func (s *SecretVersionCache) Get(upstreamName, resourceVersion string, generation int64) (ValidatedSettings, bool) { - validatedSettings := s.ValidatedSettingsByName[upstreamName] - if validatedSettings.BindSecretResourceVersion == resourceVersion && - validatedSettings.Generation == generation && validatedSettings.UserSearchBase != "" && - validatedSettings.GroupSearchBase != "" && validatedSettings.LDAPConnectionProtocol != "" { +func NewValidatedSettingsCache() ValidatedSettingsCacheI { + return &ValidatedSettingsCache{ValidatedSettingsByName: map[string]ValidatedSettings{}} +} + +func (s *ValidatedSettingsCache) Get(upstreamName, resourceVersion string, idpSpecGeneration int64) (ValidatedSettings, bool) { + validatedSettings, found := s.ValidatedSettingsByName[upstreamName] + if found && validatedSettings.BindSecretResourceVersion == resourceVersion && validatedSettings.IDPSpecGeneration == idpSpecGeneration { return validatedSettings, true } return ValidatedSettings{}, false } -func (s *SecretVersionCache) Set(upstreamName, resourceVersion string, generation int64, settings ValidatedSettings) { +func (s *ValidatedSettingsCache) Set(upstreamName string, settings ValidatedSettings) { s.ValidatedSettingsByName[upstreamName] = settings } -type ValidatedSettings struct { - Generation int64 - BindSecretResourceVersion string - LDAPConnectionProtocol upstreamldap.LDAPConnectionProtocol - UserSearchBase string - GroupSearchBase string -} - -func NewSecretVersionCache() SecretVersionCacheI { - cache := SecretVersionCache{ValidatedSettingsByName: map[string]ValidatedSettings{}} - return &cache -} - -// read only interface for sharing between ldap and active directory. +// UpstreamGenericLDAPIDP is a read-only interface for abstracting the differences between LDAP and Active Directory IDP types. type UpstreamGenericLDAPIDP interface { Spec() UpstreamGenericLDAPSpec Name() string @@ -247,8 +263,15 @@ func ValidateSecret(secretInformer corev1informers.SecretInformer, secretName st }, secret.ResourceVersion } +// gradatedCondition is a condition and a boolean that tells you whether the condition is fatal or just a warning. +type gradatedCondition struct { + condition *v1alpha1.Condition + isFatal bool +} + +// GradatedConditions is a list of conditions, where each condition can additionally be considered fatal or non-fatal. type GradatedConditions struct { - gradatedConditions []GradatedCondition + gradatedConditions []gradatedCondition } func (g *GradatedConditions) Conditions() []*v1alpha1.Condition { @@ -260,75 +283,81 @@ func (g *GradatedConditions) Conditions() []*v1alpha1.Condition { } func (g *GradatedConditions) Append(condition *v1alpha1.Condition, isFatal bool) { - g.gradatedConditions = append(g.gradatedConditions, GradatedCondition{condition: condition, isFatal: isFatal}) + g.gradatedConditions = append(g.gradatedConditions, gradatedCondition{condition: condition, isFatal: isFatal}) } -// A condition and a boolean that tells you whether it's fatal or just a warning. -type GradatedCondition struct { - condition *v1alpha1.Condition - isFatal bool -} - -func ValidateGenericLDAP(ctx context.Context, upstream UpstreamGenericLDAPIDP, secretInformer corev1informers.SecretInformer, validatedSecretVersionsCache SecretVersionCacheI, config *upstreamldap.ProviderConfig) GradatedConditions { +func ValidateGenericLDAP( + ctx context.Context, + upstream UpstreamGenericLDAPIDP, + secretInformer corev1informers.SecretInformer, + validatedSettingsCache ValidatedSettingsCacheI, + config *upstreamldap.ProviderConfig, +) GradatedConditions { conditions := GradatedConditions{} + secretValidCondition, currentSecretVersion := ValidateSecret(secretInformer, upstream.Spec().BindSecretName(), upstream.Namespace(), config) conditions.Append(secretValidCondition, true) + tlsValidCondition := ValidateTLSConfig(upstream.Spec().TLSSpec(), config) conditions.Append(tlsValidCondition, true) + var ldapConnectionValidCondition, searchBaseFoundCondition *v1alpha1.Condition // 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, searchBaseFoundCondition = validateAndSetLDAPServerConnectivityAndSearchBase(ctx, validatedSecretVersionsCache, upstream, config, currentSecretVersion) - if ldapConnectionValidCondition != nil { - conditions.Append(ldapConnectionValidCondition, false) - } - if searchBaseFoundCondition != nil { + ldapConnectionValidCondition, searchBaseFoundCondition = validateAndSetLDAPServerConnectivityAndSearchBase(ctx, validatedSettingsCache, upstream, config, currentSecretVersion) + conditions.Append(ldapConnectionValidCondition, false) + if searchBaseFoundCondition != nil { // currently, only used for AD, so may be nil conditions.Append(searchBaseFoundCondition, true) } } return conditions } -func validateAndSetLDAPServerConnectivityAndSearchBase(ctx context.Context, validatedSecretVersionsCache SecretVersionCacheI, upstream UpstreamGenericLDAPIDP, config *upstreamldap.ProviderConfig, currentSecretVersion string) (*v1alpha1.Condition, *v1alpha1.Condition) { - // previouslyValidatedSecretVersion := validatedSecretVersionsCache.ValidatedSettingsByName[upstream.Name()].BindSecretResourceVersion - // doesn't have an existing entry for ValidatedSettingsByName with this secret version -> - // lets double check tls connection - // if we can connect, put it in the secret cache - // also we KNOW we need to recheck the search base stuff too... so they should all be one function? - // but if tls validation fails no need to also try to get search base stuff? - - validatedSettings, hasPreviousValidatedSettings := validatedSecretVersionsCache.Get(upstream.Name(), currentSecretVersion, upstream.Generation()) +func validateAndSetLDAPServerConnectivityAndSearchBase( + ctx context.Context, + validatedSettingsCache ValidatedSettingsCacheI, + upstream UpstreamGenericLDAPIDP, + config *upstreamldap.ProviderConfig, + currentSecretVersion string, +) (*v1alpha1.Condition, *v1alpha1.Condition) { + validatedSettings, hasPreviousValidatedSettings := validatedSettingsCache.Get(upstream.Name(), currentSecretVersion, upstream.Generation()) var ldapConnectionValidCondition, searchBaseFoundCondition *v1alpha1.Condition - if !hasPreviousValidatedSettings { + + if hasPreviousValidatedSettings && validatedSettings.UserSearchBase != "" && validatedSettings.GroupSearchBase != "" { + // Found previously validated settings in the cache (which is also not missing search base fields), so use them. + config.ConnectionProtocol = validatedSettings.LDAPConnectionProtocol + config.UserSearch.Base = validatedSettings.UserSearchBase + config.GroupSearch.Base = validatedSettings.GroupSearchBase + ldapConnectionValidCondition = validatedSettings.ConnectionValidCondition.DeepCopy() + searchBaseFoundCondition = validatedSettings.SearchBaseFoundCondition.DeepCopy() + } else { + // Did not find previously validated settings in the cache, so probe the LDAP server. testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, probeLDAPTimeout) defer cancelFunc() - ldapConnectionValidCondition = TestConnection(testConnectionTimeout, upstream.Spec().BindSecretName(), config, currentSecretVersion) searchBaseTimeout, cancelFunc := context.WithTimeout(ctx, probeLDAPTimeout) defer cancelFunc() searchBaseFoundCondition = upstream.Spec().DetectAndSetSearchBase(searchBaseTimeout, config) - if ldapConnectionValidCondition.Status == v1alpha1.ConditionTrue { - // if it's nil, don't worry about the search base condition. But if it exists make sure the status is true. - if searchBaseFoundCondition == nil || (searchBaseFoundCondition.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. - validatedSettings.LDAPConnectionProtocol = config.ConnectionProtocol - validatedSettings.BindSecretResourceVersion = currentSecretVersion - validatedSettings.Generation = upstream.Generation() - validatedSettings.UserSearchBase = config.UserSearch.Base - validatedSettings.GroupSearchBase = config.GroupSearch.Base - validatedSecretVersionsCache.Set(upstream.Name(), currentSecretVersion, upstream.Generation(), validatedSettings) - } + // When there were no failures, write the newly validated settings to the cache. + // It's okay for the search base condition to be nil, since it's only used by Active Directory providers, + // but if it exists make sure it was not a failure. + if ldapConnectionValidCondition.Status == v1alpha1.ConditionTrue && + (searchBaseFoundCondition == nil || (searchBaseFoundCondition.Status == v1alpha1.ConditionTrue)) { + // Remember (in-memory for this pod) that the controller has successfully validated the LDAP or AD 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. + validatedSettingsCache.Set(upstream.Name(), ValidatedSettings{ + IDPSpecGeneration: upstream.Generation(), + BindSecretResourceVersion: currentSecretVersion, + LDAPConnectionProtocol: config.ConnectionProtocol, + UserSearchBase: config.UserSearch.Base, + GroupSearchBase: config.GroupSearch.Base, + ConnectionValidCondition: ldapConnectionValidCondition.DeepCopy(), + SearchBaseFoundCondition: searchBaseFoundCondition.DeepCopy(), // currently, only used for AD, so may be nil + }) } - } else { - config.ConnectionProtocol = validatedSettings.LDAPConnectionProtocol - config.UserSearch.Base = validatedSettings.UserSearchBase - config.GroupSearch.Base = validatedSettings.GroupSearchBase } return ldapConnectionValidCondition, searchBaseFoundCondition