Replace s_redirectionQTypes by a simple function and simplify logging in sanitizeRecords

diff --git a/pdns/dns.cc b/pdns/dns.cc
index 5e7e45e1f604037a34084805734099c8a40ea28e..12f97eaee9e4ea567433815f47db1a144c98f1aa 100644 (file)
--- a/pdns/dns.cc
+++ b/pdns/dns.cc
@@ -127,3 +127,17 @@ uint32_t hashQuestion(const uint8_t* packet, uint16_t packet_len, uint32_t init,
   return init;
 }
 
+static const std::array<std::string, 4> placeNames = {
+  "QUESTION",
+  "ANSWER",
+  "AUTHORITY",
+  "ADDITIONAL"
+};
+
+std::string DNSResourceRecord::placeString(uint8_t place)
+{
+  if (place >= placeNames.size()) {
+    return "?";
+  }
+  return placeNames.at(place);
+}

diff --git a/pdns/dns.hh b/pdns/dns.hh
index c95a62f1c52fdd49e9fc1c7b8b79e7f1bb7870f3..b8dd761673c2311932af6a56f9f05c69a1df2dca 100644 (file)
--- a/pdns/dns.hh
+++ b/pdns/dns.hh
@@ -66,6 +66,7 @@ public:
     ADDITIONAL = 3
   }; //!< Type describing the positioning within, say, a DNSPacket
 
+  [[nodiscard]] static std::string placeString(uint8_t place);
   void setContent(const string& content);
   [[nodiscard]] string getZoneRepresentation(bool noDot = false) const;
 

diff --git a/pdns/dnsparser.hh b/pdns/dnsparser.hh
index 20ce6396beb87d5dccc6c4750649f56ec904a390..fa54d2a1c956beafe1600a46ca70d9435754cf78 100644 (file)
--- a/pdns/dnsparser.hh
+++ b/pdns/dnsparser.hh
@@ -338,6 +338,16 @@ public:
     return s.str();
   }
 
+  [[nodiscard]] std::string toString() const
+  {
+    std::string ret(d_name.toLogString());
+    ret += '|';
+    ret += QType(d_type).toString();
+    ret += '|';
+    ret += getContent()->getZoneRepresentation();
+    return ret;
+  }
+
   void setContent(const std::shared_ptr<const DNSRecordContent>& content)
   {
     d_content = content;

diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc
index 6aa0d11496e8a51a3a30cb9197f9f7d17db49695..1bc9047bcdeb34340f76d6820d31faaecbe966cf 100644 (file)
--- a/pdns/recursordist/syncres.cc
+++ b/pdns/recursordist/syncres.cc
@@ -409,7 +409,6 @@ SuffixMatchNode SyncRes::s_ednsdomains;
 EDNSSubnetOpts SyncRes::s_ecsScopeZero;
 string SyncRes::s_serverID;
 SyncRes::LogMode SyncRes::s_lm;
-const std::unordered_set<QType> SyncRes::s_redirectionQTypes = {QType::CNAME, QType::DNAME};
 static LockGuarded<fails_t<ComboAddress>> s_fails;
 static LockGuarded<fails_t<DNSName>> s_nonresolving;
 
@@ -4207,6 +4206,11 @@ static void allowAdditionalEntry(std::unordered_set<DNSName>& allowedAdditionals
   }
 }
 
