diff --git a/src/agent_env/artifact/artifacts/docker_image.py b/src/agent_env/artifact/artifacts/docker_image.py index 0f921d59..34541423 100644 --- a/src/agent_env/artifact/artifacts/docker_image.py +++ b/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") diff --git a/src/agent_env/providers/sandbox_providers/local_docker/provider.py b/src/agent_env/providers/sandbox_providers/local_docker/provider.py index 13ca76d9..a6c1dd74 100644 --- a/src/agent_env/providers/sandbox_providers/local_docker/provider.py +++ b/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 diff --git a/src/agent_env/providers/sandbox_providers/sandbox.py b/src/agent_env/providers/sandbox_providers/sandbox.py index 3a37b372..642bfe4d 100644 --- a/src/agent_env/providers/sandbox_providers/sandbox.py +++ b/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: diff --git a/tst/unit/artifact/test_docker_image_github_clone.py b/tst/unit/artifact/test_docker_image_github_clone.py index 9eaaefbc..eb4b3ea7 100644 --- a/tst/unit/artifact/test_docker_image_github_clone.py +++ b/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" + ) diff --git a/tst/unit/artifact/test_docker_image_github_push.py b/tst/unit/artifact/test_docker_image_github_push.py index fc4d170a..cdede6a7 100644 --- a/tst/unit/artifact/test_docker_image_github_push.py +++ b/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 diff --git a/tst/unit/providers/sandbox_providers/local_sandbox_test.py b/tst/unit/providers/sandbox_providers/local_sandbox_test.py index 4c257e44..45d94b92 100644 --- a/tst/unit/providers/sandbox_providers/local_sandbox_test.py +++ b/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", [ diff --git a/tst/unit/providers/sandbox_providers/vm_sandbox_test.py b/tst/unit/providers/sandbox_providers/vm_sandbox_test.py index 7d9fd10b..a8177780 100644 --- a/tst/unit/providers/sandbox_providers/vm_sandbox_test.py +++ b/tst/unit/providers/sandbox_providers/vm_sandbox_test.py @@ -171,15 +171,59 @@ def registry(): PUBLIC = "ghcr.io/team/tool:v1" +class _StdinVm(_RecordingVmSandbox): + """Takes stdin on exec, as the local and Modal VM sandboxes do.""" + + def __init__(self): + super().__init__() + self.stdin: dict[str, bytes] = {} + + async def exec_with_output(self, *args): + if args[:4] == ("sudo", "bash", "-c", "mktemp -d"): + self.scripts.append("mktemp -d") + return 0, "/tmp/tmp.Xq3\n", "" + return await super().exec_with_output(*args) + + async def _exec_with_stdin(self, script, stdin): + self.stdin[script] = b"".join([piece async for piece in stdin]) + return 0, "", "" + + @pytest.mark.asyncio async def test_an_image_with_no_tarball_is_pulled_after_logging_in_to_the_registry_the_store_holds(registry): - sandbox = _RecordingVmSandbox() + sandbox = _StdinVm() await sandbox.load_docker_images([_image(None, PRIVATE), _image(None, PUBLIC), _image(None, PRIVATE)]) - login, *pulls = sandbox.scripts - assert login == "echo token | docker login --username user --password-stdin registry.example" - assert sorted(pulls) == sorted([f"docker pull {PRIVATE}", f"docker pull {PUBLIC}"]) + *login, pull, other_pull = sandbox.scripts + assert login == [ + "mktemp -d", + "docker login --username user --password-stdin registry.example < /tmp/tmp.Xq3/password", + "rm -rf /tmp/tmp.Xq3", + ] + assert sandbox.stdin == {"cat > /tmp/tmp.Xq3/password": b"token"} + assert sorted([pull, other_pull]) == sorted([f"docker pull {PRIVATE}", f"docker pull {PUBLIC}"]) + + +class _FileApiVm(_RecordingVmSandbox): + """Writes VM files through its own API, as sail_vm, daytona, tensorlake and vercel do.""" + + def __init__(self): + super().__init__() + self.written: dict[str, bytes] = {} + + async def _write_bytes_to_vm_path(self, data, vm_path): + self.written[vm_path] = data + + +@pytest.mark.asyncio +async def test_a_secret_goes_through_the_file_api_of_a_sandbox_that_takes_no_stdin(): + sandbox = _FileApiVm() + + await sandbox.write_secret_file(b"token", "/tmp/tmp.Xq3/password") + + assert sandbox.written == {"/tmp/tmp.Xq3/password": b"token"} + assert not any("token" in script for script in sandbox.scripts) @pytest.mark.asyncio