From: Alex Rousskov Date: Thu, 8 Dec 2022 08:25:44 +0000 (+0000) Subject: Do not overwrite caching bans (#1189) X-Git-Tag: SQUID_6_0_1~61 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=aeeff7fd728b8d53a8091fe0ae8d4fd9c3488dae;p=thirdparty%2Fsquid.git Do not overwrite caching bans (#1189) To ban caching, Squid made RequestFlags::cachable false in several places (e.g., when a "cache deny" rule matched). To permit caching, Squid also made the flag true in several places (e.g., when maybeCacheable() was true). That combination worked as intended only when the cachable=false veto always came after all the cachable=true support votes. The code did not (and should not) enforce such a complicated/fragile timing invariant. Squid now correctly honors caching bans regardless of the updates order. This change addresses an old XXX, but we are not aware of any specific bugs fixed by this change. The primary purpose of this change is to make the existing baseline ban-honoring functionality easy to maintain. Also reduced code duplication across cachable=false,noCache=true code. --- diff --git a/src/RequestFlags.cc b/src/RequestFlags.cc index 97a3a5020e..08dcda540d 100644 --- a/src/RequestFlags.cc +++ b/src/RequestFlags.cc @@ -9,8 +9,11 @@ /* DEBUG: section 73 HTTP Request */ #include "squid.h" +#include "debug/Stream.h" #include "RequestFlags.h" +#include + // When adding new flags, please update cloneAdaptationImmune() as needed. // returns a partial copy of the flags that includes only those flags // that are safe for a related (e.g., ICAP-adapted) request to inherit @@ -23,3 +26,11 @@ RequestFlags::cloneAdaptationImmune() const return *this; } +void +RequestFlags::disableCacheUse(const char * const reason) +{ + debugs(16, 3, "for " << reason); + cachable.veto(); + noCache = true; // may already be true +} + diff --git a/src/RequestFlags.h b/src/RequestFlags.h index a26a4ea4dd..f31c9240da 100644 --- a/src/RequestFlags.h +++ b/src/RequestFlags.h @@ -11,6 +11,8 @@ #ifndef SQUID_REQUESTFLAGS_H_ #define SQUID_REQUESTFLAGS_H_ +#include "base/SupportOrVeto.h" + /** request-related flags * * Contains both flags marking a request's current state, @@ -28,8 +30,10 @@ public: bool auth = false; /** do not use keytabs for peer Kerberos authentication */ bool auth_no_keytab = false; - /** he response to the request may be stored in the cache */ - bool cachable = false; + + /// whether the response may be stored in the cache + SupportOrVeto cachable; + /** the request can be forwarded through the hierarchy */ bool hierarchical = false; /** a loop was detected on this request */ @@ -124,6 +128,11 @@ public: bool noCacheHack() const { return USE_HTTP_VIOLATIONS && nocacheHack; } + + /// ban satisfying the request from the cache and ban storing the response + /// in the cache + /// \param reason summarizes the marking decision context (for debugging) + void disableCacheUse(const char *reason); }; #endif /* SQUID_REQUESTFLAGS_H_ */ diff --git a/src/base/Makefile.am b/src/base/Makefile.am index 4ff71b87ac..776eadb44f 100644 --- a/src/base/Makefile.am +++ b/src/base/Makefile.am @@ -65,6 +65,7 @@ libbase_la_SOURCES = \ RunnersRegistry.cc \ RunnersRegistry.h \ Subscription.h \ + SupportOrVeto.h \ TextException.cc \ TextException.h \ TypeTraits.h \ diff --git a/src/base/SupportOrVeto.h b/src/base/SupportOrVeto.h new file mode 100644 index 0000000000..68229cba3e --- /dev/null +++ b/src/base/SupportOrVeto.h @@ -0,0 +1,37 @@ +/* + * Copyright (C) 1996-2022 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#ifndef SQUID_SRC_BASE_SUPPORTORVETO_H +#define SQUID_SRC_BASE_SUPPORTORVETO_H + +#include "base/Optional.h" + +/// a boolean flag that is false by default and becomes permanently false if vetoed +class SupportOrVeto +{ +public: + /// either the current explicit decision or, by default, false + bool decision() const { return decision_.value_or(false); } + + /// \copydoc decision() + operator bool() const { return decision(); } + + /// Makes (or keeps) decision() true in the absence of veto() calls. + /// No effect if veto() has been called. + void support() { if (!decision_) decision_ = true; } + + /// makes decision() false regardless of past or future support() calls + void veto() { decision_ = false; } + +private: + /// current decision (if any) + Optional decision_; +}; + +#endif /* SQUID_SRC_BASE_SUPPORTORVETO_H */ + diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 3247d793cb..d6ca8bcc8d 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -500,9 +500,12 @@ ClientRequestContext::hostHeaderVerifyFailed(const char *A, const char *B) debugs(85, 3, "SECURITY ALERT: Host header forgery detected on " << http->getConn()->clientConnection << " (" << A << " does not match " << B << ") on URL: " << http->request->effectiveRequestUri()); - // NP: it is tempting to use 'flags.noCache' but that is all about READing cache data. - // The problems here are about WRITE for new cache content, which means flags.cachable - http->request->flags.cachable = false; // MUST NOT cache (for now) + // MUST NOT cache (for now). It is tempting to set flags.noCache, but + // that flag is about satisfying _this_ request. We are actually OK with + // satisfying this request from the cache, but want to prevent _other_ + // requests from being satisfied using this response. + http->request->flags.cachable.veto(); + // XXX: when we have updated the cache key to base on raw-IP + URI this cacheable limit can go. http->request->flags.hierarchical = false; // MUST NOT pass to peers (for now) // XXX: when we have sorted out the best way to relay requests properly to peers this hierarchical limit can go. @@ -1095,7 +1098,10 @@ clientInterpretRequestHeaders(ClientHttpRequest * http) #endif - request->flags.cachable = http->request->maybeCacheable(); + if (http->request->maybeCacheable()) + request->flags.cachable.support(); + else + request->flags.cachable.veto(); if (clientHierarchical(http)) request->flags.hierarchical = true; @@ -1300,9 +1306,7 @@ ClientRequestContext::clientStoreIdDone(const Helper::Reply &reply) http->doCallouts(); } -/** Test cache allow/deny configuration - * Sets flags.cachable=1 if caching is not denied. - */ +/// applies "cache allow/deny" rules, asynchronously if needed void ClientRequestContext::checkNoCache() { @@ -1331,8 +1335,7 @@ ClientRequestContext::checkNoCacheDone(const Acl::Answer &answer) { acl_checklist = nullptr; if (answer.denied()) { - http->request->flags.noCache = true; // do not read reply from cache - http->request->flags.cachable = false; // do not store reply into cache + http->request->flags.disableCacheUse("a cache deny rule matched"); } http->doCallouts(); } diff --git a/src/http.cc b/src/http.cc index cc6ee208e0..cef4ea08ac 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1861,7 +1861,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request, */ if (!we_do_ranges && request->multipartRangeRequest()) { /* don't cache the result */ - request->flags.cachable = false; + request->flags.cachable.veto(); /* pretend it's not a range request */ request->ignoreRange("want to request the whole object"); request->flags.isRanged = false; diff --git a/src/peer_digest.cc b/src/peer_digest.cc index 1be19f7d2c..87d5435597 100644 --- a/src/peer_digest.cc +++ b/src/peer_digest.cc @@ -332,7 +332,7 @@ peerDigestRequest(PeerDigest * pd) /* update timestamps */ pd->times.requested = squid_curtime; pd_last_req_time = squid_curtime; - req->flags.cachable = true; + req->flags.cachable.support(); // prevent RELEASE_REQUEST in storeCreateEntry() /* the rest is based on clientReplyContext::processExpired() */ req->flags.refresh = true; diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index 9cacf35c75..89ad44311f 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -743,8 +743,7 @@ Ftp::Server::parseOneRequest() request->http_ver = Http::ProtocolVersion(Ftp::ProtocolVersion().major, Ftp::ProtocolVersion().minor); // Our fake Request-URIs are not distinctive enough for caching to work - request->flags.cachable = false; // XXX: reset later by maybeCacheable() - request->flags.noCache = true; + request->flags.disableCacheUse("FTP command wrapper"); request->header.putStr(Http::HdrType::FTP_COMMAND, cmd.c_str()); request->header.putStr(Http::HdrType::FTP_ARGUMENTS, params.c_str()); // may be "" @@ -1742,8 +1741,7 @@ Ftp::Server::setReply(const int code, const char *msg) assert(repContext != nullptr); RequestFlags reqFlags; - reqFlags.cachable = false; // force releaseRequest() in storeCreateEntry() - reqFlags.noCache = true; + reqFlags.disableCacheUse("FTP response wrapper"); repContext->createStoreEntry(http->request->method, reqFlags); http->storeEntry()->replaceHttpReply(reply); } diff --git a/src/store_digest.cc b/src/store_digest.cc index 9d1fbf840c..9db75d5d26 100644 --- a/src/store_digest.cc +++ b/src/store_digest.cc @@ -421,7 +421,7 @@ storeDigestRewriteStart(void *) auto req = HttpRequest::FromUrlXXX(url, mx); RequestFlags flags; - flags.cachable = true; + flags.cachable.support(); // prevent RELEASE_REQUEST in storeCreateEntry() StoreEntry *e = storeCreateEntry(url, url, flags, Http::METHOD_GET); assert(e); diff --git a/src/tests/testRock.cc b/src/tests/testRock.cc index 5780fa6011..48b6ba5d01 100644 --- a/src/tests/testRock.cc +++ b/src/tests/testRock.cc @@ -187,7 +187,7 @@ StoreEntry * testRock::createEntry(const int i) { RequestFlags flags; - flags.cachable = true; + flags.cachable.support(); StoreEntry *const pe = storeCreateEntry(storeId(i), "dummy log url", flags, Http::METHOD_GET); auto &rep = pe->mem().adjustableBaseReply(); diff --git a/src/tests/testUfs.cc b/src/tests/testUfs.cc index a0687f6b43..cffe983445 100644 --- a/src/tests/testUfs.cc +++ b/src/tests/testUfs.cc @@ -145,7 +145,7 @@ testUfs::testUfsSearch() { /* Create "vary" base object */ RequestFlags flags; - flags.cachable = true; + flags.cachable.support(); StoreEntry *pe = storeCreateEntry("dummy url", "dummy log url", flags, Http::METHOD_GET); auto &reply = pe->mem().adjustableBaseReply(); reply.setHeaders(Http::scOkay, "dummy test object", "x-squid-internal/test", 0, -1, squid_curtime + 100000);