Backport of Fix two issues with chaining ECS enabled queries

Signed-off-by: Otto Moerbeek <otto.moerbeek@open-xchange.com>

diff --git a/pdns/ednsoptions.cc b/pdns/ednsoptions.cc
index 78fda3ae06dc47863641ff37815279665cea76eb..e8b3aef334125ef453e4cb80ff7a3a780fdce723 100644 (file)
--- a/pdns/ednsoptions.cc
+++ b/pdns/ednsoptions.cc
@@ -105,7 +105,7 @@ bool slowParseEDNSOptions(const PacketBuffer& packet, EDNSOptionViewMap& options
   }
 
   if (ntohs(dnsHeader->arcount) == 0) {
-    throw std::runtime_error("slowParseEDNSOptions() should not be called for queries that have no EDNS");
+    return false;
   }
 
   try {

diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc
index df3a68f7cf9ef74f9d72ae2adb0c21549ad2fae1..909f89607ab95ea48dfdba3e1099307570f14618 100644 (file)
--- a/pdns/recursordist/pdns_recursor.cc
+++ b/pdns/recursordist/pdns_recursor.cc
@@ -288,9 +288,9 @@ LWResult::Result asendto(const void* data, size_t len, int /* flags */,
     // Line below detected an issue with the two ways of ordering PacketIDs (birthday and non-birthday)
     assert(chain.first->key->domain == pident->domain); // NOLINT
     // don't chain onto existing chained waiter or a chain already processed
-    if (chain.first->key->fd > -1 && !chain.first->key->closed) {
+    if (chain.first->key->fd > -1 && !chain.first->key->closed && pident->ecsSubnet == chain.first->key->ecsSubnet) {
+      *fileDesc = -1;
       chain.first->key->chain.insert(qid); // we can chain
-      *fileDesc = -1;  // gets used in waitEvent / sendEvent later on
       return LWResult::Result::Success;
     }
   }
@@ -317,6 +317,8 @@ LWResult::Result asendto(const void* data, size_t len, int /* flags */,
   return LWResult::Result::Success;
 }
 
+static bool checkIncomingECSSource(const PacketBuffer& packet, const Netmask& subnet);
+
 LWResult::Result arecvfrom(PacketBuffer& packet, int /* flags */, const ComboAddress& fromAddr, size_t& len,
                            uint16_t qid, const DNSName& domain, uint16_t qtype, int fileDesc, const std::optional<EDNSSubnetOpts>& ecs, const struct timeval& now)
 {
@@ -349,9 +351,8 @@ LWResult::Result arecvfrom(PacketBuffer& packet, int /* flags */, const ComboAdd
     len = packet.size();
 
     // In ecs hardening mode, we consider a missing or a mismatched ECS in the reply as a case for
-    // retrying without ECS (matchingECSReceived only gets set if a matching ECS was received). The actual
-    // logic to do that is in Syncres::doResolveAtThisIP()
-    if (g_ECSHardening && pident->ecsSubnet && !*pident->matchingECSReceived) {
+    // retrying without ECS. The actual logic to do that is in Syncres::doResolveAtThisIP()
+    if (g_ECSHardening && pident->ecsSubnet && !checkIncomingECSSource(packet, *pident->ecsSubnet)) {
       t_Counters.at(rec::Counter::ecsMissingCount)++;
       return LWResult::Result::ECSMissing;
     }
@@ -2862,7 +2863,7 @@ void distributeAsyncFunction(const string& packet, const pipefunc_t& func)
 }
 
 // resend event to everybody chained onto it
-static void doResends(MT_t::waiters_t::iterator& iter, const std::shared_ptr<PacketID>& resend, const PacketBuffer& content, const std::optional<bool>& matchingECSReceived)
+static void doResends(MT_t::waiters_t::iterator& iter, const std::shared_ptr<PacketID>& resend, const PacketBuffer& content)
 {
   // We close the chain for new entries, since they won't be processed anyway
   iter->key->closed = true;
@@ -2871,10 +2872,6 @@ static void doResends(MT_t::waiters_t::iterator& iter, const std::shared_ptr<Pac
     return;
   }
 
-  // Only set if g_ECSHardening
-  if (matchingECSReceived) {
-    iter->key->matchingECSReceived = matchingECSReceived;
-  }
   for (auto i = iter->key->chain.begin(); i != iter->key->chain.end(); ++i) {
     auto packetID = std::make_shared<PacketID>(*resend);
     packetID->fd = -1;
@@ -2929,7 +2926,7 @@ static void handleUDPServerResponse(int fileDesc, FDMultiplexer::funcparam_t& va
     PacketBuffer empty;
     auto iter = g_multiTasker->d_waiters.find(pid);
     if (iter != g_multiTasker->d_waiters.end()) {
-      doResends(iter, pid, empty, false);
+      doResends(iter, pid, empty);
     }
     g_multiTasker->sendEvent(pid, &empty); // this denotes error (does retry lookup using other NS)
     return;
@@ -2990,10 +2987,7 @@ static void handleUDPServerResponse(int fileDesc, FDMultiplexer::funcparam_t& va
   if (!pident->domain.empty()) {
     auto iter = g_multiTasker->d_waiters.find(pident);
     if (iter != g_multiTasker->d_waiters.end()) {
-      if (g_ECSHardening) {
-        iter->key->matchingECSReceived = iter->key->ecsSubnet && checkIncomingECSSource(packet, *iter->key->ecsSubnet);
-      }
-      doResends(iter, pident, packet, iter->key->matchingECSReceived);
+      doResends(iter, pident, packet);
     }
   }
 

diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh
index 6ebeb251cf2c98050e9d4a222dc0abc625158d34..5bc45a292d3c991df19911e8c0aa84fbaa1c9954 100644 (file)
--- a/pdns/recursordist/syncres.hh
+++ b/pdns/recursordist/syncres.hh
@@ -775,7 +775,6 @@ struct PacketID
   bool inIncompleteOkay{false};
   uint16_t id{0}; // wait for a specific id/remote pair
   uint16_t type{0}; // and this is its type
-  std::optional<bool> matchingECSReceived; // only set in ecsHardened mode
   TCPAction highState{TCPAction::DoingRead};
   IOState lowState{IOState::NeedRead};
 
@@ -788,7 +787,7 @@ struct PacketID
 
 inline ostream& operator<<(ostream& ostr, const PacketID& pid)
 {
-  return ostr << "PacketID(id=" << pid.id << ",remote=" << pid.remote.toString() << ",type=" << pid.type << ",tcpsock=" << pid.tcpsock << ",fd=" << pid.fd << ',' << pid.domain << ')';
+  return ostr << "PacketID(id=" << pid.id << ",remote=" << pid.remote.toString() << ",type=" << pid.type << ",tcpsock=" << pid.tcpsock << ",fd=" << pid.fd << ",name=" << pid.domain << ",ecs=" << (pid.ecsSubnet ? pid.ecsSubnet->toString() : "") << ')';
 }
 
 inline ostream& operator<<(ostream& ostr, const shared_ptr<PacketID>& pid)
@@ -820,10 +819,10 @@ struct PacketIDBirthdayCompare
 {
   bool operator()(const std::shared_ptr<PacketID>& lhs, const std::shared_ptr<PacketID>& rhs) const
   {
-    if (std::tie(lhs->remote, lhs->tcpsock, lhs->type, lhs->ecsSubnet) < std::tie(rhs->remote, rhs->tcpsock, rhs->type, rhs->ecsSubnet)) {
+    if (std::tie(lhs->remote, lhs->tcpsock, lhs->type) < std::tie(rhs->remote, rhs->tcpsock, rhs->type)) {
       return true;
     }
-    if (std::tie(lhs->remote, lhs->tcpsock, lhs->type, lhs->ecsSubnet) > std::tie(rhs->remote, rhs->tcpsock, rhs->type, rhs->ecsSubnet)) {
+    if (std::tie(lhs->remote, lhs->tcpsock, lhs->type) > std::tie(rhs->remote, rhs->tcpsock, rhs->type)) {
       return false;
     }
     return lhs->domain < rhs->domain;

diff --git a/regression-tests.recursor-dnssec/recursortests.py b/regression-tests.recursor-dnssec/recursortests.py
index ead4de293fd6e7d1ed22e5ee298a302c275197ac..f9e76cdf434a318b5ab435db2f3fb3ae76621398 100644 (file)
--- a/regression-tests.recursor-dnssec/recursortests.py
+++ b/regression-tests.recursor-dnssec/recursortests.py
@@ -428,9 +428,9 @@ PrivateKey: Ep9uo6+wwjb4MaOmqq7LHav2FLrjotVOeZg8JT1Qk04=
                'zones': ['optout.example']},
         '15': {'threads': 1,
                'zones': ['insecure.optout.example', 'secure.optout.example', 'cname-secure.example']},
-        '16': {'threads': 2,
+        '16': {'threads': 10,
                'zones': ['delay1.example']},
-        '17': {'threads': 2,
+        '17': {'threads': 10,
                'zones': ['delay2.example']},
         '18': {'threads': 1,
                'zones': ['example']}