feat(ci): hash publication becomes a dispatch; flux CI loses write access to fluxhashes - #1789
feat(ci): hash publication becomes a dispatch; flux CI loses write access to fluxhashes#1789MorningLightMountain713 wants to merge 8 commits into
Conversation
…es the hash itself Replaces the clone-edit-push publish body: this workflow no longer holds write access to fluxhashes at all. It asks the signer there to publish this commit; the signer fetches the commit from this public repository, computes the tree hash from the bytes it fetched, and emits the list, the signed document and provenance in one atomic commit. A compromised token here can trigger signer runs and nothing else -- it cannot put a hash value into the list. The early Check Hash computation survives as claimed_hash, a tripwire the signer compares against its own derivation: a mismatch is a red signing run, catching runner-image drift that would otherwise silently publish a hash no node ever matches. The fork guard replaces a failing secret lookup with an explicit skip. API_TOKEN_GITHUB stays for the fluxjsdocs docs push; the new FLUXHASHES_DISPATCH_TOKEN carries Actions permission on fluxhashes and nothing else. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFrS7Q3JwuPj6Yr4vALnwp
Cabecinha84
left a comment
There was a problem hiding this comment.
Conditions on the ACK
-
Merge order is a real availability dependency, not a nicety. If #1789 lands before fluxhashes#2, hash publication
stops dead: the old push path is deleted and sign-hashlist.yml doesn't exist on fluxhashes master, so every dispatch
404s. fluxbench cannot verify any new FluxOS tree until #2 lands. Currently: fluxhashes#2 is OPEN, #1788 is OPEN. The
PR body states the order correctly — just make it an explicit merge condition rather than a note. -
The security payoff is entirely operational and outside this diff. Removing the fluxhashes grant from
API_TOKEN_GITHUB is what actually closes the hole. Until that happens, this PR only removes the use of write access,
not the access. And that PAT is shared — the Push docs to other repo step (nodejs.yml, still in build) uses the same
secret for fluxjsdocs, so the grant must be trimmed, not the secret revoked.
Things I'd raise (none blocking)
-
No permissions: block — worth fixing in a security PR. Neither the workflow nor the publish job declares one, so
GITHUB_TOKEN inherits the repo default (possibly read-write). publish needs contents: read and nothing else. One line.
The dispatch step correctly scopes GH_TOKEN to itself, so gh won't fall back to GITHUB_TOKEN — that part is right. -
Permanent red X on every push until the secret exists. Deliberate per the description, and nothing is blocked by
it. But if you'd rather not train people to ignore a red check, a job-level env gate is the clean way:
publish:
env:
GH_TOKEN: ${{ secrets.FLUXHASHES_DISPATCH_TOKEN }}
steps:
- name: Request hash publication
if: env.GH_TOKEN != ''
Counter-argument, which I think is the stronger one: this same gate would later hide an expired token silently.
Loud-and-red is defensible. Raise it as a question, not a demand.
-
The claimed_hash tripwire is a cheap DoS for a compromised dispatch token — and it blocks, not just annoys. The
description says a bad dispatch "can cause noise, not list content." Accurate on content, but understated on impact:
sign-hashlist.js:294 throws, which aborts the entire run — so nothing publishes for anyone, not just the poisoned
commit. Repeated dispatches stall publication for the whole fleet. Self-heals on the next clean run, and the fix (skip
the offending commit, keep reconciling) belongs in fluxhashes#2, not here. Worth stating plainly rather than leaving
it implied. -
"The token can trigger that workflow and nothing else" is slightly stronger than the permission grants.
Actions:write on fluxhashes also permits dispatching any workflow there, cancelling/re-running runs, and deleting logs
and artifacts. GitHub offers no finer granularity, so there's nothing to change — but the comment in the YAML
overstates the containment. -
"Forks publish nothing" is only half-true. The fork guard on publish is correct and is a genuine improvement (forks
previously went red at the hash push). But build's Push docs to other repo step still runs unconditionally with
API_TOKEN_GITHUB, so fork builds stay red at the last step. Out of scope; the same if: github.repository ==
'RunOnFlux/flux' would finish the job if you want it. -
No timeout-minutes on publish. A hung gh call occupies a runner for the 6h default. Trivial.
…lassic PAT dies entirely Both cross-repo credentials are now GitHub Apps that mint a short-lived installation token per run -- nothing long-lived to leak and nothing to renew. flux-hashlist-dispatch holds Actions on fluxhashes only (verified: contents writes refused); flux-docs-push holds Contents on fluxjsdocs only. This removes the workflow's last reference to API_TOKEN_GITHUB -- the classic PAT with org-wide repo scope -- so migration revokes it with nothing to replace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFrS7Q3JwuPj6Yr4vALnwp
…ut, honest containment comment The workflow's own GITHUB_TOKEN drops to contents: read (everything cross-repo uses the per-run app tokens); the publish job gets a 5-minute timeout instead of occupying a runner for the 6-hour default on a hung call; and the credential comment now states what Actions permission actually grants -- start, cancel, re-run, delete logs; availability, never content -- instead of overstating the containment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFrS7Q3JwuPj6Yr4vALnwp
|
Sharp review — two of its points were overtaken by the commit that landed just after it was written ( Merge order — agreed it's a real availability dependency. This PR doesn't get the "ready to merge" label until fluxhashes#2 is in; the body carries the explicit order. One nuance: publication doesn't stop entirely in the wrong order — stale branches keep publishing through their old workflow copies until the old credential is revoked; it's new pushes on this line that would lose coverage. Trimming the PAT vs revoking — overtaken twice. Nothing on this branch references
The red X — keeping loud-and-red, per your own counter-argument: a gate that skips on a missing secret also skips silently on a broken one. The app tokens removed the expiry case that made the gate tempting. claimed_hash as a blocking DoS — your impact statement is accurate and better than our wording: a poisoned dispatch aborts the whole run, so sustained spam stalls publication, not just one commit. We're keeping the abort rather than skip-and-continue, deliberately: a mismatch is indistinguishable from signer-side environment drift, and drift that publishes "just the other commits" is drift publishing silently — the one failure that must never be quiet, since it signs hashes no node can match. The stall needs a flux-collaborator-level credential, gains no content, and self-heals on the next clean run. We'll state this plainly in fluxhashes#2 and the design doc rather than leaving it implied. "and nothing else" overstates Actions:write — right; the comment now says what the permission actually grants (start/cancel/re-run/delete logs — availability, never content). Fork guard on the docs step — already in Two asks while you're here, both single actions: approve the flux-docs-push installation request pending on @RunOnFlux (Contents on fluxjsdocs only — it's what retires the classic PAT), and grant MorningLightMountain713 admin on RunOnFlux/flux so the four app secrets ( |
|
Both of those landed — thank you. Recording the result so the state of this PR is unambiguous. The flux-docs-push installation is approved (Contents on fluxjsdocs, nothing else) and the four app secrets are set here. I checked both rather than assuming: the docs app mints an installation token that reaches Uploading a secret says nothing about whether a multi-line PEM survived the round trip, so I re-ran this PR's One correction I owe you. You raised trimming Two other body corrections while I was in there: #1788 was citing the closed #1785 and had the merge order backwards, and fluxhashes#2 now discloses that key 1 was regenerated on the 24th — the pinned hex you verified in your review of that PR has changed, so that check is worth redoing. There is a longer note on fluxhashes#2 explaining that it was rewritten in place after your review. Nothing further is needed from you except the re-reviews. Merge order unchanged: fluxhashes#2 first, then this. |
Cabecinha84
left a comment
There was a problem hiding this comment.
Non-blocking findings
- Check Hash has no failure guard — and its failure mode is fleet-wide, not local. newhash=$(find ./ZelBack … |
md5sum | …) takes the exit status of the last pipeline element, so if ./ZelBack ever moves or disappears, find
errors to stderr, the step exits 0, and newhash becomes d41d8cd98f00b204e9800998ecf8427e (md5 of empty input).
That bogus claim then dispatches, and sign-hashlist.js:295 throws — which aborts the whole signing run, so
nothing publishes for anyone until someone intervenes. The signer defends its own side of this (:167 rejects a
non-md5 pipeline result); the flux side has no equivalent. One line fixes it:
[ -d ./ZelBack ] || { echo "no ZelBack in this checkout"; exit 1; }
Cheap, and it converts a fleet-wide publication stall into a red job in the repo that caused it. - cpina/github-action-push-to-another-repository@main is still unpinned, and now receives a freshly minted app
token. Strictly better than before (the token it gets went from org-wide repo to Contents-on-fluxjsdocs), so
this PR improves the situation rather than creating it. But in a PR whose whole thesis is credential
containment, an unpinned third-party action tracking main is the residual hole: whoever controls that repo can
take the token on any push. Worth a follow-up to pin it to a SHA — not a reason to hold this one. - Cosmetic: actions/create-github-app-token@v1 emits a Node 20 deprecation warning on the runner (v2 targets
Node 24). Also, publish has timeout-minutes: 5 while build still has none.
Cross-repo caveat — belongs to fluxhashes#2, but affects how you time this one
The signer is level-triggered off git ls-remote of the flux refs, which recovers ref tips, not intermediate
commits. Two consequences worth naming when you write your ACK:
- Merge order is a hard gate, not a nicety. If #1789 lands before fluxhashes#2, every push in the gap 404s. The
daily sweep then reconciles current tips — but any commit that was a tip only briefly during the gap is never
listed, and never will be. So out-of-order costs permanently unlisted trees, not just a delay. - The same shape exists at steady state. concurrency: {group: sign-hashlist, cancel-in-progress: false} means a
newer dispatch cancels an older pending one; the surviving run reconciles refs, so a rapid A-then-B push to one
branch can leave A's tree unlisted. Lower stakes than the gap case (intermediate branch commits rarely get
deployed), and out of scope here — but it's the honest reading of "repaired by whichever run survives."
The docs push carried no ref condition, so every branch build raced for fluxjsdocs master and the published JSDocs were whichever branch happened to build last -- three separate feature branches overwrote it within ten seconds on 2026-08-25, and it was serving an unmerged branch's services tree at the time this was found. Gate both docs steps on development. The token mint is gated too: there is no reason to mint a credential for a push that will not happen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQKpZbuxYqrWeErU7vKgoA
|
Heads-up that the diff moved since the last comment — one commit, The docs push had no ref condition. It only ever checked the repository, so every branch build raced for fluxjsdocs master and the published JSDocs ended up being whichever branch happened to build last. On the 25th three separate feature branches overwrote it within ten seconds — Both docs steps now require Verified rather than assumed, on this branch: the run at
|
The publish job's comment claimed "a hash value cannot enter the list through it". That is false. A fork network shares one object store, so a dispatched commit may be any commit ever pushed to this repository or to a public fork of it, and the signer derives whatever it is pointed at. What holds is narrower: the credential sends pointers rather than hash values and cannot write to fluxhashes. That bounds the credential; it does not vouch for the tree. The credential is reachable by anyone who can land a workflow change on any branch here, who can already get a hash listed by pushing one -- the same trust boundary, not a defence against it. The claim mattered because it is the reasoning used to accept looser third-party action pins elsewhere in this file. Wording only; no behaviour change. Reported by Cabecinha84 on #1789. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQKpZbuxYqrWeErU7vKgoA
…signer Check Hash took the exit status of the last element of its pipeline, so a missing ZelBack made find error to stderr while the step exited 0 with newhash set to d41d8cd98f00b204e9800998ecf8427e -- the md5 of an empty stream, and a well-formed 32-hex value that nothing downstream distinguished from a real tree hash. The signer now refuses to list that value, so it cannot reach the published list. But a claim the signer cannot match aborts the signing run, which puts the red in a different repository from the mistake and stalls publication until the next clean push or the daily sweep. Failing here keeps the diagnosis where the cause is. Two ways in, and the second is not a pipeline failure: ZelBack absent, which pipefail catches, and ZelBack present holding no regular files, where find exits 0 and emits nothing. Hence both set -eo pipefail and the explicit comparison -- a bare [ -d ./ZelBack ] would miss the second. Verified by extracting this step's script verbatim from the workflow and running it against three fixtures: absent ZelBack exits 1 on find's own error, empty ZelBack exits 1 on the guard, and a real tree returns the same hash it did before. Reported by Cabecinha84 on #1789. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQKpZbuxYqrWeErU7vKgoA
cpina/github-action-push-to-another-repository was referenced by tag @main -- a personal repository's default branch -- so every build downloaded and ran whatever was on it at that moment, and handed it a token that can write to fluxjsdocs. Pinning to a commit would close that, but the action is doing nine lines of work: clone the destination shallow, wipe the target directory, copy the source over it, commit if anything changed, push. The other 166 lines are options we do not pass, error strings, and git-lfs setup we do not need. Doing it inline removes the dependency rather than pinning it, which is the same argument this PR makes about the credential itself. Two things improve on the action besides: fluxjsdocs is public, so the clone is anonymous and the token authenticates only the push, where the action put it in the clone URL and therefore the clone's stored config; and `git add -A services` scopes the commit to the published directory instead of `git add .` over the whole destination checkout. Behaviour is preserved deliberately, including the commit message format "Update from <origin commit url>" so the fluxjsdocs history stays uniform. Verified by extracting this step verbatim from the workflow, redirecting only the two GitHub URLs at local repositories, and running it: a changed file updates, a file deleted upstream disappears downstream, nested directories are copied, an unrelated directory in the destination is left alone, the author and commit message match what the action produced, and a second run with identical content prints "docs unchanged" and pushes nothing -- master unmoved, no empty commit. Raised by Cabecinha84 on #1789 as a follow-up; taken now since it is the same PR that argues for containment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQKpZbuxYqrWeErU7vKgoA
actions/create-github-app-token v1 -> v3, on both the dispatch and docs token mints. v1 targets Node 20, which the runners now force onto Node 24 with a deprecation warning on every run. Neither major break reaches us: v2 removed the underscore input spellings (app_id, private_key) and this workflow uses the hyphenated ones; v3 removed custom proxy handling and raises the self-hosted runner floor, and these jobs use GitHub-hosted runners with no proxy. app-id, private-key, owner and repositories are all still accepted. codecov/codecov-action stays on v3 deliberately. v4 moved to the CLI uploader and dropped tokenless uploading for non-fork runs, so bumping it needs a CODECOV_TOKEN secret first or coverage upload starts failing. Unrelated to this PR; left for whoever adds the token. Raised by Cabecinha84 on #1789 as a cosmetic note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQKpZbuxYqrWeErU7vKgoA
|
All three findings taken, and the third turned into a removal rather than a pin. Tip is now Finding 1, I did not take the suggested One correction to the impact, which does not change the fix: it is not a stall "until someone intervenes". The next clean push reconciles everything, and the daily sweep does it unattended — every run reconciles the full delta rather than just what it was told about. The matching guard is on the signer side too (RunOnFlux/fluxhashes#2, Finding 2, the unpinned action — removed instead of pinned ( Two things came out better than the action managed, rather than merely equal to it: fluxjsdocs is public, so the clone is anonymous and the token authenticates only the push — the action put the token in the clone URL and therefore in the checkout's stored config — and Verified by extracting the step verbatim and pointing only the two URLs at local repositories: a changed file updates, a file deleted upstream disappears downstream, nested directories copy, an unrelated directory in the destination is untouched, author and message match what the action produced, and a second run with identical content prints Finding 3, the deprecation warning — done (
On The cross-repo caveat — your reading is sharper than what the body said, and the body now says yours. You are right that One unrelated thing this review caused me to find. The docs push had no ref condition at all — only a repository check — so every branch build raced for fluxjsdocs master and the published JSDocs were whichever branch happened to build last. On the 25th three separate feature branches overwrote it within ten seconds, and it was serving an unmerged branch's Also corrected in this range ( |
Background
fluxbench verifies each node's FluxOS tree hash against the list served from RunOnFlux/fluxhashes. Today this workflow writes that list directly: it appends its computed hash and pushes into fluxhashes with a contents-write PAT exposed to every branch build. RunOnFlux/fluxhashes#2 signs the list — but as long as this repository can write it, the signature only attests "this was in the repo when the signer ran": a compromised token here could rewrite the list and have the rewrite signed.
Related PRs and merge order:
API_TOKEN_GITHUB) is revoked outright — a classic token with org-widereposcope, and this PR removes the last reference to it. There is no narrower grant to leave behind: classic tokens carry no per-repo scoping.Supersedes #1785 (closed with explanation there).
What changes
publishjob computes the tree hash from a pristine checkout and dispatches "commit X exists, I computed H" to the fluxhashes signer. The credential is the flux-hashlist-dispatch GitHub App — Actions permission on fluxhashes and nothing else, verified able to trigger workflows and refused by every write API. The workflow mints a short-lived installation token per run, so there is no long-lived secret to leak and nothing that expires. The docs push moves to the flux-docs-push app the same way (Contents on fluxjsdocs only), which retiresAPI_TOKEN_GITHUBentirely.claimed_hashis a tripwire: a mismatch is a red signing run, never a listed value. Note what that does and does not buy: the dispatch names a commit rather than a hash, but a fork network shares one object store, so a dispatched commit may be any commit ever pushed to this repository or a public fork of it, and the signer derives whatever it is pointed at. The credential is reachable by anyone who can land a workflow change on any branch here — who can already get a hash listed by pushing. Same trust boundary, not a defence against it. The tripwire also catches runner-environment drift, which today would silently publish a hash no node matches.publishis a separate job frombuild, so a publication failure never blocks the test suite; forks skip it entirely.development. It carried no ref condition, so every branch build raced for fluxjsdocs master and the published JSDocs were whichever branch happened to build last — three separate feature branches overwrote it within ten seconds on 2026-08-25, and it was serving an unmerged branch's services tree when this was found. Both docs steps now requiredevelopment; the token mint is gated too, since there is no reason to mint a credential for a push that will not happen.Note for reviewers: the four app secrets are now set on this repository and the flux-docs-push installation is approved (both 2026-08-25).
buildis green, and the docs steps correctly skip on this branch under the newdevelopmentgate.publishmints its token successfully and stays red only until fluxhashes#2 merges, because the signer workflow it dispatches to does not exist before then — that one red is expected and is the last thing this PR is waiting on.Verification
Full sandbox rehearsal on real GitHub 2026-08-24: this exact dispatch step drove the signer end to end, including a cross-environment claimed-hash match and bootstrap from a production-shaped state. The signer side carries a 37-case local harness; an independent implementation review found nothing above cosmetic.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SFrS7Q3JwuPj6Yr4vALnwp