Merge pull request #385 from vmware-tanzu/credential_request_spec_api…

…_group

Use custom suffix in `Spec.Authenticator.APIGroup` of `TokenCredentialRequest`

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()
+ authenticators := authncache.New(*cfg.APIGroupSuffix)
 
  // 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

internal/controller/authenticator/authncache/cache.go

@@ -15,6 +15,7 @@ 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"
 )
 
@@ -26,7 +27,8 @@ 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
+ cache sync.Map
+ apiGroupSuffix string
 }
 
 type Key struct {
@@ -41,8 +43,8 @@ type Value interface {
 }
 
 // New returns an empty cache.
-func New() *Cache {
- return &Cache{}
+func New(apiGroupSuffix string) *Cache {
+ return &Cache{apiGroupSuffix: apiGroupSuffix}
 }
 
 // Get an authenticator by key.
@@ -90,7 +92,12 @@ func (c *Cache) AuthenticateTokenCredentialRequest(ctx context.Context, req *log
  Kind: req.Spec.Authenticator.Kind,
  }
  if req.Spec.Authenticator.APIGroup != nil {
- key.APIGroup = *req.Spec.Authenticator.APIGroup
+ // 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
  }
 
  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()
+ cache := New("pinniped.dev")
  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()
+ cache := New("pinniped.dev")
  for _, i := range rand.Perm(len(keysInExpectedOrder)) {
  cache.Store(keysInExpectedOrder[i], nil)
  }
@@ -91,46 +91,48 @@ func TestAuthenticateTokenCredentialRequest(t *testing.T) {
  Name: validRequest.Spec.Authenticator.Name,
  }
 
- mockCache := func(t *testing.T, res *authenticator.Response, authenticated bool, err error) *Cache {
+ mockCache := func(t *testing.T, apiGroupSuffix string, expectAuthenticatorToBeCalled bool, 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()
+ if expectAuthenticatorToBeCalled {
+ m.EXPECT().AuthenticateToken(audienceFreeContext{}, validRequest.Spec.Token).Return(res, authenticated, err)
+ }
+ c := New(apiGroupSuffix)
  c.Store(validRequestKey, m)
  return c
  }
 
  t.Run("no such authenticator", func(t *testing.T) {
- c := New()
+ c := New("pinniped.dev")
  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, nil, false, fmt.Errorf("some authenticator error"))
+ c := mockCache(t, "pinniped.dev", true, 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)
+ c := mockCache(t, "pinniped.dev", true, &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)
+ c := mockCache(t, "pinniped.dev", true, 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)
+ c := mockCache(t, "pinniped.dev", true, &authenticator.Response{}, true, nil)
  res, err := c.AuthenticateTokenCredentialRequest(context.Background(), validRequest.DeepCopy())
  require.NoError(t, err)
  require.Nil(t, res)
@@ -151,7 +153,7 @@ func TestAuthenticateTokenCredentialRequest(t *testing.T) {
  }
  },
  )
- c := New()
+ c := New("pinniped.dev")
  c.Store(validRequestKey, m)
 
  ctx, cancel := context.WithCancel(context.Background())
@@ -171,7 +173,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, &authenticator.Response{User: &userInfo}, true, nil)
+ c := mockCache(t, "pinniped.dev", true, &authenticator.Response{User: &userInfo}, true, nil)
 
  audienceCtx := authenticator.WithAudiences(context.Background(), authenticator.Audiences{"test-audience-1"})
  res, err := c.AuthenticateTokenCredentialRequest(audienceCtx, validRequest.DeepCopy())
@@ -182,6 +184,50 @@ 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()
+ cache := authncache.New("pinniped.dev")
  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()
+ cache := authncache.New("pinniped.dev")
  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()
+ cache := authncache.New("pinniped.dev")
  testLog := testlogger.New(t)
 
  controller := New(cache, informers.Authentication().V1alpha1().WebhookAuthenticators(), testLog)

internal/groupsuffix/groupsuffix.go

@@ -63,7 +63,7 @@ func New(apiGroupSuffix string) kubeclient.Middleware {
 
  kubeclient.MiddlewareFunc(func(_ context.Context, rt kubeclient.RoundTrip) {
  // always unreplace owner refs with apiGroupSuffix because we can consume those objects across all verbs
- rt.MutateResponse(mutateOwnerRefs(unreplace, apiGroupSuffix))
+ rt.MutateResponse(mutateOwnerRefs(Unreplace, apiGroupSuffix))
  }),
  }
 }
@@ -115,7 +115,8 @@ func Replace(baseAPIGroup, apiGroupSuffix string) (string, bool) {
  return strings.TrimSuffix(baseAPIGroup, pinnipedDefaultSuffix) + apiGroupSuffix, true
 }
 
-func unreplace(baseAPIGroup, apiGroupSuffix string) (string, bool) {
+// Unreplace is like performing an undo of Replace().
+func Unreplace(baseAPIGroup, apiGroupSuffix string) (string, bool) {
  if !strings.HasSuffix(baseAPIGroup, "."+apiGroupSuffix) {
  return "", false
  }

internal/groupsuffix/groupsuffix_test.go

@@ -441,17 +441,40 @@ func TestMiddlware(t *testing.T) {
 }
 
 func TestReplaceError(t *testing.T) {
- _, ok := Replace("bad-suffix-that-doesnt-end-in-pinniped-dot-dev", "shouldnt-matter.com")
+ s, ok := Replace("bad-suffix-that-doesnt-end-in-pinniped-dot-dev", "shouldnt-matter.com")
+ require.Equal(t, "", s)
  require.False(t, ok)
 
- _, ok = Replace("bad-suffix-that-end-in.prefixed-pinniped.dev", "shouldnt-matter.com")
+ s, ok = Replace("bad-suffix-that-end-in.prefixed-pinniped.dev", "shouldnt-matter.com")
+ require.Equal(t, "", s)
  require.False(t, ok)
 }
 
 func TestReplaceSuffix(t *testing.T) {
  s, ok := Replace("something.pinniped.dev.something-else.pinniped.dev", "tuna.io")
  require.Equal(t, "something.pinniped.dev.something-else.tuna.io", s)
  require.True(t, ok)
+
+ // When the replace wasn't actually needed, it still returns true.
+ s, ok = Unreplace("something.pinniped.dev", "pinniped.dev")
+ require.Equal(t, "something.pinniped.dev", s)
+ require.True(t, ok)
+}
+
+func TestUnreplaceSuffix(t *testing.T) {
+ s, ok := Unreplace("something.pinniped.dev.something-else.tuna.io", "tuna.io")
+ require.Equal(t, "something.pinniped.dev.something-else.pinniped.dev", s)
+ require.True(t, ok)
+
+ // When the unreplace wasn't actually needed, it still returns true.
+ s, ok = Unreplace("something.pinniped.dev", "pinniped.dev")
+ require.Equal(t, "something.pinniped.dev", s)
+ require.True(t, ok)
+
+ // When the unreplace was needed but did not work, return false.
+ s, ok = Unreplace("something.pinniped.dev.something-else.tuna.io", "salmon.io")
+ require.Equal(t, "", s)
+ require.False(t, ok)
 }
 
 func TestValidate(t *testing.T) {

pkg/conciergeclient/conciergeclient.go

@@ -12,7 +12,7 @@ import (
  "net/url"
  "strings"
 
- corev1 "k8s.io/api/core/v1"
+ v1 "k8s.io/api/core/v1"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
  clientauthenticationv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1"
  "k8s.io/client-go/tools/clientcmd"
@@ -34,11 +34,12 @@ type Option func(*Client) error
 
 // Client is a configuration for talking to the Pinniped concierge.
 type Client struct {
- namespace string
- authenticator *corev1.TypedLocalObjectReference
- caBundle string
- endpoint *url.URL
- apiGroupSuffix string
+ namespace string
+ authenticatorName string
+ authenticatorKind string
+ caBundle string
+ endpoint *url.URL
+ apiGroupSuffix string
 }
 
 // WithNamespace configures the namespace where the TokenCredentialRequest is to be sent.
@@ -55,18 +56,15 @@ func WithAuthenticator(authType, authName string) Option {
  if authName == "" {
  return fmt.Errorf("authenticator name must not be empty")
  }
- authenticator := corev1.TypedLocalObjectReference{Name: authName}
+ c.authenticatorName = authName
  switch strings.ToLower(authType) {
  case "webhook":
- authenticator.APIGroup = &auth1alpha1.SchemeGroupVersion.Group
- authenticator.Kind = "WebhookAuthenticator"
+ c.authenticatorKind = "WebhookAuthenticator"
  case "jwt":
- authenticator.APIGroup = &auth1alpha1.SchemeGroupVersion.Group
- authenticator.Kind = "JWTAuthenticator"
+ c.authenticatorKind = "JWTAuthenticator"
  default:
  return fmt.Errorf(`invalid authenticator type: %q, supported values are "webhook" and "jwt"`, authType)
  }
- c.authenticator = &authenticator
  return nil
  }
 }
@@ -133,7 +131,7 @@ func New(opts ...Option) (*Client, error) {
  return nil, err
  }
  }
- if c.authenticator == nil {
+ if c.authenticatorName == "" {
  return nil, fmt.Errorf("WithAuthenticator must be specified")
  }
  if c.endpoint == nil {
@@ -180,13 +178,18 @@ func (c *Client) ExchangeToken(ctx context.Context, token string) (*clientauthen
  if err != nil {
  return nil, err
  }
+ replacedAPIGroupName, _ := groupsuffix.Replace(auth1alpha1.SchemeGroupVersion.Group, c.apiGroupSuffix)
  resp, err := clientset.LoginV1alpha1().TokenCredentialRequests(c.namespace).Create(ctx, &loginv1alpha1.TokenCredentialRequest{
  ObjectMeta: metav1.ObjectMeta{
  Namespace: c.namespace,
  },
  Spec: loginv1alpha1.TokenCredentialRequestSpec{
- Token: token,
- Authenticator: *c.authenticator,
+ Token: token,
+ Authenticator: v1.TypedLocalObjectReference{
+ APIGroup: &replacedAPIGroupName,
+ Kind: c.authenticatorKind,
+ Name: c.authenticatorName,
+ },
  },
  }, metav1.CreateOptions{})
  if err != nil {