Milter-greylist

Hi,

I have switched from version 1.6 to 2.0rc3 on my production
mail server, and noticed a serious misbehaviour in the handling
of "acl whitelist domain" statements.

Having this in greylist.conf:

  acl whitelist domain example.net

will only whitelist when the hostname is exactly "example.net",
"host.example.net" will be denied. This is not how it's documented,
and in 1.6 it worked as advertised.

I think the culprit is emailcmp() in acl.c - it doesn't match
substrings, only completely equal strings. Do we need a new
domaincmp() function, which would compare hostname to domain
by starting from the end of both strings working backwards,
and returning success when the beginning of domain has been
reached without any differing characters?

A workaround would be to use a regexp:

  acl whitelist domain /.*\.example\.net$/
  
I think this should be fixed before 2.0 final.

mp.

On Tue, Jun 07, 2005 at 11:31:43AM +0200, Martin Paul wrote:
>   acl whitelist domain example.net
> 
> will only whitelist when the hostname is exactly "example.net",
> "host.example.net" will be denied. This is not how it's documented,
> and in 1.6 it worked as advertised.
(snip)
> I think this should be fixed before 2.0 final.

Yes, someone already reported that, I need to find some time to fix it.
Obviously we need a domaincmp function that does substring match. 

-- 
Emmanuel Dreyfus
manu@...

Martin Paul <martin@...> wrote:

> I have switched from version 1.6 to 2.0rc3 on my production
> mail server, and noticed a serious misbehaviour in the handling
> of "acl whitelist domain" statements.

I wrote a quick fix for that problem, but I have no time to test it. Can
you give me your feedback?
http://hcpnet.free.fr/milter-greylist/domaincmp.diff 

