Skip to content
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

SELECT from the system database requires grant now #38970

Merged

Conversation

vitlibar
Copy link
Member

@vitlibar vitlibar commented Jul 7, 2022

Changelog category:

  • Improvement

Changelog entry:

Add option enabling that SELECT from the system database requires grant. Details:

1. Add options access_control_improvements.select_from_system_db_requires_grant to the main config.

<access_control_improvements>
        <select_from_system_db_requires_grant>false</select_from_system_db_requires_grant>
</access_control_improvements>

If it's set to false a SELECT from any table in the system database can be executed without any grants.

If it's set to true then such SELECTs require GRANT SELECT ON system.<table> just like for ordinary databases.
Exceptions: a few system tables (tables, columns, databases, and some constant tables like one, contributors) are still accessible for everyone; and if there is a SHOW privilege (e.g. SHOW USERS) granted the corresponding system table (i.e. system.users) will be accessible.

2. Add option access_control_improvements.select_from_information_schema_requires_grant to the main config.

<access_control_improvements>
        <select_from_information_schema_requires_grant>false</select_from_information_schema_requires_grant>
</access_control_improvements>

If it's set to false a SELECT from any table in the information_schema database can be executed without any grants.
If it's set to true then such SELECTs require GRANT SELECT ON information_schema.<table> just like for ordinary databases.

3. Add new table function viewIfPermitted:

viewIfPermitted(query ELSE null(structure))

works as view(query) if the current user has permission to execute query, otherwise it works as null(structure)

@vitlibar vitlibar changed the title Add option enabling that SELECT from the system database requires grant. Add option enabling that SELECT from the system database requires grant Jul 7, 2022
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-improvement Pull request with some product improvements label Jul 7, 2022
@vitlibar vitlibar force-pushed the select-from-system-db-requires-grant branch 7 times, most recently from 2fa9206 to 6e900e1 Compare July 11, 2022 23:35
@vitlibar vitlibar marked this pull request as ready for review July 11, 2022 23:35
@vitlibar vitlibar changed the title Add option enabling that SELECT from the system database requires grant SELECT from the system database requires grant now Jul 11, 2022
@serxa serxa self-assigned this Jul 12, 2022
@vitlibar vitlibar force-pushed the select-from-system-db-requires-grant branch 2 times, most recently from 2c24e75 to 980964f Compare July 13, 2022 09:47
Copy link
Member

@serxa serxa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

catch (Exception & e)
{
if (e.code() == ErrorCodes::ACCESS_DENIED)
return {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just false?

Comment on lines -416 to +487
return access_granted();
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this right? looks inconsistent, and the same couple lines below

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those two lines are not the same, here we return true if this is the global context and below we return true if access flags don't contain any flags (so nothing to check).

@vitlibar vitlibar force-pushed the select-from-system-db-requires-grant branch from 980964f to 1ea8db5 Compare July 15, 2022 12:43
@vitlibar vitlibar force-pushed the select-from-system-db-requires-grant branch from 1ea8db5 to 3eb847f Compare July 15, 2022 13:44
@vitlibar
Copy link
Member Author

The issue with test test_backup_restore_on_cluster will be fixed in a separate PR.

@vitlibar vitlibar merged commit e0fb03d into ClickHouse:master Jul 16, 2022
@vitlibar vitlibar deleted the select-from-system-db-requires-grant branch July 16, 2022 20:37
@@ -106,7 +106,9 @@ void Client::processError(const String & query) const
std::vector<String> Client::loadWarningMessages()
{
std::vector<String> messages;
connection->sendQuery(connection_parameters.timeouts, "SELECT message FROM system.warnings", "" /* query_id */,
connection->sendQuery(connection_parameters.timeouts,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vitlibar Maybe client should check server version/revision to see if function is supported, to make new client compatible with older servers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants