174 lines
11 KiB
Markdown
174 lines
11 KiB
Markdown
# AMD1213-D — transaction and helper trust remediation
|
||
|
||
- **Task:** AMD1213-D (issue #1213 amendment; controlling packet `comms/20260813T212447Z__from-scrappy__dc43de.md`)
|
||
- **Objective:** Address D1–D6 on local `feat/wf-fleet-mvp`, commit-only. Never push or re-author.
|
||
- **Scope:** Existing C-fence production/tests only. No provider calls.
|
||
- **Standing constraint:** AMEND/HOLD. Do not push, do not update PR #1213, do not merge, do not
|
||
re-author. PR #1216 remains independently held for Jason.
|
||
|
||
## Where this actually stands (measured 2026-08-15, not inherited from notes)
|
||
|
||
Everything below was re-measured against the tree rather than trusted from the previous entries,
|
||
which understated progress by roughly two defects. Branch head `3667a7a7`.
|
||
|
||
| Defect | State | Evidence |
|
||
|---|---|---|
|
||
| D1 transactional rollback | Substantially implemented | `fleet-launch-command.ts` snapshots links before mutating, prepares the manifest, and has `injectFailure` seams at `prepare-manifest`, `write-manifest`, `close-manifest`, `rename-manifest`; restores `manifestLinksBefore` on failure |
|
||
| D2 exact managed-link classes | Substantially implemented | classifier at `fleet-launch-command.ts:598-663`: exact resolved credential, direct one-component plugin/skill only, symlink target and ancestor rejected, `realpath` containment, duplicates rejected |
|
||
| D3 ambient-PATH executable resolution | **Closed** — executables `585dac7a`, environment `3667a7a7` | see below; one deliberate residual (HOME) |
|
||
| D4 config check/apply safety | Substantially implemented | `secure_dir` ancestor checks, `read_private` with `O_NOFOLLOW` + fstat, apply via mkstemp + fchmod 0600 + fsync + dev/ino re-check before `os.replace`, compatibility path separated |
|
||
| D5 bounded test seam | **Closed this pass** (commit `b91b702a`) | see below |
|
||
| D6 validate-by-path then exec-by-path | Substantially implemented | helper runs as a verified snapshot piped to `bash -s`, not executed by pathname; the same binding applied to the runtime in D3 |
|
||
|
||
### D2 — one thing worth recording so it is not "fixed" later
|
||
|
||
The alias concern in the packet (`duplicates/normalization aliases`) is closed by strictness, not
|
||
by normalization. `dirname(link)` is compared literally against the seat root, so `/s/plugins//foo`
|
||
(`dirname` → `/s/plugins/`), `/s/plugins/./foo` and `/s/plugins/bar/../foo` all fail the comparison
|
||
and are rejected. Verified by direct measurement of `path.dirname` on each form. Anyone who
|
||
"improves" this by normalizing the link first would open the alias hole the strict comparison
|
||
currently closes.
|
||
|
||
### D3 — what was wrong and what was done
|
||
|
||
The launcher asked `which` whether a runtime was reachable and then spawned the **bare name**,
|
||
letting the OS resolve it a second time against an ambient PATH at a later moment. Two independent
|
||
resolutions of an attacker-influenced name with a gap between them.
|
||
|
||
Demonstrated against the old code before changing it — a world-writable `codex` shim prepended to
|
||
PATH:
|
||
|
||
```
|
||
OLD checkRuntime -> PASSED (which found it)
|
||
OLD execRuntime -> "SHIM EXECUTED — this is not the real runtime"
|
||
```
|
||
|
||
Three exposed call sites, not one: `checkRuntime`'s `which`; `execRuntime` spawning
|
||
`codex`/`opencode` by name; and `execLeaseGatedRuntime` spawning `python3` by name — the
|
||
interpreter that starts the lease gate, where a shim replaces the process that enforces every other
|
||
check. `minimalLaunchEnv` copies ambient PATH straight through.
|
||
|
||
Fix: `resolveExecutableFromPath` searches only the PATH the child will actually receive, validates
|
||
what the search lands on (regular file, executable, not group/other-writable, owned by the
|
||
launching user or root, no group/world-writable non-sticky directory and no foreign-owned directory
|
||
on the resolved path), and returns that path pinned to dev/ino. Callers execute the returned path
|
||
and never the name again. The fleet lease-gate interpreter comes from the root-owned
|
||
`trustedCapability('python3')`. `checkRuntime` is deliberately kept on the operator path, where
|
||
"is it reachable from my shell" is the right question.
|
||
|
||
**Residuals, stated not engineered around:**
|
||
|
||
1. `assertUnchangedSinceValidation` re-confirms dev/ino immediately before spawn. That narrows the
|
||
validation→exec window; it does not close it. Closing it means exec by held descriptor, which
|
||
Node cannot do portably. Same accepted boundary already documented for the fleet helper.
|
||
2. For claude and pi the runtime binary is still re-resolved inside `launch-runtime.py` after the
|
||
trusted interpreter starts it. **This change does not cover that path.**
|
||
|
||
12 tests, one per hole. One was written wrong first and is worth remembering:
|
||
`mkdirSync(path, { mode: 0o777 })` is masked by the umask to 0o755, so the world-writable-directory
|
||
case passed while testing nothing. Create at 0o755, then `chmodSync`.
|
||
|
||
### D3 environment half — measured, and mostly already true
|
||
|
||
Measured before changing anything: the real `fleet launch` route with a shim in place of the
|
||
runtime binary, the shim dumping its own environment. The subject is therefore what arrives after
|
||
composition **and** after `launch-runtime.py` adds the lease variables — not the object the
|
||
launcher builds. Those are different sets.
|
||
|
||
The complete child environment for a composed claude seat:
|
||
|
||
```
|
||
PATH HOME USER LOGNAME SHELL TERM COLORTERM TMPDIR XDG_RUNTIME_DIR (inherited allowlist)
|
||
LANG LC_ALL (fixed, this pass)
|
||
CLAUDE_CONFIG_DIR MOSAIC_AGENT_NAME <profile env> (declared)
|
||
MOSAIC_LAUNCH_ID (minted per launch)
|
||
MOSAIC_LEASE_BROKER_SOCKET MOSAIC_LEASE_GENERATION_FILE
|
||
MOSAIC_LEASE_RUNTIME MOSAIC_LEASE_SESSION_ID
|
||
MOSAIC_RECEIPT_OBSERVER_SOCKET MOSAIC_RUNTIME_GENERATION (lease gate)
|
||
```
|
||
|
||
Most of the defect was already closed **by construction and untested**. `minimalLaunchEnv` builds
|
||
from an empty object over a fixed list, so `BASH_ENV`, `ENV`, `PYTHON*`, `NODE_*`, `NPM_CONFIG_*`,
|
||
`LD_PRELOAD`, `LD_LIBRARY_PATH` and provider credentials never reach the child. All sixteen were
|
||
planted; none survived, including through the lease gate. The gap was that nothing named the
|
||
allowlist, and an allowlist no test names is one careless edit away from being a denylist.
|
||
|
||
Fixed: **locale was inherited**, so the same seat emitted different message language, collation and
|
||
number/date formatting depending on who started it. Composed launches now pin `C.UTF-8` — not `C`,
|
||
which is ASCII and would mangle non-ASCII output. A profile-declared `LANG`/`LC_ALL` still wins,
|
||
and a test holds that escape hatch open. The operator path is untouched.
|
||
|
||
**Residual, deliberate — `HOME` is still the operator's.** The card is right that this is the
|
||
remaining leak: the runtime gets its own config dir, but anything it shells out to (git, ssh, npm)
|
||
reads the operator's dotfiles and therefore the operator's credentials. Not changed here, because
|
||
a seat whose HOME is a bare directory has no gitconfig and no ssh key, so it cannot commit or push
|
||
— and the fleet MVP's proof is a seat carrying a change to a pushed branch. Moving HOME before the
|
||
per-agent home is populated improves isolation and breaks the deliverable. **Owner: the
|
||
harness-homes design**, which is exactly the track that populates a per-agent home with its own
|
||
auth bundle. Do it there, not here.
|
||
|
||
Eight tests, each falsified by inverting the property it defends; every inversion hit only its own
|
||
test: `BASH_ENV` added to the inherited list → permitted-set + loader-hook killers red (2 failed);
|
||
locale pin reverted → locale killer red; ambient `MOSAIC_LAUNCH_ID` reused → launch-id killer red;
|
||
`process.env` recorded into the ledger → ledger-value killer red.
|
||
|
||
The permitted-name list in the spec is hand-written, not derived from the launcher. Deriving it
|
||
would make the test agree with the code by construction and detect nothing.
|
||
|
||
### D5 — what remained and what was done
|
||
|
||
Most of D5 was already closed: `launchFleetRuntimeForTest` is gone, specs enter through the real
|
||
`registerFleetLaunchCommand → apply → launchFleetRuntime → launchRuntime` route on a fixture seat,
|
||
the ledger points at the fixture and **is** asserted, and the seat-seeded/HOME-empty pass plus
|
||
HOME-seeded/seat-empty fail pair both exist.
|
||
|
||
What remained was the dead `recordLaunch?: boolean` context field. Nothing in the package set it;
|
||
its only effect was to let a caller silently disable recording on the claude branch while codex,
|
||
opencode and pi recorded unconditionally. Removed.
|
||
|
||
## Not part of D1–D6, fixed because it blocked the required evidence
|
||
|
||
The amend requires a green full-package Vitest run.
|
||
`install-ordering-guard.spec.ts > defaults to the real leaseEnforcementActivatable()` made that
|
||
non-reproducible. `defaultCapabilityProbe` executes `dist/cli.js` out-of-process with a **2000 ms
|
||
timeout**; in a full run with 86 spec files scheduled at once, one observation beats the timeout and
|
||
the next does not, so the test's two observations of the same predicate disagree and it fails —
|
||
reporting machine load as a wiring defect. Passed 3/3 in isolation, failed in three consecutive
|
||
full runs.
|
||
|
||
Ruled out my own change by reverting only the `recordLaunch` edit and re-running: still failed.
|
||
The guard call is now bracketed by two observations, only an agreeing pair is used as ground truth,
|
||
a disagreeing pair is retried up to three times, and never holding still is a failure rather than a
|
||
skip. Falsified by inverting the guard's default to `!leaseEnforcementActivatable()` → red
|
||
(1 failed / 18 passed), then reverted.
|
||
|
||
## Verification state
|
||
|
||
- typecheck RC=0.
|
||
- Full package Vitest, sanitized lease env (`MOSAIC_LEASE_*` + `MOSAIC_RUNTIME_GENERATION`
|
||
stripped): **87 files / 1627 tests passed, 0 failed**, three consecutive runs plus one against
|
||
the committed tree, RC=0. That is exactly one file and eight tests above the 86/1619 baseline,
|
||
so the D3 environment work moved nothing else. eslint RC=0, prettier clean.
|
||
- Without that sanitization the suite shows 4 failures in `mutator-gate.acceptance.spec.ts`. Those
|
||
are the known host lease-identity leak into spawned hooks, **not** a product defect — the same
|
||
spec re-run with only those five variables stripped and no code change is 20/20. The standing fix
|
||
is the unpushed `fix/lease-test-env-isolation` branch (blocked on the identity blocker below).
|
||
|
||
## Still open
|
||
|
||
- **D1/D2/D4/D6 need confirmation, not assumption.** They read as substantially implemented but I
|
||
have not run the packet's full RED→GREEN control matrix against each seam. This is now the
|
||
largest remaining item in the amendment.
|
||
- **D3's HOME residual** is routed to harness-homes (see above). It is stated, not engineered
|
||
around, and it does not belong to this branch.
|
||
- Required next evidence per the packet: all D1–D6 observed RED→GREEN controls, framework-shell,
|
||
build/lint/Prettier/bash -n, fresh current-next merge-tree.
|
||
|
||
## Blocker not solvable inside this branch
|
||
|
||
No `fred` principal exists (`tea login list` has no entry; `MOSAIC_GIT_IDENTITY` never reaches the
|
||
pane). The only push path on this host is the **retired** mos-dt-0 token. That is why this work is
|
||
commit-only beyond scrappy's instruction — even after the hold lifts, the truthful authenticated
|
||
push the packet requires cannot be made under a correct identity yet. Raised with mos-claude and
|
||
with Jason; awaiting a mint decision.
|