From 20a7173c5743fed5f530ed0a023a55a58e633c52 Mon Sep 17 00:00:00 2001 From: Miod Vallat Date: Mon, 17 Feb 2025 15:14:30 +0100 Subject: [PATCH 1/4] 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 ccca24bb6ab7..2fc19f9aeaed 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -1592,7 +1592,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& cmds) { +static int addOrReplaceRecord(bool isAdd, const vector& cmds) { DNSResourceRecord rr; vector newrrs; DNSName zone(cmds.at(1)); @@ -1602,73 +1602,105 @@ static int addOrReplaceRecord(bool addOrReplace, const vector& 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)) { cerr << "Zone '" << zone << "' does not exist" << endl; return EXIT_FAILURE; } + + 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(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 "<get(oldrr)) { - if(oldrr.qtype.getCode() == QType::CNAME) - found=true; - } - if(found) { - cerr<<"Attempting to add record to "<lookup(QType(QType::CNAME), rr.qname, static_cast(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 "<getZoneRepresentation(true); - newrrs.push_back(rr); + bool skip{false}; + for (const auto &record: newrrs) { + if (rr.content == record.content) { + cout<lookup(rr.qtype, rr.qname, static_cast(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 "<replaceRRSet(di.id, name, rr.qtype, newrrs)) { cerr<<"backend did not accept the new RRset, aborting"< Date: Mon, 17 Feb 2025 15:10:38 +0100 Subject: [PATCH 2/4] Make sure a backend envvar is always provided to tests. --- regression-tests/backends/bind-master | 2 ++ regression-tests/backends/geoip-master | 1 + regression-tests/backends/gmysql-slave | 1 + regression-tests/backends/godbc_mssql-slave | 1 + regression-tests/backends/gpgsql-slave | 1 + regression-tests/backends/gsqlite3-slave | 1 + regression-tests/backends/ldap-master | 1 + regression-tests/backends/lmdb-master | 1 + regression-tests/backends/lmdb-slave | 1 + regression-tests/backends/lua2-master | 1 + regression-tests/backends/remote-master | 1 + 11 files changed, 12 insertions(+) diff --git a/regression-tests/backends/bind-master b/regression-tests/backends/bind-master index b91b83fc56c4..cd6ab907ad37 100644 --- a/regression-tests/backends/bind-master +++ b/regression-tests/backends/bind-master @@ -1,5 +1,6 @@ case $context in bind) + backend=bind cat > pdns-bind.conf << __EOF__ module-dir=$PDNS_BUILD_PATH/modules launch=bind @@ -17,6 +18,7 @@ __EOF__ bind-dnssec | bind-dnssec-nsec3 | bind-hybrid-nsec3 | bind-dnssec-nsec3-optout | bind-dnssec-nsec3-narrow) rm -f dnssec.sqlite3 + backend=bind cat > pdns-bind.conf << __EOF__ module-dir=$PDNS_BUILD_PATH/modules launch=bind diff --git a/regression-tests/backends/geoip-master b/regression-tests/backends/geoip-master index c3e3ef175796..93f8acecaa71 100644 --- a/regression-tests/backends/geoip-master +++ b/regression-tests/backends/geoip-master @@ -116,6 +116,7 @@ Rcode: 0 (No Error), RD: 0, QR: 1, TC: 0, AA: 1, opcode: 0 Reply to question for qname='continent.geo.example.com.', qtype=TXT EOF # generate pdns.conf for pdnsutil + backend=geoip cat > pdns-geoip.conf < pdns-gmysql2.conf << __EOF__ module-dir=./modules launch=gmysql diff --git a/regression-tests/backends/godbc_mssql-slave b/regression-tests/backends/godbc_mssql-slave index 1c58ac2337b7..4f57e9508494 100644 --- a/regression-tests/backends/godbc_mssql-slave +++ b/regression-tests/backends/godbc_mssql-slave @@ -5,6 +5,7 @@ echo "drop table $table" | $ISQL -b done $ISQL < ../modules/godbcbackend/schema.mssql.sql + backend=godbc2 cat > pdns-godbc2.conf << __EOF__ module-dir=./modules launch=godbc diff --git a/regression-tests/backends/gpgsql-slave b/regression-tests/backends/gpgsql-slave index e1b06f057279..96da67174eee 100644 --- a/regression-tests/backends/gpgsql-slave +++ b/regression-tests/backends/gpgsql-slave @@ -6,6 +6,7 @@ createdb --user="$GPGSQL2USER" "$GPGSQL2DB" || echo ignoring createdb failure psql --user="$GPGSQL2USER" "$GPGSQL2DB" < ../modules/gpgsqlbackend/schema.pgsql.sql + backend=gpgsql2 cat > pdns-gpgsql2.conf << __EOF__ module-dir=./modules launch=gpgsql diff --git a/regression-tests/backends/gsqlite3-slave b/regression-tests/backends/gsqlite3-slave index 6dcc71376e89..5b094a54c1e0 100644 --- a/regression-tests/backends/gsqlite3-slave +++ b/regression-tests/backends/gsqlite3-slave @@ -2,6 +2,7 @@ rm -f pdns.sqlite32 sqlite3 pdns.sqlite32 < ../modules/gsqlite3backend/schema.sqlite3.sql + backend=gsqlite32 cat > pdns-gsqlite32.conf << __EOF__ module-dir=./modules launch=gsqlite3 diff --git a/regression-tests/backends/ldap-master b/regression-tests/backends/ldap-master index cbb0d62c9147..f96e5843c077 100644 --- a/regression-tests/backends/ldap-master +++ b/regression-tests/backends/ldap-master @@ -16,6 +16,7 @@ __EOF__ $ZONE2LDAP --dnsttl=yes --basedn=$LDAPBASEDN --layout=$layout --named-conf=named.conf | ldapmodify -D $LDAPUSER -w $LDAPPASSWD -H $LDAPHOST -c > /dev/null || true + backend=ldap cat > pdns-ldap.conf << __EOF__ module-dir=$PDNS_BUILD_PATH/modules launch=ldap diff --git a/regression-tests/backends/lmdb-master b/regression-tests/backends/lmdb-master index cdeb418df05a..a9836b96b345 100644 --- a/regression-tests/backends/lmdb-master +++ b/regression-tests/backends/lmdb-master @@ -1,5 +1,6 @@ case $context in lmdb | lmdb-nodnssec | lmdb-nsec3 | lmdb-nsec3-optout | lmdb-nsec3-narrow) + backend=lmdb cat > pdns-lmdb.conf << __EOF__ module-dir=$PDNS_BUILD_PATH/modules launch=lmdb diff --git a/regression-tests/backends/lmdb-slave b/regression-tests/backends/lmdb-slave index 62057691b07a..cbbbcc0c84a0 100644 --- a/regression-tests/backends/lmdb-slave +++ b/regression-tests/backends/lmdb-slave @@ -1,4 +1,5 @@ context=${context}-presigned-lmdb + backend=lmdb2 cat > pdns-lmdb2.conf << __EOF__ module-dir=./modules launch=lmdb diff --git a/regression-tests/backends/lua2-master b/regression-tests/backends/lua2-master index f636250eb19c..b6d805bea748 100644 --- a/regression-tests/backends/lua2-master +++ b/regression-tests/backends/lua2-master @@ -22,6 +22,7 @@ case $context in fi # generate pdns.conf for pdnsutil + backend=lua2 cat > pdns-lua2.conf < pdns-remote.conf < Date: Mon, 17 Feb 2025 15:16:15 +0100 Subject: [PATCH 3/4] Add simple tests for pdnsutil add-record behaviour. --- .../tests/pdnsutil-zone-handling/command | 39 +++++++++++++++++++ .../tests/pdnsutil-zone-handling/description | 1 + .../pdnsutil-zone-handling/expected_result | 18 +++++++++ .../tests/pdnsutil-zone-handling/skip.bind | 0 .../tests/pdnsutil-zone-handling/skip.geoip | 0 .../pdnsutil-zone-handling/skip.ldap-simple | 0 .../pdnsutil-zone-handling/skip.ldap-strict | 0 .../pdnsutil-zone-handling/skip.ldap-tree | 0 .../tests/pdnsutil-zone-handling/skip.tinydns | 0 9 files changed, 58 insertions(+) create mode 100755 regression-tests/tests/pdnsutil-zone-handling/command create mode 100644 regression-tests/tests/pdnsutil-zone-handling/description create mode 100644 regression-tests/tests/pdnsutil-zone-handling/expected_result create mode 100644 regression-tests/tests/pdnsutil-zone-handling/skip.bind create mode 100644 regression-tests/tests/pdnsutil-zone-handling/skip.geoip create mode 100644 regression-tests/tests/pdnsutil-zone-handling/skip.ldap-simple create mode 100644 regression-tests/tests/pdnsutil-zone-handling/skip.ldap-strict create mode 100644 regression-tests/tests/pdnsutil-zone-handling/skip.ldap-tree create mode 100644 regression-tests/tests/pdnsutil-zone-handling/skip.tinydns diff --git a/regression-tests/tests/pdnsutil-zone-handling/command b/regression-tests/tests/pdnsutil-zone-handling/command new file mode 100755 index 000000000000..bd1b1545f844 --- /dev/null +++ b/regression-tests/tests/pdnsutil-zone-handling/command @@ -0,0 +1,39 @@ +#!/bin/sh + +# The first invocation of pdnsutil is redirected to /dev/null to hide the +# "local files have been created" message if using lmdb as backend. +# All other pdnsutil invocation have the Ueberbackend destructor messages from +# --enable-verbose-logging removed. +# Invocations which will output zone contents are passed through sort(1), as +# the order of entries for records having multiple entries is backend-specific +# and not guaranteed to be in any particular order. +$PDNSUTIL --config-dir=. --config-name=$backend \ + create-zone bug.less \ + > /dev/null 2>&1 +$PDNSUTIL --config-dir=. --config-name=$backend \ + add-record bug.less cname CNAME host \ + 2>&1 | grep -v Ueber +$PDNSUTIL --config-dir=. --config-name=$backend \ + add-record bug.less host A 127.0.0.1 \ + 2>&1 | grep -v Ueber +# Duplicate records should be omitted +$PDNSUTIL --config-dir=. --config-name=$backend \ + add-record bug.less host2 A 127.0.0.2 127.0.0.2 \ + 2>&1 | sort | grep -v Ueber +# Can't add non-CNAME record to a CNAME record +$PDNSUTIL --config-dir=. --config-name=$backend \ + add-record bug.less cname A 127.0.0.1 \ + 2>&1 | grep -v Ueber +# Can't add CNAME record if other records exist +$PDNSUTIL --config-dir=. --config-name=$backend \ + add-record bug.less host CNAME host2 \ + 2>&1 | grep -v Ueber +# Adding existing record should ignore duplicates +$PDNSUTIL --config-dir=. --config-name=$backend \ + add-record bug.less host2 A 127.0.0.2 127.0.0.3 \ + 2>&1 | sort | grep -v Ueber + +# Display zone contents for final verification +$PDNSUTIL --config-dir=. --config-name=$backend \ + list-zone bug.less \ + 2>&1 | sort | grep -v Ueber diff --git a/regression-tests/tests/pdnsutil-zone-handling/description b/regression-tests/tests/pdnsutil-zone-handling/description new file mode 100644 index 000000000000..2e6cbe81b716 --- /dev/null +++ b/regression-tests/tests/pdnsutil-zone-handling/description @@ -0,0 +1 @@ +This test checks various pdnsutil behaviour when editing zone contents. diff --git a/regression-tests/tests/pdnsutil-zone-handling/expected_result b/regression-tests/tests/pdnsutil-zone-handling/expected_result new file mode 100644 index 000000000000..7e188dfe0269 --- /dev/null +++ b/regression-tests/tests/pdnsutil-zone-handling/expected_result @@ -0,0 +1,18 @@ +New rrset: +cname.bug.less. 3600 IN CNAME host +New rrset: +host.bug.less. 3600 IN A 127.0.0.1 +Ignoring duplicate record content "127.0.0.2" +New rrset: +host2.bug.less. 3600 IN A 127.0.0.2 +Attempting to add record to cname.bug.less which already has a CNAME record +Attempting to add CNAME to host.bug.less which already has existing records +New rrset: +host2.bug.less. 3600 IN A 127.0.0.2 +host2.bug.less. 3600 IN A 127.0.0.3 +$ORIGIN . +bug.less 3600 IN SOA a.misconfigured.dns.server.invalid hostmaster.bug.less 0 10800 3600 604800 3600 +cname.bug.less 3600 IN CNAME host. +host.bug.less 3600 IN A 127.0.0.1 +host2.bug.less 3600 IN A 127.0.0.2 +host2.bug.less 3600 IN A 127.0.0.3 diff --git a/regression-tests/tests/pdnsutil-zone-handling/skip.bind b/regression-tests/tests/pdnsutil-zone-handling/skip.bind new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/regression-tests/tests/pdnsutil-zone-handling/skip.geoip b/regression-tests/tests/pdnsutil-zone-handling/skip.geoip new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/regression-tests/tests/pdnsutil-zone-handling/skip.ldap-simple b/regression-tests/tests/pdnsutil-zone-handling/skip.ldap-simple new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/regression-tests/tests/pdnsutil-zone-handling/skip.ldap-strict b/regression-tests/tests/pdnsutil-zone-handling/skip.ldap-strict new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/regression-tests/tests/pdnsutil-zone-handling/skip.ldap-tree b/regression-tests/tests/pdnsutil-zone-handling/skip.ldap-tree new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/regression-tests/tests/pdnsutil-zone-handling/skip.tinydns b/regression-tests/tests/pdnsutil-zone-handling/skip.tinydns new file mode 100644 index 000000000000..e69de29bb2d1 From 26fbfe0a315d30e5e74c08685132a87261470f1a Mon Sep 17 00:00:00 2001 From: Miod Vallat Date: Wed, 19 Feb 2025 12:16:32 +0100 Subject: [PATCH 4/4] Preserve record order in pdnsutil add-record. --- pdns/pdnsutil.cc | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index 2fc19f9aeaed..cb441a499185 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -1681,22 +1681,21 @@ static int addOrReplaceRecord(bool isAdd, const vector& cmds) { } if(isAdd) { - // the 'add' case; preserve existing records... - size_t newRecordsCount = newrrs.size(); + // the 'add' case; preserve existing records, making sure to discard + // would-be new records which contents are identical to the existing ones. + vector oldrrs; di.backend->lookup(rr.qtype, rr.qname, static_cast(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; + while (di.backend->get(oldrr)) { + oldrrs.push_back(oldrr); + for (auto iter = newrrs.begin(); iter != newrrs.end(); ++iter) { + if (iter->content == oldrr.content) { + newrrs.erase(iter); break; } } - if (!skip) { - newrrs.push_back(oldrr); - } } + oldrrs.insert(oldrrs.end(), newrrs.begin(), newrrs.end()); + newrrs = std::move(oldrrs); } else { cout<<"All existing records for "<