New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added max_sessions_for_user setting #51724
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This is an automated comment for commit 7e9e0ac with description of existing statuses. It's updated for the latest CI running
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
My understanding is that the existing server parameter
The subject of protection is the server itself. This PR aims to make this protection more fine-granular which will be handy for "untrusted" public credentials. In more detail, it can prevent the case that a single untrusted user is able to flood the server with hundreds of connections at the detriment of other (e.g. trusted) users that behave normally. Th is is different from |
src/Access/ContextAccess.cpp
Outdated
enabled_quota = access_control->getEnabledQuota( | ||
*params.user_id, user_name, roles_info->enabled_roles, params.address, params.forwarded_address, params.quota_key); | ||
|
||
/// TCP connection handler receives quota key after session creation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand the changes around the quota_key
. How is it related to the session tracking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if my solution is the best, but I haven't found a better and simpler solution to not change the current TCP protocol and make both client and server work without errors.
If you have better ideas, I will be happy to implement them.
During a session between client and server, the client assumes that there should be no errors on the server side after the server has replied Hello.
However, with the addition of the max_sessions_per_user
constraint, throwing an error after server hello
is now possible. To get max_sessions_per_user
we need to reach the profile settings
, and to do that we need to create a session context
.
Creating a session context and creating a quota without a quota key can throw exception.
To better understand the problem, I have created attached images.
I hope they will make it easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation and the figures, I understood the problem now 😄
I am not an expert with the TCP protocol, but the current solution looks fine to me (not great but also not terrible) - we can go with it. My main question at this point is about this assumption:
During a session between client and server, the client assumes that there should be no errors on the server side after the server has replied Hello.
That's interesting. What happens exactly on the client? Does the connection abruptly end?
I see that there is a handler for exceptions: sendException()
. Just curious, would it be possible to use this to send an exception after "Hello" to the client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting. What happens exactly on the client? Does the connection abruptly end?
Server closes connection.
If exception happens after hello received from server client tries to send quota key if it has it. Usually server doesn't have time to close the connection while the client is sending quota key, this step passes (Client and server are on the same computer).
If the client was in interactive mode then it waits for query to enter.
After client has an query then it tries to send it to the server. Client detects that connection is closed by Connection::ping and doesn't print any error (client does not read exception code at all), and finally client tries to reconnect and reauthenticate.
I see that there is a handler for exceptions: sendException(). Just curious, would it be possible to use this to send an exception > after "Hello" to the client?
Yes, but client doesn't read the socket data after receiving hello from server, because it does not expect an error.
Thank you for the thoughtful review!
You're getting the right idea. There are 4 analysts in the company, they use one clickhouse account, there should not be more than 4 simultaneous connections, in case of the 5th connection, it should be immediately disabled. Session Tracker currently records not only the number of connections, but also information about connections, perhaps in the future it will be possible to create a new system table |
I generally like this idea 👍 ... but before going ahead and implementing it, I recommend to first discuss the system table fields in an RFC issue on GitHub. |
src/Access/ContextAccess.cpp
Outdated
enabled_quota = access_control->getEnabledQuota( | ||
*params.user_id, user_name, roles_info->enabled_roles, params.address, params.forwarded_address, params.quota_key); | ||
|
||
/// TCP connection handler receives quota key after session creation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation and the figures, I understood the problem now 😄
I am not an expert with the TCP protocol, but the current solution looks fine to me (not great but also not terrible) - we can go with it. My main question at this point is about this assumption:
During a session between client and server, the client assumes that there should be no errors on the server side after the server has replied Hello.
That's interesting. What happens exactly on the client? Does the connection abruptly end?
I see that there is a handler for exceptions: sendException()
. Just curious, would it be possible to use this to send an exception after "Hello" to the client?
Server closes connection.
Yes, but client doesn't read the socket data after receiving hello from server, because it does not expect an error. |
Of course we will create RFC issue if there is a need to create such a table, but at the moment there is no immediate need. |
src/Interpreters/Session.cpp
Outdated
session_tracker_handle = session_context->getSessionTracker().trackSession( | ||
*user_id, | ||
{ session_name_ }, | ||
session_context->getSettingsRef().max_sessions_for_user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can a user update the value of max_sessions_for_user
in the session_context
by executing a SET
query, for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but Behaviour will not be oblivious for user,
SET
sets changes in current session, but it will not have effect, because max_sessions_for_user
value is taken from user profile, not session:
// in profile we have max_sessions_for_user = 0
Client_1 (user Alice):
SET max_sessions_for_user = 1 // changes value for client_1 only
SELECT value FROM system.settings WHERE name="max_sessions_for_user" // return 1
Client 2 (user Alice):
SELECT value FROM system.settings WHERE name="max_sessions_for_user" // return 0
Client 3 (user Alice): // connections count per user (3) already
SET max_sessions_for_user = 1 // active session willl continue.
So it looks like a bug to me. This bug can be applied to other settings from profile without contraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This SQL looks like working:
ALTER SETTINGS PROFILE <dynamic_non_readonly_profile> SETTINGS max_sessions_for_user = 1
It will not close active sessions, but at least it will not allow to start new sessions.
I would certainly throw exception when trying to set up any settings taken from the profile for a session, but during quick looking into the code of how SET
work and got no easy solution. I would create an separate issue for that and investigate the problem more thoughtful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because max_sessions_for_user value is taken from user profile, not session
No, it's taken from session_context
:
https://github.com/ClickHouse/ClickHouse/blob/d3e08a1e2a91cd4bef08b8270e5e68d05332a770/src/Interpreters/Session.cpp#L470
You probably mean that it's a new session_context
that was just created and the user could not modify it. But it's not true for named http sessions because NamedSessionsStorage
may return an existing session that was already in use, so the setting could be modified:
https://github.com/ClickHouse/ClickHouse/blob/d3e08a1e2a91cd4bef08b8270e5e68d05332a770/src/Interpreters/Session.cpp#L443-L444
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of a named session, if session_context
was already created, It is possible that max_sessions_for_user
will have an old value:
https://github.com/ClickHouse/ClickHouse/blob/d3e08a1e2a91cd4bef08b8270e5e68d05332a770/src/Interpreters/Session.cpp#L458-L459
So, yes, it looks like a problem in this case:
ALTER SETTINGS PROFILE ALICE_PROFILE max_sessions_for_user = 100
User Alice started session_id_1 // session lifetime is 1000, max_session_per_user = 100
User Alice started session_id_2 // session lifetime is 1000, max_session_per_user = 100
User Alice started session_id_3 // session lifetime is 1000, max_session_per_user = 100
ALTER SETTINGS PROFILE ALICE_PROFILE max_sessions_for_user = 1
User Alice finished session_id_3 // named session is not deleted and context remains valid with max_session_per_user = 100
User Alice started session_id_3 // existing context was reused with max_session_per_user = 100
Calling new_session_context->setUser(*user_id)
always before reading max_sessions_for_user will fix the issue.
setUser
is quite a complicated operation, do you know a better way to get the max_sessions_for_user
value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling new_session_context->setUser(*user_id) always before reading max_sessions_for_user will fix the issue.
And will break SET
queries (because it will overwrite all the settings with the user's defaults, not only max_sessions_for_user
)
setUser is quite a complicated operation, do you know a better way to get the max_sessions_for_user value?
I don't know. We can take something from the implementation of setUser
, maybe smth like access->getDefaultProfileInfo()->settings.max_sessions_for_user
will work. Or we can save the value in NamedSessionData
when creating a session. Or we can forbid changing the setting (see Context::setSettings
and Context::recalculateAccessRightsIfNeeded
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
Just pushed an update that should forbid using SET max_sessions_for_user = 10
and several other parameters that shouldn't be updated by SET query
or SETTINGS
like
clickhouse-client -q "select 1 SETTINGS max_sessions_for_user = 1"
// error
Received exception from server (version 23.7.1):
Code: 164. DB::Exception: Received from localhost:9000. DB::Exception: Setting max_sessions_for_user is not allowed to be set by query. (READONLY)
(query: select 1 SETTINGS max_sessions_for_user = 2)
ALTER SETTINGS PROFILE
works fine.
clickhouse-client -q "create profile my_profile"
clickhouse-client -q "ALTER SETTINGS PROFILE 'my_profile' SETTINGS max_sessions_for_user = 2"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed settings source restrictions from SettingsConstraints.cpp
for other parameters, and left just for max_sessions_for_user
just to not accidentally break something. This pool request is already big.
src/Interpreters/Session.cpp
Outdated
session_tracker_handle = session_context->getSessionTracker().trackSession( | ||
*user_id, | ||
{ session_name_ }, | ||
session_context->getSettingsRef().max_sessions_for_user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, seems like it will count one named http session multiple times. Please add a test with named http sessions (see session_id
parameter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a test with different sesssion IDs, but this should not be an issue because session id must be unique for all sessions on server.
I got this error, if i try to use the same session id for HTTP:
ClickHouse HTTP server returned 500 Internal Server Error: Code: 373. DB::Exception: Session 1 is locked by a concurrent client. (SESSION_IS_LOCKED) (version 23.7.1.1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (Test added)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tricky part is that named http sessions can be reused. Of course, it's not allowed to use the same session concurrently, but it's totally fine to use it sequentially from different http requests. A new session will not be created for each request, the old one will be reused instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just tested it locally, and found that it counts an http session only if there's a running query in this session. So it's possible to open an unlimited number of named http sessions (max_sessions_for_user = 2
):
~/ch/test echo "select 1" | curl -X POST "http://localhost:8123/?user=maxsessions&session_id=1" -d @- ✔
1
~/ch/test echo "select 1" | curl -X POST "http://localhost:8123/?user=maxsessions&session_id=2" -d @- ✔
1
~/ch/test echo "select 1" | curl -X POST "http://localhost:8123/?user=maxsessions&session_id=3" -d @- ✔
1
~/ch/test echo "select 1" | curl -X POST "http://localhost:8123/?user=maxsessions&session_id=4" -d @- ✔
1
~/ch/test echo "select 1" | curl -X POST "http://localhost:8123/?user=maxsessions&session_id=5" -d @- ✔
1
(now there are 5 sessions, they will be closed after default_session_timeout
seconds)
But it will not be possible to use more than 2 sessions concurrently anyway, so maybe it's fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, no, it's possible if you update the setting with a SET query: https://pastila.nl/?0120fcc0/a41fe96d1a48546e4a4939fec4fc1c7a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirm this issue. I'm going to fix it in the next update of this PR.
A simple uncoditional call to new_session_context->setUser(*user_id)
in makeSessionContext
for named sessions fixes the issue (in my old code not upmerged with master), But I'm not sure if there are any pitfalls I haven't considered.
I got several merge conflicts with master and can't update the code right now. I hope to finish the update on Monday if I do not meet other issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple uncoditional call to new_session_context->setUser(*user_id) in makeSessionContext for named sessions fixes the issue
See #51724 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue is no longer actual, because SET for max_sessions_for_user
is prohibited now.
A similar case with ALTER SETTINGS PROFILE
works fine. See the test tests/queries/0_stateless/02832_alter_max_sessions_for_user.sh
There's a minor usability issue:
|
This is a known issue. I added a comment for this issue. The client should work fine, but without receiving suggestions for connection. I see two main ways of fixing this:
|
3790f89
to
deabacf
Compare
The style check failed in files I did not change:
|
Upgrade check (asan) - #52540 |
Created and issue to fix this: #51724 (comment) #52843 |
@Demilivor, the new tests are flaky, please take a look: https://s3.amazonaws.com/clickhouse-test-reports/0/1b2e4d3ebd7a51bd528f7bb15167453180832865/integration_tests__asan__[1_6].html |
@tavplubix, we have a "fearless reverting" policy - if a test is flaky - revert the feature. |
I apologize, This test was my first integration test for ClickHouse, I have removed the dependency on sleep and test execution order in this pool request |
Closes #44032.
This change adds a limit on the maximum number of simultaneous sessions per authenticated user.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added max_sessions_for_user setting