Cleanup: Security::ContextPtr removal, pt3

Make the ContextPointer for server TLS contexts extend out of
libsecurity up the stack of callers to their main place of
med/long-term storage.

This means the code outside location where SSL contexts are created
mostly no longer needs to worry about (non-)locking details. Just
about using a smart Pointer properly.

diff --git a/src/anyp/PortCfg.cc b/src/anyp/PortCfg.cc
index 6c0c3a7a26b2908e7eb324a61f76ff3b1758370e..e062048e6b83776ef609d2e3b0ed69e0ffdd0cd3 100644 (file)
--- a/src/anyp/PortCfg.cc
+++ b/src/anyp/PortCfg.cc
@@ -103,7 +103,7 @@ AnyP::PortCfg::clone() const
 #if 0
     // TODO: AYJ: 2015-01-15: for now SSL does not clone the context object.
     // cloning should only be done before the PortCfg is post-configure initialized and opened
-    Security::ContextPtr sslContext;
+    Security::ContextPointer sslContext;
 #endif
 
 #endif /*0*/

diff --git a/src/client_side.cc b/src/client_side.cc
index 5aa1fec66c8d70d093fbbab794edcf456fb3ac7b..6647c84aafd73cc9f20ab35dbed99a79d3bdcff1 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2580,9 +2580,9 @@ httpAccept(const CommAcceptCbParams &params)
 
 /** Create SSL connection structure and update fd_table */
 static bool
-httpsCreate(const Comm::ConnectionPointer &conn, Security::ContextPtr sslContext)
+httpsCreate(const Comm::ConnectionPointer &conn, const Security::ContextPointer &ctx)
 {
-    if (Ssl::CreateServer(sslContext, conn, "client https start")) {
+    if (Ssl::CreateServer(ctx, conn, "client https start")) {
         debugs(33, 5, "will negotate SSL on " << conn);
         return true;
     }
@@ -2731,16 +2731,16 @@ clientNegotiateSSL(int fd, void *data)
 }
 
 /**
- * If Security::ContextPtr is given, starts reading the TLS handshake.
- * Otherwise, calls switchToHttps to generate a dynamic Security::ContextPtr.
+ * If Security::ContextPointer is given, starts reading the TLS handshake.
+ * Otherwise, calls switchToHttps to generate a dynamic Security::ContextPointer.
  */
 static void
-httpsEstablish(ConnStateData *connState, Security::ContextPtr sslContext)
+httpsEstablish(ConnStateData *connState, const Security::ContextPointer &ctx)
 {
     assert(connState);
     const Comm::ConnectionPointer &details = connState->clientConnection;
 
-    if (!sslContext || !httpsCreate(details, sslContext))
+    if (!ctx || !httpsCreate(details, ctx))
         return;
 
     typedef CommCbMemFunT<ConnStateData, CommTimeoutCbParams> TimeoutDialer;
@@ -2841,7 +2841,7 @@ ConnStateData::postHttpsAccept()
         acl_checklist->nonBlockingCheck(httpsSslBumpAccessCheckDone, this);
         return;
     } else {
-        httpsEstablish(this, port->secure.staticContext.get());
+        httpsEstablish(this, port->secure.staticContext);
     }
 }
 
@@ -2883,14 +2883,15 @@ ConnStateData::sslCrtdHandleReply(const Helper::Reply &reply)
                     SSL_CTX *sslContext = SSL_get_SSL_CTX(ssl);
                     Ssl::configureUnconfiguredSslContext(sslContext, signAlgorithm, *port);
                 } else {
-                    auto ctx = Ssl::generateSslContextUsingPkeyAndCertFromMemory(reply_message.getBody().c_str(), *port);
+                    Security::ContextPointer ctx(Ssl::generateSslContextUsingPkeyAndCertFromMemory(reply_message.getBody().c_str(), *port));
                     getSslContextDone(ctx, true);
                 }
                 return;
             }
         }
     }
-    getSslContextDone(NULL);
+    Security::ContextPointer nil;
+    getSslContextDone(nil);
 }
 
 void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &certProperties)
