Hi, Alexander! Thanks! Looks very straightforward. See few comments below. The main one - I am not sure I like the idea of creating numerous aliases for privileges. This approach looks rather confusing to me. On Feb 26, Alexander Barkov wrote:

revision-id: ec1a860bd09 (mariadb-10.5.0-231-gec1a860bd09) parent(s): b8b5a6a2f9d author: Alexander Barkov <bar@mariadb.com> committer: Alexander Barkov <bar@mariadb.com> timestamp: 2020-02-23 22:09:55 +0400 message:

MDEV-21743 Split up SUPER privilege to smaller privileges

diff --git a/mysql-test/main/alter_user.test b/mysql-test/main/alter_user.test index 9ea98615272..60a36499a55 100644 --- a/mysql-test/main/alter_user.test +++ b/mysql-test/main/alter_user.test @@ -30,7 +30,7 @@ alter user foo;

--echo # Grant super privilege to the user. connection default; -grant super on *.* to foo; +grant READ_ONLY ADMIN on *.* to foo;

--echo comments are now wrong

--echo # We now have super privilege. We should be able to run alter user. connect (b, localhost, foo); diff --git a/mysql-test/main/grant.result b/mysql-test/main/grant.result index e83083be4ed..1452ada11f5 100644 --- a/mysql-test/main/grant.result +++ b/mysql-test/main/grant.result @@ -631,6 +634,10 @@ Super Server Admin To use KILL thread, SET GLOBAL, CHANGE MASTER, etc. Trigger Tables To use triggers Create tablespace Server Admin To create/alter/drop tablespaces Update Tables To update existing rows +Set user Server To create views and stored routines with a different definer +Federated admin Server To execute the CREATE SERVER, ALTER SERVER, DROP SERVER statements +Connection admin Server To skip connection related limits tests

^^^ allows KILL too

+Read_only admin Server To perform write operations even if @@read_only=ON Usage Server Admin No privileges - allow connect only connect root,localhost,root,,test,$MASTER_MYPORT,$MASTER_MYSOCK; connection root; diff --git a/sql/lock.cc b/sql/lock.cc index 6f86c0a38f6..9a4024606f8 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -114,7 +113,7 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) DBUG_ENTER("lock_tables_check");

system_count= 0; - is_superuser= (thd->security_ctx->master_access & SUPER_ACL) != NO_ACL; + is_superuser= (thd->security_ctx->master_access & IGNORE_READ_ONLY_ACL) != NO_ACL;

may be then s/is_superuser/ignores_read_only/ ?

log_table_write_query= (is_log_table_write_query(thd->lex->sql_command) || ((flags & MYSQL_LOCK_LOG_TABLE) != 0));

diff --git a/sql/privilege.h b/sql/privilege.h index 5dbc0b6dbdf..f80e726d8d0 100644 --- a/sql/privilege.h +++ b/sql/privilege.h @@ -59,24 +59,60 @@ enum privilege_t: unsigned long long TRIGGER_ACL = (1UL << 27), CREATE_TABLESPACE_ACL = (1UL << 28), DELETE_HISTORY_ACL = (1UL << 29), // Added in 10.3.4 + SET_USER_ACL = (1UL << 30), // Added in 10.5.2 + FEDERATED_ADMIN_ACL = (1UL << 31), // Added in 10.5.2 + CONNECTION_ADMIN_ACL = (1ULL << 32), // Added in 10.5.2 + READ_ONLY_ADMIN_ACL = (1ULL << 33), // Added in 10.5.2 + REPL_SLAVE_ADMIN_ACL = (1ULL << 34), // Added in 10.5.2 + REPL_MASTER_ADMIN_ACL = (1ULL << 35), // Added in 10.5.2 + BINLOG_ADMIN_ACL = (1ULL << 36) // Added in 10.5.2 /* - don't forget to update - 1. static struct show_privileges_st sys_privileges[] - 2. static const char *command_array[] and static uint command_lengths[] - 3. mysql_system_tables.sql and mysql_system_tables_fix.sql - 4. acl_init() or whatever - to define behaviour for old privilege tables - 5. sql_yacc.yy - for GRANT/REVOKE to work - 6. Add a new ALL_KNOWN_ACL_VERSION - 7. Change ALL_KNOWN_ACL to ALL_KNOWN_ACL_VERSION - 8. Update User_table_json::get_access() + don't forget to update: + In this file: + - Add a new LAST_version_ACL + - Fix PRIVILEGE_T_MAX_BIT + - Add a new ALL_KNOWN_ACL_version + - Change ALL_KNOWN_ACL to ALL_KNOWN_ACL_version + - Change GLOBAL_ACLS if needed + - Change SUPER_ADDED_SINCE_USER_TABLE_ACL if needed + + In other files: + - static struct show_privileges_st sys_privileges[] + - static const char *command_array[] and static uint command_lengths[] + - mysql_system_tables.sql and mysql_system_tables_fix.sql + - acl_init() or whatever - to define behaviour for old privilege tables + - Update User_table_json::get_access() + - sql_yacc.yy - for GRANT/REVOKE to work + + Important: the enum should contain only single-bit values. + In this case, debuggers print bit combinations in the readable form: + (gdb) p (privilege_t) (15) + $8 = (SELECT_ACL | INSERT_ACL | UPDATE_ACL | DELETE_ACL) + + Bit-OR combinations of the above values should be declared outside! */ - - // A combination of all bits defined in 10.3.4 (and earlier) - ALL_KNOWN_ACL_100304 = (1UL << 30) - 1 };

