From 48567054594b6e3d16098534b19fa647a864a74b Mon Sep 17 00:00:00 2001
From: Michael Schurter <schmichael@hashicorp.com>
Date: Tue, 10 Oct 2017 10:36:54 -0700
Subject: [PATCH] Status.Members ACL enforcement

Was incorrectly checked on the HTTP API before. Moved to RPC endpoint.
---
 command/agent/agent_endpoint.go | 25 ++---------
 nomad/status_endpoint.go        | 11 ++++-
 nomad/status_endpoint_test.go   | 75 +++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+), 22 deletions(-)

diff --git a/command/agent/agent_endpoint.go b/command/agent/agent_endpoint.go
index dfd1c7396..d06502dbd 100644
--- a/command/agent/agent_endpoint.go
+++ b/command/agent/agent_endpoint.go
@@ -119,28 +119,11 @@ func (s *HTTPServer) AgentMembersRequest(resp http.ResponseWriter, req *http.Req
 		return nil, CodedError(405, ErrInvalidMethod)
 	}
 
-	var secret string
-	s.parseToken(req, &secret)
-
-	var aclObj *acl.ACL
-	var err error
-
-	if client := s.agent.Client(); client != nil {
-		aclObj, err = client.ResolveToken(secret)
-	} else {
-		aclObj, err = s.agent.Server().ResolveToken(secret)
-	}
-
-	if err != nil {
-		return nil, err
-	}
-
-	// Check node read permissions
-	if aclObj != nil && !aclObj.AllowNodeRead() {
-		return nil, structs.ErrPermissionDenied
-	}
-
 	args := &structs.GenericRequest{}
+	if s.parse(resp, req, &args.Region, &args.QueryOptions) {
+		return nil, nil
+	}
+
 	var out structs.ServerMembersResponse
 	if err := s.agent.RPC("Status.Members", args, &out); err != nil {
 		return nil, err
diff --git a/nomad/status_endpoint.go b/nomad/status_endpoint.go
index b384f3a1b..42e7fb477 100644
--- a/nomad/status_endpoint.go
+++ b/nomad/status_endpoint.go
@@ -1,6 +1,8 @@
 package nomad
 
-import "github.com/hashicorp/nomad/nomad/structs"
+import (
+	"github.com/hashicorp/nomad/nomad/structs"
+)
 
 // Status endpoint is used to check on server status
 type Status struct {
@@ -70,6 +72,13 @@ func (s *Status) Peers(args *structs.GenericRequest, reply *[]string) error {
 // Members return the list of servers in a cluster that a particular server is
 // aware of
 func (s *Status) Members(args *structs.GenericRequest, reply *structs.ServerMembersResponse) error {
+	// Check node read permissions
+	if aclObj, err := s.srv.ResolveToken(args.SecretID); err != nil {
+		return err
+	} else if aclObj != nil && !aclObj.AllowNodeRead() {
+		return structs.ErrPermissionDenied
+	}
+
 	serfMembers := s.srv.Members()
 	members := make([]*structs.ServerMember, len(serfMembers))
 	for i, mem := range serfMembers {
diff --git a/nomad/status_endpoint_test.go b/nomad/status_endpoint_test.go
index 69014ec28..da42d0835 100644
--- a/nomad/status_endpoint_test.go
+++ b/nomad/status_endpoint_test.go
@@ -4,8 +4,11 @@ import (
 	"testing"
 
 	"github.com/hashicorp/net-rpc-msgpackrpc"
+	"github.com/hashicorp/nomad/acl"
+	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/testutil"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestStatusVersion(t *testing.T) {
@@ -94,3 +97,75 @@ func TestStatusPeers(t *testing.T) {
 		t.Fatalf("no peers: %v", peers)
 	}
 }
+
+func TestStatusMembers(t *testing.T) {
+	t.Parallel()
+	s1 := testServer(t, nil)
+	defer s1.Shutdown()
+	codec := rpcClient(t, s1)
+	assert := assert.New(t)
+
+	arg := &structs.GenericRequest{
+		QueryOptions: structs.QueryOptions{
+			Region:     "global",
+			AllowStale: true,
+		},
+	}
+
+	var out structs.ServerMembersResponse
+	assert.Nil(msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out))
+	assert.Len(out.Members, 1)
+}
+
+func TestStatusMembers_ACL(t *testing.T) {
+	t.Parallel()
+	s1, root := testACLServer(t, nil)
+	defer s1.Shutdown()
+	codec := rpcClient(t, s1)
+	assert := assert.New(t)
+	state := 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))
+
+	arg := &structs.GenericRequest{
+		QueryOptions: structs.QueryOptions{
+			Region:     "global",
+			AllowStale: true,
+		},
+	}
+
+	// Try without a token and expect failure
+	{
+		var out structs.ServerMembersResponse
+		err := msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out)
+		assert.NotNil(err)
+		assert.Equal(err.Error(), structs.ErrPermissionDenied.Error())
+	}
+
+	// Try with an invalid token and expect failure
+	{
+		arg.SecretID = invalidToken.SecretID
+		var out structs.ServerMembersResponse
+		err := msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out)
+		assert.NotNil(err)
+		assert.Equal(err.Error(), structs.ErrPermissionDenied.Error())
+	}
+
+	// Try with a valid token
+	{
+		arg.SecretID = validToken.SecretID
+		var out structs.ServerMembersResponse
+		assert.Nil(msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out))
+		assert.Len(out.Members, 1)
+	}
+
+	// Try with a management token
+	{
+		arg.SecretID = root.SecretID
+		var out structs.ServerMembersResponse
+		assert.Nil(msgpackrpc.CallWithCodec(codec, "Status.Members", arg, &out))
+		assert.Len(out.Members, 1)
+	}
+}