app: the sender map's rebuild is serialised (audit W3)
Security audit 2026-10-17, finding W3 (low), stage 1. Resync read the bindings and wrote the map with no lock (the selector table has tableMu); two application changes at once could write in the order opposite to their reads and leave the map stale - an application whose binding was missing refused with reject_sender_login_mismatch until the next change. Now a mutex on the Service holds the read, the write and the reload together. Test: TestResyncRunsOneAtATime (eight at once, never two inside the rebuild). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
- **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
|
belong to a domain by name, and a domain deleted and added again showed
|
||||||
its old reports to whoever it was assigned to then.
|
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
|
- **A stranger's recipient no longer writes an Inbound › Log row** (audit
|
||||||
M2): Postfix quotes a refused recipient in its reply with the quotes
|
M2): Postfix quotes a refused recipient in its reply with the quotes
|
||||||
removed, so `RCPT TO:<"; from=<x> to=<y> …"@domain>` spelled a second
|
removed, so `RCPT TO:<"; from=<x> to=<y> …"@domain>` spelled a second
|
||||||
|
|||||||
@@ -750,8 +750,8 @@ the argument.
|
|||||||
**Sonnet**, 0.5 h. *Done 2026-10-17:* in `store.DeleteDomain`, one
|
**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
|
transaction with the domain; TestDeletingADomainDeletesItsReports. (The `domain_id` form is better and 2 h; take it if
|
||||||
stage 1 is split across two sittings.)
|
stage 1 is split across two sittings.)
|
||||||
- [ ] **W3.** A mutex around `Resync`'s read + write + reload. — **Sonnet**,
|
- [x] **W3.** A mutex around `Resync`'s read + write + reload. — **Sonnet**,
|
||||||
0.5 h.
|
0.5 h. *Done 2026-10-17:* TestResyncRunsOneAtATime.
|
||||||
|
|
||||||
### Stage 2 — the key group (~2.5 h) → patch release, then the test server's `certs/`
|
### Stage 2 — the key group (~2.5 h) → patch release, then the test server's `certs/`
|
||||||
|
|
||||||
|
|||||||
@@ -6,6 +6,8 @@
|
|||||||
package app
|
package app
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"sync"
|
||||||
|
|
||||||
"github.com/mixeme/selfpost/internal/postfix"
|
"github.com/mixeme/selfpost/internal/postfix"
|
||||||
"github.com/mixeme/selfpost/internal/store"
|
"github.com/mixeme/selfpost/internal/store"
|
||||||
)
|
)
|
||||||
@@ -25,6 +27,12 @@ type Service struct {
|
|||||||
store *store.Store
|
store *store.Store
|
||||||
sasl *SASLDB
|
sasl *SASLDB
|
||||||
pf SenderMaps
|
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
|
// 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 manual reload button; it doubles as recovery if the map ever drifts
|
||||||
// from the database.
|
// from the database.
|
||||||
func (s *Service) Resync() error {
|
func (s *Service) Resync() error {
|
||||||
|
s.resyncMu.Lock()
|
||||||
|
defer s.resyncMu.Unlock()
|
||||||
bindings, err := s.store.ListBindings()
|
bindings, err := s.store.ListBindings()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
|
|||||||
@@ -3,7 +3,10 @@ package app
|
|||||||
import (
|
import (
|
||||||
"errors"
|
"errors"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
|
"sync"
|
||||||
|
"sync/atomic"
|
||||||
"testing"
|
"testing"
|
||||||
|
"time"
|
||||||
|
|
||||||
"github.com/mixeme/selfpost/internal/postfix"
|
"github.com/mixeme/selfpost/internal/postfix"
|
||||||
"github.com/mixeme/selfpost/internal/store"
|
"github.com/mixeme/selfpost/internal/store"
|
||||||
@@ -505,3 +508,44 @@ func TestSaveSettingsIsAllOrNothing(t *testing.T) {
|
|||||||
t.Errorf("unknown application = %v, want ErrApplicationNotFound", err)
|
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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user