sql: enforce admin role for resetting sql stats and index usage stats

Resolves #79688

Previously, SQL Stats and Index Usage Stats can be reset through SQL CLI
using crdb_internal.reset_sql_stats() and
crdb_internal.reset_index_usage_stats() builtins. However, these two
builtins were not checking for users admin role. Hence, any user can
reset SQL Stats and Index Usage Stats.
This commit enforces the permission check.

Release note (security update): crdb_internal.reset_sql_stats() and
crdb_internal.reset_index_usage_stats() builtins now check if user has
admin role.

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")
+}