From a6f95cfff15ab85a47ea995de263f27ec6347185 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 18 May 2021 16:38:12 -0700 Subject: [PATCH 1/5] Configure openldap to disallow non-TLS clients - For testing purposes, we would like to ensure that when we connect to the LDAP server we cannot accidentally avoid using TLS or StartTLS. - Also enabled the openldap `memberOf` overlay in case we want to support group search using `memberOf` in the future. - This required changes to the docker.io/bitnami/openldap container image, so we're using our own fork for now. Will submit a PR to bitnami/openldap to see if they will accept it (or something similar) upstream. --- test/deploy/tools/cert-issuer.yaml | 4 +- test/deploy/tools/ldap.yaml | 122 ++++++++++++++++++++++++++++- 2 files changed, 122 insertions(+), 4 deletions(-) diff --git a/test/deploy/tools/cert-issuer.yaml b/test/deploy/tools/cert-issuer.yaml index e044b1e7b..4e4a080f8 100644 --- a/test/deploy/tools/cert-issuer.yaml +++ b/test/deploy/tools/cert-issuer.yaml @@ -77,13 +77,15 @@ spec: | cfssljson -bare dex # Cheat and add 127.0.0.1 as an IP SAN so we can use the ldaps port through port forwarding. + # Also allow the server to be accessed by multiple Service names to different Services + # can provide/hide different ports. echo "generating LDAP server certificate..." cfssl gencert \ -ca ca.pem -ca-key ca-key.pem \ -config /tmp/cfssl-default.json \ -profile www \ -cn "ldap.tools.svc.cluster.local" \ - -hostname "ldap.tools.svc.cluster.local,127.0.0.1" \ + -hostname "ldap.tools.svc.cluster.local,ldaps.tools.svc.cluster.local,ldapstarttls.tools.svc.cluster.local,127.0.0.1" \ /tmp/csr.json \ | cfssljson -bare ldap diff --git a/test/deploy/tools/ldap.yaml b/test/deploy/tools/ldap.yaml index 3728ab49f..9405df9f3 100644 --- a/test/deploy/tools/ldap.yaml +++ b/test/deploy/tools/ldap.yaml @@ -127,6 +127,63 @@ metadata: type: Opaque stringData: #@ ldapLIDIF() --- +apiVersion: v1 +kind: Secret +metadata: + name: ldap-server-config-before-ldif-files + namespace: tools +type: Opaque +stringData: + server-config.ldif: | + # Load the memberof module. + dn: cn=module,cn=config + cn: module + objectClass: olcModuleList + objectClass: top + olcModulePath: /opt/bitnami/openldap/lib/openldap + olcModuleLoad: memberof + + dn: olcOverlay={0}memberof,olcDatabase={2}hdb,cn=config + objectClass: olcConfig + objectClass: olcMemberOf + objectClass: olcOverlayConfig + objectClass: top + olcOverlay: memberof + olcMemberOfDangling: ignore + olcMemberOfRefInt: TRUE + olcMemberOfGroupOC: groupOfNames + olcMemberOfMemberAD: member + + # Load the refint module. + dn: cn=module,cn=config + cn: module + objectclass: olcModuleList + objectclass: top + olcmodulepath: /opt/bitnami/openldap/lib/openldap + olcmoduleload: refint + + dn: olcOverlay={1}refint,olcDatabase={2}hdb,cn=config + objectClass: olcConfig + objectClass: olcOverlayConfig + objectClass: olcRefintConfig + objectClass: top + olcOverlay: {1}refint + olcRefintAttribute: memberof member manager owner +--- +apiVersion: v1 +kind: Secret +metadata: + name: ldap-server-config-after-ldif-files + namespace: tools +type: Opaque +stringData: + server-config.ldif: | + # Reject any further connections that do not use TLS or StartTLS + dn: olcDatabase={2}hdb,cn=config + changetype: modify + add: olcSecurity + olcSecurity: tls=1 +--- apiVersion: apps/v1 kind: Deployment metadata: @@ -149,7 +206,10 @@ spec: spec: containers: - name: ldap - image: docker.io/bitnami/openldap + #! Use our own fork of docker.io/bitnami/openldap for now, because we added the + #! LDAP_SERVER_CONFIG_BEFORE_CUSTOM_LDIF_DIR and LDAP_SERVER_CONFIG_AFTER_CUSTOM_LDIF_DIR options. + #! See https://github.com/pinniped-ci-bot/bitnami-docker-openldap/tree/ldap_server_config + image: ghcr.io/pinniped-ci-bot/bitnami-openldap-forked:2.4.58-debian-10-r57 imagePullPolicy: Always ports: - name: ldap @@ -173,8 +233,14 @@ spec: env: #! Example ldapsearch commands that can be run from within the container based on these env vars. #! These will print the whole LDAP tree starting at our root. - #! ldapsearch -x -H 'ldap://ldap.tools.svc.cluster.local' -D 'cn=admin,dc=pinniped,dc=dev' -w password -b 'dc=pinniped,dc=dev' - #! LDAPTLS_CACERT=/var/certs/ca.pem ldapsearch -x -H 'ldaps://ldap.tools.svc.cluster.local' -D 'cn=admin,dc=pinniped,dc=dev' -w password -b 'dc=pinniped,dc=dev' + #! Using StartTLS (-ZZ) on the ldap port... + #! LDAPTLS_CACERT=/var/certs/ca.pem ldapsearch -x -ZZ -H 'ldap://ldap.tools.svc.cluster.local' -D 'cn=admin,dc=pinniped,dc=dev' -w password -b 'dc=pinniped,dc=dev' + #! Using ldaps... + #! LDAPTLS_CACERT=/var/certs/ca.pem ldapsearch -x -H 'ldaps://ldap.tools.svc.cluster.local' -D 'cn=admin,dc=pinniped,dc=dev' -w password -b 'dc=pinniped,dc=dev' + #! Note that the memberOf attribute is special and not returned by default. It must be specified as one of attributes to return in the search, e.g.: + #! LDAPTLS_CACERT=/var/certs/ca.pem ldapsearch -x -H 'ldaps://ldap.tools.svc.cluster.local' -D 'cn=admin,dc=pinniped,dc=dev' -w password -b 'dc=pinniped,dc=dev' cn uidNumber mail member memberOf + #! This should fail and report "TLS confidentiality required" because we require TLS and this does not use TLS or StartTLS... + #! ldapsearch -x -H 'ldap://ldap.tools.svc.cluster.local' -D 'cn=admin,dc=pinniped,dc=dev' -w password -b 'dc=pinniped,dc=dev' - name: BITNAMI_DEBUG value: "true" - name: LDAP_ADMIN_USERNAME @@ -192,6 +258,10 @@ spec: #! Note that the custom LDIF file is only read at pod start-up time. - name: LDAP_CUSTOM_LDIF_DIR value: "/var/ldifs" + - name: LDAP_SERVER_CONFIG_BEFORE_CUSTOM_LDIF_DIR + value: "/var/server-config-before-ldifs" + - name: LDAP_SERVER_CONFIG_AFTER_CUSTOM_LDIF_DIR + value: "/var/server-config-after-ldifs" #! Seems like LDAP_ROOT is still required when using LDAP_CUSTOM_LDIF_DIR because it effects the admin user. #! Presumably this needs to match the root that we create in the LDIF file. - name: LDAP_ROOT @@ -203,6 +273,12 @@ spec: - name: ldifs mountPath: /var/ldifs readOnly: true + - name: server-config-before-ldifs + mountPath: /var/server-config-before-ldifs + readOnly: true + - name: server-config-after-ldifs + mountPath: /var/server-config-after-ldifs + readOnly: true volumes: - name: certs secret: @@ -210,6 +286,12 @@ spec: - name: ldifs secret: secretName: ldap-ldif-files + - name: server-config-before-ldifs + secret: + secretName: ldap-server-config-before-ldif-files + - name: server-config-after-ldifs + secret: + secretName: ldap-server-config-after-ldif-files --- apiVersion: v1 kind: Service @@ -231,3 +313,37 @@ spec: port: 636 targetPort: 1636 name: ldaps +--- +apiVersion: v1 +kind: Service +metadata: + name: ldaps + namespace: tools + labels: + app: ldap +spec: + type: ClusterIP + selector: + app: ldap + ports: + - protocol: TCP + port: 636 + targetPort: 1636 + name: ldaps +--- +apiVersion: v1 +kind: Service +metadata: + name: ldapstarttls + namespace: tools + labels: + app: ldap +spec: + type: ClusterIP + selector: + app: ldap + ports: + - protocol: TCP + port: 389 + targetPort: 1389 + name: ldap From 025b37f8396710fc9d555d76c277c503d4a9c28d Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 19 May 2021 17:17:44 -0700 Subject: [PATCH 2/5] upstreamldap.New() now supports a StartTLS config option - This enhances our LDAP client code to make it possible to optionally dial an LDAP server without TLS and then use StartTLS to upgrade the connection to TLS. - The controller for LDAPIdentityProviders is not using this option yet. That will come in a future commit. --- .../ldap_upstream_watcher.go | 5 +- .../ldap_upstream_watcher_test.go | 33 +++-- internal/upstreamldap/upstreamldap.go | 111 ++++++++++++--- internal/upstreamldap/upstreamldap_test.go | 131 +++++++++++++++--- test/integration/ldap_client_test.go | 125 ++++++++++++++--- 5 files changed, 332 insertions(+), 73 deletions(-) diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go index 81837769f..47cb404d6 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go @@ -152,8 +152,9 @@ func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream * spec := upstream.Spec config := &upstreamldap.ProviderConfig{ - Name: upstream.Name, - Host: spec.Host, + Name: upstream.Name, + Host: spec.Host, + ConnectionProtocol: upstreamldap.TLS, UserSearch: upstreamldap.UserSearchConfig{ Base: spec.UserSearch.Base, Filter: spec.UserSearch.Filter, diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go index 99f09f1e8..ae9346f56 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go @@ -197,11 +197,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { } providerConfigForValidUpstream := &upstreamldap.ProviderConfig{ - Name: testName, - Host: testHost, - CABundle: testCABundle, - BindUsername: testBindUsername, - BindPassword: testBindPassword, + Name: testName, + Host: testHost, + ConnectionProtocol: upstreamldap.TLS, + CABundle: testCABundle, + BindUsername: testBindUsername, + BindPassword: testBindPassword, UserSearch: upstreamldap.UserSearchConfig{ Base: testUserSearchBase, Filter: testUserSearchFilter, @@ -442,11 +443,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, wantResultingCache: []*upstreamldap.ProviderConfig{ { - Name: testName, - Host: testHost, - CABundle: nil, - BindUsername: testBindUsername, - BindPassword: testBindPassword, + Name: testName, + Host: testHost, + ConnectionProtocol: upstreamldap.TLS, + CABundle: nil, + BindUsername: testBindUsername, + BindPassword: testBindPassword, UserSearch: upstreamldap.UserSearchConfig{ Base: testUserSearchBase, Filter: testUserSearchFilter, @@ -493,11 +495,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, wantResultingCache: []*upstreamldap.ProviderConfig{ { - Name: testName, - Host: testHost, - CABundle: nil, - BindUsername: testBindUsername, - BindPassword: testBindPassword, + Name: testName, + Host: testHost, + ConnectionProtocol: upstreamldap.TLS, + CABundle: nil, + BindUsername: testBindUsername, + BindPassword: testBindPassword, UserSearch: upstreamldap.UserSearchConfig{ Base: testUserSearchBase, Filter: testUserSearchFilter, diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 2ffec1ed6..39d51e8cf 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -60,6 +60,13 @@ func (f LDAPDialerFunc) Dial(ctx context.Context, hostAndPort string) (Conn, err return f(ctx, hostAndPort) } +type LDAPConnectionProtocol string + +const ( + StartTLS = LDAPConnectionProtocol("StartTLS") + TLS = LDAPConnectionProtocol("TLS") +) + // ProviderConfig 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. // The nested structs are not pointer fields to enable deep copy on function params and return values. @@ -71,6 +78,9 @@ type ProviderConfig struct { // the default LDAP port will be used. Host string + // ConnectionProtocol determines how to establish the connection to the server. Either StartTLS or TLS. + ConnectionProtocol LDAPConnectionProtocol + // PEM-encoded CA cert bundle to trust when connecting to the LDAP server. Can be nil. CABundle []byte @@ -137,33 +147,38 @@ func (p *Provider) GetConfig() ProviderConfig { } func (p *Provider) dial(ctx context.Context) (Conn, error) { - hostAndPort, err := hostAndPortWithDefaultPort(p.c.Host, ldap.DefaultLdapsPort) + tlsHostAndPort, err := hostAndPortWithDefaultPort(p.c.Host, ldap.DefaultLdapsPort) if err != nil { return nil, ldap.NewError(ldap.ErrorNetwork, err) } - if p.c.Dialer != nil { - return p.c.Dialer.Dial(ctx, hostAndPort) + + startTLSHostAndPort, err := hostAndPortWithDefaultPort(p.c.Host, ldap.DefaultLdapPort) + if err != nil { + return nil, ldap.NewError(ldap.ErrorNetwork, err) + } + + switch { + case p.c.Dialer != nil: + return p.c.Dialer.Dial(ctx, tlsHostAndPort) + case p.c.ConnectionProtocol == TLS: + return p.dialTLS(ctx, tlsHostAndPort) + case p.c.ConnectionProtocol == StartTLS: + return p.dialStartTLS(ctx, startTLSHostAndPort) + default: + return nil, ldap.NewError(ldap.ErrorNetwork, fmt.Errorf("did not specify valid ConnectionProtocol")) } - return p.dialTLS(ctx, hostAndPort) } -// dialTLS is the default implementation of the Dialer, used when Dialer is nil. +// dialTLS is a default implementation of the Dialer, used when Dialer is nil and ConnectionProtocol is TLS. // 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) { - var rootCAs *x509.CertPool - if p.c.CABundle != nil { - rootCAs = x509.NewCertPool() - if !rootCAs.AppendCertsFromPEM(p.c.CABundle) { - return nil, ldap.NewError(ldap.ErrorNetwork, fmt.Errorf("could not parse CA bundle")) - } + tlsConfig, err := p.tlsConfig() + if err != nil { + return nil, ldap.NewError(ldap.ErrorNetwork, err) } - dialer := &tls.Dialer{Config: &tls.Config{ - MinVersion: tls.VersionTLS12, - RootCAs: rootCAs, - }} - + dialer := &tls.Dialer{NetDialer: netDialer(), Config: tlsConfig} c, err := dialer.DialContext(ctx, "tcp", hostAndPort) if err != nil { return nil, ldap.NewError(ldap.ErrorNetwork, err) @@ -174,6 +189,52 @@ func (p *Provider) dialTLS(ctx context.Context, hostAndPort string) (Conn, error return conn, nil } +// dialTLS is a default implementation of the Dialer, used when Dialer is nil and ConnectionProtocol is StartTLS. +// 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) dialStartTLS(ctx context.Context, hostAndPort string) (Conn, error) { + tlsConfig, err := p.tlsConfig() + if err != nil { + return nil, ldap.NewError(ldap.ErrorNetwork, err) + } + + host, err := hostWithoutPort(hostAndPort) + if err != nil { + return nil, ldap.NewError(ldap.ErrorNetwork, err) + } + // Unfortunately, this seems to be required for StartTLS, even though it is not needed for regular TLS. + tlsConfig.ServerName = host + + c, err := netDialer().DialContext(ctx, "tcp", hostAndPort) + if err != nil { + return nil, ldap.NewError(ldap.ErrorNetwork, err) + } + + conn := ldap.NewConn(c, false) + conn.Start() + err = conn.StartTLS(tlsConfig) + if err != nil { + return nil, err + } + + return conn, nil +} + +func netDialer() *net.Dialer { + return &net.Dialer{Timeout: time.Minute} +} + +func (p *Provider) tlsConfig() (*tls.Config, error) { + var rootCAs *x509.CertPool + if p.c.CABundle != nil { + rootCAs = x509.NewCertPool() + if !rootCAs.AppendCertsFromPEM(p.c.CABundle) { + return nil, fmt.Errorf("could not parse CA bundle") + } + } + return &tls.Config{MinVersion: tls.VersionTLS12, RootCAs: rootCAs}, nil +} + // Adds the default port if hostAndPort did not already include a port. func hostAndPortWithDefaultPort(hostAndPort string, defaultPort string) (string, error) { host, port, err := net.SplitHostPort(hostAndPort) @@ -188,7 +249,7 @@ func hostAndPortWithDefaultPort(hostAndPort string, defaultPort string) (string, switch { case port != "" && strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]"): // don't add extra square brackets to an IPv6 address that already has them - return host + ":" + port, nil + return fmt.Sprintf("%s:%s", host, port), nil case port != "": return net.JoinHostPort(host, port), nil default: @@ -196,6 +257,22 @@ func hostAndPortWithDefaultPort(hostAndPort string, defaultPort string) (string, } } +// Strip the port from a host or host:port. +func hostWithoutPort(hostAndPort string) (string, error) { + host, _, err := net.SplitHostPort(hostAndPort) + if err != nil { + if strings.HasSuffix(err.Error(), ": missing port in address") { // sad to need to do this string compare + return hostAndPort, nil + } + return "", err // hostAndPort argument was not parsable + } + if strings.HasPrefix(hostAndPort, "[") { + // it was an IPv6 address, so preserve the square brackets. + return fmt.Sprintf("[%s]", host), nil + } + return host, nil +} + // A name for this upstream provider. func (p *Provider) GetName() string { return p.c.Name diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 365618f4f..56203fbbb 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -1132,27 +1132,55 @@ func TestRealTLSDialing(t *testing.T) { tests := []struct { name string host string + connProto LDAPConnectionProtocol caBundle []byte context context.Context wantError string }{ { - name: "happy path", - host: testServerHostAndPort, - caBundle: []byte(testServerCABundle), - context: context.Background(), + name: "happy path", + host: testServerHostAndPort, + caBundle: []byte(testServerCABundle), + connProto: TLS, + context: context.Background(), }, { - name: "invalid CA bundle", + name: "invalid CA bundle with TLS", host: testServerHostAndPort, caBundle: []byte("not a ca bundle"), + connProto: TLS, context: context.Background(), wantError: `LDAP Result Code 200 "Network Error": could not parse CA bundle`, }, + { + name: "invalid CA bundle with StartTLS", + host: testServerHostAndPort, + caBundle: []byte("not a ca bundle"), + connProto: StartTLS, + context: context.Background(), + wantError: `LDAP Result Code 200 "Network Error": could not parse CA bundle`, + }, + { + name: "invalid host with TLS", + host: "this:is:not:a:valid:hostname", + caBundle: []byte(testServerCABundle), + connProto: TLS, + context: context.Background(), + wantError: `LDAP Result Code 200 "Network Error": address this:is:not:a:valid:hostname: too many colons in address`, + }, + { + name: "invalid host with StartTLS", + host: "this:is:not:a:valid:hostname", + caBundle: []byte(testServerCABundle), + connProto: StartTLS, + context: context.Background(), + wantError: `LDAP Result Code 200 "Network Error": address this:is:not:a:valid:hostname: too many colons in address`, + }, { name: "missing CA bundle when it is required because the host is not using a trusted CA", host: testServerHostAndPort, caBundle: nil, + connProto: TLS, context: context.Background(), wantError: `LDAP Result Code 200 "Network Error": x509: certificate signed by unknown authority`, }, @@ -1161,6 +1189,7 @@ func TestRealTLSDialing(t *testing.T) { // This is assuming that this port was not reclaimed by another app since the test setup ran. Seems safe enough. host: recentlyClaimedHostAndPort, caBundle: []byte(testServerCABundle), + connProto: TLS, context: context.Background(), wantError: fmt.Sprintf(`LDAP Result Code 200 "Network Error": dial tcp %s: connect: connection refused`, recentlyClaimedHostAndPort), }, @@ -1168,25 +1197,35 @@ func TestRealTLSDialing(t *testing.T) { name: "pays attention to the passed context", host: testServerHostAndPort, caBundle: []byte(testServerCABundle), + connProto: TLS, context: alreadyCancelledContext, wantError: fmt.Sprintf(`LDAP Result Code 200 "Network Error": dial tcp %s: operation was canceled`, testServerHostAndPort), }, + { + name: "unsupported connection protocol", + host: testServerHostAndPort, + caBundle: []byte(testServerCABundle), + connProto: "bad usage of this type", + context: alreadyCancelledContext, + wantError: `LDAP Result Code 200 "Network Error": did not specify valid ConnectionProtocol`, + }, } for _, test := range tests { - test := test - t.Run(test.name, func(t *testing.T) { + tt := test + t.Run(tt.name, func(t *testing.T) { provider := New(ProviderConfig{ - Host: test.host, - CABundle: test.caBundle, - Dialer: nil, // this test is for the default (production) dialer + Host: tt.host, + CABundle: tt.caBundle, + ConnectionProtocol: tt.connProto, + Dialer: nil, // this test is for the default (production) TLS dialer }) - conn, err := provider.dial(test.context) + conn, err := provider.dial(tt.context) if conn != nil { defer conn.Close() } - if test.wantError != "" { + if tt.wantError != "" { require.Nil(t, conn) - require.EqualError(t, err, test.wantError) + require.EqualError(t, err, tt.wantError) } else { require.NoError(t, err) require.NotNil(t, conn) @@ -1231,6 +1270,12 @@ func TestHostAndPortWithDefaultPort(t *testing.T) { defaultPort: "", wantHostAndPort: "host.example.com", }, + { + name: "host has port and default port is empty", + hostAndPort: "host.example.com:42", + defaultPort: "", + wantHostAndPort: "host.example.com:42", + }, { name: "IPv6 host already has port", hostAndPort: "[::1%lo0]:80", @@ -1257,15 +1302,63 @@ func TestHostAndPortWithDefaultPort(t *testing.T) { }, } for _, test := range tests { - test := test - t.Run(test.name, func(t *testing.T) { - hostAndPort, err := hostAndPortWithDefaultPort(test.hostAndPort, test.defaultPort) - if test.wantError != "" { - require.EqualError(t, err, test.wantError) + tt := test + t.Run(tt.name, func(t *testing.T) { + hostAndPort, err := hostAndPortWithDefaultPort(tt.hostAndPort, tt.defaultPort) + if tt.wantError != "" { + require.EqualError(t, err, tt.wantError) } else { require.NoError(t, err) } - require.Equal(t, test.wantHostAndPort, hostAndPort) + require.Equal(t, tt.wantHostAndPort, hostAndPort) + }) + } +} + +// Test various cases of host and port parsing. +func TestHostWithoutPort(t *testing.T) { + tests := []struct { + name string + hostAndPort string + wantError string + wantHostAndPort string + }{ + { + name: "host already has port", + hostAndPort: "host.example.com:99", + wantHostAndPort: "host.example.com", + }, + { + name: "host does not have port", + hostAndPort: "host.example.com", + wantHostAndPort: "host.example.com", + }, + { + name: "IPv6 host already has port", + hostAndPort: "[::1%lo0]:80", + wantHostAndPort: "[::1%lo0]", + }, + { + name: "IPv6 host does not have port", + hostAndPort: "[::1%lo0]", + wantHostAndPort: "[::1%lo0]", + }, + { + name: "host is not valid", + hostAndPort: "host.example.com:port1:port2", + wantError: "address host.example.com:port1:port2: too many colons in address", + }, + } + for _, test := range tests { + tt := test + t.Run(tt.name, func(t *testing.T) { + hostAndPort, err := hostWithoutPort(tt.hostAndPort) + if tt.wantError != "" { + require.EqualError(t, err, tt.wantError) + } else { + require.NoError(t, err) + } + require.Equal(t, tt.wantHostAndPort, hostAndPort) }) } } diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 3216dde9a..d62b51f4b 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -37,15 +37,19 @@ func TestLDAPSearch(t *testing.T) { cancelFunc() // this will send SIGKILL to the subprocess, just in case }) - hostPorts := findRecentlyUnusedLocalhostPorts(t, 2) - ldapHostPort := hostPorts[0] - unusedHostPort := hostPorts[1] + localhostPorts := findRecentlyUnusedLocalhostPorts(t, 3) + ldapLocalhostPort := localhostPorts[0] + ldapsLocalhostPort := localhostPorts[1] + unusedLocalhostPort := localhostPorts[2] // Expose the the test LDAP server's TLS port on the localhost. - startKubectlPortForward(ctx, t, ldapHostPort, "ldaps", "ldap", env.ToolsNamespace) + startKubectlPortForward(ctx, t, ldapsLocalhostPort, "ldaps", "ldap", env.ToolsNamespace) + + // Expose the the test LDAP server's StartTLS port on the localhost. + startKubectlPortForward(ctx, t, ldapLocalhostPort, "ldap", "ldap", env.ToolsNamespace) providerConfig := func(editFunc func(p *upstreamldap.ProviderConfig)) *upstreamldap.ProviderConfig { - providerConfig := defaultProviderConfig(env, ldapHostPort) + providerConfig := defaultProviderConfig(env, ldapsLocalhostPort) if editFunc != nil { editFunc(providerConfig) } @@ -64,7 +68,7 @@ func TestLDAPSearch(t *testing.T) { wantUnauthenticated bool }{ { - name: "happy path", + name: "happy path with TLS", username: "pinny", password: pinnyPassword, provider: upstreamldap.New(*providerConfig(nil)), @@ -72,6 +76,18 @@ func TestLDAPSearch(t *testing.T) { User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, }, }, + { + name: "happy path with StartTLS", + username: "pinny", + password: pinnyPassword, + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { + p.Host = "127.0.0.1:" + ldapLocalhostPort + p.ConnectionProtocol = upstreamldap.StartTLS + })), + wantAuthResponse: &authenticator.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, + }, + }, { name: "using a different user search base", username: "pinny", @@ -251,6 +267,17 @@ func TestLDAPSearch(t *testing.T) { provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.BindPassword = "wrong-password" })), wantError: `error binding as "cn=admin,dc=pinniped,dc=dev" before user search: LDAP Result Code 49 "Invalid Credentials": `, }, + { + name: "when the bind user username is wrong with StartTLS: example of an error after successful connection with StartTLS", + username: "pinny", + password: pinnyPassword, + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { + p.Host = "127.0.0.1:" + ldapLocalhostPort + p.ConnectionProtocol = upstreamldap.StartTLS + p.BindUsername = "cn=wrong,dc=pinniped,dc=dev" + })), + wantError: `error binding as "cn=wrong,dc=pinniped,dc=dev" before user search: LDAP Result Code 49 "Invalid Credentials": `, + }, { name: "when the end user password is wrong", username: "pinny", @@ -296,32 +323,89 @@ func TestLDAPSearch(t *testing.T) { wantError: `error searching for user "pinny": LDAP Result Code 4 "Size Limit Exceeded": `, }, { - name: "when the server is unreachable", + name: "when the server is unreachable with TLS", username: "pinny", password: pinnyPassword, - provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.Host = "127.0.0.1:" + unusedHostPort })), - wantError: fmt.Sprintf(`error dialing host "127.0.0.1:%s": LDAP Result Code 200 "Network Error": dial tcp 127.0.0.1:%s: connect: connection refused`, unusedHostPort, unusedHostPort), + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.Host = "127.0.0.1:" + unusedLocalhostPort })), + wantError: fmt.Sprintf(`error dialing host "127.0.0.1:%s": LDAP Result Code 200 "Network Error": dial tcp 127.0.0.1:%s: connect: connection refused`, unusedLocalhostPort, unusedLocalhostPort), }, { - name: "when the server is not parsable", + name: "when the server is unreachable with StartTLS", + username: "pinny", + password: pinnyPassword, + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { + p.Host = "127.0.0.1:" + unusedLocalhostPort + p.ConnectionProtocol = upstreamldap.StartTLS + })), + wantError: fmt.Sprintf(`error dialing host "127.0.0.1:%s": LDAP Result Code 200 "Network Error": dial tcp 127.0.0.1:%s: connect: connection refused`, unusedLocalhostPort, unusedLocalhostPort), + }, + { + name: "when the server is not parsable with TLS", username: "pinny", password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.Host = "too:many:ports" })), wantError: `error dialing host "too:many:ports": LDAP Result Code 200 "Network Error": address too:many:ports: too many colons in address`, }, { - name: "when the CA bundle is not parsable", + name: "when the server is not parsable with StartTLS", + username: "pinny", + password: pinnyPassword, + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { + p.Host = "127.0.0.1:" + ldapLocalhostPort + p.ConnectionProtocol = upstreamldap.StartTLS + p.Host = "too:many:ports" + })), + wantError: `error dialing host "too:many:ports": LDAP Result Code 200 "Network Error": address too:many:ports: too many colons in address`, + }, + { + name: "when the CA bundle is not parsable with TLS", username: "pinny", password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.CABundle = []byte("invalid-pem") })), - wantError: fmt.Sprintf(`error dialing host "127.0.0.1:%s": LDAP Result Code 200 "Network Error": could not parse CA bundle`, ldapHostPort), + wantError: fmt.Sprintf(`error dialing host "127.0.0.1:%s": LDAP Result Code 200 "Network Error": could not parse CA bundle`, ldapsLocalhostPort), }, { - name: "when the CA bundle does not cause the host to be trusted", + name: "when the CA bundle is not parsable with StartTLS", + username: "pinny", + password: pinnyPassword, + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { + p.Host = "127.0.0.1:" + ldapLocalhostPort + p.ConnectionProtocol = upstreamldap.StartTLS + p.CABundle = []byte("invalid-pem") + })), + wantError: fmt.Sprintf(`error dialing host "127.0.0.1:%s": LDAP Result Code 200 "Network Error": could not parse CA bundle`, ldapLocalhostPort), + }, + { + name: "when the CA bundle does not cause the host to be trusted with TLS", username: "pinny", password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.CABundle = nil })), - wantError: fmt.Sprintf(`error dialing host "127.0.0.1:%s": LDAP Result Code 200 "Network Error": x509: certificate signed by unknown authority`, ldapHostPort), + wantError: fmt.Sprintf(`error dialing host "127.0.0.1:%s": LDAP Result Code 200 "Network Error": x509: certificate signed by unknown authority`, ldapsLocalhostPort), + }, + { + name: "when the CA bundle does not cause the host to be trusted with StartTLS", + username: "pinny", + password: pinnyPassword, + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { + p.Host = "127.0.0.1:" + ldapLocalhostPort + p.ConnectionProtocol = upstreamldap.StartTLS + p.CABundle = nil + })), + wantError: fmt.Sprintf(`error dialing host "127.0.0.1:%s": LDAP Result Code 200 "Network Error": TLS handshake failed (x509: certificate signed by unknown authority)`, ldapLocalhostPort), + }, + { + name: "when trying to use TLS to connect to a port which only supports StartTLS", + username: "pinny", + password: pinnyPassword, + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.Host = "127.0.0.1:" + ldapLocalhostPort })), + wantError: fmt.Sprintf(`error dialing host "127.0.0.1:%s": LDAP Result Code 200 "Network Error": EOF`, ldapLocalhostPort), + }, + { + name: "when trying to use StartTLS to connect to a port which only supports TLS", + username: "pinny", + password: pinnyPassword, + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.ConnectionProtocol = upstreamldap.StartTLS })), + wantError: fmt.Sprintf(`error dialing host "127.0.0.1:%s": unable to read LDAP response packet: unexpected EOF`, ldapsLocalhostPort), }, { name: "when the UsernameAttribute attribute has multiple values in the entry", @@ -541,13 +625,14 @@ type authUserResult struct { err error } -func defaultProviderConfig(env *library.TestEnv, ldapHostPort string) *upstreamldap.ProviderConfig { +func defaultProviderConfig(env *library.TestEnv, port string) *upstreamldap.ProviderConfig { return &upstreamldap.ProviderConfig{ - Name: "test-ldap-provider", - Host: "127.0.0.1:" + ldapHostPort, - CABundle: []byte(env.SupervisorUpstreamLDAP.CABundle), - BindUsername: "cn=admin,dc=pinniped,dc=dev", - BindPassword: "password", + Name: "test-ldap-provider", + Host: "127.0.0.1:" + port, + ConnectionProtocol: upstreamldap.TLS, + CABundle: []byte(env.SupervisorUpstreamLDAP.CABundle), + BindUsername: "cn=admin,dc=pinniped,dc=dev", + BindPassword: "password", UserSearch: upstreamldap.UserSearchConfig{ Base: "ou=users,dc=pinniped,dc=dev", Filter: "", // defaults to UsernameAttribute={}, i.e. "cn={}" in this case From 7e76b666390bc481493d7d3c7a26a2c07b6b83b0 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 20 May 2021 12:46:33 -0700 Subject: [PATCH 3/5] LDAP upstream watcher controller tries using both TLS and StartTLS - Automatically try to fall back to using StartTLS when using TLS doesn't work. Only complain when both don't work. - Remember (in-memory) which one worked and keeping using that one in the future (unless the pod restarts). --- .../ldap_upstream_watcher.go | 57 ++-- .../ldap_upstream_watcher_test.go | 243 ++++++++++++++---- internal/upstreamldap/upstreamldap.go | 18 +- internal/upstreamldap/upstreamldap_test.go | 24 +- 4 files changed, 263 insertions(+), 79 deletions(-) diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go index 47cb404d6..180b9ef45 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go @@ -58,13 +58,18 @@ type ldapWatcherController struct { } // An in-memory cache with an entry for each LDAPIdentityProvider, to keep track of which ResourceVersion -// of the bind Secret was used during the most recent successful validation. +// of the bind Secret and which TLS/StartTLS setting was used during the most recent successful validation. type secretVersionCache struct { - ResourceVersionsByName map[string]string + ValidatedSettingsByName map[string]validatedSettings +} + +type validatedSettings struct { + BindSecretResourceVersion string + LDAPConnectionProtocol upstreamldap.LDAPConnectionProtocol } func newSecretVersionCache() *secretVersionCache { - return &secretVersionCache{ResourceVersionsByName: map[string]string{}} + return &secretVersionCache{ValidatedSettingsByName: map[string]validatedSettings{}} } // New instantiates a new controllerlib.Controller which will populate the provided UpstreamLDAPIdentityProviderICache. @@ -152,9 +157,8 @@ func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream * spec := upstream.Spec config := &upstreamldap.ProviderConfig{ - Name: upstream.Name, - Host: spec.Host, - ConnectionProtocol: upstreamldap.TLS, + Name: upstream.Name, + Host: spec.Host, UserSearch: upstreamldap.UserSearchConfig{ Base: spec.UserSearch.Base, Filter: spec.UserSearch.Filter, @@ -229,22 +233,23 @@ func (c *ldapWatcherController) validateTLSConfig(upstream *v1alpha1.LDAPIdentit } func (c *ldapWatcherController) validateFinishedConfig(ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider, config *upstreamldap.ProviderConfig, currentSecretVersion string) *v1alpha1.Condition { - ldapProvider := upstreamldap.New(*config) - - if c.hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream, currentSecretVersion) { + if c.hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream, currentSecretVersion, config) { return nil } testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, testLDAPConnectionTimeout) defer cancelFunc() - condition := c.testConnection(testConnectionTimeout, upstream, config, ldapProvider, currentSecretVersion) + condition := c.testConnection(testConnectionTimeout, upstream, config, currentSecretVersion) if condition.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. - c.validatedSecretVersionsCache.ResourceVersionsByName[upstream.GetName()] = currentSecretVersion + c.validatedSecretVersionsCache.ValidatedSettingsByName[upstream.GetName()] = validatedSettings{ + BindSecretResourceVersion: currentSecretVersion, + LDAPConnectionProtocol: config.ConnectionProtocol, + } } return condition @@ -254,10 +259,28 @@ func (c *ldapWatcherController) testConnection( ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider, config *upstreamldap.ProviderConfig, - ldapProvider *upstreamldap.Provider, currentSecretVersion string, ) *v1alpha1.Condition { - err := ldapProvider.TestConnection(ctx) + // First try using TLS. + config.ConnectionProtocol = upstreamldap.TLS + tlsLDAPProvider := upstreamldap.New(*config) + err := tlsLDAPProvider.TestConnection(ctx) + if err != nil { + // If there was any error, try again with StartTLS instead. + config.ConnectionProtocol = upstreamldap.StartTLS + startTLSLDAPProvider := upstreamldap.New(*config) + startTLSErr := startTLSLDAPProvider.TestConnection(ctx) + if startTLSErr == nil { + // Successfully able to fall back to using StartTLS, so clear the original + // error and consider the connection test to be successful. + err = nil + } else { + // Falling back to StartTLS also failed, so put TLS back into the config + // and consider the connection test to be failed. + config.ConnectionProtocol = upstreamldap.TLS + } + } + if err != nil { return &v1alpha1.Condition{ Type: typeLDAPConnectionValid, @@ -277,14 +300,16 @@ func (c *ldapWatcherController) testConnection( } } -func (c *ldapWatcherController) hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream *v1alpha1.LDAPIdentityProvider, currentSecretVersion string) bool { +func (c *ldapWatcherController) hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream *v1alpha1.LDAPIdentityProvider, currentSecretVersion string, config *upstreamldap.ProviderConfig) bool { currentGeneration := upstream.Generation for _, cond := range upstream.Status.Conditions { if cond.Type == typeLDAPConnectionValid && 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. - validatedSecretVersion := c.validatedSecretVersionsCache.ResourceVersionsByName[upstream.GetName()] - if validatedSecretVersion == currentSecretVersion { + validatedSecretVersion := c.validatedSecretVersionsCache.ValidatedSettingsByName[upstream.GetName()] + if validatedSecretVersion.BindSecretResourceVersion == currentSecretVersion { + // Reload the TLS vs StartTLS setting that was previously validated. + config.ConnectionProtocol = validatedSecretVersion.LDAPConnectionProtocol return true } } diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go index ae9346f56..6eaab2f37 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/go-ldap/ldap/v3" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -196,7 +197,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { return deepCopy } - providerConfigForValidUpstream := &upstreamldap.ProviderConfig{ + providerConfigForValidUpstreamWithTLS := &upstreamldap.ProviderConfig{ Name: testName, Host: testHost, ConnectionProtocol: upstreamldap.TLS, @@ -216,6 +217,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, } + // Make a copy with targeted changes. + copyOfProviderConfigForValidUpstreamWithTLS := *providerConfigForValidUpstreamWithTLS + providerConfigForValidUpstreamWithStartTLS := ©OfProviderConfigForValidUpstreamWithTLS + providerConfigForValidUpstreamWithStartTLS.ConnectionProtocol = upstreamldap.StartTLS + bindSecretValidTrueCondition := func(gen int64) v1alpha1.Condition { return v1alpha1.Condition{ Type: "BindSecretValid", @@ -265,16 +271,16 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { } tests := []struct { - name string - initialValidatedSecretVersions map[string]string - inputUpstreams []runtime.Object - inputSecrets []runtime.Object - setupMocks func(conn *mockldapconn.MockConn) - dialError error - wantErr string - wantResultingCache []*upstreamldap.ProviderConfig - wantResultingUpstreams []v1alpha1.LDAPIdentityProvider - wantValidatedSecretVersions map[string]string + name string + initialValidatedSettings map[string]validatedSettings + inputUpstreams []runtime.Object + inputSecrets []runtime.Object + setupMocks func(conn *mockldapconn.MockConn) + dialErrors map[string]error + wantErr string + wantResultingCache []*upstreamldap.ProviderConfig + wantResultingUpstreams []v1alpha1.LDAPIdentityProvider + wantValidatedSettings map[string]validatedSettings }{ { name: "no LDAPIdentityProvider upstreams clears the cache", @@ -289,7 +295,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) conn.EXPECT().Close().Times(1) }, - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -297,7 +303,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, { name: "missing secret", @@ -480,7 +486,121 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + }, + { + 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) { + upstream.Spec.Host = "ldap.example.com" // when the port is not specified, automatically switch ports for StartTLS + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Should perform a test dial and bind. + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Close().Times(1) + }, + dialErrors: map[string]error{ + "ldap.example.com:" + ldap.DefaultLdapsPort: fmt.Errorf("some ldaps dial error"), + "ldap.example.com:" + ldap.DefaultLdapPort: nil, // no error on the regular ldap:// port + }, + wantResultingCache: []*upstreamldap.ProviderConfig{ + { + Name: testName, + Host: "ldap.example.com", + ConnectionProtocol: upstreamldap.StartTLS, // successfully fell back to using StartTLS + CABundle: testCABundle, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: upstreamldap.UserSearchConfig{ + Base: testUserSearchBase, + Filter: testUserSearchFilter, + UsernameAttribute: testUsernameAttrName, + UIDAttribute: testUIDAttrName, + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: testGroupSearchBase, + Filter: testGroupSearchFilter, + GroupNameAttribute: testGroupNameAttrName, + }, + }, + }, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + bindSecretValidTrueCondition(1234), + { + Type: "LDAPConnectionValid", + Status: "True", + LastTransitionTime: now, + 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"), + ObservedGeneration: 1234, + }, + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}}, + }, + { + 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) { + upstream.Spec.Host = "ldap.example.com:5678" // when the port is specified, do not automatically switch ports for StartTLS + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Both dials fail, so there should be no bind. + }, + dialErrors: map[string]error{ + "ldap.example.com:5678": fmt.Errorf("some dial error"), // both TLS and StartTLS should try the same port and both fail + }, + wantResultingCache: []*upstreamldap.ProviderConfig{ + // even though the connection test failed, still loads into the cache because it is treated like a warning + { + Name: testName, + Host: "ldap.example.com:5678", + ConnectionProtocol: upstreamldap.TLS, // need to pick TLS or StartTLS to load into the cache when both fail, so choose TLS + CABundle: testCABundle, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: upstreamldap.UserSearchConfig{ + Base: testUserSearchBase, + Filter: testUserSearchFilter, + UsernameAttribute: testUsernameAttrName, + UIDAttribute: testUIDAttrName, + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: testGroupSearchBase, + Filter: testGroupSearchFilter, + GroupNameAttribute: testGroupNameAttrName, + }, + }, + }, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + bindSecretValidTrueCondition(1234), + { + Type: "LDAPConnectionValid", + Status: "False", + LastTransitionTime: now, + Reason: "LDAPConnectionError", + Message: fmt.Sprintf( + `could not successfully connect to "%s" and bind as user "%s": error dialing host "%s": some dial error`, + "ldap.example.com:5678", testBindUsername, "ldap.example.com:5678"), + ObservedGeneration: 1234, + }, + tlsConfigurationValidLoadedTrueCondition(1234), + }, + }, + }}, }, { name: "non-nil TLS configuration with empty CertificateAuthorityData is valid", @@ -521,7 +641,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, { name: "one valid upstream and one invalid upstream updates the cache to include only the valid upstream", @@ -537,7 +657,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { conn.EXPECT().Close().Times(1) }, wantErr: controllerlib.ErrSyntheticRequeue.Error(), - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{ { ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: "other-upstream", Generation: 42}, @@ -564,7 +684,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, }, }, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, { name: "when testing the connection to the LDAP server fails then the upstream is still added to the cache anyway (treated like a warning)", @@ -572,11 +692,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { inputSecrets: []runtime.Object{validBindUserSecret("")}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. - conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1).Return(errors.New("some bind error")) - conn.EXPECT().Close().Times(1) + // Expect two calls to each of these: once for trying TLS and once for trying StartTLS. + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2).Return(errors.New("some bind error")) + conn.EXPECT().Close().Times(2) }, wantErr: controllerlib.ErrSyntheticRequeue.Error(), - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -599,19 +720,19 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }}, }, { - name: "when the LDAP server connection was already validated for the current resource generation and secret version, then do not validate it again", + name: "when the LDAP server connection was already validated using TLS for the current resource generation and secret version, then do not validate it again and keep using TLS", inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { upstream.Generation = 1234 upstream.Status.Conditions = []v1alpha1.Condition{ ldapConnectionValidTrueCondition(1234, "4242"), } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSecretVersions: map[string]string{testName: "4242"}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, 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. }, - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -619,7 +740,30 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, + }, + { + 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) { + upstream.Generation = 1234 + upstream.Status.Conditions = []v1alpha1.Condition{ + ldapConnectionValidTrueCondition(1234, "4242"), + } + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}}, + 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. + }, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithStartTLS}, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Ready", + Conditions: allConditionsTrue(1234, "4242"), + }, + }}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}}, }, { name: "when the LDAP server connection was validated for an older resource generation, then try to validate it again", @@ -629,14 +773,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ldapConnectionValidTrueCondition(1233, "4242"), // older spec generation! } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSecretVersions: map[string]string{testName: "4242"}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) conn.EXPECT().Close().Times(1) }, - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -644,7 +788,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, { name: "when the LDAP server connection validation previously failed for this resource generation, then try to validate it again", @@ -661,14 +805,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSecretVersions: map[string]string{testName: "1"}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "1", LDAPConnectionProtocol: upstreamldap.TLS}}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) conn.EXPECT().Close().Times(1) }, - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -676,7 +820,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, { name: "when the LDAP server connection was already validated for this resource generation but the bind secret has changed, then try to validate it again", @@ -686,14 +830,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ldapConnectionValidTrueCondition(1234, "4241"), // same spec generation, old secret version } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, // newer secret version! - initialValidatedSecretVersions: map[string]string{testName: "4241"}, // old version was validated + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, // newer secret version! + initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4241", LDAPConnectionProtocol: upstreamldap.TLS}}, // old version was validated setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) conn.EXPECT().Close().Times(1) }, - wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream}, + wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ @@ -701,7 +845,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSecretVersions: map[string]string{testName: "4242"}, + wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}}, }, } @@ -727,16 +871,19 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { tt.setupMocks(conn) } - dialer := &comparableDialer{upstreamldap.LDAPDialerFunc(func(ctx context.Context, _ string) (upstreamldap.Conn, error) { - if tt.dialError != nil { - return nil, tt.dialError + dialer := &comparableDialer{upstreamldap.LDAPDialerFunc(func(ctx context.Context, hostAndPort string) (upstreamldap.Conn, error) { + if tt.dialErrors != nil { + dialErr := tt.dialErrors[hostAndPort] + if dialErr != nil { + return nil, dialErr + } } return conn, nil })} validatedSecretVersionCache := newSecretVersionCache() - if tt.initialValidatedSecretVersions != nil { - validatedSecretVersionCache.ResourceVersionsByName = tt.initialValidatedSecretVersions + if tt.initialValidatedSettings != nil { + validatedSecretVersionCache.ValidatedSettingsByName = tt.initialValidatedSettings } controller := newInternal( @@ -768,11 +915,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { require.Equal(t, len(tt.wantResultingCache), len(actualIDPList)) for i := range actualIDPList { actualIDP := actualIDPList[i].(*upstreamldap.Provider) - copyOfExpectedValue := *tt.wantResultingCache[i] // copy before edit to avoid race because these tests are run in parallel + copyOfExpectedValueForResultingCache := *tt.wantResultingCache[i] // copy before edit to avoid race because these tests are run in parallel // The dialer that was passed in to the controller's constructor should always have been // passed through to the provider. - copyOfExpectedValue.Dialer = dialer - require.Equal(t, copyOfExpectedValue, actualIDP.GetConfig()) + copyOfExpectedValueForResultingCache.Dialer = dialer + require.Equal(t, copyOfExpectedValueForResultingCache, actualIDP.GetConfig()) } actualUpstreams, err := fakePinnipedClient.IDPV1alpha1().LDAPIdentityProviders(testNamespace).List(ctx, metav1.ListOptions{}) @@ -787,10 +934,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { } // Check that the controller remembered which version of the secret it most recently validated successfully with. - if tt.wantValidatedSecretVersions == nil { - tt.wantValidatedSecretVersions = map[string]string{} + if tt.wantValidatedSettings == nil { + tt.wantValidatedSettings = map[string]validatedSettings{} } - require.Equal(t, tt.wantValidatedSecretVersions, validatedSecretVersionCache.ResourceVersionsByName) + require.Equal(t, tt.wantValidatedSettings, validatedSecretVersionCache.ValidatedSettingsByName) }) } } diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 39d51e8cf..54c51293a 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -157,16 +157,26 @@ func (p *Provider) dial(ctx context.Context) (Conn, error) { return nil, ldap.NewError(ldap.ErrorNetwork, err) } + // Choose how and where to dial based on TLS vs. StartTLS config option. + var dialFunc LDAPDialerFunc + var hostAndPort string switch { - case p.c.Dialer != nil: - return p.c.Dialer.Dial(ctx, tlsHostAndPort) case p.c.ConnectionProtocol == TLS: - return p.dialTLS(ctx, tlsHostAndPort) + dialFunc = p.dialTLS + hostAndPort = tlsHostAndPort case p.c.ConnectionProtocol == StartTLS: - return p.dialStartTLS(ctx, startTLSHostAndPort) + dialFunc = p.dialStartTLS + hostAndPort = startTLSHostAndPort default: return nil, ldap.NewError(ldap.ErrorNetwork, fmt.Errorf("did not specify valid ConnectionProtocol")) } + + // Override the real dialer for testing purposes sometimes. + if p.c.Dialer != nil { + dialFunc = p.c.Dialer.Dial + } + + return dialFunc(ctx, hostAndPort) } // dialTLS is a default implementation of the Dialer, used when Dialer is nil and ConnectionProtocol is TLS. diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 56203fbbb..f66d69ce7 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -55,11 +55,12 @@ var ( func TestEndUserAuthentication(t *testing.T) { providerConfig := func(editFunc func(p *ProviderConfig)) *ProviderConfig { config := &ProviderConfig{ - 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, + 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 + ConnectionProtocol: TLS, + BindUsername: testBindUsername, + BindPassword: testBindPassword, UserSearch: UserSearchConfig{ Base: testUserSearchBase, Filter: testUserSearchFilter, @@ -989,12 +990,13 @@ func TestEndUserAuthentication(t *testing.T) { func TestTestConnection(t *testing.T) { providerConfig := func(editFunc func(p *ProviderConfig)) *ProviderConfig { config := &ProviderConfig{ - 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: UserSearchConfig{}, // not used by TestConnection + 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 + ConnectionProtocol: TLS, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: UserSearchConfig{}, // not used by TestConnection } if editFunc != nil { editFunc(config) From 8b549f66d4623b6095050dc64c1a1b07c3946a4a Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 20 May 2021 13:39:48 -0700 Subject: [PATCH 4/5] Add integration test for LDAP StartTLS --- hack/prepare-for-integration-tests.sh | 1 + .../ldapupstreamwatcher/ldap_upstream_watcher.go | 4 ++++ test/integration/supervisor_login_test.go | 10 +++++----- test/library/client.go | 5 ++++- test/library/env.go | 2 ++ 5 files changed, 16 insertions(+), 6 deletions(-) diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 0313b60a7..b918049bd 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -333,6 +333,7 @@ export PINNIPED_TEST_SUPERVISOR_HTTP_ADDRESS="127.0.0.1:12345" export PINNIPED_TEST_SUPERVISOR_HTTPS_ADDRESS="localhost:12344" export PINNIPED_TEST_PROXY=http://127.0.0.1:12346 export PINNIPED_TEST_LDAP_HOST=ldap.tools.svc.cluster.local +export PINNIPED_TEST_LDAP_STARTTLS_ONLY_HOST=ldapstarttls.tools.svc.cluster.local export PINNIPED_TEST_LDAP_LDAPS_CA_BUNDLE="${test_ca_bundle_pem}" export PINNIPED_TEST_LDAP_BIND_ACCOUNT_USERNAME="cn=admin,dc=pinniped,dc=dev" export PINNIPED_TEST_LDAP_BIND_ACCOUNT_PASSWORD=password diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go index 180b9ef45..3f1707b98 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go @@ -26,6 +26,7 @@ import ( "go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatchers" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/upstreamldap" ) @@ -266,15 +267,18 @@ func (c *ldapWatcherController) testConnection( tlsLDAPProvider := upstreamldap.New(*config) err := tlsLDAPProvider.TestConnection(ctx) if err != nil { + plog.InfoErr("testing LDAP connection using TLS failed, so trying again with StartTLS", err, "host", config.Host) // If there was any error, try again with StartTLS instead. config.ConnectionProtocol = upstreamldap.StartTLS startTLSLDAPProvider := upstreamldap.New(*config) startTLSErr := startTLSLDAPProvider.TestConnection(ctx) if startTLSErr == nil { + plog.Info("testing LDAP connection using StartTLS succeeded", "host", config.Host) // Successfully able to fall back to using StartTLS, so clear the original // error and consider the connection test to be successful. err = nil } else { + plog.InfoErr("testing LDAP connection using StartTLS also failed", err, "host", config.Host) // Falling back to StartTLS also failed, so put TLS back into the config // and consider the connection test to be failed. config.ConnectionProtocol = upstreamldap.TLS diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 41d97aebc..1690c1096 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -69,7 +69,7 @@ func TestSupervisorLogin(t *testing.T) { wantDownstreamIDTokenGroups: env.SupervisorUpstreamOIDC.ExpectedGroups, }, { - name: "ldap with email as username and groups names as DNs", + name: "ldap with email as username and groups names as DNs and using an LDAP provider which supports TLS", createIDP: func(t *testing.T) { t.Helper() secret := library.CreateTestSecret(t, env.SupervisorNamespace, "ldap-service-account", v1.SecretTypeBasicAuth, @@ -126,7 +126,7 @@ func TestSupervisorLogin(t *testing.T) { wantDownstreamIDTokenGroups: env.SupervisorUpstreamLDAP.TestUserDirectGroupsDNs, }, { - name: "ldap with CN as username and group names as CNs", // try another variation of configuration options + name: "ldap with CN as username and group names as CNs and using an LDAP provider which only supports StartTLS", // try another variation of configuration options createIDP: func(t *testing.T) { t.Helper() secret := library.CreateTestSecret(t, env.SupervisorNamespace, "ldap-service-account", v1.SecretTypeBasicAuth, @@ -136,7 +136,7 @@ func TestSupervisorLogin(t *testing.T) { }, ) ldapIDP := library.CreateTestLDAPIdentityProvider(t, idpv1alpha1.LDAPIdentityProviderSpec{ - Host: env.SupervisorUpstreamLDAP.Host, + Host: env.SupervisorUpstreamLDAP.StartTLSOnlyHost, TLS: &idpv1alpha1.TLSSpec{ CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamLDAP.CABundle)), }, @@ -161,7 +161,7 @@ func TestSupervisorLogin(t *testing.T) { }, idpv1alpha1.LDAPPhaseReady) expectedMsg := fmt.Sprintf( `successfully able to connect to "%s" and bind as user "%s" [validated with Secret "%s" at version "%s"]`, - env.SupervisorUpstreamLDAP.Host, env.SupervisorUpstreamLDAP.BindUsername, + env.SupervisorUpstreamLDAP.StartTLSOnlyHost, env.SupervisorUpstreamLDAP.BindUsername, secret.Name, secret.ResourceVersion, ) requireSuccessfulLDAPIdentityProviderConditions(t, ldapIDP, expectedMsg) @@ -176,7 +176,7 @@ func TestSupervisorLogin(t *testing.T) { }, // 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.SupervisorUpstreamLDAP.Host + "?sub=" + env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue, + "ldaps://" + env.SupervisorUpstreamLDAP.StartTLSOnlyHost + "?sub=" + env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue, ), // the ID token Username should have been pulled from the requested UserSearch.Attributes.Username attribute wantDownstreamIDTokenUsernameToMatch: regexp.QuoteMeta(env.SupervisorUpstreamLDAP.TestUserDN), diff --git a/test/library/client.go b/test/library/client.go index f256444e5..c9e9baca7 100644 --- a/test/library/client.go +++ b/test/library/client.go @@ -437,7 +437,10 @@ func CreateTestLDAPIdentityProvider(t *testing.T, spec idpv1alpha1.LDAPIdentityP return false } return result.Status.Phase == expectedPhase - }, 60*time.Second, 1*time.Second, "expected the LDAPIdentityProvider to go into phase %s, LDAPIdentityProvider was: %s", expectedPhase, Sdump(result)) + }, + 2*time.Minute, // it takes 1 minute for a failed LDAP TLS connection test to timeout before it tries using StartTLS, so wait longer than that + 1*time.Second, + "expected the LDAPIdentityProvider to go into phase %s, LDAPIdentityProvider was: %s", expectedPhase, Sdump(result)) return result } diff --git a/test/library/env.go b/test/library/env.go index e41038430..d8e7b44c8 100644 --- a/test/library/env.go +++ b/test/library/env.go @@ -73,6 +73,7 @@ type TestOIDCUpstream struct { type TestLDAPUpstream struct { Host string `json:"host"` + StartTLSOnlyHost string `json:"startTLSOnlyHost"` CABundle string `json:"caBundle"` BindUsername string `json:"bindUsername"` BindPassword string `json:"bindPassword"` @@ -240,6 +241,7 @@ func loadEnvVars(t *testing.T, result *TestEnv) { result.SupervisorUpstreamLDAP = TestLDAPUpstream{ Host: needEnv(t, "PINNIPED_TEST_LDAP_HOST"), + StartTLSOnlyHost: needEnv(t, "PINNIPED_TEST_LDAP_STARTTLS_ONLY_HOST"), CABundle: base64Decoded(t, os.Getenv("PINNIPED_TEST_LDAP_LDAPS_CA_BUNDLE")), BindUsername: needEnv(t, "PINNIPED_TEST_LDAP_BIND_ACCOUNT_USERNAME"), BindPassword: needEnv(t, "PINNIPED_TEST_LDAP_BIND_ACCOUNT_PASSWORD"), From b01665386d212c7c496eb38aeff91628113e5e24 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 20 May 2021 15:49:34 -0700 Subject: [PATCH 5/5] Use latest container image of our fork of bitnami-docker-openldap --- test/deploy/tools/ldap.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/deploy/tools/ldap.yaml b/test/deploy/tools/ldap.yaml index 9405df9f3..25410fcb2 100644 --- a/test/deploy/tools/ldap.yaml +++ b/test/deploy/tools/ldap.yaml @@ -208,8 +208,8 @@ spec: - name: ldap #! Use our own fork of docker.io/bitnami/openldap for now, because we added the #! LDAP_SERVER_CONFIG_BEFORE_CUSTOM_LDIF_DIR and LDAP_SERVER_CONFIG_AFTER_CUSTOM_LDIF_DIR options. - #! See https://github.com/pinniped-ci-bot/bitnami-docker-openldap/tree/ldap_server_config - image: ghcr.io/pinniped-ci-bot/bitnami-openldap-forked:2.4.58-debian-10-r57 + #! See https://github.com/pinniped-ci-bot/bitnami-docker-openldap/tree/pinniped + image: projects.registry.vmware.com/pinniped/test-ldap:latest imagePullPolicy: Always ports: - name: ldap