$ svn -m 'PR-5891: let AWL keep separate records
          for DKIM-signed and unsigned mail' ci
Sending        lib/Mail/SpamAssassin/AutoWhitelist.pm
Sending        lib/Mail/SpamAssassin/DBBasedAddrList.pm
Sending        lib/Mail/SpamAssassin/PersistentAddrList.pm
Sending        lib/Mail/SpamAssassin/Plugin/AWL.pm
Sending        lib/Mail/SpamAssassin/SQLBasedAddrList.pm
Sending        sql/README.awl
Sending        sql/awl_mysql.sql
Sending        sql/awl_pg.sql
Transmitting file data ........
Committed revision 650852.

> As an alternative to adding a new field, it would be just as fine to
> re-purpose a field awl.ip to carry either an IP addrress or a signer id.
> As this field if very short (10 characters), tables would need to be
> modified one way or another, so I chose a somewhat cleaner approach.

Btw, with a r720963 (on 2008-11-26) I have lengthened the awl.ip field
to 45 characters, which was necessary to support IPv6 addresses.
This opened a previously dismissed possibility of using the same awl.ip
field for a signing domain when a message carries valid signature(s).
It's probably still cleaner to use a separate field as the current
code does - I just thought I should mention it.

As a message can carry more than one valid DKIM signature, keeping
multiple signing domains in one SQL field is still somewhat unclean
from a point of view of a database design - storing signers in a
separate table would be nicer. From a practical viewpoint, the
current simpler solution suffices for the time being.

Btw, is anybody (except me) running AWL on SQL with updated schema
and  'auto_whitelist_distinguish_signed 1'  ?

Closing, if there are no objections.
The feature if off by default.

Reopening, to solicit some further comments, and document recent work.

The following changes mostly just polished my previous work on the topic,
no changes in functionality (apart from a bug fix).

r817328 | mmartinec | 2009-09-21 19:24:02 +0200 (Mon, 21 Sep 2009)
AWL/AutoWhitelist/SQLBasedAddrList cleanups: remove leftovers of
signer reputations; prevent a new nonsigned query from updating
a signed SQL entry; average across all signed entries in case of
multiple valid signatures (e.g. author domain + signing remailer);
cleanup (long lines, minor details); documentation consistency

r816683 | mmartinec | 2009-09-18 17:51:14 +0200 (Fri, 18 Sep 2009)
When storing an IPv6 address to AWL database, append
a '::' to a /48 network address -  although it wastes
2 chars, it's nice to make it look like a syntactically
correct IPv6 address; just fits into SQL 16 char field.

> Reopening, to solicit some further comments, and document recent work.

Now to my question.

Considering that an awl.ip field needs to be extended from 10 characters
(as in SA 3.2.3) to 16 characters or more to accommodate an /48 IPv6
address (RIPE is allocating /48 blocks to organizations) - perhaps
this opportunity could be seized and to also require adding a field
awl.signedby, as needed for the topic of this PR.

Note that SpamAssassin/MTA can see an IPv6 address in Received header
fields even if running on a host with no IPv6 connectivity - e.g. in
a header field inserted by a 'trusted' MTA.

Doing that could eliminate a need for the auto_whitelist_distinguish_signed
option, which is only there for the purpose of compatibility with an
unchanged awl SQL database. The less options, the fewer opportunity
for support questions, misconfigurations, undertested code ...

Also some admins would choose to keep auto_whitelist_distinguish_signed
at its default (off) compatibility setting solely out of laziness and for
sticking blindly to defaults, which has unfavourable affect on AWL results.

So, I propose that with upgrading to SA 3.3.0 and keeping existing AWL
database on SQL, one is obliged to do both changes (extend awl.ip and
add awl.signedby), not just the first. And to get rid of my compatibility
option auto_whitelist_distinguish_signed.

The required change is described in sql/README.awl :

MySQL:

  ALTER TABLE awl MODIFY ip varchar(16);

  ALTER TABLE awl
    DROP PRIMARY KEY,
    ADD signedby varchar(255) NOT NULL DEFAULT '',
    ADD PRIMARY KEY (username,email,signedby,ip);

