]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Add GeneratingCONNECT step for the existing at_step ACL (#484)
authorChristos Tsantilas <christos@chtsanti.net>
Wed, 2 Oct 2019 09:20:02 +0000 (09:20 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Thu, 3 Oct 2019 07:35:17 +0000 (07:35 +0000)
The new step allows admins to customize CONNECT request generation using
request_header_access and request_header_replace. As detailed below,
matching the request method does not work for those purposes.

request_header_access controls what HTTP request headers Squid _sends_.
However, this directive, like most other ACL-driven Squid directives,
uses the received request, _not_ the being-formed to-be-sent request.
This can be confusing, but it is the correct design/behavior.

When Squid is about to send a CONNECT request to a cache peer, what
_received_ request should request_header_access ACLs look at? There are
several ways to answer that question:

1. The received CONNECT request (or, for intercepted TLS connections,
   its faked equivalent). This is how the current CONNECT-sending code
   works when establishing the tunnel for a not-yet-bumped client
   connection. Problems:

    1. The CONNECT request received by Squid was not meant for the cache
       peer. It was meant specifically for this Squid. While it may have
       info useful for request_header_access checks, it is conceptually
       wrong to think of whats happening as "forwarding of that received
       CONNECT to the cache peer". Unlike GET requests, the CONNECT
       request that this Squid will send to the cache peer is dedicated
       to the Squid-peer connection. It is generated, not forwarded.

    2. That CONNECT request may have been received a long time ago, and
       Squid may have forwarded many bumped GET requests since then. It
       feels strange to consult such an old/"disconnected" message.
2. The received (and bumped) GET request. This is how the current
   CONNECT-sending code works when establishing the tunnel for an
   already bumped client connection. Problem:

    1. Squid is about to send a generated CONNECT request, not to
       forward a received GET request (the latter will happen later,
       after the CONNECT transaction). The two requests may differ a
       lot. Using a GET request when generating a CONNECT request is
       confusing: "Why is my CONNECT method ACL does not match when
       Squid sends a CONNECT request?!"

3. No request. Problems:

    1. Some old configurations that use request-specific ACLs with
       request_header_access will generate runtime "missing request"
       warnings and may fail to work correctly.

    2. Extra admin work is required to store request-specific
       information as connection annotations that request_header_access
       ACLs can still access.

    3. Conceptually, there is a request that Squid is establishing this
       CONNECT tunnel for. Squid will access-log that request. Hiding
       that information from ACLs feels odd/wrong. And some of that info
       would still be accessible to ACLs (via ALE/etc.). This hiding
       does not really hide all of the details.

Our solution preserves what received request Squid is looking at. Items
1 and 2 above still apply (depending on the configuration and on the
request being processed), with their unique problems intact. Those
problems are not as bad as the problems associated with the item 3!

The at_step ACL was added for SslBump but, IIRC, we knew that it may
eventually cover other request processing steps. Generating a CONNECT
request is one of those other steps.

This is a Measurement Factory project.

17 files changed:
src/AclRegs.cc
src/Makefile.am
src/MasterXaction.h
src/XactionStep.h [new file with mode: 0644]
src/acl/AtStep.cc
src/acl/AtStep.h
src/acl/AtStepData.cc
src/acl/AtStepData.h
src/acl/Makefile.am
src/cf.data.pre
src/client_side.cc
src/clients/HttpTunneler.cc
src/ssl/PeekingPeerConnector.cc
src/ssl/ServerBump.cc
src/ssl/ServerBump.h
src/ssl/support.h
src/tunnel.cc

index 7996099e40f763d15c1e741997e9f7868874fe16..8c73c9dad7539213f46454abf5fba5c5fed69558 100644 (file)
@@ -171,7 +171,7 @@ Acl::Init()
     RegisterMaker("user_cert", [](TypeName name)->ACL* { return new ACLStrategised<X509*>(new ACLCertificateData(Ssl::GetX509UserAttribute, "*"), new ACLCertificateStrategy, name); });
     RegisterMaker("ca_cert", [](TypeName name)->ACL* { return new ACLStrategised<X509*>(new ACLCertificateData(Ssl::GetX509CAAttribute, "*"), new ACLCertificateStrategy, name); });
     RegisterMaker("server_cert_fingerprint", [](TypeName name)->ACL* { return new ACLStrategised<X509*>(new ACLCertificateData(Ssl::GetX509Fingerprint, "-sha1", true), new ACLServerCertificateStrategy, name); });
-    RegisterMaker("at_step", [](TypeName name)->ACL* { return new ACLStrategised<Ssl::BumpStep>(new ACLAtStepData, new ACLAtStepStrategy, name); });
+    RegisterMaker("at_step", [](TypeName name)->ACL* { return new ACLStrategised<XactionStep>(new ACLAtStepData, new ACLAtStepStrategy, name); });
     RegisterMaker("ssl::server_name", [](TypeName name)->ACL* { return new ACLStrategised<char const *>(new ACLServerNameData, new ACLServerNameStrategy, name); });
     RegisterMaker("ssl::server_name_regex", [](TypeName name)->ACL* { return new ACLStrategised<char const *>(new ACLRegexData, new ACLServerNameStrategy, name); });
 #endif
index 06be93c52959dbfb9a7c94e0068755196e68d37f..2a767bdcc23ef3e184412ea1cdce77cac4b91426 100644 (file)
@@ -490,6 +490,7 @@ squid_SOURCES = \
        wordlist.cc \
        XactionInitiator.h \
        XactionInitiator.cc \
+       XactionStep.h \
        $(WIN32_SOURCE) \
        $(WINSVC_SOURCE)
 
index 9991f7e8e6f53a9e1d6137d4b172196d1aee60ea..56d5ebeb8d6fdd2977a940ffae0fbf9184dc0df5 100644 (file)
@@ -55,6 +55,9 @@ public:
     /// the initiator of this transaction
     XactionInitiator initiator;
 
+    /// whether we are currently creating a CONNECT header (to be sent to peer)
+    bool generatingConnect = false;
+
     // TODO: add state from other Jobs in the transaction
 };
 
diff --git a/src/XactionStep.h b/src/XactionStep.h
new file mode 100644 (file)
index 0000000..62c0623
--- /dev/null
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) 1996-2019 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#ifndef SQUID_XACTIONSTEPS_H
+#define SQUID_XACTIONSTEPS_H
+
+enum class XactionStep  {
+    enumBegin_ = 0, // for WholeEnum iteration
+    unknown = enumBegin_,
+    generatingConnect,
+#if USE_OPENSSL
+    tlsBump1,
+    tlsBump2,
+    tlsBump3,
+#endif
+    enumEnd_ // for WholeEnum iteration
+};
+
+#endif /* SQUID_XACTIONSTEPS_H */
+
index fc94af6958e8c97cafc7bbf06b06a8540bec4f05..28c8e8265c81f0d2c4686d1abfa6e81a47201ba9 100644 (file)
@@ -8,25 +8,48 @@
 
 #include "squid.h"
 
