From 8dba30b8847e4c6a2ed148ff93a2ad887263fb07 Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Thu, 16 Jul 2026 16:25:18 -0700 Subject: [PATCH 01/10] feat: support running Sessions as a jobRunAsUser on macOS openjd-sessions could not run a Session's actions as a separate user (jobRunAsUser) on macOS. Three problems in the POSIX cross-user path: - setsid(1) does not exist on macOS, so the cross-user command 'sudo -u -i setsid -w ' failed at spawn (exit 127), breaking all jobRunAsUser execution on macOS. - find_child_process_id_pgrep treated pgrep's exit code 1 (no match yet) as a fatal error, which broke out of the caller's retry loop. Since Linux uses procfs and only other POSIX platforms use pgrep, signal-target discovery never actually retried off Linux. - There was no is_macos() helper (the MACOS constant was unused). Changes: - _os_checker.py: add is_macos(). - _subprocess.py: on darwin, launch the workload via a pure-Python setsid shim ('sudo -u -i /usr/bin/python3 -I -c ') that makes the workload a new session/process-group leader before exec, reproducing the new-session behavior 'setsid -w' provides on Linux. Runs under /usr/bin/python3 (so the job user needs no venv access) with -I (isolated mode, so the working directory is not on sys.path). - _linux/_sudo.py: treat pgrep exit code 1 as "no match yet -> return None" so the retry loop polls as intended; exit >1 is still fatal. Fixes discovery and cancellation on all non-Linux POSIX hosts. - Tests for is_macos(), the pgrep exit-code handling, and the macOS command construction (plus a POSIX check that the shim creates a new process group). - README: document the macOS Command Line Tools prerequisite for impersonation. Linux and Windows behavior is unchanged. Validated end-to-end on macOS 26.5 (arm64) via the AWS Deadline Cloud worker agent against a live customer-managed fleet: running an action as a jobRunAsUser and cancellation reaping the workload's process group with no orphans. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- README.md | 9 + src/openjd/sessions/_linux/_sudo.py | 6 + src/openjd/sessions/_os_checker.py | 4 + src/openjd/sessions/_subprocess.py | 105 ++++++++++- test/openjd/sessions_v0/test_os_checker.py | 12 +- test/openjd/sessions_v0/test_subprocess.py | 208 +++++++++++++++++++++ test/openjd/sessions_v0/test_sudo.py | 71 +++++++ 7 files changed, 412 insertions(+), 3 deletions(-) create mode 100644 test/openjd/sessions_v0/test_sudo.py 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/src/openjd/sessions/_linux/_sudo.py b/src/openjd/sessions/_linux/_sudo.py index 43a1fa8c..9fd5ec4b 100644 --- a/src/openjd/sessions/_linux/_sudo.py +++ b/src/openjd/sessions/_linux/_sudo.py @@ -143,6 +143,12 @@ def find_child_process_id_pgrep( stdin=DEVNULL, text=True, ) + # pgrep exit codes: 0 = one or more processes matched; 1 = no processes matched; + # >1 = an actual error (syntax/operational). Exit 1 is NOT an error here -- it just + # means sudo has not spawned its child yet, so we return None to let the caller's + # retry loop poll again. + if pgrep_result.returncode == 1: + return None if pgrep_result.returncode != 0: raise FindSignalTargetError("Unable to query child processes of sudo process") results = pgrep_result.stdout.splitlines() 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 9b21a4ef..fefc2b86 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,84 @@ __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. 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. +# +# 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. +_MACOS_SETSID_SHIM = ( + "import os,sys;os.getpgrp()==os.getpid() or os.setsid();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: it ignores group permissions and ACLs that might + also grant access, so it can return False for a path some specific user could execute. + """ + 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 + + +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. + """ + base = os.path.realpath(getattr(sys, "_base_executable", None) or sys.executable) + if _other_users_can_execute(base): + return base + return _MACOS_FALLBACK_SHIM_INTERPRETER + + # ======================================================================== # ======================================================================== # DEVELOPER NOTE: @@ -257,7 +336,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_subprocess.py b/test/openjd/sessions_v0/test_subprocess.py index 6c014546..03ea7abf 100644 --- a/test/openjd/sessions_v0/test_subprocess.py +++ b/test/openjd/sessions_v0/test_subprocess.py @@ -1069,3 +1069,211 @@ def end_proc(): if num_children_running == 0: break 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="posix-specific test") + 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 + + +class TestMacOSShimInterpreter: + """Tests for _macos_shim_interpreter(), which selects the Python interpreter that runs + the setsid shim as the jobRunAsUser, and for the _other_users_can_execute() permission + check that backs it.""" + + 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/executable by other users + 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), + ): + result = subprocess_mod._macos_shim_interpreter() + + # THEN + assert result == subprocess_mod._MACOS_FALLBACK_SHIM_INTERPRETER + + @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")) diff --git a/test/openjd/sessions_v0/test_sudo.py b/test/openjd/sessions_v0/test_sudo.py new file mode 100644 index 00000000..eba97843 --- /dev/null +++ b/test/openjd/sessions_v0/test_sudo.py @@ -0,0 +1,71 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +from subprocess import CompletedProcess +from unittest.mock import MagicMock, patch + +import pytest + +from openjd.sessions._linux._sudo import ( + FindSignalTargetError, + find_child_process_id_pgrep, +) + + +def _pgrep_result(returncode: int, stdout: str = "") -> CompletedProcess: + return CompletedProcess(args=["pgrep"], returncode=returncode, stdout=stdout) + + +class TestFindChildProcessIdPgrep: + """Tests for the pgrep-based signal-target discovery used on non-Linux POSIX hosts.""" + + @patch("openjd.sessions._linux._sudo.run") + def test_returns_child_pid_on_match(self, mock_run: MagicMock) -> None: + # GIVEN pgrep matches a single child process + mock_run.return_value = _pgrep_result(returncode=0, stdout="4321\n") + + # WHEN + result = find_child_process_id_pgrep(sudo_pid=1234) + + # THEN + assert result == 4321 + + @patch("openjd.sessions._linux._sudo.run") + def test_returns_none_when_no_match_yet(self, mock_run: MagicMock) -> None: + # GIVEN pgrep finds no matching processes (exit code 1) -- e.g. sudo has not + # spawned its child yet. This must return None (so the caller retries), NOT raise. + mock_run.return_value = _pgrep_result(returncode=1, stdout="") + + # WHEN + result = find_child_process_id_pgrep(sudo_pid=1234) + + # THEN + assert result is None + + @patch("openjd.sessions._linux._sudo.run") + def test_raises_on_pgrep_error(self, mock_run: MagicMock) -> None: + # GIVEN pgrep reports an actual error (exit code > 1) + mock_run.return_value = _pgrep_result(returncode=2, stdout="") + + # WHEN / THEN + with pytest.raises(FindSignalTargetError): + find_child_process_id_pgrep(sudo_pid=1234) + + @patch("openjd.sessions._linux._sudo.run") + def test_raises_on_multiple_children(self, mock_run: MagicMock) -> None: + # GIVEN pgrep matches more than one child, violating the single-child assumption + mock_run.return_value = _pgrep_result(returncode=0, stdout="4321\n4322\n") + + # WHEN / THEN + with pytest.raises(FindSignalTargetError): + find_child_process_id_pgrep(sudo_pid=1234) + + @patch("openjd.sessions._linux._sudo.run") + def test_returns_none_on_empty_stdout_with_success(self, mock_run: MagicMock) -> None: + # GIVEN pgrep exits 0 but with no pids in stdout (defensive: treat as no match) + mock_run.return_value = _pgrep_result(returncode=0, stdout="") + + # WHEN + result = find_child_process_id_pgrep(sudo_pid=1234) + + # THEN + assert result is None From 16d0a137f6d748cd9b673faff350cf8c9a60d3b2 Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Fri, 17 Jul 2026 10:07:45 -0700 Subject: [PATCH 02/10] test: run POSIX cross-user impersonation tests on macOS in CI The impersonation tests already exist but xfail everywhere the OPENJD_TEST_SUDO_* environment variables are unset; on Linux they run inside a purpose-built Docker container, and nothing runs them on macOS. macOS runners have passwordless sudo, so this workflow provisions the same user/group layout with Directory Services (sysadminctl/dseditgroup) and runs the existing tests for real, covering the macOS setsid-shim launch, signalling, and process-tree termination paths end to end. A guard step fails the job if the tests regress to xfail (e.g. the provisioning breaks), instead of silently passing an empty run. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- .github/workflows/macos_cross_user_test.yml | 169 ++++++++++++++++++++ 1 file changed, 169 insertions(+) create mode 100644 .github/workflows/macos_cross_user_test.yml diff --git a/.github/workflows/macos_cross_user_test.yml b/.github/workflows/macos_cross_user_test.yml new file mode 100644 index 00000000..68e3611d --- /dev/null +++ b/.github/workflows/macos_cross_user_test.yml @@ -0,0 +1,169 @@ +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 runner's passwordless sudo is used to provision the same user/group layout +# that testing_containers/localuser_sudo_environment/Dockerfile creates for Linux: +# runner -- runs the pytests (member of the shared group) +# openjd-target -- the impersonated user (member of the shared group) +# openjd-disjoint -- a user with no group in common (temp-dir permission tests) + +on: + workflow_dispatch: + pull_request: + branches: [ mainline, release ] + paths: + - 'src/openjd/sessions/_subprocess.py' + - 'src/openjd/sessions/_linux/_sudo.py' + - 'src/openjd/sessions/_os_checker.py' + - 'src/openjd/sessions/_tempdir.py' + - '.github/workflows/macos_cross_user_test.yml' + +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 + # Hatch's default data dir is under ~/Library, which other users cannot traverse. + # The impersonation tests execute the hatch venv's python as the target user, so + # the venv must live somewhere world-traversable. + HATCH_DATA_DIR: /opt/hatch + # macOS's default 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 + # world-writable, already-resolved temp root instead, which matches the /tmp + # semantics the impersonation tests get on Linux. Created during provisioning + # owned by runner:staff because BSD filesystems give new files the GROUP OF THE + # PARENT DIRECTORY (not the creator's gid), and the same-user TempDir test + # asserts the created directory has the creating process's gid. + TMPDIR: /private/tmp/openjd-tests + +jobs: + macos-cross-user: + name: Python ${{ matrix.python-version }} + runs-on: macos-latest + timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + python-version: ['3.11', '3.13'] + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: ${{ matrix.python-version }} + + - name: Provision test users and groups + run: | + set -euxo pipefail + + # Groups + 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 + # test_cleanup_posix_user chowns to "user:user"; macOS does not, so + # create the self-named group explicitly. runner must NOT be a member. + 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 runner + 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 runner -t user "${OPENJD_TEST_SUDO_SHARED_GROUP}" + + # Passwordless sudo from runner to the target user (and itself), mirroring + # the hostuser rule in the Linux test container + echo "runner ALL=(${OPENJD_TEST_SUDO_TARGET_USER},runner) 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 + + # test_basic_operation runs a bare `python` as the target user via + # `sudo -i`; macOS ships python3 only, so provide the alias. + sudo mkdir -p /usr/local/bin + sudo ln -sf /usr/bin/python3 /usr/local/bin/python + + # Flush Directory Services caches so the new users/groups resolve + sudo dscacheutil -flushcache + + # World-writable temp root for the tests (see TMPDIR at the top of the file) + sudo mkdir -p "${TMPDIR}" + sudo chown runner:staff "${TMPDIR}" + sudo chmod 1777 "${TMPDIR}" + + - name: Verify provisioning + run: | + set -euxo pipefail + id "${OPENJD_TEST_SUDO_TARGET_USER}" + id "${OPENJD_TEST_SUDO_DISJOINT_USER}" + id runner + # The isolation invariant the tests rely on: runner and the target user + # share OPENJD_TEST_SUDO_SHARED_GROUP; the disjoint user shares nothing. + id -Gn runner | 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())' + + - name: Install hatch + run: | + sudo mkdir -p "${HATCH_DATA_DIR}" + sudo chown runner "${HATCH_DATA_DIR}" + pip install hatch + + - name: Create test environment + run: | + set -euxo pipefail + hatch env create + # The target user executes the venv python and reads test support files; + # make the venv and the workspace world-readable/traversable. + chmod -R o+rX "${HATCH_DATA_DIR}" "${GITHUB_WORKSPACE}" + + - name: Report 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. + run: | + 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)') + " + + - name: Run cross-user impersonation tests + run: | + set -euxo pipefail + # -rxX lists (x)failed and (X)passed-unexpectedly tests in the summary so the + # next step can assert nothing silently xfailed back to a no-op. + hatch run test -- test/openjd/sessions_v0/test_subprocess.py test/openjd/sessions_v0/test_tempdir.py \ + --no-cov -rxX 2>&1 | tee pytest-cross-user.log + + - name: Assert impersonation tests actually ran + run: | + set -euxo pipefail + # If the OPENJD_TEST_SUDO_* wiring regresses, the impersonation tests xfail + # with this message instead of failing the job -- catch that here. + if grep -q "Must define environment vars OPENJD_TEST_SUDO" pytest-cross-user.log; then + echo "Impersonation tests were skipped (env vars not picked up); provisioning is broken." + exit 1 + fi + + - name: Run remaining tests + run: hatch run test -- --ignore test/openjd/sessions_v0/test_subprocess.py --ignore test/openjd/sessions_v0/test_tempdir.py --no-cov From e40eee50d2888ab24dd42ab5eea3bb0377b87075 Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:29:15 -0700 Subject: [PATCH 03/10] fix: wait for the stdout-filter thread in test_def_via_stdout_fails_session_action_on_error The test asserted the parse-error log message immediately after observing the session leave RUNNING, but the message is emitted by the subprocess stdout-filter thread, which can still be draining the pipe at that point. The race is intermittent on Windows CI (observed repeatedly on python 3.9/3.14 windows-latest runners), where it fails with the message absent from caplog and then passes on re-run. Poll briefly for the message instead of racing the thread. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- test/openjd/sessions_v0/test_session.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/openjd/sessions_v0/test_session.py b/test/openjd/sessions_v0/test_session.py index f3420a92..0cfdb9c1 100644 --- a/test/openjd/sessions_v0/test_session.py +++ b/test/openjd/sessions_v0/test_session.py @@ -3027,10 +3027,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( [ From bdcffd56fa54a2b5a4b2004595fff6c58b0657c5 Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Mon, 10 Aug 2026 07:45:00 -0700 Subject: [PATCH 04/10] test: address review feedback on the macOS cross-user test scaffolding Seven changes, all to test/CI scaffolding; no change to the shim or to _subprocess.py behaviour. 1. Delete test/openjd/sessions_v0/test_sudo.py. It duplicated the pre-existing TestFindChildProcessIdPgrep in test_linux_sudo.py case for case (5 of 5), and the existing versions are better: the no-match case drives real pgrep against a real childless process instead of mocking the return, the error case is parametrized over three exit codes and asserts the message content, and the adjacent TestFindSudoChildProcessGroupId covers the late-child race the fix exists for. Nothing was lost; full suite still 936 passed. 2. Move test_setsid_shim_creates_new_process_group out of TestLoggingSubprocessMacOSSetsid into a new POSIX-scoped TestSetsidShimBehavior. It was macOS-named but gated on is_posix(), so it ran on Linux inside a macOS class. Kept POSIX rather than narrowed to macOS: the shim is portable stdlib (os.getpgrp/getpid/setsid/execvp, no platform branch) and macOS is only where it is *required*, so running it on Linux too catches a broken shim string on faster runners. Whether macOS selects the shim is a separate concern already covered by test_builds_setsid_shim_command_on_macos. 3. Add a class-level macOS skipif to TestMacOSShimInterpreter. Unlike the shim string, _macos_shim_interpreter() really is macOS-only selection logic. 4. Extract the workflow's ~80 lines of inline provisioning into scripts/run_macos_sudo_tests.sh, mirroring scripts/run_sudo_tests.sh so the job is reproducible on a developer's Mac. Because macOS cannot be containerized, the script is the counterpart to the Linux Dockerfile rather than to the docker command: it provisions, runs and then removes the users, groups, sudoers file, python symlink and temp root it created. Teardown is best-effort throughout so a partial provision is still removable, and --keep / --cleanup-only cover the CI and recovery cases. 5. Add cross-user-test / cross-user-test-macos hatch scripts so neither platform's cross-user suite requires remembering a script path. 6. Expand the Python matrix from ['3.11','3.13'] to 3.9-3.14, matching code_quality.yml and requires-python >=3.9. 7. Drop the paths: filter so the job runs on every PR. The cross-user path can break from more places than a four-file list can enumerate, and a filtered job that misses one reads as a pass. Also drops the workflow's final "run remaining tests" step: code_quality.yml already runs the whole suite on macos-latest across this same matrix, so that step only duplicated it. This job now covers the cross-user tests alone. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- .github/workflows/macos_cross_user_test.yml | 160 ++------------- hatch.toml | 6 + scripts/run_macos_sudo_tests.sh | 212 ++++++++++++++++++++ test/openjd/sessions_v0/test_subprocess.py | 26 ++- test/openjd/sessions_v0/test_sudo.py | 71 ------- 5 files changed, 263 insertions(+), 212 deletions(-) create mode 100755 scripts/run_macos_sudo_tests.sh delete mode 100644 test/openjd/sessions_v0/test_sudo.py diff --git a/.github/workflows/macos_cross_user_test.yml b/.github/workflows/macos_cross_user_test.yml index 68e3611d..4731f391 100644 --- a/.github/workflows/macos_cross_user_test.yml +++ b/.github/workflows/macos_cross_user_test.yml @@ -6,41 +6,24 @@ name: macOS Cross-User Tests # process launch as another user, new-process-group creation, signalling, and # process-tree termination. # -# The runner's passwordless sudo is used to provision the same user/group layout -# that testing_containers/localuser_sudo_environment/Dockerfile creates for Linux: -# runner -- runs the pytests (member of the shared group) -# openjd-target -- the impersonated user (member of the shared group) -# openjd-disjoint -- a user with no group in common (temp-dir permission tests) +# 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 ] - paths: - - 'src/openjd/sessions/_subprocess.py' - - 'src/openjd/sessions/_linux/_sudo.py' - - 'src/openjd/sessions/_os_checker.py' - - 'src/openjd/sessions/_tempdir.py' - - '.github/workflows/macos_cross_user_test.yml' - -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 - # Hatch's default data dir is under ~/Library, which other users cannot traverse. - # The impersonation tests execute the hatch venv's python as the target user, so - # the venv must live somewhere world-traversable. - HATCH_DATA_DIR: /opt/hatch - # macOS's default 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 - # world-writable, already-resolved temp root instead, which matches the /tmp - # semantics the impersonation tests get on Linux. Created during provisioning - # owned by runner:staff because BSD filesystems give new files the GROUP OF THE - # PARENT DIRECTORY (not the creator's gid), and the same-user TempDir test - # asserts the created directory has the creating process's gid. - TMPDIR: /private/tmp/openjd-tests jobs: macos-cross-user: @@ -50,7 +33,10 @@ jobs: strategy: fail-fast: false matrix: - python-version: ['3.11', '3.13'] + # 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 @@ -58,112 +44,10 @@ jobs: with: python-version: ${{ matrix.python-version }} - - name: Provision test users and groups - run: | - set -euxo pipefail - - # Groups - 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 - # test_cleanup_posix_user chowns to "user:user"; macOS does not, so - # create the self-named group explicitly. runner must NOT be a member. - 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 runner - 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 runner -t user "${OPENJD_TEST_SUDO_SHARED_GROUP}" - - # Passwordless sudo from runner to the target user (and itself), mirroring - # the hostuser rule in the Linux test container - echo "runner ALL=(${OPENJD_TEST_SUDO_TARGET_USER},runner) 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 - - # test_basic_operation runs a bare `python` as the target user via - # `sudo -i`; macOS ships python3 only, so provide the alias. - sudo mkdir -p /usr/local/bin - sudo ln -sf /usr/bin/python3 /usr/local/bin/python - - # Flush Directory Services caches so the new users/groups resolve - sudo dscacheutil -flushcache - - # World-writable temp root for the tests (see TMPDIR at the top of the file) - sudo mkdir -p "${TMPDIR}" - sudo chown runner:staff "${TMPDIR}" - sudo chmod 1777 "${TMPDIR}" - - - name: Verify provisioning - run: | - set -euxo pipefail - id "${OPENJD_TEST_SUDO_TARGET_USER}" - id "${OPENJD_TEST_SUDO_DISJOINT_USER}" - id runner - # The isolation invariant the tests rely on: runner and the target user - # share OPENJD_TEST_SUDO_SHARED_GROUP; the disjoint user shares nothing. - id -Gn runner | 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())' - - name: Install hatch - run: | - sudo mkdir -p "${HATCH_DATA_DIR}" - sudo chown runner "${HATCH_DATA_DIR}" - pip install hatch - - - name: Create test environment - run: | - set -euxo pipefail - hatch env create - # The target user executes the venv python and reads test support files; - # make the venv and the workspace world-readable/traversable. - chmod -R o+rX "${HATCH_DATA_DIR}" "${GITHUB_WORKSPACE}" - - - name: Report 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. - run: | - 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)') - " - - - name: Run cross-user impersonation tests - run: | - set -euxo pipefail - # -rxX lists (x)failed and (X)passed-unexpectedly tests in the summary so the - # next step can assert nothing silently xfailed back to a no-op. - hatch run test -- test/openjd/sessions_v0/test_subprocess.py test/openjd/sessions_v0/test_tempdir.py \ - --no-cov -rxX 2>&1 | tee pytest-cross-user.log - - - name: Assert impersonation tests actually ran - run: | - set -euxo pipefail - # If the OPENJD_TEST_SUDO_* wiring regresses, the impersonation tests xfail - # with this message instead of failing the job -- catch that here. - if grep -q "Must define environment vars OPENJD_TEST_SUDO" pytest-cross-user.log; then - echo "Impersonation tests were skipped (env vars not picked up); provisioning is broken." - exit 1 - fi + run: pip install hatch - - name: Run remaining tests - run: hatch run test -- --ignore test/openjd/sessions_v0/test_subprocess.py --ignore test/openjd/sessions_v0/test_tempdir.py --no-cov + - 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/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..8e501f91 --- /dev/null +++ b/scripts/run_macos_sudo_tests.sh @@ -0,0 +1,212 @@ +#!/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. + +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. +export TMPDIR="${TMPDIR_OVERRIDE:-/private/tmp/openjd-tests}" + +TEST_USER="${SUDO_USER:-$(id -un)}" +SUDOERS_FILE=/etc/sudoers.d/openjd-cross-user-tests +PYTHON_SHIM=/usr/local/bin/python + +KEEP="False" +CLEANUP_ONLY="False" +PYTEST_ARGS=() +while [[ "${1:-}" != "" ]]; do + case $1 in + -h|--help) + sed -n '4,26p' "$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 + sudo rm -f "${PYTHON_SHIM}" || true + 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 + sudo rm -rf "${TMPDIR}" || true + sudo dscacheutil -flushcache || true + echo "--- Done ---" +} + +if [[ "${CLEANUP_ONLY}" == "True" ]]; then + 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. + sudo mkdir -p "$(dirname "${PYTHON_SHIM}")" + sudo ln -sf /usr/bin/python3 "${PYTHON_SHIM}" + + sudo dscacheutil -flushcache + + sudo mkdir -p "${TMPDIR}" + # 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}" +hatch env create +# The target user executes the venv python and reads test support files, so the +# venv and the workspace must be world-readable/traversable. +chmod -R o+rX "${HATCH_DATA_DIR}" "$(pwd)" + +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/test/openjd/sessions_v0/test_subprocess.py b/test/openjd/sessions_v0/test_subprocess.py index 499043ea..23c30c48 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 @@ -1326,7 +1326,22 @@ def test_builds_setsid_shim_command_on_macos(self, queue_handler: QueueHandler) "/path/to/workload.sh", ] - @pytest.mark.skipif(not is_posix(), reason="posix-specific test") + +@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 @@ -1355,10 +1370,15 @@ def test_setsid_shim_creates_new_process_group(self) -> None: assert workload_pid == workload_pgid +@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, and for the _other_users_can_execute() permission - check that backs it.""" + check that backs it. + + 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.""" def test_prefers_base_executable(self, tmp_path: Path) -> None: # GIVEN a reachable interpreter behind sys._base_executable diff --git a/test/openjd/sessions_v0/test_sudo.py b/test/openjd/sessions_v0/test_sudo.py deleted file mode 100644 index eba97843..00000000 --- a/test/openjd/sessions_v0/test_sudo.py +++ /dev/null @@ -1,71 +0,0 @@ -# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - -from subprocess import CompletedProcess -from unittest.mock import MagicMock, patch - -import pytest - -from openjd.sessions._linux._sudo import ( - FindSignalTargetError, - find_child_process_id_pgrep, -) - - -def _pgrep_result(returncode: int, stdout: str = "") -> CompletedProcess: - return CompletedProcess(args=["pgrep"], returncode=returncode, stdout=stdout) - - -class TestFindChildProcessIdPgrep: - """Tests for the pgrep-based signal-target discovery used on non-Linux POSIX hosts.""" - - @patch("openjd.sessions._linux._sudo.run") - def test_returns_child_pid_on_match(self, mock_run: MagicMock) -> None: - # GIVEN pgrep matches a single child process - mock_run.return_value = _pgrep_result(returncode=0, stdout="4321\n") - - # WHEN - result = find_child_process_id_pgrep(sudo_pid=1234) - - # THEN - assert result == 4321 - - @patch("openjd.sessions._linux._sudo.run") - def test_returns_none_when_no_match_yet(self, mock_run: MagicMock) -> None: - # GIVEN pgrep finds no matching processes (exit code 1) -- e.g. sudo has not - # spawned its child yet. This must return None (so the caller retries), NOT raise. - mock_run.return_value = _pgrep_result(returncode=1, stdout="") - - # WHEN - result = find_child_process_id_pgrep(sudo_pid=1234) - - # THEN - assert result is None - - @patch("openjd.sessions._linux._sudo.run") - def test_raises_on_pgrep_error(self, mock_run: MagicMock) -> None: - # GIVEN pgrep reports an actual error (exit code > 1) - mock_run.return_value = _pgrep_result(returncode=2, stdout="") - - # WHEN / THEN - with pytest.raises(FindSignalTargetError): - find_child_process_id_pgrep(sudo_pid=1234) - - @patch("openjd.sessions._linux._sudo.run") - def test_raises_on_multiple_children(self, mock_run: MagicMock) -> None: - # GIVEN pgrep matches more than one child, violating the single-child assumption - mock_run.return_value = _pgrep_result(returncode=0, stdout="4321\n4322\n") - - # WHEN / THEN - with pytest.raises(FindSignalTargetError): - find_child_process_id_pgrep(sudo_pid=1234) - - @patch("openjd.sessions._linux._sudo.run") - def test_returns_none_on_empty_stdout_with_success(self, mock_run: MagicMock) -> None: - # GIVEN pgrep exits 0 but with no pids in stdout (defensive: treat as no match) - mock_run.return_value = _pgrep_result(returncode=0, stdout="") - - # WHEN - result = find_child_process_id_pgrep(sudo_pid=1234) - - # THEN - assert result is None From c016ea6b51b0e72f8cd971182f0d9bff45e3a517 Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Mon, 10 Aug 2026 08:19:50 -0700 Subject: [PATCH 05/10] fix: raise an actionable error when no interpreter is reachable on macOS _macos_shim_interpreter() returned /usr/bin/python3 without checking it. When the Command Line Tools are absent, or the fallback is itself unreachable, that path deferred the failure to Popen, and the operator saw only: Process failed to start: [Errno 2] No such file or directory: '/usr/bin/python3' on a workload that may have nothing to do with Python, naming neither the cause (cross-user execution needs an interpreter the job user can execute) nor the fix. The fallback is now verified the same way as the base interpreter, and when neither is usable a NoReachableInterpreterError names both candidates that were tried and the remedy. The caller is unchanged: _start_subprocess catches it, logs "Process failed to start: {message}" and returns None, so the launch still fails via the existing failed_to_start path rather than propagating. Also fixes the shim symlink in scripts/run_macos_sudo_tests.sh. `ln -sf` replaced whatever was at /usr/local/bin/python and cleanup() removed it unconditionally, so a default run would have deleted a developer's pyenv/Homebrew/python.org symlink, contradicting the script's promise to remove only what it created. It now creates the alias only when the path is empty, records whether it did, and removes it only in that case. --cleanup-only, which runs before provisioning, claims the alias only when it points at /usr/bin/python3. Tests: the existing fallback test now models a reachable fallback rather than patching every path unreachable. Two added: one asserting the raised message names both candidates plus 'xcode-select --install', and one driving _start_subprocess to pin that the text survives to the log, since the caller swallows the exception and the message is the operator's only diagnostic. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- scripts/run_macos_sudo_tests.sh | 27 +++++++- src/openjd/sessions/_subprocess.py | 32 +++++++-- test/openjd/sessions_v0/test_subprocess.py | 77 +++++++++++++++++++++- 3 files changed, 127 insertions(+), 9 deletions(-) diff --git a/scripts/run_macos_sudo_tests.sh b/scripts/run_macos_sudo_tests.sh index 8e501f91..044d11b9 100755 --- a/scripts/run_macos_sudo_tests.sh +++ b/scripts/run_macos_sudo_tests.sh @@ -55,6 +55,10 @@ export TMPDIR="${TMPDIR_OVERRIDE:-/private/tmp/openjd-tests}" TEST_USER="${SUDO_USER:-$(id -un)}" 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" KEEP="False" CLEANUP_ONLY="False" @@ -81,7 +85,10 @@ cleanup() { # Best-effort throughout: a partially-provisioned environment must still be # removable, so nothing here may abort the teardown. sudo rm -f "${SUDOERS_FILE}" || true - sudo rm -f "${PYTHON_SHIM}" || 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. @@ -96,6 +103,13 @@ cleanup() { } if [[ "${CLEANUP_ONLY}" == "True" ]]; then + # --cleanup-only recovers from an interrupted run, where provision() never set + # PYTHON_SHIM_CREATED in this process. Only claim the alias 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 cleanup exit 0 fi @@ -135,9 +149,16 @@ provision() { 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. + # 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}")" - sudo ln -sf /usr/bin/python3 "${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 diff --git a/src/openjd/sessions/_subprocess.py b/src/openjd/sessions/_subprocess.py index f40184cf..0b986305 100644 --- a/src/openjd/sessions/_subprocess.py +++ b/src/openjd/sessions/_subprocess.py @@ -44,9 +44,11 @@ # * 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. sys.executable -# itself is not used directly because it may live inside a virtual environment that the -# jobRunAsUser has no traverse/read permission on. +# 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. @@ -86,6 +88,13 @@ def _other_users_can_execute(path: str) -> bool: 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. @@ -100,11 +109,26 @@ def _macos_shim_interpreter() -> str: 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 - return _MACOS_FALLBACK_SHIM_INTERPRETER + 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." + ) # ======================================================================== diff --git a/test/openjd/sessions_v0/test_subprocess.py b/test/openjd/sessions_v0/test_subprocess.py index 23c30c48..ca196014 100644 --- a/test/openjd/sessions_v0/test_subprocess.py +++ b/test/openjd/sessions_v0/test_subprocess.py @@ -1435,22 +1435,95 @@ def test_uses_sys_executable_when_base_executable_unset(self, tmp_path: Path) -> 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/executable by other users + # 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", return_value=False), + 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}" + @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 1538c4b7cd200e93ca01b0ab9b3d6d9872cf70ac Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Mon, 10 Aug 2026 08:28:22 -0700 Subject: [PATCH 06/10] ci: pin virtualenv<21 on Python 3.9 for the macOS cross-user job The 3.9 leg failed at `hatch env create` with "Environment `default` is incompatible: module 'virtualenv.discovery.builtin' has no attribute 'propose_interpreters'". virtualenv 21 removed that API, and the hatch version resolvable on 3.9 still calls it. Provisioning had already succeeded, so the job failed after creating users and groups. This surfaced now because expanding the matrix to 3.9-3.14 added the only affected version; at 3.11/3.13 the job never installed a virtualenv that old. Every other workflow in this repo already carries the same constraint (code_quality.yml, both e2e workflows, release_publish.yml), so this brings the new job in line rather than inventing a fix. The environment marker scopes it to 3.9 alone, leaving 3.10+ on current virtualenv. Also surface the cause in scripts/run_macos_sudo_tests.sh: a developer running it locally on 3.9 hits the same failure, and hatch's message names neither virtualenv nor the remedy. `hatch env create` failing now prints the pin to try, gated on the interpreter actually being older than 3.10. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- .github/workflows/macos_cross_user_test.yml | 6 +++++- scripts/run_macos_sudo_tests.sh | 14 +++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/.github/workflows/macos_cross_user_test.yml b/.github/workflows/macos_cross_user_test.yml index 4731f391..80a70720 100644 --- a/.github/workflows/macos_cross_user_test.yml +++ b/.github/workflows/macos_cross_user_test.yml @@ -45,7 +45,11 @@ jobs: python-version: ${{ matrix.python-version }} - name: Install hatch - run: pip 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 diff --git a/scripts/run_macos_sudo_tests.sh b/scripts/run_macos_sudo_tests.sh index 044d11b9..39df4d66 100755 --- a/scripts/run_macos_sudo_tests.sh +++ b/scripts/run_macos_sudo_tests.sh @@ -197,7 +197,19 @@ verify echo "--- Creating the test environment ---" sudo mkdir -p "${HATCH_DATA_DIR}" sudo chown "${TEST_USER}" "${HATCH_DATA_DIR}" -hatch env create +# 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 test support files, so the # venv and the workspace must be world-readable/traversable. chmod -R o+rX "${HATCH_DATA_DIR}" "$(pwd)" From 330877b12e8bcf420c674c235cc5adbe7b0f309d Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Mon, 10 Aug 2026 08:38:32 -0700 Subject: [PATCH 07/10] fix(scripts): narrow the permission grant and remove the TMPDIR override Two problems in run_macos_sudo_tests.sh, both only dangerous on the developer machine the script exists to serve. 1. `chmod -R o+rX "$(pwd)"` made the entire working tree world-readable, including untracked files and anything credential-bearing a developer keeps under the repo root, and nothing put those bits back. The impersonated user only needs to read test/openjd/sessions_v0/support_files, so the grant is now scoped to that directory plus o+x (traverse only, no read) on the directories leading to it. Verified: the support file becomes world-readable, the path directories become 701 so they cannot be listed, and a .env at the repo root stays 600. These bits are not undone by the teardown, which the header now says explicitly rather than leaving it to be discovered. 2. TMPDIR was read from TMPDIR_OVERRIDE, an undocumented name that silently ignored a caller's own TMPDIR, and cleanup() does `rm -rf` on the result. So `TMPDIR_OVERRIDE=/tmp scripts/run_macos_sudo_tests.sh` would have ended in `sudo rm -rf /tmp`. The override is gone: the tests need any world-traversable already-resolved directory, so there was nothing to configure. Removal is also now gated on having created the directory, matching the treatment of the /usr/local/bin/python alias, so an existing one is reused and left in place. Also widens the --help line range, which the longer header had outgrown. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- scripts/run_macos_sudo_tests.sh | 46 ++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/scripts/run_macos_sudo_tests.sh b/scripts/run_macos_sudo_tests.sh index 39df4d66..bbe9efa7 100755 --- a/scripts/run_macos_sudo_tests.sh +++ b/scripts/run_macos_sudo_tests.sh @@ -23,6 +23,12 @@ # 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 @@ -50,7 +56,12 @@ export HATCH_DATA_DIR="${HATCH_DATA_DIR:-/opt/hatch}" # 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. -export TMPDIR="${TMPDIR_OVERRIDE:-/private/tmp/openjd-tests}" +# +# 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 TEST_USER="${SUDO_USER:-$(id -un)}" SUDOERS_FILE=/etc/sudoers.d/openjd-cross-user-tests @@ -59,6 +70,9 @@ PYTHON_SHIM=/usr/local/bin/python # 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" @@ -66,7 +80,7 @@ PYTEST_ARGS=() while [[ "${1:-}" != "" ]]; do case $1 in -h|--help) - sed -n '4,26p' "$0" | sed 's/^# \{0,1\}//' + sed -n '4,31p' "$0" | sed 's/^# \{0,1\}//' exit 1 ;; --keep) KEEP="True" ;; @@ -97,7 +111,11 @@ cleanup() { 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 - sudo rm -rf "${TMPDIR}" || true + # 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 ---" } @@ -162,7 +180,12 @@ provision() { sudo dscacheutil -flushcache - sudo mkdir -p "${TMPDIR}" + if [[ -d "${TMPDIR}" ]]; then + echo "${TMPDIR} already exists; reusing it and leaving it in place" + else + sudo mkdir -p "${TMPDIR}" + TMPDIR_CREATED="True" + fi # 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 @@ -210,9 +233,18 @@ if ! hatch env create; then fi exit 1 fi -# The target user executes the venv python and reads test support files, so the -# venv and the workspace must be world-readable/traversable. -chmod -R o+rX "${HATCH_DATA_DIR}" "$(pwd)" +# 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 {...} From da69789e3cd95841aa0c6e97466d01a40d24293b Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:17:34 -0700 Subject: [PATCH 08/10] test: validate TEST_USER, split permission tests, cover sudo -i quoting Three review findings. 1. TEST_USER is interpolated into /etc/sudoers.d and comes from SUDO_USER, an inherited environment variable, so a caller controlled the content of that file. `visudo -cf` did not protect against it: the file is written before the check runs, so a rejected file still lands on disk, and a payload ending in '#' comments out the remainder and validates cleanly (confirmed: visudo exits 0 on "attacker ALL=(ALL) NOPASSWD: ALL\n# ..."). TEST_USER is now checked against ^[A-Za-z0-9._-]+$ and must resolve via `id -u` before it is used anywhere, which also guards the dseditgroup and chown calls that take it. 2. The class-level macOS skipif I added on TestMacOSShimInterpreter was swallowing the four _other_users_can_execute tests, whose own is_posix / is_windows markers had become unreachable. That function decides whether cross-user execution is possible at all and is plain permission-bit logic, so it was getting zero coverage on the Linux and Windows legs -- the opposite of the reasoning applied to TestSetsidShimBehavior. Moved to a POSIX-scoped TestOtherUsersCanExecute, which makes those per-test markers meaningful again. 3. Nothing exercised the shim through `sudo -i`. The shim contains ';', '(', ')', '[', ']' and '=', and sudo -i composes a login-shell command line, so quoting is the one link with real risk -- and every existing test bypassed it by passing an argv list to sys.executable or by asserting only the constructed list. Added test_shim_survives_sudo_login_shell_quoting, which self-sudos (target == current user, already permitted by the provisioning rule) and asserts both a zero exit and pid == pgid, since a mangled shim shows up as a failed launch rather than a wrong pgid. Verified by hand on macOS 26.5 that the real chain works. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- scripts/run_macos_sudo_tests.sh | 15 +++++ test/openjd/sessions_v0/test_subprocess.py | 78 ++++++++++++++++++++-- 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/scripts/run_macos_sudo_tests.sh b/scripts/run_macos_sudo_tests.sh index bbe9efa7..80716e74 100755 --- a/scripts/run_macos_sudo_tests.sh +++ b/scripts/run_macos_sudo_tests.sh @@ -63,7 +63,22 @@ export HATCH_DATA_DIR="${HATCH_DATA_DIR:-/opt/hatch}" # 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 diff --git a/test/openjd/sessions_v0/test_subprocess.py b/test/openjd/sessions_v0/test_subprocess.py index ca196014..a90e9940 100644 --- a/test/openjd/sessions_v0/test_subprocess.py +++ b/test/openjd/sessions_v0/test_subprocess.py @@ -1369,16 +1369,71 @@ def test_setsid_shim_creates_new_process_group(self) -> None: 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 + @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, and for the _other_users_can_execute() permission - check that backs it. + 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.""" + 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 @@ -1524,6 +1579,19 @@ def test_no_reachable_interpreter_reaches_the_operator_as_a_start_failure( "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 ae80d9181caf3c013538419f061b7af0007ba02c Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:43:32 -0700 Subject: [PATCH 09/10] fix: restore SIGPIPE/SIGXFSZ in the shim, harden TMPDIR creation Three review findings. 1. The shim leaked CPython's signal ignores into the workload. CPython sets SIGPIPE and SIGXFSZ to SIG_IGN during startup, and SIG_IGN survives exec (installed handlers do not), so every impersonated macOS workload ran with SIGPIPE ignored: `producer | head` would get EPIPE write errors instead of dying on the signal. Popen's restore_signals does not help because the process it starts is the shim, which re-ignores them, and os.execvp has no equivalent. Linux's setsid(1) is a C binary that never touches these, so this was a real cross-platform divergence. Both are now reset to SIG_DFL before the exec. Confirmed with a C probe reading sigaction directly: through the old shim a workload saw SIGPIPE=SIG_IGN, plain it sees SIG_DFL, and with the fix it sees SIG_DFL again. The blocked-signal mask is also inherited across exec but CPython leaves it empty, so no pthread_sigmask reset is needed. Pinned by test_shim_restores_default_signal_dispositions, which probes with perl: a Python process reports SIG_IGN for SIGPIPE regardless of what it inherited, and /bin/sh's `trap -p` prints nothing for an inherited ignore, so neither can distinguish the two cases. Verified the test reports IGNORE against the pre-fix shim, so it is a real guard rather than a tautology. 2. TMPDIR is a fixed path under world-writable, sticky /private/tmp, and the reuse branch applied `chown`/`chmod 1777` to whatever was already there. `mkdir -p` succeeds silently on an existing symlink-to-directory and both chown and chmod follow symlinks, so a local user could pre-create it as a symlink and redirect those calls (verified: chmod through a symlink changes the target's mode). Now created with plain `mkdir` so an existing path is a hard error telling the operator to inspect it and re-run with --cleanup-only. 3. Documented that _other_users_can_execute() only inspects the interpreter file: it says nothing about whether the target user can read that interpreter's stdlib or framework dylib, so an interpreter with o+x on the binary but o-rx on .../lib passes and then fails at runtime with "Fatal Python error: init_fs_encoding". A live `-I -c ""` probe as the target user would cover it but is not worth the per-launch cost; the docstring now says what the check does and does not guarantee. Also records the discovery-timing change the shim introduces, which the previous "same assumption already holds for Linux" comment glossed over: the workload's pgid only differs from sudo's once the shim interpreter has booted and reached setsid(), which is a full CPython startup rather than Linux's tiny C binary. Measured on macOS 26.5 (arm64) with /usr/bin/python3: ~120-180ms idle, ~165-190ms with every core saturated, against the 1s default in find_sudo_child_process_group_id -- comfortable but not enormous, and the comment names raising that timeout as the fix if the margin ever proves thin. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- scripts/run_macos_sudo_tests.sh | 19 ++++++--- src/openjd/sessions/_subprocess.py | 37 +++++++++++++++-- test/openjd/sessions_v0/test_subprocess.py | 47 ++++++++++++++++++++++ 3 files changed, 95 insertions(+), 8 deletions(-) diff --git a/scripts/run_macos_sudo_tests.sh b/scripts/run_macos_sudo_tests.sh index 80716e74..99b2c0d6 100755 --- a/scripts/run_macos_sudo_tests.sh +++ b/scripts/run_macos_sudo_tests.sh @@ -195,12 +195,21 @@ provision() { sudo dscacheutil -flushcache - if [[ -d "${TMPDIR}" ]]; then - echo "${TMPDIR} already exists; reusing it and leaving it in place" - else - sudo mkdir -p "${TMPDIR}" - TMPDIR_CREATED="True" + # 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." + echo " Inspect it, then re-run with --cleanup-only to remove it." + 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 diff --git a/src/openjd/sessions/_subprocess.py b/src/openjd/sessions/_subprocess.py index 0b986305..9d0929e6 100644 --- a/src/openjd/sessions/_subprocess.py +++ b/src/openjd/sessions/_subprocess.py @@ -52,13 +52,35 @@ # * `-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,sys;os.getpgrp()==os.getpid() or os.setsid();os.execvp(sys.argv[1],sys.argv[1:])" + "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" @@ -69,8 +91,17 @@ def _other_users_can_execute(path: str) -> bool: 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: it ignores group permissions and ACLs that might - also grant access, so it can return False for a path some specific user could execute. + 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 diff --git a/test/openjd/sessions_v0/test_subprocess.py b/test/openjd/sessions_v0/test_subprocess.py index a90e9940..11231ed2 100644 --- a/test/openjd/sessions_v0/test_subprocess.py +++ b/test/openjd/sessions_v0/test_subprocess.py @@ -1422,6 +1422,53 @@ def test_shim_survives_sudo_login_shell_quoting(self) -> None: 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: From 600aedce20e85a5e62bbf1be81afa80ac1a0d2c4 Mon Sep 17 00:00:00 2001 From: andychoquette <78888816+andychoquette@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:57:17 -0700 Subject: [PATCH 10/10] fix(scripts): make --cleanup-only actually remove a leftover temp root The hard error added in the previous commit told the user to re-run with --cleanup-only, but that path could not remove the directory: cleanup() only rm -rf's the temp root when TMPDIR_CREATED is "True", and the --cleanup-only branch set only PYTHON_SHIM_CREATED. So the documented recovery printed "Done", removed nothing, and the next run hit the same error, with no way forward that the message mentioned. --cleanup-only now re-establishes TMPDIR_CREATED from disk the same way it already does for the python alias: it claims a real directory (provision() only ever creates this fresh, so one found here is ours from an interrupted run) and refuses a symlink, which this script never creates and which would let the rm -rf be redirected at the target. A symlink gets a warning naming what to do instead. The hard error also distinguishes the two cases now, rather than pointing at --cleanup-only for a symlink it deliberately will not touch. Verified all three states: a leftover directory is claimed and removed, a symlink is refused with its target intact, and an absent path is a no-op. Signed-off-by: andychoquette <78888816+andychoquette@users.noreply.github.com> --- scripts/run_macos_sudo_tests.sh | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/scripts/run_macos_sudo_tests.sh b/scripts/run_macos_sudo_tests.sh index 99b2c0d6..7805b9d0 100755 --- a/scripts/run_macos_sudo_tests.sh +++ b/scripts/run_macos_sudo_tests.sh @@ -136,13 +136,25 @@ cleanup() { } if [[ "${CLEANUP_ONLY}" == "True" ]]; then - # --cleanup-only recovers from an interrupted run, where provision() never set - # PYTHON_SHIM_CREATED in this process. Only claim the alias if it points at the - # target we would have used; a pyenv/Homebrew symlink points somewhere else and - # is left alone. + # --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 @@ -205,8 +217,13 @@ provision() { 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." - echo " Inspect it, then re-run with --cleanup-only to remove it." + 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"