Merge branch '2.3' into develop

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
+From MaxScale 2.3 onwards, the masking filter will reject statements
-access to a particular column. As the masking filter works on the column
+that use functions in conjunction with columns that should be masked.
-name alone a query like
+Allowing function usage provides a way for circumventing the masking,
-```
+unless a firewall filter is separately configured and installed.
-> 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.
 
-For a secure solution, the masking filter *must* be combined with the
+Please see the configuration parameter
-firewall filter to prevent the use of functions using which the masking
+[prevent_function_usage](#prevent_function_usage)
-can be bypassed.
+for how to change the default behaviour.
-
-In a future release, the combined use of the masking filter and the
-database firewall filter will be simplified.
 
 ## Limitations
 

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

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 '%s' failed: %s %s", test_case.name.c_str(),
-                test_case.name.c_str());
+                mysql_error(test.maxscales->conn_rwsplit[0]),
+                mysql_stmt_error(stmt));
 
     mysql_stmt_close(stmt);
 

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)");

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

server/modules/protocol/MySQL/mariadbclient/mysql_client.cc

@@ -34,7 +34,7 @@
 #include <maxscale/protocol/mysql.h>
 #include <maxscale/query_classifier.h>
 #include <maxscale/router.h>
-#include <maxscale/routingworker.h>
+#include <maxscale/routingworker.hh>
 #include <maxscale/session.h>
 #include <maxscale/ssl.h>
 #include <maxscale/utils.h>
@@ -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<mxs::RoutingWorker*>(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);

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;
 }