Bulk acl message fixup oss (#12470)

* First pass for helper for bulk changes

Signed-off-by: Mark Anderson <[email protected]>

* Convert ACLRead and ACLWrite to new form

Signed-off-by: Mark Anderson <[email protected]>

* AgentRead and AgentWRite

Signed-off-by: Mark Anderson <[email protected]>

* Fix EventWrite

Signed-off-by: Mark Anderson <[email protected]>

* KeyRead, KeyWrite, KeyList

Signed-off-by: Mark Anderson <[email protected]>

* KeyRing

Signed-off-by: Mark Anderson <[email protected]>

* NodeRead NodeWrite

Signed-off-by: Mark Anderson <[email protected]>

* OperatorRead and OperatorWrite

Signed-off-by: Mark Anderson <[email protected]>

* PreparedQuery

Signed-off-by: Mark Anderson <[email protected]>

* Intention partial

Signed-off-by: Mark Anderson <[email protected]>

* Fix ServiceRead, Write ,etc

Signed-off-by: Mark Anderson <[email protected]>

* Error check ServiceRead?

Signed-off-by: Mark Anderson <[email protected]>

* Fix Sessionread/Write

Signed-off-by: Mark Anderson <[email protected]>

* Fixup snapshot ACL

Signed-off-by: Mark Anderson <[email protected]>

* Error fixups for txn

Signed-off-by: Mark Anderson <[email protected]>

* Add changelog

Signed-off-by: Mark Anderson <[email protected]>

* Fixup review comments

Signed-off-by: Mark Anderson <[email protected]>

.changelog/12470.txt

@@ -0,0 +1,3 @@
+```release-note:enhancement
+acl: Provide fuller detail in the error messsage when an ACL denies access.
+```

acl/authorizer.go

@@ -161,6 +161,279 @@ type Authorizer interface {
 
 	// Embedded Interface for Consul Enterprise specific ACL enforcement
 	enterpriseAuthorizer
+
+	// ToAllowAuthorizer is needed until we can use ResolveResult in all the places this interface is used.
+	ToAllowAuthorizer() AllowAuthorizer
+}
+
+// AllowAuthorizer is a wrapper to expose the *Allowed methods.
+// This and the ToAllowAuthorizer function exist to tide us over until the ResolveResult struct
+// is moved into acl.
+type AllowAuthorizer struct {
+	Authorizer
+}
+
+// ACLReadAllowed checks for permission to list all the ACLs
+func (a AllowAuthorizer) ACLReadAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.ACLRead(ctx) != Allow {
+		return PermissionDeniedByACLUnnamed(a, ctx, ResourceACL, AccessRead)
+	}
+	return nil
+}
+
+// ACLWriteAllowed checks for permission to manipulate ACLs
+func (a AllowAuthorizer) ACLWriteAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.ACLWrite(ctx) != Allow {
+		return PermissionDeniedByACLUnnamed(a, ctx, ResourceACL, AccessWrite)
+	}
+	return nil
+}
+
+// AgentReadAllowed checks for permission to read from agent endpoints for a
+// given node.
+func (a AllowAuthorizer) AgentReadAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.AgentRead(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceAgent, AccessRead, name)
+	}
+	return nil
+}
+
+// AgentWriteAllowed checks for permission to make changes via agent endpoints
+// for a given node.
+func (a AllowAuthorizer) AgentWriteAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.AgentWrite(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceAgent, AccessWrite, name)
+	}
+	return nil
+}
+
+// EventReadAllowed determines if a specific event can be queried.
+func (a AllowAuthorizer) EventReadAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.EventRead(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceEvent, AccessRead, name)
+	}
+	return nil
+}
+
+// EventWriteAllowed determines if a specific event may be fired.
+func (a AllowAuthorizer) EventWriteAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.EventWrite(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceEvent, AccessWrite, name)
+	}
+	return nil
+}
+
+// IntentionDefaultAllowAllowed determines the default authorized behavior
+// when no intentions match a Connect request.
+func (a AllowAuthorizer) IntentionDefaultAllowAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.IntentionDefaultAllow(ctx) != Allow {
+		// This is a bit nuanced, in that this isn't set by a rule, but inherited globally
+		// TODO(acl-error-enhancements) revisit when we have full accessor info
+		return PermissionDeniedError{Cause: "Denied by intention default"}
+	}
+	return nil
+}
+
+// IntentionReadAllowed determines if a specific intention can be read.
+func (a AllowAuthorizer) IntentionReadAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.IntentionRead(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceIntention, AccessRead, name)
+	}
+	return nil
+}
+
+// IntentionWriteAllowed determines if a specific intention can be
+// created, modified, or deleted.
+func (a AllowAuthorizer) IntentionWriteAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.IntentionWrite(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceIntention, AccessWrite, name)
+	}
+	return nil
+}
+
+// KeyListAllowed checks for permission to list keys under a prefix
+func (a AllowAuthorizer) KeyListAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.KeyList(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceKey, AccessList, name)
+	}
+	return nil
+}
+
+// KeyReadAllowed checks for permission to read a given key
+func (a AllowAuthorizer) KeyReadAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.KeyRead(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceKey, AccessRead, name)
+	}
+	return nil
+}
+
+// KeyWriteAllowed checks for permission to write a given key
+func (a AllowAuthorizer) KeyWriteAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.KeyWrite(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceKey, AccessWrite, name)
+	}
+	return nil
+}
+
+// KeyWritePrefixAllowed checks for permission to write to an
+// entire key prefix. This means there must be no sub-policies
+// that deny a write.
+func (a AllowAuthorizer) KeyWritePrefixAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.KeyWritePrefix(name, ctx) != Allow {
+		// TODO(acl-error-enhancements) revisit this message; we may need to do some extra plumbing inside of KeyWritePrefix to
+		// return properly detailed information.
+		return PermissionDeniedByACL(a, ctx, ResourceKey, AccessWrite, name)
+	}
+	return nil
+}
+
+// KeyringReadAllowed determines if the encryption keyring used in
+// the gossip layer can be read.
+func (a AllowAuthorizer) KeyringReadAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.KeyringRead(ctx) != Allow {
+		return PermissionDeniedByACLUnnamed(a, ctx, ResourceKeyring, AccessRead)
+	}
+	return nil
+}
+
+// KeyringWriteAllowed determines if the keyring can be manipulated
+func (a AllowAuthorizer) KeyringWriteAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.KeyringWrite(ctx) != Allow {
+		return PermissionDeniedByACLUnnamed(a, ctx, ResourceKeyring, AccessWrite)
+	}
+	return nil
+}
+
+// MeshReadAllowed determines if the read-only Consul mesh functions
+// can be used.
+func (a AllowAuthorizer) MeshReadAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.MeshRead(ctx) != Allow {
+		return PermissionDeniedByACLUnnamed(a, ctx, ResourceMesh, AccessRead)
+	}
+	return nil
+}
+
+// MeshWriteAllowed determines if the state-changing Consul mesh
+// functions can be used.
+func (a AllowAuthorizer) MeshWriteAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.MeshWrite(ctx) != Allow {
+		return PermissionDeniedByACLUnnamed(a, ctx, ResourceMesh, AccessWrite)
+	}
+	return nil
+}
+
+// NodeReadAllowed checks for permission to read (discover) a given node.
+func (a AllowAuthorizer) NodeReadAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.NodeRead(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceNode, AccessRead, name)
+	}
+	return nil
+}
+
+// NodeReadAllAllowed checks for permission to read (discover) all nodes.
+func (a AllowAuthorizer) NodeReadAllAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.NodeReadAll(ctx) != Allow {
+		// This is only used to gate certain UI functions right now (e.g metrics)
+		return PermissionDeniedByACL(a, ctx, ResourceNode, AccessRead, "all nodes")
+	}
+	return nil
+}
+
+// NodeWriteAllowed checks for permission to create or update (register) a
+// given node.
+func (a AllowAuthorizer) NodeWriteAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.NodeWrite(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceNode, AccessWrite, name)
+	}
+	return nil
+}
+
+// OperatorReadAllowed determines if the read-only Consul operator functions
+// can be used.
+func (a AllowAuthorizer) OperatorReadAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.OperatorRead(ctx) != Allow {
+		return PermissionDeniedByACLUnnamed(a, ctx, ResourceOperator, AccessRead)
+	}
+	return nil
+}
+
+// OperatorWriteAllowed determines if the state-changing Consul operator
+// functions can be used.
+func (a AllowAuthorizer) OperatorWriteAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.OperatorWrite(ctx) != Allow {
+		return PermissionDeniedByACLUnnamed(a, ctx, ResourceOperator, AccessWrite)
+	}
+	return nil
+}
+
+// PreparedQueryReadAllowed determines if a specific prepared query can be read
+// to show its contents (this is not used for execution).
+func (a AllowAuthorizer) PreparedQueryReadAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.PreparedQueryRead(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceQuery, AccessRead, name)
+	}
+	return nil
+}
+
+// PreparedQueryWriteAllowed determines if a specific prepared query can be
+// created, modified, or deleted.
+func (a AllowAuthorizer) PreparedQueryWriteAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.PreparedQueryWrite(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceQuery, AccessWrite, name)
+	}
+	return nil
+}
+
+// ServiceReadAllowed checks for permission to read a given service
+func (a AllowAuthorizer) ServiceReadAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.ServiceRead(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceService, AccessRead, name)
+	}
+	return nil
+}
+
+// ServiceReadAllAllowed checks for permission to read all services
+func (a AllowAuthorizer) ServiceReadAllAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.ServiceReadAll(ctx) != Allow {
+		// This is only used to gate certain UI functions right now (e.g metrics)
+		return PermissionDeniedByACL(a, ctx, ResourceService, AccessRead, "all services") // read
+	}
+	return nil
+}
+
+// ServiceWriteAllowed checks for permission to create or update a given
+// service
+func (a AllowAuthorizer) ServiceWriteAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.ServiceWrite(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceService, AccessWrite, name)
+	}
+	return nil
+}
+
+// SessionReadAllowed checks for permission to read sessions for a given node.
+func (a AllowAuthorizer) SessionReadAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.SessionRead(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceSession, AccessRead, name)
+	}
+	return nil
+}
+
+// SessionWriteAllowed checks for permission to create sessions for a given
+// node.
+func (a AllowAuthorizer) SessionWriteAllowed(name string, ctx *AuthorizerContext) error {
+	if a.Authorizer.SessionWrite(name, ctx) != Allow {
+		return PermissionDeniedByACL(a, ctx, ResourceSession, AccessWrite, name)
+	}
+	return nil
+}
+
+// SnapshotAllowed checks for permission to take and restore snapshots.
+func (a AllowAuthorizer) SnapshotAllowed(ctx *AuthorizerContext) error {
+	if a.Authorizer.Snapshot(ctx) != Allow {
+		// Implementation of this currently just checks acl write
+		return PermissionDeniedByACLUnnamed(a, ctx, ResourceACL, AccessWrite)
+	}
+	return nil
 }
 
 func Enforce(authz Authorizer, rsc Resource, segment string, access string, ctx *AuthorizerContext) (EnforcementDecision, error) {

acl/authorizer_test.go

@@ -204,6 +204,10 @@ func (m *mockAuthorizer) Snapshot(ctx *AuthorizerContext) EnforcementDecision {
 	return ret.Get(0).(EnforcementDecision)
 }
 
+func (p *mockAuthorizer) ToAllowAuthorizer() AllowAuthorizer {
+	return AllowAuthorizer{Authorizer: p}
+}
+
 func TestACL_Enforce(t *testing.T) {
 	type testCase struct {
 		method   string

acl/chained_authorizer.go

@@ -256,3 +256,7 @@ func (c *ChainedAuthorizer) Snapshot(entCtx *AuthorizerContext) EnforcementDecis
 		return authz.Snapshot(entCtx)
 	})
 }
+
+func (c *ChainedAuthorizer) ToAllowAuthorizer() AllowAuthorizer {
+	return AllowAuthorizer{Authorizer: c}
+}

acl/chained_authorizer_test.go

@@ -99,6 +99,10 @@ func (authz testAuthorizer) Snapshot(*AuthorizerContext) EnforcementDecision {
 	return EnforcementDecision(authz)
 }
 
+func (authz testAuthorizer) ToAllowAuthorizer() AllowAuthorizer {
+	return AllowAuthorizer{Authorizer: &authz}
+}
+
 func TestChainedAuthorizer(t *testing.T) {
 	t.Run("No Authorizers", func(t *testing.T) {
 		authz := NewChainedAuthorizer([]Authorizer{})

acl/errors.go

@@ -106,7 +106,7 @@ func (e PermissionDeniedError) Error() string {
 	fmt.Fprintf(&message, " lacks permission '%s:%s'", e.Resource, e.AccessLevel.String())
 
 	if e.ResourceID.Name != "" {
-		fmt.Fprintf(&message, " %s", e.ResourceID.ToString())
+		fmt.Fprintf(&message, " on %s", e.ResourceID.ToString())
 	}
 	return message.String()
 }

acl/errors_test.go

@@ -29,7 +29,7 @@ func TestPermissionDeniedError(t *testing.T) {
 		},
 		{
 			err:      PermissionDeniedByACL(&auth1, nil, ResourceService, AccessRead, "foobar"),
-			expected: "Permission denied: provided accessor lacks permission 'service:read' foobar",
+			expected: "Permission denied: provided accessor lacks permission 'service:read' on foobar",
 		},
 		{
 			err:      PermissionDeniedByACLUnnamed(&auth1, nil, ResourceService, AccessRead),

acl/policy_authorizer.go

@@ -787,3 +787,7 @@ func (p *policyAuthorizer) SessionWrite(node string, _ *AuthorizerContext) Enfor
 	}
 	return Default
 }
+
+func (p *policyAuthorizer) ToAllowAuthorizer() AllowAuthorizer {
+	return AllowAuthorizer{Authorizer: p}
+}

acl/static_authorizer.go

@@ -240,6 +240,10 @@ func (s *staticAuthorizer) Snapshot(_ *AuthorizerContext) EnforcementDecision {
 	return Deny
 }
 
+func (s *staticAuthorizer) ToAllowAuthorizer() AllowAuthorizer {
+	return AllowAuthorizer{Authorizer: s}
+}
+
 // AllowAll returns an Authorizer that allows all operations
 func AllowAll() Authorizer {
 	return allowAll

acl/testing.go

@@ -0,0 +1,47 @@
+package acl
+
+import (
+	"github.com/stretchr/testify/require"
+	"regexp"
+	"testing"
+)
+
+func RequirePermissionDeniedError(t testing.TB, err error, _ Authorizer, _ *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) {
+	t.Helper()
+	if err == nil {
+		t.Fatal("An error is expected but got nil.")
+	}
+	if v, ok := err.(PermissionDeniedError); ok {
+		require.Equal(t, v.Resource, resource)
+		require.Equal(t, v.AccessLevel, accessLevel)
+		require.Equal(t, v.ResourceID.Name, resourceID)
+	} else {
+		t.Fatalf("Expected a permission denied error got %T %vp", err, err)
+	}
+}
+
+func RequirePermissionDeniedMessage(t testing.TB, msg string, auth Authorizer, _ *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) {
+	require.NotEmpty(t, msg, "expected non-empty error message")
+
+	var resourceIDFound string
+	if auth == nil {
+		expr := "^Permission denied" + `: provided accessor lacks permission '(\S*):(\S*)' on (.*)\s*$`
+		re, _ := regexp.Compile(expr)
+		matched := re.FindStringSubmatch(msg)
+
+		require.Equal(t, string(resource), matched[1], "resource")
+		require.Equal(t, accessLevel.String(), matched[2], "access level")
+		resourceIDFound = matched[3]
+	} else {
+		expr := "^Permission denied" + `: accessor '(\S*)' lacks permission '(\S*):(\S*)' on (.*)\s*$`
+		re, _ := regexp.Compile(expr)
+		matched := re.FindStringSubmatch(msg)
+
+		require.Equal(t, auth, matched[1], "auth")
+		require.Equal(t, string(resource), matched[2], "resource")
+		require.Equal(t, accessLevel.String(), matched[3], "access level")
+		resourceIDFound = matched[4]
+	}
+	// AuthorizerContext information should be checked here
+	require.Contains(t, resourceIDFound, resourceID, "resource id")
+}