diff --git a/CHANGELOG.md b/CHANGELOG.md index 1920354..d9b37fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,9 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version - **A domain's DMARC reports are deleted with it** (audit A5/W2): reports belong to a domain by name, and a domain deleted and added again showed its old reports to whoever it was assigned to then. +- **The sender map is rebuilt one change at a time** (audit W3): two + application changes at once could leave the map without the newer one + until the next change. - **A stranger's recipient no longer writes an Inbound › Log row** (audit M2): Postfix quotes a refused recipient in its reply with the quotes removed, so `RCPT TO:<"; from= to= …"@domain>` spelled a second diff --git a/docs/security-audit-2026-10-17.md b/docs/security-audit-2026-10-17.md index 19f5764..df5faf7 100644 --- a/docs/security-audit-2026-10-17.md +++ b/docs/security-audit-2026-10-17.md @@ -750,8 +750,8 @@ the argument. **Sonnet**, 0.5 h. *Done 2026-10-17:* in `store.DeleteDomain`, one transaction with the domain; TestDeletingADomainDeletesItsReports. (The `domain_id` form is better and 2 h; take it if stage 1 is split across two sittings.) -- [ ] **W3.** A mutex around `Resync`'s read + write + reload. — **Sonnet**, - 0.5 h. +- [x] **W3.** A mutex around `Resync`'s read + write + reload. — **Sonnet**, + 0.5 h. *Done 2026-10-17:* TestResyncRunsOneAtATime. ### Stage 2 — the key group (~2.5 h) → patch release, then the test server's `certs/` diff --git a/internal/app/service.go b/internal/app/service.go index 1a18000..538778a 100644 --- a/internal/app/service.go +++ b/internal/app/service.go @@ -6,6 +6,8 @@ package app import ( + "sync" + "github.com/mixeme/selfpost/internal/postfix" "github.com/mixeme/selfpost/internal/store" ) @@ -25,6 +27,12 @@ type Service struct { store *store.Store sasl *SASLDB pf SenderMaps + + // resyncMu makes Resync's read of the bindings and its write of the map + // one step: two application changes at once could otherwise write the + // map in the order opposite to their reads, leaving the older set until + // the next change (security audit 2026-10-17, W3). + resyncMu sync.Mutex } // NewService builds the application service over the shared store, the sasldb2 @@ -227,6 +235,8 @@ func (s *Service) PurgeDomainSASL(domainID int64) error { // from the manual reload button; it doubles as recovery if the map ever drifts // from the database. func (s *Service) Resync() error { + s.resyncMu.Lock() + defer s.resyncMu.Unlock() bindings, err := s.store.ListBindings() if err != nil { return err diff --git a/internal/app/service_test.go b/internal/app/service_test.go index fb69806..15a6ce8 100644 --- a/internal/app/service_test.go +++ b/internal/app/service_test.go @@ -3,7 +3,10 @@ package app import ( "errors" "path/filepath" + "sync" + "sync/atomic" "testing" + "time" "github.com/mixeme/selfpost/internal/postfix" "github.com/mixeme/selfpost/internal/store" @@ -505,3 +508,44 @@ func TestSaveSettingsIsAllOrNothing(t *testing.T) { t.Errorf("unknown application = %v, want ErrApplicationNotFound", err) } } + +// overlapMaps counts rebuilds that run at once. +type overlapMaps struct { + inside, most atomic.Int32 +} + +func (o *overlapMaps) RebuildSenderLoginMaps([]postfix.Binding) error { + n := o.inside.Add(1) + for { + m := o.most.Load() + if n <= m || o.most.CompareAndSwap(m, n) { + break + } + } + time.Sleep(5 * time.Millisecond) + o.inside.Add(-1) + return nil +} + +// Resync's read of the bindings and its write of the map are one step: two +// at once would write in the order opposite to their reads and leave the +// older set in the map (security audit 2026-10-17, W3). +func TestResyncRunsOneAtATime(t *testing.T) { + svc, _, _, _ := newServiceHarness(t) + maps := &overlapMaps{} + svc.pf = maps + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if err := svc.Resync(); err != nil { + t.Error(err) + } + }() + } + wg.Wait() + if got := maps.most.Load(); got != 1 { + t.Errorf("%d rebuilds ran at once, want 1", got) + } +}