Created attachment 619 [details]
Add dnsbl_exception_cdb directive

I note that your patch also exempts SMTP AUTH from getting DNSBL-checked. We've
actually been looking at something similar for DNSBLs, namely learning
"which IP addresses are ours? Skip those", and your CDB scheme does appear
to be a possibility for getting data for this that (for sites running qmail)
will make it easier on people to configure; thanks! I would _not_ want to do
it the way you appear to be doing it, however, namely "if it comes in with a
client IP address in the headers, it's OK" - forgeries are too possible (and
likely, if someone puts in private IP addresses in the file in question) -
instead, IP addresses matching said file should not be checked vs DNSBLs, but
others should be. (Sorry if I've misread the patch and it actually doesn't
do that...)

	Thanks,

	-Allen

I'd rather not depend/require CDB for the solution.

> I note that your patch also exempts SMTP AUTH from getting DNSBL-checked.

Oops, that hunk wasn't supposed to be there. I couldn't figure out a better way
to do it, so I checked for the SMTP AUTH data qmail puts in its Received header.
 Our roaming users connect via resold ports (DUL-listed) and we force them to
use SMTP AUTH to relay. They aren't listed in our tcpserver CDB, so we need to
have another way of skipping DNSBL checks for them. Is there a better way that I
chould use instead?

> I would _not_ want to do it the way you appear to be doing it, however, namely
> "if it comes in with a client IP address in the headers, it's OK" - forgeries
> are too possible (and likely, if someone puts in private IP addresses in the
> file in question) - instead, IP addresses matching said file should not be
> checked vs DNSBLs, but others should be.

Oops, nice catch. What do you think of the attached (against HEAD), which just
skips them?

Created attachment 626 [details]
updated patch

Dan:

> I'd rather not depend/require CDB for the solution.

I agree that we shouldn't require it for an overall solution, but if the
databases already exist at some sites, why not take advantage of them if it
can be done without too much trouble? It's like making a SQL configuration
option available for sites running with it, but also the text format for
others.

John:

>> I note that your patch also exempts SMTP AUTH from getting DNSBL-checked.

>Oops, that hunk wasn't supposed to be there. I couldn't figure out a better way
>to do it, so I checked for the SMTP AUTH data qmail puts in its Received
>header.
>Our roaming users connect via resold ports (DUL-listed) and we force them to
>use SMTP AUTH to relay. They aren't listed in our tcpserver CDB, so we need to
>have another way of skipping DNSBL checks for them. Is there a better way that
>I chould use instead?

Well, skipping Received headers with SMTP AUTH (and maybe those after them?)
might be OK as an _option_ - just not all the time.

>> I would _not_ want to do it the way you appear to be doing it, however,
>> namely if it comes in with a client IP address in the headers, it's OK" -
>> forgeries are too possible (and likely, if someone puts in private IP
>> addresses in the file in question) - instead, IP addresses matching said file
>> should not be checked vs DNSBLs, but others should be.

> Oops, nice catch. What do you think of the attached (against HEAD), which just
> skips them?

Definitely better; not sure whether checking vs (scalar(keys(%cdb))) might
be better than checking versus (tied(%cdb)).

	-Allen

Subject: Re: [SAdev]  Use a tcpserver-style CDB to exempt clients from DNSBL checking

On Tue, Jan 28, 2003 at 01:49:09PM -0800, bugzilla-daemon@hughes-family.org wrote:
> I agree that we shouldn't require it for an overall solution, but if the
> databases already exist at some sites, why not take advantage of them if it
> can be done without too much trouble?

I was actually thinking about adding accessdb support for 2.60.
For me, it would let me not block mails at the SMTP level (since I've
had more FPs recently), but still use the same format that all of my
tools already understand.  accessdb also has a "<entry> RELAY" listing
which could do this.  (<entry> is IP, byte-bounded network, domain, etc.)
<shrug>  Just a thought.

> Well, skipping Received headers with SMTP AUTH (and maybe those after them?)
> might be OK as an _option_ - just not all the time.

Perhaps a configuration directive to skip DNSBL checks for messages received by
the local machine with SMTP AUTH?

> Definitely better; not sure whether checking vs (scalar(keys(%cdb))) might
> be better than checking versus (tied(%cdb)).

