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

Do not require super privilege for all admin statements #14621

Closed
DanielZhangQD opened this issue Feb 4, 2020 · 9 comments
Closed

Do not require super privilege for all admin statements #14621

DanielZhangQD opened this issue Feb 4, 2020 · 9 comments
Labels
security Everything related with security type/compatibility

Comments

@DanielZhangQD
Copy link

Feature Request

Is your feature request related to a problem? Please describe:

super privilege is revoked for TiDB clusters created on DBaaS for security consideration, however, some tools, like lightning, will run some admin statements, e.g. admin checksum table, in this case, DBaaS users cannot import data with root user.
Describe the feature you'd like:

Maybe no need to require super privilege for some admin statements.
cc @gregwebs

@DanielZhangQD DanielZhangQD added the type/enhancement The issue or PR belongs to an enhancement. label Feb 4, 2020
@gregwebs gregwebs added type/compatibility security Everything related with security and removed type/enhancement The issue or PR belongs to an enhancement. labels Feb 4, 2020
@gregwebs
Copy link
Contributor

gregwebs commented Feb 4, 2020

We should carefully consider what privileges are needed. Requiring super privilege when it is unnecessary reduces security. In MySQL, the checksum command requires just SELECT privilege: https://dev.mysql.com/doc/refman/5.7/en/checksum-table.html

@DanielZhangQD lets review more statements that require super privilege.

@zz-jason
Copy link
Member

zz-jason commented Feb 5, 2020

the keyword admin implies that the command is executed by administrators, which further implies the command requires super privilege.

IMHO:

  1. we need to support commands that not require super privilege, for example support CHECKSUM TABLE tbl_name which provides the same function with ADMIN CHECKSUM TABLE tbl_name but requires only select privilege.
  2. we need to review more admin statements, check whether they can be replaced with another command, which requires a lower privilege.

@DanielZhangQD
Copy link
Author

the keyword admin implies that the command is executed by administrators, which further implies the command requires super privilege.

IMHO:

  1. we need to support commands that not require super privilege, for example support CHECKSUM TABLE tbl_name which provides the same function with ADMIN CHECKSUM TABLE tbl_name but requires only select privilege.
  2. we need to review more admin statements, check whether they can be replaced with another command, which requires a lower privilege.

Yes, a good solution.
Do we have any timeline for this fix? It would be great if we can include the fix (also some tools involved, e.g. lightning) for dbaas GA.

@DanielZhangQD
Copy link
Author

We should carefully consider what privileges are needed. Requiring super privilege when it is unnecessary reduces security. In MySQL, the checksum command requires just SELECT privilege: https://dev.mysql.com/doc/refman/5.7/en/checksum-table.html

@DanielZhangQD lets review more statements that require super privilege.

Yes, we can check what users cannot do with existing root user during dbaas development.

@kolbe
Copy link
Contributor

kolbe commented Feb 19, 2020

At a minimum, we should model behavior of SUPER on the range of things it is used for in MySQL: https://dev.mysql.com/doc/refman/8.0/en/privileges-provided.html#priv_super.

Only things that affect the entire server instance, or items outside the scope of individual privileges granted to a user (for a specific database, table, column, etc.) should ever have any relationship with the SUPER privilege.

I can't imagine why CHECKSUM TABLE would require SUPER, maybe just because it was easiest to add the CHECKSUM command as an "ADMIN" command, and it's been easiest to make all ADMIN commands require SUPER?

Note that MySQL is already moving to a system of "dynamic privileges" and is planning on removing SUPER in a future release: https://dev.mysql.com/doc/refman/8.0/en/privileges-provided.html#dynamic-privileges-migration-from-super. We should follow suit.

@kolbe
Copy link
Contributor

kolbe commented Feb 20, 2020

For the ADMIN statements documented at https://pingcap.com/docs/stable/reference/sql/statements/admin/, here's the privileges it seems like they ought to naturally require:

ADMIN...

  • SHOW DDL PROCESS
  • SHOW SLOW PROCESS
  • CANCEL DDL SUPER, PROCESS
  • CHECK TABLE SELECT
  • CHECK INDEX SELECT
  • RECOVER INDEX INDEX
  • CLEANUP INDEX INDEX

And these are not documented:

  • RESTORE TABLE
  • CHECKSUM TABLE
  • SHOW TableName NEXT_ROW_ID

@gregwebs
Copy link
Contributor

We do have RBAC now, so it seems we should be able to approach this in a MySQL compatible way now.

@kennytm
Copy link
Contributor

kennytm commented Mar 11, 2020

ADMIN CHECKSUM TABLE cannot be directly named to CHECKSUM TABLE because it would have totally different output from MySQL's counterpart.

mysql> checksum table mysql.user;
+------------+-----------+
| Table      | Checksum  |
+------------+-----------+
| mysql.user | 925182392 |
+------------+-----------+
1 row in set (0.07 sec)

mysql> admin checksum table mysql.user;
+---------+------------+---------------------+-----------+-------------+
| Db_name | Table_name | Checksum_crc64_xor  | Total_kvs | Total_bytes |
+---------+------------+---------------------+-----------+-------------+
| mysql   | user       | 2464715565045006727 |         2 |         199 |
+---------+------------+---------------------+-----------+-------------+
1 row in set (0.00 sec)

@morgo
Copy link
Contributor

morgo commented Jan 21, 2021

We are in the process of adding Dynamic privileges in #22439

I am going to close this issue now as a duplicate of dynamic privileges. Please feel free to comment if you would like a specific DYNAMIC privilege added for a subset of ADMIN commands.

There is also a proposal which also reduces the power of SUPER privilege (which might be the real answer here, combined with DYNAMIC privileges). See: #22373

@morgo morgo closed this as completed Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Everything related with security type/compatibility
Projects
None yet
Development

No branches or pull requests

6 participants