Bulk acl message fixup oss #12470

markan · 2022-03-01T01:04:34Z

This does a bulk conversion for improved Permission Denied errors.

Part of issue #12240

The intent is to incrementally move us towards higher resolution errors. While this is a relatively large PR. This can be read by individual commit, which should hopefully be relatively digestible. I've tried to split commits so semantic changes and mechanical changes are done separately in the hope that it will be easier to review.

There are a few areas we will need to pick up in later issues

Capturing identity information #12481
Reworking ResolveResult and AllowAuthorizer into one structure, and working the final product all the way through the system. This probably involves moving more stuff into acl to simplify things. This may be one as part of the capture of identity information #11337
reworking the catalog information to return errors #12241
thinking about how to give better hints on filters.
Cleaning up some of places where we directly create PermissionDenied structs to include detailed information

markan · 2022-03-01T18:01:30Z

acl/authorizer.go

+
+	// ToAllowAuthorizer is needed until we can use ResolveResult in all the places this interface is used.
+	ToAllowAuthorizer() AllowAuthorizer
+}


Eventually this will be unified with the ACLResolveResult structure, but want to take that on as a separate line item. #12481

.changelog/12470.txt

acl/authorizer.go

rboyer · 2022-03-07T22:16:45Z

acl/authorizer.go

+// 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 {


Is the gist about this that we only ever make blah(ctx) != Allow conditionals in the code, so unify those behind a better call structure?

Yes. That is by far the most common pattern we see.

acl/authorizer.go

rboyer · 2022-03-07T22:29:53Z

acl/authorizer.go

@@ -161,6 +162,280 @@ 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


Does this need to be a method? Do any of the implementation of this interface do more than &AllowAuthorizer{a}?

If they are all the same there may be a benefit towards just having func acl.NewAllowAuthorizer(Authorizer) instead

This is a transitional method; in upcoming PRs more functionality will be loaded onto it (in particular capturing authorization source information), but eventually I'd like to move a bunch of stuff into the ACL directory and hopefully remove this entirely.

acl/errors.go

rboyer · 2022-03-07T22:38:27Z

acl/authorizer.go

+}
+
+// ACLReadAllowed checks for permission to list all the ACLs
+func (a AllowAuthorizer) ACLReadAllowed(ctx *AuthorizerContext) error {


Followup: do these even need to be methods, or could they be a suite of package functions with a leading argument Authorizer so your in-endpoint checks would be:

if acl.ACLReadAllowed(authz, ctx) { ... }

which is terser than what shows up down below.

I'm curious about this too; it feels like added indirection and I think RB's suggested functions read better

The intent is to merge AllowAuthorizer and ACLResolveResult from consul/acl.go in a future PR. These would be methods on the joined class

agent/consul/connect_ca_endpoint.go

rboyer · 2022-03-07T22:45:06Z

agent/consul/health_endpoint.go

@@ -220,6 +220,8 @@ func (h *Health) ServiceNodes(args *structs.ServiceSpecificRequest, reply *struc
 	// If we're doing a connect or ingress query, we need read access to the service
 	// we're trying to find proxies for, so check that.
 	if args.Connect || args.Ingress {
+		// TODO: ACL Error improvements; can this be improved? What happens if we returned an error here?
+		// Is this similar to filters where we might want to return a hint?
 		if authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow {


The catalog and health endpoints (and parts of agent) expect the filter style of ACL enforcement (200s instead of 403s).

rboyer · 2022-03-07T22:46:12Z

agent/consul/internal_endpoint.go

@@ -464,16 +464,16 @@ func (m *Internal) KeyringOperation(
 	}
 	switch args.Operation {
 	case structs.KeyringList:
-		if authz.KeyringRead(nil) != acl.Allow {
-			return fmt.Errorf("Reading keyring denied by ACLs")


For the record this error message is now changing.

rboyer · 2022-03-07T22:46:16Z

agent/consul/internal_endpoint.go

 		}
 	case structs.KeyringInstall:
 		fallthrough
 	case structs.KeyringUse:
 		fallthrough
 	case structs.KeyringRemove:
-		if authz.KeyringWrite(nil) != acl.Allow {
-			return fmt.Errorf("Modifying keyring denied due to ACLs")


For the record this error message is now changing.

rboyer · 2022-03-07T22:49:25Z

agent/consul/txn_endpoint_test.go

@@ -554,57 +554,64 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
 	}

 	// Verify the transaction's return value.
-	var expected structs.TxnResponse
+	var outPos int


Can you explain the intent here please?

The broad purpose here is to perform a slightly stricter test on what we return; in particular make sure we have the right resource and permission information.

The original code attempted to build an identical result to the expected return value from the API call and directly compare them, but that seemed impractical given the new structure. So I altered this into an element by element comparison inline..

agent/xds/delta_test.go

rboyer

I left a few comments.

acl/errors.go

agent/acl.go

kisunji · 2022-03-08T18:52:52Z

acl/authorizer.go

+}
+
+// ACLReadAllowed checks for permission to list all the ACLs
+func (a AllowAuthorizer) ACLReadAllowed(ctx *AuthorizerContext) error {


I'm curious about this too; it feels like added indirection and I think RB's suggested functions read better

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

hc-github-team-consul-core · 2022-03-11T02:49:18Z

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/605613.

github-actions bot added theme/acls ACL and token generation theme/envoy/xds Related to Envoy support labels Mar 1, 2022

vercel bot temporarily deployed to Preview – consul-ui-staging March 1, 2022 01:15 Inactive

vercel bot temporarily deployed to Preview – consul March 1, 2022 01:15 Inactive

markan commented Mar 1, 2022

View reviewed changes

markan linked an issue Mar 1, 2022 that may be closed by this pull request

Go through permission checks and rework to generated detailed information. #12240

Closed