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..eca8cc37 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()?; @@ -97,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() { @@ -121,18 +124,37 @@ 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 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() { @@ -155,6 +177,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 +217,36 @@ async fn test_cross_user_subprocess_terminate_tree() { "Expected Timeout or Failed, got {:?}", r.state ); + // 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"), - "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 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!( + !r.stdout.contains("Log from runner 19"), + "Parent should have been killed before completion; stdout: {:?}", + r.stdout ); session.cleanup(); } @@ -366,6 +425,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 +614,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/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 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: