From b005f9823c279f8f3bc40ed8a03dea62ca2d2df9 Mon Sep 17 00:00:00 2001
From: Miod Vallat <miod.vallat@powerdns.com>
Date: Mon, 17 Feb 2025 15:14:30 +0100
Subject: [PATCH] Prevent duplicate records in pdnsutil add-record.

When adding records with pdnsutil, the combination of the existing and
to-be-added records will now be dedup'ed.

Fixes: #4727
---
 pdns/pdnsutil.cc | 104 +++++++++++++++++++++++++++++++----------------
 1 file changed, 68 insertions(+), 36 deletions(-)

diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc
index e4da8c8ee2..b12203ebeb 100644
--- a/pdns/pdnsutil.cc
+++ b/pdns/pdnsutil.cc
@@ -1635,7 +1635,7 @@ static int createZone(const DNSName &zone, const DNSName& nsname) {
 }
 
 // add-record ZONE name type [ttl] "content" ["content"]
-static int addOrReplaceRecord(bool addOrReplace, const vector<string>& cmds) {
+static int addOrReplaceRecord(bool isAdd, const vector<string>& cmds) {
   DNSResourceRecord rr;
   vector<DNSResourceRecord> newrrs;
   DNSName zone(cmds.at(1));
@@ -1645,9 +1645,6 @@ static int addOrReplaceRecord(bool addOrReplace, const vector<string>& cmds) {
   else
     name = DNSName(cmds.at(2)) + zone;
 
-  rr.qtype = DNSRecordContent::TypeToNumber(cmds.at(3));
-  rr.ttl = ::arg().asNum("default-ttl");
-
   UtilBackend B; //NOLINT(readability-identifier-length)
   DomainInfo di;
   if(!B.getDomainInfo(zone, di)) {
@@ -1657,64 +1654,99 @@ static int addOrReplaceRecord(bool addOrReplace, const vector<string>& cmds) {
   if (di.isSecondaryType() && !g_force) {
     throw PDNSException("Operation on a secondary zone is not allowed unless --force");
   }
+
+  rr.qtype = DNSRecordContent::TypeToNumber(cmds.at(3));
+  rr.ttl = ::arg().asNum("default-ttl");
   rr.auth = true;
   rr.domain_id = di.id;
   rr.qname = name;
   DNSResourceRecord oldrr;
 
-  di.backend->startTransaction(zone, -1);
-
-  if(addOrReplace) { // the 'add' case
-    di.backend->lookup(rr.qtype, rr.qname, di.id);
-
-    while(di.backend->get(oldrr))
-      newrrs.push_back(oldrr);
-  }
-
   unsigned int contentStart = 4;
   if(cmds.size() > 5) {
-    rr.ttl = atoi(cmds.at(4).c_str());
-    if (std::to_string(rr.ttl) == cmds.at(4)) {
+    uint32_t ttl = atoi(cmds.at(4).c_str());
+    if (std::to_string(ttl) == cmds.at(4)) {
+      rr.ttl = ttl;
       contentStart++;
     }
-    else {
-      rr.ttl = ::arg().asNum("default-ttl");
-    }
   }
 
-  di.backend->lookup(QType(QType::ANY), rr.qname, di.id);
-  bool found=false;
-  if(rr.qtype.getCode() == QType::CNAME) { // this will save us SO many questions
+  di.backend->startTransaction(zone, -1);
 
-    while(di.backend->get(oldrr)) {
-      if(addOrReplace || oldrr.qtype.getCode() != QType::CNAME) // the replace case is ok if we replace one CNAME by the other
-        found=true;
+  // Enforce that CNAME records can not be mixed with any other.
+  // If we add a CNAME: there should be no existing records except for one
+  // possible previous CNAME, which will get replaced.
+  // If we add something else: there should be no existing CNAME record.
+  bool reject{false};
+  if (rr.qtype.getCode() == QType::CNAME) {
+    di.backend->lookup(QType(QType::ANY), rr.qname, static_cast<int>(di.id));
+    while (di.backend->get(oldrr)) {
+      if (oldrr.qtype.getCode() == QType::CNAME) {
+        if (not isAdd) {
+          // Replacement operation: ok to replace CNAME with another
+          continue;
+        }
+      }
+      reject = true;
     }
-    if(found) {
-      cerr<<"Attempting to add CNAME to "<<rr.qname<<" which already had existing records"<<endl;
+    if (reject) {
+      cerr<<"Attempting to add CNAME to "<<rr.qname<<" which already has existing records"<<endl;
       return EXIT_FAILURE;
     }
   }
   else {
-    while(di.backend->get(oldrr)) {
-      if(oldrr.qtype.getCode() == QType::CNAME)
-        found=true;
-    }
-    if(found) {
-      cerr<<"Attempting to add record to "<<rr.qname<<" which already had a CNAME record"<<endl;
+    di.backend->lookup(QType(QType::CNAME), rr.qname, static_cast<int>(di.id));
+    // TODO: It would be nice to have a way to complete a lookup and release its
+    // resources without having to exhaust its results - here one successful
+    // get() is all we need to decide to reject the operation. I'm looking at
+    // you, lmdb backend.
+    while (di.backend->get(oldrr)) {
+      reject = true;
+    }
+    if (reject) {
+      cerr<<"Attempting to add record to "<<rr.qname<<" which already has a CNAME record"<<endl;
       return EXIT_FAILURE;
     }
   }
 
-  if(!addOrReplace) {
-    cout<<"Current records for "<<rr.qname<<" IN "<<rr.qtype.toString()<<" will be replaced"<<endl;
-  }
+  // Synthesize the new records.
   for(auto i = contentStart ; i < cmds.size() ; ++i) {
     rr.content = DNSRecordContent::make(rr.qtype.getCode(), QClass::IN, cmds.at(i))->getZoneRepresentation(true);
 
-    newrrs.push_back(rr);
+    bool skip{false};
+    for (const auto &record: newrrs) {
+      if (rr.content == record.content) {
+        cout<<R"(Ignoring duplicate record content ")"<<rr.content<<R"(")"<<endl;
+        skip = true;
+        break;
+      }
+    }
+    if (!skip) {
+      newrrs.push_back(rr);
+    }
   }
 
+  if(isAdd) {
+    // the 'add' case; preserve existing records...
+    size_t newRecordsCount = newrrs.size();
+    di.backend->lookup(rr.qtype, rr.qname, static_cast<int>(di.id));
+    while(di.backend->get(oldrr)) {
+      // ...unless their contents are identical to the records we are adding.
+      bool skip{false};
+      for (size_t idx = 0; idx < newRecordsCount; ++idx) {
+        if (newrrs.at(idx).content == oldrr.content) {
+          skip = true;
+          break;
+        }
+      }
+      if (!skip) {
+        newrrs.push_back(oldrr);
+      }
+    }
+  }
+  else {
+    cout<<"All existing records for "<<rr.qname<<" IN "<<rr.qtype.toString()<<" will be replaced"<<endl;
+  }
 
   if(!di.backend->replaceRRSet(di.id, name, rr.qtype, newrrs)) {
     cerr<<"backend did not accept the new RRset, aborting"<<endl;
-- 
2.47.2