On Fri, 17 Mar 2006 14:07:35 -0800 Ask Bjørn Hansen <[EMAIL PROTECTED]> wrote: > On Mar 16, 2006, at 10:38 PM, Hanno Hecker (via RT) wrote: > > I'd like to see a patch that makes a better API for reading the > command and the command parameters. The default can still be having > it space separated and provided in @_, but the hacks in mail() and > rcpt() to read the rest aren't very nice so we should have an API to > make it neater. > > Maybe just have a method to give the full unmodified command and use > that, I don't know. Ok, this is a first (draft of a ;->) patch. The Qpsmtpd::Command contents can easily be moved to Qpsmtpd::SMTP if desired. This patch adds some hooks: * (helo|ehlo|mail|rcpt|auth|unrecognized_command)_parse: i.e. all commands, which accept arguments (except VRFY). these hooks get no arguments except $self,$transaction/$connection They MUST return(OK, \&ref_to_parsing_sub); to be the parser for this command (in fact any return code from Constants.pm which stops any other following plugin is ok). The ref_to_parsing_sub() gets $self, $cmd, $line as arguments. $self is here a Qpsmtpd::Command, which inherits from Qpsmtpd::SMTP, so the usual $self->log(LOGDEBUG, "bla") stuff works. $cmd is the name of the command (i.e. the chars up to the first space in the line from the client), $line the rest of the line. This sub MUST return ($parsing_ok, $first, @param), like the default return($cmd, split(/ +/, $line)); The parse_addr_withhelo plugin enforces strict RFC 821 parsing, which does not allow RCPT/MAIL parameters (a quick'n'dirty demo plugin :-))
* (mail|rcpt)_pre these hooks get $self,$transaction,$addr. $addr is the $first from above. Here the 'dont_require_angelbrackets' plugin adds the "<",">" if not present to establish the current (i.e. without this patch) behaviour of accepting addresses without the <>. This (for example) can also be used to strip a trailing dot from an address, if wanted Docs will follow later if it's finished > Also: Tests, please! It's pretty simple to add tests for "is the > parsing working right". ok, done.... sometimes it fails (but never from telnet to port 2525) this seems to be a strange thing in 'make test'. Does 'make test' run the commands in the same order every time? > To add to the bikeshedding, I don't think it should be configurable > -- it'd be fun to require them and see how it goes. If it doesn't > work well, then just make them optional again and a rcpt_pre plugin > can make them required for those who want that. Options in the core > are bad, generally speaking. hmm, ok, now we have a plugin which does exactly that... don't load -> no addresses without brackets are accepted ;-) Hanno
diff -Nurx.svn ../0.3x/config.sample/plugins ./config.sample/plugins --- ../0.3x/config.sample/plugins 2006-01-13 07:28:49.000000000 +0100 +++ ./config.sample/plugins 2006-03-18 19:35:28.000000000 +0100 @@ -12,6 +12,13 @@ # from one IP! hosts_allow +# enable to accept MAIL FROM:/RCPT TO: addresses without surrounding <> +# dont_require_angelbrackets + +# enable to reject MAIL FROM:/RCPT TO: parameters if client helo was HELO +# (strict RFC 821)... this is not used in EHLO ... +# parse_addr_withhelo + quit_fortune check_earlytalker diff -Nurx.svn ../0.3x/lib/Apache/Qpsmtpd.pm ./lib/Apache/Qpsmtpd.pm --- ../0.3x/lib/Apache/Qpsmtpd.pm 2005-11-20 10:52:17.000000000 +0100 +++ ./lib/Apache/Qpsmtpd.pm 2006-03-18 19:30:04.000000000 +0100 @@ -131,8 +131,10 @@ while (defined(my $data = $self->getline)) { $data =~ s/\r?\n$//s; # advanced chomp $self->log(LOGDEBUG, "dispatching $data"); - defined $self->dispatch(split / +/, $data) - or $self->respond(502, "command unrecognized: '$data'"); + $data =~ s/^\s*//; + my ($cmd, $line) = split / +/, $data, 2); + defined $self->dispatch($cmd, $line) + or $self->respond(502, "command unrecognized: '$cmd'"); last if $self->{_quitting}; } } diff -Nurx.svn ../0.3x/lib/Qpsmtpd/Command.pm ./lib/Qpsmtpd/Command.pm --- ../0.3x/lib/Qpsmtpd/Command.pm 1970-01-01 01:00:00.000000000 +0100 +++ ./lib/Qpsmtpd/Command.pm 2006-03-18 18:29:45.000000000 +0100 @@ -0,0 +1,84 @@ +package Qpsmtpd::Command; +# use Qpsmtpd::SMTP; +use Qpsmtpd::Constants; +use vars qw(@ISA); [EMAIL PROTECTED] = qw(Qpsmtpd::SMTP); +use strict; + +sub parse { + my ($me,$cmd,$line,$sub) = @_; + my $self = {}; + bless $self, $me; + $cmd = lc $1; + if ($sub and (ref($sub) eq 'CODE')) { + my @ret = eval { $sub->($self, $cmd, $line); }; + if ($@) { + $self->log(LOGERROR, "Failed to parse command [$cmd]: $@"); + return (undef, $line, ()); + } + my @log = @ret; + for (@log) { + $_ ||= ""; + } + $self->log(LOGDEBUG, "parse($cmd) => [".join("], [", @log)."]"); + return @ret; + } + my $parse = "parse_$cmd"; + if ($self->can($parse)) { + # print "CMD=$cmd,line=$line\n"; + my @out = eval { $self->$parse($cmd, $line); }; + if ($@) { + $self->log(LOGERROR, "XXX: $@"); + return(undef, $line, ()); + } + return @out; + } + return($cmd, split(/ +/, $line)); # default :) +} + +sub parse_rcpt { + my ($self,$cmd,$line) = @_; + return undef unless $line =~ s/^to:\s*//i; + return &_get_mail_params($cmd, $line); +} + +sub parse_mail { + my ($self,$cmd,$line) = @_; + return undef unless $line =~ s/^from:\s*//i; + return &_get_mail_params($cmd, $line); +} + +sub _get_mail_params { + my ($cmd,$line) = @_; + my @params = (); + $line =~ s/\s*$//; + # start stripping parameters from the end, so the last thing not a + # parameter must be an address... + # see also RFC 1869 + # while ($line =~ s/ +([A-Za-z0-9][A-Za-z0-9\-]*)=([^= \x00-\x1f]+)$//) { + while ($line =~ s/\s+([A-Za-z0-9][A-Za-z0-9\-]*=[^= \x00-\x1f]+)$//) { + push @params, $1; + } + + # the above will "fail" on something like + # MAIL FROM: [EMAIL PROTECTED] FOO=BAR + # (not: MAIL FROM:[EMAIL PROTECTED] FOO=bar, + # MAIL FROM: <user=example.net> FOO=bar) + # so let's see if $line contains nothing and treat the first value + # as address: + unless ($line) { + $line = pop(@params) || "<>"; # treat 'MAIL FROM:' as 'MAIL FROM:<>' + } + + # now there's just an address in $line, no spaces around it + # ...ready to feed Qpsmtpd::Address... + + #### XXX: No: let this do a plugin, so it's not up to us to decide + #### if we require <> around an address :-) + # unless ($line =~ /^<.*>$/) { + # $line = "<".$line.">"; + # } + return ($cmd, $line, @params); +} + +1; diff -Nurx.svn ../0.3x/lib/Qpsmtpd/Plugin.pm ./lib/Qpsmtpd/Plugin.pm --- ../0.3x/lib/Qpsmtpd/Plugin.pm 2006-03-04 08:36:21.000000000 +0100 +++ ./lib/Qpsmtpd/Plugin.pm 2006-03-18 14:03:26.000000000 +0100 @@ -4,11 +4,12 @@ # more or less in the order they will fire our @hooks = qw( - logging config pre-connection connect ehlo helo - auth auth-plain auth-login auth-cram-md5 - rcpt mail data data_post queue_pre queue queue_post + logging config pre-connection connect ehlo_parse ehlo helo_parse helo + auth_parse auth auth-plain auth-login auth-cram-md5 + rcpt_parse rcpt_pre rcpt mail_parse mail mail_pre + data data_post queue_pre queue queue_post quit reset_transaction disconnect post-connection - unrecognized_command deny ok + unrecognized_command_parse unrecognized_command deny ok ); our %hooks = map { $_ => 1 } @hooks; diff -Nurx.svn ../0.3x/lib/Qpsmtpd/SMTP.pm ./lib/Qpsmtpd/SMTP.pm --- ../0.3x/lib/Qpsmtpd/SMTP.pm 2006-03-07 14:59:22.000000000 +0100 +++ ./lib/Qpsmtpd/SMTP.pm 2006-03-18 19:27:23.000000000 +0100 @@ -12,6 +12,7 @@ use Qpsmtpd::Constants; use Qpsmtpd::Auth; use Qpsmtpd::Address (); +use Qpsmtpd::Command; use Mail::Header (); #use Data::Dumper; @@ -47,11 +48,15 @@ sub dispatch { my $self = shift; my ($cmd) = lc shift; + my $line = shift; $self->{_counter}++; if ($cmd !~ /^(\w{1,12})$/ or !exists $self->{_commands}->{$1}) { - my ($rc, $msg) = $self->run_hooks("unrecognized_command", $cmd, @_); + my ($rc, $msg) = $self->run_hooks("unrecognized_command_parse"); + my ($ok, $cmd, @params) = Qpsmtpd::Command->parse($cmd, $line, $msg); + + ($rc, $msg) = $self->run_hooks("unrecognized_command", $cmd, @params); if ($rc == DENY_DISCONNECT) { $self->respond(521, $msg); $self->disconnect; @@ -70,7 +75,7 @@ $cmd = $1; if (1 or $self->{_commands}->{$cmd} and $self->can($cmd)) { - my ($result) = eval { $self->$cmd(@_) }; + my ($result) = eval { $self->$cmd($line) }; $self->log(LOGERROR, "XX: $@") if $@; return $result if defined $result; return $self->fault("command '$cmd' failed unexpectedly"); @@ -140,13 +145,15 @@ sub helo { - my ($self, $hello_host, @stuff) = @_; + my ($self, $line) = @_; + my ($rc, $msg) = $self->run_hooks('helo_parse'); + my ($ok, $hello_host, @stuff) = Qpsmtpd::Command->parse('helo', $line, $msg); return $self->respond (501, "helo requires domain/address - see RFC-2821 4.1.1.1") unless $hello_host; my $conn = $self->connection; return $self->respond (503, "but you already said HELO ...") if $conn->hello; - my ($rc, $msg) = $self->run_hooks("helo", $hello_host, @stuff); + ($rc, $msg) = $self->run_hooks("helo", $hello_host, @stuff); if ($rc == DONE) { # do nothing } elsif ($rc == DENY) { @@ -168,13 +175,15 @@ } sub ehlo { - my ($self, $hello_host, @stuff) = @_; + my ($self, $line) = @_; + my ($rc, $msg) = $self->run_hooks('ehlo_parse'); + my ($ok, $hello_host, @stuff) = Qpsmtpd::Command->parse('ehlo', $line, $msg); return $self->respond (501, "ehlo requires domain/address - see RFC-2821 4.1.1.1") unless $hello_host; my $conn = $self->connection; return $self->respond (503, "but you already said HELO ...") if $conn->hello; - my ($rc, $msg) = $self->run_hooks("ehlo", $hello_host, @stuff); + ($rc, $msg) = $self->run_hooks("ehlo", $hello_host, @stuff); if ($rc == DONE) { # do nothing } elsif ($rc == DENY) { @@ -226,8 +235,10 @@ } sub auth { - my ( $self, $arg, @stuff ) = @_; - + my ( $self, $line ) = @_; + my ($rc, $sub) = $self->run_hooks('auth_parse'); + my ($ok, $arg, @stuff) = Qpsmtpd::Command->parse('auth', $line, $sub); + #they AUTH'd once already return $self->respond( 503, "but you already said AUTH ..." ) if ( defined $self->{_auth} @@ -239,9 +250,7 @@ } sub mail { - my $self = shift; - return $self->respond(501, "syntax error in parameters") if !$_[0] or $_[0] !~ m/^from:/i; - + my ($self,$line) = @_; # -> from RFC2821 # The MAIL command (or the obsolete SEND, SOML, or SAML commands) # begins a mail transaction. Once started, a mail transaction @@ -266,16 +275,29 @@ return $self->respond(503, "please say hello first ..."); } else { - my $from_parameter = join " ", @_; - $self->log(LOGINFO, "full from_parameter: $from_parameter"); - - my ($from) = ($from_parameter =~ m/^from:\s*(<[^>]*>)/i)[0]; - - # support addresses without <> ... maybe we shouldn't? - ($from) = "<" . ($from_parameter =~ m/^from:\s*(\S+)/i)[0] . ">" - unless $from; - + $self->log(LOGINFO, "full from_parameter: $line"); + my ($rc, $msg) = $self->run_hooks("mail_parse"); + my ($ok,$from,@params) = Qpsmtpd::Command->parse('mail', $line, $msg); + return $self->respond(501, "syntax error in parameters") + unless ($ok and $from); + my %param; + foreach (@params) { + my ($k,$v) = split /=/, $_, 2; + $param{lc $k} = $v; + } + # to support addresses without <> we now require a plugin + # hooking "mail_pre" to + # return (OK, "<$from>"); + # or anything else parseable by Qpsmtpd::Address ;-> + # see also comment in sub rcpt() + ($rc, $msg) = $self->run_hooks("mail_pre", $from); + # $self->log(LOGALERT, "RC=$rc, MSG=$msg"); + if ($rc == OK) { + $from = $msg; + } $self->log(LOGALERT, "from email address : [$from]"); + return $self->respond(501, "could not parse your mail from command") + unless $from =~ /^<.*>$/; if ($from eq "<>" or $from =~ m/\[undefined\]/ or $from eq "<[EMAIL PROTECTED]>") { $from = Qpsmtpd::Address->new("<>"); @@ -284,8 +306,8 @@ $from = (Qpsmtpd::Address->parse($from))[0]; } return $self->respond(501, "could not parse your mail from command") unless $from; - - my ($rc, $msg) = $self->run_hooks("mail", $from); + + ($rc, $msg) = $self->run_hooks("mail", $from, %param); if ($rc == DONE) { return 1; } @@ -320,18 +342,39 @@ } sub rcpt { - my $self = shift; - return $self->respond(501, "syntax error in parameters") unless $_[0] and $_[0] =~ m/^to:/i; + my ($self,$line) = @_; + my ($rc, $msg) = $self->run_hooks("rcpt_parse"); + my ($ok, $rcpt, @param) = Qpsmtpd::Command->parse("rcpt", $line, $msg); + return $self->respond(501, "syntax error in parameters") + unless ($ok and $rcpt); return $self->respond(503, "Use MAIL before RCPT") unless $self->transaction->sender; - my ($rcpt) = ($_[0] =~ m/to:(.*)/i)[0]; - $rcpt = $_[1] unless $rcpt; + my %param; + foreach (@param) { + my ($k,$v) = split /=/, $_, 2; + $param{lc $k} = $v; + } + # to support addresses without <> we now require a plugin + # hooking "rcpt_pre" to + # return (OK, "<$rcpt>"); + # or anything else parseable by Qpsmtpd::Address ;-> + # this means, a plugin can decide to (pre-)accept + # addresses like <[EMAIL PROTECTED]> or <[EMAIL PROTECTED] > + # by removing the trailing "."/" " from this example... + ($rc, $msg) = $self->run_hooks("rcpt_pre", $rcpt); + if ($rc == OK) { + $rcpt = $msg; + } $self->log(LOGALERT, "to email address : [$rcpt]"); + return $self->respond(501, "could not parse recipient") + unless $rcpt =~ /^<.*>$/; + $rcpt = (Qpsmtpd::Address->parse($rcpt))[0]; - return $self->respond(501, "could not parse recipient") unless $rcpt; + return $self->respond(501, "could not parse recipient") + unless ($rcpt || $rcpt->format eq '<>'); - my ($rc, $msg) = $self->run_hooks("rcpt", $rcpt); + ($rc, $msg) = $self->run_hooks("rcpt", $rcpt, %param); if ($rc == DONE) { return 1; } diff -Nurx.svn ../0.3x/lib/Qpsmtpd/SelectServer.pm ./lib/Qpsmtpd/SelectServer.pm --- ../0.3x/lib/Qpsmtpd/SelectServer.pm 2005-11-20 10:52:18.000000000 +0100 +++ ./lib/Qpsmtpd/SelectServer.pm 2006-03-18 19:30:26.000000000 +0100 @@ -121,8 +121,10 @@ } else { $qp->log(LOGINFO, "dispatching $req"); - defined $qp->dispatch(split / +/, $req) - or $qp->respond(502, "command unrecognized: '$req'"); + $req =~ s/^\s*//; + my ($cmd,$line) = split / +/, $req, 2); + defined $qp->dispatch($cmd, $line) + or $qp->respond(502, "command unrecognized: '$cmd'"); } } delete $ready{$client}; diff -Nurx.svn ../0.3x/lib/Qpsmtpd/TcpServer.pm ./lib/Qpsmtpd/TcpServer.pm --- ../0.3x/lib/Qpsmtpd/TcpServer.pm 2006-01-13 07:28:50.000000000 +0100 +++ ./lib/Qpsmtpd/TcpServer.pm 2006-03-18 14:26:24.000000000 +0100 @@ -63,8 +63,10 @@ $_ =~ s/\r?\n$//s; # advanced chomp $self->log(LOGDEBUG, "dispatching $_"); $self->connection->notes('original_string', $_); - defined $self->dispatch(split / +/, $_) - or $self->respond(502, "command unrecognized: '$_'"); + $_ =~ s/^\s*//; + my ($cmd, $line) = split /\s+/, $_, 2; + defined $self->dispatch($cmd, $line) + or $self->respond(502, "command unrecognized: '$cmd'"); alarm $timeout; } alarm(0); diff -Nurx.svn ../0.3x/plugins/dont_require_angelbrackets ./plugins/dont_require_angelbrackets --- ../0.3x/plugins/dont_require_angelbrackets 1970-01-01 01:00:00.000000000 +0100 +++ ./plugins/dont_require_angelbrackets 2006-03-18 19:33:32.000000000 +0100 @@ -0,0 +1,19 @@ +# +# dont_require_angelbrackets - accept addresses in MAIL FROM:/RCPT TO: +# commands without surrounding <> +# +sub hook_mail_pre { + my ($self,$transaction, $addr) = @_; + unless ($addr =~ /^<.*>$/) { + $addr = "<".$addr.">"; + } + return (OK, $addr); +} + +sub hook_rcpt_pre { + my ($self,$transaction, $addr) = @_; + unless ($addr =~ /^<.*>$/) { + $addr = "<".$addr.">"; + } + return (OK, $addr); +} diff -Nurx.svn ../0.3x/plugins/parse_addr_withhelo ./plugins/parse_addr_withhelo --- ../0.3x/plugins/parse_addr_withhelo 1970-01-01 01:00:00.000000000 +0100 +++ ./plugins/parse_addr_withhelo 2006-03-18 19:01:31.000000000 +0100 @@ -0,0 +1,56 @@ +# parse_addr_withhelo +# +# strict RFC 821 forbids parameters after the +# MAIL FROM:<[EMAIL PROTECTED]> +# and +# RCPT TO:<[EMAIL PROTECTED]> +# +# load this plugin to enforce, else the default EHLO parsing with +# parameters is done. +# + +sub hook_mail_parse { + my $self = shift; + if ($self->qp->connection->hello eq 'helo') { + return (OK, \&_parse); + } + return (DECLINED); +} + +sub hook_rcpt_parse { + my $self = shift; + if ($self->qp->connection->hello eq 'helo') { + return (OK, \&_parse); + } + return (DECLINED); +} + +sub _parse { + my ($self,$cmd,$line) = @_; + $self->log(LOGERROR, "_parse() cmd=[$cmd], line=[$line]"); + if ($cmd eq 'mail') { + return(undef, $line, ()) + unless ($line =~ s/^from://i); + } + else { # cmd eq 'rcpt' ;-> + return(undef, $line, ()) + unless ($line =~ s/^to://i); + } + if ($line =~ s/^\s*(<.*>)//) { + my $addr = $1; + if ($line =~ /^\s*\S+.*$/) { + return (undef, $line, ()); + } + else { + return ($cmd, $addr, ()); + } + } + elsif ($line =~ /[EMAIL PROTECTED]/) { + return (undef, $line, ()); + } + else { + $line =~ s/^\s*//; + $line =~ s/\s*$//; + return ($cmd, $line, ()); + } +} diff -Nurx.svn ../0.3x/t/addresses.t ./t/addresses.t --- ../0.3x/t/addresses.t 2005-11-20 10:52:17.000000000 +0100 +++ ./t/addresses.t 2006-03-18 19:20:08.000000000 +0100 @@ -12,19 +12,26 @@ is(($smtpd->command('MAIL FROM:<ask @perl.org>'))[0], 250, 'MAIL FROM:<ask @perl.org>'); is($smtpd->transaction->sender->address, 'ask @perl.org', 'got the right sender'); -is(($smtpd->command('MAIL FROM:[EMAIL PROTECTED]'))[0], 250, 'MAIL FROM:[EMAIL PROTECTED]'); -is($smtpd->transaction->sender->format, '<[EMAIL PROTECTED]>', 'got the right sender'); +is(($smtpd->command('MAIL FROM:[EMAIL PROTECTED]'))[0], 501, 'MAIL FROM:[EMAIL PROTECTED]'); +# is($smtpd->transaction->sender->format, '<[EMAIL PROTECTED]>', 'got the right sender'); my $command = 'MAIL FROM:<[EMAIL PROTECTED]> SIZE=1230'; is(($smtpd->command($command))[0], 250, $command); is($smtpd->transaction->sender->format, '<[EMAIL PROTECTED]>', 'got the right sender'); +my $command = 'MAIL FROM:<[EMAIL PROTECTED]> SIZE=1230 INCORRECT'; +is(($smtpd->command($command))[0], 503, $command); + $command = 'MAIL FROM:<>'; is(($smtpd->command($command))[0], 250, $command); is($smtpd->transaction->sender->format, '<>', 'got the right sender'); -$command = 'MAIL FROM:<[EMAIL PROTECTED]> SIZE=1230'; +$command = 'MAIL FROM:<[EMAIL PROTECTED]> SIZE=1230 FOO=12345'; is(($smtpd->command($command))[0], 250, $command); is($smtpd->transaction->sender->format, '<[EMAIL PROTECTED]>', 'got the right sender'); - - +$smtpd->connection->relay_client(1); +$command = 'RCPT TO:<[EMAIL PROTECTED]> SIZE=1230 SOMETHING=12345'; +is(($smtpd->command($command))[0], 250, $command); +is(($smtpd->transaction->recipients)[0]->format, '<[EMAIL PROTECTED]>', 'got the right rcpt'); +$command = 'RCPT TO:[EMAIL PROTECTED]'; +is(($smtpd->command($command))[0], 501, $command);