Skip to content

[CI workflows] GITHUB_TOKEN permissions hardening; CI cleanup/optimizations#926

Open
kdmukai wants to merge 5 commits into
SeedSigner:devfrom
kdmukai:2026-04-22_CI_hardening
Open

[CI workflows] GITHUB_TOKEN permissions hardening; CI cleanup/optimizations#926
kdmukai wants to merge 5 commits into
SeedSigner:devfrom
kdmukai:2026-04-22_CI_hardening

Conversation

@kdmukai
Copy link
Copy Markdown
Contributor

@kdmukai kdmukai commented Apr 22, 2026

Summary

  • Pin GITHUB_TOKEN to "least privilege" (read-only) across CI workflows
  • Small cleanups in tests.yml and build.yml.

1.): Workflow permissions hardening

GitHub Actions inject a GITHUB_TOKEN that provides access to the repo. The default scope comes from two places:

  • the repository's default setting (Settings → Actions → General → Workflow permissions)
  • any permissions: block declared in the workflow itself.

A GITHUB_TOKEN withcontent: write permissions would be difficult to extract from a CI job (malicious CI steps would have to slip past review and get merged) but would be extremely dangerous if successful (more or less equivalent to giving an attacker full maintainer privileges for that repo).

That sounds scary, but note that PRs from downstream forks are ALWAYS limited to a read-only GITHUB_TOKEN in the related PR CI runs. This is why a malicious CI job would have to be merged first to be successful. So it's a highly unlikely scenario and so I do NOT think the current workflows (which don't declare explicit permissions) are at risk.

However, another way to extract the GITHUB_TOKEN would be if one of the actions we rely on were compromised (e.g. actions/checkout@v4). The worst-case scenario would be: we provide a writable token to a CI job that already runs internal to our repo (i.e. not in a PR context where it's inherently limited to read-only) AND one of the actions it relies on is compromised.

But if we explicitly limit the GITHUB_TOKEN to read-only in the CI job (what this PR does), then even with a compromised action we would have zero real exposure (the attacker could also read any env vars in the CI job, but we don't inject any secrets via env vars).


tldr:

Making the GITHUB_TOKEN explicitly read-only in each CI workflow makes it obvious if a malicious PR tries to elevate it to writable AND protects us against an action step being compromised out from under us.

This aligns with GitHub's own guidance to grant the token the least required access.

No step in these workflows currently writes anywhere, so this has zero effect on these jobs.


Notes for the future:

  • Probably cannot consider any legitimate use cases for CI changes that would enable a GITHUB_TOKEN with contents: write in any CI job in our main repo.
    • pull-requests: write is less damaging but would still need thorough discussion.
  • If other less critical repos end up wanting to use a GITHUB_TOKEN with any write permissions, careful consideration should be taken if any third-party actions are used (e.g. marocchino/sticky-pull-request-comment@v2).
  • A future hardening step would be to explicitly pin all actions, whether they be GitHub-provided or third-party, to a specific commit:
    uses: marocchino/sticky-pull-request-comment@efaaab3fd41a9c3de579aba759d2552635e590e8 # v2.9.0
    

2.) Cleanups suggested by Claude

2.a.): Cleanup — tests.yml

  • timeout-minutes: 20 on the test job — cap runaway test hangs (default is 6h).
  • apt-get install -y --no-install-recommends libzbar0 — defensive flags.
    • -y avoids interactive prompts on future apt versions.
    • --no-install-recommends skips libzbar0's "Recommends" entry (libmagickcore-*-extra, which pulls in ImageMagick's format-support extras and a tree of graphics libraries via transitive deps) — we only need the core zbar library for QR decoding.
  • Dropped python -m pip install --upgrade pipsetup-python already ships a
    recent pip.
  • mkdir artifactsmkdir -p artifacts — defensive against re-runs.
  • Dropped --cov-append from the first pytest invocation — there's no existing
    .coverage to append to on the first run; kept on the screenshot-generator
    run where it's actually doing something.
  • @muhahahmad68 contributed: pip caching to be a tiny bit more efficient (only saves a few seconds per run). Closes CI: Add pip dependency cache to tests workflow to reduce runtime #917.

2.b.): Cleanup — build.yml

  • Added a concurrency: block with github.ref in the fallback — cancels
    superseded runs on the same branch/PR. Chose github.ref (rather than
    github.sha as in tests.yml) because the 30-min-per-target build has no
    downstream consumers for intermediate commits; only the latest commit per ref
    needs to be built.
  • timeout-minutes: 120 on the build job — caps the blast radius of a hung
    build without risking a legitimate cache-miss run being killed. Recent runs took up to 78 minutes.
  • Inline comment on the delete unnecessary files step explaining why only
    src/ is kept from the seedsigner checkout.
  • Simplified the SHA-abbreviation step in the sha256sum job: replaced
    git init + git rev-parse --short with the bash substring
    ${GITHUB_SHA::7}. Same output, no throwaway .git directory, dropped the
    unused step id.

This pull request is categorized as a:

  • Other: Minor CI security hardening, CI cleanup/optimization

Checklist

I ran pytest locally

  • N/A

I included screenshots of any new or modified screens

Should be part of the PR description above.

  • N/A

I added or updated tests

Any new or altered functionality should be covered in a unit test. Any new or updated sequences require FlowTests.

  • N/A

I tested this PR hands-on on the following platform(s):

  • N/A

I have reviewed these notes:

  • Keep your changes limited in scope.
  • If you uncover other issues or improvements along the way, ideally submit those as a separate PR.
  • The more complicated the PR, the harder it is to review, test, and merge.
  • We appreciate your efforts, but we're a small team of volunteers so PR review can be a very slow process.
  • Please only "@" mention a contributor if their input is truly needed to enable further progress.
  • I understand

Thank you! Please join our Devs' Telegram group to get more involved.

Copy link
Copy Markdown
Contributor

@Chaitanya-Keyal Chaitanya-Keyal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 0.9.0 Needs Code Review

Development

Successfully merging this pull request may close these issues.

CI: Add pip dependency cache to tests workflow to reduce runtime

3 participants