Bug#30600321 MAX_QUERIES_PER_HOUR 0 ONLY REMOVES THE LIMIT ON 1 MYSQLD

This patch addresses two issues. First, all ALTER USER statements
were distributed as snapshots, merely because they might contain
a plaintext password, when in fact many of them could have used
the preferred method of statement distribution. To fix this, for
the SET PASSWORD and ALTER USER statements that do in fact contain
passwords, add the query rewrite parameters (which were generated
for binlogging) to the Acl_change_notification and then re-use them
to rewrite the statement for schema distribution.

Secondly, "SHOW CREATE USER" does not include resource limits that
are set to zero. When applying a snapshot in Ndb_stored_grants,
add an ALTER USER statement to explicitly set all resource limits
to zero before applying the statement obtained from SHOW CREATE USER.

The first patch alone fixes the bug for most common cases, and
improves efficiency for distributing ALTER USER.  The second patch
alone fixes the bug for all cases, but makes snapshots less efficient.

The test case is added to test ndb.stored_grants_error_handling

Change-Id: I8517ce79906ee1be16eeddf0f48fa1d8128117d6

Author: jdduncan

mysql-test/suite/ndb/r/stored_grants_error_handling.result

@@ -2,4 +2,17 @@ CREATE USER "mcmd"@"localhost";
 GRANT ALL PRIVILEGES ON *.* to "mcmd"@"localhost";
 CALL mtr.add_suppression("NDB: Error 626, Tuple did not exist");
 GRANT ALL PRIVILEGES ON *.* to "mcmd"@"localhost";
+Expect 33
+max_questions
+33
+Expect 0
+max_questions
+0
+Expect 55
+max_questions
+55
+Expect 0
+max_questions
+0
 DROP USER mcmd@localhost;
+DROP USER lu1@a;

mysql-test/suite/ndb/t/stored_grants_error_handling.test

@@ -1,7 +1,14 @@
 --source include/have_ndb.inc
 --source suite/ndb/include/ndb_find_tools.inc
 
+#
+# Open connections
+#
+connect(mysqld2,127.0.0.1,root,,test,$MASTER_MYPORT1);
+
+#
 # Bug#30556487 MYSQLD HANG IN NDB_STORED_GRANTS CODE ON CREATE USER
+#
 CREATE USER "mcmd"@"localhost";
 GRANT ALL PRIVILEGES ON *.* to "mcmd"@"localhost";
 
@@ -13,6 +20,60 @@ GRANT ALL PRIVILEGES ON *.* to "mcmd"@"localhost";
 CALL mtr.add_suppression("NDB: Error 626, Tuple did not exist");
 GRANT ALL PRIVILEGES ON *.* to "mcmd"@"localhost";
 
+#
+# Bug#30600321 MAX_QUERIES_PER_HOUR 0 ONLY REMOVES THE LIMIT ON 1 MYSQLD
+#
+--enable_result_log
+--disable_query_log
+
+# Bug#30600321 Test (1): Alter resource limit of stored user
+ALTER USER "mcmd"@"localhost" WITH MAX_QUERIES_PER_HOUR 33;
+
+connection mysqld2;
+echo Expect 33;
+SELECT max_questions from mysql.user where user = 'mcmd';
+
+connection default;
+ALTER USER "mcmd"@"localhost" WITH MAX_QUERIES_PER_HOUR 0;
+
+connection mysqld2;
+let $max= `SELECT max_questions from mysql.user where user = 'mcmd'`;
+if ($max != 0)
+{
+  die Test failed -- max_questions was not zero;
+}
+
+# Bug#30600321 Test (2): Alter both resource limit and password of stored user
+connection default;
+ALTER USER "mcmd"@"localhost" WITH MAX_QUERIES_PER_HOUR 44;
+ALTER USER "mcmd"@"localhost" IDENTIFIED BY "Garb_farb_earb" WITH MAX_QUERIES_PER_HOUR 0;
+
+connection mysqld2;
+echo Expect 0;
+SELECT max_questions from mysql.user where user = 'mcmd';
+
+# Bug#30600321 Test (3): Alter resource limit of stored and non-stored users
+connection default;
+CREATE USER "lu1"@"a";
+ALTER USER "lu1"@"a", "mcmd"@"localhost" WITH MAX_QUERIES_PER_HOUR 55;
+
+connection mysqld2;
+echo Expect 55;
+SELECT max_questions from mysql.user where user = 'mcmd';
+
+connection default;
+ALTER USER "lu1"@"a", "mcmd"@"localhost" WITH MAX_QUERIES_PER_HOUR 0;
+
+connection mysqld2;
+echo Expect 0;
+SELECT max_questions from mysql.user where user = 'mcmd';
+
+--enable_query_log
+
+#
 # Cleanup
