From 92a5adb73b7a50d725fd122c6d27cf8ce1c109f3 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 27 Jun 2023 11:58:16 +0000 Subject: [PATCH] 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). --- src/CacheManager.h | 6 +++++ src/RequestFlags.h | 8 +++++- src/cache_manager.cc | 46 +++++++++++++++++++++------------ src/client_side.cc | 6 +++++ src/client_side_reply.cc | 3 +++ src/internal.cc | 17 +++++++++--- src/internal.h | 4 +++ src/mgr/Action.cc | 15 +++++------ src/mgr/Inquirer.cc | 6 +++++ src/tests/stub_cache_manager.cc | 1 + 10 files changed, 83 insertions(+), 29 deletions(-) diff --git a/src/CacheManager.h b/src/CacheManager.h index faec85a925..ad6aabaf6b 100644 --- a/src/CacheManager.h +++ b/src/CacheManager.h @@ -20,6 +20,7 @@ #include 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 acfe3acf1d..ceef9213e5 100644 --- 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 c12dd96b86..3173860144 100644 --- 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 +#include /// \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 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 3d6f0a5727..de5c5512a3 100644 --- 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 b8718c0379..62f6da6b8e 100644 --- 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 a805dc81ef..be5d61c29b 100644 --- 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 2cc934d377..0fb2912910 100644 --- 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 56a0de55ad..2c8bf63bf5 100644 --- 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 ¶ms = 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 c6f52c72f2..6db57a9fa7 100644 --- 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 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 reply(err.BuildHttpReply()); + CacheManager::PutCommonResponseHeaders(*reply, originOrNil); replyBuf.reset(reply->pack()); } else { std::unique_ptr 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 f200ae39c4..6acb56f09e 100644 --- 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 -- 2.39.5