From 794ef75d173dd584a5c3618c0df9196071c26c9e Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Fri, 18 Apr 2025 12:34:07 +0200 Subject: [PATCH] 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 | 4 +- mox-/config.go | 16 +++++++- mox-/lookup.go | 38 +++++++++++++++-- smtpserver/server_test.go | 57 ++++++++++++++++++++++---- testdata/smtp/dmarcreport/domains.conf | 12 +++++- testdata/smtp/tlsrpt/domains.conf | 10 ++++- testdata/smtp/tlsrpt/mox.conf | 2 +- webadmin/admin.go | 39 ++++++++++++++++++ webadmin/admin_test.go | 16 ++++++-- webadmin/api.json | 4 +- webadmin/api.ts | 4 +- 11 files changed, 175 insertions(+), 27 deletions(-) diff --git a/config/config.go b/config/config.go index 8cbe745..300b073 100644 --- a/config/config.go +++ b/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. } diff --git a/mox-/config.go b/mox-/config.go index 0672f4b..fe7a359 100644 --- a/mox-/config.go +++ b/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 diff --git a/mox-/lookup.go b/mox-/lookup.go index 4946627..644a133 100644 --- a/mox-/lookup.go +++ b/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 } diff --git a/smtpserver/server_test.go b/smtpserver/server_test.go index 9aee6b6..69a2ac2 100644 --- a/smtpserver/server_test.go +++ b/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) - run(strings.ReplaceAll(dmarcReport, "xmox.nl", "mox.example"), 1) + n := 0 + 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 = ` @@ -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) - run("mjl@mox.example", strings.ReplaceAll(tlsrpt, "xmox.nl", "mox.example"), 1) - run("mjl@mailhost.mox.example", strings.ReplaceAll(tlsrpt, "xmox.nl", "mailhost.mox.example"), 2) + n := 0 + run("tls-reports@mox.example", tlsrpt, n) // Wrong domain in report. + + 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) { diff --git a/testdata/smtp/dmarcreport/domains.conf b/testdata/smtp/dmarcreport/domains.conf index dacf14f..bf09a61 100644 --- a/testdata/smtp/dmarcreport/domains.conf +++ b/testdata/smtp/dmarcreport/domains.conf @@ -1,11 +1,19 @@ Domains: mox.example: DMARC: - Account: mjl - Localpart: mjl + Account: dmarc + # 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 diff --git a/testdata/smtp/tlsrpt/domains.conf b/testdata/smtp/tlsrpt/domains.conf index 40b2e9c..f50990e 100644 --- a/testdata/smtp/tlsrpt/domains.conf +++ b/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 diff --git a/testdata/smtp/tlsrpt/mox.conf b/testdata/smtp/tlsrpt/mox.conf index 14b4adc..bbcc6ad 100644 --- a/testdata/smtp/tlsrpt/mox.conf +++ b/testdata/smtp/tlsrpt/mox.conf @@ -10,4 +10,4 @@ Listeners: HostTLSRPT: Account: mjl Mailbox: TLSRPT - Localpart: mjl + Localpart: tls-reports diff --git a/webadmin/admin.go b/webadmin/admin.go index 03de1f3..84ea8aa 100644 --- a/webadmin/admin.go +++ b/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 { diff --git a/webadmin/admin_test.go b/webadmin/admin_test.go index 97752dd..d0ca091 100644 --- a/webadmin/admin_test.go +++ b/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"}) diff --git a/webadmin/api.json b/webadmin/api.json index c6b7d8e..94f8f07 100644 --- a/webadmin/api.json +++ b/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" ] diff --git a/webadmin/api.ts b/webadmin/api.ts index a3ab439..671891e 100644 --- a/webadmin/api.ts +++ b/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. }