-
Notifications
You must be signed in to change notification settings - Fork 37
[ci] enforce zizmor checks, drop codecov #346
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
Merged
Merged
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
9a3eb61
[ci] enforce zizmor checks, drop codecov
jameslamb 8be41c9
add pre-commit job
jameslamb 3d23a68
default to contents:read
jameslamb 7b9ecdb
pin zizmor to SHA, document in CONTRIBUTING.md
jameslamb 5b77622
Add link and instructions for installing pre-commit
jayqi e003d62
Remove accidental duplication
jayqi 2f9df4e
Apply suggestion from @jameslamb
jameslamb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| repos: | ||
| - repo: https://github.com/zizmorcore/zizmor-pre-commit | ||
| rev: a4727cbbcd26d7098e96b9cb738169b59711ae51 # v1.24.1 | ||
| hooks: | ||
| - id: zizmor | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
Are we asking developers to install pre-commit so that they run zizmor on pre-commit? Otherwise, wouldn't it be better to just install and run the zizmor CLI, or to use zizmorcore/zizmore-action? This is adding pre-commit as a dependency in the chain.
Also the pre-commit package being installed here also isn't pinned to a commit hash.
Uh oh!
There was an error while loading. Please reload this page.
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 convenient to be able to run
zizmorlocally so you don't have to iterate with pushes + CI runs, but definitely not required. If you're not touching the GitHub Actions configs (which would be the case for any outside contributor, I'm guessing, as they'd probably only touch R code), then you don't need to care about it.I chose a general task runner like
pre-commitfor that instead of, say, a shell script running thezizmorCLI because I could imagine wanting more tools like this in the future. In{uptasticsearch}Austin and I usepre-committo also runcodespell(typos),shellcheck(shell code correctness/portability), andshfmt(shell code autoformatting): https://github.com/uptake/uptasticsearch/blob/main/.pre-commit-config.yamlIt's convenient to have those things bundled together, and
pre-committakes care of things like installing the tools on different platforms.If you'd prefer not to have this here I'm fine to switch to the
zizmorGitHub action, would you like me to do that?The
pre-commitGitHub Actions I've introduced here is:I'm unsure how tightly that controls the
pre-commitPython package and its dependencies or how to pin those too.Or did you mean by this comment that you'd want the
zizmor-pre-commithere to be pinned to a commit instead ofrev: 'v1.23.1'?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 don't have a super strong position on whether we use the tools directly or if we use pre-commit. If we do want to use pre-commit, it probably makes sense to document that in
CONTRIBUTING.mdso that it's more clear and standardized how to use it and what the expectations are? I'm okay with treating that as a separate PR though, but just want to point out that adopting pre-commit would make the most sense if we're intentionally using it as designed.Yes, I think we should pin
zizmor-pre-commitin the pre-commit manifest here if we're generally looking to harden against supply chain attacks where GitHub releases are modified.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.
Ok, I've just pushed a commit pinning the
pre-commithook to a SHA and documenting how to use it inCONTRIBUTING.md.7b9ecdb
Note that with that change, it'll now need to be manually updated to pull in new versions...
pre-commit autoupdatereplaces the commit SHA with a tag likev.1.24.1.I haven't pinned the
zizmorPython package to a SHA (it'd be a checksum of the wheel contents, not a git commit) because that package publishes different wheels for different architectures:https://pypi.org/project/zizmor/#files
It's
==pinned to a version in the hook: https://github.com/zizmorcore/zizmor-pre-commit/blob/a4727cbbcd26d7098e96b9cb738169b59711ae51/pyproject.toml#L6I suspect it's possible to do with platform markers but would be pretty annoying to update. And I REALLY think that'd be overkill,
zizmoris probably better hardened against supply chain attacks than 99.9% of other packages on PyPI.If you want to make any changes to the
CONTRIBUTING.mdor other details here, totally fine with me if you just want to push them directly to the branch. I'm interested in reducing our attack surface here and usingzizmorto ensure we maintain those improvements, and don't have strongly-held opinions about how that's accomplished.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 it's fine for PyPI packages to be pinned to versions and not SHA hashes. I'm pretty sure PyPI packages are not mutable. Once you've published a version, you can't publish a new distribution over that version number. The main thing is that anything that installs directly from a GitHub repository needs to be hardened, because GitHub does not do anything like that.
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.
Ok thanks, yes I agree we can feel confident about not pinning the
zizmorPython package any further.Everything else I'm about to write is just for your interest, and shouldn't affect this PR at all.
There are a couple ways this is not technically true:
==1.24.1would also match the version1.24.1.0, which could be uploaded laterzizmor 1.24.1and then much later upload a new wheel with that same versionIt's for reasons like this that pinning to a SHA that's a checksum of file contents is safer than pinning to version numbers.
There's more discussion about this here:
But still, anyway, the current state of this PR is more than enough for our purposes here and a net improvement in this repo's security posture.