Refactor to make ACL errors more structured. (#12308)

* First phase of refactoring PermissionDeniedError

Add extended type PermissionDeniedByACLError that captures information
about the accessor, particular permission type and the object and name
of the thing being checked.

It may be worth folding the test and error return into a single helper
function, that can happen at a later date.

Signed-off-by: Mark Anderson <manderson@hashicorp.com>

.changelog/12308.txt

@@ -0,0 +1,3 @@
+```release-note:enhancement
+Refactor ACL denied error code and start improving error details
+```

acl/errors.go

@@ -61,18 +61,68 @@ func IsErrPermissionDenied(err error) bool {
 	return err != nil && strings.Contains(err.Error(), errPermissionDenied)
 }
 
+// Arguably this should be some sort of union type.
+// The usage of Cause and the rest of the fields is entirely disjoint.
+//
 type PermissionDeniedError struct {
 	Cause string
+
+	// Accessor contains information on the accessor used e.g. "token <GUID>"
+	Accessor string
+	// Resource (e.g. Service)
+	Resource Resource
+	// Access leve (e.g. Read)
+	AccessLevel AccessLevel
+	// e.g. "sidecar-proxy-1"
+	ResourceID ResourceDescriptor
 }
 
+// Initially we may not have attribution information; that will become more complete as we work this change through
+// There are generally three classes of errors
+// 1) Named entities without a context
+// 2) Unnamed entities with a context
+// 3) Completely context free checks (global permissions)
+// 4) Errors that only have a cause (for example bad token)
 func (e PermissionDeniedError) Error() string {
+	var message strings.Builder
+	message.WriteString(errPermissionDenied)
+
+	// Type 4)
 	if e.Cause != "" {
-		return errPermissionDenied + ": " + e.Cause
+		fmt.Fprintf(&message, ": %s", e.Cause)
+		return message.String()
 	}
-	return errPermissionDenied
+	// Should only be empty when default struct is used.
+	if e.Resource == "" {
+		return message.String()
+	}
+
+	if e.Accessor == "" {
+		message.WriteString(": provided accessor")
+	} else {
+		fmt.Fprintf(&message, ": accessor '%s'", e.Accessor)
+	}
+
+	fmt.Fprintf(&message, " lacks permission '%s:%s'", e.Resource, e.AccessLevel.String())
+
+	if e.ResourceID.Name != "" {
+		fmt.Fprintf(&message, " %s", e.ResourceID.ToString())
+	}
+	return message.String()
 }
 
 func PermissionDenied(msg string, args ...interface{}) PermissionDeniedError {
 	cause := fmt.Sprintf(msg, args...)
 	return PermissionDeniedError{Cause: cause}
 }
+
+// TODO Extract information from Authorizer
+func PermissionDeniedByACL(_ Authorizer, context *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) PermissionDeniedError {
+	desc := NewResourceDescriptor(resourceID, context)
+	return PermissionDeniedError{Accessor: "", Resource: resource, AccessLevel: accessLevel, ResourceID: desc}
+}
+
+func PermissionDeniedByACLUnnamed(_ Authorizer, context *AuthorizerContext, resource Resource, accessLevel AccessLevel) PermissionDeniedError {
+	desc := NewResourceDescriptor("", context)
+	return PermissionDeniedError{Accessor: "", Resource: resource, AccessLevel: accessLevel, ResourceID: desc}
+}

acl/errors_oss.go

@@ -0,0 +1,18 @@
+//go:build !consulent
+// +build !consulent
+
+package acl
+
+// In some sense we really want this to contain an EnterpriseMeta, but
+// this turns out to be a convenient place to hang helper functions off of.
+type ResourceDescriptor struct {
+	Name string
+}
+
+func NewResourceDescriptor(name string, _ *AuthorizerContext) ResourceDescriptor {
+	return ResourceDescriptor{Name: name}
+}
+
+func (od *ResourceDescriptor) ToString() string {
+	return od.Name
+}

acl/errors_test.go

@@ -0,0 +1,46 @@
+package acl
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestPermissionDeniedError(t *testing.T) {
+	type testCase struct {
+		err      PermissionDeniedError
+		expected string
+	}
+
+	testName := func(t testCase) string {
+		return t.expected
+	}
+
+	auth1 := mockAuthorizer{}
+
+	cases := []testCase{
+		{
+			err:      PermissionDeniedError{},
+			expected: "Permission denied",
+		},
+		{
+			err:      PermissionDeniedError{Cause: "simon says"},
+			expected: "Permission denied: simon says",
+		},
+		{
+			err:      PermissionDeniedByACL(&auth1, nil, ResourceService, AccessRead, "foobar"),
+			expected: "Permission denied: provided accessor lacks permission 'service:read' foobar",
+		},
+		{
+			err:      PermissionDeniedByACLUnnamed(&auth1, nil, ResourceService, AccessRead),
+			expected: "Permission denied: provided accessor lacks permission 'service:read'",
+		},
+	}
+
+	for _, tcase := range cases {
+		t.Run(testName(tcase), func(t *testing.T) {
+			require.Error(t, tcase.err)
+			require.Equal(t, tcase.expected, tcase.err.Error())
+		})
+	}
+}

agent/acl.go

@@ -44,16 +44,14 @@ func (a *Agent) vetServiceRegisterWithAuthorizer(authz acl.Authorizer, service *
 	// Vet the service itself.
 	service.FillAuthzContext(&authzContext)
 	if authz.ServiceWrite(service.Service, &authzContext) != acl.Allow {
-		return acl.PermissionDenied("Missing service:write on %s",
+		return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, service.Service)
-			structs.ServiceIDString(service.Service, &service.EnterpriseMeta))
 	}
 
 	// Vet any service that might be getting overwritten.
 	if existing := a.State.Service(service.CompoundServiceID()); existing != nil {
 		existing.FillAuthzContext(&authzContext)
 		if authz.ServiceWrite(existing.Service, &authzContext) != acl.Allow {
-			return acl.PermissionDenied("Missing service:write on %s",
+			return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, existing.Service)
-				structs.ServiceIDString(service.Service, &service.EnterpriseMeta))
 		}
 	}
 
@@ -62,8 +60,7 @@ func (a *Agent) vetServiceRegisterWithAuthorizer(authz acl.Authorizer, service *
 	if service.Kind == structs.ServiceKindConnectProxy {
 		service.FillAuthzContext(&authzContext)
 		if authz.ServiceWrite(service.Proxy.DestinationServiceName, &authzContext) != acl.Allow {
-			return acl.PermissionDenied("Missing service:write on %s",
+			return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, service.Proxy.DestinationServiceName)
-				structs.ServiceIDString(service.Proxy.DestinationServiceName, &service.EnterpriseMeta))
 		}
 	}
 
@@ -77,8 +74,7 @@ func (a *Agent) vetServiceUpdateWithAuthorizer(authz acl.Authorizer, serviceID s
 	if existing := a.State.Service(serviceID); existing != nil {
 		existing.FillAuthzContext(&authzContext)
 		if authz.ServiceWrite(existing.Service, &authzContext) != acl.Allow {
-			return acl.PermissionDenied("Missing service:write on %s",
+			return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, existing.Service)
-				structs.ServiceIDString(existing.Service, &existing.EnterpriseMeta))
 		}
 	} else {
 		// Take care if modifying this error message.
@@ -100,27 +96,26 @@ func (a *Agent) vetCheckRegisterWithAuthorizer(authz acl.Authorizer, check *stru
 	// Vet the check itself.
 	if len(check.ServiceName) > 0 {
 		if authz.ServiceWrite(check.ServiceName, &authzContext) != acl.Allow {
-			return acl.PermissionDenied("Missing service:write on %s",
+			return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, check.ServiceName)
-				structs.ServiceIDString(check.ServiceName, &check.EnterpriseMeta))
 		}
 	} else {
+		// N.B. Should this authzContext be derived from a.AgentEnterpriseMeta()
 		if authz.NodeWrite(a.config.NodeName, &authzContext) != acl.Allow {
-			return acl.PermissionDenied("Missing node:write on %s",
+			return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceNode, acl.AccessWrite, a.config.NodeName)
-				structs.NodeNameString(a.config.NodeName, a.AgentEnterpriseMeta()))
 		}
 	}
 
 	// Vet any check that might be getting overwritten.
 	if existing := a.State.Check(check.CompoundCheckID()); existing != nil {
 		if len(existing.ServiceName) > 0 {
+			// N.B. Should this authzContext be derived from existing.EnterpriseMeta?
 			if authz.ServiceWrite(existing.ServiceName, &authzContext) != acl.Allow {
-				return acl.PermissionDenied("Missing service:write on %s",
+				return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, existing.ServiceName)
-					structs.ServiceIDString(existing.ServiceName, &existing.EnterpriseMeta))
 			}
 		} else {
+			// N.B. Should this authzContext be derived from a.AgentEnterpriseMeta()
 			if authz.NodeWrite(a.config.NodeName, &authzContext) != acl.Allow {
-				return acl.PermissionDenied("Missing node:write on %s",
+				return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceNode, acl.AccessWrite, a.config.NodeName)
-					structs.NodeNameString(a.config.NodeName, a.AgentEnterpriseMeta()))
 			}
 		}
 	}
