Repository navigation
fix(ci): enforce least-privilege GitHub Actions token permissions - #1011
Conversation
Declare read-only defaults across GitHub Actions workflows and pass required job scopes through reusable workflow callers. Limit CodeQL, benchmark comments, release creation, and image attestation to their required permissions. Fixes kubescape#995 Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
matthyx
left a comment
There was a problem hiding this comment.
Reviewed 8863e2312d97c3bf3f6d90c9a92597deedf4e3c7 against main at 67e455e040520a3f1f5a4ac20fbf5fe25f676d43. Verdict: needs clarification; no confirmed correctness blocker found.
The hardening is needed for #995: parsing the target finds nine of ten workflows without top-level permissions; the head has none missing. All five local reusable-workflow edges have compatible explicit job scopes. Release creation keeps contents-write, Quay uses registry credentials, and private E2E operations use the App token. The pinned attestation action documents artifact-metadata-write for storage records; its absence previously produced a warning, not necessarily an attestation failure.
History: #135 proposed read-all in two workflows and closed unmerged, with no documented maintainer rejection. #778 removed benchmark job permissions to fix a release startup failure; this PR addresses that objection by updating both callers. #710 and #833 are incorporated permission/provenance predecessors. Open #1015 overlaps workflow files but pins actions rather than replacing this change. Searches across PR states and issues used permissions, read-all, Token-Permissions, benchmark, CodeQL and startup_failure, capped at 100/query; the broad benchmark search hit that cap. No superseding fix found within those limits.
Validation: git diff --check passed. YAML parsing and a caller/callee permission-map check passed. actionlint -shellcheck= -pyflakes= -oneline reported the same eight diagnostics on base and head, with no new findings. The component suite and pin check succeeded for this exact head. Go tests, Scorecard, release, benchmark and CodeQL execution were not run locally; repository scripts were not executed. The green checks do not validate the changed release/CodeQL chains because their triggers exclude workflow-only changes. No inline threads or human reviews were present when rechecked.
Two points before approval:
- Please confirm
mainis the intended target despite the repository PR template requestingdevfor non-documentation changes. - Please provide evidence validating the changed release and CodeQL permission paths, or maintainer confirmation of an acceptable static-validation gate for this workflow-only change. The description's statement that no workflows ran should also be updated to distinguish the successful component/pin checks from these untested paths.
The unused PR-write capability on the release benchmark is a nonblocking architecture WATCH already covered by CodeRabbit; I am not duplicating that feedback. No source changes or merge actions were performed.
Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com> Signed-off-by: Gauarv Chaudhary <137998824+ANAMASGARD@users.noreply.github.com>
matthyx
left a comment
There was a problem hiding this comment.
Rechecked head 6a7779045d8c0b7eaa416b3944cb595e58c4014e against main at 67e455e040520a3f1f5a4ac20fbf5fe25f676d43. Verdict: needs clarification on the remaining release validation gate; no confirmed correctness blockers.
The follow-up resolves the target-branch question: main is the default and dev does not exist. It also replaces broad reusable-workflow defaults with contents: read and adds a repeatable checker. The hardening remains needed for #995: the target still lacks nine explicit workflow defaults. All five nested call edges now have compatible declarations.
Fresh validation:
- Ran
python scripts/check-workflow-permissions.pyin a restricted, credential-free, network-disabled container: all 10 defaults, five call edges and 11 operation maps passed. The same checker rejects the target revision and all 23 negative mutations (four broad reusable defaults and 19 removed write grants). git diff --checkpassed. Actionlint reports the same eight existing diagnostic identities on base and head, with no new diagnostics. No application tests were run locally.- The exact-head PR-created run passed; its logs explicitly confirm successful SARIF upload and completed processing. This resolves the earlier CodeQL evidence gap.
- Exact-head component tests and pin check passed.
- The benchmark run completed and uploaded artifacts, but its PR comment received HTTP 403 (
Resource not accessible by integration); the fork token had read-only PR permission. Existing error tolerance makes the run green. This is not evidence of successful comment publication or a newly introduced defect.
History rechecked across open/closed/merged PRs and issues using permissions, read-all, Token-Permissions, CodeQL and startup_failure terms (100 results/query; no query reached the cap). #135 was closed unmerged without a documented rejection; #778's caller-ceiling failure is addressed here. #710/#833 remain incorporated predecessors. #1015 has since merged and pins actions; it does not supersede permission hardening. No superseding fix was found within these search limits.
Independent correctness review found no actionable defect. Architecture remains WATCH for unused PR-write authority in release benchmarks, already documented by CodeRabbit; no duplicate inline feedback added.
One earlier gate remains: no run inspected exercises this head's merged-release/bypass chain, including OIDC, attestation, release creation and private E2E. Is the reproduced static permission check acceptable for those release-only paths, or is an upstream validation run required? Approval remains withheld pending that decision. This does not require a real production release merely to test the PR.
Immediately before this verdict, the PR remained open, non-draft and mergeable, with the same head/base and no new human review or inline comments. Branch protection still requires an approval including last-push approval. No merge, source edits or repository-setting changes were made.
Overview
Declare read-only workflow defaults and grant write access only to the jobs that need it for CodeQL uploads, benchmark PR comments, releases, image signing and attestations. Remove unnecessary GitHub Packages access because images are published to Quay using registry credentials.
Six workflows use
permissions: read-all. The four reusable workflows use the narrowercontents: readdefault to fit within their callers' explicit permission ceilings. Required job-level grants remain explicit at every level of each nested call.Fixes #995
Target branch
mainis the intended target. Verified on 2026-10-16: upstream's default branch ismain, norefs/heads/devexists, and the release workflow handles merged PRs targetingmain. The template's request to targetdevcannot currently be followed. This confirms my target choice; maintainer approval is still required.How to Test
Reproducible check and permission-path evidence: docs/WORKFLOW_PERMISSIONS.md.
Local validation for
6a7779045d8c0b7eaa416b3944cb595e58c4014e:67e455e040520a3f1f5a4ac20fbf5fe25f676d43; eight pre-existing findings remain, including obsolete CodeQL v2 runtimes.git diff --checkand Python syntax validation passed.8863e231. It was not rerun for this head because the binary download failed during the host network outage.Hosted evidence and approval gate
The component suite and pin check succeeded for earlier head
8863e2312d97c3bf3f6d90c9a92597deedf4e3c7. They do not prove release or CodeQL execution, or validate this follow-up. This corrects the earlier statement that no workflows ran.The original workflow-only diff was excluded by the release/CodeQL trigger filters. The new Python checker makes this follow-up eligible for the PR-created chain; its hosted result must be inspected separately. The merged-release path still needs a qualifying upstream event.
The evidence above validates declared permissions statically. Runtime token issuance, OIDC, registry publishing, release creation, SARIF upload, private E2E and benchmark execution remain unverified. Maintainer acceptance of this static gate or upstream execution of the affected operations remains necessary. The nonblocking observation about unused release-benchmark PR-write authority remains documented.
Checklist before requesting a review
Application tests are not the local validation gate for this workflow-permission change; the exact checks and remaining hosted gate are recorded above.
Summary by CodeRabbit