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 bf2efb29a..878dbe60a 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 6a7ad0f22..ff13dd48c 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) {