From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 7 Sep 2021 14:28:07 +0000 (+0200)
Subject: dnsdist: Ignore TCAction over TCP
X-Git-Tag: dnsdist-1.7.0-alpha1~36^2~1
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4a7a0ff8aaf6721f802c57151ff9aa3b4406bda7;p=thirdparty%2Fpdns.git

dnsdist: Ignore TCAction over TCP
---

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index 6f36fcb06a..7fc5aaf8f8 100644
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -803,13 +803,15 @@ bool processRulesResult(const DNSAction::Action& action, DNSQuestion& dq, std::s
     return true;
     break;
   case DNSAction::Action::Truncate:
-    dq.getHeader()->tc = true;
-    dq.getHeader()->qr = true;
-    dq.getHeader()->ra = dq.getHeader()->rd;
-    dq.getHeader()->aa = false;
-    dq.getHeader()->ad = false;
-    ++g_stats.ruleTruncated;
-    return true;
+    if (!dq.overTCP()) {
+      dq.getHeader()->tc = true;
+      dq.getHeader()->qr = true;
+      dq.getHeader()->ra = dq.getHeader()->rd;
+      dq.getHeader()->aa = false;
+      dq.getHeader()->ad = false;
+      ++g_stats.ruleTruncated;
+      return true;
+    }
     break;
   case DNSAction::Action::HeaderModify:
     return true;
diff --git a/pdns/dnsdistdist/docs/rules-actions.rst b/pdns/dnsdistdist/docs/rules-actions.rst
index 3fea22df58..2de3cc0fcd 100644
--- a/pdns/dnsdistdist/docs/rules-actions.rst
+++ b/pdns/dnsdistdist/docs/rules-actions.rst
@@ -47,10 +47,13 @@ If this is not enough, try::
 
 or::
 
-  addAction(MaxQPSIPRule(5), TCAction())
+  addAction(AndRule{MaxQPSIPRule(5), TCPRule(false)}, TCAction())
 
 This will respectively drop traffic exceeding that 5 QPS limit per IP or range, or return it with TC=1, forcing clients to fall back to TCP.
 
+In that last one, note the use of :func:`TCPRule`.
+Without it, clients would get TC=1 even if they correctly fell back to TCP.
+
 To turn this per IP or range limit into a global limit, use ``NotRule(MaxQPSRule(5000))`` instead of :func:`MaxQPSIPRule`.
 
 Regular Expressions
@@ -1541,7 +1544,12 @@ The following actions exist.
 
 .. function:: TCAction()
 
+  .. versionchanged:: 1.7.0
+    This action is now only performed over UDP transports.
+
   Create answer to query with the TC bit set, and the RA bit set to the value of RD in the query, to force the client to TCP.
+  Before 1.7.0 this action was performed even when the query had been received over TCP, which required the use of :func:`TCPRule` to
+  prevent the TC bit from being set over TCP transports.
 
 .. function:: TeeAction(remote[, addECS])
 
diff --git a/regression-tests.dnsdist/test_Advanced.py b/regression-tests.dnsdist/test_Advanced.py
index 27a9f6b0f5..13120564b5 100644
--- a/regression-tests.dnsdist/test_Advanced.py
+++ b/regression-tests.dnsdist/test_Advanced.py
@@ -409,49 +409,6 @@ class TestAdvancedDelay(DNSDistTest):
         self.assertEqual(response, receivedResponse)
         self.assertTrue((end - begin) < timedelta(0, 1))
 
-
-class TestAdvancedTruncateAnyAndTCP(DNSDistTest):
-
-    _config_template = """
-    truncateTC(false)
-    addAction(AndRule({QTypeRule("ANY"), TCPRule(true)}), TCAction())
-    newServer{address="127.0.0.1:%s"}
-    """
-    def testTruncateAnyOverTCP(self):
-        """
-        Advanced: Truncate ANY over TCP
-
-        Send an ANY query to "anytruncatetcp.advanced.tests.powerdns.com.",
-        should be truncated over TCP, not over UDP (yes, it makes no sense,
-        deal with it).
-        """
-        name = 'anytruncatetcp.advanced.tests.powerdns.com.'
-        query = dns.message.make_query(name, 'ANY', 'IN')
-        # dnsdist sets RA = RD for TC responses
-        query.flags &= ~dns.flags.RD
-
-        response = dns.message.make_response(query)
-        rrset = dns.rrset.from_text(name,
-                                    3600,
-                                    dns.rdataclass.IN,
-                                    dns.rdatatype.A,
-                                    '127.0.0.1')
-
-        response.answer.append(rrset)
-
-        (receivedQuery, receivedResponse) = self.sendUDPQuery(query, response)
-        self.assertTrue(receivedQuery)
-        self.assertTrue(receivedResponse)
-        receivedQuery.id = query.id
-        self.assertEqual(query, receivedQuery)
-        self.assertEqual(receivedResponse, response)
-
-        expectedResponse = dns.message.make_response(query)
-        expectedResponse.flags |= dns.flags.TC
-
-        (_, receivedResponse) = self.sendTCPQuery(query, response=None, useQueue=False)
-        self.assertEqual(receivedResponse, expectedResponse)
-
 class TestAdvancedAndNot(DNSDistTest):
 
     _config_template = """
diff --git a/regression-tests.dnsdist/test_Basics.py b/regression-tests.dnsdist/test_Basics.py
index c879566dc9..7ba61ef27b 100644
--- a/regression-tests.dnsdist/test_Basics.py
+++ b/regression-tests.dnsdist/test_Basics.py
@@ -8,7 +8,7 @@ class TestBasics(DNSDistTest):
     _config_template = """
     newServer{address="127.0.0.1:%s"}
     truncateTC(true)
-    addAction(AndRule{QTypeRule(DNSQType.ANY), TCPRule(false)}, TCAction())
+    addAction(QTypeRule(DNSQType.ANY), TCAction())
     addAction(RegexRule("evil[0-9]{4,}\\\\.regex\\\\.tests\\\\.powerdns\\\\.com$"), RCodeAction(DNSRCode.REFUSED))
     mySMN = newSuffixMatchNode()
     mySMN:add(newDNSName("nameAndQtype.tests.powerdns.com."))