diff --git a/pkg/server/admin.go b/pkg/server/admin.go index 02eba04d61ac..4eb652c9d200 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -1478,23 +1478,23 @@ func (s *adminServer) Events( ) (_ *serverpb.EventsResponse, retErr error) { ctx = s.AnnotateCtx(ctx) - err := s.privilegeChecker.RequireViewClusterMetadataPermission(ctx) - if err != nil { + if err := s.privilegeChecker.RequireViewEventLogPermission(ctx); err != nil { // NB: not using srverrors.ServerError() here since the priv checker // already returns a proper gRPC error status. return nil, err } redactEvents := !req.UnredactedEvents + userName, err := authserver.UserFromIncomingRPCContext(ctx) + if err != nil { + return nil, srverrors.ServerError(ctx, err) + } + limit := req.Limit if limit == 0 { limit = apiconstants.DefaultAPIEventLimit } - userName, err := authserver.UserFromIncomingRPCContext(ctx) - if err != nil { - return nil, srverrors.ServerError(ctx, err) - } r, err := s.eventsHelper(ctx, req, userName, int(limit), 0, redactEvents) if err != nil { return nil, srverrors.ServerError(ctx, err) diff --git a/pkg/server/privchecker/BUILD.bazel b/pkg/server/privchecker/BUILD.bazel index 0ce2188b5d56..dd43b21c5708 100644 --- a/pkg/server/privchecker/BUILD.bazel +++ b/pkg/server/privchecker/BUILD.bazel @@ -39,6 +39,7 @@ go_test( "//pkg/server", "//pkg/sql", "//pkg/sql/isql", + "//pkg/sql/privilege", "//pkg/testutils/serverutils", "//pkg/testutils/sqlutils", "//pkg/testutils/testcluster", diff --git a/pkg/server/privchecker/api.go b/pkg/server/privchecker/api.go index 6a0d2ed173e0..06011e1601da 100644 --- a/pkg/server/privchecker/api.go +++ b/pkg/server/privchecker/api.go @@ -44,6 +44,7 @@ type CheckerForRPCHandlers interface { RequireViewClusterMetadataPermission(ctx context.Context) error RequireRepairClusterPermission(ctx context.Context) error RequireViewDebugPermission(ctx context.Context) error + RequireViewEventLogPermission(ctx context.Context) error } // SQLPrivilegeChecker is the part of the privilege checker that can diff --git a/pkg/server/privchecker/privchecker.go b/pkg/server/privchecker/privchecker.go index c795d254e7ad..ad46fb46dae4 100644 --- a/pkg/server/privchecker/privchecker.go +++ b/pkg/server/privchecker/privchecker.go @@ -190,6 +190,33 @@ func (c *adminPrivilegeChecker) RequireViewDebugPermission(ctx context.Context) privilege.VIEWDEBUG.DisplayName()) } +// RequireViewEventLogPermission requires the user have admin or the +// VIEWEVENTLOG or VIEWCLUSTERMETADATA system privilege and returns an +// error if the user does not have any. +func (c *adminPrivilegeChecker) RequireViewEventLogPermission(ctx context.Context) (err error) { + userName, isAdmin, err := c.GetUserAndRole(ctx) + if err != nil { + return srverrors.ServerError(ctx, err) + } + if isAdmin { + return nil + } + if hasViewEventLog, err := c.HasGlobalPrivilege(ctx, userName, privilege.VIEWEVENTLOG); err != nil { + return srverrors.ServerError(ctx, err) + } else if hasViewEventLog { + return nil + } + if hasViewClusterMetadata, err := c.HasGlobalPrivilege(ctx, userName, privilege.VIEWCLUSTERMETADATA); err != nil { + return srverrors.ServerError(ctx, err) + } else if hasViewClusterMetadata { + return nil + } + return grpcstatus.Errorf( + codes.PermissionDenied, "this operation requires the %s or %s system privilege", + privilege.VIEWEVENTLOG.DisplayName(), + privilege.VIEWCLUSTERMETADATA.DisplayName()) +} + // GetUserAndRole is part of the CheckerForRPCHandlers interface. func (c *adminPrivilegeChecker) GetUserAndRole( ctx context.Context, diff --git a/pkg/server/privchecker/privchecker_test.go b/pkg/server/privchecker/privchecker_test.go index 6e4c49093604..14d849a6fcdb 100644 --- a/pkg/server/privchecker/privchecker_test.go +++ b/pkg/server/privchecker/privchecker_test.go @@ -15,6 +15,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/server/privchecker" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/isql" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" @@ -56,6 +57,10 @@ func TestAdminPrivilegeChecker(t *testing.T) { sqlDB.Exec(t, "GRANT SYSTEM VIEWCLUSTERMETADATA TO withviewclustermetadata") sqlDB.Exec(t, "CREATE USER withviewdebug") sqlDB.Exec(t, "GRANT SYSTEM VIEWDEBUG TO withviewdebug") + sqlDB.Exec(t, "CREATE USER withviewjob") + sqlDB.Exec(t, "GRANT SYSTEM VIEWJOB TO withviewjob") + sqlDB.Exec(t, "CREATE USER withvieweventlog") + sqlDB.Exec(t, "GRANT SYSTEM VIEWEVENTLOG TO withvieweventlog") execCfg := ts.ExecutorConfig().(sql.ExecutorConfig) kvDB := ts.DB() @@ -97,6 +102,28 @@ func TestAdminPrivilegeChecker(t *testing.T) { withVaAndRedactedGlobalPrivilege := username.MakeSQLUsernameFromPreNormalizedString("withvaandredactedglobalprivilege") withviewclustermetadata := username.MakeSQLUsernameFromPreNormalizedString("withviewclustermetadata") withViewDebug := username.MakeSQLUsernameFromPreNormalizedString("withviewdebug") + withViewJob := username.MakeSQLUsernameFromPreNormalizedString("withviewjob") + withViewEventLog := username.MakeSQLUsernameFromPreNormalizedString("withvieweventlog") + + // Test HasGlobalPrivilege for VIEWJOB and VIEWEVENTLOG. + globalPrivTests := []struct { + name string + privilege privilege.Kind + username username.SQLUsername + wantHas bool + }{ + {"viewjob-granted", privilege.VIEWJOB, withViewJob, true}, + {"viewjob-not-granted", privilege.VIEWJOB, withoutPrivs, false}, + {"vieweventlog-granted", privilege.VIEWEVENTLOG, withViewEventLog, true}, + {"vieweventlog-not-granted", privilege.VIEWEVENTLOG, withoutPrivs, false}, + } + for _, tt := range globalPrivTests { + t.Run(tt.name, func(t *testing.T) { + has, err := underTest.HasGlobalPrivilege(ctx, tt.username, tt.privilege) + require.NoError(t, err) + require.Equal(t, tt.wantHas, has) + }) + } tests := []struct { name string @@ -141,6 +168,13 @@ func TestAdminPrivilegeChecker(t *testing.T) { withAdmin: false, withoutPrivs: true, withViewDebug: false, }, }, + { + "requireViewEventLogPermission", + underTest.RequireViewEventLogPermission, + map[username.SQLUsername]bool{ + withAdmin: false, withoutPrivs: true, withViewEventLog: false, withviewclustermetadata: false, + }, + }, } for _, tt := range tests { diff --git a/pkg/sql/authorization.go b/pkg/sql/authorization.go index d86317513610..10da7afe959a 100644 --- a/pkg/sql/authorization.go +++ b/pkg/sql/authorization.go @@ -272,6 +272,32 @@ func (p *planner) CheckPrivilegeForUser( if hasViewSystemTablePriv { return nil } + // VIEWJOB grants implicit SELECT on system.scheduled_jobs and + // system.jobs, the two tables underlying SHOW SCHEDULES. + tableID := d.GetID() + if tableID == keys.ScheduledJobsTableID || tableID == keys.JobsTableID { + hasViewJob, err := p.HasPrivilege( + ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWJOB, user, + ) + if err != nil { + return err + } + if hasViewJob { + return nil + } + } + // VIEWEVENTLOG grants implicit SELECT on system.eventlog. + if tableID == keys.EventLogTableID { + hasViewEventLog, err := p.HasPrivilege( + ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWEVENTLOG, user, + ) + if err != nil { + return err + } + if hasViewEventLog { + return nil + } + } } } return insufficientPrivilegeError(user, privilegeKind, privilegeObject) diff --git a/pkg/sql/delegate/show_schedules.go b/pkg/sql/delegate/show_schedules.go index bff3197d3853..9ba439a9c9f3 100644 --- a/pkg/sql/delegate/show_schedules.go +++ b/pkg/sql/delegate/show_schedules.go @@ -10,14 +10,53 @@ import ( "strings" "github.com/cockroachdb/cockroach/pkg/jobs" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" ) // commandColumn converts executor execution arguments into jsonb representation. const commandColumn = `crdb_internal.pb_to_json('cockroach.jobs.jobspb.ExecutionArguments', execution_args, false, true)->'args'` func (d *delegator) delegateShowSchedules(n *tree.ShowSchedules) (tree.Statement, error) { + // Provide a clear privilege error upfront. Without this check, the + // delegated query would fail with "does not have SELECT privilege on + // relation scheduled_jobs", which doesn't tell the user what privilege + // they actually need. The authorization.go layer grants implicit SELECT + // on the underlying system tables when VIEWJOB or VIEWSYSTEMTABLE + // is held, so we let those users through. + cat := d.catalog + user := cat.GetCurrentUser() + globalPrivObj := syntheticprivilege.GlobalPrivilegeObject + + // processPrivError returns nil if the error is an InsufficientPrivilege + // error (meaning the user simply lacks the privilege), or the original + // error for any unexpected failure. + processPrivError := func(err error) error { + if pgerror.GetPGCode(err) == pgcode.InsufficientPrivilege { + return nil + } + return err + } + + if err := cat.CheckPrivilege(d.ctx, globalPrivObj, user, privilege.VIEWJOB); err != nil { + if unexpectedErr := processPrivError(err); unexpectedErr != nil { + return nil, unexpectedErr + } + // User lacks VIEWJOB; check VIEWSYSTEMTABLE as a fallback. + if err := cat.CheckPrivilege(d.ctx, globalPrivObj, user, privilege.VIEWSYSTEMTABLE); err != nil { + if unexpectedErr := processPrivError(err); unexpectedErr != nil { + return nil, unexpectedErr + } + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, + "user %s does not have %s system privilege", + user, privilege.VIEWJOB) + } + } + sqltelemetry.IncrementShowCounter(sqltelemetry.Schedules) columnExprs := []string{ diff --git a/pkg/sql/logictest/testdata/logic_test/show_eventlog_privilege b/pkg/sql/logictest/testdata/logic_test/show_eventlog_privilege new file mode 100644 index 000000000000..d9692e7c3c1c --- /dev/null +++ b/pkg/sql/logictest/testdata/logic_test/show_eventlog_privilege @@ -0,0 +1,62 @@ +# LogicTest: local + +# Verify that the VIEWEVENTLOG privilege grants access to system.eventlog. + +# testuser should not be able to read system.eventlog. +user testuser + +statement error user testuser does not have SELECT privilege on relation eventlog +SELECT * FROM system.eventlog LIMIT 0 + +user root + +# Grant VIEWEVENTLOG to testuser. +statement ok +GRANT SYSTEM VIEWEVENTLOG TO testuser + +user testuser + +# testuser should now be able to read system.eventlog. +statement ok +SELECT * FROM system.eventlog LIMIT 0 + +user root + +# Revoke VIEWEVENTLOG from testuser. +statement ok +REVOKE SYSTEM VIEWEVENTLOG FROM testuser + +user testuser + +# testuser should no longer be able to read system.eventlog. +statement error user testuser does not have SELECT privilege on relation eventlog +SELECT * FROM system.eventlog LIMIT 0 + +user root + +# Test that inheriting VIEWEVENTLOG through a role works. +statement ok +CREATE ROLE eventlogviewer + +statement ok +GRANT SYSTEM VIEWEVENTLOG TO eventlogviewer + +statement ok +GRANT eventlogviewer TO testuser + +user testuser + +# testuser should be able to read system.eventlog through role membership. +statement ok +SELECT * FROM system.eventlog LIMIT 0 + +user root + +statement ok +REVOKE eventlogviewer FROM testuser + +user testuser + +# testuser should no longer be able to read system.eventlog. +statement error user testuser does not have SELECT privilege on relation eventlog +SELECT * FROM system.eventlog LIMIT 0 diff --git a/pkg/sql/logictest/testdata/logic_test/show_schedules_privilege b/pkg/sql/logictest/testdata/logic_test/show_schedules_privilege new file mode 100644 index 000000000000..b8cbe1002059 --- /dev/null +++ b/pkg/sql/logictest/testdata/logic_test/show_schedules_privilege @@ -0,0 +1,84 @@ +# LogicTest: local + +# Verify that SHOW SCHEDULES requires the VIEWJOB privilege. + +# testuser should not be able to SHOW SCHEDULES. +user testuser + +statement error user testuser does not have VIEWJOB system privilege +SHOW SCHEDULES + +user root + +# Grant VIEWJOB to testuser. +statement ok +GRANT SYSTEM VIEWJOB TO testuser + +user testuser + +# testuser should now be able to SHOW SCHEDULES. +statement ok +SHOW SCHEDULES + +user root + +# Revoke VIEWJOB from testuser. +statement ok +REVOKE SYSTEM VIEWJOB FROM testuser + +user testuser + +# testuser should no longer be able to SHOW SCHEDULES. +statement error user testuser does not have VIEWJOB system privilege +SHOW SCHEDULES + +user root + +# Test that inheriting VIEWJOB through a role works. +statement ok +CREATE ROLE scheduleviewer + +statement ok +GRANT SYSTEM VIEWJOB TO scheduleviewer + +statement ok +GRANT scheduleviewer TO testuser + +user testuser + +# testuser should be able to SHOW SCHEDULES through role membership. +statement ok +SHOW SCHEDULES + +user root + +statement ok +REVOKE scheduleviewer FROM testuser + +user testuser + +# testuser should no longer be able to SHOW SCHEDULES. +statement error user testuser does not have VIEWJOB system privilege +SHOW SCHEDULES + +user root + +# Test that VIEWSYSTEMTABLE also grants access to SHOW SCHEDULES (no +# regression for users who previously relied on this broader privilege). +statement ok +GRANT SYSTEM VIEWSYSTEMTABLE TO testuser + +user testuser + +statement ok +SHOW SCHEDULES + +user root + +statement ok +REVOKE SYSTEM VIEWSYSTEMTABLE FROM testuser + +user testuser + +statement error user testuser does not have VIEWJOB system privilege +SHOW SCHEDULES diff --git a/pkg/sql/logictest/tests/local/generated_test.go b/pkg/sql/logictest/tests/local/generated_test.go index 5b6dd19c526b..e2b56b3d6d17 100644 --- a/pkg/sql/logictest/tests/local/generated_test.go +++ b/pkg/sql/logictest/tests/local/generated_test.go @@ -2563,6 +2563,13 @@ func TestLogic_show_default_privileges( runLogicTest(t, "show_default_privileges") } +func TestLogic_show_eventlog_privilege( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "show_eventlog_privilege") +} + func TestLogic_show_external_connections( t *testing.T, ) { @@ -2612,6 +2619,13 @@ func TestLogic_show_ranges( runLogicTest(t, "show_ranges") } +func TestLogic_show_schedules_privilege( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "show_schedules_privilege") +} + func TestLogic_show_source( t *testing.T, ) { diff --git a/pkg/sql/privilege/kind.go b/pkg/sql/privilege/kind.go index f1a24ab66d84..aa49b5a274f0 100644 --- a/pkg/sql/privilege/kind.go +++ b/pkg/sql/privilege/kind.go @@ -73,7 +73,8 @@ const ( BUILTIN_UNSAFE_ALLOWED Kind = 42 MAINTAIN Kind = 43 TEMPORARY Kind = 44 - largestKind = TEMPORARY + VIEWEVENTLOG Kind = 45 + largestKind = VIEWEVENTLOG // RULE, SET, and ALTERSYSTEM are PostgreSQL ACL-only pseudo-privileges. // They exist solely for ACL character mapping used by acldefault, aclexplode, @@ -187,6 +188,8 @@ func (k Kind) InternalKey() KindInternalKey { return "MAINTAIN" case TEMPORARY: return "TEMPORARY" + case VIEWEVENTLOG: + return "VIEWEVENTLOG" case SET: return "SET" case ALTERSYSTEM: diff --git a/pkg/sql/privilege/privilege.go b/pkg/sql/privilege/privilege.go index 71b2e470f715..c9169b5d805b 100644 --- a/pkg/sql/privilege/privilege.go +++ b/pkg/sql/privilege/privilege.go @@ -101,7 +101,7 @@ var ( ALL, BACKUP, RESTORE, MODIFYCLUSTERSETTING, EXTERNALCONNECTION, VIEWACTIVITY, VIEWACTIVITYREDACTED, VIEWCLUSTERSETTING, CANCELQUERY, NOSQLLOGIN, VIEWCLUSTERMETADATA, VIEWDEBUG, EXTERNALIOIMPLICITACCESS, VIEWJOB, MODIFYSQLCLUSTERSETTING, REPLICATION, MANAGEVIRTUALCLUSTER, VIEWSYSTEMTABLE, CREATEROLE, CREATELOGIN, CREATEDB, CONTROLJOB, - REPAIRCLUSTER, BYPASSRLS, REPLICATIONDEST, REPLICATIONSOURCE, INSPECT, + REPAIRCLUSTER, BYPASSRLS, REPLICATIONDEST, REPLICATIONSOURCE, INSPECT, VIEWEVENTLOG, } VirtualTablePrivileges = List{ALL, SELECT} ExternalConnectionPrivileges = List{ALL, USAGE, DROP, UPDATE} diff --git a/pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts index 4ef59fe68499..3341a9dbce8b 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts @@ -12,6 +12,7 @@ import { executeInternalSql, LARGE_RESULT_SIZE, SqlExecutionRequest, + sqlApiErrorMessage, sqlResultsAreEmpty, } from "./sqlApi"; @@ -78,6 +79,9 @@ export function getSchedules(req: { execute: true, }; return executeInternalSql(request).then(result => { + if (result.error) { + throw new Error(sqlApiErrorMessage(result.error.message)); + } const txnResults = result.execution.txn_results; if (sqlResultsAreEmpty(result)) { // No data. @@ -122,6 +126,9 @@ export function getSchedule(id: Long): Promise { execute: true, }; return executeInternalSql(request).then(result => { + if (result.error) { + throw new Error(sqlApiErrorMessage(result.error.message)); + } const txnResults = result.execution.txn_results; if (txnResults.length === 0 || !txnResults[0].rows) { // No data.