Bug 6918 - SpamAssassin needs to handle Authentication-Results data
Summary: SpamAssassin needs to handle Authentication-Results data
Status: NEW
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Plugins (show other bugs)
Version: unspecified
Hardware: All All
: P2 normal
Target Milestone: Undefined
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
: 7775 (view as bug list)
Depends on:
Blocks: 7189
  Show dependency tree
 
Reported: 2013-03-16 11:25 UTC by Alessandro Vesely
Modified: 2019-11-21 15:13 UTC (History)
3 users (show)



Attachment Type Modified Status Actions Submitter/CLA Status
Proposed plugin application/octet-stream None Alessandro Vesely [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Alessandro Vesely 2013-03-16 11:25:34 UTC
Created attachment 5138 [details]
Proposed plugin

This is slightly related to bug #6899.  Being able to handle such data can also save DNS queries to dnswl.org, omit repeating DKIM checks, and possibly more.

I propose a draft implementation, that I add as an attachment.  I run it under SA 3.3.1 for some alpha tests, and it apparently works.  However, more tests and review are needed:

* There are only two "adjustments" (commented as such).  We probably need more.

* I'm a newcomer to SA, so I might have overlooked something.
Comment 1 Henrik Krohns 2018-11-08 11:04:20 UTC
Based on this, I've cleaned up a basic version and the parser should be quite robust now. Committing to get it out of my queue, and perhaps get some developing going.

Sending        MANIFEST
Sending        UPGRADE
Adding         lib/Mail/SpamAssassin/Plugin/AuthRes.pm
Sending        t/all_modules.t
Adding         t/authres.t
Adding         t/data/nice/authres
Transmitting file data ......done
Committing transaction...
Committed revision 1846123.

There's only these options for now:
authres_networks
authres_ignored_authserv
authres_trusted_authserv

Honestly I'm not sure what we should do with it. It doesn't help "reducing" SPF/DKIM lookups, since atleast opendkim/opendmarc create quite simple Authentication-Results headers. How exactly are we going to replicate all these possibly useful rules, when we have some opendkim response of "dkim=pass,fail" without details?

full     DKIM_SIGNED         eval:check_dkim_signed()
full     DKIM_VALID          eval:check_dkim_valid()
full     DKIM_VALID_AU               eval:check_dkim_valid_author_sig()
full     DKIM_VALID_EF               eval:check_dkim_valid_envelopefrom()
full     __DKIM_DEPENDABLE   eval:check_dkim_dependable()
header   DKIM_ADSP_NXDOMAIN  eval:check_dkim_adsp('N')
header   DKIM_ADSP_DISCARD   eval:check_dkim_adsp('D')
header   DKIM_ADSP_ALL               eval:check_dkim_adsp('A')
header   DKIM_ADSP_CUSTOM_LOW        eval:check_dkim_adsp('1')
header   DKIM_ADSP_CUSTOM_MED        eval:check_dkim_adsp('2')
header   DKIM_ADSP_CUSTOM_HIGH       eval:check_dkim_adsp('3')
full   DKIM_VERIFIED         eval:check_dkim_valid()
header DKIM_POLICY_TESTING   eval:check_dkim_testing()
header DKIM_POLICY_SIGNSOME  eval:check_dkim_signsome()
header DKIM_POLICY_SIGNALL   eval:check_dkim_signall()

Same with SPF. Atleast opendmarc's inbuilt SPF check doesn't report NEUTRAL let alone some HELO stuff? Dunno if pypolicyd-spf / spf-engine gives anything useful.

header   SPF_PASS     eval:check_for_spf_pass()
header   SPF_NEUTRAL  eval:check_for_spf_neutral()
header   SPF_FAIL     eval:check_for_spf_fail()
header   SPF_SOFTFAIL eval:check_for_spf_softfail()
header   SPF_HELO_PASS                eval:check_for_spf_helo_pass()
header   SPF_HELO_NEUTRAL     eval:check_for_spf_helo_neutral()
header   SPF_HELO_FAIL                eval:check_for_spf_helo_fail()
header   SPF_HELO_SOFTFAIL    eval:check_for_spf_helo_softfail()
header   SPF_NONE             eval:check_for_spf_none()
header   SPF_HELO_NONE                eval:check_for_spf_helo_none()
Comment 2 Henrik Krohns 2018-11-11 12:03:38 UTC
Sigh, opendkim and opendmarc are broken..

https://github.com/trusteddomainproject/OpenDKIM/issues/24
https://github.com/trusteddomainproject/OpenDMARC/issues/23

So practically noone can make use the A-R headers unless compiling yourself
or some distribution decides to patch them.  The developement on these is so
darn slow, who knows when official versions are out..
Comment 3 Henrik Krohns 2018-11-12 17:12:20 UTC
https://marc.info/?l=postfix-users&m=154203866507674&w=2

As Viktor described, I think we should recommend using "authres_networks all" with authres_trusted_authserv. That way it doesn't matter where the headers are positioned and works with OpenDKIM etc out of the box.

Sending        AuthRes.pm
Transmitting file data .done
Committing transaction...
Committed revision 1846448.
Comment 4 Alessandro Vesely 2018-11-12 18:47:24 UTC
I agree with Henrik, except for missing a curly brace:
--- AuthRes.pm.old
+++ AuthRes.pm.new
@@ -179,7 +179,7 @@
   });
 
   $conf->{parser}->register_commands(\@cmds);
