Bug 4811: supply AccessLogEntry (ALE) for more fast ACL checks. (#182)

Supplying ALE for fast ACL checks allows those checks to use ACLs that
assemble values from logformat %codes. Today, such ACLs are limited to
misplaced external ACLs (that should not be used with "fast"
directives!), but it is likely that fast ACLs like annotate_client will
eventually require ALE.

The "has" ACL documentation promises ALE for every transaction, but our
code does not deliver on that promise. This change fixes a dozen of
easy cases where ALE was available nearby. Also a non-trivial
cache_peer_access case was fixed, which proved to be more complex
because of the significant call depth of the peerAllowedToUse() check,
which is a known design problem of its own.

More cases need fixing, and the whole concept of ALE probably needs to
be revised because logformat %code expansion is needed in the
increasing number of contexts that have nothing to do with access
logging.

Also fixed triggering of (probably pointless) level-1 warnings:

* ALE missing adapted HttpRequest object
* ALE missing URL

With fix applied, any ACLChecklist with ALE synchronizes it at
'pre-check' stage without logging level-1 warnings. Warnings are
triggered only if for some reason this 'pre-check' synchronization was
bypassed.

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 70d1bc8ca3f29aa6718df3c1b050839f464cd5d5..ec1414b8c2411e803bff709ba3023bacbd275f27 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -323,7 +323,9 @@ FwdState::Start(const Comm::ConnectionPointer &clientConn, StoreEntry *entry, Ht
          * we do NOT want the indirect client address to be tested here.
          */
         ACLFilledChecklist ch(Config.accessList.miss, request, NULL);
+        ch.al = al;
         ch.src_addr = request->client_addr;
+        ch.syncAle(request, nullptr);
         if (ch.fastCheck().denied()) {
             err_type page_id;
             page_id = aclGetDenyInfoPage(&Config.denyInfoList, AclMatchedName, 1);
@@ -1180,6 +1182,8 @@ FwdState::pconnPop(const Comm::ConnectionPointer &dest, const char *domain)
     bool retriable = checkRetriable();
     if (!retriable && Config.accessList.serverPconnForNonretriable) {
         ACLFilledChecklist ch(Config.accessList.serverPconnForNonretriable, request, NULL);
+        ch.al = al;
+        ch.syncAle(request, nullptr);
         retriable = ch.fastCheck().allowed();
     }
     // always call shared pool first because we need to close an idle

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index e94271daf91ea30192e74555513b16cc24b4fbf0..7f8b1d109104ca0ea7ebe36bd8a9282c4e23869d 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -704,6 +704,7 @@ HttpRequest::manager(const CbcPointer<ConnStateData> &aMgr, const AccessLogEntry
             if (Config.accessList.spoof_client_ip) {
                 ACLFilledChecklist *checklist = new ACLFilledChecklist(Config.accessList.spoof_client_ip, this, clientConnection->rfc931);
                 checklist->al = al;
+                checklist->syncAle(this, nullptr);
                 flags.spoofClientIp = checklist->fastCheck().allowed();
                 delete checklist;
             } else

diff --git a/src/Notes.cc b/src/Notes.cc
index a368ca4c18b1f6c16628303dcd1a10441c5f67df..7a9be83b8495e68c7dd03804c753b77d1c8d1a90 100644 (file)
--- a/src/Notes.cc
+++ b/src/Notes.cc
@@ -42,7 +42,9 @@ Note::match(HttpRequest *request, HttpReply *reply, const AccessLogEntry::Pointe
 
     typedef Values::iterator VLI;
     ACLFilledChecklist ch(NULL, request, NULL);
+    ch.al = al;
     ch.reply = reply;
+    ch.syncAle(request, nullptr);
     if (reply)
         HTTPMSGLOCK(ch.reply);
 

diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc
index a5de02a2e2d300f0f300a1b0af746638eba5ffea..ea891fd327654afea7ddbe9692487e8fe4074229 100644 (file)
--- a/src/acl/Acl.cc
+++ b/src/acl/Acl.cc
@@ -141,7 +141,7 @@ ACL::matches(ACLChecklist *checklist) const
     } else {
         // make sure the ALE has as much data as possible
         if (requiresAle())
-            checklist->syncAle();
+            checklist->verifyAle();
 
         // have to cast because old match() API is missing const
         result = const_cast<ACL*>(this)->match(checklist);

diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h
index 0a7646678b7d42001a545aa3c757ee9ee3a2312e..c82b7ce77b102f5a7184f5649676388648807021 100644 (file)
--- a/src/acl/Checklist.h
+++ b/src/acl/Checklist.h
@@ -13,6 +13,8 @@
 #include <stack>
 #include <vector>
 
+class HttpRequest;
+
 /// ACL checklist callback
 typedef void ACLCB(allow_t, void *);
 
@@ -164,7 +166,10 @@ public:
     virtual bool hasRequest() const = 0;
     virtual bool hasReply() const = 0;
     virtual bool hasAle() const = 0;
-    virtual void syncAle() const = 0;
+    /// assigns uninitialized adapted_request and url ALE components
+    virtual void syncAle(HttpRequest *adaptedRequest, const char *logUri) const = 0;
+    /// warns if there are uninitialized ALE components and fills them
+    virtual void verifyAle() const = 0;
 
     /// change the current ACL list
     /// \return a pointer to the old list value (may be nullptr)

diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc
index e4db02c522b9a2d9a322f0a690cd8ceee4936ffc..b5ce7d21ac38958ce72e5c2ad1de349d37c0e5c3 100644 (file)
--- a/src/acl/FilledChecklist.cc
+++ b/src/acl/FilledChecklist.cc
@@ -79,7 +79,7 @@ showDebugWarning(const char *msg)
 }
 
 void
-ACLFilledChecklist::syncAle() const
+ACLFilledChecklist::verifyAle() const
 {
     // make sure the ALE fields used by Format::assemble to
     // fill the old external_acl_type codes are set if any
@@ -93,6 +93,8 @@ ACLFilledChecklist::syncAle() const
     if (request) {
         if (!al->request) {
             showDebugWarning("HttpRequest object");
+            // XXX: al->request should be original,
+            // but the request may be already adapted
             al->request = request;
             HTTPMSGLOCK(al->request);
         }
@@ -105,6 +107,8 @@ ACLFilledChecklist::syncAle() const
 
         if (al->url.isEmpty()) {
             showDebugWarning("URL");
+            // XXX: al->url should be the request URL from client,
+            // but request->url may be different (e.g.,redirected)
             al->url = request->url.absolute();
         }
     }
@@ -123,6 +127,19 @@ ACLFilledChecklist::syncAle() const
 #endif
 }
 
+void
+ACLFilledChecklist::syncAle(HttpRequest *adaptedRequest, const char *logUri) const
+{
+    if (!al)
+        return;
+    if (!al->adapted_request) {
+        al->adapted_request = adaptedRequest;
+        HTTPMSGLOCK(al->adapted_request);
+    }
+    if (al->url.isEmpty())
+        al->url = logUri;
+}
+
 ConnStateData *
 ACLFilledChecklist::conn() const
 {

diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h
index 9455a6ceea3771501107c6af455629906e17e598..ccc41aa0eb807f0546124fcaf6be21a57e27ca90 100644 (file)
--- a/src/acl/FilledChecklist.h
+++ b/src/acl/FilledChecklist.h
@@ -61,7 +61,8 @@ public:
     virtual bool hasRequest() const { return request != NULL; }
     virtual bool hasReply() const { return reply != NULL; }
     virtual bool hasAle() const { return al != NULL; }
-    virtual void syncAle() const;
+    virtual void syncAle(HttpRequest *adaptedRequest, const char *logUri) const;
+    virtual void verifyAle() const;
 
 public:
     Ip::Address src_addr;

diff --git a/src/adaptation/AccessCheck.cc b/src/adaptation/AccessCheck.cc
index 4baa5709f6c319d28f1b59bdc5d517161365ba82..f99b2fd93e2de51e6bad088eb298fb57144a2f15 100644 (file)
--- a/src/adaptation/AccessCheck.cc
+++ b/src/adaptation/AccessCheck.cc
@@ -135,6 +135,7 @@ Adaptation::AccessCheck::checkCandidates()
             if ((acl_checklist->reply = filter.reply))
                 HTTPMSGLOCK(acl_checklist->reply);
             acl_checklist->al = filter.al;
+            acl_checklist->syncAle(filter.request, nullptr);
             acl_checklist->nonBlockingCheck(AccessCheckCallbackWrapper, this);
             return;
         }

diff --git a/src/client_side.cc b/src/client_side.cc
index f9b0853d0ac012afb889f96606d8ee833ab0be80..e43a9da06796bb55835425a827756c5ba8e1f507 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -446,11 +446,14 @@ ClientHttpRequest::logRequest()
         al->adapted_request = request;
         HTTPMSGLOCK(al->adapted_request);
     }
+    // no need checklist.syncAle(): already synced
+    checklist.al = al;
     accessLogLog(al, &checklist);
 
     bool updatePerformanceCounters = true;
     if (Config.accessList.stats_collection) {
         ACLFilledChecklist statsCheck(Config.accessList.stats_collection, request, NULL);
+        statsCheck.al = al;
         if (al->reply) {
             statsCheck.reply = al->reply;
             HTTPMSGLOCK(statsCheck.reply);
@@ -1520,7 +1523,9 @@ bool ConnStateData::serveDelayedError(Http::Stream *context)
             bool allowDomainMismatch = false;
             if (Config.ssl_client.cert_error) {
                 ACLFilledChecklist check(Config.ssl_client.cert_error, request, dash_str);
+                check.al = http->al;
                 check.sslErrors = new Security::CertErrors(Security::CertError(SQUID_X509_V_ERR_DOMAIN_MISMATCH, srvCert));
+                check.syncAle(request, http->log_uri);
                 allowDomainMismatch = check.fastCheck().allowed();
                 delete check.sslErrors;
                 check.sslErrors = NULL;
@@ -1568,10 +1573,14 @@ clientTunnelOnError(ConnStateData *conn, Http::StreamPointer &context, HttpReque
 {
     if (conn->mayTunnelUnsupportedProto()) {
         ACLFilledChecklist checklist(Config.accessList.on_unsupported_protocol, request.getRaw(), nullptr);
+        checklist.al = (context && context->http) ? context->http->al : nullptr;
         checklist.requestErrorType = requestError;
         checklist.src_addr = conn->clientConnection->remote;
         checklist.my_addr = conn->clientConnection->local;
         checklist.conn(conn);
+        ClientHttpRequest *http = context ? context->http : nullptr;
+        const char *log_uri = http ? http->log_uri : nullptr;
+        checklist.syncAle(request.getRaw(), log_uri);
         allow_t answer = checklist.fastCheck();
         if (answer.allowed() && answer.kind == 1) {
             debugs(33, 3, "Request will be tunneled to server");
@@ -2821,6 +2830,10 @@ ConnStateData::postHttpsAccept()
         HTTPMSGUNLOCK(acl_checklist->al->request);
         acl_checklist->al->request = request;
         HTTPMSGLOCK(acl_checklist->al->request);
+        Http::StreamPointer context = pipeline.front();
+        ClientHttpRequest *http = context ? context->http : nullptr;
+        const char *log_uri = http ? http->log_uri : nullptr;
+        acl_checklist->syncAle(request, log_uri);
         acl_checklist->nonBlockingCheck(httpsSslBumpAccessCheckDone, this);
 #else
         fatal("FATAL: SSL-Bump requires --with-openssl");
@@ -3286,6 +3299,8 @@ ConnStateData::startPeekAndSplice()
         acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpNone));
         acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpClientFirst));
         acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpServerFirst));
+        const char *log_uri = http ? http->log_uri : nullptr;
+        acl_checklist->syncAle(sslServerBump->request.getRaw(), log_uri);
         acl_checklist->nonBlockingCheck(httpsSslBumpStep2AccessCheckDone, this);
         return;
     }
@@ -3725,6 +3740,7 @@ clientAclChecklistCreate(const acl_access * acl, ClientHttpRequest * http)
     ACLFilledChecklist *ch = new ACLFilledChecklist(acl, http->request,
             cbdataReferenceValid(conn) && conn != NULL && conn->clientConnection != NULL ? conn->clientConnection->rfc931 : dash_str);
     ch->al = http->al;
+    ch->syncAle(http->request, http->log_uri);
     /*
      * hack for ident ACL. It needs to get full addresses, and a place to store
      * the ident result on persistent connections...

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 4b5ac98fd5ee5a51307435361425abe4127eed9e..0174d072529d01124cbb5e9f4ab407bd5649805f 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1784,8 +1784,10 @@ ClientHttpRequest::doCallouts()
         calloutContext->tosToClientDone = true;
         if (getConn() != NULL && Comm::IsConnOpen(getConn()->clientConnection)) {
             ACLFilledChecklist ch(NULL, request, NULL);
+            ch.al = calloutContext->http->al;
             ch.src_addr = request->client_addr;
             ch.my_addr = request->my_addr;
+            ch.syncAle(request, log_uri);
             tos_t tos = aclMapTOS(Ip::Qos::TheConfig.tosToClient, &ch);
             if (tos)
                 Ip::Qos::setSockTos(getConn()->clientConnection, tos);
@@ -1796,8 +1798,10 @@ ClientHttpRequest::doCallouts()
         calloutContext->nfmarkToClientDone = true;
         if (getConn() != NULL && Comm::IsConnOpen(getConn()->clientConnection)) {
             ACLFilledChecklist ch(NULL, request, NULL);
+            ch.al = calloutContext->http->al;
             ch.src_addr = request->client_addr;
             ch.my_addr = request->my_addr;
+            ch.syncAle(request, log_uri);
             nfmark_t mark = aclMapNfmark(Ip::Qos::TheConfig.nfmarkToClient, &ch);
             if (mark)
                 Ip::Qos::setSockNfmark(getConn()->clientConnection, mark);

diff --git a/src/comm/TcpAcceptor.cc b/src/comm/TcpAcceptor.cc
index 603ebd379b07f50cc34e1920cf909852f897cca6..b240a3ad28ce3d6e6a0859fe2aadf1b0317e5819 100644 (file)
--- a/src/comm/TcpAcceptor.cc
+++ b/src/comm/TcpAcceptor.cc
@@ -267,6 +267,7 @@ logAcceptError(const Comm::ConnectionPointer &conn)
     ACLFilledChecklist ch(nullptr, nullptr, nullptr);
     ch.src_addr = conn->remote;
     ch.my_addr = conn->local;
+    ch.al = al;
     accessLogLog(al, &ch);
 }
 

diff --git a/src/http.cc b/src/http.cc
index d4407585953d7ba392fe2112fcfdc14ad2f3787a..452ff28cd78343ad70445aa5e3f631c81fbd8f4f 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -801,7 +801,9 @@ HttpStateData::handle1xx(HttpReply *reply)
     // check whether the 1xx response forwarding is allowed by squid.conf
     if (Config.accessList.reply) {
         ACLFilledChecklist ch(Config.accessList.reply, originalRequest(), NULL);
+        ch.al = fwd->al;
         ch.reply = reply;
+        ch.syncAle(originalRequest(), nullptr);
         HTTPMSGLOCK(ch.reply);
         if (!ch.fastCheck().allowed()) { // TODO: support slow lookups?
             debugs(11, 3, HERE << "ignoring denied 1xx");
@@ -2334,6 +2336,8 @@ HttpStateData::finishingBrokenPost()
     }
 
     ACLFilledChecklist ch(Config.accessList.brokenPosts, originalRequest(), NULL);
+    ch.al = fwd->al;
+    ch.syncAle(originalRequest(), nullptr);
     if (!ch.fastCheck().allowed()) {
         debugs(11, 5, HERE << "didn't match brokenPosts");
         return false;

diff --git a/src/neighbors.cc b/src/neighbors.cc
index c7df0b80323aa3c85626979c31d757631190109e..aebd42824b018f9fde03a99758d387d0ec3c32f9 100644 (file)
--- a/src/neighbors.cc
+++ b/src/neighbors.cc
@@ -136,7 +136,6 @@ neighborType(const CachePeer * p, const URL &url)
 bool
 peerAllowedToUse(const CachePeer * p, HttpRequest * request)
 {
-
     assert(request != NULL);
 
     if (neighborType(p, request->url) == PEER_SIBLING) {
@@ -167,7 +166,8 @@ peerAllowedToUse(const CachePeer * p, HttpRequest * request)
         return true;
 
     ACLFilledChecklist checklist(p->access, request, NULL);
-
+//    checklist.al = ps->al;
+    checklist.syncAle(request, nullptr);
     return checklist.fastCheck().allowed();
 }
 

diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index 9f14bd375cc80e255f95c52235750f7a4c9aa8fe..08161a24229441a11c1466e2660862d492bfcccc 100644 (file)
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -131,6 +131,7 @@ Security::PeerConnector::initialize(Security::SessionPointer &serverSession)
         if (acl_access *acl = ::Config.ssl_client.cert_error) {
             ACLFilledChecklist *check = new ACLFilledChecklist(acl, request.getRaw(), dash_str);
             check->al = al;
+            check->syncAle(request.getRaw(), nullptr);
             // check->fd(fd); XXX: need client FD here
             SSL_set_ex_data(serverSession.get(), ssl_ex_index_cert_error_check, check);
         }
@@ -324,6 +325,7 @@ Security::PeerConnector::sslCrtvdCheckForErrors(Ssl::CertValidationResponse cons
     if (acl_access *acl = ::Config.ssl_client.cert_error) {
         check = new ACLFilledChecklist(acl, request.getRaw(), dash_str);
         check->al = al;
+        check->syncAle(request.getRaw(), nullptr);
     }
 
     Security::CertErrors *errs = nullptr;

diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc
index eb95af8d1a299fb903ae1269ba9c41e15b8ab733..a79a2fff9205e7c7f44685969813c108f00adfef 100644 (file)
--- a/src/ssl/PeekingPeerConnector.cc
+++ b/src/ssl/PeekingPeerConnector.cc
@@ -70,6 +70,7 @@ Ssl::PeekingPeerConnector::checkForPeekAndSplice()
         acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpSplice));
     if (!srvBio->canBump())
         acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpBump));
+    acl_checklist->syncAle(request.getRaw(), nullptr);
     acl_checklist->nonBlockingCheck(Ssl::PeekingPeerConnector::cbCheckForPeekAndSpliceDone, this);
 }
 

diff --git a/src/tunnel.cc b/src/tunnel.cc
index d46297150ac59a77fc7998f347eb6242d90f84ef..3197547adc851b74026c7da27c3f592df264ce4f 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -1089,8 +1089,10 @@ tunnelStart(ClientHttpRequest * http)
          * default is to allow.
          */
         ACLFilledChecklist ch(Config.accessList.miss, request, NULL);
+        ch.al = http->al;
         ch.src_addr = request->client_addr;
         ch.my_addr = request->my_addr;
+        ch.syncAle(request, http->log_uri);
         if (ch.fastCheck().denied()) {
             debugs(26, 4, HERE << "MISS access forbidden.");
             err = new ErrorState(ERR_FORWARDING_DENIED, Http::scForbidden, request);