Address PR #50 review feedback on enable_ipv6 flag

uzyn · uzyn · commit 85ec2ed749f1 · 2026-04-14T15:37:55.000Z
- Extract connect-target selection into pure `select_connect_target()` with
  `ConnectTarget` enum; add unit tests covering all four IPv4/IPv6 combinations.
- Skip MX hosts with no A record when `enable_ipv6 = false` instead of silently
  falling through to the hostname (which could still resolve to IPv6 and
  violate the flag). Emits a clear "no A record; skipping" error so
  `try_deliver` moves on to the next MX.
- Replace blocking `ToSocketAddrs` in `resolve_ipv4` with a new
  `mx::resolve_a()` helper built on `hickory-resolver`, for consistency with
  MX resolution and to avoid blocking getaddrinfo in tokio workers.
- Add a doc comment on `resolve_ipv4` explaining why it exists (flag
  semantics, third add/remove in three sprints) to prevent future deletion.
- Add a doc comment on TLS SNI vs. connect-target mismatch while
  `dangerous_accept_invalid_certs` is set.
- Extract `detect_server_ipv6(enable_ipv6, net)` gate in setup.rs and add
  tests using `get_server_ipv6_calls` counter on `MockNetworkOps` to verify
  the network call is made iff the flag is true.
- Correct book/configuration.md: `aimx verify` only probes port 25, not IPv6;
  the flag only affects `aimx setup`.
- Add post-merge addendum under Sprint 26 in docs/sprint.md noting that this
  follow-up flipped the default to opt-in IPv6.
diff --git a/book/configuration.md b/book/configuration.md
@@ -111,7 +111,7 @@ If your server has a global IPv6 address and you want outbound mail to use it:
 
 See the full DNS records table in [Setup](setup.md#dns-configuration) for formats. Without these DNS updates, messages delivered over IPv6 will fail SPF and may be rejected under your DMARC policy.
 
-**When `enable_ipv6` is unset or `false`:** `aimx setup` and `aimx verify` ignore IPv6 entirely — no AAAA is advertised, no `ip6:` SPF is generated, and existing AAAA records in DNS are not validated (but their presence is harmless).
+**When `enable_ipv6` is unset or `false`:** `aimx setup` ignores IPv6 entirely — no AAAA is advertised, no `ip6:` SPF is generated, and existing AAAA records in DNS are not validated (their presence is harmless). `aimx verify` only probes port 25 connectivity and is unaffected by this flag.
 
 Leave `enable_ipv6` unset (or `false`) if any of these apply:
 - Your server does not have a global IPv6 address
diff --git a/docs/sprint.md b/docs/sprint.md
@@ -244,6 +244,16 @@ Completed sprints 1–21 have been archived for context window efficiency.
 
 ## Sprint 26 — IPv6 Support for Outbound SMTP (Days 73–75.5) [DONE]
 
+> **Follow-up addendum (post-merge):** A later PR (`enable-ipv6-config-flag`)
+> flipped the default back to IPv4-only and made IPv6 outbound opt-in via a
+> new `enable_ipv6` bool in `config.toml`. The Sprint 26 ACs below still
+> describe the original "OS chooses the family" behaviour that shipped when
+> this sprint merged; the current shipped default is IPv4-only, and the
+> dual-stack SPF / AAAA guidance is only emitted by `aimx setup` when
+> `enable_ipv6 = true`. See PRD FR-7, FR-19, resolved-decision #8 and
+> `book/configuration.md` "IPv6 delivery (advanced)" for the current
+> behaviour.
+
 **Goal:** Remove the IPv4-only workaround from outbound delivery and properly support IPv6 across SPF records, DNS guidance, and verification. The IPv4 preference was added in Sprint 25 as a workaround for SPF failures — now that DKIM is fixed, let the OS resolve addresses naturally and ensure SPF covers both address families.
 
 **Dependencies:** Sprint 25
diff --git a/src/mx.rs b/src/mx.rs
@@ -37,6 +37,23 @@ async fn fallback_to_a_record(
     }
 }
 
