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 596c7fcd83430a5389a9936ac63636a6df61c82f..97be48fa5b557d46560f63395057885dd2c2d29e 100644 (file)
--- 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 dd0ff0b119ec7c7b2e6a779324fbb56790f67220..6fd868b1c61a8324374c15294696e371c2f8a4d0 100644 (file)
--- 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 5703db46488d7baa4985c3bca926569d5b8ef614..ddcfbb4154d3af440f3b9d0171749448e262f9c7 100644 (file)
--- 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 4c9d5967244bf8b8c118db49648d938056c2a984..388c917cbe87a78416f32e0da4227b0aef71b1b2 100644 (file)
--- 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 b35182944cf1ea3fb4b86eb53f12497961c5f7ce..788b60686e36b5fcc21d761820e031a72d39ee92 100644 (file)
--- 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 5b082ae4a3292115eb3cd12e8a362516976bc16c..a186d5c8d8c3167fe37c16897bfe6f12cbd219be 100644 (file)
--- 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)