From f02cec220834dfea624d061396256fff06bae290 Mon Sep 17 00:00:00 2001 From: Santamaura Date: Wed, 8 Mar 2023 17:51:29 +0000 Subject: [PATCH 1/2] sql: regulate MODIFYCLUSTERSETTING with cluster setting This PR introduces the cluster setting sql.auth.modify_cluster_setting_applies_to_all.enabled which determines whether users with MODIFYCLUSTERSETTING are able to edit non sql.defaults settings. It also allows this setting to prevent viewing sql.default settings. Additionally, a deprecation warning is added for this cluster setting to inform that it should be replaced once the cluster is finalized Fixes: #122023 Release note (bug fix): reintroduce sql.auth.modify_cluster_setting_applies_to_all.enabled so that mixed version clusters can migrate off this setting. --- .../settings/settings-for-tenants.txt | 1 + docs/generated/settings/settings.html | 1 + pkg/settings/registry.go | 15 +++--- pkg/sql/crdb_internal.go | 16 +++++-- .../testdata/logic_test/cluster_settings | 46 ++++++++++++++++++- .../testdata/logic_test/crdb_internal | 30 +++++++++++- pkg/sql/set_cluster_setting.go | 40 +++++++++++++++- 7 files changed, 133 insertions(+), 16 deletions(-) diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt index 7eda34095155..4187cb80b5c9 100644 --- a/docs/generated/settings/settings-for-tenants.txt +++ b/docs/generated/settings/settings-for-tenants.txt @@ -96,6 +96,7 @@ server.web_session.purge.ttl duration 1h0m0s if nonzero, entries in system.web_s server.web_session_timeout duration 168h0m0s the duration that a newly created web session will be valid sql.auth.change_own_password.enabled boolean false controls whether a user is allowed to change their own password, even if they have no other privileges sql.auth.createrole_allows_grant_role_membership.enabled boolean false if set, users with CREATEROLE privilege can grant/revoke membership in roles +sql.auth.modify_cluster_setting_applies_to_all.enabled boolean true a bool which indicates whether MODIFYCLUSTERSETTING is able to set all cluster settings or only settings with the sql.defaults prefix (deprecated) sql.auth.resolve_membership_single_scan.enabled boolean true determines whether to populate the role membership cache with a single scan sql.closed_session_cache.capacity integer 1000 the maximum number of sessions in the cache sql.closed_session_cache.time_to_live integer 3600 the maximum time to live, in seconds diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index 48143fd1fe52..dfde2e05655f 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -129,6 +129,7 @@
server.web_session_timeout
duration168h0m0sthe duration that a newly created web session will be valid
sql.auth.change_own_password.enabled
booleanfalsecontrols whether a user is allowed to change their own password, even if they have no other privileges
sql.auth.createrole_allows_grant_role_membership.enabled
booleanfalseif set, users with CREATEROLE privilege can grant/revoke membership in roles +
sql.auth.modify_cluster_setting_applies_to_all.enabled
booleantruea bool which indicates whether MODIFYCLUSTERSETTING is able to set all cluster settings or only settings with the sql.defaults prefix (deprecated)
sql.auth.resolve_membership_single_scan.enabled
booleantruedetermines whether to populate the role membership cache with a single scan
sql.closed_session_cache.capacity
integer1000the maximum number of sessions in the cache
sql.closed_session_cache.time_to_live
integer3600the maximum time to live, in seconds diff --git a/pkg/settings/registry.go b/pkg/settings/registry.go index 9a638d9646f9..0d46e7a0e3a9 100644 --- a/pkg/settings/registry.go +++ b/pkg/settings/registry.go @@ -153,14 +153,13 @@ var retiredSettings = map[string]struct{}{ "server.web_session.auto_logout.timeout": {}, // removed as of 23.1. - "sql.auth.modify_cluster_setting_applies_to_all.enabled": {}, - "sql.catalog.descs.validate_on_write.enabled": {}, - "sql.distsql.max_running_flows": {}, - "sql.distsql.flow_scheduler_queueing.enabled": {}, - "sql.distsql.drain.cancel_after_wait.enabled": {}, - "changefeed.active_protected_timestamps.enabled": {}, - "jobs.scheduler.single_node_scheduler.enabled": {}, - "tenant_capabilities.authorizer.enabled": {}, + "sql.catalog.descs.validate_on_write.enabled": {}, + "sql.distsql.max_running_flows": {}, + "sql.distsql.flow_scheduler_queueing.enabled": {}, + "sql.distsql.drain.cancel_after_wait.enabled": {}, + "changefeed.active_protected_timestamps.enabled": {}, + "jobs.scheduler.single_node_scheduler.enabled": {}, + "tenant_capabilities.authorizer.enabled": {}, // renamed. "spanconfig.host_coalesce_adjacent.enabled": {}, "sql.defaults.experimental_stream_replication.enabled": {}, diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 0b16b55451a2..5a7d8f49e469 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -2068,18 +2068,26 @@ CREATE TABLE crdb_internal.cluster_settings ( origin STRING NOT NULL -- the origin of the value: 'default' , 'override' or 'external-override' )`, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - canViewAll, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYCLUSTERSETTING) + canViewSqlOnly := false + canViewAll, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.VIEWCLUSTERSETTING) if err != nil { return err } + if !canViewAll { - canViewAll, err = p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.VIEWCLUSTERSETTING) + canViewAll, err = p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYCLUSTERSETTING) if err != nil { return err } + modifyClusterSettingAll := modifyClusterSettingAppliesToAll.Get(&p.ExecCfg().Settings.SV) + if canViewAll && !modifyClusterSettingAll { + // Issue a deprecation notice, since this setting had an impact. + modifyClusterSettingAppliesToAllDeprecationNotice(ctx, p) + canViewAll = false + canViewSqlOnly = true + } } - canViewSqlOnly := false - if !canViewAll { + if !canViewAll && !canViewSqlOnly { canViewSqlOnly, err = p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYSQLCLUSTERSETTING) if err != nil { return err diff --git a/pkg/sql/logictest/testdata/logic_test/cluster_settings b/pkg/sql/logictest/testdata/logic_test/cluster_settings index 7a97384a4ff7..1c3d2dd8ef7e 100644 --- a/pkg/sql/logictest/testdata/logic_test/cluster_settings +++ b/pkg/sql/logictest/testdata/logic_test/cluster_settings @@ -116,8 +116,52 @@ WHERE variable IN ('sql.index_recommendation.drop_unused_duration') ---- sql.index_recommendation.drop_unused_duration 50s 168h0m0s external-override +# Users with MODIFYCLUSTERSETTING cannot view or set non sql.default settings if sql.full_modify_cluster_setting.enabled is false. + +user root + +statement ok +SET CLUSTER SETTING sql.auth.modify_cluster_setting_applies_to_all.enabled = false + +user testuser + +statement error pq: users with the MODIFYCLUSTERSETTING privilege need the cluster setting sql.auth.modify_cluster_setting_applies_to_all.enabled to be set to true to set cluster setting 'diagnostics.reporting.enabled' +SET CLUSTER SETTING diagnostics.reporting.enabled = false + +statement error only users with MODIFYCLUSTERSETTING or VIEWCLUSTERSETTING privileges are allowed to show cluster setting 'diagnostics.reporting.enabled' +SHOW CLUSTER SETTING diagnostics.reporting.enabled + +query I +SHOW CLUSTER SETTING sql.defaults.default_int_size +---- +4 + +statement ok +SET CLUSTER SETTING sql.defaults.default_int_size=8 + +query I +SHOW CLUSTER SETTING sql.defaults.default_int_size +---- +8 + +user root + +statement ok +GRANT SYSTEM VIEWCLUSTERSETTING TO testuser + +user testuser + +statement ok +SHOW CLUSTER SETTING diagnostics.reporting.enabled + user root +statement ok +REVOKE SYSTEM VIEWCLUSTERSETTING FROM testuser + +statement ok +SET CLUSTER SETTING sql.auth.modify_cluster_setting_applies_to_all.enabled = true + statement ok ALTER USER testuser NOMODIFYCLUSTERSETTING @@ -168,7 +212,7 @@ SHOW CLUSTER SETTING diagnostics.reporting.enabled ---- true -user root +user root skipif config local-mixed-22.2-23.1 statement ok diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal b/pkg/sql/logictest/testdata/logic_test/crdb_internal index 4adc6f177946..c114801e8257 100644 --- a/pkg/sql/logictest/testdata/logic_test/crdb_internal +++ b/pkg/sql/logictest/testdata/logic_test/crdb_internal @@ -1247,7 +1247,7 @@ SELECT crdb_internal.unsafe_clear_gossip_info('unknown key') ---- false -# Verify users with VIEWCLUSTERSETTING or MODIFYCLUSTERSETTING can view non sql.defaults cluster settings. +# Verify users with VIEWCLUSTERSETTING can view non sql.defaults cluster settings. # Verify users with MODIFYSQLCLUSTERSETTING can only view sql.defaults cluster settings. user root @@ -1290,6 +1290,31 @@ REVOKE SYSTEM MODIFYSQLCLUSTERSETTING FROM testuser statement ok GRANT SYSTEM MODIFYCLUSTERSETTING TO testuser +# Verify users with MODIFYCLUSTERSETTING can view non sql.defaults cluster setting only if sql.auth.modify_cluster_setting_applies_to_all.enabled is true. +user testuser + +query T +SELECT value FROM crdb_internal.cluster_settings WHERE variable IN ('diagnostics.reporting.enabled') +---- +true + +user root + +statement ok +SET CLUSTER SETTING sql.auth.modify_cluster_setting_applies_to_all.enabled = false + +user testuser + +query T noticetrace +SELECT value FROM crdb_internal.cluster_settings WHERE variable IN ('diagnostics.reporting.enabled') +---- +NOTICE: Cluster setting sql.auth.modify_cluster_setting_applies_to_all.enabled is deprecated, please use MODIFYSQLCLUSTERSETTING instead + +user root + +statement ok +GRANT SYSTEM VIEWCLUSTERSETTING TO testuser + user testuser query T @@ -1302,6 +1327,9 @@ user root statement ok REVOKE SYSTEM MODIFYCLUSTERSETTING FROM testuser +statement ok +REVOKE SYSTEM VIEWCLUSTERSETTING FROM testuser + query TT SELECT crdb_internal.humanize_bytes(NULL), crdb_internal.humanize_bytes(102400) ---- diff --git a/pkg/sql/set_cluster_setting.go b/pkg/sql/set_cluster_setting.go index af9b48c0f8c2..8690c96a56ae 100644 --- a/pkg/sql/set_cluster_setting.go +++ b/pkg/sql/set_cluster_setting.go @@ -65,19 +65,55 @@ type setClusterSettingNode struct { value tree.TypedExpr } +var modifyClusterSettingAppliesToAll = settings.RegisterBoolSetting( + settings.TenantWritable, + "sql.auth.modify_cluster_setting_applies_to_all.enabled", + "a bool which indicates whether MODIFYCLUSTERSETTING is able to set all cluster settings "+ + "or only settings with the sql.defaults prefix (deprecated)", + true, +).WithPublic() + +func modifyClusterSettingAppliesToAllDeprecationNotice(ctx context.Context, p *planner) { + // Once the latest version is active guide users to unset this setting. + if p.execCfg.Settings.Version.IsActive(ctx, clusterversion.V23_1) { + p.noticeSender.BufferNotice(pgnotice.Newf("Cluster setting %s is deprecated,"+ + " please use MODIFYSQLCLUSTERSETTING instead", modifyClusterSettingAppliesToAll.Key())) + } +} + func checkPrivilegesForSetting(ctx context.Context, p *planner, name string, action string) error { + isSqlSetting := strings.HasPrefix(name, "sql.defaults") // If the user has modify privileges, then they can set or show any setting. hasModify, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYCLUSTERSETTING) if err != nil { return err } if hasModify { - return nil + // If the user has modify privleges and modify cluster settings should only + // be limited to SQL defaults, then block this action if the setting is not + // a default. + modifyClusterSettingAll := modifyClusterSettingAppliesToAll.Get(&p.ExecCfg().Settings.SV) + isAdmin, err := p.HasAdminRole(ctx) + if err != nil { + return err + } + if !isAdmin && !modifyClusterSettingAll && !isSqlSetting { + // Issue a deprecation notice, since this setting had an impact. + modifyClusterSettingAppliesToAllDeprecationNotice(ctx, p) + // We do not have permission to modify anymore. + hasModify = false + if action == "set" { + return pgerror.Newf(pgcode.InsufficientPrivilege, + "users with the %s privilege need the cluster setting %s to be set to true to %s cluster setting '%s'", + privilege.MODIFYCLUSTERSETTING, modifyClusterSettingAppliesToAll.Key(), action, name) + } + } else { + return nil + } } // If the user only has sql modify privileges, then they can only set or show // any sql.defaults setting. - isSqlSetting := strings.HasPrefix(name, "sql.defaults") if isSqlSetting { hasSqlModify, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYSQLCLUSTERSETTING) if err != nil { From 5d91381c71b0a8e0dd8703f214b572efa5e74016 Mon Sep 17 00:00:00 2001 From: Justin Beaver Date: Thu, 11 Apr 2024 20:58:44 +0000 Subject: [PATCH 2/2] release-23.1: released CockroachDB version 23.1.18. Next version: 23.1.19 Release note: None Epic: None Release justification: non-production (release infra) change. --- pkg/build/version.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/build/version.txt b/pkg/build/version.txt index eaca667687d2..6511900d8c52 100644 --- a/pkg/build/version.txt +++ b/pkg/build/version.txt @@ -1 +1 @@ -v23.1.18 +v23.1.19