From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Fri, 6 Jun 2025 14:36:44 +0000 (+0200)
Subject: dnsdist: Clean up existing records when turning query into response
X-Git-Tag: dnsdist-2.0.0-beta1~21^2~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a48bd8a211491dae2450a94067b1f32d560ce4a8;p=thirdparty%2Fpdns.git

dnsdist: Clean up existing records when turning query into response

Signed-off-by: Remi Gacogne <remi.gacogne@powerdns.com>
---

diff --git a/pdns/dnsdistdist/dnsdist-actions-factory.cc b/pdns/dnsdistdist/dnsdist-actions-factory.cc
index 596c7fcd83..97be48fa5b 100644
--- a/pdns/dnsdistdist/dnsdist-actions-factory.cc
+++ b/pdns/dnsdistdist/dnsdist-actions-factory.cc
@@ -387,9 +387,8 @@ public:
   DNSAction::Action operator()(DNSQuestion* dnsquestion, std::string* ruleresult) const override
   {
     (void)ruleresult;
+    dnsdist::self_answers::removeRecordsAndSetRCode(*dnsquestion, d_rcode);
     dnsdist::PacketMangling::editDNSHeaderFromPacket(dnsquestion->getMutableData(), [this](dnsheader& header) {
-      header.rcode = d_rcode;
-      header.qr = true; // for good measure
       setResponseHeadersFromConfig(header, d_responseConfig);
       return true;
     });
@@ -415,9 +414,8 @@ public:
   DNSAction::Action operator()(DNSQuestion* dnsquestion, std::string* ruleresult) const override
   {
     (void)ruleresult;
+    dnsdist::self_answers::removeRecordsAndSetRCode(*dnsquestion, (d_rcode & 0xF));
     dnsdist::PacketMangling::editDNSHeaderFromPacket(dnsquestion->getMutableData(), [this](dnsheader& header) {
-      header.rcode = (d_rcode & 0xF);
-      header.qr = true; // for good measure
       setResponseHeadersFromConfig(header, d_responseConfig);
       return true;
     });
diff --git a/pdns/dnsdistdist/dnsdist-self-answers.cc b/pdns/dnsdistdist/dnsdist-self-answers.cc
index dd0ff0b119..6fd868b1c6 100644
--- a/pdns/dnsdistdist/dnsdist-self-answers.cc
+++ b/pdns/dnsdistdist/dnsdist-self-answers.cc
@@ -240,4 +240,32 @@ bool generateAnswerFromRawPacket(DNSQuestion& dnsQuestion, const PacketBuffer& p
   return true;
 }
 
+bool removeRecordsAndSetRCode(DNSQuestion& dnsQuestion, uint16_t rcode)
+{
+  bool dnssecOK = false;
+  bool hadEDNS = false;
+  if (dnsdist::configuration::getCurrentRuntimeConfiguration().d_addEDNSToSelfGeneratedResponses && queryHasEDNS(dnsQuestion)) {
+    hadEDNS = true;
+    dnssecOK = ((dnsdist::getEDNSZ(dnsQuestion) & EDNS_HEADER_FLAG_DO) != 0);
+  }
+
+  dnsdist::PacketMangling::editDNSHeaderFromPacket(dnsQuestion.getMutableData(), [rcode](dnsheader& header) {
+    header.rcode = rcode;
+    header.qr = true;
+    header.qdcount = htons(1);
+    header.arcount = 0;
+    header.nscount = 0;
+    header.ancount = 0;
+    return true;
+  });
+  auto qnameWireLength = dnsQuestion.ids.qname.wirelength();
+  dnsQuestion.getMutableData().resize(sizeof(dnsheader) + qnameWireLength + 4);
+
+  if (hadEDNS) {
+    addEDNS(dnsQuestion.getMutableData(), dnsQuestion.getMaximumSize(), dnssecOK, dnsdist::configuration::getCurrentRuntimeConfiguration().d_payloadSizeSelfGenAnswers, 0);
+  }
+
+  return true;
+}
+
 }
diff --git a/pdns/dnsdistdist/dnsdist-self-answers.hh b/pdns/dnsdistdist/dnsdist-self-answers.hh
index 5703db4648..ddcfbb4154 100644
--- a/pdns/dnsdistdist/dnsdist-self-answers.hh
+++ b/pdns/dnsdistdist/dnsdist-self-answers.hh
@@ -29,4 +29,5 @@ bool generateAnswerFromCNAME(DNSQuestion& dnsQuestion, const DNSName& cname, con
 bool generateAnswerFromIPAddresses(DNSQuestion& dnsQuestion, const std::vector<ComboAddress>& addresses, const ResponseConfig& responseConfig);
 bool generateAnswerFromRDataEntries(DNSQuestion& dnsQuestion, const std::vector<std::string>& entries, std::optional<uint16_t> typeForAny, const ResponseConfig& responseConfig);
 bool generateAnswerFromRawPacket(DNSQuestion& dnsQuestion, const PacketBuffer& packet);
+bool removeRecordsAndSetRCode(DNSQuestion& dnsQuestion, uint16_t rcode);
 }
diff --git a/pdns/dnsdistdist/dnsdist.cc b/pdns/dnsdistdist/dnsdist.cc
index 4c9d596724..388c917cbe 100644
--- a/pdns/dnsdistdist/dnsdist.cc
+++ b/pdns/dnsdistdist/dnsdist.cc
@@ -890,11 +890,7 @@ bool processRulesResult(const DNSAction::Action& action, DNSQuestion& dnsQuestio
   }
 
   auto setRCode = [&dnsQuestion](uint8_t rcode) {
-    dnsdist::PacketMangling::editDNSHeaderFromPacket(dnsQuestion.getMutableData(), [rcode](dnsheader& header) {
-      header.rcode = rcode;
-      header.qr = true;
-      return true;
-    });
+    dnsdist::self_answers::removeRecordsAndSetRCode(dnsQuestion, rcode);
   };
 
   switch (action) {
@@ -1026,11 +1022,7 @@ static bool applyRulesToQuery(DNSQuestion& dnsQuestion, const timespec& now)
 #ifndef DISABLE_DYNBLOCKS
   const auto defaultDynBlockAction = dnsdist::configuration::getCurrentRuntimeConfiguration().d_dynBlockAction;
   auto setRCode = [&dnsQuestion](uint8_t rcode) {
-    dnsdist::PacketMangling::editDNSHeaderFromPacket(dnsQuestion.getMutableData(), [rcode](dnsheader& header) {
-      header.rcode = rcode;
-      header.qr = true;
-      return true;
-    });
+    dnsdist::self_answers::removeRecordsAndSetRCode(dnsQuestion, rcode);
   };
 
   /* the Dynamic Block mechanism supports address and port ranges, so we need to pass the full address and port */
@@ -1535,11 +1527,7 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dnsQuestion, std::shared_
 
       vinfolog("%s query for %s|%s from %s, no downstream server available", servFailOnNoPolicy ? "ServFailed" : "Dropped", dnsQuestion.ids.qname.toLogString(), QType(dnsQuestion.ids.qtype).toString(), dnsQuestion.ids.origRemote.toStringWithPort());
       if (servFailOnNoPolicy) {
-        dnsdist::PacketMangling::editDNSHeaderFromPacket(dnsQuestion.getMutableData(), [](dnsheader& header) {
-          header.rcode = RCode::ServFail;
-          header.qr = true;
-          return true;
-        });
+        dnsdist::self_answers::removeRecordsAndSetRCode(dnsQuestion, RCode::ServFail);
 
         fixUpQueryTurnedResponse(dnsQuestion, dnsQuestion.ids.origFlags);
 
@@ -1691,11 +1679,7 @@ ProcessQueryResult processQuery(DNSQuestion& dnsQuestion, std::shared_ptr<Downst
     gettime(&now);
 
     if ((dnsQuestion.ids.qtype == QType::AXFR || dnsQuestion.ids.qtype == QType::IXFR) && (dnsQuestion.getProtocol() == dnsdist::Protocol::DoH || dnsQuestion.getProtocol() == dnsdist::Protocol::DoQ || dnsQuestion.getProtocol() == dnsdist::Protocol::DoH3)) {
-      dnsQuestion.editHeader([](dnsheader& header) {
-        header.rcode = RCode::NotImp;
-        header.qr = true;
-        return true;
-      });
+      dnsdist::self_answers::removeRecordsAndSetRCode(dnsQuestion, RCode::NotImp);
       return processQueryAfterRules(dnsQuestion, selectedBackend);
     }
 
diff --git a/pdns/dnsdistdist/docs/upgrade_guide.rst b/pdns/dnsdistdist/docs/upgrade_guide.rst
index b35182944c..788b60686e 100644
--- a/pdns/dnsdistdist/docs/upgrade_guide.rst
+++ b/pdns/dnsdistdist/docs/upgrade_guide.rst
@@ -17,6 +17,15 @@ XPF support has been removed.
 
 The ``options`` parameter of :func:`HTTPStatusAction` has been deprecated because it had unexpected side-effects, and should thus no longer be used.
 
+In some cases, :program:`dnsdist` turns an incoming into a response, setting the response code in the process. When doing so, it was not properly cleaning up records present in the answer, authority or additional sections, which could have been surprising to clients and wasted bandwidth. This has now been fixed. The cases in question are:
+
+* :func:`RCodeAction`
+* :func:`ERCodeAction`
+* returning ``DNSAction.Nxdomain``, ``DNSAction.Refused`` or ``DNSAction.ServFail`` from ``Lua``
+* using the ``DNSAction.Nxdomain``, ``DNSAction.Refused`` or ``DNSAction.ServFail`` dynamic block actions
+* sending ``Server Failure`` when no downstream servers are usable
+* receiving a zone transfer request over DoQ, DoH or DoH3
+
 1.8.x to 1.9.0
 --------------
 
diff --git a/regression-tests.dnsdist/test_RecordsCount.py b/regression-tests.dnsdist/test_RecordsCount.py
index 5b082ae4a3..a186d5c8d8 100644
--- a/regression-tests.dnsdist/test_RecordsCount.py
+++ b/regression-tests.dnsdist/test_RecordsCount.py
@@ -71,12 +71,6 @@ class TestRecordsCountOnlyOneAR(DNSDistTest):
                                                     '127.0.0.1'))
         expectedResponse = dns.message.make_response(query)
         expectedResponse.set_rcode(dns.rcode.REFUSED)
-        # this is not great, we should fix that!
-        expectedResponse.additional.append(dns.rrset.from_text(name,
-                                                               3600,
-                                                               dns.rdataclass.IN,
-                                                               dns.rdatatype.A,
-                                                               '127.0.0.1'))
 
         for method in ("sendUDPQuery", "sendTCPQuery"):
             sender = getattr(self, method)
@@ -155,7 +149,6 @@ class TestRecordsCountMoreThanOneLessThanFour(DNSDistTest):
 
         expectedResponse = dns.message.make_response(query)
         expectedResponse.set_rcode(dns.rcode.REFUSED)
-        expectedResponse.answer.append(rrset)
 
         for method in ("sendUDPQuery", "sendTCPQuery"):
             sender = getattr(self, method)
@@ -188,7 +181,6 @@ class TestRecordsCountNothingInNS(DNSDistTest):
         query.flags &= ~dns.flags.RD
         expectedResponse = dns.message.make_response(query)
         expectedResponse.set_rcode(dns.rcode.REFUSED)
-        expectedResponse.authority.append(rrset)
 
         for method in ("sendUDPQuery", "sendTCPQuery"):
             sender = getattr(self, method)