Implement the OIDCClientSecretRequest API

This commit is a WIP commit because it doesn't include many tests
for the new feature.

Co-authored-by: Ryan Richard <richardry@vmware.com>
Co-authored-by: Benjamin A. Petersen <ben@benjaminapetersen.me>

deploy/supervisor/deployment.yaml

@@ -98,10 +98,26 @@ spec:
             readOnlyRootFilesystem: true
           resources:
             requests:
-              cpu: "100m"
+              #! If OIDCClient CRs are being used, then the Supervisor needs enough CPU to run expensive bcrypt
+              #! operations inside the implementation of the token endpoint for any authcode flows performed by those
+              #! clients, so for that use case administrators may wish to increase the requests.cpu value to more
+              #! closely align with their anticipated needs. Increasing this value will cause Kubernetes to give more
+              #! available CPU to this process during times of high CPU contention. By default, don't ask for too much
+              #! because that would make it impossible to install the Pinniped Supervisor on small clusters.
+              #! Aside from performing bcrypts at the token endpoint for those clients, the Supervisor is not a
+              #! particularly CPU-intensive process.
+              cpu: "100m" #! by default, request one-tenth of a CPU
               memory: "128Mi"
             limits:
-              cpu: "100m"
+              #! By declaring a CPU limit that is not equal to the CPU request value, the Supervisor will be classified
+              #! by Kubernetes to have "burstable" quality of service.
+              #! See https://kubernetes.io/docs/tasks/configure-pod-container/quality-service-pod/#create-a-pod-that-gets-assigned-a-qos-class-of-burstable
+              #! If OIDCClient CRs are being used, and lots of simultaneous users have active sessions, then it is hard
+              #! pre-determine what the CPU limit should be for that use case. Guessing too low would cause the
+              #! pod's CPU usage to be throttled, resulting in poor performance. Guessing too high would allow clients
+              #! to cause the usage of lots of CPU resources. Administrators who have a good sense of anticipated usage
+              #! patterns may choose to set the requests.cpu and limits.cpu differently from these defaults.
+              cpu: "1000m" #! by default, throttle each pod's usage at 1 CPU
               memory: "128Mi"
           volumeMounts:
             - name: config-volume

internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go

