accept incoming DMARC and TLS reports with reporting addresses containing catchall separator(s)

Such as "-" when addresses are dmarc-reports@ and tls-reports@.

Existing configuration files can have these combinations. We don't allow them
to be created through the webadmin interface, as this is a likely source of
confusion about how addresses will be matched. We already didn't allow regular
addresses containing catchall separators.

config/config.go

@@ -329,7 +329,7 @@ type DMARC struct {
 	Account   string `sconf-doc:"Account to deliver to."`
 	Mailbox   string `sconf-doc:"Mailbox to deliver to, e.g. DMARC."`
 
-	ParsedLocalpart smtp.Localpart `sconf:"-"`
+	ParsedLocalpart smtp.Localpart `sconf:"-"` // Lower-case if case-sensitivity is not configured for domain. Not "canonical" for catchall separators for backwards compatibility.
 	DNSDomain       dns.Domain     `sconf:"-"` // Effective domain, always set based on Domain field or Domain where this is configured.
 }
 
@@ -347,7 +347,7 @@ type TLSRPT struct {
 	Account   string `sconf-doc:"Account to deliver to."`
 	Mailbox   string `sconf-doc:"Mailbox to deliver to, e.g. TLSRPT."`
 
-	ParsedLocalpart smtp.Localpart `sconf:"-"`
+	ParsedLocalpart smtp.Localpart `sconf:"-"` // Lower-case if case-sensitivity is not configured for domain. Not "canonical" for catchall separators for backwards compatibility.
 	DNSDomain       dns.Domain     `sconf:"-"` // Effective domain, always set based on Domain field or Domain where this is configured.
 }
 

mox-/config.go

