From 73c5292861e57dba675cc766902b1e33cf9f2a3f Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Wed, 16 Mar 2022 17:20:20 -0400 Subject: [PATCH 1/6] identity/oidc: fix duplicate kids in well-known --- vault/identity_store_oidc.go | 23 +++++++++++---- vault/identity_store_oidc_test.go | 48 +++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index a019ed558668..44a8f57757f6 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -621,9 +621,13 @@ func (i *IdentityStore) keyIDsByName(ctx context.Context, s logical.Storage, nam if err := entry.DecodeJSON(&key); err != nil { return keyIDs, err } + for _, k := range key.KeyRing { - keyIDs = append(keyIDs, k.KeyID) + if !strutil.StrListContains(keyIDs, k.KeyID) { + keyIDs = append(keyIDs, k.KeyID) + } } + return keyIDs, nil } @@ -1685,11 +1689,20 @@ func (i *IdentityStore) generatePublicJWKS(ctx context.Context, s logical.Storag } for _, keyID := range keyIDs { - key, err := loadOIDCPublicKey(ctx, s, keyID) - if err != nil { - return nil, err + found := false + for _, key := range jwks.Keys { + if key.KeyID == keyID { + found = true + } + } + + if !found { + key, err := loadOIDCPublicKey(ctx, s, keyID) + if err != nil { + return nil, err + } + jwks.Keys = append(jwks.Keys, *key) } - jwks.Keys = append(jwks.Keys, *key) } } diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index 8d23d63b3cf8..2f8d0e3c606a 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -759,6 +759,54 @@ func TestOIDC_PublicKeys(t *testing.T) { assertPublicKeyCount(t, ctx, storage, c, 2) } +// TestOIDC_PublicKeys tests that public keys are updated by +// key creation, rotation, and deletion +func TestOIDC_SharedPublicKeysByRoles(t *testing.T) { + c, _, _ := TestCoreUnsealed(t) + ctx := namespace.RootContext(nil) + storage := &logical.InmemStorage{} + + // Create a test key "test-key" + c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/key/test-key", + Operation: logical.CreateOperation, + Storage: storage, + }) + + // Create a test role "test-role" + c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role", + Operation: logical.CreateOperation, + Data: map[string]interface{}{ + "key": "test-key", + }, + Storage: storage, + }) + + // Create a test role "test-role2" + c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role2", + Operation: logical.CreateOperation, + Data: map[string]interface{}{ + "key": "test-key", + }, + Storage: storage, + }) + + // Create a test role "test-role3" + c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role3", + Operation: logical.CreateOperation, + Data: map[string]interface{}{ + "key": "test-key", + }, + Storage: storage, + }) + + // .well-known/keys should contain 2 public keys + assertPublicKeyCount(t, ctx, storage, c, 2) +} + // TestOIDC_SignIDToken tests acquiring a signed token and verifying the public portion // of the signing key func TestOIDC_SignIDToken(t *testing.T) { From 121d95aba4b9e02a38969dc7846b2713347cdbd2 Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Wed, 16 Mar 2022 17:21:46 -0400 Subject: [PATCH 2/6] Remove unused check --- vault/identity_store_oidc.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 44a8f57757f6..e69c154b2b44 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -623,9 +623,7 @@ func (i *IdentityStore) keyIDsByName(ctx context.Context, s logical.Storage, nam } for _, k := range key.KeyRing { - if !strutil.StrListContains(keyIDs, k.KeyID) { - keyIDs = append(keyIDs, k.KeyID) - } + keyIDs = append(keyIDs, k.KeyID) } return keyIDs, nil From 76e53ef757acf987f87cbcce928001bd7e40728b Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Wed, 16 Mar 2022 17:26:11 -0400 Subject: [PATCH 3/6] changelog --- changelog/14543.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/14543.txt diff --git a/changelog/14543.txt b/changelog/14543.txt new file mode 100644 index 000000000000..300564443593 --- /dev/null +++ b/changelog/14543.txt @@ -0,0 +1,3 @@ +```release-note:bug +identity/oidc: Fix duplicate keys in well-known +``` From e7e28589596425f12a0eb5b1cfa5e260cdc811de Mon Sep 17 00:00:00 2001 From: Austin Gebauer Date: Wed, 16 Mar 2022 16:45:46 -0700 Subject: [PATCH 4/6] use map-based approach to dedup key IDs --- vault/identity_store_oidc.go | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index e69c154b2b44..0ebcbadeae03 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -1672,6 +1672,8 @@ func (i *IdentityStore) generatePublicJWKS(ctx context.Context, s logical.Storag return nil, err } + // collect and deduplicate the key IDs for all roles + keyIDs := make(map[string]struct{}) for _, roleName := range roleNames { role, err := i.getOIDCRole(ctx, s, roleName) if err != nil { @@ -1681,27 +1683,23 @@ func (i *IdentityStore) generatePublicJWKS(ctx context.Context, s logical.Storag continue } - keyIDs, err := i.keyIDsByName(ctx, s, role.Key) + roleKeyIDs, err := i.keyIDsByName(ctx, s, role.Key) if err != nil { return nil, err } - for _, keyID := range keyIDs { - found := false - for _, key := range jwks.Keys { - if key.KeyID == keyID { - found = true - } - } + for _, keyID := range roleKeyIDs { + keyIDs[keyID] = struct{}{} + } + } - if !found { - key, err := loadOIDCPublicKey(ctx, s, keyID) - if err != nil { - return nil, err - } - jwks.Keys = append(jwks.Keys, *key) - } + // load the JSON web key for each key ID + for keyID := range keyIDs { + key, err := loadOIDCPublicKey(ctx, s, keyID) + if err != nil { + return nil, err } + jwks.Keys = append(jwks.Keys, *key) } if err := i.oidcCache.SetDefault(ns, "jwks", jwks); err != nil { From 93cc19323ec465548a5ad622fa81cafa768de720 Mon Sep 17 00:00:00 2001 From: Austin Gebauer Date: Wed, 16 Mar 2022 16:48:41 -0700 Subject: [PATCH 5/6] improve changelog description --- changelog/14543.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/14543.txt b/changelog/14543.txt index 300564443593..649d1e72cc86 100644 --- a/changelog/14543.txt +++ b/changelog/14543.txt @@ -1,3 +1,3 @@ ```release-note:bug -identity/oidc: Fix duplicate keys in well-known +identity/token: Fixes a bug where duplicate public keys could appear in the .well-known JWKS ``` From d16505f2073fbc5ad2854814b280287b8f08e3f6 Mon Sep 17 00:00:00 2001 From: Austin Gebauer Date: Wed, 16 Mar 2022 17:37:06 -0700 Subject: [PATCH 6/6] move jwks closer to usage; specify capacity --- vault/identity_store_oidc.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 0ebcbadeae03..f30edf598730 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -1662,10 +1662,6 @@ func (i *IdentityStore) generatePublicJWKS(ctx context.Context, s logical.Storag return nil, err } - jwks := &jose.JSONWebKeySet{ - Keys: make([]jose.JSONWebKey, 0), - } - // only return keys that are associated with a role roleNames, err := s.List(ctx, roleConfigPath) if err != nil { @@ -1693,6 +1689,10 @@ func (i *IdentityStore) generatePublicJWKS(ctx context.Context, s logical.Storag } } + jwks := &jose.JSONWebKeySet{ + Keys: make([]jose.JSONWebKey, 0, len(keyIDs)), + } + // load the JSON web key for each key ID for keyID := range keyIDs { key, err := loadOIDCPublicKey(ctx, s, keyID)