maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sujatha!

On Nov 12, Sujatha wrote:
> revision-id: d66d5457589d (mariadb-10.5.4-315-gd66d5457589d)
> parent(s): 10b2d5726fa2
> author: Sujatha <sujatha.sivakumar@xxxxxxxxxxx>
> committer: Sujatha <sujatha.sivakumar@xxxxxxxxxxx>
> 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 security@xxxxxxxxxxx