this fixes #3261 and likely #3532 and #3446. In the new model, SyncRes has one flag...

But the processing is mostly done anyhow (except for optimization). This also fixes everyone's favorite warning about State being unset, and it restores 'dnssec=process' as default

diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index e92cd6dccfa7cdbcd7e1341e8d9b37dc12ff2288..5b61e3161cdb9dbcfaaeea5ab502996e9f1593db 100644 (file)
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -630,14 +630,18 @@ void startDoResolve(void *p)
     uint32_t minTTL=std::numeric_limits<uint32_t>::max();
 
     SyncRes sr(dc->d_now);
+    bool DNSSECOK=false;
     if(t_pdl) {
       sr.setLuaEngine(*t_pdl);
       sr.d_requestor=dc->d_remote;
     }
+
+    if(g_dnssecmode != DNSSECMode::Off)
+      sr.d_doDNSSEC=true;
     
-    if(pw.getHeader()->cd || edo.d_Z & EDNSOpts::DNSSECOK) {
+    if(pw.getHeader()->cd || (edo.d_Z & EDNSOpts::DNSSECOK)) {
+      DNSSECOK=true;
       g_stats.dnssecQueries++;
-      sr.d_doDNSSEC=true;
     }
 
     bool tracedQuery=false; // we could consider letting Lua know about this too
@@ -808,20 +812,41 @@ void startDoResolve(void *p)
 
       if(haveEDNS) {
        if(g_dnssecmode != DNSSECMode::Off && ((edo.d_Z & EDNSOpts::DNSSECOK) || g_dnssecmode == DNSSECMode::ValidateAll || g_dnssecmode==DNSSECMode::ValidateForLog)) {
+          if(sr.doLog()) {
+            L<<Logger::Warning<<"Starting validation of answer to "<<dc->d_mdp.d_qname<<" for "<<dc->d_remote.toStringWithPort()<<endl;
+          }
          auto state=validateRecords(ret);
          if(state == Secure) {
+            if(sr.doLog()) {
+              L<<Logger::Warning<<"Answer to "<<dc->d_mdp.d_qname<<" for "<<dc->d_remote.toStringWithPort()<<" validates correctly"<<endl;
+            }
+          
            pw.getHeader()->ad=1;
          }
          else if(state == Insecure) {
+            if(sr.doLog()) {
+              L<<Logger::Warning<<"Answer to "<<dc->d_mdp.d_qname<<" for "<<dc->d_remote.toStringWithPort()<<" validates as Insecure"<<endl;
+            }
+
            pw.getHeader()->ad=0;
          }
-         else if(state == Bogus && !pw.getHeader()->cd) {
-            if(g_dnssecmode == DNSSECMode::ValidateAll || (edo.d_Z & EDNSOpts::DNSSECOK)) {
+         else if(state == Bogus ) {
+            if(sr.doLog()) {
+              L<<Logger::Warning<<"Answer to "<<dc->d_mdp.d_qname<<" for "<<dc->d_remote.toStringWithPort()<<" validates as Bogus"<<endl;
+            }
+            
+            if(!pw.getHeader()->cd && (g_dnssecmode == DNSSECMode::ValidateAll || (edo.d_Z & EDNSOpts::DNSSECOK))) {
+              if(sr.doLog()) {
+                L<<Logger::Warning<<"Sending out SERVFAIL for "<<dc->d_mdp.d_qname<<" because recursor or query demands it for Bogus results"<<endl;
+              }
+
               pw.getHeader()->rcode=RCode::ServFail;
               goto sendit;
-            }
-            else {
-              L<<Logger::Warning<<"Failed to validate "<<dc->d_mdp.d_qname<<" for "<<dc->d_remote.toStringWithPort()<<endl;
+            } else {
+              if(sr.doLog()) {
+                L<<Logger::Warning<<"Not sending out SERVFAIL for "<<dc->d_mdp.d_qname<<" Bogus validation since neither config nor query demands this"<<endl;
+              }
+
             }
          }
        }
@@ -841,6 +866,8 @@ void startDoResolve(void *p)
       }
       
       for(auto i=ret.cbegin(); i!=ret.cend(); ++i) {
+        if(!DNSSECOK && (i->d_type == QType::RRSIG || i->d_type==QType::NSEC || i->d_type==QType::NSEC3))
+          continue;
        pw.startRecord(i->d_name, i->d_type, i->d_ttl, i->d_class, i->d_place);
        if(i->d_type != QType::OPT) // their TTL ain't real
          minTTL = min(minTTL, i->d_ttl);
@@ -2633,7 +2660,7 @@ int main(int argc, char **argv)
     ::arg().set("local-address","IP addresses to listen on, separated by spaces or commas. Also accepts ports.")="127.0.0.1";
     ::arg().setSwitch("non-local-bind", "Enable binding to non-local addresses by using FREEBIND / BINDANY socket options")="no";
     ::arg().set("trace","if we should output heaps of logging. set to 'fail' to only log failing domains")="off";
-    ::arg().set("dnssec", "DNSSEC mode: off (default)/process/log-fail/validate")="off";
+    ::arg().set("dnssec", "DNSSEC mode: off (default)/process/log-fail/validate")="process";
     ::arg().set("daemon","Operate as a daemon")="no";
     ::arg().setSwitch("write-pid","Write a PID file")="yes";
     ::arg().set("loglevel","Amount of logging. Higher is more. Do not set below 3")="4";

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 11f3b1fff50fd21a66b4fccb51b5bd3fcd446fc0..6d947b7185260f1266774bc85ca62f427e9e93bc 100644 (file)
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -387,7 +387,7 @@ int SyncRes::doResolve(const DNSName &qname, const QType &qtype, vector<DNSRecor
     prefix.append(depth, ' ');
   }
   
-  LOG(prefix<<qname.toString()<<": Wants "<< (d_doDNSSEC ? "" : "NO ") << "DNSSEC processing"<<endl);
+  LOG(prefix<<qname.toString()<<": Wants "<< (d_doDNSSEC ? "" : "NO ") << "DNSSEC processing in query for "<<qtype.getName()<<endl);
 
   int res=0;
   if(!(d_nocache && qtype.getCode()==QType::NS && qname.isRoot())) {
@@ -759,14 +759,14 @@ bool SyncRes::doCacheCheck(const DNSName &qname, const QType &qtype, vector<DNSR
          giveNegative=true;
          sqname=ni->d_qname;
          sqt=QType::SOA;
-         if(d_doDNSSEC) {
-           for(const auto& p : ni->d_dnssecProof) {
-             for(const auto& rec: p.second.records) 
-               ret.push_back(rec);
-             for(const auto& rec: p.second.signatures) 
-               ret.push_back(rec);
-           }
-         }
+          if(d_doDNSSEC) {
+            for(const auto& p : ni->d_dnssecProof) {
+              for(const auto& rec: p.second.records) 
+                ret.push_back(rec);
+              for(const auto& rec: p.second.signatures) 
+                ret.push_back(rec);
+            }
+          }
          moveCacheItemToBack(t_sstorage->negcache, ni);
          break;
        }
@@ -1257,7 +1257,7 @@ int SyncRes::doResolveAt(map<DNSName, pair<ComboAddress, bool> > &nameservers, D
           ret.push_back(rec);
           newtarget=std::dynamic_pointer_cast<CNAMERecordContent>(rec.d_content)->getTarget();
         }
-       else if(d_doDNSSEC && (rec.d_type==QType::RRSIG || rec.d_type==QType::NSEC || rec.d_type==QType::NSEC3) && rec.d_place==DNSResourceRecord::ANSWER){
+       else if((rec.d_type==QType::RRSIG || rec.d_type==QType::NSEC || rec.d_type==QType::NSEC3) && rec.d_place==DNSResourceRecord::ANSWER){
          if(rec.d_type != QType::RRSIG || rec.d_name == qname)
            ret.push_back(rec); // enjoy your DNSSEC
        }
@@ -1343,6 +1343,7 @@ int SyncRes::doResolveAt(map<DNSName, pair<ComboAddress, bool> > &nameservers, D
       }
       if(nsset.empty() && !lwr.d_rcode && (negindic || lwr.d_aabit || sendRDQuery)) {
         LOG(prefix<<qname.toString()<<": status=noerror, other types may exist, but we are done "<<(negindic ? "(have negative SOA) " : "")<<(lwr.d_aabit ? "(have aa bit) " : "")<<endl);
+        
         if(d_doDNSSEC)
           addNXNSECS(ret, lwr.d_records);
         return 0;

diff --git a/pdns/validate-recursor.cc b/pdns/validate-recursor.cc
index af3f18b0671abe6468ad8ffc0fedb9eaa6f8aff6..d337fc445ea0348f4b04bdc4e1638e8114201add 100644 (file)
--- a/pdns/validate-recursor.cc
+++ b/pdns/validate-recursor.cc
@@ -41,37 +41,56 @@ vState validateRecords(const vector<DNSRecord>& recs)
 
   SRRecordOracle sro;
 
-  vState state;
+  vState state=Insecure;
   if(numsigs) {
     for(const auto& csp : cspmap) {
       for(const auto& sig : csp.second.signatures) {
         state = getKeysFor(sro, sig->d_signer, keys); // XXX check validity here
         //     cerr<<"! state = "<<vStates[state]<<", now have "<<keys.size()<<" keys"<<endl;
+        // this sort of charges on and 'state' ends up as the last thing to have been checked
+        // maybe not the right idea
       }
     }
-    if(state == Bogus) return state;
+    if(state == Bogus) {
+      return state;
+    }
     validateWithKeySet(cspmap, validrrsets, keys);
   }
   else {
     //    cerr<<"no sigs, hoping for Insecure"<<endl;
     state = getKeysFor(sro, recs.begin()->d_name, keys); // um WHAT DOES THIS MEAN - try first qname??
+   
     //    cerr<<"! state = "<<vStates[state]<<", now have "<<keys.size()<<" keys "<<endl;
     return state;
   }
   
-  //  cerr<<"! validated "<<validrrsets.size()<<" RRsets out of "<<cspmap.size()<<endl;
+  //  cerr<<"Took "<<sro.d_queries<<" queries"<<endl;
+  if(validrrsets.size() == cspmap.size()) // shortcut - everything was ok
+    return Secure;
+
+  if(keys.empty()) {
+    return Insecure;
+  }
+
+#if 0
+  cerr<<"! validated "<<validrrsets.size()<<" RRsets out of "<<cspmap.size()<<endl;
 
-  //  cerr<<"% validated RRs:"<<endl;
+  cerr<<"% validated RRs:"<<endl;
   for(auto i=validrrsets.begin(); i!=validrrsets.end(); i++) {
-    //    cerr<<"% "<<i->first.first<<"/"<<DNSRecordContent::NumberToType(i->first.second)<<endl;
+        cerr<<"% "<<i->first.first<<"/"<<DNSRecordContent::NumberToType(i->first.second)<<endl;
     for(auto j=i->second.records.begin(); j!=i->second.records.end(); j++) {
-      //      cerr<<"\t% > "<<(*j)->getZoneRepresentation()<<endl;
+            cerr<<"\t% > "<<(*j)->getZoneRepresentation()<<endl;
     }
   }
-  //  cerr<<"Took "<<sro.d_queries<<" queries"<<endl;
-  if(validrrsets.size() == cspmap.size())
-    return Secure;
-  if(keys.size())
-    return Bogus;
+#endif
+  //  cerr<<"Input to validate: "<<endl;
+  for(const auto& csp : cspmap) {
+    cerr<<csp.first.first<<"|"<<csp.first.second<<" with "<<csp.second.signatures.size()<<" signatures"<<endl;
+    if(!csp.second.signatures.empty() && !validrrsets.count(csp.first)) {
+      //      cerr<<"Lacks signature, must have one"<<endl;
+      return Bogus;
+    }
+  }
+  
   return Insecure;
 }