]> git.ipfire.org Git - thirdparty/pdns.git/commitdiff
rec: Fix cache handling of ECS queries with a source length of 0 5515/head
authorRemi Gacogne <remi.gacogne@powerdns.com>
Wed, 12 Jul 2017 08:57:51 +0000 (10:57 +0200)
committerRemi Gacogne <remi.gacogne@powerdns.com>
Tue, 22 Aug 2017 08:29:57 +0000 (10:29 +0200)
The current behavior is to send a query without ECS information
and to cache it as a non ECS-specific entry. Unfortunately doing
so meant that we wouldn't send any further ECS-specific queries for
this (qname,qtype) since we could fallback to the non ECS-specific
cached answer.
This commit adds a new parameter, `ecs-scope-zero-addr`, allowing
to specify the address sent to ECS-whitelisted authoritative servers
in that case. If unset, the default is to take the first usable
(non-any) address in `query-local-address` then in `query-local-address6`,
and finally to use `127.0.0.1` as a last resort.

pdns/iputils.hh
pdns/pdns_recursor.cc
pdns/recursordist/docs/settings.rst
pdns/syncres.cc
pdns/syncres.hh
regression-tests.recursor-dnssec/test_ECS.py
regression-tests.recursor-dnssec/test_Interop.py

index bf6ba4c9ff071447e7cf12de835e648ad4e9bdd0..cb2553c5009a397a28bfcf87d52d03433d85462f 100644 (file)
@@ -435,7 +435,7 @@ public:
     }
     return result;
   }
-  int getBits() const
+  uint8_t getBits() const
   {
     return d_bits;
   }
index 8e84f339d07d59455a0faf54ad98e2452cc68594..17de605879176a5bb78d3e983df1a7503752e5f5 100644 (file)
@@ -2880,6 +2880,33 @@ static int serviceMain(int argc, char*argv[])
   SyncRes::s_ecsipv4limit = ::arg().asNum("ecs-ipv4-bits");
   SyncRes::s_ecsipv6limit = ::arg().asNum("ecs-ipv6-bits");
 
+  if (!::arg().isEmpty("ecs-scope-zero-address")) {
+    ComboAddress scopeZero(::arg()["ecs-scope-zero-address"]);
+    SyncRes::setECSScopeZeroAddress(Netmask(scopeZero, scopeZero.isIPv4() ? 32 : 128));
+  }
+  else {
+    bool found = false;
+    for (const auto& addr : g_localQueryAddresses4) {
+      if (!IsAnyAddress(addr)) {
+        SyncRes::setECSScopeZeroAddress(Netmask(addr, 32));
+        found = true;
+        break;
+      }
+    }
+    if (!found) {
+      for (const auto& addr : g_localQueryAddresses6) {
+        if (!IsAnyAddress(addr)) {
+          SyncRes::setECSScopeZeroAddress(Netmask(addr, 128));
+          found = true;
+          break;
+        }
+      }
+      if (!found) {
+        SyncRes::setECSScopeZeroAddress(Netmask("127.0.0.1/32"));
+      }
+    }
+  }
+
   g_networkTimeoutMsec = ::arg().asNum("network-timeout");
 
   g_initialDomainMap = parseAuthAndForwards();
@@ -3277,6 +3304,7 @@ int main(int argc, char **argv)
     ::arg().set("ecs-ipv4-bits", "Number of bits of IPv4 address to pass for EDNS Client Subnet")="24";
     ::arg().set("ecs-ipv6-bits", "Number of bits of IPv6 address to pass for EDNS Client Subnet")="56";
     ::arg().set("edns-subnet-whitelist", "List of netmasks and domains that we should enable EDNS subnet for")="";
+    ::arg().set("ecs-scope-zero-address", "Address to send to whitelisted authoritative servers for incoming queries with ECS prefix-length source of 0")="";
     ::arg().setSwitch( "use-incoming-edns-subnet", "Pass along received EDNS Client Subnet information")="no";
     ::arg().setSwitch( "pdns-distributes-queries", "If PowerDNS itself should distribute queries over threads")="yes";
     ::arg().setSwitch( "root-nx-trust", "If set, believe that an NXDOMAIN from the root means the TLD does not exist")="yes";