@@ -3003,13 +3004,12 @@ ConnStateData::getSslContextStart()
         if (!(sslServerBump && (sslServerBump->act.step1 == Ssl::bumpPeek || sslServerBump->act.step1 == Ssl::bumpStare))) {
             debugs(33, 5, "Finding SSL certificate for " << sslBumpCertKey << " in cache");
             Ssl::LocalContextStorage * ssl_ctx_cache = Ssl::TheGlobalContextStorage.getLocalStorage(port->s);
-            Security::ContextPtr dynCtx = nullptr;
             Security::ContextPointer *cachedCtx = ssl_ctx_cache ? ssl_ctx_cache->get(sslBumpCertKey.termedBuf()) : nullptr;
-            if (cachedCtx && (dynCtx = cachedCtx->get())) {
+            if (cachedCtx) {
                 debugs(33, 5, "SSL certificate for " << sslBumpCertKey << " found in cache");
-                if (Ssl::verifySslCertificate(dynCtx, certProperties)) {
+                if (Ssl::verifySslCertificate(cachedCtx->get(), certProperties)) {
                     debugs(33, 5, "Cached SSL certificate for " << sslBumpCertKey << " is valid");
-                    getSslContextDone(dynCtx);
+                    getSslContextDone(*cachedCtx);
                     return;
                 } else {
                     debugs(33, 5, "Cached SSL certificate for " << sslBumpCertKey << " is out of date. Delete this certificate from cache");
@@ -3049,22 +3049,24 @@ ConnStateData::getSslContextStart()
             SSL_CTX *sslContext = SSL_get_SSL_CTX(ssl);
             Ssl::configureUnconfiguredSslContext(sslContext, certProperties.signAlgorithm, *port);
         } else {
-            auto dynCtx = Ssl::generateSslContext(certProperties, *port);
+            Security::ContextPointer dynCtx(Ssl::generateSslContext(certProperties, *port));
             getSslContextDone(dynCtx, true);
         }
         return;
     }
-    getSslContextDone(NULL);
+
+    Security::ContextPointer nil;
+    getSslContextDone(nil);
 }
 
 void
-ConnStateData::getSslContextDone(Security::ContextPtr sslContext, bool isNew)
+ConnStateData::getSslContextDone(Security::ContextPointer &ctx, bool isNew)
 {
     // Try to add generated ssl context to storage.
     if (port->generateHostCertificates && isNew) {
 
-        if (sslContext && (signAlgorithm == Ssl::algSignTrusted)) {
-            Ssl::chainCertificatesToSSLContext(sslContext, *port);
+        if (ctx && (signAlgorithm == Ssl::algSignTrusted)) {
+            Ssl::chainCertificatesToSSLContext(ctx.get(), *port);
         } else if (signAlgorithm == Ssl::algSignTrusted) {
             debugs(33, DBG_IMPORTANT, "WARNING: can not add signing certificate to SSL context chain because SSL context chain is invalid!");
         }
@@ -3072,10 +3074,10 @@ ConnStateData::getSslContextDone(Security::ContextPtr sslContext, bool isNew)
 
         Ssl::LocalContextStorage *ssl_ctx_cache = Ssl::TheGlobalContextStorage.getLocalStorage(port->s);
         assert(sslBumpCertKey.size() > 0 && sslBumpCertKey[0] != '\0');
-        if (sslContext) {
-            if (!ssl_ctx_cache || !ssl_ctx_cache->add(sslBumpCertKey.termedBuf(), new Security::ContextPointer(sslContext))) {
+        if (ctx) {
+            if (!ssl_ctx_cache || !ssl_ctx_cache->add(sslBumpCertKey.termedBuf(), new Security::ContextPointer(ctx))) {
                 // If it is not in storage delete after using. Else storage deleted it.
-                fd_table[clientConnection->fd].dynamicSslContext = sslContext;
+                fd_table[clientConnection->fd].dynamicTlsContext = ctx;
             }
         } else {
             debugs(33, 2, HERE << "Failed to generate SSL cert for " << sslConnectHostOrIp);
@@ -3083,18 +3085,18 @@ ConnStateData::getSslContextDone(Security::ContextPtr sslContext, bool isNew)
     }
 
     // If generated ssl context = NULL, try to use static ssl context.
-    if (!sslContext) {
+    if (!ctx) {
         if (!port->secure.staticContext) {
             debugs(83, DBG_IMPORTANT, "Closing " << clientConnection->remote << " as lacking TLS context");
             clientConnection->close();
             return;
         } else {
             debugs(33, 5, "Using static TLS context.");
-            sslContext = port->secure.staticContext.get();
+            ctx = port->secure.staticContext;
         }
     }
 
-    if (!httpsCreate(clientConnection, sslContext))
+    if (!httpsCreate(clientConnection, ctx))
         return;
 
     // bumped intercepted conns should already have Config.Timeout.request set
@@ -3315,8 +3317,8 @@ ConnStateData::startPeekAndSpliceDone()
     }
 
     // will call httpsPeeked() with certificate and connection, eventually
-    auto unConfiguredCTX = Ssl::createSSLContext(port->signingCert, port->signPkey, *port);
-    fd_table[clientConnection->fd].dynamicSslContext = unConfiguredCTX;
+    Security::ContextPointer unConfiguredCTX(Ssl::createSSLContext(port->signingCert, port->signPkey, *port));
+    fd_table[clientConnection->fd].dynamicTlsContext = unConfiguredCTX;
 
     if (!httpsCreate(clientConnection, unConfiguredCTX))
         return;

diff --git a/src/client_side.h b/src/client_side.h
index 1cad88c7040bffe125feb162a0d4516d22fc7285..2329a98a996346363993fcff2be5a57ae6283127 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -221,14 +221,14 @@ public:
     /// \retval false otherwise
     bool spliceOnError(const err_type err);
 
-    /// Start to create dynamic Security::ContextPtr for host or uses static port SSL context.
+    /// Start to create dynamic Security::ContextPointer for host or uses static port SSL context.
     void getSslContextStart();
     /**
      * Done create dynamic ssl certificate.
      *
      * \param[in] isNew if generated certificate is new, so we need to add this certificate to storage.
      */
-    void getSslContextDone(Security::ContextPtr sslContext, bool isNew = false);
+    void getSslContextDone(Security::ContextPointer &, bool isNew = false);
     /// Callback function. It is called when squid receive message from ssl_crtd.
     static void sslCrtdHandleReplyWrapper(void *data, const Helper::Reply &reply);
     /// Proccess response from ssl_crtd.

diff --git a/src/comm.cc b/src/comm.cc
index 0edaea2ff155b8afd6923a4b45a35a1dd6a8bf9e..9b451d7f4fc0d96f7484d6f2a7dc4689b0befff7 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -839,13 +839,7 @@ comm_close_complete(const FdeCbParams &params)
 {
     fde *F = &fd_table[params.fd];
     F->ssl.reset();
-
-#if USE_OPENSSL
-    if (F->dynamicSslContext) {
-        SSL_CTX_free(F->dynamicSslContext);
-        F->dynamicSslContext = NULL;
-    }
-#endif
+    F->dynamicTlsContext.reset();
     fd_close(params.fd);        /* update fdstat */
     close(params.fd);
 

diff --git a/src/fde.h b/src/fde.h
index 938e3b79b951371ee274ef2b2baef81d49798130..69560ed87ae6849768e685c65b5517fa30321620 100644 (file)
--- a/src/fde.h
+++ b/src/fde.h
@@ -120,7 +120,7 @@ public:
     READ_HANDLER *read_method;
     WRITE_HANDLER *write_method;
     Security::SessionPointer ssl;
-    Security::ContextPtr dynamicSslContext; ///< cached and then freed when fd is closed
+    Security::ContextPointer dynamicTlsContext; ///< cached and then freed when fd is closed
 #if _SQUID_WINDOWS_
     struct {
         long handle;
@@ -168,7 +168,7 @@ public:
         read_method = NULL;
         write_method = NULL;
         ssl.reset();
-        dynamicSslContext = NULL;
+        dynamicTlsContext.reset();
 #if _SQUID_WINDOWS_
         win32.handle = (long)NULL;
 #endif

diff --git a/src/ssl/support.cc b/src/ssl/support.cc
index 641ed300fd0d107e56b22e011cdd219440975f79..33c04650128510b8e4607a4bd008a1a8d1448b32 100644 (file)
--- a/src/ssl/support.cc
+++ b/src/ssl/support.cc
@@ -235,7 +235,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx)
 
     char buffer[256] = "";
     SSL *ssl = (SSL *)X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
-    Security::ContextPtr sslctx = SSL_get_SSL_CTX(ssl);
+    SSL_CTX *sslctx = SSL_get_SSL_CTX(ssl);
     SBuf *server = (SBuf *)SSL_get_ex_data(ssl, ssl_ex_index_server);
     void *dont_verify_domain = SSL_CTX_get_ex_data(sslctx, ssl_ctx_ex_index_dont_verify_domain);
     ACLChecklist *check = (ACLChecklist*)SSL_get_ex_data(ssl, ssl_ex_index_cert_error_check);
@@ -925,45 +925,41 @@ sslGetUserCertificateChainPEM(SSL *ssl)
 }
 
 /// Create SSL context and apply ssl certificate and private key to it.
-Security::ContextPtr
+Security::ContextPointer
 Ssl::createSSLContext(Security::CertPointer & x509, Ssl::EVP_PKEY_Pointer & pkey, AnyP::PortCfg &port)
 {
-#if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
-    Security::ContextPointer sslContext(SSL_CTX_new(TLS_server_method()));
-#else
-    Security::ContextPointer sslContext(SSL_CTX_new(SSLv23_server_method()));
-#endif
+    Security::ContextPointer ctx(port.secure.createBlankContext());
 
-    if (!SSL_CTX_use_certificate(sslContext.get(), x509.get()))
-        return NULL;
+    if (!SSL_CTX_use_certificate(ctx.get(), x509.get()))
+        return Security::ContextPointer();
 
-    if (!SSL_CTX_use_PrivateKey(sslContext.get(), pkey.get()))
-        return NULL;
+    if (!SSL_CTX_use_PrivateKey(ctx.get(), pkey.get()))
+        return Security::ContextPointer();
 
-    if (!configureSslContext(sslContext.get(), port))
-        return NULL;
+    if (!configureSslContext(ctx.get(), port))
+        return Security::ContextPointer();
 
-    return sslContext.release();
+    return ctx;
 }
 
-Security::ContextPtr
+Security::ContextPointer
 Ssl::generateSslContextUsingPkeyAndCertFromMemory(const char * data, AnyP::PortCfg &port)
 {
     Security::CertPointer cert;
     Ssl::EVP_PKEY_Pointer pkey;
     if (!readCertAndPrivateKeyFromMemory(cert, pkey, data) || !cert || !pkey)
-        return nullptr;
+        return Security::ContextPointer();
 
     return createSSLContext(cert, pkey, port);
 }
 
-Security::ContextPtr
+Security::ContextPointer
 Ssl::generateSslContext(CertificateProperties const &properties, AnyP::PortCfg &port)
 {
     Security::CertPointer cert;
     Ssl::EVP_PKEY_Pointer pkey;
     if (!generateSslCertificate(cert, pkey, properties) || !cert || !pkey)
-        return nullptr;
+        return Security::ContextPointer();
 
     return createSSLContext(cert, pkey, port);
 }
@@ -1443,9 +1439,9 @@ Ssl::CreateClient(Security::ContextPtr sslContext, const Comm::ConnectionPointer
 }
 
 bool
-Ssl::CreateServer(Security::ContextPtr sslContext, const Comm::ConnectionPointer &c, const char *squidCtx)
+Ssl::CreateServer(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &c, const char *squidCtx)
 {
-    return SslCreate(sslContext, c, Ssl::Bio::BIO_TO_CLIENT, squidCtx);
+    return SslCreate(ctx.get(), c, Ssl::Bio::BIO_TO_CLIENT, squidCtx);
 }
 
 static int

diff --git a/src/ssl/support.h b/src/ssl/support.h
index f2fbfcfd124deab401f8dd0c60ba14de8b2473df..9bcf1a150245f143049005163a484df50b4758c6 100644 (file)
--- a/src/ssl/support.h
+++ b/src/ssl/support.h
@@ -79,7 +79,7 @@ bool CreateClient(Security::ContextPtr sslContext, const Comm::ConnectionPointer
 
 /// Creates SSL Server connection structure and initializes SSL I/O (Comm and BIO).
 /// On errors, emits DBG_IMPORTANT with details and returns false.
-bool CreateServer(Security::ContextPtr sslContext, const Comm::ConnectionPointer &, const char *squidCtx);
+bool CreateServer(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *squidCtx);
 
 void SetSessionCallbacks(Security::ContextPtr);
 extern Ipc::MemMap *SessionCache;
@@ -229,7 +229,7 @@ void unloadSquidUntrusted();
   \ingroup ServerProtocolSSLAPI
   * Decide on the kind of certificate and generate a CA- or self-signed one
 */
-Security::ContextPtr generateSslContext(CertificateProperties const &properties, AnyP::PortCfg &port);
+Security::ContextPointer generateSslContext(CertificateProperties const &properties, AnyP::PortCfg &port);
 
 /**
   \ingroup ServerProtocolSSLAPI
@@ -245,13 +245,13 @@ bool verifySslCertificate(Security::ContextPtr sslContext,  CertificatePropertie
   * Read private key and certificate from memory and generate SSL context
   * using their.
  */
-Security::ContextPtr generateSslContextUsingPkeyAndCertFromMemory(const char * data, AnyP::PortCfg &port);
+Security::ContextPointer generateSslContextUsingPkeyAndCertFromMemory(const char * data, AnyP::PortCfg &port);
 
 /**
   \ingroup ServerProtocolSSLAPI
   * Create an SSL context using the provided certificate and key
  */
-Security::ContextPtr createSSLContext(Security::CertPointer & x509, Ssl::EVP_PKEY_Pointer & pkey, AnyP::PortCfg &port);
+Security::ContextPointer createSSLContext(Security::CertPointer & x509, Ssl::EVP_PKEY_Pointer & pkey, AnyP::PortCfg &port);
 
 /**
  \ingroup ServerProtocolSSLAPI

diff --git a/src/tests/stub_client_side.cc b/src/tests/stub_client_side.cc
index 0e5a482ed169ab184261db7dcdc54c1d90964276..84b263a84df194a3fd7f4125ef660360d5af6e2b 100644 (file)
--- a/src/tests/stub_client_side.cc
+++ b/src/tests/stub_client_side.cc
@@ -42,7 +42,7 @@ void ConnStateData::quitAfterError(HttpRequest *) STUB
 #if USE_OPENSSL
 void ConnStateData::httpsPeeked(Comm::ConnectionPointer) STUB
 void ConnStateData::getSslContextStart() STUB
-void ConnStateData::getSslContextDone(Security::ContextPtr, bool) STUB
+void ConnStateData::getSslContextDone(Security::ContextPointer &, bool) STUB
 void ConnStateData::sslCrtdHandleReplyWrapper(void *, const Helper::Reply &) STUB
 void ConnStateData::sslCrtdHandleReply(const Helper::Reply &) STUB
 void ConnStateData::switchToHttps(HttpRequest *, Ssl::BumpMode) STUB

diff --git a/src/tests/stub_libsslsquid.cc b/src/tests/stub_libsslsquid.cc
index 32f58fa01781f194107cc7f73cf1ecc1ae28d5e8..69b01cec327c2ad91fe7483ca2635388f75b15c1 100644 (file)
--- a/src/tests/stub_libsslsquid.cc
+++ b/src/tests/stub_libsslsquid.cc
@@ -68,9 +68,9 @@ namespace Ssl
 //GETX509ATTRIBUTE GetX509Fingerprint;
 const char *BumpModeStr[] = {""};
 bool generateUntrustedCert(Security::CertPointer & untrustedCert, EVP_PKEY_Pointer & untrustedPkey, Security::CertPointer const & cert, EVP_PKEY_Pointer const & pkey) STUB_RETVAL(false)
-Security::ContextPtr generateSslContext(CertificateProperties const &properties, AnyP::PortCfg &port) STUB_RETVAL(NULL)
+Security::ContextPointer generateSslContext(CertificateProperties const &, AnyP::PortCfg &) STUB_RETVAL(Security::ContextPointer())
 bool verifySslCertificate(Security::ContextPtr sslContext,  CertificateProperties const &properties) STUB_RETVAL(false)
-Security::ContextPtr generateSslContextUsingPkeyAndCertFromMemory(const char * data, AnyP::PortCfg &port) STUB_RETVAL(NULL)
+Security::ContextPointer generateSslContextUsingPkeyAndCertFromMemory(const char *, AnyP::PortCfg &) STUB_RETVAL(Security::ContextPointer())
 void addChainToSslContext(Security::ContextPtr sslContext, STACK_OF(X509) *certList) STUB
 void readCertChainAndPrivateKeyFromFiles(Security::CertPointer & cert, EVP_PKEY_Pointer & pkey, X509_STACK_Pointer & chain, char const * certFilename, char const * keyFilename) STUB
 int matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_func)(void *check_data,  ASN1_STRING *cn_data)) STUB_RETVAL(0)