From 6cdd4a950610fd2906ec053edbe2e8e1f18b49b0 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Mon, 21 Sep 2020 11:37:54 -0500 Subject: [PATCH] Add support for multiple IDPs selected using IdentityProvider field. This also has fallback compatibility support if no IDP is specified and there is exactly one IDP in the cache. Signed-off-by: Matt Moyer --- internal/apiserver/apiserver.go | 5 +- .../identityprovider/idpcache/cache.go | 103 +++-- .../identityprovider/idpcache/cache_test.go | 253 +++++++++---- .../webhookcachecleaner.go | 5 +- .../webhookcachecleaner_test.go | 54 ++- .../webhookcachefiller/webhookcachefiller.go | 7 +- .../credentialrequestmocks.go | 96 +++++ .../mocks/credentialrequestmocks/generate.go | 6 + internal/mocks/mockcertissuer/generate.go | 6 - .../mocks/mockcertissuer/mockcertissuer.go | 56 --- internal/registry/credentialrequest/rest.go | 56 ++- .../registry/credentialrequest/rest_test.go | 356 ++++-------------- internal/server/server.go | 9 +- 13 files changed, 519 insertions(+), 493 deletions(-) create mode 100644 internal/mocks/credentialrequestmocks/credentialrequestmocks.go create mode 100644 internal/mocks/credentialrequestmocks/generate.go delete mode 100644 internal/mocks/mockcertissuer/generate.go delete mode 100644 internal/mocks/mockcertissuer/mockcertissuer.go diff --git a/internal/apiserver/apiserver.go b/internal/apiserver/apiserver.go index 2cd3ce57..98d2c7bd 100644 --- a/internal/apiserver/apiserver.go +++ b/internal/apiserver/apiserver.go @@ -12,7 +12,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" utilruntime "k8s.io/apimachinery/pkg/util/runtime" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/registry/rest" genericapiserver "k8s.io/apiserver/pkg/server" "k8s.io/client-go/pkg/version" @@ -54,7 +53,7 @@ type Config struct { } type ExtraConfig struct { - TokenAuthenticator authenticator.Token + Authenticator credentialrequest.TokenCredentialRequestAuthenticator Issuer credentialrequest.CertIssuer StartControllersPostStartHook func(ctx context.Context) } @@ -98,7 +97,7 @@ func (c completedConfig) New() (*PinnipedServer, error) { } gvr := loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests") - storage := credentialrequest.NewREST(c.ExtraConfig.TokenAuthenticator, c.ExtraConfig.Issuer) + storage := credentialrequest.NewREST(c.ExtraConfig.Authenticator, c.ExtraConfig.Issuer) if err := s.GenericAPIServer.InstallAPIGroup(&genericapiserver.APIGroupInfo{ PrioritizedVersions: []schema.GroupVersion{gvr.GroupVersion()}, VersionedResourcesStorageMap: map[string]map[string]rest.Storage{gvr.Version: {gvr.Resource: storage}}, diff --git a/internal/controller/identityprovider/idpcache/cache.go b/internal/controller/identityprovider/idpcache/cache.go index fa2480f0..ddbe63c7 100644 --- a/internal/controller/identityprovider/idpcache/cache.go +++ b/internal/controller/identityprovider/idpcache/cache.go @@ -10,15 +10,19 @@ import ( "sync" "k8s.io/apiserver/pkg/authentication/authenticator" + "k8s.io/apiserver/pkg/authentication/user" - "go.pinniped.dev/internal/controllerlib" + loginapi "go.pinniped.dev/generated/1.19/apis/login" ) var ( - // ErrNoIDPs is returned by Cache.AuthenticateToken() when there are no IDPs configured. + // ErrNoSuchIDP is returned by Cache.AuthenticateTokenCredentialRequest() when the requested IDP is not configured. + ErrNoSuchIDP = fmt.Errorf("no such identity provider") + + // ErrNoIDPs is returned by Cache.AuthenticateTokenCredentialRequest() when there are no IDPs configured. ErrNoIDPs = fmt.Errorf("no identity providers are loaded") - // ErrIndeterminateIDP is returned by Cache.AuthenticateToken() when the correct IDP cannot be determined. + // ErrIndeterminateIDP is returned by Cache.AuthenticateTokenCredentialRequest() when the correct IDP cannot be determined. ErrIndeterminateIDP = fmt.Errorf("could not uniquely match against an identity provider") ) @@ -28,48 +32,101 @@ type Cache struct { cache sync.Map } +type Key struct { + APIGroup string + Kind string + Namespace string + Name string +} + +type Value interface { + authenticator.Token +} + // New returns an empty cache. func New() *Cache { return &Cache{} } +// Get an identity provider by key. +func (c *Cache) Get(key Key) Value { + res, _ := c.cache.Load(key) + if res == nil { + return nil + } + return res.(Value) +} + // Store an identity provider into the cache. -func (c *Cache) Store(key controllerlib.Key, value authenticator.Token) { +func (c *Cache) Store(key Key, value Value) { c.cache.Store(key, value) } // Delete an identity provider from the cache. -func (c *Cache) Delete(key controllerlib.Key) { +func (c *Cache) Delete(key Key) { c.cache.Delete(key) } // Keys currently stored in the cache. -func (c *Cache) Keys() []controllerlib.Key { - var result []controllerlib.Key +func (c *Cache) Keys() []Key { + var result []Key c.cache.Range(func(key, _ interface{}) bool { - result = append(result, key.(controllerlib.Key)) + result = append(result, key.(Key)) return true }) return result } -// AuthenticateToken validates the provided token against the currently loaded identity providers. -func (c *Cache) AuthenticateToken(ctx context.Context, token string) (*authenticator.Response, bool, error) { - var matchingIDPs []authenticator.Token - c.cache.Range(func(key, value interface{}) bool { - matchingIDPs = append(matchingIDPs, value.(authenticator.Token)) - return true - }) - - // Return an error if there are no known IDPs. - if len(matchingIDPs) == 0 { - return nil, false, ErrNoIDPs +func (c *Cache) AuthenticateTokenCredentialRequest(ctx context.Context, req *loginapi.TokenCredentialRequest) (user.Info, error) { + // Map the incoming request to a cache key. + key := Key{ + Namespace: req.Namespace, + Name: req.Spec.IdentityProvider.Name, + Kind: req.Spec.IdentityProvider.Kind, + } + if req.Spec.IdentityProvider.APIGroup != nil { + key.APIGroup = *req.Spec.IdentityProvider.APIGroup } - // For now, allow there to be only exactly one IDP (until we specify a good mechanism for selecting one). - if len(matchingIDPs) != 1 { - return nil, false, ErrIndeterminateIDP + // If the IDP is unspecified (legacy requests), choose the single loaded IDP or fail if there is not exactly + // one IDP configured. + if key.Name == "" || key.Kind == "" || key.APIGroup == "" { + keys := c.Keys() + if len(keys) == 0 { + return nil, ErrNoIDPs + } + if len(keys) > 1 { + return nil, ErrIndeterminateIDP + } + key = keys[0] } - return matchingIDPs[0].AuthenticateToken(ctx, token) + val := c.Get(key) + if val == nil { + return nil, ErrNoSuchIDP + } + + // The incoming context could have an audience. Since we do not want to handle audiences right now, do not pass it + // through directly to the authentication webhook. + ctx = valuelessContext{ctx} + + // Call the selected IDP. + resp, authenticated, err := val.AuthenticateToken(ctx, req.Spec.Token) + if err != nil { + return nil, err + } + if !authenticated { + return nil, nil + } + + // Return the user.Info from the response (if it is non-nil). + var respUser user.Info + if resp != nil { + respUser = resp.User + } + return respUser, nil } + +type valuelessContext struct{ context.Context } + +func (valuelessContext) Value(interface{}) interface{} { return nil } diff --git a/internal/controller/identityprovider/idpcache/cache_test.go b/internal/controller/identityprovider/idpcache/cache_test.go index 65c1fd4e..33d156da 100644 --- a/internal/controller/identityprovider/idpcache/cache_test.go +++ b/internal/controller/identityprovider/idpcache/cache_test.go @@ -5,95 +5,212 @@ package idpcache import ( "context" + "fmt" "testing" + "time" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" - "go.pinniped.dev/internal/controllerlib" + idpv1alpha "go.pinniped.dev/generated/1.19/apis/idp/v1alpha1" + loginapi "go.pinniped.dev/generated/1.19/apis/login" "go.pinniped.dev/internal/mocks/mocktokenauthenticator" ) func TestCache(t *testing.T) { t.Parallel() - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() - tests := []struct { - name string - mockAuthenticators map[controllerlib.Key]func(*mocktokenauthenticator.MockToken) - wantResponse *authenticator.Response - wantAuthenticated bool - wantErr string - }{ - { - name: "no IDPs", - wantErr: "no identity providers are loaded", - }, - { - name: "multiple IDPs", - mockAuthenticators: map[controllerlib.Key]func(mockToken *mocktokenauthenticator.MockToken){ - controllerlib.Key{Namespace: "foo", Name: "idp-one"}: nil, - controllerlib.Key{Namespace: "foo", Name: "idp-two"}: nil, - }, - wantErr: "could not uniquely match against an identity provider", - }, - { - name: "success", - mockAuthenticators: map[controllerlib.Key]func(mockToken *mocktokenauthenticator.MockToken){ - controllerlib.Key{ - Namespace: "foo", - Name: "idp-one", - }: func(mockToken *mocktokenauthenticator.MockToken) { - mockToken.EXPECT().AuthenticateToken(ctx, "test-token").Return( - &authenticator.Response{User: &user.DefaultInfo{Name: "test-user"}}, - true, - nil, - ) - }, - }, - wantResponse: &authenticator.Response{User: &user.DefaultInfo{Name: "test-user"}}, - wantAuthenticated: true, - }, + cache := New() + require.NotNil(t, cache) + + key1 := Key{Namespace: "foo", Name: "idp-one"} + mockToken1 := mocktokenauthenticator.NewMockToken(ctrl) + cache.Store(key1, mockToken1) + require.Equal(t, mockToken1, cache.Get(key1)) + require.Equal(t, 1, len(cache.Keys())) + + key2 := Key{Namespace: "foo", Name: "idp-two"} + mockToken2 := mocktokenauthenticator.NewMockToken(ctrl) + cache.Store(key2, mockToken2) + require.Equal(t, mockToken2, cache.Get(key2)) + require.Equal(t, 2, len(cache.Keys())) + + for _, key := range cache.Keys() { + cache.Delete(key) } - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() + require.Zero(t, len(cache.Keys())) +} +func TestAuthenticateTokenCredentialRequest(t *testing.T) { + t.Parallel() + + t.Run("missing IDP selector", func(t *testing.T) { + t.Run("no IDPs", func(t *testing.T) { + c := New() + res, err := c.AuthenticateTokenCredentialRequest(context.Background(), &loginapi.TokenCredentialRequest{}) + require.EqualError(t, err, "no identity providers are loaded") + require.Nil(t, res) + }) + + t.Run("multiple IDPs", func(t *testing.T) { + c := New() + c.Store(Key{Name: "idp-one"}, nil) + c.Store(Key{Name: "idp-two"}, nil) + res, err := c.AuthenticateTokenCredentialRequest(context.Background(), &loginapi.TokenCredentialRequest{}) + require.EqualError(t, err, "could not uniquely match against an identity provider") + require.Nil(t, res) + }) + + t.Run("single IDP", func(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - cache := New() - require.NotNil(t, cache) - require.Implements(t, (*authenticator.Token)(nil), cache) + c := New() + mockToken := mocktokenauthenticator.NewMockToken(ctrl) + mockToken.EXPECT().AuthenticateToken(gomock.Any(), "test-token"). + Return(&authenticator.Response{User: &user.DefaultInfo{Name: "test-user"}}, true, nil) + c.Store(Key{Name: "idp-one"}, mockToken) - for key, mockFunc := range tt.mockAuthenticators { - mockToken := mocktokenauthenticator.NewMockToken(ctrl) - if mockFunc != nil { - mockFunc(mockToken) - } - cache.Store(key, mockToken) - } - - require.Equal(t, len(tt.mockAuthenticators), len(cache.Keys())) - - resp, authenticated, err := cache.AuthenticateToken(ctx, "test-token") - require.Equal(t, tt.wantResponse, resp) - require.Equal(t, tt.wantAuthenticated, authenticated) - if tt.wantErr != "" { - require.EqualError(t, err, tt.wantErr) - return - } + res, err := c.AuthenticateTokenCredentialRequest(context.Background(), &loginapi.TokenCredentialRequest{ + Spec: loginapi.TokenCredentialRequestSpec{Token: "test-token"}, + }) require.NoError(t, err) - - for _, key := range cache.Keys() { - cache.Delete(key) - } - require.Zero(t, len(cache.Keys())) + require.Equal(t, "test-user", res.GetName()) }) + }) + + validRequest := loginapi.TokenCredentialRequest{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-namespace", + }, + Spec: loginapi.TokenCredentialRequestSpec{ + IdentityProvider: corev1.TypedLocalObjectReference{ + APIGroup: &idpv1alpha.SchemeGroupVersion.Group, + Kind: "WebhookIdentityProvider", + Name: "test-name", + }, + Token: "test-token", + }, + Status: loginapi.TokenCredentialRequestStatus{}, } + validRequestKey := Key{ + APIGroup: *validRequest.Spec.IdentityProvider.APIGroup, + Kind: validRequest.Spec.IdentityProvider.Kind, + Namespace: validRequest.Namespace, + Name: validRequest.Spec.IdentityProvider.Name, + } + + mockCache := func(t *testing.T, res *authenticator.Response, authenticated bool, err error) *Cache { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + m := mocktokenauthenticator.NewMockToken(ctrl) + m.EXPECT().AuthenticateToken(audienceFreeContext{}, validRequest.Spec.Token).Return(res, authenticated, err) + c := New() + c.Store(validRequestKey, m) + return c + } + + t.Run("no such IDP", func(t *testing.T) { + c := New() + res, err := c.AuthenticateTokenCredentialRequest(context.Background(), validRequest.DeepCopy()) + require.EqualError(t, err, "no such identity provider") + require.Nil(t, res) + }) + + t.Run("authenticator returns error", func(t *testing.T) { + c := mockCache(t, nil, false, fmt.Errorf("some authenticator error")) + res, err := c.AuthenticateTokenCredentialRequest(context.Background(), validRequest.DeepCopy()) + require.EqualError(t, err, "some authenticator error") + require.Nil(t, res) + }) + + t.Run("authenticator returns unauthenticated without error", func(t *testing.T) { + c := mockCache(t, &authenticator.Response{}, false, nil) + res, err := c.AuthenticateTokenCredentialRequest(context.Background(), validRequest.DeepCopy()) + require.NoError(t, err) + require.Nil(t, res) + }) + + t.Run("authenticator returns nil response without error", func(t *testing.T) { + c := mockCache(t, nil, true, nil) + res, err := c.AuthenticateTokenCredentialRequest(context.Background(), validRequest.DeepCopy()) + require.NoError(t, err) + require.Nil(t, res) + }) + + t.Run("authenticator returns response with nil user", func(t *testing.T) { + c := mockCache(t, &authenticator.Response{}, true, nil) + res, err := c.AuthenticateTokenCredentialRequest(context.Background(), validRequest.DeepCopy()) + require.NoError(t, err) + require.Nil(t, res) + }) + + t.Run("context is cancelled", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + m := mocktokenauthenticator.NewMockToken(ctrl) + m.EXPECT().AuthenticateToken(gomock.Any(), validRequest.Spec.Token).DoAndReturn( + func(ctx context.Context, token string) (*authenticator.Response, bool, error) { + select { + case <-time.After(2 * time.Second): + require.Fail(t, "expected to be cancelled") + return nil, true, nil + case <-ctx.Done(): + return nil, false, ctx.Err() + } + }, + ) + c := New() + c.Store(validRequestKey, m) + + ctx, cancel := context.WithCancel(context.Background()) + errchan := make(chan error) + go func() { + _, err := c.AuthenticateTokenCredentialRequest(ctx, validRequest.DeepCopy()) + errchan <- err + }() + cancel() + require.EqualError(t, <-errchan, "context canceled") + }) + + t.Run("authenticator returns success", func(t *testing.T) { + userInfo := user.DefaultInfo{ + Name: "test-user", + UID: "test-uid", + Groups: []string{"test-group-1", "test-group-2"}, + Extra: map[string][]string{"extra-key-1": {"extra-value-1", "extra-value-2"}}, + } + c := mockCache(t, &authenticator.Response{User: &userInfo}, true, nil) + + audienceCtx := authenticator.WithAudiences(context.Background(), authenticator.Audiences{"test-audience-1"}) + res, err := c.AuthenticateTokenCredentialRequest(audienceCtx, validRequest.DeepCopy()) + require.NoError(t, err) + require.NotNil(t, res) + require.Equal(t, "test-user", res.GetName()) + require.Equal(t, "test-uid", res.GetUID()) + require.Equal(t, []string{"test-group-1", "test-group-2"}, res.GetGroups()) + require.Equal(t, map[string][]string{"extra-key-1": {"extra-value-1", "extra-value-2"}}, res.GetExtra()) + }) +} + +type audienceFreeContext struct{} + +func (audienceFreeContext) Matches(in interface{}) bool { + ctx, isCtx := in.(context.Context) + if !isCtx { + return false + } + _, hasAudiences := authenticator.AudiencesFrom(ctx) + return !hasAudiences +} + +func (audienceFreeContext) String() string { + return "is a context without authenticator audiences" } diff --git a/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner.go b/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner.go index a4d71da6..c383a18a 100644 --- a/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner.go +++ b/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner.go @@ -59,7 +59,10 @@ func (c *controller) Sync(ctx controllerlib.Context) error { // Delete any entries from the cache which are no longer in the cluster. for _, key := range c.cache.Keys() { - if _, exists := webhooksByKey[key]; !exists { + if key.APIGroup != idpv1alpha1.SchemeGroupVersion.Group || key.Kind != "WebhookIdentityProvider" { + continue + } + if _, exists := webhooksByKey[controllerlib.Key{Namespace: key.Namespace, Name: key.Name}]; !exists { c.log.WithValues("idp", klog.KRef(key.Namespace, key.Name)).Info("deleting webhook IDP from cache") c.cache.Delete(key) } diff --git a/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner_test.go b/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner_test.go index 94bc15fb..70ea0197 100644 --- a/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner_test.go +++ b/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner_test.go @@ -11,7 +11,6 @@ import ( "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apiserver/pkg/authentication/authenticator" idpv1alpha "go.pinniped.dev/generated/1.19/apis/idp/v1alpha1" pinnipedfake "go.pinniped.dev/generated/1.19/client/clientset/versioned/fake" @@ -24,22 +23,36 @@ import ( func TestController(t *testing.T) { t.Parallel() - testKey1 := controllerlib.Key{Namespace: "test-namespace", Name: "test-name-one"} - testKey2 := controllerlib.Key{Namespace: "test-namespace", Name: "test-name-two"} + testKey1 := idpcache.Key{ + APIGroup: "idp.pinniped.dev", + Kind: "WebhookIdentityProvider", + Namespace: "test-namespace", + Name: "test-name-one", + } + testKey2 := idpcache.Key{ + APIGroup: "idp.pinniped.dev", + Kind: "WebhookIdentityProvider", + Namespace: "test-namespace", + Name: "test-name-two", + } + testKeyNonwebhook := idpcache.Key{ + APIGroup: "idp.pinniped.dev", + Kind: "SomeOtherIdentityProvider", + Namespace: "test-namespace", + Name: "test-name-one", + } tests := []struct { name string - syncKey controllerlib.Key webhookIDPs []runtime.Object - initialCache map[controllerlib.Key]authenticator.Token + initialCache map[idpcache.Key]idpcache.Value wantErr string wantLogs []string - wantCacheKeys []controllerlib.Key + wantCacheKeys []idpcache.Key }{ { name: "no change", - syncKey: testKey1, - initialCache: map[controllerlib.Key]authenticator.Token{testKey1: nil}, + initialCache: map[idpcache.Key]idpcache.Value{testKey1: nil}, webhookIDPs: []runtime.Object{ &idpv1alpha.WebhookIdentityProvider{ ObjectMeta: metav1.ObjectMeta{ @@ -48,11 +61,10 @@ func TestController(t *testing.T) { }, }, }, - wantCacheKeys: []controllerlib.Key{testKey1}, + wantCacheKeys: []idpcache.Key{testKey1}, }, { name: "IDPs not yet added", - syncKey: testKey1, initialCache: nil, webhookIDPs: []runtime.Object{ &idpv1alpha.WebhookIdentityProvider{ @@ -68,14 +80,14 @@ func TestController(t *testing.T) { }, }, }, - wantCacheKeys: []controllerlib.Key{}, + wantCacheKeys: []idpcache.Key{}, }, { - name: "successful cleanup", - syncKey: testKey1, - initialCache: map[controllerlib.Key]authenticator.Token{ - testKey1: nil, - testKey2: nil, + name: "successful cleanup", + initialCache: map[idpcache.Key]idpcache.Value{ + testKey1: nil, + testKey2: nil, + testKeyNonwebhook: nil, }, webhookIDPs: []runtime.Object{ &idpv1alpha.WebhookIdentityProvider{ @@ -88,7 +100,7 @@ func TestController(t *testing.T) { wantLogs: []string{ `webhookcachecleaner-controller "level"=0 "msg"="deleting webhook IDP from cache" "idp"={"name":"test-name-two","namespace":"test-namespace"}`, }, - wantCacheKeys: []controllerlib.Key{testKey1}, + wantCacheKeys: []idpcache.Key{testKey1, testKeyNonwebhook}, }, } for _, tt := range tests { @@ -112,7 +124,13 @@ func TestController(t *testing.T) { informers.Start(ctx.Done()) controllerlib.TestRunSynchronously(t, controller) - syncCtx := controllerlib.Context{Context: ctx, Key: tt.syncKey} + syncCtx := controllerlib.Context{ + Context: ctx, + Key: controllerlib.Key{ + Namespace: "test-namespace", + Name: "test-name-one", + }, + } if err := controllerlib.TestSync(t, controller, syncCtx); tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) diff --git a/internal/controller/identityprovider/webhookcachefiller/webhookcachefiller.go b/internal/controller/identityprovider/webhookcachefiller/webhookcachefiller.go index b0343c58..e741a8b3 100644 --- a/internal/controller/identityprovider/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/identityprovider/webhookcachefiller/webhookcachefiller.go @@ -68,7 +68,12 @@ func (c *controller) Sync(ctx controllerlib.Context) error { return fmt.Errorf("failed to build webhook config: %w", err) } - c.cache.Store(ctx.Key, webhookAuthenticator) + c.cache.Store(idpcache.Key{ + APIGroup: idpv1alpha1.GroupName, + Kind: "WebhookIdentityProvider", + Namespace: ctx.Key.Namespace, + Name: ctx.Key.Name, + }, webhookAuthenticator) c.log.WithValues("idp", klog.KObj(obj), "endpoint", obj.Spec.Endpoint).Info("added new webhook IDP") return nil } diff --git a/internal/mocks/credentialrequestmocks/credentialrequestmocks.go b/internal/mocks/credentialrequestmocks/credentialrequestmocks.go new file mode 100644 index 00000000..c21e78d7 --- /dev/null +++ b/internal/mocks/credentialrequestmocks/credentialrequestmocks.go @@ -0,0 +1,96 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +// + +// Code generated by MockGen. DO NOT EDIT. +// Source: go.pinniped.dev/internal/registry/credentialrequest (interfaces: CertIssuer,TokenCredentialRequestAuthenticator) + +// Package credentialrequestmocks is a generated GoMock package. +package credentialrequestmocks + +import ( + context "context" + pkix "crypto/x509/pkix" + gomock "github.com/golang/mock/gomock" + login "go.pinniped.dev/generated/1.19/apis/login" + user "k8s.io/apiserver/pkg/authentication/user" + reflect "reflect" + time "time" +) + +// MockCertIssuer is a mock of CertIssuer interface +type MockCertIssuer struct { + ctrl *gomock.Controller + recorder *MockCertIssuerMockRecorder +} + +// MockCertIssuerMockRecorder is the mock recorder for MockCertIssuer +type MockCertIssuerMockRecorder struct { + mock *MockCertIssuer +} + +// NewMockCertIssuer creates a new mock instance +func NewMockCertIssuer(ctrl *gomock.Controller) *MockCertIssuer { + mock := &MockCertIssuer{ctrl: ctrl} + mock.recorder = &MockCertIssuerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockCertIssuer) EXPECT() *MockCertIssuerMockRecorder { + return m.recorder +} + +// IssuePEM mocks base method +func (m *MockCertIssuer) IssuePEM(arg0 pkix.Name, arg1 []string, arg2 time.Duration) ([]byte, []byte, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IssuePEM", arg0, arg1, arg2) + ret0, _ := ret[0].([]byte) + ret1, _ := ret[1].([]byte) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// IssuePEM indicates an expected call of IssuePEM +func (mr *MockCertIssuerMockRecorder) IssuePEM(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IssuePEM", reflect.TypeOf((*MockCertIssuer)(nil).IssuePEM), arg0, arg1, arg2) +} + +// MockTokenCredentialRequestAuthenticator is a mock of TokenCredentialRequestAuthenticator interface +type MockTokenCredentialRequestAuthenticator struct { + ctrl *gomock.Controller + recorder *MockTokenCredentialRequestAuthenticatorMockRecorder +} + +// MockTokenCredentialRequestAuthenticatorMockRecorder is the mock recorder for MockTokenCredentialRequestAuthenticator +type MockTokenCredentialRequestAuthenticatorMockRecorder struct { + mock *MockTokenCredentialRequestAuthenticator +} + +// NewMockTokenCredentialRequestAuthenticator creates a new mock instance +func NewMockTokenCredentialRequestAuthenticator(ctrl *gomock.Controller) *MockTokenCredentialRequestAuthenticator { + mock := &MockTokenCredentialRequestAuthenticator{ctrl: ctrl} + mock.recorder = &MockTokenCredentialRequestAuthenticatorMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockTokenCredentialRequestAuthenticator) EXPECT() *MockTokenCredentialRequestAuthenticatorMockRecorder { + return m.recorder +} + +// AuthenticateTokenCredentialRequest mocks base method +func (m *MockTokenCredentialRequestAuthenticator) AuthenticateTokenCredentialRequest(arg0 context.Context, arg1 *login.TokenCredentialRequest) (user.Info, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AuthenticateTokenCredentialRequest", arg0, arg1) + ret0, _ := ret[0].(user.Info) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// AuthenticateTokenCredentialRequest indicates an expected call of AuthenticateTokenCredentialRequest +func (mr *MockTokenCredentialRequestAuthenticatorMockRecorder) AuthenticateTokenCredentialRequest(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AuthenticateTokenCredentialRequest", reflect.TypeOf((*MockTokenCredentialRequestAuthenticator)(nil).AuthenticateTokenCredentialRequest), arg0, arg1) +} diff --git a/internal/mocks/credentialrequestmocks/generate.go b/internal/mocks/credentialrequestmocks/generate.go new file mode 100644 index 00000000..c22ec978 --- /dev/null +++ b/internal/mocks/credentialrequestmocks/generate.go @@ -0,0 +1,6 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package credentialrequestmocks + +//go:generate go run -v github.com/golang/mock/mockgen -destination=credentialrequestmocks.go -package=credentialrequestmocks -copyright_file=../../../hack/header.txt go.pinniped.dev/internal/registry/credentialrequest CertIssuer,TokenCredentialRequestAuthenticator diff --git a/internal/mocks/mockcertissuer/generate.go b/internal/mocks/mockcertissuer/generate.go deleted file mode 100644 index 82e74840..00000000 --- a/internal/mocks/mockcertissuer/generate.go +++ /dev/null @@ -1,6 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package mockcertissuer - -//go:generate go run -v github.com/golang/mock/mockgen -destination=mockcertissuer.go -package=mockcertissuer -copyright_file=../../../hack/header.txt go.pinniped.dev/internal/registry/credentialrequest CertIssuer diff --git a/internal/mocks/mockcertissuer/mockcertissuer.go b/internal/mocks/mockcertissuer/mockcertissuer.go deleted file mode 100644 index e68383c1..00000000 --- a/internal/mocks/mockcertissuer/mockcertissuer.go +++ /dev/null @@ -1,56 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 -// - -// Code generated by MockGen. DO NOT EDIT. -// Source: go.pinniped.dev/internal/registry/credentialrequest (interfaces: CertIssuer) - -// Package mockcertissuer is a generated GoMock package. -package mockcertissuer - -import ( - pkix "crypto/x509/pkix" - reflect "reflect" - time "time" - - gomock "github.com/golang/mock/gomock" -) - -// MockCertIssuer is a mock of CertIssuer interface -type MockCertIssuer struct { - ctrl *gomock.Controller - recorder *MockCertIssuerMockRecorder -} - -// MockCertIssuerMockRecorder is the mock recorder for MockCertIssuer -type MockCertIssuerMockRecorder struct { - mock *MockCertIssuer -} - -// NewMockCertIssuer creates a new mock instance -func NewMockCertIssuer(ctrl *gomock.Controller) *MockCertIssuer { - mock := &MockCertIssuer{ctrl: ctrl} - mock.recorder = &MockCertIssuerMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use -func (m *MockCertIssuer) EXPECT() *MockCertIssuerMockRecorder { - return m.recorder -} - -// IssuePEM mocks base method -func (m *MockCertIssuer) IssuePEM(arg0 pkix.Name, arg1 []string, arg2 time.Duration) ([]byte, []byte, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "IssuePEM", arg0, arg1, arg2) - ret0, _ := ret[0].([]byte) - ret1, _ := ret[1].([]byte) - ret2, _ := ret[2].(error) - return ret0, ret1, ret2 -} - -// IssuePEM indicates an expected call of IssuePEM -func (mr *MockCertIssuerMockRecorder) IssuePEM(arg0, arg1, arg2 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IssuePEM", reflect.TypeOf((*MockCertIssuer)(nil).IssuePEM), arg0, arg1, arg2) -} diff --git a/internal/registry/credentialrequest/rest.go b/internal/registry/credentialrequest/rest.go index f8ccafb0..b3e31eab 100644 --- a/internal/registry/credentialrequest/rest.go +++ b/internal/registry/credentialrequest/rest.go @@ -14,7 +14,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/apiserver/pkg/authentication/authenticator" + "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/utils/trace" @@ -35,16 +35,20 @@ type CertIssuer interface { IssuePEM(subject pkix.Name, dnsNames []string, ttl time.Duration) ([]byte, []byte, error) } -func NewREST(tokenAuthenticator authenticator.Token, issuer CertIssuer) *REST { +type TokenCredentialRequestAuthenticator interface { + AuthenticateTokenCredentialRequest(ctx context.Context, req *loginapi.TokenCredentialRequest) (user.Info, error) +} + +func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer CertIssuer) *REST { return &REST{ - tokenAuthenticator: tokenAuthenticator, - issuer: issuer, + authenticator: authenticator, + issuer: issuer, } } type REST struct { - tokenAuthenticator authenticator.Token - issuer CertIssuer + authenticator TokenCredentialRequestAuthenticator + issuer CertIssuer } func (*REST) New() runtime.Object { @@ -67,35 +71,20 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation return nil, err } - // The incoming context could have an audience. Since we do not want to handle audiences right now, do not pass it - // through directly to the authentication webhook. Instead only propagate cancellation of the parent context. - cancelCtx, cancel := context.WithCancel(context.Background()) - defer cancel() - go func() { - select { - case <-ctx.Done(): - cancel() - case <-cancelCtx.Done(): - } - }() - - authResponse, authenticated, err := r.tokenAuthenticator.AuthenticateToken(cancelCtx, credentialRequest.Spec.Token) + user, err := r.authenticator.AuthenticateTokenCredentialRequest(ctx, credentialRequest) if err != nil { traceFailureWithError(t, "webhook authentication", err) return failureResponse(), nil } - if !authenticated || authResponse == nil || authResponse.User == nil || authResponse.User.GetName() == "" { - traceSuccess(t, authResponse, authenticated, false) + if user == nil || user.GetName() == "" { + traceSuccess(t, user, false) return failureResponse(), nil } - username := authResponse.User.GetName() - groups := authResponse.User.GetGroups() - certPEM, keyPEM, err := r.issuer.IssuePEM( pkix.Name{ - CommonName: username, - Organization: groups, + CommonName: user.GetName(), + Organization: user.GetGroups(), }, []string{}, clientCertificateTTL, @@ -105,7 +94,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation return failureResponse(), nil } - traceSuccess(t, authResponse, authenticated, true) + traceSuccess(t, user, true) return &loginapi.TokenCredentialRequest{ Status: loginapi.TokenCredentialRequestStatus{ @@ -121,8 +110,8 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation func validateRequest(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions, t *trace.Trace) (*loginapi.TokenCredentialRequest, error) { credentialRequest, ok := obj.(*loginapi.TokenCredentialRequest) if !ok { - traceValidationFailure(t, "not a CredentialRequest") - return nil, apierrors.NewBadRequest(fmt.Sprintf("not a CredentialRequest: %#v", obj)) + traceValidationFailure(t, "not a TokenCredentialRequest") + return nil, apierrors.NewBadRequest(fmt.Sprintf("not a TokenCredentialRequest: %#v", obj)) } if len(credentialRequest.Spec.Token) == 0 { @@ -157,15 +146,14 @@ func validateRequest(ctx context.Context, obj runtime.Object, createValidation r return credentialRequest, nil } -func traceSuccess(t *trace.Trace, response *authenticator.Response, webhookAuthenticated bool, pinnipedAuthenticated bool) { +func traceSuccess(t *trace.Trace, userInfo user.Info, authenticated bool) { userID := "" - if response != nil && response.User != nil { - userID = response.User.GetUID() + if userInfo != nil { + userID = userInfo.GetUID() } t.Step("success", trace.Field{Key: "userID", Value: userID}, - trace.Field{Key: "idpAuthenticated", Value: webhookAuthenticated}, - trace.Field{Key: "pinnipedAuthenticated", Value: pinnipedAuthenticated}, + trace.Field{Key: "authenticated", Value: authenticated}, ) } diff --git a/internal/registry/credentialrequest/rest_test.go b/internal/registry/credentialrequest/rest_test.go index efa3071a..e44eae1b 100644 --- a/internal/registry/credentialrequest/rest_test.go +++ b/internal/registry/credentialrequest/rest_test.go @@ -17,45 +17,21 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/klog/v2" loginapi "go.pinniped.dev/generated/1.19/apis/login" - "go.pinniped.dev/internal/mocks/mockcertissuer" + "go.pinniped.dev/internal/mocks/credentialrequestmocks" "go.pinniped.dev/internal/testutil" ) -type contextKey struct{} - -type FakeToken struct { - calledWithToken string - calledWithContext context.Context - timeout time.Duration - reachedTimeout bool - cancelled bool - webhookStartedRunningNotificationChan chan bool - returnResponse *authenticator.Response - returnUnauthenticated bool - returnErr error -} - -func (f *FakeToken) AuthenticateToken(ctx context.Context, token string) (*authenticator.Response, bool, error) { - f.calledWithToken = token - f.calledWithContext = ctx - if f.webhookStartedRunningNotificationChan != nil { - f.webhookStartedRunningNotificationChan <- true - } - afterCh := time.After(f.timeout) - select { - case <-afterCh: - f.reachedTimeout = true - case <-ctx.Done(): - f.cancelled = true - } - return f.returnResponse, !f.returnUnauthenticated, f.returnErr +func TestNew(t *testing.T) { + r := NewREST(nil, nil) + require.NotNil(t, r) + require.True(t, r.NamespaceScoped()) + require.IsType(t, &loginapi.TokenCredentialRequest{}, r.New()) } func TestCreate(t *testing.T) { @@ -77,18 +53,17 @@ func TestCreate(t *testing.T) { }) it("CreateSucceedsWhenGivenATokenAndTheWebhookAuthenticatesTheToken", func() { - webhook := FakeToken{ - returnResponse: &authenticator.Response{ - User: &user.DefaultInfo{ - Name: "test-user", - UID: "test-user-uid", - Groups: []string{"test-group-1", "test-group-2"}, - }, - }, - returnUnauthenticated: false, - } + req := validCredentialRequest() - issuer := mockcertissuer.NewMockCertIssuer(ctrl) + requestAuthenticator := credentialrequestmocks.NewMockTokenCredentialRequestAuthenticator(ctrl) + requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). + Return(&user.DefaultInfo{ + Name: "test-user", + UID: "test-user-uid", + Groups: []string{"test-group-1", "test-group-2"}, + }, nil) + + issuer := credentialrequestmocks.NewMockCertIssuer(ctrl) issuer.EXPECT().IssuePEM( pkix.Name{ CommonName: "test-user", @@ -97,10 +72,9 @@ func TestCreate(t *testing.T) { 1*time.Hour, ).Return([]byte("test-cert"), []byte("test-key"), nil) - storage := NewREST(&webhook, issuer) - requestToken := "a token" + storage := NewREST(requestAuthenticator, issuer) - response, err := callCreate(context.Background(), storage, validCredentialRequestWithToken(requestToken)) + response, err := callCreate(context.Background(), storage, req) r.NoError(err) r.IsType(&loginapi.TokenCredentialRequest{}, response) @@ -119,203 +93,89 @@ func TestCreate(t *testing.T) { }, }, }) - r.Equal(requestToken, webhook.calledWithToken) - requireOneLogStatement(r, logger, `"success" userID:test-user-uid,idpAuthenticated:true`) - }) - - it("CreateSucceedsWhenGivenANewLoginAPITokenAndTheWebhookAuthenticatesTheToken", func() { - webhook := FakeToken{ - returnResponse: &authenticator.Response{ - User: &user.DefaultInfo{ - Name: "test-user", - UID: "test-user-uid", - Groups: []string{"test-group-1", "test-group-2"}, - }, - }, - returnUnauthenticated: false, - } - - issuer := mockcertissuer.NewMockCertIssuer(ctrl) - issuer.EXPECT().IssuePEM( - pkix.Name{ - CommonName: "test-user", - Organization: []string{"test-group-1", "test-group-2"}}, - []string{}, - 1*time.Hour, - ).Return([]byte("test-cert"), []byte("test-key"), nil) - - storage := NewREST(&webhook, issuer) - requestToken := "a token" - - response, err := callCreate(context.Background(), storage, &loginapi.TokenCredentialRequest{ - TypeMeta: metav1.TypeMeta{}, - ObjectMeta: metav1.ObjectMeta{ - Name: "request name", - }, - Spec: loginapi.TokenCredentialRequestSpec{ - Token: requestToken, - }, - }) - - r.NoError(err) - r.IsType(&loginapi.TokenCredentialRequest{}, response) - - expires := response.(*loginapi.TokenCredentialRequest).Status.Credential.ExpirationTimestamp - r.NotNil(expires) - r.InDelta(time.Now().Add(1*time.Hour).Unix(), expires.Unix(), 5) - response.(*loginapi.TokenCredentialRequest).Status.Credential.ExpirationTimestamp = metav1.Time{} - - r.Equal(response, &loginapi.TokenCredentialRequest{ - Status: loginapi.TokenCredentialRequestStatus{ - Credential: &loginapi.ClusterCredential{ - ExpirationTimestamp: metav1.Time{}, - ClientCertificateData: "test-cert", - ClientKeyData: "test-key", - }, - }, - }) - r.Equal(requestToken, webhook.calledWithToken) - requireOneLogStatement(r, logger, `"success" userID:test-user-uid,idpAuthenticated:true`) + requireOneLogStatement(r, logger, `"success" userID:test-user-uid,authenticated:true`) }) it("CreateFailsWithValidTokenWhenCertIssuerFails", func() { - webhook := FakeToken{ - returnResponse: &authenticator.Response{ - User: &user.DefaultInfo{ - Name: "test-user", - Groups: []string{"test-group-1", "test-group-2"}, - }, - }, - returnUnauthenticated: false, - } + req := validCredentialRequest() - issuer := mockcertissuer.NewMockCertIssuer(ctrl) + requestAuthenticator := credentialrequestmocks.NewMockTokenCredentialRequestAuthenticator(ctrl) + requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). + Return(&user.DefaultInfo{ + Name: "test-user", + Groups: []string{"test-group-1", "test-group-2"}, + }, nil) + + issuer := credentialrequestmocks.NewMockCertIssuer(ctrl) issuer.EXPECT(). IssuePEM(gomock.Any(), gomock.Any(), gomock.Any()). Return(nil, nil, fmt.Errorf("some certificate authority error")) - storage := NewREST(&webhook, issuer) - requestToken := "a token" + storage := NewREST(requestAuthenticator, issuer) - response, err := callCreate(context.Background(), storage, validCredentialRequestWithToken(requestToken)) + response, err := callCreate(context.Background(), storage, req) requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) - r.Equal(requestToken, webhook.calledWithToken) requireOneLogStatement(r, logger, `"failure" failureType:cert issuer,msg:some certificate authority error`) }) - it("CreateSucceedsWithAnUnauthenticatedStatusWhenGivenATokenAndTheWebhookReturnsUnauthenticatedWithUserId", func() { - webhook := FakeToken{ - returnResponse: &authenticator.Response{ - User: &user.DefaultInfo{UID: "test-user-uid"}, - }, - returnUnauthenticated: true, - } - storage := NewREST(&webhook, nil) - requestToken := "a token" + it("CreateSucceedsWithAnUnauthenticatedStatusWhenGivenATokenAndTheWebhookReturnsNilUser", func() { + req := validCredentialRequest() - response, err := callCreate(context.Background(), storage, validCredentialRequestWithToken(requestToken)) + requestAuthenticator := credentialrequestmocks.NewMockTokenCredentialRequestAuthenticator(ctrl) + requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req).Return(nil, nil) + + storage := NewREST(requestAuthenticator, nil) + + response, err := callCreate(context.Background(), storage, req) requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) - r.Equal(requestToken, webhook.calledWithToken) - requireOneLogStatement(r, logger, `"success" userID:test-user-uid,idpAuthenticated:false,pinnipedAuthenticated:false`) - }) - - it("CreateSucceedsWithAnUnauthenticatedStatusWhenGivenATokenAndTheWebhookReturnsUnauthenticatedWithNilUser", func() { - webhook := FakeToken{ - returnResponse: &authenticator.Response{User: nil}, - returnUnauthenticated: true, - } - storage := NewREST(&webhook, nil) - requestToken := "a token" - - response, err := callCreate(context.Background(), storage, validCredentialRequestWithToken(requestToken)) - - requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) - r.Equal(requestToken, webhook.calledWithToken) - requireOneLogStatement(r, logger, `"success" userID:,idpAuthenticated:false,pinnipedAuthenticated:false`) + requireOneLogStatement(r, logger, `"success" userID:,authenticated:false`) }) it("CreateSucceedsWithAnUnauthenticatedStatusWhenWebhookFails", func() { - webhook := FakeToken{ - returnErr: errors.New("some webhook error"), - } - storage := NewREST(&webhook, nil) + req := validCredentialRequest() - response, err := callCreate(context.Background(), storage, validCredentialRequest()) + requestAuthenticator := credentialrequestmocks.NewMockTokenCredentialRequestAuthenticator(ctrl) + requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). + Return(nil, errors.New("some webhook error")) + + storage := NewREST(requestAuthenticator, nil) + + response, err := callCreate(context.Background(), storage, req) requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) requireOneLogStatement(r, logger, `"failure" failureType:webhook authentication,msg:some webhook error`) }) - it("CreateSucceedsWithAnUnauthenticatedStatusWhenWebhookReturnsNilResponseWithAuthenticatedFalse", func() { - webhook := FakeToken{ - returnResponse: nil, - returnUnauthenticated: false, - } - storage := NewREST(&webhook, nil) - - response, err := callCreate(context.Background(), storage, validCredentialRequest()) - - requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) - requireOneLogStatement(r, logger, `"success" userID:,idpAuthenticated:true,pinnipedAuthenticated:false`) - }) - - it("CreateSucceedsWithAnUnauthenticatedStatusWhenWebhookDoesNotReturnAnyUserInfo", func() { - webhook := FakeToken{ - returnResponse: &authenticator.Response{}, - returnUnauthenticated: false, - } - storage := NewREST(&webhook, nil) - - response, err := callCreate(context.Background(), storage, validCredentialRequest()) - - requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) - requireOneLogStatement(r, logger, `"success" userID:,idpAuthenticated:true,pinnipedAuthenticated:false`) - }) - it("CreateSucceedsWithAnUnauthenticatedStatusWhenWebhookReturnsAnEmptyUsername", func() { - webhook := FakeToken{ - returnResponse: &authenticator.Response{ - User: &user.DefaultInfo{ - Name: "", - }, - }, - } - storage := NewREST(&webhook, nil) + req := validCredentialRequest() - response, err := callCreate(context.Background(), storage, validCredentialRequest()) + requestAuthenticator := credentialrequestmocks.NewMockTokenCredentialRequestAuthenticator(ctrl) + requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). + Return(&user.DefaultInfo{Name: ""}, nil) + + storage := NewREST(requestAuthenticator, nil) + + response, err := callCreate(context.Background(), storage, req) requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) - requireOneLogStatement(r, logger, `"success" userID:,idpAuthenticated:true,pinnipedAuthenticated:false`) - }) - - it("CreateDoesNotPassAdditionalContextInfoToTheWebhook", func() { - webhook := FakeToken{ - returnResponse: webhookSuccessResponse(), - } - storage := NewREST(&webhook, successfulIssuer(ctrl)) - ctx := context.WithValue(context.Background(), contextKey{}, "context-value") - - _, err := callCreate(ctx, storage, validCredentialRequest()) - - r.NoError(err) - r.Nil(webhook.calledWithContext.Value("context-key")) + requireOneLogStatement(r, logger, `"success" userID:,authenticated:false`) }) it("CreateFailsWhenGivenTheWrongInputType", func() { notACredentialRequest := runtime.Unknown{} - response, err := NewREST(&FakeToken{}, nil).Create( + response, err := NewREST(nil, nil).Create( genericapirequest.NewContext(), ¬ACredentialRequest, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) - requireAPIError(t, response, err, apierrors.IsBadRequest, "not a CredentialRequest") - requireOneLogStatement(r, logger, `"failure" failureType:request validation,msg:not a CredentialRequest`) + requireAPIError(t, response, err, apierrors.IsBadRequest, "not a TokenCredentialRequest") + requireOneLogStatement(r, logger, `"failure" failureType:request validation,msg:not a TokenCredentialRequest`) }) it("CreateFailsWhenTokenValueIsEmptyInRequest", func() { - storage := NewREST(&FakeToken{}, nil) + storage := NewREST(nil, nil) response, err := callCreate(context.Background(), storage, credentialRequest(loginapi.TokenCredentialRequestSpec{ Token: "", })) @@ -326,7 +186,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenValidationFails", func() { - storage := NewREST(&FakeToken{}, nil) + storage := NewREST(nil, nil) response, err := storage.Create( context.Background(), validCredentialRequest(), @@ -340,14 +200,16 @@ func TestCreate(t *testing.T) { }) it("CreateDoesNotAllowValidationFunctionToMutateRequest", func() { - webhook := FakeToken{ - returnResponse: webhookSuccessResponse(), - } - storage := NewREST(&webhook, successfulIssuer(ctrl)) - requestToken := "a token" + req := validCredentialRequest() + + requestAuthenticator := credentialrequestmocks.NewMockTokenCredentialRequestAuthenticator(ctrl) + requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req.DeepCopy()). + Return(&user.DefaultInfo{Name: "test-user"}, nil) + + storage := NewREST(requestAuthenticator, successfulIssuer(ctrl)) response, err := storage.Create( context.Background(), - validCredentialRequestWithToken(requestToken), + req, func(ctx context.Context, obj runtime.Object) error { credentialRequest, _ := obj.(*loginapi.TokenCredentialRequest) credentialRequest.Spec.Token = "foobaz" @@ -356,20 +218,21 @@ func TestCreate(t *testing.T) { &metav1.CreateOptions{}) r.NoError(err) r.NotEmpty(response) - r.Equal(requestToken, webhook.calledWithToken) // i.e. not called with foobaz }) it("CreateDoesNotAllowValidationFunctionToSeeTheActualRequestToken", func() { - webhook := FakeToken{ - returnResponse: webhookSuccessResponse(), - } + req := validCredentialRequest() - storage := NewREST(&webhook, successfulIssuer(ctrl)) + requestAuthenticator := credentialrequestmocks.NewMockTokenCredentialRequestAuthenticator(ctrl) + requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req.DeepCopy()). + Return(&user.DefaultInfo{Name: "test-user"}, nil) + + storage := NewREST(requestAuthenticator, successfulIssuer(ctrl)) validationFunctionWasCalled := false var validationFunctionSawTokenValue string response, err := storage.Create( context.Background(), - validCredentialRequest(), + req, func(ctx context.Context, obj runtime.Object) error { credentialRequest, _ := obj.(*loginapi.TokenCredentialRequest) validationFunctionWasCalled = true @@ -384,7 +247,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenRequestOptionsDryRunIsNotEmpty", func() { - response, err := NewREST(&FakeToken{}, nil).Create( + response, err := NewREST(nil, nil).Create( genericapirequest.NewContext(), validCredentialRequest(), rest.ValidateAllObjectFunc, @@ -396,60 +259,6 @@ func TestCreate(t *testing.T) { `.pinniped.dev "request name" is invalid: dryRun: Unsupported value: []string{"some dry run flag"}`) requireOneLogStatement(r, logger, `"failure" failureType:request validation,msg:dryRun not supported`) }) - - it("CreateCancelsTheWebhookInvocationWhenTheCallToCreateIsCancelledItself", func() { - webhookStartedRunningNotificationChan := make(chan bool) - webhook := FakeToken{ - timeout: time.Second * 2, - webhookStartedRunningNotificationChan: webhookStartedRunningNotificationChan, - returnResponse: webhookSuccessResponse(), - } - storage := NewREST(&webhook, successfulIssuer(ctrl)) - ctx, cancel := context.WithCancel(context.Background()) - - c := make(chan bool) - go func() { - _, err := callCreate(ctx, storage, validCredentialRequest()) - c <- true - r.NoError(err) - }() - - r.False(webhook.cancelled) - r.False(webhook.reachedTimeout) - <-webhookStartedRunningNotificationChan // wait long enough to make sure that the webhook has started - cancel() // cancel the context that was passed to storage.Create() above - <-c // wait for the above call to storage.Create() to be finished - r.True(webhook.cancelled) - r.False(webhook.reachedTimeout) - r.Equal(context.Canceled, webhook.calledWithContext.Err()) // the inner context is cancelled - }) - - it("CreateAllowsTheWebhookInvocationToFinishWhenTheCallToCreateIsNotCancelledItself", func() { - webhookStartedRunningNotificationChan := make(chan bool) - webhook := FakeToken{ - timeout: 0, - webhookStartedRunningNotificationChan: webhookStartedRunningNotificationChan, - returnResponse: webhookSuccessResponse(), - } - storage := NewREST(&webhook, successfulIssuer(ctrl)) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - c := make(chan bool) - go func() { - _, err := callCreate(ctx, storage, validCredentialRequest()) - c <- true - r.NoError(err) - }() - - r.False(webhook.cancelled) - r.False(webhook.reachedTimeout) - <-webhookStartedRunningNotificationChan // wait long enough to make sure that the webhook has started - <-c // wait for the above call to storage.Create() to be finished - r.False(webhook.cancelled) - r.True(webhook.reachedTimeout) - r.Equal(context.Canceled, webhook.calledWithContext.Err()) // the inner context is cancelled (in this case by the "defer") - }) }, spec.Sequential()) } @@ -488,15 +297,6 @@ func credentialRequest(spec loginapi.TokenCredentialRequestSpec) *loginapi.Token } } -func webhookSuccessResponse() *authenticator.Response { - return &authenticator.Response{User: &user.DefaultInfo{ - Name: "some-user", - UID: "", - Groups: []string{}, - Extra: nil, - }} -} - func requireAPIError(t *testing.T, response runtime.Object, err error, expectedErrorTypeChecker func(err error) bool, expectedErrorMessage string) { t.Helper() require.Nil(t, response) @@ -518,7 +318,7 @@ func requireSuccessfulResponseWithAuthenticationFailureMessage(t *testing.T, err } func successfulIssuer(ctrl *gomock.Controller) CertIssuer { - issuer := mockcertissuer.NewMockCertIssuer(ctrl) + issuer := credentialrequestmocks.NewMockCertIssuer(ctrl) issuer.EXPECT(). IssuePEM(gomock.Any(), gomock.Any(), gomock.Any()). Return([]byte("test-cert"), []byte("test-key"), nil) diff --git a/internal/server/server.go b/internal/server/server.go index 1420182d..6839c424 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -12,7 +12,6 @@ import ( "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apiserver/pkg/authentication/authenticator" genericapiserver "k8s.io/apiserver/pkg/server" genericoptions "k8s.io/apiserver/pkg/server/options" "k8s.io/client-go/kubernetes" @@ -246,8 +245,8 @@ func getClusterCASigner( // Create a configuration for the aggregated API server. func getAggregatedAPIServerConfig( dynamicCertProvider provider.DynamicTLSServingCertProvider, - tokenAuthenticator authenticator.Token, - ca credentialrequest.CertIssuer, + authenticator credentialrequest.TokenCredentialRequestAuthenticator, + issuer credentialrequest.CertIssuer, startControllersPostStartHook func(context.Context), ) (*apiserver.Config, error) { recommendedOptions := genericoptions.NewRecommendedOptions( @@ -275,8 +274,8 @@ func getAggregatedAPIServerConfig( apiServerConfig := &apiserver.Config{ GenericConfig: serverConfig, ExtraConfig: apiserver.ExtraConfig{ - TokenAuthenticator: tokenAuthenticator, - Issuer: ca, + Authenticator: authenticator, + Issuer: issuer, StartControllersPostStartHook: startControllersPostStartHook, }, }