index 08263e544ed56cb368f8c871c17900d5af9675d4..abce97942269fd56fc8935ae73c3c151106afbbe 100644 (file)
@@ -344,6 +344,22 @@ Number of bits of client IPv4 address to pass when sending EDNS Client Subnet ad
 
 Number of bits of client IPv6 address to pass when sending EDNS Client Subnet address information.
 
+.. _setting-ecs-scope-zero-address:
+
+``ecs-scope-zero-address``
+--------------------------
+.. versionadded:: 4.1.0
+
+- IPv4 or IPv6 Address
+- Default: empty
+
+The IP address sent via EDNS Client Subnet to authoritative servers listed in
+`edns-subnet-whitelist`_ when `use-incoming-ecs`_ is set and the query has
+an ECS source prefix-length set to 0.
+The default is to look for the first usable (not an `any` one) address in
+`query-local-address`_ then `query-local-address6`_. If no suitable address is
+found, the recursor fallbacks to sending 127.0.0.1.
+
 .. _setting-edns-outgoing-bufsize:
 
 ``edns-outgoing-bufsize``
index ade576f3f7251697e4450f0fc0475f292ef67706..7a9c781c07286030ac149dda2dbc7cab7ab09226 100644 (file)
@@ -41,6 +41,7 @@ std::unordered_set<DNSName> SyncRes::s_delegationOnly;
 std::unique_ptr<NetmaskGroup> SyncRes::s_dontQuery{nullptr};
 NetmaskGroup SyncRes::s_ednssubnets;
 SuffixMatchNode SyncRes::s_ednsdomains;
+EDNSSubnetOpts SyncRes::s_ecsScopeZero;
 string SyncRes::s_serverID;
 SyncRes::LogMode SyncRes::s_lm;
 
@@ -2415,22 +2416,52 @@ int SyncRes::doResolveAt(NsSet &nameservers, DNSName auth, bool flawedNSSet, con
   return -1;
 }
 
+void SyncRes::setIncomingECS(boost::optional<const EDNSSubnetOpts&> incomingECS)
+{
+  d_incomingECS = incomingECS;
+  if (incomingECS) {
+    if (d_incomingECS->source.getBits() == 0) {
+      /* RFC7871 says we MUST NOT send any ECS if the source scope is 0.
+         But using an empty ECS in that case would mean inserting
+         a non ECS-specific entry into the cache, preventing any further
+         ECS-specific query to be sent.
+         So instead we use the trick described in section 7.1.2:
+         "The subsequent Recursive Resolver query to the Authoritative Nameserver
+         will then either not include an ECS option or MAY optionally include
+         its own address information, which is what the Authoritative
+         Nameserver will almost certainly use to generate any Tailored
+         Response in lieu of an option.  This allows the answer to be handled
+         by the same caching mechanism as other queries, with an explicit
+         indicator of the applicable scope.  Subsequent Stub Resolver queries
+         for /0 can then be answered from this cached response.
+      */
+      d_incomingECS = s_ecsScopeZero;
+      d_incomingECSNetwork = s_ecsScopeZero.source.getMaskedNetwork();
+    }
+    else {
+      uint8_t bits = std::min(incomingECS->source.getBits(), (incomingECS->source.isIpv4() ? s_ecsipv4limit : s_ecsipv6limit));
+      d_incomingECS->source = Netmask(incomingECS->source.getNetwork(), bits);
+      d_incomingECSNetwork = d_incomingECS->source.getMaskedNetwork();
+    }
+  }
+  else {
+    d_incomingECSNetwork = ComboAddress();
+  }
+}
+
 boost::optional<Netmask> SyncRes::getEDNSSubnetMask(const ComboAddress& local, const DNSName&dn, const ComboAddress& rem)
 {
   boost::optional<Netmask> result;
   ComboAddress trunc;
   uint8_t bits;
   if(d_incomingECSFound) {
-    if (d_incomingECS->source.getBits() == 0) {
-      /* RFC7871 says we MUST NOT send any ECS if the source scope is 0 */
-      return result;
-    }
     trunc = d_incomingECSNetwork;
     bits = d_incomingECS->source.getBits();
   }
   else if(!local.isIPv4() || local.sin4.sin_addr.s_addr) { // detect unset 'requestor'
     trunc = local;
     bits = local.isIPv4() ? 32 : 128;
+    bits = std::min(bits, (trunc.isIPv4() ? s_ecsipv4limit : s_ecsipv6limit));
   }
   else {
     /* nothing usable */
@@ -2438,7 +2469,6 @@ boost::optional<Netmask> SyncRes::getEDNSSubnetMask(const ComboAddress& local, c
   }
 
   if(s_ednsdomains.check(dn) || s_ednssubnets.match(rem)) {
-    bits = std::min(bits, (trunc.isIPv4() ? s_ecsipv4limit : s_ecsipv6limit));
     trunc.truncate(bits);
     return boost::optional<Netmask>(Netmask(trunc, bits));
   }
index cbbae96c6e716ccc0ac4c7beec5f58957df976cf..afc52bf8d1caf766a41bfb38524a6d7ad44528d8 100644 (file)
@@ -536,6 +536,11 @@ public:
     return t_sstorage.domainmap;
   }
 
+  static void setECSScopeZeroAddress(const Netmask& scopeZeroMask)
+  {
+    s_ecsScopeZero.source = scopeZeroMask;
+  }
+
   explicit SyncRes(const struct timeval& now);
 
   int beginResolve(const DNSName &qname, const QType &qtype, uint16_t qclass, vector<DNSRecord>&ret);
@@ -625,16 +630,7 @@ public:
     d_skipCNAMECheck = skip;
   }
 
