test: merge-gate the SEC-4045 workflow injection and action pinning fixes - #52
Merged
Merged
Conversation
The workflow-injection regression test asks a question about the YAML
tree — "does any `run:` scalar contain a `${{ }}` expression" — so it
needs a parser. Matching the text instead finds the same marker in a
comment or an input description and reports it as a finding; the
verify-trust action carries exactly such a comment.
Added as a workspace dev-dependency and taken by verify-trust, which is
the crate the composite action wraps.
Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
SEC-4045 reproduced two CI findings: the verify-trust composite action
pasted `${{ inputs.* }}` into its `run:` script, where an input holding
`$(…)` became a command, and the workflows named actions by mutable tag
in jobs holding `contents: write` or an OIDC token. Both were fixed by
changing the shape of the YAML — values moved into `env:`, refs replaced
by commit SHAs — so the gate asserts that shape.
`action-selftest.yml` already catches the injection at runtime, by
calling the action with shell syntax in its inputs. This is the other
half: a structural check over every definition file, including the ones
no runtime test exercises, that fails at review time rather than needing
a runner.
The new test parses each file under `.github/workflows/` and
`.github/actions/` and walks the resulting tree:
- no `${{ }}` expression appears inside any `run:` scalar;
- every `uses:` names a 40-hex commit SHA, local `./…` paths aside.
It parses rather than greps because the injection fix left a comment in
action.yml explaining that `${{ }}` must not appear in a script body,
and several input descriptions mention expressions too. A text search
reports all of those. A YAML comment belongs to no scalar, so the
tree-walk ignores prose while still seeing a `${{ }}` hidden in a shell
comment inside a `run:` block, which GitHub does substitute.
Failures name the file, the position in the tree (`jobs.test.steps[4]`)
and the step's `name:`. Two further tests keep the gate honest: one
asserts the walk actually reached `run:` and `uses:` scalars in both
directories, so a rename cannot turn the checks green by leaving them
nothing to read; the other runs both checks over an inline fixture that
carries the marker four times outside a script and once inside one, and
asserts exactly one finding.
Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
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
Merge-gate regression tests for the SEC-4045 CI findings, plus a check of what
the exploits the programme reproduced are already gated by.
The one gap was the CI half.
.github/actions/verify-trust/action.ymlinterpolated
${{ inputs.* }}into itsrun:script (VGI-H3), and everyworkflow named actions by mutable tag (VGI-N2). #42 and #45 fixed both by
changing the shape of the YAML — values moved into
env:, refs replaced bycommit SHAs. Nothing asserted that shape, so a future edit could undo it and
still merge green. This adds a structural gate that does.
The three
did-git-signfindings turned out to be gated already, by tests #44shipped with its own fix. Rather than write a second copy, I reverted each fix
and checked the existing gate fails; the results are below. No revert survives
in this branch —
git diff origin/mainis the four files in the diff.What is new
crates/verify-trust/tests/ci_workflow_hardening.rs, four tests, andyaml-rust2as a workspace dev-dependency.The tests read every
*.yml/*.yamlunder.github/workflows/and.github/actions/, parse each one, and walk the resulting tree. Compositeactions are covered as well as workflows, because that is where the injection
was, and where a
run:script is most exposed: its inputs come from whoevercalls the action.
no_expression_is_interpolated_into_a_run_scriptGuards VGI-H3, reproduced by
gha_injection_sim.sh: GitHub substitutes anexpression into a script as raw text before any shell parses it, so an input
holding
$(…)becomes a command. Asserts no${{ }}appears inside anyrun:scalar. The fix is to bind the value in
env:and reference it as a quotedvariable, and the failure message says so.
every_action_reference_is_pinned_to_a_commit_shaGuards VGI-N2. Asserts every
uses:names a 40-hex commit SHA. Only a local./…path is exempt: it resolves inside this repository, at the commit undertest. Anything else — a tag, a branch, or a
docker://image without a digest —fails and gets looked at, because in a job holding
contents: writeor an OIDCtoken a mutable ref runs whatever the action's owner points it at after review.
the_audit_reads_every_workflow_and_action_definitionBoth checks above pass when they find nothing, so this asserts they found
something: workflows and actions both contributed files, and the walk reached
real
run:anduses:scalars. Without it, renaming a directory or breakingthe walk would turn the gates green rather than red.
the_checks_flag_a_reintroduced_injection_and_ignore_proseRuns both checks over an inline fixture carrying
${{ }}four times outside ascript (a YAML comment, an input description, an input default, an
env:binding) and once inside one, plus a pinned, a local and a floating
uses:. Itasserts exactly one finding from each check, and that each names the offending
step.
This is why the gate parses instead of grepping. The fix for the injection left
a comment in
action.ymlsaying${{ }}must not appear in a script body, andseveral input descriptions mention expressions too; a text search reports all of
them, which is how a string-matching check earlier in this programme produced
false hits on its own explanatory prose. A YAML comment belongs to no scalar, so
the tree-walk ignores prose — while still catching a
${{ }}hidden in a shellcomment inside a
run:block, which GitHub does substitute.Failures name the file, the position in the tree, and the step's
name::action-selftest.yml(#42) already catches the injection at runtime, by callingthe action with shell syntax in its inputs. This is the complementary half: it
covers the files no runtime test exercises, and it fails at review time without
needing a runner.
Red, then green
Every revert below was undone in the same shell invocation that applied it, by a
trap, and each phase printed
git diff origin/main -- <file>afterwards to provethe file was back. The reverts never left the working tree.
Injection gate —
${{ inputs.range }}back in the action'srun:Pinning gate —
Swatinem/rust-cacheback to a mutable tagInjection gate, in a workflow, to show the job in the message
Green, on this branch:
4 passed; 0 failed … finished in 0.00s.Hermetic when red as well as green. The gate reads files and parses them. It
opens no socket, spawns no process, and resolves no name, in either state — the
red runs above took 0.20–0.28 s wall clock including cargo's own overhead, and
the test binary itself reported 0.00 s.
The
did-git-signfindings were already gatedEach fix from #44 was reverted, the existing suite run, and the revert undone in
the same invocation.
bypass_requested()readsDID_GIT_SIGN_BYPASS_POLICYunconditionally again (H2)tests/policy_bypass.rs::normal_builds_ignore_the_bypass_envallowed: true, bypass: trueparent_is_allowedback tostarts_with("git")(H1)policy::tests::programs_that_merely_start_with_an_allowed_name_may_not_sign,policy::tests::other_or_unknown_parents_are_refusedgitleakscheck_namespaceaccepts every namespace (H1 friction)sign::tests::only_the_git_namespace_may_sign,sign::tests::a_non_git_namespace_is_refused_before_anything_is_readAdding a second set would have duplicated them, so this PR does not. Those red
runs were hermetic too: the namespace revert lets
handle_signcontinue pastthe check, and it stops at the missing buffer file —
failed to read file to sign: /nonexistent/did-git-sign/buffer— before the VTA is contacted. 1.9 s oftests, no egress.
What is not gated, and why
H1's agreed remediation has not shipped. The plan's real fix is to sign
inside the VTA.
sign.rsstill callsvta::get_signing_key→get_key_secret, so the Ed25519 seed still reaches the client, and a processrunning as the user can spoof a parent named
git— or just run real git — andget a signature.
live_signing_oracle.txtreproduces exactly that, and thebehaviour is unchanged on
main. There is no gate to write for it: what #44shipped is honest documentation plus two pieces of friction, and those are what
the table above covers. This stays open as the accepted residual risk the
remediation plan describes, tracked by the VTA-side signing work.
One hole in the H2 gate is closed only by CI, not by
cargo test.normal_builds_ignore_the_bypass_envis#[cfg(not(feature = "insecure-policy-bypass"))], so if that feature were ever added to[features] default, the test would disappear rather than fail andcargo test --workspacewould stay green. What catches it isci.yml'scargo build --release --locked -p did-git-signstep, which hits thecompile_error!inpolicy.rs, and the same build insidecargo packageonpublish — so the bypass cannot reach a release either way. Closing it inside
cargo testneeds a TOML parser to read the manifest; I have not added a secondparser dependency for it. Flagging it rather than leaving it implied.
Verification
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspaceRebased onto
origin/mainatdd3ea2b(vta-sdk 0.38.0,didwebvh-rs0.6.1dropped). The lockfile diff is
yaml-rust2and its two transitive crates,nothing else.
randstays at 0.8 —pgp0.20 needsrand_core0.6 RNGs.No keys or private material; the tests read only files already in the repository
and one inline fixture.