@@ -136,13 +131,11 @@ func (a *Agent) vetCheckUpdateWithAuthorizer(authz acl.Authorizer, checkID struc
 	if existing := a.State.Check(checkID); existing != nil {
 		if len(existing.ServiceName) > 0 {
 			if authz.ServiceWrite(existing.ServiceName, &authzContext) != acl.Allow {
-				return acl.PermissionDenied("Missing service:write on %s",
+				return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, existing.ServiceName)
-					structs.ServiceIDString(existing.ServiceName, &existing.EnterpriseMeta))
 			}
 		} else {
 			if authz.NodeWrite(a.config.NodeName, &authzContext) != acl.Allow {
-				return acl.PermissionDenied("Missing node:write on %s",
+				return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceNode, acl.AccessWrite, a.config.NodeName)
-					structs.NodeNameString(a.config.NodeName, a.AgentEnterpriseMeta()))
 			}
 		}
 	} else {

agent/agent_endpoint.go

@@ -622,7 +622,7 @@ func (s *HTTPHandlers) AgentJoin(resp http.ResponseWriter, req *http.Request) (i
 	var authzContext acl.AuthorizerContext
 	s.agent.AgentEnterpriseMeta().FillAuthzContext(&authzContext)
 	if authz.AgentWrite(s.agent.config.NodeName, &authzContext) != acl.Allow {
-		return nil, acl.ErrPermissionDenied
+		return nil, acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceAgent, acl.AccessWrite, s.agent.config.NodeName)
 	}
 
 	// Get the request partition and default to that of the agent.
@@ -686,7 +686,7 @@ func (s *HTTPHandlers) AgentForceLeave(resp http.ResponseWriter, req *http.Reque
 	}
 	// TODO(partitions): should this be possible in a partition?
 	if authz.OperatorWrite(nil) != acl.Allow {
-		return nil, acl.ErrPermissionDenied
+		return nil, acl.PermissionDeniedByACLUnnamed(authz, nil, acl.ResourceOperator, acl.AccessWrite)
 	}
 
 	// Get the request partition and default to that of the agent.
@@ -1008,7 +1008,7 @@ func (s *HTTPHandlers) AgentHealthServiceByID(resp http.ResponseWriter, req *htt
 
 	if service := s.agent.State.Service(sid); service != nil {
 		if authz.ServiceRead(service.Service, &authzContext) != acl.Allow {
-			return nil, acl.ErrPermissionDenied
+			return nil, acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessRead, service.Service)
 		}
 		code, status, healthChecks := agentHealthService(sid, s)
 		if returnTextPlain(req) {
@@ -1061,7 +1061,7 @@ func (s *HTTPHandlers) AgentHealthServiceByName(resp http.ResponseWriter, req *h
 	}
 
 	if authz.ServiceRead(serviceName, &authzContext) != acl.Allow {
-		return nil, acl.ErrPermissionDenied
+		return nil, acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessRead, serviceName)
 	}
 
 	if !s.validateRequestPartition(resp, &entMeta) {
@@ -1684,7 +1684,7 @@ func (s *HTTPHandlers) AgentHost(resp http.ResponseWriter, req *http.Request) (i
 
 	// TODO(partitions): should this be possible in a partition?
 	if authz.OperatorRead(nil) != acl.Allow {
-		return nil, acl.ErrPermissionDenied
+		return nil, acl.PermissionDeniedByACLUnnamed(authz, nil, acl.ResourceOperator, acl.AccessRead)
 	}
 
 	return debug.CollectHostInfo(), nil

agent/consul/operator_autopilot_endpoint.go

@@ -25,7 +25,7 @@ func (op *Operator) AutopilotGetConfiguration(args *structs.DCSpecificRequest, r
 		return err
 	}
 	if authz.OperatorRead(nil) != acl.Allow {
-		return acl.PermissionDenied("Missing operator:read permissions")
+		return acl.PermissionDeniedByACLUnnamed(authz, nil, acl.ResourceOperator, acl.AccessRead)
 	}
 
 	state := op.srv.fsm.State()
@@ -57,7 +57,7 @@ func (op *Operator) AutopilotSetConfiguration(args *structs.AutopilotSetConfigRe
 		return err
 	}
 	if authz.OperatorWrite(nil) != acl.Allow {
-		return acl.PermissionDenied("Missing operator:write permissions")
+		return acl.PermissionDeniedByACLUnnamed(authz, nil, acl.ResourceOperator, acl.AccessWrite)
 	}
 
 	// Apply the update
@@ -92,7 +92,7 @@ func (op *Operator) ServerHealth(args *structs.DCSpecificRequest, reply *structs
 		return err
 	}
 	if authz.OperatorRead(nil) != acl.Allow {
-		return acl.PermissionDenied("Missing operator:read permissions")
+		return acl.PermissionDeniedByACLUnnamed(authz, nil, acl.ResourceOperator, acl.AccessRead)
 	}
 
 	state := op.srv.autopilot.GetState()
@@ -159,7 +159,7 @@ func (op *Operator) AutopilotState(args *structs.DCSpecificRequest, reply *autop
 		return err
 	}
 	if authz.OperatorRead(nil) != acl.Allow {
-		return acl.PermissionDenied("Missing operator:read permissions")
+		return acl.PermissionDeniedByACLUnnamed(authz, nil, acl.ResourceOperator, acl.AccessRead)
 	}
 
 	state := op.srv.autopilot.GetState()