opendmarc/opendmarc.c:mlfi_eom(): respect AuthservIDWithJobID when adding AR: authservid; spf= header

[dilyanpalauzov]

In particular when SPFIgnoreResults, SPFSelfValidate and AuthservIDWithJobID are enabled:

diff --git a/opendmarc/opendmarc.c b/opendmarc/opendmarc.c
--- a/opendmarc/opendmarc.c
+++ b/opendmarc/opendmarc.c
@@ -2918,14 +2918,22 @@ mlfi_eom(SMFICTX *ctx)
                        if (spf_mode == DMARC_POLICY_SPF_ORIGIN_HELO)
                        {
                                snprintf(header, sizeof header,
-                                        "%s; spf=%s smtp.helo=%s",
-                                        authservid, pass_fail, use_domain);
+                                        "%s%s%s; spf=%s smtp.helo=%s",
+                                        authservid,
+                                        conf->conf_authservidwithjobid ? "/" : "",
+                                        conf->conf_authservidwithjobid ? dfc->mctx_jobid : "",
+                                        pass_fail, use_domain);
                        }
                        else
                        {
                                snprintf(header, sizeof header,
-                                        "%s; spf=%s smtp.mailfrom=%s",
-                                        authservid, pass_fail, use_domain);
+                                        "%s%s%s; spf=%s smtp.mailfrom=%s",
+                                        authservid,
+                                        conf->conf_authservidwithjobid ? "/" : "",
+                                        conf->conf_authservidwithjobid ? dfc->mctx_jobid : "",
+                                        pass_fail, use_domain);
+
+
                        }
 
                        if (dmarcf_insheader(ctx, 1, AUTHRESULTSHDR,

[dilyanpalauzov]

Or just add a single Authentication-Results header, that contains both the spf amd dmarc checks:

diff --git a/opendmarc/opendmarc.c b/opendmarc/opendmarc.c
--- a/opendmarc/opendmarc.c
+++ b/opendmarc/opendmarc.c
@@ -2252,6 +2252,9 @@ mlfi_eom(SMFICTX *ctx)
        unsigned char replybuf[BUFRSZ + 1];
        unsigned char pdomain[MAXHOSTNAMELEN + 1];
        struct authres ar;
+#if WITH_SPF
+       unsigned char spfheader[MAXHEADER + 1] = "";
+#endif
 
        assert(ctx != NULL);
 
@@ -2917,37 +2920,19 @@ mlfi_eom(SMFICTX *ctx)
 
                        if (spf_mode == DMARC_POLICY_SPF_ORIGIN_HELO)
                        {
-                               snprintf(header, sizeof header,
-                                        "%s%s%s; spf=%s smtp.helo=%s",
-                                        authservid,
-                                        conf->conf_authservidwithjobid ? "/" : "",
-                                        conf->conf_authservidwithjobid ? dfc->mctx_jobid : "",
+                               snprintf(spfheader, sizeof spfheader,
+                                        "; spf=%s smtp.helo=%s",
                                         pass_fail, use_domain);
                        }
                        else
                        {
-                               snprintf(header, sizeof header,
-                                        "%s%s%s; spf=%s smtp.mailfrom=%s",
-                                        authservid,
-                                        conf->conf_authservidwithjobid ? "/" : "",
-                                        conf->conf_authservidwithjobid ? dfc->mctx_jobid : "",
+                               snprintf(spfheader, sizeof spfheader,
+                                        "; spf=%s smtp.mailfrom=%s",
                                         pass_fail, use_domain);
 
                        if (dmarcf_insheader(ctx, 1, AUTHRESULTSHDR,
                        }
 
-                       if (dmarcf_insheader(ctx, 1, AUTHRESULTSHDR,
-                                            header) == MI_FAILURE)
-                       {
-                               if (conf->conf_dolog)
-                               {
-                                       syslog(LOG_ERR,
-                                              "%s: %s header add failed",
-                                              dfc->mctx_jobid,
-                                              AUTHRESULTSHDR);
-                               }
-                       }
-
                        if (conf->conf_dolog)
                        {
                                char *mode;
@@ -2998,8 +2983,13 @@ mlfi_eom(SMFICTX *ctx)
                }
 
                snprintf(header, sizeof header,
+#if WITH_SPF
+                        "%s; dmarc=permerror header.from=%s%s",
+                        authservid, dfc->mctx_fromdomain, spfheader);
+#else
                         "%s; dmarc=permerror header.from=%s",
                         authservid, dfc->mctx_fromdomain);
+#endif
 
                if (dmarcf_insheader(ctx, 1, AUTHRESULTSHDR,
                                     header) == MI_FAILURE)
@@ -3519,11 +3509,19 @@ mlfi_eom(SMFICTX *ctx)
        if (ret != SMFIS_TEMPFAIL && ret != SMFIS_REJECT)
        {
                snprintf(header, sizeof header,
+#if WITH_SPF
+                        "%s%s%s; dmarc=%s (p=%s dis=%s) header.from=%s%s",
+                        authservid,
+                        conf->conf_authservidwithjobid ? "/" : "",
+                        conf->conf_authservidwithjobid ? dfc->mctx_jobid : "",
+                        aresult, apolicy, adisposition, dfc->mctx_fromdomain, spfheader);
+#else
                         "%s%s%s; dmarc=%s (p=%s dis=%s) header.from=%s",
                         authservid,
                         conf->conf_authservidwithjobid ? "/" : "",
                         conf->conf_authservidwithjobid ? dfc->mctx_jobid : "",
                         aresult, apolicy, adisposition, dfc->mctx_fromdomain);
+#endif
 
                if (dmarcf_insheader(ctx, 1, AUTHRESULTSHDR,
                                     header) == MI_FAILURE)

[hdatma]

This issue was opened in 2018. Today, August 2020, the patched file has dramatically changed. Can you close this issue?

[dilyanpalauzov]

I do use the latter change and I do not remember having any troubles doing git-rebase. The opendmarc.c file has not changed significantly since 2018.

[hdatma]

Can you rebase the patch against the last release?

[dilyanpalauzov]

The code I use is available at https://mail.aegee.org/cgit/OpenDMARC . I do not have the change above as a single commit. I guess I have two commits: • put SPF (performed by OpenDMARC) and DMARC results in a single A-R header; • make sure that header honours the AuthservIDWithJobID configuration.

[glts]

By the way, as described in trusteddomainproject/OpenDKIM#103, this change produces invalid authserv-ids.

[thegushi]

Apologies for the long delay on this — we know we've been somewhat absent from this tracker, but work is ongoing as should be visible in the repo.

Looking at this more closely: the dmarc= A-R header already respects AuthservIDWithJobID, but the spf= header does not. We could apply the patch to bring them into consistency, but as noted in trusteddomainproject/OpenDKIM#103, the authservid/jobid format is itself syntactically invalid per RFC 8601 + RFC 2045 — / is not permitted in an unquoted token. The correct form requires a quoted-string: "mail.example.org/JOBID". Since applying this patch as written would just extend that non-conformance to the SPF header, we'd rather not ship a half-fix.

We'd welcome a patch that does this RFC-correctly — quoting the authserv-id when a job ID is appended, across both the dmarc= and spf= code paths.

[dilyanpalauzov]

The case was opened in 2018, I do not remember the details, I have to start anew from zero to understand the problem.

The code I use is published at https://mail.aegee.org/cgit/OpenDMARC/log/ , this particular change is in the two commits starting with GH#17.

[thegushi]

Thanks for the report and for pointing to your working code — it helped us track down two real bugs.

We did things slightly differently than your patch, and wanted to explain why.

What we fixed:

1. AuthservIDWithJobID was being ignored when building the spf= A-R header — it used bare authservid while the dmarc= header used authservid/jobid. Both now use the same correctly-formatted value.

2. authservid/jobid is not a valid RFC 8601 / RFC 2045 token — / is a tspecial and requires the whole value to be quoted. It now emits "authservid/jobid" (a quoted string). Bare authservids without a job ID are valid tokens and are left unquoted.

Why we didn't combine the headers:

Your patch also merged the spf= and dmarc= results into a single Authentication-Results header, which is technically valid per RFC 8601. However, a lot of downstream software — Rspamd, SpamAssassin, and others — iterates over A-R headers by name and processes each one independently. Combining them risks breaking those consumers in ways that are hard to predict and harder to debug, especially for operators who aren't running the same stack we are. We felt the principle of least astonishment argued strongly for keeping them separate.

Your subject line described the AuthservIDWithJobID inconsistency, which is what we've addressed. The header combination was a separate design choice embedded in the patch that we couldn't carry along for the reasons above.

[thegushi]

@glts — you spotted the invalid authserv-id issue back in 2021. We've addressed it in #311 — authservid/jobid is now emitted as "authservid/jobid" (quoted string) per RFC 8601 + RFC 2045 across all A-R header paths. If you have a chance to test it we'd love a second set of eyes from someone who already knew where to look.