Refactor TLS SSL context configuration

src/tls/cert.h

@@ -73,14 +73,8 @@ namespace tls
 
     ~Cert() = default;
 
-    void configure_ssl(SSL* ssl, SSL_CTX* ssl_ctx) const
+    void configure_context(SSL_CTX* ssl_ctx)
     {
-      if (peer_hostname.has_value())
-      {
-        // Peer hostname for SNI
-        CHECK1(SSL_set_tlsext_host_name(ssl, peer_hostname->c_str()));
-      }
-
       if (peer_ca)
       {
         peer_ca->configure_trusted_cert_store(ssl_ctx);
@@ -94,7 +88,6 @@ namespace tls
           return ok;
         };
         SSL_CTX_set_verify(ssl_ctx, opts, cb);
-        SSL_set_verify(ssl, opts, cb);
       }
       else
       {
@@ -106,14 +99,21 @@ namespace tls
         // to verify it here, just request it.
         auto cb = [](int, x509_store_ctx_st*) { return 1; };
         SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, cb);
-        SSL_set_verify(ssl, SSL_VERIFY_PEER, cb);
       }
 
       if (has_own_cert)
       {
         CHECK1(
           SSL_CTX_use_cert_and_key(ssl_ctx, own_cert, *own_pkey, chain, 1));
-        CHECK1(SSL_use_cert_and_key(ssl, own_cert, *own_pkey, chain, 1));
+      }
+    }
+
+    void configure_connection(SSL* ssl)
+    {
+      if (peer_hostname.has_value())
+      {
+        // Peer hostname for SNI
+        CHECK1(SSL_set_tlsext_host_name(ssl, peer_hostname->c_str()));
       }
     }
   };

src/tls/client.h

@@ -14,7 +14,9 @@ namespace tls
   public:
     Client(std::shared_ptr<Cert> cert_) : Context(true), cert(std::move(cert_))
     {
-      cert->configure_ssl(ssl, cfg);
+      cert->configure_context(cfg);
+      create_ssl();
+      cert->configure_connection(get_ssl());
     }
   };
 }

src/tls/context.h