-}
+
 
 =item authres_ignored_authserv authservid1 id2 ...   (default: none)
 
@@ -205,6 +205,7 @@
       }
     }
   });
+}
 
 =head1 METADATA
 
(perhaps sooner or later I'll find out how to commit.)

Longer rumination (written before Henryk's post):

The bugs referenced in comment #2 are about placing A-R fields before or after Received.  That's no usability problem.  My MTA, for example, places some A-R fields before and some after Received, depending on which process reports what result.  Much like X-Spam-* fields, admins know how ingress filtering works and need to configure ADMINISTRATOR OPTIONS accordingly.  In order to take into account A-R fields placed before Received, authres_networks needs to be set to "all", relying on authservid's if needed.

The only issue is to explain that in Mail::SpamAssassin::Conf.

I don't see why opendkim would create "quite simple" Authentication-Results.  From a mailing list which I know uses OpenDKIM, I got:

Nov 12 18:38:45.003 [26569] dbg: authres: parsing Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1536-bit key) header.d=iecc.com header.b=kbJdZYzc; dkim=pass (1536-bit key) header.d=taugh.com header.b=LwXl016e

Use of that result should be done in DKIM.pm, probably _check_dkim_signature, instead of looking up everything from scratch.  Ditto for SPF.
Comment 5 Henrik Krohns 2018-11-12 19:29:08 UTC
Ah yes hasty commits while doing household chores..

I need to look into all the header contents when I have more time.. feel free to explain to me if you have ideas what should hit what.. do we need to even care about some deprecated ADSP rules?
Comment 6 Alessandro Vesely 2018-11-13 19:09:13 UTC
For DKIM, Mail::DKIM::Verifier looks up keys as soon as it parses a signature, like so:

    my $signature = Mail::DKIM::Signature->parse($line);
    $self->add_signature($signature);
    $signature->fetch_public_key;

We need a parsed signature in order to understand if it was verified already, so as to skip its verification.  A domain can add multiple signatures which can have different selectors or at least different hash (header.s is not yet part of A-R; if header.b is missing, we must assume that this signature was the only one the verifier saw by the given domain.)

Of course, a domain can add signatures after A-R fields were written.  Those should not be interesting if the A-Rs are trusted.  However, A-Rs, albeit by a trusted agent, might be stale, written before a message was further relayed.  In that case  a careful MTA should invalidate them (Courier-MTA, for example, renames them to Old-Authentication-Results on ingress.)

The quick solution is to have the admin tell if A-Rs are authoritative for DKIM, which also entails that there is no valid DKIM signature unless we found the result.  Messages arriving from a different path, with no A-R, may still require to load DKIM::Verifier.

Otherwise, the hard solution requires to learn more Mail::DKIM internals.  BTW, what's that "caller of SpamAssassin already supplied DKIM signature objects" comment in SpamAssassin::DKIM?
Comment 7 Henrik Krohns 2019-11-21 15:13:37 UTC
*** Bug 7775 has been marked as a duplicate of this bug. ***