ci: stop lending the checkout token to PR code - #3
Closed
letientai299 wants to merge 2 commits into
Closed
Conversation
CodeRabbit on !354, the back-port of #1, flagged the workflows the MR carried across unchanged. Three of the four findings hold. `ci.yml` and `wheel.yml` check out with credentials so the CUBIN packs can be pulled, and every step after that -- `setup.py`, the test suite -- is code a pull request controls. actions/checkout only clears the auth header in its post step, which runs last, so an untrusted step had a usable token in `.git/config` for the length of the job. The drop goes in the cubin-packs action rather than in each caller, next to the pull that is the only reader. `Vulnerability-scan` is the one job in blossom-ci.yml that checks out unreviewed contributor code, and it inherited `pull-requests: write` from the workflow with nothing to use it; a job-level block replaces the inherited set with what blossom-action demonstrably needs. The fourth asked for `github.actor == 'blossom-jenkins-bot'` on Upload-Log. That login is not published, and a wrong guess fails silent -- the run goes green and the log link never posts. spark-rapids and nvMolKit both gate on `github.event_name` alone, so the `args` check here is already stricter. Left as a comment naming when to pin it. Signed-off-by: Tai Le <taile@nvidia.com>
The first attempt at this was a no-op. It unset `http.https://github.com/.extraheader` in `.git/config`, which is where checkout v5 and earlier put the token -- v7 writes it to a file under RUNNER_TEMP and points `.git/config` at that file with `includeIf.gitdir:`. The step reported "no checkout auth header to drop" and passed while the credential stayed readable for the rest of the job, which is the failure CodeRabbit and zizmor were pointing at in the first place. Clear both shapes, and remove the RUNNER_TEMP file rather than only unlinking it from the config, since that file is what holds the secret. Then assert it: plain `git config --get-all` reads the effective configuration, following any `includeIf` still in place, so the step now fails if either branch missed. A checkout bump that moves the credential again turns this back into a no-op otherwise -- exercised against the v5 layout, the v7 layout, a caller with no credentials at all, and a credentials file the cleanup does not recognise, which fails the step. Signed-off-by: Tai Le <taile@nvidia.com>
Collaborator
Author
|
/build-ci |
letientai299
changed the title
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
CodeRabbit reviewed !354, the back-port of #1, and flagged the
workflows that MR carried across unchanged. Addressing it here rather than
there, because
.github/**is authored on this side and syncs out. Three ofthe four findings hold.
Checkout credentials outlive their reader.
ci.ymlandwheel.ymlcheck out with credentials so
git lfs pullcan fetch the CUBIN packs bypattern, and every step after that —
setup.py, the test suite — is code apull request controls.
actions/checkoutclears the auth header only in itspost step, which runs after all of it, so an untrusted step had a usable
token sitting in
.git/configfor the length of the job. The drop goes in.github/actions/cubin-packs, next to the pull that is the only reader,rather than being repeated in both callers.
Vulnerability-scanheld a scope it never used. It is the one job inblossom-ci.ymlthat checks out unreviewed contributor code, and itinherited
pull-requests: writefrom the workflow — the comment back isUpload-Log's job. A job-level block replaces the inherited set with whatblossom-actiondemonstrably needs. The remaining jobs keep theworkflow-level set:
blossom-ciis an opaque binary on the self-hostedrunner, and narrowing it further before a run has named what it uses would be
the speculation that file already warns against.
Not applied: CodeRabbit also wanted
github.actor == 'blossom-jenkins-bot'onUpload-Log. That login is not published anywhere, and a wrong guess failsclosed and silent — the run goes green and the Jenkins log link never posts.
NVIDIA/spark-rapids and NVIDIA-BioNeMo/nvMolKit both gate on
github.event_namealone, so the
&& github.event.inputs.argsalready here is the stricter check.Left as a comment naming the condition to pin it under.
Verified: all four files parse; the effective permission set per job is
Vulnerability-scan -> {contents: read, statuses: write}with the other threeinheriting. The credential drop was exercised under
bash -eo pipefailbothwith the header present (removed, verified gone) and absent (step reports and
exits 0, so a caller using
persist-credentials: falsedoes not fail).prekis clean on the changed files.
Back-ports to GitLab once merged.