fix(sandbox): keep the GitHub token and registry password out of VM scripts

src/agent_env/artifact/artifacts/docker_image.py

@@ -14,7 +14,7 @@
 from dataclasses import dataclass
 from pathlib import Path
 from typing import Any, Callable, ClassVar, Literal
-from urllib.parse import urlparse
+from urllib.parse import quote, urlparse
 
 from pydantic import ConfigDict, Field, model_serializer
 
@@ -42,27 +42,17 @@ def _validate_git_name(value: str, label: str) -> str:
 
 
 
-def _git_clone_commands(owner: str, repo: str, ref: str | None, token: str | None) -> list[str]:
-    """Shell commands that clone ``owner/repo`` at ``ref`` into /tmp/repo. A token rides a
-    throwaway GIT_ASKPASS helper so it never appears on a command line, and the helper is
-    removed whether or not the clone succeeds; without a token the clone is anonymous and
-    fails fast on a private repository instead of prompting."""
+def _git_clone_command(owner: str, repo: str, ref: str | None, credentials: str | None) -> str:
+    """The command that clones ``owner/repo`` at ``ref`` into /tmp/repo, failing rather than prompting: signed in from
+    the ``credentials`` file when there is one, with git's other credential helpers off so none keeps a copy."""
     ref_flag = f"--branch {ref}" if ref else ""
-    clone_url = f"https://github.com/{owner}/{repo}.git"
-    if not token:
-        return [f"GIT_TERMINAL_PROMPT=0 git clone --depth 1 {ref_flag} {clone_url} /tmp/repo"]
-    askpass_script = (
-        '#!/bin/sh\n'
-        'case "$1" in\n'
-        '*sername*) echo x-access-token;;\n'
-        f'*) echo "{token}";;\n'
-        'esac\n'
-    )
-    return [
-        f"cat > /tmp/git-askpass.sh << 'ASKEOF'\n{askpass_script}ASKEOF",
-        "chmod +x /tmp/git-askpass.sh",
-        f"trap 'rm -f /tmp/git-askpass.sh' EXIT; GIT_ASKPASS=/tmp/git-askpass.sh git clone --depth 1 {ref_flag} {clone_url} /tmp/repo",
-    ]
+    helper = f"store --file={shlex.quote(credentials)}" if credentials else None
+    helpers = f"-c credential.helper= -c {shlex.quote(f'credential.helper={helper}')} " if helper else ""
+    return f"GIT_TERMINAL_PROMPT=0 git {helpers}clone --depth 1 {ref_flag} https://github.com/{owner}/{repo}.git /tmp/repo"
+
+
+def _github_credentials(token: str) -> bytes:
+    return f"https://x-access-token:{quote(token, safe='')}@github.com\n".encode()
 
 # How much of a build context's source digest its image's tag holds.
 _SOURCES_TAG_CHARS = 16