-constexpr privilege_t ALL_KNOWN_ACL= ALL_KNOWN_ACL_100304; +// Version markers +constexpr privilege_t LAST_100304_ACL= DELETE_HISTORY_ACL; +constexpr privilege_t LAST_100502_ACL= BINLOG_ADMIN_ACL; +constexpr privilege_t LAST_CURRENT_ACL= LAST_100502_ACL; +constexpr uint PRIVILEGE_T_MAX_BIT= 36; + +static_assert((privilege_t)(1ULL << PRIVILEGE_T_MAX_BIT) == LAST_CURRENT_ACL, + "LAST_CURRENT_ACL and PRIVILEGE_T_MAX_BIT do not match");

why wouldn't you define PRIVILEGE_T_MAX_BIT via LAST_CURRENT_ACL instead?

+ +// A combination of all bits defined in 10.3.4 (and earlier) +constexpr privilege_t ALL_KNOWN_ACL_100304 = + (privilege_t) ((LAST_100304_ACL << 1) - 1); + +// A combination of all bits defined in 10.5.2 +constexpr privilege_t ALL_KNOWN_ACL_100502= + (privilege_t) ((LAST_100502_ACL << 1) - 1); + +// A combination of all bits defined as of the current version +constexpr privilege_t ALL_KNOWN_ACL= ALL_KNOWN_ACL_100502;

// Unary operators @@ -229,6 +280,104 @@ constexpr privilege_t SHOW_CREATE_TABLE_ACLS= constexpr privilege_t TMP_TABLE_ACLS= COL_DML_ACLS | ALL_TABLE_DDL_ACLS;

+ + +/* + If a VIEW has a `definer=invoker@host` clause and + the specified definer does not exists, then + - The invoker with REVEAL_MISSING_DEFINER_ACL gets: + ERROR: The user specified as a definer ('definer1'@'localhost') doesn't exist + - The invoker without MISSING_DEFINER_ACL gets a generic access error, + without revealing details that the definer does not exists. + + TODO: we should eventually test the same privilege when processing + other objects that have the DEFINER clause (e.g. routines, triggers). + Currently the missing definer is revealed for non-privileged invokers + in case of routines, triggers, etc. +*/ +constexpr privilege_t REVEAL_MISSING_DEFINER_ACL= SUPER_ACL;

1. why did you create these aliases? I don't think they make the code simpler, on the contrary, now one can never know whether say, IGNORE_READ_ONLY_ACL is a real privilege like in GRANT IGNORE READ_ONLY ON *.* TO user@host or it's just an alias. 2. REVEAL_MISSING_DEFINER_ACL should be SET_USER_ACL, I think

+constexpr privilege_t DES_DECRYPT_ONE_ARG_ACL= SUPER_ACL; +constexpr privilege_t LOG_BIN_TRUSTED_SP_CREATOR_ACL= SUPER_ACL;

this could be SET_USER_ACL too

+constexpr privilege_t DEBUG_ACL= SUPER_ACL; +constexpr privilege_t SET_GLOBAL_SYSTEM_VARIABLE_ACL= SUPER_ACL; +constexpr privilege_t SET_RESTRICTED_SESSION_SYSTEM_VARIABLE_ACL= SUPER_ACL; + +/* Privileges related to --read-only */ +constexpr privilege_t IGNORE_READ_ONLY_ACL= READ_ONLY_ADMIN_ACL; + +/* Privileges related to connection handling */ +constexpr privilege_t IGNORE_INIT_CONNECT_ACL= CONNECTION_ADMIN_ACL; +constexpr privilege_t IGNORE_MAX_USER_CONNECTIONS_ACL= CONNECTION_ADMIN_ACL; +constexpr privilege_t IGNORE_MAX_CONNECTIONS_ACL= CONNECTION_ADMIN_ACL; +constexpr privilege_t IGNORE_MAX_PASSWORD_ERRORS_ACL= CONNECTION_ADMIN_ACL; +// Was SUPER_ACL prior to 10.5.2 +constexpr privilege_t KILL_OTHER_USER_PROCESS_ACL= CONNECTION_ADMIN_ACL; + + +/* + Binary log related privileges that are checked regardless + of active replication running. +*/ + +/* + This command was renamed from "SHOW MASTER STATUS" + to "SHOW BINLOG STATUS" in 10.5.2. + Was SUPER_ACL | REPL_CLIENT_ACL prior to 10.5.2 +*/ +constexpr privilege_t STMT_SHOW_BINLOG_STATUS_ACL= REPL_CLIENT_ACL; + +// Was SUPER_ACL | REPL_CLIENT_ACL prior to 10.5.2 +constexpr privilege_t STMT_SHOW_BINARY_LOGS_ACL= REPL_CLIENT_ACL; + +// Was SUPER_ACL prior to 10.5.2 +constexpr privilege_t STMT_PURGE_BINLOG_ACL= BINLOG_ADMIN_ACL; + +// Was REPL_SLAVE_ACL prior to 10.5.2 +constexpr privilege_t STMT_SHOW_BINLOG_EVENTS_ACL= PROCESS_ACL; + + +/* + Privileges for replication related statements + that are executed on the master. +*/ +constexpr privilege_t COM_REGISTER_SLAVE_ACL= REPL_SLAVE_ACL; +constexpr privilege_t COM_BINLOG_DUMP_ACL= REPL_SLAVE_ACL; +// Was REPL_SLAVE_ACL prior to 10.5.2 +constexpr privilege_t STMT_SHOW_SLAVE_HOSTS_ACL= REPL_MASTER_ADMIN_ACL; + + +/* Privileges for statements that are executed on the slave */ +// Was SUPER_ACL prior to 10.5.2 +constexpr privilege_t STMT_START_SLAVE_ACL= REPL_SLAVE_ADMIN_ACL; +// Was SUPER_ACL prior to 10.5.2 +constexpr privilege_t STMT_STOP_SLAVE_ACL= REPL_SLAVE_ADMIN_ACL; +// Was SUPER_ACL prior to 10.5.2 +constexpr privilege_t STMT_CHANGE_MASTER_ACL= REPL_SLAVE_ADMIN_ACL; +// Was (SUPER_ACL | REPL_CLIENT_ACL) prior to 10.5.2 +constexpr privilege_t STMT_SHOW_SLAVE_STATUS_ACL= REPL_SLAVE_ADMIN_ACL; +// Was SUPER_ACL prior to 10.5.2 +constexpr privilege_t STMT_BINLOG_ACL= REPL_SLAVE_ADMIN_ACL; +// Was REPL_SLAVE_ACL prior to 10.5.2 +constexpr privilege_t STMT_SHOW_RELAYLOG_EVENTS_ACL= REPL_SLAVE_ADMIN_ACL; + + +/* Privileges for federated database related statements */ +// Was SUPER_ACL prior to 10.5.2 +constexpr privilege_t STMT_CREATE_SERVER_ACL= FEDERATED_ADMIN_ACL; +// Was SUPER_ACL prior to 10.5.2 +constexpr privilege_t STMT_ALTER_SERVER_ACL= FEDERATED_ADMIN_ACL; +// Was SUPER_ACL prior to 10.5.2 +constexpr privilege_t STMT_DROP_SERVER_ACL= FEDERATED_ADMIN_ACL; + + +/* Privileges related to processes */ +constexpr privilege_t COM_PROCESS_INFO_ACL= PROCESS_ACL; +constexpr privilege_t STMT_SHOW_EXPLAIN_ACL= PROCESS_ACL; +constexpr privilege_t STMT_SHOW_ENGINE_STATUS_ACL= PROCESS_ACL; +constexpr privilege_t STMT_SHOW_ENGINE_MUTEX_ACL= PROCESS_ACL; +constexpr privilege_t STMT_SHOW_PROCESSLIST_ACL= PROCESS_ACL; + + /* Defines to change the above bits to how things are stored in tables This is needed as the 'host' and 'db' table is missing a few privileges diff --git a/sql/sql_connect.cc b/sql/sql_connect.cc index e2a3c482ae4..b096bfa7a95 100644 --- a/sql/sql_connect.cc +++ b/sql/sql_connect.cc @@ -1246,7 +1245,8 @@ void prepare_new_connection_state(THD* thd) thd->set_command(COM_SLEEP); thd->init_for_queries();

- if (opt_init_connect.length && !(sctx->master_access & SUPER_ACL)) + if (opt_init_connect.length && + (sctx->master_access & IGNORE_INIT_CONNECT_ACL) == NO_ACL)

dunno, I kind of like !(access & SOME_ACL) why not to keep privilege_t -> bool?

{ execute_init_command(thd, &opt_init_connect, &LOCK_sys_init_connect); if (unlikely(thd->is_error())) diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index dac5b025821..7f3a436a4c2 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -7138,8 +7138,7 @@ bool check_some_access(THD *thd, privilege_t want_access, TABLE_LIST *table) @warning One gets access right if one has ANY of the rights in want_access. This is useful as one in most cases only need one global right, - but in some case we want to check if the user has SUPER or - REPL_CLIENT_ACL rights. + but in some case we want to check multiple rights.

In what cases? Are there any left?

@retval 0 ok

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org