Handle Amos comments on bump-ssl-server-first patch

- Use "IF" instead of "IFDEF" in cf.data.pre. The IFDEF already used in the
  IDEF<colon> form.

- Add an assertion in setServerBump to assure that even if this method reused in
  a client request we will not try to change the existing bump mode

- Polish the code in  doCallouts method: wrap if(!calloutContext->error) { ... }
  around the whole section of callouts.

- The detailCode parameter in ErrorState::detailError(int detailCode) shadows
  the ErrorState::detailCode parameter.

- src/url.cc: Reverting change in urlParseFinish

- Allow old "ssl_bump allow/deny acl ..." syntax converting:
    * "ssl_bump allow" to "ssl_bump client-first"
    * "ssl-bump deny" to "ssl_bump none"
  Prints warning messages to urge users update their configuration.
  Does not allow to mix old and new configurations.

- Warnings and comment fixes

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index 893bd6b52de90a3b5a58e48db7e384e451d42d19..0e1c9242d22ef9b2e2df39d662555b11b38fea0b 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -55,6 +55,7 @@
 #include "auth/Config.h"
 #include "auth/Scheme.h"
 #endif
+#include "base/RunnersRegistry.h"
 #include "ConfigParser.h"
 #include "CpuAffinityMap.h"
 #include "DiskIO/DiskIOModule.h"
@@ -3808,14 +3809,14 @@ parsePortCfg(AnyP::PortCfg ** head, const char *optionName)
 
 #if USE_SSL
     if (strcasecmp(protocol, "https") == 0) {
-        /* ssl-bump on https_port configuration requires either tproxy or intercepted, and vice versa */
+        /* ssl-bump on https_port configuration requires either tproxy or intercept, and vice versa */
         const bool hijacked = s->spoof_client_ip || s->intercepted;
         if (s->sslBump && !hijacked) {
-            debugs(3, DBG_CRITICAL, "FATAL: ssl-bump on https_port requires tproxy/intercepted which is missing.");
+            debugs(3, DBG_CRITICAL, "FATAL: ssl-bump on https_port requires tproxy/intercept which is missing.");
             self_destruct();
         }
         if (hijacked && !s->sslBump) {
-            debugs(3, DBG_CRITICAL, "FATAL: tproxy/intercepted on https_port requires ssl-bump which is missing.");
+            debugs(3, DBG_CRITICAL, "FATAL: tproxy/intercept on https_port requires ssl-bump which is missing.");
             self_destruct();
         }
     }
@@ -4537,36 +4538,101 @@ static void free_sslproxy_cert_sign(sslproxy_cert_sign **cert_sign)
     }
 }
 
