maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sujatha!

Looks good, thanks!

One detail to fix - I think you've mistakenly committed an old test from
the previous version of the patch.

And two minor suggestions, you can change or not, as you like.

otherwise ok to push

On Nov 13, Sujatha wrote:
> revision-id: 255e5f269d80 (mariadb-10.5.4-319-g255e5f269d80)
> parent(s): 97569d3c37a6
> author: Sujatha <sujatha.sivakumar@xxxxxxxxxxx>
> committer: Sujatha <sujatha.sivakumar@xxxxxxxxxxx>
> timestamp: 2020-11-13 19:08:02 +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 "SLAVE MONITOR" which will grant user the permission
> to execute "SHOW SLAVE STATUS" and "SHOW RELAYLOG EVENTS" commands.
> 
> SHOW SLAVE STATUS requires either SLAVE MONITOR/SUPER
> SHOW RELAYLOG EVENTS requires SLAVE MONITOR privilege.
> 
> diff --git a/mysql-test/main/grant_slave_monitor.test b/mysql-test/main/grant_slave_monitor.test
> new file mode 100644
> index 000000000000..a4f21085e537
> --- /dev/null
> +++ b/mysql-test/main/grant_slave_monitor.test
> @@ -0,0 +1,101 @@
> +# ==== Purpose ====
> +#
> +# SLAVE 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 
> +#        SLAVE MONITOR and SUPER privileges.       
> +# Step2: Execute SHOW SLAVE STAUTS/SHOW RELAYLOG EVENTS 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 SLAVE 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.
> +#
> +# ==== 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
> +
> +CREATE USER user1@localhost IDENTIFIED BY '';
> +GRANT ALL PRIVILEGES ON *.* TO user1@localhost;
> +REVOKE SLAVE MONITOR, SUPER ON *.* FROM user1@localhost;
> +FLUSH PRIVILEGES;
> +
> +--connect(con1,localhost,user1,,)
> +--connection con1
> +SHOW GRANTS;
> +
> +--echo #
> +--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, SLAVE
> +--echo #                 MONITOR privilege(s) for this operation
> +--echo #
> +--error ER_SPECIFIC_ACCESS_DENIED_ERROR
> +SHOW SLAVE STATUS;
> +
> +--echo #
> +--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
> +--echo #                 privilege(s) for this operation
> +--echo #
> +--disable_ps_protocol
> +--error ER_SPECIFIC_ACCESS_DENIED_ERROR
> +SHOW RELAYLOG EVENTS;
> +--enable_ps_protocol
> +--disconnect con1
> +
> +--echo # 
> +--echo # SHOW SLAVE STATUS and SHOW RELAYLOG EVENTS are allowed with SLAVE MONITOR privilege
> +--echo #
> +
> +--connection default
> +GRANT SLAVE MONITOR ON *.* TO user1@localhost;
> +FLUSH PRIVILEGES;
> +
> +--connect(con1,localhost,user1,,)
> +--connection con1
> +SHOW GRANTS;
> +
> +--disable_result_log
> +SHOW SLAVE STATUS;
> +#--disable_ps_protocol
> +SHOW RELAYLOG EVENTS;
> +#--enable_ps_protocol
> +--enable_result_log
> +--disconnect con1
> +
> +--connection default
> +DROP USER user1@localhost;
> +
> +--echo #
> +--echo # SHOW SLAVE STATUS command is allowed with SUPER privilege
> +--echo #
> +CREATE USER user1@localhost IDENTIFIED BY '';
> +GRANT SUPER ON *.* TO user1@localhost;
> +
> +--connect(con1,localhost,user1,,)
> +--disable_result_log
> +SHOW SLAVE STATUS;
> +--enable_result_log
> +
> +--echo #
> +--echo # SHOW RELAYLOG EVENTS is not allowed with SUPER privilege, it requires SLAVE MONITOR
> +--echo #
> +
> +--disable_ps_protocol
> +--error ER_SPECIFIC_ACCESS_DENIED_ERROR
> +SHOW RELAYLOG EVENTS;
> +--enable_ps_protocol
> +--disconnect con1
> +
> +--connection default
> +DROP USER user1@localhost;
> diff --git a/mysql-test/main/rpl_replica_monitor_priv.test b/mysql-test/main/rpl_replica_monitor_priv.test
> --- /dev/null
> +++ b/mysql-test/main/rpl_replica_monitor_priv.test
> @@ -0,0 +1,90 @@

Did you check in this file by mistake?

It's a replication test (source include/master-slave.inc) but not in the
rpl suite. It tests the feature that does not need master-slave.inc.
And it's duplicated by grant_slave_monitor.test

> diff --git a/sql/privilege.h b/sql/privilege.h
> index 37cdf4da01ad..d0e6e532b4b5 100644
> --- a/sql/privilege.h
> +++ b/sql/privilege.h
> @@ -93,13 +94,18 @@ enum privilege_t: unsigned long long
>    */
>  };
>  
> +constexpr static inline privilege_t ALL_KNOWN_BITS(privilege_t x)
> +{
> +  return (privilege_t)((x << 1) - 1);

Generally it's safer to write

  x | (x - 1)

it won't overflow if x is already the highest possible bit.

In privileges we're quite far from it at the moment, so it's not a big
deal, just a style issue and setting a good example to others.

> +}
>  
>  // Version markers
>  constexpr privilege_t LAST_100304_ACL= DELETE_HISTORY_ACL;
>  constexpr privilege_t LAST_100502_ACL= BINLOG_REPLAY_ACL;
> +constexpr privilege_t LAST_100509_ACL= SLAVE_MONITOR_ACL;
>  
> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> index ef96e8f24843..b5b1472f004f 100644
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -1015,7 +1015,7 @@ class User_table_tabular: public User_table
>        access|= LOCK_TABLES_ACL | CREATE_TMP_ACL | SHOW_DB_ACL;
>        if (access & FILE_ACL)
>          access|= BINLOG_MONITOR_ACL | REPL_SLAVE_ACL | BINLOG_ADMIN_ACL |
> -                 BINLOG_REPLAY_ACL;
> +                 BINLOG_REPLAY_ACL | SLAVE_MONITOR_ACL;

this does not hurt, but it's rather redundant, you set SLAVE_MONITOR_ACL
below anyway.

>        if (access & PROCESS_ACL)
>          access|= SUPER_ACL | EXECUTE_ACL;
>      }
> @@ -1057,6 +1057,9 @@ class User_table_tabular: public User_table
>      if (access & REPL_SLAVE_ACL)
>        access|= REPL_MASTER_ADMIN_ACL;
>  
> +    if (access & REPL_SLAVE_ACL)
> +      access|= SLAVE_MONITOR_ACL;
> +
>      return access & GLOBAL_ACLS;
>    }
>  
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx