From fa95afdcf635ee36f5148f1e565d3e0129ea4c2b Mon Sep 17 00:00:00 2001 From: Mark Anderson Date: Fri, 11 Feb 2022 12:53:23 -0800 Subject: [PATCH] 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 --- .changelog/12308.txt | 3 ++ acl/errors.go | 54 ++++++++++++++++++++- acl/errors_oss.go | 18 +++++++ acl/errors_test.go | 46 ++++++++++++++++++ agent/acl.go | 33 +++++-------- agent/agent_endpoint.go | 10 ++-- agent/consul/operator_autopilot_endpoint.go | 8 +-- 7 files changed, 141 insertions(+), 31 deletions(-) create mode 100644 .changelog/12308.txt create mode 100644 acl/errors_oss.go create mode 100644 acl/errors_test.go diff --git a/.changelog/12308.txt b/.changelog/12308.txt new file mode 100644 index 000000000..bbb6ded2b --- /dev/null +++ b/.changelog/12308.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +Refactor ACL denied error code and start improving error details +``` \ No newline at end of file diff --git a/acl/errors.go b/acl/errors.go index 2bb05c859..4e0290f84 100644 --- a/acl/errors.go +++ b/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 " + 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} +} diff --git a/acl/errors_oss.go b/acl/errors_oss.go new file mode 100644 index 000000000..9d605b34e --- /dev/null +++ b/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 +} diff --git a/acl/errors_test.go b/acl/errors_test.go new file mode 100644 index 000000000..ea377febe --- /dev/null +++ b/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()) + }) + } +} diff --git a/agent/acl.go b/agent/acl.go index 0c0334a2f..94ec536e3 100644 --- a/agent/acl.go +++ b/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", - structs.ServiceIDString(service.Service, &service.EnterpriseMeta)) + return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, service.Service) } // 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", - structs.ServiceIDString(service.Service, &service.EnterpriseMeta)) + return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, existing.Service) } } @@ -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", - structs.ServiceIDString(service.Proxy.DestinationServiceName, &service.EnterpriseMeta)) + return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, service.Proxy.DestinationServiceName) } } @@ -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", - structs.ServiceIDString(existing.Service, &existing.EnterpriseMeta)) + return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, existing.Service) } } 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", - structs.ServiceIDString(check.ServiceName, &check.EnterpriseMeta)) + return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, check.ServiceName) } } 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", - structs.NodeNameString(a.config.NodeName, a.AgentEnterpriseMeta())) + return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceNode, acl.AccessWrite, a.config.NodeName) } } // 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", - structs.ServiceIDString(existing.ServiceName, &existing.EnterpriseMeta)) + return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, existing.ServiceName) } } 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", - structs.NodeNameString(a.config.NodeName, a.AgentEnterpriseMeta())) + return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceNode, acl.AccessWrite, a.config.NodeName) } } } @@ -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", - structs.ServiceIDString(existing.ServiceName, &existing.EnterpriseMeta)) + return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceService, acl.AccessWrite, existing.ServiceName) } } else { if authz.NodeWrite(a.config.NodeName, &authzContext) != acl.Allow { - return acl.PermissionDenied("Missing node:write on %s", - structs.NodeNameString(a.config.NodeName, a.AgentEnterpriseMeta())) + return acl.PermissionDeniedByACL(authz, &authzContext, acl.ResourceNode, acl.AccessWrite, a.config.NodeName) } } } else { diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 2de8c53c4..1b5804cd1 100644 --- a/agent/agent_endpoint.go +++ b/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 diff --git a/agent/consul/operator_autopilot_endpoint.go b/agent/consul/operator_autopilot_endpoint.go index 2ee85bb06..ccda62a5e 100644 --- a/agent/consul/operator_autopilot_endpoint.go +++ b/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()