Merge pull request #80277 from cockroachdb/blathers/backport-release-…

…21.2-79810

release-21.2: sql: enforce admin role for resetting sql stats and index usage stats

pkg/server/status_test.go

@@ -48,6 +48,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
+	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats"
 	"github.com/cockroachdb/cockroach/pkg/sql/tests"
@@ -2907,3 +2908,30 @@ func TestSQLStatsAPIInMixedVersionState(t *testing.T) {
 			"SELECT count(*) FROM system.transaction_statistics", [][]string{{"0"}})
 	})
 }
+
+func TestUnprivilegedUserResetIndexUsageStats(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+
+	s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{})
+	defer s.Stopper().Stop(ctx)
+
+	sqlConn := sqlutils.MakeSQLRunner(conn)
+	sqlConn.Exec(t, "CREATE USER nonAdminUser")
+
+	ie := s.InternalExecutor().(*sql.InternalExecutor)
+
+	_, err := ie.ExecEx(
+		ctx,
+		"test-reset-index-usage-stats-as-non-admin-user",
+		nil, /* txn */
+		sessiondata.InternalExecutorOverride{
+			User: security.MakeSQLUsernameFromPreNormalizedString("nonAdminUser"),
+		},
+		"SELECT crdb_internal.reset_index_usage_stats()",
+	)
+
+	require.Contains(t, err.Error(), "requires admin privilege")
+}

pkg/sql/sem/builtins/builtins.go

@@ -6127,12 +6127,20 @@ table's zone configuration this will return NULL.`,
 	),
 	"crdb_internal.reset_index_usage_stats": makeBuiltin(
 		tree.FunctionProperties{
-			Category: categorySystemInfo,
+			Category:         categorySystemInfo,
+			DistsqlBlocklist: true, // applicable only on the gateway
 		},
 		tree.Overload{
 			Types:      tree.ArgTypes{},
 			ReturnType: tree.FixedReturnType(types.Bool),
 			Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
+				isAdmin, err := evalCtx.SessionAccessor.HasAdminRole(evalCtx.Ctx())
+				if err != nil {
+					return nil, err
+				}
+				if !isAdmin {
+					return nil, errors.New("crdb_internal.reset_index_usage_stats() requires admin privilege")
+				}
 				if evalCtx.IndexUsageStatsController == nil {
 					return nil, errors.AssertionFailedf("index usage stats controller not set")
 				}
@@ -6148,12 +6156,20 @@ table's zone configuration this will return NULL.`,
 	),
 	"crdb_internal.reset_sql_stats": makeBuiltin(
 		tree.FunctionProperties{
-			Category: categorySystemInfo,
+			Category:         categorySystemInfo,
+			DistsqlBlocklist: true, // applicable only on the gateway
 		},
 		tree.Overload{
 			Types:      tree.ArgTypes{},
 			ReturnType: tree.FixedReturnType(types.Bool),
 			Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
+				isAdmin, err := evalCtx.SessionAccessor.HasAdminRole(evalCtx.Ctx())
+				if err != nil {
+					return nil, err
+				}
+				if !isAdmin {
+					return nil, errors.New("crdb_internal.reset_sql_stats() requires admin privilege")
+				}
 				if evalCtx.SQLStatsController == nil {
 					return nil, errors.AssertionFailedf("sql stats controller not set")
 				}

pkg/sql/sem/builtins/builtins_test.go

@@ -12,18 +12,14 @@ package builtins
 
 import (
 	"bytes"
-	"context"
 	"fmt"
 	"math/rand"
 	"testing"
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
-	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
-	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/duration"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -370,35 +366,6 @@ func TestExtractTimeSpanFromTimeTZ(t *testing.T) {
 	}
 }
 
-// TestResetIndexUsageStatsOnRemoteSQLNode asserts that the built-in for
-// resetting index usage statistics works when it's being set up on a remote
-// node via DistSQL.
-func TestResetIndexUsageStatsOnRemoteSQLNode(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	ctx := context.Background()
-
-	testCluster := serverutils.StartNewTestCluster(t, 3 /* numNodes */, base.TestClusterArgs{
-		ServerArgs: base.TestServerArgs{},
-	})
-	defer testCluster.Stopper().Stop(ctx)
-	testConn := testCluster.ServerConn(2 /* idx */)
-	sqlDB := sqlutils.MakeSQLRunner(testConn)
-
-	query := `
-CREATE TABLE t (k INT PRIMARY KEY);
-INSERT INTO t SELECT generate_series(1, 30);
-
-ALTER TABLE t SPLIT AT VALUES (10), (20);
-ALTER TABLE t EXPERIMENTAL_RELOCATE LEASE SELECT 1, 1;
-ALTER TABLE t EXPERIMENTAL_RELOCATE LEASE SELECT 2, 15;
-ALTER TABLE t EXPERIMENTAL_RELOCATE LEASE SELECT 3, 25;
-
-SELECT count(*) FROM t WHERE crdb_internal.reset_index_usage_stats();
-`
-
-	sqlDB.Exec(t, query)
-}
-
 func TestExtractTimeSpanFromInterval(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 

pkg/sql/sqlstats/sslocal/BUILD.bazel

@@ -51,6 +51,7 @@ go_test(
         "//pkg/settings",
         "//pkg/settings/cluster",
         "//pkg/sql",
+        "//pkg/sql/sessiondata",
         "//pkg/sql/sessionphase",
         "//pkg/sql/sqlstats",
         "//pkg/sql/sqlstats/persistedsqlstats",

pkg/sql/sqlstats/sslocal/sql_stats_test.go

@@ -17,10 +17,12 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
+	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessionphase"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats"
@@ -640,3 +642,30 @@ WHERE
 `,
 		[][]string{{"0"}})
 }
+
+func TestUnprivilegedUserReset(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+
+	s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{})
+	defer s.Stopper().Stop(ctx)
+
+	sqlConn := sqlutils.MakeSQLRunner(conn)
+	sqlConn.Exec(t, "CREATE USER nonAdminUser")
+
+	ie := s.InternalExecutor().(*sql.InternalExecutor)
+
+	_, err := ie.ExecEx(
+		ctx,
+		"test-reset-sql-stats-as-non-admin-user",
+		nil, /* txn */
+		sessiondata.InternalExecutorOverride{
+			User: security.MakeSQLUsernameFromPreNormalizedString("nonAdminUser"),
+		},
+		"SELECT crdb_internal.reset_sql_stats()",
+	)
+
+	require.Contains(t, err.Error(), "requires admin privilege")
+}