Mitigate security risks found using Zizmor

[fatima2003]

Found a few vulnerabilities in the workflow using zizmor.

Fixed 33 warnings by:

• Using actions/checkout with persisted credentials to false where possible.
• Using Github Variables.

Zizmor still returns some warnings:

• Persisted credentials are required for further authentication of Git operations after the checkout step in some workflows.
• Secrets unconditionally inherited by some workflows. I didn't modify those since I'm not sure which secrets are required in the workflow.

References:

• Github default variables
• Preventing script injection by using env var
• Exploring Github Vulnerabilities

[cipherboy]

Some thoughts!

[andreaso]

Looks like a great step in the right direction!

Once this PR has been merged I wouldn't mind collaborating on some additional workflow security improvements.

[fatima2003]

Hi @andreaso, thanks a lot! 😃 I'll prioritize this PR to get it merged sooner. Looking forward to more workflow security improvements!

[andreaso]

@fatima2003: Since you keep adding to this PR, I hope you won't mind me jumping in with a general piece of advice...

Right now it looks like you simply are trying to resolve as many individual zizmor warning as possible by adding more and more input validation steps? And while input validation no doubt is a good thing, maybe also consider taking a step back, and consider some more strategic approaches?

For example, rather than always creating global environment variables by writing to $GITHUB_ENV maybe sometimes it makes more sense to take the Setting an output parameter approach?

Or rather than updating PATH by writing to $GITHUB_ENV there's the simpler approach of simply adding to the PATH by writing to $GITHUB_PATH. Or maybe (I haven't actually checked) there might be cases where one might be able to pass along absolute paths rather than updating PATH?

None of these example fully does away with the dangers of untrusted inputs, but it puts everything in a smaller and more manageable scope.

Finally, how much time are you spending on evaluating to what extent each zizmor warning actually apply to the specific circumstances in these specific workflows? After all, false positives are a thing....

[fatima2003]

Hi @andreaso, thank you for your input! I'm still getting familiar with workflows, so I was referencing other articles and PR's: like packaging.python.org #1748, Exploring Github Vulnerabilities and Preventing script injection by using env var. This is prolly why my approach has been heavily focused on using environment variables 😅 I appreciate your advice and will try setting output parameters and $GITHUB_PATH where it feels appropriate.

The warnings I was focusing on addressing were persisting credentials (by setting persist-credentials: false) and code injection via template expansion (by replacing template substitution with environment variables to ensure proper shell quoting).

Regarding the last point on evaluating whether each Zizmor warning is applicable, I don’t have a specific test for that. My approach was primarily focused on replacing template substitutions in Bash scripts, since I assume the risk of template injection is most significant there, as advised in GitHub's security hardening guide.

Really appreciate the advice and look forward to more feedback as I work on this :D

[andreaso]

Yeah, setting persist-credentials: false (where doable) and getting rid of ${{ expressions }} directly within run: commands are definitely no-brainers.

I should probably clarify that my dislike for setting environment variables is strictly a dislike for setting environment variables that persist through the entire job, like when writing to $GITHUB_ENV. Something I'm on the other hand is very much in favor of is using outputs as a basis for defining environment variables on the step level, since outputs are very clearly scoped and since step environment variables only persist throughout that particular step.

To me setting environment variables that persist through the entire job vs. using outputs for defining environment variables on a step level is similar to the distinction between working with global variables vs with local variables. Yes, having access to global variables can be very convenient, but once your code base passes a certain size it can easily become really hairy to keep track of them all.

Of course, working with environment variables that persist through the entire job has the potential to become even hairier, since there's a whole bunch of them that will affect runtime behavior even if they aren't explicitly referenced.

If we now look at this from a security perspective the term blast radius ought to apply.

Yes, it's very common that documentation examples as well as third party Actions will set environment variables that persist through out a job, and I understand why. Persistent environment variables are familiar, easy to use and they do work fine as long as your workflows are sufficiency simple.

UPDATE: Added a few more words to also cover how outputs fits into the big picture.

[cipherboy]

Few minor nits about shell quoting and then I'll merge!

[fatima2003]

@cipherboy test-go (7) failure seems unrelated and also shows up in 64ce299.

Edit: hmmm it fails for the same reason as the race detection test for 7 mentioned in #1031:

For key: test-key-nil-signing-and-next-signing-key at cycle: 2 expected public keys to be at least of length 2 but was: 0

[cipherboy]

Thank you @fatima2003 and appreciate the assist @andreaso!

[fatima2003]

Thank you for the review @cipherboy and @andreaso!