@@ -1747,6 +1747,8 @@ func prepareDynamicConfig(ctx context.Context, log mlog.Log, dynamicPath string,
 		if _, ok := c.Accounts[dmarc.Account]; !ok {
 			addDomainErrorf("DMARC account %q does not exist", dmarc.Account)
 		}
+
+		// Note: For backwards compabilitiy, DMARC reporting localparts can contain catchall separators.
 		lp, err := smtp.ParseLocalpart(dmarc.Localpart)
 		if err != nil {
 			addDomainErrorf("invalid DMARC localpart %q: %s", dmarc.Localpart, err)
@@ -1760,9 +1762,13 @@ func prepareDynamicConfig(ctx context.Context, log mlog.Log, dynamicPath string,
 			addrdom, err = dns.ParseDomain(dmarc.Domain)
 			if err != nil {
 				addDomainErrorf("DMARC domain %q: %s", dmarc.Domain, err)
-			} else if _, ok := c.Domains[addrdom.Name()]; !ok {
+			} else if adomain, ok := c.Domains[addrdom.Name()]; !ok {
 				addDomainErrorf("unknown domain %q for DMARC address", addrdom)
+			} else if !adomain.LocalpartCaseSensitive {
+				lp = smtp.Localpart(strings.ToLower(string(lp)))
 			}
+		} else if !domain.LocalpartCaseSensitive {
+			lp = smtp.Localpart(strings.ToLower(string(lp)))
 		}
 		if addrdom == domain.Domain {
 			domainHasAddress[addrdom.Name()] = true
@@ -1793,6 +1799,8 @@ func prepareDynamicConfig(ctx context.Context, log mlog.Log, dynamicPath string,
 		if _, ok := c.Accounts[tlsrpt.Account]; !ok {
 			addDomainErrorf("TLSRPT account %q does not exist", tlsrpt.Account)
 		}
+
+		// Note: For backwards compabilitiy, TLS reporting localparts can contain catchall separators.
 		lp, err := smtp.ParseLocalpart(tlsrpt.Localpart)
 		if err != nil {
 			addDomainErrorf("invalid TLSRPT localpart %q: %s", tlsrpt.Localpart, err)
@@ -1807,9 +1815,13 @@ func prepareDynamicConfig(ctx context.Context, log mlog.Log, dynamicPath string,
 			addrdom, err = dns.ParseDomain(tlsrpt.Domain)
 			if err != nil {
 				addDomainErrorf("TLSRPT domain %q: %s", tlsrpt.Domain, err)
-			} else if _, ok := c.Domains[addrdom.Name()]; !ok {
+			} else if adomain, ok := c.Domains[addrdom.Name()]; !ok {
 				addDomainErrorf("unknown domain %q for TLSRPT address", tlsrpt.Domain)
+			} else if !adomain.LocalpartCaseSensitive {
+				lp = smtp.Localpart(strings.ToLower(string(lp)))
 			}
+		} else if !domain.LocalpartCaseSensitive {
+			lp = smtp.Localpart(strings.ToLower(string(lp)))
 		}
 		if addrdom == domain.Domain {
 			domainHasAddress[addrdom.Name()] = true

mox-/lookup.go

@@ -85,17 +85,49 @@ func LookupAddress(localpart smtp.Localpart, domain dns.Domain, allowPostmaster,
 	return accAddr.Account, nil, canonical, accAddr.Destination, nil
 }
 
+// lp and rlp are both lower-case when domain localparts aren't case sensitive.
+func matchReportingSeparators(lp, rlp smtp.Localpart, d config.Domain) bool {
+	lps := string(lp)
+	rlps := string(rlp)
+
+	if !strings.HasPrefix(lps, rlps) {
+		return false
+	}
+	if len(lps) == len(rlps) {
+		return true
+	}
+	rem := lps[len(rlps):]
+	for _, sep := range d.LocalpartCatchallSeparatorsEffective {
+		if strings.HasPrefix(rem, sep) {
+			return true
+		}
+	}
+	return false
+}
+
 // CanonicalLocalpart returns the canonical localpart, removing optional catchall
 // separators, and optionally lower-casing the string.
+// The DMARC and TLS reporting addresses are treated specially, they may contain a
+// localpart catchall separator for historic configurations (not for new
+// configurations). We try to match them first, still taking additional localpart
+// catchall separators into account.
 func CanonicalLocalpart(localpart smtp.Localpart, d config.Domain) smtp.Localpart {
+	if !d.LocalpartCaseSensitive {
+		localpart = smtp.Localpart(strings.ToLower(string(localpart)))
+	}
+
+	if d.DMARC != nil && matchReportingSeparators(localpart, d.DMARC.ParsedLocalpart, d) {
+		return d.DMARC.ParsedLocalpart
+	}
+	if d.TLSRPT != nil && matchReportingSeparators(localpart, d.TLSRPT.ParsedLocalpart, d) {
+		return d.TLSRPT.ParsedLocalpart
+	}
+
 	for _, sep := range d.LocalpartCatchallSeparatorsEffective {
 		t := strings.SplitN(string(localpart), sep, 2)
 		localpart = smtp.Localpart(t[0])
 	}
 
-	if !d.LocalpartCaseSensitive {
-		localpart = smtp.Localpart(strings.ToLower(string(localpart)))
-	}
 	return localpart
 }
 

smtpserver/server_test.go

@@ -1094,13 +1094,12 @@ func TestDMARCReport(t *testing.T) {
 	ts := newTestServer(t, filepath.FromSlash("../testdata/smtp/dmarcreport/mox.conf"), resolver)
 	defer ts.close()
 
-	run := func(report string, n int) {
+	run := func(rcptTo, report string, n int) {
 		t.Helper()
 		ts.run(func(client *smtpclient.Client) {
 			t.Helper()
 
 			mailFrom := "remote@example.org"
-			rcptTo := "mjl@mox.example"
 
 			msgb := &bytes.Buffer{}
 			_, xerr := fmt.Fprintf(msgb, "From: %s\r\nTo: %s\r\nSubject: dmarc report\r\nMIME-Version: 1.0\r\nContent-Type: text/xml\r\n\r\n", mailFrom, rcptTo)
@@ -1121,13 +1120,33 @@ func TestDMARCReport(t *testing.T) {
 		})
 	}
 
-	run(dmarcReport, 0)
+	n := 0
-	run(strings.ReplaceAll(dmarcReport, "xmox.nl", "mox.example"), 1)
+	run("dmarc-reports@mox.example", dmarcReport, 0) // Wrong domain in report.
+
+	report := strings.ReplaceAll(dmarcReport, "xmox.nl", "mox.example")
+	n++
+	run("dmarc-reports@mox.example", report, n)
 
 	// We always store as an evaluation, but as optional for reports.
 	evals := checkEvaluationCount(t, 2)
 	tcompare(t, evals[0].Optional, true)
 	tcompare(t, evals[1].Optional, true)
+
+	// Not a dmarc recipient, delivery should succeed.
+	run("mjl@mox.example", report, n)
+	run("mjl-test@mox.example", report, n)
+	run("mjl+test@mox.example", report, n)
+	// Likewise, address that is prefix of reporting address.
+	run("dmarc@mox.example", report, n)
+	run("Dmarc-test@mox.example", report, n)
+	run("dmarc+test@mox.example", report, n)
+
+	// Localpart catchall separators work for dmarc reporting addresses too.
+	n++
+	run("Dmarc-reports-test@mox.example", report, n)
+
+	n++
+	run("dmarc-Reports+test@mox.example", report, n)
 }
 
 const dmarcReport = `<?xml version="1.0" encoding="UTF-8" ?>
@@ -1245,17 +1264,39 @@ func TestTLSReport(t *testing.T) {
 		})
 	}
 
-	const tlsrpt = `{"organization-name":"Example.org","date-range":{"start-datetime":"2022-01-07T00:00:00Z","end-datetime":"2022-01-07T23:59:59Z"},"contact-info":"tlsrpt@example.org","report-id":"1","policies":[{"policy":{"policy-type":"no-policy-found","policy-domain":"xmox.nl"},"summary":{"total-successful-session-count":1,"total-failure-session-count":0}}]}`
+	tlsrpt := `{"organization-name":"Example.org","date-range":{"start-datetime":"2022-01-07T00:00:00Z","end-datetime":"2022-01-07T23:59:59Z"},"contact-info":"tlsrpt@example.org","report-id":"1","policies":[{"policy":{"policy-type":"no-policy-found","policy-domain":"xmox.nl"},"summary":{"total-successful-session-count":1,"total-failure-session-count":0}}]}`
 
-	run("mjl@mox.example", tlsrpt, 0)
+	n := 0
-	run("mjl@mox.example", strings.ReplaceAll(tlsrpt, "xmox.nl", "mox.example"), 1)
+	run("tls-reports@mox.example", tlsrpt, n) // Wrong domain in report.
-	run("mjl@mailhost.mox.example", strings.ReplaceAll(tlsrpt, "xmox.nl", "mailhost.mox.example"), 2)
+
+	tlsrptdom := strings.ReplaceAll(tlsrpt, "xmox.nl", "mox.example")
+	n++
+	run("tls-reports@mox.example", tlsrptdom, n)
+
+	tlsrpthost := strings.ReplaceAll(tlsrpt, "xmox.nl", "mailhost.mox.example")
+	n++
+	run("tls-reports@mailhost.mox.example", tlsrpthost, n)
 
 	// We always store as an evaluation, but as optional for reports.
 	evals := checkEvaluationCount(t, 3)
 	tcompare(t, evals[0].Optional, true)
 	tcompare(t, evals[1].Optional, true)
 	tcompare(t, evals[2].Optional, true)
+
+	// Catchall separators work for reporting address too.
+	n++
+	run("Tls-reports+more@mox.example", tlsrptdom, n)
+	n++
+	run("tls-Reports-more@mox.example", tlsrptdom, n)
+
+	// Non-reporting addresses, mail accepted, but not as report.
+	run("mjl@mox.example", tlsrptdom, n)
+	run("Mjl-other@mox.example", tlsrptdom, n)
+	run("mjl+other@mox.example", tlsrptdom, n)
+	// Likewise, address that is prefix of reporting address.
+	run("tls@mox.example", tlsrptdom, n)
+	run("Tls-other@mox.example", tlsrptdom, n)
+	run("tls+other@mox.example", tlsrptdom, n)
 }
 
 func TestRatelimitConnectionrate(t *testing.T) {

testdata/smtp/dmarcreport/domains.conf

@@ -1,11 +1,19 @@
 Domains:
 	mox.example:
 		DMARC:
-			Account: mjl
+			Account: dmarc
-			Localpart: mjl
+			# Localpart contains a catchall separator
+			Localpart: dmarc-reports
 			Mailbox: DMARC
+		LocalpartCatchallSeparators:
+			- +
+			- -
 Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
 			mjl@mox.example: nil
+	dmarc:
+		Domain: mox.example
+		Destinations:
+			dmarc@mox.example: nil

testdata/smtp/tlsrpt/domains.conf

@@ -2,10 +2,18 @@ Domains:
 	mox.example:
 		TLSRPT:
 			Account: mjl
-			Localpart: mjl
+			# Localpart contains a catchall separator
+			Localpart: tls-reports
 			Mailbox: TLSRPT
+		LocalpartCatchallSeparators:
+			- +
+			- -
 Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
 			mjl@mox.example: nil
+	tls:
+		Domain: mox.example
+		Destinations:
+			tls@mox.example: nil

testdata/smtp/tlsrpt/mox.conf

@@ -10,4 +10,4 @@ Listeners:
 HostTLSRPT:
 	Account: mjl
 	Mailbox: TLSRPT
-	Localpart: mjl
+	Localpart: tls-reports

webadmin/admin.go

@@ -2535,6 +2535,23 @@ func (Admin) DomainClientSettingsDomainSave(ctx context.Context, domainName, cli
 // settings for a domain.
 func (Admin) DomainLocalpartConfigSave(ctx context.Context, domainName string, localpartCatchallSeparators []string, localpartCaseSensitive bool) {
 	err := admin.DomainSave(ctx, domainName, func(domain *config.Domain) error {
+		// We don't allow introducing new catchall separators that are used in DMARC/TLS
+		// reporting. Can occur in existing configs for backwards compatibility.
+		containsSep := func(seps []string) bool {
+			for _, sep := range seps {
+				if domain.DMARC != nil && strings.Contains(domain.DMARC.Localpart, sep) {
+					return true
+				}
+				if domain.TLSRPT != nil && strings.Contains(domain.TLSRPT.Localpart, sep) {
+					return true
+				}
+			}
+			return false
+		}
+		if !containsSep(domain.LocalpartCatchallSeparatorsEffective) && containsSep(localpartCatchallSeparators) {
+			xusererrorf(ctx, "cannot add localpart catchall separators that are used in dmarc and/or tls reporting addresses, change reporting addresses first")
+		}
+
 		domain.LocalpartCatchallSeparatorsEffective = localpartCatchallSeparators
 		// If there is a single separator, we prefer the non-list form, it's easier to
 		// read/edit and should suffice for most setups.
@@ -2557,6 +2574,17 @@ func (Admin) DomainLocalpartConfigSave(ctx context.Context, domainName string, l
 // disabled.
 func (Admin) DomainDMARCAddressSave(ctx context.Context, domainName, localpart, domain, account, mailbox string) {
 	err := admin.DomainSave(ctx, domainName, func(d *config.Domain) error {
+		// DMARC reporting addresses can contain the localpart catchall separator(s) for
+		// backwards compability (hence not enforced when parsing the config files), but we
+		// don't allow creating them.
+		if d.DMARC == nil || d.DMARC.Localpart != localpart {
+			for _, sep := range d.LocalpartCatchallSeparatorsEffective {
+				if strings.Contains(localpart, sep) {
+					xusererrorf(ctx, "dmarc reporting address cannot contain catchall separator %q in localpart (%q)", sep, localpart)
+				}
+			}
+		}
+
 		if localpart == "" {
 			d.DMARC = nil
 		} else {
@@ -2577,6 +2605,17 @@ func (Admin) DomainDMARCAddressSave(ctx context.Context, domainName, localpart,
 // disabled.
 func (Admin) DomainTLSRPTAddressSave(ctx context.Context, domainName, localpart, domain, account, mailbox string) {
 	err := admin.DomainSave(ctx, domainName, func(d *config.Domain) error {
+		// TLS reporting addresses can contain the localpart catchall separator(s) for
+		// backwards compability (hence not enforced when parsing the config files), but we
+		// don't allow creating them.
+		if d.TLSRPT == nil || d.TLSRPT.Localpart != localpart {
+			for _, sep := range d.LocalpartCatchallSeparatorsEffective {
+				if strings.Contains(localpart, sep) {
+					xusererrorf(ctx, "tls reporting address cannot contain catchall separator %q in localpart (%q)", sep, localpart)
+				}
+			}
+		}
+
 		if localpart == "" {
 			d.TLSRPT = nil
 		} else {

webadmin/admin_test.go

@@ -279,17 +279,25 @@ func TestAdmin(t *testing.T) {
 
 	api.DomainLocalpartConfigSave(ctxbg, "mox.example", []string{"-"}, true)
 	tneedErrorCode(t, "user:error", func() { api.DomainLocalpartConfigSave(ctxbg, "bogus.example", nil, false) })
-	api.DomainLocalpartConfigSave(ctxbg, "mox.example", nil, false) // Restore.
 
-	api.DomainDMARCAddressSave(ctxbg, "mox.example", "dmarcreports", "", "mjl", "DMARC")
+	api.DomainDMARCAddressSave(ctxbg, "mox.example", "dmarc+reports", "", "mjl", "DMARC")
+	// Catchall separator, bad domain, bad account.
+	tneedErrorCode(t, "user:error", func() { api.DomainDMARCAddressSave(ctxbg, "mox.example", "dmarc-reports", "", "mjl", "DMARC") })
 	tneedErrorCode(t, "user:error", func() { api.DomainDMARCAddressSave(ctxbg, "bogus.example", "dmarcreports", "", "mjl", "DMARC") })
 	tneedErrorCode(t, "user:error", func() { api.DomainDMARCAddressSave(ctxbg, "mox.example", "dmarcreports", "", "bogus", "DMARC") })
-	api.DomainDMARCAddressSave(ctxbg, "mox.example", "", "", "", "") // Restore.
 
-	api.DomainTLSRPTAddressSave(ctxbg, "mox.example", "tlsreports", "", "mjl", "TLSRPT")
+	api.DomainTLSRPTAddressSave(ctxbg, "mox.example", "tls+reports", "", "mjl", "TLSRPT")
+	// Catchall separator, bad domain, bad account.
+	tneedErrorCode(t, "user:error", func() { api.DomainTLSRPTAddressSave(ctxbg, "mox.example", "tls-reports", "", "mjl", "TLSRPT") })
 	tneedErrorCode(t, "user:error", func() { api.DomainTLSRPTAddressSave(ctxbg, "bogus.example", "tlsreports", "", "mjl", "TLSRPT") })
 	tneedErrorCode(t, "user:error", func() { api.DomainTLSRPTAddressSave(ctxbg, "mox.example", "tlsreports", "", "bogus", "TLSRPT") })
+
+	// DMARC/TLS reporting addresses contain separator.
+	tneedErrorCode(t, "user:error", func() { api.DomainLocalpartConfigSave(ctxbg, "mox.example", []string{"+"}, true) })
+
+	api.DomainDMARCAddressSave(ctxbg, "mox.example", "", "", "", "")  // Restore.
 	api.DomainTLSRPTAddressSave(ctxbg, "mox.example", "", "", "", "") // Restore.
+	api.DomainLocalpartConfigSave(ctxbg, "mox.example", nil, false)   // Restore.
 
 	// todo: cannot enable mta-sts because we have no listener, which would require a tls cert for the domain.
 	// api.DomainMTASTSSave(ctxbg, "mox.example", "id0", mtasts.ModeEnforce, time.Hour, []string{"mail.mox.example"})

webadmin/api.json

@@ -3693,7 +3693,7 @@
 				},
 				{
 					"Name": "ParsedLocalpart",
-					"Docs": "",
+					"Docs": "Lower-case if case-sensitivity is not configured for domain. Not \"canonical\" for catchall separators for backwards compatibility.",
 					"Typewords": [
 						"Localpart"
 					]
@@ -3776,7 +3776,7 @@
 				},
 				{
 					"Name": "ParsedLocalpart",
-					"Docs": "",
+					"Docs": "Lower-case if case-sensitivity is not configured for domain. Not \"canonical\" for catchall separators for backwards compatibility.",
 					"Typewords": [
 						"Localpart"
 					]

webadmin/api.ts

@@ -309,7 +309,7 @@ export interface DMARC {
 	Domain: string
 	Account: string
 	Mailbox: string
-	ParsedLocalpart: Localpart
+	ParsedLocalpart: Localpart  // Lower-case if case-sensitivity is not configured for domain. Not "canonical" for catchall separators for backwards compatibility.
 	DNSDomain: Domain  // Effective domain, always set based on Domain field or Domain where this is configured.
 }
 
@@ -325,7 +325,7 @@ export interface TLSRPT {
 	Domain: string
 	Account: string
 	Mailbox: string
-	ParsedLocalpart: Localpart
+	ParsedLocalpart: Localpart  // Lower-case if case-sensitivity is not configured for domain. Not "canonical" for catchall separators for backwards compatibility.
 	DNSDomain: Domain  // Effective domain, always set based on Domain field or Domain where this is configured.
 }