Tighten security of Actions workflows#5930
Merged
chrisd8088 merged 2 commits intogit-lfs:mainfrom Dec 10, 2024
Merged
Conversation
As it stands, the architecture and the container are not user-controllable and do not presently contain spaces or other shell metacharacters, so there's no security or functionality problems by leaving the relevant environment variables unquoted. However, it's a good practice to quote these variables just in case, so let's do that.
By default, the `actions/checkout` action stores the credentials in the local Git config file. This is not really very secure and a credential manager would be a much better idea, but Actions has not yet done so. In the mean time, since we don't need those credentials to perform more Git operations, let's make sure to not persist them in the config, which means that it's less likely we'll accidentally expose them, such as by shipping them in an artifact.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Jan 24, 2025
In commit df6e49c of PR git-lfs#5243 we added a step to all of the jobs in our CI and release GitHub Actions workflows to work around the problem described in actions/checkout#290 and actions/checkout#882. This step, which only executes if the job is running due to the push of a new tag, performs a "git fetch" command of the tag's reference to ensure that the local copy is identical to the remote one and has not been converted from an annotated tag into a lightweight one. Starting with v2 of the actions/checkout action, annotated tags are by default replaced with lightweight ones, which then causes any subsequent "git describe" commands to return an incorrect value. Since we depend on the output of "git describe" in several places in our workflows to generate the appropriate version name for our release artifacts, we need to ensure we have the full annotated tag for the current reference rather than a lightweight one. Recently, in commit 9c3fab1 of PR git-lfs#5930, we strengthened the security of our GitHub Action workflows by setting the "persist-credentials" option of the actions/checkout action to "false", so that any cached Git credentials are removed at the end of that step. While this causes no problems when our CI workflow runs after a branch is pushed, as is the case for new PRs, when we push a new tag the "git fetch" step now fails as it depends on the cached Git credentials from the actions/checkout step. We could use the GITHUB_TOKEN Action secret to temporarily set an appropriate HTTP Authorization header to make the "git fetch" command succeed. However, a more straightforward solution exists whereby we specify explicitly the reference we want to check out using the "ref" option of the actions/checkout action. This causes the action to fetch the reference such that if the reference is an annotated tag, it remains one and is not converted into a lightweight one. For reference, see: actions/checkout#882 (comment) actions/runner-images#1717 (comment) h/t classabbyamp and xenoterracide for documenting this workaround
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
2 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.
I was recently reading an article about a repository whose release workflow was compromised due to code injection and how it could have been detected using zizmor. I did some auditing here of the Actions workflows, both manually and with zizmor, and I didn't find any obvious security problems, but I did some hardening.
The first patch was from my manual auditing and covers some improved shell quoting. This isn't currently exploitable, but we might as well avoid the problem altogether and using proper shell quoting is a best practice anyway. The second patch comes from the recommendation of zizmor.
We are following the best practice of storing expanded Actions patterns (those with
${{}}) in an environment variable before using them, because this prevents injections (such as from refs containing shell characters). Actions doesn't have any sort of quoting for these cases, but by passing them in via the environment, we can use the normal shell quoting rules for these purposes.So overall, I think we're in a good spot, but this PR just makes it a little less inviting as a target.
This PR contains independent, logical, bisectable commits each with their own commit message, and as such is best reviewed commit by commit.