*: add security enhanced mode part 2#24279
Conversation
|
Please follow PR Title Format:
Or if the count of mainly changed packages are more than 3, use
|
|
@SunRunAway There are a couple of small changes to the proposal doc in this PR. PTAL and make sure you are comfortable with them, thanks! |
LGTM for these changes! |
| * cluster_* tables | ||
| * The “instance” column will show the server ID instead of the server IP address. | ||
| * statements_summary* | ||
| * The query_sample_text will return empty unless the user has the `PROCESS` privilege. |
There was a problem hiding this comment.
I think this change may break compatibility.
If a customer does have access to the statements_summary table but does not have PROCESS privilege, they can not read the query_sample_text.
We may discuss whether there is another way to prevent cloudAdmin from accessing the original SQL text.
cc @breeswish
There was a problem hiding this comment.
I think it might be best to revert this part of the proposal if there is more discussion needed. We can handle in a followup PR instead.
This was covered in a previous update. The sentence is:
|
How about "The remaining system tables will be limited to read-only operations and can not create new tables" |
|
/lgtm |
|
/LGTM |
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. DetailsReviewer can indicate their review by writing |
|
/merge |
|
This pull request has been accepted and is ready to merge. DetailsCommit hash: 055050f |
| Hostname: "localhost", | ||
| AuthUsername: "uroot", | ||
| AuthHostname: "%", | ||
| }, nil, nil) |
There was a problem hiding this comment.
The test seems not done here？
There was a problem hiding this comment.
The comment explains why the test can't work :( The status vars don't install with mocktikv/unistore.
But checking that the permissions are assignable is still valuable, because I discovered an issue when writing this test. The original name for the permission was > 32 characters long, which is not permitted for dynamic privileges.
Co-authored-by: Arenatlx <ailinsilence4@gmail.com>
@morgo @tiancaiamao @bb7133 Cloud you solve this before merging this PR? |
I've updated this in 1e8792a |
|
/merge |
|
This pull request has been accepted and is ready to merge. DetailsCommit hash: fa92dbb |
What problem does this PR solve?
This implements the second of presumably 4 PRs
Problem Summary:
What is changed and how it works?
Proposal: https://github.com/pingcap/tidb/blob/master/docs/design/2021-03-09-security-enhanced-mode.md
What's Changed:
This introduces 2 areas of functionality:
SHOW STATUSvariables.There are two small updates to the SEM proposal:
Because the original privilege name of
RESTRICTED_STATUS_VARIABLES_ADMINexceeds 32 characters I have renamed it toRESTRICTED_STATUS_ADMIN(there is an alternative of changing the table definition, but 32 chars is mysql compatible).A user without process privilege should not be able to see query samples. This will help in the future when the tidb-dashboard connects to the TIDB server to read statements_summary with theRemoved for now, will handle in a followup PR.DASHBOARD_ADMINprivilege.Related changes
Check List
Tests
The tikv status variable only registers with the server when a tikv server is present. To test this, I've had to use a manual test (as well as unit test on the SEM package), but no integration test in TIDB.
Side effects
Release note