From a91af7030c64544d39b84479f107214ce6cd6039 Mon Sep 17 00:00:00 2001 From: pufit Date: Thu, 31 Oct 2024 23:32:39 -0400 Subject: [PATCH 1/2] User validation in `ON CLUSTER` queries. --- src/Core/ServerSettings.cpp | 1 + src/Core/Settings.cpp | 2 +- src/Interpreters/DDLTask.cpp | 41 +++++++++++++++++++ src/Interpreters/DDLTask.h | 6 ++- src/Interpreters/executeDDLQueryOnCluster.cpp | 2 + 5 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/Core/ServerSettings.cpp b/src/Core/ServerSettings.cpp index d697666d0e7..a48d90730a8 100644 --- a/src/Core/ServerSettings.cpp +++ b/src/Core/ServerSettings.cpp @@ -192,6 +192,7 @@ namespace DB DECLARE(UInt64, parts_killer_pool_size, 128, "Threads for cleanup of shared merge tree outdated threads. Only available in ClickHouse Cloud", 0) \ DECLARE(UInt64, keeper_multiread_batch_size, 10'000, "Maximum size of batch for MultiRead request to [Zoo]Keeper that support batching. If set to 0, batching is disabled. Available only in ClickHouse Cloud.", 0) \ DECLARE(Bool, use_legacy_mongodb_integration, true, "Use the legacy MongoDB integration implementation. Note: it's highly recommended to set this option to false, since legacy implementation will be removed in the future. Please submit any issues you encounter with the new implementation.", 0) \ + DECLARE(Bool, validate_access_consistency_between_instances, true, "Validate that the instance has the same user with exactly the same access before executing a DDL query. Note: turning this setting off may expose your cluster to potential permission escalation. Change this setting only if you know what you are doing.", 0) \ /// If you add a setting which can be updated at runtime, please update 'changeable_settings' map in dumpToSystemServerSettingsColumns below diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 3b63d1231af..a7b101b6e25 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -4410,7 +4410,7 @@ Possible values: Cloud default value: `none`. )", 0) \ - DECLARE(UInt64, distributed_ddl_entry_format_version, 5, R"( + DECLARE(UInt64, distributed_ddl_entry_format_version, 8, R"( Compatibility version of distributed DDL (ON CLUSTER) queries )", 0) \ \ diff --git a/src/Interpreters/DDLTask.cpp b/src/Interpreters/DDLTask.cpp index 106710a9bb4..3502ebefc36 100644 --- a/src/Interpreters/DDLTask.cpp +++ b/src/Interpreters/DDLTask.cpp @@ -1,7 +1,10 @@ +#include +#include #include #include #include #include +#include #include #include #include @@ -32,6 +35,11 @@ namespace Setting extern const SettingsUInt64 max_query_size; } +namespace ServerSetting +{ + extern const ServerSettingsBool validate_access_consistency_between_instances; +} + namespace ErrorCodes { extern const int UNKNOWN_FORMAT_VERSION; @@ -147,6 +155,14 @@ String DDLLogEntry::toString() const wb << "\n"; } + if (version >= INITIATOR_USER_VERSION) + { + wb << "initiator_user: "; + writeEscapedString(initial_query_id, wb); + wb << '\n'; + wb << "access_hash: " << access_hash << "\n"; + } + return wb.str(); } @@ -219,6 +235,14 @@ void DDLLogEntry::parse(const String & data) rb >> "\n"; } + if (version >= INITIATOR_USER_VERSION) + { + rb >> "initiator_user: "; + readEscapedString(initiator_user, rb); + rb >> "\n"; + rb >> "access_hash: " >> access_hash >> "\n"; + } + assertEOF(rb); if (!host_id_strings.empty()) @@ -253,8 +277,25 @@ ContextMutablePtr DDLTaskBase::makeQueryContext(ContextPtr from_context, const Z query_context->makeQueryContext(); query_context->setCurrentQueryId(""); // generate random query_id query_context->setQueryKind(ClientInfo::QueryKind::SECONDARY_QUERY); + + const auto & access_control = from_context->getAccessControl(); + const auto user = access_control.tryRead(entry.initiator_user); + if (!user) + { + if (from_context->getServerSettings()[ServerSetting::validate_access_consistency_between_instances]) + throw Exception(ErrorCodes::INCONSISTENT_CLUSTER_DEFINITION, "Initiator user is not present on instance."); + LOG_WARNING(getLogger("DDLTask"), "Initiator user is not present on the instance. Will use the global user for the query execution. This is a security vulnerability!"); + } + else + { + query_context->setUser(access_control.getID(entry.initiator_user)); + if (sipHash64(user->access.toString()) != entry.access_hash && from_context->getServerSettings()[ServerSetting::validate_access_consistency_between_instances]) + throw Exception(ErrorCodes::INCONSISTENT_CLUSTER_DEFINITION, "Inconsistent access for the '{}' user on the instance.", user->getName()); + } + if (entry.settings) query_context->applySettingsChanges(*entry.settings); + return query_context; } diff --git a/src/Interpreters/DDLTask.h b/src/Interpreters/DDLTask.h index f0f5d60db6d..547c8c1bbad 100644 --- a/src/Interpreters/DDLTask.h +++ b/src/Interpreters/DDLTask.h @@ -78,10 +78,11 @@ struct DDLLogEntry static constexpr const UInt64 PRESERVE_INITIAL_QUERY_ID_VERSION = 5; static constexpr const UInt64 BACKUP_RESTORE_FLAG_IN_ZK_VERSION = 6; static constexpr const UInt64 PARENT_TABLE_UUID_VERSION = 7; + static constexpr const UInt64 INITIATOR_USER_VERSION = 8; /// Add new version here /// Remember to update the value below once new version is added - static constexpr const UInt64 DDL_ENTRY_FORMAT_MAX_VERSION = 7; + static constexpr const UInt64 DDL_ENTRY_FORMAT_MAX_VERSION = 9; UInt64 version = 1; String query; @@ -95,6 +96,9 @@ struct DDLLogEntry /// Only for DatabaseReplicated. std::optional parent_table_uuid; + String initiator_user; + UInt64 access_hash; + void setSettingsIfRequired(ContextPtr context); String toString() const; void parse(const String & data); diff --git a/src/Interpreters/executeDDLQueryOnCluster.cpp b/src/Interpreters/executeDDLQueryOnCluster.cpp index c0440c755ad..6c48c9d58f4 100644 --- a/src/Interpreters/executeDDLQueryOnCluster.cpp +++ b/src/Interpreters/executeDDLQueryOnCluster.cpp @@ -189,6 +189,8 @@ BlockIO executeDDLQueryOnCluster(const ASTPtr & query_ptr_, ContextPtr context, entry.setSettingsIfRequired(context); entry.tracing_context = OpenTelemetry::CurrentContext(); entry.initial_query_id = context->getClientInfo().initial_query_id; + entry.initiator_user = context->getUserName(); + entry.access_hash = sipHash64(context->getAccess()->getAccessRights()->toString()); String node_path = ddl_worker.enqueueQuery(entry); return getDDLOnClusterStatus(node_path, ddl_worker.getReplicasDir(), entry, context); From 7a91ab7f95a4eb127c1f5b27efaef0431ea51bb8 Mon Sep 17 00:00:00 2001 From: pufit Date: Fri, 8 Nov 2024 02:52:42 -0500 Subject: [PATCH 2/2] Revert `distributed_ddl_entry_format_version` change --- src/Core/Settings.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 9ec3c25314a..6f0109fa300 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -4431,7 +4431,7 @@ Possible values: Cloud default value: `none`. )", 0) \ - DECLARE(UInt64, distributed_ddl_entry_format_version, 8, R"( + DECLARE(UInt64, distributed_ddl_entry_format_version, 5, R"( Compatibility version of distributed DDL (ON CLUSTER) queries )", 0) \ \