From 953c9bee4f4f624d57f0aa07e726e812d8c9a182 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 30 Jul 2021 14:55:35 -0400 Subject: [PATCH] acl: Remove the remaining authz == nil checks These checks were a bit more involved. They were previously skipping some code paths when the authorizer was nil. After looking through these it seems correct to remove the authz == nil check, since it will never evaluate to true. --- agent/consul/catalog_endpoint.go | 48 +++++++++++++---------------- agent/consul/coordinate_endpoint.go | 20 +++++------- agent/consul/intention_endpoint.go | 37 ++++++++++------------ agent/consul/internal_endpoint.go | 10 ++---- agent/consul/kvs_endpoint.go | 4 +-- agent/consul/session_endpoint.go | 42 ++++++++++++------------- agent/event_endpoint.go | 16 +++++----- agent/ui_endpoint.go | 21 ++++++------- 8 files changed, 85 insertions(+), 113 deletions(-) diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index 36b9bc31b..444c83e8f 100644 --- a/agent/consul/catalog_endpoint.go +++ b/agent/consul/catalog_endpoint.go @@ -200,15 +200,12 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error } // Check the complete register request against the given ACL policy. - if authz != nil { - state := c.srv.fsm.State() - _, ns, err := state.NodeServices(nil, args.Node, entMeta) - if err != nil { - return fmt.Errorf("Node lookup failed: %v", err) - } - if err := vetRegisterWithACL(authz, args, ns); err != nil { - return err - } + _, ns, err := state.NodeServices(nil, args.Node, entMeta) + if err != nil { + return fmt.Errorf("Node lookup failed: %v", err) + } + if err := vetRegisterWithACL(authz, args, ns); err != nil { + return err } _, err = c.srv.raftApply(structs.RegisterRequestType, args) @@ -238,29 +235,26 @@ func (c *Catalog) Deregister(args *structs.DeregisterRequest, reply *struct{}) e } // Check the complete deregister request against the given ACL policy. - if authz != nil { - state := c.srv.fsm.State() + state := c.srv.fsm.State() - var ns *structs.NodeService - if args.ServiceID != "" { - _, ns, err = state.NodeService(args.Node, args.ServiceID, &args.EnterpriseMeta) - if err != nil { - return fmt.Errorf("Service lookup failed: %v", err) - } + var ns *structs.NodeService + if args.ServiceID != "" { + _, ns, err = state.NodeService(args.Node, args.ServiceID, &args.EnterpriseMeta) + if err != nil { + return fmt.Errorf("Service lookup failed: %v", err) } + } - var nc *structs.HealthCheck - if args.CheckID != "" { - _, nc, err = state.NodeCheck(args.Node, args.CheckID, &args.EnterpriseMeta) - if err != nil { - return fmt.Errorf("Check lookup failed: %v", err) - } - } - - if err := vetDeregisterWithACL(authz, args, ns, nc); err != nil { - return err + var nc *structs.HealthCheck + if args.CheckID != "" { + _, nc, err = state.NodeCheck(args.Node, args.CheckID, &args.EnterpriseMeta) + if err != nil { + return fmt.Errorf("Check lookup failed: %v", err) } + } + if err := vetDeregisterWithACL(authz, args, ns, nc); err != nil { + return err } _, err = c.srv.raftApply(structs.DeregisterRequestType, args) diff --git a/agent/consul/coordinate_endpoint.go b/agent/consul/coordinate_endpoint.go index e9f714690..503a8a6f8 100644 --- a/agent/consul/coordinate_endpoint.go +++ b/agent/consul/coordinate_endpoint.go @@ -142,12 +142,10 @@ func (c *Coordinate) Update(args *structs.CoordinateUpdateRequest, reply *struct if err != nil { return err } - if authz != nil { - var authzContext acl.AuthorizerContext - structs.DefaultEnterpriseMetaInDefaultPartition().FillAuthzContext(&authzContext) - if authz.NodeWrite(args.Node, &authzContext) != acl.Allow { - return acl.ErrPermissionDenied - } + var authzContext acl.AuthorizerContext + structs.DefaultEnterpriseMetaInDefaultPartition().FillAuthzContext(&authzContext) + if authz.NodeWrite(args.Node, &authzContext) != acl.Allow { + return acl.ErrPermissionDenied } // Add the coordinate to the map of pending updates. @@ -226,12 +224,10 @@ func (c *Coordinate) Node(args *structs.NodeSpecificRequest, reply *structs.Inde if err != nil { return err } - if authz != nil { - var authzContext acl.AuthorizerContext - structs.WildcardEnterpriseMetaInDefaultPartition().FillAuthzContext(&authzContext) - if authz.NodeRead(args.Node, &authzContext) != acl.Allow { - return acl.ErrPermissionDenied - } + var authzContext acl.AuthorizerContext + structs.WildcardEnterpriseMetaInDefaultPartition().FillAuthzContext(&authzContext) + if authz.NodeRead(args.Node, &authzContext) != acl.Allow { + return acl.ErrPermissionDenied } return c.srv.blockingQuery(&args.QueryOptions, diff --git a/agent/consul/intention_endpoint.go b/agent/consul/intention_endpoint.go index 18fe2a15e..247ba8560 100644 --- a/agent/consul/intention_endpoint.go +++ b/agent/consul/intention_endpoint.go @@ -593,24 +593,22 @@ func (s *Intention) Match(args *structs.IntentionQueryRequest, reply *structs.In } } - if authz != nil { - var authzContext acl.AuthorizerContext - // Go through each entry to ensure we have intention:read for the resource. + var authzContext acl.AuthorizerContext + // Go through each entry to ensure we have intention:read for the resource. - // TODO - should we do this instead of filtering the result set? This will only allow - // queries for which the token has intention:read permissions on the requested side - // of the service. Should it instead return all matches that it would be able to list. - // if so we should remove this and call filterACL instead. Based on how this is used - // its probably fine. If you have intention read on the source just do a source type - // matching, if you have it on the dest then perform a dest type match. - for _, entry := range args.Match.Entries { - entry.FillAuthzContext(&authzContext) - if prefix := entry.Name; prefix != "" && authz.IntentionRead(prefix, &authzContext) != acl.Allow { - accessorID := s.aclAccessorID(args.Token) - // todo(kit) Migrate intention access denial logging over to audit logging when we implement it - s.logger.Warn("Operation on intention prefix denied due to ACLs", "prefix", prefix, "accessorID", accessorID) - return acl.ErrPermissionDenied - } + // TODO - should we do this instead of filtering the result set? This will only allow + // queries for which the token has intention:read permissions on the requested side + // of the service. Should it instead return all matches that it would be able to list. + // if so we should remove this and call filterACL instead. Based on how this is used + // its probably fine. If you have intention read on the source just do a source type + // matching, if you have it on the dest then perform a dest type match. + for _, entry := range args.Match.Entries { + entry.FillAuthzContext(&authzContext) + if prefix := entry.Name; prefix != "" && authz.IntentionRead(prefix, &authzContext) != acl.Allow { + accessorID := s.aclAccessorID(args.Token) + // todo(kit) Migrate intention access denial logging over to audit logging when we implement it + s.logger.Warn("Operation on intention prefix denied due to ACLs", "prefix", prefix, "accessorID", accessorID) + return acl.ErrPermissionDenied } } @@ -710,10 +708,7 @@ func (s *Intention) Check(args *structs.IntentionQueryRequest, reply *structs.In // NOTE(mitchellh): This is the same behavior as the agent authorize // endpoint. If this behavior is incorrect, we should also change it there // which is much more important. - defaultDecision := acl.Allow - if authz != nil { - defaultDecision = authz.IntentionDefaultAllow(nil) - } + defaultDecision := authz.IntentionDefaultAllow(nil) state := s.srv.fsm.State() diff --git a/agent/consul/internal_endpoint.go b/agent/consul/internal_endpoint.go index f64da2fbb..06746aa82 100644 --- a/agent/consul/internal_endpoint.go +++ b/agent/consul/internal_endpoint.go @@ -169,10 +169,7 @@ func (m *Internal) ServiceTopology(args *structs.ServiceSpecificRequest, reply * &args.QueryOptions, &reply.QueryMeta, func(ws memdb.WatchSet, state *state.Store) error { - defaultAllow := acl.Allow - if authz != nil { - defaultAllow = authz.IntentionDefaultAllow(nil) - } + defaultAllow := authz.IntentionDefaultAllow(nil) index, topology, err := state.ServiceTopology(ws, args.Datacenter, args.ServiceName, args.ServiceKind, defaultAllow, &args.EnterpriseMeta) if err != nil { @@ -216,10 +213,7 @@ func (m *Internal) IntentionUpstreams(args *structs.ServiceSpecificRequest, repl &args.QueryOptions, &reply.QueryMeta, func(ws memdb.WatchSet, state *state.Store) error { - defaultDecision := acl.Allow - if authz != nil { - defaultDecision = authz.IntentionDefaultAllow(nil) - } + defaultDecision := authz.IntentionDefaultAllow(nil) sn := structs.NewServiceName(args.ServiceName, &args.EnterpriseMeta) index, services, err := state.IntentionTopology(ws, sn, false, defaultDecision) diff --git a/agent/consul/kvs_endpoint.go b/agent/consul/kvs_endpoint.go index bb3b4881b..5f99b3667 100644 --- a/agent/consul/kvs_endpoint.go +++ b/agent/consul/kvs_endpoint.go @@ -263,9 +263,7 @@ func (k *KVS) ListKeys(args *structs.KeyListRequest, reply *structs.IndexedKeyLi reply.Index = index } - if authz != nil { - entries = FilterDirEnt(authz, entries) - } + entries = FilterDirEnt(authz, entries) // Collect the keys from the filtered entries prefixLen := len(args.Prefix) diff --git a/agent/consul/session_endpoint.go b/agent/consul/session_endpoint.go index f7a717701..9d43aef71 100644 --- a/agent/consul/session_endpoint.go +++ b/agent/consul/session_endpoint.go @@ -72,29 +72,27 @@ func (s *Session) Apply(args *structs.SessionRequest, reply *string) error { return err } - if authz != nil { - switch args.Op { - case structs.SessionDestroy: - state := s.srv.fsm.State() - _, existing, err := state.SessionGet(nil, args.Session.ID, &args.Session.EnterpriseMeta) - if err != nil { - return fmt.Errorf("Session lookup failed: %v", err) - } - if existing == nil { - return nil - } - if authz.SessionWrite(existing.Node, &authzContext) != acl.Allow { - return acl.ErrPermissionDenied - } - - case structs.SessionCreate: - if authz.SessionWrite(args.Session.Node, &authzContext) != acl.Allow { - return acl.ErrPermissionDenied - } - - default: - return fmt.Errorf("Invalid session operation %q", args.Op) + switch args.Op { + case structs.SessionDestroy: + state := s.srv.fsm.State() + _, existing, err := state.SessionGet(nil, args.Session.ID, &args.Session.EnterpriseMeta) + if err != nil { + return fmt.Errorf("Session lookup failed: %v", err) } + if existing == nil { + return nil + } + if authz.SessionWrite(existing.Node, &authzContext) != acl.Allow { + return acl.ErrPermissionDenied + } + + case structs.SessionCreate: + if authz.SessionWrite(args.Session.Node, &authzContext) != acl.Allow { + return acl.ErrPermissionDenied + } + + default: + return fmt.Errorf("Invalid session operation %q", args.Op) } // Ensure that the specified behavior is allowed diff --git a/agent/event_endpoint.go b/agent/event_endpoint.go index ca0583f41..02bf81285 100644 --- a/agent/event_endpoint.go +++ b/agent/event_endpoint.go @@ -128,16 +128,14 @@ RUN_QUERY: events := s.agent.UserEvents() // Filter the events using the ACL, if present - if authz != nil { - 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-- + 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 diff --git a/agent/ui_endpoint.go b/agent/ui_endpoint.go index 7660177d5..df2505759 100644 --- a/agent/ui_endpoint.go +++ b/agent/ui_endpoint.go @@ -9,12 +9,13 @@ import ( "sort" "strings" + "github.com/hashicorp/go-hclog" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/logging" - "github.com/hashicorp/go-hclog" ) // ServiceSummary is used to summarize a service @@ -607,17 +608,15 @@ func (s *HTTPHandlers) UIMetricsProxy(resp http.ResponseWriter, req *http.Reques return nil, err } - if authz != nil { - // This endpoint requires wildcard read on all services and all nodes. - // - // In enterprise it requires this _in all namespaces_ too. - wildMeta := structs.WildcardEnterpriseMetaInDefaultPartition() - var authzContext acl.AuthorizerContext - wildMeta.FillAuthzContext(&authzContext) + // This endpoint requires wildcard read on all services and all nodes. + // + // In enterprise it requires this _in all namespaces_ too. + wildMeta := structs.WildcardEnterpriseMetaInDefaultPartition() + var authzContext acl.AuthorizerContext + wildMeta.FillAuthzContext(&authzContext) - if authz.NodeReadAll(&authzContext) != acl.Allow || authz.ServiceReadAll(&authzContext) != acl.Allow { - return nil, acl.ErrPermissionDenied - } + if authz.NodeReadAll(&authzContext) != acl.Allow || authz.ServiceReadAll(&authzContext) != acl.Allow { + return nil, acl.ErrPermissionDenied } log := s.agent.logger.Named(logging.UIMetricsProxy)