Re: [pbs-devel] [PATCH proxmox 1/4] sendmail: add sendmail crate

[Lukas Wagner <l.wagner@proxmox.com>]

Gave these changes a quick test in Proxmox Backup Server as well as in proxmox-mail-forward.

Looks good!

Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>

(the latter iff the minor issues I mentioned are addressed)


On  2024-11-29 11:53, Shannon Sterz wrote:
> add the `proxmox-sendmail` crate that makes it easier to send mails via
> the `sendmail` utility. features include:
> 
> - multipart/alternative support for html+plain text mails
> - mulitpart/mixed support for mails with attachments
      ^ typo

> - automatic nesting of multipart/alternative and multipart/mixed parts
> - masking recipients to avoid disclosing them to everyone
> - encoding Subject, To, From, and attachment file names correctly
> - adding an `Auto-Submitted` header to avoid triggering automated mails
> 
> also includes several tests to ensure that mails are formatted
> correctly.
> 
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
>  Cargo.toml                  |   2 +
>  proxmox-sendmail/Cargo.toml |  16 +
>  proxmox-sendmail/src/lib.rs | 664 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 682 insertions(+)
>  create mode 100644 proxmox-sendmail/Cargo.toml
>  create mode 100644 proxmox-sendmail/src/lib.rs
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 84fbe979..b62fcd50 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -33,6 +33,7 @@ members = [
>      "proxmox-rrd-api-types",
>      "proxmox-schema",
>      "proxmox-section-config",
> +    "proxmox-sendmail",
>      "proxmox-serde",
>      "proxmox-shared-cache",
>      "proxmox-shared-memory",
> @@ -138,6 +139,7 @@ proxmox-rest-server = { version = "0.8.0", path = "proxmox-rest-server" }
>  proxmox-router = { version = "3.0.0", path = "proxmox-router" }
>  proxmox-schema = { version = "3.1.2", path = "proxmox-schema" }
>  proxmox-section-config = { version = "2.1.0", path = "proxmox-section-config" }
> +proxmox-sendmail = { version = "0.1.0", path = "proxmox-sendmail" }
>  proxmox-serde = { version = "0.1.1", path = "proxmox-serde", features = [ "serde_json" ] }
>  proxmox-shared-memory = { version = "0.3.0", path = "proxmox-shared-memory" }
>  proxmox-sortable-macro = { version = "0.1.3", path = "proxmox-sortable-macro" }
> diff --git a/proxmox-sendmail/Cargo.toml b/proxmox-sendmail/Cargo.toml
> new file mode 100644
> index 00000000..790b324b
> --- /dev/null
> +++ b/proxmox-sendmail/Cargo.toml
> @@ -0,0 +1,16 @@
> +[package]
> +name = "proxmox-sendmail"
> +version = "0.1.0"
> +authors.workspace = true
> +edition.workspace = true
> +license.workspace = true
> +repository.workspace = true
> +homepage.workspace = true
> +exclude.workspace = true
> +rust-version.workspace = true
> +
> +[dependencies]
> +anyhow = { workspace = true }
> +base64 = { workspace = true }
> +percent-encoding = { workspace = true }
> +proxmox-time = { workspace = true }
> diff --git a/proxmox-sendmail/src/lib.rs b/proxmox-sendmail/src/lib.rs
> new file mode 100644
> index 00000000..c97c3186
> --- /dev/null
> +++ b/proxmox-sendmail/src/lib.rs
> @@ -0,0 +1,664 @@
> +//!
> +//! This library implements the [`Mail`] trait which makes it easy to send emails with attachments
> +//! and alternative html parts to one or multiple receivers via ``sendmail``.
> +//!
> +
> +use std::io::Write;
> +use std::process::{Command, Stdio};
> +
> +use anyhow::{bail, Context, Error};
> +use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS};
> +
> +// Characters in this set will be encoded, so reproduce the inverse of the set described by RFC5987
> +// Section 3.2.1 `attr-char`, as that describes all characters that **don't** need encoding:
> +//
> +// https://datatracker.ietf.org/doc/html/rfc5987#section-3.2.1
> +//
> +// `CONTROLS` contains all control characters 0x00 - 0x1f and 0x7f as well as all non-ascii
> +// characters, so we need to add all characters here that aren't described in `attr-char` that are
> +// in the range 0x20-0x7e
> +const RFC5987SET: &AsciiSet = &CONTROLS
> +    .add(b' ')
> +    .add(b'"')
> +    .add(b'%')
> +    .add(b'&')
> +    .add(b'\'')
> +    .add(b'(')
> +    .add(b')')
> +    .add(b'*')
> +    .add(b',')
> +    .add(b'/')
> +    .add(b':')
> +    .add(b';')
> +    .add(b'<')
> +    .add(b'=')
> +    .add(b'>')
> +    .add(b'?')
> +    .add(b'@')
> +    .add(b'[')
> +    .add(b'\\')
> +    .add(b']')
> +    .add(b'{')
> +    .add(b'}');
> +
> +struct Recipient {
> +    name: Option<String>,
> +    email: String,
> +}
> +
> +struct Attachment<'a> {
> +    filename: String,
> +    mime: String,
> +    content: &'a [u8],
> +}
> +
> +pub struct Mail<'a> {

