client: fix role permission issue with duplicate policies. (#18419) (#…

…18446)

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.

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

.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
+```

client/acl.go

@@ -4,6 +4,7 @@ import (
 	"time"
 
 	metrics "github.com/armon/go-metrics"
+	"github.com/hashicorp/go-set"
 	lru "github.com/hashicorp/golang-lru"
 	"github.com/hashicorp/nomad/acl"
 	"github.com/hashicorp/nomad/nomad/structs"
@@ -269,11 +270,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.
@@ -285,6 +281,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
@@ -302,7 +303,7 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT
 		cached := raw.(*cachedACLValue)
 		if cached.Age() <= c.GetConfig().ACLRoleTTL {
 			for _, policyLink := range cached.Role.Policies {
-				policyNames = append(policyNames, policyLink.Name)
+				policyNames.Insert(policyLink.Name)
 			}
 		} else {
 			expiredRoleIDs = append(expiredRoleIDs, cached.Role.ID)
@@ -313,7 +314,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
@@ -343,20 +344,18 @@ 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
 	}
 
 	// Generate a timestamp for the cache entry. We do not need to use a
 	// timestamp per ACL role response integration.
 	now := time.Now()
-
 	for _, aclRole := range roleByIDResp.ACLRoles {
-
 		// Add an entry to the cache using the generated timestamp for future
 		// expiry calculations. Any existing, expired entry will be
 		// overwritten.
@@ -365,9 +364,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
 }

client/acl_test.go

@@ -132,13 +132,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)
@@ -151,12 +151,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) {