public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH proxmox 1/2] notify: smtp: forward original message instead nesting
@ 2023-12-13 16:42 Lukas Wagner
  2023-12-13 16:42 ` [pve-devel] [PATCH proxmox 2/2] notify: smtp: add `Auto-Submitted` header to email body Lukas Wagner
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Lukas Wagner @ 2023-12-13 16:42 UTC (permalink / raw)
  To: pve-devel

For mails forwarded by `proxmox-mail-forward` to an SMTP target, the
original message was nested as a 'message/rfc822' message part.
Originally this approach was chosen to avoid having to rewrite
message headers.
Good email-clients, such as Thunderbird can display these inline.
Other, more limited clients will show these messages as an attached
.eml file, which is not really a good user experience.

This patch changes the approach for message forwarding to be more like
forwarding mails in a mail client. We create a new message and
add the original message body as a body. Additionally, we also copy
over all message headers that are relevant to correctly display the
original message body (e.g. Content-Type, Content-Transfer-Encoding)

Tested with a couple of different email messages (varying in
structure, body parts, encoding, etc.) against the following SMTP
relays:
  - gmail
  - outlook
  - our own webmail service

Originally reported in our community forum:
https://forum.proxmox.com/threads/proxmox-mail-forward-sends-mails-as-eml.137710/

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---

Notes:
    proxmox-mail-forward needs a bump once this is applied.

 proxmox-notify/src/endpoints/smtp.rs | 99 ++++++++++++++++++++++++----
 1 file changed, 88 insertions(+), 11 deletions(-)

diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
index 064c9f9..4b8ff2d 100644
--- a/proxmox-notify/src/endpoints/smtp.rs
+++ b/proxmox-notify/src/endpoints/smtp.rs
@@ -1,8 +1,9 @@
+use std::time::Duration;
+
 use lettre::message::{Mailbox, MultiPart, SinglePart};
 use lettre::transport::smtp::client::{Tls, TlsParameters};
 use lettre::{message::header::ContentType, Message, SmtpTransport, Transport};
 use serde::{Deserialize, Serialize};
-use std::time::Duration;
 
 use proxmox_schema::api_types::COMMENT_SCHEMA;
 use proxmox_schema::{api, Updater};
@@ -231,17 +232,93 @@ impl Endpoint for SmtpEndpoint {
             }
             #[cfg(feature = "mail-forwarder")]
             Content::ForwardedMail { ref raw, title, .. } => {
-                email_builder = email_builder.subject(title);
+                use lettre::message::header::{ContentTransferEncoding, HeaderName, HeaderValue};
+                use lettre::message::Body;
 
-                // Forwarded messages are embedded inline as 'message/rfc822'
-                // this let's us avoid rewriting any headers (e.g. From)
-                email_builder
-                    .singlepart(
-                        SinglePart::builder()
-                            .header(ContentType::parse("message/rfc822").unwrap())
-                            .body(raw.to_owned()),
-                    )
-                    .map_err(|err| Error::NotifyFailed(self.name().into(), Box::new(err)))?
+                let parsed_message = mail_parser::Message::parse(raw)
+                    .ok_or_else(|| Error::Generic("could not parse forwarded email".to_string()))?;
+
+                let root_part = parsed_message
+                    .part(0)
+                    .ok_or_else(|| Error::Generic("root message part not present".to_string()))?;
+
+                let raw_body = parsed_message
+                    .raw_message()
+                    .get(root_part.offset_body..root_part.offset_end)
+                    .ok_or_else(|| Error::Generic("could not get raw body content".to_string()))?;
+
+                // We assume that the original message content is already properly
+                // encoded, thus we add the original message body 'Binary' encoding.
+                // This prohibits lettre from trying to re-encode our raw body data.
+                // lettre will automatically set the `Content-Transfer-Encoding: binary` header,
+                // which we need to remove. The actual transfer encoding is later
+                // copied from the original message headers.
+                let body =
+                    Body::new_with_encoding(raw_body.to_vec(), ContentTransferEncoding::Binary)
+                        .map_err(|_| Error::Generic("could not create body".into()))?;
+                let mut message = email_builder
+                    .subject(title)
+                    .body(body)
+                    .map_err(|err| Error::NotifyFailed(self.name().into(), Box::new(err)))?;
+                message
+                    .headers_mut()
+                    .remove_raw("Content-Transfer-Encoding");
+
+                // Copy over all headers that are relevant to display the original body correctly.
+                // Unfortunately this is a bit cumbersome, as we use separate crates for mail parsing (mail-parser)
+                // and creating/sending mails (lettre).
+                // Note: Other MIME-Headers, such as Content-{ID,Description,Disposition} are only used
+                // for body-parts in multipart messages, so we can ignore them for the messages headers.
+                // Since we send the original raw body, the part-headers will be included any way.
+                for header in parsed_message.headers() {
+                    let header_name = header.name.as_str();
+                    // Email headers are case-insensitive, so convert to lowercase...
+                    let value = match header_name.to_lowercase().as_str() {
+                        "content-type" => {
+                            if let mail_parser::HeaderValue::ContentType(ct) = header.value() {
+                                // mail_parser does not give us access to the full decoded and unfolded
+                                // header value, so we unfortunately need to reassemble it ourselves.
+                                // Meh.
+                                let mut value = ct.ctype().to_string();
+                                if let Some(subtype) = ct.subtype() {
+                                    value.push('/');
+                                    value.push_str(subtype);
+                                }
+                                if let Some(attributes) = ct.attributes() {
+                                    for attribute in attributes {
+                                        value.push_str(&format!(
+                                            "; {}=\"{}\"",
+                                            attribute.0, attribute.1
+                                        ));
+                                    }
+                                }
+                                Some(value)
+                            } else {
+                                None
+                            }
+                        }
+                        "content-transfer-encoding" | "mime-version" => {
+                            if let mail_parser::HeaderValue::Text(text) = header.value() {
+                                Some(text.to_string())
+                            } else {
+                                None
+                            }
+                        }
+                        _ => None,
+                    };
+
+                    if let Some(value) = value {
+                        match HeaderName::new_from_ascii(header_name.into()) {
+                            Ok(name) => {
+                                let header = HeaderValue::new(name, value);
+                                message.headers_mut().insert_raw(header);
+                            }
+                            Err(e) => log::error!("could not set header: {e}"),
+                        }
+                    }
+                }
+
+                message
             }
         };
 
-- 
2.39.2





^ permalink raw reply	[flat|nested] 5+ messages in thread

* [pve-devel] [PATCH proxmox 2/2] notify: smtp: add `Auto-Submitted` header to email body
  2023-12-13 16:42 [pve-devel] [PATCH proxmox 1/2] notify: smtp: forward original message instead nesting Lukas Wagner
@ 2023-12-13 16:42 ` Lukas Wagner
  2024-01-08 10:43 ` [pve-devel] [PATCH proxmox 1/2] notify: smtp: forward original message instead nesting Lukas Wagner
  2024-01-09 13:26 ` Wolfgang Bumiller
  2 siblings, 0 replies; 5+ messages in thread
