luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

Merge pull request #10738 from hashicorp/dnephin/remove-authorizer-nil-checks-2 

acl: remove the last of the authz == nil checks
This commit is contained in:
Daniel Nephin 2021-08-04 17:41:40 -04:00 committed by GitHub
parent 2e9aa91256 953c9bee4f
commit 3dc113ada6
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 132 additions and 162 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
agent/agent_endpoint.go
View File

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

2
agent/connect_auth.go
View File

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

56
agent/consul/catalog_endpoint.go
View File

@ -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
}
}
@ -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)
@ -456,7 +450,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 +653,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
}

12
agent/consul/config_endpoint.go
View File

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

20
agent/consul/coordinate_endpoint.go
View File

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

2
agent/consul/health_endpoint.go
View File

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

39
agent/consul/intention_endpoint.go
View File

@ -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
}
}
@ -690,7 +688,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)
@ -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()

16
agent/consul/internal_endpoint.go
View File

@ -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
}
@ -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)
@ -254,7 +248,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 +332,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
}

54
agent/consul/kvs_endpoint.go
View File

@ -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
}
@ -265,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)

44
agent/consul/session_endpoint.go
View File

@ -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
@ -310,7 +308,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
}

6
agent/consul/txn_endpoint.go
View File

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

16
agent/event_endpoint.go
View File

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

21
agent/ui_endpoint.go
View File

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