Skip to content

inject: per-target exposure lock — refuse overlapping -r runs - #7

Merged
timqi merged 3 commits into
timqi:mainfrom
Airead:fix/inject-exposure-lock
Jul 30, 2026
Merged

inject: per-target exposure lock — refuse overlapping -r runs#7
timqi merged 3 commits into
timqi:mainfrom
Airead:fix/inject-exposure-lock

Conversation

@Airead

@Airead Airead commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Bug

Two rapid consecutive vt inject -r FILE runs leave the decrypted plaintext on disk permanently, with nothing left for --recover to find:

  1. Run A backs up ciphertext C, exposes plaintext P at the target, arms supervisor A.
  2. Run B starts inside A's window: it reads the target — now plaintext P — and snapshots P as its "ciphertext" backup, then arms supervisor B.
  3. Supervisor A fires: restores ciphertext C. ✓
  4. Supervisor B fires last: renames its backup — plaintext P — back over the target. ✗

Both backups are consumed and both sidecars deleted, so the exposure is silent and vt inject --recover cannot undo it.

Fix

Make the ciphertext backup's name deterministic per target (.{name}.vt-backup, previously random-suffixed). The existing O_CREAT|O_EXCL|O_NOFOLLOW create now doubles as a per-target exposure lock:

  • the backup exists exactly while an exposure window is open — created before any plaintext hits disk, consumed by rename() on every restore path (supervisor, immediate_restore, --recover);
  • a second inject -r of the same file fails EEXIST and is refused, with guidance picked from the sidecar: window still live (retry shortly), supervisor dead (run vt inject --recover), or untracked backup (manual mv);
  • a dead supervisor's leftover backup keeps blocking new injections until recovery — fail-safe in the right direction, since the target may still be plaintext.

Two supporting guards:

  • a pre-decrypt check refuses before spending a Touch ID / phone approval (the O_EXCL create remains the authoritative race-free gate);
  • a -r file containing zero vt:// records is refused outright — nothing to decrypt means the wrong file, or plaintext left behind by a broken exposure.

Old random-suffixed sidecars/backups from before this change remain recoverable via --recover (it restores by the full paths recorded in the sidecar).

Testing

  • 5 new unit tests: deterministic backup path, conflict classification boundaries (deadline/grace), sidecar lookup with corrupt-entry tolerance, refusal-message selection, zero-records guard. All use temp dirs.
  • cargo test: 301 passed.
  • cargo check (host) clean; the linux-gnu cross-check fails locally only for a missing cross toolchain (pre-existing on clean main).

🤖 Generated with Claude Code

Two rapid `vt inject -r FILE` runs left the plaintext on disk forever:
the second run read the first run's exposed plaintext and snapshotted
it as its "ciphertext" backup, and its supervisor — firing last —
renamed that plaintext back over the target, with no backup or sidecar
left for --recover to find.

The ciphertext backup's name is now deterministic per target
(`.{name}.vt-backup`), so the existing O_CREAT|O_EXCL create doubles
as a per-target exposure lock: it exists exactly while an exposure
window is open (created before any plaintext hits disk, consumed by
rename() on every restore path). A second inject of the same file
fails EEXIST and is refused with guidance picked from the sidecar:
window still live (retry shortly), supervisor dead (run `vt inject
--recover`), or untracked (manual mv). A pre-decrypt check refuses
before spending a Touch ID / phone approval, and a `-r` file with
zero vt:// records is refused outright — nothing to decrypt means
the wrong file, or plaintext left by a broken exposure.
@timqi

timqi commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Codex Review — fix/inject-exposure-lock

Target: branch diff against main (reviewed at dd2e198)

The deterministic backup lock introduces unsafe interactions with stale sidecars and has edge-case recovery/refusal paths that can leave plaintext unrestored or make the command hang while building an error. These should be fixed before considering the patch correct.

Findings

[P1] Retire stale sidecars before reusing backup pathsrc/client.rs:1426

Because this path is now reused for every exposure of the same target, any stale sidecar left after a successful restore but before remove_file(sidecar) can later point at a brand-new .name.vt-backup. If vt inject --recover runs during that later exposure, plan_recovery uses the old deadline and may rename the new backup over the target, breaking the current command or consuming the only restore backup before plaintext is written; clean matching stale sidecars while the backup is absent or validate a generation before reusing this path. This undermines the exposure-lock invariant in CLAUDE.md.

[P2] Avoid sending fallback sidecars to --recoversrc/client.rs:1800-1802

When the original injection had no resolvable home dir, the sidecar is written beside the target and inject_recover still only scans the state dir, but this branch is also reached for those target-dir sidecars because backup.parent() is scanned. In that no-home crash/reboot case, telling the operator to run vt inject --recover leaves the possibly-plaintext file unrestored; emit the manual restore guidance for sidecars found outside the state dir, or teach recover to scan that exact fallback location.

[P2] Bound sidecar scans in target directoriessrc/client.rs:1768-1770

When no state-dir sidecar matches, the fallback scan runs over backup.parent(), which can be the target's normal directory rather than the private state dir. In a shared or large directory, a precreated backup plus a *.json symlink/device or huge file will be followed and read unbounded here while only formatting a refusal message, so vt inject -r can hang or OOM; restrict this fallback to regular, bounded sidecar-shaped files or avoid scanning arbitrary target-dir JSON.


