[pmg-devel] [PATCH pmg-api 0/2] fix smtputf8 handling if disabled in postfix

[pmg-devel] [PATCH pmg-api 0/2] fix smtputf8 handling if disabled in postfix

[Stoiko Ivanov]

This series addresses an issue some of our users reported in our
community forum with pmg-api_7.2-3:
* the decision if smtputf8 is needed is based on the envelope-addresses
  and the headers (if they contain non-ascii characters we use SMTPUTF8
* this breaks environments where SMTPUTF8 is disabled (mostly because
  some downstream servers do not support this), but mails still contain
  non-ascii data (while this is against the relevant rfc, which say that
  header-data must contain only ascii characters (and should be encoded
  with MIME-words otherwise), it is seemingly quite common in the wild)

one testmail I got from a user from the forum had the From header
correctly encoded, but added an X-DFrom header with the unencoded from.

The first patch simply drops the header-inspection and should enable
most of the reporters to receive mail again.

The second patch tries to address the smtputf8 issue a bit differently
than what we currently do
- For mails received via SMTP it simply sets SMTPUTF8 if the original
  postfix processes sent pmg-smtp-filter the mail with the flag, and
  does not set it otherwise
- For locally generated mail it detects if its needed by checking the
  envelope-addresses and the headers for non-ascii characters.

This should follow postfix own functioning quite closely:
https://www.postfix.org/SMTPUTF8_README.html

processing by pmg-smtp-filter should not change the need for the flag,
since we don't rewrite envelope-addresses, and modify filed does
mime-encode the resulting header.

Sending as two patches, since the first one would be good to get out
soon (as it's affecting a few setups), while the second one might
benefit from a bit more testing (I did some tests, which all looked
good, but might have overlooked some cases)

Stoiko Ivanov (2):
  utils: skip checking headers for non-ascii characters
  smtputf8: keep smtputf8 from incoming postfix, detect for local mail

 src/PMG/API2/Quarantine.pm |  7 ++++++-
 src/PMG/RuleDB/Notify.pm   |  6 +++++-
 src/PMG/SMTP.pm            |  7 ++++++-
 src/PMG/Utils.pm           | 12 +++++++++---
 4 files changed, 26 insertions(+), 6 deletions(-)

-- 
2.30.2

[pmg-devel] [PATCH pmg-api 1/2] utils: skip checking headers for non-ascii characters

[Stoiko Ivanov]

the fix for smtputf8 enablement in
191e4709e99a5975fa66cf21847887566e108501 was a bit too eager and broke
the mailflow of a few users, who have smtputf8 disabled in their
postfix config (because their downstream servers do not support this):

The issue here is that the mails they process have had
(non-rfc-compliant) non-ascii header contents, which used to work
before the patch. The postfix smtputf8 howto explains quite well that
postfix never cared too much about headers (or local-parts of
addresses) before smtputf8 [1]
```
Postfix already permitted UTF-8 in message header values and in
address localparts. This does not change.
```

While the patch only ignores the headers and still could cause issues
with non-ascii local-parts, those should occur far less frequently in
the wild (none of the reporters in our forum had this).

tested with a mail with an Euro sign in a 'X-From:' header.

[0] https://forum.proxmox.com/threads/.120886/
[1] https://www.postfix.org/SMTPUTF8_README.html#enabling

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

diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
index 825b8d9..9c6f841 100644
--- a/src/PMG/Utils.pm
+++ b/src/PMG/Utils.pm
@@ -232,10 +232,6 @@ sub mail_needs_smtputf8 {
 	}
     }
 
-    if ($entity->head()->as_string() =~ /([^\p{PosixPrint}\n\r\t])/) {
-	return 1;
-    }
-
     return 0;
 }
 
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail

[Stoiko Ivanov]

This patch changes the detection if smtputf8 is needed as option to
the 'MAIL' command:
* for mail passing arriving through postfix it is only added if the
  mail originally was received with it (Accept and BCC actions)
* for locally generated mail (Notify, reports, quarantine-link and
  ndrs) it is decided based on utf8 characters in the mail-addresses or
  headers

This should approximate postfix own behavior in those cases quite
closely:
https://www.postfix.org/SMTPUTF8_README.html#using

Notable difference is that we check the complete e-mail address and
not only the domain part, but I assume non-ascii local-parts to be a
very fringe edge-case in environments where smtputf8 is not supported.
If this occurs in the wild we would also need to adapt the
unconditional encoding of the envelope addresses in reinject_mail

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/API2/Quarantine.pm |  7 ++++++-
 src/PMG/RuleDB/Notify.pm   |  6 +++++-
 src/PMG/SMTP.pm            |  7 ++++++-
 src/PMG/Utils.pm           | 16 +++++++++++++---
 4 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
index fbb302a..352f6b6 100644
--- a/src/PMG/API2/Quarantine.pm
+++ b/src/PMG/API2/Quarantine.pm
@@ -1239,7 +1239,12 @@ my sub send_link_mail {
     );
 
     # we use an empty envelope sender (we don't want to receive NDRs)
-    PMG::Utils::reinject_mail ($mail, '', [$receiver], undef, $fqdn);
+
+    my $params;
+    if (PMG::Utils::mail_needs_smtputf8($mail, '', [$receiver])) {
+	$params->{mail}->{smtputf8} = 1;
+    }
+    PMG::Utils::reinject_mail ($mail, '', [$receiver], undef, $fqdn, $params);
 }
 
 __PACKAGE__->register_method ({
diff --git a/src/PMG/RuleDB/Notify.pm b/src/PMG/RuleDB/Notify.pm
index 68f9b4e..7887195 100644
--- a/src/PMG/RuleDB/Notify.pm
+++ b/src/PMG/RuleDB/Notify.pm
@@ -256,8 +256,12 @@ sub execute {
 	print $fh "notify end\n";
     } else {
 	my @targets = split(/\s*,\s*/, $to);
+	my $params;
+	if (PMG::Utils::mail_needs_smtputf8($top, $from, \@targets)) {
+	    $params->{mail}->{smtputf8} = 1;
+	}
 	my $qid = PMG::Utils::reinject_mail(
-	    $top, $from, \@targets, undef, $msginfo->{fqdn});
+	    $top, $from, \@targets, undef, $msginfo->{fqdn}, $params);
 	foreach (@targets) {
 	    my $target = encode('UTF-8', $_);
 	    if ($qid) {
diff --git a/src/PMG/SMTP.pm b/src/PMG/SMTP.pm
index fbf5c95..35ed9c4 100644
--- a/src/PMG/SMTP.pm
+++ b/src/PMG/SMTP.pm
@@ -111,6 +111,7 @@ sub loop {
 			$self->{param}->{mail}->{$1} = $2;
 		    } elsif ($opt =~ m/smtputf8/i) {
 			$self->{smtputf8} = 1;
+			$self->{param}->{mail}->{smtputf8} = 1;
 			$from = decode('UTF-8', $from);
 		    } else {
 			#ignore everything else
@@ -314,7 +315,11 @@ EOF
 	Encoding => '7bit',
 	Description => 'Delivery report');
 
-    my $qid = PMG::Utils::reinject_mail($ndr, '', [$sender], undef, $hostname);
+    my $params;
+    if (PMG::Utils::mail_needs_smtputf8($ndr, '', [$sender])) {
+	$params->{mail}->{smtputf8} = 1;
+    }
+    my $qid = PMG::Utils::reinject_mail($ndr, '', [$sender], undef, $hostname, $params);
     if ($qid) {
 	syslog('info', "sent NDR for rejecting recipients - $qid");
     } else {
diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
index 9c6f841..1ccd7d2 100644
--- a/src/PMG/Utils.pm
+++ b/src/PMG/Utils.pm
@@ -232,6 +232,10 @@ sub mail_needs_smtputf8 {
 	}
     }
 
+    if ($entity->head()->as_string() =~ /([^\p{PosixPrint}\n\r\t])/) {
+	return 1;
+    }
+
     return 0;
 }
 
@@ -260,10 +264,12 @@ sub reinject_mail {
 	}
 
 	my $mail_opts = " BODY=8BITMIME";
-	$mail_opts .= " SMTPUTF8" if mail_needs_smtputf8($entity, $sender, $targets);
 	my $sender_addr = encode('UTF-8', $smtp->_addr($sender));
-
 	if (defined($params->{mail})) {
+	    if (delete $params->{mail}->{smtputf8}) {
+		$mail_opts .= " SMTPUTF8";
+	    }
+
 	    my $mailparams = $params->{mail};
 	    for my $p (keys %$mailparams) {
 		$mail_opts .= " $p=$mailparams->{$p}";
@@ -1258,7 +1264,11 @@ sub finalize_report {
 	return;
     }
     # we use an empty envelope sender (we don't want to receive NDRs)
-    PMG::Utils::reinject_mail ($top, '', [$receiver], undef, $data->{fqdn});
+    my $params;
+    if (PMG::Utils::mail_needs_smtputf8($top, '', [$receiver])) {
+	$params->{mail}->{smtputf8} = 1;
+    }
+    PMG::Utils::reinject_mail ($top, '', [$receiver], undef, $data->{fqdn}, $params);
 }
 
 sub lookup_timespan {
-- 
2.30.2

Re: [pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail

[Thomas Lamprecht]

Am 23/01/2023 um 16:55 schrieb Stoiko Ivanov:
> This patch changes the detection if smtputf8 is needed as option to
> the 'MAIL' command:
> * for mail passing arriving through postfix it is only added if the
>   mail originally was received with it (Accept and BCC actions)
> * for locally generated mail (Notify, reports, quarantine-link and
>   ndrs) it is decided based on utf8 characters in the mail-addresses or
>   headers
> 
> This should approximate postfix own behavior in those cases quite
> closely:
> https://www.postfix.org/SMTPUTF8_README.html#using
> 
> Notable difference is that we check the complete e-mail address and
> not only the domain part, but I assume non-ascii local-parts to be a
> very fringe edge-case in environments where smtputf8 is not supported.
> If this occurs in the wild we would also need to adapt the
> unconditional encoding of the envelope addresses in reinject_mail
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/API2/Quarantine.pm |  7 ++++++-
>  src/PMG/RuleDB/Notify.pm   |  6 +++++-
>  src/PMG/SMTP.pm            |  7 ++++++-
>  src/PMG/Utils.pm           | 16 +++++++++++++---
>  4 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
> index fbb302a..352f6b6 100644
> --- a/src/PMG/API2/Quarantine.pm
> +++ b/src/PMG/API2/Quarantine.pm
> @@ -1239,7 +1239,12 @@ my sub send_link_mail {
>      );
>  
>      # we use an empty envelope sender (we don't want to receive NDRs)

forgot to keep comment near method call?

> -    PMG::Utils::reinject_mail ($mail, '', [$receiver], undef, $fqdn);
> +
> +    my $params;
> +    if (PMG::Utils::mail_needs_smtputf8($mail, '', [$receiver])) {
> +	$params->{mail}->{smtputf8} = 1;
> +    }

I'd rather move this into reinject mail instead of copyi-pastaing the same
code hunk five times around, after all it has all the info required to
call mail_needs_smtputf8 there. FWICT, its done on all call sites, so you
wouldn't even require to add an opt-out param.

> +    PMG::Utils::reinject_mail ($mail, '', [$receiver], undef, $fqdn, $params);
>  }
>  
>  __PACKAGE__->register_method ({


> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index 9c6f841..1ccd7d2 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -232,6 +232,10 @@ sub mail_needs_smtputf8 {
>  	}
>      }
>  
> +    if ($entity->head()->as_string() =~ /([^\p{PosixPrint}\n\r\t])/) {
> +	return 1;
> +    }


you're reintroducing the hunk you removed in patch 1/2 without really adding any
explicit reasoning, or is 1/2 just intended as uncontroversial stop gap to apply
while 2/2 is still being checked more closely, or what's the deal here?

 

Re: [pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail

[Stoiko Ivanov]

Thanks for the review!

On Wed, 25 Jan 2023 10:30:09 +0100
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

> Am 23/01/2023 um 16:55 schrieb Stoiko Ivanov:
> > This patch changes the detection if smtputf8 is needed as option to
> > the 'MAIL' command:
> > * for mail passing arriving through postfix it is only added if the
> >   mail originally was received with it (Accept and BCC actions)
> > * for locally generated mail (Notify, reports, quarantine-link and
> >   ndrs) it is decided based on utf8 characters in the mail-addresses or
> >   headers
> > 
> > This should approximate postfix own behavior in those cases quite
> > closely:
> > https://www.postfix.org/SMTPUTF8_README.html#using
> > 
> > Notable difference is that we check the complete e-mail address and
> > not only the domain part, but I assume non-ascii local-parts to be a
> > very fringe edge-case in environments where smtputf8 is not supported.
> > If this occurs in the wild we would also need to adapt the
> > unconditional encoding of the envelope addresses in reinject_mail
> > 
> > Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> > ---
> >  src/PMG/API2/Quarantine.pm |  7 ++++++-
> >  src/PMG/RuleDB/Notify.pm   |  6 +++++-
> >  src/PMG/SMTP.pm            |  7 ++++++-
> >  src/PMG/Utils.pm           | 16 +++++++++++++---
> >  4 files changed, 30 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
> > index fbb302a..352f6b6 100644
> > --- a/src/PMG/API2/Quarantine.pm
> > +++ b/src/PMG/API2/Quarantine.pm
> > @@ -1239,7 +1239,12 @@ my sub send_link_mail {
> >      );
> >  
> >      # we use an empty envelope sender (we don't want to receive NDRs)
> 
> forgot to keep comment near method call?
was actually by choice - since the empty envelope sender is used in the
mail_needs_smtputf8 call as well - but can gladly move it to before the
reinject_mail call as well

> 
> > -    PMG::Utils::reinject_mail ($mail, '', [$receiver], undef, $fqdn);
> > +
> > +    my $params;
> > +    if (PMG::Utils::mail_needs_smtputf8($mail, '', [$receiver])) {
> > +	$params->{mail}->{smtputf8} = 1;
> > +    }
> 
> I'd rather move this into reinject mail instead of copyi-pastaing the same
> code hunk five times around, after all it has all the info required to
> call mail_needs_smtputf8 there. FWICT, its done on all call sites, so you
> wouldn't even require to add an opt-out param.
The call-sites it's not added are the ones in the rulesystem -
(PMG::RuleDB::Accept/BCC) - where the mail is received from the outside
and where we don't want to autodetect the need, but simply reuse what
postfix sends us.
(maybe I could have written that a bit more explicitly in the
commit-message)

> 
> > +    PMG::Utils::reinject_mail ($mail, '', [$receiver], undef, $fqdn, $params);
> >  }
> >  
> >  __PACKAGE__->register_method ({
> 
> 
> > diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> > index 9c6f841..1ccd7d2 100644
> > --- a/src/PMG/Utils.pm
> > +++ b/src/PMG/Utils.pm
> > @@ -232,6 +232,10 @@ sub mail_needs_smtputf8 {
> >  	}
> >      }
> >  
> > +    if ($entity->head()->as_string() =~ /([^\p{PosixPrint}\n\r\t])/) {
> > +	return 1;
> > +    }
> 
> 
> you're reintroducing the hunk you removed in patch 1/2 without really adding any
> explicit reasoning, or is 1/2 just intended as uncontroversial stop gap to apply
> while 2/2 is still being checked more closely, or what's the deal here?
The idea was to apply 1/2 (as stop-gap measure) quite soon and get it out -
so that most users with disabled smtputf8 and non-ascii characters in
received mail get their systems working again, while 2/2 was something
that might benefit from a more through review.
I'll try to rewrite the commit message to reference 1/2 (or it's commit
hash once applied) explicitly

[pmg-devel] applied: [PATCH pmg-api 1/2] utils: skip checking headers for non-ascii characters

[Thomas Lamprecht]

Am 23/01/2023 um 16:55 schrieb Stoiko Ivanov:
> the fix for smtputf8 enablement in
> 191e4709e99a5975fa66cf21847887566e108501 was a bit too eager and broke
> the mailflow of a few users, who have smtputf8 disabled in their
> postfix config (because their downstream servers do not support this):
> 
> The issue here is that the mails they process have had
> (non-rfc-compliant) non-ascii header contents, which used to work
> before the patch. The postfix smtputf8 howto explains quite well that
> postfix never cared too much about headers (or local-parts of
> addresses) before smtputf8 [1]
> ```
> Postfix already permitted UTF-8 in message header values and in
> address localparts. This does not change.
> ```
> 
> While the patch only ignores the headers and still could cause issues
> with non-ascii local-parts, those should occur far less frequently in
> the wild (none of the reporters in our forum had this).
> 
> tested with a mail with an Euro sign in a 'X-From:' header.
> 
> [0] https://forum.proxmox.com/threads/.120886/
> [1] https://www.postfix.org/SMTPUTF8_README.html#enabling
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/Utils.pm | 4 ----
>  1 file changed, 4 deletions(-)
> 
>

applied, with slightly touched up commit message, mostly to denote that
this is a stop gap, thanks!

Re: [pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail

[Thomas Lamprecht]

Am 25/01/2023 um 10:48 schrieb Stoiko Ivanov:
> On Wed, 25 Jan 2023 10:30:09 +0100
> Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
>>
>>> -    PMG::Utils::reinject_mail ($mail, '', [$receiver], undef, $fqdn);
>>> +
>>> +    my $params;
>>> +    if (PMG::Utils::mail_needs_smtputf8($mail, '', [$receiver])) {
>>> +	$params->{mail}->{smtputf8} = 1;
>>> +    }
>>
>> I'd rather move this into reinject mail instead of copyi-pastaing the same
>> code hunk five times around, after all it has all the info required to
>> call mail_needs_smtputf8 there. FWICT, its done on all call sites, so you
>> wouldn't even require to add an opt-out param.
>
> The call-sites it's not added are the ones in the rulesystem -
> (PMG::RuleDB::Accept/BCC) - where the mail is received from the outside
> and where we don't want to autodetect the need, but simply reuse what
> postfix sends us.
> (maybe I could have written that a bit more explicitly in the
> commit-message)
>

then either:

create a wrapper method that adds that param handling and switch the call
sites here over to that or move it still into reinject_mail but allow for
opt-ing out. I'd favor the first varian, as that then also allows for some
clear naming distinction for which flow (direction) which method should be
called.

 
>>
>>> +    PMG::Utils::reinject_mail ($mail, '', [$receiver], undef, $fqdn, $params);
>>>  }
>>>  
>>>  __PACKAGE__->register_method ({
>>
>>
>>> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
>>> index 9c6f841..1ccd7d2 100644
>>> --- a/src/PMG/Utils.pm
>>> +++ b/src/PMG/Utils.pm
>>> @@ -232,6 +232,10 @@ sub mail_needs_smtputf8 {
>>>  	}
>>>      }
>>>  
>>> +    if ($entity->head()->as_string() =~ /([^\p{PosixPrint}\n\r\t])/) {
>>> +	return 1;
>>> +    }
>>
>>
>> you're reintroducing the hunk you removed in patch 1/2 without really adding any
>> explicit reasoning, or is 1/2 just intended as uncontroversial stop gap to apply
>> while 2/2 is still being checked more closely, or what's the deal here?
>
> The idea was to apply 1/2 (as stop-gap measure) quite soon and get it out -
> so that most users with disabled smtputf8 and non-ascii characters in
> received mail get their systems working again, while 2/2 was something
> that might benefit from a more through review.
> I'll try to rewrite the commit message to reference 1/2 (or it's commit
> hash once applied) explicitly
> 

Please mention that explicitly the next time.