From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Thu, 9 Nov 2017 15:31:11 +0000 (+0100)
Subject: rec: Allow the use of a 'self-resolving' NS if cached A/AAAA exists
X-Git-Tag: auth-4.1.0-rc3~8^2
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b4c8789abb4f64c16532548bc93f7981430f63dc;p=pdns

rec: Allow the use of a 'self-resolving' NS if cached A/AAAA exists

We just have to take care not to try to contact that NS to learn
its own IP addresses, because that does not make sense.
Before this, we could skip a perfectly valid NS for which we had
retrieved the A and/or AAAA entries, for example via a glue.
Also get rid of a flawed calculation based on whether IPv6 was
enabled whereas we were only dealing with NS at this point.
---

diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index 6584dfb94..067aead5f 100644
--- a/pdns/recursordist/test-syncres_cc.cc
+++ b/pdns/recursordist/test-syncres_cc.cc
@@ -101,12 +101,17 @@ LuaConfigItems::LuaConfigItems()
 
 static void init(bool debug=false)
 {
+  L.setName("test");
+  L.disableSyslog(true);
+
   if (debug) {
-    L.setName("test");
     L.setLoglevel((Logger::Urgency)(6)); // info and up
-    L.disableSyslog(true);
     L.toConsole(Logger::Info);
   }
+  else {
+    L.setLoglevel(Logger::None);
+    L.toConsole(Logger::Error);
+  }
 
   seedRandom("/dev/urandom");
   reportAllTypes();
@@ -775,7 +780,7 @@ BOOST_AUTO_TEST_CASE(test_all_nss_network_error) {
       }
       else {
         downServers.insert(ip);
-        return -1;
+        return 0;
       }
     });
 
@@ -791,10 +796,64 @@ BOOST_AUTO_TEST_CASE(test_all_nss_network_error) {
   for (const auto& server : downServers) {
     BOOST_CHECK_EQUAL(SyncRes::getServerFailsCount(server), 1);
     BOOST_CHECK(SyncRes::isThrottled(time(nullptr), server, target, QType::A));
-;
   }
 }
 
