From 2e36cea7867cef5ff6852ed3e330f072d849282c Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Thu, 14 Jul 2022 17:07:59 -0400 Subject: [PATCH] wip002 Signed-off-by: Monis Khan --- .../oidcclientwatcher/oidc_client_watcher.go | 2 +- internal/crud/crud.go | 52 +++++++---- .../oidcclientsecretstorage.go | 88 +++++++++++++++---- .../oidcclientsecretstorage_test.go | 4 +- internal/registry/clientsecretrequest/rest.go | 72 +++++++++++++-- 5 files changed, 174 insertions(+), 44 deletions(-) diff --git a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go index eb6ab992..18b47a36 100644 --- a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go +++ b/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. diff --git a/internal/crud/crud.go b/internal/crud/crud.go index 29ad6b65..59623cc0 100644 --- a/internal/crud/crud.go +++ b/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) { - secret, err := s.toSecret(signature, "", data, additionalLabels) +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, 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 - // 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) + secret, err := s.toSecret(signature, resourceVersion, data, nil, 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{ - SecretLabelKey: s.resource, // make it easier to find this stuff via kubectl - } + labelsToAdd := make(map[string]string, len(additionalLabels)+1) 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{ - SecretLifetimeAnnotationKey: s.clock().Add(s.lifetime).UTC().Format(SecretLifetimeAnnotationDateFormat), - }, - OwnerReferences: nil, + Annotations: annotations, + OwnerReferences: ownerReferences, }, Data: map[string][]byte{ secretDataKey: buf, diff --git a/internal/oidcclientsecretstorage/oidcclientsecretstorage.go b/internal/oidcclientsecretstorage/oidcclientsecretstorage.go index 257e674c..15c6fed5 100644 --- a/internal/oidcclientsecretstorage/oidcclientsecretstorage.go +++ b/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 { - // TODO make lifetime = 0 mean that it does not get annotated with any garbage collection annotation - return &OIDCClientSecretStorage{storage: crud.New(TypeLabelValue, secrets, clock, 0)} +func New(secrets corev1client.SecretInterface) *OIDCClientSecretStorage { + return &OIDCClientSecretStorage{storage: crud.New(TypeLabelValue, secrets, nil, 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. - b64encodedUID := base64.RawURLEncoding.EncodeToString([]byte(oidcClientUID)) - return s.storage.GetName(b64encodedUID) + return s.storage.GetName(uidToName(oidcClientUID)) } -// ReadFromSecret reads the contents of a Secret as a StoredClientSecret. -func ReadFromSecret(secret *v1.Secret) (*StoredClientSecret, error) { - storedClientSecret := &StoredClientSecret{} - err := crud.FromSecret(TypeLabelValue, secret, storedClientSecret) +func uidToName(oidcClientUID types.UID) string { + // Avoid having s.storage.GetName() base64 decode something that wasn't ever encoded by encoding it here. + return base64.RawURLEncoding.EncodeToString([]byte(oidcClientUID)) +} + +// 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 } diff --git a/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go b/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go index ac81565a..b83e8980 100644 --- a/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go +++ b/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"}, }, diff --git a/internal/registry/clientsecretrequest/rest.go b/internal/registry/clientsecretrequest/rest.go index d688d74d..6252c1ae 100644 --- a/internal/registry/clientsecretrequest/rest.go +++ b/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 - - _, err := validateRequest(obj, t) + req, 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", - TotalClientSecrets: 20, + GeneratedSecret: secret, + 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 +}