Skip to content

Commit 48d09e4

Browse files
jeremymanningclaude
andcommitted
Fix six defects found by the final red-team, four of them mine
**Single-venv jobs could never return a result (critical, introduced by me).** Result signing was added to the two-venv branch only, while the submitter recorded a signing key unconditionally -- so any job that fell back to the single-venv path (use_two_venv=False, or any two-venv setup failure or timeout) produced a result the caller then refused as unsigned. A degraded but working path became a hard failure. All four schedulers now sign on both branches, via one shared `result_signing_lines()` so a branch cannot forget again. Testing that on tensor01 uncovered three more, each hidden behind the last: * **The SSH fallback never built its venv.** Both of its fallback branches set `venv_info = None` and stopped; the SLURM path has always called `setup_remote_environment`. The generated script therefore activated a virtualenv nobody had created: "venv/bin/activate: No such file or directory". * **`python -m venv` assumed a `python` that does not exist.** Python 3 installs ship `python3`; `python` is only present where somebody added a compatibility symlink. `resolve_remote_python` now probes. * **The dill install was best-effort.** `|| echo 'Package installation failed, continuing...'` swallowed the failure of the one package the job script cannot work without, and the job died twenty lines into a remote traceback with "'NoneType' object is not callable", naming neither. Which finally exposed the real limit, which is not papered over: **the single-venv path cannot bridge Python versions at all.** dill embeds CPython bytecode, so a 3.12 payload on tensor01's `python3 (3.6.8)` gives "code() takes at most 15 arguments (20 given)". It now fails at submit time with something actionable: No python3.12 on the remote host, and dill payloads cannot cross Python minor versions. Found: python3 (3.6.8). Either install python3.12 there, set python_executable to a matching interpreter, or leave use_two_venv enabled so clustrix can build a conda environment at the right version. **Silent chmod failure -> key theft -> code execution on the submitting machine.** `execute_remote_command` never checked exit status, and job directories were `job_<unix_seconds>` -- fully predictable. On a world-writable remote_work_dir an attacker could pre-create the directory; `mkdir -p` succeeds on it, the unchecked `chmod 700` fails unnoticed, the signing key lands somewhere they can read, and they forge both result.pkl and its HMAC. Directories are now created exclusively with `mkdir -m 700` and the status checked, and their names carry four random bytes -- which also fixes two jobs submitted in the same second overwriting each other's key. **`1.5GB` produced `--mem=1.5G`**, which SLURM and PBS reject. Fractional sizes round up (down would get the job killed). Shell interpolations of config-derived paths are now `shlex.quote`d. The agent also attacked and could not break: `make_portable_function` (its source is a module constant), the HMAC's coverage of the downloaded bytes, `compare_digest`'s operands, cleanup ordering, and the dropdown reentrancy guard. All three backends re-verified afterwards: slurm PASSED t08.hpcc.dartmouth.edu python 3.12.13 gpu PASSED tensor01.dartmouth.edu python 3.12.13 hf PASSED j-contextlab-6a8401ce... python 3.12.14 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
1 parent 4346a10 commit 48d09e4

4 files changed

Lines changed: 189 additions & 128 deletions

File tree

