From ed20a23297a86624cc72b22ccbb1db32a2433c43 Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 19 Jan 2023 14:57:25 +0000 Subject: [PATCH 01/12] Resolve IP addresses explicitly --- src/imap.rs | 17 ++++++++++++----- src/imap/client.rs | 39 +++++++++++++++++++++++++++++---------- src/net.rs | 42 ++++++++++++++++++++++++++++++++++++------ src/socks.rs | 3 ++- 4 files changed, 79 insertions(+), 22 deletions(-) diff --git a/src/imap.rs b/src/imap.rs index 0d91da8f7..5eaf0fd6a 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -308,6 +308,7 @@ impl Imap { if let Some(socks5_config) = &config.socks5_config { if config.lp.security == Socket::Starttls { Client::connect_starttls_socks5( + context, imap_server, imap_port, socks5_config.clone(), @@ -315,13 +316,18 @@ impl Imap { ) .await } else { - Client::connect_insecure_socks5(imap_server, imap_port, socks5_config.clone()) - .await + Client::connect_insecure_socks5( + context, + imap_server, + imap_port, + socks5_config.clone(), + ) + .await } } else if config.lp.security == Socket::Starttls { - Client::connect_starttls(imap_server, imap_port, config.strict_tls).await + Client::connect_starttls(context, imap_server, imap_port, config.strict_tls).await } else { - Client::connect_insecure((imap_server, imap_port)).await + Client::connect_insecure(context, imap_server, imap_port).await } } else { let config = &self.config; @@ -330,6 +336,7 @@ impl Imap { if let Some(socks5_config) = &config.socks5_config { Client::connect_secure_socks5( + context, imap_server, imap_port, config.strict_tls, @@ -337,7 +344,7 @@ impl Imap { ) .await } else { - Client::connect_secure(imap_server, imap_port, config.strict_tls).await + Client::connect_secure(context, imap_server, imap_port, config.strict_tls).await } }; diff --git a/src/imap/client.rs b/src/imap/client.rs index aaada0f90..b6b106c45 100644 --- a/src/imap/client.rs +++ b/src/imap/client.rs @@ -7,11 +7,11 @@ use anyhow::{Context as _, Result}; use async_imap::Client as ImapClient; use async_imap::Session as ImapSession; use tokio::io::BufWriter; -use tokio::net::ToSocketAddrs; use super::capabilities::Capabilities; use super::session::Session; use super::session::SessionStream; +use crate::context::Context; use crate::login_param::build_tls; use crate::net::connect_tcp; use crate::socks::Socks5Config; @@ -88,8 +88,13 @@ impl Client { Ok(Session::new(session, capabilities)) } - pub async fn connect_secure(hostname: &str, port: u16, strict_tls: bool) -> Result { - let tcp_stream = connect_tcp((hostname, port), IMAP_TIMEOUT).await?; + pub async fn connect_secure( + context: &Context, + hostname: &str, + port: u16, + strict_tls: bool, + ) -> Result { + let tcp_stream = connect_tcp(context, hostname, port, IMAP_TIMEOUT).await?; let tls = build_tls(strict_tls); let tls_stream = tls.connect(hostname, tcp_stream).await?; let buffered_stream = BufWriter::new(tls_stream); @@ -104,8 +109,8 @@ impl Client { Ok(Client { inner: client }) } - pub async fn connect_insecure(addr: impl ToSocketAddrs) -> Result { - let tcp_stream = connect_tcp(addr, IMAP_TIMEOUT).await?; + pub async fn connect_insecure(context: &Context, hostname: &str, port: u16) -> Result { + let tcp_stream = connect_tcp(context, hostname, port, IMAP_TIMEOUT).await?; let buffered_stream = BufWriter::new(tcp_stream); let session_stream: Box = Box::new(buffered_stream); let mut client = ImapClient::new(session_stream); @@ -117,8 +122,13 @@ impl Client { Ok(Client { inner: client }) } - pub async fn connect_starttls(hostname: &str, port: u16, strict_tls: bool) -> Result { - let tcp_stream = connect_tcp((hostname, port), IMAP_TIMEOUT).await?; + pub async fn connect_starttls( + context: &Context, + hostname: &str, + port: u16, + strict_tls: bool, + ) -> Result { + let tcp_stream = connect_tcp(context, hostname, port, IMAP_TIMEOUT).await?; // Run STARTTLS command and convert the client back into a stream. let mut client = ImapClient::new(tcp_stream); @@ -146,12 +156,15 @@ impl Client { } pub async fn connect_secure_socks5( + context: &Context, domain: &str, port: u16, strict_tls: bool, socks5_config: Socks5Config, ) -> Result { - let socks5_stream = socks5_config.connect(domain, port, IMAP_TIMEOUT).await?; + let socks5_stream = socks5_config + .connect(context, domain, port, IMAP_TIMEOUT) + .await?; let tls = build_tls(strict_tls); let tls_stream = tls.connect(domain, socks5_stream).await?; let buffered_stream = BufWriter::new(tls_stream); @@ -166,11 +179,14 @@ impl Client { } pub async fn connect_insecure_socks5( + context: &Context, domain: &str, port: u16, socks5_config: Socks5Config, ) -> Result { - let socks5_stream = socks5_config.connect(domain, port, IMAP_TIMEOUT).await?; + let socks5_stream = socks5_config + .connect(context, domain, port, IMAP_TIMEOUT) + .await?; let buffered_stream = BufWriter::new(socks5_stream); let session_stream: Box = Box::new(buffered_stream); let mut client = ImapClient::new(session_stream); @@ -183,12 +199,15 @@ impl Client { } pub async fn connect_starttls_socks5( + context: &Context, hostname: &str, port: u16, socks5_config: Socks5Config, strict_tls: bool, ) -> Result { - let socks5_stream = socks5_config.connect(hostname, port, IMAP_TIMEOUT).await?; + let socks5_stream = socks5_config + .connect(context, hostname, port, IMAP_TIMEOUT) + .await?; // Run STARTTLS command and convert the client back into a stream. let mut client = ImapClient::new(socks5_stream); diff --git a/src/net.rs b/src/net.rs index 6c0c7dd0b..2885c0523 100644 --- a/src/net.rs +++ b/src/net.rs @@ -1,25 +1,55 @@ ///! # Common network utilities. +use std::net::SocketAddr; use std::pin::Pin; use std::time::Duration; use anyhow::{Context as _, Result}; -use tokio::net::{TcpStream, ToSocketAddrs}; +use tokio::net::{lookup_host, TcpStream}; use tokio::time::timeout; use tokio_io_timeout::TimeoutStream; +use crate::context::Context; + +async fn connect_tcp_inner(addr: SocketAddr, timeout_val: Duration) -> Result { + let tcp_stream = timeout(timeout_val, TcpStream::connect(addr)) + .await + .context("connection timeout")? + .context("connection failure")?; + Ok(tcp_stream) +} + /// Returns a TCP connection stream with read/write timeouts set /// and Nagle's algorithm disabled with `TCP_NODELAY`. /// /// `TCP_NODELAY` ensures writing to the stream always results in immediate sending of the packet /// to the network, which is important to reduce the latency of interactive protocols such as IMAP. pub(crate) async fn connect_tcp( - addr: impl ToSocketAddrs, + context: &Context, + host: &str, + port: u16, timeout_val: Duration, ) -> Result>>> { - let tcp_stream = timeout(timeout_val, TcpStream::connect(addr)) - .await - .context("connection timeout")? - .context("connection failure")?; + let mut tcp_stream = None; + for resolved_addr in lookup_host((host, port)).await? { + info!( + context, + "Resolved {}:{} into {}.", host, port, &resolved_addr + ); + match connect_tcp_inner(resolved_addr, timeout_val).await { + Ok(stream) => { + tcp_stream = Some(stream); + break; + } + Err(err) => { + warn!( + context, + "Failed to connect to {}: {:#}.", resolved_addr, err + ); + } + } + } + let tcp_stream = + tcp_stream.with_context(|| format!("failed to connect to {}:{}", host, port))?; // Disable Nagle's algorithm. tcp_stream.set_nodelay(true)?; diff --git a/src/socks.rs b/src/socks.rs index b05c265dc..52eb4fd61 100644 --- a/src/socks.rs +++ b/src/socks.rs @@ -58,11 +58,12 @@ impl Socks5Config { pub async fn connect( &self, + context: &Context, target_host: &str, target_port: u16, timeout_val: Duration, ) -> Result>>>> { - let tcp_stream = connect_tcp((self.host.clone(), self.port), timeout_val).await?; + let tcp_stream = connect_tcp(context, &self.host, self.port, timeout_val).await?; let authentication_method = if let Some((username, password)) = self.user_password.as_ref() { From 773754d74f6981f0e296fd6728889b12bc89cd02 Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 19 Jan 2023 16:17:31 +0000 Subject: [PATCH 02/12] Introduce DNS cache table Only used for IMAP connections currently. --- src/imap/client.rs | 12 +++--- src/net.rs | 88 ++++++++++++++++++++++++++++++++++++++++--- src/socks.rs | 6 ++- src/sql/migrations.rs | 13 +++++++ 4 files changed, 107 insertions(+), 12 deletions(-) diff --git a/src/imap/client.rs b/src/imap/client.rs index b6b106c45..e371a49c2 100644 --- a/src/imap/client.rs +++ b/src/imap/client.rs @@ -94,7 +94,7 @@ impl Client { port: u16, strict_tls: bool, ) -> Result { - let tcp_stream = connect_tcp(context, hostname, port, IMAP_TIMEOUT).await?; + let tcp_stream = connect_tcp(context, hostname, port, IMAP_TIMEOUT, strict_tls).await?; let tls = build_tls(strict_tls); let tls_stream = tls.connect(hostname, tcp_stream).await?; let buffered_stream = BufWriter::new(tls_stream); @@ -110,7 +110,7 @@ impl Client { } pub async fn connect_insecure(context: &Context, hostname: &str, port: u16) -> Result { - let tcp_stream = connect_tcp(context, hostname, port, IMAP_TIMEOUT).await?; + let tcp_stream = connect_tcp(context, hostname, port, IMAP_TIMEOUT, false).await?; let buffered_stream = BufWriter::new(tcp_stream); let session_stream: Box = Box::new(buffered_stream); let mut client = ImapClient::new(session_stream); @@ -128,7 +128,7 @@ impl Client { port: u16, strict_tls: bool, ) -> Result { - let tcp_stream = connect_tcp(context, hostname, port, IMAP_TIMEOUT).await?; + let tcp_stream = connect_tcp(context, hostname, port, IMAP_TIMEOUT, strict_tls).await?; // Run STARTTLS command and convert the client back into a stream. let mut client = ImapClient::new(tcp_stream); @@ -163,7 +163,7 @@ impl Client { socks5_config: Socks5Config, ) -> Result { let socks5_stream = socks5_config - .connect(context, domain, port, IMAP_TIMEOUT) + .connect(context, domain, port, IMAP_TIMEOUT, strict_tls) .await?; let tls = build_tls(strict_tls); let tls_stream = tls.connect(domain, socks5_stream).await?; @@ -185,7 +185,7 @@ impl Client { socks5_config: Socks5Config, ) -> Result { let socks5_stream = socks5_config - .connect(context, domain, port, IMAP_TIMEOUT) + .connect(context, domain, port, IMAP_TIMEOUT, false) .await?; let buffered_stream = BufWriter::new(socks5_stream); let session_stream: Box = Box::new(buffered_stream); @@ -206,7 +206,7 @@ impl Client { strict_tls: bool, ) -> Result { let socks5_stream = socks5_config - .connect(context, hostname, port, IMAP_TIMEOUT) + .connect(context, hostname, port, IMAP_TIMEOUT, strict_tls) .await?; // Run STARTTLS command and convert the client back into a stream. diff --git a/src/net.rs b/src/net.rs index 2885c0523..d5b6a1ec3 100644 --- a/src/net.rs +++ b/src/net.rs @@ -1,6 +1,7 @@ ///! # Common network utilities. use std::net::SocketAddr; use std::pin::Pin; +use std::str::FromStr; use std::time::Duration; use anyhow::{Context as _, Result}; @@ -9,6 +10,7 @@ use tokio::time::timeout; use tokio_io_timeout::TimeoutStream; use crate::context::Context; +use crate::tools::time; async fn connect_tcp_inner(addr: SocketAddr, timeout_val: Duration) -> Result { let tcp_stream = timeout(timeout_val, TcpStream::connect(addr)) @@ -18,23 +20,98 @@ async fn connect_tcp_inner(addr: SocketAddr, timeout_val: Duration) -> Result Result> { + let now = time(); + let mut resolved_addrs: Vec = lookup_host((hostname, port)).await?.collect(); + + for (i, addr) in resolved_addrs.iter().enumerate() { + info!(context, "Resolved {}:{} into {}.", hostname, port, &addr); + + let i = i64::try_from(i).unwrap_or_default(); + + // Update the cache. + // + // Add sequence number to the timestamp, so addresses are ordered by timestamp in the same + // order as the resolver returns them. + context + .sql + .execute( + "INSERT INTO dns_cache + (hostname, port, address, timestamp) + VALUES (?, ?, ?, ?) + ON CONFLICT (hostname, port, address) + DO UPDATE SET timestamp=excluded.timestamp", + paramsv![hostname, port, addr.to_string(), now.saturating_add(i)], + ) + .await?; + } + + if load_cache { + for cached_address in context + .sql + .query_map( + "SELECT address + FROM dns_cache + WHERE hostname = ? + AND ? < timestamp + 30 * 24 * 3600 + ORDER BY timestamp DESC", + paramsv![hostname, now], + |row| { + let address: String = row.get(0)?; + Ok(address) + }, + |rows| { + rows.collect::, _>>() + .map_err(Into::into) + }, + ) + .await? + { + match SocketAddr::from_str(&cached_address) { + Ok(addr) => { + if !resolved_addrs.contains(&addr) { + resolved_addrs.push(addr); + } + } + Err(err) => { + warn!( + context, + "Failed to parse cached address {:?}: {:#}.", cached_address, err + ); + } + } + } + } + + Ok(resolved_addrs) +} + /// Returns a TCP connection stream with read/write timeouts set /// and Nagle's algorithm disabled with `TCP_NODELAY`. /// /// `TCP_NODELAY` ensures writing to the stream always results in immediate sending of the packet /// to the network, which is important to reduce the latency of interactive protocols such as IMAP. +/// +/// If `load_cache` is true, may use cached DNS results. +/// Use this only if the connection is going to be protected with TLS. pub(crate) async fn connect_tcp( context: &Context, host: &str, port: u16, timeout_val: Duration, + load_cache: bool, ) -> Result>>> { let mut tcp_stream = None; - for resolved_addr in lookup_host((host, port)).await? { - info!( - context, - "Resolved {}:{} into {}.", host, port, &resolved_addr - ); + + for resolved_addr in lookup_host_with_cache(context, host, port, load_cache).await? { match connect_tcp_inner(resolved_addr, timeout_val).await { Ok(stream) => { tcp_stream = Some(stream); @@ -48,6 +125,7 @@ pub(crate) async fn connect_tcp( } } } + let tcp_stream = tcp_stream.with_context(|| format!("failed to connect to {}:{}", host, port))?; diff --git a/src/socks.rs b/src/socks.rs index 52eb4fd61..58d7aa1db 100644 --- a/src/socks.rs +++ b/src/socks.rs @@ -56,14 +56,18 @@ impl Socks5Config { } } + /// If `load_dns_cache` is true, loads cached DNS resolution results. + /// Use this only if the connection is going to be protected with TLS checks. pub async fn connect( &self, context: &Context, target_host: &str, target_port: u16, timeout_val: Duration, + load_dns_cache: bool, ) -> Result>>>> { - let tcp_stream = connect_tcp(context, &self.host, self.port, timeout_val).await?; + let tcp_stream = + connect_tcp(context, &self.host, self.port, timeout_val, load_dns_cache).await?; let authentication_method = if let Some((username, password)) = self.user_password.as_ref() { diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index fde27051c..549689c6a 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -671,6 +671,19 @@ CREATE INDEX smtp_messageid ON imap(rfc724_mid); ) .await?; } + if dbversion < 97 { + sql.execute_migration( + "CREATE TABLE dns_cache ( + hostname TEXT NOT NULL, + port INTEGER NOT NULL, + address TEXT NOT NULL, + timestamp INTEGER NOT NULL, + UNIQUE (hostname, port, address) + )", + 97, + ) + .await?; + } let new_version = sql .get_raw_config_int(VERSION_CFG) From 7d508dcb5240840fa49c0cb2b694125dd78a24ca Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 19 Jan 2023 16:21:47 +0000 Subject: [PATCH 03/12] Log DNS resolution errors instead of failing directly --- src/net.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/net.rs b/src/net.rs index d5b6a1ec3..d162c50c4 100644 --- a/src/net.rs +++ b/src/net.rs @@ -30,7 +30,16 @@ async fn lookup_host_with_cache( load_cache: bool, ) -> Result> { let now = time(); - let mut resolved_addrs: Vec = lookup_host((hostname, port)).await?.collect(); + let mut resolved_addrs: Vec = match lookup_host((hostname, port)).await { + Ok(res) => res.collect(), + Err(err) => { + warn!( + context, + "DNS resolution for {}:{} failed: {:#}.", hostname, port, err + ); + Vec::new() + } + }; for (i, addr) in resolved_addrs.iter().enumerate() { info!(context, "Resolved {}:{} into {}.", hostname, port, &addr); From c4c4c977a683b70130e9eeb3111a6fad6d63f495 Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 19 Jan 2023 16:25:27 +0000 Subject: [PATCH 04/12] Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ea9cc51f..516d85844 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Changes - Pipeline SMTP commands #3924 +- Cache DNS results #3970 ### Fixes - Securejoin: Fix adding and handling Autocrypt-Gossip headers #3914 From 9adb9ab5f499022d693e8520bf795464b1f1483d Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 19 Jan 2023 16:42:15 +0000 Subject: [PATCH 05/12] Return last connection error from `connect_tcp` --- src/net.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/net.rs b/src/net.rs index d162c50c4..7418d2f1f 100644 --- a/src/net.rs +++ b/src/net.rs @@ -4,7 +4,7 @@ use std::pin::Pin; use std::str::FromStr; use std::time::Duration; -use anyhow::{Context as _, Result}; +use anyhow::{Context as _, Error, Result}; use tokio::net::{lookup_host, TcpStream}; use tokio::time::timeout; use tokio_io_timeout::TimeoutStream; @@ -119,6 +119,7 @@ pub(crate) async fn connect_tcp( load_cache: bool, ) -> Result>>> { let mut tcp_stream = None; + let mut last_error = None; for resolved_addr in lookup_host_with_cache(context, host, port, load_cache).await? { match connect_tcp_inner(resolved_addr, timeout_val).await { @@ -131,12 +132,17 @@ pub(crate) async fn connect_tcp( context, "Failed to connect to {}: {:#}.", resolved_addr, err ); + last_error = Some(err); } } } - let tcp_stream = - tcp_stream.with_context(|| format!("failed to connect to {}:{}", host, port))?; + let tcp_stream = match tcp_stream { + Some(tcp_stream) => tcp_stream, + None => { + return Err(last_error.unwrap_or_else(|| Error::msg("no DNS resolution results"))); + } + }; // Disable Nagle's algorithm. tcp_stream.set_nodelay(true)?; From eaeaa297c72b94ac4d93095b3415e217c07a53ad Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 19 Jan 2023 16:55:43 +0000 Subject: [PATCH 06/12] Maximize priority of the cached address on successful connection --- src/net.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/net.rs b/src/net.rs index 7418d2f1f..437b991f9 100644 --- a/src/net.rs +++ b/src/net.rs @@ -58,7 +58,12 @@ async fn lookup_host_with_cache( VALUES (?, ?, ?, ?) ON CONFLICT (hostname, port, address) DO UPDATE SET timestamp=excluded.timestamp", - paramsv![hostname, port, addr.to_string(), now.saturating_add(i)], + paramsv![ + hostname, + port, + addr.to_string(), + now.saturating_add(i).saturating_add(1) + ], ) .await?; } @@ -125,6 +130,17 @@ pub(crate) async fn connect_tcp( match connect_tcp_inner(resolved_addr, timeout_val).await { Ok(stream) => { tcp_stream = Some(stream); + + // Maximize priority of this cached entry. + context + .sql + .execute( + "UPDATE dns_cache + SET timestamp = ? + WHERE address = ?", + paramsv![time(), resolved_addr.to_string()], + ) + .await?; break; } Err(err) => { From 20124bfca0fcb375cee057fae486462ae4620cef Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 19 Jan 2023 17:33:59 +0000 Subject: [PATCH 07/12] Add DNS lookup timeout --- src/net.rs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/net.rs b/src/net.rs index 437b991f9..0053109a5 100644 --- a/src/net.rs +++ b/src/net.rs @@ -20,6 +20,18 @@ async fn connect_tcp_inner(addr: SocketAddr, timeout_val: Duration) -> Result Result> { + let res = timeout(timeout_val, lookup_host((hostname, port))) + .await + .context("DNS lookup timeout")? + .context("DNS lookup failure")?; + Ok(res.collect()) +} + /// Looks up hostname and port using DNS and updates the address resolution cache. /// /// If `load_cache` is true, appends cached results not older than 30 days to the end. @@ -27,11 +39,12 @@ async fn lookup_host_with_cache( context: &Context, hostname: &str, port: u16, + timeout_val: Duration, load_cache: bool, ) -> Result> { let now = time(); - let mut resolved_addrs: Vec = match lookup_host((hostname, port)).await { - Ok(res) => res.collect(), + let mut resolved_addrs = match lookup_host_with_timeout(hostname, port, timeout_val).await { + Ok(res) => res, Err(err) => { warn!( context, @@ -126,7 +139,9 @@ pub(crate) async fn connect_tcp( let mut tcp_stream = None; let mut last_error = None; - for resolved_addr in lookup_host_with_cache(context, host, port, load_cache).await? { + for resolved_addr in + lookup_host_with_cache(context, host, port, timeout_val, load_cache).await? + { match connect_tcp_inner(resolved_addr, timeout_val).await { Ok(stream) => { tcp_stream = Some(stream); From 7a47c9e38bb8d25601b21765439edd0dba6c5518 Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 19 Jan 2023 18:29:18 +0000 Subject: [PATCH 08/12] Adapt `nicer_configuration_error` to new error message --- src/configure.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/configure.rs b/src/configure.rs index 064b9c8e0..ebce79c5d 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -665,6 +665,7 @@ async fn nicer_configuration_error(context: &Context, errors: Vec Date: Thu, 19 Jan 2023 20:26:11 +0000 Subject: [PATCH 09/12] Remove port number from DNS cache table --- src/net.rs | 18 +++++++++--------- src/sql/migrations.rs | 5 ++--- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/net.rs b/src/net.rs index 0053109a5..37648297a 100644 --- a/src/net.rs +++ b/src/net.rs @@ -1,5 +1,5 @@ ///! # Common network utilities. -use std::net::SocketAddr; +use std::net::{IpAddr, SocketAddr}; use std::pin::Pin; use std::str::FromStr; use std::time::Duration; @@ -67,14 +67,13 @@ async fn lookup_host_with_cache( .sql .execute( "INSERT INTO dns_cache - (hostname, port, address, timestamp) - VALUES (?, ?, ?, ?) - ON CONFLICT (hostname, port, address) + (hostname, address, timestamp) + VALUES (?, ?, ?) + ON CONFLICT (hostname, address) DO UPDATE SET timestamp=excluded.timestamp", paramsv![ hostname, - port, - addr.to_string(), + addr.ip().to_string(), now.saturating_add(i).saturating_add(1) ], ) @@ -102,8 +101,9 @@ async fn lookup_host_with_cache( ) .await? { - match SocketAddr::from_str(&cached_address) { - Ok(addr) => { + match IpAddr::from_str(&cached_address) { + Ok(ip_addr) => { + let addr = SocketAddr::new(ip_addr, port); if !resolved_addrs.contains(&addr) { resolved_addrs.push(addr); } @@ -153,7 +153,7 @@ pub(crate) async fn connect_tcp( "UPDATE dns_cache SET timestamp = ? WHERE address = ?", - paramsv![time(), resolved_addr.to_string()], + paramsv![time(), resolved_addr.ip().to_string()], ) .await?; break; diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 549689c6a..4611e9f7f 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -675,10 +675,9 @@ CREATE INDEX smtp_messageid ON imap(rfc724_mid); sql.execute_migration( "CREATE TABLE dns_cache ( hostname TEXT NOT NULL, - port INTEGER NOT NULL, - address TEXT NOT NULL, + address TEXT NOT NULL, -- IPv4 or IPv6 address timestamp INTEGER NOT NULL, - UNIQUE (hostname, port, address) + UNIQUE (hostname, address) )", 97, ) From 0978357c5fcc30f24b40a3fa3025042151e52ff0 Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 19 Jan 2023 20:43:53 +0000 Subject: [PATCH 10/12] Do not cache IP addresses which resolve into themselves --- src/net.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/net.rs b/src/net.rs index 37648297a..d983a6b5c 100644 --- a/src/net.rs +++ b/src/net.rs @@ -55,8 +55,13 @@ async fn lookup_host_with_cache( }; for (i, addr) in resolved_addrs.iter().enumerate() { - info!(context, "Resolved {}:{} into {}.", hostname, port, &addr); + let ip_string = addr.ip().to_string(); + if ip_string == hostname { + // IP address resolved into itself, not interesting to cache. + continue; + } + info!(context, "Resolved {}:{} into {}.", hostname, port, &addr); let i = i64::try_from(i).unwrap_or_default(); // Update the cache. @@ -71,11 +76,7 @@ async fn lookup_host_with_cache( VALUES (?, ?, ?) ON CONFLICT (hostname, address) DO UPDATE SET timestamp=excluded.timestamp", - paramsv![ - hostname, - addr.ip().to_string(), - now.saturating_add(i).saturating_add(1) - ], + paramsv![hostname, ip_string, now.saturating_add(i).saturating_add(1)], ) .await?; } From 41ccc133941f67ba1a40a0b826c2da9bc779cd2e Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 19 Jan 2023 21:06:31 +0000 Subject: [PATCH 11/12] Extend `lookup_host_with_cache` comment --- src/net.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/net.rs b/src/net.rs index d983a6b5c..e94e1484e 100644 --- a/src/net.rs +++ b/src/net.rs @@ -129,7 +129,11 @@ async fn lookup_host_with_cache( /// to the network, which is important to reduce the latency of interactive protocols such as IMAP. /// /// If `load_cache` is true, may use cached DNS results. -/// Use this only if the connection is going to be protected with TLS. +/// Because the cache may be poisoned with incorrect results by networks hijacking DNS requests, +/// this option should only be used when connection is authenticated, +/// for example using TLS. +/// If TLS is not used or invalid TLS certificates are allowed, +/// this option should be disabled. pub(crate) async fn connect_tcp( context: &Context, host: &str, From a483df8b20667540546a5307b299094387175fb7 Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 19 Jan 2023 21:29:17 +0000 Subject: [PATCH 12/12] Add all resolver results with the same timestamp --- src/net.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/net.rs b/src/net.rs index e94e1484e..c1065ea9e 100644 --- a/src/net.rs +++ b/src/net.rs @@ -54,7 +54,7 @@ async fn lookup_host_with_cache( } }; - for (i, addr) in resolved_addrs.iter().enumerate() { + for addr in resolved_addrs.iter() { let ip_string = addr.ip().to_string(); if ip_string == hostname { // IP address resolved into itself, not interesting to cache. @@ -62,12 +62,8 @@ async fn lookup_host_with_cache( } info!(context, "Resolved {}:{} into {}.", hostname, port, &addr); - let i = i64::try_from(i).unwrap_or_default(); // Update the cache. - // - // Add sequence number to the timestamp, so addresses are ordered by timestamp in the same - // order as the resolver returns them. context .sql .execute( @@ -76,7 +72,7 @@ async fn lookup_host_with_cache( VALUES (?, ?, ?) ON CONFLICT (hostname, address) DO UPDATE SET timestamp=excluded.timestamp", - paramsv![hostname, ip_string, now.saturating_add(i).saturating_add(1)], + paramsv![hostname, ip_string, now], ) .await?; }