@@ -437,8 +427,12 @@ async def put_from_github(
             _validate_git_name(repo, "repo")
             if ref:
                 _validate_git_name(ref, "ref")
-            for command in _git_clone_commands(owner, repo, ref, github_token):
-                await sandbox.exec_script(command)
+            if github_token:
+                async with sandbox.secret_dir() as secrets:
+                    await sandbox.write_secret_file(_github_credentials(github_token), f"{secrets}/credentials")
+                    await sandbox.exec_script(_git_clone_command(owner, repo, ref, f"{secrets}/credentials"))
+            else:
+                await sandbox.exec_script(_git_clone_command(owner, repo, ref, None))
 
             commit_hash = (await sandbox.exec_script("git -C /tmp/repo rev-parse HEAD")).strip()
 
@@ -449,17 +443,10 @@ async def put_from_github(
             await sandbox.exec_script(f"docker build --platform linux/amd64 -f {dockerfile_abs} -t {shlex.quote(image_tag)} {context_abs}")
 
             log("push", "Pushing image...", 60)
-            login_cmd = ""
+            await sandbox.exec_script(f"docker tag {shlex.quote(image_tag)} {shlex.quote(image_ref)}")
             if auth is not None:
-                login_cmd = (
-                    f"echo {shlex.quote(auth.password)} | docker login "
-                    f"--username {shlex.quote(auth.username)} --password-stdin {shlex.quote(auth.registry)} && "
-                )
-            await sandbox.exec_script(
-                f"docker tag {shlex.quote(image_tag)} {shlex.quote(image_ref)} && "
-                f"{login_cmd}"
-                f"docker push {shlex.quote(image_ref)}"
-            )
+                await sandbox.docker_login(auth)
+            await sandbox.exec_script(f"docker push {shlex.quote(image_ref)}")
 
             log("save", "Saving Docker image...", 65)
             await sandbox.exec_script(f"docker save {shlex.quote(image_ref)} | gzip > /tmp/image.tar.gz")

src/agent_env/providers/sandbox_providers/local_docker/provider.py

@@ -26,7 +26,7 @@
 import tempfile
 from ipaddress import ip_address
 from pathlib import Path
-from typing import TYPE_CHECKING, Any, Optional
+from typing import TYPE_CHECKING, Any, AsyncIterator, Optional
 from urllib.parse import urlsplit, urlunsplit
 from uuid import uuid4
 
@@ -384,6 +384,27 @@ async def exec(self, *command: str) -> Any:
         Returns an object with .stdout, .stderr streams and .wait() method, matching the
         interface expected by exec_with_output().
         """
+        return await self._spawn(command, asyncio.subprocess.DEVNULL)
+
+    async def _exec_with_stdin(self, script: str, stdin: AsyncIterator[bytes]) -> tuple[int, str, str]:
+        """``bash -c script`` as ``exec`` runs it, its standard input the bytes ``stdin`` yields."""
+        process = await self._spawn(("bash", "-c", script), asyncio.subprocess.PIPE)
+
+        async def feed() -> None:
+            with contextlib.suppress(BrokenPipeError, ConnectionResetError):
+                async for piece in stdin:
+                    process.stdin.write(piece)
+                    await process.stdin.drain()
+            process.stdin.close()
+
+        try:
+            _, stdout, stderr = await asyncio.gather(feed(), process.stdout.read(), process.stderr.read())
+            return await process.wait(), stdout.decode(errors="replace"), stderr.decode(errors="replace")
+        except BaseException:
+            await _stop(process)
+            raise
+
+    async def _spawn(self, command: tuple[str, ...], stdin: int) -> asyncio.subprocess.Process:
         cmd = [c for c in command if c != "sudo"]
         if not _runs_in_container(cmd):
             is_script = cmd[:2] == ["bash", "-c"]
@@ -392,14 +413,13 @@ async def exec(self, *command: str) -> Any:
                 for i, c in enumerate(cmd)
             ]
         logger.debug(f"LocalSandbox exec: {' '.join(cmd)}")
-        process = await asyncio.create_subprocess_exec(
+        return await asyncio.create_subprocess_exec(
             *cmd,
-            stdin=asyncio.subprocess.DEVNULL,
+            stdin=stdin,
             stdout=asyncio.subprocess.PIPE,
             stderr=asyncio.subprocess.PIPE,
             env={**os.environ, _SANDBOX_ENV: self.sandbox_id},
         )
-        return process
 
     async def exec_with_output(self, *args: str) -> tuple[int, str, str]:
         """Like the base, but as a remote exec session behaves. Cancelling it kills the command and every process it

src/agent_env/providers/sandbox_providers/sandbox.py

@@ -23,7 +23,7 @@
 from typing import IO, TYPE_CHECKING, Any, AsyncIterator, Callable, ClassVar, Iterable, Optional
 
 from agent_env.config import get_config
-from agent_env.store.image_store.oci_registry_credentials import registry_host_from_ref
+from agent_env.store.image_store.oci_registry_credentials import RegistryAuth, registry_host_from_ref
 from agent_env.utils.docker.build import DEFAULT_BUILD_PLATFORM
 from agent_env.utils.paths import validate_relative_filename
 
@@ -432,10 +432,7 @@ async def pull_images(self, image_names: list[str]) -> None:
         for image_name in one_per_registry.values():
             auth = await asyncio.to_thread(store.auth, image_name)
             if auth is not None:
-                await self.exec_script(
-                    f"echo {shlex.quote(auth.password)} | docker login "
-                    f"--username {shlex.quote(auth.username)} --password-stdin {shlex.quote(auth.registry)}"
-                )
+                await self.docker_login(auth)
         pulls = [asyncio.ensure_future(_pull(self, image_name)) for image_name in image_names]
         try:
             await asyncio.gather(*pulls)
@@ -606,6 +603,41 @@ async def write_host_file(self, data: bytes, vm_path: str) -> None:
             await self.exec_script(f"mkdir -p {shlex.quote(parent)}")
         await self._write_bytes_to_vm_path(data, vm_path)
 
+    async def docker_login(self, auth: RegistryAuth) -> None:
+        """Sign the VM host's Docker in to ``auth``'s registry, the password read from a secret file."""
+        async with self.secret_dir() as secrets:
+            await self.write_secret_file(auth.password.encode(), f"{secrets}/password")
+            await self.exec_script(
+                f"docker login --username {shlex.quote(auth.username)} --password-stdin {shlex.quote(auth.registry)} "
+                f"< {shlex.quote(f'{secrets}/password')}"
+            )
+
+    @contextlib.asynccontextmanager
+    async def secret_dir(self) -> AsyncIterator[str]:
+        """A directory on the VM host only its owner can open, under a name no one can guess, removed on the way out."""
+        path = (await self.exec_script("mktemp -d")).strip()
+        try:
+            yield path
+        finally:
+            try:
+                await self.exec_script(f"rm -rf {shlex.quote(path)}")
+            except Exception as e:
+                logger.warning(f"Removing {path} failed (ignored): {e}")
+
+    async def write_secret_file(self, data: bytes, vm_path: str) -> None:
+        """Write a secret to ``vm_path`` on the VM host: over the exec's stdin when this sandbox takes it, else through
+        ``write_host_file``, which keeps it off the command line only where the provider writes files through an API."""
+        async def once() -> AsyncIterator[bytes]:
+            yield data
+
+        try:
+            exit_code, stdout, stderr = await self._exec_with_stdin(f"cat > {shlex.quote(vm_path)}", once())
+        except NotImplementedError:
+            await self.write_host_file(data, vm_path)
+            return
+        if exit_code != 0:
+            raise RuntimeError(f"Writing {vm_path} failed (exit {exit_code}): {(stderr or stdout)[-1500:]}")
+
     async def write_file_from_text(self, content: str, destination_path: str, container: str | None = None) -> None:
         vm_path = self._staging_path("wft", destination_path)
         try:

tst/unit/artifact/test_docker_image_github_clone.py

@@ -1,21 +1,38 @@
-"""put_from_github clones with a throwaway askpass helper when a token exists, anonymously otherwise."""
+"""put_from_github clones signed in from a credentials file when a token exists, anonymously otherwise."""
 
-from agent_env.artifact.artifacts.docker_image import _git_clone_commands
+import os
+import shlex
+import subprocess
 
+import pytest
 
-def test_token_rides_an_askpass_helper_removed_whether_or_not_the_clone_succeeds():
-    cmds = _git_clone_commands("example-org", "example-repo", "main", "ghs_secret")
-    assert len(cmds) == 3
-    assert "ghs_secret" in cmds[0] and "x-access-token" in cmds[0]
-    assert cmds[1] == "chmod +x /tmp/git-askpass.sh"
-    assert cmds[2].startswith("trap 'rm -f /tmp/git-askpass.sh' EXIT; ")
-    assert cmds[2].endswith("GIT_ASKPASS=/tmp/git-askpass.sh git clone --depth 1 --branch main https://github.com/example-org/example-repo.git /tmp/repo")
-    assert "ghs_secret" not in cmds[2]
+from agent_env.artifact.artifacts.docker_image import _git_clone_command, _github_credentials
+
+
+@pytest.mark.parametrize("folder", ["secrets", "secret files"])
+def test_a_token_signs_the_clone_in_from_its_credentials_file_and_no_other_helper_keeps_it(tmp_path, folder):
+    credentials = tmp_path / folder / "credentials"
+    credentials.parent.mkdir()
+    credentials.write_bytes(_github_credentials("ghs_s3cret"))
+    kept = tmp_path / "kept"
+    (tmp_path / ".gitconfig").write_text(f"[credential]\n\thelper = \"!f() {{ cat >> {kept}; }}; f\"\n")
+    command = _git_clone_command("example-org", "example-repo", "main", str(credentials))
+
+    words = shlex.split(command)
+    assert words[:2] == ["GIT_TERMINAL_PROMPT=0", "git"] and words[words.index("clone"):] == [
+        "clone", "--depth", "1", "--branch", "main", "https://github.com/example-org/example-repo.git", "/tmp/repo",
+    ]
+    options = words[words.index("git") + 1:words.index("clone")]
+    env = {**os.environ, "HOME": str(tmp_path), "GIT_CONFIG_NOSYSTEM": "1", "GIT_TERMINAL_PROMPT": "0"}
+    asked = "protocol=https\nhost=github.com\n\n"
+    filled = subprocess.run(["git", *options, "credential", "fill"], input=asked, env=env, capture_output=True,
+                            text=True, check=True)
+    subprocess.run(["git", *options, "credential", "approve"], input=filled.stdout, env=env, check=True, text=True)
+    assert "username=x-access-token\npassword=ghs_s3cret\n" in filled.stdout
+    assert not kept.exists()
 
 
 def test_without_a_token_the_clone_is_anonymous_and_never_prompts():
-    cmds = _git_clone_commands("docker-library", "hello-world", None, None)
-    assert len(cmds) == 1
-    assert cmds[0].startswith("GIT_TERMINAL_PROMPT=0 git clone --depth 1")
-    assert cmds[0].endswith("https://github.com/docker-library/hello-world.git /tmp/repo")
-    assert "askpass" not in cmds[0] and "--branch" not in cmds[0]
+    assert _git_clone_command("docker-library", "hello-world", None, None) == (
+        "GIT_TERMINAL_PROMPT=0 git clone --depth 1  https://github.com/docker-library/hello-world.git /tmp/repo"
+    )

tst/unit/artifact/test_docker_image_github_push.py

@@ -15,6 +15,7 @@
 from agent_env.artifact import DockerImageArtifact
 from agent_env.config import get_config
 from agent_env.providers.sandbox_providers.local_docker.provider import LocalSandboxProvider
+from agent_env.providers.sandbox_providers.sandbox import VmSandbox
 from agent_env.store import ImageStore, RegistryAuth, set_object_store
 from agent_env.store.object_store.local import LocalFilesystemObjectStore
 
@@ -56,14 +57,20 @@ def signed_put_url(self, object_url: str, expires_in: int = 3600) -> str:
         return f"https://put.example/{object_url.rsplit('/', 1)[-1]}"
 
 
-class _BuildVm:
+class _BuildVm(VmSandbox):
     files = {"/tmp/image.tar.gz": b"the saved image", "/tmp/build-context.tar.gz": b"the build context"}
 
     def __init__(self) -> None:
         self.scripts: list[str] = []
+        self.stdin: dict[str, bytes] = {}
+        self.fails: str | None = None
 
     async def exec_script(self, script: str, **kwargs) -> str:
         self.scripts.append(script)
+        if self.fails and script.startswith(self.fails):
+            raise RuntimeError("Script failed (exit 128)")
+        if script == "mktemp -d":
+            return "/tmp/tmp.Xq3\n"
         if script.startswith("wc -c < "):
             return f"{len(self.files[script.removeprefix('wc -c < ')])}\n"
         if read := re.fullmatch(r"tail -c \+(\d+) (\S+) \| head -c (\d+) \| base64", script):
@@ -73,6 +80,10 @@ async def exec_script(self, script: str, **kwargs) -> str:
             return "FROM scratch\nCOPY app /app\n"
         return "0123abcd\n" if "rev-parse" in script else ""
 
+    async def _exec_with_stdin(self, script, stdin):
+        self.stdin[script] = b"".join([piece async for piece in stdin])
+        return 0, "", ""
+
     async def terminate(self) -> None:
         pass
 
@@ -138,6 +149,41 @@ async def test_a_store_that_signs_nothing_gets_the_build_tarballs_copied_off_the
     assert not any("curl" in script for script in vm.scripts)
 
 
+@pytest.mark.asyncio
+async def test_the_github_token_and_registry_password_reach_the_vm_in_files_no_script_holds(build):
+    _, _, vm = build
+
+    await DockerImageArtifact.put_from_github("server", "https://github.com/example/repo/blob/main/Dockerfile",
+                                              github_token="ghs_s3cret")
+
+    assert vm.stdin == {
+        "cat > /tmp/tmp.Xq3/credentials": b"https://x-access-token:ghs_s3cret@github.com\n",
+        "cat > /tmp/tmp.Xq3/password": b"token",
+    }
+    clone = vm.scripts.index(
+        "GIT_TERMINAL_PROMPT=0 git -c credential.helper= -c 'credential.helper=store --file=/tmp/tmp.Xq3/credentials' "
+        "clone --depth 1 --branch main https://github.com/example/repo.git /tmp/repo"
+    )
+    login = vm.scripts.index(
+        "docker login --username user --password-stdin registry.example < /tmp/tmp.Xq3/password"
+    )
+    assert vm.scripts[clone + 1] == vm.scripts[login + 1] == "rm -rf /tmp/tmp.Xq3"
+    assert not any(secret in script for script in vm.scripts for secret in ("ghs_s3cret", "token"))
+
+
+@pytest.mark.asyncio
+async def test_the_token_is_removed_from_the_vm_when_the_clone_fails(build):
+    _, _, vm = build
+    vm.fails = "GIT_TERMINAL_PROMPT=0 git"
+
+    with pytest.raises(RuntimeError, match="exit 128"):
+        await DockerImageArtifact.put_from_github("server", "https://github.com/example/repo/blob/main/Dockerfile",
+                                                  github_token="ghs_s3cret")
+
+    *_, clone, removal = vm.scripts
+    assert clone.startswith(vm.fails) and removal == "rm -rf /tmp/tmp.Xq3"
+
+
 @pytest.mark.asyncio
 async def test_a_registry_on_this_machine_is_refused_before_a_remote_build_vm_is_created(build, monkeypatch):
     images, _, vm = build

tst/unit/providers/sandbox_providers/local_sandbox_test.py

@@ -1,9 +1,11 @@
 import asyncio
 import contextlib
+import logging
 import os
 import platform
 import shutil
 import signal
+import stat
 import subprocess
 import tempfile
 from pathlib import Path
@@ -848,6 +850,28 @@ async def test_exec_leaves_a_script_for_another_program_alone(spawned):
     assert spawned == [("docker", "exec", "c", "bash", "-c", "cat /app/x")]
 
 
+@pytest.mark.asyncio
+async def test_a_secret_reaches_a_folder_only_its_owner_opens_on_no_command_line_or_log(monkeypatch, caplog, tmp_path):
+    sandbox = LocalSandbox(work_dir=tmp_path)
+    argvs: list[tuple[str, ...]] = []
+    spawn = ls.asyncio.create_subprocess_exec
+
+    async def recording_spawn(*argv, **kwargs):
+        argvs.append(argv)
+        return await spawn(*argv, **kwargs)
+
+    monkeypatch.setattr(ls.asyncio, "create_subprocess_exec", recording_spawn)
+    caplog.set_level(logging.DEBUG, logger=ls.__name__)
+
+    async with sandbox.secret_dir() as secrets:
+        await sandbox.write_secret_file(b"ghs_s3cret", f"{secrets}/token")
+        assert Path(secrets, "token").read_bytes() == b"ghs_s3cret"
+        assert stat.S_IMODE(os.stat(secrets).st_mode) == 0o700
+
+    assert not Path(secrets).exists()
+    assert argvs and not any("ghs_s3cret" in arg for argv in argvs for arg in argv)
+    assert "LocalSandbox exec" in caplog.text and "ghs_s3cret" not in caplog.text
+
 
 @pytest.mark.asyncio
 @pytest.mark.parametrize("command", [