Bug 6490

Created attachment 4807 [details]
Proposed patch - for alpha testing.

Here's my alpha patch for this enhancement.  Let me know if it works for you.

Some of the duplicate code could probably be collapsed intelligently.  However, the point here is to see if it works, not that it necessarily works well.

Could someone with commit access please commit this so it may be included in the 3.3.2 release.

"SPF_NONE" should have been included from the beginning of the module.  There are those who wish to "bias" against a message that doesn't authenticate its sender in any way....

Does this "SPF_NONE" patch handle DNS sub-system failure?

The reason SPF_NONE was not "included from the beginning of the module" as it "should have been" is because, at least at the time, there was no way to distinguish between no DNS response to an SPF check (for whatever reason) and there being no SPF record.

I'm -1 on any soluton which cannot handle this distinction.

Look at the patch.  It requires an AFFIRMATIVE none result.  This means:

1)  For a prior "Received-SPF" or "Authentication-Results" header, it must state "none" as the result.

2)  For a DNS lookup via the SPF-lookup function (both the old and new ones are still supported), that function must return "none."  If the DNS lookup fails, those functions return "tempfail" which does NOT set the "NONE" result flag.  therefore, as long as Mail::SPF behaves properly, so does this function.

SPF results of "temperror", "permerror", or "policy" (introduced in RFC 5451) will all set the "spf_checked" flag to 1, but NO other result flags.

You should have looked at the patch before commenting.  The issue is already avoided.

Put a little more effort into winning friends.  I did read your patch, and while I noticed most of what you said, it was still important for you to explicitly state that in the event of a DNS failure, TEMPERROR or PERMERROR will be used, and will not result in SPF_NONE.

Why need that be stated at all?  It's not part of the SPF specification per RFC 4408?  The two error conditions are separate result states from "NONE" -- the latter being strictly defined as either a DNS NXDOMAIN state or DNS NOERROR state with DNS answers being zero (meaning that some other RR-type exists for the label queried), or if temporary use of TXT-RRs, that no TXT-RR starts with the string "v=spf".*  The original module didn't make such a comment as you ask or suggest I do, so why should I?  The addition of the "NONE" state does not affect the results of the error states.

It is documented in the MAIL::SPF module.  Why should I assume that you're not familiar with it?

* - (However, NO ONE should be using TXT-RRs for SPF.  The dedicated SPF-RR was authorized by IANA in 2006, and 5 years is more than enough time to make the transition.)

Because you want your patch to be applied.

And apparently committers don't know you well enough to assume that you did it right.

Also, you would probably be better off not assuming that other people are interested enough in doing the work necessary to get your pet project committed that they're willing to read an entire RFC.

And so you better understand my perspective, I am very interested in SPF_NONE.

Considering that the patch was uploaded 6 months ago and is pretty trivial (except for the one section dealing with "Authenitcation-Results" which basically duplicates the "Received-SPF" section of the module with few changes), there's been plenty of time to review it.

I've been running it on my production machine [for 6 months] and have yet to see a problem.

(In reply to comment #6)

> * - (However, NO ONE should be using TXT-RRs for SPF.  The dedicated SPF-RR was
> authorized by IANA in 2006, and 5 years is more than enough time to make the
> transition.)

Lots and lots of people are using TXT RRs for SPF. Why should they change when it works?

> * - (However, NO ONE should be using TXT-RRs for SPF.  The dedicated SPF-RR was
> authorized by IANA in 2006, and 5 years is more than enough time to make the
> transition.)

Not all versions of bind support the SPF RR.  If you have a "stable" distro with an older version of bind, you might not be able to query or advertise the SPF RR.  TXT RR's have been around forever and are supported in all versions of bind.

To use your argument, SNMP v3 was codified in 1996, so there is no reason for anyone to use snmp v2c or snmp v1...

> Lots and lots of people are using TXT RRs for SPF. Why should they change
> when it works?

Because loading of the TXT-RR for SPF data was a TEMPORARY measure (per RFC 4408) until SPF received its own RR-type, which happened 5 years ago.  Five years is too long to be "temporary."  At some point, SPF support for TXT-RRs will be removed, and they will have nothing working.

As far as "upgrade or die" arguments go, have you noticed that some of the more popular web sites are now denying access to people using MSIE 6 on the grounds that it's obsolete software?  However, it's NOT the same with SPF, as RFC 4408 itself indicated the "temporary" status for TXT-RR from the very beginning.  The only thing the RFC didn't include is an actual schedule.

However, all of that doesn't matter to this patch, as the Mail::SPF module deals with TXT-RR support (as well as SPF-RR support) and we need not do anything special in SA.  What matters is the additional condition this patch returns for SA evaluation.


Lastly, the errata to RFC 5451 changed the "hardfail" result to "fail" (for RFC 4408 consistency)- but I wrote the patch to handle either.

I could care less what your patch looks like, really.

I asked because I seem to recall that years ago I tested this exact scenario and found that issues with DNS (such as no response to the query at all... not even a temp or perm error) caused false "none" results.

So instead of being a jerkwad, perhaps it'd be easier to just confirm if you can that everything fails safely now in the event of a busted resolver (all other DNS tests don't penalize mail in the event of a busted resolver... my recollection of "none" FPs did penalize mail and I think that's why it wasn't included at the time).  You know sometimes we weren't complete idiots when we looked at things the first time around.

