diff --git a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go index ae214c23..1466ffd3 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go @@ -5,6 +5,8 @@ package upstreamwatcher import ( "context" + "crypto/x509" + "encoding/base64" "fmt" corev1 "k8s.io/api/core/v1" @@ -28,7 +30,10 @@ const ( ldapBindAccountSecretType = corev1.SecretTypeBasicAuth // Constants related to conditions. - typeBindSecretValid = "BindSecretValid" + typeBindSecretValid = "BindSecretValid" + tlsConfigurationValid = "TLSConfigurationValid" + noTLSConfigurationMessage = "no TLS configuration provided" + loadedTLSConfigurationMessage = "loaded TLS configuration" ) // UpstreamLDAPIdentityProviderICache is a thread safe cache that holds a list of validated upstream LDAP IDP configurations. @@ -101,10 +106,10 @@ func (c *ldapWatcherController) Sync(ctx controllerlib.Context) error { func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider) provider.UpstreamLDAPIdentityProviderI { spec := upstream.Spec + result := &upstreamldap.Provider{ - Name: upstream.Name, - Host: spec.Host, - CABundle: []byte(spec.TLS.CertificateAuthorityData), + Name: upstream.Name, + Host: spec.Host, UserSearch: &upstreamldap.UserSearch{ Base: spec.UserSearch.Base, Filter: spec.UserSearch.Filter, @@ -115,6 +120,7 @@ func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream * } conditions := []*v1alpha1.Condition{ c.validateSecret(upstream, result), + c.validateTLSConfig(upstream, result), } hadErrorCondition := c.updateStatus(ctx, upstream, conditions) if hadErrorCondition { @@ -123,7 +129,49 @@ func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream * return result } -func (c ldapWatcherController) validateSecret(upstream *v1alpha1.LDAPIdentityProvider, result *upstreamldap.Provider) *v1alpha1.Condition { +func (c *ldapWatcherController) validateTLSConfig(upstream *v1alpha1.LDAPIdentityProvider, result *upstreamldap.Provider) *v1alpha1.Condition { + tlsSpec := upstream.Spec.TLS + if tlsSpec == nil { + return c.validTLSCondition(noTLSConfigurationMessage) + } + if len(tlsSpec.CertificateAuthorityData) == 0 { + return c.validTLSCondition(loadedTLSConfigurationMessage) + } + + bundle, err := base64.StdEncoding.DecodeString(tlsSpec.CertificateAuthorityData) + if err != nil { + return c.invalidTLSCondition(fmt.Sprintf("certificateAuthorityData is invalid: %s", err.Error())) + } + + ca := x509.NewCertPool() + ok := ca.AppendCertsFromPEM(bundle) + if !ok { + return c.invalidTLSCondition(fmt.Sprintf("certificateAuthorityData is invalid: %s", errNoCertificates)) + } + + result.CABundle = bundle + return c.validTLSCondition(loadedTLSConfigurationMessage) +} + +func (c *ldapWatcherController) validTLSCondition(message string) *v1alpha1.Condition { + return &v1alpha1.Condition{ + Type: tlsConfigurationValid, + Status: v1alpha1.ConditionTrue, + Reason: reasonSuccess, + Message: message, + } +} + +func (c *ldapWatcherController) invalidTLSCondition(message string) *v1alpha1.Condition { + return &v1alpha1.Condition{ + Type: tlsConfigurationValid, + Status: v1alpha1.ConditionFalse, + Reason: reasonInvalidTLSConfig, + Message: message, + } +} + +func (c *ldapWatcherController) validateSecret(upstream *v1alpha1.LDAPIdentityProvider, result *upstreamldap.Provider) *v1alpha1.Condition { secretName := upstream.Spec.Bind.SecretName secret, err := c.secretInformer.Lister().Secrets(upstream.Namespace).Get(secretName) diff --git a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go index 6f836fe6..571d3bd9 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go @@ -5,6 +5,7 @@ package upstreamwatcher import ( "context" + "encoding/base64" "fmt" "sort" "testing" @@ -20,6 +21,7 @@ import ( "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" pinnipedfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" pinnipedinformers "go.pinniped.dev/generated/latest/client/supervisor/informers/externalversions" + "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/testutil" @@ -150,15 +152,18 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { testBindUsername = "test-bind-username" testBindPassword = "test-bind-password" testHost = "ldap.example.com:123" - testCABundle = "test-ca-bundle" testUserSearchBase = "test-user-search-base" testUserSearchFilter = "test-user-search-filter" testUsernameAttrName = "test-username-attr" testUIDAttrName = "test-uid-attr" ) - var ( - testValidSecretData = map[string][]byte{"username": []byte(testBindUsername), "password": []byte(testBindPassword)} - ) + + testValidSecretData := map[string][]byte{"username": []byte(testBindUsername), "password": []byte(testBindPassword)} + + testCA, err := certauthority.New("test CA", time.Minute) + require.NoError(t, err) + testCABundle := testCA.Bundle() + testCABundleBase64Encoded := base64.StdEncoding.EncodeToString(testCABundle) successfulDialer := &comparableDialer{ f: func(ctx context.Context, hostAndPort string) (upstreamldap.Conn, error) { @@ -171,7 +176,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: testName, Namespace: testNamespace, Generation: 1234}, Spec: v1alpha1.LDAPIdentityProviderSpec{ Host: testHost, - TLS: &v1alpha1.LDAPIdentityProviderTLSSpec{CertificateAuthorityData: testCABundle}, + TLS: &v1alpha1.LDAPIdentityProviderTLSSpec{CertificateAuthorityData: testCABundleBase64Encoded}, Bind: v1alpha1.LDAPIdentityProviderBindSpec{SecretName: testSecretName}, UserSearch: v1alpha1.LDAPIdentityProviderUserSearchSpec{ Base: testUserSearchBase, @@ -192,7 +197,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { providerForValidUpstream := &upstreamldap.Provider{ Name: testName, Host: testHost, - CABundle: []byte(testCABundle), + CABundle: testCABundle, BindUsername: testBindUsername, BindPassword: testBindPassword, UserSearch: &upstreamldap.UserSearch{ @@ -239,6 +244,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Message: "loaded bind secret", ObservedGeneration: 1234, }, + { + Type: "TLSConfigurationValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded TLS configuration", + ObservedGeneration: 1234, + }, }, }, }}, @@ -263,6 +276,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`secret "%s" not found`, testSecretName), ObservedGeneration: 1234, }, + { + Type: "TLSConfigurationValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded TLS configuration", + ObservedGeneration: 1234, + }, }, }, }}, @@ -291,6 +312,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`referenced Secret "%s" has wrong type "some-other-type" (should be "kubernetes.io/basic-auth")`, testSecretName), ObservedGeneration: 1234, }, + { + Type: "TLSConfigurationValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded TLS configuration", + ObservedGeneration: 1234, + }, }, }, }}, @@ -318,6 +347,194 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`referenced Secret "%s" is missing required keys ["username" "password"]`, testSecretName), ObservedGeneration: 1234, }, + { + Type: "TLSConfigurationValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded TLS configuration", + ObservedGeneration: 1234, + }, + }, + }, + }}, + }, + { + name: "CertificateAuthorityData is not base64 encoded", + ldapDialer: successfulDialer, + inputUpstreams: []runtime.Object{modifiedCopyOfValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { + upstream.Spec.TLS.CertificateAuthorityData = "this-is-not-base64-encoded" + })}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, + Type: corev1.SecretTypeBasicAuth, + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantResultingCache: []*upstreamldap.Provider{}, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + { + Type: "BindSecretValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded bind secret", + ObservedGeneration: 1234, + }, + { + Type: "TLSConfigurationValid", + Status: "False", + LastTransitionTime: now, + Reason: "InvalidTLSConfig", + Message: "certificateAuthorityData is invalid: illegal base64 data at input byte 4", + ObservedGeneration: 1234, + }, + }, + }, + }}, + }, + { + name: "CertificateAuthorityData is not valid pem data", + ldapDialer: successfulDialer, + inputUpstreams: []runtime.Object{modifiedCopyOfValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { + upstream.Spec.TLS.CertificateAuthorityData = base64.StdEncoding.EncodeToString([]byte("this is not pem data")) + })}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, + Type: corev1.SecretTypeBasicAuth, + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantResultingCache: []*upstreamldap.Provider{}, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + { + Type: "BindSecretValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded bind secret", + ObservedGeneration: 1234, + }, + { + Type: "TLSConfigurationValid", + Status: "False", + LastTransitionTime: now, + Reason: "InvalidTLSConfig", + Message: "certificateAuthorityData is invalid: no certificates found", + ObservedGeneration: 1234, + }, + }, + }, + }}, + }, + { + name: "nil TLS configuration", + ldapDialer: successfulDialer, + inputUpstreams: []runtime.Object{modifiedCopyOfValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { + upstream.Spec.TLS = nil + })}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, + Type: corev1.SecretTypeBasicAuth, + Data: testValidSecretData, + }}, + wantResultingCache: []*upstreamldap.Provider{ + { + Name: testName, + Host: testHost, + CABundle: nil, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: &upstreamldap.UserSearch{ + Base: testUserSearchBase, + Filter: testUserSearchFilter, + UsernameAttribute: testUsernameAttrName, + UIDAttribute: testUIDAttrName, + }, + Dialer: successfulDialer, // the dialer passed to the controller's constructor should have been passed through + }, + }, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + { + Type: "BindSecretValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded bind secret", + ObservedGeneration: 1234, + }, + { + Type: "TLSConfigurationValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "no TLS configuration provided", + ObservedGeneration: 1234, + }, + }, + }, + }}, + }, + { + name: "non-nil TLS configuration with empty CertificateAuthorityData", + ldapDialer: successfulDialer, + inputUpstreams: []runtime.Object{modifiedCopyOfValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { + upstream.Spec.TLS.CertificateAuthorityData = "" + })}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, + Type: corev1.SecretTypeBasicAuth, + Data: testValidSecretData, + }}, + wantResultingCache: []*upstreamldap.Provider{ + { + Name: testName, + Host: testHost, + CABundle: nil, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: &upstreamldap.UserSearch{ + Base: testUserSearchBase, + Filter: testUserSearchFilter, + UsernameAttribute: testUsernameAttrName, + UIDAttribute: testUIDAttrName, + }, + Dialer: successfulDialer, // the dialer passed to the controller's constructor should have been passed through + }, + }, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + { + Type: "BindSecretValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded bind secret", + ObservedGeneration: 1234, + }, + { + Type: "TLSConfigurationValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded TLS configuration", + ObservedGeneration: 1234, + }, }, }, }}, @@ -351,6 +568,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`secret "%s" not found`, "non-existent-secret"), ObservedGeneration: 42, }, + { + Type: "TLSConfigurationValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded TLS configuration", + ObservedGeneration: 42, + }, }, }, }, @@ -367,6 +592,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Message: "loaded bind secret", ObservedGeneration: 1234, }, + { + Type: "TLSConfigurationValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded TLS configuration", + ObservedGeneration: 1234, + }, }, }, }, diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index cf3b5ffa..765b5fa9 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -60,7 +60,7 @@ type Provider struct { // the default LDAP port will be used. Host string - // PEM-encoded CA cert bundle to trust when connecting to the LDAP server. + // PEM-encoded CA cert bundle to trust when connecting to the LDAP server. Can be nil. CABundle []byte // BindUsername is the username to use when performing a bind with the upstream LDAP IDP. diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index c58505dd..2615f084 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -45,7 +45,9 @@ var ( func TestAuthenticateUser(t *testing.T) { provider := func(editFunc func(p *Provider)) *Provider { provider := &Provider{ + Name: "some-provider-name", Host: testHost, + CABundle: nil, // this field is only used by the production dialer, which is replaced by a mock for this test BindUsername: testBindUsername, BindPassword: testBindPassword, UserSearch: &UserSearch{