per-recipient handling

Hi,

I addressed this list some time ago when the spam filtering product that
I work on started making the switch to Qpsmtpd.  BTW, we've had an 
excellent experience with the product and there's no question that we're
going to be sticking with it permanently.  When we first made the 
switched, I was disappointed to find that QP doesn't currently support
per-recipient configuration directives or different end results for
different recipients that have been accepted at RCPT time.  It was 
mentioned that this would be a worthwhile need to support and design and
coding work on it would be welcome.  I'm aware of one solution that 
involves temp-failing at DATA time and then temp-failing recipients
selectively, but this wasn't an acceptable solution for us.  It became 
clear for this and for other reasons that QP's stock plugin code
wouldn't cut it and that I would need to basically re-write a lot of
QP's logic, which I did.  What I came up with was workable, though not 
perfect.  Here are the fundamentals:

* I invented my own Qpsmtpd::Recip object that subclasses
Qpsmtpd::Address and supports a ->config() method similar to
$self->qp->config
* In a given post-DATA plugin (e.g. spamassassin), I loop through
$transaction->recipients, and for each one get SA prefs, thresholds,
etc. and scan for each one separately (unless sets of recipients happen
to have identical configurations), and store the results in each Recip
object.
* If one recipient wants to reject a message, but others want to accept
it, I "downgrade" the first recipient's rejection.  In my 
implementation, I have made this "downgrade" action configurable, but
the default is to drop the message quietly.  No user has ever had a 
desire to change this default to my knowledge, and I'm considering
leaving the configurability out of the interface to simplify things.

At the time that I made all these changes, I hoped that I would be able
to work with the Qpsmtpd project to integrate some useful changes
upstream; however, I couldn't get clearance from higher up to contribute
code.

Now, we have begun to realize that we need to switch from forkserver to
async.  I have also noticed a number of flaws in my original 
implementation of per-recip config directives and delivery results which
need to be corrected, to the point that our switch to async will
probably be done in tandem with a switch to trunk and a complete
re-write of our plugin work.  The fact that we are now significantly 
'forked' from the majority of QP's upstream code makes this a more
difficult proposition.  Armed with this information, I again requested 
permission to contribute code so that this may be the last time I have
to deal with such inconveniences -- and it was easily granted.

So, I now have the freedom to contribute significant changes to you guys
with the basic goal of making interoperability with 'standard' QP easier
so that we don't have to deal as much (maybe someday not at all) with
forked code.  With permission, my biggest goal is to get per-recipient 
config and delivery result handling into QP.  It has been previously 
pointed out that this is not such a simple task because of the varying
needs of QP users and plugin authors -- especially the fact that plenty
of people have no desire to do different things per-recipient and a
fundamental change toward this would only make this more complex for
them.  Based on my experiences so far with the problem, and my own 
design goals, I have some ideas in mind to overcome this.

Step 1:  Add per-recipient configuration

This in itself would probably be pretty simple to add support for and
would give plugin authors some immediate power.  Basically, one should 
be able to call the 'config' method on a Qpsmtpd::Address object.  At 
that point would become a bit of a misnomer; Qpsmtpd::Recipient might be
more appropriate.  This method should be able to get its values 
similarly to qp->config:  from the file system or from a plugin.  I 
would suggest that on the filesystem end, the administrator should be
able to place directories inside /etc/qpsmtpd/ named for users on the
system, whose contents work the same way flat files in /etc/qpsmtpd/
work.  E.G. /etc/qpsmtpd/jaredj/timeout contains my customized timeout, 
just like /etc/qpsmtpd/timeout contains the global setting.

Plugin authors should also be able to create config plugins on the
recipient level.  In our case, we would use this sort of plugin to do a 
database lookup and retrieve the necessary directive.

If no plugin or file dictates a per-recipient directive, then the global
plugin logic is followed, so that a global config plugin or a file in
/etc/qpsmtpd/ can provide the defaults for users that weren't interested
in customizing a particular option.

My current spamassassin plugin examines user-specific SA prefs in the
database (e.g. bayes enabled/disabled, whitelists) and if we have
already scanned for a recipient with identical prefs, we don't bother
scanning again; if we are encountering a user with unique preferences
that we haven't seen before, we scan all over again for a new score.  In 
order to make something like this possible, we would probably also need
a 'notes' method for recipient objects as well.  This also seems fairly 
surmountable.

Step 2:  Add per-recipient results

A Qspmtpd::Address[|Recipient] object needs to have a method returning
its result, probably a Qpsmtpd::DSN object.  There should also be one or 
more transaction methods available for detailing what results have been
determined for recipients overall: one that I can think of would be a
method to determine whether there are any recipients left for which a
method is not going to be rejected.  With this support along these lines 
together with the previous step, a post-data plugin could do something like:

