From 0ca9e875e2ef21c26462eb51b45e50cff3797b63 Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Wed, 4 Aug 2021 18:18:51 -0400
Subject: [PATCH] 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 463c9087b..0fa5976fd 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 444c83e8f..eabe07e81 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 26ad1a32b..2f0081ee5 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(),