diff --git a/.github/workflows/macos_cross_user_test.yml b/.github/workflows/macos_cross_user_test.yml new file mode 100644 index 00000000..80a70720 --- /dev/null +++ b/.github/workflows/macos_cross_user_test.yml @@ -0,0 +1,57 @@ +name: macOS Cross-User Tests + +# Runs the POSIX user-impersonation tests (which xfail when the OPENJD_TEST_SUDO_* +# environment variables are unset) on a macOS runner. This exercises the real +# `sudo -u -i -I -c ` cross-user path end to end: +# process launch as another user, new-process-group creation, signalling, and +# process-tree termination. +# +# The provisioning, the test run and the teardown all live in +# scripts/run_macos_sudo_tests.sh, so a developer can reproduce this job on their +# own Mac with one command (`hatch run cross-user-test-macos`). This job is +# deliberately a thin wrapper around that script: anything it did that the script +# does not would be something a developer cannot reproduce. +# +# This job covers ONLY the cross-user tests. The rest of the suite already runs on +# macos-latest across the same Python matrix in code_quality.yml, so re-running it +# here would duplicate that coverage. +# +# Runs on every PR rather than behind a paths filter: the cross-user path can be +# broken from more places than a file list can enumerate (session setup, tempdir +# handling, signalling), and a filtered job that misses those reads as a pass. + +on: + workflow_dispatch: + pull_request: + branches: [ mainline, release ] + +jobs: + macos-cross-user: + name: Python ${{ matrix.python-version }} + runs-on: macos-latest + timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + # Matches code_quality.yml: requires-python is >=3.9, and the boundary + # versions are where an interpreter-specific difference in the setsid shim + # or in sys._base_executable resolution would surface. + python-version: ['3.9', '3.10', '3.11', '3.12', '3.13', '3.14'] + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: ${{ matrix.python-version }} + + - name: Install hatch + # virtualenv 21 removed virtualenv.discovery.builtin.propose_interpreters, which + # the hatch version resolvable on 3.9 still calls, so `hatch env create` fails with + # "Environment `default` is incompatible". Pin it for 3.9 only and leave 3.10+ on + # current virtualenv. Same constraint the other workflows in this repo use. + run: pip install --upgrade hatch 'virtualenv<21; python_version < "3.10"' + + - name: Provision, run cross-user tests, and tear down + # --keep skips the teardown: the runner is throwaway, so leaving the + # environment in place costs nothing and keeps a failed run inspectable. + run: bash scripts/run_macos_sudo_tests.sh --keep diff --git a/README.md b/README.md index ec53f71d..03a7a83d 100644 --- a/README.md +++ b/README.md @@ -234,6 +234,15 @@ with passwordless `sudo` by, for example, adding a rule like follows to your host ALL=(actions) NOPASSWD: ALL ``` +On MacOS, the impersonated command is launched under a small Python shim because macOS +lacks the `setsid(1)` utility. The shim runs with the base interpreter behind the Python +that is running this library (for a virtual environment, the interpreter the venv was +created from) provided that interpreter is reachable and executable by other users; +otherwise it falls back to the operating system's `/usr/bin/python3`, which resolves to a +working interpreter only when the Xcode Command Line Tools (or Xcode) are present +(`xcode-select --install`). No separate Python installation is required when the base +interpreter is usable. + #### Impersonating a User: Windows Systems To run an impersonated Session on Windows Systems modify the "Running a Session" example diff --git a/hatch.toml b/hatch.toml index 7dc684bb..2e86bf0e 100644 --- a/hatch.toml +++ b/hatch.toml @@ -7,6 +7,12 @@ pre-install-commands = [ sync = "pip install -r requirements-testing.txt" test = "pytest --cov-config pyproject.toml {args}" typing = "mypy {args:src test}" +# Cross-user (jobRunAsUser impersonation) tests. These need a provisioned +# user/group/sudoers environment, so they go through the platform's setup script +# rather than pytest alone: Linux uses a throwaway container, macOS provisions the +# host and cleans up after itself. +cross-user-test = "bash scripts/run_sudo_tests.sh {args}" +cross-user-test-macos = "bash scripts/run_macos_sudo_tests.sh {args}" style = [ "ruff check {args:.}", "black --check --diff {args:.}", diff --git a/scripts/run_macos_sudo_tests.sh b/scripts/run_macos_sudo_tests.sh new file mode 100755 index 00000000..7805b9d0 --- /dev/null +++ b/scripts/run_macos_sudo_tests.sh @@ -0,0 +1,318 @@ +#!/bin/bash +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +# Runs the POSIX user-impersonation tests on macOS. +# +# The Linux equivalent (scripts/run_sudo_tests.sh) gets its users, groups and +# sudoers rule from a throwaway Docker container. macOS cannot be containerized, +# so the same layout has to be created on the host itself. That makes this script +# the counterpart to that Dockerfile rather than to the docker command: it +# provisions, runs, and then removes what it created. +# +# Provisioned layout (mirrors testing_containers/localuser_sudo_environment/Dockerfile): +# -- runs the pytests; joined to the shared group +# openjd-target -- the impersonated user; also in the shared group +# openjd-disjoint -- shares no group with you (temp-dir permission tests) +# +# USAGE +# scripts/run_macos_sudo_tests.sh # provision, test, clean up +# scripts/run_macos_sudo_tests.sh --keep # leave the environment in place +# scripts/run_macos_sudo_tests.sh --cleanup-only +# scripts/run_macos_sudo_tests.sh -- -k test_basic_operation # extra pytest args +# +# Requires sudo. On your own machine prefer the default (cleaning up) run: this +# creates real local accounts, a real /etc/sudoers.d file and a symlink in +# /usr/local/bin, none of which you want left behind. +# +# NOT undone by the teardown: the impersonated user needs to read the test support +# files, so this adds o+r to test/openjd/sessions_v0/support_files and o+x (traverse +# only) to the directories leading there, plus o+rX on HATCH_DATA_DIR. Those bits stay +# after the run. The rest of the working tree is left alone, so untracked or +# credential-bearing files elsewhere under the repo are unaffected. + +set -euo pipefail + +if ! test -d scripts; then + echo "Must run from the root of the repository" + exit 1 +fi + +if [[ "$(uname -s)" != "Darwin" ]]; then + echo "This script is for macOS. On Linux use scripts/run_sudo_tests.sh." + exit 1 +fi + +export OPENJD_TEST_SUDO_TARGET_USER="${OPENJD_TEST_SUDO_TARGET_USER:-openjd-target}" +export OPENJD_TEST_SUDO_SHARED_GROUP="${OPENJD_TEST_SUDO_SHARED_GROUP:-openjd-shared}" +export OPENJD_TEST_SUDO_DISJOINT_USER="${OPENJD_TEST_SUDO_DISJOINT_USER:-openjd-disjoint}" +export OPENJD_TEST_SUDO_DISJOINT_GROUP="${OPENJD_TEST_SUDO_DISJOINT_GROUP:-openjd-disjointgrp}" + +# Hatch's default data dir lives under ~/Library, which other users cannot +# traverse. The impersonation tests execute the hatch venv's python AS the target +# user, so the venv has to sit somewhere world-traversable. +export HATCH_DATA_DIR="${HATCH_DATA_DIR:-/opt/hatch}" + +# macOS's per-user temp dir (/var/folders//T, mode 700) is not traversable +# by the impersonated user, and /var is a symlink to /private/var (which TempDir +# resolves but gettempdir() does not). Use a dedicated, already-resolved, +# world-writable temp root so the tests get the /tmp semantics they have on Linux. +# +# Deliberately NOT configurable. cleanup() does `rm -rf` on this path, so a +# caller-supplied value turns a typo (or TMPDIR=/tmp) into a destructive run. The +# tests only need *a* world-traversable, already-resolved directory, so there is +# nothing to gain by making the choice adjustable. +export TMPDIR=/private/tmp/openjd-tests + +# SUDO_USER is inherited from the environment, and TEST_USER is interpolated into a +# /etc/sudoers.d file below. Validate it before it is used anywhere: a value containing a +# newline can append arbitrary rules, and `visudo -cf` does not save us because the file is +# written before it runs (so a rejected file still lands on disk) and because a payload +# ending in '#' comments out the remainder and validates cleanly. Also guards the +# dseditgroup and chown calls that take this value. +TEST_USER="${SUDO_USER:-$(id -un)}" +if [[ ! "${TEST_USER}" =~ ^[A-Za-z0-9._-]+$ ]]; then + echo "ERROR: refusing to run with an unusual user name: ${TEST_USER}" + echo " (TEST_USER comes from SUDO_USER, or from 'id -un' when that is unset)" + exit 1 +fi +if ! id -u "${TEST_USER}" > /dev/null 2>&1; then + echo "ERROR: '${TEST_USER}' is not a local user on this host." + exit 1 +fi +SUDOERS_FILE=/etc/sudoers.d/openjd-cross-user-tests +PYTHON_SHIM=/usr/local/bin/python +# Whether *this* script created PYTHON_SHIM. On a developer machine that path is +# often a real symlink managed by pyenv, Homebrew or a python.org installer, so +# cleanup must only remove it if we were the one who put it there. +PYTHON_SHIM_CREATED="False" +# Likewise for the temp root: cleanup() does `rm -rf` on it, so only remove it if we +# were the one who made it. +TMPDIR_CREATED="False" + +KEEP="False" +CLEANUP_ONLY="False" +PYTEST_ARGS=() +while [[ "${1:-}" != "" ]]; do + case $1 in + -h|--help) + sed -n '4,31p' "$0" | sed 's/^# \{0,1\}//' + exit 1 + ;; + --keep) KEEP="True" ;; + --cleanup-only) CLEANUP_ONLY="True" ;; + --) shift; PYTEST_ARGS=("$@"); break ;; + *) + echo "Unrecognized parameter: $1" + exit 1 + ;; + esac + shift +done + +cleanup() { + echo "--- Removing the cross-user test environment ---" + # Best-effort throughout: a partially-provisioned environment must still be + # removable, so nothing here may abort the teardown. + sudo rm -f "${SUDOERS_FILE}" || true + # Only remove the python alias if provision() created it; see PYTHON_SHIM_CREATED. + if [[ "${PYTHON_SHIM_CREATED}" == "True" ]]; then + sudo rm -f "${PYTHON_SHIM}" || true + fi + for u in "${OPENJD_TEST_SUDO_TARGET_USER}" "${OPENJD_TEST_SUDO_DISJOINT_USER}"; do + sudo sysadminctl -deleteUser "${u}" > /dev/null 2>&1 || true + # -deleteUser leaves the self-named group behind when we created it ourselves. + sudo dseditgroup -o delete "${u}" > /dev/null 2>&1 || true + done + for g in "${OPENJD_TEST_SUDO_SHARED_GROUP}" "${OPENJD_TEST_SUDO_DISJOINT_GROUP}"; do + sudo dseditgroup -o delete "${g}" > /dev/null 2>&1 || true + done + # Only remove the temp root if provision() created it: the path is fixed, but a + # developer may already have one there from an earlier interrupted run or their own use. + if [[ "${TMPDIR_CREATED}" == "True" ]]; then + sudo rm -rf "${TMPDIR}" || true + fi + sudo dscacheutil -flushcache || true + echo "--- Done ---" +} + +if [[ "${CLEANUP_ONLY}" == "True" ]]; then + # --cleanup-only recovers from an interrupted run, where provision() never set the + # *_CREATED flags in this process. Each one has to be re-established from what is on + # disk, and only when it is safe to claim. + # + # The alias: only if it points at the target we would have used. A pyenv/Homebrew + # symlink points somewhere else and is left alone. + if [[ -L "${PYTHON_SHIM}" && "$(readlink "${PYTHON_SHIM}")" == "/usr/bin/python3" ]]; then + PYTHON_SHIM_CREATED="True" + fi + # The temp root: claim a real directory, refuse a symlink. provision() only ever + # creates this fresh (plain mkdir, no -p), so a directory here is one of ours from an + # interrupted run. A symlink is not something this script can have produced, and + # claiming one would let `rm -rf` be redirected at its target. + if [[ -d "${TMPDIR}" && ! -L "${TMPDIR}" ]]; then + TMPDIR_CREATED="True" + elif [[ -L "${TMPDIR}" ]]; then + echo "WARNING: ${TMPDIR} is a symlink, which this script never creates." + echo " Leaving it alone -- remove it by hand after checking where it points." + fi + cleanup + exit 0 +fi + +provision() { + echo "--- Provisioning users and groups (requires sudo) ---" + 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 the test user. + 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 test_cleanup_posix_user + # chowns to "user:user"; macOS does not, so create it explicitly. The test user + # must NOT be a member of 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: no group in common with the test user. + 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}" + + # The test-running user joins the shared group (matches the Docker layout). + sudo dseditgroup -o edit -a "${TEST_USER}" -t user "${OPENJD_TEST_SUDO_SHARED_GROUP}" + + # Passwordless sudo to the target user (and to itself), mirroring the hostuser + # rule in the Linux test container. Validated before it is trusted: a malformed + # file in /etc/sudoers.d breaks sudo host-wide. + echo "${TEST_USER} ALL=(${OPENJD_TEST_SUDO_TARGET_USER},${TEST_USER}) NOPASSWD: ALL" \ + | sudo tee "${SUDOERS_FILE}" > /dev/null + sudo chmod 440 "${SUDOERS_FILE}" + sudo visudo -cf "${SUDOERS_FILE}" + + # test_basic_operation runs a bare `python` as the target user via `sudo -i`; + # macOS ships python3 only, so provide the alias -- but only if nothing is there + # already. pyenv, Homebrew and the python.org installers all manage this path, + # and clobbering (or later deleting) a developer's `python` is not ours to do. + sudo mkdir -p "$(dirname "${PYTHON_SHIM}")" + if [[ -e "${PYTHON_SHIM}" || -L "${PYTHON_SHIM}" ]]; then + echo "${PYTHON_SHIM} already exists; leaving it in place" + else + sudo ln -s /usr/bin/python3 "${PYTHON_SHIM}" + PYTHON_SHIM_CREATED="True" + fi + + sudo dscacheutil -flushcache + + # TMPDIR is a fixed, predictable path under world-writable, sticky /private/tmp, so + # any local user can pre-create it before this runs -- including as a symlink. + # `mkdir -p` succeeds silently on an existing symlink-to-directory, and chown/chmod + # follow symlinks, so reusing whatever is there would let `ln -s /etc "${TMPDIR}"` + # turn the next run into `chown`+`chmod 1777` on /etc. Create it with plain `mkdir` + # (no -p) so an existing path is a hard error, and only touch ownership/mode on the + # directory we just made. + if ! sudo mkdir "${TMPDIR}" 2> /dev/null; then + echo "ERROR: ${TMPDIR} already exists." + echo " It is created fresh on each run and removed afterwards, so something" + echo " else put it there: an interrupted earlier run, or another user." + if [[ -L "${TMPDIR}" ]]; then + echo " It is a SYMLINK, which this script never creates. Check where it" + echo " points before removing it -- --cleanup-only will not touch it." + else + echo " Inspect it, then re-run with --cleanup-only to remove it." + fi + exit 1 + fi + TMPDIR_CREATED="True" + # Owned by the test user, group staff: BSD filesystems give a new file the + # group of its PARENT directory rather than the creator's gid, and the + # same-user TempDir test asserts the created directory has the creating + # process's gid. + sudo chown "${TEST_USER}:staff" "${TMPDIR}" + sudo chmod 1777 "${TMPDIR}" +} + +verify() { + echo "--- Verifying the isolation invariants the tests rely on ---" + id "${OPENJD_TEST_SUDO_TARGET_USER}" + id "${OPENJD_TEST_SUDO_DISJOINT_USER}" + id "${TEST_USER}" + id -Gn "${TEST_USER}" | tr ' ' '\n' | grep -qx "${OPENJD_TEST_SUDO_SHARED_GROUP}" + id -Gn "${OPENJD_TEST_SUDO_TARGET_USER}" | 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 + # Cross-user execution works at all + sudo -u "${OPENJD_TEST_SUDO_TARGET_USER}" -i /usr/bin/true + sudo -u "${OPENJD_TEST_SUDO_TARGET_USER}" -i python -c \ + 'import getpass; print("bare python runs as", getpass.getuser())' +} + +if [[ "${KEEP}" != "True" ]]; then + trap cleanup EXIT +fi + +provision +verify + +echo "--- Creating the test environment ---" +sudo mkdir -p "${HATCH_DATA_DIR}" +sudo chown "${TEST_USER}" "${HATCH_DATA_DIR}" +# On Python 3.9, virtualenv 21 removed virtualenv.discovery.builtin.propose_interpreters, +# which the hatch version resolvable there still calls; `hatch env create` then fails with +# "Environment `default` is incompatible". Say so rather than letting that message stand on +# its own, since it names neither virtualenv nor the fix. +if ! hatch env create; then + echo "" + echo "ERROR: 'hatch env create' failed." + if python3 -c 'import sys; sys.exit(0 if sys.version_info < (3, 10) else 1)'; then + echo "On Python 3.9 this is usually virtualenv 21, which dropped an API hatch still" + echo "uses. Try: pip install --upgrade hatch 'virtualenv<21'" + fi + exit 1 +fi +# The target user executes the venv python and reads the test support files, so both +# must be world-readable/traversable. +# +# Scoped to exactly those two, NOT the whole checkout. `chmod -R o+rX .` would make +# every file in the working tree world-readable, including untracked files, .env-style +# files and anything else a developer happens to keep under the repo root, and nothing +# here puts those bits back. +SUPPORT_FILES="test/openjd/sessions_v0/support_files" +chmod -R o+rX "${HATCH_DATA_DIR}" +chmod -R o+rX "${SUPPORT_FILES}" +# Traversal (o+x) only, no read, on the directories leading to the support files. +chmod o+x . test test/openjd test/openjd/sessions_v0 + +echo "--- Which interpreter the setsid shim resolves to ---" +# NOTE: no braces in this inline script -- hatch run applies its own {...} +# template substitution to the arguments it receives. +hatch run python -c " +from openjd.sessions._subprocess import _macos_shim_interpreter, _MACOS_FALLBACK_SHIM_INTERPRETER +picked = _macos_shim_interpreter() +branch = 'FALLBACK' if picked == _MACOS_FALLBACK_SHIM_INTERPRETER else 'BASE-INTERPRETER' +print('shim interpreter:', picked, '(' + branch + ' branch)') +" + +echo "--- Running the cross-user impersonation tests ---" +# -rxX lists (x)failed and (X)passed-unexpectedly tests so the check below can +# tell a real run from one that silently xfailed into a no-op. +LOG_FILE="$(mktemp)" +hatch run test -- \ + test/openjd/sessions_v0/test_subprocess.py \ + test/openjd/sessions_v0/test_tempdir.py \ + --no-cov -rxX "${PYTEST_ARGS[@]+"${PYTEST_ARGS[@]}"}" 2>&1 | tee "${LOG_FILE}" + +# The impersonation tests xfail (rather than fail) when the OPENJD_TEST_SUDO_* +# variables are missing, so a broken environment would otherwise look like a pass. +if grep -q "Must define environment vars OPENJD_TEST_SUDO" "${LOG_FILE}"; then + echo "ERROR: impersonation tests were skipped -- the environment is not being picked up." + rm -f "${LOG_FILE}" + exit 1 +fi +rm -f "${LOG_FILE}" + +echo "--- Cross-user tests passed ---" diff --git a/src/openjd/sessions/_os_checker.py b/src/openjd/sessions/_os_checker.py index c42c2dda..d87dd403 100644 --- a/src/openjd/sessions/_os_checker.py +++ b/src/openjd/sessions/_os_checker.py @@ -21,6 +21,10 @@ def is_windows() -> bool: return os.name == WINDOWS +def is_macos() -> bool: + return sys.platform == MACOS + + def check_os() -> None: if not (is_posix() or is_windows()): raise NotImplementedError( diff --git a/src/openjd/sessions/_subprocess.py b/src/openjd/sessions/_subprocess.py index 8ee6c6be..9d0929e6 100644 --- a/src/openjd/sessions/_subprocess.py +++ b/src/openjd/sessions/_subprocess.py @@ -3,6 +3,7 @@ import os import shlex import signal +import stat import sys import time from contextlib import nullcontext @@ -16,7 +17,7 @@ from ._linux._capabilities import try_use_cap_kill from ._linux._sudo import find_sudo_child_process_group_id from ._logging import LoggerAdapter, LogContent, LogExtraInfo -from ._os_checker import is_linux, is_posix, is_windows +from ._os_checker import is_linux, is_macos, is_posix, is_windows from ._session_user import PosixSessionUser, WindowsSessionUser, SessionUser from ._action_filter import redact_openjd_redacted_env_requests @@ -28,6 +29,139 @@ __all__ = ("LoggingSubprocess",) +# macOS has no `setsid(1)` binary (it is a Linux/util-linux tool), yet the new-session +# behavior it provides is still required: `sudo -u -i ` places the workload in +# sudo's own (root-owned) process group, which the jobRunAsUser cannot signal and which +# openjd must not signal (it would hit the root sudo process). We reproduce `setsid` with a +# tiny pure-Python shim, run as the workload, that makes the workload a new session/process- +# group leader and then exec's the real command. +# +# Details: +# * `os.getpgrp() == os.getpid() or os.setsid()` calls setsid() only when the process is +# NOT already a group leader; os.setsid() raises EPERM if the caller already leads a +# group, so the short-circuit avoids that. Either way the workload ends up in a process +# group distinct from sudo's, which find_sudo_child_process_group_id() then discovers. +# * Single line (no newlines) so it passes cleanly through `sudo -i` argv without any +# shell-quoting fragility. +# * The interpreter that runs the shim is the base interpreter behind the one running this +# process (see _macos_shim_interpreter()), falling back to /usr/bin/python3; if neither is +# reachable by the target user, NoReachableInterpreterError is raised rather than deferring +# an opaque errno 2 to Popen. sys.executable itself is not used directly because it may +# live inside a virtual environment that the jobRunAsUser has no traverse/read permission +# on. +# * `-I` (isolated mode) drops the current working directory from sys.path and ignores +# PYTHON* environment variables, so a file such as os.py in the session working directory +# cannot be imported ahead of the standard library before os.execvp() runs. +# * SIGPIPE and SIGXFSZ are restored to SIG_DFL before the exec. CPython ignores both +# during interpreter startup, and SIG_IGN (unlike an installed handler) SURVIVES exec, so +# without this every impersonated workload on macOS would run with SIGPIPE ignored -- +# `producer | head` would get EPIPE write errors instead of dying on the signal. Popen's +# restore_signals does not help: the process it starts is this shim, which re-ignores +# them, and os.execvp has no equivalent. Linux's setsid(1) is a small C binary that never +# touches these dispositions, so restoring them here is what keeps the two platforms +# behaviourally identical. (The blocked-signal mask is also inherited across exec, but +# CPython leaves it empty, so there is nothing to reset.) +# +# Signal-target discovery (find_sudo_child_process_group_id) locates the workload by walking +# sudo's single child and comparing process groups. This relies on `sudo -i` exec'ing the +# command into a single child rather than leaving extra long-lived processes in between; the +# same assumption already holds for the Linux `setsid -w` path. +# +# TIMING, which this path DOES change: the workload's process group is not distinct from +# sudo's until the shim interpreter has finished booting and reached setsid(). Linux flips the +# pgid inside a tiny C binary, so it is near-instant there; here it costs a full CPython +# startup as the job user. Measured on macOS 26.5 (arm64) with /usr/bin/python3: ~120-180ms +# idle and ~165-190ms with every core saturated, against the 1s default timeout in +# find_sudo_child_process_group_id. That is comfortable but not enormous, and if the margin +# ever proves too thin the fix is to raise that timeout for this path specifically rather +# than to change the shim: discovery failing means a later cancel has no signal target and +# silently does not kill the workload. +_MACOS_SETSID_SHIM = ( + "import os,signal,sys;os.getpgrp()==os.getpid() or os.setsid();" + "signal.signal(signal.SIGPIPE,signal.SIG_DFL);" + "signal.signal(signal.SIGXFSZ,signal.SIG_DFL);" + "os.execvp(sys.argv[1],sys.argv[1:])" +) +_MACOS_FALLBACK_SHIM_INTERPRETER = "/usr/bin/python3" + + +def _other_users_can_execute(path: str) -> bool: + """Returns whether an arbitrary other user can execute the file at the given path based + on the world (other) permission bits: the file itself must be o+x and every directory on + the path must be o+x (traversable). A world-executable file under e.g. a 0o750 home + directory is still unreachable, so both checks are required. + + This is a conservative approximation in one direction and an incomplete one in the other: + + * It ignores group permissions and ACLs that might also grant access, so it can return + False for a path some specific user could execute. + * It only inspects the interpreter FILE. It says nothing about whether the target user can + read that interpreter's standard library or, for a framework build, its Python dylib. An + interpreter with o+x on the binary but o-rx on .../lib passes this check and then fails + at runtime with "Fatal Python error: init_fs_encoding". Proving otherwise would mean + actually running the candidate as the target user (a `sudo -u -i -I -c + ""` probe) on every launch, which is not worth the cost; callers should treat a True + result as "the executable is reachable", not "the interpreter will boot". + """ + try: + mode = os.stat(path).st_mode + if not (stat.S_ISREG(mode) and mode & stat.S_IXOTH): + return False + parent = os.path.dirname(path) + while True: + if not os.stat(parent).st_mode & stat.S_IXOTH: + return False + next_parent = os.path.dirname(parent) + if next_parent == parent: # reached the filesystem root + return True + parent = next_parent + except OSError: + return False + + +class NoReachableInterpreterError(Exception): + """Raised on macOS when no Python interpreter can be found that the jobRunAsUser is able + to execute, so the setsid shim (and therefore cross-user execution) cannot be run.""" + + pass + + +def _macos_shim_interpreter() -> str: + """Returns the path of the Python interpreter used to run _MACOS_SETSID_SHIM as the + jobRunAsUser. + + Prefers the base interpreter behind the one running this process (sys._base_executable; + for a virtual environment this is the interpreter the venv was created from, in a system + location such as /usr/bin, /opt/homebrew, or a python.org framework install) so that no + separate Python installation is required on the host. The venv's own sys.executable is + not suitable: the jobRunAsUser typically has no traverse/read permission on the agent's + venv directory. + + Falls back to /usr/bin/python3 (the Command Line Tools shim; requires the Command Line + Tools or Xcode to be installed) when the base interpreter cannot be determined or is not + reachable and executable by other users. + + Raises: + NoReachableInterpreterError: when neither candidate is reachable and executable by + other users. The fallback is checked rather than returned on faith, because + returning an unusable path defers the failure to Popen, which reports only + "[Errno 2] No such file or directory: '/usr/bin/python3'" on a workload that may + have nothing to do with Python. + """ + base = os.path.realpath(getattr(sys, "_base_executable", None) or sys.executable) + if _other_users_can_execute(base): + return base + if _other_users_can_execute(_MACOS_FALLBACK_SHIM_INTERPRETER): + return _MACOS_FALLBACK_SHIM_INTERPRETER + raise NoReachableInterpreterError( + "No Python interpreter is executable by the target user, which macOS requires to run " + "an action as another user (it has no setsid(1), so the action is launched via a " + f"Python shim). Tried {base} and {_MACOS_FALLBACK_SHIM_INTERPRETER}. Install the Xcode " + "Command Line Tools with 'xcode-select --install', or make one of those interpreters " + "world-executable with world-traversable parent directories." + ) + + # ======================================================================== # ======================================================================== # DEVELOPER NOTE: @@ -449,7 +583,29 @@ def _start_subprocess(self) -> Optional[Popen]: # same process group as the `sudo` command. If that happens, then # we're stuck: 1/ Our user cannot kill processes by the self._user; and # 2/ The self._user cannot kill the root-owned sudo process group. - command.extend(["sudo", "-u", user.user, "-i", "setsid", "-w"]) + if is_macos(): + # macOS has no setsid(1); use a pure-Python setsid shim (see + # _MACOS_SETSID_SHIM) run as the workload to get the same + # new-session behavior that `setsid -w` provides on Linux. + shim_interpreter = _macos_shim_interpreter() + self._logger.info( + f"Using {shim_interpreter} to run the setsid shim", + extra=LogExtraInfo(openjd_log_content=LogContent.PROCESS_CONTROL), + ) + command.extend( + [ + "sudo", + "-u", + user.user, + "-i", + shim_interpreter, + "-I", + "-c", + _MACOS_SETSID_SHIM, + ] + ) + else: + command.extend(["sudo", "-u", user.user, "-i", "setsid", "-w"]) elif is_windows(): user = cast(WindowsSessionUser, self._user) # type: ignore diff --git a/test/openjd/sessions_v0/test_os_checker.py b/test/openjd/sessions_v0/test_os_checker.py index 28efcd66..b8389994 100644 --- a/test/openjd/sessions_v0/test_os_checker.py +++ b/test/openjd/sessions_v0/test_os_checker.py @@ -3,7 +3,7 @@ import unittest from enum import Enum from unittest.mock import patch -from openjd.sessions._os_checker import is_posix, is_windows, check_os +from openjd.sessions._os_checker import is_macos, is_posix, is_windows, check_os class OSName(str, Enum): @@ -32,6 +32,16 @@ def test_is_not_windows(self, mock_os): mock_os.name = OSName.POSIX self.assertFalse(is_windows()) + @patch("openjd.sessions._os_checker.sys") + def test_is_macos(self, mock_sys): + mock_sys.platform = "darwin" + self.assertTrue(is_macos()) + + @patch("openjd.sessions._os_checker.sys") + def test_is_not_macos(self, mock_sys): + mock_sys.platform = "linux" + self.assertFalse(is_macos()) + @patch("openjd.sessions._os_checker.os") def test_check_os_posix(self, mock_os): mock_os.name = OSName.POSIX diff --git a/test/openjd/sessions_v0/test_session.py b/test/openjd/sessions_v0/test_session.py index b09219d6..75caf1a0 100644 --- a/test/openjd/sessions_v0/test_session.py +++ b/test/openjd/sessions_v0/test_session.py @@ -3029,10 +3029,16 @@ def test_def_via_stdout_fails_session_action_on_error( # THEN assert session.state == SessionState.READY_ENDING - assert ( + # The error is logged by the subprocess stdout-filter thread, which can + # still be draining the pipe when the state transition is observed above. + # Wait for the message rather than racing that thread. + expected_message = ( "openjd_env: FOO -- ERROR: Failed to parse environment variable assignment." - in caplog.messages ) + deadline = time.monotonic() + 5 + while expected_message not in caplog.messages and time.monotonic() < deadline: + time.sleep(0.1) + assert expected_message in caplog.messages callback.assert_has_calls( [ diff --git a/test/openjd/sessions_v0/test_subprocess.py b/test/openjd/sessions_v0/test_subprocess.py index a5fdb349..11231ed2 100644 --- a/test/openjd/sessions_v0/test_subprocess.py +++ b/test/openjd/sessions_v0/test_subprocess.py @@ -16,7 +16,7 @@ import pytest import openjd -from openjd.sessions._os_checker import is_posix, is_windows +from openjd.sessions._os_checker import is_macos, is_posix, is_windows from openjd.sessions._session_user import PosixSessionUser, WindowsSessionUser from openjd.sessions._subprocess import LoggingSubprocess from openjd.sessions import _subprocess as subprocess_impl_mod @@ -1274,6 +1274,422 @@ def end_proc(): assert num_children_running == 0 +@pytest.mark.usefixtures("message_queue", "queue_handler") +class TestLoggingSubprocessMacOSSetsid: + """Tests for the macOS-specific cross-user command construction. + + macOS has no setsid(1), so on darwin the workload is launched under a small + pure-Python shim (run via a system-location Python interpreter with -I) that + becomes a new session/process-group leader before exec'ing the real command. + """ + + @pytest.mark.skipif( + is_windows(), reason="Constructs a PosixSessionUser, which is rejected on Windows hosts" + ) + def test_builds_setsid_shim_command_on_macos(self, queue_handler: QueueHandler) -> None: + # GIVEN + from openjd.sessions import _subprocess as subprocess_mod + + logger = build_logger(queue_handler) + target_user = MagicMock(spec=PosixSessionUser) + target_user.user = "job-user" + target_user.is_process_user.return_value = False + subproc = LoggingSubprocess( + logger=logger, + args=["/path/to/workload.sh"], + user=target_user, + ) + + # WHEN + with ( + patch.object(subprocess_mod, "is_macos", return_value=True), + patch.object(subprocess_mod, "is_posix", return_value=True), + patch.object(subprocess_mod, "is_windows", return_value=False), + patch.object( + subprocess_mod, "_macos_shim_interpreter", return_value="/usr/local/bin/python3" + ), + patch.object(subprocess_mod, "Popen") as mock_popen, + ): + subproc._start_subprocess() + + # THEN + built_command = mock_popen.call_args.kwargs["args"] + assert built_command == [ + "sudo", + "-u", + "job-user", + "-i", + "/usr/local/bin/python3", + "-I", + "-c", + subprocess_mod._MACOS_SETSID_SHIM, + "/path/to/workload.sh", + ] + + +@pytest.mark.skipif(not is_posix(), reason="process groups and setsid are posix-only") +class TestSetsidShimBehavior: + """Tests the behaviour of the setsid shim string itself, on any POSIX host. + + The shim is portable POSIX (os.getpgrp/os.getpid/os.setsid/os.execvp with no + platform branch); macOS is merely the platform where it is *required*, because + macOS ships no setsid(1) for the cross-user command to call. Running it + everywhere POSIX is deliberate: Linux exercises the same semantics on faster, + more reliable runners, so a broken shim string is caught there too rather than + only in the macOS job. + + Whether macOS actually *selects* this shim is a separate concern, covered by + TestLoggingSubprocessMacOSSetsid::test_builds_setsid_shim_command_on_macos. + """ + + def test_setsid_shim_creates_new_process_group(self) -> None: + # GIVEN the shim string that macOS uses in place of setsid(1). + from subprocess import PIPE, run + + from openjd.sessions import _subprocess as subprocess_mod + + # WHEN we run it (as the current user; no sudo) to report the workload's + # process-group id alongside the launching python's own pid. + result = run( + [ + sys.executable, + "-I", + "-c", + subprocess_mod._MACOS_SETSID_SHIM, + "/bin/sh", + "-c", + "echo $$ $(ps -o pgid= -p $$)", + ], + stdout=PIPE, + text=True, + check=True, + ) + + # THEN the workload is the leader of its own process group (pgid == its pid). + workload_pid, workload_pgid = (int(x) for x in result.stdout.split()) + assert workload_pid == workload_pgid + + @pytest.mark.skipif(not is_macos(), reason="the sudo -i shim path is macOS-only") + def test_shim_survives_sudo_login_shell_quoting(self) -> None: + """Run the shim the way production does: through `sudo -u -i`. + + `sudo -i` composes a login-shell command line, so the shim string passes through a + shell that would act on the ';', '(', ')', '[', ']' and '=' characters it contains if + any layer re-split it on whitespace. Every other test here bypasses that: they either + invoke sys.executable with an argv list, or assert only the list openjd builds. This + is the one check that the real quoting holds. + + Self-sudo (target == current user) so no second account is needed. Skipped unless the + impersonation environment is provisioned, which is what grants the NOPASSWD rule. + """ + if not has_posix_target_user(): + pytest.skip(POSIX_SET_TARGET_USER_ENV_VARS_MESSAGE) + + from subprocess import PIPE, run + + from openjd.sessions import _subprocess as subprocess_mod + + me = getpass.getuser() + + # WHEN the workload is launched through sudo's login shell with the shim + result = run( + [ + "sudo", + "-n", + "-u", + me, + "-i", + subprocess_mod._macos_shim_interpreter(), + "-I", + "-c", + subprocess_mod._MACOS_SETSID_SHIM, + "/bin/sh", + "-c", + "echo $$ $(ps -o pgid= -p $$)", + ], + stdout=PIPE, + stderr=PIPE, + text=True, + ) + + # THEN the shim arrived intact and the workload leads its own process group. A + # mangled shim surfaces as a non-zero exit (SyntaxError, or "command not found") + # rather than as a wrong pgid, so the exit status is asserted too. + assert result.returncode == 0, ( + "launch through 'sudo -i' failed, which is what a mis-quoted shim looks like: " + f"stdout={result.stdout!r} stderr={result.stderr!r}" + ) + workload_pid, workload_pgid = (int(x) for x in result.stdout.split()) + assert workload_pid == workload_pgid + + def test_shim_restores_default_signal_dispositions(self) -> None: + """The workload must not inherit CPython's SIGPIPE/SIGXFSZ ignores. + + CPython sets both to SIG_IGN during startup, and SIG_IGN survives exec (installed + handlers do not). Without the explicit reset in the shim, every impersonated macOS + workload would run with SIGPIPE ignored, so `producer | head` would see EPIPE write + errors instead of dying on the signal -- a silent divergence from Linux, where + setsid(1) is a C binary that never touches these dispositions. + + Probed with perl rather than python or sh: a Python process reports SIG_IGN for + SIGPIPE no matter what it inherited, and /bin/sh's `trap -p` prints nothing for an + inherited ignore, so neither can tell the two cases apart. perl reports "IGNORE" vs + "DEFAULT" and is present on every macOS host and on the Linux CI images. + """ + from shutil import which + from subprocess import PIPE, run + + from openjd.sessions import _subprocess as subprocess_mod + + perl = which("perl") + if perl is None: + pytest.skip("perl is not installed on this host") + + # GIVEN a probe that reports the dispositions it inherited + script = ( + 'print "SIGPIPE=", (defined $SIG{PIPE} ? $SIG{PIPE} : "DEFAULT"),' + ' " SIGXFSZ=", (defined $SIG{XFSZ} ? $SIG{XFSZ} : "DEFAULT"), "\n"' + ) + + # WHEN it is exec'd through the shim + result = run( + [sys.executable, "-I", "-c", subprocess_mod._MACOS_SETSID_SHIM, perl, "-e", script], + stdout=PIPE, + stderr=PIPE, + text=True, + ) + + # THEN both arrive at their defaults, not ignored. (Verified to fail against a shim + # without the reset, which reports "SIGPIPE=IGNORE SIGXFSZ=IGNORE".) + assert result.returncode == 0, f"probe failed: {result.stderr!r}" + assert ( + "SIGPIPE=DEFAULT" in result.stdout + ), f"workload inherited a non-default SIGPIPE: {result.stdout!r}" + assert ( + "SIGXFSZ=DEFAULT" in result.stdout + ), f"workload inherited a non-default SIGXFSZ: {result.stdout!r}" + + +@pytest.mark.skipif(not is_macos(), reason="macOS-specific interpreter selection") +class TestMacOSShimInterpreter: + """Tests for _macos_shim_interpreter(), which selects the Python interpreter that runs + the setsid shim as the jobRunAsUser. + + Unlike the shim string, this selection logic is genuinely macOS-only (it exists to find + an interpreter the job user can execute, outside the agent's venv), so these are scoped + to macOS hosts. The permission check it relies on is covered by + TestOtherUsersCanExecute, which runs on every POSIX host; these tests patch + _other_users_can_execute to a constant, so the real permission logic is exercised + there rather than here.""" + + def test_prefers_base_executable(self, tmp_path: Path) -> None: + # GIVEN a reachable interpreter behind sys._base_executable + from openjd.sessions import _subprocess as subprocess_mod + + interpreter = tmp_path / "python3" + interpreter.touch() + + # WHEN + with ( + patch.object(subprocess_mod.sys, "_base_executable", str(interpreter), create=True), + patch.object(subprocess_mod, "_other_users_can_execute", return_value=True), + ): + result = subprocess_mod._macos_shim_interpreter() + + # THEN + assert result == str(interpreter.resolve()) + + def test_resolves_symlink_to_base_interpreter(self, tmp_path: Path) -> None: + # GIVEN _base_executable is a symlink (e.g. a framework/Homebrew shim) + from openjd.sessions import _subprocess as subprocess_mod + + real_interpreter = tmp_path / "python3.11" + real_interpreter.touch() + link = tmp_path / "python3" + link.symlink_to(real_interpreter) + + # WHEN + with ( + patch.object(subprocess_mod.sys, "_base_executable", str(link), create=True), + patch.object(subprocess_mod, "_other_users_can_execute", return_value=True), + ): + result = subprocess_mod._macos_shim_interpreter() + + # THEN the symlink is resolved to the real interpreter + assert result == str(real_interpreter.resolve()) + + def test_uses_sys_executable_when_base_executable_unset(self, tmp_path: Path) -> None: + # GIVEN _base_executable is None (not a venv); sys.executable is used instead + from openjd.sessions import _subprocess as subprocess_mod + + interpreter = tmp_path / "python3" + interpreter.touch() + + # WHEN + with ( + patch.object(subprocess_mod.sys, "_base_executable", None, create=True), + patch.object(subprocess_mod.sys, "executable", str(interpreter)), + patch.object(subprocess_mod, "_other_users_can_execute", return_value=True), + ): + result = subprocess_mod._macos_shim_interpreter() + + # THEN + assert result == str(interpreter.resolve()) + + def test_falls_back_when_base_not_executable_by_others(self, tmp_path: Path) -> None: + # GIVEN the base interpreter is not reachable by other users, but the fallback is + from openjd.sessions import _subprocess as subprocess_mod + + interpreter = tmp_path / "python3" + interpreter.touch() + + def reachable(path: str) -> bool: + return path == subprocess_mod._MACOS_FALLBACK_SHIM_INTERPRETER + + # WHEN + with ( + patch.object(subprocess_mod.sys, "_base_executable", str(interpreter), create=True), + patch.object(subprocess_mod, "_other_users_can_execute", side_effect=reachable), + ): + result = subprocess_mod._macos_shim_interpreter() + + # THEN + assert result == subprocess_mod._MACOS_FALLBACK_SHIM_INTERPRETER + + def test_raises_actionable_error_when_no_interpreter_is_reachable(self, tmp_path: Path) -> None: + """With neither candidate reachable, fail at selection with an explanation. + + Returning the fallback unchecked would defer the failure to Popen, which reports + only "[Errno 2] No such file or directory: '/usr/bin/python3'" on a workload that + may have nothing to do with Python. The message is asserted rather than just the + exception type, because it is the only thing the operator sees: the caller logs + str(e) and returns None rather than propagating. + """ + # GIVEN neither the base interpreter nor the fallback is reachable + from openjd.sessions import _subprocess as subprocess_mod + + interpreter = tmp_path / "python3" + interpreter.touch() + + # WHEN + with ( + patch.object(subprocess_mod.sys, "_base_executable", str(interpreter), create=True), + patch.object(subprocess_mod, "_other_users_can_execute", return_value=False), + ): + with pytest.raises(subprocess_mod.NoReachableInterpreterError) as excinfo: + subprocess_mod._macos_shim_interpreter() + + # THEN the message names both rejected candidates and the remedy + message = str(excinfo.value) + assert str(interpreter) in message, "the rejected base interpreter must be named" + assert subprocess_mod._MACOS_FALLBACK_SHIM_INTERPRETER in message + assert "xcode-select --install" in message, "the remedy must be actionable" + # And it explains WHY an interpreter is involved at all, since the workload + # being launched may have nothing to do with Python. + assert "setsid" in message + + def test_no_reachable_interpreter_reaches_the_operator_as_a_start_failure( + self, tmp_path: Path, queue_handler: QueueHandler, message_queue: SimpleQueue + ) -> None: + """The raised message must survive the path back to the operator. + + _start_subprocess catches Exception, logs "Process failed to start: {e}" and + returns None; nothing re-raises. So the message text is the entire diagnostic, + and this pins that it is not swallowed or replaced along the way. + """ + # GIVEN a cross-user launch on macOS with no reachable interpreter + from openjd.sessions import _subprocess as subprocess_mod + + logger = build_logger(queue_handler) + target_user = MagicMock(spec=PosixSessionUser) + target_user.user = "job-user" + target_user.is_process_user.return_value = False + subproc = LoggingSubprocess( + logger=logger, + args=["/path/to/workload.sh"], + user=target_user, + ) + + # WHEN + with ( + patch.object(subprocess_mod, "is_macos", return_value=True), + patch.object(subprocess_mod, "is_posix", return_value=True), + patch.object(subprocess_mod, "is_windows", return_value=False), + patch.object(subprocess_mod, "_other_users_can_execute", return_value=False), + ): + result = subproc._start_subprocess() + + # THEN the launch fails and the operator gets the actionable message + assert result is None + messages = collect_queue_messages(message_queue) + assert any( + "Process failed to start" in m and "xcode-select --install" in m for m in messages + ), f"actionable message did not reach the log; got: {messages}" + + +class TestOtherUsersCanExecute: + """Tests for _other_users_can_execute(), the permission check behind interpreter + selection. + + POSIX-scoped rather than macOS-scoped even though only macOS calls it: the logic is + plain permission bits (o+x on the file, o+x on every ancestor, OSError -> False) with + nothing platform-specific in it, so running it on the Linux legs too is free signal on + the check that decides whether cross-user execution is possible at all. Same reasoning + as TestSetsidShimBehavior. The per-test markers below stay meaningful because this + class is not itself gated on darwin. + """ + + @pytest.mark.skipif(not is_posix(), reason="POSIX permission-bit semantics") + def test_other_users_can_execute_system_binary(self) -> None: + # GIVEN a system binary that is world-executable with world-traversable parents + from openjd.sessions import _subprocess as subprocess_mod + + # THEN + assert subprocess_mod._other_users_can_execute("/bin/sh") + + @pytest.mark.skipif(is_windows(), reason="POSIX permission bits are not honored on Windows") + def test_other_users_cannot_execute_without_o_x_bit(self, tmp_path: Path) -> None: + # GIVEN a file that other users cannot execute (no o+x bit) + from openjd.sessions import _subprocess as subprocess_mod + + interpreter = tmp_path / "python3" + interpreter.touch() + interpreter.chmod(0o750) + + # THEN + assert not subprocess_mod._other_users_can_execute(str(interpreter)) + + @pytest.mark.skipif(is_windows(), reason="POSIX permission bits are not honored on Windows") + def test_other_users_cannot_execute_behind_private_dir(self, tmp_path: Path) -> None: + # GIVEN a world-executable file inside a directory that other users cannot + # traverse (e.g. a Python install under a 0o750 home directory) + from openjd.sessions import _subprocess as subprocess_mod + + private_dir = tmp_path / "private" + private_dir.mkdir() + interpreter = private_dir / "python3" + interpreter.touch() + interpreter.chmod(0o755) + private_dir.chmod(0o750) + + # WHEN + try: + result = subprocess_mod._other_users_can_execute(str(interpreter)) + finally: + # Restore so pytest can clean up tmp_path + private_dir.chmod(0o755) + + # THEN + assert not result + + def test_other_users_cannot_execute_missing_path(self, tmp_path: Path) -> None: + # GIVEN a path that does not exist + from openjd.sessions import _subprocess as subprocess_mod + + # THEN + assert not subprocess_mod._other_users_can_execute(str(tmp_path / "no-such-python")) + + class TestFastExitingChild: """A trivial command can exit before the runner finishes recording it.