[pmg-devel] [PATCH pmg-api v2 0/2] fix #2795 - add DSN support

[pmg-devel] [PATCH pmg-api v2 0/2] fix #2795 - add DSN support

[Stoiko Ivanov]

v1->v2:
* incorporated Dominik's feedback - huge thanks!!
* all parameters we don't care about are ignored (as before)
* the options are not transformed into lowercase unconditionally anymore
  (they contain mail-addresses, and mail-ids - which are not case-insensitive)
* split up the patch into 2 commits - since the regexes for the MAIL and
  RCPT commands is more sensitive than I expected:
** read up on allowed characters in local-parts - tried extending the regex
   to parse mailboxes more correctly - failed at that
** decided it's ok to not allow '>' in localparts (we did not do so before)
** did quite a bit more testing thanks to that


Stoiko Ivanov (2):
  partially fix #2795: allow for '>' in smtp parameters
  fix #2795: add support for DSN

 src/PMG/RuleDB/Accept.pm |  2 +-
 src/PMG/RuleDB/BCC.pm    |  6 +++++-
 src/PMG/SMTP.pm          | 27 ++++++++++++++++++++++-----
 src/PMG/Utils.pm         | 19 ++++++++++++++++---
 src/bin/pmg-smtp-filter  |  1 +
 5 files changed, 45 insertions(+), 10 deletions(-)

-- 
2.30.2

[pmg-devel] [PATCH pmg-api v2 1/2] partially fix #2795: allow for '>' in smtp parameters

[Stoiko Ivanov]

The regular expressions parsing the MAIL and RCPT commands do not
cover the case where a esmtp parameter may contain angle brackets
(e.g. the ENVID parameter for the delivery status notification
extension - RFC3464 [0]).

following section 4.1.2 of RFC5321 [1] the regex is changed to:
* consider everything up to the first '>' the mailbox
* consider everything afterwards (if it starts with a ' ') as
  parameters

This is fairly robusts, only not parsing correctly if the local part
contains '>' (as quoted text) - but this did not work before anyways
(and causes problems in other places as well).

tested with:
```
cat test.eml | /usr/sbin/sendmail -N success,delay,failure \
-V '<someid@somehost>' \
-f '"local>part"@test.example' \
discard@test.example
```

