From: Amos Jeffries Date: Wed, 13 Jul 2016 12:39:22 +0000 (+1200) Subject: Cleanup: PeerConnector session initialization method X-Git-Tag: SQUID_4_0_13~33 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=eba8d9bb175bb274554853a3babae8c63f86443f;p=thirdparty%2Fsquid.git Cleanup: PeerConnector session initialization method * rename initializeSsl() method to initializeTls() * use SessionPointer instead of SessionPtr * return value is best used in boolean expressions, so make it bool this also resolves some template issues with SessionPointer return type. * rename the session parameter to serverSession which better documents what it is than 'ssl', and distinguishes it from clientSession in child methods which have to deal with both. --- diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index bb96bf2808..05eb212eab 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -62,7 +62,7 @@ public: PeerConnector(aServerConn, aCallback, alp, timeout), icapService(service) {} /* PeerConnector API */ - virtual Security::SessionPtr initializeSsl(); + virtual bool initializeTls(Security::SessionPointer &); virtual void noteNegotiationDone(ErrorState *error); virtual Security::ContextPtr getSslContext() {return icapService->sslContext;} @@ -710,24 +710,22 @@ bool Adaptation::Icap::Xaction::fillVirginHttpHeader(MemBuf &) const } #if USE_OPENSSL -Security::SessionPtr -Ssl::IcapPeerConnector::initializeSsl() +bool +Ssl::IcapPeerConnector::initializeTls(Security::SessionPointer &serverSession) { - auto ssl = Ssl::PeerConnector::initializeSsl(); - if (!ssl) - return nullptr; + if (!Ssl::PeerConnector::initializeTls(serverSession)) + return false; assert(!icapService->cfg().secure.sslDomain.isEmpty()); SBuf *host = new SBuf(icapService->cfg().secure.sslDomain); - SSL_set_ex_data(ssl, ssl_ex_index_server, host); + SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, host); - ACLFilledChecklist *check = (ACLFilledChecklist *)SSL_get_ex_data(ssl, ssl_ex_index_cert_error_check); + ACLFilledChecklist *check = static_cast(SSL_get_ex_data(serverSession.get(), ssl_ex_index_cert_error_check)); if (check) check->dst_peer_name = *host; - Security::GetSessionResumeData(Security::SessionPointer(ssl), icapService->sslSession); - - return ssl; + Security::GetSessionResumeData(serverSession, icapService->sslSession); + return true; } void diff --git a/src/security/LockingPointer.h b/src/security/LockingPointer.h index 69b94ee8eb..f7b6ee2836 100644 --- a/src/security/LockingPointer.h +++ b/src/security/LockingPointer.h @@ -49,7 +49,7 @@ class LockingPointer { public: /// a helper label to simplify this objects API definitions below - typedef LockingPointer SelfType; + typedef Security::LockingPointer SelfType; /** * Construct directly from a raw pointer. @@ -66,8 +66,10 @@ public: ~LockingPointer() { unlock(); } // copy semantics are okay only when adding a lock reference - explicit LockingPointer(const SelfType &o) : raw(nullptr) { resetAndLock(o.get()); } - SelfType &operator =(const SelfType & o) { + explicit LockingPointer(const SelfType &o) : raw(nullptr) { + resetAndLock(o.get()); + } + const SelfType &operator =(const SelfType &o) { resetAndLock(o.get()); return *this; } diff --git a/src/ssl/BlindPeerConnector.cc b/src/ssl/BlindPeerConnector.cc index 97cb67a79b..28734b64ba 100644 --- a/src/ssl/BlindPeerConnector.cc +++ b/src/ssl/BlindPeerConnector.cc @@ -29,12 +29,11 @@ Ssl::BlindPeerConnector::getSslContext() return ::Config.ssl_client.sslContext; } -Security::SessionPtr -Ssl::BlindPeerConnector::initializeSsl() +bool +Ssl::BlindPeerConnector::initializeTls(Security::SessionPointer &serverSession) { - auto ssl = Ssl::PeerConnector::initializeSsl(); - if (!ssl) - return nullptr; + if (!Ssl::PeerConnector::initializeTls(serverSession)) + return false; if (const CachePeer *peer = serverConnection()->getPeer()) { assert(peer); @@ -44,15 +43,15 @@ Ssl::BlindPeerConnector::initializeSsl() // const loss is okay here, ssl_ex_index_server is only read and not assigned a destructor SBuf *host = new SBuf(peer->secure.sslDomain); - SSL_set_ex_data(ssl, ssl_ex_index_server, host); + SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, host); - Security::SetSessionResumeData(ssl, peer->sslSession); + Security::SetSessionResumeData(serverSession.get(), peer->sslSession); } else { SBuf *hostName = new SBuf(request->url.host()); - SSL_set_ex_data(ssl, ssl_ex_index_server, (void*)hostName); + SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName); } - return ssl; + return true; } void diff --git a/src/ssl/BlindPeerConnector.h b/src/ssl/BlindPeerConnector.h index 304279dc9a..9284c7b783 100644 --- a/src/ssl/BlindPeerConnector.h +++ b/src/ssl/BlindPeerConnector.h @@ -33,9 +33,10 @@ public: /* PeerConnector API */ - /// Calls parent initializeSSL, configure the created SSL object to try reuse SSL session - /// and sets the hostname to use for certificates validation - virtual Security::SessionPtr initializeSsl(); + /// Calls parent initializeTls(), configure the created TLS session object to + /// try reuse TLS session and sets the hostname to use for certificates validation + /// \returns true on successful initialization + virtual bool initializeTls(Security::SessionPointer &); /// Return the configured Security::ContextPtr object virtual Security::ContextPtr getSslContext(); diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index dec4a337f8..cfe1902aab 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -133,12 +133,11 @@ Ssl::PeekingPeerConnector::getSslContext() return ::Config.ssl_client.sslContext; } -Security::SessionPtr -Ssl::PeekingPeerConnector::initializeSsl() +bool +Ssl::PeekingPeerConnector::initializeTls(Security::SessionPointer &serverSession) { - auto ssl = Ssl::PeerConnector::initializeSsl(); - if (!ssl) - return nullptr; + if (!Ssl::PeerConnector::initializeTls(serverSession)) + return false; if (ConnStateData *csd = request->clientConnectionManager.valid()) { @@ -147,8 +146,8 @@ Ssl::PeekingPeerConnector::initializeSsl() assert(clientConn != NULL); SBuf *hostName = NULL; - //Enable Status_request tls extension, required to bump some clients - SSL_set_tlsext_status_type(ssl, TLSEXT_STATUSTYPE_ocsp); + //Enable Status_request TLS extension, required to bump some clients + SSL_set_tlsext_status_type(serverSession.get(), TLSEXT_STATUSTYPE_ocsp); const Security::TlsDetails::Pointer details = csd->tlsParser.details; if (details && !details->serverName.isEmpty()) @@ -164,20 +163,20 @@ Ssl::PeekingPeerConnector::initializeSsl() } if (hostName) - SSL_set_ex_data(ssl, ssl_ex_index_server, (void*)hostName); + SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName); Must(!csd->serverBump() || csd->serverBump()->step <= Ssl::bumpStep2); if (csd->sslBumpMode == Ssl::bumpPeek || csd->sslBumpMode == Ssl::bumpStare) { - auto clientSsl = fd_table[clientConn->fd].ssl.get(); - Must(clientSsl); - BIO *bc = SSL_get_rbio(clientSsl); + auto clientSession = fd_table[clientConn->fd].ssl.get(); + Must(clientSession); + BIO *bc = SSL_get_rbio(clientSession); Ssl::ClientBio *cltBio = static_cast(bc->ptr); Must(cltBio); if (details && details->tlsVersion.protocol != AnyP::PROTO_NONE) { - applyTlsDetailsToSSL(ssl, details, csd->sslBumpMode); + applyTlsDetailsToSSL(serverSession.get(), details, csd->sslBumpMode); // Should we allow it for all protocols? if (details->tlsVersion.protocol == AnyP::PROTO_TLS || details->tlsVersion == AnyP::ProtocolVersion(AnyP::PROTO_SSL, 3, 0)) { - BIO *b = SSL_get_rbio(ssl); + BIO *b = SSL_get_rbio(serverSession.get()); Ssl::ServerBio *srvBio = static_cast(b->ptr); // Inherite client features, like SSL version, SNI and other srvBio->setClientFeatures(details, cltBio->rBufData()); @@ -187,7 +186,7 @@ Ssl::PeekingPeerConnector::initializeSsl() } } else { // Set client SSL options - SSL_set_options(ssl, ::Security::ProxyOutgoingConfig.parsedOptions); + SSL_set_options(serverSession.get(), ::Security::ProxyOutgoingConfig.parsedOptions); // Use SNI TLS extension only when we connect directly // to the origin server and we know the server host name. @@ -199,20 +198,20 @@ Ssl::PeekingPeerConnector::initializeSsl() sniServer = hostName->c_str(); if (sniServer) - Ssl::setClientSNI(ssl, sniServer); + Ssl::setClientSNI(serverSession.get(), sniServer); } if (Ssl::ServerBump *serverBump = csd->serverBump()) { - serverBump->attachServerSSL(ssl); + serverBump->attachServerSSL(serverSession.get()); // store peeked cert to check SQUID_X509_V_ERR_CERT_CHANGE if (X509 *peeked_cert = serverBump->serverCert.get()) { CRYPTO_add(&(peeked_cert->references),1,CRYPTO_LOCK_X509); - SSL_set_ex_data(ssl, ssl_ex_index_ssl_peeked_cert, peeked_cert); + SSL_set_ex_data(serverSession.get(), ssl_ex_index_ssl_peeked_cert, peeked_cert); } } } - return ssl; + return true; } void diff --git a/src/ssl/PeekingPeerConnector.h b/src/ssl/PeekingPeerConnector.h index 4dbc122c39..5bacd0fb5c 100644 --- a/src/ssl/PeekingPeerConnector.h +++ b/src/ssl/PeekingPeerConnector.h @@ -37,7 +37,7 @@ public: } /* PeerConnector API */ - virtual Security::SessionPtr initializeSsl(); + virtual bool initializeTls(Security::SessionPointer &); virtual Security::ContextPtr getSslContext(); virtual void noteWantWrite(); virtual void noteSslNegotiationError(const int result, const int ssl_error, const int ssl_lib_error); diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc index f4a936d134..49bbaa90bf 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -53,7 +53,8 @@ Ssl::PeerConnector::start() { AsyncJob::start(); - if (prepareSocket() && initializeSsl()) + Security::SessionPointer tmp; + if (prepareSocket() && initializeTls(tmp)) negotiateSsl(); } @@ -87,8 +88,8 @@ Ssl::PeerConnector::prepareSocket() return true; } -Security::SessionPtr -Ssl::PeerConnector::initializeSsl() +bool +Ssl::PeerConnector::initializeTls(Security::SessionPointer &serverSession) { Security::ContextPtr sslContext(getSslContext()); assert(sslContext); @@ -100,11 +101,11 @@ Ssl::PeerConnector::initializeSsl() noteNegotiationDone(anErr); bail(anErr); - return nullptr; + return false; } // A TLS/SSL session has now been created for the connection and stored in fd_table - auto &tlsSession = fd_table[serverConnection()->fd].ssl; + serverSession = fd_table[serverConnection()->fd].ssl; // If CertValidation Helper used do not lookup checklist for errors, // but keep a list of errors to send it to CertValidator @@ -115,10 +116,10 @@ Ssl::PeerConnector::initializeSsl() ACLFilledChecklist *check = new ACLFilledChecklist(acl, request.getRaw(), dash_str); check->al = al; // check->fd(fd); XXX: need client FD here - SSL_set_ex_data(tlsSession.get(), ssl_ex_index_cert_error_check, check); + SSL_set_ex_data(serverSession.get(), ssl_ex_index_cert_error_check, check); } } - return tlsSession.get(); + return true; } void diff --git a/src/ssl/PeerConnector.h b/src/ssl/PeerConnector.h index 551c0b8560..35c99dc9d7 100644 --- a/src/ssl/PeerConnector.h +++ b/src/ssl/PeerConnector.h @@ -14,6 +14,7 @@ #include "base/AsyncJob.h" #include "CommCalls.h" #include "security/EncryptorAnswer.h" +#include "security/forward.h" #include "ssl/support.h" #include @@ -102,7 +103,8 @@ protected: /// silent server void setReadTimeout(); - virtual Security::SessionPtr initializeSsl(); ///< Initializes SSL state + /// \returns true on successful TLS session initialization + virtual bool initializeTls(Security::SessionPointer &); /// Performs a single secure connection negotiation step. /// It is called multiple times untill the negotiation finish or aborted.