From 090f1d3c3c4e1aa9ab96b84787907967313ccfcf Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Wed, 2 Oct 2019 09:20:02 +0000 Subject: [PATCH] Add GeneratingCONNECT step for the existing at_step ACL (#484) 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. --- src/AclRegs.cc | 2 +- src/Makefile.am | 1 + src/MasterXaction.h | 3 ++ src/XactionStep.h | 25 ++++++++++++ src/acl/AtStep.cc | 43 +++++++++++++++----- src/acl/AtStep.h | 8 +--- src/acl/AtStepData.cc | 70 ++++++++++++++++++++------------- src/acl/AtStepData.h | 13 ++---- src/acl/Makefile.am | 8 ++-- src/cf.data.pre | 22 +++++------ src/client_side.cc | 4 +- src/clients/HttpTunneler.cc | 34 ++++++++++------ src/ssl/PeekingPeerConnector.cc | 4 +- src/ssl/ServerBump.cc | 2 +- src/ssl/ServerBump.h | 9 +++++ src/ssl/support.h | 2 - src/tunnel.cc | 2 +- 17 files changed, 165 insertions(+), 87 deletions(-) create mode 100644 src/XactionStep.h diff --git a/src/AclRegs.cc b/src/AclRegs.cc index 7996099e40..8c73c9dad7 100644 --- a/src/AclRegs.cc +++ b/src/AclRegs.cc @@ -171,7 +171,7 @@ Acl::Init() RegisterMaker("user_cert", [](TypeName name)->ACL* { return new ACLStrategised(new ACLCertificateData(Ssl::GetX509UserAttribute, "*"), new ACLCertificateStrategy, name); }); RegisterMaker("ca_cert", [](TypeName name)->ACL* { return new ACLStrategised(new ACLCertificateData(Ssl::GetX509CAAttribute, "*"), new ACLCertificateStrategy, name); }); RegisterMaker("server_cert_fingerprint", [](TypeName name)->ACL* { return new ACLStrategised(new ACLCertificateData(Ssl::GetX509Fingerprint, "-sha1", true), new ACLServerCertificateStrategy, name); }); - RegisterMaker("at_step", [](TypeName name)->ACL* { return new ACLStrategised(new ACLAtStepData, new ACLAtStepStrategy, name); }); + RegisterMaker("at_step", [](TypeName name)->ACL* { return new ACLStrategised(new ACLAtStepData, new ACLAtStepStrategy, name); }); RegisterMaker("ssl::server_name", [](TypeName name)->ACL* { return new ACLStrategised(new ACLServerNameData, new ACLServerNameStrategy, name); }); RegisterMaker("ssl::server_name_regex", [](TypeName name)->ACL* { return new ACLStrategised(new ACLRegexData, new ACLServerNameStrategy, name); }); #endif diff --git a/src/Makefile.am b/src/Makefile.am index 06be93c529..2a767bdcc2 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -490,6 +490,7 @@ squid_SOURCES = \ wordlist.cc \ XactionInitiator.h \ XactionInitiator.cc \ + XactionStep.h \ $(WIN32_SOURCE) \ $(WINSVC_SOURCE) diff --git a/src/MasterXaction.h b/src/MasterXaction.h index 9991f7e8e6..56d5ebeb8d 100644 --- a/src/MasterXaction.h +++ b/src/MasterXaction.h @@ -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 index 0000000000..62c0623ff5 --- /dev/null +++ b/src/XactionStep.h @@ -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 */ + diff --git a/src/acl/AtStep.cc b/src/acl/AtStep.cc index fc94af6958..28c8e8265c 100644 --- a/src/acl/AtStep.cc +++ b/src/acl/AtStep.cc @@ -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 * &data, ACLFilledChecklist *checklist) +ACLAtStepStrategy::match(ACLData * &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 */ - diff --git a/src/acl/AtStep.h b/src/acl/AtStep.h index 1a41e5810c..ab5e3cc448 100644 --- a/src/acl/AtStep.h +++ b/src/acl/AtStep.h @@ -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 +class ACLAtStepStrategy: public ACLStrategy { public: virtual int match (ACLData * &, ACLFilledChecklist *) override; }; -#endif /* USE_OPENSSL */ - #endif /* SQUID_ACLATSTEP_H */ diff --git a/src/acl/AtStepData.cc b/src/acl/AtStepData.cc index d7aad87870..df857a6a01 100644 --- a/src/acl/AtStepData.cc +++ b/src/acl/AtStepData.cc @@ -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(XactionStep::enumEnd_)] = { + "[unknown step]" + ,"GeneratingCONNECT" +#if USE_OPENSSL + ,"SslBump1" + ,"SslBump2" + ,"SslBump3" +#endif + }; + + assert(XactionStep::enumBegin_ <= xstep && xstep < XactionStep::enumEnd_); + return StepNames[static_cast(xstep)]; +} + +static XactionStep +StepValue(const char *name) +{ + assert(name); + + for (const auto step: WholeEnum()) { + if (strcasecmp(StepName(step), name) == 0) + return static_cast(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::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::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 */ - diff --git a/src/acl/AtStepData.h b/src/acl/AtStepData.h index 3a4cb8bd93..d5b989e02d 100644 --- a/src/acl/AtStepData.h +++ b/src/acl/AtStepData.h @@ -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 -class ACLAtStepData : public ACLData +class ACLAtStepData : public ACLData { 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 values; + std::list values; }; -#endif /* USE_OPENSSL */ - #endif /* SQUID_ACLSSL_ERRORDATA_H */ diff --git a/src/acl/Makefile.am b/src/acl/Makefile.am index 7e51e95496..d2ed0c9a08 100644 --- a/src/acl/Makefile.am +++ b/src/acl/Makefile.am @@ -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 \ diff --git a/src/cf.data.pre b/src/cf.data.pre index 1e797ba8a3..1b3191962b 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -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] # diff --git a/src/client_side.cc b/src/client_side.cc index b60e8061b9..15b25bc365 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -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); diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index f8e7f8836d..31b12d3e58 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -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----------"); diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index b9d5cf6681..e547901c80 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -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); diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc index 4f520d1486..e37d38d565 100644 --- a/src/ssl/ServerBump.cc +++ b/src/ssl/ServerBump.cc @@ -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; diff --git a/src/ssl/ServerBump.h b/src/ssl/ServerBump.h index 5bb81b288d..f62639fdf0 100644 --- a/src/ssl/ServerBump.h +++ b/src/ssl/ServerBump.h @@ -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 diff --git a/src/ssl/support.h b/src/ssl/support.h index 1c8d9011be..2fb74fb4db 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -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 diff --git a/src/tunnel.cc b/src/tunnel.cc index 5ef19aae59..519ac43a77 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -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 && -- 2.47.2