diff --git a/internal/controller/authenticator/cachecleaner/cachecleaner_test.go b/internal/controller/authenticator/cachecleaner/cachecleaner_test.go index fd637a65c..9602f05f8 100644 --- a/internal/controller/authenticator/cachecleaner/cachecleaner_test.go +++ b/internal/controller/authenticator/cachecleaner/cachecleaner_test.go @@ -8,16 +8,16 @@ import ( "testing" "github.com/stretchr/testify/require" - "go.uber.org/mock/gomock" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/authentication/authenticator" authv1alpha "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1" pinnipedfake "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned/fake" pinnipedinformers "go.pinniped.dev/generated/latest/client/concierge/informers/externalversions" + controllerAuthenticator "go.pinniped.dev/internal/controller/authenticator" "go.pinniped.dev/internal/controller/authenticator/authncache" "go.pinniped.dev/internal/controllerlib" - "go.pinniped.dev/internal/mocks/mocktokenauthenticatorcloser" "go.pinniped.dev/internal/testutil/testlogger" ) @@ -109,8 +109,8 @@ func TestController(t *testing.T) { initialCache: func(t *testing.T, cache *authncache.Cache) { cache.Store(testWebhookKey1, nil) cache.Store(testWebhookKey2, nil) - cache.Store(testJWTAuthenticatorKey1, newClosableCacheValue(t, 0)) - cache.Store(testJWTAuthenticatorKey2, newClosableCacheValue(t, 1)) + cache.Store(testJWTAuthenticatorKey1, newClosableCacheValue(t, false)) + cache.Store(testJWTAuthenticatorKey2, newClosableCacheValue(t, true)) cache.Store(testKeyUnknownType, nil) }, objects: []runtime.Object{ @@ -174,10 +174,28 @@ func TestController(t *testing.T) { } } -func newClosableCacheValue(t *testing.T, wantCloses int) authncache.Value { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - tac := mocktokenauthenticatorcloser.NewMockTokenAuthenticatorCloser(ctrl) - tac.EXPECT().Close().Times(wantCloses) - return tac +type mockValue struct { + wasClosed bool +} + +func (m *mockValue) Close() { + m.wasClosed = true +} + +func (m *mockValue) AuthenticateToken(_ context.Context, _ string) (*authenticator.Response, bool, error) { + panic("implement me") +} + +var _ authncache.Value = (*mockValue)(nil) +var _ controllerAuthenticator.Closer = (*mockValue)(nil) + +func newClosableCacheValue(t *testing.T, wantClose bool) authncache.Value { + t.Helper() + mock := &mockValue{} + + t.Cleanup(func() { + require.Equal(t, wantClose, mock.wasClosed) + }) + + return mock } diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index eda1fd78c..30016836d 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -102,10 +102,17 @@ type tokenAuthenticatorCloser interface { } type cachedJWTAuthenticator struct { - tokenAuthenticatorCloser - spec *auth1alpha1.JWTAuthenticatorSpec + authenticator.Token + spec *auth1alpha1.JWTAuthenticatorSpec + cancel context.CancelFunc } +func (c *cachedJWTAuthenticator) Close() { + c.cancel() +} + +var _ tokenAuthenticatorCloser = (*cachedJWTAuthenticator)(nil) + // New instantiates a new controllerlib.Controller which will populate the provided authncache.Cache. func New( cache *authncache.Cache, @@ -162,7 +169,7 @@ func (c *jwtCacheFillerController) Sync(ctx controllerlib.Context) error { // If this authenticator already exists, then only recreate it if is different from the desired // authenticator. We don't want to be creating a new authenticator for every resync period. // - // If we do need to recreate the authenticator, then make sure we close the old one to avoid + // If we do need to recreate the authenticator, then make sure cancel the old one to avoid // goroutine leaks. if value := c.cache.Get(cacheKey); value != nil { jwtAuthenticator := c.extractValueAsJWTAuthenticator(value) @@ -517,7 +524,8 @@ func (c *jwtCacheFillerController) newCachedJWTAuthenticator(client *http.Client groupsClaim = defaultGroupsClaim } - oidcAuthenticator, err := oidc.New(oidc.Options{ + cancelCtx, cancel := context.WithCancel(context.Background()) + oidcAuthenticator, err := oidc.New(cancelCtx, oidc.Options{ JWTAuthenticator: apiserver.JWTAuthenticator{ Issuer: apiserver.Issuer{ URL: spec.Issuer, @@ -552,6 +560,7 @@ func (c *jwtCacheFillerController) newCachedJWTAuthenticator(client *http.Client Reason: reasonInvalidAuthenticator, Message: msg, }) + cancel() // resync err, lots of possible issues that may or may not be machine related return nil, conditions, fmt.Errorf("%s: %w", errText, err) } @@ -563,8 +572,9 @@ func (c *jwtCacheFillerController) newCachedJWTAuthenticator(client *http.Client Message: msg, }) return &cachedJWTAuthenticator{ - tokenAuthenticatorCloser: oidcAuthenticator, - spec: spec, + Token: oidcAuthenticator, + spec: spec, + cancel: cancel, }, conditions, nil } diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index 188ce5398..3777051eb 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go @@ -24,7 +24,6 @@ import ( fositejwt "github.com/ory/fosite/token/jwt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.uber.org/mock/gomock" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -40,7 +39,6 @@ import ( "go.pinniped.dev/internal/controller/authenticator/authncache" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/crypto/ptls" - "go.pinniped.dev/internal/mocks/mocktokenauthenticatorcloser" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/conciergetestutil" @@ -1749,11 +1747,12 @@ func TestController(t *testing.T) { Kind: "JWTAuthenticator", Name: syncCtx.Key.Name, } - cachedAuthenticator := cache.Get(expectedCacheKey) + cachedAuthenticator, ok := cache.Get(expectedCacheKey).(tokenAuthenticatorCloser) + require.True(t, ok) require.NotNil(t, cachedAuthenticator) // Schedule it to be closed at the end of the test. - t.Cleanup(cachedAuthenticator.(*cachedJWTAuthenticator).Close) + t.Cleanup(cachedAuthenticator.Close) const ( goodSubject = "some-subject" @@ -2087,18 +2086,17 @@ func createJWT( } func newCacheValue(t *testing.T, spec auth1alpha1.JWTAuthenticatorSpec, wantClose bool) authncache.Value { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - tokenAuthenticatorCloser := mocktokenauthenticatorcloser.NewMockTokenAuthenticatorCloser(ctrl) + t.Helper() + wasClosed := false - wantCloses := 0 - if wantClose { - wantCloses++ - } - tokenAuthenticatorCloser.EXPECT().Close().Times(wantCloses) + t.Cleanup(func() { + require.Equal(t, wantClose, wasClosed) + }) return &cachedJWTAuthenticator{ - tokenAuthenticatorCloser: tokenAuthenticatorCloser, - spec: &spec, + spec: &spec, + cancel: func() { + wasClosed = true + }, } } diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 140a64e10..2a5af83c9 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -1,4 +1,4 @@ -// Copyright 2021-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package impersonatorconfig @@ -27,6 +27,7 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/apimachinery/pkg/util/validation/field" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" "k8s.io/klog/v2" @@ -1205,7 +1206,7 @@ func validateCredentialIssuerSpec(spec *v1alpha1.ImpersonationProxySpec) error { } // If specified, validate that the LoadBalancerIP is a valid IPv4 or IPv6 address. - if ip := spec.Service.LoadBalancerIP; ip != "" && len(validation.IsValidIP(ip)) > 0 { + if ip := spec.Service.LoadBalancerIP; ip != "" && len(validation.IsValidIP(field.NewPath("spec", "service", "loadBalancerIP"), ip)) > 0 { return fmt.Errorf("invalid LoadBalancerIP %q", spec.Service.LoadBalancerIP) } diff --git a/internal/endpointaddr/endpointaddr.go b/internal/endpointaddr/endpointaddr.go index c6d0223af..ebf1db88a 100644 --- a/internal/endpointaddr/endpointaddr.go +++ b/internal/endpointaddr/endpointaddr.go @@ -12,6 +12,7 @@ import ( "strings" "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/apimachinery/pkg/util/validation/field" ) type HostPort struct { @@ -41,7 +42,7 @@ func (h *HostPort) Endpoint() string { // - "[]:" (IPv6 address with port, brackets are required) // // If the input does not specify a port number, then defaultPort will be used. -func Parse(endpoint string, defaultPort uint16) (HostPort, error) { +func Parse(endpoint string, defaultPort uint16, paths ...string) (HostPort, error) { // Try parsing it both with and without an implicit port 443 at the end. host, port, err := net.SplitHostPort(endpoint) @@ -61,9 +62,13 @@ func Parse(endpoint string, defaultPort uint16) (HostPort, error) { return HostPort{}, fmt.Errorf("invalid port %q", port) } + if len(paths) < 1 { + paths = []string{"UNKNOWN_PATH"} + } + // Check if the host part is a IPv4 or IPv6 address or a valid hostname according to RFC 1123. switch { - case len(validation.IsValidIP(host)) == 0: + case len(validation.IsValidIP(field.NewPath(paths[0], paths[1:]...), host)) == 0: case len(validation.IsDNS1123Subdomain(host)) == 0: default: return HostPort{}, fmt.Errorf("host %q is not a valid hostname or IP address", host) diff --git a/internal/endpointaddr/endpointaddr_test.go b/internal/endpointaddr/endpointaddr_test.go index adf6bb0b7..15972c907 100644 --- a/internal/endpointaddr/endpointaddr_test.go +++ b/internal/endpointaddr/endpointaddr_test.go @@ -17,6 +17,7 @@ func TestParse(t *testing.T) { name string input string defaultPort uint16 + paths []string expectErr string expect HostPort expectEndpoint string @@ -42,6 +43,13 @@ func TestParse(t *testing.T) { expect: HostPort{Host: "127.0.0.1", Port: 8443}, expectEndpoint: "127.0.0.1:8443", }, + { + name: "invalid IPv4", + input: "1.1.1.", + defaultPort: 443, + paths: []string{"does", "not", "matter", "because", "this", "error", "is", "ignored"}, + expectErr: `host "1.1.1." is not a valid hostname or IP address`, + }, { name: "IPv4 as IPv6 in brackets with port", input: "[::127.0.0.1]:8443", @@ -170,7 +178,7 @@ func TestParse(t *testing.T) { } { tt := tt t.Run(tt.name, func(t *testing.T) { - got, err := Parse(tt.input, tt.defaultPort) + got, err := Parse(tt.input, tt.defaultPort, tt.paths...) if tt.expectErr == "" { assert.NoError(t, err) assert.Equal(t, tt.expect, got) diff --git a/internal/mocks/mocktokenauthenticatorcloser/generate.go b/internal/mocks/mocktokenauthenticatorcloser/generate.go deleted file mode 100644 index 02790faff..000000000 --- a/internal/mocks/mocktokenauthenticatorcloser/generate.go +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package mocktokenauthenticatorcloser - -import ( - "k8s.io/apiserver/pkg/authentication/authenticator" - - pinnipedauthenticator "go.pinniped.dev/internal/controller/authenticator" -) - -//go:generate go run -v go.uber.org/mock/mockgen -destination=mocktokenauthenticatorcloser.go -package=mocktokenauthenticatorcloser -copyright_file=../../../hack/header.txt . TokenAuthenticatorCloser - -// TokenAuthenticatorCloser is a type that can authenticate tokens and be closed idempotently. -// -// This type is slightly different from io.Closer, because io.Closer can return an error and is not -// necessarily idempotent. -type TokenAuthenticatorCloser interface { - authenticator.Token - pinnipedauthenticator.Closer -} diff --git a/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go b/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go deleted file mode 100644 index 8e5d140e9..000000000 --- a/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go +++ /dev/null @@ -1,73 +0,0 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 -// - -// Code generated by MockGen. DO NOT EDIT. -// Source: go.pinniped.dev/internal/mocks/mocktokenauthenticatorcloser (interfaces: TokenAuthenticatorCloser) -// -// Generated by this command: -// -// mockgen -destination=mocktokenauthenticatorcloser.go -package=mocktokenauthenticatorcloser -copyright_file=../../../hack/header.txt . TokenAuthenticatorCloser -// - -// Package mocktokenauthenticatorcloser is a generated GoMock package. -package mocktokenauthenticatorcloser - -import ( - context "context" - reflect "reflect" - - gomock "go.uber.org/mock/gomock" - authenticator "k8s.io/apiserver/pkg/authentication/authenticator" -) - -// MockTokenAuthenticatorCloser is a mock of TokenAuthenticatorCloser interface. -type MockTokenAuthenticatorCloser struct { - ctrl *gomock.Controller - recorder *MockTokenAuthenticatorCloserMockRecorder -} - -// MockTokenAuthenticatorCloserMockRecorder is the mock recorder for MockTokenAuthenticatorCloser. -type MockTokenAuthenticatorCloserMockRecorder struct { - mock *MockTokenAuthenticatorCloser -} - -// NewMockTokenAuthenticatorCloser creates a new mock instance. -func NewMockTokenAuthenticatorCloser(ctrl *gomock.Controller) *MockTokenAuthenticatorCloser { - mock := &MockTokenAuthenticatorCloser{ctrl: ctrl} - mock.recorder = &MockTokenAuthenticatorCloserMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockTokenAuthenticatorCloser) EXPECT() *MockTokenAuthenticatorCloserMockRecorder { - return m.recorder -} - -// AuthenticateToken mocks base method. -func (m *MockTokenAuthenticatorCloser) AuthenticateToken(arg0 context.Context, arg1 string) (*authenticator.Response, bool, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AuthenticateToken", arg0, arg1) - ret0, _ := ret[0].(*authenticator.Response) - ret1, _ := ret[1].(bool) - ret2, _ := ret[2].(error) - return ret0, ret1, ret2 -} - -// AuthenticateToken indicates an expected call of AuthenticateToken. -func (mr *MockTokenAuthenticatorCloserMockRecorder) AuthenticateToken(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AuthenticateToken", reflect.TypeOf((*MockTokenAuthenticatorCloser)(nil).AuthenticateToken), arg0, arg1) -} - -// Close mocks base method. -func (m *MockTokenAuthenticatorCloser) Close() { - m.ctrl.T.Helper() - m.ctrl.Call(m, "Close") -} - -// Close indicates an expected call of Close. -func (mr *MockTokenAuthenticatorCloserMockRecorder) Close() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockTokenAuthenticatorCloser)(nil).Close)) -} diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index c567c277a..b49333608 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -326,7 +326,7 @@ func (p *Provider) dialTLS(ctx context.Context, addr endpointaddr.HostPort) (Con } conn := ldap.NewConn(c, true) - conn.Start() //nolint:staticcheck // will need a different approach soon + conn.Start() return conn, nil } @@ -348,7 +348,7 @@ func (p *Provider) dialStartTLS(ctx context.Context, addr endpointaddr.HostPort) } conn := ldap.NewConn(c, false) - conn.Start() //nolint:staticcheck // will need a different approach soon + conn.Start() err = conn.StartTLS(tlsConfig) if err != nil { return nil, err diff --git a/test/testlib/activedirectory.go b/test/testlib/activedirectory.go index c9327d9d7..d29d29838 100644 --- a/test/testlib/activedirectory.go +++ b/test/testlib/activedirectory.go @@ -199,7 +199,7 @@ func dialTLS(t *testing.T, env *TestEnv) *ldap.Conn { c, err := dialer.DialContext(context.Background(), "tcp", env.SupervisorUpstreamActiveDirectory.Host) require.NoError(t, err) conn := ldap.NewConn(c, true) - conn.Start() //nolint:staticcheck // will need a different approach soon + conn.Start() return conn }