Repository navigation
docs(adr): add ADR-0035 host-authoritative sandbox configuration - #504
Conversation
Records the principle that acq must not rely on guest-generated or guest-tamperable state to configure the sandbox: because the in-sandbox agent has passwordless sudo, guest root-ownership is not a trust boundary against a prompt-injected agent. The only enforceable boundary is host<->guest. acq-trusted inputs (data AND code) must be either host-resident and mounted read-only into the guest, or held on the host and injected at exec time -- never read back from the guest and treated as authoritative. Introduces a per-sandbox host config directory mounted read-only, and trusted startup execution invoked from that read-only mount (three options considered; option 1 -- mount kit code read-only and invoke from the :ro path -- selected). Documents the staged migration of the existing /var/lib/acq markers (agent, workspace, ssh-auth-sock, kit-env, and the presence gates) off guest-writable paths. First consumer: patterns ADR 0003 (neutral model-provider discovery). Includes two Mermaid sequence diagrams (GitHub/Host/Guest boxes) contrasting the rejected guest-write path with the chosen host-materialized read-only path. Co-authored-by: OpenCode [claude_4_8_opus] <bret.mogilefsky@gsa.gov>
…t (ADR-0030) Introduce a host-side per-sandbox config store (acq_host_config_* in common.sh) keyed like the provenance/cli-kits records, and mount it into the msb guest READ-ONLY at ACQ_HOST_CONFIG_GUEST_DIR (default /var/lib/acq/host) via --volume host:guest:ro. This is the mechanism ADR-0030 defines: acq's trusted per-sandbox config lives on the host, and where the guest must read it, it sees a read-only mount the VMM enforces — a passwordless-sudo agent can read but cannot forge it. This commit adds only the store + mount plumbing and terminate-time cleanup on both backends; subsequent commits migrate the individual /var/lib/acq markers (agent, workspace, ssh-auth-sock, kit-env, and the presence gates) off the guest-writable paths onto this store. Refs: #503 Co-authored-by: OpenCode [claude_4_8_opus] <bret.mogilefsky@gsa.gov>
…ig store (ADR-0030) Move the four content-bearing /var/lib/acq guest markers onto the host-authoritative config store so a passwordless-sudo agent can no longer tamper the values acq reads back to configure the sandbox: - agent -> which binary a name-only re-attach launches - workspace -> the session cwd (-w) - ssh-auth-sock -> the injected SSH_AUTH_SOCK (git/ssh signing) - kit-env -> kit environment[] replayed as -e NAME=value each session acq now writes these to acq_host_config_* on the host at provision/apply and reads them back from the host on every session verb (run/attach/shell) — no guest `msb exec cat` happens for them. The kit-env NAME re-validation and the agent safe-token guard are preserved for defense-in-depth. Session verbs still survive an absent value under set -euo pipefail (the host read returns empty, not a failing pipeline). Tests: the offline stub library gains seed_host_config (seeds the host store from the STUB_RECORDED_* knobs, exported into subshells); the guest-cat stub arms for these markers are removed; provision assertions read the host store. Refs: #503 Co-authored-by: OpenCode [claude_4_8_opus] <bret.mogilefsky@gsa.gov>
Move the four presence-only idempotency gates off guest /var/lib/acq markers onto the host-authoritative config store, so a passwordless-sudo agent can no longer pre-create a marker to SUPPRESS a provisioning step: - install-<cksum> (per kit install command, run-once) - agent-installed-<agent> (agent npm install, run-once) - agent-user-ready (agent user + sudoers setup) - oci-ready (podman/OCI engine install) Each gate is now acq_host_config_has/_write on the host instead of a guest `test -f`/`touch`. Behavior is otherwise unchanged (still fail-soft, still re-runs the cheap idempotent device-node grant / shell-sync outside the gate). Tests: seed_host_config_gates seeds agent-user-ready/oci-ready from STUB_AGENT_USER_READY/STUB_OCI_READY; the guest test -f arms for these markers are removed from the stub; gate assertions read the host store. With this, no /var/lib/acq guest marker is written or read by acq anymore. Refs: #503 Co-authored-by: OpenCode [claude_4_8_opus] <bret.mogilefsky@gsa.gov>
…030) Implements ADR-0030 Mechanism 2. A kit files[] entry marked `readonly: true` carries trusted CODE that acq must run from the host-authoritative read-only mount, not a guest-writable copy a passwordless-sudo agent could tamper between restarts. - kit_spec_files surfaces a 5th tab field (readonly), default empty — 4-field consumers and unflagged files are unaffected. - _acq_msb_apply_kit_dir stages a readonly file onto the per-sandbox host config dir (kit-files/<path-slug>.<crc>, cp 0555) instead of `msb copy`ing it into the guest, and records a guest-path -> :ro-path rewrite. - _acq_msb_run_commands and the staged --script-path body (_acq_msb_startup_body_into) rewrite any startup argv token that EXACTLY equals a readonly file guest path to its :ro path (whole-token match; a --flag data path is left alone). So both the exec heal and a native replay run the :ro copy. - Identification is declared (readonly: true), never inferred from argv, so a data file the command also reads (opencode.jsonc) stays a normal guest drop. Backward compatible: no readonly files -> empty rewrite table -> unchanged behavior. Adoption of the field by the patterns-repo kits (usai-provider merge script, ADR 0003 provider normalizers) is additive and independently deployable. New suite 79-msb-readonly-kit-files.bats covers field surfacing, host-staging vs guest-copy, argv rewrite on both the exec and staged-body paths, whole-token matching, and the :ro mount. Refs: #503 Co-authored-by: OpenCode [claude_4_8_opus] <bret.mogilefsky@gsa.gov>
…ration (ADR-0030) Update the design-note and doc references that named the guest /var/lib/acq markers (agent, workspace, ssh-auth-sock, kit-env, install-*, oci-ready) to reflect that ADR-0030 moved them to acq's host-authoritative per-sandbox config store. Behavior is unchanged; only the storage location and its trust boundary moved. No bare tracker refs added. Co-authored-by: OpenCode [claude_4_8_opus] <bret.mogilefsky@gsa.gov>
Update — implementation of the migration (#503) added to this PRThis PR now carries both ADR-0030 and its implementation on the msb backend, per the request to land the ADR with the migration. The ADR is no longer docs-only. Commits (one per change, as planned in #503)
Verification
Cross-repo dependency (tracked)The RollbackRevert the 5 Security impactRemoves acq's reliance on guest-writable state for sandbox configuration under passwordless sudo (AC-6, SI-7, SI-17). Passwordless sudo itself is unchanged — this narrows what acq trusts, not what the agent can do in its sandbox. AI-assisted (OpenCode). See the commits' |
CI pre-commit shellcheck flagged two errors introduced by the ADR-0030 work: - SC2316 (acq.backends/msb.sh): `local ... readonly ...` applies `local` to a variable named `readonly`, which shellcheck reads as the readonly builtin. Rename the loop variable to `_readonly`. - SC2144 (test/bats/72, 78): `[ ! -f "$DIR"/msb/<name>.*.config/<key> ]` uses a glob with -f, which does not work as intended. Replace with a globbed `ls` under `run bash -c` asserting empty output (the key is absent). Behavior unchanged; offline bats suite still green (72/78/79/73/115). Co-authored-by: OpenCode [claude_4_8_opus] <bret.mogilefsky@gsa.gov>
|
AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed at head This is the right principle and the migration is careful. I went looking for the ways a host-authoritative mechanism usually fails and mostly did not find them — details in the verified list below, because "I checked X and it holds" is worth as much here as a finding. One substantive issue, and one documentation gap where I was wrong and the live test says so. The
|
Address adversarial review findings on PR #504: - Make the host-config mount root traversable by the guest agent (0711) while keeping flat acq-internal keys private (0600). readonly kit payloads under kit-files/ are now in a traversable directory and staged 0555, so startup code rewritten to /var/lib/acq/host/kit-files/... can actually be read/executed by user=agent through the :ro mount. - Detect existing msb sandboxes that predate the ADR-0030 /var/lib/acq/host:ro mount. If the mount is absent or not traversable, do not rewrite readonly startup argv to a missing path; warn and fall back to the legacy guest copy so old sandboxes keep working until recreated. Tests: extend 79-msb-readonly-kit-files for traversability modes and legacy fallback. Targeted bats suites 79/72/78/73/115 pass locally. Refs: #503 Co-authored-by: OpenCode [claude_4_8_opus] <bret.mogilefsky@gsa.gov>
|
Follow-up at head The legacy fallback is done right
That is the degraded-path rule applied correctly: the weaker mode is reachable, loud, and distinguishable from the strong one. The staging-failure branch now also falls back rather than leaving a startup argv pointed at a path that doesn't exist, which was the real bug. New:
|
Clear stale host-side instance state before fresh msb provisioning, verify the host-config mount refuses guest-root writes before trusting readonly staged kit code, and tighten staged kit-file directory listability. Co-authored-by: OpenCode [gpt_5_5_default_v2] <bret.mogilefsky@gsa.gov>
|
Update: addressed the review feedback in e9c6183. Changes since the last review:
Verification:
Please re-review when you have a chance. |
The fresh-provision stale-state regression must prove a stale ssh-auth-sock value is cleared, but it should not require the CI host to expose SSH_AUTH_SOCK and create a replacement value. Co-authored-by: OpenCode [gpt_5_5_default_v2] <bret.mogilefsky@gsa.gov>
Heads-up: ADR numbering contention with the msb version-pin workWhile investigating why For context on what #510 turned into: the original "block Two findings there touch this PR's neighbourhood. First, |
Corrections found by a 5-role pr_review panel plus independent verification (acq internals, cross-repo PR status), then confirmed via a 7-role higher_order consensus vote (85.7% approve) on the fix approach itself: - Consequences/Positive claimed "one shared config-merge/bounded-fetch library replaces the per-kit copies" — false against the ADR's own Decision. Layer 3 mandates each harness kit render its own native config; only the bounded-fetch helper is actually shared. Corrected the claim and stated explicitly that no shared config-merge library exists. - Added a security invariant, in three places (Layer 2 output description, Layer 3 read-list, Security model bullet list): the guest-writable aggregate catalog.json carries only model metadata, never routing/credential fields (host/baseUrl/modelsUrl/keyEnv). Those fields are read exclusively from the Layer 1 read-only facts mount. Without this, a sudo-capable in-guest agent could rewrite the catalog to pair one provider's keyEnv with another provider's allowlisted host, bypassing the Layer 1 env-var-ownership and SSRF checks entirely within caps.network.allow. - "quickstart ADR-0030" and "goose-server ADR 0004" were cited as settled sibling ADRs. Both are, as of this commit, open and unmerged (GSA-TTS/agentic-coding-quickstart#504, #415 — freshly reverified via `gh pr view` before this commit). Added "(pending — org/repo#N)" qualifiers per the repo's fully-qualified cross-repo reference convention. Consensus panel also flagged the Layer 1 artifact-location schema gap (no field locates a provider's normalizer or vendored snapshot) as a design change, not a textual fix, and recommended landing it in its own commit for separate review. That follows in the next commit. markdownlint: 0 issues. Co-authored-by: OpenCode Agent <agent@gsa.gov>
ADR-0030 was double-claimed. #474 keeps ADR-0030 because patterns main already cites it for the devenv ADR. Co-authored-by: OpenCode [gpt-5.5] <bret.mogilefsky@gsa.gov>
|
The aforementioned ADR numbering contention has been resolved. |
…tative-config-1 # Conflicts: # acq.backends/msb.sh # docs/adr/0011-msb-backend-and-neutral-kits.md # test/bats/78-msb-kit-env.bats
|
AI-assisted note (OpenCode), advisory. Checked for a rebase/merge-conflict assessment before doing a content review of the current head.
Three real conflicts, one of which is a substantive design collision rather than simple churn:
Not blocking anything new on my end — flagging so you have the collision context before resolving the conflict yourself, rather than me guessing at #2's correct resolution with less context than you have. I'll hold off on a substantive content/security review of this PR until it's green against main again, then do a full pass (I have review history here — two prior advisory rounds in September — so I'll check whether that feedback still holds at the new head too, not just review it fresh). |
|
Resolved current-main conflict review in commit 256d069. Kept ADR-0035 host-authoritative config and main OCI retirement: no adapter Podman provisioning or oci-ready marker; opt-in oci-engine kit remains. Restored read-only host-config mount and trusted startup staging, using 0711 traversal plus a guest-root write refusal probe. MSB cleanup now removes host config only after confirmed absence and retains it on an indeterminate list probe. Verification passed: bash -n, ShellCheck, git diff --check, and the full ./scripts/test-acq-bats suite. Two adversarial reviews were run; their findings were fixed and regression-tested. Markdownlint/gitleaks were unavailable locally; live backend validation requires a sandbox-capable host. AI-assisted (OpenCode); human review remains required before merge. |
|
AI-assisted re-review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed head The conflict resolution is correct, and it's the resolution I was hoping for rather than the naive one. I verified the specific risk I flagged: zero matches for One finding, reproduced and fix-verified. Everything else I probed holds. Should-fix:
|
|
Question about sandboxes created before this lands. I read the diff but haven't run it. The readers now go to the host config store only, and I don't see a step that copies the existing guest
Is this intended, with recreate as the answer for existing sandboxes, or should there be a one-time import of the legacy guest records (read once, charset-guarded as before, written to the host store, then never read from the guest again)? The import would keep the trust change for new state while avoiding a silent behavior change on upgrade. Either way, a release note or KNOWN_FAILURE_MODES entry would help. Our team runs long-lived msb sandboxes with Paseo, which relies on the replayed kit env (ADR-0033), so we'd test that path before rolling the upgrade out. |
|
Two small test-side issues I hit running the suites locally on macOS at 256d069:
|
|
Confirming @basiliskus's migration-gap finding with line references — it reproduces on inspection, and the fix shape already exists in this file. The asymmetry is real. Two readers of the same
agent=$(acq_provenance_field msb "$name" agent) # provenance first
if [ -z "$agent" ]; then
agent=$(acq_host_config_read msb "$name" agent …) # host-config fallback
fi
agent=$(acq_host_config_read msb "$name" agent | tr -d '[:space:]')
if [ -z "$agent" ] || ! _acq_msb_safe_agent_token "$agent"; then
agent="shell"
fiFor a sandbox provisioned on 4.0.1 the host-config key does not exist, Same shape in On my own review: I checked that failed writes degrade safely (failed write → absent marker → install re-runs, which is the safe direction) and reported that as clean. I did not check the absent-key-on-upgrade read path, which is the more consequential case because it applies to every sandbox created before the migration rather than only to a write failure. basiliskus's catch is the better one. Two things that follow from it, for whoever takes the fix:
Not re-reviewing the rest — my |
|
Addressed the remaining review feedback in 79743d5 ( @wz-gsa: adopted the GNU/BSD @basiliskus and @wz-gsa: the pre-ADR-0035 migration gap was valid. I did not import
Regression coverage now proves legacy agent/workspace recovery, host-config precedence over stale provenance, the helper unset-fixture path, and portable readonly file-mode assertions. Verification transcript:
Could @wz-gsa and @basiliskus please re-review the updated head, particularly the provenance-only legacy fallback and upgrade behavior? AI-assisted (OpenCode); human review remains required before merge. |
|
Verdict: comment — Prior finding: resolved at
Nits: The legacy-session fix is correctly ordered: host config wins, with trusted host provenance used only when the new key is absent. The ADR-0035 overstates the read-only mount's guarantee at Also add a workspace precedence test with conflicting host-config and provenance values; the current legacy workspace test covers fallback, not ordering. Assisted-by: OpenCode [gpt-5.6-sol-latest-guardrails-defaultv2] skill:change-review |
wz-gsa
left a comment
There was a problem hiding this comment.
Re-reviewed the current head 79743d5ea, specifically the legacy-sandbox migration gap we confirmed at 256d06958. The fix now gives the new host-config store precedence, then falls back to trusted host provenance for pre-ADR-0035 sandboxes; it never imports guest markers. _acq_msb_attach now consumes acq_backend_recorded_agent, and _acq_msb_workspace_for falls back through acq_workspace_record_read, canonicalizing when available before /home/agent. The tests pin all three contracts: a legacy sandbox resumes with its recorded agent/workspace, host-config overrides stale provenance, and acq exec uses the legacy workspace. The test-helper return 0 fixes the set -e degraded path rather than hiding it, with its own regression test. All CI gates pass, including the full bats build job under bash 3.2. No remaining findings from me. AI-assisted review (OpenCode), formally approved with the maintainer’s explicit authorization; a human owns the merge.
Fixes the reboot case of #501 (follows #413). ## Problem The automatic ssh-agent `--vsock` route pins the create-time `SSH_AUTH_SOCK` path. On macOS the launchd agent socket path changes on every login, so after a host reboot every existing msb sandbox has a dead route: `ssh-add -l` in the guest reports "communication with agent failed" and SSH commit signing fails. Until now the only remedy was `acq rm` + recreate. ## Change msb dials the route's `host_socket` per guest connection and follows symlinks (verified on msb 0.7.7). acq now emits a per-sandbox symlink, `${ACQ_STATE_DIR}/msb/ssh-agent/<sandbox>.sock`, as the route's host path and atomically re-points it at the current `SSH_AUTH_SOCK` on every bridge (re)start: provision, `acq start`, and the running re-attach re-drive. `acq rm` removes the link. - Custom `ACQ_FORWARD_HOST_SOCKETS` entries keep today's canonicalized behavior. - Sandboxes created before this keep their raw route (acq cannot change a persisted route). The unreachable-agent warning now says "recreate once" for them, and points at the host agent for a managed route. - A cksum-derived basename keeps the link inside `sun_path`; an unsafe sandbox name or a failed symlink falls back to the raw path (today's behavior). - sbx is unchanged. Docs: ADR-0021 amendment, `docs/KNOWN_FAILURE_MODES.md` §35, `docs/BACKEND_GUIDE.md`. ## Verification - `test/bats/115-ssh-agent-forward.bats`: new cases 10c22–10c30; 34/34 pass. - Full offline suite (after merging `main` at `0e4bccb`): 732 ok, 9 not ok. The same 9 fail on a clean export of `main` in the same environment (TMPDIR/volume-path and attach-line assertions), so they are pre-existing. - shellcheck (`--severity=warning`) and the pinned markdownlint pass. - Live on macOS/HVF, msb 0.7.7, throwaway agent and key: route persisted as the link path; re-pointing the link to an empty agent and back changed guest `ssh-add -l` with no restart; after `acq stop`, killing the agent, starting a new agent at a different socket path and `acq start`, the guest listed the key and `git commit` produced a good signature with no recreate; `acq rm` removed the link. ## Follow-ups (not in this PR) - #501 can be narrowed to the `acq restore` path only. - #504 has landed; `main` is merged into this branch (`cf8c02e`). The `acq_backend_terminate` conflict keeps both cleanup calls, and the bridge-restart blocks of 10c24, 10c25 and 10c30 seed the host-config record. `115-ssh-agent-forward.bats` passes 34/34. - Piping `acq create`/`acq start` stderr hangs because the msb VM process inherits fd 9; tracked in #568. AI-assisted. --------- Co-authored-by: William Zujkowski <153217227+wz-gsa@users.noreply.github.com>
Context
Adds ADR-0035 (Host-Authoritative Sandbox Configuration). This is a docs-only PR (the ADR itself); the acq-side implementation is tracked in #503.
acq deliberately gives the in-sandbox agent passwordless sudo (the sandbox is the security boundary, per ADR-0005/0011). A consequence not previously stated as a principle: inside the guest, root-ownership is not a trust boundary against the agent — a prompt-injected agent can
sudoto root and rewrite any guest file, including markers under the root-owned/var/lib/acq/.acq currently reads four content-bearing
/var/lib/acqmarkers back and acts on them to configure the sandbox (agent,workspace,ssh-auth-sock,kit-env), and re-runs guest-resident kit startup code across restarts. Both are guest-tamperable under sudo.This surfaced concretely in review of GSA-TTS/agentic-coding-patterns#423 (neutral model-provider discovery), whose provider-facts channel had originally claimed a root-owned guest path was "not agent-writable."
What this ADR records
/var/lib/acqmarkers off guest-writable paths (tracked in acq: host-authoritative sandbox config — migrate guest-writable state off /var/lib/acq (ADR-0030) #503).Verification
markdownlint-cli2(repo config, v0.22.1): 0 issues.Rollback
Docs-only. Revert the single commit / delete
docs/adr/0035-host-authoritative-sandbox-config.md. No code paths change.Security impact
Documentation of a security-hardening direction; no runtime change in this PR. The implementing change (#503) narrows what acq trusts; it does not alter the agent's in-sandbox privileges.
AI-assisted (OpenCode). See the commit's
Co-authored-bytrailer.