From 2ebc21a44afb7f1ed0e6aeb7fea5e37400fe32bd Mon Sep 17 00:00:00 2001 From: _ Date: Thu, 29 Feb 2024 19:19:00 -0600 Subject: [PATCH] fix: use `atomicwrites` to write the resolv.conf backup more robustly --- rust/connlib/shared/src/error.rs | 13 ++--- .../shared/src/linux/etc_resolv_conf.rs | 56 +++++++++++-------- 2 files changed, 38 insertions(+), 31 deletions(-) diff --git a/rust/connlib/shared/src/error.rs b/rust/connlib/shared/src/error.rs index ae3c730cc69..9faae5136ad 100644 --- a/rust/connlib/shared/src/error.rs +++ b/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")] diff --git a/rust/connlib/shared/src/linux/etc_resolv_conf.rs b/rust/connlib/shared/src/linux/etc_resolv_conf.rs index 7d6b4448307..39be197de6d 100644 --- a/rust/connlib/shared/src/linux/etc_resolv_conf.rs +++ b/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,7 +43,7 @@ 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(()); @@ -39,27 +51,27 @@ async fn configure_dns_at_paths( 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(()) }