Add feature to impersonate/proxy user sessions#70775
Add feature to impersonate/proxy user sessions#70775vitlibar merged 12 commits intoClickHouse:masterfrom
Conversation
|
Somewhat theoretical, but it could affect/change result of queries.
Ok, i see that it's covered already. |
|
One issue with executing multiple statements in sequence (e.g., Consider the following cases:
In light of these issues, we need a way to switch users "in one shot," either during connection establishment or within a single SQL statement, without depending on session persistence. Protocol extensions might increase complexity for driver authors, which could become a barrier to adoption. I would suggest considering three potential solutions:
|
|
Thanks @filimonov for the inputs regarding "one shot" switch user. My preference is also option 1 - special format user name like |
|
@vitlibar Let us know your feedback after initial review! |
|
@vitlibar Let us know if you got a chance to review the approach and it aligns well, I can then mark the PR ready for review. Thanks! |
| class ASTRolesOrUsersSet; | ||
|
|
||
| /** EXECUTE AS <user> | ||
| */ |
There was a problem hiding this comment.
It would be nice to have a command to execute only one statement as a specified user, then get back to normal mode. Perhaps something like
EXECUTE AS <user> SELECT ...
There was a problem hiding this comment.
Random passer-by here. This EXECUTE AS <user> SELECT ... syntax is exactly the thing I'm looking for, to implement proxying of multiple users with a single connection, or from the HTTP interface. I'm overall very excited for this PR!
| class ASTRolesOrUsersSet; | ||
|
|
||
| /** EXECUTE AS <user> | ||
| */ |
There was a problem hiding this comment.
It may be also useful to enable switching this feature off:
EXECUTE AS <user> ON
...
EXECUTE AS <user> OFF
Simple Execute AS <user> could be made an alias of EXECUTE AS <user> ON.
src/Interpreters/Context.cpp
Outdated
| auto new_user_uuid = *(users.ids.begin()); | ||
| setUser(new_user_uuid); | ||
| setCurrentUserName(getUser()->getName()); | ||
| setInitialUserName(getUser()->getName()); |
There was a problem hiding this comment.
Why do we need to set the initial user name here? A comment is needed.
src/Interpreters/Context.cpp
Outdated
| need_recalculate_access = true; | ||
| } | ||
|
|
||
| void Context::setAuthUserName(const String & auth_user_name) |
There was a problem hiding this comment.
Do we call this function somewhere? Let's remove it if not.
src/Access/Common/AccessType.h
Outdated
| M(SHOW_QUOTAS, "SHOW CREATE QUOTA", GLOBAL, SHOW_ACCESS) \ | ||
| M(SHOW_SETTINGS_PROFILES, "SHOW PROFILES, SHOW CREATE SETTINGS PROFILE, SHOW CREATE PROFILE", GLOBAL, SHOW_ACCESS) \ | ||
| M(SHOW_ACCESS, "", GROUP, ACCESS_MANAGEMENT) \ | ||
| M(IMPERSONATE, "", USER_NAME, ACCESS_MANAGEMENT) \ |
There was a problem hiding this comment.
Alias "EXECUTE AS" seems logical here:
M(IMPERSONATE, "EXECUTE AS", USER_NAME, ACCESS_MANAGEMENT) \
Or perhaps vice versa - perhaps it's better to name the privilege EXECUTE AS and make IMPERSONATE an alias, WDYT?
There was a problem hiding this comment.
Separating the privilege from the SQL allows us to use the privilege in some other feature in the future. e.g external authentication of the end-user and then IMPERSONATE using some connect time token etc. I have added "EXECUTE AS" an an alias. Thanks!
src/Interpreters/Context.cpp
Outdated
| setCurrentRolesImpl(user->granted_roles.findGranted(user->default_roles), /* throw_if_not_granted= */ false, /* skip_if_not_granted= */ false, user); | ||
| } | ||
|
|
||
| void Context::switchImpersonateUser(const RolesOrUsersSet & users ) |
There was a problem hiding this comment.
It seems this function is used only once and it's very simple. Let's move it to InterpreterExecuteAs.cpp perhaps?
| struct RolesOrUsersSet; | ||
| struct User; | ||
|
|
||
| class InterpreterExecuteAsQuery : public IInterpreter, WithMutableContext |
There was a problem hiding this comment.
InterpreterExecuteAs looks a bit better (because it's rather execute as <user>, not execute as <query>, and InterpreterExecuteAsUserQuery is just a bit too long).
src/Core/ServerSettings.h
Outdated
| M(Bool, disable_insertion_and_mutation, false, "Disable all insert/alter/delete queries. This setting will be enabled if someone needs read-only nodes to prevent insertion and mutation affect reading performance.", 0) \ | ||
| M(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) \ | ||
| M(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) \ | ||
| M(Bool, allow_impersonate_user, true, "Enable/disable the IMPERSONATE feature (EXECUTE AS <user>).", 0) \ |
There was a problem hiding this comment.
We usually put our experimental feature flags in Core/Settings.cpp, it seems it's better to put this flag there too.
| <path>./</path> | ||
| </local_directory> | ||
| </user_directories> | ||
| </clickhouse> |
There was a problem hiding this comment.
This config file won't be needed if you move allow_impersonate_user to Core/Settings.cpp. And the test can be made much shorter and simpler in this case too.
src/Core/ServerSettings.h
Outdated
| M(Bool, disable_insertion_and_mutation, false, "Disable all insert/alter/delete queries. This setting will be enabled if someone needs read-only nodes to prevent insertion and mutation affect reading performance.", 0) \ | ||
| M(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) \ | ||
| M(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) \ | ||
| M(Bool, allow_impersonate_user, true, "Enable/disable the IMPERSONATE feature (EXECUTE AS <user>).", 0) \ |
There was a problem hiding this comment.
Let's rename it to allow_execute_as (because execute_as is just easier to type :) )
|
@shiyer7474 Sorry for the waiting. |
|
@vitlibar Thanks for the detailed review! Please give me 2 days to incorporate and test the changes. |
|
This is an automated comment for commit 947d077 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
|
I will do the documentation changes once we finalize on the syntax and behaviour specifics! |
|
Checking the test failures. |
|
I will add the docs, code review can continue. |
|
Hi @vitlibar could you please take a look? |
|
|
||
| query->targetuser = targetuser; | ||
|
|
||
| /// support 1) EXECUTE AS <user1> 2) EXECUTE AS <user1> SELECT ... |
There was a problem hiding this comment.
It makes sense to support any query after EXECUTE AS <user>, not just SELECT.
For example,
EXECUTE AS <user1> CREATE TABLE ...
| "--listen_host=127.1" | ||
| # we will discover the real port later. | ||
| "--tcp_port=0" | ||
| "--shutdown_wait_unfinished=0" |
There was a problem hiding this comment.
It's unnecessary to start another server in this test.
access_management is already set to 1 in our CI for all our tests.
And without starting another server this test should be much simpler.
There was a problem hiding this comment.
I rewrote the test and simplified it.
|
|
||
| BlockIO return_value = {}; | ||
|
|
||
| auto restore_previous_user = [&] () { getContext()->getSessionContext()->switchImpersonateUser(RolesOrUsersSet(current_user_uuid.value())); }; |
There was a problem hiding this comment.
Here restore_previous_user() won't work good enough - because switchImpersonateUser() also resets settings and roles. What is better is to clone the query context and you don't need to worry about restoring it afterwards:
if (query.subquery)
{
auto impersonation_context = Context::createCopy(*getContext());
impersonation_context->switchImpersonateU
impersonation_context->makeQueryContext();
auto subquery_io = executeQuery(queryToString(query.subquery), impersonation_context, QueryFlags{ .internal = true }).second;
return subquery_io;
}
else
{
getContext()->getSessionContext()->switchImpersonateUser(...);
return {};
}
There was a problem hiding this comment.
I rewrote this code in a more proper way.
|
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. |
b4803ed to
97485df
Compare
5f6e62a to
afb6b6e
Compare
|
CI failures are unrelated: |
ceecd4d
Add feature to impersonate/proxy user sessions
25.3.8 Backport of ClickHouse#70775 - Add feature to impersonate/proxy user sessions
|
@vitlibar @shiyer7474
|
|
it seems that query Is it expected? I assume it should give the same exception as it does for @fm4v fyi |
I think that's OK because |
Add feature to impersonate/proxy user sessions
Add feature to impersonate/proxy user sessions
25.8.12 Backport of ClickHouse#70775: Add feature to impersonate/proxy user sessions #
25.8.12 Backport of ClickHouse#70775: Add feature to impersonate/proxy user sessions #
25.8.13 Backport of ClickHouse#70775: Add feature to impersonate/proxy user sessions
25.8.13 Backport of ClickHouse#70775: Add feature to impersonate/proxy user sessions
25.8.13 Backport of ClickHouse#70775: Add feature to impersonate/proxy user sessions
25.8.15 Backport of ClickHouse#70775: Add feature to impersonate/proxy user sessions
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Added new SQL statement EXECUTE AS to support user impersonation. Resolves #39048
Documentation entry for user-facing changes
WIP
Details
Add the ability to impersonate or proxy an end-user session over an application admin user connection.
Resolves #39048