From 57d8f2690e776b84c6c9050b575b1fb6efc988e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Dani=C4=87?= Date: Wed, 9 Sep 2026 08:24:33 +0000 Subject: [PATCH 1/2] Reuse the on-host archive when the spec names a prior deployment When a deployment spec references a prior deployment on the same host, DownloadBundle copies that deployment's unpacked archive and hard-links its bundle instead of downloading and extracting the revision again, which makes repeated deployments of the same revision substantially cheaper. Enabled by default; set enable_archive_reuse: false to always download. - Reuse falls back to a normal download whenever the referenced archive is missing, incomplete, or malformed, and never leaves a partial archive behind. - The referenced deployment id is validated before touching disk, and the AppSpec path is rejected if it climbs out of the archive. - Bundle-content hardening checks still run on every archive, reused or downloaded, as defense in depth. - The bundle ETag is carried forward on reuse so hooks keep seeing BUNDLE_ETAG even when the spec omits it. - Deployment diagnostics report whether the bundle was downloaded or reused, for observability. Bump the agent version to 2.1.0. --- Cargo.toml | 2 +- conf/codedeployagent.yml | 5 + crates/codedeploy-commands/Cargo.toml | 2 +- src/command_poller/command_processor.rs | 32 +- src/config/mod.rs | 24 + src/deployment_specification/builder.rs | 148 +++++ src/deployment_specification/types.rs | 1 + src/host_command/archive_reuse.rs | 572 +++++++++++++++++++ src/host_command/command_dispatcher.rs | 7 + src/host_command/commands/download_bundle.rs | 323 +++++++++-- src/host_command/commands/hook.rs | 1 + src/host_command/commands/install.rs | 1 + src/host_command/mod.rs | 60 ++ src/lifecycle_event/executor.rs | 1 + src/main.rs | 3 + src/system/file_ops.rs | 115 ++++ tests/end_to_end.rs | 3 + tests/hook_execution.rs | 1 + tests/security/intake/deployment_flow.rs | 2 + 19 files changed, 1262 insertions(+), 41 deletions(-) create mode 100644 src/host_command/archive_reuse.rs diff --git a/Cargo.toml b/Cargo.toml index 1806f534..91aca3a2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,7 +7,7 @@ edition = "2024" rust-version = "1.91" homepage = "https://aws.amazon.com/codedeploy/" repository = "https://github.com/aws/aws-codedeploy-agent" -version = "2.0.1" +version = "2.1.0" description = "AWS CodeDeploy agent responsible for deploying software on an individual EC2/On-prem instance" license = "Apache-2.0" authors = ["Amazon Web Services"] diff --git a/conf/codedeployagent.yml b/conf/codedeployagent.yml index 578bc309..40f6f73a 100644 --- a/conf/codedeployagent.yml +++ b/conf/codedeployagent.yml @@ -32,6 +32,11 @@ :max_revisions: 5 :enable_deployments_log: true +# Reuse a previous deployment's on-host archive when the deployment spec names +# one, instead of downloading identical bytes again (restart deployments). +# Any reuse failure falls back to a normal download. +:enable_archive_reuse: true + # Security :use_fips_mode: false :enable_auth_policy: false diff --git a/crates/codedeploy-commands/Cargo.toml b/crates/codedeploy-commands/Cargo.toml index 435c0e5f..72923d57 100644 --- a/crates/codedeploy-commands/Cargo.toml +++ b/crates/codedeploy-commands/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codedeploy-commands" -version = "2.0.1" +version = "2.1.0" edition = "2024" rust-version = "1.91" publish = false diff --git a/src/command_poller/command_processor.rs b/src/command_poller/command_processor.rs index 8f3ded12..9b28e36f 100644 --- a/src/command_poller/command_processor.rs +++ b/src/command_poller/command_processor.rs @@ -232,7 +232,8 @@ impl CommandProcessor { elapsed_ms = elapsed.as_millis().try_into().unwrap_or(u64::MAX), "Command succeeded" ); - self.report_completion(command, "Succeeded", &diagnostics::success("")); + let note = self.completion_note(&command.command_name, spec); + self.report_completion(command, "Succeeded", &diagnostics::success(¬e)); }, Err(e) => { error!( @@ -257,6 +258,34 @@ impl CommandProcessor { result.map(|_| ()) } + /// Detail the service can parse off a successful completion. Empty for commands that have none, + /// which keeps their diagnostics byte for byte what they were. + /// + /// Read back from the `.bundle-source` marker `DownloadBundle` has just written, rather than + /// returned up through the dispatcher: the marker is already the single record of where the + /// bundle came from, so reading it cannot disagree with what the host actually did. A missing + /// marker yields no note -- writing it is best-effort, and a metric is not worth failing a + /// deployment over. + fn completion_note(&self, command_name: &str, spec: &DeploymentSpec) -> String { + if command_name != "DownloadBundle" { + return String::new(); + } + + let marker = self + .dispatcher + .archives() + .deployment_root_dir(&spec.deployment_group_id, &spec.deployment_id) + .join(crate::host_command::BUNDLE_SOURCE_FILE); + + match std::fs::read_to_string(&marker) { + Ok(source) => crate::host_command::bundle_source_note(source.trim()), + Err(e) => { + debug!(path = %marker.display(), "No bundle source marker to report: {e}"); + String::new() + }, + } + } + fn report_completion(&self, command: &HostCommand, status: &str, payload: &str) { debug!("Calling PutHostCommandComplete: \"{status}\""); if let Err(e) = @@ -341,6 +370,7 @@ mod tests { bundle_type: "tar".into(), }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, } } diff --git a/src/config/mod.rs b/src/config/mod.rs index 2b4938b5..64271593 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -319,6 +319,12 @@ pub struct AgentConfig { /// Enable the local command port for debugging. Default: `false`. pub enable_command_port: bool, + /// Allow reusing a previous deployment's on-host archive when the deployment + /// spec carries `ReuseArchiveFromDeploymentId` (bounce/restart deployments). + /// Default: `true`. Any reuse failure falls back to a normal download, so + /// turning this off costs only the optimisation. + pub enable_archive_reuse: bool, + /// Capture Amazon S3 HTTP wire logs to `.aws_wire.log` in /// `log_dir`. Default: `false`. /// @@ -384,6 +390,7 @@ impl Default for AgentConfig { s3_endpoint_override: None, disable_imds_v1: false, enable_command_port: false, + enable_archive_reuse: true, log_aws_wire: false, disable_core_dumps: true, hardening: HardeningConfig::default(), @@ -902,6 +909,23 @@ mod tests { assert!(!AgentConfig::default().enable_command_port); } + #[test] + fn default_enable_archive_reuse_is_true() { + assert!(AgentConfig::default().enable_archive_reuse); + } + + #[test] + fn enable_archive_reuse_parses_from_yaml() { + let yaml = "enable_archive_reuse: false\n"; + let config = AgentConfig::from_yaml(yaml, Path::new("test.yml")).unwrap(); + assert!(!config.enable_archive_reuse); + + // Ruby `:key:` form must also parse. + let yaml = ":enable_archive_reuse: false\n"; + let config = AgentConfig::from_yaml(yaml, Path::new("test.yml")).unwrap(); + assert!(!config.enable_archive_reuse); + } + #[test] fn default_reject_symlinks_in_bundle_is_false() { assert!(!AgentConfig::default().hardening.reject_symlinks_in_bundle); diff --git a/src/deployment_specification/builder.rs b/src/deployment_specification/builder.rs index 2c93301f..32881114 100644 --- a/src/deployment_specification/builder.rs +++ b/src/deployment_specification/builder.rs @@ -66,6 +66,23 @@ pub(super) fn build( let deployment_id = extract_deployment_id(data["DeploymentId"].as_str().unwrap()); let deployment_group_id = data["DeploymentGroupId"].as_str().unwrap().to_string(); + // Both values become directory names under the deployment root + // (DeploymentArchives::deployment_root_dir joins them), and `Path::join` treats + // `..` as an ordinary parent component. Reject anything that is not a single + // safe component here, at the one place every consumer flows through, rather + // than at each of the three call sites that build paths from them. Fails + // closed: a spec carrying an unsafe value is rejected, not redirected. + for (field, value) in [ + ("DeploymentId", &deployment_id), + ("DeploymentGroupId", &deployment_group_id), + ] { + if !crate::system::file_ops::is_safe_path_component(value) { + return Err(DeploymentSpecError::ParseError(format!( + "{field} is not a valid path component: {value:?}" + ))); + } + } + let deployment_creator = data .get("DeploymentCreator") .and_then(|v| v.as_str()) @@ -84,6 +101,17 @@ pub(super) fn build( .unwrap_or(DEFAULT_APP_SPEC_PATH) .to_string(); + // The AppSpec path is joined to the unpacked archive directory by DownloadBundle, Install and + // the lifecycle-event executor, and `Path::join` treats `..` as an ordinary parent component and + // an absolute path as a replacement for the whole join. Validate it here, at the same choke + // point as the two IDs above, so every consumer inherits the guarantee rather than each having + // to repeat the check. Nested paths stay legal; climbing out does not. + if !crate::system::file_ops::is_safe_relative_path(&app_spec_path) { + return Err(DeploymentSpecError::ParseError(format!( + "AppSpecFilename is not a safe revision-relative path: {app_spec_path:?}" + ))); + } + let file_exists_behavior = data .get("AgentActionOverrides") .and_then(|overrides| overrides.get("AgentOverrides")) @@ -98,6 +126,15 @@ pub(super) fn build( .collect::>() }); + // Resolved through extract_deployment_id for parity with DeploymentId, so an + // ARN-form value normalises to the short form. Format validation happens at + // the point of use (see host_command::archive_reuse). + let reuse_archive_from_deployment_id = data + .get("ReuseArchiveFromDeploymentId") + .and_then(|v| v.as_str()) + .filter(|s| !s.is_empty()) + .map(extract_deployment_id); + Ok(DeploymentSpec { deployment_id, deployment_group_id, @@ -110,6 +147,7 @@ pub(super) fn build( revision_source, revision, all_possible_lifecycle_events, + reuse_archive_from_deployment_id, }) } @@ -119,6 +157,116 @@ mod tests { use crate::deployment_specification::types::{RevisionLocation, RevisionSource}; use serde_json::json; + fn minimal_revision() -> (RevisionSource, RevisionLocation) { + ( + RevisionSource::S3, + RevisionLocation::S3 { + bucket: "bucket".to_string(), + key: "key".to_string(), + bundle_type: "tar".to_string(), + version: None, + etag: None, + }, + ) + } + + #[test] + fn rejects_traversal_in_deployment_group_id() { + let data = json!({ + "DeploymentId": "d-12345678", + "DeploymentGroupId": "../../../../tmp/evil", + "DeploymentGroupName": "MyGroup", + "ApplicationName": "MyApp" + }); + let (src, rev) = minimal_revision(); + let err = build(&data, src, rev).unwrap_err(); + assert!( + err.to_string().contains("DeploymentGroupId is not a valid path component"), + "got: {err}" + ); + } + + /// The `AppSpec` path is joined to the unpacked archive by `DownloadBundle`, `Install` and the + /// lifecycle-event executor, and Install parses whatever it names -- so a climbing path would be + /// read and parsed as an `AppSpec`, not merely probed for existence. + #[test] + fn rejects_traversal_in_app_spec_filename() { + for bad in [ + "../../etc/passwd", + "/etc/shadow", + "configs/../../appspec.yml", + ] { + let data = json!({ + "DeploymentId": "d-12345678", + "DeploymentGroupId": "dg-12345678", + "DeploymentGroupName": "MyGroup", + "ApplicationName": "MyApp", + "AppSpecFilename": bad + }); + let (src, rev) = minimal_revision(); + let err = build(&data, src, rev).unwrap_err(); + assert!( + err.to_string().contains("AppSpecFilename is not a safe revision-relative path"), + "must reject {bad:?}, got: {err}" + ); + } + } + + #[test] + fn accepts_a_nested_app_spec_filename() { + let data = json!({ + "DeploymentId": "d-12345678", + "DeploymentGroupId": "dg-12345678", + "DeploymentGroupName": "MyGroup", + "ApplicationName": "MyApp", + "AppSpecFilename": "configs/appspec.yml" + }); + let (src, rev) = minimal_revision(); + let spec = build(&data, src, rev).unwrap(); + assert_eq!(spec.app_spec_path, "configs/appspec.yml"); + } + + #[test] + fn rejects_traversal_in_deployment_id() { + let data = json!({ + "DeploymentId": "..", + "DeploymentGroupId": "dg-12345678", + "DeploymentGroupName": "MyGroup", + "ApplicationName": "MyApp" + }); + let (src, rev) = minimal_revision(); + let err = build(&data, src, rev).unwrap_err(); + assert!( + err.to_string().contains("DeploymentId is not a valid path component"), + "got: {err}" + ); + } + + #[test] + fn rejects_separator_in_deployment_id() { + let data = json!({ + "DeploymentId": "d-A/../../etc", + "DeploymentGroupId": "dg-12345678", + "DeploymentGroupName": "MyGroup", + "ApplicationName": "MyApp" + }); + let (src, rev) = minimal_revision(); + assert!(build(&data, src, rev).is_err()); + } + + #[test] + fn accepts_uuid_shaped_deployment_group_id() { + let data = json!({ + "DeploymentId": "d-12345678", + "DeploymentGroupId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", + "DeploymentGroupName": "MyGroup", + "ApplicationName": "MyApp" + }); + let (src, rev) = minimal_revision(); + let spec = build(&data, src, rev).unwrap(); + assert_eq!(spec.deployment_group_id, "f47ac10b-58cc-4372-a567-0e02b2c3d479"); + } + #[test] fn build_minimal() { let data = json!({ diff --git a/src/deployment_specification/types.rs b/src/deployment_specification/types.rs index 36a4515e..b9bb875c 100644 --- a/src/deployment_specification/types.rs +++ b/src/deployment_specification/types.rs @@ -55,6 +55,7 @@ pub struct DeploymentSpec { pub revision_source: RevisionSource, pub revision: RevisionLocation, pub all_possible_lifecycle_events: Option>, + pub reuse_archive_from_deployment_id: Option, } #[derive(Debug, Clone)] diff --git a/src/host_command/archive_reuse.rs b/src/host_command/archive_reuse.rs new file mode 100644 index 00000000..afb1c597 --- /dev/null +++ b/src/host_command/archive_reuse.rs @@ -0,0 +1,572 @@ +//! +//! Archive reuse for bounce (restart) deployments. +//! +//! When the deployment spec carries `ReuseArchiveFromDeploymentId`, the bundle for +//! the new deployment can be materialised from the referenced deployment's on-host +//! archive instead of being downloaded again — the point of a bounce. +//! +//! Two rules govern everything here: +//! +//! 1. **Fail closed to a download.** Every failure path returns an error so the +//! caller falls back to the normal (revision-pinned) download. Reuse is an +//! optimisation; it is never the only way to get the bytes. +//! 2. **Never leave a partial archive.** A half-copied archive that got deployed +//! would be worse than any download. On failure the destination is torn down +//! before returning, so the fallback starts from clean state. +//! +//! Hard links and copies, never symlinks: `cleanup_old_archives` prunes the source +//! deployment once `last_successful` moves on, which would leave a symlinked archive +//! dangling. A hard link keeps the bundle bytes alive independently of the source +//! directory entry. + +use crate::host_command::DeploymentArchives; +use std::fs; +use std::io; +use std::path::Path; +use tracing::{debug, warn}; + +/// `BUNDLE_SOURCE_FILE` value when the archive was reused from a prior deployment. +pub const SOURCE_ARCHIVE_REUSE: &str = "archive-reuse"; + +/// `BUNDLE_SOURCE_FILE` value when the bundle was fetched from its revision source. +pub const SOURCE_DOWNLOADED: &str = "downloaded"; + +/// Upper bound on a deployment ID, so a hostile value cannot build an absurd path. +/// Real IDs are `d-` plus nine characters; this leaves generous headroom. +const MAX_DEPLOYMENT_ID_LEN: usize = 64; + +/// Whether `id` is a well-formed deployment ID that is safe to use as a single +/// path component. +/// +/// Accepts `d-` followed by one or more uppercase-alphanumeric characters. That +/// rejects everything dangerous for path construction — `/`, `\`, `..`, `.`, NUL, +/// absolute paths, empty strings — by allowing only a known-good character set +/// rather than blocklisting separators. +/// +/// A `false` result means "do not reuse", never "fail the deployment". +#[must_use] +pub fn is_valid_deployment_id(id: &str) -> bool { + let Some(suffix) = id.strip_prefix("d-") else { + return false; + }; + !suffix.is_empty() + && id.len() <= MAX_DEPLOYMENT_ID_LEN + && suffix.chars().all(|c| c.is_ascii_uppercase() || c.is_ascii_digit()) +} + +/// Materialise the new deployment's bundle from `source_deployment_id`'s archive. +/// +/// On success the destination holds a `deployment-archive/` copy and, when the +/// source had one, a hard-linked `bundle.tar`. The caller may then proceed exactly +/// as if it had downloaded, including the `AppSpec` presence check. +/// +/// # Errors +/// Returns an error — after removing any partial destination state — if the ID is +/// malformed, the source archive is missing or unusable, or any filesystem +/// operation fails. Every such error means "fall back to downloading". +pub fn reuse_archive( + archives: &DeploymentArchives, + group_id: &str, + source_deployment_id: &str, + dest_deployment_id: &str, + app_spec_path: &str, +) -> io::Result<()> { + if !is_valid_deployment_id(source_deployment_id) { + return Err(io::Error::other(format!( + "Refusing to reuse archive: {source_deployment_id:?} is not a well-formed deployment ID" + ))); + } + // Guard against reusing our own directory: the caller wipes the destination + // archive, which for a self-reference would destroy the very source. + if source_deployment_id == dest_deployment_id { + return Err(io::Error::other( + "Refusing to reuse archive: source and destination deployment are the same", + )); + } + + let source_archive = archives.archive_dir(group_id, source_deployment_id); + let source_bundle = archives.artifact_bundle_path(group_id, source_deployment_id); + let dest_archive = archives.archive_dir(group_id, dest_deployment_id); + let dest_bundle = archives.artifact_bundle_path(group_id, dest_deployment_id); + + // Completeness check before touching the destination. A pruned or half-written + // source is the expected case on a host that joined after the source deployment, + // or once retention has caught up with it. + if !source_archive.is_dir() { + return Err(io::Error::other(format!( + "Cannot reuse archive: {} is not a directory", + source_archive.display() + ))); + } + if fs::read_dir(&source_archive)?.next().is_none() { + return Err(io::Error::other(format!( + "Cannot reuse archive: {} is empty", + source_archive.display() + ))); + } + + // An archive can exist yet be unusable -- interrupted copy, partially pruned + // tree. Without this the deployment would proceed and then fail on the + // AppSpec check downstream, which fails the host instead of falling back to a + // download. Treat a missing AppSpec as "incomplete, do not reuse". + // Defence in depth: the spec builder already rejects an unsafe AppSpec path at ingest, but this + // function is reached with a caller-supplied value and builds a path from it, so it does not + // rely on that. Same shared check, so the two paths cannot drift apart. + if !crate::system::file_ops::is_safe_relative_path(app_spec_path) { + return Err(io::Error::other(format!( + "Cannot reuse archive: unsafe AppSpec path {app_spec_path:?}" + ))); + } + + let source_appspec = source_archive.join(app_spec_path); + if !source_appspec.exists() { + return Err(io::Error::other(format!( + "Cannot reuse archive: incomplete, no AppSpec at {}", + source_appspec.display() + ))); + } + + // Defence in depth: the validated ID cannot traverse, but confirm the resolved + // source really sits under this group's directory before copying from it. + let group_dir = archives.deployment_root_dir(group_id, "").canonicalize()?; + let resolved_source = source_archive.canonicalize()?; + if !resolved_source.starts_with(&group_dir) { + return Err(io::Error::other(format!( + "Cannot reuse archive: {} resolves outside {}", + resolved_source.display(), + group_dir.display() + ))); + } + + match copy_into_place(&source_archive, &source_bundle, &dest_archive, &dest_bundle) { + Ok(()) => { + carry_bundle_etag_forward(archives, group_id, source_deployment_id, dest_deployment_id); + debug!( + source = %source_deployment_id, + dest = %dest_deployment_id, + "Reused deployment archive" + ); + Ok(()) + }, + Err(e) => { + // Tear down whatever we managed to write, so the download fallback does + // not inherit a partial archive. + warn!( + source = %source_deployment_id, + "Archive reuse failed, discarding partial state: {e}" + ); + if dest_archive.exists() + && let Err(rm) = fs::remove_dir_all(&dest_archive) + { + warn!(path = %dest_archive.display(), "Failed to clean partial archive: {rm}"); + } + if dest_bundle.exists() + && let Err(rm) = fs::remove_file(&dest_bundle) + { + warn!(path = %dest_bundle.display(), "Failed to clean partial bundle: {rm}"); + } + Err(e) + }, + } +} + +/// Copy the source deployment's `.bundle-etag` alongside the reused archive. +/// +/// The marker is how `LifecycleEventExecutor` resolves `BUNDLE_ETAG` for hooks when the deployment +/// spec carries no eTag of its own, which is the common case. Reuse does not download, so without +/// this a restart would leave hooks with `BUNDLE_ETAG` unset even though the revision is byte for +/// byte the one the source deployment recorded -- a behaviour change for customer scripts, not an +/// intended part of the reuse optimisation. Copying it also keeps a chain of bounces intact, each +/// handing the value on. +/// +/// Best-effort: `BUNDLE_ETAG` is diagnostic metadata, so a missing or unreadable marker must not +/// fail an otherwise healthy deployment. +fn carry_bundle_etag_forward( + archives: &DeploymentArchives, + group_id: &str, + source_deployment_id: &str, + dest_deployment_id: &str, +) { + let source = archives + .deployment_root_dir(group_id, source_deployment_id) + .join(crate::host_command::BUNDLE_ETAG_FILE); + if !source.exists() { + return; + } + + let dest = archives + .deployment_root_dir(group_id, dest_deployment_id) + .join(crate::host_command::BUNDLE_ETAG_FILE); + if let Err(e) = fs::copy(&source, &dest) { + warn!( + source = %source.display(), + dest = %dest.display(), + "Failed to carry the bundle ETag forward to the reused deployment: {e}" + ); + } +} + +fn copy_into_place( + source_archive: &Path, + source_bundle: &Path, + dest_archive: &Path, + dest_bundle: &Path, +) -> io::Result<()> { + if let Some(parent) = dest_archive.parent() { + fs::create_dir_all(parent)?; + } + if dest_archive.exists() { + fs::remove_dir_all(dest_archive)?; + } + crate::system::file_ops::copy_dir_recursive(source_archive, dest_archive)?; + + // A LocalDirectory revision never produces a bundle.tar, so its absence is + // normal rather than an error — the archive alone is what Install consumes. + // Clear any destination bundle first, unconditionally. An earlier failed attempt on + // this same deployment can have left a bundle.tar behind, and when the source has no + // bundle of its own we would otherwise keep the stale one next to a freshly copied + // archive -- a mismatched pair, while still returning Ok. That would break this + // module's "never leave a partial archive" guarantee. + if dest_bundle.exists() { + fs::remove_file(dest_bundle)?; + } + + if source_bundle.exists() { + fs::hard_link(source_bundle, dest_bundle)?; + } + Ok(()) +} + +/// Record how the bundle was obtained, for per-host diagnostics. Best-effort: a +/// write failure must not fail an otherwise healthy deployment. +pub fn record_bundle_source(deploy_dir: &Path, source: &str, restrict_permissions: bool) { + let path = deploy_dir.join(crate::host_command::BUNDLE_SOURCE_FILE); + if let Err(e) = crate::system::write_file_secure( + &path, + source.as_bytes(), + crate::system::agent_file_mode(restrict_permissions), + ) { + warn!(path = %path.display(), "Failed to record bundle source marker: {e}"); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + fn archives(dir: &TempDir) -> DeploymentArchives { + let root = dir.path().join("deployments"); + let instructions = dir.path().join("instructions"); + fs::create_dir_all(&root).unwrap(); + fs::create_dir_all(&instructions).unwrap(); + DeploymentArchives::new(root, instructions, 5) + } + + /// Build a usable source deployment: archive dir with an appspec, plus a bundle. + fn seed_source(a: &DeploymentArchives, group: &str, id: &str) { + let archive = a.archive_dir(group, id); + fs::create_dir_all(archive.join("scripts")).unwrap(); + fs::write(archive.join("appspec.yml"), "version: 0.0\nos: linux\n").unwrap(); + fs::write(archive.join("scripts/start.sh"), "#!/bin/sh\n").unwrap(); + fs::write(a.artifact_bundle_path(group, id), "bundle-bytes").unwrap(); + } + + #[test] + fn accepts_real_deployment_id_shapes() { + assert!(is_valid_deployment_id("d-A1B2C3D4E")); + assert!(is_valid_deployment_id("d-0")); + assert!(is_valid_deployment_id("d-ABCDEFGHI")); + } + + #[test] + fn rejects_path_separators_and_traversal() { + for bad in [ + "d-../../etc", + "d-A/B", + "d-A\\B", + "../d-ABC", + "/d-ABC", + "d-A.B", + "d-", + "", + "d-abc", // lowercase is not a real ID shape + "not-an-id", + "d-A B", + "d-A\0B", + ] { + assert!(!is_valid_deployment_id(bad), "should reject {bad:?}"); + } + } + + #[test] + fn rejects_absurdly_long_id() { + let long = format!("d-{}", "A".repeat(MAX_DEPLOYMENT_ID_LEN)); + assert!(!is_valid_deployment_id(&long)); + } + + #[test] + fn reuses_archive_and_hard_links_bundle() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + seed_source(&a, "dg-1", "d-PREV"); + + reuse_archive(&a, "dg-1", "d-PREV", "d-NEW", "appspec.yml").unwrap(); + + let dest = a.archive_dir("dg-1", "d-NEW"); + assert!(dest.join("appspec.yml").exists(), "appspec must be present for Install"); + assert!(dest.join("scripts/start.sh").exists(), "nested content must be copied"); + assert!(a.artifact_bundle_path("dg-1", "d-NEW").exists()); + + // Hard link, not symlink — a symlink would dangle once the source is pruned. + let bundle = a.artifact_bundle_path("dg-1", "d-NEW"); + assert!(!bundle.is_symlink(), "bundle must not be a symlink"); + assert_eq!(fs::read_to_string(&bundle).unwrap(), "bundle-bytes"); + } + + #[test] + fn reused_archive_survives_source_pruning() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + seed_source(&a, "dg-1", "d-PREV"); + + reuse_archive(&a, "dg-1", "d-PREV", "d-NEW", "appspec.yml").unwrap(); + + // Simulate cleanup_old_archives reclaiming the source deployment. + fs::remove_dir_all(a.deployment_root_dir("dg-1", "d-PREV")).unwrap(); + + let dest = a.archive_dir("dg-1", "d-NEW"); + assert!(dest.join("appspec.yml").exists(), "copied archive must be independent"); + let bundle = a.artifact_bundle_path("dg-1", "d-NEW"); + assert_eq!( + fs::read_to_string(&bundle).unwrap(), + "bundle-bytes", + "hard-linked bundle must outlive the source directory entry" + ); + } + + #[test] + fn missing_source_archive_is_an_error() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + let err = reuse_archive(&a, "dg-1", "d-GONE", "d-NEW", "appspec.yml").unwrap_err(); + assert!(err.to_string().contains("not a directory"), "got: {err}"); + } + + #[test] + fn empty_source_archive_is_an_error() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + fs::create_dir_all(a.archive_dir("dg-1", "d-PREV")).unwrap(); + + let err = reuse_archive(&a, "dg-1", "d-PREV", "d-NEW", "appspec.yml").unwrap_err(); + assert!(err.to_string().contains("is empty"), "got: {err}"); + } + + #[test] + fn malformed_id_is_rejected_before_touching_disk() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + + let err = reuse_archive(&a, "dg-1", "d-../escape", "d-NEW", "appspec.yml").unwrap_err(); + assert!(err.to_string().contains("not a well-formed deployment ID"), "got: {err}"); + assert!( + !a.deployment_root_dir("dg-1", "d-NEW").exists(), + "nothing may be created for a rejected ID" + ); + } + + #[test] + fn self_reference_is_rejected() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + seed_source(&a, "dg-1", "d-SAME"); + + let err = reuse_archive(&a, "dg-1", "d-SAME", "d-SAME", "appspec.yml").unwrap_err(); + assert!(err.to_string().contains("same"), "got: {err}"); + // The source must still be intact — this is the case that would destroy it. + assert!(a.archive_dir("dg-1", "d-SAME").join("appspec.yml").exists()); + } + + #[test] + fn source_without_bundle_still_reuses_archive() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + // LocalDirectory revisions have no bundle.tar at all. + let archive = a.archive_dir("dg-1", "d-PREV"); + fs::create_dir_all(&archive).unwrap(); + fs::write(archive.join("appspec.yml"), "version: 0.0\n").unwrap(); + + reuse_archive(&a, "dg-1", "d-PREV", "d-NEW", "appspec.yml").unwrap(); + + assert!(a.archive_dir("dg-1", "d-NEW").join("appspec.yml").exists()); + assert!( + !a.artifact_bundle_path("dg-1", "d-NEW").exists(), + "no bundle should be invented when the source had none" + ); + } + + #[test] + fn incomplete_archive_without_appspec_is_an_error() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + // Non-empty, but no AppSpec: an interrupted copy or partially pruned tree. + let archive = a.archive_dir("dg-1", "d-PREV"); + fs::create_dir_all(&archive).unwrap(); + fs::write(archive.join("some-file.txt"), "x").unwrap(); + + let err = reuse_archive(&a, "dg-1", "d-PREV", "d-NEW", "appspec.yml").unwrap_err(); + assert!(err.to_string().contains("incomplete"), "got: {err}"); + assert!( + !a.archive_dir("dg-1", "d-NEW").exists(), + "must reject before copying anything to the destination" + ); + } + + #[test] + fn rejects_appspec_path_escaping_the_archive() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + seed_source(&a, "dg-1", "d-PREV"); + + for bad in ["../../etc/passwd", "/etc/passwd", "nested/../../escape.yml"] { + let err = reuse_archive(&a, "dg-1", "d-PREV", "d-NEW", bad).unwrap_err(); + assert!(err.to_string().contains("unsafe AppSpec path"), "{bad}: {err}"); + } + assert!(!a.archive_dir("dg-1", "d-NEW").exists()); + } + + #[test] + fn accepts_nested_appspec_path() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + let archive = a.archive_dir("dg-1", "d-PREV"); + fs::create_dir_all(archive.join("configs")).unwrap(); + fs::write( + archive.join("configs/appspec.yml"), + "version: 0.0 +", + ) + .unwrap(); + + reuse_archive(&a, "dg-1", "d-PREV", "d-NEW", "configs/appspec.yml").unwrap(); + assert!(a.archive_dir("dg-1", "d-NEW").join("configs/appspec.yml").exists()); + } + + #[test] + fn honours_custom_appspec_path() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + let archive = a.archive_dir("dg-1", "d-PREV"); + fs::create_dir_all(&archive).unwrap(); + fs::write(archive.join("custom.yml"), "version: 0.0\n").unwrap(); + + // Default name is absent, so the default lookup must reject... + assert!(reuse_archive(&a, "dg-1", "d-PREV", "d-NEW", "appspec.yml").is_err()); + // ...while the spec's actual AppSpec name is accepted. + reuse_archive(&a, "dg-1", "d-PREV", "d-NEW2", "custom.yml").unwrap(); + assert!(a.archive_dir("dg-1", "d-NEW2").join("custom.yml").exists()); + } + + #[test] + fn stale_destination_bundle_is_cleared_when_source_has_none() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + // LocalDirectory-style source: archive only, no bundle.tar of its own. + let archive = a.archive_dir("dg-1", "d-PREV"); + fs::create_dir_all(&archive).unwrap(); + fs::write(archive.join("appspec.yml"), "version: 0.0\n").unwrap(); + + // An earlier failed attempt on this deployment left a bundle behind. + let dest_bundle = a.artifact_bundle_path("dg-1", "d-NEW"); + fs::create_dir_all(dest_bundle.parent().unwrap()).unwrap(); + fs::write(&dest_bundle, "stale-bytes").unwrap(); + + reuse_archive(&a, "dg-1", "d-PREV", "d-NEW", "appspec.yml").unwrap(); + + assert!( + !dest_bundle.exists(), + "a stale bundle must not survive next to a reused archive" + ); + assert!(a.archive_dir("dg-1", "d-NEW").join("appspec.yml").exists()); + } + + #[test] + fn existing_destination_archive_is_replaced() { + let dir = TempDir::new().unwrap(); + let a = archives(&dir); + seed_source(&a, "dg-1", "d-PREV"); + + let dest = a.archive_dir("dg-1", "d-NEW"); + fs::create_dir_all(&dest).unwrap(); + fs::write(dest.join("stale.txt"), "old").unwrap(); + + reuse_archive(&a, "dg-1", "d-PREV", "d-NEW", "appspec.yml").unwrap(); + + assert!(!dest.join("stale.txt").exists(), "stale content must be cleared"); + assert!(dest.join("appspec.yml").exists()); + } + + /// Hooks read `BUNDLE_ETAG` from this marker when the spec carries no `eTag`, so a reused + /// deployment has to leave one behind exactly as a downloaded one does. + #[test] + fn carries_the_bundle_etag_forward_on_reuse() { + let dir = TempDir::new().unwrap(); + let archives = archives(&dir); + seed_source(&archives, "dg-1", "d-SRC"); + fs::write( + archives + .deployment_root_dir("dg-1", "d-SRC") + .join(crate::host_command::BUNDLE_ETAG_FILE), + "abc123", + ) + .unwrap(); + + reuse_archive(&archives, "dg-1", "d-SRC", "d-DEST", "appspec.yml").unwrap(); + + assert_eq!( + fs::read_to_string( + archives + .deployment_root_dir("dg-1", "d-DEST") + .join(crate::host_command::BUNDLE_ETAG_FILE) + ) + .unwrap(), + "abc123" + ); + } + + /// A source with no marker is normal -- an older agent, or a GitHub revision. Reuse must still + /// succeed rather than fail on missing diagnostic metadata. + #[test] + fn reuse_succeeds_when_the_source_has_no_bundle_etag() { + let dir = TempDir::new().unwrap(); + let archives = archives(&dir); + seed_source(&archives, "dg-1", "d-SRC"); + + reuse_archive(&archives, "dg-1", "d-SRC", "d-DEST", "appspec.yml").unwrap(); + + assert!( + !archives + .deployment_root_dir("dg-1", "d-DEST") + .join(crate::host_command::BUNDLE_ETAG_FILE) + .exists() + ); + } + + #[test] + fn records_bundle_source_marker() { + let dir = TempDir::new().unwrap(); + let deploy_dir = dir.path().join("d-NEW"); + fs::create_dir_all(&deploy_dir).unwrap(); + + record_bundle_source(&deploy_dir, SOURCE_ARCHIVE_REUSE, false); + + let marker = deploy_dir.join(crate::host_command::BUNDLE_SOURCE_FILE); + assert_eq!(fs::read_to_string(marker).unwrap(), SOURCE_ARCHIVE_REUSE); + } + + #[test] + fn marker_values_are_distinguishable() { + assert_ne!(SOURCE_ARCHIVE_REUSE, SOURCE_DOWNLOADED); + } +} diff --git a/src/host_command/command_dispatcher.rs b/src/host_command/command_dispatcher.rs index 60064619..e2a39698 100644 --- a/src/host_command/command_dispatcher.rs +++ b/src/host_command/command_dispatcher.rs @@ -138,6 +138,12 @@ impl CommandDispatcher { } } + /// The deployment archives these commands read and write. + #[must_use] + pub fn archives(&self) -> &DeploymentArchives { + self.hook.archives() + } + /// Check if a command is a noop (all lifecycle events have no scripts). /// `DownloadBundle` and `Install` are never noops. #[must_use] @@ -193,6 +199,7 @@ mod tests { bundle_type: "tar".into(), }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, } } diff --git a/src/host_command/commands/download_bundle.rs b/src/host_command/commands/download_bundle.rs index 80d9ddbb..0359a46c 100644 --- a/src/host_command/commands/download_bundle.rs +++ b/src/host_command/commands/download_bundle.rs @@ -8,6 +8,7 @@ use crate::aws_clients::S3Client; use crate::config::AgentConfig; use crate::deployment_specification::types::{DeploymentSpec, RevisionLocation, RevisionSource}; use crate::host_command::DeploymentArchives; +use crate::host_command::archive_reuse; use crate::host_command::bundle_downloader::{ BundleDownloader, BundleFormat, GitHubDownloader, LocalDirectoryDownloader, LocalFileDownloader, S3Downloader, @@ -53,7 +54,32 @@ impl DownloadCommand { debug!("Executing DownloadBundle command"); - let actual_etag = self.download(spec, &bundle_path, &archive_dir)?; + // A bounce (restart) deployment names the deployment whose archive is already + // on this host. Reuse it rather than fetching identical bytes again. Any + // problem here falls through to the normal download: reuse is an + // optimisation, never the only route to the revision. + // + // NOTE: cleanup_old_archives has already run above, so the source may have + // been pruned. That is expected and handled as a fallback, not an error -- + // retention deliberately protects the last successful deployment, which is + // the source a bounce normally references. + let reused = self.try_reuse_archive(spec, &deploy_dir); + + let actual_etag = if reused { + None + } else { + self.download(spec, &bundle_path, &archive_dir)? + }; + + archive_reuse::record_bundle_source( + &deploy_dir, + if reused { + archive_reuse::SOURCE_ARCHIVE_REUSE + } else { + archive_reuse::SOURCE_DOWNLOADED + }, + self.config.hardening.restrict_agent_dir_permissions, + ); self.settle_bundle_mode(&bundle_path)?; @@ -83,19 +109,68 @@ impl DownloadCommand { "Bundle downloaded" ); - if !matches!(spec.revision_source, RevisionSource::LocalDirectory) { + self.prepare_archive(spec, reused, &bundle_path, &archive_dir)?; + + let instructions_dir = self.archives.instructions_dir(); + crate::system::create_deployment_dir( + instructions_dir, + 0o700, + self.config.hardening.restrict_agent_dir_permissions, + )?; + debug!("Instructions directory created at {}", instructions_dir.display()); + + // Ruby: command_executor.rb:308-322 (app_spec_real_path) validates appspec + // exists, but only during Install. We check earlier at download time to + // fail fast with a clear message rather than letting Install discover it. + // NOTE: cleanup_old_archives has already run at this point, matching Ruby's + // destructive-then-validate ordering. A missing appspec here means the old + // archive may already be gone. + let appspec_path = archive_dir.join(&spec.app_spec_path); + if !appspec_path.exists() { + return Err(io::Error::other(format!( + "The deployment failed because the specified file does not exist at the expected \ + location: {}. Verify that your AppSpec file is named correctly and that it is in \ + the root directory of the revision's source code.", + appspec_path.display() + ))); + } + + self.archives.update_most_recent(&spec.deployment_group_id, &deploy_dir)?; + + Ok(()) + } + + /// Unpack the freshly downloaded bundle and apply the opt-in bundle-content + /// hardening checks. + /// + /// Only the **unpack** is skipped for a reused archive (already unpacked when the + /// source deployment downloaded it) and for `LocalDirectory` revisions (copied + /// straight into the archive directory). The hardening checks still run on every + /// archive, reused included, as defence in depth. + /// + /// # Errors + /// Returns an error if extraction fails or any enabled hardening check rejects + /// the archive contents. + fn prepare_archive( + &self, + spec: &DeploymentSpec, + reused: bool, + bundle_path: &Path, + archive_dir: &Path, + ) -> io::Result<()> { + if !reused && !matches!(spec.revision_source, RevisionSource::LocalDirectory) { if archive_dir.exists() { - fs::remove_dir_all(&archive_dir)?; + fs::remove_dir_all(archive_dir)?; } // Size check runs pre-extraction (inspects headers only, no disk writes). if let Some(max_size) = self.config.archive_max_extraction_size && let Err(e) = bundle_unpacker::check_extraction_size( - &bundle_path, + bundle_path, &Self::bundle_type(spec), max_size, ) { - if let Err(rm_err) = fs::remove_file(&bundle_path) { + if let Err(rm_err) = fs::remove_file(bundle_path) { tracing::warn!( path = %bundle_path.display(), error = %rm_err, @@ -107,10 +182,10 @@ impl DownloadCommand { if self.config.hardening.reject_path_traversal_in_bundle && let Err(e) = - bundle_unpacker::check_path_traversal(&bundle_path, &Self::bundle_type(spec)) + bundle_unpacker::check_path_traversal(bundle_path, &Self::bundle_type(spec)) { // rejected bundle fails; not reproducible in CI. - if let Err(rm_err) = fs::remove_file(&bundle_path) { + if let Err(rm_err) = fs::remove_file(bundle_path) { tracing::warn!( path = %bundle_path.display(), error = %rm_err, @@ -121,8 +196,8 @@ impl DownloadCommand { } bundle_unpacker::unpack( - &bundle_path, - &archive_dir, + bundle_path, + archive_dir, &Self::bundle_type(spec), self.config.hardening.restrict_agent_dir_permissions, self.config.hardening.ignore_ownership_in_bundle, @@ -130,49 +205,67 @@ impl DownloadCommand { } if self.config.hardening.reject_symlinks_in_bundle { - bundle_unpacker::reject_bundle_symlinks(&archive_dir)?; + bundle_unpacker::reject_bundle_symlinks(archive_dir)?; } if self.config.hardening.reject_path_traversal_in_bundle { - bundle_unpacker::reject_bundle_path_traversal(&archive_dir)?; + bundle_unpacker::reject_bundle_path_traversal(archive_dir)?; } if self.config.hardening.reject_unsafe_permissions_in_bundle { - bundle_unpacker::reject_bundle_unsafe_permissions(&archive_dir)?; + bundle_unpacker::reject_bundle_unsafe_permissions(archive_dir)?; } + Ok(()) + } - let instructions_dir = self.archives.instructions_dir(); - crate::system::create_deployment_dir( - instructions_dir, - 0o700, - self.config.hardening.restrict_agent_dir_permissions, - )?; - debug!("Instructions directory created at {}", instructions_dir.display()); - - // The appspec is also validated during Install; we check earlier at - // download time to fail fast with a clear message rather than letting - // Install discover it. NOTE: cleanup_old_archives has already run at - // this point (destructive-then-validate ordering), so a missing appspec - // here means the old archive may already be gone. - let appspec_path = archive_dir.join(&spec.app_spec_path); - if !appspec_path.exists() { - return Err(io::Error::other(format!( - "The deployment failed because the specified file does not exist at the expected \ - location: {}. Verify that your AppSpec file is named correctly and that it is in \ - the root directory of the revision's source code.", - appspec_path.display() - ))); + /// Attempt archive reuse for a bounce deployment. Returns `true` only when the + /// destination now holds a complete, ready-to-install archive. + /// + /// Returns `false` — never an error — when reuse is off, not requested, or not + /// possible, so the caller downloads instead. + fn try_reuse_archive(&self, spec: &DeploymentSpec, deploy_dir: &Path) -> bool { + let Some(source_id) = spec.reuse_archive_from_deployment_id.as_deref() else { + return false; + }; + if !self.config.enable_archive_reuse { + debug!( + source = %source_id, + "Deployment spec requests archive reuse but enable_archive_reuse is off; downloading" + ); + return false; } - self.archives.update_most_recent(&spec.deployment_group_id, &deploy_dir)?; - - Ok(()) + match archive_reuse::reuse_archive( + &self.archives, + &spec.deployment_group_id, + source_id, + &spec.deployment_id, + &spec.app_spec_path, + ) { + Ok(()) => { + info!( + source_deployment_id = %source_id, + deployment_id = %spec.deployment_id, + "Reusing on-host archive instead of downloading" + ); + true + }, + Err(e) => { + // Expected whenever the source archive is gone or incomplete. + warn!( + source_deployment_id = %source_id, + "Archive reuse unavailable, falling back to download: {e}" + ); + let _ = deploy_dir; + false + }, + } } /// Settle the downloaded bundle to the `restrict_agent_dir_permissions` /// policy mode. The downloaders create it 0600 (safe while streaming); this - /// is the single chokepoint for S3/GitHub/local-file sources. The default - /// (unhardened) mode is 0644. + /// is the single chokepoint for S3/GitHub/local-file sources. Ruby parity + /// is 0644 (`File.open` under umask 0022). /// /// Skips symlinks: the `LocalFile` source symlinks `bundle_path` at the user's /// ORIGINAL file (`local_file.rs`), and `set_permissions` (chmod) follows @@ -328,6 +421,7 @@ mod tests { bundle_type: "tar".into(), }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, } } @@ -820,6 +914,159 @@ mod tests { } } + /// Seed a previous deployment that is complete enough to reuse. + fn seed_previous(archives: &DeploymentArchives, group: &str, id: &str) { + let archive = archives.archive_dir(group, id); + fs::create_dir_all(&archive).unwrap(); + fs::write(archive.join("appspec.yml"), "version: 0.0\nos: linux\n").unwrap(); + fs::write(archives.artifact_bundle_path(group, id), "bundle-bytes").unwrap(); + } + + fn reuse_spec(source: &str) -> DeploymentSpec { + DeploymentSpec { reuse_archive_from_deployment_id: Some(source.into()), ..s3_spec() } + } + + #[test] + fn reuse_bypasses_download_when_enabled() { + let dir = TempDir::new().unwrap(); + let archives = test_archives(&dir); + seed_previous(&archives, "dg-1", "d-PREV"); + + let config = AgentConfig { enable_archive_reuse: true, ..AgentConfig::default() }; + // No S3 client: if this deployment tried to download it would fail, so + // success can only mean the on-host archive was reused. + let cmd = DownloadCommand::new(archives.clone(), None, Arc::new(config)); + + let deploy_dir = archives.deployment_root_dir("dg-1", "d-123"); + fs::create_dir_all(&deploy_dir).unwrap(); + + cmd.execute(&reuse_spec("d-PREV")).unwrap(); + + assert!(archives.archive_dir("dg-1", "d-123").join("appspec.yml").exists()); + assert_eq!( + fs::read_to_string(deploy_dir.join(crate::host_command::BUNDLE_SOURCE_FILE)).unwrap(), + archive_reuse::SOURCE_ARCHIVE_REUSE + ); + } + + #[test] + fn gate_off_ignores_reuse_field_and_downloads() { + let dir = TempDir::new().unwrap(); + let archives = test_archives(&dir); + seed_previous(&archives, "dg-1", "d-PREV"); + + let config = AgentConfig { enable_archive_reuse: false, ..AgentConfig::default() }; + let cmd = DownloadCommand::new(archives.clone(), None, Arc::new(config)); + + let deploy_dir = archives.deployment_root_dir("dg-1", "d-123"); + fs::create_dir_all(&deploy_dir).unwrap(); + + let err = cmd.execute(&reuse_spec("d-PREV")).unwrap_err(); + assert!( + err.to_string().contains("S3 client not configured"), + "gate off must take the download path, got: {err}" + ); + } + + #[test] + fn missing_source_archive_falls_back_to_download() { + let dir = TempDir::new().unwrap(); + let archives = test_archives(&dir); + // d-GONE was never seeded: the reuse source does not exist. + + let config = AgentConfig { enable_archive_reuse: true, ..AgentConfig::default() }; + let cmd = DownloadCommand::new(archives.clone(), None, Arc::new(config)); + + let deploy_dir = archives.deployment_root_dir("dg-1", "d-123"); + fs::create_dir_all(&deploy_dir).unwrap(); + + let err = cmd.execute(&reuse_spec("d-GONE")).unwrap_err(); + assert!( + err.to_string().contains("S3 client not configured"), + "an absent source archive must fall back to downloading, got: {err}" + ); + } + + #[test] + fn malformed_reuse_id_falls_back_to_download() { + let dir = TempDir::new().unwrap(); + let archives = test_archives(&dir); + + let config = AgentConfig { enable_archive_reuse: true, ..AgentConfig::default() }; + let cmd = DownloadCommand::new(archives.clone(), None, Arc::new(config)); + + let deploy_dir = archives.deployment_root_dir("dg-1", "d-123"); + fs::create_dir_all(&deploy_dir).unwrap(); + + let err = cmd.execute(&reuse_spec("d-../../etc")).unwrap_err(); + assert!( + err.to_string().contains("S3 client not configured"), + "a traversal attempt must fall back to downloading, got: {err}" + ); + assert!( + !dir.path().join("etc").exists(), + "traversal must not create anything outside the deployment tree" + ); + } + + #[test] + fn incomplete_reused_archive_falls_back_to_download() { + let dir = TempDir::new().unwrap(); + let archives = test_archives(&dir); + // Source archive exists and is non-empty but has no AppSpec. + let archive = archives.archive_dir("dg-1", "d-PREV"); + fs::create_dir_all(&archive).unwrap(); + fs::write(archive.join("stray.txt"), "x").unwrap(); + + let config = AgentConfig { enable_archive_reuse: true, ..AgentConfig::default() }; + let cmd = DownloadCommand::new(archives.clone(), None, Arc::new(config)); + + let deploy_dir = archives.deployment_root_dir("dg-1", "d-123"); + fs::create_dir_all(&deploy_dir).unwrap(); + + // Must fall back to downloading (which fails here for want of a client) + // rather than failing the host on the downstream AppSpec check. + let err = cmd.execute(&reuse_spec("d-PREV")).unwrap_err(); + assert!( + err.to_string().contains("S3 client not configured"), + "an incomplete archive must fall back to download, got: {err}" + ); + } + + #[cfg(unix)] + #[test] + fn successful_download_records_downloaded_marker() { + let dir = TempDir::new().unwrap(); + let archives = test_archives(&dir); + let cmd = DownloadCommand::new(archives.clone(), None, Arc::new(AgentConfig::default())); + + let src_dir = dir.path().join("src"); + fs::create_dir_all(&src_dir).unwrap(); + fs::write(src_dir.join("appspec.yml"), "version: 0.0\nos: linux").unwrap(); + let tar_path = dir.path().join("bundle.tar"); + std::process::Command::new("tar") + .args([ + "-cf", + &tar_path.display().to_string(), + "-C", + &src_dir.display().to_string(), + ".", + ]) + .output() + .unwrap(); + + let deploy_dir = archives.deployment_root_dir("dg-1", "d-123"); + fs::create_dir_all(&deploy_dir).unwrap(); + + cmd.execute(&local_file_spec(&tar_path.display().to_string())).unwrap(); + + assert_eq!( + fs::read_to_string(deploy_dir.join(crate::host_command::BUNDLE_SOURCE_FILE)).unwrap(), + archive_reuse::SOURCE_DOWNLOADED, + "a normal download must be recorded as such" + ); + } + fn raw_tar_header_for_test(name: &[u8], size: u64) -> [u8; 512] { let mut header = [0u8; 512]; let len = name.len().min(100); diff --git a/src/host_command/commands/hook.rs b/src/host_command/commands/hook.rs index 6db5ef21..1df99e94 100644 --- a/src/host_command/commands/hook.rs +++ b/src/host_command/commands/hook.rs @@ -283,6 +283,7 @@ mod tests { bundle_type: "tar".into(), }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, } } diff --git a/src/host_command/commands/install.rs b/src/host_command/commands/install.rs index 58195e71..27ecfd44 100644 --- a/src/host_command/commands/install.rs +++ b/src/host_command/commands/install.rs @@ -159,6 +159,7 @@ mod tests { bundle_type: "tar".into(), }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, } } diff --git a/src/host_command/mod.rs b/src/host_command/mod.rs index 285da6f6..4a5eed17 100644 --- a/src/host_command/mod.rs +++ b/src/host_command/mod.rs @@ -4,6 +4,7 @@ //! implementations. Each command is a separate struct with single responsibility. pub mod appspec_validator; +pub mod archive_reuse; pub mod bundle_downloader; pub mod bundle_unpacker; mod command_dispatcher; @@ -17,3 +18,62 @@ pub use deployment_archives::DeploymentArchives; /// S3 object's `ETag` so the executor can expose it to hooks as `BUNDLE_ETAG` /// even when the spec carried a null `ETag`. pub const BUNDLE_ETAG_FILE: &str = ".bundle-etag"; + +/// Key of the note appended to a successful `DownloadBundle` completion, naming where the bundle +/// came from. +/// +/// **Agent → service message format.** Carried inside the existing free-form `message` field of the +/// completion diagnostics, which the service already surfaces verbatim as +/// `"message": "Succeeded: "`: +/// +/// ```text +/// {"error_code":0,"script_name":"","message":"Succeeded: BundleSource=archive-reuse","log":""} +/// ``` +/// +/// The service reads it to count archive-reuse hits, so the key and both values are a contract: +/// renaming either silently zeroes the metric rather than failing a build. Kept in `message` rather +/// than added as a fifth JSON key, because that payload's shape is long-standing and a new key would +/// require the service to tolerate an unknown field. +pub const BUNDLE_SOURCE_NOTE: &str = "BundleSource"; + +/// Format the completion note for a bundle obtained from `source`. +#[must_use] +pub fn bundle_source_note(source: &str) -> String { + format!("{BUNDLE_SOURCE_NOTE}={source}") +} + +/// Filename, under a deployment's root dir, where `DownloadBundle` records how +/// the bundle was obtained — [`archive_reuse::SOURCE_ARCHIVE_REUSE`] or +/// [`archive_reuse::SOURCE_DOWNLOADED`]. Makes reuse-versus-fallback behaviour +/// observable per host after the fact, not just in logs. +pub const BUNDLE_SOURCE_FILE: &str = ".bundle-source"; + +#[cfg(test)] +mod bundle_source_note_tests { + use super::{BUNDLE_SOURCE_NOTE, bundle_source_note}; + use crate::host_command::archive_reuse::{SOURCE_ARCHIVE_REUSE, SOURCE_DOWNLOADED}; + + /// The agent -> service message format. Asserted on the literal strings because the service + /// parses them: renaming the key or either value silently zeroes the reuse metric instead of + /// failing a build. + #[test] + fn the_note_format_is_a_contract() { + assert_eq!(BUNDLE_SOURCE_NOTE, "BundleSource"); + assert_eq!(bundle_source_note(SOURCE_ARCHIVE_REUSE), "BundleSource=archive-reuse"); + assert_eq!(bundle_source_note(SOURCE_DOWNLOADED), "BundleSource=downloaded"); + } + + /// What the service ends up seeing -- including the empty-note case that every other command + /// produces, which must stay exactly as it was. + #[test] + fn the_note_lands_in_the_diagnostics_message() { + assert_eq!( + crate::command_poller::diagnostics::success(&bundle_source_note(SOURCE_ARCHIVE_REUSE)), + r#"{"error_code":0,"log":"","message":"Succeeded: BundleSource=archive-reuse","script_name":""}"# + ); + assert_eq!( + crate::command_poller::diagnostics::success(""), + r#"{"error_code":0,"log":"","message":"Succeeded","script_name":""}"# + ); + } +} diff --git a/src/lifecycle_event/executor.rs b/src/lifecycle_event/executor.rs index e457950b..b9262ad4 100644 --- a/src/lifecycle_event/executor.rs +++ b/src/lifecycle_event/executor.rs @@ -430,6 +430,7 @@ mod tests { revision_source, revision, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, } } diff --git a/src/main.rs b/src/main.rs index 061b227e..cbe703a9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -907,6 +907,7 @@ fn run_deploy_local( bundle_type: bundle_type.to_string(), }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, }; execute_local_deployment( @@ -1114,6 +1115,7 @@ fn run_deploy_local_s3( etag: None, }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, }; execute_local_deployment( @@ -1214,6 +1216,7 @@ fn run_deploy_local_github( bundle_type: Some(resolved_bundle_type.to_string()), }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, }; execute_local_deployment( diff --git a/src/system/file_ops.rs b/src/system/file_ops.rs index 63f0e0a3..89039cfd 100644 --- a/src/system/file_ops.rs +++ b/src/system/file_ops.rs @@ -123,6 +123,54 @@ pub type SystemFileOperations = WindowsFileOperations; #[cfg(not(target_os = "windows"))] pub type SystemFileOperations = LinuxFileOperations; +/// Whether `component` is safe to use as a single path component. +/// +/// `Path::join` treats `..` as an ordinary parent component and an absolute path +/// as a full replacement, so any externally supplied string used as a directory +/// name has to be checked before it is joined. This is an allowlist of shape +/// rather than a blocklist of characters: a component must be non-empty, must not +/// be a relative-path marker, and must contain no separator or NUL byte. +/// +/// Deliberately permissive about the rest, because callers pass identifiers of +/// several shapes (UUID deployment-group ids, `d-`-prefixed deployment ids). Use +/// a stricter check where the exact format is known. +#[must_use] +pub fn is_safe_path_component(component: &str) -> bool { + !component.is_empty() + && component != "." + && component != ".." + && !component.contains('\0') + && !component.chars().any(std::path::is_separator) + // `is_separator` is platform-specific; reject the Windows separator + // everywhere so a spec cannot behave differently per platform. + && !component.contains('\\') +} + +/// Whether a customer-supplied, revision-relative path stays inside the directory it is joined to. +/// +/// Unlike [`is_safe_path_component`] this permits separators: an `AppSpec` may legitimately be nested, +/// as in `configs/appspec.yml`. It rejects anything that could climb out of the join or re-root it -- +/// a `..` component, a leading `/`, or a Windows drive prefix. +/// +/// Checked lexically rather than by canonicalising, because the path is validated before the file it +/// names is known to exist, and the not-found case is an expected outcome rather than an error. +#[must_use] +pub fn is_safe_relative_path(path: &str) -> bool { + !path.is_empty() + && !path.contains('\0') + // `is_separator` is platform-specific; reject the Windows separator everywhere so a spec + // cannot resolve differently per platform, matching is_safe_path_component. + && !path.contains('\\') + && !Path::new(path).components().any(|c| { + matches!( + c, + std::path::Component::ParentDir + | std::path::Component::RootDir + | std::path::Component::Prefix(_) + ) + }) +} + /// Recursively copy a directory tree. /// /// # Errors @@ -177,6 +225,40 @@ impl PlatformFileOperations for MockFileOperations { } } +#[cfg(test)] +mod safe_relative_path_tests { + use super::is_safe_relative_path; + + #[test] + fn accepts_a_plain_or_nested_appspec_path() { + assert!(is_safe_relative_path("appspec.yml")); + assert!(is_safe_relative_path("configs/appspec.yml")); + assert!(is_safe_relative_path("./appspec.yml"), "a CurDir component stays inside"); + } + + #[test] + fn rejects_climbing_out_or_re_rooting() { + for bad in [ + "../appspec.yml", + "../../etc/passwd", + "configs/../../appspec.yml", + "/etc/shadow", + "", + "app\0spec.yml", + ] { + assert!(!is_safe_relative_path(bad), "must reject {bad:?}"); + } + } + + /// Rejected on every platform, so a spec cannot resolve one way on Linux and another on Windows. + #[test] + fn rejects_windows_separators_and_prefixes_everywhere() { + assert!(!is_safe_relative_path(r"..\appspec.yml")); + assert!(!is_safe_relative_path(r"configs\appspec.yml")); + assert!(!is_safe_relative_path(r"C:\Windows\system.ini")); + } +} + #[cfg(test)] mod tests { use super::*; @@ -270,6 +352,39 @@ mod tests { assert!(mock.write_with_retry(&path, "content").is_err()); } + #[cfg(unix)] + #[test] + fn safe_path_components_are_accepted() { + for good in [ + "dg-1", + "d-A1B2C3D4E", + "f47ac10b-58cc-4372-a567-0e02b2c3d479", + "arn_like-name.with.dots", + "..hidden", + "a..b", + ] { + assert!(is_safe_path_component(good), "should accept {good:?}"); + } + } + + #[test] + fn unsafe_path_components_are_rejected() { + for bad in [ + "", + ".", + "..", + "../evil", + "../../../../tmp/evil", + "a/b", + "/absolute", + "trailing/", + "back\\slash", + "nul\0byte", + ] { + assert!(!is_safe_path_component(bad), "should reject {bad:?}"); + } + } + #[cfg(unix)] #[test] fn copy_dir_recursive_with_symlink() { diff --git a/tests/end_to_end.rs b/tests/end_to_end.rs index 52626e28..56086d85 100644 --- a/tests/end_to_end.rs +++ b/tests/end_to_end.rs @@ -38,6 +38,7 @@ fn make_spec(deployment_id: &str, app_name: &str) -> DeploymentSpec { etag: None, }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, } } @@ -188,6 +189,7 @@ fn pipeline_orchestrates_full_deployment() { bundle_type: "directory".into(), }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, }; // 1. Download bundle (local directory copy) @@ -340,6 +342,7 @@ fn downloads_tar_bundle_and_deploys() { bundle_type: "tar".into(), }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, }; // 1. Download + unpack diff --git a/tests/hook_execution.rs b/tests/hook_execution.rs index 5f70dee8..519d3d83 100644 --- a/tests/hook_execution.rs +++ b/tests/hook_execution.rs @@ -29,6 +29,7 @@ fn make_spec() -> DeploymentSpec { etag: Some("abc".into()), }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, } } diff --git a/tests/security/intake/deployment_flow.rs b/tests/security/intake/deployment_flow.rs index d145eb16..8744a44a 100644 --- a/tests/security/intake/deployment_flow.rs +++ b/tests/security/intake/deployment_flow.rs @@ -64,6 +64,7 @@ hooks: etag: None, }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, }; // Execute the AfterInstall lifecycle event @@ -172,6 +173,7 @@ hooks: bundle_type: "tar".into(), }, all_possible_lifecycle_events: None, + reuse_archive_from_deployment_id: None, }; let executor = LifecycleEventExecutor::new( From a3f0ed20e9d63c7ab47b6481c73cb866a18a0425 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Dani=C4=87?= Date: Wed, 9 Sep 2026 08:02:15 +0000 Subject: [PATCH 2/2] Re-stage only author-staged files in the pre-commit hook The hook re-stages what its own auto-fix (fmt, clippy --fix) touched, but used git diff --name-only, which lists every modified file in the tree - silently sweeping deliberately-unstaged edits into the commit. Remember the staged set before the fixers run and re-stage only those files. --- hooks/pre-commit | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/hooks/pre-commit b/hooks/pre-commit index 76234912..a0394f1d 100755 --- a/hooks/pre-commit +++ b/hooks/pre-commit @@ -4,9 +4,17 @@ set -euo pipefail # Ensure cargo is on PATH (rustup installs to ~/.cargo/bin) export PATH="$HOME/.cargo/bin:$PATH" +# Remember what the author staged, before any auto-fix runs. +staged_files=$(git diff --cached --name-only --diff-filter=ACM) + # Auto-fix formatting and clippy lints before commit cargo fmt --all cargo clippy --fix --allow-dirty --allow-staged 2>/dev/null || true -# Re-stage any files that were auto-fixed -git diff --name-only | xargs -r git add +# Re-stage only the files that were already staged. `git diff --name-only` would +# list every modified file in the tree, so anything the author deliberately left +# unstaged -- unrelated edits, or formatting churn in files this commit does not +# touch -- was silently swept into the commit. +if [ -n "$staged_files" ]; then + printf '%s\n' "$staged_files" | xargs -r git add -- +fi