+#
+connection default;
 DROP USER mcmd@localhost;
+DROP USER lu1@a;
 

sql/auth/acl_change_notification.h

@@ -27,6 +27,8 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301  USA
 #include "my_sqlcommand.h"  // enum_sql_command
 #include "sql/table.h"      // LEX_USER, LEX_CSTRING, List
 
+class Rewrite_params;  // forward declaration
+
 class Acl_change_notification {
  public:
   struct User {
@@ -39,23 +41,24 @@ class Acl_change_notification {
 
   Acl_change_notification(class THD *thd, enum_sql_command op,
                           const List<LEX_USER> *users,
+                          Rewrite_params *rewrite_params,
                           const List<LEX_CSTRING> *dynamic_privs);
 
  private:
   enum_sql_command operation;
   std::string db;
-  std::string query;
   std::vector<User> user_list;
   std::vector<std::string> dynamic_privilege_list;
+  Rewrite_params *rewrite_params;
 
  public:
   enum_sql_command get_operation() const { return operation; }
   const std::string &get_db() const { return db; }
-  const std::string &get_query_for_logging() const { return query; }
   const std::vector<User> &get_user_list() const { return user_list; }
   const std::vector<std::string> &get_dynamic_privilege_list() const {
     return dynamic_privilege_list;
   }
+  Rewrite_params *get_rewrite_params() const { return rewrite_params; }
 };
 
 #endif

sql/auth/auth_internal.h

@@ -209,6 +209,7 @@ bool log_and_commit_acl_ddl(THD *thd, bool transactional_tables,
                             bool log_to_binlog = true);
 void acl_notify_htons(THD *thd, enum_sql_command operation,
                       const List<LEX_USER> *users,
+                      std::set<LEX_USER *> *rewrite_users = nullptr,
                       const List<LEX_CSTRING> *dynamic_privs = nullptr);
 
 /* sql_authorization */

sql/auth/sql_authorization.cc

@@ -3592,7 +3592,7 @@ bool mysql_grant(THD *thd, const char *db, List<LEX_USER> &list, ulong rights,
     my_ok(thd);
     /* Notify storage engines */
     acl_notify_htons(thd, revoke_grant ? SQLCOM_REVOKE : SQLCOM_GRANT, &list,
-                     granted_dynamic_privs);
+                     nullptr, granted_dynamic_privs);
   }
 
   return error;

sql/auth/sql_user.cc

@@ -1578,11 +1578,11 @@ bool change_password(THD *thd, LEX_USER *lex_user, const char *new_password,
         authentication_plugin.c_str(), is_role, nullptr, nullptr);
   } /* Critical section */
 
-  /* Notify storage engines */
+  /* Notify storage engines (including rewrite list) */
   if (!(result || commit_result)) {
     List<LEX_USER> user_list;
     user_list.push_back(lex_user);
-    acl_notify_htons(thd, SQLCOM_SET_PASSWORD, &user_list);
+    acl_notify_htons(thd, SQLCOM_SET_PASSWORD, &user_list, &users);
   }
 
   return result || commit_result;
@@ -2819,8 +2819,8 @@ bool mysql_alter_user(THD *thd, List<LEX_USER> &list, bool if_exists) {
         reset_mqh(thd, extra_user, false);
       }
     }
