Fix ConnServer stall

Author: luisremis

include/comm/ConnServer.h

@@ -88,6 +88,8 @@ namespace comm {
 
         std::unique_ptr<Connection> accept();
 
+        std::unique_ptr<Connection> negotiate_protocol(std::shared_ptr<Connection> conn);
+
     private:
 
         ConnServerConfig _config;

include/comm/Protocol.h

@@ -8,7 +8,14 @@
 
 namespace comm {
 
-    ENUM_FLAGS(Protocol, uint8_t)
+    // We are setting Protocol to be uint32_t here because
+    // otherwise there will be padding, and we
+    // observed that sometimes that padding is causing issues
+    // on the client side when decoding the protocol.
+    // TODO: Make HelloMessage and Protocol a Protobuf message.
+    // to prevent this kind of issues, and to prevent
+    // potential issues with endianness.
+    ENUM_FLAGS(Protocol, uint32_t)
     {
         None = 0,
         TCP = 1 << 0,

include/util/Utils.h

@@ -0,0 +1,21 @@
+/**
+ *
+ * @copyright Copyright (c) 2022 ApertureData Inc.
+ *
+ */
+
+#pragma once
+
+#include <util/TypeName.h>
+#include <iostream>
+
+template<typename T>
+void print_binary_data(const T& data)
+{
+    std::cout << "sizeof(" << type_name(data) << "): " << sizeof(T) << std::endl;
+    for (int i = 0; i < sizeof(T); ++i)
+    {
+        printf("%02X ", reinterpret_cast<uint8_t *>(&data)[i] & 0xff);
+    }
+    std::cout << std::endl;
+}

src/comm/ConnServer.cc

@@ -114,53 +114,74 @@ ConnServer::ConnServer(int port, ConnServerConfig config) :
 
 ConnServer::~ConnServer() = default;
 
-std::unique_ptr<Connection> ConnServer::accept()
+// c contains a TCPConnection, unencrypted, connection to a client.
+// The ConnServer will implement the protocol negotiation.
+// This right now is a simple handshake, design for ApertureDB Server use-case.
+// This protocol can be a virtual method in the future to support arbitrary protocols.
+std::unique_ptr<Connection> ConnServer::negotiate_protocol(std::shared_ptr<Connection> conn)
 {
-    auto connected_socket = TCPSocket::accept(_listening_socket);
-
-    auto tcp_connection = std::unique_ptr<TCPConnection>(
-        new TCPConnection(std::move(connected_socket), _config.metrics));
+    auto tcp_connection = std::static_pointer_cast<TCPConnection>(conn);
 
     auto response = tcp_connection->recv_message();
 
-    if (response.length() != sizeof(HelloMessage)) {
+    if (response.length() != sizeof(HelloMessage))
+    {
         THROW_EXCEPTION(ProtocolError);
     }
 
     auto client_hello_message = reinterpret_cast<const HelloMessage*>(response.data());
 
     HelloMessage server_hello_message;
 
-    if (client_hello_message->version != PROTOCOL_VERSION) {
+    if (client_hello_message->version != PROTOCOL_VERSION)
+    {
         server_hello_message.version = 0;
         server_hello_message.protocol = Protocol::None;
     }
-    else {
+    else
+    {
         server_hello_message.version = PROTOCOL_VERSION;
         server_hello_message.protocol = client_hello_message->protocol & _config.allowed_protocols;
     }
 
-    tcp_connection->send_message(reinterpret_cast<uint8_t*>(&server_hello_message), sizeof(server_hello_message));
+    tcp_connection->send_message(
+        reinterpret_cast<uint8_t *>(&server_hello_message),
+        sizeof(server_hello_message));
 
-    if (server_hello_message.version == 0) {
+    if (server_hello_message.version == 0)
+    {
         THROW_EXCEPTION(ProtocolError, "Protocol version mismatch");
     }
 
-    if ((server_hello_message.protocol & Protocol::TLS) == Protocol::TLS) {
+    if ((server_hello_message.protocol & Protocol::TLS) == Protocol::TLS)
+    {
         auto tcp_socket = tcp_connection->release_socket();
 
         auto tls_socket = TLSSocket::create(std::move(tcp_socket), _ssl_ctx);
 
         tls_socket->accept();
 
-        return std::unique_ptr<TLSConnection>(
-            new TLSConnection(std::move(tls_socket), _config.metrics));
+        return std::make_unique<TLSConnection>(std::move(tls_socket), _config.metrics);
     }
-    else if ((server_hello_message.protocol & Protocol::TCP) == Protocol::TCP) {
+    else if ((server_hello_message.protocol & Protocol::TCP) == Protocol::TCP)
+    {
+        auto tcp_socket = tcp_connection->release_socket();
         // Nothing to do, already using TCP
-        return tcp_connection;
+        // return tcp_connection;
+        return std::make_unique<TCPConnection>(std::move(tcp_socket), _config.metrics);
     }
-    else {
+    else
+    {
         THROW_EXCEPTION(ProtocolError, "Protocol negotiation failed");
     }
 }
+
+std::unique_ptr<Connection> ConnServer::accept()
+{
+    auto connected_socket = TCPSocket::accept(_listening_socket);
+
+    auto tcp_connection = std::make_unique<TCPConnection>(
+                                std::move(connected_socket), _config.metrics);
+
+    return tcp_connection;
+}

src/comm/Connection.cc

@@ -39,7 +39,6 @@ Connection::Connection(ConnMetrics* metrics)
 : _max_buffer_size(DEFAULT_BUFFER_SIZE)
 , _metrics(metrics)
 {
-
 }
 
 Connection::~Connection() = default;

src/comm/TCPSocket.cc

@@ -57,6 +57,7 @@ std::unique_ptr<TCPSocket> TCPSocket::accept(const std::unique_ptr<TCPSocket>& l
 
     errno = 0;
     int connected_socket = ::accept(listening_socket->_socket_fd, reinterpret_cast<sockaddr*>(&clnt_addr), &len);
+
     int errno_r = errno;
     if (connected_socket < 0) {
         if (errno_r == EAGAIN) {

src/comm/TLSSocket.cc

@@ -3,6 +3,7 @@
  */
 
 #include "comm/TLSSocket.h"
+#include <openssl/err.h>
 
 #include <cstring>
 #include <netdb.h>
@@ -38,6 +39,7 @@ void TLSSocket::accept()
     errno = 0;
     auto result = ::SSL_accept(_ssl);
     int errno_r = errno;
+    ERR_print_errors_fp(stdout);
     if (result < 1) {
         THROW_EXCEPTION(TLSError, errno_r, "SSL_accept()", result);
     }

src/comm/TLSSocket.h

@@ -34,6 +34,9 @@ namespace comm {
         explicit TLSSocket(std::unique_ptr<TCPSocket> tcp_socket, SSL* ssl);
 
         SSL* _ssl{nullptr};
+
+        // Even if this member is not used, it is necessary to keep it alive
+        // until the destructor is called.
         std::unique_ptr<TCPSocket> _tcp_socket;
     };
 

test/AuthEnabledVDMSServer.cc

@@ -38,7 +38,7 @@ AuthEnabledVDMSServer::AuthEnabledVDMSServer(int port, AuthEnabledVDMSServerConf
 {
     auto thread_function = [&]()
     {
-        std::shared_ptr<comm::Connection> server_conn = _server.accept();
+        std::shared_ptr<comm::Connection> server_conn = _server.negotiate_protocol(_server.accept());
 
         while (!_stop_signal) {
             protobufs::queryMessage protobuf_request;

test/TCPConnectionTests.cc

@@ -33,7 +33,7 @@ TEST(TCPConnectionTests, SyncMessages)
 
         barrier.wait();
 
-        auto server_conn = server.accept();
+        auto server_conn = server.negotiate_protocol(server.accept());
 
         for (int i = 0; i < NUMBER_OF_MESSAGES; ++i) {
             //Recieve something
@@ -81,7 +81,7 @@ TEST(TCPConnectionTests, AsyncMessages)
 
         barrier.wait();
 
-        auto server_conn = server.accept();
+        auto server_conn = server.negotiate_protocol(server.accept());
 
         for (int i = 0; i < NUMBER_OF_MESSAGES; ++i) {
             //Send something
@@ -130,7 +130,7 @@ TEST(TCPConnectionTests, ServerShutdownRecv)
 
         barrier.wait();
 
-        server.accept();
+        auto server_conn = server.negotiate_protocol(server.accept());
     });
 
     comm::ConnClient conn_client({"localhost", SERVER_PORT_INTERCHANGE});
@@ -159,7 +159,7 @@ TEST(TCPConnectionTests, SendArrayInts)
 
         barrier.wait();
 
-        auto server_conn = server.accept();
+        auto server_conn = server.negotiate_protocol(server.accept());
 
         server_conn->send_message(reinterpret_cast<const uint8_t*>(arr), sizeof(arr));
     });