Skip to content
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

Hash-pin GitHub Actions, add dependabot to keep them up-to-date #2291

Closed
wants to merge 3 commits into from

Conversation

pnacht
Copy link
Contributor

@pnacht pnacht commented Sep 6, 2023

Fixes #2290.

This PR hash-pins all workflow GitHub Actions to protect the project from supply-chain threats. This includes the Actions used by the local release-archive Action.

The hashes (and corresponding version comments) will be updated by dependabot, which this PR also adds. I've configured dependabot to use the new grouped updates feature, so it'll send a single PR updating all GitHub Actions at once.

Or, well, if actions/upload-artifact and svenstaro/release-upload-action have new versions, it'll be two PRs: one for the release-archive Action and one for the workflows.

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
@keithw
Copy link
Member

keithw commented Sep 6, 2023

Unfortunately upgrading actions/checkout from v1 to v4.0.0 seems to break the lint check (which is trying to compare against origin/main). Is there some option we can give to make a clone that includes all the branches (or at least that branch)?

@keithw
Copy link
Member

keithw commented Sep 6, 2023

On the security, I guess for my edification: if we think it's an unwise risk to trust the maintainers of the actions organization and svenstaro to be able to update their actions, what makes it better for us to pin the code but then accept PRs from Dependabot every time that the actions folks (or svenstaro) update their actions?

Should we be doing our own security review on those Dependabot PRs, by examining the underlying change in the workflows (and all the workflows transitively referenced) every time there's an update, instead of accepting the PR blindly? That seems pretty hard to do with the current tooling, and it seems silly for every single project that depends on these popular actions to have to do its own independent security review when the actions change upstream.

OTOH, if we just blindly accept every Dependabot PR, isn't that essentially the same as the status quo?

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
@pnacht
Copy link
Contributor Author

pnacht commented Sep 7, 2023

Unfortunately upgrading actions/checkout from v1 to v4.0.0 seems to break the lint check (which is trying to compare against origin/main). Is there some option we can give to make a clone that includes all the branches (or at least that branch)?

Ah, sorry, I'd missed this. v1fetched the entire history. v2 (and onwards) changed that to only fetch the most recent commit by default. I've updated the checkout for that job to fetch the entire history of all branches.

On the security, I guess for my edification: if we think it's an unwise risk to trust the maintainers of the actions organization and svenstaro to be able to update their actions, what makes it better for us to pin the code but then accept PRs from Dependabot every time that the actions folks (or svenstaro) update their actions?

Should we be doing our own security review on those Dependabot PRs, by examining the underlying change in the workflows (and all the workflows transitively referenced) every time there's an update, instead of accepting the PR blindly? That seems pretty hard to do with the current tooling, and it seems silly for every single project that depends on these popular actions to have to do its own independent security review when the actions change upstream.

OTOH, if we just blindly accept every Dependabot PR, isn't that essentially the same as the status quo?

Indeed, from a security perspective, the best thing would be to check the changes between the two versions. But I also agree this isn't entirely feasible.

However, even if you simply merge all Dependabot PRs, there's still the benefit of the delay between the new version's release and when your repository is impacted, since dependabot only updates weekly (or monthly, if you prefer). So if a broken or malicious release is published, there's a good chance it'll be detected (and likely reverted) before you even receive the update PR. Without hash-pinning, you would be immediately affected as soon as the problematic release is published.

@pnacht
Copy link
Contributor Author

pnacht commented Sep 7, 2023

One thing I forgot to mention earlier is that I have not hash-pinned the OSS-Fuzz actions in wabt-cifuzz.yml. Given how the project is designed, it unfortunately needs to run @master.

@keithw
Copy link
Member

keithw commented Sep 8, 2023

Indeed, from a security perspective, the best thing would be to check the changes between the two versions. But I also agree this isn't entirely feasible.

However, even if you simply merge all Dependabot PRs, there's still the benefit of the delay between the new version's release and when your repository is impacted, since dependabot only updates weekly (or monthly, if you prefer). So if a broken or malicious release is published, there's a good chance it'll be detected (and likely reverted) before you even receive the update PR. Without hash-pinning, you would be immediately affected as soon as the problematic release is published.

Sigh... I feel like this problem has already been solved in a better way, e.g. how GNU/Linux distributions have done things since the 1990s. One entity (the author/vendor) publishes a revision, and a separate entity (the distributor) represents the users and decides whether to accept the revision and distribute it. There's an audit trail of revisions accepted by the distributor and what changed in each one. It's depressing to see the world of GitHub-style FOSS reinvent suckier versions of these patterns and call it security. :-(

If we don't trust these action publishers, then I don't feel great about trusting a system that relies on every downstream project to either (1) review the autogenerated PR when the action changes upstream [afaik there is no tooling that would let us even see the transitive closure of changes to a bunch of hash-pinned GitHub workflows], or (2) wait a bit and hope "some other project" full of go-getters somehow reviews the PR first and detects and reports the 1-in-a-10,000 case when the PR represents a harmful update. :-(

If we think these PRs might be harmful, then somebody in particular should be reviewing them (akin to a GNU/Linux distributor) and we should be able to see that it got reviewed before we merge it. Just the negative signal of, "well, we waited a bit and nobody has complained" seems liable to induce a false sense of security -- I worry that nobody is going to actually look at these PRs.

@sbc100
Copy link
Member

sbc100 commented Sep 8, 2023

Yeah, I am also somewhat skeptical of this process. See emscripten-core/emsdk#1244.

Given that wabt is basically an offline tool that mostly processes trusted inputs (on a developers local machine) I'm not sure this level of concern over supply chain is warranted.

@sbc100
Copy link
Member

sbc100 commented Sep 8, 2023

Given that wabt is basically an offline tool that mostly processes trusted inputs (on a developers local machine) I'm not sure this level of concern over supply chain is warranted.

Maybe these will go down as famous last words though :)

@keithw
Copy link
Member

keithw commented Sep 8, 2023

Is it silly to just hash-pin the actions (except oss-fuzz) and skip the Dependabot part? Morally this seems pretty similar to where we are today...

@pnacht
Copy link
Contributor Author

pnacht commented Sep 8, 2023

Is it silly to just hash-pin the actions (except oss-fuzz) and skip the Dependabot part? Morally this seems pretty similar to where we are today...

Well, that would freeze you at the versions you're currently using. That would protect you from future breakages or vulnerabilities that may be added to the Actions. But it would keep you exposed to underlying vulnerabilities that have yet to be discovered (i.e. log4shell existed in the log4j codebase since 2013, but was only discovered in 2021).

Keeping Actions major-version pinned exposes you to new potential vulnerabilities but also protects you from bugs/vulnerabilities as they are fixed.

My suggestion, if hash-pinning isn't of interest, would be to at least have Dependabot installed to keep an eye on the version tags. For example, some wabt workflows are currently running actions/setup-python@v1 and actions/checkout@v1, but those Actions are now up at @v4. Dependabot would detect these new major versions and notify the project.

Let me know if you'd rather take this route.

@keithw
Copy link
Member

keithw commented Sep 9, 2023

I don't think the hash-pinning is the issue; it's more that it feels weird to subscribe to an inbound stream of PRs about changes to these actions if we have no way to usefully review the PR ourselves, and no third party who's affirmatively saying, "yes, I have reviewed this version on your behalf and I think you should update to it."

In the GNU/Linux world, the distributor plays that role; if I was trying to improve supply-chain security in the "GitHub-style" world, I guess I'd try to set up similar "middleman" organizations rather than asking every downstream project to start reviewing every change to their upstream dependencies (especially when the upstream dependency is written in an unfamiliar programming language, and most projects are probably just going to rubber-stamp the PR -- this seems like security theater). :-(

@pnacht
Copy link
Contributor Author

pnacht commented Sep 11, 2023

Understood. Let me know if you're interested in just dependabot (keeping major-version pins), otherwise feel free to close this PR and #2290.

@keithw
Copy link
Member

keithw commented Sep 15, 2023

All right, closing for now. If there's an opportunity to be part of the conversation that produces these recommendations, I think we'd be happy to participate if that would be viewed as helpful. My sense is that there seems to be a lot of accumulated wisdom from the first 25 years of FOSS (and a willingness to set up entities, like a Linux distribution, that intermediate between software authors and consumers) that isn't reaching the "GitHub-style" world, which seems a little more corporate-driven and wants each individual project to stand alone. :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hash-pin workflow GitHub Actions
3 participants