From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Mon, 12 Dec 2022 13:34:12 +0000 (+0000)
Subject: Ban acl key changes in req_header, rep_header, and note ACLs (#1210)
X-Git-Tag: SQUID_6_0_1~54
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4a3b853;p=thirdparty%2Fsquid.git

Ban acl key changes in req_header, rep_header, and note ACLs (#1210)

* req_header and rep_header ACLs; the key is the header-name argument:

    acl barred req_header X-1 bar
    acl barred req_header X-2 bar # never matches since commit a0b240c
    http_access deny barred

* the "note" ACL; the key is the annotation name argument:

    acl banned note color green  # never matches since commit 39baccc
    acl banned note weight heavy
    http_access deny banned

Squid did write a cache.log ERROR for req_header/rep_header key changes
but was silent about the preceding "note" ACL rules being ineffective
after a key change (e.g., Squid was not denying "green" requests above).

Now, Squid rejects configurations containing such ACL key changes.

Also significantly reduced the corresponding parsing code duplication.
---

diff --git a/src/ConfigParser.cc b/src/ConfigParser.cc
index d8a426ace3..4fa3214d4f 100644
--- a/src/ConfigParser.cc
+++ b/src/ConfigParser.cc
@@ -504,6 +504,35 @@ ConfigParser::regex(const char *expectedRegexDescription)
     return std::unique_ptr<RegexPattern>(new RegexPattern(pattern, flags));
 }
 
+void
+ConfigParser::SetAclKey(SBuf &keyStorage, const char *keyParameterName)
+{
+    extern const char *AclMatchedName;
+
+    const auto newKey = strtokFile();
+    if (!newKey) {
+        throw TextException(ToSBuf("An acl declaration is missing a ", keyParameterName,
+                                   Debug::Extra, "ACL name: ", AclMatchedName),
+                            Here());
+    }
+
+    if (keyStorage.isEmpty()) {
+        keyStorage = newKey;
+        return;
+    }
+
+    if (keyStorage.caseCmp(newKey) == 0)
+        return; // no change
+
+    throw TextException(ToSBuf("Attempt to change the value of the ", keyParameterName, " argument in a subsequent acl declaration:",
+                               Debug::Extra, "previously seen value: ", keyStorage,
+                               Debug::Extra, "new/conflicting value: ", newKey,
+                               Debug::Extra, "ACL name: ", AclMatchedName,
+                               Debug::Extra, "advice: Use a dedicated ACL name for each distinct ", keyParameterName,
+                               " (and group those ACLs together using an 'any-of' ACL)."),
+                        Here());
+}
+
 CachePeer &
 ConfigParser::cachePeer(const char *peerNameTokenDescription)
 {
diff --git a/src/ConfigParser.h b/src/ConfigParser.h
index 8f20563920..7039ffeb27 100644
--- a/src/ConfigParser.h
+++ b/src/ConfigParser.h
@@ -129,6 +129,13 @@ public:
      */
     static bool NextKvPair(char * &key, char * &value);
 
+    // TODO: Convert into a non-static method after exposing the current parser.
+    /// Extract, validate, and store the ACL key parameter for ACL types
+    /// declared using "acl aclname type key argument..." declaration that
+    /// require unique key values for each aclname+type combination.
+    /// Key comparison is case-insensitive.
+    static void SetAclKey(SBuf &keyStorage, const char *keyParameterName);
+
     /**
      * Preview the next token. The next NextToken() and strtokFile() call
      * will return the same token.
diff --git a/src/acl/HttpHeaderData.cc b/src/acl/HttpHeaderData.cc
index 7004393ad7..c3bafedb26 100644
--- a/src/acl/HttpHeaderData.cc
+++ b/src/acl/HttpHeaderData.cc
@@ -75,22 +75,8 @@ ACLHTTPHeaderData::lineOptions()
 void
 ACLHTTPHeaderData::parse()
 {
-    char* t = ConfigParser::strtokFile();
-    if (!t) {
-        debugs(28, DBG_CRITICAL, "ERROR: " << cfg_filename << " line " << config_lineno << ": " << config_input_line);
-        debugs(28, DBG_CRITICAL, "ERROR: Missing header name in ACL");
-        return;
-    }
-
-    if (hdrName.isEmpty()) {
-        hdrName = t;
-        hdrId = Http::HeaderLookupTable.lookup(hdrName).id;
-    } else if (hdrName.caseCmp(t) != 0) {
-        debugs(28, DBG_CRITICAL, "ERROR: " << cfg_filename << " line " << config_lineno << ": " << config_input_line);
-        debugs(28, DBG_CRITICAL, "ERROR: ACL cannot match both " << hdrName << " and " << t << " headers. Use 'any-of' ACL instead.");
-        return;
-    }
-
+    ConfigParser::SetAclKey(hdrName, "header-name");
+    hdrId = Http::HeaderLookupTable.lookup(hdrName).id;
     regex_rule->parse();
 }
 
diff --git a/src/acl/NoteData.cc b/src/acl/NoteData.cc
index 428b94599a..8dc5a3a072 100644
--- a/src/acl/NoteData.cc
+++ b/src/acl/NoteData.cc
@@ -47,9 +47,7 @@ ACLNoteData::dump() const
 void
 ACLNoteData::parse()
 {
-    char* t = ConfigParser::strtokFile();
-    assert (t != nullptr);
-    name = t;
+    ConfigParser::SetAclKey(name, "annotation name");
     values->parse();
 }
 
diff --git a/src/tests/stub_acl.cc b/src/tests/stub_acl.cc
index cf942a7923..f4a95908ef 100644
--- a/src/tests/stub_acl.cc
+++ b/src/tests/stub_acl.cc
@@ -9,10 +9,13 @@
 /* DEBUG: section 28    Access Control */
 
 #include "squid.h"
-#include "acl/Gadgets.h"
 
 #define STUB_API "acl/"
 #include "tests/STUB.h"
 
+#include "acl/Acl.h"
+const char *AclMatchedName = nullptr;
+
+#include "acl/Gadgets.h"
 size_t aclParseAclList(ConfigParser &, Acl::Tree **, const char *) STUB_RETVAL(0)