Warn about some bad from-helper annotations (#1221)

From-helper annotations may belong to one or more of the following
(partially overlapping) categories:

* A1: Annotations with a name ending with an underscore. Those
  annotations are reserved for admin use. Example: "category_".

* A2: Annotations that this Squid version knows about and treats
  specially. Example: "clt_conn_tag".

* B1: Annotations that this Squid version does not know about at all.
  Future Squids may start using them specially. Example: "detail".

* B2: Annotations unused by this Squid build. Example: Squid
  ./configured with --disable-auth does not use "user" and "password"
  annotations received from an external ACL helper.

* B3: Annotations incompatible with the current helper response
  type/kind. Example: Digest authentication code does not use "ha1"
  annotation in ERR responses received from digest authentication
  helpers.

* B4: Annotations with repeated names (the first of them will be used).
  Example: "user" in OK replies from negotiate authentication helpers.

Warn about received helper annotations in the B1 category.

It is possible to also warn about B2 and B3 annotations, but several
draft implementations imply that such an improvement requires complex
code changes that may also increase Squid code maintenance overheads.

Warning about B4 annotations is not that difficult, but we must decide:

* Whether custom repeated annotation are actually bad (and should be
  reported or even ignored).

* Whether Squid-recognized repeated annotations are bad enough to
  warrant the decreased performance and increased code complexity that
  reporting them would require. In other words, do we want to penalize
  good helpers that do _not_ send any repeated Squid-recognized
  annotations (except, perhaps, "message") to warn about bad helpers
  that do?

diff --git a/src/helper/Reply.cc b/src/helper/Reply.cc
index 5e8826752b8b4625c6bc4599c597a308269cc8c7..d21d5a14d069b24ce892c9cc88932481f59e6b93 100644 (file)
--- a/src/helper/Reply.cc
+++ b/src/helper/Reply.cc
@@ -151,6 +151,58 @@ isKeyNameChar(char c)
     return false;
 }
 
+/// warns admin about problematic key=value pairs
+void
+Helper::Reply::CheckReceivedKey(const SBuf &key, const SBuf &value)
+{
+    // Squid recognizes these keys (by name) in some helper responses
+    static const std::vector<SBuf> recognized = {
+        SBuf("clt_conn_tag"),
+        SBuf("group"),
+        SBuf("ha1"),
+        SBuf("log"),
+        SBuf("message"),
+        SBuf("nonce"),
+        SBuf("password"),
+        SBuf("rewrite-url"),
+        SBuf("status"),
+        SBuf("store-id"),
+        SBuf("tag"),
+        SBuf("token"),
+        SBuf("url"),
+        SBuf("user")
+    };
+
+    // TODO: Merge with Notes::ReservedKeys(). That list has an entry that Squid
+    // sources do _not_ recognize today ("ttl"), and it is missing some
+    // recognized entries ("clt_conn_tag", "nonce", store-id", and "token").
+
+    if (key.isEmpty()) {
+        debugs(84, DBG_IMPORTANT, "WARNING: Deprecated from-helper annotation without a name: " <<
+               key << '=' << value <<
+               Debug::Extra << "advice: Name or remove this annotation");
+        // TODO: Skip/ignore these annotations.
+        return;
+    }
+
+    // We do not check custom keys for repetitions because Squid supports them:
+    // The "note" ACL checks all of them and %note prints all of them.
+    if (*key.rbegin() == '_')
+        return; // a custom key
+
+    // To simplify, we allow all recognized keys, even though some of them are
+    // only expected from certain helpers or even only in certain reply types.
+    // To simplify and optimize, we do not check recognized keys for repetitions
+    // because _some_ of them (e.g., "message") do support repetitions.
+    if (std::find(recognized.begin(), recognized.end(), key) != recognized.end())
+        return; // a Squid-recognized key
+
+    debugs(84, DBG_IMPORTANT, "WARNING: Unsupported or unexpected from-helper annotation with a name reserved for Squid use: " <<
+           key << '=' << value <<
+           Debug::Extra << "advice: If this is a custom annotation, rename it to add a trailing underscore: " <<
+           key << '_');
+}
+
 void
 Helper::Reply::parseResponseKeys()
 {
@@ -176,7 +228,11 @@ Helper::Reply::parseResponseKeys()
         if (v != nullptr && urlDecode && (p-v) > 2) // 1-octet %-escaped requires 3 bytes
             rfc1738_unescape(v);
 
-        notes.add(key, v ? v : ""); // value can be empty, but must not be NULL
+        // TODO: Convert the above code to use Tokenizer and SBuf
+        const SBuf parsedKey(key);
+        const SBuf parsedValue(v); // allow empty values (!v or !*v)
+        CheckReceivedKey(parsedKey, parsedValue);
+        notes.add(parsedKey, parsedValue);
 
         other_.consume(p - other_.content());
         other_.consumeWhitespacePrefix();

diff --git a/src/helper/Reply.h b/src/helper/Reply.h
index 902051aeaa21e7be6701e6db7532b0e1f405b410..3c75a51604b29a204e4346a41ea10f61e840c791 100644 (file)
--- a/src/helper/Reply.h
+++ b/src/helper/Reply.h
@@ -64,6 +64,8 @@ public:
     /// The stateful replies should include the reservation ID
     Helper::ReservationId reservationId;
 private:
+    static void CheckReceivedKey(const SBuf &, const SBuf &);
+
     void parseResponseKeys();
 
     /// Return an empty MemBuf.