From 45a08beae865eab8e733a832e2a9c62e73daecc1 Mon Sep 17 00:00:00 2001
From: Pieter Lexis <pieter.lexis@powerdns.com>
Date: Fri, 15 Jul 2016 16:25:32 +0200
Subject: [PATCH] Validate all key paths on possible Insecure

Before, we only checked the first QName, now we go through every name we
have to verify that the answer is indeed insecure.
---
 pdns/validate-recursor.cc | 48 ++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/pdns/validate-recursor.cc b/pdns/validate-recursor.cc
index 49ce11552..5819cfe7a 100644
--- a/pdns/validate-recursor.cc
+++ b/pdns/validate-recursor.cc
@@ -32,6 +32,25 @@ inline vState increaseDNSSECStateCounter(const vState& state)
   return state;
 }
 
+/*
+ * This inline possibly sets currentState based on the new state. It will only
+ * set it to Secure iff the newState is Secure and mayUpgradeToSecure == true.
+ * This should be set by the calling function when checking more than one record
+ * and this is not the first record, this way, we can never go *back* to Secure
+ * from an Insecure vState
+ */
+inline void processNewState(vState& currentState, const vState& newState, bool& hadNTA, const bool& mayUpgradeToSecure)
+{
+  if (mayUpgradeToSecure && newState == Secure)
+    currentState = Secure;
+
+  if (newState == Insecure || newState == NTA) // We can never go back to Secure
+    currentState = Insecure;
+
+  if (newState == NTA)
+    hadNTA = true;
+}
+
 vState validateRecords(const vector<DNSRecord>& recs)
 {
   if(recs.empty())
@@ -63,15 +82,9 @@ vState validateRecords(const vector<DNSRecord>& recs)
         if (newState == Bogus) // No hope
           return increaseDNSSECStateCounter(Bogus);
 
-        if (first && newState == Secure)
-          state = Secure;
-        first = false;
-
-        if (newState == Insecure || newState == NTA) // We can never go back to Secure
-          state = Insecure;
+        processNewState(state, newState, hadNTA, first);
 
-        if (newState == NTA)
-          hadNTA = true;
+        first = false;
 
         LOG("! state = "<<vStates[state]<<", now have "<<keys.size()<<" keys"<<endl);
         for(const auto& k : keys) {
@@ -83,13 +96,22 @@ vState validateRecords(const vector<DNSRecord>& recs)
   }
   else {
     LOG("! no sigs, hoping for Insecure status of "<<recs.begin()->d_name<<endl);
-    state = getKeysFor(sro, recs.begin()->d_name, keys); // um WHAT DOES THIS MEAN - try first qname??
-   
-    LOG("! state = "<<vStates[state]<<", now have "<<keys.size()<<" keys "<<endl);
-    
+
+    bool first = true;
+    for(const auto& rec : recs) {
+      vState newState = getKeysFor(sro, rec.d_name, keys);
+
+      if (newState == Bogus) // We're done
+        return increaseDNSSECStateCounter(Bogus);
+
+      processNewState(state, newState, hadNTA, first);
+      first = false;
+
+      LOG("! state = "<<vStates[state]<<", now have "<<keys.size()<<" keys "<<endl);
+    }
     return increaseDNSSECStateCounter(state);
   }
-  
+
   LOG("Took "<<sro.d_queries<<" queries"<<endl);
   if(validrrsets.size() == cspmap.size())// shortcut - everything was ok
     return increaseDNSSECStateCounter(Secure);
-- 
2.49.0