Output user-friendly name for anonymous token

[kisunji]

Description

When ACLs are enabled but an operator forgets to pass a token, a potential ACL error may refer to the anonymous token which appears in the logs as 00000000-0000-0000-0000-000000000002.

We should avoid the accessorID being a source of confusion for users by simply aliasing it to anonymous token whenever we output it in logs.

structs.ACLTokenAnonymousID -> acl.AnonymousTokenID refactor was done partly to resolve an import cycle but a broader objective is to fold more ACL-related domain objects into acl package.

Testing & Reproduction steps

• No behavioral changes

PR Checklist

• updated test coverage
• external facing docs updated
• not a security concern

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
consul	🔄 Building (Inspect)		Jan 5, 2023 at 2:55PM (UTC)
consul-ui-staging	🔄 Building (Inspect)		Jan 5, 2023 at 2:55PM (UTC)

[cthain]

This is looking good Chris. I just had a couple of minor comments. I like that you also did some housekeeping around comments and formatting 👍

[cthain]

What do you think about making the name of this func more generic and using a LUT to find the name for the token ID? I could envision that in the future we may want to use this mechanism for other well-known tokens. Something like:

// maybe this could have package scope?
wellKnownTokens := map[string]string{AnonymousTokenID: "anonymous token"}

func ResolveToken(accessorID string) string {
  if name, ok := wellKnownTokens[accessorID]; ok {
    return name
  }
  return accessorID
}

[kisunji]

I agree that this UX improvement would be useful for other well-known tokens but my understanding based on this doc is that the anonymous token is the only static one in our system.

Every other "well-known" token is configured and generated at runtime, making it harder to maintain a lookup table.

Not impossible, but I'd like to consider it out of scope for this PR.

[jkirschner-hashicorp]

I agree with deferring that generalization (YAGNI principle). Once we have >1 such token (if ever), we can make it more general as suggested.

[cthain]

This PR only migrates the anonymous token definition for ACLs.. I don't disagree with this at all but just want to point out that now we have ACL definitions in two places, acl/ and structs/.. Is it worth adding a note in the code indicating that we intend to move the rest of the ACL defs to the acl/ package at some point?

[kisunji]

Yeah it was originally an initiative started in core before Mark A left; the intent had always been to migrate things over. I'll add a todo

[kisunji]

As I was looking over the file I realized that there are some import cycles that might make a general migration difficult. Unsure if a general TODO would be helpful (or if it will grow stale and unaddressed). I think just an ad-hoc refactoring over time might be better?

[cthain]

Ya, I'm fine with whatever you think is best. I suspect that we will be thinking about and revisiting the ACLs system over the coming months and this is not likely to fall off our radar.

[cthain]

This change creates a lot of noise in this file, but I think this is the right thing to do given that the consul/acl package was already imported and these acl vars where hiding that package in the tests. I really like the disambiguation here.

[jkirschner-hashicorp]

@kisunji : My understanding is that most ACL permission denied error messages are centrally defined here:

consul/acl/errors.go

Lines 85 to 111 in ee2d47d

	func (e PermissionDeniedError) Error() string {
	var message strings.Builder
	message.WriteString(errPermissionDenied)
	// Type 4)
	if 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 token")
	} else {
	fmt.Fprintf(&message, ": token with AccessorID '%s'", e.Accessor)
	}
	fmt.Fprintf(&message, " lacks permission '%s:%s'", e.Resource, e.AccessLevel.String())
	if e.ResourceID.Name != "" {
	fmt.Fprintf(&message, " on %s", e.ResourceID.ToString())
	}
	return message.String()
	}

I don't see that error message currently modified in this PR.

And when we do modify that error message, we might want to take a different approach than is done here.

Currently, the error message is:

Permission denied: token with AccessorID 00000000-0000-0000-0000-000000000002 lacks permission node:write on node-1

I'm not sure that replacing the accessor ID with "accessor ID" is the clearest way to communicate what's happening in this case (though I agree it's a good solution for structured logging entries like those currently included in this PR). That would look like:

Permission denied: token with AccessorID 'anonymous token' lacks permission node:write on node-1

Perhaps it could instead be something like...

Permission denied: the anonymous token with AccessorID 00000000-0000-0000-0000-000000000002 lacks permission node:write on node-1. The anonymous token is used implicitly when a request does not specify a token.

Or ...

Permission denied: token with AccessorID "anonymous token" lacks permission node:write on node-1. The anonymous token is used implicitly when a request does not specify a token.

---

... I feel a bit weird about saying that the accessor ID is "anonymous token" when that isn't actually accurate, though I think it's preferable to the alternative in the case of structure logging (leaving as ID ....002). It's not like we want to encourage the lookup and modification of the anonymous token to have broader privileges ... What we want to communicate is: you didn't specify a token, but probably should. And I think making the accessor ID display as "anonymous token" is a reasonable way to accomplish that.