[pve-devel] [PATCH common/manager v2] adapt PVE::Tools::sendmail to match rust-implementation and use it for apt update notifications

[pve-devel] [PATCH common/manager v2] adapt PVE::Tools::sendmail to match rust-implementation and use it for apt update notifications

[Stoiko Ivanov]

v1->v2:
* dropped the moving of the email regex for reuse in the sendmail helper:
  we use local usernames (mostly 'root') quite extensively in our code-base and
  our users probably do so as well (for backup notifications)
* replaced the direct invocation of /usr/sbin/sendmail by a call to the helper
  in pve-manager/PVE/API2/APT.pm
* adapted the sendmail-helper to allow for an empty display name in the from
  header, since the apt update notification mail got sent without one.

The second patch in pve-common can be squashed into the first one, if
preferred.

original cover-letter for v1:
The 2 patches adapt PVE::Tools::sendmail to closely match the recently merged
implementation in our rust repository - see [0].

I moved the email regex from JSONSchema to Tools to reuse it for the sendmail
function (and eliminate one of the few email-address regexes in our codebase).

I did not add a dependency on libtimedate-perl (where Date::Format is), since
we already use  Date::Parse in PVE::Certificate, without explicit dependency,
and it gets pulled in via libwww-perl -> libhttp-date-perl -> libtimedate-perl.

Glad to send an update for the dependency of course.

[0] https://lists.proxmox.com/pipermail/pbs-devel/2020-August/000423.html

Stoiko Ivanov (2):
  move email regex from JSONSchema to Tools
  sendmail-helper: only send multipart if necessary

 src/PVE/JSONSchema.pm |  4 ++--
 src/PVE/Tools.pm      | 49 +++++++++++++++++++++++++++++--------------
 2 files changed, 35 insertions(+), 18 deletions(-)

pve-common:
Stoiko Ivanov (2):
  sendmail-helper: only send multipart if necessary
  sendmail helper: allow empty display name in from

 src/PVE/Tools.pm | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

pve-manager:
Stoiko Ivanov (1):
  use PVE::Tools::sendmail for update notifications

 PVE/API2/APT.pm | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH common v2 1/2] sendmail-helper: only send multipart if necessary

[Stoiko Ivanov]

PVE::Tools::sendmail currently always sends a multipart/alternative message
irrespective of the actual content of the mail (e.g. a plain-text only mail
need not be sent as multipart message).

Additionally a few small refactorings based on the discussion
in https://lists.proxmox.com/pipermail/pbs-devel/2020-August/000423.html
and commited in 66004f22c6475ceb0146cf2df1f380f9f0274be4 in the
rust proxmox repository git://git.proxmox.com/git/proxmox.git
were carried over.

tested by creating a backup of a VM and setting an e-mail address, having
ha-manager send a mail after fencing as well as sending a few small mails via
'perl -e'

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PVE/Tools.pm | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index f9270d9..3b5b199 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -25,6 +25,8 @@ use Text::ParseWords;
 use String::ShellQuote;
 use Time::HiRes qw(usleep gettimeofday tv_interval alarm);
 use Scalar::Util 'weaken';
+use Date::Format qw(time2str);
+
 use PVE::Syscall;
 
 # avoid warning when parsing long hex values with hex()
@@ -1460,23 +1462,36 @@ sub sendmail {
     open (MAIL, "|-", "sendmail", "-B", "8BITMIME", "-f", $mailfrom, "--", @$mailto) ||
 	die "unable to open 'sendmail' - $!";
 
+    my $date = time2str('%a, %d %b %Y %H:%M:%S %z', time());
+
+    my $is_multipart = $text && $html;
+
     # multipart spec see https://www.ietf.org/rfc/rfc1521.txt
     my $boundary = "----_=_NextPart_001_".int(time).$$;
 
-    print MAIL "Content-Type: multipart/alternative;\n";
-    print MAIL "\tboundary=\"$boundary\"\n";
-    print MAIL "MIME-Version: 1.0\n";
+    if ($subject =~ /[^[:ascii:]]/) {
+	$subject = Encode::encode('MIME-Header', $subject);
+    }
 
-    print MAIL "FROM: $author <$mailfrom>\n";
-    print MAIL "TO: $rcvrtxt\n";
-    print MAIL "SUBJECT: $subject\n";
-    print MAIL "\n";
-    print MAIL "This is a multi-part message in MIME format.\n\n";
-    print MAIL "--$boundary\n";
+    if ($subject =~ /[^[:ascii:]]/ || $is_multipart) {
+	print MAIL "MIME-Version: 1.0\n";
+    }
+    print MAIL "From: $author <$mailfrom>\n";
+    print MAIL "To: $rcvrtxt\n";
+    print MAIL "Date: $date\n";
+    print MAIL "Subject: $subject\n";
+
+    if ($is_multipart) {
+	print MAIL "Content-Type: multipart/alternative;\n";
+	print MAIL "\tboundary=\"$boundary\"\n";
+	print MAIL "\n";
+	print MAIL "This is a multi-part message in MIME format.\n\n";
+	print MAIL "--$boundary\n";
+    }
 
     if (defined($text)) {
 	print MAIL "Content-Type: text/plain;\n";
-	print MAIL "\tcharset=\"UTF8\"\n";
+	print MAIL "\tcharset=\"UTF-8\"\n";
 	print MAIL "Content-Transfer-Encoding: 8bit\n";
 	print MAIL "\n";
 
@@ -1486,18 +1501,18 @@ sub sendmail {
 
 	print MAIL $text;
 
-	print MAIL "\n--$boundary\n";
+	print MAIL "\n--$boundary\n" if $is_multipart;
     }
 
     if (defined($html)) {
 	print MAIL "Content-Type: text/html;\n";
-	print MAIL "\tcharset=\"UTF8\"\n";
+	print MAIL "\tcharset=\"UTF-8\"\n";
 	print MAIL "Content-Transfer-Encoding: 8bit\n";
 	print MAIL "\n";
 
 	print MAIL $html;
 
-	print MAIL "\n--$boundary--\n";
+	print MAIL "\n--$boundary--\n" if $is_multipart;
     }
 
     close(MAIL);
-- 
2.20.1

[pve-devel] [PATCH common v2 2/2] sendmail helper: allow empty display name in from

[Stoiko Ivanov]

This patch enables the sendmail helper sub to send emails with a non-existant
display name in the from address. This is used to replace the direct call to
the sendmail binary in pve-manager/PVE/API2/APT.pm.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PVE/Tools.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 3b5b199..e04b504 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1457,7 +1457,7 @@ sub sendmail {
     die "illegal character in mailfrom address\n"
 	if $mailfrom =~ $mail_re;
 
-    $author = $author || 'Proxmox VE';
+    $author = $author // 'Proxmox VE';
 
     open (MAIL, "|-", "sendmail", "-B", "8BITMIME", "-f", $mailfrom, "--", @$mailto) ||
 	die "unable to open 'sendmail' - $!";
-- 
2.20.1

[pve-devel] [PATCH manager v2 1/1] use PVE::Tools::sendmail for update notifications

[Stoiko Ivanov]

instead of calling /usr/sbin/sendmail directly.

Tested with:
```
echo "[]" > /var/lib/pve-manager/pkgupdates
pvesh create /nodes/rosa/apt/update --notify 1

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 PVE/API2/APT.pm | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
index 859c5f42..984c8103 100644
--- a/PVE/API2/APT.pm
+++ b/PVE/API2/APT.pm
@@ -344,15 +344,9 @@ __PACKAGE__->register_method({
 		    my $hostname = `hostname -f` || PVE::INotify::nodename();
 		    chomp $hostname;
 		    my $mailfrom = $dcconf->{email_from} || "root";
+		    my $subject = "New software packages available ($hostname)";
 
-		    my $data = "Content-Type: text/plain;charset=\"UTF8\"\n";
-		    $data .= "Content-Transfer-Encoding: 8bit\n";
-		    $data .= "FROM: <$mailfrom>\n";
-		    $data .= "TO: $mailto\n";
-		    $data .= "SUBJECT: New software packages available ($hostname)\n";
-		    $data .= "\n";
-
-		    $data .= "The following updates are available:\n\n";
+		    my $data = "The following updates are available:\n\n";
 
 		    my $count = 0;
 		    foreach my $p (sort {$a->{Package} cmp $b->{Package} } @$pkglist) {
@@ -365,14 +359,9 @@ __PACKAGE__->register_method({
 			}
 		    }
 
-		    return if !$count; 
-
-		    my $fh = IO::File->new("|sendmail -B 8BITMIME -f $mailfrom $mailto") || 
-			die "unable to open 'sendmail' - $!";
-
-		    print $fh $data;
+		    return if !$count;
 
-		    $fh->close() || die "unable to close 'sendmail' - $!";
+		    PVE::Tools::sendmail($mailto, $subject, $data, undef, $mailfrom, '');
 
 		    foreach my $pi (@$pkglist) {
 			$pi->{NotifyStatus} = $pi->{Version};
-- 
2.20.1

[pve-devel] applied-series: [PATCH common/manager v2] adapt PVE::Tools::sendmail to match rust-implementation and use it for apt update notifications

[Thomas Lamprecht]

On 03.09.20 14:09, Stoiko Ivanov wrote:
> v1->v2:
> * dropped the moving of the email regex for reuse in the sendmail helper:
>   we use local usernames (mostly 'root') quite extensively in our code-base and
>   our users probably do so as well (for backup notifications)
> * replaced the direct invocation of /usr/sbin/sendmail by a call to the helper
>   in pve-manager/PVE/API2/APT.pm
> * adapted the sendmail-helper to allow for an empty display name in the from
>   header, since the apt update notification mail got sent without one.
> 
> The second patch in pve-common can be squashed into the first one, if
> preferred.

The separation in two patches as you did was good and warranted.

> 
> original cover-letter for v1:
> The 2 patches adapt PVE::Tools::sendmail to closely match the recently merged
> implementation in our rust repository - see [0].
> 
> I moved the email regex from JSONSchema to Tools to reuse it for the sendmail
> function (and eliminate one of the few email-address regexes in our codebase).
> 
> I did not add a dependency on libtimedate-perl (where Date::Format is), since
> we already use  Date::Parse in PVE::Certificate, without explicit dependency,
> and it gets pulled in via libwww-perl -> libhttp-date-perl -> libtimedate-perl.
> 
> Glad to send an update for the dependency of course.

should still note that, depending on indirect dependency chains isn't to ideal.

> 
> [0] https://lists.proxmox.com/pipermail/pbs-devel/2020-August/000423.html
> 
> Stoiko Ivanov (2):
>   move email regex from JSONSchema to Tools
>   sendmail-helper: only send multipart if necessary
> 
>  src/PVE/JSONSchema.pm |  4 ++--
>  src/PVE/Tools.pm      | 49 +++++++++++++++++++++++++++++--------------
>  2 files changed, 35 insertions(+), 18 deletions(-)
> 
> pve-common:
> Stoiko Ivanov (2):
>   sendmail-helper: only send multipart if necessary
>   sendmail helper: allow empty display name in from
> 
>  src/PVE/Tools.pm | 43 +++++++++++++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 14 deletions(-)
> 
> pve-manager:
> Stoiko Ivanov (1):
>   use PVE::Tools::sendmail for update notifications
> 
>  PVE/API2/APT.pm | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 



applied series, thanks!