server: require admin role to access node status

Release note (security update): The node status retrieval endpoints
over HTTP (`/_status/nodes`, `/_status/nodes/<N>` and the web UI
`/#/reports/nodes`) have been updated to require the `admin` role from
the requesting user. This ensures that operational details such as
network addresses and command-line flags do not leak to unprivileged
users.

pkg/server/admin.go

@@ -22,7 +22,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/cockroachdb/apd/v2"
+	apd "github.com/cockroachdb/apd/v2"
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/config/zonepb"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
@@ -2070,7 +2070,7 @@ func (s *adminServer) DecommissionStatus(
 
 	// If no nodeIDs given, use all nodes.
 	if len(nodeIDs) == 0 {
-		ns, err := s.server.status.Nodes(ctx, &serverpb.NodesRequest{})
+		ns, err := s.server.status.ListNodesInternal(ctx, &serverpb.NodesRequest{})
 		if err != nil {
 			return nil, errors.Wrap(err, "loading node statuses")
 		}

pkg/server/serverpb/status.go

@@ -49,10 +49,11 @@ func MakeOptionalNodesStatusServer(s NodesStatusServer) OptionalNodesStatusServe
 	}
 }
 
-// NodesStatusServer is the subset of the serverpb.StatusInterface that is used
-// by the SQL subsystem but is unavailable to tenants.
+// NodesStatusServer is an endpoint that allows the SQL subsystem
+// to observe node descriptors.
+// It is unavailable to tenants.
 type NodesStatusServer interface {
-	Nodes(context.Context, *NodesRequest) (*NodesResponse, error)
+	ListNodesInternal(context.Context, *NodesRequest) (*NodesResponse, error)
 }
 
 // RegionsServer is the subset of the serverpb.StatusInterface that is used

pkg/server/status.go

@@ -67,7 +67,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb"
 	"github.com/cockroachdb/errors"
 	gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime"
-	"go.etcd.io/etcd/raft/v3"
+	raft "go.etcd.io/etcd/raft/v3"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
@@ -1344,6 +1344,10 @@ func regionsResponseFromNodesResponse(nr *serverpb.NodesResponse) *serverpb.Regi
 
 // Nodes returns all node statuses.
 //
+// Do not use this method inside the server code! Use
+// ListNodesInternal() instead.
+// This method here is the one exposed to network clients over HTTP.
+//
 // The LivenessByNodeID in the response returns the known liveness
 // information according to gossip. Nodes for which there is no gossip
 // information will not have an entry. Clients can exploit the fact
@@ -1352,7 +1356,22 @@ func regionsResponseFromNodesResponse(nr *serverpb.NodesResponse) *serverpb.Regi
 func (s *statusServer) Nodes(
 	ctx context.Context, req *serverpb.NodesRequest,
 ) (*serverpb.NodesResponse, error) {
-	resp, _, err := s.nodesHelper(ctx, 0 /* limit */, 0 /* offset */)
+	// The node status contains details about the command line, network
+	// addresses, env vars etc which are admin-only.
+	if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil {
+		return nil, err
+	}
+
+	resp, _, err := s.nodesHelper(ctx, 0, 0)
+	return resp, err
+}
+
+// ListNodesInternal is a helper function for the benefit of SQL exclusively.
+// It skips the privilege check, assuming that SQL is doing privilege checking already.
+func (s *statusServer) ListNodesInternal(
+	ctx context.Context, req *serverpb.NodesRequest,
+) (*serverpb.NodesResponse, error) {
+	resp, _, err := s.nodesHelper(ctx, 0, 0)
 	return resp, err
 }
 
