fix(B-0853.1 post-merge): 3 Copilot P1 security tightenings on PR #5417 (job-scope id-token + tightened safety wording + pinned verification identity)#5419
Merged
Conversation
PR #5417 (cosign keyless OIDC ISO signing) merged at 70596a8 before Copilot review threads could be addressed. Fix-fwd per substrate-honest discipline; the underlying findings are all valid security improvements that don't change behavior, just tighten + clarify. Finding 1 (P1): move `id-token: write` from workflow-level permissions to jobs.build.permissions block. Matches existing repo pattern (.github/workflows/scorecard.yml). Reduces blast radius if future jobs are added to this workflow. Finding 2 (P1): tighten safety wording. Original comment claimed the OIDC token "cannot be exfiltrated to mint signatures for other workflows" — overstated. The real properties are: - Short-lived cert (Fulcio mints 10-min cert tied to this run) - Identity bound to workflow path + ref - Steps pinned to commit SHAs Comment now states the actual mitigation surfaces + acknowledges that any step in this job with id-token could in principle transmit the token off-runner. Finding 3 (P1): tighten verification regexp in workflow comments. Original recommendation: --certificate-identity-regexp '^https://github.com/Lucent-Financial-Group/Zeta' was prefix-regex matching ANY workflow in the org/repo. Defeats the workflow-identity binding. Replaced with explicit pin: --certificate-identity 'https://github.com/Lucent-Financial-Group/Zeta/.github/workflows/build-ai-cluster-iso.yml@refs/heads/main' plus variants documented for branch + tag verification. No runtime behavior change in this PR — all 3 are documentation/permission-scope tightenings. The cosign sign-blob step itself is unchanged; identity emitted into the signed cert is the same either way; consumers picking up the verification command from in-workflow comments now get the tighter recommendation. Resolves Copilot threads PRRT_kwDOSF9kNM6FBtyf + PRRT_kwDOSF9kNM6FBtzO + PRRT_kwDOSF9kNM6FBtzn on PR #5417.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Post-merge security tightening for PR #5417's cosign keyless OIDC signing setup, addressing three P1 Copilot review findings on the build-ai-cluster-iso workflow. Changes are documentation- and permission-scoping-only with no runtime behavior change.
Changes:
- Move
id-token: writefrom workflow-level tojobs.build.permissions(job scope), matching thescorecard.ymlpattern. - Replace overstated safety wording with realistic threat surfaces (short-lived cert, identity binding, pinned steps) and acknowledge the residual risk.
- Update documented
cosign verify-blobinvocation from a permissive--certificate-identity-regexpprefix to an exact--certificate-identitypin for the workflow file + ref, with variants for branches and tags.
AceHack
added a commit
that referenced
this pull request
May 27, 2026
…0 substrate for Ace migration trajectory (14 sub-steps; 12 declarative-input categories; substrate-anchor for B-0852/0853/0855/0856 cross-refs) (#5420) * docs(B-0854.1): zeta-install.sh step-state-machine inventory — Phase 0 substrate for Ace migration trajectory B-0854 sub-row .1 (Phase 0; smallest pure-analysis slice). Documents the EXISTING imperative bash state-machine in zeta-install.sh so the B-0854 Phase 2 declarative-Ace-manifest schema can express the same surface. Inventory covers: - Top-level entry (REPO_URL, HOST, ZETA_AUTO_CONFIRM env semantics) - Step-by-step state machine for all 14 sub-steps (1, 2, 3, 4, 5, 6, 6.5, 6.55, 6.6, 6.7, 6.8, 6.9, 6.95, 7) with inputs/outputs/side- effects/failure-modes/declarative-equivalent per step - Cross-cutting: operator-prompt accumulation count (7 prompts today; B-0852 phase-split target = 1 passphrase prompt) - Idempotency surface table — informs B-0855 architectural fix scope - 12 distinct declarative-input categories the Ace manifest must capture (Phase 2 sub-row scope) - Files-generated-during-install table mapping to B-0852.5 cred- manifest entries (6 mapped, 3 candidate-expansion items named) Snapshot date: 2026-05-27 (origin/main 70596a8; PR #5417 cosign merge). Future refreshes should re-snapshot when zeta-install.sh changes substantially. Composes with already-landed substrate-engineering arc: - B-0852 + sub-rows (cred persistence) — PR #5403/#5411/#5414 - B-0853.1 (cosign signing) — PR #5417 + fix-fwd #5419 - B-0855 (self-register architectural fix) — PR #5412 - B-0856 Path A (deferred /tmp coordination) — PR #5413 - B-0854 parent (Ace migration trajectory) — PR #5405 No code change; pure documentation. Doesn't affect ISO substrate; batches into substrate-engineering history independent of next ISO build cycle. * fix(B-0854.1): escape | inside code spans for MD056 table-column-count compliance * fix(B-0854.1): 10 Copilot accuracy corrections — verified against actual zeta-install.sh content PR #5420 Copilot review caught 10 substantive accuracy issues in the B-0854.1 inventory doc. All 10 verified against origin/main 70596a8's actual zeta-install.sh content + corrected. Corrections: - Name attribution → role-ref ("the human maintainer") - Step 1 inputs: actual `lsblk -d -p -n -o NAME,TYPE,RM,RO,TRAN` + awk filter (not made-up NAME,SIZE,MODEL,TRAN,ROTA) - Step 3 side effects: `sgdisk --zap-all` only (not `wipefs -af` too) - Step 4: actual `sgdisk` (NOT `parted`); GPT layout via -n + -t flags; whole-disk longhorn partitions on DATA_DISKS too - Step 6: `nixos-generate-config --root /mnt --force` (NOT --no-filesystems; --force overwrites existing config) - Step 6.5: no MAGIC_NUMBER (didn't exist in script); INJECT_OK gate flag; iter-4 v1 manual-config-edit fallback path - Step 6.9: SELF_REG_OK flag; documented graceful-skip path lines 731+ - nixos-install: actual line ~1004 (NOT 1096-1340); section renamed to "nixos-install (the actual build; ~line 1004)" since the prior range was wrong - Step 7: actual lines 1261-1336 (NOT 1341-1352); banner driven by GH_AUTH_OK/GH_KEY_COUNT/INJECT_OK/SELF_REG_OK (NOT MAGIC_NUMBER); conditional sections listed in declarative equivalent Resolves 10 Copilot threads on PR #5420. Root cause of the inaccuracies: original draft was written from `grep -E "^# ── Step"` summaries + recollection of script behavior, not careful per-step body reads. Discipline lesson: when authoring substrate-anchor docs claiming to inventory existing code, the read must be careful per-line, not skim-grep summary. Composes with .claude/rules/verify-existing-substrate-before-authoring.md at the inventory-substrate scope (verify-content-of-thing-being-inventoried before authoring claims about its content). --------- Co-authored-by: Lior <lior@zeta.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Post-merge fix-fwd for 3 Copilot P1 security findings on PR #5417 (cosign keyless OIDC ISO signing) which merged at `70596a8db` before the review threads could be addressed.
All 3 findings are valid security improvements; this PR is documentation/permission-scoping only — no runtime behavior change.
Findings + fixes
Finding 1 (P1): scope
id-token: writeto job, not workflowBefore: top-level
permissions: { id-token: write }After: scoped to
jobs.build.permissions: { id-token: write }Matches the repo pattern in `.github/workflows/scorecard.yml`. Reduces blast radius if future jobs are added to this workflow.
Finding 2 (P1): tighten safety wording
Before: "token-issuance scope is workflow-bound; granted permission cannot be exfiltrated to mint signatures for other workflows."
Overstated — any step with id-token access could in principle transmit the token off-runner.
After: comment names the actual mitigation surfaces (short-lived cert, identity binding, pinned steps) + acknowledges the realistic threat.
Finding 3 (P1): tighten verification regexp
Before: `--certificate-identity-regexp '^https://github.com/Lucent-Financial-Group/Zeta'\` (prefix-regex; accepts ANY workflow under the repo — defeats workflow-identity binding).
After: explicit pin to this specific workflow file + ref:
```
--certificate-identity 'https://github.com/Lucent-Financial-Group/Zeta/.github/workflows/build-ai-cluster-iso.yml@refs/heads/main'
```
Plus documented variants for verifying branch + tag signatures.
What this is NOT
Composes with
Resolves Copilot threads on #5417
Test plan
🤖 Generated with Claude Code