for my $rcpt ($transaction->recipients) {
     next unless $rcpt->config('spamassassin_enabled');
     my $score = scan();
     next unless $score > $rcpt->config('spamassassin_threshold');
     $rcpt->dsn(Qpsmtpd::DSN->media_unsupported('this is spam');
}
return DECLINED if $transaction->clean_recipients();
return Qpsmtpd::DSN->media_unsupported('this is spam');

Step 3:  Make per-recipient handling transparent to plugins...?

I'm not completely sure the best way to go about this.  But a problem 
needs to be solved:  once we give plugin writers granularity to do 
per-recipient handling, if we have just as many users needing per-recip
plugins and per-transaction plugins, we don't want to have two copies of
every plugin, one for per-transaction and one per-recipient, and to have
to maintain and apply bugfixes to both copies.

One way to deal with this might be to allow administrators to specify
that, even if a plugin is calling ->config() and ->dsn() on unique
recipients, the entire transaction should also be acted on globally.  An 
alternate to $transaction->recipient() could exist, a single
Qsptmd::Address object that has the special meaning of 'global'.
Perhaps the option of whether or not to bother with the whole list or
the single GLOBAL recip could be specified on a per-plugin basis on the
plugin 'command line' in /etc/qpsmtpd/plugins, so that we do:

for my $rcpt ( $global
                ? Qpsmtpd::Address->new('GLOBAL')
                : $transaction->recipients ) {
     $rcpt->things;
     ....
}

It still seems like it might be a bit annoying from the plugin's
perspective to be looping through a list of recipients that may or may
not consist of only one special entry.  It might also be feasible to 
actually process plugins differently -- if the admin has specified that
he wants per-recip processing, run a plugin once for every recipient
rather than once for every transaction, and give the plugin a modified
idea of what the transaction really looks like (e.g.
$transaction->recipients() only returns the one recipient currently in
question).  I haven't really been able to come up with a way to do this 
responsibly, though, without introducing some real efficiency problems, etc.



This represents my thinking on the subject so far.  This is sort of a 
big topic so I imagine this may be the beginning of a long discussion.
In the end, with a bit of confirmation that there is a design path that
QP developers are comfortable with, I will be happy to begin working and
submitting patches to make per-recip handling a reality.

Thanks having a great product for me to hack on :)

Jared Johnson
Software Developer and Support Engineer
Network Management Group, Inc.
620-664-6000 x118

Hi Jared,

On Mon, 10 Nov 2008 10:42:15 -0600
Jared Johnson <jaredj@nmgi.com> wrote:
> * I invented my own Qpsmtpd::Recip object that subclasses
> Qpsmtpd::Address and supports a ->config() method similar to
> $self->qp->config
[...]
> Step 1:  Add per-recipient configuration
>
> This in itself would probably be pretty simple to add support for and
> would give plugin authors some immediate power. Basically, one should
> be able to call the 'config' method on a Qpsmtpd::Address object. At
> that point would become a bit of a misnomer; Qpsmtpd::Recipient might be
> more appropriate. This method should be able to get its values
> similarly to qp->config: from the file system or from a plugin. I
This looks like it can be done in a plugin and by adding a check for
the second parameter of Qpsmtpd::config():
- in hook_rcpt replace the given Qpsmtpd::Address object by a
   Qpsmtpd::Recipient object (similar to yours?). For replacing: see
http://www.nntp.perl.org/group/perl.qpsmtpd/2008/10/msg8257.html
   You can also do 
     $_[0] = Qpsmtpd::Recipient->new($_[0]->user,$_[0]->host)
   without affecting anything... well, maybe except for setups where
   plugins replace the current Q::A object by another Q::A object
   instead of just setting ->user() and ->host(). 
      
 - Qpsmtpd::config(): if ($type and ref($type) != ""):  skip the
   $_config_cache lookup and pass the $type (in our case a
   Qpsmtpd::Recipient) to hook_config. 

This would mean that the core config is served from qmail files (or
hook_config) and all per recipient configs have to come from some
plugin hooking "config".

I, personally, would like to see some of the more common config
settings wrapped in config calls in Qpsmtpd::Recipient, like:
$rcpt->queue (which queue plugin to use), ->spamassassin_enabled,
->sa_deny_score, ->drop (if true, drop message silently), ->redirect
(sent to this address if not undef())... i.e. anything which defines a
recipient and is probably useful for most users.

I'm currently working on a Qpsmtpd::Config module supporting some
backend config files transparently. I can add support for a second
parameter to support per recipient user/per domain configs and for a
whatever structured qmail file directory hierarchy [to Ask: also plugin
only ;-)]... as long as the given parameter supports ->user and ->host I
don't care if I get a Qpsmtpd::Address or a Qpsmtpd::Recipient.
[ For configs stored in an SQL database the helper module probably needs
  some/much more work. ]

