diff --git a/CHANGELOG.md b/CHANGELOG.md index 0894e56..1dc5834 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,14 +70,28 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version with nothing assigned. Saving and creating a user are now one transaction each, the e-mail included (audit 2026-10-12, M9). +- **The users form keeps a password changed meanwhile, and says when a + domain it offered is gone.** Saving a user wrote back the password hash + read when the form was opened, which undid a password the user changed on + Account in between; a domain deleted while the form was open ended in an + error page. - **A domain import is checked like the forms.** The import trusted the file for the report address, the rate limits and the client addresses: a limit with a window of no seconds counted nothing, and an address that was not one made the application refuse every client without a word. The file is now held to the forms' rules before anything is created, and refused with - the form's own message, naming the field and the application (audit - 2026-10-12, M10). + the form's own message, naming the field and the application; the DKIM + selector is held to DNS labels; an auto limit's numbers are worked out on + this server right after the import rather than taken from the file + (audit 2026-10-12, M10). +- **An application's numbers are its own.** Statistics, the level-2 count + and an auto limit's recalculation counted every row of the login, so an + application created with the login of a deleted one - in another domain, + even - started with the old one's traffic. They now count the login's rows + of its domain, from the application's creation on. A message with an empty + envelope sender (`MAIL FROM:<>`) is held to its domain's level-2 limit by + its login, like the application's other mail. - **A domain added again starts with an empty log.** Send-log rows belonged to a domain by its name, so a domain deleted and added again showed its earlier mail to whoever it was assigned to next, and its statistics and @@ -86,12 +100,13 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version registry); the rows of a deleted domain are the global role's alone. The inbound log follows the same rule (audit 2026-10-12, L12). -- **A refusal no longer says which other domain sends from this server.** - A `From:` line that named another sender of the server was refused with - that sender's domain in the reply, which confirmed to whoever tried names - that it is hosted here. The reply now stops at "it names another sender of - this server"; the line as written stays in the reply and on the refused - message's page (audit 2026-10-12, L8). +- **A refusal no longer names the other domain.** A `From:` line that named + another sender of the server was refused with that sender's domain in the + reply. The reply now stops at "it names another sender of this server"; + the line as written stays in the reply and on the refused message's page. + That such a line is refused at all still tells the sending application + that the domain is hosted here (security.md, accepted risks) (audit + 2026-10-12, L8). ### Added diff --git a/docs/audit-2026-10-12.md b/docs/audit-2026-10-12.md index 4709b65..b43ddae 100644 --- a/docs/audit-2026-10-12.md +++ b/docs/audit-2026-10-12.md @@ -688,6 +688,13 @@ has already run is never edited — the next file is added; the published are still in the global log. — **Opus** *Done 2026-10-14.* DMARC reports stay by name on purpose: a report is about the name in DNS, and whoever holds the name now is its reader. + *Changed by the reviews of stage 2:* the backfill follows the rule new + rows follow (the application's domain, then the name) and gives a row + only to a domain - and an application - that existed when it was + written; an application's numbers are its login's rows of its domain, + from its creation on. Deleting a domain now rewrites its rows' domain_id + (14 s for 333k rows, measured by the review; milliseconds at this + installation's size) - accepted, to revisit if a log grows that large. - [x] Docs: security.md § Roles (the guard) and the struck accepted risk, guide § Domain export, schema-migrations.md. — **Sonnet** - [x] **Which key OpenDKIM signs with, seen and not reasoned** (owner, @@ -701,6 +708,14 @@ has already run is never edited — the next file is added; the published shared with the check's unit test, so a line added to one is run by the other. A disagreement found is fixed in the check before the cut, and security.md § Mail path says the claim is tested. - **Opus** + *Changed by the reviews of stage 2:* a second list, lines naming another + sender in a quoted string, a comment, a quoted pair, capitals, an encoded + word (`from-lines-other.txt`), each refused and never signed for it; the + passed list gained capitals, a quoted local part and a name with a space + after its `@` (passed by design, signed for the own domain). L8 is half + closed: refused-or-passed still tells an application which domains are + hosted here - the owner decides whether to refuse every foreign address + in a display name (asked 2026-10-14). *Done 2026-10-14:* fifteen lines (`internal/milter/testdata/from-lines-passed.txt`), each signed for the application's domain alone, the signature valid. No disagreement. Groups are refused by the check, so none is in the file; diff --git a/docs/security.md b/docs/security.md index a5492e2..d275944 100644 --- a/docs/security.md +++ b/docs/security.md @@ -216,10 +216,17 @@ operator can act on it. the line may hold one `@`. That the two agree is tested, not only argued: the e2e sends every `From:` line the check passes - a list it shares with the check's unit test, `internal/milter/testdata/from-lines-passed.txt` - - and wants the message signed for the application's domain and no other. - The reply to a refusal says that the line names another sender of the - server, not which: it would confirm to whoever tries names that a domain - is hosted here. What this does not promise is what a display + and wants the message signed for the application's domain and no other; + and every line of a second list, lines that name another sender of the + server in a quoted string, a comment, a quoted pair, capitals or an encoded + word (`from-lines-other.txt`), must be refused and never arrive signed for + that sender. The reply to a refusal says that the line names another + sender of the server, not which. That a line naming a domain is refused + when the domain is a sender of this server, and passed when it is not, + still tells an application which domains are hosted here; the probe needs + an application of this server, and refusing every foreign address in a + display name would change the 2.4.0 rule - the owner decides (accepted + risks). What this does not promise is what a display name looks like to a person: a name of another sender written without an `@` the scan can find (a full-width dot, a space inside the name) passes, signed with the application's own key - a forged display name is as old @@ -436,6 +443,15 @@ deferred item from the roadmap. through - but no message data. The inbound journal itself is in the database, 0640. Trigger to revisit: a shared host, where the sidecar should instead be given the files through a group it shares with the panel. +- **An application can learn which domains are senders of this server.** + A `From:` whose display name or comment names an address at a domain this + server signs for is refused; one naming any other domain passes. The reply + no longer names the domain (audit 2026-10-12, L8), but refused or passed + answers the question for whoever sends the line. Accepted for now: it needs + an application of this server, and closing it means refusing every + foreign address in a display name, which changes the owner's 2.4.0 rule + (a stranger's address in the name passes) - his decision, asked on + 2026-10-14. - ~~**A row of the inbound log belongs to a domain by name.**~~ Struck in `2.5.0` (audit 2026-10-12, L12): both logs now hold a domain by id, see § Roles. diff --git a/internal/milter/milter_test.go b/internal/milter/milter_test.go index 381e1bd..064a615 100644 --- a/internal/milter/milter_test.go +++ b/internal/milter/milter_test.go @@ -700,3 +700,18 @@ func TestDomainOf(t *testing.T) { } } } + +// MAIL FROM:<> has no domain of its own; it is held to the limit of its +// login's domain like the application's other mail (review of audit stage 2). +func TestRateLimitHoldsTheNullSenderToItsLoginsDomain(t *testing.T) { + rec := &fakeRecorder{ + apps: map[string]store.Application{"app1": {Login: "app1", DomainName: "example.com"}}, + limits: map[string]store.RateLimit{ + store.RateLimitScopeDomain + "|example.com": domainLimit(), + }, + counts: map[string]int64{store.RateLimitScopeDomain + "|example.com": 5}, // == max + } + if resp := mailFrom(t, rec, limitIP, "<>", "app1"); resp != milter.RespTempFail { + t.Fatalf("a null sender over its domain's limit = %v, want TempFail (4xx)", resp) + } +} diff --git a/internal/milter/ratelimit.go b/internal/milter/ratelimit.go index 0246682..7fa8e6b 100644 --- a/internal/milter/ratelimit.go +++ b/internal/milter/ratelimit.go @@ -39,6 +39,14 @@ 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 + } + } if domain == "" { return false } diff --git a/internal/milter/sender_test.go b/internal/milter/sender_test.go index 53ffa6c..69b7319 100644 --- a/internal/milter/sender_test.go +++ b/internal/milter/sender_test.go @@ -283,8 +283,15 @@ func TestSenderCheckRefusalWithNullSenderHasItsDomain(t *testing.T) { // passedFromLines reads testdata/from-lines-passed.txt with {domain} put in. func passedFromLines(t *testing.T, domain string) []string { + return fromLines(t, "from-lines-passed.txt", domain, "news.example.net") +} + +// fromLines reads one of the shared files of From: lines with the +// placeholders put in: {domain}, {DOMAIN} (in capitals), {other}, {OTHER} and +// {other_qp} (its dots as quoted pairs). +func fromLines(t *testing.T, file, domain, other string) []string { t.Helper() - raw, err := os.ReadFile(filepath.Join("testdata", "from-lines-passed.txt")) + raw, err := os.ReadFile(filepath.Join("testdata", file)) if err != nil { t.Fatal(err) } @@ -298,11 +305,27 @@ func passedFromLines(t *testing.T, domain string) []string { if err != nil { t.Fatalf("%s: %v", line, err) } - out = append(out, strings.ReplaceAll(s, "{domain}", domain)) + out = append(out, strings.NewReplacer( + "{domain}", domain, "{DOMAIN}", strings.ToUpper(domain), + "{other_qp}", strings.ReplaceAll(other, ".", `\.`), + "{other}", other, "{OTHER}", strings.ToUpper(other), + ).Replace(s)) } return out } +// Every line of the shared file of lines naming another sender is refused: +// none may be passed to be signed with that sender's key. +func TestSenderCheckRefusesTheSharedLinesNamingAnother(t *testing.T) { + bind := NewBindings(writeMaps(t, senderMaps)) + for _, line := range fromLines(t, "from-lines-other.txt", "shop.example.org", "news.example.net") { + resp, _ := offer(t, bind, "shop", "shop@shop.example.org", [2]string{"From", line}) + if resp == milter.RespContinue { + t.Errorf("From: %q passed", line) + } + } +} + // Every line of the shared file is passed for the domain's application; the // e2e then checks OpenDKIM signs each for that domain alone. func TestSenderCheckPassesTheSharedLines(t *testing.T) { diff --git a/internal/milter/testdata/from-lines-other.txt b/internal/milter/testdata/from-lines-other.txt new file mode 100644 index 0000000..2aad1ed --- /dev/null +++ b/internal/milter/testdata/from-lines-other.txt @@ -0,0 +1,22 @@ +# From: lines that name another sender of this server, {other}, beside or in +# place of the application's own address at {domain} (review of audit stage +# 2). The check must refuse each: a line it read as the application's own and +# OpenDKIM as {other}'s would be signed with {other}'s key. The unit test +# (internal/milter/sender_test.go) wants each refused; the e2e +# (test/e2e/dkim_from_test.go) sends each and wants nothing signed for +# {other}. {OTHER} is {other} in capitals; {other_qp} is {other} with each +# dot escaped as a quoted pair. One Go-quoted string per line. +"b@{other}" +"" +"\"a@{domain}\"@{other}" +"\"b@{other}\"@{domain}" +"\"x\\\" \" " +"\"x\\\" \" " +"\"b@{other}\" " +"\"b@{OTHER}\" " +"b@{other} " +"a@{domain} (b@{other})" +" (b@{other})" +"(b@{other}) a@{domain}" +"=?utf-8?q?b=40{other}?= " +"\"x\" , " diff --git a/internal/milter/testdata/from-lines-passed.txt b/internal/milter/testdata/from-lines-passed.txt index fc850c9..3ff9b0d 100644 --- a/internal/milter/testdata/from-lines-passed.txt +++ b/internal/milter/testdata/from-lines-passed.txt @@ -4,7 +4,10 @@ # domain. Two tests read this file: the check's unit test # (internal/milter/sender_test.go) wants each line passed, and the e2e # (test/e2e/dkim_from_test.go) sends each one and wants the message signed -# for {domain} and nothing else. A line added here is run by both. +# for {domain} and nothing else. A line added here is run by both. {DOMAIN} +# is {domain} in capitals; {other} is another sender of the server, which a +# line here names in a way the check's scan does not see (a space after the +# @) - passed by design, and so signed for {domain} alone. "a@{domain}" "" "\"The Shop\" " @@ -20,3 +23,7 @@ "=?utf-8?b?0JzQsNCz0LDQt9C40L0=?=\r\n\t" "a@{domain} (the shop)" "a@{domain} (the (nested) shop)" +"a@{DOMAIN}" +" (the shop)" +"\"a@{domain}\"@{domain}" +"\"b@ {other}\" " diff --git a/internal/store/applications.go b/internal/store/applications.go index c7f7e56..8346ce2 100644 --- a/internal/store/applications.go +++ b/internal/store/applications.go @@ -37,6 +37,8 @@ type Application struct { Addresses []string AuthIPRestrict bool // when true, only AuthAllowedIPs may submit as this login AuthAllowedIPs []string // client IPs permitted when AuthIPRestrict is set + // DomainName is the name of DomainID, filled by ApplicationByLogin only. + DomainName string } // AllowsAuthFromIP reports whether a client at ip may submit mail authenticated @@ -205,8 +207,16 @@ func (s *Store) GetApplicationByLogin(login string) (Application, error) { } // ApplicationByLogin is the milter-facing alias for GetApplicationByLogin. +// +// DomainName is filled too: the milter holds a message with an empty +// envelope sender to the limit of its login's domain. func (s *Store) ApplicationByLogin(login string) (Application, error) { - return s.GetApplicationByLogin(login) + a, err := s.GetApplicationByLogin(login) + if err != nil { + return a, err + } + err = s.db.QueryRow("SELECT name FROM domains WHERE id = ?", a.DomainID).Scan(&a.DomainName) + return a, err } // ListApplicationsByDomain returns a domain's applications ordered by login, diff --git a/internal/store/backfill_test.go b/internal/store/backfill_test.go new file mode 100644 index 0000000..e93adae --- /dev/null +++ b/internal/store/backfill_test.go @@ -0,0 +1,93 @@ +package store + +import ( + "strings" + "testing" +) + +// backfill0008 runs the UPDATE statements of migration 0008 again: the store +// is already at its head, so the rows a test prepares get the backfill as +// they would have on an upgrade. +func backfill0008(t *testing.T, st *Store) { + t.Helper() + raw, err := migrationsFS.ReadFile("migrations/0008_tenant_boundary.sql") + if err != nil { + t.Fatal(err) + } + var lines []string + for _, l := range strings.Split(string(raw), "\n") { + if !strings.HasPrefix(strings.TrimSpace(l), "--") { + lines = append(lines, l) + } + } + for _, stmt := range strings.Split(strings.Join(lines, "\n"), ";") { + if strings.HasPrefix(strings.TrimSpace(stmt), "UPDATE") { + if _, err := st.db.Exec(stmt); err != nil { + t.Fatalf("%s: %v", stmt, err) + } + } + } +} + +// The backfill follows the rule new rows follow - the application's domain, +// then the domain of the row's name - and gives a row only to a domain that +// already existed when it was written (review of audit stage 2). +func TestBackfillOf0008(t *testing.T) { + st := openTestStore(t) + d := addTestDomain(t, st, "a.example") + if _, err := st.AddApplication(d.ID, "shop", AddressModeWildcard, nil); err != nil { + t.Fatal(err) + } + in, err := st.AddInboundDomain("in.example") + if err != nil { + t.Fatal(err) + } + // The domain, its application and the inbound domain were created on 1 Oct. + for _, q := range []string{ + `UPDATE domains SET created_at = '2026-10-01T00:00:00Z'`, + `UPDATE applications SET created_at = '2026-10-01T00:00:00Z'`, + `UPDATE inbound_domains SET created_at = '2026-10-01T00:00:00Z'`, + } { + if _, err := st.db.Exec(q); err != nil { + t.Fatal(err) + } + } + rows := []struct{ queue, domain, login, at string }{ + {"BEFORE", "a.example", "shop", "2026-09-01T00:00:00Z"}, // a predecessor's: nobody's + {"AFTER", "a.example", "shop", "2026-10-02T00:00:00Z"}, // the application's + {"NULLSENDER", "", "shop", "2026-10-02T00:00:00Z"}, // no domain of its own: the application's + {"GONEAPP", "a.example", "gone", "2026-10-02T00:00:00Z"}, // the application is gone: by name + {"STRANGER", "b.example", "gone", "2026-10-02T00:00:00Z"}, + } + for _, r := range rows { + if _, err := st.db.Exec(`INSERT INTO send_log (queue_id, domain, app_login, from_addr, to_addr, subject, status, created_at, updated_at) + VALUES (?, ?, ?, '', 'x@example.net', '', 'sent', ?, ?)`, r.queue, r.domain, r.login, r.at, r.at); err != nil { + t.Fatal(err) + } + } + for _, at := range []string{"2026-09-01T00:00:00Z", "2026-10-02T00:00:00Z"} { + if _, err := st.db.Exec(`INSERT INTO inbound_spam_log (inbound_domain, client_ip, from_addr, to_addr, subject, decision, engine, detail, created_at) + VALUES ('in.example', '203.0.113.7', 's@x', 'r@in.example', '', 'accept', 'rspamd', '', ?)`, at); err != nil { + t.Fatal(err) + } + } + + backfill0008(t, st) + + want := map[string]int64{"BEFORE": 0, "AFTER": d.ID, "NULLSENDER": d.ID, "GONEAPP": d.ID, "STRANGER": 0} + for queue, id := range want { + var got int64 + if err := st.db.QueryRow(`SELECT COALESCE(domain_id, 0) FROM send_log WHERE queue_id = ?`, queue).Scan(&got); err != nil { + t.Fatal(err) + } + if got != id { + t.Errorf("%s: domain_id %d, want %d", queue, got, id) + } + } + var before, after int64 + st.db.QueryRow(`SELECT COALESCE(inbound_domain_id, 0) FROM inbound_spam_log WHERE created_at = '2026-09-01T00:00:00Z'`).Scan(&before) + st.db.QueryRow(`SELECT COALESCE(inbound_domain_id, 0) FROM inbound_spam_log WHERE created_at = '2026-10-02T00:00:00Z'`).Scan(&after) + if before != 0 || after != in.ID { + t.Errorf("inbound rows: before %d, after %d; want 0 and %d", before, after, in.ID) + } +} diff --git a/internal/store/deliverylog_test.go b/internal/store/deliverylog_test.go index 1199f87..74dbafb 100644 --- a/internal/store/deliverylog_test.go +++ b/internal/store/deliverylog_test.go @@ -76,3 +76,18 @@ func TestCountQueuedOlderThan(t *testing.T) { t.Fatalf("queued rows before an hour ago = %d, %v; want 0", n, err) } } + +// Statistics start no earlier than the entity: what came before is a +// predecessor's of the same name (review of audit stage 2). +func TestStatsWindowStartsAtTheEntity(t *testing.T) { + created := time.Now().UTC().Add(-48 * time.Hour) + since, _, _ := StatsWindow(90, created) + if !since.Equal(created) { + t.Fatalf("since = %s, want the entity's creation %s", since, created) + } + old := time.Now().UTC().AddDate(-1, 0, 0) + since, _, _ = StatsWindow(90, old) + if since.Before(time.Now().UTC().AddDate(0, 0, -31)) { + t.Fatalf("since = %s, want within the 30-day window", since) + } +} diff --git a/internal/store/domains.go b/internal/store/domains.go index 3d21a0e..339d7d0 100644 --- a/internal/store/domains.go +++ b/internal/store/domains.go @@ -161,6 +161,12 @@ func scanDomain(r scanRow) (Domain, error) { return d, nil } +// isForeignKeyViolation reports whether err is a SQLite FOREIGN KEY conflict. +func isForeignKeyViolation(err error) bool { + var se *sqlite.Error + return errors.As(err, &se) && se.Code() == sqlite3.SQLITE_CONSTRAINT_FOREIGNKEY +} + // isUniqueViolation reports whether err is a SQLite UNIQUE/PRIMARY-KEY conflict, // so callers can turn a duplicate insert into a friendly domain-level error. func isUniqueViolation(err error) bool { diff --git a/internal/store/migrations/0008_tenant_boundary.sql b/internal/store/migrations/0008_tenant_boundary.sql index 239aeae..c0c0423 100644 --- a/internal/store/migrations/0008_tenant_boundary.sql +++ b/internal/store/migrations/0008_tenant_boundary.sql @@ -16,13 +16,26 @@ CREATE UNIQUE INDEX idx_applications_login_nocase ON applications (login COLLATE -- the domain of the login the row was written for; a row of a domain deleted -- since - and every row older than this migration whose domain is gone - is -- NULL and nobody's but the global role's. The name stays for display and for --- the global view. Filled once from the registry as it is now. +-- the global view. +-- +-- Filled once, by the rule new rows follow: the domain of the row's +-- application, then - for a row whose application is gone - the domain of the +-- row's name. Either only if it already existed when the row was written: a +-- domain deleted and added again before this migration must not inherit the +-- rows of the one before (review of audit stage 2). Times are RFC3339 UTC +-- text, so they compare as times. ALTER TABLE send_log ADD COLUMN domain_id INTEGER REFERENCES domains(id) ON DELETE SET NULL; -UPDATE send_log SET domain_id = (SELECT d.id FROM domains d WHERE d.name = send_log.domain); +UPDATE send_log SET domain_id = COALESCE( + (SELECT a.domain_id FROM applications a JOIN domains d ON d.id = a.domain_id + WHERE a.login = send_log.app_login + AND a.created_at <= send_log.created_at AND d.created_at <= send_log.created_at), + (SELECT d.id FROM domains d + WHERE d.name = send_log.domain AND d.created_at <= send_log.created_at)); CREATE INDEX idx_send_log_domain_id_created_at ON send_log (domain_id, created_at); -- The inbound journal follows the same rule, so the two logs do not follow two -- (security.md's accepted risk of the inbound log, struck). ALTER TABLE inbound_spam_log ADD COLUMN inbound_domain_id INTEGER REFERENCES inbound_domains(id) ON DELETE SET NULL; -UPDATE inbound_spam_log SET inbound_domain_id = (SELECT d.id FROM inbound_domains d WHERE d.name = inbound_spam_log.inbound_domain); +UPDATE inbound_spam_log SET inbound_domain_id = (SELECT d.id FROM inbound_domains d + WHERE d.name = inbound_spam_log.inbound_domain AND d.created_at <= inbound_spam_log.created_at); CREATE INDEX idx_inbound_spam_log_domain_id_created_at ON inbound_spam_log (inbound_domain_id, created_at); diff --git a/internal/store/ratelimits.go b/internal/store/ratelimits.go index 962e497..d24a625 100644 --- a/internal/store/ratelimits.go +++ b/internal/store/ratelimits.go @@ -180,7 +180,7 @@ func (s *Store) CountMessages(scope, ref string, since time.Time) (int64, error) case RateLimitScopeDomain: match = domainByName case RateLimitScopeApp: - match = "app_login = ?" + match = appByLogin default: return 0, fmt.Errorf("unknown rate-limit scope %q", scope) } diff --git a/internal/store/sendlog_test.go b/internal/store/sendlog_test.go index 3f4832d..5be0f22 100644 --- a/internal/store/sendlog_test.go +++ b/internal/store/sendlog_test.go @@ -401,3 +401,36 @@ func TestADomainAddedAgainStartsWithAnEmptyLog(t *testing.T) { t.Fatalf("statistics = %+v, %v; want one message", stats, err) } } + +// A deleted application's login taken again in another domain starts with +// none of the old one's numbers (review of audit stage 2). +func TestALoginTakenAgainStartsWithNoNumbers(t *testing.T) { + st := openTestStore(t) + a := addTestDomain(t, st, "a.example") + b := addTestDomain(t, st, "b.example") + old, err := st.AddApplication(a.ID, "news", AddressModeWildcard, nil) + if err != nil { + t.Fatal(err) + } + if err := st.InsertQueued(SendLogEntry{QueueID: "OLD", Domain: "a.example", AppLogin: "news", To: "x@example.net"}); err != nil { + t.Fatal(err) + } + if _, err := st.DeleteApplication(old.ID); err != nil { + t.Fatal(err) + } + if _, err := st.AddApplication(b.ID, "news", AddressModeWildcard, nil); err != nil { + t.Fatal(err) + } + if n, err := st.CountMessages(RateLimitScopeApp, "news", time.Now().Add(-time.Hour)); err != nil || n != 0 { + t.Fatalf("level-2 count of the new news = %d, %v; want 0", n, err) + } + if stats, err := st.AppSendStats("news", 90, time.Now().Add(-time.Hour)); err != nil || stats.Total != 0 { + t.Fatalf("statistics of the new news = %+v, %v; want none", stats, err) + } + if err := st.InsertQueued(SendLogEntry{QueueID: "NEW", Domain: "b.example", AppLogin: "news", To: "x@example.net"}); err != nil { + t.Fatal(err) + } + if n, _ := st.CountMessages(RateLimitScopeApp, "news", time.Now().Add(-time.Hour)); n != 1 { + t.Fatalf("level-2 count of the new news = %d, want its own one", n) + } +} diff --git a/internal/store/stats.go b/internal/store/stats.go index 41c9045..f1d5191 100644 --- a/internal/store/stats.go +++ b/internal/store/stats.go @@ -42,6 +42,11 @@ func StatsWindow(retentionDays int, entityCreated time.Time) (since time.Time, h if hours < 1 { hours = 1 } + // Nothing before the entity was created is its own: rows from then are a + // predecessor's of the same name or login (review of audit stage 2). + if c := entityCreated.UTC(); c.After(since) { + since = c + } return since, hours, windowDays } @@ -62,7 +67,7 @@ func (s *Store) DomainSendStats(name string, retentionDays int, createdAt time.T // AppSendStats returns send statistics for an application login. func (s *Store) AppSendStats(login string, retentionDays int, createdAt time.Time) (SendStats, error) { since, hours, windowDays := StatsWindow(retentionDays, createdAt) - stats, err := s.sendStats("app_login = ?", login, since, hours) + stats, err := s.sendStats(appByLogin, login, since, hours) if err != nil { return SendStats{}, err } @@ -75,6 +80,13 @@ func (s *Store) AppSendStats(login string, retentionDays int, createdAt time.Tim // registered under the name it is given. const domainByName = "domain_id = (SELECT d.id FROM domains d WHERE d.name = ?)" +// appByLogin selects an application's send-log rows: its login's rows of its +// domain. A login is unique while it exists, but a deleted application's +// login can be taken again in another domain, and its numbers - statistics, +// the level-2 count - must not start with the old one's (review of audit +// stage 2). ?1 twice; a plain ? after it takes the next number. +const appByLogin = "app_login = ?1 AND domain_id = (SELECT a.domain_id FROM applications a WHERE a.login = ?1)" + // sendStats counts the rows that match, a condition with one placeholder // taking ref. func (s *Store) sendStats(match, ref string, since time.Time, hours float64) (SendStats, error) { diff --git a/internal/store/stats_test.go b/internal/store/stats_test.go index 358cea4..75c637f 100644 --- a/internal/store/stats_test.go +++ b/internal/store/stats_test.go @@ -19,6 +19,10 @@ func TestSendStatsTotalPeakAvg(t *testing.T) { now := time.Now().UTC() hour := now.Format("2006-01-02T15") prevHour := now.Add(-2 * time.Hour).Format("2006-01-02T15") + // Both were created before the messages below: statistics start at the + // entity's creation. + created := now.Add(-3 * time.Hour) + d.CreatedAt, a.CreatedAt = created, created // Hour 1: two messages (Q1 two recipients + Q2). for _, to := range []string{"a@x.net", "b@x.net"} { diff --git a/internal/store/users.go b/internal/store/users.go index 94d0b7d..11c3057 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -16,6 +16,10 @@ var ErrUserNotFound = errors.New("user not found") // ErrUserExists is returned when a username is already taken. var ErrUserExists = errors.New("username already taken") +// ErrNoSuchDomain is returned when a user is assigned a domain that does not +// exist - one deleted while the users form was open. +var ErrNoSuchDomain = errors.New("an assigned domain does not exist") + // ErrLastGlobal is returned when deleting or demoting the last global user. var ErrLastGlobal = errors.New("cannot remove last global administrator") @@ -225,7 +229,8 @@ func (s *Store) CreateUser(username, passwordHash string, role Role, reach Reach } // UserChange is everything the users form sets on a user, written at once by -// AddUser and SaveUser. ID is ignored by AddUser. +// AddUser and SaveUser. ID is ignored by AddUser; SaveUser keeps the stored +// password hash when PasswordHash is empty. type UserChange struct { ID int64 Username, PasswordHash, Email string @@ -238,6 +243,9 @@ type UserChange struct { // and a failure part way leaves no user rather than one without it (audit // 2026-10-12, M9). func (s *Store) AddUser(c UserChange) (int64, error) { + if err := knownRole(c.Role); err != nil { + return 0, err + } if c.Role == RoleDomain && c.Reach.Empty() { return 0, ErrEmptyReach } @@ -282,6 +290,9 @@ func (s *Store) AddUser(c UserChange) (int64, error) { // after the write, so two administrators demoting each other at once cannot // both succeed (M7). func (s *Store) SaveUser(c UserChange) error { + if err := knownRole(c.Role); err != nil { + return err + } if c.Role == RoleDomain && c.Reach.Empty() { return ErrEmptyReach } @@ -292,8 +303,10 @@ func (s *Store) SaveUser(c UserChange) error { defer tx.Rollback() // A write first: it takes the write lock, so what is counted below is // what is committed. + // An empty hash keeps the stored one: the form read it when it was + // opened, and the user may have changed it on Account since. res, err := tx.Exec( - "UPDATE users SET username = ?, password_hash = ?, email = ?, role = ? WHERE id = ?", + "UPDATE users SET username = ?, password_hash = COALESCE(NULLIF(?, ''), password_hash), email = ?, role = ? WHERE id = ?", c.Username, c.PasswordHash, c.Email, c.Role, c.ID, ) if err != nil { @@ -321,6 +334,15 @@ func (s *Store) SaveUser(c UserChange) error { return nil } +// knownRole refuses a role that is neither global nor domain. The handlers +// check the form first; this is the second line. +func knownRole(r Role) error { + if r != RoleGlobal && r != RoleDomain { + return fmt.Errorf("unknown role %q", r) + } + return nil +} + // requireAGlobal is ErrLastGlobal when the transaction has left no global // user. func requireAGlobal(tx *sql.Tx) error { @@ -510,6 +532,9 @@ func writeUserReach(tx *sql.Tx, userID int64, reach Reach) error { if !reach.AllDomains { for _, id := range reach.DomainIDs { if _, err := tx.Exec("INSERT INTO user_domains (user_id, domain_id) VALUES (?, ?)", userID, id); err != nil { + if isForeignKeyViolation(err) { + return ErrNoSuchDomain + } return fmt.Errorf("set user reach insert: %w", err) } } @@ -517,6 +542,9 @@ func writeUserReach(tx *sql.Tx, userID int64, reach Reach) error { if !reach.AllInbound { for _, id := range reach.InboundDomainIDs { if _, err := tx.Exec("INSERT INTO user_inbound_domains (user_id, inbound_domain_id) VALUES (?, ?)", userID, id); err != nil { + if isForeignKeyViolation(err) { + return ErrNoSuchDomain + } return fmt.Errorf("set user reach insert inbound: %w", err) } } diff --git a/internal/store/users_tx_test.go b/internal/store/users_tx_test.go index d4ae7d6..5d9714a 100644 --- a/internal/store/users_tx_test.go +++ b/internal/store/users_tx_test.go @@ -149,3 +149,46 @@ func TestCreateGlobalUserOnlyOnce(t *testing.T) { t.Fatalf("%d setups succeeded, %d global users; want 1 and 1", ok, n) } } + +// The store refuses a role that is neither global nor domain. +func TestTheStoreKnowsOnlyTwoRoles(t *testing.T) { + st := openTestStore(t) + if _, err := st.AddUser(UserChange{Username: "x", PasswordHash: "h", Role: "admin"}); err == nil { + t.Fatal("AddUser took the role admin") + } + id, err := st.CreateUser("y", "h", RoleGlobal, Reach{}) + if err != nil { + t.Fatal(err) + } + if err := st.SaveUser(UserChange{ID: id, Username: "y", PasswordHash: "h", Role: ""}); err == nil { + t.Fatal("SaveUser took an empty role") + } +} + +// A save without a new password keeps the stored hash - one the user may have +// changed on Account since the form was opened - and a reach naming a domain +// deleted meanwhile is ErrNoSuchDomain (review of audit stage 2). +func TestSaveUserKeepsTheHashAndNamesAMissingDomain(t *testing.T) { + st := openTestStore(t) + d := addTestDomain(t, st, "example.com") + if _, err := st.CreateUser("root", "hash", RoleGlobal, Reach{}); err != nil { + t.Fatal(err) + } + id, err := st.CreateUser("shop", "old", RoleDomain, Reach{DomainIDs: []int64{d.ID}}) + if err != nil { + t.Fatal(err) + } + if err := st.UpdateUser(id, "shop", "changed-on-account", ""); err != nil { + t.Fatal(err) + } + if err := st.SaveUser(UserChange{ID: id, Username: "shop", Role: RoleDomain, Reach: Reach{DomainIDs: []int64{d.ID}}}); err != nil { + t.Fatal(err) + } + u, _ := st.GetUser(id) + if u.PasswordHash != "changed-on-account" { + t.Fatalf("the hash became %q", u.PasswordHash) + } + if err := st.SaveUser(UserChange{ID: id, Username: "shop", Role: RoleDomain, Reach: Reach{DomainIDs: []int64{9999}}}); !errors.Is(err, ErrNoSuchDomain) { + t.Fatalf("a reach to a deleted domain = %v, want ErrNoSuchDomain", err) + } +} diff --git a/internal/web/handlers/handlers_backup.go b/internal/web/handlers/handlers_backup.go index 1d180a6..82c7dcb 100644 --- a/internal/web/handlers/handlers_backup.go +++ b/internal/web/handlers/handlers_backup.go @@ -292,6 +292,7 @@ func (h *Handlers) HandleImportDomain(w http.ResponseWriter, r *http.Request) { if exp.DMARCRua != "" { h.resyncDMARC("domain import") } + h.recalcImportedLimits(d, exp) http.Redirect(w, r, fmt.Sprintf("/outbound/domains/%d?imported=1", d.ID), http.StatusSeeOther) } @@ -384,6 +385,9 @@ func (h *Handlers) checkDomainImport(exp *domain.DomainExport) string { invalid := func(what string, err error) string { return "Invalid " + what + " in export file: " + err.Error() } + if err := validate.Selector(exp.DKIMSelector); err != nil { + return invalid("DKIM selector", err) + } if exp.DMARCRua != "" { if err := validate.Email(exp.DMARCRua); err != nil { return invalid("report address", err) @@ -397,7 +401,12 @@ func (h *Handlers) checkDomainImport(exp *domain.DomainExport) string { if err != nil { return err } + rl.Mode = mode // as the form stores it, trimmed if mode == store.RateLimitModeAuto { + // An auto limit's numbers are this server's to work out from its + // own traffic, not the file's: they are dropped here and worked + // out right after the import (review of audit stage 2). + rl.MaxMessages, rl.WindowSeconds = 0, 0 if rl.AutoMultiplier == 0 { return nil // as an empty field on the form: the default } @@ -434,3 +443,27 @@ func (h *Handlers) checkDomainImport(exp *domain.DomainExport) string { } return "" } + +// recalcImportedLimits works out the numbers of the auto limits an import +// brought, at once rather than at the next six-hourly pass: the file's own +// numbers were dropped (checkDomainImport). A failure is logged; the pass +// tries again. +func (h *Handlers) recalcImportedLimits(d store.Domain, exp domain.DomainExport) { + if exp.RateLimit != nil && exp.RateLimit.Mode == store.RateLimitModeAuto { + if err := h.recalcRateLimit(store.RateLimitScopeDomain, d.ID); err != nil { + logf("panel: import domain %q: recalculate its auto limit: %v", d.Name, err) + } + } + for _, a := range exp.Applications { + if a.RateLimit == nil || a.RateLimit.Mode != store.RateLimitModeAuto { + continue + } + app, err := h.store.GetApplicationByLogin(a.Login) + if err == nil { + err = h.recalcRateLimit(store.RateLimitScopeApp, app.ID) + } + if err != nil { + logf("panel: import domain %q: recalculate the auto limit of %s: %v", d.Name, a.Login, err) + } + } +} diff --git a/internal/web/handlers/handlers_import_test.go b/internal/web/handlers/handlers_import_test.go index 8789e54..a963402 100644 --- a/internal/web/handlers/handlers_import_test.go +++ b/internal/web/handlers/handlers_import_test.go @@ -21,9 +21,10 @@ func TestDomainImportIsCheckedLikeTheForms(t *testing.T) { h, _ := inboundHandlers(t) good := func() domain.DomainExport { return domain.DomainExport{ - Domain: "example.com", - DMARCRua: "reports@example.com", - RateLimit: &domain.RateLimitExport{Mode: store.RateLimitModeManual, MaxMessages: 10, WindowSeconds: 3600}, + Domain: "example.com", + DKIMSelector: "mail.2026", + DMARCRua: "reports@example.com", + RateLimit: &domain.RateLimitExport{Mode: store.RateLimitModeManual, MaxMessages: 10, WindowSeconds: 3600}, Applications: []domain.AppExport{{ Login: "app1", AddressMode: store.AddressModeWildcard, Password: "imported-password", AuthIPRestrict: true, AuthAllowedIPs: []string{"192.0.2.1", " 2001:DB8::1 "}, @@ -39,11 +40,26 @@ func TestDomainImportIsCheckedLikeTheForms(t *testing.T) { t.Errorf("client addresses stored as %q, want as the form stores them", got) } + // An auto limit's numbers are worked out here, not taken from the file, + // and its mode is stored trimmed. + exp = good() + exp.Applications[0].RateLimit = &domain.RateLimitExport{Mode: " auto ", MaxMessages: 1, WindowSeconds: 86400} + if msg := h.checkDomainImport(&exp); msg != "" { + t.Fatalf("an auto limit with numbers refused: %s", msg) + } + if rl := exp.Applications[0].RateLimit; rl.Mode != store.RateLimitModeAuto || rl.MaxMessages != 0 || rl.WindowSeconds != 0 { + t.Errorf("the auto limit kept %+v", rl) + } + for _, c := range []struct { name string break_ func(*domain.DomainExport) want string }{ + {"selector", func(e *domain.DomainExport) { e.DKIMSelector = "mail;x=y" }, + "Invalid DKIM selector in export file: selector: domain may contain only"}, + {"long selector", func(e *domain.DomainExport) { e.DKIMSelector = strings.Repeat("a", 64) }, + "Invalid DKIM selector in export file: selector must be at most 63 characters"}, {"report address", func(e *domain.DomainExport) { e.DMARCRua = "not an address" }, "Invalid report address in export file: "}, {"no window", func(e *domain.DomainExport) { e.RateLimit.WindowSeconds = 0 }, diff --git a/internal/web/handlers/handlers_monitor.go b/internal/web/handlers/handlers_monitor.go index acde537..90c1908 100644 --- a/internal/web/handlers/handlers_monitor.go +++ b/internal/web/handlers/handlers_monitor.go @@ -123,19 +123,19 @@ func (h *Handlers) HandleDelivery(w http.ResponseWriter, r *http.Request) { break } page := view.NewOutMessage(h.shellMeta(r), in). - WithHistory(deliveryEvents(row, h.cfg.RetryPolicy, h.sendsAs(row.AppLogin))). + WithHistory(deliveryEvents(row, h.cfg.RetryPolicy, h.sendsAs(row))). // The mail.log lines for this message, and — when there are none — the // reason, which is a normal outcome rather than a failure. WithDeliveryLog(logLines, logNote) h.view.Render(w, http.StatusOK, "out-message", page) } -// sendsAs says what an application may send as, in the words of a refusal: -// any address of its domain, or the addresses of its list. Empty when the -// login is no application's any more. -func (h *Handlers) sendsAs(login string) view.Text { - a, err := h.store.GetApplicationByLogin(login) - if err != nil { +// sendsAs says what the row's application may send as, in the words of a +// refusal: any address of its domain, or the addresses of its list. Empty when +// the login is no application's any more, or is now another domain's. +func (h *Handlers) sendsAs(row store.SendLogRow) view.Text { + a, err := h.store.GetApplicationByLogin(row.AppLogin) + if err != nil || a.DomainID != row.DomainID { return nil } if a.AddressMode == store.AddressModeList { diff --git a/internal/web/handlers/handlers_users.go b/internal/web/handlers/handlers_users.go index 7de913d..cac3ed6 100644 --- a/internal/web/handlers/handlers_users.go +++ b/internal/web/handlers/handlers_users.go @@ -217,6 +217,10 @@ func (h *Handlers) submitUserCreate(w http.ResponseWriter, r *http.Request) { h.renderUserForm(w, r, http.StatusConflict, 0, userFormView{FormErr: "That username is already in use.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach}) return } + if errors.Is(err, store.ErrNoSuchDomain) { + h.renderUserForm(w, r, http.StatusConflict, 0, userFormView{FormErr: "A domain chosen here has been deleted meanwhile. Choose again.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach}) + return + } logf("panel: create user: %v", err) h.renderUserForm(w, r, http.StatusInternalServerError, 0, userFormView{FormErr: "Could not create user. Please check the logs."}) return @@ -283,7 +287,7 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st } } - hash := u.PasswordHash + hash := "" // empty keeps the stored hash (SaveUser) if password != "" { if err := validate.AdminPassword(password); err != nil { h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: err.Error(), FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach}) @@ -308,6 +312,9 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st case errors.Is(err, store.ErrLastGlobal): lastGlobal() return + case errors.Is(err, store.ErrNoSuchDomain): + h.renderUserForm(w, r, http.StatusConflict, u.ID, userFormView{FormErr: "A domain chosen here has been deleted meanwhile. Choose again.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach}) + return case err != nil: logf("panel: update user: %v", err) h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Could not save user. Please check the logs.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach}) diff --git a/internal/web/validate/validate.go b/internal/web/validate/validate.go index a7772bb..d0ada3e 100644 --- a/internal/web/validate/validate.go +++ b/internal/web/validate/validate.go @@ -82,6 +82,26 @@ func Domain(name string) error { return nil } +// Selector checks a DKIM selector: one or more DNS labels as Domain allows +// them, at most 63 characters in all. The panel's own selector is fixed in +// its configuration; this is for one read from a domain export file, which +// goes into OpenDKIM's tables, a key file's name and every signature's s= +// (review of audit stage 2). +func Selector(s string) error { + if s == "" { + return fmt.Errorf("selector is required") + } + if len(s) > 63 { + return fmt.Errorf("selector must be at most 63 characters") + } + for _, label := range strings.Split(s, ".") { + if err := domainLabel(label); err != nil { + return fmt.Errorf("selector: %w", err) + } + } + return nil +} + func domainLabel(label string) error { if len(label) == 0 { return fmt.Errorf("domain must not contain an empty label") diff --git a/test/e2e/dkim_from_test.go b/test/e2e/dkim_from_test.go index 44b6ee9..017f268 100644 --- a/test/e2e/dkim_from_test.go +++ b/test/e2e/dkim_from_test.go @@ -13,10 +13,12 @@ import ( "github.com/emersion/go-msgauth/dkim" ) -// passedFromLines reads the From: lines the sender check passes, the file its -// unit test reads too, with the application's domain put in. -func passedFromLines(repoRoot, domain string) ([]string, error) { - raw, err := os.ReadFile(filepath.Join(repoRoot, "internal", "milter", "testdata", "from-lines-passed.txt")) +// fromLines reads one of the files of From: lines the sender check's unit +// test reads too, with the placeholders put in: {domain} the application's, +// {other} another sender of the server, each also in capitals, and +// {other_qp} with its dots as quoted pairs. +func fromLines(repoRoot, file, domain, other string) ([]string, error) { + raw, err := os.ReadFile(filepath.Join(repoRoot, "internal", "milter", "testdata", file)) if err != nil { return nil, err } @@ -30,7 +32,11 @@ func passedFromLines(repoRoot, domain string) ([]string, error) { if err != nil { return nil, fmt.Errorf("%s: %w", line, err) } - out = append(out, strings.ReplaceAll(s, "{domain}", domain)) + out = append(out, strings.NewReplacer( + "{domain}", domain, "{DOMAIN}", strings.ToUpper(domain), + "{other_qp}", strings.ReplaceAll(other, ".", `\.`), + "{other}", other, "{OTHER}", strings.ToUpper(other), + ).Replace(s)) } return out, nil } @@ -43,7 +49,7 @@ func passedFromLines(repoRoot, domain string) ([]string, error) { // application's own and OpenDKIM as another sender's would be signed with // that sender's key. func testSignedForItsDomainOnly(t *testing.T, sc *scenario) { - lines, err := passedFromLines(h.repoRoot, senderDomain) + lines, err := fromLines(h.repoRoot, "from-lines-passed.txt", senderDomain, otherDomain) if err != nil { t.Fatal(err) } @@ -82,3 +88,49 @@ func testSignedForItsDomainOnly(t *testing.T, sc *scenario) { } } } + +// testNeverSignedForAnother: the lines that name another sender of the server +// are refused by the check; whatever happens to one, nothing arrives signed +// with that sender's key (review of audit stage 2). +func testNeverSignedForAnother(t *testing.T, sc *scenario) { + lines, err := fromLines(h.repoRoot, "from-lines-other.txt", senderDomain, otherDomain) + if err != nil { + t.Fatal(err) + } + for _, line := range lines { + token := uniqueToken("dkimother") + res := attemptSend(sendAttempt{ + authLogin: sc.appLogin, authPassword: sc.appPassword, + from: "alerts@" + senderDomain, headerFrom: line, to: recipient, + subject: "e2e " + token, body: "which key signs this", + }) + if err := res.refusedAt("DATA", 550, "5.7.1 From:"); err == nil { + continue + } + t.Errorf("From: %q was not refused by the check: %v", line, res.firstErr()) + raw, err := findSinkMessage(h.stageDir, token, 15*time.Second) + if err != nil { + continue + } + verifications, _ := dkim.VerifyWithOptions(bytes.NewReader(raw), &dkim.VerifyOptions{LookupTXT: lookupTXTFunc()}) + for _, v := range verifications { + if v.Domain == otherDomain { + t.Errorf("From: %q arrived signed for %s", line, otherDomain) + } + } + } +} + +// testLoginCaseMatters: SASL compares the login byte for byte, so an +// application's login in other capitals does not authenticate. Every lookup on +// the mail path is by the exact login and relies on that (review of audit +// stage 2, F13). +func testLoginCaseMatters(t *testing.T, sc *scenario) { + res := attemptSend(sendAttempt{ + authLogin: strings.ToUpper(sc.appLogin), authPassword: sc.appPassword, + from: "alerts@" + senderDomain, to: recipient, subject: "e2e " + uniqueToken("case"), body: "x", + }) + if err := res.refusedAt("AUTH", 535, ""); err != nil { + t.Fatalf("AUTH as %s: %v", strings.ToUpper(sc.appLogin), err) + } +} diff --git a/test/e2e/main_test.go b/test/e2e/main_test.go index c5d63c3..499b3e2 100644 --- a/test/e2e/main_test.go +++ b/test/e2e/main_test.go @@ -313,6 +313,12 @@ func TestE2E(t *testing.T) { run("passed_from_lines_signed_for_their_domain_only", func(t *testing.T) { testSignedForItsDomainOnly(t, sc) }) + run("lines_naming_another_sender_never_signed_for_it", func(t *testing.T) { + testNeverSignedForAnother(t, sc) + }) + run("login_in_other_capitals_does_not_authenticate", func(t *testing.T) { + testLoginCaseMatters(t, sc) + }) run("panel_down_defers_mail", func(t *testing.T) { testPanelDownDefersMail(t, sc) })