Re: mod_smtpd module review

2005-09-20 Thread Jem Berkes
I just saw this old message now... I have been moving and my new ISP still 
hasn't connected my service after 3 weeks.



This past week I have finished up a few modules and ready for review.

http://www.brianfrance.com/software/apache/mod_smtpd_load.tar.gz


This is a nifty addition, thanks!

I would also like to set the error code, because looking over rfc0821 I 
think it should return 452 or may be that needs to be a default for 
smtpd_run_connect soft errors (552 for hard errors).  Should we allow the 
module to set the error code?


Error codes can be sent with SMTPD_DONE. Otherwise error codes won't change 
in the next fifty years for things like SMTPD_DENY and friends. I'll look 
into changing those specific codes. Most of the error codes I got from 
Postfix and Qpsmtpd but of course they aren't perfect.


I think there should be a way for other modules to specify a preferred 
return code. Access controls are a good example, whether mod_smtpd_rbl or 
something else that wants to ensure a specific code is returned.


Re: mod_smtpd module review

2005-08-31 Thread Rian Hunter


On Aug 30, 2005, at 11:19 AM, Brian J. France wrote:


This past week I have finished up a few modules and ready for review.

http://www.brianfrance.com/software/apache/mod_smtpd_load.tar.gz

mod_smtpd_load:
  This module allows rejecting connection (temporarily) based on  
server load
  It is not very cross platform (any os with getloadavg), but I am  
sure we

  can work on that.

I have finished mod_smtpd_access_dbi, but after talking with Paul  
on IRC
I need to convert it to use mod_dbd instead of mod_dbi_pool.   
Working on that
now and will post another message when done.  I could build a tar  
ball of this
module if anybody is interested as the the flow will not change,  
just how the

db connection is handled.

mod_smtpd_access_dbi:
This module is similar to sendmails access file.  It allows
checking of the ip/hostname/from/to items via a database to see if
things should be rejected.  It uses mod_dbi_pool and libdbi.
Thanks to Paul for mod_dbi_pool and code examples from  
mod_vhost_dbi.


This is great!

A few things I have found while writing these modules. How about  
changing smtpd_return_data from this:


typedef struct smtpd_return_data {
apr_pool_t *p;
/* list of messages */
apr_array_header_t *msgs;
} smtpd_return_data;

to something like this:

typedef struct smtpd_return_data {
apr_pool_t *p;
int code;
/* list of messages */
apr_array_header_t *user_msgs;
apr_array_header_t *log_msgs;
} smtpd_return_data;

While doing the mod_smtpd_load module I found when I want to deny a  
connection I can set what message the user will get, but I also  
want to log a different message instead of the default Connection  
Denied (current I log my own and the default gets logged).  Of  
course this might be another thread of how and what do we plan on  
logging.




IMO I think that logging twice is fine. mod_smtpd logs Connection  
Denied because a server admin may want to know what connections are  
getting denied for debugging and etc. and I feel its absolutely  
necessary, same with the MAIL command. If your module wants to log  
extra information, I think that's OK.


Maybe for clarity all mod_smtpd's logs can be prefixed with  
mod_smtpd: and other modules can follow suit.


I would also like to set the error code, because looking over  
rfc0821 I think it should return 452 or may be that needs to be a  
default for smtpd_run_connect soft errors (552 for hard errors).   
Should we allow the module to set the error code?


Error codes can be sent with SMTPD_DONE. Otherwise error codes won't  
change in the next fifty years for things like SMTPD_DENY and  
friends. I'll look into changing those specific codes. Most of the  
error codes I got from Postfix and Qpsmtpd but of course they aren't  
perfect.


In mod_smtpd_access_dbi I found it strange that I get a string that  
looks like this:  [EMAIL PROTECTED] instead of  
[EMAIL PROTECTED].  For the mail/rcpt hooks should we send a struct  
that has the full line sent, the data from the full line and the  
parsed email address?  I have some code that duplicates the string  
and then remove spaces and  from the beginning and  from the end,  
but that seems like it should be done before my function is  
called.  Another problem I can see is when we get into things like  
the size options:


MAIL FROM:[EMAIL PROTECTED] SIZE=50

Do we want every module to have to parse the email address  
(removing  ) and the SIZE?


This is a very good point. Honestly I'm not really sure since I'm not  
experienced with writing a plugin based architecture, but I'm almost  
positive that most people will say that mod_smtpd should parse the  
email address and options along with it. When I first wrote mod_smtpd  
I figured I'd pass the un-parsed string after from: verbatim to let  
the modules decide what to do with it. Maybe to allow some crazy  
things. Either way, having mod_smtpd parse sounds like the right idea  
so how about this hook for mail:


smtp_run_mail(smtp_conn_rec *, smtp_return_data *, char  
*parsed_address, apr_array_header_t *options);


The options won't be stored as name-value pairs: more like an array  
of strings like SIZE=5. Does that sound good?


Rian Hunter


mod_smtpd module review

2005-08-30 Thread Brian J. France

This past week I have finished up a few modules and ready for review.

http://www.brianfrance.com/software/apache/mod_smtpd_load.tar.gz

mod_smtpd_load:
  This module allows rejecting connection (temporarily) based on server 
load
  It is not very cross platform (any os with getloadavg), but I am sure 
we

  can work on that.

I have finished mod_smtpd_access_dbi, but after talking with Paul on IRC
I need to convert it to use mod_dbd instead of mod_dbi_pool.  Working 
on that
now and will post another message when done.  I could build a tar ball 
of this
module if anybody is interested as the the flow will not change, just 
how the

db connection is handled.

mod_smtpd_access_dbi:
This module is similar to sendmails access file.  It allows
checking of the ip/hostname/from/to items via a database to see if
things should be rejected.  It uses mod_dbi_pool and libdbi.
Thanks to Paul for mod_dbi_pool and code examples from mod_vhost_dbi.

A few things I have found while writing these modules. How about 
changing smtpd_return_data from this:


typedef struct smtpd_return_data {
apr_pool_t *p;
/* list of messages */
apr_array_header_t *msgs;
} smtpd_return_data;

to something like this:

typedef struct smtpd_return_data {
apr_pool_t *p;
int code;
/* list of messages */
apr_array_header_t *user_msgs;
apr_array_header_t *log_msgs;
} smtpd_return_data;

While doing the mod_smtpd_load module I found when I want to deny a 
connection I can set what message the user will get, but I also want to 
log a different message instead of the default Connection Denied 
(current I log my own and the default gets logged).  Of course this 
might be another thread of how and what do we plan on logging.


I would also like to set the error code, because looking over rfc0821 I 
think it should return 452 or may be that needs to be a default for 
smtpd_run_connect soft errors (552 for hard errors).  Should we allow 
the module to set the error code?


In mod_smtpd_access_dbi I found it strange that I get a string that 
looks like this:  [EMAIL PROTECTED] instead of [EMAIL PROTECTED].  
For the mail/rcpt hooks should we send a struct that has the full line 
sent, the data from the full line and the parsed email address?  I have 
some code that duplicates the string and then remove spaces and  from 
the beginning and  from the end, but that seems like it should be done 
before my function is called.  Another problem I can see is when we get 
into things like the size options:


MAIL FROM:[EMAIL PROTECTED] SIZE=50

Do we want every module to have to parse the email address (removing  
) and the SIZE?


Brian