add flag to ruleset that indicates a message is forwarded, slightly modifying how junk analysis is done

part of PR #50 by bobobo1618

smtpserver/analyze.go

@@ -92,6 +92,30 @@ func analyze(ctx context.Context, log *mlog.Log, resolver dns.Resolver, d delive
 		}
 	}
 
+	// For forwarded messages, we have different junk analysis. We don't reject for
+	// failing DMARC, and we clear fields that could implicate the forwarding mail
+	// server during future classifications on incoming messages (the forwarding mail
+	// server isn't responsible for the message).
+	if rs != nil && rs.IsForward {
+		d.dmarcUse = false
+		d.m.IsForward = true
+		d.m.RemoteIPMasked1 = ""
+		d.m.RemoteIPMasked2 = ""
+		d.m.RemoteIPMasked3 = ""
+		d.m.OrigEHLODomain = d.m.EHLODomain
+		d.m.EHLODomain = ""
+		d.m.MailFromDomain = "" // Still available in MailFrom.
+		d.m.OrigDKIMDomains = d.m.DKIMDomains
+		dkimdoms := []string{}
+		for _, dom := range d.m.DKIMDomains {
+			if dom != rs.VerifiedDNSDomain.Name() {
+				dkimdoms = append(dkimdoms, dom)
+			}
+		}
+		d.m.DKIMDomains = dkimdoms
+		log.Info("forwarded message, clearing identifying signals of forwarding mail server")
+	}
+
 	reject := func(code int, secode string, errmsg string, err error, reason string) analysis {
 		accept := false
 		if rs != nil && rs.AcceptRejectsToMailbox != "" {

smtpserver/reputation.go

@@ -338,17 +338,22 @@ func reputation(tx *bstore.Tx, log *mlog.Log, m *store.Message) (rjunk *bool, rc
 
 	// IP-based. A wider mask needs more messages to be conclusive.
 	// We require the resulting signal to be strong, i.e. likely ham or likely spam.
-	q := messageQuery(&store.Message{RemoteIPMasked1: m.RemoteIPMasked1}, year/4, 50)
-	msgs := xmessageList(q, "ip1")
-	need := 2
-	method := methodIP1
-	if len(msgs) == 0 {
+	var msgs []store.Message
+	var need int
+	var method reputationMethod
+	if m.RemoteIPMasked1 != "" {
+		q := messageQuery(&store.Message{RemoteIPMasked1: m.RemoteIPMasked1}, year/4, 50)
+		msgs = xmessageList(q, "ip1")
+		need = 2
+		method = methodIP1
+	}
+	if len(msgs) == 0 && m.RemoteIPMasked2 != "" {
 		q := messageQuery(&store.Message{RemoteIPMasked2: m.RemoteIPMasked2}, year/4, 50)
 		msgs = xmessageList(q, "ip2")
 		need = 5
 		method = methodIP2
 	}
-	if len(msgs) == 0 {
+	if len(msgs) == 0 && m.RemoteIPMasked3 != "" {
 		q := messageQuery(&store.Message{RemoteIPMasked3: m.RemoteIPMasked3}, year/4, 50)
 		msgs = xmessageList(q, "ip3")
 		need = 10

smtpserver/server.go

@@ -2291,6 +2291,7 @@ func (c *conn) deliver(ctx context.Context, recvHdrFor func(string) string, msgW
 			}
 
 			// todo future: make these configurable
+			// todo: should we have a limit for forwarded messages? they are stored with empty RemoteIPMasked*
 
 			const day = 24 * time.Hour
 			checkCount(store.Message{RemoteIPMasked1: ipmasked1}, time.Minute, limitIPMasked1MessagesPerMinute)
@@ -2412,6 +2413,7 @@ func (c *conn) deliver(ctx context.Context, recvHdrFor func(string) string, msgW
 			continue
 		}
 
+		delayFirstTime := true
 		if a.dmarcReport != nil {
 			// todo future: add rate limiting to prevent DoS attacks. ../rfc/7489:2570
 			if err := dmarcdb.AddReport(ctx, a.dmarcReport, msgFrom.Domain); err != nil {
@@ -2419,6 +2421,7 @@ func (c *conn) deliver(ctx context.Context, recvHdrFor func(string) string, msgW
 			} else {
 				log.Info("dmarc report processed")
 				m.Flags.Seen = true
+				delayFirstTime = false
 			}
 		}
 		if a.tlsReport != nil {
@@ -2428,13 +2431,14 @@ func (c *conn) deliver(ctx context.Context, recvHdrFor func(string) string, msgW
 			} else {
 				log.Info("tlsrpt report processed")
 				m.Flags.Seen = true
+				delayFirstTime = false
 			}
 		}
 
-		// If not dmarc or tls report (Seen set above), and this is a first-time sender,
-		// wait before actually delivering. If this turns out to be a spammer, we've kept
-		// one of their connections busy.
-		if !m.Flags.Seen && a.reason == reasonNoBadSignals && c.firstTimeSenderDelay > 0 {
+		// If a forwarded message and this is a first-time sender, wait before actually
+		// delivering. If this turns out to be a spammer, we've kept one of their
+		// connections busy.
+		if delayFirstTime && !m.IsForward && a.reason == reasonNoBadSignals && c.firstTimeSenderDelay > 0 {
 			log.Debug("delaying before delivering from sender without reputation", mlog.Field("delay", c.firstTimeSenderDelay))
 			mox.Sleep(mox.Context, c.firstTimeSenderDelay)
 		}

smtpserver/server_test.go

@@ -506,6 +506,134 @@ func TestSpam(t *testing.T) {
 	})
 }
 
+// Test accept/reject with forwarded messages, DMARC ignored, no IP/EHLO/MAIL
+// FROM-based reputation.
+func TestForward(t *testing.T) {
+	// Do a run without forwarding, and with.
+	check := func(forward bool) {
+
+		resolver := &dns.MockResolver{
+			A: map[string][]string{
+				"bad.example.":     {"127.0.0.1"},  // For mx check.
+				"good.example.":    {"127.0.0.1"},  // For mx check.
+				"forward.example.": {"127.0.0.10"}, // For mx check.
+			},
+			TXT: map[string][]string{
+				"bad.example.":            {"v=spf1 ip4:127.0.0.1 -all"},
+				"good.example.":           {"v=spf1 ip4:127.0.0.1 -all"},
+				"forward.example.":        {"v=spf1 ip4:127.0.0.10 -all"},
+				"_dmarc.bad.example.":     {"v=DMARC1;p=reject"},
+				"_dmarc.good.example.":    {"v=DMARC1;p=reject"},
+				"_dmarc.forward.example.": {"v=DMARC1;p=reject"},
+			},
+			PTR: map[string][]string{
+				"127.0.0.10": {"forward.example."}, // For iprev check.
+			},
+		}
+		rcptTo := "mjl3@mox.example"
+		if !forward {
+			// For SPF and DMARC pass, otherwise the test ends quickly.
+			resolver.TXT["bad.example."] = []string{"v=spf1 ip4:127.0.0.10 -all"}
+			resolver.TXT["good.example."] = []string{"v=spf1 ip4:127.0.0.10 -all"}
+			rcptTo = "mjl@mox.example" // Without IsForward rule.
+		}
+
+		ts := newTestServer(t, "../testdata/smtp/junk/mox.conf", resolver)
+		defer ts.close()
+
+		var msgBad = strings.ReplaceAll(`From: <remote@bad.example>
+To: <mjl3@mox.example>
+Subject: test
+Message-Id: <bad@example.org>
+
+test email
+`, "\n", "\r\n")
+		var msgOK = strings.ReplaceAll(`From: <remote@good.example>
+To: <mjl3@mox.example>
+Subject: other
+Message-Id: <good@example.org>
+
+unrelated message.
+`, "\n", "\r\n")
+		var msgOK2 = strings.ReplaceAll(`From: <other@forward.example>
+To: <mjl3@mox.example>
+Subject: non-forward
+Message-Id: <regular@example.org>
+
+happens to come from forwarding mail server.
+`, "\n", "\r\n")
+
+		// Deliver forwarded messages, then classify as junk. Normally enough to treat
+		// other unrelated messages from IP as junk, but not for forwarded messages.
+		ts.run(func(err error, client *smtpclient.Client) {
+			tcheck(t, err, "connect")
+
+			mailFrom := "remote@forward.example"
+			if !forward {
+				mailFrom = "remote@bad.example"
+			}
+
+			for i := 0; i < 10; i++ {
+				err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(msgBad)), strings.NewReader(msgBad), false, false)
+				tcheck(t, err, "deliver message")
+			}
+
+			n, err := bstore.QueryDB[store.Message](ctxbg, ts.acc.DB).UpdateFields(map[string]any{"Junk": true, "MsgFromValidated": true})
+			tcheck(t, err, "marking messages as junk")
+			tcompare(t, n, 10)
+
+			// Next delivery will fail, with negative "message From" signal.
+			err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(msgBad)), strings.NewReader(msgBad), false, false)
+			var cerr smtpclient.Error
+			if err == nil || !errors.As(err, &cerr) || cerr.Code != smtp.C451LocalErr {
+				t.Fatalf("delivery by bad sender, got err %v, expected smtpclient.Error with code %d", err, smtp.C451LocalErr)
+			}
+		})
+
+		// Delivery from different "message From" without reputation, but from same
+		// forwarding email server, should succeed under forwarding, not as regular sending
+		// server.
+		ts.run(func(err error, client *smtpclient.Client) {
+			tcheck(t, err, "connect")
+
+			mailFrom := "remote@forward.example"
+			if !forward {
+				mailFrom = "remote@good.example"
+			}
+
+			err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(msgOK)), strings.NewReader(msgOK), false, false)
+			if forward {
+				tcheck(t, err, "deliver")
+			} else {
+				var cerr smtpclient.Error
+				if err == nil || !errors.As(err, &cerr) || cerr.Code != smtp.C451LocalErr {
+					t.Fatalf("delivery by bad ip, got err %v, expected smtpclient.Error with code %d", err, smtp.C451LocalErr)
+				}
+			}
+		})
+
+		// Delivery from forwarding server that isn't a forward should get same treatment.
+		ts.run(func(err error, client *smtpclient.Client) {
+			tcheck(t, err, "connect")
+
+			mailFrom := "other@forward.example"
+
+			err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(msgOK2)), strings.NewReader(msgOK2), false, false)
+			if forward {
+				tcheck(t, err, "deliver")
+			} else {
+				var cerr smtpclient.Error
+				if err == nil || !errors.As(err, &cerr) || cerr.Code != smtp.C451LocalErr {
+					t.Fatalf("delivery by bad ip, got err %v, expected smtpclient.Error with code %d", err, smtp.C451LocalErr)
+				}
+			}
+		})
+	}
+
+	check(true)
+	check(false)
+}
+
 // Messages that we sent to, that have passing DMARC, but that are otherwise spammy, should be accepted.
 func TestDMARCSent(t *testing.T) {
 	resolver := &dns.MockResolver{