AXFRDDNS: convert to codeberg.org/miekg/dns (dnsv2) - #4658
Merged
TomOnTime merged 2 commits intoAug 2, 2026
Conversation
Converts providers/axfrddns from github.com/miekg/dns to
codeberg.org/miekg/dns. providers/mythicbeasts still imports v1, as do
models/{makers,quotes,record,t_loc,t_svcb,target}.go,
pkg/dnsrr/{dnsrr,v1v2}.go and pkg/normalize/validate.go.
DNSControl#4529 had already converted the record-conversion side; what was left
was the network layer: dnsv1.Conn, dnsv1.Transfer, dnsv1.Client and the
RFC 2136 message builders.
The dnsv2 network API differs from v1 in three ways that shaped this
diff:
* Zone transfer. dnsv1.Transfer{Conn: ...} becomes
Client.TransferInWithConn(ctx, msg, conn), so getAxfrConnection now
returns the raw net.Conn it already dialled and the TLS/unix/tcp
branches are unchanged. Envelope.RR became Envelope.Answer.
* TSIG. There is no TsigSecret map; a Client carries a TSIGSigner.
Key.secret therefore holds the decoded secret rather than its base64
form (readKey already validated the encoding, it just discarded the
result), and Key.signer() returns dnsv2.HmacTSIG, or md5Provider for
HMAC-MD5, which dnsv2.HmacTSIG does not implement. md5Provider was
already carrying HMAC-MD5 for the same reason under v1 and is
reshaped to the TSIGSigner interface (Sign/Verify/Key).
Client.Exchange does not sign or verify, unlike v1's, so the update
path calls dnsv2.TSIGSign before the exchange and dnsv2.TSIGVerify on
the reply. As in v1 (client.go:267, which verifies only when the
reply carries a TSIG), an unsigned reply is not rejected; both
directions of that gate are pinned by TestBuildCorrection.
* RFC 2136 message builders. dnsv2 has no Msg.SetUpdate/Insert/Remove/
RemoveName, so newUpdate, insert, remove and removeName reimplement
them locally against RFC 2136 sections 2.3, 2.5.1, 2.5.3 and 2.5.4.
Verified by building the same update both ways and packing both: for
A, AAAA, CNAME, MX, NS, TXT, SRV, CAA, PTR, NAPTR and TLSA the
v1-built and v2-built messages are identical byte for byte in all
three operations, and a message holding all eleven is identical in
length and decodes identically under the v1 parser, differing only in
which of two equivalent name-compression offsets is used.
Three behaviour changes are deliberate:
* FetchZoneRecords closes the connection itself once the closing SOA
has arrived. dnsv1's inAxfr leaves its first iteration before it
looks for that SOA, so it returns as soon as the SOA arrives even
when the whole zone fits in a single message. dnsv2's transferInAXFR
only makes that check once options.TimersOnly is set, and it sets it
at the end of the first iteration, so a single-message AXFR loops
once more and blocks reading until the server closes the connection
or Client.ReadTimeout (dnsTimeout, 30s) expires. Measured against
BIND 9.20 serving example.com over TCP with allow-transfer any: 4 ms
under v1, 30.0 s under a straight translation, both returning the
same 8 records. BIND's transfer-message-size defaults to 20480
bytes, so a typical AXFRDDNS zone arrives in one message and this
would have cost 30 s on every preview and every push. Closing the
connection on the SOA restores the v1 timing (4 ms, 8 records) and
leaves multi-message transfers untouched: a 1204-record zone
transfers in two messages in 17 ms either way. The remaining
envelopes are drained so that the library's reader goroutine is never
left blocked on the channel. Pinned by
TestFetchZoneRecordsReturnsOnTheClosingSOA, whose nameserver answers
in a single message and then holds the connection open.
* dnsv1.Transfer reported a read error on the envelope channel; dnsv2
closes the channel silently, so a connection dropped mid-AXFR would
be accepted as a complete zone and every missing record would be
diffed as a deletion. FetchZoneRecords now requires the closing SOA
of RFC 5936 section 2.2 before trusting the answer, which restores
the v1 failure. The guard counts records rather than messages, since
a server may answer the opening SOA in a message of its own;
TestFetchZoneRecordsDoesNotStopOnTheOpeningSOA pins that boundary.
* A refused transfer is reported as
"... refused to transfer the zone example.com: dns: bad rcode: NOTAUTH"
rather than "...: NOT AUTH (9)". The old text came from matching
msg.Error.Error() against the literal "dns: bad xfr rcode: 9", which
dnsv2 no longer produces. Pinned by
TestFetchZoneRecordsReportsRefusedTransfer.
Adds providers/axfrddns/axfrddnsProvider_test.go, the provider's first
tests: TSIG key parsing and algorithm mapping, AXFR against an
in-process nameserver unauthenticated and under HMAC-SHA256/SHA512/MD5
with the nameserver asserting that the request carried a TSIG,
rejection of a wrong TSIG secret, of a truncated transfer and of a
reply signed with the wrong key, acceptance of an unsigned reply, a
zone whose opening SOA arrives in a message of its own, return on the
closing SOA while the nameserver holds the connection open, the
reported text of a refused transfer, the RFC 2136 encoding of each
update operation, the splitting of an update that exceeds the 16 KiB
chunk size over several messages, and a DDNS update pushed to an
in-process nameserver that verifies the TSIG and replies signed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
Hey @hnrgrgr! Are you ok with these changes? They pass all tests. |
shuvamk
added a commit
to shuvamk/dnscontrol
that referenced
this pull request
Aug 2, 2026
…arness
Each file adapts one provider's record conversion functions to CheckToRC /
CheckToNative and names the recorded data after the provider and the function
under test. All 22 tests skip today ("<name> has no recorded data yet"), so
dropping a -record capture into providers/<x>/testdata/ and running
"go test ./providers/<x>/ -update" produces the goldens with no further code.
The adapters are per-provider because the signatures are: cloudflare's
nativeToRecord is a method; ns1's convert, route53's and azure's
nativeToRecords and gandiv5's nativeToRecords already return several records;
gcloud's nativeToRecord takes one rdata string out of a set, so the adapter
loops over set.Rrdatas the way getZoneSets does; digitalocean's and netlify's
toReq return no error; cnr's createRecordString is a method that also needs the
domain; vercel's toVercelCreateRequest needs the domain; luadns's
recordsToNative takes a slice but maps each record independently, so a
one-element slice exercises it faithfully.
Providers of the 21 that are not enrolled here, and why:
BIND ParseZoneContents takes a whole zone file, not a record
MYTHICBEASTS zoneFileToRecords takes an io.Reader
AXFRDDNS no separable converter (DNSControl#4658 rewrites it)
POWERDNS toRecordConfig needs the RRset's name/TTL/type, but
RecordConfig.Original holds only zones.Record{Content,Disabled};
buildRecordList is the only encoder and it takes a diff2.Change
NETNOD same on both counts: Original is
netnodPrimaryDNS.Record{Content,Disabled}
CNR and TRANSIP get the toNative half only: CNR sets Original to
deleteRecordString(rc), a string rather than the map[string]string toRC reads,
and TRANSIP never sets Original at all, so -record cannot produce a .json for
either.
The set-level encoders are left out because CheckToNative converts one record at
a time: gandiv5's recordsToNative, azuredns's and azureprivatedns's
recordToNativeDiff2, gcloud's mkRRSs and ns1's buildRecord. Each merges a whole
recordset into one native, so calling it with a one-element slice would not
exercise what it is for.
testDomain is "example.com" in every file; it has to match the zone the data
was recorded against.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
All tests pass. I'm merging. Sorry for the fast pace but this PR is blocking other work. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4358. Based on
release_candidate_v5, as you asked on the issue.#4529 had already done the record-conversion side, so what was left was the network layer:
dnsv1.Conn,dnsv1.Transfer,dnsv1.Clientand the RFC 2136 message builders.providers/axfrddnsno longer importsgithub.com/miekg/dns.Three places where the dnsv2 API forced a behaviour change
1. Single-message AXFR would have taken 30 seconds. This is the one worth reading.
transferInAXFRonly checks for the terminating SOA whenoptions.TimersOnlyis true, and it setsTimersOnlyat the end of the first loop iteration (transfer.go:174-186). v1'sinAxfrflipsfirstbefore theisSOALastcheck (xfr.go:117-131), so v1 returns on the closing SOA even when the whole zone arrives in one message. Under dnsv2 the read loop just keeps going until the server closes orReadTimeout(30 s) expires.Measured against BIND 9.20 in a container,
allow-transfer any:release_candidate_v5(dnsv1)transfer-message-sizedefaults to 20480 bytes, so a typical AXFRDDNS zone fits in one message and this would have cost 30 s on everypreviewand everypush— including the integration tests.FetchZoneRecordsnow latches once the accumulated answer ends in a SOA and closes the connection. It drains the remaining envelopes rather than breaking, becauseTransferInWithConnhands back an unbuffered channel — with a barebreak, a server that sends a spurious envelope after the closing SOA parks the library's reader goroutine forever onch <-(16 of 20 transfers, measured). Pinned byTestFetchZoneRecordsReturnsOnTheClosingSOA.The real fix is one line upstream in your fork —
transferInAXFRshould make the terminating-SOA check on the first iteration too, the way v1'sinAxfrdoes. Every other dnsv2 AXFR caller has this stall today. The provider-side latch stays correct either way, so this doesn't block on it, but you may prefer to fix it there and simplify here.2. A connection dropped mid-AXFR used to be an error and silently became a success.
dnsv1.Transferreported a read error on the envelope channel; dnsv2 closes the channel silently. Left alone, a truncated transfer would have been accepted as a complete zone and every record that didn't arrive would have been diffed as a deletion.FetchZoneRecordsnow requires the closing SOA of RFC 5936 §2.2 before trusting the answer, which restores the v1 failure. The guard counts records rather than messages, since a server may answer the opening SOA in a message of its own (v1 special-cased exactly that atxfr.go:114-118) —TestFetchZoneRecordsDoesNotStopOnTheOpeningSOApins that boundary.3. Refused-transfer error text changed. Now
refused to transfer the zone example.com: dns: bad rcode: NOTAUTHrather than...: NOT AUTH (9). The old text came from matchingmsg.Error.Error()against the literal"dns: bad xfr rcode: 9", which dnsv2 no longer produces.TSIG
There's no
TsigSecretmap in dnsv2 — aClientcarries aTSIGSigner. SoKey.secretnow holds the decoded secret rather than its base64 form (readKeyalready validated the encoding, it just threw the result away), andKey.signer()returnsdnsv2.HmacTSIG, ormd5Providerfor HMAC-MD5, whichHmacTSIGdoesn't implement.md5Providerwas already carrying HMAC-MD5 for the same reason under v1 and is reshaped to theTSIGSignerinterface.Client.Exchangeneither signs nor verifies, unlike v1's, so the update path callsdnsv2.TSIGSignbefore the exchange anddnsv2.TSIGVerifyon the reply. As in v1 (client.go:267, which verifies only when the reply carries a TSIG), an unsigned reply is still not rejected while a wrongly-signed one is — both directions are pinned byTestBuildCorrection.RFC 2136 builders
dnsv2 has no
Msg.SetUpdate/Insert/Remove/RemoveName, sonewUpdate,insert,removeandremoveNamereimplement them locally against RFC 2136 §2.3, §2.5.1, §2.5.3 and §2.5.4.Verified by construction rather than by inspection: the same update built both ways and packed, for A, AAAA, CNAME, MX, NS, TXT, SRV, CAA, PTR, NAPTR and TLSA across all three operations, is byte-for-byte identical. A message holding all eleven is identical in length and decodes identically under the v1 parser, differing only in which of two equivalent name-compression offsets it picks.
Tests
providers/axfrddns/axfrddnsProvider_test.gois new — the provider's first tests. TSIG key parsing and algorithm mapping; AXFR against an in-process nameserver unauthenticated and under HMAC-SHA256/SHA512/MD5; rejection of a wrong TSIG secret, a truncated transfer and a reply signed with the wrong key; acceptance of an unsigned reply; the closing-SOA return; a zone whose opening SOA arrives in a message of its own; the reported text of a refused transfer; the RFC 2136 encoding of each update operation; message chunking; and a DDNS update pushed to a server that verifies the TSIG and replies signed.Each behaviour above was checked by mutation — perturb the source, confirm the matching test goes red, restore, confirm green.
go test ./...71 ok / 0 fail.go vetclean,staticcheckclean,golangci-lintclean apart from the pre-existingpkg/js/hash.go:21finding that already carries a//lint:ignore.-raceand-count=5clean on the package.bin/generate-all.shleaves the tree empty.gofmt -lclean onproviders/axfrddns/. BIND integration test passes.Two things you may want to know
dnsv2.Msg.Pack()fails withdns packing: overflow RR headeron a zero-rdataANYRR whose owner name doesn't compress against the question name — e.g. zoneexample.com.with namex.net., or zoneexample.org.with namea.example.com..removeNameproduces exactly that RR shape. It can't be hit from axfrddns because owner names always come fromdcand are always in-zone, so I've left the code alone.go.modandgo.sumare untouched — both modules were already there.