Better access validation in `ON CLUSTER` queries. #71334

pufit · 2024-11-01T03:38:38Z

Changelog category (leave one):

Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

All DDL ON CLUSTER queries now execute with the original query user context for better access validation.

Details

This PR adds two new fields to the DDL entry:

initiator_user - a user's name from the original request.
initiator_roles - a user's roles from the original request.

If an initiator_user is not present on the cluster's instance, the request will fail. This behaviour can be controlled by a new server setting distributed_ddl_use_initial_user_and_roles

robot-clickhouse-ci-2 · 2024-11-01T03:40:49Z

This is an automated comment for commit f83a89a with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integration tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	❌ failure
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure

Successful checks

Check name	Description	Status
Builds	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Docs check	Builds and tests the documentation	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success

src/Core/Settings.cpp

pufit · 2024-11-01T18:56:27Z

initiator_user - a user's name from the original request.

access_hash - a hash of all access rights of the user.

If an initiator_user is not present on the cluster's instance or has different permissions the request will fail.
You can change this behavior with the server setting validate_access_consistency_between_instances

Also, @vitlibar, what do you think about this?

# Conflicts: # src/Core/ServerSettings.cpp # src/Interpreters/executeDDLQueryOnCluster.cpp

vitlibar · 2024-11-11T20:30:55Z

initiator_user - a user's name from the original request.

access_hash - a hash of all access rights of the user.

If an initiator_user is not present on the cluster's instance or has different permissions the request will fail.
You can change this behavior with the server setting validate_access_consistency_between_instances

Also, @vitlibar, what do you think about this?

For asynchronous insert queue we store and later use user id, the current roles, and the current settings (see). Maybe it's better to do the same for on cluster queries?

I'm not sure access_hash is useful. The access rights of any user or role can change dynamically, but even with ReplicatedAccessStorage there can be some delay between hosts reading those changes from ZooKeeper. It doesn't seem nice to make the whole ON CLUSTER command fail because something (perhaps not even related to the query) changed.

clickhouse-gh · 2024-12-17T13:16:23Z

Dear @tavplubix, @vitlibar, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

# Conflicts: # src/Core/ServerSettings.cpp # src/Interpreters/executeDDLQueryOnCluster.cpp

src/Interpreters/DDLTask.h

clickhouse-gh · 2025-02-07T09:25:32Z

Workflow [PR], commit [a329866]

tests/integration/test_replicated_access/test.py

clickhouse-gh · 2025-04-29T13:17:37Z

Dear @tuanpach, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

# Conflicts: # src/Interpreters/DDLTask.cpp

clickhouse-gh · 2025-07-09T03:18:45Z

Workflow [PR], commit [1916d8e]

Summary: ❌

job_name	test_name	status	info
Stateless tests (amd_binary, ParallelReplicas, s3 storage, parallel)		failure
	00440_nulls_merge_tree	FAIL	cidb
Stateless tests (amd_ubsan, sequential)		failure
	03141_fetches_errors_stress	FAIL	cidb, flaky
Integration tests (amd_asan, old analyzer, 3/6)		failure
	test_s3_access_headers/test.py::test_custom_access_header[test_access_over_custom_header]	FAIL	cidb, flaky
Integration tests (amd_binary, 5/5)		failure
	test_s3_access_headers/test.py::test_custom_access_header[test_access_over_custom_header]	FAIL	cidb, flaky
Integration tests (arm_binary, distributed plan, 4/4)		failure
	test_s3_access_headers/test.py::test_custom_access_header[test_access_over_custom_header]	FAIL	cidb, flaky
Integration tests (amd_tsan, 3/6)		failure
	test_s3_access_headers/test.py::test_custom_access_header[test_access_over_custom_header]	FAIL	cidb, flaky

tests/integration/test_replicated_access/test.py

vitlibar · 2025-07-25T16:30:45Z

src/Interpreters/executeDDLQueryOnCluster.cpp

    entry.setSettingsIfRequired(context);
    entry.tracing_context = OpenTelemetry::CurrentContext();
    entry.initial_query_id = context->getClientInfo().initial_query_id;
+    entry.initiator_user = *context->getUserID();


Can't context->getUserID() be nullopt?

src/Interpreters/DDLTask.cpp

vitlibar · 2025-07-25T16:49:47Z

src/Interpreters/DDLTask.cpp

+    if (!user)
+        LOG_INFO(getLogger("DDLTask"), "Initiator user is not present on the instance. Will use the global user for the query execution.");
+    else
+        query_context->setUser(entry.initiator_user, entry.initiator_user_roles);


What if the initiator user just hasn't been replicated to the current shard yet?
According to the code the query will be executed on behalf of the global user in that case.
It seems it would be better to fail instead.

