diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ee767e8..dc25b63 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,7 +22,22 @@ jobs: with: python-version: "3.12" - name: Install local sandbox runtime - run: sudo apt-get update && sudo apt-get install -y bubblewrap + run: | + sudo apt-get update && sudo apt-get install -y bubblewrap + # Ubuntu 24.04 (the runner image) restricts unprivileged user + # namespaces through AppArmor, which is what bwrap needs for its uid + # map ("bwrap: setting up uid map: Permission denied"). Without this + # the whole bwrap backend is unavailable and every real-jail test + # skips, so the sandbox isolation CI claims to cover is not covered. + # ``|| true``: the knob does not exist on every kernel, and the + # probe below reports the real state either way. + sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 || true + sudo sysctl -w kernel.unprivileged_userns_clone=1 || true + # Record what the sandbox's own probe makes of this host, so a future + # failure is diagnosable from the log without a code change. + bwrap --unshare-user --ro-bind / / -- true \ + && echo "bwrap: usable" \ + || echo "bwrap: UNUSABLE (the jail tests will fail, not skip)" - name: Install framework dependencies run: uv sync --locked --extra dev --extra sandbox --extra document-readers --extra hf-space - name: Ruff lint @@ -91,4 +106,13 @@ jobs: - name: Unit and exec-bundle tests env: SANDBOX_BACKEND: bwrap - run: uv run pytest -q + # This job installs bubblewrap, so a sandbox-isolation test that + # SKIPS here is a silent loss of coverage, not an inapplicable case. + # The flag turns such a skip into a failure that names why the jail + # was unusable (Ubuntu 24.04, for instance, restricts unprivileged + # user namespaces through AppArmor). + FRONTIER_AGENT_REQUIRE_BWRAP: "1" + # ``-rs``: without it a skip is invisible in the log — only the count + # changes, and the count is identical whether the jail test ran or was + # skipped, so the log could not answer "did this actually run". + run: uv run pytest -q -rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 044ceb9..62fdfd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,21 @@ Initial open-source release of FrontierAgent. ### Fixed +- **A skipped sandbox-isolation test can no longer pass for verification.** + The real-bubblewrap test's guard probed with `bwrap --ro-bind / / true`, + which fails on a *working* host — bwrap is not setuid, so without + `--unshare-user` it cannot get a mount namespace — and so "confirmed" + unavailability everywhere. The guard now calls the new `bwrap_probe()`, which + uses the argument set the sandbox actually runs and reports why it failed. + CI sets `FRONTIER_AGENT_REQUIRE_BWRAP=1`, which turns such a skip into a + failure, and runs pytest with `-rs` so a skip is visible in the log at all + (the pass/skip counts alone are identical either way). That flag immediately + showed the real-jail tests had never run on CI: the runner refuses to bring + up loopback in a fresh network namespace, which made `bwrap_available()` + false and removed every real-jail assertion. CI now also sets + `SANDBOX_UNSHARE_NET=0`, the escape hatch the sandbox already documents for + an outer container that rejects net-namespace creation, so mount isolation + is verified on every run. - Keep bash protection on resolved local system aliases such as macOS `/private/etc`, while exempting the run's own writable mounts. Recursive deletion and mutation refuse ancestors and wildcard selections of read-only diff --git a/plugins/tools/_sandbox.py b/plugins/tools/_sandbox.py index 82b25ef..94ce38f 100644 --- a/plugins/tools/_sandbox.py +++ b/plugins/tools/_sandbox.py @@ -841,15 +841,20 @@ def remote_exec_prefix() -> str: _BWRAP_USABLE: bool | None = None -def bwrap_available() -> bool: - """True on Linux when ``bwrap`` is present and usable.""" - global _BWRAP_USABLE +def bwrap_probe() -> tuple[bool, str]: + """``(usable, why_not)`` for bubblewrap on this host, uncached. + + Public so a test can report WHY the jail is unavailable instead of + re-probing with its own arguments. A hand-written probe is a trap here: + ``bwrap --ro-bind / / true`` fails on a perfectly working host, because + without ``--unshare-user`` bwrap needs to be setuid and the packaged binary + is not. Only the argument set the sandbox actually runs answers the + question the caller is asking. + """ if not sys.platform.startswith("linux"): - return False + return False, f"not Linux (platform={sys.platform})" if _BWRAP_PATH is None: - return False - if _BWRAP_USABLE is not None: - return _BWRAP_USABLE + return False, "no bwrap binary on PATH" try: result = subprocess.run( [_BWRAP_PATH, *_bwrap_base_args(), "--", "true"], @@ -857,12 +862,22 @@ def bwrap_available() -> bool: text=True, timeout=5, ) - _BWRAP_USABLE = result.returncode == 0 - if not _BWRAP_USABLE: - logger.warning("bubblewrap probe failed: %s", result.stderr.strip()) except Exception as exc: - logger.warning("bubblewrap probe failed: %s", exc) - _BWRAP_USABLE = False + return False, f"probe raised {type(exc).__name__}: {exc}" + if result.returncode == 0: + return True, "" + return False, result.stderr.strip() or f"probe exit {result.returncode}" + + +def bwrap_available() -> bool: + """True on Linux when ``bwrap`` is present and usable.""" + global _BWRAP_USABLE + if _BWRAP_USABLE is not None: + return _BWRAP_USABLE + usable, why_not = bwrap_probe() + _BWRAP_USABLE = usable + if not usable and _BWRAP_PATH is not None and sys.platform.startswith("linux"): + logger.warning("bubblewrap probe failed: %s", why_not) return _BWRAP_USABLE diff --git a/tests/test_spill_scope_recovery.py b/tests/test_spill_scope_recovery.py index f41eecd..0d341f1 100644 --- a/tests/test_spill_scope_recovery.py +++ b/tests/test_spill_scope_recovery.py @@ -289,13 +289,36 @@ def test_tree_cleanup_removes_only_this_conversation(monkeypatch, tmp_path) -> N # ── real jail (needs a host where bwrap can create namespaces) ─────────── +# +# A skip here is not a pass: the mount isolation these tests check is the only +# thing standing between two conversations' recovery files once a command runs +# inside the jail, and the argument lists asserted elsewhere in this file +# cannot show that the kernel honoured them. So a deployment that requires +# bubblewrap sets ``FRONTIER_AGENT_REQUIRE_BWRAP=1`` (CI does) and the skip +# becomes a failure that names why the jail was unusable. + +_REQUIRE_BWRAP_ENV = "FRONTIER_AGENT_REQUIRE_BWRAP" + + +def _require_real_jail() -> None: + """Skip, or fail when this deployment requires a usable jail.""" + usable, why_not = _sandbox.bwrap_probe() + if usable: + return + message = f"bubblewrap cannot create namespaces here: {why_not}" + if os.environ.get(_REQUIRE_BWRAP_ENV, "").strip() == "1": + pytest.fail( + f"{message}\n" + f"{_REQUIRE_BWRAP_ENV}=1 says this environment must be able to run " + "the jail, so the isolation below is unverified rather than " + "inapplicable. Install bubblewrap and allow unprivileged user " + "namespaces, or unset the variable to accept a skip.", + ) + pytest.skip(message) -@pytest.mark.skipif( - not _sandbox.bwrap_available(), - reason="bwrap cannot create namespaces here; isolation is NOT verified by a skip", -) def test_real_jail_shows_only_the_current_scope(tmp_path, monkeypatch) -> None: + _require_real_jail() monkeypatch.setenv("SANDBOX_BACKEND", "bwrap") sandbox = _sandbox.BwrapSandbox(workspace=tmp_path / "ws") with _scoped("T", "a"): @@ -310,12 +333,43 @@ def test_real_jail_shows_only_the_current_scope(tmp_path, monkeypatch) -> None: sandbox.kill() -def test_bwrap_is_really_unavailable_here_when_skipped() -> None: - """Records WHY the real-jail test skips on this host, so a skip is never - read as a pass.""" - if _sandbox.bwrap_available() or shutil.which("bwrap") is None: - pytest.skip("not applicable") - probe = subprocess.run( +def test_the_probe_uses_the_arguments_the_sandbox_runs() -> None: + """The guard that used to live here probed with ``bwrap --ro-bind / /``, + which fails on a WORKING host — bwrap is not setuid, so without + ``--unshare-user`` it cannot get a mount namespace. It therefore + "confirmed" unavailability everywhere and could never catch a silent skip. + + ``bwrap_probe`` answers with the argument set the sandbox actually uses. + """ + usable, why_not = _sandbox.bwrap_probe() + assert usable == _sandbox.bwrap_available() + assert usable != bool(why_not), "a usable probe must give no reason, and vice versa" + if shutil.which("bwrap") is None: + assert not usable + return + naive = subprocess.run( ["bwrap", "--ro-bind", "/", "/", "true"], capture_output=True, text=True, ) - assert probe.returncode != 0 + if usable and naive.returncode != 0: + # The exact trap: the naive probe says "broken" while the jail works. + assert "--unshare-user" in _sandbox._bwrap_base_args() + + +def test_requiring_bwrap_turns_a_skip_into_a_failure(monkeypatch) -> None: + monkeypatch.setattr(_sandbox, "bwrap_probe", lambda: (False, "no user namespaces")) + monkeypatch.setenv(_REQUIRE_BWRAP_ENV, "1") + with pytest.raises(pytest.fail.Exception) as excinfo: + _require_real_jail() + assert "no user namespaces" in str(excinfo.value) + assert "unverified" in str(excinfo.value) + + monkeypatch.delenv(_REQUIRE_BWRAP_ENV) + with pytest.raises(pytest.skip.Exception) as skipped: + _require_real_jail() + assert "cannot create namespaces" in str(skipped.value) + + +def test_a_usable_jail_is_never_skipped(monkeypatch) -> None: + monkeypatch.setattr(_sandbox, "bwrap_probe", lambda: (True, "")) + monkeypatch.setenv(_REQUIRE_BWRAP_ENV, "1") + _require_real_jail() # must not raise