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 2007/09/28 21:02:27 UTC
[Bug 5662] New: DKIM plugin overhaul - whitelisting and terminology
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
Summary: DKIM plugin overhaul - whitelisting and terminology
Product: Spamassassin
Version: SVN Trunk (Latest Devel Version)
Platform: All
OS/Version: All
Status: NEW
Severity: normal
Priority: P5
Component: Libraries
AssignedTo: dev@spamassassin.apache.org
ReportedBy: Mark.Martinec@ijs.si
As promised on a mailing list:
I would still like to clean a terminology mess (misinterpretation
of what 'identity' is) in DKIM plugin (debug, comments, POD)
and reduce its logging clutter. I think it should go to 3.2.4,
working on it...
As it turned out, the whitelisting code needed reworking too
to match the concepts, which are now cleaner than few years ago
when DKIM was still mostly vaporware.
Sending lib/Mail/SpamAssassin/Plugin/DKIM.pm
Committed revision 580453 (to trunk)
Cleaned concepts in a DKIM plugin, previous interpretation of
"identity" was incompatible with the current DKIM standard RFC 4871
and SSP, and was a mixture from DomainKeys and whitelist_from_rcvd.
Added substantial new documentation (POD), and updated comments
and debug output to use the same terminology. Reduced debug logging
clutter. Reworked whitelisting to match the new documentation
and expected use as per SSP draft. Tested whitelisting based on
originator signature as well as based on third-party signature.
Prepare grounds for messages with multiple signatures (although
Mail::DKIM does not support that yet, but is in the works).
Is there a chance of pushing it into 3.2.4? I don't think there
are any other differences in this module between HEAD and 3.2.3,
so it should be just copied across.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-10-10 11:39 -------
Created an attachment (id=4152)
--> (http://issues.apache.org/SpamAssassin/attachment.cgi?id=4152&action=view)
proposed patch for 3.2.4
The patch transplants the current trunk version of a DKIM plugin
to 3.2.3, the only difference is removed two calls to a timing module
which does not exist in 3.2.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
sidney@sidney.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Status Whiteboard|needs 1 vote for 3.2 |needs 1 vote for 3.2 and a
| |-1 from Daryl resolved
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-12-24 07:11 -------
Comment #10 From Daryl C. W. O'Shea
> - $scan->{match_in_whitelist_auth} could be named better so that it's
> not open to easy naming collision with development in other modules;
> perhaps just prepend "dkim_" to "match_in_whitelist_"
Fixed.
Comment #14 From Daryl C. W. O'Shea
> You have to disable the WHITELIST_FROM_DKIM rule (and the DEF_ one), too, to
> completely disable DKIM checks. I had been supplying a patch to some ESP
> customers that only ran the DKIM check (do the crypto and DNS checks) if the
> address appeared in the DKIM whitelist config. It makes a significant
> difference it processing time (at least wall clock wise). I thought
> I had merged this into our codebase, it turns out I never did. [...]
>From myself:
> This hasn't changed. In 3.2.3, the DKIM check is performed even if
> DKIM_SIGNED, DKIM_VERIFIED, DKIM_POLICY_SIGNALL, DKIM_POLICY_SIGNSOME,
> and DKIM_POLICY_TESTING are zero, as long as USER_IN_DKIM_WHITELIST
> or USER_IN_DEF_DKIM_WL are nonzero - even if a sender address is NOT
> found in a whitelist. So are you asking to make it a new feature?
>From Daryl:
> Sure, for 3.3. I honestly thought I had merged that into 3.1 way back,
> sorry. Ignore this concern too.
Actually, it turned out it was really easy to do so with the current code.
In _check_dkim_whitelist() just had to move collecting of applicable wl
addresses in front of a call to check_dkim_valid, conditionalized only
if the list of applicable wl entries is nonempty. (by applicable I mean:
entries matching the From address pattern (first parameter) in a wl entry,
but not yet checked whether the signature is valid or whether the signing
identity matches the second parameter in a wl entry).
> while I'm thinking of it... we really need to revert the
> change to the DKIM_POLICY_SIGNSOME eval that causes it to use the default
> policy, rather than the explicit policy as the former is absolutely useless
Actually, that change (where an absent policy defaults to singsome, as it
should) occurred in the underlying Mail::DKIM, not in the Plugin::DKIM.
As the DKIM_POLICY_SIGNSOME is useless, either a rule score could be zeroed,
or the check_dkim_signsome eval could always return false. I chose the letter,
now in the most recent patch and in trunk.
> OK, I'm confused by your "Reworked whitelisting to match the new
> documentation and expected use as per SSP draft" statement in comment #0.
> Could you elaborate on that? Pointing out, in comment #5, that RFC 5016
> has been published led me to believe that there was something of substance
> to your initial comment.
Not related to RFC 5016, just mentioned it, considering someone might
find it interesting.
The underlying reason for my whitelisting change was a fact that SSP
distinguishes between a first-party signature (also called "author signature"
or now disfavoured "originator signature") (where From matches a signing
identity), apart from a third-party signatures (e.g. from a mailing list).
The old code was unable to express whitelisting based on a first-party
signature.
Reapeating from my comment #2:
- whitelisting first-party signatures based on a wildcarded domain
does not work, e.g.:
whitelist_from_dkim *@*.cam.ac.uk
This is because the identity as stored in a whitelist is '*.cam.ac.uk'
and does not literally match a signing identity e.g. 'department.cam.ac.uk'.
For checking first-party signatures a check must me made against the
specific author address in a mail which matched the wildcard address
(first parameter in whitelist_from_dkim).
Here is a summary of terminology changes (reflected in variable and
in sub names, in comments and in POD section):
- has a concept of a first-party signature (= author signature);
- recognize the fact that a message may have more than one signature;
- renamed verified -> valid (in a backwards compatible way);
semantics of 'verified' is that signatures went through a
verification process; the outcome of which is either valid,
failed/invalid, or none
- renamed $message to $verifier - it holds a DKIM verifier object
with its methods, not a message;
- use name 'author' consistently for an address in a From header filed,
for consistency with rfc4871, with SSP draft and rfc2822/2822upd;
- although the SSP draft still uses a term 'originator signature', this
term was found inconsistent with rfc2822 (where it covers From, Sender
and Reply-To header fields), so the DKIM WG now uses term author signature,
which is now used in the DKIM plugin;
Summary of functional changes:
- as per #2, whitelisting can imply first-party signatures (when a second
parameter is absent);
- as per #2, matching on identity (second parameter) was too permissive;
- disabled check_dkim_signsome (always returns false);
- supports multiple signatures for whitelisting (requires Mail::DKIM 0.29
or later);
- new eval rule check_dkim_valid_author_sig returns true if a message
contains a valid author signature (nor just any valid signature);
(this was an easy byproduct of a fix to: a policy lookup should not be
suppressed on 3rd-party signatures);
- new eval rule check_dkim_valid is an alias for check_dkim_verified misnomer;
- new tags available: _DKIMIDENTITY_ and _DKIMDOMAIN_;
- significant verification speedup on large messages by avoiding feeding
a message to Mail::DKIM line-by-line (speedup only on versions of
Mail::DKIM 0.29_1 and later, i.e. the pre-releases of 0.30)
When reviewing, I suggest to look at the resulting code (almost the same
as in trunk) and not to bother figuring out the diffs. Or just enable
the 'dkim' debug areas and watch the log.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-10-17 08:45 -------
FYI, the draft-ietf-dkim-ssp-requirements-05 has just been published
as RFC 5016.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
Mark.Martinec@ijs.si changed:
What |Removed |Added
----------------------------------------------------------------------------
Summary|DKIM plugin overhaul - |[review] DKIM plugin
|whitelisting and terminology|overhaul - whitelisting and
| |terminology
Status Whiteboard| |needs 1 vote for 3.2
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
spamassassin@dostech.ca changed:
What |Removed |Added
----------------------------------------------------------------------------
Status Whiteboard|needs a -1 from Daryl |needs 2 votes (+1'd from
|resolved for 3.2 branch |Daryl)
------- Additional Comments From spamassassin@dostech.ca 2007-12-26 00:17 -------
Excellent. You've now made it completely clear as to why things have been
changed. The opaque references to SSP in the initial comments really threw me
off. In the absence of *any* real design documentation I feel it's important
for us to clearly document design changes via bugzilla, especially when the
changes affect code that interacts with evolving external projects/specifications.
Good work Mark. Thanks for playing this out.
+1 on 4214
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From spamassassin@dostech.ca 2007-12-16 15:32 -------
Sorry for dragging my ass on this... I still haven't tried this out, I've only
looked over the patch quickly a couple weeks ago.
I few issues I've spotted so far:
- $scan->{dkim_key_testing} is always 0
- $scan->{match_in_whitelist_auth} could be named better so that it's not open
to easy naming collision with development in other modules; perhaps just
prepend "dkim_" to "match_in_whitelist_"
and what I'm really concerned about:
- it's now impossible to use DKIM for whitelisting only by setting the scores
for the DKIM rules to 0; this forces all DKIM signed messages to be
processed... something that isn't exactly light-weight; many, many ESPs are
extermely adverse to the crypto load incurred for (currently) no benefit
(DKIM pass/verified/whatever doesn't currently get you anything without
reputation or a whitelist) so not being able to restrict processing to
whitelist eligible messages is not good and definitely something I don't want
to see changed midway through a stable branch
- this, by the way, is why the non-intuitive whitelist loops seemed to be
inside out... I should have documented that, sorry :(
Also... I'm not sure exactly how much the plugin has changed to accommodate the
current SSP RFC, but with all of the recent debate about SSP issues on the DKIM
list I'm hesitant to make major SSP related changes at this time. Either an
outline by Mark about what exactly was changed in regards to SSP, or me getting
around to reviewing what was changed myself will be required to figure out
exactly how hesitant I am to make this change now, especially mid way through a
stable branch.
More also... while I'm thinking of it... we really need to revert the change to
the DKIM_POLICY_SIGNSOME eval that causes it to use the default policy, rather
than the explicit policy as the former is absolutely useless (at least without a
way for rules to differentiate between the former and latter). The current way
is as silly as having a rules like PLAIN_TEXT_EMAIL and NON_PLAIN_TEXT_EMAIL and
having PLAIN_TEXT_EMAIL fire regardless of whether there's a content type that
says its plain text. The current situation was brought on solely by a way too
literal interpretation of the DKIM RFC. SA rules shouldn't have to act as the
output of an RFC compliance validator.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From jm@jmason.org 2007-11-12 03:19 -------
+1, looks good to me (although I haven't spent as much time looking into
this as Daryl)
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-12-18 13:10 -------
The current model in SSP is to have three possible settings:
DKIM=unknown (Signature Optional)
DKIM=all (Signature Required, Any party)
DKIM=strict (Signature Required, 1st party only)
and the 'testing' flag in SSP seems to be going away.
Besides, there is a new "handling=process" vs. "handling=deny"
attribute of the SSP.
This means that:
- DKIM_POLICY_TESTING is redundant (will always be false);
- DKIM_POLICY_SIGNSOME is redundant (is always true);
- there is no equivalent rule to test for DKIM=strict,
which is the most useful setting (allowing to block
with a high spam score) - although nobody uses it yet.
For the future, rules like the following might be desired:
- DKIM_POLICY_STRICT (most needed)
- DKIM_POLICY_DENY (useful when combined with DKIM_POLICY_STRICT)
- DKIM_KEY_TESTING (or DKIM_SIGNATURE_TESTING) (rarely useful)
- DKIM_VALID (replacing a misnomer DKIM_VERIFIED)
- DKIM_VALID_AUTHOR (contains a valid first-party signature)
- DKIM_VALID_3RDPARTY (contains a valid 3rd-party signature)
- DKIM_VALID (replacing a misnomer DKIM_VERIFIED,
where its value could be an OR of the above two,
or just a synonym for DKIM_VALID_AUTHOR)
> Processed as in running the message through Mail::DKIM.
> Crypto and blocking DNS lookups are expensive.
The current version of Mail::DKIM runs 8 times faster than previously
(I needed that speedup too, and invested some time in optimization).
It turned out that the expensive part was passing small strings
through a chain of subroutine calls. The crypto part was pretty
much insignificant. But yes, the DNS lookup still takes wall-clock
time, although (along with a dozen of RBL lookups), but it does
not reduce achievable throughput, just to contributes to latency.
Fetching a SSP policy adds another DNS lookup. Considering the
current state of SSP usefulness (useless), considering that
Mail::DKIM does not currently implement fetching of SSP
according to any of its drafts (still uses rfc4870 rules,
waiting for dust to settle), I'd like to repeat a suggestion
I made some time ago:
The score for all three SSP-fetching rules should be set to 0:
(DKIM_POLICY_SIGNALL, DKIM_POLICY_SIGNSOME, DKIM_POLICY_TESTING).
This disables one redundant DNS lookup, and avoids the clutter
the DKIM_POLICY_SIGNSOME rule makes.
> Perhaps it should even be disabled by default as I really don't
> understand what the use of this functionality is and nobody's
> suggested a use yet.
I fully agree.
> OK, I'm confused by your "Reworked whitelisting..."
> Could you elaborate on that?
Will do (later).
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-10-02 04:14 -------
svn -m 'revise r580453, accomodating some of the concerns
in bug 5662#1 for Plugin::DKIM' ci
Sending lib/Mail/SpamAssassin/Plugin/DKIM.pm
Committed revision 581122.
There are two problems in the 3.2.3 DKIM which my changes are fixing:
- whitelisting first-party signatures based on a wildcarded domain
does not work, e.g.:
whitelist_from_dkim *@*.cam.ac.uk
This is because the identity as stored in a whitelist is '*.cam.ac.uk'
and does not literally match a signing identity e.g. 'department.cam.ac.uk'.
For checking first-party signatures a check must me made against the
specific originator address in a mail which matched the wildcard address
(first parameter in whitelist_from_dkim).
- the matching on identity (second parameter) was too permissive in case a
local part is non-empty. The subdomain matching logic was inappropriately
also used in a local part. An example of an inappropriate match:
the:
whitelist_from_dkim *.example.com surname@example.com
also matched a signing identity name.surname@example.com
> The change unnecessarily removes functionality
I have now restored matching on subdomains in a signing identity, e.g.
whitelist_from_dkim *@sub.example.com example.com
now works for signing identities @sub.example.com and @example.com.
> removes useful debugging info,
It is questionable how useful is a debug along the lines of:
'what would be if the message were slightly different'. For testing
purposes a user could just as well supply a valid signature. And why
stop walking the whitelist at a first match, it might be useful to see
what other entries match... Nevertheless, I put back the debugging in
the spirit of original code (which makes code somewhat more compliceted,
but never mind).
> has overly verbose (and thus confusing) documentation,
> uses a number or real domains in examples (and those
> domains don't even support DKIM), references SSP which
> is still in limbo.
Dropped the 'BRIEF INTRODUCTION TO TERMINOLOGY' section
from a man page, changed examples, changed refs.
> and possibly hides something I've missed in the massive change
> of $scanner to $scan all over the place.
10 routines used $scan, and 3 used $scanner for the same object.
It was confusing to read - a change for consistency.
> My biggest issue is with only allowing for an exact match of the provided
> optional signing identity. At the very least you need to allow for
> sub domain globing.
Done, where the second parameter (identity) is provided.
An empty second parameter now implies first-party signature,
which needs to follow rules in RFCs.
It is compatible with the expected usage and with some example lists
that were posted on SA-user mailing list on two or three occasions.
> Additionally, when an optional signing identity is provided there
> shouldn't be any fallback to using the originator identity.
You are quite right. Fixed.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-12-28 09:35 -------
> > Doesn't Mark, Daryl, and Justin make 3 votes total?
> Yes, yes it does.
Commited to 3.2:
svn -m 'bug 5662: recognize author signature and multiple
signatures for whitelisting (with Mail::DKIM 0.29);
disable useless "check_dkim_signsome";
new eval rules "check_dkim_valid_author_sig" and
"check_dkim_valid" (an alias for a "check_dkim_verified" misnomer);
new tags _DKIMIDENTITY_ and _DKIMDOMAIN_; updated terminology;
verification speedup with Mail::DKIM 0.30 (or its pre-releases)' ci
Sending lib/Mail/SpamAssassin/Plugin/DKIM.pm
Transmitting file data .
Committed revision 607297.
> dkim.org dropped their "testing1234" selector, so the tests (which all
> depend on it) stopped working a while back. At some point we should
> probably set up our own.
Is there any simple way of adding one or two or three TXT records
under domain spamassassin.org (or maybe apache.org)?
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
sidney@sidney.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Status Whiteboard|can be committed for 3.2 |needs a -1 from Daryl
|and a -1 from Daryl resolved|resolved for 3.2 branch
------- Additional Comments From sidney@sidney.com 2007-12-22 16:48 -------
I changed the whiteboard status to make it more clear that approval would
require a vote from Daryl specifically to remove his -1 vote. If that means that
Mark comes up with a new patch which Daryl then agrees resolves his concerns, we
will be back to requiring 1 more vote.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
Mark.Martinec@ijs.si changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #4186 is|0 |1
obsolete| |
Attachment #4192 is|0 |1
obsolete| |
------- Additional Comments From Mark.Martinec@ijs.si 2007-12-24 05:58 -------
Created an attachment (id=4214)
--> (http://issues.apache.org/SpamAssassin/attachment.cgi?id=4214&action=view)
merged-in the r606658, r606698, r606699
trunk and 3.2 in sync again, apart from time_method and use re 'taint'
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From spamassassin@dostech.ca 2007-12-17 12:42 -------
Uh crap, it seems my memory is failing me lately. :o
(In reply to comment #11)
> (In reply to comment #10)
> > - it's now impossible to use DKIM for whitelisting only by setting the scores
> > for the DKIM rules to 0;
>
> Daryl, can you explain this? Are you saying it's possible to run DKIM checks
> even if the rules are set to 0 scores? because I'd prefer that wasn't the case;
> 0-scoring is the "standard" way to disable a set of rules, after all.
You have to disable the WHITELIST_FROM_DKIM rule (and the DEF_ one), too, to
completely disable DKIM checks. I had been supplying a patch to some ESP
customers that only ran the DKIM check (do the crypto and DNS checks) if the
address appeared in the DKIM whitelist config. It makes a significant
difference it processing time (at least wall clock wise). I thought I had
merged this into our codebase, it turns out I never did. Sorry for the confusion.
> > this forces all DKIM signed messages to be
> > processed... something that isn't exactly light-weight;
>
> "processed" in what way? do we not already perform some lookups?
Processed as in running the message through Mail::DKIM. Crypto and blocking DNS
lookups are expensive -- but don't actually change with the proposed patch.
(In reply to comment #12)
> > I few issues I've spotted so far:
> > - $scan->{dkim_key_testing} is always 0
>
> Yes. Same as in 3.2.3 - I haven't touched it. The 3.1.8 didn't
> even have it. All past versions only checked for a policy testing
> flag, never for a public key testing flag.
>
> Some time in 3.2 I split an internal variable dkim_testing into
> its two components dkim_key_testing and dkim_policy_testing,
> but kept the functionality unchanged. This is why it now became
> more obvious that the dkim_policy_testing is always 0.
> So, are you asking for a change there, or should the old behaviour
> be retained for 3.2?
OK, I see you made that change in r515298. I had just applied the patch and
then reviewed the resulting version by comparing it to how I remembered it was
before (apparently that wasn't a great idea!). The conditional on something
that is always 0 threw me off. In any case you can disregard this concern too.
> This hasn't changed. In 3.2.3, the DKIM check is performed even if
> DKIM_SIGNED, DKIM_VERIFIED, DKIM_POLICY_SIGNALL, DKIM_POLICY_SIGNSOME,
> and DKIM_POLICY_TESTING are zero, as long as USER_IN_DKIM_WHITELIST
> or USER_IN_DEF_DKIM_WL are nonzero - even if a sender address is NOT
> found in a whitelist.
>
> So are you asking to make it a new feature?
Sure, for 3.3. I honestly thought I had merged that into 3.1 way back, sorry.
Ignore this concern too.
> > Also... I'm not sure exactly how much the plugin has changed to accommodate
> > the current SSP RFC, but with all of the recent debate about SSP issues on
> > the DKIM list I'm hesitant to make major SSP related changes at this time.
>
> No changes in that area. The SSP specs are still very much volatile,
> and it will be a while before it comes to a more wide spread use.
> Also the format has changed in the recent draft, and Mail::DKIM
> hasn't adapted yet as far as I know.
OK, I'm confused by your "Reworked whitelisting to match the new documentation
and expected use as per SSP draft" statement in comment #0. Could you elaborate
on that? Pointing out, in comment #5, that RFC 5016 has been published led me
to believe that there was something of substance to your initial comment.
> > More also... while I'm thinking of it... we really need to revert the
> > change to the DKIM_POLICY_SIGNSOME eval that causes it to use the default
> > policy, rather than the explicit policy as the former is absolutely useless
> > (at least without a way for rules to differentiate between the former and
> > latter).
>
> If SSP says the absence of a policy is equivalent to certain settings
> of a policy, then signing domains which want to use such default settings
> are free to either publish it or not publish it. Not publishing what is a
> default must not be punished. It is not unusual that signing domains choose
> not to clutter their DNS space with what is a default anyway, especially
> as the SSP draft is still evolving.
There's no punishment of senders by not causing an informational rule that
scores ~0 to fire on damn near every email received. The only punishment is to
the receivers who it annoys the hell out of. I'm not saying that internally we
shouldn't observe a default policy when its appropriate to do so, just that we
don't need to add a rule in every email received (that the sender doesn't define
a different policy or doesn't even use DKIM) telling the recipient what the
defaults of some RFC is.
I propose that we restore the SIGNSOME rule to where it only fires on an
explicit sign some policy and add a new rule called DKIM_POLICY_SIGNSOME_DEFAULT
(or whatever) and have that fire when a default sign some policy is assumed.
This way you can retain whatever benefit of this rule is perceived by what I'm
sure is a small minority and everyone else can disable it. Perhaps it should
even be disabled by default as I really don't understand what the use of this
functionality is and nobody's suggested a use yet. It's not like we fire
SPF_NEUTRAL (the default policy) in lieu of an explicit SPF record and nobody's
asked us to do so going back to 3.0.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-12-17 06:52 -------
> I few issues I've spotted so far:
> - $scan->{dkim_key_testing} is always 0
Yes. Same as in 3.2.3 - I haven't touched it. The 3.1.8 didn't
even have it. All past versions only checked for a policy testing
flag, never for a public key testing flag.
Some time in 3.2 I split an internal variable dkim_testing into
its two components dkim_key_testing and dkim_policy_testing,
but kept the functionality unchanged. This is why it now became
more obvious that the dkim_policy_testing is always 0.
So, are you asking for a change there, or should the old behaviour
be retained for 3.2?
> - $scan->{match_in_whitelist_auth} could be named better
> so that it's not open to easy naming collision with development
> in other modules; perhaps just prepend "dkim_" to "match_in_whitelist_"
Sure.
> and what I'm really concerned about:
>
>- it's now impossible to use DKIM for whitelisting only by setting the scores
> for the DKIM rules to 0; this forces all DKIM signed messages to be
> processed... something that isn't exactly light-weight; many, many ESPs are
> extermely adverse to the crypto load incurred for (currently) no benefit
> (DKIM pass/verified/whatever doesn't currently get you anything without
> reputation or a whitelist) so not being able to restrict processing to
> whitelist eligible messages is not good and definitely something I don't
> want to see changed midway through a stable branch
This hasn't changed. In 3.2.3, the DKIM check is performed even if
DKIM_SIGNED, DKIM_VERIFIED, DKIM_POLICY_SIGNALL, DKIM_POLICY_SIGNSOME,
and DKIM_POLICY_TESTING are zero, as long as USER_IN_DKIM_WHITELIST
or USER_IN_DEF_DKIM_WL are nonzero - even if a sender address is NOT
found in a whitelist.
So are you asking to make it a new feature?
> Also... I'm not sure exactly how much the plugin has changed to accommodate
> the current SSP RFC, but with all of the recent debate about SSP issues on
> the DKIM list I'm hesitant to make major SSP related changes at this time.
No changes in that area. The SSP specs are still very much volatile,
and it will be a while before it comes to a more wide spread use.
Also the format has changed in the recent draft, and Mail::DKIM
hasn't adapted yet as far as I know.
> More also... while I'm thinking of it... we really need to revert the
> change to the DKIM_POLICY_SIGNSOME eval that causes it to use the default
> policy, rather than the explicit policy as the former is absolutely useless
> (at least without a way for rules to differentiate between the former and
> latter).
If SSP says the absence of a policy is equivalent to certain settings
of a policy, then signing domains which want to use such default settings
are free to either publish it or not publish it. Not publishing what is a
default must not be punished. It is not unusual that signing domains choose
not to clutter their DNS space with what is a default anyway, especially
as the SSP draft is still evolving.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-12-28 12:05 -------
> yep, it's relatively simple, just check in the changes under
> https://svn.apache.org/repos/asf/spamassassin/dns and I'll make
> them active.
Good.
> I was under the impression that we'd have to regenerate those test messages
> entirely, but if that can be avoided and we just "re-home" the selectors,
> great!
The signature in these messages would have to be recomputed to use a new
key. It is more of an ethical question whether these test messages should
be 're-cast', or it would be cleaner to just fabricate a couple of new
test cases to exercise some of the corner cases known to have caused
trouble in the past. I can play with this by the end of the next week,
unless somebody does it first.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
spamassassin@dostech.ca changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution| |FIXED
------- Additional Comments From spamassassin@dostech.ca 2007-12-28 14:21 -------
This bug has been resolved. I've opened bug 5760 for the DKIM test issue.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-12-23 19:02 -------
spamassassin-trunk$ svn -m "
DKIM: never trigger signsome to avoid useless rule firing;
optimize whitelisting as suggested by Daryl - don't invoke
verification when author address is not listed in wl;
add eval rules check_dkim_valid (alias for check_dkim_verified)
and check_dkim_valid_author_sig; speed-up feeding msg to verifier;
policy lookup should not be suppressed on 3rd-party sigs;
edit variable and subr. names, comments and docs" ci
Sending lib/Mail/SpamAssassin/Plugin/DKIM.pm
Transmitting file data .
Committed revision 606658.
(will comment tomorrow)
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From jm@jmason.org 2007-11-26 01:56 -------
(In reply to comment #8)>
> The resulting code is (again) the same as the current code
> in trunk, except for the commented-out calls to scoped timing,
> which does not exist in 3.2. I hope this last rather cosmetic
> change does not invalidate jm's vote, so this still leaves one
> to go, barring objections.
just to be clear: +1
> I noticed that later versions of Mail::DKIM do check for
> expiration time internally so the expiration check in the
> plugin is redundant. It does not hurt though, so I'd leave
> it as it is for the time being.
yes, there's always a possibility that some vendor may package
an older version of Mail::DKIM from before that expiration check
was added to their code.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
Mark.Martinec@ijs.si changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #4152 is|0 |1
obsolete| |
------- Additional Comments From Mark.Martinec@ijs.si 2007-11-09 10:05 -------
Created an attachment (id=4186)
--> (http://issues.apache.org/SpamAssassin/attachment.cgi?id=4186&action=view)
Merged-in the r593597 - support for Mail::DKIM 0.29
Make use of the prepared framework in the previous version of
the patch, now fully supporting whitelisting of messages with
multiple-signatures, when Mail::DKIM 0.29 or later is available.
r593597 (in trunk): Now that Mail::DKIM 0.29 has been released,
update Plugin::DKIM to use the new public API access to multiple
signatures (if available). The plugin will still only show one
signature with older versions of Mail::DKIM. Add tag DKIMDOMAIN
(in addition to DKIMIDENTITY).
Perhaps now the time is ripe for a review.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-11-23 17:33 -------
Created an attachment (id=4192)
--> (http://issues.apache.org/SpamAssassin/attachment.cgi?id=4192&action=view)
merge-in the r595503 from trunk: fix anchoring, rename "verified" to "valid"
internally
The resulting code is (again) the same as the current code
in trunk, except for the commented-out calls to scoped timing,
which does not exist in 3.2. I hope this last rather cosmetic
change does not invalidate jm's vote, so this still leaves one
to go, barring objections.
I noticed that later versions of Mail::DKIM do check for
expiration time internally so the expiration check in the
plugin is redundant. It does not hurt though, so I'd leave
it as it is for the time being.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
maddoc@maddoc.net changed:
What |Removed |Added
----------------------------------------------------------------------------
Status Whiteboard|needs 1 vote for 3.2 and a |can be committed for 3.2
|-1 from Daryl resolved |and a -1 from Daryl resolved
------- Additional Comments From maddoc@maddoc.net 2007-12-22 16:30 -------
+1
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From jm@jmason.org 2007-12-17 12:58 -------
'There's no punishment of senders by not causing an informational rule that
scores ~0 to fire on damn near every email received. The only punishment is to
the receivers who it annoys the hell out of. I'm not saying that internally we
shouldn't observe a default policy when its appropriate to do so, just that we
don't need to add a rule in every email received (that the sender doesn't define
a different policy or doesn't even use DKIM) telling the recipient what the
defaults of some RFC is.'
agreed. it'd be nice to get rid of that noise...
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From jm@jmason.org 2007-12-28 04:37 -------
(In reply to comment #25)
> > One thing: if I enable t/dkim.t, and set "run_net_tests=y", all tests fail.
> > does it need to be updated to match?
>
> dkim.org dropped their "testing1234" selector, so the tests (which all depend on
> it) stopped working a while back. At some point we should probably set up our
own.
yes. that's very annoying :(
I've added an explanatory comment to both 3.2.x and trunk's version of dkim.t
about that.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
Mark.Martinec@ijs.si changed:
What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|Undefined |3.2.4
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
spamassassin@dostech.ca changed:
What |Removed |Added
----------------------------------------------------------------------------
Status Whiteboard|needs 1 vote for 3.2 |go
------- Additional Comments From spamassassin@dostech.ca 2007-12-26 13:27 -------
> One thing: if I enable t/dkim.t, and set "run_net_tests=y", all tests fail.
> does it need to be updated to match?
dkim.org dropped their "testing1234" selector, so the tests (which all depend on
it) stopped working a while back. At some point we should probably set up our own.
> Doesn't Mark, Daryl, and Juatin make 3 votes total?
Yes, yes it does.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
jm@jmason.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Status Whiteboard|needs 2 votes (+1'd from |needs 1 vote for 3.2
|Daryl) |
------- Additional Comments From jm@jmason.org 2007-12-26 03:32 -------
(In reply to comment #22)
> Excellent. You've now made it completely clear as to why things have been
> changed. The opaque references to SSP in the initial comments really threw me
> off. In the absence of *any* real design documentation I feel it's important
> for us to clearly document design changes via bugzilla, especially when the
> changes affect code that interacts with evolving external projects/specifications.
agreed, and it works well for that!
> Good work Mark. Thanks for playing this out.
>
> +1 on 4214
+1
One thing: if I enable t/dkim.t, and set "run_net_tests=y", all tests fail.
does it need to be updated to match?
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From jm@jmason.org 2007-12-17 04:15 -------
(In reply to comment #10)
> - it's now impossible to use DKIM for whitelisting only by setting the scores
> for the DKIM rules to 0;
Daryl, can you explain this? Are you saying it's possible to run DKIM checks
even if the rules are set to 0 scores? because I'd prefer that wasn't the case;
0-scoring is the "standard" way to disable a set of rules, after all.
> this forces all DKIM signed messages to be
> processed... something that isn't exactly light-weight;
"processed" in what way? do we not already perform some lookups?
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From spamassassin@dostech.ca 2007-09-28 13:13 -------
-1 on something like r580453 for 3.2.4.
The change unnecessarily removes functionality, removes useful debugging info,
changes the behaviour of existing configs, has overly verbose (and thus
confusing) documentation, uses a number or real domains in examples (and those
domains don't even support DKIM), references SSP which is still in limbo (I
intentionally omitted it with the intention to reference it once everything was
settled with it) and possibly hides something I've missed in the massive change
of $scanner to $scan all over the place.
My biggest issue is with only allowing for an exact match of the provided
optional signing identity. At the very least you need to allow for sub domain
globing. Additionally, when an optional signing identity is provided there
shouldn't be any fallback to using the originator identity.
BTW, there was no misinterpretation (at least on my part) of what an identity
is, merely simplification. Anyone familiar with DKIM would be fine with (ie.
understand) the docs how they were, and anyone not very familiar with DKIM need
not be confused by an overly detailed explanation. You must remember that a
large portion of our user base is comprised of shrink wrap admins who are often
clueless of all things network related, such as DKIM. I'm sure you'll find that
the average SA user is far less technically adept than the average amavisd-new user.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-12-17 08:21 -------
> All past versions only checked for a policy testing
> flag, never for a public key testing flag.
The underlying reason for this absence of information on a public
key flags is that Mail::DKIM doesn't provide an official API to
access the public key and its attributes from a signature object.
One could call a $sig->get_public_key->testing for each signature
object, but a drawback is that if the first attempt to fetch a key
failed, the call to get_public_key will try to fetch it again (and
likely to fail again). Alternatively, access to Mail::DKIM internal
data structures could provide this information, but it would be a
hack. Seems we'd need to ask Jason for a new and official API to
access p.key attributes. In any case, it won't be available before
Mail::DKIM 0.30.
> This is why it now became more obvious that the dkim_policy_testing
> is always 0.
Typo, the dkim_key_testing is 0.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From Mark.Martinec@ijs.si 2007-10-05 14:28 -------
In view of the coming next version of Mail::DKIM (expected to provide
official API access to a list of all signatures of a message, such list
is already kept internally), I invested a day and modified DKIM plugin to
finally support whitelisting on all valid signatures found in a message
(e.g. DKIM and DomainKeys signed, or original and mailing-list signatures)
No changes to user interface or to functionality in single-signature
or for no-signature mail, 20 lines lengthier code, of which 10 lines
are an added POD section.
First code drafts were rather ugly, lengthy and inefficient, until I
decided to turn loops traversing whitelists inside-out, the outer loop
now walks across a list of signatures found, and the inner loop walks
across entries from all four relevant whitelists. This is to avoid
repeating the same code sections for every wl entry, avoids calling
a subroutine for each wl entry, and to avoid duplicating code - with
a goal of keeping debug logging similar to its behavior so far,
hopefully even more informative at first glance.
I also added a tag DKIMIDENTITY, which provides access to a signing
identity of a valid signature. It is a cheap addition, and I'll be
needing it some place else - but this is another story.
One problem I have with this tag is that in principle it should be
a LIST of signatures, not necessarily a single entry, and that in
principle any character (except few like CR and NUL) could show up in
an identity, so a space-separated or a comma-separated single string is
not a clean solution. Perhaps tags could be allowed to contain lists
in 3.3.0, some other tags might benefit too (e.g. LANGUAGES, etc).
When accessed as usual scalars list entries could be 'joined' with
spaces or whatever separator.
Hint: instead of looking at diff of both last revisions, it would
probably be gentler to eyes to look at the final code, or just at
a debugging output with a debug area 'dkim'. Take it or leave it
( or shoot :) .
$ svn -m 'Support multiple signatures in DKIM plugin, add tag DKIMIDENTITY' ci
Sending lib/Mail/SpamAssassin/Plugin/DKIM.pm
Committed revision 582397.
There may be a need for some small adjustment when Mail::DKIM 0.29
finally comes out in few days. I'll keep code compatible with 0.28
(and earlier).
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From jm@jmason.org 2007-12-28 09:39 -------
> > dkim.org dropped their "testing1234" selector, so the tests (which all
> > depend on it) stopped working a while back. At some point we should
> > probably set up our own.
>
> Is there any simple way of adding one or two or three TXT records
> under domain spamassassin.org (or maybe apache.org)?
yep, it's relatively simple, just check in the changes under
https://svn.apache.org/repos/asf/spamassassin/dns and I'll make them
active.
I was under the impression that we'd have to regenerate those test messages
entirely, but if that can be avoided and we just "re-home" the selectors,
great!
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 5662] [review] DKIM plugin overhaul - whitelisting and terminology
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5662
------- Additional Comments From sidney@sidney.com 2007-12-26 06:13 -------
Doesn't Mark, Daryl, and Juatin make 3 votes total?
I get failure in t/dkim.t when it is enabled both before and after the patch, so
I'm confused about how to test this with a quick check. I haven't looked at this
in depth so far.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.