rec: Fix IXFR skipping the additions part of the last sequence

Under certain conditions, we could have skipped the additions part
of the last `IXFR` sequence, because we stopped processing records
after seeing a `SOA` record with the new serial. However, as stated
in rfc1995's "Response format" section:

"the first RR of the added RRs is the newer SOA RR"

diff --git a/pdns/ixfr.cc b/pdns/ixfr.cc
index eff4587c21c9b6d92e2d37c22bec770908c7c16d..74982b518f779e2e13d24d345196c037cc4a2a07 100644 (file)
--- a/pdns/ixfr.cc
+++ b/pdns/ixfr.cc
@@ -26,6 +26,96 @@
 #include "dnssecinfra.hh"
 #include "tsigverifier.hh"
 
+vector<pair<vector<DNSRecord>, vector<DNSRecord> > > processIXFRRecords(const ComboAddress& master, const DNSName& zone,
+                                                                        const vector<DNSRecord>& records, const std::shared_ptr<SOARecordContent> masterSOA)
+{
+  vector<pair<vector<DNSRecord>, vector<DNSRecord> > >  ret;
+
+  if (records.size() == 0 || masterSOA == nullptr) {
+    return ret;
+  }
+
+  // we start at 1 to skip the first SOA record
+  // we don't increase pos because the final SOA
+  // of the previous sequence is also the first SOA
+  // of this one
+  for(unsigned int pos = 1; pos < records.size(); ) {
+    vector<DNSRecord> remove, add;
+
+    // cerr<<"Looking at record in position "<<pos<<" of type "<<QType(records[pos].d_type).getName()<<endl;
+
+    if (records[pos].d_type != QType::SOA) {
+      // this is an actual AXFR!
+      return {{remove, records}};
+    }
+
+    auto sr = getRR<SOARecordContent>(records[pos]);
+    if (!sr) {
+      throw std::runtime_error("Error getting the content of the first SOA record of this IXFR sequence for zone '"+zone.toString()+"' from master '"+master.toStringWithPort()+"'");
+    }
+
+    // cerr<<"Serial is "<<sr->d_st.serial<<", final serial is "<<masterSOA->d_st.serial<<endl;
+
+    // the serial of this SOA record is the serial of the
+    // zone before the removals and updates of this sequence
+    if (sr->d_st.serial == masterSOA->d_st.serial) {
+      // if it's the final SOA, there is nothing for us to see
+      break;
+    }
+
+    remove.push_back(records[pos]); // this adds the SOA
+
+    // process removals
+    for(pos++; pos < records.size() && records[pos].d_type != QType::SOA; ++pos) {
+      remove.push_back(records[pos]);
+    }
+
+    if (pos >= records.size()) {
+      throw std::runtime_error("No SOA record to finish the removals part of the IXFR sequence of zone '" + zone.toString() + "' from " + master.toStringWithPort());
+    }
+
+    sr = getRR<SOARecordContent>(records[pos]);
+    if (!sr) {
+      throw std::runtime_error("Invalid SOA record to finish the removals part of the IXFR sequence of zone '" + zone.toString() + "' from " + master.toStringWithPort());
+    }
+
+    // this is the serial of the zone after the removals
+    // and updates, but that might not be the final serial
+    // because there might be several sequences
+    uint32_t newSerial = sr->d_st.serial;
+    add.push_back(records[pos]); // this adds the new SOA
+
+    // process additions
+    for(pos++; pos < records.size() && records[pos].d_type != QType::SOA; ++pos)  {
+      add.push_back(records[pos]);
+    }
+
+    if (pos >= records.size()) {
+      throw std::runtime_error("No SOA record to finish the additions part of the IXFR sequence of zone '" + zone.toString() + "' from " + master.toStringWithPort());
+    }
+
+    sr = getRR<SOARecordContent>(records[pos]);
+    if (!sr) {
+      throw std::runtime_error("Invalid SOA record to finish the additions part of the IXFR sequence of zone '" + zone.toString() + "' from " + master.toStringWithPort());
+    }
+
+    if (sr->d_st.serial != newSerial) {
+      throw std::runtime_error("Invalid serial (" + std::to_string(sr->d_st.serial) + ", expecting " + std::to_string(newSerial) + ") in the SOA record finishing the additions part of the IXFR sequence of zone '" + zone.toString() + "' from " + master.toStringWithPort());
+    }
+
+    if (newSerial == masterSOA->d_st.serial) {
+      // this was the last sequence
+      if (pos != (records.size() - 1)) {
+        throw std::runtime_error("Trailing records after the last IXFR sequence of zone '" + zone.toString() + "' from " + master.toStringWithPort());
+      }
+    }
+
+    ret.push_back(make_pair(remove,add));
+  }
+
+  return ret;
+}
+
 // Returns pairs of "remove & add" vectors. If you get an empty remove, it means you got an AXFR!
 vector<pair<vector<DNSRecord>, vector<DNSRecord> > > getIXFRDeltas(const ComboAddress& master, const DNSName& zone, const DNSRecord& oursr, 
                                                                    const TSIGTriplet& tt, const ComboAddress* laddr, size_t maxReceivedBytes)
