From f2808ca0774cdf302de28b8aa785495162bbccac Mon Sep 17 00:00:00 2001 From: Nathan Inge Date: Tue, 3 Nov 2026 15:50:08 +0000 Subject: [PATCH 1/8] Restrict the Windows MSI install root to Administrators and SYSTEM The MSI now sets an explicit ACL on C:\ProgramData\Amazon\CodeDeploy instead of leaving the folder to inherit its permissions from %ProgramData%. As in the 1.x MSI, a single CreateFolder/Permission grants Administrators full control on the install root, and the subfolders inherit it. Windows Installer adds LocalSystem full control to every LockPermissions descriptor. tests/msi_install_dir_acl.rs reads wix/main.wxs and fails if the install-root ACL is removed or loosened, or if any other Permission is added. --- src/daemon/windows_service.rs | 6 +- tests/msi_install_dir_acl.rs | 244 ++++++++++++++++++++++++++++++++++ wix/main.wxs | 6 + 3 files changed, 254 insertions(+), 2 deletions(-) create mode 100644 tests/msi_install_dir_acl.rs diff --git a/src/daemon/windows_service.rs b/src/daemon/windows_service.rs index e75c94e4..40b64b1b 100644 --- a/src/daemon/windows_service.rs +++ b/src/daemon/windows_service.rs @@ -33,8 +33,10 @@ const SERVICE_TYPE: ServiceType = ServiceType::OWN_PROCESS; /// Documented AWS CodeDeploy agent install path on Windows (see /// ). -/// The MSI creates this directory with an admin-only ACL, so a non-admin -/// local user cannot replace the binary and escalate to LocalSystem. +/// The MSI gives this directory an explicit Administrators/SYSTEM-only ACL +/// that its subfolders inherit (`wix/main.wxs`, enforced by +/// `tests/msi_install_dir_acl.rs`), so only administrators can change the +/// installed files. /// /// Uppercased for case-insensitive comparison against a canonicalized /// path, which also resolves symlinks, junctions, and 8.3 short names. diff --git a/tests/msi_install_dir_acl.rs b/tests/msi_install_dir_acl.rs new file mode 100644 index 00000000..ad316215 --- /dev/null +++ b/tests/msi_install_dir_acl.rs @@ -0,0 +1,244 @@ +//! The Windows MSI (`wix/main.wxs`) installs the agent under +//! `%ProgramData%\Amazon\CodeDeploy` and gives that install root an explicit +//! Administrators-only ACL (Windows Installer adds LocalSystem itself) instead +//! of the permissions it would otherwise inherit from `%ProgramData%`. +//! +//! The model is the one the 1.x MSI shipped with: a single Administrators +//! Permission on the install root, which every subfolder inherits. The MSI is +//! not built by this crate, so this test reads the WiX source and fails the +//! build if that ACL is removed, loosened, or extended to other folders. + +use std::collections::{BTreeMap, BTreeSet}; +use std::path::Path; + +/// Directory `Id` of the install root in `wix/main.wxs`. +const INSTALL_ROOT: &str = "INSTALLDIR"; +/// The only grantee allowed in the package (LocalSystem is implicit). +const ADMINISTRATORS: &str = "Administrators"; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TagKind { + Open, + Close, + SelfClosing, +} + +#[derive(Debug)] +struct Tag { + name: String, + attrs: BTreeMap, + kind: TagKind, +} + +/// Minimal tag scanner for the WiX source: skips comments and processing +/// instructions, and ignores `>` inside quoted attribute values. Enough for +/// `main.wxs`, which uses no CDATA or entities in the elements checked here. +fn scan_tags(xml: &str) -> Vec { + let mut tags = Vec::new(); + let mut rest = xml; + while let Some(start) = rest.find('<') { + rest = &rest[start..]; + let skip_to = |end: &str, rest: &str| rest.find(end).map_or(rest.len(), |i| i + end.len()); + if rest.starts_with("", rest)..]; + continue; + } + if rest.starts_with("", rest)..]; + continue; + } + let mut in_quote = false; + let end = rest + .char_indices() + .find(|&(_, c)| { + if c == '"' { + in_quote = !in_quote; + } + c == '>' && !in_quote + }) + .map_or(rest.len(), |(i, _)| i); + tags.push(parse_tag(&rest[1..end])); + rest = &rest[(end + 1).min(rest.len())..]; + } + tags +} + +fn parse_tag(inner: &str) -> Tag { + let (kind, body) = if let Some(body) = inner.strip_prefix('/') { + (TagKind::Close, body) + } else if let Some(body) = inner.strip_suffix('/') { + (TagKind::SelfClosing, body) + } else { + (TagKind::Open, inner) + }; + let body = body.trim(); + let name_end = body.find(char::is_whitespace).unwrap_or(body.len()); + let mut attrs = BTreeMap::new(); + let mut rest = &body[name_end..]; + while let Some(eq) = rest.find('=') { + let key = rest[..eq].trim().to_string(); + let after = &rest[eq + 1..]; + let Some(open) = after.find('"') else { break }; + let Some(len) = after[open + 1..].find('"') else { + break; + }; + attrs.insert(key, after[open + 1..open + 1 + len].to_string()); + rest = &after[open + 1 + len + 1..]; + } + Tag { name: body[..name_end].to_string(), attrs, kind } +} + +/// What the ACL checks need from the package. +#[derive(Debug, Default)] +struct Package { + /// Folder Id → (component Id, Permission attributes) for each + /// `CreateFolder/Permission`. + folder_permissions: Vec<(String, String, BTreeMap)>, + /// `User` of every `Permission`/`PermissionEx`, wherever it appears. + grantees: Vec, + /// Ids referenced by `ComponentRef`. + component_refs: BTreeSet, +} + +fn attr<'a>(tag: &'a Tag, key: &str) -> Option<&'a str> { + tag.attrs.get(key).map(String::as_str) +} + +fn analyze(xml: &str) -> Package { + let mut pkg = Package::default(); + let mut stack: Vec = Vec::new(); + for tag in scan_tags(xml) { + if tag.kind == TagKind::Close { + stack.pop(); + continue; + } + let nearest = |name: &str| stack.iter().rev().find(|t| t.name == name); + // WiX resolves a folder from an explicit Directory attribute, then the + // component's Directory, then the enclosing . + let folder = |own: Option<&str>| { + own.or_else(|| nearest("Component").and_then(|c| attr(c, "Directory"))) + .or_else(|| nearest("Directory").and_then(|d| attr(d, "Id"))) + .unwrap_or_default() + .to_string() + }; + match tag.name.as_str() { + "Permission" | "PermissionEx" => { + pkg.grantees.push(attr(&tag, "User").unwrap_or_default().to_string()); + if let Some(create) = stack.last().filter(|t| t.name == "CreateFolder") { + let component = nearest("Component") + .and_then(|c| attr(c, "Id")) + .unwrap_or_default() + .to_string(); + pkg.folder_permissions.push(( + folder(attr(create, "Directory")), + component, + tag.attrs.clone(), + )); + } + }, + "ComponentRef" => { + pkg.component_refs.insert(attr(&tag, "Id").unwrap_or_default().to_string()); + }, + _ => {}, + } + if tag.kind == TagKind::Open { + stack.push(tag); + } + } + pkg +} + +fn main_wxs() -> Package { + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("wix/main.wxs"); + let xml = std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("failed to read {}: {e}", path.display())); + analyze(&xml) +} + +/// Folders that get an Administrators/GenericAll descriptor from a component +/// the feature actually installs. +fn locked_folders(pkg: &Package) -> BTreeSet { + pkg.folder_permissions + .iter() + .filter(|(_, component, attrs)| { + pkg.component_refs.contains(component) + && attrs.get("User").map(String::as_str) == Some(ADMINISTRATORS) + && attrs.get("GenericAll").map(String::as_str) == Some("yes") + }) + .map(|(folder, _, _)| folder.clone()) + .collect() +} + +#[test] +fn only_install_root_is_locked_to_administrators() { + // Arrange + let pkg = main_wxs(); + + // Act + let locked = locked_folders(&pkg); + + // Assert + assert_eq!( + locked, + BTreeSet::from([INSTALL_ROOT.to_string()]), + "expected exactly one installed CreateFolder/Permission User=\"{ADMINISTRATORS}\" \ + GenericAll=\"yes\", on {INSTALL_ROOT}" + ); +} + +#[test] +fn package_declares_no_other_permissions() { + // Arrange + let pkg = main_wxs(); + + // Act + let count = pkg.grantees.len(); + + // Assert + assert_eq!(count, 1, "wix/main.wxs must declare exactly one Permission (on {INSTALL_ROOT})"); +} + +#[test] +fn no_permission_grants_access_beyond_administrators() { + // Arrange + let pkg = main_wxs(); + + // Act + let others: Vec<_> = pkg.grantees.iter().filter(|u| *u != ADMINISTRATORS).collect(); + + // Assert + assert!(others.is_empty(), "unexpected grantees in wix/main.wxs: {others:?}"); +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn scanner_ignores_tags_inside_comments() { + let xml = r#""#; + assert!(analyze(xml).grantees.is_empty()); + } + + #[test] + fn scanner_ignores_angle_bracket_in_attribute_value() { + let tags = scan_tags(r#""#); + assert_eq!(tags[0].attrs.get("Id").map(String::as_str), Some("P")); + } + + #[test] + fn create_folder_directory_attribute_overrides_enclosing_directory() { + let xml = r#" + + "#; + assert_eq!(locked_folders(&analyze(xml)), BTreeSet::from(["B".to_string()])); + } + + #[test] + fn permission_in_unreferenced_component_does_not_lock_folder() { + let xml = r#" + + "#; + assert!(locked_folders(&analyze(xml)).is_empty()); + } +} diff --git a/wix/main.wxs b/wix/main.wxs index 6d9bf183..3976e598 100644 --- a/wix/main.wxs +++ b/wix/main.wxs @@ -32,6 +32,11 @@ + + + + + + From fc0760a3d85f59bef0be1433135d9efd3cfb8bdc Mon Sep 17 00:00:00 2001 From: Nathan Inge Date: Tue, 3 Nov 2026 15:51:18 +0000 Subject: [PATCH 2/8] Retry PutHostCommandComplete on transient errors until the service deadline A completion that hit a 5xx or a network error was logged and dropped: only throttling was retried. The lifecycle event then stayed InProgress until the service's 70-minute acknowledgement timeout failed it with "CodeDeploy agent was not able to notify CodeDeploy server the execution result of this lifecycle event", even when the hook had succeeded. report_completion now retries retryable errors until 65 minutes after the command was acknowledged, with full-jitter exponential backoff (1 s, doubling, capped at 30 s). Every attempt sends the same status and diagnostics, so a transport failure never changes the reported result. Resending is safe because the service keeps the first completion it records and discards later ones, and ignores completions after its timeout. The deadline is taken before the ack is sent so it can only fall early. Non-retryable errors (4xx, signing, serialization) are still logged and dropped. - codedeploy-commands Error keeps the HTTP status (status(), with_status()). A 503 ServiceUnavailableException and a bodyless 500 both classify as Http, so the status is what tells them apart from a 4xx. - CodeDeployClientError::is_retryable covers throttling, any 5xx, server exceptions, network errors and an unavailable IMDS. - Before retrying after a 5xx response (other than throttling), the client drops its pooled connections (Client::reset_http_connections, which rebuilds only the reqwest client and keeps the credentials), so retries don't keep going to the backend host that failed. Resets are rate-limited to one per minute per client. Only the processor's client is reset; the poller keeps its connection. - HTTP calls no longer hold the client lock for the whole request. Each call copies the client under a brief read lock and sends on the copy, so a reset or credential refresh no longer waits for in-flight requests. - Retries continue through agent shutdown, bounded by the worker's drain timeout (kill_agent_max_wait_time_seconds, as for a running hook), so a stop during an outage can take up to the 65-minute deadline. - CompletionRetryPolicy is configurable for tests. --- crates/codedeploy-commands/src/client.rs | 72 +++- crates/codedeploy-commands/src/error.rs | 50 ++- src/aws_clients/codedeploy_command_client.rs | 262 ++++++++++-- src/command_poller/command_processor.rs | 419 ++++++++++++++++++- 4 files changed, 743 insertions(+), 60 deletions(-) diff --git a/crates/codedeploy-commands/src/client.rs b/crates/codedeploy-commands/src/client.rs index c568e6e5..fbc77907 100644 --- a/crates/codedeploy-commands/src/client.rs +++ b/crates/codedeploy-commands/src/client.rs @@ -13,7 +13,10 @@ use aws_sigv4::sign::v4; use std::time::{Duration, SystemTime}; /// `CodeDeploy` Command Service client. -#[derive(Debug)] +/// +/// Cheap to clone: the credentials and the reqwest client are reference-counted, +/// so clones share one connection pool. +#[derive(Debug, Clone)] pub struct Client { endpoint: String, region: String, @@ -26,6 +29,9 @@ pub struct Client { http: reqwest::blocking::Client, credentials: AwsCredentials, agent_version: Option, + // Kept so `reset_http_connections` can rebuild `http` identically. + http_read_timeout: Duration, + proxy_uri: Option, } impl Client { @@ -52,6 +58,20 @@ impl Client { self.agent_version.as_deref() } + /// Drop pooled connections by replacing the HTTP client. + /// + /// Pooled keep-alive connections stay pinned to whichever backend host the + /// load balancer first chose. Call this after a 5xx or network error so the + /// next request opens a fresh connection and can reach a healthy host. + /// Requests already in flight on the old client finish normally. + /// + /// # Errors + /// Returns an error if the HTTP client cannot be rebuilt. + pub fn reset_http_connections(&mut self) -> Result<(), Error> { + self.http = build_http(self.http_read_timeout, self.proxy_uri.as_deref())?; + Ok(()) + } + /// Poll for a host command. /// /// # Errors @@ -318,16 +338,7 @@ impl ClientBuilder { self.enable_auth_policy, ); - let mut http_builder = - reqwest::blocking::ClientBuilder::new().timeout(self.http_read_timeout); - - if let Some(proxy) = self.proxy_uri { - let proxy = reqwest::Proxy::all(&proxy) - .map_err(|e| Error::new(ErrorKind::Build, e.to_string()))?; - http_builder = http_builder.proxy(proxy); - } - - let http = http_builder.build().map_err(|e| Error::new(ErrorKind::Build, e.to_string()))?; + let http = build_http(self.http_read_timeout, self.proxy_uri.as_deref())?; // Signing name and target prefix both track enable_auth_policy, which // also selects the `-secure` endpoint. @@ -358,10 +369,27 @@ impl ClientBuilder { http, credentials, agent_version: self.agent_version, + http_read_timeout: self.http_read_timeout, + proxy_uri: self.proxy_uri, }) } } +fn build_http( + http_read_timeout: Duration, + proxy_uri: Option<&str>, +) -> Result { + let mut http_builder = reqwest::blocking::ClientBuilder::new().timeout(http_read_timeout); + + if let Some(proxy) = proxy_uri { + let proxy = + reqwest::Proxy::all(proxy).map_err(|e| Error::new(ErrorKind::Build, e.to_string()))?; + http_builder = http_builder.proxy(proxy); + } + + http_builder.build().map_err(|e| Error::new(ErrorKind::Build, e.to_string())) +} + impl Default for ClientBuilder { fn default() -> Self { Self { @@ -386,6 +414,28 @@ mod tests { AwsCredentials::new("test-key", "test-secret", None, None, "test") } + #[test] + fn reset_http_connections_keeps_configuration() { + let mut client = Client::builder() + .region("us-west-2") + .credentials(test_credentials()) + .http_read_timeout(Duration::from_secs(7)) + .proxy_uri(Some("http://proxy.example:3128".into())) + .agent_version(Some("2.1.3".into())) + .build() + .unwrap(); + let endpoint = client.endpoint().to_string(); + + client.reset_http_connections().unwrap(); + client.reset_http_connections().unwrap(); + + assert_eq!(client.endpoint(), endpoint); + assert_eq!(client.region(), "us-west-2"); + assert_eq!(client.agent_version(), Some("2.1.3")); + assert_eq!(client.http_read_timeout, Duration::from_secs(7)); + assert_eq!(client.proxy_uri.as_deref(), Some("http://proxy.example:3128")); + } + #[test] fn build_requires_region() { let err = Client::builder().credentials(test_credentials()).build().unwrap_err(); diff --git a/crates/codedeploy-commands/src/error.rs b/crates/codedeploy-commands/src/error.rs index 65881369..905473ad 100644 --- a/crates/codedeploy-commands/src/error.rs +++ b/crates/codedeploy-commands/src/error.rs @@ -7,11 +7,13 @@ use std::fmt; pub struct Error { kind: ErrorKind, message: String, + /// HTTP status of the response, when the error came from one. + status: Option, } impl Error { pub fn new(kind: ErrorKind, message: impl Into) -> Self { - Self { kind, message: message.into() } + Self { kind, message: message.into(), status: None } } /// The error kind. @@ -20,8 +22,28 @@ impl Error { &self.kind } - /// Parse error from HTTP response body. + /// The HTTP status code, if the error came from a service response. + /// + /// `None` for errors raised before a response was received (network, + /// signing, serialization). + #[must_use] + pub fn status(&self) -> Option { + self.status + } + + /// Attach an HTTP status code. + #[must_use] + pub fn with_status(mut self, status: u16) -> Self { + self.status = Some(status); + self + } + + /// Parse error from HTTP response body, keeping the status code. pub(crate) fn from_response(status: u16, body: &[u8]) -> Self { + Self { status: Some(status), ..Self::classify_response(status, body) } + } + + fn classify_response(status: u16, body: &[u8]) -> Self { // Detect throttling (HTTP 429) before parsing the body — the service // may return 429 with or without a JSON body. if status == 429 { @@ -285,6 +307,30 @@ mod tests { let error = Error::new(ErrorKind::Http, "test"); assert_eq!(error.kind(), &ErrorKind::Http); assert_eq!(error.message, "test"); + assert_eq!(error.status(), None); + } + + #[test] + fn from_response_keeps_status_for_service_unavailable() { + // A 503 with a ServiceUnavailableException body. + let body = r#"{"__type": "com.amazonaws.codedeploy#ServiceUnavailableException"}"#; + let error = Error::from_response(503, body.as_bytes()); + assert_eq!(error.kind(), &ErrorKind::Http); + assert_eq!(error.status(), Some(503)); + } + + #[test] + fn from_response_keeps_status_for_bodyless_500() { + let error = Error::from_response(500, b""); + assert_eq!(error.kind(), &ErrorKind::Http); + assert_eq!(error.status(), Some(500)); + } + + #[test] + fn from_response_keeps_status_when_throttled() { + let error = Error::from_response(429, b"not json"); + assert_eq!(error.kind(), &ErrorKind::Throttled); + assert_eq!(error.status(), Some(429)); } #[test] diff --git a/src/aws_clients/codedeploy_command_client.rs b/src/aws_clients/codedeploy_command_client.rs index 0fb127d0..a0a84b9d 100644 --- a/src/aws_clients/codedeploy_command_client.rs +++ b/src/aws_clients/codedeploy_command_client.rs @@ -9,8 +9,8 @@ use crate::aws_clients::throttle_gate::ThrottleGate; use aws_credential_types::Credentials as AwsCredentials; use codedeploy_commands::types::{CommandStatus, Envelope, HostCommandInstance}; use codedeploy_commands::{Client, GetDeploymentSpecificationOutput}; -use std::sync::Arc; -use std::time::Duration; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; use thiserror::Error; /// Agent version for the `x-amz-codedeploy-agent-version` header @@ -46,6 +46,40 @@ impl CodeDeployClientError { pub fn is_throttle(&self) -> bool { matches!(self, Self::Service(e) if *e.kind() == codedeploy_commands::ErrorKind::Throttled) } + + /// Returns `true` if the service answered with a 5xx response other than + /// throttling. + /// + /// This is the case where reconnecting helps: the connection is healthy, + /// so the pool would reuse it and send the retry to the same backend host. + /// After a network error or timeout the HTTP client already discards the + /// connection and opens a new one. + #[must_use] + pub fn is_server_error_response(&self) -> bool { + matches!(self, Self::Service(e) + if *e.kind() != codedeploy_commands::ErrorKind::Throttled + && e.status().is_some_and(|s| s >= 500)) + } + + /// Returns `true` if the request may succeed when sent again: throttling, + /// any 5xx, a server exception, a network failure, or IMDS being briefly + /// unreachable during a credential refresh. + /// + /// 4xx client errors and local failures (signing, serialization, a + /// poisoned lock) are not retryable. + #[must_use] + pub fn is_retryable(&self) -> bool { + use codedeploy_commands::ErrorKind; + match self { + Self::Service(e) => match e.kind() { + ErrorKind::Throttled | ErrorKind::ServerException | ErrorKind::Network => true, + ErrorKind::Http => e.status().is_some_and(|s| s >= 500), + _ => false, + }, + Self::ImdsUnavailable(_) => true, + _ => false, + } + } } /// Host command returned by `PollHostCommand`. @@ -102,9 +136,9 @@ impl AckStatus { } } -/// Mutable state that gets swapped during credential refresh. -/// Behind an `RwLock` so HTTP methods can hold a read lock (concurrent) -/// while `refresh_if_needed` briefly takes a write lock (~once per 25 min). +/// Mutable state that gets swapped during credential refresh or a connection +/// reset. HTTP methods copy the client out under a brief read lock and make the +/// request without holding it, so writers never wait on in-flight requests. struct RefreshableInner { client: Client, credential_expiry: Option, @@ -131,6 +165,8 @@ pub struct CodeDeployCommandClient { proxy_uri: Option, /// Process-wide throttle circuit breaker shared across all client instances. throttle_gate: Arc, + /// When `reset_connections` last dropped the pool (rate limit). + last_connection_reset: Mutex>, } /// Compute the credential expiry instant for the given mode. @@ -222,6 +258,7 @@ impl CodeDeployCommandClient { http_read_timeout, proxy_uri, throttle_gate, + last_connection_reset: Mutex::new(None), }) } @@ -308,6 +345,58 @@ impl CodeDeployCommandClient { Ok(()) } + /// Copy of the current inner client, taken under a brief read lock. + /// + /// Callers make the HTTP request on the copy with the lock released, so a + /// slow request (up to `http_read_timeout`) never holds the lock and + /// `reset_connections` / credential refresh only wait for the swap. The + /// copy is cheap: credentials and the reqwest client are reference-counted. + fn current_client(&self) -> Result { + let state = self + .inner + .read() + .map_err(|e| CodeDeployClientError::LockPoisoned(format!("{e}")))?; + Ok(state.client.clone()) + } + + /// Minimum time between connection resets on one client. + const MIN_CONNECTION_RESET_INTERVAL: Duration = Duration::from_mins(1); + + /// Drop pooled connections so the next request reaches the service on a + /// fresh connection, and therefore possibly a different backend host. + /// + /// Rate-limited to once per [`Self::MIN_CONNECTION_RESET_INTERVAL`] across + /// every thread using this client: each reset costs the service a TCP+TLS + /// handshake, and during a fleet-wide outage every retrying agent would + /// otherwise add one per retry. Returns whether a reset happened. + /// + /// Affects only this client's pool. The poller has its own client, so its + /// connection is untouched. Requests already in flight finish on their + /// existing connection. + /// + /// # Errors + /// Returns error if the lock is poisoned or the HTTP client cannot be rebuilt. + pub fn reset_connections(&self) -> Result { + { + let mut last = self + .last_connection_reset + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let now = Instant::now(); + if last.is_some_and(|t| now.duration_since(t) < Self::MIN_CONNECTION_RESET_INTERVAL) { + return Ok(false); + } + *last = Some(now); + } + let mut state = self + .inner + .write() + .map_err(|e| CodeDeployClientError::LockPoisoned(format!("{e}")))?; + state.client.reset_http_connections().map_err(CodeDeployClientError::Service)?; + tracing::info!("Reset CodeDeploy command service connections after a server error"); + Ok(true) + } + /// Maximum retry attempts for throttled requests. const MAX_THROTTLE_RETRIES: u32 = 5; @@ -355,11 +444,8 @@ impl CodeDeployCommandClient { ) -> Result, CodeDeployClientError> { self.retry_on_throttle(|| { self.refresh_if_needed()?; - let state = self - .inner - .read() - .map_err(|e| CodeDeployClientError::LockPoisoned(format!("{e}")))?; - let result = state.client.poll_host_command(host_identifier)?; + let client = self.current_client()?; + let result = client.poll_host_command(host_identifier)?; Ok(result.map(HostCommand::from)) }) } @@ -378,12 +464,8 @@ impl CodeDeployCommandClient { let envelope = Envelope { format: "JSON".to_string(), payload: diagnostics_payload.to_string() }; - let state = self - .inner - .read() - .map_err(|e| CodeDeployClientError::LockPoisoned(format!("{e}")))?; - let status_str = state - .client + let client = self.current_client()?; + let status_str = client .put_host_command_acknowledgement(host_command_identifier, Some(&envelope))?; Ok(AckStatus::from_response(status_str.as_deref())) @@ -401,13 +483,9 @@ impl CodeDeployCommandClient { ) -> Result { self.retry_on_throttle(|| { self.refresh_if_needed()?; - let state = self - .inner - .read() - .map_err(|e| CodeDeployClientError::LockPoisoned(format!("{e}")))?; - let output = state - .client - .get_deployment_specification(deployment_execution_id, host_identifier)?; + let client = self.current_client()?; + let output = + client.get_deployment_specification(deployment_execution_id, host_identifier)?; Ok(to_deployment_spec(output)) }) @@ -437,11 +515,8 @@ impl CodeDeployCommandClient { let envelope = Envelope { format: "JSON".to_string(), payload: diagnostics_payload.to_string() }; - let state = self - .inner - .read() - .map_err(|e| CodeDeployClientError::LockPoisoned(format!("{e}")))?; - state.client.put_host_command_complete( + let client = self.current_client()?; + client.put_host_command_complete( host_command_identifier, command_status, Some(&envelope), @@ -1045,6 +1120,137 @@ mod tests { assert!(!err.is_throttle()); } + fn service_error(kind: codedeploy_commands::ErrorKind) -> CodeDeployClientError { + CodeDeployClientError::Service(codedeploy_commands::Error::new(kind, "test")) + } + + #[test] + fn reset_connections_succeeds_and_keeps_agent_version() { + let client = CodeDeployCommandClient::new( + iam_user_credentials(), + false, + false, + None, + Duration::from_secs(80), + None, + ) + .unwrap(); + let version = client.agent_version(); + assert!(client.reset_connections().unwrap()); + assert_eq!(client.agent_version(), version); + } + + /// At most one reset per interval per client, however many threads or + /// retries ask for one. + #[test] + fn reset_connections_is_rate_limited() { + let client = CodeDeployCommandClient::new( + iam_user_credentials(), + false, + false, + None, + Duration::from_secs(80), + None, + ) + .unwrap(); + assert!(client.reset_connections().unwrap(), "first reset happens"); + assert!(!client.reset_connections().unwrap(), "second within the interval is skipped"); + assert!(!client.reset_connections().unwrap()); + + // Once the interval has passed, the next reset happens again. + let past = CodeDeployCommandClient::MIN_CONNECTION_RESET_INTERVAL + Duration::from_secs(1); + *client.last_connection_reset.lock().unwrap() = + Some(Instant::now().checked_sub(past).expect("monotonic clock older than 61s")); + assert!(client.reset_connections().unwrap(), "reset allowed after the interval"); + assert!(!client.reset_connections().unwrap()); + } + + #[test] + fn is_server_error_response_only_for_5xx_responses() { + use codedeploy_commands::{Error, ErrorKind}; + let with = |kind, status| { + CodeDeployClientError::Service(Error::new(kind, "x").with_status(status)) + }; + assert!(with(ErrorKind::Http, 503).is_server_error_response()); + assert!(with(ErrorKind::Http, 500).is_server_error_response()); + assert!(with(ErrorKind::ServerException, 500).is_server_error_response()); + // Throttling, even with a 503 status, is not a bad-host signal. + assert!(!with(ErrorKind::Throttled, 503).is_server_error_response()); + assert!(!with(ErrorKind::ClientException, 400).is_server_error_response()); + // No response: the HTTP client already reconnects. + assert!(!service_error(ErrorKind::Network).is_server_error_response()); + assert!(!CodeDeployClientError::ImdsUnavailable("x".into()).is_server_error_response()); + } + + /// A request stuck on an unresponsive backend must not hold the client + /// lock, or `reset_connections` (and every other caller behind it) would + /// wait up to `http_read_timeout`. + #[test] + fn reset_connections_does_not_wait_for_in_flight_request() { + // Accepts connections and never answers. + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let endpoint = format!("http://{}", listener.local_addr().unwrap()); + let read_timeout = Duration::from_secs(5); + let client = CodeDeployCommandClient::new( + iam_user_credentials(), + false, + false, + Some(endpoint), + read_timeout, + None, + ) + .unwrap(); + + std::thread::scope(|s| { + let stuck = s.spawn(|| client.poll_host_command("i-1234")); + // Wait until the request is in flight on the server side. + let (_conn, _) = listener.accept().unwrap(); + + let start = std::time::Instant::now(); + assert!(client.reset_connections().unwrap(), "reset must actually run"); + let elapsed = start.elapsed(); + assert!( + elapsed < Duration::from_secs(1), + "reset waited {elapsed:?} for the in-flight request" + ); + + assert!(stuck.join().unwrap().is_err(), "stuck request should time out"); + }); + } + + #[test] + fn is_retryable_for_transient_service_errors() { + use codedeploy_commands::ErrorKind; + assert!(service_error(ErrorKind::Throttled).is_retryable()); + assert!(service_error(ErrorKind::ServerException).is_retryable()); + assert!(service_error(ErrorKind::Network).is_retryable()); + assert!(CodeDeployClientError::ImdsUnavailable("timeout".into()).is_retryable()); + } + + #[test] + fn is_retryable_false_for_permanent_errors() { + use codedeploy_commands::ErrorKind; + assert!(!service_error(ErrorKind::ClientException).is_retryable()); + assert!(!service_error(ErrorKind::Signing).is_retryable()); + assert!(!service_error(ErrorKind::Serialization).is_retryable()); + assert!(!service_error(ErrorKind::Deserialization).is_retryable()); + assert!(!CodeDeployClientError::LockPoisoned("test".into()).is_retryable()); + } + + #[test] + fn is_retryable_http_error_depends_on_status() { + use codedeploy_commands::{Error, ErrorKind}; + let http = |status| { + CodeDeployClientError::Service(Error::new(ErrorKind::Http, "x").with_status(status)) + }; + assert!(http(500).is_retryable()); + assert!(http(503).is_retryable()); + assert!(!http(400).is_retryable()); + assert!(!http(404).is_retryable()); + // `Http` without a status (not from a response) is not retried. + assert!(!service_error(ErrorKind::Http).is_retryable()); + } + #[test] fn retry_on_throttle_succeeds_immediately_on_non_throttle() { let client = CodeDeployCommandClient::new( diff --git a/src/command_poller/command_processor.rs b/src/command_poller/command_processor.rs index 9b28e36f..173072ee 100644 --- a/src/command_poller/command_processor.rs +++ b/src/command_poller/command_processor.rs @@ -13,8 +13,54 @@ use crate::host_command::CommandDispatcher; use crate::lifecycle_event::ScriptError; use crate::runtime::deployment_tracker::DeploymentTracker; use std::io; -use std::time::Instant; -use tracing::{debug, error, info, info_span}; +use std::time::{Duration, Instant}; +use tracing::{debug, error, info, info_span, warn}; + +/// How long to keep retrying `PutHostCommandComplete` on transient errors. +/// +/// The service fails a lifecycle event 70 minutes after it was acknowledged +/// if no completion arrives, and discards any completion sent after that. Retrying is safe before then: the service keeps +/// the first completion it records and discards later duplicates. +#[derive(Debug, Clone, Copy)] +pub struct CompletionRetryPolicy { + /// Stop retrying this long after the command was acknowledged. + pub deadline_after_ack: Duration, + /// Backoff ceiling for the first retry; doubles each attempt. + pub initial_backoff: Duration, + /// Upper bound on any single backoff. + pub max_backoff: Duration, +} + +impl Default for CompletionRetryPolicy { + fn default() -> Self { + Self { + // 5 minutes inside the service's 70-minute timeout. + deadline_after_ack: Duration::from_mins(65), + initial_backoff: Duration::from_secs(1), + max_backoff: Duration::from_secs(30), + } + } +} + +impl CompletionRetryPolicy { + /// Full-jitter exponential backoff for retry number `attempt` (1-based). + fn backoff(&self, attempt: u32) -> Duration { + let ceiling = self + .initial_backoff + .saturating_mul(2u32.saturating_pow(attempt.saturating_sub(1))) + .min(self.max_backoff); + let ceiling_ms = u64::try_from(ceiling.as_millis()).unwrap_or(u64::MAX); + if ceiling_ms == 0 { + return Duration::ZERO; + } + let mut buf = [0u8; 8]; + // getrandom never fails on supported platforms; fall back to the ceiling. + if getrandom::getrandom(&mut buf).is_err() { + return ceiling; + } + Duration::from_millis(u64::from_le_bytes(buf) % (ceiling_ms + 1)) + } +} /// Trait abstracting the `CodeDeploy` service client for testability. pub trait CommandServiceClient { @@ -42,6 +88,15 @@ pub trait CommandServiceClient { status: &str, diagnostics_payload: &str, ) -> Result<(), CodeDeployClientError>; + + /// Drop pooled connections so the next call can reach a different backend + /// host. Best effort and may be rate-limited; the default does nothing. + /// + /// # Errors + /// Returns error if the connections could not be reset. + fn reset_connections(&self) -> Result<(), CodeDeployClientError> { + Ok(()) + } } impl CommandServiceClient for CodeDeployCommandClient { @@ -69,6 +124,11 @@ impl CommandServiceClient for CodeDeployCommandClient { ) -> Result<(), CodeDeployClientError> { self.put_host_command_complete(host_command_identifier, status, diagnostics_payload) } + + fn reset_connections(&self) -> Result<(), CodeDeployClientError> { + // Inherent method; it reports whether the rate limit allowed a reset. + CodeDeployCommandClient::reset_connections(self).map(|_| ()) + } } #[derive(Debug)] @@ -79,6 +139,7 @@ pub struct CommandProcessor CommandProcessor { @@ -95,9 +156,17 @@ impl CommandProcessor { tracker, host_identifier, deployment_system: "CodeDeploy".to_string(), + completion_retry: CompletionRetryPolicy::default(), } } + /// Override how completion reports are retried (tests use short durations). + #[must_use] + pub fn with_completion_retry_policy(mut self, policy: CompletionRetryPolicy) -> Self { + self.completion_retry = policy; + self + } + /// Process a single host command: get spec, ack, execute, report. /// /// # Errors @@ -127,6 +196,9 @@ impl CommandProcessor { ); let is_noop = self.dispatcher.is_command_noop(&command.command_name, &spec); + // Taken before the ack is sent so the deadline can only err early: the + // service starts its completion timeout when it records the ack. + let report_deadline = Instant::now() + self.completion_retry.deadline_after_ack; let status = self.send_acknowledgement(command, is_noop)?; // Wrong match here skips execution or double-executes. @@ -139,13 +211,14 @@ impl CommandProcessor { command, "Succeeded", &diagnostics::success("CompletedNoopCommand"), + report_deadline, ); return Ok(()); }, _ => return Ok(()), } - self.execute_and_report(command, &spec) + self.execute_and_report(command, &spec, report_deadline) } fn get_deployment_spec(&self, command: &HostCommand) -> io::Result<(String, String)> { @@ -214,7 +287,12 @@ impl CommandProcessor { Ok(status) } - fn execute_and_report(&self, command: &HostCommand, spec: &DeploymentSpec) -> io::Result<()> { + fn execute_and_report( + &self, + command: &HostCommand, + spec: &DeploymentSpec, + report_deadline: Instant, + ) -> io::Result<()> { self.tracker .start_tracking(&spec.deployment_id, &command.host_command_identifier) .map_err(|e| io::Error::other(format!("Failed to start deployment tracking: {e}")))?; @@ -233,7 +311,12 @@ impl CommandProcessor { "Command succeeded" ); let note = self.completion_note(&command.command_name, spec); - self.report_completion(command, "Succeeded", &diagnostics::success(¬e)); + self.report_completion( + command, + "Succeeded", + &diagnostics::success(¬e), + report_deadline, + ); }, Err(e) => { error!( @@ -247,7 +330,7 @@ impl CommandProcessor { Some(script_err) => diagnostics::from_script_error(script_err), None => diagnostics::from_error(e), }; - self.report_completion(command, "Failed", &payload); + self.report_completion(command, "Failed", &payload, report_deadline); }, } @@ -286,13 +369,74 @@ impl CommandProcessor { } } - fn report_completion(&self, command: &HostCommand, status: &str, payload: &str) { - debug!("Calling PutHostCommandComplete: \"{status}\""); - if let Err(e) = - self.client - .put_host_command_complete(&command.host_command_identifier, status, payload) - { - error!("Failed to report command completion: {e}"); + /// Report the command result, retrying transient failures until `deadline`. + /// + /// A lost completion leaves the lifecycle event `InProgress` until the + /// service's 70-minute timeout fails it, so a brief 5xx burst from one + /// backend host must not drop the result. Non-retryable errors (4xx) are + /// logged and dropped. + /// + /// Retries deliberately continue through agent shutdown: the worker's drain + /// (`kill_agent_max_wait_time_seconds`, default 2h, same as a running hook) + /// waits for them, so a stop during a backend outage can take up to the + /// deadline. Stopping early would be worse while the result lives only in + /// memory: either it is lost (a 70-minute stall), or the tracking file + /// survives and crash recovery reports `Failed` for a hook that succeeded. + /// Once the result is persisted for replay after restart, this loop should + /// honour the cancel token instead. + fn report_completion( + &self, + command: &HostCommand, + status: &str, + payload: &str, + deadline: Instant, + ) { + let host_command_id = &command.host_command_identifier; + let mut attempt: u32 = 0; + loop { + attempt += 1; + debug!(attempt, "Calling PutHostCommandComplete: \"{status}\""); + let err = match self.client.put_host_command_complete(host_command_id, status, payload) + { + Ok(()) => { + if attempt > 1 { + info!(attempt, %host_command_id, "Reported command completion after retrying"); + } + return; + }, + Err(e) => e, + }; + + let now = Instant::now(); + if !err.is_retryable() || now >= deadline { + error!( + attempt, + %host_command_id, + retryable = err.is_retryable(), + "Failed to report command completion: {err}" + ); + return; + } + + let wait = self.completion_retry.backoff(attempt).min(deadline - now); + warn!( + attempt, + %host_command_id, + wait_ms = u64::try_from(wait.as_millis()).unwrap_or(u64::MAX), + "Transient error reporting command completion, retrying: {err}" + ); + // A 5xx response may come from one bad backend host that the pooled + // keep-alive connection is pinned to; reconnect so the retry can land + // elsewhere. Network errors and timeouts already get a new + // connection, and throttling is not host-specific. The client + // rate-limits resets so a fleet-wide outage doesn't become a + // handshake storm. + if err.is_server_error_response() + && let Err(e) = self.client.reset_connections() + { + warn!("Failed to reset connections before retry: {e}"); + } + std::thread::sleep(wait); } } } @@ -413,7 +557,7 @@ mod tests { ); let mut cmd = test_command(); cmd.command_name = "UnknownCommand".into(); - let result = processor.execute_and_report(&cmd, &test_spec()); + let result = processor.execute_and_report(&cmd, &test_spec(), Instant::now()); assert!(result.is_err()); } @@ -437,7 +581,7 @@ mod tests { std::fs::create_dir_all(dir.path().join("src")).unwrap(); std::fs::write(dir.path().join("src/appspec.yml"), "version: 0.0\nos: linux\n").unwrap(); - let result = processor.execute_and_report(&cmd, &spec); + let result = processor.execute_and_report(&cmd, &spec, Instant::now()); assert!(result.is_ok()); } @@ -450,8 +594,9 @@ mod tests { test_tracker(&dir), "i-1234".into(), ); - processor.report_completion(&test_command(), "Succeeded", "{}"); - processor.report_completion(&test_command(), "Failed", "{}"); + // Deadline already passed: one attempt each, no retry against the real endpoint. + processor.report_completion(&test_command(), "Succeeded", "{}", Instant::now()); + processor.report_completion(&test_command(), "Failed", "{}", Instant::now()); } #[test] @@ -525,7 +670,7 @@ mod tests { // Use a noop hook command — will succeed without needing real files let cmd = test_command(); // AfterInstall with empty mapping = noop - let result = processor.execute_and_report(&cmd, &test_spec()); + let result = processor.execute_and_report(&cmd, &test_spec(), Instant::now()); // Succeeds (noop hook) or fails (unsupported) — either way tracking is cleaned let _ = result; @@ -578,15 +723,34 @@ mod tests { // --- Mock-based tests for ack-status branching logic --- use std::cell::RefCell; + use std::collections::VecDeque; struct MockClient { ack_status: AckStatus, + /// Every `put_host_command_complete` call, including failed attempts. completions: RefCell>, + /// Errors returned by successive completion calls before they succeed. + complete_errors: RefCell>, + /// When set, every completion call fails with this error kind. + always_fail: Option, + /// Number of `reset_connections` calls. + resets: std::cell::Cell, } impl MockClient { fn new(ack_status: AckStatus) -> Self { - Self { ack_status, completions: RefCell::new(Vec::new()) } + Self { + ack_status, + completions: RefCell::new(Vec::new()), + complete_errors: RefCell::new(VecDeque::new()), + always_fail: None, + resets: std::cell::Cell::new(0), + } + } + + fn failing_completions_with(mut self, errors: Vec) -> Self { + self.complete_errors = RefCell::new(errors.into()); + self } } @@ -646,6 +810,23 @@ mod tests { ) -> Result<(), crate::aws_clients::codedeploy_command_client::CodeDeployClientError> { self.completions.borrow_mut().push((status.into(), diagnostics_payload.into())); + if let Some(kind) = &self.always_fail { + return Err(CodeDeployClientError::Service( + codedeploy_commands::Error::new(kind.clone(), "always failing") + .with_status(503), + )); + } + match self.complete_errors.borrow_mut().pop_front() { + Some(e) => Err(e), + None => Ok(()), + } + } + + fn reset_connections( + &self, + ) -> Result<(), crate::aws_clients::codedeploy_command_client::CodeDeployClientError> + { + self.resets.set(self.resets.get() + 1); Ok(()) } } @@ -788,6 +969,206 @@ mod tests { ); } + // --- Completion retry on transient errors --- + + /// Millisecond-scale policy so retry tests run fast. + fn fast_retry_policy(deadline: Duration) -> CompletionRetryPolicy { + CompletionRetryPolicy { + deadline_after_ack: deadline, + initial_backoff: Duration::from_millis(1), + max_backoff: Duration::from_millis(5), + } + } + + fn http_error(status: u16) -> CodeDeployClientError { + CodeDeployClientError::Service( + codedeploy_commands::Error::new(codedeploy_commands::ErrorKind::Http, "boom") + .with_status(status), + ) + } + + fn kind_error(kind: codedeploy_commands::ErrorKind) -> CodeDeployClientError { + CodeDeployClientError::Service(codedeploy_commands::Error::new(kind, "boom")) + } + + fn processor_with( + dir: &TempDir, + client: MockClient, + policy: CompletionRetryPolicy, + ) -> CommandProcessor, MockClient> { + CommandProcessor::new(client, test_dispatcher(dir), test_tracker(dir), "i-1234".into()) + .with_completion_retry_policy(policy) + } + + /// The 10-15 and 10-23 incidents: a burst of 503s / bodyless 500s / resets + /// on `PutHostCommandComplete`. The result must still land, unchanged. + #[test] + fn transient_completion_errors_are_retried_until_success() { + let dir = TempDir::new().unwrap(); + let client = MockClient::new(AckStatus::Failed).failing_completions_with(vec![ + http_error(503), + http_error(500), + kind_error(codedeploy_commands::ErrorKind::Network), + kind_error(codedeploy_commands::ErrorKind::ServerException), + ]); + let processor = processor_with(&dir, client, fast_retry_policy(Duration::from_secs(10))); + let mut cmd = test_command(); + cmd.command_name = "UnmappedHook".into(); // noop -> reports Succeeded on Failed ack + + assert!(processor.process(&cmd).is_ok()); + + let completions = processor.client.completions.borrow(); + assert_eq!(completions.len(), 5, "4 transient failures then 1 success"); + assert!( + completions.iter().all(|(status, _)| status == "Succeeded"), + "every attempt must resend the real result, never downgrade it" + ); + } + + #[test] + fn hook_result_is_retried_after_execution() { + let dir = TempDir::new().unwrap(); + let client = MockClient::new(AckStatus::InProgress) + .failing_completions_with(vec![http_error(503), http_error(503)]); + let processor = processor_with(&dir, client, fast_retry_policy(Duration::from_secs(10))); + let mut cmd = test_command(); + cmd.command_name = "AfterInstall".into(); + + let _ = processor.process(&cmd); + + let completions = processor.client.completions.borrow(); + assert_eq!(completions.len(), 3, "2 transient failures then 1 success"); + let first = &completions[0]; + assert!(completions.iter().all(|c| c == first), "retries resend the same status/payload"); + drop(completions); + assert!(!processor.tracker.is_deployment_in_progress().unwrap()); + } + + #[test] + fn client_errors_are_not_retried() { + let dir = TempDir::new().unwrap(); + let client = MockClient::new(AckStatus::Failed).failing_completions_with(vec![ + kind_error(codedeploy_commands::ErrorKind::ClientException), + http_error(503), // must never be reached + ]); + let processor = processor_with(&dir, client, fast_retry_policy(Duration::from_secs(10))); + let mut cmd = test_command(); + cmd.command_name = "UnmappedHook".into(); + + assert!(processor.process(&cmd).is_ok()); + assert_eq!(processor.client.completions.borrow().len(), 1); + } + + #[test] + fn http_4xx_is_not_retried() { + let dir = TempDir::new().unwrap(); + let client = MockClient::new(AckStatus::Failed) + .failing_completions_with(vec![http_error(400), http_error(503)]); + let processor = processor_with(&dir, client, fast_retry_policy(Duration::from_secs(10))); + let mut cmd = test_command(); + cmd.command_name = "UnmappedHook".into(); + + assert!(processor.process(&cmd).is_ok()); + assert_eq!(processor.client.completions.borrow().len(), 1); + } + + /// Retrying stops at the deadline even if the backend never recovers. + #[test] + fn retries_stop_at_deadline() { + let dir = TempDir::new().unwrap(); + let mut client = MockClient::new(AckStatus::Failed); + client.always_fail = Some(codedeploy_commands::ErrorKind::Http); + let deadline = Duration::from_millis(200); + let processor = processor_with(&dir, client, fast_retry_policy(deadline)); + let mut cmd = test_command(); + cmd.command_name = "UnmappedHook".into(); + + let start = Instant::now(); + assert!(processor.process(&cmd).is_ok()); + let elapsed = start.elapsed(); + + let attempts = processor.client.completions.borrow().len(); + assert!(attempts > 1, "should have retried; got {attempts} attempt(s)"); + assert!( + elapsed < deadline + Duration::from_secs(2), + "must give up near the deadline; took {elapsed:?}" + ); + } + + /// Only complete 5xx responses reconnect: there the pooled connection is + /// healthy and would carry the retry back to the same backend host. + /// Network errors and timeouts already get a new connection from the HTTP + /// client, and throttling is not host-specific. + #[test] + fn only_server_error_responses_reset_connections() { + let dir = TempDir::new().unwrap(); + let client = MockClient::new(AckStatus::Failed).failing_completions_with(vec![ + http_error(503), + kind_error(codedeploy_commands::ErrorKind::Throttled), + kind_error(codedeploy_commands::ErrorKind::Network), + http_error(500), + ]); + let processor = processor_with(&dir, client, fast_retry_policy(Duration::from_secs(10))); + let mut cmd = test_command(); + cmd.command_name = "UnmappedHook".into(); + + assert!(processor.process(&cmd).is_ok()); + assert_eq!(processor.client.completions.borrow().len(), 5); + assert_eq!(processor.client.resets.get(), 2, "only the 503 and the 500 reconnect"); + } + + #[test] + fn non_retryable_errors_do_not_reset_connections() { + let dir = TempDir::new().unwrap(); + let client = + MockClient::new(AckStatus::Failed).failing_completions_with(vec![http_error(400)]); + let processor = processor_with(&dir, client, fast_retry_policy(Duration::from_secs(10))); + let mut cmd = test_command(); + cmd.command_name = "UnmappedHook".into(); + + assert!(processor.process(&cmd).is_ok()); + assert_eq!(processor.client.resets.get(), 0); + } + + #[tracing_test::traced_test] + #[test] + fn retry_and_recovery_are_logged() { + let dir = TempDir::new().unwrap(); + let client = + MockClient::new(AckStatus::Failed).failing_completions_with(vec![http_error(503)]); + let processor = processor_with(&dir, client, fast_retry_policy(Duration::from_secs(10))); + let mut cmd = test_command(); + cmd.command_name = "UnmappedHook".into(); + + assert!(processor.process(&cmd).is_ok()); + assert!(logs_contain("Transient error reporting command completion, retrying")); + assert!(logs_contain("Reported command completion after retrying")); + } + + #[test] + fn backoff_is_bounded_by_ceiling() { + let policy = CompletionRetryPolicy { + deadline_after_ack: Duration::from_mins(65), + initial_backoff: Duration::from_secs(1), + max_backoff: Duration::from_secs(30), + }; + for attempt in 1..=20 { + let ceiling = Duration::from_secs(1) + .saturating_mul(2u32.saturating_pow(attempt - 1)) + .min(Duration::from_secs(30)); + for _ in 0..20 { + assert!(policy.backoff(attempt) <= ceiling, "attempt {attempt}"); + } + } + // Large attempt counts must not overflow. + assert!(policy.backoff(u32::MAX) <= Duration::from_secs(30)); + } + + #[test] + fn default_policy_stays_inside_service_timeout() { + assert!(CompletionRetryPolicy::default().deadline_after_ack < Duration::from_mins(70)); + } + #[test] fn ack_failed_without_noop_returns_early() { let dir = TempDir::new().unwrap(); From b6934efef0e6c37bc67dc49ee3cad7587bda8dba Mon Sep 17 00:00:00 2001 From: Nathan Inge Date: Tue, 3 Nov 2026 15:51:58 +0000 Subject: [PATCH 3/8] Preserve directory symlinks and replace existing links Adds an opt-in preserve_directory_symlinks setting (default false, Linux and macOS only) that installs a symlink to a directory in the revision as a symlink instead of copying the directory it points to (#152). The permissions section is never applied through a preserved link; a warning is logged instead. Installing a symlink over an existing file or symlink now replaces it atomically (#143), using a temporary link and rename. A symlink over an existing directory is refused before any file is copied. Cleanup also removes dangling symlinks. --- conf/codedeployagent.yml | 5 + src/config/mod.rs | 21 ++ src/host_command/commands/install.rs | 3 +- src/installer/builder.rs | 21 +- src/installer/commands/copy_command.rs | 137 +++++++++- src/installer/commands/remove_command.rs | 32 ++- src/installer/core.rs | 314 ++++++++++++++++++++++- 7 files changed, 518 insertions(+), 15 deletions(-) diff --git a/conf/codedeployagent.yml b/conf/codedeployagent.yml index 03688648..2e3fb84a 100644 --- a/conf/codedeployagent.yml +++ b/conf/codedeployagent.yml @@ -39,6 +39,11 @@ # Any reuse failure falls back to a normal download. :enable_archive_reuse: true +# Copy a symlink to a directory in the revision as a symlink (default: false). +# When false, the agent copies the contents of the directory the link points to. +# Linux and macOS only. +# :preserve_directory_symlinks: false + # Security :use_fips_mode: false :enable_auth_policy: false diff --git a/src/config/mod.rs b/src/config/mod.rs index 72bb8b30..fb88cc2c 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -352,6 +352,10 @@ pub struct AgentConfig { /// turning this off costs only the optimisation. pub enable_archive_reuse: bool, + /// Copy a symlink to a directory in the revision as a symlink instead of + /// copying the directory it points to. Default: `false`. Unix only. + pub preserve_directory_symlinks: bool, + /// Capture Amazon S3 HTTP wire logs to `.aws_wire.log` in /// `log_dir`. Default: `false`. /// @@ -420,6 +424,7 @@ impl Default for AgentConfig { disable_imds_v1: false, enable_command_port: false, enable_archive_reuse: true, + preserve_directory_symlinks: false, log_aws_wire: false, disable_core_dumps: true, hardening: HardeningConfig::default(), @@ -962,6 +967,22 @@ mod tests { assert!(AgentConfig::default().enable_archive_reuse); } + #[test] + fn default_preserve_directory_symlinks_is_false() { + assert!(!AgentConfig::default().preserve_directory_symlinks); + } + + #[test] + fn preserve_directory_symlinks_parses_from_yaml() { + let yaml = "preserve_directory_symlinks: true\n"; + let config = AgentConfig::from_yaml(yaml, Path::new("test.yml")).unwrap(); + assert!(config.preserve_directory_symlinks); + + let yaml = ":preserve_directory_symlinks: true\n"; + let config = AgentConfig::from_yaml(yaml, Path::new("test.yml")).unwrap(); + assert!(config.preserve_directory_symlinks); + } + #[test] fn enable_archive_reuse_parses_from_yaml() { let yaml = "enable_archive_reuse: false\n"; diff --git a/src/host_command/commands/install.rs b/src/host_command/commands/install.rs index 27ecfd44..0da1a75d 100644 --- a/src/host_command/commands/install.rs +++ b/src/host_command/commands/install.rs @@ -70,7 +70,8 @@ impl InstallCommand { .with_restrict_permissions(self.config.hardening.restrict_agent_dir_permissions) .with_reject_symlink_permission_targets( self.config.hardening.reject_symlink_permission_targets, - ); + ) + .with_preserve_directory_symlinks(self.config.preserve_directory_symlinks); debug!("Installing revision in instance group {}", spec.deployment_group_id); diff --git a/src/installer/builder.rs b/src/installer/builder.rs index cb6cc538..5cea1d39 100644 --- a/src/installer/builder.rs +++ b/src/installer/builder.rs @@ -9,6 +9,7 @@ use crate::installer::{InstallerError, Result}; use nu_path; use serde_json::Value; use std::collections::{HashMap, HashSet}; +use std::fs; use std::path::{Path, PathBuf}; use tracing::debug; @@ -280,6 +281,21 @@ impl CommandBuilder { self.copy_targets.contains_key(&file) } + /// Whether `file` is a copy target whose source is a symlink to a + /// directory. Permissions are not applied to such targets. + #[must_use] + pub fn copying_directory_link(&self, file: &Path) -> bool { + let file = Self::expand_path(file); + self.copy_targets + .get(&file) + .is_some_and(|source| Self::is_directory_link(source)) + } + + fn is_directory_link(source: &Path) -> bool { + fs::symlink_metadata(source).is_ok_and(|m| m.is_symlink()) + && fs::metadata(source).is_ok_and(|m| m.is_dir()) + } + #[must_use] pub fn making_directory(&self, dir: &Path) -> bool { debug!("Checking if making directory {}", dir.display()); @@ -294,7 +310,10 @@ impl CommandBuilder { let mut matches = Vec::new(); if permission.types().contains(&crate::application_specification::ObjectType::File) { - for object in self.copy_targets.keys() { + for (object, source) in &self.copy_targets { + if Self::is_directory_link(source) { + continue; + } if permission.matches_pattern(object) && !permission.matches_except(object) { // The directory-object path validates only the ACL per // matched file, never the pattern/except (that validation diff --git a/src/installer/commands/copy_command.rs b/src/installer/commands/copy_command.rs index 2247b382..41d52ce4 100644 --- a/src/installer/commands/copy_command.rs +++ b/src/installer/commands/copy_command.rs @@ -37,7 +37,7 @@ impl CopyCommand { if metadata.is_symlink() { let target = fs::read_link(&self.source)?; #[cfg(unix)] - std::os::unix::fs::symlink(&target, &self.destination)?; + self.create_symlink(&target)?; #[cfg(windows)] { // Symlink creation may fail on Windows; fall back to copy @@ -116,6 +116,60 @@ impl CopyCommand { Ok(()) } + /// Create `destination` as a symlink to `target`, replacing an existing + /// file or symlink there. The link is created under a temporary name and + /// renamed over `destination`, so the path is never missing. An existing + /// directory is left in place and returned as an error. + #[cfg(unix)] + fn create_symlink(&self, target: &std::path::Path) -> Result<()> { + match fs::symlink_metadata(&self.destination) { + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + std::os::unix::fs::symlink(target, &self.destination)?; + return Ok(()); + }, + Err(e) => return Err(e.into()), + Ok(meta) if meta.is_dir() => { + return Err(std::io::Error::new( + std::io::ErrorKind::AlreadyExists, + format!( + "cannot replace directory {} with a symlink", + self.destination.display() + ), + ) + .into()); + }, + Ok(_) => {}, + } + + let tmp = self.create_temp_symlink(target)?; + if let Err(e) = fs::rename(&tmp, &self.destination) { + let _ = fs::remove_file(&tmp); + return Err(e.into()); + } + Ok(()) + } + + /// Create a symlink to `target` beside `destination` under an unused + /// random name and return its path. + #[cfg(unix)] + fn create_temp_symlink(&self, target: &std::path::Path) -> Result { + const ATTEMPTS: usize = 8; + let mut last_err = None; + for _ in 0..ATTEMPTS { + let mut suffix = [0u8; 8]; + getrandom::getrandom(&mut suffix).map_err(std::io::Error::other)?; + let tmp = self + .destination + .with_file_name(format!(".cd-{:016x}.tmp", u64::from_le_bytes(suffix))); + match std::os::unix::fs::symlink(target, &tmp) { + Ok(()) => return Ok(tmp), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => last_err = Some(e), + Err(e) => return Err(e.into()), + } + } + Err(last_err.unwrap_or_else(|| std::io::Error::other("no temporary name")).into()) + } + /// Serialize command to JSON hash for debugging/audit #[must_use] pub fn to_h(&self) -> Value { @@ -257,4 +311,85 @@ mod tests { fs::remove_file(&src).ok(); fs::remove_file(&dst).ok(); } + + #[cfg(unix)] + fn symlink_source(dir: &std::path::Path) -> PathBuf { + let source = dir.join("src-link"); + std::os::unix::fs::symlink("new-target", &source).unwrap(); + source + } + + #[cfg(unix)] + #[test] + fn execute_symlink_replaces_existing_symlink() { + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("dest"); + std::os::unix::fs::symlink("old-target", &dest).unwrap(); + + let cmd = CopyCommand::new(symlink_source(dir.path()), dest.clone()); + cmd.execute(&mut Vec::new()).unwrap(); + + assert_eq!(fs::read_link(&dest).unwrap(), PathBuf::from("new-target")); + } + + #[cfg(unix)] + #[test] + fn execute_symlink_replaces_existing_file() { + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("dest"); + fs::write(&dest, "file").unwrap(); + + let cmd = CopyCommand::new(symlink_source(dir.path()), dest.clone()); + cmd.execute(&mut Vec::new()).unwrap(); + + assert_eq!(fs::read_link(&dest).unwrap(), PathBuf::from("new-target")); + } + + #[cfg(unix)] + #[test] + fn execute_symlink_does_not_replace_directory() { + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("dest"); + fs::create_dir(&dest).unwrap(); + fs::write(dest.join("keep.txt"), "keep").unwrap(); + + let cmd = CopyCommand::new(symlink_source(dir.path()), dest.clone()); + let mut cleanup = Vec::new(); + assert!(cmd.execute(&mut cleanup).is_err()); + + assert!(cleanup.is_empty(), "refused directory must not be recorded for cleanup"); + assert!(dest.join("keep.txt").exists(), "existing directory should be untouched"); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 2, "no temporary link left behind"); + } + + #[cfg(unix)] + #[test] + fn execute_symlink_replaces_symlink_to_directory() { + let dir = tempfile::TempDir::new().unwrap(); + let target = dir.path().join("target-dir"); + fs::create_dir(&target).unwrap(); + fs::write(target.join("keep.txt"), "keep").unwrap(); + let dest = dir.path().join("dest"); + std::os::unix::fs::symlink(&target, &dest).unwrap(); + + let cmd = CopyCommand::new(symlink_source(dir.path()), dest.clone()); + cmd.execute(&mut Vec::new()).unwrap(); + + assert_eq!(fs::read_link(&dest).unwrap(), std::path::Path::new("new-target")); + assert!(target.join("keep.txt").exists(), "old link target should be untouched"); + } + + #[cfg(unix)] + #[test] + fn temp_symlink_name_has_fixed_length() { + let dir = tempfile::TempDir::new().unwrap(); + let long = "a".repeat(250); + let dest = dir.path().join(&long); + fs::write(&dest, "old").unwrap(); + + let cmd = CopyCommand::new(symlink_source(dir.path()), dest.clone()); + cmd.execute(&mut Vec::new()).unwrap(); + + assert_eq!(fs::read_link(&dest).unwrap(), std::path::Path::new("new-target")); + } } diff --git a/src/installer/commands/remove_command.rs b/src/installer/commands/remove_command.rs index 22bde5fd..1ba55b6d 100644 --- a/src/installer/commands/remove_command.rs +++ b/src/installer/commands/remove_command.rs @@ -21,11 +21,9 @@ impl RemoveCommand { pub fn execute(&self, _cleanup_file: &mut dyn Write) -> Result<()> { let path = &self.file_path; - if !path.exists() { + let Ok(metadata) = fs::symlink_metadata(path) else { return Ok(()); - } - - let metadata = fs::symlink_metadata(path)?; + }; if metadata.is_symlink() { fs::remove_file(path)?; @@ -88,6 +86,32 @@ mod tests { fs::remove_file(&target).ok(); } + #[cfg(unix)] + #[test] + fn execute_removes_dangling_symlink() { + let dir = tempfile::TempDir::new().unwrap(); + let link = dir.path().join("dangling"); + unix_fs::symlink("/nonexistent", &link).unwrap(); + + let cmd = RemoveCommand::new(link.clone()); + cmd.execute(&mut Vec::new()).unwrap(); + + assert!(fs::symlink_metadata(&link).is_err()); + } + + #[cfg(unix)] + #[test] + fn execute_skips_path_under_a_file() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("file"); + fs::write(&file, "x").unwrap(); + + let cmd = RemoveCommand::new(file.join("child")); + cmd.execute(&mut Vec::new()).unwrap(); + + assert!(file.exists()); + } + #[test] fn execute_directory() { let dir = std::env::temp_dir().join("test_remove_dir"); diff --git a/src/installer/core.rs b/src/installer/core.rs index 5585af20..f1e240ec 100644 --- a/src/installer/core.rs +++ b/src/installer/core.rs @@ -18,8 +18,8 @@ use tracing::{debug, info}; // `system::agent_file_mode`: 0644 by default, 0600 under // `restrict_agent_dir_permissions`. -// Four independent opt-in hardening toggles mirroring their AgentConfig -// fields; an enum would force invalid combinations to be representable. +// Independent opt-in toggles mirroring their AgentConfig fields; an enum +// would force invalid combinations to be representable. #[allow(clippy::struct_excessive_bools)] #[derive(Debug)] pub struct Installer { @@ -31,6 +31,7 @@ pub struct Installer { reject_path_traversal: bool, reject_symlink_permission_targets: bool, restrict_permissions: bool, + preserve_directory_symlinks: bool, } impl Installer { @@ -75,6 +76,7 @@ impl Installer { reject_path_traversal, reject_symlink_permission_targets: false, restrict_permissions: false, + preserve_directory_symlinks: false, } } @@ -96,6 +98,17 @@ impl Installer { self } + /// Copy a symlink to a directory as a symlink instead of walking into it, + /// from `preserve_directory_symlinks`. Defaults to `false`. Unix only. + #[must_use] + pub fn with_preserve_directory_symlinks(mut self, preserve: bool) -> Self { + if preserve && !cfg!(unix) { + tracing::warn!("preserve_directory_symlinks is not supported on this platform"); + } + self.preserve_directory_symlinks = cfg!(unix) && preserve; + self + } + /// Writes files to the customer filesystem; wrong behavior corrupts deployments. /// /// # Errors @@ -240,10 +253,9 @@ impl Installer { let behavior = spec.file_exists_behavior().unwrap_or(self.file_exists_behavior); - // Handle directory copy - if source.is_dir() { + if self.is_directory(&source) { Self::fill_missing_ancestors(builder, Path::new(file_mapping.destination()))?; - Self::generate_directory_copy( + self.generate_directory_copy( builder, &source, Path::new(file_mapping.destination()), @@ -285,6 +297,14 @@ impl Installer { return Ok(()); } + if builder.copying_directory_link(object) { + tracing::warn!( + object = %object.display(), + "Skipping permissions for a directory symlink" + ); + return Ok(()); + } + // Handle file permissions if !permission.types().contains(&crate::application_specification::ObjectType::File) { return Ok(()); @@ -298,7 +318,19 @@ impl Installer { Ok(()) } + /// Whether `path` is walked as a directory. With + /// `preserve_directory_symlinks` a symlink is never walked, even when it + /// points at a directory. + fn is_directory(&self, path: &Path) -> bool { + if self.preserve_directory_symlinks { + fs::symlink_metadata(path).is_ok_and(|m| m.is_dir()) + } else { + path.is_dir() + } + } + fn generate_directory_copy( + &self, builder: &mut CommandBuilder, source: &Path, dest: &Path, @@ -315,8 +347,8 @@ impl Installer { let entry_path = entry.path(); let entry_dest = dest.join(entry.file_name()); - if entry_path.is_dir() { - Self::generate_directory_copy(builder, &entry_path, &entry_dest, behavior)?; + if self.is_directory(&entry_path) { + self.generate_directory_copy(builder, &entry_path, &entry_dest, behavior)?; } else { Self::generate_normal_copy(builder, &entry_path, &entry_dest, behavior)?; } @@ -331,7 +363,14 @@ impl Installer { dest: &Path, behavior: FileExistsBehavior, ) -> Result<()> { - if dest.exists() { + // A symlink source cannot be written through a dangling symlink at + // `dest`, so for it any entry at `dest` counts as existing. + let dest_exists = if fs::symlink_metadata(source).is_ok_and(|m| m.is_symlink()) { + fs::symlink_metadata(dest).is_ok() + } else { + dest.exists() + }; + if dest_exists { match behavior { FileExistsBehavior::Disallow => { return Err(InstallerError::FileAlreadyExists { @@ -783,6 +822,265 @@ files: ); } + /// Bundle with `releases/v1/app.txt` and `current -> releases/v1`. + #[cfg(unix)] + fn archive_with_directory_symlink() -> TempDir { + let archive_dir = TempDir::new().unwrap(); + fs::create_dir_all(archive_dir.path().join("releases/v1")).unwrap(); + fs::write(archive_dir.path().join("releases/v1/app.txt"), "v1").unwrap(); + std::os::unix::fs::symlink("releases/v1", archive_dir.path().join("current")).unwrap(); + archive_dir + } + + #[cfg(unix)] + fn whole_revision_spec(dest: &Path) -> AppSpec { + AppSpec::parse(&format!( + "version: 0.0\nos: linux\nfiles:\n - source: /\n destination: {}\n", + dest.display() + )) + .unwrap() + } + + #[cfg(unix)] + #[test] + fn install_copies_directory_symlink_contents_by_default() { + let archive_dir = archive_with_directory_symlink(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ); + installer.install("test-group", &whole_revision_spec(dest_dir.path())).unwrap(); + + let current = dest_dir.path().join("current"); + assert!( + !fs::symlink_metadata(¤t).unwrap().is_symlink(), + "current should be a copied directory" + ); + assert_eq!(fs::read_to_string(current.join("app.txt")).unwrap(), "v1"); + } + + #[cfg(unix)] + #[test] + fn install_preserves_directory_symlink_when_enabled() { + let archive_dir = archive_with_directory_symlink(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ) + .with_preserve_directory_symlinks(true); + installer.install("test-group", &whole_revision_spec(dest_dir.path())).unwrap(); + + let current = dest_dir.path().join("current"); + assert!( + fs::symlink_metadata(¤t).unwrap().is_symlink(), + "current should be a symlink" + ); + assert_eq!(fs::read_link(¤t).unwrap(), Path::new("releases/v1")); + assert_eq!(fs::read_to_string(current.join("app.txt")).unwrap(), "v1"); + } + + #[cfg(unix)] + #[test] + fn install_preserves_directory_symlink_pointing_outside_revision() { + let archive_dir = TempDir::new().unwrap(); + let outside = TempDir::new().unwrap(); + fs::write(outside.path().join("secret.txt"), "outside").unwrap(); + std::os::unix::fs::symlink(outside.path(), archive_dir.path().join("shared")).unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ) + .with_preserve_directory_symlinks(true); + installer.install("test-group", &whole_revision_spec(dest_dir.path())).unwrap(); + + let shared = dest_dir.path().join("shared"); + assert!(fs::symlink_metadata(&shared).unwrap().is_symlink()); + assert_eq!(fs::read_link(&shared).unwrap(), outside.path()); + let install_json = + fs::read_to_string(instructions_dir.path().join("test-group-install.json")).unwrap(); + assert!(!install_json.contains("secret.txt")); + } + + #[cfg(unix)] + #[test] + fn install_preserves_directory_symlink_given_as_source() { + let archive_dir = archive_with_directory_symlink(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ) + .with_preserve_directory_symlinks(true); + let spec = AppSpec::parse(&format!( + "version: 0.0\nos: linux\nfiles:\n - source: current\n destination: {}\n", + dest_dir.path().display() + )) + .unwrap(); + installer.install("test-group", &spec).unwrap(); + + let current = dest_dir.path().join("current"); + assert!(fs::symlink_metadata(¤t).unwrap().is_symlink()); + } + + #[cfg(unix)] + #[test] + fn redeploy_removes_directory_symlink_and_keeps_its_target() { + let archive_dir = archive_with_directory_symlink(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + let outside = TempDir::new().unwrap(); + fs::write(outside.path().join("keep.txt"), "keep").unwrap(); + std::os::unix::fs::symlink(outside.path(), archive_dir.path().join("shared")).unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Disallow, + ) + .with_preserve_directory_symlinks(true); + let spec = whole_revision_spec(dest_dir.path()); + installer.install("test-group", &spec).unwrap(); + installer.install("test-group", &spec).unwrap(); + + assert!(fs::symlink_metadata(dest_dir.path().join("shared")).unwrap().is_symlink()); + assert_eq!(fs::read_to_string(outside.path().join("keep.txt")).unwrap(), "keep"); + } + + /// A symlink in the revision whose destination already holds a symlink + /// not created by an earlier deployment. + #[cfg(unix)] + fn install_symlink_over_existing_link( + behavior: FileExistsBehavior, + existing_target: &str, + ) -> (Result<()>, TempDir) { + let archive_dir = TempDir::new().unwrap(); + fs::write(archive_dir.path().join("v2.txt"), "v2").unwrap(); + std::os::unix::fs::symlink("v2.txt", archive_dir.path().join("link")).unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + std::os::unix::fs::symlink(existing_target, dest_dir.path().join("link")).unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + behavior, + ); + let spec = AppSpec::parse(&format!( + "version: 0.0\nos: linux\nfiles:\n - source: link\n destination: {}\n", + dest_dir.path().display() + )) + .unwrap(); + (installer.install("test-group", &spec), dest_dir) + } + + #[cfg(unix)] + #[test] + fn overwrite_replaces_existing_symlink() { + let (result, dest_dir) = + install_symlink_over_existing_link(FileExistsBehavior::Overwrite, "old.txt"); + result.unwrap(); + assert_eq!(fs::read_link(dest_dir.path().join("link")).unwrap(), Path::new("v2.txt")); + } + + #[cfg(unix)] + #[test] + fn overwrite_replaces_dangling_symlink() { + let (result, dest_dir) = + install_symlink_over_existing_link(FileExistsBehavior::Overwrite, "/nonexistent"); + result.unwrap(); + assert_eq!(fs::read_link(dest_dir.path().join("link")).unwrap(), Path::new("v2.txt")); + } + + #[cfg(unix)] + #[test] + fn retain_keeps_dangling_symlink() { + let (result, dest_dir) = + install_symlink_over_existing_link(FileExistsBehavior::Retain, "/nonexistent"); + result.unwrap(); + assert_eq!(fs::read_link(dest_dir.path().join("link")).unwrap(), Path::new("/nonexistent")); + } + + #[cfg(unix)] + #[test] + fn regular_file_writes_through_dangling_symlink() { + let archive_dir = TempDir::new().unwrap(); + fs::write(archive_dir.path().join("app.txt"), "new").unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + std::os::unix::fs::symlink("/nonexistent", dest_dir.path().join("app.txt")).unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Disallow, + ); + let spec = AppSpec::parse(&format!( + "version: 0.0\nos: linux\nfiles:\n - source: app.txt\n destination: {}\n", + dest_dir.path().display() + )) + .unwrap(); + installer.install("test-group", &spec).unwrap(); + + assert_eq!(fs::read_to_string(dest_dir.path().join("app.txt")).unwrap(), "new"); + } + + #[cfg(unix)] + #[test] + fn file_permissions_skip_preserved_directory_symlink() { + use std::os::unix::fs::PermissionsExt; + let archive_dir = TempDir::new().unwrap(); + let outside = TempDir::new().unwrap(); + fs::set_permissions(outside.path(), fs::Permissions::from_mode(0o755)).unwrap(); + std::os::unix::fs::symlink(outside.path(), archive_dir.path().join("shared")).unwrap(); + fs::write(archive_dir.path().join("app.txt"), "app").unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ) + .with_preserve_directory_symlinks(true); + let dest = dest_dir.path().display(); + let spec = AppSpec::parse(&format!( + "version: 0.0\nos: linux\nfiles:\n - source: /\n destination: {dest}\n\ + permissions:\n - object: {dest}\n mode: 600\n type:\n - file\n \ + - object: {dest}/shared\n mode: 600\n" + )) + .unwrap(); + installer.install("test-group", &spec).unwrap(); + + let mode = fs::metadata(outside.path()).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o755, "link target mode should be unchanged"); + let app_mode = + fs::metadata(dest_dir.path().join("app.txt")).unwrap().permissions().mode() & 0o777; + assert_eq!(app_mode, 0o600, "regular files should still get file permissions"); + } + + #[cfg(unix)] + #[test] + fn disallow_rejects_dangling_symlink() { + let (result, _dest_dir) = + install_symlink_over_existing_link(FileExistsBehavior::Disallow, "/nonexistent"); + assert!(matches!(result, Err(InstallerError::FileAlreadyExists { .. }))); + } + #[test] fn install_leading_slash_source_resolves_inside_archive() { let archive_dir = TempDir::new().unwrap(); From cd848348b99c205aec7ed46421f4891e4dd4c1ab Mon Sep 17 00:00:00 2001 From: Nathan Inge Date: Tue, 3 Nov 2026 15:52:29 +0000 Subject: [PATCH 4/8] Handle non-UTF-8 file names in install instructions A file name that isn't valid UTF-8 panicked the command thread, so the Install event never completed (#284). - install.json paths use to_string_lossy(). - The cleanup file stores raw path bytes on Unix and is read back as bytes, so such files and directories are removed on the next deployment. Windows is unchanged (text lines). - A destination containing a line break fails planning, and the cleanup writer refuses one too, so a file name can't add cleanup entries. --- src/installer/cleanup_file.rs | 98 ++++++ src/installer/commands/change_acl_command.rs | 2 +- .../commands/change_context_command.rs | 11 +- src/installer/commands/change_mode_command.rs | 2 +- .../commands/change_owner_command.rs | 2 +- src/installer/commands/copy_command.rs | 32 +- .../commands/make_directory_command.rs | 4 +- src/installer/commands/remove_command.rs | 2 +- src/installer/core.rs | 333 +++++++++++++++--- src/installer/error.rs | 40 +++ src/installer/mod.rs | 1 + 11 files changed, 475 insertions(+), 52 deletions(-) create mode 100644 src/installer/cleanup_file.rs diff --git a/src/installer/cleanup_file.rs b/src/installer/cleanup_file.rs new file mode 100644 index 00000000..a76f742c --- /dev/null +++ b/src/installer/cleanup_file.rs @@ -0,0 +1,98 @@ +//! Cleanup file line encoding. +//! +//! Each line is a path. On Unix the path's raw bytes are written so names that +//! are not valid UTF-8 round-trip exactly. On Windows paths are written as text. + +use std::io::{self, Write}; +use std::path::{Path, PathBuf}; + +/// Prefix of a line that names an `SELinux` context to remove. +pub const SEMANAGE_PREFIX: &[u8] = b"semanage\0"; + +/// Write `prefix` followed by `path` and a newline. +/// +/// # Errors +/// Returns `InvalidInput` if `path` contains a line break, or any error from +/// the underlying writer. +pub fn write_line(out: &mut dyn Write, prefix: &[u8], path: &Path) -> io::Result<()> { + let bytes = path_to_bytes(path); + if bytes.iter().any(|b| matches!(b, b'\n' | b'\r')) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("path contains a line break: {}", path.display().to_string().escape_debug()), + )); + } + let mut line = prefix.to_vec(); + line.extend_from_slice(&bytes); + line.push(b'\n'); + out.write_all(&line) +} + +#[cfg(unix)] +fn path_to_bytes(path: &Path) -> Vec { + use std::os::unix::ffi::OsStrExt; + path.as_os_str().as_bytes().to_vec() +} + +#[cfg(not(unix))] +fn path_to_bytes(path: &Path) -> Vec { + path.display().to_string().into_bytes() +} + +/// Build a path from the bytes of one cleanup line. +#[cfg(unix)] +#[must_use] +pub fn path_from_bytes(bytes: &[u8]) -> PathBuf { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + PathBuf::from(OsStr::from_bytes(bytes)) +} + +/// Build a path from the bytes of one cleanup line. +#[cfg(not(unix))] +#[must_use] +pub fn path_from_bytes(bytes: &[u8]) -> PathBuf { + PathBuf::from(String::from_utf8_lossy(bytes).into_owned()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn write_line_rejects_line_breaks() { + for path in ["/opt/app/a\nb", "/opt/app/a\rb"] { + let mut buf = Vec::new(); + let err = write_line(&mut buf, b"", Path::new(path)).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::InvalidInput); + assert!(buf.is_empty()); + } + } + + #[test] + fn round_trips_plain_path() { + let mut buf = Vec::new(); + write_line(&mut buf, b"", Path::new("/opt/app/a.txt")).unwrap(); + assert_eq!(buf, b"/opt/app/a.txt\n"); + assert_eq!(path_from_bytes(&buf[..buf.len() - 1]), PathBuf::from("/opt/app/a.txt")); + } + + #[test] + fn writes_prefix() { + let mut buf = Vec::new(); + write_line(&mut buf, SEMANAGE_PREFIX, Path::new("/opt/app")).unwrap(); + assert_eq!(buf, b"semanage\0/opt/app\n"); + } + + #[cfg(unix)] + #[test] + fn round_trips_non_utf8_path() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + let path = Path::new(OsStr::from_bytes(b"/opt/caf\xe9.txt")); + let mut buf = Vec::new(); + write_line(&mut buf, b"", path).unwrap(); + assert_eq!(buf, b"/opt/caf\xe9.txt\n"); + assert_eq!(path_from_bytes(&buf[..buf.len() - 1]), path); + } +} diff --git a/src/installer/commands/change_acl_command.rs b/src/installer/commands/change_acl_command.rs index e2d60f96..38a25d3a 100644 --- a/src/installer/commands/change_acl_command.rs +++ b/src/installer/commands/change_acl_command.rs @@ -194,7 +194,7 @@ impl ChangeAclCommand { json!({ "type": "setfacl", "acl": acl_strings, - "file": self.object + "file": self.object.to_string_lossy() }) } } diff --git a/src/installer/commands/change_context_command.rs b/src/installer/commands/change_context_command.rs index dc6b5e73..1e428b7d 100644 --- a/src/installer/commands/change_context_command.rs +++ b/src/installer/commands/change_context_command.rs @@ -89,12 +89,19 @@ impl ChangeContextCommand { } else { self.object.clone() }; + if object.as_os_str().as_encoded_bytes().iter().any(|b| matches!(b, b'\n' | b'\r')) { + return Err(InstallerError::LineBreakInPath { path: object }); + } self.selinux_ops.set_context(&args_vec, &object).map_err(InstallerError::Io)?; self.selinux_ops.restore_context(&object).map_err(InstallerError::Io)?; - writeln!(cleanup_file, "semanage\0{}", object.display())?; + crate::installer::cleanup_file::write_line( + cleanup_file, + crate::installer::cleanup_file::SEMANAGE_PREFIX, + &object, + )?; Ok(()) } @@ -109,7 +116,7 @@ impl ChangeContextCommand { "type": self.context.type_(), "range": self.context.range().map(ToString::to_string) }, - "file": self.object + "file": self.object.to_string_lossy() }) } } diff --git a/src/installer/commands/change_mode_command.rs b/src/installer/commands/change_mode_command.rs index e84ae4aa..29952482 100644 --- a/src/installer/commands/change_mode_command.rs +++ b/src/installer/commands/change_mode_command.rs @@ -77,7 +77,7 @@ impl ChangeModeCommand { json!({ "type": "chmod", "mode": self.mode, - "file": self.object + "file": self.object.to_string_lossy() }) } } diff --git a/src/installer/commands/change_owner_command.rs b/src/installer/commands/change_owner_command.rs index 3114d1cf..daaa303f 100644 --- a/src/installer/commands/change_owner_command.rs +++ b/src/installer/commands/change_owner_command.rs @@ -70,7 +70,7 @@ impl ChangeOwnerCommand { "type": "chown", "owner": self.owner, "group": self.group, - "file": self.object + "file": self.object.to_string_lossy() }) } } diff --git a/src/installer/commands/copy_command.rs b/src/installer/commands/copy_command.rs index 41d52ce4..a77921a6 100644 --- a/src/installer/commands/copy_command.rs +++ b/src/installer/commands/copy_command.rs @@ -30,10 +30,21 @@ impl CopyCommand { /// # Errors /// Returns an error if the command execution fails. pub fn execute(&self, cleanup_file: &mut dyn Write) -> Result<()> { - writeln!(cleanup_file, "{}", self.destination.display())?; - let metadata = fs::symlink_metadata(&self.source)?; + #[cfg(unix)] + if metadata.is_symlink() + && fs::symlink_metadata(&self.destination).is_ok_and(|m| m.is_dir()) + { + return Err(std::io::Error::new( + std::io::ErrorKind::AlreadyExists, + format!("cannot replace directory {} with a symlink", self.destination.display()), + ) + .into()); + } + + crate::installer::cleanup_file::write_line(cleanup_file, b"", &self.destination)?; + if metadata.is_symlink() { let target = fs::read_link(&self.source)?; #[cfg(unix)] @@ -175,8 +186,8 @@ impl CopyCommand { pub fn to_h(&self) -> Value { json!({ "type": "copy", - "source": self.source, - "destination": self.destination + "source": self.source.to_string_lossy(), + "destination": self.destination.to_string_lossy() }) } } @@ -284,6 +295,19 @@ mod tests { assert_eq!(hash["destination"], "/dest/file.txt"); } + #[test] + #[cfg(unix)] + fn to_h_non_utf8_path() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let name = OsStr::from_bytes(b"/dest/caf\xe9.txt"); + let cmd = CopyCommand::new("/source/file.txt".into(), name.into()); + let hash = cmd.to_h(); + + assert_eq!(hash["destination"], "/dest/caf\u{fffd}.txt"); + } + #[test] #[cfg(unix)] fn execute_preserves_timestamps() { diff --git a/src/installer/commands/make_directory_command.rs b/src/installer/commands/make_directory_command.rs index e861b671..0a9fe6cc 100644 --- a/src/installer/commands/make_directory_command.rs +++ b/src/installer/commands/make_directory_command.rs @@ -22,7 +22,7 @@ impl MakeDirectoryCommand { pub fn execute(&self, cleanup_file: &mut dyn Write) -> Result<()> { match fs::create_dir(&self.directory) { Ok(()) => { - writeln!(cleanup_file, "{}", self.directory.display())?; + crate::installer::cleanup_file::write_line(cleanup_file, b"", &self.directory)?; }, Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists && self.directory.is_dir() => { }, @@ -37,7 +37,7 @@ impl MakeDirectoryCommand { pub fn to_h(&self) -> Value { json!({ "type": "mkdir", - "directory": self.directory + "directory": self.directory.to_string_lossy() }) } } diff --git a/src/installer/commands/remove_command.rs b/src/installer/commands/remove_command.rs index 1ba55b6d..c17530fd 100644 --- a/src/installer/commands/remove_command.rs +++ b/src/installer/commands/remove_command.rs @@ -45,7 +45,7 @@ impl RemoveCommand { pub fn to_h(&self) -> Value { json!({ "type": "remove", - "file": self.file_path + "file": self.file_path.to_string_lossy() }) } } diff --git a/src/installer/core.rs b/src/installer/core.rs index f1e240ec..2506a9c1 100644 --- a/src/installer/core.rs +++ b/src/installer/core.rs @@ -1,4 +1,5 @@ //! Installer core — executes install commands and manages cleanup files. +use super::cleanup_file; #[cfg(target_os = "linux")] use super::commands::RemoveContextCommand; use super::{ @@ -14,12 +15,20 @@ use std::fs; use std::path::{Path, PathBuf}; use tracing::{debug, info}; +/// Cleanup entries are newline-delimited, so a destination containing a +/// line break is refused. +fn reject_line_break(path: &Path) -> Result<()> { + if path.as_os_str().as_encoded_bytes().iter().any(|b| matches!(b, b'\n' | b'\r')) { + return Err(InstallerError::LineBreakInPath { path: path.to_path_buf() }); + } + Ok(()) +} + // Instruction-file mode (install.json, cleanup) comes from // `system::agent_file_mode`: 0644 by default, 0600 under // `restrict_agent_dir_permissions`. -// Independent opt-in toggles mirroring their AgentConfig fields; an enum -// would force invalid combinations to be representable. +// One independent toggle per AgentConfig field; every combination is valid. #[allow(clippy::struct_excessive_bools)] #[derive(Debug)] pub struct Installer { @@ -129,8 +138,8 @@ impl Installer { // replaced it — skip cleanup rather than delete injected paths. if Self::cleanup_file_is_trusted(&file, &cleanup_file)? { use std::io::Read; - let mut contents = String::new(); - file.read_to_string(&mut contents)?; + let mut contents = Vec::new(); + file.read_to_end(&mut contents)?; Self::execute_cleanup(&contents)?; } drop(file); @@ -336,6 +345,7 @@ impl Installer { dest: &Path, behavior: FileExistsBehavior, ) -> Result<()> { + reject_line_break(dest)?; if !dest.is_dir() { builder.mkdir(dest)?; } @@ -363,13 +373,25 @@ impl Installer { dest: &Path, behavior: FileExistsBehavior, ) -> Result<()> { - // A symlink source cannot be written through a dangling symlink at - // `dest`, so for it any entry at `dest` counts as existing. + reject_line_break(dest)?; + #[cfg(unix)] let dest_exists = if fs::symlink_metadata(source).is_ok_and(|m| m.is_symlink()) { - fs::symlink_metadata(dest).is_ok() + // A symlink cannot be written through a dangling symlink at + // `dest`, and cannot replace a directory there. + match fs::symlink_metadata(dest) { + Ok(meta) if meta.is_dir() && behavior == FileExistsBehavior::Overwrite => { + return Err(InstallerError::DirectoryReplacedBySymlink { + destination: dest.to_path_buf(), + }); + }, + Ok(_) => true, + Err(_) => false, + } } else { dest.exists() }; + #[cfg(not(unix))] + let dest_exists = dest.exists(); if dest_exists { match behavior { FileExistsBehavior::Disallow => { @@ -497,61 +519,55 @@ impl Installer { /// [`Self::destination_escapes_root`]). Legitimate cleanup entries are the /// destination paths the agent itself wrote — absolute, relative, or Windows /// paths — none of which escape their own root. - fn cleanup_line_is_valid(line: &str) -> bool { - if line.is_empty() || line.contains('\0') { + fn cleanup_line_is_valid(line: &[u8]) -> bool { + if line.is_empty() || line.contains(&0) { return false; } - !Self::destination_escapes_root(Path::new(line)) + !Self::destination_escapes_root(&cleanup_file::path_from_bytes(line)) } - fn execute_cleanup(contents: &str) -> Result<()> { - let lines: Vec<&str> = contents.lines().collect(); - - // Handle incomplete last line: remove if it doesn't end with newline, - // indicating the write was interrupted (partial cleanup file from a crash). - // cleanup file format. - let lines = if !contents.ends_with('\n') && !lines.is_empty() { - &lines[..lines.len() - 1] - } else { - &lines[..] - }; + fn execute_cleanup(contents: &[u8]) -> Result<()> { + let mut lines: Vec<&[u8]> = contents.split(|b| *b == b'\n').collect(); + // The final element is empty when the file ends with a newline, or a + // partial line from an interrupted write when it does not. Drop it either way. + lines.pop(); debug!("Executing {} cleanup commands", lines.len()); for line in lines.iter().rev() { - let line = line.trim(); + let line = line.strip_suffix(b"\r").unwrap_or(line); if line.is_empty() { continue; } - if line.starts_with("semanage\0") { + if let Some(object) = line.strip_prefix(cleanup_file::SEMANAGE_PREFIX) { #[cfg(target_os = "linux")] { - let object = line.strip_prefix("semanage\0").unwrap(); - // Validate the SELinux object path the same way as a removal - // path (the NUL is the sentinel, already stripped here). + let object_path = cleanup_file::path_from_bytes(object); if !Self::cleanup_line_is_valid(object) { tracing::warn!( - object = %object, + object = %object_path.display(), "Skipping malformed SELinux cleanup entry" ); continue; } - debug!("Cleanup: removing SELinux context for {object:?}"); + debug!("Cleanup: removing SELinux context for {object_path:?}"); // Semanage removal failures are silently ignored - let _ = RemoveContextCommand::new(PathBuf::from(object)) - .execute(&mut std::io::sink()); + let _ = RemoveContextCommand::new(object_path).execute(&mut std::io::sink()); } + #[cfg(not(target_os = "linux"))] + let _ = object; } else { + let path = cleanup_file::path_from_bytes(line); // Skip empty/NUL-bearing/root-escaping lines rather than removing // whatever path they name; legitimate entries are the agent's own // destination paths. if !Self::cleanup_line_is_valid(line) { - tracing::warn!(line = %line, "Skipping malformed cleanup entry"); + tracing::warn!(line = %path.display(), "Skipping malformed cleanup entry"); continue; } - debug!("Cleanup: removing {line:?}"); - RemoveCommand::new(PathBuf::from(line)).execute(&mut std::io::sink())?; + debug!("Cleanup: removing {path:?}"); + RemoveCommand::new(path).execute(&mut std::io::sink())?; } } @@ -683,6 +699,141 @@ files: installer.install("test-group", &spec).unwrap(); } + #[test] + #[cfg(unix)] + fn install_non_utf8_file_name() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let archive_dir = TempDir::new().unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + let src_dir = archive_dir.path().join("myapp"); + fs::create_dir_all(&src_dir).unwrap(); + let name = OsStr::from_bytes(b"caf\xe9.txt"); + fs::write(src_dir.join(name), "content").unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Disallow, + ); + + let appspec_yaml = format!( + r" +version: 0.0 +os: linux +files: + - source: myapp + destination: {} +", + dest_dir.path().display() + ); + let spec = AppSpec::parse(&appspec_yaml).unwrap(); + installer.install("test-group", &spec).unwrap(); + assert_eq!(fs::read(dest_dir.path().join(name)).unwrap(), b"content"); + + let cleanup = fs::read(instructions_dir.path().join("test-group-cleanup")).unwrap(); + assert!( + cleanup.windows(name.len()).any(|w| w == name.as_bytes()), + "cleanup file should hold the raw file name" + ); + + // Redeploying under DISALLOW works because cleanup removed the file. + installer.install("test-group", &spec).unwrap(); + assert_eq!(fs::read(dest_dir.path().join(name)).unwrap(), b"content"); + + // A revision without the file removes it. + fs::remove_file(src_dir.join(name)).unwrap(); + fs::write(src_dir.join("other.txt"), "other").unwrap(); + installer.install("test-group", &spec).unwrap(); + assert!(!dest_dir.path().join(name).exists(), "dropped file should be removed"); + assert!(dest_dir.path().join("other.txt").exists()); + } + + #[cfg(unix)] + #[test] + fn install_non_utf8_directory_name_redeploys() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let archive_dir = TempDir::new().unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + let src_dir = archive_dir.path().join("myapp"); + let dir_name = OsStr::from_bytes(b"r\xe9sum\xe9"); + fs::create_dir_all(src_dir.join(dir_name)).unwrap(); + fs::write(src_dir.join(dir_name).join("a.txt"), "a").unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Disallow, + ); + let appspec_yaml = format!( + r" +version: 0.0 +os: linux +files: + - source: myapp + destination: {} +", + dest_dir.path().display() + ); + let spec = AppSpec::parse(&appspec_yaml).unwrap(); + installer.install("test-group", &spec).unwrap(); + installer.install("test-group", &spec).unwrap(); + assert!(dest_dir.path().join(dir_name).join("a.txt").exists()); + + fs::remove_dir_all(src_dir.join(dir_name)).unwrap(); + installer.install("test-group", &spec).unwrap(); + assert!(!dest_dir.path().join(dir_name).exists(), "dropped directory should be removed"); + } + + #[cfg(unix)] + #[test] + fn execute_cleanup_removes_non_utf8_path() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let dir = TempDir::new().unwrap(); + let victim = dir.path().join(OsStr::from_bytes(b"caf\xe9.txt")); + fs::write(&victim, "x").unwrap(); + let mut contents = Vec::new(); + cleanup_file::write_line(&mut contents, b"", &victim).unwrap(); + // A trailing partial line is ignored. + contents.extend_from_slice(b"/partial"); + Installer::execute_cleanup(&contents).unwrap(); + assert!(!victim.exists()); + } + + #[cfg(unix)] + #[test] + fn execute_cleanup_keeps_surrounding_whitespace_in_names() { + let dir = TempDir::new().unwrap(); + let spaced = dir.path().join(" a.txt\t"); + let plain = dir.path().join("a.txt"); + fs::write(&spaced, "x").unwrap(); + fs::write(&plain, "keep").unwrap(); + let mut contents = Vec::new(); + cleanup_file::write_line(&mut contents, b"", &spaced).unwrap(); + Installer::execute_cleanup(&contents).unwrap(); + assert!(!spaced.exists(), "entry should be removed by its exact name"); + assert!(plain.exists(), "a different file must not be removed"); + } + + #[test] + fn execute_cleanup_accepts_crlf_lines() { + let dir = TempDir::new().unwrap(); + let victim = dir.path().join("a.txt"); + fs::write(&victim, "x").unwrap(); + let contents = format!("{}\r\n", victim.display()).into_bytes(); + Installer::execute_cleanup(&contents).unwrap(); + assert!(!victim.exists()); + } + #[test] fn install_with_permissions() { let archive_dir = TempDir::new().unwrap(); @@ -887,6 +1038,57 @@ files: assert_eq!(fs::read_to_string(current.join("app.txt")).unwrap(), "v1"); } + #[cfg(unix)] + #[test] + fn install_refuses_directory_symlink_over_existing_directory_before_copying() { + let archive_dir = archive_with_directory_symlink(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + let current = dest_dir.path().join("current"); + fs::create_dir(¤t).unwrap(); + fs::write(current.join("app.log"), "log").unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ) + .with_preserve_directory_symlinks(true); + let result = installer.install("test-group", &whole_revision_spec(dest_dir.path())); + + assert!( + matches!(result, Err(InstallerError::DirectoryReplacedBySymlink { .. })), + "expected DirectoryReplacedBySymlink, got {result:?}" + ); + assert_eq!(fs::read_dir(dest_dir.path()).unwrap().count(), 1, "nothing else installed"); + assert_eq!(fs::read_to_string(current.join("app.log")).unwrap(), "log"); + } + + #[cfg(unix)] + #[test] + fn install_rejects_line_break_in_file_name() { + use std::os::unix::ffi::OsStrExt; + let archive_dir = TempDir::new().unwrap(); + fs::write(archive_dir.path().join("ok.txt"), "ok").unwrap(); + let name = std::ffi::OsStr::from_bytes(b"x\nevil.txt"); + fs::write(archive_dir.path().join(name), "evil").unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ); + let result = installer.install("test-group", &whole_revision_spec(dest_dir.path())); + + assert!( + matches!(result, Err(InstallerError::LineBreakInPath { .. })), + "expected LineBreakInPath, got {result:?}" + ); + assert_eq!(fs::read_dir(dest_dir.path()).unwrap().count(), 0, "nothing installed"); + } + #[cfg(unix)] #[test] fn install_preserves_directory_symlink_pointing_outside_revision() { @@ -1017,7 +1219,7 @@ files: #[cfg(unix)] #[test] - fn regular_file_writes_through_dangling_symlink() { + fn regular_file_replaces_dangling_symlink() { let archive_dir = TempDir::new().unwrap(); fs::write(archive_dir.path().join("app.txt"), "new").unwrap(); let instructions_dir = TempDir::new().unwrap(); @@ -1036,7 +1238,55 @@ files: .unwrap(); installer.install("test-group", &spec).unwrap(); - assert_eq!(fs::read_to_string(dest_dir.path().join("app.txt")).unwrap(), "new"); + let dest = dest_dir.path().join("app.txt"); + assert!(!fs::symlink_metadata(&dest).unwrap().is_symlink()); + assert_eq!(fs::read_to_string(&dest).unwrap(), "new"); + } + + #[cfg(unix)] + #[test] + fn file_symlink_over_directory_fails_before_copying_without_flag() { + let archive_dir = TempDir::new().unwrap(); + fs::write(archive_dir.path().join("a.txt"), "a").unwrap(); + std::os::unix::fs::symlink("a.txt", archive_dir.path().join("link")).unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + fs::create_dir(dest_dir.path().join("link")).unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ); + let result = installer.install("test-group", &whole_revision_spec(dest_dir.path())); + + assert!( + matches!(result, Err(InstallerError::DirectoryReplacedBySymlink { .. })), + "expected DirectoryReplacedBySymlink, got {result:?}" + ); + assert!(!dest_dir.path().join("a.txt").exists(), "nothing copied"); + } + + #[cfg(unix)] + #[test] + fn install_rejects_carriage_return_in_directory_name() { + use std::os::unix::ffi::OsStrExt; + let archive_dir = TempDir::new().unwrap(); + let dir = archive_dir.path().join(std::ffi::OsStr::from_bytes(b"d\rx")); + fs::create_dir(&dir).unwrap(); + fs::write(dir.join("a.txt"), "a").unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ); + let result = installer.install("test-group", &whole_revision_spec(dest_dir.path())); + + assert!(matches!(result, Err(InstallerError::LineBreakInPath { .. })), "got {result:?}"); + assert_eq!(fs::read_dir(dest_dir.path()).unwrap().count(), 0); } #[cfg(unix)] @@ -2082,13 +2332,16 @@ permissions: "relative/dir", r"c:\inetpub\app", ] { - assert!(Installer::cleanup_line_is_valid(ok), "{ok} should be a valid cleanup line"); + assert!( + Installer::cleanup_line_is_valid(ok.as_bytes()), + "{ok} should be a valid cleanup line" + ); } // Malformed / escaping entries are rejected. - assert!(!Installer::cleanup_line_is_valid(""), "empty line invalid"); - assert!(!Installer::cleanup_line_is_valid("/a\0/b"), "embedded NUL invalid"); + assert!(!Installer::cleanup_line_is_valid(b""), "empty line invalid"); + assert!(!Installer::cleanup_line_is_valid(b"/a\0/b"), "embedded NUL invalid"); assert!( - !Installer::cleanup_line_is_valid("../../etc/cron.d/x"), + !Installer::cleanup_line_is_valid(b"../../etc/cron.d/x"), "root-escaping line invalid" ); } @@ -2107,7 +2360,7 @@ permissions: // Build cleanup contents: the escaping line uses `..` to climb out. let contents = format!("{}\n../../{}\n", victim.display(), "should_not_matter"); - Installer::execute_cleanup(&contents).unwrap(); + Installer::execute_cleanup(contents.as_bytes()).unwrap(); assert!(!victim.exists(), "valid cleanup entry should be removed"); assert!(outside.exists(), "escaping entry must be skipped, unrelated file intact"); diff --git a/src/installer/error.rs b/src/installer/error.rs index 43dbb1d1..c3b1d34f 100644 --- a/src/installer/error.rs +++ b/src/installer/error.rs @@ -42,6 +42,12 @@ pub enum InstallerError { InvalidSourceFileName { source: String, }, + LineBreakInPath { + path: PathBuf, + }, + DirectoryReplacedBySymlink { + destination: PathBuf, + }, // Permission errors SymlinkDestinationRejected { @@ -160,6 +166,22 @@ impl fmt::Display for InstallerError { ) }, + Self::LineBreakInPath { path } => { + write!( + f, + "The deployment failed because a path ({}) contains a line break. Rename the file or directory in the application revision, correct the AppSpec file, or check the instance for symbolic links that resolve to this path, and then try again.", + path.display().to_string().escape_debug() + ) + }, + + Self::DirectoryReplacedBySymlink { destination } => { + write!( + f, + "The deployment failed because the application revision contains a symbolic link, but a directory already exists at its destination ({}). Remove the directory from the instance or update the files section of the AppSpec file, and then try again.", + destination.display() + ) + }, + Self::InvalidFileExistsBehavior { behavior } => { write!( f, @@ -342,6 +364,24 @@ mod tests { assert!(msg.contains("no final path component")); } + #[test] + fn line_break_in_path_display() { + let err = InstallerError::LineBreakInPath { path: PathBuf::from("/dest/a\nb") }; + let msg = err.to_string(); + assert!(msg.contains("contains a line break")); + assert!(!msg.contains('\n'), "line break must be escaped: {msg}"); + } + + #[test] + fn directory_replaced_by_symlink_display() { + let err = InstallerError::DirectoryReplacedBySymlink { + destination: PathBuf::from("/dest/current"), + }; + let msg = err.to_string(); + assert!(msg.contains("/dest/current")); + assert!(msg.contains("symbolic link")); + } + #[test] fn selinux_role_not_supported_display() { let err = InstallerError::SelinuxRoleNotSupported; diff --git a/src/installer/mod.rs b/src/installer/mod.rs index e1fc7a70..6c704803 100644 --- a/src/installer/mod.rs +++ b/src/installer/mod.rs @@ -5,6 +5,7 @@ //! rollback on the next deployment. pub mod builder; +pub mod cleanup_file; pub mod commands; pub mod core; pub mod error; From 389ede29beffa799147c0dce4bcf328f3d6e0e63 Mon Sep 17 00:00:00 2001 From: Nathan Inge Date: Tue, 3 Nov 2026 15:52:46 +0000 Subject: [PATCH 5/8] Skip non-UTF-8 fixture tests on macOS install_non_utf8_file_name, install_non_utf8_directory_name_redeploys and execute_cleanup_removes_non_utf8_path create a file or directory whose name is not valid UTF-8. APFS rejects such names with EILSEQ ("Illegal byte sequence", os error 92), so on macOS the tests fail in setup, before any installer code runs. Run the three tests on Unix other than macOS. No revision on macOS can hold such a name, so the case they cover doesn't arise there. The in-memory non-UTF-8 tests create no files and still run on macOS. --- src/installer/core.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/installer/core.rs b/src/installer/core.rs index 2506a9c1..fa0e0b59 100644 --- a/src/installer/core.rs +++ b/src/installer/core.rs @@ -699,8 +699,10 @@ files: installer.install("test-group", &spec).unwrap(); } + // Not on macOS: APFS rejects names that aren't valid UTF-8 (EILSEQ), so + // the fixture can't be created there, and no revision can hold one. #[test] - #[cfg(unix)] + #[cfg(all(unix, not(target_os = "macos")))] fn install_non_utf8_file_name() { use std::ffi::OsStr; use std::os::unix::ffi::OsStrExt; @@ -752,7 +754,9 @@ files: assert!(dest_dir.path().join("other.txt").exists()); } - #[cfg(unix)] + // Not on macOS: APFS rejects names that aren't valid UTF-8 (EILSEQ), so + // the fixture can't be created there, and no revision can hold one. + #[cfg(all(unix, not(target_os = "macos")))] #[test] fn install_non_utf8_directory_name_redeploys() { use std::ffi::OsStr; @@ -792,7 +796,9 @@ files: assert!(!dest_dir.path().join(dir_name).exists(), "dropped directory should be removed"); } - #[cfg(unix)] + // Not on macOS: APFS rejects names that aren't valid UTF-8 (EILSEQ), so + // the fixture can't be created there, and no revision can hold one. + #[cfg(all(unix, not(target_os = "macos")))] #[test] fn execute_cleanup_removes_non_utf8_path() { use std::ffi::OsStr; From 90b124188f755fbd3353c50532c045a68582326f Mon Sep 17 00:00:00 2001 From: Nathan Inge Date: Tue, 3 Nov 2026 15:53:11 +0000 Subject: [PATCH 6/8] Keep agent deployment dirs readable by Users on Windows The install root now carries an explicit Administrators/SYSTEM ACL that its subfolders inherit, so agent dirs created under the default restrict_agent_dir_permissions: false no longer inherit read access for BUILTIN\Users from %ProgramData%. - create_dir_world_readable on Windows grants BUILTIN\Users ReadAndExecute on the dir, inherited by files and subdirs, keeping the dir's other ACEs and its protection state. This is the Windows counterpart of the 0755 dirs on Linux and macOS. Users never get write. It's a no-op when Users can already read the dir, so re-touching a dir doesn't re-propagate ACLs through its subtree on every deployment. - New create_root_dir, used by ensure_dirs for root_dir. On Windows root_dir is the install root, which also holds bin\, conf.yml and conf.onpremises.yml, so its installer ACL is left as is. Elsewhere, and with the flag set, it's create_deployment_dir. - restrict_agent_dir_permissions: true is unchanged. --- src/config/mod.rs | 7 +- src/system/mod.rs | 2 +- src/system/secure_files.rs | 281 ++++++++++++++++++++++++++++++++++++- 3 files changed, 283 insertions(+), 7 deletions(-) diff --git a/src/config/mod.rs b/src/config/mod.rs index fb88cc2c..605eeee5 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -612,7 +612,12 @@ impl AgentConfig { /// Returns an error if PID or state directories cannot be created. pub fn ensure_dirs(&self) -> std::io::Result<()> { self.ensure_deployment_dir(&self.pid_dir, 0o700)?; - self.ensure_deployment_dir(&self.root_dir, 0o711)?; + // root_dir is the install root on Windows; see create_root_dir. + crate::system::create_root_dir( + &self.root_dir, + 0o711, + self.hardening.restrict_agent_dir_permissions, + )?; self.ensure_deployment_dir(&self.root_dir.join(&self.ongoing_deployment_tracking), 0o700)?; // Best-effort: log dir may be under /var/log which requires root. // Policy per `restrict_log_dir_permissions` — 0755 default so diff --git a/src/system/mod.rs b/src/system/mod.rs index 3495e969..a7bc0b3c 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -22,7 +22,7 @@ pub use linux_ops::{LinuxOps, SystemLinuxOps}; pub use process_ops::kill_process_group; pub use secure_files::{ agent_file_mode, create_deployment_dir, create_dir_secure, create_dir_world_readable, - create_file_secure, write_file_secure, + create_file_secure, create_root_dir, write_file_secure, }; #[cfg(target_os = "linux")] pub use selinux_ops::{SeLinuxOps, SystemSeLinuxOps}; diff --git a/src/system/secure_files.rs b/src/system/secure_files.rs index a676eb44..6fe9c762 100644 --- a/src/system/secure_files.rs +++ b/src/system/secure_files.rs @@ -84,6 +84,139 @@ fn apply_agent_dacl(path: &Path) -> io::Result<()> { Ok(()) } +/// Grant `BUILTIN\Users` read and execute on a directory, inherited by the +/// files and subdirectories under it, keeping the directory's other ACEs. +/// Never grants write. A no-op when Users can already read the directory +/// (for example through an ACE inherited from an agent dir above it), so +/// re-touching a dir doesn't re-propagate ACLs through its whole subtree. +/// +/// This is the Windows counterpart of the Unix 0755 parity mode: the MSI +/// locks the install root to SYSTEM + Administrators, so agent dirs no +/// longer inherit the `%ProgramData%` Users read ACE and need it explicitly. +#[cfg(windows)] +#[allow(unsafe_code)] +fn grant_users_read(path: &Path) -> io::Result<()> { + use std::os::windows::ffi::OsStrExt; + use windows_sys::Win32::Foundation::{ERROR_SUCCESS, LocalFree}; + use windows_sys::Win32::Security::Authorization::{ + EXPLICIT_ACCESS_W, GRANT_ACCESS, GetEffectiveRightsFromAclW, GetNamedSecurityInfoW, + NO_MULTIPLE_TRUSTEE, SE_FILE_OBJECT, SetEntriesInAclW, SetNamedSecurityInfoW, + TRUSTEE_IS_SID, TRUSTEE_IS_WELL_KNOWN_GROUP, TRUSTEE_W, + }; + use windows_sys::Win32::Security::{ + ACL, CreateWellKnownSid, DACL_SECURITY_INFORMATION, SUB_CONTAINERS_AND_OBJECTS_INHERIT, + WinBuiltinUsersSid, + }; + use windows_sys::Win32::Storage::FileSystem::{FILE_GENERIC_EXECUTE, FILE_GENERIC_READ}; + + const SECURITY_MAX_SID_SIZE: u32 = 68; + let read_execute = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE; + + let mut sid = [0u8; SECURITY_MAX_SID_SIZE as usize]; + let mut sid_len = SECURITY_MAX_SID_SIZE; + // SAFETY: `sid` is a writable buffer of `sid_len` bytes; a null domain + // SID is valid for a BUILTIN well-known SID. + if unsafe { + CreateWellKnownSid( + WinBuiltinUsersSid, + std::ptr::null_mut(), + sid.as_mut_ptr().cast(), + &mut sid_len, + ) + } == 0 + { + return Err(io::Error::last_os_error()); + } + let trustee = TRUSTEE_W { + pMultipleTrustee: std::ptr::null_mut(), + MultipleTrusteeOperation: NO_MULTIPLE_TRUSTEE, + TrusteeForm: TRUSTEE_IS_SID, + TrusteeType: TRUSTEE_IS_WELL_KNOWN_GROUP, + ptstrName: sid.as_mut_ptr().cast(), + }; + + let wide: Vec = path.as_os_str().encode_wide().chain(std::iter::once(0)).collect(); + let mut dacl: *mut ACL = std::ptr::null_mut(); + let mut sd: *mut core::ffi::c_void = std::ptr::null_mut(); + // SAFETY: `wide` is NUL-terminated; the out-pointers are valid. `dacl` + // points into `sd`, which is freed below with `LocalFree`. + let ret = unsafe { + GetNamedSecurityInfoW( + wide.as_ptr(), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + std::ptr::null_mut(), + std::ptr::null_mut(), + &mut dacl, + std::ptr::null_mut(), + &mut sd, + ) + }; + if ret != ERROR_SUCCESS { + return Err(win32_error(ret)); + } + + let mut current: u32 = 0; + // SAFETY: `dacl` comes from `GetNamedSecurityInfoW` and `trustee` holds a + // valid SID that outlives the call. + let have = unsafe { GetEffectiveRightsFromAclW(dacl, &trustee, &mut current) }; + let result = if have == ERROR_SUCCESS && current & read_execute == read_execute { + Ok(()) + } else { + let entry = EXPLICIT_ACCESS_W { + grfAccessPermissions: read_execute, + grfAccessMode: GRANT_ACCESS, + grfInheritance: SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: trustee, + }; + let mut new_dacl: *mut ACL = std::ptr::null_mut(); + // SAFETY: `entry` is a valid EXPLICIT_ACCESS_W; `dacl` is the + // existing DACL; `new_dacl` is a valid out-pointer. + let ret = unsafe { SetEntriesInAclW(1, &entry, dacl, &mut new_dacl) }; + if ret == ERROR_SUCCESS { + // DACL_SECURITY_INFORMATION alone keeps the dir's protection + // state: a protected DACL stays protected, an inheriting one + // keeps inheriting. + // SAFETY: `wide` is NUL-terminated; `new_dacl` is a valid ACL from + // `SetEntriesInAclW`. + let ret = unsafe { + SetNamedSecurityInfoW( + wide.as_ptr(), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + std::ptr::null_mut(), + std::ptr::null_mut(), + new_dacl, + std::ptr::null(), + ) + }; + // SAFETY: `new_dacl` was allocated by `SetEntriesInAclW`. + unsafe { LocalFree(new_dacl.cast()) }; + if ret == ERROR_SUCCESS { + Ok(()) + } else { + Err(win32_error(ret)) + } + } else { + Err(win32_error(ret)) + } + }; + + // SAFETY: `sd` was allocated by `GetNamedSecurityInfoW` and is freed once. + unsafe { LocalFree(sd) }; + result +} + +/// Convert a `WIN32_ERROR` return code (the security APIs return it rather +/// than setting last-error) into an `io::Error`. +#[cfg(windows)] +fn win32_error(code: u32) -> io::Error { + i32::try_from(code).map_or_else( + |_| io::Error::other(format!("Win32 error {code}")), + io::Error::from_raw_os_error, + ) +} + /// Create a file with the agent DACL applied atomically via /// `CreateFileW` + `SECURITY_ATTRIBUTES`. Returns the open handle wrapped /// in a `std::fs::File`. @@ -266,9 +399,11 @@ pub fn agent_file_mode(restrict: bool) -> u32 { /// force-loosen matters on upgrade: a host that already ran a tightened agent /// has these dirs at 0700/0711, and [`create_dir_secure`] only ever tightens. /// -/// On Windows, creates the directory with the default inherited ACL (no -/// protected DACL), so the same host tooling keeps its access. A pre-existing -/// protected DACL from an earlier agent version is not reset. +/// On Windows, creates the directory with its inherited ACL and grants +/// `BUILTIN\Users` read and execute, inherited by everything under it (see +/// [`grant_users_read`]). Users never get write. A pre-existing protected +/// DACL from an earlier agent version stays protected but gains the Users +/// read ACE. /// /// # Errors /// Returns an error if directory creation or permission setting fails. @@ -300,9 +435,14 @@ pub fn create_dir_world_readable(path: &Path) -> io::Result<()> { } Ok(()) } - #[cfg(not(unix))] + #[cfg(windows)] + { + std::fs::create_dir_all(path)?; + grant_users_read(path) + } + #[cfg(not(any(unix, windows)))] { - std::fs::create_dir_all(path) + compile_error!("secure_files requires Unix or Windows"); } } @@ -340,6 +480,24 @@ pub fn create_deployment_dir(path: &Path, hardened_mode: u32, restrict: bool) -> } } +/// Create the agent `root_dir` with the policy selected by +/// `restrict_agent_dir_permissions`. Same as [`create_deployment_dir`], +/// except on Windows under the parity default: there `root_dir` is the MSI +/// install root (`%PROGRAMDATA%\Amazon\CodeDeploy`), which also holds `bin\`, +/// `conf.yml` and `conf.onpremises.yml`, so its installer ACL (SYSTEM + +/// Administrators) is left as is. The agent dirs under it get Users read +/// from [`create_dir_world_readable`]. +/// +/// # Errors +/// Returns an error if directory creation or permission setting fails. +pub fn create_root_dir(path: &Path, hardened_mode: u32, restrict: bool) -> io::Result<()> { + #[cfg(windows)] + if !restrict { + return std::fs::create_dir_all(path); + } + create_deployment_dir(path, hardened_mode, restrict) +} + /// Write `content` to `path` atomically, with the file created at exactly /// `mode` regardless of process umask. /// @@ -856,4 +1014,117 @@ mod tests { assert_eq!(std::fs::read(&path).unwrap(), b"new"); assert_agent_dacl(&path); } + + /// SID of `BUILTIN\Users`. + const USERS_SID: &str = "S-1-5-32-545"; + /// `FILE_GENERIC_READ | FILE_GENERIC_EXECUTE` (`ReadAndExecute`). + const READ_EXECUTE: u32 = 0x0012_00A9; + /// `FILE_WRITE_DATA`, `FILE_APPEND_DATA`, `FILE_WRITE_EA`, + /// `FILE_WRITE_ATTRIBUTES`, `DELETE`, `WRITE_DAC` and `WRITE_OWNER`. + const ANY_WRITE: u32 = + 0x0002 | 0x0004 | 0x0010 | 0x0100 | 0x0001_0000 | 0x0004_0000 | 0x0008_0000; + /// `OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE`. + const OI_CI: u8 = 0x01 | 0x02; + + /// Allow ACEs for `BUILTIN\Users` on `path`: `(mask, flags)` pairs. + fn users_allow_aces(path: &Path) -> Vec<(u32, u8)> { + use windows_acl::acl::{ACL, AceType}; + + let acl = ACL::from_file_path(path.to_str().unwrap(), false).unwrap(); + acl.all() + .unwrap() + .iter() + .filter(|e| e.entry_type == AceType::AccessAllow && e.string_sid == USERS_SID) + .map(|e| (e.mask, e.flags)) + .collect() + } + + #[test] + fn create_dir_world_readable_grants_users_read_inherited_never_write() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("deployment-logs"); + create_dir_world_readable(&dir).unwrap(); + + let aces = users_allow_aces(&dir); + assert!( + aces.iter() + .any(|&(m, f)| m & READ_EXECUTE == READ_EXECUTE && f & OI_CI == OI_CI), + "expected an inheritable Users ReadAndExecute ACE, got {aces:?}" + ); + assert!( + aces.iter().all(|&(m, _)| m & ANY_WRITE == 0), + "Users must not get write: {aces:?}" + ); + + // Files and subdirectories created inside inherit the read ACE. + let file = dir.join("codedeploy-agent-deployments.log"); + std::fs::write(&file, b"x").unwrap(); + let sub = dir.join("d-123"); + std::fs::create_dir(&sub).unwrap(); + for p in [&file, &sub] { + let aces = users_allow_aces(p); + assert!( + aces.iter().any(|&(m, _)| m & READ_EXECUTE == READ_EXECUTE), + "{} should inherit Users read, got {aces:?}", + p.display() + ); + assert!(aces.iter().all(|&(m, _)| m & ANY_WRITE == 0)); + } + } + + #[test] + fn create_dir_world_readable_is_idempotent() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("ongoing-deployment"); + create_dir_world_readable(&dir).unwrap(); + let first = users_allow_aces(&dir); + create_dir_world_readable(&dir).unwrap(); + assert_eq!(users_allow_aces(&dir), first, "re-touching must not add ACEs"); + } + + #[test] + fn create_dir_world_readable_adds_read_to_protected_dir() { + // A dir a hardened agent left protected gains Users read and stays + // protected (no inherited ACEs come back). + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("state"); + create_dir_secure(&dir, 0o700).unwrap(); + create_dir_world_readable(&dir).unwrap(); + + let aces = users_allow_aces(&dir); + assert!(aces.iter().any(|&(m, _)| m & READ_EXECUTE == READ_EXECUTE), "got {aces:?}"); + assert!(aces.iter().all(|&(m, _)| m & ANY_WRITE == 0)); + } + + #[test] + fn create_root_dir_parity_adds_no_users_ace() { + use windows_acl::acl::{ACL, AceType}; + + // root_dir is the MSI install root on Windows: its ACL is the + // installer's, so the parity default must not add an explicit ACE. + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("CodeDeploy"); + create_root_dir(&root, 0o711, false).unwrap(); + + let acl = ACL::from_file_path(root.to_str().unwrap(), false).unwrap(); + let explicit_users = acl + .all() + .unwrap() + .iter() + .filter(|e| { + e.entry_type == AceType::AccessAllow + && e.string_sid == USERS_SID + && e.flags & INHERITED_ACE == 0 + }) + .count(); + assert_eq!(explicit_users, 0, "create_root_dir must leave the root ACL alone"); + } + + #[test] + fn create_root_dir_restrict_applies_agent_dacl() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("CodeDeploy"); + create_root_dir(&root, 0o711, true).unwrap(); + assert_agent_dacl(&root); + } } From 5ad34282af5a255a0d544b60aa5c6ab065280788 Mon Sep 17 00:00:00 2001 From: Nathan Inge Date: Tue, 3 Nov 2026 15:53:24 +0000 Subject: [PATCH 7/8] Bump agent version to 2.1.3 --- Cargo.toml | 2 +- crates/codedeploy-commands/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index ab6bb07b..ffc6c2cb 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.1.2" +version = "2.1.3" 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/crates/codedeploy-commands/Cargo.toml b/crates/codedeploy-commands/Cargo.toml index c67f3241..2bb97018 100644 --- a/crates/codedeploy-commands/Cargo.toml +++ b/crates/codedeploy-commands/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codedeploy-commands" -version = "2.1.2" +version = "2.1.3" edition = "2024" rust-version = "1.91" publish = false From 4028210a1ee64e544b637643893a8de3c9aac655 Mon Sep 17 00:00:00 2001 From: Nathan Inge Date: Wed, 4 Nov 2026 09:34:17 +0000 Subject: [PATCH 8/8] Add the 2.1.3 CHANGELOG entry --- CHANGELOG.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 108edd94..b6ce6626 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,22 @@ +## 2.1.3 + +### Added + +* **Optional `preserve_directory_symlinks`** (default `false`, Linux and macOS). A symlink to a directory in the revision is installed as a symlink instead of copying the directory it points to ([#152](https://github.com/aws/aws-codedeploy-agent/issues/152)). The AppSpec `permissions` section is not applied through a preserved link; a warning is logged instead. + +### Fixed + +* **Lifecycle event results are retried on transient errors.** When reporting a result fails with a 5xx or network error, the agent retries with exponential backoff for up to 65 minutes after the command was acknowledged. Previously only throttling was retried, so a hook that succeeded could fail with "CodeDeploy agent was not able to notify CodeDeploy server the execution result of this lifecycle event". Stopping the agent during retries can take as long as the retry period, bounded by `kill_agent_max_wait_time_seconds`. +* **Symlinks replace existing files and symlinks ([#143](https://github.com/aws/aws-codedeploy-agent/issues/143)).** Installing a symlink over an existing file or symlink now replaces it. A symlink over an existing directory is refused before any file is copied, and cleanup removes dangling symlinks. +* **File names that are not valid UTF-8 ([#284](https://github.com/aws/aws-codedeploy-agent/issues/284)).** Such files no longer hang the Install event; they are installed and removed on the next deployment when a revision drops them. A destination path that contains a line break fails the deployment before any files are copied. +* **Concurrent deployments that create the same missing parent directory ([#429](https://github.com/aws/aws-codedeploy-agent/pull/429))** no longer fail the Install event. +* **GitHub deployments that use an access token ([#434](https://github.com/aws/aws-codedeploy-agent/issues/434))** no longer fail with HTTP 401. The token is now redacted only in the debug log. +* **On Windows, agent deployment folders stay readable by standard users** (`deployment-logs`, `ongoing-deployment`, and the per-deployment folders), matching the 0755 folders on Linux and macOS. Standard users cannot modify them. + +### Security + +* Improved file system permissions on the agent installation folder on Windows Server. + ## 2.1.2 ### Added