From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Fri, 9 Nov 2018 10:36:09 +0000 (+0100)
Subject: rec: Revert 'Keep the EDNS status of a server on FormErr with EDNS'
X-Git-Tag: rec-4.1.7^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F7172%2Fhead;p=thirdparty%2Fpdns.git

rec: Revert 'Keep the EDNS status of a server on FormErr with EDNS'
---

diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index e95262e540..f5270aaba8 100644
--- a/pdns/recursordist/test-syncres_cc.cc
+++ b/pdns/recursordist/test-syncres_cc.cc
@@ -652,8 +652,9 @@ BOOST_AUTO_TEST_CASE(test_edns_formerr_but_edns_enabled) {
   initSR(sr);
 
   /* in this test, the auth answers with FormErr to an EDNS-enabled
-     query, but the response does contain EDNS so we should not mark
-     it as EDNS ignorant or intolerant.
+     query, but the response does contain EDNS so we might consider
+     that the server knows a bit about EDNS. It turns out that we can't
+     because there are too many awful servers out there.
   */
   size_t queriesWithEDNS = 0;
   size_t queriesWithoutEDNS = 0;
@@ -669,11 +670,15 @@ BOOST_AUTO_TEST_CASE(test_edns_formerr_but_edns_enabled) {
       }
       usedServers.insert(ip);
 
-      if (type == QType::DNAME) {
-        setLWResult(res, RCode::FormErr);
+      if (type == QType::A) {
         if (EDNS0Level > 0) {
+          setLWResult(res, RCode::FormErr);
           res->d_haveEDNS = true;
         }
+        else {
+          setLWResult(res, 0, true, false, false);
+          addRecordToLW(res, domain, QType::A, "192.0.2.1");
+        }
         return 1;
       }
 
@@ -683,15 +688,15 @@ BOOST_AUTO_TEST_CASE(test_edns_formerr_but_edns_enabled) {
   primeHints();
 
   vector<DNSRecord> ret;
-  int res = sr->beginResolve(DNSName("powerdns.com."), QType(QType::DNAME), QClass::IN, ret);
-  BOOST_CHECK_EQUAL(res, RCode::ServFail);
-  BOOST_CHECK_EQUAL(ret.size(), 0);
-  BOOST_CHECK_EQUAL(queriesWithEDNS, 26);
-  BOOST_CHECK_EQUAL(queriesWithoutEDNS, 0);
-  BOOST_CHECK_EQUAL(SyncRes::getEDNSStatusesSize(), 26);
-  BOOST_CHECK_EQUAL(usedServers.size(), 26);
+  int res = sr->beginResolve(DNSName("powerdns.com."), QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(ret.size(), 1);
+  BOOST_CHECK_EQUAL(queriesWithEDNS, 1);
+  BOOST_CHECK_EQUAL(queriesWithoutEDNS, 1);
+  BOOST_CHECK_EQUAL(SyncRes::getEDNSStatusesSize(), 1);
+  BOOST_CHECK_EQUAL(usedServers.size(), 1);
   for (const auto& server : usedServers) {
-    BOOST_CHECK_EQUAL(SyncRes::getEDNSStatus(server), SyncRes::EDNSStatus::EDNSOK);
+    BOOST_CHECK_EQUAL(SyncRes::getEDNSStatus(server), SyncRes::EDNSStatus::NOEDNS);
   }
 }
 
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index b33775f75c..2d0e4000f9 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -479,7 +479,12 @@ int SyncRes::asyncresolveWrapper(const ComboAddress& ip, bool ednsMANDATORY, con
       return ret;
     }
     else if(mode==EDNSStatus::UNKNOWN || mode==EDNSStatus::EDNSOK || mode == EDNSStatus::EDNSIGNORANT ) {
-      if(!res->d_haveEDNS && (res->d_rcode == RCode::FormErr || res->d_rcode == RCode::NotImp)) {
+      /* So, you might be tempted to treat the presence of EDNS in a response as meaning that the
+         server does understand EDNS, and thus prevent a downgrade to no EDNS.
+         It turns out that you can't because there are a lot of crappy servers out there,
+         so you have to treat a FormErr as 'I have no idea what this EDNS thing is' no matter what.
+      */
+      if(res->d_rcode == RCode::FormErr || res->d_rcode == RCode::NotImp) {
 	//	cerr<<"Downgrading to NOEDNS because of "<<RCode::to_s(res->d_rcode)<<" for query to "<<ip.toString()<<" for '"<<domain<<"'"<<endl;
         mode = EDNSStatus::NOEDNS;
         continue;