From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 41CD38166C for ; Wed, 24 Nov 2021 10:33:28 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3599B1C4EC for ; Wed, 24 Nov 2021 10:33:28 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 6A9F51C4C6 for ; Wed, 24 Nov 2021 10:33:24 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D3E71447EE; Wed, 24 Nov 2021 10:33:23 +0100 (CET) Message-ID: Date: Wed, 24 Nov 2021 10:33:19 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:95.0) Gecko/20100101 Thunderbird/95.0 Content-Language: en-US To: Stoiko Ivanov , pmg-devel@lists.proxmox.com References: <20211122195032.359071-1-s.ivanov@proxmox.com> From: Dominik Csapak In-Reply-To: <20211122195032.359071-1-s.ivanov@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.961 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -3.515 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pmg-devel] [PATCH pmg-api 1/1] fix #2795: add support for DSN X-BeenThere: pmg-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Mail Gateway development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Nov 2021 09:33:28 -0000 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 ''\ > -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 > --- > 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:
"); > 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:
"); > 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) { >