From fdc245c4e1c2fff1ae1b9d2980098edb34bec1e7 Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Fri, 16 Oct 2026 09:59:56 +0000 Subject: [PATCH 1/2] Clarify docker params usage --- .../DockerShellExecutor.cs | 47 ++- .../DockerShellExecutorOptions.cs | 12 +- .../ShellExecutor.cs | 6 +- .../DockerShellExecutorTests.cs | 59 ++++ python/packages/tools/README.md | 13 + .../agent_framework_tools/shell/_docker.py | 188 ++---------- .../tools/tests/test_docker_shell_tool.py | 272 ++++-------------- 7 files changed, 197 insertions(+), 400 deletions(-) diff --git a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutor.cs b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutor.cs index a3b123e75b1..e83a459bd7a 100644 --- a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutor.cs +++ b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutor.cs @@ -26,8 +26,12 @@ namespace Microsoft.Agents.AI.Tools.Shell; /// --cap-drop=ALL, --security-opt=no-new-privileges, memory and /// pids limits, --tmpfs /tmp). These are a best-effort starting point, /// NOT a guarantee: the actual isolation you get depends on the host kernel, -/// the container runtime, the image, and any caller-supplied -/// ExtraRunArgs. Do not rely on this tool as your sole defense against +/// the container runtime, the image, and the complete caller configuration. +/// must come only +/// from developer-controlled configuration, never users, model output, or +/// other untrusted sources; it can weaken or remove isolation. The model-facing +/// function accepts only commands and cannot choose Docker options. Do not +/// rely on this tool as your sole defense against /// untrusted input. Approval gating via is the /// primary safety control; pair it with the precautions you would normally /// apply when running adversarial code: review the model's output before @@ -92,7 +96,7 @@ public sealed class DockerShellExecutor : ShellExecutor private readonly int _pidsLimit; private readonly ContainerUser _user; private readonly bool _readOnlyRoot; - private readonly IReadOnlyList _extraRunArgs; + private readonly IReadOnlyList _unsafeExtraRunArgs; private readonly IReadOnlyDictionary _env; private readonly ShellPolicy _policy; private readonly TimeSpan? _timeout; @@ -125,6 +129,8 @@ public DockerShellExecutor(DockerShellExecutorOptions options) { throw new ArgumentOutOfRangeException(nameof(options), $"{nameof(options.MemoryBytes)} must be positive."); } + string[] unsafeExtraRunArgs = options.UnsafeExtraRunArgs is null ? [] : options.UnsafeExtraRunArgs.ToArray(); + ValidateUnsafeExtraRunArgs(unsafeExtraRunArgs); this._image = options.Image; this.ContainerName = options.ContainerName ?? GenerateContainerName(); @@ -137,7 +143,7 @@ public DockerShellExecutor(DockerShellExecutorOptions options) this._pidsLimit = options.PidsLimit; this._user = options.User ?? ContainerUser.Default; this._readOnlyRoot = options.ReadOnlyRoot; - this._extraRunArgs = options.ExtraRunArgs ?? []; + this._unsafeExtraRunArgs = unsafeExtraRunArgs; this._env = options.Environment ?? new Dictionary(); this._policy = options.Policy ?? new ShellPolicy(); this._timeout = options.Timeout; @@ -277,9 +283,8 @@ internal static string FormatMemoryBytes(long memoryBytes) => public override AIFunction AsAIFunction(string name = "run_shell", string? description = null, bool requireApproval = true) { description ??= - "Execute a single shell command inside an isolated Docker container and return its " + - "stdout, stderr, and exit code. The container has no network, no host filesystem access " + - "(except an optional read-only workspace mount), and runs as a non-root user. " + + "Execute a single shell command using the configured Docker container and return its " + + "stdout, stderr, and exit code. Container isolation depends on the application's complete configuration. " + (this._mode == ShellMode.Persistent ? "PERSISTENT MODE: a single long-lived container handles every call; cd and exported variables persist." : "STATELESS MODE: each call runs in a fresh container."); @@ -430,10 +435,7 @@ public static IReadOnlyList BuildExecArgv(string binary, string containe private async Task StartContainerAsync(CancellationToken cancellationToken) { - var argv = BuildRunArgv( - this.DockerBinary, this._image, this.ContainerName, this._user, this._network, - this._memoryBytes, this._pidsLimit, this._containerWorkdir, this._hostWorkdir, - this._mountReadonly, this._readOnlyRoot, this._env, this._extraRunArgs); + var argv = this.BuildRunArgvPersistent(); var (exit, _, stderr) = await RunDockerCommandAsync(argv, cancellationToken).ConfigureAwait(false); if (exit != 0) @@ -535,7 +537,7 @@ private async Task RunStatelessAsync(string command, CancellationTo TimedOut: timedOut); } - private List BuildRunArgvStateless(string perCallName) + internal List BuildRunArgvStateless(string perCallName) { var argv = new List { @@ -563,10 +565,29 @@ private List BuildRunArgvStateless(string perCallName) argv.Add("-e"); argv.Add($"{kv.Key}={kv.Value}"); } - argv.AddRange(this._extraRunArgs); + argv.AddRange(this._unsafeExtraRunArgs); return argv; } + internal IReadOnlyList BuildRunArgvPersistent() => + BuildRunArgv( + this.DockerBinary, this._image, this.ContainerName, this._user, this._network, + this._memoryBytes, this._pidsLimit, this._containerWorkdir, this._hostWorkdir, + this._mountReadonly, this._readOnlyRoot, this._env, this._unsafeExtraRunArgs); + + private static void ValidateUnsafeExtraRunArgs(IReadOnlyList args) + { + for (var index = 0; index < args.Count; index++) + { + if (args[index] is null) + { + throw new ArgumentException( + $"{nameof(DockerShellExecutorOptions.UnsafeExtraRunArgs)} contains a null token at index {index}.", + nameof(DockerShellExecutorOptions.UnsafeExtraRunArgs)); + } + } + } + private async Task BestEffortKillContainerAsync(string containerName) { try diff --git a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutorOptions.cs b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutorOptions.cs index 4211ac6a2f3..cc4e9e97d6a 100644 --- a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutorOptions.cs +++ b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutorOptions.cs @@ -52,8 +52,16 @@ public sealed class DockerShellExecutorOptions /// When (the default), the container root filesystem is read-only. public bool ReadOnlyRoot { get; set; } = true; - /// Additional args appended to docker run. - public IReadOnlyList? ExtraRunArgs { get; set; } + /// + /// Trusted application-supplied options appended verbatim to docker run. + /// Set this only from developer-controlled configuration; never use values + /// from users, model output, or other untrusted sources. These options can + /// weaken or completely remove container isolation. Review every option + /// carefully. In particular, avoid --privileged, --network=host, + /// host-path volume mounts, and added capabilities unless the application + /// explicitly requires and secures them. + /// + public IReadOnlyList? UnsafeExtraRunArgs { get; set; } /// Environment variables passed via -e to every command. public IReadOnlyDictionary? Environment { get; set; } diff --git a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellExecutor.cs b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellExecutor.cs index 957b7fd657b..09731286f25 100644 --- a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellExecutor.cs +++ b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellExecutor.cs @@ -14,8 +14,10 @@ namespace Microsoft.Agents.AI.Tools.Shell; /// /// runs commands directly on the host (no /// isolation; approval-in-the-loop is the security boundary). -/// runs them inside a container with resource -/// limits, network isolation, and a non-root user. +/// runs them inside a configured container; +/// its default profile applies resource limits, network isolation, and a +/// non-root user, but effective isolation depends on the complete application +/// configuration. /// /// /// This is an abstract class rather than an interface so the surface can be diff --git a/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/DockerShellExecutorTests.cs b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/DockerShellExecutorTests.cs index db845f1af24..08511229654 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/DockerShellExecutorTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/DockerShellExecutorTests.cs @@ -124,6 +124,51 @@ public void BuildRunArgv_EnvAndExtraArgs_AreAppended() Assert.Contains("owner=test", list); } + [Fact] + public void Ctor_RejectsNullUnsafeExtraRunArg() + { + // Arrange + var options = new DockerShellExecutorOptions { UnsafeExtraRunArgs = [null!] }; + + // Act + var exception = Assert.Throws(() => new DockerShellExecutor(options)); + + // Assert + Assert.Equal("UnsafeExtraRunArgs", exception.ParamName); + Assert.Contains("null token", exception.Message, StringComparison.Ordinal); + } + + [Fact] + public async Task Ctor_SnapshotsCallerOwnedUnsafeExtraRunArgListAsync() + { + // Arrange + var unsafeArgs = new List { "--network", "host" }; + await using var unsafeExecutor = new DockerShellExecutor(new() { UnsafeExtraRunArgs = unsafeArgs }); + + // Act + unsafeArgs.Add("--privileged"); + + // Assert + Assert.DoesNotContain("--privileged", unsafeExecutor.BuildRunArgvPersistent()); + Assert.DoesNotContain("--privileged", unsafeExecutor.BuildRunArgvStateless("unsafe")); + } + + [Fact] + public async Task Ctor_PassesUnsafeExtraRunArgsVerbatimInBothModesAsync() + { + // Arrange + string[] unsafeArgs = ["--network", "host", "--entrypoint", "/bin/custom"]; + await using var executor = new DockerShellExecutor(new() { UnsafeExtraRunArgs = unsafeArgs }); + + // Act + var persistent = executor.BuildRunArgvPersistent(); + var stateless = executor.BuildRunArgvStateless("stateless"); + + // Assert + AssertOrderedSubsequence(persistent, unsafeArgs); + AssertOrderedSubsequence(stateless, unsafeArgs); + } + private static readonly string[] s_expectedInteractive = new[] { "docker", "exec", "-i", "af-shell-x", "bash", "--noprofile", "--norc" }; [Fact] @@ -169,6 +214,9 @@ public async Task AsAIFunction_DefaultRequireApproval_IsApprovalGatedAsync() var fn = t.AsAIFunction(); Assert.IsType(fn); Assert.Equal("run_shell", fn.Name); + Assert.NotNull(fn.Description); + Assert.Contains("configured Docker container", fn.Description, StringComparison.Ordinal); + Assert.DoesNotContain("has no network", fn.Description, StringComparison.Ordinal); } [Fact] @@ -211,4 +259,15 @@ public async Task RunAsync_RejectedCommand_ThrowsShellCommandRejectedAsync() await Assert.ThrowsAsync( () => t.RunAsync("rm -rf /")); } + + private static void AssertOrderedSubsequence(IReadOnlyList actual, IReadOnlyList expected) + { + var start = 0; + foreach (var value in expected) + { + var index = actual.ToList().FindIndex(start, item => string.Equals(item, value, StringComparison.Ordinal)); + Assert.True(index >= 0, $"Expected '{value}' after index {start - 1}."); + start = index + 1; + } + } } diff --git a/python/packages/tools/README.md b/python/packages/tools/README.md index 6d585ee6c17..12fea77a9e5 100644 --- a/python/packages/tools/README.md +++ b/python/packages/tools/README.md @@ -172,6 +172,19 @@ To expose a host directory, pass `host_workdir="/path"` (mounted read-only by default; `mount_readonly=False` to allow writes). Swap the container runtime with `docker_binary="podman"`. +Applications that must pass additional Docker options can use +`unsafe_extra_run_args`. These trusted setup options are appended verbatim +and must come only from developer-controlled configuration. Never use values +from users, model output, or other untrusted sources. These options can weaken +or completely remove isolation. Review every option carefully. In particular, +avoid `--privileged`, `--network=host`, host-path volume mounts, and added +capabilities unless the application explicitly requires and secures them. + +Arbitrary Docker configuration is trusted application setup. The +model-facing function accepts only a shell command and cannot choose Docker +options, but the restrictive profile is effective only when the complete +application configuration preserves it. + ## Sandbox tiers at a glance | Use case | Tool | Sandbox | diff --git a/python/packages/tools/agent_framework_tools/shell/_docker.py b/python/packages/tools/agent_framework_tools/shell/_docker.py index 316ba38c15c..a5ad3814a15 100644 --- a/python/packages/tools/agent_framework_tools/shell/_docker.py +++ b/python/packages/tools/agent_framework_tools/shell/_docker.py @@ -78,151 +78,12 @@ DEFAULT_PIDS_LIMIT = 256 DEFAULT_WORKDIR = "/workspace" -# Docker run flags that would silently dismantle the isolation defaults -# (--cap-drop ALL, --security-opt no-new-privileges, --network none, -# --read-only root, pids/memory caps) if spliced in via ``extra_run_args``. -# These are rejected at construction time so the failure surfaces loudly -# rather than as a silently-unsandboxed container at runtime. -_BLOCKED_EXTRA_RUN_FLAGS: tuple[str, ...] = ( - "--privileged", - "--cap-add", - "--security-opt", - "--network", - "--net", - "--volume", - "-v", - "--mount", - "--device", - "--device-cgroup-rule", - "--pid", - "--ipc", - "--userns", - "--user", - "-u", - "--cgroupns", - "--add-host", - "--gpus", - "--read-only", - "--tmpfs", - "--memory", - "-m", - "--memory-swap", - "--pids-limit", -) - -# ``docker run`` long options that take no value. Everything else is assumed to -# consume the following token when its value is not attached with "=", so a -# detached value that happens to start with a dash ("--env-file -vars.env") is -# not mistaken for an option. Booleans are the smaller and more stable half of -# the flag table, and assuming an unknown flag takes a value keeps the -# validator from rejecting configurations that docker accepts. -_VALUELESS_LONG_FLAGS: frozenset[str] = frozenset({ - "--detach", - "--disable-content-trust", - "--help", - "--init", - "--interactive", - "--no-healthcheck", - "--oom-kill-disable", - "--privileged", - "--publish-all", - "--quiet", - "--read-only", - "--rm", - "--sig-proxy", - "--tty", - "--use-api-socket", -}) - -# ``docker run`` short options, split by whether they consume a value. Docker's -# flag parser lets boolean shorthands be clustered ("-it") and lets a -# value-taking shorthand carry its value attached to it, so "-v/:/host:rw" is -# "-v /:/host:rw" and "-u0:0" is "--user 0:0". Resolving which shorthands a -# single-dash token actually sets therefore needs both sets: scanning stops at -# the first value-taking shorthand because the rest of the token is its value. -_BOOLEAN_SHORT_FLAGS = frozenset("diPqt") -_VALUE_SHORT_FLAGS = frozenset("acehlmpuvw") - - -def _short_flags_in_token(token: str) -> list[str]: - """Expand a single-dash token into the short flags it sets.""" - flags: list[str] = [] - for char in token[1:]: - if char in _BOOLEAN_SHORT_FLAGS: - flags.append(f"-{char}") - continue - if char in _VALUE_SHORT_FLAGS: - flags.append(f"-{char}") - # Everything after a value-taking shorthand is its value. - break - # Unknown shorthand: stop rather than misread a value as more flags. - break - return flags - - -def _blocked_flags_in_token(raw: str) -> list[str]: - """Return the blocked flags a single ``docker run`` token resolves to.""" - # Split off any "=value" tail so "--network=host" matches "--network". - flag = raw.split("=", 1)[0] - if flag.startswith("--"): - return [flag] if flag in _BLOCKED_EXTRA_RUN_FLAGS else [] - return [short for short in _short_flags_in_token(flag) if short in _BLOCKED_EXTRA_RUN_FLAGS] - - -def _consumes_next_token(raw: str) -> bool: - """Return whether this token takes the following token as its value. - - Needed so a detached value is not decoded as an option: docker accepts - ``--env-file -vars.env``, where ``-vars.env`` is a filename rather than - ``-v``. - """ - name, sep, _ = raw.partition("=") - if sep: - # Value is attached, e.g. "--network=host". - return False - if name.startswith("--"): - # pflag booleans only accept "--flag=false", never "--flag false". - return name not in _VALUELESS_LONG_FLAGS - shorts = _short_flags_in_token(name) - if not shorts: - return False - if len(name) - 1 > len(shorts): - # Trailing text is the value, e.g. "-v/:/host:rw" or "-m0". - return False - return shorts[-1][1] in _VALUE_SHORT_FLAGS - - -def _validate_extra_run_args(args: Sequence[str]) -> None: - """Reject extra ``docker run`` args that would break the isolation contract. - A caller can otherwise pass ``["--privileged"]``, ``["--network=host"]``, - ``["-v", "/:/host:rw"]``, etc. and silently undo every isolation flag - this tool sets. The blocklist covers the obvious offenders; operators - that genuinely need one of these flags should subclass the tool or - build their own argv rather than slip past the check. - """ - bad: list[str] = [] - index = 0 - while index < len(args): - raw = args[index] - index += 1 - # "-" is a conventional positional, and "--" only ends option parsing; - # neither takes a value, and args after "--" are still worth checking - # so a blocked flag cannot hide behind the marker. - if raw in ("-", "--") or not raw.startswith("-"): - continue - if _blocked_flags_in_token(raw): - bad.append(raw) - if _consumes_next_token(raw): - index += 1 - if bad: - raise ValueError( - "extra_run_args contains flags that would dismantle DockerShellTool's " - f"isolation defaults: {bad}. Override these via dedicated constructor " - "arguments (network, host_workdir, mount_readonly, read_only_root, " - "memory, pids_limit, user) or subclass the tool if you really need " - "to relax the sandbox." - ) +def _validate_unsafe_extra_run_args(args: Sequence[str]) -> None: + """Validate the shape of trusted ``docker run`` extensions.""" + for index, raw in enumerate(args): + if not isinstance(raw, str): + raise ValueError(f"unsafe_extra_run_args contains a non-string token at index {index}: {raw!r}") class DockerNotAvailableError(RuntimeError): @@ -387,26 +248,14 @@ class DockerShellTool: pids_limit: Max processes inside the container. user: ``UID:GID`` to run as. Default ``65534:65534`` (nobody). read_only_root: Mount the root filesystem read-only. Default ``True``. - extra_run_args: Additional args appended to ``docker run``. - - .. warning:: - This parameter can dismantle the tool's isolation - contract. Flags that would undo the default sandbox - (``--privileged``, ``--cap-add``, ``--security-opt``, - ``--network``/``--net``, ``-v``/``--volume``, - ``--mount``, ``--device``, ``--pid``, ``--ipc``, - ``--userns``, ``-u``/``--user``, ``--read-only``, - ``--tmpfs``, ``--add-host``, ``--gpus``, ``--cgroupns``, - ``--device-cgroup-rule``, ``-m``/``--memory``, - ``--memory-swap``, ``--pids-limit``) are rejected at - construction time, including when a short flag carries - its value attached to it (``-v/:/host:rw``) or is - clustered with other short flags (``-itv/:/host:rw``). - Override the corresponding dedicated argument - (``network``, ``host_workdir``, ``mount_readonly``, - ``read_only_root``, ``user``, etc.) instead. If you - genuinely need to relax the sandbox further, subclass - the tool — don't slip past this check. + unsafe_extra_run_args: Trusted application-supplied Docker options + appended verbatim to ``docker run``. Set this only from + developer-controlled configuration; never use values from users, + model output, or other untrusted sources. These options can weaken + or completely remove container isolation. Review every option + carefully. In particular, avoid options such as ``--privileged``, + ``--network=host``, host-path volume mounts, and added capabilities + unless the application explicitly requires and secures them. env: Environment variables to set inside the container. These are passed via ``-e`` and apply to every command. policy: Optional :class:`ShellPolicy`. Less critical than for @@ -443,7 +292,7 @@ def __init__( pids_limit: int = DEFAULT_PIDS_LIMIT, user: str = DEFAULT_CONTAINER_USER, read_only_root: bool = True, - extra_run_args: Sequence[str] | None = None, + unsafe_extra_run_args: Sequence[str] | None = None, env: Mapping[str, str] | None = None, policy: ShellPolicy | None = None, timeout: float | None = 30.0, @@ -455,7 +304,8 @@ def __init__( ) -> None: if mode not in ("persistent", "stateless"): raise ValueError(f"mode must be 'persistent' or 'stateless', got {mode!r}") - _validate_extra_run_args(tuple(extra_run_args or ())) + unsafe_extra_run_args_snapshot = tuple(unsafe_extra_run_args or ()) + _validate_unsafe_extra_run_args(unsafe_extra_run_args_snapshot) self._image = image self._container_name = container_name or f"af-shell-{secrets.token_hex(6)}" self._mode: ShellMode = mode @@ -467,7 +317,7 @@ def __init__( self._pids_limit = pids_limit self._user = user self._read_only_root = read_only_root - self._extra_run_args = tuple(extra_run_args or ()) + self._unsafe_extra_run_args = unsafe_extra_run_args_snapshot self._env = dict(env or {}) self._policy = policy or ShellPolicy() self._timeout = timeout @@ -607,7 +457,7 @@ async def _run_stateless(self, command: str, *, timeout: float | None) -> ShellR argv.extend(["-v", f"{self._host_workdir}:{self._workdir}:{ro}"]) for k, v in self._env.items(): argv.extend(["-e", f"{k}={v}"]) - argv.extend(self._extra_run_args) + argv.extend(self._unsafe_extra_run_args) argv.extend([self._image, self._shell, "-c", command]) started = time.monotonic() @@ -716,7 +566,7 @@ async def _start_container(self) -> None: mount_readonly=self._mount_readonly, read_only_root=self._read_only_root, extra_env=self._env, - extra_args=self._extra_run_args, + extra_args=self._unsafe_extra_run_args, ) proc = await asyncio.create_subprocess_exec( *argv, diff --git a/python/packages/tools/tests/test_docker_shell_tool.py b/python/packages/tools/tests/test_docker_shell_tool.py index b4f001877a2..932fdb39bb2 100644 --- a/python/packages/tools/tests/test_docker_shell_tool.py +++ b/python/packages/tools/tests/test_docker_shell_tool.py @@ -29,15 +29,7 @@ ShellResult, is_docker_available, ) -from agent_framework_tools.shell._docker import ( - _BLOCKED_EXTRA_RUN_FLAGS, - _BOOLEAN_SHORT_FLAGS, - _VALUE_SHORT_FLAGS, - _VALUELESS_LONG_FLAGS, - _consumes_next_token, - build_exec_argv, - build_run_argv, -) +from agent_framework_tools.shell._docker import build_exec_argv, build_run_argv _CommunicateOutcome: TypeAlias = tuple[bytes, bytes] | BaseException _WaitOutcome: TypeAlias = int | None | BaseException @@ -213,209 +205,20 @@ def test_build_exec_argv_interactive(): assert argv == ["docker", "exec", "-i", "c", "bash", "--noprofile", "--norc"] -# --------------------------------------------------------------------- extra_run_args validation - - -@pytest.mark.parametrize( - "extra", - [ - ("--privileged",), - ("--network=host",), - ("--network", "host"), - ("--net=host",), - ("-v", "/:/host:rw"), - ("--volume=/etc:/etc",), - ("--cap-add=ALL",), - ("--cap-add", "SYS_ADMIN"), - ("--security-opt", "seccomp=unconfined"), - ("--device", "/dev/kvm"), - ("--pid=host",), - ("--ipc=host",), - ("--userns=host",), - ("--user=0:0",), - ("--read-only=false",), - ("--tmpfs", "/var:rw"), - ("--gpus", "all"), - ("--add-host", "evil:1.2.3.4"), - ("--label", "x=1", "--privileged"), # mixed safe + unsafe - # Short flags carrying an attached value: docker parses these the same - # as the space-separated form, so they must be rejected too. - ("-v/:/host:rw",), - ("-v/var/run/docker.sock:/var/run/docker.sock",), - ("-v=/etc:/etc",), - ("-u0:0",), - # The -u alias of --user, both spellings. - ("-u", "0:0"), - ("-u=0:0",), - # Blocked short flag clustered behind boolean short flags. - ("-itv/:/host:rw",), - ("-itu0:0",), - # Resource caps the tool sets as isolation defaults: overriding these - # silently removes the memory/pids limits. - ("-m0",), - ("-m", "0"), - ("--memory=0",), - ("--memory", "0"), - ("--memory-swap=-1",), - ("--pids-limit=-1",), - ("--pids-limit", "-1"), - ("-m0", "--pids-limit=-1"), - # A blocked flag after a token that consumes nothing is still caught. - ("--", "-u0:0"), - ("-it", "-u0:0"), - ("--privileged", "-v/:/host:rw"), - ], -) -def test_dockershell_rejects_isolation_breaking_extra_run_args(extra): - with pytest.raises(ValueError, match="isolation defaults"): - DockerShellTool(extra_run_args=list(extra)) - - -def test_dockershell_rejection_message_reports_the_raw_token(): - with pytest.raises(ValueError, match=r"-v/:/host:rw"): - DockerShellTool(extra_run_args=("-v/:/host:rw",)) - - -def test_dockershell_accepts_benign_extra_run_args(): - # Should not raise. - DockerShellTool(extra_run_args=("--label", "team=af", "--name-suffix", "x")) - - -@pytest.mark.parametrize( - "extra", - [ - # Long flags that merely share a prefix with a blocked flag. - ("--userland-proxy=false",), - ("--volumes-from", "other"), - ("--network-alias", "svc"), - # Value-taking short flags that are not blocked, including attached - # values whose text contains a blocked flag's letter. - ("-l", "team=af"), - ("-lversion=1",), - ("-e", "USER=root"), - ("-eversion=1",), - ("-w/workspace",), - ("-it",), - # End-of-options marker and a bare dash. - ("--",), - ("-",), - # Detached values that start with a dash belong to the preceding - # option and must not be decoded as options themselves. - ("--env-file", "-variables.env"), - ("--name", "-upper"), - ("--label", "-v/:/host:rw"), - ("--entrypoint", "-v"), - ("-e", "-value"), - ("--hostname", "-unusual"), - # Attached values on non-blocked options are likewise not options. - ("--env-file=-variables.env",), - ], -) -def test_dockershell_accepts_extra_run_args_that_are_not_blocked(extra): - # Should not raise: normalization must not over-reject. - DockerShellTool(extra_run_args=list(extra)) - - -# ``docker run --help`` short options, transcribed so the blocklist can be -# audited without a docker daemon. Keep in sync when bumping the supported -# docker/podman baseline. -_DOCKER_RUN_SHORT_ALIASES = { - "--attach": "-a", - "--cpu-shares": "-c", - "--detach": "-d", - "--env": "-e", - "--hostname": "-h", - "--interactive": "-i", - "--label": "-l", - "--memory": "-m", - "--publish": "-p", - "--publish-all": "-P", - "--quiet": "-q", - "--tty": "-t", - "--user": "-u", - "--volume": "-v", - "--workdir": "-w", -} - - -def test_blocked_flags_cover_every_docker_short_alias(): - """Any blocked long flag with a short alias must block the alias too. - - Guards against a blocked flag being added without its short form, which - would let the alias slip past the isolation check. - """ - missing = [ - long_flag - for long_flag, short in _DOCKER_RUN_SHORT_ALIASES.items() - if long_flag in _BLOCKED_EXTRA_RUN_FLAGS and short not in _BLOCKED_EXTRA_RUN_FLAGS - ] - assert not missing, f"blocked flags missing their docker short alias: {missing}" - - -def test_short_flag_tables_match_docker_run_options(): - """The short-flag tables must classify exactly docker run's shorthands. - - A missing entry would let a blocked shorthand hide inside a cluster; a - spurious one would misread an attached value as further flags. - """ - docker_shorts = set(_DOCKER_RUN_SHORT_ALIASES.values()) - modelled = {f"-{c}" for c in _BOOLEAN_SHORT_FLAGS | _VALUE_SHORT_FLAGS} - assert modelled == docker_shorts - # A shorthand cannot be both boolean and value-taking. - assert not (_BOOLEAN_SHORT_FLAGS & _VALUE_SHORT_FLAGS) - - -# ``docker run --help`` long options that take no value, transcribed for the -# same reason as the short aliases above. -_DOCKER_RUN_BOOLEAN_FLAGS = frozenset({ - "--detach", - "--help", - "--init", - "--interactive", - "--no-healthcheck", - "--oom-kill-disable", - "--privileged", - "--publish-all", - "--quiet", - "--read-only", - "--rm", - "--sig-proxy", - "--tty", - "--use-api-socket", -}) - - -def test_valueless_long_flags_covers_every_docker_boolean(): - """A boolean flag missing from the table would swallow the next token. - - That would let ``("--some-bool", "-u0:0")`` hide a blocked flag, so the - table must list every valueless long option. - """ - missing = _DOCKER_RUN_BOOLEAN_FLAGS - _VALUELESS_LONG_FLAGS - assert not missing, f"boolean flags missing from _VALUELESS_LONG_FLAGS: {sorted(missing)}" - - -@pytest.mark.parametrize( - ("token", "consumes"), - [ - # Long options: "=" attaches the value, otherwise arity decides. - ("--env-file", True), - ("--env-file=vars.env", False), - ("--privileged", False), - ("--read-only", False), - ("--network", True), - # Short options: an attached value means the next token is unrelated. - ("-v", True), - ("-v/:/host:rw", False), - ("-m", True), - ("-m0", False), - ("-it", False), - ("-itv", True), - ("-e", True), - ], -) -def test_consumes_next_token(token, consumes): - assert _consumes_next_token(token) is consumes +# --------------------------------------------------------------------- unsafe_extra_run_args validation + + +@pytest.mark.parametrize("token", [None, 42]) +def test_dockershell_rejects_non_string_unsafe_extra_run_args(token): + with pytest.raises(ValueError, match="unsafe_extra_run_args contains a non-string token"): + DockerShellTool(unsafe_extra_run_args=[token]) # type: ignore[list-item] + + +def test_dockershell_snapshots_unsafe_extra_run_args(): + unsafe = ["--network", "host"] + unsafe_tool = DockerShellTool(unsafe_extra_run_args=unsafe) + unsafe.append("--privileged") + assert unsafe_tool._unsafe_extra_run_args == ("--network", "host") def test_build_exec_argv_non_interactive_appends_dash_c(): @@ -526,6 +329,21 @@ async def test_start_creates_and_reuses_persistent_session() -> None: assert session.start.await_count == 2 +async def test_start_container_passes_unsafe_extra_run_args_verbatim() -> None: + tool = DockerShellTool(unsafe_extra_run_args=("--network", "host")) + proc = _FakeProcess(returncode=0) + + with patch( + "agent_framework_tools.shell._docker.asyncio.create_subprocess_exec", + AsyncMock(return_value=proc), + ) as create_proc: + await tool._start_container() + + assert create_proc.await_args is not None + argv = create_proc.await_args.args + assert argv[argv.index("--network", argv.index("--network") + 1) + 1] == "host" + + async def test_close_terminates_session_and_container() -> None: tool = DockerShellTool() tool._container_started = True @@ -620,8 +438,13 @@ async def test_run_stateless_builds_expected_argv() -> None: assert result.timed_out is False -async def test_run_stateless_places_extra_args_after_isolation_defaults() -> None: - tool = DockerShellTool(mode="stateless", image="alpine:3", shell="sh", extra_run_args=("--label", "team=af")) +async def test_run_stateless_places_unsafe_extra_args_after_isolation_defaults() -> None: + tool = DockerShellTool( + mode="stateless", + image="alpine:3", + shell="sh", + unsafe_extra_run_args=("--label", "team=af"), + ) proc = _FakeProcess(returncode=0, communicate_results=[(b"", b"")]) with patch( @@ -640,6 +463,27 @@ async def test_run_stateless_places_extra_args_after_isolation_defaults() -> Non assert argv.index("--label") < argv.index("alpine:3") +async def test_run_stateless_passes_unsafe_extra_run_args_verbatim() -> None: + tool = DockerShellTool( + mode="stateless", + image="alpine:3", + shell="sh", + unsafe_extra_run_args=("--entrypoint", "/bin/custom"), + ) + proc = _FakeProcess(returncode=0, communicate_results=[(b"", b"")]) + + with patch( + "agent_framework_tools.shell._docker.asyncio.create_subprocess_exec", + AsyncMock(return_value=proc), + ) as create_proc: + await tool._run_stateless("echo hi", timeout=5.0) + + assert create_proc.await_args is not None + argv = create_proc.await_args.args + assert argv[argv.index("--entrypoint") + 1] == "/bin/custom" + assert argv.index("--entrypoint") < argv.index("alpine:3") + + async def test_run_stateless_timeout_reaps_container_when_kill_fails() -> None: tool = DockerShellTool(mode="stateless") command_proc = _FakeProcess( From 318cfc36ea0bc6fbb9434f699b663111c043fb49 Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Fri, 16 Oct 2026 11:59:33 +0000 Subject: [PATCH 2/2] Fix build errors --- .../Microsoft.Agents.AI.Tools.Shell/DockerShellExecutor.cs | 7 +++---- .../DockerShellExecutorTests.cs | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutor.cs b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutor.cs index e83a459bd7a..7a6206d9dfe 100644 --- a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutor.cs +++ b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutor.cs @@ -575,15 +575,14 @@ internal IReadOnlyList BuildRunArgvPersistent() => this._memoryBytes, this._pidsLimit, this._containerWorkdir, this._hostWorkdir, this._mountReadonly, this._readOnlyRoot, this._env, this._unsafeExtraRunArgs); - private static void ValidateUnsafeExtraRunArgs(IReadOnlyList args) + private static void ValidateUnsafeExtraRunArgs(string[] args) { - for (var index = 0; index < args.Count; index++) + for (var index = 0; index < args.Length; index++) { if (args[index] is null) { throw new ArgumentException( - $"{nameof(DockerShellExecutorOptions.UnsafeExtraRunArgs)} contains a null token at index {index}.", - nameof(DockerShellExecutorOptions.UnsafeExtraRunArgs)); + $"{nameof(DockerShellExecutorOptions.UnsafeExtraRunArgs)} contains a null token at index {index}."); } } } diff --git a/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/DockerShellExecutorTests.cs b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/DockerShellExecutorTests.cs index 08511229654..9f4a82896c2 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/DockerShellExecutorTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/DockerShellExecutorTests.cs @@ -134,7 +134,7 @@ public void Ctor_RejectsNullUnsafeExtraRunArg() var exception = Assert.Throws(() => new DockerShellExecutor(options)); // Assert - Assert.Equal("UnsafeExtraRunArgs", exception.ParamName); + Assert.Contains("UnsafeExtraRunArgs", exception.Message, StringComparison.Ordinal); Assert.Contains("null token", exception.Message, StringComparison.Ordinal); }