From 27c36771bf145c2f8ca1efab6743b9e087867ab5 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 8 Jan 2023 12:43:48 +0000 Subject: [PATCH] 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? --- src/helper/Reply.cc | 58 ++++++++++++++++++++++++++++++++++++++++++++- src/helper/Reply.h | 2 ++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/helper/Reply.cc b/src/helper/Reply.cc index 5e8826752b..d21d5a14d0 100644 --- 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 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 902051aeaa..3c75a51604 100644 --- 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. -- 2.39.5