From 55fb62d060e312f7859dc39b9ee202402cde11c3 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Thu, 8 Sep 2022 11:41:48 -0400 Subject: [PATCH] Fix OIDCClientSecretStorage.Get to check version, add tests for OIDCClientSecretStorage Co-authored-by: Ryan Richard Co-authored-by: Benjamin A. Petersen --- .../oidcclientsecretstorage.go | 18 +- .../oidcclientsecretstorage_test.go | 428 ++++++++++++++++++ 2 files changed, 442 insertions(+), 4 deletions(-) diff --git a/internal/oidcclientsecretstorage/oidcclientsecretstorage.go b/internal/oidcclientsecretstorage/oidcclientsecretstorage.go index b8eeaf781..70a046969 100644 --- a/internal/oidcclientsecretstorage/oidcclientsecretstorage.go +++ b/internal/oidcclientsecretstorage/oidcclientsecretstorage.go @@ -49,19 +49,29 @@ func New(secrets corev1client.SecretInterface) *OIDCClientSecretStorage { } } +// Get returns the resourceVersion of the storage secret, the hashes within the secret, and an error. +// When the storage secret is not found, it will simply return "", nil, nil to make it easy to pass the +// results of Get directly to Set. func (s *OIDCClientSecretStorage) Get(ctx context.Context, oidcClientUID types.UID) (string, []string, error) { - secret := &storedClientSecret{} - rv, err := s.storage.Get(ctx, uidToName(oidcClientUID), secret) + clientSecret := &storedClientSecret{} + rv, err := s.storage.Get(ctx, uidToName(oidcClientUID), clientSecret) 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 + if clientSecret.Version != oidcClientSecretStorageVersion { + return "", nil, fmt.Errorf("%w: OIDC client secret storage has version %s instead of %s", + ErrOIDCClientSecretStorageVersion, clientSecret.Version, oidcClientSecretStorageVersion) + } + return rv, clientSecret.SecretHashes, nil } +// Set will create or update the values of the storage secret associated with an OIDCClient. +// Set takes the resourceVersion to know if we are doing a create or update and to ensure we do not edit an old version of the storage secret. +// Set takes the oidcClientName to set up the owner reference of the storage secret to that of the OIDCClient. +// Set takes the oidcClientUID to find the correct storage secret. func (s *OIDCClientSecretStorage) Set(ctx context.Context, resourceVersion, oidcClientName string, oidcClientUID types.UID, secretHashes []string) error { secret := &storedClientSecret{ SecretHashes: secretHashes, diff --git a/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go b/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go index 3092e53e7..5f8c3e741 100644 --- a/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go +++ b/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go @@ -4,15 +4,443 @@ package oidcclientsecretstorage import ( + "context" + "errors" "testing" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/fake" + coretesting "k8s.io/client-go/testing" "go.pinniped.dev/internal/testutil" ) +func TestGet(t *testing.T) { + tests := []struct { + name string + uid types.UID + secret *corev1.Secret + wantRV string + wantHashes []string + wantErr string + }{ + { + name: "happy path", + uid: types.UID("some-example-uid1"), + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Namespace: "some-namespace", + UID: "", + ResourceVersion: "123", + Labels: map[string]string{ + "storage.pinniped.dev/type": "oidc-client-secret", + }, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{ + "hashes": ["foo", "bar"], + "version": "1" + }`), + "pinniped-storage-version": []byte("1"), + }, + }, + wantRV: "123", + wantHashes: []string{"foo", "bar"}, + }, + { + name: "incorrect storage version: Data.pinniped-storage-version does not match crud storage version value", + uid: types.UID("some-example-uid1"), + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Namespace: "some-namespace", + UID: "", + ResourceVersion: "123", + Labels: map[string]string{ + "storage.pinniped.dev/type": "oidc-client-secret", + }, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{ + "hashes": ["orcs", "goblins"], + "version": "1" + }`), + "pinniped-storage-version": []byte("9999999999"), + }, + }, + wantRV: "", + wantHashes: []string(nil), + wantErr: "failed to get client secret for uid some-example-uid1: error during get for signature c29tZS1leGFtcGxlLXVpZDE: secret storage data has incorrect version", + }, + { + name: "incorrect storage version: Data.pinniped-storage-data.version does not match package const oidcClientSecretStorageVersion", + uid: types.UID("some-example-uid1"), + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Namespace: "some-namespace", + UID: "", + ResourceVersion: "123", + Labels: map[string]string{ + "storage.pinniped.dev/type": "oidc-client-secret", + }, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{ + "hashes": ["orcs", "goblins"], + "version": "9999999999" + }`), + "pinniped-storage-version": []byte("1"), + }, + }, + wantRV: "", + wantHashes: []string(nil), + wantErr: "OIDC client secret storage data has wrong version: OIDC client secret storage has version 9999999999 instead of 1", + }, { + name: "not found", + uid: types.UID("some-example-uid1"), + secret: nil, + wantRV: "", + wantHashes: nil, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + kubeClient := fake.NewSimpleClientset() + if tt.secret != nil { + require.NoError(t, kubeClient.Tracker().Add(tt.secret)) + } + subject := New(kubeClient.CoreV1().Secrets("some-namespace")) + rv, secretHashes, err := subject.Get(context.Background(), tt.uid) + if tt.wantErr == "" { + require.NoError(t, err) + } else { + require.EqualError(t, err, tt.wantErr) + } + require.Equal(t, tt.wantRV, rv) + require.Equal(t, tt.wantHashes, secretHashes) + }) + } +} + +func TestSet(t *testing.T) { + secretsGVR := schema.GroupVersionResource{ + Group: "", + Version: "v1", + Resource: "secrets", + } + namespace := "some-namespace" + tests := []struct { + name string + rv string + oidcClientName string + oidcClientUID types.UID + hashes []string + seedSecret *corev1.Secret + addReactors func(*fake.Clientset) + wantErr string + wantActions []coretesting.Action + }{ + { + name: "happy path: new secret", + oidcClientName: "some-client", + oidcClientUID: types.UID("some-example-uid1"), + hashes: []string{"foo"}, + seedSecret: nil, + wantActions: []coretesting.Action{ + coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Labels: map[string]string{ + "storage.pinniped.dev/type": "oidc-client-secret", + }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "config.supervisor.pinniped.dev/v1alpha1", + Kind: "OIDCClient", + Name: "some-client", + UID: "some-example-uid1", + }}, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"hashes":["foo"],"version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + }), + }, + }, + { + name: "happy path: update existing secret and maintains existing owner reference", + rv: "9999", + oidcClientName: "some-client", + oidcClientUID: types.UID("some-example-uid1"), + hashes: []string{"foo", "bar"}, + seedSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Namespace: namespace, + Labels: map[string]string{ + "storage.pinniped.dev/type": "oidc-client-secret", + }, + ResourceVersion: "9999", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "config.supervisor.pinniped.dev/v1alpha1", + Kind: "OIDCClient", + Name: "some-client", + UID: "some-example-uid1", + }}, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"hashes":["foo"],"version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + }, + wantActions: []coretesting.Action{ + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq"), + coretesting.NewUpdateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Labels: map[string]string{ + "storage.pinniped.dev/type": "oidc-client-secret", + }, + ResourceVersion: "9999", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "config.supervisor.pinniped.dev/v1alpha1", + Kind: "OIDCClient", + Name: "some-client", + UID: "some-example-uid1", + }}, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"hashes":["foo","bar"],"version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + }), + }, + }, + { + name: "failed to create client secret", + oidcClientName: "some-client", + oidcClientUID: types.UID("some-example-uid1"), + hashes: []string{"foo", "bar"}, + addReactors: func(clientSet *fake.Clientset) { + clientSet.PrependReactor("create", "secrets", func(action coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some create error") + }) + }, + wantActions: []coretesting.Action{ + coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Labels: map[string]string{ + "storage.pinniped.dev/type": "oidc-client-secret", + }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "config.supervisor.pinniped.dev/v1alpha1", + Kind: "OIDCClient", + Name: "some-client", + UID: "some-example-uid1", + }}, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"hashes":["foo","bar"],"version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + }), + }, + wantErr: "failed to create client secret for uid some-example-uid1: failed to create oidc-client-secret for signature c29tZS1leGFtcGxlLXVpZDE: some create error", + }, + { + name: "conflict: editing wrong resource version", + rv: "22", + oidcClientName: "some-client", + oidcClientUID: types.UID("some-example-uid1"), + hashes: []string{"foo", "bar"}, + seedSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Namespace: namespace, + Labels: map[string]string{ + "storage.pinniped.dev/type": "oidc-client-secret", + }, + ResourceVersion: "23", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "config.supervisor.pinniped.dev/v1alpha1", + Kind: "OIDCClient", + Name: "some-client", + UID: "some-example-uid1", + }}, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"hashes":["foo"],"version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + }, + wantActions: []coretesting.Action{ + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq"), + }, + wantErr: "failed to update client secret for uid some-example-uid1: Operation cannot be fulfilled on Secret \"pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq\": resource version 23 does not match expected value: 22", + }, + { + name: "failed to update", + rv: "23", + oidcClientName: "some-client", + oidcClientUID: types.UID("some-example-uid1"), + hashes: []string{"foo", "bar"}, + seedSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Namespace: namespace, + Labels: map[string]string{ + "storage.pinniped.dev/type": "oidc-client-secret", + }, + ResourceVersion: "23", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "config.supervisor.pinniped.dev/v1alpha1", + Kind: "OIDCClient", + Name: "some-client", + UID: "some-example-uid1", + }}, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"hashes":["foo"],"version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + }, + addReactors: func(clientSet *fake.Clientset) { + clientSet.PrependReactor("update", "secrets", func(action coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some update error") + }) + }, + wantActions: []coretesting.Action{ + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq"), + coretesting.NewUpdateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Labels: map[string]string{ + "storage.pinniped.dev/type": "oidc-client-secret", + }, + ResourceVersion: "23", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "config.supervisor.pinniped.dev/v1alpha1", + Kind: "OIDCClient", + Name: "some-client", + UID: "some-example-uid1", + }}, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"hashes":["foo","bar"],"version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + }), + }, + wantErr: "failed to update client secret for uid some-example-uid1: failed to update oidc-client-secret for signature c29tZS1leGFtcGxlLXVpZDE at resource version 23: some update error", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + kubeClient := fake.NewSimpleClientset() + if tt.seedSecret != nil { + require.NoError(t, kubeClient.Tracker().Add(tt.seedSecret)) + } + if tt.addReactors != nil { + tt.addReactors(kubeClient) + } + subject := New(kubeClient.CoreV1().Secrets("some-namespace")) + err := subject.Set( + context.Background(), + tt.rv, + tt.oidcClientName, + tt.oidcClientUID, + tt.hashes, + ) + + if tt.wantErr == "" { + require.NoError(t, err) + } else { + require.EqualError(t, err, tt.wantErr) + } + + require.Equal(t, tt.wantActions, kubeClient.Actions()) + }) + } +} + +func TestGetStorageSecret(t *testing.T) { + tests := []struct { + name string + uid types.UID + secret *corev1.Secret + wantSecret *corev1.Secret + wantErr string + }{ + { + name: "happy path", + uid: types.UID("some-example-uid1"), + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Namespace: "some-namespace", + }, + Data: map[string][]byte{ + "foo": []byte("bar"), + }, + }, + wantSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", + Namespace: "some-namespace", + }, + Data: map[string][]byte{ + "foo": []byte("bar"), + }, + }, + wantErr: "", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + kubeClient := fake.NewSimpleClientset() + require.NoError(t, kubeClient.Tracker().Add(tt.secret)) + subject := New(kubeClient.CoreV1().Secrets("some-namespace")) + + secret, err := subject.GetStorageSecret(context.Background(), tt.uid) + require.Equal(t, tt.wantSecret, secret) + + if tt.wantErr == "" { + require.NoError(t, err) + } else { + require.EqualError(t, err, tt.wantErr) + } + }) + } +} + 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)