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 a2080047d6281d657c6e7a9d31f6d5e298efcf9e..11e948c28e28f376d69e8c856e814354d7fea0ea 100644 (file)
--- 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 0cae9962318fe6d935d8a10c6223aa9f442f27ad..c9b63ffe8725ef30c766724743146e94d71ab6e2 100644 (file)
--- 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 39d027734f35ead6c66d10ada1c67e7c141785a2..9e7c339dba20141b289d602677521b4ae837a4a5 100644 (file)
--- 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 54cfd374c0a1f1ee1c33558e2d9d3d178b27a8bb..7c8ce0fda1db97d6e669debe96c3f18c6cce5798 100644 (file)
--- 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