diff --git a/TODO.md b/TODO.md index 928baeb..4278559 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,8 @@ https://git.eeqj.de/sneak/dnswatcher/issues/149 # Completed Steps +- 2026-10-01: `ResolveIPAddresses` returns an error, not no addresses, when no + nameserver of the name's zone answered (closes #190). - 2026-10-01: a hostname is queried at the servers of the zone it is in, found by following delegations for the name, not its last two labels (closes #189). - 2026-10-01: each nameserver's addresses are saved with its domain, and a diff --git a/internal/resolver/errors.go b/internal/resolver/errors.go index 3f203d4..cc2cbd2 100644 --- a/internal/resolver/errors.go +++ b/internal/resolver/errors.go @@ -10,6 +10,11 @@ var ( "no authoritative nameservers found", ) + // ErrNoNameserverAnswered is returned when every nameserver + // asked about a name timed out or failed, so whether the name + // has addresses is unknown. + ErrNoNameserverAnswered = errors.New("no nameserver answered") + // 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 d8e382b..9569a3a 100644 --- a/internal/resolver/export_test.go +++ b/internal/resolver/export_test.go @@ -11,6 +11,13 @@ func ExtractRecordValue(rr dns.RR) string { return extractRecordValue(rr) } +// CollectIPs exports collectIPs for testing. +func CollectIPs( + results map[string]*NameserverResponse, +) ([]string, string, error) { + return collectIPs(results) +} + // QueryEachNS exports queryEachNS for testing. func (r *Resolver) QueryEachNS( ctx context.Context, diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index 93f6b9b..921870c 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -734,7 +734,9 @@ func (r *Resolver) LookupAllRecords( } // ResolveIPAddresses resolves a hostname to all IPv4 and IPv6 -// addresses, following CNAME chains up to MaxCNAMEDepth. +// addresses, following CNAME chains up to MaxCNAMEDepth. When no +// nameserver of the name's zone answered, it returns an error wrapping +// ErrNoNameserverAnswered rather than no addresses. func (r *Resolver) ResolveIPAddresses( ctx context.Context, hostname string, @@ -760,7 +762,10 @@ func (r *Resolver) resolveIPWithCNAME( return nil, err } - ips, cnameTarget := collectIPs(results) + ips, cnameTarget, err := collectIPs(results) + if err != nil { + return nil, fmt.Errorf("resolving %s: %w", hostname, err) + } if len(ips) == 0 && cnameTarget != "" { return r.resolveIPWithCNAME(ctx, cnameTarget, depth+1) @@ -771,16 +776,28 @@ func (r *Resolver) resolveIPWithCNAME( return ips, nil } +// collectIPs returns the addresses in the nameservers' answers and the +// first CNAME target among them. It returns ErrNoNameserverAnswered when +// every nameserver timed out or failed: that is not a name with no +// addresses. func collectIPs( results map[string]*NameserverResponse, -) ([]string, string) { +) ([]string, string, error) { seen := make(map[string]bool) var ips []string var cnameTarget string + answered := false + for _, resp := range results { + if resp.Status == StatusTimeout || resp.Status == StatusError { + continue + } + + answered = true + if resp.Status == StatusNXDomain { continue } @@ -804,5 +821,9 @@ func collectIPs( } } - return ips, cnameTarget + if !answered { + return nil, "", ErrNoNameserverAnswered + } + + return ips, cnameTarget, nil } diff --git a/internal/resolver/iterative_test.go b/internal/resolver/iterative_test.go index 781e90b..04a33b9 100644 --- a/internal/resolver/iterative_test.go +++ b/internal/resolver/iterative_test.go @@ -5,10 +5,26 @@ import ( "github.com/miekg/dns" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "sneak.berlin/go/dnswatcher/internal/resolver" ) +// TestCollectIPs_OneAnswerIsEnough checks that one nameserver answering +// NXDOMAIN says the name has no addresses, though the other timed out. +func TestCollectIPs_OneAnswerIsEnough(t *testing.T) { + t.Parallel() + + ips, _, err := resolver.CollectIPs( + map[string]*resolver.NameserverResponse{ + "ns1.example.": {Status: resolver.StatusTimeout}, + "ns2.example.": {Status: resolver.StatusNXDomain}, + }, + ) + require.NoError(t, err) + assert.Empty(t, ips) +} + func TestExtractRecordValue_LetterCase(t *testing.T) { t.Parallel() diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index 15a18c5..e8bce6a 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -633,6 +633,35 @@ func TestQueryNameserverIP_Timeout(t *testing.T) { assert.NotEmpty(t, resp.Error) } +// TestCollectIPs_NoNameserverAnswered takes the response of a +// nameserver at 192.0.2.1, where nothing answers, as +// TestQueryNameserverIP_Timeout does. Addresses collected from +// nameservers that all failed to answer are an error, not none. +func TestCollectIPs_NoNameserverAnswered(t *testing.T) { + t.Parallel() + + r := newTestResolver(t) + + // The deadline outlasts the first try, as in + // TestQueryNameserverIP_Timeout. + ctx, cancel := context.WithTimeout( + context.Background(), 3*time.Second, + ) + t.Cleanup(cancel) + + resp, err := r.QueryNameserverIP( + ctx, "unreachable.test.", "192.0.2.1", + "example.com", + ) + require.NoError(t, err) + + ips, _, err := resolver.CollectIPs( + map[string]*resolver.NameserverResponse{resp.Nameserver: resp}, + ) + require.ErrorIs(t, err, resolver.ErrNoNameserverAnswered) + assert.Empty(t, ips) +} + func TestResolveIPAddresses_ContextCanceled(t *testing.T) { t.Parallel() diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 2a2c523..98f0c68 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -321,10 +321,9 @@ func (w *Watcher) detectNSChanges( } // resolveNameserverAddresses returns the sorted addresses each -// nameserver's name resolves to. A nameserver whose lookup fails or -// finds no address keeps its addresses from prev: the resolver finds no -// address, without an error, when every server it asks times out, and -// that is not an address change. +// nameserver's name resolves to. A nameserver whose lookup fails, as it +// does when no nameserver of the name's zone answers, or finds no +// address keeps its addresses from prev and is not an address change. func (w *Watcher) resolveNameserverAddresses( ctx context.Context, nameservers []string,