From: Amos Jeffries Date: Wed, 21 Sep 2016 17:56:27 +0000 (+1200) Subject: Cleanup: Security::ContextPtr removal, pt3 X-Git-Tag: SQUID_4_0_15~27 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=0476ec45c041d9a6e0860deb9705587a38a1e34f;p=thirdparty%2Fsquid.git 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 6c0c3a7a26..e062048e6b 100644 --- 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 5aa1fec66c..6647c84aaf 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2580,9 +2580,9 @@ httpAccept(const CommAcceptCbParams ¶ms) /** 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 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 1cad88c704..2329a98a99 100644 --- 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 0edaea2ff1..9b451d7f4f 100644 --- a/src/comm.cc +++ b/src/comm.cc @@ -839,13 +839,7 @@ comm_close_complete(const FdeCbParams ¶ms) { 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 938e3b79b9..69560ed87a 100644 --- 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 641ed300fd..33c0465012 100644 --- 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 f2fbfcfd12..9bcf1a1502 100644 --- 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 0e5a482ed1..84b263a84d 100644 --- 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 32f58fa017..69b01cec32 100644 --- 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)