> Step 2:  Add per-recipient results
>
> A Qspmtpd::Address[|Recipient] object needs to have a method returning
> its result, probably a Qpsmtpd::DSN object.
...or some other flag based list what to do with the message (drop
silently, deny, accept, redirect, ...)?
[...]
> for my $rcpt ($transaction->recipients) {
> next unless $rcpt->config('spamassassin_enabled');
>      my $score = scan();
Mhh, maybe ask the SA people if it's possible to scan one mail and then
switch user and just re-calculate the score for this user. This is
unlikely to work with spamd, but with a qpsmtpd plugin "use()ing"
Mail::SpamAssassin it may work.

Hanno

Take a look at
http://svn.perl.org/viewvc/qpsmtpd/contrib/hjp/address_notes/

It adds a notes() method to the Qpsmtpd::Address class which works
just the same the transaction and connection notes.


> - in hook_rcpt replace the given Qpsmtpd::Address object by a
> Qpsmtpd::Recipient object (similar to yours?). For replacing: see
> http://www.nntp.perl.org/group/perl.qpsmtpd/2008/10/msg8257.html
>    You can also do 
> $_[0] = Qpsmtpd::Recipient->new($_[0]->user,$_[0]->host)
> without affecting anything... well, maybe except for setups where
> plugins replace the current Q::A object by another Q::A object
> instead of just setting ->user() and ->host().

Or where the Q::A object contains more state than just user and host.

> This would mean that the core config is served from qmail files (or
> hook_config) and all per recipient configs have to come from some
> plugin hooking "config".
>
> I, personally, would like to see some of the more common config
> settings wrapped in config calls in Qpsmtpd::Recipient, like:
> $rcpt->queue (which queue plugin to use), ->spamassassin_enabled,
> ->sa_deny_score, ->drop (if true, drop message silently), ->redirect
> (sent to this address if not undef())... i.e. anything which defines a
> recipient and is probably useful for most users.

I do almost all of these with address notes. (Some older plugins still
use transaction notes with the recipient address as part of the key, I
should really update them ...).

And I guess I should have moved address_notes from contrib to trunk long
ago. It's really useful, fits into the model of qpsmtpd, and nobody
looks in contrib before reinventing the wheel ;-(.

hp

Why support both?  Just svn mv lib/Qpsmtpd/Address.pm 
lib/Qpsmtpd/Recipient.pm and add a few new methods; then make
Qpsmtpd::Address::new just return a Qpsmtpd::Recipient object for
compatibility.  There wouldn't be any real overhead for anyone using the 
Qpsmtpd::Recipient object for addresses only.  That would also deal with 
a plugin completely replacing one object with another.

BTW, I've talked previously about subclassing the real Mail::Address,
but looking further into Mail::Address vs. Qpsmtpd::Address, that
doesn't look sensible at all.  I thought that Qpsmtpd::Address was 
basically Mail::Address copied and pasted with a few small changes, but
they really are pretty different.

> I, personally, would like to see some of the more common config
> settings wrapped in config calls in Qpsmtpd::Recipient, like:
> $rcpt->queue (which queue plugin to use), ->spamassassin_enabled,
> ->sa_deny_score, ->drop (if true, drop message silently), ->redirect
> (sent to this address if not undef())... i.e. anything which defines a
> recipient and is probably useful for most users.

I would personally like to see s/connection/cxn/ and s/transaction/txn/
everywhere in QP ;)

> I'm currently working on a Qpsmtpd::Config module supporting some
> backend config files transparently. I can add support for a second
> parameter to support per recipient user/per domain configs and for a
> whatever structured qmail file directory hierarchy [to Ask: also plugin
> only ;-)]... as long as the given parameter supports ->user and ->host I
> don't care if I get a Qpsmtpd::Address or a Qpsmtpd::Recipient.
> [ For configs stored in an SQL database the helper module probably needs
>   some/much more work. ]

Any ETA on this?  I'd be happy to do this work (and will need to on my 
way to our own async migration whether or not it's useful to the QP
project, unless you're expecting to have it completed fairly soon)

>> A Qspmtpd::Address[|Recipient] object needs to have a method returning
>> its result, probably a Qpsmtpd::DSN object.
> ...or some other flag based list what to do with the message (drop
> silently, deny, accept, redirect, ...)?

I personally like a DSN object, because I think Qpsmtpd::DSN should have
an expanded roll in QP.  In our forked code we have made our own DSN 
objects based on the easy Qpsmtpd::DSN API with some success.  Since we 
store this DSN object, with accessors telling us what we did with the
message for each recipient and why, in our recipient object, logging
everything to the database in our own particular archive staging schema
is really easy.  I haven't really given much thought to how much of that 
would be useful upstream.  That's for another thread I guess :)

> Mhh, maybe ask the SA people if it's possible to scan one mail and then
> switch user and just re-calculate the score for this user. This is
> unlikely to work with spamd, but with a qpsmtpd plugin "use()ing"
> Mail::SpamAssassin it may work.

Has anyone been able to measure the difference in performance use()ing M::S?

-Jared