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 97a3a5020e9fa5972008456f1ea928d4d54cb45f..08dcda540d2a0dbdcf3620f99b6bf5c2f4cad01c 100644 (file)
--- 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 <iostream>
+
 // 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 a26a4ea4dd759e69ab5cb7095b35a58381cb74e5..f31c9240da28ef87de047e6aef4a4feead54624f 100644 (file)
--- 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 4ff71b87acb92b7bb3521d19bba608b2c1c827e1..776eadb44f0e1e3ad488000026cbc80903788cdf 100644 (file)
--- 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 (file)
index 0000000..68229cb
--- /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<bool> decision_;
+};
+
+#endif /* SQUID_SRC_BASE_SUPPORTORVETO_H */
+

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 3247d793cb9be83074290835282439ce7e5a20d4..d6ca8bcc8d9bf43bfb767b09a325ffe3da119008 100644 (file)
--- 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 cc6ee208e05cfa9b771019adf5486b6e7bdd774c..cef4ea08ac469a2b03f2a9ae4aa17e53e0a749b5 100644 (file)
--- 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 1be19f7d2c2acb98deeb87e8b2f104febd277c03..87d5435597f2ad0f4d90b43a9e980726943fb177 100644 (file)
--- 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 9cacf35c75bdcac98a9f8dc3d43ab0f2d1a11a99..89ad44311faac6761471c7682e269d10dd987605 100644 (file)
--- 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 9d1fbf840c1521c978250ed2c899f568a970471f..9db75d5d265555269780eec543e2fbd67c24163b 100644 (file)
--- 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 5780fa6011db8a52c628834527e159273ae90bc3..48b6ba5d01962ce74e61a98bbedbcc1acd4ca686 100644 (file)
--- 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 a0687f6b4317b65f99368aa41ef553f47cb235c0..cffe9834451440b73db760b250c54171aac77e2f 100644 (file)
--- 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);