Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

acls: Update ACL authorizer to return meaningful permission when ACLs are disabled #10632

Merged
merged 5 commits into from Jul 30, 2021
Merged

acls: Update ACL authorizer to return meaningful permission when ACLs are disabled #10632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
84fac3c
acl: use acl.ManangeAll when ACLs are disabled
dnephin Jul 15, 2021
727b81a
Fix intention endpoint test
eculver Jul 15, 2021
f2d30bb
Fix api/agent test
eculver Jul 15, 2021
24db06f
Fix maint test
eculver Jul 15, 2021
4c4bbfb
Fix api/agent test
eculver Jul 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 2 additions & 5 deletions agent/acl.go
View file
Open in desktop
Expand Up @@ -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)
Expand All @@ -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.
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion agent/acl_endpoint.go
View file
Open in desktop
Expand Up @@ -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
}

Expand Down
20 changes: 10 additions & 10 deletions agent/agent_endpoint.go
View file
Open in desktop
Expand Up @@ -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
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

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

Expand Down Expand Up @@ -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
}

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

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down
9 changes: 3 additions & 6 deletions agent/agent_endpoint_test.go
View file
Open in desktop
Expand Up @@ -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_"`)
})
}

Expand Down
2 changes: 1 addition & 1 deletion agent/consul/acl.go
View file
Open in desktop
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion agent/consul/acl_test.go
View file
Open in desktop
Expand Up @@ -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)
}

Expand Down
11 changes: 6 additions & 5 deletions agent/consul/connect_ca_endpoint.go
View file
Open in desktop
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand All @@ -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
}
}
Expand Down Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion agent/consul/discovery_chain_endpoint.go
View file
Open in desktop
Expand Up @@ -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
}

Expand Down
6 changes: 3 additions & 3 deletions agent/consul/federation_state_endpoint.go
View file
Open in desktop
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion agent/consul/internal_endpoint.go
View file
Open in desktop
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions agent/consul/operator_autopilot_endpoint.go
View file
Open in desktop
Expand Up @@ -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")
}

Expand Down Expand Up @@ -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")
}

Expand Down Expand Up @@ -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")
}

Expand Down Expand Up @@ -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")
}

Expand Down
6 changes: 3 additions & 3 deletions agent/consul/operator_raft_endpoint.go
View file
Open in desktop
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down
4 changes: 2 additions & 2 deletions agent/consul/prepared_query_endpoint.go
View file
Open in desktop
Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
5 changes: 3 additions & 2 deletions agent/consul/snapshot_endpoint.go
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion agent/http.go
View file
Open in desktop
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion agent/structs/intention.go
View file
Open in desktop
Expand Up @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the authz somehow still be nil here?

Also curious as to what checking against a specific authz singleton provides for us here. IIUC without this check it should fall through and call authz.IntentionWrite on line 338 which would be allowed with the ManageAll authorizer anyways.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's right. I'm going to do a second pass after this to remove more of the nil checks that should no longer be necessary.

return true
}
var authzContext acl.AuthorizerContext
Expand Down
4 changes: 2 additions & 2 deletions agent/xds/server.go
View file
Open in desktop
Expand Up @@ -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:
Expand Down