From d767896bd6912d1ed2f4e752c4752f021dba3981 Mon Sep 17 00:00:00 2001 From: James Wong <2421248+jameswnl@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:20:53 -0400 Subject: [PATCH 1/3] Fix OpenShellSpawner: send baseline Landlock filesystem policy for non-advisory spawns Every non-advisory (spawn: ephemeral) run previously sent an empty filesystem policy to the OpenShell gateway, causing it to fall back to its own hardcoded default allowlist -- which never includes /opt/app-root or /opt/lightspeed, where this sandbox image's Python packages and app code live. Landlock then denied the sandbox's own process access to its own dependencies (e.g. uvicorn), failing every non-advisory ephemeral run on real OpenShift clusters. `oc exec` bypasses the gateway's Landlock enforcement entirely, so this was invisible to that debugging path (issue #189). - Add `_build_baseline_filesystem_policy()`: sends OpenShell's full default read_only allowlist UNION a configurable `extra_readable_paths` constructor arg (default /opt/app-root, /opt/lightspeed), plus default read_write and include_workdir. OpenShell replaces rather than merges a supplied policy, so the full default list must always be sent alongside the extras. - Wire it into `_do_spawn`'s existing `if read_only` branch as the `else` case, so baseline and the existing advisory `_build_filesystem_policy()` (unchanged) are mutually exclusive. - Validate `extra_readable_paths`: reject empty strings, relative paths, and `..` segments. - Add `extra_readable_paths` to factory.py's `_OPENSHELL_EXTRA_PARAMS` allowlist so `build_spawner()` doesn't silently drop it. - Set Landlock `compatibility: best_effort` explicitly on the baseline policy. Unit tests assert policy shape but can't catch the real bug (empty policy field handling is gateway-side). Added a real e2e test suite (`tests/e2e/test_guardrails.py::TestOpenShellGuardrails`) against a live Kind + OpenShell gateway with the real reference sandbox image. Discovered the current image build installs packages under /usr/local (already allowlisted), so also added a mechanism-level test using `/home` (confirmed genuinely Landlock-denied) to prove `extra_readable_paths` composition reaches the real gateway -- manually verified RED before the fix and GREEN after, against the live gateway. Co-Authored-By: Claude Sonnet 5 --- docs/gaps/gaps-implementation-plan.md | 4 +- src/cloud_agents/spawner/factory.py | 9 +- src/cloud_agents/spawner/openshell_spawner.py | 112 ++++++++ tests/e2e/test_guardrails.py | 169 +++++++++++- tests/unit/spawner/test_factory.py | 37 +++ tests/unit/spawner/test_openshell_spawner.py | 247 ++++++++++++++++++ 6 files changed, 571 insertions(+), 7 deletions(-) diff --git a/docs/gaps/gaps-implementation-plan.md b/docs/gaps/gaps-implementation-plan.md index 96585606..298b1c02 100644 --- a/docs/gaps/gaps-implementation-plan.md +++ b/docs/gaps/gaps-implementation-plan.md @@ -793,11 +793,11 @@ PoC1 leftover. In the Temporal architecture, the activity calls the sandbox sync **Spike findings**: OpenShellSpawner prototype implements AgentSpawner ABC via OpenShell gRPC API. Single `_do_spawn` replaces both K8s and Podman paths. Gateway handles sandbox lifecycle, network isolation, and credential management. Service exposure via `ExposeService` RPC preserves the `POST /v1/agent/run` contract. Full findings: `docs/spikes/openshell-spawner-spike.md`. **What to build** (if spike -> go): -1. Production-harden the `OpenShellSpawner` prototype — SDK migrated (PRs #97–#100), parallel-safe readiness, auto-cleanup on failure ✅ +1. Production-harden the `OpenShellSpawner` prototype — SDK migrated (PRs #97–#100), parallel-safe readiness, auto-cleanup on failure, non-advisory Landlock baseline filesystem policy so sandboxes can read their own image contents ([issue #189](https://github.com/jameswnl/lightspeed-cloud-agents/issues/189)) ✅ 2. Add skills_image support (init container equivalent via OpenShell exec) 3. Add credential provider integration (replace K8s Secret env vars) 4. L7 network policy configuration via SandboxPolicy — auto-derived from provider + MCP config via `_build_network_policy()` (PR #102) ✅ -5. Integration tests with a running OpenShell gateway +5. Integration tests with a running OpenShell gateway — `tests/e2e/test_guardrails.py::TestOpenShellGuardrails` against a real Kind + podman-driver gateway ([issue #189](https://github.com/jameswnl/lightspeed-cloud-agents/issues/189)) ✅ 6. Deployment guide: gateway setup for K8s and Podman modes **Effort**: 2-3 weeks (production), 1 week (spike done) diff --git a/src/cloud_agents/spawner/factory.py b/src/cloud_agents/spawner/factory.py index 9e1accdd..4c9bdbdb 100644 --- a/src/cloud_agents/spawner/factory.py +++ b/src/cloud_agents/spawner/factory.py @@ -42,7 +42,7 @@ } ) _PODMAN_PARAMS = frozenset({"network", "volume_mounts", "max_pods"}) -_OPENSHELL_EXTRA_PARAMS = frozenset({"max_pods"}) +_OPENSHELL_EXTRA_PARAMS = frozenset({"max_pods", "extra_readable_paths"}) def _filtered(params: dict[str, Any], known: frozenset[str]) -> dict[str, Any]: @@ -65,9 +65,10 @@ def build_spawner(spawner_type: str, **params: Any) -> "AgentSpawner": tools_configmap, secret_env_vars, projected_sa_token, max_pods. For "podman": network, volume_mounts, max_pods. For "openshell": gateway_url, driver, workspace, http_endpoint, - tls_ca, tls_cert, tls_key, bearer_token, max_pods -- used to - build the underlying SandboxClient (with TLS/bearer auth) - and then OpenShellSpawner. + tls_ca, tls_cert, tls_key, bearer_token, max_pods, + extra_readable_paths -- used to build the underlying + SandboxClient (with TLS/bearer auth) and then + OpenShellSpawner. Callers may pass a broader dict (e.g. a Pydantic model_dump()) containing extra keys -- unrecognized keys and explicit None values are dropped rather than forwarded, so passing an unset diff --git a/src/cloud_agents/spawner/openshell_spawner.py b/src/cloud_agents/spawner/openshell_spawner.py index 40451d29..4baeed48 100644 --- a/src/cloud_agents/spawner/openshell_spawner.py +++ b/src/cloud_agents/spawner/openshell_spawner.py @@ -69,6 +69,66 @@ class OpenShellSpawner(AgentSpawner): for the exec'd server process. """ + # Extra read-only paths granted on top of OpenShell's own hardcoded + # default allowlist for every non-advisory spawn (see + # _build_baseline_filesystem_policy()). These are where the reference + # sandbox image (quay.io/jameswong/lightspeed-agentic-sandbox) installs + # Python packages and application code -- neither is in OpenShell's + # restrictive_default_policy(), so without this every non-advisory + # spawn fails with Landlock EACCES on its own dependencies (issue #189). + _DEFAULT_EXTRA_READABLE_PATHS: ClassVar[list[str]] = ["/opt/app-root", "/opt/lightspeed"] + + # OpenShell's own hardcoded restrictive_default_policy() (Rust side, + # crates/openshell-sandbox/src/lib.rs). Mirrored here because OpenShell + # does NOT merge a supplied filesystem policy with its own default -- + # a supplied policy fully *replaces* it. So the baseline builder below + # must always send this full list, union'd with the extra paths, never + # just the extras alone -- see issue #189. + _DEFAULT_BASELINE_READ_ONLY: ClassVar[list[str]] = [ + "/usr", + "/lib", + "/proc", + "/dev/urandom", + "/app", + "/etc", + "/var/log", + ] + _DEFAULT_BASELINE_READ_WRITE: ClassVar[list[str]] = ["/tmp", "/dev/null"] + + @staticmethod + def _validate_extra_readable_paths(paths: list[str]) -> list[str]: + """Validate extra_readable_paths before they reach a Landlock policy. + + Extra readable paths *widen* the sandbox's filesystem access, so + malformed input here is a security concern, not just a usability + one. Rejects relative paths (meaningless to Landlock, which rules + operate on absolute filesystem paths), ".." segments (could escape + the intended directory), and empty strings. + + Args: + paths: Candidate list of absolute directory paths. + + Returns: + A new list with the same entries, if all are valid. A copy is + returned (rather than the input list itself) so a caller + mutating the list they passed in can't retroactively change + the spawner's stored policy. + + Raises: + ValueError: If any path is empty, relative, or contains a + ".." segment. + """ + for path in paths: + if not path: + raise ValueError("extra_readable_paths entries must not be empty") + if not path.startswith("/"): + raise ValueError(f"extra_readable_paths entries must be absolute paths: {path!r}") + if ".." in path.split("/"): + raise ValueError( + f"extra_readable_paths entries must not contain '..' segments: {path!r}" + ) + return list(paths) + def __init__( self, openshell_client: Any = None, @@ -80,6 +140,7 @@ def __init__( tls_cert: str = "", tls_key: str = "", bearer_token: str = "", + extra_readable_paths: list[str] | None = None, **kwargs: Any, ) -> None: """Initialize the OpenShell spawner. @@ -107,6 +168,14 @@ def __init__( tls_key: Path to client key for mTLS. bearer_token: OIDC bearer token for gRPC auth. Applied to raw gRPC channels via call credentials interceptor. + extra_readable_paths: Additional absolute paths to grant + read-only Landlock access to, on top of OpenShell's own + default allowlist, for every non-advisory spawn. Defaults + to ["/opt/app-root", "/opt/lightspeed"] -- where the + reference sandbox image installs Python packages and + application code. A derived image with a different + layout should override this. See + _build_baseline_filesystem_policy() and issue #189. """ super().__init__(**kwargs) self._client = openshell_client @@ -118,6 +187,11 @@ def __init__( self._tls_cert = tls_cert self._tls_key = tls_key self._bearer_token = bearer_token + self._extra_readable_paths = self._validate_extra_readable_paths( + extra_readable_paths + if extra_readable_paths is not None + else list(self._DEFAULT_EXTRA_READABLE_PATHS) + ) self._podman_cli: str | None = None self._sandbox_names: dict[str, str] = {} self._sandbox_ids: dict[str, str] = {} @@ -444,6 +518,8 @@ async def _do_spawn( if read_only: self._build_filesystem_policy(spec) + else: + self._build_baseline_filesystem_policy(spec) # Skills image: Podman driver mounts the OCI image directly # via driver_config (no extraction needed). K8s driver falls @@ -588,6 +664,42 @@ def _build_filesystem_policy(spec: "openshell_pb2.SandboxSpec") -> None: spec.policy.filesystem.read_write.append(rw_path) spec.policy.filesystem.include_workdir = True + def _build_baseline_filesystem_policy(self, spec: "openshell_pb2.SandboxSpec") -> None: + """Set the baseline filesystem policy sent for every non-advisory spawn. + + Without this, a non-advisory spawn sends an empty + `spec.policy.filesystem`, and OpenShell's gateway falls back to + its own hardcoded `restrictive_default_policy()` -- which never + includes /opt/app-root or /opt/lightspeed, where this sandbox + image's Python packages and application code live. That causes + Landlock to deny the sandbox's own process access to its own + dependencies (e.g. uvicorn), failing every non-advisory ephemeral + run on real OpenShift clusters (issue #189). + + This is additive, not a replacement for _build_filesystem_policy() + (the advisory/full-lockdown path) -- the two are mutually + exclusive per spawn and this method must never run alongside it. + + Critically, OpenShell does NOT merge a supplied filesystem policy + with its own default -- a supplied policy fully *replaces* it. So + this sends the complete default read_only/read_write allowlist + (mirrored in _DEFAULT_BASELINE_READ_ONLY/_DEFAULT_BASELINE_READ_WRITE) + union'd with self._extra_readable_paths, never just the extra + paths alone -- sending only the extras would drop /usr, /lib, + /proc, /etc, and /tmp, breaking things worse than the bug this + fixes. + """ + for path in self._DEFAULT_BASELINE_READ_ONLY: + spec.policy.filesystem.read_only.append(path) + for path in self._extra_readable_paths: + spec.policy.filesystem.read_only.append(path) + for path in self._DEFAULT_BASELINE_READ_WRITE: + spec.policy.filesystem.read_write.append(path) + spec.policy.filesystem.include_workdir = True + # Already the proto default, but set explicitly to match the + # live-verified fix YAML in issue #189 exactly. + spec.policy.landlock.compatibility = "best_effort" + async def _inject_credentials( self, agent_name: str, diff --git a/tests/e2e/test_guardrails.py b/tests/e2e/test_guardrails.py index 7c1d17b2..b3a72230 100644 --- a/tests/e2e/test_guardrails.py +++ b/tests/e2e/test_guardrails.py @@ -1,4 +1,4 @@ -"""E2E guardrails test for both Podman and Kubernetes. +"""E2E guardrails test for Podman, Kubernetes, and OpenShell. Verifies that security guardrails are enforced on real containers: - securityContext (non-root, read-only root fs) @@ -7,16 +7,21 @@ - orphan reconciliation cleans up on startup - advisory mode sets read-only filesystem - spawned-by label present for crash recovery +- OpenShell non-advisory spawns can read their own image contents + (Landlock filesystem policy -- issue #189) Prerequisites: - podman running with socket accessible - lightspeed-agentic-sandbox:temporal image built - For Kind tests: Kind cluster running with images loaded + - For OpenShell tests: a reachable OpenShell gateway (see + OPENSHELL_GATEWAY_URL / OPENSHELL_SANDBOX_IMAGE below) Usage: uv run pytest tests/e2e/test_guardrails.py -v uv run pytest tests/e2e/test_guardrails.py -v -k podman uv run pytest tests/e2e/test_guardrails.py -v -k kind + uv run pytest tests/e2e/test_guardrails.py -v -k openshell """ from __future__ import annotations @@ -24,7 +29,9 @@ import asyncio import os import sys +from typing import Any +import httpx import pytest sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "src")) @@ -240,3 +247,163 @@ async def test_spawned_job_has_tmp_tmpfs(self, spawner) -> None: assert tmp_vols[0].empty_dir.medium == "Memory" finally: await spawner.destroy(name) + + +OPENSHELL_GATEWAY_URL = os.environ.get("OPENSHELL_GATEWAY_URL", "localhost:9080") +OPENSHELL_SANDBOX_IMAGE = os.environ.get( + "OPENSHELL_SANDBOX_IMAGE", "quay.io/jameswong/lightspeed-agentic-sandbox:latest" +) + + +async def _exec(client: Any, sandbox_id: str, command: list[str]) -> Any: + """Run a blocking exec_stream() call off the event loop and return the ExecResult. + + OpenShell's SandboxClient.exec_stream() is a synchronous generator, so + it must be driven from a worker thread (same pattern OpenShellSpawner + itself uses internally for start_server()/_consume_exec()). + """ + + def _sync() -> Any: + result = None + for item in client.exec_stream(sandbox_id, command): + if hasattr(item, "exit_code"): + result = item + return result + + return await asyncio.to_thread(_sync) + + +class TestOpenShellGuardrails: + """E2E guardrail test for OpenShellSpawner against a REAL gateway (issue #189). + + Regression coverage for the Landlock filesystem-policy gap: unit tests + can only assert the constructed policy's *shape* (see + tests/unit/spawner/test_openshell_spawner.py::TestBaselineFilesystemPolicy) + -- they cannot catch the actual bug, which is about what OpenShell's + real gateway/supervisor does with an empty filesystem-policy field, + something no mock reproduces. This class is the real gate. + + A bare `exec(["echo", "hello"])` would pass today without ever + touching a Landlock-restricted path, and would not catch this bug -- + so these tests deliberately probe real filesystem access instead. + """ + + @pytest.fixture(autouse=True) + def _skip_if_no_gateway(self) -> None: + """Skip (rather than fail) when no real OpenShell gateway is reachable.""" + pytest.importorskip("openshell") + try: + httpx.get(f"http://{OPENSHELL_GATEWAY_URL}/", timeout=3.0) + except httpx.HTTPError: + pytest.skip(f"No accessible OpenShell gateway at {OPENSHELL_GATEWAY_URL}") + + @pytest.fixture + def spawner(self): + """Build a real OpenShellSpawner against the configured gateway.""" + from cloud_agents.spawner.factory import build_spawner + + return build_spawner( + "openshell", + gateway_url=OPENSHELL_GATEWAY_URL, + driver="podman", + workspace="default", + ) + + @pytest.mark.asyncio + async def test_non_advisory_spawn_can_import_its_own_dependencies(self, spawner) -> None: + """A normal (non-advisory) ephemeral spawn must be able to read its own image. + + The reference sandbox image installs its Python packages under + /usr/local (already in OpenShell's own hardcoded default + allowlist), so this test alone does not reproduce the exact + /opt/app-root path from issue #189 against the currently + published image tag -- see + test_extra_readable_paths_widens_landlock_access_on_real_gateway + below for the test that exercises the actual policy-composition + mechanism the fix depends on. This test is still valuable as a + real-world integration check: it proves the new baseline + filesystem policy doesn't regress normal, non-advisory server + startup. OpenShellSpawner._do_spawn() internally waits for + /health to return 200 (_wait_ready_with_host) and raises + RuntimeError if it times out -- so spawn() completing without + raising is already proof the server started and uvicorn imported + cleanly. This test additionally re-confirms /health directly. + """ + name = "landlock-baseline-e2e-test" + try: + endpoint = await spawner.spawn( + name, + OPENSHELL_SANDBOX_IMAGE, + env={"LIGHTSPEED_PROVIDER": "openai", "LIGHTSPEED_MODEL": "gpt-4o-mini"}, + read_only=False, + ) + assert endpoint + + headers = spawner.get_sandbox_headers(name) + async with httpx.AsyncClient(timeout=10.0) as client: + resp = await client.get(f"{endpoint}/health", headers=headers) + assert resp.status_code == 200 + finally: + await spawner.destroy(name) + + @pytest.mark.asyncio + async def test_extra_readable_paths_widens_landlock_access_on_real_gateway(self) -> None: + """The core regression test for issue #189, against a REAL gateway. + + /home is outside OpenShell's own hardcoded default allowlist + (/usr, /lib, /proc, /dev/urandom, /app, /etc, /var/log) and + outside this reference image's own package layout, so it is a + stable stand-in for "a path the caller's image needs that the + gateway's default doesn't grant" -- exactly the shape of the + original /opt/app-root bug, without depending on where any one + image build happens to install its packages. + + Without `/home` in `extra_readable_paths`, a non-advisory spawn + must still be denied (`ls /home` -> Permission denied) -- proving + Landlock enforcement is genuinely active in this environment, not + silently skipped. With `/home` added to `extra_readable_paths`, + the same command must succeed -- proving the baseline builder's + default-allowlist-union-extras is what actually reaches the real + gateway's enforcement, not just a shape asserted against a mock. + """ + from cloud_agents.spawner.factory import build_spawner + + denied_spawner = build_spawner( + "openshell", + gateway_url=OPENSHELL_GATEWAY_URL, + driver="podman", + workspace="default", + extra_readable_paths=[], + ) + allowed_spawner = build_spawner( + "openshell", + gateway_url=OPENSHELL_GATEWAY_URL, + driver="podman", + workspace="default", + extra_readable_paths=["/home"], + ) + env = {"LIGHTSPEED_PROVIDER": "openai", "LIGHTSPEED_MODEL": "gpt-4o-mini"} + + denied_name = "landlock-extra-denied-test" + try: + await denied_spawner.spawn( + denied_name, OPENSHELL_SANDBOX_IMAGE, env=env, read_only=False + ) + denied_id = denied_spawner.get_sandbox_id(denied_name) + denied_result = await _exec(denied_spawner._client, denied_id, ["ls", "/home"]) + assert denied_result is not None + assert denied_result.exit_code != 0 + finally: + await denied_spawner.destroy(denied_name) + + allowed_name = "landlock-extra-allowed-test" + try: + await allowed_spawner.spawn( + allowed_name, OPENSHELL_SANDBOX_IMAGE, env=env, read_only=False + ) + allowed_id = allowed_spawner.get_sandbox_id(allowed_name) + allowed_result = await _exec(allowed_spawner._client, allowed_id, ["ls", "/home"]) + assert allowed_result is not None + assert allowed_result.exit_code == 0 + finally: + await allowed_spawner.destroy(allowed_name) diff --git a/tests/unit/spawner/test_factory.py b/tests/unit/spawner/test_factory.py index 2b7c7272..fb93815b 100644 --- a/tests/unit/spawner/test_factory.py +++ b/tests/unit/spawner/test_factory.py @@ -248,6 +248,43 @@ def test_unknown_extra_keys_are_dropped_not_forwarded(self, mocker: MockerFixtur assert spawner._endpoint == "gw:17670" + def test_extra_readable_paths_forwarded(self, mocker: MockerFixture) -> None: + """extra_readable_paths reaches OpenShellSpawner (issue #189). + + _OPENSHELL_EXTRA_PARAMS previously only allowed `max_pods` through + -- an unlisted kwarg like extra_readable_paths would be silently + dropped by build_spawner's kwarg filtering, even though + OpenShellSpawner's own constructor accepts it. + """ + from cloud_agents.spawner.factory import build_spawner + + mocker.patch("openshell.SandboxClient") + + spawner = build_spawner( + "openshell", + gateway_url="gw:17670", + extra_readable_paths=["/opt/custom", "/srv/app"], + ) + + assert spawner._extra_readable_paths == ["/opt/custom", "/srv/app"] + + def test_extra_readable_paths_none_falls_back_to_spawner_default( + self, mocker: MockerFixture + ) -> None: + """Omitting extra_readable_paths (or passing None) uses OpenShellSpawner's own default. + + None must be dropped rather than forwarded, same as other Optional + fields -- forwarding None would override the spawner's own + ["/opt/app-root", "/opt/lightspeed"] default with an empty value. + """ + from cloud_agents.spawner.factory import build_spawner + + mocker.patch("openshell.SandboxClient") + + spawner = build_spawner("openshell", gateway_url="gw:17670", extra_readable_paths=None) + + assert spawner._extra_readable_paths == ["/opt/app-root", "/opt/lightspeed"] + class TestBuildSpawnerUnknownType: """Tests for the error path on unrecognized spawner types.""" diff --git a/tests/unit/spawner/test_openshell_spawner.py b/tests/unit/spawner/test_openshell_spawner.py index 630337f5..913af50e 100644 --- a/tests/unit/spawner/test_openshell_spawner.py +++ b/tests/unit/spawner/test_openshell_spawner.py @@ -1010,6 +1010,253 @@ def test_includes_workdir(self, mocker: MockerFixture) -> None: assert spec.policy.filesystem.include_workdir is True +class TestBaselineFilesystemPolicy: + """Tests for _build_baseline_filesystem_policy() (issue #189). + + Runs for every non-advisory spawn. Unlike _build_filesystem_policy() + (the advisory/full-lockdown path, read_only=["/"]), this sends + OpenShell's own default read_only allowlist UNION extra_readable_paths + -- never just the extras alone, since OpenShell replaces (does not + merge with) a supplied filesystem policy. + """ + + _DEFAULT_RO = ["/usr", "/lib", "/proc", "/dev/urandom", "/app", "/etc", "/var/log"] + + def _make_spec(self, mocker: MockerFixture): + spec = mocker.Mock() + spec.policy.filesystem.read_only = [] + spec.policy.filesystem.read_write = [] + spec.policy.filesystem.include_workdir = False + spec.policy.landlock.compatibility = "" + return spec + + def test_includes_full_default_read_only_allowlist(self, mocker: MockerFixture) -> None: + """Baseline must include OpenShell's complete default RO list, not just the extras. + + Sending only the extra paths would replace (not merge with) + OpenShell's default and drop /usr, /lib, /proc, /etc, breaking + things worse than the bug this fixes. + """ + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = OpenShellSpawner(openshell_client=object()) + spec = self._make_spec(mocker) + + spawner._build_baseline_filesystem_policy(spec) + + for path in self._DEFAULT_RO: + assert path in spec.policy.filesystem.read_only + + def test_includes_default_extra_readable_paths(self, mocker: MockerFixture) -> None: + """Baseline includes the default extra paths (/opt/app-root, /opt/lightspeed).""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = OpenShellSpawner(openshell_client=object()) + spec = self._make_spec(mocker) + + spawner._build_baseline_filesystem_policy(spec) + + assert "/opt/app-root" in spec.policy.filesystem.read_only + assert "/opt/lightspeed" in spec.policy.filesystem.read_only + + def test_includes_custom_extra_readable_paths(self, mocker: MockerFixture) -> None: + """A constructor override for extra_readable_paths is honored in the baseline.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = OpenShellSpawner(openshell_client=object(), extra_readable_paths=["/srv/custom"]) + spec = self._make_spec(mocker) + + spawner._build_baseline_filesystem_policy(spec) + + assert "/srv/custom" in spec.policy.filesystem.read_only + # Default RO allowlist is still present alongside the override. + for path in self._DEFAULT_RO: + assert path in spec.policy.filesystem.read_only + + def test_sets_default_read_write(self, mocker: MockerFixture) -> None: + """Baseline read_write matches OpenShell's own default (/tmp, /dev/null).""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = OpenShellSpawner(openshell_client=object()) + spec = self._make_spec(mocker) + + spawner._build_baseline_filesystem_policy(spec) + + assert spec.policy.filesystem.read_write == ["/tmp", "/dev/null"] + + def test_includes_workdir(self, mocker: MockerFixture) -> None: + """Baseline sets include_workdir.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = OpenShellSpawner(openshell_client=object()) + spec = self._make_spec(mocker) + + spawner._build_baseline_filesystem_policy(spec) + + assert spec.policy.filesystem.include_workdir is True + + def test_sets_landlock_best_effort_compatibility(self, mocker: MockerFixture) -> None: + """Landlock compatibility is explicitly set to best_effort. + + Matches the live-verified fix YAML in issue #189, even though + best_effort is already the proto default -- an explicit setting + survives even if the proto default ever changes. + """ + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = OpenShellSpawner(openshell_client=object()) + spec = self._make_spec(mocker) + + spawner._build_baseline_filesystem_policy(spec) + + assert spec.policy.landlock.compatibility == "best_effort" + + def test_empty_extra_readable_paths_still_sets_defaults(self, mocker: MockerFixture) -> None: + """An explicitly empty extra_readable_paths list doesn't drop the OpenShell defaults.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = OpenShellSpawner(openshell_client=object(), extra_readable_paths=[]) + spec = self._make_spec(mocker) + + spawner._build_baseline_filesystem_policy(spec) + + for path in self._DEFAULT_RO: + assert path in spec.policy.filesystem.read_only + assert "/opt/app-root" not in spec.policy.filesystem.read_only + + +class TestFilesystemPolicySelection: + """Tests that _do_spawn picks exactly one filesystem policy builder (issue #189). + + Baseline and advisory are mutually exclusive: baseline runs for every + non-advisory spawn (read_only=False, the default for normal agent + runs), and the existing advisory full-lockdown builder is unchanged + and still only runs when read_only=True. + """ + + def _make_spawner_ready_to_spawn(self, mocker: MockerFixture): + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + class SandboxRef: + id: str = "test-id" + + def __init__(self, name): + self.name = name + + mock_client = mocker.Mock() + mock_client.create.return_value = SandboxRef("ca-agent-agent-1") + mock_client.wait_ready.return_value = SandboxRef("ca-agent-agent-1") + mock_client.exec_stream.return_value = iter([]) + + spawner = OpenShellSpawner(openshell_client=mock_client) + mocker.patch.object( + spawner, + "_expose_service", + return_value=("http://gateway:17670", "sandbox.openshell.localhost"), + ) + + async def mock_ready(*args, **kwargs): + return True + + mocker.patch.object(spawner, "_wait_ready_with_host", side_effect=mock_ready) + mocker.patch.object(OpenShellSpawner, "_build_network_policy") + return spawner + + @pytest.mark.asyncio + async def test_non_advisory_spawn_uses_baseline_not_advisory( + self, mocker: MockerFixture + ) -> None: + """A normal (non-advisory) spawn builds the baseline policy, not the full lockdown.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = self._make_spawner_ready_to_spawn(mocker) + baseline_spy = mocker.patch.object( + OpenShellSpawner, "_build_baseline_filesystem_policy", autospec=True + ) + advisory_spy = mocker.patch.object(OpenShellSpawner, "_build_filesystem_policy") + + await spawner.spawn("agent-1", "sandbox:latest", env={}, read_only=False) + + baseline_spy.assert_called_once() + advisory_spy.assert_not_called() + + @pytest.mark.asyncio + async def test_advisory_spawn_uses_advisory_not_baseline(self, mocker: MockerFixture) -> None: + """An advisory (read_only=True) spawn keeps using the existing full-lockdown builder.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = self._make_spawner_ready_to_spawn(mocker) + baseline_spy = mocker.patch.object( + OpenShellSpawner, "_build_baseline_filesystem_policy", autospec=True + ) + advisory_spy = mocker.patch.object(OpenShellSpawner, "_build_filesystem_policy") + + await spawner.spawn("agent-1", "sandbox:latest", env={}, read_only=True) + + advisory_spy.assert_called_once() + baseline_spy.assert_not_called() + + +class TestExtraReadablePathsConstructor: + """Tests for the `extra_readable_paths` constructor argument (issue #189). + + These paths let a baseline (non-advisory) filesystem policy include + directories beyond OpenShell's own hardcoded default allowlist -- + e.g. /opt/app-root and /opt/lightspeed, where this sandbox image's + Python packages and application code live. + """ + + def test_defaults_to_opt_app_root_and_opt_lightspeed(self) -> None: + """Constructor default matches the reference sandbox image's layout.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = OpenShellSpawner(openshell_client=object()) + + assert spawner._extra_readable_paths == ["/opt/app-root", "/opt/lightspeed"] + + def test_accepts_custom_paths(self) -> None: + """Constructor accepts an override for derived images with a different layout.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = OpenShellSpawner( + openshell_client=object(), + extra_readable_paths=["/opt/custom", "/srv/app"], + ) + + assert spawner._extra_readable_paths == ["/opt/custom", "/srv/app"] + + def test_rejects_relative_path(self) -> None: + """A relative path is not a valid Landlock filesystem rule.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + with pytest.raises(ValueError, match="absolute"): + OpenShellSpawner(openshell_client=object(), extra_readable_paths=["relative/path"]) + + def test_rejects_dotdot_segment(self) -> None: + """A path containing a `..` segment could escape the intended directory.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + with pytest.raises(ValueError, match=r"\.\."): + OpenShellSpawner( + openshell_client=object(), extra_readable_paths=["/opt/app-root/../etc"] + ) + + def test_rejects_empty_string(self) -> None: + """An empty string is not a meaningful filesystem path.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + with pytest.raises(ValueError, match="empty"): + OpenShellSpawner(openshell_client=object(), extra_readable_paths=["/opt/app-root", ""]) + + def test_empty_list_is_valid(self) -> None: + """An explicit empty list disables the extra-paths feature entirely.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + spawner = OpenShellSpawner(openshell_client=object(), extra_readable_paths=[]) + + assert spawner._extra_readable_paths == [] + + class TestCredentialInjection: """Tests for _inject_credentials() and Provider API integration.""" From d4407e07142d40f988178bb7926e436efb2fe00f Mon Sep 17 00:00:00 2001 From: James Wong <2421248+jameswnl@users.noreply.github.com> Date: Tue, 25 Aug 2026 17:02:33 -0400 Subject: [PATCH 2/3] Reject filesystem-root extra_readable_paths; strengthen e2e assertion Per PR #191 review (beesarmy, approved with non-blocking follow-ups): - [MED] _validate_extra_readable_paths accepted "/" -- on the baseline (non-advisory) policy this would grant full-filesystem read without advisory mode's compensating write lockdown, effectively disabling the read restriction this fix exists to preserve. Now rejected. Verified during implementation that a naive os.path.normpath(path) == "/" check misses "//" (Python's posixpath.normpath preserves exactly two leading slashes per POSIX rather than collapsing them) -- used path.strip("/") == "" instead, which catches both "/" and "//"/"///". - [LOW] The e2e deny-side assertion only checked exit_code != 0, which ENOENT or a broken exec would also satisfy without proving Landlock denial specifically. Now also asserts "permission denied" in stderr (confirmed present in the real gateway's actual error output while verifying this PR). - [LOW] The _DEFAULT_BASELINE_READ_ONLY comment cited the wrong Rust crate (openshell-sandbox instead of openshell-policy, where restrictive_default_policy() actually lives) -- found while trying to add the commit reference the reviewer asked for. Fixed and pinned to openshell@679fe4c, confirmed by reading that source directly. Re-verified against the real Kind + OpenShell gateway: both e2e tests in TestOpenShellGuardrails still pass with these changes. Co-Authored-By: Claude Sonnet 5 --- src/cloud_agents/spawner/openshell_spawner.py | 31 ++++++++++++++----- tests/e2e/test_guardrails.py | 3 ++ tests/unit/spawner/test_openshell_spawner.py | 21 +++++++++++++ 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/src/cloud_agents/spawner/openshell_spawner.py b/src/cloud_agents/spawner/openshell_spawner.py index 4baeed48..b332823f 100644 --- a/src/cloud_agents/spawner/openshell_spawner.py +++ b/src/cloud_agents/spawner/openshell_spawner.py @@ -79,11 +79,14 @@ class OpenShellSpawner(AgentSpawner): _DEFAULT_EXTRA_READABLE_PATHS: ClassVar[list[str]] = ["/opt/app-root", "/opt/lightspeed"] # OpenShell's own hardcoded restrictive_default_policy() (Rust side, - # crates/openshell-sandbox/src/lib.rs). Mirrored here because OpenShell - # does NOT merge a supplied filesystem policy with its own default -- - # a supplied policy fully *replaces* it. So the baseline builder below - # must always send this full list, union'd with the extra paths, never - # just the extras alone -- see issue #189. + # crates/openshell-policy/src/lib.rs, confirmed against openshell@679fe4c). + # Mirrored here because OpenShell does NOT merge a supplied filesystem + # policy with its own default -- a supplied policy fully *replaces* it. + # So the baseline builder below must always send this full list, + # union'd with the extra paths, never just the extras alone -- see + # issue #189. If this list ever drifts from OpenShell's own, re-verify + # against restrictive_default_policy() directly rather than trusting + # this comment. _DEFAULT_BASELINE_READ_ONLY: ClassVar[list[str]] = [ "/usr", "/lib", @@ -103,7 +106,10 @@ def _validate_extra_readable_paths(paths: list[str]) -> list[str]: malformed input here is a security concern, not just a usability one. Rejects relative paths (meaningless to Landlock, which rules operate on absolute filesystem paths), ".." segments (could escape - the intended directory), and empty strings. + the intended directory), empty strings, and "/" itself (would grant + full-filesystem read on the baseline policy without advisory + mode's write lockdown -- effectively disabling the read + restriction this fix exists to preserve). Args: paths: Candidate list of absolute directory paths. @@ -115,8 +121,8 @@ def _validate_extra_readable_paths(paths: list[str]) -> list[str]: the spawner's stored policy. Raises: - ValueError: If any path is empty, relative, or contains a - ".." segment. + ValueError: If any path is empty, relative, "/", or contains + a ".." segment. """ for path in paths: if not path: @@ -127,6 +133,15 @@ def _validate_extra_readable_paths(paths: list[str]) -> list[str]: raise ValueError( f"extra_readable_paths entries must not contain '..' segments: {path!r}" ) + # strip("/") rather than normpath(): normpath() special-cases + # exactly two leading slashes ("//") per POSIX and leaves it + # unchanged instead of collapsing it to "/", which would let + # "//" slip past a normpath-based check. + if path.strip("/") == "": + raise ValueError( + "extra_readable_paths entries must not be the filesystem root " + f"('/') -- this would grant full-filesystem read: {path!r}" + ) return list(paths) def __init__( diff --git a/tests/e2e/test_guardrails.py b/tests/e2e/test_guardrails.py index b3a72230..8f3869b1 100644 --- a/tests/e2e/test_guardrails.py +++ b/tests/e2e/test_guardrails.py @@ -393,6 +393,9 @@ async def test_extra_readable_paths_widens_landlock_access_on_real_gateway(self) denied_result = await _exec(denied_spawner._client, denied_id, ["ls", "/home"]) assert denied_result is not None assert denied_result.exit_code != 0 + # Assert the specific reason, not just a nonzero exit -- ENOENT or a + # broken exec would also be nonzero without proving Landlock denial. + assert "permission denied" in denied_result.stderr.lower() finally: await denied_spawner.destroy(denied_name) diff --git a/tests/unit/spawner/test_openshell_spawner.py b/tests/unit/spawner/test_openshell_spawner.py index 913af50e..8f3476ca 100644 --- a/tests/unit/spawner/test_openshell_spawner.py +++ b/tests/unit/spawner/test_openshell_spawner.py @@ -1256,6 +1256,27 @@ def test_empty_list_is_valid(self) -> None: assert spawner._extra_readable_paths == [] + def test_rejects_filesystem_root(self) -> None: + """"/" widens the baseline policy to full-filesystem read (issue #189 review). + + The baseline policy's write list stays narrow (/tmp, /dev/null), + but "/" in read_only would grant read access to everything -- + effectively disabling the read restriction this fix exists to + preserve, without any of advisory mode's compensating write + lockdown. + """ + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + with pytest.raises(ValueError, match="root"): + OpenShellSpawner(openshell_client=object(), extra_readable_paths=["/"]) + + def test_rejects_filesystem_root_via_trailing_slash(self) -> None: + """"//" and other root-equivalent forms are caught by normpath, not just "/" itself.""" + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + with pytest.raises(ValueError, match="root"): + OpenShellSpawner(openshell_client=object(), extra_readable_paths=["//"]) + class TestCredentialInjection: """Tests for _inject_credentials() and Provider API integration.""" From e30aa8a1e731ab7b39c16016b7d3cd0e2c02d967 Mon Sep 17 00:00:00 2001 From: James Wong <2421248+jameswnl@users.noreply.github.com> Date: Tue, 25 Aug 2026 17:24:57 -0400 Subject: [PATCH 3/3] Close /. and /././ bypass of the filesystem-root check Per PR #191 re-review (beesarmy): path.strip("/") == "" alone missed "/." and "/././" -- strip() only trims leading/trailing "/" characters, it doesn't resolve "." segments, so "/.".strip("/") == "." (truthy, passes validation) even though Landlock/kernel path resolution treats "/." identically to "/". Confirmed the bypass and the fix locally before applying: "/.".strip("/") == "." # doesn't catch it posixpath.normpath("/.") == "/" # catches it posixpath.normpath("//") == "//" # but normpath alone misses "//" Neither check alone is sufficient; now uses both (path.strip("/") == "" or normpath(path) in ("/", "//")). Added tests for "/.", "/././", and "///" alongside the existing "/" and "//" cases. Also fixed a stale test docstring/name from the prior round that said "caught by normpath" when the implementation actually used strip(). Co-Authored-By: Claude Sonnet 5 --- src/cloud_agents/spawner/openshell_spawner.py | 12 +++++----- tests/unit/spawner/test_openshell_spawner.py | 22 +++++++++++++++++-- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/cloud_agents/spawner/openshell_spawner.py b/src/cloud_agents/spawner/openshell_spawner.py index b332823f..e4065030 100644 --- a/src/cloud_agents/spawner/openshell_spawner.py +++ b/src/cloud_agents/spawner/openshell_spawner.py @@ -133,11 +133,13 @@ def _validate_extra_readable_paths(paths: list[str]) -> list[str]: raise ValueError( f"extra_readable_paths entries must not contain '..' segments: {path!r}" ) - # strip("/") rather than normpath(): normpath() special-cases - # exactly two leading slashes ("//") per POSIX and leaves it - # unchanged instead of collapsing it to "/", which would let - # "//" slip past a normpath-based check. - if path.strip("/") == "": + # Both checks are needed, neither alone is sufficient: + # - strip("/") == "" catches "/", "//", "///" -- but NOT "/." + # or "/././", since strip() doesn't resolve "." segments. + # - normpath() resolves "/." and "/././" to "/" -- but leaves + # "//" as "//" unchanged (POSIX special-cases exactly two + # leading slashes), so it alone would miss "//". + if path.strip("/") == "" or os.path.normpath(path) in ("/", "//"): raise ValueError( "extra_readable_paths entries must not be the filesystem root " f"('/') -- this would grant full-filesystem read: {path!r}" diff --git a/tests/unit/spawner/test_openshell_spawner.py b/tests/unit/spawner/test_openshell_spawner.py index 8f3476ca..6dbd9910 100644 --- a/tests/unit/spawner/test_openshell_spawner.py +++ b/tests/unit/spawner/test_openshell_spawner.py @@ -1270,12 +1270,30 @@ def test_rejects_filesystem_root(self) -> None: with pytest.raises(ValueError, match="root"): OpenShellSpawner(openshell_client=object(), extra_readable_paths=["/"]) - def test_rejects_filesystem_root_via_trailing_slash(self) -> None: - """"//" and other root-equivalent forms are caught by normpath, not just "/" itself.""" + def test_rejects_filesystem_root_via_extra_slashes(self) -> None: + """"//" and "///" are root-equivalent, caught by the strip("/") check.""" from cloud_agents.spawner.openshell_spawner import OpenShellSpawner with pytest.raises(ValueError, match="root"): OpenShellSpawner(openshell_client=object(), extra_readable_paths=["//"]) + with pytest.raises(ValueError, match="root"): + OpenShellSpawner(openshell_client=object(), extra_readable_paths=["///"]) + + def test_rejects_filesystem_root_via_dot_segments(self) -> None: + """"/." and "/././" are root-equivalent once path-resolved, caught by normpath. + + strip("/") alone is not enough here: "/.".strip("/") == "." (not + empty), since strip() only trims leading/trailing "/" characters + and doesn't resolve "." segments. Landlock/kernel path resolution + treats "/." the same as "/", so this needs the normpath() check + combined with the strip() check, not either alone. + """ + from cloud_agents.spawner.openshell_spawner import OpenShellSpawner + + with pytest.raises(ValueError, match="root"): + OpenShellSpawner(openshell_client=object(), extra_readable_paths=["/."]) + with pytest.raises(ValueError, match="root"): + OpenShellSpawner(openshell_client=object(), extra_readable_paths=["/././"]) class TestCredentialInjection: