diff --git a/pkg/server/admin.go b/pkg/server/admin.go index 018e89d2cc58..98ee71ae4c4a 100644 --- a/pkg/server/admin.go +++ b/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") } diff --git a/pkg/server/serverpb/status.go b/pkg/server/serverpb/status.go index f6183ae6756c..754f5ef73461 100644 --- a/pkg/server/serverpb/status.go +++ b/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 diff --git a/pkg/server/status.go b/pkg/server/status.go index 1f1d06123463..15a4c27df6b8 100644 --- a/pkg/server/status.go +++ b/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,6 +1245,10 @@ 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 @@ -1252,9 +1256,24 @@ func (s *statusServer) Profile( // 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 } diff --git a/pkg/server/status_test.go b/pkg/server/status_test.go index 057072555e73..ab5dc7ba88d6 100644 --- a/pkg/server/status_test.go +++ b/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) { diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 4ee487442d90..139fc9725a57 100644 --- a/pkg/sql/crdb_internal.go +++ b/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 } diff --git a/pkg/sql/distsql_plan_bulk.go b/pkg/sql/distsql_plan_bulk.go index b680e20dbb05..82661f393151 100644 --- a/pkg/sql/distsql_plan_bulk.go +++ b/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 } diff --git a/pkg/sql/set_zone_config.go b/pkg/sql/set_zone_config.go index 5bb72c417956..f4345800ec0e 100644 --- a/pkg/sql/set_zone_config.go +++ b/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