From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 22 Sep 2020 14:49:34 +0000 (+0200)
Subject: rec: Fix returning more than one cached records
X-Git-Tag: auth-4.4.0-alpha2~39^2~2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=dc3f2d3827e28044473d9e211043742cc69021c6;p=thirdparty%2Fpdns.git

rec: Fix returning more than one cached records

The existing implementation did not properly update the DNSSEC
and authoritative status, and did not include all the needed
RRSIG and additional records.
---

diff --git a/pdns/recursor_cache.cc b/pdns/recursor_cache.cc
index 4f209d3147..2169670d69 100644
--- a/pdns/recursor_cache.cc
+++ b/pdns/recursor_cache.cc
@@ -104,20 +104,20 @@ int32_t MemRecursorCache::handleHit(MapCombo& map, MemRecursorCache::OrderedTagI
     }
   }
 
-  if(signatures) { // if you do an ANY lookup you are hosed XXXX
-    *signatures = entry->d_signatures;
+  if (signatures) {
+    signatures->insert(signatures->end(), entry->d_signatures.begin(), entry->d_signatures.end());
   }
 
-  if(authorityRecs) {
-    *authorityRecs = entry->d_authorityRecs;
+  if (authorityRecs) {
+    authorityRecs->insert(authorityRecs->end(), entry->d_authorityRecs.begin(), entry->d_authorityRecs.end());
   }
 
   if (state) {
-    *state = entry->d_state;
+    updateDNSSECValidationState(*state, entry->d_state);
   }
 
   if (wasAuth) {
-    *wasAuth = entry->d_auth;
+    *wasAuth = *wasAuth && entry->d_auth;
   }
 
   moveCacheItemToBack<SequencedTag>(map.d_map, entry);
@@ -224,6 +224,14 @@ int32_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType& qt,
     res->clear();
   }
   const uint16_t qtype = qt.getCode();
+  if (wasAuth) {
+    // we might retrieve more than one entry, we need to set that to true
+    // so it will be set to false if at least one entry is not auth
+    *wasAuth = true;
+  }
+  if (state) {
+    *state = vState::Indeterminate;
+  }
 
   auto& map = getMap(qname);
   const lock l(map);
@@ -494,12 +502,19 @@ bool MemRecursorCache::doAgeCache(time_t now, const DNSName& name, uint16_t qtyp
 
 bool MemRecursorCache::updateValidationStatus(time_t now, const DNSName &qname, const QType& qt, const ComboAddress& who, const OptTag& routingTag, bool requireAuth, vState newState, boost::optional<time_t> capTTD)
 {
+  uint16_t qtype = qt.getCode();
+  if (qtype == QType::ANY) {
+    throw std::runtime_error("Trying to update the DNSSEC validation status of all (via ANY) records for " + qname.toLogString());
+  }
+  if (qtype == QType::ADDR) {
+    throw std::runtime_error("Trying to update the DNSSEC validation status of several (via ADDR) records for " + qname.toLogString());
+  }
+
   auto& map = getMap(qname);
   const lock l(map);
 
   bool updated = false;
-  uint16_t qtype = qt.getCode();
-  if (qtype != QType::ANY && qtype != QType::ADDR && !map.d_ecsIndex.empty() && !routingTag) {
+  if (!map.d_ecsIndex.empty() && !routingTag) {
     auto entry = getEntryUsingECSIndex(map, now, qname, qtype, requireAuth, who);
     if (entry == map.d_map.end()) {
       return false;
@@ -527,8 +542,7 @@ bool MemRecursorCache::updateValidationStatus(time_t now, const DNSName &qname,
     }
     updated = true;
 
-    if(qtype != QType::ANY && qtype != QType::ADDR) // normally if we have a hit, we are done
-      break;
+    break;
   }
 
   return updated;
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 89df89bd86..b1049fc0d2 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -2246,24 +2246,7 @@ uint32_t SyncRes::computeLowestTTD(const std::vector<DNSRecord>& records, const
 void SyncRes::updateValidationState(vState& state, const vState stateUpdate)
 {
   LOG(d_prefix<<"validation state was "<<state<<", state update is "<<stateUpdate);
-
-  if (stateUpdate == vState::TA) {
-    state = vState::Secure;
-  }
-  else if (stateUpdate == vState::NTA) {
-    state = vState::Insecure;
-  }
-  else if (stateUpdate == vState::Bogus) {
-    state = vState::Bogus;
-  }
-  else if (state == vState::Indeterminate) {
-    state = stateUpdate;
-  }
-  else if (stateUpdate == vState::Insecure) {
-    if (state != vState::Bogus) {
-      state = vState::Insecure;
-    }
-  }
+  updateDNSSECValidationState(state, stateUpdate);
   LOG(", validation state is now "<<state<<endl);
 }
 
diff --git a/pdns/validate.cc b/pdns/validate.cc
index 88a120826e..56bdb98842 100644
--- a/pdns/validate.cc
+++ b/pdns/validate.cc
@@ -1152,3 +1152,24 @@ std::ostream& operator<<(std::ostream &os, const dState d)
   os<<dStates.at(static_cast<size_t>(d));
   return os;
 }
+
+void updateDNSSECValidationState(vState& state, const vState stateUpdate)
+{
+  if (stateUpdate == vState::TA) {
+    state = vState::Secure;
+  }
+  else if (stateUpdate == vState::NTA) {
+    state = vState::Insecure;
+  }
+  else if (stateUpdate == vState::Bogus) {
+    state = vState::Bogus;
+  }
+  else if (state == vState::Indeterminate) {
+    state = stateUpdate;
+  }
+  else if (stateUpdate == vState::Insecure) {
+    if (state != vState::Bogus) {
+      state = vState::Insecure;
+    }
+  }
+}
diff --git a/pdns/validate.hh b/pdns/validate.hh
index 60054aa130..6627bee1f1 100644
--- a/pdns/validate.hh
+++ b/pdns/validate.hh
@@ -82,3 +82,4 @@ bool denialProvesNoDelegation(const DNSName& zone, const std::vector<DNSRecord>&
 bool isRRSIGNotExpired(const time_t now, const shared_ptr<RRSIGRecordContent> sig);
 bool isWildcardExpanded(unsigned int labelCount, const std::shared_ptr<RRSIGRecordContent>& sign);
 bool isWildcardExpandedOntoItself(const DNSName& owner, unsigned int labelCount, const std::shared_ptr<RRSIGRecordContent>& sign);
+void updateDNSSECValidationState(vState& state, const vState stateUpdate);