From e37329828e58a274b01ef4276d96607bf190f8a3 Mon Sep 17 00:00:00 2001
From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Date: Fri, 16 Jun 2023 15:10:59 +0000
Subject: [PATCH] Forget non-peer access details (#1378)

We were using the CachePeer class to record the address of each non-peer
sending us certain unwanted ICP responses, along with the number of such
responses and a histogram of associated ICP opcodes. Since 1997 commit
e102ebd, these historical records were accumulated without a limit and
linearly searched, endangering a Squid instance that used ICP. Using
CachePeer to store this non-cache_peer information also complicated
cache_peer code.

This change removes these records and the corresponding mgr:non_peers
report, leaving just the cache.log warning about unexpected messages.
The warning is useful because these ICP messages from non-peers indicate
a cache hierarchy misconfiguration or a fairly sophisticated attack.

This is the simplest fix that minimizes Squid and developer resources
spent on handling these errors.
---
 doc/release-notes/release-7.sgml.in | 20 +++++++++++--
 src/CachePeer.h                     |  6 ++--
 src/cf.data.pre                     |  1 -
 src/neighbors.cc                    | 46 ++++-------------------------
 4 files changed, 25 insertions(+), 48 deletions(-)

diff --git a/doc/release-notes/release-7.sgml.in b/doc/release-notes/release-7.sgml.in
index 086e6350d8..dffca86487 100644
--- a/doc/release-notes/release-7.sgml.in
+++ b/doc/release-notes/release-7.sgml.in
@@ -30,11 +30,23 @@ The Squid-@SQUID_RELEASE@ change history can be <url url="https://github.com/squ
 
 <p>The most important of these new features are:
 <itemize>
-	<item>
+	<item>Cache Manager changes
 </itemize>
 
-Most user-facing changes are reflected in squid.conf (see below).
+<p>Most user-facing changes are reflected in squid.conf (see further below).
 
+<sect1>Cache Manager changes<label id="mgr">
+<p>For more information about the Cache Manager feature, see <url url="https://wiki.squid-cache.org/Features/CacheManager/Index" name="wiki">.
+
+<p>
+<descrip>
+	<tag>non_peers</tag>
+	<p>Removed the <em>mgr:non_peers</em> report. Squid still ignores
+	unexpected ICP responses but no longer remembers the details that comprised
+	the removed report. The senders of these ICP messages are still reported to
+	cache.log at debugging level 1 (with an exponential backoff).
+
+</descrip>
 
 <sect>Changes to squid.conf since Squid-@SQUID_RELEASE_OLD@
 <p>
@@ -60,6 +72,10 @@ This section gives an account of those changes in three categories:
 	<tag>buffered_logs</tag>
 	<p>Honor the <em>off</em> setting in 'udp' access_log module.
 
+	<tag>cachemgr_passwd</tag>
+	<p>Removed the <em>non_peers</em> action. See the Cache Manager
+	<ref id="mgr" name="section"> for details.
+
 </descrip>
 
 <sect1>Removed directives<label id="removeddirectives">
diff --git a/src/CachePeer.h b/src/CachePeer.h
index a5cfbea3f3..2f3ff30af5 100644
--- a/src/CachePeer.h
+++ b/src/CachePeer.h
@@ -52,16 +52,14 @@ public:
 
     /// cache_peer name (if explicitly configured) or hostname (otherwise).
     /// Unique across already configured cache_peers in the current configuration.
-    /// Not necessarily unique across discovered non-peers (see mgr:non_peers).
     /// The value may change during CachePeer configuration.
     /// The value affects various peer selection hashes (e.g., carp.hash).
     /// Preserves configured spelling (i.e. does not lower letters case).
     /// Never nil.
     char *name = nullptr;
 
-    /// The lowercase version of the configured cache_peer hostname or
-    /// the IP address of a non-peer (see mgr:non_peers).
-    /// May not be unique among cache_peers and non-peers.
+    /// The lowercase version of the configured cache_peer hostname.
+    /// May not be unique among cache_peers.
     /// Never nil.
     char *host = nullptr;
 
diff --git a/src/cf.data.pre b/src/cf.data.pre
index c6783f7be0..6438b254e9 100644
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -10354,7 +10354,6 @@ DOC_START
 		mem
 		menu
 		netdb
-		non_peers
 		objects
 		offline_toggle *
 		pconn
diff --git a/src/neighbors.cc b/src/neighbors.cc
index cf03505159..1de37d55af 100644
--- a/src/neighbors.cc
+++ b/src/neighbors.cc
@@ -70,7 +70,6 @@ static IRCB peerCountHandleIcpReply;
 
 static void neighborIgnoreNonPeer(const Ip::Address &, icp_opcode);
 static OBJH neighborDumpPeers;
-static OBJH neighborDumpNonPeers;
 static void dump_peers(StoreEntry * sentry, CachePeer * peers);
 
 static unsigned short echo_port;
@@ -545,12 +544,6 @@ neighborsRegisterWithCacheManager()
     Mgr::RegisterAction("server_list",
                         "Peer Cache Statistics",
                         neighborDumpPeers, 0, 1);
-
-    if (Comm::IsConnOpen(icpIncomingConn)) {
-        Mgr::RegisterAction("non_peers",
-                            "List of Unknown sites sending ICP messages",
-                            neighborDumpNonPeers, 0, 1);
-    }
 }
 
 void
@@ -942,38 +935,15 @@ neighborCountIgnored(CachePeer * p)
     ++NLateReplies;
 }
 
-static CachePeer *non_peers = nullptr;
-
 static void
 neighborIgnoreNonPeer(const Ip::Address &from, icp_opcode opcode)
 {
-    CachePeer *np;
-
-    for (np = non_peers; np; np = np->next) {
-        if (np->in_addr != from)
-            continue;
-
-        if (np->in_addr.port() != from.port())
-            continue;
-
-        break;
-    }
-
-    if (np == nullptr) {
-        char fromStr[MAX_IPSTRLEN];
-        from.toStr(fromStr, sizeof(fromStr));
-        np = new CachePeer(fromStr);
-        np->in_addr = from;
-        np->icp.port = from.port();
-        np->type = PEER_NONE;
-        np->next = non_peers;
-        non_peers = np;
+    static uint64_t ignoredReplies = 0;
+    if (isPowTen(++ignoredReplies)) {
+        debugs(15, DBG_IMPORTANT, "WARNING: Ignored " << ignoredReplies << " ICP replies from non-peers" <<
+               Debug::Extra << "last seen non-peer source address: " << from <<
+               Debug::Extra << "last seen ICP reply opcode: " << icp_opcode_str[opcode]);
     }
-
-    ++ np->icp.counts[opcode];
-
-    if (isPowTen(++np->stats.ignored_replies))
-        debugs(15, DBG_IMPORTANT, "WARNING: Ignored " << np->stats.ignored_replies << " replies from non-peer " << *np);
 }
 
 /* ignoreMulticastReply
@@ -1451,12 +1421,6 @@ neighborDumpPeers(StoreEntry * sentry)
     dump_peers(sentry, Config.peers);
 }
 
-static void
-neighborDumpNonPeers(StoreEntry * sentry)
-{
-    dump_peers(sentry, non_peers);
-}
-
 void
 dump_peer_options(StoreEntry * sentry, CachePeer * p)
 {
-- 
2.47.3