diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 774716b7c8..31dba80174 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -1420,11 +1420,14 @@ func (f *aclFilter) filterCoordinates(coords *structs.Coordinates) { // filterIntentions is used to filter intentions based on ACL rules. // We prune entries the user doesn't have access to, and we redact any tokens -// if the user doesn't have a management token. -func (f *aclFilter) filterIntentions(ixns *structs.Intentions) { +// if the user doesn't have a management token. Returns true if any elements +// were removed. +func (f *aclFilter) filterIntentions(ixns *structs.Intentions) bool { ret := make(structs.Intentions, 0, len(*ixns)) + var removed bool for _, ixn := range *ixns { if !ixn.CanRead(f.authorizer) { + removed = true f.logger.Debug("dropping intention from result due to ACLs", "intention", ixn.ID) continue } @@ -1433,6 +1436,7 @@ func (f *aclFilter) filterIntentions(ixns *structs.Intentions) { } *ixns = ret + return removed } // filterNodeDump is used to filter through all parts of a node dump and @@ -1824,7 +1828,7 @@ func filterACLWithAuthorizer(logger hclog.Logger, authorizer acl.Authorizer, sub v.QueryMeta.ResultsFilteredByACLs = filt.filterHealthChecks(&v.HealthChecks) case *structs.IndexedIntentions: - filt.filterIntentions(&v.Intentions) + v.QueryMeta.ResultsFilteredByACLs = filt.filterIntentions(&v.Intentions) case *structs.IndexedNodeDump: filt.filterNodeDump(&v.Dump) diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index 090e024e79..f0b18cf034 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -2243,54 +2243,63 @@ func TestACL_filterHealthChecks(t *testing.T) { func TestACL_filterIntentions(t *testing.T) { t.Parallel() - assert := assert.New(t) - fill := func() structs.Intentions { - return structs.Intentions{ - &structs.Intention{ - ID: "f004177f-2c28-83b7-4229-eacc25fe55d1", - DestinationName: "bar", - }, - &structs.Intention{ - ID: "f004177f-2c28-83b7-4229-eacc25fe55d2", - DestinationName: "foo", + logger := hclog.NewNullLogger() + + makeList := func() *structs.IndexedIntentions { + return &structs.IndexedIntentions{ + Intentions: structs.Intentions{ + &structs.Intention{ + ID: "f004177f-2c28-83b7-4229-eacc25fe55d1", + DestinationName: "bar", + }, + &structs.Intention{ + ID: "f004177f-2c28-83b7-4229-eacc25fe55d2", + DestinationName: "foo", + }, }, } } - // Try permissive filtering. - { - ixns := fill() - filt := newACLFilter(acl.AllowAll(), nil) - filt.filterIntentions(&ixns) - assert.Len(ixns, 2) - } + t.Run("allowed", func(t *testing.T) { + require := require.New(t) - // Try restrictive filtering. - { - ixns := fill() - filt := newACLFilter(acl.DenyAll(), nil) - filt.filterIntentions(&ixns) - assert.Len(ixns, 0) - } + list := makeList() + filterACLWithAuthorizer(logger, acl.AllowAll(), list) - // Policy to see one - policy, err := acl.NewPolicyFromSource(` -service "foo" { - policy = "read" -} -`, acl.SyntaxLegacy, nil, nil) - assert.Nil(err) - perms, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - assert.Nil(err) + require.Len(list.Intentions, 2) + require.False(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + }) - // Filter - { - ixns := fill() - filt := newACLFilter(perms, nil) - filt.filterIntentions(&ixns) - assert.Len(ixns, 1) - } + t.Run("allowed to read 1", func(t *testing.T) { + require := require.New(t) + + policy, err := acl.NewPolicyFromSource(` + service "foo" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) + + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) + + list := makeList() + filterACLWithAuthorizer(logger, authz, list) + + require.Len(list.Intentions, 1) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) + + t.Run("denied", func(t *testing.T) { + require := require.New(t) + + list := makeList() + filterACLWithAuthorizer(logger, acl.DenyAll(), list) + + require.Empty(list.Intentions) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) } func TestACL_filterServices(t *testing.T) { diff --git a/agent/consul/intention_endpoint.go b/agent/consul/intention_endpoint.go index 4f6da3b9bb..862637de5a 100644 --- a/agent/consul/intention_endpoint.go +++ b/agent/consul/intention_endpoint.go @@ -550,17 +550,19 @@ func (s *Intention) List(args *structs.IntentionListRequest, reply *structs.Inde } else { reply.DataOrigin = structs.IntentionDataOriginLegacy } - - if err := s.srv.filterACL(args.Token, reply); err != nil { - return err - } - raw, err := filter.Execute(reply.Intentions) if err != nil { return err } reply.Intentions = raw.(structs.Intentions) + // Note: we filter the results with ACLs *after* applying the user-supplied + // bexpr filter, to ensure QueryMeta.ResultsFilteredByACLs does not include + // results that would be filtered out even if the user did have permission. + if err := s.srv.filterACL(args.Token, reply); err != nil { + return err + } + return nil }, ) diff --git a/agent/consul/intention_endpoint_test.go b/agent/consul/intention_endpoint_test.go index 59f450ff90..ec658fddba 100644 --- a/agent/consul/intention_endpoint_test.go +++ b/agent/consul/intention_endpoint_test.go @@ -1635,6 +1635,7 @@ func TestIntentionList_acl(t *testing.T) { var resp structs.IndexedIntentions require.NoError(t, msgpackrpc.CallWithCodec(codec, "Intention.List", req, &resp)) require.Len(t, resp.Intentions, 0) + require.False(t, resp.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") }) // Test with management token @@ -1646,6 +1647,7 @@ func TestIntentionList_acl(t *testing.T) { var resp structs.IndexedIntentions require.NoError(t, msgpackrpc.CallWithCodec(codec, "Intention.List", req, &resp)) require.Len(t, resp.Intentions, 3) + require.False(t, resp.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") }) // Test with user token @@ -1657,6 +1659,7 @@ func TestIntentionList_acl(t *testing.T) { var resp structs.IndexedIntentions require.NoError(t, msgpackrpc.CallWithCodec(codec, "Intention.List", req, &resp)) require.Len(t, resp.Intentions, 1) + require.True(t, resp.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") }) t.Run("filtered", func(t *testing.T) { @@ -1671,6 +1674,7 @@ func TestIntentionList_acl(t *testing.T) { var resp structs.IndexedIntentions require.NoError(t, msgpackrpc.CallWithCodec(codec, "Intention.List", req, &resp)) require.Len(t, resp.Intentions, 1) + require.False(t, resp.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") }) }