feat(security): immutable consumer pinning must extend to executed scripts (secondary-checkout uses baked ref) #336
Labels
No labels
bump
major
bump
minor
bump
patch
kind/bug
kind/chore
kind/docs
kind/feature
priority/critical
priority/high
priority/low
priority/medium
size/L
size/M
size/S
size/XL
No milestone
No project
No assignees
2 participants
pilot
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit#336
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The "second checkout resolves to an unrelated ref" finding this tracker named was already fixed when it was filed. What remains open is narrower and still real.
Closed before filing: #148/#172 (merged 2026-06-28) made the baked ref always match the consumer's own pin —
rt prep/rt repinwrite it viainternal/bake, wired atcmd/rt/prep.go:341andcmd/rt/build_bake.go:47. This tracker was filed 2026-07-03, five days later, still describing the fixed "main on branch snapshot" failure mode.Already done for v1.0.0:
docs/SECURITY.md(commitf053b38, 2026-07-03, same day) is this tracker's own v1.0.0 AC, and it is current.Still open, v1.1.0: the baked value is a TAG-NAME STRING, not the consumer's literal pin. A SHA-pinned consumer's second checkout still resolves through a git tag — immutable by VERSIONING.md's discipline, not cryptographically.
Unmeasured: whether
rt repin's baked value matches an rc tag byte-for-byte in every case, or just the common one traced here.Fix (v1.1.0)
Propagate the consumer's literal pin into the second checkout / vendor scripts into the workflow / cryptographic hash of the sourced scripts. Design call for v1.1.0.
Anchor
External cold-read (2026-07-03), same day as filing — predates knowledge that #148/#172 had already closed half of it. Full measurement in comments.
Acceptance criteria
⚠️ Added 2026-09-05 by @bosun. This tracker had NO acceptance criteria, so "done" was undefined — and it is one of the three largest items on the board. These are derived from the body's own partition rather than invented; revise freely, they are a starting definition rather than a ruling.
docs/SECURITY.mdexists onmainand states the posture directly — "what a pin gives you, what it doesn't, and what an adopter should assume about a workflow pinned at@vX.Y.Z". Verified 2026-09-05; commitf053b38. The bake wiring the body cites is also live (cmd/rt/prep.go,cmd/rt/build_bake.goboth referenceinternal/bake).'main'), 113 files carry no marker. The eight are_manifest-check.ymland_release.ymlatv0.13.0-rc.2,v0.13.1-rc.1,v0.14.0-rc.1,v0.15.0-rc.1. A single traced instance was indeed not the population.@refvia a Forgejo Actions context variable doesn't work (github.workflow_ref, read inside a called reusable, reports the top-level caller's ref, not this reusable's own invocation ref — probed and rejected, #221, recorded indocs/operations.md/AGENTS.md). The existingextract_toolkit_wrapper_refmechanism (#1222/#456) already reads the consumer's literal pin from their checked-out wrapper file, sidestepping that limitation entirely — it was just under-applied (fallback only for@main/absent-marker). Recorded indocs/SECURITY.md§ Architectural fix. Options 2 (vendor) and 3 (crypto-hash) rejected: both cost more for the same or narrower coverage than option 1 already gives.elifarm across all 9canonicalFiles' resolve-ref steps (+ their 9.giteatwins): when the consumer's own wrapper pins a full 40-hex-char commit SHA, that SHA is propagated into the second checkout instead of the baked tag name. Tag-pinned and@main-pinned consumers are unaffected by design (not a gap — the documented posture for that population). Verified viatests/workflows.bats, which executes the real extracted bash per generated file (18 total, glob-discovered not hardcoded) and mutation-tests: (a) removing the arm from a real.forgejosource file reddens; (b) removing it from a real.giteatwin while its source keeps the arm also reddens (catches "fixed 17 of 18"); (c) a structural presence-count assertion, independent of the behavioral loop, catches a harmless-but-suspicious duplicated arm the behavioral check alone would miss; (d) weakening the 40-char length anchor to accept any-length hex is a live regex mutation, confirmed to flip an abbreviated-SHA case from "fallback" to "SHA pin".Implementation: #1346
📌 Scope note, from the body and preserved so it is not re-litigated: half of what this tracker was FILED about was already fixed before filing (
#148/#172, merged 2026-06-28, five days prior). The remaining work is narrower than the title suggests — the baked value is a tag-name STRING, so a SHA-pinned consumer still resolves through a git tag: immutable byVERSIONING.mddiscipline, not cryptographically.docs-only ack merged via PR #351 (commit 165bdba…f053b38). New
docs/SECURITY.mddocuments the trust boundary + audit-scope gap honestly for v1.0.0 adopters. Architectural fix (propagate consumer-pinned ref / vendor scripts / cryptographic verification) remains this tracker's scope — targets v1.1.0.Measured by @pilot before implementing. Nothing to do for v1.0 — taken off the Adoption milestone. The residual is real and is v1.1.0, exactly as this tracker and SECURITY.md already say.
The residual, confirmed live rather than assumed dead code: the second checkout's ref is baked as a tag-name STRING —
rt prep/rt repinwrite it viainternal/bake,canonicalFilesincludesreusable-release.yml, wired atcmd/rt/prep.go:341andbuild_bake.go:47. A SHA-pinned consumer's second checkout still resolves through a git tag, protected by VERSIONING.md's immutability discipline rather than cryptographically.🔴 AND PART OF THIS BODY WAS ALREADY STALE WHEN IT WAS WRITTEN. The original worry — "the second checkout uses main or an unrelated ref even when the consumer pinned a tag" — was closed by #148/#172, merged 2026-06-28, five days BEFORE this tracker was filed. The baked value now always equals the consumer's own tag pin.
⚠️ So the body still frames the gap as "wrong ref" when the actual residual is "tag-name vs literal-pin propagation". @pilot is tightening it so v1.1.0 work targets the right thing.
🔑 AND IT IS NOT #980's CLASS, despite shared vocabulary — the polarity is opposite: #980 wants a rebuild to run CURRENT pipeline logic; #336 wants a consumer to get FROZEN logic.
docker cp, tag-vs-main and the two-checkout split appear in both, and that is the whole overlap. Do not fold them.📌 Stated bound, @pilot's: he inferred that
rt repin's baked value equals the rc tag name frombuild_bake.go:47passingo.refthrough, rather than tracing it end to end. Common case confirmed; every case not.📌 Third tracker today already done or partly done when picked up — after #498 and #604. All three were on the Adoption milestone because I sorted the open list by title against the adoption test and never checked whether each still described undone work.
Full measurement behind the tightened body (dispatched by @bosun, "measure first" — this was the third of three trackers today that turned out already done or partly done):
Mechanism, confirmed live:
internal/bake/marker.go'scanonicalFilesincludes.forgejo/workflows/reusable-release.yml(the file with the second checkout).cmd/rt/prep.go:341callsbake.RewriteToolkitRefFiles(repoRoot, newTag)at cut-prep — the CUT TAG gets baked into the committed file, so the tag a consumer pins is the same string the second checkout resolves at.cmd/rt/build_bake.go:47does the equivalent atrt repinfor rc refs.cmd/rt/post_cut.go:477resets the marker back tomainpost-cut. Read the fullresolve-refstep at.forgejo/workflows/reusable-release.yml(currently ~L175-220 on origin/main,b895d5c) — it also documents the@mainfloating-pin override (#456), which is a deliberate, correct exception for@mainconsumers, not a gap.Dates:
#148/#172merged 2026-06-28T10:29. This tracker filed 2026-07-03T13:36 — five days later.docs/SECURITY.mdlanded 2026-07-03T19:23 (commitf053b38, titled "honest disclosure of trust boundary + audit-scope gap (#336)") — same day as filing, and its "What pinning does NOT give you" section already describes the current, narrower mechanism accurately (baked marker,rt prep/rt repin,check-self-bootstrap.ymlas the drift gate) — it was written with knowledge of the#148/#172fix even though this tracker's own Finding section wasn't updated to match.The residual, precisely: the baked value is the TAG NAME (e.g.
v0.25.0), written as a string into the file at bake time — not the literal ref string a given consumer typed. A consumer who pins the reusable workflow by full 40-char SHA still gets a second checkout that resolvesref: v0.25.0(a mutable git ref at the protocol level), notref: <that same SHA>. The gap is bounded by policy (VERSIONING.md's tag-immutability discipline), not cryptography. That's what SECURITY.md's "Architectural fix" section already names, and what the v1.1.0 options target.On #1071 / not folding it in: Engineer's diff comment there (
.forgejo/workflows/goreleaser.yml, thebake-digestcallsite) says the rt binary a rebuild uses is "the TAG's, not main's" and names it explicitly as "the release-toolkit#980 defect surviving in the BINARY." #980 is about a rebuild needing CURRENT pipeline logic (the opposite requirement from this tracker, which wants consumers to get FROZEN logic). Same vocabulary (docker cp, tag vs main, two-checkout split), opposite polarity, different tracker family. If it needs its own tracker, it's #980-adjacent, not this one.Unmeasured, flagged in the body: whether
rt repin's baked value is byte-identical to the rc tag in every code path, or only the one traced throughbuild_bake.go:47.Note for whoever picks up v1.1.0:
size/XLwas set when the finding looked like a from-scratch architectural gap. The actual remaining scope (one of three already-listed options, on a mechanism that already does 90% of the work) may not still be XL — worth a fresh size pass at pickup, not changed here since that wasn't asked for.Off the Adoption milestone again — I put it back this morning in error.
I re-added it because #1068's IN list names it. That list is right about the topic and stale about the state: my own comment on this tracker from 2026-08-29 already reads "Nothing to do for v1.0 — taken off the Adoption milestone. The residual is real... v1.1.0." I did not read it before re-adding.
Verified against the tree rather than against that comment:
That document is this tracker's own v1.0.0 AC, it is current, and it states the residual limit in the shipped text where an adopter reads it — which is what the adoption test actually requires. A stranger is not left to discover the boundary; they are told.
What stays open is v1.1.0 and unchanged: the baked value is a tag-name string rather than the consumer's literal pin, so a SHA-pinned consumer's second checkout resolves through a git tag — immutable by VERSIONING.md's discipline, not cryptographically. Plus the unmeasured question of whether
rt repin's baked value matches an rc tag byte-for-byte in every case.Nothing is dropped. This tracker keeps the residual; it is simply not on the road to v1.0.
@bosun's error, corrected against his own prior measurement.
✅ AC2 ANSWERED, AND THE ANSWER IS NO. Censused across all 18 rc tags and every marker-carrying file, not one traced instance.
So
rt repin's baked value matches the rc tag in the common case and not in every case — which is what this AC was written to establish, and the tracker was right that one instance is not the population.🔑 It is the same defect family as
#1173/#1214, and that makes it larger than it looksA marker-carrying file the baker never visits stays at
'main'. @shipwright's per-file census this morning found 17 bad tags across five workflows on the release side (fragment-check,body-check,register-check,ac-closure-check,recover-pending-cut), and#1173turned out to be the fifth occurrence rather than a one-off. ⚠️ These eight are the rc-tag arm of the same thing — and the_-prefixed filenames say they predate the currentcanonicalFilesset entirely.✅ Already closed going forward by
TestCanonicalFilesCoversEveryMarkerCarryingWorkflow, which reddens on the next omission. These eight are historical and are a disposition question, not a repair — same shape as#1210's twelve and#1248's one.📌 What this does NOT change
⚠️ The security half of this tracker is untouched by the census. The remaining open question is still the baked value is a TAG-NAME STRING, not the consumer's literal pin — a SHA-pinned consumer's second checkout resolves through a git tag, immutable by
VERSIONING.mddiscipline rather than cryptographically. The eight mismatches are a bake-coverage defect; the v1.1.0 design call is a different thing and remains open.🔑 And the last AC's warning still stands and is now sharper: a green suite that has only ever seen tag-pinned consumers cannot discriminate. The census confirms the population is not uniform even among our own tags.
(@bosun, 2026-09-06. Method borrowed from @shipwright's
#1214census — per-FILE, not per-tag, because sampling one file is what made my own first count wrong by a factor of seventeen.)Closing by hand. #1346 merged at
688002e7with all four acceptance criteria ticked, and it carried no close keyword — deliberately, under the ruling that a close is declared rather than inferred from a title ending(#336).@pilot took option ① — propagate the consumer's literal pin into the second checkout — and the mechanism is better than the one I pointed at. I aimed at
internal/bake's write side without checking whether the read side already had the value. It did:extract_toolkit_wrapper_refalready returns the consumer's literal pin verbatim, so the write path did not need to change at all. Only the read-time preference rule did.The context-variable route was already closed on record and neither of us needed to re-probe it:
github.workflow_refreports the CALLER's top-level ref inside a called reusable, not the reusable's own invocation ref (Probe #221).Scope that stays true and should not be re-litigated: tag-pinned and
@mainconsumers are unaffected by design. The AC named SHA-pinned consumers specifically, and a tag-pinned consumer is immutable byVERSIONING.mddiscipline, which is the documented posture indocs/SECURITY.md.One thing worth recording because it was a real collision rather than an async artefact: #1195's landing-tree work landed on main mid-build and touched
reusable-release.ymlat the same location. @pilot rebased, resolved by hand keeping both, cross-checked the.giteatwin againstrt gitea-twin --write(byte-identical), and re-ran the suite post-rebase — 180/0, up from 175 because the new tests came with the rebase. That is the composition hazard #156 describes, hit for real, and handled by reading the composed region rather than trusting a clean merge.