Merge pull request #67069 from knz/backport20.2-67067

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"
@@ -1657,7 +1657,7 @@ func (s *adminServer) DecommissionStatus(
 ) (*serverpb.DecommissionStatusResponse, error) {
 	// Get the number of replicas on each node. We *may* not need all of them,
 	// but that would be more complicated than seems worth it right now.
-	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

@@ -43,10 +43,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)
 }
 
 // OptionalNodesStatusServer returns the wrapped NodesStatusServer, if it is

pkg/server/status.go

@@ -63,7 +63,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"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"
@@ -1245,16 +1245,35 @@ func (s *statusServer) Profile(
 
 // 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
 // that status "UNKNOWN" has value 0 (the default) when accessing the
 // map.
 func (s *statusServer) Nodes(
 	ctx context.Context, req *serverpb.NodesRequest,
+) (*serverpb.NodesResponse, error) {
+	// 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
+	}
+
+	return s.ListNodesInternal(ctx, req)
+}
+
+// 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) {
 	ctx = propagateGatewayMetadata(ctx)
 	ctx = s.AnnotateCtx(ctx)
+
 	startKey := keys.StatusNodePrefix
 	endKey := startKey.PrefixEnd()
 
@@ -1287,7 +1306,7 @@ func (s *statusServer) Nodes(
 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
 	}
@@ -1321,6 +1340,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, grpcstatus.Errorf(codes.InvalidArgument, err.Error())
@@ -1375,7 +1401,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

@@ -800,8 +800,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)
 	}
@@ -818,7 +824,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 !proto.Equal(&s.node.Descriptor, &nodeStatus.Desc) {

pkg/sql/crdb_internal.go

@@ -3028,7 +3028,7 @@ CREATE TABLE crdb_internal.gossip_liveness (
 	populate: func(ctx context.Context, p *planner, _ *dbdesc.Immutable, 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
@@ -3430,7 +3430,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
 		}
@@ -3544,7 +3544,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

@@ -29,7 +29,7 @@ func (dsp *DistSQLPlanner) SetupAllNodesPlanning(
 	if err != nil {
 		return nil, nil, err
 	}
-	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

@@ -38,7 +38,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 {
@@ -581,7 +581,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