diff --git a/agent/acl.go b/agent/acl.go index 6eaeadc74..b08bb4067 100644 --- a/agent/acl.go +++ b/agent/acl.go @@ -75,6 +75,7 @@ func (a *Agent) vetServiceRegisterWithAuthorizer(authz acl.Authorizer, service * // vetServiceUpdate makes sure the service update action is allowed by the given // token. +// TODO: move to test package func (a *Agent) vetServiceUpdate(token string, serviceID structs.ServiceID) error { // Resolve the token and bail if ACLs aren't enabled. authz, err := a.delegate.ResolveTokenAndDefaultMeta(token, nil, nil) @@ -86,10 +87,6 @@ func (a *Agent) vetServiceUpdate(token string, serviceID structs.ServiceID) erro } func (a *Agent) vetServiceUpdateWithAuthorizer(authz acl.Authorizer, serviceID structs.ServiceID) error { - if authz == nil { - return nil - } - var authzContext acl.AuthorizerContext // Vet any changes based on the existing services's info. @@ -100,7 +97,7 @@ func (a *Agent) vetServiceUpdateWithAuthorizer(authz acl.Authorizer, serviceID s return acl.PermissionDenied("Missing service:write on %s", serviceName.String()) } } else { - return fmt.Errorf("Unknown service %q", serviceID) + return NotFoundError{Reason: fmt.Sprintf("Unknown service %q", serviceID)} } return nil diff --git a/agent/acl_endpoint.go b/agent/acl_endpoint.go index 5adf5cbad..a796cb3d8 100644 --- a/agent/acl_endpoint.go +++ b/agent/acl_endpoint.go @@ -110,7 +110,7 @@ func (s *HTTPHandlers) ACLRulesTranslate(resp http.ResponseWriter, req *http.Req } // Should this require lesser permissions? Really the only reason to require authorization at all is // to prevent external entities from DoS Consul with repeated rule translation requests - if rule != nil && rule.ACLRead(nil) != acl.Allow { + if rule.ACLRead(nil) != acl.Allow { return nil, acl.ErrPermissionDenied } diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index b8f9452c3..be2b4fe04 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -55,7 +55,7 @@ func (s *HTTPHandlers) AgentSelf(resp http.ResponseWriter, req *http.Request) (i if err != nil { return nil, err } - if rule != nil && rule.AgentRead(s.agent.config.NodeName, nil) != acl.Allow { + if rule.AgentRead(s.agent.config.NodeName, nil) != acl.Allow { return nil, acl.ErrPermissionDenied } @@ -144,7 +144,7 @@ func (s *HTTPHandlers) AgentMetrics(resp http.ResponseWriter, req *http.Request) if err != nil { return nil, err } - if rule != nil && rule.AgentRead(s.agent.config.NodeName, nil) != acl.Allow { + if rule.AgentRead(s.agent.config.NodeName, nil) != acl.Allow { return nil, acl.ErrPermissionDenied } if enablePrometheusOutput(req) { @@ -224,7 +224,7 @@ func (s *HTTPHandlers) AgentReload(resp http.ResponseWriter, req *http.Request) if err != nil { return nil, err } - if rule != nil && rule.AgentWrite(s.agent.config.NodeName, nil) != acl.Allow { + if rule.AgentWrite(s.agent.config.NodeName, nil) != acl.Allow { return nil, acl.ErrPermissionDenied } @@ -512,7 +512,7 @@ func (s *HTTPHandlers) AgentJoin(resp http.ResponseWriter, req *http.Request) (i if err != nil { return nil, err } - if rule != nil && rule.AgentWrite(s.agent.config.NodeName, nil) != acl.Allow { + if rule.AgentWrite(s.agent.config.NodeName, nil) != acl.Allow { return nil, acl.ErrPermissionDenied } @@ -544,7 +544,7 @@ func (s *HTTPHandlers) AgentLeave(resp http.ResponseWriter, req *http.Request) ( if err != nil { return nil, err } - if rule != nil && rule.AgentWrite(s.agent.config.NodeName, nil) != acl.Allow { + if rule.AgentWrite(s.agent.config.NodeName, nil) != acl.Allow { return nil, acl.ErrPermissionDenied } @@ -562,7 +562,7 @@ func (s *HTTPHandlers) AgentForceLeave(resp http.ResponseWriter, req *http.Reque if err != nil { return nil, err } - if rule != nil && rule.OperatorWrite(nil) != acl.Allow { + if rule.OperatorWrite(nil) != acl.Allow { return nil, acl.ErrPermissionDenied } @@ -1198,7 +1198,7 @@ func (s *HTTPHandlers) AgentNodeMaintenance(resp http.ResponseWriter, req *http. if err != nil { return nil, err } - if rule != nil && rule.NodeWrite(s.agent.config.NodeName, nil) != acl.Allow { + if rule.NodeWrite(s.agent.config.NodeName, nil) != acl.Allow { return nil, acl.ErrPermissionDenied } @@ -1219,7 +1219,7 @@ func (s *HTTPHandlers) AgentMonitor(resp http.ResponseWriter, req *http.Request) if err != nil { return nil, err } - if rule != nil && rule.AgentRead(s.agent.config.NodeName, nil) != acl.Allow { + if rule.AgentRead(s.agent.config.NodeName, nil) != acl.Allow { return nil, acl.ErrPermissionDenied } @@ -1298,7 +1298,7 @@ func (s *HTTPHandlers) AgentToken(resp http.ResponseWriter, req *http.Request) ( if err != nil { return nil, err } - if rule != nil && rule.AgentWrite(s.agent.config.NodeName, nil) != acl.Allow { + if rule.AgentWrite(s.agent.config.NodeName, nil) != acl.Allow { return nil, acl.ErrPermissionDenied } @@ -1476,7 +1476,7 @@ func (s *HTTPHandlers) AgentHost(resp http.ResponseWriter, req *http.Request) (i return nil, err } - if rule != nil && rule.OperatorRead(nil) != acl.Allow { + if rule.OperatorRead(nil) != acl.Allow { return nil, acl.ErrPermissionDenied } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index d4ab2b322..9f2e4870f 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -4522,12 +4522,9 @@ func TestAgent_ServiceMaintenance_BadRequest(t *testing.T) { t.Run("bad service id", func(t *testing.T) { req, _ := http.NewRequest("PUT", "/v1/agent/service/maintenance/_nope_?enable=true", nil) resp := httptest.NewRecorder() - if _, err := a.srv.AgentServiceMaintenance(resp, req); err != nil { - t.Fatalf("err: %s", err) - } - if resp.Code != 404 { - t.Fatalf("expected 404, got %d", resp.Code) - } + a.srv.h.ServeHTTP(resp, req) + require.Equal(t, 404, resp.Code) + require.Contains(t, resp.Body.String(), `Unknown service "_nope_"`) }) } diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 456a6efa2..463c9087b 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -1186,7 +1186,7 @@ func (r *ACLResolver) resolveLocallyManagedToken(token string) (structs.ACLIdent func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs.ACLIdentity, acl.Authorizer, error) { if !r.ACLsEnabled() { - return nil, nil, nil + return nil, acl.ManageAll(), nil } if acl.RootAuthorizer(token) != nil { diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index c85e5c2a0..f1582c8f5 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -757,7 +757,7 @@ func TestACLResolver_Disabled(t *testing.T) { r := newTestACLResolver(t, delegate, nil) authz, err := r.ResolveToken("does not exist") - require.Nil(t, authz) + require.Equal(t, acl.ManageAll(), authz) require.Nil(t, err) } diff --git a/agent/consul/connect_ca_endpoint.go b/agent/consul/connect_ca_endpoint.go index ab361f1a3..f12abea9a 100644 --- a/agent/consul/connect_ca_endpoint.go +++ b/agent/consul/connect_ca_endpoint.go @@ -65,7 +65,7 @@ func (s *ConnectCA) ConfigurationGet( if err != nil { return err } - if rule != nil && rule.OperatorWrite(nil) != acl.Allow { + if rule.OperatorWrite(nil) != acl.Allow { return acl.ErrPermissionDenied } @@ -97,7 +97,7 @@ func (s *ConnectCA) ConfigurationSet( if err != nil { return err } - if rule != nil && rule.OperatorWrite(nil) != acl.Allow { + if rule.OperatorWrite(nil) != acl.Allow { return acl.ErrPermissionDenied } @@ -175,7 +175,7 @@ func (s *ConnectCA) Sign( if isService { entMeta.Merge(serviceID.GetEnterpriseMeta()) entMeta.FillAuthzContext(&authzContext) - if rule != nil && rule.ServiceWrite(serviceID.Service, &authzContext) != acl.Allow { + if rule.ServiceWrite(serviceID.Service, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } @@ -186,8 +186,9 @@ func (s *ConnectCA) Sign( "we are %s", serviceID.Datacenter, s.srv.config.Datacenter) } } else if isAgent { + structs.DefaultEnterpriseMetaInDefaultPartition().FillAuthzContext(&authzContext) - if rule != nil && rule.NodeWrite(agentID.Agent, &authzContext) != acl.Allow { + if rule.NodeWrite(agentID.Agent, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } } @@ -223,7 +224,7 @@ func (s *ConnectCA) SignIntermediate( if err != nil { return err } - if rule != nil && rule.OperatorWrite(nil) != acl.Allow { + if rule.OperatorWrite(nil) != acl.Allow { return acl.ErrPermissionDenied } diff --git a/agent/consul/discovery_chain_endpoint.go b/agent/consul/discovery_chain_endpoint.go index 00327d1bb..d71a636e1 100644 --- a/agent/consul/discovery_chain_endpoint.go +++ b/agent/consul/discovery_chain_endpoint.go @@ -35,7 +35,7 @@ func (c *DiscoveryChain) Get(args *structs.DiscoveryChainRequest, reply *structs if err != nil { return err } - if rule != nil && rule.ServiceRead(args.Name, &authzContext) != acl.Allow { + if rule.ServiceRead(args.Name, &authzContext) != acl.Allow { return acl.ErrPermissionDenied } diff --git a/agent/consul/federation_state_endpoint.go b/agent/consul/federation_state_endpoint.go index d4eafe835..3d796f019 100644 --- a/agent/consul/federation_state_endpoint.go +++ b/agent/consul/federation_state_endpoint.go @@ -63,7 +63,7 @@ func (c *FederationState) Apply(args *structs.FederationStateRequest, reply *boo if err != nil { return err } - if rule != nil && rule.OperatorWrite(nil) != acl.Allow { + if rule.OperatorWrite(nil) != acl.Allow { return acl.ErrPermissionDenied } @@ -109,7 +109,7 @@ func (c *FederationState) Get(args *structs.FederationStateQuery, reply *structs if err != nil { return err } - if rule != nil && rule.OperatorRead(nil) != acl.Allow { + if rule.OperatorRead(nil) != acl.Allow { return acl.ErrPermissionDenied } @@ -150,7 +150,7 @@ func (c *FederationState) List(args *structs.DCSpecificRequest, reply *structs.I if err != nil { return err } - if rule != nil && rule.OperatorRead(nil) != acl.Allow { + if rule.OperatorRead(nil) != acl.Allow { return acl.ErrPermissionDenied } diff --git a/agent/consul/internal_endpoint.go b/agent/consul/internal_endpoint.go index dad798dfc..a2c2890eb 100644 --- a/agent/consul/internal_endpoint.go +++ b/agent/consul/internal_endpoint.go @@ -409,7 +409,7 @@ func (m *Internal) EventFire(args *structs.EventFireRequest, return err } - if rule != nil && rule.EventWrite(args.Name, nil) != acl.Allow { + if rule.EventWrite(args.Name, nil) != acl.Allow { accessorID := m.aclAccessorID(args.Token) m.logger.Warn("user event blocked by ACLs", "event", args.Name, "accessorID", accessorID) return acl.ErrPermissionDenied diff --git a/agent/consul/operator_autopilot_endpoint.go b/agent/consul/operator_autopilot_endpoint.go index b415a7cc2..53babd0e8 100644 --- a/agent/consul/operator_autopilot_endpoint.go +++ b/agent/consul/operator_autopilot_endpoint.go @@ -24,7 +24,7 @@ func (op *Operator) AutopilotGetConfiguration(args *structs.DCSpecificRequest, r if err := op.srv.validateEnterpriseToken(identity); err != nil { return err } - if rule != nil && rule.OperatorRead(nil) != acl.Allow { + if rule.OperatorRead(nil) != acl.Allow { return acl.PermissionDenied("Missing operator:read permissions") } @@ -56,7 +56,7 @@ func (op *Operator) AutopilotSetConfiguration(args *structs.AutopilotSetConfigRe if err := op.srv.validateEnterpriseToken(identity); err != nil { return err } - if rule != nil && rule.OperatorWrite(nil) != acl.Allow { + if rule.OperatorWrite(nil) != acl.Allow { return acl.PermissionDenied("Missing operator:write permissions") } @@ -91,7 +91,7 @@ func (op *Operator) ServerHealth(args *structs.DCSpecificRequest, reply *structs if err := op.srv.validateEnterpriseToken(identity); err != nil { return err } - if rule != nil && rule.OperatorRead(nil) != acl.Allow { + if rule.OperatorRead(nil) != acl.Allow { return acl.PermissionDenied("Missing operator:read permissions") } @@ -158,7 +158,7 @@ func (op *Operator) AutopilotState(args *structs.DCSpecificRequest, reply *autop if err := op.srv.validateEnterpriseToken(identity); err != nil { return err } - if rule != nil && rule.OperatorRead(nil) != acl.Allow { + if rule.OperatorRead(nil) != acl.Allow { return acl.PermissionDenied("Missing operator:read permissions") } diff --git a/agent/consul/operator_raft_endpoint.go b/agent/consul/operator_raft_endpoint.go index d7ceb7e9a..72cd7a3ff 100644 --- a/agent/consul/operator_raft_endpoint.go +++ b/agent/consul/operator_raft_endpoint.go @@ -23,7 +23,7 @@ func (op *Operator) RaftGetConfiguration(args *structs.DCSpecificRequest, reply if err != nil { return err } - if rule != nil && rule.OperatorRead(nil) != acl.Allow { + if rule.OperatorRead(nil) != acl.Allow { return acl.ErrPermissionDenied } @@ -88,7 +88,7 @@ func (op *Operator) RaftRemovePeerByAddress(args *structs.RaftRemovePeerRequest, if err := op.srv.validateEnterpriseToken(identity); err != nil { return err } - if rule != nil && rule.OperatorWrite(nil) != acl.Allow { + if rule.OperatorWrite(nil) != acl.Allow { return acl.ErrPermissionDenied } @@ -141,7 +141,7 @@ func (op *Operator) RaftRemovePeerByID(args *structs.RaftRemovePeerRequest, repl if err := op.srv.validateEnterpriseToken(identity); err != nil { return err } - if rule != nil && rule.OperatorWrite(nil) != acl.Allow { + if rule.OperatorWrite(nil) != acl.Allow { return acl.ErrPermissionDenied } diff --git a/agent/consul/prepared_query_endpoint.go b/agent/consul/prepared_query_endpoint.go index 827ad8630..84b2f3527 100644 --- a/agent/consul/prepared_query_endpoint.go +++ b/agent/consul/prepared_query_endpoint.go @@ -86,7 +86,7 @@ func (p *PreparedQuery) Apply(args *structs.PreparedQueryRequest, reply *string) // need to make sure they have write access for whatever they are // proposing. if prefix, ok := args.Query.GetACLPrefix(); ok { - if rule != nil && rule.PreparedQueryWrite(prefix, nil) != acl.Allow { + if rule.PreparedQueryWrite(prefix, nil) != acl.Allow { p.logger.Warn("Operation on prepared query denied due to ACLs", "query", args.Query.ID) return acl.ErrPermissionDenied } @@ -106,7 +106,7 @@ func (p *PreparedQuery) Apply(args *structs.PreparedQueryRequest, reply *string) } if prefix, ok := query.GetACLPrefix(); ok { - if rule != nil && rule.PreparedQueryWrite(prefix, nil) != acl.Allow { + if rule.PreparedQueryWrite(prefix, nil) != acl.Allow { p.logger.Warn("Operation on prepared query denied due to ACLs", "query", args.Query.ID) return acl.ErrPermissionDenied } diff --git a/agent/consul/snapshot_endpoint.go b/agent/consul/snapshot_endpoint.go index 7cddd733e..489a2520f 100644 --- a/agent/consul/snapshot_endpoint.go +++ b/agent/consul/snapshot_endpoint.go @@ -16,11 +16,12 @@ import ( "net" "time" + "github.com/hashicorp/go-msgpack/codec" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/pool" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/snapshot" - "github.com/hashicorp/go-msgpack/codec" ) // dispatchSnapshotRequest takes an incoming request structure with possibly some @@ -61,7 +62,7 @@ func (s *Server) dispatchSnapshotRequest(args *structs.SnapshotRequest, in io.Re // all the ACLs and you could escalate from there. if rule, err := s.ResolveToken(args.Token); err != nil { return nil, err - } else if rule != nil && rule.Snapshot(nil) != acl.Allow { + } else if rule.Snapshot(nil) != acl.Allow { return nil, acl.ErrPermissionDenied } diff --git a/agent/http.go b/agent/http.go index 409ecbda4..f2d785054 100644 --- a/agent/http.go +++ b/agent/http.go @@ -253,7 +253,7 @@ func (s *HTTPHandlers) handler(enableDebug bool) http.Handler { // If the token provided does not have the necessary permissions, // write a forbidden response - if rule != nil && rule.OperatorRead(nil) != acl.Allow { + if rule.OperatorRead(nil) != acl.Allow { resp.WriteHeader(http.StatusForbidden) return } diff --git a/agent/structs/intention.go b/agent/structs/intention.go index 2549f8d5e..078bcbe50 100644 --- a/agent/structs/intention.go +++ b/agent/structs/intention.go @@ -325,7 +325,7 @@ func (ixn *Intention) CanRead(authz acl.Authorizer) bool { } func (ixn *Intention) CanWrite(authz acl.Authorizer) bool { - if authz == nil { + if authz == nil || authz == acl.ManageAll() { return true } var authzContext acl.AuthorizerContext diff --git a/agent/xds/server.go b/agent/xds/server.go index 2bd90a8e7..35e86b1c9 100644 --- a/agent/xds/server.go +++ b/agent/xds/server.go @@ -583,12 +583,12 @@ func (s *Server) checkStreamACLs(streamCtx context.Context, cfgSnap *proxycfg.Co switch cfgSnap.Kind { case structs.ServiceKindConnectProxy: cfgSnap.ProxyID.EnterpriseMeta.FillAuthzContext(&authzContext) - if rule != nil && rule.ServiceWrite(cfgSnap.Proxy.DestinationServiceName, &authzContext) != acl.Allow { + if rule.ServiceWrite(cfgSnap.Proxy.DestinationServiceName, &authzContext) != acl.Allow { return status.Errorf(codes.PermissionDenied, "permission denied") } case structs.ServiceKindMeshGateway, structs.ServiceKindTerminatingGateway, structs.ServiceKindIngressGateway: cfgSnap.ProxyID.EnterpriseMeta.FillAuthzContext(&authzContext) - if rule != nil && rule.ServiceWrite(cfgSnap.Service, &authzContext) != acl.Allow { + if rule.ServiceWrite(cfgSnap.Service, &authzContext) != acl.Allow { return status.Errorf(codes.PermissionDenied, "permission denied") } default: diff --git a/api/agent_test.go b/api/agent_test.go index 9f717732f..9e468095c 100644 --- a/api/agent_test.go +++ b/api/agent_test.go @@ -618,7 +618,11 @@ func TestAPI_AgentServiceAddress(t *testing.T) { require.Equal(t, services["foo2"].TaggedAddresses["wan"].Address, "198.18.0.1") require.Equal(t, services["foo2"].TaggedAddresses["wan"].Port, 80) - if err := agent.ServiceDeregister("foo"); err != nil { + if err := agent.ServiceDeregister("foo1"); err != nil { + t.Fatalf("err: %v", err) + } + + if err := agent.ServiceDeregister("foo2"); err != nil { t.Fatalf("err: %v", err) } } @@ -1641,7 +1645,7 @@ func TestAPI_AgentConnectAuthorize(t *testing.T) { auth, err := agent.ConnectAuthorize(params) require.Nil(err) require.True(auth.Authorized) - require.Equal(auth.Reason, "ACLs disabled, access is allowed by default") + require.Equal(auth.Reason, "Default behavior configured by ACLs") } func TestAPI_AgentHealthServiceOpts(t *testing.T) { diff --git a/command/maint/maint_test.go b/command/maint/maint_test.go index 18ee9eb40..9e538eba9 100644 --- a/command/maint/maint_test.go +++ b/command/maint/maint_test.go @@ -256,7 +256,7 @@ func TestMaintCommand_ServiceMaintenance_NoService(t *testing.T) { t.Fatalf("expected response code 1, got %d", code) } - if !strings.Contains(ui.ErrorWriter.String(), "No service registered") { + if !strings.Contains(ui.ErrorWriter.String(), "Unknown service") { t.Fatalf("bad: %#v", ui.ErrorWriter.String()) } }