From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
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))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id D5872623DC
 for <pbs-devel@lists.proxmox.com>; Thu, 20 Aug 2020 16:08:25 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id C34892DEEA
 for <pbs-devel@lists.proxmox.com>; Thu, 20 Aug 2020 16:08:25 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [212.186.127.180])
 (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 A88202DEDA
 for <pbs-devel@lists.proxmox.com>; Thu, 20 Aug 2020 16:08:23 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 32D2143398
 for <pbs-devel@lists.proxmox.com>; Thu, 20 Aug 2020 16:08:23 +0200 (CEST)
To: Proxmox Backup Server development discussion
 <pbs-devel@lists.proxmox.com>, Stoiko Ivanov <s.ivanov@proxmox.com>,
 Hannes Laimer <h.laimer@proxmox.com>
References: <20200820092351.71864-1-h.laimer@proxmox.com>
 <20200820092351.71864-2-h.laimer@proxmox.com>
 <20200820143403.17201a47@rosa.proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
Message-ID: <a5bd1a81-7d1d-89ae-27b7-cd2f15f4c21b@proxmox.com>
Date: Thu, 20 Aug 2020 16:08:19 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:80.0) Gecko/20100101
 Thunderbird/80.0
MIME-Version: 1.0
In-Reply-To: <20200820143403.17201a47@rosa.proxmox.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.606 Adjusted score from AWL reputation of From: address
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 NICE_REPLY_A           -1.361 Looks like a legit reply (A)
 RCVD_IN_DNSWL_MED        -2.3 Sender listed at https://www.dnswl.org/,
 medium trust
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
 http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
 information. [mod.rs, proxmox.com, email.rs, ietf.org]
Subject: Re: [pbs-devel] [PATCH proxmox 1/1] email: add small function to
 send multi-part emails using sendmail
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
 <pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Thu, 20 Aug 2020 14:08:25 -0000

On 20.08.20 14:34, Stoiko Ivanov wrote:
> Thanks for the patch!
> 
> some comments/suggestions on e-mail inline:
> 
> On Thu, 20 Aug 2020 11:23:50 +0200
> Hannes Laimer <h.laimer@proxmox.com> wrote:
> 
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>>  proxmox/src/tools/email.rs | 130 +++++++++++++++++++++++++++++++++++++
>>  proxmox/src/tools/mod.rs   |   1 +
>>  2 files changed, 131 insertions(+)
>>  create mode 100644 proxmox/src/tools/email.rs
>>
>> diff --git a/proxmox/src/tools/email.rs b/proxmox/src/tools/email.rs
>> new file mode 100644
>> index 0000000..b73a6d6
>> --- /dev/null
>> +++ b/proxmox/src/tools/email.rs
>> @@ -0,0 +1,130 @@
>> +//! Email related utilities.
>> +
>> +use std::process::{Command, Stdio};
>> +use anyhow::{bail, Error};
>> +use std::io::Write;
>> +use crate::tools::time::time;
>> +
>> +
>> +/// Sends multi-part mail with text and/or html to a list of recipients
>> +///
>> +/// ``sendmail`` is used for sending the mail.
>> +pub fn sendmail(mailto: Vec<&str>,
>> +                subject: &str,
>> +                text: Option<&str>,
>> +                html: Option<&str>,
>> +                mailfrom: Option<&str>,
>> +                author: Option<&str>) -> Result<(), Error> {
>> +    let mail_regex = regex::Regex::new(r"^[a-zA-Z\.0-9]+@[a-zA-Z\.0-9]+$").unwrap();
>> +
>> +    if mailto.is_empty() {
>> +        bail!("At least one recipient has to be specified!")
>> +    }
>> +
>> +    for recipient in &mailto {
>> +        if !mail_regex.is_match(recipient) {
>> +            bail!("'{}' is not a valid email address", recipient)
>> +        }
>> +    }
>> +
>> +    let mailfrom = mailfrom.unwrap_or("root");
>> +    if !mailfrom.eq("root") && !mail_regex.is_match(mailfrom) {
>> +        bail!("'{}' is not a valid email address", mailfrom)
>> +    }
>> +
>> +    let recipients = mailto.join(",");
>> +    let author = author.unwrap_or("Proxmox Backup Server");
>> +
>> +    let mut sendmail_process = match Command::new("/usr/sbin/sendmail")
>> +        .arg("-B")
>> +        .arg("8BITMIME")
>> +        .arg("-f")
>> +        .arg(mailfrom)
>> +        .arg("--")
>> +        .arg(&recipients)
>> +        .stdin(Stdio::piped())
>> +        .spawn() {
>> +        Err(err) => bail!("could not spawn sendmail process: {}", err),
>> +        Ok(process) => process
>> +    };
>> +    let mut body = String::new();
>> +    let boundary = format!("----_=_NextPart_001_{}", time()?);
>> +
>> +    body.push_str("Content-Type: multipart/alternative;\n");
>> +    body.push_str(&format!("\tboundary=\"{}\"\n", boundary));
>> +    body.push_str("MIME-Version: 1.0\n");
>> +    body.push_str(&format!("FROM: {} <{}>\n", author, mailfrom));
>> +    body.push_str(&format!("TO: {}\n", &recipients));
>> +    body.push_str(&format!("SUBJECT: {}\n", subject));
> 
> iirc the header needs to be encoded if it contains non-ascii characters
> (the content-type charset only relates to the body) - I assume that
> postfix/exim would do the correct thing these days (and send the mail with
> smtputf-8 extension (though I'm not sure what would happen if the
> receiving server does not support that)
> - would suggest to test sending such a mail with a non-ascii character
> in the subject.
> 
> writing the header names (TO FROM SUBJECT) in all-caps seems odd to my
> eyes (but I guess this is pretty cosmetic)
> 
> I would suggest adding a Date header (though postfix does that for locally
> submitted e-mail) - else the mails are more likely to be considered spam.
> 
> Finally - if you only have a text/plain part I would only send that part
> (without the wrapping in multipart/alternative) (similarly for a
> single text/html part without text/plain part).
> 
> 
>> +    body.push('\n');
>> +    body.push_str("This is a multi-part message in MIME format.\n\n");
>> +    body.push_str(&format!("--{}\n", boundary));
>> +    if let Some(text) = text {
>> +        body.push_str("Content-Type: text/plain;\n");
>> +        body.push_str("\tcharset=\"UTF8\"\n");
> While most MTA, scanners, MUAs won't care much - the charset should
> probably be written as 'UTF-8' (see https://tools.ietf.org/html/rfc3629
> (section 8))
> 

You're right with your feedback, but I guess that most stems from the original
Perl implementation[0] Hannes used as model.

We (meaning one of you two ;P) should overhaul that one too then.

[0]: https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/Tools.pm;h=f9270d9b10340eec789346e81f4a926772ccabfd;hb=HEAD#l1439