From c4c68915c90f6e5ddb1f230af17d688d59828691 Mon Sep 17 00:00:00 2001 From: Dan Upton Date: Fri, 3 Dec 2021 17:38:59 +0000 Subject: [PATCH] event: support `X-Consul-Results-Filtered-By-ACLs` header in list (#11616) --- agent/event_endpoint.go | 41 ++++++++++++++----- agent/event_endpoint_test.go | 78 +++++++++++++++++++++++++----------- 2 files changed, 85 insertions(+), 34 deletions(-) diff --git a/agent/event_endpoint.go b/agent/event_endpoint.go index 02bf81285f..eb6561a1b7 100644 --- a/agent/event_endpoint.go +++ b/agent/event_endpoint.go @@ -127,17 +127,6 @@ RUN_QUERY: // Get the recent events events := s.agent.UserEvents() - // Filter the events using the ACL, if present - for i := 0; i < len(events); i++ { - name := events[i].Name - if authz.EventRead(name, nil) == acl.Allow { - continue - } - s.agent.logger.Debug("dropping event from result due to ACLs", "event", name) - events = append(events[:i], events[i+1:]...) - i-- - } - // Filter the events if requested if nameFilter != "" { for i := 0; i < len(events); i++ { @@ -148,6 +137,36 @@ RUN_QUERY: } } + // Filter the events using the ACL, if present + // + // Note: we filter the results with ACLs *after* applying the user-supplied + // name filter, to ensure the filtered-by-acls header we set below does not + // include results that would be filtered out even if the user did have + // permission. + var removed bool + for i := 0; i < len(events); i++ { + name := events[i].Name + if authz.EventRead(name, nil) == acl.Allow { + continue + } + s.agent.logger.Debug("dropping event from result due to ACLs", "event", name) + removed = true + events = append(events[:i], events[i+1:]...) + i-- + } + + // Set the X-Consul-Results-Filtered-By-ACLs header, but only if the user is + // authenticated (to prevent information leaking). + // + // This is done automatically for HTTP endpoints that proxy to an RPC endpoint + // that sets QueryMeta.ResultsFilteredByACLs, but must be done manually for + // agent-local endpoints. + // + // For more information see the comment on: Server.maskResultsFilteredByACLs. + if token != "" { + setResultsFilteredByACLs(resp, removed) + } + // Determine the index var index uint64 if len(events) == 0 { diff --git a/agent/event_endpoint_test.go b/agent/event_endpoint_test.go index 476bf0cc1e..d3ca95077b 100644 --- a/agent/event_endpoint_test.go +++ b/agent/event_endpoint_test.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" + "github.com/stretchr/testify/require" ) func TestEventFire(t *testing.T) { @@ -199,47 +200,78 @@ func TestEventList_ACLFilter(t *testing.T) { defer a.Shutdown() testrpc.WaitForLeader(t, a.RPC, "dc1") - // Fire an event. - p := &UserEvent{Name: "foo"} - if err := a.UserEvent("dc1", "root", p); err != nil { - t.Fatalf("err: %v", err) + // Fire some events. + events := []*UserEvent{ + {Name: "foo"}, + {Name: "bar"}, + } + for _, e := range events { + err := a.UserEvent("dc1", "root", e) + require.NoError(t, err) } t.Run("no token", func(t *testing.T) { retry.Run(t, func(r *retry.R) { - req, _ := http.NewRequest("GET", "/v1/event/list", nil) + require := require.New(r) + + req := httptest.NewRequest("GET", "/v1/event/list", nil) resp := httptest.NewRecorder() + obj, err := a.srv.EventList(resp, req) - if err != nil { - r.Fatal(err) - } + require.NoError(err) list, ok := obj.([]*UserEvent) - if !ok { - r.Fatalf("bad: %#v", obj) - } - if len(list) != 0 { - r.Fatalf("bad: %#v", list) - } + require.True(ok) + require.Empty(list) + require.Empty(resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) + }) + }) + + t.Run("token with access to one event type", func(t *testing.T) { + retry.Run(t, func(r *retry.R) { + require := require.New(r) + + token := testCreateToken(t, a, ` + event "foo" { + policy = "read" + } + `) + + req := httptest.NewRequest("GET", fmt.Sprintf("/v1/event/list?token=%s", token), nil) + resp := httptest.NewRecorder() + + obj, err := a.srv.EventList(resp, req) + require.NoError(err) + + list, ok := obj.([]*UserEvent) + require.True(ok) + require.Len(list, 1) + require.Equal("foo", list[0].Name) + require.NotEmpty(resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) }) }) t.Run("root token", func(t *testing.T) { retry.Run(t, func(r *retry.R) { - req, _ := http.NewRequest("GET", "/v1/event/list?token=root", nil) + require := require.New(r) + + req := httptest.NewRequest("GET", "/v1/event/list?token=root", nil) resp := httptest.NewRecorder() + obj, err := a.srv.EventList(resp, req) - if err != nil { - r.Fatal(err) - } + require.NoError(err) list, ok := obj.([]*UserEvent) - if !ok { - r.Fatalf("bad: %#v", obj) - } - if len(list) != 1 || list[0].Name != "foo" { - r.Fatalf("bad: %#v", list) + require.True(ok) + require.Len(list, 2) + + var names []string + for _, e := range list { + names = append(names, e.Name) } + require.ElementsMatch([]string{"foo", "bar"}, names) + + require.Empty(resp.Header().Get("X-Consul-Results-Filtered-By-ACLs")) }) }) }