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)