From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Wed, 25 Jan 2023 09:18:01 +0000 (+0100)
Subject: Process review comments from @rgacogne, thanks!
X-Git-Tag: dnsdist-1.8.0-rc1~83^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F12434%2Fhead;p=thirdparty%2Fpdns.git

Process review comments from @rgacogne, thanks!
---

diff --git a/pdns/logger.hh b/pdns/logger.hh
index bbbc8cd50b..d05b532744 100644
--- a/pdns/logger.hh
+++ b/pdns/logger.hh
@@ -24,7 +24,9 @@
 #include <string>
 #include <ctime>
 #include <iostream>
+#include <optional>
 #include <sstream>
+#include <variant>
 #include <syslog.h>
 
 #include "namespaces.hh"
@@ -168,7 +170,7 @@ Logger& getLogger();
 #endif
 
 // The types below are used by rec, which can log to g_log (general logging) or a string stream
-// (trace-regexp). We feed an OptLog object to the code that should not know anything about this
+// (trace-regexp). We pass an OptLog object to the code that should not know anything about this
 // That code should then log using VLOG
 
 struct LogVariant
@@ -181,7 +183,7 @@ struct LogVariant
 using OptLog = std::optional<LogVariant>;
 
 #ifndef RECURSOR
-// Originally there was a flag but is was never set from !RECURSOR
+// Originally there was a flag but it was never set from !RECURSOR
 #define VLOG(log, x) VLOG only works in recursor
 #else
 #define VLOG(log, x)                                                       \
diff --git a/pdns/validate.cc b/pdns/validate.cc
index b75071b576..5a085feca1 100644
--- a/pdns/validate.cc
+++ b/pdns/validate.cc
@@ -896,41 +896,6 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
   return dState::NODENIAL;
 }
 
