From 68119434531233d1a5349272e34404d614178bb5 Mon Sep 17 00:00:00 2001 From: Bruno Souza Date: Tue, 7 Jan 2025 18:16:15 -0300 Subject: [PATCH 1/5] run oidcPeriodicFunc for each namespace id store --- vault/identity_store.go | 5 +- vault/identity_store_oidc.go | 117 ++++++++++++++---------------- vault/identity_store_oidc_test.go | 80 +++++++------------- 3 files changed, 83 insertions(+), 119 deletions(-) diff --git a/vault/identity_store.go b/vault/identity_store.go index 22196269c8ff..e73b9a60c0b4 100644 --- a/vault/identity_store.go +++ b/vault/identity_store.go @@ -120,7 +120,7 @@ func NewIdentityStore(ctx context.Context, core *Core, config *logical.BackendCo }, }, PeriodicFunc: func(ctx context.Context, req *logical.Request) error { - iStore.oidcPeriodicFunc(ctx) + iStore.oidcPeriodicFunc(ctx, req.Storage) return nil }, @@ -882,8 +882,7 @@ func (i *IdentityStore) invalidateOIDCToken(ctx context.Context) { return } - // Wipe the cache for the requested namespace. This will also clear - // the shared namespace as well. + // Wipe the cache for the requested namespace if err := i.oidcCache.Flush(ns); err != nil { i.logger.Error("error flushing oidc cache", "error", err) return diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index a5c487456b2e..7ba9e413327b 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -1503,10 +1503,10 @@ func (i *IdentityStore) pathOIDCDiscovery(ctx context.Context, req *logical.Requ // getKeysCacheControlHeader returns the cache control header for all public // keys at the .well-known/keys endpoint -func (i *IdentityStore) getKeysCacheControlHeader() (string, error) { +func (i *IdentityStore) getKeysCacheControlHeader(ns *namespace.Namespace) (string, error) { // if jwksCacheControlMaxAge is set use that, otherwise fall back on the // more conservative nextRun values - jwksCacheControlMaxAge, ok, err := i.oidcCache.Get(noNamespace, "jwksCacheControlMaxAge") + jwksCacheControlMaxAge, ok, err := i.oidcCache.Get(ns, "jwksCacheControlMaxAge") if err != nil { return "", err } @@ -1518,7 +1518,7 @@ func (i *IdentityStore) getKeysCacheControlHeader() (string, error) { return fmt.Sprintf("max-age=%.0f", durationInSeconds), nil } - nextRun, ok, err := i.oidcCache.Get(noNamespace, "nextRun") + nextRun, ok, err := i.oidcCache.Get(ns, "nextRun") if err != nil { return "", err } @@ -1590,7 +1590,7 @@ func (i *IdentityStore) pathOIDCReadPublicKeys(ctx context.Context, req *logical return nil, err } if len(keys) > 0 { - header, err := i.getKeysCacheControlHeader() + header, err := i.getKeysCacheControlHeader(ns) if err != nil { return nil, err } @@ -2118,17 +2118,22 @@ func (i *IdentityStore) oidcKeyRotation(ctx context.Context, s logical.Storage) // oidcPeriodFunc is invoked by the backend's periodFunc and runs regular key // rotations and expiration actions. -func (i *IdentityStore) oidcPeriodicFunc(ctx context.Context) { +func (i *IdentityStore) oidcPeriodicFunc(ctx context.Context, s logical.Storage) { // Key rotations write to storage, so only run this on the primary cluster. // The periodic func does not run on perf standbys or DR secondaries. if i.System().ReplicationState().HasState(consts.ReplicationPerformanceSecondary) { return } - var nextRun time.Time now := time.Now() - v, ok, err := i.oidcCache.Get(noNamespace, "nextRun") + ns, err := namespace.FromContext(ctx) + if err != nil && !errors.Is(err, namespace.ErrNoNamespace) { + return + } + + var nextRun time.Time + v, ok, err := i.oidcCache.Get(ns, "nextRun") if err != nil { i.Logger().Error("error reading oidc cache", "err", err) return @@ -2142,68 +2147,52 @@ func (i *IdentityStore) oidcPeriodicFunc(ctx context.Context) { // be run at any time safely, but there is no need to invoke them (which // might be somewhat expensive if there are many roles/keys) if we're not // past any rotation/expiration TTLs. - if now.After(nextRun) { - // Initialize to a fairly distant next run time. This will be brought in - // based on key rotation times. - nextRun = now.Add(24 * time.Hour) - minJwksClientCacheDuration := time.Duration(math.MaxInt64) - - for _, ns := range i.namespacer.ListNamespaces(true) { - nsPath := ns.Path - - s := i.router.MatchingStorageByAPIPath(ctx, nsPath+"identity/oidc") - - if s == nil { - continue - } - - nextRotation, jwksClientCacheDuration, err := i.oidcKeyRotation(ctx, s) - if err != nil { - i.Logger().Warn("error rotating OIDC keys", "err", err) - } - - nextExpiration, err := i.expireOIDCPublicKeys(ctx, s) - if err != nil { - i.Logger().Warn("error expiring OIDC public keys", "err", err) - } + if now.Before(nextRun) { + return + } - if err := i.oidcCache.Flush(ns); err != nil { - i.Logger().Error("error flushing oidc cache", "err", err) - } + nextRotation, jwksClientCacheDuration, err := i.oidcKeyRotation(ctx, s) + if err != nil { + i.Logger().Warn("error rotating OIDC keys", "err", err) + } - // re-run at the soonest expiration or rotation time - if nextRotation.Before(nextRun) { - nextRun = nextRotation - } + nextExpiration, err := i.expireOIDCPublicKeys(ctx, s) + if err != nil { + i.Logger().Warn("error expiring OIDC public keys", "err", err) + } - if nextExpiration.Before(nextRun) { - nextRun = nextExpiration - } + if err := i.oidcCache.Flush(ns); err != nil { + i.Logger().Error("error flushing oidc cache", "err", err) + } - if jwksClientCacheDuration < minJwksClientCacheDuration { - minJwksClientCacheDuration = jwksClientCacheDuration - } - } + // use the soonest time between nextRotation and nextExpiration for the next run. + // Allow at most 24 hours though, keeping the legacy behavior from the original + // introduction of namespaces (unclear if necessary but safer to keep for now). + nextRun = now.Add(24 * time.Hour) + if nextRotation.Before(nextRun) { + nextRun = nextRotation + } + if nextExpiration.Before(nextRun) { + nextRun = nextExpiration + } - if err := i.oidcCache.SetDefault(noNamespace, "nextRun", nextRun); err != nil { - i.Logger().Error("error setting oidc cache", "err", err) - } + if err := i.oidcCache.SetDefault(ns, "nextRun", nextRun); err != nil { + i.Logger().Error("error setting oidc cache", "err", err) + } - if minJwksClientCacheDuration < math.MaxInt64 { - // the OIDC JWKS endpoint returns a Cache-Control HTTP header time between - // 0 and the minimum verificationTTL or minimum rotationPeriod out of all - // keys, whichever value is lower. - // - // This smooths calls from services validating JWTs to Vault, while - // ensuring that operators can assert that servers honoring the - // Cache-Control header will always have a superset of all valid keys, and - // not trust any keys longer than a jwksCacheControlMaxAge duration after a - // key is rotated out of signing use - if err := i.oidcCache.SetDefault(noNamespace, "jwksCacheControlMaxAge", minJwksClientCacheDuration); err != nil { - i.Logger().Error("error setting jwksCacheControlMaxAge in oidc cache", "err", err) - } + if jwksClientCacheDuration < math.MaxInt64 { + // the OIDC JWKS endpoint returns a Cache-Control HTTP header time between + // 0 and the minimum verificationTTL or minimum rotationPeriod out of all + // keys, whichever value is lower. + // + // This smooths calls from services validating JWTs to Vault, while + // ensuring that operators can assert that servers honoring the + // Cache-Control header will always have a superset of all valid keys, and + // not trust any keys longer than a jwksCacheControlMaxAge duration after a + // key is rotated out of signing use + if err := i.oidcCache.SetDefault(ns, "jwksCacheControlMaxAge", jwksClientCacheDuration); err != nil { + i.Logger().Error("error setting jwksCacheControlMaxAge in oidc cache", "err", err) } - } } @@ -2248,9 +2237,9 @@ func (c *oidcCache) Flush(ns *namespace.Namespace) error { return errNilNamespace } - // Remove all items from the provided namespace as well as the shared, "no namespace" section. + // Remove all items from the provided namespace for itemKey := range c.c.Items() { - if isTargetNamespacedKey(itemKey, []string{noNamespace.ID, ns.ID}) { + if isTargetNamespacedKey(itemKey, []string{ns.ID}) { c.c.Delete(itemKey) } } diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index 0edcfb900747..652b1f1492ac 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -1192,14 +1192,14 @@ func TestOIDC_PeriodicFunc(t *testing.T) { namedKeySamples := make([]*logical.StorageEntry, len(testSet.testCases)) publicKeysSamples := make([][]string, len(testSet.testCases)) for i := range testSet.testCases { - c.identityStore.oidcPeriodicFunc(ctx) + c.identityStore.oidcPeriodicFunc(ctx, storage) namedKeyEntry, _ := storage.Get(ctx, namedKeyConfigPath+testSet.namedKey.name) publicKeysEntry, _ := storage.List(ctx, publicKeysConfigPath) namedKeySamples[i] = namedKeyEntry publicKeysSamples[i] = publicKeysEntry // sleep until we are in the next cycle - where a next run will happen - v, _, _ := c.identityStore.oidcCache.Get(noNamespace, "nextRun") + v, _, _ := c.identityStore.oidcCache.Get(namespace.RootNamespace, "nextRun") nextRun := v.(time.Time) now := time.Now() diff := nextRun.Sub(now) @@ -1602,61 +1602,37 @@ func TestOIDC_isTargetNamespacedKey(t *testing.T) { func TestOIDC_Flush(t *testing.T) { c := newOIDCCache(gocache.NoExpiration, gocache.NoExpiration) ns := []*namespace.Namespace{ - noNamespace, // ns[0] is nilNamespace + {ID: "ns0"}, {ID: "ns1"}, - {ID: "ns2"}, } - // populateNs populates cache by ns with some data - populateNs := func() { - for i := range ns { - for _, val := range []string{"keyA", "keyB", "keyC"} { - if err := c.SetDefault(ns[i], val, struct{}{}); err != nil { - t.Fatal(err) - } - } - } - } - - // validate verifies that cache items exist or do not exist based on their namespaced key - verify := func(items map[string]gocache.Item, expect, doNotExpect []*namespace.Namespace) { - for _, expectNs := range expect { - found := false - for i := range items { - if isTargetNamespacedKey(i, []string{expectNs.ID}) { - found = true - break - } - } - if !found { - t.Fatalf("Expected cache to contain an entry with a namespaced key for namespace: %q but did not find one", expectNs.ID) - } - } - - for _, doNotExpectNs := range doNotExpect { - for i := range items { - if isTargetNamespacedKey(i, []string{doNotExpectNs.ID}) { - t.Fatalf("Did not expect cache to contain an entry with a namespaced key for namespace: %q but found the key: %q", doNotExpectNs.ID, i) - } - } - } + // Add a key to the cache under each namespace + if err := c.SetDefault(ns[0], "keyA", struct{}{}); err != nil { + t.Fatal(err) } - - // flushing ns1 should flush ns1 and nilNamespace but not ns2 - populateNs() - if err := c.Flush(ns[1]); err != nil { + if err := c.SetDefault(ns[1], "keyB", struct{}{}); err != nil { t.Fatal(err) } - items := c.c.Items() - verify(items, []*namespace.Namespace{ns[2]}, []*namespace.Namespace{ns[0], ns[1]}) - // flushing nilNamespace should flush nilNamespace but not ns1 or ns2 - populateNs() + // Flush ns0 if err := c.Flush(ns[0]); err != nil { t.Fatal(err) } - items = c.c.Items() - verify(items, []*namespace.Namespace{ns[1], ns[2]}, []*namespace.Namespace{ns[0]}) + + // Verify that ns0 was flushed but ns1 was not + items := c.c.Items() + ns1KeyFound := false + for i := range items { + if isTargetNamespacedKey(i, []string{ns[0].ID}) { + t.Fatalf("Expected cache to not contain an entry with a namespaced key for namespace: %q but found one", ns[0].ID) + } + if isTargetNamespacedKey(i, []string{ns[1].ID}) { + ns1KeyFound = true + } + } + if !ns1KeyFound { + t.Fatalf("Expected cache to contain an entry with a namespaced key for namespace: %q but did not find one", ns[1].ID) + } } func TestOIDC_CacheNamespaceNilCheck(t *testing.T) { @@ -1679,7 +1655,7 @@ func TestOIDC_GetKeysCacheControlHeader(t *testing.T) { c, _, _ := TestCoreUnsealed(t) // get default value - header, err := c.identityStore.getKeysCacheControlHeader() + header, err := c.identityStore.getKeysCacheControlHeader(namespace.RootNamespace) if err != nil { t.Fatalf("expected success, got error:\n%v", err) } @@ -1691,11 +1667,11 @@ func TestOIDC_GetKeysCacheControlHeader(t *testing.T) { // set nextRun nextRun := time.Now().Add(24 * time.Hour) - if err = c.identityStore.oidcCache.SetDefault(noNamespace, "nextRun", nextRun); err != nil { + if err = c.identityStore.oidcCache.SetDefault(namespace.RootNamespace, "nextRun", nextRun); err != nil { t.Fatal(err) } - header, err = c.identityStore.getKeysCacheControlHeader() + header, err = c.identityStore.getKeysCacheControlHeader(namespace.RootNamespace) if err != nil { t.Fatalf("expected success, got error:\n%v", err) } @@ -1708,11 +1684,11 @@ func TestOIDC_GetKeysCacheControlHeader(t *testing.T) { // set jwksCacheControlMaxAge durationSeconds := 60 jwksCacheControlMaxAge := time.Duration(durationSeconds) * time.Second - if err = c.identityStore.oidcCache.SetDefault(noNamespace, "jwksCacheControlMaxAge", jwksCacheControlMaxAge); err != nil { + if err = c.identityStore.oidcCache.SetDefault(namespace.RootNamespace, "jwksCacheControlMaxAge", jwksCacheControlMaxAge); err != nil { t.Fatal(err) } - header, err = c.identityStore.getKeysCacheControlHeader() + header, err = c.identityStore.getKeysCacheControlHeader(namespace.RootNamespace) if err != nil { t.Fatalf("expected success, got error:\n%v", err) } From d2559d68ee8bd7798e92038e380a263223ac3df6 Mon Sep 17 00:00:00 2001 From: Bruno Souza Date: Tue, 7 Jan 2025 18:20:11 -0300 Subject: [PATCH 2/5] remove unused noNamespace var --- vault/identity_store_oidc.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 7ba9e413327b..7cab82ed0df4 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -126,9 +126,6 @@ type oidcCache struct { var ( errNilNamespace = errors.New("nil namespace in oidc cache request") - // pseudo-namespace for cache items that don't belong to any real namespace. - noNamespace = &namespace.Namespace{ID: "__NO_NAMESPACE"} - reservedClaims = []string{ "iat", "aud", "exp", "iss", "sub", "namespace", "nonce", From bd92f1a18e1519467aeaced75843d0062a488fbe Mon Sep 17 00:00:00 2001 From: Bruno Souza Date: Tue, 7 Jan 2025 18:51:07 -0300 Subject: [PATCH 3/5] properly check for errors getting namespace not sure why I decided to ignore the NoNamespace error before or not log the unexpected error, that doesn't make sense. --- vault/identity_store_oidc.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 7cab82ed0df4..159fe70066b2 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -2125,7 +2125,8 @@ func (i *IdentityStore) oidcPeriodicFunc(ctx context.Context, s logical.Storage) now := time.Now() ns, err := namespace.FromContext(ctx) - if err != nil && !errors.Is(err, namespace.ErrNoNamespace) { + if err != nil { + i.Logger().Error("error getting namespace from context", "err", err) return } From 5dd8b3c9d7a7d577b59c71cfd5cf2d44f59f9c88 Mon Sep 17 00:00:00 2001 From: Bruno Souza Date: Wed, 8 Jan 2025 18:33:28 -0300 Subject: [PATCH 4/5] add changelog --- changelog/29312.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/29312.txt diff --git a/changelog/29312.txt b/changelog/29312.txt new file mode 100644 index 000000000000..aa629de3a653 --- /dev/null +++ b/changelog/29312.txt @@ -0,0 +1,3 @@ +```release-note:bug +identity/oidc: Run OIDC key rotations for each namespace individually. +``` From 7174313d15ce5bbcfddc18d41264d2132db9773f Mon Sep 17 00:00:00 2001 From: Bruno Souza Date: Mon, 27 Jan 2025 17:58:22 -0300 Subject: [PATCH 5/5] improve changelog --- changelog/29312.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/changelog/29312.txt b/changelog/29312.txt index aa629de3a653..999dc382b4f8 100644 --- a/changelog/29312.txt +++ b/changelog/29312.txt @@ -1,3 +1,5 @@ ```release-note:bug -identity/oidc: Run OIDC key rotations for each namespace individually. +identity/oidc (enterprise): Fix delays in rotation and invalidation of OIDC keys when there are too many namespaces. +The Cache-Control header returned by the identity/oidc/.well-known/keys endpoint now depends only on the named keys for +the queried namespace. ```