Merge #42563

42563: server: properly authenticate some admin RPC requests r=knz a=knz

Needed for #42520.
Fixes the vulnerability described in #42567 (but does not durably prevent it from creeping back, so more work is needed to fully close the issue).

The the individual commits for details.


Co-authored-by: Raphael 'kena' Poss <[email protected]>

docs/generated/sql/functions.md

@@ -1022,6 +1022,8 @@ SELECT * FROM crdb_internal.check_consistency(true, ‘\x02’, ‘\x04’)</p>
 </span></td></tr>
 <tr><td><a name="crdb_internal.force_retry"></a><code>crdb_internal.force_retry(val: <a href="interval.html">interval</a>) &rarr; <a href="int.html">int</a></code></td><td><span class="funcdesc"><p>This function is used only by CockroachDB’s developers for testing purposes.</p>
 </span></td></tr>
+<tr><td><a name="crdb_internal.is_admin"></a><code>crdb_internal.is_admin() &rarr; <a href="bool.html">bool</a></code></td><td><span class="funcdesc"><p>Retrieves the current user’s admin status.</p>
+</span></td></tr>
 <tr><td><a name="crdb_internal.json_num_index_entries"></a><code>crdb_internal.json_num_index_entries(val: jsonb) &rarr; <a href="int.html">int</a></code></td><td><span class="funcdesc"><p>This function is used only by CockroachDB’s developers for testing purposes.</p>
 </span></td></tr>
 <tr><td><a name="crdb_internal.lease_holder"></a><code>crdb_internal.lease_holder(key: <a href="bytes.html">bytes</a>) &rarr; <a href="int.html">int</a></code></td><td><span class="funcdesc"><p>This function is used to fetch the leaseholder corresponding to a request key</p>

pkg/ccl/logictestccl/testdata/logic_test/role

@@ -219,6 +219,12 @@ user root
 statement ok
 GRANT admin TO testuser
 
+# Verify that is_admin reports the right value.
+query B
+SELECT crdb_internal.is_admin()
+----
+true
+
 # Dropping users/roles deletes all their memberships.
 query TTB colnames
 SELECT * FROM system.role_members
@@ -735,3 +741,5 @@ REVOKE public FROM testuser
 
 statement error user or role public does not exist
 REVOKE admin FROM public
+
+

pkg/server/admin.go

@@ -46,8 +46,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
+	"github.com/cockroachdb/errors"
 	gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime"
-	"github.com/pkg/errors"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -116,15 +116,6 @@ func (s *adminServer) RegisterGateway(
 	return serverpb.RegisterAdminHandler(ctx, mux, conn)
 }
 