-/*
- * Finds all the zone-cuts between begin (longest name) and end (shortest name),
- * returns them all zone cuts, including end, but (possibly) not begin
- */
-static const vector<DNSName> getZoneCuts(const DNSName& begin, const DNSName& end, DNSRecordOracle& dro)
-{
-  vector<DNSName> ret;
-  if(!begin.isPartOf(end))
-    throw PDNSException(end.toLogString() + "is not part of " + begin.toLogString());
-
-  DNSName qname(end);
-  vector<string> labelsToAdd = begin.makeRelative(end).getRawLabels();
-
-  // The shortest name is assumed to a zone cut
-  ret.push_back(qname);
-  while(qname != begin) {
-    bool foundCut = false;
-    if (labelsToAdd.empty())
-      break;
-
-    qname.prependRawLabel(labelsToAdd.back());
-    labelsToAdd.pop_back();
-    auto records = dro.get(qname, (uint16_t)QType::NS);
-    for (const auto& record : records) {
-      if(record.d_type != QType::NS || record.d_name != qname)
-        continue;
-      foundCut = true;
-      break;
-    }
-    if (foundCut)
-      ret.push_back(qname);
-  }
-  return ret;
-}
-
 bool isRRSIGNotExpired(const time_t now, const shared_ptr<RRSIGRecordContent>& sig)
 {
   // Should use https://www.rfc-editor.org/rfc/rfc4034.txt section 3.1.5
@@ -1038,23 +1003,6 @@ vState validateWithKeySet(time_t now, const DNSName& name, const sortedRecords_t
   return vState::BogusNoValidRRSIG;
 }
 
-void validateWithKeySet(const cspmap_t& rrsets, cspmap_t& validated, const skeyset_t& keys, const OptLog& log)
-{
-  validated.clear();
-  /*  cerr<<"Validating an rrset with following keys: "<<endl;
-  for(auto& key : keys) {
-    cerr<<"\tTag: "<<key->getTag()<<" -> "<<key->getZoneRepresentation()<<endl;
-  }
-  */
-  time_t now = time(nullptr);
-  for(auto i=rrsets.cbegin(); i!=rrsets.cend(); i++) {
-    VLOG(log, "validating "<<(i->first.first)<<"/"<<DNSRecordContent::NumberToType(i->first.second)<<" with "<<i->second.signatures.size()<<" sigs"<<endl);
-    if (validateWithKeySet(now, i->first.first, i->second.records, i->second.signatures, keys, log, true) == vState::Secure) {
-      validated[i->first] = i->second;
-    }
-  }
-}
-
 // returns vState
 // should return vState, zone cut and validated keyset
 // i.e. www.7bits.nl -> insecure/7bits.nl/[]
@@ -1241,160 +1189,6 @@ vState validateDNSKeysAgainstDS(time_t now, const DNSName& zone, const dsmap_t&
   return vState::Secure;
 }
 
-vState getKeysFor(DNSRecordOracle& dro, const DNSName& zone, skeyset_t& keyset, const OptLog& log)
-{
-  auto luaLocal = g_luaconfs.getLocal();
-  const auto anchors = luaLocal->dsAnchors;
-  if (anchors.empty()) // Nothing to do here
-    return vState::Insecure;
-
-  // Determine the lowest (i.e. with the most labels) Trust Anchor for zone
-  DNSName lowestTA(".");
-  for (auto const &anchor : anchors)
-    if (zone.isPartOf(anchor.first) && lowestTA.countLabels() < anchor.first.countLabels())
-      lowestTA = anchor.first;
-
-  // Before searching for the keys, see if we have a Negative Trust Anchor. If
-  // so, test if the NTA is valid and return an NTA state
-  const auto negAnchors = luaLocal->negAnchors;
-
-  if (!negAnchors.empty()) {
-    DNSName lowestNTA;
-
-    for (auto const &negAnchor : negAnchors)
-      if (zone.isPartOf(negAnchor.first) && lowestNTA.countLabels() <= negAnchor.first.countLabels())
-        lowestNTA = negAnchor.first;
-
-    if(!lowestNTA.empty()) {
-      VLOG(log, "Found a Negative Trust Anchor for "<<lowestNTA<<", which was added with reason '"<<negAnchors.at(lowestNTA)<<"', ");
-
-      /* RFC 7646 section 2.1 tells us that we SHOULD still validate if there
-       * is a Trust Anchor below the Negative Trust Anchor for the name we
-       * attempt validation for. However, section 3 tells us this positive
-       * Trust Anchor MUST be *below* the name and not the name itself
-       */
-      if(lowestTA.countLabels() <= lowestNTA.countLabels()) {
-        VLOG(log, "marking answer Insecure"<<endl);
-        return vState::NTA; // Not Insecure, this way validateRecords() can shortcut
-      }
-      VLOG(log, "but a Trust Anchor for "<<lowestTA<<" is configured, continuing validation."<<endl);
-    }
-  }
-
-  skeyset_t validkeys;
-  dsmap_t dsmap;
-
-  dsmap_t* tmp = (dsmap_t*) rplookup(anchors, lowestTA);
-  if (tmp)
-    dsmap = *tmp;
-
-  auto zoneCuts = getZoneCuts(zone, lowestTA, dro);
-
-  VLOG(log, "Found the following zonecuts:")
-  for(const auto& zonecut : zoneCuts)
-    VLOG(log, " => "<<zonecut);
-  VLOG(log, endl);
-
-  for(auto zoneCutIter = zoneCuts.cbegin(); zoneCutIter != zoneCuts.cend(); ++zoneCutIter)
-  {
-    vector<shared_ptr<RRSIGRecordContent> > sigs;
-    sortedRecords_t toSign;
-
-    skeyset_t tkeys; // tentative keys
-    validkeys.clear();
-
-    //    cerr<<"got DS for ["<<qname<<"], grabbing DNSKEYs"<<endl;
-    auto records=dro.get(*zoneCutIter, (uint16_t)QType::DNSKEY);
-    // this should use harvest perhaps
-    for(const auto& rec : records) {
-      if(rec.d_name != *zoneCutIter)
-        continue;
-
-      if(rec.d_type == QType::RRSIG)
-      {
-        auto rrc=getRR<RRSIGRecordContent> (rec);
-        if(rrc) {
-          VLOG(log, "Got signature: "<<rrc->getZoneRepresentation()<<" with tag "<<rrc->d_tag<<", for type "<<DNSRecordContent::NumberToType(rrc->d_type)<<endl);
-          if(rrc->d_type != QType::DNSKEY)
-            continue;
-          sigs.push_back(rrc);
-        }
-      }
-      else if(rec.d_type == QType::DNSKEY)
-      {
-        auto drc=getRR<DNSKEYRecordContent> (rec);
-        if(drc) {
-          tkeys.insert(drc);
-          VLOG(log, "Inserting key with tag "<<drc->getTag()<<" and algorithm "<<DNSSECKeeper::algorithm2name(drc->d_algorithm)<<": "<<drc->getZoneRepresentation()<<endl);
-
-          toSign.insert(rec.d_content);
-        }
-      }
-    }
-    VLOG(log, "got "<<tkeys.size()<<" keys and "<<sigs.size()<<" sigs from server"<<endl);
-
-    /*
-     * Check all DNSKEY records against all DS records and place all DNSKEY records
-     * that have DS records (that we support the algo for) in the tentative key storage
-     */
-    auto state = validateDNSKeysAgainstDS(time(nullptr), *zoneCutIter, dsmap, tkeys, toSign, sigs, validkeys, log);
-
-    if (validkeys.empty())
-    {
-      VLOG(log, "ended up with zero valid DNSKEYs, going Bogus"<<endl);
-      return state;
-    }
-    VLOG(log, "situation: we have one or more valid DNSKEYs for ["<<*zoneCutIter<<"] (want ["<<zone<<"])"<<endl);
-
-    if (zoneCutIter == zoneCuts.cend()-1) {
-      VLOG(log, "requested keyset found! returning Secure for the keyset"<<endl);
-      keyset.insert(validkeys.cbegin(), validkeys.cend());
-      return state;
-    }
-
-    // We now have the DNSKEYs, use them to validate the DS records at the next zonecut
-    VLOG(log, "next name ["<<*(zoneCutIter+1)<<"], trying to get DS"<<endl);
-
-    dsmap_t tdsmap; // tentative DSes
-    dsmap.clear();
-    toSign.clear();
-
-    auto recs=dro.get(*(zoneCutIter+1), QType::DS);
-
-    cspmap_t cspmap=harvestCSPFromRecs(recs);
-
-    cspmap_t validrrsets;
-    validateWithKeySet(cspmap, validrrsets, validkeys, log);
-
-    VLOG(log, "got "<<cspmap.count(pair(*(zoneCutIter+1),QType::DS))<<" records for DS query of which "<<validrrsets.count(pair(*(zoneCutIter+1),QType::DS))<<" valid "<<endl);
-
-    auto r = validrrsets.equal_range(pair(*(zoneCutIter+1), QType::DS));
-    if(r.first == r.second) {
-      VLOG(log, "No DS for "<<*(zoneCutIter+1)<<", now look for a secure denial"<<endl);
-      dState res = getDenial(validrrsets, *(zoneCutIter+1), QType::DS, true, true, log);
-      if (res == dState::INSECURE || res == dState::NXDOMAIN)
-        return vState::BogusInvalidDenial;
-      if (res == dState::NXQTYPE || res == dState::OPTOUT)
-        return vState::Insecure;
-    }
-
-    /*
-     * Collect all DS records and add them to the dsmap for the next iteration
-     */
-    for(auto cspiter =r.first;  cspiter!=r.second; cspiter++) {
-      for(auto j=cspiter->second.records.cbegin(); j!=cspiter->second.records.cend(); j++)
-      {
-        const auto dsrc=std::dynamic_pointer_cast<DSRecordContent>(*j);
-        if(dsrc) {
-          dsmap.insert(*dsrc);
-        }
-      }
-    }
-  }
-  // There were no zone cuts (aka, we should never get here)
-  return vState::BogusUnableToGetDNSKEYs;
-}
-
 bool isSupportedDS(const DSRecordContent& ds, const OptLog& log)
 {
   if (!DNSCryptoKeyEngine::isAlgorithmSupported(ds.d_algorithm)) {
diff --git a/pdns/validate.hh b/pdns/validate.hh
index c211b2a4eb..3226c2c3d8 100644
--- a/pdns/validate.hh
+++ b/pdns/validate.hh
@@ -77,9 +77,7 @@ vState validateWithKeySet(time_t now, const DNSName& name, const sortedRecords_t
 bool isCoveredByNSEC(const DNSName& name, const DNSName& begin, const DNSName& next);
 bool isCoveredByNSEC3Hash(const std::string& h, const std::string& beginHash, const std::string& nextHash);
 bool isCoveredByNSEC3Hash(const DNSName& h, const DNSName& beginHash, const DNSName& nextHash);
-void validateWithKeySet(const cspmap_t& rrsets, cspmap_t& validated, const skeyset_t& keys, const OptLog& log);
 cspmap_t harvestCSPFromRecs(const vector<DNSRecord>& recs);
-vState getKeysFor(DNSRecordOracle& dro, const DNSName& zone, skeyset_t& keyset);
 bool getTrustAnchor(const map<DNSName,dsmap_t>& anchors, const DNSName& zone, dsmap_t &res);
 bool haveNegativeTrustAnchor(const map<DNSName,std::string>& negAnchors, const DNSName& zone, std::string& reason);
 vState validateDNSKeysAgainstDS(time_t now, const DNSName& zone, const dsmap_t& dsmap, const skeyset_t& tkeys, const sortedRecords_t& toSign, const vector<shared_ptr<RRSIGRecordContent> >& sigs, skeyset_t& validkeys, const OptLog&);