From 6b71b8d8ad0158de9d81a92bbb954711acdb4d16 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 9 Feb 2021 15:51:35 -0500 Subject: [PATCH 1/2] Revert server side token credential request API group changes Signed-off-by: Monis Khan --- internal/concierge/server/server.go | 2 +- .../authenticator/authncache/cache.go | 15 ++-- .../authenticator/authncache/cache_test.go | 70 ++++--------------- .../cachecleaner/cachecleaner_test.go | 2 +- .../jwtcachefiller/jwtcachefiller_test.go | 2 +- .../webhookcachefiller_test.go | 2 +- 6 files changed, 20 insertions(+), 73 deletions(-) diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index a27aeb724..584a84b61 100644 --- a/internal/concierge/server/server.go +++ b/internal/concierge/server/server.go @@ -110,7 +110,7 @@ func (a *App) runServer(ctx context.Context) error { } // Initialize the cache of active authenticators. - authenticators := authncache.New(*cfg.APIGroupSuffix) + authenticators := authncache.New() // This cert provider will provide certs to the API server and will // be mutated by a controller to keep the certs up to date with what diff --git a/internal/controller/authenticator/authncache/cache.go b/internal/controller/authenticator/authncache/cache.go index 7d629eb52..87f317dff 100644 --- a/internal/controller/authenticator/authncache/cache.go +++ b/internal/controller/authenticator/authncache/cache.go @@ -15,7 +15,6 @@ import ( "k8s.io/klog/v2" loginapi "go.pinniped.dev/generated/1.20/apis/concierge/login" - "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/plog" ) @@ -27,8 +26,7 @@ var ( // Cache implements the authenticator.Token interface by multiplexing across a dynamic set of authenticators // loaded from authenticator resources. type Cache struct { - cache sync.Map - apiGroupSuffix string + cache sync.Map } type Key struct { @@ -43,8 +41,8 @@ type Value interface { } // New returns an empty cache. -func New(apiGroupSuffix string) *Cache { - return &Cache{apiGroupSuffix: apiGroupSuffix} +func New() *Cache { + return &Cache{} } // Get an authenticator by key. @@ -92,12 +90,7 @@ func (c *Cache) AuthenticateTokenCredentialRequest(ctx context.Context, req *log Kind: req.Spec.Authenticator.Kind, } if req.Spec.Authenticator.APIGroup != nil { - // The key must always be API group pinniped.dev because that's what the cache filler will always use. - apiGroup, replaced := groupsuffix.Unreplace(*req.Spec.Authenticator.APIGroup, c.apiGroupSuffix) - if !replaced { - return nil, ErrNoSuchAuthenticator - } - key.APIGroup = apiGroup + key.APIGroup = *req.Spec.Authenticator.APIGroup } val := c.Get(key) diff --git a/internal/controller/authenticator/authncache/cache_test.go b/internal/controller/authenticator/authncache/cache_test.go index fdb0fd039..d48a1951e 100644 --- a/internal/controller/authenticator/authncache/cache_test.go +++ b/internal/controller/authenticator/authncache/cache_test.go @@ -28,7 +28,7 @@ func TestCache(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - cache := New("pinniped.dev") + cache := New() require.NotNil(t, cache) key1 := Key{Namespace: "foo", Name: "authenticator-one"} @@ -57,7 +57,7 @@ func TestCache(t *testing.T) { {APIGroup: "b", Kind: "b", Namespace: "b", Name: "b"}, } for tries := 0; tries < 10; tries++ { - cache := New("pinniped.dev") + cache := New() for _, i := range rand.Perm(len(keysInExpectedOrder)) { cache.Store(keysInExpectedOrder[i], nil) } @@ -91,48 +91,46 @@ func TestAuthenticateTokenCredentialRequest(t *testing.T) { Name: validRequest.Spec.Authenticator.Name, } - mockCache := func(t *testing.T, apiGroupSuffix string, expectAuthenticatorToBeCalled bool, res *authenticator.Response, authenticated bool, err error) *Cache { + 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) - if expectAuthenticatorToBeCalled { - m.EXPECT().AuthenticateToken(audienceFreeContext{}, validRequest.Spec.Token).Return(res, authenticated, err) - } - c := New(apiGroupSuffix) + m.EXPECT().AuthenticateToken(audienceFreeContext{}, validRequest.Spec.Token).Return(res, authenticated, err) + c := New() c.Store(validRequestKey, m) return c } t.Run("no such authenticator", func(t *testing.T) { - c := New("pinniped.dev") + c := New() res, err := c.AuthenticateTokenCredentialRequest(context.Background(), validRequest.DeepCopy()) require.EqualError(t, err, "no such authenticator") require.Nil(t, res) }) t.Run("authenticator returns error", func(t *testing.T) { - c := mockCache(t, "pinniped.dev", true, nil, false, fmt.Errorf("some authenticator error")) + 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, "pinniped.dev", true, &authenticator.Response{}, false, nil) + 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, "pinniped.dev", true, nil, true, nil) + 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, "pinniped.dev", true, &authenticator.Response{}, true, nil) + c := mockCache(t, &authenticator.Response{}, true, nil) res, err := c.AuthenticateTokenCredentialRequest(context.Background(), validRequest.DeepCopy()) require.NoError(t, err) require.Nil(t, res) @@ -153,7 +151,7 @@ func TestAuthenticateTokenCredentialRequest(t *testing.T) { } }, ) - c := New("pinniped.dev") + c := New() c.Store(validRequestKey, m) ctx, cancel := context.WithCancel(context.Background()) @@ -173,7 +171,7 @@ func TestAuthenticateTokenCredentialRequest(t *testing.T) { Groups: []string{"test-group-1", "test-group-2"}, Extra: map[string][]string{"extra-key-1": {"extra-value-1", "extra-value-2"}}, } - c := mockCache(t, "pinniped.dev", true, &authenticator.Response{User: &userInfo}, true, nil) + 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()) @@ -184,50 +182,6 @@ func TestAuthenticateTokenCredentialRequest(t *testing.T) { 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()) }) - - t.Run("using a non-default API group suffix still performs the cache lookup using the pinniped.dev suffix", func(t *testing.T) { - authenticationGroupWithCustomSuffix := "authentication.concierge.custom-suffix.com" - validRequestForAlternateAPIGroup := loginapi.TokenCredentialRequest{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test-namespace", - }, - Spec: loginapi.TokenCredentialRequestSpec{ - Authenticator: corev1.TypedLocalObjectReference{ - APIGroup: &authenticationGroupWithCustomSuffix, - Kind: "WebhookAuthenticator", - Name: "test-name", - }, - Token: "test-token", - }, - Status: loginapi.TokenCredentialRequestStatus{}, - } - - 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, "custom-suffix.com", true, &authenticator.Response{User: &userInfo}, true, nil) - - audienceCtx := authenticator.WithAudiences(context.Background(), authenticator.Audiences{"test-audience-1"}) - res, err := c.AuthenticateTokenCredentialRequest(audienceCtx, validRequestForAlternateAPIGroup.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()) - }) - - t.Run("using a non-default API group suffix and the incoming request mentions a different API group, results in no such authenticator", func(t *testing.T) { - c := mockCache(t, "custom-suffix.com", false, &authenticator.Response{User: &user.DefaultInfo{Name: "someone"}}, true, nil) - - // Note that the validRequest.Spec.Authenticator.APIGroup value uses "pinniped.dev", not "custom-suffix.com" - res, err := c.AuthenticateTokenCredentialRequest(context.Background(), validRequest.DeepCopy()) - require.EqualError(t, err, "no such authenticator") - require.Nil(t, res) - }) } type audienceFreeContext struct{} diff --git a/internal/controller/authenticator/cachecleaner/cachecleaner_test.go b/internal/controller/authenticator/cachecleaner/cachecleaner_test.go index e4a9cad62..a735900b4 100644 --- a/internal/controller/authenticator/cachecleaner/cachecleaner_test.go +++ b/internal/controller/authenticator/cachecleaner/cachecleaner_test.go @@ -153,7 +153,7 @@ func TestController(t *testing.T) { fakeClient := pinnipedfake.NewSimpleClientset(tt.objects...) informers := pinnipedinformers.NewSharedInformerFactory(fakeClient, 0) - cache := authncache.New("pinniped.dev") + cache := authncache.New() if tt.initialCache != nil { tt.initialCache(t, cache) } diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index b1aee0d8b..17b8073d7 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go @@ -327,7 +327,7 @@ func TestController(t *testing.T) { fakeClient := pinnipedfake.NewSimpleClientset(tt.jwtAuthenticators...) informers := pinnipedinformers.NewSharedInformerFactory(fakeClient, 0) - cache := authncache.New("pinniped.dev") + cache := authncache.New() testLog := testlogger.New(t) if tt.cache != nil { diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index 27dd16371..6b0bec0b0 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -90,7 +90,7 @@ func TestController(t *testing.T) { fakeClient := pinnipedfake.NewSimpleClientset(tt.webhooks...) informers := pinnipedinformers.NewSharedInformerFactory(fakeClient, 0) - cache := authncache.New("pinniped.dev") + cache := authncache.New() testLog := testlogger.New(t) controller := New(cache, informers.Authentication().V1alpha1().WebhookAuthenticators(), testLog) From 2679d27ced0de507ce0b10f2da71bfcd1647d747 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 9 Feb 2021 15:51:38 -0500 Subject: [PATCH 2/2] Use server scheme to handle credential request API group changes Signed-off-by: Monis Khan --- internal/concierge/server/server.go | 53 +++++++++++++++--- internal/concierge/server/server_test.go | 56 ++++++++++++++++--- .../authenticator/authncache/cache.go | 8 +-- 3 files changed, 97 insertions(+), 20 deletions(-) diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index 584a84b61..dbcafb0aa 100644 --- a/internal/concierge/server/server.go +++ b/internal/concierge/server/server.go @@ -174,17 +174,17 @@ func getAggregatedAPIServerConfig( startControllersPostStartHook func(context.Context), apiGroupSuffix string, ) (*apiserver.Config, error) { - apiGroup, ok := groupsuffix.Replace(loginv1alpha1.GroupName, apiGroupSuffix) + loginConciergeAPIGroup, ok := groupsuffix.Replace(loginv1alpha1.GroupName, apiGroupSuffix) if !ok { return nil, fmt.Errorf("cannot make api group from %s/%s", loginv1alpha1.GroupName, apiGroupSuffix) } - scheme := getAggregatedAPIServerScheme(apiGroup) + scheme := getAggregatedAPIServerScheme(loginConciergeAPIGroup, apiGroupSuffix) codecs := serializer.NewCodecFactory(scheme) - defaultEtcdPathPrefix := fmt.Sprintf("/registry/%s", apiGroup) + defaultEtcdPathPrefix := fmt.Sprintf("/registry/%s", loginConciergeAPIGroup) groupVersion := schema.GroupVersion{ - Group: apiGroup, + Group: loginConciergeAPIGroup, Version: loginv1alpha1.SchemeGroupVersion.Version, } @@ -224,7 +224,7 @@ func getAggregatedAPIServerConfig( return apiServerConfig, nil } -func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme { +func getAggregatedAPIServerScheme(loginConciergeAPIGroup, apiGroupSuffix string) *runtime.Scheme { // standard set up of the server side scheme scheme := runtime.NewScheme() @@ -232,7 +232,7 @@ func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme { metav1.AddToGroupVersion(scheme, metav1.Unversioned) // nothing fancy is required if using the standard group - if apiGroup == loginv1alpha1.GroupName { + if loginConciergeAPIGroup == loginv1alpha1.GroupName { utilruntime.Must(loginv1alpha1.AddToScheme(scheme)) utilruntime.Must(loginapi.AddToScheme(scheme)) return scheme @@ -257,7 +257,7 @@ func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme { panic(err) // programmer error, scheme internal code is broken } newGVK := schema.GroupVersionKind{ - Group: apiGroup, + Group: loginConciergeAPIGroup, Version: gvk.Version, Kind: gvk.Kind, } @@ -270,5 +270,44 @@ func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme { utilruntime.Must(loginv1alpha1.RegisterConversions(scheme)) utilruntime.Must(loginv1alpha1.RegisterDefaults(scheme)) + // we do not want to return errors from the scheme and instead would prefer to defer + // to the REST storage layer for consistency. The simplest way to do this is to force + // a cache miss from the authenticator cache. Kube API groups are validated via the + // IsDNS1123Subdomain func thus we can easily create a group that is guaranteed never + // to be in the authenticator cache. Add a timestamp just to be extra sure. + const authenticatorCacheMissPrefix = "_INVALID_API_GROUP_" + authenticatorCacheMiss := authenticatorCacheMissPrefix + time.Now().UTC().String() + + // we do not have any defaulting functions for *loginv1alpha1.TokenCredentialRequest + // today, but we may have some in the future. Calling AddTypeDefaultingFunc overwrites + // any previously registered defaulting function. Thus to make sure that we catch + // a situation where we add a defaulting func, we attempt to call it here with a nil + // *loginv1alpha1.TokenCredentialRequest. This will do nothing when there is no + // defaulting func registered, but it will almost certainly panic if one is added. + scheme.Default((*loginv1alpha1.TokenCredentialRequest)(nil)) + + // on incoming requests, restore the authenticator API group to the standard group + // note that we are responsible for duplicating this logic for every external API version + scheme.AddTypeDefaultingFunc(&loginv1alpha1.TokenCredentialRequest{}, func(obj interface{}) { + credentialRequest := obj.(*loginv1alpha1.TokenCredentialRequest) + + if credentialRequest.Spec.Authenticator.APIGroup == nil { + // force a cache miss because this is an invalid request + plog.Debug("invalid token credential request, nil group", "authenticator", credentialRequest.Spec.Authenticator) + credentialRequest.Spec.Authenticator.APIGroup = &authenticatorCacheMiss + return + } + + restoredGroup, ok := groupsuffix.Unreplace(*credentialRequest.Spec.Authenticator.APIGroup, apiGroupSuffix) + if !ok { + // force a cache miss because this is an invalid request + plog.Debug("invalid token credential request, wrong group", "authenticator", credentialRequest.Spec.Authenticator) + credentialRequest.Spec.Authenticator.APIGroup = &authenticatorCacheMiss + return + } + + credentialRequest.Spec.Authenticator.APIGroup = &restoredGroup + }) + return scheme } diff --git a/internal/concierge/server/server_test.go b/internal/concierge/server/server_test.go index c270837f7..258e914ef 100644 --- a/internal/concierge/server/server_test.go +++ b/internal/concierge/server/server_test.go @@ -13,12 +13,14 @@ import ( "github.com/google/go-cmp/cmp" "github.com/spf13/cobra" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" loginapi "go.pinniped.dev/generated/1.20/apis/concierge/login" loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" + "go.pinniped.dev/internal/groupsuffix" ) const knownGoodUsage = ` @@ -124,13 +126,13 @@ func Test_getAggregatedAPIServerScheme(t *testing.T) { } tests := []struct { - name string - apiGroup string - want map[schema.GroupVersionKind]reflect.Type + name string + apiGroupSuffix string + want map[schema.GroupVersionKind]reflect.Type }{ { - name: "regular api group", - apiGroup: "login.concierge.pinniped.dev", + name: "regular api group", + apiGroupSuffix: "pinniped.dev", want: map[schema.GroupVersionKind]reflect.Type{ // all the types that are in the aggregated API group @@ -171,8 +173,8 @@ func Test_getAggregatedAPIServerScheme(t *testing.T) { }, }, { - name: "other api group", - apiGroup: "login.concierge.walrus.tld", + name: "other api group", + apiGroupSuffix: "walrus.tld", want: map[schema.GroupVersionKind]reflect.Type{ // all the types that are in the aggregated API group @@ -216,8 +218,46 @@ func Test_getAggregatedAPIServerScheme(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - scheme := getAggregatedAPIServerScheme(tt.apiGroup) + loginConciergeAPIGroup, ok := groupsuffix.Replace("login.concierge.pinniped.dev", tt.apiGroupSuffix) + require.True(t, ok) + + scheme := getAggregatedAPIServerScheme(loginConciergeAPIGroup, tt.apiGroupSuffix) require.Equal(t, tt.want, scheme.AllKnownTypes()) + + // make a credential request like a client would send + authenticationConciergeAPIGroup := "authentication.concierge." + tt.apiGroupSuffix + credentialRequest := &loginv1alpha1.TokenCredentialRequest{ + Spec: loginv1alpha1.TokenCredentialRequestSpec{ + Authenticator: corev1.TypedLocalObjectReference{ + APIGroup: &authenticationConciergeAPIGroup, + }, + }, + } + + // run defaulting on it + scheme.Default(credentialRequest) + + // make sure the group is restored if needed + require.Equal(t, "authentication.concierge.pinniped.dev", *credentialRequest.Spec.Authenticator.APIGroup) + + // make a credential request in the standard group + defaultAuthenticationConciergeAPIGroup := "authentication.concierge.pinniped.dev" + defaultCredentialRequest := &loginv1alpha1.TokenCredentialRequest{ + Spec: loginv1alpha1.TokenCredentialRequestSpec{ + Authenticator: corev1.TypedLocalObjectReference{ + APIGroup: &defaultAuthenticationConciergeAPIGroup, + }, + }, + } + + // run defaulting on it + scheme.Default(defaultCredentialRequest) + + if tt.apiGroupSuffix == "pinniped.dev" { // when using the standard group, this should just work + require.Equal(t, "authentication.concierge.pinniped.dev", *defaultCredentialRequest.Spec.Authenticator.APIGroup) + } else { // when using any other group, this should always be a cache miss + require.True(t, strings.HasPrefix(*defaultCredentialRequest.Spec.Authenticator.APIGroup, "_INVALID_API_GROUP_2")) + } }) } } diff --git a/internal/controller/authenticator/authncache/cache.go b/internal/controller/authenticator/authncache/cache.go index 87f317dff..2816d27c2 100644 --- a/internal/controller/authenticator/authncache/cache.go +++ b/internal/controller/authenticator/authncache/cache.go @@ -6,7 +6,6 @@ package authncache import ( "context" - "fmt" "sort" "sync" @@ -15,13 +14,12 @@ import ( "k8s.io/klog/v2" loginapi "go.pinniped.dev/generated/1.20/apis/concierge/login" + "go.pinniped.dev/internal/constable" "go.pinniped.dev/internal/plog" ) -var ( - // ErrNoSuchAuthenticator is returned by Cache.AuthenticateTokenCredentialRequest() when the requested authenticator is not configured. - ErrNoSuchAuthenticator = fmt.Errorf("no such authenticator") -) +// ErrNoSuchAuthenticator is returned by Cache.AuthenticateTokenCredentialRequest() when the requested authenticator is not configured. +const ErrNoSuchAuthenticator = constable.Error("no such authenticator") // Cache implements the authenticator.Token interface by multiplexing across a dynamic set of authenticators // loaded from authenticator resources.