sql, server: regulate access to remaining observability features

This change will control access to various observability
features based on system privileges including the following:
- admin ui databases/tables/schema endpoints requires admin or VIEWACTIVITY
- EXPERIMENTAL_AUDIT requires admin or MODIFYCLUSTERSETTING
- sql login requires not having NOSQLLOGIN or the equivalent
role option

Resolves: #83848, #83863, #83862

Release note (security update): Change requirements to access some
observability features. Databases/tables/schema endpoints for
admin ui require admin or VIEWACTIVITY. EXPERIMENTAL_AUDIT
requires admin or MODIFYCLUSTERSETTING. SQL login requires not
having NOSQLLOGIN or the equivalent role option.

pkg/server/admin.go

@@ -248,6 +248,10 @@ func (s *adminServer) Databases(
 		return nil, serverError(ctx, err)
 	}
 
+	if err := s.requireViewActivityPermission(ctx); err != nil {
+		return nil, err
+	}
+
 	r, err := s.databasesHelper(ctx, req, sessionUser, 0, 0)
 	return r, maybeHandleNotFoundError(ctx, err)
 }
@@ -314,6 +318,10 @@ func (s *adminServer) DatabaseDetails(
 		return nil, serverError(ctx, err)
 	}
 
+	if err := s.requireViewActivityPermission(ctx); err != nil {
+		return nil, err
+	}
+
 	r, err := s.databaseDetailsHelper(ctx, req, userName)
 	return r, maybeHandleNotFoundError(ctx, err)
 }
@@ -678,6 +686,10 @@ func (s *adminServer) TableDetails(
 		return nil, serverError(ctx, err)
 	}
 
+	if err := s.requireViewActivityPermission(ctx); err != nil {
+		return nil, err
+	}
+
 	r, err := s.tableDetailsHelper(ctx, req, userName)
 	return r, maybeHandleNotFoundError(ctx, err)
 }
@@ -1073,7 +1085,13 @@ func (s *adminServer) TableStats(
 	ctx context.Context, req *serverpb.TableStatsRequest,
 ) (*serverpb.TableStatsResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
-	userName, err := s.requireAdminUser(ctx)
+
+	userName, err := userFromContext(ctx)
+	if err != nil {
+		return nil, serverError(ctx, err)
+	}
+
+	err = s.requireViewActivityPermission(ctx)
 	if err != nil {
 		// NB: not using serverError() here since the priv checker
 		// already returns a proper gRPC error status.
@@ -1103,7 +1121,7 @@ func (s *adminServer) NonTableStats(
 	ctx context.Context, req *serverpb.NonTableStatsRequest,
 ) (*serverpb.NonTableStatsResponse, error) {
 	ctx = s.server.AnnotateCtx(ctx)
-	if _, err := s.requireAdminUser(ctx); err != nil {
+	if err := s.requireViewActivityPermission(ctx); err != nil {
 		// NB: not using serverError() here since the priv checker
 		// already returns a proper gRPC error status.
 		return nil, err

pkg/server/admin_test.go

@@ -386,6 +386,16 @@ func TestAdminAPIDatabases(t *testing.T) {
 	if _, err := db.Exec(query); err != nil {
 		t.Fatal(err)
 	}
+	// Non admins now also require VIEWACTIVITY.
+	sysPrivileges := []string{"VIEWACTIVITY"}
+	query = fmt.Sprintf(
+		"GRANT SYSTEM %s TO %s",
+		sysPrivileges,
+		authenticatedUserNameNoAdmin().SQLIdentifier(),
+	)
+	if _, err := db.Exec(query); err != nil {
+		t.Fatal(err)
+	}
 
 	for _, tc := range []struct {
 		expectedDBs []string

pkg/server/index_usage_stats.go

@@ -194,7 +194,7 @@ func (s *statusServer) TableIndexStats(
 	ctx = propagateGatewayMetadata(ctx)
 	ctx = s.AnnotateCtx(ctx)
 
-	if err := s.privilegeChecker.requireViewActivityOrViewActivityRedactedPermission(ctx); err != nil {
+	if err := s.privilegeChecker.requireViewActivityPermission(ctx); err != nil {
 		return nil, err
 	}
 	return getTableIndexUsageStats(ctx, req, s.sqlServer.pgServer.SQLServer.GetLocalIndexStatistics(),

pkg/sql/alter_table.go

@@ -18,6 +18,7 @@ import (
 	"sort"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
@@ -34,6 +35,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
+	"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
@@ -43,6 +45,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/stats"
 	"github.com/cockroachdb/cockroach/pkg/sql/storageparam"
 	"github.com/cockroachdb/cockroach/pkg/sql/storageparam/tablestorageparam"
+	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
@@ -907,10 +910,28 @@ func (p *planner) setAuditMode(
 	p.curPlan.auditEvents = append(p.curPlan.auditEvents,
 		auditEvent{desc: desc, writing: true})
 
-	// We require root for now. Later maybe use a different permission?
-	if err := p.RequireAdminRole(ctx, "change auditing settings on a table"); err != nil {
+	// Requires admin or MODIFYCLUSTERSETTING as of 22.2
+	hasAdmin, err := p.HasAdminRole(ctx)
+	if err != nil {
 		return false, err
 	}
+	if !hasAdmin {
+		// Check for system privilege first, otherwise fall back to role options.
+		hasModify := false
+		if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
+			hasModify = p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING) == nil
+		}
+		if !hasModify {
+			hasModify, err = p.HasRoleOption(ctx, roleoption.MODIFYCLUSTERSETTING)
+			if err != nil {
+				return false, err
+			}
+			if !hasModify {
+				return false, pgerror.Newf(pgcode.InsufficientPrivilege,
+					"only users with admin or %s system privilege are allowed to change audit settings on a table ", privilege.MODIFYCLUSTERSETTING.String())
+			}
+		}
+	}
 
 	telemetry.Inc(sqltelemetry.SchemaSetAuditModeCounter(auditMode.TelemetryName()))
 

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -850,7 +850,7 @@ statement ok
 ALTER TABLE audit ADD COLUMN y INT
 
 # But not the audit settings.
-statement error change auditing settings on a table
+statement error pq: only users with admin or MODIFYCLUSTERSETTING system privilege are allowed to change audit settings on a table
 ALTER TABLE audit EXPERIMENTAL_AUDIT SET OFF
 
 user root

pkg/sql/sessioninit/BUILD.bazel

@@ -42,9 +42,11 @@ go_test(
         "//pkg/security/securitytest",
         "//pkg/security/username",
         "//pkg/server",
+        "//pkg/settings/cluster",
         "//pkg/sql",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
+        "//pkg/sql/sessiondatapb",
         "//pkg/sql/sqlutil",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/sqlutils",

pkg/sql/sessioninit/cache.go

@@ -114,10 +114,13 @@ func (a *Cache) GetAuthInfo(
 		ctx context.Context,
 		ie sqlutil.InternalExecutor,
 		username username.SQLUsername,
+		makePlanner func(opName string) (interface{}, func()),
+		settings *cluster.Settings,
 	) (AuthInfo, error),
+	makePlanner func(opName string) (interface{}, func()),
 ) (aInfo AuthInfo, err error) {
 	if !CacheEnabled.Get(&settings.SV) {
-		return readFromSystemTables(ctx, ie, username)
+		return readFromSystemTables(ctx, ie, username, makePlanner, settings)
 	}
 
 	var usersTableDesc catalog.TableDescriptor
@@ -164,7 +167,7 @@ func (a *Cache) GetAuthInfo(
 	val, err := a.loadValueOutsideOfCache(
 		ctx, fmt.Sprintf("authinfo-%s-%d-%d", username.Normalized(), usersTableVersion, roleOptionsTableVersion),
 		func(loadCtx context.Context) (interface{}, error) {
-			return readFromSystemTables(loadCtx, ie, username)
+			return readFromSystemTables(loadCtx, ie, username, makePlanner, settings)
 		})
 	if err != nil {
 		return aInfo, err

pkg/sql/sessioninit/cache_test.go

@@ -19,9 +19,11 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
+	"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -74,18 +76,30 @@ func TestCacheInvalidation(t *testing.T) {
 		return settings, didReadFromSystemTable, err
 	}
 	getAuthInfoFromCache := func() (sessioninit.AuthInfo, bool, error) {
+		makePlanner := func(opName string) (interface{}, func()) {
+			return sql.NewInternalPlanner(
+				opName,
+				execCfg.DB.NewTxn(ctx, opName),
+				username.RootUserName(),
+				&sql.MemoryMetrics{},
+				s.ExecutorConfig().(*sql.ExecutorConfig),
+				sessiondatapb.SessionData{},
+			)
+		}
 		didReadFromSystemTable := false
+		settings := s.ClusterSettings()
 		aInfo, err := execCfg.SessionInitCache.GetAuthInfo(
 			ctx,
-			s.ClusterSettings(),
+			settings,
 			s.InternalExecutor().(sqlutil.InternalExecutor),
 			s.DB(),
 			s.CollectionFactory().(*descs.CollectionFactory),
 			username.TestUserName(),
-			func(ctx context.Context, ie sqlutil.InternalExecutor, userName username.SQLUsername) (sessioninit.AuthInfo, error) {
+			func(ctx context.Context, ie sqlutil.InternalExecutor, userName username.SQLUsername, makePlanner func(opName string) (interface{}, func()), settings *cluster.Settings) (sessioninit.AuthInfo, error) {
 				didReadFromSystemTable = true
 				return sessioninit.AuthInfo{}, nil
-			})
+			},
+			makePlanner)
 		return aInfo, didReadFromSystemTable, err
 	}
 
@@ -202,6 +216,7 @@ func TestCacheSingleFlight(t *testing.T) {
 	ctx := context.Background()
 	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(ctx)
+	execCfg := s.ExecutorConfig().(sql.ExecutorConfig)
 	settings := s.ExecutorConfig().(sql.ExecutorConfig).Settings
 	ie := s.InternalExecutor().(sqlutil.InternalExecutor)
 	c := s.ExecutorConfig().(sql.ExecutorConfig).SessionInitCache
@@ -219,18 +234,32 @@ func TestCacheSingleFlight(t *testing.T) {
 	wgFirstGetAuthInfoCallInProgress.Add(1)
 	wgForTestComplete.Add(3)
 
+	makePlanner := func(opName string) (interface{}, func()) {
+		return sql.NewInternalPlanner(
+			opName,
+			execCfg.DB.NewTxn(ctx, opName),
+			username.RootUserName(),
+			&sql.MemoryMetrics{},
+			s.ExecutorConfig().(*sql.ExecutorConfig),
+			sessiondatapb.SessionData{},
+		)
+	}
+
 	go func() {
 		didReadFromSystemTable := false
 		_, err := c.GetAuthInfo(ctx, settings, ie, s.DB(), s.ExecutorConfig().(sql.ExecutorConfig).CollectionFactory, testuser, func(
 			ctx context.Context,
 			ie sqlutil.InternalExecutor,
 			userName username.SQLUsername,
+			makePlanner func(opName string) (interface{}, func()),
+			settings *cluster.Settings,
 		) (sessioninit.AuthInfo, error) {
 			wgFirstGetAuthInfoCallInProgress.Done()
 			wgForConcurrentReadWrite.Wait()
 			didReadFromSystemTable = true
 			return sessioninit.AuthInfo{}, nil
-		})
+		},
+			makePlanner)
 		require.NoError(t, err)
 		require.True(t, didReadFromSystemTable)
 		wgForTestComplete.Done()
@@ -249,10 +278,13 @@ func TestCacheSingleFlight(t *testing.T) {
 				ctx context.Context,
 				ie sqlutil.InternalExecutor,
 				userName username.SQLUsername,
+				makePlanner func(opName string) (interface{}, func()),
+				settings *cluster.Settings,
 			) (sessioninit.AuthInfo, error) {
 				didReadFromSystemTable = true
 				return sessioninit.AuthInfo{}, nil
-			})
+			},
+				makePlanner)
 			require.NoError(t, err)
 			require.False(t, didReadFromSystemTable)
 			wgForTestComplete.Done()
@@ -270,10 +302,13 @@ func TestCacheSingleFlight(t *testing.T) {
 		ctx context.Context,
 		ie sqlutil.InternalExecutor,
 		userName username.SQLUsername,
+		makePlanner func(opName string) (interface{}, func()),
+		settings *cluster.Settings,
 	) (sessioninit.AuthInfo, error) {
 		didReadFromSystemTable = true
 		return sessioninit.AuthInfo{}, nil
-	})
+	},
+		makePlanner)
 
 	require.NoError(t, err)
 	require.True(t, didReadFromSystemTable)

pkg/sql/user.go

@@ -14,20 +14,25 @@ import (
 	"context"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/security/password"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/settings"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"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/sessiondata"
+	"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
+	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
 	"github.com/cockroachdb/cockroach/pkg/util/contextutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
@@ -207,6 +212,16 @@ func retrieveSessionInitInfoWithCache(
 	databaseName string,
 ) (aInfo sessioninit.AuthInfo, settingsEntries []sessioninit.SettingsCacheEntry, err error) {
 	if err = func() (retErr error) {
+		makePlanner := func(opName string) (interface{}, func()) {
+			return NewInternalPlanner(
+				opName,
+				execCfg.DB.NewTxn(ctx, opName),
+				username.RootUserName(),
+				&MemoryMetrics{},
+				execCfg,
+				sessiondatapb.SessionData{},
+			)
+		}
 		aInfo, retErr = execCfg.SessionInitCache.GetAuthInfo(
 			ctx,
 			execCfg.Settings,
@@ -215,6 +230,7 @@ func retrieveSessionInitInfoWithCache(
 			execCfg.CollectionFactory,
 			userName,
 			retrieveAuthInfo,
+			makePlanner,
 		)
 		if retErr != nil {
 			return retErr
@@ -243,7 +259,11 @@ func retrieveSessionInitInfoWithCache(
 }
 
 func retrieveAuthInfo(
-	ctx context.Context, ie sqlutil.InternalExecutor, user username.SQLUsername,
+	ctx context.Context,
+	ie sqlutil.InternalExecutor,
+	user username.SQLUsername,
+	makePlanner func(opName string) (interface{}, func()),
+	settings *cluster.Settings,
 ) (aInfo sessioninit.AuthInfo, retErr error) {
 	// Use fully qualified table name to avoid looking up "".system.users.
 	// We use a nil txn as login is not tied to any transaction state, and
@@ -297,6 +317,23 @@ func retrieveAuthInfo(
 	aInfo.CanLoginSQL = true
 	aInfo.CanLoginDBConsole = true
 	var ok bool
+
+	// Check system privilege to see if user can sql login.
+	planner, cleanup := makePlanner("check-privilege")
+	defer cleanup()
+	aa := planner.(AuthorizationAccessor)
+	hasAdmin, err := aa.HasAdminRole(ctx)
+	if err != nil {
+		return aInfo, err
+	}
+	if !hasAdmin {
+		if settings.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
+			if noSQLLogin := aa.CheckPrivilegeForUser(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.NOSQLLOGIN, user) == nil; noSQLLogin {
+				aInfo.CanLoginSQL = false
+			}
+		}
+	}
+
 	for ok, err = roleOptsIt.Next(ctx); ok; ok, err = roleOptsIt.Next(ctx) {
 		row := roleOptsIt.Cur()
 		option := string(tree.MustBeDString(row[0]))
@@ -305,7 +342,9 @@ func retrieveAuthInfo(
 			aInfo.CanLoginSQL = false
 			aInfo.CanLoginDBConsole = false
 		}
-		if option == "NOSQLLOGIN" {
+		// If the user did not have the NOSQLLOGIN system privilege but has the
+		// equivalent role option set the flag to false.
+		if option == "NOSQLLOGIN" && aInfo.CanLoginSQL {
 			aInfo.CanLoginSQL = false
 		}