Do not cache (and do not serve cached) cache manager responses (#1185)

The fixed bug affected cache manager transactions that were using
/squid-internal-mgr URL path prefix with http(s) URL scheme. It did not
affect transactions that were using legacy cache_object URL scheme.

Stale cache manager responses had their Age response header set to the
number of seconds since Unix epoch. If disk and memory caches were
disabled, then cache manager requests just triggered "found KEY_PRIVATE"
WARNINGs in cache.log (for reasons that remain unclear).

Probably broken since 2011 commit e37bd29 that did not expand
HttpRequest::maybeCacheable() (called cacheable() back then)
PROTO_CACHE_OBJECT check to include /squid-internal-mgr requests.

Also added missing Access-Control-* response headers to cache manager
responses in SMP mode and reduced code duplication related to sending
those headers (which led to them missing in SMP Squids).

diff --git a/src/CacheManager.h b/src/CacheManager.h
index faec85a925ff6bd49dd905ae1f2dd30243696822..ad6aabaf6b4cd2fe46a02d6579ec3d770cb78f13 100644 (file)
--- a/src/CacheManager.h
+++ b/src/CacheManager.h
@@ -20,6 +20,7 @@
 #include <vector>
 
 class HttpRequest;
+class HttpReply;
 
 /**
  * a CacheManager - the menu system for interacting with squid.
@@ -49,6 +50,11 @@ public:
     static CacheManager* GetInstance();
     const char *ActionProtection(const Mgr::ActionProfilePointer &profile);
 
+    /// Add HTTP response headers specific/common to all cache manager replies,
+    /// including cache manager errors and Action reports.
+    /// \param httpOrigin the value of Origin header in the trigger HTTP request (or nil)
+    static void PutCommonResponseHeaders(HttpReply &, const char *httpOrigin);
+
 protected:
     CacheManager() {} ///< use Instance() instead
 

diff --git a/src/RequestFlags.h b/src/RequestFlags.h
index acfe3acf1ded6ca71997218987eb348b37e32186..ceef9213e519df88ab4e8a48b10b07def0809583 100644 (file)
--- a/src/RequestFlags.h
+++ b/src/RequestFlags.h
@@ -72,8 +72,14 @@ public:
     /// This applies to TPROXY traffic that has not had spoofing disabled through
     /// the spoof_client_ip squid.conf ACL.
     bool spoofClientIp = false;
-    /** set if the request is internal (\see ClientHttpRequest::flags.internal)*/
+
+    /// whether the request targets a /squid-internal- resource (e.g., a MIME
+    /// icon or a cache manager page) served by this Squid instance
+    /// \sa ClientHttpRequest::flags.internal
+    /// TODO: Rename to avoid a false implication that this flag is true for
+    /// requests for /squid-internal- resources served by other Squid instances.
     bool internal = false;
+
     /** if set, request to try very hard to keep the connection alive */
     bool mustKeepalive = false;
     /** set if the request wants connection oriented auth */

diff --git a/src/cache_manager.cc b/src/cache_manager.cc
index c12dd96b86a0761dcd807c802161c8a50b3fca5b..317386014492515929d6e7a7368787f662a062ed 100644 (file)
--- a/src/cache_manager.cc
+++ b/src/cache_manager.cc
@@ -17,6 +17,7 @@
 #include "error/ExceptionErrorDetail.h"
 #include "errorpage.h"
 #include "fde.h"
+#include "HttpHdrCc.h"
 #include "HttpReply.h"
 #include "HttpRequest.h"
 #include "mgr/Action.h"
@@ -38,6 +39,7 @@
 #include "wordlist.h"
 
 #include <algorithm>
+#include <memory>
 
 /// \ingroup CacheManagerInternal
 #define MGR_PASSWD_SZ 128
@@ -369,14 +371,9 @@ CacheManager::start(const Comm::ConnectionPointer &client, HttpRequest *request,
          */
         rep->header.putAuth("Basic", actionName);
 #endif
-        // Allow cachemgr and other XHR scripts access to our version string
-        if (request->header.has(Http::HdrType::ORIGIN)) {
-            rep->header.putExt("Access-Control-Allow-Origin",request->header.getStr(Http::HdrType::ORIGIN));
-#if HAVE_AUTH_MODULE_BASIC
-            rep->header.putExt("Access-Control-Allow-Credentials","true");
-#endif
-            rep->header.putExt("Access-Control-Expose-Headers","Server");
-        }
+
+        const auto originOrNil = request->header.getStr(Http::HdrType::ORIGIN);
+        PutCommonResponseHeaders(*rep, originOrNil);
 
         /* store the reply */
         entry->replaceHttpReply(rep);
@@ -404,14 +401,10 @@ CacheManager::start(const Comm::ConnectionPointer &client, HttpRequest *request,
         HttpReply *rep = err.BuildHttpReply();
         if (strncmp(rep->body.content(),"Internal Error:", 15) == 0)
             rep->sline.set(Http::ProtocolVersion(1,1), Http::scNotFound);
-        // Allow cachemgr and other XHR scripts access to our version string
-        if (request->header.has(Http::HdrType::ORIGIN)) {
-            rep->header.putExt("Access-Control-Allow-Origin",request->header.getStr(Http::HdrType::ORIGIN));
-#if HAVE_AUTH_MODULE_BASIC
-            rep->header.putExt("Access-Control-Allow-Credentials","true");
-#endif
-            rep->header.putExt("Access-Control-Expose-Headers","Server");
-        }
+
+        const auto originOrNil = request->header.getStr(Http::HdrType::ORIGIN);
+        PutCommonResponseHeaders(*rep, originOrNil);
+
         entry->replaceHttpReply(rep);
         entry->complete();
         return;
@@ -475,6 +468,27 @@ CacheManager::PasswdGet(Mgr::ActionPasswordList * a, const char *action)
     return nullptr;
 }
 
+void
+CacheManager::PutCommonResponseHeaders(HttpReply &response, const char *httpOrigin)
+{
+    // Allow cachemgr and other XHR scripts access to our version string
+    if (httpOrigin) {
+        response.header.putExt("Access-Control-Allow-Origin", httpOrigin);
+#if HAVE_AUTH_MODULE_BASIC
+        response.header.putExt("Access-Control-Allow-Credentials", "true");
+#endif
+        response.header.putExt("Access-Control-Expose-Headers", "Server");
+    }
+
+    std::unique_ptr<HttpHdrCc> cc(new HttpHdrCc());
+    // this is honored by more caches but allows pointless revalidation;
+    // revalidation will always fail because we do not support it (yet?)
+    cc->noCache(String());
+    // this is honored by fewer caches but prohibits pointless revalidation
+    cc->noStore(true);
+    response.putCc(cc.release());
+}
+
 CacheManager*
 CacheManager::GetInstance()
 {

diff --git a/src/client_side.cc b/src/client_side.cc
index 3d6f0a5727ec66c359e891a8104917594db7c401..de5c5512a377786df5191541f92f4c21c5b8447a 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1654,10 +1654,16 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
             http->setLogUriToRequestUri();
         } else
             debugs(33, 2, "internal URL found: " << request->url.getScheme() << "://" << request->url.authority(true) << " (not this proxy)");
+
+        if (ForSomeCacheManager(request->url.path()))
+            request->flags.disableCacheUse("cache manager URL");
     }
 
     request->flags.internal = http->flags.internal;
 
+    if (request->url.getScheme() == AnyP::PROTO_CACHE_OBJECT)
+        request->flags.disableCacheUse("cache_object URL scheme");
+
     if (!isFtp) {
         // XXX: for non-HTTP messages instantiate a different Http::Message child type
         // for now Squid only supports HTTP requests

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index b8718c03799f75cbf79ddd37abb8c154e6e76931..62f6da6b8e05e319f1488e9064ca9e56a38ba42e 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -1480,6 +1480,9 @@ clientReplyContext::identifyStoreObject()
 
     // client sent CC:no-cache or some other condition has been
     // encountered which prevents delivering a public/cached object.
+    // XXX: The above text does not match the condition below. It might describe
+    // the opposite condition, but the condition itself should be adjusted
+    // (e.g., to honor flags.noCache in cache manager requests).
     if (!r->flags.noCache || r->flags.internal) {
         const auto e = storeGetPublicByRequest(r);
         identifyFoundObject(e, storeLookupString(bool(e)));

diff --git a/src/internal.cc b/src/internal.cc
index a805dc81ef82cf099ec4fd9823a3e8b8881e574f..be5d61c29b0098baa2c1b2694febc8a9c9aa86da 100644 (file)
--- a/src/internal.cc
+++ b/src/internal.cc
@@ -10,6 +10,7 @@
 
 #include "squid.h"
 #include "AccessLogEntry.h"
+#include "base/Assure.h"
 #include "CacheManager.h"
 #include "comm/Connection.h"
 #include "errorpage.h"
@@ -32,12 +33,15 @@ void
 internalStart(const Comm::ConnectionPointer &clientConn, HttpRequest * request, StoreEntry * entry, const AccessLogEntry::Pointer &ale)
 {
     ErrorState *err;
+
+    Assure(request);
     const SBuf upath = request->url.path();
     debugs(76, 3, clientConn << " requesting '" << upath << "'");
 
+    Assure(request->flags.internal);
+
     static const SBuf netdbUri("/squid-internal-dynamic/netdb");
     static const SBuf storeDigestUri("/squid-internal-periodic/store_digest");
-    static const SBuf mgrPfx("/squid-internal-mgr/");
 
     if (upath == netdbUri) {
         netdbBinaryExchange(entry);
@@ -54,8 +58,8 @@ internalStart(const Comm::ConnectionPointer &clientConn, HttpRequest * request,
         entry->replaceHttpReply(reply);
         entry->append(msgbuf, strlen(msgbuf));
         entry->complete();
-    } else if (upath.startsWith(mgrPfx)) {
-        debugs(17, 2, "calling CacheManager due to URL-path " << mgrPfx);
+    } else if (ForSomeCacheManager(upath)) {
+        debugs(17, 2, "calling CacheManager due to URL-path");
         CacheManager::GetInstance()->start(clientConn, request, entry, ale);
     } else {
         debugObj(76, 1, "internalStart: unknown request:\n",
@@ -79,6 +83,13 @@ internalStaticCheck(const SBuf &urlPath)
     return urlPath.startsWith(InternalStaticPfx);
 }
 
+bool
+ForSomeCacheManager(const SBuf &urlPath)
+{
+    static const SBuf mgrPfx("/squid-internal-mgr");
+    return urlPath.startsWith(mgrPfx);
+}
+
 /*
  * makes internal url with a given host and port (remote internal url)
  */

diff --git a/src/internal.h b/src/internal.h
index 2cc934d377c5a361fb0b3706eb4844a07fdce8c7..0fb291291053e9e43e445ac855bc9aedf1b1d220 100644 (file)
--- a/src/internal.h
+++ b/src/internal.h
@@ -29,5 +29,9 @@ char *internalRemoteUri(bool, const char *, unsigned short, const char *, const
 const char *internalHostname(void);
 int internalHostnameIs(const char *);
 
+/// whether the given request URL path points to a cache manager (not
+/// necessarily running on this Squid instance)
+bool ForSomeCacheManager(const SBuf &);
+
 #endif /* SQUID_INTERNAL_H_ */
 

diff --git a/src/mgr/Action.cc b/src/mgr/Action.cc
index 56a0de55adf738cd6c771ee2b707806ba8943439..2c8bf63bf573b820c0cd5b5910d12b2877c214ac 100644 (file)
--- a/src/mgr/Action.cc
+++ b/src/mgr/Action.cc
@@ -9,6 +9,7 @@
 /* DEBUG: section 16    Cache Manager API */
 
 #include "squid.h"
+#include "CacheManager.h"
 #include "comm/Connection.h"
 #include "HttpReply.h"
 #include "ipc/Port.h"
@@ -103,15 +104,11 @@ Mgr::Action::fillEntry(StoreEntry* entry, bool writeHttpHeader)
     if (writeHttpHeader) {
         HttpReply *rep = new HttpReply;
         rep->setHeaders(Http::scOkay, nullptr, contentType(), -1, squid_curtime, squid_curtime);
-        // Allow cachemgr and other XHR scripts access to our version string
-        const ActionParams &params = command().params;
-        if (params.httpOrigin.size() > 0) {
-            rep->header.putExt("Access-Control-Allow-Origin", params.httpOrigin.termedBuf());
-#if HAVE_AUTH_MODULE_BASIC
-            rep->header.putExt("Access-Control-Allow-Credentials","true");
-#endif
-            rep->header.putExt("Access-Control-Expose-Headers","Server");
-        }
+
+        const auto &origin = command().params.httpOrigin;
+        const auto originOrNil = origin.size() ? origin.termedBuf() : nullptr;
+        CacheManager::PutCommonResponseHeaders(*rep, originOrNil);
+
         entry->replaceHttpReply(rep);
     }
 

diff --git a/src/mgr/Inquirer.cc b/src/mgr/Inquirer.cc
index c6f52c72f26caa43fb40f17b66c8201bf779ef64..6db57a9fa70f129a95a6f27464e2cf0c6020bbf6 100644 (file)
--- a/src/mgr/Inquirer.cc
+++ b/src/mgr/Inquirer.cc
@@ -11,6 +11,7 @@
 #include "squid.h"
 #include "AccessLogEntry.h"
 #include "base/TextException.h"
+#include "CacheManager.h"
 #include "comm.h"
 #include "comm/Connection.h"
 #include "comm/Write.h"
@@ -73,6 +74,9 @@ Mgr::Inquirer::start()
     Must(Comm::IsConnOpen(conn));
     Must(aggrAction != nullptr);
 
+    const auto &origin = aggrAction->command().params.httpOrigin;
+    const auto originOrNil = origin.size() ? origin.termedBuf() : nullptr;
+
     std::unique_ptr<MemBuf> replyBuf;
     if (strands.empty()) {
         const char *url = aggrAction->command().params.httpUri.termedBuf();
@@ -80,10 +84,12 @@ Mgr::Inquirer::start()
         auto *req = HttpRequest::FromUrlXXX(url, mx);
         ErrorState err(ERR_INVALID_URL, Http::scNotFound, req, nullptr);
         std::unique_ptr<HttpReply> reply(err.BuildHttpReply());
+        CacheManager::PutCommonResponseHeaders(*reply, originOrNil);
         replyBuf.reset(reply->pack());
     } else {
         std::unique_ptr<HttpReply> reply(new HttpReply);
         reply->setHeaders(Http::scOkay, nullptr, "text/plain", -1, squid_curtime, squid_curtime);
+        CacheManager::PutCommonResponseHeaders(*reply, originOrNil);
         reply->header.putStr(Http::HdrType::CONNECTION, "close"); // until we chunk response
         replyBuf.reset(reply->pack());
     }

diff --git a/src/tests/stub_cache_manager.cc b/src/tests/stub_cache_manager.cc
index f200ae39c4b5dc0b264a1931206f9c91d99f681f..6acb56f09eb1f578c643903e208a43f8cabd4933 100644 (file)
--- a/src/tests/stub_cache_manager.cc
+++ b/src/tests/stub_cache_manager.cc
@@ -26,4 +26,5 @@ void Mgr::RegisterAction(char const*, char const*, OBJH, int, int) {}
 void Mgr::RegisterAction(char const *, char const *, Mgr::ClassActionCreationHandler *, int, int) {}
 
 Mgr::Action::Pointer CacheManager::createRequestedAction(const Mgr::ActionParams &) STUB_RETVAL(nullptr)
+void CacheManager::PutCommonResponseHeaders(HttpReply &, const char *) STUB