From fe02f2e498493f327ec639ccceddb6121a3bcd19 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 31 Aug 2026 06:27:39 +1000 Subject: [PATCH 1/2] ci(security): tamper-evidence for deny.toml and security.yml (LAB-1151) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The supply-chain gate read its policy and its own definition from the PR head, so one commit could violate a ban and remove the ban. Deletion already fails closed (required context, no bypass actors); this adds the modification half: a tripwire step that diffs deny.toml and security.yml against the PR base and fails the required check unless the PR body carries [gate-change-approved]. Residual (documented in-step and in README): the tripwire itself lives in the PR head; removing it in the same commit is deliberate evasion, only closable by an org-ruleset workflows rule — escalated, not self-served. --- .github/workflows/security.yml | 45 ++++++++++++++++++++++++++++++++++ Makefile | 5 ++-- README.md | 40 ++++++++++++++++++++++++++++-- 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index da24f7e..730f9dc 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -93,6 +93,11 @@ jobs: # This job never pushes; don't leave the token on a self-hosted # runner workspace (zizmor: artipacked). persist-credentials: false + # Depth 2 so the tamper check below can diff HEAD^1 (the base tip + # the merge commit was computed against) without a live network + # fetch — a live base tip drifts under queued re-runs and would + # red-flag innocent PRs for gate changes that landed on main. + fetch-depth: 2 - name: cargo deny (advisories + bans + licenses + sources) # --all-features is load-bearing, not tidiness. cargo-deny builds a @@ -126,3 +131,43 @@ jobs: # Runs even when the step above failed so one run shows both verdicts. if: ${{ !cancelled() }} run: cargo audit + + - name: Gate tamper check (deny.toml / security.yml vs base) + # This job reads its own policy (deny.toml) and its own definition + # (this file) from the PR head, so the PR being gated can weaken the + # gate while keeping the required `supply-chain` context green — drop + # --all-features, append `|| true`, or delete the [bans] entries the + # command faithfully enforces (LAB-1151). Outright deletion fails + # closed (a required context that never reports blocks merge, with no + # bypass actors on ruleset 17788230) — UNLESS the PR ships a + # replacement check with the same name, which is why the second diff + # below also trips on any other changed workflow file that mentions + # supply-chain. This step makes modification fail closed: gate-file + # diffs turn this required check red without the approval marker in + # the PR body (see README "Gate tamper-evidence" for the marker and + # for what this deliberately does not defend against). + # + # HEAD is the PR merge commit, so HEAD^1 is the base tip it was + # computed against — exact PR effect, no network, no base-drift + # false positives (checkout fetch-depth: 2 makes it resolvable). + # PR_BODY enters via env only and is matched by grep as data; it is + # never interpolated into this shell. + if: ${{ github.event_name == 'pull_request' && !cancelled() }} + env: + PR_BODY: ${{ github.event.pull_request.body }} + run: | + set -o pipefail + changed=$(git diff --name-only HEAD^1 HEAD -- deny.toml .github/workflows/security.yml | tr '\n' ' ') + shadow="" + while IFS= read -r f; do + [ -n "$f" ] || continue + if git grep -qe supply-chain HEAD -- "$f"; then shadow="$shadow$f "; fi + done < <(git diff --name-only HEAD^1 HEAD -- '.github/workflows/' ':!.github/workflows/security.yml') + if [ -z "$changed$shadow" ]; then + echo "Gate files unchanged vs PR base." + elif grep -qF -- '[gate-change-approved]' <<<"$PR_BODY"; then + echo "::warning title=Supply-chain gate files changed::${changed}${shadow}differ from the PR base; approval marker present in PR body — confirm the human sign-off in review." + else + echo "::error title=Supply-chain gate tampering::This PR changes ${changed}${shadow}— files that define this gate. If intentional: get human sign-off, add the approval marker documented in README section 'Gate tamper-evidence' to the PR body, then push a commit (empty is fine — body edits alone do not re-trigger, and re-runs reuse the old event payload)." + exit 1 + fi diff --git a/Makefile b/Makefile index 4290544..e231e02 100644 --- a/Makefile +++ b/Makefile @@ -26,9 +26,10 @@ test-wasm: CARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUNNER=wasm-bindgen-test-runner \ $(CARGO) test -p cachekit-rs --target wasm32-unknown-unknown --no-default-features --features workers,cachekitio,encryption,macros --test wasm_session_tests -# Supply-chain gate — the same commands CI runs in +# Supply-chain gate — the same enforcement commands CI runs in # .github/workflows/security.yml, so a local pass means a CI pass. (CI runs the -# audit step even when deny fails; make stops at the first failure.) +# audit step even when deny fails, plus a PR-only gate tamper check with no +# local equivalent; make stops at the first failure.) # Kept out of `quick-check`: both tools fetch the RustSec advisory database over # the network, which does not belong in a per-commit loop. # Why both tools, and why --all-features: see the table in README.md. diff --git a/README.md b/README.md index a771728..d2bc207 100644 --- a/README.md +++ b/README.md @@ -505,8 +505,10 @@ make build # cargo build --release make build-wasm # wasm32-unknown-unknown (workers feature) ``` -`make security` runs the same two commands as the `supply-chain` job in -`.github/workflows/security.yml`, so a local pass means a CI pass. It needs +`make security` runs the same two enforcement commands as the `supply-chain` +job in `.github/workflows/security.yml`, so a local pass means a CI pass +(the job's final step, the gate tamper check below, is PR-context-only and +has no local equivalent). It needs `cargo-deny` and `cargo-audit` installed, and it reaches the network to refresh the RustSec advisory database — which is why it is not folded into `quick-check`. @@ -529,6 +531,40 @@ reintroduced behind an optional feature passes a bare `cargo deny check`. and `toxiproxy_rust`, because this SDK is rustls-only. Run `make deny` before adding or bumping a dependency. +### Gate tamper-evidence + +The `supply-chain` check reads both its policy (`deny.toml`) and its own +definition (`security.yml`) from the PR head, so a PR could weaken the gate it +is being graded by — delete a `[bans]` entry, or drop `--all-features` while +keeping the job name green. Two properties defend against that: + +- **Deletion fails closed.** `supply-chain` is a required status check with no + bypass actors; a PR that deletes the workflow leaves the context unreported + and the PR permanently unmergeable. The one deletion variant that would not + fail closed — shipping a replacement check under the same name — is why the + wire below also trips on any other changed workflow file mentioning + `supply-chain`, and why the required check is pinned to the GitHub Actions + app, so an API-posted commit status cannot impersonate it. +- **Modification trips a wire.** The job's final step diffs `deny.toml` and + `security.yml` against the PR's base and fails the required check on any + change, unless the PR body contains the exact, case-sensitive string + `[gate-change-approved]` (add it after human sign-off, *then* push a commit + — the marker is read from the push-time event, so a body edit alone does + not re-trigger). Legitimate policy updates therefore stay possible, but + only as a conscious, loudly-marked act. + +What this does **not** defend against: a PR that edits the tamper-check step +itself out in the same commit; an author who self-serves the marker without +sign-off; and a marker hidden inside an HTML comment, which satisfies the +check but is invisible in the rendered body — when reviewing a gate-file +diff, check the raw PR body, not just the rendered view. All are deliberate +evasion, not the lazy path — each leaves an explicit trail in a reviewed +diff or the PR body source. Closing the first mechanically requires an +org-ruleset `workflows` rule pinning `security.yml` to an out-of-tree ref +(an org-scope decision, tracked on LAB-1151), which would still not protect +`deny.toml` — the wire above remains the only guard on the policy file +itself. + ## Minimum Supported Rust Version **Rust 1.85** or later (Edition 2021). From fe08ebd6aa6c48ac1c81967c18547f51ac12f5fd Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 31 Aug 2026 07:27:49 +1000 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20coderabbit=20review=20?= =?UTF-8?q?=E2=80=94=20state=20the=20delete-and-replace=20residual=20hones?= =?UTF-8?q?tly?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The README bullet and the workflow comment both implied the shadow-workflow wire covers a same-named replacement check unconditionally. The wire lives in security.yml, so deleting that file and shipping the replacement in one commit removes the wire with it. Name that case explicitly as the first documented residual instead of claiming coverage; the mechanical closure (org workflows rule) stays escalated on LAB-1151 per the ticket's org-scope boundary. CodeRabbit-Resolved: security.yml:165:Move the tamper check outside --- .github/workflows/security.yml | 9 ++++++--- README.md | 19 ++++++++++++------- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 730f9dc..f5f5681 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -140,9 +140,12 @@ jobs: # command faithfully enforces (LAB-1151). Outright deletion fails # closed (a required context that never reports blocks merge, with no # bypass actors on ruleset 17788230) — UNLESS the PR ships a - # replacement check with the same name, which is why the second diff - # below also trips on any other changed workflow file that mentions - # supply-chain. This step makes modification fail closed: gate-file + # replacement check with the same name. The second diff below catches + # that, but only while this file still runs: delete this file and ship + # the replacement in one commit and the wire dies with it. That + # residual is documented in README "Gate tamper-evidence" and is only + # closable out-of-tree (org workflows rule — tracked on LAB-1151). + # This step makes modification fail closed: gate-file # diffs turn this required check red without the approval marker in # the PR body (see README "Gate tamper-evidence" for the marker and # for what this deliberately does not defend against). diff --git a/README.md b/README.md index d2bc207..5b64392 100644 --- a/README.md +++ b/README.md @@ -540,11 +540,14 @@ keeping the job name green. Two properties defend against that: - **Deletion fails closed.** `supply-chain` is a required status check with no bypass actors; a PR that deletes the workflow leaves the context unreported - and the PR permanently unmergeable. The one deletion variant that would not - fail closed — shipping a replacement check under the same name — is why the - wire below also trips on any other changed workflow file mentioning - `supply-chain`, and why the required check is pinned to the GitHub Actions - app, so an API-posted commit status cannot impersonate it. + and the PR permanently unmergeable. A replacement check shipped under the + same name trips the wire below (it fires on any other changed workflow file + mentioning `supply-chain`) — but the wire lives in `security.yml`, so it + only fires while that file still runs. Deleting it and shipping the + replacement in the same commit removes the wire with it; that + self-protection gap is the first residual listed below. Either way an + API-posted commit status cannot impersonate the check: it is pinned to the + GitHub Actions app. - **Modification trips a wire.** The job's final step diffs `deny.toml` and `security.yml` against the PR's base and fails the required check on any change, unless the PR body contains the exact, case-sensitive string @@ -553,8 +556,10 @@ keeping the job name green. Two properties defend against that: not re-trigger). Legitimate policy updates therefore stay possible, but only as a conscious, loudly-marked act. -What this does **not** defend against: a PR that edits the tamper-check step -itself out in the same commit; an author who self-serves the marker without +What this does **not** defend against: a PR that removes the tamper-check +step in the same commit — by editing it out, or by deleting `security.yml` +outright while shipping a same-named replacement check that reports the +required context; an author who self-serves the marker without sign-off; and a marker hidden inside an HTML comment, which satisfies the check but is invisible in the rendered body — when reviewing a gate-file diff, check the raw PR body, not just the rendered view. All are deliberate