fix(transport-smtp): Use 127.0.0.1 literal as EHLO parameter when we have no hostname

Also fix formatting of address literals

Comes from 2275fd8d13
with a different approach for default value.

CHANGELOG.md

@@ -31,6 +31,9 @@ Several breaking changes were made between 0.9 and 0.10, but changes should be s
 * Change website url schemes to https ([6014f5c](https://github.com/lettre/lettre/commit/6014f5c))
 * Use serde's `derive` feature instead of the `serde_derive` crate ([4fbe700](https://github.com/lettre/lettre/commit/4fbe700))
 * Merge `Email` and `SendableEmail` into `lettre::Email` ([ce37464](https://github.com/lettre/lettre/commit/ce37464))
+* When the hostname feature is disabled or hostname cannot be fetched, `127.0.0.1` is used instead of `localhost` as
+  EHLO parameter (for better RFC compliance and mail server compatibility)
+* The `new` method of `ClientId` is renamed to `new_domain`
 
 #### Bug Fixes
 

src/transport/smtp/client/async_connection.rs

@@ -1,5 +1,4 @@
-use std::fmt::Display;
-use std::io;
+use std::{fmt::Display, io};
 
 use futures_util::io::{AsyncBufReadExt, AsyncWriteExt, BufReader};
 
@@ -7,14 +6,16 @@ use futures_util::io::{AsyncBufReadExt, AsyncWriteExt, BufReader};
 use log::debug;
 
 use super::{AsyncNetworkStream, ClientCodec, TlsParameters};
-use crate::transport::smtp::authentication::{Credentials, Mechanism};
-use crate::transport::smtp::commands::*;
-use crate::transport::smtp::error::Error;
-use crate::transport::smtp::extension::{
-    ClientId, Extension, MailBodyParameter, MailParameter, ServerInfo,
+use crate::{
+    transport::smtp::{
+        authentication::{Credentials, Mechanism},
+        commands::*,
+        error::Error,
+        extension::{ClientId, Extension, MailBodyParameter, MailParameter, ServerInfo},
+        response::{parse_response, Response},
+    },
+    Envelope,
 };
-use crate::transport::smtp::response::{parse_response, Response};
-use crate::Envelope;
 
 #[cfg(feature = "log")]
 use super::escape_crlf;
@@ -144,11 +145,7 @@ impl AsyncSmtpConnection {
 
     /// Send EHLO and update server info
     async fn ehlo(&mut self, hello_name: &ClientId) -> Result<(), Error> {
-        let ehlo_response = try_smtp!(
-            self.command(Ehlo::new(ClientId::new(hello_name.to_string())))
-                .await,
-            self
-        );
+        let ehlo_response = try_smtp!(self.command(Ehlo::new(hello_name.clone())).await, self);
         self.server_info = try_smtp!(ServerInfo::from_response(&ehlo_response), self);
         Ok(())
     }

src/transport/smtp/client/connection.rs

@@ -1,20 +1,24 @@
-use std::fmt::Display;
-use std::io::{self, BufRead, BufReader, Write};
-use std::net::ToSocketAddrs;
-use std::time::Duration;
+use std::{
+    fmt::Display,
+    io::{self, BufRead, BufReader, Write},
+    net::ToSocketAddrs,
+    time::Duration,
+};
 
 #[cfg(feature = "log")]
 use log::debug;
 
 use super::{ClientCodec, NetworkStream, TlsParameters};
-use crate::transport::smtp::authentication::{Credentials, Mechanism};
-use crate::transport::smtp::commands::*;
-use crate::transport::smtp::error::Error;
-use crate::transport::smtp::extension::{
-    ClientId, Extension, MailBodyParameter, MailParameter, ServerInfo,
+use crate::{
+    transport::smtp::{
+        authentication::{Credentials, Mechanism},
+        commands::*,
+        error::Error,
+        extension::{ClientId, Extension, MailBodyParameter, MailParameter, ServerInfo},
+        response::{parse_response, Response},
+    },
+    Envelope,
 };
-use crate::transport::smtp::response::{parse_response, Response};
-use crate::Envelope;
 
 #[cfg(feature = "log")]
 use super::escape_crlf;
@@ -138,10 +142,7 @@ impl SmtpConnection {
 
     /// Send EHLO and update server info
     fn ehlo(&mut self, hello_name: &ClientId) -> Result<(), Error> {
-        let ehlo_response = try_smtp!(
-            self.command(Ehlo::new(ClientId::new(hello_name.to_string()))),
-            self
-        );
+        let ehlo_response = try_smtp!(self.command(Ehlo::new(hello_name.clone())), self);
         self.server_info = try_smtp!(ServerInfo::from_response(&ehlo_response), self);
         Ok(())
     }

src/transport/smtp/client/mod.rs

@@ -7,12 +7,14 @@ use std::fmt::Debug;
 pub(crate) use self::async_connection::AsyncSmtpConnection;
 #[cfg(feature = "tokio02")]
 pub(crate) use self::async_net::AsyncNetworkStream;
-pub use self::connection::SmtpConnection;
-pub use self::mock::MockStream;
 use self::net::NetworkStream;
 #[cfg(any(feature = "native-tls", feature = "rustls-tls"))]
 pub(super) use self::tls::InnerTlsParameters;
-pub use self::tls::{Tls, TlsParameters};
+pub use self::{
+    connection::SmtpConnection,
+    mock::MockStream,
+    tls::{Tls, TlsParameters},
+};
 
 #[cfg(feature = "tokio02")]
 mod async_connection;

src/transport/smtp/client/net.rs

@@ -1,8 +1,10 @@
-use std::io::{self, Read, Write};
-use std::net::{Ipv4Addr, Shutdown, SocketAddr, SocketAddrV4, TcpStream, ToSocketAddrs};
 #[cfg(feature = "rustls-tls")]
 use std::sync::Arc;
-use std::time::Duration;
+use std::{
+    io::{self, Read, Write},
+    net::{Ipv4Addr, Shutdown, SocketAddr, SocketAddrV4, TcpStream, ToSocketAddrs},
+    time::Duration,
+};
 
 #[cfg(feature = "native-tls")]
 use native_tls::TlsStream;

src/transport/smtp/extension.rs

@@ -10,9 +10,6 @@ use std::{
     result::Result,
 };
 
-/// Default client id
-const DEFAULT_DOMAIN_CLIENT_ID: &str = "localhost";
-
 /// Client identifier, the parameter to `EHLO`
 #[derive(PartialEq, Eq, Clone, Debug)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
@@ -25,35 +22,44 @@ pub enum ClientId {
     Ipv6(Ipv6Addr),
 }
 
+const LOCALHOST_CLIENT: ClientId = ClientId::Ipv4(Ipv4Addr::new(127, 0, 0, 1));
+
+impl Default for ClientId {
+    fn default() -> Self {
+        // https://tools.ietf.org/html/rfc5321#section-4.1.4
+        //
+        // The SMTP client MUST, if possible, ensure that the domain parameter
+        // to the EHLO command is a primary host name as specified for this
+        // command in Section 2.3.5.  If this is not possible (e.g., when the
+        // client's address is dynamically assigned and the client does not have
+        // an obvious name), an address literal SHOULD be substituted for the
+        // domain name.
+        #[cfg(feature = "hostname")]
+        {
+            hostname::get()
+                .ok()
+                .and_then(|s| s.into_string().map(Self::Domain).ok())
+                .unwrap_or(LOCALHOST_CLIENT)
+        }
+        #[cfg(not(feature = "hostname"))]
+        LOCALHOST_CLIENT
+    }
+}
+
 impl Display for ClientId {
     fn fmt(&self, f: &mut Formatter) -> fmt::Result {
         match *self {
-            ClientId::Domain(ref value) => f.write_str(value),
-            ClientId::Ipv4(ref value) => write!(f, "{}", value),
-            ClientId::Ipv6(ref value) => write!(f, "{}", value),
+            Self::Domain(ref value) => f.write_str(value),
+            Self::Ipv4(ref value) => write!(f, "[{}]", value),
+            Self::Ipv6(ref value) => write!(f, "[IPv6:{}]", value),
         }
     }
 }
 
 impl ClientId {
     /// Creates a new `ClientId` from a fully qualified domain name
-    pub fn new(domain: String) -> ClientId {
-        ClientId::Domain(domain)
-    }
-
-    /// Defines a `ClientId` with the current hostname, of `localhost` if hostname could not be
-    /// found
-    pub fn hostname() -> ClientId {
-        #[cfg(feature = "hostname")]
-        return ClientId::Domain(
-            hostname::get()
-                .ok()
-                .and_then(|s| s.into_string().ok())
-                .unwrap_or_else(|| DEFAULT_DOMAIN_CLIENT_ID.to_string()),
-        );
-
-        #[cfg(not(feature = "hostname"))]
-        return ClientId::Domain(DEFAULT_DOMAIN_CLIENT_ID.to_string());
+    pub fn new_domain(domain: String) -> Self {
+        Self::Domain(domain)
     }
 }
 
@@ -274,7 +280,7 @@ impl Display for RcptParameter {
 #[cfg(test)]
 mod test {
 
-    use super::{ClientId, Extension, ServerInfo};
+    use super::*;
     use crate::transport::smtp::{
         authentication::Mechanism,
         response::{Category, Code, Detail, Response, Severity},
@@ -284,9 +290,10 @@ mod test {
     #[test]
     fn test_clientid_fmt() {
         assert_eq!(
-            format!("{}", ClientId::new("test".to_string())),
+            format!("{}", ClientId::new_domain("test".to_string())),
             "test".to_string()
         );
+        assert_eq!(format!("{}", LOCALHOST_CLIENT), "[127.0.0.1]".to_string());
     }
 
     #[test]

src/transport/smtp/mod.rs

@@ -162,7 +162,7 @@
 //! # {
 //! use lettre::transport::smtp::{SMTP_PORT, extension::ClientId, commands::*, client::SmtpConnection};
 //!
-//! let hello = ClientId::new("my_hostname".to_string());
+//! let hello = ClientId::new_domain("my_hostname".to_string());
 //! let mut client = SmtpConnection::connect(&("localhost", SMTP_PORT), None, &hello, None).unwrap();
 //! client.command(
 //!         Mail::new(Some("user@example.com".parse().unwrap()), vec![])
@@ -249,7 +249,7 @@ impl Default for SmtpInfo {
         Self {
             server: "localhost".to_string(),
             port: SMTP_PORT,
-            hello_name: ClientId::hostname(),
+            hello_name: ClientId::default(),
             credentials: None,
             authentication: DEFAULT_MECHANISMS.into(),
             timeout: Some(DEFAULT_TIMEOUT),