* [pmg-devel] [PATCH pmg-api 0/2] fix smtputf8 handling if disabled in postfix @ 2023-01-23 15:55 Stoiko Ivanov 2023-01-23 15:55 ` [pmg-devel] [PATCH pmg-api 1/2] utils: skip checking headers for non-ascii characters Stoiko Ivanov 2023-01-23 15:55 ` [pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail Stoiko Ivanov 0 siblings, 2 replies; 7+ messages in thread From: Stoiko Ivanov @ 2023-01-23 15:55 UTC (permalink / raw) To: pmg-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pmg-devel] [PATCH pmg-api 1/2] utils: skip checking headers for non-ascii characters 2023-01-23 15:55 [pmg-devel] [PATCH pmg-api 0/2] fix smtputf8 handling if disabled in postfix Stoiko Ivanov @ 2023-01-23 15:55 ` Stoiko Ivanov 2023-01-25 10:04 ` [pmg-devel] applied: " Thomas Lamprecht 2023-01-23 15:55 ` [pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail Stoiko Ivanov 1 sibling, 1 reply; 7+ messages in thread From: Stoiko Ivanov @ 2023-01-23 15:55 UTC (permalink / raw) To: pmg-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pmg-devel] applied: [PATCH pmg-api 1/2] utils: skip checking headers for non-ascii characters 2023-01-23 15:55 ` [pmg-devel] [PATCH pmg-api 1/2] utils: skip checking headers for non-ascii characters Stoiko Ivanov @ 2023-01-25 10:04 ` Thomas Lamprecht 0 siblings, 0 replies; 7+ messages in thread From: Thomas Lamprecht @ 2023-01-25 10:04 UTC (permalink / raw) To: Stoiko Ivanov, pmg-devel 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! ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail 2023-01-23 15:55 [pmg-devel] [PATCH pmg-api 0/2] fix smtputf8 handling if disabled in postfix Stoiko Ivanov 2023-01-23 15:55 ` [pmg-devel] [PATCH pmg-api 1/2] utils: skip checking headers for non-ascii characters Stoiko Ivanov @ 2023-01-23 15:55 ` Stoiko Ivanov 2023-01-25 9:30 ` Thomas Lamprecht 1 sibling, 1 reply; 7+ messages in thread From: Stoiko Ivanov @ 2023-01-23 15:55 UTC (permalink / raw) To: pmg-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail 2023-01-23 15:55 ` [pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail Stoiko Ivanov @ 2023-01-25 9:30 ` Thomas Lamprecht 2023-01-25 9:48 ` Stoiko Ivanov 0 siblings, 1 reply; 7+ messages in thread From: Thomas Lamprecht @ 2023-01-25 9:30 UTC (permalink / raw) To: Stoiko Ivanov, pmg-devel 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail 2023-01-25 9:30 ` Thomas Lamprecht @ 2023-01-25 9:48 ` Stoiko Ivanov 2023-01-25 10:04 ` Thomas Lamprecht 0 siblings, 1 reply; 7+ messages in thread From: Stoiko Ivanov @ 2023-01-25 9:48 UTC (permalink / raw) To: Thomas Lamprecht; +Cc: pmg-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail 2023-01-25 9:48 ` Stoiko Ivanov @ 2023-01-25 10:04 ` Thomas Lamprecht 0 siblings, 0 replies; 7+ messages in thread From: Thomas Lamprecht @ 2023-01-25 10:04 UTC (permalink / raw) To: Stoiko Ivanov; +Cc: pmg-devel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-01-25 10:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-23 15:55 [pmg-devel] [PATCH pmg-api 0/2] fix smtputf8 handling if disabled in postfix Stoiko Ivanov 2023-01-23 15:55 ` [pmg-devel] [PATCH pmg-api 1/2] utils: skip checking headers for non-ascii characters Stoiko Ivanov 2023-01-25 10:04 ` [pmg-devel] applied: " Thomas Lamprecht 2023-01-23 15:55 ` [pmg-devel] [PATCH pmg-api 2/2] smtputf8: keep smtputf8 from incoming postfix, detect for local mail Stoiko Ivanov 2023-01-25 9:30 ` Thomas Lamprecht 2023-01-25 9:48 ` Stoiko Ivanov 2023-01-25 10:04 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox