diff --git a/.changelog/18972.txt b/.changelog/18972.txt new file mode 100644 index 000000000..8a8387366 --- /dev/null +++ b/.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 +``` diff --git a/nomad/acl_endpoint.go b/nomad/acl_endpoint.go index 6a61a2cd4..e246b5083 100644 --- a/nomad/acl_endpoint.go +++ b/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 diff --git a/nomad/acl_endpoint_test.go b/nomad/acl_endpoint_test.go index 1db4c10ad..72fd3bbae 100644 --- a/nomad/acl_endpoint_test.go +++ b/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) {