Automated review by Codex (gpt-5.5), review-only — no code changes were made. New commits to this PR will be reviewed and appended automatically.

The deterministic backup path made the sidecar ambiguous in the other
direction: a stale record (crash between a restore's rename and the
sidecar's removal) could pair with a LATER exposure's backup at the
same path, and a delayed supervisor (suspend pauses its monotonic
sleep while the wall-clock deadline lapses) could blindly rename a
successor's backup — either way consuming the only ciphertext copy
and, in the worst interleaving, stranding plaintext with no recovery
record.

Four guards, from review:

- every new sidecar records its backup's (dev, ino); arming aborts
  before any plaintext if the id cannot be captured
- recovery re-checks the id plus an mtime ordering bound (a
  legitimate backup always predates its own deadline) — the bound is
  all an id-less legacy record has
- arming retires stale sidecars naming the path while the O_EXCL
  lock is held
- the supervisor receives dev:ino in its argv and renames only the
  generation it armed for, keeping the sidecar when the restore
  fails or the backup's state is unknowable

Also hardened: sidecar reads are bounded (O_NOFOLLOW|O_NONBLOCK,
regular-file check, 128 KiB cap shared with the write side, so an
over-cap record fails loudly at arm time instead of being skipped at
recovery); only "not there" counts as backup-consumed — other stat
errors keep the recovery record; orphan guidance for a no-home-dir
fallback sidecar says manual restore instead of naming --recover,
which never scans there; sidecar write failures warn on stderr.
@timqi

timqi commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Codex Review — update (475cbb1)

Reviewed dd2e198..475cbb1 ("inject: bind every restore path to its backup's generation") as a branch diff against main.

The two P2s from the previous pass (fallback sidecars routed to --recover, unbounded sidecar scans in target directories) no longer appear. The previous P1 has narrowed to a P2 on the same theme. CI is green on this SHA.

The patch improves overlap handling, but the new deterministic backup path introduces failure/race cases where a stale parent or partial backup can consume or corrupt the wrong restore source. These should be fixed before considering the change correct.

Findings

[P2] Check backup generation in parent-side restoresrc/client.rs:1426

Because this backup path is now reused for every exposure, the parent failure paths still later call immediate_restore() with only the path and no (dev, ino) check. If the original parent is stopped/suspended past its timeout and then hits a tmp-rename or exec failure while a later inject -r has created the same deterministic .vt-backup, the old parent can rename the later exposure's backup and leave that later run without a restore source. Please carry backup_id into the parent-side restore and verify it as the supervisor does; this is the generation invariant described in CLAUDE.md.

[P2] Remove partial lock backup on write failuresrc/client.rs:1484

This cleanup only covers metadata failures after the deterministic backup has been created. If write_all above fails part-way (for example ENOSPC/EIO), the partial .vt-backup is left behind without a sidecar or supervisor; the next inject -r will treat it as an untracked exposure lock and even suggest moving that partial file over the valid ciphertext target. Please delete the just-created backup on backup-write failure too, so leftover lock files keep the meaning required by CLAUDE.md.


Automated review by Codex (gpt-5.5), review-only — no code changed. cargo test passed during review. Further commits will be reviewed and appended automatically.

Codex review follow-ups on the exposure-lock series:

- The parent's step-4/5/exec failure paths called immediate_restore(),
  a blind rename of the deterministic backup path. A parent suspended
  past its own window could resume into a NEWER exposure owning that
  path and consume its backup — stranding that run's plaintext with no
  restore source. Those paths now share the supervisor's restore body
  (renamed restore_exposure): rename only the (dev, ino) generation
  this run armed, and keep the sidecar for --recover when a restore
  fails or the backup's state is unknowable.

- A backup write failing part-way (ENOSPC/EIO) left a truncated
  .vt-backup with no sidecar or supervisor; the next inject would read
  it as an untracked exposure lock and suggest moving the partial copy
  over the intact ciphertext. create_exposure_backup() now removes the
  just-created file on any post-create failure, while propagating
  EEXIST untouched — that file is another exposure's live lock.
@timqi

timqi commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Codex Review — update (ce6f35e)

Reviewed 475cbb1..ce6f35e ("inject: generation-check parent restores, clean partial backups") as a branch diff against main.

Reviewed clean — no findings. Both P2s from the previous pass are resolved: the parent-side restore now carries and verifies the backup generation, and a partial backup is removed when the backup write itself fails.

Codex's verdict, verbatim:

The changes build and the added inject overlap/recovery logic is covered by focused tests. I did not find a discrete introduced correctness or security issue that warrants an inline finding.

CI is green on this SHA (run 30507031265).

Two commands Codex ran during the review failed, and I checked both — neither is caused by this PR:

  • cargo fmt -- --check fails identically on main, in build.rs and src/cf.rs, which this PR does not touch. Local rustfmt version drift.
  • cargo clippy --tests -- -D warnings reports warnings in core.rs, hook.rs, ssh_sign.rs, main.rs, and five sites in client.rs — all five outside this PR's changed line ranges.

Neither fmt nor clippy is run by CI, so they are not gates for this repository.


Automated review by Codex (gpt-5.5), review-only — no code changed.

@timqi
timqi merged commit d6ea0cf into timqi:main Jul 30, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants