You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spamassassin.apache.org by bu...@bugzilla.spamassassin.org on 2018/09/01 22:27:24 UTC
[Bug 7610] New: Don't say "DKIM-Signature header exists but is not
valid" just because module missing!
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7610
Bug ID: 7610
Summary: Don't say "DKIM-Signature header exists but is not
valid" just because module missing!
Product: Spamassassin
Version: SVN Trunk (Latest Devel Version)
Hardware: PC
OS: Linux
Status: NEW
Severity: normal
Priority: P2
Component: Plugins
Assignee: dev@spamassassin.apache.org
Reporter: jidanni@jidanni.org
Target Milestone: Undefined
It is just plain wrong to say
0.0 T_DKIM_INVALID DKIM-Signature header exists but is not valid
even if the score is 0, just because the
optional Mail::DKIM Perl module is not installed.
http://spamassassin.1065346.n5.nabble.com/I-m-getting-T-DKIM-INVALID-from-gmail-tp109464p109488.html
:
"DKIM_INVALID will hit on any DKIM signed message, if the
optional Mail::DKIM Perl module is not installed."
It is just like saying "the defendant is hereby pronounced guilty"
because the judge didn't show up today.
Even if you say "well it doesn't matter, because we gave the criminal a
sentence of 0 years, so who cares."
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7610] Don't say "DKIM-Signature header exists but is not valid"
just because module missing!
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7610
--- Comment #6 from John Hardin <jh...@impsec.org> ---
Just had a thought: if we remove the (useless) timing call from
_dkim_load_modules we can write the new sub this way:
sub is_mail_dkim_unavailable {
return !_dkim_load_modules();
}
I think that's the best approach overall...
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7610] Don't say "DKIM-Signature header exists but is not valid"
just because module missing!
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7610
--- Comment #2 from jidanni@jidanni.org ---
So say
0.0 T_DKIM_UNTESTED DKIM-Signature header exists but Mail::DKIM Perl
module is not installed to test it
or something, if anything.
And retire T_DKIM_INVALID which is a mess, and create a new
T_DKIM_REALLY_INVALID or something, to distinguish the real cases.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7610] Don't say "DKIM-Signature header exists but is not valid"
just because module missing!
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7610
--- Comment #9 from John Hardin <jh...@impsec.org> ---
Sorry, I got distracted by personal issues midway through that and never
returned to it.
Looks OK to me, I think.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7610] Don't say "DKIM-Signature header exists but is not valid"
just because module missing!
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7610
Henrik Krohns <he...@hege.li> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #8 from Henrik Krohns <he...@hege.li> ---
No comments, so DKIM_INVALID added to 25_dkim.cf as the meta mentioned above.
Test case added to check that either DKIM_VALID or DKIM_INVALID must always be
hit.
Sending spamassassin-3.4/t/dkim.t
Sending trunk/rules/25_dkim.cf
Sending trunk/rules/50_scores.cf
Sending trunk/rulesrc/sandbox/khopesh/20_khop_experimental.cf
Sending trunk/t/dkim.t
Transmitting file data .....done
Committing transaction...
Committed revision 1841821.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7610] Don't say "DKIM-Signature header exists but is not valid"
just because module missing!
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7610
Henrik Krohns <he...@hege.li> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |hege@hege.li
--- Comment #7 from Henrik Krohns <he...@hege.li> ---
If it's useful rule, why not simply move to rules/25_dkim.cf where it logically
belongs, instead of entertaining some strange sandbox meta rule?
There already exists everything needed for it:
full DKIM_SIGNED eval:check_dkim_signed()
full DKIM_VALID eval:check_dkim_valid()
Would this not work?
meta DKIM_INVALID DKIM_SIGNED && !DKIM_VALID
I guess it needs some score too.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7610] Don't say "DKIM-Signature header exists but is not valid"
just because module missing!
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7610
jidanni@jidanni.org changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jidanni@jidanni.org
--- Comment #1 from jidanni@jidanni.org ---
You might say "who cares" "next time remember to install the module".
But this could be the source of many people sending mail to many big companies'
security departments, reminding them to use valid headers next time, when all
along it is the sloppy wording of the testing program (spamassassin) to blame.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7610] Don't say "DKIM-Signature header exists but is not valid"
just because module missing!
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7610
Kevin A. McGrail <km...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |kmcgrail@apache.org
--- Comment #4 from Kevin A. McGrail <km...@apache.org> ---
A conditional wrapper sounds like a great idea.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7610] Don't say "DKIM-Signature header exists but is not valid"
just because module missing!
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7610
John Hardin <jh...@impsec.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jhardin@impsec.org
--- Comment #3 from John Hardin <jh...@impsec.org> ---
(In reply to jidanni from comment #0)
> It is just plain wrong to say
> 0.0 T_DKIM_INVALID DKIM-Signature header exists but is not valid
> even if the score is 0, just because the
> optional Mail::DKIM Perl module is not installed.
>
> http://spamassassin.1065346.n5.nabble.com/I-m-getting-T-DKIM-INVALID-from-
> gmail-tp109464p109488.html :
>
> "DKIM_INVALID will hit on any DKIM signed message, if the
> optional Mail::DKIM Perl module is not installed."
Perhaps an "if can(something)" can be put around that rule definition to pick
which message to display based on whether the module is installed?
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7610] Don't say "DKIM-Signature header exists but is not valid"
just because module missing!
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7610
--- Comment #5 from John Hardin <jh...@impsec.org> ---
The rules parsing occurs before the DKIM module first tries to "require
Mail::DKIM::Verifier", and the sub that loads Mail::DKIM has a call to the SA
log-the-time utility which doesn't (in the current form) work from can():
warn: config: error in if - $self->cond_clause_can(
"Mail::SpamAssassin::Plugin::DKIM::_dkim_load_modules" ) : Can't call method
"can" on an undefined value at lib/Mail/SpamAssassin/Plugin/DKIM.pm line 672.
671 my $timemethod = $self->{main}->UNIVERSAL::can("time_method") &&
672 $self->{main}->time_method("dkim_load_modules");
So a code change will be needed somewhere to support this. The options I see:
(1) My perl is way rusty so I don't offhand know how to fix that timing call so
it would work in that situation - advice?
(2) Lose the timing call in _dkim_load_modules so we can do
"can(Mail::SpamAssassin::Plugin::DKIM::_dkim_load_modules)"
(3) Add a much simpler can-able sub like:
sub is_mail_dkim_unavailable {
return !eval { require Mail::DKIM::Verifier; }
}
...which has the side-effect of loading the module and rendering the timing
measurement from the actual call to _dkim_load_modules useless.
Which approach is preferable?
I think the new sub() is preferable because that approach doesn't generate an
error from can(Mail::SpamAssassin::Plugin::DKIM::_dkim_load_modules) if (when)
the rules get run against an older version of SA. Perhaps also with removing
the timing call from _dkim_load_modules since it's now pointless...
Which would lead to this in the rules file:
# This is useless while __DKIM_EXISTS continues to perform so well.
# This is useless while ruleqa continues to lack DKIM support.
ifplugin Mail::SpamAssassin::Plugin::DKIM
meta DKIM_INVALID __DKIM_EXISTS && !DKIM_VALID
if can(Mail::SpamAssassin::Plugin::DKIM::is_mail_dkim_unavailable)
describe DKIM_INVALID Mail::DKIM module unavailable, cannot verify
DKIM header
else
describe DKIM_INVALID DKIM-Signature header exists but is not valid
endif
# masscheck ignores ifplugins and thus always has DKIM_INVALID==__DKIM_EXISTS
endif
Comments solicited before I make any commits.
--
You are receiving this mail because:
You are the assignee for the bug.