+static bool isRedirection(QType qtype)
+{
+  return qtype == QType::CNAME || qtype == QType::DNAME;
+}
+
 void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DNSName& qname, const QType qtype, const DNSName& auth, bool wasForwarded, bool rdQuery)
 {
   const bool wasForwardRecurse = wasForwarded && rdQuery;
@@ -4241,7 +4245,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
 
     // Disallow any name not part of qname requested
     if (!rec->d_name.isPartOf(auth)) {
-      LOG(prefix << qname << ": Removing record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the " << (int)rec->d_place << " section received from " << auth << endl);
+      LOG(prefix << qname << ": Removing record '" << rec->toString() << "' in the " << DNSResourceRecord::placeString(rec->d_place) << " section received from " << auth << endl);
       rec = lwr.d_records.erase(rec);
       continue;
     }
@@ -4252,7 +4256,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
       /* for now we allow a CNAME for the exact qname in ANSWER with AA=0, because Amazon DNS servers
          are sending such responses */
       if (rec->d_type != QType::CNAME || qname != rec->d_name) {
-        LOG(prefix << qname << ": Removing record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the answer section without the AA bit set received from " << auth << endl);
+        LOG(prefix << qname << ": Removing record '" << rec->toString() << "' in the ANSWER section without the AA bit set received from " << auth << endl);
         rec = lwr.d_records.erase(rec);
         continue;
       }
@@ -4260,15 +4264,15 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
 
     // Disallow QType DNAME in non-answer section or containing an answer that is not a parent of or equal to the question name
     if (rec->d_type == QType::DNAME && (rec->d_place != DNSResourceRecord::ANSWER || !qname.isPartOf(rec->d_name))) {
-      LOG(prefix << qname << ": Removing invalid DNAME record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the " << (int)rec->d_place << " section received from " << auth << endl);
+      LOG(prefix << qname << ": Removing invalid DNAME record '" << rec->toString() << "' in the " << DNSResourceRecord::placeString(rec->d_place) << " section received from " << auth << endl);
       rec = lwr.d_records.erase(rec);
       continue;
     }
 
     // Disallow answer records not anwering the QType requested. ANY, CNAME, DNAME, RRSIG complicate matters here
     // Question: is the SOA check OK?  See RFC2181 section 7.1
-    if (rec->d_place == DNSResourceRecord::ANSWER && (qtype != QType::ANY && rec->d_type != qtype.getCode() && s_redirectionQTypes.count(rec->d_type) == 0 && rec->d_type != QType::SOA && rec->d_type != QType::RRSIG)) {
-      LOG(prefix << qname << ": Removing irrelevant record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the ANSWER section received from " << auth << endl);
+    if (rec->d_place == DNSResourceRecord::ANSWER && (qtype != QType::ANY && rec->d_type != qtype.getCode() && !isRedirection(rec->d_type) && rec->d_type != QType::SOA && rec->d_type != QType::RRSIG)) {
+      LOG(prefix << qname << ": Removing irrelevant record '" << rec->toString() << "' in the ANSWER section received from " << auth << endl);
       rec = lwr.d_records.erase(rec);
       continue;
     }
@@ -4284,7 +4288,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
     /* dealing with the records in authority */
     // Only allow NS, DS, SOA, RRSIG, NSEC, NSEC3 in AUTHORITY section
     if (rec->d_place == DNSResourceRecord::AUTHORITY && rec->d_type != QType::NS && rec->d_type != QType::DS && rec->d_type != QType::SOA && rec->d_type != QType::RRSIG && rec->d_type != QType::NSEC && rec->d_type != QType::NSEC3) {
-      LOG(prefix << qname << ": Removing irrelevant record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the AUTHORITY section received from " << auth << endl);
+      LOG(prefix << qname << ": Removing irrelevant record '" << rec->toString() << "' in the AUTHORITY section received from " << auth << endl);
       rec = lwr.d_records.erase(rec);
       continue;
     }
@@ -4292,14 +4296,14 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
     if (rec->d_place == DNSResourceRecord::AUTHORITY && rec->d_type == QType::SOA) {
       // Disallow a SOA record with a name that is not a parent of or equal to the name we asked
       if (!qname.isPartOf(rec->d_name)) {
-        LOG(prefix << qname << ": Removing irrelevant SOA record '" << rec->d_name << "|" << rec->getContent()->getZoneRepresentation() << "' in the AUTHORITY section received from " << auth << endl);
+        LOG(prefix << qname << ": Removing irrelevant SOA record '" << rec->toString() << "' in the AUTHORITY section received from " << auth << endl);
         rec = lwr.d_records.erase(rec);
         continue;
       }
 
       // Disallow SOA without AA bit (except for forward with RD=1)
       if (!(lwr.d_aabit || wasForwardRecurse)) {
-        LOG(prefix << qname << ": Removing irrelevant record (AA not set) '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the AUTHORITY section received from " << auth << endl);
+        LOG(prefix << qname << ": Removing irrelevant record (AA not set) '" << rec->toString() << "' in the AUTHORITY section received from " << auth << endl);
         rec = lwr.d_records.erase(rec);
         continue;
       }
@@ -4322,7 +4326,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
        * because they are somewhat easy to insert into a large, fragmented UDP response
        * for an off-path attacker by injecting spoofed UDP fragments. So do not add these to allowedAdditionals.
        */
-      LOG(prefix << qname << ": Removing NS record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the " << (int)rec->d_place << " section of a " << (isNXDomain ? "NXD" : "NXQTYPE") << " response received from " << auth << endl);
+      LOG(prefix << qname << ": Removing NS record '" << rec->toString() << "' in the AUTHORITY section of a " << (isNXDomain ? "NXD" : "NXQTYPE") << " response received from " << auth << endl);
       rec = lwr.d_records.erase(rec);
       continue;
     }
@@ -4332,7 +4336,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
        * We don't want to pick up root NS records in AUTHORITY and their associated ADDITIONAL sections of random queries.
        * So don't add them to allowedAdditionals.
        */
-      LOG(prefix << qname << ": Removing NS record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the " << (int)rec->d_place << " section of a response received from " << auth << endl);
+      LOG(prefix << qname << ": Removing NS record '" << rec->toString() << "' in the AUTHORITY section of a response received from " << auth << endl);
       rec = lwr.d_records.erase(rec);
       continue;
     }
@@ -4343,13 +4347,13 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
 
     /* dealing with the records in additional */
     if (rec->d_place == DNSResourceRecord::ADDITIONAL && rec->d_type != QType::A && rec->d_type != QType::AAAA && rec->d_type != QType::RRSIG) {
-      LOG(prefix << qname << ": Removing irrelevant record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the ADDITIONAL section received from " << auth << endl);
+      LOG(prefix << qname << ": Removing irrelevant record '" << rec->toString() << "' in the ADDITIONAL section received from " << auth << endl);
       rec = lwr.d_records.erase(rec);
       continue;
     }
 
     if (rec->d_place == DNSResourceRecord::ADDITIONAL && allowedAdditionals.count(rec->d_name) == 0) {
-      LOG(prefix << qname << ": Removing irrelevant additional record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the ADDITIONAL section received from " << auth << endl);
+      LOG(prefix << qname << ": Removing irrelevant record '" << rec->toString() << "' in the ADDITIONAL section received from " << auth << endl);
       rec = lwr.d_records.erase(rec);
       continue;
     }
@@ -4999,8 +5003,8 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
       negIndicHasSignatures = !negEntry.authoritySOA.signatures.empty() || !negEntry.DNSSECRecords.signatures.empty();
       negindic = true;
     }
-    else if (rec.d_place == DNSResourceRecord::ANSWER && s_redirectionQTypes.count(rec.d_type) > 0 && // CNAME or DNAME answer
-             s_redirectionQTypes.count(qtype.getCode()) == 0) { // But not in response to a CNAME or DNAME query
+    else if (rec.d_place == DNSResourceRecord::ANSWER && isRedirection(rec.d_type) && // CNAME or DNAME answer
+             !isRedirection(qtype.getCode())) { // But not in response to a CNAME or DNAME query
       if (rec.d_type == QType::CNAME && rec.d_name == qname) {
         if (!dnameOwner.empty()) { // We synthesize ourselves
           continue;

diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh
index bd1d02768ea581b000bd7c2ff113db4c459547b4..a1e0e0e3e905806f936ab6be2897aa53510bf191 100644 (file)
--- a/pdns/recursordist/syncres.hh
+++ b/pdns/recursordist/syncres.hh
@@ -603,7 +603,6 @@ private:
   static EDNSSubnetOpts s_ecsScopeZero;
   static LogMode s_lm;
   static std::unique_ptr<NetmaskGroup> s_dontQuery;
-  const static std::unordered_set<QType> s_redirectionQTypes;
 
   struct GetBestNSAnswer
   {