WL#15524 Patch #2 TLS-safe upgrade of mgm socket to transporter

This changes TransporterRegistry::connect_ndb_mgmd() to return
NdbSocket rather than ndb_socket_t.

It extends the StartTls test in testMgmd to test upgrading the
TLS MGM protocol socket to a transporter.

Change-Id: Ic3b9ccf39ec78ed25705a4bbbdc5ac2953a35611

Author: jdduncan

storage/ndb/include/transporter/TransporterRegistry.hpp

@@ -262,26 +262,20 @@ class TransporterRegistry
                       bool& close_with_reset,
                       bool& log_failure);
 
-  bool connect_server(ndb_socket_t sockfd, BaseString & msg,
-                      bool & close_with_reset, bool & log_failure) {
-    NdbSocket sock(sockfd, NdbSocket::From::Existing);
-    return connect_server(sock, msg, close_with_reset, log_failure);
-  }
-
   bool connect_client(NdbMgmHandle *h);
 
   /**
    * Given a hostname and port, creates a NdbMgmHandle, turns it into
    * a transporter, and returns the socket.
    */
-  ndb_socket_t connect_ndb_mgmd(const char* server_name,
-                                unsigned short server_port);
+  NdbSocket connect_ndb_mgmd(const char* server_name,
+                             unsigned short server_port);
 
   /**
    * Given a connected NdbMgmHandle, turns it into a transporter
    * and returns the socket.
    */
-  ndb_socket_t connect_ndb_mgmd(NdbMgmHandle *h);
+  NdbSocket connect_ndb_mgmd(NdbMgmHandle *h);
 
   /**
    * Manage allTransporters and theNodeIdTransporters when using

storage/ndb/src/common/transporter/Transporter.cpp

@@ -277,11 +277,20 @@ tls_error(int code)
   g_eventLogger->error("TLS error %d '%s'", code, TlsKeyError::message(code));
 }
 
+bool
+Transporter::connect_client_mgm(int port)
+{
+  require(!isPartOfMultiTransporter());
+  NdbSocket secureSocket =
+    m_transporter_registry.connect_ndb_mgmd(remoteHostName, port);
+  return connect_client(secureSocket);
+}
+
+
 bool
 Transporter::connect_client()
 {
   NdbSocket secureSocket;
-  ndb_socket_t sockfd;
   DBUG_ENTER("Transporter::connect_client");
 
   require(!isMultiTransporter());
@@ -302,9 +311,7 @@ Transporter::connect_client()
 
   if(isMgmConnection)
   {
-    require(!isPartOfMultiTransporter());
-    sockfd= m_transporter_registry.connect_ndb_mgmd(remoteHostName, port);
-    secureSocket.init_from_new(sockfd);
+    return connect_client_mgm(port);
   }
   else
   {

storage/ndb/src/common/transporter/Transporter.hpp

@@ -133,6 +133,7 @@ class Transporter {
     NdbSocket socket(fd, NdbSocket::From::Existing);
     return connect_client(socket);
   }
+  bool connect_client_mgm(int);
   bool connect_server(NdbSocket & socket, BaseString& errormsg);
 
   /**

storage/ndb/src/common/transporter/TransporterRegistry.cpp

@@ -3784,7 +3784,8 @@ bool TransporterRegistry::connect_client(NdbMgmHandle *h)
   }
   require(!t->isMultiTransporter());
   require(!t->isPartOfMultiTransporter());
-  bool res = t->connect_client(connect_ndb_mgmd(h));
+  NdbSocket secureSocket = connect_ndb_mgmd(h);
+  bool res = t->connect_client(secureSocket);
   if (res == true)
   {
     DEBUG_FPRINTF((stderr, "(%u)performStates[%u] = DISCONNECTING,"
@@ -3836,55 +3837,53 @@ bool TransporterRegistry::report_dynamic_ports(NdbMgmHandle h) const
  * Given a connected NdbMgmHandle, turns it into a transporter
  * and returns the socket.
  */
