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
Improve isolation of query cache entries under re-created users or role switches #58611
Conversation
This is an automated comment for commit bd9e38f 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
|
src/Interpreters/Cache/QueryCache.h
Outdated
/// - after DROP USER, it must not be possible to create a new user with with the dropped user name and access the dropped user's | ||
/// query cache entries | ||
/// - different roles of the same user may be tied to different row-level policies. It must not be possible to switch role and | ||
/// access another role's cache entries | ||
const String user_name; |
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.
Is user_name
still used? It seems it's enough to compare only user_id
.
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.
You are right. But before fixing this: do you know why the user_id
/ getUserID()
in Context
is an std::optional
? When would the user id be not set?
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.
user_id
can be nullopt only if it's the global context. Some internal queries are executed in the global context. For example ATTACH TABLE
queries for existing tables when the server starts; or INSERT
queries from a Buffer to its destination table. If user_id
is nullopt then user_name
is always empty anyway.
@vitlibar Approve? Thanks! |
src/Common/TTLCachePolicy.h
Outdated
@@ -11,37 +13,47 @@ namespace DB | |||
class PerUserTTLCachePolicyUserQuota : public ICachePolicyUserQuota | |||
{ | |||
public: | |||
void setQuotaForUser(const String & user_name, size_t max_size_in_bytes, size_t max_entries) override | |||
void setQuotaForUser(std::optional<UUID> user_id, size_t max_size_in_bytes, size_t max_entries) override |
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.
setQuotaForUser()
doesn't look good when its parameter is std::optional<UUID>
. I mean logically setQuotaForUser(nullopt, ...)
looks a bit pointless (you set something for an absent user). The function doesn't do anything useful anyway if this parameter is nullopt, so what I suggest is just moving this condition if (!user_id)
outside of the function and turn the type of the parameter into a plain const UUID & user_id
. And for other functions increaseActual()
, decreaseActual()
, approveWrite()
it's the same.
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 a good idea, the internal users would have std::optional<UUID> = std::nullopt
and that can't be configured via settings anyways. Rewrote this the way you proposed.
# - drop the user, recreate it with the same name | ||
# - test that the cache entry is inaccessible | ||
|
||
${CLICKHOUSE_CLIENT} --query "SELECT 'Attack 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.
echo "Attack 1"
is shorter (though it doesn't matter)
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
…d28e2f1c322d38d66bcf1f9ab5c096 Cherry pick #58611 to 23.12: Improve isolation of query cache entries under re-created users or role switches
…der re-created users or role switches
Backport #58611 to 23.12: Improve isolation of query cache entries under re-created users or role switches
…28e2f1c322d38d66bcf1f9ab5c096 Cherry pick #58611 to 23.8: Improve isolation of query cache entries under re-created users or role switches
…er re-created users or role switches
Backport #58611 to 23.8: Improve isolation of query cache entries under re-created users or role switches
Fixes #58054
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
The query cache now denies access to entries when the user is re-created or assumes another role. This improves prevents attacks where 1. an user with the same name as a dropped user may access the old user's cache entries or 2. a user with a different role may access cache entries of a role with a different row policy.