From c491a15c7005f1a0c7522e272d7f4fe136117a94 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 29 Jul 2021 11:25:21 -0500 Subject: [PATCH 1/8] identity: handle creation of role without a key parameter --- vault/identity_store_oidc.go | 29 +++++++++------- vault/identity_store_oidc_test.go | 56 +++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 13 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 5c70edc0f2009..19cf60e1e206e 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -989,20 +989,23 @@ 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 - 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 { + // get the key referenced by this role (if it exists) + if role.Key != "" { + var key namedKey + entry, err := req.Storage.Get(ctx, namedKeyConfigPath+role.Key) + if 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 entry != nil { + if err := entry.DecodeJSON(&key); err != nil { + return nil, err + } + } else { + return logical.ErrorResponse("cannot find key %q", role.Key), nil + } + 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 { @@ -1019,7 +1022,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 } diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index a334393ca352b..fd64f98dd8153 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -18,6 +18,61 @@ import ( "gopkg.in/square/go-jose.v2/jwt" ) +// TestOIDC_Path_OIDC_RoleNoKeyParameter +func TestOIDC_Path_OIDC_RoleNoKeyParameter(t *testing.T) { + c, _, _ := TestCoreUnsealed(t) + ctx := namespace.RootContext(nil) + storage := &logical.InmemStorage{} + + // Create a test role "test-role1" without a key param -- should succeed + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.CreateOperation, + Storage: storage, + }) + expectSuccess(t, resp, err) + + // Read "test-role1" and validate + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.ReadOperation, + Storage: storage, + }) + expectSuccess(t, resp, err) + expected := map[string]interface{}{ + "ttl": int64(86400), + "template": "", + "key": "", + "client_id": resp.Data["client_id"], + } + if diff := deep.Equal(expected, resp.Data); diff != nil { + t.Fatal(diff) + } +} + +// TestOIDC_Path_OIDC_RoleNilKeyEntry +func TestOIDC_Path_OIDC_RoleNilKeyEntry(t *testing.T) { + c, _, _ := TestCoreUnsealed(t) + ctx := namespace.RootContext(nil) + storage := &logical.InmemStorage{} + + // Create a test role "test-role1" with a non-existent key -- should fail + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.CreateOperation, + Data: map[string]interface{}{ + "key": "test-key", + }, + Storage: storage, + }) + expectError(t, resp, err) + // validate error message + expectedStrings := map[string]interface{}{ + "cannot find key \"test-key\"": true, + } + expectStrings(t, []string{resp.Data["error"].(string)}, expectedStrings) +} + // TestOIDC_Path_OIDCRoleRole tests CRUD operations for roles func TestOIDC_Path_OIDCRoleRole(t *testing.T) { c, _, _ := TestCoreUnsealed(t) @@ -1196,6 +1251,7 @@ func expectError(t *testing.T, resp *logical.Response, err error) { // expectString fails unless every string in actualStrings is also included in expectedStrings and // the length of actualStrings and expectedStrings are the same func expectStrings(t *testing.T, actualStrings []string, expectedStrings map[string]interface{}) { + t.Helper() if len(actualStrings) != len(expectedStrings) { t.Fatalf("expectStrings mismatch:\nactual strings:\n%#v\nexpected strings:\n%#v\n", actualStrings, expectedStrings) } From b58fb2601761c2dbde4fe8f9a2744b4f6d253ef9 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 29 Jul 2021 11:26:00 -0500 Subject: [PATCH 2/8] update docs to not require key parameter for creation of a role --- website/content/api-docs/secret/identity/tokens.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/content/api-docs/secret/identity/tokens.mdx b/website/content/api-docs/secret/identity/tokens.mdx index 030c6ee264b18..47507bfe1ac8b 100644 --- a/website/content/api-docs/secret/identity/tokens.mdx +++ b/website/content/api-docs/secret/identity/tokens.mdx @@ -238,7 +238,7 @@ Create or update a role. ID tokens are generated against a role and signed again - `name` `(string)` – Name of the role. -- `key` `(string)` – A configured named key, the key must already exist. +- `key` `(string: )` – A configured named key, the key must already exist. - `template` `(string: )` - The template string to use for generating tokens. This may be in string-ified JSON or base64 format. From 1c8092fec8fbb272b2aeeb953fe7dff608892322 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 29 Jul 2021 11:34:38 -0500 Subject: [PATCH 3/8] add changelog --- changelog/12208.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/12208.txt diff --git a/changelog/12208.txt b/changelog/12208.txt new file mode 100644 index 0000000000000..a741453b40112 --- /dev/null +++ b/changelog/12208.txt @@ -0,0 +1,3 @@ +```release-note:bug +identity: handle creation of role without a key parameter +``` From 73ea373251d4117c31f371ceeda38a4fe5eac694 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 29 Jul 2021 13:24:18 -0500 Subject: [PATCH 4/8] require key param when creating a role --- vault/identity_store_oidc.go | 13 ++++---- vault/identity_store_oidc_test.go | 30 +++++++------------ .../api-docs/secret/identity/tokens.mdx | 2 +- 3 files changed, 19 insertions(+), 26 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 19cf60e1e206e..5dc3ea6553012 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -946,6 +946,10 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic role.Key = d.Get("key").(string) } + if role.Key == "" { + return logical.ErrorResponse("the key parameter is required"), nil + } + if template, ok := d.GetOk("template"); ok { role.Template = template.(string) } else if req.Operation == logical.CreateOperation { @@ -996,13 +1000,12 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic if err != nil { return nil, err } - if entry != nil { - if err := entry.DecodeJSON(&key); err != nil { - return nil, err - } - } else { + if entry == nil { return logical.ErrorResponse("cannot find key %q", role.Key), 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 } diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index fd64f98dd8153..e1cd51dbdfb59 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -18,39 +18,29 @@ import ( "gopkg.in/square/go-jose.v2/jwt" ) -// TestOIDC_Path_OIDC_RoleNoKeyParameter +// TestOIDC_Path_OIDC_RoleNoKeyParameter tests that a role cannot be created +// without a key parameter func TestOIDC_Path_OIDC_RoleNoKeyParameter(t *testing.T) { c, _, _ := TestCoreUnsealed(t) ctx := namespace.RootContext(nil) storage := &logical.InmemStorage{} - // Create a test role "test-role1" without a key param -- should succeed + // Create a test role "test-role1" without a key param -- should fail resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ Path: "oidc/role/test-role1", Operation: logical.CreateOperation, Storage: storage, }) - expectSuccess(t, resp, err) - - // Read "test-role1" and validate - resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ - Path: "oidc/role/test-role1", - Operation: logical.ReadOperation, - Storage: storage, - }) - expectSuccess(t, resp, err) - expected := map[string]interface{}{ - "ttl": int64(86400), - "template": "", - "key": "", - "client_id": resp.Data["client_id"], - } - if diff := deep.Equal(expected, resp.Data); diff != nil { - t.Fatal(diff) + expectError(t, resp, err) + // validate error message + expectedStrings := map[string]interface{}{ + "the key parameter is required": true, } + expectStrings(t, []string{resp.Data["error"].(string)}, expectedStrings) } -// TestOIDC_Path_OIDC_RoleNilKeyEntry +// TestOIDC_Path_OIDC_RoleNilKeyEntry tests taht a role cannot be created when +// a key parameter is provided but the key does not exist func TestOIDC_Path_OIDC_RoleNilKeyEntry(t *testing.T) { c, _, _ := TestCoreUnsealed(t) ctx := namespace.RootContext(nil) diff --git a/website/content/api-docs/secret/identity/tokens.mdx b/website/content/api-docs/secret/identity/tokens.mdx index 47507bfe1ac8b..030c6ee264b18 100644 --- a/website/content/api-docs/secret/identity/tokens.mdx +++ b/website/content/api-docs/secret/identity/tokens.mdx @@ -238,7 +238,7 @@ Create or update a role. ID tokens are generated against a role and signed again - `name` `(string)` – Name of the role. -- `key` `(string: )` – A configured named key, the key must already exist. +- `key` `(string)` – A configured named key, the key must already exist. - `template` `(string: )` - The template string to use for generating tokens. This may be in string-ified JSON or base64 format. From a0e2449bd29bb60052d45e5487692cb0f57f2d6b Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 29 Jul 2021 13:33:19 -0500 Subject: [PATCH 5/8] lock create/update role; remove now redundant key check --- vault/identity_store_oidc.go | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 5dc3ea6553012..f9406c0ab387d 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -927,6 +927,9 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic name := d.Get("name").(string) + i.oidcLock.Lock() + defer i.oidcLock.Unlock() + var role role if req.Operation == logical.UpdateOperation { entry, err := req.Storage.Get(ctx, roleConfigPath+name) @@ -993,22 +996,20 @@ 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 (if it exists) - if role.Key != "" { - var key namedKey - entry, err := req.Storage.Get(ctx, namedKeyConfigPath+role.Key) - if err != nil { - return nil, err - } - if entry == nil { - return logical.ErrorResponse("cannot find key %q", role.Key), 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 - } + // get the key referenced by this role + var key namedKey + entry, err := req.Storage.Get(ctx, namedKeyConfigPath+role.Key) + if err != nil { + return nil, err + } + if entry == nil { + return logical.ErrorResponse("cannot find key %q", role.Key), 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 { @@ -1025,7 +1026,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 86b8311854d59c4d76271e648df15d57be92b1a7 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 30 Jul 2021 15:00:48 -0500 Subject: [PATCH 6/8] update changelog and UTs --- vault/identity_store_oidc.go | 1 + vault/identity_store_oidc_test.go | 115 +++++++++++++++++++++++++++++- 2 files changed, 115 insertions(+), 1 deletion(-) diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index f9406c0ab387d..dd659448d96e3 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -243,6 +243,7 @@ func oidcPaths(i *IdentityStore) []*framework.Path { "key": { Type: framework.TypeString, Description: "The OIDC key to use for generating tokens. The specified key must already exist.", + Required: true, }, "template": { Type: framework.TypeString, diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index e1cd51dbdfb59..5e310055bfe2c 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -39,7 +39,7 @@ func TestOIDC_Path_OIDC_RoleNoKeyParameter(t *testing.T) { expectStrings(t, []string{resp.Data["error"].(string)}, expectedStrings) } -// TestOIDC_Path_OIDC_RoleNilKeyEntry tests taht a role cannot be created when +// TestOIDC_Path_OIDC_RoleNilKeyEntry tests that a role cannot be created when // a key parameter is provided but the key does not exist func TestOIDC_Path_OIDC_RoleNilKeyEntry(t *testing.T) { c, _, _ := TestCoreUnsealed(t) @@ -63,6 +63,119 @@ func TestOIDC_Path_OIDC_RoleNilKeyEntry(t *testing.T) { expectStrings(t, []string{resp.Data["error"].(string)}, expectedStrings) } +// TestOIDC_Path_OIDCRole_UpdateNoKey test that we cannot update a role without +// prividing a key param +func TestOIDC_Path_OIDCRole_UpdateNoKey(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": "2m", + "rotation_period": "2m", + }, + Storage: storage, + }) + + // Create a test role "test-role1" with a valid key -- should succeed + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.CreateOperation, + Data: map[string]interface{}{ + "key": "test-key", + "ttl": "1m", + }, + Storage: storage, + }) + expectSuccess(t, resp, err) + + // Update "test-role1" without prividing a key param -- should succeed + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "ttl": "2m", + }, + Storage: storage, + }) + expectSuccess(t, resp, err) + + // Read "test-role1" again and validate + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.ReadOperation, + Storage: storage, + }) + expectSuccess(t, resp, err) + expected := map[string]interface{}{ + "key": "test-key", + "ttl": int64(120), + "template": "", + "client_id": resp.Data["client_id"], + } + if diff := deep.Equal(expected, resp.Data); diff != nil { + t.Fatal(diff) + } +} + +// TestOIDC_Path_OIDCRole_UpdateEmptyKey test that we cannot update a role with an +// empty key +func TestOIDC_Path_OIDCRole_UpdateEmptyKey(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-role1" with a valid key -- should succeed + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.CreateOperation, + Data: map[string]interface{}{ + "key": "test-key", + }, + Storage: storage, + }) + expectSuccess(t, resp, err) + + // Update "test-role1" with valid parameters -- should fail + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "key": "", + }, + Storage: storage, + }) + expectError(t, resp, err) + + // Read "test-role1" again and validate + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.ReadOperation, + Storage: storage, + }) + expectSuccess(t, resp, err) + expected := map[string]interface{}{ + "key": "test-key", + "ttl": int64(86400), + "template": "", + "client_id": resp.Data["client_id"], + } + if diff := deep.Equal(expected, resp.Data); diff != nil { + t.Fatal(diff) + } +} + // TestOIDC_Path_OIDCRoleRole tests CRUD operations for roles func TestOIDC_Path_OIDCRoleRole(t *testing.T) { c, _, _ := TestCoreUnsealed(t) From 174d04a59fc6ef45885235af3ae76c817d9c10f3 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 30 Jul 2021 15:02:37 -0500 Subject: [PATCH 7/8] update change log to refelct actual implementation --- changelog/12208.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/12208.txt b/changelog/12208.txt index a741453b40112..ab1aa83193da7 100644 --- a/changelog/12208.txt +++ b/changelog/12208.txt @@ -1,3 +1,3 @@ ```release-note:bug -identity: handle creation of role without a key parameter +identity: disallow creation of role without a key parameter ``` From 3ecb5d6c35f1e92d0cb0862aa3b317a2850be5f1 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Wed, 8 Sep 2021 09:43:08 -0500 Subject: [PATCH 8/8] remove deprecated test case --- vault/identity_store_oidc_test.go | 35 ------------------------------- 1 file changed, 35 deletions(-) diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index f1cb3b0425476..f916cd97ecefb 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -271,41 +271,6 @@ func TestOIDC_Path_OIDCRoleRole(t *testing.T) { } } -// TestOIDC_Path_OIDCRole_NoKey tests that a role can be created with a non-existent key -func TestOIDC_Path_OIDCRole_NoKey(t *testing.T) { - c, _, _ := TestCoreUnsealed(t) - ctx := namespace.RootContext(nil) - storage := &logical.InmemStorage{} - - // Create a test role "test-role1" with a non-existent key -- should succeed - resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ - Path: "oidc/role/test-role1", - Operation: logical.CreateOperation, - Data: map[string]interface{}{ - "key": "test-key", - }, - Storage: storage, - }) - expectSuccess(t, resp, err) - - // Read "test-role1" and validate - resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ - Path: "oidc/role/test-role1", - Operation: logical.ReadOperation, - Storage: storage, - }) - expectSuccess(t, resp, err) - expected := map[string]interface{}{ - "key": "test-key", - "ttl": int64(86400), - "template": "", - "client_id": resp.Data["client_id"], - } - if diff := deep.Equal(expected, resp.Data); diff != nil { - t.Fatal(diff) - } -} - // TestOIDC_Path_OIDCRole_InvalidTokenTTL tests the TokenTTL validation func TestOIDC_Path_OIDCRole_InvalidTokenTTL(t *testing.T) { c, _, _ := TestCoreUnsealed(t)