trio icon indicating copy to clipboard operation
trio copied to clipboard

Try out PyPI's trusted publishers

Open A5rocks opened this issue 1 year ago • 10 comments

This probably won't work first try, but I think this is a good improvement! Since I used to think this was neutral or outright bad, here's a few reasons:

  1. supply-chain security is topical again
  2. it's significantly easier on myself! When I release, I need to make a new venv, install build, etc. It's not hard, just annoying.
  3. I think only a specific set of people can approve the release? We'll need to make sure that's the case after merging of course.

Also I don't think automating other parts of the release process is worth the effort. Just this.

A5rocks avatar Mar 30 '24 08:03 A5rocks

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.63%. Comparing base (ccd40e1) to head (25383d9).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2980   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files         120      120           
  Lines       17710    17710           
  Branches     3179     3179           
=======================================
  Hits        17645    17645           
  Misses         46       46           
  Partials       19       19           

codecov[bot] avatar Mar 30 '24 08:03 codecov[bot]

Windows 3.8 CI failed: https://github.com/A5rocks/trio/actions/runs/8489525533/job/23259717631#step:4:2288

Not gonna debug here but just keeping track

A5rocks avatar Mar 30 '24 08:03 A5rocks

I have this setup on my own project, it's real neat. Once built and tested the run pauses, you get an email, and can then press the approve button. The security settings are to do with the "environment", in the repo settings you can configure who's allowed to authorise the deploy.

TeamSpen210 avatar Mar 30 '24 08:03 TeamSpen210

Yeah there's a release environment on this repo -- hopefully you can see it? For transparency's sake:

image

A5rocks avatar Mar 30 '24 09:03 A5rocks

Actually I don't have the settings tab at all, that's fine though. The important transparency bit is that everyone can see the code that's being run, nobody's able to fiddle with the archive.

In regards to the CI config, I think it's better to split it up into two jobs - one to make the archive, one to do the upload. You'd use artefact upload/download to communicate between. This way, the sdist gets fully built (and can be inspected) before you give out permission to publish. And it lets you partition the permissions, so minimal code has to execute with access to the PyPI token. See the cibuildwheel setup as an example of that way.

TeamSpen210 avatar Mar 30 '24 09:03 TeamSpen210

Alright, I separated out the build step. We'll need to test this a bit but hopefully it's fine -- after all most is copied from that example!

I'm not sure how much more secure this is, but might as well do it while I'm here. (After all the only dependencies we ever even touch should be build and setuptools and if those are compromised we have bigger issues that "is someone going to exploit our OIDC thing")

A5rocks avatar Apr 02 '24 08:04 A5rocks

Indeed it's probably not too critical, but it would protect against us adding something else in the future. We could also say add a test step in between to verify they install + import. Probably more relevant on a non-pure Python project though.

TeamSpen210 avatar Apr 02 '24 23:04 TeamSpen210

Re: your PR about testing the sdist/wheel, I have a few concerns and I didn't want to think too hard about it yet:

  1. the guides I saw all used a release.yml file, presumably for extra security:tm:. How does inter-file dependencies for gha work?
  2. I'm not sure if push events encompass tag pushing...

I had some other stuff but I forgot. And now that I think about it I think we could use that workflow_call event + move artifact creation to ci.yml. Nonetheless that can be done later.


And I was the one to bring up threat models for building dependencies cause I don't quite see the point of separating out building (for trio, atm, specifically); I don't see why not though so might as well do it if others think it's good!


Your PR feedback makes sense, I'll do that.

A5rocks avatar Apr 14 '24 00:04 A5rocks

Re: your PR about testing the sdist/wheel, I have a few concerns and I didn't want to think too hard about it yet:

  1. the guides I saw all used a release.yml file, presumably for extra security:tm:. How does inter-file dependencies for gha work?

There are a few ways this works. First of all, you can "chain" workflows so one runs after another. In this case, you cannot share state/data between those. Second, you can "include" the reusable workflows — this is very useful for modularization but PyPI doesn't support reusable workflows currently. So I usually recommend doing this with other parts of the CI except for the PyPI publish job.

I found that a nice way to apply this in practice is having a "top-level" workflow that lists the event triggers and dependencies between different processes but the processes themselves are declared in reusable workflows. You can pass inputs into such workflows and they can set outputs. These "modules" don't list normal events as triggers. The PyPI publish job is the only one fully declared on the top level fully.

  1. I'm not sure if push events encompass tag pushing...

I had some other stuff but I forgot. And now that I think about it I think we could use that workflow_call event + move artifact creation to ci.yml. Nonetheless that can be done later.

The push event happens whenever a tag or a branch is pushed (or force-pushed). This requires filtering "in the runtime". When there's no tag existing at the time of the push, a create event also fires (if you delete a tag and re-push it'll happen again too). However, personally, I like triggering the release automation via a workflow_dispatch event that allows typing in the target version in the GH UI. Though, this is more complex and I don't suggest extending the scope of this PR.

And I was the one to bring up threat models for building dependencies cause I don't quite see the point of separating out building (for trio, atm, specifically); I don't see why not though so might as well do it if others think it's good!

Yeah, but the case I describe targets things beyond the Python ecosystem and is therefore more dangerous. Another reason is that you need to have the publishing part protected and conditional. But it's a good idea to have a smoke test building and testing the artifacts in PRs and on pushes. You can also check the metadata with twine check --strict so that you don't hit upload failures too late in the process.

webknjaz avatar Apr 14 '24 21:04 webknjaz

Sorry about the delay, that should be all the changes. I meant to get to this earlier (just like making a release earlier) but, oh well.

This doesn't have any provenance stuff yet, I would rather push those off for now. To make checking through my comments I left faster: the only relevant thing is that I don't know how to rename the already created release environment to be pypi so that's staying the same.

A5rocks avatar May 16 '24 07:05 A5rocks

Hi @webknjaz, just a bump. I'll merge this in a few days, unless there's something obviously wrong.

A5rocks avatar Aug 02 '24 22:08 A5rocks

Feel free to leave a review for me to fix in a followup PR.

A5rocks avatar Aug 05 '24 00:08 A5rocks