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()