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

Expose PEP 740 attestations functionality #236

Merged
merged 37 commits into from
Sep 1, 2024

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented May 2, 2024

WIP, still experimenting here. Not ready for review 🙂

This adds PEP 740 attestation generation to the workflow: when the Trusted Publishing flow is used, this will generate a publish attestation for each distribution being uploaded. These generated attestations are then fed into twine, which newly supports them via --attestations.

Breakdown:

  • Generate attestations for each dist
  • Actually hook up twine --attestations
  • Documentation

See: pypi/warehouse#15871

@woodruffw
Copy link
Member Author

I've confirmed that the basic version of this works as expected (attestations is currently ignored by PyPI and TestPyPI): https://github.com/woodruffw-experiments/gha-pep740-experiments/actions/runs/9008318313/job/24750093954

@webknjaz
Copy link
Member

@woodruffw I just bumped Twine FYI. And pre-commit is fine now. Rebasing should get the blockers out of the way.

@woodruffw
Copy link
Member Author

@woodruffw I just bumped Twine FYI. And pre-commit is fine now. Rebasing should get the blockers out of the way.

Good timing, so did @facutuesca 😅

@facutuesca
Copy link
Contributor

I'm squashing and rebasing this branch now

Signed-off-by: Facundo Tuesca <[email protected]>
@facutuesca
Copy link
Contributor

@woodruffw @webknjaz The remaining lint failure is due to an error message string (_TOKEN_RETRIEVAL_FAILED_MESSAGE) being duplicated in attestations.py and oidc-exchange.py.
Would you prefer de-duplicating it by moving it into a new common.py file, or another approach?

@woodruffw
Copy link
Member Author

Would you prefer de-duplicating it by moving it into a new common.py file, or another approach?

That's what I was thinking originally, although common.py won't be on sys.path so importing it won't be straightforward 🙁

I think the "clean" thing to do here would be to turn the Python files here into a project structure that gets installed as part of the Docker image's build. But that's a little heavyweight, so @webknjaz may have another idea 🙂

@webknjaz
Copy link
Member

I'm leaning towards just being available on $PYTHONPATH, not pip installed. The way I did it @ https://github.com/re-actors/alls-green/blob/223e4bb/action.yml#L50-L53. However, such a restructuring should be separate and would probably include a src/pypi_publish/__main__.py entrypoint invoking the current main script through subprocess as the first step. I don't want a huge PR since big ones tend to cause long reviews and merge delays. Another thing to do would be to start to use our own managed virtualenv in the container rather than installing the deps globally into the shared userspace.

@webknjaz webknjaz added the enhancement New feature or request label May 29, 2024
@webknjaz
Copy link
Member

@woodruffw
Copy link
Member Author

(Sorry, was on PTO -- catching up on mentions now)

@woodruffw hey, it looks like GitHub rolled out their own attestations in beta: https://github.com/actions/attest-build-provenance / https://github.com/pypa/gh-action-pypi-publish/attestations / https://github.com/orgs/community/discussions/122028. I wonder if we could somehow integrate with that...

Yep, I've been thinking about how to make integrate the two -- the last comment on the PEP discussion thread suggests an approach that would allow GitHub-generated attestations to be compatible with this PEP.

TL;DR: I think our options here are:

  1. Move forward with the current "version 1" specified in the PEP. This would mean no further changes are required on the PEP side. However, it would also mean that "version 1" isn't compatible with the default attestation format generated by GitHub, and that we would have to create a "version 2" that is compatible.
  2. Edit the PEP in its current form and make it compatible with GitHub's attestation format. This would be a small change to the PEP overall, but has knock-on consequences (e.g. conceptually unbounded JSON payloads being signed over).

@woodruffw
Copy link
Member Author

woodruffw commented Jun 10, 2024

Just to update here: I've begun updating PEP 740 to be compatible with what GitHub's artifact attestations feature is doing, and it looks like it'll be pretty smooth.

That then leaves the question of how to approach attestation generation here 🙂:

  1. Go forward with what's currently in this PR (a small script that uses the id-token permission + pypi_attestation_models to manually generate the attestation)
  2. Move to a more discrete/black-boxed approach, e.g. something like this:
