Merge #84198

84198: sql, server: observability replace role options with system privileges r=Santamaura a=Santamaura

This patch introduces system privileges that exist currently as
role options. For observability they are the following: VIEWACTIVITY,
VIEWACTIVTYREDACTED, VIEWCLUSTERSETTING, CANCELQUERY and NOSQLLOGIN.
Users should now do GRANT SYSTEM `privilege` TO `role`; instead of
using role options.
Brand new system privileges for observability will be introduced in
a subsequent PR.
Any additional gating with these system privileges beyond the basic
ones made in this PR will be done in a subsequent PR.

Resolves #83843

Release note (sql change): introduced
VIEWACTIVITY, VIEWACTIVITYREDACTED,
VIEWCLUSTERSETTING, CANCELQUERY,
and NOSQLLOGIN as system privileges.

Co-authored-by: Santamaura <[email protected]>

Author: craig[bot]

pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant

@@ -8,9 +8,11 @@ SELECT count(distinct(node_id)), count(*)  FROM crdb_internal.node_runtime_info
 query IT
 SELECT node_id, name FROM crdb_internal.leases ORDER BY name
 ----
+0  defaultdb
 0  eventlog
 0  jobs
 0  locations
+0  privileges
 0  protected_ts_meta
 0  protected_ts_records
 0  role_members

pkg/server/BUILD.bazel

@@ -182,6 +182,7 @@ go_library(
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/pgwire/pgwirecancel",
         "//pkg/sql/physicalplan",
+        "//pkg/sql/privilege",
         "//pkg/sql/querycache",
         "//pkg/sql/rangeprober",
         "//pkg/sql/roleoption",
@@ -205,6 +206,7 @@ go_library(
         "//pkg/sql/sqlutil",
         "//pkg/sql/stats",
         "//pkg/sql/stmtdiagnostics",
+        "//pkg/sql/syntheticprivilege",
         "//pkg/sql/ttl/ttljob",
         "//pkg/sql/ttl/ttlschedule",
         "//pkg/sql/types",
@@ -416,6 +418,7 @@ go_test(
         "//pkg/sql/sem/catconstants",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sessiondata",
+        "//pkg/sql/sessiondatapb",
         "//pkg/sql/sqlstats",
         "//pkg/sql/sqlstats/persistedsqlstats",
         "//pkg/sql/tests",

pkg/server/admin.go

@@ -24,6 +24,7 @@ import (
 
 	"github.com/cockroachdb/apd/v3"
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/config/zonepb"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
@@ -46,10 +47,12 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
+	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
+	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
 	"github.com/cockroachdb/cockroach/pkg/ts/catalog"
 	"github.com/cockroachdb/cockroach/pkg/util/contextutil"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
@@ -1828,20 +1831,27 @@ func (s *adminServer) Settings(
 		// Non-root access cannot see the values in any case.
 		lookupPurpose = settings.LookupForReporting
 
-		hasView, err := s.hasRoleOption(ctx, user, roleoption.VIEWCLUSTERSETTING)
-		if err != nil {
-			return nil, err
-		}
-
-		hasModify, err := s.hasRoleOption(ctx, user, roleoption.MODIFYCLUSTERSETTING)
-		if err != nil {
-			return nil, err
+		hasView := false
+		hasModify := false
+		if s.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
+			hasView = s.checkHasSystemPrivilege(ctx, user, privilege.VIEWCLUSTERSETTING)
+			hasModify = s.checkHasSystemPrivilege(ctx, user, privilege.MODIFYCLUSTERSETTING)
 		}
-
 		if !hasModify && !hasView {
-			return nil, status.Errorf(
-				codes.PermissionDenied, "this operation requires either %s or %s role options",
-				roleoption.VIEWCLUSTERSETTING, roleoption.MODIFYCLUSTERSETTING)
+			hasView, err := s.hasRoleOption(ctx, user, roleoption.VIEWCLUSTERSETTING)
+			if err != nil {
+				return nil, err
+			}
+
+			hasModify, err := s.hasRoleOption(ctx, user, roleoption.MODIFYCLUSTERSETTING)
+			if err != nil {
+				return nil, err
+			}
+			if !hasModify && !hasView {
+				return nil, status.Errorf(
+					codes.PermissionDenied, "this operation requires either %s or %s system privileges",
+					privilege.VIEWCLUSTERSETTING, privilege.MODIFYCLUSTERSETTING)
+			}
 		}
 	}
 
@@ -3410,7 +3420,9 @@ func (s *adminServer) dialNode(
 // adminPrivilegeChecker is a helper struct to check whether given usernames
 // have admin privileges.
 type adminPrivilegeChecker struct {
-	ie *sql.InternalExecutor
+	ie          *sql.InternalExecutor
+	st          *cluster.Settings
+	makePlanner func(opName string) (interface{}, func())
 }
 
 // requireAdminUser's error return is a gRPC error.
@@ -3434,15 +3446,20 @@ func (c *adminPrivilegeChecker) requireViewActivityPermission(ctx context.Contex
 		return serverError(ctx, err)
 	}
 	if !isAdmin {
-		hasViewActivity, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITY)
-		if err != nil {
-			return serverError(ctx, err)
+		hasView := false
+		if c.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
+			hasView = c.checkHasSystemPrivilege(ctx, userName, privilege.VIEWACTIVITY)
 		}
-
-		if !hasViewActivity {
-			return status.Errorf(
-				codes.PermissionDenied, "this operation requires the %s role option",
-				roleoption.VIEWACTIVITY)
+		if !hasView {
+			hasView, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITY)
+			if err != nil {
+				return serverError(ctx, err)
+			}
+			if !hasView {
+				return status.Errorf(
+					codes.PermissionDenied, "this operation requires the %s system privilege",
+					roleoption.VIEWACTIVITY)
+			}
 		}
 	}
 	return nil
@@ -3457,20 +3474,24 @@ func (c *adminPrivilegeChecker) requireViewActivityOrViewActivityRedactedPermiss
 		return serverError(ctx, err)
 	}
 	if !isAdmin {
-		hasViewActivity, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITY)
-		if err != nil {
-			return serverError(ctx, err)
-		}
-
-		if !hasViewActivity {
-			hasViewActivityRedacted, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITYREDACTED)
+		hasView := false
+		hasViewRedacted := false
+		if c.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
+			hasView = c.checkHasSystemPrivilege(ctx, userName, privilege.VIEWACTIVITY)
+			hasViewRedacted = c.checkHasSystemPrivilege(ctx, userName, privilege.VIEWACTIVITYREDACTED)
+		}
+		if !hasView && !hasViewRedacted {
+			hasView, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITY)
 			if err != nil {
 				return serverError(ctx, err)
 			}
-
-			if !hasViewActivityRedacted {
+			hasViewRedacted, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITYREDACTED)
+			if err != nil {
+				return serverError(ctx, err)
+			}
+			if !hasView && !hasViewRedacted {
 				return status.Errorf(
-					codes.PermissionDenied, "this operation requires the %s or %s role options",
+					codes.PermissionDenied, "this operation requires the %s or %s system privileges",
 					roleoption.VIEWACTIVITY, roleoption.VIEWACTIVITYREDACTED)
 			}
 		}
@@ -3490,14 +3511,24 @@ func (c *adminPrivilegeChecker) requireViewActivityAndNoViewActivityRedactedPerm
 	}
 
 	if !isAdmin {
-		hasViewActivityRedacted, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITYREDACTED)
-		if err != nil {
-			return serverError(ctx, err)
+		hasViewRedacted := false
+		if c.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
+			hasViewRedacted = c.checkHasSystemPrivilege(ctx, userName, privilege.VIEWACTIVITYREDACTED)
 		}
-		if hasViewActivityRedacted {
+		if !hasViewRedacted {
+			hasViewRedacted, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITYREDACTED)
+			if err != nil {
+				return serverError(ctx, err)
+			}
+			if hasViewRedacted {
+				return status.Errorf(
+					codes.PermissionDenied, "this operation requires %s role option and is not allowed for %s role option",
+					roleoption.VIEWACTIVITY, roleoption.VIEWACTIVITYREDACTED)
+			}
+		} else {
 			return status.Errorf(
-				codes.PermissionDenied, "this operation requires %s role option and is not allowed for %s role option",
-				roleoption.VIEWACTIVITY, roleoption.VIEWACTIVITYREDACTED)
+				codes.PermissionDenied, "this operation requires %s system privilege and is not allowed for %s system privilege",
+				privilege.VIEWACTIVITY, privilege.VIEWACTIVITYREDACTED)
 		}
 		return c.requireViewActivityPermission(ctx)
 	}
@@ -3575,6 +3606,19 @@ func (c *adminPrivilegeChecker) hasRoleOption(
 	return bool(dbDatum), nil
 }
 
