On Feb 8, 2006, at 12:42 AM, Rian Hunter wrote:
On Feb 7, 2006, at 8:15 PM, Brian J. France wrote:
Before I started converting my other modules to the new code I
figured I would start with writing a new module to handle the SIZE
extension. I needed to apply the following patch (link below) to
the mod_smtpd code to get access at the max data size.
+0, Well this is an interesting point. The max data size should be
settable and gettable from extending modules. In a way max_data is
a lot like the list of extensions: should it be set on every
connection or upon server initialization (ie should it's scope be
per connection or per mod_smtpd instance). I chose that the list of
extensions might want to be per-connection since modules may not
want to offer all clients all extensions, just the same way modules
may want to enforce different max_data sizes for different clients.
Where do you think these variables belong?
One thing I do know is that max_data doesn't belong in mod_smtpd's
configuration structure and rather either in some other "per
mod_smtpd instance" structure accessible to extending modules or
just in scr (the per connection structure). Right? I think that
would be cleaner and more modular. We should talk about this some
more before I apply this patch.
+1, I like the idea of the storing the setting in the per connection
instance. Leaving the default in the core and copying it to an per
connection struct would allow modules to tweak the setting per
connection.
One other addition to the smtpd_session_rec I would like to see would
be something like r->notes or r->subprocess_env. Some way for
modules to set flags that would change the behavior of another
module. As an example I might want to add a new modules that changes
the max data size based on either connection ip or mail from
address. This module would want to set a flag telling the size
module to not show the numerical size limit in the ehlo response as
it might change based on the the mail from address.
I hooked the mail from hook, check for a valid SIZE in
mail_parameters and check to make sure it is not over the limit.
If it is over the limit I can use smtpd_respond_oneline to send
the 552 "message exceeds fixed maximum message size" line back to
the client, but what should the function return to make it force a
QUIT or REST command as anything but SMTPD_DONE sends more stuff
to the client.
Should I just return SMTPD_DONE and set scr->should_disconnect?
Could we tweak it to support two different settings, one would
only allow QUIT only and the other would allow QUIT and REST (to
start over).
+1, Since quit and rset are both handled by mod_smtpd there should
probably be another variable called scr->only_quit_or_rset, also
because I think there are other times when the client should only
issue a quit or rset. More discussion follows though:
My basic design strategy has been that mod_smtpd should do as
little as possible or what will be practical to a large amount of
extending modules. If mod_smtpd sets a variable in a structure it
should use it. If an extending module sets a variable in a
structure it should use it. Having an "only_quit_or_rset" variable
in the scr structure with mod_smtpd consciously checking for it,
but never changing the value itself sort of violates that strategy.
That doesn't mean I'm against it because like I already mentioned I
think a lot of modules will need this sort of thing.
Have you thought about hooking into all the commands, and then
sending a "503 Only QUIT and RSET" from your SIZE module until a
rset is received?
I only added the scr->should_disconnect logic since it's what
should happen when a simple module wants to SMTPD_DENY some
connections. I didn't want every module that wanted to implement
connection policy to have to deal with being hooked into all the
commands, and just be able to return SMTPD_DENY.
The scr->should_disconnect situation I just explained (where I
didn't want every modules to have to deal with hooking all the
commands for a redundant task) could be applied to any potential
module that wants to disallow any of the built in commands with
instead a bit-field that specifies which mod_smtpd core commands
are currently allowed. I think I like this idea the best, what do
you think?
+1, if I understand you currently. Changing scr->should_disconnect
to scr->allows_commands which is a bit-field of what commands that
are allowed. Then smtp_protocol.c handles denying command instead of
having a module hook every command and doing it, right?
Brian
