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.
Add zizmor to pre-commit and fix most findings #127749
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
Add zizmor to pre-commit and fix most findings #127749
Changes from all commits
596c49e
7f5a8ec
6b7f89f
b6115ca
26c4d7d
bd9e472
c33dcba
a246790
e5ba3d3
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.
These labels are only used on PRs, so I'm pretty sure the
issues: write
can be removed. In addition, I think we might not even need thepull-requests: write
since AFAICT we are not writing anything.I don't have enough energy today to also dig into this to make sure that the permissions are not needed, so feel free to merge this as is and we can open a separate issue to investigate and possibly remove these.
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.
Yep, we can check with this separately. This PR isn't adding any extra permissions 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.
There's a
git fetch
in the next step. Wouldn't that still need the credentials in order to work?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 think it needs to be an authenticated git fetch?
It ran okay here: https://github.com/python/cpython/actions/runs/12225521755/job/34099604804
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.
Yep,
persist-credentials
is only needed forgit
ops that need authentication. In the context of public repos and their workflows, in practice that means only ops that mutate the repo, rather than pulling from 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.
That makes sense, but if this is the case, does it also mean that as long as we are just reading from public repos, there are no credentials used so there's nothing to share with the other jobs?
IOW,
persist-credentials: true
would only be a concern when a token is used, either for mutating operations (e.g. pushes) or for accessing private repos, and in this case there's actually no security risk, right?(It might still be better to explicitly add
persist-credentials: false
regardless though -- e.g. in case someone adds a token 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.
Nope, unfortunately the default means that a credential is persisted, even though it isn't necessary. So
persist-credentials: false
actually does something usefully by explicitly removing the credential.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.
Thanks for your reply! I'm still not sure I understand if there is an actual issue with reading (i.e. with no writing/mutating operations) public repos though. I tried digging a bit deeper to understand and this is what I found.
The
actions/checkout
README says that it accepts both atoken
and anssh-key
(which we are not using):The comment for
persist-credentials
says:So my understanding is that if I explicitly set a token/key with
token: '...'
orssh-key: '...'
and usepersist-credentials: true
(either explicitly or implicitly since it's the default), the token/key will be "persisted in the local git config" (which I assume is.git/config
, i.e. it will be written on a file). This will expose the token/key to the other jobs -- which is a security risk.If I don't explicitly specify any token/key (i.e. what we are doing in our workflows),
actions/checkout
will setgithub.token
as default token and persist that. Sincegithub.token
is already available to all jobs, the fact that it is persisted shouldn't be an issue -- unless having its value written on a file poses a new threat that I'm not aware of.If no credentials are passed explicitly by using
token
/ssh-key
(which are not needed while reading public repos, like in our workflows) and assumingpersist-credentials: true
, either:token
orssh-key
are explicitly set);github.token
is still a security issue (maybe because it's written on a file?); 1Footnotes
if this is a problem, maybe setting
token: ''
explicitly will prevent thegithub.token
to be used (and persisted) at all (depending on how the action is implemented) ↩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.
No problem! Thanks for your detailed response as well.
Yep: the risk with this is that it's easy to inadvertently upload/log/otherwise persist that local filesystem credential. This can't happen with the in-memory token (i.e.
github.token
) in the ordinary case.For example, until recently, this would cause a token disclosure via a public artifact:
This post has some interesting/elucidative examples of that: https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/
Yeah, I believe this will result in an empty string being saved on the filesystem for the token. I'm not 100% sure but I think this will cause GitHub API errors in some cases.
(I think
persist-credentials: false
is the intended way to disablegithub.token
.)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.
Thanks for sharing, this is really interesting!
Now that I have all the pieces of the puzzle it finally makes sense:
github.token
token: ...
is specified inactions/checkout
, thengithub.token
is usedpersist-credentials
istrue
(the default), either thegithub.token
or the specifiedtoken
will be written in.git/config
actions/upload-artifact
) an attacker could access it and use itpersist-credentials: false
avoids writing the tokens on files and prevents token leaksThere are a few more caveats here and there, but this is already more than enough to justify the use of
persist-credentials: false
.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.
No problem! Your summary is great and matches my understanding perfectly 🙂