From 9dd6d26d05c1d8ce035d63d2f37eb7b9afc8f089 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 30 Jul 2021 13:16:38 -0400 Subject: [PATCH] acl: remove rule == nil checks --- agent/acl.go | 4 +-- agent/agent_endpoint.go | 2 +- agent/agent_endpoint_test.go | 2 +- agent/consul/acl_endpoint.go | 7 ++--- agent/consul/acl_endpoint_legacy.go | 4 +-- agent/consul/internal_endpoint.go | 30 +++++++++---------- .../acl/bindingrule/read/bindingrule_read.go | 8 +++-- 7 files changed, 27 insertions(+), 30 deletions(-) diff --git a/agent/acl.go b/agent/acl.go index b08bb4067c..d54fa28428 100644 --- a/agent/acl.go +++ b/agent/acl.go @@ -193,9 +193,6 @@ func (a *Agent) filterMembers(token string, members *[]serf.Member) error { if err != nil { return err } - if rule == nil { - return nil - } var authzContext acl.AuthorizerContext structs.DefaultEnterpriseMetaInDefaultPartition().FillAuthzContext(&authzContext) @@ -216,6 +213,7 @@ func (a *Agent) filterMembers(token string, members *[]serf.Member) error { } // filterServices redacts services that the token doesn't have access to. +// TODO: move to test file func (a *Agent) filterServices(token string, services *map[structs.ServiceID]*structs.NodeService) error { // Resolve the token and bail if ACLs aren't enabled. authz, err := a.delegate.ResolveTokenAndDefaultMeta(token, nil, nil) diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index be2b4fe049..fe0b2a6035 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -175,7 +175,7 @@ func (s *HTTPHandlers) AgentMetricsStream(resp http.ResponseWriter, req *http.Re switch { case err != nil: return nil, err - case rule != nil && rule.AgentRead(s.agent.config.NodeName, nil) != acl.Allow: + case rule.AgentRead(s.agent.config.NodeName, nil) != acl.Allow: return nil, acl.ErrPermissionDenied } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 9f2e4870f4..4b45c193b3 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -1448,7 +1448,7 @@ func TestHTTPHandlers_AgentMetricsStream(t *testing.T) { bd.Tokens = new(tokenStore.Store) sink := metrics.NewInmemSink(20*time.Millisecond, time.Second) bd.MetricsHandler = sink - d := fakeResolveTokenDelegate{} + d := fakeResolveTokenDelegate{authorizer: acl.ManageAll()} agent := &Agent{ baseDeps: bd, delegate: d, diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index 582a23631e..5841c1132c 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -2028,11 +2028,10 @@ func (a *ACL) BindingRuleDelete(args *structs.ACLBindingRuleDeleteRequest, reply } _, rule, err := a.srv.fsm.State().ACLBindingRuleGetByID(nil, args.BindingRuleID, &args.EnterpriseMeta) - if err != nil { + switch { + case err != nil: return err - } - - if rule == nil { + case rule == nil: return nil } diff --git a/agent/consul/acl_endpoint_legacy.go b/agent/consul/acl_endpoint_legacy.go index 3653ba8f90..ab004fa3a2 100644 --- a/agent/consul/acl_endpoint_legacy.go +++ b/agent/consul/acl_endpoint_legacy.go @@ -169,7 +169,7 @@ func (a *ACL) Apply(args *structs.ACLRequest, reply *string) error { // NOTE: We will not support enterprise authorizer contexts with legacy ACLs if rule, err := a.srv.ResolveToken(args.Token); err != nil { return err - } else if rule == nil || rule.ACLWrite(nil) != acl.Allow { + } else if rule.ACLWrite(nil) != acl.Allow { return acl.ErrPermissionDenied } @@ -261,7 +261,7 @@ func (a *ACL) List(args *structs.DCSpecificRequest, // and this check for ACLWrite is basically what it did before. if rule, err := a.srv.ResolveToken(args.Token); err != nil { return err - } else if rule == nil || rule.ACLWrite(nil) != acl.Allow { + } else if rule.ACLWrite(nil) != acl.Allow { return acl.ErrPermissionDenied } diff --git a/agent/consul/internal_endpoint.go b/agent/consul/internal_endpoint.go index a2c2890eb5..c6bd10b8ef 100644 --- a/agent/consul/internal_endpoint.go +++ b/agent/consul/internal_endpoint.go @@ -452,23 +452,21 @@ func (m *Internal) KeyringOperation( if err := m.srv.validateEnterpriseToken(identity); err != nil { return err } - if rule != nil { - switch args.Operation { - case structs.KeyringList: - if rule.KeyringRead(nil) != acl.Allow { - return fmt.Errorf("Reading keyring denied by ACLs") - } - case structs.KeyringInstall: - fallthrough - case structs.KeyringUse: - fallthrough - case structs.KeyringRemove: - if rule.KeyringWrite(nil) != acl.Allow { - return fmt.Errorf("Modifying keyring denied due to ACLs") - } - default: - panic("Invalid keyring operation") + switch args.Operation { + case structs.KeyringList: + if rule.KeyringRead(nil) != acl.Allow { + return fmt.Errorf("Reading keyring denied by ACLs") } + case structs.KeyringInstall: + fallthrough + case structs.KeyringUse: + fallthrough + case structs.KeyringRemove: + if rule.KeyringWrite(nil) != acl.Allow { + return fmt.Errorf("Modifying keyring denied due to ACLs") + } + default: + panic("Invalid keyring operation") } if args.LocalOnly || args.Forwarded || m.srv.serfWAN == nil { diff --git a/command/acl/bindingrule/read/bindingrule_read.go b/command/acl/bindingrule/read/bindingrule_read.go index cb31664da0..9957e5fdb9 100644 --- a/command/acl/bindingrule/read/bindingrule_read.go +++ b/command/acl/bindingrule/read/bindingrule_read.go @@ -5,10 +5,11 @@ import ( "fmt" "strings" + "github.com/mitchellh/cli" + "github.com/hashicorp/consul/command/acl" "github.com/hashicorp/consul/command/acl/bindingrule" "github.com/hashicorp/consul/command/flags" - "github.com/mitchellh/cli" ) func New(ui cli.Ui) *cmd { @@ -85,10 +86,11 @@ func (c *cmd) Run(args []string) int { } rule, _, err := client.ACL().BindingRuleRead(ruleID, nil) - if err != nil { + switch { + case err != nil: c.UI.Error(fmt.Sprintf("Error reading binding rule %q: %v", ruleID, err)) return 1 - } else if rule == nil { + case rule == nil: c.UI.Error(fmt.Sprintf("Binding rule not found with ID %q", ruleID)) return 1 }