From 849a873e616ad2d4ff87f4a11f6ce81c707d4a4d Mon Sep 17 00:00:00 2001
From: link2xt <link2xt@testrun.org>
Date: Sun, 14 Jan 2024 05:32:26 +0000
Subject: [PATCH] feat: only try to configure non-strict TLS checks if
 explicitly set

Trying non-strict TLS checks is not necessary
for most servers with proper TLS setup,
but doubles the time needed to fail configuration
when the server is not responding, e.g.
when all connection attempts time out.

There is also a risk of accidentally
configuring non-strict TLS checks in a rare case
that strict TLS check configuration spuriously failed,
e.g. on a bad network.

If the server has a known broken TLS setup,
it can still be added to the provider database
or configured with non-strict TLS check manually.
User can also configure another email provider,
such as chatmail servers, instead of using the server
with invalid TLS hostname.

This change does not affect exising setups.
---
 src/configure/server_params.rs | 58 ++++------------------------------
 src/login_param.rs             | 18 +++++++++--
 2 files changed, 22 insertions(+), 54 deletions(-)

diff --git a/src/configure/server_params.rs b/src/configure/server_params.rs
index 019104655..fd921532c 100644
--- a/src/configure/server_params.rs
+++ b/src/configure/server_params.rs
@@ -137,20 +137,11 @@ impl ServerParams {
     }
 
     fn expand_strict_tls(self) -> Vec<ServerParams> {
-        if self.strict_tls.is_none() {
-            vec![
-                Self {
-                    strict_tls: Some(true), // Strict.
-                    ..self.clone()
-                },
-                Self {
-                    strict_tls: None, // Automatic.
-                    ..self
-                },
-            ]
-        } else {
-            vec![self]
-        }
+        vec![Self {
+            // Strict if not set by the user or provider database.
+            strict_tls: Some(self.strict_tls.unwrap_or(true)),
+            ..self
+        }]
     }
 }
 
@@ -162,31 +153,10 @@ pub(crate) fn expand_param_vector(
     domain: &str,
 ) -> Vec<ServerParams> {
     v.into_iter()
-        .map(|params| {
-            if params.socket == Socket::Plain {
-                ServerParams {
-                    // Avoid expanding plaintext configuration into configuration with and without
-                    // `strict_tls` if `strict_tls` is set to `None` as `strict_tls` is not used for
-                    // plaintext connections. Always setting it to "enabled", just in case.
-                    strict_tls: Some(true),
-                    ..params
-                }
-            } else {
-                params
-            }
-        })
         // The order of expansion is important.
         //
         // Ports are expanded the last, so they are changed the first.  Username is only changed if
         // default value (address with domain) didn't work for all available hosts and ports.
-        //
-        // Strict TLS must be expanded first, so we try all configurations with strict TLS first
-        // and only then try again without strict TLS. Otherwise we may lock to wrong hostname
-        // without strict TLS when another hostname with strict TLS is available.  For example, if
-        // both smtp.example.net and mail.example.net are running an SMTP server, but both use a
-        // certificate that is only valid for mail.example.net, we want to skip smtp.example.net
-        // and use mail.example.net with strict TLS instead of using smtp.example.net without
-        // strict TLS.
         .flat_map(|params| params.expand_strict_tls().into_iter())
         .flat_map(|params| params.expand_usernames(addr).into_iter())
         .flat_map(|params| params.expand_hostnames(domain).into_iter())
@@ -257,22 +227,6 @@ mod tests {
                     username: "foobar".to_string(),
                     strict_tls: Some(true)
                 },
-                ServerParams {
-                    protocol: Protocol::Smtp,
-                    hostname: "example.net".to_string(),
-                    port: 123,
-                    socket: Socket::Ssl,
-                    username: "foobar".to_string(),
-                    strict_tls: None,
-                },
-                ServerParams {
-                    protocol: Protocol::Smtp,
-                    hostname: "example.net".to_string(),
-                    port: 123,
-                    socket: Socket::Starttls,
-                    username: "foobar".to_string(),
-                    strict_tls: None
-                }
             ],
         );
 
@@ -284,7 +238,7 @@ mod tests {
                 port: 123,
                 socket: Socket::Plain,
                 username: "foobar".to_string(),
-                strict_tls: None,
+                strict_tls: Some(true),
             }],
             "foobar@example.net",
             "example.net",
diff --git a/src/login_param.rs b/src/login_param.rs
index 25511c419..c37fe2931 100644
--- a/src/login_param.rs
+++ b/src/login_param.rs
@@ -14,8 +14,22 @@ use crate::socks::Socks5Config;
 #[repr(u32)]
 #[strum(serialize_all = "snake_case")]
 pub enum CertificateChecks {
-    /// Same as AcceptInvalidCertificates unless overridden by
-    /// `strict_tls` setting in provider database.
+    /// Same as AcceptInvalidCertificates if stored in the database
+    /// as `configured_{imap,smtp}_certificate_checks`.
+    ///
+    /// Previous Delta Chat versions stored this in `configured_*`
+    /// if Automatic configuration
+    /// was selected, configuration with strict TLS checks failed
+    /// and configuration without strict TLS checks succeeded.
+    ///
+    /// Currently Delta Chat stores only
+    /// `Strict` or `AcceptInvalidCertificates` variants
+    /// in `configured_*` settings.
+    ///
+    /// `Automatic` in `{imap,smtp}_certificate_checks`
+    /// means that provider database setting should be taken.
+    /// If there is no provider database setting for certificate checks,
+    /// `Automatic` is the same as `Strict`.
     Automatic = 0,
 
     Strict = 1,