+// checkHasSystemPrivilege is a helper function which calls
+// CheckPrivilege and returns a true/false based on the returned
+// result.
+func (c *adminPrivilegeChecker) checkHasSystemPrivilege(
+	ctx context.Context, user username.SQLUsername, privilege privilege.Kind,
+) bool {
+	planner, cleanup := c.makePlanner("check-system-privilege")
+	defer cleanup()
+	aa := planner.(sql.AuthorizationAccessor)
+	err := aa.CheckPrivilegeForUser(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege, user)
+	return err == nil
+}
+
 var errRequiresAdmin = status.Error(codes.PermissionDenied, "this operation requires admin privilege")
 
 func errRequiresRoleOption(option roleoption.Option) error {

pkg/server/admin_test.go

@@ -28,6 +28,7 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/config/zonepb"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
@@ -48,6 +49,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/idxusage"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
@@ -2748,7 +2750,7 @@ func TestAdminPrivilegeChecker(t *testing.T) {
 	defer log.Scope(t).Close(t)
 
 	ctx := context.Background()
-	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
+	s, db, kvDB := serverutils.StartServer(t, base.TestServerArgs{
 		// Disable the default test tenant for now as this tests fails
 		// with it enabled. Tracked with #81590.
 		DisableDefaultTestTenant: true,
@@ -2767,8 +2769,26 @@ func TestAdminPrivilegeChecker(t *testing.T) {
 	sqlDB.Exec(t, "ALTER ROLE withvaandredacted WITH VIEWACTIVITYREDACTED")
 	sqlDB.Exec(t, "CREATE USER withoutprivs")
 
+	execCfg := s.ExecutorConfig().(sql.ExecutorConfig)
+
+	plannerFn := func(opName string) (interface{}, func()) {
+		// This is a hack to get around a Go package dependency cycle. See comment
+		// in sql/jobs/registry.go on planHookMaker.
+		txn := kvDB.NewTxn(ctx, "test")
+		return sql.NewInternalPlanner(
+			opName,
+			txn,
+			username.RootUserName(),
+			&sql.MemoryMetrics{},
+			&execCfg,
+			sessiondatapb.SessionData{},
+		)
+	}
+
 	underTest := &adminPrivilegeChecker{
-		ie: s.InternalExecutor().(*sql.InternalExecutor),
+		ie:          s.InternalExecutor().(*sql.InternalExecutor),
+		st:          s.ClusterSettings(),
+		makePlanner: plannerFn,
 	}
 
 	withAdmin, err := username.MakeSQLUsernameFromPreNormalizedStringChecked("withadmin")
@@ -2809,6 +2829,31 @@ func TestAdminPrivilegeChecker(t *testing.T) {
 			},
 		},
 	}
+	// test system privileges if valid version
+	if s.ClusterSettings().Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
+		sqlDB.Exec(t, "CREATE USER withvasystemprivilege")
+		sqlDB.Exec(t, "GRANT SYSTEM VIEWACTIVITY TO withvasystemprivilege")
+		sqlDB.Exec(t, "CREATE USER withvaredactedsystemprivilege")
+		sqlDB.Exec(t, "GRANT SYSTEM VIEWACTIVITYREDACTED TO withvaredactedsystemprivilege")
+		sqlDB.Exec(t, "CREATE USER withvaandredactedsystemprivilege")
+		sqlDB.Exec(t, "GRANT SYSTEM VIEWACTIVITY TO withvaandredactedsystemprivilege")
+		sqlDB.Exec(t, "GRANT SYSTEM VIEWACTIVITYREDACTED TO withvaandredactedsystemprivilege")
+
+		withVaSystemPrivilege := username.MakeSQLUsernameFromPreNormalizedString("withvasystemprivilege")
+		withVaRedactedSystemPrivilege := username.MakeSQLUsernameFromPreNormalizedString("withvaredactedsystemprivilege")
+		withVaAndRedactedSystemPrivilege := username.MakeSQLUsernameFromPreNormalizedString("withvaandredactedsystemprivilege")
+
+		tests[0].usernameWantErr[withVaSystemPrivilege] = false
+		tests[1].usernameWantErr[withVaSystemPrivilege] = false
+		tests[2].usernameWantErr[withVaSystemPrivilege] = false
+		tests[0].usernameWantErr[withVaRedactedSystemPrivilege] = true
+		tests[1].usernameWantErr[withVaRedactedSystemPrivilege] = false
+		tests[2].usernameWantErr[withVaRedactedSystemPrivilege] = true
+		tests[0].usernameWantErr[withVaAndRedactedSystemPrivilege] = false
+		tests[1].usernameWantErr[withVaAndRedactedSystemPrivilege] = false
+		tests[2].usernameWantErr[withVaAndRedactedSystemPrivilege] = true
+
+	}
 	for _, tt := range tests {
 		for userName, wantErr := range tt.usernameWantErr {
 			t.Run(fmt.Sprintf("%s-%s", tt.name, userName), func(t *testing.T) {

pkg/server/server.go

@@ -45,6 +45,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/rpc"
 	"github.com/cockroachdb/cockroach/pkg/rpc/nodedialer"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/server/debug"
 	"github.com/cockroachdb/cockroach/pkg/server/diagnostics"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
@@ -67,8 +68,9 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/optionalnodeliveness"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire"
 	_ "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scjob" // register jobs declared outside of pkg/sql
-	_ "github.com/cockroachdb/cockroach/pkg/sql/ttl/ttljob"          // register jobs declared outside of pkg/sql
-	_ "github.com/cockroachdb/cockroach/pkg/sql/ttl/ttlschedule"     // register schedules declared outside of pkg/sql
+	"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
+	_ "github.com/cockroachdb/cockroach/pkg/sql/ttl/ttljob"      // register jobs declared outside of pkg/sql
+	_ "github.com/cockroachdb/cockroach/pkg/sql/ttl/ttlschedule" // register schedules declared outside of pkg/sql
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/ts"
 	"github.com/cockroachdb/cockroach/pkg/util"
@@ -714,7 +716,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 
 	lateBoundServer := &Server{}
 	// TODO(tbg): give adminServer only what it needs (and avoid circular deps).
-	adminAuthzCheck := &adminPrivilegeChecker{ie: internalExecutor}
+	adminAuthzCheck := &adminPrivilegeChecker{ie: internalExecutor, st: st}
 	sAdmin := newAdminServer(lateBoundServer, adminAuthzCheck, internalExecutor)
 
 	// These callbacks help us avoid a dependency on gossip in httpServer.
@@ -814,6 +816,20 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 		return nil, err
 	}
 
+	adminAuthzCheck.makePlanner = func(opName string) (interface{}, func()) {
+		// This is a hack to get around a Go package dependency cycle. See comment
+		// in sql/jobs/registry.go on planHookMaker.
+		txn := db.NewTxn(ctx, "check-system-privilege")
+		return sql.NewInternalPlanner(
+			opName,
+			txn,
+			username.RootUserName(),
+			&sql.MemoryMetrics{},
+			sqlServer.execCfg,
+			sessiondatapb.SessionData{},
+		)
+	}
+
 	sAuth := newAuthenticationServer(cfg.Config, sqlServer)
 	for i, gw := range []grpcGatewayServer{sAdmin, sStatus, sAuth, &sTS} {
 		if reflect.ValueOf(gw).IsNil() {

pkg/server/status.go

@@ -32,6 +32,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/build"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/gossip"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
@@ -59,6 +60,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/contentionpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/flowinfra"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirecancel"
+	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/util"
@@ -322,12 +324,18 @@ func (b *baseStatusServer) checkCancelPrivilege(
 		if sessionUser != reqUser {
 			// Must have CANCELQUERY privilege to cancel other users'
 			// sessions/queries.
-			ok, err := b.privilegeChecker.hasRoleOption(ctx, reqUser, roleoption.CANCELQUERY)
-			if err != nil {
-				return serverError(ctx, err)
+			hasCancelQuery := false
+			if b.privilegeChecker.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
+				hasCancelQuery = b.privilegeChecker.checkHasSystemPrivilege(ctx, reqUser, privilege.CANCELQUERY)
 			}
-			if !ok {
-				return errRequiresRoleOption(roleoption.CANCELQUERY)
+			if !hasCancelQuery {
+				ok, err := b.privilegeChecker.hasRoleOption(ctx, reqUser, roleoption.CANCELQUERY)
+				if err != nil {
+					return serverError(ctx, err)
+				}
+				if !ok {
+					return errRequiresRoleOption(roleoption.CANCELQUERY)
+				}
 			}
 			// Non-admins cannot cancel admins' sessions/queries.
 			isAdminSession, err := b.privilegeChecker.hasAdminRole(ctx, sessionUser)