diff --git a/changelog/29312.txt b/changelog/29312.txt new file mode 100644 index 000000000000..999dc382b4f8 --- /dev/null +++ b/changelog/29312.txt @@ -0,0 +1,5 @@ +```release-note:bug +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. +``` diff --git a/vault/identity_store.go b/vault/identity_store.go index 6fec26332c60..5084d54dd177 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..159fe70066b2 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", @@ -1503,10 +1500,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 +1515,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 +1587,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 +2115,23 @@ 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 { + i.Logger().Error("error getting namespace from context", "err", err) + 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 +2145,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 +2235,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) } diff --git a/website/content/docs/enterprise/namespaces/namespace-limits.mdx b/website/content/docs/enterprise/namespaces/namespace-limits.mdx index 8f9b1830130f..cf99d00447a8 100644 --- a/website/content/docs/enterprise/namespaces/namespace-limits.mdx +++ b/website/content/docs/enterprise/namespaces/namespace-limits.mdx @@ -114,24 +114,3 @@ the following metrics: |----------------|--------------------------------------------------------------------------------------------------| | 0 – 256 | [`vault.rollback.queued`](/vault/docs/internals/telemetry/metrics/core-system#rollback-metrics) | | 0 – 60000 | [`vault.rollback.waiting`](/vault/docs/internals/telemetry/metrics/core-system#rollback-metrics) | - -## Identity secret engine warnings - -When using OIDC with many namespaces, you may see warnings in your Vault logs -from the `identity` secret mount under the `root` namespace. For example: - -```text -2023-10-24T15:47:56.594Z [WARN] secrets.identity.identity_51eb2411: error expiring OIDC public keys: err="context deadline exceeded" -2023-10-24T15:47:56.594Z [WARN] secrets.identity.identity_51eb2411: error rotating OIDC keys: err="context deadline exceeded" -``` - -The `secrets.identity` warnings occur because the root namespace is responsible -for rotating the [OIDC keys](/vault/docs/secrets/identity/oidc-provider) of all -other namespaces. - - - - Using Vault as an [OIDC provider](/vault/docs/concepts/oidc-provider) with - many namespaces can severely delay the rotation and invalidation of OIDC keys. - -