From c2f63d278929a9f09d094837268db40a98a55d6a Mon Sep 17 00:00:00 2001 From: Mix Date: Fri, 16 Oct 2026 01:44:59 +0300 Subject: [PATCH] panel-dkim-signing: the queue by name, and findings of the reviews The first rollout of faa3c11 to the test server crash-looped: without opendkim the image's package accounts got other numbers (postfix 101 -> 100, postdrop 103 -> 102), the queue under /data kept the old ones, and postsuper, which "postfix check" runs as the mail owner (src/postsuper/postsuper.c:1442), could not open it. Rolled back within five minutes. The entrypoint now hands an existing queue to Postfix's users by name before postfix-config.sh, and again after it (own_queue). Technical review (F1-F10): - F1 high: a backup of an upgraded installation carried /data/opendkim, a restore left it root's, and the panel refused every start. The tree is left out of archives, and the entrypoint makes it the panel's alone (0700/0600; also the security review's F3 - nothing of 2.8 needs the group any more; a 2.7.x start re-normalises it). - F2: List-Unsubscribe-Post is signed (RFC 8058); Sender too (security F1). - F3: SetServerName keeps the name in lower case - a key written under "Mail.Example.com" was never found. Only the DKIM side: the SASL realm is compared as written. - F4: the signer rewrites the selector table at every start (WriteSelectors, replacing EnsureServerSigned), and tries again every 10 s while a rewrite fails. - F5: e2e signature_over_awkward_headers (folded, tab-led, no space, two To:, 8-bit body, List-Unsubscribe-Post) checked by dkimpy; TestStartsOnA27Data starts on a 2.7 /data (old key tree, a queue of foreign numbers) and checks the copied key signs, the old tree's modes and the queue's owners; unit tests for repeated headers and the header limit. - F6: the table's writers are serialised (tableMu). - F7: the table is read for every message; keys the table no longer names are dropped from memory. - F8: the signature's value brings its own space (SMFIP_HDR_LEADSPC gets none from Postfix, cleanup_milter.c:1085). - F9: a body line over 998 bytes is logged with its domain: Postfix's SMTP client breaks it after signing (smtp_proto.c:1266-1274). - F10: the keys are copied while /data/dkim holds none (not only when it is absent), after the old tree is made the panel's (an install from 2.6 had another uid); the plan's rollback note says what a rollback keeps. Security review: F2 a header section over 1 MiB is refused (552 5.3.4) and the kept headers freed once the block is built - fifty connections of header-only messages could take the panel down; F5 the shown headers a message has (Subject, Date, To, Cc, Reply-To, Sender) are over-signed, only when present, since cleanup adds a missing Date: after the milters. F1 (holding Sender: to the login like From:) changes what is refused and waits for the owner. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 22 ++++- build/entrypoint.sh | 88 +++++++++++------ cmd/panel/signer.go | 40 ++++++-- docs/plans/panel-dkim-signing.md | 11 ++- internal/backup/backup.go | 7 +- internal/backup/backup_test.go | 5 + internal/domain/keys.go | 21 ---- internal/domain/keys_test.go | 7 +- internal/domain/servermail_test.go | 72 +++++++++----- internal/domain/service.go | 52 +++++----- internal/milter/sign.go | 150 ++++++++++++++++++++++------- internal/milter/sign_test.go | 81 ++++++++++++++-- test/e2e/dkim_shapes_test.go | 90 +++++++++++++++++ test/e2e/main_test.go | 3 + test/e2e/smtp_client.go | 6 ++ test/e2e/upgrade_test.go | 121 +++++++++++++++++++++++ 16 files changed, 617 insertions(+), 159 deletions(-) create mode 100644 test/e2e/dkim_shapes_test.go create mode 100644 test/e2e/upgrade_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ecee1d3..71af0b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,12 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version once more, as OpenDKIM signed; strict. Before OpenDKIM left, both signed every message on the e2e stand and agreed on `h=` and `bh=`; dkimpy - an independent implementation - verifies the panel's signature on every run. + Beyond what OpenDKIM signed: `Sender` and `List-Unsubscribe-Post` (RFC 8058 + wants it signed for one-click unsubscribe), and the shown headers a message + has - Subject, Date, To, Cc, Reply-To, Sender - named once more, so that + one added on the way breaks the signature. A header section over 1 MiB is + refused (`552 5.3.4`); a body line over 998 bytes, which Postfix breaks on + delivery, is logged with its domain. ### Removed @@ -26,10 +32,12 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version ### Changed - **The DKIM keys moved to `/data/dkim`** and are the panel's alone (0600): - the first start copies them from `/data/opendkim`, which is left as it was - for a rollback to 2.7.x. Keys, selectors and DNS records do not change. - The KeyTable and SigningTable are gone; a selector table the signer reads - takes their place. `OPENDKIM_DIR` and `OPENDKIM_SOCKET` are gone. + the first start copies them from `/data/opendkim`, which is kept, the + panel's alone as well, for a rollback to 2.7.x, and left out of backups. + Keys, selectors and DNS records do not change. The KeyTable and + SigningTable are gone; a selector table the signer reads takes their + place, rewritten from the database at every start. `OPENDKIM_DIR` and + `OPENDKIM_SOCKET` are gone. - **The sender check reads `From:` as a mail program does** and no longer as OpenDKIM did: the line must be one address at a domain the application may send as, and the 256-byte limit of OpenDKIM's buffer goes. The key is @@ -41,6 +49,12 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version it as quoted-printable for a server that does not announce 8BITMIME, after it was signed; `disable_mime_output_conversion=yes` now delivers it as signed. OpenDKIM's signatures were exposed to this too. +- **A start on a queue of an earlier image could stop at `postfix check`.** + The numbers the image's package accounts get depend on what else it + installs, and the queue under `/data` kept the old ones; `postsuper`, which + the check runs as the mail owner, could not read it. The entrypoint hands + the queue to Postfix's users by name before the check now. The first + rollout of this version to the test server met it and was rolled back. ## [2.7.0] - 2026-10-15 diff --git a/build/entrypoint.sh b/build/entrypoint.sh index a839499..47c6365 100644 --- a/build/entrypoint.sh +++ b/build/entrypoint.sh @@ -108,27 +108,42 @@ find /data -mindepth 1 -maxdepth 1 ! -name log ! -name postfix ! -name opendkim # DKIM keys and the selector table, /data/dkim: the panel's alone - it makes # the keys, writes the table and signs (internal/domain, Keys; internal/milter, # sign.go). Until 2.8.0 the keys were under /data/opendkim, shared with -# OpenDKIM through group selfpost. The first start without /data/dkim copies -# them over and leaves the old tree as it was, for an image of 2.7.x rolled -# back to (docs/plans/panel-dkim-signing.md, Q2). The copy is made as the -# panel user, which can read nothing there the panel could not, into a -# directory beside, renamed into place when it is whole: a start that stops -# midway copies again. A copy that fails stops the start (set -e) - the +# OpenDKIM through group selfpost. That tree is kept for an image of 2.7.x +# rolled back to (docs/plans/panel-dkim-signing.md, Q2), but nothing of +# 2.8.0 reads it beyond the copy below: it is the panel's alone like the new +# one - a 2.7.x start gives it back to group selfpost itself - and a backup +# leaves it out (internal/backup, excludedFromArchive). While /data/dkim +# holds no key and the old tree does, the keys are copied over. The copy is +# made as the panel user, which can read nothing there the panel could not, +# into a directory beside, renamed into place when it is whole: a start that +# stops midway copies again. A copy that fails stops the start (set -e) - the # panel would otherwise make the server's own name a new key, and its # published record would no longer match. The selector table is written by -# the panel before its signer serves (cmd/panel/signer.go). +# the panel before its signer serves (cmd/panel/signer.go). After a rollback +# to 2.7.x, a domain added there has its key only in the old tree: remove +# /data/dkim before the next start of 2.8.x to copy again. # (util-linux 2.41, setpriv(1): --reuid, --regid, --clear-groups; cp(1) -# -P: never follow a symbolic link in the source.) +# -P: never follow a symbolic link in the source; chown -R and find never +# follow one either. GNU chmod keeps a directory's set-group-ID bit under a +# numeric mode unless it is cleared by name - "g-s" - and 2.7.x set it on +# the old tree: e2e TestStartsOnA27Data.) +has_keys() { [ -d "$1" ] && [ -n "$(find "$1" -type f -name '*.private' | head -n 1)" ]; } +if [ -d /data/opendkim ]; then + chown -R panel:panel /data/opendkim + find /data/opendkim -type d -exec chmod 0700 {} + -exec chmod g-s {} + + find /data/opendkim -type f -exec chmod 0600 {} + +fi rm -rf /data/dkim.new -if [ ! -e /data/dkim ] && [ -d /data/opendkim/keys ]; then +if ! has_keys /data/dkim/keys && has_keys /data/opendkim/keys; then setpriv --reuid=panel --regid=panel --clear-groups -- \ sh -c 'mkdir /data/dkim.new && cp -RP /data/opendkim/keys /data/dkim.new/keys' + rm -rf /data/dkim mv /data/dkim.new /data/dkim echo "entrypoint: the DKIM keys were copied from /data/opendkim to /data/dkim; /data/opendkim is left for a rollback" >&2 fi mkdir -p /data/dkim/keys chown -R panel:panel /data/dkim -find /data/dkim -type d -exec chmod 0700 {} + +find /data/dkim -type d -exec chmod 0700 {} + -exec chmod g-s {} + find /data/dkim -type f -exec chmod 0600 {} + # Application SASL accounts. The panel (user `panel`) writes the @@ -219,34 +234,53 @@ chmod 2750 /run/selfpost # (test(1), -S: true for any socket file, whether or not anything listens.) rm -f /run/selfpost/*.sock /run/selfpost/*.alive +# The queue tree belongs to Postfix's users by name. Their numbers are what +# the package's install hands out and depend on what else the image installs: +# without opendkim (2.8.0) postfix went from uid 101 to 100 and postdrop from +# gid 103 to 102, and a queue of an earlier image kept the old numbers. So an +# existing tree is handed over before postfix-config.sh runs "postfix check", +# whose postsuper drops to the mail owner before it reads the queue and +# otherwise stops the start ("scan_dir_push: open directory defer: Permission +# denied", the test server, 2026-10-16; Postfix 3.10.13, +# src/postsuper/postsuper.c:1442, set_ugid(var_owner_uid, var_owner_gid); +# conf/postfix-script:342). After the check it is done again for a tree the +# check has just built. +own_queue() { + [ -d /data/postfix/queue/active ] || return 0 + chown -R postfix:postfix /data/postfix/queue + # The recursive chown would leave public/ and maildrop/ in group postfix. + # Postfix keeps those two in its setgid group: postqueue and postdrop are + # setgid postdrop and reach the showq socket and the maildrop directory + # through that group alone. With the wrong group `postqueue -p` run by the + # panel answers "Connect to the Postfix showq service: Permission denied" + # and the panel cannot read the queue (the Queue card and the Outbound + # queue page). + chgrp postdrop /data/postfix/queue/public /data/postfix/queue/maildrop + # It would also leave the queue directory itself and pid/ owned by + # postfix. Postfix keeps those two owned by root (master, which runs as + # root, is the only writer) and logs "warning: not owned by root" for each + # at every start. (Postfix 3.10.13, conf/postfix-files:51-68: + # $queue_directory root 755, maildrop $setgid_group 730, public + # $setgid_group 710, pid root 755, the other queue directories + # $mail_owner 700; conf/postfix-script:354-356, "check-warn": "not owned + # by root".) + chown root /data/postfix/queue /data/postfix/queue/pid +} +own_queue + # 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 # optional 587 service are all driven by env at run time, and re-derived on every # start the same way the /data normalisation above is. /usr/local/bin/postfix-config.sh -# The queue tree, on a first start or after a restore, is already built here: +# The queue tree, on a first start or after a restore, is built here: # postfix-config.sh ends with "postfix check", whose check-fatal runs # "post-install create-missing" (Postfix 3.10.13, conf/postfix-script:314; # conf/postfix-files:55, "$queue_directory/active:d:$mail_owner:-:700:ucr"). # A "postfix set-permissions" for a missing active/ stood here and never ran # (assumptions C-P34). -chown -R postfix:postfix /data/postfix/queue -# The recursive chown above would leave public/ and maildrop/ in group postfix. -# Postfix keeps those two in its setgid group: postqueue and postdrop are setgid -# postdrop and reach the showq socket and the maildrop directory through that -# group alone. With the wrong group `postqueue -p` run by the panel answers -# "Connect to the Postfix showq service: Permission denied" and the panel cannot -# read the queue (the Queue card and the Outbound queue page). -chgrp postdrop /data/postfix/queue/public /data/postfix/queue/maildrop -# It would also leave the queue directory itself and pid/ owned by postfix. -# Postfix keeps those two owned by root (master, which runs as root, is the only -# writer) and logs "warning: not owned by root" for each at every start. -# (Postfix 3.10.13, conf/postfix-files:51-68: $queue_directory root 755, maildrop -# $setgid_group 730, public $setgid_group 710, pid root 755, the other queue -# directories $mail_owner 700; conf/postfix-script:354-356, "check-warn": "not -# owned by root".) -chown root /data/postfix/queue /data/postfix/queue/pid +own_queue # [program:postfix]'s startsecs (build/supervisord.conf): longer than the # wrapper's wait for the milter sockets, so its exit at the end of the wait is diff --git a/cmd/panel/signer.go b/cmd/panel/signer.go index c9de628..f55acc5 100644 --- a/cmd/panel/signer.go +++ b/cmd/panel/signer.go @@ -5,12 +5,17 @@ import ( "log" "path/filepath" "strings" + "time" "github.com/mixeme/selfpost/internal/milter" "github.com/mixeme/selfpost/internal/postfix" "github.com/mixeme/selfpost/internal/store" ) +// rewriteRetry is how often the signer tries again to write the selector +// table when the write at its start failed. +const rewriteRetry = 10 * time.Second + // serveSigner runs the DKIM signer (internal/milter/sign.go) on its two // sockets: signSocket for what applications send on 465/587, signed with the // key of the domain the login belongs to - the sender_login_maps Postfix and @@ -20,16 +25,33 @@ import ( // build/postfix-config.sh), so while this role is not serving, mail waits. // // The keys and selectors come from the key tree and the selector table under -// dkimDir (internal/domain, Keys). Before it serves, the table is made to -// hold the server's own name - and written at all on the first start of -// 2.8.0, whose table replaced OpenDKIM's: Postfix waits for these sockets -// (build/postfix-wrapper.sh), so no message meets the table before it. +// dkimDir (internal/domain, Keys). Before it serves, the table is rewritten +// from the registry (WriteSelectors): Postfix waits for these sockets +// (build/postfix-wrapper.sh), so no message meets a table from before. A +// rewrite that fails is tried again every rewriteRetry until it succeeds; +// meanwhile what the signer finds no key for it defers, and the panel's +// pages - Reload configuration among them - stay up. func serveSigner(ctx context.Context, cfg config, st *store.Store) error { - if rebuilt, err := newMailStack(cfg, st).Domains.EnsureServerSigned(); err != nil { - // Mail waits: what the signer cannot find a key for it defers. - log.Printf("dkim-signer: the selector table: %v", err) - } else if rebuilt { - log.Printf("dkim-signer: the selector table was rewritten from the registry") + domains := newMailStack(cfg, st).Domains + if err := domains.WriteSelectors(); err != nil { + log.Printf("dkim-signer: the selector table: %v - trying again every %s", err, rewriteRetry) + go func() { + tick := time.NewTicker(rewriteRetry) + defer tick.Stop() + for { + select { + case <-ctx.Done(): + return + case <-tick.C: + if err := domains.WriteSelectors(); err != nil { + log.Printf("dkim-signer: the selector table: %v", err) + continue + } + log.Printf("dkim-signer: the selector table is written") + return + } + } + }() } keys := milter.NewSignKeys(filepath.Join(cfg.dkimDir, "selectors"), filepath.Join(cfg.dkimDir, "keys")) bind := milter.NewBindings(postfix.New(cfg.postfixDir).SenderLoginMapsPath()) diff --git a/docs/plans/panel-dkim-signing.md b/docs/plans/panel-dkim-signing.md index aea683c..1d4c882 100644 --- a/docs/plans/panel-dkim-signing.md +++ b/docs/plans/panel-dkim-signing.md @@ -218,6 +218,11 @@ libmilter1.0.1. - **go-msgauth is small and released slowly.** It can be vendored if it ever matters; the signing path is about 300 lines of it. - **Rollback:** until stage 2 ships, the chain change is one line; after - the cut, the previous image (2.7.x) still has OpenDKIM and reads the same - keys - under Q2's rename, from the old path, which the move leaves as a - copy until the next release. + the cut, the previous image (2.7.x) still has OpenDKIM and reads the keys + from the old path, which the move leaves as a copy until the next release + - the keys as they were at the upgrade: a domain added under 2.8.x is not + in that tree, and 2.7.x sends its mail unsigned; one re-added under 2.8.x + has another key there. Going forward again after a rollback, a domain + added under 2.7.x has its key only in the old tree: remove `/data/dkim` + before the start, and the entrypoint copies again (it copies while + `/data/dkim` holds no key). diff --git a/internal/backup/backup.go b/internal/backup/backup.go index 31837e9..b06ceb1 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -105,7 +105,11 @@ const QueueDir = "postfix/queue" // fresh one is written instead); a "tls" directory under /data is skipped when // an operator pointed TLS_CERT_FILE inside /data; and "log" is Postfix's raw // delivery log plus its fourteen rotated files, which is diagnostic output, not -// state to restore, and by far the largest thing under /data. +// state to restore, and by far the largest thing under /data. "opendkim" is the +// key tree OpenDKIM read until 2.8.0, kept on disk only for a rollback to 2.7.x: +// an archive restores into its own version alone, which reads the keys from +// /data/dkim, and a second copy of every key - one that outlives a deleted +// domain's - has no place in it. var excludedFromArchive = map[string]bool{ "selfpost.db": true, "selfpost.db-wal": true, @@ -114,6 +118,7 @@ var excludedFromArchive = map[string]bool{ "setup-token": true, "tls": true, "log": true, + "opendkim": true, ManifestName: true, ContentsName: true, contentsCheckedName: true, diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index cb73a41..65dff05 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -44,6 +44,9 @@ func seedDataDir(t *testing.T) (dataDir, dbPath, deployRoot string) { } writeFile(t, filepath.Join(dataDir, "dkim", "keys", "example.com", "selfpost.private"), "PRIVATE KEY") + // The key tree OpenDKIM read until 2.8.0, left on disk for a rollback. + writeFile(t, filepath.Join(dataDir, "opendkim", "keys", "example.com", "selfpost.private"), "OLD KEY") + writeFile(t, filepath.Join(dataDir, "opendkim", "KeyTable"), "example.com example.com:selfpost:/data/opendkim/keys/example.com/selfpost.private") writeFile(t, filepath.Join(dataDir, "sasl", "sasldb2"), "SASLDB") writeFile(t, filepath.Join(dataDir, "postfix", "sender_login_maps"), "@example.com login") writeFile(t, filepath.Join(dataDir, "antispam", "deny_domain.map"), "evil.example\n") @@ -141,6 +144,8 @@ func TestCreateIncludesStateExcludesTransient(t *testing.T) { DataArchivePrefix + "log/mail.log", DataArchivePrefix + "log/mail.log.1", DataArchivePrefix + "postfix/queue/deferred/sample", + DataArchivePrefix + "opendkim/keys/example.com/selfpost.private", + DataArchivePrefix + "opendkim/KeyTable", } { if _, ok := files[name]; ok { t.Errorf("archive should not contain %s", name) diff --git a/internal/domain/keys.go b/internal/domain/keys.go index fa62fb5..4bec5f9 100644 --- a/internal/domain/keys.go +++ b/internal/domain/keys.go @@ -167,27 +167,6 @@ func (o *Keys) Rebuild(domains []SigningDomain) error { return writeFileAtomic(o.selectorsPath, table, 0o600) } -// signs reports whether the selector table signs name with the key of -// selector. A table that does not exist yet signs nothing. The selector is -// part of the question because the panel shows and checks the record of one -// key; a table that names another would have the signer sign with a key whose -// record nobody is told to publish. -func (o *Keys) signs(name, selector string) (bool, error) { - data, err := os.ReadFile(o.selectorsPath) - if os.IsNotExist(err) { - return false, nil - } - if err != nil { - return false, fmt.Errorf("read %s: %w", filepath.Base(o.selectorsPath), err) - } - for _, line := range strings.Split(string(data), "\n") { - if line == name+" "+selector { - return true, nil - } - } - return false, nil -} - // renderSelectors builds the selector table for a domain set, a line // " " each, sorted by name so the output is deterministic. // Every domain is re-checked for config-injection safety before being written diff --git a/internal/domain/keys_test.go b/internal/domain/keys_test.go index a13c74d..ad6a398 100644 --- a/internal/domain/keys_test.go +++ b/internal/domain/keys_test.go @@ -91,11 +91,8 @@ func TestRebuildWritesThePanelsOwnFiles(t *testing.T) { if err := o.Rebuild([]SigningDomain{{Name: "example.com", Selector: "selfpost"}}); err != nil { t.Fatalf("Rebuild: %v", err) } - if signed, err := o.signs("example.com", "selfpost"); err != nil || !signed { - t.Errorf("signs after Rebuild = %v, %v", signed, err) - } - if signed, err := o.signs("example.com", "other"); err != nil || signed { - t.Errorf("signs with another selector = %v, %v", signed, err) + if got, err := os.ReadFile(o.selectorsPath); err != nil || string(got) != "example.com selfpost"+"\n" { + t.Errorf("selector table after Rebuild = %q, %v", got, err) } if runtime.GOOS == "windows" { return // no Unix modes diff --git a/internal/domain/servermail_test.go b/internal/domain/servermail_test.go index 7a300f4..0a6c540 100644 --- a/internal/domain/servermail_test.go +++ b/internal/domain/servermail_test.go @@ -82,10 +82,10 @@ func TestServerNameThatIsAlsoADomain(t *testing.T) { } } -// The first start of 2.8.0 finds no selector table - OpenDKIM's tables were -// the earlier versions' - and an installation may lack the server's line. -// The start rebuilds, once. -func TestEnsureServerSignedRebuildsOnce(t *testing.T) { +// The signer rewrites the table at every start: the first start of 2.8.0 +// finds none (OpenDKIM's tables were the earlier versions'), and a rewrite +// cut short leaves a domain without its line while the server's is there. +func TestWriteSelectorsAtTheStart(t *testing.T) { svc, keys := newTestService(t, &fakeApps{}) if _, err := svc.Add("example.com"); err != nil { t.Fatal(err) @@ -95,43 +95,51 @@ func TestEnsureServerSignedRebuildsOnce(t *testing.T) { } svc.SetServerName("mail.example.org") - if rebuilt, err := svc.EnsureServerSigned(); err != nil || !rebuilt { - t.Fatalf("first start: rebuilt = %v, %v; want a rebuild", rebuilt, err) + if err := svc.WriteSelectors(); err != nil { + t.Fatalf("first start: %v", err) } - if got, want := readSelectors(t, keys), "example.com selfpost\nmail.example.org selfpost\n"; got != want { - t.Errorf("selector table after the rebuild = %q, want %q", got, want) + want := "example.com selfpost" + "\n" + "mail.example.org selfpost" + "\n" + if got := readSelectors(t, keys); got != want { + t.Errorf("selector table after the first start = %q, want %q", got, want) } - if rebuilt, err := svc.EnsureServerSigned(); err != nil || rebuilt { - t.Errorf("second start: rebuilt = %v, %v; want nothing to do", rebuilt, err) + + // A domain's line lost (a rewrite cut short): the next start puts it back. + if err := os.WriteFile(keys.selectorsPath, []byte("mail.example.org selfpost"+"\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := svc.WriteSelectors(); err != nil { + t.Fatal(err) + } + if got := readSelectors(t, keys); got != want { + t.Errorf("selector table after the next start = %q, want %q", got, want) } // A name that only begins like the server's is not it. other, otherKeys := newTestService(t, &fakeApps{}) other.SetServerName("mail.example.org.evil.example") - if _, err := other.EnsureServerSigned(); err != nil { + if err := other.WriteSelectors(); err != nil { t.Fatal(err) } - if signed, err := otherKeys.signs("mail.example.org", "selfpost"); err != nil || signed { - t.Errorf("a longer name was taken for the server's: %v, %v", signed, err) + if got := readSelectors(t, otherKeys); got != "mail.example.org.evil.example selfpost"+"\n" { + t.Errorf("selector table = %q", got) } } -// A table that signs the server's name with the key of another selector - the -// default selector was changed - is rebuilt: the signer must sign with the key -// whose record the panel shows. -func TestEnsureServerSignedFollowsTheSelector(t *testing.T) { +// A changed default selector: the server's name is signed with the key of the +// new one, whose record the panel shows. +func TestWriteSelectorsFollowsTheSelector(t *testing.T) { svc, keys := newTestService(t, &fakeApps{}) svc.SetServerName("mail.example.org") - if _, err := svc.EnsureServerSigned(); err != nil { + if err := svc.WriteSelectors(); err != nil { t.Fatal(err) } svc.selector = "sp2027" - if rebuilt, err := svc.EnsureServerSigned(); err != nil || !rebuilt { - t.Fatalf("after the selector changed: rebuilt = %v, %v; want a rebuild", rebuilt, err) + if err := svc.WriteSelectors(); err != nil { + t.Fatal(err) } - if got := readSelectors(t, keys); got != "mail.example.org sp2027\n" { - t.Errorf("selector table after the rebuild = %q", got) + if got := readSelectors(t, keys); got != "mail.example.org sp2027"+"\n" { + t.Errorf("selector table after the change = %q", got) } mail, err := svc.ServerMail() if err != nil || mail.Selector != "sp2027" { @@ -139,6 +147,26 @@ func TestEnsureServerSignedFollowsTheSelector(t *testing.T) { } } +// A hostname written with capitals is the server's name in lower case: the +// signer looks a key up by the name in lower case, and a key written under the +// capitals would never be found. +func TestServerNameInCapitals(t *testing.T) { + svc, keys := newTestService(t, &fakeApps{}) + svc.SetServerName("Mail.Example.ORG") + if err := svc.WriteSelectors(); err != nil { + t.Fatal(err) + } + if got := readSelectors(t, keys); got != "mail.example.org selfpost"+"\n" { + t.Errorf("selector table = %q", got) + } + if _, err := os.Stat(KeyPath(keys.keysDir, "mail.example.org", "selfpost")); err != nil { + t.Errorf("the key is not under the name in lower case: %v", err) + } + if mail, err := svc.ServerMail(); err != nil || mail.Name != "mail.example.org" { + t.Errorf("server mail = %+v, %v", mail, err) + } +} + // Without a hostname nothing is added and nothing is signed for it. func TestNoServerName(t *testing.T) { svc, keys := newTestService(t, &fakeApps{}) diff --git a/internal/domain/service.go b/internal/domain/service.go index 6dd9fd8..784aca1 100644 --- a/internal/domain/service.go +++ b/internal/domain/service.go @@ -3,6 +3,8 @@ package domain import ( "errors" "fmt" + "strings" + "sync" "github.com/mixeme/selfpost/internal/store" ) @@ -50,9 +52,13 @@ type Service struct { // them the name is signed like a sending domain: it gets a DKIM key and a line // in the selector table, without being a row of the registry - no application // can send as it. The name comes from SELFPOST_HOSTNAME, which the entrypoint -// has validated; it is not user input. +// has validated; it is not user input. It is kept in lower case, as every +// domain is (validate.NormalizeDomain): the signer finds a key by the name in +// lower case (internal/milter, SignKeys), and a key written under +// "Mail.Example.com" would never be found. Only the DKIM side lowers it - the +// SASL realm, which defaults to the hostname, is compared as written. func (s *Service) SetServerName(hostname string) { - s.serverName = hostname + s.serverName = strings.ToLower(hostname) } // ServerMail is how the server's own name is signed. @@ -84,29 +90,16 @@ func (s *Service) ServerMail() (ServerMail, error) { return ServerMail{Name: s.serverName, Selector: selector, Record: rec}, nil } -// EnsureServerSigned makes sure the selector table has the line for the -// server's own name, rebuilding it if it does not - or if there is no table -// yet, as on the first start of 2.8.0, whose table replaced OpenDKIM's. The -// table is otherwise rebuilt only when a domain is added or deleted, so an -// installation whose DKIM_SELECTOR_DEFAULT was changed would go on signing -// with the key of the old selector while the panel shows the record of the -// new. It reports whether it had to rebuild. -func (s *Service) EnsureServerSigned() (bool, error) { - if s.serverName == "" { - return false, nil - } - selector, err := s.serverSelector() - if err != nil { - return false, err - } - signed, err := s.keys.signs(s.serverName, selector) - if err != nil { - return false, err - } - if signed { - return false, nil - } - return true, s.resync() +// WriteSelectors rewrites the selector table from the registry. The signer +// calls it before it serves (cmd/panel/signer.go): the table is otherwise +// rewritten only when a domain is added, deleted or imported, by Reload +// configuration and after a restore, so a start would leave it out of step +// after a rewrite that was cut short (a panel killed between a domain's row +// and its line), after a changed DKIM_SELECTOR_DEFAULT (the server's name +// would be signed with the old selector's key while the panel shows the new +// record), and on the first start of 2.8.0, which has no table yet. +func (s *Service) WriteSelectors() error { + return s.resync() } // serverSelector is the selector the server's own name is signed with: that of @@ -297,8 +290,17 @@ func (s *Service) Resync() error { return nil } +// tableMu orders the selector table's writers. Each role builds a Service of +// its own (cmd/panel, newMailStack), and a rewrite is a read of the registry +// and then a write: without it, a writer that read before another's domain +// was added could write after it, and the table would lack that domain until +// the next rewrite. +var tableMu sync.Mutex + // resync rebuilds the selector table from the current domain set. func (s *Service) resync() error { + tableMu.Lock() + defer tableMu.Unlock() domains, err := s.store.ListDomains() if err != nil { return err diff --git a/internal/milter/sign.go b/internal/milter/sign.go index b3feea8..2fa0d2b 100644 --- a/internal/milter/sign.go +++ b/internal/milter/sign.go @@ -30,16 +30,46 @@ import ( // signHeaders is the set of header names signed when present: RFC 6376 // § 5.4.1's list, as OpenDKIM's dkim_should_signhdrs[] held it (OpenDKIM // 2.11.0~beta2, libopendkim/dkim.c:223-246), which SelfPost signed with until -// 2.8.0. Every occurrence is signed, in message order, and From once more +// 2.8.0; and two that list leaves out: Sender, which a mail program shows +// beside From ("on behalf of"), and List-Unsubscribe-Post, which RFC 8058 +// § 4 wants signed for a one-click unsubscribe to be honoured. Every +// occurrence is signed, in message order, and the shown ones once more // (signLines). var signHeaders = map[string]bool{ - "from": true, "reply-to": true, "subject": true, "date": true, "to": true, "cc": true, + "from": true, "sender": true, "reply-to": true, "subject": true, "date": true, "to": true, "cc": true, "resent-date": true, "resent-from": true, "resent-sender": true, "resent-to": true, "resent-cc": true, "in-reply-to": true, "references": true, - "list-id": true, "list-help": true, "list-unsubscribe": true, "list-subscribe": true, - "list-post": true, "list-owner": true, "list-archive": true, + "list-id": true, "list-help": true, "list-unsubscribe": true, "list-unsubscribe-post": true, + "list-subscribe": true, "list-post": true, "list-owner": true, "list-archive": true, } +// overSign are the signed headers a mail program shows: each that the message +// has is named once more in h=, so that one added after signing breaks the +// signature (RFC 6376 § 5.4.2, § 8.15). Only those present: Postfix's cleanup +// adds a Date: or Message-ID: a message lacks after the milters, out of their +// sight (src/cleanup/cleanup_message.c:722, cleanup_header_done_callback(); a +// header cleanup adds is hidden from milters, cleanup_milter.c:758, +// hidden_header), and an absent name in h= would refuse it. From is present +// in every message the signer signs (the sender check refuses one without; +// the panel writes its own) and comes last. +var overSign = map[string]bool{"subject": true, "date": true, "to": true, "cc": true, "reply-to": true, "sender": true} + +// headerLimit bounds the header section the signer keeps for a message. The +// signer holds it whole until the body starts (go-msgauth hashes headers at +// the end of the block), and Postfix bounds one header (60000 bytes with +// milters, src/cleanup/cleanup_message.c, KLUDGE_HEADER_LIMIT) but not their +// number: a message of nothing but headers up to message_size_limit, sent on +// fifty connections at once, would cost the panel gigabytes (security review +// of panel-dkim-signing). A mebibyte is far beyond any mail program's. +const headerLimit = 1 << 20 + +// smtpLineLimit is the longest line Postfix's SMTP client sends as it is: +// smtp_line_length_limit, 998 by default (Postfix 3.10.13, +// src/global/mail_params.h:1188), past which src/smtp/smtp_proto.c:1266-1274 +// breaks the line and starts the rest with a space - after the signer hashed +// it whole, so the body hash no longer matches. +const smtpLineLimit = 998 + // signLimit bounds how long a message may take from the end of its headers // to its end. go-milter v0.4.1 tells a session nothing when its connection // ends early (session.go, HandleMilterCommands(): it returns on EOF or QUIT @@ -53,15 +83,17 @@ const signLimit = time.Hour // SignKeys reads the names the server signs for and their selectors from the // selector table the panel writes (internal/domain, renderSelectors(): a line // " " each); the key of a name is -// keysDir//.private (domain.KeyPath). The table is read again -// when it changes, a key file when it does. +// keysDir//.private (domain.KeyPath). The table, a few +// lines, is read for every message: a stamp of modification time and size +// would miss a rewrite of the same size within one tick of the file system's +// clock (review of panel-dkim-signing, F7). A key is parsed again when its +// file changes, and forgotten when the table no longer names it. type SignKeys struct { table, keysDir string - mu sync.Mutex - stamp fileStamp - rows map[string]keyRow - keys map[string]loadedKey + mu sync.Mutex + rows map[string]keyRow + keys map[string]loadedKey } type keyRow struct{ selector, path string } @@ -71,10 +103,11 @@ type loadedKey struct { signer crypto.Signer } -// fileStamp tells a file's versions apart: the panel replaces a table or a -// key by writing a new file and renaming it over the old one -// (internal/atomicfile), so a new version is a new modification time or -// size. +// fileStamp tells a key file's versions apart: the panel replaces a key by +// writing a new file and renaming it over the old one (internal/atomicfile), +// so a new version is a new modification time or size - a key of the same +// size written within the same tick as the one before it is not told apart, +// which a person replacing a key by hand in the panel cannot do. type fileStamp struct { mod time.Time size int64 @@ -119,13 +152,6 @@ func (k *SignKeys) Lookup(domain string) (string, crypto.Signer, error) { } func (k *SignKeys) readTable() error { - fi, err := os.Stat(k.table) - if err != nil { - return fmt.Errorf("selector table: %w", err) - } - if k.rows != nil && k.stamp == stampOf(fi) { - return nil - } data, err := os.ReadFile(k.table) if err != nil { return fmt.Errorf("selector table: %w", err) @@ -144,7 +170,17 @@ func (k *SignKeys) readTable() error { domain := strings.ToLower(name) rows[domain] = keyRow{selector, filepath.Join(k.keysDir, domain, selector+".private")} } - k.rows, k.stamp = rows, stampOf(fi) + k.rows = rows + // A deleted domain's key does not stay in memory. + named := map[string]bool{} + for _, r := range rows { + named[r.path] = true + } + for path := range k.keys { + if !named[path] { + delete(k.keys, path) + } + } return nil } @@ -189,8 +225,12 @@ type signSession struct { login string headers []field + size int // the header section's bytes so far + tooBig bool // past headerLimit: refused at the end of the headers signer *dkim.Signer domain string + line int // bytes of the body line being read + long bool // a body line longer than smtpLineLimit was logged expired chan struct{} // closed by the watchdog at signLimit done chan struct{} // closed when the message ends, to stop it } @@ -202,7 +242,8 @@ func (s *signSession) reset() { close(s.done) _ = s.signer.Close() } - s.login, s.headers, s.signer, s.domain = "", nil, nil, "" + s.login, s.headers, s.size, s.tooBig, s.signer, s.domain = "", nil, 0, false, nil, "" + s.line, s.long = 0, false } // MailFrom starts a message. {auth_authen} is the SASL login (see @@ -219,7 +260,14 @@ func (s *signSession) MailFrom(from string, m *milter.Modifier) (milter.Response // ServeSign); a header of several lines as text separated by "\n"; and not // Postfix's own Received:, which goes above the signature anyway (an // inserted header is placed relative to the message as delivered). +// +// Past headerLimit nothing more is kept, and Headers refuses the message. func (s *signSession) Header(name, value string, m *milter.Modifier) (milter.Response, error) { + s.size += len(name) + len(value) + 3 + if s.size > headerLimit { + s.tooBig, s.headers = true, nil + return milter.RespContinue, nil + } s.headers = append(s.headers, field{name, value}) return milter.RespContinue, nil } @@ -231,6 +279,10 @@ func (s *signSession) Header(name, value string, m *milter.Modifier) (milter.Res // h is not read: go-milter v0.4.1 keeps every header of the connection in it // (see senderSession.Headers). func (s *signSession) Headers(h textproto.MIMEHeader, m *milter.Modifier) (milter.Response, error) { + if s.tooBig { + log.Printf("dkim-signer: login %q: a header section of more than %d bytes - refused", s.login, headerLimit) + return milter.NewResponseStr('y', fmt.Sprintf("552 5.3.4 The header section is larger than %d bytes", headerLimit)), nil + } s.domain = s.domainOf(s.login) if s.domain == "" { log.Printf("dkim-signer: no sending domain for login %q - deferring", s.login) @@ -242,6 +294,7 @@ func (s *signSession) Headers(h textproto.MIMEHeader, m *milter.Modifier) (milte return milter.RespTempFail, nil } keys, block := signLines(s.headers) + s.headers = nil // the block is what is kept now signer, err := dkim.NewSigner(&dkim.SignOptions{ Domain: s.domain, Selector: selector, @@ -280,33 +333,61 @@ func watch(signer *dkim.Signer, expired, done chan struct{}) { // Each header is written as Postfix will deliver it: the name, the colon, the // value with its own leading white space, the lines of a folded value joined // with CRLF; and the empty line that ends the block. The names: every header -// in signHeaders, in message order, and From once more, so that a From: -// added later breaks the signature (RFC 6376 § 5.4.2; OversignHeaders From in -// OpenDKIM's configuration until 2.8.0; go-msgauth v0.7.0, dkim/sign.go, -// NewSigner(): a name with no instance left is kept in h= and hashes -// nothing). +// in signHeaders, in message order; then each of overSign the message has, +// once more, in the order it first comes; then From once more (RFC 6376 +// § 5.4.2; OversignHeaders From in OpenDKIM's configuration until 2.8.0; +// go-msgauth v0.7.0, dkim/sign.go, NewSigner(): a name with no instance left +// is kept in h= and hashes nothing). func signLines(headers []field) ([]string, []byte) { - var keys []string + var keys, again []string + seen := map[string]bool{} var b strings.Builder for _, f := range headers { b.WriteString(f.name) b.WriteString(":") b.WriteString(strings.ReplaceAll(strings.ReplaceAll(f.value, "\r\n", "\n"), "\n", "\r\n")) b.WriteString("\r\n") - if signHeaders[strings.ToLower(strings.TrimSpace(f.name))] { - keys = append(keys, strings.TrimSpace(f.name)) + name := strings.TrimSpace(f.name) + lower := strings.ToLower(name) + if !signHeaders[lower] { + continue + } + keys = append(keys, name) + if overSign[lower] && !seen[lower] { + seen[lower] = true + again = append(again, name) } } b.WriteString("\r\n") + keys = append(keys, again...) return append(keys, "From"), []byte(b.String()) } // BodyChunk hashes the body as it comes. Postfix 3.10.13, // src/milter/milter8.c:2470-2500, milter8_body(): every line ends "\r\n". +// A line longer than smtpLineLimit is logged, once a message: its signature +// will not verify where the line is broken on the way out - the panel's log +// names the domain for whoever has to tell the application's author. func (s *signSession) BodyChunk(chunk []byte, m *milter.Modifier) (milter.Response, error) { if s.signer == nil { return milter.RespContinue, nil } + if !s.long { + for _, c := range chunk { + if c == '\n' { + s.line = 0 + continue + } + if c != '\r' { + s.line++ + } + if s.line > smtpLineLimit { + s.long = true + log.Printf("dkim-signer: %s (login %q): a body line longer than %d bytes - Postfix breaks it on delivery and the signature will not verify", s.domain, s.login, smtpLineLimit) + break + } + } + } if _, err := s.signer.Write(chunk); err != nil { log.Printf("dkim-signer: %s: %v - deferring", s.domain, err) s.reset() @@ -324,7 +405,10 @@ func (s *signSession) BodyChunk(chunk []byte, m *milter.Modifier) (milter.Respon // own Received:, which a milter never sees (milter8.c:2352-2358, "insert // requests are relative to the message as delivered"). go-milter v0.4.1, // modifier.go, InsertHeader() turns the folded value's CRLF into LF, as -// Postfix keeps a header. +// Postfix keeps a header. The value carries its own leading space: a milter +// that asked for SMFIP_HDR_LEADSPC gets none from Postfix +// (milter8.c:1378, :1464, MILTER8_HDR_SPACE; cleanup_milter.c:1085, +// "%s:%s%s"). func (s *signSession) Body(m *milter.Modifier) (milter.Response, error) { signer := s.signer if signer == nil { @@ -348,7 +432,7 @@ func (s *signSession) Body(m *milter.Modifier) (milter.Response, error) { log.Printf("dkim-signer: %s: malformed signature from the library - deferring", s.domain) return milter.RespTempFail, nil } - if err := m.InsertHeader(0, name, strings.TrimSuffix(value, "\r\n")); err != nil { + if err := m.InsertHeader(0, name, " "+strings.TrimSuffix(value, "\r\n")); err != nil { return nil, err } return milter.RespContinue, nil diff --git a/internal/milter/sign_test.go b/internal/milter/sign_test.go index 5034059..e65dc2c 100644 --- a/internal/milter/sign_test.go +++ b/internal/milter/sign_test.go @@ -136,11 +136,14 @@ func sendMessage(t *testing.T, s *milter.ClientSession, login string, headers [] } // delivered is the message as Postfix delivers it with the signature -// inserted at the top: every header line ending CRLF. +// inserted at the top: every header line ending CRLF, and the signature as +// "name:value" - its value brings its own space (Postfix 3.10.13, +// src/cleanup/cleanup_milter.c:1085, "%s:%s%s" with no space for a milter +// that asked for SMFIP_HDR_LEADSPC). func delivered(sig milter.ModifyAction, headers []field, body string) []byte { var b bytes.Buffer crlf := func(v string) string { return strings.ReplaceAll(v, "\n", "\r\n") } - b.WriteString(sig.HeaderName + ": " + crlf(sig.HeaderValue) + "\r\n") + b.WriteString(sig.HeaderName + ":" + crlf(sig.HeaderValue) + "\r\n") for _, h := range headers { b.WriteString(h.name + ":" + crlf(h.value) + "\r\n") } @@ -187,15 +190,28 @@ func TestSignerSignsWithTheLoginsDomain(t *testing.T) { if v.Domain != "example.org" { t.Errorf("d=%s, want example.org", v.Domain) } - for _, want := range []string{"c=relaxed/simple", "s=sel", "h=From:To:Subject:Date:From"} { - if !strings.Contains(strings.ReplaceAll(sig.HeaderValue, "\n", ""), want) { + if !strings.HasPrefix(sig.HeaderValue, " ") { + t.Errorf("the signature's value does not bring its space: %q", sig.HeaderValue) + } + for _, want := range []string{"c=relaxed/simple;", "s=sel;", "h=From:To:Subject:Date:To:Subject:Date:From;"} { + if !strings.Contains(strings.Join(strings.Fields(sig.HeaderValue), ""), want) { t.Errorf("signature lacks %q: %s", want, sig.HeaderValue) } } - // A second From: added on the way breaks it: From is over-signed. - tampered := append([]field{{"From", " Mallory "}}, testHeaders...) - if v := verifyWith(t, f.record, delivered(sig, tampered, testBody)); v.Err == nil { - t.Error("a From: added above still verifies") + // A second From:, Subject: or To: added on the way breaks it: each is + // over-signed. + for _, added := range []field{{"From", " Mallory "}, {"Subject", " urgent: wire the money"}, {"To", " victim@example.net"}} { + tampered := append([]field{added}, testHeaders...) + if v := verifyWith(t, f.record, delivered(sig, tampered, testBody)); v.Err == nil { + t.Errorf("a %s: added above still verifies", added.name) + } + } + // A Cc: the message had none of may be added: only what is present is + // over-signed, as Postfix's cleanup adds a missing Date: after the + // milters. + withCc := append([]field{{"Cc", " someone@example.net"}}, testHeaders...) + if v := verifyWith(t, f.record, delivered(sig, withCc, testBody)); v.Err != nil { + t.Errorf("a Cc: the message did not have breaks it: %v", v.Err) } // A changed body breaks it. if v := verifyWith(t, f.record, delivered(sig, testHeaders, testBody+"more\r\n")); v.Err == nil { @@ -297,10 +313,57 @@ func TestSignerDefersWhatItCannotSign(t *testing.T) { } } +// Two To: lines and the headers OpenDKIM's list left out verify as signed. +func TestSignerSignsRepeatedHeaders(t *testing.T) { + f := newSignFixture(t) + s := startSigner(t, f, appDomains) + headers := []field{ + {"From", " Alice "}, {"Sender", " list@example.org"}, + {"To", " bob@example.net"}, {"To", " carol@example.net"}, {"Cc", " dave@example.net"}, + {"Subject", " two To lines"}, + {"List-Unsubscribe", " "}, {"List-Unsubscribe-Post", " List-Unsubscribe=One-Click"}, + } + sig := signed(t, s, "app1", headers, testBody) + if v := verifyWith(t, f.record, delivered(sig, headers, testBody)); v.Err != nil { + t.Fatalf("signature does not verify: %v", v.Err) + } + // The second To: changed: it was signed too. + changed := append([]field(nil), headers...) + changed[3] = field{"To", " mallory@example.net"} + if v := verifyWith(t, f.record, delivered(sig, changed, testBody)); v.Err == nil { + t.Error("a changed second To: still verifies") + } +} + +// A header section past headerLimit is refused for good, before any key is +// looked up, and nothing is inserted. +func TestSignerRefusesAHugeHeaderSection(t *testing.T) { + f := newSignFixture(t) + s := startSigner(t, f, appDomains) + headers := []field{{"From", " Alice "}} + filler := strings.Repeat("x", 50000) + for i := 0; i*50000 < headerLimit; i++ { + headers = append(headers, field{"X-Filler", " " + filler}) + } + mods, act := sendMessage(t, s, "app1", headers, testBody) + if act.Code != milter.ActReplyCode || act.SMTPCode != 552 || len(mods) != 0 { + t.Fatalf("got %+v with %d modifications, want a 552 reply and none", act, len(mods)) + } +} + // signLines writes the block Postfix will deliver and names what is signed. func TestSignLines(t *testing.T) { keys, block := signLines(testHeaders) - if got := strings.Join(keys, ":"); got != "From:To:Subject:Date:From" { + if got := strings.Join(keys, ":"); got != "From:To:Subject:Date:To:Subject:Date:From" { + t.Errorf("keys %s", got) + } + // Every occurrence is signed; a shown one is named once more, once. + keys, _ = signLines([]field{ + {"From", " a@example.org"}, {"Sender", " b@example.org"}, {"To", " c@example.net"}, {"To", " d@example.net"}, + {"List-Unsubscribe", " "}, {"List-Unsubscribe-Post", " List-Unsubscribe=One-Click"}, + {"X-Other", " not signed"}, + }) + if got := strings.Join(keys, ":"); got != "From:Sender:To:To:List-Unsubscribe:List-Unsubscribe-Post:Sender:To:From" { t.Errorf("keys %s", got) } want := "From: Alice \r\nTo: bob@example.net\r\n" + diff --git a/test/e2e/dkim_shapes_test.go b/test/e2e/dkim_shapes_test.go new file mode 100644 index 0000000..130d6da --- /dev/null +++ b/test/e2e/dkim_shapes_test.go @@ -0,0 +1,90 @@ +package e2e + +import ( + "strings" + "testing" + "time" +) + +// testSignatureOverAwkwardHeaders: the panel's signature verifies, by dkimpy - +// an implementation that is not the signer's library - over the header shapes +// Postfix hands a milter in ways of its own: a folded value (lines joined by +// "\n"), a value led by a tab, one with no space after the colon, a header +// twice, and an 8-bit body (Postfix 3.10.13, src/milter/milter8.c:2329-2400, +// milter8_header(); build/postfix-config.sh, disable_mime_output_conversion). +// The unit tests verify with go-msgauth, the signer's own library, over a +// message they rebuild themselves; this is what Postfix delivers (review of +// panel-dkim-signing, F5). +func testSignatureOverAwkwardHeaders(t *testing.T, sc *scenario) { + token := uniqueToken("shapes") + raw := strings.Join([]string{ + "From: =?utf-8?b?0J7RgtC00LXQuyDRgdCx0YvRgtCw?= ", + "To: first@sink.e2e.test", + "To: second@sink.e2e.test", + "Cc:third@sink.e2e.test", + "Subject: e2e " + token + " with a subject long enough", + "\tto be folded onto a second line", + "X-Tab:\tled by a tab", + "Date: " + time.Now().UTC().Format(time.RFC1123Z), + "MIME-Version: 1.0", + "Content-Type: text/plain; charset=utf-8", + "Content-Transfer-Encoding: 8bit", + "List-Unsubscribe: ", + "List-Unsubscribe-Post: List-Unsubscribe=One-Click", + "", + "Привет - an 8-bit body, sent as it is.", + "A line with trailing white space ", + "", + "", + }, "\r\n") + res := attemptSend(sendAttempt{ + authLogin: sc.appLogin, authPassword: sc.appPassword, + from: "alerts@" + senderDomain, to: recipient, raw: raw, + }) + if !res.ok() { + t.Fatalf("send: %v", res.firstErr()) + } + file, delivered, err := findSinkFile(h.stageDir, token, 30*time.Second) + if err != nil { + t.Fatalf("the message never reached the sink: %v", err) + } + if err := checkPanelSignature(h, file, delivered, senderDomain); err != nil { + t.Fatalf("%v\n%s", err, delivered) + } + for _, want := range []string{"Привет", "List-Unsubscribe-Post"} { + if !strings.Contains(string(delivered), want) { + t.Errorf("the delivered message lacks %q:\n%s", want, delivered) + } + } + // Both To: lines, the one Cc:, the Subject and List-Unsubscribe-Post are + // signed, the shown ones once more. + signedNames := strings.ToLower(dkimTagsOf(t, delivered)["h"]) + for _, want := range []string{"to:to:", "cc:", "subject:", "list-unsubscribe-post:"} { + if !strings.Contains(signedNames, want) { + t.Errorf("h=%s lacks %q", signedNames, want) + } + } +} + +// dkimTagsOf is the tags of the message's one DKIM-Signature. +func dkimTagsOf(t *testing.T, raw []byte) map[string]string { + t.Helper() + text := strings.ReplaceAll(string(raw), "\r\n", "\n") + head, _, _ := strings.Cut(text, "\n\n") + var value string + in := false + for _, line := range strings.Split(head, "\n") { + switch { + case strings.HasPrefix(strings.ToLower(line), "dkim-signature:"): + in, value = true, line[len("dkim-signature:"):] + case in && (strings.HasPrefix(line, " ") || strings.HasPrefix(line, "\t")): + value += line + default: + in = false + } + } + if value == "" { + t.Fatalf("no DKIM-Signature in:\n%s", raw) + } + return dkimTags(value) +} diff --git a/test/e2e/main_test.go b/test/e2e/main_test.go index eb9d2e1..b6daa52 100644 --- a/test/e2e/main_test.go +++ b/test/e2e/main_test.go @@ -303,6 +303,9 @@ func TestE2E(t *testing.T) { sc.deliveryPage = page }) + run("signature_over_awkward_headers", func(t *testing.T) { + testSignatureOverAwkwardHeaders(t, sc) + }) run("notifications_set_up", func(t *testing.T) { testNotificationsSetUp(t, sc) }) diff --git a/test/e2e/smtp_client.go b/test/e2e/smtp_client.go index 56e3125..acf3f12 100644 --- a/test/e2e/smtp_client.go +++ b/test/e2e/smtp_client.go @@ -26,6 +26,9 @@ type sendAttempt struct { moreTo []string // further RCPT TO, after to headerFrom string subject, body string + // raw, when set, is the whole message as written - headers, the empty + // line and the body - in place of buildMessage's. + raw string } // sendResult records the error (if any) at each stage the transaction @@ -148,6 +151,9 @@ func attemptSend(a sendAttempt) sendResult { return res } msg := buildMessage(a) + if a.raw != "" { + msg = a.raw + } if _, err := w.Write([]byte(msg)); err != nil { res.dataErr = err } else if err := w.Close(); err != nil { diff --git a/test/e2e/upgrade_test.go b/test/e2e/upgrade_test.go new file mode 100644 index 0000000..1c88758 --- /dev/null +++ b/test/e2e/upgrade_test.go @@ -0,0 +1,121 @@ +package e2e + +import ( + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "encoding/pem" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" +) + +// TestStartsOnA27Data: the image starts on a /data an earlier image left - +// the DKIM keys under /data/opendkim, as 2.7.x kept them, and a Postfix queue +// owned by numbers that are not this image's postfix and postdrop. Without +// opendkim installed the package accounts get other numbers (postfix 101 -> +// 100, postdrop 103 -> 102 in 2.8.0), and the first rollout of 2.8.0 to the +// test server crash-looped on "postfix check" (2026-10-16): the queue is +// handed to Postfix's users by name before the check now. And the old key is +// the one signing: copied, not made anew, so the published record holds +// (build/entrypoint.sh; review of panel-dkim-signing, F5 - no test ran the +// copy). +func TestStartsOnA27Data(t *testing.T) { + const host = "mail.example.test" + dataDir := t.TempDir() + if err := os.Chmod(dataDir, 0o755); err != nil { + t.Fatal(err) + } + certDir := t.TempDir() + if err := writeSelfSignedCert(certDir+"/fullchain.pem", certDir+"/privkey.pem"); err != nil { + t.Fatalf("generate throwaway TLS cert: %v", err) + } + + // The server's key as 2.7.x kept it, and its KeyTable. + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + pemKey := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(key)}) + keyFile := filepath.Join(dataDir, "opendkim", "keys", host, "selfpost.private") + if err := os.MkdirAll(filepath.Dir(keyFile), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(keyFile, pemKey, 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dataDir, "opendkim", "KeyTable"), + []byte(host+" "+host+":selfpost:/data/opendkim/keys/"+host+"/selfpost.private\n"), 0o644); err != nil { + t.Fatal(err) + } + + // The owners and modes 2.7.x left, and a queue of foreign numbers, laid + // out as root in a throwaway container of the image. + layout := `set -e +chown -R 10999:10998 /data/opendkim +find /data/opendkim -type d -exec chmod 2750 {} + +find /data/opendkim -type f -exec chmod 0640 {} + +q=/data/postfix/queue +for d in active bounce corrupt defer deferred flush hold incoming private saved trace; do + mkdir -p $q/$d; chown 4242:4243 $q/$d; chmod 700 $q/$d +done +mkdir -p $q/maildrop $q/public $q/pid +chown 4242:4244 $q/maildrop $q/public; chmod 730 $q/maildrop; chmod 710 $q/public +chown root:root $q $q/pid; chmod 755 $q $q/pid` + if out, err := exec.Command("docker", "run", "--rm", "--entrypoint", "sh", + "-v", dataDir+":/data", "selfpost:e2e", "-c", layout).CombinedOutput(); err != nil { + t.Fatalf("lay out the 2.7 /data: %v\n%s", err, out) + } + + name := "selfpost-e2e-upgrade" + _ = exec.Command("docker", "rm", "-f", name).Run() + defer func() { + _ = exec.Command("docker", "exec", name, "sh", "-c", + "chown -R root:root /data && chmod -R a+rwX /data").Run() + _ = exec.Command("docker", "rm", "-f", name).Run() + }() + if out, err := exec.Command("docker", "run", "-d", "--name", name, + "-e", "SELFPOST_HOSTNAME="+host, "-e", "MILTER_WAIT_TIMEOUT=10", + "-v", dataDir+":/data", "-v", certDir+":/etc/postfix/tls:ro", + "selfpost:e2e").CombinedOutput(); err != nil { + t.Fatalf("docker run: %v\n%s", err, out) + } + logs := func() string { + out, _ := exec.Command("docker", "logs", name).CombinedOutput() + return string(out) + } + exec1 := func(script string) string { + out, err := exec.Command("docker", "exec", name, "sh", "-c", script).CombinedOutput() + if err != nil { + t.Fatalf("%s: %v\n%s", script, err, out) + } + return strings.TrimSpace(string(out)) + } + + // postfix's startsecs is MILTER_WAIT_TIMEOUT plus ten: RUNNING after 20 s. + if err := waitFor("postfix to be RUNNING", 90*time.Second, time.Second, func() (bool, error) { + out, _ := exec.Command("docker", "exec", name, "supervisorctl", "-c", "/etc/supervisor/supervisord.conf", "status", "postfix").CombinedOutput() + return strings.Contains(string(out), "RUNNING"), nil + }); err != nil { + t.Fatalf("%v\n%s", err, logs()) + } + + if got := exec1(`cmp /data/opendkim/keys/` + host + `/selfpost.private /data/dkim/keys/` + host + `/selfpost.private && echo same`); got != "same" { + t.Errorf("the key under /data/dkim is not the one copied: %s", got) + } + if got := exec1(`cat /data/dkim/selectors`); got != host+" selfpost" { + t.Errorf("selector table = %q", got) + } + if got := exec1(`stat -c '%U:%G %a' /data/opendkim/keys/` + host + `/selfpost.private /data/opendkim/keys`); got != "panel:panel 600\npanel:panel 700" { + t.Errorf("the old tree is not the panel's alone: %q", got) + } + if got := exec1(`stat -c '%U:%G' /data/postfix/queue/active /data/postfix/queue/maildrop /data/postfix/queue/pid`); got != "postfix:postfix\npostfix:postdrop\nroot:postfix" { + t.Errorf("the queue is not Postfix's by name: %q", got) + } + if !strings.Contains(logs(), "the DKIM keys were copied from /data/opendkim to /data/dkim") { + t.Errorf("the entrypoint did not say it copied the keys:\n%s", logs()) + } +}