@@ -17,67 +17,70 @@ namespace ccf::tls
   {
   protected:
     ccf::crypto::OpenSSL::Unique_SSL_CTX cfg;
-    ccf::crypto::OpenSSL::Unique_SSL ssl;
+    std::unique_ptr<ccf::crypto::OpenSSL::Unique_SSL> ssl;
+    bool client;
+
+    void create_ssl()
+    {
+      ssl = std::make_unique<ccf::crypto::OpenSSL::Unique_SSL>(cfg);
+
+      // Initialise connection
+      if (client)
+      {
+        SSL_set_connect_state(*ssl);
+      }
+      else
+      {
+        SSL_set_accept_state(*ssl);
+      }
+    }
+
+    SSL* get_ssl()
+    {
+      // Context construction is split from SSL creation, so catch accidental
+      // use before create_ssl().
+      CHECKNULL(ssl.get());
+      CHECKNULL(*ssl);
+      return *ssl;
+    }
 
   public:
-    Context(bool client) :
-      cfg(client ? TLS_client_method() : TLS_server_method()),
-      ssl(cfg)
+    Context(bool client_) :
+      cfg(client_ ? TLS_client_method() : TLS_server_method()),
+      client(client_)
     {
       // Require at least TLS 1.2, support up to 1.3
-      SSL_CTX_set_min_proto_version(cfg, TLS1_2_VERSION);
-      SSL_set_min_proto_version(ssl, TLS1_2_VERSION);
+      CHECK1(SSL_CTX_set_min_proto_version(cfg, TLS1_2_VERSION));
 
       // Disable renegotiation to avoid DoS
       SSL_CTX_set_options(
         cfg,
         SSL_OP_CIPHER_SERVER_PREFERENCE |
           SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION |
           SSL_OP_NO_RENEGOTIATION);
-      SSL_set_options(
-        ssl,
-        SSL_OP_CIPHER_SERVER_PREFERENCE |
-          SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION |
-          SSL_OP_NO_RENEGOTIATION);
 
       // Set cipher for TLS 1.2
       const auto* const cipher_list =
         "ECDHE-ECDSA-AES256-GCM-SHA384:"
         "ECDHE-ECDSA-AES128-GCM-SHA256:"
         "ECDHE-RSA-AES256-GCM-SHA384:"
         "ECDHE-RSA-AES128-GCM-SHA256";
-      SSL_CTX_set_cipher_list(cfg, cipher_list);
-      SSL_set_cipher_list(ssl, cipher_list);
+      CHECK1(SSL_CTX_set_cipher_list(cfg, cipher_list));
 
       // Set cipher for TLS 1.3
       const auto* const ciphersuites =
         "TLS_AES_256_GCM_SHA384:"
         "TLS_AES_128_GCM_SHA256";
-      SSL_CTX_set_ciphersuites(cfg, ciphersuites);
-      SSL_set_ciphersuites(ssl, ciphersuites);
+      CHECK1(SSL_CTX_set_ciphersuites(cfg, ciphersuites));
 
       // Restrict the curves to approved ones
-      SSL_CTX_set1_curves_list(cfg, "P-521:P-384:P-256");
-      SSL_set1_curves_list(ssl, "P-521:P-384:P-256");
+      CHECK1(SSL_CTX_set1_curves_list(cfg, "P-521:P-384:P-256"));
 
       // Allow buffer to be relocated between WANT_WRITE retries, and do partial
       // writes if possible
       SSL_CTX_set_mode(
         cfg,
         SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER | SSL_MODE_ENABLE_PARTIAL_WRITE);
-      SSL_set_mode(
-        ssl,
-        SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER | SSL_MODE_ENABLE_PARTIAL_WRITE);
-
-      // Initialise connection
-      if (client)
-      {
-        SSL_set_connect_state(ssl);
-      }
-      else
-      {
-        SSL_set_accept_state(ssl);
-      }
     }
 
     virtual ~Context() = default;
@@ -86,27 +89,33 @@ namespace ccf::tls
       void* cb_obj, BIO_callback_fn_ex send, BIO_callback_fn_ex recv)
     {
       // Read/Write BIOs will be used by TLS
-      BIO* rbio = BIO_new(BIO_s_mem());
-      BIO_set_mem_eof_return(rbio, -1);
-      BIO_set_callback_arg(rbio, static_cast<char*>(cb_obj));
-      BIO_set_callback_ex(rbio, recv);
-      SSL_set0_rbio(ssl, rbio);
-
-      BIO* wbio = BIO_new(BIO_s_mem());
-      BIO_set_mem_eof_return(wbio, -1);
-      BIO_set_callback_arg(wbio, static_cast<char*>(cb_obj));
-      BIO_set_callback_ex(wbio, send);
-      SSL_set0_wbio(ssl, wbio);
+      std::unique_ptr<BIO, decltype(&BIO_free)> rbio(
+        BIO_new(BIO_s_mem()), BIO_free);
+      CHECKNULL(rbio.get());
+
+      std::unique_ptr<BIO, decltype(&BIO_free)> wbio(
+        BIO_new(BIO_s_mem()), BIO_free);
+      CHECKNULL(wbio.get());
+
+      BIO_set_mem_eof_return(rbio.get(), -1);
+      BIO_set_callback_arg(rbio.get(), static_cast<char*>(cb_obj));
+      BIO_set_callback_ex(rbio.get(), recv);
+      SSL_set0_rbio(get_ssl(), rbio.release());
+
+      BIO_set_mem_eof_return(wbio.get(), -1);
+      BIO_set_callback_arg(wbio.get(), static_cast<char*>(cb_obj));
+      BIO_set_callback_ex(wbio.get(), send);
+      SSL_set0_wbio(get_ssl(), wbio.release());
     }
 
     virtual int handshake()
     {
-      if (SSL_is_init_finished(ssl) != 0)
+      if (SSL_is_init_finished(get_ssl()) != 0)
       {
         return 0;
       }
 
-      int rc = SSL_do_handshake(ssl);
+      int rc = SSL_do_handshake(get_ssl());
       // Success in OpenSSL is 1, MBed is 0
       if (rc > 0)
       {
@@ -115,12 +124,12 @@ namespace ccf::tls
       }
 
       // Want read/write needs special return
-      if (SSL_want_read(ssl))
+      if (SSL_want_read(get_ssl()))
       {
         return TLS_ERR_WANT_READ;
       }
 
-      if (SSL_want_write(ssl))
+      if (SSL_want_write(get_ssl()))
       {
         return TLS_ERR_WANT_WRITE;
       }
@@ -135,7 +144,7 @@ namespace ccf::tls
       LOG_TRACE_FMT("Context::handshake() : Error code {}", rc);
 
       // As an MBedTLS emulation, we return negative for errors.
-      return -SSL_get_error(ssl, rc);
+      return -SSL_get_error(get_ssl(), rc);
     }
 
     virtual int read(uint8_t* buf, size_t len)
@@ -145,12 +154,12 @@ namespace ccf::tls
         return 0;
       }
       size_t readbytes = 0;
-      int rc = SSL_read_ex(ssl, buf, len, &readbytes);
+      int rc = SSL_read_ex(get_ssl(), buf, len, &readbytes);
       if (rc > 0)
       {
         return readbytes;
       }
-      if (SSL_want_read(ssl))
+      if (SSL_want_read(get_ssl()))
       {
         return TLS_ERR_WANT_READ;
       }
