diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 685159071..774716b7c 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -1219,10 +1219,12 @@ func (f *aclFilter) allowSession(node string, ent *acl.AuthorizerContext) bool { } // filterHealthChecks is used to filter a set of health checks down based on -// the configured ACL rules for a token. -func (f *aclFilter) filterHealthChecks(checks *structs.HealthChecks) { +// the configured ACL rules for a token. Returns true if any elements were +// removed. +func (f *aclFilter) filterHealthChecks(checks *structs.HealthChecks) bool { hc := *checks var authzContext acl.AuthorizerContext + var removed bool for i := 0; i < len(hc); i++ { check := hc[i] @@ -1232,10 +1234,12 @@ func (f *aclFilter) filterHealthChecks(checks *structs.HealthChecks) { } f.logger.Debug("dropping check from result due to ACLs", "check", check.CheckID) + removed = true hc = append(hc[:i], hc[i+1:]...) i-- } *checks = hc + return removed } // filterServices is used to filter a set of services based on ACLs. @@ -1332,10 +1336,12 @@ func (f *aclFilter) filterNodeServiceList(services **structs.NodeServiceList) { } } -// filterCheckServiceNodes is used to filter nodes based on ACL rules. -func (f *aclFilter) filterCheckServiceNodes(nodes *structs.CheckServiceNodes) { +// filterCheckServiceNodes is used to filter nodes based on ACL rules. Returns +// true if any elements were removed. +func (f *aclFilter) filterCheckServiceNodes(nodes *structs.CheckServiceNodes) bool { csn := *nodes var authzContext acl.AuthorizerContext + var removed bool for i := 0; i < len(csn); i++ { node := csn[i] @@ -1344,22 +1350,20 @@ func (f *aclFilter) filterCheckServiceNodes(nodes *structs.CheckServiceNodes) { continue } f.logger.Debug("dropping node from result due to ACLs", "node", structs.NodeNameString(node.Node.Node, node.Node.GetEnterpriseMeta())) + removed = true csn = append(csn[:i], csn[i+1:]...) i-- } *nodes = csn + return removed } // filterServiceTopology is used to filter upstreams/downstreams based on ACL rules. // this filter is unlike others in that it also returns whether the result was filtered by ACLs func (f *aclFilter) filterServiceTopology(topology *structs.ServiceTopology) bool { - numUp := len(topology.Upstreams) - numDown := len(topology.Downstreams) - - f.filterCheckServiceNodes(&topology.Upstreams) - f.filterCheckServiceNodes(&topology.Downstreams) - - return numUp != len(topology.Upstreams) || numDown != len(topology.Downstreams) + filteredUpstreams := f.filterCheckServiceNodes(&topology.Upstreams) + filteredDownstreams := f.filterCheckServiceNodes(&topology.Downstreams) + return filteredUpstreams || filteredDownstreams } // filterDatacenterCheckServiceNodes is used to filter nodes based on ACL rules. @@ -1801,7 +1805,7 @@ func filterACLWithAuthorizer(logger hclog.Logger, authorizer acl.Authorizer, sub filt.filterCheckServiceNodes(v) case *structs.IndexedCheckServiceNodes: - filt.filterCheckServiceNodes(&v.Nodes) + v.QueryMeta.ResultsFilteredByACLs = filt.filterCheckServiceNodes(&v.Nodes) case *structs.IndexedServiceTopology: filtered := filt.filterServiceTopology(v.ServiceTopology) @@ -1817,7 +1821,7 @@ func filterACLWithAuthorizer(logger hclog.Logger, authorizer acl.Authorizer, sub filt.filterCoordinates(&v.Coordinates) case *structs.IndexedHealthChecks: - filt.filterHealthChecks(&v.HealthChecks) + v.QueryMeta.ResultsFilteredByACLs = filt.filterHealthChecks(&v.HealthChecks) case *structs.IndexedIntentions: filt.filterIntentions(&v.Intentions) diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index 61b4df204..090e024e7 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-uuid" msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" "github.com/mitchellh/copystructure" @@ -2151,72 +2152,93 @@ func testACLResolver_variousTokens(t *testing.T, delegate *ACLResolverTestDelega func TestACL_filterHealthChecks(t *testing.T) { t.Parallel() - // Create some health checks. - fill := func() structs.HealthChecks { - return structs.HealthChecks{ - &structs.HealthCheck{ - Node: "node1", - CheckID: "check1", - ServiceName: "foo", + + logger := hclog.NewNullLogger() + + makeList := func() *structs.IndexedHealthChecks { + return &structs.IndexedHealthChecks{ + HealthChecks: structs.HealthChecks{ + { + Node: "node1", + CheckID: "check1", + ServiceName: "foo", + }, }, } } - { - hc := fill() - filt := newACLFilter(acl.DenyAll(), nil) - filt.filterHealthChecks(&hc) - if len(hc) != 0 { - t.Fatalf("bad: %#v", hc) - } - } + t.Run("allowed", func(t *testing.T) { + require := require.New(t) - // Allowed to see the service but not the node. - policy, err := acl.NewPolicyFromSource(` -service "foo" { - policy = "read" -} -`, acl.SyntaxLegacy, nil, nil) - if err != nil { - t.Fatalf("err %v", err) - } - perms, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - if err != nil { - t.Fatalf("err: %v", err) - } + policy, err := acl.NewPolicyFromSource(` + service "foo" { + policy = "read" + } + node "node1" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) - { - hc := fill() - filt := newACLFilter(perms, nil) - filt.filterHealthChecks(&hc) - if len(hc) != 0 { - t.Fatalf("bad: %#v", hc) - } - } + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) - // Chain on access to the node. - policy, err = acl.NewPolicyFromSource(` -node "node1" { - policy = "read" -} -`, acl.SyntaxLegacy, nil, nil) - if err != nil { - t.Fatalf("err %v", err) - } - perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil) - if err != nil { - t.Fatalf("err: %v", err) - } + list := makeList() + filterACLWithAuthorizer(logger, authz, list) - // Now it should go through. - { - hc := fill() - filt := newACLFilter(perms, nil) - filt.filterHealthChecks(&hc) - if len(hc) != 1 { - t.Fatalf("bad: %#v", hc) - } - } + require.Len(list.HealthChecks, 1) + require.False(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + }) + + t.Run("allowed to read the service, but not the node", 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.Empty(list.HealthChecks) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) + + t.Run("allowed to read the node, but not the service", func(t *testing.T) { + require := require.New(t) + + policy, err := acl.NewPolicyFromSource(` + node "node1" { + 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.Empty(list.HealthChecks) + 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.HealthChecks) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) } func TestACL_filterIntentions(t *testing.T) { @@ -2474,100 +2496,104 @@ node "node1" { func TestACL_filterCheckServiceNodes(t *testing.T) { t.Parallel() - // Create some nodes. - fill := func() structs.CheckServiceNodes { - return structs.CheckServiceNodes{ - structs.CheckServiceNode{ - Node: &structs.Node{ - Node: "node1", - }, - Service: &structs.NodeService{ - ID: "foo", - Service: "foo", - }, - Checks: structs.HealthChecks{ - &structs.HealthCheck{ - Node: "node1", - CheckID: "check1", - ServiceName: "foo", + + logger := hclog.NewNullLogger() + + makeList := func() *structs.IndexedCheckServiceNodes { + return &structs.IndexedCheckServiceNodes{ + Nodes: structs.CheckServiceNodes{ + { + Node: &structs.Node{ + Node: "node1", + }, + Service: &structs.NodeService{ + ID: "foo", + Service: "foo", + }, + Checks: structs.HealthChecks{ + { + Node: "node1", + CheckID: "check1", + ServiceName: "foo", + }, }, }, }, } } - // Try permissive filtering. - { - nodes := fill() - filt := newACLFilter(acl.AllowAll(), nil) - filt.filterCheckServiceNodes(&nodes) - if len(nodes) != 1 { - t.Fatalf("bad: %#v", nodes) - } - if len(nodes[0].Checks) != 1 { - t.Fatalf("bad: %#v", nodes[0].Checks) - } - } + t.Run("allowed", func(t *testing.T) { + require := require.New(t) - // Try restrictive filtering. - { - nodes := fill() - filt := newACLFilter(acl.DenyAll(), nil) - filt.filterCheckServiceNodes(&nodes) - if len(nodes) != 0 { - t.Fatalf("bad: %#v", nodes) - } - } + policy, err := acl.NewPolicyFromSource(` + service "foo" { + policy = "read" + } + node "node1" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) - // Allowed to see the service but not the node. - policy, err := acl.NewPolicyFromSource(` -service "foo" { - policy = "read" -} -`, acl.SyntaxLegacy, nil, nil) - if err != nil { - t.Fatalf("err %v", err) - } - perms, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - if err != nil { - t.Fatalf("err: %v", err) - } + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) - { - nodes := fill() - filt := newACLFilter(perms, nil) - filt.filterCheckServiceNodes(&nodes) - if len(nodes) != 0 { - t.Fatalf("bad: %#v", nodes) - } - } + list := makeList() + filterACLWithAuthorizer(logger, authz, list) - // Chain on access to the node. - policy, err = acl.NewPolicyFromSource(` -node "node1" { - policy = "read" -} -`, acl.SyntaxLegacy, nil, nil) - if err != nil { - t.Fatalf("err %v", err) - } - perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil) - if err != nil { - t.Fatalf("err: %v", err) - } + require.Len(list.Nodes, 1) + require.False(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + }) - // Now it should go through. - { - nodes := fill() - filt := newACLFilter(perms, nil) - filt.filterCheckServiceNodes(&nodes) - if len(nodes) != 1 { - t.Fatalf("bad: %#v", nodes) - } - if len(nodes[0].Checks) != 1 { - t.Fatalf("bad: %#v", nodes[0].Checks) - } - } + t.Run("allowed to read the service, but not the node", 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.Empty(list.Nodes) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) + + t.Run("allowed to read the node, but not the service", func(t *testing.T) { + require := require.New(t) + + policy, err := acl.NewPolicyFromSource(` + node "node1" { + 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.Empty(list.Nodes) + 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.Nodes) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) } func TestACL_filterServiceTopology(t *testing.T) { diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go index c1b2a4d64..b8b4fe256 100644 --- a/agent/consul/catalog_endpoint_test.go +++ b/agent/consul/catalog_endpoint_test.go @@ -2742,6 +2742,7 @@ node_prefix "" { CheckID: "service:bar", Name: "service:bar", ServiceID: "bar", + Status: api.HealthPassing, }, WriteRequest: structs.WriteRequest{Token: "root"}, } diff --git a/agent/consul/health_endpoint.go b/agent/consul/health_endpoint.go index 5ef0e9e3b..2fe95e6db 100644 --- a/agent/consul/health_endpoint.go +++ b/agent/consul/health_endpoint.go @@ -55,9 +55,6 @@ func (h *Health) ChecksInState(args *structs.ChecksInStateRequest, return err } reply.Index, reply.HealthChecks = index, checks - if err := h.srv.filterACL(args.Token, reply); err != nil { - return err - } raw, err := filter.Execute(reply.HealthChecks) if err != nil { @@ -65,6 +62,13 @@ func (h *Health) ChecksInState(args *structs.ChecksInStateRequest, } reply.HealthChecks = raw.(structs.HealthChecks) + // 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 := h.srv.filterACL(args.Token, reply); err != nil { + return err + } + return h.srv.sortNodesByDistanceFrom(args.Source, reply.HealthChecks) }) } @@ -99,15 +103,20 @@ func (h *Health) NodeChecks(args *structs.NodeSpecificRequest, return err } reply.Index, reply.HealthChecks = index, checks - if err := h.srv.filterACL(args.Token, reply); err != nil { - return err - } raw, err := filter.Execute(reply.HealthChecks) if err != nil { return err } reply.HealthChecks = raw.(structs.HealthChecks) + + // 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 := h.srv.filterACL(args.Token, reply); err != nil { + return err + } + return nil }) } @@ -156,9 +165,6 @@ func (h *Health) ServiceChecks(args *structs.ServiceSpecificRequest, return err } reply.Index, reply.HealthChecks = index, checks - if err := h.srv.filterACL(args.Token, reply); err != nil { - return err - } raw, err := filter.Execute(reply.HealthChecks) if err != nil { @@ -166,6 +172,13 @@ func (h *Health) ServiceChecks(args *structs.ServiceSpecificRequest, } reply.HealthChecks = raw.(structs.HealthChecks) + // 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 := h.srv.filterACL(args.Token, reply); err != nil { + return err + } + return h.srv.sortNodesByDistanceFrom(args.Source, reply.HealthChecks) }) } @@ -232,16 +245,19 @@ func (h *Health) ServiceNodes(args *structs.ServiceSpecificRequest, reply *struc reply.Nodes = nodeMetaFilter(args.NodeMetaFilters, reply.Nodes) } - if err := h.srv.filterACL(args.Token, reply); err != nil { - return err - } - raw, err := filter.Execute(reply.Nodes) if err != nil { return err } reply.Nodes = raw.(structs.CheckServiceNodes) + // 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 := h.srv.filterACL(args.Token, reply); err != nil { + return err + } + return h.srv.sortNodesByDistanceFrom(args.Source, reply.Nodes) }) diff --git a/agent/consul/health_endpoint_test.go b/agent/consul/health_endpoint_test.go index ac85c68d2..fe3e3ef6d 100644 --- a/agent/consul/health_endpoint_test.go +++ b/agent/consul/health_endpoint_test.go @@ -1431,6 +1431,9 @@ func TestHealth_NodeChecks_FilterACL(t *testing.T) { } t.Parallel() + + require := require.New(t) + dir, token, srv, codec := testACLFilterServer(t) defer os.RemoveAll(dir) defer srv.Shutdown() @@ -1442,9 +1445,9 @@ func TestHealth_NodeChecks_FilterACL(t *testing.T) { QueryOptions: structs.QueryOptions{Token: token}, } reply := structs.IndexedHealthChecks{} - if err := msgpackrpc.CallWithCodec(codec, "Health.NodeChecks", &opt, &reply); err != nil { - t.Fatalf("err: %s", err) - } + err := msgpackrpc.CallWithCodec(codec, "Health.NodeChecks", &opt, &reply) + require.NoError(err) + found := false for _, chk := range reply.HealthChecks { switch chk.ServiceName { @@ -1454,9 +1457,8 @@ func TestHealth_NodeChecks_FilterACL(t *testing.T) { t.Fatalf("bad: %#v", reply.HealthChecks) } } - if !found { - t.Fatalf("bad: %#v", reply.HealthChecks) - } + require.True(found, "bad: %#v", reply.HealthChecks) + require.True(reply.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") // We've already proven that we call the ACL filtering function so we // test node filtering down in acl.go for node cases. This also proves @@ -1471,6 +1473,9 @@ func TestHealth_ServiceChecks_FilterACL(t *testing.T) { } t.Parallel() + + require := require.New(t) + dir, token, srv, codec := testACLFilterServer(t) defer os.RemoveAll(dir) defer srv.Shutdown() @@ -1482,9 +1487,9 @@ func TestHealth_ServiceChecks_FilterACL(t *testing.T) { QueryOptions: structs.QueryOptions{Token: token}, } reply := structs.IndexedHealthChecks{} - if err := msgpackrpc.CallWithCodec(codec, "Health.ServiceChecks", &opt, &reply); err != nil { - t.Fatalf("err: %s", err) - } + err := msgpackrpc.CallWithCodec(codec, "Health.ServiceChecks", &opt, &reply) + require.NoError(err) + found := false for _, chk := range reply.HealthChecks { if chk.ServiceName == "foo" { @@ -1492,18 +1497,14 @@ func TestHealth_ServiceChecks_FilterACL(t *testing.T) { break } } - if !found { - t.Fatalf("bad: %#v", reply.HealthChecks) - } + require.True(found, "bad: %#v", reply.HealthChecks) opt.ServiceName = "bar" reply = structs.IndexedHealthChecks{} - if err := msgpackrpc.CallWithCodec(codec, "Health.ServiceChecks", &opt, &reply); err != nil { - t.Fatalf("err: %s", err) - } - if len(reply.HealthChecks) != 0 { - t.Fatalf("bad: %#v", reply.HealthChecks) - } + err = msgpackrpc.CallWithCodec(codec, "Health.ServiceChecks", &opt, &reply) + require.NoError(err) + require.Empty(reply.HealthChecks) + require.True(reply.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") // We've already proven that we call the ACL filtering function so we // test node filtering down in acl.go for node cases. This also proves @@ -1518,6 +1519,9 @@ func TestHealth_ServiceNodes_FilterACL(t *testing.T) { } t.Parallel() + + require := require.New(t) + dir, token, srv, codec := testACLFilterServer(t) defer os.RemoveAll(dir) defer srv.Shutdown() @@ -1529,21 +1533,16 @@ func TestHealth_ServiceNodes_FilterACL(t *testing.T) { QueryOptions: structs.QueryOptions{Token: token}, } reply := structs.IndexedCheckServiceNodes{} - if err := msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &opt, &reply); err != nil { - t.Fatalf("err: %s", err) - } - if len(reply.Nodes) != 1 { - t.Fatalf("bad: %#v", reply.Nodes) - } + err := msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &opt, &reply) + require.NoError(err) + require.Len(reply.Nodes, 1) opt.ServiceName = "bar" reply = structs.IndexedCheckServiceNodes{} - if err := msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &opt, &reply); err != nil { - t.Fatalf("err: %s", err) - } - if len(reply.Nodes) != 0 { - t.Fatalf("bad: %#v", reply.Nodes) - } + err = msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &opt, &reply) + require.NoError(err) + require.Empty(reply.Nodes) + require.True(reply.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") // We've already proven that we call the ACL filtering function so we // test node filtering down in acl.go for node cases. This also proves @@ -1558,6 +1557,9 @@ func TestHealth_ChecksInState_FilterACL(t *testing.T) { } t.Parallel() + + require := require.New(t) + dir, token, srv, codec := testACLFilterServer(t) defer os.RemoveAll(dir) defer srv.Shutdown() @@ -1569,9 +1571,8 @@ func TestHealth_ChecksInState_FilterACL(t *testing.T) { QueryOptions: structs.QueryOptions{Token: token}, } reply := structs.IndexedHealthChecks{} - if err := msgpackrpc.CallWithCodec(codec, "Health.ChecksInState", &opt, &reply); err != nil { - t.Fatalf("err: %s", err) - } + err := msgpackrpc.CallWithCodec(codec, "Health.ChecksInState", &opt, &reply) + require.NoError(err) found := false for _, chk := range reply.HealthChecks { @@ -1582,9 +1583,8 @@ func TestHealth_ChecksInState_FilterACL(t *testing.T) { t.Fatalf("bad service 'bar': %#v", reply.HealthChecks) } } - if !found { - t.Fatalf("missing service 'foo': %#v", reply.HealthChecks) - } + require.True(found, "missing service 'foo': %#v", reply.HealthChecks) + require.True(reply.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") // We've already proven that we call the ACL filtering function so we // test node filtering down in acl.go for node cases. This also proves