From 95cbd0e1c87abf1200d09060d1b10c0a608df225 Mon Sep 17 00:00:00 2001 From: codex Date: Mon, 27 Jul 2026 03:55:04 +0000 Subject: [PATCH] fix: enforce cleanup and secret integrity --- bot_bottle/backend/base.py | 4 ++ bot_bottle/backend/cleanup_control.py | 48 +++++++++++++++++++ .../backend/docker/bottle_cleanup_plan.py | 16 +++++++ bot_bottle/backend/docker/cleanup.py | 32 +++++-------- .../firecracker/bottle_cleanup_plan.py | 8 ++++ bot_bottle/backend/firecracker/cleanup.py | 6 ++- .../macos_container/bottle_cleanup_plan.py | 8 ++++ bot_bottle/backend/macos_container/cleanup.py | 15 +++--- bot_bottle/cli/commands/cleanup.py | 17 +++++-- bot_bottle/gateway/git_gate/http_backend.py | 21 ++++---- bot_bottle/orchestrator/service.py | 8 +--- .../orchestrator/store/registry_store.py | 4 ++ bot_bottle/orchestrator/store/secret_store.py | 37 ++------------ tests/unit/test_cli_cleanup_cross_backend.py | 8 +++- tests/unit/test_firecracker_cleanup.py | 6 +-- tests/unit/test_git_http_backend.py | 13 +++++ tests/unit/test_macos_container_cleanup.py | 18 ++++++- .../unit/test_orchestrator_registry_store.py | 11 +++++ tests/unit/test_orchestrator_secret_store.py | 27 ++++++----- 19 files changed, 206 insertions(+), 101 deletions(-) create mode 100644 bot_bottle/backend/cleanup_control.py diff --git a/bot_bottle/backend/base.py b/bot_bottle/backend/base.py index 73fa744d..1216a1a7 100644 --- a/bot_bottle/backend/base.py +++ b/bot_bottle/backend/base.py @@ -172,6 +172,10 @@ class BottleCleanupPlan(ABC): """True iff there is nothing to clean up; the CLI uses this to short-circuit before showing the y/N.""" + @abstractmethod + def intersect(self, current: "BottleCleanupPlan") -> "BottleCleanupPlan": + """Resources both displayed to the operator and currently removable.""" + @dataclass(frozen=True) class ExecResult: diff --git a/bot_bottle/backend/cleanup_control.py b/bot_bottle/backend/cleanup_control.py new file mode 100644 index 00000000..b82efd6f --- /dev/null +++ b/bot_bottle/backend/cleanup_control.py @@ -0,0 +1,48 @@ +"""Shared destructive-cleanup execution and failure accounting.""" + +from __future__ import annotations + +import shutil +import subprocess +from collections.abc import Sequence +from pathlib import Path + + +class CleanupError(RuntimeError): + """One or more approved cleanup mutations did not complete.""" + + +class CleanupFailures: + """Attempt every approved mutation, then fail with complete diagnostics.""" + + def __init__(self) -> None: + self._messages: list[str] = [] + + def run(self, argv: Sequence[str], description: str) -> None: + try: + result = subprocess.run( + list(argv), capture_output=True, text=True, check=False, + ) + except OSError as exc: + self._messages.append(f"{description}: {exc}") + return + if result.returncode != 0: + detail = (result.stderr or result.stdout).strip() + self._messages.append( + f"{description}: {detail or f'exit {result.returncode}'}" + ) + + def remove_tree(self, path: Path, description: str) -> None: + try: + shutil.rmtree(path) + except FileNotFoundError: + return + except OSError as exc: + self._messages.append(f"{description}: {exc}") + + def raise_if_any(self) -> None: + if self._messages: + raise CleanupError("; ".join(self._messages)) + + +__all__ = ["CleanupError", "CleanupFailures"] diff --git a/bot_bottle/backend/docker/bottle_cleanup_plan.py b/bot_bottle/backend/docker/bottle_cleanup_plan.py index 13dea062..9ceb919c 100644 --- a/bot_bottle/backend/docker/bottle_cleanup_plan.py +++ b/bot_bottle/backend/docker/bottle_cleanup_plan.py @@ -46,6 +46,22 @@ class DockerBottleCleanupPlan(BottleCleanupPlan): and not self.orphan_state_dirs ) + def intersect(self, current: BottleCleanupPlan) -> "DockerBottleCleanupPlan": + if not isinstance(current, DockerBottleCleanupPlan): + raise TypeError("cleanup plans must have the same backend type") + return DockerBottleCleanupPlan( + projects=tuple(x for x in self.projects if x in current.projects), + stray_containers=tuple( + x for x in self.stray_containers if x in current.stray_containers + ), + stray_networks=tuple( + x for x in self.stray_networks if x in current.stray_networks + ), + orphan_state_dirs=tuple( + x for x in self.orphan_state_dirs if x in current.orphan_state_dirs + ), + ) + def print(self) -> None: print(file=sys.stderr) for name in self.projects: diff --git a/bot_bottle/backend/docker/cleanup.py b/bot_bottle/backend/docker/cleanup.py index 4cc0f331..72b27b12 100644 --- a/bot_bottle/backend/docker/cleanup.py +++ b/bot_bottle/backend/docker/cleanup.py @@ -23,12 +23,12 @@ Active-agent enumeration lives in `backend/docker/enumerate.py`. from __future__ import annotations -import shutil import subprocess from ...paths import bot_bottle_root -from ...log import info, warn +from ...log import info from .. import EnumerationError +from ..cleanup_control import CleanupFailures from . import util as docker_mod from .bottle_cleanup_plan import DockerBottleCleanupPlan from ...bottle_state import bottle_state_dir, is_preserved @@ -150,40 +150,30 @@ def cleanup(plan: DockerBottleCleanupPlan) -> None: """Remove everything in the plan. Projects first (whose `compose down` reaps their containers + networks atomically), then stray legacy resources, then orphan state dirs.""" + failures = CleanupFailures() for project in plan.projects: info(f"docker compose down ({project})") - result = subprocess.run( + failures.run( ["docker", "compose", "-p", project, "down", "--volumes"], - capture_output=True, text=True, check=False, + f"docker compose down failed for {project}", ) - if result.returncode != 0: - warn( - f"compose down failed for {project}: " - f"{result.stderr.strip()}" - ) for name in plan.stray_containers: info(f"removing stray container {name}") - subprocess.run( + failures.run( ["docker", "rm", "-f", name], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - check=False, + f"removing stray container {name}", ) for name in plan.stray_networks: info(f"removing stray network {name}") - subprocess.run( + failures.run( ["docker", "network", "rm", name], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - check=False, + f"removing stray network {name}", ) for identity in plan.orphan_state_dirs: path = bottle_state_dir(identity) info(f"removing orphan state dir {path}") - try: - shutil.rmtree(path, ignore_errors=True) - except OSError as e: - warn(f"failed to remove {path}: {e}") + failures.remove_tree(path, f"removing orphan state dir {path}") + failures.raise_if_any() diff --git a/bot_bottle/backend/firecracker/bottle_cleanup_plan.py b/bot_bottle/backend/firecracker/bottle_cleanup_plan.py index a0522395..fd9787e1 100644 --- a/bot_bottle/backend/firecracker/bottle_cleanup_plan.py +++ b/bot_bottle/backend/firecracker/bottle_cleanup_plan.py @@ -27,3 +27,11 @@ class FirecrackerBottleCleanupPlan(BottleCleanupPlan): @property def empty(self) -> bool: return not (self.vm_pids or self.run_dirs) + + def intersect(self, current: BottleCleanupPlan) -> "FirecrackerBottleCleanupPlan": + if not isinstance(current, FirecrackerBottleCleanupPlan): + raise TypeError("cleanup plans must have the same backend type") + return FirecrackerBottleCleanupPlan( + vm_pids=tuple(x for x in self.vm_pids if x in current.vm_pids), + run_dirs=tuple(x for x in self.run_dirs if x in current.run_dirs), + ) diff --git a/bot_bottle/backend/firecracker/cleanup.py b/bot_bottle/backend/firecracker/cleanup.py index f0f63b6f..0e560684 100644 --- a/bot_bottle/backend/firecracker/cleanup.py +++ b/bot_bottle/backend/firecracker/cleanup.py @@ -23,13 +23,13 @@ from __future__ import annotations from collections.abc import Sequence import os -import shutil import signal import subprocess from pathlib import Path from ...log import info from .. import EnumerationError +from ..cleanup_control import CleanupFailures from . import lifecycle_lock, util from .bottle_cleanup_plan import FirecrackerBottleCleanupPlan @@ -151,11 +151,13 @@ def cleanup(plan: FirecrackerBottleCleanupPlan) -> None: fresh = prepare_cleanup() approved_pids = set(plan.vm_pids).intersection(fresh.vm_pids) approved_dirs = set(plan.run_dirs).intersection(fresh.run_dirs) + failures = CleanupFailures() for pid in sorted(approved_pids): _terminate_orphan(pid, _run_root()) for path in sorted(approved_dirs): info(f"rm -rf {path}") - shutil.rmtree(path, ignore_errors=True) + failures.remove_tree(Path(path), f"removing Firecracker run dir {path}") + failures.raise_if_any() def _terminate_orphan(pid: int, run_root: Path) -> None: diff --git a/bot_bottle/backend/macos_container/bottle_cleanup_plan.py b/bot_bottle/backend/macos_container/bottle_cleanup_plan.py index 679a4f60..17232aab 100644 --- a/bot_bottle/backend/macos_container/bottle_cleanup_plan.py +++ b/bot_bottle/backend/macos_container/bottle_cleanup_plan.py @@ -25,3 +25,11 @@ class MacosContainerBottleCleanupPlan(BottleCleanupPlan): @property def empty(self) -> bool: return not self.containers and not self.networks + + def intersect(self, current: BottleCleanupPlan) -> "MacosContainerBottleCleanupPlan": + if not isinstance(current, MacosContainerBottleCleanupPlan): + raise TypeError("cleanup plans must have the same backend type") + return MacosContainerBottleCleanupPlan( + containers=tuple(x for x in self.containers if x in current.containers), + networks=tuple(x for x in self.networks if x in current.networks), + ) diff --git a/bot_bottle/backend/macos_container/cleanup.py b/bot_bottle/backend/macos_container/cleanup.py index f3beb2de..026ea65c 100644 --- a/bot_bottle/backend/macos_container/cleanup.py +++ b/bot_bottle/backend/macos_container/cleanup.py @@ -5,6 +5,7 @@ from __future__ import annotations import subprocess from .. import EnumerationError +from ..cleanup_control import CleanupFailures from ...log import info from . import util as container_mod from .bottle_cleanup_plan import MacosContainerBottleCleanupPlan @@ -53,19 +54,17 @@ def prepare_cleanup() -> MacosContainerBottleCleanupPlan: def cleanup(plan: MacosContainerBottleCleanupPlan) -> None: + failures = CleanupFailures() for name in plan.containers: info(f"container delete --force {name}") - subprocess.run( + failures.run( ["container", "delete", "--force", name], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - check=False, + f"deleting container {name}", ) for name in plan.networks: info(f"container network delete {name}") - subprocess.run( + failures.run( ["container", "network", "delete", name], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - check=False, + f"deleting network {name}", ) + failures.raise_if_any() diff --git a/bot_bottle/cli/commands/cleanup.py b/bot_bottle/cli/commands/cleanup.py index d64013d6..24d7c297 100644 --- a/bot_bottle/cli/commands/cleanup.py +++ b/bot_bottle/cli/commands/cleanup.py @@ -22,6 +22,7 @@ from __future__ import annotations import sys from ...backend import get_bottle_backend, has_backend, known_backend_names +from ...backend.cleanup_control import CleanupError from ...log import info from ...util import read_tty_line @@ -54,12 +55,18 @@ def cmd_cleanup(_argv: list[str]) -> int: # Confirmation authorizes a fresh authoritative snapshot, not blind use of # identities that may have changed while the operator reviewed the preview. - refreshed = [(name, backend, backend.prepare_cleanup()) - for name, backend, _plan in prepared] - for name, backend, plan in refreshed: - if plan.empty: + failures: list[str] = [] + for name, backend, displayed in prepared: + current = backend.prepare_cleanup() + approved = displayed.intersect(current) + if approved.empty: continue - backend.cleanup(plan) + try: + backend.cleanup(approved) + except CleanupError as exc: + failures.append(f"{name}: {exc}") + if failures: + raise CleanupError("cleanup incomplete: " + "; ".join(failures)) info("cleanup: done") return 0 diff --git a/bot_bottle/gateway/git_gate/http_backend.py b/bot_bottle/gateway/git_gate/http_backend.py index cb33d239..4ac3eb39 100644 --- a/bot_bottle/gateway/git_gate/http_backend.py +++ b/bot_bottle/gateway/git_gate/http_backend.py @@ -203,14 +203,19 @@ class GitHttpHandler(BaseHTTPRequestHandler): except BodyReadError as exc: self.send_error(exc.status, exc.message) return - proc = subprocess.run( - ["git", "http-backend"], - input=body, - env=env, - capture_output=True, - check=False, - timeout=GIT_GATE_TIMEOUT_SECS, - ) + try: + proc = subprocess.run( + ["git", "http-backend"], + input=body, + env=env, + capture_output=True, + check=False, + timeout=GIT_GATE_TIMEOUT_SECS, + ) + except (OSError, subprocess.SubprocessError) as exc: + self.log_message("git http-backend unavailable: %s", exc) + self.send_error(503, "git backend unavailable") + return self._write_cgi_response(proc.stdout) def _repo_dir(self, sandbox_root: Path, path: str) -> Path | None: diff --git a/bot_bottle/orchestrator/service.py b/bot_bottle/orchestrator/service.py index 68e6e7fd..a3c5bd8b 100644 --- a/bot_bottle/orchestrator/service.py +++ b/bot_bottle/orchestrator/service.py @@ -366,7 +366,7 @@ class OrchestratorCore: value with *env_var_secret*, and restores ``_tokens[bottle_id]``. Returns True on success, False when no stored secrets exist for this bottle or decryption fails (wrong key / corrupt data).""" - from .store.secret_store import decrypt_value, encrypt_value, is_legacy_blob + from .store.secret_store import decrypt_value encrypted = self.registry.get_agent_secrets(bottle_id) if not encrypted: return False @@ -377,12 +377,6 @@ class OrchestratorCore: except ValueError: return False self._tokens[bottle_id] = decrypted - if any(is_legacy_blob(value) for value in encrypted.values()): - migrated = { - key: encrypt_value(env_var_secret, value) - for key, value in decrypted.items() - } - self.registry.store_agent_secrets(bottle_id, migrated) return True # --- consolidated gateway ---------------------------------------------- diff --git a/bot_bottle/orchestrator/store/registry_store.py b/bot_bottle/orchestrator/store/registry_store.py index f6284929..0225cd25 100644 --- a/bot_bottle/orchestrator/store/registry_store.py +++ b/bot_bottle/orchestrator/store/registry_store.py @@ -129,6 +129,10 @@ _MIGRATIONS = TableMigrations( # v5 — index for fast per-bottle lookups and bulk DELETE on teardown. "CREATE INDEX IF NOT EXISTS idx_bottled_agent_secrets_id " "ON bottled_agent_secrets (bottled_agent_id, type)", + # v6 — unauthenticated legacy ciphertext must never be selected by + # attacker-controlled blob contents. Existing local agents are + # intentionally reprovisioned instead of retaining downgrade support. + "DELETE FROM bottled_agent_secrets", ], ) diff --git a/bot_bottle/orchestrator/store/secret_store.py b/bot_bottle/orchestrator/store/secret_store.py index 10d7d209..465b25f2 100644 --- a/bot_bottle/orchestrator/store/secret_store.py +++ b/bot_bottle/orchestrator/store/secret_store.py @@ -18,9 +18,9 @@ value is encrypted independently. New output blobs are: ``version || nonce (16 bytes) || ciphertext || tag (32 bytes)`` -encoded as URL-safe base64 (no padding). The version marker lets the reader -accept legacy ``nonce || ciphertext`` rows long enough to rewrite them in the -authenticated format after a successful reprovision. +encoded as URL-safe base64 (no padding). Unversioned legacy ciphertext is +rejected; the registry migration clears those rows rather than allowing blob +contents to select an unauthenticated decoder. keystream_block_i = HMAC-SHA256(key, nonce || i.to_bytes(4, "big")) ciphertext_i = plaintext_i XOR keystream_block_i[:len(plaintext_i)] @@ -84,44 +84,18 @@ def encrypt_value(secret_b64: str, plaintext: str) -> str: return base64.urlsafe_b64encode(authenticated + tag).rstrip(b"=").decode() -def is_legacy_blob(blob_b64: str) -> bool: - """Whether *blob_b64* uses the pre-authentication storage format.""" - try: - return not _b64dec(blob_b64).startswith(_VERSION) - except (ValueError, TypeError): - return False - - -def _decrypt_legacy(key: bytes, blob: bytes) -> str: - """Read the original ``nonce || ciphertext`` format for migration only.""" - if len(blob) < _NONCE_BYTES: - raise ValueError("ciphertext blob too short") - nonce, ciphertext = blob[:_NONCE_BYTES], blob[_NONCE_BYTES:] - pt = bytearray() - # The legacy format used the byte offset as the PRF counter. - for i in range(0, len(ciphertext), _BLOCK): - chunk = ciphertext[i : i + _BLOCK] - ks = _keystream(key, nonce, i)[: len(chunk)] - pt.extend(c ^ k for c, k in zip(chunk, ks)) - try: - return bytes(pt).decode() - except UnicodeDecodeError as exc: - raise ValueError(f"decryption produced non-UTF-8 output: {exc}") from exc - - def decrypt_value(secret_b64: str, blob_b64: str) -> str: """Decrypt a blob produced by :func:`encrypt_value`. Returns the original plaintext string. Raises ``ValueError`` for malformed - input, authentication failure, or a key mismatch. Legacy unauthenticated - rows remain readable so callers can migrate them immediately.""" + input, authentication failure, or a key mismatch.""" key = _b64dec(secret_b64) try: blob = _b64dec(blob_b64) except (ValueError, TypeError) as exc: raise ValueError(f"invalid ciphertext blob: {exc}") from exc if not blob.startswith(_VERSION): - return _decrypt_legacy(key, blob) + raise ValueError("unsupported ciphertext format") minimum = len(_VERSION) + _NONCE_BYTES + _TAG_BYTES if len(blob) < minimum: raise ValueError("ciphertext blob too short") @@ -152,5 +126,4 @@ __all__ = [ "new_env_var_secret", "encrypt_value", "decrypt_value", - "is_legacy_blob", ] diff --git a/tests/unit/test_cli_cleanup_cross_backend.py b/tests/unit/test_cli_cleanup_cross_backend.py index d66fa673..5a4f0158 100644 --- a/tests/unit/test_cli_cleanup_cross_backend.py +++ b/tests/unit/test_cli_cleanup_cross_backend.py @@ -17,6 +17,7 @@ from bot_bottle.cli.commands import cleanup as cmd def _make_backend(empty: bool = True): backend = MagicMock() plan = MagicMock(empty=empty) + plan.intersect.return_value = plan backend.prepare_cleanup.return_value = plan backend.cleanup = MagicMock() return backend, plan @@ -135,10 +136,12 @@ class TestCmdCleanup(unittest.TestCase): docker.cleanup.assert_called_once_with(docker_plan) fc.cleanup.assert_not_called() - def test_executes_refreshed_plan_after_confirmation(self): + def test_executes_only_displayed_resources_still_current(self): backend = MagicMock() preview = MagicMock(empty=False) refreshed = MagicMock(empty=False) + approved = MagicMock(empty=False) + preview.intersect.return_value = approved backend.prepare_cleanup.side_effect = [preview, refreshed] with patch.object( @@ -152,7 +155,8 @@ class TestCmdCleanup(unittest.TestCase): ): self.assertEqual(0, cmd.cmd_cleanup([])) - backend.cleanup.assert_called_once_with(refreshed) + preview.intersect.assert_called_once_with(refreshed) + backend.cleanup.assert_called_once_with(approved) if __name__ == "__main__": diff --git a/tests/unit/test_firecracker_cleanup.py b/tests/unit/test_firecracker_cleanup.py index f41256e8..c9a54083 100644 --- a/tests/unit/test_firecracker_cleanup.py +++ b/tests/unit/test_firecracker_cleanup.py @@ -167,11 +167,11 @@ class TestCleanupRemoval(unittest.TestCase): with patch.object(fc_cleanup, "prepare_cleanup", return_value=plan), \ patch.object(fc_cleanup, "_run_root", return_value=Path("/run")), \ patch.object(fc_cleanup, "_terminate_orphan") as terminate, \ - patch.object(fc_cleanup.shutil, "rmtree") as rmtree, \ + patch("bot_bottle.backend.cleanup_control.shutil.rmtree") as rmtree, \ patch.object(fc_cleanup, "info"): fc_cleanup.cleanup(plan) terminate.assert_called_once_with(101, Path("/run")) - rmtree.assert_called_once_with("/run/dev-x", ignore_errors=True) + rmtree.assert_called_once_with(Path("/run/dev-x")) def test_cleanup_skips_resources_no_longer_in_refreshed_plan(self): preview = FirecrackerBottleCleanupPlan( @@ -181,7 +181,7 @@ class TestCleanupRemoval(unittest.TestCase): fc_cleanup, "prepare_cleanup", return_value=FirecrackerBottleCleanupPlan(), ), patch.object(fc_cleanup, "_terminate_orphan") as terminate, \ - patch.object(fc_cleanup.shutil, "rmtree") as rmtree: + patch("bot_bottle.backend.cleanup_control.shutil.rmtree") as rmtree: fc_cleanup.cleanup(preview) terminate.assert_not_called() rmtree.assert_not_called() diff --git a/tests/unit/test_git_http_backend.py b/tests/unit/test_git_http_backend.py index 89f162cb..427e469b 100644 --- a/tests/unit/test_git_http_backend.py +++ b/tests/unit/test_git_http_backend.py @@ -486,6 +486,19 @@ class TestMalformedStatusHeader(unittest.TestCase): ) self.assertEqual(500, status) + def test_backend_timeout_returns_503(self): + with mock.patch( + "bot_bottle.gateway.git_gate.http_backend.subprocess.run", + side_effect=subprocess.TimeoutExpired(["git", "http-backend"], 1), + ): + req = urllib.request.Request( + f"http://127.0.0.1:{self._port}/repo.git/info/refs", + method="GET", + ) + with self.assertRaises(urllib.error.HTTPError) as raised: + urllib.request.urlopen(req, timeout=3) + self.assertEqual(503, raised.exception.code) + class TestContentLengthBounds(unittest.TestCase): """PRD 0041: malformed or oversized Content-Length is rejected before diff --git a/tests/unit/test_macos_container_cleanup.py b/tests/unit/test_macos_container_cleanup.py index fbe13a4e..c1892137 100644 --- a/tests/unit/test_macos_container_cleanup.py +++ b/tests/unit/test_macos_container_cleanup.py @@ -6,6 +6,7 @@ import unittest from unittest.mock import patch from bot_bottle.backend import EnumerationError +from bot_bottle.backend.cleanup_control import CleanupError from bot_bottle.backend.macos_container import cleanup, enumerate as enum_mod from bot_bottle.backend.macos_container.bottle_cleanup_plan import ( MacosContainerBottleCleanupPlan, @@ -31,7 +32,10 @@ class TestMacosContainerCleanup(unittest.TestCase): containers=("bot-bottle-a",), networks=("bot-bottle-net-a",), ) - with patch.object(cleanup.subprocess, "run") as run: + completed = cleanup.subprocess.CompletedProcess( + args=[], returncode=0, stdout="", stderr="", + ) + with patch.object(cleanup.subprocess, "run", return_value=completed) as run: cleanup.cleanup(plan) self.assertEqual( ["container", "delete", "--force", "bot-bottle-a"], @@ -42,6 +46,18 @@ class TestMacosContainerCleanup(unittest.TestCase): run.call_args_list[1].args[0], ) + def test_cleanup_attempts_all_resources_then_raises(self): + plan = MacosContainerBottleCleanupPlan( + containers=("bot-bottle-a",), networks=("bot-bottle-net-a",), + ) + failed = cleanup.subprocess.CompletedProcess( + args=[], returncode=1, stdout="", stderr="unavailable", + ) + with patch.object(cleanup.subprocess, "run", return_value=failed) as run, \ + self.assertRaisesRegex(CleanupError, "unavailable"): + cleanup.cleanup(plan) + self.assertEqual(2, run.call_count) + def test_container_enumeration_failure_aborts(self): completed = cleanup.subprocess.CompletedProcess( args=[], returncode=1, stdout="", stderr="service unavailable", diff --git a/tests/unit/test_orchestrator_registry_store.py b/tests/unit/test_orchestrator_registry_store.py index 8b46397a..45608dfc 100644 --- a/tests/unit/test_orchestrator_registry_store.py +++ b/tests/unit/test_orchestrator_registry_store.py @@ -224,6 +224,17 @@ class TestAgentSecrets(unittest.TestCase): reopened = RegistryStore(self.db) self.assertEqual({"K": "v"}, reopened.get_agent_secrets("bottle-1")) + def test_v6_migration_clears_legacy_secret_rows(self) -> None: + self.store.store_agent_secrets("bottle-1", {"K": "legacy"}) + with closing(sqlite3.connect(self.db)) as conn: + conn.execute( + "UPDATE schema_versions SET version = 5 " + "WHERE module = 'orchestrator_registry'" + ) + conn.commit() + self.store.migrate() + self.assertEqual({}, self.store.get_agent_secrets("bottle-1")) + class TestReapAbsent(unittest.TestCase): """`reap_absent` — the self-heal for rows whose bottle is gone. diff --git a/tests/unit/test_orchestrator_secret_store.py b/tests/unit/test_orchestrator_secret_store.py index 070d3c00..dc433960 100644 --- a/tests/unit/test_orchestrator_secret_store.py +++ b/tests/unit/test_orchestrator_secret_store.py @@ -3,8 +3,6 @@ from __future__ import annotations import base64 -import hashlib -import hmac import unittest from bot_bottle.orchestrator.store.secret_store import ( @@ -83,16 +81,21 @@ class TestDecryptErrors(unittest.TestCase): with self.assertRaisesRegex(ValueError, "authentication failed"): decrypt_value(self.secret, tampered) - def test_reads_legacy_ciphertext_for_migration(self) -> None: - key = base64.urlsafe_b64decode(self.secret + "==") - nonce = b"0123456789abcdef" - plaintext = b"legacy-token" - stream = hmac.new( - key, nonce + (0).to_bytes(4, "big"), hashlib.sha256, - ).digest() - ciphertext = bytes(p ^ k for p, k in zip(plaintext, stream)) - legacy = base64.urlsafe_b64encode(nonce + ciphertext).rstrip(b"=").decode() - self.assertEqual("legacy-token", decrypt_value(self.secret, legacy)) + def test_rejects_legacy_ciphertext(self) -> None: + legacy = base64.urlsafe_b64encode( + b"0123456789abcdeflegacy-token", + ).rstrip(b"=").decode() + with self.assertRaisesRegex(ValueError, "unsupported ciphertext format"): + decrypt_value(self.secret, legacy) + + def test_rejects_authenticated_blob_with_changed_version(self) -> None: + raw = bytearray(base64.urlsafe_b64decode( + encrypt_value(self.secret, "secret-token") + "==" + )) + raw[0] ^= 1 + downgraded = base64.urlsafe_b64encode(raw).rstrip(b"=").decode() + with self.assertRaisesRegex(ValueError, "unsupported ciphertext format"): + decrypt_value(self.secret, downgraded) def test_truncated_blob_raises_value_error(self) -> None: with self.assertRaises(ValueError):