* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.Service provided by Proxmox Server Solutions GmbH | Privacy | Legal