TLS: Fixed issues found during PR review

Author: Enmk

.github/workflows/linux_ssl.yml

@@ -12,5 +12,5 @@ jobs:
     uses: Enmk/clickhouse-cpp/.github/workflows/linux.yml@master
     with:
       extra_cmake_flags: -DWITH_OPENSSL=ON
-      extra_install: libssl1.1 libssl-dev
-      gtest_args: --gtest_filter="-*LocalhostTLS*"
+      extra_install: libssl-dev
+#      gtest_args: --gtest_filter="-*LocalhostTLS*"

CMakeLists.txt

@@ -4,30 +4,30 @@ INCLUDE (cmake/cpp17.cmake)
 INCLUDE (cmake/subdirs.cmake)
 INCLUDE (cmake/openssl.cmake)
 
-OPTION(BUILD_BENCHMARK "Build benchmark" OFF)
-OPTION(BUILD_TESTS "Build tests" OFF)
-OPTION(WITH_OPENSSL "Use OpenSSL for TLS connections" OFF)
+OPTION (BUILD_BENCHMARK "Build benchmark" OFF)
+OPTION (BUILD_TESTS "Build tests" OFF)
+OPTION (WITH_OPENSSL "Use OpenSSL for TLS connections" OFF)
 
 PROJECT (CLICKHOUSE-CLIENT)
 
     USE_CXX17()
     USE_OPENSSL()
 
     IF ("${CMAKE_BUILD_TYPE}" STREQUAL "")
-        set(CMAKE_BUILD_TYPE "Debug")
+        SET (CMAKE_BUILD_TYPE "Debug")
     ENDIF()
 
     IF (UNIX)
         IF (APPLE)
             SET (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O2 -Wall -Wextra -Werror")
         ELSE ()
-            SET (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O2 -pthread -Wall -Wextra -Werror")
+            SET (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -pthread -Wall -Wextra -Werror")
         ENDIF ()
         SET (CMAKE_EXE_LINKER_FLAGS, "${CMAKE_EXE_LINKER_FLAGS} -lpthread")
     ENDIF ()
 
-    INCLUDE_DIRECTORIES(.)
-    INCLUDE_DIRECTORIES(contrib)
+    INCLUDE_DIRECTORIES (.)
+    INCLUDE_DIRECTORIES (contrib)
 
     SUBDIRS (
         clickhouse

clickhouse/CMakeLists.txt

@@ -1,10 +1,10 @@
 SET ( clickhouse-cpp-lib-src
+    base/coded.cpp
     base/compressed.cpp
     base/input.cpp
     base/output.cpp
     base/platform.cpp
     base/socket.cpp
-    base/coded.cpp
 
     columns/array.cpp
     columns/date.cpp
@@ -31,9 +31,9 @@ SET ( clickhouse-cpp-lib-src
     query.cpp
 )
 
-if (WITH_OPENSSL)
+IF (WITH_OPENSSL)
     LIST(APPEND clickhouse-cpp-lib-src base/sslsocket.cpp)
-endif()
+ENDIF ()
 
 ADD_LIBRARY (clickhouse-cpp-lib SHARED ${clickhouse-cpp-lib-src})
 SET_TARGET_PROPERTIES(clickhouse-cpp-lib PROPERTIES LINKER_LANGUAGE CXX)
@@ -56,7 +56,7 @@ IF (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
     TARGET_LINK_LIBRARIES (clickhouse-cpp-lib-static gcc_s)
 ENDIF ()
 
-INSTALL(TARGETS clickhouse-cpp-lib clickhouse-cpp-lib-static
+INSTALL (TARGETS clickhouse-cpp-lib clickhouse-cpp-lib-static
     ARCHIVE DESTINATION lib
     LIBRARY DESTINATION lib
 )
@@ -71,6 +71,7 @@ INSTALL(FILES protocol.h DESTINATION include/clickhouse/)
 INSTALL(FILES query.h DESTINATION include/clickhouse/)
 
 # base
+INSTALL(FILES base/coded.h DESTINATION include/clickhouse/base/)
 INSTALL(FILES base/buffer.h DESTINATION include/clickhouse/base/)
 INSTALL(FILES base/compressed.h DESTINATION include/clickhouse/base/)
 INSTALL(FILES base/input.h DESTINATION include/clickhouse/base/)
@@ -104,7 +105,7 @@ INSTALL(FILES columns/uuid.h DESTINATION include/clickhouse/columns/)
 INSTALL(FILES types/type_parser.h DESTINATION include/clickhouse/types/)
 INSTALL(FILES types/types.h DESTINATION include/clickhouse/types/)
 
-if (WITH_OPENSSL)
-    target_link_libraries(clickhouse-cpp-lib OpenSSL::SSL)
-    target_link_libraries(clickhouse-cpp-lib-static OpenSSL::SSL)
-endif()
+IF (WITH_OPENSSL)
+    TARGET_LINK_LIBRARIES (clickhouse-cpp-lib OpenSSL::SSL)
+    TARGET_LINK_LIBRARIES (clickhouse-cpp-lib-static OpenSSL::SSL)
+ENDIF ()

clickhouse/base/socket.cpp

@@ -131,8 +131,8 @@ SOCKET SocketConnect(const NetworkAddress& addr) {
 } // namespace
 
 NetworkAddress::NetworkAddress(const std::string& host, const std::string& port)
-    : host_(host),
-      info_(nullptr)
+    : host_(host)
+    , info_(nullptr)
 {
     struct addrinfo hints;
     memset(&hints, 0, sizeof(hints));

clickhouse/base/socket.h

@@ -30,7 +30,7 @@ struct addrinfo;
 
 namespace clickhouse {
 
-/**
+/** Address of a host to establish connection to.
  *
  */
 class NetworkAddress {

clickhouse/base/sslsocket.cpp

@@ -7,37 +7,43 @@
 #include <openssl/err.h>
 #include <openssl/asn1.h>
 
-#include <iostream>
 
 namespace {
 
 std::string getCertificateInfo(X509* cert)
 {
+    if (!cert)
+        return "No certificate";
+
     std::unique_ptr<BIO, decltype(&BIO_free)> mem_bio(BIO_new(BIO_s_mem()), &BIO_free);
     X509_print(mem_bio.get(), cert);
+
     char * data = nullptr;
-    size_t len = BIO_get_mem_data(mem_bio.get(), &data);
+    auto len = BIO_get_mem_data(mem_bio.get(), &data);
+    if (len < 0)
+        return "Can't get certificate info due to BIO error " + std::to_string(len);
 
     return std::string(data, len);
 }
 
-void throwSSLError(SSL * ssl, int error, const char * location, const char * statement) {
+void throwSSLError(SSL * ssl, int error, const char * /*location*/, const char * /*statement*/) {
     const auto detail_error = ERR_get_error();
     auto reason = ERR_reason_error_string(detail_error);
     reason = reason ? reason : "Unknown SSL error";
 
     std::string reason_str = reason;
     if (ssl) {
-        if (auto ssl_session = SSL_get_session(ssl))
-            if (auto server_certificate = SSL_SESSION_get0_peer(ssl_session))
-                reason_str += "\nServer certificate: " + getCertificateInfo(server_certificate);
+        // TODO: maybe print certificate only if handshake isn't completed (SSL_get_state(ssl) != TLS_ST_OK)
+        if (auto ssl_session = SSL_get_session(ssl)) {
+            reason_str += "\nServer certificate: " + getCertificateInfo(SSL_SESSION_get0_peer(ssl_session));
+        }
     }
 
-    std::cerr << "!!! SSL error at " << location
-              << "\n\tcaused by " << statement
-              << "\n\t: "<< reason_str << "(" << error << ")"
-              << "\n\t last err: " << ERR_peek_last_error()
-              << std::endl;
+//    std::cerr << "!!! SSL error at " << location
+//              << "\n\tcaused by " << statement
+//              << "\n\t: "<< reason_str << "(" << error << ")"
+//              << "\n\t last err: " << ERR_peek_last_error()
+//              << std::endl;
 
     throw std::runtime_error(std::string("OpenSSL error: ") + std::to_string(error) + " : " + reason_str);
 }
@@ -64,8 +70,8 @@ SSL_CTX * prepareSSLContext(const clickhouse::SSLParams & context_params) {
         throw std::runtime_error("Failed to initialize SSL context");
 
 #define HANDLE_SSL_CTX_ERROR(statement) do { \
-    if (const auto ret_code = statement; !ret_code) \
-        throwSSLError(nullptr, ERR_peek_error(), LOCATION, STRINGIFY(statement)); \
+    if (const auto ret_code = (statement); !ret_code) \
+        throwSSLError(nullptr, ERR_peek_error(), LOCATION, #statement); \
 } while(false);
 
     if (context_params.use_default_ca_locations)
@@ -91,47 +97,48 @@ SSL_CTX * prepareSSLContext(const clickhouse::SSLParams & context_params) {
             SSL_CTX_set_max_proto_version(ctx.get(), context_params.max_protocol_version));
 
     return ctx.release();
+#undef HANDLE_SSL_CTX_ERROR
 }
 
-
-
 }
 
-#define HANDLE_SSL_ERROR(statement) do { \
-    if (const auto ret_code = statement; ret_code <= 0) \
-        throwSSLError(ssl_, SSL_get_error(ssl_, ret_code), LOCATION, STRINGIFY(statement)); \
-} while(false);
-
 namespace clickhouse {
 
 SSLContext::SSLContext(SSL_CTX & context)
-    : context_(&context)
+    : context_(&context, &SSL_CTX_free)
 {
-    SSL_CTX_up_ref(context_);
+    SSL_CTX_up_ref(context_.get());
 }
 
 SSLContext::SSLContext(const SSLParams & context_params)
-    : context_(prepareSSLContext(context_params))
+    : context_(prepareSSLContext(context_params), &SSL_CTX_free)
 {
 }
 
-SSLContext::~SSLContext() {
-    SSL_CTX_free(context_);
-}
-
 SSL_CTX * SSLContext::getContext() {
-    return context_;
+    return context_.get();
 }
 
+// Allows caller to use returned value of `statement` if there was no error, throws exception otherwise.
+#define HANDLE_SSL_ERROR(statement) [&] { \
+    if (const auto ret_code = (statement); ret_code <= 0) { \
+        throwSSLError(ssl_, SSL_get_error(ssl_, ret_code), LOCATION, #statement); \
+        return static_cast<decltype(ret_code)>(0); \
+    } \
+    else \
+        return ret_code; \
+}()
+
 /* // debug macro for tracing SSL state
 #define LOG_SSL_STATE() std::cerr << "!!!!" << LOCATION << " @" << __FUNCTION__ \
     << "\t" << SSL_get_version(ssl_) << " state: "  << SSL_state_string_long(ssl_) \
     << "\n\t handshake state: " << SSL_get_state(ssl_) \
     << std::endl
 */
 SSLSocket::SSLSocket(const NetworkAddress& addr, const SSLParams & ssl_params, SSLContext& context)
-    : Socket(addr),
-    ssl_(SSL_new(context.getContext()))
+    : Socket(addr)
+    , ssl_ptr_(SSL_new(context.getContext()), &SSL_free)
+    , ssl_(ssl_ptr_.get())
 {
     if (!ssl_)
         throw std::runtime_error("Failed to create SSL instance");
@@ -143,41 +150,37 @@ SSLSocket::SSLSocket(const NetworkAddress& addr, const SSLParams & ssl_params, S
     SSL_set_connect_state(ssl_);
     HANDLE_SSL_ERROR(SSL_connect(ssl_));
     HANDLE_SSL_ERROR(SSL_set_mode(ssl_, SSL_MODE_AUTO_RETRY));
+    auto peer_certificate = SSL_get_peer_certificate(ssl_);
 
-    if(const auto verify_result = SSL_get_verify_result(ssl_); verify_result != X509_V_OK) {
-        auto error_message = X509_verify_cert_error_string(verify_result);
-        auto ssl_session = SSL_get_session(ssl_);
-        auto cert = SSL_SESSION_get0_peer(ssl_session);
+    if (!peer_certificate)
+        throw std::runtime_error("Failed to verify SSL connection: server provided no ceritificate.");
 
+    if (const auto verify_result = SSL_get_verify_result(ssl_); verify_result != X509_V_OK) {
+        auto error_message = X509_verify_cert_error_string(verify_result);
         throw std::runtime_error("Failed to verify SSL connection, X509_v error: "
-                                 + std::to_string(verify_result)
-                                 + " " + error_message + "\n" + getCertificateInfo(cert));
+                + std::to_string(verify_result)
+                + " " + error_message
+                + "\nServer certificate: " + getCertificateInfo(peer_certificate));
     }
 
     if (ssl_params.use_SNI) {
-        auto ssl_session = SSL_get_session(ssl_);
-        auto peer_cert = SSL_SESSION_get0_peer(ssl_session);
         auto hostname = addr.Host();
         char * out_name = nullptr;
 
         std::unique_ptr<ASN1_OCTET_STRING, decltype(&ASN1_OCTET_STRING_free)> addr(a2i_IPADDRESS(hostname.c_str()), &ASN1_OCTET_STRING_free);
         if (addr) {
             // if hostname is actually an IP address
             HANDLE_SSL_ERROR(X509_check_ip(
-                    peer_cert,
+                    peer_certificate,
                     ASN1_STRING_get0_data(addr.get()),
                     ASN1_STRING_length(addr.get()),
                     0));
         } else {
-            HANDLE_SSL_ERROR(X509_check_host(peer_cert, hostname.c_str(), hostname.length(), 0, &out_name));
+            HANDLE_SSL_ERROR(X509_check_host(peer_certificate, hostname.c_str(), hostname.length(), 0, &out_name));
         }
     }
 }
 
-SSLSocket::~SSLSocket() {
-    SSL_free(ssl_);
-}
-
 std::unique_ptr<InputStream> SSLSocket::makeInputStream() const {
     return std::make_unique<SSLSocketInput>(ssl_);
 }
@@ -190,8 +193,6 @@ SSLSocketInput::SSLSocketInput(SSL *ssl)
     : ssl_(ssl)
 {}
 
-SSLSocketInput::~SSLSocketInput() = default;
-
 size_t SSLSocketInput::DoRead(void* buf, size_t len) {
     size_t actually_read;
     HANDLE_SSL_ERROR(SSL_read_ex(ssl_, buf, len, &actually_read));
@@ -202,8 +203,6 @@ SSLSocketOutput::SSLSocketOutput(SSL *ssl)
     : ssl_(ssl)
 {}
 
-SSLSocketOutput::~SSLSocketOutput() = default;
-
 void SSLSocketOutput::DoWrite(const void* data, size_t len) {
     HANDLE_SSL_ERROR(SSL_write(ssl_, data, len));
 }

clickhouse/base/sslsocket.h

@@ -2,6 +2,8 @@
 
 #include "socket.h"
 
+#include <memory>
+
 typedef struct ssl_ctx_st SSL_CTX;
 typedef struct ssl_st SSL;
 
@@ -23,7 +25,7 @@ class SSLContext
 public:
     explicit SSLContext(SSL_CTX & context);
     explicit SSLContext(const SSLParams & context_params);
-    ~SSLContext();
+    ~SSLContext() = default;
 
     SSLContext(const SSLContext &) = delete;
     SSLContext& operator=(const SSLContext &) = delete;
@@ -35,14 +37,14 @@ class SSLContext
     SSL_CTX * getContext();
 
 private:
-    SSL_CTX * const context_;
+    std::unique_ptr<SSL_CTX, void (*)(SSL_CTX*)> context_;
 };
 
 class SSLSocket : public Socket {
 public:
     explicit SSLSocket(const NetworkAddress& addr, const SSLParams & ssl_params, SSLContext& context);
     SSLSocket(SSLSocket &&) = default;
-    ~SSLSocket();
+    ~SSLSocket() = default;
 
     SSLSocket(const SSLSocket & ) = delete;
     SSLSocket& operator=(const SSLSocket & ) = delete;
@@ -51,30 +53,33 @@ class SSLSocket : public Socket {
     std::unique_ptr<OutputStream> makeOutputStream() const override;
 
 private:
-    SSL *ssl_;
+    std::unique_ptr<SSL, void (*)(SSL *s)> ssl_ptr_;
+    SSL *ssl_; // for convinience with SSL API
 };
 
 class SSLSocketInput : public InputStream {
 public:
     explicit SSLSocketInput(SSL *ssl);
-    ~SSLSocketInput();
+    ~SSLSocketInput() = default;
 
 protected:
     size_t DoRead(void* buf, size_t len) override;
 
 private:
+    // Not owning
     SSL *ssl_;
 };
 
 class SSLSocketOutput : public OutputStream {
 public:
     explicit SSLSocketOutput(SSL *ssl);
-    ~SSLSocketOutput();
+    ~SSLSocketOutput() = default;
 
 protected:
     void DoWrite(const void* data, size_t len) override;
 
 private:
+    // Not owning
     SSL *ssl_;
 };