From dd70506220bd14e45e03a80bf3f82639516527df Mon Sep 17 00:00:00 2001 From: Jim Kalafut Date: Wed, 24 Oct 2018 09:33:55 -0700 Subject: [PATCH 1/2] Fix duplicate role definition check (role_id, scope) must be unique, not role_id alone. Fixes #15 --- path_roles.go | 11 ++++--- path_roles_test.go | 60 +++++++++++++++++++++++++++++++++- path_service_principal_test.go | 20 ++++++++---- 3 files changed, 79 insertions(+), 12 deletions(-) diff --git a/path_roles.go b/path_roles.go index c98b46ea..6a77ea56 100644 --- a/path_roles.go +++ b/path_roles.go @@ -166,7 +166,7 @@ func (b *azureSecretBackend) pathRoleUpdate(ctx context.Context, req *logical.Re role.AzureRoles = parsedRoles } - roleIDs := make(map[string]bool) + roleSet := make(map[azureRole]bool) for _, r := range role.AzureRoles { var roleDef authorization.RoleDefinition if r.RoleID != "" { @@ -192,12 +192,13 @@ func (b *azureSecretBackend) pathRoleUpdate(ctx context.Context, req *logical.Re roleDefID := to.String(roleDef.ID) roleDefName := to.String(roleDef.RoleName) - if roleIDs[roleDefID] { - return logical.ErrorResponse(fmt.Sprintf("duplicate role_id: '%s'", *roleDef.ID)), nil - } - roleIDs[roleDefID] = true r.RoleName, r.RoleID = roleDefName, roleDefID + + if roleSet[*r] { + return logical.ErrorResponse(fmt.Sprintf("duplicate role_id and scope: '%s', '%s'", r.RoleID, r.Scope)), nil + } + roleSet[*r] = true } if role.ApplicationObjectID == "" && len(role.AzureRoles) == 0 { diff --git a/path_roles_test.go b/path_roles_test.go index 78c547bf..d17c02fb 100644 --- a/path_roles_test.go +++ b/path_roles_test.go @@ -181,7 +181,7 @@ func TestRoleCreate(t *testing.T) { }) nilErr(t, err) if resp.IsError() { - t.Fatal("received unxpected error response") + t.Fatalf("received unexpected error response: %v", resp.Error()) } resp, err = testRoleRead(t, b, s, name) @@ -193,6 +193,64 @@ func TestRoleCreate(t *testing.T) { equal(t, "/subscriptions/FAKE_SUB_ID/providers/Microsoft.Authorization/roleDefinitions/FAKE_ROLE-Contributor", roles[1].RoleID) }) + t.Run("Duplicate role name and scope", func(t *testing.T) { + b, s := getTestBackend(t, true) + + var role = map[string]interface{}{ + "azure_roles": compactJSON(`[ + { + "role_name": "Owner", + "scope": "test_scope_1" + }, + { + "role_name": "Owner", + "scope": "test_scope_1" + } + ]`), + } + + name := generateUUID() + resp, err := b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.CreateOperation, + Path: "roles/" + name, + Data: role, + Storage: s, + }) + nilErr(t, err) + if !resp.IsError() { + t.Fatal("expected error response for duplicate role & scope") + } + }) + + t.Run("Duplicate role name, different scope", func(t *testing.T) { + b, s := getTestBackend(t, true) + + var role = map[string]interface{}{ + "azure_roles": compactJSON(`[ + { + "role_name": "Owner", + "scope": "test_scope_1" + }, + { + "role_name": "Owner", + "scope": "test_scope_2" + } + ]`), + } + + name := generateUUID() + resp, err := b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.CreateOperation, + Path: "roles/" + name, + Data: role, + Storage: s, + }) + nilErr(t, err) + if resp.IsError() { + t.Fatalf("received unexpected error response: %v", resp.Error()) + } + }) + t.Run("Role name lookup (multiple match)", func(t *testing.T) { b, s := getTestBackend(t, true) diff --git a/path_service_principal_test.go b/path_service_principal_test.go index 79226512..0d45914a 100644 --- a/path_service_principal_test.go +++ b/path_service_principal_test.go @@ -348,12 +348,20 @@ func TestCredentialInteg(t *testing.T) { nilErr(t, err) // Add a Vault role that will provide creds with Azure "Reader" permissions + // Resources groups "vault-azure-secrets-test1" and "vault-azure-secrets-test2" + // should already exist in the test infrastructure. (The test can be simplified + // to just use scope "/subscriptions/%s" if need be.) rolename := "test_role" role := map[string]interface{}{ - "azure_roles": fmt.Sprintf(`[{ - "role_name": "Reader", - "scope": "/subscriptions/%s" - }]`, subscriptionID), + "azure_roles": fmt.Sprintf(`[ + { + "role_name": "Reader", + "scope": "/subscriptions/%s/resourceGroups/vault-azure-secrets-test1" + }, + { + "role_name": "Reader", + "scope": "/subscriptions/%s/resourceGroups/vault-azure-secrets-test2" + }]`, subscriptionID, subscriptionID), } resp, err := b.HandleRequest(context.Background(), &logical.Request{ Operation: logical.CreateOperation, @@ -411,10 +419,10 @@ func TestCredentialInteg(t *testing.T) { t.Fatalf("Expected nil error on GET of new SP, got: %#v", err) } - // Verify that a role assignment was created. Get the assignment + // Verify that the role assignments were created. Get the assignment // info from Azure and verify it matches the Reader role. raIDs := resp.Secret.InternalData["role_assignment_ids"].([]string) - equal(t, 1, len(raIDs)) + equal(t, 2, len(raIDs)) ra, err := provider.raClient.GetByID(context.Background(), raIDs[0]) nilErr(t, err) From c418d147eab816612c1884e2d2845c704ff98f19 Mon Sep 17 00:00:00 2001 From: Jim Kalafut Date: Wed, 24 Oct 2018 14:19:47 -0700 Subject: [PATCH 2/2] Use only required members of struct --- path_roles.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/path_roles.go b/path_roles.go index 6a77ea56..929b9d4a 100644 --- a/path_roles.go +++ b/path_roles.go @@ -166,7 +166,7 @@ func (b *azureSecretBackend) pathRoleUpdate(ctx context.Context, req *logical.Re role.AzureRoles = parsedRoles } - roleSet := make(map[azureRole]bool) + roleSet := make(map[string]bool) for _, r := range role.AzureRoles { var roleDef authorization.RoleDefinition if r.RoleID != "" { @@ -195,10 +195,11 @@ func (b *azureSecretBackend) pathRoleUpdate(ctx context.Context, req *logical.Re r.RoleName, r.RoleID = roleDefName, roleDefID - if roleSet[*r] { + rsKey := r.RoleID + "||" + r.Scope + if roleSet[rsKey] { return logical.ErrorResponse(fmt.Sprintf("duplicate role_id and scope: '%s', '%s'", r.RoleID, r.Scope)), nil } - roleSet[*r] = true + roleSet[rsKey] = true } if role.ApplicationObjectID == "" && len(role.AzureRoles) == 0 {