From: Lukas Wagner @ 2023-12-13 16:42 UTC (permalink / raw)
  To: pve-devel

`Auto-Submitted` is defined in the rfc 5436 [1] and describes how
an automatic response (f.e. ooo replies, etc.) should behave on the
emails. When using `Auto-Submitted: auto-generated` (or any value
other than `none`) automatic replies won't be triggered.

[1]: https://www.rfc-editor.org/rfc/rfc3834.html

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/src/endpoints/smtp.rs | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
index 4b8ff2d..15724d3 100644
--- a/proxmox-notify/src/endpoints/smtp.rs
+++ b/proxmox-notify/src/endpoints/smtp.rs
@@ -1,5 +1,6 @@
 use std::time::Duration;
 
+use lettre::message::header::{HeaderName, HeaderValue};
 use lettre::message::{Mailbox, MultiPart, SinglePart};
 use lettre::transport::smtp::client::{Tls, TlsParameters};
 use lettre::{message::header::ContentType, Message, SmtpTransport, Transport};
@@ -199,7 +200,7 @@ impl Endpoint for SmtpEndpoint {
             email_builder = email_builder.to(parse_address(&recipient)?);
         }
 
-        let email = match &notification.content {
+        let mut email = match &notification.content {
             Content::Template {
                 title_template,
                 body_template,
@@ -232,7 +233,7 @@ impl Endpoint for SmtpEndpoint {
             }
             #[cfg(feature = "mail-forwarder")]
             Content::ForwardedMail { ref raw, title, .. } => {
-                use lettre::message::header::{ContentTransferEncoding, HeaderName, HeaderValue};
+                use lettre::message::header::ContentTransferEncoding;
                 use lettre::message::Body;
 
                 let parsed_message = mail_parser::Message::parse(raw)
@@ -322,6 +323,15 @@ impl Endpoint for SmtpEndpoint {
             }
         };
 
+        // `Auto-Submitted` is defined in RFC 5436 and describes how
+        // an automatic response (f.e. ooo replies, etc.) should behave on the
+        // emails. When using `Auto-Submitted: auto-generated` (or any value
+        // other than `none`) automatic replies won't be triggered.
+        email.headers_mut().insert_raw(HeaderValue::new(
+            HeaderName::new_from_ascii_str("Auto-Submitted"),
+            "auto-generated;".into(),
+        ));
+
         transport
             .send(&email)
             .map_err(|err| Error::NotifyFailed(self.name().into(), err.into()))?;
-- 
2.39.2





^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pve-devel] [PATCH proxmox 1/2] notify: smtp: forward original message instead nesting
  2023-12-13 16:42 [pve-devel] [PATCH proxmox 1/2] notify: smtp: forward original message instead nesting Lukas Wagner
  2023-12-13 16:42 ` [pve-devel] [PATCH proxmox 2/2] notify: smtp: add `Auto-Submitted` header to email body Lukas Wagner
@ 2024-01-08 10:43 ` Lukas Wagner
  2024-01-09 13:26 ` Wolfgang Bumiller
  2 siblings, 0 replies; 5+ messages in thread
From: Lukas Wagner @ 2024-01-08 10:43 UTC (permalink / raw)
  To: pve-devel

ping

On 12/13/23 17:42, Lukas Wagner wrote:
> For mails forwarded by `proxmox-mail-forward` to an SMTP target, the
> original message was nested as a 'message/rfc822' message part.
> Originally this approach was chosen to avoid having to rewrite
> message headers.
> Good email-clients, such as Thunderbird can display these inline.
> Other, more limited clients will show these messages as an attached
> .eml file, which is not really a good user experience.
> 
> This patch changes the approach for message forwarding to be more like
> forwarding mails in a mail client. We create a new message and
> add the original message body as a body. Additionally, we also copy
> over all message headers that are relevant to correctly display the
> original message body (e.g. Content-Type, Content-Transfer-Encoding)
> 
> Tested with a couple of different email messages (varying in
> structure, body parts, encoding, etc.) against the following SMTP
> relays:
>    - gmail
>    - outlook
>    - our own webmail service
> 
> Originally reported in our community forum:
> https://forum.proxmox.com/threads/proxmox-mail-forward-sends-mails-as-eml.137710/
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> 
> Notes:
>      proxmox-mail-forward needs a bump once this is applied.
> 
>   proxmox-notify/src/endpoints/smtp.rs | 99 ++++++++++++++++++++++++----
>   1 file changed, 88 insertions(+), 11 deletions(-)
> 
> diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
> index 064c9f9..4b8ff2d 100644
> --- a/proxmox-notify/src/endpoints/smtp.rs
> +++ b/proxmox-notify/src/endpoints/smtp.rs
> @@ -1,8 +1,9 @@
> +use std::time::Duration;
> +
>   use lettre::message::{Mailbox, MultiPart, SinglePart};
>   use lettre::transport::smtp::client::{Tls, TlsParameters};
>   use lettre::{message::header::ContentType, Message, SmtpTransport, Transport};
>   use serde::{Deserialize, Serialize};
> -use std::time::Duration;
>   
>   use proxmox_schema::api_types::COMMENT_SCHEMA;
>   use proxmox_schema::{api, Updater};
> @@ -231,17 +232,93 @@ impl Endpoint for SmtpEndpoint {
>               }
>               #[cfg(feature = "mail-forwarder")]
>               Content::ForwardedMail { ref raw, title, .. } => {
> -                email_builder = email_builder.subject(title);
> +                use lettre::message::header::{ContentTransferEncoding, HeaderName, HeaderValue};
> +                use lettre::message::Body;
>   
> -                // Forwarded messages are embedded inline as 'message/rfc822'
> -                // this let's us avoid rewriting any headers (e.g. From)
> -                email_builder
> -                    .singlepart(
> -                        SinglePart::builder()
> -                            .header(ContentType::parse("message/rfc822").unwrap())
> -                            .body(raw.to_owned()),
> -                    )
> -                    .map_err(|err| Error::NotifyFailed(self.name().into(), Box::new(err)))?
> +                let parsed_message = mail_parser::Message::parse(raw)
> +                    .ok_or_else(|| Error::Generic("could not parse forwarded email".to_string()))?;
> +
> +                let root_part = parsed_message
> +                    .part(0)
> +                    .ok_or_else(|| Error::Generic("root message part not present".to_string()))?;
> +
> +                let raw_body = parsed_message
> +                    .raw_message()
> +                    .get(root_part.offset_body..root_part.offset_end)
> +                    .ok_or_else(|| Error::Generic("could not get raw body content".to_string()))?;
> +
> +                // We assume that the original message content is already properly
> +                // encoded, thus we add the original message body 'Binary' encoding.
> +                // This prohibits lettre from trying to re-encode our raw body data.
> +                // lettre will automatically set the `Content-Transfer-Encoding: binary` header,
> +                // which we need to remove. The actual transfer encoding is later
> +                // copied from the original message headers.
> +                let body =
> +                    Body::new_with_encoding(raw_body.to_vec(), ContentTransferEncoding::Binary)
> +                        .map_err(|_| Error::Generic("could not create body".into()))?;
> +                let mut message = email_builder
> +                    .subject(title)
> +                    .body(body)
> +                    .map_err(|err| Error::NotifyFailed(self.name().into(), Box::new(err)))?;
> +                message
> +                    .headers_mut()
> +                    .remove_raw("Content-Transfer-Encoding");
> +
> +                // Copy over all headers that are relevant to display the original body correctly.
> +                // Unfortunately this is a bit cumbersome, as we use separate crates for mail parsing (mail-parser)
> +                // and creating/sending mails (lettre).
> +                // Note: Other MIME-Headers, such as Content-{ID,Description,Disposition} are only used
> +                // for body-parts in multipart messages, so we can ignore them for the messages headers.
> +                // Since we send the original raw body, the part-headers will be included any way.
> +                for header in parsed_message.headers() {
> +                    let header_name = header.name.as_str();
> +                    // Email headers are case-insensitive, so convert to lowercase...
> +                    let value = match header_name.to_lowercase().as_str() {
> +                        "content-type" => {
> +                            if let mail_parser::HeaderValue::ContentType(ct) = header.value() {
> +                                // mail_parser does not give us access to the full decoded and unfolded
> +                                // header value, so we unfortunately need to reassemble it ourselves.
> +                                // Meh.
> +                                let mut value = ct.ctype().to_string();
> +                                if let Some(subtype) = ct.subtype() {
> +                                    value.push('/');
> +                                    value.push_str(subtype);
> +                                }
> +                                if let Some(attributes) = ct.attributes() {
> +                                    for attribute in attributes {
> +                                        value.push_str(&format!(
> +                                            "; {}=\"{}\"",
> +                                            attribute.0, attribute.1
> +                                        ));
> +                                    }
> +                                }
> +                                Some(value)
> +                            } else {
> +                                None
> +                            }
> +                        }
> +                        "content-transfer-encoding" | "mime-version" => {
> +                            if let mail_parser::HeaderValue::Text(text) = header.value() {
> +                                Some(text.to_string())
> +                            } else {
> +                                None
> +                            }
> +                        }
> +                        _ => None,
> +                    };
> +
> +                    if let Some(value) = value {
> +                        match HeaderName::new_from_ascii(header_name.into()) {
> +                            Ok(name) => {
> +                                let header = HeaderValue::new(name, value);
> +                                message.headers_mut().insert_raw(header);
> +                            }
> +                            Err(e) => log::error!("could not set header: {e}"),
> +                        }
> +                    }
> +                }
> +
> +                message
>               }
>           };
>   

-- 
- Lukas




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pve-devel] [PATCH proxmox 1/2] notify: smtp: forward original message instead nesting
  2023-12-13 16:42 [pve-devel] [PATCH proxmox 1/2] notify: smtp: forward original message instead nesting Lukas Wagner
  2023-12-13 16:42 ` [pve-devel] [PATCH proxmox 2/2] notify: smtp: add `Auto-Submitted` header to email body Lukas Wagner
  2024-01-08 10:43 ` [pve-devel] [PATCH proxmox 1/2] notify: smtp: forward original message instead nesting Lukas Wagner
@ 2024-01-09 13:26 ` Wolfgang Bumiller
  2024-01-10  8:28   ` Lukas Wagner
  2 siblings, 1 reply; 5+ messages in thread
From: Wolfgang Bumiller @ 2024-01-09 13:26 UTC (permalink / raw)
  To: Lukas Wagner; +Cc: pve-devel

On Wed, Dec 13, 2023 at 05:42:00PM +0100, Lukas Wagner wrote:
> For mails forwarded by `proxmox-mail-forward` to an SMTP target, the
> original message was nested as a 'message/rfc822' message part.
> Originally this approach was chosen to avoid having to rewrite
> message headers.
> Good email-clients, such as Thunderbird can display these inline.
> Other, more limited clients will show these messages as an attached
> .eml file, which is not really a good user experience.
> 
> This patch changes the approach for message forwarding to be more like
> forwarding mails in a mail client. We create a new message and
> add the original message body as a body. Additionally, we also copy
> over all message headers that are relevant to correctly display the
> original message body (e.g. Content-Type, Content-Transfer-Encoding)
> 
> Tested with a couple of different email messages (varying in
> structure, body parts, encoding, etc.) against the following SMTP
> relays:
>   - gmail
>   - outlook
>   - our own webmail service
> 
> Originally reported in our community forum:
> https://forum.proxmox.com/threads/proxmox-mail-forward-sends-mails-as-eml.137710/
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> 
> Notes:
>     proxmox-mail-forward needs a bump once this is applied.
> 
>  proxmox-notify/src/endpoints/smtp.rs | 99 ++++++++++++++++++++++++----
>  1 file changed, 88 insertions(+), 11 deletions(-)
> 
> diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
> index 064c9f9..4b8ff2d 100644
> --- a/proxmox-notify/src/endpoints/smtp.rs
> +++ b/proxmox-notify/src/endpoints/smtp.rs
> @@ -1,8 +1,9 @@
> +use std::time::Duration;
> +
>  use lettre::message::{Mailbox, MultiPart, SinglePart};
>  use lettre::transport::smtp::client::{Tls, TlsParameters};
>  use lettre::{message::header::ContentType, Message, SmtpTransport, Transport};
>  use serde::{Deserialize, Serialize};
> -use std::time::Duration;
>  
>  use proxmox_schema::api_types::COMMENT_SCHEMA;
>  use proxmox_schema::{api, Updater};
> @@ -231,17 +232,93 @@ impl Endpoint for SmtpEndpoint {
>              }
>              #[cfg(feature = "mail-forwarder")]
>              Content::ForwardedMail { ref raw, title, .. } => {
> -                email_builder = email_builder.subject(title);
> +                use lettre::message::header::{ContentTransferEncoding, HeaderName, HeaderValue};
> +                use lettre::message::Body;
>  
> -                // Forwarded messages are embedded inline as 'message/rfc822'
> -                // this let's us avoid rewriting any headers (e.g. From)
> -                email_builder
> -                    .singlepart(
> -                        SinglePart::builder()
> -                            .header(ContentType::parse("message/rfc822").unwrap())
> -                            .body(raw.to_owned()),
> -                    )
> -                    .map_err(|err| Error::NotifyFailed(self.name().into(), Box::new(err)))?
> +                let parsed_message = mail_parser::Message::parse(raw)
> +                    .ok_or_else(|| Error::Generic("could not parse forwarded email".to_string()))?;
> +
> +                let root_part = parsed_message
> +                    .part(0)
> +                    .ok_or_else(|| Error::Generic("root message part not present".to_string()))?;
> +
> +                let raw_body = parsed_message
> +                    .raw_message()
> +                    .get(root_part.offset_body..root_part.offset_end)
> +                    .ok_or_else(|| Error::Generic("could not get raw body content".to_string()))?;
> +
> +                // We assume that the original message content is already properly
> +                // encoded, thus we add the original message body 'Binary' encoding.
> +                // This prohibits lettre from trying to re-encode our raw body data.
> +                // lettre will automatically set the `Content-Transfer-Encoding: binary` header,
> +                // which we need to remove. The actual transfer encoding is later
> +                // copied from the original message headers.
> +                let body =
> +                    Body::new_with_encoding(raw_body.to_vec(), ContentTransferEncoding::Binary)
> +                        .map_err(|_| Error::Generic("could not create body".into()))?;
> +                let mut message = email_builder
> +                    .subject(title)
> +                    .body(body)
> +                    .map_err(|err| Error::NotifyFailed(self.name().into(), Box::new(err)))?;
> +                message
> +                    .headers_mut()
> +                    .remove_raw("Content-Transfer-Encoding");
> +
> +                // Copy over all headers that are relevant to display the original body correctly.
> +                // Unfortunately this is a bit cumbersome, as we use separate crates for mail parsing (mail-parser)
> +                // and creating/sending mails (lettre).
> +                // Note: Other MIME-Headers, such as Content-{ID,Description,Disposition} are only used
> +                // for body-parts in multipart messages, so we can ignore them for the messages headers.
> +                // Since we send the original raw body, the part-headers will be included any way.
> +                for header in parsed_message.headers() {
> +                    let header_name = header.name.as_str();
> +                    // Email headers are case-insensitive, so convert to lowercase...
> +                    let value = match header_name.to_lowercase().as_str() {
> +                        "content-type" => {
> +                            if let mail_parser::HeaderValue::ContentType(ct) = header.value() {
> +                                // mail_parser does not give us access to the full decoded and unfolded
> +                                // header value, so we unfortunately need to reassemble it ourselves.
> +                                // Meh.

urgh

> +                                let mut value = ct.ctype().to_string();
> +                                if let Some(subtype) = ct.subtype() {
> +                                    value.push('/');
> +                                    value.push_str(subtype);
> +                                }
> +                                if let Some(attributes) = ct.attributes() {
> +                                    for attribute in attributes {
> +                                        value.push_str(&format!(
> +                                            "; {}=\"{}\"",
> +                                            attribute.0, attribute.1
> +                                        ));

should be more efficient to use
let _ = write!(value, "...");
instead of `push_str()` on a temporarily allocated (format!()) string.

> +                                    }
> +                                }
> +                                Some(value)
> +                            } else {
> +                                None
> +                            }
> +                        }
> +                        "content-transfer-encoding" | "mime-version" => {
> +                            if let mail_parser::HeaderValue::Text(text) = header.value() {
> +                                Some(text.to_string())
> +                            } else {
> +                                None
> +                            }
> +                        }
> +                        _ => None,
> +                    };
> +
> +                    if let Some(value) = value {
> +                        match HeaderName::new_from_ascii(header_name.into()) {
> +                            Ok(name) => {
> +                                let header = HeaderValue::new(name, value);
> +                                message.headers_mut().insert_raw(header);
> +                            }
> +                            Err(e) => log::error!("could not set header: {e}"),
> +                        }
> +                    }
> +                }
> +
> +                message
>              }
>          };
>  
> -- 
> 2.39.2




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pve-devel] [PATCH proxmox 1/2] notify: smtp: forward original message instead nesting
  2024-01-09 13:26 ` Wolfgang Bumiller
@ 2024-01-10  8:28   ` Lukas Wagner
  0 siblings, 0 replies; 5+ messages in thread
From: Lukas Wagner @ 2024-01-10  8:28 UTC (permalink / raw)
  To: Wolfgang Bumiller; +Cc: pve-devel

Thanks for the review!

On 1/9/24 14:26, Wolfgang Bumiller wrote:
> On Wed, Dec 13, 2023 at 05:42:00PM +0100, Lukas Wagner wrote:
>> For mails forwarded by `proxmox-mail-forward` to an SMTP target, the
>> original message was nested as a 'message/rfc822' message part.
>> Originally this approach was chosen to avoid having to rewrite
>> message headers.
>> Good email-clients, such as Thunderbird can display these inline.
>> Other, more limited clients will show these messages as an attached
>> .eml file, which is not really a good user experience.
>>
>> This patch changes the approach for message forwarding to be more like
>> forwarding mails in a mail client. We create a new message and
>> add the original message body as a body. Additionally, we also copy
>> over all message headers that are relevant to correctly display the
>> original message body (e.g. Content-Type, Content-Transfer-Encoding)
>>
>> Tested with a couple of different email messages (varying in
>> structure, body parts, encoding, etc.) against the following SMTP
>> relays:
>>    - gmail
>>    - outlook
>>    - our own webmail service
>>
>> Originally reported in our community forum:
>> https://forum.proxmox.com/threads/proxmox-mail-forward-sends-mails-as-eml.137710/
>>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> ---
>>
>> Notes:
>>      proxmox-mail-forward needs a bump once this is applied.
>>
>>   proxmox-notify/src/endpoints/smtp.rs | 99 ++++++++++++++++++++++++----
>>   1 file changed, 88 insertions(+), 11 deletions(-)
>>
>> diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
>> index 064c9f9..4b8ff2d 100644
>> --- a/proxmox-notify/src/endpoints/smtp.rs
>> +++ b/proxmox-notify/src/endpoints/smtp.rs
>> @@ -1,8 +1,9 @@
>> +use std::time::Duration;
>> +
>>   use lettre::message::{Mailbox, MultiPart, SinglePart};
>>   use lettre::transport::smtp::client::{Tls, TlsParameters};
>>   use lettre::{message::header::ContentType, Message, SmtpTransport, Transport};
>>   use serde::{Deserialize, Serialize};
>> -use std::time::Duration;
>>   
>>   use proxmox_schema::api_types::COMMENT_SCHEMA;
>>   use proxmox_schema::{api, Updater};
>> @@ -231,17 +232,93 @@ impl Endpoint for SmtpEndpoint {
>>               }
>>               #[cfg(feature = "mail-forwarder")]
>>               Content::ForwardedMail { ref raw, title, .. } => {
>> -                email_builder = email_builder.subject(title);
>> +                use lettre::message::header::{ContentTransferEncoding, HeaderName, HeaderValue};
>> +                use lettre::message::Body;
>>   
>> -                // Forwarded messages are embedded inline as 'message/rfc822'
>> -                // this let's us avoid rewriting any headers (e.g. From)
>> -                email_builder
>> -                    .singlepart(
>> -                        SinglePart::builder()
>> -                            .header(ContentType::parse("message/rfc822").unwrap())
>> -                            .body(raw.to_owned()),
>> -                    )
>> -                    .map_err(|err| Error::NotifyFailed(self.name().into(), Box::new(err)))?
>> +                let parsed_message = mail_parser::Message::parse(raw)
>> +                    .ok_or_else(|| Error::Generic("could not parse forwarded email".to_string()))?;
>> +
>> +                let root_part = parsed_message
>> +                    .part(0)
>> +                    .ok_or_else(|| Error::Generic("root message part not present".to_string()))?;
>> +
>> +                let raw_body = parsed_message
>> +                    .raw_message()
>> +                    .get(root_part.offset_body..root_part.offset_end)
>> +                    .ok_or_else(|| Error::Generic("could not get raw body content".to_string()))?;
>> +
>> +                // We assume that the original message content is already properly
>> +                // encoded, thus we add the original message body 'Binary' encoding.
>> +                // This prohibits lettre from trying to re-encode our raw body data.
>> +                // lettre will automatically set the `Content-Transfer-Encoding: binary` header,
>> +                // which we need to remove. The actual transfer encoding is later
>> +                // copied from the original message headers.
>> +                let body =
>> +                    Body::new_with_encoding(raw_body.to_vec(), ContentTransferEncoding::Binary)
>> +                        .map_err(|_| Error::Generic("could not create body".into()))?;
>> +                let mut message = email_builder
>> +                    .subject(title)
>> +                    .body(body)
>> +                    .map_err(|err| Error::NotifyFailed(self.name().into(), Box::new(err)))?;
>> +                message
>> +                    .headers_mut()
>> +                    .remove_raw("Content-Transfer-Encoding");
>> +
>> +                // Copy over all headers that are relevant to display the original body correctly.
>> +                // Unfortunately this is a bit cumbersome, as we use separate crates for mail parsing (mail-parser)
>> +                // and creating/sending mails (lettre).
>> +                // Note: Other MIME-Headers, such as Content-{ID,Description,Disposition} are only used
>> +                // for body-parts in multipart messages, so we can ignore them for the messages headers.
>> +                // Since we send the original raw body, the part-headers will be included any way.
>> +                for header in parsed_message.headers() {
>> +                    let header_name = header.name.as_str();
>> +                    // Email headers are case-insensitive, so convert to lowercase...
>> +                    let value = match header_name.to_lowercase().as_str() {
>> +                        "content-type" => {
>> +                            if let mail_parser::HeaderValue::ContentType(ct) = header.value() {
>> +                                // mail_parser does not give us access to the full decoded and unfolded
>> +                                // header value, so we unfortunately need to reassemble it ourselves.
>> +                                // Meh.
> 
> urgh
> 

urgh indeed...
>> +                                let mut value = ct.ctype().to_string();
>> +                                if let Some(subtype) = ct.subtype() {
>> +                                    value.push('/');
>> +                                    value.push_str(subtype);
>> +                                }
>> +                                if let Some(attributes) = ct.attributes() {
>> +                                    for attribute in attributes {
>> +                                        value.push_str(&format!(
>> +                                            "; {}=\"{}\"",
>> +                                            attribute.0, attribute.1
>> +                                        ));
> 
> should be more efficient to use
> let _ = write!(value, "...");
> instead of `push_str()` on a temporarily allocated (format!()) string.

Right, thanks! Will send a v2.
> 
>> +                                    }
>> +                                }
>> +                                Some(value)
>> +                            } else {
>> +                                None
>> +                            }
>> +                        }
>> +                        "content-transfer-encoding" | "mime-version" => {
>> +                            if let mail_parser::HeaderValue::Text(text) = header.value() {
>> +                                Some(text.to_string())
>> +                            } else {
>> +                                None
>> +                            }
>> +                        }
>> +                        _ => None,
>> +                    };
>> +
>> +                    if let Some(value) = value {
>> +                        match HeaderName::new_from_ascii(header_name.into()) {
>> +                            Ok(name) => {
>> +                                let header = HeaderValue::new(name, value);
>> +                                message.headers_mut().insert_raw(header);
>> +                            }
>> +                            Err(e) => log::error!("could not set header: {e}"),
>> +                        }
>> +                    }
>> +                }
>> +
>> +                message
>>               }
>>           };
>>   
>> -- 
>> 2.39.2

-- 
- Lukas




^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-01-10  8:28 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-13 16:42 [pve-devel] [PATCH proxmox 1/2] notify: smtp: forward original message instead nesting Lukas Wagner
2023-12-13 16:42 ` [pve-devel] [PATCH proxmox 2/2] notify: smtp: add `Auto-Submitted` header to email body Lukas Wagner
2024-01-08 10:43 ` [pve-devel] [PATCH proxmox 1/2] notify: smtp: forward original message instead nesting Lukas Wagner
2024-01-09 13:26 ` Wolfgang Bumiller
2024-01-10  8:28   ` Lukas Wagner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal