From e9f8af71353b17b01b70104b1c5114121a097cbf Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Wed, 21 Jul 2021 15:59:26 -0500 Subject: [PATCH 01/12] do not allow token_ttl to be longer than verification_ttl --- vault/identity_store_oidc.go | 20 +++++++++++++- vault/identity_store_oidc_test.go | 46 +++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 924050a90c1b4..e48db59c26a93 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -867,7 +867,7 @@ func (i *IdentityStore) pathOIDCRoleExistenceCheck(ctx context.Context, req *log return role != nil, nil } -// handleOIDCCreateRole is used to create a new role or update an existing one +// pathOIDCCreateUpdateRole is used to create a new role or update an existing one func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { ns, err := namespace.FromContext(ctx) if err != nil { @@ -938,6 +938,24 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic role.TokenTTL = time.Duration(d.Get("ttl").(int)) * time.Second } + // get the key referenced by this role + var key namedKey + if req.Operation == logical.CreateOperation || req.Operation == logical.UpdateOperation { + entry, err := req.Storage.Get(ctx, namedKeyConfigPath+role.Key) + if err != nil { + return nil, err + } + if entry != nil { + if err := entry.DecodeJSON(&key); err != nil { + return nil, err + } + } + } + + if role.TokenTTL > key.VerificationTTL { + return logical.ErrorResponse("a role's token_ttl cannot be longer than the verification_ttl of the key it references"), nil + } + if clientID, ok := d.GetOk("client_id"); ok { role.ClientID = clientID.(string) } diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index e49eb48ba3fd5..15aebba46075b 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -113,6 +113,51 @@ func TestOIDC_Path_OIDCRoleRole(t *testing.T) { } } +// TestOIDC_Path_OIDCRole_InvalidTokenTTL tests the TokenTTL validation +func TestOIDC_Path_OIDCRole_InvalidTokenTTL(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, + Data: map[string]interface{}{ + "verification_ttl": int64(60), + }, + Storage: storage, + }) + + // Create a test role "test-role1" with a ttl longer than the + // verification_ttl -- should fail with error + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.CreateOperation, + Data: map[string]interface{}{ + "key": "test-key", + "ttl": int64(3600), + }, + Storage: storage, + }) + expectError(t, resp, err) + + // Read "test-role1" + respReadTestRole1, err3 := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.ReadOperation, + Storage: storage, + }) + // Ensure that "test-role1" was not created + expectSuccess(t, respReadTestRole1, err3) + if respReadTestRole1 != nil { + t.Fatalf("Expected a nil response but instead got:\n%#v", respReadTestRole1) + } + if respReadTestRole1 != nil { + t.Fatalf("Expected role to have been deleted but read response was:\n%#v", respReadTestRole1) + } +} + // TestOIDC_Path_OIDCRole tests the List operation for roles func TestOIDC_Path_OIDCRole(t *testing.T) { c, _, _ := TestCoreUnsealed(t) @@ -1100,6 +1145,7 @@ func expectSuccess(t *testing.T, resp *logical.Response, err error) { } func expectError(t *testing.T, resp *logical.Response, err error) { + t.Helper() if err == nil { if resp == nil || !resp.IsError() { t.Fatalf("expected error but got success; error:\n%v\nresp: %#v", err, resp) From f2b84c1c9c5c53a5136e9c6c8357983d05fe21c1 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 22 Jul 2021 13:22:12 -0500 Subject: [PATCH 02/12] add verification when updating an existing key When updating a key, ensure any roles referencing the key do not already have a token_ttl greater than the key's verification_ttl --- vault/identity_store_oidc.go | 47 +++++++++++++++++++++++++ vault/identity_store_oidc_test.go | 58 +++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index e48db59c26a93..556ebcc470ed3 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -472,6 +472,25 @@ func (i *IdentityStore) pathOIDCCreateUpdateKey(ctx context.Context, req *logica return logical.ErrorResponse("verification_ttl cannot be longer than 10x rotation_period"), nil } + if req.Operation == logical.UpdateOperation { + // ensure any roles referencing this key do not already have a token_ttl + // greater than the key's verification_ttl + rolesReferencingTargetKeyName, err := i.getRolesReferencingTargetKeyName(ctx, req, name) + if err != nil { + return nil, err + } + for _, role := range rolesReferencingTargetKeyName { + if role.TokenTTL > key.VerificationTTL { + errorMessage := fmt.Sprintf( + "unable to update key %q because it is currently referenced by one or more roles with a token_ttl greater than %d seconds", + name, + key.VerificationTTL/time.Second, + ) + return logical.ErrorResponse(errorMessage), nil + } + } + } + if allowedClientIDsRaw, ok := d.GetOk("allowed_client_ids"); ok { key.AllowedClientIDs = allowedClientIDsRaw.([]string) } else if req.Operation == logical.CreateOperation { @@ -556,6 +575,34 @@ func (i *IdentityStore) pathOIDCReadKey(ctx context.Context, req *logical.Reques }, nil } +// getRolesReferencingTargetKeyName returns a slice of roles referenced by targetKeyName. +// Note: this is not threadsafe. It is to be called with Lock already held. +func (i *IdentityStore) getRolesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) ([]role, error) { + roleNames, err := req.Storage.List(ctx, roleConfigPath) + if err != nil { + return nil, err + } + + var tempRole role + rolesReferencingTargetKeyName := make([]role, 0) + for _, roleName := range roleNames { + entry, err := req.Storage.Get(ctx, roleConfigPath+roleName) + if err != nil { + return nil, err + } + if entry != nil { + if err := entry.DecodeJSON(&tempRole); err != nil { + return nil, err + } + if tempRole.Key == targetKeyName { + rolesReferencingTargetKeyName = append(rolesReferencingTargetKeyName, tempRole) + } + } + } + + return rolesReferencingTargetKeyName, nil +} + // handleOIDCDeleteKey is used to delete a key func (i *IdentityStore) pathOIDCDeleteKey(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { ns, err := namespace.FromContext(ctx) diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index 15aebba46075b..906a096f592de 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -329,6 +329,64 @@ func TestOIDC_Path_OIDCKeyKey(t *testing.T) { expectSuccess(t, resp, err) } +// TestOIDC_Path_OIDCKey_InvalidTokenTTL tests the TokenTTL validation +func TestOIDC_Path_OIDCKey_InvalidTokenTTL(t *testing.T) { + c, _, _ := TestCoreUnsealed(t) + ctx := namespace.RootContext(nil) + storage := &logical.InmemStorage{} + + // Create a test key "test-key" -- should succeed + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/key/test-key", + Operation: logical.CreateOperation, + Data: map[string]interface{}{ + "verification_ttl": "4m", + }, + Storage: storage, + }) + expectSuccess(t, resp, err) + + // Create a role that depends on test key + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/allowed-test-role", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "key": "test-key", + "ttl": "4m", + }, + Storage: storage, + }) + expectSuccess(t, resp, err) + + // Update "test-key" -- should fail since allowed-test-role ttl is less than 2m + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/key/test-key", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "rotation_period": "10m", + "verification_ttl": "2m", + "allowed_client_ids": "allowed-test-role", + }, + Storage: storage, + }) + expectError(t, resp, err) + + // Delete allowed-test-role + c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/allowed-test-role", + Operation: logical.DeleteOperation, + Storage: storage, + }) + + // Delete test-key -- should succeed this time because no roles depend on test-key + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/key/test-key", + Operation: logical.DeleteOperation, + Storage: storage, + }) + expectSuccess(t, resp, err) +} + // TestOIDC_Path_OIDCKey tests the List operation for keys func TestOIDC_Path_OIDCKey(t *testing.T) { c, _, _ := TestCoreUnsealed(t) From 606ffd9f4777f8174824d1601505223a3e5e88ae Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 22 Jul 2021 14:49:51 -0500 Subject: [PATCH 03/12] add changelog --- changelog/12151.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/12151.txt diff --git a/changelog/12151.txt b/changelog/12151.txt new file mode 100644 index 0000000000000..19e2eed380167 --- /dev/null +++ b/changelog/12151.txt @@ -0,0 +1,3 @@ +```release-note:improvement +identity: do not allow a role's token_ttl to be longer than the signing key's verification_ttl +``` From fc98fc0637327a3ec0d9ff21f2888ec1e2287e08 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 22 Jul 2021 15:08:37 -0500 Subject: [PATCH 04/12] remove unneeded UT check and comment --- vault/identity_store_oidc_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index 906a096f592de..910109e3b0914 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -153,9 +153,6 @@ func TestOIDC_Path_OIDCRole_InvalidTokenTTL(t *testing.T) { if respReadTestRole1 != nil { t.Fatalf("Expected a nil response but instead got:\n%#v", respReadTestRole1) } - if respReadTestRole1 != nil { - t.Fatalf("Expected role to have been deleted but read response was:\n%#v", respReadTestRole1) - } } // TestOIDC_Path_OIDCRole tests the List operation for roles @@ -378,7 +375,7 @@ func TestOIDC_Path_OIDCKey_InvalidTokenTTL(t *testing.T) { Storage: storage, }) - // Delete test-key -- should succeed this time because no roles depend on test-key + // Delete test-key resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ Path: "oidc/key/test-key", Operation: logical.DeleteOperation, From 46bbfb2bcff68e9715ff129cffb409c2c859aa6e Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 23 Jul 2021 09:34:35 -0500 Subject: [PATCH 05/12] refactor based on PR comments - remove make slice in favor of var delcaration - remove unneeded if check - validate expiry value during token generation - update changelog as bug --- changelog/12151.txt | 2 +- vault/identity_store_oidc.go | 30 +++++++++++++++++------------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/changelog/12151.txt b/changelog/12151.txt index 19e2eed380167..2d2a8342df48a 100644 --- a/changelog/12151.txt +++ b/changelog/12151.txt @@ -1,3 +1,3 @@ -```release-note:improvement +```release-note:bug identity: do not allow a role's token_ttl to be longer than the signing key's verification_ttl ``` diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 556ebcc470ed3..82ac9f216855a 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -482,7 +482,7 @@ func (i *IdentityStore) pathOIDCCreateUpdateKey(ctx context.Context, req *logica for _, role := range rolesReferencingTargetKeyName { if role.TokenTTL > key.VerificationTTL { errorMessage := fmt.Sprintf( - "unable to update key %q because it is currently referenced by one or more roles with a token_ttl greater than %d seconds", + "unable to update key %q because it is currently referenced by one or more roles with a token ttl greater than %d seconds", name, key.VerificationTTL/time.Second, ) @@ -584,7 +584,7 @@ func (i *IdentityStore) getRolesReferencingTargetKeyName(ctx context.Context, re } var tempRole role - rolesReferencingTargetKeyName := make([]role, 0) + var rolesReferencingTargetKeyName []role for _, roleName := range roleNames { entry, err := req.Storage.Get(ctx, roleConfigPath+roleName) if err != nil { @@ -794,13 +794,19 @@ func (i *IdentityStore) pathOIDCGenerateToken(ctx context.Context, req *logical. return nil, err } + expiry := role.TokenTTL + if expiry > key.VerificationTTL { + expiry := key.VerificationTTL + i.Logger().Warn("a role's token ttl cannot be longer than the verification_ttl of the key it references, setting token ttl to %d", expiry) + } + now := time.Now() idToken := idToken{ Issuer: config.effectiveIssuer, Namespace: ns.ID, Subject: req.EntityID, Audience: role.ClientID, - Expiry: now.Add(role.TokenTTL).Unix(), + Expiry: now.Add(expiry).Unix(), IssuedAt: now.Unix(), } @@ -987,20 +993,18 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic // get the key referenced by this role var key namedKey - if req.Operation == logical.CreateOperation || req.Operation == logical.UpdateOperation { - entry, err := req.Storage.Get(ctx, namedKeyConfigPath+role.Key) - if err != nil { + entry, err := req.Storage.Get(ctx, namedKeyConfigPath+role.Key) + if err != nil { + return nil, err + } + if entry != nil { + if err := entry.DecodeJSON(&key); err != nil { return nil, err } - if entry != nil { - if err := entry.DecodeJSON(&key); err != nil { - return nil, err - } - } } if role.TokenTTL > key.VerificationTTL { - return logical.ErrorResponse("a role's token_ttl cannot be longer than the verification_ttl of the key it references"), nil + return logical.ErrorResponse("a role's token ttl cannot be longer than the verification_ttl of the key it references"), nil } if clientID, ok := d.GetOk("client_id"); ok { @@ -1017,7 +1021,7 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic } // store role (which was either just created or updated) - entry, err := logical.StorageEntryJSON(roleConfigPath+name, role) + entry, err = logical.StorageEntryJSON(roleConfigPath+name, role) if err != nil { return nil, err } From 1f7c6dc2261186f431524f9a70fb4e5cd8123d02 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 23 Jul 2021 15:21:19 -0500 Subject: [PATCH 06/12] refactor get roles referencing target key names logic --- vault/identity_store_oidc.go | 48 ++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 82ac9f216855a..08d7089bd86c4 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -577,14 +577,14 @@ func (i *IdentityStore) pathOIDCReadKey(ctx context.Context, req *logical.Reques // getRolesReferencingTargetKeyName returns a slice of roles referenced by targetKeyName. // Note: this is not threadsafe. It is to be called with Lock already held. -func (i *IdentityStore) getRolesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) ([]role, error) { +func (i *IdentityStore) getRolesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) (map[string]role, error) { roleNames, err := req.Storage.List(ctx, roleConfigPath) if err != nil { return nil, err } var tempRole role - var rolesReferencingTargetKeyName []role + rolesReferencingTargetKeyName := make(map[string]role) for _, roleName := range roleNames { entry, err := req.Storage.Get(ctx, roleConfigPath+roleName) if err != nil { @@ -595,7 +595,7 @@ func (i *IdentityStore) getRolesReferencingTargetKeyName(ctx context.Context, re return nil, err } if tempRole.Key == targetKeyName { - rolesReferencingTargetKeyName = append(rolesReferencingTargetKeyName, tempRole) + rolesReferencingTargetKeyName[roleName] = tempRole } } } @@ -603,6 +603,21 @@ func (i *IdentityStore) getRolesReferencingTargetKeyName(ctx context.Context, re return rolesReferencingTargetKeyName, nil } +// getRoleNamesReferencingTargetKeyName returns a slice of strings of role +// names referenced by targetKeyName. +func (i *IdentityStore) getRoleNamesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) ([]string, error) { + rolesReferencingTargetKeyName, err := i.getRolesReferencingTargetKeyName(ctx, req, targetKeyName) + if err != nil { + return nil, err + } + + var names []string + for key, _ := range rolesReferencingTargetKeyName { + names = append(names, key) + } + return names, nil +} + // handleOIDCDeleteKey is used to delete a key func (i *IdentityStore) pathOIDCDeleteKey(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { ns, err := namespace.FromContext(ctx) @@ -614,35 +629,14 @@ func (i *IdentityStore) pathOIDCDeleteKey(ctx context.Context, req *logical.Requ i.oidcLock.Lock() - // it is an error to delete a key that is actively referenced by a role - roleNames, err := req.Storage.List(ctx, roleConfigPath) + roleNamesReferencingTargetKeyName, err := i.getRoleNamesReferencingTargetKeyName(ctx, req, targetKeyName) if err != nil { - i.oidcLock.Unlock() return nil, err } - var role *role - rolesReferencingTargetKeyName := make([]string, 0) - for _, roleName := range roleNames { - entry, err := req.Storage.Get(ctx, roleConfigPath+roleName) - if err != nil { - i.oidcLock.Unlock() - return nil, err - } - if entry != nil { - if err := entry.DecodeJSON(&role); err != nil { - i.oidcLock.Unlock() - return nil, err - } - if role.Key == targetKeyName { - rolesReferencingTargetKeyName = append(rolesReferencingTargetKeyName, roleName) - } - } - } - - if len(rolesReferencingTargetKeyName) > 0 { + if len(roleNamesReferencingTargetKeyName) > 0 { errorMessage := fmt.Sprintf("unable to delete key %q because it is currently referenced by these roles: %s", - targetKeyName, strings.Join(rolesReferencingTargetKeyName, ", ")) + targetKeyName, strings.Join(roleNamesReferencingTargetKeyName, ", ")) i.oidcLock.Unlock() return logical.ErrorResponse(errorMessage), logical.ErrInvalidRequest } From e714df7ed7cf6fd1337d5c1cb9ee5f1bcf1004fc Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 23 Jul 2021 15:28:05 -0500 Subject: [PATCH 07/12] add note about thread safety to helper func --- vault/identity_store_oidc.go | 1 + 1 file changed, 1 insertion(+) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 08d7089bd86c4..ae8b0a4fbe660 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -605,6 +605,7 @@ func (i *IdentityStore) getRolesReferencingTargetKeyName(ctx context.Context, re // getRoleNamesReferencingTargetKeyName returns a slice of strings of role // names referenced by targetKeyName. +// Note: this is not threadsafe. It is to be called with Lock already held. func (i *IdentityStore) getRoleNamesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) ([]string, error) { rolesReferencingTargetKeyName, err := i.getRolesReferencingTargetKeyName(ctx, req, targetKeyName) if err != nil { From 7b2cbb8d15324d3e54c915e3675d36bae13fa747 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Mon, 26 Jul 2021 09:39:38 -0500 Subject: [PATCH 08/12] update func 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 ae8b0a4fbe660..052171ab33795 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -575,7 +575,7 @@ func (i *IdentityStore) pathOIDCReadKey(ctx context.Context, req *logical.Reques }, nil } -// getRolesReferencingTargetKeyName returns a slice of roles referenced by targetKeyName. +// getRolesReferencingTargetKeyName returns a map of role names to roles referenced by targetKeyName. // Note: this is not threadsafe. It is to be called with Lock already held. func (i *IdentityStore) getRolesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) (map[string]role, error) { roleNames, err := req.Storage.List(ctx, roleConfigPath) From 5dddaf58eabecdb529eb21aad8dd3dd7b9a8b574 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Tue, 27 Jul 2021 09:04:01 -0500 Subject: [PATCH 09/12] sort array and refactor func names --- vault/identity_store_oidc.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 052171ab33795..309567d497e26 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -11,6 +11,7 @@ import ( "errors" "fmt" "net/url" + "sort" "strings" "time" @@ -475,11 +476,11 @@ func (i *IdentityStore) pathOIDCCreateUpdateKey(ctx context.Context, req *logica if req.Operation == logical.UpdateOperation { // ensure any roles referencing this key do not already have a token_ttl // greater than the key's verification_ttl - rolesReferencingTargetKeyName, err := i.getRolesReferencingTargetKeyName(ctx, req, name) + roles, err := i.rolesReferencingTargetKeyName(ctx, req, name) if err != nil { return nil, err } - for _, role := range rolesReferencingTargetKeyName { + for _, role := range roles { if role.TokenTTL > key.VerificationTTL { errorMessage := fmt.Sprintf( "unable to update key %q because it is currently referenced by one or more roles with a token ttl greater than %d seconds", @@ -575,16 +576,16 @@ func (i *IdentityStore) pathOIDCReadKey(ctx context.Context, req *logical.Reques }, nil } -// getRolesReferencingTargetKeyName returns a map of role names to roles referenced by targetKeyName. +// rolesReferencingTargetKeyName returns a map of role names to roles referenced by targetKeyName. // Note: this is not threadsafe. It is to be called with Lock already held. -func (i *IdentityStore) getRolesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) (map[string]role, error) { +func (i *IdentityStore) rolesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) (map[string]role, error) { roleNames, err := req.Storage.List(ctx, roleConfigPath) if err != nil { return nil, err } var tempRole role - rolesReferencingTargetKeyName := make(map[string]role) + roles := make(map[string]role) for _, roleName := range roleNames { entry, err := req.Storage.Get(ctx, roleConfigPath+roleName) if err != nil { @@ -595,27 +596,28 @@ func (i *IdentityStore) getRolesReferencingTargetKeyName(ctx context.Context, re return nil, err } if tempRole.Key == targetKeyName { - rolesReferencingTargetKeyName[roleName] = tempRole + roles[roleName] = tempRole } } } - return rolesReferencingTargetKeyName, nil + return roles, nil } -// getRoleNamesReferencingTargetKeyName returns a slice of strings of role +// roleNamesReferencingTargetKeyName returns a slice of strings of role // names referenced by targetKeyName. // Note: this is not threadsafe. It is to be called with Lock already held. -func (i *IdentityStore) getRoleNamesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) ([]string, error) { - rolesReferencingTargetKeyName, err := i.getRolesReferencingTargetKeyName(ctx, req, targetKeyName) +func (i *IdentityStore) roleNamesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) ([]string, error) { + roles, err := i.rolesReferencingTargetKeyName(ctx, req, targetKeyName) if err != nil { return nil, err } var names []string - for key, _ := range rolesReferencingTargetKeyName { + for key, _ := range roles { names = append(names, key) } + sort.Strings(names) return names, nil } @@ -630,14 +632,14 @@ func (i *IdentityStore) pathOIDCDeleteKey(ctx context.Context, req *logical.Requ i.oidcLock.Lock() - roleNamesReferencingTargetKeyName, err := i.getRoleNamesReferencingTargetKeyName(ctx, req, targetKeyName) + roleNames, err := i.roleNamesReferencingTargetKeyName(ctx, req, targetKeyName) if err != nil { return nil, err } - if len(roleNamesReferencingTargetKeyName) > 0 { + if len(roleNames) > 0 { errorMessage := fmt.Sprintf("unable to delete key %q because it is currently referenced by these roles: %s", - targetKeyName, strings.Join(roleNamesReferencingTargetKeyName, ", ")) + targetKeyName, strings.Join(roleNames, ", ")) i.oidcLock.Unlock() return logical.ErrorResponse(errorMessage), logical.ErrInvalidRequest } From 217bb1a295108c21faecb4e4409abb2fe5aae36c Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Tue, 27 Jul 2021 09:22:10 -0500 Subject: [PATCH 10/12] add warning to return response --- vault/identity_store_oidc.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 309567d497e26..e2a14336f9fa7 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -791,10 +791,12 @@ func (i *IdentityStore) pathOIDCGenerateToken(ctx context.Context, req *logical. return nil, err } + retResp := &logical.Response{} expiry := role.TokenTTL if expiry > key.VerificationTTL { expiry := key.VerificationTTL - i.Logger().Warn("a role's token ttl cannot be longer than the verification_ttl of the key it references, setting token ttl to %d", expiry) + retResp.AddWarning(fmt.Sprintf("a role's token ttl cannot be longer "+ + "than the verification_ttl of the key it references, setting token ttl to %d", expiry)) } now := time.Now() @@ -832,13 +834,12 @@ func (i *IdentityStore) pathOIDCGenerateToken(ctx context.Context, req *logical. return nil, fmt.Errorf("error signing OIDC token: %w", err) } - return &logical.Response{ - Data: map[string]interface{}{ - "token": signedIdToken, - "client_id": role.ClientID, - "ttl": int64(role.TokenTTL.Seconds()), - }, - }, nil + retResp.Data = map[string]interface{}{ + "token": signedIdToken, + "client_id": role.ClientID, + "ttl": int64(role.TokenTTL.Seconds()), + } + return retResp, nil } func (tok *idToken) generatePayload(logger hclog.Logger, template string, entity *identity.Entity, groups []*identity.Group) ([]byte, error) { From d0d4a9b4c4880d89c97656297bb14b7b778e424d Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Wed, 28 Jul 2021 16:53:52 -0500 Subject: [PATCH 11/12] remove unnecessary code from unit test --- vault/identity_store_oidc_test.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index 910109e3b0914..a334393ca352b 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -367,21 +367,6 @@ func TestOIDC_Path_OIDCKey_InvalidTokenTTL(t *testing.T) { Storage: storage, }) expectError(t, resp, err) - - // Delete allowed-test-role - c.identityStore.HandleRequest(ctx, &logical.Request{ - Path: "oidc/role/allowed-test-role", - Operation: logical.DeleteOperation, - Storage: storage, - }) - - // Delete test-key - resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ - Path: "oidc/key/test-key", - Operation: logical.DeleteOperation, - Storage: storage, - }) - expectSuccess(t, resp, err) } // TestOIDC_Path_OIDCKey tests the List operation for keys From 3d67ba235f04fd9aadf63ae89ebe8d4e17046277 Mon Sep 17 00:00:00 2001 From: John-Michael Faircloth Date: Wed, 28 Jul 2021 17:14:27 -0500 Subject: [PATCH 12/12] Update vault/identity_store_oidc.go Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> --- 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 e2a14336f9fa7..5c70edc0f2009 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -794,7 +794,7 @@ func (i *IdentityStore) pathOIDCGenerateToken(ctx context.Context, req *logical. retResp := &logical.Response{} expiry := role.TokenTTL if expiry > key.VerificationTTL { - expiry := key.VerificationTTL + expiry = key.VerificationTTL retResp.AddWarning(fmt.Sprintf("a role's token ttl cannot be longer "+ "than the verification_ttl of the key it references, setting token ttl to %d", expiry)) }