I double-checked the perl docs; tied() returns undef if a variable isn't tied to
a package; scalar keys returns the number of keys in the hash. tied sounds more
efficient and the CDB could (conceivably) have zero keys.

Allen writes:

> I agree that we shouldn't require it for an overall solution, but if the
> databases already exist at some sites, why not take advantage of them if it
> can be done without too much trouble? It's like making a SQL configuration
> option available for sites running with it, but also the text format for
> others.

The directive and patch should be non-specific to CDB.  We're not going to
add different configuration items for every database type.  Any DB-specific
code should be abstracted out or removed.  We use DBs in other code, but
they are not tied to any one format.  Check the Bayes and AWL code for examples.

"if the databases already exist"?  I don't see that mentioned anywhere nor
do I think it's a reason to implement this in a CDB-specific manner (the wrong
way, in other words).

The SQL stuff should be cleaned up as well, but that's a different matter.

Subject: Re: [SAdev]  Use a tcpserver-style CDB to exempt clients from DNSBL checking

>------- Additional Comments From quinlan@pathname.com  2003-01-28 19:05
>Allen writes:
>
>> I agree that we shouldn't require it for an overall solution, but if the
>> databases already exist at some sites, why not take advantage of them if it
>> can be done without too much trouble? It's like making a SQL configuration
>> option available for sites running with it, but also the text format for
>> others.
>
>The directive and patch should be non-specific to CDB. We're not going to
>add different configuration items for every database type.  Any DB-specific
>code should be abstracted out or removed.  We use DBs in other code, but
>they are not tied to any one format.

Fine, so long as pre-existing databases, such as those that tcpserver uses,
the ones that sendmail uses, etcetera are allowed for, including for cases
where it may not be possible to deduce the type of DB from the filename or
other heuristics.

> Check the Bayes and AWL code for examples.

I'm not arguing against code that's flexible - just the reverse.

>"if the databases already exist"?

They're the databases that tcpserver uses already, similarly to sendmail's
accessdb. For people using tcpserver and qmail, they evidently already exist
(I wouldn't know for sure - I don't use Bernstein's programs); I'm certain
that the equivalent sendmail ones do...

	-Allen

> For people using tcpserver and qmail, they evidently already exist
> (I wouldn't know for sure - I don't use Bernstein's programs);

Right; they're somewhat akin to sendmail's accessdb. If you want your clients to
be able to relay, you do it by setting environment variables on a
per-IP/netblock basis in tcpserver's CDB.

Created attachment 876 [details]
updated for current CVS

The DNS code in CVS has massively changed over the last few weeks.

First, is this code still necessary now that we have the trusted IP code?
I suspect you'll say it is, but it never hurts to ask.  :-)

The patch will also have some performance problems as it's currently written.
The code to remove IP addresses should be run only once per message (perhaps
in Received.pm) not in the inside of a relatively hot loop.  There's also
some code for handling bitmasks and such now too, so the foreach loop should
not be necessary now.  I guess it really just needs to be rewritten for the
new DNS code, sorry, but it should be possible to make it faster (and the
patch smaller) now.

The only other worry I have is that this code is so specific to CDB.  I'd
prefer if it was based on DBI/DBD or AnyDBM.

Theo, can you take a look at this bug and update it with your thoughts?  I
don't use CDB here myself.

Subject: Re: [SAdev]  Use a tcpserver-style CDB to exempt clients from DNSBL checking 


> First, is this code still necessary now that we have the trusted IP code?
> I suspect you'll say it is, but it never hurts to ask.  :-)

I would feel it probably *isn't*.  If you know a relay's IP address,
and want to exempt it from RBL testing, chances are you know who's
running it and trust them not to inject spam to you -- so you can
just specify that relay as trusted.

--j.

The trusted_networks code will do fine.

For bug archival purposes, I'll plan on using something like:

cdbdump <smtp.cdb | egrep -av '\+[0-9]+,[0-9]+:->' | \
sed 's#[^:]*:\([0-9\.]*\)->.*#trusted_networks \1#'

The only issue is that SpamAssassin defaults to a /32 mask if none is specified.
tcpserver's CDB support looks for short addresses and assumes a classful mask, e.g.:

127.   # 127/8
1.2.   # 1.2.0.0/16
1.2.3. # 1.2.3.0/24

The above example isn't quite smart enough to deal with this (you'll get
"trusted_networks 127. in your SpamAssassin configuration), but that can be
written off as a matter for local scripting.

Dunno what reason to use when closing this bug, I'll leave it up to you.

Subject: Re: [SAdev]  Use a tcpserver-style CDB to exempt clients from DNSBL checking 


> The only issue is that SpamAssassin defaults to a /32 mask if none is
> specifi ed.  tcpserver's CDB support looks for short addresses and
> assumes a classful mask , e.g.:
> 
> 127.   # 127/8
> 1.2.   # 1.2.0.0/16
> 1.2.3. # 1.2.3.0/24
> 
> The above example isn't quite smart enough to deal with this (you'll get
> "trusted_networks 127. in your SpamAssassin configuration), but that can
> be written off as a matter for local scripting.

Hey, I'll gladly accept a patch ;)  That would be a cool usability feature.

--j.

retitling bug to reflect current status.  reminder: a patch would be great ;)

Yup, I'll look at writing something in a couple of weeks. Quite busy at the
moment...

Subject: Re: [SAdev]  trusted_networks should accept short CIDR specifications

On Wed, May 07, 2003 at 11:14:35AM -0700, bugzilla-daemon@hughes-family.org wrote:
> retitling bug to reflect current status.  reminder: a patch would be great ;)

