diff --git a/.changelog/21950.txt b/.changelog/21950.txt new file mode 100644 index 0000000000..e15f9d6c80 --- /dev/null +++ b/.changelog/21950.txt @@ -0,0 +1,3 @@ +```release-note:security +Removed ability to use bexpr to filter results without ACL read on endpoint +``` \ No newline at end of file diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 996212c97e..0a4402ffcb 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -380,16 +380,14 @@ func (s *HTTPHandlers) AgentServices(resp http.ResponseWriter, req *http.Request return nil, err } - raw, err := filter.Execute(agentSvcs) - if err != nil { - return nil, err - } - agentSvcs = raw.(map[string]*api.AgentService) - - // Note: we filter the results with ACLs *after* applying the user-supplied - // bexpr filter, to ensure total (and the filter-by-acls header we set below) - // do not include results that would be filtered out even if the user did have - // permission. + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. total := len(agentSvcs) if err := s.agent.filterServicesWithAuthorizer(authz, agentSvcs); err != nil { return nil, err @@ -407,6 +405,12 @@ func (s *HTTPHandlers) AgentServices(resp http.ResponseWriter, req *http.Request setResultsFilteredByACLs(resp, total != len(agentSvcs)) } + raw, err := filter.Execute(agentSvcs) + if err != nil { + return nil, err + } + agentSvcs = raw.(map[string]*api.AgentService) + return agentSvcs, nil } @@ -540,16 +544,14 @@ func (s *HTTPHandlers) AgentChecks(resp http.ResponseWriter, req *http.Request) } } - raw, err := filter.Execute(agentChecks) - if err != nil { - return nil, err - } - agentChecks = raw.(map[types.CheckID]*structs.HealthCheck) - - // Note: we filter the results with ACLs *after* applying the user-supplied - // bexpr filter, to ensure total (and the filter-by-acls header we set below) - // do not include results that would be filtered out even if the user did have - // permission. + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. total := len(agentChecks) if err := s.agent.filterChecksWithAuthorizer(authz, agentChecks); err != nil { return nil, err @@ -567,6 +569,12 @@ func (s *HTTPHandlers) AgentChecks(resp http.ResponseWriter, req *http.Request) setResultsFilteredByACLs(resp, total != len(agentChecks)) } + raw, err := filter.Execute(agentChecks) + if err != nil { + return nil, err + } + agentChecks = raw.(map[types.CheckID]*structs.HealthCheck) + return agentChecks, nil } @@ -623,21 +631,14 @@ func (s *HTTPHandlers) AgentMembers(resp http.ResponseWriter, req *http.Request) } } - // filter the members by parsed filter expression - var filterExpression string - s.parseFilter(req, &filterExpression) - if filterExpression != "" { - filter, err := bexpr.CreateFilter(filterExpression, nil, members) - if err != nil { - return nil, err - } - raw, err := filter.Execute(members) - if err != nil { - return nil, err - } - members = raw.([]serf.Member) - } - + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. total := len(members) if err := s.agent.filterMembers(token, &members); err != nil { return nil, err @@ -655,6 +656,21 @@ func (s *HTTPHandlers) AgentMembers(resp http.ResponseWriter, req *http.Request) setResultsFilteredByACLs(resp, total != len(members)) } + // filter the members by parsed filter expression + var filterExpression string + s.parseFilter(req, &filterExpression) + if filterExpression != "" { + filter, err := bexpr.CreateFilter(filterExpression, nil, members) + if err != nil { + return nil, err + } + raw, err := filter.Execute(members) + if err != nil { + return nil, err + } + members = raw.([]serf.Member) + } + return members, nil } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 69551d7c36..451c412b4b 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -433,6 +433,60 @@ func TestAgent_Services_ACLFilter(t *testing.T) { require.Len(t, val, 2) require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) }) + + // ensure ACL filtering occurs before bexpr filtering. + const bexprMatchingUserTokenPermissions = "Service matches `web.*`" + const bexprNotMatchingUserTokenPermissions = "Service matches `api.*`" + + tokenWithWebRead := testCreateToken(t, a, ` + service "web" { + policy = "read" + } + `) + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req, _ := http.NewRequest("GET", "/v1/agent/services?filter="+url.QueryEscape(bexprMatchingUserTokenPermissions), nil) + req.Header.Add("X-Consul-Token", tokenWithWebRead) + resp := httptest.NewRecorder() + a.srv.h.ServeHTTP(resp, req) + dec := json.NewDecoder(resp.Body) + var val map[string]*api.AgentService + err := dec.Decode(&val) + if err != nil { + t.Fatalf("Err: %v", err) + } + require.Len(t, val, 1) + require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req, _ := http.NewRequest("GET", "/v1/agent/services?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil) + req.Header.Add("X-Consul-Token", tokenWithWebRead) + resp := httptest.NewRecorder() + a.srv.h.ServeHTTP(resp, req) + dec := json.NewDecoder(resp.Body) + var val map[string]*api.AgentService + err := dec.Decode(&val) + if err != nil { + t.Fatalf("Err: %v", err) + } + require.Len(t, val, 0) + require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + req, _ := http.NewRequest("GET", "/v1/agent/services?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil) + resp := httptest.NewRecorder() + a.srv.h.ServeHTTP(resp, req) + dec := json.NewDecoder(resp.Body) + var val map[string]*api.AgentService + err := dec.Decode(&val) + if err != nil { + t.Fatalf("Err: %v", err) + } + require.Len(t, val, 0) + require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) + }) } func TestAgent_Service(t *testing.T) { @@ -1432,6 +1486,57 @@ func TestAgent_Checks_ACLFilter(t *testing.T) { require.Len(t, val, 2) require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) }) + + // ensure ACL filtering occurs before bexpr filtering. + const bexprMatchingUserTokenPermissions = "ServiceName matches `web.*`" + const bexprNotMatchingUserTokenPermissions = "ServiceName matches `api.*`" + + tokenWithWebRead := testCreateToken(t, a, ` + service "web" { + policy = "read" + } + `) + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req, _ := http.NewRequest("GET", "/v1/agent/checks?filter="+url.QueryEscape(bexprMatchingUserTokenPermissions), nil) + req.Header.Add("X-Consul-Token", tokenWithWebRead) + resp := httptest.NewRecorder() + a.srv.h.ServeHTTP(resp, req) + dec := json.NewDecoder(resp.Body) + val := make(map[types.CheckID]*structs.HealthCheck) + if err := dec.Decode(&val); err != nil { + t.Fatalf("Err: %v", err) + } + require.Len(t, val, 1) + require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req, _ := http.NewRequest("GET", "/v1/agent/checks?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil) + req.Header.Add("X-Consul-Token", tokenWithWebRead) + resp := httptest.NewRecorder() + a.srv.h.ServeHTTP(resp, req) + dec := json.NewDecoder(resp.Body) + val := make(map[types.CheckID]*structs.HealthCheck) + if err := dec.Decode(&val); err != nil { + t.Fatalf("Err: %v", err) + } + require.Len(t, val, 0) + require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + req, _ := http.NewRequest("GET", "/v1/agent/checks?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil) + resp := httptest.NewRecorder() + a.srv.h.ServeHTTP(resp, req) + dec := json.NewDecoder(resp.Body) + val := make(map[types.CheckID]*structs.HealthCheck) + if err := dec.Decode(&val); err != nil { + t.Fatalf("Err: %v", err) + } + require.Len(t, val, 0) + require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) + }) } func TestAgent_Self(t *testing.T) { @@ -2110,6 +2215,57 @@ func TestAgent_Members_ACLFilter(t *testing.T) { require.Len(t, val, 2) require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) }) + + // ensure ACL filtering occurs before bexpr filtering. + bexprMatchingUserTokenPermissions := fmt.Sprintf("Name matches `%s.*`", b.Config.NodeName) + bexprNotMatchingUserTokenPermissions := fmt.Sprintf("Name matches `%s.*`", a.Config.NodeName) + + tokenWithReadOnMemberB := testCreateToken(t, a, fmt.Sprintf(` + node "%s" { + policy = "read" + } + `, b.Config.NodeName)) + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req, _ := http.NewRequest("GET", "/v1/agent/members?filter="+url.QueryEscape(bexprMatchingUserTokenPermissions), nil) + req.Header.Add("X-Consul-Token", tokenWithReadOnMemberB) + resp := httptest.NewRecorder() + a.srv.h.ServeHTTP(resp, req) + dec := json.NewDecoder(resp.Body) + val := make([]serf.Member, 0) + if err := dec.Decode(&val); err != nil { + t.Fatalf("Err: %v", err) + } + require.Len(t, val, 1) + require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req, _ := http.NewRequest("GET", "/v1/agent/members?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil) + req.Header.Add("X-Consul-Token", tokenWithReadOnMemberB) + resp := httptest.NewRecorder() + a.srv.h.ServeHTTP(resp, req) + dec := json.NewDecoder(resp.Body) + val := make([]serf.Member, 0) + if err := dec.Decode(&val); err != nil { + t.Fatalf("Err: %v", err) + } + require.Len(t, val, 0) + require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + req, _ := http.NewRequest("GET", "/v1/agent/members?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil) + resp := httptest.NewRecorder() + a.srv.h.ServeHTTP(resp, req) + dec := json.NewDecoder(resp.Body) + val := make([]serf.Member, 0) + if err := dec.Decode(&val); err != nil { + t.Fatalf("Err: %v", err) + } + require.Len(t, val, 0) + require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) + }) } func TestAgent_Join(t *testing.T) { diff --git a/agent/catalog_endpoint_test.go b/agent/catalog_endpoint_test.go index c06efc748c..af1f2d36e3 100644 --- a/agent/catalog_endpoint_test.go +++ b/agent/catalog_endpoint_test.go @@ -122,7 +122,7 @@ func TestCatalogDeregister(t *testing.T) { a := NewTestAgent(t, "") defer a.Shutdown() - // Register node + // Deregister node args := &structs.DeregisterRequest{Node: "foo"} req, _ := http.NewRequest("PUT", "/v1/catalog/deregister", jsonReader(args)) obj, err := a.srv.CatalogDeregister(nil, req) diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index 36426afe25..09c23d90c5 100644 --- a/agent/consul/catalog_endpoint.go +++ b/agent/consul/catalog_endpoint.go @@ -533,18 +533,23 @@ func (c *Catalog) ListNodes(args *structs.DCSpecificRequest, reply *structs.Inde return nil } - raw, err := filter.Execute(reply.Nodes) - if err != nil { + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. + if err := c.srv.filterACL(args.Token, reply); err != nil { return err } - reply.Nodes = raw.(structs.Nodes) - // 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 := c.srv.filterACL(args.Token, reply); err != nil { + raw, err := filter.Execute(reply.Nodes) + if err != nil { return err } + reply.Nodes = raw.(structs.Nodes) return c.srv.sortNodesByDistanceFrom(args.Source, reply.Nodes) }) @@ -607,14 +612,25 @@ func (c *Catalog) ListServices(args *structs.DCSpecificRequest, reply *structs.I return nil } - raw, err := filter.Execute(serviceNodes) + // need to temporarily create an IndexedServiceNode so that the ACL filter can be applied + // to the service nodes and then re-use those same node to run the filter expression. + idxServiceNodeReply := &structs.IndexedServiceNodes{ + ServiceNodes: serviceNodes, + QueryMeta: reply.QueryMeta, + } + + // enforce ACLs + c.srv.filterACLWithAuthorizer(authz, idxServiceNodeReply) + + // run the filter expression + raw, err := filter.Execute(idxServiceNodeReply.ServiceNodes) if err != nil { return err } + // convert the result back to the original type reply.Services = servicesTagsByName(raw.(structs.ServiceNodes)) - - c.srv.filterACLWithAuthorizer(authz, reply) + reply.QueryMeta = idxServiceNodeReply.QueryMeta return nil }) @@ -813,6 +829,18 @@ func (c *Catalog) ServiceNodes(args *structs.ServiceSpecificRequest, reply *stru reply.ServiceNodes = filtered } + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. + if err := c.srv.filterACL(args.Token, reply); err != nil { + return err + } + // This is safe to do even when the filter is nil - its just a no-op then raw, err := filter.Execute(reply.ServiceNodes) if err != nil { @@ -820,13 +848,6 @@ func (c *Catalog) ServiceNodes(args *structs.ServiceSpecificRequest, reply *stru } reply.ServiceNodes = raw.(structs.ServiceNodes) - // 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 := c.srv.filterACL(args.Token, reply); err != nil { - return err - } - return c.srv.sortNodesByDistanceFrom(args.Source, reply.ServiceNodes) }) @@ -904,6 +925,18 @@ func (c *Catalog) NodeServices(args *structs.NodeSpecificRequest, reply *structs } reply.Index, reply.NodeServices = index, services + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. + if err := c.srv.filterACL(args.Token, reply); err != nil { + return err + } + if reply.NodeServices != nil { raw, err := filter.Execute(reply.NodeServices.Services) if err != nil { @@ -912,13 +945,6 @@ func (c *Catalog) NodeServices(args *structs.NodeSpecificRequest, reply *structs reply.NodeServices.Services = raw.(map[string]*structs.NodeService) } - // 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 := c.srv.filterACL(args.Token, reply); err != nil { - return err - } - return nil }) } @@ -1009,21 +1035,26 @@ func (c *Catalog) NodeServiceList(args *structs.NodeSpecificRequest, reply *stru if mergedServices != nil { reply.NodeServices = *mergedServices - - raw, err := filter.Execute(reply.NodeServices.Services) - if err != nil { - return err - } - reply.NodeServices.Services = raw.([]*structs.NodeService) } - // 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. + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. if err := c.srv.filterACL(args.Token, reply); err != nil { return err } + raw, err := filter.Execute(reply.NodeServices.Services) + if err != nil { + return err + } + reply.NodeServices.Services = raw.([]*structs.NodeService) + return nil }) } diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go index 18827dc981..1f3311b0cd 100644 --- a/agent/consul/catalog_endpoint_test.go +++ b/agent/consul/catalog_endpoint_test.go @@ -984,26 +984,27 @@ func TestCatalog_RPC_Filter(t *testing.T) { require.Equal(t, "baz", out.Nodes[0].Node) }) - t.Run("ServiceNodes", func(t *testing.T) { + t.Run("ListServices", func(t *testing.T) { args := structs.ServiceSpecificRequest{ Datacenter: "dc1", ServiceName: "redis", QueryOptions: structs.QueryOptions{Filter: "ServiceMeta.version == 1"}, } - out := new(structs.IndexedServiceNodes) - require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &args, &out)) - require.Len(t, out.ServiceNodes, 2) - require.Condition(t, func() bool { - return (out.ServiceNodes[0].Node == "foo" && out.ServiceNodes[1].Node == "bar") || - (out.ServiceNodes[0].Node == "bar" && out.ServiceNodes[1].Node == "foo") - }) + out := new(structs.IndexedServices) + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &args, &out)) + require.Len(t, out.Services, 2) + require.Len(t, out.Services["redis"], 1) + require.Len(t, out.Services["web"], 2) args.Filter = "ServiceMeta.version == 2" - out = new(structs.IndexedServiceNodes) - require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &args, &out)) - require.Len(t, out.ServiceNodes, 1) - require.Equal(t, "foo", out.ServiceNodes[0].Node) + out = new(structs.IndexedServices) + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &args, &out)) + require.Len(t, out.Services, 4) + require.Len(t, out.Services["redis"], 1) + require.Len(t, out.Services["web"], 2) + require.Len(t, out.Services["critical"], 1) + require.Len(t, out.Services["warning"], 1) }) t.Run("NodeServices", func(t *testing.T) { @@ -1022,6 +1023,46 @@ func TestCatalog_RPC_Filter(t *testing.T) { require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &out)) require.Len(t, out.NodeServices.Services, 1) }) + + t.Run("NodeServiceList", func(t *testing.T) { + args := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: "baz", + QueryOptions: structs.QueryOptions{Filter: "Service == web"}, + } + + out := new(structs.IndexedNodeServiceList) + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.NodeServiceList", &args, &out)) + require.Len(t, out.NodeServices.Services, 2) + + args.Filter = "Service == web and Meta.version == 2" + out = new(structs.IndexedNodeServiceList) + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.NodeServiceList", &args, &out)) + require.Len(t, out.NodeServices.Services, 1) + }) + + t.Run("ServiceNodes", func(t *testing.T) { + args := structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "redis", + QueryOptions: structs.QueryOptions{Filter: "ServiceMeta.version == 1"}, + } + + out := new(structs.IndexedServiceNodes) + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &args, &out)) + require.Len(t, out.ServiceNodes, 2) + require.Condition(t, func() bool { + return (out.ServiceNodes[0].Node == "foo" && out.ServiceNodes[1].Node == "bar") || + (out.ServiceNodes[0].Node == "bar" && out.ServiceNodes[1].Node == "foo") + }) + + args.Filter = "ServiceMeta.version == 2" + out = new(structs.IndexedServiceNodes) + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &args, &out)) + require.Len(t, out.ServiceNodes, 1) + require.Equal(t, "foo", out.ServiceNodes[0].Node) + }) + } func TestCatalog_ListNodes_StaleRead(t *testing.T) { @@ -1332,6 +1373,7 @@ func TestCatalog_ListNodes_ACLFilter(t *testing.T) { Datacenter: "dc1", } + readToken := token("read") t.Run("deny", func(t *testing.T) { args.Token = token("deny") @@ -1348,7 +1390,7 @@ func TestCatalog_ListNodes_ACLFilter(t *testing.T) { }) t.Run("allow", func(t *testing.T) { - args.Token = token("read") + args.Token = readToken var reply structs.IndexedNodes if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply); err != nil { @@ -1361,6 +1403,67 @@ func TestCatalog_ListNodes_ACLFilter(t *testing.T) { t.Fatal("ResultsFilteredByACLs should not true") } }) + + // Register additional node + regArgs := &structs.RegisterRequest{ + Datacenter: "dc1", + Node: "foo", + Address: "127.0.0.1", + WriteRequest: structs.WriteRequest{ + Token: "root", + }, + } + + var out struct{} + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.Register", regArgs, &out)) + + bexprMatchingUserTokenPermissions := fmt.Sprintf("Node matches `%s.*`", s1.config.NodeName) + const bexpNotMatchingUserTokenPermissions = "Node matches `node-deny.*`" + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + var reply structs.IndexedNodes + args = structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: readToken, + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedNodes{} + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply)) + require.Equal(t, 1, len(reply.Nodes)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + var reply structs.IndexedNodes + args = structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: readToken, + Filter: bexpNotMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedNodes{} + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply)) + require.Empty(t, reply.Nodes) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + var reply structs.IndexedNodes + args = structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: "", // no token + Filter: bexpNotMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedNodes{} + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply)) + require.Empty(t, reply.Nodes) + require.False(t, reply.ResultsFilteredByACLs) + }) } func Benchmark_Catalog_ListNodes(t *testing.B) { @@ -2758,6 +2861,14 @@ service "foo" { node_prefix "" { policy = "read" } + +node "node-deny" { + policy = "deny" +} + +service "service-deny" { + policy = "deny" +} ` token = createToken(t, codec, rules) @@ -2915,23 +3026,76 @@ func TestCatalog_ListServices_FilterACL(t *testing.T) { defer codec.Close() testrpc.WaitForTestAgent(t, srv.RPC, "dc1", testrpc.WithToken("root")) - opt := structs.DCSpecificRequest{ - Datacenter: "dc1", - QueryOptions: structs.QueryOptions{Token: token}, - } - reply := structs.IndexedServices{} - if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &opt, &reply); err != nil { - t.Fatalf("err: %s", err) - } - if _, ok := reply.Services["foo"]; !ok { - t.Fatalf("bad: %#v", reply.Services) - } - if _, ok := reply.Services["bar"]; ok { - t.Fatalf("bad: %#v", reply.Services) - } - if !reply.QueryMeta.ResultsFilteredByACLs { - t.Fatal("ResultsFilteredByACLs should be true") - } + t.Run("request with user token without filter param sets ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{Token: token}, + } + reply := structs.IndexedServices{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + if _, ok := reply.Services["foo"]; !ok { + t.Fatalf("bad: %#v", reply.Services) + } + if _, ok := reply.Services["bar"]; ok { + t.Fatalf("bad: %#v", reply.Services) + } + if !reply.QueryMeta.ResultsFilteredByACLs { + t.Fatal("ResultsFilteredByACLs should be true") + } + }) + + const bexprMatchingUserTokenPermissions = "ServiceName matches `f.*`" + const bexpNotMatchingUserTokenPermissions = "ServiceName matches `b.*`" + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedServices{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Equal(t, 1, len(reply.Services)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexpNotMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedServices{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.Services)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + req := structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: "", // no token + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedServices{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.Services)) + require.False(t, reply.ResultsFilteredByACLs) + }) } func TestCatalog_ServiceNodes_FilterACL(t *testing.T) { @@ -2982,11 +3146,80 @@ func TestCatalog_ServiceNodes_FilterACL(t *testing.T) { } require.True(t, 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 - // that we respect the version 8 ACL flag, since the test server sets - // that to false (the regression value of *not* changing this is better - // for now until we change the sense of the version 8 ACL flag). + bexprMatchingUserTokenPermissions := fmt.Sprintf("Node matches `%s.*`", srv.config.NodeName) + const bexpNotMatchingUserTokenPermissions = "Node matches `node-deny.*`" + + // Register a service of the same name on the denied node + regArg := structs.RegisterRequest{ + Datacenter: "dc1", + Node: "node-deny", + Address: "127.0.0.1", + Service: &structs.NodeService{ + ID: "foo", + Service: "foo", + }, + Check: &structs.HealthCheck{ + CheckID: "service:foo", + Name: "service:foo", + ServiceID: "foo", + Status: api.HealthPassing, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + if err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", ®Arg, nil); err != nil { + t.Fatalf("err: %s", err) + } + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + opt = structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "foo", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedServiceNodes{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &opt, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Equal(t, 1, len(reply.ServiceNodes)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + opt = structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "foo", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexpNotMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedServiceNodes{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &opt, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.ServiceNodes)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + opt = structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "foo", + QueryOptions: structs.QueryOptions{ + Token: "", // no token + Filter: bexpNotMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedServiceNodes{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &opt, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.ServiceNodes)) + require.False(t, reply.ResultsFilteredByACLs) + }) } func TestCatalog_NodeServices_ACL(t *testing.T) { @@ -3075,6 +3308,139 @@ func TestCatalog_NodeServices_FilterACL(t *testing.T) { svc, ok := reply.NodeServices.Services["foo"] require.True(t, ok) require.Equal(t, "foo", svc.ID) + + const bexprMatchingUserTokenPermissions = "Service matches `f.*`" + const bexpNotMatchingUserTokenPermissions = "Service matches `b.*`" + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: srv.config.NodeName, + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedNodeServices{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Equal(t, 1, len(reply.NodeServices.Services)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: srv.config.NodeName, + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexpNotMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedNodeServices{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.NodeServices.Services)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + req := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: srv.config.NodeName, + QueryOptions: structs.QueryOptions{ + Token: "", // no token + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedNodeServices{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Nil(t, reply.NodeServices) + require.False(t, reply.ResultsFilteredByACLs) + }) +} + +func TestCatalog_NodeServicesList_FilterACL(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + + t.Parallel() + dir, token, srv, codec := testACLFilterServer(t) + defer os.RemoveAll(dir) + defer srv.Shutdown() + defer codec.Close() + testrpc.WaitForTestAgent(t, srv.RPC, "dc1", testrpc.WithToken("root")) + + opt := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: srv.config.NodeName, + QueryOptions: structs.QueryOptions{Token: token}, + } + + var reply structs.IndexedNodeServiceList + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.NodeServiceList", &opt, &reply)) + + require.NotNil(t, reply.NodeServices) + require.Len(t, reply.NodeServices.Services, 1) + + const bexprMatchingUserTokenPermissions = "Service matches `f.*`" + const bexpNotMatchingUserTokenPermissions = "Service matches `b.*`" + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: srv.config.NodeName, + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedNodeServiceList{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServiceList", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Equal(t, 1, len(reply.NodeServices.Services)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: srv.config.NodeName, + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexpNotMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedNodeServiceList{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServiceList", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.NodeServices.Services)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + req := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: srv.config.NodeName, + QueryOptions: structs.QueryOptions{ + Token: "", // no token + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply = structs.IndexedNodeServiceList{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServiceList", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Empty(t, reply.NodeServices.Services) + require.False(t, reply.ResultsFilteredByACLs) + }) } func TestCatalog_GatewayServices_TerminatingGateway(t *testing.T) { diff --git a/agent/consul/config_endpoint.go b/agent/consul/config_endpoint.go index 96906dac68..a0d78cc6d2 100644 --- a/agent/consul/config_endpoint.go +++ b/agent/consul/config_endpoint.go @@ -9,7 +9,6 @@ import ( "time" metrics "github.com/armon/go-metrics" - "github.com/armon/go-metrics/prometheus" hashstructure_v2 "github.com/mitchellh/hashstructure/v2" "github.com/hashicorp/go-bexpr" @@ -22,33 +21,6 @@ import ( "github.com/hashicorp/consul/agent/structs" ) -var ConfigSummaries = []prometheus.SummaryDefinition{ - { - Name: []string{"config_entry", "apply"}, - Help: "", - }, - { - Name: []string{"config_entry", "get"}, - Help: "", - }, - { - Name: []string{"config_entry", "list"}, - Help: "", - }, - { - Name: []string{"config_entry", "listAll"}, - Help: "", - }, - { - Name: []string{"config_entry", "delete"}, - Help: "", - }, - { - Name: []string{"config_entry", "resolve_service_config"}, - Help: "", - }, -} - // The ConfigEntry endpoint is used to query centralized config information type ConfigEntry struct { srv *Server @@ -280,7 +252,14 @@ func (c *ConfigEntry) List(args *structs.ConfigEntryQuery, reply *structs.Indexe return err } - // Filter the entries returned by ACL permissions. + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. filteredEntries := make([]structs.ConfigEntry, 0, len(entries)) for _, entry := range entries { if err := entry.CanRead(authz); err != nil { diff --git a/agent/consul/config_endpoint_test.go b/agent/consul/config_endpoint_test.go index 49a10dce21..cf503ee525 100644 --- a/agent/consul/config_endpoint_test.go +++ b/agent/consul/config_endpoint_test.go @@ -783,7 +783,7 @@ service "foo" { } operator = "read" ` - id := createToken(t, codec, rules) + token := createToken(t, codec, rules) // Create some dummy service/proxy configs to be looked up. state := s1.fsm.State() @@ -804,7 +804,7 @@ operator = "read" args := structs.ConfigEntryQuery{ Kind: structs.ServiceDefaults, Datacenter: s1.config.Datacenter, - QueryOptions: structs.QueryOptions{Token: id}, + QueryOptions: structs.QueryOptions{Token: token}, } var out structs.IndexedConfigEntries err := msgpackrpc.CallWithCodec(codec, "ConfigEntry.List", &args, &out) @@ -828,6 +828,58 @@ operator = "read" require.Equal(t, structs.ProxyConfigGlobal, proxyConf.Name) require.Equal(t, structs.ProxyDefaults, proxyConf.Kind) require.False(t, out.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + + // ensure ACL filtering occurs before bexpr filtering. + const bexprMatchingUserTokenPermissions = "Name matches `f.*`" + const bexprNotMatchingUserTokenPermissions = "Name matches `db.*`" + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + args = structs.ConfigEntryQuery{ + Kind: structs.ServiceDefaults, + Datacenter: s1.config.Datacenter, + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprMatchingUserTokenPermissions, + }, + } + var reply structs.IndexedConfigEntries + err = msgpackrpc.CallWithCodec(codec, "ConfigEntry.List", &args, &reply) + require.NoError(t, err) + require.Equal(t, 1, len(reply.Entries)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + args = structs.ConfigEntryQuery{ + Kind: structs.ServiceDefaults, + Datacenter: s1.config.Datacenter, + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprNotMatchingUserTokenPermissions, + }, + } + var reply structs.IndexedConfigEntries + err = msgpackrpc.CallWithCodec(codec, "ConfigEntry.List", &args, &reply) + require.NoError(t, err) + require.Zero(t, len(reply.Entries)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + args = structs.ConfigEntryQuery{ + Kind: structs.ServiceDefaults, + Datacenter: s1.config.Datacenter, + QueryOptions: structs.QueryOptions{ + Token: "", // no token + Filter: bexprNotMatchingUserTokenPermissions, + }, + } + var reply structs.IndexedConfigEntries + err = msgpackrpc.CallWithCodec(codec, "ConfigEntry.List", &args, &reply) + require.NoError(t, err) + require.Zero(t, len(reply.Entries)) + require.False(t, reply.ResultsFilteredByACLs) + }) } func TestConfigEntry_ListAll_ACLDeny(t *testing.T) { diff --git a/agent/consul/health_endpoint.go b/agent/consul/health_endpoint.go index 6f00ec4b08..4ded41bcce 100644 --- a/agent/consul/health_endpoint.go +++ b/agent/consul/health_endpoint.go @@ -63,18 +63,23 @@ func (h *Health) ChecksInState(args *structs.ChecksInStateRequest, } reply.Index, reply.HealthChecks = index, checks - raw, err := filter.Execute(reply.HealthChecks) - if err != nil { + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. + if err := h.srv.filterACL(args.Token, reply); 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 { + raw, err := filter.Execute(reply.HealthChecks) + if err != nil { return err } + reply.HealthChecks = raw.(structs.HealthChecks) return h.srv.sortNodesByDistanceFrom(args.Source, reply.HealthChecks) }) @@ -111,18 +116,23 @@ func (h *Health) NodeChecks(args *structs.NodeSpecificRequest, } reply.Index, reply.HealthChecks = index, checks - raw, err := filter.Execute(reply.HealthChecks) - if err != nil { + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. + if err := h.srv.filterACL(args.Token, reply); 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 { + raw, err := filter.Execute(reply.HealthChecks) + if err != nil { return err } + reply.HealthChecks = raw.(structs.HealthChecks) return nil }) @@ -303,6 +313,18 @@ func (h *Health) ServiceNodes(args *structs.ServiceSpecificRequest, reply *struc thisReply.Nodes = nodeMetaFilter(arg.NodeMetaFilters, thisReply.Nodes) } + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. + if err := h.srv.filterACL(arg.Token, &thisReply); err != nil { + return err + } + raw, err := filter.Execute(thisReply.Nodes) if err != nil { return err @@ -310,13 +332,6 @@ func (h *Health) ServiceNodes(args *structs.ServiceSpecificRequest, reply *struc filteredNodes := raw.(structs.CheckServiceNodes) thisReply.Nodes = filteredNodes.Filter(structs.CheckServiceNodeFilterOptions{FilterType: arg.HealthFilterType}) - // 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(arg.Token, &thisReply); err != nil { - return err - } - if err := h.srv.sortNodesByDistanceFrom(arg.Source, thisReply.Nodes); err != nil { return err } diff --git a/agent/consul/health_endpoint_test.go b/agent/consul/health_endpoint_test.go index 07f23cc2e0..fef8d285a6 100644 --- a/agent/consul/health_endpoint_test.go +++ b/agent/consul/health_endpoint_test.go @@ -1527,11 +1527,62 @@ func TestHealth_NodeChecks_FilterACL(t *testing.T) { require.True(t, found, "bad: %#v", reply.HealthChecks) require.True(t, 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 - // that we respect the version 8 ACL flag, since the test server sets - // that to false (the regression value of *not* changing this is better - // for now until we change the sense of the version 8 ACL flag). + const bexprMatchingUserTokenPermissions = "ServiceName matches `f.*`" + const bexprNotMatchingUserTokenPermissions = "ServiceName matches `b.*`" + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + opt := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: srv.config.NodeName, + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedHealthChecks{} + + if err := msgpackrpc.CallWithCodec(codec, "Health.NodeChecks", &opt, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Equal(t, 1, len(reply.HealthChecks)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + opt := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: srv.config.NodeName, + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprNotMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedHealthChecks{} + + if err := msgpackrpc.CallWithCodec(codec, "Health.NodeChecks", &opt, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.HealthChecks)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + opt := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: srv.config.NodeName, + QueryOptions: structs.QueryOptions{ + Token: "", // no token + Filter: bexprNotMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedHealthChecks{} + + if err := msgpackrpc.CallWithCodec(codec, "Health.NodeChecks", &opt, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.HealthChecks)) + require.False(t, reply.ResultsFilteredByACLs) + }) } func TestHealth_ServiceChecks_FilterACL(t *testing.T) { @@ -1571,11 +1622,77 @@ func TestHealth_ServiceChecks_FilterACL(t *testing.T) { require.Empty(t, reply.HealthChecks) require.True(t, 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 - // that we respect the version 8 ACL flag, since the test server sets - // that to false (the regression value of *not* changing this is better - // for now until we change the sense of the version 8 ACL flag). + // Register a service of the same name on the denied node + regArg := structs.RegisterRequest{ + Datacenter: "dc1", + Node: "node-deny", + Address: "127.0.0.1", + Service: &structs.NodeService{ + ID: "foo", + Service: "foo", + }, + Check: &structs.HealthCheck{ + CheckID: "service:foo", + Name: "service:foo", + ServiceID: "foo", + Status: api.HealthPassing, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + if err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", ®Arg, nil); err != nil { + t.Fatalf("err: %s", err) + } + const bexprMatchingUserTokenPermissions = "ServiceName matches `f.*`" + const bexprNotMatchingUserTokenPermissions = "Node matches `node-deny.*`" + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + opt := structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "foo", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedHealthChecks{} + err := msgpackrpc.CallWithCodec(codec, "Health.ServiceChecks", &opt, &reply) + require.NoError(t, err) + + require.Equal(t, 1, len(reply.HealthChecks)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + opt := structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "foo", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprNotMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedHealthChecks{} + err := msgpackrpc.CallWithCodec(codec, "Health.ServiceChecks", &opt, &reply) + require.NoError(t, err) + require.Zero(t, len(reply.HealthChecks)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + opt := structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "foo", + QueryOptions: structs.QueryOptions{ + Token: "", // no token + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedHealthChecks{} + err := msgpackrpc.CallWithCodec(codec, "Health.ServiceChecks", &opt, &reply) + require.NoError(t, err) + require.Zero(t, len(reply.HealthChecks)) + require.False(t, reply.ResultsFilteredByACLs) + }) } func TestHealth_ServiceNodes_FilterACL(t *testing.T) { @@ -1607,11 +1724,77 @@ func TestHealth_ServiceNodes_FilterACL(t *testing.T) { require.Empty(t, reply.Nodes) require.True(t, 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 - // that we respect the version 8 ACL flag, since the test server sets - // that to false (the regression value of *not* changing this is better - // for now until we change the sense of the version 8 ACL flag). + // Register a service of the same name on the denied node + regArg := structs.RegisterRequest{ + Datacenter: "dc1", + Node: "node-deny", + Address: "127.0.0.1", + Service: &structs.NodeService{ + ID: "foo", + Service: "foo", + }, + Check: &structs.HealthCheck{ + CheckID: "service:foo", + Name: "service:foo", + ServiceID: "foo", + Status: api.HealthPassing, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + if err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", ®Arg, nil); err != nil { + t.Fatalf("err: %s", err) + } + const bexprMatchingUserTokenPermissions = "Service.Service matches `f.*`" + const bexprNotMatchingUserTokenPermissions = "Node.Node matches `node-deny.*`" + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + opt := structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "foo", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedCheckServiceNodes{} + err := msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &opt, &reply) + require.NoError(t, err) + + require.Equal(t, 1, len(reply.Nodes)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + opt := structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "foo", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprNotMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedCheckServiceNodes{} + err := msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &opt, &reply) + require.NoError(t, err) + require.Zero(t, len(reply.Nodes)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + opt := structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "foo", + QueryOptions: structs.QueryOptions{ + Token: "", // no token + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedCheckServiceNodes{} + err := msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &opt, &reply) + require.NoError(t, err) + require.Zero(t, len(reply.Nodes)) + require.False(t, reply.ResultsFilteredByACLs) + }) } func TestHealth_ChecksInState_FilterACL(t *testing.T) { @@ -1647,11 +1830,59 @@ func TestHealth_ChecksInState_FilterACL(t *testing.T) { require.True(t, found, "missing service 'foo': %#v", reply.HealthChecks) require.True(t, 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 - // that we respect the version 8 ACL flag, since the test server sets - // that to false (the regression value of *not* changing this is better - // for now until we change the sense of the version 8 ACL flag). + const bexprMatchingUserTokenPermissions = "ServiceName matches `f.*`" + const bexprNotMatchingUserTokenPermissions = "ServiceName matches `b.*`" + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := structs.ChecksInStateRequest{ + Datacenter: "dc1", + State: api.HealthPassing, + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedHealthChecks{} + if err := msgpackrpc.CallWithCodec(codec, "Health.ChecksInState", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Equal(t, 1, len(reply.HealthChecks)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := structs.ChecksInStateRequest{ + Datacenter: "dc1", + State: api.HealthPassing, + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprNotMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedHealthChecks{} + if err := msgpackrpc.CallWithCodec(codec, "Health.ChecksInState", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.HealthChecks)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + req := structs.ChecksInStateRequest{ + Datacenter: "dc1", + State: api.HealthPassing, + QueryOptions: structs.QueryOptions{ + Token: "", // no token + Filter: bexprNotMatchingUserTokenPermissions, + }, + } + reply := structs.IndexedHealthChecks{} + if err := msgpackrpc.CallWithCodec(codec, "Health.ChecksInState", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.HealthChecks)) + require.False(t, reply.ResultsFilteredByACLs) + }) } func TestHealth_RPC_Filter(t *testing.T) { diff --git a/agent/consul/intention_endpoint.go b/agent/consul/intention_endpoint.go index df05428145..6fc7f8132a 100644 --- a/agent/consul/intention_endpoint.go +++ b/agent/consul/intention_endpoint.go @@ -550,18 +550,24 @@ func (s *Intention) List(args *structs.IntentionListRequest, reply *structs.Inde } else { reply.DataOrigin = structs.IntentionDataOriginLegacy } - raw, err := filter.Execute(reply.Intentions) - if err != nil { + + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. + if err := s.srv.filterACL(args.Token, reply); 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 { + raw, err := filter.Execute(reply.Intentions) + if err != nil { return err } + reply.Intentions = raw.(structs.Intentions) return nil }, diff --git a/agent/consul/intention_endpoint_test.go b/agent/consul/intention_endpoint_test.go index 08480501d7..d1a70fd5bc 100644 --- a/agent/consul/intention_endpoint_test.go +++ b/agent/consul/intention_endpoint_test.go @@ -1639,6 +1639,11 @@ func TestIntentionList_acl(t *testing.T) { token, err := upsertTestTokenWithPolicyRules(codec, TestDefaultInitialManagementToken, "dc1", `service_prefix "foo" { policy = "write" }`) require.NoError(t, err) + const ( + bexprMatch = "DestinationName matches `f.*`" + bexprNoMatch = "DestinationName matches `nomatch.*`" + ) + // Create a few records for _, name := range []string{"foobar", "bar", "baz"} { ixn := structs.IntentionRequest{ @@ -1691,12 +1696,29 @@ func TestIntentionList_acl(t *testing.T) { require.True(t, resp.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") }) - t.Run("filtered", func(t *testing.T) { + // maskResultsFilteredByACLs() in rpc.go sets ResultsFilteredByACLs to false if the token is an empty string + // after resp.QueryMeta.ResultsFilteredByACLs has been determined to be true from filterACLs(). + t.Run("filtered with no token should return no results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + req := &structs.IntentionListRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Filter: bexprMatch, + }, + } + + 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") + }) + + // has access to everything + t.Run("filtered with initial management token should return 1 and ResultsFilteredByACLs equal to false", func(t *testing.T) { req := &structs.IntentionListRequest{ Datacenter: "dc1", QueryOptions: structs.QueryOptions{ Token: TestDefaultInitialManagementToken, - Filter: "DestinationName == foobar", + Filter: bexprMatch, }, } @@ -1705,6 +1727,54 @@ func TestIntentionList_acl(t *testing.T) { require.Len(t, resp.Intentions, 1) require.False(t, resp.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") }) + + // ResultsFilteredByACLs should reflect user does not have access to read all intentions but has access to some. + t.Run("filtered with user token whose permissions match filter should return 1 and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := &structs.IntentionListRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: token.SecretID, + Filter: bexprMatch, + }, + } + + 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") + }) + + // ResultsFilteredByACLs need to act as though no filter was applied. + t.Run("filtered with user token whose permissions do match filter should return 0 and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := &structs.IntentionListRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: token.SecretID, + Filter: bexprNoMatch, + }, + } + + var resp structs.IndexedIntentions + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Intention.List", req, &resp)) + require.Len(t, resp.Intentions, 0) + require.True(t, resp.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) + + // ResultsFilteredByACLs should reflect user does not have access to read any intentions + t.Run("filtered with anonymous token should return 0 and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := &structs.IntentionListRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: "anonymous", + Filter: bexprMatch, + }, + } + + var resp structs.IndexedIntentions + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Intention.List", req, &resp)) + require.Len(t, resp.Intentions, 0) + require.True(t, resp.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) } // Test basic matching. We don't need to exhaustively test inputs since this diff --git a/agent/consul/internal_endpoint.go b/agent/consul/internal_endpoint.go index af27842d20..cab2195845 100644 --- a/agent/consul/internal_endpoint.go +++ b/agent/consul/internal_endpoint.go @@ -117,6 +117,18 @@ func (m *Internal) NodeDump(args *structs.DCSpecificRequest, } reply.Index = maxIndex + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. + if err := m.srv.filterACL(args.Token, reply); err != nil { + return err + } + raw, err := filter.Execute(reply.Dump) if err != nil { return fmt.Errorf("could not filter local node dump: %w", err) @@ -129,13 +141,6 @@ func (m *Internal) NodeDump(args *structs.DCSpecificRequest, } reply.ImportedDump = importedRaw.(structs.NodeDump) - // 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 := m.srv.filterACL(args.Token, reply); err != nil { - return err - } - return nil }) } @@ -235,13 +240,26 @@ func (m *Internal) ServiceDump(args *structs.ServiceDumpRequest, reply *structs. } } reply.Index = maxIndex - raw, err := filter.Execute(reply.Nodes) - if err != nil { - return fmt.Errorf("could not filter local service dump: %w", err) - } - reply.Nodes = raw.(structs.CheckServiceNodes) } + // Note: we filter the results with ACLs *before* applying the user-supplied + // bexpr filter to ensure that the user can only run expressions on data that + // they have access to. This is a security measure to prevent users from + // running arbitrary expressions on data they don't have access to. + // QueryMeta.ResultsFilteredByACLs being true already indicates to the user + // that results they don't have access to have been removed. If they were + // also allowed to run the bexpr filter on the data, they could potentially + // infer the specific attributes of data they don't have access to. + if err := m.srv.filterACL(args.Token, reply); err != nil { + return err + } + + raw, err := filter.Execute(reply.Nodes) + if err != nil { + return fmt.Errorf("could not filter local service dump: %w", err) + } + reply.Nodes = raw.(structs.CheckServiceNodes) + if !args.NodesOnly { importedRaw, err := filter.Execute(reply.ImportedNodes) if err != nil { @@ -249,12 +267,6 @@ func (m *Internal) ServiceDump(args *structs.ServiceDumpRequest, reply *structs. } reply.ImportedNodes = importedRaw.(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 := m.srv.filterACL(args.Token, reply); err != nil { - return err - } return nil }) diff --git a/agent/consul/internal_endpoint_test.go b/agent/consul/internal_endpoint_test.go index e4b9a14b70..91f46cb760 100644 --- a/agent/consul/internal_endpoint_test.go +++ b/agent/consul/internal_endpoint_test.go @@ -656,11 +656,73 @@ func TestInternal_NodeDump_FilterACL(t *testing.T) { t.Fatal("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 - // that we respect the version 8 ACL flag, since the test server sets - // that to false (the regression value of *not* changing this is better - // for now until we change the sense of the version 8 ACL flag). + // need to ensure that ACLs are filtered prior to bexprFiltering + // Register additional node + regArgs := &structs.RegisterRequest{ + Datacenter: "dc1", + Node: "foo", + Address: "127.0.0.1", + WriteRequest: structs.WriteRequest{ + Token: "root", + }, + } + + var out struct{} + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.Register", regArgs, &out)) + + bexprMatchingUserTokenPermissions := fmt.Sprintf("Node matches `%s.*`", srv.config.NodeName) + const bexpNotMatchingUserTokenPermissions = "Node matches `node-deny.*`" + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprMatchingUserTokenPermissions, + }, + } + + reply = structs.IndexedNodeDump{} + if err := msgpackrpc.CallWithCodec(codec, "Internal.NodeDump", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Equal(t, 1, len(reply.Dump)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + req := structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexpNotMatchingUserTokenPermissions, + }, + } + + reply = structs.IndexedNodeDump{} + if err := msgpackrpc.CallWithCodec(codec, "Internal.NodeDump", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.Dump)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would match only record without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + req := structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: "", + Filter: bexprMatchingUserTokenPermissions, + }, + } + + reply = structs.IndexedNodeDump{} + if err := msgpackrpc.CallWithCodec(codec, "Internal.NodeDump", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Empty(t, reply.Dump) + require.False(t, reply.ResultsFilteredByACLs) + }) } func TestInternal_EventFire_Token(t *testing.T) { @@ -1064,6 +1126,113 @@ func TestInternal_ServiceDump_ACL(t *testing.T) { require.Empty(t, out.Gateways) require.True(t, out.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") }) + + // need to ensure that ACLs are filtered prior to bexprFiltering + // Register additional node + regArgs := &structs.RegisterRequest{ + Datacenter: "dc1", + Node: "node-deny", + ID: types.NodeID("e0155642-135d-4739-9853-b1ee6c9f945b"), + Address: "192.18.1.2", + Service: &structs.NodeService{ + Kind: structs.ServiceKindTypical, + ID: "memcached", + Service: "memcached", + Port: 5678, + }, + Check: &structs.HealthCheck{ + Name: "memcached check", + Status: api.HealthPassing, + ServiceID: "memcached", + }, + WriteRequest: structs.WriteRequest{ + Token: "root", + }, + } + + var out struct{} + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.Register", regArgs, &out)) + + const ( + bexprMatchingUserTokenPermissions = "Service.Service matches `redis.*`" + bexpNotMatchingUserTokenPermissions = "Node.Node matches `node-deny.*`" + ) + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + token := tokenWithRules(t, ` + node "node-deny" { + policy = "deny" + } + node "node1" { + policy = "read" + } + service "redis" { + policy = "read" + } + `) + var reply structs.IndexedNodesWithGateways + req := structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexprMatchingUserTokenPermissions, + }, + } + + reply = structs.IndexedNodesWithGateways{} + if err := msgpackrpc.CallWithCodec(codec, "Internal.ServiceDump", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Equal(t, 1, len(reply.Nodes)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + token := tokenWithRules(t, ` + node "node-deny" { + policy = "deny" + } + node "node1" { + policy = "read" + } + service "redis" { + policy = "read" + } + `) + var reply structs.IndexedNodesWithGateways + req := structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: token, + Filter: bexpNotMatchingUserTokenPermissions, + }, + } + + reply = structs.IndexedNodesWithGateways{} + if err := msgpackrpc.CallWithCodec(codec, "Internal.ServiceDump", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Zero(t, len(reply.Nodes)) + require.True(t, reply.ResultsFilteredByACLs) + }) + + t.Run("request with filter that would match only record without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) { + var reply structs.IndexedNodesWithGateways + req := structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{ + Token: "", // no token + Filter: bexpNotMatchingUserTokenPermissions, + }, + } + + reply = structs.IndexedNodesWithGateways{} + if err := msgpackrpc.CallWithCodec(codec, "Internal.ServiceDump", &req, &reply); err != nil { + t.Fatalf("err: %s", err) + } + require.Empty(t, reply.Nodes) + require.False(t, reply.ResultsFilteredByACLs) + }) } func TestInternal_GatewayServiceDump_Terminating(t *testing.T) { diff --git a/agent/proxycfg-glue/internal_service_dump.go b/agent/proxycfg-glue/internal_service_dump.go index d1c701083d..dd8293f781 100644 --- a/agent/proxycfg-glue/internal_service_dump.go +++ b/agent/proxycfg-glue/internal_service_dump.go @@ -81,19 +81,21 @@ func (s *serverInternalServiceDump) Notify(ctx context.Context, req *structs.Ser return 0, nil, err } + totalNodeLength := len(nodes) + aclfilter.New(authz, s.deps.Logger).Filter(&nodes) + raw, err := filter.Execute(nodes) if err != nil { return 0, nil, fmt.Errorf("could not filter local service dump: %w", err) } nodes = raw.(structs.CheckServiceNodes) - aclfilter.New(authz, s.deps.Logger).Filter(&nodes) - return idx, &structs.IndexedCheckServiceNodes{ Nodes: nodes, QueryMeta: structs.QueryMeta{ - Index: idx, - Backend: structs.QueryBackendBlocking, + Index: idx, + Backend: structs.QueryBackendBlocking, + ResultsFilteredByACLs: totalNodeLength != len(nodes), }, }, nil }, diff --git a/agent/proxycfg-glue/internal_service_dump_test.go b/agent/proxycfg-glue/internal_service_dump_test.go index 1eba4c0438..e3d65ac6ae 100644 --- a/agent/proxycfg-glue/internal_service_dump_test.go +++ b/agent/proxycfg-glue/internal_service_dump_test.go @@ -55,6 +55,10 @@ func TestServerInternalServiceDump(t *testing.T) { Service: "web", Kind: structs.ServiceKindTypical, }, + { + Service: "web-deny", + Kind: structs.ServiceKindTypical, + }, { Service: "db", Kind: structs.ServiceKindTypical, @@ -67,14 +71,14 @@ func TestServerInternalServiceDump(t *testing.T) { })) } - authz := newStaticResolver( - policyAuthorizer(t, ` + policyAuth := policyAuthorizer(t, ` service "mgw" { policy = "read" } service "web" { policy = "read" } + service "web-deny" { policy = "deny" } service "db" { policy = "read" } node_prefix "node-" { policy = "read" } - `), - ) + `) + authz := newStaticResolver(policyAuth) dataSource := ServerInternalServiceDump(ServerDataSourceDeps{ GetStore: func() Store { return store }, @@ -121,6 +125,42 @@ func TestServerInternalServiceDump(t *testing.T) { result := getEventResult[*structs.IndexedCheckServiceNodes](t, eventCh) require.Empty(t, result.Nodes) }) + + const ( + bexprMatchingUserTokenPermissions = "Service.Service matches `web.*`" + bexpNotMatchingUserTokenPermissions = "Service.Service matches `mgw.*`" + ) + + authz.SwapAuthorizer(policyAuthorizer(t, ` + service "mgw" { policy = "deny" } + service "web" { policy = "read" } + service "web-deny" { policy = "deny" } + service "db" { policy = "read" } + node_prefix "node-" { policy = "read" } + `)) + + t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) { + eventCh := make(chan proxycfg.UpdateEvent) + require.NoError(t, dataSource.Notify(ctx, &structs.ServiceDumpRequest{ + QueryOptions: structs.QueryOptions{Filter: bexprMatchingUserTokenPermissions}, + }, "", eventCh)) + + result := getEventResult[*structs.IndexedCheckServiceNodes](t, eventCh) + require.Len(t, result.Nodes, 1) + require.Equal(t, "web", result.Nodes[0].Service.Service) + require.True(t, result.ResultsFilteredByACLs) + }) + + t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) { + eventCh := make(chan proxycfg.UpdateEvent) + require.NoError(t, dataSource.Notify(ctx, &structs.ServiceDumpRequest{ + QueryOptions: structs.QueryOptions{Filter: bexpNotMatchingUserTokenPermissions}, + }, "", eventCh)) + + result := getEventResult[*structs.IndexedCheckServiceNodes](t, eventCh) + require.Len(t, result.Nodes, 0) + require.True(t, result.ResultsFilteredByACLs) + }) }) } diff --git a/internal/resource/filter.go b/internal/resource/filter.go deleted file mode 100644 index 09e251e218..0000000000 --- a/internal/resource/filter.go +++ /dev/null @@ -1,109 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -package resource - -import ( - "fmt" - - "github.com/hashicorp/go-bexpr" - - "github.com/hashicorp/consul/proto-public/pbresource" -) - -type MetadataFilterableResources interface { - GetMetadata() map[string]string -} - -// FilterResourcesByMetadata will use the provided go-bexpr based filter to -// retain matching items from the provided slice. -// -// The only variables usable in the expressions are the metadata keys prefixed -// by "metadata." -// -// If no filter is provided, then this does nothing and returns the input. -func FilterResourcesByMetadata[T MetadataFilterableResources](resources []T, filter string) ([]T, error) { - if filter == "" || len(resources) == 0 { - return resources, nil - } - - eval, err := createMetadataFilterEvaluator(filter) - if err != nil { - return nil, err - } - - filtered := make([]T, 0, len(resources)) - for _, res := range resources { - vars := &metadataFilterFieldDetails{ - Meta: res.GetMetadata(), - } - match, err := eval.Evaluate(vars) - if err != nil { - return nil, err - } - if match { - filtered = append(filtered, res) - } - } - if len(filtered) == 0 { - return nil, nil - } - return filtered, nil -} - -// FilterMatchesResourceMetadata will use the provided go-bexpr based filter to -// determine if the provided resource matches. -// -// The only variables usable in the expressions are the metadata keys prefixed -// by "metadata." -// -// If no filter is provided, then this returns true. -func FilterMatchesResourceMetadata(res *pbresource.Resource, filter string) (bool, error) { - if res == nil { - return false, nil - } else if filter == "" { - return true, nil - } - - eval, err := createMetadataFilterEvaluator(filter) - if err != nil { - return false, err - } - - vars := &metadataFilterFieldDetails{ - Meta: res.Metadata, - } - match, err := eval.Evaluate(vars) - if err != nil { - return false, err - } - return match, nil -} - -// ValidateMetadataFilter will validate that the provided filter is going to be -// a valid input to the FilterResourcesByMetadata function. -// -// This is best called from a Validate hook. -func ValidateMetadataFilter(filter string) error { - if filter == "" { - return nil - } - - _, err := createMetadataFilterEvaluator(filter) - return err -} - -func createMetadataFilterEvaluator(filter string) (*bexpr.Evaluator, error) { - sampleVars := &metadataFilterFieldDetails{ - Meta: make(map[string]string), - } - eval, err := bexpr.CreateEvaluatorForType(filter, nil, sampleVars) - if err != nil { - return nil, fmt.Errorf("filter %q is invalid: %w", filter, err) - } - return eval, nil -} - -type metadataFilterFieldDetails struct { - Meta map[string]string `bexpr:"metadata"` -} diff --git a/internal/resource/filter_test.go b/internal/resource/filter_test.go deleted file mode 100644 index e15ec08030..0000000000 --- a/internal/resource/filter_test.go +++ /dev/null @@ -1,195 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -package resource - -import ( - "testing" - - "github.com/stretchr/testify/require" - - "github.com/hashicorp/consul/proto-public/pbresource" - "github.com/hashicorp/consul/proto/private/prototest" - "github.com/hashicorp/consul/sdk/testutil" -) - -func TestFilterResourcesByMetadata(t *testing.T) { - type testcase struct { - in []*pbresource.Resource - filter string - expect []*pbresource.Resource - expectErr string - } - - create := func(name string, kvs ...string) *pbresource.Resource { - require.True(t, len(kvs)%2 == 0) - - meta := make(map[string]string) - for i := 0; i < len(kvs); i += 2 { - meta[kvs[i]] = kvs[i+1] - } - - return &pbresource.Resource{ - Id: &pbresource.ID{ - Name: name, - }, - Metadata: meta, - } - } - - run := func(t *testing.T, tc testcase) { - got, err := FilterResourcesByMetadata(tc.in, tc.filter) - if tc.expectErr != "" { - require.Error(t, err) - testutil.RequireErrorContains(t, err, tc.expectErr) - } else { - require.NoError(t, err) - prototest.AssertDeepEqual(t, tc.expect, got) - } - } - - cases := map[string]testcase{ - "nil input": {}, - "no filter": { - in: []*pbresource.Resource{ - create("one"), - create("two"), - create("three"), - create("four"), - }, - filter: "", - expect: []*pbresource.Resource{ - create("one"), - create("two"), - create("three"), - create("four"), - }, - }, - "bad filter": { - in: []*pbresource.Resource{ - create("one"), - create("two"), - create("three"), - create("four"), - }, - filter: "garbage.value == zzz", - expectErr: `Selector "garbage" is not valid`, - }, - "filter everything out": { - in: []*pbresource.Resource{ - create("one"), - create("two"), - create("three"), - create("four"), - }, - filter: "metadata.foo == bar", - }, - "filter simply": { - in: []*pbresource.Resource{ - create("one", "foo", "bar"), - create("two", "foo", "baz"), - create("three", "zim", "gir"), - create("four", "zim", "gaz", "foo", "bar"), - }, - filter: "metadata.foo == bar", - expect: []*pbresource.Resource{ - create("one", "foo", "bar"), - create("four", "zim", "gaz", "foo", "bar"), - }, - }, - "filter prefix": { - in: []*pbresource.Resource{ - create("one", "foo", "bar"), - create("two", "foo", "baz"), - create("three", "zim", "gir"), - create("four", "zim", "gaz", "foo", "bar"), - create("four", "zim", "zzz"), - }, - filter: "(zim in metadata) and (metadata.zim matches `^g.`)", - expect: []*pbresource.Resource{ - create("three", "zim", "gir"), - create("four", "zim", "gaz", "foo", "bar"), - }, - }, - } - - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - run(t, tc) - }) - } -} - -func TestFilterMatchesResourceMetadata(t *testing.T) { - type testcase struct { - res *pbresource.Resource - filter string - expect bool - expectErr string - } - - create := func(name string, kvs ...string) *pbresource.Resource { - require.True(t, len(kvs)%2 == 0) - - meta := make(map[string]string) - for i := 0; i < len(kvs); i += 2 { - meta[kvs[i]] = kvs[i+1] - } - - return &pbresource.Resource{ - Id: &pbresource.ID{ - Name: name, - }, - Metadata: meta, - } - } - - run := func(t *testing.T, tc testcase) { - got, err := FilterMatchesResourceMetadata(tc.res, tc.filter) - if tc.expectErr != "" { - require.Error(t, err) - testutil.RequireErrorContains(t, err, tc.expectErr) - } else { - require.NoError(t, err) - require.Equal(t, tc.expect, got) - } - } - - cases := map[string]testcase{ - "nil input": {}, - "no filter": { - res: create("one"), - filter: "", - expect: true, - }, - "bad filter": { - res: create("one"), - filter: "garbage.value == zzz", - expectErr: `Selector "garbage" is not valid`, - }, - "no match": { - res: create("one"), - filter: "metadata.foo == bar", - }, - "match simply": { - res: create("one", "foo", "bar"), - filter: "metadata.foo == bar", - expect: true, - }, - "match via prefix": { - res: create("four", "zim", "gaz", "foo", "bar"), - filter: "(zim in metadata) and (metadata.zim matches `^g.`)", - expect: true, - }, - "no match via prefix": { - res: create("four", "zim", "zzz", "foo", "bar"), - filter: "(zim in metadata) and (metadata.zim matches `^g.`)", - }, - } - - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - run(t, tc) - }) - } -}