Determine some more criteria for actions review

[potiuk]

Following the discussion in #674 (comment)

Possible criteria:

• Reproducibility — implemented in Add action build verification script #561, Add automated build verification for dependabot PRs #614, Enhance verify-action-build.py with deep composite/docker verification #629, verify-action-build: add node_modules verification for vendored deps #652, Retry JS verification with approved version's lock files on mismatch #674, Refactor verify-action-build into modular package with tests #675, verify-action-build: support .cjs and .mjs compiled bundles #734
• Code review of differences vs. past approved version — implemented in Improve verify-action-build.py CI output and add security review checklist #647, Add interactive open-in-browser and approve flow #671
• Passing cooldown (now 0 days) — implemented in Add cooldown to dependabot and expand .gitignore #560, dependabot: apply cooldown except for action reviews #712, Update README with current 0 day cooldown #737
• Correctness of the action (for example errors in build pipelines) — ongoing fixes in Fix verify-action-build.py for monorepo and non-dist output dirs #617, Fix verify-action-build.py Node.js version detection #628, Fix verify-action-build to try 'npm run start' for actions that use it #664, Fix Docker build for actions with multi-step builds #685, verify-action-build: skip pin warning for multi-stage FROM references #733, verify-action-build: support .cjs and .mjs compiled bundles #734
• Hash-pinning of composite actions — implemented in Enhance verify-action-build.py with deep composite/docker verification #629 (supporting fixes: Fix pypa/gh-action-pypi-publish: pin to commit SHA instead of branch pattern #619, Fix dtolnay/rust-toolchain: pin to commit SHA instead of branch name #620)
• Checking if downloaded binaries are verified — pending in verify-action-build: add binary download verification check #743
• Reject actions that ship pre-compiled native binaries (Go, Rust, C, etc.) in-tree without verifiable build provenance.
• Compatible licencing

Goal not criteria:

• Reject actions with suspicious builds, e.g. those that use different dependencies than what the source implies (proposed by @dave2wave) — partially addressed by verify-action-build warnings

Rejected:

- [ ] Attestations

Maybe others?

I would love to hear what others think.

[potiuk]

I'd personally add [x] hash pinning and [x] Compatible licencing (even if optional and default is not-compatible). Possibly we should add some checks for the latter if the actions are used properly.

[potiuk]

Also I think good idea when we reject some action we should follow up with the 3rd party - to at least let them know they should fix things (or even create PR). That should be IMHO default behaviour when we reject things.

[dave2wave]

#683 is for discussion of what cooldown periods should be recommended.

[dave2wave]

What is the point of enforcing "compatible licensing"? Policy allows any build tool as long as it is unmodified.

[dave2wave]

• Reject actions with suspicious builds like those that use different dependencies than what the source implies.

[potiuk]

• Reject actions with suspicious builds like those that use different dependencies than what the source implies.

I don't think this was "suspicious" - there is clear explanation on why it happened (and why it could have happened for others). The problem was (see my PR SonarSource/sonarqube-scan-action#228 ) was that their dependabot PRs did not have a check if dist was rebuilt and release was made after that without rebuiliding it with the new tools.

This likely common pattern that might happen to others. Problem with rebuilding in dependabot PRs is that such dependabot PRs will always fail and will have to be manually updated - when such compiled javascript is committed to the repo. Or there must be another mechanism implemented to prevent the release if rebuild is not done (let's see what SonarQube will respond to my PR). So I think there is nothing "suspicious" about it - this is an innocent build/release pipeline bug.

[dave2wave]

One criteria would be to list which repositories are impacted by the approval. We have capability to survey periodically all repositories and provide an inventory. This inventory could be inverted to list out every use of an action along with its current pin. The users of the action should be able to help judge the correctness of any upgrade.

[ppkarwasz]

I was experimenting with using Ampel to automatically check how actions have been released during a Dependabot PR.

For example aquasecurity/trivy-action now releases attestation that can be verified.

[potiuk]

Do you know how well it stacks against the actions we currently have in our "approved" list ?

[ppkarwasz]

I am working on it.

[ppkarwasz]

I perused an LLM to check which actions have additional authentication factors we could use. As it turns out aquasecurity/trivy-action seems the only one to publish an attestation.

Thinking about it a little bit more, attestations don't seem to be very relevant here, since they can only attest that the action's distribution was build from the source. Unless I am mistaken, actions are always built locally and committed to the same commit that does source code changes.

Actions with attestations (non-empty store)

Only pypa/cibuildwheel has actual attestations — 44 entries covering wheel releases
from v2.23.4 through v3.4.1. All other repos with an attestation store are empty.

Note: attestations cover the published wheel packages, not the action source itself.

Actions with attestations attached to the release

Only aquasecurity/trivy-action seems to publish an attestation, which is no longer allowed.

Actions with GPG-signed tags (verified=valid)

Repository	Tag
1Password/load-secrets-action	v4.0.0
browser-actions/setup-firefox	v1.7.1
dorny/test-reporter	v3.0.0
erisu/license-checker-action	v2.1.0
ilammy/setup-nasm	v1.5.2
sbt/setup-sbt	v1.1.22

Notable findings

• helm/chart-releaser-action has an annotated tag with reason=bad_cert — signed
  but the certificate is no longer trusted.
• 9 repos use unsigned annotated tags: bazel-contrib/setup-bazel, biomejs/setup-biome,
  JetBrains/qodana-action, JustinBeckwith/linkinator-action, matlab-actions/run-tests,
  matlab-actions/setup-matlab, mozilla-actions/sccache-action, nwtgck/actions-netlify,
  pypa/cibuildwheel, runs-on/action.
• The remaining 60+ actions use lightweight tags with no signing at all.
• 2 entries (browser-actions/setup-geckodriver, damccorm/tag-ur-it) have no tag
  comment in action.yml and were not checked.

[potiuk]

Thinking about it a little bit more, attestations don't seem to be very relevant here, since they can only attest that the action's distribution was build from the source. Unless I am mistaken, actions are always built locally and committed to the same commit that does source code changes.

Quite agree.

[potiuk]

I updated the issue to reflect the current state of proposed checks:

Also @dave2wave

• Reject actions with suspicious builds like those that use different dependencies than what the source implies.

I think that really depends on the type of "issue" - but I would agree in general. I am continuously updating the "verify" action whenever we find an action that needs it - so this rule might also be pretty conditional. My idea is that if:

a) we are able to explain what's going on
b) there is no security risk

Warning and action of addressing it (for example opening a PR or issue in action repository) might be better choice than blocking. In the verify script those things might cause "error" (this is what I did with lack of dist rebuilding after dependencies updated in #662 - is a good "middle ground" for those - they will be flagged with failing check, but from the output it will be clear if the only issue is because old dependencies were used to build it (which is a relatively common issue for those who implemented trivial dependabot upgrade check without running rebuild after.