Hi, Sujatha!
On Nov 12, Sujatha wrote:
> revision-id: d66d5457589d (mariadb-10.5.4-315-gd66d5457589d)
> parent(s): 10b2d5726fa2
> author: Sujatha <sujatha.sivaku...@mariadb.com>
> committer: Sujatha <sujatha.sivaku...@mariadb.com>
> timestamp: 2020-11-11 18:41:44 +0530
> message:
>
> MDEV-23610: Slave user can't run "SHOW SLAVE STATUS" anymore after upgrade to
> 10.5, mysql_upgrade should take of that
>
> Add a new privilege "REPLICA MONITOR" which will grant user the permission
> to execute "SHOW SLAVE STATUS" and "SHOW RELAYLOG EVENTS" commands.
>
> SHOW SLAVE STATUS requires either REPLICA MONITOR/SUPER
> SHOW RELAYLOG EVENTS requires REPLICA MONITOR privilege.
>
> diff --git a/mysql-test/main/grant.result b/mysql-test/main/grant.result
> index 01daf0271867..a41b38aaa0ba 100644
> --- a/mysql-test/main/grant.result
> +++ b/mysql-test/main/grant.result
> @@ -1966,10 +1967,11 @@ GRANT REPLICATION SLAVE ON *.* TO
> mysqltest_u1@localhost;
> GRANT SHOW DATABASES ON *.* TO mysqltest_u1@localhost;
> GRANT SHUTDOWN ON *.* TO mysqltest_u1@localhost;
> GRANT USAGE ON *.* TO mysqltest_u1@localhost;
> +GRANT REPLICA MONITOR ON *.* TO mysqltest_u1@localhost;
I don't think we should have a mix of REPLICA and SLAVE terms.
Call it SLAVE MONITOR and when we flip the switch on grants, we'll do
it for all grants at the same time.
> SHOW GRANTS FOR mysqltest_u1@localhost;
> Grants for mysqltest_u1@localhost
> -GRANT RELOAD, SHUTDOWN, PROCESS, FILE, SHOW DATABASES, REPLICATION SLAVE,
> BINLOG MONITOR, CREATE USER ON *.* TO `mysqltest_u1`@`localhost`
> +GRANT RELOAD, SHUTDOWN, PROCESS, FILE, SHOW DATABASES, REPLICATION SLAVE,
> BINLOG MONITOR, CREATE USER, REPLICA MONITOR ON *.* TO
> `mysqltest_u1`@`localhost`
> GRANT CREATE TEMPORARY TABLES, LOCK TABLES, EXECUTE, CREATE ROUTINE, ALTER
> ROUTINE, EVENT ON `mysqltest_db1`.* TO `mysqltest_u1`@`localhost`
> connect con1,localhost,mysqltest_u1,,mysqltest_db1;
> connection con1;
> diff --git a/mysql-test/main/grant.test b/mysql-test/main/grant.test
> index 38baf6738255..63be18ecb292 100644
> --- a/mysql-test/main/grant.test
> +++ b/mysql-test/main/grant.test
> @@ -1833,6 +1833,7 @@ GRANT REPLICATION SLAVE ON *.* TO
> mysqltest_u1@localhost;
> GRANT SHOW DATABASES ON *.* TO mysqltest_u1@localhost;
> GRANT SHUTDOWN ON *.* TO mysqltest_u1@localhost;
> GRANT USAGE ON *.* TO mysqltest_u1@localhost;
> +GRANT REPLICA MONITOR ON *.* TO mysqltest_u1@localhost;
But the parser should, of course, accept both, as everywhere.
> diff --git a/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test
> b/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test
> new file mode 100644
> index 000000000000..f7b2af2e0c51
> --- /dev/null
> +++ b/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test
> @@ -0,0 +1,90 @@
> +# ==== Purpose ====
> +#
> +# REPLICA MONITOR privilege is required to execute following commands.
> +# SHOW SLAVE STATUS
> +# SHOW RELAYLOG EVENTS
> +#
> +# ==== Implementation ====
> +#
> +# Step1: GRANT ALL privileges for a new user 'user1' and then REVOKE REPLICA
> +# MONITOR and SUPER privileges.
> +# Step2: Execute SHOW SLAVE STAUTS/SHOW RELAYLOG commands and expect
> +# ER_SPECIFIC_ACCESS_DENIED_ERROR. This also verifies that REPLICATION
> +# SLAVE ADMIN privilege is not required for these two commands.
> +# Step3: GRANT REPLICA MONITOR privilege and observe that both commands are
> +# allowd to execute.
> +# Step4: GRANT SUPER privilege and observe that only SHOW SLAVE STATUS
> command
> +# is allowed.
why do you need a full master-slave replication setup for that?
> +# ==== References ====
> +#
> +# MDEV-23610: Slave user can't run "SHOW SLAVE STATUS" anymore after upgrade
> +# to 10.5, mysql_upgrade should take of that
> +# MDEV-23918: admin privlege required to view contents of relay logs in 10.5
> +#
> +
> +--source include/not_embedded.inc
> +--source include/have_binlog_format_mixed.inc
> +--source include/master-slave.inc
> +
> +--connection master
> +CREATE USER user1@localhost IDENTIFIED BY '';
> +GRANT ALL PRIVILEGES ON *.* TO user1@localhost;
> +REVOKE REPLICA MONITOR, SUPER ON *.* FROM user1@localhost;
> +FLUSH PRIVILEGES;
> +--sync_slave_with_master
> +--connect(con1,127.0.0.1,user1,,test,$SLAVE_MYPORT)
> +--connection con1
> +SELECT CURRENT_USER();
> +SHOW GRANTS;
> +--echo "Verify that having REPLICATION SLAVE ADMIN doesn't allow SHOW SLAVE
> STATUS"
> +--echo "Expected error:Access denied; you need (at least one of) the SUPER,
> REPLICA MONITOR privilege(s) for this operation"
better use not quotes, but #, like elsewhere.
> +--error ER_SPECIFIC_ACCESS_DENIED_ERROR
> +SHOW SLAVE STATUS;
> +--echo "Verify that having REPLICATION SLAVE ADMIN doesn't allow SHOW
> RELAYLOG EVENTS"
> +--echo "Expected error: Access denied; you need (at least one of) the
> REPLICA MONITOR privilege(s) for this operation"
> +--disable_ps_protocol
why?
> +--error ER_SPECIFIC_ACCESS_DENIED_ERROR
> +SHOW RELAYLOG EVENTS;
> +--enable_ps_protocol
> +--disconnect con1
> +
> diff --git a/sql/privilege.h b/sql/privilege.h
> index 37cdf4da01ad..e7686a206b84 100644
> --- a/sql/privilege.h
> +++ b/sql/privilege.h
> @@ -115,8 +117,12 @@ constexpr privilege_t ALL_KNOWN_ACL_100304 =
> constexpr privilege_t ALL_KNOWN_ACL_100502=
> (privilege_t) ((LAST_100502_ACL << 1) - 1);
>
> +// A combination of all bits defined in 10.5.8
> +constexpr privilege_t ALL_KNOWN_ACL_100508=
> + (privilege_t) ((LAST_100508_ACL << 1) - 1);
> +
> // A combination of all bits defined as of the current version
> -constexpr privilege_t ALL_KNOWN_ACL= ALL_KNOWN_ACL_100502;
> +constexpr privilege_t ALL_KNOWN_ACL= ALL_KNOWN_ACL_100508;
may be
(privilege_t) ((LAST_CURRENT_ACL << 1) - 1)
and then we won't need to change it anymore?
Also, I think this deserves a function, like
static inline privilege_t ALL_ACLS_TO(privilege_t x) { return (x << 1) - 1; }
or, rather, a bit safer
return x | (x - 1)
> // Unary operators
> @@ -505,9 +511,11 @@ constexpr privilege_t PRIV_STMT_STOP_SLAVE=
> REPL_SLAVE_ADMIN_ACL | SUPER_ACL;
> // Was SUPER_ACL prior to 10.5.2
> constexpr privilege_t PRIV_STMT_CHANGE_MASTER= REPL_SLAVE_ADMIN_ACL |
> SUPER_ACL;
> // Was (SUPER_ACL | REPL_CLIENT_ACL) prior to 10.5.2
> -constexpr privilege_t PRIV_STMT_SHOW_SLAVE_STATUS= REPL_SLAVE_ADMIN_ACL |
> SUPER_ACL;
> +// Was REPL_SLAVE_ADMIN_ACL from 10.5.2 to 10.5.7
// Was SUPER_ACL | REPL_SLAVE_ADMIN_ACL from 10.5.2 to 10.5.7
otherwise it's unclear why you have SUPER_ACL below and not just
REPLICA_MONITOR_ACL
> +constexpr privilege_t PRIV_STMT_SHOW_SLAVE_STATUS= REPLICA_MONITOR_ACL |
> SUPER_ACL;
> // Was REPL_SLAVE_ACL prior to 10.5.2
> -constexpr privilege_t PRIV_STMT_SHOW_RELAYLOG_EVENTS= REPL_SLAVE_ADMIN_ACL;
> +// Was REPL_SLAVE_ADMIN_ACL from 10.5.2 to 10.5.7
> +constexpr privilege_t PRIV_STMT_SHOW_RELAYLOG_EVENTS= REPLICA_MONITOR_ACL;
>
> /*
> Privileges related to binlog replying.
> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> index ef96e8f24843..8a3551c14f53 100644
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -1057,6 +1057,9 @@ class User_table_tabular: public User_table
> if (access & REPL_SLAVE_ACL)
> access|= REPL_MASTER_ADMIN_ACL;
>
> + if (access & FILE_ACL)
> + access|= REPLICA_MONITOR_ACL;
Why is that?
> +
> return access & GLOBAL_ACLS;
> }
>
> @@ -1541,19 +1548,22 @@ class User_table_json: public User_table
> */
> if (access & SUPER_ACL)
> {
> - if (access & REPL_SLAVE_ACL)
> - {
> - /*
> - The user could do both before the upgrade:
> - - set global variables (because of SUPER_ACL)
> - - execute "SHOW SLAVE HOSTS" (because of REPL_SLAVE_ACL)
> - Grant all new privileges that were splitted from SUPER (in
> 10.5.2),
> - and REPLICATION MASTER ADMIN, so it still can do "SHOW SLAVE
> HOSTS".
> - */
> - access|= REPL_MASTER_ADMIN_ACL;
> - }
> access|= GLOBAL_SUPER_ADDED_SINCE_USER_TABLE_ACLS;
> }
> + if (access & REPL_SLAVE_ACL)
> + {
> + /*
> + The user could do both before the upgrade:
> + - set global variables (because of SUPER_ACL)
this looked fine where it was before, but not anymore after
you moved it out of `if (access & SUPER_ACL)`
> + - execute "SHOW SLAVE HOSTS" (because of REPL_SLAVE_ACL)
> + Grant all new privileges that were splitted from SUPER (in
> 10.5.2),
> + and REPLICATION MASTER ADMIN, so it still can do "SHOW SLAVE
> HOSTS".
> + - execute "SHOW RELAYLOG EVENTS" (because of REPL_SLAVE_ACL)
> + */
> + access|= (REPL_MASTER_ADMIN_ACL | REPLICA_MONITOR_ACL);
and REPL_MASTER_ADMIN_ACL allows to modify global variables was was
earlier possible only with SUPER_ACL.
So, indeed, old code was correct, that if() should be inside SUPER_ACL,
one should only get REPL_MASTER_ADMIN_ACL if one had both SUPER_ACL and
REPL_SLAVE_ACL.
> + }
> + if (access & BINLOG_MONITOR_ACL)
> + access|= REPLICA_MONITOR_ACL;
> }
>
> if (orig_access & ~mask)
> @@ -8974,7 +8984,7 @@ static const char *command_array[]=
> "CREATE USER", "EVENT", "TRIGGER", "CREATE TABLESPACE", "DELETE HISTORY",
> "SET USER", "FEDERATED ADMIN", "CONNECTION ADMIN", "READ_ONLY ADMIN",
> "REPLICATION SLAVE ADMIN", "REPLICATION MASTER ADMIN", "BINLOG ADMIN",
> - "BINLOG REPLAY"
> + "BINLOG REPLAY", "REPLICA MONITOR"
SLAVE MONITOR
> };
>
> static uint command_lengths[]=
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
