feat(agent-sandbox): log denied-command attempts via hardened-image shim (#2662 option 2)

[Carlos Hernandez (carloshvp)]

Summary

Option 2 of tracker #2662: convert silently-blocked denied commands in the hardened sandbox image into logged denials, so an agent's attempt to shell out to a dangerous CLI is recorded — not just refused.

Option 1 (the minimal-PATH image, #2713) already prevents denied network/infra CLIs, but a bare "command not found" / EACCES produces no AGT signal. The issue is explicit: "detecting the attempt matters as much as preventing the outcome."

What changed

• New docker/agt-deny-shim.py — a stdlib-only Python logging shim. On invocation it writes a structured command_denied JSON record to stderr (captured in SandboxResult.stderr), optionally appends it to $AGT_DENIED_LOG, and exits 126 so the real command never runs.
• docker/Dockerfile.sandbox — a new stage routes the denied network/infra CLIs (curl, wget, ssh, git, az, aws, gcloud, kubectl, terraform, helm, ansible, apt, …) to the shim, installed both at each binary's real path (explicit-dir iteration, so absolute-path calls are caught) and under its name in the pinned PATH dir (so by-name calls are logged instead of failing silently). Extend via the DENIED_LOGGED_BIN_NAMES build-arg.
• README.md — documents the logging-deny behavior.
• tests/test_deny_shim.py — Docker-free tests.

Design notes

• Why Python, not a shell script: the image strips the execute bit off every shell (sh, bash, …), so a shell shim could not run; python3 is an allowed interpreter. Shells, interpreters, and encoders stay execute-bit-stripped — disabled but not logged; the network/infra tooling is what carries the compliance signal.
• Fail-closed: a missing/unwritable $AGT_DENIED_LOG never turns a denial into a hard error; the command is still denied.
• Scope: this is the image-level (option 2) layer. Option 4 (a Python runtime interception shim) was evaluated and dropped as substantially redundant with the existing static scanner (code_scanner.py), which already host-side blocks subprocess and process-spawning calls in sandboxed code. Option 3 (AppArmor) remains open on the tracker.

Testing

tests/test_deny_shim.py — 10 tests covering the shim (exit 126, structured + human-readable records, argv[0]-derived binary name, audit-log append, unwritable-log resilience) and the Dockerfile wiring (parsed as text). Existing TestMinimalPathSandboxImage still passes (15 passed total).

Note: Dockerfile.sandbox is an opt-in image that is not built in CI (the provider falls back to python:3.11-slim when it is absent), so the image wiring is validated by inspection + the text-level Dockerfile tests, and the shim behavior by the Docker-free unit tests.

Refs #2662 (tracker — does not close it).

🤖 Generated with Claude Code

[github-actions]

PR Review Summary

Check	Status	Details
🔍 Code Review	⚠️ Missing	No current-run comment
🛡️ Security Scan	⚠️ Missing	No current-run comment
🔄 Breaking Changes	⚠️ Missing	No current-run comment
📝 Docs Sync	⚠️ Missing	No current-run comment
🧪 Test Coverage	⚠️ Missing	No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

[Imran Siddique (imran-siddique)]

REQUEST_CHANGES — 3 blockers, 5 required fixes, 2 nits. The shim design is sound and the stdlib-only constraint is met. Address these before merge.

Blocker 1 — Schema break: ts vs timestamp

command_denied records use "ts" but every other AGT audit record (compliance.py, flight_recorder.py, time_travel_debugger.py) uses hard bracket e["timestamp"]. A mixed audit log raises KeyError on every shim-sourced entry. Rename to "timestamp".

Blocker 2 — EACCES contract silently broken

Stage 2 strips execute bits to guarantee PermissionError on absolute-path calls. Stage 3's ln -sf to a chmod 0755 shim restores executability — the error changes from PermissionError (EACCES) to exit 126. Callers that catch OSError/PermissionError for the block signal stop seeing denials. Either document this contract change with a migration note, or update existing callers that gate on PermissionError.

Blocker 3 — AGT_DENIED_LOG follows symlinks unconditionally

open(log_path, "a") follows symlinks. If the orchestrator or a compromised env injects a symlink for AGT_DENIED_LOG, the shim appends to whatever it resolves to. Fix: validate the path stays within an expected prefix, or use os.open with os.O_WRONLY | os.O_CREAT | os.O_APPEND | os.O_NOFOLLOW.

Required fix 1 — Glob injection in Dockerfile for-loop

for bin in $DENIED_LOGGED_BIN_NAMES with set -eux but no set -f: glob metacharacters in a custom --build-arg expand against the filesystem. Add set -f before the loop.

Required fix 2 — ln -sf into sandbox-bin overwrites allowed binaries

A binary in both ALLOWED_BIN_NAMES and DENIED_LOGGED_BIN_NAMES is silently overwritten with the shim. Add [ -e "/usr/local/sandbox-bin/$bin" ] && continue before the ln -sf.

Required fix 3 — Shebang fragility

#!/usr/bin/env python3 depends on /usr/bin/env being executable. Stage 2's chmod pass could strip it. Use an absolute shebang or have symlinks exec python3 /usr/local/lib/agt-deny-shim.py directly.

Required fix 4 — Non-atomic stderr writes

Two sequential sys.stderr.write() calls interleave with concurrent deny events. Merge into one write:

sys.stderr.write(line + "\n" + f"agt-sandbox: command denied: {invoked}\n")

Required fix 5 — Silent log failure

except OSError: pass drops the file-based log record with no signal. Replace with:

except OSError as exc:
    sys.stderr.write(f"agt-sandbox: warning: could not write AGT_DENIED_LOG: {exc}\n")

Nit 1 — Document the minimum required DENIED_LOGGED_BIN_NAMES set; a single-entry build-arg silently under-restricts the image with no warning.

Nit 2 — if argv and argv[0] is unreachable via main(sys.argv). Remove or add a direct test for main([]).

[Carlos Hernandez (carloshvp)]

Thanks Imran Siddique (@imran-siddique) — thorough review. Addressed in 77d58c0. Point by point:

Blocker 1 (ts → timestamp) ✅ Renamed; now matches the ["timestamp"] convention used across the audit records. Test updated.

Blocker 2 (EACCES → exit 126) ✅ Documented the contract change in the shim header, the DENY_EXIT_CODE comment, and the README ("Behavior change vs. the bare minimal-PATH image"). On the "update callers" half: I grepped agent-sandbox/src and there is no code that gates on PermissionError/OSError/EACCES as the denial signal, so there are no callers to update — sandboxed code is still denied either way (non-zero exit + record). Flagging in case you know of a consumer outside this package.

Blocker 3 (symlink-following on AGT_DENIED_LOG) ✅ Switched to os.open(..., O_WRONLY|O_CREAT|O_APPEND|O_NOFOLLOW, 0o600). Added test_audit_log_does_not_follow_symlink (planted symlink → target untouched + stderr warning).

Required 1 (glob injection) ✅ Added set -f before the loop.

Required 2 (allow-list clobber) ✅ Added [ -e "/usr/local/sandbox-bin/$bin" ] && continue — allow-list wins; documented in the README that a name shouldn't appear in both lists.

Required 3 (shebang fragility) ✅ Changed to the absolute #!/usr/local/bin/python3 (no env/PATH dependency). Tests invoke via the interpreter directly, so host portability is unaffected.

Required 4 (non-atomic stderr) ✅ Merged the structured record and human line into a single write().

Required 5 (silent log failure) ✅ OSError now emits agt-sandbox: warning: could not write AGT_DENIED_LOG: … on stderr; covered by the unwritable-path test.

Nit 1 (build-arg under-restriction) ✅ README now notes DENIED_LOGGED_BIN_NAMES replaces the default set.

Nit 2 (if argv and argv[0] unreachable) ✅ Kept the guard and added test_main_with_empty_argv_reports_unknown (direct main([]) call → binary: "unknown").

Tests: 17 passing (12 shim + 5 Dockerfile-text). The hardened image still isn't built in CI, so the image wiring remains validated by the text-level Dockerfile tests + the Docker-free shim tests. Ready for another look.

[Imran Siddique (imran-siddique)]

Thorough resolution — all 8 items addressed. The symlink-safe log write (O_NOFOLLOW), single atomic write() for stderr, absolute shebang, set -f before the loop, allow-list wins over deny-list, and the behavior-change documentation are all exactly right. 17 tests passing. Merging.