wip002

Signed-off-by: Monis Khan <mok@vmware.com>

internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go

@@ -107,7 +107,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)
@@ -169,7 +183,7 @@ func validateSecret(resource string, secret *corev1.Secret) error {
 	return nil
 }
 
-//nolint: gochecknoglobals
+// nolint: gochecknoglobals
 var b32 = base32.StdEncoding.WithPadding(base32.NoPadding)
 
 func (s *secretsStorage) GetName(signature string) string {
@@ -180,28 +194,32 @@ 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 {
+		annotations = map[string]string{
+			SecretLifetimeAnnotationKey: s.clock().Add(s.lifetime).UTC().Format(SecretLifetimeAnnotationDateFormat),
+		}
+	}
 
 	return &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:            s.GetName(signature),
 			ResourceVersion: resourceVersion,
 			Labels:          labelsToAdd,
-			Annotations: map[string]string{
+			Annotations:     annotations,
-				SecretLifetimeAnnotationKey: s.clock().Add(s.lifetime).UTC().Format(SecretLifetimeAnnotationDateFormat),
+			OwnerReferences: ownerReferences,
-			},
-			OwnerReferences: nil,
 		},
 		Data: map[string][]byte{
 			secretDataKey:    buf,

internal/oidcclientsecretstorage/oidcclientsecretstorage.go

@@ -4,14 +4,17 @@
 package oidcclientsecretstorage
 
 import (
+	"context"
 	"encoding/base64"
 	"fmt"
-	"time"
 
-	v1 "k8s.io/api/core/v1"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"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"
 )
@@ -28,9 +31,9 @@ type OIDCClientSecretStorage struct {
 	storage crud.Storage
 }
 
-// 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.
@@ -38,30 +41,79 @@ type StoredClientSecret struct {
 	Version string `json:"version"`
 }
 
-func New(secrets corev1client.SecretInterface, clock func() time.Time) *OIDCClientSecretStorage {
+func New(secrets corev1client.SecretInterface) *OIDCClientSecretStorage {
-	// TODO make lifetime = 0 mean that it does not get annotated with any garbage collection annotation
+	return &OIDCClientSecretStorage{storage: crud.New(TypeLabelValue, secrets, nil, 0)}
-	return &OIDCClientSecretStorage{storage: crud.New(TypeLabelValue, secrets, clock, 0)}
 }
 
-// TODO expose other methods as needed for get, create, update, etc.
+func (s *OIDCClientSecretStorage) Get(ctx context.Context, oidcClientUID types.UID) ([]string, error) {
+	secret := &storedClientSecret{}
+	_, 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 secret.SecretHashes, nil
+}
+
+func (s *OIDCClientSecretStorage) Set(ctx context.Context, oidcClientName string, oidcClientUID types.UID, secretHashes []string) error {
+	secret := &storedClientSecret{
+		SecretHashes: secretHashes,
+		Version:      oidcClientSecretStorageVersion,
+	}
+	name := uidToName(oidcClientUID)
+
+	rv, err := s.storage.Get(ctx, name, &storedClientSecret{})
+	if errors.IsNotFound(err) {
+		ownerReferences := []metav1.OwnerReference{
+			{
+				APIVersion:         configv1alpha1.SchemeGroupVersion.String(), // TODO uh API group suffix?
+				Kind:               "OIDCClient",
+				Name:               oidcClientName,
+				UID:                oidcClientUID,
+				Controller:         nil, // TODO should this be true?
+				BlockOwnerDeletion: nil,
+			},
+		}
+		_, err := s.storage.Create(ctx, name, secret, nil, ownerReferences)
+		if err != nil {
+			return fmt.Errorf("failed to create client secret for uid %s: %w", oidcClientUID, err)
+		}
+		return nil
+	}
+	if err != nil {
+		return fmt.Errorf("failed to get client secret for uid %s: %w", oidcClientUID, err)
+	}
+
+	_, err = s.storage.Update(ctx, name, rv, secret)
+	if err != nil {
+		return fmt.Errorf("failed to update client secret for uid %s: %w", oidcClientUID, err)
+	}
+	return nil
+}
 
 // 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 *v1.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.
+func ReadFromSecret(s *corev1.Secret) (*storedClientSecret, error) {
+	secret := &storedClientSecret{}
+	err := crud.FromSecret(TypeLabelValue, s, secret)
 	if err != nil {
 		return nil, err
 	}
-	if storedClientSecret.Version != oidcClientSecretStorageVersion {
+	if secret.Version != oidcClientSecretStorageVersion {
 		return nil, fmt.Errorf("%w: OIDC client secret storage has version %s instead of %s",
-			ErrOIDCClientSecretStorageVersion, storedClientSecret.Version, oidcClientSecretStorageVersion)
+			ErrOIDCClientSecretStorageVersion, secret.Version, oidcClientSecretStorageVersion)
 	}
-	return storedClientSecret, nil
+	return secret, nil
 }

internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go

@@ -28,7 +28,7 @@ func TestReadFromSecret(t *testing.T) {
 	tests := []struct {
 		name       string
 		secret     *corev1.Secret
-		wantStored *StoredClientSecret
+		wantStored *storedClientSecret
 		wantErr    string
 	}{
 		{
@@ -47,7 +47,7 @@ func TestReadFromSecret(t *testing.T) {
 				},
 				Type: "storage.pinniped.dev/oidc-client-secret",
 			},
-			wantStored: &StoredClientSecret{
+			wantStored: &storedClientSecret{
 				Version:      "1",
 				SecretHashes: []string{"first-hash", "second-hash"},
 			},

internal/registry/clientsecretrequest/rest.go

@@ -6,8 +6,12 @@ package clientsecretrequest
 
 import (
 	"context"
+	"crypto/rand"
+	"encoding/hex"
 	"fmt"
+	"io"
 
+	"golang.org/x/crypto/bcrypt"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -17,16 +21,27 @@ import (
 	"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/kubeclient"
+	"go.pinniped.dev/internal/oidcclientsecretstorage"
 )
 
-func NewREST(resource schema.GroupResource) *REST {
+const cost = 15 // a good bcrypt cost for 2022, should take about a second to validate
+
+func NewREST(resource schema.GroupResource, client *kubeclient.Client, namespace string) *REST {
 	return &REST{
 		tableConvertor: rest.NewDefaultTableConvertor(resource),
+		secretStorage:  oidcclientsecretstorage.New(client.Kubernetes.CoreV1().Secrets(namespace)),
+		clients:        client.PinnipedSupervisor.ConfigV1alpha1().OIDCClients(namespace),
+		rand:           rand.Reader,
 	}
 }
 
 type REST struct {
 	tableConvertor rest.TableConvertor
+	secretStorage  *oidcclientsecretstorage.OIDCClientSecretStorage
+	clients        configv1alpha1clientset.OIDCClientInterface
+	rand           io.Reader
 }
 
 // Assert that our *REST implements all the optional interfaces that we expect it to implement.
@@ -48,6 +63,9 @@ func (*REST) NewList() runtime.Object {
 	return &clientsecretapi.OIDCClientSecretRequestList{}
 }
 
+// support `kubectl get pinniped`
+// to make sure all resources are in the pinniped category and
+// avoid kubectl errors when kubectl lists you must support the list verb
 func (*REST) List(_ context.Context, _ *metainternalversion.ListOptions) (runtime.Object, error) {
 	return &clientsecretapi.OIDCClientSecretRequestList{
 		ListMeta: metav1.ListMeta{
@@ -76,17 +94,51 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 	})
 	defer t.Log()
 
-	// TODO
+	req, err := validateRequest(obj, t)
-
-	_, err := validateRequest(obj, t)
 	if err != nil {
 		return nil, err
 	}
 
+	oidcClient, err := r.clients.Get(ctx, req.Name, metav1.GetOptions{})
+	if err != nil {
+		return nil, err // TODO obfuscate
+	}
+
+	hashes, err := r.secretStorage.Get(ctx, oidcClient.UID)
+	if err != nil {
+		return nil, err // TODO obfuscate
+	}
+
+	var secret string
+	if req.Spec.GenerateNewSecret {
+		secret, err = generateSecret(r.rand)
+		if err != nil {
+			return nil, err // TODO obfuscate
+		}
+
+		hash, err := bcrypt.GenerateFromPassword([]byte(secret), cost)
+		if err != nil {
+			return nil, err // TODO obfuscate
+		}
+
+		hashes = append([]string{string(hash)}, hashes...)
+
+		err = r.secretStorage.Set(ctx, oidcClient.Name, oidcClient.UID, hashes)
+		if err != nil {
+			return nil, err // TODO obfuscate
+		}
+	}
+
+	if req.Spec.RevokeOldSecrets && len(hashes) > 0 {
+		hashes = []string{hashes[0]}
+	}
+
+	// do not let them have more than 100? secrets
+
 	return &clientsecretapi.OIDCClientSecretRequest{
 		Status: clientsecretapi.OIDCClientSecretRequestStatus{
-			GeneratedSecret:    "not-a-real-secret",
+			GeneratedSecret:    secret,
-			TotalClientSecrets: 20,
+			TotalClientSecrets: len(hashes), // TODO what about validation of hashes??
 		},
 	}, nil
 }
@@ -107,3 +159,11 @@ func traceValidationFailure(t *trace.Trace, msg string) {
 		trace.Field{Key: "msg", Value: msg},
 	)
 }
+
+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
+}