@@ -1361,6 +1380,7 @@ func (s *statusServer) nodesHelper(
 ) (*serverpb.NodesResponse, int, error) {
 	ctx = propagateGatewayMetadata(ctx)
 	ctx = s.AnnotateCtx(ctx)
+
 	startKey := keys.StatusNodePrefix
 	endKey := startKey.PrefixEnd()
 
@@ -1399,7 +1419,7 @@ func (s *statusServer) nodesHelper(
 func (s *statusServer) nodesStatusWithLiveness(
 	ctx context.Context,
 ) (map[roachpb.NodeID]nodeStatusWithLiveness, error) {
-	nodes, err := s.Nodes(ctx, nil)
+	nodes, err := s.ListNodesInternal(ctx, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -1433,6 +1453,13 @@ func (s *statusServer) Node(
 ) (*statuspb.NodeStatus, error) {
 	ctx = propagateGatewayMetadata(ctx)
 	ctx = s.AnnotateCtx(ctx)
+
+	// The node status contains details about the command line, network
+	// addresses, env vars etc which are admin-only.
+	if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil {
+		return nil, err
+	}
+
 	nodeID, _, err := s.parseNodeID(req.NodeId)
 	if err != nil {
 		return nil, status.Errorf(codes.InvalidArgument, err.Error())
@@ -1487,7 +1514,7 @@ func (s *statusServer) RaftDebug(
 		return nil, err
 	}
 
-	nodes, err := s.Nodes(ctx, nil)
+	nodes, err := s.ListNodesInternal(ctx, nil)
 	if err != nil {
 		return nil, err
 	}

pkg/server/status_test.go

@@ -786,8 +786,14 @@ func TestNodeStatusResponse(t *testing.T) {
 	s := startServer(t)
 	defer s.Stopper().Stop(context.Background())
 
-	// First fetch all the node statuses.
 	wrapper := serverpb.NodesResponse{}
+
+	// Check that the node statuses cannot be accessed via a non-admin account.
+	if err := getStatusJSONProtoWithAdminOption(s, "nodes", &wrapper, false /* isAdmin */); !testutils.IsError(err, "status: 403") {
+		t.Fatalf("expected privilege error, got %v", err)
+	}
+
+	// Now fetch all the node statuses as admin.
 	if err := getStatusJSONProto(s, "nodes", &wrapper); err != nil {
 		t.Fatal(err)
 	}
@@ -804,7 +810,14 @@ func TestNodeStatusResponse(t *testing.T) {
 	// ids only.
 	for _, oldNodeStatus := range nodeStatuses {
 		nodeStatus := statuspb.NodeStatus{}
-		if err := getStatusJSONProto(s, "nodes/"+oldNodeStatus.Desc.NodeID.String(), &nodeStatus); err != nil {
+		nodeURL := "nodes/" + oldNodeStatus.Desc.NodeID.String()
+		// Check that the node statuses cannot be accessed via a non-admin account.
+		if err := getStatusJSONProtoWithAdminOption(s, nodeURL, &nodeStatus, false /* isAdmin */); !testutils.IsError(err, "status: 403") {
+			t.Fatalf("expected privilege error, got %v", err)
+		}
+
+		// Now access that node's status.
+		if err := getStatusJSONProto(s, nodeURL, &nodeStatus); err != nil {
 			t.Fatal(err)
 		}
 		if !s.node.Descriptor.Equal(&nodeStatus.Desc) {

pkg/sql/crdb_internal.go

@@ -3648,7 +3648,7 @@ CREATE TABLE crdb_internal.gossip_liveness (
 	populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
 		// ATTENTION: The contents of this table should only access gossip data
 		// which is highly available. DO NOT CALL functions which require the
-		// cluster to be healthy, such as NodesStatusServer.Nodes().
+		// cluster to be healthy, such as NodesStatusServer.ListNodesInternal().
 
 		if err := p.RequireAdminRole(ctx, "read crdb_internal.gossip_liveness"); err != nil {
 			return err
@@ -4080,7 +4080,7 @@ CREATE TABLE crdb_internal.kv_node_status (
 		if err != nil {
 			return err
 		}
-		response, err := ss.Nodes(ctx, &serverpb.NodesRequest{})
+		response, err := ss.ListNodesInternal(ctx, &serverpb.NodesRequest{})
 		if err != nil {
 			return err
 		}
@@ -4195,7 +4195,7 @@ CREATE TABLE crdb_internal.kv_store_status (
 		if err != nil {
 			return err
 		}
-		response, err := ss.Nodes(ctx, &serverpb.NodesRequest{})
+		response, err := ss.ListNodesInternal(ctx, &serverpb.NodesRequest{})
 		if err != nil {
 			return err
 		}

pkg/sql/distsql_plan_bulk.go

@@ -30,7 +30,7 @@ func (dsp *DistSQLPlanner) SetupAllNodesPlanning(
 	if err != nil {
 		return planCtx, []roachpb.NodeID{dsp.gatewayNodeID}, nil //nolint:returnerrcheck
 	}
-	resp, err := ss.Nodes(ctx, &serverpb.NodesRequest{})
+	resp, err := ss.ListNodesInternal(ctx, &serverpb.NodesRequest{})
 	if err != nil {
 		return nil, nil, err
 	}

pkg/sql/set_zone_config.go

@@ -39,7 +39,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
 	"github.com/gogo/protobuf/proto"
-	"gopkg.in/yaml.v2"
+	yaml "gopkg.in/yaml.v2"
 )
 
 type optionValue struct {
@@ -681,7 +681,7 @@ func (n *setZoneConfigNode) startExec(params runParams) error {
 			// Validate that the result makes sense.
 			if err := validateZoneAttrsAndLocalities(
 				params.ctx,
-				ss.Nodes,
+				ss.ListNodesInternal,
 				&newZone,
 			); err != nil {
 				return err