fix: Shared SSL context (#3137)

src/app/ClioApplication.cpp

@@ -22,6 +22,7 @@
 #include "util/build/Build.hpp"
 #include "util/config/ConfigDefinition.hpp"
 #include "util/log/Logger.hpp"
+#include "util/requests/SslContext.hpp"
 #include "web/AdminVerificationStrategy.hpp"
 #include "web/RPCServerHandler.hpp"
 #include "web/Server.hpp"
@@ -79,6 +80,13 @@ ClioApplication::ClioApplication(util::config::ClioConfigDefinition const& confi
 int
 ClioApplication::run(bool const useNgWebServer)
 {
+    if (auto const sslContext = util::requests::initClientSslContext();
+        not sslContext.has_value()) {
+        LOG(util::LogService::fatal())
+            << "Failed to create client SSL context: " << sslContext.error();
+        return EXIT_FAILURE;
+    }
+
     auto const threads = config_.get<uint16_t>("io_threads");
     LOG(util::LogService::info()) << "Number of io threads = " << threads;
 

src/util/CMakeLists.txt

@@ -21,6 +21,7 @@ target_sources(
         Retry.cpp
         Repeat.cpp
         requests/RequestBuilder.cpp
+        requests/SslContext.cpp
         requests/Types.cpp
         requests/WsConnection.cpp
         requests/impl/SslContext.cpp

src/util/requests/RequestBuilder.cpp

@@ -117,18 +117,16 @@ std::expected<std::string, RequestError>
 RequestBuilder::doSslRequest(asio::yield_context yield, beast::http::verb method)
 {
     auto streamData = impl::SslTcpStreamData::create(yield);
-    if (not streamData.has_value())
-        return std::unexpected{std::move(streamData).error()};
 
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wold-style-cast"
-    if (!SSL_set_tlsext_host_name(streamData->stream.native_handle(), host_.c_str())) {
+    if (!SSL_set_tlsext_host_name(streamData.stream.native_handle(), host_.c_str())) {
 #pragma GCC diagnostic pop
         beast::error_code errorCode;
         errorCode.assign(static_cast<int>(::ERR_get_error()), asio::error::get_ssl_category());
         return std::unexpected{RequestError{"SSL setup failed", errorCode}};
     }
-    return doRequestImpl(std::move(streamData).value(), yield, method);
+    return doRequestImpl(std::move(streamData), yield, method);
 }
 
 std::expected<std::string, RequestError>

src/util/requests/SslContext.cpp

@@ -0,0 +1,16 @@
+#include "util/requests/SslContext.hpp"
+
+#include "util/requests/impl/SslContext.hpp"
+
+#include <expected>
+#include <string>
+
+namespace util::requests {
+
+std::expected<void, std::string>
+initClientSslContext()
+{
+    return impl::initClientSslContext();
+}
+
+}  // namespace util::requests

src/util/requests/SslContext.hpp

@@ -0,0 +1,21 @@
+#pragma once
+
+#include <expected>
+#include <string>
+
+namespace util::requests {
+
+/**
+ * @brief Create and cache the shared client SSL context.
+ *
+ * Intended to be called once during application startup so that an unrecoverable SSL
+ * misconfiguration (e.g. a missing system root certificate bundle) is reported immediately instead
+ * of causing every outgoing request to fail later. The context (including the potentially expensive
+ * parse of the root certificate bundle) is created once and reused for the lifetime of the process.
+ *
+ * @return An error message if the context could not be created
+ */
+std::expected<void, std::string>
+initClientSslContext();
+
+}  // namespace util::requests

src/util/requests/WsConnection.cpp

@@ -80,12 +80,10 @@ std::expected<WsConnectionPtr, RequestError>
 WsConnectionBuilder::sslConnect(asio::yield_context yield) const
 {
     auto streamData = impl::SslWsStreamData::create(yield);
-    if (not streamData.has_value())
-        return std::unexpected{std::move(streamData).error()};
 
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wold-style-cast"
-    if (!SSL_set_tlsext_host_name(streamData->stream.next_layer().native_handle(), host_.c_str())) {
+    if (!SSL_set_tlsext_host_name(streamData.stream.next_layer().native_handle(), host_.c_str())) {
 #pragma GCC diagnostic pop
         beast::error_code errorCode;
         errorCode.assign(
@@ -93,7 +91,7 @@ WsConnectionBuilder::sslConnect(asio::yield_context yield) const
         );
         return std::unexpected{RequestError{"SSL setup failed", errorCode}};
     }
-    return connectImpl(std::move(streamData).value(), yield);
+    return connectImpl(std::move(streamData), yield);
 }
 
 std::expected<WsConnectionPtr, RequestError>

src/util/requests/impl/SslContext.cpp

@@ -47,55 +47,80 @@ constexpr std::array kCertFilePaths{
 std::optional<std::string>
 readCertificateFile(std::filesystem::path const& path)
 {
-    if (not std::filesystem::exists(path)) {
+    if (not std::filesystem::exists(path))
         return std::nullopt;
-    }
+
     std::ifstream const fileStream{path, std::ios::in};
-    if (not fileStream.is_open()) {
+    if (not fileStream.is_open())
         return std::nullopt;
-    }
+
     std::stringstream buffer;
     buffer << fileStream.rdbuf();
+
     return std::move(buffer).str();
 }
 
-std::expected<std::string, RequestError>
+std::optional<std::string>
 getRootCertificate()
 {
     // Honor the OpenSSL-standard SSL_CERT_FILE environment variable first. Some
     // environments (e.g. the Nix-based CI/runtime image) point it at their CA
     // bundle instead of installing certificates at the well-known system paths.
     if (char const* const certFile = std::getenv("SSL_CERT_FILE"); certFile != nullptr) {
-        if (auto contents = readCertificateFile(certFile); contents.has_value()) {
-            return *std::move(contents);
-        }
+        if (auto contents = readCertificateFile(certFile); contents.has_value())
+            return contents;
     }
 
     for (auto const& path : kCertFilePaths) {
-        if (auto contents = readCertificateFile(path); contents.has_value()) {
-            return *std::move(contents);
-        }
+        if (auto contents = readCertificateFile(path); contents.has_value())
+            return contents;
     }
-    return std::unexpected{RequestError{"SSL setup failed: could not find root certificate"}};
+
+    return std::nullopt;
+}
+
+std::expected<ssl::context, RequestError>&
+cachedClientSslContext()
+{
+    static std::expected<ssl::context, RequestError> context =
+        makeClientSslContext(getRootCertificate());
+    return context;
 }
 
 }  // namespace
 
-std::expected<boost::asio::ssl::context, RequestError>
-makeClientSslContext()
+std::expected<ssl::context, RequestError>
+makeClientSslContext(std::optional<std::string> const& rootCertificate)
 {
+    if (not rootCertificate.has_value())
+        return std::unexpected{RequestError{"SSL setup failed: could not find root certificate"}};
+
     ssl::context context{ssl::context::tls_client};
     context.set_verify_mode(ssl::verify_peer);
-    auto const rootCertificate = getRootCertificate();
-    if (not rootCertificate.has_value()) {
-        return std::unexpected{rootCertificate.error()};
-    }
-    context.add_certificate_authority(
+    context.add_certificate_authority(  //
         asio::buffer(rootCertificate->data(), rootCertificate->size())
     );
+
     return context;
 }
 
+std::expected<void, std::string>
+initClientSslContext()
+{
+    auto const& context = cachedClientSslContext();
+    if (not context.has_value())
+        return std::unexpected{context.error().message()};
+
+    return {};
+}
+
+ssl::context&
+getClientSslContext()
+{
+    // initClientSslContext() called during startup
+    return cachedClientSslContext().value();
+}
+
 std::optional<std::string>
 sslErrorToString(boost::beast::error_code const& error)
 {

src/util/requests/impl/SslContext.hpp

@@ -12,7 +12,13 @@
 namespace util::requests::impl {
 
 std::expected<boost::asio::ssl::context, RequestError>
-makeClientSslContext();
+makeClientSslContext(std::optional<std::string> const& rootCertificate);
+
+std::expected<void, std::string>
+initClientSslContext();
+
+boost::asio::ssl::context&
+getClientSslContext();
 
 std::optional<std::string>
 sslErrorToString(boost::beast::error_code const& error);

src/util/requests/impl/StreamData.hpp

@@ -1,6 +1,5 @@
 #pragma once
 
-#include "util/requests/Types.hpp"
 #include "util/requests/impl/SslContext.hpp"
 
 #include <boost/asio/associated_executor.hpp>
@@ -13,9 +12,6 @@
 #include <boost/beast/websocket.hpp>
 #include <boost/beast/websocket/stream.hpp>
 
-#include <expected>
-#include <utility>
-
 namespace util::requests::impl {
 
 template <typename StreamType>
@@ -34,29 +30,19 @@ using TcpStreamData = PlainStreamData<boost::beast::tcp_stream>;
 using WsStreamData = PlainStreamData<boost::beast::websocket::stream<boost::beast::tcp_stream>>;
 
 template <typename StreamType>
-class SslStreamData {
-    boost::asio::ssl::context sslContext_;
-
-public:
+struct SslStreamData {
     static constexpr bool kSslEnabled = true;
-
-    static std::expected<SslStreamData, RequestError>
-    create(boost::asio::yield_context yield)
-    {
-        auto sslContext = makeClientSslContext();
-        if (not sslContext.has_value()) {
-            return std::unexpected{std::move(sslContext.error())};
-        }
-        return SslStreamData{std::move(sslContext).value(), yield};
-    }
-
     StreamType stream;
 
-private:
-    SslStreamData(boost::asio::ssl::context sslContext, boost::asio::yield_context yield)
-        : sslContext_(std::move(sslContext))
-        , stream(boost::asio::get_associated_executor(yield), sslContext_)
+    static SslStreamData
+    create(boost::asio::yield_context yield)
+    {
+        return SslStreamData{getClientSslContext(), yield};
+    }
 
+private:
+    SslStreamData(boost::asio::ssl::context& sslContext, boost::asio::yield_context yield)
+        : stream(boost::asio::get_associated_executor(yield), sslContext)
     {
     }
 };

tests/unit/util/requests/SslContextTests.cpp

@@ -1,11 +1,36 @@
 #include "util/requests/impl/SslContext.hpp"
 
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
+#include <optional>
+
 using namespace util::requests::impl;
 
 TEST(SslContext, Create)
 {
-    auto ctx = makeClientSslContext();
-    EXPECT_TRUE(ctx);
+    auto& ctx = getClientSslContext();
+    EXPECT_NE(ctx.native_handle(), nullptr);
+}
+
+TEST(SslContext, IsCached)
+{
+    auto& first = getClientSslContext();
+    auto& second = getClientSslContext();
+
+    // Same shared instance is returned on every call
+    EXPECT_EQ(&first, &second);
+}
+
+TEST(SslContext, InitSucceedsWithSystemCertificates)
+{
+    EXPECT_TRUE(initClientSslContext().has_value());
+}
+
+TEST(SslContext, MakeWithoutRootCertificateReturnsError)
+{
+    auto const ctx = makeClientSslContext(std::nullopt);
+
+    ASSERT_FALSE(ctx.has_value());
+    EXPECT_THAT(ctx.error().message(), testing::HasSubstr("could not find root certificate"));
 }