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

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.
This commit is contained in:
Daniel Nephin 2021-07-30 14:55:35 -04:00
parent e4821a58ee
commit 953c9bee4f
8 changed files with 85 additions and 113 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
agent/consul/catalog_endpoint.go
View File

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

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,

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

10
agent/consul/internal_endpoint.go
View File

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

4
agent/consul/kvs_endpoint.go
View File

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

42
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

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)