steps:
  - name: attest
    # does not exist yet!
    uses: pypa/gh-action-pypi-attest
    with:
      # this would be the default; just for illustration
      packages-dir: dist/

  - name: publish
    uses: pypa/gh-action-pypi-publish@release/v1

The benefit to approach (2) is that it's more devolved and can be built up on top of actions/attest@v1. OTOH it requires users to add another workflow step and get the order right, versus doing it implicitly in this action. Thoughts @webknjaz?

Edit: for context, the reason we can't use actions/attest-build-provenance or actions/attest directly is because (1) we have our own custom predicate, and (2) our attestation format requires a transformation step from the output bundle produced by those actions, so we need to wrap them in a "composite" or similar action to get everything into the shape PyPI expects.

@ofek
Copy link

ofek commented Jun 11, 2024

Awesome stuff! I would much prefer option 1, we should prioritize UX.

@sethmlarson
Copy link

I agree with @ofek that ergonomics is the most important part here, requiring users add another action step means we will see delayed rollout of the feature compared to adding it on to the existing workflow.

@woodruffw
Copy link
Member Author

SG, I'll keep going with the current approach then 🙂

@woodruffw woodruffw marked this pull request as ready for review June 11, 2024 21:52
Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Member Author

This is good for an initial review! To summarize:

  1. By default, nothing changes.
  2. If the user sets attestations: true and is in a Trusted Publishing flow, this workflow will now transparently generate a "PyPI Publish" attestation for them, for each distribution being uploaded.
  3. This attestation will be transparently stabled to each distribution, thanks to twine --attestations.

@sethmlarson
Copy link

@woodruffw Just double checking long-term strategy, is the attestations: true setting only there because this is an experimental feature and the goal is that one day a new version of the workflow will automatically do this without the attestations: true setting?

@woodruffw
Copy link
Member Author

Just double checking long-term strategy, is the attestations: true setting only there because this is an experimental feature and the goal is that one day a new version of the workflow will automatically do this without the attestations: true setting?

Yes, exactly -- my thinking is that it's already non-default due to the tail of people using older versions of this action, so having it be (temporarily) opt-in makes experimenting easier while giving us the ability to flip the switch once everything is stable.

@webknjaz
Copy link
Member

Sorry I didn't react earlier. I also think that UX is important. Not sure how well it plays with uploading attestations to GitHub, though.

Can we attempt relying on the attestations: write privilege instead of the action input? Can we feature-detect that it's enabled and use that as a flag? It feels like better ergonomics + potentially better integration with GH IIUC.

Another thought: why do we need to make attestations opt-in even? If it's supported in the Twine version and PyPI side is a no-op (still?), what's the harm in doing it unconditionally whenever we have OIDC privilege? If you're worried about crashes in our own code, we could just stick overly generic exception handling for the experimentation period.

@woodruffw
Copy link
Member Author

Gentle ping for review 🙂 -- we're getting close to enabling attestation persistence on PyPI, so being able to roll this out (with the current toggle) will help us smoke test the functionality there.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I was hoping the refactoring PR would be completed by now, but it looks like that ain't happening... I suppose, this is a good patch to accept. Could you address a few comments below before I merge?

attestations.py Outdated Show resolved Hide resolved
attestations.py Outdated Show resolved Hide resolved
attestations.py Outdated Show resolved Hide resolved
requirements/runtime.in Outdated Show resolved Hide resolved
twine-upload.sh Outdated Show resolved Hide resolved
woodruffw and others added 4 commits August 21, 2024 11:55
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
...to make the linters happy.

Signed-off-by: William Woodruff <[email protected]>
@woodruffw woodruffw requested a review from webknjaz August 21, 2024 16:21
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

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

This LGTM, one thoughts arises about future work: We should eventually support attestations: false if/when this becomes the defualt so users can easily turn this off if they need to.

Comment on lines +78 to +84
# Setting `attestations: true` with an index other than PyPI or TestPyPI
# indicates user confusion, since attestations are not supported on other
# indices presently.
if [[ ! "${INPUT_REPOSITORY_URL}" =~ pypi\.org ]] ; then
echo "${ATTESTATIONS_WRONG_INDEX_WARNING}"
INPUT_ATTESTATIONS="false"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary given pypa/twine#1099?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not necessary, since twine will catch this case and produce an appropriate error. OTOH catching it here might make the error slightly more intelligible, since the twine error doesn't get propagated into a ::warning annotation like this one does.

This is similar to how the action pre-validates the dist directory, even though twine also validates it. But if you think it makes it makes things more confusing than necessary, I can drop this check 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I think it's probably fine for this to catch it before twine and give a more tailored error message, I'm mostly just thinking about the long-term pain of maintaining/updating this list of supported indexes in multiple places, but this is fine for now.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Thanks! Love the attention to detail in your PRs, as always :)

@webknjaz webknjaz merged commit 8a08d61 into pypa:unstable/v1 Sep 1, 2024
2 checks passed
@webknjaz
Copy link
Member

webknjaz commented Sep 1, 2024

v1.10.0 is now live. Appologies that it took so long to merge this.

@woodruffw
Copy link
Member Author

No problem whatsoever! Thank you for your diligent reviews and for seeing this through!

@webknjaz
Copy link
Member

webknjaz commented Sep 2, 2024

UPD: Upon seeing Dependabot sending bumps of this version to repositories, I improved the release notes to include a call to try out attestations with that code example so it's discoverable more easily.

@webknjaz
Copy link
Member

webknjaz commented Sep 3, 2024

@woodruffw looks like the sanity check is broken — it exploded in attrs..

webknjaz added a commit that referenced this pull request Sep 3, 2024
This bug sneaked into #236 but should not affect many people as the
attestations generation feature is experimental and opt-in.

Fixes #256
@webknjaz
Copy link
Member

webknjaz commented Sep 3, 2024

UPD: v1.10.1 fixed that bug via 0ab0b79 and Hugo reported that it works.

attestations.py Outdated
for dist_path in dist_paths:
if not dist_path.is_file():
die(f'Path looks like a distribution but is not a file: {dist_path}')
if (invalid_dists := [_path for _path in dist_paths if _path.is_file()]):
Copy link
Member

Choose a reason for hiding this comment

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

This is where the bug snuck in that needed fixing by 0ab0b79.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦 thanks for fixing that.

github-merge-queue bot pushed a commit to python-attrs/attrs that referenced this pull request Sep 3, 2024
…again (#1345)

Opt into uploading PEP 740 digital publish attestations to PyPI

pypa/gh-action-pypi-publish#236

Co-authored-by: Hynek Schlawack <[email protected]>
@woodruffw woodruffw deleted the ww/attestations branch September 3, 2024 14:23
Pierre-Sassoulas pushed a commit to pytest-dev/pytest-asyncio that referenced this pull request Sep 6, 2024
)

* Build(deps): Bump pypa/gh-action-pypi-publish from 1.9.0 to 1.10.0

Bumps [pypa/gh-action-pypi-publish](https://github.com/pypa/gh-action-pypi-publish) from 1.9.0 to 1.10.0.
- [Release notes](https://github.com/pypa/gh-action-pypi-publish/releases)
- [Commits](pypa/gh-action-pypi-publish@v1.9.0...v1.10.0)

---
updated-dependencies:
- dependency-name: pypa/gh-action-pypi-publish
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Opt into uploading PEP 740 digital publish attestations to PyPI

pypa/gh-action-pypi-publish#236

* Bump `pypa/gh-action-pypi-publish` 1.10.1

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
@webknjaz
Copy link
Member

webknjaz commented Nov 5, 2024

@woodruffw re: GH Attestations API — it seems pretty straightforward and doesn't really need going the composite action route, it seems. See https://github.com/actions/toolkit/blob/77f247b/packages/attest/src/store.ts#L5-L44.

@webknjaz
Copy link
Member

webknjaz commented Nov 5, 2024

xref #288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants