Polish peek-and-splice related code

 - Record SSL bump action at each bumping step in the Ssl::ServerBump.
   The new Ssl::ServerBump::act member added for this purpose.

 - Split Ssl::PeerConnector::checkForPeekAndSplice  to two methods
   (checkForPeekAndSplice and checkForPeekAndSpliceDone) add some
   documentation, and polish the code.

 - Polish  httpsSslBumpStep2AccessCheckDone function (client_side.cc file)

This is a Measurement Factory project

diff --git a/src/client_side.cc b/src/client_side.cc
index f22f3b4fd3c6d9946d10c13cd0a5543d2271bc55..4af2cc9baca2a404012383f821a398fef933e4b3 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3954,7 +3954,7 @@ ConnStateData::sslCrtdHandleReply(const Helper::Reply &reply)
                 debugs(33, 5, HERE << "Certificate for " << sslConnectHostOrIp << " cannot be generated. ssl_crtd response: " << reply_message.getBody());
             } else {
                 debugs(33, 5, HERE << "Certificate for " << sslConnectHostOrIp << " was successfully recieved from ssl_crtd");
-                if (sslServerBump && (sslServerBump->mode == Ssl::bumpPeek || sslServerBump->mode == Ssl::bumpStare)) {
+                if (sslServerBump && (sslServerBump->act.step1 == Ssl::bumpPeek || sslServerBump->act.step1 == Ssl::bumpStare)) {
                     doPeekAndSpliceStep();
                     SSL *ssl = fd_table[clientConnection->fd].ssl;
                     bool ret = Ssl::configureSSLUsingPkeyAndCertFromMemory(ssl, reply_message.getBody().c_str(), *port);
@@ -4071,7 +4071,7 @@ ConnStateData::getSslContextStart()
         assert(sslBumpCertKey.size() > 0 && sslBumpCertKey[0] != '\0');
 
         // Disable caching for bumpPeekAndSplice mode
-        if (!(sslServerBump && (sslServerBump->mode == Ssl::bumpPeek || sslServerBump->mode == Ssl::bumpStare))) {
+        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);
             SSL_CTX * dynCtx = NULL;
@@ -4111,7 +4111,7 @@ ConnStateData::getSslContextStart()
 #endif // USE_SSL_CRTD
 
         debugs(33, 5, HERE << "Generating SSL certificate for " << certProperties.commonName);
-        if (sslServerBump && (sslServerBump->mode == Ssl::bumpPeek || sslServerBump->mode == Ssl::bumpStare)) {
+        if (sslServerBump && (sslServerBump->act.step1 == Ssl::bumpPeek || sslServerBump->act.step1 == Ssl::bumpStare)) {
             doPeekAndSpliceStep();
             SSL *ssl = fd_table[clientConnection->fd].ssl;
             if (!Ssl::configureSSL(ssl, certProperties, *port))
@@ -4280,16 +4280,25 @@ void httpsSslBumpStep2AccessCheckDone(allow_t answer, void *data)
         return;
 
     debugs(33, 5, "Answer: " << answer << " kind:" << answer.kind);
-    if (answer == ACCESS_ALLOWED && answer.kind != Ssl::bumpNone && answer.kind != Ssl::bumpSplice) {
-        if (answer.kind == Ssl::bumpTerminate)
-            comm_close(connState->clientConnection->fd);
-        else {
-            if (answer.kind != Ssl::bumpPeek && answer.kind != Ssl::bumpStare)
-                connState->sslBumpMode = Ssl::bumpBump;
-            else
-                connState->sslBumpMode = (Ssl::BumpMode)answer.kind;
-            connState->startPeekAndSpliceDone();
-        }
+    assert(connState->serverBump());
+    Ssl::BumpMode bumpAction;
+    if (answer == ACCESS_ALLOWED) {
+        if (answer.kind == Ssl::bumpNone)
+            bumpAction = Ssl::bumpSplice;
+        else if (answer.kind == Ssl::bumpClientFirst || answer.kind == Ssl::bumpServerFirst)
+            bumpAction = Ssl::bumpBump;
+        else
+            bumpAction = (Ssl::BumpMode)answer.kind;
+    } else
+        bumpAction = Ssl::bumpSplice;
+
+    connState->serverBump()->act.step2 = bumpAction;
+    connState->sslBumpMode = bumpAction;
+
+    if (bumpAction == Ssl::bumpTerminate) {
+        comm_close(connState->clientConnection->fd);
+    } else if (bumpAction != Ssl::bumpSplice) {
+        connState->startPeekAndSpliceDone();
     } else {
         //Normally we can splice here, because we just got client hello message
         SSL *ssl = fd_table[connState->clientConnection->fd].ssl;
@@ -4298,8 +4307,6 @@ void httpsSslBumpStep2AccessCheckDone(allow_t answer, void *data)
         MemBuf const &rbuf = bio->rBufData();
         debugs(83,5, "Bio for  " << connState->clientConnection << " read " << rbuf.contentSize() << " helo bytes");
         // Do splice:
-
-        connState->sslBumpMode = Ssl::bumpSplice;
         fd_table[connState->clientConnection->fd].read_method = &default_read_method;
         fd_table[connState->clientConnection->fd].write_method = &default_write_method;
 

diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc
index fabbda26422caeea18b0621e2ead41ad12a0fba9..82e24a99184e89a43aa36b80abcfa14c4c9de3b4 100644 (file)
--- a/src/ssl/PeerConnector.cc
+++ b/src/ssl/PeerConnector.cc
@@ -287,14 +287,14 @@ Ssl::PeerConnector::negotiateSsl()
 void switchToTunnel(HttpRequest *request, int *status_ptr, Comm::ConnectionPointer & clientConn, Comm::ConnectionPointer &srvConn);
 
 void
-Ssl::PeerConnector::cbCheckForPeekAndSplice(allow_t answer, void *data)
+Ssl::PeerConnector::cbCheckForPeekAndSpliceDone(allow_t answer, void *data)
 {
     Ssl::PeerConnector *peerConnect = (Ssl::PeerConnector *) data;
-    peerConnect->checkForPeekAndSplice(true, (Ssl::BumpMode)answer.kind);
+    peerConnect->checkForPeekAndSpliceDone((Ssl::BumpMode)answer.kind);
 }
 
-bool
-Ssl::PeerConnector::checkForPeekAndSplice(bool checkDone, Ssl::BumpMode peekMode)
+void
+Ssl::PeerConnector::checkForPeekAndSplice()
 {
     SSL *ssl = fd_table[serverConn->fd].ssl;
     // Mark Step3 of bumping
@@ -306,44 +306,48 @@ Ssl::PeerConnector::checkForPeekAndSplice(bool checkDone, Ssl::BumpMode peekMode
         }
     }
 
-    if (!checkDone) {
-        ACLFilledChecklist *acl_checklist = new ACLFilledChecklist(
-            ::Config.accessList.ssl_bump,
-            request.getRaw(), NULL);
-        acl_checklist->nonBlockingCheck(Ssl::PeerConnector::cbCheckForPeekAndSplice, this);
-        return false;
-    }
+    ACLFilledChecklist *acl_checklist = new ACLFilledChecklist(
+        ::Config.accessList.ssl_bump,
+        request.getRaw(), NULL);
+    acl_checklist->nonBlockingCheck(Ssl::PeerConnector::cbCheckForPeekAndSpliceDone, this);
+}
 
+void
+Ssl::PeerConnector::checkForPeekAndSpliceDone(Ssl::BumpMode const action)
+{
+    SSL *ssl = fd_table[serverConn->fd].ssl;
     BIO *b = SSL_get_rbio(ssl);
     Ssl::ServerBio *srvBio = static_cast<Ssl::ServerBio *>(b->ptr);
     debugs(83,5, "Will check for peek and splice on FD " << serverConn->fd);
 
-    // bump, peek, stare, server-first,client-first are all mean bump the connection
-    if (peekMode < Ssl::bumpSplice)
-        peekMode = Ssl::bumpBump;
+    Ssl::BumpMode finalAction = action;
+    // adjust the final bumping mode if needed
+    if (finalAction < Ssl::bumpSplice)
+        finalAction = Ssl::bumpBump;
 
-    if (peekMode == Ssl::bumpSplice && !srvBio->canSplice())
-        peekMode = Ssl::bumpPeek;
-    else if (peekMode == Ssl::bumpBump && !srvBio->canBump())
-        peekMode = Ssl::bumpSplice;
+    if (finalAction == Ssl::bumpSplice && !srvBio->canSplice())
+        finalAction = Ssl::bumpBump;
+    else if (finalAction == Ssl::bumpBump && !srvBio->canBump())
+        finalAction = Ssl::bumpSplice;
 
-    if (peekMode == Ssl::bumpTerminate) {
+    // Record final decision
+    if (request->clientConnectionManager.valid())
+        request->clientConnectionManager->sslBumpMode = finalAction;
+
+    if (finalAction == Ssl::bumpTerminate) {
         comm_close(serverConn->fd);
         comm_close(clientConn->fd);
-    } else if (peekMode != Ssl::bumpSplice) {
+    } else if (finalAction != Ssl::bumpSplice) {
         //Allow write, proceed with the connection
         srvBio->holdWrite(false);
         srvBio->recordInput(false);
         Comm::SetSelect(serverConn->fd, COMM_SELECT_WRITE, &NegotiateSsl, this, 0);
         debugs(83,5, "Retry the fwdNegotiateSSL on FD " << serverConn->fd);
-        return true;
     } else {
         static int status_code = 0;
         debugs(83,5, "Revert to tunnel FD " << clientConn->fd << " with FD " << serverConn->fd);
         switchToTunnel(request.getRaw(), &status_code, clientConn, serverConn);
-        return false;
     }
-    return false;
 }
 
 void
@@ -496,7 +500,7 @@ Ssl::PeerConnector::handleNegotiateError(const int ret)
     case SSL_ERROR_WANT_WRITE:
         if ((request->clientConnectionManager->sslBumpMode == Ssl::bumpPeek || request->clientConnectionManager->sslBumpMode == Ssl::bumpStare) && srvBio->holdWrite()) {
             debugs(81, DBG_IMPORTANT, "hold write on SSL connection on FD " << fd);
-            checkForPeekAndSplice(false, Ssl::bumpNone);
+            checkForPeekAndSplice();
             return;
         }
         Comm::SetSelect(fd, COMM_SELECT_WRITE, &NegotiateSsl, this, 0);
@@ -514,7 +518,7 @@ Ssl::PeerConnector::handleNegotiateError(const int ret)
 #if 1
         if ((request->clientConnectionManager->sslBumpMode == Ssl::bumpPeek  || request->clientConnectionManager->sslBumpMode == Ssl::bumpStare) && srvBio->holdWrite()) {
             debugs(81, 3, "Error ("  << ERR_error_string(ssl_lib_error, NULL) <<  ") but, hold write on SSL connection on FD " << fd);
-            checkForPeekAndSplice(false, Ssl::bumpNone);
+            checkForPeekAndSplice();
             return;
         }
 #endif

diff --git a/src/ssl/PeerConnector.h b/src/ssl/PeerConnector.h
index a8695d7528ab9f8956fde8c3a7530d2743289550..7f9e680621cd77548dd74a1ffe5ea479a3cd1a61 100644 (file)
--- a/src/ssl/PeerConnector.h
+++ b/src/ssl/PeerConnector.h
@@ -116,7 +116,13 @@ protected:
     /// It is called multiple times untill the negotiation finish or aborted.
     void negotiateSsl();
 
-    bool checkForPeekAndSplice(bool, Ssl::BumpMode);
+    /// Initiates the ssl_bump acl check in step3 SSL bump step to decide
+    /// about bumping, splicing or terminating the connection.
+    void checkForPeekAndSplice();
+
+    /// Callback function for ssl_bump acl check in step3  SSL bump step.
+    /// Handles the final bumping decision.
+    void checkForPeekAndSpliceDone(Ssl::BumpMode const);
 
     /// Called when the SSL negotiation step aborted because data needs to
     /// be transferred to/from SSL server or on error. In the first case
@@ -149,8 +155,8 @@ private:
     /// A wrapper function for negotiateSsl for use with Comm::SetSelect
     static void NegotiateSsl(int fd, void *data);
 
-    /// A wrapper function for checkForPeekAndSplice for use with acl
-    static void cbCheckForPeekAndSplice(allow_t answer, void *data);
+    /// A wrapper function for checkForPeekAndSpliceDone for use with acl
+    static void cbCheckForPeekAndSpliceDone(allow_t answer, void *data);
 
     HttpRequestPointer request; ///< peer connection trigger or cause
     Comm::ConnectionPointer serverConn; ///< TCP connection to the peer

diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc
index f4674e9f417267879d6be9d903071302e285509e..258c7557bec454b3ca9537f7cbb540df3befba5a 100644 (file)
--- a/src/ssl/ServerBump.cc
+++ b/src/ssl/ServerBump.cc
@@ -22,10 +22,12 @@ CBDATA_NAMESPACED_CLASS_INIT(Ssl, ServerBump);
 Ssl::ServerBump::ServerBump(HttpRequest *fakeRequest, StoreEntry *e, Ssl::BumpMode md):
         request(fakeRequest),
         sslErrors(NULL),
-        mode(md),
         step(bumpStep1)
 {
     debugs(33, 4, HERE << "will peek at " << request->GetHost() << ':' << request->port);
+    act.step1 = md;
+    act.step2 = act.step3 = Ssl::bumpNone;
+
     const char *uri = urlCanonical(request.getRaw());
     if (e) {
         entry = e;

diff --git a/src/ssl/ServerBump.h b/src/ssl/ServerBump.h
index fc868e9a30cf91c95e23ac8b86458d7ad8a9ea8d..23fbceadb26143b9d29ddb4d0b75e6ea50234e3f 100644 (file)
--- a/src/ssl/ServerBump.h
+++ b/src/ssl/ServerBump.h
@@ -36,8 +36,12 @@ public:
     StoreEntry *entry; ///< for receiving Squid-generated error messages
     Ssl::X509_Pointer serverCert; ///< HTTPS server certificate
     Ssl::CertErrors *sslErrors; ///< SSL [certificate validation] errors
-    Ssl::BumpMode mode; ///< The SSL server bump mode
-    Ssl::BumpStep step; ///< The SSL server bumping step
+    struct {
+        Ssl::BumpMode step1; ///< The SSL bump mode at step1
+        Ssl::BumpMode step2; ///< The SSL bump mode at step2
+        Ssl::BumpMode step3; ///< The SSL bump mode at step3
+    } act; ///< bumping actions at various bumping steps
+    Ssl::BumpStep step; ///< The SSL bumping step
     SBuf clientSni; ///< the SSL client SNI name
 
 private: