acl/client: fix incorrect denied error on calls with dangling policies. (#18972)

When a user performs a client API call, the Nomad client will
perform an RPC which looks up the ACL policies which the callers
ACL token is assigned. If the ACL token includes dangling (deleted)
policies, the call would previously fail with a permission denied
error.

This change ensures this error is not returned and that the lookup
will succeed in the event of dangling policies.

.changelog/18972.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+client: Fixed a bug where client API calls would fail incorrectly with permission denied errors when using ACL tokens with dangling policies
+```

nomad/acl_endpoint.go

@@ -389,28 +389,33 @@ func (a *ACL) GetPolicies(args *structs.ACLPolicySetRequest, reply *structs.ACLP
 	// Add the token policies which are directly referenced into the set.
 	tokenPolicyNames.InsertSlice(token.Policies)
 
-	// Ensure the token has enough permissions to query the named policies.
-	if token.Type != structs.ACLManagementToken && !tokenPolicyNames.ContainsSlice(args.Names) {
-		return structs.ErrPermissionDenied
-	}
-
 	// Setup the blocking query
 	opts := blockingOptions{
 		queryOpts: &args.QueryOptions,
 		queryMeta: &reply.QueryMeta,
 		run: func(ws memdb.WatchSet, state *state.StateStore) error {
 			// Setup the output
-			reply.Policies = make(map[string]*structs.ACLPolicy, len(args.Names))
+			reply.Policies = make(map[string]*structs.ACLPolicy)
 
-			// Look for the policy
+			// Look for the policy and check whether the caller has the
+			// permission to view it. This endpoint is used by the replication
+			// process, or Nomad clients looking up a callers policies. It is
+			// therefore safe, to perform auth checks here and ensures we do
+			// not return erroneous permission denied errors when a caller uses
+			// a token with dangling policies.
 			for _, policyName := range args.Names {
 				out, err := state.ACLPolicyByName(ws, policyName)
 				if err != nil {
 					return err
 				}
-				if out != nil {
-					reply.Policies[policyName] = out
+				if out == nil {
+					continue
 				}
+
+				if token.Type != structs.ACLManagementToken && !tokenPolicyNames.Contains(policyName) {
+					return structs.ErrPermissionDenied
+				}
+				reply.Policies[policyName] = out
 			}
 
 			// Use the last index that affected the policy table

nomad/acl_endpoint_test.go

@@ -271,11 +271,11 @@ func TestACLEndpoint_GetPolicies_TokenSubset(t *testing.T) {
 	// Create the register request
 	policy := mock.ACLPolicy()
 	policy2 := mock.ACLPolicy()
-	s1.fsm.State().UpsertACLPolicies(structs.MsgTypeTestSetup, 1000, []*structs.ACLPolicy{policy, policy2})
+	must.NoError(t, s1.fsm.State().UpsertACLPolicies(structs.MsgTypeTestSetup, 1000, []*structs.ACLPolicy{policy, policy2}))
 
 	token := mock.ACLToken()
 	token.Policies = []string{policy.Name}
-	s1.fsm.State().UpsertACLTokens(structs.MsgTypeTestSetup, 1000, []*structs.ACLToken{token})
+	must.NoError(t, s1.fsm.State().UpsertACLTokens(structs.MsgTypeTestSetup, 1000, []*structs.ACLToken{token}))
 
 	// Lookup the policy which is a subset of our tokens
 	get := &structs.ACLPolicySetRequest{
@@ -286,19 +286,15 @@ func TestACLEndpoint_GetPolicies_TokenSubset(t *testing.T) {
 		},
 	}
 	var resp structs.ACLPolicySetResponse
-	if err := msgpackrpc.CallWithCodec(codec, "ACL.GetPolicies", get, &resp); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	assert.Equal(t, uint64(1000), resp.Index)
-	assert.Equal(t, 1, len(resp.Policies))
-	assert.Equal(t, policy, resp.Policies[policy.Name])
+	must.NoError(t, msgpackrpc.CallWithCodec(codec, "ACL.GetPolicies", get, &resp))
+	must.Eq(t, uint64(1000), resp.Index)
+	must.Eq(t, 1, len(resp.Policies))
+	must.Eq(t, policy, resp.Policies[policy.Name])
 
 	// Lookup non-associated policy
 	get.Names = []string{policy2.Name}
 	resp = structs.ACLPolicySetResponse{}
-	if err := msgpackrpc.CallWithCodec(codec, "ACL.GetPolicies", get, &resp); err == nil {
-		t.Fatalf("expected error")
-	}
+	must.Error(t, msgpackrpc.CallWithCodec(codec, "ACL.GetPolicies", get, &resp))
 
 	// Generate and upsert an ACL role which links to the previously created
 	// policy.
@@ -352,6 +348,27 @@ func TestACLEndpoint_GetPolicies_TokenSubset(t *testing.T) {
 	must.NoError(t, msgpackrpc.CallWithCodec(codec, "ACL.GetPolicies", req2, &resp2))
 	must.Eq(t, 1000, resp2.Index)
 	must.Eq(t, 2, len(resp2.Policies))
+
+	// Delete one of the policies, which means the ACL token has a dangling
+	// policy. When a Nomad client perform an ACL lookup, it adds the policies
+	// attached to the token within the request arguments. This test section
+	// mimics the behaviour when a token is being used that contains dangling
+	// policies.
+	must.NoError(t, s1.fsm.State().DeleteACLPolicies(structs.MsgTypeTestSetup, 1040, []string{policy.Name}))
+
+	req3 := &structs.ACLPolicySetRequest{
+		Names: []string{policy.Name, policy2.Name},
+		QueryOptions: structs.QueryOptions{
+			Region:    "global",
+			AuthToken: mockTokenWithRolePolicy.SecretID,
+		},
+	}
+
+	var resp3 structs.ACLPolicySetResponse
+	must.NoError(t, msgpackrpc.CallWithCodec(codec, "ACL.GetPolicies", req3, &resp3))
+	must.Eq(t, 1040, resp3.Index)
+	must.MapLen(t, 1, resp3.Policies)
+	must.MapContainsKey(t, resp3.Policies, policy2.Name)
 }
 
 func TestACLEndpoint_GetPolicies_Blocking(t *testing.T) {