+/// Resolves a host to its IPv4 (A-record) addresses only, skipping AAAA.
+///
+/// Used by outbound SMTP when `enable_ipv6 = false`, so the connect target is
+/// pinned to an IPv4 address rather than letting the OS pick an address family.
+/// Returns an empty Vec when the host has no A record (e.g. AAAA-only hosts).
+pub async fn resolve_a(host: &str) -> Result<Vec<std::net::Ipv4Addr>, Box<dyn std::error::Error>> {
+    let resolver = TokioResolver::builder_tokio()
+        .map_err(|e| format!("Failed to create DNS resolver: {e}"))?
+        .build();
+
+    match resolver.ipv4_lookup(host).await {
+        Ok(response) => Ok(response.iter().map(|a| a.0).collect()),
+        Err(e) if e.is_no_records_found() || e.is_nx_domain() => Ok(vec![]),
+        Err(e) => Err(format!("DNS A-record lookup failed for {host}: {e}").into()),
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/src/send.rs b/src/send.rs
@@ -19,20 +19,64 @@ pub struct LettreTransport {
     enable_ipv6: bool,
 }
 
+/// Outcome of picking a connect target for outbound SMTP.
+///
+/// Exists so the `enable_ipv6 = false` path can be tested without real DNS.
+#[derive(Debug, PartialEq, Eq)]
+pub(crate) enum ConnectTarget {
+    /// Connect over this literal (hostname when `enable_ipv6 = true`, or an
+    /// IPv4 string when `enable_ipv6 = false` and an A record was found).
+    Target(String),
+    /// IPv4-only was requested but the MX host has no A record. Caller should
+    /// skip this MX so the flag is not silently violated.
+    SkipNoIpv4,
+}
+
+/// Pure, testable connect-target selection.
+///
+/// - `enable_ipv6 = true` → always use the hostname; OS picks the family.
+/// - `enable_ipv6 = false` + at least one A record → use the first A.
+/// - `enable_ipv6 = false` + no A records → `SkipNoIpv4` so the caller can
+///   move on to the next MX instead of silently falling through to the
+///   hostname (which may resolve to IPv6 and violate the flag).
+pub(crate) fn select_connect_target(
+    host: &str,
+    enable_ipv6: bool,
+    ipv4_addrs: &[std::net::Ipv4Addr],
+) -> ConnectTarget {
+    if enable_ipv6 {
+        return ConnectTarget::Target(host.to_string());
+    }
+    match ipv4_addrs.first() {
+        Some(addr) => ConnectTarget::Target(addr.to_string()),
+        None => ConnectTarget::SkipNoIpv4,
+    }
+}
+
 impl LettreTransport {
     pub fn new(enable_ipv6: bool) -> Self {
         Self { enable_ipv6 }
     }
 
-    fn resolve_ipv4(host: &str) -> Option<std::net::Ipv4Addr> {
-        use std::net::ToSocketAddrs;
-        format!("{host}:25")
-            .to_socket_addrs()
-            .ok()?
-            .find_map(|addr| match addr {
-                std::net::SocketAddr::V4(v4) => Some(*v4.ip()),
-                _ => None,
-            })
+    /// Resolves an MX hostname's A records only (no AAAA).
+    ///
+    /// This helper has been added, removed, and re-added across Sprints 25, 26,
+    /// and this follow-up PR. It exists specifically to honour the opt-in
+    /// `enable_ipv6` flag: when the flag is false we pin the connect target to
+    /// an IPv4 literal so `lettre`/the OS cannot silently select an AAAA
+    /// record. Do not delete without re-auditing the flag semantics.
+    fn resolve_ipv4(host: &str) -> Result<Vec<std::net::Ipv4Addr>, Box<dyn std::error::Error>> {
+        let rt = tokio::runtime::Handle::try_current();
+        match rt {
+            Ok(handle) => {
+                tokio::task::block_in_place(|| handle.block_on(crate::mx::resolve_a(host)))
+            }
+            Err(_) => {
+                let rt = tokio::runtime::Runtime::new()
+                    .map_err(|e| format!("Failed to create runtime: {e}"))?;
+                rt.block_on(crate::mx::resolve_a(host))
+            }
+        }
     }
 
     fn extract_domain(recipient: &str) -> Result<String, Box<dyn std::error::Error>> {
@@ -117,17 +161,27 @@ impl LettreTransport {
     ) -> Result<String, Box<dyn std::error::Error>> {
         use lettre::Transport;
 
+        // Note: SNI uses `host` while the connect target may be a bare IPv4
+        // literal (IPv4-only mode). This is fine here because
+        // `dangerous_accept_invalid_certs(true)` is set — cert name mismatch
+        // is accepted. If that flag is ever flipped, SNI and the TLS peer
+        // identity would need to be reconciled.
         let tls_params = lettre::transport::smtp::client::TlsParameters::builder(host.to_string())
             .dangerous_accept_invalid_certs(true)
             .build_rustls()
             .map_err(|e| format!("TLS configuration error: {e}"))?;
 
-        let connect_target = if self.enable_ipv6 {
-            host.to_string()
+        let ipv4_addrs = if self.enable_ipv6 {
+            Vec::new()
         } else {
-            Self::resolve_ipv4(host)
-                .map(|ip| ip.to_string())
-                .unwrap_or_else(|| host.to_string())
+            Self::resolve_ipv4(host).unwrap_or_default()
+        };
+
+        let connect_target = match select_connect_target(host, self.enable_ipv6, &ipv4_addrs) {
+            ConnectTarget::Target(t) => t,
+            ConnectTarget::SkipNoIpv4 => {
+                return Err(format!("{host}: no A record (enable_ipv6 = false); skipping").into());
+            }
         };
 
         let transport = lettre::SmtpTransport::builder_dangerous(&connect_target)
@@ -997,6 +1051,35 @@ mod tests {
         );
     }
 
+    #[test]
+    fn select_connect_target_ipv6_enabled_returns_hostname() {
+        let target = select_connect_target("mx.example.com", true, &[]);
+        assert_eq!(target, ConnectTarget::Target("mx.example.com".to_string()));
+    }
+
+    #[test]
+    fn select_connect_target_ipv6_enabled_ignores_ipv4_addrs() {
+        let addrs = vec!["1.2.3.4".parse().unwrap()];
+        let target = select_connect_target("mx.example.com", true, &addrs);
+        assert_eq!(target, ConnectTarget::Target("mx.example.com".to_string()));
+    }
+
+    #[test]
+    fn select_connect_target_ipv4_mode_uses_first_a_record() {
+        let addrs: Vec<std::net::Ipv4Addr> = vec![
+            "203.0.113.10".parse().unwrap(),
+            "203.0.113.11".parse().unwrap(),
+        ];
+        let target = select_connect_target("mx.example.com", false, &addrs);
+        assert_eq!(target, ConnectTarget::Target("203.0.113.10".to_string()));
+    }
+
+    #[test]
+    fn select_connect_target_ipv4_mode_without_a_record_skips() {
+        let target = select_connect_target("aaaa-only.example.com", false, &[]);
+        assert_eq!(target, ConnectTarget::SkipNoIpv4);
+    }
+
     #[test]
     fn lettre_transport_extract_domain() {
         assert_eq!(
diff --git a/src/setup.rs b/src/setup.rs
@@ -1063,6 +1063,24 @@ pub fn is_already_configured(sys: &dyn SystemOps, _domain: &str, data_dir: &Path
     service_running && cert_exists && dkim_exists
 }
 
+/// Detects the server's IPv6 address iff `enable_ipv6 = true`.
+///
+/// IPv6 outbound is opt-in via `enable_ipv6` in `config.toml`. When disabled,
+/// the call to `net.get_server_ipv6()` is skipped entirely — AAAA + `ip6:`
+/// SPF guidance/verification are omitted to match the IPv4-only default of
+/// `aimx send`. Extracted as a standalone helper so the gating can be tested
+/// without driving the whole setup flow.
+pub(crate) fn detect_server_ipv6(
+    enable_ipv6: bool,
+    net: &dyn NetworkOps,
+) -> Result<Option<IpAddr>, Box<dyn std::error::Error>> {
+    if enable_ipv6 {
+        net.get_server_ipv6()
+    } else {
+        Ok(None)
+    }
+}
+
 pub fn run_setup(
     domain: Option<&str>,
     data_dir: Option<&Path>,
@@ -1187,14 +1205,7 @@ pub fn run_setup(
 
     // Step 7: DNS guidance and verification (section [DNS])
     let server_ip = net.get_server_ip()?;
-    // IPv6 outbound is opt-in via `enable_ipv6` in config.toml. When disabled,
-    // skip IPv6 detection entirely so AAAA and `ip6:` SPF guidance/verification
-    // are omitted — matching the IPv4-only default of `aimx send`.
-    let server_ipv6 = if enable_ipv6 {
-        net.get_server_ipv6()?
-    } else {
-        None
-    };
+    let server_ipv6 = detect_server_ipv6(enable_ipv6, net)?;
     let dkim_value = dkim::dns_record_value(data_dir)?;
 
     let local_dkim_pubkey = dkim_value
@@ -1285,6 +1296,7 @@ mod tests {
         a_records: HashMap<String, Vec<IpAddr>>,
         aaaa_records: HashMap<String, Vec<IpAddr>>,
         txt_records: HashMap<String, Vec<String>>,
+        get_server_ipv6_calls: std::cell::Cell<u32>,
     }
 
     impl Default for MockNetworkOps {
@@ -1299,6 +1311,7 @@ mod tests {
                 a_records: HashMap::new(),
                 aaaa_records: HashMap::new(),
                 txt_records: HashMap::new(),
+                get_server_ipv6_calls: std::cell::Cell::new(0),
             }
         }
     }
@@ -1317,6 +1330,8 @@ mod tests {
             Ok(self.server_ip)
         }
         fn get_server_ipv6(&self) -> Result<Option<IpAddr>, Box<dyn std::error::Error>> {
+            self.get_server_ipv6_calls
+                .set(self.get_server_ipv6_calls.get() + 1);
             Ok(self.server_ipv6)
         }
         fn resolve_mx(&self, domain: &str) -> Result<Vec<String>, Box<dyn std::error::Error>> {
@@ -2211,6 +2226,47 @@ mod tests {
         );
     }
 
+    #[test]
+    fn detect_server_ipv6_false_does_not_query_net() {
+        let net = MockNetworkOps {
+            server_ipv6: Some("2001:db8::1".parse().unwrap()),
+            ..Default::default()
+        };
+        let result = detect_server_ipv6(false, &net).unwrap();
+        assert!(result.is_none(), "ipv6 should be None when flag disabled");
+        assert_eq!(
+            net.get_server_ipv6_calls.get(),
+            0,
+            "get_server_ipv6 must NOT be called when enable_ipv6 = false"
+        );
+    }
+
+    #[test]
+    fn detect_server_ipv6_true_queries_net() {
+        let net = MockNetworkOps {
+            server_ipv6: Some("2001:db8::1".parse().unwrap()),
+            ..Default::default()
+        };
+        let result = detect_server_ipv6(true, &net).unwrap();
+        assert_eq!(result, Some("2001:db8::1".parse().unwrap()));
+        assert_eq!(
+            net.get_server_ipv6_calls.get(),
+            1,
+            "get_server_ipv6 must be called exactly once when enable_ipv6 = true"
+        );
+    }
+
+    #[test]
+    fn detect_server_ipv6_true_returns_none_when_net_has_none() {
+        let net = MockNetworkOps {
+            server_ipv6: None,
+            ..Default::default()
+        };
+        let result = detect_server_ipv6(true, &net).unwrap();
+        assert!(result.is_none());
+        assert_eq!(net.get_server_ipv6_calls.get(), 1);
+    }
+
     #[test]
     fn setup_with_domain_arg_skips_prompt() {
         let tmp = TempDir::new().unwrap();
