Merge pull request #12166 from hashicorp/dnephin/acl-resolve-token-2

acl: remove ResolveTokenToIdentity

.changelog/12166.txt

@@ -0,0 +1,3 @@
+```release-note:deprecation
+acl: The `consul.acl.ResolveTokenToIdentity` metric is no longer reported. The values that were previous reported as part of this metric will now be part of the `consul.acl.ResolveToken` metric.
+```

agent/acl.go

@@ -15,7 +15,7 @@ import (
 // critical purposes, such as logging. Therefore we interpret all errors as empty-string
 // so we can safely log it without handling non-critical errors at the usage site.
 func (a *Agent) aclAccessorID(secretID string) string {
-	ident, err := a.delegate.ResolveTokenToIdentity(secretID)
+	ident, err := a.delegate.ResolveTokenAndDefaultMeta(secretID, nil, nil)
 	if acl.IsErrNotFound(err) {
 		return ""
 	}
@@ -23,10 +23,7 @@ func (a *Agent) aclAccessorID(secretID string) string {
 		a.logger.Debug("non-critical error resolving acl token accessor for logging", "error", err)
 		return ""
 	}
-	if ident == nil {
+	return ident.AccessorID()
-		return ""
-	}
-	return ident.ID()
 }
 
 // vetServiceRegister makes sure the service registration action is allowed by
@@ -174,7 +171,7 @@ func (a *Agent) filterMembers(token string, members *[]serf.Member) error {
 		if authz.NodeRead(node, &authzContext) == acl.Allow {
 			continue
 		}
-		accessorID := a.aclAccessorID(token)
+		accessorID := authz.AccessorID()
 		a.logger.Debug("dropping node from result due to ACLs", "node", node, "accessorID", accessorID)
 		m = append(m[:i], m[i+1:]...)
 		i--

agent/acl_test.go

@@ -94,18 +94,10 @@ func (a *TestACLAgent) ResolveTokenToIdentityAndAuthorizer(secretID string) (str
 	return a.resolveAuthzFn(secretID)
 }
 
-func (a *TestACLAgent) ResolveTokenToIdentity(secretID string) (structs.ACLIdentity, error) {
+func (a *TestACLAgent) ResolveTokenAndDefaultMeta(secretID string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (consul.ACLResolveResult, error) {
-	if a.resolveIdentFn == nil {
-		return nil, fmt.Errorf("ResolveTokenToIdentity call is unexpected - no ident resolver callback set")
-	}
-
-	return a.resolveIdentFn(secretID)
-}
-
-func (a *TestACLAgent) ResolveTokenAndDefaultMeta(secretID string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) {
 	identity, authz, err := a.ResolveTokenToIdentityAndAuthorizer(secretID)
 	if err != nil {
-		return nil, err
+		return consul.ACLResolveResult{}, err
 	}
 
 	// Default the EnterpriseMeta based on the Tokens meta or actual defaults
@@ -119,7 +111,7 @@ func (a *TestACLAgent) ResolveTokenAndDefaultMeta(secretID string, entMeta *stru
 	// Use the meta to fill in the ACL authorization context
 	entMeta.FillAuthzContext(authzContext)
 
-	return authz, err
+	return consul.ACLResolveResult{Authorizer: authz, ACLIdentity: identity}, err
 }
 
 // All of these are stubs to satisfy the interface
@@ -523,22 +515,3 @@ func TestACL_filterChecksWithAuthorizer(t *testing.T) {
 	_, ok = checks["my-other"]
 	require.False(t, ok)
 }
-
-// TODO: remove?
-func TestACL_ResolveIdentity(t *testing.T) {
-	t.Parallel()
-	a := NewTestACLAgent(t, t.Name(), TestACLConfig(), nil, catalogIdent)
-
-	// this test is meant to ensure we are calling the correct function
-	// which is ResolveTokenToIdentity on the Agent delegate. Our
-	// nil authz resolver will cause it to emit an error if used
-	ident, err := a.delegate.ResolveTokenToIdentity(nodeROSecret)
-	require.NoError(t, err)
-	require.NotNil(t, ident)
-
-	// just double checkingto ensure if we had used the wrong function
-	// that an error would be produced
-	_, err = a.delegate.ResolveTokenAndDefaultMeta(nodeROSecret, nil, nil)
-	require.Error(t, err)
-
-}

agent/agent.go

@@ -167,14 +167,11 @@ type delegate interface {
 	// RemoveFailedNode is used to remove a failed node from the cluster.
 	RemoveFailedNode(node string, prune bool, entMeta *structs.EnterpriseMeta) error
 
-	// TODO: replace this method with consul.ACLResolver
-	ResolveTokenToIdentity(token string) (structs.ACLIdentity, error)
-
 	// ResolveTokenAndDefaultMeta returns an acl.Authorizer which authorizes
 	// actions based on the permissions granted to the token.
 	// If either entMeta or authzContext are non-nil they will be populated with the
 	// default partition and namespace from the token.
-	ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error)
+	ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (consul.ACLResolveResult, error)
 
 	RPC(method string, args interface{}, reply interface{}) error
 	SnapshotRPC(args *structs.SnapshotRequest, in io.Reader, out io.Writer, replyFn structs.SnapshotReplyFn) error

agent/agent_endpoint_test.go

@@ -1640,8 +1640,8 @@ type fakeResolveTokenDelegate struct {
 	authorizer acl.Authorizer
 }
 
-func (f fakeResolveTokenDelegate) ResolveTokenAndDefaultMeta(_ string, _ *structs.EnterpriseMeta, _ *acl.AuthorizerContext) (acl.Authorizer, error) {
+func (f fakeResolveTokenDelegate) ResolveTokenAndDefaultMeta(_ string, _ *structs.EnterpriseMeta, _ *acl.AuthorizerContext) (consul.ACLResolveResult, error) {
-	return f.authorizer, nil
+	return consul.ACLResolveResult{Authorizer: f.authorizer}, nil
 }
 
 func TestAgent_Reload(t *testing.T) {

agent/consul/acl.go

@@ -34,10 +34,6 @@ var ACLSummaries = []prometheus.SummaryDefinition{
 		Name: []string{"acl", "ResolveToken"},
 		Help: "This measures the time it takes to resolve an ACL token.",
 	},
-	{
-		Name: []string{"acl", "ResolveTokenToIdentity"},
-		Help: "This measures the time it takes to resolve an ACL token to an Identity.",
-	},
 }
 
 // These must be kept in sync with the constants in command/agent/acl.go.
@@ -1117,31 +1113,17 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs
 	return identity, acl.NewChainedAuthorizer(chain), nil
 }
 
-// TODO: rename to AccessorIDFromToken. This method is only used to retrieve the
+type ACLResolveResult struct {
-// ACLIdentity.ID, so we don't need to return a full ACLIdentity. We could
+	acl.Authorizer
-// return a much smaller type (instad of just a string) to allow for changes
+	// TODO: likely we can reduce this interface
-// in the future.
+	structs.ACLIdentity
-func (r *ACLResolver) ResolveTokenToIdentity(token string) (structs.ACLIdentity, error) {
+}
-	if !r.ACLsEnabled() {
+
-		return nil, nil
+func (a ACLResolveResult) AccessorID() string {
+	if a.ACLIdentity == nil {
+		return ""
 	}
-
+	return a.ACLIdentity.ID()
-	if acl.RootAuthorizer(token) != nil {
-		return nil, acl.ErrRootDenied
-	}
-
-	// handle the anonymous token
-	if token == "" {
-		token = anonymousToken
-	}
-
-	if ident, _, ok := r.resolveLocallyManagedToken(token); ok {
-		return ident, nil
-	}
-
-	defer metrics.MeasureSince([]string{"acl", "ResolveTokenToIdentity"}, time.Now())
-
-	return r.resolveIdentityFromToken(token)
 }
 
 func (r *ACLResolver) ACLsEnabled() bool {
@@ -1160,10 +1142,10 @@ func (r *ACLResolver) ACLsEnabled() bool {
 	return true
 }
 
-func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) {
+func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (ACLResolveResult, error) {
 	identity, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token)
 	if err != nil {
-		return nil, err
+		return ACLResolveResult{}, err
 	}
 
 	if entMeta == nil {
@@ -1181,7 +1163,7 @@ func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs.
 	// Use the meta to fill in the ACL authorization context
 	entMeta.FillAuthzContext(authzContext)
 
-	return authz, err
+	return ACLResolveResult{Authorizer: authz, ACLIdentity: identity}, err
 }
 
 // aclFilter is used to filter results from our state store based on ACL rules

agent/consul/acl_server.go

@@ -165,13 +165,12 @@ func (s *serverACLResolverBackend) ResolveRoleFromID(roleID string) (bool, *stru
 	return s.InPrimaryDatacenter() || index > 0, role, acl.ErrNotFound
 }
 
+// TODO: remove
 func (s *Server) ResolveToken(token string) (acl.Authorizer, error) {
 	_, authz, err := s.ACLResolver.ResolveTokenToIdentityAndAuthorizer(token)
 	return authz, err
 }
 
-// TODO: Client has an identical implementation, remove duplication
-
 func (s *Server) filterACL(token string, subj interface{}) error {
 	return filterACL(s.ACLResolver, token, subj)
 }

agent/consul/acl_test.go

@@ -1534,36 +1534,6 @@ func TestACLResolver_Client(t *testing.T) {
 		require.Equal(t, policyResolves, int32(3))
 	})
 
-	t.Run("Resolve-Identity", func(t *testing.T) {
-		t.Parallel()
-
-		delegate := &ACLResolverTestDelegate{
-			enabled:       true,
-			datacenter:    "dc1",
-			legacy:        false,
-			localTokens:   false,
-			localPolicies: false,
-		}
-
-		delegate.tokenReadFn = delegate.plainTokenReadFn
-		delegate.policyResolveFn = delegate.plainPolicyResolveFn
-		delegate.roleResolveFn = delegate.plainRoleResolveFn
-
-		r := newTestACLResolver(t, delegate, nil)
-
-		ident, err := r.ResolveTokenToIdentity("found-policy-and-role")
-		require.NoError(t, err)
-		require.NotNil(t, ident)
-		require.Equal(t, "5f57c1f6-6a89-4186-9445-531b316e01df", ident.ID())
-		require.EqualValues(t, 0, delegate.localTokenResolutions)
-		require.EqualValues(t, 1, delegate.remoteTokenResolutions)
-		require.EqualValues(t, 0, delegate.localPolicyResolutions)
-		require.EqualValues(t, 0, delegate.remotePolicyResolutions)
-		require.EqualValues(t, 0, delegate.localRoleResolutions)
-		require.EqualValues(t, 0, delegate.remoteRoleResolutions)
-		require.EqualValues(t, 0, delegate.remoteLegacyResolutions)
-	})
-
 	t.Run("Concurrent-Token-Resolve", func(t *testing.T) {
 		t.Parallel()
 

agent/consul/intention_endpoint.go

@@ -100,6 +100,7 @@ func (s *Intention) Apply(args *structs.IntentionRequest, reply *string) error {
 	}
 
 	// Get the ACL token for the request for the checks below.
+	// TODO: use ResolveTokenAndDefaultMeta
 	identity, authz, err := s.srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(args.Token)
 	if err != nil {
 		return err
@@ -432,7 +433,8 @@ func (s *Intention) Get(args *structs.IntentionQueryRequest, reply *structs.Inde
 
 	// Get the ACL token for the request for the checks below.
 	var entMeta structs.EnterpriseMeta
-	if _, err := s.srv.ResolveTokenAndDefaultMeta(args.Token, &entMeta, nil); err != nil {
+	authz, err := s.srv.ResolveTokenAndDefaultMeta(args.Token, &entMeta, nil)
+	if err != nil {
 		return err
 	}
 
@@ -479,13 +481,11 @@ func (s *Intention) Get(args *structs.IntentionQueryRequest, reply *structs.Inde
 			reply.Intentions = structs.Intentions{ixn}
 
 			// Filter
-			if err := s.srv.filterACL(args.Token, reply); err != nil {
+			s.srv.filterACLWithAuthorizer(authz, reply)
-				return err
-			}
 
 			// If ACLs prevented any responses, error
 			if len(reply.Intentions) == 0 {
-				accessorID := s.aclAccessorID(args.Token)
+				accessorID := authz.AccessorID()
 				// todo(kit) Migrate intention access denial logging over to audit logging when we implement it
 				s.logger.Warn("Request to get intention denied due to ACLs", "intention", args.IntentionID, "accessorID", accessorID)
 				return acl.ErrPermissionDenied
@@ -618,7 +618,7 @@ func (s *Intention) Match(args *structs.IntentionQueryRequest, reply *structs.In
 	for _, entry := range args.Match.Entries {
 		entry.FillAuthzContext(&authzContext)
 		if prefix := entry.Name; prefix != "" && authz.IntentionRead(prefix, &authzContext) != acl.Allow {
-			accessorID := s.aclAccessorID(args.Token)
+			accessorID := authz.AccessorID()
 			// todo(kit) Migrate intention access denial logging over to audit logging when we implement it
 			s.logger.Warn("Operation on intention prefix denied due to ACLs", "prefix", prefix, "accessorID", accessorID)
 			return acl.ErrPermissionDenied
@@ -708,7 +708,7 @@ func (s *Intention) Check(args *structs.IntentionQueryRequest, reply *structs.In
 		var authzContext acl.AuthorizerContext
 		query.FillAuthzContext(&authzContext)
 		if authz.ServiceRead(prefix, &authzContext) != acl.Allow {
-			accessorID := s.aclAccessorID(args.Token)
+			accessorID := authz.AccessorID()
 			// todo(kit) Migrate intention access denial logging over to audit logging when we implement it
 			s.logger.Warn("test on intention denied due to ACLs", "prefix", prefix, "accessorID", accessorID)
 			return acl.ErrPermissionDenied
@@ -760,24 +760,6 @@ func (s *Intention) Check(args *structs.IntentionQueryRequest, reply *structs.In
 	return nil
 }
 
-// aclAccessorID is used to convert an ACLToken's secretID to its accessorID for non-
-// critical purposes, such as logging. Therefore we interpret all errors as empty-string
-// so we can safely log it without handling non-critical errors at the usage site.
-func (s *Intention) aclAccessorID(secretID string) string {
-	_, ident, err := s.srv.ResolveIdentityFromToken(secretID)
-	if acl.IsErrNotFound(err) {
-		return ""
-	}
-	if err != nil {
-		s.logger.Debug("non-critical error resolving acl token accessor for logging", "error", err)
-		return ""
-	}
-	if ident == nil {
-		return ""
-	}
-	return ident.ID()
-}
-
 func (s *Intention) validateEnterpriseIntention(ixn *structs.Intention) error {
 	if err := s.srv.validateEnterpriseIntentionPartition(ixn.SourcePartition); err != nil {
 		return fmt.Errorf("Invalid source partition %q: %v", ixn.SourcePartition, err)

agent/consul/internal_endpoint.go

@@ -401,13 +401,13 @@ func (m *Internal) EventFire(args *structs.EventFireRequest,
 	}
 
 	// Check ACLs
-	authz, err := m.srv.ResolveToken(args.Token)
+	authz, err := m.srv.ResolveTokenAndDefaultMeta(args.Token, nil, nil)
 	if err != nil {
 		return err
 	}
 
 	if authz.EventWrite(args.Name, nil) != acl.Allow {
-		accessorID := m.aclAccessorID(args.Token)
+		accessorID := authz.AccessorID()
 		m.logger.Warn("user event blocked by ACLs", "event", args.Name, "accessorID", accessorID)
 		return acl.ErrPermissionDenied
 	}
@@ -545,21 +545,3 @@ func (m *Internal) executeKeyringOpMgr(
 
 	return serfResp, err
 }
-
-// aclAccessorID is used to convert an ACLToken's secretID to its accessorID for non-
-// critical purposes, such as logging. Therefore we interpret all errors as empty-string
-// so we can safely log it without handling non-critical errors at the usage site.
-func (m *Internal) aclAccessorID(secretID string) string {
-	_, ident, err := m.srv.ResolveIdentityFromToken(secretID)
-	if acl.IsErrNotFound(err) {
-		return ""
-	}
-	if err != nil {
-		m.logger.Debug("non-critical error resolving acl token accessor for logging", "error", err)
-		return ""
-	}
-	if ident == nil {
-		return ""
-	}
-	return ident.ID()
-}

agent/delegate_mock_test.go

@@ -47,11 +47,6 @@ func (m *delegateMock) RemoveFailedNode(node string, prune bool, entMeta *struct
 	return m.Called(node, prune, entMeta).Error(0)
 }
 
-func (m *delegateMock) ResolveTokenToIdentity(token string) (structs.ACLIdentity, error) {
-	ret := m.Called(token)
-	return ret.Get(0).(structs.ACLIdentity), ret.Error(1)
-}
-
 func (m *delegateMock) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) {
 	ret := m.Called(token, entMeta, authzContext)
 	return ret.Get(0).(acl.Authorizer), ret.Error(1)

agent/local/state.go

@@ -14,6 +14,7 @@ import (
 	"github.com/hashicorp/go-hclog"
 
 	"github.com/hashicorp/consul/acl"
+	"github.com/hashicorp/consul/agent/consul"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/agent/token"
 	"github.com/hashicorp/consul/api"
@@ -150,7 +151,7 @@ func (c *CheckState) CriticalFor() time.Duration {
 
 type rpc interface {
 	RPC(method string, args interface{}, reply interface{}) error
-	ResolveTokenToIdentity(secretID string) (structs.ACLIdentity, error)
+	ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (consul.ACLResolveResult, error)
 }
 
 // State is used to represent the node's services,
@@ -1538,7 +1539,7 @@ func (l *State) notifyIfAliased(serviceID structs.ServiceID) {
 // critical purposes, such as logging. Therefore we interpret all errors as empty-string
 // so we can safely log it without handling non-critical errors at the usage site.
 func (l *State) aclAccessorID(secretID string) string {
-	ident, err := l.Delegate.ResolveTokenToIdentity(secretID)
+	ident, err := l.Delegate.ResolveTokenAndDefaultMeta(secretID, nil, nil)
 	if acl.IsErrNotFound(err) {
 		return ""
 	}
@@ -1546,8 +1547,5 @@ func (l *State) aclAccessorID(secretID string) string {
 		l.logger.Debug("non-critical error resolving acl token accessor for logging", "error", err)
 		return ""
 	}
-	if ident == nil {
+	return ident.AccessorID()
-		return ""
-	}
-	return ident.ID()
 }

agent/local/state_test.go

@@ -12,8 +12,10 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/agent"
 	"github.com/hashicorp/consul/agent/config"
+	"github.com/hashicorp/consul/agent/consul"
 	"github.com/hashicorp/consul/agent/local"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/agent/token"
@@ -2372,6 +2374,6 @@ func (f *fakeRPC) RPC(method string, args interface{}, reply interface{}) error
 	return nil
 }
 
-func (f *fakeRPC) ResolveTokenToIdentity(_ string) (structs.ACLIdentity, error) {
+func (f *fakeRPC) ResolveTokenAndDefaultMeta(string, *structs.EnterpriseMeta, *acl.AuthorizerContext) (consul.ACLResolveResult, error) {
-	return nil, nil
+	return consul.ACLResolveResult{}, nil
 }

agent/structs/acl.go

@@ -89,8 +89,8 @@ var ACLBootstrapNotAllowedErr = errors.New("ACL bootstrap no longer allowed")
 var ACLBootstrapInvalidResetIndexErr = errors.New("Invalid ACL bootstrap reset index")
 
 type ACLIdentity interface {
-	// ID returns a string that can be used for logging and telemetry. This should not
+	// ID returns the accessor ID, a string that can be used for logging and
-	// contain any secret data used for authentication
+	// telemetry. It is not the secret ID used for authentication.
 	ID() string
 	SecretToken() string
 	PolicyIDs() []string

website/content/docs/agent/telemetry.mdx

@@ -388,7 +388,7 @@ These metrics are used to monitor the health of the Consul servers.
 | Metric                                              | Description                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                               | Unit                              | Type    |
 | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- | ------- |
 | `consul.acl.ResolveToken`                           | Measures the time it takes to resolve an ACL token.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  | ms                                | timer   |
-| `consul.acl.ResolveTokenToIdentity`                 | Measures the time it takes to resolve an ACL token to an Identity.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                   | ms                                | timer   |
+| `consul.acl.ResolveTokenToIdentity`                 | Measures the time it takes to resolve an ACL token to an Identity. This metric was removed in Consul 1.12. The time will now be reflected in `consul.acl.ResolveToken`.                                                                                                                                                                                                              | ms                                | timer   |
 | `consul.acl.token.cache_hit`                        | Increments if Consul is able to resolve a token's identity, or a legacy token, from the cache.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                            | cache read op                     | counter |
 | `consul.acl.token.cache_miss`                       | Increments if Consul cannot resolve a token's identity, or a legacy token, from the cache.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                | cache read op                     | counter |
 | `consul.cache.bypass`                               | Counts how many times a request bypassed the cache because no cache-key was provided.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     | counter                           | counter |