from-header-binding: findings of the technical and security reviews
test / test (push) Canceled after 0s
test / test (push) Canceled after 0s
Both reviews were of c171fb5, read-only, given the code and questions.
Found by both, and the one that would have refused ordinary mail from the
first day: a folded From: line - what a mail library makes of a long display
name - did not parse and was refused with 550. The line is unfolded before it
is read. e2e sends one through Postfix now.
Technical (Opus)
- A display name in a charset Go does not convert (windows-1251, koi8-r) was
refused: such a name is passed through unread, only the address is judged.
- The statistics ran between binding the socket and serving it, with a
supervisorctl that has no timeout: they are kept beside the check now and
nothing of them is waited for. A supervisorctl that fails is logged.
- The count of deferred mail followed smtpd's reject lines and so missed a
check that stops answering in the middle of a message, where cleanup
writes them: it is taken at Postfix's warning, the line that names the
socket. The test line for that case had been invented with the wrong
process; it is marked as composed from Postfix's source.
- A card whose figures could not be read showed zeros and "ok": dashes and
"unknown".
- entrypoint removes the sockets a killed container left, which satisfied
the wrapper's wait before the check was listening.
- Outside quoted strings the line may hold one "@" only - a rule that needs
no knowledge of OpenDKIM's parser.
- Comments that still spoke of two sockets, or of a session replaced after a
refusal; the limits of the cards layout on a phone, said where it is defined.
Security (Fable; no Critical or High)
- Another sender's name in an encoded display name passed the second look:
the decoded name is looked at as well.
- A client could arm the deferred counter by sending the socket's name where
Postfix quotes it in a warning: the two warnings are matched from their
first word.
- A refused message wrote a row per recipient: one row, naming the first
recipient and how many more.
- Docs: the check's lookup is stricter than Postfix's for address extensions
and the case of logins, rather than "cannot disagree"; what a display name
looks like to a person is not promised; the timeouts and the 454 at AUTH
stated as they are; "messages" are connections turned away.
Left as found, in the plan: downtime that crosses midnight is booked on the
day of the restart; a message with the null envelope sender leaves a row
without a domain; a log read again after an unclean stop counts again.
Test server, this tree: go vet ok, go test 23 packages ok, make e2e ok (330 s).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
+7
-2
@@ -142,8 +142,8 @@ chmod 2750 /data/log
|
||||
find /data/log -type f -exec chmod 0640 {} +
|
||||
|
||||
# Milter socket directories. Postfix (user `postfix`) must actually CONNECT to
|
||||
# both milter sockets — OpenDKIM's and the panel's journal-milter — not just
|
||||
# probe them at start-up. The sockets are created by the opendkim and panel
|
||||
# the milter sockets — OpenDKIM's and the panel's — not just probe them at
|
||||
# start-up. The sockets are created by the opendkim and panel
|
||||
# users respectively, so bridge them to `postfix` through the shared `selfpost`
|
||||
# group: group-owned + setgid dirs mean each socket created inside inherits
|
||||
# group `selfpost`, and group-traversable (2750) lets postfix reach it. Without
|
||||
@@ -153,6 +153,11 @@ mkdir -p /run/opendkim /run/selfpost
|
||||
chown opendkim:selfpost /run/opendkim
|
||||
chown panel:selfpost /run/selfpost
|
||||
chmod 2750 /run/opendkim /run/selfpost
|
||||
# A container that was killed leaves its sockets behind, and a file nobody
|
||||
# listens on would satisfy postfix-wrapper.sh's wait: Postfix would start
|
||||
# before the sender check does and defer mail until it is there. The check's
|
||||
# alive mark stays - it is how the downtime of a restart is told.
|
||||
rm -f /run/opendkim/*.sock /run/selfpost/*.sock
|
||||
|
||||
# Generate the outbound-relay Postfix configuration from the environment (spec
|
||||
# 5). Kept out of the image build so cert paths, rate limits, hostname and the
|
||||
|
||||
@@ -93,8 +93,9 @@ fi
|
||||
RATE_MSGS="${RATE_LIMIT_MESSAGES_PER_IP:-100}"
|
||||
RATE_WINDOW="${RATE_LIMIT_WINDOW_SECONDS:-3600}"
|
||||
|
||||
# Milter sockets: OpenDKIM (signing, strict) and the panel journal-milter
|
||||
# (monitoring, fail-open). Fixed container paths, matched by postfix-wrapper.sh.
|
||||
# Milter sockets: OpenDKIM (signing, strict), the panel's sender check
|
||||
# (strict) and its journal-milter (monitoring, fail-open). Fixed container
|
||||
# paths, matched by postfix-wrapper.sh.
|
||||
OPENDKIM_SOCK="${OPENDKIM_SOCKET:-/run/opendkim/opendkim.sock}"
|
||||
JOURNAL_SOCK="${JOURNAL_MILTER_SOCKET:-/run/selfpost/journal.sock}"
|
||||
# The panel's sender check (the From: line against the login, strict).
|
||||
|
||||
@@ -9,8 +9,9 @@
|
||||
# timeout it exits non-zero WITHOUT starting Postfix, so supervisord/Docker see
|
||||
# an explicit startup failure instead of a relay running blind.
|
||||
#
|
||||
# This handles cold start only. Runtime milter failures after a successful start
|
||||
# are governed by fail-open (milter_default_action).
|
||||
# This handles cold start only. Once Postfix runs, a milter that does not
|
||||
# answer costs what its default_action says (build/postfix-config.sh): mail is
|
||||
# deferred for OpenDKIM and the sender check, and passes the journal-milter.
|
||||
set -eu
|
||||
|
||||
OPENDKIM_SOCK="${OPENDKIM_SOCKET:-/run/opendkim/opendkim.sock}"
|
||||
|
||||
@@ -1,8 +1,9 @@
|
||||
; SelfPost process supervision.
|
||||
;
|
||||
; Start ORDER is enforced by priority=: OpenDKIM, then the panel (which opens
|
||||
; the journal-milter socket), then the Postfix wrapper — which additionally
|
||||
; blocks until both milter sockets are ready before starting Postfix.
|
||||
; the sender check's and the journal-milter's sockets), then the Postfix
|
||||
; wrapper — which additionally blocks until the milter sockets are ready
|
||||
; before starting Postfix.
|
||||
;
|
||||
; If any managed process exhausts its restart retries (FATAL), the crashexit
|
||||
; event listener brings the whole container down so Docker's restart policy can
|
||||
|
||||
+17
-12
@@ -30,20 +30,23 @@ func serveSenderCheck(ctx context.Context, cfg config, st *store.Store) error {
|
||||
}
|
||||
log.Printf("sender-check listening on %s", cfg.senderSocket)
|
||||
|
||||
// Statistics are the database's business and the check is not: none of
|
||||
// this may keep the check from serving.
|
||||
now := time.Now()
|
||||
mark := cfg.senderSocket + ".alive"
|
||||
if err := st.StartSenderCheckStats(now); err != nil {
|
||||
log.Printf("sender-check: statistics: %v", err)
|
||||
}
|
||||
if down := senderCheckDowntime(mark, now, postfixStarted); down > 0 {
|
||||
log.Printf("sender-check: was not answering for %s while Postfix was running", down.Round(time.Second))
|
||||
if err := st.AddSenderCheckDown(now, down); err != nil {
|
||||
// Statistics are the database's business and supervisord's, and the check
|
||||
// is neither: they are kept beside it, and nothing of them is waited for
|
||||
// before it serves.
|
||||
go func() {
|
||||
now := time.Now()
|
||||
mark := cfg.senderSocket + ".alive"
|
||||
if err := st.StartSenderCheckStats(now); err != nil {
|
||||
log.Printf("sender-check: statistics: %v", err)
|
||||
}
|
||||
}
|
||||
go keepAlive(ctx, mark, st)
|
||||
if down := senderCheckDowntime(mark, now, postfixStarted); down > 0 {
|
||||
log.Printf("sender-check: was not answering for %s while Postfix was running", down.Round(time.Second))
|
||||
if err := st.AddSenderCheckDown(now, down); err != nil {
|
||||
log.Printf("sender-check: statistics: %v", err)
|
||||
}
|
||||
}
|
||||
keepAlive(ctx, mark, st)
|
||||
}()
|
||||
|
||||
// The two tables the mail path itself goes by: whom Postfix lets send as
|
||||
// what, and what OpenDKIM signs.
|
||||
@@ -80,6 +83,8 @@ func senderCheckDowntime(path string, now time.Time, postfixStarted func() (time
|
||||
func postfixStarted() (time.Time, bool) {
|
||||
procs, err := health.Processes()
|
||||
if err != nil {
|
||||
// Said aloud: the downtime of this start is then not counted.
|
||||
log.Printf("sender-check: statistics: when Postfix started is not known: %v", err)
|
||||
return time.Time{}, false
|
||||
}
|
||||
for _, p := range procs {
|
||||
|
||||
+14
-4
@@ -233,8 +233,10 @@ server itself.
|
||||
bound to the login, or `@domain` is. So an application set to any address of
|
||||
its domain may write any address of that domain, and one with a list only
|
||||
the listed ones; everything else is refused - another registered domain,
|
||||
the server's name, a domain the server knows nothing of. One source for
|
||||
both senders: the two cannot disagree.
|
||||
the server's name, a domain the server knows nothing of. One table for
|
||||
both senders. Where the lookups differ the check is the stricter one:
|
||||
Postfix also tries an address without its `+extension` and compares logins
|
||||
without regard to case.
|
||||
- **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
|
||||
@@ -242,7 +244,13 @@ server itself.
|
||||
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`.
|
||||
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.
|
||||
- **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
|
||||
@@ -257,7 +265,9 @@ server itself.
|
||||
table from before would let a deleted application go on sending. The
|
||||
send-log row of a refusal (`status=rejected`, `reason=from`,
|
||||
`header_from`) is handed to another goroutine through a buffer and is not
|
||||
waited for.
|
||||
waited for. It is one row for the message, naming the first recipient and
|
||||
how many more there were: a row per recipient would let one refused message
|
||||
write a thousand.
|
||||
- **What it costs is counted.** The role marks itself alive every two
|
||||
seconds (the socket's path with `.alive`, under `/run`, so a new container
|
||||
starts without one). At start the time since the mark is the check's
|
||||
|
||||
+15
-6
@@ -392,7 +392,10 @@ readings, in the order of what stops mail first:
|
||||
panel did not answer while Postfix was running, and how many messages
|
||||
Postfix deferred because of it (see [Who an application may send
|
||||
as](#who-an-application-may-send-as)). A restart of the whole container is
|
||||
not counted: mail was stopped for another reason then. The card warns when
|
||||
not counted: mail was stopped for another reason then. What is counted as
|
||||
a message is a connection Postfix turned away - it refuses before a
|
||||
message is offered - so a monitor that only connects during an outage is
|
||||
counted too. Days are UTC days. The card warns when
|
||||
a message was deferred in the last 24 hours. If these numbers do not stay
|
||||
at nothing, the panel is in the way of your mail more than it should be -
|
||||
report it.
|
||||
@@ -1097,8 +1100,13 @@ listed for it. Since `2.4.0` that rule covers both senders a message has:
|
||||
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. A name this server signs for may not appear anywhere
|
||||
in the line besides the address itself, a display name included. The refusal
|
||||
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." <orders@…>`), as the mail standard asks and mail libraries
|
||||
do. 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.
|
||||
@@ -1113,9 +1121,10 @@ What this stops working when you upgrade from an earlier version:
|
||||
`Reply-To:` and send from an address of your own, which is also what
|
||||
DMARC requires.
|
||||
|
||||
The check is strict: while the panel does not answer, Postfix replies `451`
|
||||
and applications send again later; nothing is accepted unchecked. **Server ›
|
||||
Health** counts how often that happens.
|
||||
The check is strict: while the panel does not answer, Postfix answers with a
|
||||
temporary error (`451`, and `454` to the login) and applications send again
|
||||
later; nothing is accepted unchecked. **Server › Health** counts how often
|
||||
that happens.
|
||||
|
||||
*Export domain* writes a single-domain file; *Import a domain* on *Server ›
|
||||
Backup* reads one back in (**global role only**) — see [Exporting and importing
|
||||
|
||||
@@ -146,5 +146,5 @@ the fourth was not.
|
||||
- [x] Panel: Health (card, sockets row, layout), Outbound log tags, the message's page, help
|
||||
- [x] guide.md, architecture.md, security.md (the accepted risk goes), schema-migrations.md
|
||||
- [x] `go vet`, `go test`, e2e: refused for another domain's `From:`, for the hostname's and for an address off the list; accepted for its own (on the test server: vet ok, 23 packages ok, e2e ok - `sender_check_holds_the_from_line`, `panel_down_defers_mail`)
|
||||
- [ ] Technical review — **Opus**
|
||||
- [ ] Security review — **Fable**
|
||||
- [x] Technical review — **Opus** (2026-10-12; one Critical, fixed: a folded `From:` line - what a mail library makes of a long display name - was refused for good. Also fixed: a display name in a charset Go does not convert was refused; the statistics ran before the check served, and a hung `supervisorctl` would have kept it from serving; the count of deferred mail missed a check that stops answering in the middle of a message, and is now taken at Postfix's warning; a card whose figures could not be read said ok; a killed container's stale socket satisfied the wrapper's wait; outside quoted strings the line may now hold one `@` only. Left as found: downtime that crosses midnight is booked on the day of the restart; a message with the null envelope sender leaves a row without a domain; a log read again after an unclean stop counts its deferrals again)
|
||||
- [x] Security review — **Fable** (2026-10-12; no Critical or High. The same folded line, as its one Medium. Fixed: another sender's name in an encoded display name passed the second look; a client could arm the deferred counter by writing the socket's name where Postfix quotes it in a warning; a refused message wrote a row per recipient, a thousand at most - now one. Recorded: the check's lookup is stricter than Postfix's in two places, address extensions and the case of logins; what a display name looks like to a person is not promised. Confirmed from the code: no path to the signer around the check, and every failure of the check defers)
|
||||
|
||||
+1
-1
@@ -39,7 +39,7 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md).
|
||||
| delivery-log-storage | Keep a message's `mail.log` lines with its send-log row | shipped `2.2.0` | 8/8 | [plans/delivery-log-storage.md](plans/delivery-log-storage.md) |
|
||||
| password-reset | Password reset by e-mail | candidate | — | — |
|
||||
| phone-desktop-mode | A phone browser in "desktop site" mode gets the phone navigation | candidate | — | — |
|
||||
| from-header-binding | Hold a message's `From:` header against the application that sends it | in progress, for `2.4.0` | 7/9 | [plans/from-header-binding.md](plans/from-header-binding.md) |
|
||||
| from-header-binding | Hold a message's `From:` header against the application that sends it | built and reviewed, for `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) |
|
||||
|
||||
|
||||
+11
-4
@@ -202,8 +202,14 @@ operator can act on it.
|
||||
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. What the client wrote
|
||||
reaches the reply, the log and the send log only as printable ASCII, cut.
|
||||
differently is refused rather than argued about; outside quoted strings
|
||||
the line may hold one `@`. 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
|
||||
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,
|
||||
`postfix` and the panel can submit locally. Architecture § Sender check.
|
||||
- **Inbound relay (optional, `INBOUND_RELAY_ENABLE`)** — port 25 is not an
|
||||
@@ -459,8 +465,9 @@ deferred item from the roadmap.
|
||||
the rules arrive at.
|
||||
- **The sender check stops mail while the panel does not answer.** It is a
|
||||
role of the panel process, attached with `default_action=tempfail`, so a
|
||||
panel that is down or slower than the milter timeouts (15 s a command)
|
||||
defers every submission: availability of sending now depends on the panel,
|
||||
panel that is down or slower than the milter timeouts (15 s to connect
|
||||
and for a command, 30 s for the verdict at the end of the headers) defers
|
||||
every submission: availability of sending now depends on the panel,
|
||||
which it did not before `2.4.0`. Chosen by the owner over the alternative,
|
||||
a check that is absent whenever the panel is - that one could be switched
|
||||
off by whoever can make the panel slow. Supervisord restarts the panel at
|
||||
|
||||
@@ -13,7 +13,8 @@ import (
|
||||
// writes about it, and shown on Server > Health. It is the figure that also
|
||||
// sees a panel which is running and not answering.
|
||||
//
|
||||
// Postfix says it in lines of one smtpd process - why, then what:
|
||||
// Postfix says it in lines of one smtpd process - why, then what (seen on
|
||||
// the e2e stand, 2026-10-12):
|
||||
//
|
||||
// postfix/smtpd[1183]: warning: connect to Milter service unix:/run/selfpost/sender.sock: No such file or directory
|
||||
// postfix/smtpd[1183]: NOQUEUE: milter-reject: CONNECT from unknown[10.77.0.1]: 451 4.7.1 Service unavailable - try again later; proto=SMTP
|
||||
@@ -24,10 +25,13 @@ import (
|
||||
// away, once: a client brings a message when it connects, and it will bring
|
||||
// it again.
|
||||
//
|
||||
// The reject lines do not name the milter, and the same words are written
|
||||
// when OpenDKIM does not answer or when the journal milter refuses under a
|
||||
// level-2 limit. So a process is remembered from the warning that names this
|
||||
// socket, and counted at its next reject line.
|
||||
// It is counted at the warning, which is the one line that names the socket.
|
||||
// The reject lines do not: the same words are written when OpenDKIM does not
|
||||
// answer and when the journal milter refuses under a level-2 limit, and for a
|
||||
// check that stops answering in the middle of a message they are written by
|
||||
// cleanup, another process, under a queue id. The warning for that case is
|
||||
// "milter unix:<socket>: can't read … reply packet header" (Postfix's
|
||||
// milter8.c; not seen on the stand).
|
||||
|
||||
// SenderCheckStore is where the count goes. *store.Store satisfies it.
|
||||
type SenderCheckStore interface {
|
||||
@@ -44,7 +48,6 @@ type SenderCheck struct {
|
||||
var smtpdLineRe = regexp.MustCompile(`^\S+ postfix/(?:[a-z0-9-]+/)?smtpd\[(\d+)\]: (.*)$`)
|
||||
|
||||
const (
|
||||
milterUnavailable = "Service unavailable - try again later"
|
||||
// senderCheckPids bounds the processes remembered; smtpd processes are a
|
||||
// few dozen, and a log that names more is not kept track of.
|
||||
senderCheckPids = 1024
|
||||
@@ -52,13 +55,20 @@ const (
|
||||
|
||||
// deferredCounter is the per-run state of the count.
|
||||
type deferredCounter struct {
|
||||
st SenderCheckStore
|
||||
needle string // the socket as Postfix names it in a warning
|
||||
failed map[string]bool // smtpd pid -> the check did not answer its current client
|
||||
st SenderCheckStore
|
||||
// The two warnings, up to and including the socket's name: nobody
|
||||
// listening, and a reply that did not come.
|
||||
connect, broken string
|
||||
failed map[string]bool // smtpd pid -> its current client was counted already
|
||||
}
|
||||
|
||||
func newDeferredCounter(sc *SenderCheck) *deferredCounter {
|
||||
return &deferredCounter{st: sc.Store, needle: "unix:" + sc.Socket, failed: make(map[string]bool)}
|
||||
return &deferredCounter{
|
||||
st: sc.Store,
|
||||
connect: "warning: connect to Milter service unix:" + sc.Socket + ":",
|
||||
broken: "warning: milter unix:" + sc.Socket + ":",
|
||||
failed: make(map[string]bool),
|
||||
}
|
||||
}
|
||||
|
||||
// handle looks at one mail.log line.
|
||||
@@ -72,17 +82,18 @@ func (c *deferredCounter) handle(line string, now time.Time) {
|
||||
switch {
|
||||
case strings.HasPrefix(text, "connect from "):
|
||||
delete(c.failed, pid)
|
||||
case strings.HasPrefix(text, "warning: ") && strings.Contains(text, c.needle) && strings.Contains(strings.ToLower(text), "milter"):
|
||||
// "connect to Milter service <socket>: …" when nobody listens, and
|
||||
// "milter <socket>: can't read … reply packet header: …" when the
|
||||
// check stopped answering in the middle of a message.
|
||||
case strings.HasPrefix(text, c.connect) || strings.HasPrefix(text, c.broken):
|
||||
// Postfix's own two sentences, from their first word: a warning also
|
||||
// quotes what a client sent ("Illegal address syntax ... in MAIL
|
||||
// command: <text>"), and a client can send the socket's name.
|
||||
// Once for the connection, however many warnings it draws.
|
||||
if c.failed[pid] {
|
||||
return
|
||||
}
|
||||
if len(c.failed) >= senderCheckPids {
|
||||
c.failed = make(map[string]bool)
|
||||
}
|
||||
c.failed[pid] = true
|
||||
case c.failed[pid] && strings.Contains(text, "milter-reject: ") && strings.Contains(text, milterUnavailable):
|
||||
// Once for the connection, however many commands the client tries.
|
||||
delete(c.failed, pid)
|
||||
if err := c.st.AddSenderCheckDeferred(at, 1); err != nil {
|
||||
log.Printf("log-tailer: sender check statistics: %v", err)
|
||||
}
|
||||
|
||||
@@ -17,9 +17,12 @@ func (d *deferredStore) AddSenderCheckDeferred(at time.Time, n int64) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// The lines are Postfix 3.7's, as the e2e stand wrote them with the panel
|
||||
// stopped (2026-10-12), with the lines of two other things that use the same
|
||||
// words: a level-2 refusal of the journal milter and OpenDKIM not answering.
|
||||
// The lines of process 1183 and of cleanup are as the e2e stand wrote them
|
||||
// with the panel stopped and for a refused From: (2026-10-12; the stamps are
|
||||
// put in ISO form). Around them are the lines of two other things that use
|
||||
// the same words - a level-2 refusal of the journal milter, OpenDKIM not
|
||||
// answering - and, composed from Postfix's source rather than seen, a check
|
||||
// that stops answering in the middle of a message.
|
||||
func TestDeferredCounter(t *testing.T) {
|
||||
st := &deferredStore{}
|
||||
c := newDeferredCounter(&SenderCheck{Store: st, Socket: "/run/selfpost/sender.sock"})
|
||||
@@ -41,6 +44,12 @@ func TestDeferredCounter(t *testing.T) {
|
||||
`2026-10-12T16:23:30.000000+00:00 mail postfix/smtpd[1183]: connect from unknown[10.77.0.1]`,
|
||||
`2026-10-12T16:23:30.100000+00:00 mail postfix/smtpd[1183]: NOQUEUE: milter-reject: MAIL from unknown[10.77.0.1]: 451 4.7.1 Service unavailable - try again later; from=<a@sender.e2e.test> proto=ESMTP helo=<x>`,
|
||||
|
||||
// A client that writes the socket's name where Postfix will quote it
|
||||
// in a warning, and then runs into a level-2 refusal: not counted.
|
||||
`2026-10-12T16:23:40.000000+00:00 mail postfix/smtpd[1190]: connect from unknown[10.77.0.1]`,
|
||||
`2026-10-12T16:23:40.100000+00:00 mail postfix/smtpd[1190]: warning: Illegal address syntax from unknown[10.77.0.1] in MAIL command: <warning: connect to Milter service unix:/run/selfpost/sender.sock: milter>`,
|
||||
`2026-10-12T16:23:40.200000+00:00 mail postfix/smtpd[1190]: NOQUEUE: milter-reject: MAIL from unknown[10.77.0.1]: 451 4.7.1 Service unavailable - try again later; from=<a@sender.e2e.test> proto=ESMTP helo=<x>`,
|
||||
|
||||
// OpenDKIM does not answer: another socket, not counted.
|
||||
`2026-10-12T16:24:00.000000+00:00 mail postfix/smtpd[1201]: connect from unknown[10.77.0.1]`,
|
||||
`2026-10-12T16:24:00.100000+00:00 mail postfix/smtpd[1201]: warning: connect to Milter service unix:/run/opendkim/opendkim.sock: Connection refused`,
|
||||
@@ -49,7 +58,8 @@ func TestDeferredCounter(t *testing.T) {
|
||||
// The check stops answering in the middle of a message, the next day.
|
||||
`2026-10-13T00:00:05.000000+00:00 mail postfix/smtpd[1300]: connect from unknown[10.77.0.1]`,
|
||||
`2026-10-13T00:00:21.000000+00:00 mail postfix/smtpd[1300]: warning: milter unix:/run/selfpost/sender.sock: can't read SMFIC_EOH reply packet header: Connection timed out`,
|
||||
`2026-10-13T00:00:21.100000+00:00 mail postfix/smtpd[1300]: 4XcB7k2Jm9z1: milter-reject: END-OF-HEADER from unknown[10.77.0.1]: 4.7.1 Service unavailable - try again later; from=<a@sender.e2e.test> to=<b@sink.e2e.test> proto=ESMTP helo=<x>`,
|
||||
`2026-10-13T00:00:21.050000+00:00 mail postfix/smtpd[1300]: warning: milter unix:/run/selfpost/sender.sock: can't read SMFIC_BODYEOB reply packet header: Connection timed out`,
|
||||
`2026-10-13T00:00:21.100000+00:00 mail postfix/cleanup[1195]: 4XcB7k2Jm9z1: milter-reject: END-OF-MESSAGE from unknown[10.77.0.1]: 4.7.1 Service unavailable - try again later; from=<a@sender.e2e.test> to=<b@sink.e2e.test> proto=ESMTP helo=<x>`,
|
||||
|
||||
// The check's own verdict is a refusal, not a deferral, and it is
|
||||
// cleanup that writes it.
|
||||
|
||||
+78
-26
@@ -3,7 +3,9 @@ package milter
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"io"
|
||||
"log"
|
||||
"mime"
|
||||
"net"
|
||||
"net/mail"
|
||||
"net/textproto"
|
||||
@@ -28,8 +30,10 @@ import (
|
||||
// (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.
|
||||
// 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 cannot
|
||||
// disagree. The send-log row of a refusal is written by another goroutine and
|
||||
// of sender bindings Postfix itself reads for the envelope, so the two go by
|
||||
// one table. Where the lookups differ the check is the stricter: Postfix
|
||||
// also tries an address without its "+extension" and compares logins without
|
||||
// regard to case. The send-log row of a refusal is written by another goroutine and
|
||||
// is not waited for.
|
||||
|
||||
// tableFile is a table of the mail path that the panel rewrites and the check
|
||||
@@ -183,8 +187,9 @@ type senderSession struct {
|
||||
froms []string // the value of every From: field of the message
|
||||
}
|
||||
|
||||
// MailFrom starts a message. Like the journal milter it reads everything of a
|
||||
// message again here: the library replaces the session after a refusal.
|
||||
// MailFrom starts a message and forgets the one before it: a connection may
|
||||
// carry several, and after a temporary failure the library hands the
|
||||
// connection a new session.
|
||||
func (s *senderSession) MailFrom(from string, m *milter.Modifier) (milter.Response, error) {
|
||||
s.from = cleanAddress(from)
|
||||
s.login = macro(m, "auth_authen")
|
||||
@@ -249,9 +254,11 @@ 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
|
||||
}
|
||||
raw := s.froms[0]
|
||||
// Postfix hands a milter a header as it came, folded lines and all; the
|
||||
// address parser reads one line.
|
||||
raw := unfold(s.froms[0])
|
||||
shown = printable(raw)
|
||||
list, perr := mail.ParseAddressList(raw)
|
||||
list, perr := fromParser.ParseList(raw)
|
||||
if perr != nil || len(list) != 1 || list[0].Address == "" {
|
||||
return shown, "it must be one address", nil
|
||||
}
|
||||
@@ -269,8 +276,12 @@ func (s *senderSession) judge() (shown, why string, err error) {
|
||||
// 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 mentionedDomains(raw) {
|
||||
for _, d := range append(mentionedDomains(raw), mentionedDomains(list[0].Name)...) {
|
||||
if d == own {
|
||||
continue
|
||||
}
|
||||
@@ -282,35 +293,76 @@ func (s *senderSession) judge() (shown, why string, err error) {
|
||||
return printable(raw), "it names another sender of this server, " + d, 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
|
||||
}
|
||||
|
||||
// record leaves a row per recipient for the Outbound log. It never waits: a
|
||||
// database that is slow must not make the check slow.
|
||||
// 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
|
||||
// a refused message with a thousand recipients would otherwise be a thousand
|
||||
// rows at the price of one.
|
||||
func (s *senderSession) record(shown string) {
|
||||
if s.refused == nil {
|
||||
return
|
||||
}
|
||||
rcpts := s.rcpts
|
||||
if len(rcpts) == 0 {
|
||||
rcpts = []string{""}
|
||||
to := ""
|
||||
if len(s.rcpts) > 0 {
|
||||
to = s.rcpts[0]
|
||||
}
|
||||
for _, to := range rcpts {
|
||||
e := store.SendLogEntry{
|
||||
Domain: domainOf(s.from),
|
||||
AppLogin: s.login,
|
||||
From: s.from,
|
||||
To: to,
|
||||
Subject: s.subject,
|
||||
Reason: store.ReasonFrom,
|
||||
HeaderFrom: shown,
|
||||
}
|
||||
select {
|
||||
case s.refused <- e:
|
||||
default:
|
||||
log.Printf("sender-check: the refusal of %s -> %s is not in the log: too many are waiting to be written", s.from, to)
|
||||
if more := len(s.rcpts) - 1; more > 0 {
|
||||
to += fmt.Sprintf(" and %d more", more)
|
||||
}
|
||||
e := store.SendLogEntry{
|
||||
Domain: domainOf(s.from),
|
||||
AppLogin: s.login,
|
||||
From: s.from,
|
||||
To: to,
|
||||
Subject: s.subject,
|
||||
Reason: store.ReasonFrom,
|
||||
HeaderFrom: shown,
|
||||
}
|
||||
select {
|
||||
case s.refused <- e:
|
||||
default:
|
||||
log.Printf("sender-check: the refusal of %s -> %s is not in the log: too many are waiting to be written", s.from, to)
|
||||
}
|
||||
}
|
||||
|
||||
// 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()
|
||||
}
|
||||
|
||||
// mentionedDomains returns, lower-cased, what follows every "@" of v as far
|
||||
|
||||
@@ -97,6 +97,11 @@ func TestSenderCheck(t *testing.T) {
|
||||
{"header name in another case", "shop", [][2]string{{"FROM", "a@shop.example.org"}}, true, ""},
|
||||
{"its own name inside the display name", "shop", [][2]string{from(`"shop@shop.example.org" <a@shop.example.org>`)}, true, ""},
|
||||
{"an encoded display name", "shop", [][2]string{from("=?utf-8?b?0JzQsNCz0LDQt9C40L0=?= <a@shop.example.org>")}, true, ""},
|
||||
{"a folded line", "shop", [][2]string{from("\"A very long display name of the shop\"\r\n <a@shop.example.org>")}, true, ""},
|
||||
{"an encoded name, folded", "shop", [][2]string{from("=?utf-8?b?0JzQsNCz0LDQt9C40L0=?=\n\t<a@shop.example.org>")}, true, ""},
|
||||
{"a name in windows-1251", "shop", [][2]string{from("=?windows-1251?B?zODj4Ofo7Q==?= <a@shop.example.org>")}, true, ""},
|
||||
{"a name in koi8-r, quoted-printable", "shop", [][2]string{from("=?koi8-r?Q?=ED=C1=C7=C1=DA=C9=CE?= <a@shop.example.org>")}, true, ""},
|
||||
{"a comment after the address", "shop", [][2]string{from("a@shop.example.org (the shop)")}, 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"},
|
||||
@@ -112,6 +117,10 @@ func TestSenderCheck(t *testing.T) {
|
||||
{"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" <a@shop.example.org>`)}, false, "names another sender of this server"},
|
||||
{"a stranger's address in the display name", "shop", [][2]string{from(`"someone@gmail.example" <a@shop.example.org>`)}, true, ""},
|
||||
{"another sender in an encoded display name", "shop", [][2]string{from("=?utf-8?q?director=40example=2Eorg?= <a@shop.example.org>")}, 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 <director@example.org>")}, false, "not an address this application sends as"},
|
||||
{"a quote left open before another address", "shop", [][2]string{from(`"a@shop.example.org <director@example.org>`)}, false, "one address"},
|
||||
{"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"},
|
||||
}
|
||||
@@ -224,3 +233,24 @@ func TestSenderCheckResetsBetweenMessages(t *testing.T) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// A refused message is one row of the log however many recipients it had.
|
||||
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.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))
|
||||
}
|
||||
_, _ = s.Header("From", "director@example.org", mods(nil))
|
||||
_, _ = s.Headers(nil, mods(nil))
|
||||
close(refused)
|
||||
var rows []store.SendLogEntry
|
||||
for e := range refused {
|
||||
rows = append(rows, e)
|
||||
}
|
||||
if len(rows) != 1 || rows[0].To != "a@x.example and 2 more" {
|
||||
t.Fatalf("rows = %+v", rows)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -149,6 +149,13 @@ func (h *Handlers) healthView(r *http.Request, flash string) *view.Health {
|
||||
logf("panel: health: sender check statistics: %v", err)
|
||||
}
|
||||
rows, since, status := senderCheckCard(stats, time.Now())
|
||||
if err != nil {
|
||||
// Zeros that were not read are not zeros.
|
||||
for i := range rows {
|
||||
rows[i].Down, rows[i].Deferred = "—", "—"
|
||||
}
|
||||
status = string(health.StatusUnknown)
|
||||
}
|
||||
return page.WithSenderCheck(rows, since, status)
|
||||
}
|
||||
|
||||
|
||||
@@ -93,7 +93,9 @@
|
||||
`column is-6` stacks of numbered boxes that are read in rows, the odd ones
|
||||
on the left and the even ones on the right, each stack closed up so that a
|
||||
short box leaves no gap beside a tall one. On a phone the stacks dissolve
|
||||
and the boxes come in the order of their numbers (panel.css). */}}
|
||||
and the boxes come in the order of their numbers (panel.css) - which
|
||||
knows four boxes to a stack and takes every child of a stack for a box:
|
||||
a fifth, or a flash put between them, needs the rules extended first. */}}
|
||||
{{define "cards_open"}}<div class="columns sp-cards">{{end}}
|
||||
{{define "cards_close"}}</div>{{end}}
|
||||
|
||||
|
||||
@@ -40,6 +40,9 @@ func testSenderCheck(t *testing.T, sc *scenario) {
|
||||
{"own address", wildcard, "alerts@" + senderDomain, true},
|
||||
{"another address of the domain, with a name", wildcard, `"The Shop" <orders@` + senderDomain + `>`, true},
|
||||
{"the listed address", list, "billing@" + senderDomain, true},
|
||||
// What a mail library does with a long display name: the line is
|
||||
// folded, and Postfix hands the milter both of its lines.
|
||||
{"a long name, folded onto a second line", wildcard, "\"The Shop, its orders and its returns desk\"\r\n <orders@" + senderDomain + ">", true},
|
||||
|
||||
{"another registered domain", wildcard, "director@" + otherDomain, false},
|
||||
{"the server's own name", wildcard, "postmaster@" + selfpostHostname, false},
|
||||
|
||||
Reference in New Issue
Block a user