rec: Fix the padding size computation, remove outdated comment

diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index f219de1672fecaf48d087cc877f1dba61ea8aedb..6e516e76ad2572d3b74152e7e3931e1af1bdae39 100644 (file)
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -1489,8 +1489,8 @@ static void startDoResolve(void *p)
     std::vector<pair<uint16_t, string> > ednsOpts;
     bool variableAnswer = dc->d_variable;
     bool haveEDNS=false;
-    bool paddingAllowed = g_paddingFrom.match(dc->d_remote);
-    bool padResponse = paddingAllowed ? (g_paddingMode == PaddingMode::Always) : false;
+    bool paddingAllowed = false;
+    bool padResponse = false;
 #ifdef NOD_ENABLED
     bool hasUDR = false;
 #endif /* NOD_ENABLED */
@@ -1511,6 +1511,13 @@ static void startDoResolve(void *p)
       ednsOpts = edo.d_options;
       maxanswersize -= 11; // EDNS header size
 
+      if (g_paddingFrom.match(dc->d_remote)) {
+        paddingAllowed = true;
+        if (g_paddingMode == PaddingMode::Always) {
+          padResponse = true;
+        }
+      }
+
       for (const auto& o : edo.d_options) {
         if (o.first == EDNSOptionCode::ECS && g_useIncomingECS && !dc->d_ecsParsed) {
           dc->d_ecsFound = getEDNSSubnetOptsFromString(o.second, &dc->d_ednssubnet);
@@ -1523,15 +1530,7 @@ static void startDoResolve(void *p)
             maxanswersize -= EDNSOptionCodeSize + EDNSOptionLengthSize + mode_server_id.size();
           }
         } else if (paddingAllowed && padResponse == false && g_paddingMode == PaddingMode::PaddedQueries && o.first == EDNSOptionCode::PADDING) {
-          /* we should only pad on 'secure' cases to limit amplification:
-             - over TCP ;
-             - from 'known-good' sources (dnsdist for example).
-             We should also support a strict mode (only pad if the query has a padding option)
-             and a relaxed mode (pad if the client supports EDNS).
-             The block-size should be configurable as well.
-          */
           padResponse = true;
-          maxanswersize -= 4;
         }
       }
     }
@@ -1990,10 +1989,14 @@ static void startDoResolve(void *p)
       }
     }
 
