diff --git a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go index 569320059..207d7a79d 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go @@ -31,7 +31,7 @@ type UpstreamLDAPIdentityProviderICache interface { type ldapWatcherController struct { cache UpstreamLDAPIdentityProviderICache - ldapDialFunc upstreamldap.LDAPDialerFunc + ldapDialer upstreamldap.LDAPDialer client pinnipedclientset.Interface ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer secretInformer corev1informers.SecretInformer @@ -40,7 +40,7 @@ type ldapWatcherController struct { // NewLDAPUpstreamWatcherController instantiates a new controllerlib.Controller which will populate the provided UpstreamLDAPIdentityProviderICache. func NewLDAPUpstreamWatcherController( idpCache UpstreamLDAPIdentityProviderICache, - ldapDialFunc upstreamldap.LDAPDialerFunc, + ldapDialer upstreamldap.LDAPDialer, client pinnipedclientset.Interface, ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer, secretInformer corev1informers.SecretInformer, @@ -48,7 +48,7 @@ func NewLDAPUpstreamWatcherController( ) controllerlib.Controller { c := ldapWatcherController{ cache: idpCache, - ldapDialFunc: ldapDialFunc, + ldapDialer: ldapDialer, client: client, ldapIdentityProviderInformer: ldapIdentityProviderInformer, secretInformer: secretInformer, @@ -93,5 +93,44 @@ func (c *ldapWatcherController) Sync(ctx controllerlib.Context) error { } func (c *ldapWatcherController) validateUpstream(upstream *v1alpha1.LDAPIdentityProvider) provider.UpstreamLDAPIdentityProviderI { - return &upstreamldap.Provider{Name: upstream.Name, Dial: c.ldapDialFunc} + spec := upstream.Spec + result := &upstreamldap.Provider{ + Name: upstream.Name, + Host: spec.Host, + CABundle: []byte(spec.TLS.CertificateAuthorityData), + UserSearch: &upstreamldap.UserSearch{ + Base: spec.UserSearch.Base, + Filter: spec.UserSearch.Filter, + UsernameAttribute: spec.UserSearch.Attributes.Username, + UIDAttribute: spec.UserSearch.Attributes.UniqueID, + }, + Dialer: c.ldapDialer, + } + _ = c.validateSecret(upstream, result) + return result +} + +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) + if err != nil { + // TODO + return nil + } + + if secret.Type != corev1.SecretTypeBasicAuth { + // TODO + return nil + } + + result.BindUsername = string(secret.Data[corev1.BasicAuthUsernameKey]) + result.BindPassword = string(secret.Data[corev1.BasicAuthPasswordKey]) + if len(result.BindUsername) == 0 || len(result.BindPassword) == 0 { + // TODO + return nil + } + + var cond *v1alpha1.Condition // satisfy linter + return cond } diff --git a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go index e1e688fb4..e7f9f5037 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go @@ -127,42 +127,69 @@ func TestLDAPUpstreamWatcherControllerFilterLDAPIdentityProviders(t *testing.T) } } +// Wrap the func into a struct so the test can do deep equal assertions on instances of upstreamldap.Provider. +type comparableDialer struct { + f upstreamldap.LDAPDialerFunc +} + +func (d *comparableDialer) Dial(ctx context.Context, hostAndPort string) (upstreamldap.Conn, error) { + return d.f(ctx, hostAndPort) +} + func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { t.Parallel() + const ( + testNamespace = "test-namespace" + testName = "test-name" + testSecretName = "test-client-secret" + 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 ( - testNamespace = "test-namespace" - testName = "test-name" - testSecretName = "test-client-secret" - testBindUsername = "test-bind-username" - testBindPassword = "test-bind-password" testValidSecretData = map[string][]byte{"username": []byte(testBindUsername), "password": []byte(testBindPassword)} ) + + successfulDialer := &comparableDialer{ + f: func(ctx context.Context, hostAndPort string) (upstreamldap.Conn, error) { + // TODO return a fake implementation of upstreamldap.Conn, or return an error for testing errors + return nil, nil + }, + } + tests := []struct { name string inputUpstreams []runtime.Object inputSecrets []runtime.Object + ldapDialer upstreamldap.LDAPDialer wantErr string - wantResultingCache []provider.UpstreamLDAPIdentityProviderI + wantResultingCache []*upstreamldap.Provider wantResultingUpstreams []v1alpha1.LDAPIdentityProvider }{ { name: "no LDAPIdentityProvider upstreams clears the cache", }, { - name: "one valid upstream updates the cache to include only that upstream", + name: "one valid upstream updates the cache to include only that upstream", + ldapDialer: successfulDialer, inputUpstreams: []runtime.Object{&v1alpha1.LDAPIdentityProvider{ ObjectMeta: metav1.ObjectMeta{Name: testName, Namespace: testNamespace, Generation: 1234}, Spec: v1alpha1.LDAPIdentityProviderSpec{ - Host: "TODO", // TODO - TLS: &v1alpha1.LDAPIdentityProviderTLSSpec{CertificateAuthorityData: "TODO"}, // TODO + Host: testHost, + TLS: &v1alpha1.LDAPIdentityProviderTLSSpec{CertificateAuthorityData: testCABundle}, Bind: v1alpha1.LDAPIdentityProviderBindSpec{SecretName: testSecretName}, UserSearch: v1alpha1.LDAPIdentityProviderUserSearchSpec{ - Base: "TODO", // TODO - Filter: "TODO", // TODO + Base: testUserSearchBase, + Filter: testUserSearchFilter, Attributes: v1alpha1.LDAPIdentityProviderUserSearchAttributesSpec{ - Username: "TODO", // TODO - UniqueID: "TODO", // TODO + Username: testUsernameAttrName, + UniqueID: testUIDAttrName, }, }, }, @@ -172,10 +199,20 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Type: corev1.SecretTypeBasicAuth, Data: testValidSecretData, }}, - wantResultingCache: []provider.UpstreamLDAPIdentityProviderI{ - &upstreamldap.Provider{ - Name: testName, - // TODO test more stuff + wantResultingCache: []*upstreamldap.Provider{ + { + Name: testName, + Host: testHost, + CABundle: []byte(testCABundle), + 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{{ @@ -202,10 +239,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { controller := NewLDAPUpstreamWatcherController( cache, - func(ctx context.Context, hostAndPort string) (upstreamldap.Conn, error) { - // TODO return a fake implementation of upstreamldap.Conn, or return an error for testing errors - return nil, nil - }, + successfulDialer, fakePinnipedClient, pinnipedInformers.IDP().V1alpha1().LDAPIdentityProviders(), kubeInformers.Core().V1().Secrets(), @@ -231,8 +265,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { require.Equal(t, len(tt.wantResultingCache), len(actualIDPList)) for i := range actualIDPList { actualIDP := actualIDPList[i].(*upstreamldap.Provider) - require.Equal(t, tt.wantResultingCache[i].GetName(), actualIDP.GetName()) - // TODO more assertions + require.Equal(t, tt.wantResultingCache[i], actualIDP) } actualUpstreams, err := fakePinnipedClient.IDPV1alpha1().LDAPIdentityProviders(testNamespace).List(ctx, metav1.ListOptions{}) diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index b8ed92e24..8e62f7246 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -12,7 +12,7 @@ import ( "net" "strings" - ldap "github.com/go-ldap/ldap/v3" + "github.com/go-ldap/ldap/v3" "k8s.io/apiserver/pkg/authentication/authenticator" ) @@ -32,9 +32,18 @@ type Conn interface { // Our Conn type is subset of the ldap.Client interface, which is implemented by ldap.Conn. var _ Conn = &ldap.Conn{} -// LDAPDialerFunc is a factory of Conn, and the resulting Conn can then be used to interact with an upstream LDAP IDP. +// LDAPDialer is a factory of Conn, and the resulting Conn can then be used to interact with an upstream LDAP IDP. +type LDAPDialer interface { + Dial(ctx context.Context, hostAndPort string) (Conn, error) +} + +// LDAPDialerFunc makes it easy to use a func as an LDAPDialer. type LDAPDialerFunc func(ctx context.Context, hostAndPort string) (Conn, error) +func (f LDAPDialerFunc) Dial(ctx context.Context, hostAndPort string) (Conn, error) { + return f(ctx, hostAndPort) +} + // Provider includes all of the settings for connection and searching for users and groups in // the upstream LDAP IDP. It also provides methods for testing the connection and performing logins. type Provider struct { @@ -57,8 +66,8 @@ type Provider struct { // UserSearch contains information about how to search for users in the upstream LDAP IDP. UserSearch *UserSearch - // Dial exists to enable testing. When nil, will use a default appropriate for production use. - Dial LDAPDialerFunc + // Dialer exists to enable testing. When nil, will use a default appropriate for production use. + Dialer LDAPDialer } // UserSearch contains information about how to search for users in the upstream LDAP IDP. @@ -83,13 +92,13 @@ func (p *Provider) dial(ctx context.Context) (Conn, error) { if err != nil { return nil, ldap.NewError(ldap.ErrorNetwork, err) } - if p.Dial != nil { - return p.Dial(ctx, hostAndPort) + if p.Dialer != nil { + return p.Dialer.Dial(ctx, hostAndPort) } return p.dialTLS(ctx, hostAndPort) } -// dialTLS is the default implementation of the Dial func, used when Dial is nil. +// dialTLS is the default implementation of the Dialer, used when Dialer is nil. // Unfortunately, the go-ldap library does not seem to support dialing with a context.Context, // so we implement it ourselves, heavily inspired by ldap.DialURL. func (p *Provider) dialTLS(ctx context.Context, hostAndPort string) (Conn, error) { diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index f031838be..358582f7a 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -94,11 +94,11 @@ func TestAuthenticateUser(t *testing.T) { conn.EXPECT().Close().Times(1) dialWasAttempted := false - test.provider.Dial = func(ctx context.Context, hostAndPort string) (Conn, error) { + test.provider.Dialer = LDAPDialerFunc(func(ctx context.Context, hostAndPort string) (Conn, error) { dialWasAttempted = true require.Equal(t, test.provider.Host, hostAndPort) return conn, nil - } + }) authResponse, authenticated, err := test.provider.AuthenticateUser(context.Background(), upstreamUsername, upstreamPassword) require.True(t, dialWasAttempted, "AuthenticateUser was supposed to try to dial, but didn't") @@ -181,7 +181,7 @@ func TestRealTLSDialing(t *testing.T) { provider := &Provider{ Host: test.host, CABundle: test.caBundle, - Dial: nil, // this test is for the default (production) dialer + Dialer: nil, // this test is for the default (production) dialer } conn, err := provider.dial(test.context) if conn != nil { @@ -198,7 +198,7 @@ func TestRealTLSDialing(t *testing.T) { // Can't test its methods here because we are not dialed to a real LDAP server. require.IsType(t, &ldap.Conn{}, conn) - // Indirectly checking that the Dial method constructed the ldap.Conn with isTLS set to true, + // Indirectly checking that the Dialer method constructed the ldap.Conn with isTLS set to true, // since this is always the correct behavior unless/until we want to support StartTLS. err := conn.(*ldap.Conn).StartTLS(&tls.Config{}) require.EqualError(t, err, `LDAP Result Code 200 "Network Error": ldap: already encrypted`)