From 5b3a209643d180e7764cec1299c5a9fb030ea654 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 9 Nov 2018 10:22:32 +0200 Subject: [PATCH 1/6] Update the masking documentation --- Documentation/Filters/Masking.md | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/Documentation/Filters/Masking.md b/Documentation/Filters/Masking.md index 4b68df3a3..d87151c6c 100644 --- a/Documentation/Filters/Masking.md +++ b/Documentation/Filters/Masking.md @@ -37,25 +37,14 @@ the _ssn_ would be masked, as in ## Security -Note that he masking filter alone is *not* sufficient for preventing -access to a particular column. As the masking filter works on the column -name alone a query like -``` -> SELECT name, concat(ssn) FROM person; -``` -will reveal the value. Also, executing a query like -``` -> SELECT name FROM person WHERE ssn = ...; -``` -a sufficient number of times with different _ssn_ values, will, eventually, -reveal the social security number of all persons in the database. +From MaxScale 2.3 onwards, the masking filter will reject statements +that use functions in conjunction with columns that should be masked. +Allowing function usage provides a way for circumventing the masking, +unless a firewall filter is separately configured and installed. -For a secure solution, the masking filter *must* be combined with the -firewall filter to prevent the use of functions using which the masking -can be bypassed. - -In a future release, the combined use of the masking filter and the -database firewall filter will be simplified. +Please see the configuration parameter +[prevent_function_usage](#prevent_function_usage) +for how to change the default behaviour. ## Limitations From 226fe4871d4a3b2aacfac4a97d6cb36b97e13c91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 9 Nov 2018 08:20:36 +0200 Subject: [PATCH 2/6] Log mysql error message in mxs1776_ps_exec_hang If the test fails, log the error message. This should help understand why the test failed. --- maxscale-system-test/mxs1776_ps_exec_hang.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/maxscale-system-test/mxs1776_ps_exec_hang.cpp b/maxscale-system-test/mxs1776_ps_exec_hang.cpp index 09c41799c..e8d0c0d9e 100644 --- a/maxscale-system-test/mxs1776_ps_exec_hang.cpp +++ b/maxscale-system-test/mxs1776_ps_exec_hang.cpp @@ -50,8 +50,9 @@ void run_test(TestConnections& test, TestCase test_case) cout << test_case.name << endl; test.expect(test_case.func(test.maxscales->conn_rwsplit[0], stmt, bind), - "Test '%s' failed", - test_case.name.c_str()); + "Test '%s' failed: %s %s", test_case.name.c_str(), + mysql_error(test.maxscales->conn_rwsplit[0]), + mysql_stmt_error(stmt)); mysql_stmt_close(stmt); From a9e236497963251f8b4afa07484b88ad97e73a03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 9 Nov 2018 08:57:06 +0200 Subject: [PATCH 3/6] Fix unknown PS ID on query re-routing If a PS command is routed multiple times, the ID will not be reverted to the external ID in the failure cases. This prevented prepared statements from being re-routed correctly. --- .../readwritesplit/rwsplit_route_stmt.cc | 36 +++++++++++++------ 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc index d673de23d..a8802cc7e 100644 --- a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc +++ b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc @@ -106,6 +106,12 @@ void replace_binary_ps_id(GWBUF* buffer, uint32_t id) uint8_t* ptr = GWBUF_DATA(buffer) + MYSQL_PS_ID_OFFSET; gw_mysql_set_byte4(ptr, id); } + +uint32_t extract_binary_ps_id(GWBUF* buffer) +{ + uint8_t* ptr = GWBUF_DATA(buffer) + MYSQL_PS_ID_OFFSET; + return gw_mysql_get_byte4(ptr); +} } bool RWSplitSession::have_connected_slaves() const @@ -178,14 +184,6 @@ bool RWSplitSession::route_single_stmt(GWBUF* querybuf) uint8_t command = info.command(); uint32_t qtype = info.type_mask(); route_target_t route_target = info.target(); - bool not_locked_to_master = !is_locked_to_master(); - - if (not_locked_to_master && mxs_mysql_is_ps_command(command) && !m_qc.large_query()) - { - /** Replace the client statement ID with our internal one only if the - * target node is not the current master */ - replace_binary_ps_id(querybuf, stmt_id); - } SRWBackend target; @@ -312,7 +310,7 @@ bool RWSplitSession::route_single_stmt(GWBUF* querybuf) // Target server was found and is in the correct state succp = handle_got_target(querybuf, target, store_stmt); - if (succp && command == MXS_COM_STMT_EXECUTE && not_locked_to_master) + if (succp && command == MXS_COM_STMT_EXECUTE && !is_locked_to_master()) { /** Track the targets of the COM_STMT_EXECUTE statements. This * information is used to route all COM_STMT_FETCH commands @@ -1106,6 +1104,16 @@ bool RWSplitSession::handle_got_target(GWBUF* querybuf, SRWBackend& target, bool */ mxb_assert(target->get_reply_state() == REPLY_STATE_DONE || m_qc.large_query()); + uint32_t orig_id = 0; + + if (!is_locked_to_master() && mxs_mysql_is_ps_command(cmd) && !m_qc.large_query()) + { + // Store the original ID in case routing fails + orig_id = extract_binary_ps_id(querybuf); + // Replace the ID with our internal one, the backends will replace it with their own ID + replace_binary_ps_id(querybuf, m_qc.current_route_info().stmt_id()); + } + /** * If we are starting a new query, we use RWBackend::write, otherwise we use * RWBackend::continue_write to continue an ongoing query. RWBackend::write @@ -1156,11 +1164,17 @@ bool RWSplitSession::handle_got_target(GWBUF* querybuf, SRWBackend& target, bool { m_target_node.reset(); } - return true; } else { + if (orig_id) + { + // Put the original ID back in case we try to route the query again + replace_binary_ps_id(querybuf, orig_id); + } + MXS_ERROR("Routing query failed."); - return false; } + + return success; } From 514b5f7856fe6ba13756038b7280fd491a1e41da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 9 Nov 2018 09:26:42 +0200 Subject: [PATCH 4/6] Fix mxs359_read_only Due to the changes in the monitor, an explicit failcount=1 and extra waits are required to make sure the master actually changes. --- maxscale-system-test/cnf/maxscale.cnf.template.mxs359_read_only | 1 + maxscale-system-test/mxs359_read_only.cpp | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/maxscale-system-test/cnf/maxscale.cnf.template.mxs359_read_only b/maxscale-system-test/cnf/maxscale.cnf.template.mxs359_read_only index 45249cce3..cc1816d43 100644 --- a/maxscale-system-test/cnf/maxscale.cnf.template.mxs359_read_only +++ b/maxscale-system-test/cnf/maxscale.cnf.template.mxs359_read_only @@ -8,6 +8,7 @@ servers=server1,server2,server3,server4 user=maxskysql password=skysql monitor_interval=1000 +failcount=1 [RW-Split-Router] type=service diff --git a/maxscale-system-test/mxs359_read_only.cpp b/maxscale-system-test/mxs359_read_only.cpp index 34af9a794..7e45da89a 100644 --- a/maxscale-system-test/mxs359_read_only.cpp +++ b/maxscale-system-test/mxs359_read_only.cpp @@ -65,7 +65,7 @@ void test_new_master(TestConnections& test, std::ostream& out) test.try_query(test.maxscales->conn_rwsplit[0], "SELECT * FROM test.t1"); change_master(1, 0); - test.maxscales->wait_for_monitor(); + test.maxscales->wait_for_monitor(2); out << "Both reads and writes after master change should work" << endl; test.try_query(test.maxscales->conn_rwsplit[0], "INSERT INTO test.t1 VALUES (2)"); From 0a9d24230a03586b6cba48b088c980bc0ec2d44d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 9 Nov 2018 09:51:47 +0200 Subject: [PATCH 5/6] Fix cross-thread buffer usage If the initial handshake that is sent by the accepting thread is buffered, the subsequent flushing of it is done by the owning thread. As cross-thread buffer usage is not allowed, the initial handshake must be sent by the owning thread. --- .../MySQL/mariadbclient/mysql_client.cc | 31 +++++++------------ 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index 66c5356f1..23850111d 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -34,7 +34,7 @@ #include #include #include -#include +#include #include #include #include @@ -424,6 +424,7 @@ int MySQLSendHandshake(DCB* dcb) // writing data in the Client buffer queue dcb->func.write(dcb, buf); + protocol->protocol_auth_state = MXS_AUTH_STATE_MESSAGE_READ; return sizeof(mysql_packet_header) + mysql_payload_size; } @@ -1422,25 +1423,6 @@ int gw_MySQLAccept(DCB* listener) static void gw_process_one_new_client(DCB* client_dcb) { - MySQLProtocol* protocol; - - protocol = mysql_protocol_init(client_dcb, client_dcb->fd); - - if (protocol == NULL) - { - /** delete client_dcb */ - dcb_close(client_dcb); - MXS_ERROR("Failed to create protocol object for client connection."); - return; - } - client_dcb->protocol = protocol; - - // send handshake to the client_dcb - MySQLSendHandshake(client_dcb); - - // client protocol state change - protocol->protocol_auth_state = MXS_AUTH_STATE_MESSAGE_READ; - /** * Set new descriptor to event set. At the same time, * change state to DCB_STATE_POLLING so that @@ -1466,6 +1448,15 @@ static void gw_process_one_new_client(DCB* client_dcb) } else { + // Move the rest of the initialization process to the owning worker + mxs::RoutingWorker* worker = static_cast(client_dcb->poll.owner); + + worker->execute([=](){ + client_dcb->protocol = mysql_protocol_init(client_dcb, client_dcb->fd); + MXS_ABORT_IF_NULL(client_dcb->protocol); + MySQLSendHandshake(client_dcb); + }, mxs::RoutingWorker::EXECUTE_AUTO); + MXS_DEBUG("Added dcb %p for fd %d to epoll set.", client_dcb, client_dcb->fd); From 00eb7cb4eeecbcdcfd4c6f17ad82a71e1ad543d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 9 Nov 2018 10:10:48 +0200 Subject: [PATCH 6/6] Automatically stop secondary MaxScale If the test uses two MaxScales, they are automatically stopped after the test. This prevents the second MaxScale from interfering with subsequent tests. --- maxscale-system-test/testconnections.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/maxscale-system-test/testconnections.cpp b/maxscale-system-test/testconnections.cpp index 8611658a4..510f7456c 100644 --- a/maxscale-system-test/testconnections.cpp +++ b/maxscale-system-test/testconnections.cpp @@ -395,6 +395,11 @@ TestConnections::~TestConnections() delete galera; } + if (maxscale::multiple_maxscales) + { + maxscales->stop_all(); + } + if (global_result) { // This causes the test to fail if a core dump is found