From 0d04a9892a078558adffe2cba48147a5dbfe47fc Mon Sep 17 00:00:00 2001 From: John-Michael Faircloth Date: Wed, 8 Sep 2021 10:46:58 -0500 Subject: [PATCH] identity: enforce key param and key existence on role creation (#12208) * identity: handle creation of role without a key parameter * update docs to not require key parameter for creation of a role * add changelog * require key param when creating a role * lock create/update role; remove now redundant key check * update changelog and UTs * update change log to refelct actual implementation * remove deprecated test case --- changelog/12208.txt | 3 + vault/identity_store_oidc.go | 23 ++-- vault/identity_store_oidc_test.go | 193 ++++++++++++++++++++++++------ 3 files changed, 177 insertions(+), 42 deletions(-) create mode 100644 changelog/12208.txt diff --git a/changelog/12208.txt b/changelog/12208.txt new file mode 100644 index 000000000..ab1aa8319 --- /dev/null +++ b/changelog/12208.txt @@ -0,0 +1,3 @@ +```release-note:bug +identity: disallow creation of role without a key parameter +``` diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 4f6b96363..cb2a73745 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, @@ -942,6 +943,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) @@ -961,6 +965,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 { @@ -1010,14 +1018,15 @@ 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 - } + if entry == nil { + 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 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 { diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index dd927399a..f916cd97e 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -18,6 +18,164 @@ import ( "gopkg.in/square/go-jose.v2/jwt" ) +// 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 fail + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.CreateOperation, + Storage: storage, + }) + 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 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) + 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_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) @@ -113,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)