From 668dc5f7a767e85d62379e3e02405d2afa93f1db Mon Sep 17 00:00:00 2001 From: James Rasell Date: Mon, 11 Sep 2023 12:52:08 +0100 Subject: [PATCH] client: fix role permission issue with duplicate policies. (#18419) This change deduplicates the ACL policy list generated from ACL roles referenced within an ACL token on the client. Previously the list could contain duplicates, which would cause erronous permission denied errors when calling client related RPC/ HTTP API endpoints. This is because the client calls the ACL get policies endpoint which subsequently ensures the caller has permission to view the ACL policies. This check is performed by comparing the requested list args with the policies referenced by the caller ACL token. When a duplicate is present, this check fails, as the check must ensure the slices match exactly. --- .changelog/18419.txt | 3 +++ client/acl.go | 23 ++++++++++++----------- client/acl_test.go | 28 ++++++++++++++++++++++++---- 3 files changed, 39 insertions(+), 15 deletions(-) create mode 100644 .changelog/18419.txt diff --git a/.changelog/18419.txt b/.changelog/18419.txt new file mode 100644 index 000000000..065a2eaf4 --- /dev/null +++ b/.changelog/18419.txt @@ -0,0 +1,3 @@ +```release-note:bug +acl: Fixed a bug where ACL tokens linked to ACL roles containing duplicate policies would cause erronous permission denined responses +``` diff --git a/client/acl.go b/client/acl.go index 0fbdea87b..206e72271 100644 --- a/client/acl.go +++ b/client/acl.go @@ -7,6 +7,7 @@ import ( "time" "github.com/armon/go-metrics" + "github.com/hashicorp/go-set" "github.com/hashicorp/nomad/acl" "github.com/hashicorp/nomad/nomad/structs" ) @@ -252,11 +253,6 @@ func (c *Client) resolvePolicies(secretID string, policies []string) ([]*structs func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLTokenRoleLink) ([]string, error) { var ( - // policyNames tracks the resolved ACL policies which are linked to the - // role. This is the output object and represents the authorisation - // this role provides token bearers. - policyNames []string - // missingRoleIDs are the roles linked which are not found within our // cache. These must be looked up from the server via and RPC, so we // can correctly identify the policy links. @@ -268,6 +264,11 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT expiredRoleIDs []string ) + // policyNames tracks the resolved ACL policies which are linked to the + // role as a deduplicated list. This is the output object and represents + // the authorisation this role provides token bearers. + policyNames := set.New[string](0) + for _, roleLink := range roleLinks { // Look within the cache to see if the role is already present. If we @@ -284,7 +285,7 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT // each policy name to our return object tracking. if entry.Age() <= c.GetConfig().ACLRoleTTL { for _, policyLink := range entry.Get().Policies { - policyNames = append(policyNames, policyLink.Name) + policyNames.Insert(policyLink.Name) } } else { expiredRoleIDs = append(expiredRoleIDs, entry.Get().ID) @@ -295,7 +296,7 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT // generate a list of linked policy names. Therefore, we can avoid making // any RPC calls. if len(missingRoleIDs)+len(expiredRoleIDs) == 0 { - return policyNames, nil + return policyNames.Slice(), nil } // Created a combined list of role IDs that we need to lookup from server @@ -325,10 +326,10 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT c.logger.Warn("failed to resolve ACL roles, using expired cached value", "error", err) for _, aclRole := range roleByIDResp.ACLRoles { for _, rolePolicyLink := range aclRole.Policies { - policyNames = append(policyNames, rolePolicyLink.Name) + policyNames.Insert(rolePolicyLink.Name) } } - return policyNames, nil + return policyNames.Slice(), nil } return nil, err } @@ -345,9 +346,9 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT // Iterate the role policy links, extracting the name and adding this // to our return response tracking. for _, rolePolicyLink := range aclRole.Policies { - policyNames = append(policyNames, rolePolicyLink.Name) + policyNames.Insert(rolePolicyLink.Name) } } - return policyNames, nil + return policyNames.Slice(), nil } diff --git a/client/acl_test.go b/client/acl_test.go index 16d97b1ba..26d8a8986 100644 --- a/client/acl_test.go +++ b/client/acl_test.go @@ -131,13 +131,13 @@ func TestClient_resolveTokenACLRoles(t *testing.T) { defer cleanup() // Create an ACL Role and a client token which is linked to this. - mockACLRole := mock.ACLRole() + mockACLRole1 := mock.ACLRole() mockACLToken := mock.ACLToken() mockACLToken.Policies = []string{} - mockACLToken.Roles = []*structs.ACLTokenRoleLink{{ID: mockACLRole.ID}} + mockACLToken.Roles = []*structs.ACLTokenRoleLink{{ID: mockACLRole1.ID}} - err := testServer.State().UpsertACLRoles(structs.MsgTypeTestSetup, 10, []*structs.ACLRole{mockACLRole}, true) + err := testServer.State().UpsertACLRoles(structs.MsgTypeTestSetup, 10, []*structs.ACLRole{mockACLRole1}, true) must.NoError(t, err) err = testServer.State().UpsertACLTokens(structs.MsgTypeTestSetup, 20, []*structs.ACLToken{mockACLToken}) must.NoError(t, err) @@ -150,12 +150,32 @@ func TestClient_resolveTokenACLRoles(t *testing.T) { // Test the cache directly and check that the ACL role previously queried // is now cached. must.Eq(t, 1, testClient.roleCache.Len()) - must.True(t, testClient.roleCache.Contains(mockACLRole.ID)) + must.True(t, testClient.roleCache.Contains(mockACLRole1.ID)) // Resolve the roles again to check we get the same results. resolvedRoles2, err := testClient.resolveTokenACLRoles(rootACLToken.SecretID, mockACLToken.Roles) must.NoError(t, err) must.SliceContainsAll(t, resolvedRoles1, resolvedRoles2) + + // Create another ACL role which will have the same ACL policy links as the + // previous + mockACLRole2 := mock.ACLRole() + must.NoError(t, + testServer.State().UpsertACLRoles( + structs.MsgTypeTestSetup, 30, []*structs.ACLRole{mockACLRole2}, true)) + + // Update the ACL token so that it links to two ACL roles, which include + // duplicate ACL policies. + mockACLToken.Roles = append(mockACLToken.Roles, &structs.ACLTokenRoleLink{ID: mockACLRole2.ID}) + must.NoError(t, + testServer.State().UpsertACLTokens( + structs.MsgTypeTestSetup, 40, []*structs.ACLToken{mockACLToken})) + + // Ensure when resolving the ACL token, we are returned a deduplicated list + // of ACL policy names. + resolvedRoles3, err := testClient.resolveTokenACLRoles(rootACLToken.SecretID, mockACLToken.Roles) + must.NoError(t, err) + must.SliceContainsAll(t, []string{"mocked-test-policy-1", "mocked-test-policy-2"}, resolvedRoles3) } func TestClient_ACL_ResolveToken_Disabled(t *testing.T) {