Merge pull request #10743 from hashicorp/dnephin/acl-resolver-2

acl: decouple filtering from ACLResolver and remove a couple methods

agent/consul/acl.go

@@ -1244,6 +1244,10 @@ 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
+// ACLIdentity.ID, so we don't need to return a full ACLIdentity. We could
+// return a much smaller type (instad of just a string) to allow for changes
+// in the future.
 func (r *ACLResolver) ResolveTokenToIdentity(token string) (structs.ACLIdentity, error) {
 	if !r.ACLsEnabled() {
 		return nil, nil
@@ -1928,12 +1932,11 @@ func (f *aclFilter) filterGatewayServices(mappings *structs.GatewayServices) {
 	*mappings = ret
 }
 
-func (r *ACLResolver) filterACLWithAuthorizer(authorizer acl.Authorizer, subj interface{}) error {
+func filterACLWithAuthorizer(logger hclog.Logger, authorizer acl.Authorizer, subj interface{}) {
 	if authorizer == nil {
-		return nil
+		return
 	}
-	// Create the filter
+	filt := newACLFilter(authorizer, logger)
-	filt := newACLFilter(authorizer, r.logger)
 
 	switch v := subj.(type) {
 	case *structs.CheckServiceNodes:
@@ -2028,23 +2031,17 @@ func (r *ACLResolver) filterACLWithAuthorizer(authorizer acl.Authorizer, subj in
 	default:
 		panic(fmt.Errorf("Unhandled type passed to ACL filter: %T %#v", subj, subj))
 	}
-
-	return nil
 }
 
-// filterACL is used to filter results from our service catalog based on the
+// filterACL uses the ACLResolver to resolve the token in an acl.Authorizer,
-// rules configured for the provided token.
+// then uses the acl.Authorizer to filter subj. Any entities in subj that are
-func (r *ACLResolver) filterACL(token string, subj interface{}) error {
+// not authorized for read access will be removed from subj.
+func filterACL(r *ACLResolver, token string, subj interface{}) error {
 	// Get the ACL from the token
 	_, authorizer, err := r.ResolveTokenToIdentityAndAuthorizer(token)
 	if err != nil {
 		return err
 	}
-
+	filterACLWithAuthorizer(r.logger, authorizer, subj)
-	// Fast path if ACLs are not enabled
+	return nil
-	if authorizer == nil {
-		return nil
-	}
-
-	return r.filterACLWithAuthorizer(authorizer, subj)
 }

agent/consul/acl_client.go

@@ -93,6 +93,7 @@ func (c *Client) ResolveTokenToIdentity(token string) (structs.ACLIdentity, erro
 	return c.acls.ResolveTokenToIdentity(token)
 }
 
+// TODO: Server has an identical implementation, remove duplication
 func (c *Client) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) {
 	identity, authz, err := c.acls.ResolveTokenToIdentityAndAuthorizer(token)
 	if err != nil {

agent/consul/acl_endpoint.go

@@ -956,9 +956,7 @@ func (a *ACL) TokenList(args *structs.ACLTokenListRequest, reply *structs.ACLTok
 			}
 
 			// filter down to just the tokens that the requester has permissions to read
-			if err := a.srv.filterACLWithAuthorizer(authz, &stubs); err != nil {
+			a.srv.filterACLWithAuthorizer(authz, &stubs)
-				return err
-			}
 
 			reply.Index, reply.Tokens = index, stubs
 			return nil

agent/consul/acl_server.go

@@ -224,7 +224,7 @@ func (s *Server) ResolveRoleFromID(roleID string) (bool, *structs.ACLRole, error
 }
 
 func (s *Server) ResolveToken(token string) (acl.Authorizer, error) {
-	_, authz, err := s.ResolveTokenToIdentityAndAuthorizer(token)
+	_, authz, err := s.acls.ResolveTokenToIdentityAndAuthorizer(token)
 	return authz, err
 }
 
@@ -235,16 +235,11 @@ func (s *Server) ResolveTokenToIdentity(token string) (structs.ACLIdentity, erro
 	return s.acls.ResolveTokenToIdentity(token)
 }
 
-func (s *Server) ResolveTokenToIdentityAndAuthorizer(token string) (structs.ACLIdentity, acl.Authorizer, error) {
+// TODO: Client has an identical implementation, remove duplication
-	return s.acls.ResolveTokenToIdentityAndAuthorizer(token)
+func (s *Server) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) {
-}
+	identity, authz, err := s.acls.ResolveTokenToIdentityAndAuthorizer(token)
-
-// ResolveTokenIdentityAndDefaultMeta retrieves an identity and authorizer for the caller,
-// and populates the EnterpriseMeta based on the AuthorizerContext.
-func (s *Server) ResolveTokenIdentityAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (structs.ACLIdentity, acl.Authorizer, error) {
-	identity, authz, err := s.ResolveTokenToIdentityAndAuthorizer(token)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	// Default the EnterpriseMeta based on the Tokens meta or actual defaults
@@ -258,19 +253,13 @@ func (s *Server) ResolveTokenIdentityAndDefaultMeta(token string, entMeta *struc
 	// Use the meta to fill in the ACL authorization context
 	entMeta.FillAuthzContext(authzContext)
 
-	return identity, authz, err
-}
-
-// ResolveTokenAndDefaultMeta passes through to ResolveTokenIdentityAndDefaultMeta, eliding the identity from its response.
-func (s *Server) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) {
-	_, authz, err := s.ResolveTokenIdentityAndDefaultMeta(token, entMeta, authzContext)
 	return authz, err
 }
 
 func (s *Server) filterACL(token string, subj interface{}) error {
-	return s.acls.filterACL(token, subj)
+	return filterACL(s.acls, token, subj)
 }
 
-func (s *Server) filterACLWithAuthorizer(authorizer acl.Authorizer, subj interface{}) error {
+func (s *Server) filterACLWithAuthorizer(authorizer acl.Authorizer, subj interface{}) {
-	return s.acls.filterACLWithAuthorizer(authorizer, subj)
+	filterACLWithAuthorizer(s.acls.logger, authorizer, subj)
 }

agent/consul/acl_test.go

@@ -3276,7 +3276,7 @@ func TestACL_redactPreparedQueryTokens(t *testing.T) {
 	}
 }
 
-func TestACL_redactTokenSecret(t *testing.T) {
+func TestFilterACL_redactTokenSecret(t *testing.T) {
 	t.Parallel()
 	delegate := &ACLResolverTestDelegate{
 		enabled:       true,
@@ -3293,16 +3293,16 @@ func TestACL_redactTokenSecret(t *testing.T) {
 		SecretID:   "6a5e25b3-28f2-4085-9012-c3fb754314d1",
 	}
 
-	err := r.filterACL("acl-wr", &token)
+	err := filterACL(r, "acl-wr", &token)
 	require.NoError(t, err)
 	require.Equal(t, "6a5e25b3-28f2-4085-9012-c3fb754314d1", token.SecretID)
 
-	err = r.filterACL("acl-ro", &token)
+	err = filterACL(r, "acl-ro", &token)
 	require.NoError(t, err)
 	require.Equal(t, redactedToken, token.SecretID)
 }
 
-func TestACL_redactTokenSecrets(t *testing.T) {
+func TestFilterACL_redactTokenSecrets(t *testing.T) {
 	t.Parallel()
 	delegate := &ACLResolverTestDelegate{
 		enabled:       true,
@@ -3321,11 +3321,11 @@ func TestACL_redactTokenSecrets(t *testing.T) {
 		},
 	}
 
-	err := r.filterACL("acl-wr", &tokens)
+	err := filterACL(r, "acl-wr", &tokens)
 	require.NoError(t, err)
 	require.Equal(t, "6a5e25b3-28f2-4085-9012-c3fb754314d1", tokens[0].SecretID)
 
-	err = r.filterACL("acl-ro", &tokens)
+	err = filterACL(r, "acl-ro", &tokens)
 	require.NoError(t, err)
 	require.Equal(t, redactedToken, tokens[0].SecretID)
 }

agent/consul/catalog_endpoint.go

@@ -545,7 +545,8 @@ func (c *Catalog) ListServices(args *structs.DCSpecificRequest, reply *structs.I
 				return nil
 			}
 
-			return c.srv.filterACLWithAuthorizer(authz, reply)
+			c.srv.filterACLWithAuthorizer(authz, reply)
+			return nil
 		})
 }
 
@@ -573,7 +574,8 @@ func (c *Catalog) ServiceList(args *structs.DCSpecificRequest, reply *structs.In
 			}
 
 			reply.Index, reply.Services = index, services
-			return c.srv.filterACLWithAuthorizer(authz, reply)
+			c.srv.filterACLWithAuthorizer(authz, reply)
+			return nil
 		})
 }
 

agent/consul/intention_endpoint.go

@@ -100,15 +100,18 @@ func (s *Intention) Apply(args *structs.IntentionRequest, reply *string) error {
 	}
 
 	// Get the ACL token for the request for the checks below.
-	var entMeta structs.EnterpriseMeta
+	identity, authz, err := s.srv.acls.ResolveTokenToIdentityAndAuthorizer(args.Token)
-	ident, authz, err := s.srv.ResolveTokenIdentityAndDefaultMeta(args.Token, &entMeta, nil)
 	if err != nil {
 		return err
 	}
 
 	var accessorID string
-	if ident != nil {
+	var entMeta structs.EnterpriseMeta
-		accessorID = ident.ID()
+	if identity != nil {
+		entMeta.Merge(identity.EnterpriseMetadata())
+		accessorID = identity.ID()
+	} else {
+		entMeta.Merge(structs.DefaultEnterpriseMetaInDefaultPartition())
 	}
 
 	var (

agent/consul/internal_endpoint.go

@@ -222,7 +222,8 @@ func (m *Internal) IntentionUpstreams(args *structs.ServiceSpecificRequest, repl
 			}
 
 			reply.Index, reply.Services = index, services
-			return m.srv.filterACLWithAuthorizer(authz, reply)
+			m.srv.filterACLWithAuthorizer(authz, reply)
+			return nil
 		})
 }
 
@@ -439,7 +440,7 @@ func (m *Internal) KeyringOperation(
 	}
 
 	// Check ACLs
-	identity, rule, err := m.srv.ResolveTokenToIdentityAndAuthorizer(args.Token)
+	identity, rule, err := m.srv.acls.ResolveTokenToIdentityAndAuthorizer(args.Token)
 	if err != nil {
 		return err
 	}

agent/consul/operator_autopilot_endpoint.go

@@ -17,7 +17,7 @@ func (op *Operator) AutopilotGetConfiguration(args *structs.DCSpecificRequest, r
 	}
 
 	// This action requires operator read access.
-	identity, rule, err := op.srv.ResolveTokenToIdentityAndAuthorizer(args.Token)
+	identity, rule, err := op.srv.acls.ResolveTokenToIdentityAndAuthorizer(args.Token)
 	if err != nil {
 		return err
 	}
@@ -49,7 +49,7 @@ func (op *Operator) AutopilotSetConfiguration(args *structs.AutopilotSetConfigRe
 	}
 
 	// This action requires operator write access.
-	identity, rule, err := op.srv.ResolveTokenToIdentityAndAuthorizer(args.Token)
+	identity, rule, err := op.srv.acls.ResolveTokenToIdentityAndAuthorizer(args.Token)
 	if err != nil {
 		return err
 	}
@@ -84,7 +84,7 @@ func (op *Operator) ServerHealth(args *structs.DCSpecificRequest, reply *structs
 	}
 
 	// This action requires operator read access.
-	identity, rule, err := op.srv.ResolveTokenToIdentityAndAuthorizer(args.Token)
+	identity, rule, err := op.srv.acls.ResolveTokenToIdentityAndAuthorizer(args.Token)
 	if err != nil {
 		return err
 	}
@@ -151,7 +151,7 @@ func (op *Operator) AutopilotState(args *structs.DCSpecificRequest, reply *autop
 	}
 
 	// This action requires operator read access.
-	identity, rule, err := op.srv.ResolveTokenToIdentityAndAuthorizer(args.Token)
+	identity, rule, err := op.srv.acls.ResolveTokenToIdentityAndAuthorizer(args.Token)
 	if err != nil {
 		return err
 	}

agent/consul/operator_raft_endpoint.go

@@ -81,7 +81,7 @@ func (op *Operator) RaftRemovePeerByAddress(args *structs.RaftRemovePeerRequest,
 
 	// This is a super dangerous operation that requires operator write
 	// access.
-	identity, rule, err := op.srv.ResolveTokenToIdentityAndAuthorizer(args.Token)
+	identity, rule, err := op.srv.acls.ResolveTokenToIdentityAndAuthorizer(args.Token)
 	if err != nil {
 		return err
 	}
@@ -134,7 +134,7 @@ func (op *Operator) RaftRemovePeerByID(args *structs.RaftRemovePeerRequest, repl
 
 	// This is a super dangerous operation that requires operator write
 	// access.
-	identity, rule, err := op.srv.ResolveTokenToIdentityAndAuthorizer(args.Token)
+	identity, rule, err := op.srv.acls.ResolveTokenToIdentityAndAuthorizer(args.Token)
 	if err != nil {
 		return err
 	}

agent/consul/session_endpoint.go

@@ -199,9 +199,7 @@ func (s *Session) Get(args *structs.SessionSpecificRequest,
 			} else {
 				reply.Sessions = nil
 			}
-			if err := s.srv.filterACLWithAuthorizer(authz, reply); err != nil {
+			s.srv.filterACLWithAuthorizer(authz, reply)
-				return err
-			}
 			return nil
 		})
 }
@@ -233,9 +231,7 @@ func (s *Session) List(args *structs.SessionSpecificRequest,
 			}
 
 			reply.Index, reply.Sessions = index, sessions
-			if err := s.srv.filterACLWithAuthorizer(authz, reply); err != nil {
+			s.srv.filterACLWithAuthorizer(authz, reply)
-				return err
-			}
 			return nil
 		})
 }
@@ -267,9 +263,7 @@ func (s *Session) NodeSessions(args *structs.NodeSpecificRequest,
 			}
 
 			reply.Index, reply.Sessions = index, sessions
-			if err := s.srv.filterACLWithAuthorizer(authz, reply); err != nil {
+			s.srv.filterACLWithAuthorizer(authz, reply)
-				return err
-			}
 			return nil
 		})
 }