dnscheck: query recursive resolvers directly, not the system one

The PTR check reported a correctly published record as wrong. The lookups went through the container's resolver (127.0.0.11) which forwards to the host's systemd-resolved, and systemd-resolved synthesises the reverse lookup of the machine's own addresses from the local hostname rather than asking public DNS. On the production host that meant 81.30.105.2 -> magenta-pink-heliotrope16786 (does not match) while public DNS has had 81.30.105.2 -> selfpost.mixfed.ru all along. These checks exist to report what a receiving mail server sees, so they now dial recursive resolvers themselves, defaulting to 1.1.1.1, 8.8.8.8 and 9.9.9.9 and overridable with SELFPOST_DNS_RESOLVERS. The e2e stand sets it to its CoreDNS, which the `dns:` directive alone no longer covers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-03 21:53:24 +03:00
parent 12a11f185c
commit 0ca5cba453
10 changed files with 259 additions and 5 deletions
@@ -103,10 +103,11 @@ type cached[T any] struct {
 	expires time.Time
 }

-// New returns a Checker using the process resolver and the package's default
-// timeout and cache lifetimes.
-func New() *Checker {
-	return newChecker(net.DefaultResolver, lookupTimeout, serverTTL, domainTTL)
+// New returns a Checker querying the given recursive resolvers (empty means
+// DefaultResolvers) with the package's default timeout and cache lifetimes.
+// The lookups deliberately bypass the system resolver — see externalResolver.
+func New(resolvers []string) *Checker {
+	return newChecker(newExternalResolver(resolvers), lookupTimeout, serverTTL, domainTTL)
 }

 func newChecker(r resolver, timeout, srvTTL, domTTL time.Duration) *Checker {
@@ -0,0 +1,112 @@
+package dnscheck
+
+import (
+	"context"
+	"errors"
+	"net"
+	"strings"
+)
+
+// DefaultResolvers are the recursive resolvers the checks query when the
+// deployment does not name its own (SELFPOST_DNS_RESOLVERS). Three independent
+// operators, so one being unreachable from the host does not blind the checks.
+var DefaultResolvers = []string{"1.1.1.1:53", "8.8.8.8:53", "9.9.9.9:53"}
+
+// externalResolver talks to a fixed list of recursive resolvers directly
+// instead of going through the system resolver.
+//
+// That detour is deliberate: these checks answer "what does a receiving mail
+// server see about us?", and the machine's own stub resolver is the one place
+// where the answer differs. systemd-resolved — which a Docker container reaches
+// through the embedded 127.0.0.11 forwarder — synthesises a PTR record for the
+// host's own addresses out of the local hostname, never asking public DNS. A
+// server whose reverse DNS was published correctly therefore had its
+// provider-assigned hostname reported back to the panel, and the FCrDNS check
+// failed a record that was in fact right.
+//
+// This bypasses the resolvers in /etc/resolv.conf, not /etc/hosts: Go still
+// consults the hosts file first, as any local program would.
+type externalResolver struct {
+	servers []*net.Resolver
+}
+
+// newExternalResolver builds a resolver over addrs ("host" or "host:port"). An
+// empty list falls back to DefaultResolvers.
+func newExternalResolver(addrs []string) *externalResolver {
+	if len(addrs) == 0 {
+		addrs = DefaultResolvers
+	}
+	e := &externalResolver{}
+	for _, a := range addrs {
+		addr := withDefaultPort(a)
+		e.servers = append(e.servers, &net.Resolver{
+			PreferGo: true,
+			Dial: func(ctx context.Context, network, _ string) (net.Conn, error) {
+				var d net.Dialer
+				return d.DialContext(ctx, network, addr)
+			},
+		})
+	}
+	return e
+}
+
+func (e *externalResolver) LookupTXT(ctx context.Context, name string) ([]string, error) {
+	return queryEach(e.servers, func(r *net.Resolver) ([]string, error) { return r.LookupTXT(ctx, name) })
+}
+
+func (e *externalResolver) LookupIPAddr(ctx context.Context, host string) ([]net.IPAddr, error) {
+	return queryEach(e.servers, func(r *net.Resolver) ([]net.IPAddr, error) { return r.LookupIPAddr(ctx, host) })
+}
+
+func (e *externalResolver) LookupAddr(ctx context.Context, addr string) ([]string, error) {
+	return queryEach(e.servers, func(r *net.Resolver) ([]string, error) { return r.LookupAddr(ctx, addr) })
+}
+
+func (e *externalResolver) LookupMX(ctx context.Context, name string) ([]*net.MX, error) {
+	return queryEach(e.servers, func(r *net.Resolver) ([]*net.MX, error) { return r.LookupMX(ctx, name) })
+}
+
+// queryEach asks each resolver in turn and stops at the first one that
+// answers. "No such name" is an answer — only a resolver that cannot be
+// reached, or that times out, moves the query on to the next one.
+func queryEach[T any](servers []*net.Resolver, ask func(*net.Resolver) (T, error)) (T, error) {
+	var zero T
+	var lastErr error
+	for _, r := range servers {
+		v, err := ask(r)
+		if err == nil {
+			return v, nil
+		}
+		var dnsErr *net.DNSError
+		if errors.As(err, &dnsErr) && dnsErr.IsNotFound {
+			return zero, err
+		}
+		lastErr = err
+	}
+	if lastErr == nil {
+		lastErr = errors.New("no DNS resolver configured")
+	}
+	return zero, lastErr
+}
+
+// withDefaultPort appends the DNS port to a bare address, so the environment
+// variable can name a resolver as plainly as "1.1.1.1".
+func withDefaultPort(addr string) string {
+	if _, _, err := net.SplitHostPort(addr); err == nil {
+		return addr
+	}
+	return net.JoinHostPort(addr, "53")
+}
+
+// ParseResolvers reads a comma-separated resolver list, as it arrives from the
+// environment. Blank entries are skipped; an empty result means "use
+// DefaultResolvers".
+func ParseResolvers(s string) []string {
+	var out []string
+	for _, part := range strings.Split(s, ",") {
+		if p := strings.TrimSpace(part); p != "" {
+			out = append(out, p)
+		}
+	}
+	return out
+}
@@ -0,0 +1,100 @@
+package dnscheck
+
+import (
+	"context"
+	"errors"
+	"net"
+	"testing"
+)
+
+func TestWithDefaultPort(t *testing.T) {
+	cases := map[string]string{
+		"1.1.1.1":          "1.1.1.1:53",
+		"1.1.1.1:5353":     "1.1.1.1:5353",
+		"dns.example.com":  "dns.example.com:53",
+		"2606:4700:4700::": "[2606:4700:4700::]:53",
+		"[::1]:5353":       "[::1]:5353",
+	}
+	for in, want := range cases {
+		if got := withDefaultPort(in); got != want {
+			t.Errorf("withDefaultPort(%q) = %q, want %q", in, got, want)
+		}
+	}
+}
+
+func TestParseResolvers(t *testing.T) {
+	got := ParseResolvers(" 1.1.1.1 , ,8.8.8.8:53,")
+	want := []string{"1.1.1.1", "8.8.8.8:53"}
+	if len(got) != len(want) {
+		t.Fatalf("got %q, want %q", got, want)
+	}
+	for i := range want {
+		if got[i] != want[i] {
+			t.Fatalf("got %q, want %q", got, want)
+		}
+	}
+	if r := ParseResolvers(""); len(r) != 0 {
+		t.Errorf("empty string parsed to %q, want nothing", r)
+	}
+}
+
+func TestNewExternalResolverFallsBackToDefaults(t *testing.T) {
+	if n := len(newExternalResolver(nil).servers); n != len(DefaultResolvers) {
+		t.Errorf("resolver count = %d, want %d (the defaults)", n, len(DefaultResolvers))
+	}
+}
+
+// The checks must reach the second resolver when the first is unreachable, but
+// must not second-guess an authoritative "no such name" — otherwise a domain
+// that genuinely lacks a record costs one timeout per configured resolver.
+func TestQueryEachTriesTheNextResolverOnlyOnFailure(t *testing.T) {
+	servers := []*net.Resolver{{}, {}, {}}
+
+	asked := 0
+	got, err := queryEach(servers, func(*net.Resolver) (string, error) {
+		asked++
+		if asked < 3 {
+			return "", &net.DNSError{Err: "timed out", IsTimeout: true}
+		}
+		return "answer", nil
+	})
+	if err != nil || got != "answer" {
+		t.Fatalf("got (%q, %v), want (\"answer\", nil)", got, err)
+	}
+	if asked != 3 {
+		t.Errorf("asked %d resolvers, want 3", asked)
+	}
+
+	asked = 0
+	_, err = queryEach(servers, func(*net.Resolver) (string, error) {
+		asked++
+		return "", notFound("absent.example")
+	})
+	var dnsErr *net.DNSError
+	if !errors.As(err, &dnsErr) || !dnsErr.IsNotFound {
+		t.Fatalf("err = %v, want a not-found DNSError", err)
+	}
+	if asked != 1 {
+		t.Errorf("NXDOMAIN asked %d resolvers, want 1 — it is an answer, not a failure", asked)
+	}
+}
+
+// The reason this package does not use net.DefaultResolver: systemd-resolved
+// answers PTR queries for the machine's own addresses out of the local
+// hostname, which is not what the rest of the internet sees. The dial hook must
+// therefore ignore the address the standard resolver picked from
+// /etc/resolv.conf and connect to the configured one.
+func TestExternalResolverDialsOnlyTheConfiguredAddress(t *testing.T) {
+	e := newExternalResolver([]string{"192.0.2.53"})
+	if len(e.servers) != 1 {
+		t.Fatalf("servers = %d, want 1", len(e.servers))
+	}
+	conn, err := e.servers[0].Dial(context.Background(), "udp", "127.0.0.53:53")
+	if err != nil {
+		t.Fatalf("dial: %v", err)
+	}
+	defer conn.Close()
+	if got := conn.RemoteAddr().String(); got != "192.0.2.53:53" {
+		t.Errorf("connected to %q, want 192.0.2.53:53 (the system resolver won)", got)
+	}
+}
@@ -63,6 +63,11 @@ type Config struct {
 	// session expires (env PANEL_SESSION_IDLE_DAYS, plan B.1). Non-positive
 	// falls back to the 7-day default.
 	SessionIdleDays int
+	// DNSResolvers are the recursive resolvers the deliverability checks query
+	// (env SELFPOST_DNS_RESOLVERS). Empty uses dnscheck.DefaultResolvers. The
+	// checks must not go through the system resolver — see dnscheck's
+	// externalResolver — so this is how a closed network points them at its own.
+	DNSResolvers []string
 }

 // Server is the panel HTTP application.
@@ -105,7 +110,7 @@ func New(st *store.Store, domains *domain.Service, apps *app.Service, cfg Config
 		// Published-DNS checks for the status page and the domain pages. The
 		// checker caches its own results, so page views do not each pay for a
 		// round of lookups (phase 13).
-		dns: dnscheck.New(),
+		dns: dnscheck.New(cfg.DNSResolvers),
 		// Setup: a handful of attempts per minute per IP is plenty for a
 		// legitimate admin and blunts automated probing (spec 7.6.1).
 		setupLimiter: newRateLimiter(10, time.Minute),