From 7c783e8fa83ac80707f6cbaa79e570d6c2f5e817 Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Mon, 27 Jul 2026 12:54:24 -0700 Subject: [PATCH 1/3] test(sessions): run POSIX cross-user tests on macOS in CI Validates cross-user Sessions on macOS (issue #263). No code changes were needed: the crate's mechanism (process_group(0) at spawn, killpg for signalling, no setsid(1) binary, no pgrep, no procfs) is pure POSIX syscalls, all present on macOS. All 16 cross-user integration tests pass on a macOS host once the environment is provisioned correctly. Adds a cross-user-macos CI job: macOS runners have no Docker but do have passwordless sudo, so the same user/group layout as the Linux test containers is provisioned directly on the host with Directory Services (sysadminctl/dseditgroup). Two macOS-specific provisioning requirements, documented in specs/sessions/cross-user-testing.md: - The default per-user TMPDIR (/var/folders//T, mode 0700) is not traversable by the target user, so sudo cannot reach the extracted helper binary ("Helper process closed stdout unexpectedly"). The job sets a world-traversable TMPDIR. This constraint applies to real deployments' session roots as well. - macOS does not create a self-named group per user the way useradd does; the tempdir cleanup test chowns to user:user, so the group is created explicitly. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- .github/workflows/ci.yml | 140 +++++++++++++++++ DEVELOPMENT.md | 5 +- crates/openjd-sessions/src/tempdir.rs | 88 +++++++++-- .../tests/integration/test_cross_user.rs | 145 +++++++++++++++--- specs/sessions/cross-user-testing.md | 49 +++++- 5 files changed, 393 insertions(+), 34 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2da222c4..26dc2b05 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -352,6 +352,146 @@ jobs: bash scripts/run_cross_user_tests.sh fi + # macOS has no Docker, but the runner has passwordless sudo, so the test + # users/groups are provisioned directly on the host with Directory Services + # (the macOS analog of the useradd/groupadd calls in the Linux containers). + cross-user-macos: + name: Cross-User Tests (macOS) + runs-on: macos-latest + timeout-minutes: 30 + env: + OPENJD_TEST_SUDO_TARGET_USER: openjd-target + OPENJD_TEST_SUDO_SHARED_GROUP: openjd-shared + OPENJD_TEST_SUDO_DISJOINT_USER: openjd-disjoint + OPENJD_TEST_SUDO_DISJOINT_GROUP: openjd-disjointgrp + # The default per-user TMPDIR (/var/folders//T) is mode 0700, so the + # target user cannot traverse to the session working directory or execute + # the extracted helper binary inside it. Tests need a world-traversable + # temp root (real deployments use a shared session root such as + # /var/lib/deadline/sessions for the same reason). Job-scoped, so it must + # be created before any step that writes temp files -- see the first step. + TMPDIR: /private/tmp/openjd-tests + steps: + - name: Create temp root + # TMPDIR is exported to every step; the directory must exist before + # checkout/rustup/cache write their first temp file. + run: | + sudo mkdir -p "${TMPDIR}" + sudo chmod 1777 "${TMPDIR}" + # Record the default per-user TMPDIR this runner would otherwise use, + # so the override is self-documenting: the tests rely on it being + # world-traversable, unlike the default /var/folders//T (0700). + echo "default TMPDIR mode: $(stat -f '%Lp %N' "${TMPDIR:-/tmp}" 2>/dev/null || true)" + echo "runner default temp: ${RUNNER_TEMP:-unset}" + - uses: actions/checkout@v7 + - name: Install rust stable + run: | + rustup toolchain install stable --profile minimal + rustup override set stable + - uses: actions/cache@v6 + with: + path: | + ~/.cargo/registry/index + ~/.cargo/registry/cache + ~/.cargo/git/db + target + crates/openjd-sessions/src/helper/target + key: rust-cross-user-macos-${{ hashFiles('**/Cargo.lock') }} + restore-keys: | + rust-cross-user-macos- + - name: Provision test users and groups + run: | + set -euxo pipefail + + sudo dseditgroup -o create "${OPENJD_TEST_SUDO_SHARED_GROUP}" + sudo dseditgroup -o create "${OPENJD_TEST_SUDO_DISJOINT_GROUP}" + + # Target user: impersonated by the tests; shares a group with runner. + sudo sysadminctl -addUser "${OPENJD_TEST_SUDO_TARGET_USER}" \ + -fullName "OpenJD Test Target" -password "OpenJD-ci-test-1!" -shell /bin/zsh + sudo createhomedir -c -u "${OPENJD_TEST_SUDO_TARGET_USER}" > /dev/null + sudo dseditgroup -o edit -a "${OPENJD_TEST_SUDO_TARGET_USER}" -t user "${OPENJD_TEST_SUDO_SHARED_GROUP}" + # Linux useradd gives every user a self-named group and the tempdir + # cleanup test chowns to "user:user"; macOS does not, so create it. + sudo dseditgroup -o create "${OPENJD_TEST_SUDO_TARGET_USER}" + sudo dseditgroup -o edit -a "${OPENJD_TEST_SUDO_TARGET_USER}" -t user "${OPENJD_TEST_SUDO_TARGET_USER}" + + # Disjoint user: shares no relevant group with the runner. What the + # disjoint test exercises is that the runner is NOT a member of the + # DISJOINT_GROUP that TempDir chowns to; on macOS both accounts are + # unavoidably in `staff` and Apple's implicit groups (everyone, + # localaccounts, ...), so make the disjoint user's PRIMARY group the + # disjoint group rather than staff to keep the setup as close to the + # Linux container's (truly disjoint) layout as macOS allows. + sudo sysadminctl -addUser "${OPENJD_TEST_SUDO_DISJOINT_USER}" \ + -fullName "OpenJD Test Disjoint" -password "OpenJD-ci-test-1!" -shell /bin/zsh + sudo createhomedir -c -u "${OPENJD_TEST_SUDO_DISJOINT_USER}" > /dev/null + sudo dseditgroup -o edit -a "${OPENJD_TEST_SUDO_DISJOINT_USER}" -t user "${OPENJD_TEST_SUDO_DISJOINT_GROUP}" + disjoint_gid="$(dscl . -read "/Groups/${OPENJD_TEST_SUDO_DISJOINT_GROUP}" PrimaryGroupID | awk '{print $2}')" + sudo dscl . -create "/Users/${OPENJD_TEST_SUDO_DISJOINT_USER}" PrimaryGroupID "${disjoint_gid}" + # NOTE: no `dseditgroup -d ... staff` here — sysadminctl places users in + # staff via PrimaryGroupID, not a member record, and the reassignment + # above has already moved them off it. A delete would be a no-op. + + # The test-running user joins the shared group (matches the Docker layout). + sudo dseditgroup -o edit -a runner -t user "${OPENJD_TEST_SUDO_SHARED_GROUP}" + + # Passwordless sudo from runner to the test users, mirroring the + # hostuser rule in the Linux containers. + echo "runner ALL=(${OPENJD_TEST_SUDO_TARGET_USER},${OPENJD_TEST_SUDO_DISJOINT_USER}) NOPASSWD: ALL" | sudo tee /etc/sudoers.d/openjd-cross-user-tests + sudo chmod 440 /etc/sudoers.d/openjd-cross-user-tests + sudo visudo -cf /etc/sudoers.d/openjd-cross-user-tests + + sudo dscacheutil -flushcache + - name: Verify provisioning + run: | + set -euxo pipefail + id "${OPENJD_TEST_SUDO_TARGET_USER}" + id "${OPENJD_TEST_SUDO_DISJOINT_USER}" + # Check membership at the PROCESS-CREDENTIAL level, not just the + # directory record: bare `id -Gn` reports this step's own kernel group + # set. On macOS, new processes resolve supplementary groups through + # Directory Services at spawn, so steps created after provisioning pick + # up the membership even though the parent Runner.Worker process + # predates it -- this asserts that stays true (the test step below + # inherits its groups the same way this step does). + id -Gn | tr ' ' '\n' | grep -qx "${OPENJD_TEST_SUDO_SHARED_GROUP}" + id -Gn runner | tr ' ' '\n' | grep -qx "${OPENJD_TEST_SUDO_SHARED_GROUP}" + if id -Gn "${OPENJD_TEST_SUDO_DISJOINT_USER}" | tr ' ' '\n' | grep -qx "${OPENJD_TEST_SUDO_SHARED_GROUP}"; then + echo "disjoint user must not be in the shared group" && exit 1 + fi + # The property the disjoint tempdir test relies on: the runner is not + # a member of the disjoint group (TempDir chowns to it and must fail). + if id -Gn runner | tr ' ' '\n' | grep -qx "${OPENJD_TEST_SUDO_DISJOINT_GROUP}"; then + echo "runner must not be in the disjoint group" && exit 1 + fi + # And the disjoint user's primary group is the disjoint group, not staff. + test "$(id -gn "${OPENJD_TEST_SUDO_DISJOINT_USER}")" = "${OPENJD_TEST_SUDO_DISJOINT_GROUP}" + sudo -u "${OPENJD_TEST_SUDO_TARGET_USER}" -i /usr/bin/true + # The sibling Windows modules are cfg-gated off on unix, so selecting the + # test_cross_user:: module path runs exactly the POSIX cross-user tests. + - name: Run cross-user tests + run: | + set -euo pipefail + # libtest exits 0 when a filter matches nothing, so a future rename or + # cfg change to the test module would silently turn this job green. + # Assert the filter still selects tests before running them. + count=$(cargo test -p openjd-sessions --features test-utils --test integration -- \ + --include-ignored --list test_cross_user:: 2>/dev/null | grep -c ': test$' || true) + echo "cross-user tests matched: ${count}" + if [ "${count}" -eq 0 ]; then + echo "ERROR: test_cross_user:: filter matched no tests — the module may have been renamed or cfg'd out." >&2 + exit 1 + fi + cargo test -p openjd-sessions --features test-utils --test integration -- \ + --include-ignored --test-threads=1 test_cross_user:: + - name: Remove test users and sudoers rule + if: always() + run: | + sudo rm -f /etc/sudoers.d/openjd-cross-user-tests || true + sudo sysadminctl -deleteUser "${OPENJD_TEST_SUDO_TARGET_USER}" 2>/dev/null || true + sudo sysadminctl -deleteUser "${OPENJD_TEST_SUDO_DISJOINT_USER}" 2>/dev/null || true + # Reuses the Windows build-test cache via a restore-only step so we don't # pay another 3+ minutes recompiling openjd-expr/model/sessions. The # test-utils feature used here isn't enabled in build-test, so cargo will diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index fbde61d3..20ccce31 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -16,8 +16,9 @@ project was originally ported from Python and the prompts used, see 2. `cargo` (included with the Rust toolchain). 3. Nightly rustfmt for formatting checks (`rustup toolchain install nightly`). -Linux, macOS, and Windows all work. Some sessions tests require Docker (Linux) or a test -user account (Windows) — see `specs/sessions/cross-user-testing.md`. +Linux, macOS, and Windows all work. Some sessions tests require Docker (Linux), locally +provisioned test users (macOS), or a test user account (Windows) — see +`specs/sessions/cross-user-testing.md`. ## The three artifacts diff --git a/crates/openjd-sessions/src/tempdir.rs b/crates/openjd-sessions/src/tempdir.rs index 62b18591..2f6b5530 100644 --- a/crates/openjd-sessions/src/tempdir.rs +++ b/crates/openjd-sessions/src/tempdir.rs @@ -169,6 +169,26 @@ impl TempDir { source: e, })?; + // Ownership/permission setup after this point can fail (e.g. chown to a + // group the process user is not a member of). The TempDir struct — and + // its Drop-based cleanup — does not exist yet, so on any such failure + // remove the just-created directory rather than orphaning it. + if let Err(e) = Self::setup_permissions(&path, _user) { + let _ = std::fs::remove_dir_all(&path); + return Err(e); + } + + Ok(Self { + path, + cleaned_up: false, + }) + } + + /// Apply ownership and permissions to a freshly-created session directory. + /// + /// Split out of [`TempDir::new`] so that a failure here can trigger removal + /// of the directory (the caller owns that cleanup). + fn setup_permissions(path: &Path, _user: Option<&dyn SessionUser>) -> Result<(), SessionError> { #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -176,7 +196,7 @@ impl TempDir { // Cross-user: chown group then set 0o770 // chown before chmod — security: don't grant group access if chown fails if let Ok(Some(grp)) = nix::unistd::Group::from_name(u.group()) { - nix::unistd::chown(&path, None, Some(grp.gid)).map_err(|e| { + nix::unistd::chown(path, None, Some(grp.gid)).map_err(|e| { SessionError::PathPermissions { path: path.display().to_string(), reason: format!( @@ -190,12 +210,12 @@ impl TempDir { } else { 0o700 }; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode)).map_err( - |e| SessionError::TempDir { - path: path.clone(), + std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)).map_err(|e| { + SessionError::TempDir { + path: path.to_path_buf(), source: e, - }, - )?; + } + })?; } // Windows: set DACL — full control for process user, modify for session user. @@ -225,10 +245,7 @@ impl TempDir { } } - Ok(Self { - path, - cleaned_up: false, - }) + Ok(()) } pub fn path(&self) -> &Path { @@ -317,6 +334,57 @@ mod tests { assert_eq!(result, None); } + /// A cross-user TempDir under a parent that the session user cannot traverse + /// (e.g. a 0700 directory such as macOS's default per-user `$TMPDIR`, + /// `/var/folders//T`) is created by the process user but is unusable: + /// `sudo -u -i ` cannot reach the helper binary and the + /// working directory cannot be entered. This test pins TODAY'S behavior — + /// creation currently *succeeds* (the traversability of ancestors is not + /// checked) — so that both a regression and a future pre-flight + /// traversability check become visible here. `find_missing_sticky_bit` + /// only checks the opposite direction (world-writable WITHOUT sticky), so + /// it does not catch this. + /// + /// Requires a non-root process user (root traverses any directory), so it + /// is ignored by default and run explicitly. + #[cfg(unix)] + #[test] + #[ignore = "documents un-traversable-parent behavior; run explicitly as non-root"] + fn tempdir_under_untraversable_parent_is_currently_created() { + use std::os::unix::fs::PermissionsExt; + + if nix::unistd::geteuid().is_root() { + eprintln!("skipping: root traverses any directory regardless of mode"); + return; + } + + let tmp = tempfile::TempDir::new().unwrap(); + let private = tmp.path().join("private"); + std::fs::create_dir(&private).unwrap(); + std::fs::set_permissions(&private, std::fs::Permissions::from_mode(0o700)).unwrap(); + + // Same-user TempDir under the 0700 parent: creation succeeds today. + // No ancestor o+x/g+x (traversability) check is performed. + let td = TempDir::new(Some(&private), Some("untraversable-"), None) + .expect("TempDir::new currently succeeds even under a 0700 parent"); + assert!(td.path().exists()); + // The un-traversability is a property of the 0700 PARENT, which a + // different (session) user could not enter -- asserted directly here + // since this test runs single-user. + let parent_mode = std::fs::metadata(&private).unwrap().permissions().mode() & 0o777; + assert_eq!( + parent_mode, + 0o700, + "the parent is owner-only; another user cannot traverse it to reach {}", + td.path().display() + ); + assert!( + find_missing_sticky_bit(td.path()).is_none(), + "sticky-bit policy does not (and is not meant to) flag an \ + un-traversable ancestor" + ); + } + /// Mirrors Python TestTempDirWindows::test_windows_temp_dir — verifies the /// warning when `PROGRAMDATA` is unset. /// diff --git a/crates/openjd-sessions/tests/integration/test_cross_user.rs b/crates/openjd-sessions/tests/integration/test_cross_user.rs index fa6bfe88..9f200591 100644 --- a/crates/openjd-sessions/tests/integration/test_cross_user.rs +++ b/crates/openjd-sessions/tests/integration/test_cross_user.rs @@ -23,6 +23,7 @@ use openjd_sessions::action::ActionState; use openjd_sessions::session::{Session, SessionCancelHandle, SessionConfig}; use openjd_sessions::session_user::PosixSessionUser; use openjd_sessions::tempdir::TempDir; +use openjd_sessions::SessionError; fn target_user() -> Option> { let user = std::env::var("OPENJD_TEST_SUDO_TARGET_USER").ok()?; @@ -121,18 +122,27 @@ async fn test_cross_user_subprocess_notify() { "Expected Timeout or Failed, got {:?}", r.state ); + // NOTE: run_subprocess's timeout uses CancelMethod::Terminate (immediate + // SIGKILL), so the workload's SIGTERM trap is intentionally NOT expected to + // fire here — asserting "Trapped" would be wrong. Assert instead that the + // workload ran as the target user and was cut off mid-flight (distinguishing + // a real kill from a failure-to-launch, which also yields Failed). The + // SIGTERM/notify path is covered by test_cross_user_notify_delivers_sigterm. assert!( r.stdout.contains("Log from test 0"), - "Should see early output" + "Should see early output; stdout: {:?}", + r.stdout ); assert!( !r.stdout.contains("Log from test 19"), - "Should not complete all iterations" + "Should not complete all iterations; stdout: {:?}", + r.stdout ); session.cleanup(); } -/// Run long_running.sh with timeout — SIGKILL cannot be trapped. +/// Run long_running_ignore.sh with timeout — the workload traps SIGTERM but does +/// not exit, so the runner must escalate to SIGKILL (which cannot be trapped). #[tokio::test(flavor = "multi_thread")] #[ignore] async fn test_cross_user_subprocess_terminate() { @@ -155,6 +165,20 @@ async fn test_cross_user_subprocess_terminate() { "Expected Timeout or Failed, got {:?}", r.state ); + // Assert the workload actually ran and was cut off mid-flight, rather than + // failing to launch as the target user (which would also yield `Failed` and + // is otherwise indistinguishable from a successful kill). The script prints + // bare iteration numbers. + assert!( + r.stdout.contains('0'), + "Should see early output; stdout: {:?}", + r.stdout + ); + assert!( + !r.stdout.contains("19"), + "SIGKILL should have stopped the loop before completion; stdout: {:?}", + r.stdout + ); session.cleanup(); } @@ -181,13 +205,26 @@ async fn test_cross_user_subprocess_terminate_tree() { "Expected Timeout or Failed, got {:?}", r.state ); + // spawn_child.sh launches long_running.sh as a child ("Log from test N") + // then runs its own loop ("Log from runner N"). Seeing the child's early + // output confirms the whole tree launched; neither loop completing confirms + // the process-GROUP kill reached both parent and child, rather than only the + // parent dying and the child being orphaned (which is the failure mode a + // pgid-based kill exists to prevent). assert!( r.stdout.contains("Log from test 0"), - "Should see early output" + "Child process should have produced early output; stdout: {:?}", + r.stdout ); assert!( !r.stdout.contains("Log from test 19"), - "Should not complete all iterations" + "Child should have been killed with the group, not completed; stdout: {:?}", + r.stdout + ); + assert!( + !r.stdout.contains("Log from runner 19"), + "Parent should have been killed before completion; stdout: {:?}", + r.stdout ); session.cleanup(); } @@ -366,6 +403,58 @@ fn spawn_cancel_with_retry( }) } +/// A notify-then-terminate cancel must actually deliver SIGTERM to the +/// cross-user workload's process group — not go straight to SIGKILL. The trap +/// script (`long_running.sh`, `trap 'echo Trapped; exit 1' TERM`) prints +/// "Trapped" only if SIGTERM reaches it; cancelling with a grace period gives +/// the trap time to fire and flush before escalation. This is the only test +/// that pins the SIGTERM path across the sudo boundary (the timeout tests use +/// CancelMethod::Terminate, i.e. immediate SIGKILL, so the trap never fires +/// there). +#[tokio::test(flavor = "multi_thread")] +#[ignore] +async fn test_cross_user_notify_delivers_sigterm() { + let user = require_target_user(); + let mut session = make_session(user); + // Grace period: SIGTERM, wait up to 3s for the trap to run + flush, then SIGKILL. + let canceller = { + let handle = session.cancel_handle(); + tokio::spawn(async move { + for _ in 0..60 { + tokio::time::sleep(Duration::from_millis(500)).await; + if handle.cancel(Some(Duration::from_secs(3)), false) { + return true; + } + } + false + }) + }; + + let script = support_dir().join("long_running.sh"); + let r = session + .run_subprocess(&script.to_string_lossy(), None, None, None, true, None) + .await + .unwrap(); + let delivered = canceller.await.expect("canceller task must not panic"); + + assert!( + delivered, + "handle must find the in-flight action and cancel it" + ); + assert!( + r.stdout.contains("Log from test 0"), + "workload should have run as the target user; stdout: {:?}", + r.stdout + ); + assert!( + r.stdout.contains("Trapped"), + "SIGTERM should have reached the workload's process group and fired its \ + trap before SIGKILL escalation; stdout: {:?}", + r.stdout + ); + session.cleanup(); +} + /// A `SessionCancelHandle` must be able to cancel a subprocess that runs via /// the cross-user helper: the helper path registers per-action cancel state, /// and the handle delivers the cancel command over the helper pipe. This is @@ -503,24 +592,38 @@ async fn test_cross_user_tempdir_cleanup() { #[ignore] async fn test_cross_user_tempdir_disjoint_fails() { let user = require_disjoint_user(); + // The process user is not a member of the disjoint user's group, so the + // chown to that group must fail. TempDir::new chowns before chmod and + // propagates the failure (SessionError::PathPermissions) rather than + // silently creating a directory with the wrong group — so this must be an + // Err. Snapshot the parent so we can also assert the just-created directory + // is removed on failure (the TempDir struct, and its Drop cleanup, never + // exist when chown fails, so TempDir::new must remove it explicitly). + let parent = openjd_sessions::tempdir::openjd_temp_dir(None).unwrap(); + let before: std::collections::HashSet = std::fs::read_dir(&parent) + .map(|rd| rd.filter_map(|e| e.ok().map(|e| e.path())).collect()) + .unwrap_or_default(); + let result = TempDir::new(None, None, Some(&*user)); - // Python raises RuntimeError. In Rust, chown failure is currently silent - // (let _ = nix::unistd::chown...), so the dir may be created but with wrong group. - if let Ok(td) = result { - use std::os::unix::fs::MetadataExt; - let meta = std::fs::metadata(td.path()).unwrap(); - let disjoint_gid = nix::unistd::Group::from_name(&user.group) - .unwrap() - .unwrap() - .gid - .as_raw(); - assert_ne!( - meta.gid(), - disjoint_gid, - "Should not be able to chown to disjoint group" - ); + match result { + Err(SessionError::PathPermissions { .. }) => {} + Err(other) => panic!("Expected PathPermissions error, got {other:?}"), + Ok(td) => panic!( + "Expected chown to the disjoint group to fail, but a directory was created at {}", + td.path().display() + ), } - // Err is also acceptable — means the crate properly rejects it + + // No orphaned directory: the set of entries under the parent is unchanged. + let after: std::collections::HashSet = std::fs::read_dir(&parent) + .map(|rd| rd.filter_map(|e| e.ok().map(|e| e.path())).collect()) + .unwrap_or_default(); + assert_eq!( + before, + after, + "failed cross-user TempDir::new must not leave a directory behind under {}", + parent.display() + ); } // === Cross-user embedded files permission tests === diff --git a/specs/sessions/cross-user-testing.md b/specs/sessions/cross-user-testing.md index 26435ae3..eebed733 100644 --- a/specs/sessions/cross-user-testing.md +++ b/specs/sessions/cross-user-testing.md @@ -10,7 +10,9 @@ LDAP-based user management. The infrastructure uses Docker containers to create isolated environments with the required user/group/sudo configuration. Tests are gated by `#[ignore]` and only run -inside these containers via `--include-ignored`. +inside these containers via `--include-ignored`. On macOS (which has no Docker in CI), +the same tests run directly on the runner with users provisioned through Directory +Services — see [macOS](#macos) below. This design was ported from the Python `openjd-sessions-for-python` library's Docker test infrastructure. @@ -143,6 +145,51 @@ process user. | `test_cross_user_tempdir_cleanup` | Cleanup works when target user has created files inside | | `test_cross_user_tempdir_disjoint_fails` | TempDir with disjoint user (no shared group) fails or has wrong group | +## macOS + +The full POSIX cross-user suite passes on macOS with no code changes — the crate's +mechanism (`process_group(0)` at spawn, `killpg` for signalling, no `setsid(1)` binary, +no `pgrep`, no procfs) is pure POSIX syscalls, all present on macOS. The +`cross-user-macos` CI job runs the suite on `macos-latest`, where the runner has +passwordless sudo, so users are provisioned directly on the host instead of in Docker. + +macOS-specific provisioning differences (encoded in the CI job): + +| Concern | Linux (Docker) | macOS | +|---|---|---| +| Create users/groups | `useradd`/`groupadd` | `sysadminctl -addUser` / `dseditgroup -o create` | +| Self-named user group | Created automatically by `useradd` | Must be created explicitly (`sysadminctl` assigns primary group `staff`); the tempdir cleanup test chowns to `user:user` | +| Temp root | `/tmp` (world-writable) | The default per-user `TMPDIR` (`/var/folders//T`) is mode `0700`, so the target user cannot traverse to the session working directory or execute the extracted helper binary. Tests set a world-traversable `TMPDIR`. | +| Resolve new users | immediate | `dscacheutil -flushcache` after provisioning | + +> **Deployment note:** the `TMPDIR` constraint applies to real macOS hosts too — the +> session root must be traversable by the session user (e.g. the Deadline Cloud worker +> agent uses `/var/lib/deadline/sessions` on macOS). A session root under a `0700` +> per-user directory fails with "Helper process closed stdout unexpectedly" because +> `sudo -u -i ` cannot reach the helper binary. Whether this bites +> depends on launch context: launchd sets the private per-user `TMPDIR` for per-user +> domain services and login sessions, whereas system `LaunchDaemon`s typically get +> `/tmp`. So the same binary can work as a daemon and fail when run from a terminal — +> which is why the caller should set an explicit session root rather than relying on +> `std::env::temp_dir()`. `TempDir` does not currently pre-check ancestor traversability +> (see the `#[ignore]`d `tempdir_under_untraversable_parent_is_currently_created` test, +> which pins this). + +> **Default-group sharp edge (macOS):** `PosixSessionUser::new(user, None)` defaults the +> group to the process's effective group. On macOS an ordinary local account's effective +> group is `staff`, so a cross-user `TempDir`/helper created with a `None` group is +> chmod'd `0o770`/`0o750` with group `staff` — readable/writable by *all* local users. +> This is inherited parity with the Python reference implementation +> (`grp.getgrgid(os.getegid())`), not a Rust-specific defect, but callers on macOS should +> pass an explicit dedicated group. + +There is no macOS equivalent of the LDAP variant. The macOS job provisions **local** +Directory Services accounts, which is the same local-node resolution path any ordinary +macOS user takes — it does not exercise a networked directory the way the Linux LDAP +container does. `nix::unistd::User::from_name()` does go through Directory Services +rather than reading `/etc/passwd` directly, but that is not equivalent to +networked-directory (LDAP/Active Directory) coverage. + ## Entry Script `scripts/run_cross_user_tests.sh` orchestrates the full test run: From fd69061ed13ccb65efd781cfe37ba737c2c6da17 Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Fri, 7 Aug 2026 08:27:35 -0700 Subject: [PATCH 2/3] docs(test): correct the timeout-path docstrings on cross-user tests Both docstrings described a SIGTERM->SIGKILL escalation that the timeout path does not perform. `run_subprocess`'s timeout uses `CancelMethod::Terminate`, whose `send_terminate` calls `terminate_process_group` -> SIGKILL directly (subprocess.rs:728 logs "Action timed out, sending SIGKILL to process group"). The workload's SIGTERM trap never fires on this path. - test_cross_user_subprocess_notify: said "the trap handler should fire", which contradicted the NOTE added directly below it and the removed "Trapped" assertion. - test_cross_user_subprocess_terminate: said the runner "must escalate to SIGKILL". It does not escalate; it sends SIGKILL outright. Since that makes the outcome identical to long_running.sh, the docstring now states what this test is actually worth: proving a workload that ignores SIGTERM is still reaped cross-user, which is the property that regresses if the timeout path ever switches to a trappable signal without a follow-up kill. Comments only; no test logic changed. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- .../tests/integration/test_cross_user.rs | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/crates/openjd-sessions/tests/integration/test_cross_user.rs b/crates/openjd-sessions/tests/integration/test_cross_user.rs index 9f200591..36ba05c1 100644 --- a/crates/openjd-sessions/tests/integration/test_cross_user.rs +++ b/crates/openjd-sessions/tests/integration/test_cross_user.rs @@ -98,8 +98,10 @@ async fn test_cross_user_subprocess_basic() { session.cleanup(); } -/// Run long_running.sh (traps SIGTERM) with a timeout — process should be killed -/// before completing all iterations, and the trap handler should fire. +/// Run long_running.sh (traps SIGTERM) with a timeout — the process should be +/// killed before completing all iterations. The trap handler does NOT fire: the +/// timeout path kills the process group with SIGKILL directly (see the NOTE +/// below), so this test asserts a mid-flight kill, not graceful trap handling. #[tokio::test(flavor = "multi_thread")] #[ignore] async fn test_cross_user_subprocess_notify() { @@ -141,8 +143,18 @@ async fn test_cross_user_subprocess_notify() { session.cleanup(); } -/// Run long_running_ignore.sh with timeout — the workload traps SIGTERM but does -/// not exit, so the runner must escalate to SIGKILL (which cannot be trapped). +/// Run long_running_ignore.sh with a timeout — the workload traps SIGTERM and +/// never exits on it, so only an untrappable signal can stop it. +/// +/// NOTE: there is no SIGTERM->SIGKILL escalation on this path. `run_subprocess`'s +/// timeout uses `CancelMethod::Terminate`, which sends SIGKILL to the process +/// group directly (subprocess.rs: "Action timed out, sending SIGKILL to process +/// group"), so the workload's TERM trap never fires. That makes the outcome here +/// identical to long_running.sh; this test's value is proving that a workload +/// which *ignores* SIGTERM is still reaped cross-user, which is the property that +/// would regress if the timeout path ever switched to a trappable signal without +/// a follow-up kill. Genuine SIGTERM delivery is covered by +/// test_cross_user_notify_delivers_sigterm. #[tokio::test(flavor = "multi_thread")] #[ignore] async fn test_cross_user_subprocess_terminate() { From d62ca9404be311b6b8017efc96c90e3bf7e000fc Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Fri, 7 Aug 2026 09:00:25 -0700 Subject: [PATCH 3/3] test(sessions): make the tree-kill assertions actually exercise the tree kill test_cross_user_subprocess_terminate_tree asserted the parent never reached "Log from runner 19", but spawn_child.sh ran `wait $CHILD` BEFORE its own loop. The child runs ~20s and the timeout is 3s, so the parent was still blocked in `wait` when the kill landed and its loop never started. The assertion therefore held whether or not the kill reached the parent -- including in the failure case where the parent is orphaned and still waiting. Reproduced both halves locally by mimicking run_subprocess (new process group, 3s timeout, SIGKILL to the group, 1s drain): * group kill, old script: stdout was only "Log from test 0..2" -- no "runner" output at all, confirming the loop never ran. * parent-only kill (orphan failure mode): "Log from runner 19" still absent, while "Log from test 19" appeared once the orphaned child ran to completion. So `test 19` was carrying the whole tree-kill property and `runner 19` was inert. Fix the fixture rather than delete the assertion: spawn_child.sh now runs its loop concurrently with the child and `wait`s afterward, so both processes are mid-loop when the kill lands. Verified the same way -- output now interleaves ("runner 0", "test 0", "runner 1", "test 1", ...), so "neither reached iteration 19" is real evidence both died. Added a "Log from runner 0" assertion so the concurrency the other assertions depend on is itself checked, and rewrote the comment to state that the child assertions are the ones carrying the orphan-detection property. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- .../tests/integration/test_cross_user.rs | 24 +++++++++++++------ .../tests/support/spawn_child.sh | 13 +++++++++- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/crates/openjd-sessions/tests/integration/test_cross_user.rs b/crates/openjd-sessions/tests/integration/test_cross_user.rs index 36ba05c1..eca8cc37 100644 --- a/crates/openjd-sessions/tests/integration/test_cross_user.rs +++ b/crates/openjd-sessions/tests/integration/test_cross_user.rs @@ -217,12 +217,17 @@ async fn test_cross_user_subprocess_terminate_tree() { "Expected Timeout or Failed, got {:?}", r.state ); - // spawn_child.sh launches long_running.sh as a child ("Log from test N") - // then runs its own loop ("Log from runner N"). Seeing the child's early - // output confirms the whole tree launched; neither loop completing confirms - // the process-GROUP kill reached both parent and child, rather than only the - // parent dying and the child being orphaned (which is the failure mode a - // pgid-based kill exists to prevent). + // spawn_child.sh launches long_running.sh ("Log from test N") and runs its + // own loop ("Log from runner N") CONCURRENTLY -- it does not `wait` first. + // That concurrency is what makes these assertions meaningful: both processes + // are mid-loop when the kill lands, so each "did not reach iteration 19" + // check is real evidence that process died rather than a side effect of it + // never having started. + // + // The child assertions carry the actual tree-kill property: if a kill reaped + // only the parent and orphaned the child, the child would keep printing and + // reach "Log from test 19". That is the failure mode a pgid-based kill exists + // to prevent, and it is directly observable here. assert!( r.stdout.contains("Log from test 0"), "Child process should have produced early output; stdout: {:?}", @@ -230,7 +235,12 @@ async fn test_cross_user_subprocess_terminate_tree() { ); assert!( !r.stdout.contains("Log from test 19"), - "Child should have been killed with the group, not completed; stdout: {:?}", + "Child should have been killed with the group, not orphaned to completion; stdout: {:?}", + r.stdout + ); + assert!( + r.stdout.contains("Log from runner 0"), + "Parent should have produced early output concurrently with the child; stdout: {:?}", r.stdout ); assert!( diff --git a/crates/openjd-sessions/tests/support/spawn_child.sh b/crates/openjd-sessions/tests/support/spawn_child.sh index c724e0e3..31ea8c98 100755 --- a/crates/openjd-sessions/tests/support/spawn_child.sh +++ b/crates/openjd-sessions/tests/support/spawn_child.sh @@ -2,11 +2,22 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # Copyright by contributors to this project. # SPDX-License-Identifier: (Apache-2.0 OR MIT) + +# Spawns a child, then emits its OWN output concurrently with the child's. +# +# The parent must NOT block in `wait` before its loop. A group-kill test needs +# both processes to be actively producing output when the kill lands, so that +# "neither loop finished" is evidence the kill reached BOTH. If the parent sat in +# `wait` until the child exited, its loop would never start within the test's +# timeout and any assertion about the parent's output would hold whether or not +# the kill reached it -- passing even if the parent were orphaned and still +# waiting. The trailing `wait` keeps the parent alive until the child is done in +# the no-kill case. DIR=$(dirname "$0") "$DIR/long_running.sh" & CHILD=$! -wait $CHILD for i in $(seq 0 19); do echo "Log from runner $i" sleep 1 done +wait $CHILD