provenance: bind running-image checks to process start time (closes #60)

[iceteaSA]

Closes #60.

ExecutableIdentityProbe::observe read /proc/<pid>/exe without confirming the PID still belonged to the process the supervisor spawned. If a child exits and the OS recycles its PID before the probe runs, that path points at an unrelated process.

The dangerous outcome is not the false alarm. If the recycled process happens to be running the same binary — another ck-subc, another copy of the same module, which is entirely plausible on a busy box — the probe returns Match, the strongest positive attestation the daemon makes, about a process that is not ours. An attestation that fails open is worse than one that fails loudly.

Fix

Pair the PID with the process start time, which the kernel assigns at exec and a recycled PID cannot reproduce. The supervisor captures it alongside the existing spawn facts; the probe re-reads it and compares before touching the image.

When identity cannot be confirmed — recorded and current start times differ, or the value is unreadable — the probe returns a new typed process_identity_unconfirmed, never a verdict.

Precedence is load-bearing. The identity check runs strictly before the digest comparison, and its unavailable wins. A recycled PID running a different binary makes both facts true at once, and reporting Mismatch there would assert "the running image is the wrong binary" about a process we cannot identify. An unconfirmable identity forfeits the right to make any claim about the image, in either direction. unavailable is not the cautious middle between Match and Mismatch — it is the only honest answer when we cannot say whose process we are looking at.

Tests

Four arms, each proved load-bearing by breaking the seam it guards and showing it goes red:

Arm	Red proof
differing start time → unavailable	guard removed → returned Match
live un-recycled child still returns Match	guard inverted → arm failed
precedence: differing time beats differing digest	checks reordered → returned Mismatch
/proc comm-field parse	naive split_whitespace → 18 instead of 424242

The positive arm is the one that matters. A gate that always returns unavailable passes the obvious mismatch test while destroying the feature, and it looks like caution while doing it — so the test that a real, live, un-recycled process still returns Match is what proves the check does not manufacture false unavailables. It spawns a real process and captures its start time through the production path.

The parse arm covers a trap worth naming: field 2 of /proc/<pid>/stat is the executable name in parentheses and can contain both spaces and parentheses, so a naive split_whitespace().nth(21) misparses for any such process and yields a permanent false unavailable. The parser splits on the last ) instead. That test is deliberately not platform-gated — it parses a string, touches no /proc, and the trap is equally wrong everywhere, so it runs on every platform's CI.

Start-time values are compared exactly as read, in raw clock ticks. No conversion to wall time: every conversion introduces rounding or drift, and drift here manufactures false unavailable results on healthy processes.

Scope

Linux only. The macOS arm compares file identity and already discards the PID, so it has no exposure to this race; Windows remains UnsupportedPlatform. Wire surface is additive — one new RunningImageUnavailableReason variant, subc-control 0.6.0 → 0.7.0 with the dependent cascade. subc-protocol and subc-transport are untouched. No golden fixtures changed.

CI will be red, and not from this change

Master is currently red for both platforms — the history rewrite removed docs/team-mode/ while three code paths still reference fixtures there, so agent-token-vectors fails to compile its test target and one subc-core test panics. Filed as #75. Any branch cut from current master inherits it.

Attribution against that baseline: 2 workspace failures, both traced to the two missing fixture files, leaving 0 attributable to this change. Everything else is green, run independently of the implementer's own report — cargo fmt --all --check, cargo build --workspace --locked, clippy --all-targets -D warnings (excluding the uncompilable crate), check-wire-crate-versions.sh origin/master, and the full provenance suite at 15/15.

I did not generate the missing fixtures to get a green gate. A fabricated conformance vector passes the tests while encoding values nobody verified, and the next reader cannot tell it from a real one.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Fixes a Linux-only PID-recycling race in running-image provenance checks (closes #60). The probe previously read /proc/<pid>/exe without confirming the PID still belonged to the spawned process, so a recycled PID running the same binary could produce a false Match; it now captures the process start time at spawn, compares it against the current process, and returns a new ProcessIdentityUnconfirmed unavailable verdict when identity cannot be confirmed.

• The probe opens the running executable first and hashes the retained descriptor, so a PID recycled between validation and hashing cannot substitute a different image.
• The identity check runs strictly before the digest comparison, so an unconfirmable identity never reports Match or Mismatch.
• Start-time values are compared in raw clock ticks with no wall-clock conversion; the /proc/<pid>/stat parser splits on the last ) so executable names containing spaces or parentheses parse correctly.
• The supervisor captures start time at spawn for supervised children and daemon identity facts; macOS and Windows are unaffected.

Migration

• subc-control bumps 0.6.0 → 0.7.0 with the additive ProcessIdentityUnconfirmed reason; consumers must handle the new unavailable case.
• subc-client-rs (0.8.0 → 0.9.0) and subc-core (0.8.1 → 0.9.0) bump via the dependency cascade.
• CI is red on master from a pre-existing missing-fixtures issue (Fresh clone cannot run cargo test: history rewrite removed docs/team-mode/ but three code paths still reference fixtures there (CI red on master) #75), unrelated to this change.

^{Written for commit 3aff07f. Summary will update on new commits.}

[cubic-dev-ai]

1 issue found across 8 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/subc-control/src/lib.rs">

<violation number="1" location="crates/subc-control/src/lib.rs:495">
P2: When a Linux daemon reports `process_identity_unconfirmed`, clients built against subc-control 0.6.0 reject the entire `supervisor.provenance` response because this closed enum has no unknown-value fallback. Preserve mixed-version decoding by using a forward-compatible reason representation or negotiate/gate the new value before emitting it to older clients.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: When a Linux daemon reports process_identity_unconfirmed, clients built against subc-control 0.6.0 reject the entire supervisor.provenance response because this closed enum has no unknown-value fallback. Preserve mixed-version decoding by using a forward-compatible reason representation or negotiate/gate the new value before emitting it to older clients.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-control/src/lib.rs, line 495:

<comment>When a Linux daemon reports `process_identity_unconfirmed`, clients built against subc-control 0.6.0 reject the entire `supervisor.provenance` response because this closed enum has no unknown-value fallback. Preserve mixed-version decoding by using a forward-compatible reason representation or negotiate/gate the new value before emitting it to older clients.</comment>

<file context>
@@ -492,6 +492,7 @@ pub enum RunningImageUnavailableReason {
     RunningExecutableUnreadable,
     SpawnedPathUnreadable,
     HashFailed,
+    ProcessIdentityUnconfirmed,
 }
 
</file context>

[iceteaSA]

Both findings reviewed at source. P1 is valid and I am fixing it. P2 identifies a real break, but its proposed fix contradicts a documented invariant in this repo, so I want your ruling rather than my unilateral change.

P1 — valid, confirmed, being fixed

The TOCTOU is real. compare_opened_paths calls File::open(running_path) internally, so the open of /proc/<pid>/exe happens after the start-time check. My change narrowed the window; it did not close it.

Taking the suggested fix — open the descriptor first, validate identity second, hash the retained descriptor. That ordering is sound in every interleaving:

Recycled…	Descriptor	Stat	Result
before the open	impostor	impostor	start time differs → unavailable
between open and stat	ours	impostor	start time differs → unavailable (conservative)
not at all	ours	ours	honest Match/Mismatch

The invariant that buys: recycling can only ever cost a verdict, never buy a wrong one. Fix in flight, with a test that proves descriptor retention directly — open a descriptor, replace the file at that path with different content, hash, assert the digest is the original inode's.

P2 — the break is real; I verified it. The fix is your call.

First, the concern is not theoretical. Old client, new payload:

OLD CLIENT FAILED: unknown variant `process_identity_unconfirmed`,
  expected one of `not_running`, `unsupported_platform`,
  `running_executable_unreadable`, `spawned_path_unreadable`, `hash_failed`
CONTROL (known reason): decoded OK, 3 modules

And the blast radius is worse than the bot states. That payload held three modules — aft and insula reporting clean match, one module carrying the new reason. A 0.6.0 client loses the entire response, including provenance for the two modules that had nothing wrong. One module's new reason blinds the operator to all of them.

But the suggested remedy — a forward-compatible/open reason representation — runs straight into docs/subc-control-protocol.md:275:

The enum is deliberately strict — no catch-all variant — because a fallback variant would make an unknown op and a MALFORMED body for a KNOWN op decode alike, and those two warrant opposite responses.

That is the reasoning from #31, and I argued for it there, so I am not going to quietly reverse it in a PR about something else. RunningImageUnavailableReason also carries the doc comment "Closed reasons why an executable identity could not be observed" — closed is stated intent, not an oversight.

Where I think the policy genuinely does not reach: line 275 is about op enums, and line 291's "old peers reject unknown with a typed error" is also about op values. Both are cases where rejection is scoped to one request and the peer learns something actionable. Line 17's additive-only rule covers struct fields. RunningImageUnavailableReason is neither — it is a nested value enum inside a list response, where a single unknown value discards every sibling entry. The no-catch-all justification does not transfer cleanly either: for an op, "unknown op" and "malformed body" warrant opposite responses, which is what makes conflating them harmful; for a diagnostic reason nobody dispatches on, a client that decoded unavailable but not why still holds the actionable half.

Options as I see them:

1. Ship as-is. subc-control 0.6.0 → 0.7.0 already signals it, and "old peers reject unknown" is the documented behaviour. Cost: the all-or-nothing blast radius above, on an operator diagnostic surface.
2. #[serde(other)] on this enum only, justified explicitly as a value enum rather than an op enum, with the protocol doc amended to draw that line. Does nothing for clients already built against 0.6.0 — it protects the next addition.
3. Both: land 2 now for future additions, accept 1's skew for this one, and note the upgrade ordering.
4. Gate emission on negotiated version. Correct in principle, but it puts version-conditional wire behaviour in the daemon, which I suspect you will not want for a diagnostic field.

I lean 3 — it fixes the recurrence without pretending to fix the current skew, and it forces the doc to say which enums are closed and why. But this is a protocol-policy decision with a documented precedent pointing the other way, so I would rather have your call than guess. Happy to implement whichever in this PR.

One note on practical exposure while you decide: supervisor.* is operator surface, and ck ships with the daemon, so the realistic skew case is an operator running an older ck against a newer daemon — which the existing build-skew warning already flags, though it does not prevent the decode failure.

[iceteaSA]

P1 fixed in de6f5689 (amended, single commit).

The order inside spawn_blocking is now open → validate → hash the retained descriptor:

let running = match File::open(&running_path) { ... };          // 1. pins the inode
if process_start_time(pid) != Some(expected_start_time) {       // 2. validate identity
    return unavailable(RunningImageUnavailableReason::ProcessIdentityUnconfirmed);
}
compare_opened_descriptor(&mut cache, &running_path, running, &spawned_from)  // 3. hashes the descriptor

compare_opened_descriptor never re-resolves the running path — it hashes the File it was handed. Only the spawned-path side still opens by path, which is a real file on disk and carries no recycling hazard.

The new test proves descriptor retention directly rather than trying to race a PID: write content A, open a descriptor, delete the path and recreate it with content B, then hash through the retained descriptor and assert the digest is A's. Red-proved by switching the implementation back to re-opening by path — the test fails with B's digest and a Mismatch. Provenance suite 16/16.

Two notes for you.

A vestigial parameter I left rather than removed. compare_opened_descriptor still takes _running_path: &Path, unused. I am ambivalent: the _ prefix is the idiomatic "deliberately unused" marker, but an unused path parameter sitting in the signature of the one function whose contract is do not resolve the running path is a loaded gun for a future edit — re-resolving is precisely the defect this round fixed. Say the word and it comes out in a one-line change; I did not want to churn the signature during an open review.

Gate provenance, since it bears on how much these numbers are worth. All gates for this branch ran in a worktree. A sibling flagged a case today where a linked worktree produced 19 test failures that vanished in the artifact-owner checkout — baseline-equivalent failures read as "no regression" while meaning the tests exercised nothing. My two excluded failures are not that: I reproduced both in the primary checkout on unmodified master before excluding them (#75), so they are upstream-real rather than worktree artifacts. The four new arms are mutation-proved, which is the direct defense against a vacuous test — a test that exercises nothing cannot be made to go red by breaking what it guards.

[iceteaSA]

Rebased onto 2a0fbb67 — your fixture relocation landed, so the workspace gate is now complete with zero exclusions for the first time on this branch:

cargo test --workspace       770 passed  0 failed  1 ignored
clippy --all-targets -D warnings   PASS   (no --exclude)
fmt --check                        PASS
build --workspace --locked         PASS
check-wire-crate-versions.sh       PASS

The two failures I was excluding are gone at the source rather than worked around, so the earlier attribution arithmetic is retired — there is nothing left to attribute.

Moving the vectors into the crate that consumes them is the right structural fix, and stronger than what I suggested on #75. A crate reaching up into docs/ for an include_str! was going to break under any future doc surgery; now the compile-time dependency and its data live in the same tree. #75 still shows open — closing is yours, but from here it is resolved.

Also folded in the TOCTOU fix from the review (head b5814321): the running descriptor is now opened before the start-time comparison and the digest is taken from that retained descriptor, so PID reuse in the check-to-hash window can no longer substitute a different image. Recycling can cost a verdict; it can no longer buy a wrong one.

The protocol question in my previous comment — new unavailable reason on a deliberately-closed enum, and what an old client does with a response it cannot decode — is still open and still yours.

[iceteaSA]

Rebased onto 7ce6ef51 (your sentinel-filter landing) and re-gated — green on the new base:

cargo test --workspace       771 passed  0 failed
clippy --all-targets -D warnings · fmt · build --locked · check-wire-crate-versions   PASS

Head 3aff07f8. No conflict with the filter change despite both touching subc-client-rs.

The version collision with #77 still stands — both bump subc-control 0.6.0 → 0.7.0, since 7ce6ef51 left the versions alone. Still happy to rebase whichever you merge second and re-derive its cascade.

[subc-alfonso]

Review by execution: the descriptor-then-verify ordering is the load-bearing choice and it is correct — /proc/pid/exe opened FIRST, start-time confirmed through the live process AFTER, so a recycled pid fails the confirm and a process that dies post-confirm still hashes the pinned descriptor. The recycled-inode note on cache_key (five fields, none redundant) is the same identifier-reuse hazard closed at the second layer, and documenting it at the key site is what keeps a refactor from 'simplifying' it away. Full workspace green locally at the pre-rebase head (759/0), full matrix green on feat/probe-pid-identity at the rebased head. Merging.