diff --git a/Cargo.lock b/Cargo.lock index 5fffb3eb0f..5b98ec9471 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14489,6 +14489,7 @@ dependencies = [ "sysinfo", "tabwriter", "tantivy", + "tar", "tempfile", "thiserror 2.0.17", "thousands", diff --git a/app/Cargo.toml b/app/Cargo.toml index a88eee5de0..19862347cc 100644 --- a/app/Cargo.toml +++ b/app/Cargo.toml @@ -197,6 +197,7 @@ static_assertions.workspace = true string-offset.workspace = true sum_tree.workspace = true tabwriter = "1.4" +tar = "0.4" tempfile.workspace = true thiserror.workspace = true thousands = "0.2.0" diff --git a/app/src/remote_server/ssh_transport/installation/scp_fallback.rs b/app/src/remote_server/ssh_transport/installation/scp_fallback.rs index 94c7d94b1d..5ca2476489 100644 --- a/app/src/remote_server/ssh_transport/installation/scp_fallback.rs +++ b/app/src/remote_server/ssh_transport/installation/scp_fallback.rs @@ -1,7 +1,10 @@ +use std::fs::File; use std::path::{Path, PathBuf}; use std::time::Duration; use anyhow::Context as _; +use blocking::unblock; +use flate2::read::GzDecoder; use futures::AsyncWriteExt as _; use futures::TryStreamExt as _; use http_client::StatusCode; @@ -37,7 +40,7 @@ pub(super) async fn install(socket_path: &Path) -> Result<(), Error> { let client_tarball_path = cached_remote_server_tarball(&platform) .await - .map_err(Error::Other)?; + .map_err(|source| Error::ClientDownloadFailed { source })?; let timeout = remote_server::setup::SCP_INSTALL_TIMEOUT; let install_dir = remote_server::setup::remote_server_dir(); let remote_tarball_name = format!("oz-upload-{}.tar.gz", uuid::Uuid::new_v4()); @@ -96,10 +99,6 @@ fn remote_server_tarball_cache_root() -> PathBuf { .join("tarballs") } -fn remote_server_tarball_cache_temp_dir() -> PathBuf { - remote_server_tarball_cache_root().join(".tmp") -} - fn current_remote_server_tarball_cache_version() -> &'static str { remote_server::setup::remote_server_artifact_version() } @@ -116,9 +115,42 @@ fn remote_server_tarball_cache_path(platform: &RemotePlatform) -> PathBuf { } async fn is_valid_cached_tarball(path: &Path) -> bool { - async_fs::metadata(path) + let metadata_is_valid = async_fs::metadata(path) .await - .is_ok_and(|metadata| metadata.is_file() && metadata.len() > 0) + .is_ok_and(|metadata| metadata.is_file() && metadata.len() > 0); + if !metadata_is_valid { + return false; + } + + let path = path.to_path_buf(); + unblock(move || validate_gzip_tarball(&path)).await.is_ok() +} + +fn validate_gzip_tarball(path: &Path) -> anyhow::Result<()> { + let file = File::open(path) + .with_context(|| format!("Failed to open remote-server tarball '{}'", path.display()))?; + let decoder = GzDecoder::new(file); + let mut archive = tar::Archive::new(decoder); + let entries = archive + .entries() + .with_context(|| format!("Failed to read tar entries from '{}'", path.display()))?; + let mut entry_count = 0; + + for entry in entries { + let mut entry = + entry.with_context(|| format!("Failed to read tar entry from '{}'", path.display()))?; + std::io::copy(&mut entry, &mut std::io::sink()) + .with_context(|| format!("Failed to validate tar entry from '{}'", path.display()))?; + entry_count += 1; + } + + anyhow::ensure!( + entry_count > 0, + "Remote-server tarball '{}' contained no entries", + path.display() + ); + + Ok(()) } /// Returns a local tarball for the remote platform. @@ -127,25 +159,36 @@ async fn is_valid_cached_tarball(path: &Path) -> bool { /// tarball into the cache and returns the newly cached path. async fn cached_remote_server_tarball(platform: &RemotePlatform) -> anyhow::Result { let cache_path = remote_server_tarball_cache_path(platform); - if is_valid_cached_tarball(&cache_path).await { + let url = remote_server::setup::download_tarball_url(platform); + cached_remote_server_tarball_from(&url, &cache_path).await +} + +async fn cached_remote_server_tarball_from( + url: &str, + cache_path: &Path, +) -> anyhow::Result { + if is_valid_cached_tarball(cache_path).await { log::info!( "Using cached remote-server tarball at {}", cache_path.display() ); - return Ok(cache_path); + return Ok(cache_path.to_path_buf()); } - if async_fs::metadata(&cache_path).await.is_ok() { - let _ = async_fs::remove_file(&cache_path).await; + if async_fs::metadata(cache_path).await.is_ok() { + log::warn!( + "Discarding invalid cached remote-server tarball at {}", + cache_path.display() + ); + let _ = async_fs::remove_file(cache_path).await; } - let url = remote_server::setup::download_tarball_url(platform); log::info!( "Downloading remote-server tarball from {url} into cache at {}", cache_path.display() ); - download_remote_server_tarball_to_cache(&url, &cache_path).await?; - Ok(cache_path) + download_remote_server_tarball_to_cache(url, cache_path).await?; + Ok(cache_path.to_path_buf()) } async fn download_remote_server_tarball_to_cache( @@ -161,7 +204,7 @@ async fn download_remote_server_tarball_to_cache( parent.display() ) })?; - let temp_dir = remote_server_tarball_cache_temp_dir(); + let temp_dir = parent.join(".tmp"); async_fs::create_dir_all(&temp_dir).await.with_context(|| { format!( "Failed to create remote-server tarball cache temp directory '{}'", @@ -169,57 +212,24 @@ async fn download_remote_server_tarball_to_cache( ) })?; - // Download into a unique temp path first so a failed or partial download - // never appears at the shared cache path that other installs may reuse. - let temp_path = temp_dir.join(format!( - ".{REMOTE_SERVER_TARBALL_CACHE_FILE_NAME}.{}.tmp", - uuid::Uuid::new_v4() - )); - - if let Err(e) = download_remote_server_tarball_with_retries(url, &temp_path).await { - let _ = async_fs::remove_file(&temp_path).await; - return Err(e); - } - if !is_valid_cached_tarball(&temp_path).await { - let _ = async_fs::remove_file(&temp_path).await; - anyhow::bail!("Downloaded remote-server tarball from {url} was empty"); - } - - if is_valid_cached_tarball(cache_path).await { - let _ = async_fs::remove_file(&temp_path).await; - return Ok(()); - } - - // Publish the validated temp file to the shared cache path. If another - // concurrent fallback populated the cache after the check above, that valid - // cache hit is good enough for this install, so discard our temp file. - match async_fs::rename(&temp_path, cache_path).await { - Ok(()) => Ok(()), - Err(e) if is_valid_cached_tarball(cache_path).await => { - let _ = async_fs::remove_file(&temp_path).await; - Ok(()) - } - Err(e) => { - let _ = async_fs::remove_file(&temp_path).await; - Err(e).with_context(|| { - format!( - "Failed to move remote-server tarball into cache at '{}'", - cache_path.display() - ) - }) - } - } -} - -async fn download_remote_server_tarball_with_retries( - url: &str, - temp_path: &Path, -) -> anyhow::Result<()> { let http_client = http_client::Client::new(); let mut last_retryable_error = None; for attempt in 1..=REMOTE_SERVER_TARBALL_DOWNLOAD_ATTEMPTS { - match download_remote_server_tarball_internal(&http_client, url, temp_path).await { + // Download into a fresh unique temp path for every attempt so a failed + // or partial response body can never be reused by a later retry. + let temp_path = temp_dir.join(format!( + ".{REMOTE_SERVER_TARBALL_CACHE_FILE_NAME}.{}.tmp", + uuid::Uuid::new_v4() + )); + + let attempt_result = + download_remote_server_tarball_attempt(&http_client, url, cache_path, &temp_path).await; + if attempt_result.is_err() { + let _ = async_fs::remove_file(&temp_path).await; + } + + match attempt_result { Ok(()) => return Ok(()), Err(DownloadAttemptError::Permanent(e)) => return Err(e), Err(DownloadAttemptError::Retryable(e)) => { @@ -232,15 +242,60 @@ async fn download_remote_server_tarball_with_retries( } } - Err(last_retryable_error.unwrap_or_else(|| { - anyhow::anyhow!("Remote-server tarball download failed without an error") - })) + Err(last_retryable_error + .unwrap_or_else(|| anyhow::anyhow!("Remote-server tarball download failed without an error")) + .context(format!( + "Remote-server tarball client download failed after {REMOTE_SERVER_TARBALL_DOWNLOAD_ATTEMPTS} attempts" + ))) +} + +async fn download_remote_server_tarball_attempt( + http_client: &http_client::Client, + url: &str, + cache_path: &Path, + temp_path: &Path, +) -> Result<(), DownloadAttemptError> { + download_remote_server_tarball_internal(http_client, url, temp_path).await?; + if !is_valid_cached_tarball(temp_path).await { + return Err(DownloadAttemptError::Retryable(anyhow::anyhow!( + "Downloaded remote-server tarball from {url} was not a valid gzip/tar archive" + ))); + } + + publish_remote_server_tarball_cache(temp_path, cache_path).await } enum DownloadAttemptError { Retryable(anyhow::Error), Permanent(anyhow::Error), } +async fn publish_remote_server_tarball_cache( + temp_path: &Path, + cache_path: &Path, +) -> Result<(), DownloadAttemptError> { + if is_valid_cached_tarball(cache_path).await { + let _ = async_fs::remove_file(temp_path).await; + return Ok(()); + } + + // Publish the validated temp file to the shared cache path. If another + // concurrent fallback populated the cache after the check above, that valid + // cache hit is good enough for this install, so discard our temp file. + match async_fs::rename(temp_path, cache_path).await { + Ok(()) => Ok(()), + Err(e) if is_valid_cached_tarball(cache_path).await => { + let _ = async_fs::remove_file(temp_path).await; + Ok(()) + } + Err(e) => { + let _ = async_fs::remove_file(temp_path).await; + Err(DownloadAttemptError::Permanent(anyhow::anyhow!( + "Failed to move remote-server tarball into cache at '{}': {e}", + cache_path.display() + ))) + } + } +} async fn download_remote_server_tarball_internal( http_client: &http_client::Client, @@ -305,3 +360,7 @@ fn is_retryable_download_status(status: StatusCode) -> bool { StatusCode::REQUEST_TIMEOUT | StatusCode::TOO_MANY_REQUESTS ) || status.is_server_error() } + +#[cfg(test)] +#[path = "scp_fallback_tests.rs"] +mod tests; diff --git a/app/src/remote_server/ssh_transport/installation/scp_fallback_tests.rs b/app/src/remote_server/ssh_transport/installation/scp_fallback_tests.rs new file mode 100644 index 0000000000..e9c7a8b5ed --- /dev/null +++ b/app/src/remote_server/ssh_transport/installation/scp_fallback_tests.rs @@ -0,0 +1,169 @@ +use std::fs; +use std::io::Write as _; + +use flate2::write::GzEncoder; +use flate2::Compression; +use mockito::Server; +use tempfile::tempdir; + +use super::*; + +fn valid_tarball() -> Vec { + let mut encoder = GzEncoder::new(Vec::new(), Compression::default()); + { + let mut archive = tar::Builder::new(&mut encoder); + let mut header = tar::Header::new_gnu(); + let body = b"fake remote server"; + header.set_path("oz").unwrap(); + header.set_size(body.len() as u64); + header.set_mode(0o755); + header.set_cksum(); + archive.append(&header, &body[..]).unwrap(); + archive.finish().unwrap(); + } + encoder.finish().unwrap() +} + +fn invalid_tarball() -> Vec { + b"not a gzip tarball".to_vec() +} + +fn cache_path(tempdir: &tempfile::TempDir) -> PathBuf { + tempdir.path().join("cache").join("oz.tar.gz") +} + +fn cache_temp_dir(tempdir: &tempfile::TempDir) -> PathBuf { + tempdir.path().join("cache").join(".tmp") +} + +#[tokio::test] +async fn retry_uses_clean_temp_file_after_invalid_download() { + let tempdir = tempdir().unwrap(); + let cache_path = cache_path(&tempdir); + let mut server = Server::new_async().await; + let invalid = invalid_tarball(); + let valid = valid_tarball(); + let first_attempt = server + .mock("GET", "/oz.tar.gz") + .with_status(200) + .with_body(invalid) + .expect(1) + .create_async() + .await; + let retry_attempt = server + .mock("GET", "/oz.tar.gz") + .with_status(200) + .with_body(valid.clone()) + .expect(1) + .create_async() + .await; + + download_remote_server_tarball_to_cache(&format!("{}/oz.tar.gz", server.url()), &cache_path) + .await + .unwrap(); + + first_attempt.assert_async().await; + retry_attempt.assert_async().await; + assert_eq!(fs::read(&cache_path).unwrap(), valid); + assert!(is_valid_cached_tarball(&cache_path).await); + assert!(fs::read_dir(cache_temp_dir(&tempdir)) + .unwrap() + .next() + .is_none()); +} + +#[tokio::test] +async fn invalid_cached_tarball_is_discarded_and_redownloaded() { + let tempdir = tempdir().unwrap(); + let cache_path = cache_path(&tempdir); + fs::create_dir_all(cache_path.parent().unwrap()).unwrap(); + fs::write(&cache_path, invalid_tarball()).unwrap(); + + let mut server = Server::new_async().await; + let valid = valid_tarball(); + let download = server + .mock("GET", "/oz.tar.gz") + .with_status(200) + .with_body(valid.clone()) + .expect(1) + .create_async() + .await; + + let cached_path = + cached_remote_server_tarball_from(&format!("{}/oz.tar.gz", server.url()), &cache_path) + .await + .unwrap(); + + download.assert_async().await; + assert_eq!(cached_path, cache_path); + assert_eq!(fs::read(&cache_path).unwrap(), valid); + assert!(is_valid_cached_tarball(&cache_path).await); +} + +#[tokio::test] +async fn valid_cached_tarball_is_reused_without_download() { + let tempdir = tempdir().unwrap(); + let cache_path = cache_path(&tempdir); + let valid = valid_tarball(); + fs::create_dir_all(cache_path.parent().unwrap()).unwrap(); + fs::write(&cache_path, &valid).unwrap(); + + let mut server = Server::new_async().await; + let unexpected_download = server + .mock("GET", "/oz.tar.gz") + .with_status(500) + .expect(0) + .create_async() + .await; + + let cached_path = + cached_remote_server_tarball_from(&format!("{}/oz.tar.gz", server.url()), &cache_path) + .await + .unwrap(); + + unexpected_download.assert_async().await; + assert_eq!(cached_path, cache_path); + assert_eq!(fs::read(&cache_path).unwrap(), valid); +} + +#[tokio::test] +async fn retry_exhaustion_returns_client_download_error_without_cache_file() { + let tempdir = tempdir().unwrap(); + let cache_path = cache_path(&tempdir); + let mut server = Server::new_async().await; + let invalid = invalid_tarball(); + let invalid_downloads = server + .mock("GET", "/oz.tar.gz") + .with_status(200) + .with_body(invalid) + .expect(REMOTE_SERVER_TARBALL_DOWNLOAD_ATTEMPTS) + .create_async() + .await; + + let err = download_remote_server_tarball_to_cache( + &format!("{}/oz.tar.gz", server.url()), + &cache_path, + ) + .await + .unwrap_err(); + + invalid_downloads.assert_async().await; + assert!(err + .to_string() + .contains("Remote-server tarball client download failed after 3 attempts")); + assert!(!cache_path.exists()); + assert!(fs::read_dir(cache_temp_dir(&tempdir)) + .unwrap() + .next() + .is_none()); +} + +#[test] +fn invalid_cached_tarball_fails_gzip_tar_validation() { + let tempdir = tempdir().unwrap(); + let invalid_path = tempdir.path().join("oz.tar.gz"); + let mut file = fs::File::create(&invalid_path).unwrap(); + file.write_all(&invalid_tarball()).unwrap(); + + assert!(validate_gzip_tarball(&invalid_path).is_err()); +} diff --git a/crates/remote_server/src/setup.rs b/crates/remote_server/src/setup.rs index 3270b50bf3..e11097fc11 100644 --- a/crates/remote_server/src/setup.rs +++ b/crates/remote_server/src/setup.rs @@ -132,6 +132,7 @@ impl UnsupportedReason { } crate::transport::Error::TimedOut | crate::transport::Error::ScriptFailed { .. } + | crate::transport::Error::ClientDownloadFailed { .. } | crate::transport::Error::Other(_) => None, } } diff --git a/crates/remote_server/src/transport.rs b/crates/remote_server/src/transport.rs index c3e9ce9a12..4024346279 100644 --- a/crates/remote_server/src/transport.rs +++ b/crates/remote_server/src/transport.rs @@ -96,6 +96,10 @@ pub enum Error { /// A remote script ran but exited with a non-zero code. #[error("script failed (exit {exit_code}): {stderr}")] ScriptFailed { exit_code: i32, stderr: String }, + /// The client-side SCP fallback could not download a valid tarball before + /// uploading it to the remote host. + #[error("client download failed: {source}")] + ClientDownloadFailed { source: anyhow::Error }, /// Any other transport-level or unexpected failure. #[error(transparent)] Other(anyhow::Error), @@ -131,6 +135,9 @@ impl Error { }; Some(format!("Script exited with code {exit_code}: {truncated}")) } + Self::ClientDownloadFailed { source } => Some(format!( + "Failed to download SSH extension on this computer: {source}" + )), Self::Other(_) => None, }; UserFacingError { body, detail }