From 851a8850bc3f9bb9c311a42c4d2c6f04a6890784 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Thu, 22 Aug 2024 16:29:22 +0200 Subject: [PATCH 1/2] Fix regression on 'COM_CHANGE_USER' for 'mysql_native_password' - Closes #4589 Fixes handing of 'COM_CHANGE_USER' for 'mysql_native_password' when using hashed user passwords. --- lib/MySQL_Protocol.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/MySQL_Protocol.cpp b/lib/MySQL_Protocol.cpp index 5392a229eb..cd13ea68fe 100644 --- a/lib/MySQL_Protocol.cpp +++ b/lib/MySQL_Protocol.cpp @@ -1225,7 +1225,7 @@ bool MySQL_Protocol::verify_user_pass( ret = false; } } else { - if (auth_plugin_id == 2) { + if (auth_plugin_id == 0) { if (session_type == PROXYSQL_SESSION_MYSQL || session_type == PROXYSQL_SESSION_SQLITE) { ret=proxy_scramble_sha1((char *)pass,(*myds)->myconn->scramble_buff,password+1, reply); if (ret) { From f934c3a0486b17f466c905e7beea1d6ccd20446c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Thu, 22 Aug 2024 18:33:35 +0200 Subject: [PATCH 2/2] Several improvements for 'reg_test_3504-change_user-t' - Improved test documentation and logging format. - Added extra options checking invalid passwords and hashed passwords. - Improved combinations on options being check. --- .../tap/tests/reg_test_3504-change_user-t.cpp | 242 ++++++++++++------ .../reg_test_3504-change_user_helper.cpp | 14 +- 2 files changed, 168 insertions(+), 88 deletions(-) diff --git a/test/tap/tests/reg_test_3504-change_user-t.cpp b/test/tap/tests/reg_test_3504-change_user-t.cpp index 29b0bcb1fb..9b488585bf 100644 --- a/test/tap/tests/reg_test_3504-change_user-t.cpp +++ b/test/tap/tests/reg_test_3504-change_user-t.cpp @@ -1,79 +1,155 @@ /** * @file reg_test_3504-change_user-t.cpp * @brief This test checks the new implementation for 'COM_CHANGE_USER' - * introduced in issue #3504. The test connects using different authentication - * methods: 'mysql_clear_password', 'mysql_native_password' and - * 'caching_sha2_password', with and without SSL enabled. And verifies that both, - * the initial connection and the later 'mysql_change_user' are properly executed. - * Connections are performed using 'libmysqlclient' and 'libmariadb'. - * @details For making this possible the test uses two helper binaries, which - * are the ones performing the connection to ProxySQL, and communicates to - * them through a payload format that is specified in this helper tests files: - * - 'reg_test_3504-change_user_libmysql_helper.cpp' - * - 'reg_test_3504-change_user_libmariadb_helper.cpp' + * introduced in issue #3504. The test connects using different authentication methods: + * - 'mysql_clear_password' + * - 'mysql_native_password' + * - 'caching_sha2_password' + * It also checks that the following options are handled correctly: + * - With and without SSL. + * - Using same and different users. + * - Hashed and non-hashed user passwords are correctly handled. + * The test verifies that both, the initial connection and the later 'mysql_change_user' are + * properly executed. Connections are performed using 'libmysqlclient' and 'libmariadb'. + * @details For making this possible the test uses two helper binaries, which are the ones performing the + * connection to ProxySQL, and communicates to them through a payload format that is specified in this helper + * tests files: + * - 'reg_test_3504-change_user_libmysql_helper.cpp' + * - 'reg_test_3504-change_user_libmariadb_helper.cpp' */ #include #include #include #include -#include +#include #include #include #include "mysql.h" -#include "mysqld_error.h" #include "command_line.h" -#include "proxysql_utils.h" #include "json.hpp" #include "tap.h" #include "utils.h" using nlohmann::json; +using std::string; +using std::vector; + +struct test_opts { + string auth; + bool use_ssl; + bool change_user; + bool hashed_pass; + bool inv_pass; +}; -using test_opts = std::tuple; +const vector client_req_auths { + "mysql_clear_password", + "mysql_native_password", + "caching_sha2_password" +}; -const std::vector tests_defs { - std::make_tuple("mysql_clear_password", false, false), - std::make_tuple("mysql_native_password", false, false), - std::make_tuple("caching_sha2_password", false, false), +vector gen_tests_defs() { + // Gen all option permutations - SSL, different user, and hashed user passwords. + const auto flags_perms { get_all_bin_vec(4) }; - std::make_tuple("mysql_clear_password", true, false), - std::make_tuple("mysql_native_password", true, false), - std::make_tuple("caching_sha2_password", true, false), + // Use all options for each supported auth method + vector res {}; - std::make_tuple("mysql_clear_password", false, true), - std::make_tuple("mysql_native_password", false, true), - std::make_tuple("caching_sha2_password", false, true), + for (const auto& flags : flags_perms) { + for (const string& auth : client_req_auths) { + res.push_back({auth, flags[0], flags[1], flags[2], flags[3]}); + } + } - std::make_tuple("mysql_clear_password", true, true), - std::make_tuple("mysql_native_password", true, true), - std::make_tuple("caching_sha2_password", true, true), -}; + return res; +} + +const string PRIM_USER { get_env_str("TAP_CHANGE_USER__PRIM_USER", "sbtest1") }; +const string SECD_USER { get_env_str("TAP_CHANGE_USER__SECD_USER", "root") }; + +const char LOAD_USERS_TO_RUNTIME[] { "LOAD MYSQL USERS TO RUNTIME" }; + +int update_user_pass(MYSQL* admin, const string& user, const string& pass) { + int rc = mysql_query_t(admin, + ("UPDATE mysql_users SET password=" + pass + "" + " WHERE username='" + user + "'").c_str() + ); + if (rc) { + diag( + "Failed to set HASHED user pass. Aborting check user='%s' error='%s'", + user.c_str(), mysql_error(admin) + ); + } + + return rc; +} + +string gen_inv_pass(const string& pass) { + string rnd_str { random_string(rand() % 60 + 1) }; + + while (rnd_str == pass) { + rnd_str = random_string(rand() % 60 + 1); + } + + return rnd_str; +} + +const string opts_to_string(const test_opts& opts) { + nlohmann::json j_opts {}; + + j_opts["auth"] = opts.auth; + j_opts["use_ssl"] = opts.use_ssl; + j_opts["mix_users"] = opts.change_user; + j_opts["hashed_pass"] = opts.hashed_pass; + j_opts["inv_pass"] = opts.inv_pass; + + return j_opts.dump(); +} void perform_helper_test( + MYSQL* admin, const std::string& helper_path, - const test_opts& test_opts + const test_opts& opts ) { + diag("Preparing call to helper opts='%s'", opts_to_string(opts).c_str()); + std::string result {}; - std::string auth { std::get<0>(test_opts) }; - bool exp_SSL_val = std::get<1>(test_opts); - bool change_user = std::get<2>(test_opts); + + if (opts.hashed_pass) { + for (const string& user : { PRIM_USER, SECD_USER }) { + int rc = update_user_pass(admin, user, "MYSQL_NATIVE_PASSWORD('" + user + "')"); + if (rc) { return; } + } + } else { + for (const string& user : { PRIM_USER, SECD_USER }) { + int rc = update_user_pass(admin, user, "'" + user + "'"); + if (rc) { return; } + } + } + + int rc = mysql_query_t(admin, "LOAD MYSQL USERS TO RUNTIME"); + if (rc) { + diag("Failed to execute query. Aborting check error='%s'", mysql_error(admin)); + return; + } nlohmann::json input_json {}; - input_json["user"] = "sbtest1"; - input_json["pass"] = "sbtest1"; - input_json["ch_user"] = "root"; - input_json["ch_pass"] = "root"; - input_json["auth"] = auth; + input_json["user"] = PRIM_USER; + input_json["pass"] = PRIM_USER; + input_json["ch_user"] = SECD_USER; + input_json["ch_pass"] = opts.inv_pass ? gen_inv_pass(SECD_USER) : SECD_USER; + input_json["auth"] = opts.auth; input_json["charset"] = ""; input_json["port"] = 6033; - input_json["SSL"] = exp_SSL_val; - input_json["CHANGE_USER"] = change_user; + input_json["SSL"] = opts.use_ssl; + input_json["CHANGE_USER"] = opts.change_user; std::string input_str { input_json.dump() }; + diag("Calling test helper params='%s'", input_json.dump().c_str()); + std::vector v_argv { helper_path.c_str(), input_str.c_str() }; int res = execvp(helper_path, v_argv, result); @@ -86,7 +162,7 @@ void perform_helper_test( bool act_SSL_val; std::vector exp_ch_usernames {}; - if (change_user) { + if (opts.change_user) { exp_ch_usernames = { "root", "sbtest1", "root" }; } else { exp_ch_usernames = { "sbtest1", "sbtest1", "sbtest1" }; @@ -105,11 +181,11 @@ void perform_helper_test( def_auth_plugin = output_res.at("def_auth_plugin"); act_SSL_val = output_res.at("ssl_enabled"); - if (auth == "mysql_clear_password") { + if (opts.auth == "mysql_clear_password") { exp_switching_auth_type = -1; - } else if (auth == "mysql_native_password") { + } else if (opts.auth == "mysql_native_password") { exp_switching_auth_type = -1; - } else if (auth == "caching_sha2_password") { + } else if (opts.auth == "caching_sha2_password") { exp_switching_auth_type = 0; } @@ -120,28 +196,39 @@ void perform_helper_test( diag("Invalid JSON result from helper, parsing failed: '%s'", ex.what()); } - std::string exp_user_names_str = - std::accumulate(exp_ch_usernames.begin(), exp_ch_usernames.end(), std::string(), - [](const std::string& str, const std::string& splice) -> std::string { - return str + (str.length() > 0 ? "," : "") + splice; - }); - std::string act_user_names_str = - std::accumulate(act_ch_usernames.begin(), act_ch_usernames.end(), std::string(), - [](const std::string& str, const std::string& splice) -> std::string { - return str + (str.length() > 0 ? "," : "") + splice; - }); - - ok( - (exp_switching_auth_type == act_switching_auth_type) && - (exp_SSL_val == act_SSL_val) && err_msg.empty() && - exp_ch_usernames == act_ch_usernames, - "Connect and COM_CHANGE_USER should work for the supplied values.\n" - " + Expected values where: (client_auth_plugin='%s', switching_auth_type='%d', SSL='%d', usernames=['%s']),\n" - " + Actual values where: (client_auth_plugin='%s', switching_auth_type='%d', SSL='%d, usernames=['%s']').\n" - " Error message: %s.\n", - auth.c_str(), exp_switching_auth_type, exp_SSL_val, exp_user_names_str.c_str(), def_auth_plugin.c_str(), - act_switching_auth_type, act_SSL_val, act_user_names_str.c_str(), err_msg.c_str() - ); + // Failure with invalid CHANGE_USER pass only for real change user ops - src_user != tg_user. + if (!opts.inv_pass || !opts.change_user) { + std::string exp_user_names_str = + std::accumulate(exp_ch_usernames.begin(), exp_ch_usernames.end(), std::string(), + [](const std::string& str, const std::string& splice) -> std::string { + return str + (str.length() > 0 ? "," : "") + splice; + }); + std::string act_user_names_str = + std::accumulate(act_ch_usernames.begin(), act_ch_usernames.end(), std::string(), + [](const std::string& str, const std::string& splice) -> std::string { + return str + (str.length() > 0 ? "," : "") + splice; + }); + + ok( + (exp_switching_auth_type == act_switching_auth_type) && + (opts.use_ssl == act_SSL_val) && err_msg.empty() && + exp_ch_usernames == act_ch_usernames, + "Connect and COM_CHANGE_USER should work for the supplied values.\n" + " + Expected: (client_auth_plugin='%s', switching_auth_type='%d', SSL='%d', usernames=['%s']),\n" + " + Actual: (client_auth_plugin='%s', switching_auth_type='%d', SSL='%d, usernames=['%s']').\n" + " Error message: %s.", + opts.auth.c_str(), exp_switching_auth_type, opts.use_ssl, exp_user_names_str.c_str(), + def_auth_plugin.c_str(), act_switching_auth_type, act_SSL_val, act_user_names_str.c_str(), + err_msg.c_str() + ); + } else { + const string::size_type f_it { err_msg.find("Failed to change user") }; + ok( + res != 0 && !err_msg.empty() && f_it != string::npos, + "COM_CHANGE_USER should fail with 'Access denied' for invalid creds res=%d err='%s'", + res, err_msg.c_str() + ); + } } int main(int argc, char** argv) { @@ -152,24 +239,25 @@ int main(int argc, char** argv) { return EXIT_FAILURE; } - MYSQL* proxysql_admin = mysql_init(NULL); + srand(time(NULL)); + MYSQL* admin = mysql_init(NULL); if ( !mysql_real_connect( - proxysql_admin, "127.0.0.1", cl.admin_username, cl.admin_password, - "information_schema", cl.admin_port, NULL, 0 + admin, "127.0.0.1", cl.admin_username, cl.admin_password, NULL, cl.admin_port, NULL, 0 ) ) { - fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_admin)); + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); return EXIT_FAILURE; } - MYSQL_QUERY(proxysql_admin, "SET mysql-have_ssl='true'"); - MYSQL_QUERY(proxysql_admin, "LOAD MYSQL VARIABLES TO RUNTIME"); - - // Give some time after the 'LOAD TO RUNTIME' - usleep(500 * 1000); + // TODO: This test now only checks support for 'mysql_native_password'. This should be changed once + // 'COM_CHANGE_USER' is supported for 'caching_sha2_password'. See #4618. + MYSQL_QUERY(admin, "SET mysql-default_authentication_plugin='mysql_native_password'"); + MYSQL_QUERY(admin, "SET mysql-have_ssl='true'"); + MYSQL_QUERY(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); + const vector tests_defs { gen_tests_defs() }; plan(tests_defs.size() * 2); diag("Starting tests for helper 'reg_test_3504-change_user_libmysql_helper'\n"); @@ -178,7 +266,7 @@ int main(int argc, char** argv) { std::string { cl.workdir } + "reg_test_3504-change_user_libmysql_helper" }; for (const auto& test_opts : tests_defs) { - perform_helper_test(libmysql_helper_path, test_opts); + perform_helper_test(admin, libmysql_helper_path, test_opts); } std::cout << "\n"; @@ -188,10 +276,10 @@ int main(int argc, char** argv) { std::string { cl.workdir } + "reg_test_3504-change_user_libmariadb_helper" }; for (const auto& test_opts : tests_defs) { - perform_helper_test(libmariadb_helper_path, test_opts); + perform_helper_test(admin, libmariadb_helper_path, test_opts); } - mysql_close(proxysql_admin); + mysql_close(admin); return exit_status(); } diff --git a/test/tap/tests/reg_test_3504-change_user_helper.cpp b/test/tap/tests/reg_test_3504-change_user_helper.cpp index ee6ba80dbb..a6ac1ef4e2 100644 --- a/test/tap/tests/reg_test_3504-change_user_helper.cpp +++ b/test/tap/tests/reg_test_3504-change_user_helper.cpp @@ -210,10 +210,7 @@ int main(int argc, char** argv) { port, NULL, 0 ) ) { - string_format( - "Failed to connect to database: Error: %s\n", err_msg, - mysql_error(&mysql) - ); + string_format("Failed to connect to database: Error: %s", err_msg, mysql_error(&mysql)); output["err_msg"] = err_msg; res = EXIT_FAILURE; @@ -234,10 +231,7 @@ int main(int argc, char** argv) { } if (!conn_res) { - string_format( - "Failed to connect to database: Error: %s\n", err_msg, - mysql_error(&mysql) - ); + string_format("Failed to connect to database: Error: %s", err_msg, mysql_error(&mysql)); output["err_msg"] = err_msg; res = EXIT_FAILURE; @@ -282,9 +276,7 @@ int main(int argc, char** argv) { if (CHANGE_USER) { if (mysql_change_user(&mysql, user.c_str(), pass.c_str(), "information_schema")) { - string_format( - "Failed to change user. Error: %s\n", err_msg, mysql_error(&mysql) - ); + string_format("Failed to change user. Error: %s", err_msg, mysql_error(&mysql)); output["err_msg"] = err_msg; tmp_res = EXIT_FAILURE; }