Merge pull request #394 from enj/enj/i/server_side_tcr_api_group

Use server scheme to handle credential request API group changes

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
@@ -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
 }

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
+		name           string
-		apiGroup string
+		apiGroupSuffix string
-		want     map[schema.GroupVersionKind]reflect.Type
+		want           map[schema.GroupVersionKind]reflect.Type
 	}{
 		{
-			name:     "regular api group",
+			name:           "regular api group",
-			apiGroup: "login.concierge.pinniped.dev",
+			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",
+			name:           "other api group",
-			apiGroup: "login.concierge.walrus.tld",
+			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"))
+			}
 		})
 	}
 }

internal/controller/authenticator/authncache/cache.go

@@ -6,7 +6,6 @@ package authncache
 
 import (
 	"context"
-	"fmt"
 	"sort"
 	"sync"
 
@@ -15,20 +14,17 @@ import (
 	"k8s.io/klog/v2"
 
 	loginapi "go.pinniped.dev/generated/1.20/apis/concierge/login"
-	"go.pinniped.dev/internal/groupsuffix"
+	"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 is returned by Cache.AuthenticateTokenCredentialRequest() when the requested authenticator is not configured.
+const ErrNoSuchAuthenticator = constable.Error("no such authenticator")
-	ErrNoSuchAuthenticator = fmt.Errorf("no such authenticator")
-)
 
 // Cache implements the authenticator.Token interface by multiplexing across a dynamic set of authenticators
 // loaded from authenticator resources.
 type Cache struct {
-	cache          sync.Map
+	cache sync.Map
-	apiGroupSuffix string
 }
 
 type Key struct {
@@ -43,8 +39,8 @@ type Value interface {
 }
 
 // New returns an empty cache.
-func New(apiGroupSuffix string) *Cache {
+func New() *Cache {
-	return &Cache{apiGroupSuffix: apiGroupSuffix}
+	return &Cache{}
 }
 
 // Get an authenticator by key.
@@ -92,12 +88,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.
+		key.APIGroup = *req.Spec.Authenticator.APIGroup
-		apiGroup, replaced := groupsuffix.Unreplace(*req.Spec.Authenticator.APIGroup, c.apiGroupSuffix)
-		if !replaced {
-			return nil, ErrNoSuchAuthenticator
-		}
-		key.APIGroup = apiGroup
 	}
 
 	val := c.Get(key)

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)
-			m.EXPECT().AuthenticateToken(audienceFreeContext{}, validRequest.Spec.Token).Return(res, authenticated, err)
+		c := New()
-		}
-		c := New(apiGroupSuffix)
 		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{}

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)
 			}

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 {

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)