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 <b...@mariadb.com>
> committer: Alexander Barkov <b...@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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
