Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add SPEC 8 - Securing the Release Process #325
Changes from 11 commits
95bf50c
be7c750
eaa1ae4
ce242c1
2addd03
9c22f17
afe1f7d
214438c
1be873f
a782ef2
3484313
db51e0d
4816dbf
8d492bd
18e8c82
5ddb16e
f492de4
01bacf6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
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.
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.
@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.
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 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)
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.
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).
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.
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.
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.
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.
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.
At minimum, I would like to see changing all usage of
maintainers
in this document to berelease 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.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.
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").
👍
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.
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...
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.
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?
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.
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...
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.
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.
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.
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.
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 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 thesdist
orwheel
they can verify the artifact's attestation: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 theuv
team though. 🤔)@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.
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.
@charliermarsh is this something you thought about maybe for uv?
The quick TLDR, adding SLSA verification when installing packages.
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 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.