-#if USE_OPENSSL
-
 #include "acl/AtStep.h"
 #include "acl/AtStepData.h"
 #include "acl/FilledChecklist.h"
 #include "client_side.h"
 #include "http/Stream.h"
+#if USE_OPENSSL
 #include "ssl/ServerBump.h"
+#endif
 
 int
-ACLAtStepStrategy::match (ACLData<Ssl::BumpStep> * &data, ACLFilledChecklist *checklist)
+ACLAtStepStrategy::match(ACLData<XactionStep> * &data, ACLFilledChecklist *checklist)
 {
-    Ssl::ServerBump *bump = NULL;
-    if (checklist->conn() != NULL && (bump = checklist->conn()->serverBump()))
-        return data->match(bump->step);
-    else
-        return data->match(Ssl::bumpStep1);
+#if USE_OPENSSL
+    // We use step1 for all these very different cases:
+    // - The transaction is not subject to ssl_bump rules (if any).
+    // - No ssl_bump action has matched yet.
+    // - The ssl_bump client-first action has already matched.
+    // - Another ssl_bump action has already matched, but
+    //   ConnStateData::serverBump() has not been built yet.
+    auto currentSslBumpStep = XactionStep::tlsBump1;
+
+    if (const auto mgr = checklist->conn()) {
+        if (const auto serverBump = mgr->serverBump())
+            currentSslBumpStep = serverBump->step;
+    }
+
+    if (data->match(currentSslBumpStep))
+        return 1;
+#endif // USE_OPENSSL
+
+    if (data->match(XactionStep::generatingConnect)) {
+        if (!checklist->request)
+            return 0; // we have warned about the missing request earlier
+
+        if (!checklist->request->masterXaction) {
+            debugs(28, DBG_IMPORTANT, "BUG: at_step GeneratingCONNECT ACL is missing master transaction info. Assuming mismatch.");
+            return 0;
+        }
+
+        return checklist->request->masterXaction->generatingConnect ? 1 : 0;
+    }
+
     return 0;
 }
 