If it were to happen that a DNS timeout caused "none" to be returned, then such would be an error inside the Mail::SPF function, which is outside of SA's control and also not the documented result of that perl module.

I can only assume that a perl module will act as documented.

kd6lvw+software@kd6lvw.ampr.org: Please add "[review]" to the beginning of the summary of this bug to indicate that it just needs votes and a commit.

(In reply to comment #13)
> If it were to happen that a DNS timeout caused "none" to be returned, then such
> would be an error inside the Mail::SPF function, which is outside of SA's
> control and also not the documented result of that perl module.
> 
> I can only assume that a perl module will act as documented.

OK, so politics aside, the patch is marked as alpha status but has been tested on a production system for 6 months.  Is this something we want to target for 3.3.X or for 3.4?

KAM

Except for the section at line 396 (for processing an "Authentication-Results:" header), the patch is pretty trivial, so I'm hoping for 3.3.2.  As for that non-trivial section, it pretty much copies the code for "Received-SPF" headers, with very few differences.

Note that the patch doesn't actually introduce the "SPF_NONE" rule into the ruleset, but is purely the code side of implementing such.

Since I authored it, I abstain from voting on it.

software+spamassassin@kd6lvw.ampr.org:  Who are you?  The lack of anything resembling a name on the left side of the email address you use here makes it hard for me to remember who you are.

Only committers get votes.  3 votes are required to commit a patch to a stable branch (currently 3.3).  If a patch author is a committer, he generally is assumed to be voting +1.

I'm actually not clear on what the requirement is for a patch from a non-committer to get into trunk (currently 3.4.0).  I don't actually see anything that says it requires votes, so maybe one committer can just commit it without any votes?
http://wiki.apache.org/spamassassin/DevelopmentMode
http://wiki.apache.org/spamassassin/ReleasePolicy

I'd like to see this make 3.3.2 or 3.3.3.  (I'm not a committer.)

(In reply to comment #17)
> I'm actually not clear on what the requirement is for a patch from a
> non-committer to get into trunk (currently 3.4.0).  I don't actually see
> anything that says it requires votes, so maybe one committer can just commit it
> without any votes?
> http://wiki.apache.org/spamassassin/DevelopmentMode
> http://wiki.apache.org/spamassassin/ReleasePolicy
> 
> I'd like to see this make 3.3.2 or 3.3.3.  (I'm not a committer.)

It depends on several things such as the trivial nature of the patch to see if it needs a CLA.

However, the patch appears trivial.  What we need is a committer who is willing to spearhead and commit the patch because that's what is needed for trunk.

I will do that.

Then they have to ask for votes for 3.3.2.

P.S., from the patch, his name is D. Stussy.

Back in a moment.

Committed to trunk.

Sending        lib/Mail/SpamAssassin/Plugin/SPF.pm
Transmitting file data .
Committed revision 1101130.