-    /* Notify storage engines */
-    acl_notify_htons(thd, SQLCOM_ALTER_USER, &list);
+    /* Notify storage engines (including rewrite list) */
+    acl_notify_htons(thd, SQLCOM_ALTER_USER, &list, &extra_users);
   }
 
   if (result == 0) {

sql/auth/sql_user_table.cc

@@ -558,13 +558,10 @@ ulong get_access(TABLE *form, uint fieldnr, uint *next_field) {
 */
 Acl_change_notification::Acl_change_notification(
     THD *thd, enum_sql_command op, const List<LEX_USER> *users,
-    const List<LEX_CSTRING> *dynamic_privs)
-    : operation(op), db(thd->db().str, thd->db().length) {
-  if (thd->rewritten_query().length()) {
-    query.assign(thd->rewritten_query().ptr(), thd->rewritten_query().length());
-  } else {
-    query.assign(thd->query().str, thd->query().length);
-  }
+    Rewrite_params *rewrite, const List<LEX_CSTRING> *dynamic_privs)
+    : operation(op),
+      db(thd->db().str, thd->db().length),
+      rewrite_params(rewrite) {
   if (users) {
     /* Copy data out of List<LEX_USER> */
     user_list.reserve(users->size());
@@ -581,11 +578,12 @@ Acl_change_notification::Acl_change_notification(
   }
 }
 
-void acl_notify_htons(THD *thd MY_ATTRIBUTE((unused)),
-                      enum_sql_command operation MY_ATTRIBUTE((unused)),
-                      const List<LEX_USER> *users MY_ATTRIBUTE((unused)),
-                      const List<LEX_CSTRING> *dynamic_privs
-                          MY_ATTRIBUTE((unused))) {
+void acl_notify_htons(
+    THD *thd MY_ATTRIBUTE((unused)),
+    enum_sql_command operation MY_ATTRIBUTE((unused)),
+    const List<LEX_USER> *users MY_ATTRIBUTE((unused)),
+    std::set<LEX_USER *> *rewrite_users MY_ATTRIBUTE((unused)),
+    const List<LEX_CSTRING> *dynamic_privs MY_ATTRIBUTE((unused))) {
   DBUG_TRACE;
   DBUG_PRINT("enter", ("db: %s query: '%s'", thd->db().str, thd->query().str));
 #ifdef WITH_NDBCLUSTER_STORAGE_ENGINE
@@ -594,7 +592,9 @@ void acl_notify_htons(THD *thd MY_ATTRIBUTE((unused)),
     So, instantiate it and send a notification only if the Server is
     built with ndbcluster SE.
   */
-  Acl_change_notification notice(thd, operation, users, dynamic_privs);
+  User_params rewrite_user_params(rewrite_users);
+  User_params *rewrite = rewrite_users ? &rewrite_user_params : nullptr;
+  Acl_change_notification notice(thd, operation, users, rewrite, dynamic_privs);
   ha_acl_notify(thd, &notice);
 #endif
 }

storage/ndb/plugin/ha_ndbcluster_binlog.cc

@@ -40,6 +40,7 @@
 #include "sql/rpl_injector.h"
 #include "sql/rpl_slave.h"
 #include "sql/sql_lex.h"
+#include "sql/sql_rewrite.h"
 #include "sql/sql_table.h"  // build_table_filename
 #include "sql/sql_thd_internal_api.h"
 #include "sql/thd_raii.h"
@@ -596,8 +597,17 @@ static void ndbcluster_acl_notify(THD *thd,
     return;
   }
 
-  const std::string &query = notice->get_query_for_logging();
+  /* Obtain the query in a form suitable for writing to the error log.
+     The password is replaced with the string "<secret>".
+  */
+  std::string query;
+  if (thd->rewritten_query().length())
+    query.assign(thd->rewritten_query().ptr(), thd->rewritten_query().length());
+  else
+    query.assign(thd->query().str, thd->query().length);
+  DBUG_ASSERT(query.length());
   ndb_log_verbose(9, "ACL considering: %s", query.c_str());
+
   std::string user_list;
   bool dist_use_db = false;   // Prepend "use [db];" to statement
   bool dist_refresh = false;  // All participants must refresh their caches
@@ -632,6 +642,19 @@ static void ndbcluster_acl_notify(THD *thd,
 
   DBUG_ASSERT(strategy == Ndb_stored_grants::Strategy::STATEMENT);
   ndb_log_verbose(9, "ACL change distribution: STATEMENT");
+
+  /* If the notice contains rewrite_params, query is an ALTER USER or SET
+     PASSWORD statement and must be rewritten again, as if for the binlog,
+     replacing a plaintext password with a crytpographic hash.
+  */
+  if (notice->get_rewrite_params()) {
+    String rewritten_query;
+    mysql_rewrite_acl_query(thd, rewritten_query, Consumer_type::BINLOG,
+                            notice->get_rewrite_params(), false);
+    query.assign(rewritten_query.c_ptr_safe(), rewritten_query.length());
+    DBUG_ASSERT(query.length());
+  }
+
   if (!schema_dist_client.acl_notify(
           dist_use_db ? notice->get_db().c_str() : nullptr, query.c_str(),
           query.length(), dist_refresh))

storage/ndb/plugin/ndb_stored_grants.cc

@@ -617,16 +617,17 @@ int ThreadContext::drop_users(ChangeNotice *notice,
    from SHOW CREATE USER, so its exact format is known. For idempotence, it
    must be rewritten as several statements. The final result is:
       CREATE USER IF NOT EXISTS user@host;
-      ALTER USER user@host ... ;
       REVOKE ALL ON *.* FROM user@host;
-      GRANT ... TO user@host;
-      GRANT ... TO user@host;
-      ALTER USER user@host DEFAULT ROLE r;
+      ALTER USER user@host ...;   [CLEAR RESOURCE LIMITS]
+      ALTER USER user@host ...;   [SET VALUES FROM SHOW CREATE USER]
 */
 void ThreadContext::create_user(std::string &name, std::string &statement) {
   const std::string create_user("CREATE USER IF NOT EXISTS ");
   const std::string alter_user("ALTER USER ");
   const std::string revoke_all("REVOKE ALL ON *.* FROM ");
+  const std::string set_resource_defaults(
+      " WITH MAX_QUERIES_PER_HOUR 0 MAX_UPDATES_PER_HOUR 0 "
+      " MAX_CONNECTIONS_PER_HOUR 0 MAX_USER_CONNECTIONS 0");
 
   /* Run statement CREATE USER IF NOT EXISTS */
   if (!get_local_user(name)) {
@@ -635,6 +636,12 @@ void ThreadContext::create_user(std::string &name, std::string &statement) {
     run_acl_statement(create_user + name);
   }
 
+  /* Revoke any privileges the user may have had prior to this snapshot. */
+  run_acl_statement(revoke_all + name);
+
+  /* Clear resource limits (this is not included in SHOW CREATE USER) */
+  run_acl_statement(alter_user + name + set_resource_defaults);
+
   /* Rewrite CREATE to ALTER */
   statement = statement.replace(0, 6, "ALTER");
 
@@ -658,9 +665,6 @@ void ThreadContext::create_user(std::string &name, std::string &statement) {
 
   /* Run the rest of the statement. */
   run_acl_statement(statement.erase(default_role_pos, role_clause_len));
-
-  /* Revoke any privileges the user may have had prior to this snapshot. */
-  run_acl_statement(revoke_all + name);
 }
 
 /* Apply the snapshot in m_current_rows,
@@ -849,10 +853,6 @@ Ndb_stored_grants::Strategy ThreadContext::handle_change(ChangeNotice *notice) {
     /* ALTER USER, SET PASSWORD, or GRANT or REVOKE of misc. privileges */
     rebuild_local_cache = false;
     update_list = &m_intersection;
-    /* Distribute ALTER USER and SET PASSWORD as snapshot refreshes
-       in order to avoid transmitting plaintext passwords. */
-    if (operation == SQLCOM_ALTER_USER || operation == SQLCOM_SET_PASSWORD)
-      dist_as_snapshot = true;
   }
 
   /* drop_users() will DROP USER or REVOKE NDB_STORED_USER, as appropriate */