findings of the reviews of audit stage 2
test / test (push) Canceled after 0s

Technical review (Opus) and security review (Fable) of a59e3d4..cb5a4a2;
reports in the session's scratchpad. Nothing critical, high or medium; no
outright bug. Fixed:

- F2, F3 (0008 backfill, before it runs on the live database): rows are
  filled by the rule new rows follow - the application's domain, then the
  name's - and only by a domain and application that existed when the row
  was written (created_at); the inbound log likewise. Test: the migration's
  UPDATEs re-run on prepared rows (before the domain, after, null sender,
  application gone, a stranger's name). Dry run on a copy of the live
  database: 68/68 and 10/10 rows filled, none disagreeing with its login's
  domain, integrity ok.
- V2, F4: an application's statistics and level-2 count are its login's
  rows of its domain (appByLogin), and StatsWindow starts no earlier than
  the entity's creation; the message page's "may send as" only for a login
  still of the row's domain. Test: a login taken again in another domain
  starts at zero.
- F7: MAIL FROM:<> is held to the domain limit of its login
  (ApplicationByLogin fills DomainName).
- V3: an imported DKIM selector is held to DNS labels (validate.Selector).
- F5, F6: an imported auto limit's numbers are dropped and recalculated
  right after the import; its mode is stored trimmed.
- V6: the store refuses a role that is neither global nor domain.
- F9: SaveUser keeps the stored hash when no new password is given (the
  form no longer writes back the hash it read). F10: a reach naming a
  domain deleted meanwhile is ErrNoSuchDomain, answered on the form.
- V4, F12: from-lines-other.txt - lines naming another sender ({other}) in
  a quoted string, a quoted pair, capitals, a comment, an encoded word, a
  quoted local part; the unit test wants each refused, the e2e
  (lines_naming_another_sender_never_signed_for_it) wants each refused and
  none signed for it. The passed list gained a.{DOMAIN}, a comment after
  the angle address, a quoted local part, and "b@ {other}" - passed by
  design (security.md), and the e2e shows it signed for the own domain.
- F13: e2e login_in_other_capitals_does_not_authenticate - AUTH with the
  login upper-cased gets 535.

Not changed, written down: V1/F1 - L8 is half closed (refused or passed
still tells an application which domains are hosted here); security.md's
accepted risks and the CHANGELOG say so, and the owner is asked whether to
refuse every foreign address in a display name (changes the 2.4.0 rule).
F11 - deleting a domain rewrites its rows' domain_id (seconds at 1e5 rows),
accepted at this size. F8 (test cannot tell write-first from read-first;
the reviewer verified it with two stores, 30/30), V5, V7, P2: noted.

Evidence on the test server for this tree: go vet clean, go test ./... 23
packages ok, make e2e exit 0 with the three new cases.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
mix
2026-10-14 16:17:57 +03:00
co-authored by Claude Opus 5.5
parent 8b53d63160
commit ef18ef21a0
26 changed files with 552 additions and 40 deletions
+23 -8
View File
@@ -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
+15
View File
@@ -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;
+20 -4
View File
@@ -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.
+15
View File
@@ -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)
}
}
+8
View File
@@ -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
}
+25 -2
View File
@@ -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) {
+22
View File
@@ -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}"
"<b@{other}>"
"\"a@{domain}\"@{other}"
"\"b@{other}\"@{domain}"
"\"x\\\" <b@{other}>\" <a@{domain}>"
"\"x\\\" <b@{other_qp}>\" <a@{domain}>"
"\"b@{other}\" <a@{domain}>"
"\"b@{OTHER}\" <a@{domain}>"
"b@{other} <a@{domain}>"
"a@{domain} (b@{other})"
"<a@{domain}> (b@{other})"
"(b@{other}) a@{domain}"
"=?utf-8?q?b=40{other}?= <a@{domain}>"
"\"x\" <b@{other}>, <a@{domain}>"
+8 -1
View File
@@ -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}"
"<a@{domain}>"
"\"The Shop\" <Orders@{domain}>"
@@ -20,3 +23,7 @@
"=?utf-8?b?0JzQsNCz0LDQt9C40L0=?=\r\n\t<a@{domain}>"
"a@{domain} (the shop)"
"a@{domain} (the (nested) shop)"
"a@{DOMAIN}"
"<a@{domain}> (the shop)"
"\"a@{domain}\"@{domain}"
"\"b@ {other}\" <a@{domain}>"
+11 -1
View File
@@ -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,
+93
View File
@@ -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)
}
}
+15
View File
@@ -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)
}
}
+6
View File
@@ -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 {
@@ -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);
+1 -1
View File
@@ -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)
}
+33
View File
@@ -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)
}
}
+13 -1
View File
@@ -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) {
+4
View File
@@ -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"} {
+30 -2
View File
@@ -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)
}
}
+43
View File
@@ -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)
}
}
+33
View File
@@ -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)
}
}
}
+19 -3
View File
@@ -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 },
+7 -7
View File
@@ -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 {
+8 -1
View File
@@ -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})
+20
View File
@@ -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")
+58 -6
View File
@@ -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)
}
}
+6
View File
@@ -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)
})