From 5c08701983ad1b8412619678e92d83243f26110b Mon Sep 17 00:00:00 2001 From: didericis Date: Fri, 17 Jul 2026 00:31:00 -0400 Subject: [PATCH 1/4] feat(firecracker): port freeze/migrate off host Docker (PRD 0069 / #397) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last host-Docker dependency in the Firecracker launch path. Freeze and resume no longer touch the docker daemon, so the backend needs firecracker + KVM only — completing #348. Freeze: stream the guest rootfs over SSH straight into a persistent committed-rootfs.tar (the resumable/migratable artifact) instead of round-tripping through `docker build` from a scratch image. Written to a .partial sibling and atomically renamed so a failed freeze leaves no truncated artifact. Resume: extract the snapshot tar into a cached base dir and feed it to the existing rootless `mke2fs -d` pipeline, replacing the `docker create` + `docker export | tar` path. Recreate the proc/sys/dev/run mount points the freezer excludes so the guest init can mount them. `util.build_base_rootfs_dir` / `docker_image_id` stay — they still back the opt-in BOT_BOTTLE_INFRA_BUILD=local dev path and off-host publish_infra, which are out of scope. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ --- bot_bottle/backend/firecracker/freezer.py | 61 ++++++++-------- bot_bottle/backend/firecracker/launch.py | 20 +++-- bot_bottle/backend/firecracker/util.py | 44 +++++++++++ bot_bottle/bottle_state.py | 11 +++ tests/unit/test_backend_freezer.py | 15 ++-- tests/unit/test_firecracker_helpers.py | 89 +++++++++++++++++++++++ 6 files changed, 193 insertions(+), 47 deletions(-) diff --git a/bot_bottle/backend/firecracker/freezer.py b/bot_bottle/backend/firecracker/freezer.py index 16c6370..e1f3735 100644 --- a/bot_bottle/backend/firecracker/freezer.py +++ b/bot_bottle/backend/firecracker/freezer.py @@ -1,9 +1,12 @@ -"""FirecrackerFreezer — snapshot a running microVM to a Docker image. +"""FirecrackerFreezer — snapshot a running microVM to a rootfs tar. The VM is live and can't be block-copied safely, so — like the macOS backend — we stream the guest root filesystem out over the control -channel (SSH here) and rebuild an image from it. The bottle keeps -running after the snapshot. +channel (SSH here). Unlike the other backends this needs no Docker: the +tar *is* the resumable artifact. `resume` extracts it and rebuilds a +fresh per-bottle ext4 with `mke2fs -d` (see `util.build_committed_rootfs_dir` +and `launch._build_agent_base`). The bottle keeps running after the +snapshot. """ from __future__ import annotations @@ -11,9 +14,9 @@ from __future__ import annotations import json import os import subprocess -import tempfile from pathlib import Path +from ...bottle_state import committed_rootfs_path from ...log import die, info from .. import ActiveAgent from ..freeze import Freezer @@ -30,14 +33,13 @@ class FirecrackerFreezer(Freezer): if not private_key.is_file() or not guest_ip: die(f"cannot freeze {agent.slug}: run dir {run_dir} is missing the " f"SSH key or VM config (is the bottle still running?)") - image_tag = f"bot-bottle-committed-{agent.slug}:latest" - _commit_via_ssh(private_key, guest_ip, image_tag) - info(f"committed {agent.slug} -> {image_tag!r}") - return image_tag + tar_path = committed_rootfs_path(agent.slug) + _commit_rootfs_via_ssh(private_key, guest_ip, tar_path) + info(f"committed {agent.slug} -> {tar_path}") + return str(tar_path) def _export_hint(self, slug: str, image_ref: str) -> None: - info(f"to export for migration: docker image save {image_ref} " - f"-o {slug}.tar") + info(f"to export for migration: cp {image_ref} {slug}.tar") def _guest_ip_from_config(config_path: Path) -> str: @@ -53,24 +55,23 @@ def _guest_ip_from_config(config_path: Path) -> str: return "" -def _commit_via_ssh(private_key: Path, guest_ip: str, image_tag: str) -> None: - with tempfile.TemporaryDirectory(prefix="bot-bottle-fc-commit.") as tmp: - rootfs_tar = os.path.join(tmp, "rootfs.tar") - ssh = util.ssh_base_argv(private_key, guest_ip) - with open(rootfs_tar, "wb") as tar_out: - result = subprocess.run( - [*ssh, "--", "tar", "--create", "--one-file-system", - "--exclude=./proc", "--exclude=./sys", "--exclude=./dev", - "--exclude=./run", "--file=-", "--directory=/", "."], - stdout=tar_out, stderr=subprocess.PIPE, check=False, - ) - if result.returncode != 0: - die(f"ssh tar for {guest_ip} failed: " - f"{(result.stderr or b'').decode().strip() or ''}") - with open(os.path.join(tmp, "Dockerfile"), "w", encoding="utf-8") as f: - f.write("FROM scratch\nADD rootfs.tar /\nUSER node\nWORKDIR /home/node\n") - build = subprocess.run( - ["docker", "build", "-t", image_tag, tmp], check=False, +def _commit_rootfs_via_ssh(private_key: Path, guest_ip: str, tar_path: Path) -> None: + """Stream the guest rootfs out over SSH into `tar_path`. Excludes the + virtual/live mounts (proc/sys/dev/run) — resume recreates those empty + mount points. Written to a `.partial` sibling and renamed on success so + a failed freeze never leaves a truncated artifact in its place.""" + tar_path.parent.mkdir(parents=True, exist_ok=True) + partial = tar_path.with_name(tar_path.name + ".partial") + ssh = util.ssh_base_argv(private_key, guest_ip) + with open(partial, "wb") as tar_out: + result = subprocess.run( + [*ssh, "--", "tar", "--create", "--one-file-system", + "--exclude=./proc", "--exclude=./sys", "--exclude=./dev", + "--exclude=./run", "--file=-", "--directory=/", "."], + stdout=tar_out, stderr=subprocess.PIPE, check=False, ) - if build.returncode != 0: - die(f"docker build for {image_tag!r} failed") + if result.returncode != 0: + partial.unlink(missing_ok=True) + die(f"ssh tar for {guest_ip} failed: " + f"{(result.stderr or b'').decode().strip() or ''}") + os.replace(partial, tar_path) diff --git a/bot_bottle/backend/firecracker/launch.py b/bot_bottle/backend/firecracker/launch.py index a486894..26d3779 100644 --- a/bot_bottle/backend/firecracker/launch.py +++ b/bot_bottle/backend/firecracker/launch.py @@ -1,7 +1,8 @@ """Launch flow for the Firecracker backend (PRD 0070, consolidated). Per bottle: - 1. build the agent image (docker), export it to a cached ext4 rootfs; + 1. build the agent rootfs in a builder VM (buildah, no host docker), or + resume a frozen bottle from its committed rootfs tar; cache the ext4; 2. ensure the per-host orchestrator + shared gateway are up; 3. claim a free TAP pool slot (rootless flock); 4. register the bottle on the orchestrator by the VM's guest IP (the @@ -31,6 +32,7 @@ from typing import Callable, Generator from ...agent_provider import runtime_for from ...bottle_state import ( + committed_rootfs_path, egress_state_dir, git_gate_state_dir, read_committed_image, @@ -45,7 +47,6 @@ from ...git_gate import ( ) from ...log import info, warn from ...supervise import SUPERVISE_PORT -from ..docker import util as docker_mod from ..docker.egress import EGRESS_PORT from ..util import AGENT_CA_BUNDLE, AGENT_CA_PATH from . import firecracker_vm, image_builder, isolation_probe, netpool, util @@ -210,16 +211,13 @@ def _build_agent_base( ) -> tuple[FirecrackerBottlePlan, Path]: """Produce the agent's base rootfs dir. Primary path: build the Dockerfile inside a Firecracker builder VM (buildah, no host docker), smoke-testing - the image before export. A committed snapshot (freeze/migrate) is still - exported via the host docker path until that is ported too.""" + the image before export. A committed snapshot (freeze/migrate) is resumed + directly from the rootfs tar the freezer wrote — no host docker either.""" committed = read_committed_image(plan.slug) - if committed and docker_mod.image_exists(committed): - info(f"using committed image {committed!r}") - plan = dataclasses.replace( - plan, - agent_provision=dataclasses.replace(plan.agent_provision, image=committed), - ) - return plan, util.build_base_rootfs_dir(committed) + committed_tar = committed_rootfs_path(plan.slug) + if committed and committed_tar.is_file(): + info(f"resuming from committed rootfs {committed_tar}") + return plan, util.build_committed_rootfs_dir(committed_tar) base = image_builder.build_agent_rootfs_dir( Path(plan.dockerfile_path), image_tag=plan.image, diff --git a/bot_bottle/backend/firecracker/util.py b/bot_bottle/backend/firecracker/util.py index 50bcd9e..4421714 100644 --- a/bot_bottle/backend/firecracker/util.py +++ b/bot_bottle/backend/firecracker/util.py @@ -14,6 +14,7 @@ and `./cli.py backend setup --backend=firecracker`. from __future__ import annotations +import hashlib import os import platform import shutil @@ -212,6 +213,49 @@ def build_base_rootfs_dir( return base +def build_committed_rootfs_dir(tar_path: Path) -> Path: + """Prepare a base rootfs dir from a frozen-bottle snapshot tar (the + freeze/resume path — no Docker). Extracts the snapshot, recreates the + virtual mount points the freezer excluded, and injects the guest init + + static dropbear, mirroring `build_base_rootfs_dir` but sourced from a tar + we control rather than a Docker image. + + Cached under the rootfs cache, keyed by the tar's size+mtime so a + re-freeze re-extracts but repeated resumes of the same snapshot don't. + Returns the prepared directory (read as the `mke2fs -d` source).""" + st = tar_path.stat() + fingerprint = hashlib.sha256( + f"{tar_path}:{st.st_size}:{st.st_mtime_ns}".encode() + ).hexdigest()[:16] + base = cache_dir() / "rootfs" / f"committed-{fingerprint}" + ready = base / ".bb-ready" + if ready.is_file(): + return base + + if base.exists(): + shutil.rmtree(base, ignore_errors=True) + base.mkdir(parents=True) + + info(f"extracting committed rootfs {tar_path} -> {base}") + result = subprocess.run( + ["tar", "-x", "-f", str(tar_path), "-C", str(base)], + capture_output=True, text=True, check=False, + ) + if result.returncode != 0: + die(f"extracting committed rootfs {tar_path} failed: " + f"{result.stderr.strip() or ''}") + + # The freezer excludes the live/virtual filesystems from the snapshot; + # recreate them as empty mount points so the guest init can mount + # proc/sys/dev and dropbear has a writable /run. + for mount_point in ("proc", "sys", "dev", "run"): + (base / mount_point).mkdir(mode=0o755, exist_ok=True) + + inject_guest_boot(base) + ready.write_text("ok\n") + return base + + def inject_guest_boot(rootfs: Path, init_script: str | None = None) -> None: """Drop the static dropbear and the PID-1 init into the rootfs. `init_script` defaults to the SSH-only agent init; the infra VM diff --git a/bot_bottle/bottle_state.py b/bot_bottle/bottle_state.py index 4f3b04d..a0ee82a 100644 --- a/bot_bottle/bottle_state.py +++ b/bot_bottle/bottle_state.py @@ -44,6 +44,7 @@ from .paths import bot_bottle_root _STATE_SUBDIR = "state" _PER_BOTTLE_DOCKERFILE_NAME = "Dockerfile" _COMMITTED_IMAGE_NAME = "committed-image" +_COMMITTED_ROOTFS_NAME = "committed-rootfs.tar" _TRANSCRIPT_SUBDIR = "transcript" # Per-daemon scratch subdirs. PRD 0018 chunk 2: bind-mount sources # live here so chunk 3's `docker compose up` can find them at stable @@ -200,6 +201,15 @@ def committed_image_path(identity: str) -> Path: return bottle_state_dir(identity) / _COMMITTED_IMAGE_NAME +def committed_rootfs_path(identity: str) -> Path: + """Where the Firecracker freezer stores a snapshot of the bottle's + guest rootfs (a plain tar). This is the resumable/migratable artifact + the Firecracker backend boots from — no Docker image involved. The + matching `committed-image` state file records that a snapshot exists + (and its path); `resume` boots from this tar when both are present.""" + return bottle_state_dir(identity) / _COMMITTED_ROOTFS_NAME + + def write_committed_image(identity: str, image_tag: str) -> Path: """Persist the committed image tag for `identity`. The next `cli.py resume ` will boot from this image instead of @@ -354,6 +364,7 @@ __all__ = [ "cleanup_state", "clear_preserve_marker", "committed_image_path", + "committed_rootfs_path", "egress_state_dir", "git_gate_state_dir", "is_preserved", diff --git a/tests/unit/test_backend_freezer.py b/tests/unit/test_backend_freezer.py index 3cb7d54..b46ee72 100644 --- a/tests/unit/test_backend_freezer.py +++ b/tests/unit/test_backend_freezer.py @@ -205,25 +205,28 @@ class TestFirecrackerFreezer(_FakeHomeMixin, unittest.TestCase): ) def test_snapshots_running_vm_without_stopping(self): - """Commit should tar the running guest rootfs over SSH, not stop it.""" + """Commit should tar the running guest rootfs over SSH into the + committed-rootfs artifact (no Docker), not stop the VM.""" slug = "dev-abc12" self._write_meta(slug) self._stage_run_dir(slug) freezer = FirecrackerFreezer() agent = _make_agent(slug, "firecracker") - with patch("bot_bottle.backend.firecracker.freezer._commit_via_ssh") as mock_commit, \ + with patch("bot_bottle.backend.firecracker.freezer._commit_rootfs_via_ssh") as mock_commit, \ patch("bot_bottle.backend.freeze.info"), \ patch("bot_bottle.backend.firecracker.freezer.info"): freezer.commit(agent) - image_tag = f"bot-bottle-committed-{slug}:latest" + tar_path = bottle_state.committed_rootfs_path(slug) self.assertEqual(1, mock_commit.call_count) - # (private_key, guest_ip, image_tag) — guest_ip parsed from config. + # (private_key, guest_ip, tar_path) — guest_ip parsed from config. args = mock_commit.call_args.args self.assertEqual("100.64.0.1", args[1]) - self.assertEqual(image_tag, args[2]) - self.assertEqual(image_tag, bottle_state.read_committed_image(slug)) + self.assertEqual(tar_path, args[2]) + # The committed-image state records the artifact path; resume boots + # from the tar rather than a Docker image. + self.assertEqual(str(tar_path), bottle_state.read_committed_image(slug)) self.assertTrue(bottle_state.is_preserved(slug)) diff --git a/tests/unit/test_firecracker_helpers.py b/tests/unit/test_firecracker_helpers.py index d48c7a2..e1173d1 100644 --- a/tests/unit/test_firecracker_helpers.py +++ b/tests/unit/test_firecracker_helpers.py @@ -6,6 +6,7 @@ branches. Mock subprocess/os so nothing needs KVM or a live VM. from __future__ import annotations import json +import os import subprocess import tempfile import unittest @@ -128,5 +129,93 @@ class TestRequireFirecracker(unittest.TestCase): util.require_firecracker() +class TestBuildCommittedRootfsDir(unittest.TestCase): + """Resume prepares the base rootfs dir from the freezer's snapshot tar + with no Docker: extract, recreate the excluded mount points, inject the + guest boot bits.""" + + def _make_tar(self, tmp: Path) -> Path: + import tarfile + + src = tmp / "src" + (src / "home" / "node").mkdir(parents=True) + (src / "home" / "node" / "hello").write_text("hi") + tar_path = tmp / "rootfs.tar" + with tarfile.open(tar_path, "w") as tar: + tar.add(src, arcname=".") + return tar_path + + def test_extracts_recreates_mountpoints_and_injects_boot(self): + with tempfile.TemporaryDirectory(prefix="fc-committed.") as d: + tmp = Path(d) + tar_path = self._make_tar(tmp) + # Stand in for the static dropbear that inject_guest_boot copies. + dropbear = tmp / "dropbear" + dropbear.write_text("#!/bin/true\n") + cache = tmp / "cache" + cache.mkdir() + + with patch.object(util, "cache_dir", return_value=cache), \ + patch.object(util, "dropbear_path", return_value=dropbear), \ + patch.object(util, "info"): + base = util.build_committed_rootfs_dir(tar_path) + + self.assertEqual("hi", (base / "home" / "node" / "hello").read_text()) + for mount_point in ("proc", "sys", "dev", "run"): + self.assertTrue((base / mount_point).is_dir(), + f"missing recreated mount point /{mount_point}") + self.assertTrue((base / "bb-dropbear").is_file()) + self.assertTrue((base / "bb-init").is_file()) + self.assertTrue((base / ".bb-ready").is_file()) + + def test_caches_on_repeat_and_reextracts_after_refreeze(self): + with tempfile.TemporaryDirectory(prefix="fc-committed.") as d: + tmp = Path(d) + tar_path = self._make_tar(tmp) + dropbear = tmp / "dropbear" + dropbear.write_text("#!/bin/true\n") + cache = tmp / "cache" + cache.mkdir() + + ctx = [ + patch.object(util, "cache_dir", return_value=cache), + patch.object(util, "dropbear_path", return_value=dropbear), + patch.object(util, "info"), + ] + for c in ctx: + c.start() + self.addCleanup(lambda: [c.stop() for c in ctx]) + + real_run = subprocess.run + calls = {"n": 0} + + def counting_run(argv, *a, **k): + if argv and argv[0] == "tar": + calls["n"] += 1 + return real_run(argv, *a, **k) + + with patch.object(util.subprocess, "run", side_effect=counting_run): + first = util.build_committed_rootfs_dir(tar_path) + second = util.build_committed_rootfs_dir(tar_path) + self.assertEqual(first, second) + self.assertEqual(1, calls["n"]) # cached — no re-extract + + # A re-freeze rewrites the tar; a new size/mtime -> new cache + # key -> re-extract. Force a distinct mtime so the test isn't + # at the mercy of filesystem timestamp granularity. + import tarfile + extra = tmp / "extra" + extra.mkdir() + (extra / "note").write_text("v2") + with tarfile.open(tar_path, "w") as tar: + tar.add(extra, arcname=".") + st = tar_path.stat() + os.utime(tar_path, ns=(st.st_atime_ns, st.st_mtime_ns + 1_000_000_000)) + + third = util.build_committed_rootfs_dir(tar_path) + self.assertNotEqual(first, third) + self.assertEqual(2, calls["n"]) + + if __name__ == "__main__": unittest.main() -- 2.52.0 From d0a0ce8d606e5e3f21e2490230c567d515677692 Mon Sep 17 00:00:00 2001 From: didericis Date: Fri, 17 Jul 2026 00:36:43 -0400 Subject: [PATCH 2/4] test(firecracker): satisfy pyright strict + line length in committed-rootfs tests Annotate the counting_run subprocess.run wrapper (reportMissingParameterType) and wrap an over-long patch target line. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ --- tests/unit/test_backend_freezer.py | 3 ++- tests/unit/test_firecracker_helpers.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_backend_freezer.py b/tests/unit/test_backend_freezer.py index b46ee72..304f6d6 100644 --- a/tests/unit/test_backend_freezer.py +++ b/tests/unit/test_backend_freezer.py @@ -213,7 +213,8 @@ class TestFirecrackerFreezer(_FakeHomeMixin, unittest.TestCase): freezer = FirecrackerFreezer() agent = _make_agent(slug, "firecracker") - with patch("bot_bottle.backend.firecracker.freezer._commit_rootfs_via_ssh") as mock_commit, \ + commit_fn = "bot_bottle.backend.firecracker.freezer._commit_rootfs_via_ssh" + with patch(commit_fn) as mock_commit, \ patch("bot_bottle.backend.freeze.info"), \ patch("bot_bottle.backend.firecracker.freezer.info"): freezer.commit(agent) diff --git a/tests/unit/test_firecracker_helpers.py b/tests/unit/test_firecracker_helpers.py index e1173d1..76f97d8 100644 --- a/tests/unit/test_firecracker_helpers.py +++ b/tests/unit/test_firecracker_helpers.py @@ -11,6 +11,7 @@ import subprocess import tempfile import unittest from pathlib import Path +from typing import Any from unittest.mock import patch from bot_bottle.backend.firecracker import firecracker_vm, freezer, netpool, util @@ -189,7 +190,7 @@ class TestBuildCommittedRootfsDir(unittest.TestCase): real_run = subprocess.run calls = {"n": 0} - def counting_run(argv, *a, **k): + def counting_run(argv: list[str], *a: Any, **k: Any) -> Any: if argv and argv[0] == "tar": calls["n"] += 1 return real_run(argv, *a, **k) -- 2.52.0 From 39d47b8108702d898cb00db5ba4b654350be144d Mon Sep 17 00:00:00 2001 From: didericis Date: Fri, 17 Jul 2026 00:44:06 -0400 Subject: [PATCH 3/4] fix(firecracker): harden committed-snapshot resume against guest-controlled data Address the codex review on #398: - P1: inject_guest_boot no longer follows a symlink at bb-init/bb-dropbear. A committed snapshot is guest-controlled and could plant those paths as symlinks aimed at a host file (e.g. bb-init -> ~/.bashrc); write_text / copy2 would then overwrite the target as the host user during resume. Replace any pre-existing entry and create the files with O_EXCL|O_NOFOLLOW so the write stays inside the staging tree. - P2: write the snapshot tar owner-only (0600). It can contain the bottle's private workspace; it was being created world-readable (0644). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ --- bot_bottle/backend/firecracker/freezer.py | 7 ++- bot_bottle/backend/firecracker/util.py | 34 +++++++++++--- tests/unit/test_firecracker_helpers.py | 55 +++++++++++++++++++++++ 3 files changed, 89 insertions(+), 7 deletions(-) diff --git a/bot_bottle/backend/firecracker/freezer.py b/bot_bottle/backend/firecracker/freezer.py index e1f3735..55f0a1c 100644 --- a/bot_bottle/backend/firecracker/freezer.py +++ b/bot_bottle/backend/firecracker/freezer.py @@ -63,7 +63,11 @@ def _commit_rootfs_via_ssh(private_key: Path, guest_ip: str, tar_path: Path) -> tar_path.parent.mkdir(parents=True, exist_ok=True) partial = tar_path.with_name(tar_path.name + ".partial") ssh = util.ssh_base_argv(private_key, guest_ip) - with open(partial, "wb") as tar_out: + # The snapshot can contain the bottle's private workspace, so keep it + # owner-only (0600) — create it that way and re-assert after the rename + # (os.replace carries the source mode, but be explicit). + fd = os.open(partial, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, "wb") as tar_out: result = subprocess.run( [*ssh, "--", "tar", "--create", "--one-file-system", "--exclude=./proc", "--exclude=./sys", "--exclude=./dev", @@ -75,3 +79,4 @@ def _commit_rootfs_via_ssh(private_key: Path, guest_ip: str, tar_path: Path) -> die(f"ssh tar for {guest_ip} failed: " f"{(result.stderr or b'').decode().strip() or ''}") os.replace(partial, tar_path) + os.chmod(tar_path, 0o600) diff --git a/bot_bottle/backend/firecracker/util.py b/bot_bottle/backend/firecracker/util.py index 4421714..425addd 100644 --- a/bot_bottle/backend/firecracker/util.py +++ b/bot_bottle/backend/firecracker/util.py @@ -259,12 +259,34 @@ def build_committed_rootfs_dir(tar_path: Path) -> Path: def inject_guest_boot(rootfs: Path, init_script: str | None = None) -> None: """Drop the static dropbear and the PID-1 init into the rootfs. `init_script` defaults to the SSH-only agent init; the infra VM - passes its own (control plane + gateway) init.""" - shutil.copy2(dropbear_path(), rootfs / "bb-dropbear") - os.chmod(rootfs / "bb-dropbear", 0o755) - init = rootfs / "bb-init" - init.write_text(init_script or _GUEST_INIT) - os.chmod(init, 0o755) + passes its own (control plane + gateway) init. + + A committed snapshot is guest-controlled, so `bb-dropbear`/`bb-init` + may already exist as symlinks aimed at a host file (e.g. bb-init -> + ~/.bashrc). Replace whatever is there and create the files with + O_EXCL|O_NOFOLLOW so the write always lands a fresh regular file in + the staging tree and never follows a planted symlink out of it.""" + _write_staged_file(rootfs / "bb-dropbear", dropbear_path().read_bytes()) + _write_staged_file(rootfs / "bb-init", (init_script or _GUEST_INIT).encode()) + + +def _write_staged_file(path: Path, data: bytes) -> None: + """Write `data` to `path` (mode 0755) as a fresh regular file inside a + staging rootfs, replacing any pre-existing entry without following a + symlink at `path`. Fails closed on anything unexpected there.""" + if path.is_symlink() or path.exists(): + if path.is_dir() and not path.is_symlink(): + shutil.rmtree(path) + else: + path.unlink() + fd = os.open( + path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW, 0o755 + ) + try: + os.write(fd, data) + finally: + os.close(fd) + os.chmod(path, 0o755) def build_rootfs_ext4(base_dir: Path, out_path: Path, *, slack_mib: int = 1024) -> None: diff --git a/tests/unit/test_firecracker_helpers.py b/tests/unit/test_firecracker_helpers.py index 76f97d8..85113d1 100644 --- a/tests/unit/test_firecracker_helpers.py +++ b/tests/unit/test_firecracker_helpers.py @@ -7,6 +7,7 @@ from __future__ import annotations import json import os +import stat import subprocess import tempfile import unittest @@ -218,5 +219,59 @@ class TestBuildCommittedRootfsDir(unittest.TestCase): self.assertEqual(2, calls["n"]) +class TestInjectGuestBootSymlinkSafe(unittest.TestCase): + """A committed snapshot is guest-controlled: inject_guest_boot must not + follow a planted symlink and overwrite a host file during resume.""" + + def test_planted_symlink_does_not_escape_staging_tree(self): + with tempfile.TemporaryDirectory(prefix="fc-inject.") as d: + tmp = Path(d) + dropbear = tmp / "dropbear" + dropbear.write_bytes(b"DROPBEAR") + # A host file the malicious snapshot tries to clobber. + victim = tmp / "victim" + victim.write_text("original") + + rootfs = tmp / "rootfs" + rootfs.mkdir() + # The snapshot planted bb-init/bb-dropbear as symlinks to it. + (rootfs / "bb-init").symlink_to(victim) + (rootfs / "bb-dropbear").symlink_to(victim) + + with patch.object(util, "dropbear_path", return_value=dropbear): + util.inject_guest_boot(rootfs, init_script="#!/bin/sh\nreal\n") + + # Host file untouched; the staged paths are fresh regular files. + self.assertEqual("original", victim.read_text()) + self.assertFalse((rootfs / "bb-init").is_symlink()) + self.assertFalse((rootfs / "bb-dropbear").is_symlink()) + self.assertEqual("#!/bin/sh\nreal\n", (rootfs / "bb-init").read_text()) + self.assertEqual(b"DROPBEAR", (rootfs / "bb-dropbear").read_bytes()) + + +class TestCommitRootfsPermissions(unittest.TestCase): + """The snapshot tar can hold the bottle's private workspace, so the + freezer must write it owner-only (0600).""" + + def test_snapshot_created_owner_only(self): + with tempfile.TemporaryDirectory(prefix="fc-freeze.") as d: + tmp = Path(d) + key = tmp / "key" + key.write_text("K") + tar_path = tmp / "state" / "committed-rootfs.tar" + + def fake_run(argv: list[str], *a: Any, **k: Any) -> Any: + # Emulate the ssh|tar pipe streaming the snapshot to stdout. + k["stdout"].write(b"TARDATA") + return subprocess.CompletedProcess(argv, 0, b"", b"") + + with patch.object(freezer.util, "ssh_base_argv", return_value=["ssh"]), \ + patch.object(freezer.subprocess, "run", side_effect=fake_run): + freezer._commit_rootfs_via_ssh(key, "10.0.0.1", tar_path) + + self.assertEqual(b"TARDATA", tar_path.read_bytes()) + self.assertEqual(0o600, stat.S_IMODE(tar_path.stat().st_mode)) + + if __name__ == "__main__": unittest.main() -- 2.52.0 From dfc693e0b62c5679134ff35b02b7e7a5c61ac2b1 Mon Sep 17 00:00:00 2001 From: didericis Date: Fri, 17 Jul 2026 01:02:19 -0400 Subject: [PATCH 4/4] fix(firecracker): keep the snapshot partial private even if one was left behind Follow-up to the codex review on #398. os.open's mode arg only applies on creation, so a committed-rootfs.tar.partial left 0644 by an interrupted run would be opened/truncated (not re-moded) and stay world-readable for the whole SSH stream. Unlink any leftover and exclusively recreate it (O_EXCL|O_NOFOLLOW), then fchmod 0600 immediately so umask can't loosen it. Test pre-creates a 0644 partial and asserts the fd is 0600 mid-stream. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ --- bot_bottle/backend/firecracker/freezer.py | 14 +++++-- tests/unit/test_firecracker_helpers.py | 47 +++++++++++++++++------ 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/bot_bottle/backend/firecracker/freezer.py b/bot_bottle/backend/firecracker/freezer.py index 55f0a1c..4ffbaed 100644 --- a/bot_bottle/backend/firecracker/freezer.py +++ b/bot_bottle/backend/firecracker/freezer.py @@ -64,9 +64,17 @@ def _commit_rootfs_via_ssh(private_key: Path, guest_ip: str, tar_path: Path) -> partial = tar_path.with_name(tar_path.name + ".partial") ssh = util.ssh_base_argv(private_key, guest_ip) # The snapshot can contain the bottle's private workspace, so keep it - # owner-only (0600) — create it that way and re-assert after the rename - # (os.replace carries the source mode, but be explicit). - fd = os.open(partial, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + # owner-only (0600) for the whole stream. The `os.open` mode only applies + # on *creation*, so unlink any leftover partial (a prior interrupted run + # could have left it world-readable, or something could swap in a symlink + # at this predictable name) and exclusively recreate it — O_EXCL|O_NOFOLLOW + # — then fchmod immediately so umask can't loosen it. Re-assert after the + # rename too (os.replace carries the source mode, but be explicit). + partial.unlink(missing_ok=True) + fd = os.open( + partial, os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW, 0o600 + ) + os.fchmod(fd, 0o600) with os.fdopen(fd, "wb") as tar_out: result = subprocess.run( [*ssh, "--", "tar", "--create", "--one-file-system", diff --git a/tests/unit/test_firecracker_helpers.py b/tests/unit/test_firecracker_helpers.py index 85113d1..a40f41e 100644 --- a/tests/unit/test_firecracker_helpers.py +++ b/tests/unit/test_firecracker_helpers.py @@ -253,22 +253,47 @@ class TestCommitRootfsPermissions(unittest.TestCase): """The snapshot tar can hold the bottle's private workspace, so the freezer must write it owner-only (0600).""" + def _commit(self, tar_path: Path) -> int: + """Run _commit_rootfs_via_ssh with a stubbed ssh|tar pipe; return the + mode of the open partial observed mid-stream (from subprocess.run).""" + key = tar_path.parent.parent / "key" + key.write_text("K") + seen: dict[str, int] = {} + + def fake_run(argv: list[str], *a: Any, **k: Any) -> Any: + out = k["stdout"] + seen["mode"] = stat.S_IMODE(os.fstat(out.fileno()).st_mode) + out.write(b"TARDATA") + return subprocess.CompletedProcess(argv, 0, b"", b"") + + with patch.object(freezer.util, "ssh_base_argv", return_value=["ssh"]), \ + patch.object(freezer.subprocess, "run", side_effect=fake_run): + freezer._commit_rootfs_via_ssh(key, "10.0.0.1", tar_path) + return seen["mode"] + def test_snapshot_created_owner_only(self): with tempfile.TemporaryDirectory(prefix="fc-freeze.") as d: - tmp = Path(d) - key = tmp / "key" - key.write_text("K") - tar_path = tmp / "state" / "committed-rootfs.tar" + tar_path = Path(d) / "state" / "committed-rootfs.tar" + tar_path.parent.mkdir() + stream_mode = self._commit(tar_path) - def fake_run(argv: list[str], *a: Any, **k: Any) -> Any: - # Emulate the ssh|tar pipe streaming the snapshot to stdout. - k["stdout"].write(b"TARDATA") - return subprocess.CompletedProcess(argv, 0, b"", b"") + self.assertEqual(0o600, stream_mode) # private during the stream + self.assertEqual(b"TARDATA", tar_path.read_bytes()) + self.assertEqual(0o600, stat.S_IMODE(tar_path.stat().st_mode)) - with patch.object(freezer.util, "ssh_base_argv", return_value=["ssh"]), \ - patch.object(freezer.subprocess, "run", side_effect=fake_run): - freezer._commit_rootfs_via_ssh(key, "10.0.0.1", tar_path) + def test_leftover_world_readable_partial_is_recreated_private(self): + """A partial left 0644 by an interrupted prior run must not keep the + new snapshot world-readable while it streams.""" + with tempfile.TemporaryDirectory(prefix="fc-freeze.") as d: + tar_path = Path(d) / "state" / "committed-rootfs.tar" + tar_path.parent.mkdir() + partial = tar_path.with_name(tar_path.name + ".partial") + partial.write_bytes(b"stale") + os.chmod(partial, 0o644) + stream_mode = self._commit(tar_path) + + self.assertEqual(0o600, stream_mode) self.assertEqual(b"TARDATA", tar_path.read_bytes()) self.assertEqual(0o600, stat.S_IMODE(tar_path.stat().st_mode)) -- 2.52.0