Do not access-log SslBump-faked CONNECTs with _ABORTED suffixes.

All successful transactions, including fake CONNECTs, must be removed
from the pipeline (via a finished() call) to avoid _ABORTED suffix
added by Pipeline::terminateAll() which always calls noteIoError().

Also added a check that the remaining request in the pipeline is the
expected CONNECT. Old comments said as much but the code did not throw
if a different request was pipelined (and asserted if more than one
requests were). We now throw in all these problematic cases. I cannot
track all ConnStateData::getSslContextStart() callers deep enough to
be sure that only a single CONNECT can get through, but the change
reflects the expected behavior (and it may be useful to discover any
reality deviations from our expectations).

diff --git a/src/client_side.cc b/src/client_side.cc
index a84b0589ddd1889dcc8df42e93342a5471237b6b..de14fe96ab9d217e64eccd566baeb9702bd1945a 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2981,10 +2981,14 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer
 void
 ConnStateData::getSslContextStart()
 {
-    // XXX starting SSL with a pipeline of requests still waiting for non-SSL replies?
-    assert(pipeline.count() < 2); // the CONNECT is okay for now. Anything else is a bug.
-    pipeline.terminateAll(0);
-    /* careful: terminateAll(0) above frees request, host, etc. */
+    // If we are called, then CONNECT has succeeded. Finalize it.
+    if (auto xact = pipeline.front()) {
+        if (xact->http && xact->http->request && xact->http->request->method == Http::METHOD_CONNECT)
+            xact->finished();
+        // cannot proceed with encryption if requests wait for plain responses
+        Must(pipeline.empty());
+    }
+    /* careful: finished() above frees request, host, etc. */
 
     if (port->generateHostCertificates) {
         Ssl::CertificateProperties certProperties;