content: Add draft of the Source track. #1037
Conversation
✅ Deploy Preview for slsa ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
kpk47
changed the title
docs/spec/v1.1/source-att.md
Outdated
| | Field | Type | Description | ||
| | --- | --- | --- | ||
| | `enabled` | boolean | If true, then the feature was enabled at the time of attestation generation. | ||
| | `since` | string | ISO 8601 representation of the timestamp at which the feature was last enabled. |
There was a problem hiding this comment.
Sure. This challenges the ability for an entity that isn't the source control platform to be able to generate the data required for an attestation. Any other entity wouldn't be continuously watching the configurations to be able to know if the features have been switched off. If the systems only check for the presence at the time of a build, for example, then there is no way for that system to know that the setting wasn't disabled temporarily, a commit or two deleted, and then reenabled before the build.
docs/spec/v1.1/source-attestation.md
Outdated
|
|
||
| | Field | Type | Required at Level | Description | ||
| | --- | --- | --- | --- | ||
| | `projectID` | string | L1+ | Immutable project ID. For example, the SLSA Specification's project ID is `www.github.com/slsa-framework/slsa`. |
There was a problem hiding this comment.
Repositories and usernames / orgs can be renamed, so I don't know if immutable works here.
There was a problem hiding this comment.
note that it's possible to provide an immutable ID: GitHub does support immutable user and repository IDs that don't change even when the names do
docs/spec/v1.1/source-attestation.md
Outdated
| | Field | Type | Required at Level | Description | ||
| | --- | --- | --- | --- | ||
| | `projectID` | string | L1+ | Immutable project ID. For example, the SLSA Specification's project ID is `www.github.com/slsa-framework/slsa`. | ||
| | `revisionID` | string | L1+ | Immutable revision ID. In git terminology, this is the commit hash. |
There was a problem hiding this comment.
Is this or digest below going to match the subject of the attestation?
There was a problem hiding this comment.
Also, could this be a tag object ID or must it always be resolved to a commit?
There was a problem hiding this comment.
Is this or digest below going to match the subject of the attestation?
The digest below.
Also, could this be a tag object ID or must it always be resolved to a commit?
I've been avoiding tying anything to tags because they're mutable. Do we gain something by using tags here?
There was a problem hiding this comment.
The digest below.
got it.
FWIW, I could also see both the revision ID and the tree ID listed in the subject as digests (the algorithm would be gitCommit and gitTree rather than sha1 so there wouldn't be a collision). I'm mentioning it now but it can likely wait until the exact format for the predicate is decided, per the other thread.
I've been avoiding tying anything to tags because they're mutable. Do we gain something by using tags here?
A tag object ID (ie its SHA-1 hash) is immutable while a tag reference is not. I'm primarily asking for cases where a source attestation is created for a release identified by a tag object. If we expect the field to be a commit ID, we should maybe call out that tag objects for a release must be resolved to the commit they point to (also, there's an edge case here because a tag object can point to any git object type, not just commits, so maybe that should be called out too).
docs/spec/v1.1/levels.md
Outdated
| - **[Context-specific approvals]** Approvals are for a specific context, such as a repo + target branch + revision in Git. Moving fully reviewed content from one context to another still requires review. (Exact definition of "context" depends on the project, and this does not preclude well-understood automatic or reviewless merges, such as cutting a release branch.) For example, if a fully reviewed commit in one repo is merged into a different repo, or a commit in one branch is merged into a different branch, then the merge still requires review. | ||
| - **[Atomic change sets]** Changes are recorded in the change history as a single revision that consists of the net delta between the proposed revision and the parent revision. In the case of a nonlinear version control system, where a revision can have more than one parent, the diff must be against the "first common parent" between the parents. In other words, when a feature branch is merged back into the main branch, only the merge itself is in scope. | ||
|
|
||
| Trusted robots MAY be exempted from the code review process. It is RECOMMENDED that trusted robots so exempted run only software built at Build L3+ from sources that meet Source L3. Note: Meeting Build L3+ and Source L3 are not necessarily sufficient evidence that software is safe to run as a trusted robot. |
There was a problem hiding this comment.
This first sentence makes sense, but I am not sure where the recommendation is coming from. It seems unrelated to the track and more related to some applications of SLSA.
There was a problem hiding this comment.
I interpret the recommendation to mean something like "if you're going to exempt an automated process from meeting the source track requirements, make sure you can trust that automated process—i.e., it's tamper-resistant thanks to coming from a SLSA Build L3+ process"
There was a problem hiding this comment.
This topic (trusted robots) probably warrants further discussion and more guidance in the spec. Maybe just add a TODO to flesh it out? I doubt we'll get it right on the first try.
|
@adityasaky @arewm I think we should go back to the drawing board with the source attestation/evidence, so I've removed it from this PR. Let's get the level spec to a place we're happy merging, and then I'll open a new PR where we can continue working on the attestation/evidence requirements. |
|
Sounds good! |
There was a problem hiding this comment.
Apologies for being late to review this. It's fantastic to see the beginnings of a Source track emerge. Thank you for the continued effort on this @kpk47.
This feels equivalent to what we had in v0.1, only compressed into 3 levels instead of 4 and with more affordances for changes from robots. Does that assessment feel fair/accurate to you?
Most of my comments are related to presentation, the requirements feeling so close to the v0.1 version is a great baseline.
The formatting and structure of the Levels changes don’t match the existing Build track levels.
Build levels are broken down into producer and platform requirements (see change from benefits to focus in cc812c3) and are a descriptive overview requirements, whereas the newly added source requirements are detailed and specific enough that they run the risk of seeming authoritative and contradictory to the track text.
The formatting and structure of the Source requirements doesn’t match the “Producing artefacts” document, where the requirements are in a table with check marks for which level they apply to (and further, there are tables per requirement category: provenance generation and isolation, I’m not sure this split makes sense for the source track).
Note: we should update future-directions to remove the source track and/or update it to mention what could come next. Threats and mitigations should be updated too.
Final thought, is adding a new level appropriate for a minor release, or should we consider a major release for this?
| - **[Change history]** There exists a record of the history of changes that went into the revision. Each change MUST contain: | ||
| - The immutable reference to the new revision | ||
| - The identities of the proposer, reviewers (if any), and merger (if different to the proposer) | ||
| - Timestamps of the reviews (if any) and submission |
There was a problem hiding this comment.
For most revision control systems the reviews are a feature of a larger platform (i.e., a forge like GitHub or GitLab, or a review tool like Gerrit or ReviewBoard). Do we know that the requirement to include timestamps of reviews and identify of reviewers is met by comon platforms?
Put differently, as these properties are part of the Level 1 requirements, is Level 1 easily attainable? Perhaps the platform requirements should be levelled?
Additionally, does submission here refer to submitting source code, reviews, or both? Could be worth clarification.
docs/spec/v1.1/levels.md
Outdated
| - **[Context-specific approvals]** Approvals are for a specific context, such as a repo + target branch + revision in Git. Moving fully reviewed content from one context to another still requires review. (Exact definition of "context" depends on the project, and this does not preclude well-understood automatic or reviewless merges, such as cutting a release branch.) For example, if a fully reviewed commit in one repo is merged into a different repo, or a commit in one branch is merged into a different branch, then the merge still requires review. | ||
| - **[Atomic change sets]** Changes are recorded in the change history as a single revision that consists of the net delta between the proposed revision and the parent revision. In the case of a nonlinear version control system, where a revision can have more than one parent, the diff must be against the "first common parent" between the parents. In other words, when a feature branch is merged back into the main branch, only the merge itself is in scope. | ||
|
|
||
| Trusted robots MAY be exempted from the code review process. It is RECOMMENDED that trusted robots so exempted run only software built at Build L3+ from sources that meet Source L3. Note: Meeting Build L3+ and Source L3 are not necessarily sufficient evidence that software is safe to run as a trusted robot. |
There was a problem hiding this comment.
I interpret the recommendation to mean something like "if you're going to exempt an automated process from meeting the source track requirements, make sure you can trust that automated process—i.e., it's tamper-resistant thanks to coming from a SLSA Build L3+ process"
docs/spec/v1.1/terminology.md
Outdated
|
|
||
| #### Source Roles | ||
|
|
||
| | Role | Description |
There was a problem hiding this comment.
Same meta-comment on sort order here.
There was a problem hiding this comment.
I did my best to reorder the roles conceptually, but it's a little tricky since none of the terms appear in other definitions. I'm open to other orderings.
We've talked about this on Monday's call actually. We agreed to punt on this issue for now and wait until we decide to publish the next version of the spec. We can then discuss what change to the version number is the most appropriate based on what ends up being in the spec. For now we can still work on "1.1" without it being binding. |
Sounds reasonable, thanks for sharing the outcome of the discussion. |
Signed-off-by: kpk47 <kkris@google.com>
Co-authored-by: Zachariah Cox <zachariahcox@github.com> Co-authored-by: Joshua Lock <joshuagloe@gmail.com> Signed-off-by: Mark Lodato <lodatom@gmail.com>
Signed-off-by: Joshua Lock <joshuagloe@gmail.com>
|
I rebased the PR on main and pushed a commit to resolve the linter warnings. Let's figure out remaining minor changes to land this PR and then build on it in future PRs like #1066. |
There was a problem hiding this comment.
This is good progress.
I vote to merge this one and address the remaining questions with subsequent prs:
source-track
|
+1 I think we can merge this which will unblock additional changes. IIUC after Monday's meeting @joshuagl was looking for someone like @mlieberman85 to review, approve, and merge. WDYT Mike? |
There was a problem hiding this comment.
I think merging this PR as-is is a good path forward. I started to cherry-pick @Nikokrock's top commit from #1082, but it failed the linting phase and the edits I had to make touched a lot of lines.
It seems to me the easiest way to progress is to merge this PR and iterate in future smaller PRs, alleviating the need for managing the significant git history in this branch or requiring a maintainer to edit this branch.
@arewm @lehors @mlieberman85 let us know if you disagree with this approach, otherwise I will plan to merge this by EOD (UTC+1) on Friday.
…nd provenance. (#1083) fixes #1072 This PR modifies _draft_ content of the slsa spec. ## Context Based on discussion from #1037 See [discussion here](https://docs.google.com/document/d/13Xt8mA_2b00McGX2vkyhu4GQdFAqtXPu7YXE8ZA6ISE/edit?resourcekey=0-EqfHF79tUWAKp4PzsE3z1A#heading=h.svjr333bawb). Copied from [draft proposal here](https://docs.google.com/document/d/13Xt8mA_2b00McGX2vkyhu4GQdFAqtXPu7YXE8ZA6ISE/edit?resourcekey=0-EqfHF79tUWAKp4PzsE3z1A#bookmark=id.4qr65cfy6ufj). Google document requires slsa-discussion@googlegroups.com membership. ## Source revision provenance Repos contain many revisions, most of which are not "official" or otherwise approved for release. The goal of the source track is to attest to why a specific revision _was_ approved for release. We can think of the SCP / code review tool as “building” the next official revision of a repository using a codified process that involves collecting commits, acquiring reviews, running CI, etc. If the change review process is successful, the code review tooling will merge the code changes and attest to the process used to produce the new revision. The source provenance attestations associate a specific revision of a repository to security claims and documents (basically build logs) of the process that produced it. In GitHub terms, a merged pull request and its associated rules evaluation justify why and how a specific git SHA is reachable from a protected branch. ## Example Scenario 1. A CI system is trying to build some artifact and will download all necessary resources, including repos and packages. 2. After download, the system will proceed to verify all fetched resources. 1. For package artifacts, it takes the hash and looks for build provenance attestations from sigstore or github. 1. For source artifacts that are not packaged (EG, cloned via git), it takes the revision id and looks for the source provenance from sigstore or github. 5. Based on the claims in the provenance attestations, the CI system can determine if all resources comply with required policy and choose to proceed. --------- Signed-off-by: Zachariah Cox <zachariahcox@github.com> Co-authored-by: Joshua Lock <joshuagloe@gmail.com> Co-authored-by: Tom Hennen <TomHennen@users.noreply.github.com>
This change adds the working draft of SLSA's Source track. It includes basic terminology, level requirements, and an attestation format.