>  if ($hdr =~ /^Authentication-Results:.*;\s*SPF=([^;]*)/i) {

RFC 5451 allows whitespace and even a comment around '=':

  methodspec = [CFWS] method [CFWS] "=" [CFWS] result

so at least a \s* should be allowed, so perhaps:

  if ($hdr =~ /^Authentication-Results:.*;\s*SPF\s*=\s*([^;]*)/i) {



> if ($tmphdr =~
>    /^(pass|neutral|(?:hard|soft)?fail|none)(?:\b+smtp\.(\S+)=[^;]+)?/i) {

What is the \b+ supposed to do?
The RFC 5451 requires some separation between methodspec and reasonspec:

      resinfo = [CFWS] ";" methodspec [ CFWS reasonspec ]

the optional reasonspec in the above regexp would never match.

Also the propspec permits CFWS before the '=' :

     propspec = ptype [CFWS] "." [CFWS] property [CFWS] "=" pvalue

so, perhaps the following should do as a quick fix (but does not work
if a comment includes semicolons):

  if ($tmphdr =~ /^(pass|neutral|(?:hard|soft)?fail|none)(?:[^;]*?\bsmtp\.(\S+)\s*=[^;]+)?/i) {

(untested)

next approximation:
- suggestions from comment 20
- indentation
- if -> elsif
- local($1,$2), just in case

trunk:
$ svn ci -m '
 Bug 6490: Mail::SpamAssassin::Plugin::SPF - Two enhancement issues (some tweaks)
 Sending lib/Mail/SpamAssassin/Plugin/SPF.pm
Committed revision 1101160.

Adjusting the regex:  If you think it needs additional items which collapse out (i.e. * = 0 or more, especially the whitespace), not a problem with me.

Although the syntax may allow comments in "unlikely" places, in practice, they aren't seen.

trunk:
  $ svn ci -m '
  Bug 6490: Mail::SpamAssassin::Plugin::SPF - Two enhancement issues -
  - (fix a warning: "my" variable $result masks earlier declaration)'
  Sending lib/Mail/SpamAssassin/Plugin/SPF.pm
Committed revision 1101249.

Created attachment 4882 [details]
For ease of inspection, here is the combined patch of what went into trunk

+1

(Daryl's comment about testing the SPF_NONE is valid, but as long as
there is no such rule in the package (just the infrastructure support
for it), there is no immediate danger of DNS failures causing false
reports)

(In reply to comment #24)
> Created attachment 4882 [details]
> For ease of inspection, here is the combined patch of what went into trunk
> 
> +1
> 
> (Daryl's comment about testing the SPF_NONE is valid, but as long as
> there is no such rule in the package (just the infrastructure support
> for it), there is no immediate danger of DNS failures causing false
> reports)

+1 as well for 3.3.

+1.

I don't see any existing rules that need modifications from this.

My first pass looked for negated SPF hits: 

    grep -riP '!(?:\([^)]*)?\s*\w*spf' trunk/rules*

second pass, less thoroughly reviewed:

    grep -ri spf trunk/rules*

That looks like sufficient votes to commit to 3.3.2: Mark, Kevin, and Adam?

3.3:
  Bug 6490: Mail::SpamAssassin::Plugin::SPF - Two enhancement issues'
  Sending lib/Mail/SpamAssassin/Plugin/SPF.pm
Committed revision 1101815.


> I don't see any existing rules that need modifications from this.

I don't think there would be any.

(In reply to comment #28)
> 3.3:
>   Bug 6490: Mail::SpamAssassin::Plugin::SPF - Two enhancement issues'
>   Sending lib/Mail/SpamAssassin/Plugin/SPF.pm
> Committed revision 1101815.
> 
> 
> > I don't see any existing rules that need modifications from this.
> 
> I don't think there would be any.

Should we open a new bug for the rule for SPF_NONE(In reply to comment #28)
> 3.3:
>   Bug 6490: Mail::SpamAssassin::Plugin::SPF - Two enhancement issues'
>   Sending lib/Mail/SpamAssassin/Plugin/SPF.pm
> Committed revision 1101815.
> 
> 
> > I don't see any existing rules that need modifications from this.
> 
> I don't think there would be any.

I've added the disabled rule for spf_none to v340.pre in trunk.

KAM

Why didn't you just add the SPF_NONE rule, enabled?

(In reply to comment #30)
> Why didn't you just add the SPF_NONE rule, enabled?

A) The original ticket comment adds the rule with a score of 0.0 which is effectively disabled.

B) I personally don't think senders should be penalized for not implementing SPF. I recommend it, though.

Regards,
KAM

*** Bug 6586 has been marked as a duplicate of this bug. ***

There are lots of things that are entirely legitimate that SA penalizes for (the rules for no brackets in To: come to mind), but that doesn't mean they don't usefully correlate to spamminess.  I think SPF_NONE should be added and it's score floated, unless it is shown to have a problematic rate of false positives.  Just like everything else.

(In reply to comment #33)
> There are lots of things that are entirely legitimate that SA penalizes for
> (the rules for no brackets in To: come to mind), but that doesn't mean they
> don't usefully correlate to spamminess.  I think SPF_NONE should be added and
> it's score floated, unless it is shown to have a problematic rate of false
> positives.  Just like everything else.

Good argument.  A small 0.1 score to start encouraging people to use SPF isn't going to ruffle my feathers.

I recommend you add the rule for masscheck to see how things pan out.  I was simply trying to implement the consensus that was clear in the ticket to complete the issue.

> I've added the disabled rule for spf_none to v340.pre in trunk.

You shouldn't add rules, even disabled, to .pre files.

Add this to a .cf file.

(In reply to comment #35)
> > I've added the disabled rule for spf_none to v340.pre in trunk.
> 
> You shouldn't add rules, even disabled, to .pre files.
> 
> Add this to a .cf file.

I was following the administrative concepts that were discussed with bug 6526 and implemented with v332.pre.  

I agree that pre might be the wrong places.  Perhaps move these to a v332.cf and a v340.cf that is specifically for rules and configuration that are disabled that admins might consider?

Or is there an appropriate existing place?  

KAM

As the author:  I am satisfied with the code which was committed.

There are no existing rules which are affected by this change.  Furthermore, because it is debatable whether an unauthenticated sender should be penalized, I suggested a "SPF_NONE" rule in the OP with a zero score.  This need not actually be added to the formal ruleset for that reason.  Some may feel that "unauthenicated" should mean possessing NO sender authentication at all, and therefore, an SPF "none" result should be matrixed with similar "none" results from DKIM and PGP (of which we do have other modules to check for such signatures) before penalizing (if at all) the message for using nothing to verify its sender.

This is why I decided on proposing only the infrastructure patch to the code.  I expect that those who will use the the information of this additional result state will do so in their local configuration files.  I use a rule with a score of 2.0 (and a threshold of 7.5, not the default 5.0).  However, I also check for SPF in the MTA at "mail from" so my implementation will never pass to SA a message which SPF hardfails or errs, but all other results (pass, none, neutral, softfail, and policy [RFC 5451]) do get passed.  (I generate a policy result when I have a pre-registered, authenticated forwarding MTA delivering a message.  I don't rely on a forwarder implementing SRS, etc....)

Internally, I note that a "policy" result is treated the same as the error results - no state is set.  That is intentional as it means that the true SPF state was overridden for some reason (usually forwarding).

As for new rules, I suggest that if anything is added at this time, it be added as COMMENTS so as to alert the SA user population that the ability to act on SPF=none exists but that no active rule is being implemented at this time (as opposed to a proposal of adding a rule with a score of 0.1 or other trivial amount).  An actual score for a global distribution of the rule may be determined at a later date by testing to see how often it fires.  This rule is likely to fire on "ham" as well as spam as it is an indicator of an orthogonal property which is often seen with spam; not spammy by itself.  Testing on the rule is needed before setting a non-zero score.

(In reply to comment #28)
> > I don't see any existing rules that need modifications from this.
> 
> I don't think there would be any.

I didn't either, but it didn't hurt to look.  I was specifically looking for things that constructed an (invalidly) assumed SPF_NONE by means of negating other SPF rules.

(In reply to comment #35)
> > I've added the disabled rule for spf_none to v340.pre in trunk.
> 
> You shouldn't add rules, even disabled, to .pre files.
> 
> Add this to a .cf file.

I agree with Michael.

If we're adding the code, we might as well add the rule.  Score it 0.001 or else make it a __RULE, though that would require figuring out how to use it as a dependency.  Should be useful as anti-phish.

(In reply to comment #35)
> > I've added the disabled rule for spf_none to v340.pre in trunk.
> You shouldn't add rules, even disabled, to .pre files.

I fully agree. A .pre file is for plugins and possibly for
fixing some other anomaly, not for rules or scores.
Re-opened the Bug 6526 for this same reason.


(In reply to comment 37)
> As the author:  I am satisfied with the code which was committed.
> There are no existing rules which are affected by this change.  Furthermore,
> because it is debatable whether an unauthenticated sender should be penalized,
> I suggested a "SPF_NONE" rule in the OP with a zero score.  This need not
> actually be added to the formal ruleset for that reason.

Agreed. I don't feel any need for adding the SPF_NONE to the formal ruleset,
even with score 0 or even when commented out - let alone be left to float.
It should suffice for the infrastructure support to be there, in case
someone really needs it.

SPF may be beneficial for sites that only send direct mail, but it should
not be forced onto sites whose users post to arbitrary destinations, like
third party forwarders or mailing list, over which a site has no control
and which may or may note implement SRS. Also Daryl's concern about testing
for the handling of DNS failures is still unproven.

Pre Files removed for v332 and v340 and added to local.cf for consideration in trunk.  Removeing v332.pre in 3.3 branch as that's clearly wrong. 

3.3 branch:
Deleting       rules/v332.pre

Committed revision 1102298.

Trunk:
Sending        rules/local.cf
Deleting       rules/v332.pre
Deleting       rules/v340.pre
Transmitting file data .
Committed revision 1102296.

Rules commented out in local.cf in trunk and will make this comment on 6526 which is open.  This makes the issue "solved" for 3.3.2.

> 3.3 branch:
>   Deleting       rules/v332.pre
> Trunk:
>   Deleting       rules/v332.pre

Thanks.


> Trunk:
> Deleting       rules/v340.pre

The v340.pre is needed because of its:
  loadplugin Mail::SpamAssassin::Plugin::AskDNS
Please put it back to trunk, just remove the SPF_NONE rule from it.

> > Trunk:
> > Deleting       rules/v340.pre
> 
> The v340.pre is needed because of its:
>   loadplugin Mail::SpamAssassin::Plugin::AskDNS
> Please put it back to trunk, just remove the SPF_NONE rule from it.

Good catch.

Adding         rules/v340.pre
Transmitting file data .
Committed revision 1102306.

Created attachment 4928 [details]
Remove gunk from trunk's local.cf

svn commit -m 'remove SPF_NONE now that it is documented on http://wiki.apache.org/spamassassin/RemovedRulesets'                    
Sending        rules/local.cf
Transmitting file data .
Committed revision 1139018.

Re:  http://wiki.apache.org/spamassassin/RemovedRulesets

I concur and appreciate this listing (both here and for #6526 - RFC Ignorant).

[Now if only the apache.org DNS administrator would insert an SPF record for bugzilla's use, the world would be one step closer to perfect.]  ;-)

Re - SPF:  I simply find it ironic that an anti-spam oriented organization with such a product as SA wouldn't take steps to validate mail it sends out as non-forged.  SPF seems to be the easiest of all (DK/DKIM and PGP so far) to implement.

Personally, I assign an SA score of zero to SPF=PASS and positive scores to all the other non-error results detectable (fail, softfail, neutral, and none).  However, for SPF=NONE, the score I assigned has not once tipped the classification scale into the spammy range, although it is enough to trip my threshold for attaching an SA report to mail.

This is a matter of principle.  Nothing more.

(In reply to comment #46)
> Re - SPF:  I simply find it ironic that an anti-spam oriented organization with
> such a product as SA wouldn't take steps to validate mail it sends out as
> non-forged.  SPF seems to be the easiest of all (DK/DKIM and PGP so far) to
> implement.
> 
> Personally, I assign an SA score of zero to SPF=PASS and positive scores to all
> the other non-error results detectable (fail, softfail, neutral, and none). 
> However, for SPF=NONE, the score I assigned has not once tipped the
> classification scale into the spammy range, although it is enough to trip my
> threshold for attaching an SA report to mail.
> 
> This is a matter of principle.  Nothing more.

Mostly it's the fate of a cobbler's kid.  Since INFRA has plenty to do, I'm choosing not to saddle them with the requests since there is no gain, sorry.

*** Bug 6899 has been marked as a duplicate of this bug. ***