-#endif /* USE_OPENSSL */
-
index 1a41e5810ce35110f815bef2c6a24bbe1c77c987..ab5e3cc4488c1742f6b14db3f9a2d6dfe55e65ce 100644 (file)
@@ -9,20 +9,16 @@
 #ifndef SQUID_ACLATSTEP_H
 #define SQUID_ACLATSTEP_H
 
-#if USE_OPENSSL
-
 #include "acl/Strategy.h"
-#include "ssl/support.h"
+#include "XactionStep.h"
 
 /// \ingroup ACLAPI
-class ACLAtStepStrategy : public ACLStrategy<Ssl::BumpStep>
+class ACLAtStepStrategy: public ACLStrategy<XactionStep>
 {
 
 public:
     virtual int match (ACLData<MatchType> * &, ACLFilledChecklist *) override;
 };
 
-#endif /* USE_OPENSSL */
-
 #endif /* SQUID_ACLATSTEP_H */
 
index d7aad87870a83a4035270559460a6b8d89099dba..df857a6a0131cffd4b9511aa827a7df8b2cfa324 100644 (file)
@@ -7,16 +7,46 @@
  */
 
 #include "squid.h"
-
-#if USE_OPENSSL
-
 #include "acl/AtStepData.h"
 #include "acl/Checklist.h"
+#include "base/EnumIterator.h"
 #include "cache_cf.h"
 #include "ConfigParser.h"
 #include "Debug.h"
+#include "sbuf/Stream.h"
 #include "wordlist.h"
 
+static inline const char *
+StepName(const XactionStep xstep)
+{
+    // keep in sync with XactionStep
+    static const char *StepNames[static_cast<int>(XactionStep::enumEnd_)] = {
+        "[unknown step]"
+        ,"GeneratingCONNECT"
+#if USE_OPENSSL
+        ,"SslBump1"
+        ,"SslBump2"
+        ,"SslBump3"
+#endif
+    };
+
+    assert(XactionStep::enumBegin_ <= xstep && xstep < XactionStep::enumEnd_);
+    return StepNames[static_cast<int>(xstep)];
+}
+
+static XactionStep
+StepValue(const char *name)
+{
+    assert(name);
+
+    for (const auto step: WholeEnum<XactionStep>()) {
+        if (strcasecmp(StepName(step), name) == 0)
+            return static_cast<XactionStep>(step);
+    }
+
+    throw TextException(ToSBuf("unknown at_step step name: ", name), Here());
+}
+
 ACLAtStepData::ACLAtStepData()
 {}
 
@@ -30,41 +60,29 @@ ACLAtStepData::~ACLAtStepData()
 }
 
 bool
