From d398f488e2f2b0cac2ab8bb584422571ed02f735 Mon Sep 17 00:00:00 2001 From: Mix Date: Mon, 12 Oct 2026 08:34:29 +0300 Subject: [PATCH] inbound-antispam-panel: the filter signs what it judged Closes what the security review left as an accepted risk: with the spam filter down - it is attached fail-open - nothing removes verdict headers a sender wrote himself, and the inbound journal recorded his verdict as the filter's. The owner asked whether rspamd could add a header that proves the others are ours (2026-10-12) and agreed to a signature: a fixed mark could be written by a sender as easily as the verdict. - inbound.FilterKey: a key of 32 random bytes, 64 hex characters, in /data/antispam/filter.key, made on the first start with the relay on. 0644 like the list files beside it: the filter is another user in another container. - deploy/antispam/rspamd/milter_headers.conf: a custom routine reads the key through the directory the sidecar already mounts and adds X-SelfPost-Filter, the HMAC-SHA256 of the message's queue id, hex; a header of that name from the sender is removed. - internal/milter/inbound.go: the verdict headers are kept as they are read, and at end-of-message - when the queue id is known - they count only if the signature fits (FilterSignature, hmac.Equal). No key, no queue id, no signature or a wrong one: the row is that of mail nobody judged (accept, selfpost, no detail). The signature goes on to the upstream with the mail and is of no use there: it fits one queue id. Tried in a throwaway rspamd 3.11 container on the test server before the Go side was written: the routine loads, signs (2aa1ca6d... for key 0123...cdef and queue id 4A1B2C3D5E, the value python's hmac gives and TestFilterSignatureIsWhatTheFilterWrites pins), removes a sender's header of the same name, and adds nothing without a queue id. Tests: signed, unsigned, made-up, another message's, another key's, the sender's followed by the real one, the next message on the connection, no queue id; the key made once, read after, replaced when damaged, left alone by the lists. e2e: filter.key is there, 644, 65 bytes. docs/security.md: the accepted risk is replaced by a narrower one (the key is readable on the host); Mail path describes the signature. Checked on the test server, uncommitted tree: go vet ./... ok, go test ./... ok (22 packages), make e2e ok. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 13 +++- cmd/panel/journal.go | 13 +++- cmd/panel/main.go | 7 ++ deploy/antispam/rspamd/milter_headers.conf | 34 ++++++++- docs/architecture.md | 9 ++- docs/guide.md | 11 +++ docs/plans/inbound-antispam-panel.md | 9 ++- docs/security.md | 42 ++++++----- internal/inbound/lists.go | 44 ++++++++++++ internal/inbound/lists_test.go | 43 ++++++++++++ internal/milter/inbound.go | 81 ++++++++++++++++------ internal/milter/inbound_test.go | 79 +++++++++++++++++++-- test/e2e/port25_features_test.go | 7 ++ 13 files changed, 340 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc53be8..9ab41b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,9 +24,9 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version a connection's later messages had no client address; two changes of the filter lists at once could leave the filter's files behind the page; a held message was logged twice; after an unclean stop of the panel the refusals of - its last seconds were logged twice. Recorded as accepted in - [docs/security.md](docs/security.md): the filter's verdict in the log is the - sender's own while the filter is down; an allow entry for a sender address + its last seconds were logged twice; with the spam filter down, a sender's + own verdict headers were recorded as the filter's. Recorded as accepted in + [docs/security.md](docs/security.md): an allow entry for a sender address trusts an address anyone can claim; the list files are readable by anyone on the host. - `INBOUND_ANTISPAM_MILTER` and `INBOUND_JOURNAL_MILTER_SOCKET` are checked @@ -91,6 +91,13 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version relay on. - Help: *Inbound log* and *Filter lists*. - New setting `ANTISPAM_DIR` (default `/data/antispam`). +- **The spam filter signs what it judged.** rspamd adds `X-SelfPost-Filter` to + every message it lets through - an HMAC of the message's queue id under a + key SelfPost creates in `data/antispam/filter.key` - and the inbound log + shows a verdict only next to a signature that fits. Nothing to configure, + but the sidecar needs the new `antispam/rspamd/milter_headers.conf` and a + restart after SelfPost's first start; until then accepted mail is logged + as *selfpost*, with no verdict. - A full backup carries the inbound log, the filter lists and their files under `data/antispam/`. It does not carry the `antispam/` directory with the compose fragment and the rspamd configuration - copy that from diff --git a/cmd/panel/journal.go b/cmd/panel/journal.go index 05933c0..d84835f 100644 --- a/cmd/panel/journal.go +++ b/cmd/panel/journal.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" + "github.com/mixeme/selfpost/internal/inbound" "github.com/mixeme/selfpost/internal/milter" "github.com/mixeme/selfpost/internal/store" ) @@ -33,8 +34,18 @@ func serveInboundJournal(ctx context.Context, cfg config, st *store.Store) error if err != nil { return err } + // With a filter attached, the key it signs with. Without the key no + // verdict can be told from a sender's own, so none is read: the journal + // still records the mail, and the log says why it shows no verdicts. + key := "" + if cfg.inboundAntispamMilter != "" { + if key, err = inbound.FilterKey(cfg.antispamDir); err != nil { + log.Printf("inbound-journal-milter: no key for the filter's signature, verdicts will not be recorded: %v", err) + key = "" + } + } log.Printf("inbound-journal-milter listening on %s", cfg.inboundJournalSocket) - return milter.ServeInbound(ctx, ln, st, cfg.inboundAntispamMilter != "") + return milter.ServeInbound(ctx, ln, st, key) } // listenMilter opens a milter's Unix socket where Postfix can reach it. diff --git a/cmd/panel/main.go b/cmd/panel/main.go index a14e0e1..10b9166 100644 --- a/cmd/panel/main.go +++ b/cmd/panel/main.go @@ -312,6 +312,13 @@ func run() error { log.Printf("filter lists bootstrap: %v", err) } } + // The key the filter signs with has to be in the shared directory before + // the filter reads it, whether or not a filter is attached yet. + if cfg.inboundEnabled { + if _, err := inbound.FilterKey(cfg.antispamDir); err != nil { + log.Printf("filter key bootstrap: %v", err) + } + } if err := st.EnsureSendLogRetentionDays(cfg.retentionDays); err != nil { return fmt.Errorf("bootstrap send-log retention: %w", err) diff --git a/deploy/antispam/rspamd/milter_headers.conf b/deploy/antispam/rspamd/milter_headers.conf index 4b57e74..6dc2df6 100644 --- a/deploy/antispam/rspamd/milter_headers.conf +++ b/deploy/antispam/rspamd/milter_headers.conf @@ -8,4 +8,36 @@ # rspamd removes headers of these names that a sender wrote himself before it # adds its own. Without this file the journal still lists accepted mail, with # no verdict next to it. -use = ["x-spamd-result", "x-rspamd-action"]; +# +# The third header is the proof that the other two are rspamd's. The filter is +# attached so that mail still flows when it is down, and then nothing removes +# what a sender wrote: he could send his own verdict. So rspamd signs each +# message - an HMAC-SHA256 of the message's queue id under a key SelfPost +# generates and leaves in the directory it shares with this container - and +# SelfPost reads a verdict only next to a signature it can recompute. The +# signature travels on with the mail and is of no use to whoever sees it: it +# fits that one queue id. +use = ["x-spamd-result", "x-rspamd-action", "selfpost-signature"]; + +custom { + selfpost-signature = <= 32 && !strings.ContainsAny(key, " \t\r\n") { + return key, nil + } + // Empty or cut short: not a key anyone should sign with. Replaced below. + } else if !os.IsNotExist(err) { + return "", fmt.Errorf("read %s: %w", path, err) + } + if err := os.MkdirAll(dir, 0o755); err != nil { + return "", fmt.Errorf("create %s: %w", dir, err) + } + buf := make([]byte, 32) + if _, err := rand.Read(buf); err != nil { + return "", fmt.Errorf("generate the filter key: %w", err) + } + key := hex.EncodeToString(buf) + if err := atomicfile.Write(path, []byte(key+"\n"), 0o644); err != nil { + return "", err + } + return key, nil +} diff --git a/internal/inbound/lists_test.go b/internal/inbound/lists_test.go index d3b2676..4647d2d 100644 --- a/internal/inbound/lists_test.go +++ b/internal/inbound/lists_test.go @@ -395,3 +395,46 @@ func TestListable(t *testing.T) { } } } + +// The key the filter signs with is made once and then read, and is readable by +// the filter, which is another user in another container. +func TestFilterKey(t *testing.T) { + dir := filepath.Join(t.TempDir(), "antispam") + key, err := FilterKey(dir) + if err != nil || len(key) != 64 || strings.Trim(key, "0123456789abcdef") != "" { + t.Fatalf("FilterKey = %q, %v", key, err) + } + again, err := FilterKey(dir) + if err != nil || again != key { + t.Errorf("a second call = %q, %v; want the same key", again, err) + } + raw, err := os.ReadFile(filepath.Join(dir, FilterKeyFile)) + if err != nil || string(raw) != key+"\n" { + t.Errorf("file = %q, %v", raw, err) + } + if info, _ := os.Stat(filepath.Join(dir, FilterKeyFile)); os.PathSeparator == '/' && info.Mode().Perm() != 0o644 { + t.Errorf("mode = %o", info.Mode().Perm()) + } + + // A file that is not a key is replaced, not signed with. + if err := os.WriteFile(filepath.Join(dir, FilterKeyFile), []byte("short\n"), 0o644); err != nil { + t.Fatal(err) + } + fresh, err := FilterKey(dir) + if err != nil || len(fresh) != 64 || fresh == key { + t.Errorf("after a damaged file: %q, %v", fresh, err) + } + + // The lists leave it alone. + st, err := store.Open(filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatal(err) + } + defer st.Close() + if err := NewLists(st, dir).Sync(); err != nil { + t.Fatal(err) + } + if kept, _ := FilterKey(dir); kept != fresh { + t.Errorf("the key changed when the lists were written") + } +} diff --git a/internal/milter/inbound.go b/internal/milter/inbound.go index a2ff7b4..fa4f5e6 100644 --- a/internal/milter/inbound.go +++ b/internal/milter/inbound.go @@ -2,6 +2,9 @@ package milter import ( "context" + "crypto/hmac" + "crypto/sha256" + "encoding/hex" "log" "net" "net/textproto" @@ -33,10 +36,12 @@ type InboundStore interface { type inboundSession struct { milter.NoOpMilter rec InboundStore - // filter says whether an external filter is attached in front of this - // milter. Without one the verdict headers below can only have been written - // by the sender, and are not read. - filter bool + // key is what the filter signs each message with (FilterSignature), or + // empty when no filter is attached. A verdict is read only next to a + // signature that fits: the filter is attached so that mail flows when it is + // down, and then the verdict headers of a message are whatever its sender + // wrote. + key string // clientIP is taken at Connect and again from {client_addr} with every // MAIL FROM: a session the library made after a non-continue reply never @@ -49,6 +54,7 @@ type inboundSession struct { action string // X-Rspamd-Action result string // X-Spamd-Result seen bool // any verdict header of the filter + sig string // X-SelfPost-Filter } func (s *inboundSession) Connect(host, family string, port uint16, addr net.IP, m *milter.Modifier) (milter.Response, error) { @@ -73,10 +79,12 @@ func (s *inboundSession) RcptTo(rcpt string, m *milter.Modifier) (milter.Respons return milter.RespContinue, nil } -// Header keeps the first Subject and the filter's verdict. Postfix hands the -// message to its milters one after another at end-of-message, each seeing what -// the one before added, which is how the filter's headers arrive here. The -// filter replaces headers of these names, so the last one read is its own. +// Header keeps the first Subject, the filter's verdict and its signature. +// Postfix hands the message to its milters one after another at +// end-of-message, each seeing what the one before added, which is how the +// filter's headers arrive here. The filter replaces headers of these names, so +// the last one read is its own - when it is up. Whether it was is decided at +// end-of-message, by the signature. func (s *inboundSession) Header(name, value string, m *milter.Modifier) (milter.Response, error) { switch textproto.CanonicalMIMEHeaderKey(name) { case "Subject": @@ -84,21 +92,50 @@ func (s *inboundSession) Header(name, value string, m *milter.Modifier) (milter. s.subject = mailhdr.DecodeSubject(value) } case "X-Rspamd-Action": - if s.filter { - s.action, s.seen = value, true - } + s.action, s.seen = value, true case "X-Spamd-Result": - if s.filter { - s.result, s.seen = value, true - } + s.result, s.seen = value, true + case signatureHeader: + s.sig = strings.TrimSpace(value) } return milter.RespContinue, nil } +// SignatureHeader is the header the filter signs a message in +// (deploy/antispam/rspamd/milter_headers.conf writes it). +const SignatureHeader = "X-SelfPost-Filter" + +var signatureHeader = textproto.CanonicalMIMEHeaderKey(SignatureHeader) + +// FilterSignature is what the filter writes into SignatureHeader for a message: +// the HMAC-SHA256 of its queue id under the key SelfPost shares with the +// filter, in hex. The key is used as the text it is stored as. A signature +// fits one queue id, so the header going on to the upstream with the mail +// gives nothing to whoever reads it there. +func FilterSignature(key, queueID string) string { + mac := hmac.New(sha256.New, []byte(key)) + mac.Write([]byte(queueID)) + return hex.EncodeToString(mac.Sum(nil)) +} + +// signed reports whether the message carries the filter's signature for its +// queue id. No key, no queue id or no signature is "no". +func (s *inboundSession) signed(queueID string) bool { + if s.key == "" || queueID == "" || s.sig == "" { + return false + } + return hmac.Equal([]byte(strings.ToLower(s.sig)), []byte(FilterSignature(s.key, queueID))) +} + // Body fires at end-of-message: the message is about to be queued for its // upstream. "Continue" lets it through as "accept" would, and keeps this // session for the connection's next message. func (s *inboundSession) Body(m *milter.Modifier) (milter.Response, error) { + // The queue id is known only now, and with it whether the verdict headers + // read on the way here are the filter's. + if !s.signed(macro(m, "i")) { + s.seen = false + } s.record() s.reset() return milter.RespContinue, nil @@ -111,7 +148,7 @@ func (s *inboundSession) Abort(m *milter.Modifier) error { func (s *inboundSession) reset() { s.from, s.rcpts, s.subject = "", nil, "" - s.action, s.result, s.seen = "", "", false + s.action, s.result, s.seen, s.sig = "", "", false, "" } // record writes one row per recipient at an inbound domain. Other recipients @@ -169,9 +206,8 @@ var ( // rspamdVerdict reduces the filter's headers to an action and a one-line // summary: the action, the score against the threshold, and the first few // symbols that carried weight. It reads only what it recognises - the headers -// are the filter's when it is up, but the sender's own when it is not (it is -// attached fail-open), so nothing of them is kept verbatim except through -// these patterns. +// are read only next to the filter's signature, and even so nothing of them is +// kept verbatim except through these patterns. func rspamdVerdict(actionHeader, resultHeader string) (action, detail string) { action = strings.ToLower(strings.TrimSpace(actionHeader)) for _, r := range action { @@ -229,11 +265,12 @@ func rspamdVerdict(actionHeader, resultHeader string) (action, detail string) { } // ServeInbound runs the inbound journal milter on ln until ctx is cancelled. -// filter says whether an external filter is attached in front of it -// (INBOUND_ANTISPAM_MILTER). It returns nil on a clean shutdown. -func ServeInbound(ctx context.Context, ln net.Listener, rec InboundStore, filter bool) error { +// key is what the external filter in front of it signs messages with, or +// empty when none is attached (INBOUND_ANTISPAM_MILTER): no verdict is then +// read at all. It returns nil on a clean shutdown. +func ServeInbound(ctx context.Context, ln net.Listener, rec InboundStore, key string) error { srv := &milter.Server{ - NewMilter: func() milter.Milter { return &inboundSession{rec: rec, filter: filter} }, + NewMilter: func() milter.Milter { return &inboundSession{rec: rec, key: key} }, Actions: 0, // read-only Protocol: milter.OptNoBody, // headers and end-of-message, not the body } diff --git a/internal/milter/inbound_test.go b/internal/milter/inbound_test.go index 9ecd915..2f5712e 100644 --- a/internal/milter/inbound_test.go +++ b/internal/milter/inbound_test.go @@ -60,9 +60,18 @@ func message(t *testing.T, s *inboundSession, from string, rcpts []string, heade return resp } +// testKey is the key the tests' filter signs with; the messages they replay +// have the queue id ABC123. +const testKey = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + +var testSig = [2]string{SignatureHeader, FilterSignature(testKey, "ABC123")} + func inboundSessionFor(t *testing.T, rec InboundStore, filter bool) *inboundSession { t.Helper() - s := &inboundSession{rec: rec, filter: filter} + s := &inboundSession{rec: rec} + if filter { + s.key = testKey + } if _, err := s.Connect("mx.example.net", "tcp4", 0, net.ParseIP("203.0.113.7"), mods(nil)); err != nil { t.Fatalf("Connect: %v", err) } @@ -116,6 +125,7 @@ func TestInboundReadsTheFiltersVerdict(t *testing.T) { {"Subject", "Cheap pills"}, {"X-Rspamd-Action", "add header"}, {"X-Spamd-Result", spamdResult}, + testSig, }) if len(rec.rows) != 1 { t.Fatalf("rows = %+v", rec.rows) @@ -133,7 +143,7 @@ func TestInboundQuarantineAction(t *testing.T) { rec := &fakeInbound{domains: map[string]bool{"in.example.com": true}} s := inboundSessionFor(t, rec, true) message(t, s, "sender@example.net", []string{"a@in.example.com"}, - [][2]string{{"x-rspamd-action", "quarantine"}}) + [][2]string{{"x-rspamd-action", "quarantine"}, {"x-selfpost-filter", strings.ToUpper(testSig[1])}}) if len(rec.rows) != 1 || rec.rows[0].Decision != store.SpamDecisionQuarantine || rec.rows[0].Detail != "quarantine" { t.Errorf("rows = %+v", rec.rows) } @@ -180,7 +190,7 @@ func TestInboundResetsBetweenMessages(t *testing.T) { rec := &fakeInbound{domains: map[string]bool{"in.example.com": true}} s := inboundSessionFor(t, rec, true) message(t, s, "one@example.net", []string{"a@in.example.com"}, [][2]string{ - {"Subject", "First"}, {"X-Rspamd-Action", "add header"}, + {"Subject", "First"}, {"X-Rspamd-Action", "add header"}, testSig, }) message(t, s, "two@example.net", []string{"b@in.example.com"}, nil) if len(rec.rows) != 2 { @@ -259,7 +269,7 @@ func TestInboundClientAddressFromTheMailMacro(t *testing.T) { // cost it its session. func TestInboundOnlyEverContinues(t *testing.T) { rec := &fakeInbound{domains: map[string]bool{"in.example.com": true}} - s := &inboundSession{rec: rec, filter: true} + s := &inboundSession{rec: rec, key: testKey} check := func(stage string, resp milter.Response, err error) { t.Helper() if err != nil || resp != milter.RespContinue { @@ -277,3 +287,64 @@ func TestInboundOnlyEverContinues(t *testing.T) { resp, err = s.Body(mods(nil)) check("end of message", resp, err) } + +// The filter is attached so that mail flows when it is down, and then the +// verdict headers of a message are its sender's own. A verdict is recorded only +// next to the filter's signature for this message's queue id. +func TestInboundReadsAVerdictOnlyWithTheFiltersSignature(t *testing.T) { + verdict := [][2]string{{"X-Rspamd-Action", "quarantine"}, {"X-Spamd-Result", spamdResult}} + for name, c := range map[string]struct { + extra [][2]string + signed bool + }{ + "signed": {[][2]string{testSig}, true}, + "no signature": {nil, false}, + "a made-up signature": {[][2]string{{SignatureHeader, strings.Repeat("ab", 32)}}, false}, + "another message's signature": {[][2]string{{SignatureHeader, FilterSignature(testKey, "OTHER1")}}, false}, + "a signature under another key": {[][2]string{{SignatureHeader, FilterSignature(strings.Repeat("f", 64), "ABC123")}}, false}, + "the sender's first, then the real": {[][2]string{{SignatureHeader, "forged"}, testSig}, true}, + } { + rec := &fakeInbound{domains: map[string]bool{"in.example.com": true}} + s := inboundSessionFor(t, rec, true) + message(t, s, "sender@example.net", []string{"a@in.example.com"}, append(append([][2]string{}, verdict...), c.extra...)) + if len(rec.rows) != 1 { + t.Fatalf("%s: rows = %+v", name, rec.rows) + } + got := rec.rows[0] + if c.signed && (got.Engine != store.SpamEngineRspamd || got.Decision != store.SpamDecisionQuarantine) { + t.Errorf("%s: a signed verdict was not read: %+v", name, got) + } + if !c.signed && (got.Engine != store.SpamEngineSelfPost || got.Decision != store.SpamDecisionAccept || got.Detail != "") { + t.Errorf("%s: an unsigned verdict was recorded: %+v", name, got) + } + } + + // A signature is of no use to the next message on the connection. + rec := &fakeInbound{domains: map[string]bool{"in.example.com": true}} + s := inboundSessionFor(t, rec, true) + message(t, s, "one@example.net", []string{"a@in.example.com"}, append(append([][2]string{}, verdict...), testSig)) + message(t, s, "two@example.net", []string{"a@in.example.com"}, verdict) + if len(rec.rows) != 2 || rec.rows[1].Engine != store.SpamEngineSelfPost { + t.Errorf("rows = %+v", rec.rows) + } + + // With no queue id nothing can be checked, so nothing is trusted. + rec = &fakeInbound{domains: map[string]bool{"in.example.com": true}} + s = inboundSessionFor(t, rec, true) + _, _ = s.MailFrom("sender@example.net", mods(nil)) + _, _ = s.RcptTo("a@in.example.com", mods(nil)) + _, _ = s.Header("X-Rspamd-Action", "reject", mods(nil)) + _, _ = s.Header(SignatureHeader, FilterSignature(testKey, ""), mods(nil)) + _, _ = s.Body(mods(nil)) + if len(rec.rows) != 1 || rec.rows[0].Engine != store.SpamEngineSelfPost { + t.Errorf("no queue id: rows = %+v", rec.rows) + } +} + +// What rspamd 3.11 wrote for this key and queue id in a throwaway container +// (dev trial, 2026-10-12): the two ends must compute the same thing. +func TestFilterSignatureIsWhatTheFilterWrites(t *testing.T) { + if got := FilterSignature(testKey, "4A1B2C3D5E"); got != "2aa1ca6dd1545d3f16c8c0c29ba59bfc28c050224ac2c5c953bd2617f73b6de2" { + t.Errorf("FilterSignature = %s", got) + } +} diff --git a/test/e2e/port25_features_test.go b/test/e2e/port25_features_test.go index 9d81bff..62e3e48 100644 --- a/test/e2e/port25_features_test.go +++ b/test/e2e/port25_features_test.go @@ -283,6 +283,13 @@ func testInboundRelay(t *testing.T, sc *scenario) { } // The inbound journal milter saw it go through, and the Inbound log shows it. + // Beside the maps, the key the filter signs each message with: there from + // the first start with the relay on, whether or not a filter is attached + // yet, one line of 64 hex characters, readable by the filter. + if key, err := h.execIn("selfpost", "stat", "-c", "%a %s", "/data/antispam/filter.key"); err != nil || strings.TrimSpace(key) != "644 65" { + t.Fatalf("/data/antispam/filter.key: %q, %v; want mode 644 and 65 bytes", key, err) + } + // No filter is attached on the stand: the row is an accept by nobody, so its // reason is the engine alone. logRows := func(want string) []string {