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 bb96bf2808c4acdd7e99564440291774c35d7eef..05eb212eabbf7cb85f0f7a8a34af570714d892ea 100644 (file)
--- 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<ACLFilledChecklist *>(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 69b94ee8ebdb78ce782551650ebd3f8cc159f5c6..f7b6ee28364399986d39f9df5b5337c44ecb04e0 100644 (file)
--- 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<T, UnLocker, lockId> SelfType;
+    typedef Security::LockingPointer<T, UnLocker, lockId> 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 97cb67a79bf5f116028fa90ed36ae7adfb00aeb4..28734b64babe25e53f2e62ef21a4c9618f9bde52 100644 (file)
--- 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 304279dc9a841ecf449e2b5f539ff058c6e482d0..9284c7b783dfb7126de3d76d6ce6aa8a7e6ee685 100644 (file)
--- 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 dec4a337f8d7a3fef42b580933176a64f8b1e2ff..cfe1902aab7c63d159a8a413fabc4ff14ce8a15f 100644 (file)
--- 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<Ssl::ClientBio *>(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<Ssl::ServerBio *>(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 4dbc122c39dbf3706654abf5a252e51aee869f71..5bacd0fb5c8c5974c64a5d5f5eeabe592f65a588 100644 (file)
--- 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 f4a936d1345e2c3bddde945fc330f507064a87ca..49bbaa90bfef01b84e7ae3c481d90eb9390f844b 100644 (file)
--- 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 551c0b8560acfa77ffec1bca4c1735a24a318166..35c99dc9d797ef4484d6e8f92e3be48766dadd6a 100644 (file)
--- 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 <iosfwd>
@@ -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.