-  void setIncomingECS(boost::optional<const EDNSSubnetOpts&> incomingECS)
-  {
-    d_incomingECS = incomingECS;
-    if (incomingECS) {
-      d_incomingECSNetwork = incomingECS->source.getMaskedNetwork();
-    }
-    else {
-      d_incomingECSNetwork = ComboAddress();
-    }
-  }
+  void setIncomingECS(boost::optional<const EDNSSubnetOpts&> incomingECS);
 
 #ifdef HAVE_PROTOBUF
   void setInitialRequestId(boost::optional<const boost::uuids::uuid&> initialRequestId)
@@ -702,6 +698,7 @@ private:
   static std::unordered_set<DNSName> s_delegationOnly;
   static NetmaskGroup s_ednssubnets;
   static SuffixMatchNode s_ednsdomains;
+  static EDNSSubnetOpts s_ecsScopeZero;
   static LogMode s_lm;
   static std::unique_ptr<NetmaskGroup> s_dontQuery;
 
@@ -773,7 +770,7 @@ private:
   zonesStates_t d_cutStates;
   ostringstream d_trace;
   shared_ptr<RecursorLua4> d_pdl;
-  boost::optional<const EDNSSubnetOpts&> d_incomingECS;
+  boost::optional<EDNSSubnetOpts> d_incomingECS;
   ComboAddress d_incomingECSNetwork;
 #ifdef HAVE_PROTOBUF
   boost::optional<const boost::uuids::uuid&> d_initialRequestId;
index 2e51c7b888b8a413fa5c0193c39f0a2c5c06dfe4..f4d351db650575d89e3f9d798c9e1f7dc4d48ff8 100644 (file)
@@ -12,6 +12,7 @@ from twisted.internet import reactor
 emptyECSText = 'No ECS received'
 nameECS = 'ecs-echo.example.'
 ttlECS = 60
