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.
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
feat: Add GitLab CI provenance #6375
feat: Add GitLab CI provenance #6375
Changes from all commits
030956b
d22dcee
0830de9
cabf14f
19bbf76
47c544d
2806562
3853fbc
8b1257d
3bb9bfb
cd8a88d
fcf02f9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Looking at the claims mapped here, do you have access to the
CI_RUNNER_ENVIRONMENT
here so we could use that instead of the id? Or is id the env? This would make it easier to write policies trust only thegitlab-hosted
runner for exampleThere 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.
Also should this be per project or just set to the server url runner to make it easier to have some kind of stable identifier for all gitlab.com hosted runners for example?
?
env.CI_SERVER_URL/-/runners/${env. CI_RUNNER_ENVIRONMENT}
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.
Looking at https://github.com/in-toto/attestation/blob/v0.1.0/spec/predicates/provenance.md#fields, I don't think that the runner id is an appropriate value to use here.
The runner id identifies the specific machine, and we cycle through machines quite often. For gitlab-hosted runners, we probably want this to be a generic value that denotes the runner is hosted by GitLab. For self-hosted runners, we probably want some way to map this back to the instance, group, or project that is hosting the runner.
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 generally in favor, but I think there's some desire to have this match the existing GitLab provenance document, which uses CI_RUNNER_ID (see https://gist.github.com/wlynch/c7fd8f53adc77d3c0ec82356e4d43cb5 for an example provenance doc I pulled that I used as the basis for this).
This is a common theme for why fields are the way they are for this provenance document.
I think my preference would to start with parity with the existing GitLab SLSA v0.2 spec, and I can sync with @Brcrwilliams @marshall007 and other GitLab folks to how we want this to change this moving forward.
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.
The docs say
builder.id
defines the "trust base that consumers care about". I can't say too much about GitLab without more knowledge of that platform, but generally I think this should identify something like "GitLab hosted runner" or "GitLab custom runner". I don't think the runner ID will identify a significant difference in the "trust base"./cc @MarkLodato @kpk47
However, wrt the format overall maybe this is best taken up with the GitLab Supply Chain Security working group of which I think @kpk47 is a member.
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.
Keeping parity with the existing GitLab SLSA v0.2 spec makes sense to me. Sounds like it would also be useful to start an effort to figure out how the 1.0 spec should look for GitLab and get this included in the slsa org.
Before you can actually test this against the npm registry, we'll need to update the server side checks to accept gitlab provenance statements. There's currently a simple allow list of issuers to allow us to gradually roll out support, as well as checks to make sure the extensions in the signing cert matches what's in the provenance statement.
@wlynch to aid with registry integration, could you include a complete
.sigstore
bundle files for GitLab? It's a bit annoying getting the sigstore bundle at the moment, but you could modify this line and write the file to disc during a pipeline run:cli/workspaces/libnpmpublish/lib/publish.js
Line 185 in a558bbd
Ideally we would also have the fulcio certificate updated to include all the new gitlab claims but can start without this.
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 already brought it up in the WG. 🙂 There's general interest in working towards improving the existing SLSA v0.2 spec + moving to v1.0. We can follow up there.
I'll do this as soon as sigstore/fulcio#983 goes live!
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.
One additional problem of using
builder: { id: ${env.CI_PROJECT_URL}/-/runners/${env.CI_RUNNER_ID}}
as builder.id is that every project has a different builder. This kinda defeats the purpose of SLSA which scales by having a small set of builders, which you can then use for policy enforcement during verification. The builder.id, as written in this PR, would need to be generated dynamically for verification, even though the builders are technically the same (for GitLab hosted, I mean). See a longer discussion in slsa-framework/slsa#655.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.
@wlynch could we add
pipeline_ref
andpipeline_sha
that are now in the claims? Or are these in here but with different names?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.
Ah I see these are still in progress: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/117923
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.
pipeline_ref is populated in
environent.pipeline.ref
viaCI_CONFIG_PATH
.pipeline_sha
is WIP on the GitLab side and may not always be populated (i.e. if being fetched from remote sources) - I've raised with GitLab folks. The safer thing to key off of for provenance checking is pipeline id / job id. I don't think this is blocking for this PR.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.
@wlynch
pipeline_ref
is actually a URI that links to the pipeline configuration. The name is easy to confuse with a git ref.@marshall007 @aladh Maybe we should call it something else like
pipeline_config_uri
andpipeline_config_sha
.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.
@wlynch
pipeline_ref
is actually a URI that links to the pipeline configuration. The name is easy to confuse with a git ref.@marshall007 @aladh Maybe we should call it something else like
pipeline_config_uri
andpipeline_config_sha
.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.
That's what we want. 🙂
What we're ultimately looking for is a hook to the Fulcio cert Build Config URI to match for verification.
No objections to renaming though.