From f754a679c2b73119ab82ab5769a1fdd4880d8a06 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 | 28 ++++++++++++++++++ pkg/sql/sem/builtins/builtins.go | 20 +++++++++++-- pkg/sql/sem/builtins/builtins_test.go | 33 ---------------------- pkg/sql/sqlstats/sslocal/BUILD.bazel | 1 + pkg/sql/sqlstats/sslocal/sql_stats_test.go | 29 +++++++++++++++++++ 5 files changed, 76 insertions(+), 35 deletions(-) diff --git a/pkg/server/status_test.go b/pkg/server/status_test.go index 1ca5700da405..464daa6eec6c 100644 --- a/pkg/server/status_test.go +++ b/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") +} diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 0d3784192d8f..737164db6ddc 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/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") } diff --git a/pkg/sql/sem/builtins/builtins_test.go b/pkg/sql/sem/builtins/builtins_test.go index a7e845f34dbb..744358359eca 100644 --- a/pkg/sql/sem/builtins/builtins_test.go +++ b/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)() diff --git a/pkg/sql/sqlstats/sslocal/BUILD.bazel b/pkg/sql/sqlstats/sslocal/BUILD.bazel index 25e8aafe58b4..b50b20f00757 100644 --- a/pkg/sql/sqlstats/sslocal/BUILD.bazel +++ b/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", diff --git a/pkg/sql/sqlstats/sslocal/sql_stats_test.go b/pkg/sql/sqlstats/sslocal/sql_stats_test.go index a1b3cd18679a..f89aa614e3c8 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") +}