From 6d0893cf57a7421fb3673660b6465bef41b00f12 Mon Sep 17 00:00:00 2001 From: James Rasell Date: Thu, 2 Nov 2023 15:23:42 +0000 Subject: [PATCH] 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 | 3 +++ nomad/acl_endpoint.go | 23 +++++++++++++--------- nomad/acl_endpoint_test.go | 39 +++++++++++++++++++++++++++----------- 3 files changed, 45 insertions(+), 20 deletions(-) create mode 100644 .changelog/18972.txt 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) {