@@ -82,7 +82,7 @@ func (c *oidcClientWatcherController) Sync(ctx controllerlib.Context) error {
 
 	// We're only going to use storage to call GetName(), which happens to not need the constructor params.
 	// This is because we can read the Secrets from the informer cache here, instead of doing live reads.
-	storage := oidcclientsecretstorage.New(nil, nil)
+	storage := oidcclientsecretstorage.New(nil)
 
 	for _, oidcClient := range oidcClients {
 		// Skip the OIDCClients that we are not trying to observe.

internal/crud/crud.go

@@ -14,8 +14,10 @@ import (
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 
 	"go.pinniped.dev/internal/constable"
@@ -40,7 +42,7 @@ const (
 )
 
 type Storage interface {
-	Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string) (resourceVersion string, err error)
+	Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string, ownerReferences []metav1.OwnerReference) (resourceVersion string, err error)
 	Get(ctx context.Context, signature string, data JSON) (resourceVersion string, err error)
 	Update(ctx context.Context, signature, resourceVersion string, data JSON) (newResourceVersion string, err error)
 	Delete(ctx context.Context, signature string) error
@@ -68,8 +70,8 @@ type secretsStorage struct {
 	lifetime   time.Duration
 }
 
-func (s *secretsStorage) Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string) (string, error) {
+func (s *secretsStorage) Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string, ownerReferences []metav1.OwnerReference) (string, error) {
-	secret, err := s.toSecret(signature, "", data, additionalLabels)
+	secret, err := s.toSecret(signature, "", data, additionalLabels, ownerReferences)
 	if err != nil {
 		return "", err
 	}
@@ -94,14 +96,26 @@ func (s *secretsStorage) Get(ctx context.Context, signature string, data JSON) (
 }
 
 func (s *secretsStorage) Update(ctx context.Context, signature, resourceVersion string, data JSON) (string, error) {
-	// Note: There may be a small bug here in that toSecret will move the SecretLifetimeAnnotationKey date forward
+	secret, err := s.toSecret(signature, resourceVersion, data, nil, nil)
-	// instead of keeping the storage resource's original SecretLifetimeAnnotationKey value. However, we only use
-	// this Update method in one place, and it doesn't matter in that place. Be aware that it might need improvement
-	// if we start using this Update method in more places.
-	secret, err := s.toSecret(signature, resourceVersion, data, nil)
 	if err != nil {
 		return "", err
 	}
+
+	oldSecret, err := s.secrets.Get(ctx, secret.Name, metav1.GetOptions{})
+	if err != nil {
+		return "", fmt.Errorf("failed to get %s for signature %s: %w", s.resource, signature, err)
+	}
+	// do not assume that our secret client does live reads
+	if oldSecret.ResourceVersion != resourceVersion {
+		return "", errors.NewConflict(schema.GroupResource{Resource: "Secret"}, secret.Name,
+			fmt.Errorf("resource version %s does not match expected value: %s", oldSecret.ResourceVersion, resourceVersion))
+	}
+
+	// preserve these fields - they are effectively immutable on update
+	secret.Labels = oldSecret.Labels
+	secret.Annotations = oldSecret.Annotations
+	secret.OwnerReferences = oldSecret.OwnerReferences
+
 	secret, err = s.secrets.Update(ctx, secret, metav1.UpdateOptions{})
 	if err != nil {
 		return "", fmt.Errorf("failed to update %s for signature %s at resource version %s: %w", s.resource, signature, resourceVersion, err)
@@ -180,18 +194,17 @@ func (s *secretsStorage) GetName(signature string) string {
 	return fmt.Sprintf(secretNameFormat, s.resource, signatureAsValidName)
 }
 
-func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, additionalLabels map[string]string) (*corev1.Secret, error) {
+func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, additionalLabels map[string]string, ownerReferences []metav1.OwnerReference) (*corev1.Secret, error) {
 	buf, err := json.Marshal(data)
 	if err != nil {
 		return nil, fmt.Errorf("failed to encode secret data for %s: %w", s.GetName(signature), err)
 	}
 
-	labelsToAdd := map[string]string{
+	labelsToAdd := make(map[string]string, len(additionalLabels)+1)
-		SecretLabelKey: s.resource, // make it easier to find this stuff via kubectl
-	}
 	for labelName, labelValue := range additionalLabels {
 		labelsToAdd[labelName] = labelValue
 	}
+	labelsToAdd[SecretLabelKey] = s.resource // make it easier to find this stuff via kubectl
 
 	var annotations map[string]string
 	if s.lifetime > 0 {
@@ -206,7 +219,7 @@ func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON,
 			ResourceVersion: resourceVersion,
 			Labels:          labelsToAdd,
 			Annotations:     annotations,
-			OwnerReferences: nil,
+			OwnerReferences: ownerReferences,
 		},
 		Data: map[string][]byte{
 			secretDataKey:    buf,

internal/crud/crud_test.go

@@ -120,7 +120,7 @@ func TestStorage(t *testing.T) {
 				require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is
 
 				data := &testJSON{Data: "create-and-get"}
-				rv1, err := storage.Create(ctx, signature, data, nil)
+				rv1, err := storage.Create(ctx, signature, data, nil, nil)
 				require.Empty(t, rv1) // fake client does not set this
 				require.NoError(t, err)
 
@@ -180,14 +180,14 @@ func TestStorage(t *testing.T) {
 			mocks:    nil,
 			run: func(t *testing.T, storage Storage, fakeClock *clocktesting.FakeClock) error {
 				data := &testJSON{Data: "create1"}
-				rv1, err := storage.Create(ctx, "sig1", data, nil)
+				rv1, err := storage.Create(ctx, "sig1", data, nil, nil)
 				require.Empty(t, rv1) // fake client does not set this
 				require.NoError(t, err)
 
 				fakeClock.Step(42 * time.Minute) // simulate that a known amount of time has passed
 
 				data = &testJSON{Data: "create2"}
-				rv1, err = storage.Create(ctx, "sig2", data, nil)
+				rv1, err = storage.Create(ctx, "sig2", data, nil, nil)
 				require.Empty(t, rv1) // fake client does not set this
 				require.NoError(t, err)
 
@@ -279,7 +279,7 @@ func TestStorage(t *testing.T) {
 				require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is
 
 				data := &testJSON{Data: "create-and-get"}
-				rv1, err := storage.Create(ctx, signature, data, map[string]string{"label1": "value1", "label2": "value2"})
+				rv1, err := storage.Create(ctx, signature, data, map[string]string{"label1": "value1", "label2": "value2"}, nil)
 				require.Empty(t, rv1) // fake client does not set this
 				require.NoError(t, err)
 
@@ -456,6 +456,7 @@ func TestStorage(t *testing.T) {
 				return nil
 			},
 			wantActions: []coretesting.Action{
+				coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-stores-4wssc5gzt5mlln6iux6gl7hzz3klsirisydaxn7indnpvdnrs5ba"),
 				coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-stores-4wssc5gzt5mlln6iux6gl7hzz3klsirisydaxn7indnpvdnrs5ba"),
 				coretesting.NewUpdateAction(secretsGVR, namespace, &corev1.Secret{
 					ObjectMeta: metav1.ObjectMeta{
@@ -1026,7 +1027,7 @@ func TestStorage(t *testing.T) {
 				require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is
 
 				data := &testJSON{Data: "create-and-get"}
-				rv1, err := storage.Create(ctx, signature, data, nil)
+				rv1, err := storage.Create(ctx, signature, data, nil, nil)
 				require.Empty(t, rv1) // fake client does not set this
 				require.NoError(t, err)
 

internal/fositestorage/accesstoken/accesstoken.go

@@ -85,6 +85,7 @@ func (a *accessTokenStorage) CreateAccessTokenSession(ctx context.Context, signa
 		signature,
 		&Session{Request: request, Version: accessTokenStorageVersion},
 		map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()},
+		nil,
 	)
 	return err
 }

internal/fositestorage/authorizationcode/authorizationcode.go

@@ -89,7 +89,7 @@ func (a *authorizeCodeStorage) CreateAuthorizeCodeSession(ctx context.Context, s
 	//      of the consent authorization request. It is used to identify the session.
 	//  signature for lookup in the DB
 
-	_, err = a.storage.Create(ctx, signature, &Session{Active: true, Request: request, Version: authorizeCodeStorageVersion}, nil)
+	_, err = a.storage.Create(ctx, signature, &Session{Active: true, Request: request, Version: authorizeCodeStorageVersion}, nil, nil)
 	return err
 }
 

internal/fositestorage/authorizationcode/authorizationcode_test.go

@@ -72,6 +72,7 @@ func TestAuthorizationCodeStorage(t *testing.T) {
 		}),
 		kubetesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-authcode-pwu5zs7lekbhnln2w4"),
 		kubetesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-authcode-pwu5zs7lekbhnln2w4"),
+		kubetesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-authcode-pwu5zs7lekbhnln2w4"),
 		kubetesting.NewUpdateAction(secretsGVR, namespace, &corev1.Secret{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:            "pinniped-storage-authcode-pwu5zs7lekbhnln2w4",
@@ -134,7 +135,7 @@ func TestAuthorizationCodeStorage(t *testing.T) {
 	require.NoError(t, err)
 
 	testutil.LogActualJSONFromCreateAction(t, client, 0) // makes it easier to update expected values when needed
-	testutil.LogActualJSONFromUpdateAction(t, client, 3) // makes it easier to update expected values when needed
+	testutil.LogActualJSONFromUpdateAction(t, client, 4) // makes it easier to update expected values when needed
 	require.Equal(t, wantActions, client.Actions())
 
 	// Doing a Get on an invalidated session should still return the session, but also return an error.

internal/fositestorage/openidconnect/openidconnect.go

@@ -60,7 +60,7 @@ func (a *openIDConnectRequestStorage) CreateOpenIDConnectSession(ctx context.Con
 		return err
 	}
 
-	_, err = a.storage.Create(ctx, signature, &session{Request: request, Version: oidcStorageVersion}, nil)
+	_, err = a.storage.Create(ctx, signature, &session{Request: request, Version: oidcStorageVersion}, nil, nil)
 	return err
 }
 

internal/fositestorage/pkce/pkce.go

@@ -53,7 +53,7 @@ func (a *pkceStorage) CreatePKCERequestSession(ctx context.Context, signature st
 		return err
 	}
 
-	_, err = a.storage.Create(ctx, signature, &session{Request: request, Version: pkceStorageVersion}, nil)
+	_, err = a.storage.Create(ctx, signature, &session{Request: request, Version: pkceStorageVersion}, nil, nil)
 	return err
 }
 

internal/fositestorage/refreshtoken/refreshtoken.go

@@ -91,6 +91,7 @@ func (a *refreshTokenStorage) CreateRefreshTokenSession(ctx context.Context, sig
 		signature,
 		&Session{Request: request, Version: refreshTokenStorageVersion},
 		map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()},
+		nil,
 	)
 	return err
 }

internal/oidc/clientregistry/clientregistry_test.go

@@ -237,7 +237,7 @@ func TestClientManager(t *testing.T) {
 			oidcClientsClient := supervisorClient.ConfigV1alpha1().OIDCClients(testNamespace)
 			subject := NewClientManager(
 				oidcClientsClient,
-				oidcclientsecretstorage.New(secrets, time.Now),
+				oidcclientsecretstorage.New(secrets),
 				oidcclientvalidator.DefaultMinBcryptCost,
 			)
 

internal/oidc/kube_storage.go

@@ -43,7 +43,7 @@ func NewKubeStorage(
 ) *KubeStorage {
 	nowFunc := time.Now
 	return &KubeStorage{
-		clientManager:            clientregistry.NewClientManager(oidcClientsClient, oidcclientsecretstorage.New(secrets, nowFunc), minBcryptCost),
+		clientManager:            clientregistry.NewClientManager(oidcClientsClient, oidcclientsecretstorage.New(secrets), minBcryptCost),
 		authorizationCodeStorage: authorizationcode.New(secrets, nowFunc, timeoutsConfiguration.AuthorizationCodeSessionStorageLifetime),
 		pkceStorage:              pkce.New(secrets, nowFunc, timeoutsConfiguration.PKCESessionStorageLifetime),
 		oidcStorage:              openidconnect.New(secrets, nowFunc, timeoutsConfiguration.OIDCSessionStorageLifetime),

internal/oidc/nullstorage.go

@@ -5,7 +5,6 @@ package oidc
 
 import (
 	"context"
-	"time"
 
 	"github.com/ory/fosite"
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
@@ -32,7 +31,7 @@ func NewNullStorage(
 	minBcryptCost int,
 ) *NullStorage {
 	return &NullStorage{
-		ClientManager: clientregistry.NewClientManager(oidcClientsClient, oidcclientsecretstorage.New(secrets, time.Now), minBcryptCost),
+		ClientManager: clientregistry.NewClientManager(oidcClientsClient, oidcclientsecretstorage.New(secrets), minBcryptCost),
 	}
 }
 

internal/oidc/oidcclientvalidator/oidcclientvalidator.go

@@ -145,7 +145,7 @@ func validateSecret(secret *v1.Secret, conditions []*v1alpha1.Condition, minBcry
 		return conditions, emptyList
 	}
 
-	storedClientSecret, err := oidcclientsecretstorage.ReadFromSecret(secret)
+	storedClientSecrets, err := oidcclientsecretstorage.ReadFromSecret(secret)
 	if err != nil {
 		// Invalid: storage Secret exists but its data could not be parsed.
 		conditions = append(conditions, &v1alpha1.Condition{
@@ -158,7 +158,7 @@ func validateSecret(secret *v1.Secret, conditions []*v1alpha1.Condition, minBcry
 	}
 
 	// Successfully read the stored client secrets, so check if there are any stored in the list.
-	storedClientSecretsCount := len(storedClientSecret.SecretHashes)
+	storedClientSecretsCount := len(storedClientSecrets)
 	if storedClientSecretsCount == 0 {
 		// Invalid: no client secrets stored.
 		conditions = append(conditions, &v1alpha1.Condition{
@@ -172,7 +172,7 @@ func validateSecret(secret *v1.Secret, conditions []*v1alpha1.Condition, minBcry
 
 	// Check each hashed password's format and bcrypt cost.
 	bcryptErrs := make([]string, 0, storedClientSecretsCount)
-	for i, p := range storedClientSecret.SecretHashes {
+	for i, p := range storedClientSecrets {
 		cost, err := bcrypt.Cost([]byte(p))
 		if err != nil {
 			bcryptErrs = append(bcryptErrs, fmt.Sprintf(
@@ -203,7 +203,7 @@ func validateSecret(secret *v1.Secret, conditions []*v1alpha1.Condition, minBcry
 		Reason:  reasonSuccess,
 		Message: fmt.Sprintf("%d client secret(s) found", storedClientSecretsCount),
 	})
-	return conditions, storedClientSecret.SecretHashes
+	return conditions, storedClientSecrets
 }
 
 func allowedGrantTypesContains(haystack *v1alpha1.OIDCClient, needle string) bool {

internal/oidc/provider/manager/manager_test.go

@@ -217,7 +217,7 @@ func TestManager(t *testing.T) {
 			oidctestutil.VerifyECDSAIDToken(t, jwkIssuer, downstreamClientID, privateKey, idToken)
 
 			// Make sure that we wired up the callback endpoint to use kube storage for fosite sessions.
-			r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+8,
+			r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+9,
 				"did not perform any kube actions during the callback request, but should have")
 		}
 

internal/oidcclientsecretstorage/oidcclientsecretstorage.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"encoding/base64"
 	"fmt"
-	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -15,6 +14,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 
+	configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1"
 	"go.pinniped.dev/internal/constable"
 	"go.pinniped.dev/internal/crud"
 )
@@ -32,9 +32,9 @@ type OIDCClientSecretStorage struct {
 	secrets corev1client.SecretInterface
 }
 
-// StoredClientSecret defines the format of the content of a client's secrets when stored in a Secret
+// storedClientSecret defines the format of the content of a client's secrets when stored in a Secret
 // as a JSON string value.
-type StoredClientSecret struct {
+type storedClientSecret struct {
 	// List of bcrypt hashes.
 	SecretHashes []string `json:"hashes"`
 	// The format version. Take care when updating. We cannot simply bump the storage version and drop/ignore old data.
@@ -42,14 +42,55 @@ type StoredClientSecret struct {
 	Version string `json:"version"`
 }
 
-func New(secrets corev1client.SecretInterface, clock func() time.Time) *OIDCClientSecretStorage {
+func New(secrets corev1client.SecretInterface) *OIDCClientSecretStorage {
 	return &OIDCClientSecretStorage{
-		storage: crud.New(TypeLabelValue, secrets, clock, 0),
+		storage: crud.New(TypeLabelValue, secrets, nil, 0), // can use nil clock because we are using infinite lifetime
 		secrets: secrets,
 	}
 }
 
-// TODO expose other methods as needed for get, create, update, etc.
+func (s *OIDCClientSecretStorage) Get(ctx context.Context, oidcClientUID types.UID) (string, []string, error) {
+	secret := &storedClientSecret{}
+	rv, err := s.storage.Get(ctx, uidToName(oidcClientUID), secret)
+	if errors.IsNotFound(err) {
+		return "", nil, nil
+	}
+	if err != nil {
+		return "", nil, fmt.Errorf("failed to get client secret for uid %s: %w", oidcClientUID, err)
+	}
+
+	return rv, secret.SecretHashes, nil
+}
+
+func (s *OIDCClientSecretStorage) Set(ctx context.Context, resourceVersion, oidcClientName string, oidcClientUID types.UID, secretHashes []string) error {
+	secret := &storedClientSecret{
+		SecretHashes: secretHashes,
+		Version:      oidcClientSecretStorageVersion,
+	}
+	name := uidToName(oidcClientUID)
+
+	if mustBeCreate := len(resourceVersion) == 0; mustBeCreate {
+		ownerReferences := []metav1.OwnerReference{
+			{
+				APIVersion:         configv1alpha1.SchemeGroupVersion.String(),
+				Kind:               "OIDCClient",
+				Name:               oidcClientName,
+				UID:                oidcClientUID,
+				Controller:         nil, // TODO should this be true?
+				BlockOwnerDeletion: nil,
+			},
+		}
+		if _, err := s.storage.Create(ctx, name, secret, nil, ownerReferences); err != nil {
+			return fmt.Errorf("failed to create client secret for uid %s: %w", oidcClientUID, err)
+		}
+		return nil
+	}
+
+	if _, err := s.storage.Update(ctx, name, resourceVersion, secret); err != nil {
+		return fmt.Errorf("failed to update client secret for uid %s: %w", oidcClientUID, err)
+	}
+	return nil
+}
 
 // GetStorageSecret gets the corev1.Secret which is used to store the client secrets for the given client.
 // Returns nil,nil when the corev1.Secret was not found, as this is not an error for a client to not have any secrets yet.
@@ -66,21 +107,24 @@ func (s *OIDCClientSecretStorage) GetStorageSecret(ctx context.Context, oidcClie
 
 // GetName returns the name of the Secret which would be used to store data for the given signature.
 func (s *OIDCClientSecretStorage) GetName(oidcClientUID types.UID) string {
-	// Avoid having s.storage.GetName() base64 decode something that wasn't ever encoded by encoding it here.
+	return s.storage.GetName(uidToName(oidcClientUID))
-	b64encodedUID := base64.RawURLEncoding.EncodeToString([]byte(oidcClientUID))
-	return s.storage.GetName(b64encodedUID)
 }
 
-// ReadFromSecret reads the contents of a Secret as a StoredClientSecret.
+func uidToName(oidcClientUID types.UID) string {
-func ReadFromSecret(secret *corev1.Secret) (*StoredClientSecret, error) {
+	// Avoid having s.storage.GetName() base64 decode something that wasn't ever encoded by encoding it here.
-	storedClientSecret := &StoredClientSecret{}
+	return base64.RawURLEncoding.EncodeToString([]byte(oidcClientUID))
-	err := crud.FromSecret(TypeLabelValue, secret, storedClientSecret)
+}
+
+// ReadFromSecret reads the contents of a Secret as a storedClientSecret and returns the associated hashes.
+func ReadFromSecret(secret *corev1.Secret) ([]string, error) {
+	clientSecret := &storedClientSecret{}
+	err := crud.FromSecret(TypeLabelValue, secret, clientSecret)
 	if err != nil {
 		return nil, err
 	}
-	if storedClientSecret.Version != oidcClientSecretStorageVersion {
+	if clientSecret.Version != oidcClientSecretStorageVersion {
 		return nil, fmt.Errorf("%w: OIDC client secret storage has version %s instead of %s",
-			ErrOIDCClientSecretStorageVersion, storedClientSecret.Version, oidcClientSecretStorageVersion)
+			ErrOIDCClientSecretStorageVersion, clientSecret.Version, oidcClientSecretStorageVersion)
 	}
-	return storedClientSecret, nil
+	return clientSecret.SecretHashes, nil
 }

internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go

@@ -15,7 +15,7 @@ import (
 
 func TestGetName(t *testing.T) {
 	// Note that GetName() should not depend on the constructor params, to make it easier to use in various contexts.
-	subject := New(nil, nil)
+	subject := New(nil)
 
 	require.Equal(t,
 		"pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq",
@@ -30,7 +30,7 @@ func TestReadFromSecret(t *testing.T) {
 	tests := []struct {
 		name       string
 		secret     *corev1.Secret
-		wantStored *StoredClientSecret
+		wantHashes []string
 		wantErr    string
 	}{
 		{
@@ -49,10 +49,7 @@ func TestReadFromSecret(t *testing.T) {
 				},
 				Type: "storage.pinniped.dev/oidc-client-secret",
 			},
-			wantStored: &StoredClientSecret{
+			wantHashes: []string{"first-hash", "second-hash"},
-				Version:      "1",
-				SecretHashes: []string{"first-hash", "second-hash"},
-			},
 		},
 		{
 			name: "wrong secret type",
@@ -113,20 +110,14 @@ func TestReadFromSecret(t *testing.T) {
 			secret: testutil.OIDCClientSecretStorageSecretForUID(t,
 				"some-namespace", "some-uid", []string{"first-hash", "second-hash"},
 			),
-			wantStored: &StoredClientSecret{
+			wantHashes: []string{"first-hash", "second-hash"},
-				Version:      "1",
-				SecretHashes: []string{"first-hash", "second-hash"},
-			},
 		},
 		{
 			name: "OIDCClientSecretStorageSecretWithoutName() test helper generates readable format, to ensure that test helpers are kept up to date",
 			secret: testutil.OIDCClientSecretStorageSecretWithoutName(t,
 				"some-namespace", []string{"first-hash", "second-hash"},
 			),
-			wantStored: &StoredClientSecret{
+			wantHashes: []string{"first-hash", "second-hash"},
-				Version:      "1",
-				SecretHashes: []string{"first-hash", "second-hash"},
-			},
 		},
 		{
 			name:    "OIDCClientSecretStorageSecretForUIDWithWrongVersion() test helper generates readable format, to ensure that test helpers are kept up to date",
@@ -139,13 +130,13 @@ func TestReadFromSecret(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			session, err := ReadFromSecret(tt.secret)
+			hashes, err := ReadFromSecret(tt.secret)
 			if tt.wantErr == "" {
 				require.NoError(t, err)
-				require.Equal(t, tt.wantStored, session)
+				require.Equal(t, tt.wantHashes, hashes)
 			} else {
 				require.EqualError(t, err, tt.wantErr)
-				require.Nil(t, session)
+				require.Nil(t, hashes)
 			}
 		})
 	}

internal/registry/clientsecretrequest/rest.go

@@ -6,27 +6,76 @@ package clientsecretrequest
 
 import (
 	"context"
+	"crypto/rand"
+	"encoding/hex"
 	"fmt"
+	"io"
+	"strings"
 
+	"golang.org/x/crypto/bcrypt"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apiextensions-apiserver/pkg/registry/customresource/tableconvertor"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
+	genericvalidation "k8s.io/apimachinery/pkg/api/validation"
+	"k8s.io/apimachinery/pkg/api/validation/path"
 	metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
+	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/utils/trace"
 
 	clientsecretapi "go.pinniped.dev/generated/latest/apis/supervisor/clientsecret"
+	configv1alpha1clientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/typed/config/v1alpha1"
+	"go.pinniped.dev/internal/oidcclientsecretstorage"
 )
 
-func NewREST(resource schema.GroupResource) *REST {
+// cost is a good bcrypt cost for 2022, should take about 250 ms to validate.
+// This value is expected to be increased over time to match CPU improvements.
+const cost = 12
+
+// nolint: gochecknoglobals
+var tableConvertor = func() rest.TableConvertor {
+	// sadly this is not useful at the moment because `kubectl create` does not support table output
+	columns := []apiextensionsv1.CustomResourceColumnDefinition{
+		{
+			Name:        "Secret",
+			Type:        "string",
+			Description: "", // TODO generate SwaggerDoc() method to fill this field
+			JSONPath:    ".status.generatedSecret",
+		},
+		{
+			Name:        "Total",
+			Type:        "integer",
+			Description: "", // TODO generate SwaggerDoc() method to fill this field
+			JSONPath:    ".status.totalClientSecrets",
+		},
+	}
+	tc, err := tableconvertor.New(columns) // just re-use the CRD table code so we do not have to implement the interface ourselves
+	if err != nil {
+		panic(err) // inputs are static so this should never happen
+	}
+	return tc
+}()
+
+func NewREST(secrets corev1client.SecretInterface, clients configv1alpha1clientset.OIDCClientInterface, namespace string) *REST {
 	return &REST{
-		tableConvertor: rest.NewDefaultTableConvertor(resource),
+		secretStorage: oidcclientsecretstorage.New(secrets),
+		clients:       clients,
+		namespace:     namespace,
+		rand:          rand.Reader,
 	}
 }
 
 type REST struct {
-	tableConvertor rest.TableConvertor
+	secretStorage *oidcclientsecretstorage.OIDCClientSecretStorage
+	clients       configv1alpha1clientset.OIDCClientInterface
+	namespace     string
+	rand          io.Reader
 }
 
 // Assert that our *REST implements all the optional interfaces that we expect it to implement.
@@ -50,6 +99,8 @@ func (*REST) NewList() runtime.Object {
 	return &clientsecretapi.OIDCClientSecretRequestList{}
 }
 
+// List implements the list verb. Support the list verb to support `kubectl get pinniped`, to make sure all resources
+// are in the pinniped category, and avoid kubectl errors when kubectl lists.
 func (*REST) List(_ context.Context, _ *metainternalversion.ListOptions) (runtime.Object, error) {
 	return &clientsecretapi.OIDCClientSecretRequestList{
 		ListMeta: metav1.ListMeta{
@@ -60,7 +111,7 @@ func (*REST) List(_ context.Context, _ *metainternalversion.ListOptions) (runtim
 }
 
 func (r *REST) ConvertToTable(ctx context.Context, obj runtime.Object, tableOptions runtime.Object) (*metav1.Table, error) {
-	return r.tableConvertor.ConvertToTable(ctx, obj, tableOptions)
+	return tableConvertor.ConvertToTable(ctx, obj, tableOptions)
 }
 
 func (*REST) NamespaceScoped() bool {
@@ -72,32 +123,162 @@ func (*REST) Categories() []string {
 }
 
 func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) {
-	t := trace.FromContext(ctx).Nest("create", trace.Field{
+	t := trace.FromContext(ctx).Nest("create",
-		Key:   "kind",
+		trace.Field{Key: "kind", Value: "OIDCClientSecretRequest"},
-		Value: "OIDCClientSecretRequest",
+		trace.Field{Key: "metadata.name", Value: name(obj)},
-	})
+	)
 	defer t.Log()
 
-	_, err := validateRequest(obj, t)
+	// Validate the create request before honoring it.
+	req, err := r.validateRequest(ctx, obj, createValidation, options, t)
 	if err != nil {
 		return nil, err
 	}
+	t.Step("validateRequest")
 
+	// Find the specified OIDCClient.
+	oidcClient, err := r.clients.Get(ctx, req.Name, metav1.GetOptions{})
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			traceValidationFailure(t, fmt.Sprintf("client %q does not exist", req.Name))
+			errs := field.ErrorList{field.NotFound(field.NewPath("metadata", "name"), req.Name)}
+			return nil, apierrors.NewInvalid(kindFromContext(ctx), req.Name, errs)
+		}
+		traceFailureWithError(t, "clients.Get", err)
+		return nil, apierrors.NewInternalError(fmt.Errorf("getting client %q failed", req.Name))
+	}
+	t.Step("clients.Get")
+
+	// Using the OIDCClient's UID, check to see if the storage Secret for its client secrets already exists.
+	// Note that when it does not exist, this Get() function will not return an error, and will return nil rv and hashes.
+	rv, hashes, err := r.secretStorage.Get(ctx, oidcClient.UID)
+	if err != nil {
+		traceFailureWithError(t, "secretStorage.Get", err)
+		return nil, apierrors.NewInternalError(fmt.Errorf("getting secret for client %q failed", req.Name))
+	}
+	t.Step("secretStorage.Get")
+
+	// If requested, generate a new client secret and add it to the list.
+	var secret string
+	if req.Spec.GenerateNewSecret {
+		secret, err = generateSecret(r.rand)
+		if err != nil {
+			traceFailureWithError(t, "generateSecret", err)
+			return nil, apierrors.NewInternalError(fmt.Errorf("client secret generation failed"))
+		}
+		t.Step("generateSecret")
+
+		hash, err := bcrypt.GenerateFromPassword([]byte(secret), cost)
+		if err != nil {
+			traceFailureWithError(t, "bcrypt.GenerateFromPassword", err)
+			return nil, apierrors.NewInternalError(fmt.Errorf("hash generation failed"))
+		}
+		t.Step("bcrypt.GenerateFromPassword")
+
+		hashes = append([]string{string(hash)}, hashes...)
+	}
+
+	// If requested, remove all client secrets except for the most recent one.
+	needsRevoke := req.Spec.RevokeOldSecrets && len(hashes) > 0
+	if needsRevoke {
+		hashes = []string{hashes[0]}
+	}
+
+	// If anything was requested to change...
+	if req.Spec.GenerateNewSecret || needsRevoke {
+		// Each bcrypt comparison is expensive, and we do not want a large list to cause wasted CPU.
+		if len(hashes) > 5 {
+			return nil, apierrors.NewRequestEntityTooLargeError(
+				fmt.Sprintf("OIDCClient %s has too many secrets, spec.revokeOldSecrets must be true", oidcClient.Name))
+		}
+
+		// Create or update the storage Secret for client secrets.
+		if err := r.secretStorage.Set(ctx, rv, oidcClient.Name, oidcClient.UID, hashes); err != nil {
+			if apierrors.IsAlreadyExists(err) || apierrors.IsConflict(err) {
+				return nil, apierrors.NewConflict(qualifiedResourceFromContext(ctx), req.Name,
+					fmt.Errorf("multiple concurrent secret generation requests for same client"))
+			}
+
+			traceFailureWithError(t, "secretStorage.Set", err)
+			return nil, apierrors.NewInternalError(fmt.Errorf("setting client secret failed"))
+		}
+		t.Step("secretStorage.Set")
+	}
+
+	// Return the new secret in plaintext, if one was generated, along with the total number of secrets.
 	return &clientsecretapi.OIDCClientSecretRequest{
 		Status: clientsecretapi.OIDCClientSecretRequestStatus{
-			GeneratedSecret:    "not-a-real-secret",
+			GeneratedSecret:    secret,
-			TotalClientSecrets: 20,
+			TotalClientSecrets: len(hashes),
 		},
 	}, nil
 }
 
-func validateRequest(obj runtime.Object, t *trace.Trace) (*clientsecretapi.OIDCClientSecretRequest, error) {
+func (r *REST) validateRequest(
+	ctx context.Context,
+	obj runtime.Object,
+	createValidation rest.ValidateObjectFunc,
+	options *metav1.CreateOptions,
+	t *trace.Trace,
+) (*clientsecretapi.OIDCClientSecretRequest, error) {
 	clientSecretRequest, ok := obj.(*clientsecretapi.OIDCClientSecretRequest)
 	if !ok {
 		traceValidationFailure(t, "not an OIDCClientSecretRequest")
 		return nil, apierrors.NewBadRequest(fmt.Sprintf("not an OIDCClientSecretRequest: %#v", obj))
 	}
 
+	// Ensure namespace on the object is correct, or error if a conflicting namespace was set in the object.
+	requestNamespace, ok := genericapirequest.NamespaceFrom(ctx)
+	if !ok {
+		return nil, apierrors.NewInternalError(fmt.Errorf("no namespace information found in request context"))
+	}
+	if err := rest.EnsureObjectNamespaceMatchesRequestNamespace(requestNamespace, clientSecretRequest); err != nil {
+		return nil, err
+	}
+	// Making client secrets outside the supervisor's namespace does not make sense.
+	if requestNamespace != r.namespace {
+		msg := fmt.Sprintf("namespace must be %s on OIDCClientSecretRequest, was %s", r.namespace, requestNamespace)
+		traceValidationFailure(t, msg)
+		return nil, apierrors.NewBadRequest(msg)
+	}
+
+	if errs := genericvalidation.ValidateObjectMetaAccessor(
+		clientSecretRequest,
+		true,
+		func(name string, prefix bool) []string {
+			if prefix {
+				return []string{"generateName is not supported"}
+			}
+			var errs []string
+			if name == "client.oauth.pinniped.dev-" {
+				errs = append(errs, `must not equal 'client.oauth.pinniped.dev-'`)
+			}
+			if !strings.HasPrefix(name, "client.oauth.pinniped.dev-") {
+				errs = append(errs, `must start with 'client.oauth.pinniped.dev-'`)
+			}
+			return append(errs, path.IsValidPathSegmentName(name)...)
+		},
+		field.NewPath("metadata"),
+	); len(errs) > 0 {
+		return nil, apierrors.NewInvalid(kindFromContext(ctx), clientSecretRequest.Name, errs)
+	}
+
+	// just a sanity check, not sure how to honor a dry run on a virtual API
+	if options != nil {
+		if len(options.DryRun) != 0 {
+			traceValidationFailure(t, "dryRun not supported")
+			errs := field.ErrorList{field.NotSupported(field.NewPath("dryRun"), options.DryRun, nil)}
+			return nil, apierrors.NewInvalid(kindFromContext(ctx), clientSecretRequest.Name, errs)
+		}
+	}
+
+	if createValidation != nil {
+		if err := createValidation(ctx, obj.DeepCopyObject()); err != nil {
+			traceFailureWithError(t, "validation webhook", err)
+			return nil, err
+		}
+	}
+
 	return clientSecretRequest, nil
 }
 
@@ -107,3 +288,42 @@ func traceValidationFailure(t *trace.Trace, msg string) {
 		trace.Field{Key: "msg", Value: msg},
 	)
 }
+
+func traceFailureWithError(t *trace.Trace, failureType string, err error) {
+	t.Step("failure",
+		trace.Field{Key: "failureType", Value: failureType},
+		trace.Field{Key: "msg", Value: err.Error()},
+	)
+}
+
+func generateSecret(rand io.Reader) (string, error) {
+	var buf [32]byte
+	if _, err := io.ReadFull(rand, buf[:]); err != nil {
+		return "", fmt.Errorf("could not generate client secret: %w", err)
+	}
+	return hex.EncodeToString(buf[:]), nil
+}
+
+func name(obj runtime.Object) string {
+	accessor, err := meta.Accessor(obj)
+	if err != nil {
+		return "<unknown>"
+	}
+	return accessor.GetName()
+}
+
+func qualifiedResourceFromContext(ctx context.Context) schema.GroupResource {
+	if info, ok := genericapirequest.RequestInfoFrom(ctx); ok {
+		return schema.GroupResource{Group: info.APIGroup, Resource: info.Resource}
+	}
+	// this should never happen in practice
+	return clientsecretapi.Resource("oidcclientsecretrequests")
+}
+
+func kindFromContext(ctx context.Context) schema.GroupKind {
+	if info, ok := genericapirequest.RequestInfoFrom(ctx); ok {
+		return schema.GroupKind{Group: info.APIGroup, Kind: "OIDCClientSecretRequest"}
+	}
+	// this should never happen in practice
+	return clientsecretapi.Kind("OIDCClientSecretRequest")
+}

internal/supervisor/apiserver/apiserver.go

@@ -14,8 +14,10 @@ import (
 	"k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apiserver/pkg/registry/rest"
 	genericapiserver "k8s.io/apiserver/pkg/server"
+	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/pkg/version"
 
+	configv1alpha1clientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/typed/config/v1alpha1"
 	"go.pinniped.dev/internal/controllerinit"
 	"go.pinniped.dev/internal/plog"
 	"go.pinniped.dev/internal/registry/clientsecretrequest"
@@ -31,6 +33,9 @@ type ExtraConfig struct {
 	Scheme                             *runtime.Scheme
 	NegotiatedSerializer               runtime.NegotiatedSerializer
 	ClientSecretSupervisorGroupVersion schema.GroupVersion
+	Secrets                            corev1client.SecretInterface
+	OIDCClients                        configv1alpha1clientset.OIDCClientInterface
+	Namespace                          string
 }
 
 type PinnipedServer struct {
@@ -75,7 +80,7 @@ func (c completedConfig) New() (*PinnipedServer, error) {
 	for _, f := range []func() (schema.GroupVersionResource, rest.Storage){
 		func() (schema.GroupVersionResource, rest.Storage) {
 			clientSecretReqGVR := c.ExtraConfig.ClientSecretSupervisorGroupVersion.WithResource("oidcclientsecretrequests")
-			clientSecretReqStorage := clientsecretrequest.NewREST(clientSecretReqGVR.GroupResource())
+			clientSecretReqStorage := clientsecretrequest.NewREST(c.ExtraConfig.Secrets, c.ExtraConfig.OIDCClients, c.ExtraConfig.Namespace)
 			return clientSecretReqGVR, clientSecretReqStorage
 		},
 	} {

internal/supervisor/server/server.go

@@ -31,6 +31,7 @@ import (
 	genericoptions "k8s.io/apiserver/pkg/server/options"
 	kubeinformers "k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes"
+	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/pkg/version"
 	"k8s.io/client-go/rest"
 	aggregatorclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset"
@@ -38,6 +39,7 @@ import (
 
 	configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1"
 	pinnipedclientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned"
+	"go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/typed/config/v1alpha1"
 	pinnipedinformers "go.pinniped.dev/generated/latest/client/supervisor/informers/externalversions"
 	"go.pinniped.dev/internal/apiserviceref"
 	"go.pinniped.dev/internal/config/supervisor"
@@ -475,6 +477,9 @@ func runSupervisor(ctx context.Context, podInfo *downward.PodInfo, cfg *supervis
 		*cfg.AggregatedAPIServerPort,
 		scheme,
 		clientSecretGV,
+		clientWithoutLeaderElection.Kubernetes.CoreV1().Secrets(serverInstallationNamespace),
+		client.PinnipedSupervisor.ConfigV1alpha1().OIDCClients(serverInstallationNamespace),
+		serverInstallationNamespace,
 	)
 	if err != nil {
 		return fmt.Errorf("could not configure aggregated API server: %w", err)
@@ -568,7 +573,6 @@ func runSupervisor(ctx context.Context, podInfo *downward.PodInfo, cfg *supervis
 	return nil
 }
 
-// Create a configuration for the aggregated API server.
 func getAggregatedAPIServerConfig(
 	dynamicCertProvider dynamiccert.Private,
 	buildControllers controllerinit.RunnerBuilder,
@@ -576,6 +580,9 @@ func getAggregatedAPIServerConfig(
 	aggregatedAPIServerPort int64,
 	scheme *runtime.Scheme,
 	clientSecretSupervisorGroupVersion schema.GroupVersion,
+	secrets corev1client.SecretInterface,
+	oidcClients v1alpha1.OIDCClientInterface,
+	serverInstallationNamespace string,
 ) (*apiserver.Config, error) {
 	codecs := serializer.NewCodecFactory(scheme)
 
@@ -620,6 +627,9 @@ func getAggregatedAPIServerConfig(
 			Scheme:                             scheme,
 			NegotiatedSerializer:               codecs,
 			ClientSecretSupervisorGroupVersion: clientSecretSupervisorGroupVersion,
+			Secrets:                            secrets,
+			OIDCClients:                        oidcClients,
+			Namespace:                          serverInstallationNamespace,
 		},
 	}
 	return apiServerConfig, nil

proposals/1125_dynamic-supervisor-oidc-clients/ACCEPTANCE-NOTES.md

@@ -0,0 +1,200 @@
+# Notes for story acceptance for the dynamic clients feature
+
+Rather than writing a webapp to manually test the dynamic client features during user story acceptance,
+we can simulate the requests that a webapp would make to the Supervisor using the commands shown below.
+The commands below the happy path for a fully-capable OIDCClient which is allowed to use all supported
+grant types and scopes. These commands can be adjusted to test other scenarios of interest.
+
+## Deploy and configure a basic Supervisor locally
+
+We can use the developer hack scripts to deploy a working Supervisor on a local Kind cluster.
+These clusters have no ingress, so we use Kind's port mapping feature to expose a web proxy outside
+the cluster. The proxy can then be used to access the Supervisor. In this setup, the Supervisor's CA
+is not trusted by the web browser, however, the curl commands can trust the CA cert by using the `--cacert` flag.
+
+```shell
+./hack/prepare-for-integration-tests.sh -c
+source /tmp/integration-test-env
+# We'll use LDAP so we can login in via curl commands through the Supervisor.
+./hack/prepare-supervisor-on-kind.sh --ldap --flow browser_authcode
+```
+
+Alternatively, the Supervisor could be installed into a cluster in a more production-like way, with ingress,
+a DNS entry, and TLS certs. In this case, the proxy env vars used below would not be needed, and the issuer string
+would be adjusted to match the Supervisor's ingress DNS hostname.
+
+## Create an OIDCClient
+
+```shell
+cat <<EOF | kubectl apply -f -
+apiVersion: config.supervisor.pinniped.dev/v1alpha1
+kind: OIDCClient
+metadata:
+  # name must have client.oauth.pinniped.dev- prefix
+  name: client.oauth.pinniped.dev-my-webapp-client
+  namespace: supervisor # must be in the same namespace as the Supervisor
+spec:
+  allowedRedirectURIs:
+    - https://webapp.example.com/callback
+  allowedGrantTypes:
+    - authorization_code
+    - refresh_token
+    - urn:ietf:params:oauth:grant-type:token-exchange
+  allowedScopes:
+    - openid
+    - offline_access
+    - pinniped:request-audience
+    - username
+    - groups
+EOF
+```
+
+Get the OIDCClient to check its status:
+```shell
+kubectl get oidcclient -n supervisor client.oauth.pinniped.dev-my-webapp-client -o yaml
+```
+
+Create a client secret for the OIDCClient:
+
+```shell
+cat <<EOF | kubectl create -o yaml -f -
+apiVersion: clientsecret.supervisor.pinniped.dev/v1alpha1
+kind: OIDCClientSecretRequest
+metadata:
+  name: client.oauth.pinniped.dev-my-webapp-client # the name of the OIDCClient
+  namespace: supervisor # the namespace of the OIDCClient
+spec:
+  generateNewSecret: true
+EOF
+```
+
+Example response:
+
+```yaml
+apiVersion: clientsecret.supervisor.pinniped.dev/v1alpha1
+kind: OIDCClientSecretRequest
+metadata:
+  creationTimestamp: null
+spec:
+  generateNewSecret: false
+  revokeOldSecrets: false
+status:
+  generatedSecret: 0cc65d46fb5c0fb80123b28bd8093ae0e61e568b6c35cbca82941dcaa8c67b5b
+  totalClientSecrets: 1
+```
+
+Make a note of the `generatedSecret` value. It will never be shown again.
+
+## Make an authorization request
+
+The OIDC authcode flow always starts with an authorization request. A webapp would redirect the user's browser
+to make this request in a browser. For story acceptance, this request could also be made in a web browser by typing
+the full URL with params into the browser's address bar, although here we'll show how to use curl to ensure that we
+are documenting the exact requirements of the authorization request.
+
+Authorization parameter notes:
+- Authorization requests must use PKCE. For manual testing, these sample values can be used. For production use,
+  each authorization request must have a new PKCE value computed for that request.
+  - Example code challenge: vTu6b5Jm2hpi1vjRJw7HB820EYNq7AFT1IHDLBQMc3Q
+  - Example code verifier: UDABWPiROQh0nfhGzd_7OetrEJZZ7S-Z_H8_ZLB2i8Yc2wix
+- Nonce values should also be unique per authorization request in production use.
+- State values are optional and will be passed back in the authcode callback if provided.
+
+```shell
+PARAMS='?response_type=code'\
+'&client_id=client.oauth.pinniped.dev-my-webapp-client'\
+'&code_challenge=vTu6b5Jm2hpi1vjRJw7HB820EYNq7AFT1IHDLBQMc3Q'\
+'&code_challenge_method=S256'\
+'&nonce=9902045656a1c29b95515f7f45b40773'\
+'&redirect_uri=https%3A%2F%2Fwebapp.example.com%2Fcallback'\
+'&scope=openid+offline_access+username+groups+pinniped%3Arequest-audience'\
+'&state=cfcd3a3e72774bee1e748e6bf4a70f5c'
+
+https_proxy="http://127.0.0.1:12346" no_proxy="127.0.0.1" \
+  curl -vfLsS --cookie-jar cookies.txt --cacert root_ca.crt \
+  "https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path/oauth2/authorize$PARAMS"
+```
+
+When successful, this should redirect to the Supervisor's LDAP login page and return its HTML.
+The resulting HTML form will include a hidden param called `state`.
+Make a note of its value for the next step.
+
+The LDAP login page's form can be submitted with:
+
+```shell
+STATE='COPY_PASTE_HIDDEN_STATE_PARAM_FROM_PREVIOUS_CURL_RESULT_HERE'
+
+https_proxy="http://127.0.0.1:12346" no_proxy="127.0.0.1" \
+  curl -vfsS --cookie cookies.txt --cacert root_ca.crt \
+  "https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path/login" \
+  --form-string "username=$PINNIPED_TEST_LDAP_USER_CN" \
+  --form-string "password=$PINNIPED_TEST_LDAP_USER_PASSWORD" \
+  --form-string "state=$STATE"
+```
+
+When successful, this should result in an HTTP 302 or 303 redirect response. The `location` header should look something like
+`https://webapp.example.com/callback?code=pin_ac_oq7m9z...wuzQ&scope=openid+offline_access+pinniped%3Arequest-audience+username+groups&state=cfcd3a3e72774bee1e748e6bf4a70f5c`
+which includes the authcode as the `code` param. Make a note of its value for the next step.
+
+## Make a token request to exchange the authcode obtained in the previous step
+
+The authcode callback would be handled by the webapp's backend. The backend code would then use the authcode
+to make a token request to the Supervisor. This would happen as a backend request, so the user's browser would not be
+involved.
+
+```shell
+CODE='COPY_AUTHCODE_FROM_PREVIOUS_CURL_RESULT_HERE'
+CLIENT_SECRET='COPY_CLIENT_SECRET_HERE'
+
+https_proxy="http://127.0.0.1:12346" no_proxy="127.0.0.1" \
+  curl -vfsS --cacert root_ca.crt \
+  "https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path/oauth2/token" \
+  --form-string "grant_type=authorization_code" \
+  --form-string "code=$CODE" \
+  --form-string "redirect_uri=https://webapp.example.com/callback" \
+  --form-string "code_verifier=UDABWPiROQh0nfhGzd_7OetrEJZZ7S-Z_H8_ZLB2i8Yc2wix" \
+  -u "client.oauth.pinniped.dev-my-webapp-client:$CLIENT_SECRET"
+```
+
+When successful, this should return some JSON which includes the Supervisor-issued tokens.
+The ID token can be decoded for inspection (e.g. using https://jwt.io).
+Make a note of the access token and the refresh token for the next steps.
+
+## Make a request for a cluster-scoped ID token
+
+If the webapp wanted to access a Kubernetes cluster on behalf of the end user, it would need to make
+an additional request (per cluster) to get a cluster-scoped ID token.
+
+```shell
+ACCESS='COPY_ACCESS_TOKEN_FROM_PREVIOUS_CURL_RESULT_HERE'
+
+https_proxy="http://127.0.0.1:12346" no_proxy="127.0.0.1" \
+  curl -vfsS --cacert root_ca.crt \
+  "https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path/oauth2/token" \
+  --form-string "grant_type=urn:ietf:params:oauth:grant-type:token-exchange" \
+  --form-string "audience=my-workload-cluster-audience-name" \
+  --form-string "subject_token=$ACCESS" \
+  --form-string "subject_token_type=urn:ietf:params:oauth:token-type:access_token" \
+  --form-string "requested_token_type=urn:ietf:params:oauth:token-type:jwt" \
+  -u "client.oauth.pinniped.dev-my-webapp-client:$CLIENT_SECRET"
+```
+
+If successful, this should return some JSON with a new cluster-scoped ID token in the response.
+
+## Make a refresh request
+
+The ID and access tokens are very short-lived, so the backend of the webapp should refresh them as needed.
+
+```shell
+REFRESH='COPY_REFRESH_TOKEN_FROM_PREVIOUS_CURL_RESULT_HERE'
+
+https_proxy="http://127.0.0.1:12346" no_proxy="127.0.0.1" \
+  curl -vfsS --cacert root_ca.crt \
+  "https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path/oauth2/token" \
+  --form-string "grant_type=refresh_token" \
+  --form-string "refresh_token=$REFRESH" \
+  -u "client.oauth.pinniped.dev-my-webapp-client:$CLIENT_SECRET"
+```
+
+When successful, this should return some JSON which includes the new Supervisor-issued tokens.
+The old refresh token is revoked and the next refresh request must use the newest refresh token.

proposals/1125_dynamic-supervisor-oidc-clients/README.md

@@ -1,9 +1,9 @@
 ---
 title: "Dynamic Supervisor OIDC Clients"
 authors: [ "@cfryanr", "@enj" ]
-status: "in-review"
+status: "approved"
 sponsor: [ ]
-approval_date: ""
+approval_date: "Jul 26, 2022"
 ---
 
 *Disclaimer*: Proposals are point-in-time designs and decisions. Once approved and implemented, they become historical

test/integration/supervisor_login_test.go

@@ -1404,7 +1404,7 @@ func TestSupervisorLogin_Browser(t *testing.T) {
 				require.Equal(t, http.StatusForbidden, status)
 				require.Equal(t,
 					`{"error":"access_denied","error_description":"The resource owner or authorization server denied the request. `+
-						`missing the 'pinniped:request-audience' scope"}`,
+						`Missing the 'pinniped:request-audience' scope."}`,
 					body)
 			},
 		},

test/integration/supervisor_oidc_client_test.go

@@ -361,6 +361,53 @@ func TestOIDCClientStaticValidation_Parallel(t *testing.T) {
 			},
 			wantErr: `OIDCClient.config.supervisor.pinniped.dev "zone" is invalid: [metadata.name: Invalid value: "zone": metadata.name in body should match '^client\.oauth\.pinniped\.dev-', spec.allowedGrantTypes[0]: Unsupported value: "the": supported values: "authorization_code", "refresh_token", "urn:ietf:params:oauth:grant-type:token-exchange", spec.allowedRedirectURIs[0]: Invalid value: "of": spec.allowedRedirectURIs[0] in body should match '^https://.+|^http://(127\.0\.0\.1|\[::1\])(:\d+)?/', spec.allowedScopes[0]: Unsupported value: "enders": supported values: "openid", "offline_access", "username", "groups", "pinniped:request-audience"]`,
 		},
+		{
+			name: "just the prefix is not valid",
+			client: &supervisorconfigv1alpha1.OIDCClient{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "client.oauth.pinniped.dev-",
+				},
+				Spec: supervisorconfigv1alpha1.OIDCClientSpec{
+					AllowedRedirectURIs: []supervisorconfigv1alpha1.RedirectURI{
+						"https://example.com",
+						"http://127.0.0.1/yoyo",
+					},
+					AllowedGrantTypes: []supervisorconfigv1alpha1.GrantType{
+						"authorization_code",
+						"refresh_token",
+						"urn:ietf:params:oauth:grant-type:token-exchange",
+					},
+					AllowedScopes: []supervisorconfigv1alpha1.Scope{
+						"openid",
+						"offline_access",
+						"username",
+						"groups",
+						"pinniped:request-audience",
+					},
+				},
+			},
+			fixWant: func(t *testing.T, err error, want string) string {
+				require.Error(t, err)
+				prefix := `OIDCClient.config.supervisor.pinniped.dev "client.oauth.pinniped.dev-" is invalid: metadata.name: Invalid value: "client.oauth.pinniped.dev-": `
+				suffix := ` must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')`
+
+				oldErrContains := `a DNS-1123 subdomain`           // Kube 1.19 and before used this error text
+				newErrContains := `a lowercase RFC 1123 subdomain` // Newer versions of Kube use this error text
+
+				gotErr := err.Error()
+				switch {
+				case strings.Contains(gotErr, oldErrContains):
+					return prefix + oldErrContains + suffix
+				case strings.Contains(gotErr, newErrContains):
+					return prefix + newErrContains + suffix
+				default:
+					require.Failf(t, "the error message did not contain %q or %q. actual message was: %s",
+						oldErrContains, newErrContains, gotErr)
+					return ""
+				}
+			},
+			wantErr: `this will be replaced by fixWant()`,
+		},
 		{
 			name: "everything valid",
 			client: &supervisorconfigv1alpha1.OIDCClient{
@@ -606,7 +653,7 @@ func TestOIDCClientControllerValidations_Parallel(t *testing.T) {
 
 			if tt.secret != nil {
 				// Force the Secret's name to match the client created above.
-				tt.secret.Name = oidcclientsecretstorage.New(nil, nil).GetName(client.UID)
+				tt.secret.Name = oidcclientsecretstorage.New(nil).GetName(client.UID)
 				secret, err := secrets.Create(ctx, tt.secret, metav1.CreateOptions{})
 				require.NoError(t, err)
 				t.Cleanup(func() {

test/integration/supervisor_oidcclientsecret_test.go

@@ -5,6 +5,7 @@ package integration
 
 import (
 	"context"
+	"encoding/hex"
 	"testing"
 	"time"
 
@@ -12,6 +13,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"go.pinniped.dev/generated/latest/apis/supervisor/clientsecret/v1alpha1"
+	supervisorconfigv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1"
 	"go.pinniped.dev/test/testlib"
 )
 
@@ -19,27 +21,61 @@ func TestOIDCClientSecretRequest_HappyPath_Parallel(t *testing.T) {
 	env := testlib.IntegrationEnv(t)
 
 	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
-	defer cancel()
+	t.Cleanup(cancel)
 
 	client := testlib.NewSupervisorClientset(t)
 
+	oidcClient, err := client.ConfigV1alpha1().OIDCClients(env.SupervisorNamespace).Create(ctx,
+		&supervisorconfigv1alpha1.OIDCClient{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: "client.oauth.pinniped.dev-",
+			},
+			Spec: supervisorconfigv1alpha1.OIDCClientSpec{
+				AllowedRedirectURIs: []supervisorconfigv1alpha1.RedirectURI{
+					"https://example.com",
+					"http://127.0.0.1/yoyo",
+				},
+				AllowedGrantTypes: []supervisorconfigv1alpha1.GrantType{
+					"authorization_code",
+					"refresh_token",
+					"urn:ietf:params:oauth:grant-type:token-exchange",
+				},
+				AllowedScopes: []supervisorconfigv1alpha1.Scope{
+					"openid",
+					"offline_access",
+					"username",
+					"groups",
+					"pinniped:request-audience",
+				},
+			},
+		},
+		metav1.CreateOptions{},
+	)
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		deleteErr := client.ConfigV1alpha1().OIDCClients(env.SupervisorNamespace).Delete(ctx, oidcClient.Name, metav1.DeleteOptions{})
+		require.NoError(t, deleteErr)
+	})
+
 	response, err := client.ClientsecretV1alpha1().OIDCClientSecretRequests(env.SupervisorNamespace).Create(ctx,
 		&v1alpha1.OIDCClientSecretRequest{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: oidcClient.Name,
+			},
 			Spec: v1alpha1.OIDCClientSecretRequestSpec{
 				GenerateNewSecret: true,
 			},
 		}, metav1.CreateOptions{})
 	require.NoError(t, err)
-	// the hardcoded values from the nonfunctional request
+	require.Equal(t, response.Status.TotalClientSecrets, 1)
-	require.Equal(t, response.Status.TotalClientSecrets, 20)
+	require.Len(t, response.Status.GeneratedSecret, hex.EncodedLen(32))
-	require.Equal(t, response.Status.GeneratedSecret, "not-a-real-secret")
 }
 
 func TestOIDCClientSecretRequest_Unauthenticated_Parallel(t *testing.T) {
 	env := testlib.IntegrationEnv(t)
 
 	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
-	defer cancel()
+	t.Cleanup(cancel)
 
 	client := testlib.NewAnonymousSupervisorClientset(t)
 

test/testlib/client.go

@@ -441,7 +441,7 @@ func createOIDCClientSecret(t *testing.T, forOIDCClient *configv1alpha1.OIDCClie
 
 	created, err := kubeClient.CoreV1().Secrets(env.SupervisorNamespace).Create(ctx, &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:        oidcclientsecretstorage.New(nil, nil).GetName(forOIDCClient.UID), // use the required name
+			Name:        oidcclientsecretstorage.New(nil).GetName(forOIDCClient.UID), // use the required name
 			Labels:      map[string]string{"storage.pinniped.dev/type": "oidc-client-secret", "pinniped.dev/test": ""},
 			Annotations: map[string]string{"pinniped.dev/testName": t.Name()},
 		},