-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Update version comments for SHA-pinned GitHub Actions #5951
Update version comments for SHA-pinned GitHub Actions #5951
Conversation
@jproberts Thanks so much for doing this! The PR looks pretty neat and seems to have extensive test coverage 💪. I'll hope to get to do a proper review of this PR soon-ish unless someone else beat me to it, but I wanted to drop a big THANKS for now. Also pretty nice detail to open a PR to your fork first and get a colleague to review it 🥇. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic to me, very well done.
Well done writing this flexibly enough to handle all the permutations people use for specifying version strings.
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # tag=v2.1.0 | ||
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0 | ||
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 #v2.1.0 | ||
- uses: actions/checkout@01aecc # v2.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Major 💖 for phenom set of test cases.
Two more cases:
- What about the case where a comment includes the version, but then some text?
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # Versions older than v2.1.0 have a security vulnerability
- What if the version string is repeated twice?
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0 - Versions older than v2.1.0 have a security vulnerability
I don't want to throw up roadblocks on this awesome PR, so if for simplicity neither case is bumped, that's perfectly acceptable. So one simple heuristic could be if there's any text after the version string, then don't bump the version string... Users that still want it bumped can instead leave their comment as a full-line comment above the actual code line:
# Versions older than v2.1.0 have a security vulnerability
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0
But we do need to ensure we never auto-bump the comment to start incorrectly saying "Versions older than v2.2.0 have a security vulnerability"
...
So can you add these two test cases to ensure no future regressions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. I was worried the simple search-and-replace was too broad but I couldn't come up with a sample comment where we wouldn't want to bump the version. The cases you mentioned are exactly what I was looking for!
I've updated the PR with the simple fix to only change comments if there's no text after the version string.
GitHub encourages pinning third-party GitHub Actions to a full length commit SHA. It's common for actions pinned by commit SHA to include a comment specifying the version associated with the commit. For example: - uses: actions/checkout@01aecc # v2.1.0 This change updates the GitHub Actions manager to bump versions in comments that follow SHA-pinned actions, so the comment stays up-to-date with the SHA being updated. The file_updater now searches the comment string for all references to the previous version and replaces them with the new version. To avoid changing unrelated comments, the comment updater only updates dependencies that pin SHA refs.
If the version in a comment is followed by additional text, it could be documentation explaining some behavior, rather than a tag corresponding to the SHA in the action version. To be safe, don't update the version unless it's the end of the comment.
96b107c
to
a459c88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me.
Had one minor pedantic suggestion about tweaking the description of intended behavior, but I'm also fine to merge as-is... up to you.
We should be able to get this deployed/merged in the next few days...
For the CI failure here, it's probably best to just disable the Rubocop line-length check for that block of code: Example of how to do that here: Just don't forget to re-enable it after the block as demonstrated in the above example as well. |
FYI @jproberts this got a nice shout out on the blog: https://github.blog/changelog/2022-10-31-dependabot-now-updates-comments-in-github-actions-workflows-referencing-action-versions/ |
I just saw the post and I'm amazed. Thanks so much for your support @jeffwidman ❤️ |
Congrats on the post! Is this feature compatible with renovatebot's way of handling comments? Renovatebot expects a comment of the form It would be useful for the various update tools to be compatible on this point. Wdut? /cc @rarkins from renovatebot |
I have created renovatebot/renovate#18640 recently to align Renovate with this approach. As described in that issue, I have concerns about assuming any comment represents a tag (e.g. |
Thanks for swinging by. Agreed that getting the update tools aligned on the implicit format spec would be great. The Thankfully @jproberts was thorough and already included unit tests for all the formats I've seen in the wild: dependabot-core/github_actions/spec/fixtures/workflow_files/pinned_sources_version_comments.yml Lines 7 to 30 in 45f52b3
@rarkins you might find the whole set of examples useful to test against, particularly the negative tests. If you come up with additional edge cases we missed, feel free to circle back and let us know. |
Thanks for this, saves a lot of time! |
Do I need to do anything to opt in to this? Not seeing the version update: https://github.com/argoproj/argo-cd/pull/11480/files |
Dependabot should be able to update these and preserve the tag in the comment, as per: dependabot/dependabot-core#5951 Workflows updated with: https://app.stepsecurity.io/secureworkflow Signed-off-by: Sunjay Bhatia <[email protected]>
Dependabot should be able to update these and preserve the tag in the comment, as per: dependabot/dependabot-core#5951 Workflows updated with: https://app.stepsecurity.io/secureworkflow Signed-off-by: Sunjay Bhatia <[email protected]>
Dependabot should be able to update these and preserve the tag in the comment, as per: dependabot/dependabot-core#5951 Workflows updated with: https://app.stepsecurity.io/secureworkflow Signed-off-by: Sunjay Bhatia <[email protected]>
Danger of enabling SHA pinning for actions is that they're not particualrly readable and so we could strand versions and not get legitimate updates. DepBot now supports updates for pinned actions where there is the version number in a comment afterwards (as the tool linked in previous commits provides automatically). So let's enable DepBot scans, initially set at weekly. See: dependabot/dependabot-core#5951 https://docs.github.com/en/code-security/dependabot/working-with-dependabot/keeping-your-actions-up-to-date-with-dependabot https://github.blog/changelog/2022-10-31-dependabot-now-updates-comments-in-github-actions-workflows-referencing-action-versions/
Danger of enabling SHA pinning for actions is that they're not particualrly readable and so we could strand versions and not get legitimate updates. DepBot now supports updates for pinned actions where there is the version number in a comment afterwards (as the tool linked in previous commits provides automatically). So let's enable DepBot scans, initially set at weekly. See: dependabot/dependabot-core#5951 https://docs.github.com/en/code-security/dependabot/working-with-dependabot/keeping-your-actions-up-to-date-with-dependabot https://github.blog/changelog/2022-10-31-dependabot-now-updates-comments-in-github-actions-workflows-referencing-action-versions/
SHA pinning is a sensible approach to mitigate potential supply chain attacks. See some great blog posts here on the approach: https://blog.rafaelgss.dev/why-you-should-pin-actions-by-commit-hash also https://michaelheap.com/improve-your-github-actions-security/#using-pin-github-action However one down side is that SHA's are not very human readable. It can be difficult to tell from the SHA if the version we've pinned has an update, or if that update is a security or important fix. Best practice therefore is to place a comment after the pinned version listing the semantic version for a third party github action. This gets you best of both worlds, maintainability plus certainty. It might look at bit like this: ``` jobs: check-pull-request: runs-on: ubuntu-latest steps: - name: Check out repository code uses: actions/checkout@ee066bloop # pin @v2 - name: Install Ruby uses: ruby/setup-ruby@22acsewblah # pin@v1 ``` Consistency here also helps us manage this code in line with the GDS Way requirement to Update dependencies frequently when managing third party dependencies: https://gds-way.digital.cabinet-office.gov.uk/standards/tracking-dependencies.html#update-dependencies-frequently Since October 2022 DependaBot will now look for comments on SHA pinning and automatically suggest updates. Similar approaches may be possible for other dependency management tools. Dependabot currently supports a range of different comment syntaxses which can be viewed here: dependabot/dependabot-core#5951 (comment) I've tried to keep the guidance general and open, leaving detail to this commit history, given the range of different tools on use across GDS. The principles are: - Pin your actions using SHAs - Ensure human readability by commenting the semver on the line with the action - Explore if your usual dependency management process, especially automated ones like DependaBot can help flag and raise visibility on new versions.
SHA pinning is a sensible approach to mitigate potential supply chain attacks. See some great blog posts here on the approach: https://blog.rafaelgss.dev/why-you-should-pin-actions-by-commit-hash also https://michaelheap.com/improve-your-github-actions-security/#using-pin-github-action However one down side is that SHA's are not very human readable. It can be difficult to tell from the SHA if the version we've pinned has an update, or if that update is a security or important fix. Best practice therefore is to place a comment after the pinned version listing the semantic version for a third party github action. This gets you best of both worlds, maintainability plus certainty. It might look at bit like this: ``` jobs: check-pull-request: runs-on: ubuntu-latest steps: - name: Check out repository code uses: actions/checkout@ee066bloop # pin @v2 - name: Install Ruby uses: ruby/setup-ruby@22acsewblah # pin@v1 ``` Consistency here also helps us manage this code in line with the GDS Way requirement to Update dependencies frequently when managing third party dependencies: https://gds-way.digital.cabinet-office.gov.uk/standards/tracking-dependencies.html#update-dependencies-frequently Since October 2022 DependaBot will now look for comments on SHA pinning and automatically suggest updates. Similar approaches may be possible for other dependency management tools. Dependabot currently supports a range of different comment syntaxses which can be viewed here: dependabot/dependabot-core#5951 (comment) I've tried to keep the guidance general and open, leaving detail to this commit history, given the range of different tools on use across GDS. The principles are: - Pin your actions using SHAs - Ensure human readability by commenting the semver on the line with the action - Explore if your usual dependency management process, especially automated ones like DependaBot can help flag and raise visibility on new versions.
SHA pinning is a sensible approach to mitigate potential supply chain attacks. See some great blog posts here on the approach: https://blog.rafaelgss.dev/why-you-should-pin-actions-by-commit-hash also https://michaelheap.com/improve-your-github-actions-security/#using-pin-github-action However one down side is that SHA's are not very human readable. It can be difficult to tell from the SHA if the version we've pinned has an update, or if that update is a security or important fix. Best practice therefore is to place a comment after the pinned version listing the semantic version for a third party github action. This gets you best of both worlds, maintainability plus certainty. It might look at bit like this: ``` jobs: check-pull-request: runs-on: ubuntu-latest steps: - name: Check out repository code uses: actions/checkout@ee066bloop # pin @v2 - name: Install Ruby uses: ruby/setup-ruby@22acsewblah # pin@v1 ``` Consistency here also helps us manage this code in line with the GDS Way requirement to Update dependencies frequently when managing third party dependencies: https://gds-way.digital.cabinet-office.gov.uk/standards/tracking-dependencies.html#update-dependencies-frequently Since October 2022 DependaBot will now look for comments on SHA pinning and automatically suggest updates. Similar approaches may be possible for other dependency management tools. Dependabot currently supports a range of different comment syntaxses which can be viewed here: dependabot/dependabot-core#5951 (comment) I've tried to keep the guidance general and open, leaving detail to this commit history, given the range of different tools on use across GDS. The principles are: - Pin your actions using SHAs - Ensure human readability by commenting the semver on the line with the action - Explore if your usual dependency management process, especially automated ones like DependaBot can help flag and raise visibility on new versions.
GitHub encourages pinning third-party GitHub Actions to a full length commit SHA. It's common for actions pinned by commit SHA to include a comment specifying the version associated with the commit. For example:
This change updates the GitHub Actions manager to bump versions in comments that follow SHA-pinned actions, so the comment stays up-to-date with the SHA being updated.
The file_updater now searches the comment string for all references to the previous version and replaces them with the new version. To avoid changing unrelated comments, the comment updater only updates dependencies that pin SHA refs.
Closes #4691