diff --git a/README.md b/README.md index d1ec4db..1850c65 100644 --- a/README.md +++ b/README.md @@ -382,6 +382,13 @@ performs full iterative resolution: 4. **Authoritative query**: Queries all discovered authoritative nameservers directly for the requested records. +In steps 2 and 3 the servers are asked one at a time in a random order, chosen +anew each time, so no one root server gets every first query. A server that does +not reply, refuses the query, or gives an error reply such as SERVFAIL or a +referral that leads no closer to the name is passed over for the next one. When +a referral names a zone's nameservers without their addresses, the addresses of +all of them are looked up, so that each can be asked. + This approach ensures: - Independence from any upstream resolver's cache or filtering. diff --git a/TODO.md b/TODO.md index 6b45c81..69d8e06 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-02: the resolver tries root servers, and every other server list it + walks, in a random order each time, not always from the top (closes #138). - 2026-10-01: a certificate within the expiry warning period is warned about on every TLS check, where some checks used to skip it at random (closes #204). - 2026-10-01: a domain's NS set is its delegation from the parent zone's @@ -128,5 +130,4 @@ trial run of the finished image: https://git.eeqj.de/sneak/dnswatcher/issues/149 - 1.0 readiness: run it with a real config and read the logs: https://git.eeqj.de/sneak/dnswatcher/issues/66 - README accuracy sweep: https://git.eeqj.de/sneak/dnswatcher/issues/108 -- fixed root server order: https://git.eeqj.de/sneak/dnswatcher/issues/138 - review toward 1.0: https://git.eeqj.de/sneak/dnswatcher/issues/144 diff --git a/internal/resolver/export_test.go b/internal/resolver/export_test.go index 5c59228..868174c 100644 --- a/internal/resolver/export_test.go +++ b/internal/resolver/export_test.go @@ -36,3 +36,24 @@ func (r *Resolver) QueryEachNS( ) (map[string]*NameserverResponse, error) { return r.queryEachNS(ctx, nameservers, hostname) } + +// ResolveNSIPs exports resolveNSIPs for testing. +func (r *Resolver) ResolveNSIPs( + ctx context.Context, + nsNames []string, +) []string { + return r.resolveNSIPs(ctx, nsNames) +} + +// RootServerList exports rootServerList for testing. +func RootServerList() []string { + return rootServerList() +} + +// Shuffled exports shuffled for testing. +func Shuffled( + servers []string, + shuffle func(n int, swap func(i, j int)), +) []string { + return shuffled(servers, shuffle) +} diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index d495193..0c526e5 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -4,7 +4,9 @@ import ( "context" "errors" "fmt" + "math/rand/v2" "net" + "slices" "sort" "strings" "time" @@ -259,9 +261,25 @@ func (r *Resolver) followDelegation( 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. +// shuffled returns a copy of servers in the order shuffle puts them +// in. The resolver passes rand.Shuffle, so each time it walks a list of +// servers it starts at a random one, and no one server gets every +// first query. +func shuffled( + servers []string, + shuffle func(n int, swap func(i, j int)), +) []string { + order := slices.Clone(servers) + shuffle(len(order), func(i, j int) { + order[i], order[j] = order[j], order[i] + }) + + return order +} + +// queryServers asks servers, the servers of zone, about name in a random +// order 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, @@ -271,7 +289,7 @@ func (r *Resolver) queryServers( ) (*dns.Msg, error) { var lastErr error - for _, ip := range servers { + for _, ip := range shuffled(servers, rand.Shuffle) { if checkCtx(ctx) != nil { return nil, ErrContextCanceled } @@ -340,6 +358,10 @@ func nsSetFrom(resp *dns.Msg, domain string) []string { return extractNSSet(resp.Answer) } +// resolveNSIPs returns the addresses of every nameserver in nsNames +// whose name resolves, for a referral that carries none. The walk can +// then go on to the zone's other nameservers when one gives no usable +// reply. func (r *Resolver) resolveNSIPs( ctx context.Context, nsNames []string, @@ -351,10 +373,6 @@ func (r *Resolver) resolveNSIPs( if err == nil { ips = append(ips, resolved...) } - - if len(ips) > 0 { - break - } } return ips diff --git a/internal/resolver/iterative_test.go b/internal/resolver/iterative_test.go index 413cc71..4f539ae 100644 --- a/internal/resolver/iterative_test.go +++ b/internal/resolver/iterative_test.go @@ -1,6 +1,8 @@ package resolver_test import ( + "math/rand/v2" + "slices" "testing" "github.com/miekg/dns" @@ -235,3 +237,30 @@ func TestExtractRecordValue_LetterCase(t *testing.T) { }) } } + +// TestShuffled shuffles the root servers with many seeds. Every order +// must hold each root server once, so each is tried before a +// resolution fails; each root server must come first for some seed, so +// no one root server gets every first query; and the list passed in +// must be left as it was. +func TestShuffled(t *testing.T) { + t.Parallel() + + const seeds = 1000 + + roots := resolver.RootServerList() + before := slices.Clone(roots) + first := make(map[string]bool) + + for seed := range uint64(seeds) { + rng := rand.New(rand.NewPCG(seed, 0)) //nolint:gosec // seeded on purpose + order := resolver.Shuffled(roots, rng.Shuffle) + + assert.ElementsMatch(t, roots, order) + + first[order[0]] = true + } + + assert.Len(t, first, len(roots)) + assert.Equal(t, before, roots) +} diff --git a/internal/resolver/livedns_test.go b/internal/resolver/livedns_test.go index fb425a8..0bbccb4 100644 --- a/internal/resolver/livedns_test.go +++ b/internal/resolver/livedns_test.go @@ -383,3 +383,37 @@ func liveResolveIPsAllowingEmpty( return out } + +// liveResolveNSIPs looks up the addresses of the nameservers named +// names, retrying until there are at least atLeast of them: a name +// whose lookup got no reply is left out of the result, not an error. +func liveResolveNSIPs( + t *testing.T, + r *resolver.Resolver, + names []string, + atLeast int, +) []string { + t.Helper() + + var out []string + + livednstest.Retry( + t, + "ResolveNSIPs("+strings.Join(names, ", ")+")", + func(ctx context.Context) error { + ips := r.ResolveNSIPs(ctx, names) + if len(ips) < atLeast { + return fmt.Errorf( + "%w: %d addresses, expected at least %d", + livednstest.ErrNoAnswer, len(ips), atLeast, + ) + } + + out = ips + + return nil + }, + ) + + return out +} diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index dd585c8..abb6b92 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -139,6 +139,28 @@ func TestFindAuthoritativeNameservers_CloudflareDomain( } } +// TestResolveNSIPs_EveryNameserver looks up the addresses of two of +// google.com's nameservers together, as the walk does when a referral +// names a zone's nameservers without their addresses, and compares them +// with each looked up alone. Together they must give the addresses of +// both, not only of the first that resolves, so that when one gives no +// usable reply the walk goes on to the other. +func TestResolveNSIPs_EveryNameserver(t *testing.T) { + t.Parallel() + + r := newTestResolver(t) + names := []string{"ns3.google.com.", "ns4.google.com."} + want := make([]string, 0, len(names)) + + for _, name := range names { + want = append(want, liveResolveNSIPs(t, r, []string{name}, 1)...) + } + + got := liveResolveNSIPs(t, r, names, len(want)) + + assert.ElementsMatch(t, want, got) +} + // ---------------------------------------------------------------- // QueryNameserver tests // ----------------------------------------------------------------