diff --git a/connect-lib/bin/src/main.rs b/connect-lib/bin/src/main.rs index 2679091..63d0560 100644 --- a/connect-lib/bin/src/main.rs +++ b/connect-lib/bin/src/main.rs @@ -700,10 +700,8 @@ async fn run() -> n0_error::Result<()> { let tunnel = service.create_active(&label, &endpoint).await?; // Persist the in-memory key to the per-tunnel directory. if let Some(ref secret_key) = in_memory_key { - let key_dir = repo.path().join(&project_id).join(&tunnel.id); - let key_path = key_dir.join(Repo::LISTEN_KEY_FILE); - tokio::fs::create_dir_all(&key_dir).await?; - tokio::fs::write(&key_path, secret_key.to_bytes()).await?; + repo.save_listen_key_for_tunnel(&project_id, &tunnel.id, secret_key) + .await?; } if json { println!( diff --git a/connect-lib/lib/src/repo.rs b/connect-lib/lib/src/repo.rs index 8a1c6c8..137df5d 100644 --- a/connect-lib/lib/src/repo.rs +++ b/connect-lib/lib/src/repo.rs @@ -1,4 +1,4 @@ -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use iroh::SecretKey; use n0_error::{Result, StackResultExt, StdResultExt}; @@ -149,7 +149,7 @@ impl Repo { if let Some(parent) = key_file_path.parent() { tokio::fs::create_dir_all(parent).await?; } - tokio::fs::write(&key_file_path, key.to_bytes()).await?; + write_secret_key(&key_file_path, &key).await?; Ok(key) } @@ -233,7 +233,7 @@ impl Repo { let tunnel_dir = self.0.join(project_id).join(tunnel_name); let key_file_path = tunnel_dir.join(Self::LISTEN_KEY_FILE); tokio::fs::create_dir_all(&tunnel_dir).await?; - tokio::fs::write(&key_file_path, key.to_bytes()).await?; + write_secret_key(&key_file_path, key).await?; Ok(()) } @@ -251,9 +251,9 @@ impl Repo { Ok(SecretKey::from_bytes(key)) } - async fn create_key(&self, key_file_path: &PathBuf) -> Result { + async fn create_key(&self, key_file_path: &Path) -> Result { let key = SecretKey::generate(&mut rand::rng()); - tokio::fs::write(key_file_path, key.to_bytes()).await?; + write_secret_key(key_file_path, &key).await?; Ok(key) } @@ -272,9 +272,59 @@ impl Repo { } } +/// Write a secret key to disk atomically, readable only by the current user. +/// +/// The key is written to a fresh temporary file beside `path` and then +/// renamed over it, so a crash mid-write can never leave a truncated key +/// behind, and a reader holding a descriptor to the previous file never +/// observes the new key. +/// +/// On Unix the temporary file is created with mode 0600 so the iroh +/// identity is not exposed to other local users through the default umask. +/// On other platforms the file inherits the access control of its parent +/// directory; the caller is responsible for choosing a directory that only +/// the current user can read. +async fn write_secret_key(path: &Path, key: &SecretKey) -> Result<()> { + use tokio::io::AsyncWriteExt; + + let file_name = path + .file_name() + .and_then(|name| name.to_str()) + .context("secret key path has no file name")?; + let tmp_path = path.with_file_name(format!( + "{file_name}.{}.{}.tmp", + std::process::id(), + chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default() + )); + + let mut options = tokio::fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + options.mode(0o600); + + let result = async { + let mut file = options.open(&tmp_path).await?; + file.write_all(&key.to_bytes()).await?; + file.sync_all().await?; + drop(file); + tokio::fs::rename(&tmp_path, path).await?; + Ok::<(), std::io::Error>(()) + } + .await; + + if result.is_err() { + let _ = tokio::fs::remove_file(&tmp_path).await; + } + result?; + Ok(()) +} + #[cfg(test)] #[allow(clippy::expect_used, clippy::panic)] mod tests { + #[cfg(unix)] + use std::os::unix::fs::PermissionsExt; + use super::*; fn temp_repo_dir() -> PathBuf { @@ -468,6 +518,121 @@ mod tests { ); Ok(()) } + + type TestResult = Result<(), Box>; + + #[cfg(unix)] + fn mode_of(path: &Path) -> std::io::Result { + Ok(std::fs::metadata(path)?.permissions().mode() & 0o777) + } + + async fn leftover_temp_files(dir: &Path) -> std::io::Result> { + let mut leftovers = Vec::new(); + let mut entries = tokio::fs::read_dir(dir).await?; + while let Some(entry) = entries.next_entry().await? { + let name = entry.file_name().to_string_lossy().into_owned(); + if name.ends_with(".tmp") { + leftovers.push(name); + } + } + Ok(leftovers) + } + + #[cfg(unix)] + #[tokio::test] + async fn generated_keys_are_private() -> TestResult { + let repo = Repo::open_or_create(temp_repo_dir()).await?; + + repo.connect_key().await?; + assert_eq!(mode_of(&repo.0.join(Repo::CONNECT_KEY_FILE))?, 0o600); + + repo.listen_key_for_project("proj").await?; + assert_eq!( + mode_of(&repo.0.join("proj").join(Repo::LISTEN_KEY_FILE))?, + 0o600 + ); + + let key = SecretKey::generate(&mut rand::rng()); + repo.save_listen_key_for_tunnel("proj", "tun", &key).await?; + assert_eq!( + mode_of(&repo.0.join("proj").join("tun").join(Repo::LISTEN_KEY_FILE))?, + 0o600 + ); + + repo.listen_key(Some("proj")).await?; + let mut entries = tokio::fs::read_dir(&repo.0).await?; + let mut found = false; + while let Some(entry) = entries.next_entry().await? { + let name = entry.file_name(); + let name = name.to_string_lossy(); + if name.starts_with("listen_key.proj.") && !name.ends_with(".tmp") { + found = true; + assert_eq!(mode_of(&entry.path())?, 0o600, "{name}"); + } + } + assert!(found, "timestamped listen key must have been written"); + Ok(()) + } + + #[cfg(unix)] + #[tokio::test] + async fn replacing_a_world_readable_key_yields_a_private_file() -> TestResult { + let repo = Repo::open_or_create(temp_repo_dir()).await?; + let tunnel_dir = repo.0.join("proj").join("tun"); + tokio::fs::create_dir_all(&tunnel_dir).await?; + let key_path = tunnel_dir.join(Repo::LISTEN_KEY_FILE); + tokio::fs::write(&key_path, b"old").await?; + std::fs::set_permissions(&key_path, std::fs::Permissions::from_mode(0o644))?; + assert_eq!(mode_of(&key_path)?, 0o644, "precondition"); + + // A reader that opened the old, world-readable file must not see the + // new key: the write goes to a fresh inode and is renamed into place. + let old_handle = std::fs::File::open(&key_path)?; + + let key = SecretKey::generate(&mut rand::rng()); + repo.save_listen_key_for_tunnel("proj", "tun", &key).await?; + assert_eq!(mode_of(&key_path)?, 0o600); + assert_eq!(tokio::fs::read(&key_path).await?, key.to_bytes()); + + let mut through_old_handle = Vec::new(); + std::io::Read::read_to_end(&mut &old_handle, &mut through_old_handle)?; + assert_eq!( + through_old_handle, b"old", + "old descriptor must still point at the old contents" + ); + Ok(()) + } + + #[tokio::test] + async fn key_writes_leave_no_temp_files_behind() -> TestResult { + let repo = Repo::open_or_create(temp_repo_dir()).await?; + repo.connect_key().await?; + let key = SecretKey::generate(&mut rand::rng()); + repo.save_listen_key_for_tunnel("proj", "tun", &key).await?; + repo.save_listen_key_for_tunnel("proj", "tun", &key).await?; + + assert!(leftover_temp_files(&repo.0).await?.is_empty()); + assert!( + leftover_temp_files(&repo.0.join("proj").join("tun")) + .await? + .is_empty() + ); + Ok(()) + } + + #[tokio::test] + async fn failed_key_write_cleans_up_its_temp_file() -> TestResult { + let repo = Repo::open_or_create(temp_repo_dir()).await?; + // Make the rename fail by putting a non-empty directory where the key + // file should go. + let key_path = repo.0.join(Repo::CONNECT_KEY_FILE); + tokio::fs::create_dir_all(key_path.join("occupied")).await?; + + let key = SecretKey::generate(&mut rand::rng()); + assert!(write_secret_key(&key_path, &key).await.is_err()); + assert!(leftover_temp_files(&repo.0).await?.is_empty()); + Ok(()) + } } #[cfg(test)]