diff --git a/build/postfix-config.sh b/build/postfix-config.sh index bbf5109..0940e10 100644 --- a/build/postfix-config.sh +++ b/build/postfix-config.sh @@ -230,6 +230,14 @@ postconf -e \ "smtpd_milters={ unix:${OPENDKIM_SOCK}, default_action=tempfail }, { unix:${JOURNAL_SOCK}, default_action=accept }" \ "non_smtpd_milters=" +# What Postfix tells a milter with MAIL FROM: its defaults plus the client's +# address. The panel's milters judge every message by that address (an +# application's client-address list) and record it (the inbound journal), and +# the address given once at connect is not there for a connection's later +# messages - see MailFrom in internal/milter/milter.go. +postconf -e \ + "milter_mail_macros=i {auth_type} {auth_authen} {auth_author} {mail_addr} {mail_host} {mail_mailer} {client_addr}" + # Bounded milter timeouts: a *hung* milter (socket accepts but never replies) # must fail open just like a crash, not stall mail acceptance until the Postfix # defaults (300s content) elapse. With default_action per milter, a @@ -364,7 +372,14 @@ if [ "${INBOUND_RELAY_ENABLE}" = "true" ] || [ "${DMARC_REPORTS_ENABLE}" = "true # panel's inbound journal milter, or nothing at all. INBOUND_MILTERS="" if [ "${INBOUND_RELAY_ENABLE}" = "true" ] && [ -n "${INBOUND_ANTISPAM_MILTER}" ]; then + # A glob's "*" matches anything, so the shape alone would pass + # "inet:a:1},{unix:/elsewhere": characters outside the set are refused + # first, then the shape is checked. case "${INBOUND_ANTISPAM_MILTER}" in + *[!A-Za-z0-9._:/-]*) + echo "FATAL: INBOUND_ANTISPAM_MILTER must be inet:host:port or unix:/path, got: ${INBOUND_ANTISPAM_MILTER}" >&2 + exit 1 + ;; inet:[A-Za-z0-9._-]*:[0-9]* | unix:/[A-Za-z0-9._/-]* ) ;; *) echo "FATAL: INBOUND_ANTISPAM_MILTER must be inet:host:port or unix:/path, got: ${INBOUND_ANTISPAM_MILTER}" >&2 @@ -388,6 +403,10 @@ if [ "${INBOUND_RELAY_ENABLE}" = "true" ] || [ "${DMARC_REPORTS_ENABLE}" = "true # it is down. A comma, not a space, between the two - see # master_cf_set_smtp_inet_o. case "${INBOUND_JOURNAL_SOCK}" in + *[!A-Za-z0-9._/-]*) + echo "FATAL: INBOUND_JOURNAL_MILTER_SOCKET must be an absolute path, got: ${INBOUND_JOURNAL_SOCK}" >&2 + exit 1 + ;; /[A-Za-z0-9._/-]*) ;; *) echo "FATAL: INBOUND_JOURNAL_MILTER_SOCKET must be an absolute path, got: ${INBOUND_JOURNAL_SOCK}" >&2 diff --git a/internal/milter/authip.go b/internal/milter/authip.go index 7b3c483..d9b9d21 100644 --- a/internal/milter/authip.go +++ b/internal/milter/authip.go @@ -8,11 +8,12 @@ import ( ) // authIPAllowed reports whether the authenticated application may submit from -// the connecting client IP. When the application has no IP restriction, or the -// client IP is not known, the check passes. Store errors are fail-open (see -// overLimit). +// the connecting client IP. When the application has no IP restriction the +// check passes. When it has one and the client's address is not known, the +// check fails: an address list that an unknown address satisfies is no list. +// Store errors are fail-open (see overLimit). func (s *session) authIPAllowed() bool { - if s.login == "" || s.clientIP == "" { + if s.login == "" { return true } a, err := s.rec.ApplicationByLogin(s.login) @@ -26,6 +27,10 @@ func (s *session) authIPAllowed() bool { if a.AllowsAuthFromIP(s.clientIP) { return true } + if s.clientIP == "" { + log.Printf("journal-milter: application %q refused — its client IPs are restricted and Postfix gave no client address", s.login) + return false + } log.Printf("journal-milter: application %q refused from %s — client IP not allowed", s.login, s.clientIP) return false } diff --git a/internal/milter/inbound.go b/internal/milter/inbound.go index 5f50c89..a2ff7b4 100644 --- a/internal/milter/inbound.go +++ b/internal/milter/inbound.go @@ -27,8 +27,8 @@ type InboundStore interface { // of a message that reached end-of-message - that is, one the filter did not // refuse; refusals never get this far and are read from mail.log instead. // -// Like the send-log milter it only watches: every callback continues or -// accepts, and Postfix attaches the socket with default_action=accept, so +// Like the send-log milter it only watches: every callback continues, and +// Postfix attaches the socket with default_action=accept, so // neither an error here nor the panel being down holds up inbound mail. type inboundSession struct { milter.NoOpMilter @@ -38,7 +38,10 @@ type inboundSession struct { // by the sender, and are not read. filter bool - clientIP string // once per connection + // 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 + // sees Connect (see session.MailFrom in milter.go). + clientIP string from string rcpts []string @@ -58,6 +61,9 @@ func (s *inboundSession) Connect(host, family string, port uint16, addr net.IP, // MailFrom starts a message: a connection may carry several. func (s *inboundSession) MailFrom(from string, m *milter.Modifier) (milter.Response, error) { s.reset() + if ip := clientAddr(m); ip != "" { + s.clientIP = ip + } s.from = cleanAddress(from) return milter.RespContinue, nil } @@ -90,11 +96,12 @@ func (s *inboundSession) Header(name, value string, m *milter.Modifier) (milter. } // Body fires at end-of-message: the message is about to be queued for its -// upstream. +// 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) { s.record() s.reset() - return milter.RespAccept, nil + return milter.RespContinue, nil } func (s *inboundSession) Abort(m *milter.Modifier) error { diff --git a/internal/milter/inbound_test.go b/internal/milter/inbound_test.go index a4b9d34..9ecd915 100644 --- a/internal/milter/inbound_test.go +++ b/internal/milter/inbound_test.go @@ -77,8 +77,8 @@ func TestInboundRecordsAcceptedMessage(t *testing.T) { resp := message(t, s, " SIZE=1200", []string{"", "b@IN.example.com"}, [][2]string{{"Subject", "=?utf-8?Q?Sch=C3=B6n?="}}) - if resp != milter.RespAccept { - t.Errorf("end-of-message response = %v, want accept", resp) + if resp != milter.RespContinue { + t.Errorf("end-of-message response = %v, want continue", resp) } if len(rec.rows) != 2 { t.Fatalf("rows = %+v, want one per recipient", rec.rows) @@ -217,8 +217,8 @@ func TestInboundAcceptsWhenTheStoreFails(t *testing.T) { "lookup": {lookupErr: errors.New("boom")}, } { s := inboundSessionFor(t, rec, false) - if resp := message(t, s, "sender@example.net", []string{"a@in.example.com"}, nil); resp != milter.RespAccept { - t.Errorf("%s failure: response = %v, want accept", name, resp) + if resp := message(t, s, "sender@example.net", []string{"a@in.example.com"}, nil); resp != milter.RespContinue { + t.Errorf("%s failure: response = %v, want continue", name, resp) } } } @@ -234,3 +234,46 @@ func TestRspamdVerdictKeepsTheListSymbols(t *testing.T) { t.Errorf("detail = %q\nwant %q", detail, want) } } + +// The client address of a connection's later messages comes from the MAIL +// macro: a session the library made after a non-continue reply saw no Connect. +func TestInboundClientAddressFromTheMailMacro(t *testing.T) { + rec := &fakeInbound{domains: map[string]bool{"in.example.com": true}} + s := &inboundSession{rec: rec} // no Connect + if _, err := s.MailFrom("sender@example.net", mods(map[string]string{"{client_addr}": "IPv6:2001:db8::7"})); err != nil { + t.Fatal(err) + } + if _, err := s.RcptTo("a@in.example.com", mods(nil)); err != nil { + t.Fatal(err) + } + if _, err := s.Body(mods(nil)); err != nil { + t.Fatal(err) + } + if len(rec.rows) != 1 || rec.rows[0].ClientIP != "2001:db8::7" { + t.Errorf("rows = %+v", rec.rows) + } +} + +// No callback of the inbound milter answers anything but "continue": it must +// never be the reason a message is held up, and a non-continue reply would +// 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} + check := func(stage string, resp milter.Response, err error) { + t.Helper() + if err != nil || resp != milter.RespContinue { + t.Errorf("%s: %v, %v", stage, resp, err) + } + } + resp, err := s.Connect("mx", "tcp4", 0, net.ParseIP("203.0.113.7"), mods(nil)) + check("connect", resp, err) + resp, err = s.MailFrom("a@example.net", mods(nil)) + check("mail", resp, err) + resp, err = s.RcptTo("a@in.example.com", mods(nil)) + check("rcpt", resp, err) + resp, err = s.Header("X-Rspamd-Action", "reject", mods(nil)) + check("header", resp, err) + resp, err = s.Body(mods(nil)) + check("end of message", resp, err) +} diff --git a/internal/milter/milter.go b/internal/milter/milter.go index aed7e31..f49e62b 100644 --- a/internal/milter/milter.go +++ b/internal/milter/milter.go @@ -53,7 +53,11 @@ type session struct { // valid zero value (no in-flight accounting). flight *inflight - clientIP string // captured once per connection + // clientIP is the client's address. Postfix gives it at Connect, once per + // connection, and again with every MAIL FROM as the {client_addr} macro + // (milter_mail_macros in build/postfix-config.sh). The second is the one + // that can be relied on: see MailFrom. + clientIP string login string from string @@ -62,9 +66,8 @@ type session struct { reserved []*reservation // level-2 slots held by the current message } -// Connect captures the client IP, which comes from the addr parameter rather -// than a macro (the {client_addr} macro was empty in testing). It is the -// rate-limit key; here it is recorded for completeness. +// Connect captures the client IP of a connection's first message. It is not +// enough on its own - see MailFrom. func (s *session) Connect(host, family string, port uint16, addr net.IP, m *milter.Modifier) (milter.Response, error) { if addr != nil { s.clientIP = addr.String() @@ -79,8 +82,21 @@ func (s *session) Connect(host, family string, port uint16, addr net.IP, m *milt // limit is enforced here: over the limit, the message is refused with a 4xx // tempfail before recipients are even offered (guide § Rate limiting). // Enforcement is fail-open — see overLimit. +// +// The client address is read again here, from the {client_addr} macro. The +// milter library replaces the session with a new one after every reply that is +// not "continue" - a refusal, or the accept that used to end a message - and +// Postfix does not send Connect again on the same connection. A session made +// that way never saw the client, so every message of a connection after the +// first, and every MAIL FROM after a refused one, used to be judged with no +// client address: the level-2 limit and an application's client-address list +// both let it through. (Found on 2026-10-12; test/e2e sends two MAIL FROM on +// one connection.) func (s *session) MailFrom(from string, m *milter.Modifier) (milter.Response, error) { s.releaseReservations() // a previous transaction that ended without EOM/ABORT + if ip := clientAddr(m); ip != "" { + s.clientIP = ip + } s.from = cleanAddress(from) s.login = macro(m, "auth_authen") s.rcpts = nil @@ -113,13 +129,15 @@ func (s *session) Header(name, value string, m *milter.Modifier) (milter.Respons // Body fires at end-of-message, when the queue-id macro {i} is set and the // message is about to be committed to the queue. This is where the "queued" -// rows are written. We accept (this milter is done) without ever rejecting. +// rows are written. The reply is "continue", which at end-of-message lets the +// message through just as "accept" does - and, unlike it, leaves this session +// in place for the connection's next message (see MailFrom). func (s *session) Body(m *milter.Modifier) (milter.Response, error) { s.record(macro(m, "i")) // The rows are in the send log now, so the stored count sees this message // and its level-2 slots are no longer needed. s.releaseReservations() - return milter.RespAccept, nil + return milter.RespContinue, nil } // Abort ends the current transaction without an end-of-message (client RSET, or @@ -143,6 +161,21 @@ func macro(m *milter.Modifier, name string) string { return m.Macros["{"+name+"}"] } +// clientAddr is the client's address from the {client_addr} macro, or "" when +// Postfix did not send one that reads as an address. An IPv6 address may come +// in Sendmail's form, with "IPv6:" in front. +func clientAddr(m *milter.Modifier) string { + raw := strings.TrimSpace(macro(m, "client_addr")) + if len(raw) > 5 && strings.EqualFold(raw[:5], "IPv6:") { + raw = raw[5:] + } + ip := net.ParseIP(raw) + if ip == nil { + return "" + } + return ip.String() +} + // record writes one send-log row per recipient. Failures are logged, never // propagated: journalling must not affect mail acceptance (architecture.md § // Mail path). diff --git a/internal/milter/milter_test.go b/internal/milter/milter_test.go index c38ea18..381e1bd 100644 --- a/internal/milter/milter_test.go +++ b/internal/milter/milter_test.go @@ -166,8 +166,10 @@ func TestBodyAcceptsEvenWhenRecorderFails(t *testing.T) { if err != nil { t.Fatalf("Body returned error, must fail open: %v", err) } - if resp != milter.RespAccept { - t.Fatalf("Body response = %v, want Accept", resp) + // "Continue" at end-of-message lets the message through, and keeps the + // session for the connection's next message. + if resp != milter.RespContinue { + t.Fatalf("Body response = %v, want Continue", resp) } } @@ -389,21 +391,77 @@ func TestRateLimitFailsOpenOnCountError(t *testing.T) { } } -func TestRateLimitNoIPKeyDoesNotApply(t *testing.T) { +// A ceiling is on a domain or an application, not on an address: a message that +// arrives with no client address is counted like any other. It used to be let +// through, which is how the second MAIL FROM of a connection got past a limit. +func TestRateLimitAppliesWithoutAClientIP(t *testing.T) { rec := &fakeRecorder{ limits: map[string]store.RateLimit{ store.RateLimitScopeDomain + "|example.com": domainLimit(), }, counts: map[string]int64{store.RateLimitScopeDomain + "|example.com": 999}, } - // A session with no client IP (e.g. local submission) cannot be keyed. s := &session{rec: rec} resp, err := s.MailFrom("a@example.com", mods(map[string]string{"auth_authen": "app1"})) if err != nil { t.Fatalf("MailFrom: %v", err) } - if resp != milter.RespContinue { - t.Fatalf("no-IP session = %v, want Continue", resp) + if resp != milter.RespTempFail { + t.Fatalf("over the limit with no client IP = %v, want TempFail", resp) + } +} + +// The milter library hands the milter a fresh session after every reply that +// is not "continue", and Postfix sends Connect once per connection. Such a +// session knows the client only from the {client_addr} macro of MAIL FROM, and +// that has to be enough for an application's client-address list. +func TestAuthIPRestrictHoldsForASessionThatNeverSawConnect(t *testing.T) { + rec := &fakeRecorder{apps: map[string]store.Application{ + "app1": {Login: "app1", AuthIPRestrict: true, AuthAllowedIPs: []string{"198.51.100.1"}}, + }} + for name, c := range map[string]struct { + macros map[string]string + want milter.Response + }{ + "an unlisted address in the macro": {map[string]string{"auth_authen": "app1", "{client_addr}": limitIP}, milter.RespTempFail}, + "the listed address in the macro": {map[string]string{"auth_authen": "app1", "{client_addr}": "198.51.100.1"}, milter.RespContinue}, + // No address at all is not a pass: a list an unknown address satisfies is no list. + "no client address": {map[string]string{"auth_authen": "app1"}, milter.RespTempFail}, + "not an address": {map[string]string{"auth_authen": "app1", "{client_addr}": "unknown"}, milter.RespTempFail}, + } { + s := &session{rec: rec} // no Connect + resp, err := s.MailFrom("a@example.com", mods(c.macros)) + if err != nil || resp != c.want { + t.Errorf("%s: %v, %v; want %v", name, resp, err, c.want) + } + } + // An application with no list is not held up by a missing address. + open := &fakeRecorder{apps: map[string]store.Application{"app1": {Login: "app1"}}} + s := &session{rec: open} + if resp, _ := s.MailFrom("a@example.com", mods(map[string]string{"auth_authen": "app1"})); resp != milter.RespContinue { + t.Errorf("no list, no address: %v, want Continue", resp) + } +} + +// The macro wins over what Connect said, and reads both spellings of IPv6. +func TestClientAddrFromTheMailMacro(t *testing.T) { + for raw, want := range map[string]string{ + "203.0.113.7": "203.0.113.7", "2001:db8::1": "2001:db8::1", "IPv6:2001:DB8::1": "2001:db8::1", + "ipv6:::1": "::1", "": "", "unknown": "", "IPv6:": "", + } { + if got := clientAddr(mods(map[string]string{"{client_addr}": raw})); got != want { + t.Errorf("clientAddr(%q) = %q, want %q", raw, got, want) + } + } + s := &session{rec: &fakeRecorder{}} + _, _ = s.Connect("h", "tcp4", 0, net.ParseIP("192.0.2.1"), mods(nil)) + _, _ = s.MailFrom("a@example.com", mods(map[string]string{"client_addr": "192.0.2.2"})) + if s.clientIP != "192.0.2.2" { + t.Errorf("clientIP = %q, want the macro's", s.clientIP) + } + _, _ = s.MailFrom("a@example.com", mods(nil)) + if s.clientIP != "192.0.2.2" { + t.Errorf("clientIP = %q after a MAIL FROM with no macro, want it kept", s.clientIP) } } diff --git a/internal/milter/ratelimit.go b/internal/milter/ratelimit.go index d713936..410b1a7 100644 --- a/internal/milter/ratelimit.go +++ b/internal/milter/ratelimit.go @@ -26,10 +26,9 @@ import ( // reaches the send log (or is abandoned) — see inflight for why the stored // count alone is not enough. func (s *session) overLimit() bool { - if s.clientIP == "" { - return false // no client IP to key on; level-2 does not apply - } - + // A ceiling is on a domain or an application, whatever address the mail + // comes from, so the client address is not asked for here. It used to be, + // and a message that arrived without one was let through uncounted. if s.login != "" { rl, ok, err := s.rec.RateLimit(store.RateLimitScopeApp, s.login) if err != nil { diff --git a/test/e2e/negative_test.go b/test/e2e/negative_test.go index 7dd39d8..76253c7 100644 --- a/test/e2e/negative_test.go +++ b/test/e2e/negative_test.go @@ -52,6 +52,17 @@ func testLevel2RateLimit(t *testing.T, sc *scenario) { second.dialErr, second.authErr, second.rcptErr, second.dataErr) } + // The limit holds for every message of a connection, not for its first + // only. The milter library hands the milter a fresh session after any reply + // that is not "continue" - the refusal above is one - and Postfix does not + // introduce the client again, so a second MAIL FROM on the same connection + // used to arrive with no client address and was let through (found by the + // technical review of 2026-10-12; the same hole let an application past its + // client-address list). + if errs := mailTwiceOnOneConnection(login, password, from); errs[0] == nil || errs[1] == nil { + t.Fatalf("over the level-2 limit, two MAIL FROM on one connection were answered %v and %v, want both refused", errs[0], errs[1]) + } + if err := waitFor("a rejected row for l2app in the send log", 15*time.Second, 500*time.Millisecond, func() (bool, error) { rows, err := sc.panel.sendLogRows(senderDomain, login) if err != nil { diff --git a/test/e2e/smtp_client.go b/test/e2e/smtp_client.go index e89f867..55a53cf 100644 --- a/test/e2e/smtp_client.go +++ b/test/e2e/smtp_client.go @@ -140,3 +140,35 @@ func buildMessage(a sendAttempt) string { a.from, a.to, a.subject, time.Now().UTC().Format(time.RFC1123Z), a.body, ) } + +// mailTwiceOnOneConnection authenticates once and sends MAIL FROM twice on the +// same connection, with a RSET between, and returns how each was answered. No +// message is sent: this is about what the server says to the second +// transaction of a connection whose first was refused. +func mailTwiceOnOneConnection(login, password, from string) [2]error { + var out [2]error + conn, err := tls.DialWithDialer(&net.Dialer{Timeout: 10 * time.Second}, "tcp", smtpsAddr, &tls.Config{ + InsecureSkipVerify: true, + ServerName: selfpostHostname, + }) + if err != nil { + return [2]error{nil, nil} + } + c, err := smtp.NewClient(conn, selfpostHostname) + if err != nil { + _ = conn.Close() + return [2]error{nil, nil} + } + defer c.Close() + if err := c.Hello("e2e-client.e2e.test"); err != nil { + return [2]error{nil, nil} + } + if err := c.Auth(smtp.PlainAuth("", login, password, selfpostHostname)); err != nil { + return [2]error{nil, nil} + } + out[0] = c.Mail(from) + _ = c.Reset() + out[1] = c.Mail(from) + _ = c.Quit() + return out +}