[0] https://tools.ietf.org/html/rfc3464
[1] https://tools.ietf.org/html/rfc5321#section-4.1.2

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/SMTP.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PMG/SMTP.pm b/src/PMG/SMTP.pm
index 68c833e..24cce23 100644
--- a/src/PMG/SMTP.pm
+++ b/src/PMG/SMTP.pm
@@ -100,7 +100,7 @@ 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) {
 		delete $self->{to};
 		my ($from, $opts) = ($1, $2);
 		if ($opts =~ m/\sSMTPUTF8/) {
@@ -115,7 +115,7 @@ sub loop {
 		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;
 		push @{$self->{to}} , $to;
 		$self->reply ('250 2.5.0 OK');
-- 
2.30.2

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

[Stoiko Ivanov]

store the esmtp parameters for the MAIL and RCPT command needed to
support Delivery status notifications (DSN - RFC 3464 [0]) and pass
them to the outbound postfix instance (port 10025) used for sending
the mail further (see also [1]).

Postfix does syntax-checking before passing the mail to the proxy
also in before-queue filtering mode.

Since the handling is done by postfix we don't need to generate any
DSN.

We only store a whitelist of parameters, instead of passing all,
because some parameters might not be valid anymore after processing
(e.g. SIZE)

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          | 23 ++++++++++++++++++++---
 src/PMG/Utils.pm         | 19 ++++++++++++++++---
 src/bin/pmg-smtp-filter  |  1 +
 5 files changed, 43 insertions(+), 8 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 24cce23..2b5c944 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 {
@@ -103,9 +104,16 @@ sub loop {
 	    if ($args =~ m/^from:\s*<([^\s\>]*?)>( .*)$/i) {
 		delete $self->{to};
 		my ($from, $opts) = ($1, $2);
-		if ($opts =~ m/\sSMTPUTF8/) {
-		    $self->{smtputf8} = 1;
-		    $from = decode('UTF-8', $from);
+
+		for my $opt (split(' ', $opts)) {
+		    if ($opt =~ /(ret|envid)=([^ =]+)/i ) {
+			$self->{param}->{mail}->{$1} = $2;
+		    } elsif ($opt =~ m/smtputf8/i) {
+			$self->{smtputf8} = 1;
+			$from = decode('UTF-8', $from);
+		    } else {
+			#ignore everything else
+		    }
 		}
 		$self->{from} = $from;
 		$self->reply ('250 2.5.0 OK');
@@ -117,7 +125,16 @@ sub loop {
 	} elsif ($cmd eq 'rcpt') {
 	    if ($args =~ m/^to:\s*<([^\s\>]+?)>( .*)$/i) {
 		my $to = $self->{smtputf8} ? decode('UTF-8', $1) : $1;
+		my $opts = $2;
 		push @{$self->{to}} , $to;
+		my $err = 0;
+		for my $opt (split(' ', $opts)) {
+		    if ($opt =~ /(notify|orcpt)=([^ =]+)/i ) {
+			$self->{param}->{rcpt}->{$to}->{$1} = $2;
+		    } else {
+			#ignore everything else
+		    }
+		}
 		$self->reply ('250 2.5.0 OK');
 		next;
 	    } else {
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) {
-- 
2.30.2

Re: [pmg-devel] [PATCH pmg-api v2 0/2] fix #2795 - add DSN support

[Dominik Csapak]

On 11/25/21 13:03, Stoiko Ivanov wrote:
> v1->v2:
> * incorporated Dominik's feedback - huge thanks!!
> * all parameters we don't care about are ignored (as before)
> * the options are not transformed into lowercase unconditionally anymore
>    (they contain mail-addresses, and mail-ids - which are not case-insensitive)
> * split up the patch into 2 commits - since the regexes for the MAIL and
>    RCPT commands is more sensitive than I expected:
> ** read up on allowed characters in local-parts - tried extending the regex
>     to parse mailboxes more correctly - failed at that
> ** decided it's ok to not allow '>' in localparts (we did not do so before)
> ** did quite a bit more testing thanks to that
> 
> 
> Stoiko Ivanov (2):
>    partially fix #2795: allow for '>' in smtp parameters
>    fix #2795: add support for DSN
> 
>   src/PMG/RuleDB/Accept.pm |  2 +-
>   src/PMG/RuleDB/BCC.pm    |  6 +++++-
>   src/PMG/SMTP.pm          | 27 ++++++++++++++++++++++-----
>   src/PMG/Utils.pm         | 19 ++++++++++++++++---
>   src/bin/pmg-smtp-filter  |  1 +
>   5 files changed, 45 insertions(+), 10 deletions(-)
> 

sadly this series breaks it with after-queue:

the mail cannot be deliviered (queued/bounced)
error message from the bounce:

<mail> :host 127.0.0.1[127.0.0.1] said: 501 5.5.2 Syntax: MAIL
     FROM: <address> (in reply to MAIL FROM command)

Re: [pmg-devel] [PATCH pmg-api v2 0/2] fix #2795 - add DSN support

[Dominik Csapak]

LGTM

Tested with after/before queue with and withoud dsn
(got a dsn reply everytime i expected it)

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

On 11/25/21 13:03, Stoiko Ivanov wrote:
> v1->v2:
> * incorporated Dominik's feedback - huge thanks!!
> * all parameters we don't care about are ignored (as before)
> * the options are not transformed into lowercase unconditionally anymore
>    (they contain mail-addresses, and mail-ids - which are not case-insensitive)
> * split up the patch into 2 commits - since the regexes for the MAIL and
>    RCPT commands is more sensitive than I expected:
> ** read up on allowed characters in local-parts - tried extending the regex
>     to parse mailboxes more correctly - failed at that
> ** decided it's ok to not allow '>' in localparts (we did not do so before)
> ** did quite a bit more testing thanks to that
> 
> 
> Stoiko Ivanov (2):
>    partially fix #2795: allow for '>' in smtp parameters
>    fix #2795: add support for DSN
> 
>   src/PMG/RuleDB/Accept.pm |  2 +-
>   src/PMG/RuleDB/BCC.pm    |  6 +++++-
>   src/PMG/SMTP.pm          | 27 ++++++++++++++++++++++-----
>   src/PMG/Utils.pm         | 19 ++++++++++++++++---
>   src/bin/pmg-smtp-filter  |  1 +
>   5 files changed, 45 insertions(+), 10 deletions(-)
> 

Re: [pmg-devel] [PATCH pmg-api v2 0/2] fix #2795 - add DSN support

[Dominik Csapak]

argh sorry should be going to v3 not v2... wrong mail
clicked in thunderbird^^

Re: [pmg-devel] [PATCH pmg-api v2 0/2] fix #2795 - add DSN support

[Stoiko Ivanov]

On Fri, 26 Nov 2021 11:40:34 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> argh sorry should be going to v3 not v2... wrong mail
> clicked in thunderbird^^

and sorry from my side for replying to this mail
the series I applied was the v3!

[pmg-devel] applied-series: [PATCH pmg-api v2 0/2] fix #2795 - add DSN support

[Stoiko Ivanov]

On Fri, 26 Nov 2021 11:10:45 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> LGTM
> 
> Tested with after/before queue with and withoud dsn
> (got a dsn reply everytime i expected it)
> 
> Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
> Tested-by: Dominik Csapak <d.csapak@proxmox.com>

Huge thanks again - applied with added R-b and T-b

> 
> On 11/25/21 13:03, Stoiko Ivanov wrote:
> > v1->v2:
> > * incorporated Dominik's feedback - huge thanks!!
> > * all parameters we don't care about are ignored (as before)
> > * the options are not transformed into lowercase unconditionally anymore
> >    (they contain mail-addresses, and mail-ids - which are not case-insensitive)
> > * split up the patch into 2 commits - since the regexes for the MAIL and
> >    RCPT commands is more sensitive than I expected:
> > ** read up on allowed characters in local-parts - tried extending the regex
> >     to parse mailboxes more correctly - failed at that
> > ** decided it's ok to not allow '>' in localparts (we did not do so before)
> > ** did quite a bit more testing thanks to that
> > 
> > 
> > Stoiko Ivanov (2):
> >    partially fix #2795: allow for '>' in smtp parameters
> >    fix #2795: add support for DSN
> > 
> >   src/PMG/RuleDB/Accept.pm |  2 +-
> >   src/PMG/RuleDB/BCC.pm    |  6 +++++-
> >   src/PMG/SMTP.pm          | 27 ++++++++++++++++++++++-----
> >   src/PMG/Utils.pm         | 19 ++++++++++++++++---
> >   src/bin/pmg-smtp-filter  |  1 +
> >   5 files changed, 45 insertions(+), 10 deletions(-)
> >   
> 
>