From d74e5b85b3a7522e4ea39d3262b070b86f10ebb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jagoda=20Estera=20=C5=9Al=C4=85zak?= <128227338+j-g00da@users.noreply.github.com> Date: Tue, 12 May 2026 12:37:02 +0200 Subject: [PATCH] feat: Improved SMTP error responses (#147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: #140 Signed-off-by: Jagoda Ślązak --- filtermail/src/error.rs | 37 +++++++++--- filtermail/src/smtp_client.rs | 103 +++++++++++++++++++++++++++------- filtermail/src/transport.rs | 23 +++++++- 3 files changed, 134 insertions(+), 29 deletions(-) diff --git a/filtermail/src/error.rs b/filtermail/src/error.rs index 546881ea..873f7134 100644 --- a/filtermail/src/error.rs +++ b/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() + } } } diff --git a/filtermail/src/smtp_client.rs b/filtermail/src/smtp_client.rs index 852ddeda..2227942b 100644 --- a/filtermail/src/smtp_client.rs +++ b/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 { + 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: + /// - `:` if `address` is an IP matching underlying TCP connection. + /// - `
[:]` otherwise. + /// + /// `` is either `` or `[]`. + /// + /// Infallible, fallbacks to `
[?:?]` 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) -> 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, pool: Arc, ) -> 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, + #[case] host: &str, + #[case] expected: &str, + ) { + let result = SmtpStream::format_host_inner(host, socket_addr); + assert_eq!(result, expected); + } +} diff --git a/filtermail/src/transport.rs b/filtermail/src/transport.rs index ce3a17f9..cda9442e 100644 --- a/filtermail/src/transport.rs +++ b/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, }) } }