Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VAULT-21474 Run oidcPeriodicFunc for each namespace id store #29312

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

VAULT-21474 Run oidcPeriodicFunc for each namespace id store #29312

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6811943
run oidcPeriodicFunc for each namespace id store
bosouza Jan 7, 2025
d2559d6
remove unused noNamespace var
bosouza Jan 7, 2025
bd92f1a
properly check for errors getting namespace
bosouza Jan 7, 2025
5dd8b3c
add changelog
bosouza Jan 8, 2025
7174313
improve changelog
bosouza Jan 27, 2025
e191ee3
Merge branch 'main' into bosouza/VAULT-21474-namespace-oidc-rotation
bosouza Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/29312.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
identity/oidc: Run OIDC key rotations for each namespace individually.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a change release note to say that the cache control header returned from OIDC queries will only be for the queried namespace?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to connect with @austingebauer and @fairclothjm, since I know they've done worth with OIDC before, to try to see whether this change in behavior is problematic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was planning on adding a separate changelog on the ent PR but that doesn't make much sense... yeah I'll update this entry with more details regarding what this means for enterprise

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miagilepner John took a look at the enterprise PR here. He also asked me to run a quick smoke test for upgrade which I did and didn't see anything weird

```
5 changes: 2 additions & 3 deletions vault/identity_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
Expand Down Expand Up @@ -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
Expand Down
121 changes: 54 additions & 67 deletions vault/identity_store_oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,6 @@
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",
Expand Down Expand Up @@ -1503,10 +1500,10 @@

// 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
}
Expand All @@ -1518,7 +1515,7 @@
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
}
Expand Down Expand Up @@ -1590,7 +1587,7 @@
return nil, err
}
if len(keys) > 0 {
header, err := i.getKeysCacheControlHeader()
header, err := i.getKeysCacheControlHeader(ns)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -2118,17 +2115,23 @@

// 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)
Dismissed Show dismissed Hide dismissed
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
Expand All @@ -2142,68 +2145,52 @@
// 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)
Dismissed Show dismissed Hide dismissed
}

// 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)
Dismissed Show dismissed Hide dismissed
}

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)
}

}
}

Expand Down Expand Up @@ -2248,9 +2235,9 @@
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)
}
}
Expand Down
80 changes: 28 additions & 52 deletions vault/identity_store_oidc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand Down
Loading