From 3584adcbc2bce663f4e662dcf5c1cbc0c6f49dca Mon Sep 17 00:00:00 2001 From: Mix Date: Thu, 15 Oct 2026 02:48:02 +0300 Subject: [PATCH] sender check: From: read with OpenDKIM's own parser; L8 closed Audit 2026-10-12, stage 2c, the owner's decision of 2026-10-14: the check decides by the address OpenDKIM signs for, whatever a display name or a comment holds. internal/milter/mailparse.go ports libopendkim's dkim_mail_parse() with dkim_mail_first_special(), dkim_mail_token() and dkim_mail_matching_paren() from Debian's opendkim 2.11.0~beta2-8+deb12u1, line for line: the '<' form, group and list punctuation, an '@' before the first ':', ';' or ',' deciding where the address starts, source routes dropped, quoted strings and domain literals kept with their delimiters, comments skipped, whitespace between atoms one space, no encoded word decoded; and the C's own quirks ("@d" gives d as the local part too). testdata/mailparse_harness.c compiles the C from the image's source; the test feeds it and the port 200 019 lines (the hand-written cases and 200 000 random ones over the specials) - 93 443 parse and agree byte for byte, 106 576 fail and agree on the error (on an error the C leaves its in-place result unterminated, and OpenDKIM uses neither part then). judge(): one From: field and one address stay required (moreAfter: only list or group punctuation may follow); what OpenDKIM cannot read is refused (mlfi_eoh() would not sign it); the address is lower-cased in ASCII only, as dkimf_lowercase() and Postfix's tables do. Found reading dkimf_apply_signtable(): OpenDKIM matches its refile SigningTable against "user@domain" printed into MAXADDRESS + 1 = 257 bytes, so a longer address is matched cut short - a quoted local part padded to end there in "@another.tenant" would be signed with that tenant's key while its real domain is the application's. Such an address (over 256 bytes) is refused; one that fits is matched whole. The display-name and comment scan, the "one @ outside quotes" rule and the SigningTable reader (Signers) go; the check asks for header values with their leading space as OpenDKIM does (OptHeaderLeadingSpace; Postfix keeps a v2 milter's event mask, milter8.c milter8_negotiate()). L8 closed: refused or passed no longer depends on other domains of the server; security.md's accepted risk is struck. The shared other-sender lines are now held to "never signed for the other sender": unit test (a passed line is read at the application's domain) and e2e (a passed line arrives signed for the application's domain, none for the other). Documents: architecture "How the header is read", security's sender check paragraph, the guide; the help topic per 2cf118c. Stand (2026-10-15, make e2e, exit 0): sender_check_holds_the_from_line - another sender's address in the display name now passes; a From: whose quoted local part is padded so that, cut at 256 bytes, it ends in "@other.e2e.test" is refused 550 5.7.1 "the address is longer than OpenDKIM reads whole"; lines_naming_another_sender_never_signed_for_it and passed_from_lines_signed_for_their_domain_only pass. go vet and go test ./... pass on the server (24 packages); the differential test ran there against the compiled C. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 13 + cmd/panel/sendercheck.go | 4 +- docs/architecture.md | 24 +- docs/audit-2026-10-12.md | 12 +- docs/guide.md | 14 +- docs/roadmap.md | 2 +- docs/security.md | 61 ++-- internal/milter/mailparse.go | 327 ++++++++++++++++++ internal/milter/mailparse_test.go | 163 +++++++++ internal/milter/sender.go | 202 +++-------- internal/milter/sender_test.go | 82 ++--- internal/milter/testdata/from-lines-other.txt | 12 +- internal/milter/testdata/mailparse_harness.c | 55 +++ internal/web/view/help.go | 2 +- test/e2e/dkim_from_test.go | 19 +- test/e2e/sender_check_test.go | 10 +- 16 files changed, 734 insertions(+), 268 deletions(-) create mode 100644 internal/milter/mailparse.go create mode 100644 internal/milter/mailparse_test.go create mode 100644 internal/milter/testdata/mailparse_harness.c diff --git a/CHANGELOG.md b/CHANGELOG.md index c723062..c00a0f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,19 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version case, owners may be separated by blanks or continue on the next line, and a key written twice makes the check defer, as it makes Postfix refuse the whole table (assumptions B-PS1, B-PS4). +- **The sender check reads `From:` with OpenDKIM's own parser.** It used its + own parser and, to cover where the two could disagree, refused any line + naming another domain of the server in a display name or a comment - which + told an application, by refused or passed, which domains are hosted here + (audit 2026-10-12, L8, an accepted risk until now). It now runs OpenDKIM's + `dkim_mail_parse()`, ported line for line and tested against the C on two + hundred thousand lines, and judges the address OpenDKIM signs for. A display + name or a comment no longer matters - a line with another sender's address + only in its display name now passes, signed with the application's own + key; a line OpenDKIM finds no address in, or two addresses in, is refused, + and so is an address over 256 bytes, which OpenDKIM would match against its + signing table cut short - cut there, a quoted local part could end in + another domain's name. - **Over IPv6 every client looks the same to SelfPost** - documented, not changed. The compose network has no IPv6, so Docker's proxy carries an IPv6 connection and the container sees the network's gateway diff --git a/cmd/panel/sendercheck.go b/cmd/panel/sendercheck.go index c42daf5..a8a8147 100644 --- a/cmd/panel/sendercheck.go +++ b/cmd/panel/sendercheck.go @@ -6,7 +6,6 @@ import ( "os" "time" - "github.com/mixeme/selfpost/internal/domain" "github.com/mixeme/selfpost/internal/health" "github.com/mixeme/selfpost/internal/milter" "github.com/mixeme/selfpost/internal/postfix" @@ -67,8 +66,7 @@ func serveSenderCheck(ctx context.Context, cfg config, st *store.Store) error { // The two tables the mail path itself goes by: whom Postfix lets send as // what, and what OpenDKIM signs. bind := milter.NewBindings(postfix.New(cfg.postfixDir).SenderLoginMapsPath()) - signers := milter.NewSigners(domain.NewOpenDKIM(cfg.opendkimDir).SigningTablePath()) - return milter.ServeSender(ctx, ln, bind, signers, st) + return milter.ServeSender(ctx, ln, bind, st) } // postfixState is what supervisord said about Postfix: since when it runs, if diff --git a/docs/architecture.md b/docs/architecture.md index b972d58..cca09d2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -286,19 +286,17 @@ server itself. `src/global/mail_addr_find.c`, `mail_addr_find_opt()`). A key written twice makes Postfix refuse the whole table, and the check defers then too. - **How the header is read.** Exactly one `From:` field (none, or more than - one, is refused), parsed as an address list that must hold exactly one - address. Then a cruder second look at the raw value: OpenDKIM finds the - address with a parser of its own, and wherever the two could read a line - differently - a display name, a comment, a quoted string - the other - address would have to be spelled out in it. So no `@name` may appear there - that is another name the server signs for; those names are read from - OpenDKIM's own `SigningTable`; the display name is looked at once more - as decoded, since a mail program shows `=?utf-8?q?director=40example.org?=` - as an address though no `@` is written in it. And, needing no knowledge of - that parser: - outside quoted strings the line has exactly one `@`. A folded line is - unfolded first, and a display name in a charset Go does not convert is - passed through unread - only the address is judged. + one, is refused), read with OpenDKIM's own parser: libopendkim's + `dkim_mail_parse()`, ported line for line from Debian's opendkim + 2.11.0~beta2 (`internal/milter/mailparse.go`) and tested against the C on + two hundred thousand lines. The check asks Postfix for header values with + their leading space, as OpenDKIM does (`SMFIP_HDR_LEADSPC`), so both read the + same bytes. The address it returns is the one judged - the one OpenDKIM + signs for - whatever a display name, a comment or a quoted local part + holds; a parse failure, no address, or a second one after it is refused, + and so is an address over 256 bytes, which OpenDKIM would match its + `SigningTable` against cut short (`dkimf_apply_signtable()`, + `MAXADDRESS`). Encoded words are not decoded, by either. - **The verdict** comes at the end of the headers: `550 5.7.1`, permanent, naming the line, as a reply code of the milter's own. What the client wrote is reduced to printable ASCII and cut before it is said back, logged or diff --git a/docs/audit-2026-10-12.md b/docs/audit-2026-10-12.md index 6a07a44..d9dadbb 100644 --- a/docs/audit-2026-10-12.md +++ b/docs/audit-2026-10-12.md @@ -852,7 +852,7 @@ guard against two parsers parting ways - and it is what makes L8 half closed. This stage replaces the guard with the parser itself. Stage 3 and those after it move up one patch number each. -- [ ] **Port `dkim_mail_parse` to Go, from the version the image runs** +- [x] **Port `dkim_mail_parse` to Go, from the version the image runs** (Debian bookworm's opendkim 2.11.0~beta2 source package, not upstream's develop branch), line for line, with its tokenizer (`dkim_mail_first_special`, `dkim_mail_token`). The check then decides by @@ -865,6 +865,16 @@ those after it move up one patch number each. here (L8 closed; security.md's accepted risk struck). Whether the "one @ outside quotes" rule is still needed is decided against the port, and said. - **Opus** + *Done 2026-10-15:* `internal/milter/mailparse.go`; the C itself compiled + from the image's source agrees with the port on 200 019 lines + (`testdata/mailparse_harness.c`). The "one @ outside quotes" rule goes: + the owner's decision says the address decides "whatever the display name + or a comment holds", and with OpenDKIM's parser there is no second one to + part ways with. Found reading the source: OpenDKIM matches its + SigningTable against "user@domain" cut to 256 bytes (MAXADDRESS), so a + longer address is refused - cut there, a quoted local part could end in + another tenant's domain. The check asks for header values with their + leading space, as OpenDKIM does; the SigningTable reader goes. - [ ] **The port checked against OpenDKIM itself.** On the stand, several hundred generated `From:` lines - quotes, escapes, comments nested and open, groups, brackets, encoded words, a second domain of the server in diff --git a/docs/guide.md b/docs/guide.md index 75f1640..5466380 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -1204,13 +1204,13 @@ Postfix asks who owns the address before it asks who owns the domain. A message whose `From:` is anything else is refused with `550 5.7.1` and a text naming the line: another of your domains, the server's own name, an address at a domain that is not on this server, a line with two addresses, -or no `From:` at all. The line is read strictly, because two programs read -it - the check and the signer - and must not be able to read it differently: -outside a quoted display name it may hold one `@`, the address's own, and no -other name this server signs for may follow an `@` anywhere in it, a quoted -name included. A display name with a comma or brackets has to be quoted -(`"Shop, Inc." `), as the mail standard asks and mail libraries -do. The refusal +or no `From:` at all. The line is read the way the signer, OpenDKIM, reads +it - the panel runs OpenDKIM's own parser - so only its address counts: what a +display name or a comment around it says is not judged, and a line with more +than one address is refused. A display name with a comma or brackets has to +be quoted (`"Shop, Inc." `), as the mail standard asks and mail +libraries do; unquoted, OpenDKIM may take a word of the name for the +address. The refusal is permanent, so the application does not send the message again; it is a row of the [Outbound log](#outbound-log) tagged `wrong From`, and the row's page shows what the line said. diff --git a/docs/roadmap.md b/docs/roadmap.md index 46ba06f..d2963b2 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -42,7 +42,7 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md). | from-header-binding | Hold a message's `From:` header against the application that sends it | shipped `2.4.0` | 9/9 | [plans/from-header-binding.md](plans/from-header-binding.md) | | panel-redesign | Panel redesign (accepted mockups, design contract) | shipped `2.0.0` | 27/27 | [plans/panel-redesign.md](plans/panel-redesign.md) | | schema-squash | Squash SQLite migrations into a 2.x baseline | built in panel-redesign (stage 1) | — | [plans/panel-redesign.md](plans/panel-redesign.md) | -| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`, stages 1+2 `2.5.0`, 2b `2.5.1`; next stage 2c → `2.5.2`) | 46/81 | [audit-2026-10-12.md](audit-2026-10-12.md) | +| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`, stages 1+2 `2.5.0`, 2b `2.5.1`; next stage 2c → `2.5.2`) | 47/81 | [audit-2026-10-12.md](audit-2026-10-12.md) | **Recommended order** (not binding; owner, 2026-10-09: feature order follows development convenience): **panel-redesign** shipped as `2.0.0` (2026-10-11) and diff --git a/docs/security.md b/docs/security.md index 5a97463..42ba128 100644 --- a/docs/security.md +++ b/docs/security.md @@ -215,28 +215,27 @@ operator can act on it. does not sign for included (the owner's rule: the header may say what the envelope may, nothing more). It is the first milter of the chain and strict: a message it could not judge is deferred, never passed on to the - signer. It reads the header as one field holding one address and then - looks at the raw value once more for any other name of OpenDKIM's - `SigningTable`, so that a line its parser and OpenDKIM's could read - differently is refused rather than argued about; outside quoted strings - 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; - 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 - accepted by the owner - (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 - as mail, and no key of another's is involved. What the client wrote + signer. Since `2.5.2` it reads the header with OpenDKIM's own parser, + `dkim_mail_parse()` ported line for line from the opendkim the image runs + (`internal/milter/mailparse.go`), and judges the address that parser + returns - the address OpenDKIM signs for - whatever a display name, a + comment or a quoted local part holds; one `From:` field and one address in + it stay required, and an address longer than the 256 bytes OpenDKIM + matches its `SigningTable` against is refused, since cut there it could end + in another domain's name (opendkim.c, `dkimf_apply_signtable()`, + `MAXADDRESS`). That the port is OpenDKIM's parser is tested against the C + itself, two hundred thousand lines and every result alike; that the signed + domain follows, by the e2e: it sends every `From:` line the check passes (a + list shared with the unit test, + `internal/milter/testdata/from-lines-passed.txt`) and wants it signed for + the application's domain and no other, and every + line of a second list naming 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 or arrive signed for the + application's domain, never for that sender. What this does not promise is + what a display name looks like to a person: a name of another sender in + it passes, signed with the application's own key - a forged display name is + as old as mail, and no key of another's is involved. What the client wrote reaches the reply, the log and the send log only as printable ASCII, cut, and a refused message is one row of the log whatever its recipient count. Mail the panel submits itself does not pass the check: only root, @@ -459,17 +458,13 @@ 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 by the owner, - 2026-10-14: it needs an application of this server - a trusted tenant - guessing names one at a time, as it already had to before L8, when the - reply only said in words what the line had named - and closing it means - refusing every foreign address in a display name, which would change his - 2.4.0 rule (a stranger's address in the name passes). Trigger to revisit: - tenants who are not trusted, or a display-name rule decided anew. +- ~~**An application can learn which domains are senders of this server.**~~ + Struck in `2.5.2` (audit 2026-10-12, stage 2c; L8 closed): the sender + check reads a `From:` line with OpenDKIM's own parser, ported + (`internal/milter/mailparse.go`), and decides by the address OpenDKIM signs + for; what a display name, a comment or a quoted local part names no longer + matters, so refused or passed says nothing about other domains of the + server. - ~~**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/mailparse.go b/internal/milter/mailparse.go new file mode 100644 index 0000000..eee2aab --- /dev/null +++ b/internal/milter/mailparse.go @@ -0,0 +1,327 @@ +package milter + +// The address OpenDKIM signs for, found as OpenDKIM finds it: a port of +// libopendkim's dkim_mail_parse() and its tokenizer, from Debian's +// opendkim 2.11.0~beta2-8+deb12u1 (libopendkim/dkim-mailparse.c, identical +// there to upstream's develop branch), line for line. OpenDKIM calls it on the +// value of the message's first From: field (opendkim/opendkim.c, mlfi_eoh(), +// dkimf_findheader(dfc, "from", 0): build/opendkim.conf sets no SenderHeaders), +// lower-cases the domain it returns, and signs with the key whose SigningTable +// pattern matches "user@domain" (dkimf_apply_signtable()). Whatever the port +// returns, OpenDKIM does: quoted strings and domain literals are kept with +// their delimiters, comments are dropped, a source route resets the address, +// an '@' before the first ':', ';' or ',' decides where the address starts, +// and encoded words are not decoded. +// +// The C works in place on a NUL-terminated buffer, writing the result over +// the input; here the input is read through at(), which gives 0 at its end as +// the C finds the terminator, and the result goes to a buffer of its own, in +// which 0 ends a string as '\0' does there. + +// mailparse errors, DKIM_MAILPARSE_ERR_* (dkim-mailparse.c). +const ( + mailparseOK = 0 + mailparsePUnbalanced = 1 // unbalanced parentheses + mailparseQUnbalanced = 2 // unbalanced quotes + mailparseSUnbalanced = 3 // unbalanced square brackets +) + +// mailparseSpecials is SPECIALS: "<>@,;:\\\"/[]?=". +var mailparseSpecials = func() (t [256]bool) { + for _, c := range []byte("<>@,;:\\\"/[]?=") { + t[c] = true + } + return t +}() + +// mailparseSpace is isascii(c) && isspace(c) in the C locale. +func mailparseSpace(c byte) bool { + return c == ' ' || (c >= '\t' && c <= '\r') +} + +type mailparser struct { + b []byte // the input up to its first NUL, as strlen() sees it +} + +// at is *p: the byte, or the terminator at and beyond the end. +func (m *mailparser) at(i int) byte { + if i >= len(m.b) { + return 0 + } + return m.b[i] +} + +// matchingParen is dkim_mail_matching_paren(): the position of the close +// character that balances the open one before s, or e. An open character of +// 0 is never met inside the string, so for quotes and brackets it only skips +// to the close, past backslash escapes. +func (m *mailparser) matchingParen(s, e int, open, close byte) int { + paren := 1 + for ; s < e; s++ { + c := m.b[s] + if c == close { + paren-- + if paren == 0 { + break + } + } else if c == open { + paren++ + } else if c == '\\' { + if m.at(s+1) != 0 { + s++ + } + } + } + return s +} + +// firstSpecial is dkim_mail_first_special(): the '<' that opens an address, +// or the ':', ';' or ',' that ends a part - or the '@' seen before it - or +// the end. +func (m *mailparser) firstSpecial(p, e int) (int, int) { + atPtr := -1 + for ; p < e && m.at(p) != 0; p++ { + // skip white space between tokens + for p < e && (m.at(p) == '(' || mailparseSpace(m.at(p))) { + if m.at(p) != '(' { + p++ + } else { + p = m.matchingParen(p+1, e, '(', ')') + if m.at(p) == 0 { + return p, mailparsePUnbalanced + } + p++ + } + } + if m.at(p) == 0 { + break + } + c := m.at(p) + switch { + case c == '"': + p = m.matchingParen(p+1, e, 0, '"') + if m.at(p) == 0 { + return p, mailparseQUnbalanced + } + case c == '[': + p = m.matchingParen(p+1, e, 0, ']') + if m.at(p) == 0 { + return p, mailparseSUnbalanced + } + case mailparseSpecials[c]: + if c == '<' { + return p, mailparseOK + } else if c == ':' || c == ';' || c == ',' { + if atPtr >= 0 { + return atPtr, mailparseOK + } + return p, mailparseOK + } else if c == '@' { + atPtr = p + } + default: + for m.at(p) != 0 && !mailparseSpecials[m.at(p)] && !mailparseSpace(m.at(p)) && m.at(p) != '(' { + p++ + } + p-- + } + } + return p, mailparseOK +} + +// token is dkim_mail_token(): the next token from s - a quoted string or a +// domain literal with its delimiters, a special character alone, or an atom +// ('x') - its type (0 at the end), and whether whitespace outside a comment +// came before it (ws is set, never cleared, as in the C). +func (m *mailparser) token(s, e int, ws *bool) (typ int, start, end int, err int) { + p := s + // skip white space between tokens + for p < e && (m.at(p) == '(' || mailparseSpace(m.at(p))) { + if m.at(p) != '(' { + *ws = true + p++ + } else { + p = m.matchingParen(p+1, e, '(', ')') + if m.at(p) == 0 { + return 0, 0, 0, mailparsePUnbalanced + } + p++ + } + } + if p >= e || m.at(p) == 0 { + return 0, 0, 0, mailparseOK + } + start = p + c := m.at(p) + switch { + case c == '"': + end = m.matchingParen(p+1, e, 0, '"') + typ = '"' + if m.at(end) != 0 { + end++ + } else { + err = mailparseQUnbalanced + } + case c == '[': + end = m.matchingParen(p+1, e, 0, ']') + typ = '[' + if m.at(end) != 0 { + end++ + } else { + err = mailparseSUnbalanced + } + case mailparseSpecials[c]: + end = p + 1 + typ = int(c) + default: + for p < e && m.at(p) != 0 && !mailparseSpecials[m.at(p)] && !mailparseSpace(m.at(p)) && m.at(p) != '(' { + p++ + } + end = p + typ = 'x' + } + return typ, start, end, err +} + +// mailParsed is what dkim_mail_parse() returns: the local part and the +// domain, each absent (NULL) or a string, and the error. rest is not the +// C's: where in the line it stopped reading, for the caller's question +// whether anything follows the address. +type mailParsed struct { + user, domain string + hasUser, hasDomain bool + err int + rest int +} + +// dkimMailParse is dkim_mail_parse(). +func dkimMailParse(line string) mailParsed { + if i := indexByte(line, 0); i >= 0 { + line = line[:i] // strlen() + } + m := &mailparser{b: []byte(line)} + e := len(m.b) + var out []byte + user, domain := -1, -1 + result := func(err, rest int) mailParsed { + r := mailParsed{err: err, rest: rest} + if user >= 0 { + r.user, r.hasUser = cstring(out[user:]), true + } + if domain >= 0 { + r.domain, r.hasDomain = cstring(out[domain:]), true + } + return r + } + ws := false + pos := 0 + for { + special, err := m.firstSpecial(pos, e) + if err != 0 { + return result(err, special) + } + switch m.at(special) { + case '<': + // display name
+ pos = special + 1 + for { + typ, ts, te, err := m.token(pos, e, &ws) + if err != 0 { + return result(err, pos) + } + if typ == '>' || typ == 0 { + out = append(out, 0) + if typ == 0 { + te = e + } + return result(mailparseOK, te) + } else if typ == '@' { + out = append(out, 0) + domain = len(out) + } else if typ == ',' || typ == ':' { + // source route punctuation + user, domain = -1, -1 + } else { + if user < 0 { + user = len(out) + } + out = append(out, m.b[ts:te]...) + } + pos = te + } + case ';', ':', ',': + // skip a group name or result + pos = special + 1 + default: + // (display name) addr(display name)ess + ws = false + for { + typ, ts, te, err := m.token(pos, e, &ws) + if err != 0 { + return result(err, pos) + } + if typ == 0 || typ == ',' || typ == ';' { + out = append(out, 0) + if typ == 0 { + te = e + } + return result(mailparseOK, te) + } else if typ == '@' { + out = append(out, 0) + domain = len(out) + ws = false + } else { + if user < 0 { + user = len(out) + } else if typ == 'x' && ws { + out = append(out, ' ') + } + out = append(out, m.b[ts:te]...) + ws = false + } + pos = te + } + } + } +} + +// moreAfter says whether anything but list and group punctuation follows +// position rest of line: another address, or text after the one found. +func moreAfter(line string, rest int) bool { + if i := indexByte(line, 0); i >= 0 { + line = line[:i] + } + m := &mailparser{b: []byte(line)} + ws := false + for pos := rest; ; { + typ, _, te, err := m.token(pos, len(m.b), &ws) + if err != 0 { + return true + } + switch typ { + case 0: + return false + case ',', ';': + pos = te + default: + return true + } + } +} + +func indexByte(s string, c byte) int { + for i := 0; i < len(s); i++ { + if s[i] == c { + return i + } + } + return -1 +} + +// cstring reads a NUL-terminated string from b. +func cstring(b []byte) string { + if i := indexByte(string(b), 0); i >= 0 { + return string(b[:i]) + } + return string(b) +} diff --git a/internal/milter/mailparse_test.go b/internal/milter/mailparse_test.go new file mode 100644 index 0000000..82be29d --- /dev/null +++ b/internal/milter/mailparse_test.go @@ -0,0 +1,163 @@ +package milter + +import ( + "bufio" + "encoding/hex" + "fmt" + "math/rand" + "os" + "os/exec" + "strings" + "testing" +) + +// mailparseCases are lines whose result is written down here, read off +// dkim-mailparse.c by hand and confirmed against it by +// TestMailParseMatchesOpenDKIM. +var mailparseCases = []struct { + line string + user, domain string // "-" for absent + err int +}{ + {"user@example.com", "user", "example.com", 0}, + {" Name ", "user", "example.com", 0}, + {`"Some One" `, "user", "example.com", 0}, + {"user@example.com (Some One)", "user", "example.com", 0}, + {"Some One ", "user", "example.com", 0}, + // An '@' before the first ',' decides: the address is read from the start. + {"ceo@other.com ", "me", "mine.com", 0}, + {"me@mine.com, you@yours.com", "me", "mine.com", 0}, + // Quoted local parts keep their quotes; an '@' inside them is no '@'. + {`"x@other.com"@mine.com`, `"x@other.com"`, "mine.com", 0}, + {`<"a b"@mine.com>`, `"a b"`, "mine.com", 0}, + // A source route is dropped. + {"<@relay.example,@r2.example:u@mine.com>", "u", "mine.com", 0}, + // Group syntax: the name is skipped. + {"group: u@mine.com;", "u", "mine.com", 0}, + // No local part: the domain is taken for it, so OpenDKIM has both. + {"@mine.com", "mine.com", "mine.com", 0}, + // Whitespace between atoms of the default form becomes one space. + {"a b@mine.com", "a b", "mine.com", 0}, + // Unbalanced. + {"Name (comment ", "-", "-", mailparsePUnbalanced}, + {`"open `, "-", "-", mailparseQUnbalanced}, + {"u@[1.2.3.4", "-", "-", mailparseSUnbalanced}, + // Encoded words are not decoded. + {"=?utf-8?q?boss=40other.com?= ", "u", "mine.com", 0}, + // No address at all. + {"Undisclosed recipients:;", "-", "-", 0}, + {"", "-", "-", 0}, +} + +func parsedParts(r mailParsed) (string, string) { + u, d := "-", "-" + if r.hasUser { + u = r.user + } + if r.hasDomain { + d = r.domain + } + return u, d +} + +func TestMailParseCases(t *testing.T) { + for _, c := range mailparseCases { + r := dkimMailParse(c.line) + u, d := parsedParts(r) + if r.err != c.err || (c.err == 0 && (u != c.user || d != c.domain)) { + t.Errorf("%q: user %q domain %q err %d; want %q %q %d", c.line, u, d, r.err, c.user, c.domain, c.err) + } + } +} + +func TestMoreAfter(t *testing.T) { + for line, want := range map[string]bool{ + "user@example.com": false, + "Name ": false, + "Name (note)": false, + "group: u@mine.com;": false, + "me@mine.com, you@yours.com": true, + "Name , b@y": true, + "Name trailing": true, + } { + r := dkimMailParse(line) + if got := moreAfter(line, r.rest); got != want { + t.Errorf("%q: moreAfter %v, want %v", line, got, want) + } + } +} + +// mailparseCorpus is the cases above and n random lines over an alphabet +// weighted toward what the parser treats specially. +func mailparseCorpus(n int) []string { + lines := []string{} + for _, c := range mailparseCases { + lines = append(lines, c.line) + } + alphabet := []string{"<", ">", "@", ",", ";", ":", "\\", `"`, "/", "[", "]", "?", "=", "(", ")", + " ", "\t", "\n", "\r", ".", "a", "b", "x", "mine.com", "other.com", "\xc3\xa9", "\xff", "=?utf-8?q?", "?="} + rng := rand.New(rand.NewSource(20261015)) + for i := 0; i < n; i++ { + var b strings.Builder + for j := rng.Intn(24); j >= 0; j-- { + b.WriteString(alphabet[rng.Intn(len(alphabet))]) + } + lines = append(lines, b.String()) + } + return lines +} + +// TestMailParseMatchesOpenDKIM feeds the corpus to OpenDKIM's own C +// dkim_mail_parse() through testdata/mailparse_harness.c, built against the +// opendkim source the image runs, and wants the port's answer for every line. +// It runs where MAILPARSE_C names the built harness. +func TestMailParseMatchesOpenDKIM(t *testing.T) { + bin := os.Getenv("MAILPARSE_C") + if bin == "" { + t.Skip("MAILPARSE_C not set (see testdata/mailparse_harness.c)") + } + lines := mailparseCorpus(200000) + var in strings.Builder + for _, l := range lines { + in.WriteString(hex.EncodeToString([]byte(l))) + in.WriteByte('\n') + } + cmd := exec.Command(bin) + cmd.Stdin = strings.NewReader(in.String()) + out, err := cmd.Output() + if err != nil { + t.Fatalf("harness: %v", err) + } + sc := bufio.NewScanner(strings.NewReader(string(out))) + sc.Buffer(make([]byte, 1<<20), 1<<20) + enc := func(present bool, s string) string { + if !present { + return "-" + } + return "=" + hex.EncodeToString([]byte(s)) + } + diffs, failed := 0, 0 + for i := 0; sc.Scan(); i++ { + r := dkimMailParse(lines[i]) + got := fmt.Sprintf("%d %s %s", r.err, enc(r.hasUser, r.user), enc(r.hasDomain, r.domain)) + want := sc.Text() + if r.err != 0 { + // On an error the C returns before it writes the result's end, so + // what it prints of user and domain runs on into the unread + // input; OpenDKIM uses neither then (mlfi_eoh(): status != 0 is + // BADFORMAT, no signature). The error itself must agree. + failed++ + got, want = fmt.Sprint(r.err), strings.Fields(want)[0] + } + if got != want { + diffs++ + if diffs <= 20 { + t.Errorf("line %q: port %s, OpenDKIM %s", lines[i], got, want) + } + } + } + if diffs > 0 { + t.Fatalf("%d of %d lines differ", diffs, len(lines)) + } + t.Logf("%d lines, the port and OpenDKIM agree on every one (%d of them errors, compared by code)", len(lines), failed) +} diff --git a/internal/milter/sender.go b/internal/milter/sender.go index bec1851..cda8970 100644 --- a/internal/milter/sender.go +++ b/internal/milter/sender.go @@ -3,11 +3,8 @@ package milter import ( "context" "fmt" - "io" "log" - "mime" "net" - "net/mail" "net/textproto" "os" "strings" @@ -26,9 +23,13 @@ import ( // that line, so without the check an application could have mail signed as // another domain of the server, or as the server itself. // +// The line is read with OpenDKIM's own parser, ported (mailparse.go), so the +// address judged is the one OpenDKIM signs for. +// // Unlike the journal milter this one is attached strictly // (default_action=tempfail in build/postfix-config.sh): while it does not -// answer, Postfix defers mail. It therefore depends on as little as it can. +// answer, Postfix answers the client with a temporary error. It therefore +// depends on as little as it can. // It reads no database: what an application may send as comes from the file // of sender bindings Postfix itself reads for the envelope, so the two go by // one table, read the way Postfix reads it and looked up the way Postfix looks @@ -258,51 +259,6 @@ func (b *Bindings) DomainOf(login string) string { return b.domainOf[strings.ToLower(login)] } -// Signers is OpenDKIM's SigningTable as the check reads it: the names this -// server signs for - every sending domain, with applications or without, and -// the server's own name. -type Signers struct { - mu sync.Mutex - file tableFile - names map[string]bool -} - -// NewSigners reads the file at path on first use. -func NewSigners(path string) *Signers { - return &Signers{file: tableFile{path: path}} -} - -// Signs reports whether mail whose From: is at domain gets a signature. -// -// OpenDKIM 2.11.0~beta2, opendkim/opendkim-db.c, dkimf_db_open(), case -// DKIMF_DB_TYPE_REFILE: the pattern is a line up to its first blank, "#" -// starts a comment; opendkim/util.c, dkimf_mkregexp(): "^" + the pattern -// ("*" as ".*", "." escaped) + "$", compiled REG_ICASE (DKIMF_DB_FLAG_ICASE, -// opendkim.c). So "*@d" signs user@d in any case, and no subdomain of d. -func (s *Signers) Signs(domain string) (bool, error) { - s.mu.Lock() - defer s.mu.Unlock() - lines, again, err := s.file.changed() - if err != nil { - return false, err - } - if again { - names := make(map[string]bool) - for _, line := range lines { - // "*@ " (internal/domain renderTables). - fields := strings.Fields(line) - if len(fields) < 2 || strings.HasPrefix(fields[0], "#") { - continue - } - if d := domainOf(fields[0]); d != "" { - names[d] = true - } - } - s.names = names - } - return s.names[strings.ToLower(domain)], nil -} - // Refusals is where the check leaves the send-log rows of what it refused. // *store.Store satisfies it. type Refusals interface { @@ -313,7 +269,6 @@ type Refusals interface { type senderSession struct { milter.NoOpMilter bind *Bindings - signers *Signers refused chan<- store.SendLogEntry login string @@ -411,58 +366,58 @@ func (s *senderSession) judge() (shown, why string, err error) { default: return printable(strings.Join(s.froms, " | ")), "the message has more than one From: line", nil } - // Postfix hands a milter a header as it came, folded lines and all; the - // address parser reads one line. - raw := unfold(s.froms[0]) + // The value is the one OpenDKIM gets - Postfix hands both milters the + // field as it came, folded lines and all, and with the leading space each + // asked for (OptHeaderLeadingSpace, ServeSender) - and it is read with + // OpenDKIM's own parser (mailparse.go). So the address judged here is the + // address OpenDKIM signs for, whatever the display name, a comment or a + // quoted string around it holds; the domain of the key it signs with is + // that address's (assumptions B-PM12; audit 2026-10-12, stage 2c). + raw := s.froms[0] shown = printable(raw) - list, perr := fromParser.ParseList(raw) - if perr != nil || len(list) != 1 || list[0].Address == "" { + r := dkimMailParse(raw) + if r.err != mailparseOK || !r.hasUser || !r.hasDomain || r.user == "" || r.domain == "" { + // OpenDKIM does not sign then either (mlfi_eoh(): BADFORMAT). + return shown, "it holds no address OpenDKIM can read", nil + } + if moreAfter(raw, r.rest) { + // OpenDKIM takes the first; a reader may show the others. return shown, "it must be one address", nil } - addr := strings.ToLower(list[0].Address) + addr := r.user + "@" + asciiLower(r.domain) + // OpenDKIM matches its SigningTable against "user@domain" as + // snprintf() leaves it in a buffer of MAXADDRESS + 1 = 257 bytes + // (opendkim.c, dkimf_apply_signtable(); libopendkim/dkim-internal.h). + // Cut there, a quoted local part could end in another tenant's + // "@domain" and pick that tenant's key; an address that fits is read + // whole, and its domain (DKIM_MAXHOSTNAMELEN, 256) with it. + if len(addr) > 256 { + return printable(addr), "the address is longer than OpenDKIM reads whole", nil + } shown = printable(addr) - ok, err := s.bind.Allowed(s.login, addr) + ok, err := s.bind.Allowed(s.login, asciiLower(addr)) if err != nil { return shown, "", fmt.Errorf("sender bindings: %w", err) } if !ok { return shown, "not an address this application sends as", nil } - // A cruder second look. OpenDKIM finds the address in this line with a - // parser of its own, and the key it signs with follows what that parser - // finds. Wherever the two could read the line differently - a display - // name, a comment, a quoted string - the other address would have to be - // spelled out in it, so no other name this server signs for may appear. - // - // The display name is looked at once more as a reader will see it: a mail - // program decodes an encoded word, and "=?utf-8?q?director=40example.org?=" - // shows as an address of another sender though no "@" is written in it. - own := domainOf(addr) - for _, d := range append(mentionedDomains(raw), mentionedDomains(list[0].Name)...) { - if d == own { - continue - } - signs, err := s.signers.Signs(d) - if err != nil { - return shown, "", fmt.Errorf("signing table: %w", err) - } - if signs { - // Which one is not said: the reply would confirm to whoever - // tries names that another tenant sends from this server (audit - // 2026-10-12, L8). The line as written is in the reply and on - // the refused message's page. - return printable(raw), "it names another sender of this server", nil - } - } - // And one that needs to know nothing of OpenDKIM's parser: outside quoted - // strings the line has the one "@" of its address. A second one - in a - // comment, in an unquoted name - is where two parsers could part ways. - if strings.Count(unquoted(raw), "@") != 1 { - return printable(raw), "outside quotes it must have one @, the address's own", nil - } return shown, "", nil } +// asciiLower lower-cases A-Z only, as OpenDKIM's dkimf_lowercase() and +// Postfix's table lookups do; strings.ToLower would also fold characters +// outside ASCII - the Kelvin sign to "k" - into names they are not. +func asciiLower(v string) string { + b := []byte(v) + for i, c := range b { + if c >= 'A' && c <= 'Z' { + b[i] = c + 'a' - 'A' + } + } + return string(b) +} + // record leaves a row for the Outbound log. It never waits: a database that // is slow must not make the check slow. One row for the message, not one for // each recipient as an accepted message has: nothing was sent to anybody, and @@ -496,37 +451,6 @@ func (s *senderSession) record(shown, queueID string) { } } -// fromParser reads the From: line. A display name encoded in a charset Go -// does not convert (windows-1251, koi8-r, ...) is still a display name: its -// bytes are passed through unread, since only the address is judged. -var fromParser = mail.AddressParser{WordDecoder: &mime.WordDecoder{ - CharsetReader: func(charset string, input io.Reader) (io.Reader, error) { return input, nil }, -}} - -// unfold joins the lines of a folded header value (RFC 5322 2.2.3): a line -// break is dropped, the white space that follows it stays. -func unfold(v string) string { - return strings.NewReplacer("\r\n", "", "\n", "", "\r", "").Replace(v) -} - -// unquoted returns v without its quoted strings, a backslash inside one -// escaping the next character. -func unquoted(v string) string { - var sb strings.Builder - in := false - for i := 0; i < len(v); i++ { - switch c := v[i]; { - case in && c == '\\': - i++ - case c == '"': - in = !in - case !in: - sb.WriteByte(c) - } - } - return sb.String() -} - // domain is the sending domain of the refused message for its row in the // log: the envelope sender's, which Postfix has held against the login - or, // for the null sender of a bounce an application writes itself, which Postfix @@ -543,29 +467,6 @@ func (s *senderSession) domain() string { return s.bind.DomainOf(s.login) } -// mentionedDomains returns, lower-cased, what follows every "@" of v as far -// as it reads as a host name. -func mentionedDomains(v string) []string { - var out []string - for i := 0; i < len(v); i++ { - if v[i] != '@' { - continue - } - j := i + 1 - for j < len(v) && isHostByte(v[j]) { - j++ - } - if d := strings.Trim(strings.ToLower(v[i+1:j]), ".-"); d != "" { - out = append(out, d) - } - } - return out -} - -func isHostByte(c byte) bool { - return c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' || c == '.' || c == '-' -} - // printable reduces what a client wrote to printable ASCII without the // characters that mean something in an SMTP reply or a log line, and cuts it: // it is said back to the client, logged and stored. @@ -600,7 +501,7 @@ func printable(v string) string { // ServeSender runs the sender check on ln until ctx is cancelled. rec may be // nil: refusals are then not logged. What it relies on of go-milter and // Postfix is cited at Serve in milter.go. -func ServeSender(ctx context.Context, ln net.Listener, bind *Bindings, signers *Signers, rec Refusals) error { +func ServeSender(ctx context.Context, ln net.Listener, bind *Bindings, rec Refusals) error { var refused chan store.SendLogEntry if rec != nil { refused = make(chan store.SendLogEntry, 256) @@ -619,10 +520,17 @@ func ServeSender(ctx context.Context, ln net.Listener, bind *Bindings, signers * } srv := &milter.Server{ NewMilter: func() milter.Milter { - return &senderSession{bind: bind, signers: signers, refused: refused} + return &senderSession{bind: bind, refused: refused} }, - Actions: 0, // read-only - Protocol: milter.OptNoBody, // the headers decide + Actions: 0, // read-only + // The headers decide; and each header value with its leading space, + // as OpenDKIM asks for it (opendkim.c, mlfi_negotiate(), + // SMFIP_HDR_LEADSPC), so that both read the same bytes. Postfix + // 3.7.11, src/milter/milter8.c: milter8_negotiate() keeps the event + // mask a milter of any version asks for (go-milter v0.4.1 says + // version 2), and milter8_header() drops the space only for a milter + // without SMFIP_HDR_LEADSPC. + Protocol: milter.OptNoBody | milter.OptHeaderLeadingSpace, } go func() { diff --git a/internal/milter/sender_test.go b/internal/milter/sender_test.go index 5ae55e9..437ac0f 100644 --- a/internal/milter/sender_test.go +++ b/internal/milter/sender_test.go @@ -24,22 +24,6 @@ invoices@example.org billing billing@example.org billing ` -// signingTable is a SigningTable for the same server, composed by hand: the -// three sending domains, one more that has no application yet, and the -// server's own name. Its key-name column is not what internal/domain -// renderTables writes (the domain name); the check reads only the pattern. -const signingTable = `*@news.example.net selfpost._domainkey.news.example.net -*@shop.example.org selfpost._domainkey.shop.example.org -*@example.org selfpost._domainkey.example.org -*@empty.example.com selfpost._domainkey.empty.example.com -*@mail.example.org selfpost._domainkey.mail.example.org -` - -func signers(t *testing.T) *Signers { - t.Helper() - return NewSigners(writeMaps(t, signingTable)) -} - func writeMaps(t *testing.T, content string) string { t.Helper() p := filepath.Join(t.TempDir(), "table") @@ -54,7 +38,7 @@ func writeMaps(t *testing.T, content string) string { func offer(t *testing.T, bind *Bindings, login, envelope string, headers ...[2]string) (milter.Response, []store.SendLogEntry) { t.Helper() refused := make(chan store.SendLogEntry, 8) - s := &senderSession{bind: bind, signers: signers(t), refused: refused} + s := &senderSession{bind: bind, refused: refused} if _, err := s.MailFrom(envelope, mods(map[string]string{"{auth_authen}": login})); err != nil { t.Fatal(err) } @@ -109,6 +93,18 @@ func TestSenderCheck(t *testing.T) { {"a name in windows-1251", "shop", [][2]string{from("=?windows-1251?B?zODj4Ofo7Q==?= ")}, true, ""}, {"a name in koi8-r, quoted-printable", "shop", [][2]string{from("=?koi8-r?Q?=ED=C1=C7=C1=DA=C9=CE?= ")}, true, ""}, {"a comment after the address", "shop", [][2]string{from("a@shop.example.org (the shop)")}, true, ""}, + {"the leading space Postfix keeps for OpenDKIM", "shop", [][2]string{from(" a@shop.example.org")}, true, ""}, + // Whatever the name, a comment or a quoted local part holds: OpenDKIM + // signs for the address it reads, and that is the one judged. + {"another sender in the display name", "shop", [][2]string{from(`"director@example.org" `)}, true, ""}, + {"the server's name in a comment", "shop", [][2]string{from("a@shop.example.org (postmaster@mail.example.org)")}, true, ""}, + {"a domain with a key and no application, in the display name", "shop", [][2]string{from(`"x@empty.example.com" `)}, true, ""}, + {"a stranger's address in the display name", "shop", [][2]string{from(`"someone@gmail.example" `)}, true, ""}, + {"another sender in an encoded display name", "shop", [][2]string{from("=?utf-8?q?director=40example=2Eorg?= ")}, true, ""}, + {"a stranger's address in a comment", "shop", [][2]string{from("a@shop.example.org (someone@gmail.example)")}, true, ""}, + {"an unquoted address as the display name", "shop", [][2]string{from("director@example.org ")}, true, ""}, + {"another domain inside a quoted local part", "shop", [][2]string{from(`"director@example.org"@shop.example.org`)}, true, ""}, + {"no local part: OpenDKIM takes the domain for it", "shop", [][2]string{from("@shop.example.org")}, true, ""}, {"another registered domain", "shop", [][2]string{from("director@example.org")}, false, "not an address this application sends as"}, {"the server's own name", "shop", [][2]string{from("postmaster@mail.example.org")}, false, "not an address this application sends as"}, @@ -120,16 +116,14 @@ func TestSenderCheck(t *testing.T) { {"no From: line", "shop", [][2]string{{"Subject", "hello"}}, false, "no From: line"}, {"two From: lines", "shop", [][2]string{from("a@shop.example.org"), from("director@example.org")}, false, "more than one From: line"}, {"two addresses in one line", "shop", [][2]string{from("a@shop.example.org, director@example.org")}, false, "one address"}, - {"not an address", "shop", [][2]string{from("the shop")}, false, "one address"}, + {"not an address", "shop", [][2]string{from("the shop")}, false, "no address OpenDKIM can read"}, + {"an address before a list comma, then another", "shop", [][2]string{from("director@example.org, ")}, false, "one address"}, + // Past 256 bytes OpenDKIM matches its SigningTable against a cut + // "user@domain" - which could end in another tenant's domain. + {"an address longer than OpenDKIM reads", "shop", [][2]string{from(`"` + strings.Repeat("a", 240) + `@example.org"@shop.example.org`)}, false, "longer than OpenDKIM reads whole"}, {"a group", "shop", [][2]string{from("friends: a@shop.example.org, b@shop.example.org;")}, false, "one address"}, - {"another sender in the display name", "shop", [][2]string{from(`"director@example.org" `)}, false, "names another sender of this server"}, - {"the server's name in a comment", "shop", [][2]string{from("a@shop.example.org (postmaster@mail.example.org)")}, false, "names another sender of this server"}, - {"a domain with a key and no application, in the display name", "shop", [][2]string{from(`"x@empty.example.com" `)}, false, "names another sender of this server"}, - {"a stranger's address in the display name", "shop", [][2]string{from(`"someone@gmail.example" `)}, true, ""}, - {"another sender in an encoded display name", "shop", [][2]string{from("=?utf-8?q?director=40example=2Eorg?= ")}, false, "names another sender of this server"}, - {"a stranger's address in a comment", "shop", [][2]string{from("a@shop.example.org (someone@gmail.example)")}, false, "one @"}, {"another sender folded onto the next line", "shop", [][2]string{from("\"x\"\r\n ")}, false, "not an address this application sends as"}, - {"a quote left open before another address", "shop", [][2]string{from(`"a@shop.example.org `)}, false, "one address"}, + {"a quote left open before another address", "shop", [][2]string{from(`"a@shop.example.org `)}, false, "no address OpenDKIM can read"}, {"no login", "", [][2]string{from("a@shop.example.org")}, false, "no authenticated application"}, {"an unknown login", "ghost", [][2]string{from("a@shop.example.org")}, false, "not an address this application sends as"}, } @@ -290,30 +284,10 @@ func TestBindingsDuplicateKeyBreaksTheTable(t *testing.T) { } } -// Without the signing table the second look cannot be taken, and a line it -// would have to judge is deferred; a line that names nobody else is not held -// up by it. -func TestSenderCheckDefersWithoutSigningTable(t *testing.T) { - bind := NewBindings(writeMaps(t, senderMaps)) - play := func(v string) milter.Response { - s := &senderSession{bind: bind, signers: NewSigners(filepath.Join(t.TempDir(), "absent"))} - _, _ = s.MailFrom("shop@shop.example.org", mods(map[string]string{"{auth_authen}": "shop"})) - _, _ = s.Header("From", v, mods(nil)) - resp, _ := s.Headers(nil, mods(nil)) - return resp - } - if resp := play(`"x@example.org" `); resp != milter.RespTempFail { - t.Fatalf("reply %s, want a temporary failure", replyText(resp)) - } - if resp := play(`a@shop.example.org`); resp != milter.RespContinue { - t.Fatalf("a plain own address: %s", replyText(resp)) - } -} - // A second message on the connection is judged by its own headers. func TestSenderCheckResetsBetweenMessages(t *testing.T) { bind := NewBindings(writeMaps(t, senderMaps)) - s := &senderSession{bind: bind, signers: signers(t)} + s := &senderSession{bind: bind} auth := mods(map[string]string{"{auth_authen}": "shop"}) for i, v := range []string{"director@example.org", "a@shop.example.org", "director@example.org"} { _, _ = s.MailFrom("shop@shop.example.org", auth) @@ -329,7 +303,7 @@ func TestSenderCheckResetsBetweenMessages(t *testing.T) { func TestSenderCheckRefusalIsOneRow(t *testing.T) { bind := NewBindings(writeMaps(t, senderMaps)) refused := make(chan store.SendLogEntry, 8) - s := &senderSession{bind: bind, signers: signers(t), refused: refused} + s := &senderSession{bind: bind, refused: refused} _, _ = s.MailFrom("shop@shop.example.org", mods(map[string]string{"{auth_authen}": "shop"})) for _, r := range []string{"a@x.example", "b@x.example", "c@x.example"} { _, _ = s.RcptTo(r, mods(nil)) @@ -392,14 +366,18 @@ func fromLines(t *testing.T, file, domain, other string) []string { 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) { +// No line of the shared file of lines naming another sender is passed to be +// signed with that sender's key: a line the check passes is one OpenDKIM +// reads the application's own address in (the e2e looks at the signature). +func TestSenderCheckNeverPassesTheSharedLinesForAnother(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) + if resp != milter.RespContinue { + continue + } + if r := dkimMailParse(line); asciiLower(r.domain) != "shop.example.org" { + t.Errorf("From: %q passed, and OpenDKIM reads the domain %q in it", line, r.domain) } } } diff --git a/internal/milter/testdata/from-lines-other.txt b/internal/milter/testdata/from-lines-other.txt index 1630a7a..0e8ecbb 100644 --- a/internal/milter/testdata/from-lines-other.txt +++ b/internal/milter/testdata/from-lines-other.txt @@ -1,10 +1,12 @@ # 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 +# 2). None may be signed with {other}'s key. Since stage 2c the check reads a +# line with OpenDKIM's own parser, so it passes those whose address OpenDKIM +# reads at {domain} - {other} in a display name, a comment or a quoted local +# part - and refuses the rest. The unit test +# (internal/milter/sender_test.go) wants every line it passes read at +# {domain}; 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. The lines are # composed by hand; that OpenDKIM signs none for {other} is measured, by the # e2e on every run. diff --git a/internal/milter/testdata/mailparse_harness.c b/internal/milter/testdata/mailparse_harness.c new file mode 100644 index 0000000..1d3d466 --- /dev/null +++ b/internal/milter/testdata/mailparse_harness.c @@ -0,0 +1,55 @@ +/* + * Differential harness for internal/milter/mailparse.go: OpenDKIM's own + * dkim_mail_parse() (libopendkim/dkim-mailparse.c of the opendkim source the + * image runs, compiled in by #include), fed one hex-encoded line per input + * line. Output per line: the error, then the local part and the domain, each + * "-" when NULL and "=" followed by its bytes in hex otherwise. + * + * cc -o mailparse_harness -I /libopendkim mailparse_harness.c + * MAILPARSE_C=./mailparse_harness go test -run TestMailParseMatchesOpenDKIM ./internal/milter + */ +#include +#include +#include +#include +#include "dkim-mailparse.c" + +static int hexval(int c) +{ + if (c >= '0' && c <= '9') return c - '0'; + if (c >= 'a' && c <= 'f') return c - 'a' + 10; + return -1; +} + +static void put(const unsigned char *s) +{ + if (s == NULL) { + fputs(" -", stdout); + return; + } + fputs(" =", stdout); + for (; *s != '\0'; s++) + printf("%02x", *s); +} + +int main(void) +{ + static char hex[1 << 20]; + static unsigned char buf[1 << 19]; + + while (fgets(hex, sizeof hex, stdin) != NULL) { + size_t n = strcspn(hex, "\r\n"), i; + unsigned char *user, *domain; + int err; + + for (i = 0; i + 1 < n; i += 2) + buf[i / 2] = (unsigned char) (hexval(hex[i]) << 4 | hexval(hex[i + 1])); + buf[n / 2] = '\0'; + err = dkim_mail_parse(buf, &user, &domain); + printf("%d", err); + put(user); + put(domain); + putchar('\n'); + } + return 0; +} diff --git a/internal/web/view/help.go b/internal/web/view/help.go index ae60a93..cd1eb56 100644 --- a/internal/web/view/help.go +++ b/internal/web/view/help.go @@ -169,7 +169,7 @@ var outboundTopics = []HelpTopic{ Rich("An application sends only as itself, and that holds for both senders a message has. The envelope sender is held against the login by Postfix. The ", Code("From:"), " line, the one a mail program shows and the one the DKIM signature is chosen by, is held against it by the panel, under the same rule: an application set to any address of its domain may write any address of that domain, and one with a list of addresses only those. A message with any other ", Code("From:"), " - another of your domains, the server's own name, an address somewhere else, or none at all - is refused for good with ", - Code("550 5.7.1"), ". It is a rejected row of the Outbound log, and its page says which address was refused. This is what keeps one application from getting mail signed with another domain's key.", + Code("550 5.7.1"), ". The line is read the way the signer, OpenDKIM, reads it, so only its address counts: what a display name or a comment around it says is not judged, and a line holding more than one address is refused. It is a rejected row of the Outbound log, and its page says which address was refused. This is what keeps one application from getting mail signed with another domain's key.", Br(), Br(), "The check is strict. While the panel does not answer, Postfix does not accept mail unchecked: it replies with a temporary error and the application sends again later. Server › Health counts what that costs: for how long the panel did not answer while Postfix itself was running; how many connections Postfix turned away because of it - it refuses from the connection on, before a message is offered, so a client that tries three times is three and a monitor that only connects is one; and how many messages were refused with a temporary error in the middle, when the panel stopped answering after a message had begun. A time marked ≈ includes a start at which supervisord could not say since when Postfix was running: the time since the panel's last sign of life is counted, which may be more than Postfix was up. The card warns when a connection was turned away or a message refused in the last 24 hours. If the numbers stay at nothing, the check is where it should be; if they do not, it belongs in a process of its own."), }}, diff --git a/test/e2e/dkim_from_test.go b/test/e2e/dkim_from_test.go index 017f268..f6cb2bf 100644 --- a/test/e2e/dkim_from_test.go +++ b/test/e2e/dkim_from_test.go @@ -89,9 +89,11 @@ 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). +// testNeverSignedForAnother: no line that names another sender of the server +// arrives signed with that sender's key (review of audit stage 2). Since stage +// 2c the check reads a line as OpenDKIM does: one whose address is the +// application's passes, wherever the other sender's name is written, and must +// arrive signed for the application's domain; the rest are refused. func testNeverSignedForAnother(t *testing.T, sc *scenario) { lines, err := fromLines(h.repoRoot, "from-lines-other.txt", senderDomain, otherDomain) if err != nil { @@ -107,16 +109,25 @@ func testNeverSignedForAnother(t *testing.T, sc *scenario) { 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()) + if !res.ok() { + t.Errorf("From: %q: neither passed nor refused by the check: %v", line, res.firstErr()) + continue + } raw, err := findSinkMessage(h.stageDir, token, 15*time.Second) if err != nil { + t.Errorf("From: %q passed and never arrived: %v", line, err) continue } verifications, _ := dkim.VerifyWithOptions(bytes.NewReader(raw), &dkim.VerifyOptions{LookupTXT: lookupTXTFunc()}) + own := false for _, v := range verifications { if v.Domain == otherDomain { t.Errorf("From: %q arrived signed for %s", line, otherDomain) } + own = own || (v.Domain == senderDomain && v.Err == nil) + } + if !own { + t.Errorf("From: %q passed and did not arrive signed for %s: %+v", line, senderDomain, verifications) } } } diff --git a/test/e2e/sender_check_test.go b/test/e2e/sender_check_test.go index 3a14844..e72987a 100644 --- a/test/e2e/sender_check_test.go +++ b/test/e2e/sender_check_test.go @@ -66,7 +66,15 @@ func testSenderCheck(t *testing.T, sc *scenario) { // app-list, so the wildcard application may not have it, in the // header as in the envelope (assumptions B-PS1; the envelope below). {"the address listed for another application", wildcard, "billing@" + senderDomain, false}, - {"another sender in the display name", wildcard, `"director@` + otherDomain + `" `, false}, + // Since 2.5.2 the address OpenDKIM reads decides (OpenDKIM's parser, + // internal/milter/mailparse.go): another sender's name in the display + // name passes, and is signed for the application's own domain + // (passed_from_lines_signed_for_their_domain_only looks at the key). + {"another sender in the display name", wildcard, `"director@` + otherDomain + `" `, true}, + // Past 256 bytes OpenDKIM matches its SigningTable against the + // address cut short, which here would end in the other domain's + // name (opendkim.c, dkimf_apply_signtable(), MAXADDRESS). + {"an address longer than OpenDKIM reads", wildcard, `"` + strings.Repeat("a", 256-len("@"+otherDomain)-1) + `@` + otherDomain + `x"@` + senderDomain, false}, } { token := uniqueToken("from") res := attemptSend(sendAttempt{