From e0c36edb8f1b13a9e9acfab08842ac2f200c9b1a Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Tue, 5 Mar 2024 11:43:49 +0100 Subject: [PATCH] accept tls reports with both host & recipient domains, and with multiple recipient domains embarrassingly, we didn't accept all reports we generated. after the changed handling of reports about mx/mail host vs recipient domains, would send reports to mail hosts about multiple recipient domains + the mail host. and we included a policy domain of the mail host when sending to a recipient domain. we were still being strict in what we accepted: only a single domain in total in the entire report, and we still enforced that a report sent to the mx host tlsrpt address only contained the mx host as policy domain. and likewise for recipient domains and their tls reporting addresses. those checks would reject reports generated by a mox instance. this probably only happens with dane configured, probably most users haven't seen it because of that. somewhat related to issue #125 --- tlsrptdb/report.go | 74 +++++++-------- tlsrptdb/report_test.go | 201 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 237 insertions(+), 38 deletions(-) diff --git a/tlsrptdb/report.go b/tlsrptdb/report.go index e1e1ba6..838891b 100644 --- a/tlsrptdb/report.go +++ b/tlsrptdb/report.go @@ -78,8 +78,9 @@ func reportDB(ctx context.Context) (rdb *bstore.DB, rerr error) { // verifiedFromDomain. Using HTTPS for reports is not recommended as there is no // authentication on the reports origin. // -// The report is currently required to only cover a single domain in its policy -// domain. Only reports for known domains are added to the database. +// Only reports for known domains are added to the database. Unknown domains are +// ignored without causing an error, unless no known domain was found in the report +// at all. // // Prometheus metrics are updated only for configured domains. func AddReport(ctx context.Context, log mlog.Log, verifiedFromDomain dns.Domain, mailFrom string, hostReport bool, r *tlsrpt.Report) error { @@ -92,44 +93,43 @@ func AddReport(ctx context.Context, log mlog.Log, verifiedFromDomain dns.Domain, return fmt.Errorf("no policies in report") } - var reportdom, zerodom dns.Domain - record := TLSReportRecord{0, "", verifiedFromDomain.Name(), mailFrom, hostReport, *r} + var inserted int + return db.Write(ctx, func(tx *bstore.Tx) error { + for _, p := range r.Policies { + pp := p.Policy - for _, p := range r.Policies { - pp := p.Policy - - // Check domain, they must all be the same for now. We are not expecting senders to - // coalesce TLS results for different policy domains in a single report. - d, err := dns.ParseDomain(pp.Domain) - if err != nil { - log.Errorx("invalid domain in tls report", err, slog.Any("domain", pp.Domain), slog.String("mailfrom", mailFrom)) - continue - } - if hostReport && d != mox.Conf.Static.HostnameDomain { - log.Info("unknown mail host policy domain in tls report, not storing", slog.Any("domain", d), slog.String("mailfrom", mailFrom)) - return fmt.Errorf("unknown mail host policy domain") - } else if _, ok := mox.Conf.Domain(d); !hostReport && !ok { - log.Info("unknown recipient policy domain in tls report, not storing", slog.Any("domain", d), slog.String("mailfrom", mailFrom)) - return fmt.Errorf("unknown recipient policy domain") - } - if reportdom != zerodom && d != reportdom { - return fmt.Errorf("multiple domains in report %s and %s", reportdom, d) - } - reportdom = d - - metricSession.WithLabelValues("success").Add(float64(p.Summary.TotalSuccessfulSessionCount)) - for _, f := range p.FailureDetails { - var result string - if _, ok := knownResultTypes[f.ResultType]; ok { - result = string(f.ResultType) - } else { - result = "other" + d, err := dns.ParseDomain(pp.Domain) + if err != nil { + return fmt.Errorf("invalid domain %v in tls report: %v", d, err) } - metricSession.WithLabelValues(result).Add(float64(f.FailedSessionCount)) + + if _, ok := mox.Conf.Domain(d); !ok && d != mox.Conf.Static.HostnameDomain { + log.Info("unknown host/recipient policy domain in tls report, not storing", slog.Any("domain", d), slog.String("mailfrom", mailFrom)) + continue + } + + metricSession.WithLabelValues("success").Add(float64(p.Summary.TotalSuccessfulSessionCount)) + for _, f := range p.FailureDetails { + var result string + if _, ok := knownResultTypes[f.ResultType]; ok { + result = string(f.ResultType) + } else { + result = "other" + } + metricSession.WithLabelValues(result).Add(float64(f.FailedSessionCount)) + } + + record := TLSReportRecord{0, d.Name(), verifiedFromDomain.Name(), mailFrom, d == mox.Conf.Static.HostnameDomain, *r} + if err := tx.Insert(&record); err != nil { + return fmt.Errorf("inserting report for domain: %w", err) + } + inserted++ } - } - record.Domain = reportdom.Name() - return db.Insert(ctx, &record) + if inserted == 0 { + return fmt.Errorf("no domains in report recognized") + } + return nil + }) } // Records returns all TLS reports in the database. diff --git a/tlsrptdb/report_test.go b/tlsrptdb/report_test.go index efeb976..7cbe9e3 100644 --- a/tlsrptdb/report_test.go +++ b/tlsrptdb/report_test.go @@ -62,14 +62,183 @@ const reportJSON = `{ }] }` +const reportMultipleJSON = `{ + "organization-name": "remote.example", + "date-range": { + "start-datetime": "2024-02-25T00:00:00Z", + "end-datetime": "2024-02-25T23:59:59Z" + }, + "contact-info": "postmaster@remote.example", + "report-id": "20240225.mail.mox.example@remote.example", + "policies": [ + { + "policy": { + "policy-type": "tlsa", + "policy-string": [ + "3 1 1 206d5f55ecb9f8389bc57b5ba14716dd5b23d0834fd2c99fd402f0bda32e9523", + "3 1 1 4201e4b741c746b62ff806c142158c35ecbbbd9ac56b6d791f760e272736f8d0" + ], + "policy-domain": "test2.xmox.nl", + "mx-host": [ + "mail.mox.example" + ] + }, + "summary": { + "total-successful-session-count": 1, + "total-failure-session-count": 0 + }, + "failure-details": [] + }, + { + "policy": { + "policy-type": "tlsa", + "policy-string": [ + "3 1 1 206d5f55ecb9f8389bc57b5ba14716dd5b23d0834fd2c99fd402f0bda32e9523", + "3 1 1 4201e4b741c746b62ff806c142158c35ecbbbd9ac56b6d791f760e272736f8d0" + ], + "policy-domain": "test.xmox.nl", + "mx-host": [ + "mail.mox.example" + ] + }, + "summary": { + "total-successful-session-count": 1, + "total-failure-session-count": 0 + }, + "failure-details": [] + } + ] +} +` + +const reportMixedJSON = `{ + "organization-name": "remote.example", + "date-range": { + "start-datetime": "2024-02-25T00:00:00Z", + "end-datetime": "2024-02-25T23:59:59Z" + }, + "contact-info": "postmaster@remote.example", + "report-id": "20240225.test.xmox.nl@remote.example", + "policies": [ + { + "policy": { + "policy-type": "tlsa", + "policy-string": [ + "3 1 1 206d5f55ecb9f8389bc57b5ba14716dd5b23d0834fd2c99fd402f0bda32e9523", + "3 1 1 4201e4b741c746b62ff806c142158c35ecbbbd9ac56b6d791f760e272736f8d0" + ], + "policy-domain": "mail.mox.example", + "mx-host": [] + }, + "summary": { + "total-successful-session-count": 1, + "total-failure-session-count": 0 + }, + "failure-details": [] + }, + { + "policy": { + "policy-type": "sts", + "policy-string": [ + "version: STSv1", + "mode: enforce", + "max_age: 86400", + "mx: mail.mox.example" + ], + "policy-domain": "unknown.xmox.nl", + "mx-host": [ + "mail.mox.example" + ] + }, + "summary": { + "total-successful-session-count": 1, + "total-failure-session-count": 0 + }, + "failure-details": [] + }, + { + "policy": { + "policy-type": "sts", + "policy-string": [ + "version: STSv1", + "mode: enforce", + "max_age: 86400", + "mx: mail.mox.example" + ], + "policy-domain": "test.xmox.nl", + "mx-host": [ + "mail.mox.example" + ] + }, + "summary": { + "total-successful-session-count": 1, + "total-failure-session-count": 0 + }, + "failure-details": [] + } + ] +} +` + +const reportUnknownJSON = `{ + "organization-name": "remote.example", + "date-range": { + "start-datetime": "2024-02-25T00:00:00Z", + "end-datetime": "2024-02-25T23:59:59Z" + }, + "contact-info": "postmaster@remote.example", + "report-id": "20240225.test.xmox.nl@remote.example", + "policies": [ + { + "policy": { + "policy-type": "tlsa", + "policy-string": [ + "3 1 1 206d5f55ecb9f8389bc57b5ba14716dd5b23d0834fd2c99fd402f0bda32e9523", + "3 1 1 4201e4b741c746b62ff806c142158c35ecbbbd9ac56b6d791f760e272736f8d0" + ], + "policy-domain": "unknown.mox.example", + "mx-host": [] + }, + "summary": { + "total-successful-session-count": 1, + "total-failure-session-count": 0 + }, + "failure-details": [] + }, + { + "policy": { + "policy-type": "sts", + "policy-string": [ + "version: STSv1", + "mode: enforce", + "max_age: 86400", + "mx: mail.mox.example" + ], + "policy-domain": "unknown.xmox.nl", + "mx-host": [ + "unknown.mox.example" + ] + }, + "summary": { + "total-successful-session-count": 1, + "total-failure-session-count": 0 + }, + "failure-details": [] + } + ] +} +` + func TestReport(t *testing.T) { mox.Context = ctxbg mox.Shutdown, mox.ShutdownCancel = context.WithCancel(ctxbg) mox.ConfigStaticPath = filepath.FromSlash("../testdata/tlsrpt/fake.conf") + mox.Conf.Static.HostnameDomain = dns.Domain{ASCII: "mail.mox.example"} mox.Conf.Static.DataDir = "." // Recognize as configured domain. mox.Conf.Dynamic.Domains = map[string]config.Domain{ - "test.xmox.nl": {}, + "test.xmox.nl": {}, + "test2.xmox.nl": {}, } dbpath := mox.DataDirPath("tlsrpt.db") @@ -133,4 +302,34 @@ func TestReport(t *testing.T) { if err != nil || len(records) != 1 { t.Fatalf("got err %v, records %#v, expected no error with 1 record", err, records) } + + // Add report with multiple recipient domains. + reportJSON, err = tlsrpt.Parse(strings.NewReader(reportMultipleJSON)) + if err != nil { + t.Fatalf("parsing report: %v", err) + } + report = reportJSON.Convert() + if err := AddReport(ctxbg, pkglog, dns.Domain{ASCII: "remote.example"}, "postmaster@remote.example", false, &report); err != nil { + t.Errorf("adding report to database: %s", err) + } + + // Add report with mixed host and domain policies. The unknown domain is ignored. + reportJSON, err = tlsrpt.Parse(strings.NewReader(reportMixedJSON)) + if err != nil { + t.Fatalf("parsing report: %v", err) + } + report = reportJSON.Convert() + if err := AddReport(ctxbg, pkglog, dns.Domain{ASCII: "remote.example"}, "postmaster@remote.example", false, &report); err != nil { + t.Errorf("adding report to database: %s", err) + } + + // All unknown domains in report should cause error. + reportJSON, err = tlsrpt.Parse(strings.NewReader(reportUnknownJSON)) + if err != nil { + t.Fatalf("parsing report: %v", err) + } + report = reportJSON.Convert() + if err := AddReport(ctxbg, pkglog, dns.Domain{ASCII: "remote.example"}, "postmaster@remote.example", false, &report); err == nil { + t.Errorf("adding report with all unknown domains, expected error") + } }