clustrix/executor_connections.py

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -217,14 +217,29 @@ def _configure_kubectl_for_provisioned_cluster(self, cluster_info: Dict[str, Any
217217
logger.error(f"Failed to configure kubectl for provisioned cluster: {e}")
218218
raise
219219

220-
def execute_remote_command(self, command: str) -> tuple:
221-
"""Execute command on remote cluster."""
220+
def execute_remote_command(self, command: str, check: bool = False) -> tuple:
221+
"""Execute command on remote cluster.
222+
223+
``check`` raises when the command exits non-zero. It is off by default
224+
because most callers here inspect the output themselves and tolerate
225+
failure, but anything whose failure would be *unsafe* rather than
226+
merely unhelpful must opt in -- a `chmod 700` that quietly does nothing
227+
leaves a secret readable.
228+
"""
222229
if self.ssh_client is None:
223230
raise RuntimeError(
224231
"SSH client not connected. Call setup_ssh_connection() first."
225232
)
226233
stdin, stdout, stderr = self.ssh_client.exec_command(command)
227-
return stdout.read().decode(), stderr.read().decode()
234+
out = stdout.read().decode()
235+
err = stderr.read().decode()
236+
if check:
237+
status = stdout.channel.recv_exit_status()
238+
if status != 0:
239+
raise RuntimeError(
240+
f"Remote command failed (exit {status}): {command}\n{err.strip()}"
241+
)
242+
return out, err
228243

229244
def resolve_remote_path(self, path: str) -> str:
230245
"""Expand a leading ``~/`` against the remote account's home directory.

clustrix/executor_core.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
"""
66

77
import hashlib
8+
import shlex
89
import hmac
910
import time
1011
import tempfile
@@ -190,7 +191,7 @@ def _verify_result_signature(
190191

191192
try:
192193
stdout, _ = self.connection_manager.execute_remote_command(
193-
f"cat {remote_dir}/result.pkl.hmac 2>/dev/null"
194+
f"cat {shlex.quote(f'{remote_dir}/result.pkl.hmac')} 2>/dev/null"
194195
)
195196
except Exception as e: # pragma: no cover - defensive
196197
raise RuntimeError(

clustrix/executor_schedulers.py

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import os
88
import secrets
9+
import shlex
910
import time
1011
import tempfile
1112
import pickle
@@ -42,8 +43,18 @@ def _prepare_job_dir(self, remote_job_dir: str) -> str:
4243
read another user's command line out of `ps` -- which would hand the
4344
secret to exactly the people the 0700 directory is meant to exclude.
4445
"""
46+
# `mkdir -p` succeeds on a directory that already exists and is owned
47+
# by somebody else, and an unchecked `chmod` then fails silently. Job
48+
# directory names were fully predictable, so on a world-writable
49+
# remote_work_dir an attacker could pre-create the directory, receive
50+
# the signing key into it, and forge a result -- turning the defence
51+
# into a code-execution path on the *submitting* machine. Create it
52+
# exclusively, and refuse to continue if that fails.
4553
self.connection_manager.execute_remote_command(
46-
f"mkdir -p {remote_job_dir} && chmod 700 {remote_job_dir}"
54+
f"mkdir -p {shlex.quote(os.path.dirname(remote_job_dir))}", check=True
55+
)
56+
self.connection_manager.execute_remote_command(
57+
f"mkdir -m 700 {shlex.quote(remote_job_dir)}", check=True
4758
)
4859
key = secrets.token_hex(32)
4960
self.connection_manager.create_remote_file(
@@ -73,7 +84,7 @@ def submit_slurm_job(
7384
work_dir = self.connection_manager.resolve_remote_path(
7485
self.config.remote_work_dir
7586
)
76-
remote_job_dir = f"{work_dir}/job_{int(time.time())}"
87+
remote_job_dir = f"{work_dir}/job_{int(time.time())}_{secrets.token_hex(4)}"
7788
result_key = self._prepare_job_dir(remote_job_dir)
7889

7990
# Upload function data
@@ -198,7 +209,7 @@ def submit_pbs_job(
198209
work_dir = self.connection_manager.resolve_remote_path(
199210
self.config.remote_work_dir
200211
)
201-
remote_job_dir = f"{work_dir}/job_{int(time.time())}"
212+
remote_job_dir = f"{work_dir}/job_{int(time.time())}_{secrets.token_hex(4)}"
202213
result_key = self._prepare_job_dir(remote_job_dir)
203214

204215
# Upload function data
@@ -245,7 +256,7 @@ def submit_sge_job(
245256
work_dir = self.connection_manager.resolve_remote_path(
246257
self.config.remote_work_dir
247258
)
248-
remote_job_dir = f"{work_dir}/job_{int(time.time())}"
259+
remote_job_dir = f"{work_dir}/job_{int(time.time())}_{secrets.token_hex(4)}"
249260
result_key = self._prepare_job_dir(remote_job_dir)
250261

251262
# Upload function data
@@ -302,7 +313,7 @@ def submit_ssh_job(
302313
work_dir = self.connection_manager.resolve_remote_path(
303314
self.config.remote_work_dir
304315
)
305-
remote_job_dir = f"{work_dir}/job_{int(time.time())}"
316+
remote_job_dir = f"{work_dir}/job_{int(time.time())}_{secrets.token_hex(4)}"
306317
result_key = self._prepare_job_dir(remote_job_dir)
307318

308319
# Upload function data
@@ -368,10 +379,27 @@ def setup_venv():
368379

369380
except Exception as e:
370381
logger.warning(f"Failed to setup two-venv environment: {e}")
371-
# Fall back to original approach
382+
# Fall back to the single-venv approach -- which means actually
383+
# building that venv. Both fallback branches used to set
384+
# venv_info = None and stop there, so the generated script
385+
# activated a virtualenv nobody had created and every job died
386+
# with "venv/bin/activate: No such file or directory". The
387+
# SLURM path has always called this; the SSH path never did.
388+
setup_remote_environment(
389+
self.connection_manager.ssh_client,
390+
remote_job_dir,
391+
func_data["requirements"],
392+
self.config,
393+
)
372394
updated_config.venv_info = None
373395
else:
374396
logger.info("Two-venv setup disabled, using basic environment setup")
397+
setup_remote_environment(
398+
self.connection_manager.ssh_client,
399+
remote_job_dir,
400+
func_data["requirements"],
401+
self.config,
402+
)
375403
updated_config.venv_info = None
376404

377405
# Create execution script

0 commit comments

Comments
 (0)