-ndb_socket_t TransporterRegistry::connect_ndb_mgmd(NdbMgmHandle *h)
+NdbSocket TransporterRegistry::connect_ndb_mgmd(NdbMgmHandle *h)
 {
-  ndb_socket_t sockfd;
-
   DBUG_ENTER("TransporterRegistry::connect_ndb_mgmd(NdbMgmHandle)");
 
   if ( h==nullptr || *h == nullptr )
   {
     g_eventLogger->error("Mgm handle is NULL (%s:%d)", __FILE__, __LINE__);
-    DBUG_RETURN(sockfd);
+    DBUG_RETURN(NdbSocket());  // an invalid socket, newly created on the stack
   }
 
   if (!report_dynamic_ports(*h))
   {
     ndb_mgm_destroy_handle(h);
-    DBUG_RETURN(sockfd);
+    DBUG_RETURN(NdbSocket());  // an invalid socket, newly created on the stack
   }
 
   /**
    * convert_to_transporter also disposes of the handle (i.e. we don't leak
-   * memory here.
+   * memory here).
    */
   DBUG_PRINT("info", ("Converting handle to transporter"));
-  sockfd= ndb_mgm_convert_to_transporter(h);
-  if (!ndb_socket_valid(sockfd))
+  NdbSocket socket;
+  ndb_mgm_convert_to_transporter(h, &socket);
+  if (! socket.is_valid())
   {
     g_eventLogger->error("Failed to convert to transporter (%s: %d)",
                          __FILE__, __LINE__);
     ndb_mgm_destroy_handle(h);
   }
-  DBUG_RETURN(sockfd);
+  DBUG_RETURN(NdbSocket::copy(socket));
 }
 
 /**
  * Given a SocketClient, creates a NdbMgmHandle, turns it into a transporter
  * and returns the socket.
  */
