docs: Add Proposal for dynamic privileges #23224
Conversation
|
|
||
| ## Impact & Risks | ||
|
|
||
| In its initial release, dynamic privilege usage will be controlled by an experimental feature flag. The implementation will be via restricting GRANT and REVOKE statements from creating dynamic privileges (it is too invasive to conditionally modify the ast visitor functionaliy). |
There was a problem hiding this comment.
This text is new from the original proposal.
| * Privileges can only be global scoped. | ||
| * Privileges are stored in the table `mysql.global_grants` and not `mysql.user`. |
There was a problem hiding this comment.
which privilege, dynamic or static?
There was a problem hiding this comment.
These points are all describing dynamic privileges. See above where it says "dynamic privileges are different from static privileges in the following ways".
| For TiDB, we can use the same table structure as MySQL: | ||
|
|
||
| ```sql | ||
| CREATE TABLE `global_grants` ( |
There was a problem hiding this comment.
the host field in mysql.user table is defined as char(64), which is smaller than this table, which is char(2455):
| Host | char(64) | NO | PRI | NULL | |I'm curious which one is correct, should we unify the length of this field for different tables?
There was a problem hiding this comment.
I think we should use the same format as MySQL, since this supports the use case of restoring a mysqldump (which overwrites these tables). The example here for global_grants is identical to MySQL 8.0, but the mysql.user table is not.
There are several reasons why the mysql.user table is not:
- TiDB adds privileges like Config (really, this should be a dynamic Priv)
- TiDB stores TLS settings in global_priv, not mysql.user.
- TiDB does not yet support authentication plugins.
So yes, the mysql.user table is incorrect. In MySQL 8.0 this is:
| Host | char(255) | NO | PRI | | || ); | ||
| ``` | ||
|
|
||
| There is an existing table called “global_priv” which initially looked like it provided similar functionality, except: |
There was a problem hiding this comment.
global_priv and global_grants might confuse the users, IMHO, it's better to merge them or have distinguishing names based on their purposes.
@lysu, seems like the global_priv table was introduced by you, do you have any idea about how to merge these two tables? Or can we rename this table to another name?
There was a problem hiding this comment.
The global_priv table does not exist in MySQL. As far as I can tell these columns are stored in mysql.user in MySQL.
I think it should be cleaned up to be MySQL compatible so restoring mysqldump can work as expected, but would suggest we handle it outside of this proposal, since for storage this follows MySQL exactly.
|
|
||
| This document was created to discuss the design of Dynamic Privileges. It is intended to be implemented in combination with Security Enhanced Mode, but there no interdependencies between the two features. | ||
|
|
||
| ## Motivation or Background |
There was a problem hiding this comment.
could you make a specific example problem to help users have a better understanding of this proposal?
There was a problem hiding this comment.
I think the following "Audit Admin" is the example: suppose we want some administrators to be able to change audit settings, but before Dynamic Priviligies, the only way is to grant "SUPER" because the privilige is too coarse.
BTW, IMHO the naming of 'Dyanmic Priviligies' is kind of misleading, the core intention of it is make the priviligies 'fine grained' and 'dynamic'.
There was a problem hiding this comment.
I think it is maybe an implementation detail, but I don't think it is misleading. In my original proposal I started it with a section called 'terminology', but this is not part of the new standard template, so I removed it.
The implementation detail is more relevant to MySQL, because it has quite a number of plugins. It is less relevant to TiDB, but it is refered to as dynamic privileges in the documentation, so I don't recommend changing it.
I am happy to add an example to make it clearer though.
There was a problem hiding this comment.
I have added a paste of a testcase. There are additional examples in the linked MySQL worklog WL#8131, which is in the first sentence.
|
|
||
| ### Scenario Tests | ||
|
|
||
| The use-cases required by the DBaaS team should be validated when combined with `security-enhanced-mode`. They are: |
There was a problem hiding this comment.
We also need to add scenario tests for:
- all the dynamic privileges
- several user-defined dynamic privileges
By validating what they can and can't do to ensure the dynamic privilege works as expected.
|
|
||
| ### Compatibility Tests | ||
|
|
||
| The introduction of `DYNAMIC` privileges is not expected to introduce any compatibility issues, because backwards compatibility is ensured. However, plugins should migrate to registering their own dynamic privileges and not rely on the use of `SUPER`. This is considered an enhancement, and not included in-scope for the initial introduction of dynamic privileges (which introduces the framework for plugins to use). |
There was a problem hiding this comment.
- Should BR, lightning use the new dynamic privilege as well?
- We need to ensure the cluster can be downgraded in case the upgrading is failed due to some reasons, how to downgrade the cluster to an older version?
There was a problem hiding this comment.
Downgrade should work, but I can explain why/how. The reason is that the code requests for a specific privilege (such as "BACKUP_ADMIN"), which TIDB also allows SUPER. In an earlier version of the code the privilege check just requires SUPER.
However, if they don't have SUPER and explicitly grant BACKUP ADMIN, then downgrade will cause a privilege check failure becaues they don't have SUPER.
There was a problem hiding this comment.
The built-in BACKUP command will be changed to BACKUP_ADMIN. Currently lightning is not a server-plugin (IMHO it should be) so it can't register new dynamic privileges, but maybe the recommended credentials required to do a lightning restore could change to be more fine grained.
We should always recommend the minimum set of privileges, so this makes sense to me. But it is not specifically a compatibility problem because backwards compatibility is ensured.
|
|
||
| ## Impact & Risks | ||
|
|
||
| In its initial release, dynamic privilege usage will be controlled by an experimental feature flag. The implementation will be via restricting `GRANT` and `REVOKE` statements from creating dynamic privileges (it is too invasive to conditionally modify the ast visitor functionaliy). |
There was a problem hiding this comment.
could you describe more about the feature flag? is it a global variable that can or can't be seen by users via show variables?
There was a problem hiding this comment.
Yes. I added tidb_enable_dynamic_privileges in b7809f6
|
|
||
| In its initial release, dynamic privilege usage will be controlled by an experimental feature flag. The implementation will be via restricting `GRANT` and `REVOKE` statements from creating dynamic privileges (it is too invasive to conditionally modify the ast visitor functionaliy). | ||
|
|
||
| For backwards compatibility, the MySQL-compatible dynamic privileges will also permit `SUPER`. This helps prevent upgrade issues, such a when TiDB was bootstrapped `GRANT ALL ON *.*` would not have granted all the dynamic privileges. There might be some impact on Upgrade/Downgrade story if eventually the `BACKUP_ADMIN` privilege is used instead of `SUPER`, but for the initial release I am planning to allow either. |
There was a problem hiding this comment.
I'm curious whether there are requirements for MySQL compatible dynamic privileges at present, @IANTHEREAL do you have any idea?
There was a problem hiding this comment.
To paraphrase your question: is there a requirement where a user with SUPER but not a DYNAMIC privilege will be denied access to perform a task such as BACKUP. The main difference is:
- In MySQL, you can not
GRANTa dynamic privilegefoounless you have the dynamic privilegefoowithgrant_option. - In TiDB you need either the dynamic privilege
foowithgrant optionorsuper with grant option.
This caters for the case that the root user already has super, but not each of the dynamic privileges. There is an existing incompatibility in TiDB where grant all does not actually truly grant all. I don't plan to fix it.
(Note: Slightly contradicting this, if SEM is enabled, there will be cases where super is locked down and a tidb-specific dynamic privilege is required. This is the main goal of SEM: restrict SUPER.)
There was a problem hiding this comment.
I think @zz-jason’s question is,
is there a scenario where a user with BACKUP_ADMIN but not a SUPER privilege?
Yes, it has. On cloud, SRE should have the ability to perform BACKUP, without a SUPER privilege. So the BACKUP_ADMIN privilege should be granted to SRE’s account.
There was a problem hiding this comment.
Ah okay. Yes, consider the case that the backup is encrypted. You might have permission to configure the backup and restore, but no visibility into the data. This is a good feature to have.
Co-authored-by: Zhang Jian <zjsariel@gmail.com>
…roposal-dynamic-privs
| | Account Name | root | cloudAdmin | | ||
| | --------------- | --------------- | --------------- | | ||
| | Backup & Restore to cloud | Y | Y | | ||
| | File privilege | N | N | |
There was a problem hiding this comment.
Another question is currently cloudAdmin does not have FILE privilege, but if SREs do want this privilege in the future, how will they grant it to themselves.
There was a problem hiding this comment.
It is possible by starting the server up in skip-grant-tables mode. I would say for security reasons they should avoid needing FILE though. Instead of using SELECT INTO OUTFILE, they can use the client to write the file:
mysql -BNe "SELECT * FROM db.table" > output.txtThis is a lot less vulnerable to privilege escalations.
There was a problem hiding this comment.
Can cloudAdmin insert into some information_schema tables to obtain some privileges?
I just point FILE as an example, in the future we may implement more dynamic privileges and SRE may want to use some of them.
There was a problem hiding this comment.
Information_schema is always non-insertable. I think we would have to write a session/bootstrap.go task to essentially "split" a privilege to say if you have XYZ now you have XYZ and ZYX.
The problem with allowing cloudAdmin to extend their privileges is that it is essentially a privilege escalation, since you are only supposed to be able to grant privileges that you already have. There is one such "legal" privilege escalation that is hard coded into this design (SUPER always satisfies a non-restricted dynamic privilege*). The design of this legal escalation is to get around a limitation:
visitorInfo does not currently support the concept of an OR for permissions, so while in MySQL it is possible to require either BACKUP_ADMIN or SUPER the actual implementation in TiDB is that it requires BACKUP_ADMIN, but the privilege check API will explicitly fallback and check if the SUPER privilege is assigned.
* Exception = Restricted privileges
There was a problem hiding this comment.
sorry, I said the wrong table before. cloudAdmin can insert mysql.user table to extend their privilege by GRANT SELECT, INSERT, UPDATE ON mysql.* TO 'cloudAdmin'@'%%'; when initializing the account.
The requirement is reasonable because in the future we may implement more privileges like XYZ and cloudAdmin needs that.
I know there's no design for privilege escalation so maybe inserting into mysql.user is the best solution.
And I'm happy if you put this solution into your design doc and future user doc.
There was a problem hiding this comment.
It would be mysql.user to extend static privileges or mysql.global_grants for XYZ since it is a dynamic priv, followed by FLUSH PRIVILEGES. I can add a section on extending privileges; it's actually a risk to grant SELECT, INSERT, UPDATE ON mysql.*
There was a problem hiding this comment.
This has been added to unanswered questions here: 0ea98a1
There was a problem hiding this comment.
I think "split" a privilege is too hacking in bootstrap.go. If you like this option, I say why not we maintain the cloudAdmin account in bootstrap.go
|
|
||
| ### Plugin API | ||
|
|
||
| There will need to be a way for plugins to register new dynamic privileges via their OnInit method. I propose the following: |
There was a problem hiding this comment.
Dynamic privilege is bind with the plugin?
There was a problem hiding this comment.
The plugin registers the permission, but the binding is loose. If the plugin is uninstalled there could be users who still have grants assigned to those dynamic names. I checked the intended behavior in MySQL, which is to report error ErrDynamicPrivilegeNotRegistered = 3929 in revoke context, and parse error in GRANT context.
My plan is to implement a technically incompatble (but improved behavior) or returning 3929 in both instances. For example see: https://github.com/pingcap/tidb/pull/22778/files#diff-83d52377771c642f1bbe6ecaf9a98e6d3787f57c715fb468e26abce2f3d51dadR437-R442 and https://github.com/pingcap/tidb/pull/22778/files#diff-fc0b54ba2dcfe653ca33434b6eabd9d5a06e87883827b29deddc134f6941fb05R193-R195
In MySQL, uninstalling a plugin is only effective upon server restart. I believe the same is true of TiDB plugins, but effectively because there are multiple TiDB instances we need to expect that some servers may effectively not have all plugins loaded.
An alternative design would be to permanently retain registered DYNAMIC privileges in an internal table. The upside of this is fewer errors to clients. The downside is that it can produce a similar semantic to "parse but ignore", because it accepts input for which the plugins are not correctly loaded and will never be able to correctly use this functionality. i.e. it is a feature to return an error, not a bug.
|
I like this design, DBaaS may benefit a lot from the fine-grained privileges. @tennix |
|
/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. Reviewer can indicate their review by writing |
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 5a0a102
|
|
Please hold the merging, I have a few more comments. Sorry. |
|
❤️ I like this feature. It is a must-have to securely serve TiDB Dashboard to users in TiDB Cloud. It also enables users to use TiDB Dashboard features without a dangerous |
|
/lgtm |
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 77c7b13
|
|
/merge |
|
Hi @morgo, we are preparing the user docs for TiDB v5.1 and would like to know whether the doc changes of this PR apply to TiDB v5.1 or not. Would you please check and reply? Thanks! |
Yes it is. |
What problem does this PR solve?
Issue Number: Design doc for #22439
Problem Summary:
What is changed and how it works?
Adds a proposal for dynamic privileges.
This was proposed earlier, but rejected for sprint 2. I am re-adding it because it was clarified that if features can be disabled by default behind a feature flag they are still permitted.
Related changes
Check List
Tests
Side effects
Release note