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 <moyerm@vmware.com>

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 }

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

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

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)

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
 }