+ecsReactorRunning = False
 
 class ECSTest(RecursorTest):
     _config_template_default = """
@@ -58,22 +59,21 @@ disable-syslog=yes
 
     @classmethod
     def startResponders(cls):
+        global ecsReactorRunning
         print("Launching responders..")
 
         address = cls._PREFIX + '.21'
         port = 53
 
-        if not reactor.running:
+        if not ecsReactorRunning:
             reactor.listenUDP(port, UDPECSResponder(), interface=address)
+            ecsReactorRunning = True
 
+        if not reactor.running:
             cls._UDPResponder = threading.Thread(name='UDP ECS Responder', target=reactor.run, args=(False,))
             cls._UDPResponder.setDaemon(True)
             cls._UDPResponder.start()
 
-    @classmethod
-    def tearDownResponders(cls):
-        reactor.stop()
-
     @classmethod
     def setUpClass(cls):
         cls.setUpSockets()
@@ -110,6 +110,13 @@ forward-zones=ecs-echo.example=%s.21
         query = dns.message.make_query(nameECS, 'TXT')
         self.sendECSQuery(query, expected)
 
+    def testRequireNoECS(self):
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText)
+
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected)
+
 class testIncomingNoECS(ECSTest):
     _confdir = 'IncomingNoECS'
 
@@ -131,6 +138,13 @@ forward-zones=ecs-echo.example=%s.21
         query = dns.message.make_query(nameECS, 'TXT')
         self.sendECSQuery(query, expected)
 
+    def testRequireNoECS(self):
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText)
+
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected)
+
 class testECSByName(ECSTest):
     _confdir = 'ECSByName'
 
@@ -154,6 +168,14 @@ forward-zones=ecs-echo.example=%s.21
         query = dns.message.make_query(nameECS, 'TXT')
         self.sendECSQuery(query, expected)
 
+    def testRequireNoECS(self):
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '127.0.0.0/24')
+
+        # the request for no ECS is ignored because use-incoming-edns-subnet is not set
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected)
+
 class testECSByNameLarger(ECSTest):
     _confdir = 'ECSByNameLarger'
 
@@ -178,6 +200,14 @@ forward-zones=ecs-echo.example=%s.21
         query = dns.message.make_query(nameECS, 'TXT')
         self.sendECSQuery(query, expected)
 
+    def testRequireNoECS(self):
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '127.0.0.1/32')
+
+        # the request for no ECS is ignored because use-incoming-edns-subnet is not set
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected)
+
 class testECSByNameSmaller(ECSTest):
     _confdir = 'ECSByNameLarger'
 
@@ -197,12 +227,21 @@ forward-zones=ecs-echo.example=%s.21
         query = dns.message.make_query(nameECS, 'TXT')
         self.sendECSQuery(query, expected)
 
+    def testRequireNoECS(self):
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '127.0.0.0/16')
+
+        # the request for no ECS is ignored because use-incoming-edns-subnet is not set
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected)
+
 class testIncomingECSByName(ECSTest):
     _confdir = 'ECSIncomingByName'
 
     _config_template = """edns-subnet-whitelist=ecs-echo.example.
 use-incoming-edns-subnet=yes
 forward-zones=ecs-echo.example=%s.21
+ecs-scope-zero-address=2001:db8::42
     """ % (os.environ['PREFIX'])
 
     def testSendECS(self):
@@ -227,6 +266,13 @@ forward-zones=ecs-echo.example=%s.21
         query = dns.message.make_query(nameECS, 'TXT')
         self.sendECSQuery(query, expected, ttlECS)
 
+    def testRequireNoECS(self):
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', "2001:db8::42/128")
+
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected, ttlECS)
+
 class testIncomingECSByNameLarger(ECSTest):
     _confdir = 'ECSIncomingByNameLarger'
 
@@ -234,6 +280,7 @@ class testIncomingECSByNameLarger(ECSTest):
 use-incoming-edns-subnet=yes
 ecs-ipv4-bits=32
 forward-zones=ecs-echo.example=%s.21
+ecs-scope-zero-address=192.168.0.1
     """ % (os.environ['PREFIX'])
 
     def testSendECS(self):
@@ -249,6 +296,13 @@ forward-zones=ecs-echo.example=%s.21
         query = dns.message.make_query(nameECS, 'TXT')
         self.sendECSQuery(query, expected, ttlECS)
 
+    def testRequireNoECS(self):
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '192.168.0.1/32')
+
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected, ttlECS)
+
 class testIncomingECSByNameSmaller(ECSTest):
     _confdir = 'ECSIncomingByNameSmaller'
 
@@ -256,6 +310,7 @@ class testIncomingECSByNameSmaller(ECSTest):
 use-incoming-edns-subnet=yes
 ecs-ipv4-bits=16
 forward-zones=ecs-echo.example=%s.21
