From 455e94bc6007b7bd8bfe94176b4374fb6e559a3a Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Wed, 6 Oct 2021 09:27:45 -0500 Subject: [PATCH 1/9] filter identity token keys --- vault/identity_store_oidc.go | 62 ++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 8dd8be260688..b3288b791307 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -613,6 +613,61 @@ func (i *IdentityStore) pathOIDCReadKey(ctx context.Context, req *logical.Reques }, nil } +func (i *IdentityStore) keyIDsByName(ctx context.Context, s logical.Storage, name string) ([]string, error) { + entry, err := s.Get(ctx, namedKeyConfigPath+name) + var keyIDs []string + if err != nil { + return keyIDs, err + } + if entry == nil { + return keyIDs, nil + } + + var key *namedKey + if err := entry.DecodeJSON(&key); err != nil { + return keyIDs, err + } + keyIDs = make([]string, 0) + for _, k := range key.KeyRing { + keyIDs = append(keyIDs, k.KeyID) + } + return keyIDs, nil +} + +// roleReferencesTargetKeyID returns a true if a role references the given +// targetKeyID. +func (i *IdentityStore) roleReferencesTargetKeyID(ctx context.Context, s logical.Storage, targetKeyID string) (bool, error) { + roleNames, err := s.List(ctx, roleConfigPath) + if err != nil { + return false, err + } + + var tempRole role + for _, roleName := range roleNames { + entry, err := s.Get(ctx, roleConfigPath+roleName) + if err != nil { + return false, err + } + if entry != nil { + if err := entry.DecodeJSON(&tempRole); err != nil { + return false, err + } + ids, err := i.keyIDsByName(ctx, s, tempRole.Key) + if err != nil { + return false, err + } + + for _, id := range ids { + if id == targetKeyID { + return true, nil + } + } + } + } + + return false, nil +} + // rolesReferencingTargetKeyName returns a map of role names to roles // referencing targetKeyName. // @@ -1548,6 +1603,13 @@ func (i *IdentityStore) generatePublicJWKS(ctx context.Context, s logical.Storag } for _, keyID := range keyIDs { + ok, err := i.roleReferencesTargetKeyID(ctx, s, keyID) + if err != nil { + return nil, err + } + if !ok { + continue + } key, err := loadOIDCPublicKey(ctx, s, keyID) if err != nil { return nil, err From 6729ff93032208afa8399fef8868a4edff75f8db Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 7 Oct 2021 10:18:47 -0500 Subject: [PATCH 2/9] Update test cases to associate keys with roles --- vault/identity_store_oidc_test.go | 106 ++++++++++++++++++++---------- 1 file changed, 72 insertions(+), 34 deletions(-) diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index 45a5da3ee9ef..d75c28130a49 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -1,6 +1,7 @@ package vault import ( + "context" "crypto/rand" "crypto/rsa" "encoding/json" @@ -637,37 +638,72 @@ func TestOIDC_Path_OIDCKey_DeleteWithExistingClient(t *testing.T) { expectError(t, resp, err) } -// TestOIDC_PublicKeys tests that public keys are updated by -// key creation, rotation, and deletion -func TestOIDC_PublicKeys(t *testing.T) { +// TestOIDC_PublicKeys_NoRole tests that public keys are not returned by the +// oidc/.well-known/keys endpoint when they are not associated with a role +func TestOIDC_PublicKeys_NoRole(t *testing.T) { c, _, _ := TestCoreUnsealed(t) ctx := namespace.RootContext(nil) - storage := &logical.InmemStorage{} + s := &logical.InmemStorage{} // Create a test key "test-key" c.identityStore.HandleRequest(ctx, &logical.Request{ Path: "oidc/key/test-key", Operation: logical.CreateOperation, - Storage: storage, + Storage: s, }) - // .well-known/keys should contain 2 public keys + // .well-known/keys should contain 0 public keys + assertPublicKeyCount(t, ctx, s, c, 0) +} + +func assertPublicKeyCount(t *testing.T, ctx context.Context, s logical.Storage, c *Core, keyCount int) { + t.Helper() + + // .well-known/keys should contain keyCount public keys resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ Path: "oidc/.well-known/keys", Operation: logical.ReadOperation, - Storage: storage, + Storage: s, }) expectSuccess(t, resp, err) // parse response responseJWKS := &jose.JSONWebKeySet{} json.Unmarshal(resp.Data["http_raw_body"].([]byte), responseJWKS) - if len(responseJWKS.Keys) != 2 { - t.Fatalf("expected 2 public keys but instead got %d", len(responseJWKS.Keys)) + if len(responseJWKS.Keys) != keyCount { + t.Fatalf("expected %d public keys but instead got %d", keyCount, len(responseJWKS.Keys)) } +} + +// TestOIDC_PublicKeys tests that public keys are updated by +// key creation, rotation, and deletion +func TestOIDC_PublicKeys(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, + }) + + // .well-known/keys should contain 2 public keys + assertPublicKeyCount(t, ctx, storage, c, 2) // rotate test-key a few times, each rotate should increase the length of public keys returned // by the .well-known endpoint - resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ Path: "oidc/key/test-key/rotate", Operation: logical.UpdateOperation, Storage: storage, @@ -681,45 +717,47 @@ func TestOIDC_PublicKeys(t *testing.T) { expectSuccess(t, resp, err) // .well-known/keys should contain 4 public keys + assertPublicKeyCount(t, ctx, storage, c, 4) + + // create another named key "test-key2" resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ - Path: "oidc/.well-known/keys", - Operation: logical.ReadOperation, + Path: "oidc/key/test-key2", + Operation: logical.CreateOperation, Storage: storage, }) expectSuccess(t, resp, err) - // parse response - json.Unmarshal(resp.Data["http_raw_body"].([]byte), responseJWKS) - if len(responseJWKS.Keys) != 4 { - t.Fatalf("expected 4 public keys but instead got %d", len(responseJWKS.Keys)) - } - // create another named key - c.identityStore.HandleRequest(ctx, &logical.Request{ - Path: "oidc/key/test-key2", + // Create a test role "test-role2" + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role2", Operation: logical.CreateOperation, - Storage: storage, + Data: map[string]interface{}{ + "key": "test-key2", + }, + Storage: storage, }) + expectSuccess(t, resp, err) + // .well-known/keys should contain 6 public keys + assertPublicKeyCount(t, ctx, storage, c, 6) - // delete test key - c.identityStore.HandleRequest(ctx, &logical.Request{ - Path: "oidc/key/test-key", + // delete test role that references "test-key" + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role", Operation: logical.DeleteOperation, Storage: storage, }) - - // .well-known/keys should contain 2 public key, all of the public keys - // from named key "test-key" should have been deleted + expectSuccess(t, resp, err) + // delete test key resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ - Path: "oidc/.well-known/keys", - Operation: logical.ReadOperation, + Path: "oidc/key/test-key", + Operation: logical.DeleteOperation, Storage: storage, }) expectSuccess(t, resp, err) - // parse response - json.Unmarshal(resp.Data["http_raw_body"].([]byte), responseJWKS) - if len(responseJWKS.Keys) != 2 { - t.Fatalf("expected 2 public keys but instead got %d", len(responseJWKS.Keys)) - } + + // .well-known/keys should contain 2 public keys, all of the public keys + // from named key "test-key" should have been deleted + assertPublicKeyCount(t, ctx, storage, c, 2) } // TestOIDC_SignIDToken tests acquiring a signed token and verifying the public portion From fa1ed765e369609690e056ac9db60a91c9a8e66d Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 8 Oct 2021 14:53:32 -0500 Subject: [PATCH 3/9] use getOIDCRole helper --- vault/identity_store_oidc.go | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index b3288b791307..631235aac301 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -642,25 +642,19 @@ func (i *IdentityStore) roleReferencesTargetKeyID(ctx context.Context, s logical return false, err } - var tempRole role for _, roleName := range roleNames { - entry, err := s.Get(ctx, roleConfigPath+roleName) + role, err := i.getOIDCRole(ctx, s, roleName) + if err != nil { + return false, err + } + ids, err := i.keyIDsByName(ctx, s, role.Key) if err != nil { return false, err } - if entry != nil { - if err := entry.DecodeJSON(&tempRole); err != nil { - return false, err - } - ids, err := i.keyIDsByName(ctx, s, tempRole.Key) - if err != nil { - return false, err - } - for _, id := range ids { - if id == targetKeyID { - return true, nil - } + for _, id := range ids { + if id == targetKeyID { + return true, nil } } } From 6e214c7fb97aa91c8bfe2574387a477343696587 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 8 Oct 2021 15:01:52 -0500 Subject: [PATCH 4/9] add func comment and test assertion --- vault/identity_store_oidc.go | 3 ++- vault/identity_store_oidc_test.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 631235aac301..ffd7e9f49136 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -613,9 +613,10 @@ func (i *IdentityStore) pathOIDCReadKey(ctx context.Context, req *logical.Reques }, nil } +// keyIDsByName will return a slice of key IDs for the given key name func (i *IdentityStore) keyIDsByName(ctx context.Context, s logical.Storage, name string) ([]string, error) { - entry, err := s.Get(ctx, namedKeyConfigPath+name) var keyIDs []string + entry, err := s.Get(ctx, namedKeyConfigPath+name) if err != nil { return keyIDs, err } diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index d75c28130a49..ba63a940f896 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -646,11 +646,12 @@ func TestOIDC_PublicKeys_NoRole(t *testing.T) { s := &logical.InmemStorage{} // Create a test key "test-key" - c.identityStore.HandleRequest(ctx, &logical.Request{ + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ Path: "oidc/key/test-key", Operation: logical.CreateOperation, Storage: s, }) + expectSuccess(t, resp, err) // .well-known/keys should contain 0 public keys assertPublicKeyCount(t, ctx, s, c, 0) From 0718e97e9bd682da958da9d0a8b83733e7794a3c Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 8 Oct 2021 15:35:04 -0500 Subject: [PATCH 5/9] add changelog --- changelog/12780.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/12780.txt diff --git a/changelog/12780.txt b/changelog/12780.txt new file mode 100644 index 000000000000..6ae867707f0d --- /dev/null +++ b/changelog/12780.txt @@ -0,0 +1,3 @@ +```release-note:improvement +identity: Prevent clients to the identity token system from reading/caching keys that aren’t eligible to sign tokens (aren’t associated with any role). +``` From d73eb3f2717a0d2eaea636860f5a4810742a843c Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Mon, 11 Oct 2021 09:16:02 -0500 Subject: [PATCH 6/9] remove unnecessary code --- vault/identity_store_oidc.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index ffd7e9f49136..9f1f3599d789 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -624,18 +624,17 @@ func (i *IdentityStore) keyIDsByName(ctx context.Context, s logical.Storage, nam return keyIDs, nil } - var key *namedKey + var key namedKey if err := entry.DecodeJSON(&key); err != nil { return keyIDs, err } - keyIDs = make([]string, 0) for _, k := range key.KeyRing { keyIDs = append(keyIDs, k.KeyID) } return keyIDs, nil } -// roleReferencesTargetKeyID returns a true if a role references the given +// roleReferencesTargetKeyID returns true if a role references the given // targetKeyID. func (i *IdentityStore) roleReferencesTargetKeyID(ctx context.Context, s logical.Storage, targetKeyID string) (bool, error) { roleNames, err := s.List(ctx, roleConfigPath) From 15155682ef6fcdc8786e6bf85fe6f7ccdf53c922 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Mon, 11 Oct 2021 14:04:31 -0500 Subject: [PATCH 7/9] build list of keys to return by starting with a list of roles --- vault/identity_store_oidc.go | 57 ++++++++++++------------------------ 1 file changed, 19 insertions(+), 38 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 9f1f3599d789..35b011599bb2 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -634,34 +634,6 @@ func (i *IdentityStore) keyIDsByName(ctx context.Context, s logical.Storage, nam return keyIDs, nil } -// roleReferencesTargetKeyID returns true if a role references the given -// targetKeyID. -func (i *IdentityStore) roleReferencesTargetKeyID(ctx context.Context, s logical.Storage, targetKeyID string) (bool, error) { - roleNames, err := s.List(ctx, roleConfigPath) - if err != nil { - return false, err - } - - for _, roleName := range roleNames { - role, err := i.getOIDCRole(ctx, s, roleName) - if err != nil { - return false, err - } - ids, err := i.keyIDsByName(ctx, s, role.Key) - if err != nil { - return false, err - } - - for _, id := range ids { - if id == targetKeyID { - return true, nil - } - } - } - - return false, nil -} - // rolesReferencingTargetKeyName returns a map of role names to roles // referencing targetKeyName. // @@ -1587,28 +1559,37 @@ func (i *IdentityStore) generatePublicJWKS(ctx context.Context, s logical.Storag return nil, err } - keyIDs, err := listOIDCPublicKeys(ctx, s) - if err != nil { - return nil, err + jwks := &jose.JSONWebKeySet{ + Keys: make([]jose.JSONWebKey, 0), } - jwks := &jose.JSONWebKeySet{ - Keys: make([]jose.JSONWebKey, 0, len(keyIDs)), + roleNames, err := s.List(ctx, roleConfigPath) + if err != nil { + return nil, err } - for _, keyID := range keyIDs { - ok, err := i.roleReferencesTargetKeyID(ctx, s, keyID) + for _, roleName := range roleNames { + role, err := i.getOIDCRole(ctx, s, roleName) if err != nil { return nil, err } - if !ok { + if role == nil { + // only return keys that are associated with a role continue } - key, err := loadOIDCPublicKey(ctx, s, keyID) + + keyIDs, err := i.keyIDsByName(ctx, s, role.Key) if err != nil { return nil, err } - jwks.Keys = append(jwks.Keys, *key) + + 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 7a8c172687943b19f1eaad3430a1be95b36e9fe9 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Mon, 11 Oct 2021 14:07:04 -0500 Subject: [PATCH 8/9] move comment --- vault/identity_store_oidc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 35b011599bb2..ce4d628b869e 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -1563,6 +1563,7 @@ func (i *IdentityStore) generatePublicJWKS(ctx context.Context, s logical.Storag Keys: make([]jose.JSONWebKey, 0), } + // only return keys that are associated with a role roleNames, err := s.List(ctx, roleConfigPath) if err != nil { return nil, err @@ -1574,7 +1575,6 @@ func (i *IdentityStore) generatePublicJWKS(ctx context.Context, s logical.Storag return nil, err } if role == nil { - // only return keys that are associated with a role continue } From 47c8e3d5163cc1553ad2f96ae4b13230484c8705 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Tue, 12 Oct 2021 06:50:13 -0500 Subject: [PATCH 9/9] update changelog --- changelog/12780.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/12780.txt b/changelog/12780.txt index 6ae867707f0d..61a2c5d4f8cc 100644 --- a/changelog/12780.txt +++ b/changelog/12780.txt @@ -1,3 +1,3 @@ ```release-note:improvement -identity: Prevent clients to the identity token system from reading/caching keys that aren’t eligible to sign tokens (aren’t associated with any role). +identity/token: Only return keys from the `.well-known/keys` endpoint that are being used by roles to sign/verify tokens. ```