claude: feat(verify): flip run_verify.sh to one wrangle-attest verify per subject (#484 spike, v0.5.0)#781
Draft
TomHennen wants to merge 6 commits into
Draft
claude: feat(verify): flip run_verify.sh to one wrangle-attest verify per subject (#484 spike, v0.5.0)#781TomHennen wants to merge 6 commits into
TomHennen wants to merge 6 commits into
Conversation
… sign the VSA wrangle-attest verify evaluates the policy over one subject by exec-ing the sibling ampel binary from the attest-toolbox image, then signs the emitted VSA verbatim (the --statement signer path) and appends the signed line to the per-artifact bundle. The verdict protocol is deliberately stricter than an exit-code check: ampel v1.3.1 overloads exit 1 (FAILED verdict and every tool error) and exit 0 also covers SOFTFAIL and the empty-chain soft-pass, so the engine requires exit 0 AND an emitted VSA that parses, binds exactly the requested single-sha256 subject, and says verificationResult PASSED before anything is signed. The ampel child env is scrubbed of SIGSTORE_ID_TOKEN, ACTIONS_ID_TOKEN_REQUEST_*, and AMPEL_* (ampel resolves policy context from AMPEL_<KEY> env vars). Part of #484. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…ject (#484) wrangle_subject_arg, wrangle_ampel_verify_args, wrangle_ampel, wrangle_verify_emit_vsa, wrangle_bnd_sign_args, and wrangle_sign_vsa collapse into wrangle_engine_verify: one VSA-gated toolbox run per subject in which the engine execs the in-image ampel, dual-checks the verdict (exit code AND the emitted VSA must agree), signs the VSA, writes the signed line, and appends it to the per-artifact bundle. bnd push github, cosign attach, and the release attach flow stay shell per the #485 boundary; wrangle_resolve_policy stays because the toolbox mount decision needs the resolved path host-side. Topology note, stated explicitly: the signing token now rides the same container as the ampel run (previously ampel ran token-less and bnd signed in a separate run). The engine strips SIGSTORE_ID_TOKEN / ACTIONS_ID_TOKEN_REQUEST_* / AMPEL_* from ampel's child env (asserted in go test); the request-URL vars still never enter any container. The in-shell wrangle_retry_once no longer wraps the verify run: the engine retries the ampel exec itself and carabiner-dev/signer retries the Sigstore hops; an in-shell re-run after a torn bundle append could double-append. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
jq -c exits 0 on empty input, so the old host-side non-empty guard on the signed VSA was not actually replaced by the jq flatten. Restore it: an engine that exits 0 without landing the line here (a mount mismatch) would otherwise push an empty statement to the store and the registry. Also drop the overclaiming retry comment — the in-shell retry is gone because a re-run after a completed bundle append would double-append, not because the signer's retry behavior is settled. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Source-verified (pkg/attest/vsa.go resultStringToSLSAResult): ampel maps both StatusPASS and StatusSOFTFAIL to verificationResult PASSED, and exits nonzero only on StatusFAIL. So the dual-check does NOT distinguish a SOFTFAIL from a PASS — and must not: wrangle's own wrangle-release-pinned tenet is an enforce: OFF advisory group that deliberately SOFTFAILs a branch build into a PASSED VSA. The SPEC and the test case claimed otherwise; both corrected. What the dual-check does catch is unchanged: an absent, unparseable, wrong-subject, wrong-predicate-type, empty-verdict, or FAILED-with-exit-0 VSA. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Owner
Author
|
claude: Status note — this shows Not fixing it now on purpose: this PR targets v0.5.0 and must be rebased + pins re-converged anyway after #780 merges and the attest-toolbox image republishes with the new engine. Rebasing it today would just have to be redone. It stays DRAFT until then — it is not ready and is not claiming to be. The verdict write-up for the spike is on #484. |
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.
Stage 2 of the #484 exec-ampel spike (stacked on #780): flip
actions/verify/run_verify.shfrom four toolbox calls per subject (ampel verify → bnd statement, each retried, plus shell hashing/append) to onewrangle-attest verifyrun per subject. Net −150 lines on the flip commit; ~90 lines of shell verdict/sign plumbing become engine code that is unit-tested in Go.DRAFT, targeting v0.5.0
#484 is deferred out of v0.4.0 — nothing here needs to land now. Additionally, the engine binary ships in the digest-pinned
attest-toolboximage. Until # merges, the image republishes, and the catalog auto-bump pins the new digest, the dispatch e2e here runs an image whose engine lacks theverifysubcommand and fails by design (fail-closed, exit 2 — the same shape as #761's ladder). Do not mark ready until #780 merges, the catalog bump lands, and this branch is rebased + pins re-converged.Merge as a MERGE COMMIT, not a squash — the branch carries pin-convergence cycle commits.
What moved, what stayed
Moved into the engine (
wrangle_subject_arg,wrangle_ampel_verify_args,wrangle_ampel,wrangle_verify_emit_vsa,wrangle_bnd_sign_args,wrangle_sign_vsa→wrangle_engine_verify): subject hashing, ampel argv, verdict handling, VSA sign, bundle append.Stayed shell, deliberately:
wrangle_resolve_policy— the brief said move it, but the toolbox--mountdecision needs the resolved policy path host-side; moving the 6-line resolver into the engine would still leave the mount logic in shell. Reported as a deviation, not smuggled.bnd push github,cosign attach, the whole release-attach flow (ghorchestration) — the wrangle-attest as the verify+attest engine — move the verify job's fiddly shell into tested Go #485 boundary.lib/toolbox_run.shuntouched.The in-shell
wrangle_retry_onceno longer wraps the verify run: the engine retries the ampel exec itself andcarabiner-dev/signerretries the Sigstore hops. Known small delta: a transientdocker run/image-pull failure of this one dispatch is no longer retried in-shell (an in-shell re-run after a torn bundle append could double-append a VSA line, which is worse).The topology change — read this before LGTM
Today ampel runs in a container without the signing token, and
bnd statementsigns in a separate run with it; a bats test asserted that separation. One-call-per-subject necessarily putsSIGSTORE_ID_TOKENin the same container as the ampel child. Mitigations: the engine execs ampel with the token (andACTIONS_ID_TOKEN_REQUEST_*,AMPEL_*) stripped from its env — asserted by Go test — and the mint-anything request vars still never enter any container (bats-asserted). Residual exposure, stated plainly: same-UID processes share/proc, so a compromised ampel could still read the engine's environ. Weighing it: ampel is our own digest-pinned build and already is the verdict oracle — a compromised ampel can already emit a PASSED VSA that we then sign — so the marginal gain for an attacker is signing arbitrary statements as the workflow identity during the token's short validity. Real but bounded. If that widening is unacceptable, the fallback that keeps this spike's verdict-protocol gains is a two-run split (wrangle-attest verify --no-signtoken-less + the existing--statement --appendwith the token, i.e. #761's shape) at the cost of the one-call collapse.Fail-closed accounting (old abort path → new home)
--artifactself-digest fails closed (Go test)set -eabort (bats + Go)--statement)--bundleprecheck (bats + Go)enforce: OFFadvisory group)wrangle-release-pinnedtenet relies on this. The dual-check does not and must not catch it.Every abort is now also classified: engine exit 1 = policy verdict FAILED, 2 = anything else — ampel itself cannot distinguish these on exit code (it exits 1 for both); the engine tells them apart by the emitted VSA, which ampel writes on FAIL but not on error.
Tests
79 → 70 bats (the deleted ones are subject-hash/bnd-argv tests whose logic moved to Go, covered by 14 new engine test functions); the full pinned-container suite (
./test.sh quick) is 1398 ok / 0 not ok, counted withgrep -c, not read off a tail. New real-binary integration bats drives the real engine into the real ampel over the exec seam. The token-threading assertions (name-only, no value on argv, no request vars in-container, grant/mint fail-closed, VSA-gate fail-closed) all carry over againstwrangle_engine_verify.An independent adversarial review (fresh context, briefed to find a failed-verify-read-as-pass path) ran before this PR was surfaced; findings: no fail-open found. Two of its three findings are fixed here (a restored host-side non-empty guard on the signed VSA —
jq -cexits 0 on empty input, so the old guard was NOT replaced as I had claimed; and an overclaiming retry comment). The third is surfaced below.Relationship to #761: this supersedes its
run_verify.shchange (the sign flip is subsumed by the engine call); the workflow-comment sweeps overlap and will conflict textually — sequencing is the owner's call.Part of #484 / #485 / #420.
🤖 Generated with Claude Code