-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add signatures audit to CI build pipeline #11103
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
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
3adcd9d
FORCE_COLOR as global env variable
Harvester57 c4aa2ea
Add names to steps
Harvester57 72ff6ca
Add signatures audit after clean-install
Harvester57 550f432
Add vulnerability audit for production packages
Harvester57 3d9eaa3
Add linting step before build
Harvester57 10a320b
Add steps names and signatures audit to staging
Harvester57 761356b
Remvoe audit step
Harvester57 ea196f4
Remove linting step
Harvester57 08ab855
Remove erroneous submodule
Harvester57 fe46288
Pin Actions with SHA256
Harvester57 20d9298
Remove redundant steps names
Harvester57 afa38a2
pin action denencencies to the commit hash
tyrasd 6be5beb
ping action version to commit hash
tyrasd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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.
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.
what's the benefit of pinning to a commit hash for first-party packages published by GitHub themselves?
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.
I'm not sure there is such a distinction between first-party and third-party actions, in the sense that the checkout action, and all other GitHub official actions, are following the same development model as any other action: they have a public GH repository (https://github.com/actions/checkout), that is open to external contributors, and therefore the threat model is the same for this action as any other one.
They are not immune to a repository takeover that would push or rewrite an existing tag, and I would argue that given the ubiquity of this particular checkout action, it is even more important to pin them.
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.
I see, I was wondering becuase I regularly hear the opposite advice: never pin to a commit hash, because the commit hash could come from a fork of the repository (which is a well known GitHub quirk
[better source needed])
For example, a malicious contributor could change this line to the following:
And for a code reviewer, it's a lot of work to manually check if the commit hash matches the version in the code comment. So the code reviers might not bother to check, since "actions/checkout" looks legitimate.
In that example above, commit 5a9695 comes from a deleted fork, so it could contain any malicious code.
...and GitHub Actions will happily pull the malicious commit from the fork and run it, ignoring the explicit repository name of "actions/checkout":
anyway.... this is all very hypothetical, so don't read this as a code review, I'm just curious about the rationale behind this change
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.
But
actions/checkoutis actually maintained by Github, right? At least the COC suggests so. TBH I have a higher trust that Github has their merges under control … or resolves them quickly than any other dependency we have in iD. Which is why I would vote to keep this simple and follow usage recommendations they provide https://github.com/actions/checkout?tab=readme-ov-file#usageThere 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.
@k-yle: manual updates of Actions SHA should be prohibited and only PRs opened by Dependabot should be reviewed and merged, especially since you already have Dependabot configured for this repository.
Even GitHub's own Secure Reference manual recommend to pin the SHA of third party Actions: https://docs.github.com/en/actions/reference/security/secure-use#using-third-party-actions
@tordans: it is indeed maintained by GitHub, but the question is, why should you treat Actions developed by GitHub any differently than any other Action ? From your perspective, they are part of your supply chain, and you use them to deploy staging instances and build your code in your workflows.
I would argue that, given their market size, and give the amount of repos using these basic Actions (checkout, setup, ...), it makes them a way more interesting target than any other third party Action. You might expect (and this could be the case in practice) that they have additional security measures and processes in place for these repositories, but their employees and code owners are still vulnerable to phishing, or any other type of attack that you could read about in other supply chain attacks in the past few years.
Regarding the argument of speed of resolution, you might be right, but if I look at the iD repository statistics of your workflows (https://github.com/openstreetmap/iD/actions/metrics/usage?dateRangeType=DATE_RANGE_TYPE_CURRENT_WEEK), the build job ran on average 10 times per day for the last week, and the deploy to staging instance 3.5 times per day, which in my opinion leaves plenty of time to an attacker that compromised a tagged Action to run its code.
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.
I'd like to point out that you already have a very good best practice in place (#9273) by reducing the GITHUB_TOKEN default permissions, very positive !
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.
i find this topic quite interesting, because this approach basically moves the onus to the downstream maintainer to manually enforce your rule that "manual updates of Actions SHA should be prohibited"
and the likelihood of someone forgetting this rule is significantly higher than the likelihood of github's own repository getting compromised.
which is why i'm quite suprised that github's advice is the opposite of defensive design. There's also a mistake on that docs page which affects the viability of this solution. The docs proclaim:
But this contradicts dependabot/dependabot-core#5951 😆 suggesting that (at least part of) those docs are 3 years out-of-date
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.
I'm failing a bit to see how it's opposing the defensive design paradigm, could you expand on this ? To be noted that pinning hashes is also recommended by the OpenSSF in their Scorecard approach, with the same rationale, and the same logic to let the dependencies be updated by automated means: https://github.com/ossf/scorecard/blob/main/docs/checks.md#pinned-dependencies
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.
PS: I've now applied the version/hash pinning also to the other two workflows (codespell and transifex) that we use in this repo.
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.
sorry, I realised I never replied to this discussion 😅 For completeness, I meant that pinning to a commit hash requires maintainers to do extra work1, with no safeguards to ensure that the hashes are actually checked.
Generally speaking, requiring people to remember and follow an SOP isn't great defensive design, and in other industries this would be one the weakest types of defenses.
So it's easy for a malicious user to fool a maintainer into accepting code that looks legitimate, such as this line:
It includes the words actions/checkout, so many people would assume that this code must come from the actions/checkout repo. but it doesn't!
if we open that commit (actions/checkout@85b8272) the author and content match the real v5 commit, the purported author is a github employee, and the URL contains actions/checkout. So it looks very legitimate.
There is a warning banner, but it only explains the problem, not the risk. And unfortunately this banner is understandably oft ignored because of several known false-positives.
In reality,
85b8272is not from theactions/checkoutrepo. It's from a deleted fork, and contains malicious code added several commit ago, with fake commit history replayed ontop of the malicious commit.So anyway, this approach works for us, but it's shockingly easy to fool people when you use commit hashes
Footnotes
(manually validating every commit hash added by external contributors) ↩