Doc comment is missing

> +    mail_author: String,
> +    mail_from: String,
> +    subject: String,
> +    to: Vec<Recipient>,
> +    body_txt: String,
> +    body_html: Option<String>,
> +    attachments: Vec<Attachment<'a>>,
> +}
> +
> +impl<'a> Mail<'a> {
> +    /// Creates a new mail with a mail author, from address, subject line and a plain text body.
> +    ///
> +    /// Note: If the author's name or the subject line contains UTF-8 characters they will be
> +    /// appropriatelly encoded.

nit: typo in appropriately

> +    pub fn new(mail_author: &str, mail_from: &str, subject: &str, body_txt: &str) -> Self {
> +        Self {
> +            mail_author: mail_author.to_string(),
> +            mail_from: mail_from.to_string(),
> +            subject: subject.to_string(),
> +            to: Vec::new(),
> +            body_txt: body_txt.to_string(),
> +            body_html: None,
> +            attachments: Vec::new(),
> +        }
> +    }
> +
> +    /// Adds a recipient to the mail without specifying a name separately.
> +    ///
> +    /// Note: No formatting or encoding will be done here, the value will be passed to the `To:`
> +    /// header directly.
> +    pub fn add_recipient(&mut self, email: &str) {
> +        self.to.push(Recipient {
> +            name: None,
> +            email: email.to_string(),
> +        });
> +    }
> +
> +    /// Builder-pattern method to conveniently add a recipient to an email without specifying a
> +    /// name separately.
> +    ///
> +    /// Note: No formatting or encoding will be done here, the value will be passed to the `To:`
> +    /// header directly.
> +    pub fn with_recipient(mut self, email: &str) -> Self {
> +        self.add_recipient(email);
> +        self
> +    }
> +
> +    /// Adds a recipient to the mail with a name.
> +    ///
> +    /// Notes:
> +    ///
> +    /// - If the name contains UTF-8 characters it will be encoded. Then the possibly encoded name
> +    ///   and non-encoded email address will be passed to the `To:` header in this format:
> +    ///   `{encoded_name} <{email}>`
> +    /// - If multiple receivers are specified, they will be masked so as not to disclose them to

Thinking about the main usecase of this new crate, which is to provide the implementation
to send notification mails from PVE and PBS, I think the masking part should be 
configurable.

In the most common case, a notification mail might go to the members of an infra team of
a organization, where the identities of other team members is not really sensitive information.
I'd actually go as far and say that the info "who else was notified" is actually quite valuable
and useful to have.

Then again, I can see the benefits of masking, e.g. in the case of PBS datastore notifications,
which might go to non-admin users (e.g. when PBS is offered as a service a la Tuxis).

I don't care that much whether this is opt-in or opt-out at the crate level, but at the
'sendmail target' level I'd make this configurable and opt-in (gut feeling and to not
change the current behavior, I'd be happy to be convinced for another way :) )

What are your thoughts about this?

> +    ///   other receivers.
> +    pub fn add_recipient_and_name(&mut self, name: &str, email: &str) {
> +        self.to.push(Recipient {
> +            name: Some(name.to_string()),
> +            email: email.to_string(),
> +        });
> +    }
> +
> +    /// Builder-style method to conveniently add a reciepient with a name to an email.

nit: typo in recipient

> +    ///
> +    /// Notes:
> +    ///
> +    /// - If the name contains UTF-8 characters it will be encoded. Then the possibly encoded name
> +    ///   and non-encoded email address will be passed to the `To:` header in this format:
> +    ///   `{encoded_name} <{email}>`
> +    /// - If multiple receivers are specified, they will be masked so as not to disclose them to
> +    ///   other receivers.
> +    pub fn with_recipient_and_name(mut self, name: &str, email: &str) -> Self {
> +        self.add_recipient_and_name(name, email);
> +        self
> +    }
> +
> +    /// Adds an attachment with a specified file name and mime-type to an email.
> +    ///
> +    /// Note: Adding attachements triggers `multipart/mixed` mode.

nit: typo in attachment

> +    pub fn add_attachment(&mut self, filename: &str, mime_type: &str, content: &'a [u8]) {
> +        self.attachments.push(Attachment {
> +            filename: filename.to_string(),
> +            mime: mime_type.to_string(),
> +            content,
> +        });
> +    }
> +
> +    /// Builder-style method to conveniently add an attachment with a specific filename and
> +    /// mime-type to an email.
> +    ///
> +    /// Note: Adding attachements triggers `multipart/mixed` mode.
> +    pub fn with_attachment(mut self, filename: &str, mime_type: &str, content: &'a [u8]) -> Self {
> +        self.add_attachment(filename, mime_type, content);
> +        self
> +    }
> +
> +    /// Set an alternative HTML part.
> +    ///
> +    /// Note: This triggers `multipart/alternative` mode. If both an HTML part and at least one
> +    /// attachement are specified, the `multipart/alternative` part will be nested within the first

nit: typo in attachment

> +    /// `multipart/mixed` part. This should ensure that the HTML is displayed properly by client's
> +    /// that prioritize it over the plain text part (should be the default for most clients) while
> +    /// also properly displaying the attachments.
> +    pub fn set_html_alt(&mut self, body_html: &str) {
> +        self.body_html.replace(body_html.to_string());
> +    }
> +
> +    /// Builder-style method to add an alternative HTML part.
> +    ///
> +    /// Note: This triggers `multipart/alternative` mode. If both an HTML part and at least one
> +    /// attachement are specified, the `multipart/alternative` part will be nested within the 

nit: typo in attachment

first
> +    /// `multipart/mixed` part. This should ensure that the HTML is displayed properly by client's
> +    /// that prioritize it over the plain text part (should be the default for most clients) while
> +    /// also properly displaying the attachments.
> +    pub fn with_html_alt(mut self, body_html: &str) -> Self {
> +        self.set_html_alt(body_html);
> +        self
> +    }
> +
> +    /// Sends the email. This will fail if no reciepient's have been added.

Nit: typo in 'recipient'

> +    ///
> +    /// Note: An `Auto-Submitted: auto-generated` header is added to avoid triggering OOO and
> +    /// similar mails.
> +    pub fn send(&self) -> Result<(), Error> {
> +        if self.to.is_empty() {
> +            bail!("no recipients provided for the mail, cannot send it.");
> +        }
> +
> +        let now = proxmox_time::epoch_i64();
> +        let body = self.format_mail(now)?;
> +
> +        let mut sendmail_process = Command::new("/usr/sbin/sendmail")
> +            .arg("-B")
> +            .arg("8BITMIME")
> +            .arg("-f")
> +            .arg(&self.mail_from)
> +            .arg("--")
> +            .args(self.to.iter().map(|p| &p.email).collect::<Vec<&String>>())
> +            .stdin(Stdio::piped())
> +            .spawn()
> +            .with_context(|| "could not spawn sendmail process")?;
> +
> +        sendmail_process
> +            .stdin
> +            .as_ref()
> +            .unwrap()
> +            .write_all(body.as_bytes())
> +            .with_context(|| "couldn't write to sendmail stdin")?;
> +
> +        sendmail_process
> +            .wait()
> +            .with_context(|| "sendmail did not exit successfully")?;
> +
> +        Ok(())
> +    }
> +
> +    fn format_mail(&self, now: i64) -> Result<String, Error> {
> +        let mut body = String::new();
> +        let file_boundary = format!("----_=_NextPart_001_{}", now);
> +        let html_boundary = format!("----_=_NextPart_002_{}", now);
> +
> +        let to = if self.to.len() > 1 {
> +            // don't disclose all recipients if the mail goes out to multiple
> +            &Recipient {
> +                name: Some("Undisclosed".to_string()),
> +                email: "noreply".to_string(),
> +            }
> +        } else {
> +            self.to
> +                .first()
> +                .expect("the checks before make sure there is at least one recipient")
> +        };
> +
> +        if !self.attachments.is_empty() {
> +            body.push_str("Content-Type: multipart/mixed;\n");
> +            body.push_str(&format!("\tboundary=\"{file_boundary}\"\n"));
> +            body.push_str("MIME-Version: 1.0\n");
> +        } else if self.body_html.is_some() {
> +            body.push_str("Content-Type: multipart/alternative;\n");
> +            body.push_str(&format!("\tboundary=\"{html_boundary}\"\n"));
> +            body.push_str("MIME-Version: 1.0\n");
> +        } else if !self.subject.is_ascii()
> +            || !self.mail_author.is_ascii()
> +            || !to.name.as_ref().map(|t| t.is_ascii()).unwrap_or(true)
> +        {
> +            body.push_str("MIME-Version: 1.0\n");
> +        }
> +
> +        let subject = if !self.subject.is_ascii() {
> +            format!("Subject: =?utf-8?B?{}?=\n", base64::encode(&self.subject))
> +        } else {
> +            format!("Subject: {}\n", self.subject)
> +        };
> +
> +        body.push_str(&subject);
> +
> +        let from = if !self.mail_author.is_ascii() {
> +            format!(
> +                "From: =?utf-8?B?{}?= <{}>\n",
> +                base64::encode(&self.mail_author),
> +                self.mail_from
> +            )
> +        } else {
> +            format!("From: {} <{}>\n", self.mail_author, self.mail_from)
> +        };
> +
> +        body.push_str(&from);
> +
> +        let to = if let Some(name) = &to.name {
> +            if !name.is_ascii() {
> +                format!("To: =?utf-8?B?{}?= <{}>\n", base64::encode(&name), to.email)
> +            } else {
> +                format!("To: {} <{}>\n", name, to.email)
> +            }
> +        } else {
> +            format!("To: {}\n", to.email)
> +        };
> +
> +        body.push_str(&to);
> +
> +        let rfc2822_date = proxmox_time::epoch_to_rfc2822(now)
> +            .with_context(|| "could not convert epoch to rfc2822 date")?;
> +        body.push_str(&format!("Date: {rfc2822_date}\n"));
> +        body.push_str("Auto-Submitted: auto-generated;\n");
> +
> +        if self.body_html.is_some() && !self.attachments.is_empty() {
> +            body.push_str("\nThis is a multi-part message in MIME format.\n");
> +            body.push_str(&format!("\n--{file_boundary}\n"));
> +            body.push_str(&format!(
> +                "Content-Type: multipart/alternative; boundary=\"{html_boundary}\"\n"
> +            ));
> +            body.push_str("MIME-Version: 1.0\n");
> +            body.push_str(&format!("\n--{html_boundary}\n"));
> +        } else if self.body_html.is_some() {
> +            body.push_str("\nThis is a multi-part message in MIME format.\n");
> +            body.push_str(&format!("\n--{html_boundary}\n"));
> +        } else if self.body_html.is_none() && !self.attachments.is_empty() {
> +            body.push_str("\nThis is a multi-part message in MIME format.\n");
> +            body.push_str(&format!("\n--{file_boundary}\n"));
> +        }
> +
> +        body.push_str("Content-Type: text/plain;\n");
> +        body.push_str("\tcharset=\"UTF-8\"\n");
> +        body.push_str("Content-Transfer-Encoding: 8bit\n\n");
> +        body.push_str(&self.body_txt);
> +
> +        if let Some(html) = &self.body_html {
> +            body.push_str(&format!("\n--{html_boundary}\n"));
> +            body.push_str("Content-Type: text/html;\n");
> +            body.push_str("\tcharset=\"UTF-8\"\n");
> +            body.push_str("Content-Transfer-Encoding: 8bit\n\n");
> +            body.push_str(html);
> +            body.push_str(&format!("\n--{html_boundary}--"));
> +        }
> +
> +        for attachment in &self.attachments {
> +            let filename = &attachment.filename;
> +
> +            body.push_str(&format!("\n--{file_boundary}\n"));
> +            body.push_str(&format!(
> +                "Content-Type: {}; name=\"{filename}\"\n",
> +                attachment.mime
> +            ));
> +
> +            // both `filename` and `filename*` are included for additional compatability
> +            body.push_str(&format!(
> +                "Content-Disposition: attachment; filename=\"{filename}\"; filename*=UTF-8''{}\n",
> +                utf8_percent_encode(filename, RFC5987SET)
> +            ));
> +            body.push_str("Content-Transfer-Encoding: base64\n\n");
> +
> +            // wrap the base64 string every 72 characters. this improves compatability
> +            let base64 = base64::encode(attachment.content)
> +                .chars()
> +                .enumerate()
> +                .flat_map(|(i, c)| {
> +                    if i != 0 && i % 72 == 0 {
> +                        Some('\n')
> +                    } else {
> +                        None
> +                    }
> +                    .into_iter()
> +                    .chain(std::iter::once(c))
> +                })
> +                .collect::<String>();
> +            body.push_str(&base64);
> +        }
> +
> +        if !self.attachments.is_empty() {
> +            body.push_str(&format!("\n--{file_boundary}--"));
> +        }
> +
> +        Ok(body)
> +    }

I think with added support for attachments and the other changes it might make sense
to start breaking the method into smaller sub-methods. Personally I found it a bit
hard to follow the way it is right now :) At very least I'd try to break this into format_header
and format_body, in the latter one could probably also break out the attachment part.

What do you think?


Also, I think debian packaging should be added in this or a separate commit.


-- 
- Lukas



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel