Skip to content
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 #2473

Merged
merged 4 commits into from
May 16, 2024

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented May 3, 2024

Description

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Add generation of GitHub artifact attestations to built sdist and wheel
  before upload.
  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
* 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

@matthewfeickert matthewfeickert added CI CI systems, GitHub Actions need-to-backport tmp label until can be backported to patch release branch labels May 3, 2024
@matthewfeickert matthewfeickert self-assigned this May 3, 2024
@matthewfeickert
Copy link
Member Author

Currently blocked by actions/runner-images#9784, without updating ubuntu-latest to ubuntu-24.04.

Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.21%. Comparing base (ecd0613) to head (1b0191c).
Report is 29 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2473   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files          69       69           
  Lines        4543     4543           
  Branches      804      804           
=======================================
  Hits         4462     4462           
  Misses         48       48           
  Partials       33       33           
Flag Coverage Δ
contrib 97.79% <ø> (ø)
doctest 98.08% <ø> (ø)
unittests-3.10 96.23% <ø> (ø)
unittests-3.11 96.23% <ø> (ø)
unittests-3.12 96.23% <ø> (ø)
unittests-3.8 96.25% <ø> (ø)
unittests-3.9 96.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* Add generation of GitHub artifact attestations to built sdist and wheel
  before upload.
  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
* 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
@matthewfeickert matthewfeickert force-pushed the ci/add-artifact-attestations branch from e33ba6e to 2e1aab3 Compare May 16, 2024 12:25
@matthewfeickert
Copy link
Member Author

GitHub CLI is updated to v2.49.2 in actions/runner-images#9784 and so should work now.

@matthewfeickert
Copy link
Member Author

matthewfeickert commented May 16, 2024

So it seems that the most important takeaway is described in https://github.blog/2024-05-02-introducing-artifact-attestations-now-in-public-beta/#a-more-secure-future, and while it seems that this should mean that the number of attestations generated isn't relevant as everything will be machine generated, it seems that it might(?) be useful to keep https://github.com/scikit-hep/pyhf/attestations containing mostly only releases.

@woodruffw @di, not to bug you with things that go beyond https://github.com/sigstore/sigstore-python, but is this the correct take (that we should only be publishing attestations for releases)?

@matthewfeickert
Copy link
Member Author

Also cc @sethmlarson given very helpful feedback on scientific-python/summit-2024#9 (comment).

@woodruffw
Copy link

Thanks for the ping @matthewfeickert!

IMO, the number of attestations generated is not currently relevant: these attestations don't appear on PyPI and are "generic" in nature (i.e. don't express a specific intent to build, release, etc.), so having multiple of them doesn't accomplish much at the moment.

However, to take a step back: we're building PEP 740 on the same foundations (Sigstore + sigstore-python), and it will have support for multiple attestations, e.g. for distinguishing publish, release, and also third-party counter-attestations. That PEP uses the same underlying technologies as GitHub's Attestations feature but requires attestations to be uploaded to PyPI as well, which means we'll be building it into the standard PyPA tooling.

That's still a little out and not directly related to what you asked though 😅. But the TL;DR: IMO one GitHub attestation is fine, and in the future PEP 740 will also allow your tools to (automatically) load multiple attestations per-file onto PyPI.

@matthewfeickert
Copy link
Member Author

matthewfeickert commented May 16, 2024

we're building PEP 740 on the same foundations (Sigstore + sigstore-python), and it will have support for multiple attestations, e.g. for distinguishing publish, release, and also third-party counter-attestations. That PEP uses the same underlying technologies as GitHub's Attestations feature but requires attestations to be uploaded to PyPI as well, which means we'll be building it into the standard PyPA tooling.

Amazing! Thank you very much for all of this work and this is actually quite useful to know about. 👍 I also now have some reading to do. :) https://peps.python.org/pep-0740/

But the TL;DR: IMO one GitHub attestation is fine, and in the future PEP 740 will also allow your tools to (automatically) load multiple attestations per-file onto PyPI.

Okay thanks this is useful. My question was more in the context of the fact that https://github.com/scikit-hep/pyhf/attestations is a log of all attestations, and if I was to remove the if guards that I place here it would generate attestations for each run of the CI, which just seems like it would be noisy. So while not a problem I'll leave the if guards in and do similar things as I propagate this out across other Scikit-HEP libraries and to Scientific Python libraries. 👍

Comment on lines +34 to +37
permissions:
id-token: write
attestations: write
contents: read
Copy link
Member Author

Choose a reason for hiding this comment

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

@matthewfeickert
Copy link
Member Author

@meeseeksdev backport to release/v0.7.x

meeseeksmachine pushed a commit to meeseeksmachine/pyhf that referenced this pull request May 30, 2024
@matthewfeickert matthewfeickert removed the need-to-backport tmp label until can be backported to patch release branch label May 30, 2024
matthewfeickert added a commit that referenced this pull request May 30, 2024
#2496)

* Backport:
   - PR #2473
   - PR #2478

---------

Co-authored-by: Matthew Feickert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI CI systems, GitHub Actions
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants