From c51e946e65a189357cc94400f0c4fd283f55bbaa Mon Sep 17 00:00:00 2001 From: Paulo Date: Sun, 6 Sep 2026 17:09:52 +0200 Subject: [PATCH] Remove a box's secrets at teardown, and finish the sbx implementation Docker Sandboxes keeps a per-sandbox secret after the sandbox is removed, so a host that goes must take its secrets with it. Host deletion now calls delete_secrets for the box before the VM goes. A provider failure there keeps the row for a retry, like a VM failure does. The seam gets the box alone. Teardown never reads the host row, so a row whose secrets no longer decrypt still goes. On docker-sbx, delete_secrets removes the github secret by service name and every custom secret by the placeholder sbx lists for it, since the CLI has no JSON and no way to clear a scope. Removal can run again after a partial teardown: sbx answers a missing secret with success. The value files go with the secrets. sbx's own github secret now applies only to the catalog's github service. A custom entry named github with a host of its own is a custom secret on that host, so the value reaches that host and never public GitHub. The deploy doc says which sbx secret each service is, that deletion removes the sandbox's scope, and that no global sbx secret may name a destination drukbox manages, since sbx applies the global one first. --- docs/architecture.md | 4 +- docs/deploy.md | 10 ++- src/hosts/service.py | 5 +- src/hosts/tests/test_secrets.py | 68 ++++++++++++++++++- src/providers/capabilities.py | 7 +- src/providers/docker_sbx/api.py | 15 ++++ src/providers/docker_sbx/secrets.py | 25 ++++--- src/providers/docker_sbx/tests/test_api.py | 38 +++++++++++ .../docker_sbx/tests/test_secrets.py | 61 ++++++++++++----- src/providers/tests/test_capabilities.py | 4 +- 10 files changed, 196 insertions(+), 41 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 38a06e4..b1a3865 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -146,7 +146,9 @@ the proxy. Every connection goes to the address the proxy checked, and the upstream certificate is checked against the CONNECT host. A request whose `Host` differs from the CONNECT host is refused. On docker-sbx the sandbox gets only the placeholder. Drukbox puts the value in sbx's own secret store for that sandbox, and sbx's proxy swaps -the placeholder on the way out. +the placeholder on the way out. Host deletion calls `delete_secrets` for the +box before the VM goes, so nothing the seam put anywhere outlives the box. It +never reads the row's secrets, so a lost key cannot block a teardown. A template is a persistent provider image keyed by provider, base image, and setup-script hash. `POST /templates` creates a `building` record and diff --git a/docs/deploy.md b/docs/deploy.md index e757f8b..4ff00b4 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -364,8 +364,14 @@ from the sandbox tag to the proxy host: A sandbox then reaches that port on that host and nothing else there. A Docker Sandboxes sandbox dials nothing. Drukbox puts each value in sbx's own secret store for that sandbox, and sbx's proxy swaps the placeholder. The -value files that sbx reads live in a `secrets` directory under -`DOCKER_SBX_WORKSPACE_ROOT`, beside the workspaces and never inside one. +`github` service is sbx's own `github` secret, which covers git and gh. Any +other service, and a custom entry that names a host of its own, is a custom +secret on its hosts. The value files that sbx reads live in a `secrets` +directory under `DOCKER_SBX_WORKSPACE_ROOT`, beside the workspaces and never +inside one. sbx keeps a sandbox's secrets after the sandbox is removed, so +host deletion removes every secret in the sandbox's scope and the files. Do +not set a global sbx secret for a destination drukbox manages. sbx applies +the global one first, and drukbox's value never reaches the sandbox. Give secrets to `POST /hosts`. Provisioning delivers the placeholders in the sandbox's boot environment, on every provider, the same way as `env`. A diff --git a/src/hosts/service.py b/src/hosts/service.py index 30602f0..fd79e78 100644 --- a/src/hosts/service.py +++ b/src/hosts/service.py @@ -461,8 +461,11 @@ async def delete_host( host.tailscale_device_id = None host.updated_at = utc_now() await self.session.commit() + # Secrets go before the VM. A provider failure keeps the row for a retry. + vm = get_vm_provider(host.provider) + await vm.secrets.delete_secrets(vm=host.name) try: - await get_vm_provider(host.provider).delete_vm(host.name) + await vm.delete_vm(host.name) except ProviderNotFoundError: # VM already absent at the provider — exe.dev may have evicted # it, or a previous delete partially succeeded. Treat as done diff --git a/src/hosts/tests/test_secrets.py b/src/hosts/tests/test_secrets.py index 35312bc..8f1ed3d 100644 --- a/src/hosts/tests/test_secrets.py +++ b/src/hosts/tests/test_secrets.py @@ -5,6 +5,7 @@ import pytest import respx from sqlalchemy import select, text +from sqlalchemy_encrypted_field import SecretDecryptError from core.database import async_session_factory from core.settings import Settings, get_settings @@ -16,6 +17,7 @@ from hosts.tests.conftest import StubVMProvider from providers.base import VMCreateResult from providers.capabilities import SecretInjectionCapability +from providers.exceptions import ProviderTransportError ISSUER = {"url": "https://mint.test/box/github", "headers": {"X-Key": "k"}, "refresh": "1h"} SECRETS = { @@ -37,6 +39,7 @@ class RecordingInjection(SecretInjectionCapability): def __init__(self) -> None: self.values: dict[str, str] = {} + self.deleted: list[str] = [] async def put_secret( self, *, vm: str, service: Service, placeholder: Placeholder, value: str @@ -44,8 +47,8 @@ async def put_secret( self.values[placeholder.service] = value return {service.auth_variable: str(placeholder)} - async def delete_secret(self, *, vm: str, placeholder: Placeholder) -> None: - return + async def delete_secrets(self, *, vm: str) -> None: + self.deleted.append(vm) @pytest.fixture @@ -220,6 +223,67 @@ async def test_a_wrong_issuer_answer_at_boot_never_reaches_the_log_or_the_host( assert "secret-xyz" not in str(failure.value) +async def test_teardown_removes_the_secrets_before_the_vm( + settings: Settings, create_vm: AsyncMock, stub_provider: StubVMProvider +) -> None: + recording = RecordingInjection() + stub_provider.secrets = recording + async with async_session_factory() as session: + host = await HostService(session, settings=settings).create_host( + env={}, secrets={"anthropic": {"value": "sk-ant-real"}}, image=None, provider="stub" + ) + + async with async_session_factory() as session: + assert await HostService(session, settings=settings).delete_host(host.id) + + assert recording.deleted == [host.name] + assert stub_provider.deleted == [host.name] + + +async def test_a_secret_that_cannot_be_removed_keeps_the_host_for_a_retry( + settings: Settings, create_vm: AsyncMock, stub_provider: StubVMProvider +) -> None: + recording = RecordingInjection() + stub_provider.secrets = recording + async with async_session_factory() as session: + host = await HostService(session, settings=settings).create_host( + env={}, secrets={"anthropic": {"value": "sk-ant-real"}}, image=None, provider="stub" + ) + recording.delete_secrets = AsyncMock(side_effect=ProviderTransportError("sbx is down")) + + async with async_session_factory() as session: + service = HostService(session, settings=settings) + with pytest.raises(ProviderTransportError): + await service.delete_host(host.id) + assert await service.get_host(host.id) + assert stub_provider.deleted == [] + + +async def test_teardown_needs_no_secrets_key( + settings: Settings, + create_vm: AsyncMock, + stub_provider: StubVMProvider, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A row whose secrets no longer decrypt still goes, VM and all.""" + stub_provider.secrets = RecordingInjection() + async with async_session_factory() as session: + host = await HostService(session, settings=settings).create_host( + env={}, secrets={"anthropic": {"value": "sk-ant-real"}}, image=None, provider="stub" + ) + monkeypatch.setenv("SECRETS_KEY", "MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTE=") + get_settings.cache_clear() + + async with async_session_factory() as session: + service = HostService(session, settings=settings) + stored = await service.get_host(host.id) + assert stored + with pytest.raises(SecretDecryptError): + dict(stored.secrets) + assert await service.delete_host(host.id) + assert stub_provider.deleted == [host.name] + + async def test_secrets_on_a_provider_that_holds_the_value_need_no_proxy( client, monkeypatch: pytest.MonkeyPatch, stub_provider: StubVMProvider ) -> None: diff --git a/src/providers/capabilities.py b/src/providers/capabilities.py index 4eb5dc4..42ae2eb 100644 --- a/src/providers/capabilities.py +++ b/src/providers/capabilities.py @@ -30,7 +30,8 @@ def resolve_capability(provider, capability: type[CapabilityT]) -> CapabilityT: class SecretInjectionCapability(abc.ABC): """How a secret reaches one provider's boxes. ``put_secret`` returns the - environment the box needs.""" + environment the box needs. ``delete_secrets`` gets the box alone, so + teardown never reads the host row.""" # sbx keeps the value in its own store. The proxy needs only the placeholder. needs_value: ClassVar[bool] @@ -46,7 +47,7 @@ async def put_secret( ) -> dict[str, str]: ... @abc.abstractmethod - async def delete_secret(self, *, vm: str, placeholder: Placeholder) -> None: ... + async def delete_secrets(self, *, vm: str) -> None: ... class ProxyInjection(SecretInjectionCapability): @@ -76,7 +77,7 @@ async def put_secret( "NODE_EXTRA_CA_CERTS": environment.PROXY_CA_PATH, } - async def delete_secret(self, *, vm: str, placeholder: Placeholder) -> None: + async def delete_secrets(self, *, vm: str) -> None: return def get_public_certificate(self) -> bytes: diff --git a/src/providers/docker_sbx/api.py b/src/providers/docker_sbx/api.py index b15bd60..a29b6bc 100644 --- a/src/providers/docker_sbx/api.py +++ b/src/providers/docker_sbx/api.py @@ -86,6 +86,21 @@ async def set_custom_secret( command, ) + async def custom_placeholders(self, *, sandbox: str) -> list[str]: + """``sbx secret ls`` has no JSON. The custom rows follow a ``CUSTOM + SECRETS`` header, with the placeholder in the fourth column.""" + output = await self._run("secret", "ls", "--sandbox", sandbox) + placeholders: list[str] = [] + custom = False + for line in output.splitlines(): + if line.startswith("CUSTOM SECRETS"): + custom = True + continue + columns = re.split(r"\s{2,}", line.strip()) + if custom and len(columns) >= 4 and columns[0] == sandbox: + placeholders.append(columns[3]) + return placeholders + async def remove_secret(self, service: str, *, sandbox: str) -> None: # Without -f the CLI waits for a confirmation. await self._run("secret", "rm", "-f", service, "--sandbox", sandbox) diff --git a/src/providers/docker_sbx/secrets.py b/src/providers/docker_sbx/secrets.py index 045e5b8..756cd8b 100644 --- a/src/providers/docker_sbx/secrets.py +++ b/src/providers/docker_sbx/secrets.py @@ -1,7 +1,8 @@ import shlex +import shutil from pathlib import Path -from host_secrets.catalog import Service +from host_secrets.catalog import CATALOG, Service from host_secrets.placeholder import Placeholder from providers.capabilities import SecretInjectionCapability from providers.exceptions import ProviderTransportError @@ -9,9 +10,9 @@ from .api import SbxCLI from .exceptions import DockerSbxProviderError -# sbx's own secret for these covers git and gh. Every other service is a -# custom secret on its host. -_NATIVE_SERVICES = frozenset({"github"}) +# sbx's own secret for these covers git and gh, when the entry reaches the +# service itself. Every other entry is a custom secret on its hosts. +_NATIVE_SERVICES = {"github": CATALOG["github"]} class SbxInjection(SecretInjectionCapability): @@ -41,7 +42,7 @@ async def put_secret( path.write_text(value) command = f"cat {shlex.quote(str(path))}" try: - if placeholder.service in _NATIVE_SERVICES: + if _NATIVE_SERVICES.get(placeholder.service) == service: await self.api.set_secret(placeholder.service, sandbox=vm, command=command) else: await self.api.set_custom_secret( @@ -55,15 +56,17 @@ async def put_secret( raise ProviderTransportError(str(exc)) from exc return {service.auth_variable: str(placeholder)} - async def delete_secret(self, *, vm: str, placeholder: Placeholder) -> None: + async def delete_secrets(self, *, vm: str) -> None: + """sbx keeps a sandbox's secrets after the sandbox is removed, and + answers a missing one with success, so this can run again.""" try: - if placeholder.service in _NATIVE_SERVICES: - await self.api.remove_secret(placeholder.service, sandbox=vm) - else: - await self.api.remove_custom_secret(sandbox=vm, placeholder=str(placeholder)) + for name in _NATIVE_SERVICES: + await self.api.remove_secret(name, sandbox=vm) + for placeholder in await self.api.custom_placeholders(sandbox=vm): + await self.api.remove_custom_secret(sandbox=vm, placeholder=placeholder) except DockerSbxProviderError as exc: raise ProviderTransportError(str(exc)) from exc - self.value_path(vm, placeholder.service).unlink() + shutil.rmtree(self.secrets_root / vm, ignore_errors=True) def value_path(self, vm: str, service: str) -> Path: return self.secrets_root / vm / service diff --git a/src/providers/docker_sbx/tests/test_api.py b/src/providers/docker_sbx/tests/test_api.py index 938ab2a..fa438b4 100644 --- a/src/providers/docker_sbx/tests/test_api.py +++ b/src/providers/docker_sbx/tests/test_api.py @@ -106,6 +106,44 @@ async def fake_exec(*args, **kwargs): ) +LISTING = """SCOPE TYPE NAME SECRET +sb-test service github (stored) + +CUSTOM SECRETS +SCOPE TARGETS ENV PLACEHOLDER SECRET +sb-test api.anthropic.com ANTHROPIC_AUTH_TOKEN drk.0123.anthropic.abc cmd (on-demand) +sb-test api.acme.test, acme.example ACME_TOKEN drk.0123.acme.def cmd (on-demand) +""" + + +@pytest.mark.asyncio +async def test_custom_placeholders_reads_the_placeholder_of_each_custom_row( + monkeypatch, +): + captured: dict = {} + + async def fake_exec(*args, **kwargs): + captured["args"] = args + return _process(stdout=LISTING.encode()) + + monkeypatch.setattr("providers.docker_sbx.api.asyncio.create_subprocess_exec", fake_exec) + + placeholders = await SbxCLI().custom_placeholders(sandbox="sb-test") + + assert captured["args"] == ("sbx", "secret", "ls", "--sandbox", "sb-test") + assert placeholders == ["drk.0123.anthropic.abc", "drk.0123.acme.def"] + + +@pytest.mark.asyncio +async def test_custom_placeholders_of_an_empty_scope(monkeypatch): + async def fake_exec(*args, **kwargs): + return _process(stdout=b'No secrets found for scope "sb-test".\n') + + monkeypatch.setattr("providers.docker_sbx.api.asyncio.create_subprocess_exec", fake_exec) + + assert await SbxCLI().custom_placeholders(sandbox="sb-test") == [] + + @pytest.mark.asyncio async def test_sandbox_count_reads_the_listing(monkeypatch): listing = b'{"sandboxes": [{"name": "sb-a"}, {"name": "sb-b"}]}' diff --git a/src/providers/docker_sbx/tests/test_secrets.py b/src/providers/docker_sbx/tests/test_secrets.py index 0c2692a..fdb7ec1 100644 --- a/src/providers/docker_sbx/tests/test_secrets.py +++ b/src/providers/docker_sbx/tests/test_secrets.py @@ -5,7 +5,7 @@ import pytest -from host_secrets.catalog import CATALOG +from host_secrets.catalog import CATALOG, Service, Upstream from host_secrets.placeholder import Placeholder from providers.docker_sbx.exceptions import DockerSbxTransportError from providers.docker_sbx.secrets import SbxInjection @@ -18,6 +18,7 @@ def _api_mock() -> MagicMock: api.set_custom_secret = AsyncMock() api.remove_secret = AsyncMock() api.remove_custom_secret = AsyncMock() + api.custom_placeholders = AsyncMock(return_value=[]) return api @@ -76,35 +77,58 @@ async def test_a_new_value_replaces_the_file_and_the_secret(tmp_path: Path) -> N assert api.set_custom_secret.await_count == 2 -async def test_delete_removes_a_custom_secret_by_placeholder_and_its_file(tmp_path: Path) -> None: +async def test_a_custom_entry_named_github_is_a_custom_secret_on_its_host(tmp_path: Path) -> None: api = _api_mock() - injection = SbxInjection(api, tmp_path) - placeholder = Placeholder.mint(uuid.uuid4(), "anthropic") - await injection.put_secret( - vm="sb-one", service=CATALOG["anthropic"], placeholder=placeholder, value="sk-ant-real" - ) + placeholder = Placeholder.mint(uuid.uuid4(), "github") + enterprise = Service("GH_ENTERPRISE_TOKEN", (Upstream("github.enterprise.example"),)) - await injection.delete_secret(vm="sb-one", placeholder=placeholder) + environment = await SbxInjection(api, tmp_path).put_secret( + vm="sb-one", service=enterprise, placeholder=placeholder, value="ghe_real" + ) - api.remove_custom_secret.assert_awaited_once_with( - sandbox="sb-one", placeholder=str(placeholder) + api.set_custom_secret.assert_awaited_once_with( + sandbox="sb-one", + hosts=["github.enterprise.example"], + env="GH_ENTERPRISE_TOKEN", + placeholder=str(placeholder), + command=f"cat {tmp_path / 'sb-one' / 'github'}", ) - assert not (tmp_path / "sb-one" / "anthropic").exists() + api.set_secret.assert_not_awaited() + assert environment == {"GH_ENTERPRISE_TOKEN": str(placeholder)} -async def test_delete_removes_a_github_secret_by_service_name_and_its_file(tmp_path: Path) -> None: +async def test_delete_secrets_removes_the_scope_and_the_files(tmp_path: Path) -> None: api = _api_mock() injection = SbxInjection(api, tmp_path) - placeholder = Placeholder.mint(uuid.uuid4(), "github") + anthropic = Placeholder.mint(uuid.uuid4(), "anthropic") + github = Placeholder.mint(uuid.uuid4(), "github") await injection.put_secret( - vm="sb-one", service=CATALOG["github"], placeholder=placeholder, value="ghs_real" + vm="sb-one", service=CATALOG["anthropic"], placeholder=anthropic, value="sk-ant-real" ) + await injection.put_secret( + vm="sb-one", service=CATALOG["github"], placeholder=github, value="ghs_real" + ) + api.custom_placeholders.return_value = [str(anthropic), "drk.x.other.y"] + + await injection.delete_secrets(vm="sb-one") + + api.remove_secret.assert_awaited_once_with("github", sandbox="sb-one") + api.custom_placeholders.assert_awaited_once_with(sandbox="sb-one") + assert [call.kwargs for call in api.remove_custom_secret.await_args_list] == [ + {"sandbox": "sb-one", "placeholder": str(anthropic)}, + {"sandbox": "sb-one", "placeholder": "drk.x.other.y"}, + ] + assert not (tmp_path / "sb-one").exists() + + +async def test_delete_secrets_can_run_again_after_a_partial_teardown(tmp_path: Path) -> None: + api = _api_mock() - await injection.delete_secret(vm="sb-one", placeholder=placeholder) + await SbxInjection(api, tmp_path).delete_secrets(vm="sb-one") api.remove_secret.assert_awaited_once_with("github", sandbox="sb-one") api.remove_custom_secret.assert_not_awaited() - assert not (tmp_path / "sb-one" / "github").exists() + assert not (tmp_path / "sb-one").exists() @pytest.mark.parametrize("service", ["github", "anthropic"]) @@ -113,7 +137,6 @@ async def test_a_cli_failure_is_a_provider_error(tmp_path: Path, service: str) - api.set_secret.side_effect = DockerSbxTransportError("daemon unavailable") api.set_custom_secret.side_effect = DockerSbxTransportError("daemon unavailable") api.remove_secret.side_effect = DockerSbxTransportError("daemon unavailable") - api.remove_custom_secret.side_effect = DockerSbxTransportError("daemon unavailable") injection = SbxInjection(api, tmp_path) placeholder = Placeholder.mint(uuid.uuid4(), service) @@ -122,4 +145,6 @@ async def test_a_cli_failure_is_a_provider_error(tmp_path: Path, service: str) - vm="sb-one", service=CATALOG[service], placeholder=placeholder, value="v" ) with pytest.raises(ProviderTransportError, match="daemon unavailable"): - await injection.delete_secret(vm="sb-one", placeholder=placeholder) + await injection.delete_secrets(vm="sb-one") + # The value file stays for the retry. + assert (tmp_path / "sb-one" / service).exists() diff --git a/src/providers/tests/test_capabilities.py b/src/providers/tests/test_capabilities.py index 810f713..547e226 100644 --- a/src/providers/tests/test_capabilities.py +++ b/src/providers/tests/test_capabilities.py @@ -143,9 +143,7 @@ async def test_the_proxy_injection_hands_out_a_public_certificate_and_nothing_el async def test_the_proxy_injection_holds_no_value_and_has_nothing_to_delete() -> None: assert ProxyInjection.needs_value is False - await ProxyInjection().delete_secret( - vm="sb-one", placeholder=Placeholder.mint(uuid.uuid4(), "anthropic") - ) + await ProxyInjection().delete_secrets(vm="sb-one") @pytest.mark.parametrize(