From aeab727a6ba17b3f53d3cd6ccbd8fc7dc66d5ca5 Mon Sep 17 00:00:00 2001 From: Azhng Date: Mon, 11 Apr 2022 23:15:41 +0000 Subject: [PATCH] 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 | 27 ++++++++++++++++++++ pkg/sql/sem/builtins/builtins.go | 20 +++++++++++++-- pkg/sql/sem/builtins/builtins_test.go | 29 ---------------------- pkg/sql/sqlstats/sslocal/BUILD.bazel | 1 + pkg/sql/sqlstats/sslocal/sql_stats_test.go | 29 ++++++++++++++++++++++ 5 files changed, 75 insertions(+), 31 deletions(-) diff --git a/pkg/server/status_test.go b/pkg/server/status_test.go index 3f95ad69dcba..799ba2b9cc6f 100644 --- a/pkg/server/status_test.go +++ b/pkg/server/status_test.go @@ -3539,3 +3539,30 @@ func TestTransactionContentionEvents(t *testing.T) { }) } } + +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") +} diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 976561c7662d..74bb5ab0816b 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -6421,12 +6421,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") } @@ -6442,12 +6450,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") } diff --git a/pkg/sql/sem/builtins/builtins_test.go b/pkg/sql/sem/builtins/builtins_test.go index 964b311ced23..6687ca9fa7f9 100644 --- a/pkg/sql/sem/builtins/builtins_test.go +++ b/pkg/sql/sem/builtins/builtins_test.go @@ -469,35 +469,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)() diff --git a/pkg/sql/sqlstats/sslocal/BUILD.bazel b/pkg/sql/sqlstats/sslocal/BUILD.bazel index 766833fb35d9..8fb12789d4ab 100644 --- a/pkg/sql/sqlstats/sslocal/BUILD.bazel +++ b/pkg/sql/sqlstats/sslocal/BUILD.bazel @@ -52,6 +52,7 @@ go_test( "//pkg/settings", "//pkg/settings/cluster", "//pkg/sql", + "//pkg/sql/sessiondata", "//pkg/sql/sessionphase", "//pkg/sql/sqlstats", "//pkg/sql/sqlstats/persistedsqlstats", diff --git a/pkg/sql/sqlstats/sslocal/sql_stats_test.go b/pkg/sql/sqlstats/sslocal/sql_stats_test.go index b352c2898490..08b57e8a1d69 100644 --- a/pkg/sql/sqlstats/sslocal/sql_stats_test.go +++ b/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") +}