From 6a14da494cd9faea9fe71297594bc53722c6684d Mon Sep 17 00:00:00 2001 From: gtr Date: Tue, 18 Jul 2023 10:17:37 -0400 Subject: [PATCH] sql: add system privileges to error messages in statement stats tables This commit implements the VIEWACTIVITY and VIEWACTIVITYREDACTED system privileges to the crdb_internal.node_statement_statistics table to provide fine-grained permissions to view the error message for failed statements. Release note (sql change): the crdb_interanal.node_statement_statistics table redact the error message if the user has VIEWACTIVITYREDACTED, and do not redact the error message if the user has VIEWACTIVITY. If the user has both, VIEWACTIVITYREDACTED takes precedence and the last error is redacted. --- .../tests/3node-tenant/generated_test.go | 7 ++ pkg/sql/crdb_internal.go | 30 ++++-- .../logic_test/statement_statistics_errors | 52 +++++----- .../statement_statistics_errors_redacted | 96 +++++++++++++++++++ .../tests/fakedist-disk/generated_test.go | 7 ++ .../tests/fakedist-vec-off/generated_test.go | 7 ++ .../tests/fakedist/generated_test.go | 7 ++ .../generated_test.go | 7 ++ .../local-mixed-22.2-23.1/generated_test.go | 7 ++ .../tests/local-vec-off/generated_test.go | 7 ++ .../logictest/tests/local/generated_test.go | 7 ++ 11 files changed, 202 insertions(+), 32 deletions(-) create mode 100644 pkg/sql/logictest/testdata/logic_test/statement_statistics_errors_redacted diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go index c059ea9dafee..dc70b65e5fdf 100644 --- a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go @@ -1886,6 +1886,13 @@ func TestTenantLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestTenantLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestTenantLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 0d88accd2a55..93cb66828d08 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -1516,12 +1516,24 @@ CREATE TABLE crdb_internal.node_statement_statistics ( latency_seconds_p99 FLOAT )`, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - hasViewActivityOrViewActivityRedacted, err := p.HasViewActivityOrViewActivityRedactedRole(ctx) - if err != nil { + shouldRedactError := false + // Check if the user is admin. + if isAdmin, err := p.HasAdminRole(ctx); err != nil { return err - } - if !hasViewActivityOrViewActivityRedacted { - return noViewActivityOrViewActivityRedactedRoleError(p.User()) + } else if !isAdmin { + // If the user is not admin, check the individual VIEWACTIVITY and VIEWACTIVITYREDACTED + // privileges. + if hasViewActivityRedacted, err := p.HasViewActivityRedacted(ctx); err != nil { + return err + } else if hasViewActivityRedacted { + shouldRedactError = true + } else if hasViewActivity, err := p.HasViewActivity(ctx); err != nil { + return err + } else if !hasViewActivity { + // If the user is not admin and does not have VIEWACTIVITY or VIEWACTIVITYREDACTED, + // return insufficient privileges error. + return noViewActivityOrViewActivityRedactedRoleError(p.User()) + } } var alloc tree.DatumAlloc @@ -1530,8 +1542,12 @@ CREATE TABLE crdb_internal.node_statement_statistics ( statementVisitor := func(_ context.Context, stats *appstatspb.CollectedStatementStatistics) error { errString := tree.DNull - if stats.Stats.SensitiveInfo.LastErr != "" { - errString = alloc.NewDString(tree.DString(stats.Stats.SensitiveInfo.LastErr)) + if shouldRedactError { + errString = alloc.NewDString(tree.DString("")) + } else { + if stats.Stats.SensitiveInfo.LastErr != "" { + errString = alloc.NewDString(tree.DString(stats.Stats.SensitiveInfo.LastErr)) + } } errCode := tree.DNull diff --git a/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors b/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors index d2329bb75d74..e1f1d123d4ee 100644 --- a/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors +++ b/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors @@ -6,14 +6,16 @@ # by CRDB ("$ internal-migration-manager-find-jobs" for example) since they can be flaky. We are # also ordering by "last_error_code" to maintain consistency. +user root + # Test 1: division by zero. Error code "22012" should be added. statement error division by zero SELECT 2/0; -query T -SELECT last_error_code FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; +query TT +SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; ---- -22012 +22012 division by zero # Test 2: database does not exist. Error code "3D000" should be added. query TTTTTT colnames,rowsort @@ -28,34 +30,34 @@ test root NULL NULL {} NULL statement error pq: database "posgres" does not exist use posgres -query T -SELECT last_error_code FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; +query TT +SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; ---- -22012 -3D000 +22012 division by zero +3D000 database "posgres" does not exist # Test 3: Nonexistant user. Error code "42704" should be added. statement error pq: role/user "who" does not exist ALTER DEFAULT PRIVILEGES GRANT SELECT ON TABLES to who -query T -SELECT last_error_code FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; +query TT +SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; ---- -22012 -3D000 -42704 +22012 division by zero +3D000 database "posgres" does not exist +42704 role/user "who" does not exist # Test 4: Insufficient privilege. Error code "42501" should be added. statement error schema cannot be modified: "crdb_internal" CREATE TABLE crdb_internal.example (abc INT) -query T -SELECT last_error_code FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; +query TT +SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; ---- -22012 -3D000 -42501 -42704 +22012 division by zero +3D000 database "posgres" does not exist +42501 schema cannot be modified: "crdb_internal" +42704 role/user "who" does not exist # Test 5: Foreign key violation. Error code "23503" should be added. statement ok @@ -67,11 +69,11 @@ INSERT INTO dst(x) VALUES ('example') statement error foreign key UPDATE dst SET x = 'xyz' -query T -SELECT last_error_code FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; +query TT +SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; ---- -22012 -23503 -3D000 -42501 -42704 +22012 division by zero +23503 update on table "dst" violates foreign key constraint "dst_x_fkey" +3D000 database "posgres" does not exist +42501 schema cannot be modified: "crdb_internal" +42704 role/user "who" does not exist diff --git a/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors_redacted b/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors_redacted new file mode 100644 index 000000000000..efaca48b2189 --- /dev/null +++ b/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors_redacted @@ -0,0 +1,96 @@ +# Check that error messages and error codes are being surfaced correctly +# +# For these tests, a statement containing an intentional error is made and the +# crdb_internal.node_statement_statistics virtual table is queried to validate that the error code +# is recorded. When querying the node statistics table, we are ignoring statements made internally +# by CRDB ("$ internal-migration-manager-find-jobs" for example) since they can be flaky. We are +# also ordering by "last_error_code" to maintain consistency. Here we are also testing that the +# error message is redacted for the VIEWACTIVITYREDACTED privilege. + + +# Grant testuser the VIEWACTIVITYREDACTED system privilege. +statement ok +GRANT SYSTEM VIEWACTIVITYREDACTED TO testuser; + +query TTB +SHOW SYSTEM GRANTS +---- +testuser VIEWACTIVITYREDACTED false + + +# Switch to testuser +user testuser + +# Test 1: division by zero. Error code "22012" should be added. +statement error division by zero +SELECT 2/0; + +query TT +SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; +---- +22012 + +# Test 2: database does not exist. Error code "3D000" should be added. +query TTTTTT colnames,rowsort +SHOW DATABASES +---- +database_name owner primary_region secondary_region regions survival_goal +defaultdb root NULL NULL {} NULL +postgres root NULL NULL {} NULL +test root NULL NULL {} NULL + +statement error pq: database "posgres" does not exist +use posgres + +query TT +SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; +---- +22012 +3D000 + +# Test 3: Nonexistant user. Error code "42704" should be added. +statement error pq: role/user "who" does not exist +ALTER DEFAULT PRIVILEGES GRANT SELECT ON TABLES to who + +query TT +SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; +---- +22012 +3D000 +42704 + +# Test 4: Give testuser the VIEWACTIVITY system privilege and remove the VIEWACTIVITYREDACTED privilege. +user root + +statement ok +GRANT SYSTEM VIEWACTIVITY TO testuser + +statement ok +REVOKE SYSTEM VIEWACTIVITYREDACTED FROM testuser + +user testuser + +# Now the error message should not be redacted. + +query TT +SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; +---- +22012 division by zero +3D000 database "posgres" does not exist +42704 role/user "who" does not exist + +# Test 5: Give tesuser the VIEWACTIVITYREDACTED privilege again. This time, the error message should be redacted, +# as VIEWACTIVITYREDACTED takes precedence over VIEWACTIVITY. +user root + +statement ok +GRANT SYSTEM VIEWACTIVITYREDACTED TO testuser + +user testuser + +query TT +SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC; +---- +22012 +3D000 +42704 diff --git a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go index ab65395c96b6..bad463f4afc6 100644 --- a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go @@ -1864,6 +1864,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_storing( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go index 3720fab4b21e..dd8abee4ed88 100644 --- a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go @@ -1864,6 +1864,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist/generated_test.go b/pkg/sql/logictest/tests/fakedist/generated_test.go index 6d79c2e6e3b9..8a4d446e58cd 100644 --- a/pkg/sql/logictest/tests/fakedist/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist/generated_test.go @@ -1878,6 +1878,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go index 034a0e06ecd1..916df7689581 100644 --- a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go +++ b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go @@ -1843,6 +1843,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go b/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go index 8fa551abc51b..47dd1b73f21e 100644 --- a/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go +++ b/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go @@ -1822,6 +1822,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-vec-off/generated_test.go b/pkg/sql/logictest/tests/local-vec-off/generated_test.go index 70a53f3fb97d..a48735ccf560 100644 --- a/pkg/sql/logictest/tests/local-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/local-vec-off/generated_test.go @@ -1871,6 +1871,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local/generated_test.go b/pkg/sql/logictest/tests/local/generated_test.go index 165c495c46fe..6f1523c8ce03 100644 --- a/pkg/sql/logictest/tests/local/generated_test.go +++ b/pkg/sql/logictest/tests/local/generated_test.go @@ -2067,6 +2067,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) {