(in a nutshell, I used the older emailcmp function for domain
comparison.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

> > I have switched from version 1.6 to 2.0rc3 on my production
> > mail server, and noticed a serious misbehaviour in the handling
> > of "acl whitelist domain" statements.
> 
> I wrote a quick fix for that problem, but I have no time to test it. Can
> you give me your feedback?
> http://hcpnet.free.fr/milter-greylist/domaincmp.diff 
> 
> (in a nutshell, I used the older emailcmp function for domain
> comparison.

In the meantime I had produced a similar fix, also using a new
domaincmp() function, but written from scratch. I'll attach the
patch.

I'm running with my patch in production now, it works fine.

There's one functional difference between our two implementations
of domaincmp() - yours matches any hostname that contains domain
anywhere (/^.*domain.*$/), mine matches from the end to the beginning
(/^.*domain$). I would much prefer the behaviour of my function -
a hostname "worldnet.att.com" would match a domain "net.at" with
your function, which is terribly confusing IMHO. I'd leave substring
matching to regexps. I hope you agree.

mp.

Martin Paul <martin@...> wrote:

> There's one functional difference between our two implementations
> of domaincmp() - yours matches any hostname that contains domain
> anywhere (/^.*domain.*$/), mine matches from the end to the beginning
> (/^.*domain$). I would much prefer the behaviour of my function -
> a hostname "worldnet.att.com" would match a domain "net.at" with
> your function, which is terribly confusing IMHO. I'd leave substring
> matching to regexps. I hope you agree.

My goal was to preserve the historic (but odd) behaviour. You are
probably right to do it that way. 

-- 
Emmanuel Dreyfus
Publicité subliminale: achetez ce livre!
http://www.eyrolles.com/Informatique/Livre/9782212114638/livre-bsd.php
manu@netbsd.org

> > I would much prefer the behaviour of my function -
> > a hostname "worldnet.att.com" would match a domain "net.at" with
> > your function, which is terribly confusing IMHO. I'd leave substring
> > matching to regexps. I hope you agree.
> 
> My goal was to preserve the historic (but odd) behaviour.

I understand that, but ..

> You are probably right to do it that way. 

.. as the odd substring matching behaviour was never documented
like that, as no one seems to be interested in it, and as going
from 1.6 to 2.0 is a new major release which valids some minor
changes in behaviour, I think it's the way to go.

The similar behaviour in emailcmp (for "rcpt" options) in 1.6
had resulted in some debugging for me when starting to use
milter-greylist, too. I was running in testmode with an
"rcpt ian@..." clause in greylist.conf, and wondered
why mails to marian@... got greylisted, too, until
I found out that it matches substrings ..

mp.

Martin Paul <martin@...> wrote:

> .. as the odd substring matching behaviour was never documented
> like that, as no one seems to be interested in it, and as going
> from 1.6 to 2.0 is a new major release which valids some minor
> changes in behaviour, I think it's the way to go.

Yep, I checked in your code.

-- 
Emmanuel Dreyfus
Publicité subliminale: achetez ce livre!
http://www.eyrolles.com/Informatique/Livre/9782212114638/livre-bsd.php
manu@...

Here is milter-greylist 2.0 release candidate 4: 

http://ftp.espci.fr/pub/milter-greylist/milter-greylist-2.0rc4.tgz
MD5 (milter-greylist-2.0rc4.tgz) = 1acf3770dd590b34f58e7051c5cb50cc  

The only fix is the domain suffix match, from Martin Paul.

-- 
Emmanuel Dreyfus
Un bouquin en français sur BSD:
http://www.eyrolles.com/Informatique/Livre/9782212114638/livre-bsd.php
manu@...

On Thu, 9 Jun 2005 manu@... wrote:
> Here is milter-greylist 2.0 release candidate 4: 
> http://ftp.espci.fr/pub/milter-greylist/milter-greylist-2.0rc4.tgz
> MD5 (milter-greylist-2.0rc4.tgz) = 1acf3770dd590b34f58e7051c5cb50cc  
> The only fix is the domain suffix match, from Martin Paul.

It was packaged incorrectly.

# rpmbuild -ta milter-greylist-2.0rc4.tgz 
error: File /usr/src/milter-greylist/milter-greylist-2.0rc3.tgz: No such file or directory

Maybe you should script your packaging steps?

-Dan

Dan Hollis <goemon@...> wrote:

> It was packaged incorrectly.
> 
> # rpmbuild -ta milter-greylist-2.0rc4.tgz 
> error: File /usr/src/milter-greylist/milter-greylist-2.0rc3.tgz: No such
> file or directory
> 
> Maybe you should script your packaging steps?

Well, sure I should, but here it would not have saved me because I
checked out 2.0rc3 instead of 2.0rc4 from CVS.

Here is the real 2.0rc4 with its real MD5 sum:

http://ftp.espci.fr/pub/milter-greylist/milter-greylist-2.0rc4.tgz
MD5 (milter-greylist-2.0rc4.tgz) = a019f395c009fd398a3a5efc60382b1e

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

> Here is milter-greylist 2.0 release candidate 4: 
> 
> http://ftp.espci.fr/pub/milter-greylist/milter-greylist-2.0rc4.tgz
> MD5 (milter-greylist-2.0rc4.tgz) = 1acf3770dd590b34f58e7051c5cb50cc  
> 
> The only fix is the domain suffix match, from Martin Paul.

You made a mistake when integrating the domaincmp() patch -
unlike emailcmp() it returns 1 (TRUE) on a match and 0 (FALSE)
when not matching, so you have to fix this in acl.c:

% diff acl.c.orig acl.c
452c452
<                       if (domaincmp(hostname, acl->a_domain) == 0)
---
>                       if (domaincmp(hostname, acl->a_domain))

As it is now, "acl whitelist domain" will do the wrong thing, so
nobody should use it with 2.0rc4.

The change you made to greylist.conf.5 needs a fix, too. from/rcpt
are not doing an exact, but a substring match, as mentioned in a
previous message.

% diff greylist.conf.5.orig greylist.conf.5
123c123
< When regluar expressions are not used, 
---
> When regular expressions are not used, 
127c127
< perform a case insensitive exact match with leading and trailing brackets, 
---
> perform a case insensitive substring match with leading and trailing brackets,

mp.

On Fri, Jun 10, 2005 at 09:58:36AM +0200, Martin Paul wrote:
> You made a mistake when integrating the domaincmp() patch -
> unlike emailcmp() it returns 1 (TRUE) on a match and 0 (FALSE)
> when not matching, so you have to fix this in acl.c:
> 
> % diff acl.c.orig acl.c
> 452c452
> <                       if (domaincmp(hostname, acl->a_domain) == 0)
> ---
> >                       if (domaincmp(hostname, acl->a_domain))
> 
> As it is now, "acl whitelist domain" will do the wrong thing, so
> nobody should use it with 2.0rc4.

Sometime a fix looks so simple you think there is no need for testing, 
this seems to turn always wrong... Let's roll 2.0rc5 (where I added your
fix without testing it because I'm swamped with other duties, let's hope
it's okay)

-- 
Emmanuel Dreyfus
manu@...

> Sometime a fix looks so simple you think there is no need for testing, 
> this seems to turn always wrong... Let's roll 2.0rc5 (where I added your
> fix without testing it because I'm swamped with other duties, let's hope
> it's okay)

2.0rc5 looks fine now. I've put it into production. Thanks for the
quick reaction.

mp.