From b3aab92031cff1ce5d7a05092585bd023e8f3e6c Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 1 Oct 2026 22:37:02 +0000 Subject: [PATCH] resolver: pass over a server that answers SERVFAIL or refers no closer (closes #197) When the resolver walks from the root servers towards a name, a server that answered SERVFAIL, or referred the query back to its own zone, up or sideways, ended the step, so finding a zone's servers gave up on the zone though its other servers would answer. Such a reply is now passed over for the zone's next server, as a timeout or a refusal already was. To tell a referral that leads closer to the name from one that does not, each walk keeps the zone of the servers it is asking. Other error replies, such as FORMERR, are passed over too. The walk that finds a nameserver's address shares the same server loop, so it changes too. Model: opus-5-5 --- TODO.md | 2 + internal/resolver/errors.go | 7 +++ internal/resolver/export_test.go | 5 ++ internal/resolver/iterative.go | 56 +++++++++++++++++- internal/resolver/iterative_test.go | 91 +++++++++++++++++++++++++++++ 5 files changed, 158 insertions(+), 3 deletions(-) diff --git a/TODO.md b/TODO.md index dc9e0a3..8972d49 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,8 @@ trial run of the finished image: https://git.eeqj.de/sneak/dnswatcher/issues/149 # Completed Steps +- 2026-10-01: a zone's server that answers SERVFAIL or a referral leading no + closer is passed over for the next, as one that times out is (closes #197). - 2026-10-01: when none of a configured name's nameservers answered, the port state saved for its addresses is kept, not removed (closes #193). - 2026-10-01: `ResolveIPAddresses` returns an error, not no addresses, when no diff --git a/internal/resolver/errors.go b/internal/resolver/errors.go index ab05678..91eea16 100644 --- a/internal/resolver/errors.go +++ b/internal/resolver/errors.go @@ -15,6 +15,13 @@ var ( // so whether the name has addresses is unknown. ErrNoNameserverAnswered = errors.New("no nameserver answered") + // ErrUnusableReply is returned when a server replied with an + // error such as SERVFAIL, or with a referral that leads no + // closer to the name asked about. + ErrUnusableReply = errors.New( + "reply is an error or a referral that leads no closer", + ) + // ErrCNAMEDepthExceeded is returned when a CNAME chain // exceeds MaxCNAMEDepth. ErrCNAMEDepthExceeded = errors.New( diff --git a/internal/resolver/export_test.go b/internal/resolver/export_test.go index 9569a3a..73c3a33 100644 --- a/internal/resolver/export_test.go +++ b/internal/resolver/export_test.go @@ -11,6 +11,11 @@ func ExtractRecordValue(rr dns.RR) string { return extractRecordValue(rr) } +// UsableReply exports usableReply for testing. +func UsableReply(resp *dns.Msg, zone string, name string) bool { + return usableReply(resp, zone, name) +} + // CollectIPs exports collectIPs for testing. func CollectIPs( results map[string]*NameserverResponse, diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index a4c88ab..7c5cfa7 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -207,13 +207,16 @@ func (r *Resolver) followDelegation( domain string, servers []string, ) ([]string, error) { + // servers are the root servers, the servers of zone ".". + zone := "." + for range maxDelegation { if checkCtx(ctx) != nil { return nil, ErrContextCanceled } resp, err := r.queryServers( - ctx, servers, domain, dns.TypeNS, + ctx, servers, zone, domain, dns.TypeNS, ) if err != nil { return nil, err @@ -250,14 +253,19 @@ func (r *Resolver) followDelegation( } servers = nextServers + zone = referralZone(resp) } return nil, ErrNoNameservers } +// queryServers asks servers, the servers of zone, about name until one +// gives a usable reply. A server that times out, refuses or gives a +// reply that is not usable is passed over for the next. func (r *Resolver) queryServers( ctx context.Context, servers []string, + zone string, name string, qtype uint16, ) (*dns.Msg, error) { @@ -269,6 +277,12 @@ func (r *Resolver) queryServers( } resp, err := r.queryDNS(ctx, ip, name, qtype) + if err == nil && !usableReply(resp, zone, name) { + err = fmt.Errorf( + "query %s @%s: %w", name, ip, ErrUnusableReply, + ) + } + if err == nil { return resp, nil } @@ -279,6 +293,38 @@ func (r *Resolver) queryServers( return nil, fmt.Errorf("all servers failed: %w", lastErr) } +// usableReply reports whether resp, a reply from one of the servers of +// zone to a query about name, is usable. An error reply such as SERVFAIL +// is not. Nor is a referral, unless it refers the query to a zone below +// zone that name is in: a server that refers it back to zone, up or +// sideways does not serve zone as it should. +func usableReply(resp *dns.Msg, zone string, name string) bool { + if resp.Rcode != dns.RcodeSuccess && resp.Rcode != dns.RcodeNameError { + return false + } + + child := referralZone(resp) + if resp.Authoritative || len(resp.Answer) > 0 || child == "" { + return true + } + + return child != zone && dns.IsSubDomain(zone, child) && + dns.IsSubDomain(child, name) +} + +// referralZone returns the zone a referral refers the query to: the +// owner name of the NS records in resp's authority section, or "" when +// there are none. +func referralZone(resp *dns.Msg) string { + for _, rr := range resp.Ns { + if ns, ok := rr.(*dns.NS); ok { + return strings.ToLower(ns.Hdr.Name) + } + } + + return "" +} + func (r *Resolver) resolveNSIPs( ctx context.Context, nsNames []string, @@ -312,6 +358,7 @@ func (r *Resolver) resolveNSIterative( domain = dns.Fqdn(domain) servers := rootServerList() + zone := "." for range maxDelegation { if checkCtx(ctx) != nil { @@ -319,7 +366,7 @@ func (r *Resolver) resolveNSIterative( } resp, err := r.queryServers( - ctx, servers, domain, dns.TypeNS, + ctx, servers, zone, domain, dns.TypeNS, ) if err != nil { return nil, err @@ -344,6 +391,7 @@ func (r *Resolver) resolveNSIterative( } servers = nextServers + zone = referralZone(resp) } return nil, ErrNoNameservers @@ -361,6 +409,7 @@ func (r *Resolver) resolveARecord( hostname = dns.Fqdn(hostname) servers := rootServerList() + zone := "." for range maxDelegation { if checkCtx(ctx) != nil { @@ -368,7 +417,7 @@ func (r *Resolver) resolveARecord( } resp, err := r.queryServers( - ctx, servers, hostname, dns.TypeA, + ctx, servers, zone, hostname, dns.TypeA, ) if err != nil { return nil, fmt.Errorf( @@ -406,6 +455,7 @@ func (r *Resolver) resolveARecord( } servers = nextServers + zone = referralZone(resp) } return nil, fmt.Errorf( diff --git a/internal/resolver/iterative_test.go b/internal/resolver/iterative_test.go index f3440e7..cc1b30b 100644 --- a/internal/resolver/iterative_test.go +++ b/internal/resolver/iterative_test.go @@ -41,6 +41,97 @@ func TestCollectIPs_FailedIsNoAnswer(t *testing.T) { assert.Empty(t, ips) } +// exampleCom is the zone most cases of TestUsableReply are about. +const exampleCom = "example.com." + +// nsRecord builds an NS record that names a server of zone. +func nsRecord(zone string) *dns.NS { + return &dns.NS{ + Hdr: dns.RR_Header{ + Name: zone, Rrtype: dns.TypeNS, Class: dns.ClassINET, + }, + Ns: "ns1.example.net.", + } +} + +// referralTo builds a reply that refers the query to the servers of +// zone. +func referralTo(zone string) *dns.Msg { + msg := new(dns.Msg) + msg.Ns = []dns.RR{nsRecord(zone)} + + return msg +} + +// TestUsableReply checks which replies from one of a zone's servers are +// used. A reply that is not usable moves the query on to the zone's +// next server. +func TestUsableReply(t *testing.T) { + t.Parallel() + + servfail := new(dns.Msg) + servfail.Rcode = dns.RcodeServerFailure + + answer := new(dns.Msg) + answer.Authoritative = true + answer.Answer = []dns.RR{nsRecord(exampleCom)} + + nxdomain := new(dns.Msg) + nxdomain.Authoritative = true + nxdomain.Rcode = dns.RcodeNameError + + tests := []struct { + name string + resp *dns.Msg + zone string + query string + want bool + }{ + { + name: "SERVFAIL", resp: servfail, + zone: exampleCom, query: exampleCom, want: false, + }, + { + name: "answer", resp: answer, + zone: exampleCom, query: exampleCom, want: true, + }, + { + name: "NXDOMAIN", resp: nxdomain, + zone: ".", query: exampleCom, want: true, + }, + { + name: "root refers to com", resp: referralTo("com."), + zone: ".", query: exampleCom, want: true, + }, + { + name: "com refers to example.com", resp: referralTo(exampleCom), + zone: "com.", query: "www.example.com.", want: true, + }, + { + name: "referral back to the zone", resp: referralTo(exampleCom), + zone: exampleCom, query: exampleCom, want: false, + }, + { + name: "referral up to the root", resp: referralTo("."), + zone: exampleCom, query: exampleCom, want: false, + }, + { + name: "referral sideways", resp: referralTo("net."), + zone: ".", query: exampleCom, want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.want, + resolver.UsableReply(tt.resp, tt.zone, tt.query), + ) + }) + } +} + func TestExtractRecordValue_LetterCase(t *testing.T) { t.Parallel() -- 2.54.0