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.

src/FwdState.cc

@@ -332,7 +332,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);
@@ -1219,6 +1221,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

src/HttpRequest.cc

@@ -729,6 +729,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

src/MemObject.h

@@ -28,6 +28,7 @@ typedef void STMCB (void *data, StoreIOBuffer wroteBuffer);
 typedef void STABH(void *);
 
 class store_client;
+class PeerSelector;
 
 class MemObject
 {
@@ -161,7 +162,7 @@ class MemObject
 
     struct timeval start_ping;
     IRCB *ping_reply_callback;
-    void *ircb_data;
+    PeerSelector *ircb_data;
 
     struct {
         STABH *callback;

src/Notes.cc

@@ -69,7 +69,9 @@ bool
 Note::match(HttpRequest *request, HttpReply *reply, const AccessLogEntry::Pointer &al, SBuf &matched)
 {
     ACLFilledChecklist ch(nullptr, request, nullptr);
+    ch.al = al;
     ch.reply = reply;
+    ch.syncAle(request, nullptr);
     if (reply)
         HTTPMSGLOCK(ch.reply);
 

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);

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 @@ class ACLChecklist
     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)

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
 {

src/acl/FilledChecklist.h

@@ -66,7 +66,8 @@ class ACLFilledChecklist: public ACLChecklist
     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;

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;
         }

src/carp.cc

@@ -13,6 +13,7 @@
 #include "HttpRequest.h"
 #include "mgr/Registration.h"
 #include "neighbors.h"
+#include "PeerSelectState.h"
 #include "SquidConfig.h"
 #include "Store.h"
 #include "URL.h"
@@ -143,8 +144,11 @@ carpInit(void)
 }
 
 CachePeer *
-carpSelectParent(HttpRequest * request)
+carpSelectParent(PeerSelector *ps)
 {
+    assert(ps);
+    HttpRequest *request = ps->request;
+
     int k;
     CachePeer *p = NULL;
     CachePeer *tp;
@@ -204,7 +208,7 @@ carpSelectParent(HttpRequest * request)
         debugs(39, 3, "carpSelectParent: key=" << key << " name=" << tp->name << " combined_hash=" << combined_hash  <<
                " score=" << std::setprecision(0) << score);
 
-        if ((score > high_score) && peerHTTPOkay(tp, request)) {
+        if ((score > high_score) && peerHTTPOkay(tp, ps)) {
             p = tp;
             high_score = score;
         }

src/carp.h

@@ -12,10 +12,10 @@
 #define SQUID_CARP_H_
 
 class CachePeer;
-class HttpRequest;
+class PeerSelector;
 
 void carpInit(void);
-CachePeer *carpSelectParent(HttpRequest *);
+CachePeer *carpSelectParent(PeerSelector *);
 
 #endif /* SQUID_CARP_H_ */
 

src/client_side.cc

@@ -447,11 +447,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);
@@ -1521,7 +1524,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;
@@ -1569,10 +1574,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");
@@ -2822,6 +2831,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");
@@ -3287,6 +3300,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;
     }
@@ -3732,6 +3747,7 @@ clientAclChecklistFill(ACLFilledChecklist &checklist, ClientHttpRequest *http)
 {
     checklist.setRequest(http->request);
     checklist.al = http->al;
+    checklist.syncAle(http->request, http->log_uri);
 
     // TODO: If http->getConn is always http->request->clientConnectionManager,
     // then call setIdent() inside checklist.setRequest(). Otherwise, restore

src/client_side_request.cc

@@ -1779,8 +1779,10 @@ ClientHttpRequest::doCallouts()
     // Set appropriate MARKs and CONNMARKs if needed.
     if (getConn() && Comm::IsConnOpen(getConn()->clientConnection)) {
         ACLFilledChecklist ch(nullptr, request, nullptr);
+        ch.al = calloutContext->http->al;
         ch.src_addr = request->client_addr;
         ch.my_addr = request->my_addr;
+        ch.syncAle(request, log_uri);
 
         if (!calloutContext->toClientMarkingDone) {
             calloutContext->toClientMarkingDone = true;

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);
 }
 

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().getRaw());
+        ch.al = fwd->al;
         ch.reply = reply;
+        ch.syncAle(originalRequest().getRaw(), 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().getRaw());
+    ch.al = fwd->al;
+    ch.syncAle(originalRequest().getRaw(), nullptr);
     if (!ch.fastCheck().allowed()) {
         debugs(11, 5, HERE << "didn't match brokenPosts");
         return false;

src/icmp/net_db.cc

@@ -32,6 +32,7 @@
 #include "mgr/Registration.h"
 #include "mime_header.h"
 #include "neighbors.h"
+#include "PeerSelectState.h"
 #include "SquidConfig.h"
 #include "SquidTime.h"
 #include "Store.h"
@@ -1304,9 +1305,12 @@ netdbExchangeStart(void *data)
 }
 
 CachePeer *
-netdbClosestParent(HttpRequest * request)
+netdbClosestParent(PeerSelector *ps)
 {
 #if USE_ICMP
+    assert(ps);
+    HttpRequest *request = ps->request;
+
     CachePeer *p = NULL;
     netdbEntry *n;
     const ipcache_addrs *ia;
@@ -1350,7 +1354,7 @@ netdbClosestParent(HttpRequest * request)
         if (neighborType(p, request->url) != PEER_PARENT)
             continue;
 
-        if (!peerHTTPOkay(p, request))  /* not allowed */
+        if (!peerHTTPOkay(p, ps))  /* not allowed */
             continue;
 
         return p;

src/icmp/net_db.h

@@ -16,6 +16,7 @@
 class CachePeer;
 class HttpRequest;
 class netdbEntry;
+class PeerSelector;
 class StoreEntry;
 class URL;
 
@@ -80,7 +81,7 @@ void netdbBinaryExchange(StoreEntry *);
 void netdbExchangeStart(void *);
 
 void netdbExchangeUpdatePeer(Ip::Address &, CachePeer *, double, double);
-CachePeer *netdbClosestParent(HttpRequest *);
+CachePeer *netdbClosestParent(PeerSelector *);
 void netdbHostData(const char *host, int *samp, int *rtt, int *hops);
 
 #endif /* ICMP_NET_DB_H */