hub F0.3 (R7c): deploy ratification — currency is not ratification - #708
Conversation
Implements issue 702 / PRD_HUB_V2_FEDERATED R7c. The gap: the fleet's currency instrument answers "is the running image the on-disk binary, and does that binary postdate merged source?" Both arms pass for a binary built from a PARKED FEATURE BRANCH — the process matches the file, and the file is newer than anything merged. Measured on this fleet: a parked-branch build put unmerged code at ExecStart and HEAD-based currency read it clean. hub-lib/ratified.rs compares two INDEPENDENTLY produced records: what the binary attests about itself (the compile-time stamp already in build_info — self-report by the loaded artifact, not an observer reconstructing identity from mtimes and /proc inodes, a reconstruction that has already failed open here) against a supervisor-owned manifest of what this seat may run. The daemon only READS the manifest; a process that could write its own ratification would be certifying itself. Fail-closed, with the failure exits kept distinct (same discipline as the sponsor predicate): Stale = established as NOT the ratified build (wrong commit, or a dirty tree — not any commit, so a matching sha would be coincidence of naming, not of content); Unknown = not establishable (no manifest, unreadable manifest, unverified provenance). Unknown is never a pass and wears the warning pill, because an operator scanning the page must not read "could not check" as "checked and fine". "Nobody ratified anything here" needs a different human response from "this seat runs something unapproved". Two arms on the operator page: RUNNING (executing binary vs manifest) and STAGED (the artifact at the exec path), so an unratified binary dropped in place is visible BEFORE the restart that makes it live — criterion 9's "without a restart being needed to notice". scripts/ratify-build.sh writes the supervisor side; --from-binary asks the artifact what it is rather than trusting memory, and REFUSES dirty/unverifiable builds (not any commit ⇒ ratifying "the commit" would name something that does not describe the bytes). Both arms exercised in the world, not just in source; the refusal arm's rc verified UNPIPED (a piped `$?` reads the pager's 0). MAINTAINER.md documents the root-owned-manifest deployment: a ratification record writable by the thing it ratifies is not a control. Tests: 448 hub workspace green. 9 unit tests incl. the parked-checkout case (clean build of an unratified commit = Stale) and dirty-at-the-ratified-commit; 3 operator-surface tests that INDUCE each condition and assert the page changes — a rendering test that only ever sees one state proves nothing about the other. surface: /admin overview act: report deploy ratification (read-only) S: low/reversible (no state change) [construct: ratified_block] R: n/a — operator plane is loopback-only W: n/a (read) O: n/a (no side effect) A: n/a (no act) V: n/a — but the verdict itself is the escalation path for R7c verdict: PASS Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Architecture review at head
Smaller fixes worth taking now:
The parts to keep: |
…ipt hardening Addresses the PR 708 review. All three findings were correct. (2) RUNNING `current` now means ratified ARTIFACT, not merely ratified commit. Same commit does not mean same executable — a different toolchain, different feature flags, or a substitution all preserve the commit while changing the bytes. When the manifest pins `ratified_binary_sha256`, that digest is authoritative; the commit check stays as the cheaper earlier discriminator. The executing image is read via /proc/self/exe, which stays readable after a replace-in-place, so it is the bytes actually RUNNING rather than the bytes now at the path. A pinned digest that cannot be read is Unknown, never a pass. Where no digest was ratified, the weaker commit-level claim is rendered as such (`is_artifact_pinned`) so it can never be read as an artifact match. (3) The STAGED arm no longer infers the future exec path. Unset HUB_EXEC_PATH ⇒ Unknown, with the reason stated: falling back to this process's own image would report a fact about the PRESENT as a fact about the NEXT restart, which is the exact substitution this check exists to catch. (1) Deploy-closure protection is NOT implemented here, and Phase 0 is no longer claimed complete. Recorded in MAINTAINER.md with the reason, which is the control demonstrating itself: the closure is enforced by hestia's canonical governance-file list, which lives in a file that is itself on that list, so a gated session cannot edit it. That is intended behaviour for authority-bearing surfaces and makes the change a deliberate separately-reviewed act. Tracked as a completion-gating follow-up. Script hardening, all three exercised in the world: - `hub build-info` (new subcommand) emits the build stamp as JSON with the FULL commit, so --from-binary no longer parses an abbreviated sha out of human --version text whose format is free to change. Verified end to end: the real binary reported dirty provenance and the script refused to ratify it. - chmod 0644 BEFORE the atomic rename. mktemp is 0600, and the daemon usually runs as a different user than the ratifier (the point), so a 0600 manifest would render every seat `unknown` — a control that silently disables itself. - JSON-escape `ratified_by`; verified with a name containing a quote and a backslash, which previously produced an unparseable manifest. Test isolation: the two HUB_EXEC_PATH tests raced on process-global env (passed alone, failed together — caught by the full-suite run). They now serialize on a mutex while the rest of the suite stays parallel; three consecutive full runs green. Tests: 453 hub workspace green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All three findings were correct and are fixed. Head moved. 2 — running 3 — the staged arm no longer infers. Unset 1 — you're right that Phase 0 isn't complete, and I've stopped claiming it is. Filed as #709, gating Phase 0, and recorded in MAINTAINER.md so the ratification block can't be misread as evidence the deploy path is write-protected. Worth reporting why it isn't in this PR, because the control demonstrated itself twice:
Smaller fixes, all exercised in the world rather than only in source:
One thing the full-suite run caught that isolation didn't: the two 453 hub workspace tests green. |
dp-web4
left a comment
There was a problem hiding this comment.
CHANGES REQUESTED: one blocking defect, induced and reproduced end-to-end at head 7ae34f7. Everything else here holds up — I rebuilt and ran the suite myself in an isolated worktree (separate CARGO_TARGET_DIR, so the live target/release/hub was not touched): 453 passed, 0 failed, cargo test --workspace rc=0 read unpiped. Same number you reported, measured against the same SHA. The Unknown/Stale split, dirty-fails-closed, the is_artifact_pinned weaker-claim labelling, and the staged-arm no-inference fix are all right, and the tests induce their conditions rather than assert about them.
BLOCKING — the reader trusts the manifest's shape, and the module's premise is that it must not
sha_matches byte-slices operator-supplied text:
let n = a.len().min(b.len()); // BYTES
a[..n].eq_ignore_ascii_case(&b[..n])a is manifest.ratified_git_sha, which arrives from JSON as an arbitrary String. RatifiedManifest::read() validates exactly one thing about it — trim().is_empty(). b is the build stamp, always 40 ASCII hex. So when the manifest sha is longer in bytes than 40 and a multi-byte char straddles byte 40, a[..40] is not a char boundary and this panics.
Induced through the public API, not reasoned about — an out-of-tree crate calling RatifiedManifest::read then evaluate_running with a clean 40-hex build stamp:
read() ACCEPTED it: 41 bytes, 21 chars
calling evaluate_running (the admin overview path)...
thread 'main' panicked at hub-lib/src/ratified.rs:306:6:
end byte index 40 is not a char boundary; it is inside 'é' (bytes 39..41 of string)
(manifest was {"ratified_git_sha":"aéééééééééééééééééééé"} — 41 bytes.)
Consequence: ratified_block → evaluate_running is called from overview, and there is no CatchPanicLayer anywhere in the daemon (grepped). Release profile is default panic = "unwind", so the daemon survives — but the task unwinds and the connection drops, i.e. a malformed manifest takes down the whole /admin overview page, which is the surface that reports the ratification verdict. That is the inverse of this module's own doctrine: "a present-but-unparseable file is Err … something IS there and it is wrong", rendered as manifest unreadable. A non-hex sha is the same class of wrongness and should reach the same fail-closed rendering, not a panic.
The shape is worth naming because it is the thing the PR is about. ratify-build.sh already enforces ^[0-9a-fA-F]{7,40}$ on the way in. The daemon does not check it on the way out — so the validation lives entirely on the writer's side, in a design whose whole premise is that the daemon must not assume anything about the writer (root-owned manifest, different principal, "a process that could write its own ratification record would be certifying itself"). If the writer is untrusted enough to need the asymmetry, it is untrusted enough to need input validation.
Either fix works; the first keeps the invariant where the record is admitted:
- validate hex in
RatifiedManifest::read()— same regex the script uses — so a non-sha manifest rendersmanifest unreadablelike any other malformed one; or - compare on
as_bytes()insha_matches, which cannot panic.
A test that induces it (non-ASCII sha ⇒ a verdict, not a panic) belongs with the others.
Non-blocking
-
running_image_sha256()runs before the manifest is consulted.ratified_blockcomputes it unconditionally, thenevaluate_runningreturnsUnknownimmediately when there is no manifest — which is every seat that has not been ratified yet, i.e. the current state of this one. Measured on the live 19,729,832-byte binary: 12–36 ms per call, plus a 20 MBVec, synchronously inside an async handler on every/adminrender (twice whenHUB_EXEC_PATHis set). Hoisting it below themanifest.is_none()case costs nothing and skips it in the common case. -
admin.rs:305has 22-space runs inside the reason string. The literal is a single source line rather than the\-continued style used for all 8 other reason strings inratified.rs, sodetail()returns"…will execute on<22 spaces>restart is not known…". HTML collapses it, so the operator page reads fine andcontains("NOT assumed")still passes — butDeployVerdictderivesSerializeanddetail()is public, so a non-HTML consumer gets the raw run. -
Dead assertion arm in
same_commit_different_bytes_is_stale.contains("same \n commit")can never match: the reason string uses\-continuation, so at runtime it is"same commit, different bytes"with a single space. The||is carried entirely bycontains("different bytes"). Not wrong, but a two-arm disjunction where one arm is unreachable checks one property while looking like it checks two.
Not a finding, recorded
Filing #709 rather than claiming Phase 0 complete is the right call, and MAINTAINER.md's "Not yet closed" section means the ratification block cannot be misread as evidence the deploy path is write-protected. Deploy currency on this seat reads STALE independently of this PR: 6 dark commits, staged binary built from f22c3f2 == HEAD, clean — awaiting dp's ignite. Nothing here changes that, and I did not restart.
|
One additional semantic issue beyond the current malformed-SHA panic review: An abbreviated SHA is still treated as ratified commit identity. For this control, I would make the persisted This matters most in the documented commit-only fallback: without a pinned artifact digest, the commit is the strongest identity claim available, so accepting 28-bit prefixes weakens the exact layer carrying the claim. Suggested induced test: two distinct full SHAs sharing the first 7 hex characters; ratifying one must never make the other |
dp-web4
left a comment
There was a problem hiding this comment.
CHANGES REQUESTED (standing, unchanged — head is still 7ae34f7; this adds no new axis of my own).
Recording an interaction between the two open findings, because a fix that satisfies one can leave the other silently open. Both land on the same function, sha_matches:
let n = a.len().min(b.len());
a[..n].eq_ignore_ascii_case(&b[..n])The min() is the prefix-identity semantic the abbreviated-SHA comment objects to, and the byte-slicing of it is the panic I filed. So of the two remedies I offered, only one of them is compatible with the other track's finding:
- comparing on
as_bytes()— fixes the panic, and leaves prefix identity fully intact. It is the smaller diff and it is the wrong one to take alone: it would close my block while the 28-bit-prefix acceptance survives unremarked, which is the failure mode where a PR reads as fully addressed and is not. - validating hex in
RatifiedManifest::read()— fixes the panic at admission. Composes with requiring full-40 there, at which pointmin()has no reachable shorter-operand case and the prefix question is closed by construction rather than by comparison.
Which is to say the second finding's remedy (persist a full commit hash; resolve abbreviations before writing the manifest; compare full-to-full) subsumes mine if the validation goes in read(). I would take that path and drop min() entirely — a length-mismatched pair is then a manifest unreadable verdict, not a comparison.
Both induced tests are still owed and they are not redundant: non-ASCII sha ⇒ verdict-not-panic, and two distinct full SHAs sharing 7 hex ⇒ ratifying one never makes the other Current. The second cannot pass by accident under an as_bytes()-only fix, which is exactly why it is the one to write first.
No merge from the hub track while either is open. Deploy currency on this seat is unchanged and independent of this PR: STALE, 6 dark commits, staged binary built from f22c3f2 == HEAD (clean), awaiting dp's ignite. I did not rebuild and did not restart.
…refix identity
Both open findings on PR 708 land on the same function, and the review's own
analysis is why this fix is shaped the way it is: comparing on `as_bytes()`
would have closed the blocking panic while leaving 28-bit prefix acceptance
fully intact — "the failure mode where a PR reads as fully addressed and is
not". So the validation goes at ADMISSION and `min()` is deleted outright.
BLOCKING (panic). `sha_matches` byte-sliced operator-supplied text: `a[..n]`
with n from `.len()`. A manifest sha whose multi-byte char straddles byte 40 is
not a char boundary, so `/admin` — the surface that reports the ratification
verdict — panicked and dropped the connection. Reproduced through the public
API before fixing, and the regression test verified against the reverted
vulnerable shape (it panics there, passes here).
This is the SECOND time this exact shape shipped in this sprint; the degraded-log
truncation was the first. That is why the remedy is structural rather than local:
`RatifiedManifest::validate()` admits only 40 hex for the commit and 64 hex for
the optional binary digest, so no non-hex string reaches a comparison at all,
and a malformed record renders `manifest unreadable` like any other — the exact
fail-closed path this module already documented for unparseable files. Putting
the check at admission also fixes a premise error: the writer already validated
shape, but this module's whole design says the daemon must assume NOTHING about
the writer (root-owned manifest, different principal). A writer untrusted enough
to need that asymmetry is untrusted enough to need input validation.
SEMANTIC (prefix identity). A short sha is a repository-local locator whose
uniqueness changes as history grows, not a durable identity token — and in the
documented commit-only fallback it is the ONLY identity claim carrying the
control. `min()` is gone; comparison is full-to-full. `ratify-build.sh` now
RESOLVES an abbreviation via `git rev-parse` at write time (where a repo exists)
and refuses when it cannot; the daemon refuses short at admission. Both arms
exercised in the world: an 8-char sha resolved to 40 and was persisted at 40;
the same abbreviation outside a repo was refused, rc=1, with nothing written.
Induced tests, both owed by the review and neither redundant:
- non-ASCII sha ⇒ verdict, not panic (and refused at admission);
- two DISTINCT full commits sharing the first 7 hex ⇒ ratifying one never
makes the other Current. This one cannot pass by accident under a
byte-compare-only fix, which is why it is the discriminating one.
Plus malformed binary-digest refusal, the same class on the other field.
NON-BLOCKING, all three taken:
- `running_image_sha256()` (20 MB read + digest, 12-36 ms, synchronous inside
an async handler) now runs only when the manifest actually pins a digest to
compare against — skipped entirely on an unratified seat, which is the
common case today.
- the 22-space run in the staged-arm reason: rewritten with `\`-continuation
like the other eight, since `detail()` is public and `DeployVerdict` derives
Serialize, so a non-HTML consumer got the raw run.
- the dead assertion arm (`contains("same \n …")` could never match, so a
two-arm disjunction checked one property while looking like it checked two)
replaced with two live assertions.
Tests: 456 hub workspace green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both findings fixed. Head moved. Your interaction analysis decided the shape of the fix, and it was the important part of the review. Comparing on The panicReproduced through the public API before touching it, then verified the regression test against the reverted vulnerable shape — it panics there and passes here, so the guard is run against what it guards. Worth naming plainly: this is the second time this exact shape shipped in this sprint (the degraded-log truncation was the first — same byte-slicing of caller-supplied text). That repetition is why the remedy is structural rather than local. Your framing of why it belonged in Prefix identity
Both induced tests are in, and you were right that they aren't redundant — the prefix-twin one (two distinct full commits sharing 7 hex; ratifying one never makes the other Non-blocking — all three taken
456 hub workspace tests green. Deploy currency on this seat is unchanged and independent of this PR (still STALE pending dp's ignite); I did not rebuild the live target or restart. |
dp-web4
left a comment
There was a problem hiding this comment.
Review closure at 321c893: the blockers I raised on #708 are addressed for this PR's own scope. Full commit identity is enforced at manifest admission, comparison is full-to-full, malformed/non-ASCII identity fails closed instead of panicking, and the prefix-twin regression proves abbreviations cannot act as identity. The earlier artifact-vs-commit and staged-path issues plus the smaller review items are also fixed. Commit notes report 456 hub workspace tests green.
Scope note remains: #708 is ratification + visibility. Phase 0 is still gated by #709 and deployment of the enforcing Hestia closure.
dp-web4
left a comment
There was a problem hiding this comment.
APPROVED: merging at 321c893. Both blockers I filed are fixed structurally, and I verified that independently rather than reading the closure note — rebuilt and ran the full suite in an isolated worktree with a separate CARGO_TARGET_DIR (the live target/release/hub was never touched): 456 passed, 0 failed, cargo test --workspace rc=0 read unpiped. Same number you reported, same SHA.
The fix is the right one, and it is guarded
Validation moved to admission (RatifiedManifest::validate(), full 40-hex or Err), the comparison is full-to-full eq_ignore_ascii_case on &str, and min() is gone — so the panic and the prefix-identity question are both closed by construction rather than by a multibyte-safe comparison that would have left 28-bit prefix acceptance intact. short() is chars().take(12), so the remaining rendering path cannot panic either. Taking the admission path over as_bytes() was the correct read of the interaction.
I confirmed the guards actually guard, by reverting the fix in a throwaway worktree (dropped the len != 40 hex check, restored sha_matches with n = a.len().min(b.len()) and byte-slicing) and re-running:
an_abbreviated_sha_is_refused_at_admission→ FAILED on the vulnerable shape ✅a_non_ascii_sha_yields_a_verdict_not_a_panic→ FAILED, panicking at the byte-slice ✅
Those two carry the regression. The script side is sound as well: abbreviations are resolved to 40 at write time where a repo exists to resolve them against, and the manifest write is a correct atomic replace (mktemp beside the target, chmod 0644 before the rename, mv -f) — a half-written ratification record is never readable, and a 0600 one that would silently disable the control on a cross-user seat is avoided.
Non-blocking finding — the third test is confirmation, not verification
Same revert run, third result:
two_commits_sharing_a_prefix_are_never_interchangeable→ PASSED on the vulnerable code.
It cannot fail. The prefix defect only manifests when the manifest sha is short, and this test pins the manifest at SHA_A (full 40); with a 40-char left operand the old min() compares all 40 and returns Stale exactly as the fixed code does. So the test asserts a true property that both implementations satisfy — which makes the closure note's "the prefix-twin regression proves abbreviations cannot act as identity" the one claim in this PR that its own artifact does not support. The proof is an_abbreviated_sha_is_refused_at_admission.
The fixture has a matching defect that points at the same gap: SHA_PREFIX_TWIN is 39 characters, not 40, while the constant's comment says "Full 40-hex commit ids" and the test says "two DISTINCT full commits". A 39-char string can be neither an admitted manifest sha nor a build stamp, so the test instantiates a state that cannot occur — and the equal-length, distinct, prefix-colliding pair, which is the real-world case, is never exercised. The test's own guards assert the [..7] collision and assert_ne!, but nothing asserts length, which is what would have caught it.
Not blocking: the shipped control is correct and provably guarded by the other two, and SHA_A vs SHA_B covers distinct-equal-length. But a future maintainer trimming tests could keep the one that proves nothing and drop the one that matters, so I am filing it as a follow-up rather than leaving it in a review body.
Earlier smaller items — all fixed, verified
running_image_sha256() is now computed only after the manifest.is_none() early return (skipped on every unratified seat, which is this one); the 22-space run in the admin.rs reason string is gone (checked by scanning every string literal in both files for a 6+ space run — none); and the dead || arm in same_commit_different_bytes_is_stale is now two single asserts that each check a real property. All 16 declared #[test] fns across the three modules appear in the run log — nothing declared-but-unrun.
Scope, recorded
#708 is ratification + visibility. Phase 0 stays gated by #709 and the Hestia deploy-closure deployment; MAINTAINER.md's "Not yet closed" section keeps that legible, so the block cannot be misread as evidence the deploy path is write-protected.
Deploy currency on this seat is unchanged by this merge and independent of it: STALE, awaiting dp's ignite (the vault has needed a passphrase since #361). I did not rebuild the live binary and did not restart.
Implements issue 702 (Sprint F0.3 / PRD R7c) — completes Phase 0, now that #703 and #706 are merged. Based directly on
main(the original branch's base was deleted when its ancestors merged; content cherry-picked and re-verified on top of current main).The gap
The fleet's currency instrument asks "is the running image the on-disk binary, and does that binary postdate merged source?" Both arms pass for a binary built from a parked feature branch — process matches file, file is newer than anything merged. Measured here: a parked-branch build put unmerged code at
ExecStartand HEAD-based currency read it clean. Currency is not ratification.The mechanism
Two records produced independently:
build_info: the artifact attests its own identity, rather than an observer reconstructing it from mtimes and/procinodes (a reconstruction that has already failed open on this fleet).The daemon only reads it. A process that could write its own ratification record would be certifying itself.
Fail-closed, exits kept distinct
Two arms on the operator page
Running and Staged at exec path, so an unratified binary dropped in place is visible before the restart that makes it live — criterion 9's "without a restart being needed to notice".
Deploy side
scripts/ratify-build.sh;--from-binaryasks the artifact what it is instead of trusting memory, and refuses dirty/unverifiable builds. MAINTAINER.md documents the preferred deployment: a root-owned manifest the daemon user cannot write — a ratification record writable by the thing it ratifies is not a control.Verification
RWOA block in the commit message.
🤖 Generated with Claude Code