@@ -75,24 +165,26 @@ vector<pair<vector<DNSRecord>, vector<DNSRecord> > > getIXFRDeltas(const ComboAd
   //   SOA WHERE THIS DELTA GOES
   //   RECORDS TO ADD
   // CURRENT MASTER SOA 
-  shared_ptr<SOARecordContent> masterSOA;
+  std::shared_ptr<SOARecordContent> masterSOA = nullptr;
   vector<DNSRecord> records;
   size_t receivedBytes = 0;
 
   for(;;) {
-    if(s.read((char*)&len, 2)!=2)
+    if(s.read((char*)&len, sizeof(len)) != sizeof(len))
       break;
+
     len=ntohs(len);
     //    cout<<"Got chunk of "<<len<<" bytes"<<endl;
     if(!len)
       break;
 
     if (maxReceivedBytes > 0 && (maxReceivedBytes - receivedBytes) < (size_t) len)
-      throw std::runtime_error("Reached the maximum number of received bytes in an IXFR delta for zone '"+zone.toString()+"' from master '"+master.toStringWithPort());
+      throw std::runtime_error("Reached the maximum number of received bytes in an IXFR delta for zone '"+zone.toString()+"' from master "+master.toStringWithPort());
 
     char reply[len]; 
     readn2(s.getHandle(), reply, len);
     receivedBytes += len;
+
     MOADNSParser mdp(false, string(reply, len));
     if(mdp.d_header.rcode) 
       throw std::runtime_error("Got an error trying to IXFR zone '"+zone.toString()+"' from master '"+master.toStringWithPort()+"': "+RCode::to_s(mdp.d_header.rcode));
@@ -104,49 +196,41 @@ vector<pair<vector<DNSRecord>, vector<DNSRecord> > > getIXFRDeltas(const ComboAd
     }
 
     for(auto& r: mdp.d_answers) {
-      if(r.first.d_type == QType::TSIG) 
-        continue;
       //      cout<<r.first.d_name<< " " <<r.first.d_content->getZoneRepresentation()<<endl;
-      r.first.d_name = r.first.d_name.makeRelative(zone);
-      records.push_back(r.first);
-      if(r.first.d_type == QType::SOA) {
+      if(!masterSOA) {
+        // we have not seen the first SOA record yet
+        if (r.first.d_type != QType::SOA) {
+          throw std::runtime_error("The first record of the IXFR answer for zone '"+zone.toString()+"' from master '"+master.toStringWithPort()+"' is not a SOA ("+QType(r.first.d_type).getName()+")");
+        }
+
        auto sr = getRR<SOARecordContent>(r.first);
-       if(sr) {
-         if(!masterSOA) {
-           if(sr->d_st.serial == std::dynamic_pointer_cast<SOARecordContent>(oursr.d_content)->d_st.serial) { // we are up to date
-             goto done;
-           }
-           masterSOA=sr;
-         }
-         else if(sr->d_st.serial == masterSOA->d_st.serial)
-           goto done;
-       }
+       if (!sr) {
+          throw std::runtime_error("Error getting the content of the first SOA record of the IXFR answer for zone '"+zone.toString()+"' from master '"+master.toStringWithPort()+"'");
+        }
+
+        if(sr->d_st.serial == std::dynamic_pointer_cast<SOARecordContent>(oursr.d_content)->d_st.serial) {
+          // we are up to date
+          return ret;
+        }
+        masterSOA = sr;
       }
-    }
-  }
-  //  cout<<"Got "<<records.size()<<" records"<<endl;
- done:;
-  for(unsigned int pos = 1;pos < records.size();) {
-    auto sr = getRR<SOARecordContent>(records[pos]);
-    vector<DNSRecord> remove, add;
-    if(!sr) { // this is an actual AXFR!
-      return {{remove, records}};
-    }
-    if(sr->d_st.serial == masterSOA->d_st.serial)
-      break;
-    
 
-    remove.push_back(records[pos]); // this adds the SOA
-    for(pos++; pos < records.size() && records[pos].d_type != QType::SOA; ++pos) {
-      remove.push_back(records[pos]);
-    }
-    sr = getRR<SOARecordContent>(records[pos]);
+      if(r.first.d_place != DNSResourceRecord::ANSWER) {
+        if(r.first.d_type == QType::TSIG)
+          continue;
 
-    add.push_back(records[pos]); // this adds the new SOA
-    for(pos++; pos < records.size() && records[pos].d_type != QType::SOA; ++pos)  {
-      add.push_back(records[pos]);
+        if(r.first.d_type == QType::OPT)
+          continue;
+
+        throw std::runtime_error("Unexpected record (" +QType(r.first.d_type).getName()+") in non-answer section ("+std::to_string(r.first.d_place)+")in IXFR response for zone '"+zone.toString()+"' from master '"+master.toStringWithPort());
+      }
+
+      r.first.d_name.makeUsRelative(zone);
+      records.push_back(r.first);
     }
-    ret.push_back(make_pair(remove,add));
   }
-  return ret;
+
+  //  cout<<"Got "<<records.size()<<" records"<<endl;
+
+  return processIXFRRecords(master, zone, records, masterSOA);
 }

diff --git a/pdns/ixfr.hh b/pdns/ixfr.hh
index 537da35f062568b0bcc78b10ccc6a6efc4a24973..970222f7e50b3681b9ee4ada30dbc24ae8485b78 100644 (file)
--- a/pdns/ixfr.hh
+++ b/pdns/ixfr.hh
@@ -22,7 +22,11 @@
 #include "namespaces.hh"
 #include "iputils.hh"
 #include "dnsparser.hh"
+#include "dnsrecords.hh"
 
 vector<pair<vector<DNSRecord>, vector<DNSRecord> > >   getIXFRDeltas(const ComboAddress& master, const DNSName& zone, 
                                                                      const DNSRecord& sr, const TSIGTriplet& tt=TSIGTriplet(),
                                                                      const ComboAddress* laddr=0, size_t maxReceivedBytes=0);
+
+vector<pair<vector<DNSRecord>, vector<DNSRecord> > > processIXFRRecords(const ComboAddress& master, const DNSName& zone,
+                                                                        const vector<DNSRecord>& records, const std::shared_ptr<SOARecordContent> masterSOA);