From f1a8d8b2b32faaf6f6fa751b89059b335a3d9364 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Tue, 13 Oct 2026 08:33:12 +0000 Subject: [PATCH 01/13] Retry exec on ETXTBSY in the PATH isolation test When tests run in parallel, a child forked by another test thread can briefly inherit the just-written script's write descriptor, so exec fails with "Text file busy". Retry a few times with a short sleep. --- tests/security/intake/process_isolation.rs | 35 ++++++++++++++++------ 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/tests/security/intake/process_isolation.rs b/tests/security/intake/process_isolation.rs index aec17b2..f4d8e71 100644 --- a/tests/security/intake/process_isolation.rs +++ b/tests/security/intake/process_isolation.rs @@ -358,16 +358,33 @@ fn script_path_does_not_contain_dangerous_directories() { let log = Arc::new(Mutex::new(ScriptRunLog::open(&dir.path().join("log")).expect("create log"))); - let script = Script::with_env_policy( - script_path, - None, - false, - &HashMap::::new(), - HARDENED, - log, - ); + let make_script = || { + Script::with_env_policy( + script_path.clone(), + None, + false, + &HashMap::::new(), + HARDENED, + log.clone(), + ) + }; - let exit_code = script.execute(Duration::from_secs(5)).expect("execute script"); + // Retry exec on ETXTBSY: when tests run in parallel, a child forked by + // another test thread can briefly inherit this just-written script's + // write descriptor, making exec fail with "Text file busy". The window + // is microseconds wide; a bounded retry removes the flake without + // masking real failures. + let mut attempts = 0; + let exit_code = loop { + match make_script().execute(Duration::from_secs(5)) { + Ok(code) => break code, + Err(e) if attempts < 5 && e.contains("Text file busy") => { + attempts += 1; + std::thread::sleep(Duration::from_millis(50)); + }, + Err(e) => panic!("execute script: {e:?}"), + } + }; assert_eq!(exit_code, 0); let path_value = std::fs::read_to_string(&output_file) From 29af9e31784e7836b889a5034742afbfb1f2f0f0 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Tue, 13 Oct 2026 08:33:45 +0000 Subject: [PATCH 02/13] Map EUSC and ISO partition regions to their DNS suffixes The command endpoint was built with amazonaws.com for every region outside the China partition, so hosts in the European Sovereign Cloud and the ISO partitions resolved an endpoint that does not exist. Map each partition's region prefix to its DNS suffix: - eusc-* (aws-eusc) -> amazonaws.eu - us-iso-* (aws-iso) -> c2s.ic.gov - us-isob-* (aws-iso-b) -> sc2s.sgov.gov - eu-isoe-* (aws-iso-e) -> cloud.adc-e.uk - us-isof-* (aws-iso-f) -> csp.hci.ic.gov Bump codedeploy-commands to 2.1.1. --- crates/codedeploy-commands/Cargo.toml | 2 +- crates/codedeploy-commands/src/endpoint.rs | 103 ++++++++++++++++++++- 2 files changed, 100 insertions(+), 5 deletions(-) diff --git a/crates/codedeploy-commands/Cargo.toml b/crates/codedeploy-commands/Cargo.toml index 72923d5..1453066 100644 --- a/crates/codedeploy-commands/Cargo.toml +++ b/crates/codedeploy-commands/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codedeploy-commands" -version = "2.1.0" +version = "2.1.1" edition = "2024" rust-version = "1.91" publish = false diff --git a/crates/codedeploy-commands/src/endpoint.rs b/crates/codedeploy-commands/src/endpoint.rs index 379d77a..1844054 100644 --- a/crates/codedeploy-commands/src/endpoint.rs +++ b/crates/codedeploy-commands/src/endpoint.rs @@ -24,13 +24,44 @@ pub fn resolve( service.push_str("-fips"); } - let domain = if region.starts_with("cn-") { + format!("https://{service}.{region}.{}", partition_dns_suffix(region)) +} + +/// Map a region to its partition's DNS suffix. +/// +/// The default AWS partition uses `amazonaws.com`; other partitions have +/// distinct suffixes and are matched by their region-name prefix: +/// - `cn-*` (China) -> `amazonaws.com.cn` +/// - `eusc-*` (European Sovereign Cloud) -> `amazonaws.eu` +/// - `us-iso-*` (ISO) -> `c2s.ic.gov` +/// - `us-isob-*` (ISO-B) -> `sc2s.sgov.gov` +/// - `eu-isoe-*` (ISO-E) -> `cloud.adc-e.uk` +/// - `us-isof-*` (ISO-F) -> `csp.hci.ic.gov` +/// +/// `GovCloud` (`us-gov-*`) intentionally stays on `amazonaws.com`. +/// +/// NOTE: this prefix table is a stopgap. The legacy Ruby agent read the domain +/// directly from IMDS (`/latest/meta-data/services/domain`), which resolves +/// every partition without hardcoding. Once IMDS domain resolution lands, the +/// non-commercial entries here should be removed. The ISO prefixes are +/// mutually disjoint (`us-iso-`, `us-isob-`, `us-isof-` never overlap), so +/// match order is not significant. +fn partition_dns_suffix(region: &str) -> &'static str { + if region.starts_with("cn-") { "amazonaws.com.cn" + } else if region.starts_with("eusc-") { + "amazonaws.eu" + } else if region.starts_with("us-iso-") { + "c2s.ic.gov" + } else if region.starts_with("us-isob-") { + "sc2s.sgov.gov" + } else if region.starts_with("eu-isoe-") { + "cloud.adc-e.uk" + } else if region.starts_with("us-isof-") { + "csp.hci.ic.gov" } else { "amazonaws.com" - }; - - format!("https://{service}.{region}.{domain}") + } } #[cfg(test)] @@ -77,6 +108,70 @@ mod tests { ); } + #[test] + fn eusc_region_endpoint() { + assert_eq!( + resolve("eusc-de-east-1", None, false, false), + "https://codedeploy-commands.eusc-de-east-1.amazonaws.eu" + ); + } + + #[test] + fn eusc_region_secure_endpoint() { + assert_eq!( + resolve("eusc-de-east-1", None, false, true), + "https://codedeploy-commands-secure.eusc-de-east-1.amazonaws.eu" + ); + } + + #[test] + fn gov_cloud_stays_on_default_domain() { + assert_eq!( + resolve("us-gov-west-1", None, false, true), + "https://codedeploy-commands-secure.us-gov-west-1.amazonaws.com" + ); + } + + #[test] + fn iso_region_endpoint() { + assert_eq!( + resolve("us-iso-east-1", None, false, false), + "https://codedeploy-commands.us-iso-east-1.c2s.ic.gov" + ); + } + + #[test] + fn iso_b_region_endpoint() { + assert_eq!( + resolve("us-isob-east-1", None, false, false), + "https://codedeploy-commands.us-isob-east-1.sc2s.sgov.gov" + ); + } + + #[test] + fn iso_e_region_endpoint() { + assert_eq!( + resolve("eu-isoe-west-1", None, false, false), + "https://codedeploy-commands.eu-isoe-west-1.cloud.adc-e.uk" + ); + } + + #[test] + fn iso_f_region_endpoint() { + assert_eq!( + resolve("us-isof-south-1", None, false, false), + "https://codedeploy-commands.us-isof-south-1.csp.hci.ic.gov" + ); + } + + #[test] + fn iso_region_secure_endpoint() { + assert_eq!( + resolve("us-iso-east-1", None, false, true), + "https://codedeploy-commands-secure.us-iso-east-1.c2s.ic.gov" + ); + } + #[test] fn custom_override_bypasses_construction() { assert_eq!( From e975efb641de7c63f447d9efebbeefa8a9e4a230 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Tue, 13 Oct 2026 08:36:19 +0000 Subject: [PATCH 03/13] Add macOS support Build, package and run the agent on macOS 15 and later (EC2 Mac, aarch64 and x86_64): - Gate the ACL (setfacl) and SELinux (semanage/restorecon) installer commands on Linux. On other Unix hosts, AppSpec permissions that set `acls` or `context` are rejected before any install command runs; `owner`, `group` and `mode` still apply. - Strip the dyld loader variables (DYLD_INSERT_LIBRARIES and friends) on macOS where LD_* is stripped on Linux, since dyld ignores LD_*. - Refuse self-update on macOS before any network use: the install script has no pkg package type yet. - Add a launchd LaunchDaemon that mirrors the systemd unit, and .pkg build, install and uninstall scripts under macos/pkg. - Gate the ACL and SELinux security tests on Linux. - Document supported platforms in README.md and the macOS build in DEVELOPMENT.md. --- .github/workflows/ci.yml | 4 +- DEVELOPMENT.md | 31 +++ README.md | 28 +++ conf/codedeployagent.yml | 2 + launchd/com.amazon.aws.codedeploy-agent.plist | 101 +++++++++ macos/pkg/Distribution.xml.in | 24 +++ macos/pkg/build-pkg.sh | 90 ++++++++ macos/pkg/scripts/postinstall | 47 +++++ macos/pkg/scripts/preinstall | 38 ++++ macos/pkg/uninstall.sh | 40 ++++ src/application_specification/error.rs | 6 + src/application_specification/hooks.rs | 58 +++++- src/application_specification/parse.rs | 18 +- src/application_specification/selinux.rs | 4 +- src/host_command/command_dispatcher.rs | 21 ++ src/host_command/commands/update_agent.rs | 87 ++++++-- src/installer/builder.rs | 197 +++++++++++++++--- src/installer/commands/change_mode_command.rs | 4 +- src/installer/commands/copy_command.rs | 4 +- src/installer/commands/mod.rs | 12 +- src/installer/core.rs | 9 +- src/installer/error.rs | 29 +++ src/lifecycle_event/script.rs | 122 +++++++++-- src/main.rs | 7 + src/runtime/file_based_deployment_tracker.rs | 3 +- src/system/env_ops.rs | 35 ++++ src/system/file_ops.rs | 17 ++ src/system/mod.rs | 18 +- tests/security/intake/appspec_validation.rs | 3 +- .../security/intake/installer_permissions.rs | 10 + tests/security/intake/process_isolation.rs | 70 +++---- 31 files changed, 1007 insertions(+), 132 deletions(-) create mode 100644 launchd/com.amazon.aws.codedeploy-agent.plist create mode 100644 macos/pkg/Distribution.xml.in create mode 100755 macos/pkg/build-pkg.sh create mode 100755 macos/pkg/scripts/postinstall create mode 100755 macos/pkg/scripts/preinstall create mode 100755 macos/pkg/uninstall.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1094fa1..4bdc8d3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,7 +40,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest] + os: [ubuntu-latest, windows-latest, macos-latest] steps: - uses: actions/checkout@v4 - name: Run tests @@ -54,7 +54,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest] + os: [ubuntu-latest, windows-latest, macos-latest] steps: - uses: actions/checkout@v4 - name: Build release binary diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index eccb52f..7d2164b 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -319,6 +319,36 @@ rustup default stable-x86_64-pc-windows-gnu cargo build --release ``` +## Building on macOS + +Targets: `aarch64-apple-darwin` (Apple silicon) and `x86_64-apple-darwin` +(Intel). Both build on an Apple-silicon Mac with the Xcode command-line tools; +there is no Intel-only step. `MACOSX_DEPLOYMENT_TARGET=15.0` matches the +oldest supported macOS (15 Sequoia). + +```bash +xcode-select --install +rustup target add aarch64-apple-darwin x86_64-apple-darwin + +export MACOSX_DEPLOYMENT_TARGET=15.0 +cargo build --release --target aarch64-apple-darwin +cargo build --release --target x86_64-apple-darwin +``` + +Package each binary as a `.pkg` (payload: binary, `codedeploy-local` symlink, +config template, LaunchDaemon plist; see `macos/pkg/`): + +```bash +macos/pkg/build-pkg.sh target/aarch64-apple-darwin/release/codedeploy-agent aarch64 2.1.1 2.1.1-1 out/ +macos/pkg/build-pkg.sh target/x86_64-apple-darwin/release/codedeploy-agent x86_64 2.1.1 2.1.1-1 out/ +sudo installer -pkg out/codedeploy-agent-2.1.1-1.aarch64.pkg -target / +``` + +The package installs `/Library/LaunchDaemons/com.amazon.aws.codedeploy-agent.plist` +(the launchd counterpart of `systemd/codedeploy-agent.service`) and starts the +agent. `sudo launchctl print system/com.amazon.aws.codedeploy-agent` shows its +state; `sudo /opt/codedeploy-agent/bin/uninstall.sh` removes it. + ## Running the Agent Locally Build and locate the binary: @@ -471,6 +501,7 @@ so `teardown` works across terminal sessions. | `e2e-ec2-iam-session.sh` | Fresh EC2 instance | On-prem registration, INI file — IMDS bypassed | | `e2e-ec2-concurrent.sh` | Fresh EC2 instance | IMDS, runs N parallel deployments (default 3) against the same instance | | `e2e-windows.sh` | Fresh Windows Server EC2 | IMDS | +| _(macOS)_ | No script: EC2 Mac requires a Dedicated Host with a 24 h minimum charge. Build the `.pkg` (see [Building on macOS](#building-on-macos)), install it on a Mac instance and run a deployment by hand. | IMDS | ### Shortest path to a green E2E run diff --git a/README.md b/README.md index 11932d5..d31204c 100644 --- a/README.md +++ b/README.md @@ -14,6 +14,34 @@ AWS Lambda compute platform. The agent is licensed under [Apache License 2.0](LICENSE). +## Supported platforms + +The agent is built and tested on the following operating systems. Linux and +Windows ship as rpm/deb and msi; macOS ships as a `.pkg` (see +[Building on macOS](DEVELOPMENT.md#building-on-macos)). + +| Platform | Versions | Architectures | Service manager | +|---|---|---|---| +| Amazon Linux | 2, 2023 | x86_64, aarch64 | systemd | +| RHEL, Oracle Linux | 8, 9, 10 | x86_64, aarch64 | systemd | +| Rocky Linux, CentOS Stream | 9, 10 | x86_64, aarch64 | systemd | +| SLES | 15 | x86_64, aarch64 | systemd | +| Debian | 11, 12, 13 | x86_64, aarch64 | systemd | +| Ubuntu Server | 16.04–26.04 LTS, 25.04 | x86_64, aarch64 | systemd | +| Windows Server | 2016, 2019, 2022, 2025 | x86_64 | Service Control Manager | +| macOS (EC2 Mac) | 15 Sequoia, 26 Tahoe, 27 | aarch64 (all); x86_64 (15) | launchd | + +macOS notes: +- Supported macOS versions are the current major release and the two before + it, reviewed each September. On EC2, Tahoe (26) and later are Apple silicon + only; `mac1.metal` (x86_64) tops out at 15. +- AppSpec `os: linux` is used for macOS deployments (`os: macos` is not a + valid value). +- AppSpec `permissions` fields `acls` and `context` (SELinux) are rejected on + macOS before the install runs; `owner`, `group` and `mode` work. +- Agent self-update (`UpdateDeploymentAgent`, `codedeploy-agent update`) is + not available on macOS yet; install the latest `.pkg` instead. + ## Getting Started ### Prerequisites diff --git a/conf/codedeployagent.yml b/conf/codedeployagent.yml index 40f6f73..238625e 100644 --- a/conf/codedeployagent.yml +++ b/conf/codedeployagent.yml @@ -4,6 +4,8 @@ # Logging :program_name: codedeploy-agent :log_dir: /var/log/aws/codedeploy-agent +# macOS: the LaunchDaemon plist also writes codedeploy-agent.launchd.log into +# the default directory above; changing :log_dir does not move that file. :verbose: false # Capture Amazon S3 HTTP wire logs to .aws_wire.log in log_dir diff --git a/launchd/com.amazon.aws.codedeploy-agent.plist b/launchd/com.amazon.aws.codedeploy-agent.plist new file mode 100644 index 0000000..ddb43e8 --- /dev/null +++ b/launchd/com.amazon.aws.codedeploy-agent.plist @@ -0,0 +1,101 @@ + + + + + + Label + com.amazon.aws.codedeploy-agent + + ProgramArguments + + /bin/bash + -a + -c + [ -f /etc/profile ] && source /etc/profile; BASH_EXECUTION_STRING='[ -f /etc/profile ] && source /etc/profile; /opt/codedeploy-agent/bin/codedeploy-agent start'; exec /opt/codedeploy-agent/bin/codedeploy-agent start + + + UserName + root + + RunAtLoad + + + KeepAlive + + Crashed + + + + ThrottleInterval + 10 + + ExitTimeOut + 7200 + + + SoftResourceLimits + + NumberOfFiles + 65536 + Core + -1 + + HardResourceLimits + + NumberOfFiles + 65536 + Core + -1 + + + StandardOutPath + /var/log/aws/codedeploy-agent/codedeploy-agent.launchd.log + StandardErrorPath + /var/log/aws/codedeploy-agent/codedeploy-agent.launchd.log + + + ProcessType + Adaptive + + diff --git a/macos/pkg/Distribution.xml.in b/macos/pkg/Distribution.xml.in new file mode 100644 index 0000000..9f12bbe --- /dev/null +++ b/macos/pkg/Distribution.xml.in @@ -0,0 +1,24 @@ + + + + AWS CodeDeploy Agent + com.amazon.aws + + + + + + + + + + + + + + + + + + com.amazon.aws.codedeploy-agent.pkg + diff --git a/macos/pkg/build-pkg.sh b/macos/pkg/build-pkg.sh new file mode 100755 index 0000000..e3f76de --- /dev/null +++ b/macos/pkg/build-pkg.sh @@ -0,0 +1,90 @@ +#!/bin/sh +# Build the macOS installer package (.pkg) for one architecture. +# +# Run after `cargo build --release --target `. +# Runs on macOS only (pkgbuild/productbuild are Xcode command-line tools). +# +# Usage: +# macos/pkg/build-pkg.sh +# binary path to the release codedeploy-agent binary for +# arch x86_64 | aarch64 (matches LATEST_VERSION pkg_ keys) +# version bare agent version, e.g. 2.1.1 (package version) +# full-version version-build, e.g. 2.1.1-1234 (file name only) +# out-dir directory to write codedeploy-agent-..pkg +# +# Payload (install-location /): +# /opt/codedeploy-agent/bin/codedeploy-agent 0755 +# /opt/codedeploy-agent/bin/codedeploy-local -> codedeploy-agent (symlink; see legacy_local_argv in src/main.rs) +# /private/etc/codedeploy-agent/conf/codedeployagent.yml 0644 (kept on upgrade, see scripts/) +# /Library/LaunchDaemons/com.amazon.aws.codedeploy-agent.plist 0644 +# (/etc is a symlink to /private/etc on macOS; the payload uses the real path.) +# Signing and notarization are separate steps, not done here. +set -eu + +if [ "$#" -ne 5 ]; then + sed -n '2,/^set -eu/p' "$0" | sed 's/^# \{0,1\}//' >&2 + exit 2 +fi + +BINARY=$1 +ARCH=$2 +VERSION=$3 +FULL_VERSION=$4 +OUT_DIR=$5 + +case "$ARCH" in + x86_64) HOST_ARCH=x86_64 ;; + aarch64) HOST_ARCH=arm64 ;; # Distribution.xml uses Apple's name + *) echo "arch must be x86_64 or aarch64, got '$ARCH'" >&2; exit 2 ;; +esac +[ -f "$BINARY" ] || { echo "binary not found: $BINARY" >&2; exit 2; } + +HERE=$(cd "$(dirname "$0")" && pwd) +REPO=$(cd "$HERE/../.." && pwd) +IDENTIFIER=com.amazon.aws.codedeploy-agent +MIN_OS=15.0 # oldest supported macOS; must match MACOSX_DEPLOYMENT_TARGET + +WORK=$(mktemp -d "${TMPDIR:-/tmp}/codedeploy-pkg.XXXXXX") +trap 'rm -rf "$WORK"' EXIT +ROOT="$WORK/root" + +# --- payload --------------------------------------------------------------- +install -d -m 0755 "$ROOT/opt/codedeploy-agent/bin" +install -m 0755 "$BINARY" "$ROOT/opt/codedeploy-agent/bin/codedeploy-agent" +ln -s codedeploy-agent "$ROOT/opt/codedeploy-agent/bin/codedeploy-local" +# pkg has no uninstall hook; ship the manual uninstaller next to the binary. +install -m 0755 "$HERE/uninstall.sh" "$ROOT/opt/codedeploy-agent/bin/uninstall.sh" + +install -d -m 0755 "$ROOT/private/etc/codedeploy-agent/conf" +install -m 0644 "$REPO/conf/codedeployagent.yml" "$ROOT/private/etc/codedeploy-agent/conf/codedeployagent.yml" + +install -d -m 0755 "$ROOT/Library/LaunchDaemons" +install -m 0644 "$REPO/launchd/$IDENTIFIER.plist" "$ROOT/Library/LaunchDaemons/$IDENTIFIER.plist" + +# --- component package ----------------------------------------------------- +COMPONENT="$WORK/$IDENTIFIER.pkg" +# installer silently skips non-executable scripts. +chmod 0755 "$HERE/scripts/preinstall" "$HERE/scripts/postinstall" +pkgbuild \ + --root "$ROOT" \ + --install-location / \ + --identifier "$IDENTIFIER" \ + --version "$VERSION" \ + --scripts "$HERE/scripts" \ + --ownership recommended \ + "$COMPONENT" + +# --- distribution package -------------------------------------------------- +sed -e "s|@VERSION@|$VERSION|g" \ + -e "s|@HOST_ARCH@|$HOST_ARCH|g" \ + -e "s|@MIN_OS@|$MIN_OS|g" \ + "$HERE/Distribution.xml.in" > "$WORK/Distribution.xml" + +mkdir -p "$OUT_DIR" +OUT="$OUT_DIR/codedeploy-agent-$FULL_VERSION.$ARCH.pkg" +productbuild \ + --distribution "$WORK/Distribution.xml" \ + --package-path "$WORK" \ + "$OUT" + +echo "$OUT" diff --git a/macos/pkg/scripts/postinstall b/macos/pkg/scripts/postinstall new file mode 100755 index 0000000..1e273e4 --- /dev/null +++ b/macos/pkg/scripts/postinstall @@ -0,0 +1,47 @@ +#!/bin/sh +# pkg postinstall: runs as root after the payload is written. +# +# Like rpm %post / deb postinst: restore the config preinstall saved (keeping +# the new template as .pkgnew), create the log dir launchd writes to, then +# enable and bootstrap the LaunchDaemon. +set -eu + +LABEL=com.amazon.aws.codedeploy-agent +PLIST=/Library/LaunchDaemons/$LABEL.plist +CONF=/etc/codedeploy-agent/conf/codedeployagent.yml + +if [ -f "$CONF.pkg-save" ]; then + cp -p "$CONF" "$CONF.pkgnew" + mv -f "$CONF.pkg-save" "$CONF" +fi + +install -d -m 0755 /var/log/aws/codedeploy-agent +install -d -m 0755 /opt/codedeploy-agent + +launchctl enable "system/$LABEL" || : +# bootout any stale definition (kickstart would reuse the old one), then +# bootstrap the plist just installed; a failure fails the install. +launchctl bootout "system/$LABEL" 2>/dev/null || : +# Normally nothing is running here; if something re-bootstrapped the agent +# mid-deployment, wait the same ExitTimeOut bound as preinstall. +i=0 +while launchctl print "system/$LABEL" >/dev/null 2>&1; do + i=$((i + 1)) + if [ "$i" -ge 7260 ]; then # ExitTimeOut + margin: let launchd's own SIGKILL land first + echo "$LABEL is still loaded after ${i}s (ExitTimeOut); cannot bootstrap the new plist" >&2 + exit 1 + fi + sleep 1 +done +# bootstrap can fail transiently right after a bootout; retry briefly. +i=0 +until launchctl bootstrap system "$PLIST"; do + i=$((i + 1)) + if [ "$i" -ge 5 ]; then + echo "$LABEL: launchctl bootstrap failed after $i attempts" >&2 + exit 1 + fi + sleep 1 +done + +exit 0 diff --git a/macos/pkg/scripts/preinstall b/macos/pkg/scripts/preinstall new file mode 100755 index 0000000..3feed5e --- /dev/null +++ b/macos/pkg/scripts/preinstall @@ -0,0 +1,38 @@ +#!/bin/sh +# pkg preinstall: runs as root before the payload is written. +# +# Like rpm %pre / deb preinst: save the operator's config for postinstall to +# restore (the payload ships the template), then stop the agent and wait for +# it to exit so the binary is not replaced under it. +set -eu + +LABEL=com.amazon.aws.codedeploy-agent +CONF=/etc/codedeploy-agent/conf/codedeployagent.yml + +if [ -f "$CONF" ]; then + # Keep an existing save: after a crashed upgrade $CONF may be the template + # and .pkg-save the only copy of the operator's config. postinstall removes + # the save on success, so a lingering one always means "restore me". + [ -f "$CONF.pkg-save" ] || cp -p "$CONF" "$CONF.pkg-save" +else + # Fresh install: a stale save must not be restored over the template. + rm -f "$CONF.pkg-save" +fi + +if launchctl print "system/$LABEL" >/dev/null 2>&1; then + launchctl bootout "system/$LABEL" || : + # bootout only sends SIGTERM; wait for the job to unload before the payload + # replaces the binary. launchd SIGKILLs at ExitTimeOut (7200 s), so a job + # still present after that means launchd is wedged: fail the install. + i=0 + while launchctl print "system/$LABEL" >/dev/null 2>&1; do + i=$((i + 1)) + if [ "$i" -ge 7260 ]; then # ExitTimeOut + margin: let launchd's own SIGKILL land first + echo "$LABEL did not exit within ExitTimeOut; aborting install" >&2 + exit 1 + fi + sleep 1 + done +fi + +exit 0 diff --git a/macos/pkg/uninstall.sh b/macos/pkg/uninstall.sh new file mode 100755 index 0000000..bd8be3c --- /dev/null +++ b/macos/pkg/uninstall.sh @@ -0,0 +1,40 @@ +#!/bin/sh +# Uninstall the AWS CodeDeploy agent on macOS. +# +# .pkg has no uninstall hook; this is the rpm %preun / deb prerm counterpart: +# sudo /opt/codedeploy-agent/bin/uninstall.sh +# Deployment state, logs and the operator's config are left in place, as the +# Linux packages do; only the .pkgnew template copy is removed. +set -eu + +LABEL=com.amazon.aws.codedeploy-agent +PLIST=/Library/LaunchDaemons/$LABEL.plist + +if launchctl print "system/$LABEL" >/dev/null 2>&1; then + launchctl bootout "system/$LABEL" || : + # bootout only sends SIGTERM; wait (up to ExitTimeOut) for the job to + # unload before removing the binary it may still re-exec. + i=0 + while launchctl print "system/$LABEL" >/dev/null 2>&1; do + i=$((i + 1)) + if [ "$i" -ge 7260 ]; then # ExitTimeOut + margin: let launchd's own SIGKILL land first + echo "$LABEL did not exit within ExitTimeOut (7200 s); aborting uninstall" >&2 + exit 1 + fi + sleep 1 + done +fi +launchctl disable "system/$LABEL" || : + +pkgutil --forget "$LABEL" >/dev/null 2>&1 || : + +rm -f "$PLIST" +rm -f /opt/codedeploy-agent/bin/codedeploy-local +rm -f /opt/codedeploy-agent/bin/codedeploy-agent +rm -f /opt/codedeploy-agent/bin/uninstall.sh # this script; sh keeps its own fd open +rmdir /opt/codedeploy-agent/bin 2>/dev/null || : +# Keep codedeployagent.yml; drop only the .pkgnew template copy. +rm -f /etc/codedeploy-agent/conf/codedeployagent.yml.pkgnew +rmdir /etc/codedeploy-agent/conf /etc/codedeploy-agent 2>/dev/null || : + +echo "AWS CodeDeploy agent removed." diff --git a/src/application_specification/error.rs b/src/application_specification/error.rs index 58ffe5a..1ea30d4 100644 --- a/src/application_specification/error.rs +++ b/src/application_specification/error.rs @@ -33,6 +33,11 @@ pub enum ParseError { )] InvalidTimeout, + #[error( + "The deployment failed because the runas value ({0}) in the application specification file is not a valid user name. The name must not be empty, must not start with '-', and must not contain whitespace or control characters. Update the value, and then try again." + )] + InvalidRunas(String), + #[error( "The deployment failed because the application specification file specifies a destination file, but no source file. Update the files section of the AppSpec file, and then try again." )] @@ -267,6 +272,7 @@ mod tests { ParseError::InvalidFileExistsBehavior("bad".to_string()), ParseError::EmptyScriptLocation, ParseError::InvalidTimeout, + ParseError::InvalidRunas("-c".to_string()), ParseError::MissingSource, ParseError::MissingDestination("file".to_string()), ParseError::PermissionsOnWindows, diff --git a/src/application_specification/hooks.rs b/src/application_specification/hooks.rs index 4c79988..ce39fd3 100644 --- a/src/application_specification/hooks.rs +++ b/src/application_specification/hooks.rs @@ -87,7 +87,24 @@ impl ScriptLocation { } #[derive(Debug, Clone, PartialEq)] -pub struct Username(pub(crate) String); +pub struct Username(String); + +impl Username { + /// Reject a `runas` that `su`/`sudo` would read as an option (leading `-`) + /// or that contains whitespace/control characters. Everything else is + /// accepted (`DOMAIN\user`, `user@realm`); shell metacharacters are inert + /// because `runas` is passed as its own argv entry, never through a shell. + pub(crate) fn new(name: &str) -> Result { + let valid = !name.is_empty() + && !name.starts_with('-') + && !name.bytes().any(|b| b.is_ascii_whitespace() || b.is_ascii_control()); + if valid { + Ok(Username(name.to_string())) + } else { + Err(ParseError::InvalidRunas(name.to_string())) + } + } +} #[derive(Debug, Clone, Copy, PartialEq)] pub struct Timeout(u32); @@ -117,6 +134,7 @@ impl Default for Timeout { #[cfg(test)] mod tests { + use super::{ParseError, Username}; use crate::application_specification::AppSpec; #[test] @@ -139,4 +157,42 @@ mod tests { let scripts = spec.hooks().get("ApplicationStop"); assert!(scripts.is_empty()); } + + #[test] + fn username_accepts_real_account_names_including_domain_forms() { + for n in [ + "root", + "deploy", + "www-data", + "svc_user", + "user.name", + "host$", + "DOMAIN\\user", + "user@realm", + "a/b", + "root;id", + "$(id)", + ] { + assert!(Username::new(n).is_ok(), "{n:?} should be accepted"); + } + } + + #[test] + fn username_rejects_option_like_and_whitespace_names() { + for n in [ + "-c", + "--command=id", + "-", + "a b", + "\troot", + "root\n", + "a\0b", + "", + ] { + assert!( + matches!(Username::new(n), Err(ParseError::InvalidRunas(_))), + "{n:?} should be rejected" + ); + } + } } diff --git a/src/application_specification/parse.rs b/src/application_specification/parse.rs index 149c0d9..14a78b3 100644 --- a/src/application_specification/parse.rs +++ b/src/application_specification/parse.rs @@ -80,14 +80,10 @@ impl RawScript { None => return Err(ParseError::EmptyScriptLocation), }; - let runas = self.runas.and_then(|s| { - let trimmed = s.trim().to_string(); - if trimmed.is_empty() { - None - } else { - Some(Username(trimmed)) - } - }); + let runas = match self.runas.as_deref().map(str::trim) { + None | Some("") => None, + Some(trimmed) => Some(Username::new(trimmed)?), + }; let timeout = match self.timeout { Some(t) => Timeout::new(t)?, @@ -1059,4 +1055,10 @@ mod tests { assert_eq!(ctx.user(), Some("system_u")); assert_eq!(ctx.type_(), "object_t"); } + + #[test] + fn script_runas_option_like_value_is_rejected() { + let yaml = "version: 0.0\nos: linux\nhooks:\n ApplicationStart:\n - location: start.sh\n runas: \"-c\"\n"; + assert!(matches!(AppSpec::parse(yaml), Err(ParseError::InvalidRunas(v)) if v == "-c")); + } } diff --git a/src/application_specification/selinux.rs b/src/application_specification/selinux.rs index e99cf7f..1f1c639 100644 --- a/src/application_specification/selinux.rs +++ b/src/application_specification/selinux.rs @@ -28,7 +28,9 @@ impl SeLinuxContext { /// /// This is only used for testing the role validation error path. /// In production, role is always None. - #[cfg(test)] + // SELinux is Linux-only; macOS is unix but not linux. The only caller is the + // Linux-gated ChangeContextCommand test. + #[cfg(all(test, target_os = "linux"))] pub(crate) fn new_with_role( user: Option, role: Option, diff --git a/src/host_command/command_dispatcher.rs b/src/host_command/command_dispatcher.rs index e2a3969..cdfc0aa 100644 --- a/src/host_command/command_dispatcher.rs +++ b/src/host_command/command_dispatcher.rs @@ -389,6 +389,9 @@ mod tests { assert!(result.is_ok()); } + // Split by platform: on macOS UpdateAgentCommand refuses before the S3 check + // (UNSUPPORTED_PLATFORM_MESSAGE), so the S3-error assertion only holds elsewhere. + #[cfg(not(target_os = "macos"))] #[test] fn update_deployment_agent_returns_error_without_s3() { let dir = tempfile::TempDir::new().unwrap(); @@ -405,6 +408,24 @@ mod tests { assert!(result.unwrap_err().to_string().contains("S3 client not configured")); } + // On macOS self-update is refused before the S3 check (see update_agent.rs). + #[cfg(target_os = "macos")] + #[test] + fn update_deployment_agent_refuses_on_macos() { + let dir = tempfile::TempDir::new().unwrap(); + let dispatcher = CommandDispatcher::new( + test_archives(&dir), + None, + HashMap::new(), + "us-east-1", + None, + Arc::new(AgentConfig::default()), + ); + let result = dispatcher.execute_command("UpdateDeploymentAgent", &test_spec()); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("not supported on macOS")); + } + #[test] fn update_deployment_agent_never_noop() { let dir = tempfile::TempDir::new().unwrap(); diff --git a/src/host_command/commands/update_agent.rs b/src/host_command/commands/update_agent.rs index c4256bd..94fb0dc 100644 --- a/src/host_command/commands/update_agent.rs +++ b/src/host_command/commands/update_agent.rs @@ -26,6 +26,13 @@ const SHELL: &str = "/bin/sh"; /// Package-type arg: `auto` tells the script to detect the package manager. const PACKAGE_TYPE_ARG: &str = "auto"; +/// Why self-update is refused on macOS. The bucket install script has no +/// `pkg` package type yet; until it does, running it on macOS would +/// only fail inside the script after the download. +#[cfg(target_os = "macos")] +pub(crate) const UNSUPPORTED_PLATFORM_MESSAGE: &str = "Agent self-update is not supported on macOS yet. \ + Install the latest .pkg from the regional aws-codedeploy- bucket instead."; + pub struct UpdateAgentCommand { region: String, s3_client: Option, @@ -70,6 +77,8 @@ impl UpdateAgentCommand { pub fn execute(&self) -> io::Result> { info!("UpdateDeploymentAgent command received"); + Self::check_platform()?; + let s3 = self .s3_client .as_ref() @@ -90,6 +99,25 @@ impl UpdateAgentCommand { } } +impl UpdateAgentCommand { + /// Refuse self-update on platforms the bucket install script cannot + /// install on. Called before any credential, IMDS or S3 use so the + /// refusal is immediate and needs no network access. + /// + /// # Errors + /// `ErrorKind::Unsupported` on macOS. `Ok` elsewhere. + pub fn check_platform() -> io::Result<()> { + #[cfg(target_os = "macos")] + { + Err(io::Error::new(io::ErrorKind::Unsupported, UNSUPPORTED_PLATFORM_MESSAGE)) + } + #[cfg(not(target_os = "macos"))] + { + Ok(()) + } + } +} + /// Construct the S3 bucket name for agent packages. fn s3_bucket_name(region: &str) -> String { format!("aws-codedeploy-{region}") @@ -136,12 +164,13 @@ fn execute_script(script_path: &Path, region: &str) -> io::Result { cmd.output() } -/// Strip `LD_*` loader vars so they can't inject a shared library into the -/// root-run install script or its sub-processes. Mirrors `Script::execute_async`. +/// Strip the dynamic-loader vars (`LD_*` on Linux, `DYLD_*` on macOS) so they +/// can't inject a shared library into the root-run install script or its +/// sub-processes. Mirrors `Script::execute_async`. /// Call after any `.env()` mutation so the removal is unconditional. fn strip_dynamic_loader_vars(cmd: &mut Command) { #[cfg(unix)] - for var in ["LD_PRELOAD", "LD_LIBRARY_PATH", "LD_AUDIT"] { + for var in crate::system::DYNAMIC_LOADER_ENV_VARS { cmd.env_remove(var); } #[cfg(not(unix))] @@ -259,6 +288,24 @@ mod tests { assert!(debug.contains("us-east-1")); } + #[cfg(target_os = "macos")] + #[test] + fn check_platform_refuses_macos_before_any_network_use() { + let err = UpdateAgentCommand::check_platform().unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::Unsupported); + assert!(err.to_string().contains("not supported on macOS")); + // execute() must return the same refusal even with no S3 client. + let cmd = UpdateAgentCommand::new("us-east-1".into(), None); + assert_eq!(cmd.execute().unwrap_err().kind(), io::ErrorKind::Unsupported); + } + + #[cfg(not(target_os = "macos"))] + #[test] + fn check_platform_allows_non_macos() { + assert!(UpdateAgentCommand::check_platform().is_ok()); + } + + #[cfg(not(target_os = "macos"))] #[test] fn execute_without_s3_client_errors() { let cmd = UpdateAgentCommand::new("us-east-1".into(), None); @@ -355,6 +402,7 @@ mod tests { #[cfg(unix)] #[test] fn strip_dynamic_loader_vars_removes_loader_env_from_child() { + use crate::system::DYNAMIC_LOADER_ENV_VARS; use std::os::unix::fs::OpenOptionsExt; let tmp = tempfile::tempdir().unwrap(); let script = tmp.path().join("install"); @@ -366,30 +414,37 @@ mod tests { .mode(0o600) .open(&script) .unwrap(); - // Report the loader vars the script sees; all must be empty. - writeln!( - f, - "#!/bin/sh\nprintf '[%s][%s][%s]' \ - \"$LD_PRELOAD\" \"$LD_LIBRARY_PATH\" \"$LD_AUDIT\" > \"{}\"", - marker.display() - ) - .unwrap(); + // Print every platform loader var; all must be empty. + let refs: Vec = DYNAMIC_LOADER_ENV_VARS.iter().map(|v| format!("[${v}]")).collect(); + writeln!(f, "#!/bin/sh\nprintf '%s' \"{}\" > \"{}\"", refs.join(""), marker.display()) + .unwrap(); drop(f); // Seed the loader vars directly on the Command (simulating inheritance // from the agent env) without mutating the test process's own env — // the crate forbids unsafe, so std::env::set_var is unavailable. let mut cmd = Command::new(SHELL); - cmd.arg(&script) - .env("LD_PRELOAD", "/tmp/evil.so") - .env("LD_LIBRARY_PATH", "/tmp/evil") - .env("LD_AUDIT", "/tmp/audit.so"); + cmd.arg(&script); + for var in DYNAMIC_LOADER_ENV_VARS { + cmd.env(var, "/tmp/evil"); + } strip_dynamic_loader_vars(&mut cmd); + // `env_remove` shows as `None` in `get_envs()`. This is the check that + // holds on macOS, where SIP scrubs DYLD_* before /bin/sh could see them. + for var in DYNAMIC_LOADER_ENV_VARS { + let entry = cmd.get_envs().find(|(k, _)| *k == std::ffi::OsStr::new(var)); + assert!( + matches!(entry, Some((_, None))), + "{var} must be marked removed on the Command, got {entry:?}" + ); + } + let output = cmd.output().unwrap(); assert!(output.status.success()); - assert_eq!(std::fs::read_to_string(&marker).unwrap(), "[][][]"); + let expected = "[]".repeat(DYNAMIC_LOADER_ENV_VARS.len()); + assert_eq!(std::fs::read_to_string(&marker).unwrap(), expected); } #[cfg(unix)] diff --git a/src/installer/builder.rs b/src/installer/builder.rs index 8565838..fa07eb9 100644 --- a/src/installer/builder.rs +++ b/src/installer/builder.rs @@ -1,9 +1,9 @@ //! Install command builder — generates copy, permission, and cleanup commands from `AppSpec`. use crate::application_specification::Permission; +#[cfg(target_os = "linux")] +use crate::installer::commands::{ChangeAclCommand, ChangeContextCommand}; #[cfg(unix)] -use crate::installer::commands::{ - ChangeAclCommand, ChangeContextCommand, ChangeModeCommand, ChangeOwnerCommand, -}; +use crate::installer::commands::{ChangeModeCommand, ChangeOwnerCommand}; use crate::installer::commands::{CopyCommand, MakeDirectoryCommand, RemoveCommand}; use crate::installer::{InstallerError, Result}; use nu_path; @@ -18,6 +18,8 @@ pub struct CommandBuilder { copy_targets: HashMap, mkdir_targets: HashSet, permission_targets: HashSet, + // SELinux is Linux-only; macOS is unix but not linux. + #[cfg(target_os = "linux")] reject_unconfined_selinux: bool, reject_unsafe_permissions: bool, reject_symlink_permission_targets: bool, @@ -32,9 +34,9 @@ pub enum Command { Chmod(ChangeModeCommand), #[cfg(unix)] Chown(ChangeOwnerCommand), - #[cfg(unix)] + #[cfg(target_os = "linux")] Acl(ChangeAclCommand), - #[cfg(unix)] + #[cfg(target_os = "linux")] Context(ChangeContextCommand), } @@ -50,9 +52,9 @@ impl Command { Command::Chmod(cmd) => cmd.execute(cleanup), #[cfg(unix)] Command::Chown(cmd) => cmd.execute(cleanup), - #[cfg(unix)] + #[cfg(target_os = "linux")] Command::Acl(cmd) => cmd.execute(cleanup), - #[cfg(unix)] + #[cfg(target_os = "linux")] Command::Context(cmd) => cmd.execute(cleanup), } } @@ -68,9 +70,9 @@ impl Command { Command::Chmod(cmd) => cmd.to_h(), #[cfg(unix)] Command::Chown(cmd) => cmd.to_h(), - #[cfg(unix)] + #[cfg(target_os = "linux")] Command::Acl(cmd) => cmd.to_h(), - #[cfg(unix)] + #[cfg(target_os = "linux")] Command::Context(cmd) => cmd.to_h(), } } @@ -94,11 +96,16 @@ impl CommandBuilder { reject_unsafe_permissions: bool, reject_symlink_permission_targets: bool, ) -> Self { + // Same signature on every platform; unused off Linux. + #[cfg(not(target_os = "linux"))] + let _ = reject_unconfined_selinux; Self { commands: Vec::new(), copy_targets: HashMap::new(), mkdir_targets: HashSet::new(), permission_targets: HashSet::new(), + // SELinux is Linux-only; macOS is unix but not linux. + #[cfg(target_os = "linux")] reject_unconfined_selinux, reject_unsafe_permissions, reject_symlink_permission_targets, @@ -166,6 +173,38 @@ impl CommandBuilder { /// # Errors /// Returns an error if there's a duplicate permission target. + /// `acls` (setfacl) and `context` (semanage) exist on Linux only; + /// reject them on other Unix before any command is built. Called on entry + /// to `Installer::process_permission` (so it fires even when the entry + /// matches nothing) and from `set_permissions`. + pub fn check_permission_fields_supported(object: &Path, permission: &Permission) -> Result<()> { + #[cfg(target_os = "linux")] + { + let _ = (object, permission); + Ok(()) + } + #[cfg(not(target_os = "linux"))] + { + let platform = if cfg!(target_os = "macos") { + "macOS" + } else { + std::env::consts::OS + }; + let field = if permission.acls().is_some() { + "acls" + } else if permission.context().is_some() { + "context" + } else { + return Ok(()); + }; + Err(InstallerError::PermissionFieldNotSupported { + object: object.to_path_buf(), + field, + platform, + }) + } + } + pub fn set_permissions(&mut self, object: &Path, permission: &Permission) -> Result<()> { debug!("Setting permissions on {}", object.display()); @@ -181,6 +220,8 @@ impl CommandBuilder { #[cfg(unix)] { + Self::check_permission_fields_supported(&object, permission)?; + if let Some(mode) = permission.mode() { self.commands.push(Command::Chmod(ChangeModeCommand::new( object.clone(), @@ -190,21 +231,24 @@ impl CommandBuilder { ))); } - if let Some(acls) = permission.acls() { - self.commands.push(Command::Acl(ChangeAclCommand::new( - object.clone(), - acls.clone(), - self.reject_symlink_permission_targets, - ))); - } + #[cfg(target_os = "linux")] + { + if let Some(acls) = permission.acls() { + self.commands.push(Command::Acl(ChangeAclCommand::new( + object.clone(), + acls.clone(), + self.reject_symlink_permission_targets, + ))); + } - if let Some(context) = permission.context() { - self.commands.push(Command::Context(ChangeContextCommand::new( - object.clone(), - context.clone(), - self.reject_unconfined_selinux, - self.reject_symlink_permission_targets, - ))); + if let Some(context) = permission.context() { + self.commands.push(Command::Context(ChangeContextCommand::new( + object.clone(), + context.clone(), + self.reject_unconfined_selinux, + self.reject_symlink_permission_targets, + ))); + } } if permission.owner().is_some() || permission.group().is_some() { @@ -502,7 +546,7 @@ mod tests { } #[test] - #[cfg(unix)] + #[cfg(target_os = "linux")] fn command_execute_acl() { use crate::application_specification::Acl; @@ -518,7 +562,7 @@ mod tests { } #[test] - #[cfg(unix)] + #[cfg(target_os = "linux")] fn command_execute_context() { use crate::application_specification::SeLinuxContext; @@ -715,7 +759,7 @@ mod tests { } #[test] - #[cfg(unix)] + #[cfg(target_os = "linux")] fn set_permissions_with_acl() { use crate::application_specification::{Acl, ObjectType, Permission}; @@ -747,8 +791,103 @@ mod tests { fs::remove_file(&dst).ok(); } + /// On a non-Linux Unix host `acls` has no setfacl to call, so the + /// builder must reject it before any command is queued. #[test] - #[cfg(unix)] + #[cfg(all(unix, not(target_os = "linux")))] + fn set_permissions_rejects_acls_on_non_linux_unix() { + use crate::application_specification::{Acl, ObjectType, Permission}; + + let mut builder = CommandBuilder::new(); + let dst = std::env::temp_dir().join("test_setperm_acl_nonlinux.txt"); + fs::write(&dst, "test").unwrap(); + + let acl = Acl::parse(&["user:testuser:rwx".to_string()]).unwrap(); + let perm = Permission::new_for_test( + "**".to_string(), + vec![ObjectType::File], + vec![], + None, + None, + // `mode` set: the rejection must come before the chmod is queued. + Some(crate::application_specification::Mode::from_octal("0644").unwrap()), + Some(acl), + None, + ); + + let err = builder.set_permissions(&dst, &perm).unwrap_err(); + assert!( + matches!(err, InstallerError::PermissionFieldNotSupported { field: "acls", .. }), + "expected PermissionFieldNotSupported for acls, got {err:?}" + ); + assert!(builder.commands().is_empty(), "no command may be queued after rejection"); + + fs::remove_file(&dst).ok(); + } + + /// The rejection is independent of whether the permission entry + /// matches any object; `process_permission` calls this on entry. + #[test] + #[cfg(all(unix, not(target_os = "linux")))] + fn check_permission_fields_rejects_context_without_a_match() { + use crate::application_specification::{ObjectType, Permission, SeLinuxContext}; + + let ctx = SeLinuxContext::new(None, "httpd_sys_content_t".to_string(), None); + let perm = Permission::new_for_test( + "/nonexistent/dir".to_string(), + vec![ObjectType::Directory], + vec![], + None, + None, + None, + None, + Some(ctx), + ); + + let err = + CommandBuilder::check_permission_fields_supported(Path::new("/nonexistent/dir"), &perm) + .unwrap_err(); + assert!( + matches!(err, InstallerError::PermissionFieldNotSupported { field: "context", .. }), + "expected PermissionFieldNotSupported for context, got {err:?}" + ); + } + + /// `context` (SELinux) is rejected the same way on non-Linux Unix. + #[test] + #[cfg(all(unix, not(target_os = "linux")))] + fn set_permissions_rejects_context_on_non_linux_unix() { + use crate::application_specification::{ObjectType, Permission, SeLinuxContext}; + + let mut builder = CommandBuilder::new(); + let dst = std::env::temp_dir().join("test_setperm_ctx_nonlinux.txt"); + fs::write(&dst, "test").unwrap(); + + let ctx = SeLinuxContext::new(None, "httpd_sys_content_t".to_string(), None); + let perm = Permission::new_for_test( + "**".to_string(), + vec![ObjectType::File], + vec![], + None, + None, + // `mode` set: the rejection must come before the chmod is queued. + Some(crate::application_specification::Mode::from_octal("0644").unwrap()), + None, + Some(ctx), + ); + + let err = builder.set_permissions(&dst, &perm).unwrap_err(); + assert!( + matches!(err, InstallerError::PermissionFieldNotSupported { field: "context", .. }), + "expected PermissionFieldNotSupported for context, got {err:?}" + ); + assert!(builder.commands().is_empty()); + + fs::remove_file(&dst).ok(); + } + + #[test] + #[cfg(target_os = "linux")] fn set_permissions_with_context() { use crate::application_specification::{ObjectType, Permission, SeLinuxContext}; @@ -855,7 +994,7 @@ mod tests { } #[test] - #[cfg(unix)] + #[cfg(target_os = "linux")] fn command_to_h_acl() { use crate::application_specification::Acl; let file = std::env::temp_dir().join("test_acl.txt"); @@ -870,7 +1009,7 @@ mod tests { } #[test] - #[cfg(unix)] + #[cfg(target_os = "linux")] fn command_to_h_context() { use crate::application_specification::SeLinuxContext; let file = std::env::temp_dir().join("test_ctx.txt"); diff --git a/src/installer/commands/change_mode_command.rs b/src/installer/commands/change_mode_command.rs index 859e71b..e84ae4a 100644 --- a/src/installer/commands/change_mode_command.rs +++ b/src/installer/commands/change_mode_command.rs @@ -26,7 +26,7 @@ impl ChangeModeCommand { /// # Errors /// Returns an error if the command execution fails. pub fn execute(&self, _cleanup_file: &mut dyn Write) -> Result<()> { - use nix::sys::stat::{Mode, fchmod}; + use nix::sys::stat::fchmod; use std::os::fd::AsRawFd; use std::os::unix::fs::OpenOptionsExt; @@ -40,7 +40,7 @@ impl ChangeModeCommand { }); } - let mode_bits = Mode::from_bits_truncate(mode); + let mode_bits = crate::system::file_ops::mode_from_bits(mode); // By default the chmod follows symlinks (backwards-compatible behavior), // which a path-based `chmod` gives. diff --git a/src/installer/commands/copy_command.rs b/src/installer/commands/copy_command.rs index c868fc5..2247b38 100644 --- a/src/installer/commands/copy_command.rs +++ b/src/installer/commands/copy_command.rs @@ -48,7 +48,7 @@ impl CopyCommand { } else { #[cfg(unix)] { - use nix::sys::stat::{Mode, fchmod}; + use nix::sys::stat::fchmod; use nix::unistd::fchown; use std::os::fd::AsRawFd; use std::os::unix::fs::{MetadataExt, OpenOptionsExt, PermissionsExt}; @@ -89,7 +89,7 @@ impl CopyCommand { // Preserve permissions on the fd, after fchown so SUID/SGID bits // survive. Uses fd-based src_meta so owner, mode, and timestamps // share one race-free source. - let mode = Mode::from_bits_truncate(src_meta.permissions().mode()); + let mode = crate::system::file_ops::mode_from_bits(src_meta.permissions().mode()); fchmod(dest.as_raw_fd(), mode) .map_err(|e| std::io::Error::from_raw_os_error(e as i32))?; diff --git a/src/installer/commands/mod.rs b/src/installer/commands/mod.rs index 4bdbdfb..d92c6b6 100644 --- a/src/installer/commands/mod.rs +++ b/src/installer/commands/mod.rs @@ -1,7 +1,7 @@ //! Installer command types — copy, mkdir, chmod, chown, setfacl, semanage. -#[cfg(unix)] +#[cfg(target_os = "linux")] mod change_acl_command; -#[cfg(unix)] +#[cfg(target_os = "linux")] mod change_context_command; #[cfg(unix)] mod change_mode_command; @@ -10,12 +10,12 @@ mod change_owner_command; mod copy_command; mod make_directory_command; mod remove_command; -#[cfg(unix)] +#[cfg(target_os = "linux")] mod remove_context_command; -#[cfg(unix)] +#[cfg(target_os = "linux")] pub use change_acl_command::ChangeAclCommand; -#[cfg(unix)] +#[cfg(target_os = "linux")] pub use change_context_command::ChangeContextCommand; #[cfg(unix)] pub use change_mode_command::ChangeModeCommand; @@ -24,5 +24,5 @@ pub use change_owner_command::ChangeOwnerCommand; pub use copy_command::CopyCommand; pub use make_directory_command::MakeDirectoryCommand; pub use remove_command::RemoveCommand; -#[cfg(unix)] +#[cfg(target_os = "linux")] pub use remove_context_command::RemoveContextCommand; diff --git a/src/installer/core.rs b/src/installer/core.rs index fad9008..5585af2 100644 --- a/src/installer/core.rs +++ b/src/installer/core.rs @@ -1,5 +1,5 @@ //! Installer core — executes install commands and manages cleanup files. -#[cfg(unix)] +#[cfg(target_os = "linux")] use super::commands::RemoveContextCommand; use super::{ builder::{Command, CommandBuilder}, @@ -270,6 +270,11 @@ impl Installer { ) -> Result<()> { let object = Path::new(permission.object()); + // On non-Linux Unix, `acls`/`context` are rejected here, before + // matching, so an entry that matches no object is still refused. + #[cfg(unix)] + CommandBuilder::check_permission_fields_supported(object, permission)?; + // Handle directory permissions if !builder.copying_file(object) { if builder.making_directory(object) || object.is_dir() { @@ -481,7 +486,7 @@ impl Installer { } if line.starts_with("semanage\0") { - #[cfg(unix)] + #[cfg(target_os = "linux")] { let object = line.strip_prefix("semanage\0").unwrap(); // Validate the SELinux object path the same way as a removal diff --git a/src/installer/error.rs b/src/installer/error.rs index 7ac17fd..43dbb1d 100644 --- a/src/installer/error.rs +++ b/src/installer/error.rs @@ -48,6 +48,14 @@ pub enum InstallerError { object: PathBuf, }, SelinuxRoleNotSupported, + /// An `AppSpec` `permissions` field that requires a Linux-only tool + /// (`acls` → setfacl, `context` → semanage/restorecon) was used on a + /// non-Linux Unix host (macOS). Raised before any command is built. + PermissionFieldNotSupported { + object: PathBuf, + field: &'static str, + platform: &'static str, + }, UnconfinedSelinuxRejected { type_: String, }, @@ -174,6 +182,14 @@ impl fmt::Display for InstallerError { ) }, + Self::PermissionFieldNotSupported { object, field, platform } => { + write!( + f, + "The deployment failed because the application specification file specifies '{field}' for {}, but '{field}' is not supported on {platform}. Remove '{field}' from the permissions entry in the AppSpec file, and then try again.", + object.display() + ) + }, + Self::UnconfinedSelinuxRejected { type_ } => { write!( f, @@ -332,6 +348,19 @@ mod tests { assert!(err.to_string().contains("roles are not supported")); } + #[test] + fn permission_field_not_supported_display() { + let err = InstallerError::PermissionFieldNotSupported { + object: PathBuf::from("/var/www/app"), + field: "context", + platform: "macOS", + }; + let msg = err.to_string(); + assert!(msg.contains("'context'")); + assert!(msg.contains("/var/www/app")); + assert!(msg.contains("not supported on macOS")); + } + #[test] fn unconfined_selinux_rejected_display() { let err = InstallerError::UnconfinedSelinuxRejected { type_: "unconfined_t".to_string() }; diff --git a/src/lifecycle_event/script.rs b/src/lifecycle_event/script.rs index bb0a94d..970aa20 100644 --- a/src/lifecycle_event/script.rs +++ b/src/lifecycle_event/script.rs @@ -32,7 +32,8 @@ const SIGKILL_GRACE: Duration = Duration::from_secs(5); /// profile suppression). #[derive(Debug, Clone, Copy, Default)] pub struct HookEnvPolicy { - /// Strip `LD_PRELOAD`/`LD_LIBRARY_PATH`/`LD_AUDIT` from the hook env. + /// Strip the dynamic-loader env vars (`LD_*` on Linux, `DYLD_*` on + /// macOS; see `system::DYNAMIC_LOADER_ENV_VARS`) from the hook env. pub strip_loader_vars: bool, /// Clear the inherited env and rebuild from shell basics + deployment vars. pub restrict_to_allowlist: bool, @@ -166,7 +167,7 @@ impl Script { // Strip loader vars last, so an AppSpec `environment:` entry cannot re-add them. #[cfg(unix)] if self.env_policy.strip_loader_vars { - for var in ["LD_PRELOAD", "LD_LIBRARY_PATH", "LD_AUDIT"] { + for var in crate::system::DYNAMIC_LOADER_ENV_VARS { cmd.env_remove(var); } } @@ -309,12 +310,12 @@ pub(super) fn build_command( match (runas, sudo) { (Some(user), true) => { let mut cmd = Command::new("sudo"); - cmd.args(["su", user, "-c", &script_path.display().to_string()]); + cmd.args(["su", user, "-c", &su_command_arg(script_path)]); cmd }, (Some(user), false) => { let mut cmd = Command::new("su"); - cmd.args([user, "-c", &script_path.display().to_string()]); + cmd.args([user, "-c", &su_command_arg(script_path)]); cmd }, (None, true) => { @@ -326,6 +327,24 @@ pub(super) fn build_command( } } +/// The argument `su -c` hands to the target user's shell. A plain path is +/// passed as-is (hooks may read `BASH_EXECUTION_STRING` and expect it to +/// equal the bare path); a path with a space or shell metacharacter is +/// single-quoted so the shell runs that file instead of parsing the path. +#[cfg(unix)] +fn su_command_arg(script_path: &Path) -> String { + let path = script_path.display().to_string(); + let plain = path.bytes().all(|b| { + b.is_ascii_alphanumeric() + || matches!(b, b'/' | b'.' | b'_' | b'-' | b'+' | b'@' | b':' | b'=') + }); + if plain { + path + } else { + format!("'{}'", path.replace('\'', "'\\''")) + } +} + /// Build the command for script execution (Windows). /// /// `runas`/`sudo` are not supported on Windows. The AppSpec validator rejects @@ -454,9 +473,13 @@ mod tests { use std::time::Duration; use tempfile::{NamedTempFile, TempDir}; - fn sample_log() -> Arc> { - let file = NamedTempFile::new().unwrap(); - Arc::new(Mutex::new(ScriptRunLog::open(file.path()).unwrap())) + // Own the parent dir: ScriptRunLog::open chmods it, which is EPERM on + // macOS's root-owned $TMPDIR. + fn sample_log() -> (TempDir, Arc>) { + let dir = TempDir::new().unwrap(); + let log = + Arc::new(Mutex::new(ScriptRunLog::open(&dir.path().join("scripts.log")).unwrap())); + (dir, log) } #[tokio::test] @@ -467,8 +490,8 @@ mod tests { // 64 MiB rotation cap. Asserted on the FILE (the in-memory diagnostics // buffer is byte-capped and only keeps the tail, so it can't show the // chunk count). - let file = NamedTempFile::new().unwrap(); - let path = file.path().to_path_buf(); + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scripts.log"); let log = Arc::new(Mutex::new(ScriptRunLog::open(&path).unwrap())); let blob = vec![b'A'; MAX_LOG_LINE_BYTES * 3 + 100]; // ~192 KiB, no '\n' stream_to_log(Some(std::io::Cursor::new(blob)), &log, "[stdout]").await; @@ -493,7 +516,7 @@ mod tests { #[tokio::test] async fn stream_to_log_preserves_normal_lines() { - let log = sample_log(); + let (_dir, log) = sample_log(); let input = b"first line\nsecond line\n".to_vec(); stream_to_log(Some(std::io::Cursor::new(input)), &log, "[stdout]").await; let entries = log.lock().unwrap().entries(); @@ -504,6 +527,31 @@ mod tests { // --- build_command --- + #[cfg(unix)] + #[test] + fn su_command_arg_passes_plain_paths_unchanged() { + assert_eq!( + su_command_arg(std::path::Path::new( + "/opt/codedeploy-agent/deployment-root/d-1/deployment-archive/scripts/start.sh" + )), + "/opt/codedeploy-agent/deployment-root/d-1/deployment-archive/scripts/start.sh" + ); + } + + #[cfg(unix)] + #[test] + fn su_command_arg_quotes_paths_with_shell_characters() { + assert_eq!( + su_command_arg(std::path::Path::new("/tmp/my app/run.sh")), + "'/tmp/my app/run.sh'" + ); + assert_eq!(su_command_arg(std::path::Path::new("/tmp/a;id/run.sh")), "'/tmp/a;id/run.sh'"); + assert_eq!( + su_command_arg(std::path::Path::new("/tmp/it's/run.sh")), + "'/tmp/it'\\''s/run.sh'" + ); + } + #[cfg(unix)] #[test] fn build_command_no_runas_no_sudo() { @@ -629,13 +677,9 @@ mod tests { let mut env = HashMap::new(); env.insert("KEY".to_string(), "VAL".to_string()); - let script = Script::new( - "/test/script.sh".into(), - Some("deploy".to_string()), - true, - &env, - sample_log(), - ); + let (_dir, log) = sample_log(); + let script = + Script::new("/test/script.sh".into(), Some("deploy".to_string()), true, &env, log); let debug = format!("{script:?}"); assert!(debug.contains("script.sh")); @@ -645,12 +689,13 @@ mod tests { #[test] fn new_no_runas() { + let (_dir, log) = sample_log(); let script = Script::new( "/test/script.sh".into(), None, false, &HashMap::::new(), - sample_log(), + log, ); let debug = format!("{script:?}"); @@ -662,12 +707,13 @@ mod tests { #[test] fn execute_nonexistent_script() { + let (_dir, log) = sample_log(); let script = Script::new( "/nonexistent/script.sh".into(), None, false, &HashMap::::new(), - sample_log(), + log, ); let result = script.execute(Duration::from_secs(10)); @@ -813,6 +859,44 @@ mod tests { ); } + /// Checked on the `Command` (`get_envs()`), not in a child: on macOS SIP + /// scrubs `DYLD_*` before `/bin/sh` runs, so a child-side check proves nothing. + #[cfg(unix)] + #[test] + fn strip_loader_vars_marks_platform_loader_vars_removed_on_command() { + use crate::system::DYNAMIC_LOADER_ENV_VARS; + + let dir = TempDir::new().unwrap(); + let log = Arc::new(Mutex::new(ScriptRunLog::open(&dir.path().join("log")).unwrap())); + let mut env_vars = HashMap::::new(); + for var in DYNAMIC_LOADER_ENV_VARS { + env_vars.insert((*var).to_string(), "/tmp/evil".to_string()); + } + let script = Script::with_env_policy( + dir.path().join("noop.sh"), + None, + false, + &env_vars, + HookEnvPolicy { + strip_loader_vars: true, + restrict_to_allowlist: false, + disable_powershell_profile: false, + }, + log, + ); + + let mut cmd = Command::new("true"); + script.apply_env_policy(&mut cmd); + + for var in DYNAMIC_LOADER_ENV_VARS { + let entry = cmd.as_std().get_envs().find(|(k, _)| *k == std::ffi::OsStr::new(var)); + assert!( + matches!(entry, Some((_, None))), + "{var} must be marked removed on the Command, got {entry:?}" + ); + } + } + /// The three AWS credential-override names must never reach the hook, even /// under the default full-inheritance policy. #[cfg(unix)] diff --git a/src/main.rs b/src/main.rs index cbe703a..9bc1f70 100644 --- a/src/main.rs +++ b/src/main.rs @@ -682,6 +682,13 @@ const VALID_FILE_EXISTS_BEHAVIORS: &[&str] = &["DISALLOW", "OVERWRITE", "RETAIN" fn run_update(config: &AgentConfig) { use codedeploy_agent::host_command::commands::UpdateAgentCommand; + // Refuse unsupported platforms (macOS) before touching + // credentials, IMDS or S3 — same shape as the Windows branch above. + if let Err(e) = UpdateAgentCommand::check_platform() { + eprintln!("{e}"); + process::exit(1); + } + // Resolve credentials + region exactly as the worker / S3 deploy path do: // on-premises config region first, then AWS_REGION env / IMDS. let mut credentials = match Credentials::load(&config.on_premises_config_file) { diff --git a/src/runtime/file_based_deployment_tracker.rs b/src/runtime/file_based_deployment_tracker.rs index 941c711..09e15c5 100644 --- a/src/runtime/file_based_deployment_tracker.rs +++ b/src/runtime/file_based_deployment_tracker.rs @@ -610,7 +610,8 @@ mod edge_case_tests { ); } - #[cfg(unix)] + // APFS (macOS) rejects non-UTF-8 filenames with EILSEQ; the invalid-name path is only reachable on Linux filesystems. + #[cfg(target_os = "linux")] #[test] fn get_active_deployment_invalid_filename() { use std::ffi::OsStr; diff --git a/src/system/env_ops.rs b/src/system/env_ops.rs index 0c211b2..b09559b 100644 --- a/src/system/env_ops.rs +++ b/src/system/env_ops.rs @@ -1,4 +1,21 @@ //! Testable environment variable access. + +/// Dynamic-loader variables that can inject shared libraries into a child +/// process; stripped from the self-update script and (opt-in) from hooks. +/// glibc reads `LD_*`, dyld reads `DYLD_*` and ignores `LD_*`. +#[cfg(target_os = "macos")] +pub const DYNAMIC_LOADER_ENV_VARS: &[&str] = &[ + "DYLD_INSERT_LIBRARIES", + "DYLD_LIBRARY_PATH", + "DYLD_FRAMEWORK_PATH", + "DYLD_FALLBACK_LIBRARY_PATH", + "DYLD_FALLBACK_FRAMEWORK_PATH", + "DYLD_VERSIONED_LIBRARY_PATH", + "DYLD_VERSIONED_FRAMEWORK_PATH", +]; +#[cfg(all(unix, not(target_os = "macos")))] +pub const DYNAMIC_LOADER_ENV_VARS: &[&str] = &["LD_PRELOAD", "LD_LIBRARY_PATH", "LD_AUDIT"]; + /// Environment variable access abstraction. /// /// Production code uses `SystemEnvOps` which reads real env vars. @@ -51,6 +68,24 @@ mod mock { mod tests { use super::*; + #[cfg(target_os = "macos")] + #[test] + fn loader_vars_are_dyld_names_on_macos() { + assert!(DYNAMIC_LOADER_ENV_VARS.iter().all(|v| v.starts_with("DYLD_"))); + assert!(DYNAMIC_LOADER_ENV_VARS.contains(&"DYLD_INSERT_LIBRARIES")); + assert!(DYNAMIC_LOADER_ENV_VARS.contains(&"DYLD_FALLBACK_LIBRARY_PATH")); + assert!(DYNAMIC_LOADER_ENV_VARS.contains(&"DYLD_FALLBACK_FRAMEWORK_PATH")); + assert!(DYNAMIC_LOADER_ENV_VARS.contains(&"DYLD_VERSIONED_LIBRARY_PATH")); + assert!(DYNAMIC_LOADER_ENV_VARS.contains(&"DYLD_VERSIONED_FRAMEWORK_PATH")); + } + + #[cfg(all(unix, not(target_os = "macos")))] + #[test] + fn loader_vars_are_ld_names_on_linux() { + assert!(DYNAMIC_LOADER_ENV_VARS.iter().all(|v| v.starts_with("LD_"))); + assert!(DYNAMIC_LOADER_ENV_VARS.contains(&"LD_PRELOAD")); + } + #[test] fn system_env_reads_path() { let ops = SystemEnvOps; diff --git a/src/system/file_ops.rs b/src/system/file_ops.rs index 89039cf..9f2e3db 100644 --- a/src/system/file_ops.rs +++ b/src/system/file_ops.rs @@ -18,6 +18,15 @@ pub trait PlatformFileOperations: Send + Sync { fn write_with_retry(&self, path: &Path, content: &str) -> io::Result<()>; } +/// Build a `Mode` from `u32` permission bits. `mode_t` is `u32` on Linux but +/// `u16` on macOS, so mask to the permission bits (`0o7777`) before the cast. +#[cfg(unix)] +#[must_use] +#[allow(clippy::unnecessary_cast, clippy::cast_possible_truncation)] // width of mode_t is platform-specific +pub fn mode_from_bits(bits: u32) -> nix::sys::stat::Mode { + nix::sys::stat::Mode::from_bits_truncate((bits & 0o7777) as nix::libc::mode_t) +} + /// If the file has no execute bits, add them. Mirrors /// `script_executable?` check + `make_executable` fallback. /// @@ -262,6 +271,14 @@ mod safe_relative_path_tests { #[cfg(test)] mod tests { use super::*; + + #[cfg(unix)] + #[test] + fn mode_from_bits_keeps_permission_bits_and_drops_file_type() { + let mode = mode_from_bits(0o100_644); + assert_eq!(mode, nix::sys::stat::Mode::from_bits_truncate(0o644)); + assert_eq!(mode_from_bits(0o4755), nix::sys::stat::Mode::from_bits_truncate(0o4755)); + } use std::path::PathBuf; use tempfile::TempDir; diff --git a/src/system/mod.rs b/src/system/mod.rs index 1709e69..fe7d1aa 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -1,31 +1,37 @@ //! System abstraction layer — file ops, process ops, `SELinux`, env vars. +//! +//! `linux_ops` (setfacl) and `selinux_ops` (semanage/restorecon) shell out to +//! Linux-only tools, so they are gated on `target_os = "linux"`, not `unix`; +//! macOS takes the rejection path in `installer::builder` instead. pub mod env_ops; pub mod file_ops; -#[cfg(unix)] +#[cfg(target_os = "linux")] pub mod linux_ops; pub mod process_ops; pub mod secure_files; -#[cfg(unix)] +#[cfg(target_os = "linux")] pub mod selinux_ops; pub mod version_file; +#[cfg(unix)] +pub use env_ops::DYNAMIC_LOADER_ENV_VARS; pub use env_ops::{EnvOps, SystemEnvOps}; pub use file_ops::{PlatformFileOperations, SystemFileOperations, ensure_executable}; -#[cfg(unix)] +#[cfg(target_os = "linux")] 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, }; -#[cfg(unix)] +#[cfg(target_os = "linux")] pub use selinux_ops::{SeLinuxOps, SystemSeLinuxOps}; #[cfg(test)] pub use env_ops::MockEnvOps; #[cfg(test)] pub use file_ops::MockFileOperations; -#[cfg(all(test, unix))] +#[cfg(all(test, target_os = "linux"))] pub use linux_ops::MockLinuxOps; -#[cfg(all(test, unix))] +#[cfg(all(test, target_os = "linux"))] pub use selinux_ops::MockSeLinuxOps; diff --git a/tests/security/intake/appspec_validation.rs b/tests/security/intake/appspec_validation.rs index 36c61a0..4729978 100644 --- a/tests/security/intake/appspec_validation.rs +++ b/tests/security/intake/appspec_validation.rs @@ -86,7 +86,8 @@ fn mode_suid_bits_are_detectable() { // --------------------------------------------------------------------------- /// Parser accepts unconfined types; install-time rejection happens in `ChangeContextCommand`. -#[cfg(unix)] +// ChangeContextCommand is cfg(target_os = "linux") (semanage is Linux-only). +#[cfg(target_os = "linux")] #[test] fn appspec_rejects_unconfined_selinux_context() { use codedeploy_agent::installer::InstallerError; diff --git a/tests/security/intake/installer_permissions.rs b/tests/security/intake/installer_permissions.rs index 1b807c4..f5183ae 100644 --- a/tests/security/intake/installer_permissions.rs +++ b/tests/security/intake/installer_permissions.rs @@ -205,7 +205,9 @@ fn privilege_separation_is_documented() { // --------------------------------------------------------------------------- /// `ChangeContextCommand` rejects unconfined types when the toggle is on. +// setfacl/semanage are Linux-only; the commands are cfg(target_os = "linux"). #[test] +#[cfg(target_os = "linux")] fn change_context_rejects_unconfined() { use codedeploy_agent::application_specification::AppSpec; use codedeploy_agent::installer::InstallerError; @@ -253,7 +255,9 @@ fn change_context_rejects_unconfined() { /// Backwards-compatible default: unconfined types reach `semanage` when the /// toggle is off. /// Accepts `Ok` (semanage present and ran) or any non-`UnconfinedSelinuxRejected` error. +// setfacl/semanage are Linux-only; the commands are cfg(target_os = "linux"). #[test] +#[cfg(target_os = "linux")] fn unconfined_selinux_allowed_when_rejection_disabled() { use codedeploy_agent::application_specification::AppSpec; use codedeploy_agent::installer::InstallerError; @@ -470,7 +474,9 @@ fn change_mode_rejects_symlink_target() { /// A symlinked permission target must be rejected by ChangeContextCommand /// before the `canonicalize`/`semanage` path (which resolves the link). +// setfacl/semanage are Linux-only; the commands are cfg(target_os = "linux"). #[test] +#[cfg(target_os = "linux")] fn change_context_rejects_symlink_target() { use codedeploy_agent::application_specification::AppSpec; use codedeploy_agent::installer::InstallerError; @@ -500,7 +506,9 @@ fn change_context_rejects_symlink_target() { /// A symlinked permission target must be rejected by ChangeAclCommand rather /// than followed (setfacl redirected onto the link target). +// setfacl/semanage are Linux-only; the commands are cfg(target_os = "linux"). #[test] +#[cfg(target_os = "linux")] fn change_acl_rejects_symlink_target() { use codedeploy_agent::application_specification::AppSpec; use codedeploy_agent::installer::InstallerError; @@ -596,6 +604,8 @@ proptest! { // --------------------------------------------------------------------------- // Every blocklist member is rejected across randomised target paths. +// setfacl/semanage are Linux-only; ChangeContextCommand is cfg(target_os = "linux"). +#[cfg(target_os = "linux")] proptest! { #![proptest_config(ProptestConfig::with_cases(100))] diff --git a/tests/security/intake/process_isolation.rs b/tests/security/intake/process_isolation.rs index f4d8e71..ae1540b 100644 --- a/tests/security/intake/process_isolation.rs +++ b/tests/security/intake/process_isolation.rs @@ -183,8 +183,9 @@ fn infinite_loop_script_terminated_at_timeout() { assert!(elapsed >= Duration::from_secs(1), "Timeout fired too early: {elapsed:?}"); } -// Validates: A script trapping SIGTERM (trap '' TERM; sleep infinity) is eventually killed +// Validates: A script trapping SIGTERM (trap '' TERM; sleep 3600) is eventually killed // via SIGKILL escalation after a grace period, preventing unkillable zombie processes. +// `sleep infinity` is GNU-only; BSD sleep (macOS) rejects it. #[cfg(unix)] #[test] fn sigterm_trapping_script_killed_by_sigkill() { @@ -197,7 +198,7 @@ fn sigterm_trapping_script_killed_by_sigkill() { std::fs::write( &script_path, - format!("#!/bin/sh\ntrap '' TERM\necho $$ > {}\nsleep infinity\n", pid_file.display()), + format!("#!/bin/sh\ntrap '' TERM\necho $$ > {}\nsleep 3600\n", pid_file.display()), ) .expect("write script"); std::fs::set_permissions(&script_path, std::fs::Permissions::from_mode(0o755)) @@ -411,44 +412,47 @@ fn script_path_does_not_contain_dangerous_directories() { } } -// Validates: with the opt-in strip_loader_vars policy, LD_PRELOAD, LD_LIBRARY_PATH -// and LD_AUDIT are absent from the lifecycle script environment, preventing shared -// library injection attacks — even when supplied via the AppSpec env map. -#[cfg(unix)] +// Validates: with the opt-in strip_loader_vars policy, the platform's dynamic +// loader variables (LD_PRELOAD/LD_LIBRARY_PATH/LD_AUDIT on Linux, +// DYLD_INSERT_LIBRARIES/DYLD_LIBRARY_PATH/DYLD_FRAMEWORK_PATH on macOS) are +// absent from the lifecycle script environment, preventing shared library +// injection attacks — even when supplied via the AppSpec env map. +// +// Linux-only: on macOS SIP scrubs DYLD_* before /bin/sh runs, so this check +// would pass regardless. `script::tests::strip_loader_vars_marks_platform_ +// loader_vars_removed_on_command` covers every Unix via `Command::get_envs()`. +#[cfg(target_os = "linux")] #[test] fn ld_preload_and_ld_library_path_not_in_script_env() { + use codedeploy_agent::system::DYNAMIC_LOADER_ENV_VARS; use std::os::unix::fs::PermissionsExt; let dir = tempfile::TempDir::new().expect("create tempdir"); let script_path = dir.path().join("check_ld.sh"); let output_file = dir.path().join("ld_output.txt"); - std::fs::write( - &script_path, - format!( - "#!/bin/sh\n\ - echo \"LD_PRELOAD=${{LD_PRELOAD:-UNSET}}\" > {out}\n\ - echo \"LD_LIBRARY_PATH=${{LD_LIBRARY_PATH:-UNSET}}\" >> {out}\n\ - echo \"LD_AUDIT=${{LD_AUDIT:-UNSET}}\" >> {out}\n", + // One `NAME=${NAME:-UNSET}` line per platform loader var. + let mut body = String::from("#!/bin/sh\n"); + for (i, var) in DYNAMIC_LOADER_ENV_VARS.iter().enumerate() { + let redirect = if i == 0 { ">" } else { ">>" }; + body.push_str(&format!( + "echo \"{var}=${{{var}:-UNSET}}\" {redirect} {out}\n", out = output_file.display() - ), - ) - .expect("write script"); + )); + } + std::fs::write(&script_path, body).expect("write script"); std::fs::set_permissions(&script_path, std::fs::Permissions::from_mode(0o755)) .expect("set permissions"); let log = Arc::new(Mutex::new(ScriptRunLog::open(&dir.path().join("log")).expect("create log"))); - // Pass LD_PRELOAD/LD_LIBRARY_PATH/LD_AUDIT via the AppSpec environment-variable - // HashMap — these must be stripped even when supplied directly by the customer - // bundle, not only when inherited from the agent's own environment. Stripping - // must therefore happen after the `env_vars` entries are applied to the - // command, otherwise those entries silently re-add the variables. + // Supply the loader vars via the AppSpec env map: they must be stripped + // even when the bundle sets them, not only when inherited from the agent. let mut env_vars = HashMap::::new(); - env_vars.insert("LD_PRELOAD".to_string(), "/tmp/evil.so".to_string()); - env_vars.insert("LD_LIBRARY_PATH".to_string(), "/tmp/evil-libs".to_string()); - env_vars.insert("LD_AUDIT".to_string(), "/tmp/evil-audit.so".to_string()); + for var in DYNAMIC_LOADER_ENV_VARS { + env_vars.insert((*var).to_string(), "/tmp/evil".to_string()); + } let script = Script::with_env_policy(script_path, None, false, &env_vars, HARDENED, log); @@ -457,18 +461,12 @@ fn ld_preload_and_ld_library_path_not_in_script_env() { let output = std::fs::read_to_string(&output_file).expect("read output file"); - assert!( - output.contains("LD_PRELOAD=UNSET"), - "LD_PRELOAD must be stripped even when supplied via AppSpec env_vars. Got: {output}" - ); - assert!( - output.contains("LD_LIBRARY_PATH=UNSET"), - "LD_LIBRARY_PATH must be stripped even when supplied via AppSpec env_vars. Got: {output}" - ); - assert!( - output.contains("LD_AUDIT=UNSET"), - "LD_AUDIT must be stripped even when supplied via AppSpec env_vars. Got: {output}" - ); + for var in DYNAMIC_LOADER_ENV_VARS { + assert!( + output.contains(&format!("{var}=UNSET")), + "{var} must be stripped even when supplied via AppSpec env_vars. Got: {output}" + ); + } } // Validates: IFS (Internal Field Separator) in the script environment is the default From 4575a113caf67d4d2e00f673b4e0af7d05d53b3a Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Tue, 13 Oct 2026 08:37:04 +0000 Subject: [PATCH 04/13] Loosen wall-clock upper bounds in timing-sensitive tests Raise the elapsed-time upper bounds that flaked on loaded runners, and test the throttle jitter bound directly instead. --- src/aws_clients/throttle_gate.rs | 13 +++++++++++-- src/command_poller/host_command_poller.rs | 2 +- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/aws_clients/throttle_gate.rs b/src/aws_clients/throttle_gate.rs index 5cd3017..28a6228 100644 --- a/src/aws_clients/throttle_gate.rs +++ b/src/aws_clients/throttle_gate.rs @@ -164,7 +164,7 @@ mod tests { let gate = ThrottleGate::new(); let start = Instant::now(); gate.wait_if_throttled(); - assert!(start.elapsed() < Duration::from_millis(50)); + assert!(start.elapsed() < Duration::from_millis(500)); } #[test] @@ -179,7 +179,16 @@ mod tests { gate.wait_if_throttled(); // Wait is 100ms + up to 25ms jitter assert!(start.elapsed() >= Duration::from_millis(90)); - assert!(start.elapsed() < Duration::from_millis(200)); + assert!(start.elapsed() < Duration::from_millis(750)); + } + + #[test] + fn jitter_is_at_most_quarter() { + let base = Duration::from_millis(100); + for _ in 0..1000 { + assert!(ThrottleGate::jitter(base) < Duration::from_millis(25)); + } + assert_eq!(ThrottleGate::jitter(Duration::from_millis(3)), Duration::ZERO); } #[test] diff --git a/src/command_poller/host_command_poller.rs b/src/command_poller/host_command_poller.rs index 28488ff..b2615e7 100644 --- a/src/command_poller/host_command_poller.rs +++ b/src/command_poller/host_command_poller.rs @@ -647,7 +647,7 @@ mod tests { let pool = CommandThreadPool::new(16); let start = Instant::now(); pool.wait_for_completion(Duration::from_secs(5)); - assert!(start.elapsed() < Duration::from_millis(200)); + assert!(start.elapsed() < Duration::from_secs(1)); } #[test] From afcccfb95fd1b416c766488b57ce5a0595af84f9 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Tue, 13 Oct 2026 08:37:04 +0000 Subject: [PATCH 05/13] Document the errors set_permissions returns --- src/installer/builder.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/installer/builder.rs b/src/installer/builder.rs index fa07eb9..cb6cc53 100644 --- a/src/installer/builder.rs +++ b/src/installer/builder.rs @@ -205,6 +205,14 @@ impl CommandBuilder { } } + /// Queue the mode, ACL, `SELinux` context and owner changes `permission` + /// asks for on `object`. + /// + /// # Errors + /// + /// Returns [`InstallerError::DuplicatePermission`] if `object` already has + /// permissions queued, and [`InstallerError::PermissionFieldNotSupported`] + /// if `permission` uses a field this platform cannot apply. pub fn set_permissions(&mut self, object: &Path, permission: &Permission) -> Result<()> { debug!("Setting permissions on {}", object.display()); From 67698957fbb09bafa742c1d8e8c3013026d00537 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Tue, 13 Oct 2026 08:37:04 +0000 Subject: [PATCH 06/13] Exit the macOS worker when its master dies On Linux the worker uses PR_SET_PDEATHSIG to die with its master. macOS has no parent-death signal, and launchd considers the job stopped once the master exits, so a restart would start a second master and worker next to the orphaned worker. Watch the master's pid with kqueue (EVFILT_PROC / NOTE_EXIT) on a background thread and exit when it fires. --- Cargo.toml | 2 +- launchd/com.amazon.aws.codedeploy-agent.plist | 5 +- src/daemon/worker.rs | 129 +++++++++++++++++- src/main.rs | 4 + 4 files changed, 135 insertions(+), 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 91aca3a..7ff6855 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,7 +53,7 @@ subtle = "2" strip-ansi-escapes = "0.2" [target.'cfg(unix)'.dependencies] -nix = { version = "0.29", features = ["user", "fs", "signal", "process", "resource"] } +nix = { version = "0.29", features = ["user", "fs", "signal", "process", "resource", "event"] } signal-hook = "0.3" [target.'cfg(windows)'.dependencies] diff --git a/launchd/com.amazon.aws.codedeploy-agent.plist b/launchd/com.amazon.aws.codedeploy-agent.plist index ddb43e8..5aa2b75 100644 --- a/launchd/com.amazon.aws.codedeploy-agent.plist +++ b/launchd/com.amazon.aws.codedeploy-agent.plist @@ -34,8 +34,9 @@ the agent's logs but not this file. Stop is `launchctl bootout` (SIGTERM to the agent; daemon/signal.rs). No - KillMode=control-group equivalent: launchd SIGKILLs the job's process group - when the master exits (covers workers); hooks run in their own group and + KillMode=control-group equivalent: launchd treats the job as stopped once + the master exits, so the worker watches the master's pid (kqueue NOTE_EXIT, + daemon/worker.rs) and exits with it. Hooks run in their own group and are orphaned only if the master is SIGKILLed at ExitTimeOut mid-hook. --> diff --git a/src/daemon/worker.rs b/src/daemon/worker.rs index 55da967..2a051b3 100644 --- a/src/daemon/worker.rs +++ b/src/daemon/worker.rs @@ -329,14 +329,139 @@ pub fn bind_lifetime_to_parent() { } } -/// No-op on non-Linux: `PR_SET_PDEATHSIG` is Linux-specific. -#[cfg(not(target_os = "linux"))] +/// macOS counterpart of the Linux `PR_SET_PDEATHSIG` binding. macOS has no +/// parent-death signal, and launchd has no control group that holds a stop open +/// until every process exits: once the master is gone launchd considers the job +/// stopped, and a restart starts a second master and worker next to a worker +/// that is still running. +/// +/// Watch the master's pid with kqueue (`EVFILT_PROC` / `NOTE_EXIT`) on a +/// background thread and exit the moment it fires, however the master died. +/// The window between `getppid()` and registering the watch is closed the same +/// way as on Linux: `ESRCH` from the registration, or a parent that is no +/// longer the one we registered, means the master is already gone. +#[cfg(target_os = "macos")] +pub fn bind_lifetime_to_parent() { + use nix::errno::Errno; + use nix::unistd::{Pid, getppid}; + + let parent = getppid(); + if parent == Pid::from_raw(1) { + info!("Master already exited before worker startup; exiting to avoid orphan"); + process::exit(0); + } + let kq = match watch_process_exit(parent) { + Ok(kq) => kq, + Err(Errno::ESRCH) => { + info!("Master already exited before worker startup; exiting to avoid orphan"); + process::exit(0); + }, + Err(e) => { + error!("Worker failed to watch master pid {parent}; master death may orphan it: {e}"); + return; + }, + }; + if getppid() != parent { + info!("Master already exited before worker startup; exiting to avoid orphan"); + process::exit(0); + } + let spawned = std::thread::Builder::new().name("master-watch".into()).spawn(move || { + match wait_for_process_exit(&kq) { + Ok(()) => { + error!("Master (pid {parent}) exited; worker exiting to avoid running orphaned"); + // Give the non-blocking log writer a moment to flush that line; + // process::exit runs no destructors. + std::thread::sleep(std::time::Duration::from_millis(200)); + process::exit(1); + }, + Err(e) => { + error!("Watch on master pid {parent} failed; master death may orphan it: {e}"); + }, + } + }); + if let Err(e) = spawned { + error!("Worker failed to start the master watch thread; master death may orphan it: {e}"); + } +} + +/// Register a one-shot kqueue watch for `pid`'s exit. Returns `ESRCH` when the +/// process no longer exists. +#[cfg(target_os = "macos")] +fn watch_process_exit(pid: nix::unistd::Pid) -> nix::Result { + use nix::sys::event::{EventFilter, EventFlag, FilterFlag, KEvent, Kqueue}; + + let ident = usize::try_from(pid.as_raw()).map_err(|_| nix::errno::Errno::EINVAL)?; + let kq = Kqueue::new()?; + let watch = KEvent::new( + ident, + EventFilter::EVFILT_PROC, + EventFlag::EV_ADD | EventFlag::EV_ONESHOT, + FilterFlag::NOTE_EXIT, + 0, + 0, + ); + kq.kevent(&[watch], &mut [], None)?; + Ok(kq) +} + +/// Block until the watch registered by [`watch_process_exit`] fires. +#[cfg(target_os = "macos")] +fn wait_for_process_exit(kq: &nix::sys::event::Kqueue) -> nix::Result<()> { + use nix::errno::Errno; + use nix::sys::event::{EventFilter, EventFlag, FilterFlag, KEvent}; + + let mut fired = [KEvent::new( + 0, + EventFilter::EVFILT_PROC, + EventFlag::empty(), + FilterFlag::empty(), + 0, + 0, + )]; + loop { + match kq.kevent(&[], &mut fired, None) { + Ok(0) | Err(Errno::EINTR) => {}, + Ok(_) => return Ok(()), + Err(e) => return Err(e), + } + } +} + +/// No-op on other targets (Windows): neither mechanism above exists there. +#[cfg(not(any(target_os = "linux", target_os = "macos")))] pub fn bind_lifetime_to_parent() {} #[cfg(test)] mod tests { use super::*; + /// The kqueue watch the macOS worker puts on its master fires when the + /// watched process exits, so a worker cannot outlive its master. + #[cfg(target_os = "macos")] + #[test] + fn process_exit_watch_fires_when_watched_process_dies() { + let mut child = process::Command::new("sleep").arg("60").spawn().expect("spawn sleep"); + let pid = nix::unistd::Pid::from_raw(i32::try_from(child.id()).expect("pid fits i32")); + let kq = watch_process_exit(pid).expect("register exit watch"); + + child.kill().expect("kill sleep"); + + assert_eq!(wait_for_process_exit(&kq), Ok(())); + child.wait().expect("reap sleep"); + } + + /// A master that is already gone is reported as `ESRCH`, so the worker + /// exits instead of waiting on a watch that can never fire. + #[cfg(target_os = "macos")] + #[test] + fn process_exit_watch_reports_esrch_for_a_gone_process() { + let mut child = process::Command::new("true").spawn().expect("spawn true"); + let pid = nix::unistd::Pid::from_raw(i32::try_from(child.id()).expect("pid fits i32")); + child.wait().expect("reap true"); + + assert_eq!(watch_process_exit(pid).err(), Some(nix::errno::Errno::ESRCH)); + } + /// The worker must reset the umask to 0o022 so no-stored-mode customer files /// land at 0644. #[cfg(unix)] diff --git a/src/main.rs b/src/main.rs index 9bc1f70..e7d033c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -603,6 +603,10 @@ fn run(command: &Command, config_file: Option<&Path>) { eprintln!("Failed to register signal handlers: {e}"); process::exit(1); } + // Ensure this worker dies with the master, however the master + // dies (e.g. by SIGKILL, which runs no graceful shutdown), so it + // can't be orphaned: on Linux it would hold the systemd service + // cgroup open, on macOS it would run next to a restarted agent. worker::bind_lifetime_to_parent(); worker::run(&shutdown, &config); }, From 5a8b7ca51aed58beb041499edfdc1b41d3977636 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Tue, 13 Oct 2026 08:38:22 +0000 Subject: [PATCH 07/13] Write each script log line to the deployment log as it is produced Write each scripts.log line to the deployment log when it is produced, as "[] []". Previously the hook command copied the 2048-byte diagnostic tail into the log after each lifecycle event, which dropped the deployment ID, doubled the timestamp, added a blank line per record, and lost every line older than the tail. All deployments now share one deployment-log handle, so concurrent deployments no longer rotate the file independently. The failure-summary line is removed; the log holds only script log lines. Blank script output lines are now logged as a bare "[stdout]" record in both files; they were previously dropped. The deployment log gets the same ANSI-stripped line as scripts.log. --- src/host_command/commands/hook.rs | 254 ++++++++++++++++---------- src/lifecycle_event/executor.rs | 24 ++- src/lifecycle_event/script.rs | 79 +++++++- src/lifecycle_event/script_run_log.rs | 42 +++++ src/logging/deployment_logger.rs | 134 +++++++++++++- src/logging/mod.rs | 2 +- 6 files changed, 420 insertions(+), 115 deletions(-) diff --git a/src/host_command/commands/hook.rs b/src/host_command/commands/hook.rs index 1df99e9..5cfaef6 100644 --- a/src/host_command/commands/hook.rs +++ b/src/host_command/commands/hook.rs @@ -6,7 +6,7 @@ use crate::deployment_specification::types::DeploymentSpec; use crate::host_command::DeploymentArchives; use crate::lifecycle_event::{HookEnvPolicy, LifecycleEventExecutor, ScriptError}; -use crate::logging::{DeploymentLogger, LogConfig}; +use crate::logging::{LogConfig, SharedDeploymentLog}; use std::collections::HashMap; use std::sync::Arc; use tracing::{info, warn}; @@ -18,11 +18,12 @@ pub type HookMapping = HashMap>; pub struct HookCommand { archives: Arc, hook_mapping: HookMapping, - /// When set, per-deployment lifecycle/script log lines are mirrored to + /// When set, every lifecycle/script log line is also written to /// `/deployment-logs/-deployments.log` (the /// documented `CodeDeploy` deployment log). `None` disables it - /// (`enable_deployments_log: false`, and the default in tests). - deployment_log_config: Option, + /// (`enable_deployments_log: false`, and the default in tests). One handle + /// is shared by all deployments. + deployment_log: Option, /// Opt-in hook-env hardening, from `strip_loader_env_in_hooks` / /// `restrict_hook_env_to_allowlist`. Default (all-`false`) = full inheritance. env_policy: HookEnvPolicy, @@ -42,7 +43,7 @@ impl HookCommand { Self { archives, hook_mapping, - deployment_log_config: None, + deployment_log: None, env_policy: HookEnvPolicy::default(), reject_path_traversal: false, restrict_log_permissions: false, @@ -80,7 +81,7 @@ impl HookCommand { /// `LogConfig`'s `root_dir`/`program_name` determine the log location. #[must_use] pub fn with_deployment_log_config(mut self, config: LogConfig) -> Self { - self.deployment_log_config = Some(config); + self.deployment_log = Some(SharedDeploymentLog::new(config)); self } @@ -122,56 +123,27 @@ impl HookCommand { "Executing hook command" ); - // Open the per-deployment log up front (best-effort) so the + // Open the deployment log up front (best-effort) so the // `deployment-logs/` directory and `…-deployments.log` file exist for // the deployment even before the first line is written. A failure here // must not fail the deployment — it's a diagnostic side-channel. - let mut deployment_log = - self.deployment_log_config - .as_ref() - .and_then(|cfg| match DeploymentLogger::new(cfg) { - Ok(logger) => Some(logger), - Err(e) => { - warn!("Could not open deployment log: {e}"); - None - }, - }); + if let Some(log) = &self.deployment_log + && let Err(e) = log.ensure_open() + { + warn!("Could not open deployment log: {e}"); + } + // Lines reach the deployment log as each one is written to + // `scripts.log`, so a failing event needs no extra writes here and + // the deployment log gets no failure-summary line. let mut all_logs = Vec::new(); for event_name in events { let executor = self.create_executor(event_name, spec)?; - // Run the event; on script failure still flush whatever was logged - // to the deployment log before propagating the error. - let result = executor.execute(); - if let Some(logger) = deployment_log.as_mut() { - match &result { - // Success: the returned lines already include the - // "LifecycleEvent - {event}" header, so just mirror them. - Ok(logs) => Self::mirror_to_deployment_log(logger, logs), - // Failure: `execute()` returns no lines, so the header is - // not in `result`. Write it before the failure summary so - // the deployment log keeps context for the event that failed. - Err(e) => { - let _ = logger.log(&format!("LifecycleEvent - {event_name}")); - let _ = logger - .log(&format!("{command_name}/{event_name} failed: {}", e.message)); - }, - } - } - let logs = result?; - all_logs.extend(logs); + all_logs.extend(executor.execute()?); } Ok(all_logs) } - /// Mirror a lifecycle event's collected log lines to the per-deployment log. - /// Best-effort: write failures are ignored (the log is diagnostic only). - fn mirror_to_deployment_log(logger: &mut DeploymentLogger, lines: &[String]) { - for line in lines { - let _ = logger.log(line); - } - } - /// Check if a command has any lifecycle events mapped. /// /// Pure lookup on `hook_mapping` — does not parse the `AppSpec`. Any @@ -248,6 +220,9 @@ impl HookCommand { .with_env_policy(self.env_policy) .with_reject_path_traversal(self.reject_path_traversal) .with_restrict_log_permissions(self.restrict_log_permissions) + .with_deployment_log( + self.deployment_log.as_ref().map(|log| log.for_deployment(&spec.deployment_id)), + ) }) } } @@ -463,43 +438,105 @@ mod tests { } } - /// When the deployment log is enabled, executing a hook creates - /// `deployment-logs/-deployments.log` and writes the event's - /// log lines to it. + /// Write an `AfterInstall` hook running `script_body` into the archive of + /// `deployment_id`, and return the `HookCommand` spec for it. #[cfg(unix)] - #[test] - fn deployment_log_created_and_written_when_enabled() { + fn stage_after_install( + archives: &DeploymentArchives, + deployment_id: &str, + script_body: &str, + ) -> DeploymentSpec { use std::os::unix::fs::PermissionsExt; - let dir = TempDir::new().unwrap(); - let archives = test_archives(&dir); - // root_dir for the deployment log = the executor's deploy dir parent; - // point LogConfig.root_dir at the deployments root so we can assert. - let root = dir.path().join("deployments"); - let cmd = HookCommand::new(archives.clone(), test_mapping()) - .with_deployment_log_config(log_config_for(&root)); - - // A real AfterInstall hook that succeeds, so execute() returns log lines. - let archive_dir = archives.archive_dir("dg-1", "d-123"); + let archive_dir = archives.archive_dir("dg-1", deployment_id); let scripts = archive_dir.join("scripts"); fs::create_dir_all(&scripts).unwrap(); fs::write( archive_dir.join("appspec.yml"), - "version: 0.0\nos: linux\nhooks:\n AfterInstall:\n - location: scripts/ok.sh\n timeout: 10\n", + "version: 0.0\nos: linux\nhooks:\n AfterInstall:\n - location: scripts/run.sh\n timeout: 30\n", ) .unwrap(); - fs::write(scripts.join("ok.sh"), "#!/bin/sh\necho hi\n").unwrap(); - fs::set_permissions(scripts.join("ok.sh"), fs::Permissions::from_mode(0o755)).unwrap(); + fs::write(scripts.join("run.sh"), format!("#!/bin/sh\n{script_body}")).unwrap(); + fs::set_permissions(scripts.join("run.sh"), fs::Permissions::from_mode(0o755)).unwrap(); + DeploymentSpec { deployment_id: deployment_id.into(), ..test_spec() } + } - cmd.execute("AfterInstall", &test_spec()).unwrap(); + /// Lines of the deployment log under `root`, with the + /// `[YYYY-MM-DD HH:MM:SS.mmm] ` prefix checked and removed. + #[cfg(unix)] + fn deployment_log_messages(root: &std::path::Path) -> Vec { + let body = + fs::read_to_string(root.join("deployment-logs/codedeploy-agent-deployments.log")) + .unwrap(); + body.lines() + .map(|line| { + let (ts, msg) = line + .strip_prefix('[') + .and_then(|l| l.split_once("] ")) + .unwrap_or_else(|| panic!("line lacks a [timestamp] prefix: {line:?}")); + assert!( + chrono::NaiveDateTime::parse_from_str(ts, "%Y-%m-%d %H:%M:%S%.3f").is_ok(), + "bad timestamp {ts:?} in {line:?}" + ); + msg.to_string() + }) + .collect() + } + + /// Each line is written as + /// `[] []`, once, with no second + /// timestamp and no blank line, and stdout/stderr are included. + #[cfg(unix)] + #[test] + fn deployment_log_uses_one_record_per_script_line() { + let dir = TempDir::new().unwrap(); + let archives = test_archives(&dir); + let root = dir.path().join("deployments"); + let cmd = HookCommand::new(archives.clone(), test_mapping()) + .with_deployment_log_config(log_config_for(&root)); + let spec = stage_after_install(&archives, "d-123", "echo hi\necho oops >&2\n"); + + cmd.execute("AfterInstall", &spec).unwrap(); + + let mut messages = deployment_log_messages(&root); + // stdout and stderr are read by separate tasks, so their relative + // order is not deterministic; sort the output lines. + messages[2..].sort(); + assert_eq!( + messages, + vec![ + "[d-123]LifecycleEvent - AfterInstall", + "[d-123]Script - scripts/run.sh", + "[d-123][stderr]oops", + "[d-123][stdout]hi", + ] + ); + } - let log_path = root.join("deployment-logs/codedeploy-agent-deployments.log"); - assert!(log_path.exists(), "deployment log must be created at {}", log_path.display()); - let body = fs::read_to_string(&log_path).unwrap(); - assert!( - body.contains("LifecycleEvent - AfterInstall"), - "deployment log must contain the event's lines, got: {body:?}" + /// The deployment log must hold all script output, not the 2048-byte + /// diagnostic tail that is sent to the service. + #[cfg(unix)] + #[test] + fn deployment_log_keeps_output_beyond_diagnostic_tail() { + let dir = TempDir::new().unwrap(); + let archives = test_archives(&dir); + let root = dir.path().join("deployments"); + let cmd = HookCommand::new(archives.clone(), test_mapping()) + .with_deployment_log_config(log_config_for(&root)); + // 200 lines of ~40 bytes is well past the 2048-byte tail. + let spec = stage_after_install( + &archives, + "d-123", + "i=0; while [ $i -lt 200 ]; do echo \"line $i padded to be longer\"; i=$((i+1)); done\n", ); + + cmd.execute("AfterInstall", &spec).unwrap(); + + let messages = deployment_log_messages(&root); + assert_eq!(messages.len(), 202, "header + script line + 200 output lines"); + assert_eq!(messages[0], "[d-123]LifecycleEvent - AfterInstall"); + assert_eq!(messages[2], "[d-123][stdout]line 0 padded to be longer"); + assert_eq!(messages[201], "[d-123][stdout]line 199 padded to be longer"); } /// When the deployment log is NOT configured (the default, i.e. @@ -532,49 +569,66 @@ mod tests { ); } - /// A failing script with the deployment log enabled must still record a - /// failure line in the deployment log and then propagate the error. - /// Exercises the `Err(e) => logger.log(...)` arm in `execute`. + /// A failing script's output must be in the deployment log, and no + /// failure-summary line is added: the deployment log holds only script + /// log lines. #[cfg(unix)] #[test] - fn deployment_log_records_failure_line() { - use std::os::unix::fs::PermissionsExt; - + fn deployment_log_keeps_failing_script_output_without_summary_line() { let dir = TempDir::new().unwrap(); let archives = test_archives(&dir); let root = dir.path().join("deployments"); let cmd = HookCommand::new(archives.clone(), test_mapping()) .with_deployment_log_config(log_config_for(&root)); + let spec = stage_after_install(&archives, "d-123", "echo before-failure\nexit 1\n"); - let archive_dir = archives.archive_dir("dg-1", "d-123"); - let scripts = archive_dir.join("scripts"); - fs::create_dir_all(&scripts).unwrap(); - fs::write( - archive_dir.join("appspec.yml"), - "version: 0.0\nos: linux\nhooks:\n AfterInstall:\n - location: scripts/fail.sh\n timeout: 10\n", - ) - .unwrap(); - fs::write(scripts.join("fail.sh"), "#!/bin/sh\nexit 1\n").unwrap(); - fs::set_permissions(scripts.join("fail.sh"), fs::Permissions::from_mode(0o755)).unwrap(); - - let result = cmd.execute("AfterInstall", &test_spec()); + let result = cmd.execute("AfterInstall", &spec); assert!(result.is_err(), "failing script must propagate an error"); - let log_path = root.join("deployment-logs/codedeploy-agent-deployments.log"); - assert!(log_path.exists(), "deployment log must exist even on failure"); - let body = fs::read_to_string(&log_path).unwrap(); - assert!( - body.contains("AfterInstall/AfterInstall failed:"), - "deployment log must record the failure, got: {body:?}" - ); - // The event header must precede the failure summary so the log keeps - // context for which event failed. - assert!( - body.contains("LifecycleEvent - AfterInstall"), - "deployment log must record the event header on failure, got: {body:?}" + assert_eq!( + deployment_log_messages(&root), + vec![ + "[d-123]LifecycleEvent - AfterInstall", + "[d-123]Script - scripts/run.sh", + "[d-123][stdout]before-failure", + ] ); } + /// Concurrent deployments share one deployment log handle: every record stays whole and is tagged with + /// its own deployment ID. + #[cfg(unix)] + #[test] + fn concurrent_deployments_share_one_deployment_log() { + let dir = TempDir::new().unwrap(); + let archives = test_archives(&dir); + let root = dir.path().join("deployments"); + let cmd = HookCommand::new(archives.clone(), test_mapping()) + .with_deployment_log_config(log_config_for(&root)); + let script = "i=0; while [ $i -lt 50 ]; do echo \"out $i\"; i=$((i+1)); done\n"; + let specs: Vec<_> = ["d-aaa", "d-bbb"] + .iter() + .map(|id| stage_after_install(&archives, id, script)) + .collect(); + + std::thread::scope(|scope| { + for spec in &specs { + let cmd = &cmd; + scope.spawn(move || cmd.execute("AfterInstall", spec).unwrap()); + } + }); + + let messages = deployment_log_messages(&root); + for id in ["d-aaa", "d-bbb"] { + let mine: Vec<_> = + messages.iter().filter(|m| m.starts_with(&format!("[{id}]"))).collect(); + assert_eq!(mine.len(), 52, "{id}: header + script line + 50 output lines"); + assert_eq!(*mine[0], format!("[{id}]LifecycleEvent - AfterInstall")); + assert_eq!(*mine[51], format!("[{id}][stdout]out 49")); + } + assert_eq!(messages.len(), 104, "no stray or split records: {messages:?}"); + } + /// If the deployment log can't be opened (its dir can't be created), the /// deployment still succeeds — the log is a best-effort side-channel. /// Exercises the `Err(e) => warn!(...)` arm of the logger open. diff --git a/src/lifecycle_event/executor.rs b/src/lifecycle_event/executor.rs index b9262ad..c040365 100644 --- a/src/lifecycle_event/executor.rs +++ b/src/lifecycle_event/executor.rs @@ -3,11 +3,9 @@ //! Selects the correct deployment directory, parses the appspec, and executes //! each script for a given lifecycle event in order. //! -//! ## Not yet implemented -//! -//! - **Deployment log**: Writes to a secondary `DeploymentLog` when -//! `enable_deployments_log` config is set. `DeploymentLogger` exists in `logging/` -//! but is not wired into the executor yet. Needs config system integration. +//! Every `scripts.log` line is also written to the deployment log when one is +//! attached with [`LifecycleEventExecutor::with_deployment_log`] (set when +//! `enable_deployments_log` is on). use super::LifecycleEventType; use super::deployment_selector::select_deployment_dir; @@ -16,6 +14,7 @@ use super::script::{HookEnvPolicy, Script}; use super::script_run_log::ScriptRunLog; use crate::application_specification::AppSpec; use crate::deployment_specification::types::{DeploymentSpec, RevisionLocation, RevisionSource}; +use crate::logging::DeploymentLogSink; use crate::paths::APPSPEC_PATH_SEPARATORS; use crate::system::file_ops::ensure_executable; use std::collections::HashMap; @@ -43,6 +42,9 @@ pub struct LifecycleEventExecutor { /// `restrict_agent_dir_permissions`. Default `false` (world-readable /// 0755/0644); `true` = hardened 0750/0640. restrict_log_permissions: bool, + /// Deployment log writer for this deployment; `None` when + /// `enable_deployments_log` is off. + deployment_log: Option, } impl LifecycleEventExecutor { @@ -94,9 +96,18 @@ impl LifecycleEventExecutor { env_policy: HookEnvPolicy::default(), reject_path_traversal: false, restrict_log_permissions: false, + deployment_log: None, }) } + /// Write every `scripts.log` line to the deployment log too. `None` + /// (the default) disables it. + #[must_use] + pub fn with_deployment_log(mut self, sink: Option) -> Self { + self.deployment_log = sink; + self + } + /// Set the opt-in hook-environment hardening policy. Defaults to /// [`HookEnvPolicy::default`] (full env inheritance). #[must_use] @@ -175,7 +186,8 @@ impl LifecycleEventExecutor { let log_path = self.current_deployment_root_dir.join("logs/scripts.log"); let log = Arc::new(Mutex::new( ScriptRunLog::open_with_policy(&log_path, self.restrict_log_permissions) - .unwrap_or_else(|_| ScriptRunLog::in_memory()), + .unwrap_or_else(|_| ScriptRunLog::in_memory()) + .with_deployment_log(self.deployment_log.clone()), )); log.lock().unwrap().write_line("", &format!("LifecycleEvent - {event_name}")); diff --git a/src/lifecycle_event/script.rs b/src/lifecycle_event/script.rs index 970aa20..8e3d41f 100644 --- a/src/lifecycle_event/script.rs +++ b/src/lifecycle_event/script.rs @@ -410,10 +410,12 @@ async fn stream_to_log( let mut buf: Vec = Vec::with_capacity(MAX_LOG_LINE_BYTES); let mut byte = [0u8; 1]; - let flush = |log: &Arc>, buf: &mut Vec| { - if buf.is_empty() { - return; - } + // `write_line` records `buf` as one line. A blank output line is logged + // as a bare `[stdout]` record. A newline that + // ends a line already flushed at the chunk cap is not a blank line, so + // `chunk_flushed` suppresses that record. At EOF an empty buffer is not a + // line either. + let write_line = |log: &Arc>, buf: &mut Vec| { let line = String::from_utf8_lossy(buf); if let Ok(mut log) = log.lock() { log.write_line(prefix, &line); @@ -421,25 +423,32 @@ async fn stream_to_log( buf.clear(); }; + let mut chunk_flushed = false; loop { match reader.read(&mut byte).await { // EOF (Ok(0)) or a read error: stop draining this stream. Ok(0) | Err(_) => break, Ok(_) => { if byte[0] == b'\n' { - flush(log, &mut buf); + if !(buf.is_empty() && chunk_flushed) { + write_line(log, &mut buf); + } + chunk_flushed = false; } else { buf.push(byte[0]); if buf.len() >= MAX_LOG_LINE_BYTES { // Newline-free run hit the chunk cap — flush so rotation // is evaluated and memory stays bounded. - flush(log, &mut buf); + write_line(log, &mut buf); + chunk_flushed = true; } } }, } } - flush(log, &mut buf); // trailing partial line (no final newline) + if !buf.is_empty() { + write_line(log, &mut buf); // trailing partial line (no final newline) + } } /// Log process exit diagnostics at debug level. @@ -475,6 +484,15 @@ mod tests { // Own the parent dir: ScriptRunLog::open chmods it, which is EPERM on // macOS's root-owned $TMPDIR. + /// `scripts.log` records with the `YYYY-MM-DD HH:MM:SS ` prefix removed. + fn logged_records(dir: &TempDir) -> Vec { + std::fs::read_to_string(dir.path().join("scripts.log")) + .unwrap() + .lines() + .map(|l| l.get(20..).unwrap_or_default().to_string()) + .collect() + } + fn sample_log() -> (TempDir, Arc>) { let dir = TempDir::new().unwrap(); let log = @@ -525,6 +543,53 @@ mod tests { assert!(entries[1].contains("second line")); } + #[tokio::test] + async fn stream_to_log_keeps_blank_lines() { + let (_dir, log) = sample_log(); + let input = b"before\n\nafter\n".to_vec(); + + stream_to_log(Some(std::io::Cursor::new(input)), &log, "[stdout]").await; + + let entries = log.lock().unwrap().entries(); + assert_eq!(entries.len(), 3, "blank line must be its own record: {entries:?}"); + assert!(entries[1].ends_with(" [stdout]\n"), "got {:?}", entries[1]); + } + + #[tokio::test] + async fn stream_to_log_adds_no_blank_record_after_line_at_exact_chunk_cap() { + let (dir, log) = sample_log(); + let mut input = vec![b'x'; MAX_LOG_LINE_BYTES]; + input.extend_from_slice(b"\nnext\n"); + + stream_to_log(Some(std::io::Cursor::new(input)), &log, "[stdout]").await; + + let records = logged_records(&dir); + assert_eq!(records.len(), 2, "cap-length line + next, no phantom blank record"); + assert_eq!(records[1], "[stdout]next"); + } + + #[tokio::test] + async fn stream_to_log_keeps_blank_line_after_line_at_exact_chunk_cap() { + let (dir, log) = sample_log(); + let mut input = vec![b'x'; MAX_LOG_LINE_BYTES]; + input.extend_from_slice(b"\n\nnext\n"); + + stream_to_log(Some(std::io::Cursor::new(input)), &log, "[stdout]").await; + + let records = logged_records(&dir); + assert_eq!(records.len(), 3, "cap-length line, real blank line, next"); + assert_eq!(records[1], "[stdout]"); + } + + #[tokio::test] + async fn stream_to_log_writes_nothing_for_empty_output() { + let (_dir, log) = sample_log(); + + stream_to_log(Some(std::io::Cursor::new(Vec::new())), &log, "[stdout]").await; + + assert!(log.lock().unwrap().entries().is_empty()); + } + // --- build_command --- #[cfg(unix)] diff --git a/src/lifecycle_event/script_run_log.rs b/src/lifecycle_event/script_run_log.rs index f45923a..76d3db0 100644 --- a/src/lifecycle_event/script_run_log.rs +++ b/src/lifecycle_event/script_run_log.rs @@ -11,6 +11,7 @@ //! from filling the disk. use super::bounded_fifo_vec::BoundedFifoVec; +use crate::logging::DeploymentLogSink; use chrono::Local; use std::fs::{self, File, OpenOptions}; use std::io::Write; @@ -43,6 +44,8 @@ pub struct ScriptRunLog { buffer: BoundedFifoVec, /// Mode policy for the file (and rotation reopens); see [`log_file_mode`]. restrict_permissions: bool, + /// When set, every line is also written to the shared deployment log. + deployment_log: Option, } impl ScriptRunLog { @@ -73,6 +76,7 @@ impl ScriptRunLog { file: Some(file), buffer: BoundedFifoVec::new(), restrict_permissions, + deployment_log: None, }) } @@ -85,9 +89,18 @@ impl ScriptRunLog { file: None, buffer: BoundedFifoVec::new(), restrict_permissions: false, + deployment_log: None, } } + /// Also write every line to the deployment log + /// (`deployment-logs/-deployments.log`). `None` disables it. + #[must_use] + pub fn with_deployment_log(mut self, sink: Option) -> Self { + self.deployment_log = sink; + self + } + /// Write a single line to both the file and the bounded buffer. /// /// `prefix` is `"[stdout]"`, `"[stderr]"`, or `""` for headers. @@ -98,8 +111,16 @@ impl ScriptRunLog { /// ANSI escape sequences are stripped from `line` before formatting so /// hostile script output cannot forge log entries via cursor positioning, /// screen clearing, or color codes. + /// + /// With a deployment log attached, the line is written there at the same + /// time, as `[]`, under one lock so both + /// files see lines in the same order. The deployment log gets the same + /// ANSI-stripped line. pub fn write_line(&mut self, prefix: &str, line: &str) { let line = strip_ansi_escapes::strip_str(line); + if let Some(sink) = &self.deployment_log { + sink.log(&format!("{prefix}{line}")); + } let ts = Local::now().format("%Y-%m-%d %H:%M:%S"); let formatted = format!("{ts} {prefix}{line}\n"); @@ -313,6 +334,27 @@ mod tests { assert_eq!(entries.len(), 2); } + #[test] + fn write_line_sends_ansi_stripped_line_to_deployment_log() { + let dir = tempfile::tempdir().unwrap(); + let config = crate::logging::LogConfig { + root_dir: dir.path().to_path_buf(), + ..crate::logging::LogConfig::default() + }; + let shared = crate::logging::SharedDeploymentLog::new(config); + shared.ensure_open().unwrap(); + let mut log = + ScriptRunLog::in_memory().with_deployment_log(Some(shared.for_deployment("d-1"))); + + log.write_line("[stdout]", "\x1b[31mred\x1b[0m"); + + assert!(log.entries()[0].ends_with(" [stdout]red\n")); + let body = + fs::read_to_string(dir.path().join("deployment-logs/codedeploy-agent-deployments.log")) + .unwrap(); + assert!(body.ends_with("] [d-1][stdout]red\n"), "got {body:?}"); + } + #[test] fn rotated_path_appends_index() { use std::path::PathBuf; diff --git a/src/logging/deployment_logger.rs b/src/logging/deployment_logger.rs index 2137abe..68f73bc 100644 --- a/src/logging/deployment_logger.rs +++ b/src/logging/deployment_logger.rs @@ -4,6 +4,7 @@ use std::io::{self, Write}; #[cfg(unix)] use std::os::unix::fs::OpenOptionsExt; use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex, PoisonError}; use chrono::Local; @@ -62,7 +63,8 @@ impl DeploymentLogger { pub fn log(&mut self, message: &str) -> io::Result<()> { self.rotate_if_needed()?; let ts = Local::now().format("%Y-%m-%d %H:%M:%S%.3f"); - writeln!(self.file, "[{ts}] {message}") + // One `write_all` per record so concurrent appenders cannot split it. + self.file.write_all(format!("[{ts}] {message}\n").as_bytes()) } fn rotate_if_needed(&mut self) -> io::Result<()> { @@ -93,6 +95,78 @@ impl DeploymentLogger { } } +/// Process-wide handle to the deployment log, shared by every deployment. +/// +/// All concurrent deployments append through one handle and one rotation. A +/// handle per hook command would let one deployment rotate the file while +/// another keeps writing to the renamed file. +/// +/// The file is opened by [`Self::ensure_open`]; a failed open is retried on +/// the next call, so a transient failure does not disable the log for the +/// life of the process. +#[derive(Debug, Clone)] +pub struct SharedDeploymentLog { + config: LogConfig, + logger: Arc>>, +} + +impl SharedDeploymentLog { + #[must_use] + pub fn new(config: LogConfig) -> Self { + Self { config, logger: Arc::new(Mutex::new(None)) } + } + + /// Open the log file if it is not open yet. + /// + /// # Errors + /// + /// Returns an error if the log directory or file cannot be created. + pub fn ensure_open(&self) -> io::Result<()> { + let mut guard = self.logger.lock().unwrap_or_else(PoisonError::into_inner); + if guard.is_none() { + *guard = Some(DeploymentLogger::new(&self.config)?); + } + Ok(()) + } + + /// A writer that tags every record with `deployment_id`. + #[must_use] + pub fn for_deployment(&self, deployment_id: &str) -> DeploymentLogSink { + DeploymentLogSink { log: self.clone(), deployment_id: deployment_id.to_string() } + } + + fn log(&self, message: &str) { + let mut guard = self.logger.lock().unwrap_or_else(PoisonError::into_inner); + if let Some(logger) = guard.as_mut() { + // Best-effort: the deployment log is diagnostic only. + let _ = logger.log(message); + } + } +} + +/// Writes one deployment's script-log records to the shared deployment log. +#[derive(Debug, Clone)] +pub struct DeploymentLogSink { + log: SharedDeploymentLog, + deployment_id: String, +} + +impl DeploymentLogSink { + /// Write `message` as `[]`. + /// + /// Called for every `scripts.log` line. Does nothing if the log is not open. + pub fn log(&self, message: &str) { + self.log + .log(&format!("[{}]{}", self.deployment_id, strip_ascii_whitespace(message))); + } +} + +/// Trims NUL, `\t`, `\n`, `\v`, `\f`, `\r` and space. +/// Narrower than `str::trim`, which also removes Unicode whitespace. +fn strip_ascii_whitespace(s: &str) -> &str { + s.trim_matches(|c| matches!(c, '\0' | '\t' | '\n' | '\x0b' | '\x0c' | '\r' | ' ')) +} + /// Open (or create) the deployment log file with the policy mode. /// Unix: 0644 default, 0640 restricted. The mode is force-set /// on every open so a file a tightened agent left 0640 heals to 0644 on the @@ -219,6 +293,64 @@ mod tests { assert!(lines[0].contains(']')); } + #[test] + fn writes_one_line_per_message_without_blank_lines() { + let dir = tempfile::tempdir().unwrap(); + let mut logger = DeploymentLogger::new(&test_config(dir.path())).unwrap(); + + logger.log("one").unwrap(); + logger.log("two").unwrap(); + + let content = fs::read_to_string(&logger.path).unwrap(); + assert_eq!(content.matches('\n').count(), 2, "exactly one newline per record: {content:?}"); + assert!(!content.contains("\n\n")); + } + + #[test] + fn strip_ascii_whitespace_leaves_unicode_whitespace() { + assert_eq!(strip_ascii_whitespace(" \t[stdout]hi\r\n\0"), "[stdout]hi"); + // Unicode whitespace is not stripped. + assert_eq!(strip_ascii_whitespace("\u{a0}x\u{a0}"), "\u{a0}x\u{a0}"); + } + + #[test] + fn sink_prefixes_deployment_id_and_strips_message() { + let dir = tempfile::tempdir().unwrap(); + let shared = SharedDeploymentLog::new(test_config(dir.path())); + shared.ensure_open().unwrap(); + + shared.for_deployment("d-1").log("[stdout]hello \n"); + + let path = dir.path().join("deployment-logs/test-agent-deployments.log"); + let content = fs::read_to_string(path).unwrap(); + let (_, message) = content.trim_end().split_once("] ").unwrap(); + assert_eq!(message, "[d-1][stdout]hello"); + } + + #[test] + fn sink_is_a_no_op_until_opened() { + let dir = tempfile::tempdir().unwrap(); + let shared = SharedDeploymentLog::new(test_config(dir.path())); + + shared.for_deployment("d-1").log("dropped"); + + assert!(!dir.path().join("deployment-logs").exists()); + } + + #[test] + fn ensure_open_retries_after_a_failure() { + let dir = tempfile::tempdir().unwrap(); + // A regular file where `deployment-logs/`'s parent should be. + let blocker = dir.path().join("root"); + fs::write(&blocker, "x").unwrap(); + let shared = SharedDeploymentLog::new(test_config(&blocker)); + assert!(shared.ensure_open().is_err()); + + fs::remove_file(&blocker).unwrap(); + + assert!(shared.ensure_open().is_ok(), "a later call must open the log"); + } + #[test] fn rotated_path_appends_index() { let base = PathBuf::from("/var/log/test.log"); diff --git a/src/logging/mod.rs b/src/logging/mod.rs index 74eb579..a4a59a0 100644 --- a/src/logging/mod.rs +++ b/src/logging/mod.rs @@ -3,7 +3,7 @@ mod agent_logger; mod deployment_logger; pub use agent_logger::LogGuard; -pub use deployment_logger::DeploymentLogger; +pub use deployment_logger::{DeploymentLogSink, DeploymentLogger, SharedDeploymentLog}; use std::path::PathBuf; From 68abe79f65a8a180189c5621f2ac8a9080bd9a34 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Wed, 14 Oct 2026 07:09:24 +0000 Subject: [PATCH 08/13] Add optional parallel range downloads for S3 bundles A bundle is downloaded with one GetObject over one connection, so large bundles are limited by single-connection throughput. Setting s3_download_max_concurrency above 1 splits the download into parallel range requests. The default stays 1, a single unranged GetObject. - The first request fetches the first range and reads the object size from Content-Range, so an object of one chunk or less still takes one request. - Later ranges send If-Match with the first ETag (and the versionId, if pinned), so an object replaced mid-download fails the deployment instead of producing a mixed bundle. Each response must carry the Content-Range that was asked for, and at most that many bytes are written. - Each range retries up to 3 times when its body fails mid-stream, which the SDK does not retry. Failed requests are left to the SDK's own retries, and local write errors fail at once. - Ranged requests turn off the SDK's response checksum check, since a full-object checksum cannot be checked against one range. - Ranges are written to their own offsets of one file with the new write_all_at (pwrite on Unix, seek_write on Windows). - The runtime is multi-threaded only when concurrency is above 1, with min(cores, concurrency) workers, capped at 8. The worker's agent-update client only fetches the install script, so it stays single-threaded. - The download falls back to one unranged GetObject when the endpoint refuses ranges (501) or the object is empty (416), judged by HTTP status when the error has no body; when there is no strong ETag or version to pin the object; or when a response without Content-Range may be truncated. New config keys, clamped rather than rejected: s3_download_max_concurrency (default 1, max 64) and s3_download_chunk_size (default 8MiB, minimum 64KiB). tests/s3_parallel_download.rs runs against a fake S3 endpoint and checks parallelism, the concurrency cap, If-Match, the fallbacks, retries, a replaced object, a later range answered with the whole object, and a checksum header on a range response. --- Cargo.toml | 3 +- conf/codedeployagent.yml | 9 + src/aws_clients/s3_client.rs | 778 ++++++++++++++++++++-- src/config/mod.rs | 133 +++- src/daemon/worker.rs | 29 +- src/host_command/commands/update_agent.rs | 2 +- src/main.rs | 5 + src/system/file_ops.rs | 109 +++ src/system/mod.rs | 2 +- tests/s3_parallel_download.rs | 643 ++++++++++++++++++ 10 files changed, 1653 insertions(+), 60 deletions(-) create mode 100644 tests/s3_parallel_download.rs diff --git a/Cargo.toml b/Cargo.toml index 7ff6855..9202a6e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,7 +27,8 @@ tracing-subscriber = { version = "0.3", features = ["env-filter", "fmt", "regist tracing-appender = "0.2" time = { version = "0.3", features = ["macros", "formatting"] } nu-path = "0.110" -tokio = { version = "1", features = ["process", "io-util", "time", "rt", "macros", "sync"] } +# `rt-multi-thread`: parallel S3 range downloads. +tokio = { version = "1", features = ["process", "io-util", "time", "rt", "rt-multi-thread", "macros", "sync"] } chrono = "0.4" reqwest = { version = "0.12", default-features = false, features = ["blocking", "rustls-tls"] } # `vendored`: ship OpenSSL inside the binary so one artifact runs across diff --git a/conf/codedeployagent.yml b/conf/codedeployagent.yml index 238625e..0368864 100644 --- a/conf/codedeployagent.yml +++ b/conf/codedeployagent.yml @@ -125,6 +125,15 @@ # :archive_max_extraction_size: "4GiB" # binary (powers of 1024) # :archive_max_extraction_size: "4GB" # SI (powers of 1000) +# Bundle download tuning (Amazon S3 revisions). By default the agent downloads +# a bundle with a single GetObject. +# s3_download_max_concurrency: parallel HTTP range requests in flight +# (default 1, max 64). Set above 1 to download in parallel ranges. +# s3_download_chunk_size: bytes per request (default 8MiB, minimum 64KiB). +# Accepts bytes or strings like "16MB". +# :s3_download_max_concurrency: 10 +# :s3_download_chunk_size: "8MiB" + # Endpoints (leave empty for defaults) # :deploy_control_endpoint: # :s3_endpoint_override: diff --git a/src/aws_clients/s3_client.rs b/src/aws_clients/s3_client.rs index 7606162..b3e918e 100644 --- a/src/aws_clients/s3_client.rs +++ b/src/aws_clients/s3_client.rs @@ -3,9 +3,23 @@ //! Wraps `aws-sdk-s3` with agent-specific configuration: endpoint override, //! FIPS mode, proxy, and signature v4 (SDK default). //! -//! Downloads use a single `GetObject` request with streaming body. -//! For very large bundles, consider switching to `aws-s3-transfer-manager` -//! which splits downloads into parallel range requests for higher throughput. +//! ## Parallel downloads +//! +//! When `download_max_concurrency` is above 1, a download is split into +//! `download_chunk_size` range requests (default 8 MiB) with up to that many in +//! flight, each written to its own offset of the destination file. +//! +//! - The first request fetches the first chunk and reads the object size from +//! `Content-Range`, so a small object still costs one request. +//! - Later ranges send `If-Match` with the first `ETag` (and the `versionId`, if +//! pinned), so an object replaced mid-download fails instead of mixing two +//! revisions. +//! - Each range retries a body that fails mid-stream, which the SDK does not. +//! - The download falls back to one unranged `GetObject` when the endpoint +//! refuses ranges, the object is empty, there is no strong `ETag` or version +//! to pin the object, or a response without `Content-Range` may be truncated. +//! +//! The default concurrency, 1, downloads with one unranged `GetObject`. //! //! ## Wire logging (`log_aws_wire`) //! @@ -30,12 +44,35 @@ use super::aws_client::AwsClient; use super::credentials::{CredentialMode, Credentials}; -use std::io::{self, Write}; +use std::fs::File; +use std::io; use std::path::Path; -use tracing::{debug, info}; +use std::sync::Arc; +use tracing::{debug, info, warn}; + +/// Read buffer per streaming body. Peak memory is `concurrency * this`. +const STREAM_BUFFER_SIZE: usize = 1024 * 1024; + +/// Default range requests in flight. +pub const DEFAULT_DOWNLOAD_MAX_CONCURRENCY: usize = 1; + +/// Default range size. +pub const DEFAULT_DOWNLOAD_CHUNK_SIZE: u64 = 8 * 1024 * 1024; + +/// Maximum range requests in flight. +pub const MAX_DOWNLOAD_MAX_CONCURRENCY: usize = 64; -/// Streaming buffer size for S3 downloads. -const STREAM_BUFFER_SIZE: usize = 8 * 1024 * 1024; +/// Minimum range size. +pub const MIN_DOWNLOAD_CHUNK_SIZE: u64 = 64 * 1024; + +/// Maximum download worker threads. +const MAX_DOWNLOAD_WORKER_THREADS: usize = 8; + +/// Attempts per range. +const RANGE_ATTEMPTS: u32 = 3; + +/// Backoff between range attempts, multiplied by the attempt number. +const RANGE_RETRY_BACKOFF: std::time::Duration = std::time::Duration::from_millis(500); /// Load additional CA certificates from `AWS_SSL_CA_DIRECTORY` into a /// [`TrustStore`], if the env var is set and points at a directory with `*.pem` @@ -197,7 +234,7 @@ fn build_custom_http_client( } /// Configuration for S3 client construction. -#[derive(Debug, Default)] +#[derive(Debug, Clone)] pub struct S3ClientConfig { /// Custom S3 endpoint URL (overrides default). pub endpoint_override: Option, @@ -214,9 +251,37 @@ pub struct S3ClientConfig { /// wire logging is disabled (the default). The tuple is /// `(log_dir, program_name)`. pub wire_log: Option<(std::path::PathBuf, String)>, + /// Range requests in flight per download; `1` means one unranged + /// `GetObject`. Clamped to `1..=`[`MAX_DOWNLOAD_MAX_CONCURRENCY`]. + pub download_max_concurrency: usize, + /// Bytes per range request. Floored at [`MIN_DOWNLOAD_CHUNK_SIZE`]. + pub download_chunk_size: u64, +} + +impl Default for S3ClientConfig { + fn default() -> Self { + Self { + endpoint_override: None, + use_fips: false, + proxy_uri: None, + wire_log: None, + download_max_concurrency: DEFAULT_DOWNLOAD_MAX_CONCURRENCY, + download_chunk_size: DEFAULT_DOWNLOAD_CHUNK_SIZE, + } + } } impl S3ClientConfig { + /// Clamped concurrency; `0` becomes `1`. + fn effective_concurrency(&self) -> usize { + self.download_max_concurrency.clamp(1, MAX_DOWNLOAD_MAX_CONCURRENCY) + } + + /// Chunk size floored at [`MIN_DOWNLOAD_CHUNK_SIZE`]. + fn effective_chunk_size(&self) -> u64 { + self.download_chunk_size.max(MIN_DOWNLOAD_CHUNK_SIZE) + } + fn resolve_endpoint(&self, region: &str) -> Option { if let Some(endpoint) = &self.endpoint_override { debug!("Using S3 override endpoint {endpoint}"); @@ -235,6 +300,8 @@ pub struct S3Client { credentials: Credentials, inner: aws_sdk_s3::Client, runtime: tokio::runtime::Runtime, + max_concurrency: usize, + chunk_size: u64, } impl S3Client { @@ -243,11 +310,24 @@ impl S3Client { /// # Errors /// Returns an error if the tokio runtime cannot be created. pub fn new(credentials: Credentials, config: &S3ClientConfig) -> io::Result { - let runtime = tokio::runtime::Builder::new_current_thread().enable_all().build()?; + let max_concurrency = config.effective_concurrency(); + let chunk_size = config.effective_chunk_size(); + + // A current-thread runtime would serialize the ranges' TLS decryption + // and file writes. The workers live as long as the client. + let runtime = if max_concurrency > 1 { + tokio::runtime::Builder::new_multi_thread() + .worker_threads(download_worker_threads(max_concurrency)) + .thread_name("codedeploy-s3") + .enable_all() + .build()? + } else { + tokio::runtime::Builder::new_current_thread().enable_all().build()? + }; let inner = runtime.block_on(Self::build_client(&credentials, config))?; - Ok(Self { credentials, inner, runtime }) + Ok(Self { credentials, inner, runtime, max_concurrency, chunk_size }) } async fn build_client( @@ -334,9 +414,8 @@ impl S3Client { Ok(aws_sdk_s3::Client::from_conf(s3_config.build())) } - /// Download an S3 object to a local file. - /// - /// Returns the `ETag` of the downloaded object (with quotes stripped). + /// Download an S3 object to a local file and return its `ETag` (quotes + /// stripped). Uses parallel range requests unless concurrency is 1. /// /// # Errors /// Returns an error if the download or file write fails. @@ -362,42 +441,418 @@ impl S3Client { "Downloading artifact bundle from bucket '{bucket}' and key '{key}', version '{version_str}'" ); - let mut req = self.inner.get_object().bucket(bucket).key(key); - if let Some(v) = version { - req = req.version_id(v); - } + let etag = if self.max_concurrency == 1 { + self.download_whole_object(bucket, key, version, dest).await? + } else { + match self.download_ranged(bucket, key, version, dest).await { + Ok(etag) => etag, + Err(RangedDownloadError::Unsupported(reason)) => { + warn!( + "Ranged S3 download unavailable ({reason}); retrying as a single request" + ); + self.download_whole_object(bucket, key, version, dest).await? + }, + Err(RangedDownloadError::Failed(e)) => return Err(e), + } + }; + + info!("Download complete from bucket '{bucket}' and key '{key}'"); + Ok(etag) + } - let resp = req + /// One unranged `GetObject` streamed to disk. + async fn download_whole_object( + &self, + bucket: &str, + key: &str, + version: Option<&str>, + dest: &Path, + ) -> io::Result> { + let resp = self + .inner + .get_object() + .bucket(bucket) + .key(key) + .set_version_id(version.map(str::to_string)) .send() .await - .map_err(|e| io::Error::other(format_s3_get_object_error(&e, bucket, key, version)))?; + .map_err(|e| { + io::Error::other(format_s3_get_object_error(&e, bucket, key, version, None)) + })?; let etag = resp.e_tag().map(|e| e.trim_matches('"').to_string()); + let file = create_download_file(dest)?; + stream_body_at(&file, resp.body.into_async_read(), 0) + .await + .map_err(StreamError::into_io)?; + Ok(etag) + } - let body = resp.body.into_async_read(); - stream_to_file(body, dest).await?; + /// Download with parallel range requests. The first request fetches the first + /// chunk and learns the object size from `Content-Range`. + async fn download_ranged( + &self, + bucket: &str, + key: &str, + version: Option<&str>, + dest: &Path, + ) -> Result, RangedDownloadError> { + let first = ranged_get(&self.inner, bucket, key, version, None, 0, self.chunk_size) + .send() + .await + .map_err(|e| { + if range_unsupported(&s3_error_code(&e), http_status(&e)) { + RangedDownloadError::Unsupported(s3_error_code(&e)) + } else { + RangedDownloadError::Failed(io::Error::other(format_s3_get_object_error( + &e, + bucket, + key, + version, + Some(0), + ))) + } + })?; + + // The quoted ETag goes back out as If-Match; callers get it unquoted. + let raw_etag = first.e_tag().map(str::to_string); + let etag = raw_etag.as_deref().map(|e| e.trim_matches('"').to_string()); + let total = first + .content_range() + .and_then(parse_content_range) + .and_then(|(start, total)| (start == 0).then_some(total)); + + let file = create_download_file(dest)?; + let body = first.body.into_async_read(); + + let Some(total) = total else { + // Without a size there is nothing to plan, so the body must be the + // whole object or we start over unranged. + let len = stream_body_at(&file, body, 0).await.map_err(StreamError::into_io)?; + if first_response_is_complete(len, self.chunk_size) { + return Ok(etag); + } + return Err(RangedDownloadError::Unsupported( + "response carried no usable Content-Range".to_string(), + )); + }; - info!("Download complete from bucket '{bucket}' and key '{key}'"); + // We know the size, so a first body that fails mid-stream is re-fetched + // with the other ranges. A local write failure fails the download. + let head_len = + match stream_body_at(&file, tokio::io::AsyncReadExt::take(body, self.chunk_size), 0) + .await + { + Ok(len) => len, + Err(StreamError::Read(e)) => { + warn!( + "First range of s3://{bucket}/{key} failed mid-stream ({e}); re-fetching it" + ); + 0 + }, + Err(StreamError::Write(e)) => return Err(e.into()), + }; + if head_len >= total { + return Ok(etag); + } + + // Without a strong ETag or a version, nothing stops the object changing + // between ranges, so download it in one request instead. + let pin_etag = raw_etag.filter(|e| !e.starts_with("W/")); + if pin_etag.is_none() && version.is_none() { + return Err(RangedDownloadError::Unsupported( + "no strong ETag or version to pin the object".to_string(), + )); + } + + let ranges = plan_ranges(head_len, total, self.chunk_size); + info!( + "Bundle is {total} bytes: fetching {} ranges of up to {} bytes, {} at a time", + ranges.len(), + self.chunk_size, + self.max_concurrency + ); + file.set_len(total)?; + + let job = Arc::new(RangeJob { + client: self.inner.clone(), + bucket: bucket.to_string(), + key: key.to_string(), + version: version.map(str::to_string), + etag: pin_etag, + total, + file, + }); + self.fetch_ranges(&job, ranges).await?; Ok(etag) } + + /// Fetch `ranges` with at most `max_concurrency` in flight. On the first + /// error the other tasks are cancelled and awaited, so none of them is still + /// writing to the file when this returns. + async fn fetch_ranges(&self, job: &Arc, ranges: Vec<(u64, u64)>) -> io::Result<()> { + let permits = Arc::new(tokio::sync::Semaphore::new(self.max_concurrency)); + let mut tasks = tokio::task::JoinSet::new(); + for (offset, len) in ranges { + let job = Arc::clone(job); + let permits = Arc::clone(&permits); + tasks.spawn(async move { + let _permit = permits.acquire().await.map_err(io::Error::other)?; + job.fetch_with_retry(offset, len).await + }); + } + while let Some(joined) = tasks.join_next().await { + if let Err(e) = joined.map_err(io::Error::other).and_then(|r| r) { + tasks.shutdown().await; + return Err(e); + } + } + Ok(()) + } +} + +/// A ranged `GetObject` request. The SDK's response checksum check is turned +/// off because a full-object checksum header cannot be checked against one +/// range; each range's length is checked instead. +fn ranged_get( + client: &aws_sdk_s3::Client, + bucket: &str, + key: &str, + version: Option<&str>, + if_match: Option<&str>, + offset: u64, + len: u64, +) -> aws_sdk_s3::client::customize::CustomizableOperation< + aws_sdk_s3::operation::get_object::GetObjectOutput, + aws_sdk_s3::operation::get_object::GetObjectError, + aws_sdk_s3::operation::get_object::builders::GetObjectFluentBuilder, +> { + client + .get_object() + .bucket(bucket) + .key(key) + .set_version_id(version.map(str::to_string)) + .set_if_match(if_match.map(str::to_string)) + .range(range_header(offset, len)) + .customize() + .config_override(aws_sdk_s3::config::Builder::default().response_checksum_validation( + aws_sdk_s3::config::ResponseChecksumValidation::WhenRequired, + )) +} + +/// Why a ranged download did not complete. +enum RangedDownloadError { + /// The endpoint or object cannot serve ranges; fall back to one request. + Unsupported(String), + /// A real failure; the deployment fails. + Failed(io::Error), +} + +impl From for RangedDownloadError { + fn from(e: io::Error) -> Self { + Self::Failed(e) + } +} + +/// Worker threads for the download runtime: `min(cores, concurrency)`, capped +/// at [`MAX_DOWNLOAD_WORKER_THREADS`]. +fn download_worker_threads(max_concurrency: usize) -> usize { + let cores = std::thread::available_parallelism().map_or(1, std::num::NonZeroUsize::get); + cores.min(max_concurrency).clamp(1, MAX_DOWNLOAD_WORKER_THREADS) +} + +/// Create the download file with mode 0600. +fn create_download_file(dest: &Path) -> io::Result { + crate::system::create_file_secure(dest, 0o600) +} + +/// `Range` header for `len` (> 0) bytes from `offset`; HTTP ranges are inclusive. +fn range_header(offset: u64, len: u64) -> String { + debug_assert!(len > 0, "a range must cover at least one byte"); + format!("bytes={offset}-{}", offset + len - 1) +} + +/// Split `[start, total)` into `(offset, len)` chunks. +fn plan_ranges(start: u64, total: u64, chunk_size: u64) -> Vec<(u64, u64)> { + let chunk_size = chunk_size.max(1); + let mut ranges = Vec::new(); + let mut offset = start; + while offset < total { + let len = chunk_size.min(total - offset); + ranges.push((offset, len)); + offset += len; + } + ranges +} + +/// `(start, total)` from `Content-Range: bytes 0-99/1234`. `None` if malformed +/// or the total is `*`. +fn parse_content_range(header: &str) -> Option<(u64, u64)> { + let (range, total) = header.trim().strip_prefix("bytes ")?.split_once('/')?; + let (start, _) = range.split_once('-')?; + Some((start.trim().parse().ok()?, total.trim().parse().ok()?)) +} + +/// Whether a first response without `Content-Range` holds the whole object. +/// +/// It normally does (the endpoint ignored `Range` and sent everything). A body of +/// exactly the requested length could also be a truncated range response, so +/// that one case is downloaded again. +fn first_response_is_complete(body_len: u64, requested: u64) -> bool { + body_len != requested +} + +/// Whether a failed first request means the endpoint or object cannot serve +/// ranges: `416` on an empty object, or `501` from endpoints without range +/// support. The status is checked too, because error responses without an XML +/// body carry no code. +fn range_unsupported(code: &str, status: Option) -> bool { + matches!(code, "InvalidRange" | "NotImplemented") || matches!(status, Some(416 | 501)) +} + +/// Whether a failed range request means the object changed (`412`). +fn precondition_failed(code: &str, status: Option) -> bool { + code == "PreconditionFailed" || status == Some(412) +} + +/// S3 error code, or `"unknown"` when the failure carried none. +fn s3_error_code(err: &aws_sdk_s3::error::SdkError) -> String +where + E: aws_sdk_s3::error::ProvideErrorMetadata, +{ + use aws_sdk_s3::error::ProvideErrorMetadata; + err.code().unwrap_or("unknown").to_string() +} + +/// HTTP status of a failed request, when a response was received. +fn http_status( + err: &aws_sdk_s3::error::SdkError, +) -> Option { + err.raw_response().map(|r| r.status().as_u16()) +} + +/// Everything a range task needs, shared by all ranges of one download. +struct RangeJob { + client: aws_sdk_s3::Client, + bucket: String, + key: String, + version: Option, + /// Strong `ETag` as the first response sent it (quoted), sent as `If-Match`. + etag: Option, + /// Object size from the first response. + total: u64, + file: File, +} + +/// A range failure: a body that failed mid-stream can be retried; anything +/// else fails the download. +enum RangeError { + BodyFailed(io::Error), + Fatal(io::Error), +} + +impl RangeJob { + /// Fetch one range, retrying a body that fails mid-stream. The SDK already + /// retries failed requests, so those are not retried again here. Retries are + /// safe because each range is rewritten at a fixed offset. + async fn fetch_with_retry(&self, offset: u64, len: u64) -> io::Result<()> { + let mut attempt = 1; + loop { + match self.fetch(offset, len).await { + Ok(()) => return Ok(()), + Err(RangeError::BodyFailed(e)) if attempt < RANGE_ATTEMPTS => { + warn!( + "Range at offset {offset} of s3://{}/{} failed mid-stream (attempt {attempt} of {RANGE_ATTEMPTS}), retrying: {e}", + self.bucket, self.key + ); + tokio::time::sleep(RANGE_RETRY_BACKOFF * attempt).await; + attempt += 1; + }, + Err(RangeError::BodyFailed(e) | RangeError::Fatal(e)) => return Err(e), + } + } + } + + async fn fetch(&self, offset: u64, len: u64) -> Result<(), RangeError> { + let (bucket, key, version) = (&self.bucket, &self.key, self.version.as_deref()); + // versionId and If-Match pin the revision, so an object overwritten + // between ranges fails instead of producing a mixed file. + let resp = + ranged_get(&self.client, bucket, key, version, self.etag.as_deref(), offset, len) + .send() + .await + .map_err(|e| { + let mut message = + format_s3_get_object_error(&e, bucket, key, version, Some(offset)); + if precondition_failed(&s3_error_code(&e), http_status(&e)) { + message.push_str(" (the object changed during the download)"); + } + RangeError::Fatal(io::Error::other(message)) + })?; + + // The response must be the range we asked for, from the same object. + let content_range = resp.content_range().and_then(parse_content_range); + if content_range != Some((offset, self.total)) { + return Err(RangeError::Fatal(io::Error::other(format!( + "S3 range at offset {offset} of s3://{bucket}/{key} returned Content-Range {:?}, expected bytes {offset}-{}/{}", + resp.content_range(), + offset + len - 1, + self.total + )))); + } + + let body = tokio::io::AsyncReadExt::take(resp.body.into_async_read(), len); + let written = match stream_body_at(&self.file, body, offset).await { + Ok(n) => n, + Err(StreamError::Read(e)) => return Err(RangeError::BodyFailed(e)), + Err(StreamError::Write(e)) => return Err(RangeError::Fatal(e)), + }; + if written != len { + return Err(RangeError::BodyFailed(io::Error::other(format!( + "S3 range at offset {offset} of s3://{bucket}/{key} returned {written} bytes, expected {len}" + )))); + } + Ok(()) + } +} + +/// Which side of [`stream_body_at`] failed. +#[derive(Debug)] +enum StreamError { + /// Reading the response body (network). + Read(io::Error), + /// Writing the file (local disk). + Write(io::Error), } -async fn stream_to_file( +impl StreamError { + fn into_io(self) -> io::Error { + match self { + Self::Read(e) | Self::Write(e) => e, + } + } +} + +/// Stream `reader` into `file` from `offset` and return the byte count. Uses +/// positioned writes, so ranges can share one file handle. +async fn stream_body_at( + file: &File, mut reader: impl tokio::io::AsyncRead + Unpin, - dest: &Path, -) -> io::Result<()> { - // Downloaded bundles are created 0600 so unprivileged users cannot - // read the archive while the agent is extracting it. - let mut file = crate::system::create_file_secure(dest, 0o600)?; + offset: u64, +) -> Result { let mut buf = vec![0u8; STREAM_BUFFER_SIZE]; + let mut written = 0u64; loop { - let n = tokio::io::AsyncReadExt::read(&mut reader, &mut buf).await?; + let n = tokio::io::AsyncReadExt::read(&mut reader, &mut buf) + .await + .map_err(StreamError::Read)?; if n == 0 { - break; + return Ok(written); } - file.write_all(&buf[..n])?; + crate::system::write_all_at(file, &buf[..n], offset + written) + .map_err(StreamError::Write)?; + written += n as u64; } - file.flush() } /// Format an S3 `GetObject` failure naming the bucket/key and surfacing the S3 @@ -408,11 +863,14 @@ async fn stream_to_file( /// code/message from the error metadata; falls back to the full source-error /// chain for non-service failures (dispatch/timeout/TLS) that carry no response /// metadata. +/// +/// `range_offset` is the offset of the failing range, if any. fn format_s3_get_object_error( err: &aws_sdk_s3::error::SdkError, bucket: &str, key: &str, version: Option<&str>, + range_offset: Option, ) -> String where E: aws_sdk_s3::error::ProvideErrorMetadata + std::error::Error + Send + Sync + 'static, @@ -420,10 +878,11 @@ where { use aws_sdk_s3::error::ProvideErrorMetadata; - let location = match version { - Some(v) => format!("s3://{bucket}/{key} (version {v})"), - None => format!("s3://{bucket}/{key}"), - }; + let location = format!( + "s3://{bucket}/{key}{}{}", + version.map_or_else(String::new, |v| format!(" (version {v})")), + range_offset.map_or_else(String::new, |o| format!(" (range at offset {o})")), + ); // S3 error code and service message, when response metadata is present. let code = err.code().map(str::to_string); @@ -558,29 +1017,258 @@ mod tests { } #[tokio::test] - async fn stream_to_file_writes_content() { + async fn stream_body_at_writes_content() { let dir = tempfile::TempDir::new().unwrap(); let dest = dir.path().join("out.bin"); let data = b"hello world"; - let reader = &data[..]; - stream_to_file(reader, &dest).await.unwrap(); + let file = create_download_file(&dest).unwrap(); + let written = stream_body_at(&file, &data[..], 0).await.unwrap(); + assert_eq!(written, data.len() as u64); + drop(file); assert_eq!(std::fs::read(&dest).unwrap(), data); } #[tokio::test] - async fn stream_to_file_empty_body() { + async fn stream_body_at_empty_body() { let dir = tempfile::TempDir::new().unwrap(); let dest = dir.path().join("empty.bin"); let reader: &[u8] = &[]; - stream_to_file(reader, &dest).await.unwrap(); + let file = create_download_file(&dest).unwrap(); + assert_eq!(stream_body_at(&file, reader, 0).await.unwrap(), 0); + drop(file); assert_eq!(std::fs::read(&dest).unwrap().len(), 0); } #[tokio::test] - async fn stream_to_file_bad_path() { - let reader: &[u8] = b"data"; - let result = stream_to_file(reader, Path::new("/nonexistent/dir/file")).await; - assert!(result.is_err()); + async fn stream_body_at_honors_offset() { + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("offset.bin"); + let file = create_download_file(&dest).unwrap(); + file.set_len(6).unwrap(); + stream_body_at(&file, &b"world"[..], 1).await.unwrap(); + stream_body_at(&file, &b"h"[..], 0).await.unwrap(); + drop(file); + assert_eq!(std::fs::read(&dest).unwrap(), b"hworld"); + } + + #[test] + fn create_download_file_bad_path() { + assert!(create_download_file(Path::new("/nonexistent/dir/file")).is_err()); + } + + #[test] + fn range_header_is_inclusive_end() { + assert_eq!(range_header(0, 8 * 1024 * 1024), "bytes=0-8388607"); + assert_eq!(range_header(8_388_608, 100), "bytes=8388608-8388707"); + } + + #[test] + fn plan_ranges_splits_remainder() { + // 20 bytes total, first 8 already on disk, 8-byte chunks. + assert_eq!(plan_ranges(8, 20, 8), vec![(8, 8), (16, 4)]); + } + + #[test] + fn plan_ranges_exact_multiple() { + assert_eq!(plan_ranges(10, 30, 10), vec![(10, 10), (20, 10)]); + } + + #[test] + fn plan_ranges_nothing_left() { + assert!(plan_ranges(20, 20, 8).is_empty()); + assert!(plan_ranges(25, 20, 8).is_empty()); + } + + #[test] + fn plan_ranges_covers_whole_object_without_gaps() { + let ranges = plan_ranges(0, 1000, 128); + let mut expected_offset = 0; + for (offset, len) in &ranges { + assert_eq!(*offset, expected_offset, "gap or overlap in {ranges:?}"); + expected_offset += len; + } + assert_eq!(expected_offset, 1000, "ranges must cover the object exactly"); + } + + #[test] + fn plan_ranges_zero_chunk_size_does_not_hang() { + // Defensive: a 0 chunk size would otherwise loop forever. + assert_eq!(plan_ranges(0, 3, 0), vec![(0, 1), (1, 1), (2, 1)]); + } + + #[test] + fn parse_content_range_reads_start_and_total() { + assert_eq!(parse_content_range("bytes 0-8388607/12345678"), Some((0, 12_345_678))); + assert_eq!(parse_content_range("bytes 65536-131071/200000"), Some((65_536, 200_000))); + } + + #[test] + fn parse_content_range_rejects_unknown_and_malformed() { + assert_eq!(parse_content_range("bytes 0-99/*"), None); + assert_eq!(parse_content_range("bytes 0-99"), None); + assert_eq!(parse_content_range(""), None); + assert_eq!(parse_content_range("garbage/notanumber"), None); + } + + #[test] + fn first_response_completeness() { + // Shorter than requested → the object is smaller than one chunk. + assert!(first_response_is_complete(100, 8 * 1024 * 1024)); + // Longer than requested → `Range` was ignored and this is the whole object. + assert!(first_response_is_complete(32 * 1024 * 1024, 8 * 1024 * 1024)); + // Exactly the requested length → ambiguous, so treated as incomplete. + assert!(!first_response_is_complete(8 * 1024 * 1024, 8 * 1024 * 1024)); + } + + #[test] + fn precondition_failed_by_code_or_status() { + assert!(precondition_failed("PreconditionFailed", None)); + assert!(precondition_failed("unknown", Some(412))); + assert!(!precondition_failed("AccessDenied", Some(403))); + } + + /// A `GetObject` service error with S3 response metadata. The raw response + /// type is `()`, since the helpers only need it to be `Debug + Send + Sync`. + fn service_error( + code: &str, + message: &str, + ) -> aws_sdk_s3::error::SdkError { + let metadata = aws_smithy_types::error::ErrorMetadata::builder() + .code(code) + .message(message) + .build(); + aws_sdk_s3::error::SdkError::service_error( + aws_sdk_s3::operation::get_object::GetObjectError::generic(metadata), + (), + ) + } + + /// A failure with no response metadata, such as a connection or timeout error. + fn transport_error() + -> aws_sdk_s3::error::SdkError { + aws_sdk_s3::error::SdkError::timeout_error(Box::new(io::Error::other("connect timed out"))) + } + + #[test] + fn s3_error_code_reads_response_metadata() { + assert_eq!(s3_error_code(&service_error("AccessDenied", "denied")), "AccessDenied"); + } + + #[test] + fn s3_error_code_without_metadata_is_unknown() { + assert_eq!(s3_error_code(&transport_error()), "unknown"); + } + + #[test] + fn range_unsupported_by_code_or_status() { + // 416 on an empty object and 501 from endpoints without range support, + // with or without an XML error body. + assert!(range_unsupported("InvalidRange", Some(416))); + assert!(range_unsupported("NotImplemented", Some(501))); + assert!(range_unsupported("unknown", Some(416))); + assert!(range_unsupported("unknown", Some(501))); + } + + #[test] + fn range_unsupported_does_not_swallow_real_failures() { + for (code, status) in [ + ("AccessDenied", Some(403)), + ("NoSuchKey", Some(404)), + ("SlowDown", Some(503)), + ("unknown", None), + ] { + assert!(!range_unsupported(code, status), "{code} must not fall back"); + } + } + + #[test] + fn format_error_names_bucket_key_and_s3_code() { + let msg = format_s3_get_object_error( + &service_error("AccessDenied", "Access Denied"), + "my-bucket", + "path/to/bundle.zip", + None, + None, + ); + assert!(msg.contains("s3://my-bucket/path/to/bundle.zip"), "{msg}"); + assert!(msg.contains("AccessDenied"), "{msg}"); + assert!(msg.contains("Access Denied"), "{msg}"); + } + + #[test] + fn format_error_names_version_and_range_offset() { + let msg = format_s3_get_object_error( + &service_error("PreconditionFailed", "At least one precondition failed"), + "my-bucket", + "bundle.zip", + Some("v7"), + Some(8_388_608), + ); + assert!(msg.contains("version v7"), "{msg}"); + assert!(msg.contains("range at offset 8388608"), "{msg}"); + } + + #[test] + fn format_error_without_metadata_falls_back_to_the_error_chain() { + // No S3 code to report, so the underlying cause must still be visible + // rather than the SDK's opaque top-level "dispatch failure". + let msg = format_s3_get_object_error(&transport_error(), "bucket", "key", None, Some(1024)); + assert!(msg.contains("s3://bucket/key"), "{msg}"); + assert!(msg.contains("range at offset 1024"), "{msg}"); + assert!(msg.to_lowercase().contains("timeout"), "expected the cause, got: {msg}"); + } + + #[test] + fn effective_concurrency_is_clamped() { + let low = S3ClientConfig { download_max_concurrency: 0, ..Default::default() }; + assert_eq!(low.effective_concurrency(), 1, "0 must degrade to serial, not divide by zero"); + let high = S3ClientConfig { download_max_concurrency: 10_000, ..Default::default() }; + assert_eq!(high.effective_concurrency(), MAX_DOWNLOAD_MAX_CONCURRENCY); + assert_eq!(S3ClientConfig::default().effective_concurrency(), 1); + } + + #[test] + fn effective_chunk_size_is_floored() { + let tiny = S3ClientConfig { download_chunk_size: 1, ..Default::default() }; + assert_eq!(tiny.effective_chunk_size(), MIN_DOWNLOAD_CHUNK_SIZE); + assert_eq!(S3ClientConfig::default().effective_chunk_size(), 8 * 1024 * 1024); + } + + #[test] + fn download_worker_threads_bounded() { + assert_eq!(download_worker_threads(1), 1); + assert!(download_worker_threads(64) <= MAX_DOWNLOAD_WORKER_THREADS); + assert!(download_worker_threads(64) >= 1); + // Never more threads than ranges we are willing to have in flight. + assert!(download_worker_threads(2) <= 2); + } + + #[test] + fn serial_client_uses_single_threaded_runtime() { + // Concurrency 1 must not spin up worker threads for the daemon's lifetime. + let config = S3ClientConfig { download_max_concurrency: 1, ..Default::default() }; + let client = S3Client::new(test_credentials(), &config).unwrap(); + assert_eq!(client.max_concurrency, 1); + assert_eq!( + client.runtime.handle().runtime_flavor(), + tokio::runtime::RuntimeFlavor::CurrentThread + ); + } + + #[test] + fn parallel_client_carries_clamped_settings() { + let config = S3ClientConfig { + download_max_concurrency: 10_000, + download_chunk_size: 1, + ..Default::default() + }; + let client = S3Client::new(test_credentials(), &config).unwrap(); + assert_eq!(client.max_concurrency, MAX_DOWNLOAD_MAX_CONCURRENCY); + assert_eq!(client.chunk_size, MIN_DOWNLOAD_CHUNK_SIZE); + assert_eq!( + client.runtime.handle().runtime_flavor(), + tokio::runtime::RuntimeFlavor::MultiThread + ); } #[test] diff --git a/src/config/mod.rs b/src/config/mod.rs index 6427159..72bb8b3 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -36,6 +36,14 @@ const FIPS_ENABLED_REGIONS: &[&str] = &[ "us-gov-east-1", ]; +/// S3 download defaults and bounds, defined once in the S3 client. +pub use crate::aws_clients::s3_client::{ + DEFAULT_DOWNLOAD_CHUNK_SIZE as DEFAULT_S3_DOWNLOAD_CHUNK_SIZE, + DEFAULT_DOWNLOAD_MAX_CONCURRENCY as DEFAULT_S3_DOWNLOAD_MAX_CONCURRENCY, + MAX_DOWNLOAD_MAX_CONCURRENCY as MAX_S3_DOWNLOAD_MAX_CONCURRENCY, + MIN_DOWNLOAD_CHUNK_SIZE as MIN_S3_DOWNLOAD_CHUNK_SIZE, +}; + /// Maximum allowed value for `kill_agent_max_wait_time_seconds` (24 hours). /// /// A misconfigured value (e.g. `999999999`) would prevent the agent from @@ -56,7 +64,7 @@ where use serde::de::Error; #[derive(Deserialize)] - #[serde(untagged)] + #[serde(untagged, expecting = "a byte count or a size string like \"8MiB\"")] enum ByteSizeValue { Int(u64), Str(String), @@ -72,6 +80,14 @@ where } } +/// Deserialize `s3_download_chunk_size`; an empty value means the default. +fn deserialize_chunk_size<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + Ok(deserialize_byte_size_option(deserializer)?.unwrap_or(DEFAULT_S3_DOWNLOAD_CHUNK_SIZE)) +} + /// Parse a human-readable byte size string into bytes. /// /// Accepts integer values with optional suffix. Fractional values (e.g. "1.5GB") @@ -312,6 +328,17 @@ pub struct AgentConfig { /// Custom S3 endpoint override. `None`. pub s3_endpoint_override: Option, + /// Parallel range requests per S3 bundle download. Default: `1`, which means + /// a single `GetObject`. Clamped to + /// `1..=`[`MAX_S3_DOWNLOAD_MAX_CONCURRENCY`]. + pub s3_download_max_concurrency: usize, + + /// Bytes per S3 range request. Default: 8 MiB. Objects this size or smaller + /// take one request. Accepts bytes or strings like `"16MB"`. Floored at + /// [`MIN_S3_DOWNLOAD_CHUNK_SIZE`]. + #[serde(deserialize_with = "deserialize_chunk_size")] + pub s3_download_chunk_size: u64, + /// Disable `IMDSv1` fallback. Default: `false`. /// When `true`, only `IMDSv2` (token-based) is used for metadata requests. pub disable_imds_v1: bool, @@ -388,6 +415,8 @@ impl Default for AgentConfig { ongoing_deployment_tracking: "ongoing-deployment".to_string(), deploy_control_endpoint: None, s3_endpoint_override: None, + s3_download_max_concurrency: DEFAULT_S3_DOWNLOAD_MAX_CONCURRENCY, + s3_download_chunk_size: DEFAULT_S3_DOWNLOAD_CHUNK_SIZE, disable_imds_v1: false, enable_command_port: false, enable_archive_reuse: true, @@ -481,6 +510,25 @@ impl AgentConfig { ); self.kill_agent_max_wait_time_seconds = MAX_KILL_AGENT_WAIT_SECONDS; } + // Clamp the download settings rather than fail startup. + let concurrency = + self.s3_download_max_concurrency.clamp(1, MAX_S3_DOWNLOAD_MAX_CONCURRENCY); + if concurrency != self.s3_download_max_concurrency { + tracing::warn!( + configured = self.s3_download_max_concurrency, + using = concurrency, + "s3_download_max_concurrency out of range" + ); + self.s3_download_max_concurrency = concurrency; + } + if self.s3_download_chunk_size < MIN_S3_DOWNLOAD_CHUNK_SIZE { + tracing::warn!( + configured = self.s3_download_chunk_size, + using = MIN_S3_DOWNLOAD_CHUNK_SIZE, + "s3_download_chunk_size below minimum" + ); + self.s3_download_chunk_size = MIN_S3_DOWNLOAD_CHUNK_SIZE; + } } /// Validate post-parse fields that should fail loud rather than be clamped. @@ -1285,6 +1333,8 @@ disable_imds_v1: true enable_command_port: true disable_core_dumps: false log_aws_wire: true +s3_download_max_concurrency: 20 +s3_download_chunk_size: "16MiB" "#; let config = AgentConfig::from_yaml(yaml, Path::new("test.yml")).unwrap(); assert_eq!(config.program_name, "my-agent"); @@ -1309,6 +1359,87 @@ log_aws_wire: true assert!(config.enable_command_port); assert!(!config.disable_core_dumps); assert!(config.log_aws_wire); + assert_eq!(config.s3_download_max_concurrency, 20); + assert_eq!(config.s3_download_chunk_size, 16 * 1024 * 1024); + } + + #[test] + fn s3_download_defaults() { + let config = AgentConfig::default(); + assert_eq!(config.s3_download_max_concurrency, 1); + assert_eq!(config.s3_download_chunk_size, 8 * 1024 * 1024); + } + + #[test] + fn s3_download_settings_parse_symbol_keys() { + let config = AgentConfig::from_yaml( + ":s3_download_max_concurrency: 4\n:s3_download_chunk_size: 32MB\n", + Path::new("test.yml"), + ) + .unwrap(); + assert_eq!(config.s3_download_max_concurrency, 4); + assert_eq!(config.s3_download_chunk_size, 32_000_000); + assert!( + config.unknown_keys.is_empty(), + "download keys must not land in the unknown_keys catch-all: {:?}", + config.unknown_keys + ); + } + + #[test] + fn s3_download_chunk_size_accepts_raw_bytes() { + let config = + AgentConfig::from_yaml("s3_download_chunk_size: 1048576\n", Path::new("test.yml")) + .unwrap(); + assert_eq!(config.s3_download_chunk_size, 1_048_576); + } + + #[test] + fn s3_download_chunk_size_empty_value_uses_default() { + let config = + AgentConfig::from_yaml(":s3_download_chunk_size:\n", Path::new("test.yml")).unwrap(); + assert_eq!(config.s3_download_chunk_size, DEFAULT_S3_DOWNLOAD_CHUNK_SIZE); + } + + #[test] + fn s3_download_chunk_size_rejects_bad_suffix() { + assert!( + AgentConfig::from_yaml("s3_download_chunk_size: 5ZB\n", Path::new("test.yml")).is_err() + ); + } + + #[test] + fn normalize_clamps_s3_download_concurrency_high() { + let mut config = + AgentConfig { s3_download_max_concurrency: 10_000, ..AgentConfig::default() }; + config.normalize(); + assert_eq!(config.s3_download_max_concurrency, MAX_S3_DOWNLOAD_MAX_CONCURRENCY); + } + + #[test] + fn normalize_treats_zero_s3_download_concurrency_as_serial() { + let mut config = AgentConfig { s3_download_max_concurrency: 0, ..AgentConfig::default() }; + config.normalize(); + assert_eq!(config.s3_download_max_concurrency, 1); + } + + #[test] + fn normalize_floors_s3_download_chunk_size() { + let mut config = AgentConfig { s3_download_chunk_size: 512, ..AgentConfig::default() }; + config.normalize(); + assert_eq!(config.s3_download_chunk_size, MIN_S3_DOWNLOAD_CHUNK_SIZE); + } + + #[test] + fn normalize_leaves_valid_s3_download_settings_alone() { + let mut config = AgentConfig { + s3_download_max_concurrency: 16, + s3_download_chunk_size: 16 * 1024 * 1024, + ..AgentConfig::default() + }; + config.normalize(); + assert_eq!(config.s3_download_max_concurrency, 16); + assert_eq!(config.s3_download_chunk_size, 16 * 1024 * 1024); } #[test] diff --git a/src/daemon/worker.rs b/src/daemon/worker.rs index 2a051b3..d8b6c3a 100644 --- a/src/daemon/worker.rs +++ b/src/daemon/worker.rs @@ -174,6 +174,8 @@ pub fn run(shutdown: &ShutdownFlag, config: &AgentConfig) { wire_log: config .log_aws_wire .then(|| (config.log_dir.clone(), config.program_name.clone())), + download_max_concurrency: config.s3_download_max_concurrency, + download_chunk_size: config.s3_download_chunk_size, ..Default::default() }; let s3_client = match crate::aws_clients::s3_client::S3Client::new(s3_credentials, &s3_config) { @@ -183,17 +185,22 @@ pub fn run(shutdown: &ShutdownFlag, config: &AgentConfig) { return; }, }; - // S3Client is not Clone (tokio Runtime is not Clone), so we create a - // separate client for the update command. Both use the same credentials - // and config. - let update_s3_client = - match crate::aws_clients::s3_client::S3Client::new(credentials.clone(), &s3_config) { - Ok(c) => c, - Err(e) => { - error!("Worker failed to create update S3 client: {e}"); - return; - }, - }; + // The update command only fetches the install script, so its client + // downloads serially and starts no worker threads. + let update_s3_config = crate::aws_clients::s3_client::S3ClientConfig { + download_max_concurrency: 1, + ..s3_config.clone() + }; + let update_s3_client = match crate::aws_clients::s3_client::S3Client::new( + credentials.clone(), + &update_s3_config, + ) { + Ok(c) => c, + Err(e) => { + error!("Worker failed to create update S3 client: {e}"); + return; + }, + }; let region = credentials.region.clone(); let dispatcher = CommandDispatcher::new( archives, diff --git a/src/host_command/commands/update_agent.rs b/src/host_command/commands/update_agent.rs index 94fb0dc..ce80328 100644 --- a/src/host_command/commands/update_agent.rs +++ b/src/host_command/commands/update_agent.rs @@ -86,7 +86,7 @@ impl UpdateAgentCommand { let bucket = s3_bucket_name(&self.region); - // Download to a temp file (created 0600 by stream_to_file, auto-cleaned on drop). + // Download to a temp file (created 0600 by the S3 client, auto-cleaned on drop). let tmp_file = NamedTempFile::new()?; let tmp_path = tmp_file.path(); s3.download_to_file(&bucket, INSTALL_SCRIPT_KEY, None, tmp_path)?; diff --git a/src/main.rs b/src/main.rs index e7d033c..9e1124a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -721,6 +721,9 @@ fn run_update(config: &AgentConfig) { wire_log: config .log_aws_wire .then(|| (config.log_dir.clone(), config.program_name.clone())), + // Only fetches the small install script. + download_max_concurrency: 1, + download_chunk_size: config.s3_download_chunk_size, ..Default::default() }; let s3_client = match S3Client::new(credentials, &s3_config) { @@ -1098,6 +1101,8 @@ fn run_deploy_local_s3( wire_log: config .log_aws_wire .then(|| (config.log_dir.clone(), config.program_name.clone())), + download_max_concurrency: config.s3_download_max_concurrency, + download_chunk_size: config.s3_download_chunk_size, ..Default::default() }; let s3_client = match S3Client::new(credentials, &s3_config) { diff --git a/src/system/file_ops.rs b/src/system/file_ops.rs index 9f2e3db..fe94492 100644 --- a/src/system/file_ops.rs +++ b/src/system/file_ops.rs @@ -217,6 +217,40 @@ pub fn copy_dir_recursive(src: &Path, dest: &Path) -> io::Result<()> { Ok(()) } +/// Write all of `buf` to `file` at `offset`, so several threads can write +/// different regions of one handle. Uses `pwrite` on Unix and `seek_write` on +/// Windows; `seek_write` also moves the cursor, which is harmless because every +/// write passes its own offset. +/// +/// # Errors +/// Returns an error if the write fails or writes zero bytes. +pub fn write_all_at(file: &std::fs::File, buf: &[u8], offset: u64) -> io::Result<()> { + let mut buf = buf; + let mut offset = offset; + while !buf.is_empty() { + #[cfg(unix)] + let written = std::os::unix::fs::FileExt::write_at(file, buf, offset); + #[cfg(windows)] + let written = std::os::windows::fs::FileExt::seek_write(file, buf, offset); + + match written { + Ok(0) => { + return Err(io::Error::new( + io::ErrorKind::WriteZero, + "positioned write wrote zero bytes", + )); + }, + Ok(n) => { + buf = &buf[n..]; + offset += n as u64; + }, + Err(e) if e.kind() == io::ErrorKind::Interrupted => {}, + Err(e) => return Err(e), + } + } + Ok(()) +} + #[cfg(test)] #[derive(Debug)] pub struct MockFileOperations { @@ -444,4 +478,79 @@ mod tests { let mode = std::fs::metadata(dest.join("scripts")).unwrap().permissions().mode() & 0o777; assert_eq!(mode, 0o751, "expected 0751 preserved, got {mode:#o}"); } + + #[test] + fn write_all_at_writes_at_offset() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("out.bin"); + let file = std::fs::File::create(&path).unwrap(); + file.set_len(8).unwrap(); + write_all_at(&file, b"BBBB", 4).unwrap(); + write_all_at(&file, b"AAAA", 0).unwrap(); + drop(file); + assert_eq!(std::fs::read(&path).unwrap(), b"AAAABBBB"); + } + + #[test] + fn write_all_at_extends_file_and_zero_fills_gap() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("sparse.bin"); + let file = std::fs::File::create(&path).unwrap(); + // Writing past the end extends the file; the skipped region reads as zeros. + write_all_at(&file, b"Z", 3).unwrap(); + drop(file); + assert_eq!(std::fs::read(&path).unwrap(), b"\0\0\0Z"); + } + + #[test] + fn write_all_at_empty_buffer_is_noop() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("empty.bin"); + let file = std::fs::File::create(&path).unwrap(); + write_all_at(&file, b"", 0).unwrap(); + drop(file); + assert_eq!(std::fs::read(&path).unwrap().len(), 0); + } + + #[test] + fn write_all_at_read_only_handle_errors() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("ro.bin"); + std::fs::write(&path, b"data").unwrap(); + let file = std::fs::File::open(&path).unwrap(); + assert!(write_all_at(&file, b"x", 0).is_err()); + } + + #[test] + fn write_all_at_concurrent_ranges_do_not_interleave() { + use std::sync::Arc; + // Threads writing distinct regions of one handle produce the exact file. + let dir = TempDir::new().unwrap(); + let path = dir.path().join("parallel.bin"); + let chunk = 4096; + let threads = 8; + let file = Arc::new(std::fs::File::create(&path).unwrap()); + file.set_len((chunk * threads) as u64).unwrap(); + + let handles: Vec<_> = (0..threads) + .map(|i| { + let file = Arc::clone(&file); + std::thread::spawn(move || { + let buf = vec![b'a' + u8::try_from(i).unwrap(); chunk]; + write_all_at(&file, &buf, (i * chunk) as u64).unwrap(); + }) + }) + .collect(); + for h in handles { + h.join().unwrap(); + } + drop(file); + + let written = std::fs::read(&path).unwrap(); + assert_eq!(written.len(), chunk * threads); + for i in 0..threads { + let expected = vec![b'a' + u8::try_from(i).unwrap(); chunk]; + assert_eq!(&written[i * chunk..(i + 1) * chunk], &expected[..], "region {i} corrupted"); + } + } } diff --git a/src/system/mod.rs b/src/system/mod.rs index fe7d1aa..3495e96 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -16,7 +16,7 @@ pub mod version_file; #[cfg(unix)] pub use env_ops::DYNAMIC_LOADER_ENV_VARS; pub use env_ops::{EnvOps, SystemEnvOps}; -pub use file_ops::{PlatformFileOperations, SystemFileOperations, ensure_executable}; +pub use file_ops::{PlatformFileOperations, SystemFileOperations, ensure_executable, write_all_at}; #[cfg(target_os = "linux")] pub use linux_ops::{LinuxOps, SystemLinuxOps}; pub use process_ops::kill_process_group; diff --git a/tests/s3_parallel_download.rs b/tests/s3_parallel_download.rs new file mode 100644 index 0000000..fbb6355 --- /dev/null +++ b/tests/s3_parallel_download.rs @@ -0,0 +1,643 @@ +//! Tests for the parallel S3 downloader against a fake S3 endpoint that records +//! the requests it receives (count, ranges, peak in flight). Each response is +//! held briefly so concurrent requests really overlap. + +use std::collections::HashSet; +use std::io::{BufRead, BufReader, Write}; +use std::net::{TcpListener, TcpStream}; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::{Arc, Mutex}; +use std::time::Duration; + +use codedeploy_agent::aws_clients::credentials::{CredentialMode, Credentials}; +use codedeploy_agent::aws_clients::{S3Client, S3ClientConfig}; + +/// Position-dependent bytes, so a range at the wrong offset fails comparison. +fn object_bytes(len: usize) -> Vec { + (0..len).map(|i| u8::try_from(i % 251).unwrap()).collect() +} + +/// How the fake endpoint should behave. +#[derive(Clone, Copy, PartialEq)] +enum ServerMode { + /// Honor `Range` like S3 does. + Ranges, + /// Ignore `Range` and always return the whole object with `200`. + IgnoreRanges, + /// Answer any `Range` with `501 NotImplemented`. + RejectRanges, + /// Answer any `If-Match` with `412`, as if the object was replaced. + ObjectChanged, + /// Send only half of each range's body the first time it is requested. + TruncateFirstAttempt, + /// Send only half of every range's body. + TruncateAlways, + /// Honor the first range, then answer later ranges with the whole object. + IgnoreLaterRanges, + /// Like `RejectRanges`, but the error has no body. + BareRejectRanges, + /// Like `ObjectChanged`, but the error has no body. + BareObjectChanged, + /// Honor `Range` but send no `ETag`. + NoEtag, + /// Honor `Range` and add a wrong full-object checksum header to every 206. + BadChecksumHeader, +} + +/// What the fake endpoint saw. +#[derive(Default)] +struct Observations { + requests: usize, + unranged_requests: usize, + ranges: HashSet<(u64, u64)>, + /// Every range request, including repeats, so retries are countable. + range_request_log: Vec<(u64, u64)>, + peak_in_flight: usize, + if_match_values: HashSet, + /// Ranged requests that asked the SDK to validate response checksums. + ranged_checksum_mode_requests: usize, + /// Ranges already truncated once. + truncated: HashSet<(u64, u64)>, +} + +struct FakeS3 { + uri: String, + observations: Arc>, +} + +impl FakeS3 { + fn start(body: Vec, etag: &'static str, mode: ServerMode) -> Self { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind fake S3"); + let uri = format!("http://{}", listener.local_addr().unwrap()); + let observations = Arc::new(Mutex::new(Observations::default())); + + let body = Arc::new(body); + let obs = Arc::clone(&observations); + let in_flight = Arc::new(AtomicUsize::new(0)); + + // Detached; the listener lives until the test process exits. + std::thread::spawn(move || { + for stream in listener.incoming() { + let Ok(stream) = stream else { continue }; + let body = Arc::clone(&body); + let obs = Arc::clone(&obs); + let in_flight = Arc::clone(&in_flight); + std::thread::spawn(move || { + handle_connection(stream, &body, etag, mode, &obs, &in_flight); + }); + } + }); + + Self { uri, observations } + } + + fn observations(&self) -> std::sync::MutexGuard<'_, Observations> { + self.observations.lock().unwrap() + } +} + +fn handle_connection( + mut stream: TcpStream, + body: &[u8], + etag: &str, + mode: ServerMode, + obs: &Mutex, + in_flight: &AtomicUsize, +) { + stream.set_read_timeout(Some(Duration::from_secs(10))).ok(); + let Some(request) = read_request(&mut stream) else { + return; + }; + + let concurrent = in_flight.fetch_add(1, Ordering::SeqCst) + 1; + { + let mut obs = obs.lock().unwrap(); + obs.requests += 1; + obs.peak_in_flight = obs.peak_in_flight.max(concurrent); + if let Some(if_match) = &request.if_match { + obs.if_match_values.insert(if_match.clone()); + } + match request.range { + Some((start, end)) => { + obs.ranged_checksum_mode_requests += usize::from(request.checksum_mode); + obs.ranges.insert((start, end)); + obs.range_request_log.push((start, end)); + }, + None => obs.unranged_requests += 1, + } + } + + // Truncate each range once, so its retry succeeds. + let truncate = match (mode, request.range) { + (ServerMode::TruncateFirstAttempt, Some(range)) => { + obs.lock().unwrap().truncated.insert(range) + }, + (ServerMode::TruncateAlways, Some(_)) => true, + _ => false, + }; + + // Hold the response briefly so overlapping requests actually overlap. Leave + // the in-flight count before answering: once the client has the response it + // may start its next request before this thread runs again. + std::thread::sleep(Duration::from_millis(50)); + in_flight.fetch_sub(1, Ordering::SeqCst); + let _ = write_response(&mut stream, body, etag, mode, &request, truncate); +} + +struct Request { + range: Option<(u64, u64)>, + if_match: Option, + checksum_mode: bool, +} + +fn read_request(stream: &mut TcpStream) -> Option { + let mut reader = BufReader::new(stream.try_clone().ok()?); + let mut line = String::new(); + reader.read_line(&mut line).ok()?; + if line.trim().is_empty() { + return None; + } + + let mut range = None; + let mut if_match = None; + let mut checksum_mode = false; + loop { + let mut header = String::new(); + if reader.read_line(&mut header).ok()? == 0 || header.trim().is_empty() { + break; + } + let Some((name, value)) = header.split_once(':') else { + continue; + }; + let value = value.trim().to_string(); + match name.trim().to_ascii_lowercase().as_str() { + "range" => range = parse_range_header(&value), + "if-match" => if_match = Some(value), + "x-amz-checksum-mode" => checksum_mode = value.eq_ignore_ascii_case("enabled"), + _ => {}, + } + } + Some(Request { range, if_match, checksum_mode }) +} + +/// `bytes=0-262143` → `(0, 262143)`. +fn parse_range_header(value: &str) -> Option<(u64, u64)> { + let spec = value.trim().strip_prefix("bytes=")?; + let (start, end) = spec.split_once('-')?; + Some((start.trim().parse().ok()?, end.trim().parse().ok()?)) +} + +fn write_response( + stream: &mut TcpStream, + body: &[u8], + etag: &str, + mode: ServerMode, + request: &Request, + truncate: bool, +) -> std::io::Result<()> { + let total = body.len() as u64; + + let etag = (mode != ServerMode::NoEtag).then_some(etag); + let checksum = mode == ServerMode::BadChecksumHeader; + + if mode == ServerMode::ObjectChanged && request.if_match.is_some() { + return write_error(stream, 412, "Precondition Failed", Some("PreconditionFailed")); + } + if mode == ServerMode::BareObjectChanged && request.if_match.is_some() { + return write_error(stream, 412, "Precondition Failed", None); + } + if mode == ServerMode::BareRejectRanges && request.range.is_some() { + return write_error(stream, 501, "Not Implemented", None); + } + + match (mode, request.range) { + (ServerMode::RejectRanges, Some(_)) => { + write_error(stream, 501, "Not Implemented", Some("NotImplemented")) + }, + (ServerMode::IgnoreLaterRanges, Some((start, _))) if start > 0 => { + write_body(stream, 200, "OK", etag, body, None, false, false) + }, + (ServerMode::IgnoreRanges, _) | (_, None) => { + write_body(stream, 200, "OK", etag, body, None, false, false) + }, + (_, Some((start, end))) => { + // S3 answers any range on a zero-byte object, or a range that starts + // past the end, with 416/InvalidRange. + if start >= total { + return write_error( + stream, + 416, + "Requested Range Not Satisfiable", + Some("InvalidRange"), + ); + } + let last = end.min(total - 1); + let slice = &body[usize::try_from(start).unwrap()..=usize::try_from(last).unwrap()]; + let content_range = format!("bytes {start}-{last}/{total}"); + write_body( + stream, + 206, + "Partial Content", + etag, + slice, + Some(&content_range), + truncate, + checksum, + ) + }, + } +} + +/// `truncate` sends half the promised body and closes the connection. +/// `bad_checksum` adds a CRC32 header that does not match the body. +#[allow(clippy::too_many_arguments)] +fn write_body( + stream: &mut TcpStream, + status: u16, + reason: &str, + etag: Option<&str>, + body: &[u8], + content_range: Option<&str>, + truncate: bool, + bad_checksum: bool, +) -> std::io::Result<()> { + let mut head = format!( + "HTTP/1.1 {status} {reason}\r\nAccept-Ranges: bytes\r\n\ + Content-Length: {}\r\nConnection: close\r\n", + body.len() + ); + if let Some(etag) = etag { + head.push_str(&format!("ETag: \"{etag}\"\r\n")); + } + if bad_checksum { + head.push_str("x-amz-checksum-crc32: AAAAAA==\r\n"); + } + if let Some(cr) = content_range { + head.push_str(&format!("Content-Range: {cr}\r\n")); + } + head.push_str("\r\n"); + stream.write_all(head.as_bytes())?; + if truncate { + stream.write_all(&body[..body.len() / 2])?; + return stream.flush(); + } + stream.write_all(body)?; + stream.flush() +} + +/// An S3 XML error, or an empty body when `code` is `None`. +fn write_error( + stream: &mut TcpStream, + status: u16, + reason: &str, + code: Option<&str>, +) -> std::io::Result<()> { + let body = code.map_or_else(String::new, |code| { + format!( + "{code}\ + {reason}fake" + ) + }); + let head = format!( + "HTTP/1.1 {status} {reason}\r\nContent-Type: application/xml\r\n\ + Content-Length: {}\r\nConnection: close\r\n\r\n", + body.len() + ); + stream.write_all(head.as_bytes())?; + stream.write_all(body.as_bytes())?; + stream.flush() +} + +fn test_credentials() -> Credentials { + Credentials { + region: "us-east-1".into(), + host_identifier: "i-parallel-download".into(), + mode: CredentialMode::IamUser { + access_key_id: "AKIAIOSFODNN7EXAMPLE".into(), + secret_access_key: "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY".into(), + }, + } +} + +fn client_for(server: &FakeS3, max_concurrency: usize, chunk_size: u64) -> S3Client { + let config = S3ClientConfig { + endpoint_override: Some(server.uri.clone()), + download_max_concurrency: max_concurrency, + download_chunk_size: chunk_size, + ..Default::default() + }; + S3Client::new(test_credentials(), &config).expect("build S3 client") +} + +const CHUNK: u64 = 64 * 1024; + +#[test] +fn parallel_download_reassembles_object_exactly() { + // 10 chunks: 9 ranges fan out after the first request. + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 10); + let server = FakeS3::start(body.clone(), "bundleetag", ServerMode::Ranges); + let client = client_for(&server, 6, CHUNK); + + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("bundle.zip"); + let etag = client.download_to_file("bucket", "bundle.zip", None, &dest).expect("download"); + + assert_eq!( + etag.as_deref(), + Some("bundleetag"), + "ETag must be reported with quotes stripped" + ); + assert_eq!(std::fs::read(&dest).unwrap(), body, "downloaded bytes must match the object"); + + let obs = server.observations(); + assert_eq!(obs.ranges.len(), 10, "expected one request per chunk, saw {:?}", obs.ranges); + assert!( + obs.peak_in_flight > 1, + "downloads were serialized: peak in-flight requests was {}", + obs.peak_in_flight + ); + assert!( + obs.peak_in_flight <= 6, + "concurrency cap exceeded: {} requests in flight, limit 6", + obs.peak_in_flight + ); + // Follow-up ranges send If-Match with the ETag exactly as served (quoted). + assert_eq!( + obs.if_match_values.iter().collect::>(), + vec!["\"bundleetag\""], + "expected every follow-up range to send If-Match: \"bundleetag\", saw {:?}", + obs.if_match_values + ); +} + +#[test] +fn parallel_download_respects_concurrency_limit() { + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 8); + let server = FakeS3::start(body.clone(), "cap", ServerMode::Ranges); + let client = client_for(&server, 2, CHUNK); + + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("bundle.tgz"); + client.download_to_file("bucket", "bundle.tgz", None, &dest).expect("download"); + + assert_eq!(std::fs::read(&dest).unwrap(), body); + let obs = server.observations(); + assert!( + obs.peak_in_flight <= 2, + "max_concurrency=2 must never exceed 2 in flight, saw {}", + obs.peak_in_flight + ); +} + +#[test] +fn small_object_costs_a_single_request() { + // Smaller than one chunk: one request, no separate size probe. + let body = object_bytes(1024); + let server = FakeS3::start(body.clone(), "small", ServerMode::Ranges); + let client = client_for(&server, 10, CHUNK); + + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("small.zip"); + client.download_to_file("bucket", "small.zip", None, &dest).expect("download"); + + assert_eq!(std::fs::read(&dest).unwrap(), body); + assert_eq!(server.observations().requests, 1, "a small object must not cost extra requests"); +} + +#[test] +fn concurrency_one_uses_a_single_unranged_request() { + // Concurrency 1: one unranged GetObject. + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 4); + let server = FakeS3::start(body.clone(), "serial", ServerMode::Ranges); + let client = client_for(&server, 1, CHUNK); + + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("serial.zip"); + client.download_to_file("bucket", "serial.zip", None, &dest).expect("download"); + + assert_eq!(std::fs::read(&dest).unwrap(), body); + let obs = server.observations(); + assert_eq!(obs.requests, 1, "serial mode must issue exactly one request"); + assert_eq!(obs.unranged_requests, 1, "serial mode must not send a Range header"); +} + +#[test] +fn endpoint_that_rejects_ranges_falls_back_to_one_request() { + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 4); + let server = FakeS3::start(body.clone(), "norange", ServerMode::RejectRanges); + let client = client_for(&server, 8, CHUNK); + + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("fallback.zip"); + let etag = client + .download_to_file("bucket", "fallback.zip", None, &dest) + .expect("fallback download"); + + assert_eq!(etag.as_deref(), Some("norange")); + assert_eq!( + std::fs::read(&dest).unwrap(), + body, + "fallback must still produce the whole object" + ); + assert!( + server.observations().unranged_requests >= 1, + "expected an unranged retry after the endpoint refused ranges" + ); +} + +#[test] +fn endpoint_that_ignores_ranges_still_downloads_whole_object() { + // A 200 with the full body must not be truncated to the first chunk. + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 3); + let server = FakeS3::start(body.clone(), "ignored", ServerMode::IgnoreRanges); + let client = client_for(&server, 8, CHUNK); + + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("ignored.zip"); + client.download_to_file("bucket", "ignored.zip", None, &dest).expect("download"); + + assert_eq!(std::fs::read(&dest).unwrap(), body); +} + +#[test] +fn empty_object_downloads_via_fallback() { + // S3 answers any range on an empty object with 416, so this uses the fallback. + let server = FakeS3::start(Vec::new(), "empty", ServerMode::Ranges); + let client = client_for(&server, 8, CHUNK); + + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("empty.zip"); + client.download_to_file("bucket", "empty.zip", None, &dest).expect("download"); + + assert_eq!(std::fs::read(&dest).unwrap().len(), 0); +} + +#[test] +fn object_replaced_mid_download_fails_loudly() { + // If-Match must fail the download rather than mix two revisions. + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 4); + let server = FakeS3::start(body, "stale", ServerMode::ObjectChanged); + let client = client_for(&server, 4, CHUNK); + + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("changed.zip"); + let err = client + .download_to_file("bucket", "changed.zip", None, &dest) + .expect_err("a replaced object must not produce a spliced bundle"); + + let message = err.to_string(); + assert!( + message.contains("changed during the download"), + "error must name the cause, got: {message}" + ); + // PreconditionFailed is not retried: at most one request per chunk. + let obs = server.observations(); + assert!( + obs.requests <= 4, + "PreconditionFailed must not be retried, saw {} requests", + obs.requests + ); +} + +#[test] +fn range_body_that_dies_mid_stream_is_retried() { + // A body cut off mid-stream is retried (the SDK does not retry this). + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 4); + let server = FakeS3::start(body.clone(), "flaky", ServerMode::TruncateFirstAttempt); + let client = client_for(&server, 4, CHUNK); + + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("flaky.zip"); + client + .download_to_file("bucket", "flaky.zip", None, &dest) + .expect("retry must recover"); + + assert_eq!( + std::fs::read(&dest).unwrap(), + body, + "a retried range must overwrite its own offset exactly, leaving no stale half-chunk" + ); + + // Every range was truncated once, so each was requested at least twice. + let obs = server.observations(); + for range in &obs.ranges { + let attempts = obs.range_request_log.iter().filter(|r| *r == range).count(); + assert!(attempts >= 2, "range {range:?} was not retried (only {attempts} attempt(s))"); + } +} + +#[test] +fn versioned_download_reassembles_object() { + // Ranged downloads must keep threading versionId through every request. + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 5); + let server = FakeS3::start(body.clone(), "versioned", ServerMode::Ranges); + let client = client_for(&server, 5, CHUNK); + + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join("versioned.zip"); + client + .download_to_file("bucket", "versioned.zip", Some("abc123version"), &dest) + .expect("download"); + + assert_eq!(std::fs::read(&dest).unwrap(), body); +} + +fn download(server: &FakeS3, max_concurrency: usize, name: &str) -> (std::io::Result<()>, Vec) { + let client = client_for(server, max_concurrency, CHUNK); + let dir = tempfile::TempDir::new().unwrap(); + let dest = dir.path().join(name); + let result = client.download_to_file("bucket", name, None, &dest).map(|_| ()); + let bytes = std::fs::read(&dest).unwrap_or_default(); + (result, bytes) +} + +#[test] +fn later_range_answered_with_whole_object_fails_without_retrying() { + // A proxy that drops Range on later requests must not overwrite other ranges. + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 4); + let server = FakeS3::start(body, "later", ServerMode::IgnoreLaterRanges); + let (result, _) = download(&server, 4, "later.zip"); + + let message = result.expect_err("a 200 for a range must fail").to_string(); + assert!(message.contains("Content-Range"), "{message}"); + assert!(server.observations().requests <= 4, "must not retry a wrong response"); +} + +#[test] +fn bodiless_501_falls_back_to_one_request() { + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 3); + let server = FakeS3::start(body.clone(), "bare", ServerMode::BareRejectRanges); + let (result, bytes) = download(&server, 4, "bare.zip"); + + result.expect("a 501 with no body must fall back"); + assert_eq!(bytes, body); + assert_eq!(server.observations().unranged_requests, 1); +} + +#[test] +fn bodiless_412_fails_without_retrying() { + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 4); + let server = FakeS3::start(body, "bare412", ServerMode::BareObjectChanged); + let (result, _) = download(&server, 4, "bare412.zip"); + + let message = result.expect_err("a replaced object must fail").to_string(); + assert!(message.contains("changed during the download"), "{message}"); + assert!(server.observations().requests <= 4, "a 412 must not be retried"); +} + +#[test] +fn missing_etag_falls_back_to_one_request() { + // Nothing would detect the object changing between ranges. + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 3); + let server = FakeS3::start(body.clone(), "unused", ServerMode::NoEtag); + let (result, bytes) = download(&server, 4, "noetag.zip"); + + result.expect("download"); + assert_eq!(bytes, body); + let obs = server.observations(); + assert_eq!(obs.ranges.len(), 1, "only the first range should be requested"); + assert_eq!(obs.unranged_requests, 1); +} + +#[test] +fn full_object_checksum_header_on_a_range_is_ignored() { + // A full-object checksum cannot be checked against one range. + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 3); + let server = FakeS3::start(body.clone(), "crc", ServerMode::BadChecksumHeader); + let (result, bytes) = download(&server, 4, "crc.zip"); + + result.expect("ranged downloads must not validate full-object checksums"); + assert_eq!(bytes, body); + assert_eq!( + server.observations().ranged_checksum_mode_requests, + 0, + "ranged requests must turn off SDK response checksum validation" + ); +} + +#[test] +fn whole_object_of_exactly_one_chunk_is_downloaded_again() { + // No Content-Range and exactly one chunk could be a truncated range. + let body = object_bytes(usize::try_from(CHUNK).unwrap()); + let server = FakeS3::start(body.clone(), "exact", ServerMode::IgnoreRanges); + let (result, bytes) = download(&server, 4, "exact.zip"); + + result.expect("download"); + assert_eq!(bytes, body); + assert_eq!(server.observations().requests, 2); +} + +#[test] +fn range_that_keeps_failing_gives_up_after_three_attempts() { + let body = object_bytes(usize::try_from(CHUNK).unwrap() * 3); + let server = FakeS3::start(body, "broken", ServerMode::TruncateAlways); + let (result, _) = download(&server, 4, "broken.zip"); + + result.expect_err("a range that never completes must fail the download"); + let obs = server.observations(); + for range in &obs.ranges { + let attempts = obs.range_request_log.iter().filter(|r| *r == range).count(); + // The first range is also requested once by the size probe. + assert!(attempts <= 4, "range {range:?} requested {attempts} times"); + } +} From df733f96cfd4b312386211c7890fd3a324577146 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Tue, 13 Oct 2026 08:40:14 +0000 Subject: [PATCH 09/13] Bump the agent version to 2.1.2 Add CHANGELOG entries for 2.0.1, 2.1.0 and 2.1.2. --- CHANGELOG.md | 35 +++++++++++++++++++++++++++ Cargo.toml | 2 +- crates/codedeploy-commands/Cargo.toml | 2 +- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1408026..42e1627 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,38 @@ +## 2.1.2 + +### Added + +* **macOS support** on EC2 Mac, macOS 15 and later (aarch64 and x86_64). The agent ships as a `.pkg` and runs as a launchd LaunchDaemon. Deployments use AppSpec `os: linux`. AppSpec `permissions` entries that set `acls` or `context` are rejected before the install runs; `owner`, `group` and `mode` apply. Agent self-update is not available on macOS yet. +* **Optional parallel S3 bundle downloads.** Setting `s3_download_max_concurrency` above 1 (max 64) downloads a bundle in parallel HTTP range requests of `s3_download_chunk_size` bytes (default 8MiB, minimum 64KiB). Every range is pinned to the object's ETag, or to its version when one is set, so an object replaced mid-download fails the deployment. The default of 1 keeps the single-request download. + +### Changed + +* **The deployment log is written as script output is produced.** Each `scripts.log` line is also written to the deployment log as `[] []`, replacing the 2048-byte tail copied after each lifecycle event. Older output is no longer lost, the failure-summary line is removed, and blank script output lines are logged as a bare `[stdout]` record in both files. + +### Fixed + +* **Command endpoints in the European Sovereign Cloud and ISO partitions.** Hosts in `eusc-*`, `us-iso-*`, `us-isob-*`, `eu-isoe-*` and `us-isof-*` regions resolved an `amazonaws.com` endpoint that does not exist. Each partition now uses its own DNS suffix. + +### Security + +* **(opt-in)** On macOS, `strip_loader_env_in_hooks` strips the `DYLD_*` loader variables from hook environments. + +## 2.1.0 + +### Added + +* **`RESTART` deployment mode.** The agent installs from the copy of the revision from the deployment group's last successful deployment already on the instance. If that copy is missing or incomplete, the agent downloads the revision. + +### Security + +* A deployment whose AppSpec file path resolves outside the application revision directory is rejected. Nested paths such as `configs/appspec.yml` are still supported. + +## 2.0.1 + +### Fixed + +* **Windows AppSpec paths with leading separators.** A `files.source` or `hooks.location` beginning with `\` (including the bare `\` form) or `/` resolved against the drive root instead of the deployment archive, failing Install events with access-denied errors. Leading separators are now stripped. + ## 2.0.0 Version 2.0.0 replaces the Ruby runtime with a single statically linked binary in Rust, completely removing the dependency on a Ruby runtime. diff --git a/Cargo.toml b/Cargo.toml index 9202a6e..ab6bb07 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.0" +version = "2.1.2" 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 1453066..c67f324 100644 --- a/crates/codedeploy-commands/Cargo.toml +++ b/crates/codedeploy-commands/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codedeploy-commands" -version = "2.1.1" +version = "2.1.2" edition = "2024" rust-version = "1.91" publish = false From 9ee7ef4ecab8c44da562c8bcd5acdefaa96eb5a6 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Wed, 14 Oct 2026 11:29:10 +0000 Subject: [PATCH 10/13] Limit documented macOS support to Apple silicon --- CHANGELOG.md | 2 +- DEVELOPMENT.md | 12 +++++------- README.md | 6 +++--- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42e1627..f7dae02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### Added -* **macOS support** on EC2 Mac, macOS 15 and later (aarch64 and x86_64). The agent ships as a `.pkg` and runs as a launchd LaunchDaemon. Deployments use AppSpec `os: linux`. AppSpec `permissions` entries that set `acls` or `context` are rejected before the install runs; `owner`, `group` and `mode` apply. Agent self-update is not available on macOS yet. +* **macOS support** on EC2 Mac, macOS 15 and later (Apple silicon, aarch64). The agent ships as a `.pkg` and runs as a launchd LaunchDaemon. Deployments use AppSpec `os: linux`. AppSpec `permissions` entries that set `acls` or `context` are rejected before the install runs; `owner`, `group` and `mode` apply. Agent self-update is not available on macOS yet. * **Optional parallel S3 bundle downloads.** Setting `s3_download_max_concurrency` above 1 (max 64) downloads a bundle in parallel HTTP range requests of `s3_download_chunk_size` bytes (default 8MiB, minimum 64KiB). Every range is pinned to the object's ETag, or to its version when one is set, so an object replaced mid-download fails the deployment. The default of 1 keeps the single-request download. ### Changed diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 7d2164b..44b16e1 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -321,21 +321,19 @@ cargo build --release ## Building on macOS -Targets: `aarch64-apple-darwin` (Apple silicon) and `x86_64-apple-darwin` -(Intel). Both build on an Apple-silicon Mac with the Xcode command-line tools; -there is no Intel-only step. `MACOSX_DEPLOYMENT_TARGET=15.0` matches the -oldest supported macOS (15 Sequoia). +Target: `aarch64-apple-darwin` (Apple silicon), built with the Xcode +command-line tools. `MACOSX_DEPLOYMENT_TARGET=15.0` matches the oldest +supported macOS (15 Sequoia). ```bash xcode-select --install -rustup target add aarch64-apple-darwin x86_64-apple-darwin +rustup target add aarch64-apple-darwin export MACOSX_DEPLOYMENT_TARGET=15.0 cargo build --release --target aarch64-apple-darwin -cargo build --release --target x86_64-apple-darwin ``` -Package each binary as a `.pkg` (payload: binary, `codedeploy-local` symlink, +Package the binary as a `.pkg` (payload: binary, `codedeploy-local` symlink, config template, LaunchDaemon plist; see `macos/pkg/`): ```bash diff --git a/README.md b/README.md index d31204c..2965801 100644 --- a/README.md +++ b/README.md @@ -29,12 +29,12 @@ Windows ship as rpm/deb and msi; macOS ships as a `.pkg` (see | Debian | 11, 12, 13 | x86_64, aarch64 | systemd | | Ubuntu Server | 16.04–26.04 LTS, 25.04 | x86_64, aarch64 | systemd | | Windows Server | 2016, 2019, 2022, 2025 | x86_64 | Service Control Manager | -| macOS (EC2 Mac) | 15 Sequoia, 26 Tahoe, 27 | aarch64 (all); x86_64 (15) | launchd | +| macOS (EC2 Mac) | 15 Sequoia, 26 Tahoe, 27 | aarch64 | launchd | macOS notes: - Supported macOS versions are the current major release and the two before - it, reviewed each September. On EC2, Tahoe (26) and later are Apple silicon - only; `mac1.metal` (x86_64) tops out at 15. + it, reviewed each September. Only Apple silicon (aarch64) EC2 Mac instances + are supported. - AppSpec `os: linux` is used for macOS deployments (`os: macos` is not a valid value). - AppSpec `permissions` fields `acls` and `context` (SELinux) are rejected on From 89dc6fa75c38ca52d8e086354a071e72722af1e8 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Wed, 14 Oct 2026 11:37:08 +0000 Subject: [PATCH 11/13] List the deployment log change as a fix for #430 --- CHANGELOG.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f7dae02..b2ef7d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,12 +5,9 @@ * **macOS support** on EC2 Mac, macOS 15 and later (Apple silicon, aarch64). The agent ships as a `.pkg` and runs as a launchd LaunchDaemon. Deployments use AppSpec `os: linux`. AppSpec `permissions` entries that set `acls` or `context` are rejected before the install runs; `owner`, `group` and `mode` apply. Agent self-update is not available on macOS yet. * **Optional parallel S3 bundle downloads.** Setting `s3_download_max_concurrency` above 1 (max 64) downloads a bundle in parallel HTTP range requests of `s3_download_chunk_size` bytes (default 8MiB, minimum 64KiB). Every range is pinned to the object's ETag, or to its version when one is set, so an object replaced mid-download fails the deployment. The default of 1 keeps the single-request download. -### Changed - -* **The deployment log is written as script output is produced.** Each `scripts.log` line is also written to the deployment log as `[] []`, replacing the 2048-byte tail copied after each lifecycle event. Older output is no longer lost, the failure-summary line is removed, and blank script output lines are logged as a bare `[stdout]` record in both files. - ### Fixed +* **Deployment log format matches the 1.x agent ([#430](https://github.com/aws/aws-codedeploy-agent/issues/430)).** Each `scripts.log` line is now written to `codedeploy-agent-deployments.log` as it is produced, as `[] []`. Previously the agent copied the last 2048 bytes of script output into the log after each lifecycle event, which dropped the deployment ID, doubled the timestamp, added a blank line after each record, and lost `LifecycleEvent` headers and any older output. The failure-summary line is removed, and blank script output lines are logged as a bare `[stdout]` record in both files. * **Command endpoints in the European Sovereign Cloud and ISO partitions.** Hosts in `eusc-*`, `us-iso-*`, `us-isob-*`, `eu-isoe-*` and `us-isof-*` regions resolved an `amazonaws.com` endpoint that does not exist. Each partition now uses its own DNS suffix. ### Security From 3ca2a97a51ff0dc7e974e5962e2f65634f5bd988 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Wed, 14 Oct 2026 11:38:09 +0000 Subject: [PATCH 12/13] Remove the macOS notes from the README --- README.md | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/README.md b/README.md index 2965801..74b4423 100644 --- a/README.md +++ b/README.md @@ -31,17 +31,6 @@ Windows ship as rpm/deb and msi; macOS ships as a `.pkg` (see | Windows Server | 2016, 2019, 2022, 2025 | x86_64 | Service Control Manager | | macOS (EC2 Mac) | 15 Sequoia, 26 Tahoe, 27 | aarch64 | launchd | -macOS notes: -- Supported macOS versions are the current major release and the two before - it, reviewed each September. Only Apple silicon (aarch64) EC2 Mac instances - are supported. -- AppSpec `os: linux` is used for macOS deployments (`os: macos` is not a - valid value). -- AppSpec `permissions` fields `acls` and `context` (SELinux) are rejected on - macOS before the install runs; `owner`, `group` and `mode` work. -- Agent self-update (`UpdateDeploymentAgent`, `codedeploy-agent update`) is - not available on macOS yet; install the latest `.pkg` instead. - ## Getting Started ### Prerequisites From e444ff0a25fd09f5a50a6d53abfd6b9440f8c5b6 Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Wed, 14 Oct 2026 11:39:14 +0000 Subject: [PATCH 13/13] Remove the partition DNS suffix doc comment --- crates/codedeploy-commands/src/endpoint.rs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/crates/codedeploy-commands/src/endpoint.rs b/crates/codedeploy-commands/src/endpoint.rs index 1844054..f0882f8 100644 --- a/crates/codedeploy-commands/src/endpoint.rs +++ b/crates/codedeploy-commands/src/endpoint.rs @@ -27,25 +27,6 @@ pub fn resolve( format!("https://{service}.{region}.{}", partition_dns_suffix(region)) } -/// Map a region to its partition's DNS suffix. -/// -/// The default AWS partition uses `amazonaws.com`; other partitions have -/// distinct suffixes and are matched by their region-name prefix: -/// - `cn-*` (China) -> `amazonaws.com.cn` -/// - `eusc-*` (European Sovereign Cloud) -> `amazonaws.eu` -/// - `us-iso-*` (ISO) -> `c2s.ic.gov` -/// - `us-isob-*` (ISO-B) -> `sc2s.sgov.gov` -/// - `eu-isoe-*` (ISO-E) -> `cloud.adc-e.uk` -/// - `us-isof-*` (ISO-F) -> `csp.hci.ic.gov` -/// -/// `GovCloud` (`us-gov-*`) intentionally stays on `amazonaws.com`. -/// -/// NOTE: this prefix table is a stopgap. The legacy Ruby agent read the domain -/// directly from IMDS (`/latest/meta-data/services/domain`), which resolves -/// every partition without hardcoding. Once IMDS domain resolution lands, the -/// non-commercial entries here should be removed. The ISO prefixes are -/// mutually disjoint (`us-iso-`, `us-isob-`, `us-isof-` never overlap), so -/// match order is not significant. fn partition_dns_suffix(region: &str) -> &'static str { if region.starts_with("cn-") { "amazonaws.com.cn"