Repository navigation
docs: reduce permission scope - #2261
Merged
Merged
Conversation
Only the `deploy` task needs the "pages/id-token:write" permissions. Let's try to limit the potential for badness by only giving that specific job step elevated permission. This was flagged[0] by running Zizmor on the `.github/workflows` dir with Zizmor 0.7.0 in offline mode and seems like a sensible idea. [0]: https://woodruffw.github.io/zizmor/audits/#remediation_2
djc
approved these changes
Dec 6, 2024
Benchmark resultsInstruction countsSignificant differencesThere are no significant instruction count differences Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2261 +/- ##
=======================================
Coverage 94.64% 94.64%
=======================================
Files 102 102
Lines 23974 23974
=======================================
Hits 22690 22690
Misses 1284 1284 ☔ View full report in Codecov by Sentry. |
ctz
approved these changes
Dec 6, 2024
ctz
left a comment
ctz
left a comment
Member
There was a problem hiding this comment.
Thanks for doing this!
My thoughts:: I'd prefer to concentrate on minimising any and all privileges for GHA (like this PR does), rather than try and affix on third party steps. I don't think it's feasible to review those, so we should treat them as untrustworthy and pre-compromised.
(This is also why I don't personally care for release-plz and similar, giving GHA rights to publish your crate feels like the first step on a very painful journey.)
Member
Author
That aligns with my views. I appreciate that our CI is "boring". I won't bother with any follow-ups to switch to SHA pins.
I'm warmer to the idea depending on specific context but for a crate like Rustls it feels like the wrong tradeoff 👍 |
Member
Agreed that reviewing actions changes in any amount of depth is not feasible, so keeping scope minimal seems like the right direction to me.
There will be first-class support for trusted publishing soonish so that might be a more interesting avenue to explore. |
I think this specific error code is because |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Only the
deploytask needs the "pages/id-token:write" permissions. Let's try to limit the potential for badness by only giving that specific job step elevated permission instead of doing it at the workflow level.This was flagged by running Zizmor on the
.github/workflowsdir with Zizmor 0.7.0 in offline mode and seems like a sensible idea.If you run this tool in online mode w/ a
GH_TOKENit also flags several ref-confusion findings for our usage of install-action. I'm not 100% sure why it didn't flag the other instances where we specify dependent actions by tag (e.g.checkout@v4), but the fix in either case is to pin our actions by SHA hash. Since that's a bit more invasive I haven't made that change yet pending some discussion on whether it's something folks are on-board for.