ci: fix zizmor security lints#95
Conversation
ubiratansoares
left a comment
ubiratansoares
left a comment
There was a problem hiding this comment.
Hi @oscarosk . Looks we are in the right track here, added one request and one suggestion to improve the PR a bit.
| zizmor: | ||
| name: Zizmor | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| steps: | ||
| - name: Checkout the source code | ||
| uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | ||
| with: | ||
| persist-credentials: false | ||
|
|
||
| - name: Install zizmor | ||
| run: pip install zizmor | ||
|
|
||
| - name: Run zizmor | ||
| run: zizmor --persona=pedantic .github/workflows/ No newline at end of file |
There was a problem hiding this comment.
Perhaps we can make the most of the official zizmor action here.
In addition, I personally don't think we need to configure the check as pedantic on CI, since they intended to be more informational audits and have no security impact in practice.
|
Thanks for the review @ubiratansoares I'll switch to the official zizmor action and drop the pedantic persona for CI. Will push the update shortly. |
|
Updated! Switched to the official zizmor-action and dropped the pedantic persona as suggested. Thanks for the pointers. Let me know if there's anything else to adjust. |
| - name: Install zizmor | ||
| run: pip install zizmor |
|
Removed the leftover pip install step. |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
|
I have updated the workflow to use the official zizmor action, removed the pedantic configuration and install step and aligned it with the recommended setup. please let me know if anything else should be adjusted. |
| security-events: write | ||
| steps: | ||
| - name: Checkout the source code | ||
| uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 |
There was a problem hiding this comment.
why you are using v4 instead of v6 for this new job?
|
|
||
| - name: Checkout the source code | ||
| uses: actions/checkout@v2 | ||
| uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 |
There was a problem hiding this comment.
why you updated to v4 instead of v6?
|
Thanks for pointing that. I have updated actions/checkout to v6 in both jobs and pushed the change. Please let me know if there are any other adjustments needed. |
|
Thanks! Btw, why you chose v4 initially instead of v6? |
I initially updated it from v2 to v4 while addressing the zizmor findings and didn’t check for a newer major version at that moment. |
ubiratansoares
left a comment
ubiratansoares
left a comment
There was a problem hiding this comment.
@oscarosk thanks for the updates.
I think me and @marcoieni agree that, when adding a CI check to lint with zizmor, we want the job to fail if there zizmor finds (new) issues. Hence, please adjust your PR, in a way that we allow these secrets outside an env for now.
In addition, squash your commits into a single one.
- switch to official zizmor action - opt out of advanced-security for now - allow known secrets-outside-env finding temporarily - update actions/checkout to v6
oscarosk
force-pushed
the
fix-zizmor-security-lints
branch
from
f014824 to
8659a08
Compare
|
I have squashed the commits into a single one as requested and confirmed that all checks are passing. Please let me know if anything else should be adjusted. |
ubiratansoares
left a comment
ubiratansoares
left a comment
There was a problem hiding this comment.
Thanks for this contribution @oscarosk
4 participants
Ran zizmor with the pedantic persona and fixed 6 out of 8 findings:
2 secrets-outside-env warnings remain because the deploy step passes AWS credentials directly to the upload-docker-image action as inputs. This cant be changed without modifying the upstream action.
Before: 8 findings (2 high, 5 medium, 1 low), After: 2 findings (2 medium)