Add signatures audit to CI build pipeline

[Harvester57]

Hi !

This PR introduce the following changes, related to security/quality of the build pipeline:

• Add formal names for each build step
• Audit production packages for known vulnerabilities, after checkout and before performing the clean installation
• Audit signatures of installed packages after installation, to detect potential software supply chain attacks
• Add linting step before building
• Move the FORCE_COLOR environment variable as a global variable, to avoid repetition for every build steps

[tyrasd]

Honestly, I'm not the biggest fan of repeating a command like npm run dist just slightly differently worded as the step's name: NPM - Build dist. That's just needlessly adding lines to the script without any benefit. 🤷

On the concrete suggestions to the changed workflow logic: I've got a few questions, see them inline below.

[Harvester57]

Honestly, I'm not the biggest fan of repeating a command like npm run dist just slightly differently worded as the step's name: NPM - Build dist. That's just needlessly adding lines to the script without any benefit. 🤷

Fair enough, I'll remove the redundant names for the steps

[k-yle]

what's the benefit of pinning to a commit hash for first-party packages published by GitHub themselves?

[Harvester57]

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.

[k-yle]

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:

Suggested change

	- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
	- uses: actions/checkout@5a96951a20872f15b767339256a01fe5c4d1478f # v4.123.123

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

[tordans]

But actions/checkout is 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#usage

[Harvester57]

@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.

[Harvester57]

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 !

[k-yle]

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:

Dependabot only creates alerts for vulnerable actions that use semantic versioning and will not create alerts for actions pinned to SHA values. [source]

But this contradicts dependabot/dependabot-core#5951 😆 suggesting that (at least part of) those docs are 3 years out-of-date

[Harvester57]

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

[tyrasd]

PS: I've now applied the version/hash pinning also to the other two workflows (codespell and transifex) that we use in this repo.

[k-yle]

sorry, I realised I never replied to this discussion 😅 For completeness, I meant that pinning to a commit hash requires maintainers to do extra work¹, 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:

- uses: actions/checkout@85b8272e24166014adcf9ff6ef3701031878ec80 # v5.0.0

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, 85b8272 is not from the actions/checkout repo. 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

1. (manually validating every commit hash added by external contributors) ↩

addressed

[tyrasd]

Thanks for the PR and the extra background information @Harvester57. I did also learn quite a bit in this thread. At first I was of the impression that it might be overkill to pin dependencies from the very same company that provides us also with the rest of our build infrastructure (i.e. dependabot and the run environment itself, which theoretically could also become compromised).

But I agree that there is some merit to apply the same guideline that we have for other dependencies (i.e. pin versions to their commit hash; for npm dependencies via the package-lock file) also for all other dependencies, regardless of who the maintainer is.

Yes, we might get some extra dependabot PRs from time to time, but that shouldn't be an issue in practice. And any PRs from external contributors that update the hashes (in the gh workflows or package-lock) should automatically ring the bells to be extra cautious when reviewing a PR.