From b0c354637d091c5154c7041cb044422cb79bd556 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Thu, 10 Dec 2020 12:15:40 -0800 Subject: [PATCH 1/4] WIP passing lifetime through to storage, unit tests are failing Signed-off-by: Ryan Richard --- internal/crud/crud.go | 14 +- internal/crud/crud_test.go | 136 +++++++++++++++++- .../authorizationcode/authorizationcode.go | 5 +- .../authorizationcode_test.go | 31 ++-- internal/fositestorage/pkce/pkce.go | 2 +- .../oidc/callback/callback_handler_test.go | 5 +- internal/oidc/kube_storage.go | 4 +- internal/oidc/provider/manager/manager.go | 6 +- internal/oidc/token/token_handler_test.go | 2 +- 9 files changed, 183 insertions(+), 22 deletions(-) diff --git a/internal/crud/crud.go b/internal/crud/crud.go index dee3f49b..e9982734 100644 --- a/internal/crud/crud.go +++ b/internal/crud/crud.go @@ -11,6 +11,7 @@ import ( "encoding/json" "fmt" "strings" + "time" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -22,7 +23,8 @@ import ( //nolint:gosec // ignore lint warnings that these are credentials const ( - SecretLabelKey = "storage.pinniped.dev/type" + SecretLabelKey = "storage.pinniped.dev/type" + SecretLifetimeAnnotationKey = "storage.pinniped.dev/garbage-collect-after" secretNameFormat = "pinniped-storage-%s-%s" secretTypeFormat = "storage.pinniped.dev/%s" @@ -45,12 +47,14 @@ type Storage interface { type JSON interface{} // document that we need valid JSON types -func New(resource string, secrets corev1client.SecretInterface) Storage { +func New(resource string, secrets corev1client.SecretInterface, clock func() time.Time, lifetime time.Duration) Storage { return &secretsStorage{ resource: resource, secretType: corev1.SecretType(fmt.Sprintf(secretTypeFormat, resource)), secretVersion: []byte(secretVersion), secrets: secrets, + clock: clock, + lifetime: lifetime, } } @@ -59,6 +63,8 @@ type secretsStorage struct { secretType corev1.SecretType secretVersion []byte secrets corev1client.SecretInterface + clock func() time.Time + lifetime time.Duration } func (s *secretsStorage) Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string) (string, error) { @@ -162,12 +168,16 @@ func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, for labelName, labelValue := range additionalLabels { labels[labelName] = labelValue } + annotations := map[string]string{ + SecretLifetimeAnnotationKey: s.clock().Add(s.lifetime).UTC().String(), + } return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: s.getName(signature), ResourceVersion: resourceVersion, Labels: labels, + Annotations: annotations, OwnerReferences: nil, }, Data: map[string][]byte{ diff --git a/internal/crud/crud_test.go b/internal/crud/crud_test.go index 58c5f6ed..9e4ed216 100644 --- a/internal/crud/crud_test.go +++ b/internal/crud/crud_test.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "testing" + "time" "github.com/ory/fosite/compose" "github.com/stretchr/testify/require" @@ -45,6 +46,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 + const ( namespace = "test-ns" authorizationCode1 = "81qE408EKL-e99gcXo3UnXBz9W05yGm92_hBmvXeadM.R5h38Bmw7yOaWNy0ypB3feh9toM-3T2zlwMXQyeE9B0" @@ -119,6 +123,11 @@ func TestStorage(t *testing.T) { 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(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), @@ -137,6 +146,11 @@ func TestStorage(t *testing.T) { 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(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), @@ -179,6 +193,11 @@ func TestStorage(t *testing.T) { "label1": "value1", "label2": "value2", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), @@ -199,6 +218,11 @@ func TestStorage(t *testing.T) { "label1": "value1", "label2": "value2", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), @@ -221,6 +245,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "pandas-are-best", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"snorlax"}`), @@ -256,6 +285,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "pandas-are-best", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"snorlax"}`), @@ -278,6 +312,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "stores", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"pants"}`), @@ -327,6 +366,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "stores", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"shirts"}`), @@ -345,6 +389,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "stores", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"shirts"}`), @@ -367,6 +416,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "seals", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"sad-seal"}`), @@ -402,6 +456,11 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "seals", "additionalLabel": "matching-value", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"sad-seal"}`), @@ -418,6 +477,11 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "seals", "additionalLabel": "matching-value", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"happy-seal"}`), @@ -434,6 +498,11 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "seals", // same type as above "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(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"sad-seal2"}`), @@ -450,6 +519,11 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "walruses", // different type from above "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(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"sad-seal3"}`), @@ -479,6 +553,11 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "seals", // same type as above "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(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"sad-seal2"}`), @@ -496,6 +575,11 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "walruses", // different type from above "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(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"sad-seal3"}`), @@ -519,6 +603,11 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "seals", "additionalLabel": "matching-value", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"sad-seal"}`), @@ -549,6 +638,11 @@ func TestStorage(t *testing.T) { "storage.pinniped.dev/type": "seals", "additionalLabel": "matching-value", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"sad-seal"}`), @@ -602,6 +696,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "candies", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"twizzlers"}`), @@ -637,6 +736,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "candies", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"twizzlers"}`), @@ -659,6 +763,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "candies-are-bad", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"twizzlers"}`), @@ -694,6 +803,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "candies-are-bad", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"twizzlers"}`), @@ -716,6 +830,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "candies", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"twizzlers"}`), @@ -751,6 +870,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "candies", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"twizzlers"}`), @@ -773,6 +897,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "candies", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`}}bad data{{`), @@ -807,6 +936,11 @@ func TestStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "candies", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`}}bad data{{`), @@ -828,7 +962,7 @@ func TestStorage(t *testing.T) { tt.mocks(t, client) } secrets := client.CoreV1().Secrets(namespace) - storage := New(tt.resource, secrets) + storage := New(tt.resource, secrets, func() time.Time { return fakeNow }, fakeDuration) err := tt.run(t, storage) diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index fc4cb1e7..99b2c5bd 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -7,6 +7,7 @@ import ( "context" stderrors "errors" "fmt" + "time" "github.com/ory/fosite" "github.com/ory/fosite/handler/oauth2" @@ -40,8 +41,8 @@ type AuthorizeCodeSession struct { Version string `json:"version"` } -func New(secrets corev1client.SecretInterface) oauth2.AuthorizeCodeStorage { - return &authorizeCodeStorage{storage: crud.New(TypeLabelValue, secrets)} +func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionStorageLifetime time.Duration) oauth2.AuthorizeCodeStorage { + return &authorizeCodeStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)} } func (a *authorizeCodeStorage) CreateAuthorizeCodeSession(ctx context.Context, signature string, requester fosite.Requester) error { diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index 904d6074..4139feca 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode_test.go +++ b/internal/fositestorage/authorizationcode/authorizationcode_test.go @@ -35,6 +35,9 @@ import ( const namespace = "test-ns" +var fakeNow = time.Date(2030, time.January, 1, 0, 0, 0, 0, time.UTC) +var fakeDuration = time.Minute * 10 + func TestAuthorizationCodeStorage(t *testing.T) { ctx := context.Background() secretsGVR := schema.GroupVersionResource{ @@ -51,6 +54,11 @@ func TestAuthorizationCodeStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "authcode", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"active":true,"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"}`), @@ -67,6 +75,11 @@ func TestAuthorizationCodeStorage(t *testing.T) { Labels: map[string]string{ "storage.pinniped.dev/type": "authcode", }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": metav1.Time{ + Time: fakeNow.Add(fakeDuration), + }.String(), + }, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"active":false,"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"}`), @@ -78,7 +91,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { client := fake.NewSimpleClientset() secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) request := &fosite.Request{ ID: "abcd-1", @@ -136,7 +149,7 @@ func TestGetNotFound(t *testing.T) { ctx := context.Background() client := fake.NewSimpleClientset() secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) _, notFoundErr := storage.GetAuthorizeCodeSession(ctx, "non-existent-signature", nil) require.EqualError(t, notFoundErr, "not_found") @@ -147,7 +160,7 @@ func TestInvalidateWhenNotFound(t *testing.T) { ctx := context.Background() client := fake.NewSimpleClientset() secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) notFoundErr := storage.InvalidateAuthorizeCodeSession(ctx, "non-existent-signature") require.EqualError(t, notFoundErr, "not_found") @@ -158,7 +171,7 @@ func TestInvalidateWhenConflictOnUpdateHappens(t *testing.T) { ctx := context.Background() client := fake.NewSimpleClientset() secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) client.PrependReactor("update", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { return true, nil, apierrors.NewConflict(schema.GroupResource{ @@ -182,7 +195,7 @@ func TestWrongVersion(t *testing.T) { ctx := context.Background() client := fake.NewSimpleClientset() secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -210,7 +223,7 @@ func TestNilSessionRequest(t *testing.T) { ctx := context.Background() client := fake.NewSimpleClientset() secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -238,7 +251,7 @@ func TestCreateWithNilRequester(t *testing.T) { ctx := context.Background() client := fake.NewSimpleClientset() secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) err := storage.CreateAuthorizeCodeSession(ctx, "signature-doesnt-matter", nil) require.EqualError(t, err, "requester must be of type fosite.Request") @@ -248,7 +261,7 @@ func TestCreateWithWrongRequesterDataTypes(t *testing.T) { ctx := context.Background() client := fake.NewSimpleClientset() secrets := client.CoreV1().Secrets(namespace) - storage := New(secrets) + storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) request := &fosite.Request{ Session: nil, @@ -365,7 +378,7 @@ func TestFuzzAndJSONNewValidEmptyAuthorizeCodeSession(t *testing.T) { const name = "fuzz" // value is irrelevant ctx := context.Background() secrets := fake.NewSimpleClientset().CoreV1().Secrets(name) - storage := New(secrets) + storage := New(secrets, func() time.Time { return fakeNow }, fakeDuration) // issue a create using the fuzzed request to confirm that marshalling works err = storage.CreateAuthorizeCodeSession(ctx, name, validSession.Request) diff --git a/internal/fositestorage/pkce/pkce.go b/internal/fositestorage/pkce/pkce.go index 24d554d2..35aec20c 100644 --- a/internal/fositestorage/pkce/pkce.go +++ b/internal/fositestorage/pkce/pkce.go @@ -39,7 +39,7 @@ type session struct { } func New(secrets corev1client.SecretInterface) pkce.PKCERequestStorage { - return &pkceStorage{storage: crud.New(TypeLabelValue, secrets)} + return &pkceStorage{storage: crud.New(TypeLabelValue, secrets, nil, 0)} } func (a *pkceStorage) CreatePKCERequestSession(ctx context.Context, signature string, requester fosite.Requester) error { diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index df0c2779..dfc153f1 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -457,11 +457,12 @@ func TestCallbackEndpoint(t *testing.T) { // Configure fosite the same way that the production code would. // Inject this into our test subject at the last second so we get a fresh storage for every test. - oauthStore := oidc.NewKubeStorage(secrets) + timeoutsConfiguration := oidc.DefaultOIDCTimeoutsConfiguration() + oauthStore := oidc.NewKubeStorage(secrets, timeoutsConfiguration) hmacSecret := []byte("some secret - must have at least 32 bytes") require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() - oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused, timeoutsConfiguration) idpListGetter := oidctestutil.NewIDPListGetter(&test.idp) subject := NewHandler(idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec, happyUpstreamRedirectURI) diff --git a/internal/oidc/kube_storage.go b/internal/oidc/kube_storage.go index b4329722..bbc00b46 100644 --- a/internal/oidc/kube_storage.go +++ b/internal/oidc/kube_storage.go @@ -31,9 +31,9 @@ type KubeStorage struct { refreshTokenStorage refreshtoken.RevocationStorage } -func NewKubeStorage(secrets corev1client.SecretInterface) *KubeStorage { +func NewKubeStorage(secrets corev1client.SecretInterface, timeoutsConfiguration TimeoutsConfiguration) *KubeStorage { return &KubeStorage{ - authorizationCodeStorage: authorizationcode.New(secrets), + authorizationCodeStorage: authorizationcode.New(secrets, time.Now, timeoutsConfiguration.AuthorizationCodeSessionStorageLifetime), pkceStorage: pkce.New(secrets), oidcStorage: openidconnect.New(secrets), accessTokenStorage: accesstoken.New(secrets), diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index fc4ff1eb..907bbd73 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -77,12 +77,14 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { fositeHMACSecretForThisProvider := []byte("some secret - must have at least 32 bytes") // TODO replace this secret + timeoutsConfiguration := oidc.DefaultOIDCTimeoutsConfiguration() + // Use NullStorage for the authorize endpoint because we do not actually want to store anything until // the upstream callback endpoint is called later. - oauthHelperWithNullStorage := oidc.FositeOauth2Helper(oidc.NullStorage{}, issuer, fositeHMACSecretForThisProvider, nil, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelperWithNullStorage := oidc.FositeOauth2Helper(oidc.NullStorage{}, issuer, fositeHMACSecretForThisProvider, nil, timeoutsConfiguration) // For all the other endpoints, make another oauth helper with exactly the same settings except use real storage. - oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient), issuer, fositeHMACSecretForThisProvider, m.dynamicJWKSProvider, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient, timeoutsConfiguration), issuer, fositeHMACSecretForThisProvider, m.dynamicJWKSProvider, timeoutsConfiguration) // TODO use different codecs for the state and the cookie, because: // 1. we would like to state to have an embedded expiration date while the cookie does not need that diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index d0ae5f3a..87d3c637 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1155,7 +1155,7 @@ func exchangeAuthcodeForTokens(t *testing.T, test authcodeExchangeInputs) ( var oauthHelper fosite.OAuth2Provider - oauthStore = oidc.NewKubeStorage(secrets) + oauthStore = oidc.NewKubeStorage(secrets, oidc.DefaultOIDCTimeoutsConfiguration()) if test.makeOathHelper != nil { oauthHelper, authCode, jwtSigningKey = test.makeOathHelper(t, authRequest, oauthStore) } else { From afd216308b169dfeb4dce86f0c6bd99bceeccbec Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 10 Dec 2020 14:47:58 -0800 Subject: [PATCH 2/4] KubeStorage annotates every Secret with garbage-collect-after timestamp Signed-off-by: Margo Crawford --- internal/crud/crud.go | 16 +- internal/crud/crud_test.go | 256 +++++++++++------- .../fositestorage/accesstoken/accesstoken.go | 5 +- .../accesstoken/accesstoken_test.go | 66 +++-- .../authorizationcode_test.go | 67 ++--- .../openidconnect/openidconnect.go | 5 +- .../openidconnect/openidconnect_test.go | 50 ++-- internal/fositestorage/pkce/pkce.go | 5 +- internal/fositestorage/pkce/pkce_test.go | 52 ++-- .../refreshtoken/refreshtoken.go | 5 +- .../refreshtoken/refreshtoken_test.go | 65 +++-- internal/oidc/kube_storage.go | 11 +- internal/oidc/token/token_handler_test.go | 23 -- test/integration/storage_test.go | 18 +- 14 files changed, 339 insertions(+), 305 deletions(-) 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) From ed9b3ffce503be2125f43ad768d946e5baccbcad Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Thu, 10 Dec 2020 17:34:05 -0800 Subject: [PATCH 3/4] Add controller for garbage collecting secrets Signed-off-by: Ryan Richard --- .../supervisorstorage/garbage_collector.go | 71 ++++ .../garbage_collector_test.go | 307 ++++++++++++++++++ internal/controller/utils.go | 5 + 3 files changed, 383 insertions(+) create mode 100644 internal/controller/supervisorstorage/garbage_collector.go create mode 100644 internal/controller/supervisorstorage/garbage_collector_test.go diff --git a/internal/controller/supervisorstorage/garbage_collector.go b/internal/controller/supervisorstorage/garbage_collector.go new file mode 100644 index 00000000..95b87639 --- /dev/null +++ b/internal/controller/supervisorstorage/garbage_collector.go @@ -0,0 +1,71 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package supervisorstorage + +import ( + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + corev1informers "k8s.io/client-go/informers/core/v1" + "k8s.io/client-go/kubernetes" + + pinnipedcontroller "go.pinniped.dev/internal/controller" + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/crud" + "go.pinniped.dev/internal/plog" +) + +type garbageCollectorController struct { + secretInformer corev1informers.SecretInformer + kubeClient kubernetes.Interface +} + +func GarbageCollectorController( + kubeClient kubernetes.Interface, + secretInformer corev1informers.SecretInformer, + withInformer pinnipedcontroller.WithInformerOptionFunc, +) controllerlib.Controller { + return controllerlib.New( + controllerlib.Config{ + Name: "garbage-collector-controller", + Syncer: &garbageCollectorController{ + secretInformer: secretInformer, + kubeClient: kubeClient, + }, + }, + withInformer( + secretInformer, + pinnipedcontroller.MatchNothingFilter(nil), + controllerlib.InformerOption{}, + ), + ) +} + +func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error { + listOfSecrets, err := c.secretInformer.Lister().List(labels.Everything()) + if err != nil { + return err + } + for i := range listOfSecrets { + secret := listOfSecrets[i] + s, ok := secret.Annotations[crud.SecretLifetimeAnnotationKey] + if !ok { + continue + } + currentTime := time.Now() + garbageCollectAfterTime, err := time.Parse(time.RFC3339, s) + if err != nil { + plog.WarningErr("could not parse for garbage collection", err, "secretName", secret.Name, "garbageCollectAfter", s) + continue + } + if garbageCollectAfterTime.Before(currentTime) { + err = c.kubeClient.CoreV1().Secrets(secret.Namespace).Delete(ctx.Context, secret.Name, metav1.DeleteOptions{}) + if err != nil { + plog.WarningErr("failed to garbage collect value", err, "secretName", secret.Name, "garbageCollectAfter", s) + } + } + } + return nil +} diff --git a/internal/controller/supervisorstorage/garbage_collector_test.go b/internal/controller/supervisorstorage/garbage_collector_test.go new file mode 100644 index 00000000..e2167b88 --- /dev/null +++ b/internal/controller/supervisorstorage/garbage_collector_test.go @@ -0,0 +1,307 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package supervisorstorage + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/sclevine/spec" + "github.com/sclevine/spec/report" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + kubeinformers "k8s.io/client-go/informers" + kubernetesfake "k8s.io/client-go/kubernetes/fake" + kubetesting "k8s.io/client-go/testing" + + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/testutil" +) + +func TestGarbageCollectorControllerInformerFilters(t *testing.T) { + spec.Run(t, "informer filters", func(t *testing.T, when spec.G, it spec.S) { + var ( + r *require.Assertions + observableWithInformerOption *testutil.ObservableWithInformerOption + secretsInformerFilter controllerlib.Filter + ) + + it.Before(func() { + r = require.New(t) + observableWithInformerOption = testutil.NewObservableWithInformerOption() + secretsInformer := kubeinformers.NewSharedInformerFactory(nil, 0).Core().V1().Secrets() + _ = GarbageCollectorController( + nil, + secretsInformer, + observableWithInformerOption.WithInformer, // make it possible to observe the behavior of the Filters + ) + secretsInformerFilter = observableWithInformerOption.GetFilterForInformer(secretsInformer) + }) + + when("watching Secret objects", func() { + var ( + subject controllerlib.Filter + secret, otherSecret *corev1.Secret + ) + + it.Before(func() { + subject = secretsInformerFilter + secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}} + otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}} + }) + + when("any Secret changes", func() { + it("returns false to avoid triggering the sync function", func() { + r.False(subject.Add(secret)) + r.False(subject.Update(secret, otherSecret)) + r.False(subject.Update(otherSecret, secret)) + r.False(subject.Delete(secret)) + }) + }) + }) + }, spec.Parallel(), spec.Report(report.Terminal{})) +} + +func TestGarbageCollectorControllerSync(t *testing.T) { + secretsGVR := schema.GroupVersionResource{ + Group: "", + Version: "v1", + Resource: "secrets", + } + + firstExpiredTime := time.Date(1900, time.January, 1, 1, 0, 0, 0, time.UTC).Format(time.RFC3339) + secondExpiredTime := time.Date(1901, time.January, 1, 1, 0, 0, 0, time.UTC).Format(time.RFC3339) + unexpiredTime := time.Now().Add(time.Hour * 24).UTC().Format(time.RFC3339) + + spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) { + const ( + installedInNamespace = "some-namespace" + ) + + var ( + r *require.Assertions + subject controllerlib.Controller + kubeInformerClient *kubernetesfake.Clientset + kubeClient *kubernetesfake.Clientset + kubeInformers kubeinformers.SharedInformerFactory + timeoutContext context.Context + timeoutContextCancel context.CancelFunc + syncContext *controllerlib.Context + ) + + // Defer starting the informers until the last possible moment so that the + // nested Before's can keep adding things to the informer caches. + var startInformersAndController = func() { + // Set this at the last second to allow for injection of server override. + subject = GarbageCollectorController( + kubeClient, + kubeInformers.Core().V1().Secrets(), + controllerlib.WithInformer, + ) + + // Set this at the last second to support calling subject.Name(). + syncContext = &controllerlib.Context{ + Context: timeoutContext, + Name: subject.Name(), + Key: controllerlib.Key{ + Namespace: "", + Name: "", + }, + } + + // Must start informers before calling TestRunSynchronously() + kubeInformers.Start(timeoutContext.Done()) + controllerlib.TestRunSynchronously(t, subject) + } + + it.Before(func() { + r = require.New(t) + + timeoutContext, timeoutContextCancel = context.WithTimeout(context.Background(), time.Second*3) + + kubeInformerClient = kubernetesfake.NewSimpleClientset() + kubeClient = kubernetesfake.NewSimpleClientset() + kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0) + + unrelatedSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some other unrelated secret", + Namespace: installedInNamespace, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(unrelatedSecret)) + r.NoError(kubeClient.Tracker().Add(unrelatedSecret)) + }) + + it.After(func() { + timeoutContextCancel() + }) + + when("there are secrets without the garbage-collect-after annotation", func() { + it("does not delete those secrets", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + require.Empty(t, kubeClient.Actions()) + list, err := kubeClient.CoreV1().Secrets(installedInNamespace).List(context.Background(), metav1.ListOptions{}) + r.NoError(err) + r.Len(list.Items, 1) + r.Equal("some other unrelated secret", list.Items[0].Name) + }) + }) + + when("there are secrets with the garbage-collect-after annotation", func() { + it.Before(func() { + firstExpiredSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "first expired secret", + Namespace: installedInNamespace, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": firstExpiredTime, + }, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(firstExpiredSecret)) + r.NoError(kubeClient.Tracker().Add(firstExpiredSecret)) + secondExpiredSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "second expired secret", + Namespace: installedInNamespace, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": secondExpiredTime, + }, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(secondExpiredSecret)) + r.NoError(kubeClient.Tracker().Add(secondExpiredSecret)) + unexpiredSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unexpired secret", + Namespace: installedInNamespace, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": unexpiredTime, + }, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(unexpiredSecret)) + r.NoError(kubeClient.Tracker().Add(unexpiredSecret)) + }) + + it("should delete any that are past their expiration", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "first expired secret"), + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "second expired secret"), + }, + kubeClient.Actions(), + ) + list, err := kubeClient.CoreV1().Secrets(installedInNamespace).List(context.Background(), metav1.ListOptions{}) + r.NoError(err) + r.Len(list.Items, 2) + r.ElementsMatch([]string{"unexpired secret", "some other unrelated secret"}, []string{list.Items[0].Name, list.Items[1].Name}) + }) + }) + + when("there is a secret with a malformed garbage-collect-after date", func() { + it.Before(func() { + malformedSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "malformed secret", + Namespace: installedInNamespace, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": "not-a-real-date-string", + }, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(malformedSecret)) + r.NoError(kubeClient.Tracker().Add(malformedSecret)) + expiredSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "expired secret", + Namespace: installedInNamespace, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": firstExpiredTime, + }, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(expiredSecret)) + r.NoError(kubeClient.Tracker().Add(expiredSecret)) + }) + + it("does not delete that secret", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "expired secret"), + }, + kubeClient.Actions(), + ) + list, err := kubeClient.CoreV1().Secrets(installedInNamespace).List(context.Background(), metav1.ListOptions{}) + r.NoError(err) + r.Len(list.Items, 2) + r.ElementsMatch([]string{"malformed secret", "some other unrelated secret"}, []string{list.Items[0].Name, list.Items[1].Name}) + }) + }) + + when("the delete call fails", func() { + it.Before(func() { + erroringSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "erroring secret", + Namespace: installedInNamespace, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": firstExpiredTime, + }, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(erroringSecret)) + r.NoError(kubeClient.Tracker().Add(erroringSecret)) + kubeClient.PrependReactor("delete", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + if action.(kubetesting.DeleteActionImpl).Name == "erroring secret" { + return true, nil, errors.New("delete failed: some delete error") + } + return false, nil, nil + }) + expiredSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "expired secret", + Namespace: installedInNamespace, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": firstExpiredTime, + }, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(expiredSecret)) + r.NoError(kubeClient.Tracker().Add(expiredSecret)) + }) + + it("continues on to delete the next one", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "erroring secret"), + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "expired secret"), + }, + kubeClient.Actions(), + ) + list, err := kubeClient.CoreV1().Secrets(installedInNamespace).List(context.Background(), metav1.ListOptions{}) + r.NoError(err) + r.Len(list.Items, 2) + r.ElementsMatch([]string{"erroring secret", "some other unrelated secret"}, []string{list.Items[0].Name, list.Items[1].Name}) + }) + }) + }, spec.Parallel(), spec.Report(report.Terminal{})) +} diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 354b6a3d..4d0080e1 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -22,6 +22,11 @@ func MatchAnythingFilter(parentFunc controllerlib.ParentFunc) controllerlib.Filt return SimpleFilter(func(object metav1.Object) bool { return true }, parentFunc) } +// MatchNothingFilter returns a controllerlib.Filter that allows no objects. +func MatchNothingFilter(parentFunc controllerlib.ParentFunc) controllerlib.Filter { + return SimpleFilter(func(object metav1.Object) bool { return false }, parentFunc) +} + // SimpleFilter takes a single boolean match function on a metav1.Object and wraps it into a proper controllerlib.Filter. func SimpleFilter(match func(metav1.Object) bool, parentFunc controllerlib.ParentFunc) controllerlib.Filter { return controllerlib.FilterFuncs{ From baa1a4a2fcfe185c823e979090dfb15ba3fd7e9c Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 11 Dec 2020 15:21:34 -0800 Subject: [PATCH 4/4] Supervisor storage garbage collection controller enabled in production - Also add more log statements to the controller - Also have the controller apply a rate limit to itself, to avoid having a very chatty controller that runs way more often than is needed. - Also add an integration test for the controller's behavior. Signed-off-by: Margo Crawford --- cmd/pinniped-supervisor/main.go | 10 ++ .../supervisorstorage/garbage_collector.go | 52 +++++-- .../garbage_collector_test.go | 85 +++++++++--- internal/controller/utils.go | 5 - internal/crud/crud.go | 8 +- ...ervisor_storage_garbage_collection_test.go | 130 ++++++++++++++++++ ...age_test.go => supervisor_storage_test.go} | 0 7 files changed, 256 insertions(+), 34 deletions(-) create mode 100644 test/integration/supervisor_storage_garbage_collection_test.go rename test/integration/{storage_test.go => supervisor_storage_test.go} (100%) diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 31f5dff8..0c54964d 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -29,6 +29,7 @@ import ( "go.pinniped.dev/internal/config/supervisor" "go.pinniped.dev/internal/controller/supervisorconfig" "go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatcher" + "go.pinniped.dev/internal/controller/supervisorstorage" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/downward" "go.pinniped.dev/internal/oidc/jwks" @@ -84,6 +85,15 @@ func startControllers( // Create controller manager. controllerManager := controllerlib. NewManager(). + WithController( + supervisorstorage.GarbageCollectorController( + clock.RealClock{}, + kubeClient, + kubeInformers.Core().V1().Secrets(), + controllerlib.WithInformer, + ), + singletonWorker, + ). WithController( supervisorconfig.NewOIDCProviderWatcherController( issuerManager, diff --git a/internal/controller/supervisorstorage/garbage_collector.go b/internal/controller/supervisorstorage/garbage_collector.go index 95b87639..d2f6aef3 100644 --- a/internal/controller/supervisorstorage/garbage_collector.go +++ b/internal/controller/supervisorstorage/garbage_collector.go @@ -6,8 +6,10 @@ package supervisorstorage import ( "time" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/util/clock" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" @@ -17,12 +19,17 @@ import ( "go.pinniped.dev/internal/plog" ) +const minimumRepeatInterval = 30 * time.Second + type garbageCollectorController struct { - secretInformer corev1informers.SecretInformer - kubeClient kubernetes.Interface + secretInformer corev1informers.SecretInformer + kubeClient kubernetes.Interface + clock clock.Clock + timeOfMostRecentSweep time.Time } func GarbageCollectorController( + clock clock.Clock, kubeClient kubernetes.Interface, secretInformer corev1informers.SecretInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, @@ -33,39 +40,66 @@ func GarbageCollectorController( Syncer: &garbageCollectorController{ secretInformer: secretInformer, kubeClient: kubeClient, + clock: clock, }, }, withInformer( secretInformer, - pinnipedcontroller.MatchNothingFilter(nil), + pinnipedcontroller.MatchAnythingFilter(nil), controllerlib.InformerOption{}, ), ) } func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error { + // The Sync method is triggered upon any change to any Secret, which would make this + // controller too chatty, so it rate limits itself to a more reasonable interval. + // Note that even during a period when no secrets are changing, it will still run + // at the informer's full-resync interval (as long as there are some secrets). + if c.clock.Now().Sub(c.timeOfMostRecentSweep) < minimumRepeatInterval { + return nil + } + + plog.Info("starting storage garbage collection sweep") + c.timeOfMostRecentSweep = c.clock.Now() + listOfSecrets, err := c.secretInformer.Lister().List(labels.Everything()) if err != nil { return err } + for i := range listOfSecrets { secret := listOfSecrets[i] - s, ok := secret.Annotations[crud.SecretLifetimeAnnotationKey] + + timeString, ok := secret.Annotations[crud.SecretLifetimeAnnotationKey] if !ok { continue } - currentTime := time.Now() - garbageCollectAfterTime, err := time.Parse(time.RFC3339, s) + + garbageCollectAfterTime, err := time.Parse(crud.SecretLifetimeAnnotationDateFormat, timeString) if err != nil { - plog.WarningErr("could not parse for garbage collection", err, "secretName", secret.Name, "garbageCollectAfter", s) + plog.WarningErr("could not parse resource timestamp for garbage collection", err, logKV(secret)) continue } - if garbageCollectAfterTime.Before(currentTime) { + + if garbageCollectAfterTime.Before(c.clock.Now()) { err = c.kubeClient.CoreV1().Secrets(secret.Namespace).Delete(ctx.Context, secret.Name, metav1.DeleteOptions{}) if err != nil { - plog.WarningErr("failed to garbage collect value", err, "secretName", secret.Name, "garbageCollectAfter", s) + plog.WarningErr("failed to garbage collect resource", err, logKV(secret)) + continue } + plog.Info("storage garbage collector deleted resource", logKV(secret)) } } + return nil } + +func logKV(secret *v1.Secret) []interface{} { + return []interface{}{ + "secretName", secret.Name, + "secretNamespace", secret.Namespace, + "secretType", string(secret.Type), + "garbageCollectAfter", secret.Annotations[crud.SecretLifetimeAnnotationKey], + } +} diff --git a/internal/controller/supervisorstorage/garbage_collector_test.go b/internal/controller/supervisorstorage/garbage_collector_test.go index e2167b88..81f104df 100644 --- a/internal/controller/supervisorstorage/garbage_collector_test.go +++ b/internal/controller/supervisorstorage/garbage_collector_test.go @@ -16,6 +16,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" kubeinformers "k8s.io/client-go/informers" kubernetesfake "k8s.io/client-go/kubernetes/fake" kubetesting "k8s.io/client-go/testing" @@ -37,6 +38,7 @@ func TestGarbageCollectorControllerInformerFilters(t *testing.T) { observableWithInformerOption = testutil.NewObservableWithInformerOption() secretsInformer := kubeinformers.NewSharedInformerFactory(nil, 0).Core().V1().Secrets() _ = GarbageCollectorController( + clock.RealClock{}, nil, secretsInformer, observableWithInformerOption.WithInformer, // make it possible to observe the behavior of the Filters @@ -57,11 +59,11 @@ func TestGarbageCollectorControllerInformerFilters(t *testing.T) { }) when("any Secret changes", func() { - it("returns false to avoid triggering the sync function", func() { - r.False(subject.Add(secret)) - r.False(subject.Update(secret, otherSecret)) - r.False(subject.Update(otherSecret, secret)) - r.False(subject.Delete(secret)) + it("returns true to trigger the sync function for all secrets", func() { + r.True(subject.Add(secret)) + r.True(subject.Update(secret, otherSecret)) + r.True(subject.Update(otherSecret, secret)) + r.True(subject.Delete(secret)) }) }) }) @@ -75,10 +77,6 @@ func TestGarbageCollectorControllerSync(t *testing.T) { Resource: "secrets", } - firstExpiredTime := time.Date(1900, time.January, 1, 1, 0, 0, 0, time.UTC).Format(time.RFC3339) - secondExpiredTime := time.Date(1901, time.January, 1, 1, 0, 0, 0, time.UTC).Format(time.RFC3339) - unexpiredTime := time.Now().Add(time.Hour * 24).UTC().Format(time.RFC3339) - spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) { const ( installedInNamespace = "some-namespace" @@ -93,6 +91,8 @@ func TestGarbageCollectorControllerSync(t *testing.T) { timeoutContext context.Context timeoutContextCancel context.CancelFunc syncContext *controllerlib.Context + fakeClock *clock.FakeClock + frozenNow time.Time ) // Defer starting the informers until the last possible moment so that the @@ -100,6 +100,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { var startInformersAndController = func() { // Set this at the last second to allow for injection of server override. subject = GarbageCollectorController( + fakeClock, kubeClient, kubeInformers.Core().V1().Secrets(), controllerlib.WithInformer, @@ -128,6 +129,8 @@ func TestGarbageCollectorControllerSync(t *testing.T) { kubeInformerClient = kubernetesfake.NewSimpleClientset() kubeClient = kubernetesfake.NewSimpleClientset() kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0) + frozenNow = time.Now().UTC() + fakeClock = clock.NewFakeClock(frozenNow) unrelatedSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -163,7 +166,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { Name: "first expired secret", Namespace: installedInNamespace, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": firstExpiredTime, + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), }, }, } @@ -174,7 +177,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { Name: "second expired secret", Namespace: installedInNamespace, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": secondExpiredTime, + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-2 * time.Second).Format(time.RFC3339), }, }, } @@ -185,7 +188,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { Name: "unexpired secret", Namespace: installedInNamespace, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": unexpiredTime, + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(time.Second).Format(time.RFC3339), }, }, } @@ -211,6 +214,54 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) }) + when("very little time has passed since the previous sync call", func() { + it.Before(func() { + // Add a secret that will expire in 20 seconds. + expiredSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "expired secret", + Namespace: installedInNamespace, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(20 * time.Second).Format(time.RFC3339), + }, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(expiredSecret)) + r.NoError(kubeClient.Tracker().Add(expiredSecret)) + }) + + it("should do nothing to avoid being super chatty since it is called for every change to any Secret, until more time has passed", func() { + startInformersAndController() + require.Empty(t, kubeClient.Actions()) + + // Run sync once with the current time set to frozenTime. + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + require.Empty(t, kubeClient.Actions()) + + // Run sync again when not enough time has passed since the most recent run, so no delete + // operations should happen even though there is a expired secret now. + fakeClock.Step(29 * time.Second) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + require.Empty(t, kubeClient.Actions()) + + // Step to the exact threshold and run Sync again. Now we are past the rate limiting period. + fakeClock.Step(1*time.Second + 1*time.Millisecond) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // It should have deleted the expired secret. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "expired secret"), + }, + kubeClient.Actions(), + ) + list, err := kubeClient.CoreV1().Secrets(installedInNamespace).List(context.Background(), metav1.ListOptions{}) + r.NoError(err) + r.Len(list.Items, 1) + r.Equal("some other unrelated secret", list.Items[0].Name) + }) + }) + when("there is a secret with a malformed garbage-collect-after date", func() { it.Before(func() { malformedSecret := &corev1.Secret{ @@ -229,7 +280,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { Name: "expired secret", Namespace: installedInNamespace, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": firstExpiredTime, + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), }, }, } @@ -254,14 +305,14 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) }) - when("the delete call fails", func() { + when("the kube API delete call fails", func() { it.Before(func() { erroringSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "erroring secret", Namespace: installedInNamespace, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": firstExpiredTime, + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), }, }, } @@ -278,7 +329,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { Name: "expired secret", Namespace: installedInNamespace, Annotations: map[string]string{ - "storage.pinniped.dev/garbage-collect-after": firstExpiredTime, + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), }, }, } @@ -286,7 +337,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { r.NoError(kubeClient.Tracker().Add(expiredSecret)) }) - it("continues on to delete the next one", func() { + it("ignores the error and continues on to delete the next expired Secret", func() { startInformersAndController() r.NoError(controllerlib.TestSync(t, subject, *syncContext)) diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 4d0080e1..354b6a3d 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -22,11 +22,6 @@ func MatchAnythingFilter(parentFunc controllerlib.ParentFunc) controllerlib.Filt return SimpleFilter(func(object metav1.Object) bool { return true }, parentFunc) } -// MatchNothingFilter returns a controllerlib.Filter that allows no objects. -func MatchNothingFilter(parentFunc controllerlib.ParentFunc) controllerlib.Filter { - return SimpleFilter(func(object metav1.Object) bool { return false }, parentFunc) -} - // SimpleFilter takes a single boolean match function on a metav1.Object and wraps it into a proper controllerlib.Filter. func SimpleFilter(match func(metav1.Object) bool, parentFunc controllerlib.ParentFunc) controllerlib.Filter { return controllerlib.FilterFuncs{ diff --git a/internal/crud/crud.go b/internal/crud/crud.go index d1edeca2..84abe142 100644 --- a/internal/crud/crud.go +++ b/internal/crud/crud.go @@ -23,8 +23,10 @@ import ( //nolint:gosec // ignore lint warnings that these are credentials const ( - SecretLabelKey = "storage.pinniped.dev/type" - SecretLifetimeAnnotationKey = "storage.pinniped.dev/garbage-collect-after" + SecretLabelKey = "storage.pinniped.dev/type" + + SecretLifetimeAnnotationKey = "storage.pinniped.dev/garbage-collect-after" + SecretLifetimeAnnotationDateFormat = time.RFC3339 secretNameFormat = "pinniped-storage-%s-%s" secretTypeFormat = "storage.pinniped.dev/%s" @@ -178,7 +180,7 @@ func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, ResourceVersion: resourceVersion, Labels: labelsToAdd, Annotations: map[string]string{ - SecretLifetimeAnnotationKey: s.clock().Add(s.lifetime).UTC().Format(time.RFC3339), + SecretLifetimeAnnotationKey: s.clock().Add(s.lifetime).UTC().Format(SecretLifetimeAnnotationDateFormat), }, OwnerReferences: nil, }, diff --git a/test/integration/supervisor_storage_garbage_collection_test.go b/test/integration/supervisor_storage_garbage_collection_test.go new file mode 100644 index 00000000..a80a6a79 --- /dev/null +++ b/test/integration/supervisor_storage_garbage_collection_test.go @@ -0,0 +1,130 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package integration + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + v1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + + "go.pinniped.dev/internal/crud" + "go.pinniped.dev/test/library" +) + +func TestStorageGarbageCollection(t *testing.T) { + // Run this test in parallel with the other integration tests because it does a lot of waiting + // and will not impact other tests, or be impacted by other tests, when run in parallel. + t.Parallel() + + env := library.IntegrationEnv(t) + client := library.NewClientset(t) + secrets := client.CoreV1().Secrets(env.SupervisorNamespace) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + secretAlreadyExpired := createSecret(ctx, t, secrets, "past", time.Now().Add(-time.Second)) + secretWhichWillExpireBeforeTheTestEnds := createSecret(ctx, t, secrets, "near-future", time.Now().Add(30*time.Second)) + secretNotYetExpired := createSecret(ctx, t, secrets, "far-future", time.Now().Add(10*time.Minute)) + + var err error + secretIsNotFound := func(secretName string) func() bool { + return func() bool { + _, err = secrets.Get(ctx, secretName, metav1.GetOptions{}) + return k8serrors.IsNotFound(err) + } + } + + // Start a background goroutine which will end as soon as the test ends. + // Keep updating a secret in the same namespace just to get the controller to respond faster. + // This is just a performance optimization because otherwise this test has to wait + // ~3 minutes for the controller's next full-resync. + stopCh := make(chan bool, 1) // It is important that this channel be buffered. + go createAndUpdateSecretEveryTwoSeconds(t, stopCh, secrets) + t.Cleanup(func() { + stopCh <- true + }) + + // Wait long enough for the next periodic sweep of the GC controller for the secrets to be deleted, which + // is the worst-case length of time that we should ever need to wait. Because of the goroutine above, + // in practice we should only need to wait about 30 seconds, which is the GC controller's self-imposed + // rate throttling time period. + slightlyLongerThanGCControllerFullResyncPeriod := 3*time.Minute + 30*time.Second + assert.Eventually(t, secretIsNotFound(secretAlreadyExpired.Name), slightlyLongerThanGCControllerFullResyncPeriod, 250*time.Millisecond) + require.Truef(t, k8serrors.IsNotFound(err), "wanted a NotFound error but got %v", err) // prints out the error and stops the test in case of failure + assert.Eventually(t, secretIsNotFound(secretWhichWillExpireBeforeTheTestEnds.Name), slightlyLongerThanGCControllerFullResyncPeriod, 250*time.Millisecond) + require.Truef(t, k8serrors.IsNotFound(err), "wanted a NotFound error but got %v", err) // prints out the error and stops the test in case of failure + + // The unexpired secret should not have been deleted within the timeframe of this test run. + _, err = secrets.Get(ctx, secretNotYetExpired.Name, metav1.GetOptions{}) + require.NoError(t, err) +} + +func createAndUpdateSecretEveryTwoSeconds(t *testing.T, stopCh chan bool, secrets corev1client.SecretInterface) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + unrelatedSecret := createSecret(ctx, t, secrets, "unrelated-to-gc", time.Time{}) + + i := 0 + for { + select { + case <-stopCh: + // Got a signal, so stop running. + return + default: + // Channel had no message, so keep running. + } + + time.Sleep(2 * time.Second) + + i++ + unrelatedSecret.Data["foo"] = []byte(fmt.Sprintf("bar-%d", i)) + var updateErr error + unrelatedSecret, updateErr = secrets.Update(ctx, unrelatedSecret, metav1.UpdateOptions{}) + require.NoError(t, updateErr) + } +} + +func createSecret(ctx context.Context, t *testing.T, secrets corev1client.SecretInterface, name string, expiresAt time.Time) *v1.Secret { + secret, err := secrets.Create(ctx, newSecret("pinniped-storage-gc-integration-test-"+name+"-", expiresAt), metav1.CreateOptions{}) + require.NoError(t, err) + + // Make sure the Secret is deleted when the test ends. + t.Cleanup(func() { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + err := secrets.Delete(ctx, secret.Name, metav1.DeleteOptions{}) + notFound := k8serrors.IsNotFound(err) + if !notFound { + // it's okay if the Secret was already deleted, but other errors are cleanup failures + require.NoError(t, err) + } + }) + + return secret +} + +func newSecret(namePrefix string, expiresAt time.Time) *v1.Secret { + annotations := map[string]string{} + if !expiresAt.Equal(time.Time{}) { + // Mark the secret for garbage collection. + annotations[crud.SecretLifetimeAnnotationKey] = expiresAt.UTC().Format(time.RFC3339) + } + return &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: namePrefix, + Annotations: annotations, + }, + Data: map[string][]byte{"some-key": []byte("fake-data")}, + Type: "storage.pinniped.dev/gc-test-integration-test", + } +} diff --git a/test/integration/storage_test.go b/test/integration/supervisor_storage_test.go similarity index 100% rename from test/integration/storage_test.go rename to test/integration/supervisor_storage_test.go