-// getUserProto will return the authenticated user. For now, this is just a stub until we
-// figure out our authentication mechanism.
-//
-// TODO(cdo): Make this work when we have an authentication scheme for the
-// API.
-func (s *adminServer) getUser(_ protoutil.Message) string {
-	return security.RootUser
-}
-
 // serverError logs the provided error and returns an error that should be returned by
 // the RPC endpoint method.
 func (s *adminServer) serverError(err error) error {
@@ -189,8 +180,14 @@ func (s *adminServer) Databases(
 	ctx context.Context, req *serverpb.DatabasesRequest,
 ) (*serverpb.DatabasesResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
+
+	sessionUser, err := userFromContext(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	rows, _ /* cols */, err := s.server.internalExecutor.QueryWithUser(
-		ctx, "admi-show-db", nil /* txn */, s.getUser(req), "SHOW DATABASES",
+		ctx, "admin-show-dbs", nil /* txn */, sessionUser, "SHOW DATABASES",
 	)
 	if err != nil {
 		return nil, s.serverError(err)
@@ -215,7 +212,10 @@ func (s *adminServer) DatabaseDetails(
 	ctx context.Context, req *serverpb.DatabaseDetailsRequest,
 ) (*serverpb.DatabaseDetailsResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
-	userName := s.getUser(req)
+	userName, err := userFromContext(ctx)
+	if err != nil {
+		return nil, err
+	}
 
 	escDBName := tree.NameStringP(&req.Database)
 	// Placeholders don't work with SHOW statements, so we need to manually
@@ -330,7 +330,10 @@ func (s *adminServer) TableDetails(
 	ctx context.Context, req *serverpb.TableDetailsRequest,
 ) (*serverpb.TableDetailsResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
-	userName := s.getUser(req)
+	userName, err := userFromContext(ctx)
+	if err != nil {
+		return nil, err
+	}
 
 	escDBName := tree.NameStringP(&req.Database)
 	// TODO(cdo): Use real placeholders for the table and database names when we've extended our SQL
@@ -581,9 +584,16 @@ func generateTableSpan(tableID sqlbase.ID) roachpb.Span {
 func (s *adminServer) TableStats(
 	ctx context.Context, req *serverpb.TableStatsRequest,
 ) (*serverpb.TableStatsResponse, error) {
+	// TODO(someone): perform authorization based on the requesting user's
+	// SELECT privilege over the requested table.
+	userName, err := s.requireAdminUser(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	// Get table span.
 	path, err := s.queryDescriptorIDPath(
-		ctx, s.getUser(req), []string{req.Database, req.Table},
+		ctx, userName, []string{req.Database, req.Table},
 	)
 	if err != nil {
 		return nil, s.serverError(err)
@@ -599,6 +609,10 @@ func (s *adminServer) TableStats(
 func (s *adminServer) NonTableStats(
 	ctx context.Context, req *serverpb.NonTableStatsRequest,
 ) (*serverpb.NonTableStatsResponse, error) {
+	if _, err := s.requireAdminUser(ctx); err != nil {
+		return nil, err
+	}
+
 	timeSeriesStats, err := s.statsForSpan(ctx, roachpb.Span{
 		Key:    keys.TimeseriesPrefix,
 		EndKey: keys.TimeseriesPrefix.PrefixEnd(),
@@ -779,9 +793,13 @@ func (s *adminServer) Users(
 	ctx context.Context, req *serverpb.UsersRequest,
 ) (*serverpb.UsersResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
+	userName, err := userFromContext(ctx)
+	if err != nil {
+		return nil, err
+	}
 	query := `SELECT username FROM system.users WHERE "isRole" = false`
 	rows, _ /* cols */, err := s.server.internalExecutor.QueryWithUser(
-		ctx, "admin-users", nil /* txn */, s.getUser(req), query,
+		ctx, "admin-users", nil /* txn */, userName, query,
 	)
 	if err != nil {
 		return nil, s.serverError(err)
@@ -804,6 +822,16 @@ func (s *adminServer) Events(
 ) (*serverpb.EventsResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
 
+	userName, isAdmin, err := s.getUserAndRole(ctx)
+	if err != nil {
+		return nil, err
+	}
+	redactEvents := false
+	if isAdmin {
+		// We obey the redacted bit only if the user is admin.
+		redactEvents = !req.UnredactedEvents
+	}
+
 	limit := req.Limit
 	if limit == 0 {
 		limit = defaultAPIEventLimit
@@ -828,7 +856,7 @@ func (s *adminServer) Events(
 		return nil, s.serverErrors(q.Errors())
 	}
 	rows, cols, err := s.server.internalExecutor.QueryWithUser(
-		ctx, "admin-events", nil /* txn */, s.getUser(req), q.String(), q.QueryArguments()...)
+		ctx, "admin-events", nil /* txn */, userName, q.String(), q.QueryArguments()...)
 	if err != nil {
 		return nil, s.serverError(err)
 	}
@@ -856,10 +884,9 @@ func (s *adminServer) Events(
 			return nil, err
 		}
 		if event.EventType == string(sql.EventLogSetClusterSetting) {
-
-			// TODO: `if s.getUser(req) != security.RootUser` when we have auth.
-
-			event.Info = redactSettingsChange(event.Info)
+			if redactEvents {
+				event.Info = redactSettingsChange(event.Info)
+			}
 		}
 		if err := scanner.ScanIndex(row, 5, &event.UniqueID); err != nil {
 			return nil, err
@@ -890,6 +917,12 @@ func (s *adminServer) RangeLog(
 ) (*serverpb.RangeLogResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
 
+	// Range keys, even when pretty-printed, contain PII.
+	userName, err := s.requireAdminUser(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	limit := req.Limit
 	if limit == 0 {
 		limit = defaultAPIEventLimit
@@ -914,7 +947,7 @@ func (s *adminServer) RangeLog(
 	}
 	rows, cols, err := s.server.internalExecutor.QueryWithUser(
 		ctx, "admin-range-log",
-		nil /* txn */, s.getUser(req), q.String(), q.QueryArguments()...,
+		nil /* txn */, userName, q.String(), q.QueryArguments()...,
 	)
 	if err != nil {
 		return nil, s.serverError(err)
@@ -1062,6 +1095,11 @@ func (s *adminServer) SetUIData(
 ) (*serverpb.SetUIDataResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
 
+	userName, err := userFromContext(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	if len(req.KeyValues) == 0 {
 		return nil, status.Errorf(codes.InvalidArgument, "KeyValues cannot be empty")
 	}
@@ -1071,7 +1109,7 @@ func (s *adminServer) SetUIData(
 		// avoid long-running transactions and possible deadlocks.
 		query := `UPSERT INTO system.ui (key, value, "lastUpdated") VALUES ($1, $2, now())`
 		rowsAffected, err := s.server.internalExecutor.ExecWithUser(
-			ctx, "admin-set-ui-data", nil /* txn */, s.getUser(req), query, key, val)
+			ctx, "admin-set-ui-data", nil /* txn */, userName, query, key, val)
 		if err != nil {
 			return nil, s.serverError(err)
 		}
@@ -1093,11 +1131,17 @@ func (s *adminServer) GetUIData(
 	ctx context.Context, req *serverpb.GetUIDataRequest,
 ) (*serverpb.GetUIDataResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
+
+	userName, err := userFromContext(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	if len(req.Keys) == 0 {
 		return nil, status.Errorf(codes.InvalidArgument, "keys cannot be empty")
 	}
 
-	resp, err := s.getUIData(ctx, s.getUser(req), req.Keys)
+	resp, err := s.getUIData(ctx, userName, req.Keys)
 	if err != nil {
 		return nil, s.serverError(err)
 	}
@@ -1186,6 +1230,11 @@ func (s *adminServer) Jobs(
 ) (*serverpb.JobsResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
 
+	userName, err := userFromContext(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	q := makeSQLQuery()
 	q.Append(`
       SELECT job_id, job_type, description, statement, user_name, descriptor_ids, status,
@@ -1208,7 +1257,7 @@ func (s *adminServer) Jobs(
 		q.Append(" LIMIT $", tree.DInt(req.Limit))
 	}
 	rows, cols, err := s.server.internalExecutor.QueryWithUser(
-		ctx, "admin-jobs", nil /* txn */, s.getUser(req), q.String(), q.QueryArguments()...,
+		ctx, "admin-jobs", nil /* txn */, userName, q.String(), q.QueryArguments()...,
 	)
 	if err != nil {
 		return nil, s.serverError(err)
@@ -1268,10 +1317,15 @@ func (s *adminServer) Locations(
 ) (*serverpb.LocationsResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
 
+	userName, err := userFromContext(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	q := makeSQLQuery()
 	q.Append(`SELECT "localityKey", "localityValue", latitude, longitude FROM system.locations`)
 	rows, cols, err := s.server.internalExecutor.QueryWithUser(
-		ctx, "admin-locations", nil /* txn */, s.getUser(req), q.String(),
+		ctx, "admin-locations", nil /* txn */, userName, q.String(),
 	)
 	if err != nil {
 		return nil, s.serverError(err)
@@ -1306,6 +1360,11 @@ func (s *adminServer) QueryPlan(
 ) (*serverpb.QueryPlanResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
 
+	userName, err := userFromContext(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	// As long as there's only one query provided it's safe to construct the
 	// explain query.
 	stmts, err := parser.Parse(req.Query)
@@ -1320,7 +1379,7 @@ func (s *adminServer) QueryPlan(
 		"SELECT json FROM [EXPLAIN (DISTSQL) %s]",
 		strings.Trim(req.Query, ";"))
 	rows, _ /* cols */, err := s.server.internalExecutor.QueryWithUser(
-		ctx, "admin-query-plan", nil /* txn */, s.getUser(req), explain,
+		ctx, "admin-query-plan", nil /* txn */, userName, explain,
 	)
 	if err != nil {
 		return nil, s.serverError(err)
@@ -1502,18 +1561,26 @@ func (s *adminServer) Decommission(
 func (s *adminServer) DataDistribution(
 	ctx context.Context, req *serverpb.DataDistributionRequest,
 ) (*serverpb.DataDistributionResponse, error) {
+	if _, err := s.requireAdminUser(ctx); err != nil {
+		return nil, err
+	}
+
 	resp := &serverpb.DataDistributionResponse{
 		DatabaseInfo: make(map[string]serverpb.DataDistributionResponse_DatabaseInfo),
 		ZoneConfigs:  make(map[string]serverpb.DataDistributionResponse_ZoneConfig),
 	}
 
+	userName, err := userFromContext(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	// Get ids and names for databases and tables.
 	// Set up this structure in the response.
 
 	// This relies on crdb_internal.tables returning data even for newly added tables
 	// and deleted tables (as opposed to e.g. information_schema) because we are interested
 	// in the data for all ranges, not just ranges for visible tables.
-	userName := s.getUser(req)
 	tablesQuery := `SELECT name, table_id, database_name, drop_time FROM "".crdb_internal.tables WHERE schema_name = 'public'`
 	rows1, _ /* cols */, err := s.server.internalExecutor.QueryWithUser(
 		ctx, "admin-replica-matrix", nil /* txn */, userName, tablesQuery,
@@ -1661,6 +1728,10 @@ func (s *adminServer) DataDistribution(
 func (s *adminServer) EnqueueRange(
 	ctx context.Context, req *serverpb.EnqueueRangeRequest,
 ) (*serverpb.EnqueueRangeResponse, error) {
+	if _, err := s.requireAdminUser(ctx); err != nil {
+		return nil, err
+	}
+
 	if !debug.GatewayRemoteAllowed(ctx, s.server.ClusterSettings()) {
 		return nil, remoteDebuggingErr
 	}
@@ -2134,3 +2205,48 @@ func (s *adminServer) dialNode(
 	}
 	return serverpb.NewAdminClient(conn), nil
 }
+
+func (s *adminServer) requireAdminUser(ctx context.Context) (userName string, err error) {
+	userName, isAdmin, err := s.getUserAndRole(ctx)
+	if err != nil {
+		return "", err
+	}
+	if !isAdmin {
+		return "", errInsufficientPrivilege
+	}
+	return userName, nil
+}
+
+func (s *adminServer) getUserAndRole(
+	ctx context.Context,
+) (userName string, isAdmin bool, err error) {
+	userName, err = userFromContext(ctx)
+	if err != nil {
+		return "", false, err
+	}
+	isAdmin, err = s.hasAdminRole(ctx, userName)
+	return userName, isAdmin, err
+}
+
+func (s *adminServer) hasAdminRole(ctx context.Context, sessionUser string) (bool, error) {
+	if sessionUser == security.RootUser {
+		// Shortcut.
+		return true, nil
+	}
+	rows, cols, err := s.server.internalExecutor.QueryWithUser(
+		ctx, "check-is-admin",
+		nil /* txn */, sessionUser, "SELECT crdb_internal.is_admin()")
+	if err != nil {
+		return false, err
+	}
+	if len(rows) != 1 || len(cols) != 1 {
+		return false, errors.AssertionFailedf("hasAdminRole: expected 1 row, got %d", len(rows))
+	}
+	dbDatum, ok := tree.AsDBool(rows[0][0])
+	if !ok {
+		return false, errors.AssertionFailedf("hasAdminRole: expected bool, got %T", rows[0][0])
+	}
+	return bool(dbDatum), nil
+}
+
+var errInsufficientPrivilege = errors.New("this operation requires admin privilege")

pkg/server/admin_cluster_test.go

@@ -45,7 +45,7 @@ func TestAdminAPITableStats(t *testing.T) {
 	// Create clients (SQL, HTTP) connected to server 0.
 	db := tc.ServerConn(0)
 
-	client, err := server0.GetAuthenticatedHTTPClient()
+	client, err := server0.GetAdminAuthenticatedHTTPClient()
 	if err != nil {
 		t.Fatal(err)
 	}