Move static token resolution into the ACLResolver (#10013)

.changelog/10013.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+audit-logging: (Enterprise only) Fixed an issue that resulted in usage of the agent master token or managed service provider tokens from being resolved properly.
+```

agent/acl.go

@@ -9,34 +9,6 @@ import (
 	"github.com/hashicorp/consul/agent/structs"
 )
 
-// resolveToken is the primary interface used by ACL-checkers in the agent
-// endpoints, which is the one place where we do some ACL enforcement on
-// clients. Some of the enforcement is normative (e.g. self and monitor)
-// and some is informative (e.g. catalog and health).
-func (a *Agent) resolveToken(id string) (acl.Authorizer, error) {
-	return a.resolveTokenAndDefaultMeta(id, nil, nil)
-}
-
-// resolveTokenAndDefaultMeta is used to resolve an ACL token secret to an
-// acl.Authorizer and to default any enterprise specific metadata for the request.
-// The defaulted metadata is then used to fill in an acl.AuthorizationContext.
-func (a *Agent) resolveTokenAndDefaultMeta(id string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) {
-	// ACLs are disabled
-	if !a.config.ACLsEnabled {
-		return nil, nil
-	}
-
-	if acl.RootAuthorizer(id) != nil {
-		return nil, acl.ErrRootDenied
-	}
-
-	if a.tokens.IsAgentMasterToken(id) {
-		return a.aclMasterAuthorizer, nil
-	}
-
-	return a.delegate.ResolveTokenAndDefaultMeta(id, entMeta, authzContext)
-}
-
 // 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.
@@ -55,36 +27,11 @@ func (a *Agent) aclAccessorID(secretID string) string {
 	return ident.ID()
 }
 
-func initializeACLs(nodeName string) (acl.Authorizer, error) {
-	// Build a policy for the agent master token.
-	// The builtin agent master policy allows reading any node information
-	// and allows writes to the agent with the node name of the running agent
-	// only. This used to allow a prefix match on agent names but that seems
-	// entirely unnecessary so it is now using an exact match.
-	policy := &acl.Policy{
-		PolicyRules: acl.PolicyRules{
-			Agents: []*acl.AgentRule{
-				{
-					Node:   nodeName,
-					Policy: acl.PolicyWrite,
-				},
-			},
-			NodePrefixes: []*acl.NodeRule{
-				{
-					Name:   "",
-					Policy: acl.PolicyRead,
-				},
-			},
-		},
-	}
-	return acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
-}
-
 // vetServiceRegister makes sure the service registration action is allowed by
 // the given token.
 func (a *Agent) vetServiceRegister(token string, service *structs.NodeService) error {
 	// Resolve the token and bail if ACLs aren't enabled.
-	authz, err := a.resolveToken(token)
+	authz, err := a.delegate.ResolveTokenAndDefaultMeta(token, nil, nil)
 	if err != nil {
 		return err
 	}
@@ -130,7 +77,7 @@ func (a *Agent) vetServiceRegisterWithAuthorizer(authz acl.Authorizer, service *
 // token.
 func (a *Agent) vetServiceUpdate(token string, serviceID structs.ServiceID) error {
 	// Resolve the token and bail if ACLs aren't enabled.
-	authz, err := a.resolveToken(token)
+	authz, err := a.delegate.ResolveTokenAndDefaultMeta(token, nil, nil)
 	if err != nil {
 		return err
 	}
@@ -163,7 +110,7 @@ func (a *Agent) vetServiceUpdateWithAuthorizer(authz acl.Authorizer, serviceID s
 // given token.
 func (a *Agent) vetCheckRegister(token string, check *structs.HealthCheck) error {
 	// Resolve the token and bail if ACLs aren't enabled.
-	authz, err := a.resolveToken(token)
+	authz, err := a.delegate.ResolveTokenAndDefaultMeta(token, nil, nil)
 	if err != nil {
 		return err
 	}
@@ -208,7 +155,7 @@ func (a *Agent) vetCheckRegisterWithAuthorizer(authz acl.Authorizer, check *stru
 // vetCheckUpdate makes sure that a check update is allowed by the given token.
 func (a *Agent) vetCheckUpdate(token string, checkID structs.CheckID) error {
 	// Resolve the token and bail if ACLs aren't enabled.
-	authz, err := a.resolveToken(token)
+	authz, err := a.delegate.ResolveTokenAndDefaultMeta(token, nil, nil)
 	if err != nil {
 		return err
 	}
@@ -245,7 +192,7 @@ func (a *Agent) vetCheckUpdateWithAuthorizer(authz acl.Authorizer, checkID struc
 // filterMembers redacts members that the token doesn't have access to.
 func (a *Agent) filterMembers(token string, members *[]serf.Member) error {
 	// Resolve the token and bail if ACLs aren't enabled.
-	rule, err := a.resolveToken(token)
+	rule, err := a.delegate.ResolveTokenAndDefaultMeta(token, nil, nil)
 	if err != nil {
 		return err
 	}
@@ -274,7 +221,7 @@ func (a *Agent) filterMembers(token string, members *[]serf.Member) error {
 // filterServices redacts services that the token doesn't have access to.
 func (a *Agent) filterServices(token string, services *map[structs.ServiceID]*structs.NodeService) error {
 	// Resolve the token and bail if ACLs aren't enabled.
-	authz, err := a.resolveToken(token)
+	authz, err := a.delegate.ResolveTokenAndDefaultMeta(token, nil, nil)
 	if err != nil {
 		return err
 	}
@@ -302,7 +249,7 @@ func (a *Agent) filterServicesWithAuthorizer(authz acl.Authorizer, services *map
 // filterChecks redacts checks that the token doesn't have access to.
 func (a *Agent) filterChecks(token string, checks *map[structs.CheckID]*structs.HealthCheck) error {
 	// Resolve the token and bail if ACLs aren't enabled.
-	authz, err := a.resolveToken(token)
+	authz, err := a.delegate.ResolveTokenAndDefaultMeta(token, nil, nil)
 	if err != nil {
 		return err
 	}

agent/acl_endpoint.go

@@ -104,7 +104,7 @@ func (s *HTTPHandlers) ACLRulesTranslate(resp http.ResponseWriter, req *http.Req
 
 	var token string
 	s.parseToken(req, &token)
-	rule, err := s.agent.resolveToken(token)
+	rule, err := s.agent.delegate.ResolveTokenAndDefaultMeta(token, nil, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -1153,7 +1153,7 @@ func (s *HTTPHandlers) ACLAuthorize(resp http.ResponseWriter, req *http.Request)
 			return nil, err
 		}
 	} else {
-		authz, err := s.agent.resolveToken(request.Token)
+		authz, err := s.agent.delegate.ResolveTokenAndDefaultMeta(request.Token, nil, nil)
 		if err != nil {
 			return nil, err
 		} else if authz == nil {

agent/acl_test.go

@@ -176,46 +176,11 @@ func TestACL_Version8EnabledByDefault(t *testing.T) {
 	}
 	a := NewTestACLAgent(t, t.Name(), TestACLConfig(), resolveFn, nil)
 
-	_, err := a.resolveToken("nope")
+	_, err := a.delegate.ResolveTokenAndDefaultMeta("nope", nil, nil)
 	require.Error(t, err)
 	require.True(t, called)
 }
 
-func TestACL_AgentMasterToken(t *testing.T) {
-	t.Parallel()
-
-	a := NewTestACLAgent(t, t.Name(), TestACLConfig(), nil, nil)
-	err := a.tokens.Load(a.config.ACLTokens, a.logger)
-	require.NoError(t, err)
-
-	authz, err := a.resolveToken("towel")
-	require.NotNil(t, authz)
-	require.Nil(t, err)
-
-	require.Equal(t, acl.Allow, authz.AgentRead(a.config.NodeName, nil))
-	require.Equal(t, acl.Allow, authz.AgentWrite(a.config.NodeName, nil))
-	require.Equal(t, acl.Allow, authz.NodeRead("foobarbaz", nil))
-	require.Equal(t, acl.Deny, authz.NodeWrite("foobarbaz", nil))
-}
-
-func TestACL_RootAuthorizersDenied(t *testing.T) {
-	t.Parallel()
-
-	a := NewTestACLAgent(t, t.Name(), TestACLConfig(), nil, nil)
-	authz, err := a.resolveToken("deny")
-	require.Nil(t, authz)
-	require.Error(t, err)
-	require.True(t, acl.IsErrRootDenied(err))
-	authz, err = a.resolveToken("allow")
-	require.Nil(t, authz)
-	require.Error(t, err)
-	require.True(t, acl.IsErrRootDenied(err))
-	authz, err = a.resolveToken("manage")
-	require.Nil(t, authz)
-	require.Error(t, err)
-	require.True(t, acl.IsErrRootDenied(err))
-}
-
 func authzFromPolicy(policy *acl.Policy, cfg *acl.Config) (acl.Authorizer, error) {
 	return acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, cfg)
 }
@@ -534,7 +499,7 @@ func TestACL_ResolveIdentity(t *testing.T) {
 
 	// just double checkingto ensure if we had used the wrong function
 	// that an error would be produced
-	_, err = a.resolveToken(nodeROSecret)
+	_, err = a.delegate.ResolveTokenAndDefaultMeta(nodeROSecret, nil, nil)
 	require.Error(t, err)
 
 }

agent/agent.go

@@ -382,13 +382,6 @@ func New(bd BaseDeps) (*Agent, error) {
 
 	a.serviceManager = NewServiceManager(&a)
 
-	// TODO: do this somewhere else, maybe move to newBaseDeps
-	var err error
-	a.aclMasterAuthorizer, err = initializeACLs(bd.RuntimeConfig.NodeName)
-	if err != nil {
-		return nil, err
-	}
-
 	// We used to do this in the Start method. However it doesn't need to go
 	// there any longer. Originally it did because we passed the agent
 	// delegate to some of the cache registrations. Now we just
@@ -651,9 +644,11 @@ func (a *Agent) listenAndServeGRPC() error {
 	}
 
 	xdsServer := &xds.Server{
-		Logger:             a.logger.Named(logging.Envoy),
-		CfgMgr:             a.proxyConfig,
-		ResolveToken:       a.resolveToken,
+		Logger: a.logger.Named(logging.Envoy),
+		CfgMgr: a.proxyConfig,
+		ResolveToken: func(id string) (acl.Authorizer, error) {
+			return a.delegate.ResolveTokenAndDefaultMeta(id, nil, nil)
+		},
 		CheckFetcher:       a,
 		CfgFetcher:         a,
 		AuthCheckFrequency: xds.DefaultAuthCheckFrequency,