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

REL: implement SPEC 8 #166

Merged
merged 4 commits into from
Mar 30, 2025
Merged

REL: implement SPEC 8 #166

merged 4 commits into from
Mar 30, 2025

Conversation

lucascolley
Copy link
Member

@lucascolley lucascolley commented Mar 19, 2025

  • RELEASING.md
  • Run triggers
  • "require multiple release team members to approve the workflow" / "enforce additional review by at least one other release team maintainer" probably okay to omit while I am the sole release team member
  • require signed commits SPEC 8 signed commits clarification scientific-python/specs#380
  • protect main branch
  • restrict CI permissions
  • restrict permitted actions doable but needs admin settings access
  • use GHA env
  • pin full hashes for actions
  • attestations
  • trusted publishers

@lucascolley
Copy link
Member Author

Hi @matthewfeickert! I'm taking a look at implementing SPEC 8 for this repo and I've checked off what I think is done in the description of this PR.

Of the unchecked boxes, what do you think about which ones are required/optional?

@matthewfeickert
Copy link

Thanks @lucascolley. I'm in meetings all day today so if I don't get to this before EOD please ping me on any and all channels of communication. 👍

@lucascolley lucascolley added this to the 1.0.0 milestone Mar 19, 2025
@matthewfeickert
Copy link

I'm taking a look at implementing SPEC 8 for this repo and I've checked off what I think is done in the description of this PR.

Of the unchecked boxes, what do you think about which ones are required/optional?

@lucascolley This is great that you're working to adopt SPEC 8, so thank you. 👍 I'll do my best to summarize things, but I'll also tag @tupui, @juanis2112, and @lagru who might have some additional thoughts.

  • "require multiple release team members to approve the workflow" / "enforce additional review by at least one other release team maintainer"

This is more just trying to reinforce the "4 eyes" policy that having at least two people look at something (1 reviewer who isn't the author) is a great way to catch bugs before they enter into the workflow and also ensure that accidental security mistakes that are just from small human error don't have time to become exploits.

  • require signed commits

I again think that this is a solid idea in general, given that anyone can make commits as anyone else on GitHub by just commiting with another person's author information. So if you think that projects could be at risk of people trying impersonation attacks, having known signatures can provide an additional level of verification.

  • protect main branch

This is just a "no brainer" to me. I've definitely thought that I'm working on a branch that I could have sworn I checked out and then just before I run git push -u origin HEAD I realize that I'm on main and not feat/my-new-feature. Ensuring that people don't accidentally kick off workflows that are restricted to main (which could be deployments) is a good idea and very easy to prevent.

If memory serves, I think that @lagru (?) had strong thoughts here, but this is just an additional step to make sure that you're only allowing the workflows you think you're allowing to run. You can imagine that in a high activity week with overworked maintainers that someone could sneak in a small additional GitHub Actions workflow to a change in CI that looks benign but allows for malicious code to run once merged.

To address your question of "required/optional", these are all recommendations, and we aren't going to police you. So they're all "optional" to an extent, but we'd suggest that people consider how a little bit of friction up front can avoid a lot of pain in the long run.

It is also worth mentioning that in the time since SPEC 8 was accepted there has been a lot of work by great teams in this space that have simplified and hardened things even more. So SPEC 8 could probably do with some updating to 2025 recommendations of workflows.

@rgommers
Copy link
Member

  • protect main branch

This is just a "no brainer" to me.

Agreed, I turned on branch protection for main, preventing deletions, force pushes, and requiring a PR rather than direct push.

@rgommers
Copy link
Member

This is quite topical (maybe even what triggered this PR?), and makes sense in principle. I'd add though that if one is worried about this, pre-commit is a larger problem since it already points at a bunch of unpinned random stuff from individual devs that don't look familiar.

@lucascolley
Copy link
Member Author

lucascolley commented Mar 20, 2025

maybe even what triggered this PR?

That's actually just a coincidence (I noticed the SPEC 0 badge and wondered whether there were any other specs that made sense for small projects).

RE points above:

enforce additional review by at least one other release team maintainer

I would be happy with this, but at the minute the 'release team' has just been me.

require signed commits

I again think that this is a solid idea in general

While I've personally started signing commits as of yesterday, I'm concerned that this would be a barrier to entry for new contributors.

if one is worried about this, pre-commit is a larger problem since it already points at a bunch of unpinned random stuff from individual devs that don't look familiar.

It sounds like I'd be happy to follow the recommendations in the SPEC for this, then, but that may not have a big impact if we keep using pre-commit.

@matthewfeickert
Copy link

matthewfeickert commented Mar 20, 2025

I'd add though that if one is worried about this, pre-commit is a larger problem since it already points at a bunch of unpinned random stuff from individual devs that don't look familiar.

Are you running pre-commit in your release process? I assume probably not. If so, why? Note that SPEC-8 is about the release process in particular, though some of these recommendations bleed over a bit.

I would be happy with this, but at the minute the 'release team' has just been me.

Yeah, again these are recommendations in general that you need to adapt to the "on the ground" reality. If you're a single maintainer and no one else works on the project with you at all, then you don't have much choice.

While I've personally started signing commits as of yesterday, I'm concerned that this would be a barrier to entry for new contributors.

These are recommendations for maintainers, not restrictions for all contributors.

@lucascolley
Copy link
Member Author

While I've personally started signing commits as of yesterday, I'm concerned that this would be a barrier to entry for new contributors

These are recommendations for maintainers, not restrictions for all contributors.

Is there a way to require signed commits just from maintainers / just during the release process? I guess create a separate branch for releasing?

@matthewfeickert
Copy link

matthewfeickert commented Mar 20, 2025

Is there a way to require signed commits just from maintainers / just during the release process? I guess create a separate branch for releasing?

Ah excellent question that I don't know off the top of my head, and would be worth looking up. It might not matter, as in the rule protection set the "Require signed commits" options reads

Commits pushed to matching refs must have verified signatures.

here refs are the target branches or tags, so you could create a ruleset that restricts this to to release refs (potentially), which would only be coming from maintainers.

Seems like this should maybe get clarified though in a revision to the SPEC.

@rgommers
Copy link
Member

Are you running pre-commit in your release process? I assume probably not. If so, why? Note that SPEC-8 is about the release process in particular, though some of these recommendations bleed over a bit.

Not in this project, but I'm commenting on the overall security aspect here, we're not reviewing SPEC 8 itself. Also, there are many projects who do indeed run it in CI on the repo from which releases are made - which is part of the release process I'd say (can potentially access repository secrets).

Anyway, whitelisting actions seems perfectly fine, not that cumbersome. Just pointing out there's more to do here.

@lucascolley
Copy link
Member Author

whitelisting actions seems perfectly fine

would you be able to whitelist the currently used actions, Ralf? After that, this should be ready to merge

@lucascolley lucascolley marked this pull request as ready for review March 28, 2025 21:24
@rgommers
Copy link
Member

Done:

image

Observation: pinning to commit hashes helps more than whitelisting probably, since there are a lot of actions here and each of those may depend on yet more actions, pre-commit hooks, and packages from other individuals. The surface area is still quite large. Commit hash pinning insulates from that pretty well, as long as the pins aren't updated aggressively, since no one actually reviews such updates by checking what happened between the old and new commit.

@lucascolley lucascolley merged commit c4a3df6 into data-apis:main Mar 30, 2025
11 checks passed
@lucascolley
Copy link
Member Author

thanks Ralf and all!

@lucascolley lucascolley deleted the spec-8 branch March 30, 2025 10:26
@lucascolley
Copy link
Member Author

It looks like there is a problem:

[Error](https://github.com/data-apis/array-api-extra/actions/runs/14155178857/workflow)
pre-commit/action@2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd, prefix-dev/setup-pixi@92815284c57faa15cd896c4d5cfb2d59f32dc43d, and codecov/codecov-action@0565863a31f2c772f9f0395002a31e3f06189574 are not allowed to be used in data-apis/array-api-extra. Actions in this workflow must be: within a repository owned by data-apis, created by GitHub, or matching the following: JamesIves/github-pages-deploy-action, codecov/codecov-action, dawidd6/action-download-artifact, dependabot/fetch-metadata, hynek/build-and-inspect-python-package, pre-commit/action, prefix-dev/setup-pixi, pypa/gh-action-pypi-publish.

@matthewfeickert have you implemented this elsewhere? Do exact commit hashes need to be whitelisted in the admin settings?!

@lucascolley
Copy link
Member Author

image

@rgommers it looks like we may instead want e.g. codecov/codecov-action@* whitelisted

@rgommers
Copy link
Member

updated, I hope that fixes it

NeilGirdhar pushed a commit to NeilGirdhar/array-api-extra that referenced this pull request Apr 2, 2025
* CI: use hashes for actions

* CI: restrict permissions

* DOC: add SPEC 8 badge

* lint
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.

3 participants