diff --git a/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go b/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go index 5f8c3e74..f2b46db0 100644 --- a/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go +++ b/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go @@ -265,38 +265,6 @@ func TestSet(t *testing.T) { }, 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", @@ -326,7 +294,7 @@ func TestSet(t *testing.T) { }, 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") + return true, nil, errors.New("some update error maybe a conflict or something else") }) }, wantActions: []coretesting.Action{ @@ -352,7 +320,7 @@ func TestSet(t *testing.T) { }, }), }, - 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", + 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 maybe a conflict or something else", }, } diff --git a/internal/registry/clientsecretrequest/rest.go b/internal/registry/clientsecretrequest/rest.go index 1df7f045..7494288f 100644 --- a/internal/registry/clientsecretrequest/rest.go +++ b/internal/registry/clientsecretrequest/rest.go @@ -11,7 +11,6 @@ import ( "io" "strings" - "golang.org/x/crypto/bcrypt" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" genericvalidation "k8s.io/apimachinery/pkg/api/validation" @@ -35,23 +34,36 @@ import ( // This value is expected to be increased over time to match CPU improvements. const Cost = 12 -func NewREST(resource schema.GroupResource, secrets corev1client.SecretInterface, clients configv1alpha1clientset.OIDCClientInterface, namespace string, cost int) *REST { +type byteHasher func(password []byte, cost int) ([]byte, error) + +func NewREST( + resource schema.GroupResource, + secretsClient corev1client.SecretInterface, + oidcClientsClient configv1alpha1clientset.OIDCClientInterface, + namespace string, + cost int, + randByteGenerator io.Reader, + byteHasher byteHasher, +) *REST { return &REST{ - secretStorage: oidcclientsecretstorage.New(secrets), - clients: clients, - namespace: namespace, - cost: cost, - tableConvertor: rest.NewDefaultTableConvertor(resource), + secretStorage: oidcclientsecretstorage.New(secretsClient), + oidcClientsClient: oidcClientsClient, + namespace: namespace, + cost: cost, + randByteGenerator: randByteGenerator, + byteHasher: byteHasher, + tableConvertor: rest.NewDefaultTableConvertor(resource), } } type REST struct { - secretStorage *oidcclientsecretstorage.OIDCClientSecretStorage - clients configv1alpha1clientset.OIDCClientInterface - namespace string - rand io.Reader - cost int - tableConvertor rest.TableConvertor + secretStorage *oidcclientsecretstorage.OIDCClientSecretStorage + oidcClientsClient configv1alpha1clientset.OIDCClientInterface + namespace string + randByteGenerator io.Reader + cost int + byteHasher byteHasher + tableConvertor rest.TableConvertor } // Assert that our *REST implements all the optional interfaces that we expect it to implement. @@ -114,17 +126,17 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation t.Step("validateRequest") // Find the specified OIDCClient. - oidcClient, err := r.clients.Get(ctx, req.Name, metav1.GetOptions{}) + oidcClient, err := r.oidcClientsClient.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) + traceFailureWithError(t, "oidcClientsClient.Get", err) return nil, apierrors.NewInternalError(fmt.Errorf("getting client %q failed", req.Name)) } - t.Step("clients.Get") + t.Step("oidcClientsClient.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. @@ -138,14 +150,14 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation // 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) + secret, err = generateSecret(r.randByteGenerator) 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), r.cost) + hash, err := r.byteHasher([]byte(secret), r.cost) if err != nil { traceFailureWithError(t, "bcrypt.GenerateFromPassword", err) return nil, apierrors.NewInternalError(fmt.Errorf("hash generation failed")) @@ -165,8 +177,9 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation 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)) + return nil, apierrors.NewBadRequest( + fmt.Sprintf("OIDCClient %s has too many secrets, spec.revokeOldSecrets must be true", oidcClient.Name), + ) } // Create or update the storage Secret for client secrets. diff --git a/internal/registry/clientsecretrequest/rest_test.go b/internal/registry/clientsecretrequest/rest_test.go index be0f87c5..8928ef7f 100644 --- a/internal/registry/clientsecretrequest/rest_test.go +++ b/internal/registry/clientsecretrequest/rest_test.go @@ -1,43 +1,38 @@ -// NOTES: -// Take a look at the unit tests from the sibling files as we already have some there. -// whoamirequest is super simple, zero side effects... -// - add bits similar to what we did for crud.go to seed kube with secrets & prove your updates -// - perhaps look at oidcclientsecretstorage.go also for the seeding code we wrote -// - the things we are seeding here IS the storage of client secrets + hashes -// - this is the specific secret formats that we need -// - but instead of hello world hashes we will want to put real BCrypt hashes in the seeds -// - there is a test helper with some real BCrypt hashes we can use in oidcclient.go -- HashedPassword1AtGoMinCost - 04 for unit tests for speed -// - look for a test using HashedPassword1AtGoMinCost to see how we step around the min value of 11 tho production code must do that -// - NewKubeStorage, bcrypt.MinCost will show this -// // Use lower minimum required bcrypt cost than we would use in production to keep unit the tests fast. -// oauthStore := oidc.NewKubeStorage(secrets, oidcClientsClient, timeoutsConfiguration, bcrypt.MinCost) -// credentialrequest also has no side effects... -// -// // Copyright 2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 + package clientsecretrequest import ( "context" + "encoding/hex" "errors" + "fmt" + "io" "net/http" + "strings" "testing" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" + kubefake "k8s.io/client-go/kubernetes/fake" + coretesting "k8s.io/client-go/testing" clientsecretapi "go.pinniped.dev/generated/latest/apis/supervisor/clientsecret" + "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" + supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" + "go.pinniped.dev/internal/oidcclientsecretstorage" ) func TestNew(t *testing.T) { - - r := NewREST(schema.GroupResource{Group: "bears", Resource: "panda"}, nil, nil, "foobar", 4) + r := NewREST(schema.GroupResource{Group: "bears", Resource: "panda"}, nil, nil, "foobar", 4, nil, nil) require.NotNil(t, r) require.True(t, r.NamespaceScoped()) @@ -70,6 +65,10 @@ func TestNew(t *testing.T) { } func TestCreate(t *testing.T) { + type wantHashes struct { + UID string + hashes []string + } type args struct { ctx context.Context obj runtime.Object @@ -77,47 +76,68 @@ func TestCreate(t *testing.T) { options *metav1.CreateOptions } namespace := "some-namespace" + namespacedContext := genericapirequest.WithNamespace( + genericapirequest.WithRequestInfo( + genericapirequest.NewContext(), + &genericapirequest.RequestInfo{ + APIGroup: "clientsecret.supervisor.pinniped.dev", + APIVersion: "v1alpha1", + Resource: "oidcclientsecretrequests", + }, + ), + namespace, + ) + + fakeRandomBytes := "0123456789abcdefghijklmnopqrstuv" + fakeHexEncodedRandomBytes := hex.EncodeToString([]byte(fakeRandomBytes)) + fakeBcryptRandomBytes := fakeHexEncodedRandomBytes + ":4-fake-hash" tests := []struct { - name string - args args - want runtime.Object - wantErrStatus metav1.Status + name string + args args + seedOIDCClients []*v1alpha1.OIDCClient + seedHashes func(storage *oidcclientsecretstorage.OIDCClientSecretStorage) + addReactors func(*kubefake.Clientset, *supervisorfake.Clientset) + fakeByteGenerator io.Reader + fakeHasher byteHasher + want runtime.Object + wantErrStatus *metav1.Status + wantHashes *wantHashes }{ { - name: "wrong type", + name: "wrong type of request object provided", args: args{ - ctx: genericapirequest.NewContext(), - obj: &metav1.Status{}, - createValidation: nil, - options: nil, + ctx: namespacedContext, + obj: &metav1.Status{}, }, want: nil, - wantErrStatus: metav1.Status{ - Status: metav1.StatusFailure, - Message: `not an OIDCClientSecretRequest: &v1.Status{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ListMeta:v1.ListMeta{SelfLink:"", ResourceVersion:"", Continue:"", RemainingItemCount:(*int64)(nil)}, Status:"", Message:"", Reason:"", Details:(*v1.StatusDetails)(nil), Code:0}`, - Reason: metav1.StatusReasonBadRequest, - Code: http.StatusBadRequest, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `not an OIDCClientSecretRequest: &v1.Status{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ` + + `ListMeta:v1.ListMeta{SelfLink:"", ResourceVersion:"", Continue:"", RemainingItemCount:(*int64)(nil)},` + + ` Status:"", Message:"", Reason:"", Details:(*v1.StatusDetails)(nil), Code:0}`, + Reason: metav1.StatusReasonBadRequest, + Code: http.StatusBadRequest, }, }, { name: "bad options for dry run", args: args{ - ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + ctx: namespacedContext, obj: &clientsecretapi.OIDCClientSecretRequest{ ObjectMeta: metav1.ObjectMeta{ Name: "client.oauth.pinniped.dev-some-client-name", }, }, - createValidation: nil, - options: &metav1.CreateOptions{DryRun: []string{"stuff"}}, + options: &metav1.CreateOptions{DryRun: []string{"stuff"}}, }, want: nil, - wantErrStatus: metav1.Status{ - Status: metav1.StatusFailure, - Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "client.oauth.pinniped.dev-some-client-name" is invalid: dryRun: Unsupported value: []string{"stuff"}`, - Reason: metav1.StatusReasonInvalid, - Code: http.StatusUnprocessableEntity, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "client.oauth.pinniped.dev-some-client-name" ` + + `is invalid: dryRun: Unsupported value: []string{"stuff"}`, + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, Details: &metav1.StatusDetails{ Group: "clientsecret.supervisor.pinniped.dev", Kind: "OIDCClientSecretRequest", @@ -131,7 +151,7 @@ func TestCreate(t *testing.T) { }, }, { - name: "incorrect namespace", + name: "incorrect namespace on request context", args: args{ ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), "wrong-namespace"), obj: &clientsecretapi.OIDCClientSecretRequest{ @@ -139,11 +159,9 @@ func TestCreate(t *testing.T) { Name: "client.oauth.pinniped.dev-some-client-name", }, }, - createValidation: nil, - options: nil, }, want: nil, - wantErrStatus: metav1.Status{ + wantErrStatus: &metav1.Status{ Status: metav1.StatusFailure, Message: `namespace must be some-namespace on OIDCClientSecretRequest, was wrong-namespace`, Reason: metav1.StatusReasonBadRequest, @@ -153,7 +171,7 @@ func TestCreate(t *testing.T) { { name: "create validation: failure from kube api-server rest.ValidateObjectFunc", args: args{ - ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + ctx: namespacedContext, obj: &clientsecretapi.OIDCClientSecretRequest{ ObjectMeta: metav1.ObjectMeta{ Name: "client.oauth.pinniped.dev-some-client-name", @@ -162,10 +180,9 @@ func TestCreate(t *testing.T) { createValidation: func(ctx context.Context, obj runtime.Object) error { return apierrors.NewInternalError(errors.New("some-error-here")) }, - options: nil, }, want: nil, - wantErrStatus: metav1.Status{ + wantErrStatus: &metav1.Status{ Status: metav1.StatusFailure, Message: "Internal error occurred: some-error-here", Reason: metav1.StatusReasonInternalError, @@ -186,10 +203,9 @@ func TestCreate(t *testing.T) { Name: "client.oauth.pinniped.dev-some-client-name", }, }, - options: nil, }, want: nil, - wantErrStatus: metav1.Status{ + wantErrStatus: &metav1.Status{ Status: metav1.StatusFailure, Message: "Internal error occurred: no namespace information found in request context", Reason: metav1.StatusReasonInternalError, @@ -204,17 +220,16 @@ func TestCreate(t *testing.T) { { name: "create validation: namespace on object does not match namespace on request", args: args{ - ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + ctx: namespacedContext, obj: &clientsecretapi.OIDCClientSecretRequest{ ObjectMeta: metav1.ObjectMeta{ Name: "client.oauth.pinniped.dev-some-client-name", Namespace: "not-a-matching-namespace", }, }, - options: nil, }, want: nil, - wantErrStatus: metav1.Status{ + wantErrStatus: &metav1.Status{ Status: metav1.StatusFailure, Message: "the namespace of the provided object does not match the namespace sent on the request", Reason: metav1.StatusReasonBadRequest, @@ -224,27 +239,27 @@ func TestCreate(t *testing.T) { { name: "create validation: generateName is unsupported", args: args{ - ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + ctx: namespacedContext, obj: &clientsecretapi.OIDCClientSecretRequest{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "foo", }, }, - options: nil, }, want: nil, - wantErrStatus: metav1.Status{ - Status: metav1.StatusFailure, - Message: "OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev \"\" is invalid: [metadata.generateName: Invalid value: \"foo\": generateName is not supported, metadata.name: Required value: name or generateName is required]", - Reason: metav1.StatusReasonInvalid, - Code: http.StatusUnprocessableEntity, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "" is invalid: [metadata.generateName: ` + + `Invalid value: "foo": generateName is not supported, metadata.name: Required value: name or generateName is required]`, + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, Details: &metav1.StatusDetails{ Group: "clientsecret.supervisor.pinniped.dev", Kind: "OIDCClientSecretRequest", Name: "", Causes: []metav1.StatusCause{{ Type: metav1.CauseTypeFieldValueInvalid, - Message: "Invalid value: \"foo\": generateName is not supported", + Message: `Invalid value: "foo": generateName is not supported`, Field: "metadata.generateName", }, { Type: metav1.CauseTypeFieldValueRequired, @@ -257,27 +272,27 @@ func TestCreate(t *testing.T) { { name: "create validation: name cannot exactly match client.oauth.pinniped.dev-", args: args{ - ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + ctx: namespacedContext, obj: &clientsecretapi.OIDCClientSecretRequest{ ObjectMeta: metav1.ObjectMeta{ Name: "client.oauth.pinniped.dev-", }, }, - options: nil, }, want: nil, - wantErrStatus: metav1.Status{ - Status: metav1.StatusFailure, - Message: "OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev \"client.oauth.pinniped.dev-\" is invalid: metadata.name: Invalid value: \"client.oauth.pinniped.dev-\": must not equal 'client.oauth.pinniped.dev-'", - Reason: metav1.StatusReasonInvalid, - Code: http.StatusUnprocessableEntity, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "client.oauth.pinniped.dev-" is invalid: ` + + `metadata.name: Invalid value: "client.oauth.pinniped.dev-": must not equal 'client.oauth.pinniped.dev-'`, + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, Details: &metav1.StatusDetails{ Group: "clientsecret.supervisor.pinniped.dev", Kind: "OIDCClientSecretRequest", Name: "client.oauth.pinniped.dev-", Causes: []metav1.StatusCause{{ Type: metav1.CauseTypeFieldValueInvalid, - Message: "Invalid value: \"client.oauth.pinniped.dev-\": must not equal 'client.oauth.pinniped.dev-'", + Message: `Invalid value: "client.oauth.pinniped.dev-": must not equal 'client.oauth.pinniped.dev-'`, Field: "metadata.name", }}, }, @@ -286,27 +301,27 @@ func TestCreate(t *testing.T) { { name: "create validation: name must contain prefix client.oauth.pinniped.dev-", args: args{ - ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + ctx: namespacedContext, obj: &clientsecretapi.OIDCClientSecretRequest{ ObjectMeta: metav1.ObjectMeta{ Name: "does-not-contain-the-prefix", }, }, - options: nil, }, want: nil, - wantErrStatus: metav1.Status{ - Status: metav1.StatusFailure, - Message: "OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev \"does-not-contain-the-prefix\" is invalid: metadata.name: Invalid value: \"does-not-contain-the-prefix\": must start with 'client.oauth.pinniped.dev-'", - Reason: metav1.StatusReasonInvalid, - Code: http.StatusUnprocessableEntity, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "does-not-contain-the-prefix" is invalid: ` + + `metadata.name: Invalid value: "does-not-contain-the-prefix": must start with 'client.oauth.pinniped.dev-'`, + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, Details: &metav1.StatusDetails{ Group: "clientsecret.supervisor.pinniped.dev", Kind: "OIDCClientSecretRequest", Name: "does-not-contain-the-prefix", Causes: []metav1.StatusCause{{ Type: metav1.CauseTypeFieldValueInvalid, - Message: "Invalid value: \"does-not-contain-the-prefix\": must start with 'client.oauth.pinniped.dev-'", + Message: `Invalid value: "does-not-contain-the-prefix": must start with 'client.oauth.pinniped.dev-'`, Field: "metadata.name", }}, }, @@ -315,20 +330,20 @@ func TestCreate(t *testing.T) { { name: "create validation: name with invalid characters should error", args: args{ - ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + ctx: namespacedContext, obj: &clientsecretapi.OIDCClientSecretRequest{ ObjectMeta: metav1.ObjectMeta{ Name: "client.oauth.pinniped.dev-contains/invalid/characters", }, }, - options: nil, }, want: nil, - wantErrStatus: metav1.Status{ - Status: metav1.StatusFailure, - Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "client.oauth.pinniped.dev-contains/invalid/characters" is invalid: metadata.name: Invalid value: "client.oauth.pinniped.dev-contains/invalid/characters": may not contain '/'`, - Reason: metav1.StatusReasonInvalid, - Code: http.StatusUnprocessableEntity, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "client.oauth.pinniped.dev-contains/invalid/characters" ` + + `is invalid: metadata.name: Invalid value: "client.oauth.pinniped.dev-contains/invalid/characters": may not contain '/'`, + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, Details: &metav1.StatusDetails{ Group: "clientsecret.supervisor.pinniped.dev", Kind: "OIDCClientSecretRequest", @@ -344,21 +359,23 @@ func TestCreate(t *testing.T) { { name: "create validation: name validation may return multiple errors", args: args{ - ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + ctx: namespacedContext, obj: &clientsecretapi.OIDCClientSecretRequest{ ObjectMeta: metav1.ObjectMeta{ Name: "multiple/errors/aggregated", GenerateName: "no-generate-allowed", }, }, - options: nil, }, want: nil, - wantErrStatus: metav1.Status{ - Status: metav1.StatusFailure, - Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "multiple/errors/aggregated" is invalid: [metadata.generateName: Invalid value: "no-generate-allowed": generateName is not supported, metadata.name: Invalid value: "multiple/errors/aggregated": must start with 'client.oauth.pinniped.dev-', metadata.name: Invalid value: "multiple/errors/aggregated": may not contain '/']`, - Reason: metav1.StatusReasonInvalid, - Code: http.StatusUnprocessableEntity, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "multiple/errors/aggregated" is invalid: [metadata.generateName: ` + + `Invalid value: "no-generate-allowed": generateName is not supported, metadata.name: ` + + `Invalid value: "multiple/errors/aggregated": must start with 'client.oauth.pinniped.dev-', metadata.name: ` + + `Invalid value: "multiple/errors/aggregated": may not contain '/']`, + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, Details: &metav1.StatusDetails{ Group: "clientsecret.supervisor.pinniped.dev", Kind: "OIDCClientSecretRequest", @@ -379,26 +396,913 @@ func TestCreate(t *testing.T) { }, }, }, - // {name: ""}, + { + name: "oidcClient does not exist 404", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-oidc-client-does-not-exist-404", + }, + }, + }, + want: nil, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "client.oauth.pinniped.dev-oidc-client-does-not-exist-404" ` + + `is invalid: metadata.name: Not found: "client.oauth.pinniped.dev-oidc-client-does-not-exist-404"`, + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, + Details: &metav1.StatusDetails{ + Group: "clientsecret.supervisor.pinniped.dev", + Kind: "OIDCClientSecretRequest", + Name: "client.oauth.pinniped.dev-oidc-client-does-not-exist-404", + Causes: []metav1.StatusCause{{ + Type: metav1.CauseTypeFieldValueNotFound, + Message: `Not found: "client.oauth.pinniped.dev-oidc-client-does-not-exist-404"`, + Field: "metadata.name", + }}, + }, + }, + }, + { + name: "unexpected error getting oidcClient 500", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-internal-error-could-not-get-client", + }, + }, + }, + addReactors: func(kubeClient *kubefake.Clientset, supervisorClient *supervisorfake.Clientset) { + supervisorClient.PrependReactor("get", "oidcclients", func(action coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("unexpected error darn") + }) + }, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusInternalServerError, + Reason: metav1.StatusReasonInternalError, + Message: `Internal error occurred: getting client "client.oauth.pinniped.dev-internal-error-could-not-get-client" failed`, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{{ + Message: `getting client "client.oauth.pinniped.dev-internal-error-could-not-get-client" failed`, + }}, + }, + }, + }, + { + name: "failed to get kube secret for oidcClient", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-no-secret-for-oidcclient", + }, + }, + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-no-secret-for-oidcclient", + Namespace: namespace, + }, + }}, + addReactors: func(kubeClient *kubefake.Clientset, supervisorClient *supervisorfake.Clientset) { + kubeClient.PrependReactor("get", "secrets", func(action coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("sadly no secrets") + }) + }, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusInternalServerError, + Reason: metav1.StatusReasonInternalError, + Message: `Internal error occurred: getting secret for client "client.oauth.pinniped.dev-no-secret-for-oidcclient" failed`, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{{ + Message: `getting secret for client "client.oauth.pinniped.dev-no-secret-for-oidcclient" failed`, + }}, + }, + }, + }, + { + name: "failed to generate new client secret for oidcClient", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-fail-to-generate-secret", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: false, + }, + }, + }, + fakeByteGenerator: readerAlwaysErrors{}, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-fail-to-generate-secret", + Namespace: namespace, + }, + }}, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusInternalServerError, + Reason: metav1.StatusReasonInternalError, + Message: `Internal error occurred: client secret generation failed`, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{{ + Message: `client secret generation failed`, + }}, + }, + }, + }, + { + name: "failed to generate hash for new client secret for oidcClient", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-fail-to-hash-secret", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: false, + }, + }, + }, + fakeHasher: func(password []byte, cost int) ([]byte, error) { + return nil, errors.New("can't hash stuff") + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-fail-to-hash-secret", + Namespace: namespace, + }, + }}, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusInternalServerError, + Reason: metav1.StatusReasonInternalError, + Message: `Internal error occurred: hash generation failed`, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{{ + Message: `hash generation failed`, + }}, + }, + }, + }, + { + name: "happy path: no secrets exist, create secret and hash for found oidcclient", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-happy-new-secret", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: false, + }, + }, + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-happy-new-secret", + Namespace: namespace, + UID: "12345", + }, + }}, + want: &clientsecretapi.OIDCClientSecretRequest{ + Status: clientsecretapi.OIDCClientSecretRequestStatus{ + GeneratedSecret: fakeHexEncodedRandomBytes, + TotalClientSecrets: 1, + }, + }, + wantErrStatus: nil, + wantHashes: &wantHashes{ + UID: "12345", + hashes: []string{ + fakeBcryptRandomBytes, + }, + }, + }, + { + name: "happy path: secret exists, prepend new secret hash to secret to the list of hashes for found oidcclient", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-append-new-secret-hash", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: false, + }, + }, + }, + seedHashes: func(storage *oidcclientsecretstorage.OIDCClientSecretStorage) { + require.NoError(t, + storage.Set( + context.Background(), + "", + "client.oauth.pinniped.dev-append-new-secret-hash", + "12345", + []string{ + "hashed-password-1", + "hashed-password-2", + }, + ), + ) + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-append-new-secret-hash", + Namespace: namespace, + UID: "12345", + }, + }}, + wantHashes: &wantHashes{ + UID: "12345", + hashes: []string{ + fakeBcryptRandomBytes, + "hashed-password-1", + "hashed-password-2", + }, + }, + want: &clientsecretapi.OIDCClientSecretRequest{ + Status: clientsecretapi.OIDCClientSecretRequestStatus{ + GeneratedSecret: fakeHexEncodedRandomBytes, + TotalClientSecrets: 3, + }, + }, + }, + { + name: "happy path: secret exists, append new secret hash to secret and revoke old for found oidcclient", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-append-new-secret-hash", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: true, + }, + }, + }, + seedHashes: func(storage *oidcclientsecretstorage.OIDCClientSecretStorage) { + require.NoError(t, + storage.Set( + context.Background(), + "", + "client.oauth.pinniped.dev-append-new-secret-hash", + "12345", + []string{ + "hashed-password-1", + "hashed-password-2", + }, + )) + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-append-new-secret-hash", + Namespace: namespace, + UID: "12345", + }, + }}, + wantHashes: &wantHashes{ + UID: "12345", + hashes: []string{ + fakeBcryptRandomBytes, + }, + }, + want: &clientsecretapi.OIDCClientSecretRequest{ + Status: clientsecretapi.OIDCClientSecretRequestStatus{ + GeneratedSecret: fakeHexEncodedRandomBytes, + TotalClientSecrets: 1, + }, + }, + }, + { + name: "happy path: secret exists, revoke old secrets but retain latest for found oidcclient", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: false, + RevokeOldSecrets: true, + }, + }, + }, + seedHashes: func(storage *oidcclientsecretstorage.OIDCClientSecretStorage) { + require.NoError(t, + storage.Set( + context.Background(), + "", + "client.oauth.pinniped.dev-some-client", + "12345", + []string{ + "hashed-password-1", + "hashed-password-2", + }, + )) + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + Namespace: namespace, + UID: "12345", + }, + }}, + wantHashes: &wantHashes{ + UID: "12345", + hashes: []string{ + "hashed-password-1", + }, + }, + want: &clientsecretapi.OIDCClientSecretRequest{ + Status: clientsecretapi.OIDCClientSecretRequestStatus{ + GeneratedSecret: "", + TotalClientSecrets: 1, + }, + }, + }, + { + name: "secret exists but oidcclient secret has too many hashes, fails to create when RevokeOldSecrets:false (max 5), secret is not updated", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: false, + }, + }, + }, + seedHashes: func(storage *oidcclientsecretstorage.OIDCClientSecretStorage) { + require.NoError(t, + storage.Set( + context.Background(), + "", + "client.oauth.pinniped.dev-some-client", + "12345", + []string{ + "hashed-password-1", + "hashed-password-2", + "hashed-password-3", + "hashed-password-4", + "hashed-password-5", + }, + )) + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + Namespace: namespace, + UID: "12345", + }, + }}, + wantHashes: &wantHashes{ + UID: "12345", + hashes: []string{ + "hashed-password-1", + "hashed-password-2", + "hashed-password-3", + "hashed-password-4", + "hashed-password-5", + }, + }, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClient client.oauth.pinniped.dev-some-client has too many secrets, spec.revokeOldSecrets must be true`, + Reason: metav1.StatusReasonBadRequest, + Code: http.StatusBadRequest, + }, + }, + { + name: "secret exists but oidcclient secret has too many hashes, fails to create when RevokeOldSecrets:false (greater than 5), secret is not updated", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: false, + }, + }, + }, + seedHashes: func(storage *oidcclientsecretstorage.OIDCClientSecretStorage) { + require.NoError(t, + storage.Set( + context.Background(), + "", + "client.oauth.pinniped.dev-some-client", + "12345", + []string{ + "hashed-password-1", + "hashed-password-2", + "hashed-password-3", + "hashed-password-4", + "hashed-password-5", + "hashed-password-6", + }, + )) + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + Namespace: namespace, + UID: "12345", + }, + }}, + wantHashes: &wantHashes{ + UID: "12345", + hashes: []string{ + "hashed-password-1", + "hashed-password-2", + "hashed-password-3", + "hashed-password-4", + "hashed-password-5", + "hashed-password-6", + }, + }, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClient client.oauth.pinniped.dev-some-client has too many secrets, spec.revokeOldSecrets must be true`, + Reason: metav1.StatusReasonBadRequest, + Code: http.StatusBadRequest, + }, + }, + { + name: "attempted to create storage secret because it did not initially exist but was created by someone else while generating new client secret & hash", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: false, + }, + }, + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + Namespace: namespace, + UID: "12345", + }, + }}, + addReactors: func(kubeClient *kubefake.Clientset, supervisorClient *supervisorfake.Clientset) { + kubeClient.PrependReactor("create", "secrets", func(action coretesting.Action) (bool, runtime.Object, error) { + secret := action.(coretesting.UpdateAction).GetObject().(*corev1.Secret) + return true, nil, apierrors.NewAlreadyExists(schema.GroupResource{Group: "", Resource: "secrets"}, secret.Name) + }) + }, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `Operation cannot be fulfilled on oidcclientsecretrequests.clientsecret.supervisor.pinniped.dev ` + + `"client.oauth.pinniped.dev-some-client": multiple concurrent secret generation requests for same client`, + Reason: metav1.StatusReasonConflict, + Code: http.StatusConflict, + Details: &metav1.StatusDetails{ + Group: "clientsecret.supervisor.pinniped.dev", + Kind: "oidcclientsecretrequests", + Name: "client.oauth.pinniped.dev-some-client", + }, + }, + }, + { + name: "attempted to create storage secret because it did not initially exist but received a conflict error", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: false, + }, + }, + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + Namespace: namespace, + UID: "12345", + }, + }}, + addReactors: func(kubeClient *kubefake.Clientset, supervisorClient *supervisorfake.Clientset) { + kubeClient.PrependReactor("create", "secrets", func(action coretesting.Action) (bool, runtime.Object, error) { + secret := action.(coretesting.UpdateAction).GetObject().(*corev1.Secret) + return true, nil, apierrors.NewConflict( + schema.GroupResource{Group: "", Resource: "secrets"}, + secret.Name, + errors.New("something deeply conflicted"), + ) + }) + }, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `Operation cannot be fulfilled on oidcclientsecretrequests.clientsecret.supervisor.pinniped.dev ` + + `"client.oauth.pinniped.dev-some-client": multiple concurrent secret generation requests for same client`, + Reason: metav1.StatusReasonConflict, + Code: http.StatusConflict, + Details: &metav1.StatusDetails{ + Group: "clientsecret.supervisor.pinniped.dev", + Kind: "oidcclientsecretrequests", + Name: "client.oauth.pinniped.dev-some-client", + }, + }, + }, + { + name: "attempted to create storage secret but received an unknown error", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: false, + }, + }, + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + Namespace: namespace, + UID: "12345", + }, + }}, + addReactors: func(kubeClient *kubefake.Clientset, supervisorClient *supervisorfake.Clientset) { + kubeClient.PrependReactor("create", "secrets", func(action coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some random error") + }) + }, + wantErrStatus: &metav1.Status{ + Status: metav1.StatusFailure, + Message: `Internal error occurred: setting client secret failed`, + Reason: metav1.StatusReasonInternalError, + Code: http.StatusInternalServerError, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{{ + Message: "setting client secret failed", + }}, + }, + }, + }, + { + name: "happy path noop: do not create a new secret, revoke old secrets, but there is no existing storage secret", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-happy-new-secret", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: false, + RevokeOldSecrets: true, + }, + }, + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-happy-new-secret", + Namespace: namespace, + UID: "12345", + }, + }}, + want: &clientsecretapi.OIDCClientSecretRequest{ + Status: clientsecretapi.OIDCClientSecretRequestStatus{ + GeneratedSecret: "", + TotalClientSecrets: 0, + }, + }, + }, + { + name: "happy path noop: do not create a new secret, revoke old secrets, but there is no existing storage secret", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: false, + RevokeOldSecrets: false, + }, + }, + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + Namespace: namespace, + UID: "12345", + }, + }}, + want: &clientsecretapi.OIDCClientSecretRequest{ + Status: clientsecretapi.OIDCClientSecretRequestStatus{ + GeneratedSecret: "", + TotalClientSecrets: 0, + }, + }, + }, + { + name: "happy path noop: do not create a new secret, revoke old secrets, and there is an existing storage secret", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: false, + RevokeOldSecrets: false, + }, + }, + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + Namespace: namespace, + UID: "12345", + }, + }}, + seedHashes: func(storage *oidcclientsecretstorage.OIDCClientSecretStorage) { + require.NoError(t, + storage.Set( + context.Background(), + "", + "client.oauth.pinniped.dev-some-client", + "12345", + []string{ + "hashed-password-1", + "hashed-password-2", + }, + )) + }, + wantHashes: &wantHashes{ + UID: "12345", + hashes: []string{ + "hashed-password-1", + "hashed-password-2", + }, + }, + want: &clientsecretapi.OIDCClientSecretRequest{ + Status: clientsecretapi.OIDCClientSecretRequestStatus{ + GeneratedSecret: "", + TotalClientSecrets: 2, + }, + }, + }, + { + name: "happy path: generate new secret and revoking old secret when there was a single secret hash to start with", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: true, + }, + }, + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + Namespace: namespace, + UID: "12345", + }, + }}, + seedHashes: func(storage *oidcclientsecretstorage.OIDCClientSecretStorage) { + require.NoError(t, + storage.Set( + context.Background(), + "", + "client.oauth.pinniped.dev-some-client", + "12345", + []string{ + "hashed-password-1", + }, + )) + }, + wantHashes: &wantHashes{ + UID: "12345", + hashes: []string{ + fakeBcryptRandomBytes, + }, + }, + want: &clientsecretapi.OIDCClientSecretRequest{ + Status: clientsecretapi.OIDCClientSecretRequestStatus{ + GeneratedSecret: fakeHexEncodedRandomBytes, + TotalClientSecrets: 1, + }, + }, + }, + { + name: "happy path: generate new secret when existing secrets is max (5)", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: true, + }, + }, + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + Namespace: namespace, + UID: "12345", + }, + }}, + seedHashes: func(storage *oidcclientsecretstorage.OIDCClientSecretStorage) { + require.NoError(t, + storage.Set( + context.Background(), + "", + "client.oauth.pinniped.dev-some-client", + "12345", + []string{ + "hashed-password-1", + "hashed-password-2", + "hashed-password-3", + "hashed-password-4", + "hashed-password-5", + }, + )) + }, + wantHashes: &wantHashes{ + UID: "12345", + hashes: []string{ + fakeBcryptRandomBytes, + }, + }, + want: &clientsecretapi.OIDCClientSecretRequest{ + Status: clientsecretapi.OIDCClientSecretRequestStatus{ + GeneratedSecret: fakeHexEncodedRandomBytes, + TotalClientSecrets: 1, + }, + }, + }, + { + name: "happy path: generate new secret when existing secrets exceeds maximum (5)", + args: args{ + ctx: namespacedContext, + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + }, + Spec: clientsecretapi.OIDCClientSecretRequestSpec{ + GenerateNewSecret: true, + RevokeOldSecrets: true, + }, + }, + }, + seedOIDCClients: []*v1alpha1.OIDCClient{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client", + Namespace: namespace, + UID: "12345", + }, + }}, + seedHashes: func(storage *oidcclientsecretstorage.OIDCClientSecretStorage) { + require.NoError(t, + storage.Set( + context.Background(), + "", + "client.oauth.pinniped.dev-some-client", + "12345", + []string{ + "hashed-password-1", + "hashed-password-2", + "hashed-password-3", + "hashed-password-4", + "hashed-password-5", + "hashed-password-6", + }, + )) + }, + wantHashes: &wantHashes{ + UID: "12345", + hashes: []string{ + fakeBcryptRandomBytes, + }, + }, + want: &clientsecretapi.OIDCClientSecretRequest{ + Status: clientsecretapi.OIDCClientSecretRequestStatus{ + GeneratedSecret: fakeHexEncodedRandomBytes, + TotalClientSecrets: 1, + }, + }, + }, } for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - // TODO: update and fill these in with actual values, not nil! - r := NewREST(schema.GroupResource{Group: "bears", Resource: "panda"}, nil, nil, namespace, 4) + t.Parallel() + + kubeClient := kubefake.NewSimpleClientset() + secretsClient := kubeClient.CoreV1().Secrets(namespace) + // Production code depends on secrets having a resource version. + // Our seedHashes mechanism with the fake client unfortunately does not cause a resourceVersion to be set on the secret. + // Therefore, we need to add this reactor before we seed hashes so our secrets have RVs. + kubeClient.PrependReactor("create", "secrets", func(action coretesting.Action) (bool, runtime.Object, error) { + secret := action.(coretesting.UpdateAction).GetObject().(*corev1.Secret) + secret.ResourceVersion = "1" + return false, nil, nil + }) + + oidcClientSecretStore := oidcclientsecretstorage.New(secretsClient) + if tt.seedHashes != nil { + tt.seedHashes(oidcClientSecretStore) + } + + supervisorClient := supervisorfake.NewSimpleClientset() + if tt.seedOIDCClients != nil { + for _, client := range tt.seedOIDCClients { + require.NoError(t, supervisorClient.Tracker().Add(client)) + } + } + oidcClientClient := supervisorClient.ConfigV1alpha1().OIDCClients(namespace) + + if tt.addReactors != nil { + tt.addReactors(kubeClient, supervisorClient) + } + + fakeHasher := tt.fakeHasher + if tt.fakeHasher == nil { + fakeHasher = func(password []byte, cost int) ([]byte, error) { + return []byte(fmt.Sprintf("%s:%d-fake-hash", password, cost)), nil + } + } + fakeByteGenerator := tt.fakeByteGenerator + if tt.fakeByteGenerator == nil { + fakeByteGenerator = strings.NewReader(fakeRandomBytes + "these extra bytes should be ignored since we only read 32 bytes") + } + + r := NewREST( + schema.GroupResource{Group: "bears", Resource: "panda"}, + secretsClient, + oidcClientClient, + namespace, + 4, + fakeByteGenerator, + fakeHasher, + ) + got, err := r.Create(tt.args.ctx, tt.args.obj, tt.args.createValidation, tt.args.options) - require.Equal(t, &apierrors.StatusError{ - ErrStatus: tt.wantErrStatus, - }, err) + require.Equal(t, tt.want, got) + if tt.wantErrStatus != nil { + require.Equal(t, &apierrors.StatusError{ErrStatus: *tt.wantErrStatus}, err) + } else { + require.NoError(t, err) + } + + if tt.wantHashes != nil { + secretStoreName := oidcClientSecretStore.GetName(types.UID(tt.wantHashes.UID)) + secretGVR := schema.GroupVersionResource{ + Group: corev1.SchemeGroupVersion.Group, + Version: corev1.SchemeGroupVersion.Version, + Resource: "secrets", + } + storeSecret, err := kubeClient.Tracker().Get(secretGVR, namespace, secretStoreName) + require.NoError(t, err) + require.IsType(t, &corev1.Secret{}, storeSecret) + secretHashes, err := oidcclientsecretstorage.ReadFromSecret(storeSecret.(*corev1.Secret)) + require.NoError(t, err) + require.Equal(t, tt.wantHashes.hashes, secretHashes) + } else { + secrets, err := secretsClient.List(context.Background(), metav1.ListOptions{}) + require.NoError(t, err) + require.Empty(t, secrets.Items) + } }) } } -func errString(err error) string { - if err == nil { - return "" - } +type readerAlwaysErrors struct{} - return err.Error() +func (r readerAlwaysErrors) Read(p []byte) (n int, err error) { + return 0, errors.New("always errors") } diff --git a/internal/supervisor/apiserver/apiserver.go b/internal/supervisor/apiserver/apiserver.go index a6deec6d..2adb83c1 100644 --- a/internal/supervisor/apiserver/apiserver.go +++ b/internal/supervisor/apiserver/apiserver.go @@ -5,9 +5,11 @@ package apiserver import ( "context" + "crypto/rand" "fmt" "sync" + "golang.org/x/crypto/bcrypt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -80,7 +82,15 @@ 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(), c.ExtraConfig.Secrets, c.ExtraConfig.OIDCClients, c.ExtraConfig.Namespace, clientsecretrequest.Cost) + clientSecretReqStorage := clientsecretrequest.NewREST( + clientSecretReqGVR.GroupResource(), + c.ExtraConfig.Secrets, + c.ExtraConfig.OIDCClients, + c.ExtraConfig.Namespace, + clientsecretrequest.Cost, + rand.Reader, + bcrypt.GenerateFromPassword, + ) return clientSecretReqGVR, clientSecretReqStorage }, } {