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

feat: Add SPEC 8 - Securing the Release Process #325

Merged
merged 18 commits into from
Jul 29, 2024

Conversation

matthewfeickert
Copy link
Member

This SPEC draft was written at the 2024 Scientific Python Developer Summit (scientific-python/summit-2024#9).

This SPEC outlines the process of adopting security tools and standards to securely publish release artifacts that have already been built (securely building release artifacts will be addressed in a future SPEC). There is a plan to also add a publishing topical guide to the Scientific Python Developer guides to provide more context, nuance, and details following the adoption of this SPEC.

We're also very happy to have had guidance from @sethmlarson, the PSF Security Developer in Residence on this SPEC. 🚀 Seth had multiple excellent suggestions that aren't in the draft (as of now) given that this SPEC is meant to be short and focus on the biggest impacts, but all of his recommendations should be present in the planned topical guide.

Comments and questions are very much welcomed.

@matthewfeickert matthewfeickert changed the title feat: Add SPEC 8 feat: Add SPEC 8 - Securing the Release Process Jun 5, 2024
@tupui tupui added the New SPEC label Jun 5, 2024
@matthewfeickert matthewfeickert force-pushed the feat/add-spec-08-draft branch from 5be58c9 to 1e30555 Compare June 5, 2024 19:04
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
@betatim
Copy link
Contributor

betatim commented Jun 5, 2024

I left a few comments, mostly for my education.

I think this is a great start. Maybe it is worth expanding on the words a bit. Some of the topics I am more familiar with (ok with little words) and some I am less familiar with and there it would be nice to have a few more words describing things and splleing out in more detail what shold be done. Yes, it would repeat information that is already there, but I think for people who aren't already familiar with the topic saying the same thing in two different ways is useful.

@tupui
Copy link
Member

tupui commented Jun 5, 2024

I left a few comments, mostly for my education.

I think this is a great start. Maybe it is worth expanding on the words a bit. Some of the topics I am more familiar with (ok with little words) and some I am less familiar with and there it would be nice to have a few more words describing things and splleing out in more detail what shold be done. Yes, it would repeat information that is already there, but I think for people who aren't already familiar with the topic saying the same thing in two different ways is useful.

Can you point out to specific things which we should focus on? We are in our echo chamber now so it might be more tricky for us to properly expand on some topics 😅

Also our goal after this is to make a proper guide. So there would be this short document with the list of recommendations and then a how-to with extensive descriptions on all the options to click etc. (for GitHub+PyPI at least.)

@lagru
Copy link
Member

lagru commented Jun 5, 2024

I concur that this document should stay reasonably concise and link to more detailed resources. I think the ideal goal is to give a quick (<5 min) overview about stuff you can do / should be aware of.

Copy link
Contributor

@betatim betatim left a comment

Choose a reason for hiding this comment

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

I added some comments on points where I think it would be useful to add a sentence or two to help people who are new to the topic understand what they are reading.

spec-0008/index.md Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
@tupui
Copy link
Member

tupui commented Jun 24, 2024

Hey folks, should we move forward? What is left from your side? I think it's a good first version to circulate and start getting feedback from the wider community.

@matthewfeickert
Copy link
Member Author

Hey folks, should we move forward? What is left from your side?

I think just some text revisions given people's comments so far and then to get some feedback from Seth on one point.

I think it's a good first version to circulate and start getting feedback from the wider community.

Agreed. I can try to make time to implement these on Tuesday/Wednesday. I've been overrun with work travel and paper reviews for the last 2 weeks straight, so I've been very unresponsive in general.

Copy link

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for putting this together.

@tupui
Copy link
Member

tupui commented Jun 28, 2024

No worries @matthewfeickert I just want to make sure we don't let this one slip.

Thanks @sethmlarson for the review!

@hugovk thanks for the links. If people really want to I won't block adding a few links.


My view:

I personally think it's not that helpful because all projects have their own setup and ways of doing things. There is nothing special about describing what you do during the release. It's just about being transparent. Also very easy to write. Eg make a release, note what you do. Next time ask someone else to strictly follow the document and see how it goes.

For the tricky part, I don't want to have to recommend something without reviewing carefully. And these can change and need another review to keep recommending it, etc. There are so many things to consider here. When we put links in such documents people are going to blindly copy paste and we certainly don't want that.

@betatim
Copy link
Contributor

betatim commented Jul 1, 2024

I agree you don't want people to blindly copy things. You can probably also make the case that people who would blindly copy do this because they have no idea of their own, so giving them something to follow or imitate will improve the situation.

In the end y'all are the authors so you need to decide what you want to include or not.

spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
### Hardening workflow environment permissions

Workflows that publish release artifacts should have _run triggers_ that require intentional actions by maintainers (e.g., `workflow_dispatch` in GitHub Actions) and require multiple maintainers to approve the workflow to run (c.f. "Use GitHub Actions environments" section below).
This is to safeguard the project from any one maintainer having the ability to commit to the default branch and make a release directly.
Copy link
Member

Choose a reason for hiding this comment

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

Confused; wouldn't most projects want the release manager have this ability? Also, instead of using technical jargon, perhaps the principle can be described in more generic terms first. E.g., if you want to highlight that one maintainer should not be able to make a release by themselves, then that sentence can go first.

Copy link
Member

Choose a reason for hiding this comment

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

That's the thing, you don't need a release manager (from the artifact's standpoint) if the whole process is automated. What you need is multiple maintainers to approve the release flow to run.

Copy link
Member Author

Choose a reason for hiding this comment

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

@stefanv (Asking really because I don't know) In your experience is the expectation that a release manager is the only person who should be involved in a release (and that they don't coordinate with the other project maintainers)?

I agree that a release manager is going to be the one clicking the button, but the recommendation is to make sure that multiple maintainers are on the same page that a release should be cut and that it is happening from the correct branch. You could imagine that the release manager is actually the person that is the second approver here.

Copy link
Member

Choose a reason for hiding this comment

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

I practice what ensures that there is enough maintainers around to not just blindly push the button? E.g. in practice bigger projects are struggling to have multiple release managers, so if cutting a release requires coordinated effort of multiple individuals, that may not make things better or safer (I already see cases where PR approvals are given without a careful review, or even PR related CI failures)

Copy link
Contributor

Choose a reason for hiding this comment

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

The staging area is useful and nice, but the "any couple of maintainers can push the button" is certainly not universal. Thinking about how that would work for NumPy or SciPy: it'd be both more cumbersome and significantly less secure. We now have 5 people with access to PyPI, and 30-40 people with commit rights, and the bar to gain PyPI access is way higher than that to gain commit rights.

I think that that's true for a majority of projects. Giving out commit rights to promising new contributors fairly quickly can be good to engage them; having those be the keys to PyPI as well would make the bar to giving out commit rights higher, or the setup less secure. (requiring 2 maintainers to hit the button only helps a little, since it's not hard for a bad actor to start contributing under 2 different github handles).

Copy link
Member

Choose a reason for hiding this comment

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

And as someone being a release manager for many years for a large package, I would definitely argue that it is not at all easy to get careful review/sign-off from multiple people even when the release cycle is on the 6 month timescale.
Checking on the release notes is not at all equals with checking off on everything required for a release, and if that is not required for ticking that box, then I would argue it's not at all a safer process.

Copy link
Member

Choose a reason for hiding this comment

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

It's safer as in, now to make a release, we need to compromise 2 people if we want to inject something at the last minute. But sure, if the people you would allow to release would not even bother looking at the last commits... then it won't do much.

I still think that a release should be considered as more critical than a PR, hence require a more careful review.

Copy link
Member

Choose a reason for hiding this comment

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

At minimum, I would like to see changing all usage of maintainers in this document to be release team, as that really throws the discussion off track. As my experience just doesn't overlap on how difficult it is to get a release team together and even just alternating releases within them, let alone requiring multiple of them doing all the checks and work for all the releases. I agree it would be nice, but I don't think it's realistic for any of the projects I know, neither big nor small.

Copy link
Member Author

Choose a reason for hiding this comment

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

After talking with @bsipocz IRL I've added "release team" as an adjective to member/maintainer to make it clear who we are talking about.

Also there was some confusion about the number of people: To make it explicit, we are saying 1 additional person review (so 2 people total -> "4 eyes").

👍

Copy link
Member

Choose a reason for hiding this comment

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

Yeap, not using the word maintainer but a much smaller group as the "release team" addresses my main concern. Also 4 vs 2 is now clear, maybe due to having more coffee or having more discussions about it...

spec-0008/index.md Outdated Show resolved Hide resolved
print-hash: true
```

## Notes
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this is out of scope for the SPEC, but how does it look from the user's perspective? Can they use the attestation to check anything? Or do they just pip/conda install and hope for the best. Does conda forge follow these practices? Any recommendations we need to make w.r.t. their release process?

Copy link
Member

@tupui tupui Jul 13, 2024

Choose a reason for hiding this comment

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

End users can check the attestations and check the whole flow yes. It would be awesome if there could be an integration in pip checking things like that (like a --secure flag). @sethmlarson do you know if that was ever brought up?
Slightly out of scope I would say, but definitely should be part of the page we would put on the learn website.

For conda I am not sure. They don't even support (yet) basics like 2FA (just some precision: anaconda accounts, diff than conda-forge), so they have a long way ahead still...

Copy link
Member

Choose a reason for hiding this comment

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

Though thinking a more about it. It should not be hard to add SLSA to the workflows and the good thing with conda-forge is that releases need a PR. So there is a clear lineage vs pip not enforcing that.

A recommendation could be to start the conda workflow by checking the SLSA attestation (if from pip, otherwise we need to check for signed commits or something else to attest the source authenticity), building for conda, then signing everything with a new SLSA.

Copy link
Member

@tupui tupui Jul 13, 2024

Choose a reason for hiding this comment

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

To verify, here is the post from @sethmlarson https://sethmlarson.dev/python-and-slsa#verifying-provenance-of-a-python-package. This is the sort of thing that I would like to see directly built into pip. This way we could ask to get some warnings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps this is out of scope for the SPEC, but how does it look from the user's perspective? Can they use the attestation to check anything? Or do they just pip/conda install and hope for the best.

I think this is out of scope to address in the SPEC itself, but it is a good question, @stefanv. I agree with everything @tupui already said but to give some an explicit example (using the gh command line too, sorry) if they download the sdist or wheel they can verify the artifact's attestation:

$ python -m pip download --no-deps scikit-image
Collecting scikit-image
  Downloading scikit_image-0.24.0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl.metadata (14 kB)
Downloading scikit_image-0.24.0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (14.9 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 14.9/14.9 MB 720.6 kB/s eta 0:00:00
Saved ./scikit_image-0.24.0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
Successfully downloaded scikit-image
$ gh attestation verify scikit_image-*.whl --repo scikit-image/scikit-image
Loaded digest sha256:fa27b3a0dbad807b966b8db2d78da734cb812ca4787f7fbb143764800ce2fa9c for file://scikit_image-0.24.0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
Loaded 1 attestation from GitHub API
✓ Verification succeeded!

sha256:fa27b3a0dbad807b966b8db2d78da734cb812ca4787f7fbb143764800ce2fa9c was attested by:
REPO                       PREDICATE_TYPE                  WORKFLOW                                                       
scikit-image/scikit-image  https://slsa.dev/provenance/v1  .github/workflows/wheel_tests_and_release.yml@refs/tags/v0.24.0

This is obviously not something that is nicely integrated into a security install at the moment, but I also wouldn't expect that yet as that's extra work from the pip team and attestations are somewhat new. (Maybe worth a feature request Issue to the uv team though. 🤔)

Does conda forge follow these practices? Any recommendations we need to make w.r.t. their release process?

@jakirkham has an Issue open on this somewhere (I forget where, but know that I'm tagged in it) but I think there was hesitancy on the part of Conda-forge to use a tool from GitHub and not use Sigstore directly(?). @jakirkham can link us and also give more explicit context on the conda-forge maintainer perspective.

Copy link
Member

Choose a reason for hiding this comment

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

@charliermarsh is this something you thought about maybe for uv?

The quick TLDR, adding SLSA verification when installing packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is probably hard given that to verify current attestations you need to provide information like the repository that created the attestation. Not sure how to do this programmatically during an install.

spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
@matthewfeickert
Copy link
Member Author

matthewfeickert commented Jul 14, 2024

Thank you very much everyone for your comments and review! I have finally gotten around to addressing @betatim and @hugovk's comments at the SciPy 2024 sprints, and @juanis2112 also addressed @stefanv comments (though @stefanv also had some comments/questions that I tried to address in addition to @tupui's responses).

I think this is ready for a final review and then it can get merged! Please give us some additional 👀 (I will do my best to me far more responsive, though I am about to go run a physics conference for a week 😬).

@matthewfeickert
Copy link
Member Author

I'm also going to rebase this now, so apologies if this is annoying to anyone.

matthewfeickert and others added 5 commits July 14, 2024 09:28
* This SPEC draft was written at the 2024 Scientific Python Developer Summit.
Co-authored-by: Tim Head <betatim@gmail.com>
…eason


This isn't great, but it works for the time being.
Co-authored-by: Matthew Feickert <matthew.feickert@cern.ch>
Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
tupui and others added 5 commits July 14, 2024 09:28
Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
* Format things that are supposed to be variables that are project
  dependent like Shell variables (preceeded with a '$').
* Be more explicit with the actions permissions.
* Add information on using RELEASING.md for making a release.
Co-authored-by: Juanita Gomez <juanitagomezr2112@gmail.com>
@matthewfeickert matthewfeickert force-pushed the feat/add-spec-08-draft branch from 1ed0ed6 to 3484313 Compare July 14, 2024 16:28
Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

I had another full read and this is great 🙌 Thanks everyone, especially @matthewfeickert for pushing this 🚀

I would target a merge next Friday if there is no blocker by then. We should not delay this more IMHO and we can always rephrase things later if there is a need.

@matthewfeickert
Copy link
Member Author

(Not really sure why pre-commit is failing, but I can try to look at this later tonight)

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Minor comments and wordsmithing

spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
### Hardening workflow environment permissions

Workflows that publish release artifacts should have _run triggers_ that require intentional actions by maintainers (e.g., `workflow_dispatch` in GitHub Actions) and require multiple maintainers to approve the workflow to run (c.f. "Use GitHub Actions environments" section below).
This is to safeguard the project from any one maintainer having the ability to commit to the default branch and make a release directly.
Copy link
Member

Choose a reason for hiding this comment

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

I practice what ensures that there is enough maintainers around to not just blindly push the button? E.g. in practice bigger projects are struggling to have multiple release managers, so if cutting a release requires coordinated effort of multiple individuals, that may not make things better or safer (I already see cases where PR approvals are given without a careful review, or even PR related CI failures)

spec-0008/index.md Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
spec-0008/index.md Outdated Show resolved Hide resolved
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

This looks good IMO! Thank you @matthewfeickert for pushing it along and dealing with comments. 🫶 Happy to deal with more minor or major tweaks in follow-up PRs.

Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

OK, now with the cleanup wording this reads as a reasonable suggestions and requirements even without reading through all the linked documents.

spec-0008/index.md Outdated Show resolved Hide resolved
@matthewfeickert
Copy link
Member Author

I've been on holiday hiking in the PNW with no cell phone or internet for the last week ( 🌲 🌊 ⛰️ 🌄 ), so now that I'm back I just wanted to touch base here to see what is remaining to do to move forward (the prettier pre-commit hook failure is a separate issue that we also need to fix in a different PR).

@lagru @juanis2112 @tupui @bsipocz @betatim @stefanv Thoughts on how to move forward or remaining questions? As of the end of the SciPy 2024 sprints ( 🙌 ) we had 4 approvals on this PR.

@tupui
Copy link
Member

tupui commented Jul 29, 2024

I would merge. This is ready and we can do follow ups if needed.. The guide in learn is the next thing to do and while we do that, there might be things we will want to update here.

Copy link
Contributor

@jni jni left a comment

Choose a reason for hiding this comment

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

Thanks! I learned a lot here. 😃

@bsipocz
Copy link
Member

bsipocz commented Jul 29, 2024

remaining questions

My only ones are related to the 🌲 🌊 ⛰️ 🌄 part, so 👍 for merging the PR.

@tupui
Copy link
Member

tupui commented Jul 29, 2024

Alright, let's go 🚀

Thanks again everyone for the awesome work. Let's meet soon for the follow up guide.

@tupui tupui merged commit 41f2a84 into scientific-python:main Jul 29, 2024
1 of 2 checks passed
@matthewfeickert matthewfeickert deleted the feat/add-spec-08-draft branch July 30, 2024 05:52
@matthewfeickert
Copy link
Member Author

Awesome. Thanks everyone!

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

Successfully merging this pull request may close these issues.