From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Date: Thu, 30 May 2024 15:41:15 +0000 (+0000)
Subject: annotate_client and annotate_transaction ACLs must always match (#1820)
X-Git-Tag: SQUID_7_0_1~114
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cc71dd7cd757c77c6c9df72a4c60b2d6209def96;p=thirdparty%2Fsquid.git

annotate_client and annotate_transaction ACLs must always match (#1820)

    WARNING: markAsTunneled ACL is used in context without an HTTP
    request. Assuming mismatch.

Our annotate_client and annotate_transaction ACLs are documented as
"always matching", and some existing Squid configurations rely on that
invariant. Both ACLs did not match when they lacked access to the
current transaction information because their requiresRequest() method
returned true; annotate_client ACL also did not match after the
client-to-Squid connection was gone and when the transaction was not
associated with a client-to-Squid connection.

This change makes ACL code conform to documentation. Squid still warns
the admin if an ACL cannot annotate. Such warnings may indicate s Squid
bug or misconfiguration, but mismatching in those cases causes more harm
because it makes it impossible for the admin to rely on the primary
matching invariant. "One reliable invariant plus one unreliable side
effect" is the lesser evil than "two unreliable effects".

    # the following must deny even if it cannot annotate
    http_access deny markAsDenied

    # the following might not log denied traffic (with a prior warning)
    access_log syslog:daemon.err markedAsDenied

Also fixed annotate_client to annotate the current transaction even
after ConnStateData destruction. Such annotations may happen when, for
example, Squid continues a large download after the HTTP client is gone.
---

diff --git a/src/acl/AnnotateClient.cc b/src/acl/AnnotateClient.cc
index a2080047d6..11e948c28e 100644
--- a/src/acl/AnnotateClient.cc
+++ b/src/acl/AnnotateClient.cc
@@ -17,15 +17,29 @@ int
 Acl::AnnotateClientCheck::match(ACLChecklist * const ch)
 {
     const auto checklist = Filled(ch);
+    auto annotated = false;
+    const auto tdata = dynamic_cast<ACLAnnotationData*>(data.get());
+    assert(tdata);
+    const auto conn = checklist->conn();
 
-    if (const auto conn = checklist->conn()) {
-        const auto tdata = dynamic_cast<ACLAnnotationData*>(data.get());
-        assert(tdata);
+    if (conn) {
         tdata->annotate(conn->notes(), &delimiters.value, checklist->al);
-        if (const auto request = checklist->request)
-            tdata->annotate(request->notes(), &delimiters.value, checklist->al);
-        return 1;
+        annotated = true;
     }
-    return 0;
+
+    if (const auto &request = checklist->request) {
+        tdata->annotate(request->notes(), &delimiters.value, checklist->al);
+        annotated = true;
+    } else if (conn && !conn->pipeline.empty()) {
+        debugs(28, DBG_IMPORTANT, "ERROR: Squid BUG: " << name << " ACL is used in context with " <<
+               "an unexpectedly nil ACLFilledChecklist::request. Did not annotate the current transaction.");
+    }
+
+    if (!annotated) {
+        debugs(28, DBG_IMPORTANT, "WARNING: " << name << " ACL is used in context without " <<
+               "active client-to-Squid connection and current transaction information. Did not annotate.");
+    }
+
+    return 1; // this is an "always matching" ACL
 }
 
diff --git a/src/acl/AnnotateClient.h b/src/acl/AnnotateClient.h
index 0cae996231..c9b63ffe87 100644
--- a/src/acl/AnnotateClient.h
+++ b/src/acl/AnnotateClient.h
@@ -20,7 +20,6 @@ class AnnotateClientCheck: public Acl::AnnotationCheck
 public:
     /* Acl::Node API */
     int match(ACLChecklist *) override;
-    bool requiresRequest() const override { return true; }
 };
 
 } // namespace Acl
diff --git a/src/acl/AnnotateTransaction.cc b/src/acl/AnnotateTransaction.cc
index 39d027734f..9e7c339dba 100644
--- a/src/acl/AnnotateTransaction.cc
+++ b/src/acl/AnnotateTransaction.cc
@@ -21,8 +21,10 @@ Acl::AnnotateTransactionCheck::match(ACLChecklist * const ch)
         const auto tdata = dynamic_cast<ACLAnnotationData*>(data.get());
         assert(tdata);
         tdata->annotate(request->notes(), &delimiters.value, checklist->al);
-        return 1;
+    } else {
+        debugs(28, DBG_IMPORTANT, "WARNING: " << name << " ACL is used in context without " <<
+               "current transaction information. Did not annotate.");
     }
-    return 0;
+    return 1; // this is an "always matching" ACL
 }
 
diff --git a/src/acl/AnnotateTransaction.h b/src/acl/AnnotateTransaction.h
index 54cfd374c0..7c8ce0fda1 100644
--- a/src/acl/AnnotateTransaction.h
+++ b/src/acl/AnnotateTransaction.h
@@ -20,7 +20,6 @@ class AnnotateTransactionCheck: public Acl::AnnotationCheck
 public:
     /* Acl::Node API */
     int match(ACLChecklist *) override;
-    bool requiresRequest() const override { return true; }
 };
 
 } // namespace Acl