From: Christos Tsantilas Date: Mon, 16 Apr 2018 14:46:08 +0000 (+0000) Subject: Avoid ssl/helper.cc "ssl_crtd" assertions on reconfiguration (#186) X-Git-Tag: M-staged-PR186 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=23da195f75b394d00ddac4fa67ce6895d96292d7;p=thirdparty%2Fsquid.git Avoid ssl/helper.cc "ssl_crtd" assertions on reconfiguration (#186) Reconfiguration process consists of mainReconfigureStart() and mainReconfigureFinish() steps separated by at least one main loop iteration. Clearing a Squid global variable in mainReconfigureStart() creates two problems for transactions that were started before reconfiguration: 1. Transactions accessing that global _during_ reconfiguration loop iteration(s) may be confused by the variable sudden disappearance. 2. Transactions accessing that global _after_ mainReconfigureFinish() may be confused by the variable disappearance if reconfiguration resulted in the global variable becoming nil. To remove the first problem for ssl_crtd, external_acl, and redirecting helpers, all of them are now reconfigured "instantly", during mainReconfigureFinish(). To prevent crashes due to the second problem, Squid now generates helper errors if the disappeared ssl_crtd or external_acl helpers are accessed after reconfiguration. The admin is warned about such problems via level-1 cache.log ERROR messages. The second problem cannot be fully solved without storing (refcounted) configuration globals inside each transaction that uses them. Such serious changes are outside this small assertion-fixing project scope. This is a Measurement Factory project. --- diff --git a/src/client_side.cc b/src/client_side.cc index d2cbf30862..c4465aaeb9 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -3024,7 +3024,7 @@ ConnStateData::getSslContextStart() request_message.setCode(Ssl::CrtdMessage::code_new_certificate); request_message.composeRequest(certProperties); debugs(33, 5, HERE << "SSL crtd request: " << request_message.compose().c_str()); - Ssl::Helper::GetInstance()->sslSubmit(request_message, sslCrtdHandleReplyWrapper, this); + Ssl::Helper::Submit(request_message, sslCrtdHandleReplyWrapper, this); return; } catch (const std::exception &e) { debugs(33, DBG_IMPORTANT, "ERROR: Failed to compose ssl_crtd " << diff --git a/src/external_acl.cc b/src/external_acl.cc index 4e2deebb99..1b3ea43a88 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -991,6 +991,8 @@ externalAclHandleReply(void *data, const Helper::Reply &reply) entryData.password = label; #endif + // XXX: This state->def access conflicts with the cbdata validity check + // below. dlinkDelete(&state->list, &state->def->queue); ExternalACLEntryPointer entry; diff --git a/src/main.cc b/src/main.cc index 91ed2d4b86..869c76abef 100644 --- a/src/main.cc +++ b/src/main.cc @@ -880,15 +880,9 @@ mainReconfigureStart(void) #if USE_HTCP htcpClosePorts(); #endif -#if USE_SSL_CRTD - Ssl::Helper::GetInstance()->Shutdown(); -#endif #if USE_OPENSSL - if (Ssl::CertValidationHelper::GetInstance()) - Ssl::CertValidationHelper::GetInstance()->Shutdown(); Ssl::TheGlobalContextStorage.reconfigureStart(); #endif - redirectShutdown(); #if USE_AUTH authenticateReset(); #endif @@ -976,14 +970,13 @@ mainReconfigureFinish(void *) storeLogOpen(); Dns::Init(); #if USE_SSL_CRTD - Ssl::Helper::GetInstance()->Init(); + Ssl::Helper::Reconfigure(); #endif #if USE_OPENSSL - if (Ssl::CertValidationHelper::GetInstance()) - Ssl::CertValidationHelper::GetInstance()->Init(); + Ssl::CertValidationHelper::Reconfigure(); #endif - redirectInit(); + redirectReconfigure(); #if USE_AUTH authenticateInit(&Auth::TheConfig.schemes); #endif @@ -1186,12 +1179,11 @@ mainInitialize(void) Dns::Init(); #if USE_SSL_CRTD - Ssl::Helper::GetInstance()->Init(); + Ssl::Helper::Init(); #endif #if USE_OPENSSL - if (Ssl::CertValidationHelper::GetInstance()) - Ssl::CertValidationHelper::GetInstance()->Init(); + Ssl::CertValidationHelper::Init(); #endif redirectInit(); @@ -2063,11 +2055,10 @@ SquidShutdown() debugs(1, DBG_IMPORTANT, "Shutting down..."); #if USE_SSL_CRTD - Ssl::Helper::GetInstance()->Shutdown(); + Ssl::Helper::Shutdown(); #endif #if USE_OPENSSL - if (Ssl::CertValidationHelper::GetInstance()) - Ssl::CertValidationHelper::GetInstance()->Shutdown(); + Ssl::CertValidationHelper::Shutdown(); #endif redirectShutdown(); externalAclShutdown(); diff --git a/src/redirect.cc b/src/redirect.cc index 4e16e03f41..3f741622f6 100644 --- a/src/redirect.cc +++ b/src/redirect.cc @@ -434,3 +434,9 @@ redirectShutdown(void) storeIdExtrasFmt = NULL; } +void +redirectReconfigure() +{ + redirectShutdown(); + redirectInit(); +} diff --git a/src/redirect.h b/src/redirect.h index 8c204c624d..edc2d33bae 100644 --- a/src/redirect.h +++ b/src/redirect.h @@ -19,6 +19,7 @@ class ClientHttpRequest; void redirectInit(void); void redirectShutdown(void); +void redirectReconfigure(); void redirectStart(ClientHttpRequest *, HLPCB *, void *); void storeIdStart(ClientHttpRequest *, HLPCB *, void *); diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index bfa82c1adc..d3d99d5e65 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -241,7 +241,7 @@ Security::PeerConnector::sslFinalized() try { debugs(83, 5, "Sending SSL certificate for validation to ssl_crtvd."); AsyncCall::Pointer call = asyncCall(83,5, "Security::PeerConnector::sslCrtvdHandleReply", Ssl::CertValidationHelper::CbDialer(this, &Security::PeerConnector::sslCrtvdHandleReply, nullptr)); - Ssl::CertValidationHelper::GetInstance()->sslSubmit(validationRequest, call); + Ssl::CertValidationHelper::Submit(validationRequest, call); return false; } catch (const std::exception &e) { debugs(83, DBG_IMPORTANT, "ERROR: Failed to compose ssl_crtvd " << diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc index 2763bad304..e6aa41ddfa 100644 --- a/src/ssl/helper.cc +++ b/src/ssl/helper.cc @@ -67,20 +67,7 @@ operator <<(std::ostream &os, const Ssl::GeneratorRequest &gr) /// pending Ssl::Helper requests (to all certificate generator helpers combined) static Ssl::GeneratorRequests TheGeneratorRequests; -Ssl::Helper * Ssl::Helper::GetInstance() -{ - static Ssl::Helper sslHelper; - return &sslHelper; -} - -Ssl::Helper::Helper() : ssl_crtd(NULL) -{ -} - -Ssl::Helper::~Helper() -{ - Shutdown(); -} +helper *Ssl::Helper::ssl_crtd = nullptr; void Ssl::Helper::Init() { @@ -123,10 +110,15 @@ void Ssl::Helper::Shutdown() ssl_crtd = NULL; } -void Ssl::Helper::sslSubmit(CrtdMessage const & message, HLPCB * callback, void * data) +void +Ssl::Helper::Reconfigure() { - assert(ssl_crtd); + Shutdown(); + Init(); +} +void Ssl::Helper::Submit(CrtdMessage const & message, HLPCB * callback, void * data) +{ SBuf rawMessage(message.compose().c_str()); // XXX: helpers cannot use SBuf rawMessage.append("\n", 1); @@ -142,7 +134,9 @@ void Ssl::Helper::sslSubmit(CrtdMessage const & message, HLPCB * callback, void request->emplace(callback, data); TheGeneratorRequests.emplace(request->query, request); debugs(83, 5, "request from " << data << " as " << *request); - if (ssl_crtd->trySubmit(request->query.c_str(), HandleGeneratorReply, request)) + // ssl_crtd becomes nil if Squid is reconfigured without SslBump or + // certificate generation disabled in the new configuration + if (ssl_crtd && ssl_crtd->trySubmit(request->query.c_str(), HandleGeneratorReply, request)) return; ::Helper::Reply failReply(::Helper::BrokenHelper); @@ -168,25 +162,13 @@ Ssl::HandleGeneratorReply(void *data, const ::Helper::Reply &reply) } #endif //USE_SSL_CRTD -Ssl::CertValidationHelper * Ssl::CertValidationHelper::GetInstance() -{ - static Ssl::CertValidationHelper sslHelper; - if (!Ssl::TheConfig.ssl_crt_validator) - return NULL; - return &sslHelper; -} - -Ssl::CertValidationHelper::CertValidationHelper() : ssl_crt_validator(NULL) -{ -} - -Ssl::CertValidationHelper::~CertValidationHelper() -{ - Shutdown(); -} +helper *Ssl::CertValidationHelper::ssl_crt_validator = nullptr; void Ssl::CertValidationHelper::Init() { + if (!Ssl::TheConfig.ssl_crt_validator) + return; + assert(ssl_crt_validator == NULL); // we need to start ssl_crtd only if some port(s) need to bump SSL @@ -249,6 +231,13 @@ void Ssl::CertValidationHelper::Shutdown() HelperCache = NULL; } +void +Ssl::CertValidationHelper::Reconfigure() +{ + Shutdown(); + Init(); +} + class submitData { CBDATA_CLASS(submitData); @@ -298,10 +287,8 @@ sslCrtvdHandleReplyWrapper(void *data, const ::Helper::Reply &reply) delete crtdvdData; } -void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &request, AsyncCall::Pointer &callback) +void Ssl::CertValidationHelper::Submit(Ssl::CertValidationRequest const &request, AsyncCall::Pointer &callback) { - assert(ssl_crt_validator); - Ssl::CertValidationMsg message(Ssl::CrtdMessage::REQUEST); message.setCode(Ssl::CertValidationMsg::code_cert_validate); message.composeRequest(request); @@ -325,15 +312,18 @@ void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &requ return; } - if (!ssl_crt_validator->trySubmit(crtdvdData->query.c_str(), sslCrtvdHandleReplyWrapper, crtdvdData)) { - Ssl::CertValidationResponse::Pointer resp = new Ssl::CertValidationResponse(crtdvdData->ssl); - resp->resultCode = ::Helper::BrokenHelper; - Ssl::CertValidationHelper::CbDialer *dialer = dynamic_cast(callback->getDialer()); - Must(dialer); - dialer->arg1 = resp; - ScheduleCallHere(callback); - delete crtdvdData; + // ssl_crt_validator becomes nil if Squid is reconfigured with cert + // validator disabled in the new configuration + if (ssl_crt_validator && ssl_crt_validator->trySubmit(crtdvdData->query.c_str(), sslCrtvdHandleReplyWrapper, crtdvdData)) return; - } + + Ssl::CertValidationResponse::Pointer resp = new Ssl::CertValidationResponse(crtdvdData->ssl); + resp->resultCode = ::Helper::BrokenHelper; + Ssl::CertValidationHelper::CbDialer *dialer = dynamic_cast(callback->getDialer()); + Must(dialer); + dialer->arg1 = resp; + ScheduleCallHere(callback); + delete crtdvdData; + return; } diff --git a/src/ssl/helper.h b/src/ssl/helper.h index 217021047c..5f958ad95e 100644 --- a/src/ssl/helper.h +++ b/src/ssl/helper.h @@ -22,23 +22,19 @@ namespace Ssl { #if USE_SSL_CRTD /** - * Set of thread for ssl_crtd. This class is singleton. Use this class only - * over GetIntance() static method. This class use helper structure - * for threads management. + * Set of thread for ssl_crtd. This class is singleton. + * This class use helper structure for threads management. */ class Helper { public: - static Helper * GetInstance(); ///< Instance class. - void Init(); ///< Init helper structure. - void Shutdown(); ///< Shutdown helper structure. + static void Init(); ///< Init helper structure. + static void Shutdown(); ///< Shutdown helper structure. + static void Reconfigure(); ///< Reconfigure helper structure. /// Submit crtd message to external crtd server. - void sslSubmit(CrtdMessage const & message, HLPCB * callback, void *data); + static void Submit(CrtdMessage const & message, HLPCB * callback, void *data); private: - Helper(); - ~Helper(); - - helper * ssl_crtd; ///< helper for management of ssl_crtd. + static helper * ssl_crtd; ///< helper for management of ssl_crtd. }; #endif @@ -50,16 +46,13 @@ public: typedef UnaryMemFunT CbDialer; typedef void CVHCB(void *, Ssl::CertValidationResponse const &); - static CertValidationHelper * GetInstance(); ///< Instance class. - void Init(); ///< Init helper structure. - void Shutdown(); ///< Shutdown helper structure. + static void Init(); ///< Init helper structure. + static void Shutdown(); ///< Shutdown helper structure. + static void Reconfigure(); ///< Reconfigure helper structure /// Submit crtd request message to external crtd server. - void sslSubmit(Ssl::CertValidationRequest const & request, AsyncCall::Pointer &); + static void Submit(Ssl::CertValidationRequest const & request, AsyncCall::Pointer &); private: - CertValidationHelper(); - ~CertValidationHelper(); - - helper * ssl_crt_validator; ///< helper for management of ssl_crtd. + static helper * ssl_crt_validator; ///< helper for management of ssl_crtd. public: typedef LruMap LruCache; static LruCache *HelperCache; ///< cache for cert validation helper diff --git a/src/ssl/stub_libsslutil.cc b/src/ssl/stub_libsslutil.cc index e8cedaa4ed..58728a5589 100644 --- a/src/ssl/stub_libsslutil.cc +++ b/src/ssl/stub_libsslutil.cc @@ -37,8 +37,7 @@ void Ssl::readCertAndPrivateKeyFromFiles(Security::CertPointer &, Security::Priv bool Ssl::sslDateIsInTheFuture(char const *) STUB_RETVAL(false) #include "ssl/helper.h" -Ssl::Helper * Ssl::Helper::GetInstance() STUB_RETVAL(NULL) void Ssl::Helper::Init() STUB void Ssl::Helper::Shutdown() STUB -void Ssl::Helper::sslSubmit(Ssl::CrtdMessage const & message, HLPCB * callback, void *data) STUB +void Ssl::Helper::Submit(Ssl::CrtdMessage const & message, HLPCB * callback, void *data) STUB