Milter-greylist

Hi,

	I think that I have found a problem in the MX synchronisation code.
It seems to me that a tuple that is autowhitelisted for more time is not sent
to the peers.  As I understand the code, milter-greylist tells the peers
about tuples that are removed from the pending list and added to the
autowhite list, i.e. mails that have been delayed and that are autowhitelisted
for the first time.

	IMO, this means that milter-greylist does not tell the peers about
tuples that are already present in the autowhite list and that are
autowhitelisted for more time when a new mail with same IP, from and to is
received.  I think that this can introduce an unneeded delay if the primary
MX is unreachable: since peers do not know about the additional
autowhitelisting, the entry can expire on secondary MX while it still exists
on the primary MX.

	I do not have a patch for this problem (the version of milter-greylist
that I use is quite different from the standard one and a patch against it
would be unusable by anybody but me) but I think that the problem could be
solved by adding a call to peer_delete() in autowhite_add() when an entry
is found in the autowhite list and its life time is prolongated, i.e. when
an entry is autowhitelisted for more time.  The code could be similar to
the one in pending_check() when the greylist delay is over, I think.

	Comments, anyone?

	Cheers

		R\ufffdmy

Remy Card <Remy.Card@...> wrote:

>       I think that I have found a problem in the MX synchronisation code.
> It seems to me that a tuple that is autowhitelisted for more time is not sent
> to the peers.  As I understand the code, milter-greylist tells the peers
> about tuples that are removed from the pending list and added to the
> autowhite list, i.e. mails that have been delayed and that are autowhitelisted
> for the first time.

Yes, this has been a known minor bug for a while. 
 
>       I do not have a patch for this problem (the version of milter-greylist
> that I use is quite different from the standard one and a patch against it
> would be unusable by anybody but me) but I think that the problem could be
> solved by adding a call to peer_delete() in autowhite_add() when an entry
> is found in the autowhite list and its life time is prolongated, i.e. when
> an entry is autowhitelisted for more time.  The code could be similar to
> the one in pending_check() when the greylist delay is over, I think.
> 
>       Comments, anyone?

Sounds good. Someone test that fix?

-- 
Emmanuel Dreyfus
Il y a 10 sortes de personnes dans le monde: ceux qui comprennent 
le binaire et ceux qui ne le comprennent pas.
manu@...

On Mon, Dec 06, 2004 at 02:22:19AM +0100, manu@... wrote:
> Remy Card <Remy.Card@...> wrote:
> > As I understand the code, milter-greylist tells the peers
> > about tuples that are removed from the pending list and added to the
> > autowhite list, i.e. mails that have been delayed and that are autowhitelisted
> > for the first time.
> 
> Yes, this has been a known minor bug for a while. 

	Ok.  I did not know about this since I am quite new to this list.

> >       I do not have a patch for this problem (the version of milter-greylist
> > that I use is quite different from the standard one and a patch against it
> > would be unusable by anybody but me) but I think that the problem could be
> > solved by adding a call to peer_delete() in autowhite_add() when an entry
> > is found in the autowhite list and its life time is prolongated, i.e. when
> > an entry is autowhitelisted for more time.  The code could be similar to
> > the one in pending_check() when the greylist delay is over, I think.
> 
> Sounds good. Someone test that fix?

	Note that this could be a bit more complex than what I wrote.  After
re-reading the code and thinking of it again, I believe that a fictive
pending record should be allocated and initialized to be used as a
parameter to peer_delete().  The fictive record should not be added to
the pending list.

		R\ufffdmy

On Mon, Dec 06, 2004 at 07:27:55AM +0100, Remy Card wrote:
> > Sounds good. Someone test that fix?
> 
> 	Note that this could be a bit more complex than what I wrote.  After
> re-reading the code and thinking of it again, I believe that a fictive
> pending record should be allocated and initialized to be used as a
> parameter to peer_delete().  The fictive record should not be added to
> the pending list.

	Ok, I wrote the fix.  I cannot send the patch since I added the fix
in my ACL development version, but it is quite easy to add in autowhite.c:

- add:
#include "sync.h"
- in autowhite_check, add a local variable:
        struct pending *pending;
- in autowhite_check, add:
                        /*
                         * We need to tell our peers about this, we use a
                         * fictive pending record
                         */
                        pending = pending_get(sa, salen, from, rcpt,
                            (time_t)0);
                        if (pending != NULL) {
                                peer_delete(pending);
                                pending_put(pending);
                        }
  after:
                        syslog(LOG_INFO, "%s: addr %s from %s rcpt %s: "
                                "autowhitelisted for more %02d:%02d:%02d",
                                queueid, addr, from, rcpt, h, mn, s);

	I plan to release an ACL patch RSN.  The two MX sync fixes will be
included in this patch.

		R\ufffdmy