-    if (padResponse) {
+    if (haveEDNS && padResponse) {
       size_t currentSize = pw.getSizeWithOpts(returnedEdnsOptions);
-      if (currentSize < (static_cast<size_t>(maxanswersize) - 4)) {
-        size_t remaining = static_cast<size_t>(maxanswersize) - currentSize;
+      /* we don't use maxawnswersize because it accounts for some EDNS options, but
+         not all of them (for example ECS) */
+      size_t maxSize = min(static_cast<uint16_t>(edo.d_packetsize >= 512 ? edo.d_packetsize : 512), g_udpTruncationThreshold);
+
+      if (currentSize < (maxSize - 4)) {
+        size_t remaining = maxSize - (currentSize + 4);
         /* from rfc8647, "4.1.  Recommended Strategy: Block-Length Padding":
            If a server receives a query that includes the EDNS(0) "Padding"
            option, it MUST pad the corresponding response (see Section 4 of

diff --git a/regression-tests.recursor-dnssec/test_EDNSPadding.py b/regression-tests.recursor-dnssec/test_EDNSPadding.py
index f2a40c4de7e9fcf0867cfd11af01bc0c40178208..d00f2cb49e48e693f9139f29738f9ed664790bb8 100644 (file)
--- a/regression-tests.recursor-dnssec/test_EDNSPadding.py
+++ b/regression-tests.recursor-dnssec/test_EDNSPadding.py
@@ -6,7 +6,7 @@ import paddingoption
 
 from recursortests import RecursorTest
 
-class TestRecursorEDNSPadding(RecursorTest):
+class RecursorEDNSPaddingTest(RecursorTest):
 
     @classmethod
     def setUpClass(cls):
@@ -42,6 +42,9 @@ class TestRecursorEDNSPadding(RecursorTest):
       self.assertEqual(message.edns, 0)
       self.assertEquals(len(message.options), 0)
 
+    def checkNoEDNS(self, message):
+      self.assertEqual(message.edns, -1)
+
     def sendUDPQueryOverIPv6(self, query, timeout=2.0):
       sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
       sock.settimeout(2.0)
@@ -64,7 +67,16 @@ class TestRecursorEDNSPadding(RecursorTest):
         message = dns.message.from_wire(data)
       return message
 
-class PaddingDefaultTest(TestRecursorEDNSPadding):
+    def testQueryWithoutEDNS(self):
+        name = 'secure.example.'
+        expected = dns.rrset.from_text(name, 0, dns.rdataclass.IN, 'A', '192.0.2.17')
+        query = dns.message.make_query(name, 'A', want_dnssec=False)
+        query.flags |= dns.flags.CD
+        res = self.sendUDPQuery(query)
+        self.checkNoEDNS(res)
+        self.assertRRsetInAnswer(res, expected)
+
+class PaddingDefaultTest(RecursorEDNSPaddingTest):
 
     _confdir = 'PaddingDefault'
 
@@ -78,7 +90,7 @@ class PaddingDefaultTest(TestRecursorEDNSPadding):
         self.checkNoPadding(res)
         self.assertRRsetInAnswer(res, expected)
 
-    def testqueryWithoutPadding(self):
+    def testQueryWithoutPadding(self):
         name = 'secure.example.'
         expected = dns.rrset.from_text(name, 0, dns.rdataclass.IN, 'A', '192.0.2.17')
         query = dns.message.make_query(name, 'A', want_dnssec=True)
@@ -87,7 +99,7 @@ class PaddingDefaultTest(TestRecursorEDNSPadding):
         self.checkNoPadding(res)
         self.assertRRsetInAnswer(res, expected)
 
-class PaddingAllowedAlwaysTest(TestRecursorEDNSPadding):
+class PaddingAllowedAlwaysTest(RecursorEDNSPaddingTest):
 
     _confdir = 'PaddingAlways'
     _config_template = """edns-padding-from=127.0.0.1
@@ -105,7 +117,7 @@ edns-padding-tag=7830
         self.checkPadding(res)
         self.assertRRsetInAnswer(res, expected)
 
-    def testqueryWithoutPadding(self):
+    def testQueryWithoutPadding(self):
         name = 'secure.example.'
         expected = dns.rrset.from_text(name, 0, dns.rdataclass.IN, 'A', '192.0.2.17')
         query = dns.message.make_query(name, 'A', want_dnssec=True)
@@ -114,7 +126,7 @@ edns-padding-tag=7830
         self.checkPadding(res)
         self.assertRRsetInAnswer(res, expected)
 
-class PaddingAllowedWhenPaddedTest(TestRecursorEDNSPadding):
+class PaddingAllowedWhenPaddedTest(RecursorEDNSPaddingTest):
 
     _confdir = 'PaddingWhenPadded'
     _config_template = """edns-padding-from=127.0.0.1
@@ -132,7 +144,7 @@ edns-padding-tag=7830
         self.checkPadding(res)
         self.assertRRsetInAnswer(res, expected)
 
-    def testqueryWithoutPadding(self):
+    def testQueryWithoutPadding(self):
         name = 'secure.example.'
         expected = dns.rrset.from_text(name, 0, dns.rdataclass.IN, 'A', '192.0.2.17')
         query = dns.message.make_query(name, 'A', want_dnssec=True)
@@ -141,7 +153,7 @@ edns-padding-tag=7830
         self.checkNoPadding(res)
         self.assertRRsetInAnswer(res, expected)
 
-class PaddingAllowedAlwaysSameTagTest(TestRecursorEDNSPadding):
+class PaddingAllowedAlwaysSameTagTest(RecursorEDNSPaddingTest):
 
     # we use the default tag (0) for padded responses, which will cause
     # the same packet cache entry (with padding ) to be returned to a client
@@ -167,7 +179,7 @@ local-address=127.0.0.1, ::1
         self.checkPadding(res)
         self.assertRRsetInAnswer(res, expected)
 
-    def testqueryWithoutPadding(self):
+    def testQueryWithoutPadding(self):
         name = 'secure.example.'
         expected = dns.rrset.from_text(name, 0, dns.rdataclass.IN, 'A', '192.0.2.17')
         query = dns.message.make_query(name, 'A', want_dnssec=True)