From: Eduard Bagdasaryan Date: Fri, 28 Feb 2025 15:48:30 +0000 (+0000) Subject: NoNewGlobals for OpenSSL-related structures (#2006) X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9e779e40ef96d05dd8d1b56eec2b56bfa9fec0e1;p=thirdparty%2Fsquid.git NoNewGlobals for OpenSSL-related structures (#2006) These changes were anticipated in Bug 5390 fix (recent commit c565067): https://bugs.squid-cache.org/show_bug.cgi?id=5390#c16 They eliminate all known OpenSSL-related globals: * Security::ProxyOutgoingConfig * Ssl::SquidUntrustedCerts * Ssl::TheGeneratorRequests * Ssl::TheGlobalContextStorage Also applied AAA and range-based `for` loop upgrades to modified lines. --- diff --git a/src/cache_cf.cc b/src/cache_cf.cc index 24105a6ae0..0627a4dd13 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -957,10 +957,10 @@ configDoConfigure(void) Ssl::loadSquidUntrusted(Config.ssl_client.foreignIntermediateCertsPath); #endif - if (Security::ProxyOutgoingConfig.encryptTransport) { + if (Security::ProxyOutgoingConfig().encryptTransport) { debugs(3, 2, "initializing https:// proxy context"); - const auto rawSslContext = Security::ProxyOutgoingConfig.createClientContext(false); + const auto rawSslContext = Security::ProxyOutgoingConfig().createClientContext(false); Config.ssl_client.sslContext_ = rawSslContext ? new Security::ContextPointer(rawSslContext) : nullptr; if (!Config.ssl_client.sslContext_) { #if USE_OPENSSL @@ -972,7 +972,7 @@ configDoConfigure(void) #if USE_OPENSSL Ssl::useSquidUntrusted(Config.ssl_client.sslContext_->get()); #endif - Config.ssl_client.defaultPeerContext = new Security::FuturePeerContext(Security::ProxyOutgoingConfig, *Config.ssl_client.sslContext_); + Config.ssl_client.defaultPeerContext = new Security::FuturePeerContext(Security::ProxyOutgoingConfig(), *Config.ssl_client.sslContext_); } for (const auto &p: CurrentCachePeers()) { @@ -1112,7 +1112,7 @@ parse_obsolete(const char *name) throw TextException(ToSBuf("missing required parameter for obsolete directive: ", name), Here()); tmp.append(token, strlen(token)); - Security::ProxyOutgoingConfig.parse(tmp.c_str()); + Security::ProxyOutgoingConfig().parse(tmp.c_str()); } } diff --git a/src/cf.data.pre b/src/cf.data.pre index 560573fac6..3bfcc9e1ef 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -3024,7 +3024,7 @@ NAME: tls_outgoing_options IFDEF: HAVE_LIBGNUTLS||USE_OPENSSL TYPE: securePeerOptions DEFAULT: min-version=1.0 -LOC: Security::ProxyOutgoingConfig +LOC: Security::ProxyOutgoingConfig() DOC_START disable Do not support https:// URLs. diff --git a/src/client_side.cc b/src/client_side.cc index deb64d19ea..ee43129ca0 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2647,7 +2647,7 @@ Security::ContextPointer ConnStateData::getTlsContextFromCache(const SBuf &cacheKey, const Ssl::CertificateProperties &certProperties) { debugs(33, 5, "Finding SSL certificate for " << cacheKey << " in cache"); - Ssl::LocalContextStorage * ssl_ctx_cache = Ssl::TheGlobalContextStorage.getLocalStorage(port->s); + const auto ssl_ctx_cache = Ssl::TheGlobalContextStorage().getLocalStorage(port->s); if (const auto ctx = ssl_ctx_cache ? ssl_ctx_cache->get(cacheKey) : nullptr) { if (Ssl::verifySslCertificate(*ctx, certProperties)) { debugs(33, 5, "Cached SSL certificate for " << certProperties.commonName << " is valid"); @@ -2664,7 +2664,7 @@ ConnStateData::getTlsContextFromCache(const SBuf &cacheKey, const Ssl::Certifica void ConnStateData::storeTlsContextToCache(const SBuf &cacheKey, Security::ContextPointer &ctx) { - Ssl::LocalContextStorage *ssl_ctx_cache = Ssl::TheGlobalContextStorage.getLocalStorage(port->s); + const auto ssl_ctx_cache = Ssl::TheGlobalContextStorage().getLocalStorage(port->s); if (!ssl_ctx_cache || !ssl_ctx_cache->add(cacheKey, ctx)) { // If it is not in storage delete after using. Else storage deleted it. fd_table[clientConnection->fd].dynamicTlsContext = ctx; @@ -3268,7 +3268,7 @@ clientHttpConnectionsOpen(void) } if (s->flags.tunnelSslBumping) { // Create ssl_ctx cache for this port. - Ssl::TheGlobalContextStorage.addLocalStorage(s->s, s->secure.dynamicCertMemCacheSize); + Ssl::TheGlobalContextStorage().addLocalStorage(s->s, s->secure.dynamicCertMemCacheSize); } } #endif diff --git a/src/main.cc b/src/main.cc index f673d50ff6..deaa3dc901 100644 --- a/src/main.cc +++ b/src/main.cc @@ -857,7 +857,7 @@ mainReconfigureStart(void) htcpClosePorts(); #endif #if USE_OPENSSL - Ssl::TheGlobalContextStorage.reconfigureStart(); + Ssl::TheGlobalContextStorage().reconfigureStart(); #endif #if USE_AUTH authenticateReset(); diff --git a/src/security/PeerOptions.cc b/src/security/PeerOptions.cc index 62bc08d046..d505ef475b 100644 --- a/src/security/PeerOptions.cc +++ b/src/security/PeerOptions.cc @@ -21,7 +21,12 @@ #include -Security::PeerOptions Security::ProxyOutgoingConfig; +Security::PeerOptions & +Security::ProxyOutgoingConfig() +{ + static const auto peerOptions = new PeerOptions(); + return *peerOptions; +} Security::PeerOptions::PeerOptions() { diff --git a/src/security/PeerOptions.h b/src/security/PeerOptions.h index 5db3d5a716..984a9afe1d 100644 --- a/src/security/PeerOptions.h +++ b/src/security/PeerOptions.h @@ -161,13 +161,13 @@ public: }; /// configuration options for DIRECT server access -extern PeerOptions ProxyOutgoingConfig; +PeerOptions &ProxyOutgoingConfig(); } // namespace Security // parse the tls_outgoing_options directive void parse_securePeerOptions(Security::PeerOptions *); -#define free_securePeerOptions(x) Security::ProxyOutgoingConfig.clear() +#define free_securePeerOptions(x) Security::ProxyOutgoingConfig().clear() #define dump_securePeerOptions(e,n,x) do { PackableStream os_(*(e)); os_ << n; (x).dumpCfg(os_,""); os_ << '\n'; } while (false) #endif /* SQUID_SRC_SECURITY_PEEROPTIONS_H */ diff --git a/src/ssl/context_storage.cc b/src/ssl/context_storage.cc index 36d549c382..a17a3f0b8b 100644 --- a/src/ssl/context_storage.cc +++ b/src/ssl/context_storage.cc @@ -41,9 +41,9 @@ void Ssl::CertificateStorageAction::dump (StoreEntry *sentry) stream << "Port" << delimiter << "Max mem(KB)" << delimiter << "Cert number" << delimiter << "KB/cert" << delimiter << "Mem used(KB)" << delimiter << "Mem free(KB)" << endString; // Add info for each port. - for (std::map::iterator i = TheGlobalContextStorage.storage.begin(); i != TheGlobalContextStorage.storage.end(); ++i) { - stream << i->first << delimiter; - LocalContextStorage & ssl_store_policy(*(i->second)); + for (const auto &i: TheGlobalContextStorage().storage) { + stream << i.first << delimiter; + const auto &ssl_store_policy = *i.second; const auto memoryPerEntry = ssl_store_policy.entries() ? ssl_store_policy.memoryUsed() / ssl_store_policy.entries() : 0; stream << ssl_store_policy.memLimit() / 1024 << delimiter; @@ -120,5 +120,10 @@ void Ssl::GlobalContextStorage::reconfigureFinish() } } -Ssl::GlobalContextStorage Ssl::TheGlobalContextStorage; +Ssl::GlobalContextStorage & +Ssl::TheGlobalContextStorage() +{ + static const auto storage = new GlobalContextStorage(); + return *storage; +} diff --git a/src/ssl/context_storage.h b/src/ssl/context_storage.h index 4dd49968c4..caac510ba5 100644 --- a/src/ssl/context_storage.h +++ b/src/ssl/context_storage.h @@ -74,7 +74,7 @@ private: }; /// Global cache for store all SSL server certificates. -extern GlobalContextStorage TheGlobalContextStorage; +GlobalContextStorage &TheGlobalContextStorage(); } //namespace Ssl #endif // USE_OPENSSL diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc index 42b4594b2e..7063546918 100644 --- a/src/ssl/helper.cc +++ b/src/ssl/helper.cc @@ -57,6 +57,14 @@ typedef std::unordered_map GeneratorRequests; static void HandleGeneratorReply(void *data, const ::Helper::Reply &reply); +/// pending Ssl::Helper requests (to all certificate generator helpers combined) +static GeneratorRequests & +TheGeneratorRequests() +{ + static auto generatorRequests = new GeneratorRequests(); + return *generatorRequests; +} + } // namespace Ssl CBDATA_NAMESPACED_CLASS_INIT(Ssl, GeneratorRequest); @@ -68,9 +76,6 @@ operator <<(std::ostream &os, const Ssl::GeneratorRequest &gr) return os << "crtGenRq" << gr.query.id.value << "/" << gr.requestors.size(); } -/// pending Ssl::Helper requests (to all certificate generator helpers combined) -static Ssl::GeneratorRequests TheGeneratorRequests; - Helper::Client::Pointer Ssl::Helper::ssl_crtd; void Ssl::Helper::Init() @@ -125,8 +130,8 @@ void Ssl::Helper::Submit(CrtdMessage const & message, HLPCB * callback, void * d SBuf rawMessage(message.compose().c_str()); // XXX: helpers cannot use SBuf rawMessage.append("\n", 1); - const auto pending = TheGeneratorRequests.find(rawMessage); - if (pending != TheGeneratorRequests.end()) { + const auto pending = TheGeneratorRequests().find(rawMessage); + if (pending != TheGeneratorRequests().end()) { pending->second->emplace(callback, data); debugs(83, 5, "collapsed request from " << data << " onto " << *pending->second); return; @@ -135,7 +140,7 @@ void Ssl::Helper::Submit(CrtdMessage const & message, HLPCB * callback, void * d GeneratorRequest *request = new GeneratorRequest; request->query = rawMessage; request->emplace(callback, data); - TheGeneratorRequests.emplace(request->query, request); + TheGeneratorRequests().emplace(request->query, request); debugs(83, 5, "request from " << data << " as " << *request); // ssl_crtd becomes nil if Squid is reconfigured without SslBump or // certificate generation disabled in the new configuration @@ -153,7 +158,7 @@ Ssl::HandleGeneratorReply(void *data, const ::Helper::Reply &reply) { const std::unique_ptr request(static_cast(data)); assert(request); - const auto erased = TheGeneratorRequests.erase(request->query); + const auto erased = TheGeneratorRequests().erase(request->query); assert(erased); for (auto &requestor: request->requestors) { diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 7026301b27..d0e57fc1e2 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -41,8 +41,6 @@ // TODO: Move ssl_ex_index_* global variables from global.cc here. static int ssl_ex_index_verify_callback_parameters = -1; -static Ssl::CertsIndexedList SquidUntrustedCerts; - const EVP_MD *Ssl::DefaultSignHash = nullptr; std::vector Ssl::BumpModeStr = { @@ -73,6 +71,13 @@ protected: AnyP::Host needle_; ///< a name we are looking for }; +static CertsIndexedList & +SquidUntrustedCerts() +{ + static auto untrustedCerts = new CertsIndexedList(); + return *untrustedCerts; +} + } // namespace Ssl bool @@ -1306,7 +1311,7 @@ Ssl::findIssuerCertificate(X509 *cert, const STACK_OF(X509) *serverCertificates, } // check untrusted certificates - if (const auto issuer = findCertIssuerFast(SquidUntrustedCerts, cert)) { + if (const auto issuer = findCertIssuerFast(SquidUntrustedCerts(), cert)) { X509_up_ref(issuer); return Security::CertPointer(issuer); } @@ -1347,7 +1352,7 @@ static void completeIssuers(X509_STORE_CTX *ctx, STACK_OF(X509) &untrustedCerts) { debugs(83, 2, "completing " << sk_X509_num(&untrustedCerts) << - " OpenSSL untrusted certs using " << SquidUntrustedCerts.size() << + " OpenSSL untrusted certs using " << Ssl::SquidUntrustedCerts().size() << " configured untrusted certificates"); const X509_VERIFY_PARAM *param = X509_STORE_CTX_get0_param(ctx); @@ -1397,7 +1402,7 @@ VerifyCtxCertificates(X509_STORE_CTX *ctx, STACK_OF(X509) *extraCerts) // If the local untrusted certificates internal database is used // run completeIssuers to add missing certificates if possible. - if (SquidUntrustedCerts.size() > 0) + if (Ssl::SquidUntrustedCerts().size() > 0) completeIssuers(ctx, *untrustedCerts); X509_STORE_CTX_set0_untrusted(ctx, untrustedCerts.get()); // No locking/unlocking, just sets ctx->untrusted @@ -1441,17 +1446,17 @@ Ssl::useSquidUntrusted(SSL_CTX *sslContext) bool Ssl::loadSquidUntrusted(const char *path) { - return Ssl::loadCerts(path, SquidUntrustedCerts); + return Ssl::loadCerts(path, SquidUntrustedCerts()); } void Ssl::unloadSquidUntrusted() { - if (SquidUntrustedCerts.size()) { - for (Ssl::CertsIndexedList::iterator it = SquidUntrustedCerts.begin(); it != SquidUntrustedCerts.end(); ++it) { - X509_free(it->second); + if (SquidUntrustedCerts().size()) { + for (const auto &i: SquidUntrustedCerts()) { + X509_free(i.second); } - SquidUntrustedCerts.clear(); + SquidUntrustedCerts().clear(); } } diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index ee57bca870..f7314a6c24 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -112,7 +112,7 @@ EncryptorAnswer &PeerConnector::answer() STUB_RETREF(EncryptorAnswer) } #include "security/PeerOptions.h" -Security::PeerOptions Security::ProxyOutgoingConfig; +Security::PeerOptions &Security::ProxyOutgoingConfig() STUB_RETREF(Security::PeerOptions) Security::PeerOptions::PeerOptions() { #if USE_OPENSSL diff --git a/src/tests/stub_libsslsquid.cc b/src/tests/stub_libsslsquid.cc index a3cd170e6b..0ef8a4cccc 100644 --- a/src/tests/stub_libsslsquid.cc +++ b/src/tests/stub_libsslsquid.cc @@ -40,7 +40,6 @@ void Ssl::GlobalContextStorage::addLocalStorage(Ip::Address const &, size_t ) ST Ssl::LocalContextStorage *Ssl::GlobalContextStorage::getLocalStorage(Ip::Address const &) { fatal(STUB_API " required"); static LocalContextStorage v(0); return &v; } void Ssl::GlobalContextStorage::reconfigureStart() STUB -//Ssl::GlobalContextStorage Ssl::TheGlobalContextStorage; #include "ssl/ErrorDetail.h" #include "ssl/support.h"