diff --git a/internal/crud/crud.go b/internal/crud/crud.go index e9982734..d1edeca2 100644 --- a/internal/crud/crud.go +++ b/internal/crud/crud.go @@ -135,6 +135,9 @@ func (s *secretsStorage) DeleteByLabel(ctx context.Context, labelName string, la if err != nil { return fmt.Errorf(`failed to list secrets for resource "%s" matching label "%s=%s": %w`, s.resource, labelName, labelValue, err) } + if len(list.Items) == 0 { + return fmt.Errorf(`failed to delete secrets for resource "%s" matching label "%s=%s": none found`, s.resource, labelName, labelValue) + } // TODO try to delete all of the items and consolidate all of the errors and return them all for _, secret := range list.Items { err = s.secrets.Delete(ctx, secret.Name, metav1.DeleteOptions{}) @@ -162,22 +165,21 @@ func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, return nil, fmt.Errorf("failed to encode secret data for %s: %w", s.getName(signature), err) } - labels := map[string]string{ + labelsToAdd := map[string]string{ SecretLabelKey: s.resource, // make it easier to find this stuff via kubectl } for labelName, labelValue := range additionalLabels { - labels[labelName] = labelValue - } - annotations := map[string]string{ - SecretLifetimeAnnotationKey: s.clock().Add(s.lifetime).UTC().String(), + labelsToAdd[labelName] = labelValue } return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: s.getName(signature), ResourceVersion: resourceVersion, - Labels: labels, - Annotations: annotations, + Labels: labelsToAdd, + Annotations: map[string]string{ + SecretLifetimeAnnotationKey: s.clock().Add(s.lifetime).UTC().Format(time.RFC3339), + }, OwnerReferences: nil, }, Data: map[string][]byte{ diff --git a/internal/crud/crud_test.go b/internal/crud/crud_test.go index 9e4ed216..8a910b8a 100644 --- a/internal/crud/crud_test.go +++ b/internal/crud/crud_test.go @@ -18,6 +18,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/clock" "k8s.io/client-go/kubernetes/fake" coretesting "k8s.io/client-go/testing" ) @@ -46,8 +47,9 @@ func TestStorage(t *testing.T) { validateSecretName := validation.NameIsDNSSubdomain // matches k/k - var fakeNow = time.Date(2030, time.January, 1, 0, 0, 0, 0, time.UTC) - var fakeDuration = time.Minute * 10 + fakeNow := time.Date(2030, time.January, 1, 0, 0, 0, 0, time.UTC) + lifetime := time.Minute * 10 + fakeNowPlusLifetimeAsString := metav1.Time{Time: fakeNow.Add(lifetime)}.Format(time.RFC3339) const ( namespace = "test-ns" @@ -60,7 +62,7 @@ func TestStorage(t *testing.T) { name string resource string mocks func(*testing.T, mocker) - run func(*testing.T, Storage) error + run func(*testing.T, Storage, *clock.FakeClock) error wantActions []coretesting.Action wantSecrets []corev1.Secret wantErr string @@ -69,7 +71,7 @@ func TestStorage(t *testing.T) { name: "get non-existent", resource: "authcode", mocks: nil, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { _, err := storage.Get(ctx, "not-exists", nil) return err }, @@ -83,7 +85,7 @@ func TestStorage(t *testing.T) { name: "delete non-existent", resource: "tokens", mocks: nil, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { return storage.Delete(ctx, "not-a-token") }, wantActions: []coretesting.Action{ @@ -92,12 +94,26 @@ func TestStorage(t *testing.T) { wantSecrets: nil, wantErr: `failed to delete tokens for signature not-a-token: secrets "pinniped-storage-tokens-t2fx427lnci6s" not found`, }, - // TODO make a delete non-existent test for DeleteByLabel + { + name: "delete non-existent by label", + resource: "tokens", + mocks: nil, + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { + return storage.DeleteByLabel(ctx, "additionalLabel", "matching-value") + }, + wantActions: []coretesting.Action{ + coretesting.NewListAction(secretsGVR, schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, namespace, metav1.ListOptions{ + LabelSelector: "storage.pinniped.dev/type=tokens,additionalLabel=matching-value", + }), + }, + wantSecrets: nil, + wantErr: `failed to delete secrets for resource "tokens" matching label "additionalLabel=matching-value": none found`, + }, { name: "create and get", resource: "access-tokens", mocks: nil, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { signature := hmac.AuthorizeCodeSignature(authorizationCode1) require.NotEmpty(t, signature) require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is @@ -124,9 +140,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "access-tokens", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -147,9 +161,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "access-tokens", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -161,11 +173,106 @@ func TestStorage(t *testing.T) { }, wantErr: "", }, + { + name: "create multiple, each gets the correct lifetime timestamp", + resource: "access-tokens", + mocks: nil, + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { + data := &testJSON{Data: "create1"} + rv1, err := storage.Create(ctx, "sig1", data, nil) + require.Empty(t, rv1) // fake client does not set this + require.NoError(t, err) + + fakeClock.Step(42 * time.Minute) // simulate that a known amount of time has passed + + data = &testJSON{Data: "create2"} + rv1, err = storage.Create(ctx, "sig2", data, nil) + require.Empty(t, rv1) // fake client does not set this + require.NoError(t, err) + + return nil + }, + wantActions: []coretesting.Action{ + coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-tokens-wiudk", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-tokens", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"create1"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-tokens", + }), + coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-tokens-wiudm", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-tokens", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{Time: fakeNow.Add(42 * time.Minute).Add(lifetime)}.Format(time.RFC3339), + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"create2"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-tokens", + }), + }, + wantSecrets: []corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-tokens-wiudk", + Namespace: namespace, + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-tokens", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"create1"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-tokens", + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-tokens-wiudm", + Namespace: namespace, + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-tokens", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{Time: fakeNow.Add(42 * time.Minute).Add(lifetime)}.Format(time.RFC3339), + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"create2"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-tokens", + }, + }, + wantErr: "", + }, { name: "create and get with additional labels", resource: "access-tokens", mocks: nil, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { signature := hmac.AuthorizeCodeSignature(authorizationCode1) require.NotEmpty(t, signature) require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is @@ -194,9 +301,7 @@ func TestStorage(t *testing.T) { "label2": "value2", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -219,9 +324,7 @@ func TestStorage(t *testing.T) { "label2": "value2", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -246,9 +349,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "pandas-are-best", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -259,7 +360,7 @@ func TestStorage(t *testing.T) { }) require.NoError(t, err) }, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { signature := hmac.AuthorizeCodeSignature(authorizationCode2) require.NotEmpty(t, signature) require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is @@ -286,9 +387,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "pandas-are-best", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -313,9 +412,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "stores", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -332,7 +429,7 @@ func TestStorage(t *testing.T) { return false, nil, nil // we mutated the secret in place but we do not "handle" it }) }, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { signature := hmac.AuthorizeCodeSignature(authorizationCode3) require.NotEmpty(t, signature) require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is @@ -367,9 +464,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "stores", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -390,9 +485,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "stores", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -417,9 +510,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "seals", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -430,7 +521,7 @@ func TestStorage(t *testing.T) { }) require.NoError(t, err) }, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { signature := hmac.AuthorizeCodeSignature(authorizationCode2) require.NotEmpty(t, signature) require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is @@ -457,9 +548,7 @@ func TestStorage(t *testing.T) { "additionalLabel": "matching-value", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -478,9 +567,7 @@ func TestStorage(t *testing.T) { "additionalLabel": "matching-value", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -499,9 +586,7 @@ func TestStorage(t *testing.T) { "additionalLabel": "non-matching-value", // different value for the same label }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -520,9 +605,7 @@ func TestStorage(t *testing.T) { "additionalLabel": "matching-value", // same value for the same label as above }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -532,7 +615,7 @@ func TestStorage(t *testing.T) { Type: "storage.pinniped.dev/walruses", })) }, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { return storage.DeleteByLabel(ctx, "additionalLabel", "matching-value") }, wantActions: []coretesting.Action{ @@ -554,9 +637,7 @@ func TestStorage(t *testing.T) { "additionalLabel": "non-matching-value", // different value for the same label }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -576,9 +657,7 @@ func TestStorage(t *testing.T) { "additionalLabel": "matching-value", // same value for the same label as above }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -604,9 +683,7 @@ func TestStorage(t *testing.T) { "additionalLabel": "matching-value", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -619,7 +696,7 @@ func TestStorage(t *testing.T) { return true, nil, fmt.Errorf("some delete error") }) }, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { return storage.DeleteByLabel(ctx, "additionalLabel", "matching-value") }, wantActions: []coretesting.Action{ @@ -639,9 +716,7 @@ func TestStorage(t *testing.T) { "additionalLabel": "matching-value", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -674,7 +749,7 @@ func TestStorage(t *testing.T) { return true, nil, fmt.Errorf("some listing error") }) }, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { return storage.DeleteByLabel(ctx, "additionalLabel", "matching-value") }, wantActions: []coretesting.Action{ @@ -697,9 +772,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "candies", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -710,7 +783,7 @@ func TestStorage(t *testing.T) { }) require.NoError(t, err) }, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { signature := hmac.AuthorizeCodeSignature(authorizationCode3) require.NotEmpty(t, signature) require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is @@ -737,9 +810,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "candies", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -764,9 +835,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "candies-are-bad", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -777,7 +846,7 @@ func TestStorage(t *testing.T) { }) require.NoError(t, err) }, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { signature := hmac.AuthorizeCodeSignature(authorizationCode3) require.NotEmpty(t, signature) require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is @@ -804,9 +873,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "candies-are-bad", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -831,9 +898,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "candies", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -844,7 +909,7 @@ func TestStorage(t *testing.T) { }) require.NoError(t, err) }, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { signature := hmac.AuthorizeCodeSignature(authorizationCode3) require.NotEmpty(t, signature) require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is @@ -871,9 +936,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "candies", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -898,9 +961,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "candies", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -911,7 +972,7 @@ func TestStorage(t *testing.T) { }) require.NoError(t, err) }, - run: func(t *testing.T, storage Storage) error { + run: func(t *testing.T, storage Storage, fakeClock *clock.FakeClock) error { signature := hmac.AuthorizeCodeSignature(authorizationCode3) require.NotEmpty(t, signature) require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is @@ -937,9 +998,7 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "candies", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -962,9 +1021,10 @@ func TestStorage(t *testing.T) { tt.mocks(t, client) } secrets := client.CoreV1().Secrets(namespace) - storage := New(tt.resource, secrets, func() time.Time { return fakeNow }, fakeDuration) + fakeClock := clock.NewFakeClock(fakeNow) + storage := New(tt.resource, secrets, fakeClock.Now, lifetime) - err := tt.run(t, storage) + err := tt.run(t, storage, fakeClock) require.Equal(t, tt.wantErr, errString(err)) require.Equal(t, tt.wantActions, client.Actions()) diff --git a/internal/fositestorage/accesstoken/accesstoken.go b/internal/fositestorage/accesstoken/accesstoken.go index 39e63f2b..0acde890 100644 --- a/internal/fositestorage/accesstoken/accesstoken.go +++ b/internal/fositestorage/accesstoken/accesstoken.go @@ -6,6 +6,7 @@ package accesstoken import ( "context" "fmt" + "time" "github.com/ory/fosite" "github.com/ory/fosite/handler/oauth2" @@ -43,8 +44,8 @@ type session struct { Version string `json:"version"` } -func New(secrets corev1client.SecretInterface) RevocationStorage { - return &accessTokenStorage{storage: crud.New(TypeLabelValue, secrets)} +func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionStorageLifetime time.Duration) RevocationStorage { + return &accessTokenStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)} } func (a *accessTokenStorage) RevokeAccessToken(ctx context.Context, requestID string) error { diff --git a/internal/fositestorage/accesstoken/accesstoken_test.go b/internal/fositestorage/accesstoken/accesstoken_test.go index 9ac39995..614b1e0f 100644 --- a/internal/fositestorage/accesstoken/accesstoken_test.go +++ b/internal/fositestorage/accesstoken/accesstoken_test.go @@ -16,12 +16,18 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/clock" "k8s.io/client-go/kubernetes/fake" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" coretesting "k8s.io/client-go/testing" ) const namespace = "test-ns" +var fakeNow = time.Date(2030, time.January, 1, 0, 0, 0, 0, time.UTC) +var lifetime = time.Minute * 10 +var fakeNowPlusLifetimeAsString = metav1.Time{Time: fakeNow.Add(lifetime)}.Format(time.RFC3339) + var secretsGVR = schema.GroupVersionResource{ Group: "", Version: "v1", @@ -29,8 +35,6 @@ var secretsGVR = schema.GroupVersionResource{ } func TestAccessTokenStorage(t *testing.T) { - ctx := context.Background() - wantActions := []coretesting.Action{ coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -40,6 +44,9 @@ func TestAccessTokenStorage(t *testing.T) { "storage.pinniped.dev/type": "access-token", "storage.pinniped.dev/request-id": "abcd-1", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"1"}`), @@ -51,9 +58,7 @@ func TestAccessTokenStorage(t *testing.T) { coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-access-token-pwu5zs7lekbhnln2w4"), } - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, client, _, storage := makeTestSubject() request := &fosite.Request{ ID: "abcd-1", @@ -103,8 +108,6 @@ func TestAccessTokenStorage(t *testing.T) { } func TestAccessTokenStorageRevocation(t *testing.T) { - ctx := context.Background() - wantActions := []coretesting.Action{ coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -114,6 +117,9 @@ func TestAccessTokenStorageRevocation(t *testing.T) { "storage.pinniped.dev/type": "access-token", "storage.pinniped.dev/request-id": "abcd-1", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"1"}`), @@ -127,9 +133,7 @@ func TestAccessTokenStorageRevocation(t *testing.T) { coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-access-token-pwu5zs7lekbhnln2w4"), } - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, client, _, storage := makeTestSubject() request := &fosite.Request{ ID: "abcd-1", @@ -159,10 +163,7 @@ func TestAccessTokenStorageRevocation(t *testing.T) { } func TestGetNotFound(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() _, notFoundErr := storage.GetAccessTokenSession(ctx, "non-existent-signature", nil) require.EqualError(t, notFoundErr, "not_found") @@ -170,10 +171,7 @@ func TestGetNotFound(t *testing.T) { } func TestWrongVersion(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, secrets, storage := makeTestSubject() secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -182,6 +180,9 @@ func TestWrongVersion(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "access-token", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"not-the-right-version"}`), @@ -198,10 +199,7 @@ func TestWrongVersion(t *testing.T) { } func TestNilSessionRequest(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, secrets, storage := makeTestSubject() secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -210,6 +208,9 @@ func TestNilSessionRequest(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "access-token", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"nonsense-key": "nonsense-value","version":"1"}`), @@ -226,20 +227,14 @@ func TestNilSessionRequest(t *testing.T) { } func TestCreateWithNilRequester(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() err := storage.CreateAccessTokenSession(ctx, "signature-doesnt-matter", nil) require.EqualError(t, err, "requester must be of type fosite.Request") } func TestCreateWithWrongRequesterDataTypes(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() request := &fosite.Request{ Session: nil, @@ -257,10 +252,7 @@ func TestCreateWithWrongRequesterDataTypes(t *testing.T) { } func TestCreateWithoutRequesterID(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, client, _, storage := makeTestSubject() request := &fosite.Request{ ID: "", // empty ID @@ -280,3 +272,9 @@ func TestCreateWithoutRequesterID(t *testing.T) { // The generated secret was labeled with that auto-generated request ID require.Equal(t, request.ID, actualSecret.Labels["storage.pinniped.dev/request-id"]) } + +func makeTestSubject() (context.Context, *fake.Clientset, corev1client.SecretInterface, RevocationStorage) { + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + return context.Background(), client, secrets, New(secrets, clock.NewFakeClock(fakeNow).Now, lifetime) +} diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index 4139feca..9f6956a0 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode_test.go +++ b/internal/fositestorage/authorizationcode/authorizationcode_test.go @@ -15,19 +15,21 @@ import ( "testing" "time" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime" - fuzz "github.com/google/gofuzz" "github.com/ory/fosite" + "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" "github.com/pkg/errors" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" 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/util/clock" "k8s.io/client-go/kubernetes/fake" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" kubetesting "k8s.io/client-go/testing" "go.pinniped.dev/internal/fositestorage" @@ -36,10 +38,10 @@ import ( const namespace = "test-ns" var fakeNow = time.Date(2030, time.January, 1, 0, 0, 0, 0, time.UTC) -var fakeDuration = time.Minute * 10 +var lifetime = time.Minute * 10 +var fakeNowPlusLifetimeAsString = metav1.Time{Time: fakeNow.Add(lifetime)}.Format(time.RFC3339) func TestAuthorizationCodeStorage(t *testing.T) { - ctx := context.Background() secretsGVR := schema.GroupVersionResource{ Group: "", Version: "v1", @@ -55,9 +57,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { "storage.pinniped.dev/type": "authcode", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -76,9 +76,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { "storage.pinniped.dev/type": "authcode", }, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": metav1.Time{ - Time: fakeNow.Add(fakeDuration), - }.String(), + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, }, Data: map[string][]byte{ @@ -89,9 +87,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { }), } - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) + ctx, client, _, storage := makeTestSubject() request := &fosite.Request{ ID: "abcd-1", @@ -146,10 +142,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { } func TestGetNotFound(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) + ctx, _, _, storage := makeTestSubject() _, notFoundErr := storage.GetAuthorizeCodeSession(ctx, "non-existent-signature", nil) require.EqualError(t, notFoundErr, "not_found") @@ -157,10 +150,7 @@ func TestGetNotFound(t *testing.T) { } func TestInvalidateWhenNotFound(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) + ctx, _, _, storage := makeTestSubject() notFoundErr := storage.InvalidateAuthorizeCodeSession(ctx, "non-existent-signature") require.EqualError(t, notFoundErr, "not_found") @@ -168,10 +158,7 @@ func TestInvalidateWhenNotFound(t *testing.T) { } func TestInvalidateWhenConflictOnUpdateHappens(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) + ctx, client, _, storage := makeTestSubject() client.PrependReactor("update", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { return true, nil, apierrors.NewConflict(schema.GroupResource{ @@ -192,10 +179,7 @@ func TestInvalidateWhenConflictOnUpdateHappens(t *testing.T) { } func TestWrongVersion(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) + ctx, _, secrets, storage := makeTestSubject() secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -220,10 +204,7 @@ func TestWrongVersion(t *testing.T) { } func TestNilSessionRequest(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) + ctx, _, secrets, storage := makeTestSubject() secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -248,20 +229,14 @@ func TestNilSessionRequest(t *testing.T) { } func TestCreateWithNilRequester(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) + ctx, _, _, storage := makeTestSubject() err := storage.CreateAuthorizeCodeSession(ctx, "signature-doesnt-matter", nil) require.EqualError(t, err, "requester must be of type fosite.Request") } func TestCreateWithWrongRequesterDataTypes(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) + ctx, _, _, storage := makeTestSubject() request := &fosite.Request{ Session: nil, @@ -278,6 +253,12 @@ func TestCreateWithWrongRequesterDataTypes(t *testing.T) { require.EqualError(t, err, "requester's client must be of type fosite.DefaultOpenIDConnectClient") } +func makeTestSubject() (context.Context, *fake.Clientset, corev1client.SecretInterface, oauth2.AuthorizeCodeStorage) { + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + return context.Background(), client, secrets, New(secrets, clock.NewFakeClock(fakeNow).Now, lifetime) +} + // TestFuzzAndJSONNewValidEmptyAuthorizeCodeSession asserts that we can correctly round trip our authorize code session. // It will detect any changes to fosite.AuthorizeRequest and guarantees that all interface types have concrete implementations. func TestFuzzAndJSONNewValidEmptyAuthorizeCodeSession(t *testing.T) { @@ -378,7 +359,7 @@ func TestFuzzAndJSONNewValidEmptyAuthorizeCodeSession(t *testing.T) { const name = "fuzz" // value is irrelevant ctx := context.Background() secrets := fake.NewSimpleClientset().CoreV1().Secrets(name) - storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) + storage := New(secrets, func() time.Time { return fakeNow }, lifetime) // issue a create using the fuzzed request to confirm that marshalling works err = storage.CreateAuthorizeCodeSession(ctx, name, validSession.Request) diff --git a/internal/fositestorage/openidconnect/openidconnect.go b/internal/fositestorage/openidconnect/openidconnect.go index 932e7d35..6a9292d4 100644 --- a/internal/fositestorage/openidconnect/openidconnect.go +++ b/internal/fositestorage/openidconnect/openidconnect.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "strings" + "time" "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" @@ -39,8 +40,8 @@ type session struct { Version string `json:"version"` } -func New(secrets corev1client.SecretInterface) openid.OpenIDConnectRequestStorage { - return &openIDConnectRequestStorage{storage: crud.New(TypeLabelValue, secrets)} +func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionStorageLifetime time.Duration) openid.OpenIDConnectRequestStorage { + return &openIDConnectRequestStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)} } func (a *openIDConnectRequestStorage) CreateOpenIDConnectSession(ctx context.Context, authcode string, requester fosite.Requester) error { diff --git a/internal/fositestorage/openidconnect/openidconnect_test.go b/internal/fositestorage/openidconnect/openidconnect_test.go index 83e86d4b..e727e36b 100644 --- a/internal/fositestorage/openidconnect/openidconnect_test.go +++ b/internal/fositestorage/openidconnect/openidconnect_test.go @@ -16,14 +16,19 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/clock" "k8s.io/client-go/kubernetes/fake" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" coretesting "k8s.io/client-go/testing" ) const namespace = "test-ns" +var fakeNow = time.Date(2030, time.January, 1, 0, 0, 0, 0, time.UTC) +var lifetime = time.Minute * 10 +var fakeNowPlusLifetimeAsString = metav1.Time{Time: fakeNow.Add(lifetime)}.Format(time.RFC3339) + func TestOpenIdConnectStorage(t *testing.T) { - ctx := context.Background() secretsGVR := schema.GroupVersionResource{ Group: "", Version: "v1", @@ -38,6 +43,9 @@ func TestOpenIdConnectStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "oidc", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"1"}`), @@ -49,9 +57,7 @@ func TestOpenIdConnectStorage(t *testing.T) { coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-oidc-pwu5zs7lekbhnln2w4"), } - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, client, _, storage := makeTestSubject() request := &fosite.Request{ ID: "abcd-1", @@ -101,10 +107,7 @@ func TestOpenIdConnectStorage(t *testing.T) { } func TestGetNotFound(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() _, notFoundErr := storage.GetOpenIDConnectSession(ctx, "authcode.non-existent-signature", nil) require.EqualError(t, notFoundErr, "not_found") @@ -112,10 +115,7 @@ func TestGetNotFound(t *testing.T) { } func TestWrongVersion(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, secrets, storage := makeTestSubject() secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -140,10 +140,7 @@ func TestWrongVersion(t *testing.T) { } func TestNilSessionRequest(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, secrets, storage := makeTestSubject() secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -168,20 +165,14 @@ func TestNilSessionRequest(t *testing.T) { } func TestCreateWithNilRequester(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() err := storage.CreateOpenIDConnectSession(ctx, "authcode.signature-doesnt-matter", nil) require.EqualError(t, err, "requester must be of type fosite.Request") } func TestCreateWithWrongRequesterDataTypes(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() request := &fosite.Request{ Session: nil, @@ -199,11 +190,14 @@ func TestCreateWithWrongRequesterDataTypes(t *testing.T) { } func TestAuthcodeHasNoDot(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() err := storage.CreateOpenIDConnectSession(ctx, "all-one-part", nil) require.EqualError(t, err, "malformed authorization code") } + +func makeTestSubject() (context.Context, *fake.Clientset, corev1client.SecretInterface, openid.OpenIDConnectRequestStorage) { + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + return context.Background(), client, secrets, New(secrets, clock.NewFakeClock(fakeNow).Now, lifetime) +} diff --git a/internal/fositestorage/pkce/pkce.go b/internal/fositestorage/pkce/pkce.go index 35aec20c..6903eb90 100644 --- a/internal/fositestorage/pkce/pkce.go +++ b/internal/fositestorage/pkce/pkce.go @@ -6,6 +6,7 @@ package pkce import ( "context" "fmt" + "time" "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" @@ -38,8 +39,8 @@ type session struct { Version string `json:"version"` } -func New(secrets corev1client.SecretInterface) pkce.PKCERequestStorage { - return &pkceStorage{storage: crud.New(TypeLabelValue, secrets, nil, 0)} +func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionStorageLifetime time.Duration) pkce.PKCERequestStorage { + return &pkceStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)} } func (a *pkceStorage) CreatePKCERequestSession(ctx context.Context, signature string, requester fosite.Requester) error { diff --git a/internal/fositestorage/pkce/pkce_test.go b/internal/fositestorage/pkce/pkce_test.go index be1b9bf4..aa9c3faa 100644 --- a/internal/fositestorage/pkce/pkce_test.go +++ b/internal/fositestorage/pkce/pkce_test.go @@ -11,19 +11,25 @@ import ( "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" + "github.com/ory/fosite/handler/pkce" "github.com/pkg/errors" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/clock" "k8s.io/client-go/kubernetes/fake" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" coretesting "k8s.io/client-go/testing" ) const namespace = "test-ns" +var fakeNow = time.Date(2030, time.January, 1, 0, 0, 0, 0, time.UTC) +var lifetime = time.Minute * 10 +var fakeNowPlusLifetimeAsString = metav1.Time{Time: fakeNow.Add(lifetime)}.Format(time.RFC3339) + func TestPKCEStorage(t *testing.T) { - ctx := context.Background() secretsGVR := schema.GroupVersionResource{ Group: "", Version: "v1", @@ -38,6 +44,9 @@ func TestPKCEStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "pkce", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"1"}`), @@ -49,9 +58,7 @@ func TestPKCEStorage(t *testing.T) { coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-pkce-pwu5zs7lekbhnln2w4"), } - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, client, _, storage := makeTestSubject() request := &fosite.Request{ ID: "abcd-1", @@ -101,10 +108,7 @@ func TestPKCEStorage(t *testing.T) { } func TestGetNotFound(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() _, notFoundErr := storage.GetPKCERequestSession(ctx, "non-existent-signature", nil) require.EqualError(t, notFoundErr, "not_found") @@ -112,10 +116,7 @@ func TestGetNotFound(t *testing.T) { } func TestWrongVersion(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, secrets, storage := makeTestSubject() secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -124,6 +125,9 @@ func TestWrongVersion(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "pkce", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"not-the-right-version"}`), @@ -140,10 +144,7 @@ func TestWrongVersion(t *testing.T) { } func TestNilSessionRequest(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, secrets, storage := makeTestSubject() secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -152,6 +153,9 @@ func TestNilSessionRequest(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "pkce", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"nonsense-key": "nonsense-value","version":"1"}`), @@ -168,20 +172,14 @@ func TestNilSessionRequest(t *testing.T) { } func TestCreateWithNilRequester(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() err := storage.CreatePKCERequestSession(ctx, "signature-doesnt-matter", nil) require.EqualError(t, err, "requester must be of type fosite.Request") } func TestCreateWithWrongRequesterDataTypes(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() request := &fosite.Request{ Session: nil, @@ -197,3 +195,9 @@ func TestCreateWithWrongRequesterDataTypes(t *testing.T) { err = storage.CreatePKCERequestSession(ctx, "signature-doesnt-matter", request) require.EqualError(t, err, "requester's client must be of type fosite.DefaultOpenIDConnectClient") } + +func makeTestSubject() (context.Context, *fake.Clientset, corev1client.SecretInterface, pkce.PKCERequestStorage) { + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + return context.Background(), client, secrets, New(secrets, clock.NewFakeClock(fakeNow).Now, lifetime) +} diff --git a/internal/fositestorage/refreshtoken/refreshtoken.go b/internal/fositestorage/refreshtoken/refreshtoken.go index 5cccbf74..dcd25a99 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken.go +++ b/internal/fositestorage/refreshtoken/refreshtoken.go @@ -6,6 +6,7 @@ package refreshtoken import ( "context" "fmt" + "time" "github.com/ory/fosite" "github.com/ory/fosite/handler/oauth2" @@ -43,8 +44,8 @@ type session struct { Version string `json:"version"` } -func New(secrets corev1client.SecretInterface) RevocationStorage { - return &refreshTokenStorage{storage: crud.New(TypeLabelValue, secrets)} +func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionStorageLifetime time.Duration) RevocationStorage { + return &refreshTokenStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)} } func (a *refreshTokenStorage) RevokeRefreshToken(ctx context.Context, requestID string) error { diff --git a/internal/fositestorage/refreshtoken/refreshtoken_test.go b/internal/fositestorage/refreshtoken/refreshtoken_test.go index bb1e664f..a776b408 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken_test.go +++ b/internal/fositestorage/refreshtoken/refreshtoken_test.go @@ -16,7 +16,9 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/clock" "k8s.io/client-go/kubernetes/fake" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" coretesting "k8s.io/client-go/testing" ) @@ -27,10 +29,11 @@ var secretsGVR = schema.GroupVersionResource{ Version: "v1", Resource: "secrets", } +var fakeNow = time.Date(2030, time.January, 1, 0, 0, 0, 0, time.UTC) +var lifetime = time.Minute * 10 +var fakeNowPlusLifetimeAsString = metav1.Time{Time: fakeNow.Add(lifetime)}.Format(time.RFC3339) func TestRefreshTokenStorage(t *testing.T) { - ctx := context.Background() - wantActions := []coretesting.Action{ coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -40,6 +43,9 @@ func TestRefreshTokenStorage(t *testing.T) { "storage.pinniped.dev/type": "refresh-token", "storage.pinniped.dev/request-id": "abcd-1", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"1"}`), @@ -51,9 +57,7 @@ func TestRefreshTokenStorage(t *testing.T) { coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4"), } - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, client, _, storage := makeTestSubject() request := &fosite.Request{ ID: "abcd-1", @@ -103,8 +107,6 @@ func TestRefreshTokenStorage(t *testing.T) { } func TestRefreshTokenStorageRevocation(t *testing.T) { - ctx := context.Background() - wantActions := []coretesting.Action{ coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -114,6 +116,9 @@ func TestRefreshTokenStorageRevocation(t *testing.T) { "storage.pinniped.dev/type": "refresh-token", "storage.pinniped.dev/request-id": "abcd-1", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"1"}`), @@ -127,9 +132,7 @@ func TestRefreshTokenStorageRevocation(t *testing.T) { coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4"), } - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, client, _, storage := makeTestSubject() request := &fosite.Request{ ID: "abcd-1", @@ -159,10 +162,7 @@ func TestRefreshTokenStorageRevocation(t *testing.T) { } func TestGetNotFound(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() _, notFoundErr := storage.GetRefreshTokenSession(ctx, "non-existent-signature", nil) require.EqualError(t, notFoundErr, "not_found") @@ -170,10 +170,7 @@ func TestGetNotFound(t *testing.T) { } func TestWrongVersion(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, secrets, storage := makeTestSubject() secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -182,6 +179,9 @@ func TestWrongVersion(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "refresh-token", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"not-the-right-version"}`), @@ -198,10 +198,7 @@ func TestWrongVersion(t *testing.T) { } func TestNilSessionRequest(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, secrets, storage := makeTestSubject() secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -210,6 +207,9 @@ func TestNilSessionRequest(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "refresh-token", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"nonsense-key": "nonsense-value","version":"1"}`), @@ -226,20 +226,14 @@ func TestNilSessionRequest(t *testing.T) { } func TestCreateWithNilRequester(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() err := storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", nil) require.EqualError(t, err, "requester must be of type fosite.Request") } func TestCreateWithWrongRequesterDataTypes(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, _, _, storage := makeTestSubject() request := &fosite.Request{ Session: nil, @@ -257,10 +251,7 @@ func TestCreateWithWrongRequesterDataTypes(t *testing.T) { } func TestCreateWithoutRequesterID(t *testing.T) { - ctx := context.Background() - client := fake.NewSimpleClientset() - secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + ctx, client, _, storage := makeTestSubject() request := &fosite.Request{ ID: "", // empty ID @@ -280,3 +271,9 @@ func TestCreateWithoutRequesterID(t *testing.T) { // The generated secret was labeled with that auto-generated request ID require.Equal(t, request.ID, actualSecret.Labels["storage.pinniped.dev/request-id"]) } + +func makeTestSubject() (context.Context, *fake.Clientset, corev1client.SecretInterface, RevocationStorage) { + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + return context.Background(), client, secrets, New(secrets, clock.NewFakeClock(fakeNow).Now, lifetime) +} diff --git a/internal/oidc/kube_storage.go b/internal/oidc/kube_storage.go index bbc00b46..46f9c947 100644 --- a/internal/oidc/kube_storage.go +++ b/internal/oidc/kube_storage.go @@ -32,12 +32,13 @@ type KubeStorage struct { } func NewKubeStorage(secrets corev1client.SecretInterface, timeoutsConfiguration TimeoutsConfiguration) *KubeStorage { + nowFunc := time.Now return &KubeStorage{ - authorizationCodeStorage: authorizationcode.New(secrets, time.Now, timeoutsConfiguration.AuthorizationCodeSessionStorageLifetime), - pkceStorage: pkce.New(secrets), - oidcStorage: openidconnect.New(secrets), - accessTokenStorage: accesstoken.New(secrets), - refreshTokenStorage: refreshtoken.New(secrets), + authorizationCodeStorage: authorizationcode.New(secrets, nowFunc, timeoutsConfiguration.AuthorizationCodeSessionStorageLifetime), + pkceStorage: pkce.New(secrets, nowFunc, timeoutsConfiguration.PKCESessionStorageLifetime), + oidcStorage: openidconnect.New(secrets, nowFunc, timeoutsConfiguration.OIDCSessionStorageLifetime), + accessTokenStorage: accesstoken.New(secrets, nowFunc, timeoutsConfiguration.AccessTokenSessionStorageLifetime), + refreshTokenStorage: refreshtoken.New(secrets, nowFunc, timeoutsConfiguration.RefreshTokenSessionStorageLifetime), } } diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 87d3c637..fa64dc3b 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -495,29 +495,6 @@ func TestTokenEndpoint(t *testing.T) { }, }, }, - { - name: "auth code is invalidated", - authcodeExchange: authcodeExchangeInputs{ - modifyStorage: func( - t *testing.T, - s interface { - oauth2.TokenRevocationStorage - oauth2.CoreStorage - openid.OpenIDConnectRequestStorage - pkce.PKCERequestStorage - fosite.ClientManager - }, - authCode string, - ) { - err := s.InvalidateAuthorizeCodeSession(context.Background(), getFositeDataSignature(t, authCode)) - require.NoError(t, err) - }, - want: tokenEndpointResponseExpectedValues{ - wantStatus: http.StatusBadRequest, - wantErrorResponseBody: fositeReusedAuthCodeErrorBody, - }, - }, - }, { name: "redirect uri is missing in request", authcodeExchange: authcodeExchangeInputs{ diff --git a/test/integration/storage_test.go b/test/integration/storage_test.go index 501099fe..f551d2cd 100644 --- a/test/integration/storage_test.go +++ b/test/integration/storage_test.go @@ -14,10 +14,12 @@ import ( "github.com/ory/fosite" "github.com/ory/fosite/compose" "github.com/stretchr/testify/require" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "go.pinniped.dev/internal/fositestorage/authorizationcode" + "go.pinniped.dev/internal/testutil" "go.pinniped.dev/test/library" ) @@ -54,7 +56,8 @@ func TestAuthorizeCodeStorage(t *testing.T) { err := json.Unmarshal([]byte(authorizationcode.ExpectedAuthorizeCodeSessionJSONFromFuzzing), session) require.NoError(t, err) - storage := authorizationcode.New(secrets) + sessionStorageLifetime := 5 * time.Minute + storage := authorizationcode.New(secrets, time.Now, sessionStorageLifetime) // the session for this signature should not exist yet notFoundRequest, err := storage.GetAuthorizeCodeSession(ctx, signature, nil) @@ -75,6 +78,19 @@ func TestAuthorizeCodeStorage(t *testing.T) { require.NoError(t, err) require.JSONEq(t, authorizationcode.ExpectedAuthorizeCodeSessionJSONFromFuzzing, string(initialSecret.Data["pinniped-storage-data"])) + // check that the Secret got the expected annotations + actualGCAfterValue := initialSecret.Annotations["storage.pinniped.dev/garbage-collect-after"] + require.NotEmpty(t, actualGCAfterValue) + parsedActualGCAfterValue, err := time.Parse(time.RFC3339, actualGCAfterValue) + require.NoError(t, err) + testutil.RequireTimeInDelta(t, time.Now().Add(sessionStorageLifetime), parsedActualGCAfterValue, 30*time.Second) + + // check that the Secret got the right labels + require.Equal(t, map[string]string{"storage.pinniped.dev/type": "authcode"}, initialSecret.Labels) + + // check that the Secret got the right type + require.Equal(t, v1.SecretType("storage.pinniped.dev/authcode"), initialSecret.Type) + // we should be able to get the session now and the request should be the same as what we put in request, err := storage.GetAuthorizeCodeSession(ctx, signature, nil) require.NoError(t, err)