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

workflows: address zizmor findings #1397

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

woodruffw
Copy link
Member

Summary

Addresses a handful of findings identified by zizmor: https://github.com/woodruffw/zizmor

cc @jku

Release Note

No functional changes.

Documentation

No functional changes.

Identified with zizmor: https://github.com/woodruffw/zizmor

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Member Author

woodruffw commented Nov 12, 2024

NB: I set persist-credentials: false everywhere, since I think none of the workflows need to run git push. But if I missed something, some of those may need to be changed to persist-credentials: true.

Edit: NB 2: I also didn't add a zizmor.yml or similar workflow to run zizmor continuously. If that's of interest, I'd be happy to add one similar to the ones in Warehouse and Homebrew's CI/CD setups.

Copy link
Member

@kommendorkapten kommendorkapten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, nice work.

Copy link
Member

@kommendorkapten kommendorkapten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was too fast with the approval :)


env:
METADATA_URL: ${{ inputs.metadata_url }}
IDENTITY: ${{ github.server_url }}/${{ github.repository }}/.github/workflows/custom-test.yml@${{ github.ref }}

jobs:
sigstore-python:
permissions:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would need to be replicated to all jobs in this file, as they all will request an id-token to use the ephemeral key flow to sign.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, thanks for catching that! I missed that there were other individual jobs that did signing. Will fix now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done -- FWICT only two other jobs do signing, so I added it to each.

@kommendorkapten
Copy link
Member

kommendorkapten commented Nov 13, 2024

I will carry over all findings to root-signing-staging too, so we can first test them there (just to be sure).

edit: spelling error

Signed-off-by: William Woodruff <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants