From 5b2e5882b43f3fdf9e32543e0e42568d76d6c040 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 4 Aug 2021 18:06:44 -0400 Subject: [PATCH 1/2] acl: move check for Intention.DestinationName into Authorizer Follow up to https://github.com/hashicorp/consul/pull/10737#discussion_r680134445 Move the check for the Intention.DestinationName into the Authorizer to remove the need to check what kind of Authorizer is being used. It sounds like this check is only for legacy ACLs, so is probably just a safeguard . --- acl/policy_authorizer.go | 3 +++ agent/structs/intention.go | 9 --------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/acl/policy_authorizer.go b/acl/policy_authorizer.go index af52418c25..9985c8feb1 100644 --- a/acl/policy_authorizer.go +++ b/acl/policy_authorizer.go @@ -524,6 +524,9 @@ func (p *policyAuthorizer) IntentionRead(prefix string, _ *AuthorizerContext) En // IntentionWrite checks if writing (creating, updating, or deleting) of an // intention is allowed. func (p *policyAuthorizer) IntentionWrite(prefix string, _ *AuthorizerContext) EnforcementDecision { + if prefix == "" { + return Deny + } if prefix == "*" { return p.allAllowed(p.intentionRules, AccessWrite) } diff --git a/agent/structs/intention.go b/agent/structs/intention.go index 15c4017645..c2240c4149 100644 --- a/agent/structs/intention.go +++ b/agent/structs/intention.go @@ -322,16 +322,7 @@ func (ixn *Intention) CanRead(authz acl.Authorizer) bool { } func (ixn *Intention) CanWrite(authz acl.Authorizer) bool { - if authz == acl.ManageAll() { - return true - } var authzContext acl.AuthorizerContext - - // TODO: this line seems to require checking 'authz == acl.ManageAll()' above - if ixn.DestinationName == "" { - return false - } - ixn.FillAuthzContext(&authzContext, true) return authz.IntentionWrite(ixn.DestinationName, &authzContext) == acl.Allow } From c718de730b08ea58d79d57952d5233b2e629e375 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 4 Aug 2021 18:18:51 -0400 Subject: [PATCH 2/2] acl: remove special handling of services in txn_endpoint Follow up to: https://github.com/hashicorp/consul/pull/10738#discussion_r680190210 Previously we were passing an Authorizer that would always allow the operation, then later checking the authorization using vetServiceTxnOp. On the surface this seemed strange, but I think it was actually masking a bug as well. Over time `servicePreApply` was changed to add additional authorization for `service.Proxy.DestinationServiceName`, but because we were passing a nil Authorizer, that authorization was not handled on the txn_endpoint. `TxnServiceOp.FillAuthzContext` has some special handling in enterprise, so we need to make sure to continue to use that from the Txn endpoint. This commit removes the `vetServiceTxnOp` function, and passes in the `FillAuthzContext` function so that `servicePreApply` can be used by both the catalog and txn endpoints. This should be much less error prone and prevent bugs like this in the future. --- agent/consul/acl.go | 17 ----------------- agent/consul/catalog_endpoint.go | 6 +++--- agent/consul/txn_endpoint.go | 13 +------------ 3 files changed, 4 insertions(+), 32 deletions(-) diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 463c9087b5..0fa5976fd0 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -2252,23 +2252,6 @@ func vetNodeTxnOp(op *structs.TxnNodeOp, rule acl.Authorizer) error { return nil } -// vetServiceTxnOp applies the given ACL policy to a service transaction operation. -func vetServiceTxnOp(op *structs.TxnServiceOp, rule acl.Authorizer) error { - // Fast path if ACLs are not enabled. - if rule == nil { - return nil - } - - var authzContext acl.AuthorizerContext - op.FillAuthzContext(&authzContext) - - if rule.ServiceWrite(op.Service.Service, &authzContext) != acl.Allow { - return acl.ErrPermissionDenied - } - - return nil -} - // vetCheckTxnOp applies the given ACL policy to a check transaction operation. func vetCheckTxnOp(op *structs.TxnCheckOp, rule acl.Authorizer) error { // Fast path if ACLs are not enabled. diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index 444c83e8f2..eabe07e811 100644 --- a/agent/consul/catalog_endpoint.go +++ b/agent/consul/catalog_endpoint.go @@ -85,7 +85,7 @@ func nodePreApply(nodeName, nodeID string) error { return nil } -func servicePreApply(service *structs.NodeService, authz acl.Authorizer) error { +func servicePreApply(service *structs.NodeService, authz acl.Authorizer, authzCtxFill func(*acl.AuthorizerContext)) error { // Validate the service. This is in addition to the below since // the above just hasn't been moved over yet. We should move it over // in time. @@ -110,7 +110,7 @@ func servicePreApply(service *structs.NodeService, authz acl.Authorizer) error { } var authzContext acl.AuthorizerContext - service.FillAuthzContext(&authzContext) + authzCtxFill(&authzContext) // Apply the ACL policy if any. The 'consul' service is excluded // since it is managed automatically internally (that behavior @@ -175,7 +175,7 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error // Handle a service registration. if args.Service != nil { - if err := servicePreApply(args.Service, authz); err != nil { + if err := servicePreApply(args.Service, authz, args.Service.FillAuthzContext); err != nil { return err } } diff --git a/agent/consul/txn_endpoint.go b/agent/consul/txn_endpoint.go index 26ad1a32b2..2f0081ee59 100644 --- a/agent/consul/txn_endpoint.go +++ b/agent/consul/txn_endpoint.go @@ -81,18 +81,7 @@ func (t *Txn) preCheck(authorizer acl.Authorizer, ops structs.TxnOps) structs.Tx } service := &op.Service.Service - // 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(), - }) - break - } - - // Check that the token has permissions for the given operation. - if err := vetServiceTxnOp(op.Service, authorizer); err != nil { + if err := servicePreApply(service, authorizer, op.Service.FillAuthzContext); err != nil { errors = append(errors, &structs.TxnError{ OpIndex: i, What: err.Error(),