Browse Source

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>
pull/12630/head
Mark Anderson 3 years ago committed by GitHub
parent
commit
1a16f7ee70
  1. 3
      .changelog/12308.txt
  2. 54
      acl/errors.go
  3. 18
      acl/errors_oss.go
  4. 46
      acl/errors_test.go
  5. 33
      agent/acl.go
  6. 10
      agent/agent_endpoint.go
  7. 8
      agent/consul/operator_autopilot_endpoint.go

3
.changelog/12308.txt

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

54
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()
}
// 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 errPermissionDenied
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}
}

18
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
}

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

33
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 {

10
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

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

Loading…
Cancel
Save