Hello Serg, Thank you for the review comments. Please find my replies inline. On 12/11/20 7:36 pm, Sergei Golubchik wrote:

Hi, Sujatha!

On Nov 12, Sujatha wrote:

...

revision-id: d66d5457589d (mariadb-10.5.4-315-gd66d5457589d) parent(s): 10b2d5726fa2 author: Sujatha committer: Sujatha 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.

Sure.

...

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.

Yes, both REPLICA MONITOR/ SLAVE MONITOR will work. Since we wanted to stop the usage of term 'SLAVE' I used 'REPLICA' 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?

It can be added in main suite as well. Since these are replication specific privileges, added them as part of 'rpl' suite.

...

+# ==== 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.

Sure.

...

+--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?

I observed following failure during buildbot testing http://buildbot.askmonty.org/buildbot/builders/kvm-fulltest/builds/26635/ste... rpl.rpl_replica_monitor_priv 'mix' w1 [ fail ] Test ended at 2020-11-10 11:22:28 CURRENT_TEST: rpl.rpl_replica_monitor_priv mysqltest: At line 46: query 'SHOW RELAYLOG EVENTS' failed with wrong errno 1295: 'This command is not supported in the prepared statement protocol yet', instead of 1227... To address this issue I disabled the command.

...

+--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)

Sure.

...

// 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

Sure.

...

+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?

Please find the reason mentioned as part of the following comment. https://jira.mariadb.org/browse/MDEV-23610?focusedCommentId=171800&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-171800

...

+ 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.

I will try to rewrite this part such that SUPER_ACL / REPL_CLIENT_ACL - SLAVE_MONITOR_ACL REPL_SLAVE_ACL - SLAVE_MONITOR_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

Sure will change. Thank you S.Sujatha

...

};

static uint command_lengths[]= Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org