From 481a25f29474f6fef32c4748af4efc5676b690b5 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Thu, 2 Nov 2023 17:54:24 +0100 Subject: [PATCH] improvements to outgoing dmarc reports and displaying evaluations - more eagerly report about overrides, so domain owners can better tell that switching from p=none to p=reject will not cause trouble for these messages. - report multiple reasons, e.g. mailing list and sampled out - in dmarc analysis for rejects from first-time senders (possibly spammers), fix the conditional check on nonjunk messages. - in evaluations view in admin, show unaligned spf pass in yellow too and a few more small tweaks. --- smtpserver/server.go | 29 +++++++++++++++-------------- webadmin/admin.html | 8 ++++---- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/smtpserver/server.go b/smtpserver/server.go index 4bee75d..db735e8 100644 --- a/smtpserver/server.go +++ b/smtpserver/server.go @@ -2411,24 +2411,26 @@ func (c *conn) deliver(ctx context.Context, recvHdrFor func(string) string, msgW // Any DMARC result override is stored in the evaluation for outgoing DMARC // aggregate reports, and added to the Authentication-Results message header. - var dmarcOverride string - if dmarcResult.Record != nil { - if !dmarcUse { - dmarcOverride = string(dmarcrpt.PolicyOverrideSampledOut) - } else if a.dmarcOverrideReason != "" && (a.accept && !m.IsReject) == dmarcResult.Reject { - dmarcOverride = a.dmarcOverrideReason - } + // We want to tell the sender that we have an override, e.g. for mailing lists, so + // they don't overestimate the potential damage of switching from p=none to + // p=reject. + var dmarcOverrides []string + if a.dmarcOverrideReason != "" { + dmarcOverrides = []string{a.dmarcOverrideReason} + } + if dmarcResult.Record != nil && !dmarcUse { + dmarcOverrides = append(dmarcOverrides, string(dmarcrpt.PolicyOverrideSampledOut)) } // Add per-recipient DMARC method to Authentication-Results. Each account can have // their own override rules, e.g. based on configured mailing lists/forwards. // ../rfc/7489:1486 rcptDMARCMethod := dmarcMethod - if dmarcOverride != "" { + if len(dmarcOverrides) > 0 { if rcptDMARCMethod.Comment != "" { rcptDMARCMethod.Comment += ", " } - rcptDMARCMethod.Comment += "override " + dmarcOverride + rcptDMARCMethod.Comment += "override " + strings.Join(dmarcOverrides, ",") } rcptAuthResults := authResults rcptAuthResults.Methods = append([]message.AuthMethod{}, authResults.Methods...) @@ -2477,7 +2479,7 @@ func (c *conn) deliver(ctx context.Context, recvHdrFor func(string) string, msgW // See if we received a non-junk message from this organizational domain. q := bstore.QueryTx[store.Message](tx) q.FilterNonzero(store.Message{MsgFromOrgDomain: m.MsgFromOrgDomain}) - q.FilterEqual("Notjunk", false) + q.FilterEqual("Notjunk", true) exists, err := q.Exists() if err != nil { return fmt.Errorf("querying for non-junk message from organizational domain: %v", err) @@ -2544,10 +2546,9 @@ func (c *conn) deliver(ctx context.Context, recvHdrFor func(string) string, msgW HeaderFrom: msgFrom.Domain.Name(), } - if dmarcOverride != "" { - eval.OverrideReasons = []dmarcrpt.PolicyOverrideReason{ - {Type: dmarcrpt.PolicyOverride(dmarcOverride)}, - } + for _, s := range dmarcOverrides { + reason := dmarcrpt.PolicyOverrideReason{Type: dmarcrpt.PolicyOverride(s)} + eval.OverrideReasons = append(eval.OverrideReasons, reason) } // We'll include all signatures for the organizational domain, even if they weren't diff --git a/webadmin/admin.html b/webadmin/admin.html index d19c774..3be1fbb 100644 --- a/webadmin/admin.html +++ b/webadmin/admin.html @@ -1148,7 +1148,7 @@ const dmarcEvaluationsDomain = async (domain) => { const authStatus = (v) => inlineBox(v ? '' : yellow, v ? 'pass' : 'fail') const formatDKIMResults = (results) => results.map(r => dom.div('selector '+r.Selector+(r.Domain !== domain ? ', domain '+r.Domain : '') + ': ', inlineBox(r.Result === "pass" ? '' : yellow, r.Result))) - const formatSPFResults = (results) => results.map(r => dom.div(''+r.Scope+(r.Domain !== domain ? ', domain '+r.Domain : '') + ': ', inlineBox(r.Result === "pass" ? '' : yellow, r.Result))) + const formatSPFResults = (alignedpass, results) => results.map(r => dom.div(''+r.Scope+(r.Domain !== domain ? ', domain '+r.Domain : '') + ': ', inlineBox(r.Result === "pass" && alignedpass ? '' : yellow, r.Result))) const sourceIP = (ip) => { const r = dom.span(ip, attr({title: 'Click to do a reverse lookup of the IP.'}), style({cursor: 'pointer'}), async function click(e) { @@ -1198,7 +1198,7 @@ const dmarcEvaluationsDomain = async (domain) => { dom.th('Policy', attr({title: 'Summary of the policy as encountered in the DMARC DNS record of the domain, and used for evaluation.'})), dom.th('IP', attr({title: 'IP address of delivery attempt that was evaluated, relevant for SPF.'})), dom.th('Disposition', attr({title: 'Our decision to accept/reject this message. It may be different than requested by the published policy. For example, when overriding due to delivery from a mailing list or forwarded address.'})), - dom.th('DKIM/SPF', attr({title: 'Whether DKIM and SPF had an aligned pass, where strict/relaxed alignment means whether the domain of an SPF pass and DKIM pass matches the exact domain (strict) or optionally a subdomain (relaxed). A DMARC pass requires at least one pass.'})), + dom.th('Aligned DKIM/SPF', attr({title: 'Whether DKIM and SPF had an aligned pass, where strict/relaxed alignment means whether the domain of an SPF pass and DKIM pass matches the exact domain (strict) or optionally a subdomain (relaxed). A DMARC pass requires at least one pass.'})), dom.th('Envelope to', attr({title: 'Domain used in SMTP RCPT TO during delivery.'})), dom.th('Envelope from', attr({title: 'Domain used in SMTP MAIL FROM during delivery.'})), dom.th('Message from', attr({title: 'Domain in "From" message header.'})), @@ -1228,13 +1228,13 @@ const dmarcEvaluationsDomain = async (domain) => { dom.td(addresses), dom.td(policy), dom.td(sourceIP(e.SourceIP)), - dom.td(inlineBox(e.Disposition === 'none' ? '' : 'red', e.Disposition), (e.OverrideReasons || []).length > 0 ? ' ('+e.OverrideReasons.map(r => r.Type).join(', ')+')' : ''), + dom.td(inlineBox(e.Disposition === 'none' ? '' : red, e.Disposition), (e.OverrideReasons || []).length > 0 ? ' ('+e.OverrideReasons.map(r => r.Type).join(', ')+')' : ''), dom.td(authStatus(e.AlignedDKIMPass), '/', authStatus(e.AlignedSPFPass)), dom.td(e.EnvelopeTo), dom.td(e.EnvelopeFrom), dom.td(e.HeaderFrom), dom.td(formatDKIMResults(e.DKIMResults || [])), - dom.td(formatSPFResults(e.SPFResults || [])), + dom.td(formatSPFResults(e.AlignedSPFPass, e.SPFResults || [])), ) }), evaluations.length === 0 ? dom.tr(dom.td(attr({colspan: '14'}), 'No evaluations.')) : [],