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

@@ -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)

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,

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()
 

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)

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)

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

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

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)