milter: the level-2 domain limit is the login's domain's (audit M1)
Security audit 2026-10-17, finding M1, stage 1. overLimit took domainOf(MAIL FROM) whatever the login, and MailFrom ran it with an empty login: with smtpd_delay_reject at its default Postfix runs no sender restriction at MAIL and sends the milter's MAIL event at once, so a stranger on 465/587 with no AUTH, or another tenant's application, could name a victim domain in MAIL FROM and take its slots for the 10-minute in-flight TTL, get the victim's application answered 451, and have a limit row (and the "a rate limit is refusing mail" notification) written under the victim's domain. Now overLimit returns at once without a login and takes the domain from ApplicationByLogin (loginDomain, fail-open like the rest); recordRejected's Domain is the login's too. The send log already counts a domain's messages by the login's domain (store.CountMessages, loginDomainID), so the limit and the count now agree. The null-sender special case is gone: every message is held to its login's domain. Tests: TestRateLimitIsNotReservedByAnotherSendersMailFrom (a stranger with no login; another tenant's application) - fails on the old code at the second MAIL FROM; the domain-limit tests sign in as app1 at example.com. e2e domain_limit_not_reserved_by_a_stranger: a domain with a limit of one, three unauthenticated MAIL FROM naming it on 465, then its own message passes and its second is refused 451. Docs: security.md § Mail path; CHANGELOG; plan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -13,6 +13,11 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version
|
||||
check. Adding a user, setting a password, changing an e-mail or a role on
|
||||
the user form, and deleting a user now ask for your own account password,
|
||||
as a backup does, under the same limit on wrong guesses.
|
||||
- **A domain's rate limit is taken only by its own applications** (audit
|
||||
M1): the level-2 domain limit was read from `MAIL FROM`, which reaches the
|
||||
milter before Postfix holds it to the login, so a client that never
|
||||
signed in, or another tenant's application, could use up a domain's
|
||||
limit. It is now the signed-in application's domain.
|
||||
- **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=<x> to=<y> …"@domain>` spelled a second
|
||||
|
||||
@@ -719,12 +719,14 @@ the argument.
|
||||
|
||||
### Stage 1 — the tenant boundary in the mail path (~7 h) → patch release
|
||||
|
||||
- [ ] **M1.** `overLimit` and `recordRejected` take the domain from the login
|
||||
- [x] **M1.** `overLimit` and `recordRejected` take the domain from the login
|
||||
(`ApplicationByLogin`), return early on an empty login; the reader's two
|
||||
tests (stranger; other tenant) become the regression tests; an e2e sends an
|
||||
unauthenticated `MAIL FROM` for a domain with a limit of 1 and then the
|
||||
domain's own message, which must pass. security.md § Mail path records
|
||||
that an unauthenticated session reserves nothing. — **Opus**, 2 h.
|
||||
*Done 2026-10-17:* TestRateLimitIsNotReservedByAnotherSendersMailFrom;
|
||||
e2e `domain_limit_not_reserved_by_a_stranger`.
|
||||
- [ ] **M3.** De-duplicate recipients in `inboundSession.RcptTo` (case-folded,
|
||||
as the outbound session does); the milter's inserts count toward the prune
|
||||
trigger (a counter in the store beside `InsertInboundSpam`, the prune
|
||||
|
||||
@@ -399,6 +399,16 @@ operator can act on it.
|
||||
ask for it at all, an application with a client-address list is refused when
|
||||
no address is known, and neither milter replies anything but "continue" to a
|
||||
message it lets through. `test/e2e` sends two `MAIL FROM` on one connection.
|
||||
- **A level-2 limit is reserved only by its own senders.** The domain limit
|
||||
is the domain of the signed-in application, never the `MAIL FROM`
|
||||
domain, and a session with no login reserves nothing. `MAIL FROM` reaches
|
||||
the journal milter before Postfix holds it to the login (with
|
||||
`smtpd_delay_reject` at its default no sender restriction runs at MAIL),
|
||||
so until `2.14.4` a stranger on 465/587 who never signed in, or another
|
||||
tenant's application, could name a domain there and take its slots for
|
||||
the in-flight TTL - and have a `limit` refusal written under it (security
|
||||
audit 2026-10-17, M1). `test/e2e` names a limited domain in a stranger's
|
||||
`MAIL FROM` and then sends the domain's own message.
|
||||
- **The upstream's answer is an update of a row, not a row**
|
||||
(inbound-upstream-result, `2.10.0`). A delivery line of mail accepted on
|
||||
port 25 changes the result of that message's journal rows and adds
|
||||
|
||||
@@ -221,6 +221,11 @@ func TestBracedMacros(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// app1AtExample is the application the rate-limit tests sign in as: the
|
||||
// domain limit is its login's domain's, whatever MAIL FROM says (audit
|
||||
// 2026-10-17, M1).
|
||||
var app1AtExample = map[string]store.Application{"app1": {Login: "app1", DomainName: "example.com"}}
|
||||
|
||||
// limitIP is the client IP rate-limit tests connect from.
|
||||
const limitIP = "203.0.113.7"
|
||||
|
||||
@@ -249,6 +254,7 @@ func mailFrom(t *testing.T, rec Store, ip, from, login string) milter.Response {
|
||||
|
||||
func TestRateLimitRefusesWhenDomainOverLimit(t *testing.T) {
|
||||
rec := &fakeRecorder{
|
||||
apps: app1AtExample,
|
||||
limits: map[string]store.RateLimit{
|
||||
store.RateLimitScopeDomain + "|example.com": domainLimit(),
|
||||
},
|
||||
@@ -276,6 +282,7 @@ func TestRateLimitRefusesWhenAppOverLimit(t *testing.T) {
|
||||
|
||||
func TestRateLimitAllowsUnderLimit(t *testing.T) {
|
||||
rec := &fakeRecorder{
|
||||
apps: app1AtExample,
|
||||
limits: map[string]store.RateLimit{
|
||||
store.RateLimitScopeDomain + "|example.com": domainLimit(),
|
||||
},
|
||||
@@ -291,6 +298,7 @@ func TestRateLimitAllowsUnderLimit(t *testing.T) {
|
||||
|
||||
func TestRateLimitDomainAppliesToAnyIP(t *testing.T) {
|
||||
rec := &fakeRecorder{
|
||||
apps: app1AtExample,
|
||||
limits: map[string]store.RateLimit{
|
||||
store.RateLimitScopeDomain + "|example.com": domainLimit(),
|
||||
},
|
||||
@@ -304,6 +312,7 @@ func TestRateLimitDomainAppliesToAnyIP(t *testing.T) {
|
||||
|
||||
func TestRateLimitAppSkipsDomainWhenActive(t *testing.T) {
|
||||
rec := &fakeRecorder{
|
||||
apps: app1AtExample,
|
||||
limits: map[string]store.RateLimit{
|
||||
store.RateLimitScopeDomain + "|example.com": {MaxMessages: 1, WindowSeconds: 3600},
|
||||
store.RateLimitScopeApp + "|app1": appLimit(),
|
||||
@@ -364,6 +373,7 @@ func TestAuthIPRestrictOffAllowsAnyIP(t *testing.T) {
|
||||
|
||||
func TestRateLimitInactiveWithoutCeiling(t *testing.T) {
|
||||
rec := &fakeRecorder{
|
||||
apps: app1AtExample,
|
||||
limits: map[string]store.RateLimit{
|
||||
store.RateLimitScopeDomain + "|example.com": {}, // no max/window
|
||||
},
|
||||
@@ -383,6 +393,7 @@ func TestRateLimitFailsOpenOnLookupError(t *testing.T) {
|
||||
|
||||
func TestRateLimitFailsOpenOnCountError(t *testing.T) {
|
||||
rec := &fakeRecorder{
|
||||
apps: app1AtExample,
|
||||
limits: map[string]store.RateLimit{
|
||||
store.RateLimitScopeDomain + "|example.com": domainLimit(),
|
||||
},
|
||||
@@ -398,6 +409,7 @@ func TestRateLimitFailsOpenOnCountError(t *testing.T) {
|
||||
// through, which is how the second MAIL FROM of a connection got past a limit.
|
||||
func TestRateLimitAppliesWithoutAClientIP(t *testing.T) {
|
||||
rec := &fakeRecorder{
|
||||
apps: app1AtExample,
|
||||
limits: map[string]store.RateLimit{
|
||||
store.RateLimitScopeDomain + "|example.com": domainLimit(),
|
||||
},
|
||||
@@ -484,6 +496,7 @@ func mailFromIn(t *testing.T, rec Store, fl *inflight, ip, from, login string) (
|
||||
|
||||
func limitedRecorder(count int64) *fakeRecorder {
|
||||
return &fakeRecorder{
|
||||
apps: app1AtExample,
|
||||
limits: map[string]store.RateLimit{
|
||||
store.RateLimitScopeDomain + "|example.com": domainLimit(),
|
||||
},
|
||||
@@ -603,6 +616,7 @@ func TestReservationReleasedOnAbort(t *testing.T) {
|
||||
// no domain reservation should linger after the refusal.
|
||||
func TestRefusalDoesNotLeaveDomainReservation(t *testing.T) {
|
||||
rec := &fakeRecorder{
|
||||
apps: app1AtExample,
|
||||
limits: map[string]store.RateLimit{
|
||||
store.RateLimitScopeDomain + "|example.com": domainLimit(),
|
||||
store.RateLimitScopeApp + "|app1": appLimit(),
|
||||
@@ -717,3 +731,46 @@ func TestRateLimitHoldsTheNullSenderToItsLoginsDomain(t *testing.T) {
|
||||
t.Fatalf("a null sender over its domain's limit = %v, want TempFail (4xx)", resp)
|
||||
}
|
||||
}
|
||||
|
||||
// The domain limit is the login's domain's. MAIL FROM reaches the milter
|
||||
// before Postfix holds it to the login, so a stranger with no login, or
|
||||
// another tenant's application, naming a victim's domain there must take
|
||||
// none of its slots, and write no refusal under it (security audit
|
||||
// 2026-10-17, M1).
|
||||
func TestRateLimitIsNotReservedByAnotherSendersMailFrom(t *testing.T) {
|
||||
victim := func() *fakeRecorder {
|
||||
return &fakeRecorder{
|
||||
apps: map[string]store.Application{
|
||||
"victim-app": {Login: "victim-app", DomainName: "victim.example"},
|
||||
"tenant-a": {Login: "tenant-a", DomainName: "tenant-a.example"},
|
||||
},
|
||||
limits: map[string]store.RateLimit{
|
||||
store.RateLimitScopeDomain + "|victim.example": {MaxMessages: 1, WindowSeconds: 3600},
|
||||
},
|
||||
counts: map[string]int64{},
|
||||
}
|
||||
}
|
||||
for name, login := range map[string]string{"a stranger with no login": "", "another tenant's application": "tenant-a"} {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
rec := victim()
|
||||
fl := &inflight{}
|
||||
for i := 0; i < 5; i++ {
|
||||
if _, resp := mailFromIn(t, rec, fl, limitIP, "x@victim.example", login); resp != milter.RespContinue {
|
||||
t.Fatalf("MAIL FROM %d = %v, want Continue", i, resp)
|
||||
}
|
||||
}
|
||||
if n := fl.count(store.RateLimitScopeDomain+"|victim.example", time.Now().Add(-time.Hour)); n != 0 {
|
||||
t.Errorf("%d slots of victim.example taken", n)
|
||||
}
|
||||
// The victim's own application still has its one message.
|
||||
if _, resp := mailFromIn(t, rec, fl, limitIP, "news@victim.example", "victim-app"); resp != milter.RespContinue {
|
||||
t.Errorf("the victim's own message = %v, want Continue", resp)
|
||||
}
|
||||
for _, r := range rec.rejected {
|
||||
if r.Domain == "victim.example" {
|
||||
t.Errorf("a refusal was written under the victim's domain: %+v", r)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,7 +1,9 @@
|
||||
package milter
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"log"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"github.com/mixeme/selfpost/internal/store"
|
||||
@@ -38,15 +40,19 @@ func (s *session) overLimit() bool {
|
||||
}
|
||||
}
|
||||
|
||||
domain := domainOf(s.from)
|
||||
if domain == "" && s.login != "" {
|
||||
// MAIL FROM:<> has no domain to read; the login has one. A null
|
||||
// sender is held to its application's domain like any other message
|
||||
// of it (review of audit stage 2).
|
||||
if a, err := s.rec.ApplicationByLogin(s.login); err == nil {
|
||||
domain = a.DomainName
|
||||
}
|
||||
// The domain is the login's, never the envelope's. MAIL FROM reaches
|
||||
// this milter before Postfix has held it to the login - with
|
||||
// smtpd_delay_reject at its default no sender restriction runs at MAIL
|
||||
// (Postfix 3.10.13, src/smtpd/smtpd.c, mail_cmd()) - so a domain read
|
||||
// from it let a stranger on 465/587, with no login at all, or another
|
||||
// tenant's application reserve a victim domain's slots (security audit
|
||||
// 2026-10-17, M1). A session without a login can deliver nothing on
|
||||
// those ports and reserves nothing; the send log counts a domain's
|
||||
// messages by the login's domain too (store.CountMessages).
|
||||
if s.login == "" {
|
||||
return false
|
||||
}
|
||||
domain := s.loginDomain()
|
||||
if domain == "" {
|
||||
return false
|
||||
}
|
||||
@@ -99,7 +105,7 @@ func (s *session) releaseReservations() {
|
||||
// and never affects the response.
|
||||
func (s *session) recordRejected(reason string) {
|
||||
err := s.rec.InsertRejected(store.SendLogEntry{
|
||||
Domain: domainOf(s.from),
|
||||
Domain: s.loginDomain(),
|
||||
AppLogin: s.login,
|
||||
From: s.from,
|
||||
Reason: reason,
|
||||
@@ -109,3 +115,19 @@ func (s *session) recordRejected(reason string) {
|
||||
log.Printf("journal-milter: record rejected %s: %v", s.from, err)
|
||||
}
|
||||
}
|
||||
|
||||
// loginDomain is the domain of the session's application, "" without a login
|
||||
// or when the store cannot say (fail-open, as overLimit is).
|
||||
func (s *session) loginDomain() string {
|
||||
if s.login == "" {
|
||||
return ""
|
||||
}
|
||||
a, err := s.rec.ApplicationByLogin(s.login)
|
||||
if err != nil {
|
||||
if !errors.Is(err, store.ErrApplicationNotFound) {
|
||||
log.Printf("journal-milter: domain of application %q: %v (fail-open)", s.login, err)
|
||||
}
|
||||
return ""
|
||||
}
|
||||
return strings.ToLower(a.DomainName)
|
||||
}
|
||||
|
||||
@@ -0,0 +1,97 @@
|
||||
package e2e
|
||||
|
||||
import (
|
||||
"crypto/tls"
|
||||
"fmt"
|
||||
"net"
|
||||
"net/http"
|
||||
"net/smtp"
|
||||
"net/url"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// limitDomain is a sending domain of its own for the domain-limit case, so
|
||||
// that its ceiling of one touches no other step.
|
||||
const limitDomain = "limit.e2e.test"
|
||||
|
||||
// testDomainLimitNotReservedByAStranger is the "done when" of the security
|
||||
// audit's M1 (2026-10-17): the level-2 domain limit is the login's domain's.
|
||||
// MAIL FROM reaches the journal milter before Postfix holds it to a login, so
|
||||
// a client with no login naming a domain there used to take that domain's
|
||||
// slots. Here a domain has a limit of one message an hour; a stranger opens
|
||||
// the submission port, names the domain in MAIL FROM three times without
|
||||
// signing in, and the domain's own application still sends its one message -
|
||||
// and is refused the second, so the limit is in force.
|
||||
func testDomainLimitNotReservedByAStranger(t *testing.T, sc *scenario) {
|
||||
domainID, err := sc.panel.addDomain(limitDomain)
|
||||
if err != nil {
|
||||
t.Fatalf("add %s: %v", limitDomain, err)
|
||||
}
|
||||
login, password, err := sc.panel.addApplication(domainID, "limit-app", "wildcard", "")
|
||||
if err == nil {
|
||||
err = waitForSenderMap(login, password, "alerts@"+limitDomain)
|
||||
}
|
||||
if err != nil {
|
||||
t.Fatalf("add the application: %v", err)
|
||||
}
|
||||
resp, body, err := sc.panel.postForm("/outbound/domains/"+domainID+"/settings/ratelimit", url.Values{
|
||||
"mode": {"manual"}, "max_messages": {"1"}, "window_seconds": {"3600"},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if resp.StatusCode != http.StatusOK || resp.Request.URL.Path != "/outbound/domains/"+domainID {
|
||||
t.Fatalf("save the domain limit: status %d, landed on %s:\n%.800s", resp.StatusCode, resp.Request.URL.Path, body)
|
||||
}
|
||||
|
||||
if err := strangerMailFrom("x@"+limitDomain, 3); err != nil {
|
||||
t.Fatalf("the stranger's MAIL FROM: %v", err)
|
||||
}
|
||||
|
||||
send := func(token string) sendResult {
|
||||
return attemptSend(sendAttempt{
|
||||
authLogin: login, authPassword: password,
|
||||
from: "alerts@" + limitDomain, to: recipient, subject: "e2e " + uniqueToken(token), body: "ok",
|
||||
})
|
||||
}
|
||||
if res := send("m1-own"); !res.ok() {
|
||||
t.Fatalf("the domain's own message after a stranger's MAIL FROM: %v", res.firstErr())
|
||||
}
|
||||
if err := send("m1-second").refusedAt("MAIL", 451, "4.7.1 Service unavailable"); err != nil {
|
||||
t.Fatalf("the second message over the domain's limit of one: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// strangerMailFrom opens the implicit-TLS submission port, says EHLO and
|
||||
// sends MAIL FROM:<from> n times with RSET between, never signing in. Each
|
||||
// MAIL FROM is answered 250: the journal milter continues, and Postfix
|
||||
// refuses such a client only at RCPT (smtpd_delay_reject).
|
||||
func strangerMailFrom(from string, n int) error {
|
||||
conn, err := tls.DialWithDialer(&net.Dialer{Timeout: 10 * time.Second}, "tcp", smtpsAddr, &tls.Config{
|
||||
InsecureSkipVerify: true,
|
||||
ServerName: selfpostHostname,
|
||||
})
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
c, err := smtp.NewClient(conn, selfpostHostname)
|
||||
if err != nil {
|
||||
_ = conn.Close()
|
||||
return err
|
||||
}
|
||||
defer c.Close()
|
||||
if err := c.Hello("stranger.e2e.test"); err != nil {
|
||||
return err
|
||||
}
|
||||
for i := 0; i < n; i++ {
|
||||
if err := c.Mail(from); err != nil {
|
||||
return fmt.Errorf("MAIL FROM %d: %w", i+1, err)
|
||||
}
|
||||
if err := c.Reset(); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
_ = c.Quit()
|
||||
return nil
|
||||
}
|
||||
@@ -315,6 +315,9 @@ func TestE2E(t *testing.T) {
|
||||
run("rate_limit_refusal_is_notified", func(t *testing.T) {
|
||||
testRateLimitRefusalIsNotified(t, sc)
|
||||
})
|
||||
run("domain_limit_not_reserved_by_a_stranger", func(t *testing.T) {
|
||||
testDomainLimitNotReservedByAStranger(t, sc)
|
||||
})
|
||||
run("negative_sender_login_mismatch", func(t *testing.T) {
|
||||
testSenderLoginMismatch(t, sc)
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user