I have some code to figure out if an IP is in a given network.  supports
netmask and cidr.  I can dig it out of my mkrdns script if it'd fit
here. :)

Subject: Re: [SAdev]  trusted_networks should accept short CIDR
	specifications

On Wed, 2003-05-07 at 16:48, bugzilla-daemon@hughes-family.org wrote:
> http://www.hughes-family.org/bugzilla/show_bug.cgi?id=1425

> ------- Additional Comments From jwm@horde.net  2003-05-07 13:48 -------
> Yup, I'll look at writing something in a couple of weeks. Quite busy at the
> moment...

I'm not sure I understand this bug. Is the attached patch roughly what's
being looked for? This is off the top of my head, feel free to verify it
against your test case that broke.

Aaron--

I'm going to patch SpamAssassin to support short classful specifications in
trusted_networks, i.e.:

trusted_networks 127.   # adds 127/8
trusted_networks 1.2.3. # adds 1.2.3.0/24

John, BZ seems to have dropped my patch, but if you look at the dev mailing list
version of my mail, it's a very simple change. Just split out any ip ending in
".", join in back together using map to add 0 to each entry. Under -w, that will
give you a warning about undefined values being used (when you actually handle
such an IP), but other than that it's safe as houses. I do a bit of error
checking as well.

That should address exactly what you said.

Would this be the type of functionality best left to a simple shell
script rather than adding to the trusted_networks specification and code?

So, can we close this bug?

dan, not sure how a shell script would help; are you suggesting preprocessing
the system's SA config files?  urgh ;)

I'll take a look at this to see if I can get some kind of CIDR support for the
trusted_networks code quickly.

to me!

> dan, not sure how a shell script would help; are you suggesting preprocessing
> the system's SA config files?  urgh ;)

Yep, I'm suggesting exactly that.  Anyone exporting addresses from a CDB is
going to have a shell script to create SA config files anyway.

> I'll take a look at this to see if I can get some kind of CIDR support for the
> trusted_networks code quickly.

Why is this needed?

ok, now in (eventually). closing bug

Does nobody agree with me that this code should not be part of the SA module-set?

It's only used for CDB.  It should be a separate script.  People exporting
from CDB to SA are going to have to format stuff (at least adding the
configuration option name and whitespace) for configuration files anyway.

Also, putting tests in the modules adds to their length which makes perlapp
executables larger, I'd prefer to keep tests in separate files.

Subject: Re:  trusted_networks should accept short CIDR specifications 


> Does nobody agree with me that this code should not be part of the SA module-
> set?
> It's only used for CDB.  It should be a separate script.  People exporting
> from CDB to SA are going to have to format stuff (at least adding the
> configuration option name and whitespace) for configuration files anyway.

No -- it's not just used for CDB.  it's a way to replace the CDB code.
It allows trusted_networks to use default netmasks -- so "127." can
be used instead of "127/8" in CIDR notation.  it's a convenience...

> Also, putting tests in the modules adds to their length which makes perlapp
> executables larger, I'd prefer to keep tests in separate files.

I can make it a POD section instead?

--j.