From 668e299c69fea72c61428684a5c497d4ef6b5523 Mon Sep 17 00:00:00 2001 From: Mix Date: Sat, 17 Oct 2026 22:25:20 +0300 Subject: [PATCH] postfix: no reload for a map that did not change; DNS re-checks limited (audit W6/M9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security audit 2026-10-17, findings W6/M9 (low), stage 4. Every application, upstream, recipient or report-address save reloaded Postfix through supervisord, and POST .../dns-recheck bypassed the five-minute cache for up to ~14 resolver queries - both at whatever rate an authenticated user pressed. - writeChanged: each generated map is written only when its content differs from the file on disk, and Postfix is reloaded only when one was (sender map, inbound maps, DMARC maps). Health's Reload configuration ends with an explicit reload (app.Service.ReloadPostfix, SenderMaps gains Reload), so it still re-reads whatever is on disk. No debounce: with unchanged saves gone, a burst is real changes Postfix must see. - auth.AllowDNSRecheck: twenty forced re-checks in 15 minutes per user; past that the outbound, inbound and settings re-check show the cached result without the "re-checked" flash, and the log says why. Tests: TestARebuildThatChangesNothingDoesNotReload, TestDNSRechecksArePerUser; the fakes gain Reload. Docs: guide § Overview and Health; CHANGELOG; plan. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 ++++ docs/guide.md | 6 ++-- docs/security-audit-2026-10-17.md | 9 ++++-- internal/app/service.go | 9 ++++++ internal/app/service_test.go | 4 +++ internal/postfix/dmarc.go | 9 ++---- internal/postfix/inbound.go | 12 ++------ internal/postfix/postfix.go | 3 +- internal/postfix/postfix_test.go | 30 +++++++++++++++++++ internal/postfix/write.go | 26 ++++++++++++++++ internal/web/auth/auth.go | 12 ++++++++ internal/web/auth/limitkeys_test.go | 18 +++++++++++ internal/web/handlers/handlers_apps.go | 16 ++++++++++ internal/web/handlers/handlers_apps_test.go | 2 ++ internal/web/handlers/handlers_domains.go | 3 ++ internal/web/handlers/handlers_errors_test.go | 2 ++ internal/web/handlers/handlers_inbound.go | 4 +++ internal/web/handlers/handlers_settings.go | 4 +++ 18 files changed, 153 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a09f861..b4fb652 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,12 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version a monitor next to it; the internet gets `404`. - **Every container has a process ceiling** (audit I3): `pids_limit` 1024 for SelfPost, 256 for the proxies and rspamd, 64 for certbot. +- **A save that changes no map no longer reloads Postfix, and DNS + re-checks are limited** (audit W6/M9): every application, upstream, + recipient or report-address save reloaded Postfix, and *Re-check DNS* + bypassed the cache without limit. Now only a changed file is written and + reloaded (Health's *Reload configuration* always re-reads), and a user + re-checks twenty times in 15 minutes. - **The TLS private key no longer has to be readable by Postfix** (audit I1): the guide gave it to group 10998 for the panel's backup, and `postfix` is in that group, so every Postfix daemon - an smtpd talking to diff --git a/docs/guide.md b/docs/guide.md index e4d65e8..20690f6 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -516,7 +516,8 @@ readings, numbered in the order of what stops mail first: - **Hostname and reverse DNS** — compares `SELFPOST_HOSTNAME` against the PTR record the internet publishes for this server's IP (forward-confirmed reverse DNS), see [Server-level DNS](#server-level-dns-ptrrdns); use - *Re-check DNS* after changing DNS. *Greeting* is the `220` line Postfix + *Re-check DNS* after changing DNS (twenty re-checks in 15 minutes per user; +past that the page shows the last result). *Greeting* is the `220` line Postfix opens every connection with; it must name the hostname, and another name there is a warning. - **Notification mail** — whether the DNS records the panel's own mail is @@ -538,7 +539,8 @@ readings, numbered in the order of what stops mail first: selector table (`/data/dkim/selectors`), the Postfix sender map, the inbound relay maps and the filter lists (with `INBOUND_RELAY_ENABLE=true`), the DMARC report maps (with `DMARC_REPORTS_ENABLE=true`) - removes the keys - of deleted domains, and has Postfix re-read them; the signer needs no + of deleted domains, and has Postfix re-read them (a save elsewhere that + changes no file asks no reload; this button always does); the signer needs no reload, it reads the table for every message. This is what the first start after a restore does (see [Restore](#restore)). Use it if the daemons drifted from what the panel shows after manual edits under diff --git a/docs/security-audit-2026-10-17.md b/docs/security-audit-2026-10-17.md index dd57ec4..4e8e1e5 100644 --- a/docs/security-audit-2026-10-17.md +++ b/docs/security-audit-2026-10-17.md @@ -804,9 +804,14 @@ the argument. SelfPost (Postfix's default_process_limit of 100 per service, three smtpd services), 256 for the proxies and rspamd, 64 for certbot; e2e reads `pids.max` back (checkPidsLimit). -- [ ] **W6/M9.** Coalesce Postfix reloads (reload only when the rendered map +- [x] **W6/M9.** Coalesce Postfix reloads (reload only when the rendered map changed; a 2 s debounce for bursts); a per-user limiter on `dns-recheck`. - — **Opus**, 1.5 h. + — **Opus**, 1.5 h. *Done 2026-10-17:* a rebuild that changes no file + writes nothing and asks no reload (`writeChanged`), Health's button + re-reads in any case; no debounce - with unchanged saves gone, a burst is + real changes, each of which Postfix must see; twenty re-checks in 15 + minutes per user, then the cached result. TestARebuildThatChangesNothingDoesNotReload, + TestDNSRechecksArePerUser. - [ ] **W4/W5/W8.** Key size 1024-4096 on import; ≤ 500 applications per export; an import named as the hostname refused with a sentence. — **Sonnet**, 1.5 h. diff --git a/internal/app/service.go b/internal/app/service.go index 538778a..4bfb033 100644 --- a/internal/app/service.go +++ b/internal/app/service.go @@ -17,6 +17,15 @@ import ( // *postfix.Postfix satisfies it; tests substitute a fake. type SenderMaps interface { RebuildSenderLoginMaps(bindings []postfix.Binding) error + // Reload has Postfix re-read its configuration whatever changed. + Reload() error +} + +// ReloadPostfix has Postfix re-read its configuration. A rebuild that +// changes no file asks no reload; Health's Reload configuration ends with +// this one, so that it re-reads whatever is on disk. +func (s *Service) ReloadPostfix() error { + return s.pf.Reload() } // Service coordinates application state across SQLite, sasldb2 and the Postfix diff --git a/internal/app/service_test.go b/internal/app/service_test.go index 15a6ce8..4a04003 100644 --- a/internal/app/service_test.go +++ b/internal/app/service_test.go @@ -20,6 +20,8 @@ type fakeMaps struct { failNext bool } +func (f *fakeMaps) Reload() error { return nil } + func (f *fakeMaps) RebuildSenderLoginMaps(b []postfix.Binding) error { f.calls++ if f.failNext { @@ -514,6 +516,8 @@ type overlapMaps struct { inside, most atomic.Int32 } +func (o *overlapMaps) Reload() error { return nil } + func (o *overlapMaps) RebuildSenderLoginMaps([]postfix.Binding) error { n := o.inside.Add(1) for { diff --git a/internal/postfix/dmarc.go b/internal/postfix/dmarc.go index f9b9a58..1b71f8f 100644 --- a/internal/postfix/dmarc.go +++ b/internal/postfix/dmarc.go @@ -29,13 +29,8 @@ func (p *Postfix) RebuildDMARCMaps(cfg DMARCMapsConfig) error { return err } rc, tr, rd := p.dmarcMapPaths() - if err := writeFileAtomic(rc, recip, 0o640); err != nil { - return err - } - if err := writeFileAtomic(tr, transport, 0o640); err != nil { - return err - } - if err := writeFileAtomic(rd, relay, 0o640); err != nil { + changed, err := writeChanged(mapFile{rc, recip}, mapFile{tr, transport}, mapFile{rd, relay}) + if err != nil || !changed { return err } return p.reload() diff --git a/internal/postfix/inbound.go b/internal/postfix/inbound.go index 89087c5..a43941d 100644 --- a/internal/postfix/inbound.go +++ b/internal/postfix/inbound.go @@ -40,16 +40,8 @@ func (p *Postfix) RebuildInboundMaps(routes []InboundRoute) error { return err } rd, tr, rc, tl := p.inboundMapPaths() - if err := writeFileAtomic(rd, relay, 0o640); err != nil { - return err - } - if err := writeFileAtomic(tr, transport, 0o640); err != nil { - return err - } - if err := writeFileAtomic(rc, recipients, 0o640); err != nil { - return err - } - if err := writeFileAtomic(tl, tlsPolicy, 0o640); err != nil { + changed, err := writeChanged(mapFile{rd, relay}, mapFile{tr, transport}, mapFile{rc, recipients}, mapFile{tl, tlsPolicy}) + if err != nil || !changed { return err } return p.reload() diff --git a/internal/postfix/postfix.go b/internal/postfix/postfix.go index 8b9c6af..af7bee0 100644 --- a/internal/postfix/postfix.go +++ b/internal/postfix/postfix.go @@ -69,7 +69,8 @@ func (p *Postfix) RebuildSenderLoginMaps(bindings []Binding) error { if err != nil { return err } - if err := writeFileAtomic(p.senderLoginMapsPath, content, 0o640); err != nil { + changed, err := writeChanged(mapFile{p.senderLoginMapsPath, content}) + if err != nil || !changed { return err } return p.reload() diff --git a/internal/postfix/postfix_test.go b/internal/postfix/postfix_test.go index 6b0f747..2b32719 100644 --- a/internal/postfix/postfix_test.go +++ b/internal/postfix/postfix_test.go @@ -132,3 +132,33 @@ func TestRebuildRejectsUnsafeWithoutWriting(t *testing.T) { t.Errorf("reload called %d times, want 1 (no reload on failure)", *reloads) } } + +// A rebuild that changes no file writes nothing and asks no reload; a change +// in any one file reloads once (security audit 2026-10-17, W6). +func TestARebuildThatChangesNothingDoesNotReload(t *testing.T) { + p, reloads := newTestPostfix(t) + b := []Binding{{"@example.com", "app1"}} + for i := 0; i < 3; i++ { + if err := p.RebuildSenderLoginMaps(b); err != nil { + t.Fatal(err) + } + } + if *reloads != 1 { + t.Errorf("three identical sender-map rebuilds reloaded %d times, want 1", *reloads) + } + routes := []InboundRoute{{Domain: "in.example.com", Host: "mx.example.net", Port: 25, TLSMode: "may", RecipientMode: "any"}} + for i := 0; i < 2; i++ { + if err := p.RebuildInboundMaps(routes); err != nil { + t.Fatal(err) + } + } + if *reloads != 2 { + t.Errorf("two identical inbound rebuilds after it: %d reloads in all, want 2", *reloads) + } + if err := p.RebuildSenderLoginMaps(append(b, Binding{"@other.example", "app2"})); err != nil { + t.Fatal(err) + } + if *reloads != 3 { + t.Errorf("a changed map: %d reloads in all, want 3", *reloads) + } +} diff --git a/internal/postfix/write.go b/internal/postfix/write.go index 939a67f..10cde12 100644 --- a/internal/postfix/write.go +++ b/internal/postfix/write.go @@ -1,6 +1,7 @@ package postfix import ( + "bytes" "os" "github.com/mixeme/selfpost/internal/atomicfile" @@ -12,3 +13,28 @@ import ( func writeFileAtomic(path string, data []byte, perm os.FileMode) error { return atomicfile.Write(path, data, perm) } + +// mapFile is one generated table and what it should hold. +type mapFile struct { + path string + data []byte +} + +// writeChanged writes each file whose content differs from what is on disk +// and says whether any did. A save that changes nothing Postfix reads - a +// domain user saving a form as it was, or changing a field no map holds - +// then writes nothing and asks no reload: every save used to reload Postfix, +// at whatever rate a user cared to press (security audit 2026-10-17, W6). +func writeChanged(files ...mapFile) (bool, error) { + changed := false + for _, f := range files { + if old, err := os.ReadFile(f.path); err == nil && bytes.Equal(old, f.data) { + continue + } + if err := writeFileAtomic(f.path, f.data, 0o640); err != nil { + return changed, err + } + changed = true + } + return changed, nil +} diff --git a/internal/web/auth/auth.go b/internal/web/auth/auth.go index 633a3ac..7226d14 100644 --- a/internal/web/auth/auth.go +++ b/internal/web/auth/auth.go @@ -54,6 +54,8 @@ type Module struct { // ignored (links.go, sendReset). resetDayLimiter *rateLimiter resetNoticeLimiter *rateLimiter + // recheckLimiter counts the DNS re-checks a user forces (AllowDNSRecheck). + recheckLimiter *rateLimiter proxyWatch proxyWatch refusals *refusalLog @@ -98,6 +100,7 @@ func New(st *store.Store, cfg Config, v *view.Engine, setupTokenPath string) *Mo // silence for the rest of the day (security audit 2026-10-17, A3). resetDayLimiter: newRateLimiter(resetsPerDay, 24*time.Hour), resetNoticeLimiter: newRateLimiter(1, 24*time.Hour), + recheckLimiter: newRateLimiter(20, 15*time.Minute), trustedProxies: cfg.TrustedProxyCIDRs, } m.setupLimiter.startSweeper() @@ -112,6 +115,7 @@ func New(st *store.Store, cfg Config, v *view.Engine, setupTokenPath string) *Mo m.mailLimiter.startSweeper() m.resetDayLimiter.startSweeper() m.resetNoticeLimiter.startSweeper() + m.recheckLimiter.startSweeper() // Made now rather than at the first unknown name, which would otherwise // pay for two bcrypt runs (review of audit stage 4). go noSuchUserHash() @@ -131,6 +135,14 @@ func (m *Module) AllowPasswordChange(userID int64) bool { return m.passwordLimiter.Allow("user:" + strconv.FormatInt(userID, 10)) } +// AllowDNSRecheck is whether a user may make the DNS checks again, past the +// five-minute cache: each re-check is up to fourteen resolver queries, and a +// domain user could press it without end (security audit 2026-10-17, M9). +// Twenty in fifteen minutes per user. +func (m *Module) AllowDNSRecheck(userID int64) bool { + return m.recheckLimiter.Allow("user:" + strconv.FormatInt(userID, 10)) +} + // PasswordCheckPassed gives back the attempt AllowPasswordChange took for a // check that found the right password: only wrong guesses count, so an // administrator who adds ten users in a quarter of an hour is not locked out diff --git a/internal/web/auth/limitkeys_test.go b/internal/web/auth/limitkeys_test.go index f14343f..f0876ab 100644 --- a/internal/web/auth/limitkeys_test.go +++ b/internal/web/auth/limitkeys_test.go @@ -114,3 +114,21 @@ func TestProxyWatchAndRefusalsAreLogged(t *testing.T) { t.Errorf("refusals logged %d times (want 5), or the name unescaped:\n%s", n, got) } } + +// A user forces the DNS checks twenty times in fifteen minutes, then sees +// the cached result (security audit 2026-10-17, M9); another user is not +// affected. +func TestDNSRechecksArePerUser(t *testing.T) { + m := testModule(t, false) + for i := 0; i < 20; i++ { + if !m.AllowDNSRecheck(1) { + t.Fatalf("re-check %d refused", i+1) + } + } + if m.AllowDNSRecheck(1) { + t.Error("the 21st re-check was allowed") + } + if !m.AllowDNSRecheck(2) { + t.Error("another user's first re-check was refused") + } +} diff --git a/internal/web/handlers/handlers_apps.go b/internal/web/handlers/handlers_apps.go index 7c3381e..f04794f 100644 --- a/internal/web/handlers/handlers_apps.go +++ b/internal/web/handlers/handlers_apps.go @@ -273,10 +273,26 @@ func (h *Handlers) HandleDomainDNSRecheck(w http.ResponseWriter, r *http.Request http.Error(w, "internal error", http.StatusInternalServerError) return } + if !h.allowRecheck(r) { + http.Redirect(w, r, fmt.Sprintf("/outbound/domains/%d", d.ID), http.StatusSeeOther) + return + } h.domainDNS(d, record, d.DMARCRua, true) http.Redirect(w, r, fmt.Sprintf("/outbound/domains/%d?rechecked=1", d.ID), http.StatusSeeOther) } +// allowRecheck is whether the signed-in user may force the DNS checks now +// (auth.AllowDNSRecheck). Past the limit the page shows the cached result, +// without saying it re-checked; the log says why. +func (h *Handlers) allowRecheck(r *http.Request) bool { + p, ok := h.principal(r) + if !ok || h.auth == nil || h.auth.AllowDNSRecheck(p.ID) { + return true + } + logf("panel: user %d: DNS re-checks asked for too often; the cached result is shown", p.ID) + return false +} + // limitChoice names which of the form's three limit choices an application has: // the domain limit (none of its own), manual, or auto. func limitChoice(rl store.RateLimit, configured bool) string { diff --git a/internal/web/handlers/handlers_apps_test.go b/internal/web/handlers/handlers_apps_test.go index 559e529..a2dc53f 100644 --- a/internal/web/handlers/handlers_apps_test.go +++ b/internal/web/handlers/handlers_apps_test.go @@ -20,6 +20,8 @@ var offlineDNS = dnscheck.New([]string{"127.0.0.1:9"}) type nopSenderMaps struct{} +func (nopSenderMaps) Reload() error { return nil } + func (nopSenderMaps) RebuildSenderLoginMaps([]postfix.Binding) error { return nil } // appStand is a panel with one sending domain and a working application diff --git a/internal/web/handlers/handlers_domains.go b/internal/web/handlers/handlers_domains.go index fefcf4e..53988b6 100644 --- a/internal/web/handlers/handlers_domains.go +++ b/internal/web/handlers/handlers_domains.go @@ -206,6 +206,9 @@ func (h *Handlers) HandleReload(w http.ResponseWriter, r *http.Request) { if h.dmarc != nil && h.dmarc.Enabled() { steps = append(steps, step{"the DMARC report maps", h.dmarc.Resync}) } + // A rebuild that changed nothing asked no reload (security audit + // 2026-10-17, W6): this button re-reads whatever is on disk. + steps = append(steps, step{"Postfix's reload", h.apps.ReloadPostfix}) for _, s := range steps { if err := s.run(); err != nil { logf("panel: manual reload (%s): %v", s.name, err) diff --git a/internal/web/handlers/handlers_errors_test.go b/internal/web/handlers/handlers_errors_test.go index 097fe77..fcca3e4 100644 --- a/internal/web/handlers/handlers_errors_test.go +++ b/internal/web/handlers/handlers_errors_test.go @@ -26,6 +26,8 @@ const savedElsewhere = "Check the logs and try again." type failingSenderMaps struct{} +func (failingSenderMaps) Reload() error { return nil } + func (failingSenderMaps) RebuildSenderLoginMaps([]postfix.Binding) error { return errors.New("write /srv/" + leak + "/sender_login_maps: permission denied") } diff --git a/internal/web/handlers/handlers_inbound.go b/internal/web/handlers/handlers_inbound.go index 58efbc1..25f9e20 100644 --- a/internal/web/handlers/handlers_inbound.go +++ b/internal/web/handlers/handlers_inbound.go @@ -275,6 +275,10 @@ func (h *Handlers) HandleInboundDNSRecheck(w http.ResponseWriter, r *http.Reques if !ok { return } + if !h.allowRecheck(r) { + http.Redirect(w, r, fmt.Sprintf("/inbound/domains/%d", d.ID), http.StatusSeeOther) + return + } if h.dns != nil && h.cfg.Hostname != "" { h.dns.InboundMX(d.Name, h.cfg.Hostname, true) } diff --git a/internal/web/handlers/handlers_settings.go b/internal/web/handlers/handlers_settings.go index 85c826e..5e487aa 100644 --- a/internal/web/handlers/handlers_settings.go +++ b/internal/web/handlers/handlers_settings.go @@ -203,6 +203,10 @@ func (h *Handlers) HandleSettingsDNSRecheck(w http.ResponseWriter, r *http.Reque if _, ok := h.requireGlobal(w, r); !ok { return } + if !h.allowRecheck(r) { + http.Redirect(w, r, "/server/settings", http.StatusSeeOther) + return + } h.dns.Server(h.cfg.Hostname, true) h.serverMail(true) http.Redirect(w, r, "/server/settings?rechecked=1", http.StatusSeeOther)