Milter-greylist

I am testing the mxsync function and it is stable, but I have some 
discrepancies in the databases.

If a host that is currently in the database, either greylisted or 
auto-whitelisted, and is deemed ok for auto-whitelisting AND the host is 
NOT the same as before BUT in the same D-class network (I use 
subnetmatch /24) the following happens:

server_1 updates its database correctly, but does not update the IP, 
since it matches the subnetmatch. server_2, which we are syncing to, 
writes a new entry with the current IP and does NOT remove the old entry 
which matches the subnetmatch. Please note that server_2 does no actual 
work, it only recieves mxsync-data.

An example:

maillog:
Sep 27 11:17:30 milter-greylist: j8R8HUIk007657: addr 85.37.17.69 from 
<community@...> rcpt <XXXXX@...>: autowhitelisted 
for more 864:00:00
Sep 27 11:17:30 milter-greylist: (mxsync): addr 85.37.17.69 from 
<community@...> rcpt <XXXXX@...>: autowhitelisted 
for 864:00:00

server_1:
dump.server_1:85.37.17.70        <community@...> 
<XXXXX@...>       1130919450 AUTO # 2005-11-02 10:17:30

server_2:
dump.server_2:85.37.17.69  <community@...> 
<XXXXX@...>       1130919450 AUTO # 2005-11-02 10:17:30
dump.server_2:85.37.17.70  <community@...> 
<XXXXX@...>       1129999192 AUTO # 2005-10-22 19:39:52


Could it be that mxsyncing does not check for subnetmatch? I will try to 
look at the code myself, but any help would be appreciated.

Another thing: it seems to me that the database is not flushed to disk 
on exit. atexit is used, but it's manpage says it needs exit() or a 
return from main() to be run. The current init.d script for redhat uses 
killproc to kill the process, should perhaps a signalhandler be used 
instead of atexit?

Thanks again,
Fredrik Nyberg

> 
> I am testing the mxsync function and it is stable, but I have some 
> discrepancies in the databases.
> 
> If a host that is currently in the database, either greylisted or 
> auto-whitelisted, and is deemed ok for auto-whitelisting AND 
> the host is 
> NOT the same as before BUT in the same D-class network (I use 
> subnetmatch /24) the following happens:
> 
> server_1 updates its database correctly, but does not update the IP, 
> since it matches the subnetmatch. server_2, which we are syncing to, 
> writes a new entry with the current IP and does NOT remove 
> the old entry 
> which matches the subnetmatch. Please note that server_2 does 
> no actual 
> work, it only recieves mxsync-data.
> 

Hello,

The patch I submitted near a week ago is addressing exactly that problem.
On our site I am using it in production since than and the redundancy of the autowhitelisted entries from the same class D has been reduced from 76% to 0%.

Bests,
Attila

Fredrik Nyberg DC wrote:
> Another thing: it seems to me that the database is not flushed to disk 
> on exit. atexit is used, but it's manpage says it needs exit() or a 
> return from main() to be run. The current init.d script for redhat uses 
> killproc to kill the process, should perhaps a signalhandler be used 
> instead of atexit?

Here is a very small patch so the database is dumped when the redhat 
init script is used for stopping milter-greylist.

Cheers,
Fredrik Nyberg

On Thu, Oct 20, 2005 at 10:44:23AM +0300, Fredrik Nyberg DC wrote:
> Here is a very small patch so the database is dumped when the redhat 
> init script is used for stopping milter-greylist.

We can't do that: milter documentation says that libmilter handles the
signals. Catching SIGTERM could cause problems with future libmilter 
implementations.

-- 
Emmanuel Dreyfus
manu@...

Emmanuel Dreyfus wrote:
> On Thu, Oct 20, 2005 at 10:44:23AM +0300, Fredrik Nyberg DC wrote:
> 
>>Here is a very small patch so the database is dumped when the redhat 
>>init script is used for stopping milter-greylist.
> 
> 
> We can't do that: milter documentation says that libmilter handles the
> signals. Catching SIGTERM could cause problems with future libmilter 
> implementations.

I see. That's a problem, because lot's of data is lost on exit. USR1 or 
USR2 could be used, I guess. I wonder why libmilter is not returning to 
main after it catches SIGTERM. atexit is not triggered correctly.

Fredrik Nyberg

On Thu, Oct 20, 2005 at 11:45:03AM +0300, Fredrik Nyberg DC wrote:
> I see. That's a problem, because lot's of data is lost on exit. USR1 or 
> USR2 could be used, I guess. I wonder why libmilter is not returning to 
> main after it catches SIGTERM. atexit is not triggered correctly.

We could trigger milter-greylist exit using a control socket, or using the
MX sync socket...

-- 
Emmanuel Dreyfus
manu@...

Emmanuel Dreyfus wrote:
> On Thu, Oct 20, 2005 at 11:45:03AM +0300, Fredrik Nyberg DC wrote:
> 
>>I see. That's a problem, because lot's of data is lost on exit. USR1 or 
>>USR2 could be used, I guess. I wonder why libmilter is not returning to 
>>main after it catches SIGTERM. atexit is not triggered correctly.
> 
> 
> We could trigger milter-greylist exit using a control socket, or using the
> MX sync socket...