-ndb_socket_t
+NdbSocket
 TransporterRegistry::connect_ndb_mgmd(const char* server_name,
                                       unsigned short server_port)
 {
   NdbMgmHandle h= ndb_mgm_create_handle();
-  ndb_socket_t s;
 
   DBUG_ENTER("TransporterRegistry::connect_ndb_mgmd(SocketClient)");
 
   if ( h == nullptr )
   {
-    DBUG_RETURN(s);
+    DBUG_RETURN(NdbSocket());
   }
 
   /**
@@ -3900,7 +3899,7 @@ TransporterRegistry::connect_ndb_mgmd(const char* server_name,
   {
     DBUG_PRINT("info", ("connection to mgmd failed"));
     ndb_mgm_destroy_handle(&h);
-    DBUG_RETURN(s);
+    DBUG_RETURN(NdbSocket());
   }
 
   DBUG_RETURN(connect_ndb_mgmd(&h));

storage/ndb/src/mgmapi/mgmapi.cpp

@@ -3743,36 +3743,28 @@ ndb_mgm_get_connection_int_parameter(NdbMgmHandle handle,
   DBUG_RETURN(res);
 }
 
-ndb_socket_t
-ndb_mgm_convert_to_transporter(NdbMgmHandle *handle)
+void
+ndb_mgm_convert_to_transporter(NdbMgmHandle *handle, NdbSocket *s)
 {
-  DBUG_ENTER("ndb_mgm_convert_to_transporter");
-  ndb_socket_t s;
-
   if(handle == nullptr)
   {
     SET_ERROR(*handle, NDB_MGM_ILLEGAL_SERVER_HANDLE, "");
-    ndb_socket_invalidate(&s);
-    DBUG_RETURN(s);
+    return;
   }
 
   if ((*handle)->connected != 1)
   {
     SET_ERROR(*handle, NDB_MGM_SERVER_NOT_CONNECTED , "");
-    ndb_socket_invalidate(&s);
-    DBUG_RETURN(s);
+    return;
   }
 
-  (*handle)->connected= 0;   // we pretend we're disconnected
-  s= (*handle)->socket.ndb_socket();
-
-  SocketOutputStream s_output(s, (*handle)->timeout);
+  NdbSocket::transfer(*s, (*handle)->socket);
+  SecureSocketOutputStream s_output(*s, (*handle)->timeout);
   s_output.println("transporter connect");
   s_output.println("%s", "");
 
+  (*handle)->connected= 0;   // The handle no longer owns the connection
   ndb_mgm_destroy_handle(handle); // set connected=0, so won't disconnect
-
-  DBUG_RETURN(s);
 }
 
 extern "C"

storage/ndb/src/mgmapi/mgmapi_internal.h

@@ -85,12 +85,12 @@ int ndb_mgm_get_connection_int_parameter(NdbMgmHandle handle,
 /**
  * Convert connection to transporter
  * @param   handle    NDB management handle.
- *
- * @return socket
+ * @param   s         Transporter socket.
  *
  * @note the socket is now able to be used as a transporter connection
+ * @note the management handle is no longer valid after this call
  */
-ndb_socket_t ndb_mgm_convert_to_transporter(NdbMgmHandle *handle);
+void ndb_mgm_convert_to_transporter(NdbMgmHandle *handle, class NdbSocket * s);
 
 int ndb_mgm_disconnect_quiet(NdbMgmHandle handle);
 

storage/ndb/src/mgmsrv/MgmtSrvr.cpp

@@ -5121,14 +5121,14 @@ MgmtSrvr::getConnectionDbParameter(int node1, int node2,
 
 
 bool
-MgmtSrvr::transporter_connect(ndb_socket_t sockfd,
+MgmtSrvr::transporter_connect(NdbSocket & socket,
                               BaseString& msg,
                               bool& close_with_reset,
                               bool& log_failure)
 {
   DBUG_ENTER("MgmtSrvr::transporter_connect");
   TransporterRegistry* tr= theFacade->get_registry();
-  if (!tr->connect_server(sockfd, msg, close_with_reset, log_failure))
+  if (!tr->connect_server(socket, msg, close_with_reset, log_failure))
     DBUG_RETURN(false);
 
   /**

storage/ndb/src/mgmsrv/MgmtSrvr.hpp

@@ -359,7 +359,7 @@ class MgmtSrvr : private ConfigSubscriber, public trp_client {
   int getConnectionDbParameter(int node1, int node2, int param,
 			       int *value, BaseString& msg);
 
-  bool transporter_connect(ndb_socket_t sockfd,
+  bool transporter_connect(NdbSocket & socket,
                            BaseString& errormsg,
                            bool& close_with_reset,
                            bool& log_failure);

storage/ndb/src/mgmsrv/Services.cpp

@@ -1930,8 +1930,8 @@ MgmApiSession::transporter_connect(Parser_t::Context &ctx,
   bool close_with_reset = true;
   bool log_failure = false;
   BaseString errormsg;
-  if (!m_mgmsrv.transporter_connect(m_secure_socket.ndb_socket(), errormsg,
-                                    close_with_reset, log_failure))
+  if (!m_mgmsrv.transporter_connect(m_secure_socket,
+                                    errormsg, close_with_reset, log_failure))
   {
     // Connection not allowed or failed
     if (log_failure)
@@ -1953,7 +1953,7 @@ MgmApiSession::transporter_connect(Parser_t::Context &ctx,
       but don't close the socket, it's been taken over
       by the transporter
     */
-    m_secure_socket.invalidate();  // so nobody closes it
+    NdbSocket s = NdbSocket::transfer(m_secure_socket);
   }
 
   m_stop= true; // Stop the session

storage/ndb/test/include/NdbMgmd.hpp

@@ -104,6 +104,10 @@ class NdbMgmd {
     return m_handle;
   }
 
+  void convert_to_transporter(NdbSocket * s) {
+    ndb_mgm_convert_to_transporter(& m_handle, s);
+  }
+
   ndb_socket_t socket(void) const {
     return _ndb_mgm_get_socket(m_handle);
   }

storage/ndb/test/ndbapi/testMgmd.cpp

@@ -342,6 +342,10 @@ class Mgmd
 
   NdbMgmHandle handle() { return m_mgmd_client.handle(); }
 
+  void convert_to_transporter(NdbSocket * sock) {
+    m_mgmd_client.convert_to_transporter(sock);
+  }
+
 private:
 
   bool get_section_string(const Properties& config,
@@ -2024,6 +2028,7 @@ runTestNdbdWithCert(NDBT_Context* ctx, NDBT_Step* step)
   CHECK(ndbd.wait_started(handle));
 
   CHECK(mgmd.stop());
+  CHECK(ndbd.stop());
   return NDBT_OK;
 }
 
@@ -2082,6 +2087,11 @@ int runTestStartTls(NDBT_Context* ctx, NDBT_Step* step)
   struct ndb_mgm_cluster_state *state = ndb_mgm_get_status(mgmd.handle());
   CHECK(state != nullptr);
 
+  /* Now convert the socket to a transporter */
+  NdbSocket s;
+  mgmd.convert_to_transporter(&s);
+  CHECK(s.is_valid());
+
   return NDBT_OK;
 }