From dfc693e0b62c5679134ff35b02b7e7a5c61ac2b1 Mon Sep 17 00:00:00 2001 From: didericis Date: Fri, 17 Jul 2026 01:02:19 -0400 Subject: [PATCH] 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))