How does this solve the problem that libmilter apparently catches 
SIGTERM and does not return to main?

Or am I missing something?

Cheers,
Fredrik Nyberg

On Fri, Oct 21, 2005 at 12:27:11PM +0300, Fredrik Nyberg DC wrote:
> >>I see. That's a problem, because lot's of data is lost on exit. USR1 or 
> >>USR2 could be used, I guess. I wonder why libmilter is not returning to 
> >>main after it catches SIGTERM. atexit is not triggered correctly.
> >We could trigger milter-greylist exit using a control socket, or using the
> >MX sync socket...

> How does this solve the problem that libmilter apparently catches 
> SIGTERM and does not return to main?
> Or am I missing something?

The goal is to remotely shut-down milter-greylist with a disk dump. You
can do this by issuing a signal and catching it, but that is wrong
because libmilter documentation says we should not catch signals. 

An alternative is to have a control socket, or to use the MX sync socket
to get the shutdown command.

-- 
Emmanuel Dreyfus
manu@...

Emmanuel Dreyfus wrote:
> The goal is to remotely shut-down milter-greylist with a disk dump. You
> can do this by issuing a signal and catching it, but that is wrong
> because libmilter documentation says we should not catch signals. 
> 
> An alternative is to have a control socket, or to use the MX sync socket
> to get the shutdown command.

OK, I see your point. Of course, the problem could also be solved using 
a persistent datastore. SQLLite has been mentioned, I also saw it on the 
todo list on the milter-greylist homepage. Where do you think efforts 
should be concentrated?

Fredrik Nyberg

On Fri, Oct 21, 2005 at 02:18:36PM +0300, Fredrik Nyberg DC wrote:
> OK, I see your point. Of course, the problem could also be solved using 
> a persistent datastore. SQLLite has been mentioned, I also saw it on the 
> todo list on the milter-greylist homepage. Where do you think efforts 
> should be concentrated?

IMO the first step is to change autowhite.c and pending.c so that
there is a clean separation between the storage backend and milter-greylist.

The two files shouldbe merged into a unique file for the local storage
backend. it could be named storage_local.c for instance.

Then we would define a structure containing function pointers for the
various methods. storage_local.c would define an instance of the structure 
for the local storage backend, and we could add a file for a SQLite backend.

An idea for the structure, after a quick look at the code
struct storage {
	/* Initialization */
	void (*s_init)(void); 

	/* Autowhitelist management */
	struct autowhite *(*s_awget)(struct sockaddr *, 
				     socklen_t, char *, char *, time_t *);
	void (*s_awput)(struct autowhite *);
	void (*s_awadd)(struct sockaddr *, socklen_t, char *, 
			char *, time_t *, char *);
	int (*s_awcheck)(struct sockaddr *, socklen_t, char *, char *, char *);
	int (*s_awtextdump)(FILE *);

	/* greylist management */
	struct pending *(*s_pget)(struct sockaddr *, socklen_t, 
				  char *, char *, time_t);
	int (*s_pcheck)(struct sockaddr *, socklen_t, char *, 
		        char *, time_t *, time_t *, char *);
	void (*s_pdel)(struct sockaddr *, socklen_t, char *, char *, time_t);  
	void (*s_pput)(struct pending *);
	int (*s_ptextdump)(FILE *);
};

storage_local.c would define
struct storage storage_local = {
	local_init, 	/* Would call former autowhite_init and pending_init */
	local_awget,	/* former autowhite_get */
	local_awput,	/* former autowhite_put */
	...
};

Next step is to have a field in struct conf for the storage:
	struct storage *c_storage;

And set defconf so that this field defaults to &storage_local.

Then you have to change any call to autowhite_* or pending_* in the
code to a reference through the the conf.c_storage structure:
	pending = pending_get(sa, salen, from, rcpt, (time_t)0) 
turns into
	pending = (*conf.c_storage->s_pget)(sa, salen, from, rcpt, (time_t)0);


Then you can start writing the SQLite backend, in storage_sqlite.c. At some 
point you'll have to change the config file parser to add a storage keyword.

Of course you might discover that the API I just defined isn't suited for 
other use, or that it bounds to a lot of code duplication. We might have 
to refactor the code to avoid that.

Do you want to give it a try?

-- 
Emmanuel Dreyfus
manu@...

Emmanuel Dreyfus wrote:
> Then you can start writing the SQLite backend, in storage_sqlite.c. At some 
> point you'll have to change the config file parser to add a storage keyword.
> 
> Of course you might discover that the API I just defined isn't suited for 
> other use, or that it bounds to a lot of code duplication. We might have 
> to refactor the code to avoid that.
> 
> Do you want to give it a try?

I have have just gotten a new project at work, so I will not have time 
for a while. Perhaps later, if no-one else does it.

Fredrik Nyberg