search

slsa-framework / slsa

Supply-chain Levels for Software Artifacts
https://slsa.dev
Other
1.49k stars 214 forks source link

PR Merge commits and verified history requirement #298

Open ianlewis opened 2 years ago

ianlewis commented 2 years ago

https://slsa.dev/spec/v0.1/requirements#verified-history

The verified history requirement currently states:

Every change in the revision’s history has at least one strongly authenticated actor identity (author, uploader, reviewer, etc.) and timestamp. It must be clear which identities were verified, and those identities must use two-step verification or similar.

Also:

[First-parent history] In the case of a non-linear version control system, where a revision can have more than one parent, only the “first parent history” is in scope. In other words, when a feature branch is merged back into the main branch, only the merge itself is in scope.

When using review systems or hosting platforms like Github where PRs are merged and by the system it seems like this requirement cannot be met because the Github identity isn't necessarily a relevant actor (is it?), and the two-step verification part is irrelevant (it should probably be a requirement for the person who hits the merge button?).

The only alternative to meet this requirement is to require the owner/admin to manually merge and sign the merge commits themselves? Am I understanding correctly?

mlieberman85 commented 2 years ago

Can you go a bit more in detail on the Github example?

The way I view it is that the persons reviewing the code should be strongly authenticated. In this case the reviewer's identity is the one responsible for the merge.

puerco commented 2 years ago

I think if a human actor merges a PR (at least in GitHub), the requirement is met. But maybe you mean when a bot merges the PRs @ianlewis? Say for example Prow in the Kubernetes repos:

commit e74c42aaf2b1eff98e15d46026ce13c6be987a4e (HEAD -> master, upstream/master)
Merge: 6ab748eeecb fca9b1d9fcc
Author: Kubernetes Prow Robot <k8s-ci-robot@users.noreply.github.com>
Date:   Wed Feb 9 14:10:01 2022 -0800

    Merge pull request #107880 from liggitt/kubectl-auth-token

    Add command to request a bound service account token

If that is what you mean, a short discussion started some months back about a possible exemption for bots: https://github.com/slsa-framework/slsa/issues/196

ianlewis commented 2 years ago

@puerco Yeah, I assume that a person on Github pushing the merge button is strongly authenticated (but is authenticated via the website ok? I read the SLSA language as requiring that the source commit is signed by that person). The issue I see is that the merge commit is signed by Github so the commit itself isn't authenticated with the key of the person who merged it.

It may be ok for the purposes of SLSA that we take Github's word for it that Github signing it means that the person is authenticated and has the right authority but that's what I would like clarification on as we are basically taking Github's word for it at that point. Bots I suppose are a similar issue. I think the issue is maybe whether we can trust that bots are a trusted actor or not. I'm thinking that if one day folks want to make policy regarding this, that it won't be a problem that none of the commits in scope are actually signed by the people making the change.

@mlieberman85 yeah, I think that should be right but the "First-parent history" language confused me. Is the mainline commits the only thing in scope? or do we actually care about the commits being reviewed (even though they might be essentially thrown away in a squash merge)?

TomHennen commented 2 years ago

Our view (at least @MarkLodato and I, I think) has been that GitHub doing a merge is acceptable as long as the author of the code was strongly authenticated and (if we're looking at SLSA L4) that the reviewer was strongly authenticated. How that authentication happens is unspecified at the moment (I'm sure different systems will want to do it different).

Are we "taking GitHub's word for it" I think so yes. Some of the other source requirements (like retention) depend upon some level of trust in the source control system. It still probably makes sense for someone to audit the service and certify that yes GitHub merges are safe, code is retained properly, etc...

ianlewis commented 2 years ago

I think that view is entirely reasonable but I think the language could be updated to make that more clear. Particularly the "first-parent-history" part.