+class sslBumpCfgRr: public ::RegisteredRunner
+{
+public:
+    static Ssl::BumpMode lastDeprecatedRule;
+    /* RegisteredRunner API */
+    virtual void run(const RunnerRegistry &);
+};
+
+Ssl::BumpMode sslBumpCfgRr::lastDeprecatedRule = Ssl::bumpEnd;
+
+RunnerRegistrationEntry(rrFinalizeConfig, sslBumpCfgRr);
+
+void sslBumpCfgRr::run(const RunnerRegistry &r)
+{
+    if (lastDeprecatedRule != Ssl::bumpEnd) {
+        assert( lastDeprecatedRule == Ssl::bumpClientFirst || lastDeprecatedRule == Ssl::bumpNone);
+        static char buf[1024];
+        if (lastDeprecatedRule == Ssl::bumpClientFirst) {
+            strcpy(buf, "ssl_bump deny all");
+            debugs(3, DBG_CRITICAL, "WARNING: auto-converting deprecated implicit "
+                   "\"ssl_bump deny all\" to \"ssl_bump none all\". New ssl_bump configurations "
+                   "must not use implicit rules. Update your ssl_bump rules.");
+        } else {
+            strcpy(buf, "ssl_bump allow all");
+            debugs(3, DBG_CRITICAL, "SECURITY NOTICE: auto-converting deprecated implicit "
+                   "\"ssl_bump allow all\" to \"ssl_bump client-first all\" which is usually "
+                   "inferior to the newer server-first bumping mode. New ssl_bump"
+                   " configurations must not use implicit rules. Update your ssl_bump rules.");
+        }
+        parse_line(buf);
+    }
+}
+
 static void parse_sslproxy_ssl_bump(acl_access **ssl_bump)
 {
+    typedef const char *BumpCfgStyle;
+    BumpCfgStyle bcsNone = NULL;
+    BumpCfgStyle bcsNew = "new client/server-first/none";
+    BumpCfgStyle bcsOld = "deprecated allow/deny";
+    static BumpCfgStyle bumpCfgStyleLast = bcsNone;
+    BumpCfgStyle bumpCfgStyleNow = bcsNone;
     char *bm;
     if ((bm = strtok(NULL, w_space)) == NULL) {
         self_destruct();
         return;
     }
 
+    // if this is the first rule proccessed
+    if (*ssl_bump == NULL) {
+        bumpCfgStyleLast = bcsNone;
+        sslBumpCfgRr::lastDeprecatedRule = Ssl::bumpEnd;
+    }
+
     acl_access *A = new acl_access;
     A->allow = allow_t(ACCESS_ALLOWED);
 
-    if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpClientFirst]) == 0)
+    if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpClientFirst]) == 0) {
         A->allow.kind = Ssl::bumpClientFirst;
-    else if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpServerFirst]) == 0)
+        bumpCfgStyleNow = bcsNew;
+    } else if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpServerFirst]) == 0) {
         A->allow.kind = Ssl::bumpServerFirst;
-    else if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpNone]) == 0)
+        bumpCfgStyleNow = bcsNew;
+    } else if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpNone]) == 0) {
         A->allow.kind = Ssl::bumpNone;
-    else if (strcmp(bm, "allow") == 0 || strcmp(bm, "deny") == 0) {
-        // allow/deny rule sets may rely on an implicit "negate the last one"
-        // rule which we cannot support due to multuple "allow" keywords
-        debugs(3, DBG_CRITICAL, "FATAL: ssl_bump allow/deny rule(s) " <<
-               "must be CAREFULLY converted to specify bump mode(s).");
-        self_destruct();
-        return;
+        bumpCfgStyleNow = bcsNew;
+    } else if (strcmp(bm, "allow") == 0) {
+        debugs(3, DBG_CRITICAL, "SECURITY NOTICE: auto-converting deprecated "
+               "\"ssl_bump allow <acl>\" to \"ssl_bump client-first <acl>\" which "
+               "is usually inferior to the newer server-first "
+               "bumping mode. Update your ssl_bump rules.");
+        A->allow.kind = Ssl::bumpClientFirst;
+        bumpCfgStyleNow = bcsOld;
+        sslBumpCfgRr::lastDeprecatedRule = Ssl::bumpClientFirst;
+    } else if (strcmp(bm, "deny") == 0) {
+        debugs(3, DBG_CRITICAL, "WARNING: auto-converting deprecated "
+               "\"ssl_bump deny <acl>\" to \"ssl_bump none <acl>\". Update "
+               "your ssl_bump rules.");
+        A->allow.kind = Ssl::bumpNone;
+        bumpCfgStyleNow = bcsOld;
+        sslBumpCfgRr::lastDeprecatedRule = Ssl::bumpNone;
     } else {
         debugs(3, DBG_CRITICAL, "FATAL: unknown ssl_bump mode: " << bm);
         self_destruct();
         return;
     }
 
+    if (bumpCfgStyleLast != bcsNone && bumpCfgStyleNow != bumpCfgStyleLast) {
+        debugs(3, DBG_CRITICAL, "FATAL: do not mix " << bumpCfgStyleNow << " actions with " << 
+               bumpCfgStyleLast << " actions. Update your ssl_bump rules.");
+        self_destruct();
+        return;
+    }
+
+    bumpCfgStyleLast = bumpCfgStyleNow;
+
     aclParseAclList(LegacyParser, &A->aclList);
 
     acl_access *B, **T;

diff --git a/src/cf.data.pre b/src/cf.data.pre
index 66573a8a6f789f1aa6c9bbe0fbb1bf935934cb9e..458c8500107affb40960e8d0902a1a76ddb6c672 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -647,7 +647,7 @@ DOC_END
 NAME: acl
 TYPE: acl
 LOC: Config.aclList
-IFDEF USE_SSL
+IF USE_SSL
 DEFAULT: ssl::certHasExpired ssl_error X509_V_ERR_CERT_HAS_EXPIRED
 DEFAULT: ssl::certNotYetValid ssl_error X509_V_ERR_CERT_NOT_YET_VALID
 DEFAULT: ssl::certDomainMismatch ssl_error SQUID_X509_V_ERR_DOMAIN_MISMATCH
@@ -878,7 +878,7 @@ DOC_START
          # effect in rules that affect the reply data stream such as
          # http_reply_access.
 
-IFDEF USE_SSL
+IF USE_SSL
        acl aclname ssl_error errorname
          # match against SSL certificate validation error [fast]
          #
@@ -1562,6 +1562,10 @@ DOC_START
 
           accel        Accelerator / reverse proxy mode
 
+          intercept    Support for IP-Layer interception of
+                       outgoing requests without browser settings.
+                       NP: disables authentication and IPv6 on the port.
+
           tproxy       Support Linux TPROXY for spoofing outgoing
                        connections using the client IP address.
                        NP: disables authentication and maybe IPv6 on the port.
@@ -1575,7 +1579,7 @@ DOC_START
                        An "ssl_bump server-first" match is required to
                        fully enable bumping of intercepted SSL connections.
 
-                       Requires tproxy.
+                       Requires tproxy or intercept.
 
        Omitting the mode flag causes default forward proxy mode to be used.
 

diff --git a/src/cf_gen.cc b/src/cf_gen.cc
index 29adf0e67f2da470b9eb7d5567e25bcbd9b8ff67..3a067ec12f39322264d906e66536cb235dcba991 100644 (file)
--- a/src/cf_gen.cc
+++ b/src/cf_gen.cc
@@ -259,16 +259,16 @@ main(int argc, char *argv[])
         if ((t = strchr(buff, '\n')))
             *t = '\0';
 
-        if(strncmp(buff, "IFDEF ", 6) == 0) {
-            if ((ptr = strtok(buff + 6, WS)) == NULL) {
-                std::cerr << "Missing IFDEF parameter on line" << linenum << std::endl;
+        if(strncmp(buff, "IF ", 3) == 0) {
+            if ((ptr = strtok(buff + 3, WS)) == NULL) {
+                std::cerr << "Missing IF parameter on line" << linenum << std::endl;
                 exit(1);
             }
             IFDEFS.push(ptr);
             continue;
         } else if (strcmp(buff, "ENDIF") == 0) {
             if (IFDEFS.size() == 0) {
-                std::cerr << "ENDIF without IFDEF before on line " << linenum << std::endl;
+                std::cerr << "ENDIF without IF before on line " << linenum << std::endl;
                 exit(1);
             }
             IFDEFS.pop();

diff --git a/src/client_side.cc b/src/client_side.cc
index 594e204b26e8ad29798a77e9e9a9fc66eb7e24a5..9914034c34de0f0459d6496b7fa5ec247c05bbd9 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3648,7 +3648,7 @@ httpsAccept(const CommAcceptCbParams &params)
         }
   
         // Create a fake HTTP request for ssl_bump ACL check,
-        // using tproxy-provided destination IP and port.
+        // using tproxy/intercept provided destination IP and port.
         HttpRequest *request = new HttpRequest();
         static char ip[MAX_IPSTRLEN];
         assert(params.conn->flags & (COMM_TRANSPARENT | COMM_INTERCEPTION));

diff --git a/src/client_side.h b/src/client_side.h
index ca5e1b25fdf019ba1407d9ef40d88a39f43f90e4..6c43258e5fba8164c3b6f3272b05f99ab134eb7c 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -343,7 +343,12 @@ public:
     void switchToHttps(HttpRequest *request, Ssl::BumpMode bumpServerMode);
     bool switchedToHttps() const { return switchedToHttps_; }
     Ssl::ServerBump *serverBump() {return sslServerBump;}
-    void setServerBump(Ssl::ServerBump *srvBump) {if (!sslServerBump) sslServerBump = srvBump;}
+    inline void setServerBump(Ssl::ServerBump *srvBump) {
+        if (!sslServerBump)
+            sslServerBump = srvBump;
+        else
+            assert(sslServerBump == srvBump);
+    }
     /// Fill the certAdaptParams with the required data for certificate adaptation
     /// and create the key for storing/retrieve the certificate to/from the cache
     void buildSslCertGenerationParams(Ssl::CertificateProperties &certProperties);

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index c27598f93d80f62a715e76e99b60df4bb188d562..4522ac483c72c0515e18e189426fe93e185220bb 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1523,65 +1523,67 @@ ClientHttpRequest::doCallouts()
     if (!calloutContext->http->al.request)
         calloutContext->http->al.request = HTTPMSGLOCK(request);
 
+    if (!calloutContext->error) {
     // CVE-2009-0801: verify the Host: header is consistent with other known details.
-    if (!calloutContext->error && !calloutContext->host_header_verify_done) {
-        debugs(83, 3, HERE << "Doing calloutContext->hostHeaderVerify()");
-        calloutContext->host_header_verify_done = true;
-        calloutContext->hostHeaderVerify();
-        return;
-    }
+        if (!calloutContext->host_header_verify_done) {
+            debugs(83, 3, HERE << "Doing calloutContext->hostHeaderVerify()");
+            calloutContext->host_header_verify_done = true;
+            calloutContext->hostHeaderVerify();
+            return;
+        }
 
-    if (!calloutContext->error && !calloutContext->http_access_done) {
-        debugs(83, 3, HERE << "Doing calloutContext->clientAccessCheck()");
-        calloutContext->http_access_done = true;
-        calloutContext->clientAccessCheck();
-        return;
-    }
+        if (!calloutContext->http_access_done) {
+            debugs(83, 3, HERE << "Doing calloutContext->clientAccessCheck()");
+            calloutContext->http_access_done = true;
+            calloutContext->clientAccessCheck();
+            return;
+        }
 
 #if USE_ADAPTATION
-    if (!calloutContext->error && !calloutContext->adaptation_acl_check_done) {
-        calloutContext->adaptation_acl_check_done = true;
-        if (Adaptation::AccessCheck::Start(
+        if (!calloutContext->adaptation_acl_check_done) {
+            calloutContext->adaptation_acl_check_done = true;
+            if (Adaptation::AccessCheck::Start(
                     Adaptation::methodReqmod, Adaptation::pointPreCache,
                     request, NULL, this))
-            return; // will call callback
-    }
+                return; // will call callback
+        }
 #endif
 
-    if (!calloutContext->error && !calloutContext->redirect_done) {
-        calloutContext->redirect_done = true;
-        assert(calloutContext->redirect_state == REDIRECT_NONE);
+        if (!calloutContext->redirect_done) {
+            calloutContext->redirect_done = true;
+            assert(calloutContext->redirect_state == REDIRECT_NONE);
 
-        if (Config.Program.redirect) {
-            debugs(83, 3, HERE << "Doing calloutContext->clientRedirectStart()");
-            calloutContext->redirect_state = REDIRECT_PENDING;
-            calloutContext->clientRedirectStart();
-            return;
+            if (Config.Program.redirect) {
+                debugs(83, 3, HERE << "Doing calloutContext->clientRedirectStart()");
+                calloutContext->redirect_state = REDIRECT_PENDING;
+                calloutContext->clientRedirectStart();
+                return;
+            }
         }
-    }
 
-    if (!calloutContext->error && !calloutContext->adapted_http_access_done) {
-        debugs(83, 3, HERE << "Doing calloutContext->clientAccessCheck2()");
-        calloutContext->adapted_http_access_done = true;
-        calloutContext->clientAccessCheck2();
-        return;
-    }
+        if (!calloutContext->adapted_http_access_done) {
+            debugs(83, 3, HERE << "Doing calloutContext->clientAccessCheck2()");
+            calloutContext->adapted_http_access_done = true;
+            calloutContext->clientAccessCheck2();
+            return;
+        }
 
-    if (!calloutContext->error && !calloutContext->interpreted_req_hdrs) {
-        debugs(83, 3, HERE << "Doing clientInterpretRequestHeaders()");
-        calloutContext->interpreted_req_hdrs = 1;
-        clientInterpretRequestHeaders(this);
-    }
+        if (!calloutContext->interpreted_req_hdrs) {
+            debugs(83, 3, HERE << "Doing clientInterpretRequestHeaders()");
+            calloutContext->interpreted_req_hdrs = 1;
+            clientInterpretRequestHeaders(this);
+        }
 
-    if (!calloutContext->error && !calloutContext->no_cache_done) {
-        calloutContext->no_cache_done = true;
+        if (!calloutContext->no_cache_done) {
+            calloutContext->no_cache_done = true;
 
-        if (Config.accessList.noCache && request->flags.cachable) {
-            debugs(83, 3, HERE << "Doing calloutContext->checkNoCache()");
-            calloutContext->checkNoCache();
-            return;
+            if (Config.accessList.noCache && request->flags.cachable) {
+                debugs(83, 3, HERE << "Doing calloutContext->checkNoCache()");
+                calloutContext->checkNoCache();
+                return;
+            }
         }
-    }
+    } //  if !calloutContext->error
 
     if (!calloutContext->tosToClientDone) {
         calloutContext->tosToClientDone = true;

diff --git a/src/errorpage.cc b/src/errorpage.cc
index 0f90ad3509d7a1b51703fb7df0c6241861f1a75d..d073580b5e1bb4004d2a74fb64e2394e9c70c2a9 100644 (file)
--- a/src/errorpage.cc
+++ b/src/errorpage.cc
@@ -597,12 +597,6 @@ ErrorState::ErrorState(err_type t, http_status status, HttpRequest * req) :
     }
 }
 
-void
-ErrorState::detailError(int detailCode)
-{
-    detailCode = detailCode;
-}
-
 void
 errorAppendEntry(StoreEntry * entry, ErrorState * err)
 {

diff --git a/src/errorpage.h b/src/errorpage.h
index 525310686011a9b0b48a0d980a4058d1af62aa9c..22e6228eee8aa4bbe222ac0c12852e1952724923 100644 (file)
--- a/src/errorpage.h
+++ b/src/errorpage.h
@@ -106,7 +106,7 @@ public:
     HttpReply *BuildHttpReply(void);
 
     /// set error type-specific detail code
-    void detailError(int detailCode);
+    void detailError(int dCode) {detailCode = dCode;}
 
 private:
     /**

diff --git a/src/url.cc b/src/url.cc
index b5608481663468db71d3bfa75a385da41a46c21f..44876fe144da240149ed86a1b21d40f213475473 100644 (file)
--- a/src/url.cc
+++ b/src/url.cc
@@ -462,6 +462,7 @@ urlParseFinish(const HttpRequestMethod& method,
         request = new HttpRequest(method, protocol, urlpath);
     else {
         request->initHTTP(method, protocol, urlpath);
+        safe_free(request->canonical);
     }
 
     request->SetHost(host);