client: pass secret in Status.Members RPC

In #16217 we switched clients using Consul discovery to the `Status.Members`
endpoint for getting the list of servers so that we're using the correct
address. This endpoint is authenticated, and we failed to add the `AuthToken`
with the client secret to the request.

This changeset also expands the test coverage of the RPC endpoint and closes up
potential holes in the `ResolveACL` method that aren't currently bugs but easily
could become bugs if we called the method without ensuring its invariants are
upheld.

client/client.go

@@ -2910,7 +2910,8 @@ func (c *Client) consulDiscoveryImpl() error {
 	region := c.Region()
 	rpcargs := structs.GenericRequest{
 		QueryOptions: structs.QueryOptions{
-			Region: region,
+			Region:    region,
+			AuthToken: c.secretNodeID(),
 		},
 	}
 

nomad/acl.go

@@ -161,9 +161,14 @@ func (s *Server) remoteIPFromRPCContext(ctx *RPCContext) (net.IP, error) {
 // for the identity they intend the operation to be performed with.
 func (s *Server) ResolveACL(args structs.RequestWithIdentity) (*acl.ACL, error) {
 	identity := args.GetIdentity()
-	if !s.config.ACLEnabled || identity == nil {
+	if !s.config.ACLEnabled {
 		return nil, nil
 	}
+	if identity == nil {
+		// Server.Authenticate should never return a nil identity unless there's
+		// an authentication error, but enforce that invariant here
+		return nil, structs.ErrPermissionDenied
+	}
 	aclToken := identity.GetACLToken()
 	if aclToken != nil {
 		return s.ResolveACLForToken(aclToken)
@@ -172,7 +177,10 @@ func (s *Server) ResolveACL(args structs.RequestWithIdentity) (*acl.ACL, error)
 	if claims != nil {
 		return s.ResolveClaims(claims)
 	}
-	return nil, nil
+
+	// return an error here so that we enforce the invariant that we check for
+	// Identity.ClientID before trying to resolve ACLs
+	return nil, structs.ErrPermissionDenied
 }
 
 // ResolveACLForToken resolves an ACL from a token only. It should be used only

nomad/status_endpoint.go

@@ -86,11 +86,15 @@ func (s *Status) Members(args *structs.GenericRequest, reply *structs.ServerMemb
 	if authErr != nil {
 		return structs.ErrPermissionDenied
 	}
+
 	// Check node read permissions
-	if aclObj, err := s.srv.ResolveACL(args); err != nil {
-		return err
-	} else if aclObj != nil && !aclObj.AllowNodeRead() {
-		return structs.ErrPermissionDenied
+	if args.GetIdentity().ClientID == "" {
+		aclObj, err := s.srv.ResolveACL(args)
+		if err != nil {
+			return err
+		} else if aclObj != nil && !aclObj.AllowNodeRead() {
+			return structs.ErrPermissionDenied
+		}
 	}
 
 	serfMembers := s.srv.Members()

nomad/status_endpoint_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/testutil"
+	"github.com/shoenig/test/must"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -99,12 +100,13 @@ func TestStatusMembers_ACL(t *testing.T) {
 	s1, root, cleanupS1 := TestACLServer(t, nil)
 	defer cleanupS1()
 	codec := rpcClient(t, s1)
-	assert := assert.New(t)
-	state := s1.fsm.State()
+	store := s1.fsm.State()
 
 	// Create the namespace policy and tokens
-	validToken := mock.CreatePolicyAndToken(t, state, 1001, "test-valid", mock.NodePolicy(acl.PolicyRead))
-	invalidToken := mock.CreatePolicyAndToken(t, state, 1003, "test-invalid", mock.AgentPolicy(acl.PolicyRead))
+	validToken := mock.CreatePolicyAndToken(t, store, 1001, "test-valid", mock.NodePolicy(acl.PolicyRead))
+	invalidToken := mock.CreatePolicyAndToken(t, store, 1003, "test-invalid", mock.AgentPolicy(acl.PolicyRead))
+	node := mock.Node()
+	must.NoError(t, store.UpsertNode(structs.MsgTypeTestSetup, 1004, node))
 
 	arg := &structs.GenericRequest{
 		QueryOptions: structs.QueryOptions{
@@ -113,38 +115,46 @@ func TestStatusMembers_ACL(t *testing.T) {
 		},
 	}
 
-	// Try without a token and expect failure
-	{
+	t.Run("without token should fail", func(t *testing.T) {
 		var out structs.ServerMembersResponse
 		err := msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out)
-		assert.NotNil(err)
-		assert.Equal(err.Error(), structs.ErrPermissionDenied.Error())
-	}
+		must.ErrorContains(t, err, structs.ErrPermissionDenied.Error())
+	})
 
-	// Try with an invalid token and expect failure
-	{
+	t.Run("unauthorized token should fail", func(t *testing.T) {
 		arg.AuthToken = invalidToken.SecretID
 		var out structs.ServerMembersResponse
 		err := msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out)
-		assert.NotNil(err)
-		assert.Equal(err.Error(), structs.ErrPermissionDenied.Error())
-	}
+		must.ErrorContains(t, err, structs.ErrPermissionDenied.Error())
+	})
+
+	t.Run("invalid token should fail", func(t *testing.T) {
+		arg.AuthToken = uuid.Generate()
+		var out structs.ServerMembersResponse
+		err := msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out)
+		must.ErrorContains(t, err, structs.ErrPermissionDenied.Error())
+	})
 
-	// Try with a valid token
-	{
+	t.Run("valid token should work", func(t *testing.T) {
 		arg.AuthToken = validToken.SecretID
 		var out structs.ServerMembersResponse
-		assert.Nil(msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out))
-		assert.Len(out.Members, 1)
-	}
+		must.NoError(t, msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out))
+		must.Len(t, 1, out.Members)
+	})
 
-	// Try with a management token
-	{
+	t.Run("management token should work", func(t *testing.T) {
 		arg.AuthToken = root.SecretID
 		var out structs.ServerMembersResponse
-		assert.Nil(msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out))
-		assert.Len(out.Members, 1)
-	}
+		must.NoError(t, msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out))
+		must.Len(t, 1, out.Members)
+	})
+
+	t.Run("valid node token should work", func(t *testing.T) {
+		arg.AuthToken = node.SecretID
+		var out structs.ServerMembersResponse
+		must.NoError(t, msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out))
+		must.Len(t, 1, out.Members)
+	})
 }
 
 func TestStatus_HasClientConn(t *testing.T) {