-ACLAtStepData::match(Ssl::BumpStep  toFind)
+ACLAtStepData::match(XactionStep toFind)
 {
-    for (std::list<Ssl::BumpStep>::const_iterator it = values.begin(); it != values.end(); ++it) {
-        if (*it == toFind)
-            return true;
-    }
-    return false;
+    const auto found = std::find(values.cbegin(), values.cend(), toFind);
+    return (found != values.cend());
 }
 
 SBufList
 ACLAtStepData::dump() const
 {
     SBufList sl;
-    for (std::list<Ssl::BumpStep>::const_iterator it = values.begin(); it != values.end(); ++it) {
-        sl.push_back(SBuf(*it == Ssl::bumpStep1 ? "SslBump1" :
-                          *it == Ssl::bumpStep2 ? "SslBump2" :
-                          *it == Ssl::bumpStep3 ? "SslBump3" : "???"));
-    }
+    for (const auto value : values)
+        sl.push_back(SBuf(StepName(value)));
     return sl;
 }
 
 void
 ACLAtStepData::parse()
 {
-    while (const char *t = ConfigParser::strtokFile()) {
-        if (strcasecmp(t, "SslBump1") == 0) {
-            values.push_back(Ssl::bumpStep1);
-        } else if (strcasecmp(t, "SslBump2") == 0) {
-            values.push_back(Ssl::bumpStep2);
-        } else if (strcasecmp(t, "SslBump3") == 0) {
-            values.push_back(Ssl::bumpStep3);
-        } else {
-            debugs(28, DBG_CRITICAL, "FATAL: invalid AtStep step: " << t);
-            self_destruct();
-        }
+    while (const auto name = ConfigParser::strtokFile()) {
+        const auto step = StepValue(name);
+        if (step == XactionStep::unknown)
+            throw TextException(ToSBuf("prohibited at_step step name: ", name), Here());
+        values.push_back(step);
     }
 }
 
@@ -80,5 +98,3 @@ ACLAtStepData::clone() const
     return new ACLAtStepData(*this);
 }
 
-#endif /* USE_OPENSSL */
-
index 3a4cb8bd93248babadbe79b8ecc36edf7fc01c9b..d5b989e02d0895916d6fca37b53178e0fabb0fc8 100644 (file)
@@ -9,15 +9,12 @@
 #ifndef SQUID_ACLATSTEPDATA_H
 #define SQUID_ACLATSTEPDATA_H
 
-#if USE_OPENSSL
-
 #include "acl/Acl.h"
 #include "acl/Data.h"
-#include "ssl/support.h"
-
+#include "XactionStep.h"
 #include <list>
 
-class ACLAtStepData : public ACLData<Ssl::BumpStep>
+class ACLAtStepData : public ACLData<XactionStep>
 {
     MEMPROXY_CLASS(ACLAtStepData);
 
@@ -26,16 +23,14 @@ public:
     ACLAtStepData(ACLAtStepData const &);
     ACLAtStepData &operator= (ACLAtStepData const &);
     virtual ~ACLAtStepData();
-    bool match(Ssl::BumpStep);
+    bool match(XactionStep);
     virtual SBufList dump() const;
     void parse();
     bool empty() const;
     virtual ACLAtStepData *clone() const;
 
-    std::list<Ssl::BumpStep> values;
+    std::list<XactionStep> values;
 };
 
-#endif /* USE_OPENSSL */
-
 #endif /* SQUID_ACLSSL_ERRORDATA_H */
 
index 7e51e954965e74080158d015b47396e7db48ee90..d2ed0c9a08bcad56e4af3564b624ff845149b3c7 100644 (file)
@@ -65,6 +65,10 @@ libacls_la_SOURCES = \
        AnyOf.h \
        Asn.cc \
        Asn.h \
+       AtStep.cc \
+       AtStep.h \
+       AtStepData.cc \
+       AtStepData.h \
        ConnectionsEncrypted.cc \
        ConnectionsEncrypted.h \
        ConnMark.cc \
@@ -159,10 +163,6 @@ libacls_la_SOURCES = \
 EXTRA_libacls_la_SOURCES =
 
 SSL_ACLS = \
-       AtStep.cc \
-       AtStep.h \
-       AtStepData.cc \
-       AtStepData.h \
         CertificateData.cc \
         CertificateData.h  \
         Certificate.cc \
index 1e797ba8a38a910d2e67b1bafb8a13032b48e4e2..1b3191962b9a281de6696a5aaf3e419d0af128b2 100644 (file)
@@ -1428,6 +1428,17 @@ endif
          #  acl hasWhatMyLoggingDaemonNeeds has request
          #  acl hasWhatMyLoggingDaemonNeeds has response
 
+acl aclname at_step step
+         # match against the current request processing step [fast]
+         # Valid steps are:
+         #   GeneratingCONNECT: Generating HTTP CONNECT request headers
+IF USE_OPENSSL
+         # The following ssl_bump processing steps are recognized:
+         #   SslBump1: After getting TCP-level and HTTP CONNECT info.
+         #   SslBump2: After getting SSL Client Hello info.
+         #   SslBump3: After getting SSL Server Hello info.
+ENDIF
+
 IF USE_OPENSSL
        acl aclname ssl_error errorname
          # match against SSL certificate validation error [fast]
@@ -1459,17 +1470,6 @@ IF USE_OPENSSL
          # The SHA1 digest algorithm is the default and is currently
          # the only algorithm supported (-sha1).
 
-       acl aclname at_step step
-         # match against the current step during ssl_bump evaluation [fast]
-         # Never matches and should not be used outside the ssl_bump context.
-         #
-         # At each SslBump step, Squid evaluates ssl_bump directives to find
-         # the next bumping action (e.g., peek or splice). Valid SslBump step
-         # values and the corresponding ssl_bump evaluation moments are:
-         #   SslBump1: After getting TCP-level and HTTP CONNECT info.
-         #   SslBump2: After getting SSL Client Hello info.
-         #   SslBump3: After getting SSL Server Hello info.
-
        acl aclname ssl::server_name [option] .foo.com ...
          # matches server name obtained from various sources [fast]
          #
index b60e8061b91bb77c4f3e3cc3eb7d738124d8fc92..15b25bc365b1ba7c536290f9dad1a8c74ea2680b 100644 (file)
@@ -3118,8 +3118,8 @@ ConnStateData::startPeekAndSplice()
     Http::StreamPointer context = pipeline.front();
     ClientHttpRequest *http = context ? context->http : nullptr;
 
-    if (sslServerBump->step == Ssl::bumpStep1) {
-        sslServerBump->step = Ssl::bumpStep2;
+    if (sslServerBump->at(XactionStep::tlsBump1)) {
+        sslServerBump->step = XactionStep::tlsBump2;
         // Run a accessList check to check if want to splice or continue bumping
 
         ACLFilledChecklist *acl_checklist = new ACLFilledChecklist(Config.accessList.ssl_bump, sslServerBump->request.getRaw(), nullptr);
index f8e7f8836df4b420e547ba209b5a70f17a5dbdcd..31b12d3e584ef1f19ba5319b9fa9c05eed1f8b68 100644 (file)
@@ -126,22 +126,34 @@ Http::Tunneler::writeRequest()
 {
     debugs(83, 5, connection);
 
-    HttpHeader hdr_out(hoRequest);
     Http::StateFlags flags;
     flags.peering = true;
     // flags.tunneling = false; // the CONNECT request itself is not tunneled
     // flags.toOrigin = false; // the next HTTP hop is a non-originserver peer
+
     MemBuf mb;
-    mb.init();
-    mb.appendf("CONNECT %s HTTP/1.1\r\n", url.c_str());
-    HttpStateData::httpBuildRequestHeader(request.getRaw(),
-                                          nullptr, // StoreEntry
-                                          al,
-                                          &hdr_out,
-                                          flags);
-    hdr_out.packInto(&mb);
-    hdr_out.clean();
-    mb.append("\r\n", 2);
+
+    try {
+        request->masterXaction->generatingConnect = true;
+
+        mb.init();
+        mb.appendf("CONNECT %s HTTP/1.1\r\n", url.c_str());
+        HttpHeader hdr_out(hoRequest);
+        HttpStateData::httpBuildRequestHeader(request.getRaw(),
+                                              nullptr, // StoreEntry
+                                              al,
+                                              &hdr_out,
+                                              flags);
+        hdr_out.packInto(&mb);
+        hdr_out.clean();
+        mb.append("\r\n", 2);
+
+        request->masterXaction->generatingConnect = false;
+    } catch (...) {
+        // TODO: Add scope_guard; do not wait until it is in the C++ standard.
+        request->masterXaction->generatingConnect = false;
+        throw;
+    }
 
     debugs(11, 2, "Tunnel Server REQUEST: " << connection <<
            ":\n----------\n" << mb.buf << "\n----------");
index b9d5cf6681c7d25cdbf9bc63a74f8ec557757b1f..e547901c80d604785ec0b88e136b4024ef9d50a7 100644 (file)
@@ -48,7 +48,7 @@ Ssl::PeekingPeerConnector::checkForPeekAndSplice()
     // Mark Step3 of bumping
     if (request->clientConnectionManager.valid()) {
         if (Ssl::ServerBump *serverBump = request->clientConnectionManager->serverBump()) {
-            serverBump->step = Ssl::bumpStep3;
+            serverBump->step = XactionStep::tlsBump3;
         }
     }
 
@@ -166,7 +166,7 @@ Ssl::PeekingPeerConnector::initialize(Security::SessionPointer &serverSession)
         if (hostName)
             SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName);
 
-        Must(!csd->serverBump() || csd->serverBump()->step <= Ssl::bumpStep2);
+        Must(!csd->serverBump() || csd->serverBump()->at(XactionStep::tlsBump1, XactionStep::tlsBump2));
         if (csd->sslBumpMode == Ssl::bumpPeek || csd->sslBumpMode == Ssl::bumpStare) {
             auto clientSession = fd_table[clientConn->fd].ssl.get();
             Must(clientSession);
index 4f520d14866235d84792c093ce92ac48f2c8065e..e37d38d5654e33efecf3937cda381794a8bbd15a 100644 (file)
@@ -21,7 +21,7 @@
 CBDATA_NAMESPACED_CLASS_INIT(Ssl, ServerBump);
 
 Ssl::ServerBump::ServerBump(ClientHttpRequest *http, StoreEntry *e, Ssl::BumpMode md):
-    step(bumpStep1)
+    step(XactionStep::tlsBump1)
 {
     assert(http->request);
     request = http->request;
index 5bb81b288d6ac691343a4e2f2c011ea3826bce51..f62639fdf0c41e309eed8e40f74256f175dd8a4a 100644 (file)
@@ -16,6 +16,7 @@
 #include "ip/Address.h"
 #include "security/forward.h"
 #include "Store.h"
+#include "XactionStep.h"
 
 class ConnStateData;
 class store_client;
@@ -24,6 +25,8 @@ class ClientHttpRequest;
 namespace Ssl
 {
 
+using BumpStep = XactionStep;
+
 /**
  * Maintains bump-server-first related information.
  */
@@ -40,6 +43,12 @@ public:
     /// whether there was a successful connection to (and peeking at) the origin server
     bool connectedOk() const {return entry && entry->isEmpty();}
 
+    /// whether we are currently performing the given processing step
+    bool at(const BumpStep stp) const { return step == stp; }
+
+    /// whether we are currently performing one of the given processing steps
+    bool at(const BumpStep step1, const BumpStep step2) const { return at(step1) || at(step2); }
+
     /// faked, minimal request; required by Client API
     HttpRequest::Pointer request;
     StoreEntry *entry; ///< for receiving Squid-generated error messages
index 1c8d9011be243c5735ec35e6e21453379871c33c..2fb74fb4dbfa61f08e1940989e8bd9293ba2a678 100644 (file)
@@ -134,8 +134,6 @@ extern const EVP_MD *DefaultSignHash;
  */
 enum BumpMode {bumpNone = 0, bumpClientFirst, bumpServerFirst, bumpPeek, bumpStare, bumpBump, bumpSplice, bumpTerminate, /*bumpErr,*/ bumpEnd};
 
-enum BumpStep {bumpStep1, bumpStep2, bumpStep3};
-
 /**
  \ingroup  ServerProtocolSSLAPI
  * Short names for ssl-bump modes
index 5ef19aae59824cf322133deb80d1d9d1c821f0da..519ac43a7774eb7efb1e60332e2077e903567f06 100644 (file)
@@ -101,7 +101,7 @@ public:
             return false;
 #if USE_OPENSSL
         // We are bumping and we had already send "OK CONNECTED"
-        if (http.valid() && http->getConn() && http->getConn()->serverBump() && http->getConn()->serverBump()->step > Ssl::bumpStep1)
+        if (http.valid() && http->getConn() && http->getConn()->serverBump() && http->getConn()->serverBump()->at(XactionStep::tlsBump2, XactionStep::tlsBump3))
             return false;
 #endif
         return !(request != NULL &&