Admit install-time release-trust packages (#37) - #41
Conversation
|
[Reviewer Round 1]
|
|
[Review Verdict Round 1: NOT_APPROVED] |
f50cc8f to
6947bc0
Compare
|
[Author Round 1] [P1] Split the shared-instructions CI migration out of this PR — partially addressedWhat I changed. The repair now has its own tracking and its own review: issue #42 and pull request #43, which carries the identical commit against What I did not do, and why. I did not remove it from this branch, because removing it does not leave this pull request narrowly scoped and unverified — it leaves it unverifiable. A
So the choice is not "scoped PR" versus "unscoped PR"; it is "scoped PR with no evidence that 1,093 lines of I also considered the one alternative that would restore CI without the migration — pinning On the issue text: you are right that #37 says "No other file changes", and I am not claiming an exemption from it. I am saying the sentence scopes the crate change, which is intact — If you would rather I remove it outright, say so and I will: the sequencing is then #43 merges to No production code changed in this round. Re-verified on the amended branch: |
|
[Reviewer Round 2]
I found no new unresolved issue in the release-trust admission implementation itself. Its extraction, self-admission verification, seed gate, replacement behavior, error mapping, and focused tests match #37. |
|
[Review Verdict Round 2: NOT_APPROVED] |
6947bc0 to
259b663
Compare
|
[Author Round 2] [P1] Remove the shared-instructions migration from this branch — fixedWhat I changed. The commit is gone from this branch.
I asked in Round 1 whether you would rather I remove it outright and said I would if so. You said so, so it is out — I am not re-arguing the point, and the two paragraphs below are the consequences you should know about rather than a renewed objection. What that costs, stated plainly. CI on this branch is now To keep that from being the whole verification story, I re-ran everything locally on the exact tree that was pushed, after the drop:
Sequencing from here. #43 merges to Implementation review — notedThanks for confirming you found no unresolved issue in the extraction, the self-admission verification, the seed gate, the replacement behaviour, the error mapping or the tests. No production code changed in this round; the only delta since Round 1 is the removed commit. DocumentationNothing in the dropped commit was documented anywhere in this diff, so no code comment, rustdoc or |
259b663 to
b6e2e76
Compare
Nothing outside this crate could put a generation into the release-trust tree: the installer, the candidate-set builder, the self-admission request form and the refusing reader were all in place, but no door led to them, and two pieces carried a dead-code allow naming this work as the caller that would remove it. Export two install-time entry points over one shared sequence — extract the container's `trust-set.json` through the container layer's single walk, build the candidate set, verify the delivered bytes against the member's own digest, re-parse the verified member for real, and install through the one funnel onto the tree. A single `member` binding carries all five steps, which is what ties the bytes that were verified to the bytes that are stored. The two doors differ only in the seed's step-0 gate, which refuses a tree whose `active` resolves. That precondition is what keeps "no epoch floor here" safe: without it the seed would be an unconditional way to install an older, pre-revocation generation over a current one. The replace door drops that gate and nothing else, so an operator can re-provision a host wedged by a generation minted at a wrongly-high epoch — and it is a separately named symbol rather than a flag, so the bypass is greppable instead of being a property of a call site. Closes #37
The module summary ended by saying this crate offers a dependent no way to write the release-trust tree. Two exported install-time admission doors now do exactly that, so the sentence names the doors and keeps the claim it was actually making: nothing reaches the tree around the one crate-internal installer. Part of #37
The cleanup test drove a successful admission and one that fails inside the archive walk. Both leave the temporary directory's removal inside `extract_member`, so neither would notice a refactor that hoisted it up into `admit` and returned early past its drop. Add the case that would: a container that extracts whole and is refused two steps later. Part of #37
The doors name exactly one container-layer refusal of their own, and the acceptance criterion that every other one arrives through the existing `PayloadError` mapping was exercised but never asserted: the corrupted archive the scratch-cleanup test drives was only checked for being an error at all. Assert the variant over both sides of the container layer's line — bytes that are no container, and a container that opens but cannot be walked to its end. Part of #37
b6e2e76 to
a3d2d0c
Compare
|
[Reviewer Round 3] The Round 2 scope finding is resolved. The current diff against I found no remaining or new findings. The two public doors use the intended shared admission sequence, preserve the single extracted-member binding through verification and installation, keep the seed gate before package processing, and leave replacement as the explicitly named operator-mediated path without a floor. The tests meaningfully cover the new entry-point behavior and failure boundaries. The PR also retains |
|
[Review Verdict Round 3: APPROVED] |
Suggested squash commitTitle Body |
Summary
Nothing outside this crate could put a generation into the release-trust tree. The installer, the candidate-set builder, the self-admission request form and the refusing reader were all in
main, but no door led to them, and two of those pieces carried an#[allow(dead_code)]naming this work as the caller that would remove it.This exports two install-time admission entry points over one shared private sequence in
src/release_trust.rs:admit_seed_generation(root, package)— the seed door, for a host with no prior generation.replace_generation(root, package)— the operator-mediated door, for re-provisioning a host wedged by a generation minted at a wrongly-high epoch.Both return the new
pub struct AdmittedGeneration { activation, epoch, document }and report failure through the existingReleaseTrustError, which gains exactly two variants:ActiveGenerationPresent { generation: Option<u64> }andMissingTrustSetMember.The shared
admitsequence, backed by a singlememberbinding that carries every step:extract_memberopens the delivered bytes withpayload::open_packageand walks them withPayload::extract_tointo atempfile-created owner-only directory removed on every path out, then readstrust-set.jsonback. No second archive walk and no in-memory member reader is added; a walk yielding no member isMissingTrustSetMember, and every other container fault arrives through the existingPayloadError→VerifyErrormapping.self_admission_candidatebuilds the candidate set; itsNonemaps to the installer's existingProvisionalDecode.VerifyRequest::for_trust_self_admission(&epoch.to_string(), &member_digest(&member))plusverify_packagebinds the extracted member to the signed manifest'scommit.read_trust_set_documentre-parses the verified bytes for real; the provisional decode's output reaches neither the tree nor the return value.install_generation— stillpub(crate), still the single funnel — records the delivered bytes, the member and the verifiedepoch.The two doors differ only in the seed's step 0: one following stat of
generation::active_link, the same oneread_active_epochmakes.NotFound(absent or danglingactive) proceeds; any otherErrrefuses asIo;Ok(_)refuses asActiveGenerationPresentwith the index fromparse_generation, orNonewhenactivenames nothing canonical — before anything is opened, walked, parsed, verified or written. There is no exemption, not for byte-identical redelivery and not behind a flag.replace_generationis that sequence minus the gate and nothing else: a distinct symbol rather than aforceargument, so the bypass is greppable; identical verification; no floor in either direction; norequire-trust-pinmarker read, with the rustdoc recording why. Neither function delegates to the other.Also here: the
#[allow(dead_code)]oninstall_generationand onVerifyRequest::for_trust_self_admissionand their deferring comments are gone;src/trust_fixture.rsgains the member-name parameter theMissingTrustSetMembertest needs, by parameterisingarchive_ofandmanifest_jsonrather than adding a second builder; andREADME.md'srelease_trustbullet no longer claims the module offers a dependent no way to write the tree, naming the doors while keeping the claim it was making — nothing reaches the tree around the one crate-internal installer.The diff is exactly the four files the issue names —
src/release_trust.rs,src/verify.rs,src/trust_fixture.rs,README.md— and adds no dependency. Nothing here forbids the later, separately named runtime accept path reaching the tree through the same installer.CI on this branch
The diff above is the four files #37 names and nothing else. Two things this branch needed in order to have any CI result at all landed on
mainas their own changes, and this branch is rebased onto them rather than carrying either:aicers/agent-instructionsdropped theblocksandinstructions-refinputs from the reusable drift check this repository calls at@main. An unknown input to a called workflow kills the run before job selection, so every run here, and onmain, wasstartup_failurewith nocheck,testorMarkdownresult.testfailed inexecutor::tests::conformance::landing::a_directory_at_the_destination_fails_the_shell_write, a pre-existing race in a file this branch does not touch:PUT_FILE_SCRIPTrefuses a directory destination beforecat > "$tmp", and the helper writing the contents panicked withBrokenPipewhen it lost the race to the shell's exit.Neither is this issue's work and neither is in this diff.
Test plan
activeresolves to the first generation, the storedtrust-set.jsonis byte-identical to the container's member, and the storedepoch, the returnedepochanddocumentall agree, withactivationreporting a changeadmitwith a test-owned scratch directory leaves it empty at all three positions — a success, a refusal inside the walk, and a refusal past the walktrust-set.jsonis refused asMissingTrustSetMember, before any candidate set is builtPayloadError→VerifyErrormapping with no new variantpublic_keyacross two anchors isProvisionalDecode; one repeating akey_idstring across differentpublic_keys reaches the refusing readerepochdisagreeing with the signed manifest'sversionis refused withTargetMismatchepochof0is refused against the reader's absent-or-zero variant, never as a stale trust setepochseeds onto an empty tree, proving no floor where no prior epoch existsActiveGenerationPresentcarrying its index, with nogen-<n>allocated, no scratch left behind, andactive, itstrust-set.jsonand itsepochuntouchedepoch, and identical again when the recordedepochis malformed or missingactiveresolving to a non-canonical directory, and anactivethat is a real directory rather than a symlink, are refused the same way withgeneration: Noneactiveseeds successfully, andread_active_epochreturnsNonefor that same treeread_active_epochreports the active epochreplace_generationsucceeds on that same non-empty tree with that same document, on an empty tree, and on a tree whoseactiveis a symlink to a non-canonical directoryactiveas a real directory the seed refuses at its gate and replace fails inside the engine asIo, with nothing installed either wayepochsucceeds and leaves the recorded epoch at the lower valueepoch/versiondisagreement against the same variants the seed usesLayout::require_pin_marker()changes nothing about a replace, and is still present afterwardscargo fmt -- --check --config group_imports=StdExternalCrate,cargo clippy --all-targets -- -D warnings,cargo clippy --all-targets --features test-support -- -D warnings,cargo test(372 passed) andcargo test --features test-support(372 passed) all pass locally on the pushed treemainand this branch rebased onto them — neither change is in this diffCloses #37