From 7e21e3aedd4efba890419480a25ac080d0545ba4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Oct 2018 11:01:03 +0300 Subject: [PATCH 1/5] MXS-2115: Fix handshake version string The intention was to send the lowest backend version string automatically to the client instead of the default handshake version. This did not work as the service version string was used instead of the server version. --- maxscale-system-test/CMakeLists.txt | 3 ++ .../mxs2115_version_string.cpp | 21 +++++++++ .../MySQL/mariadbclient/mysql_client.cc | 44 ++++++++++++------- 3 files changed, 51 insertions(+), 17 deletions(-) create mode 100644 maxscale-system-test/mxs2115_version_string.cpp diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 994fa48e2..0ddff7c5b 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -981,6 +981,9 @@ add_test_executable(mxs2043_select_for_update.cpp mxs2043_select_for_update repl # MXS-2054: Hybrid clusters add_test_executable(mxs2054_hybrid_cluster.cpp mxs2054_hybrid_cluster mxs2054_hybrid_cluster LABELS REPL_BACKEND) +# MXS-2115: Automatic version_string detection +add_test_executable(mxs2115_version_string.cpp mxs2115_version_string replication LABELS REPL_BACKEND) + configure_file(templates.h.in templates.h @ONLY) include(CTest) diff --git a/maxscale-system-test/mxs2115_version_string.cpp b/maxscale-system-test/mxs2115_version_string.cpp new file mode 100644 index 000000000..48530c3ae --- /dev/null +++ b/maxscale-system-test/mxs2115_version_string.cpp @@ -0,0 +1,21 @@ +/** + * MXS-2115: Automatic version string detection doesn't work + * + * When servers are available, the backend server and Maxscale should return the + * same version string. + */ + +#include "testconnections.h" + +int main(int argc, char **argv) +{ + TestConnections test(argc, argv); + test.repl->connect(); + test.maxscales->connect(); + + std::string direct = mysql_get_server_info(test.repl->nodes[0]); + std::string mxs = mysql_get_server_info(test.maxscales->conn_rwsplit[0]); + test.expect(direct == mxs, "MaxScale sends wrong version: %s != %s", direct.c_str(), mxs.c_str()); + + return test.global_result; +} diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index 12d81dbd0..2f79a36c6 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -196,6 +196,29 @@ static char *gw_default_auth() return (char*)"MySQLAuth"; } +std::string get_version_string(SERVICE* service) +{ + std::string rval; + uint64_t intver = UINT64_MAX; + + for (SERVER_REF* ref = service->dbref; ref; ref = ref->next) + { + if (ref->server->version && ref->server->version < intver) + { + rval = ref->server->version_string; + intver = ref->server->version; + } + } + + // Get the version string from service if no server version is available + if (rval.empty()) + { + rval = service->version_string[0] ? service->version_string : GW_MYSQL_VERSION; + } + + return rval; +} + /** * MySQLSendHandshake * @@ -222,8 +245,6 @@ int MySQLSendHandshake(DCB* dcb) uint8_t mysql_filler_ten[10] = {}; /* uint8_t mysql_last_byte = 0x00; not needed */ char server_scramble[GW_MYSQL_SCRAMBLE_SIZE + 1] = ""; - char *version_string; - int len_version_string = 0; bool is_maria = false; @@ -240,18 +261,7 @@ int MySQLSendHandshake(DCB* dcb) MySQLProtocol *protocol = DCB_PROTOCOL(dcb, MySQLProtocol); GWBUF *buf; - - /* get the version string from service property if available*/ - if (dcb->service->version_string[0]) - { - version_string = dcb->service->version_string; - len_version_string = strlen(version_string); - } - else - { - version_string = (char*)GW_MYSQL_VERSION; - len_version_string = strlen(GW_MYSQL_VERSION); - } + std::string version = get_version_string(dcb->service); gw_generate_random_str(server_scramble, GW_MYSQL_SCRAMBLE_SIZE); @@ -285,7 +295,7 @@ int MySQLSendHandshake(DCB* dcb) int plugin_name_len = strlen(plugin_name); mysql_payload_size = - sizeof(mysql_protocol_version) + (len_version_string + 1) + sizeof(mysql_thread_id_num) + 8 + + sizeof(mysql_protocol_version) + (version.length() + 1) + sizeof(mysql_thread_id_num) + 8 + sizeof(/* mysql_filler */ uint8_t) + sizeof(mysql_server_capabilities_one) + sizeof(mysql_server_language) + sizeof(mysql_server_status) + sizeof(mysql_server_capabilities_two) + sizeof(mysql_scramble_len) + sizeof(mysql_filler_ten) + 12 + sizeof(/* mysql_last_byte */ uint8_t) + plugin_name_len + @@ -314,8 +324,8 @@ int MySQLSendHandshake(DCB* dcb) mysql_handshake_payload = mysql_handshake_payload + sizeof(mysql_protocol_version); // write server version plus 0 filler - strcpy((char *)mysql_handshake_payload, version_string); - mysql_handshake_payload = mysql_handshake_payload + len_version_string; + strcpy((char *)mysql_handshake_payload, version.c_str()); + mysql_handshake_payload = mysql_handshake_payload + version.length(); *mysql_handshake_payload = 0x00; From 2594a0d913827e13cb8b6f44452e65ce38b36467 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Oct 2018 15:17:38 +0300 Subject: [PATCH 2/5] Improve detection of problems caused by MDEV-13453 Instead of looking at the server version, the actual error message should be inspected. This guarantees that the correct error message is logged even with custom builds. --- server/modules/authenticator/MySQLAuth/dbusers.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/modules/authenticator/MySQLAuth/dbusers.c b/server/modules/authenticator/MySQLAuth/dbusers.c index f02f34da4..9e601b49f 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.c +++ b/server/modules/authenticator/MySQLAuth/dbusers.c @@ -863,8 +863,7 @@ static bool roles_are_available(MYSQL* conn, SERVICE* service, SERVER* server) static void report_mdev13453_problem(MYSQL *con, SERVER *server) { - if (server->version >= 100200 && server->version < 100211 && - mxs_pcre2_simple_match("SELECT command denied to user .* for table 'users'", + if (mxs_pcre2_simple_match("SELECT command denied to user .* for table 'users'", mysql_error(con), 0, NULL) == MXS_PCRE2_MATCH) { char user[256] = ""; // Enough for all user-hostname combinations From 93b9ed744fd885e8fab14991cc107aed140705f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Oct 2018 15:12:54 +0300 Subject: [PATCH 3/5] MXS-2111: Use authentication_string when password is empty If the password field in mysql.user is empty, it is possible that the actual password is stored in the authentication_string field. Most of the time this happens due to MDEV-16774 which causes the password to be stored in the authentication_string field. Also added a test case that verifies the problem and that it is fixed by this commit. --- maxscale-system-test/CMakeLists.txt | 3 ++ maxscale-system-test/mxs2111_auth_string.cpp | 32 +++++++++++++++++++ .../modules/authenticator/MySQLAuth/dbusers.c | 8 +++-- 3 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 maxscale-system-test/mxs2111_auth_string.cpp diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 0ddff7c5b..2feaa1a80 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -981,6 +981,9 @@ add_test_executable(mxs2043_select_for_update.cpp mxs2043_select_for_update repl # MXS-2054: Hybrid clusters add_test_executable(mxs2054_hybrid_cluster.cpp mxs2054_hybrid_cluster mxs2054_hybrid_cluster LABELS REPL_BACKEND) +# MXS-2111: mysql.user sometimes has SHA1 in authentication_string instead of password +add_test_executable(mxs2111_auth_string.cpp mxs2111_auth_string replication LABELS REPL_BACKEND) + # MXS-2115: Automatic version_string detection add_test_executable(mxs2115_version_string.cpp mxs2115_version_string replication LABELS REPL_BACKEND) diff --git a/maxscale-system-test/mxs2111_auth_string.cpp b/maxscale-system-test/mxs2111_auth_string.cpp new file mode 100644 index 000000000..50d008ee8 --- /dev/null +++ b/maxscale-system-test/mxs2111_auth_string.cpp @@ -0,0 +1,32 @@ +/** + * MXS-2111: The password is stored in `authentication_string` instead of `password` due to MDEV-16774 + */ + +#include "testconnections.h" + +int main(int argc, char **argv) +{ + TestConnections::require_repl_version("10.2.0"); + TestConnections test(argc, argv); + + auto batch = [&](std::vector queries){ + test.maxscales->connect(); + for (const auto& a: queries) + { + test.try_query(test.maxscales->conn_rwsplit[0], "%s", a.c_str()); + } + test.maxscales->disconnect(); + }; + + batch({"CREATE USER 'test' IDENTIFIED BY 'test'", + "GRANT SELECT ON *.* TO test", + "SET PASSWORD FOR 'test' = PASSWORD('test')"}); + + MYSQL* conn = open_conn(test.maxscales->rwsplit_port[0], test.maxscales->IP[0], "test", "test"); + test.try_query(conn, "SELECT 1"); + mysql_close(conn); + + batch({"DROP USER 'test'"}); + + return test.global_result; +} diff --git a/server/modules/authenticator/MySQLAuth/dbusers.c b/server/modules/authenticator/MySQLAuth/dbusers.c index 9e601b49f..0dc0c1ed2 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.c +++ b/server/modules/authenticator/MySQLAuth/dbusers.c @@ -56,11 +56,15 @@ const char* mariadb_102_users_query = // `t` is users that are not roles "WITH RECURSIVE t AS ( " - " SELECT u.user, u.host, d.db, u.select_priv, u.password AS password, u.is_role, u.default_role" + " SELECT u.user, u.host, d.db, u.select_priv, " + " IF(u.password <> '', u.password, u.authentication_string) AS password, " + " u.is_role, u.default_role" " FROM mysql.user AS u LEFT JOIN mysql.db AS d " " ON (u.user = d.user AND u.host = d.host) " " UNION " - " SELECT u.user, u.host, t.db, u.select_priv, u.password AS password, u.is_role, u.default_role " + " SELECT u.user, u.host, t.db, u.select_priv, " + " IF(u.password <> '', u.password, u.authentication_string), " + " u.is_role, u.default_role " " FROM mysql.user AS u LEFT JOIN mysql.tables_priv AS t " " ON (u.user = t.user AND u.host = t.host)" "), users AS (" From eb10b723dd6593dc57124c8fc8fef4d49ab10c3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Oct 2018 16:09:28 +0300 Subject: [PATCH 4/5] MXS-2117: Fall back to old style query with 10.2.11 If a 10.2.11 or older server without a grant on all mysql tables is found, the authenticator now falls back to the 10.1 behavior that uses subqueries instead of CTEs. This is a more user friendly way of working around MDEV-13453 that causes the problem as all functionality except the support for composite roles is retained. --- .../modules/authenticator/MySQLAuth/dbusers.c | 116 +++++++++++------- 1 file changed, 70 insertions(+), 46 deletions(-) diff --git a/server/modules/authenticator/MySQLAuth/dbusers.c b/server/modules/authenticator/MySQLAuth/dbusers.c index 0dc0c1ed2..2b5f12f61 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.c +++ b/server/modules/authenticator/MySQLAuth/dbusers.c @@ -865,8 +865,10 @@ static bool roles_are_available(MYSQL* conn, SERVICE* service, SERVER* server) return rval; } -static void report_mdev13453_problem(MYSQL *con, SERVER *server) +static bool have_mdev13453_problem(MYSQL *con, SERVER *server) { + bool rval = false; + if (mxs_pcre2_simple_match("SELECT command denied to user .* for table 'users'", mysql_error(con), 0, NULL) == MXS_PCRE2_MATCH) { @@ -888,9 +890,58 @@ static void report_mdev13453_problem(MYSQL *con, SERVER *server) mysql_free_result(res); } - MXS_ERROR("Due to MDEV-13453, the service user requires extra grants on the `mysql` database. " - "To fix the problem, add the following grant: GRANT SELECT ON `mysql`.* TO %s", user); + MXS_WARNING("Due to MDEV-13453, the service user requires extra grants on the `mysql` database in " + "order for roles to be used. To fix the problem, add the following grant: " + "GRANT SELECT ON `mysql`.* TO %s", user); + rval = true; } + + return rval; +} + +bool query_and_process_users(const char* query, MYSQL *con, sqlite3* handle, SERVICE* service, bool* anon_user, int* users) +{ + bool rval = false; + + if (mxs_mysql_query(con, "USE mysql") == 0 && // Set default database in case we use CTEs + mxs_mysql_query(con, query) == 0) + { + MYSQL_RES *result = mysql_store_result(con); + + if (result) + { + MYSQL_ROW row; + + while ((row = mysql_fetch_row(result))) + { + if (service->strip_db_esc) + { + strip_escape_chars(row[2]); + } + + if (strchr(row[1], '/')) + { + merge_netmask(row[1]); + } + + add_mysql_user(handle, row[0], row[1], row[2], + row[3] && strcmp(row[3], "Y") == 0, row[4]); + (*users)++; + + if (row[0] && *row[0] == '\0') + { + /** Empty username is used for the anonymous user. This means + that localhost does not match wildcard host. */ + *anon_user = true; + } + } + + mysql_free_result(result); + rval = true; + } + } + + return rval; } int get_users_from_server(MYSQL *con, SERVER_REF *server_ref, SERVICE *service, SERV_LISTENER *listener) @@ -910,53 +961,26 @@ int get_users_from_server(MYSQL *con, SERVER_REF *server_ref, SERVICE *service, bool anon_user = false; int users = 0; - if (query) + bool rv = query_and_process_users(query, con, handle, service, &anon_user, &users); + + if (!rv && have_mdev13453_problem(con, server_ref->server)) { - if (mxs_mysql_query(con, "USE mysql") == 0 && // Set default database in case we use CTEs - mxs_mysql_query(con, query) == 0) - { - MYSQL_RES *result = mysql_store_result(con); - - if (result) - { - MYSQL_ROW row; - - while ((row = mysql_fetch_row(result))) - { - if (service->strip_db_esc) - { - strip_escape_chars(row[2]); - } - - if (strchr(row[1], '/')) - { - merge_netmask(row[1]); - } - - add_mysql_user(handle, row[0], row[1], row[2], - row[3] && strcmp(row[3], "Y") == 0, row[4]); - users++; - - if (row[0] && *row[0] == '\0') - { - /** Empty username is used for the anonymous user. This means - that localhost does not match wildcard host. */ - anon_user = true; - } - } - - mysql_free_result(result); - } - } - else - { - MXS_ERROR("Failed to load users from server '%s': %s", server_ref->server->name, mysql_error(con)); - report_mdev13453_problem(con, server_ref->server); - } - + /** + * Try to work around MDEV-13453 by using a query without CTEs. Masquerading as + * a 10.1.10 server makes sure CTEs aren't used. + */ MXS_FREE(query); + query = get_users_query(server_ref->server->version_string, 100110, service->enable_root, true); + rv = query_and_process_users(query, con, handle, service, &anon_user, &users); } + if (!rv) + { + MXS_ERROR("Failed to load users from server '%s': %s", server_ref->server->name, mysql_error(con)); + } + + MXS_FREE(query); + /** Set the parameter if it is not configured by the user */ if (service->localhost_match_wildcard_host == SERVICE_PARAM_UNINIT) { From 91c5f8580c76b1161636972b7a1322a4fec0a147 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 29 Oct 2018 02:04:21 +0200 Subject: [PATCH 5/5] MXS-2119: Fix file permissions The admin files are now created with 640 permissions and automatically created directories now properly set the permissions for the group as well. All files and directories created by avrorouter and binlogrouter also now correctly limit the read and write permissions only to the owner and the group. --- server/core/adminusers.cc | 41 +++++++++++++++---- server/core/config.cc | 9 ++-- server/modules/routing/avrorouter/avro.c | 2 +- .../modules/routing/binlogrouter/blr_file.c | 8 ++-- .../routing/binlogrouter/maxbinlogcheck.c | 2 +- 5 files changed, 43 insertions(+), 19 deletions(-) diff --git a/server/core/adminusers.cc b/server/core/adminusers.cc index dd8d807b2..6af66349c 100644 --- a/server/core/adminusers.cc +++ b/server/core/adminusers.cc @@ -15,12 +15,14 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include #include @@ -60,8 +62,6 @@ void admin_users_init() static bool admin_dump_users(USERS* users, const char* fname) { - char path[PATH_MAX]; - if (access(get_datadir(), F_OK) != 0) { if (mkdir(get_datadir(), S_IRWXU) != 0 && errno != EEXIST) @@ -72,17 +72,40 @@ static bool admin_dump_users(USERS* users, const char* fname) } } - snprintf(path, sizeof(path), "%s/%s", get_datadir(), fname); - json_t* json = users_to_json(users); - bool rval = true; + bool rval = false; + std::string path = std::string(get_datadir()) + "/" + fname; + std::string tmppath = path + ".tmp"; - if (json_dump_file(json, path, 0) == -1) + int fd = open(tmppath.c_str(), O_CREAT | O_WRONLY | O_TRUNC, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP); + + if (fd == -1) { - MXS_ERROR("Failed to dump admin users to file"); - rval = false; + MXS_ERROR("Failed to create '%s': %d, %s", tmppath.c_str(), errno, mxs_strerror(errno)); } + else + { + json_t* json = users_to_json(users); + char* str = json_dumps(json, 0); + json_decref(json); - json_decref(json); + if (write(fd, str, strlen(str)) == -1) + { + MXS_ERROR("Failed to dump admin users to '%s': %d, %s", + tmppath.c_str(), errno, mxs_strerror(errno)); + } + else if (rename(tmppath.c_str(), path.c_str()) == -1) + { + MXS_ERROR("Failed to rename to '%s': %d, %s", + path.c_str(), errno, mxs_strerror(errno)); + } + else + { + rval = true; + } + + MXS_FREE(str); + close(fd); + } return rval; } diff --git a/server/core/config.cc b/server/core/config.cc index 62f86730a..f0d44e12d 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -3811,21 +3811,22 @@ static bool check_path_parameter(const MXS_MODULE_PARAM *params, const char *val } int mode = F_OK; - int mask = X_OK; + int mask = 0; if (params->options & MXS_MODULE_OPT_PATH_W_OK) { - mask |= S_IWUSR; + mask |= S_IWUSR | S_IWGRP; mode |= W_OK; } if (params->options & MXS_MODULE_OPT_PATH_R_OK) { - mask |= S_IRUSR; + mask |= S_IRUSR | S_IRGRP; mode |= R_OK; } if (params->options & MXS_MODULE_OPT_PATH_X_OK) { - mask |= S_IXUSR; + mask |= S_IXUSR | S_IXGRP; + mode |= X_OK; } if (access(buf, mode) == 0) diff --git a/server/modules/routing/avrorouter/avro.c b/server/modules/routing/avrorouter/avro.c index 74bc6cfbb..bf9c44ddf 100644 --- a/server/modules/routing/avrorouter/avro.c +++ b/server/modules/routing/avrorouter/avro.c @@ -1274,7 +1274,7 @@ static bool ensure_dir_ok(const char* path, int mode) if (rp) { /** Make sure the directory exists */ - if (mkdir(rp, 0774) == 0 || errno == EEXIST) + if (mkdir(rp, 0770) == 0 || errno == EEXIST) { if (access(rp, mode) == 0) { diff --git a/server/modules/routing/binlogrouter/blr_file.c b/server/modules/routing/binlogrouter/blr_file.c index d2865032f..26b835a9b 100644 --- a/server/modules/routing/binlogrouter/blr_file.c +++ b/server/modules/routing/binlogrouter/blr_file.c @@ -511,7 +511,7 @@ blr_file_create(ROUTER_INSTANCE *router, char *orig_file) // Set final file name full path strcat(path, file); - int fd = open(path, O_RDWR | O_CREAT, 0666); + int fd = open(path, O_RDWR | O_CREAT, 0660); if (fd != -1) { @@ -614,7 +614,7 @@ blr_file_append(ROUTER_INSTANCE *router, char *file) //Add filename strcat(path, file); - if ((fd = open(path, flags, 0666)) == -1) + if ((fd = open(path, flags, 0660)) == -1) { MXS_ERROR("Failed to open binlog file %s for append.", path); @@ -937,7 +937,7 @@ blr_open_binlog(ROUTER_INSTANCE *router, /* Add file name */ strcat(path, binlog); - if ((file->fd = open(path, O_RDONLY, 0666)) == -1) + if ((file->fd = open(path, O_RDONLY, 0660)) == -1) { MXS_ERROR("Failed to open binlog file %s", path); MXS_FREE(file); @@ -1526,7 +1526,7 @@ blr_cache_response(ROUTER_INSTANCE *router, char *response, GWBUF *buf) strcat(path, "/"); strcat(path, response); - if ((fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666)) == -1) + if ((fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0660)) == -1) { return; } diff --git a/server/modules/routing/binlogrouter/maxbinlogcheck.c b/server/modules/routing/binlogrouter/maxbinlogcheck.c index ad02d5a93..c6f179769 100644 --- a/server/modules/routing/binlogrouter/maxbinlogcheck.c +++ b/server/modules/routing/binlogrouter/maxbinlogcheck.c @@ -174,7 +174,7 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } - int fd = open(path, binlog_file.fix ? O_RDWR : O_RDONLY, 0666); + int fd = open(path, binlog_file.fix ? O_RDWR : O_RDONLY, 0660); if (fd == -1) { printf("ERROR: Failed to open binlog file %s: %s.\n",