acl: improve the ResolveToken interface #11690
Labels
theme/acls
ACL and token generation
theme/internal-cleanup
Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization
The interface for resolving a token is a bit fragmented. The following methods exist:
Server.ResolveTokenAndDefaultMeta
Client.ResolveTokenAndDefaultMeta
- duplicate ofServer.ResolveTokenAndDefaultMeta
Server.ResolveTokenToIdentity
- mostly the same underlying logic as above, but different args and return valueServer.ResolveToken
- basically the same with fewer args and return values, legacy call that has not been cleaned up yetClient.ResolveTokenToIdentity
- duplicate ofServer.ResolveTokenToIdentity
ACLResolver.ResolveTokenToIdentityAndAuthorizer
- called by the aboveACLResolver.ResolveTokenToIdentity
similar to the aboveServer.ResolveIdentityFromToken
- used internally byACLResolver
, but occasionally not, see acl: extract a backend type for the ACLResolverBackend #11221The large number of similar methods makes it difficult to understand which one to use, and increase the potential for bugs when the wrong one is called.
The "main" call is supposed to be
{Server, Client}.ResolveTokenAndDefaultMeta
. That method has a few problems:entMeta
andauthzContext
args are things to "fill". There are other ways to fill these two things, and it's not really obvious from the function signature how this "fill" worksacl.Identity
from the return value, which is why we have the additionalResolveTokenToIdentity
calls. In practice all we really need from the Identity is an accessor ID. The rest of theacl.Identity
methods are internal to the ACL system.acl.Authorizer
is a nice interface, but all of its methods return anEnforcementDecision
instead of an error. This makes it difficult to solve problems like Get Token call to API, returns 403 rather than 404 if token not found #8428 and Reading non-existing policy returns "ACL Not found" in a 403 Forbidden #9933. We'd like to be able to return more information to authenticated, but not authorized requests, but we can't do that from a central place right now because of this interface.Proposal
To address these problems I propose we replace all of these methods with a single new method on
ACLResolver
.The
agent.delegate
interface can replace its two methods with a singleACLResolver() ACLResolver
for getting access to the resolver. That alone removes the duplication between Client and Server.The remaining problems can be addressed by changing the interface to something like this:
All requests should already embed either
WriteRequest
, orQueryOptions
, and both of those already implementTokenSecret
. Most requests should already have anEnterpriseMeta
.EnterpriseMeta.Merge
is effectively what we want forSetEnterpriseIdentity
, so we could either renameMerge
, or make it an alias. Any ones that do already embedEnterpriseMeta
can easily embed a struct with a no-op implementation of that method.The returned
Authorizer
would be a struct (not an interface), with methods similar to the existingacl.Authorizer
(https://pkg.go.dev/github.com/hashicorp/consul@v1.10.4/acl#Authorizer). The difference would be that:string
and anAuthContext
, we would accept an interface that accepts an identifier of a resource. Both request structs and data structs (items in responses) can then implement this interface to communicate the correct identity (including EnterpriseMeta). Any authorization that doesn't require the AuthorizerContext (ex:OperatorRead/Write
, orKeyRead/Write
) would accept no args.EnforcementDecision
we would return a typederror
. That error would allow API and RPC endpoints to return the appropriate amount of information based on if the request was authenticated or not.AccessorID() string
method for returning the accessor ID, so that we don't need duplicate methods for exposing the accessor ID.The text was updated successfully, but these errors were encountered: