From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 316DD6379D for ; Tue, 25 Aug 2020 11:16:27 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1999C24BA8 for ; Tue, 25 Aug 2020 11:15:57 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id EE3BC24B9A for ; Tue, 25 Aug 2020 11:15:55 +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 B93A24486E for ; Tue, 25 Aug 2020 11:15:55 +0200 (CEST) Date: Tue, 25 Aug 2020 11:15:54 +0200 From: Stoiko Ivanov To: Hannes Laimer Cc: Proxmox Backup Server development discussion Message-ID: <20200825111554.379b23ef@rosa.proxmox.com> In-Reply-To: <20200824082632.470138-2-h.laimer@proxmox.com> References: <20200824082632.470138-1-h.laimer@proxmox.com> <20200824082632.470138-2-h.laimer@proxmox.com> X-Mailer: Claws Mail 3.17.3 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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, email.rs] Subject: Re: [pbs-devel] [PATCH proxmox v2 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Aug 2020 09:16:27 -0000 Thanks for the v2 - some comments inline: On Mon, 24 Aug 2020 10:26:32 +0200 Hannes Laimer wrote: > Signed-off-by: Hannes Laimer > --- > v1->v2: as mentioned in cover-letter > proxmox/src/tools/email.rs | 137 +++++++++++++++++++++++++++++++++++++ > proxmox/src/tools/mod.rs | 1 + > 2 files changed, 138 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..99e2906 > --- /dev/null > +++ b/proxmox/src/tools/email.rs > @@ -0,0 +1,137 @@ > +//! Email related utilities. > + > +use std::process::{Command, Stdio}; > +use anyhow::{bail, Error}; > +use std::io::Write; > +use chrono::{DateTime, Utc}; > +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(); the regex is a bit too restricted (and we should probably consider introducing one to which we refer everywhere where we need e-mails in general (in the perl code base we have a few, all having small distinctions, which cause confusion quite often) - but this might be out of scope for this patch... In this case I noticed that '-' is not allowed neither in the local part nor in the domain part - but e.g. 'siv@pbs-test-container.test.proxmox.com' is a valid e-mail address... > + > + 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 now: DateTime = Utc::now(); Could/should be in local Timezone (would be less confusing to me) > + > + 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()?); > + if let (Some(_), Some(_)) = (text, html) { > + body.push_str("Content-Type: multipart/alternative;\n"); > + } > + body.push_str(&format!("\tboundary=\"{}\"\n", boundary)); > + body.push_str("MIME-Version: 1.0\n"); the above two lines also belong inside the if - otherwise you get broken headers (the 'boundary=' is part of the Content-Type header that starts above) - also I would combine the headers with the if condition below ([1]) > + body.push_str(&format!("From: {} <{}>\n", author, mailfrom)); > + body.push_str(&format!("To: {}\n", &recipients)); > + body.push_str(&format!("Date: {}\n", now.to_rfc2822())); > + body.push_str(&format!("Subject: =?utf-8?B?{}?=\n", base64::encode(subject))); cosmetic: how about: if !subject.is_ascii() { body.push_str(&format!("Subject: {}\n", subject)); } else { body.push_str(&format!("Subject: =?utf-8?B?{}?=\n", base64::encode(subject))); } > + body.push('\n'); > + if let (Some(_), Some(_)) = (text, html) { > + body.push_str("This is a multi-part message in MIME format.\n\n"); > + } [1] > + body.push_str(&format!("--{}\n", boundary)); boundary should only be printed in the multipart-case > + if let Some(text) = text { > + body.push_str("Content-Type: text/plain;\n"); > + body.push_str("\tcharset=\"UTF-8\"\n"); > + body.push_str("Content-Transfer-Encoding: 8bit\n"); > + body.push('\n'); > + body.push_str(text); > + body.push_str(&format!("\n--{}\n", boundary)); here as well - only print the boundary if there is another part > + } > + if let Some(html) = html { > + body.push_str("Content-Type: text/html;\n"); > + body.push_str("\tcharset=\"UTF-8\"\n"); > + body.push_str("Content-Transfer-Encoding: 8bit\n"); > + body.push('\n'); > + body.push_str(html); > + body.push_str(&format!("\n--{}\n", boundary)); here as well - only print the boundary if there is another part > + } > + > + if let Err(err) = sendmail_process.stdin.take().unwrap().write_all(body.as_bytes()) { > + bail!("couldn't write to sendmail stdin: {}", err) > + }; > + > + // wait() closes stdin of the child > + if let Err(err) = sendmail_process.wait() { > + bail!("sendmail did not exit successfully: {}", err) > + } > + > + Ok(()) > +} > + > +#[cfg(test)] > +mod test { > + use crate::tools::email::sendmail; > + > + #[test] > + fn test1() { > + let result = sendmail( > + vec!["somenotvalidemail!", "somealmostvalid email"], > + "Subject1", > + Some("TEXT"), > + Some("HTML"), > + Some("bim@bam.bum"), > + Some("test1")); > + assert!(result.is_err()); > + } > + > + #[test] > + fn test2() { > + let result = sendmail( > + vec![], > + "Subject2", > + None, > + Some("HTML"), > + None, > + Some("test1")); > + assert!(result.is_err()); > + } > + > + #[test] > + fn test3() { > + let result = sendmail( > + vec!["a@b.c"], > + "Subject3", > + None, > + Some("HTML"), > + Some("notv@lid.com!"), > + Some("test1")); > + assert!(result.is_err()); > + } > +} > \ No newline at end of file > diff --git a/proxmox/src/tools/mod.rs b/proxmox/src/tools/mod.rs > index 721e5d1..df6c429 100644 > --- a/proxmox/src/tools/mod.rs > +++ b/proxmox/src/tools/mod.rs > @@ -10,6 +10,7 @@ pub mod borrow; > pub mod byte_buffer; > pub mod common_regex; > pub mod constnamemap; > +pub mod email; > pub mod fd; > pub mod fs; > pub mod io;