Do not forward HTCP CLR requests denied by htcp_clr_access (#2246)

Before this change:

* htcp_access controls TST access
* htcp_clr_access controls CLR access to local cache purging
* nothing controls CLR forwarding -- it is always allowed

After this change:

* htcp_access controls TST access
* htcp_clr_access controls CLR access to local cache purging
* htcp_clr_access controls CLR forwarding

This bug was discovered and detailed by Joshua Rogers
https://joshua.hu/

diff --git a/doc/release-notes/release-8.sgml.in b/doc/release-notes/release-8.sgml.in
index a521cf4f8d32927a2c715f8f5c7aab7f16677a6a..8ddb88821e7c9683a90f240551172fd17b9866ec 100644 (file)
--- a/doc/release-notes/release-8.sgml.in
+++ b/doc/release-notes/release-8.sgml.in
@@ -114,6 +114,12 @@ This section gives an account of those changes in three categories:
        connection should increase the configured limit by one to preserve
        previous behavior.
 
+       <tag>htcp_clr_access</tag>
+
+       <p>HTCP CLR requests denied by this directive are no longer forwarded to
+       cache_peers that are configured to receive forwarded CLR requests. Only
+       HTCP CLR requests allowed by this directive are forwarded to those
+       cache_peers.
 
 </descrip>
 

diff --git a/src/cf.data.pre b/src/cf.data.pre
index 5a6f7b90f3ff79a6bbea007c25bb36e50e96d627..76570527c2684e072bd430185e042565707a27fc 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -2037,22 +2037,24 @@ LOC: Config.accessList.htcp
 DEFAULT: none
 DEFAULT_DOC: Deny, unless rules exist in squid.conf.
 DOC_START
-       Allowing or Denying access to the HTCP port based on defined
-       access lists
+       Controls whether HTCP TST requests received on htcp_port are allowed.
 
        htcp_access  allow|deny [!]aclname ...
 
-       See also htcp_clr_access for details on access control for
-       cache purge (CLR) HTCP messages.
+       This directive does not control whether HTCP CLR requests are allowed.
+       Use htcp_clr_access directive for that.
+
+       This directive does not control whether HTCP requests with other opcodes
+       are allowed (e.g., NOP, MON, and SET). Squid ignores those HTCP requests.
 
        NOTE: The default if no htcp_access lines are present is to
-       deny all traffic. This default may cause problems with peers
+       deny all HTCP TST traffic. This default may cause problems with peers
        using the htcp option.
 
        This clause only supports fast acl types.
        See https://wiki.squid-cache.org/SquidFaq/SquidAcl for details.
 
-# Allow HTCP queries from local networks only
+# Allow HTCP TST queries from local networks only
 #htcp_access allow localnet
 #htcp_access deny all
 DOC_END
@@ -2064,12 +2066,14 @@ LOC: Config.accessList.htcp_clr
 DEFAULT: none
 DEFAULT_DOC: Deny, unless rules exist in squid.conf.
 DOC_START
-       Allowing or Denying access to purge content using HTCP based
-       on defined access lists.
-       See htcp_access for details on general HTCP access control.
+       Controls whether HTCP CLR requests received on htcp_port are allowed.
+       See htcp_access for controlling other HTCP messages.
 
        htcp_clr_access  allow|deny [!]aclname ...
 
+       HTCP CLR requests purge matching cached entries. They may be forwarded to
+       specially marked cache_peers (see cache_peer HTCP options for details).
+
        This clause only supports fast acl types.
        See https://wiki.squid-cache.org/SquidFaq/SquidAcl for details.
 

diff --git a/src/htcp.cc b/src/htcp.cc
index f3957899e53fec5d219f945d5bf788707733b807..da1e616b35e0d35d8ff0ca15124f70d6b0390c69 100644 (file)
--- a/src/htcp.cc
+++ b/src/htcp.cc
@@ -1236,7 +1236,7 @@ htcpSpecifier::checkedHit(StoreEntry *e)
 }
 
 static void
-htcpHandleClr(htcpDataHeader * hdr, char *buf, int sz, Ip::Address &from)
+htcpHandleClr(htcpDataHeader * const hdr, char * const buf, const int sz, Ip::Address &from)
 {
     /* buf[0/1] is reserved and reason */
     if (sz < 2) {
@@ -1246,10 +1246,9 @@ htcpHandleClr(htcpDataHeader * hdr, char *buf, int sz, Ip::Address &from)
     }
     int reason = static_cast<unsigned char>(buf[1]) << 4;
     debugs(31, 2, "HTCP CLR reason: " << reason);
-    buf += 2;
-    sz -= 2;
 
-    /* buf should be a SPECIFIER */
+    const auto specifierStart = buf + 2;
+    const auto specifierSize = sz - 2;
 
     if (sz == 0) {
         debugs(31, 4, "nothing to do");
@@ -1257,7 +1256,7 @@ htcpHandleClr(htcpDataHeader * hdr, char *buf, int sz, Ip::Address &from)
         return;
     }
 
-    htcpSpecifier::Pointer s(htcpUnpackSpecifier(buf, sz));
+    const auto s = htcpUnpackSpecifier(specifierStart, specifierSize);
 
     if (!s) {
         debugs(31, 3, "htcpUnpackSpecifier failed");
@@ -1302,15 +1301,10 @@ htcpHandleClr(htcpDataHeader * hdr, char *buf, int sz, Ip::Address &from)
     default:
         break;
     }
-}
 
-/*
- * Forward a CLR request to all peers who have requested that CLRs be
- * forwarded to them.
- */
-static void
-htcpForwardClr(char *buf, int sz)
-{
+    // Forward this CLR request to all peers who have requested that CLRs be
+    // forwarded to them.
+    // TODO: Consider not forwarding requests with htcpClrStore() < 0.
     for (const auto &p: CurrentCachePeers()) {
         if (!p->options.htcp) {
             continue;
@@ -1457,7 +1451,6 @@ htcpHandleMsg(char *buf, int sz, Ip::Address &from)
         break;
     case HTCP_CLR:
         htcpHandleClr(&hdr, hbuf, hsz, from);
-        htcpForwardClr(buf, sz);
         break;
     default:
         break;