Re: [pmg-devel] [PATCH pmg-api 1/1] fix #2795: add support for DSN

[Dominik Csapak <d.csapak@proxmox.com>]

works generally as advertised, i have some (minor) remarks
though (comments inline)

On 11/22/21 20:50, Stoiko Ivanov wrote:
> Delivery status notifications (DSN) are defined in RFC 3464 [0].
> They introduce additional key-value parameters for the MAIL and RCPT
> commands.
> 
> The default config we ship offers DSN on the internal port (but not on
> the external port) - which works fine in after-queue filtering mode,
> but not in before-queue filtering mode.
> 
> This patchset adds support for collecting the relevant parameters and
> passing them on to the postfix instance [1], which actually sends out the
> mail.
> 
> additionally postfix does syntax-checking before passing the mail to
> the proxy (before queue filtering).
> 
> Since the handling is done by postfix we don't need to generate the
> DSN for any of the cases.
> 
> tested with various combinations of the -V, -N and -R parameters to
> sendmail (e.g.):
> ```
> /usr/sbin/sendmail -N success,delay,failure \
> -V '<xxxxxxxx@test.proxmox.com>'\
> -R hdrs test@test.domain.example
> ```
> 
> some tests with invalid combinations were also done with netcat.
> 
> [0] https://tools.ietf.org/html/rfc3464
> [1] http://www.postfix.org/DSN_README.html
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>   src/PMG/RuleDB/Accept.pm |  2 +-
>   src/PMG/RuleDB/BCC.pm    |  6 +++++-
>   src/PMG/SMTP.pm          | 46 +++++++++++++++++++++++++++++++++-------
>   src/PMG/Utils.pm         | 19 ++++++++++++++---
>   src/bin/pmg-smtp-filter  |  1 +
>   5 files changed, 61 insertions(+), 13 deletions(-)
> 
> diff --git a/src/PMG/RuleDB/Accept.pm b/src/PMG/RuleDB/Accept.pm
> index 0bcf250..cd67ea2 100644
> --- a/src/PMG/RuleDB/Accept.pm
> +++ b/src/PMG/RuleDB/Accept.pm
> @@ -122,7 +122,7 @@ sub execute {
>   	} else {
>   	    my ($qid, $code, $mess) = PMG::Utils::reinject_mail(
>   		$entity, $msginfo->{sender}, $tg,
> -		$msginfo->{xforward}, $msginfo->{fqdn});
> +		$msginfo->{xforward}, $msginfo->{fqdn}, $msginfo->{param});
>   	    if ($qid) {
>   		foreach (@$tg) {
>   		    syslog('info', "%s: accept mail to <%s> (%s) (rule: %s)", $queue->{logid}, encode('UTF-8', $_), $qid, $rulename);
> diff --git a/src/PMG/RuleDB/BCC.pm b/src/PMG/RuleDB/BCC.pm
> index a8db3f5..d364690 100644
> --- a/src/PMG/RuleDB/BCC.pm
> +++ b/src/PMG/RuleDB/BCC.pm
> @@ -156,9 +156,13 @@ sub execute {
>   	    $entity->print ($fh);
>   	    print $fh "bcc end\n";
>   	} else {
> +	    my $param = {};
> +	    for my $bcc (@bcc_targets) {
> +		$param->{rcpt}->{$bcc}->{notify} = "never";
> +	    }
>   	    my $qid = PMG::Utils::reinject_mail(
>   		$entity, $msginfo->{sender}, \@bcc_targets,
> -		$msginfo->{xforward}, $msginfo->{fqdn}, 1);
> +		$msginfo->{xforward}, $msginfo->{fqdn}, $param);
>   	    foreach (@bcc_targets) {
>   		if ($qid) {
>   		    syslog('info', "%s: bcc to <%s> (rule: %s, %s)", $queue->{logid}, $_, $rulename, $qid);
> diff --git a/src/PMG/SMTP.pm b/src/PMG/SMTP.pm
> index 68c833e..c7aba01 100644
> --- a/src/PMG/SMTP.pm
> +++ b/src/PMG/SMTP.pm
> @@ -38,6 +38,7 @@ sub reset {
>       delete $self->{smtputf8};
>       delete $self->{xforward};
>       delete $self->{status};
> +    delete $self->{param};
>   }
>   
>   sub abort {
> @@ -100,25 +101,54 @@ sub loop {
>   	    $self->reply ("250 2.5.0 OK");
>   	    next;
>   	} elsif ($cmd eq 'mail') {
> -	    if ($args =~ m/^from:\s*<([^\s\>]*)>([^>]*)$/i) {
> +	    if ($args =~ m/^from:\s*<([^\s\>]*?)>(.*)$/i) {

this change looks like it's unrelated on the first glance
i understand this is because options can contain '>'
(i mean local parts of mails can too, but this'll break other
parts already...)

a small notice in the commit message (or a comment) would be good though

could also be it's own patch (smthg like 'fix smtp option parsing' ?)


>   		delete $self->{to};
> -		my ($from, $opts) = ($1, $2);
> -		if ($opts =~ m/\sSMTPUTF8/) {
> -		    $self->{smtputf8} = 1;
> -		    $from = decode('UTF-8', $from);
> +		my ($from, $opts) = ($1, lc($2));
> +
> +		my $err = 0;
> +		for my $opt (split(' ', $opts)) {
> +		    if ($opt =~ /(ret|envid)=([^ =]+)/ ) {
> +			$self->{param}->{mail}->{$1} = $2;
> +		    } elsif ($opt =~ m/smtputf8/) {
> +			$self->{smtputf8} = 1;
> +			$from = decode('UTF-8', $from);
> +		    } elsif ($opt =~ m/(?:size=(:?\d+)|body=8bitmime)/) {
> +			#skip
> +		    } else {
> +			$err = 1;
> +			last;
> +		    }

as talked off-list, i am unsure if rejecting 'unknown' parameters is
the best way. before, we simply ignore them, so if a mail-server already
sent invalid/unknown paramters it will now be blocked which is not
that good imho. i think we should still ignore/drop them or pass them
along.

>   		}
>   		$self->{from} = $from;
> -		$self->reply ('250 2.5.0 OK');
> +		if ($err) {
> +		    $self->reply ("501 5.5.2 Syntax: MAIL FROM: unknown parameter");
> +		} else {
> +		    $self->reply ('250 2.5.0 OK');
> +		}
>   		next;
>   	    } else {
>   		$self->reply ("501 5.5.2 Syntax: MAIL FROM: <address>");
>   		next;
>   	    }
>   	} elsif ($cmd eq 'rcpt') {
> -	    if ($args =~ m/^to:\s*<([^\s\>]+)>[^>]*$/i) {
> +	    if ($args =~ m/^to:\s*<([^\s\>]+?)>(.*)$/i) {
>   		my $to = $self->{smtputf8} ? decode('UTF-8', $1) : $1;
> +		my $opts = lc($2);
>   		push @{$self->{to}} , $to;
> -		$self->reply ('250 2.5.0 OK');
> +		my $err = 0;
> +		for my $opt (split(' ', $opts)) {
> +		    if ($opt =~ /(notify|orcpt)=([^ =]+)/i ) {
> +			$self->{param}->{rcpt}->{$to}->{$1} = $2;
> +		    } else {
> +			$err = 1;
> +			last;
> +		    }
> +		}
> +		if ($err) {
> +		    $self->reply ("501 5.5.2 Syntax: RCPT TO: unknown parameter");
> +		} else {
> +		    $self->reply ('250 2.5.0 OK');
> +		}
>   		next;
>   	    } else {
>   		$self->reply ("501 5.5.2 Syntax: RCPT TO: <address>");
> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index 92c3a7a..4eebfa5 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -203,7 +203,7 @@ sub subst_values {
>   }
>   
>   sub reinject_mail {
> -    my ($entity, $sender, $targets, $xforward, $me, $nodsn) = @_;
> +    my ($entity, $sender, $targets, $xforward, $me, $params) = @_;
>   
>       my $smtp;
>       my $resid;
> @@ -244,15 +244,28 @@ sub reinject_mail {
>   	    $mail_opts .= " SMTPUTF8" if $has_utf8_targets;
>   	}
>   
> +	if (defined($params->{mail})) {
> +	    my $mailparams = $params->{mail};
> +	    for my $p (keys %$mailparams) {
> +		$mail_opts .= " $p=$mailparams->{$p}";
> +	    }
> +	}
> + >   	if (!$smtp->_MAIL("FROM:" . $sender_addr . $mail_opts)) {
>   	    syslog('err', "smtp error - got: %s %s", $smtp->code, scalar ($smtp->message));
>   	    die "smtp from: ERROR";
>   	}
>   
> -	my $rcpt_opts = $nodsn ? " NOTIFY=NEVER" : "";
> -
>   	foreach my $target (@$targets) {
>   	    my $rcpt_addr;
> +	    my $rcpt_opts = '';
> +	    if (defined($params->{rcpt}->{$target})) {
> +		my $rcptparams = $params->{rcpt}->{$target};
> +		for my $p (keys %$rcptparams) {
> +		    $rcpt_opts .= " $p=$rcptparams->{$p}";
> +		}
> +	    }
> +
>   	    if (utf8::is_utf8($target)) {
>   		$rcpt_addr = encode('UTF-8', $smtp->_addr($target));
>   	    } else {
> diff --git a/src/bin/pmg-smtp-filter b/src/bin/pmg-smtp-filter
> index b070c8e..45eb125 100755
> --- a/src/bin/pmg-smtp-filter
> +++ b/src/bin/pmg-smtp-filter
> @@ -640,6 +640,7 @@ sub handle_smtp {
>   	$msginfo->{sender} = $smtp->{from};
>   	$msginfo->{xforward} = $smtp->{xforward};
>   	$msginfo->{targets} = $smtp->{to};
> +	$msginfo->{param} = $smtp->{param};
>   
>   	my $dkim_sign = $msginfo->{trusted} && $pmg_cfg->get('admin', 'dkim_sign');
>   	if ($dkim_sign) {
>