hashicorp / hashicorp/consul

acl: improve the ResolveToken interface

Open
#11,690 1 comment 4 reactions 0 assignees View on GitHub
theme/acls theme/internal-cleanup
Dominant language
Go
Stars
30.1k
Forks
4.6k
Avg merge
1d 18h
Merged PRs (30d)
39

Description

The interface for resolving a token is a bit fragmented. The following methods exist:
* `Server.ResolveTokenAndDefaultMeta`
* `Client.ResolveTokenAndDefaultMeta` - duplicate of `Server.ResolveTokenAndDefaultMeta`
* `Server.ResolveTokenToIdentity` - mostly the same underlying logic as above, but different args and return value
* `Server.ResolveToken` - basically the same with fewer args and return values, legacy call that has not been cleaned up yet
* `Client.ResolveTokenToIdentity` - duplicate of `Server.ResolveTokenToIdentity`
* `ACLResolver.ResolveTokenToIdentityAndAuthorizer` - called by the above
* `ACLResolver.ResolveTokenToIdentity` similar to the above
* `Server.ResolveIdentityFromToken` - used internally by `ACLResolver`, but occasionally not, see #11221

The 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:
1. the `entMeta` and `authzContext` 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" works
2. there is no way to access the `acl.Identity` from the return value, which is why we have the additional `ResolveTokenToIdentity` calls. In practice all we really need from the Identity is an accessor ID. The rest of the `acl.Identity` methods are internal to the ACL system.
3. the returned `acl.Authorizer` is a nice interface, but all of its methods return an `EnforcementDecision` instead of an error. This makes it difficult to solve problems like #8428 and #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 single `ACLResolver() 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:

```go
func (a *ACLResolver) ResolveToken(req Request) (Authorizer, error) {
...
}

type Request interface {
TokenSecret() string
SetEnterpriseIdentity(structs.EnterpriseMeta)
}
```

All requests should already embed either `WriteRequest`, or `QueryOptions`, and both of those already implement `TokenSecret`. Most requests should already have an `EnterpriseMeta`. `EnterpriseMeta.Merge` is effectively what we want for `SetEnterpriseIdentity`, so we could either rename `Merge`, or make it an alias. Any ones that do already embed `EnterpriseMeta` 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 existing `acl.Authorizer` (https://pkg.go.dev/github.com/hashicorp/consul@v1.10.4/acl#Authorizer). The difference would be that:
* Instead of accepting a `string` and an `AuthContext` , 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` , or `KeyRead/Write`) would accept no args.
* instead of returning an `EnforcementDecision` we would return a typed `error`. That error would allow API and RPC endpoints to return the appropriate amount of information based on if the request was authenticated or not.
* it should have an `AccessorID() string` method for returning the accessor ID, so that we don't need duplicate methods for exposing the accessor ID.

Contributor guide

Open the contributing guide

Research direction

Start by tracing the listed Server, Client, ACLResolver, and agent.delegate methods, then compare them with the existing acl.Authorizer interface linked in the proposal. Determine the affected request types and EnterpriseMeta implementations before assessing whether the proposed single ACLResolver.ResolveToken method can replace the duplicates. Done would require an agreed interface design and corresponding migration, but no files or tests are named.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
authorization, backend-api-design, security
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.