diff --git a/src/seclab_taskflows/mcp_servers/container_shell.py b/src/seclab_taskflows/mcp_servers/container_shell.py index 81f863e..78b8874 100644 --- a/src/seclab_taskflows/mcp_servers/container_shell.py +++ b/src/seclab_taskflows/mcp_servers/container_shell.py @@ -8,6 +8,13 @@ - ``CONTAINER_IMAGE`` — image to run (required). - ``CONTAINER_WORKSPACE`` — host path bind-mounted at ``/workspace`` (optional). +- ``CONTAINER_WORKSPACE_MOUNT_MODE`` — bind-mount mode for that path, ``rw`` (default) + or ``ro``. The default preserves the historical writable mount, because the + shipped source-access and SAST prompts build symbol indexes in-tree + (``ctags -R .``, ``cscope -R -b``, ``gtags``). Set it to ``ro`` when the + workspace should be left pristine — for example when the caller collects the + tree afterwards as an artifact and wants a faithful copy of the input. Only a + literal ``ro`` selects read-only; any other value mounts read-write. - ``CONTAINER_TIMEOUT`` — default per-command timeout in seconds (default 30). - ``CONTAINER_PERSIST`` — reuse a deterministic container across runs when truthy. - ``CONTAINER_PERSIST_KEY`` — extra key to distinguish persistent containers. @@ -66,6 +73,18 @@ CONTAINER_IMAGE = os.environ.get("CONTAINER_IMAGE", "") CONTAINER_WORKSPACE = os.environ.get("CONTAINER_WORKSPACE", "") +# Bind-mount mode for CONTAINER_WORKSPACE. Defaults to "rw", which preserves +# the historical behaviour: the shipped source-access and SAST prompts tell the +# model to build symbol indexes in-tree (`ctags -R .`, `cscope -R -b`, `gtags`), +# which write into the workspace. Setting this to "ro" is worth doing whenever +# the caller collects the workspace afterwards as an artifact, since a writable +# mount makes that artifact agent-influenced rather than a faithful copy of the +# input. Flipping the default would require redirecting those index outputs +# first. Only a literal "ro" selects read-only, so a blank or unrecognized +# value cannot accidentally break a workflow that needs to write. +CONTAINER_WORKSPACE_MOUNT_MODE = ( + "ro" if os.environ.get("CONTAINER_WORKSPACE_MOUNT_MODE", "").strip().lower() == "ro" else "rw" +) CONTAINER_TIMEOUT = int(os.environ.get("CONTAINER_TIMEOUT", "30")) CONTAINER_PERSIST = os.environ.get("CONTAINER_PERSIST", "").lower() in ("1", "true", "yes") CONTAINER_PERSIST_KEY = os.environ.get("CONTAINER_PERSIST_KEY", "") @@ -103,8 +122,19 @@ def _persistent_name() -> str: network (e.g. the default "none") never reuses a persistent container that was created with a different, more permissive network (e.g. "bridge"), which would otherwise silently re-enable egress. + + The workspace mount mode is included for the same reason, but only when it + differs from the "rw" default. Appending it unconditionally would change the + derived name for every persistent container that already exists, orphaning + each one on upgrade: _start_container() only ever inspects or removes the + name it computes, and persistent containers are normally left running, so a + non-forcing ``docker rm`` would not collect them either. Omitting the field + at the default keeps those names byte-identical, while an explicit "ro" run + still gets a distinct name and so cannot reuse a writable container. """ key_material = f"{CONTAINER_IMAGE}:{CONTAINER_WORKSPACE}:net={CONTAINER_NETWORK}" + if CONTAINER_WORKSPACE_MOUNT_MODE != "rw": + key_material += f":ws={CONTAINER_WORKSPACE_MOUNT_MODE}" if CONTAINER_PERSIST_KEY: key_material += f":{CONTAINER_PERSIST_KEY}" digest = hashlib.sha256(key_material.encode()).hexdigest()[:12] @@ -172,7 +202,7 @@ def _start_container() -> str: if not CONTAINER_PERSIST: cmd.append("--rm") if CONTAINER_WORKSPACE: - cmd += ["-v", f"{CONTAINER_WORKSPACE}:/workspace"] + cmd += ["-v", f"{CONTAINER_WORKSPACE}:/workspace:{CONTAINER_WORKSPACE_MOUNT_MODE}"] cmd += [CONTAINER_IMAGE, "tail", "-f", "/dev/null"] logging.debug(f"Starting container: {' '.join(cmd)}") result = subprocess.run(cmd, capture_output=True, text=True, timeout=_DOCKER_TIMEOUT) diff --git a/src/seclab_taskflows/toolboxes/container_shell_base.yaml b/src/seclab_taskflows/toolboxes/container_shell_base.yaml index 186a541..4952a59 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_base.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_base.yaml @@ -12,6 +12,7 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-base:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) }}" + CONTAINER_WORKSPACE_MOUNT_MODE: "{{ env('CONTAINER_WORKSPACE_MOUNT_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '30') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml b/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml index 734ecd4..b342148 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml @@ -12,6 +12,7 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-malware-analysis:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) }}" + CONTAINER_WORKSPACE_MOUNT_MODE: "{{ env('CONTAINER_WORKSPACE_MOUNT_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '60') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml b/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml index 0b5c878..d5722dc 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml @@ -12,6 +12,7 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-network-analysis:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) }}" + CONTAINER_WORKSPACE_MOUNT_MODE: "{{ env('CONTAINER_WORKSPACE_MOUNT_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '30') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_sast.yaml b/src/seclab_taskflows/toolboxes/container_shell_sast.yaml index 93ae25b..3aeb113 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_sast.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_sast.yaml @@ -12,6 +12,12 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-sast:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) }}" + # Writable: the documented workflow for this toolbox builds symbol indexes + # in-tree (`ctags -R .`, `cscope -R -b`, `gtags`), which write tags, + # cscope.out and GTAGS/GRTAGS into the workspace. Set this to "ro" and + # redirect those outputs (e.g. `ctags -f /tmp/tags`) if you need the + # workspace left pristine. + CONTAINER_WORKSPACE_MOUNT_MODE: "{{ env('CONTAINER_WORKSPACE_MOUNT_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '60') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml b/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml index badfcd6..7c434b6 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml @@ -12,6 +12,10 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-source-access:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) or env('DATA_DIR') ~ '/repo_under_test' }}" + # The prompt for this toolbox builds symbol indexes in-tree (`ctags -R .`, + # `gtags`), so the mount must stay writable by default. Set this to "ro" + # if you redirect those outputs and want the workspace left pristine. + CONTAINER_WORKSPACE_MOUNT_MODE: "{{ env('CONTAINER_WORKSPACE_MOUNT_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '60') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', 'true') }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/tests/test_container_shell.py b/tests/test_container_shell.py index ca1baad..360e275 100644 --- a/tests/test_container_shell.py +++ b/tests/test_container_shell.py @@ -2,6 +2,7 @@ # SPDX-License-Identifier: MIT import atexit +import hashlib import importlib import os import subprocess @@ -67,6 +68,7 @@ def test_start_container_success(self): with ( patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "rw"), patch("subprocess.run", return_value=_make_proc(returncode=0)) as mock_run, ): name = cs_mod._start_container() @@ -76,7 +78,7 @@ def test_start_container_success(self): assert "run" in cmd assert "--name" in cmd assert "-v" in cmd - assert "/host/workspace:/workspace" in cmd + assert "/host/workspace:/workspace:rw" in cmd assert "test-image:latest" in cmd assert "tail" in cmd @@ -91,6 +93,49 @@ def test_start_container_no_workspace(self): cmd = mock_run.call_args[0][0] assert "-v" not in cmd + def test_start_container_workspace_writable_by_default(self): + """The default preserves the historical writable mount.""" + with ( + patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), + patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "rw"), + patch("subprocess.run", return_value=_make_proc(returncode=0)) as mock_run, + ): + cs_mod._start_container() + cmd = mock_run.call_args[0][0] + assert "/host/workspace:/workspace:rw" in cmd + + def test_start_container_workspace_ro_opt_in(self): + """CONTAINER_WORKSPACE_MOUNT_MODE=ro mounts the workspace read-only.""" + with ( + patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), + patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "ro"), + patch("subprocess.run", return_value=_make_proc(returncode=0)) as mock_run, + ): + cs_mod._start_container() + cmd = mock_run.call_args[0][0] + assert "/host/workspace:/workspace:ro" in cmd + + def test_persistent_name_unchanged_at_default_mode(self): + """Upgrading must not rename existing persistent containers. + + The pre-CONTAINER_WORKSPACE_MOUNT_MODE name is the one derived without a + ws= field, so the default must reproduce it exactly or every existing + persistent container is orphaned on upgrade. + """ + with ( + patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), + patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), + patch.object(cs_mod, "CONTAINER_NETWORK", "none"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "rw"), + patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), + ): + pre_mode = hashlib.sha256( + b"test-image:latest:/host/workspace:net=none" + ).hexdigest()[:12] + assert cs_mod._persistent_name() == f"seclab-persist-{pre_mode}" + def test_start_container_failure(self): with ( patch.object(cs_mod, "CONTAINER_IMAGE", "missing-image:latest"), @@ -140,6 +185,39 @@ def test_start_container_opt_in_network(self): assert "--network" in cmd assert cmd[cmd.index("--network") + 1] == "bridge" + def test_workspace_mode_defaults_to_rw_when_unset(self, monkeypatch): + """Unset preserves the historical writable mount.""" + original = os.environ.get("CONTAINER_WORKSPACE_MOUNT_MODE") + monkeypatch.delenv("CONTAINER_WORKSPACE_MOUNT_MODE", raising=False) + try: + reloaded = _reload_cs() + assert reloaded.CONTAINER_WORKSPACE_MOUNT_MODE == "rw" + finally: + _restore_env_and_reload("CONTAINER_WORKSPACE_MOUNT_MODE", original) + + @pytest.mark.parametrize( + "value", ["", " ", "\t", "bogus", "readonly", "rw", "ro; rm -rf /", "ro rw"] + ) + def test_workspace_mode_falls_back_to_rw(self, monkeypatch, value): + """Only a literal "ro" opts in; anything else stays writable.""" + original = os.environ.get("CONTAINER_WORKSPACE_MOUNT_MODE") + monkeypatch.setenv("CONTAINER_WORKSPACE_MOUNT_MODE", value) + try: + reloaded = _reload_cs() + assert reloaded.CONTAINER_WORKSPACE_MOUNT_MODE == "rw" + finally: + _restore_env_and_reload("CONTAINER_WORKSPACE_MOUNT_MODE", original) + + @pytest.mark.parametrize("value", ["ro", "RO", " ro "]) + def test_workspace_mode_ro_opt_in(self, monkeypatch, value): + original = os.environ.get("CONTAINER_WORKSPACE_MOUNT_MODE") + monkeypatch.setenv("CONTAINER_WORKSPACE_MOUNT_MODE", value) + try: + reloaded = _reload_cs() + assert reloaded.CONTAINER_WORKSPACE_MOUNT_MODE == "ro" + finally: + _restore_env_and_reload("CONTAINER_WORKSPACE_MOUNT_MODE", original) + def test_network_defaults_to_none_when_unset(self, monkeypatch): original = os.environ.get("CONTAINER_NETWORK") monkeypatch.delenv("CONTAINER_NETWORK", raising=False) @@ -301,6 +379,19 @@ def test_persistent_name_differs_for_different_workspaces(self): name_b = cs_mod._persistent_name() assert name_a != name_b + def test_persistent_name_varies_with_workspace_mode(self): + """A "ro" run must not reuse a container created with a writable mount.""" + with ( + patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), + patch.object(cs_mod, "CONTAINER_WORKSPACE", "/source/tree"), + patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), + ): + with patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "ro"): + name_ro = cs_mod._persistent_name() + with patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "rw"): + name_rw = cs_mod._persistent_name() + assert name_ro != name_rw + def test_persistent_name_varies_with_network(self): with ( patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), @@ -343,13 +434,18 @@ def test_start_persistent_no_rm_flag(self): patch.object(cs_mod, "CONTAINER_WORKSPACE", ""), patch.object(cs_mod, "CONTAINER_PERSIST", True), patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), - patch("subprocess.run", side_effect=[inspect_proc, rm_proc, run_proc]) as mock_run, + patch( + "subprocess.run", + side_effect=[inspect_proc, rm_proc, run_proc], + ) as mock_run, ): name = cs_mod._start_container() assert name.startswith("seclab-persist-") - # The docker run call is the third one - run_cmd = mock_run.call_args_list[2][0][0] - assert "--rm" not in run_cmd + # Locate the docker run call rather than indexing, so inserting + # another docker invocation ahead of it does not break this test. + run_cmds = [c[0][0] for c in mock_run.call_args_list if c[0][0][:2] == ["docker", "run"]] + assert len(run_cmds) == 1 + assert "--rm" not in run_cmds[0] def test_stop_skips_persistent_container(self): cs_mod._container_name = "seclab-persist-abc123"