Repository navigation
refactor(sandbox): container hardening is copy-pasted across three Docker clients, with a docstring as the only thing preventing drift #17267
Description
Problem
Three modules each open their own Docker client and each re-declare the container hardening flags:
| Module | Client |
|---|---|
autobot-backend/secure_sandbox_executor.py:132 |
docker.from_env() |
autobot-backend/services/execution/docker_backend.py:96 |
docker.from_env(timeout=10) |
autobot-backend/services/docker_task_workspace.py:196 |
docker.from_env() |
The constraint set — network_mode: "none", read_only, no-new-privileges, cap_drop: ALL, memory/CPU limits — is duplicated rather than shared.
The codebase already knows this is a risk
services/docker_task_workspace.py's own docstring states it "copies the exact constraint set" from the sandbox executor "so the two paths can never diverge independently". That is drift being prevented by hand, in a comment. A shared constant would make divergence impossible instead of merely discouraged — and a security constraint that relies on a human noticing a copy is the kind that silently weakens on the third copy.
Scope note
This is consolidation of an existing, working constraint set — not a behaviour change. Per the project's canonical-source discipline, one definition, three consumers.
Acceptance criteria
- The container security constraints are defined once and imported by all three call sites
- A test asserts every container-creating path applies the shared constraints
- Adding a new container-creating module without the shared constraints fails a guard
- No behaviour change: the resulting flags are identical to today's on each path
Refs #17265.
Activity
mrveiss commentedon Oct 10, 2026
Two of three delivered on branch
issue-17267-container-hardening(d61381f411, queued).secure_sandbox_executoranddocker_task_workspacenow take their hardening fromautobot_shared/container_hardening.hardening_kwargs. The kwargs reaching docker are identical before and after, pinned by a test against the old literals.Decision needed (owner), and it's a security finding rather than a copy: the third client,
services/execution/docker_backend.py(containers.runat :272 and the snapshot-restore at :488), never had any hardening: nocap_drop, nono-new-privileges, noread_only, no network isolation, no pids limit, no swap cap. Code run through that backend runs in a default-privileged container. Adopting the shared builder makes it stricter and changes what that code can do (e.g. no network forpip, read-only rootfs):cap_drop=['ALL'],no-new-privileges, a pids limit). No network or filesystem change.network_modeand a writable tmpfs plan.services/execution/container_pool.py:207also callscontainers.runwith caller kwargs and still needs a look.mrveiss commentedon Oct 10, 2026
Owner decision: (c).
services/execution/docker_backend.py(containers.runat :272 and the snapshot-restore at :488) adopts the non-behavioural subset through the sharedhardening_kwargs:cap_drop=['ALL'],no-new-privilegesand a pids limit. Network and filesystem stay unchanged. Full hardening (option b) remains open as a follow-up on this issue.container_pool.py:207still needs the same review. Recorded by the merge coordinator on the owner's instruction.mrveiss commentedon Oct 10, 2026
Option (c) applied, and the
container_pool.py:207reviewBranch
issue-17267-container-hardening, local commit9ef024a5a7(on top ofd61381f411). This is option (c) only. Option (b), full hardening, stays open.Per-site change. Each site gains exactly three keys from the shared builder:
security_opt=["no-new-privileges"],cap_drop=["ALL"],pids_limit=512. Network, read_only, volumes, memory, cpus, user and environment are unchanged at every site. Per-site tests pin the full kwargs before and after.services/execution/docker_backend.py:272(cold start)detach, mem_limit, cpus, environment, stdout, stderr, remove=Falseservices/execution/docker_backend.py:488(snapshot restore)detach, remove=Falseservices/execution/container_pool.py:207(warm pool)detach, remove=False, and no caller-supplied hardeningWhy
container_poolneeded it, with evidence.WarmContainerPoolis constructed in exactly one place, byContainerPoolRegistry(container_pool.py:283), whosedefault_container_kwargsdefaults to{}(:268,:272). Its only caller,docker_backend.py:122, callsContainerPoolRegistry(docker_client=..., pool_size=...)and passes no kwargs. So pooled containers ran with neithercap_dropnorno-new-privileges. The floor is merged last ({**caller_kwargs, **baseline_hardening_kwargs()}), so a caller cannot weaken it, while other caller keys survive.Shared builder.
autobot_shared/container_hardening.pygainsbaseline_hardening_kwargs(); the existinghardening_kwargs()composes it and its output is unchanged. The pids limit moved into that module asCONTAINER_PIDS_LIMIT. It still reads the existingAUTOBOT_WORKSPACE_PIDS_LIMITenv var (default 512), which now governs the execution-backend containers too.Open points
read_only. Those belong to option (b).mrveiss commentedon Oct 10, 2026
Owner decision (2026-10-10): option (b), full hardening for the pooled and execution containers: a memory limit, network disabled by default with explicit per-workload opt-in, a read-only root filesystem with tmpfs for scratch. Opt-in flags are named and audited; defaults are hardened.
mrveiss commentedon Oct 10, 2026
Status note: AC4 is overtaken by the owner's decision (b), which applies full hardening rather than the narrower (c) subset. AC3, the guard covering every container-creating site, is met only once the guard runs over the whole tree and matches
createas well asrun(in progress on the branch). Real-container verification is tracked in #18211.chore(merge): bring #17267 hardening branch current with main (#17267)