diff --git a/CHANGELOG.md b/CHANGELOG.md index bff40fc..421f4bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,26 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version principal's own domains and applications before the query runs, so a hand-written URL cannot widen the scope. Global administrators are unaffected. +- mail (level-2 rate limit): the ceiling is no longer overshot by messages that + arrive at the same instant. The milter counted the stored and in-flight + messages and reserved its own slot in two separate steps, so several SMTP + sessions could pass the same check before any of them had reserved. Counting + and reserving now happen as one operation, and the ceiling is handed out + exactly as many times as configured. Postfix's level-1 limit remains the + backstop and the level-2 check stays fail-open on store errors. + +- panel (sign-in): a session that cannot be written to the database no longer + produces a session cookie. The login used to log the failure, set the cookie + and redirect to the dashboard, leaving the browser looking signed in while + every request bounced back to `/login`; it now fails closed with an error on + the sign-in page. + +- panel (applications): deleting an application removes its SASL credentials + before its registry row. If `saslpasswd2` fails, the application stays listed + and the delete can be retried, instead of leaving a hidden account that could + still authenticate to Postfix. This matches the order domain deletion already + used. + ### Changed - ci: gofmt on eight files that failed the formatting workflow check (panel diff --git a/docs/plans/code-review.md b/docs/plans/code-review.md index b9a7685..3bd7aa6 100644 --- a/docs/plans/code-review.md +++ b/docs/plans/code-review.md @@ -504,11 +504,15 @@ the list, the fragment, or query parameters. `go test ./...` green. **Model: Opus.** -- [ ] `inflight.tryAdmit` (count + reserve under one lock). Extend milter +- [x] `inflight.tryAdmit` (count + reserve under one lock). Extend milter tests with overlapping `MailFrom` (true concurrency, not sequential). -- [ ] `sessionStore.Create` returns an error; login does not set a cookie on + Two tests: concurrent `MailFrom` sessions gated so they all read the + stored count before anyone reserves (exactly one admitted), and a + saturation test on `tryAdmit` that overshoots the ceiling whenever count + and reserve are separate critical sections. +- [x] `sessionStore.Create` returns an error; login does not set a cookie on failure. -- [ ] `app.Service.Delete`: SASL (and rate-limit row) before or compensating +- [x] `app.Service.Delete`: SASL (and rate-limit row) before or compensating with the registry row; match domain-delete ordering. Test the failure path with a fake SASL that errors. diff --git a/internal/app/service.go b/internal/app/service.go index c4bb392..f691cb3 100644 --- a/internal/app/service.go +++ b/internal/app/service.go @@ -166,14 +166,24 @@ func (s *Service) RegeneratePassword(id int64) (string, error) { // Delete removes an application: its SASL account, its registry row (and address // rows via cascade) and its sender-map bindings, then reloads Postfix (spec // 7.2.8). The domain and other applications are untouched. +// +// The order matches domain deletion: the SASL account goes first, while the +// login is still in the registry. Dropping the row first would, on a +// saslpasswd2 failure, leave an account that can still authenticate to Postfix +// but that the panel no longer knows about — an orphan no operator can see or +// remove. Failing before the row is deleted is recoverable: the application is +// still listed and the delete can be retried. func (s *Service) Delete(id int64) error { - a, err := s.store.DeleteApplication(id) + a, err := s.store.GetApplication(id) if err != nil { return err } if err := s.sasl.Delete(a.Login); err != nil { return err } + if _, err := s.store.DeleteApplication(id); err != nil { + return err + } // Drop the application's level-2 limit, if any (guide § Rate limiting); // rate_limits has no cascade of its own. if err := s.store.DeleteRateLimit(store.RateLimitScopeApp, id); err != nil { diff --git a/internal/app/service_test.go b/internal/app/service_test.go index bc99516..e598c76 100644 --- a/internal/app/service_test.go +++ b/internal/app/service_test.go @@ -181,6 +181,37 @@ func TestServiceDelete(t *testing.T) { } } +// If sasldb2 cannot be updated the application must stay in the registry: an +// account that still authenticates but has no panel row is invisible to the +// operator and cannot be deleted again. +func TestServiceDeleteKeepsRowWhenSASLFails(t *testing.T) { + svc, st, rec, _ := newServiceHarness(t) + d := addDomain(t, st, "example.com") + a, _, err := svc.Create(d.ID, "app1", store.AddressModeWildcard, nil) + if err != nil { + t.Fatal(err) + } + + rec.failNext = true // saslpasswd2 -d fails + if err := svc.Delete(a.ID); err == nil { + t.Fatal("Delete reported success although the SASL account was not removed") + } + apps, _ := st.ListApplicationsByDomain(d.ID) + if len(apps) != 1 { + t.Fatalf("registry row dropped while the SASL account can still authenticate: %+v", apps) + } + if _, ok := rec.set["app1"]; !ok { + t.Fatal("SASL account gone despite the failure — the harness no longer proves the ordering") + } + // The delete is retryable now that the row is still there. + if err := svc.Delete(a.ID); err != nil { + t.Fatalf("retried Delete: %v", err) + } + if _, ok := rec.set["app1"]; ok { + t.Error("SASL account not deleted on retry") + } +} + func TestServiceUpdateMode(t *testing.T) { svc, st, _, maps := newServiceHarness(t) d := addDomain(t, st, "example.com") diff --git a/internal/milter/inflight.go b/internal/milter/inflight.go index 78ca52b..e1a4b30 100644 --- a/internal/milter/inflight.go +++ b/internal/milter/inflight.go @@ -36,6 +36,29 @@ type inflight struct { m map[string]map[*reservation]struct{} } +// tryAdmit decides whether one more message may be sent for key and, if so, +// claims its slot — both under a single lock. Counting and reserving in two +// separate critical sections would let two SMTP sessions that reach MAIL FROM +// at the same moment observe the same total and each take the last free slot, +// which is exactly the overshoot the in-flight registry exists to prevent. +// +// stored is the count the send log already holds for the limit's window and max +// is the ceiling; the caller supplies both because only it can query the store. +// The returned total is what was measured, for the refusal log line. +func (f *inflight) tryAdmit(key string, since time.Time, stored, max int64) (*reservation, int64, bool) { + if f == nil { + return nil, stored, stored < max // no in-flight accounting (tests) + } + f.mu.Lock() + defer f.mu.Unlock() + + total := stored + f.countLocked(key, since) + if total >= max { + return nil, total, false + } + return f.reserveLocked(key), total, true +} + // count returns how many reservations for key were taken within the limit's // window (at or after since), pruning any that outlived reservationTTL. func (f *inflight) count(key string, since time.Time) int64 { @@ -45,6 +68,11 @@ func (f *inflight) count(key string, since time.Time) int64 { f.mu.Lock() defer f.mu.Unlock() + return f.countLocked(key, since) +} + +// countLocked is count's body; the caller holds mu. +func (f *inflight) countLocked(key string, since time.Time) int64 { set := f.m[key] cutoff := time.Now().Add(-reservationTTL) var n int64 @@ -63,14 +91,8 @@ func (f *inflight) count(key string, since time.Time) int64 { return n } -// reserve claims a slot for key until the message is recorded or released. -func (f *inflight) reserve(key string) *reservation { - if f == nil { - return nil - } - f.mu.Lock() - defer f.mu.Unlock() - +// reserveLocked is reserve's body; the caller holds mu. +func (f *inflight) reserveLocked(key string) *reservation { if f.m == nil { f.m = make(map[string]map[*reservation]struct{}) } diff --git a/internal/milter/milter_test.go b/internal/milter/milter_test.go index d2d2a34..0d2e167 100644 --- a/internal/milter/milter_test.go +++ b/internal/milter/milter_test.go @@ -3,6 +3,7 @@ package milter import ( "errors" "net" + "sync" "testing" "time" @@ -15,7 +16,10 @@ import ( // swallows recorder errors and still accepts the message. By default it reports // no configured rate limit, so the level-2 check is inert unless a test sets // limits (see fakeRecorder fields). +// mu guards the recorded slices so several sessions may drive one recorder +// concurrently, as they do in the real server. type fakeRecorder struct { + mu sync.Mutex entries []store.SendLogEntry rejected []store.SendLogEntry fail bool @@ -27,17 +31,25 @@ type fakeRecorder struct { counts map[string]int64 lookupErr error countErr error + + // onCount, if set, runs inside CountMessages. It lets a test hold every + // racing session at the store lookup until they can all proceed together. + onCount func() } func (f *fakeRecorder) InsertQueued(e store.SendLogEntry) error { if f.fail { return errors.New("boom") } + f.mu.Lock() + defer f.mu.Unlock() f.entries = append(f.entries, e) return nil } func (f *fakeRecorder) InsertRejected(e store.SendLogEntry) error { + f.mu.Lock() + defer f.mu.Unlock() f.rejected = append(f.rejected, e) return nil } @@ -57,6 +69,9 @@ func (f *fakeRecorder) CountMessages(scope, ref string, _ time.Time) (int64, err if f.countErr != nil { return 0, f.countErr } + if f.onCount != nil { + f.onCount() + } return f.counts[scope+"|"+ref], nil } @@ -415,6 +430,58 @@ func TestRateLimitCountsInFlightMessages(t *testing.T) { } } +// The case above is sequential: the second session reads the stored count after +// the first has already reserved. Here every session reads it first — the gate +// holds them all inside the lookup — which is what concurrent SMTP connections +// actually do. However many then race for the single free slot, exactly one may +// pass. (TestTryAdmitHandsOutEachSlotOnce is the test that fails when counting +// and reserving are not one step; this one pins the session-level behaviour.) +func TestRateLimitAdmitsOnlyOneRacingSession(t *testing.T) { + rec := limitedRecorder(4) // one below the ceiling of 5 + fl := &inflight{} + gate := make(chan struct{}) + + // Every session is held inside the stored-count lookup until all of them + // have read it, which is the state the race needs: none of them can see + // another's reservation, because none has been taken yet. + const racers = 32 + var atCount, done sync.WaitGroup + atCount.Add(racers) + go func() { atCount.Wait(); close(gate) }() + rec.onCount = func() { atCount.Done(); <-gate } + + responses := make([]milter.Response, racers) + for i := range racers { + done.Add(1) + go func() { + defer done.Done() + // Connect is skipped so every goroutine starts from the same point; + // the client IP is what Connect would have captured. + s := &session{rec: rec, flight: fl, clientIP: limitIP} + resp, err := s.MailFrom("a@example.com", mods(map[string]string{"auth_authen": "app1"})) + if err != nil { + resp = nil // reported as a missing Continue below + } + responses[i] = resp + }() + } + done.Wait() + + admitted := 0 + for _, resp := range responses { + if resp == milter.RespContinue { + admitted++ + } + } + if admitted != 1 { + t.Fatalf("%d of %d racing sessions admitted, want exactly 1 — the last slot was handed out twice", + admitted, racers) + } + if n := fl.count(store.RateLimitScopeDomain+"|example.com", time.Now().Add(-time.Hour)); n != 1 { + t.Fatalf("in-flight reservations = %d, want 1", n) + } +} + // Once the message is recorded the stored count sees it, so its reservation // must be given back — otherwise it would be counted twice and the ceiling // would drift closed. @@ -477,12 +544,55 @@ func TestRefusalDoesNotLeaveDomainReservation(t *testing.T) { } } +// The ceiling is handed out exactly max times however the sessions interleave. +// Counting and reserving in two critical sections passes the sequential tests +// above and still overshoots here, because between one session's count and its +// reservation any number of others can pass the same check. +func TestTryAdmitHandsOutEachSlotOnce(t *testing.T) { + const ( + max = 500 + workers = 8 + ) + fl := &inflight{} + since := time.Now().Add(-time.Hour) + start := make(chan struct{}) + admitted := make([]int, workers) + var wg sync.WaitGroup + for i := range workers { + wg.Add(1) + go func() { + defer wg.Done() + <-start + for { + _, _, ok := fl.tryAdmit("domain|example.com", since, 0, max) + if !ok { + return + } + admitted[i]++ + } + }() + } + close(start) + wg.Wait() + + total := 0 + for _, n := range admitted { + total += n + } + if total != max { + t.Fatalf("admitted %d messages under a ceiling of %d", total, max) + } +} + // The in-flight count only covers the limit's own window: a reservation older // than it (a session stuck mid-DATA for longer than the window) must not be // counted against a window it no longer belongs to. func TestInflightIgnoresReservationsOutsideWindow(t *testing.T) { fl := &inflight{} - r := fl.reserve("domain|example.com") + r, _, ok := fl.tryAdmit("domain|example.com", time.Now().Add(-time.Hour), 0, 1) + if !ok { + t.Fatal("tryAdmit refused the first message under a ceiling of 1") + } r.at = time.Now().Add(-time.Minute) if n := fl.count("domain|example.com", time.Now().Add(-time.Hour)); n != 1 { diff --git a/internal/milter/ratelimit.go b/internal/milter/ratelimit.go index 913304b..291c79b 100644 --- a/internal/milter/ratelimit.go +++ b/internal/milter/ratelimit.go @@ -54,22 +54,23 @@ func (s *session) overLimit() bool { } // enforceLimit counts recent messages for scope/ref and refuses when at or -// above the ceiling. On admit it reserves an in-flight slot on the session. +// above the ceiling. The stored count and the in-flight slots are weighed and +// the admitted message's own slot is taken in one atomic step (tryAdmit), so +// two sessions racing at MAIL FROM cannot both claim the last free slot. func (s *session) enforceLimit(scope, ref string, rl store.RateLimit) bool { since := time.Now().Add(-time.Duration(rl.WindowSeconds) * time.Second) - n, err := s.rec.CountMessages(scope, ref, since) + stored, err := s.rec.CountMessages(scope, ref, since) if err != nil { log.Printf("journal-milter: rate-limit count %s %q: %v (fail-open)", scope, ref, err) return false } - key := scope + "|" + ref - n += s.flight.count(key, since) - if n >= int64(rl.MaxMessages) { + r, n, ok := s.flight.tryAdmit(scope+"|"+ref, since, stored, int64(rl.MaxMessages)) + if !ok { log.Printf("journal-milter: %s %q over limit: %d/%d in %ds from %s — refusing 4xx", scope, ref, n, rl.MaxMessages, rl.WindowSeconds, s.clientIP) return true } - s.reserved = append(s.reserved, s.flight.reserve(key)) + s.reserved = append(s.reserved, r) return false } diff --git a/internal/web/auth/auth_test.go b/internal/web/auth/auth_test.go index 2c59422..157c87e 100644 --- a/internal/web/auth/auth_test.go +++ b/internal/web/auth/auth_test.go @@ -1,13 +1,17 @@ package auth import ( + "database/sql" "net/http" "net/http/httptest" + "net/url" "path/filepath" "strings" "testing" "time" + "golang.org/x/crypto/bcrypt" + "github.com/mixeme/selfpost/internal/store" "github.com/mixeme/selfpost/internal/web/view" ) @@ -22,6 +26,15 @@ func newTestSessionStore(t *testing.T) *sessionStore { return newSessionStore(st, 7*24*time.Hour) } +func mustCreate(t *testing.T, s *sessionStore, username string) string { + t.Helper() + token, err := s.Create(username) + if err != nil { + t.Fatalf("create session: %v", err) + } + return token +} + func mustView(t *testing.T) *view.Engine { t.Helper() v, err := view.New("test") @@ -86,7 +99,7 @@ func TestSessionTokenIgnoresTheOtherName(t *testing.T) { func TestRequireAuthRejectsDuplicateCookies(t *testing.T) { m := testModule(t, false) - token := m.sessions.Create("admin") + token := mustCreate(t, m.sessions, "admin") reached := false h := m.RequireAuth(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached = true })) @@ -107,7 +120,7 @@ func TestRequireAuthRejectsDuplicateCookies(t *testing.T) { func TestLogoutClearsBothCookieNames(t *testing.T) { m := testModule(t, true) - token := m.sessions.Create("admin") + token := mustCreate(t, m.sessions, "admin") r := httptest.NewRequest(http.MethodPost, "http://panel.example.com/logout", nil) r.Host = "panel.example.com" @@ -132,9 +145,63 @@ func TestLogoutClearsBothCookieNames(t *testing.T) { } } +// A session that could not be stored must not turn into a cookie: the browser +// would look signed in, and every request it made would be bounced to /login +// with no explanation. Only the sessions table is broken here, so the request +// gets past the user lookup and password check and fails exactly where the +// session is written. +func TestLoginSetsNoCookieWhenTheSessionCannotBeStored(t *testing.T) { + path := filepath.Join(t.TempDir(), "test.db") + st, err := store.Open(path) + if err != nil { + t.Fatalf("open store: %v", err) + } + t.Cleanup(func() { st.Close() }) + hash, err := bcrypt.GenerateFromPassword([]byte("correct-horse-battery"), bcrypt.MinCost) + if err != nil { + t.Fatalf("hash password: %v", err) + } + if err := st.CreateGlobalUser("admin", string(hash)); err != nil { + t.Fatalf("create user: %v", err) + } + dropSessionsTable(t, path) + + m := New(st, Config{}, mustView(t), "") + form := url.Values{"username": {"admin"}, "password": {"correct-horse-battery"}} + r := httptest.NewRequest(http.MethodPost, "http://panel.example.com/login", + strings.NewReader(form.Encode())) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + m.HandleLogin(rec, r) + + if got := rec.Header().Values("Set-Cookie"); len(got) != 0 { + t.Errorf("a session cookie was issued for a session that was never stored: %v", got) + } + if rec.Code != http.StatusInternalServerError { + t.Errorf("status = %d, want 500 (the login failed)", rec.Code) + } + if loc := rec.Header().Get("Location"); loc != "" { + t.Errorf("the browser was sent to %q as if it were signed in", loc) + } +} + +// dropSessionsTable breaks session persistence while leaving the rest of the +// schema usable. The SQLite driver is registered by internal/store. +func dropSessionsTable(t *testing.T, path string) { + t.Helper() + db, err := sql.Open("sqlite", "file:"+path+"?_pragma=busy_timeout(5000)") + if err != nil { + t.Fatalf("open database directly: %v", err) + } + defer db.Close() + if _, err := db.Exec("DROP TABLE sessions"); err != nil { + t.Fatalf("drop sessions table: %v", err) + } +} + func TestSessionRename(t *testing.T) { s := newTestSessionStore(t) - token := s.Create("admin") + token := mustCreate(t, s, "admin") s.Rename(token, "operator") @@ -149,8 +216,8 @@ func TestSessionRename(t *testing.T) { func TestSessionDestroyOthers(t *testing.T) { s := newTestSessionStore(t) - keep := s.Create("admin") - other := s.Create("admin") + keep := mustCreate(t, s, "admin") + other := mustCreate(t, s, "admin") s.DestroyOthers(keep) @@ -165,7 +232,7 @@ func TestSessionDestroyOthers(t *testing.T) { func TestSessionLookupRejectsExpired(t *testing.T) { s := newTestSessionStore(t) s.idle = -time.Minute - token := s.Create("admin") + token := mustCreate(t, s, "admin") if _, ok := s.Lookup(token); ok { t.Fatal("expired session was accepted") @@ -174,7 +241,7 @@ func TestSessionLookupRejectsExpired(t *testing.T) { func TestSessionTouchThrottled(t *testing.T) { s := newTestSessionStore(t) - token := s.Create("admin") + token := mustCreate(t, s, "admin") if s.Touch(token) { t.Fatal("touch renewed a session created moments ago") diff --git a/internal/web/auth/handlers.go b/internal/web/auth/handlers.go index 0b7e762..331d039 100644 --- a/internal/web/auth/handlers.go +++ b/internal/web/auth/handlers.go @@ -120,7 +120,12 @@ func (m *Module) submitLogin(w http.ResponseWriter, r *http.Request) { return } - token := m.sessions.Create(user.Username) + token, err := m.sessions.Create(user.Username) + if err != nil { + logf("panel: login: create session failed: %v", err) + m.renderLogin(w, http.StatusInternalServerError, "Internal error. Please try again.") + return + } m.setSessionCookie(w, token) http.Redirect(w, r, "/", http.StatusSeeOther) } diff --git a/internal/web/auth/session.go b/internal/web/auth/session.go index 29c588d..dace9ef 100644 --- a/internal/web/auth/session.go +++ b/internal/web/auth/session.go @@ -41,17 +41,22 @@ func hashToken(token string) string { return hex.EncodeToString(sum[:]) } -// Create issues a new session for username and returns its token. -func (s *sessionStore) Create(username string) string { +// Create issues a new session for username and returns its token. It fails +// closed: if the row cannot be written the caller gets an error and must not +// hand out a cookie, because a token that is not in the database looks like a +// signed-in browser while every request it makes bounces back to /login. +func (s *sessionStore) Create(username string) (string, error) { token := randomToken(32) now := time.Now() if err := s.store.CreateSession(hashToken(token), username, now.Add(s.idle)); err != nil { - logf("panel: session: create failed: %v", err) + return "", err } + // Pruning is housekeeping: the new session is already valid, so a failure + // here is logged and does not fail the login. if _, err := s.store.DeleteExpiredSessions(now); err != nil { logf("panel: session: prune expired failed: %v", err) } - return token + return token, nil } // Lookup returns the session username for a token if it exists and is