Bug 6759 - "ESMTPA;" not detected by Received.pm (again)
Summary: "ESMTPA;" not detected by Received.pm (again)
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: 3.3.2
Hardware: All Linux
: P2 normal
Target Milestone: 3.4.0
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-15 21:35 UTC by Alex
Modified: 2012-05-15 12:10 UTC (History)
2 users (show)


Attachment Type Modified Status Actions Submitter/CLA Status
deal with semicolon, recognize newer standard protocols for auth mail submission patch None Mark Martinec [HasCLA]
change expected auth result of three test cases in t/rcvd_parser.t patch None Mark Martinec [HasCLA]
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alex 2012-02-15 21:35:06 UTC 
r447014 reverted r157052 from bug 4184

Postfix sometimes(!?) adds a semicolon after "with ESMTPA".
r447014 should delete any ";" on the end of line but somehow it doesn't.

I patched line 391 of Received.pm

< if (/ by / && / with (ESMTPA|ESMTPSA|LMTPA|LMTPSA|ASMTP|HTTPU?)(?: |$)/i) {
> if (/ by / && / with (ESMTPA|ESMTPSA|LMTPA|LMTPSA|ASMTP|HTTPU?)(?: |$|;)/i) {

Before:
X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on
        mail.wissler-group.com
X-Spam-Flag: YES
X-Spam-Level: **********
X-Spam-Status: Yes, score=11.0 required=5.0 tests=DYN_RDNS_AND_INLINE_IMAGE,
        DYN_RDNS_SHORT_HELO_HTML,DYN_RDNS_SHORT_HELO_IMAGE,FSL_HELO_NON_FQDN_1,
        HELO_NO_DOMAIN,HTML_MESSAGE,RCVD_IN_PBL,RCVD_IN_RP_RNBL,RCVD_IN_SEMBLACK,
        RDNS_DYNAMIC,SHORT_HELO_AND_INLINE_IMAGE,TVD_RCVD_SINGLE,
        T_DOS_OUTLOOK_TO_MX_IMAGE,T_FRT_CONTACT autolearn=no version=3.3.2
Received: from AGNBJL (ip-2-204-127-124.web.vodafone.de [2.204.127.124])
        by mail.wissler-group.com (Postfix) with ESMTPSA;
        Wed, 15 Feb 2012 09:24:57 +0100 (CET)


After:
X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on
        mail.wissler-group.com
X-Spam-Level:
X-Spam-Status: No, score=0.2 required=5.0 tests=ALL_TRUSTED,HTML_MESSAGE,
        TVD_RCVD_SINGLE,T_FRT_CONTACT autolearn=no version=3.3.2
Received: from AGNBJL (ip-2-204-127-124.web.vodafone.de [2.204.127.124])
        by mail.wissler-group.com (Postfix) with ESMTPSA;
        Wed, 15 Feb 2012 09:24:57 +0100 (CET)


Maybe this should be solved in Postfix (2.8.3). One could also use "smtpd_sasl_authenticated_header = yes", yet what does it harm adding a test for the semicolon in spamassassin?

Alex
Comment 1 D. Stussy 2012-02-16 04:09:39 UTC 
ASMTP?  HTTPU?  WTF?  Those aren't listed at http://www.iana.org/assignments/mail-parameters.  Regardless, the regex could be written much better:

  "(?:(?:(?:D|E|UTF8)?(?:L|S)MTP8?|(?:HT|NN)TP)S?A?|MMS)"

I think that covers everthing that the IANA says is legal plus webmail and Usenet gateways (authenticated and/or SSL'ed or not).

However, I do agree that a semicolon need be added as the "with" clause could be the last before the date field.
Comment 2 Kevin A. McGrail 2012-02-19 14:27:21 UTC 
(In reply to comment #1)
> ASMTP?  HTTPU?  WTF?  Those aren't listed at
> http://www.iana.org/assignments/mail-parameters.  Regardless, the regex could
> be written much better:
> 
>   "(?:(?:(?:D|E|UTF8)?(?:L|S)MTP8?|(?:HT|NN)TP)S?A?|MMS)"
> 
> I think that covers everthing that the IANA says is legal plus webmail and
> Usenet gateways (authenticated and/or SSL'ed or not).
> 
> However, I do agree that a semicolon need be added as the "with" clause could
> be the last before the date field.

What these need is examples added to the test rcvd_parser.t so that we have a make test framework on these.  Can you please look for real-world examples to add?
Comment 3 D. Stussy 2012-02-19 20:05:45 UTC 
As far as the addition of the semicolon to the end of the regex goes, why do we need a real-world example if such is perfectly legal given the ABNF syntax in RFC 5321?  If the "id" and "for" clauses are omitted, the semicolon will indeed trail the "with" clause's parameter.  However, as a semicolon followed by a date is always required per the syntax (cf. RFC 5322), the "$" to terminate the string should NEVER occur in a properly formatted message.  Even if we're testing a wrapped header without unwrapping it, the semicolon will appear BEFORE the newline+tab wrapping the date on the next line.
Comment 4 Kevin A. McGrail 2012-02-20 14:27:17 UTC 
(In reply to comment #3)
> As far as the addition of the semicolon to the end of the regex goes, why do we
> need a real-world example if such is perfectly legal given the ABNF syntax in
> RFC 5321?  If the "id" and "for" clauses are omitted, the semicolon will indeed
> trail the "with" clause's parameter.  However, as a semicolon followed by a
> date is always required per the syntax (cf. RFC 5322), the "$" to terminate the
> string should NEVER occur in a properly formatted message.  Even if we're
> testing a wrapped header without unwrapping it, the semicolon will appear
> BEFORE the newline+tab wrapping the date on the next line.

I'd *prefer* real-world but will accept synthesized.

Overall though, if you add failure and success cases it will help avoid issues where things get forgotten because according to Alex, this bug was caused by an update that overrode an older bug.

Test cases would have solved that issue and they would prove the regexs work as intended both for success and failure.

Regards,
KAM
Comment 5 Mark Martinec 2012-04-24 19:24:03 UTC 
Created attachment 5057 [details]
deal with semicolon, recognize newer standard protocols for auth mail submission

http://www.iana.org/assignments/mail-parameters
Sub-registry: WITH protocol types
  (only the authenticated subset of protocol types)
Comment 6 Mark Martinec 2012-05-14 14:17:01 UTC 
trunk (3.4.0):

Bug 6759: "ESMTPA;" not detected by Received.pm
  (recognize semicolon as a terminator)
  Sending lib/Mail/SpamAssassin/Message/Metadata/Received.pm
Committed revision 1338216.
Comment 7 Mark Martinec 2012-05-14 22:54:04 UTC 
Created attachment 5063 [details]
change expected auth result of three test cases in t/rcvd_parser.t

Kevin wrote:
> I think you might have broken things with the Received.pm test.
> I'm currently not able to do a make test on trunk either.

Thanks. Actually I think that three test cases in t/rcvd_parser.t
are broken (wrong expected 'auth' result), and the fix just exposed
the problem.

Attached is a change to these test cases in t/rcvd_parser.t,
please review if my assumption is correct.


trunk (3.4.0):

Bug 6759: "ESMTPA;" not detected by Received.pm - fix
three test cases in rcvd_parser.t
  Sending t/rcvd_parser.t
Committed revision 1338477.
Comment 8 Kevin A. McGrail 2012-05-15 12:10:58 UTC 
Agreed.  The auth=HTTP certainly makes sense to me and the tests look good.