-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CI: Add GitHub artifact attestations to package distribution #7427
CI: Add GitHub artifact attestations to package distribution #7427
Conversation
- name: Verify the distribution | ||
run: pipx run twine check --strict dist/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beyond this workflow, the only area twine
is used is
scikit-image/tools/upload_wheels.sh
Lines 3 to 5 in 765e568
if [ "`which twine`" == "" ]; then | |
echo "twine not on path; need to pip install twine?" | |
exit 1 |
and tools/upload_wheels.sh
seems to no longer be used anywhere in this repository
$ git grep "twine"
.github/workflows/wheel_tests_and_release.yml: run: pipx run twine check --strict dist/*
tools/upload_wheels.sh:if [ "`which twine`" == "" ]; then
tools/upload_wheels.sh: echo "twine not on path; need to pip install twine?"
$ git grep "upload_wheels"
so instead of installing twine
into the global environment can just use the pre-installed instance of pipx
on the runner to get it (in a virtual environment) for a distribution metadata check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not pin the version of twine, to avoid the threat of a hacked version of twine modifying uploaded wheel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version of twine
doesn't do anything with the upload. All of that is handled through the pypa/gh-action-pypi-publish
GitHub action below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are also concerned with twine
getting compromised and then modifying the wheel during the twine check
command then you could pin to a trusted version, though I don't necessarily know if that's needed. If you were concerned you could also do a checksum before and after twine check
ran to verify that it hasn't changed the distributions at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably being paranoid; we install a lot of tools that can be compromised. Somehow, in my head, that one seemed like a good target because it does uploading. Feel free to do the checksumming, but not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not wrong, so we could do this: #7427 (comment)
(Posting here as I messed up the view on the review tab)
- name: Generate artifact attestation for sdist and wheels | ||
uses: actions/attest-build-provenance@173725a1209d09b31f9d30a3890cf2757ebbff0d # v1.1.2 | ||
with: | ||
subject-path: "dist/scikit_image-*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.f. https://github.com/actions/attest-build-provenance for more information.
Once this runs during a release the attestations will be uploaded to https://github.com/scikit-image/scikit-image/attestations and can be verified from a wheel or sdist artifact using the gh attestation verify
CLI API. c.f. scikit-hep/pyhf#2473 for examples of that.
Given the maintenance history of this workflow (https://github.com/scikit-image/scikit-image/commits/main/.github/workflows/wheel_tests_and_release.yml) tagging @jarrodmillman for review. |
That first document is clear as mud, unless you know all the acronymns, so here is the tl;dr from it's summary:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @matthewfeickert!
- name: Verify the distribution | ||
run: pipx run twine check --strict dist/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not pin the version of twine, to avoid the threat of a hacked version of twine modifying uploaded wheel?
* Add generation of GitHub artifact attestations to built sdist and wheels before upload to PyPI. c.f.: - https://github.blog/2024-05-02-introducing-artifact-attestations-now-in-public-beta/ - https://docs.github.com/en/actions/security-guides/using-artifact-attestations-to-establish-provenance-for-builds
20eb44f
to
36e5753
Compare
4dbbab8
to
9ecc29b
Compare
- name: Verify the distribution | ||
run: pipx run twine check --strict dist/* | ||
|
||
# Ensure that a compromised twine couldn't have altered the distributions | ||
# Required to resolved sdist and whl separately | ||
- name: Verify sdist artifact attestation | ||
env: | ||
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
run: gh attestation verify dist/scikit_image-*.tar.gz --repo ${{ github.repository }} | ||
|
||
- name: Verify wheel artifact attestation | ||
env: | ||
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
run: gh attestation verify dist/scikit_image-*.whl --repo ${{ github.repository }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of manually doing checksum validation, validate with twine
after the attestations are created, and then use the attestations just created and published to validate that the distributions are unaltered before upload to PyPI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES, that is so cool 😎
* Add verification of artifact attestation before publishing to PyPI using the 'gh attestation verify' CLI API, added in v2.49.0. - c.f. https://github.com/cli/cli/releases/tag/v2.49.0 * If twine became compromised and altered the distributions _after_ they had an attestation created for them then the attestation check would fail.
adf53d5
to
b0b1d92
Compare
pre-commit.ci was failing given a timeout, so repushed to retrigger. |
@@ -22,6 +22,7 @@ jobs: | |||
permissions: | |||
contents: write # for softprops/action-gh-release to create GitHub release | |||
id-token: write # IMPORTANT: this permission is mandatory for trusted publishing | |||
attestations: write # for GitHub attestations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, so they do require write access? 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, they require the following permissions:
permissions:
id-token: write
attestations: write
c.f. https://github.com/actions/attest-build-provenance?tab=readme-ov-file#usage
@lagru @jarrodmillman I think this is RTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewfeickert, thanks for working on this! This seems useful and like an improvement. So I'm approving, but I'd still like to clarify a few things.
Even after reading the linked documentation, I'm still a bit unclear on how this compares to "just signing the artifact" (and some context info) with a key and uploading the signature somewhere to check against...
@@ -48,6 +45,26 @@ jobs: | |||
python -m build --no-isolation --skip-dependency-check --sdist . | |||
ls -la ${{ github.workspace }}/dist | |||
|
|||
- name: Generate artifact attestation for sdist and wheels | |||
uses: actions/attest-build-provenance@173725a1209d09b31f9d30a3890cf2757ebbff0d # v1.1.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to create the attestation as close to artifact generation as possible, e.g. in the job call-workflow-build-wheels
? Ideally, before anything else like the test suite is run on the created wheel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lagru Good question/point. I think that to some degree I would say yes, but if you want to only create attestations for releases and not just every run of the CI then you would either need to totally refactor the
jobs:
call-workflow-build-wheels:
uses: ./.github/workflows/wheels_recipe.yml
workflow to know about release situations (probably not worth it) or just do the slightly less perfect situation of this PR (signing the wheels just after download...the sdist is signed immediatley after build here at least).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I thing we can probably come up with a solution that uses inputs
to enable artifacts in wheels_recipe
only for releases. But let's do that in another PR (I'll open an issue) and merge this for now.
@lagru Sorry, I don't think I fully understand what you're asking, as when I read this it seems like you're asking "Why don't I just become my own security provider?". I don't think that's what you're actually meaning though, so maybe you can clarify and I can leave this overview page for Sigstore (which is the underlying service that GitHub uses/owns): https://www.sigstore.dev/how-it-works edit: To be fully transparent, I am not a security expert, and I can not give a comprehensive and short summary of how all the underlying infrastructure works in a single comment (you'll have to read the docs for that). However, if having a comprehensive executive summary would be useful I can go dig around on the internet and find one for you. |
Thank you @matthewfeickert! |
Description
This PR is related to scientific-python/summit-2024#9.
Checklist
./doc/examples
for new featuresRelease note
For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.