diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b3ca1c..0a82d1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,19 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version ## [Unreleased] +### Fixed + +- panel: the PTR (reverse DNS) check no longer reports a correctly published + record as wrong. The checks went through the container's own resolver, which + forwards to the host's systemd-resolved — and systemd-resolved answers the + reverse lookup of the machine's own IP from the local hostname instead of + asking public DNS. A server with `81.30.105.2 → selfpost.example.com` in DNS + was told its PTR pointed at the provider-assigned hostname. All four + deliverability checks (PTR, SPF, DKIM, DMARC) now query recursive resolvers + directly, so the panel reports what a receiving mail server actually sees. + Set `SELFPOST_DNS_RESOLVERS` if outbound port 53 is closed or you run your + own recursor; it defaults to 1.1.1.1, 8.8.8.8 and 9.9.9.9. + ### Changed - panel: the three monitoring pages now live at URLs that match their nav diff --git a/cmd/panel/httpserver.go b/cmd/panel/httpserver.go index 72ca803..8a1e6b8 100644 --- a/cmd/panel/httpserver.go +++ b/cmd/panel/httpserver.go @@ -38,6 +38,7 @@ func serveHTTP(ctx context.Context, cfg config, st *store.Store) error { OpenDKIMSocket: cfg.opendkimSocket, JournalSocket: cfg.journalSocket, SessionIdleDays: cfg.sessionIdleDays, + DNSResolvers: cfg.dnsResolvers, }, cfg.setupTokenPath) if err != nil { return err diff --git a/cmd/panel/main.go b/cmd/panel/main.go index 6d6daa2..8c11635 100644 --- a/cmd/panel/main.go +++ b/cmd/panel/main.go @@ -23,6 +23,7 @@ import ( "codeberg.org/mix/selfpost/internal/backup" "codeberg.org/mix/selfpost/internal/buildinfo" + "codeberg.org/mix/selfpost/internal/dnscheck" "codeberg.org/mix/selfpost/internal/logtail" "codeberg.org/mix/selfpost/internal/store" ) @@ -61,6 +62,7 @@ type config struct { submissionEnabled bool trustedProxies []*net.IPNet sessionIdleDays int + dnsResolvers []string // Read-only inputs to the panel's status page: the certificate Postfix // serves and the two milter sockets it connects to. The defaults mirror @@ -107,6 +109,9 @@ func loadConfig() config { // Sliding session idle timeout (spec 7.6.6, plan B.1). Non-positive/invalid // falls back to the 7-day default inside internal/web. sessionIdleDays: envInt("PANEL_SESSION_IDLE_DAYS", 7), + // Recursive resolvers the deliverability checks query directly. Empty + // means dnscheck's public defaults; a closed network names its own here. + dnsResolvers: dnscheck.ParseResolvers(os.Getenv("SELFPOST_DNS_RESOLVERS")), tlsCertFile: envDefault("TLS_CERT_FILE", "/etc/postfix/tls/fullchain.pem"), opendkimSocket: envDefault("OPENDKIM_SOCKET", "/run/opendkim/opendkim.sock"), diff --git a/deploy/.env.example b/deploy/.env.example index de7dadb..4e045b8 100644 --- a/deploy/.env.example +++ b/deploy/.env.example @@ -22,6 +22,15 @@ SEND_LOG_RETENTION_DAYS=90 # monitoring screens does not count as activity, only navigation/actions do. PANEL_SESSION_IDLE_DAYS=7 +# Comma-separated resolvers the panel's deliverability checks (PTR, SPF, DKIM, +# DMARC) query directly. They deliberately skip this host's own resolver: with +# systemd-resolved the reverse lookup of the server's IP answers with the local +# hostname instead of the PTR record published in public DNS, which made the +# panel report a correct PTR as wrong. Leave unset for the public defaults +# (1.1.1.1, 8.8.8.8, 9.9.9.9); set it if outbound port 53 is closed or you run +# your own recursor, e.g. SELFPOST_DNS_RESOLVERS=10.0.0.2:53 +# SELFPOST_DNS_RESOLVERS= + # Comma-separated CIDRs (bare IPs allowed) of reverse proxies allowed to # supply X-Forwarded-For for login/setup rate-limiting. Leave unset unless # you know the exact address of your reverse proxy — trusting the wrong diff --git a/deploy/docker-compose.yml b/deploy/docker-compose.yml index 03ff977..1f547a0 100644 --- a/deploy/docker-compose.yml +++ b/deploy/docker-compose.yml @@ -41,6 +41,10 @@ services: SEND_LOG_RETENTION_DAYS: "${SEND_LOG_RETENTION_DAYS:-90}" # Sliding idle timeout for the panel login session, in days (spec 7.6.6). PANEL_SESSION_IDLE_DAYS: "${PANEL_SESSION_IDLE_DAYS:-7}" + # Resolvers the panel's PTR/SPF/DKIM/DMARC checks query directly, so they + # report what the internet sees rather than what this host's own stub + # resolver synthesises. Empty uses public defaults. + SELFPOST_DNS_RESOLVERS: "${SELFPOST_DNS_RESOLVERS:-}" volumes: # All persistent state lives under /data (spec 9): SQLite DB, DKIM keys, # sasldb2, sender map, setup token. Back this up (panel button or the diff --git a/internal/dnscheck/dnscheck.go b/internal/dnscheck/dnscheck.go index 4fb4184..87e945c 100644 --- a/internal/dnscheck/dnscheck.go +++ b/internal/dnscheck/dnscheck.go @@ -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 { diff --git a/internal/dnscheck/resolver.go b/internal/dnscheck/resolver.go new file mode 100644 index 0000000..3c8f358 --- /dev/null +++ b/internal/dnscheck/resolver.go @@ -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 +} diff --git a/internal/dnscheck/resolver_test.go b/internal/dnscheck/resolver_test.go new file mode 100644 index 0000000..83a913f --- /dev/null +++ b/internal/dnscheck/resolver_test.go @@ -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) + } +} diff --git a/internal/web/web.go b/internal/web/web.go index ce53883..8741720 100644 --- a/internal/web/web.go +++ b/internal/web/web.go @@ -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), diff --git a/test/e2e/compose.override.yml b/test/e2e/compose.override.yml index e913561..ee6de56 100644 --- a/test/e2e/compose.override.yml +++ b/test/e2e/compose.override.yml @@ -30,6 +30,10 @@ services: SELFPOST_HOSTNAME: mail.e2e.test # Plain HTTP panel for the test client — no reverse proxy in this stand. PANEL_COOKIE_SECURE: "false" + # The panel's deliverability checks query resolvers directly rather than + # the container's own (see internal/dnscheck), so the fake zone has to be + # named here too — `dns:` below only redirects the system resolver. + SELFPOST_DNS_RESOLVERS: 10.77.0.10:53 # High enough that the whole suite's other ~10 sends (positive path, # level-2 test, the various rejected attempts) never trip it by # accident — only the dedicated level-1 test (negative check 4, which