security: update actions for artifact handling only by id
#3648
Conversation
I do not have the ability to add labels to this PR, but I do not think this change needs a changelog entry so this label should be added. |
.github/workflows/publish.yml
Outdated
| @@ -47,7 +48,8 @@ jobs: | |||
| cd dist && echo "hashes=$(sha256sum * | base64 -w0)" >> $GITHUB_OUTPUT | |||
|
|
|||
| - name: "Upload dists" | |||
| uses: actions/upload-artifact@65462800fd760344b1a7b4382951275a0abb4808 # v4.3.3 | |||
| uses: actions/upload-artifact@4.6.2 # immutable release | |||
There was a problem hiding this comment.
Can you please stick to the existing scheme? There may be an argument to switch back to version numbers, but I'd rather do this separately, for all actions, and make sure that the format will be recognized by dependabot.
There was a problem hiding this comment.
@pquentin done! ✅
|
@GrantBirki This didn't work: https://github.com/urllib3/urllib3/actions/runs/16756161327/job/47439256407 Here's what a working run looks like: https://github.com/urllib3/urllib3/actions/runs/16593313383/job/46934035410 |
| with: | ||
| name: "dist" |
There was a problem hiding this comment.
I guess removing this line led to the job failure
There was a problem hiding this comment.
Looking at the logs I can see: Starting download of artifact to: /home/runner/work/urllib3/urllib3/dist/dist (ref) and in the working logs, it looks like this:
Starting download of artifact to: /home/runner/work/urllib3/urllib3/dist
So it appears that there is some sort of double nested dist/dist action going on when not referencing by name 🤔
There was a problem hiding this comment.
Yep that is exactly what is going on! When using a single artifact by name, it resolves the path slightly differently:
So I think the easy fix here would just be to adjust the path used for the artifacts in other steps. I'll open a PR to fix this now.
|
@GrantBirki thanks for this security hardening and the quick fix! |
This pull request updates the
publish.ymlworkflow to explicitly useartifact-idsinstead of thenameinput option when handling artifacts. This is important because artifacts produced by GitHub Actions can be completely overwritten by other workflow runs if they use the samename. To avoid potential TOCTOU issues/vulnerabilities where an artifact might be replaced between upload and download, the newartifact-idsinput allows you to download artifacts by their specific ID rather than by name.In the context of this repo, this is especially important as the SLSA L3 badge is present on this project, but in order to be truly at SLSA L3, you cannot use
namefor artifacts as it could be replaced between thebuildandrelease...stages of this workflow. This means that another workflow could potentially change the artifact being used by thispublish.ymljob and release a malicious package topypi.org.The pull request where I originally implemented this feature in
actions/download-artifactdoes a pretty good job at summarizing why this is important in the context of security in general: actions/download-artifact#401