I thought about this a lot. Since both replicated storage and DDL queue use ZooKeeper, which is a log-based consensus algorithm, it's technically impossible to have a state where a user is not yet written to the log, but it already writes DDL queries.
On the other hand, clusters with no access replications should work as they did before. Otherwise, the incompatibility will be devastating.

At least DDLWorker's queue and ReplicatedAccessStorage's queue work in different threads. They don't have to do things in sync. Also there can be a configuration which uses multiple ZooKeeper servers so these queues can connect even to different servers.

It seems a good enough solution to that is adding a setting which sets some waiting time for such cases. I mean if a shard fails to find the initiator by its UUID it could wait for a while before throwing an exception and thus failing the current query.

User creation in replicated storage is an operation with a commit to the Keeper log, so no issue on the second consern.
For the first part, yes, indeed, it can potentially be a problem (mostly on paper, the atacker has to fully control network to the target instance to use this potential vurnalability). An easy fix would be just to throw an error if we use ReplicatedAccessStorage only.

the atacker has to fully control network to the target instance to use this potential vurnalability

If a client executes hundreds or thousands of queries (which is quite normal for our Cloud) then this problem will appear quite often. And throwing an error immediately is secure but it is not nice to users so they will be complaining.

I've got another idea but it requires changing the interface of IAccessStorage. I think we could modify ReplicatedAccessStorage so it could check more thoroughly that a specified user doesn't really exist (no node in ZooKeeper) before throwing exception.

I mean that exception-throwing functions ReplicatedAccessStorage::getID() and ReplicatedAccessStorage::read(const UUID & id, bool throw_if_not_exists) in case if there is no entry in memory_storage and if throw_if_not_exists == true then these function could try to read the correspondent nodes from ZooKeeper immediately without waiting a queue, and then throw exception User not found only after that.

Upd:

What if the initiator user just hasn't been replicated to the current shard yet?

Let's just ignore this problem in this PR. It seems it shouldn't be a very big problem in most configurations. If it becomes kind of a big problem for some customer they will be able to just turn off the server setting enabling this PR's improvements (see my comment) and ignore initiator_user & initiator_roles specified in DDLTasks.

According to the code the query will be executed on behalf of the global user in that case. It seems it would be better to fail instead.

Let's just fail if initiator_user or initiator_roles don't exist. Using the global context seems very random, it's better not to do so. If a customer doesn't like this PR they will have the server setting to turn it off.

I've got another idea but it requires changing the interface of IAccessStorage. I think we could modify ReplicatedAccessStorage so it could check more thoroughly that a specified user doesn't really exist (no node in ZooKeeper) before throwing exception.

Let's not do that - any big changes of ReplicatedAccessStorage can cause new troubles, we don't want to go into that because of this PR.

vitlibar · 2025-07-25T17:48:55Z

src/Interpreters/DDLTask.h

    std::optional<UUID> parent_table_uuid;

+    UUID initiator_user;
+    std::vector<UUID> initiator_user_roles;