+BOOST_AUTO_TEST_CASE(test_only_one_ns_up_resolving_itself_with_glue) {
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr);
+
+  primeHints();
+
+  DNSName target("www.powerdns.com.");
+
+  sr->setAsyncCallback([target](const ComboAddress& ip, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional<Netmask>& srcmask, boost::optional<const ResolveContext&> context, std::shared_ptr<RemoteLogger> outgoingLogger, LWResult* res) {
+
+      if (isRootServer(ip)) {
+        setLWResult(res, 0, false, false, true);
+        if (domain == target) {
+          addRecordToLW(res, "powerdns.com.", QType::NS, "pdns-public-ns1.powerdns.com.", DNSResourceRecord::AUTHORITY, 172800);
+          addRecordToLW(res, "powerdns.com.", QType::NS, "pdns-public-ns2.powerdns.net.", DNSResourceRecord::AUTHORITY, 172800);
+          addRecordToLW(res, "pdns-public-ns1.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, 172800);
+          addRecordToLW(res, "pdns-public-ns1.powerdns.com.", QType::AAAA, "2001:DB8::2", DNSResourceRecord::ADDITIONAL, 172800);
+        }
+        else if (domain == DNSName("pdns-public-ns2.powerdns.net.")) {
+          addRecordToLW(res, "powerdns.net.", QType::NS, "pdns-public-ns2.powerdns.net.", DNSResourceRecord::AUTHORITY, 172800);
+          addRecordToLW(res, "powerdns.net.", QType::NS, "pdns-public-ns1.powerdns.com.", DNSResourceRecord::AUTHORITY, 172800);
+          addRecordToLW(res, "pdns-public-ns2.powerdns.net.", QType::A, "192.0.2.3", DNSResourceRecord::ADDITIONAL, 172800);
+          addRecordToLW(res, "pdns-public-ns2.powerdns.net.", QType::AAAA, "2001:DB8::3", DNSResourceRecord::ADDITIONAL, 172800);
+        }
+        return 1;
+      }
+      else if (ip == ComboAddress("192.0.2.3:53")) {
+        setLWResult(res, 0, true, false, true);
+        if (domain == DNSName("pdns-public-ns2.powerdns.net.")) {
+          if (type == QType::A) {
+            addRecordToLW(res, "pdns-public-ns2.powerdns.net.", QType::A, "192.0.2.3");
+          }
+          else if (type == QType::AAAA) {
+            addRecordToLW(res, "pdns-public-ns2.powerdns.net.", QType::AAAA, "2001:DB8::3");
+          }
+        }
+        else if (domain == target) {
+          if (type == QType::A) {
+            addRecordToLW(res, domain, QType::A, "192.0.2.1");
+          }
+          else if (type == QType::AAAA) {
+            addRecordToLW(res, domain, QType::AAAA, "2001:DB8::1");
+          }
+        }
+        return 1;
+      }
+      return 0;
+    });
+
+  vector<DNSRecord> ret;
+  int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(ret.size(), 1);
+}
+
 BOOST_AUTO_TEST_CASE(test_os_limit_errors) {
   std::unique_ptr<SyncRes> sr;
   initSR(sr);
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 474ee41cf..32cae7ae1 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -624,7 +624,7 @@ struct speedOrderCA
 
 /** This function explicitly goes out for A or AAAA addresses
 */
-vector<ComboAddress> SyncRes::getAddrs(const DNSName &qname, unsigned int depth, set<GetBestNSAnswer>& beenthere)
+vector<ComboAddress> SyncRes::getAddrs(const DNSName &qname, unsigned int depth, set<GetBestNSAnswer>& beenthere, bool cacheOnly)
 {
   typedef vector<DNSRecord> res_t;
   res_t res;
@@ -633,10 +633,12 @@ vector<ComboAddress> SyncRes::getAddrs(const DNSName &qname, unsigned int depth,
   ret_t ret;
 
   QType type;
+  bool oldCacheOnly = d_cacheonly;
   bool oldRequireAuthData = d_requireAuthData;
   bool oldValidationRequested = d_DNSSECValidationRequested;
   d_requireAuthData = false;
   d_DNSSECValidationRequested = false;
+  d_cacheonly = cacheOnly;
 
   for(int j=1; j<2+s_doIPv6; j++)
   {
@@ -685,6 +687,7 @@ vector<ComboAddress> SyncRes::getAddrs(const DNSName &qname, unsigned int depth,
 
   d_requireAuthData = oldRequireAuthData;
   d_DNSSECValidationRequested = oldValidationRequested;
+  d_cacheonly = oldCacheOnly;
 
   /* we need to remove from the nsSpeeds collection the existing IPs
      for this nameserver that are no longer in the set, even if there
@@ -1388,13 +1391,13 @@ bool SyncRes::nameserverIPBlockedByRPZ(const DNSFilterEngine& dfe, const ComboAd
   return false;
 }
 
-vector<ComboAddress> SyncRes::retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector<DNSName >::const_iterator& tns, const unsigned int depth, set<GetBestNSAnswer>& beenthere, const vector<DNSName >& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet)
+vector<ComboAddress> SyncRes::retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector<DNSName >::const_iterator& tns, const unsigned int depth, set<GetBestNSAnswer>& beenthere, const vector<DNSName >& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly)
 {
   vector<ComboAddress> result;
 
   if(!tns->empty()) {
     LOG(prefix<<qname<<": Trying to resolve NS '"<<*tns<< "' ("<<1+tns-rnameservers.begin()<<"/"<<(unsigned int)rnameservers.size()<<")"<<endl);
-    result = getAddrs(*tns, depth+2, beenthere);
+    result = getAddrs(*tns, depth+2, beenthere, cacheOnly);
     pierceDontQuery=false;
   }
   else {
@@ -2580,10 +2583,13 @@ int SyncRes::doResolveAt(NsSet &nameservers, DNSName auth, bool flawedNSSet, con
         return -1;
       }
 
-      // this line needs to identify the 'self-resolving' behaviour, but we get it wrong now
-      if(qname == *tns && qtype.getCode()==QType::A && rnameservers.size() > (size_t)(1+1*s_doIPv6)) {
-        LOG(prefix<<qname<<": Not using NS to resolve itself! ("<<(1+tns-rnameservers.cbegin())<<"/"<<rnameservers.size()<<")"<<endl);
-        continue;
+      bool cacheOnly = false;
+      // this line needs to identify the 'self-resolving' behaviour
+      if(qname == *tns && (qtype.getCode() == QType::A || qtype.getCode() == QType::AAAA)) {
+        /* we might have a glue entry in cache so let's try this NS
+           but only if we have enough in the cache to know how to reach it */
+        LOG(prefix<<qname<<": Using NS to resolve itself, but only using what we have in cache ("<<(1+tns-rnameservers.cbegin())<<"/"<<rnameservers.size()<<")"<<endl);
+        cacheOnly = true;
       }
 
       typedef vector<ComboAddress> remoteIPs_t;
@@ -2615,7 +2621,7 @@ int SyncRes::doResolveAt(NsSet &nameservers, DNSName auth, bool flawedNSSet, con
       }
       else {
         /* if tns is empty, retrieveAddressesForNS() knows we have hardcoded servers (i.e. "forwards") */
-        remoteIPs = retrieveAddressesForNS(prefix, qname, tns, depth, beenthere, rnameservers, nameservers, sendRDQuery, pierceDontQuery, flawedNSSet);
+        remoteIPs = retrieveAddressesForNS(prefix, qname, tns, depth, beenthere, rnameservers, nameservers, sendRDQuery, pierceDontQuery, flawedNSSet, cacheOnly);
 
         if(remoteIPs.empty()) {
           LOG(prefix<<qname<<": Failed to get IP for NS "<<*tns<<", trying next if available"<<endl);
diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index d93138043..43523e293 100644
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -735,13 +735,13 @@ private:
 
   inline vector<DNSName> shuffleInSpeedOrder(NsSet &nameservers, const string &prefix);
   bool moreSpecificThan(const DNSName& a, const DNSName &b) const;
-  vector<ComboAddress> getAddrs(const DNSName &qname, unsigned int depth, set<GetBestNSAnswer>& beenthere);
+  vector<ComboAddress> getAddrs(const DNSName &qname, unsigned int depth, set<GetBestNSAnswer>& beenthere, bool cacheOnly);
 
   bool nameserversBlockedByRPZ(const DNSFilterEngine& dfe, const NsSet& nameservers);
   bool nameserverIPBlockedByRPZ(const DNSFilterEngine& dfe, const ComboAddress&);
   bool throttledOrBlocked(const std::string& prefix, const ComboAddress& remoteIP, const DNSName& qname, const QType& qtype, bool pierceDontQuery);
 
-  vector<ComboAddress> retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector<DNSName >::const_iterator& tns, const unsigned int depth, set<GetBestNSAnswer>& beenthere, const vector<DNSName >& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet);
+  vector<ComboAddress> retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector<DNSName >::const_iterator& tns, const unsigned int depth, set<GetBestNSAnswer>& beenthere, const vector<DNSName >& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly);
   RCode::rcodes_ updateCacheFromRecords(unsigned int depth, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, const boost::optional<Netmask>, vState& state, bool& needWildcardProof);
   bool processRecords(const std::string& prefix, const DNSName& qname, const QType& qtype, const DNSName& auth, LWResult& lwr, const bool sendRDQuery, vector<DNSRecord>& ret, set<DNSName>& nsset, DNSName& newtarget, DNSName& newauth, bool& realreferral, bool& negindic, vState& state, bool needWildcardProof);