-
-
Notifications
You must be signed in to change notification settings - Fork 415
Don't persist GitHub authentication token in .git/config on CI
#2187
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
Don't persist GitHub authentication token in .git/config on CI
#2187
Conversation
When `actions/checkout` is used to check out the repository on CI, it persists credentials related to the GitHub token in the local scope configuration at `.git/config`, unless `persist-credentials` is explicitly set to `false`. This facilitates subsequent remote operations on the repository that could otherwise fail, but we have no such operations in any of our workflows. As an added layer of protection to keep these credentials from leaking into logs (or otherwise being displayed or subject to exfiltration) in case there is ever unintended coupling between the operation of the test suite (or any step subsequent to checkout that is used to prepare or run tests or other checks) and the cloned `gitoxide` repository itself, this: - Adds `persist-credentials: false` in a `with` mapping on every step that uses `actions/checkout`. - Adds a new CI job that checks that every `actions/checkout` step in any job in any workflow sets `persist-credentials` to `false`. In addition to usual testing on CI, the `release.yml` workflow is among the workflows changed here, and it has also been tested: https://github.com/EliahKagan/gitoxide/actions/runs/17899238656 See also: - https://github.com/actions/checkout/blob/main/README.md (Covers what happens with/without `persist-credentials: false`). - actions/checkout#485
This also tests the job by manually trying out several ways it should fail to make sure it does, but I squashed those out. The can be seen at #105 and are summarized as follows: * Test that we always have `actions/checkout` not persist credentials * Check that we catch `actions/checkout` with no `with` * Improve `check-no-persist-credentials` output and maintainability * Check that we catch checkout `with` without `persist-credentials` * Check that we catch `persist-credentials` not set to boolean false * Having tested the new check, restore `persist-credentials: false`
|
I just realized that the way I did the partial squash was different from the way I described it, so the commit that is described as refining the new check is actually the one that adds the new check (with all the refinements included). That is, I had planned to have three commits and I ended up having two, with the first commit claiming to contain some changes that only come in as of the second commit. That might be better anyway, but it's not better that the commit messages are both slightly wrong; sorry about that, and I hope it doesn't cause any confusion. I think the actual changes are fine, and this issue with the messages doesn't hurt gitbutlerapp/gitbutler#10399 either. |
|
I noticed it when looking at this PR but also didn't think it was worth fixing 😅. |
- Give the workflow a shorter name - Also trigger on "run-ci" branches (in addition to main) - Also allow to be triggered from Actions tab - Comment out currently unneeded permissions - Use v5 of actions/checkout (rather than v4) - Don't persist auth token after checkout (see GitoxideLabs#2187)
- Give the workflow a shorter name - Also trigger on "run-ci" branches (in addition to main) - Also allow to be triggered from Actions tab - Comment out currently unneeded permissions - Use v5 of actions/checkout (rather than v4) - Don't persist auth token after checkout (see GitoxideLabs#2187)
This adds `persist-credientials: false` to all uses of `actions/checkout`, as was done in GitoxideLabs/gitoxide#2187. This doesn't add a hard check to fail CI if this isn't present. But Zizmor does catch its absence, which might be sufficient in view of the lower (but not zero) attack surface for persisted credentials here compared to `gitoxide` in the event of unintended coupling between the test suite here and CI clone of the repository itself. Another factor that reduces the risk across the board (though not necessarily by enough that we should rely solely on it and Zizmor for this in the `gitoxide` repository) is that `actions/checkout` keeps its credentials in a separate file, rather than in `.git/config`, since `v6`. For details on that, see: - https://github.com/actions/checkout/blob/main/CHANGELOG.md#v600 - actions/checkout#2286
When
actions/checkoutis used to check out the repository on CI, it persists credentials related to the GitHub token in the local scope configuration at.git/config, unlesspersist-credentialsis explicitly set tofalse. This facilitates subsequent remote operations on the repository that could otherwise fail, but we have no such operations in any of our workflows.As an added layer of protection to keep these credentials from leaking into logs (or otherwise being displayed or subject to exfiltration) in case there is ever unintended coupling between the operation of the test suite (or any step subsequent to checkout that is used to prepare or run tests or other checks) and the cloned
gitoxiderepository itself, this:persist-credentials: falsein awithmapping on every step that usesactions/checkout.actions/checkoutstep in any job in any workflow setspersist-credentialstofalse.In addition to usual testing on CI, the
release.ymlworkflow is among the workflows changed here, and it has also been tested: https://github.com/EliahKagan/gitoxide/actions/runs/17899238656See also:
(covers what happens with/without
persist-credentials: false)persist-credentialsor change the default tofalseactions/checkout#485