From e4821a58ee8f43cd14565e69bac42487fc8ca99c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 30 Jul 2021 14:28:19 -0400 Subject: [PATCH] acl: remove acl == nil checks --- agent/agent_endpoint.go | 6 ++-- agent/connect_auth.go | 2 +- agent/consul/catalog_endpoint.go | 8 ++--- agent/consul/config_endpoint.go | 12 +++---- agent/consul/health_endpoint.go | 2 +- agent/consul/intention_endpoint.go | 2 +- agent/consul/internal_endpoint.go | 6 ++-- agent/consul/kvs_endpoint.go | 50 ++++++++++++++---------------- agent/consul/session_endpoint.go | 2 +- agent/consul/txn_endpoint.go | 6 ++-- 10 files changed, 47 insertions(+), 49 deletions(-) diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index fe0b2a603..ff7ecb8ba 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -391,7 +391,7 @@ func (s *HTTPHandlers) AgentService(resp http.ResponseWriter, req *http.Request) } var authzContext acl.AuthorizerContext svc.FillAuthzContext(&authzContext) - if authz != nil && authz.ServiceRead(svc.Service, &authzContext) != acl.Allow { + if authz.ServiceRead(svc.Service, &authzContext) != acl.Allow { return "", nil, acl.ErrPermissionDenied } @@ -837,7 +837,7 @@ func (s *HTTPHandlers) AgentHealthServiceByID(resp http.ResponseWriter, req *htt dc := s.agent.config.Datacenter if service := s.agent.State.Service(sid); service != nil { - if authz != nil && authz.ServiceRead(service.Service, &authzContext) != acl.Allow { + if authz.ServiceRead(service.Service, &authzContext) != acl.Allow { return nil, acl.ErrPermissionDenied } code, status, healthChecks := agentHealthService(sid, s) @@ -886,7 +886,7 @@ func (s *HTTPHandlers) AgentHealthServiceByName(resp http.ResponseWriter, req *h return nil, err } - if authz != nil && authz.ServiceRead(serviceName, &authzContext) != acl.Allow { + if authz.ServiceRead(serviceName, &authzContext) != acl.Allow { return nil, acl.ErrPermissionDenied } diff --git a/agent/connect_auth.go b/agent/connect_auth.go index 293ac7016..5d40ed630 100644 --- a/agent/connect_auth.go +++ b/agent/connect_auth.go @@ -65,7 +65,7 @@ func (a *Agent) ConnectAuthorize(token string, return returnErr(err) } - if authz != nil && authz.ServiceWrite(req.Target, &authzContext) != acl.Allow { + if authz.ServiceWrite(req.Target, &authzContext) != acl.Allow { return returnErr(acl.ErrPermissionDenied) } diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index a9771edbc..36b9bc31b 100644 --- a/agent/consul/catalog_endpoint.go +++ b/agent/consul/catalog_endpoint.go @@ -118,14 +118,14 @@ func servicePreApply(service *structs.NodeService, authz acl.Authorizer) error { // later if version 0.8 is enabled, so we can eventually just // delete this and do all the ACL checks down there. if service.Service != structs.ConsulServiceName { - if authz != nil && authz.ServiceWrite(service.Service, &authzContext) != acl.Allow { + if authz.ServiceWrite(service.Service, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } } // Proxies must have write permission on their destination if service.Kind == structs.ServiceKindConnectProxy { - if authz != nil && authz.ServiceWrite(service.Proxy.DestinationServiceName, &authzContext) != acl.Allow { + if authz.ServiceWrite(service.Proxy.DestinationServiceName, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } } @@ -456,7 +456,7 @@ func (c *Catalog) ServiceNodes(args *structs.ServiceSpecificRequest, reply *stru // If we're doing a connect query, we need read access to the service // we're trying to find proxies for, so check that. if args.Connect { - if authz != nil && authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { + if authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { // Just return nil, which will return an empty response (tested) return nil } @@ -659,7 +659,7 @@ func (c *Catalog) GatewayServices(args *structs.ServiceSpecificRequest, reply *s return err } - if authz != nil && authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { + if authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } diff --git a/agent/consul/config_endpoint.go b/agent/consul/config_endpoint.go index e433e79e1..60729c3dc 100644 --- a/agent/consul/config_endpoint.go +++ b/agent/consul/config_endpoint.go @@ -81,7 +81,7 @@ func (c *ConfigEntry) Apply(args *structs.ConfigEntryRequest, reply *bool) error return err } - if authz != nil && !args.Entry.CanWrite(authz) { + if !args.Entry.CanWrite(authz) { return acl.ErrPermissionDenied } @@ -122,7 +122,7 @@ func (c *ConfigEntry) Get(args *structs.ConfigEntryQuery, reply *structs.ConfigE } lookupEntry.GetEnterpriseMeta().Merge(&args.EnterpriseMeta) - if authz != nil && !lookupEntry.CanRead(authz) { + if !lookupEntry.CanRead(authz) { return acl.ErrPermissionDenied } @@ -180,7 +180,7 @@ func (c *ConfigEntry) List(args *structs.ConfigEntryQuery, reply *structs.Indexe // Filter the entries returned by ACL permissions. filteredEntries := make([]structs.ConfigEntry, 0, len(entries)) for _, entry := range entries { - if authz != nil && !entry.CanRead(authz) { + if !entry.CanRead(authz) { continue } filteredEntries = append(filteredEntries, entry) @@ -240,7 +240,7 @@ func (c *ConfigEntry) ListAll(args *structs.ConfigEntryListAllRequest, reply *st // Filter the entries returned by ACL permissions or by the provided kinds. filteredEntries := make([]structs.ConfigEntry, 0, len(entries)) for _, entry := range entries { - if authz != nil && !entry.CanRead(authz) { + if !entry.CanRead(authz) { continue } // Doing this filter outside of memdb isn't terribly @@ -290,7 +290,7 @@ func (c *ConfigEntry) Delete(args *structs.ConfigEntryRequest, reply *struct{}) return err } - if authz != nil && !args.Entry.CanWrite(authz) { + if !args.Entry.CanWrite(authz) { return acl.ErrPermissionDenied } @@ -315,7 +315,7 @@ func (c *ConfigEntry) ResolveServiceConfig(args *structs.ServiceConfigRequest, r if err != nil { return err } - if authz != nil && authz.ServiceRead(args.Name, &authzContext) != acl.Allow { + if authz.ServiceRead(args.Name, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } diff --git a/agent/consul/health_endpoint.go b/agent/consul/health_endpoint.go index 2dd7d575b..5ef0e9e3b 100644 --- a/agent/consul/health_endpoint.go +++ b/agent/consul/health_endpoint.go @@ -207,7 +207,7 @@ func (h *Health) ServiceNodes(args *structs.ServiceSpecificRequest, reply *struc // If we're doing a connect or ingress query, we need read access to the service // we're trying to find proxies for, so check that. if args.Connect || args.Ingress { - if authz != nil && authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { + if authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { // Just return nil, which will return an empty response (tested) return nil } diff --git a/agent/consul/intention_endpoint.go b/agent/consul/intention_endpoint.go index a8b71b724..18fe2a15e 100644 --- a/agent/consul/intention_endpoint.go +++ b/agent/consul/intention_endpoint.go @@ -690,7 +690,7 @@ func (s *Intention) Check(args *structs.IntentionQueryRequest, reply *structs.In if prefix, ok := query.GetACLPrefix(); ok { var authzContext acl.AuthorizerContext query.FillAuthzContext(&authzContext) - if authz != nil && authz.ServiceRead(prefix, &authzContext) != acl.Allow { + if authz.ServiceRead(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("test on intention denied due to ACLs", "prefix", prefix, "accessorID", accessorID) diff --git a/agent/consul/internal_endpoint.go b/agent/consul/internal_endpoint.go index c6bd10b8e..f64da2fbb 100644 --- a/agent/consul/internal_endpoint.go +++ b/agent/consul/internal_endpoint.go @@ -161,7 +161,7 @@ func (m *Internal) ServiceTopology(args *structs.ServiceSpecificRequest, reply * if err := m.srv.validateEnterpriseRequest(&args.EnterpriseMeta, false); err != nil { return err } - if authz != nil && authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { + if authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } @@ -254,7 +254,7 @@ func (m *Internal) GatewayServiceDump(args *structs.ServiceSpecificRequest, repl } // We need read access to the gateway we're trying to find services for, so check that first. - if authz != nil && authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { + if authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } @@ -338,7 +338,7 @@ func (m *Internal) GatewayIntentions(args *structs.IntentionQueryRequest, reply } // We need read access to the gateway we're trying to find intentions for, so check that first. - if authz != nil && authz.ServiceRead(args.Match.Entries[0].Name, &authzContext) != acl.Allow { + if authz.ServiceRead(args.Match.Entries[0].Name, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } diff --git a/agent/consul/kvs_endpoint.go b/agent/consul/kvs_endpoint.go index 1b748c51e..bb3b4881b 100644 --- a/agent/consul/kvs_endpoint.go +++ b/agent/consul/kvs_endpoint.go @@ -39,37 +39,35 @@ func kvsPreApply(logger hclog.Logger, srv *Server, authz acl.Authorizer, op api. } // Apply the ACL policy if any. - if authz != nil { - switch op { - case api.KVDeleteTree: - var authzContext acl.AuthorizerContext - dirEnt.FillAuthzContext(&authzContext) + switch op { + case api.KVDeleteTree: + var authzContext acl.AuthorizerContext + dirEnt.FillAuthzContext(&authzContext) - if authz.KeyWritePrefix(dirEnt.Key, &authzContext) != acl.Allow { - return false, acl.ErrPermissionDenied - } + if authz.KeyWritePrefix(dirEnt.Key, &authzContext) != acl.Allow { + return false, acl.ErrPermissionDenied + } - case api.KVGet, api.KVGetTree: - // Filtering for GETs is done on the output side. + case api.KVGet, api.KVGetTree: + // Filtering for GETs is done on the output side. - case api.KVCheckSession, api.KVCheckIndex: - // These could reveal information based on the outcome - // of the transaction, and they operate on individual - // keys so we check them here. - var authzContext acl.AuthorizerContext - dirEnt.FillAuthzContext(&authzContext) + case api.KVCheckSession, api.KVCheckIndex: + // These could reveal information based on the outcome + // of the transaction, and they operate on individual + // keys so we check them here. + var authzContext acl.AuthorizerContext + dirEnt.FillAuthzContext(&authzContext) - if authz.KeyRead(dirEnt.Key, &authzContext) != acl.Allow { - return false, acl.ErrPermissionDenied - } + if authz.KeyRead(dirEnt.Key, &authzContext) != acl.Allow { + return false, acl.ErrPermissionDenied + } - default: - var authzContext acl.AuthorizerContext - dirEnt.FillAuthzContext(&authzContext) + default: + var authzContext acl.AuthorizerContext + dirEnt.FillAuthzContext(&authzContext) - if authz.KeyWrite(dirEnt.Key, &authzContext) != acl.Allow { - return false, acl.ErrPermissionDenied - } + if authz.KeyWrite(dirEnt.Key, &authzContext) != acl.Allow { + return false, acl.ErrPermissionDenied } } @@ -244,7 +242,7 @@ func (k *KVS) ListKeys(args *structs.KeyListRequest, reply *structs.IndexedKeyLi return err } - if authz != nil && k.srv.config.ACLEnableKeyListPolicy && authz.KeyList(args.Prefix, &authzContext) != acl.Allow { + if k.srv.config.ACLEnableKeyListPolicy && authz.KeyList(args.Prefix, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } diff --git a/agent/consul/session_endpoint.go b/agent/consul/session_endpoint.go index ca81e5cdc..f7a717701 100644 --- a/agent/consul/session_endpoint.go +++ b/agent/consul/session_endpoint.go @@ -310,7 +310,7 @@ func (s *Session) Renew(args *structs.SessionSpecificRequest, return nil } - if authz != nil && authz.SessionWrite(session.Node, &authzContext) != acl.Allow { + if authz.SessionWrite(session.Node, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } diff --git a/agent/consul/txn_endpoint.go b/agent/consul/txn_endpoint.go index 6f6db7737..26ad1a32b 100644 --- a/agent/consul/txn_endpoint.go +++ b/agent/consul/txn_endpoint.go @@ -81,9 +81,9 @@ func (t *Txn) preCheck(authorizer acl.Authorizer, ops structs.TxnOps) structs.Tx } service := &op.Service.Service - // This is intentionally nil as we will authorize the request - // using vetServiceTxnOp next instead of doing it in servicePreApply - if err := servicePreApply(service, nil); err != nil { + // acl.ManageAll is used here because the request will be authorized + // later using vetServiceTxnOp. + if err := servicePreApply(service, acl.ManageAll()); err != nil { errors = append(errors, &structs.TxnError{ OpIndex: i, What: err.Error(),