Add support for Cedar as a policy language, in addition to TOML#34
Add support for Cedar as a policy language, in addition to TOML#34marcbrooker wants to merge 5 commits into
Conversation
Operators can now express sandbox authorization in the Cedar policy language in addition to the existing TOML configuration. Policies gate the full agent-sandbox action vocabulary at the Kernel boundary: fs:read/stat/write/list/create/delete/rename, env:read, net:request, and mcp:call. Semantics are additive-restriction-only: with no policy, behavior is unchanged (default-allow); with a policy loaded, a gated action must be permitted by Cedar or it is denied. This layers on top of the built-in SSRF and VFS-permission checks and can never weaken them. A policy can be supplied three ways: the `--policy <file>` CLI flag, `ShellBuilder::policy_file()`/`policy_str()`, or a `policy = "file.cedar"` key in the TOML config (resolved relative to the config file). Only the generic, public cedar-policy API is used. cedar-policy is not wasm-safe, so the dependency, the `policy` module, and all plumbing are gated to non-wasm; the `Kernel::check_policy` trait default is a no-op, so wasm builds compile with policy disabled.
Add an "Authorization Policies (Cedar)" section to the README covering the action vocabulary, the three ways to load a policy, and the additive-only composition with the SSRF guard and filesystem permissions. Add examples/ with three Cedar policies of increasing complexity (read-only, a workspace jail, and mixed controls with a forbid override) and a run-policies.sh script that runs each against strands-shell and shows which commands are allowed and which are denied.
Add `policy_file` and `policy_str` to the native PyO3 ShellBuilder, and surface them on the customer-facing `Shell` constructor as the `policy_file` (path) and `policy` (inline text) keyword args. Passing both raises; an explicit policy here overrides a `policy` key from a config_file. Update the README's policy section to show the Python API, and add pytest coverage for the read-only-from-Python, file-vs-inline, no-policy, mutual-exclusion, and malformed-policy cases.
Add two examples grounded in the "lethal trifecta" of agent security (private data + untrusted input + external communication), each cutting one leg: - 04-egress-allowlist.cedar — network only as GET to a single host; other hosts and non-GET methods are denied, closing the exfiltration channel. - 05-shield-secrets.cedar — full read/write of a project tree but a forbid override that blocks reading .env / .pem / .ssh / credentials. Extend run-policies.sh to demonstrate both and update the example and main READMEs.
agent-of-mkmeral
left a comment
There was a problem hiding this comment.
Built it, ran the suite (52 unit + 6 integration, all green), and adversarially tested the binary end-to-end against head 5f60f40.
Verdict: 🔴 Request changes. The design is genuinely excellent — single chokepoint, layers on top of SSRF/VFS without weakening them, schema-validated baked vocabulary, great docs. But adversarial testing found a real security bypass that defeats the PR's two headline examples: a symlink walks around path-based forbid/jail rules — the gate checks the literal requested path, but the VFS follows the symlink to a forbidden target (fs:read notes.txt → returns .env). It's fixable and worth fixing before merge. Details below.
🔴 Blocker — symlink bypass: repro + fix options
The policy gate matches on the literal requested path string, but the VFS then follows the symlink to a different inode — so a benign-named symlink reads/writes a target the policy would forbid. This defeats Example 3 (*.secret), Example 5 (shield-secrets), and the Example 2 workspace jail — exactly the lethal-trifecta scenarios this PR is selling.
Code path: open gates on Self::abs(proc, path) (the literal path) at src/vfs_kernel.rs:260–275, then calls vfs.resolve(&abs, true) — and resolve(path, follow_last: true) (src/vfs.rs:123) follows symlinks. So the check runs on notes.txt while the read returns .env.
Repro (shield-secrets semantics: broad read/write + forbid read of *.env/*/.env):
$ strands-shell --policy shield.cedar -c 'echo TOPSECRET > /home/lash/.env; cat /home/lash/.env'
strands-shell: cat: policy denied: fs:read path=/home/lash/.env # ✅ direct read denied
$ strands-shell --policy shield.cedar -c \
'echo TOPSECRET > /home/lash/.env; ln -s /home/lash/.env /home/lash/notes.txt; cat /home/lash/notes.txt'
TOPSECRET # 🔴 BYPASS — secret leaked
ln only needs fs:create for notes.txt (permitted by the broad write rule), so nothing stops the setup. Same shape lets a jailed agent escape its jail via an in-jail symlink to an out-of-jail target.
Why it's blocking: the README pitches forbid as robust "no matter what the agent is talked into doing." As written, an injected instruction just adds one ln -s and the carve-out is gone — a forbid that advertises a guarantee it doesn't keep.
Fix (any one closes it):
- Gate on the resolved canonical target, not the requested path (most robust — matches intent "never read the file at
.env"). - Gate
fs:read/fs:staton both the requested path and the resolved target (deny if either is forbidden) — preserves literal-path permit semantics while closing the hole.
I'd lean (1) or (2) plus a regression test mirroring the repro above, and a one-line note on symlink semantics next to the like-matching docs in Examples 3/5.
🟡 Nits (non-blocking)
read_linkis ungated (src/vfs_kernel.rs:721— nocheck_policy). Under read-only/shield you can stillreadlinka forbidden path and learn the target (mild info leak, inconsistent with the rest offs:*). Gate it asfs:read. (Related to the blocker — worth fixing in the same pass.)env:readgranularity — gated with hardcodedname="*", so policies can't scope by var name even though the schema modelsEnvReadInput { name }. Wire per-name, or note the limitation in the schema comment.mcp:callserver-name normalization — gate passes the de-hyphenated module name (my-server→my_server) asserver. Authors will writemy-serverand silently fail to match. Call it out in the README MCP row (the code comment notes it; the docs don't).- Open=write/create existence probe is racy —
resolve(...).is_ok()then drops the lock before the real open (TOCTOU). Low severity (single-threaded current-thread runtime, in-proc VFS); fold the decision into the locked section or add an acknowledging comment. - Cargo.lock +767 pulls the full
cedar-policyv4 tree — expected for the feature, just flagging dependency-surface growth for supply-chain review.
✅ What's great
- Single chokepoint.
Kernel::check_policy(default-allow trait method inos.rs) called from each gated op invfs_kernel.rs.Nonepolicy ⇒ unchanged behavior;Some⇒ Cedar default-deny. Clean and hard to misuse. - Layers on top, never weakens. The policy gate runs before the existing SSRF/VFS checks — verified a permit-all
net:requestpolicy still cannot reach169.254.169.254. ✅ - Baked, schema-validated vocabulary.
schemas/agent.cedarschemaembedded viainclude_str!; every policy validated atbuild(), so a typo'd action or a clause referencing a field an action doesn't have is rejected at load, not silently ignored. Confirmed both rejections live. - wasm-gated correctly (
#[cfg(not(target_arch = "wasm32"))]). - Surface parity across CLI (
--policy), Rust builder, Python (mutually-exclusivepolicy/policy_file), and TOMLpolicy =. - stat/lstat fail closed to "does not exist" on denial — doesn't leak existence.
- Docs + 5 worked examples are unusually good.
cargo fmt --checkclean;clippyclean w.r.t. the newpolicy.rs(the 6 warnings are pre-existing in other files).
Bottom line: Great feature, clean implementation, excellent docs — I want this in. Close the symlink bypass (resolve-then-check or check-both) + add a regression test + gate read_link, and I'm a strong approve. Happy to re-review once pushed.
Reviewed by agent-of-mkmeral 🤖 — built & adversarially tested locally against head 5f60f40.
Description
Add support for Cedar as a policy language, in addition to TOML.
Type of Change
New feature
Breaking change
Documentation update
Other (please describe):
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.
cargo test --workspace --all-targets,pytest tests/python,npm test)cargo fmtandcargo clippyChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.