@@ -159,7 +168,7 @@ namespace ccf::tls
       LOG_TRACE_FMT("Context::read() : Error code {}", rc);
 
       // As an MBedTLS emulation, we return negative for errors.
-      return -SSL_get_error(ssl, rc);
+      return -SSL_get_error(get_ssl(), rc);
     }
 
     virtual int write(const uint8_t* buf, size_t len)
@@ -169,12 +178,12 @@ namespace ccf::tls
         return 0;
       }
       size_t written = 0;
-      int rc = SSL_write_ex(ssl, buf, len, &written);
+      int rc = SSL_write_ex(get_ssl(), buf, len, &written);
       if (rc > 0)
       {
         return written;
       }
-      if (SSL_want_write(ssl))
+      if (SSL_want_write(get_ssl()))
       {
         return TLS_ERR_WANT_WRITE;
       }
@@ -183,23 +192,23 @@ namespace ccf::tls
       LOG_TRACE_FMT("Context::write() : Error code {}", rc);
 
       // As an MBedTLS emulation, we return negative for errors.
-      return -SSL_get_error(ssl, rc);
+      return -SSL_get_error(get_ssl(), rc);
     }
 
     virtual int close()
     {
       LOG_TRACE_FMT("Context::close() : Shutdown");
-      return SSL_shutdown(ssl);
+      return SSL_shutdown(get_ssl());
     }
 
     virtual bool peer_cert_ok()
     {
-      return SSL_get_verify_result(ssl) == X509_V_OK;
+      return SSL_get_verify_result(get_ssl()) == X509_V_OK;
     }
 
     virtual std::string get_verify_error()
     {
-      return X509_verify_cert_error_string(SSL_get_verify_result(ssl));
+      return X509_verify_cert_error_string(SSL_get_verify_result(get_ssl()));
     }
 
     virtual std::string host()
@@ -214,7 +223,7 @@ namespace ccf::tls
       // SSL_get_peer_certificate just to extract it from the context.
 
       ccf::crypto::OpenSSL::Unique_X509 cert(
-        SSL_get_peer_certificate(ssl), /*check_null=*/false);
+        SSL_get_peer_certificate(get_ssl()), /*check_null=*/false);
       if (cert == nullptr)
       {
         LOG_TRACE_FMT("Empty peer cert");

src/tls/server.h

@@ -47,7 +47,7 @@ namespace tls
       Context(false),
       cert(cert_)
     {
-      cert->configure_ssl(ssl, cfg);
+      cert->configure_context(cfg);
 
       // Configure protocols negotiated by ALPN
       // See https://nghttp2.org/documentation/tutorial-server.html and use of
@@ -67,6 +67,8 @@ namespace tls
           alpn_protos_data, sizeof(alpn_protos_data)};
         SSL_CTX_set_alpn_select_cb(cfg, alpn_select_cb, &alpn_protos);
       }
+
+      create_ssl();
     }
   };
 }

src/tls/test/main.cpp

@@ -305,9 +305,10 @@ TEST_CASE("Cert configures TLS verification and own certificate")
   auto ca = get_ca();
   auto cert = get_dummy_cert(ca, "server");
   ccf::crypto::OpenSSL::Unique_SSL_CTX ctx(TLS_method());
-  ccf::crypto::OpenSSL::Unique_SSL ssl(ctx);
 
-  cert->configure_ssl(ssl, ctx);
+  cert->configure_context(ctx);
+  ccf::crypto::OpenSSL::Unique_SSL ssl(ctx);
+  cert->configure_connection(ssl);
 
   constexpr auto expected_verify_mode =
     SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT;
@@ -466,6 +467,38 @@ void run_test_case(
   server.close();
 }
 
+class InspectableClient : public tls::Client
+{
+public:
+  using tls::Client::Client;
+
+  int verify_mode()
+  {
+    auto* ssl = get_ssl();
+    REQUIRE(ssl != nullptr);
+    return SSL_get_verify_mode(ssl);
+  }
+};
+
+TEST_CASE("connection inherits verification mode from context")
+{
+  auto ca = get_ca();
+
+  InspectableClient verified_client(get_dummy_cert(ca, "verified"));
+  REQUIRE(
+    (verified_client.verify_mode() &
+     (SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) ==
+    (SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT));
+
+  InspectableClient request_only_client(
+    get_dummy_cert(ca, "request_only", false));
+  // auth_required=false still requests a peer certificate, but does not fail
+  // the handshake if the peer certificate is missing.
+  REQUIRE((request_only_client.verify_mode() & SSL_VERIFY_PEER) != 0);
+  REQUIRE(
+    (request_only_client.verify_mode() & SSL_VERIFY_FAIL_IF_NO_PEER_CERT) == 0);
+}
+
 TEST_CASE("unverified handshake")
 {
   // Create a CA