+ecs-scope-zero-address=192.168.0.1
     """ % (os.environ['PREFIX'])
 
     def testSendECS(self):
@@ -269,6 +324,13 @@ forward-zones=ecs-echo.example=%s.21
         query = dns.message.make_query(nameECS, 'TXT')
         self.sendECSQuery(query, expected)
 
+    def testRequireNoECS(self):
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '192.168.0.1/32')
+
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected, ttlECS)
+
 class testIncomingECSByNameV6(ECSTest):
     _confdir = 'ECSIncomingByNameV6'
 
@@ -276,6 +338,7 @@ class testIncomingECSByNameV6(ECSTest):
 use-incoming-edns-subnet=yes
 ecs-ipv6-bits=128
 forward-zones=ecs-echo.example=%s.21
+query-local-address6=::1
     """ % (os.environ['PREFIX'])
 
     def testSendECS(self):
@@ -291,6 +354,15 @@ forward-zones=ecs-echo.example=%s.21
         res = self.sendUDPQuery(query)
         self.sendECSQuery(query, expected, ttlECS)
 
+    def testRequireNoECS(self):
+        # we should get ::1/128 because neither ecs-scope-zero-addr nor query-local-address are set,
+        # but query-local-address6 is set to ::1
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', "::1/128")
+
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected, ttlECS)
+
 class testECSNameMismatch(ECSTest):
     _confdir = 'ECSNameMismatch'
 
@@ -309,6 +381,13 @@ forward-zones=ecs-echo.example=%s.21
         query = dns.message.make_query(nameECS, 'TXT')
         self.sendECSQuery(query, expected)
 
+    def testRequireNoECS(self):
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText)
+
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected)
+
 class testECSByIP(ECSTest):
     _confdir = 'ECSByIP'
 
@@ -327,12 +406,21 @@ forward-zones=ecs-echo.example=%s.21
         query = dns.message.make_query(nameECS, 'TXT')
         self.sendECSQuery(query, expected)
 
+    def testRequireNoECS(self):
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '127.0.0.0/24')
+
+        # the request for no ECS is ignored because use-incoming-edns-subnet is not set
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected)
+
 class testIncomingECSByIP(ECSTest):
     _confdir = 'ECSIncomingByIP'
 
     _config_template = """edns-subnet-whitelist=%s.21
 use-incoming-edns-subnet=yes
 forward-zones=ecs-echo.example=%s.21
+ecs-scope-zero-address=::1
     """ % (os.environ['PREFIX'], os.environ['PREFIX'])
 
     def testSendECS(self):
@@ -347,6 +435,14 @@ forward-zones=ecs-echo.example=%s.21
         query = dns.message.make_query(nameECS, 'TXT')
         self.sendECSQuery(query, expected)
 
+    def testRequireNoECS(self):
+        # we will get ::1 because ecs-scope-zero-addr is set to ::1
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '::1/128')
+
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected, ttlECS)
+
 class testECSIPMismatch(ECSTest):
     _confdir = 'ECSIPMismatch'
 
@@ -366,6 +462,13 @@ forward-zones=ecs-echo.example=%s.21
         res = self.sendUDPQuery(query)
         self.sendECSQuery(query, expected)
 
+    def testRequireNoECS(self):
+        expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText)
+
+        ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0)
+        query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512)
+        self.sendECSQuery(query, expected)
+
 class UDPECSResponder(DatagramProtocol):
     @staticmethod
     def ipToStr(option):
index 74a51e6fc06b78ab51fe6b5fbbfaca34d80bdfb9..4e4c3eb88585c4f59bfda6db518865e0349ebf7c 100644 (file)
@@ -130,9 +130,10 @@ forward-zones+=undelegated.insecure.example=%s.12
 
         reactor.listenUDP(port, UDPResponder(), interface=address)
 
-        cls._UDPResponder = threading.Thread(name='UDP Responder', target=reactor.run, args=(False,))
-        cls._UDPResponder.setDaemon(True)
-        cls._UDPResponder.start()
+        if not reactor.running:
+            cls._UDPResponder = threading.Thread(name='UDP Responder', target=reactor.run, args=(False,))
+            cls._UDPResponder.setDaemon(True)
+            cls._UDPResponder.start()
 
     @classmethod
     def tearDownResponders(cls):