fix: use atomicwrites to back up /etc/resolv.conf more robustly

rust/connlib/shared/src/error.rs

@@ -161,15 +161,10 @@ pub enum ConnlibError {
     #[error(transparent)]
     JoinError(#[from] JoinError),
 
-    // Error variants for `/etc/resolv.conf` DNS control
-    #[error("Failed to read `resolv.conf`: {0}")]
-    ReadResolvConf(std::io::Error),
-    #[error("Failed to parse `resolv.conf`")]
-    ParseResolvConf,
-    #[error("Failed to backup `resolv.conf`: {0}")]
-    WriteResolvConfBackup(std::io::Error),
-    #[error("Failed to rewrite `resolv.conf`: {0}")]
-    RewriteResolvConf(std::io::Error),
+    #[cfg(target_os = "linux")]
+    #[error("Error while rewriting `/etc/resolv.conf`: {0}")]
+    ResolvConf(#[from] crate::linux::etc_resolv_conf::Error),
+
     #[error(transparent)]
     Snownet(#[from] snownet::Error),
     #[error("Detected non-allowed packet in channel")]

rust/connlib/shared/src/linux/etc_resolv_conf.rs

@@ -1,15 +1,27 @@
-use crate::{Error, Result};
-use std::{net::IpAddr, path::Path};
-use tokio::io::AsyncWriteExt;
+use std::{io::Write, net::IpAddr, path::Path};
 
 pub const ETC_RESOLV_CONF: &str = "/etc/resolv.conf";
 pub const ETC_RESOLV_CONF_BACKUP: &str = "/etc/resolv.conf.before-firezone";
 
+#[derive(Debug, thiserror::Error)]
+pub enum Error {
+    #[error("Failed to read `resolv.conf`: {0}")]
+    Read(std::io::Error),
+    #[error("Failed to parse `resolv.conf`")]
+    Parse,
+    #[error("Failed to rewrite `resolv.conf`: {0}")]
+    Rewrite(std::io::Error),
+    #[error("Failed to sync `resolv.conf` backup: {0}")]
+    SyncBackup(std::io::Error),
+    #[error("Failed to backup `resolv.conf`: {0}")]
+    WriteBackup(std::io::Error),
+}
+
 /// Back up `/etc/resolv.conf`(sic) and then modify it in-place
 ///
 /// This is async because it's called in a Tokio context and it's nice to use their
 /// `fs` module
-pub async fn configure_dns(dns_config: &[IpAddr]) -> Result<()> {
+pub async fn configure_dns(dns_config: &[IpAddr]) -> Result<(), Error> {
     configure_dns_at_paths(
         dns_config,
         Path::new(ETC_RESOLV_CONF),
@@ -22,7 +34,7 @@ pub async fn configure_dns(dns_config: &[IpAddr]) -> Result<()> {
 ///
 /// This is sync because it's called from the Linux CLI client where we don't have our own
 /// Tokio context.
-pub fn unconfigure_dns() -> Result<()> {
+pub fn unconfigure_dns() -> Result<(), Error> {
     tracing::debug!("Unconfiguring `/etc/resolv.conf` not implemented yet");
     Ok(())
 }
@@ -31,35 +43,35 @@ async fn configure_dns_at_paths(
     dns_config: &[IpAddr],
     resolv_path: &Path,
     backup_path: &Path,
-) -> Result<()> {
+) -> Result<(), Error> {
     if dns_config.is_empty() {
         tracing::info!("`dns_config` is empty, leaving `/etc/resolv.conf` unchanged");
         return Ok(());
     }
 
     let text = tokio::fs::read_to_string(resolv_path)
         .await
-        .map_err(Error::ReadResolvConf)?;
-    let parsed = resolv_conf::Config::parse(&text).map_err(|_| Error::ParseResolvConf)?;
+        .map_err(Error::Read)?;
+    let parsed = resolv_conf::Config::parse(&text).map_err(|_| Error::Parse)?;
 
     // Back up the original resolv.conf. If there's already a backup, don't modify it
-    match tokio::fs::File::options()
-        .write(true)
-        .create_new(true)
-        .open(backup_path)
-        .await
-    {
-        Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {
+    // `atomicwrites` handles the fsync and rename-into-place tricks to resist file corruption
+    // if we lose power during the write.
+    let backup_file = atomicwrites::AtomicFile::new(
+        backup_path,
+        atomicwrites::OverwriteBehavior::DisallowOverwrite,
+    );
+    match backup_file.write(|f| f.write_all(text.as_bytes())) {
+        Err(atomicwrites::Error::Internal(error))
+            if error.kind() == std::io::ErrorKind::AlreadyExists =>
+        {
             tracing::info!(?backup_path, "Backup path already exists, won't overwrite");
         }
-        Err(error) => return Err(Error::WriteResolvConfBackup(error)),
-        // TODO: Would do a rename-into-place here if the contents of the file mattered more
-        Ok(mut f) => f.write_all(text.as_bytes()).await?,
+        Err(atomicwrites::Error::Internal(error)) => return Err(Error::SyncBackup(error)),
+        Err(atomicwrites::Error::User(error)) => return Err(Error::WriteBackup(error)),
+        Ok(()) => {}
     }
 
-    // TODO: Would do an fsync here if resolv.conf was important and not
-    // auto-generated by Docker on every run.
-
     let mut new_resolv_conf = parsed.clone();
     new_resolv_conf.nameservers = dns_config.iter().map(|addr| (*addr).into()).collect();
 
@@ -82,7 +94,7 @@ async fn configure_dns_at_paths(
 
     tokio::fs::write(resolv_path, new_text)
         .await
-        .map_err(Error::RewriteResolvConf)?;
+        .map_err(Error::Rewrite)?;
 
     Ok(())
 }