maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sujatha!

On Nov 12, sujatha wrote:
> >> 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.

Right, I know. I meant that it's fine (and even good) to use
GRANT REPLICA MONITOR in test files, no matter how it's shown
in SHOW GRANTS.

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

I think it's better to have it in the main suite.

rpl suite is for testing replication features that need a working
replication topology set up.

For example binlog specific tests that do not need working replication
go into the binlog suite, not in rpl.

> >> +--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/steps/mtr_ps/logs/stdio
> 
> 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.

This is a bug.

It is assumed that mysqltest will only use ps protocol for statements
that support it. It has a big regex to detect that. Alas, it only
has

      "|[[:space:]]*SHOW[[:space:]]|"

which matches all SHOW commands, and it'd be rather annoying and fragile
to list all SHOW variants excluding just RELAYLOG (and BINLOG) EVENTS.

On the other hand, it seems really easy to make them to support ps
protocol. Perhaps it only needs something like (untested):

===============================
diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
--- a/sql/sql_prepare.cc
+++ b/sql/sql_prepare.cc
@@ -2369,6 +2369,14 @@ static bool check_prepared_statement(Prepared_statement >
       DBUG_RETURN(FALSE);
     }
     break;
+  case SQLCOM_SHOW_BINLOG_EVENTS:
+  case SQLCOM_SHOW_RELAYLOG_EVENTS:
+    {
+      List<Item> field_list;
+      Log_event::init_show_field_list(thd, &field_list);
+      if ((res= send_stmt_metadata(thd, stmt, &field_list)) == 2)
+        DBUG_RETURN(FALSE);
+    }
 #endif /* EMBEDDED_LIBRARY */
   case SQLCOM_SHOW_CREATE_PROC:
     if ((res= mysql_test_show_create_routine(stmt, &sp_handler_procedure)) == >
===============================

Could you try that, please? If it'll work, better to commit it
separately, though.

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

Yes, I've seen it already, but I still don't understand
what FILE_ACL has to do with REPLICA_MONITOR_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

Exactly. Thanks!

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx