feat: Improved SMTP error responses (#147)

Fixes: #140

Signed-off-by: Jagoda Ślązak <jslazak@jslazak.com>

filtermail/src/error.rs

@@ -14,10 +14,11 @@ pub enum Error {
     Resolve(#[from] hickory_resolver::net::NetError),
     #[error("OpenPGP packet header is truncated - can't validate!")]
     TruncatedHeader,
-    #[error("Unable to send email, Error during {context}, server said: {raw_smtp_answer}")]
+    #[error("Unable to send email, Error during {context}, host {host} said: {raw_smtp_answer}")]
     MailSend {
         context: String,
         raw_smtp_answer: String,
+        host: String,
     },
     #[error("Invalid email address: {0}")]
     InvalidEmailAddress(String),
@@ -38,14 +39,36 @@ pub enum Error {
 impl Error {
     /// Formats [`Error`] as an SMTP response.
     pub fn smtp_response(&self) -> String {
+        macro_rules! format_smtp {
+            ($code:expr) => {
+                format!("{} {}", $code, self.to_string())
+            };
+        }
+
         match self {
-            // We transparently pass postfix/milter errors reported on reinjection
+            // Errors returned by server we connect to are forwarded.
+            // We add "(forwarded from ...)" to distinguish these from our local errors.
             Error::MailSend {
-                raw_smtp_answer, ..
-            } => raw_smtp_answer.clone(),
-            Error::TruncatedHeader => self.to_string(),
-            Error::InvalidEmailAddress(address) => format!("500 Invalid email address: {address}"),
-            _ => "451 Local error".to_string(),
+                raw_smtp_answer,
+                host,
+                ..
+            } => format!("{raw_smtp_answer} (forwarded from {host})"),
+
+            // Permanent errors
+            Error::TruncatedHeader => format_smtp!("554"),
+            Error::InvalidEmailAddress(_) => format_smtp!("553"),
+            Error::InvalidDnsName(_) => format_smtp!("501"),
+
+            // Transient errors
+            Error::ConnectionFailed(_) => format_smtp!("450"),
+            // We don't want to leak chatmail.ini config and other local error details.
+            Error::Config(_) => "451 Filtermail misconfigured; contact admin".to_string(),
+            Error::Io(_) => "451 I/O error".to_string(),
+            Error::Tls(_) => "451 TLS error".to_string(),
+            Error::Resolve(_) => "451 Resolver error".to_string(),
+            Error::Hyper(_) | Error::HyperHttp(_) | Error::HyperClient(_) => {
+                "451 HTTP error".to_string()
+            }
         }
     }
 

filtermail/src/smtp_client.rs

@@ -84,6 +84,41 @@ impl SmtpStream {
         timeout_stream.set_read_timeout(Some(Duration::from_secs(60)));
         Self::Plain(Box::pin(timeout_stream))
     }
+
+    /// Returns the peer address of the underlying TCP stream.
+    pub fn peer_addr(&self) -> std::io::Result<std::net::SocketAddr> {
+        match self {
+            SmtpStream::Plain(stream) => stream.get_ref().peer_addr(),
+            SmtpStream::Tls(stream) => stream.get_ref().0.get_ref().peer_addr(),
+        }
+    }
+
+    /// Formats a peer host, including underlying TCP connection's socket address.
+    ///
+    /// Returns either:
+    /// - `<ip>:<port>` if `address` is an IP matching underlying TCP connection.
+    /// - `<address>[<ip>:<port>]` otherwise.
+    ///
+    /// `<ip>` is either `<ipv4>` or `[<ipv6>]`.
+    ///
+    /// Infallible, fallbacks to `<address>[?:?]` if peer address is unavailable.
+    fn format_host(&self, address: &str) -> String {
+        let socket_addr = self.peer_addr().ok();
+        Self::format_host_inner(address, socket_addr)
+    }
+
+    /// Internal logic of [`SmtpStream::format_host`], only for testing purposes.
+    fn format_host_inner(address: &str, socket_addr: Option<std::net::SocketAddr>) -> String {
+        let socket_addr_str = if let Some(socket_addr) = socket_addr {
+            if socket_addr.ip().to_string().eq_ignore_ascii_case(address) {
+                return socket_addr.to_string();
+            }
+            socket_addr.to_string()
+        } else {
+            "?:?".to_string()
+        };
+        format!("{address}[{socket_addr_str}]")
+    }
 }
 
 #[derive(Debug, Clone)]
@@ -210,23 +245,28 @@ pub async fn send(
     dns_resolver: Arc<TokioResolver>,
     pool: Arc<SmtpConnectionPool>,
 ) -> Result<(), crate::error::Error> {
-    let (mut buf_stream, reused, mut pipelining) =
-        if let Some(connection) = pool.take(address, port).await {
-            log::debug!("Reusing existing connection to {address}:{port}",);
-            if tls_config.is_some() {
-                // This should never happen,
-                // assert to make sure we never accidentally use a plain connection while expecting TLS.
-                assert!(
-                    matches!(connection.stream.get_ref(), SmtpStream::Tls(_)),
-                    "Expected TLS stream from pool, but got plain stream."
-                );
-            }
-            (connection.stream, true, connection.pipelining)
-        } else {
-            let stream = establish_tcp_connection(address, port, dns_resolver.clone()).await?;
-            log::debug!("Successfully connected to {}", stream.peer_addr()?);
-            (BufStream::new(SmtpStream::plain(stream)), false, false)
-        };
+    let (mut buf_stream, reused, mut pipelining) = if let Some(connection) =
+        pool.take(address, port).await
+    {
+        log::debug!(
+            "Reusing existing connection to {}",
+            connection.stream.get_ref().format_host(address)
+        );
+        if tls_config.is_some() {
+            // This should never happen,
+            // assert to make sure we never accidentally use a plain connection while expecting TLS.
+            assert!(
+                matches!(connection.stream.get_ref(), SmtpStream::Tls(_)),
+                "Expected TLS stream from pool, but got plain stream."
+            );
+        }
+        (connection.stream, true, connection.pipelining)
+    } else {
+        let stream =
+            SmtpStream::plain(establish_tcp_connection(address, port, dns_resolver.clone()).await?);
+        log::debug!("Successfully connected to {}", stream.format_host(address));
+        (BufStream::new(stream), false, false)
+    };
 
     let mut response = String::new();
 
@@ -265,6 +305,7 @@ pub async fn send(
                 Err(crate::error::Error::MailSend {
                     context: $context.to_string(),
                     raw_smtp_answer: response.clone(),
+                    host: buf_stream.get_ref().format_host(address),
                 })
             } else {
                 Ok(())
@@ -323,6 +364,7 @@ pub async fn send(
                 return Err(crate::error::Error::MailSend {
                     context: "STARTTLS".to_string(),
                     raw_smtp_answer: response.clone(),
+                    host: buf_stream.get_ref().format_host(address),
                 });
             }
 
@@ -401,11 +443,12 @@ pub async fn send(
             // > but if the DATA command was accepted the client SMTP should send a single dot.
             if data_354 {
                 log::warn!(
-                    "Server {address} advertised PIPELINING support, \
+                    "Server {} advertised PIPELINING support, \
                     but accepted DATA despite error response to at least one \
                     previous command in the group: \n\
                     {e} \n\
-                    Sending a single dot (RFC2920 section 3.1)."
+                    Sending a single dot (RFC2920 section 3.1).",
+                    buf_stream.get_ref().format_host(address)
                 );
                 smtp_cmd!(b".\r\n", "end of DATA", "250")?;
             }
@@ -428,3 +471,25 @@ pub async fn send(
 
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use rstest::rstest;
+    use std::net::SocketAddr;
+
+    #[rstest]
+    #[case::ipv4("192.0.2.0:25".parse().ok(), "192.0.2.0", "192.0.2.0:25")]
+    #[case::ipv6("[2001:db8::1]:25".parse().ok(), "2001:db8::1", "[2001:db8::1]:25")]
+    #[case::domain_ipv4("192.0.2.0:25".parse().ok(), "example.org", "example.org[192.0.2.0:25]")]
+    #[case::domain_ipv6("[2001:db8::1]:25".parse().ok(), "example.org", "example.org[[2001:db8::1]:25]")]
+    #[case::unknown(None, "example.org", "example.org[?:?]")]
+    fn test_format_host_inner(
+        #[case] socket_addr: Option<SocketAddr>,
+        #[case] host: &str,
+        #[case] expected: &str,
+    ) {
+        let result = SmtpStream::format_host_inner(host, socket_addr);
+        assert_eq!(result, expected);
+    }
+}

filtermail/src/transport.rs

@@ -189,6 +189,8 @@ impl TransportHandler {
             }),
         };
 
+        let mut last_error = None;
+
         // we try sequentially in order of MX preference,
         // but the IPv4 and IPv6 connections (after `smtp_client::send` resolves mx hostname)
         // happens in parallel.
@@ -243,7 +245,7 @@ impl TransportHandler {
                     return Ok("250 Ok (SMTP)".to_string());
                 }
                 Err(error) => {
-                    match error {
+                    match &error {
                         // We only want to try other MX hosts if we encounter a problem
                         // related to connection.
                         // (So we don't spam other servers if the message is actually rejected.)
@@ -255,18 +257,31 @@ impl TransportHandler {
                             log::warn!(
                                 "Connection error relaying to mail server {mx_host}: {error}"
                             );
+                            last_error = Some((error.smtp_response(), mx_host.clone()));
                             continue 'try_relay;
                         }
-                        _ => {
+                        crate::error::Error::MailSend { .. } => {
                             log::warn!("Message rejected by mail server {mx_host}: {error}");
                             return Err(error.smtp_response());
                         }
+                        _ => {
+                            log::warn!(
+                                "Unexpected error while delivering to mail server {mx_host}: {error}"
+                            );
+                            return Err(format!(
+                                "{} (while attempting delivery to {mx_host})",
+                                error.smtp_response()
+                            ));
+                        }
                     }
                 }
             }
         }
 
-        Err("421 Failed to connect to any mail server".to_string())
+        let (error, mx_host) = last_error.unwrap_or(("?".to_string(), "?".to_string()));
+        Err(format!(
+            "421 Failed to connect to any mail server; last attempt to {mx_host}: {error}"
+        ))
     }
 
     /// Performs mail delivery to `mx_host` over HTTPS.
@@ -305,6 +320,7 @@ impl TransportHandler {
             .map_err(|_| crate::error::Error::MailSend {
                 context: "HTTPS delivery".to_string(),
                 raw_smtp_answer: "[timeout]".to_string(),
+                host: mx_host.clone(),
             })??;
         if response.status().is_success() {
             Ok(())
@@ -313,6 +329,7 @@ impl TransportHandler {
             Err(crate::error::Error::MailSend {
                 context: "HTTPS delivery".to_string(),
                 raw_smtp_answer: String::from_utf8_lossy(&response_body).into(),
+                host: mx_host,
             })
         }
     }