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 <noreply@anthropic.com>

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,

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)

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
 // "<name> <selector>" each, sorted by name so the output is deterministic.
 // Every domain is re-checked for config-injection safety before being written

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

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{})

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

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
 // "<name> <selector>" each); the key of a name is
-// keysDir/<name>/<selector>.private (domain.KeyPath). The table is read again
-// when it changes, a key file when it does.
+// keysDir/<name>/<selector>.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

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 <m@evil.example>"}}, 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 <m@evil.example>"}, {"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 <alice@example.org>"}, {"Sender", " list@example.org"},
+		{"To", " bob@example.net"}, {"To", " carol@example.net"}, {"Cc", " dave@example.net"},
+		{"Subject", " two To lines"},
+		{"List-Unsubscribe", " <https://example.org/u/1>"}, {"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 <alice@example.org>"}}
+	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", " <https://example.org/u>"}, {"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 <alice@example.org>\r\nTo: bob@example.net\r\n" +