diff --git a/.changelog/18419.txt b/.changelog/18419.txt new file mode 100644 index 000000000000..065a2eaf41a4 --- /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 19efb6cd083d..db9b162d542b 100644 --- a/client/acl.go +++ b/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,10 +344,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 } @@ -354,9 +355,7 @@ func (c *Client) resolveTokenACLRoles(secretID string, roleLinks []*structs.ACLT // 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 } diff --git a/client/acl_test.go b/client/acl_test.go index 7a108b8db894..0de41bf38d2f 100644 --- a/client/acl_test.go +++ b/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) {