diff --git a/mysql-test/suite/galera/r/pxc_create_user_auth_policy.result b/mysql-test/suite/galera/r/pxc_create_user_auth_policy.result new file mode 100644 index 000000000000..1e11021aaad3 --- /dev/null +++ b/mysql-test/suite/galera/r/pxc_create_user_auth_policy.result @@ -0,0 +1,16 @@ +CALL mtr.add_suppression("Error 'Plugin 'nonexistent_plugin' is not loaded' on query."); +CALL mtr.add_suppression("Error 'Plugin 'nonexistent_plugin' is not loaded' on query."); +CREATE USER 'testuser'@'%' IDENTIFIED BY 'pass'; +GRANT ALL ON *.* TO testuser; +REVOKE AUTHENTICATION_POLICY_ADMIN ON *.* FROM 'testuser'; +CREATE USER 'new_user_1'@'%' IDENTIFIED BY 'pass'; +CREATE USER 'new_user_2'@'%' IDENTIFIED BY 'pass', 'new_user_3'@'%' IDENTIFIED WITH mysql_native_password BY 'pass'; +ERROR HY000: 1 factor authentication method does not match against authentication policy. Please refer @@authentication_policy system variable. +ALTER USER 'new_user_1'@'%' IDENTIFIED WITH mysql_native_password BY 'newpass'; +ERROR HY000: 1 factor authentication method does not match against authentication policy. Please refer @@authentication_policy system variable. +CREATE USER 'new_user_4'@'%' IDENTIFIED WITH 'nonexistent_plugin' BY 'pass'; +ERROR HY000: Plugin 'nonexistent_plugin' is not loaded +CREATE USER 'new_user_4'@'%' IDENTIFIED WITH 'nonexistent_plugin' BY 'pass'; +ERROR HY000: Plugin 'nonexistent_plugin' is not loaded +DROP USER 'new_user_1'@'%'; +DROP USER 'testuser'@'%'; diff --git a/mysql-test/suite/galera/t/pxc_create_user_auth_policy.cnf b/mysql-test/suite/galera/t/pxc_create_user_auth_policy.cnf new file mode 100644 index 000000000000..f230eea909ec --- /dev/null +++ b/mysql-test/suite/galera/t/pxc_create_user_auth_policy.cnf @@ -0,0 +1,7 @@ +!include ../galera_2nodes.cnf + +[mysqld.1] +authentication_policy=caching_sha2_password,, + +[mysqld.2] +authentication_policy=caching_sha2_password,, diff --git a/mysql-test/suite/galera/t/pxc_create_user_auth_policy.test b/mysql-test/suite/galera/t/pxc_create_user_auth_policy.test new file mode 100644 index 000000000000..8ae001e748de --- /dev/null +++ b/mysql-test/suite/galera/t/pxc_create_user_auth_policy.test @@ -0,0 +1,56 @@ +# +# Test that CREATE/ALTER USER statement that fails on source node +# is not replicated. +# If it was replicated, it would be executed from wsrep_applier thread context +# on replica node. It would not fail there, because wsrep_applier works in root +# user context. As the result, inconsistency voting protocol would detect +# inconsistency between nodes and evict one node. +# + +--source include/galera_cluster.inc +--source include/count_sessions.inc + +--connection node_1 +CALL mtr.add_suppression("Error 'Plugin 'nonexistent_plugin' is not loaded' on query."); +--connection node_2 +CALL mtr.add_suppression("Error 'Plugin 'nonexistent_plugin' is not loaded' on query."); + +# Create the user we will use during the test +CREATE USER 'testuser'@'%' IDENTIFIED BY 'pass'; +GRANT ALL ON *.* TO testuser; + +# revoke AUTHENTICATION_POLICY_ADMIN, so CREATE/ALTER USER that mismatch +# authentication_policy will cause error (not only warning) +REVOKE AUTHENTICATION_POLICY_ADMIN ON *.* FROM 'testuser'; + +# Create the user we will try to ALTER +CREATE USER 'new_user_1'@'%' IDENTIFIED BY 'pass'; + +--connect(con_node_1_testuser, localhost, testuser, pass,,$NODE_MYPORT_1) + +# Because of authentication_policy=caching_sha2_password,, +# creation of new_user_3 is not allowed, so the following statement is expected +# to fail +--error ER_AUTHENTICATION_POLICY_MISMATCH +CREATE USER 'new_user_2'@'%' IDENTIFIED BY 'pass', 'new_user_3'@'%' IDENTIFIED WITH mysql_native_password BY 'pass'; + +# Because of authentication_policy=caching_sha2_password,, +# altering new_user_1 to use mysql_native_password is expected to fail +--error ER_AUTHENTICATION_POLICY_MISMATCH +ALTER USER 'new_user_1'@'%' IDENTIFIED WITH mysql_native_password BY 'newpass'; + +# +# Try with nonexistent auth plugin +# +--error ER_PLUGIN_IS_NOT_LOADED +CREATE USER 'new_user_4'@'%' IDENTIFIED WITH 'nonexistent_plugin' BY 'pass'; +--connection node_1 +--error ER_PLUGIN_IS_NOT_LOADED +CREATE USER 'new_user_4'@'%' IDENTIFIED WITH 'nonexistent_plugin' BY 'pass'; + +# cleanup +--disconnect con_node_1_testuser +--connection node_1 +DROP USER 'new_user_1'@'%'; +DROP USER 'testuser'@'%'; +--source include/wait_until_count_sessions.inc diff --git a/sql/auth/auth_internal.h b/sql/auth/auth_internal.h index 90d7ea3af007..3dce9d816fd3 100644 --- a/sql/auth/auth_internal.h +++ b/sql/auth/auth_internal.h @@ -215,7 +215,7 @@ int replace_routine_table(THD *thd, GRANT_NAME *grant_name, TABLE *table, Access_bitmask rights, bool revoke_grant); #ifdef WITH_WSREP int open_grant_tables(THD *thd, Table_ref *tables, bool *transactional_tables, - const char *db = WSREP_MYSQL_DB, + bool block_toi = false, const char *db = WSREP_MYSQL_DB, const char *table = nullptr); #else int open_grant_tables(THD *thd, Table_ref *tables, bool *transactional_tables); @@ -322,11 +322,20 @@ Auth_id_ref create_authid_from(const LEX_CSTRING &user, const LEX_CSTRING &host); bool roles_rename_authid(THD *thd, TABLE *edge_table, TABLE *defaults_table, LEX_USER *user_from, LEX_USER *user_to); +#ifdef WITH_WSREP +bool set_and_validate_user_attributes( + THD *thd, LEX_USER *Str, acl_table::Pod_user_what_to_update &what_to_set, + bool is_privileged_user, bool is_role, Table_ref *history_table, + bool *history_check_done, const char *cmd, Userhostpassword_list &, + I_multi_factor_auth **mfa = nullptr, bool if_not_exists = false, + bool verify_passwd_history = true); +#else bool set_and_validate_user_attributes( THD *thd, LEX_USER *Str, acl_table::Pod_user_what_to_update &what_to_set, bool is_privileged_user, bool is_role, Table_ref *history_table, bool *history_check_done, const char *cmd, Userhostpassword_list &, I_multi_factor_auth **mfa = nullptr, bool if_not_exists = false); +#endif /* WITH_WSREP */ typedef std::pair Grant_privilege; typedef std::unordered_multimap User_to_dynamic_privileges_map; diff --git a/sql/auth/sql_user.cc b/sql/auth/sql_user.cc index 3f39c654eb1f..c503172ce9ef 100644 --- a/sql/auth/sql_user.cc +++ b/sql/auth/sql_user.cc @@ -112,6 +112,8 @@ #include // RAND_bytes #ifdef WITH_WSREP +#include "sql/error_handler.h" +#include "sql/raii/sentry.h" #include "sql/wsrep_trans_observer.h" #endif /* WITH_WSREP */ @@ -1295,13 +1297,21 @@ static bool check_for_authentication_policy(THD *thd, LEX_USER *user_name, @retval 0 ok @retval 1 ERROR; */ - +#ifdef WITH_WSREP +bool set_and_validate_user_attributes( + THD *thd, LEX_USER *Str, acl_table::Pod_user_what_to_update &what_to_set, + bool is_privileged_user, bool is_role, Table_ref *history_table, + bool *history_check_done, const char *cmd, + Userhostpassword_list &generated_passwords, I_multi_factor_auth **i_mfa, + bool if_not_exists, bool verify_passwd_history) { +#else bool set_and_validate_user_attributes( THD *thd, LEX_USER *Str, acl_table::Pod_user_what_to_update &what_to_set, bool is_privileged_user, bool is_role, Table_ref *history_table, bool *history_check_done, const char *cmd, Userhostpassword_list &generated_passwords, I_multi_factor_auth **i_mfa, bool if_not_exists) { +#endif /* WITH_WSREP */ bool user_exists = false; ACL_USER *acl_user; plugin_ref plugin = nullptr; @@ -1768,6 +1778,16 @@ bool set_and_validate_user_attributes( std::string(Str->host.str), gen_password, 1}; generated_passwords.push_back(p); } +#ifdef WITH_WSREP + if (auth->generate_authentication_string(outbuf, &buflen, inbuf, + inbuflen) || + (verify_passwd_history && + auth_verify_password_history(thd, &Str->user, &Str->host, + Str->alter_status.password_history_length, + Str->alter_status.password_reuse_interval, + auth, inbuf, inbuflen, outbuf, buflen, + history_table, what_to_set.m_what))) { +#else if (auth->generate_authentication_string(outbuf, &buflen, inbuf, inbuflen) || auth_verify_password_history(thd, &Str->user, &Str->host, @@ -1775,6 +1795,8 @@ bool set_and_validate_user_attributes( Str->alter_status.password_reuse_interval, auth, inbuf, inbuflen, outbuf, buflen, history_table, what_to_set.m_what)) { + +#endif plugin_unlock(nullptr, plugin); what_to_set.m_what = NONE_ATTR; /* @@ -1852,7 +1874,12 @@ bool set_and_validate_user_attributes( Covers replication scenario too since the IDENTIFIED BY will get rewritten to IDENTIFIED ... WITH ... AS */ +#ifdef WITH_WSREP + if (verify_passwd_history && + auth_verify_password_history( +#else if (auth_verify_password_history( +#endif thd, &Str->user, &Str->host, Str->alter_status.password_history_length, Str->alter_status.password_reuse_interval, auth, nullptr, 0, @@ -2144,7 +2171,7 @@ bool change_password(THD *thd, LEX_USER *lex_user, const char *new_password, buff[query_length_max - 1] = 0; thd->set_query(buff, query_length); - if ((ret = open_grant_tables(thd, tables, &transactional_tables, + if ((ret = open_grant_tables(thd, tables, &transactional_tables, false, WSREP_MYSQL_DB, "user"))) { thd->set_query(query_save); return (ret != 1); @@ -2864,6 +2891,116 @@ static bool check_orphaned_definers(THD *thd, List &list) { return false; } +#ifdef WITH_WSREP +static LEX_USER *deep_copy(THD *thd, LEX_USER *src) { + LEX_USER *dest = LEX_USER::alloc(thd); + assert(dest); + *dest = *src; + dest->user = LexStringDupRootUnlessEmpty(thd->mem_root, src->user); + dest->host = LexStringDupRootUnlessEmpty(thd->mem_root, src->host); + dest->current_auth = + LexStringDupRootUnlessEmpty(thd->mem_root, src->current_auth); + dest->first_factor_auth_info.auth = LexStringDupRootUnlessEmpty( + thd->mem_root, src->first_factor_auth_info.auth); + return dest; +} + +/* + Q: Why for CREATE/ALTER USER we don't stop the execution and return with error + collected there but continue the normal flow and skip TOI? + A: TOI is replicated before local execution. On replica side the execution is + from wsrep_applier (root) context, so it will succeed. But locally it may fail + which will trigger inconsistency voting. That's why we need to check the + execution against authentication policy before TOI. + + CREATE/ALTER USER, does several checks under MDL lock, in particular: + 1. check if auth plugin is installed. If not => ER_PLUGIN_IS_NOT_LOADED + 2. check if authentication_policy allows CREATE/ALTER. In this case the result + depends on grants of executing user. We may end up with + ER_AUTHENTICATION_POLICY_MISMATCH which is error or warning. In case of warning + we need to continue, in case of error - do not replicate. + + Inconsistency voting is based on all collected warnings and errors (but is + triggered only if there are any errors). + If we collected everything we would end with: + 1. Source: ER_AUTHENTICATION_POLICY_MISMATCH (warning) + + ER_PLUGIN_IS_NOT_LOADED + 2. Replica ER_PLUGIN_IS_NOT_LOADED + This will trigger inconsistency voting on both sides, but with different errors + so node will be evicted which is not correct, because on Source + ER_AUTHENTICATION_POLICY_MISMATCH was only warning. + + That's why here we are only detecting if we should replicate or not and + letting the rest of the flow do the job. Let's consider a few cases. In + original flow which follows the call of wsrep_check_for_auth_policy() the check + order is: plugin then policy check. We return on first error, continue on + warning. + + 1. Source: ER_AUTHENTICATION_POLICY_MISMATCH warning here => TOI + Replica: No warnings, no errors + Inconsistency voting: not triggered + 2. Source: ER_AUTHENTICATION_POLICY_MISMATCH error here => no TOI, continue + flow, return with ER_AUTHENTICATION_POLICY_MISMATCH + Inconsistency voting: not triggered + 3. Source: ER_AUTHENTICATION_POLICY_MISMATCH warning here => do TOI then return + with ER_PLUGIN_IS_NOT_LOADED + Replica: error with ER_PLUGIN_IS_NOT_LOADED + Inconsistency voting: triggered for both sides with ER_PLUGIN_IS_NOT_LOADED + */ +static bool wsrep_check_for_auth_policy(THD *thd, List &list, + const char *cmd) { + if (WSREP(thd) && !thd->wsrep_applier && list.size() > 0) { + // Accessing ACL cache requires lock. We get WRITE_MODE lock since the call + // to is_privileged_user_for_credential_change() below, might have to update + // part of ACL cache with DB-level privileges while it it checks if user has + // UPDATE privilege on 'mysql' database. + Acl_cache_lock_guard acl_cache_lock(thd, Acl_cache_lock_mode::WRITE_MODE); + + if (!acl_cache_lock.lock()) { + return true; + } + + LEX_USER *user_from, *tmp_user_from; + List_iterator user_list(list); + Dummy_error_handler error_handler; + bool is_privileged_user = is_privileged_user_for_credential_change(thd); + + thd->push_internal_handler(&error_handler); + raii::Sentry<> error_handler_pop_guard{ + [thd]() -> void { thd->pop_internal_handler(); }}; + + while ((tmp_user_from = user_list++)) { + acl_table::Pod_user_what_to_update dummy_what_to_alter; + I_multi_factor_auth *dummy_mfa = nullptr; + Userhostpassword_list dummy_generated_passwords; + + if (!(user_from = get_current_user(thd, tmp_user_from))) { + // let the original flow do the job + return false; + } + + /* set_and_validate_user_attributes() will hash provided password and + wipe-out its plaintext version in + user_from_tmp.first_factor_auth_info.auth.str. + LEX_CSTRING holds just a pointer, so we need to do a deep copy of + LEX_USER for original flow to work properly. */ + LEX_USER *user_from_tmp = deep_copy(thd, user_from); + + /* Skip password verification. It would append keys to writeset, but TOI + is not started yet, so it is not allowed. We don't need it now. Original + flow will handle it */ + if (set_and_validate_user_attributes( + thd, user_from_tmp, dummy_what_to_alter, is_privileged_user, + false, nullptr, nullptr, cmd, dummy_generated_passwords, + &dummy_mfa, false, false)) { + return true; + } + } + } + return false; +} +#endif /* WITH_WSREP */ + /* Create a list of users. @@ -2901,6 +3038,7 @@ bool mysql_create_user(THD *thd, List &list, bool if_not_exists, if (wsrep_check_system_user_privilege(thd, list)) { return true; } + bool block_toi = wsrep_check_for_auth_policy(thd, list, "CREATE USER"); #endif /* @@ -2912,9 +3050,14 @@ bool mysql_create_user(THD *thd, List &list, bool if_not_exists, Save_and_Restore_binlog_format_state binlog_format_state(thd); /* CREATE USER may be skipped on replication client. */ +#ifdef WITH_WSREP + if ((result = + open_grant_tables(thd, tables, &transactional_tables, block_toi))) + return result != 1; +#else if ((result = open_grant_tables(thd, tables, &transactional_tables))) return result != 1; - +#endif { /* Critical section */ Acl_cache_lock_guard acl_cache_lock(thd, Acl_cache_lock_mode::WRITE_MODE); @@ -3619,6 +3762,7 @@ bool mysql_alter_user(THD *thd, List &list, bool if_exists) { if (wsrep_check_system_user_privilege(thd, list)) { return true; } + bool block_toi = wsrep_check_for_auth_policy(thd, list, "ALTER USER"); #endif /* @@ -3628,9 +3772,14 @@ bool mysql_alter_user(THD *thd, List &list, bool if_exists) { values when we are out of this function scope */ Save_and_Restore_binlog_format_state binlog_format_state(thd); +#ifdef WITH_WSREP + if ((result = + open_grant_tables(thd, tables, &transactional_tables, block_toi))) + return result != 1; +#else if ((result = open_grant_tables(thd, tables, &transactional_tables))) return result != 1; - +#endif { /* Critical section */ Acl_cache_lock_guard acl_cache_lock(thd, Acl_cache_lock_mode::WRITE_MODE); diff --git a/sql/auth/sql_user_table.cc b/sql/auth/sql_user_table.cc index 0d12f64105c0..f8e5cfdd746e 100644 --- a/sql/auth/sql_user_table.cc +++ b/sql/auth/sql_user_table.cc @@ -1984,7 +1984,7 @@ static bool acl_tables_setup_for_write_and_acquire_mdl(THD *thd, #ifdef WITH_WSREP int open_grant_tables(THD *thd, Table_ref *tables, bool *transactional_tables, - const char *db [[maybe_unused]], + bool block_toi, const char *db [[maybe_unused]], const char *table [[maybe_unused]]) { #else int open_grant_tables(THD *thd, Table_ref *tables, bool *transactional_tables) { @@ -2043,7 +2043,8 @@ int open_grant_tables(THD *thd, Table_ref *tables, bool *transactional_tables) { bool skip_toi = (thd->lex->sql_command == SQLCOM_CREATE_SPFUNCTION || thd->lex->sql_command == SQLCOM_CREATE_PROCEDURE || thd->lex->sql_command == SQLCOM_DROP_FUNCTION || - thd->lex->sql_command == SQLCOM_DROP_PROCEDURE); + thd->lex->sql_command == SQLCOM_DROP_PROCEDURE) || + block_toi; /* Perform the TOI after the replication filter check to avoid replicating commands that won't be applied locally (due to a filter). diff --git a/sql/wsrep_applier.cc b/sql/wsrep_applier.cc index d99acf0720c2..73c007a65502 100644 --- a/sql/wsrep_applier.cc +++ b/sql/wsrep_applier.cc @@ -98,6 +98,11 @@ static uint wsrep_errno_for_voting(uint err) { return err; } +/* KH: We trigger inconsistency voting if there is an error (thd->is_error()) + but then calculate a vote using errors and warnings. Is it OK? + Eg. It may happen that source node issues some warnings because of executing + user privileges, and then fail with error. On replica side it can fail without + preceding warnings as we execute with root privileges */ void wsrep_store_error(const THD *const thd, wsrep::mutable_buffer &dst) { Diagnostics_area::Sql_condition_iterator it = thd->get_stmt_da()->sql_conditions();