I suppose it's better to use names just because it's more general (and for ReplicatedAccessStorage it's almost the same). Also we need a way to enable/disable this feature, probably via some configuration setting, because not everyone will want it.

This is disabled by default right now with the distributed_ddl_entry_format_version

And what if someone wants to disable it after a while after we increase distributed_ddl_entry_format_version once again?

Then, they can set distributed_ddl_entry_format_version to the previous version. That is also why I don't want to throw any exceptions in the DDL execution to make these changes as backward compatible as possible, so there is no need to turn off this feature.

Upd:

And what if someone wants to disable it after a while after we increase distributed_ddl_entry_format_version once again?

Let's add a server setting which will allow to enable or disable both sending initiator_user / initiator_roles and using them on replicas/shards.

clickhouse-gh · 2025-08-26T13:20:13Z

Dear @vitlibar, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

# Conflicts: # src/Interpreters/DDLTask.cpp

Add a server setting.

vitlibar · 2025-11-11T12:31:37Z

CI failures are unrelated:

00440_nulls_merge_tree - see issue [CI] MergeTreeBackgroundExecutor: Lock acquisition took 1352 ms (in DROP TABLE) #89849
03141_fetches_errors_stress - test is flaky
test_s3_access_headers/test.py::test_custom_access_header[test_access_over_custom_header] - test was flaky

User validation in ON CLUSTER queries.

a91af70

robot-ch-test-poll3 added the pr-improvement Pull request with some product improvements label Nov 1, 2024

tavplubix reviewed Nov 1, 2024

View reviewed changes

src/Core/Settings.cpp Outdated Show resolved Hide resolved

tavplubix self-assigned this Nov 1, 2024

pufit added 2 commits November 8, 2024 02:51

Merge branch 'master' into pufit/fix-on-cluster-user

23f9f24

# Conflicts: # src/Core/ServerSettings.cpp # src/Interpreters/executeDDLQueryOnCluster.cpp

Revert distributed_ddl_entry_format_version change

7a91ab7

vitlibar self-assigned this Nov 11, 2024

clickhouse-gh bot unassigned tavplubix and vitlibar Dec 17, 2024

pufit and others added 3 commits January 18, 2025 16:36

Merge branch 'master' into pufit/fix-on-cluster-user

af04abb

# Conflicts: # src/Core/ServerSettings.cpp # src/Interpreters/executeDDLQueryOnCluster.cpp

Store UUIDs instead of names

a4cf904

Automatic style fix

48489b2

pufit requested a review from tavplubix January 19, 2025 00:27

nikitamikhaylov reviewed Jan 27, 2025

View reviewed changes

src/Interpreters/DDLTask.h Outdated Show resolved Hide resolved

pufit added 4 commits January 27, 2025 19:56

Update DDLTask.h

0533fe9

Change log level

f83a89a

Merge branch 'master' into pufit/fix-on-cluster-user

a448333

Fix test

1935ac7

pufit added 2 commits February 7, 2025 13:40

Fix test

7e821b0

Fix test

b15c7cf

pufit requested a review from vitlibar March 10, 2025 15:07

Fix test

a329866

tuanpach self-assigned this Mar 25, 2025

tuanpach reviewed Mar 25, 2025

View reviewed changes

tests/integration/test_replicated_access/test.py Show resolved Hide resolved

pufit added 2 commits July 8, 2025 23:12

Merge branch 'master' into pufit/fix-on-cluster-user

5a423d0

# Conflicts: # src/Interpreters/DDLTask.cpp

Fix test

d64f475

pufit requested review from nikitamikhaylov and tuanpach July 9, 2025 03:18

vitlibar self-assigned this Jul 25, 2025

vitlibar reviewed Jul 25, 2025

View reviewed changes

tests/integration/test_replicated_access/test.py Show resolved Hide resolved

vitlibar reviewed Jul 25, 2025

View reviewed changes

src/Interpreters/DDLTask.cpp Show resolved Hide resolved

vitlibar reviewed Jul 25, 2025

View reviewed changes

clickhouse-gh bot unassigned vitlibar Aug 26, 2025

vitlibar self-assigned this Nov 4, 2025

pufit added 4 commits November 7, 2025 14:14

Merge branch 'master' into pufit/fix-on-cluster-user

64bc834

# Conflicts: # src/Interpreters/DDLTask.cpp

Send user and role names instead of UUID.

9b0da05

Add a server setting.

Rename server setting

3a916b8

Update test config

bab4ec7

vitlibar approved these changes Nov 7, 2025

View reviewed changes

pufit added 4 commits November 8, 2025 17:47

Add test for setting

29e4b4f

Fix flaky test

3b7952a

Merge branch 'master' into pufit/fix-on-cluster-user

ad90db6

Fix flaky test

1916d8e

vitlibar enabled auto-merge November 11, 2025 12:34

vitlibar added this pull request to the merge queue Nov 11, 2025

Merged via the queue into master with commit 543ce91 Nov 11, 2025
124 of 131 checks passed

vitlibar deleted the pufit/fix-on-cluster-user branch November 11, 2025 14:49

robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Nov 11, 2025

Better access validation in ON CLUSTER queries. #71334

Better access validation in ON CLUSTER queries. #71334

Uh oh!

Conversation

pufit commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Details

Uh oh!

robot-clickhouse-ci-2 commented Nov 1, 2024 • edited by robot-clickhouse-ci-1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pufit commented Nov 1, 2024

Uh oh!

vitlibar commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clickhouse-gh bot commented Dec 17, 2024

Uh oh!

Uh oh!

clickhouse-gh bot commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

clickhouse-gh bot commented Apr 29, 2025

Uh oh!

clickhouse-gh bot commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitlibar Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitlibar Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitlibar Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitlibar Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitlibar Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh bot commented Aug 26, 2025

Uh oh!

vitlibar commented Nov 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Better access validation in `ON CLUSTER` queries. #71334

Better access validation in `ON CLUSTER` queries. #71334

pufit commented Nov 1, 2024 •

edited

Loading

robot-clickhouse-ci-2 commented Nov 1, 2024 •

edited by robot-clickhouse-ci-1

Loading

vitlibar commented Nov 11, 2024 •

edited

Loading

clickhouse-gh bot commented Feb 7, 2025 •

edited

Loading

clickhouse-gh bot commented Jul 9, 2025 •

edited

Loading

vitlibar Jul 25, 2025 •

edited

Loading

vitlibar Jul 25, 2025 •

edited

Loading

vitlibar Nov 5, 2025 •

edited

Loading

vitlibar Jul 25, 2025 •

edited

Loading

vitlibar Nov 5, 2025 •

edited

Loading