PostgreSQL:

  ALTER TABLE awl ALTER ip TYPE varchar(16);

  DROP INDEX awl_pkey;
  ALTER TABLE awl
    ADD signedby varchar(255) NOT NULL DEFAULT '',
    ADD PRIMARY KEY (username,email,signedby,ip);

Mark, you positive adding a awl.signedby column is going to change AWL's life?

Isn't that most of the dkim and spf-signed mail from freemail providers is after all caming from a well-defined bunch of net bunches? This would end in a co-relation between the signedby field and the ip one. Signedby would then add almost nothing to the equation.

Also, please note that a awl.signedby would make the AWL test a network-based one, which actually is not.

I do understand AWL needs to adjust for ipv6 addresses. I would also agree it could be "squeezed" a bit further. But my very personal thinking about this, that is the one of (mostly) a SA user, is that anything is going to break existing setups must be worth the troubles.

So, the ipv6 problem do or will exist. Since patching it would require some changes to the AWL table structure, I am quite in favor of adding anything can squeeze more from AWL. But pardon me, I think a signedby column may at most make a difference in border cases.

In another bug you opened about AWL I said that detecting the assigned bunch address to which a source belongs was unfeasible. Well, that's true, but I forgot ASN routes. We can instead use ASN routes to discriminate the source. SA already uses the ASN plugin to discover the ASN number related to a source. The ASN plugin issues a DNS query to ans.routeviews.org in order to get it, but the reply from routeviews.org also contains route netaddress and netmask length... So, AWL should store a cidr instead of a fixed-length address. This would at least mean adding a column to the AWL table and change somehow the content (and possibly the name) of the ip one in order to accomodate a variable-length, IPvX network address. This would add a bit of complexity to the AWL code, but I believe it is worth the effort. I don't have numbers for this and I really would like to. What do you think about it?

(In reply to comment #6)
> Mark, you positive adding a awl.signedby column is going to change AWL's life?
> 
> Isn't that most of the dkim and spf-signed mail from freemail providers is
> after all coming from a well-defined bunch of net bunches? This would end in a
> co-relation between the signedby field and the ip one. Signedby would then add
> almost nothing to the equation.

Let's keep the awl.signedby optional, as it is now for some time (enabled
by setting the: auto_whitelist_distinguish_signed 1).

I agree there is a strong correlation between an IP address and a signing
domain for direct mail. But it also does help validate some re-routed mail
(forwarding, and non-clobbering mailing lists like the users@spamassassin
or postfix-users). The collected data is also useful for assessing domain
reputations (the code to dynamically do that was in the awl for a while,
but I removed it, as it suffices to collect reputations offline from the
awl database).

> Also, please note that a awl.signedby would make the AWL test a network-based
> one, which actually is not.

Right.

> I do understand AWL needs to adjust for ipv6 addresses. I would also agree it
> could be "squeezed" a bit further. But my very personal thinking about this,
> that is the one of (mostly) a SA user, is that anything is going to break
> existing setups must be worth the troubles.
> 
> So, the ipv6 problem do or will exist. Since patching it would require some
> changes to the AWL table structure, I am quite in favor of adding anything can
> squeeze more from AWL. But pardon me, I think a signedby column may at most
> make a difference in border cases.

The IPv6 issues and the configurable net mask length is now tracked by Bug 6203.

> In another bug you opened about AWL I said that detecting the assigned bunch
> address to which a source belongs was unfeasible. Well, that's true, but I
> forgot ASN routes. We can instead use ASN routes to discriminate the source. SA
> already uses the ASN plugin to discover the ASN number related to a source. The
> ASN plugin issues a DNS query to ans.routeviews.org in order to get it, but the
> reply from routeviews.org also contains route netaddress and netmask length...
> So, AWL should store a cidr instead of a fixed-length address. This would at
> least mean adding a column to the AWL table and change somehow the content (and
> possibly the name) of the ip one in order to accomodate a variable-length, IPvX
> network address. This would add a bit of complexity to the AWL code, but I
> believe it is worth the effort. I don't have numbers for this and I really
> would like to. What do you think about it?

It is a good idea worth remembering, indeed the ASN data is already available
(if the plugin is enabled), it would just need to be passed to awl and managed
somehow. Let's leave this for some future version, where awl could be enhanced
into a more general purpose reputation-assessing tool.

I'd just close this ticket for now (3.3.0), but may be reopened later for
some new thoughts on future versions.