Add GitLab.com OIDC to Fulcio #983

cpanato · 2023-01-27T10:31:11Z

Summary

Add GitLab.com OIDC to Fulcio

Initial work to support gitlab.com, for self-hosted we will need to discuss how we want to deal with it.
similar work that was done in #890

sample gitlab pipeline

docker-build:
  # Use the official docker image.
  image: docker:latest
  stage: build
  variables:
    COSIGN_EXPERIMENTAL: "true"

  id_tokens:
    SIGSTORE_JWT:
      aud: "sigstore"
  before_script:
    - docker login -u "$CI_REGISTRY_USER" -p "$CI_REGISTRY_PASSWORD" $CI_REGISTRY
    - apk add --update curl && rm -rf /var/cache/apk/*
    - curl -L https://github.com/sigstore/cosign/releases/download/v1.13.1/cosign-linux-amd64  -o cosign
    - chmod +x cosign
    - mv cosign /usr/bin
  script:
     - cosign sign --identity-token=${SIGSTORE_JWT} registry.gitlab.com/cpanato/testing-cosign:main

sample token payload

{
  "namespace_id": "1730270",
  "namespace_path": "cpanato",
  "project_id": "42831435",
  "project_path": "cpanato/testing-cosign",
  "user_id": "1430381",
  "user_login": "cpanato",
  "user_email": "ctadeu@example.com",
  "pipeline_id": "757451528",
  "pipeline_source": "push",
  "job_id": "3659681386",
  "ref": "main",
  "ref_type": "branch",
  "ref_protected": "true",
  "jti": "914910cc-09f6-4217-8091-a1d3231a37db",
  "iss": "https://gitlab.com",
  "iat": 1674658264,
  "nbf": 1674658259,
  "exp": 1674661864,
  "sub": "project_path:cpanato/testing-cosign:ref_type:branch:ref:main",
  "aud": "sigstore"
}

Fixes #243

codecov-commenter · 2023-01-27T10:40:31Z

Codecov Report

Merging #983 (213c563) into main (be32ddb) will increase coverage by 0.11%.
The diff coverage is 62.80%.

@@            Coverage Diff             @@
##             main     #983      +/-   ##
==========================================
+ Coverage   55.91%   56.02%   +0.11%     
==========================================
  Files          48       50       +2     
  Lines        2783     2904     +121     
==========================================
+ Hits         1556     1627      +71     
- Misses       1097     1133      +36     
- Partials      130      144      +14

Impacted Files	Coverage Δ
pkg/challenges/challenges.go	`37.50% <0.00%> (-1.64%)`	⬇️
pkg/server/grpc_server.go	`51.70% <ø> (ø)`
pkg/identity/gitlabcom/issuer.go	`62.50% <62.50%> (ø)`
pkg/identity/gitlabcom/principal.go	`62.61% <62.61%> (ø)`
pkg/config/config.go	`70.03% <100.00%> (+0.21%)`	⬆️
pkg/server/issuer_pool.go	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

cpanato · 2023-01-27T12:17:36Z

this is a payload from a self-hosted instance (changed the gitlab url for security) but that pointed to the base url for the self hosted instance

{
  "namespace_id": "4",
  "namespace_path": "cpanato",
  "project_id": "10",
  "project_path": "cpanato/testing-cosign",
  "user_id": "2",
  "user_login": "cpanato",
  "user_email": "cpanato@example.dev",
  "pipeline_id": "322",
  "pipeline_source": "push",
  "job_id": "1139",
  "ref": "main",
  "ref_type": "branch",
  "ref_protected": "true",
  "jti": "f94ba179-77b2-4b3f-b179-b2df0698ed70",
  "iss": "https://xxxxxxxxx",
  "iat": 1674821675,
  "nbf": 1674821670,
  "exp": 1674825275,
  "sub": "project_path:cpanato/testing-cosign:ref_type:branch:ref:main",
  "aud": "sigstore"
}

lukehinds · 2023-01-27T22:19:38Z

pkg/identity/gitlabcom/principal_test.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package gitlabcom


Any reason for not just having gitlab as the package name without the TLD (like github and buildkite)?

Maybe to make it clear this is scoped to gitlab.com (the hosted instance) for now and custom gitlab deployments are not supported (yet)?

we can rename that, did this in the beginning because i don't know how we want to deal with GitLab self-hosted instances

All the tokens should have the same shape, just like Kubernetes tokens, but the issuer will vary.

haydentherapper · 2023-01-28T08:13:05Z

Will take a closer look on Monday, thanks for adding this!

marshall007 · 2023-02-01T17:57:57Z

@cpanato thanks for working on this! As an FYI we are likely going to stand up a separate OIDC issuer for CI job tokens. I'm hoping we can get that done very soon but we have not established a timeline just yet. Feel free to chime in on the discussion here: https://gitlab.com/groups/gitlab-org/-/epics/9212#note_1256006253

I think we can proceed with integrating our current issuer, but we should be mindful that the issuer claim embedded in the certs is subject to change.

haydentherapper · 2023-02-01T23:54:51Z

pkg/identity/gitlabcom/principal.go

+	return &jobPrincipal{
+		subject:        token.Subject,
+		issuer:         token.Issuer,
+		url:            fmt.Sprintf("https://gitlab.com/%s/-/jobs/%s", claims.ProjectPath, claims.JobID),


I wonder if there is a better stable identifier to represent the certificate identity. From #945, we want the SAN to be a "reference to specific build instructions that are responsible for signing". Is the job ID too granular? What are the other claims available?

cc @asraa

I’ve been told that jobID is likely the best option because it most closely maps to something like the job workflow ref from GH

I agree Job ID is closest to job_workflow_ref, but I don't know if it fits the same intent for the SAN. Thinking about it's use in cosign verify --certificate-identity, SANs map more closely to a service account identity rather than a specific identifier of build instructions.

If we used the Job ID I suspect most users would use --certificate-identity-regexp="https://gitlab.com/my/repo/.*" and match on other values instead - it would be very rare that you would want to match on exact Job IDs because they are a unique identifier per run.

I think https://gitlab.com/org/repo@ref probably makes the most sense right now. @marshall007 mentioned that this may become more granular in the future, but for now GitLab CI pipelines only have a single config per repo.

cc @aladh @marshall007 Any opinions here?

haydentherapper · 2023-02-01T23:56:20Z

pkg/identity/gitlabcom/principal.go

+		PipelineID     string `json:"pipeline_id"`
+		PipelineSource string `json:"pipeline_source"`
+		JobID          string `json:"job_id"`
+		Ref            string `json:"ref"`


If the claim is not going to be used, we can omit it from the struct.

haydentherapper · 2023-02-02T00:03:43Z

@marshall007 Thanks for the information!

If it would be helpful to start testing integrations with GitLab-bound certificates, then I would be fine merging this in with a warning that the issuer will change in the near future, and we would only roll this out to staging, not production. If this is not yet doing to be used, then I'd hold off on this PR until #945 is in and the issuer has been stood up for GitLab.

cpanato · 2023-02-08T15:08:38Z

@marshall007 @haydentherapper, thanks for the updates and clarification. I will keep my eye on the GitLab issue.

@haydentherapper should we pause this or should I make the changes you requested and move forward?

mattmoor · 2023-02-08T15:36:44Z

Once this lands, we should get ambient cred support into cosign: #890 (comment)

haydentherapper · 2023-02-23T18:58:54Z

Sorry for the delay, @cpanato I think we should pause until GitLab has chosen its new issuer.

@mattmoor, yes, let's track that in Cosign.

cpanato · 2023-02-24T10:25:37Z

@haydentherapper sure thing

haydentherapper · 2023-02-28T21:44:31Z

Moving this to draft for now

haydentherapper · 2023-03-22T17:27:53Z

Does anyone in this thread know if there's been any progress on standing up a separate issuer?

cc @wlynch

aladh · 2023-03-22T18:45:41Z

@haydentherapper Unfortunately, there doesn't seem to have been any progress on the new OIDC issuer. Here's the issue for reference: https://gitlab.com/gitlab-org/gitlab/-/issues/390711. I'll try to follow up internally and find out when it should be ready.

Brcrwilliams · 2023-04-03T16:14:15Z

@cpanato @haydentherapper Hey all. I'm sorry that roadblocks got thrown up here and slowed the momentum on this. After some discussion, we've decided that we won't be going through with the changes proposed in https://gitlab.com/gitlab-org/gitlab/-/issues/390711. Please let me know if there's anything GitLab can do to help get this PR finished. We're happy to lend a hand in this effort.

haydentherapper · 2023-04-05T15:38:18Z

Not a blocker for us if you’d prefer to reuse the issuer!

We will want to include the newly added extensions in this PR too. I see an open PR being worked on now to update the mapping between extension and value, we would appreciate any feedback there since y’all have the most up to date view of the purpose of each identity token claim.

priyawadhwa

LGTM

haydentherapper · 2023-04-27T16:18:27Z

@wlynch Can you take a pass over this and make sure this looks good? I'll take a look after!

wlynch

Looks great! Thanks @cpanato! 🎉

wlynch · 2023-04-27T17:26:34Z

pkg/identity/gitlabcom/principal_test.go

+
+// reflect hack because "claims" field is unexported by oidc IDToken
+// https://github.com/coreos/go-oidc/pull/329
+func withClaims(token *oidc.IDToken, data []byte) {


Out of scope for this PR, but we should look into pulling this out into a library so we're not copying the same hacks for each identity provider. 🤔

sounds good, i can work on that in a follow up

haydentherapper · 2023-04-28T00:20:03Z

pkg/identity/gitlabcom/principal.go

+	}
+
+	// Set workflow ref URL to SubjectAlternativeName on certificate
+	cert.URIs = []*url.URL{baseURL.JoinPath(p.repository, "/-/jobs/", p.jobID)}


@wlynch Is this correct for the SAN? I recall you said that job was too granular. pipeline_ref? Or something else?

Good catch - you're right. This should be

Suggested change

cert.URIs = []*url.URL{baseURL.JoinPath(p.repository, "/-/jobs/", p.jobID)}

cert.URIs = []*url.URL{baseURL.JoinPath(p.repository, "@", ref)}

Is pipeline ref too granular? That represents the build configuration, but I could imagine creating policy around that.

If not, for ref, i think we’d need a different delimiter, unless @ creates a valid uri

@ can be a valid URI (i.e. ssh://user@host - we also use it in the GitHub Actions SAN)

re pipeline_ref: the pipeline_ref can be a URI to a location not even on GitLab which makes me a little doubtful of whether we want to use that as the SAN.
Most pipeline_refs will be just .gitlab-ci.yml, which means most SANs will be identical unless we transform them. If we do something like prepend the project URL then it becomes harder to distinguish whether the SAN was originally given to us as a URI.

As an example, if you had a repo using a config fetched from http://example.com, I think my preference would be to use https://gitlab.com/user/repo@refs/heads/main rather than http://example.com, since knowing it came from the repo from a trusted branch seems like a more useful base identifier. You'd still have the pipeline_ref in another claim as a way to have finer-grained policies.

That said, we could use it if we really wanted. What do you think? Happy to talk this through more.

LGTM on @ in the SAN.

Hm, I definitely agree that if pipeline_ref points to a URI outside of gitlab, then we should avoid that. I'm good with this then. Thanks for the background!

haydentherapper · 2023-04-28T00:21:57Z

pkg/identity/gitlabcom/principal.go

+
+	var ref string
+	switch claims.RefType {
+	case "branch":


Generally we don't want to mutate any claims, thoughts on removing this? I saw the comment from @wlynch though. If you think this would be good to standardize on, let's first open a PR to discuss requiring this prefix.

The problem is without the prefix ref alone is ambiguous - you could have ref/tags/main or ref/heads/v1.0.0. ref_type is what informs what the full-formed ref is. Ideally it would be better if GitLab had a full_ref claim (this way they could also support non branch/tag refs as well), but for now I think this is the best we can do.

cc @marshall007 @Brcrwilliams FYI

Given that ref type comes from the token, this looks alright.

Ideally it would be better if GitLab had a full_ref claim

@wlynch we can certainly do that: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/119075

In the code, we currently refer to this as the ref_path. Do you think that makes sense as a user-facing name?

That sounds fine. Thanks! 🚀

Thanks y'all! So we'll assume that ref_path will be equivalent to what's here and will update once it's implemented.

Should we wait for the MR on the GitLab side to be merged or we stick with this implementation for now?

I think it's fine to proceed. The result should be the same in the end for branches/tags.

pkg/identity/gitlabcom/principal.go

haydentherapper

LGTM, @cpanato, if you can apply the suggested change, we'll be good to go!

I'll cut a new release early next week and get the change out in staging.

Brcrwilliams · 2023-04-28T17:15:38Z

pkg/identity/gitlabcom/principal.go

+	return &jobPrincipal{
+		subject:           token.Subject,
+		issuer:            token.Issuer,
+		url:               `https://gitlab.com/`,


The comment for this field says:

// The URL of the pipeline, the container of many builds. This will be // set as a human-friendly SubjectAlternativeName URI in the certificate. url string

To make that comment accurate, this should be

Suggested change

url: `https://gitlab.com/`,

url: fmt.Sprintf("https://gitlab.com/%s/-/pipelines/%s", claims.ProjectPath, claims.ProjectID)

We could also add another claim for this in the JWT if need be.

URL is just the base URL, Embed takes the URL and appends that.

We should update the comment to note that (a previous iteration of Fulcio had the URL as the SAN, some refactoring changed that). Thanks for noticing.

We do the same in GH - https://github.com/sigstore/fulcio/blob/main/pkg/identity/github/principal.go#L187-L188, and the URL comment https://github.com/sigstore/fulcio/blob/main/pkg/identity/github/principal.go#L37

Brcrwilliams · 2023-04-28T17:29:59Z

pkg/identity/gitlabcom/principal.go

+		url:               `https://gitlab.com/`,
+		eventName:         claims.PipelineSource,
+		pipelineID:        claims.PipelineID,
+		repository:        claims.ProjectPath,


In GitLab, we distinguish between projects and repositories. https://gitlab.com/gitlab-org/gitlab is a URL to a project, https://gitlab.com/gitlab-org/gitlab.git is a URL to a repository. A project contains a repository, along with other things like issues, merge requests, pipelines, etc.

TL;DR: project is the correct thing to call this, but repository is ok if we want to keep consistency with the other principals.

wlynch · 2023-05-01T13:24:26Z

pkg/identity/gitlabcom/principal.go

+
+	var ref string
+	switch claims.RefType {
+	case "branch":


I think it's fine to proceed. The result should be the same in the end for branches/tags.

pkg/identity/gitlabcom/principal.go

Signed-off-by: cpanato <ctadeu@gmail.com>

haydentherapper

Thanks @cpanato!

priyawadhwa

LGTM!

cpanato requested review from mattmoor, bobcallaway, asraa and haydentherapper January 27, 2023 10:31

cpanato force-pushed the gitlabcom branch from 21d3af4 to 572b7c8 Compare January 27, 2023 10:35

lukehinds reviewed Jan 27, 2023

View reviewed changes

priyawadhwa requested a review from wlynch January 28, 2023 19:54

This was referenced Jan 31, 2023

Support ambient credential detection sigstore/sigstore-python#31

Closed

Add support for GitLab tokens #243

Closed

haydentherapper reviewed Feb 1, 2023

View reviewed changes

haydentherapper marked this pull request as draft February 28, 2023 21:44

wlynch mentioned this pull request Apr 4, 2023

Environment variable ID token detection sigstore/cosign#2863

Closed

aladh mentioned this pull request Apr 5, 2023

Map GitLab OIDC token claims to Fulcio OIDs #1097

Merged

cpanato force-pushed the gitlabcom branch 2 times, most recently from 62c55c5 to d757969 Compare April 5, 2023 15:27

cpanato force-pushed the gitlabcom branch from d757969 to ac1c6c4 Compare April 6, 2023 08:16

cpanato force-pushed the gitlabcom branch 4 times, most recently from ca97df4 to e8eac6d Compare April 27, 2023 12:11

cpanato marked this pull request as ready for review April 27, 2023 12:19

cpanato requested review from wlynch and priyawadhwa April 27, 2023 12:19

priyawadhwa previously approved these changes Apr 27, 2023

View reviewed changes

wlynch previously approved these changes Apr 27, 2023

View reviewed changes

haydentherapper requested changes Apr 28, 2023

View reviewed changes

haydentherapper reviewed Apr 28, 2023

View reviewed changes

Brcrwilliams reviewed Apr 28, 2023

View reviewed changes

cpanato dismissed stale reviews from wlynch and priyawadhwa via dff2b0c April 29, 2023 14:37

cpanato force-pushed the gitlabcom branch 2 times, most recently from dff2b0c to 58353d4 Compare April 29, 2023 14:39

cpanato requested review from wlynch and haydentherapper April 29, 2023 14:56

wlynch previously approved these changes May 1, 2023

View reviewed changes

haydentherapper reviewed May 1, 2023

View reviewed changes

pkg/identity/gitlabcom/principal.go Outdated Show resolved Hide resolved

Add GitLab.com OIDC to Fulcio

213c563

Signed-off-by: cpanato <ctadeu@gmail.com>

cpanato dismissed wlynch’s stale review via 213c563 May 1, 2023 14:24

cpanato force-pushed the gitlabcom branch from 58353d4 to 213c563 Compare May 1, 2023 14:24

haydentherapper approved these changes May 1, 2023

View reviewed changes

wlynch approved these changes May 1, 2023

View reviewed changes

cpanato requested a review from priyawadhwa May 1, 2023 14:43

priyawadhwa approved these changes May 1, 2023

View reviewed changes

priyawadhwa merged commit 2714dd5 into sigstore:main May 1, 2023

cpanato deleted the gitlabcom branch May 1, 2023 18:02

	cert.URIs = []*url.URL{baseURL.JoinPath(p.repository, "/-/jobs/", p.jobID)}
	cert.URIs = []*url.URL{baseURL.JoinPath(p.repository, "@", ref)}

	url: `https://gitlab.com/`,
	url: fmt.Sprintf("https://gitlab.com/%s/-/pipelines/%s", claims.ProjectPath, claims.ProjectID)

Add GitLab.com OIDC to Fulcio #983

Add GitLab.com OIDC to Fulcio #983

Conversation

cpanato commented Jan 27, 2023 • edited by haydentherapper Loading

Summary

codecov-commenter commented Jan 27, 2023 • edited by codecov bot Loading

Codecov Report

cpanato commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper commented Jan 28, 2023

marshall007 commented Feb 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper commented Feb 2, 2023 • edited Loading

cpanato commented Feb 8, 2023

mattmoor commented Feb 8, 2023

haydentherapper commented Feb 23, 2023

cpanato commented Feb 24, 2023

haydentherapper commented Feb 28, 2023

haydentherapper commented Mar 22, 2023

aladh commented Mar 22, 2023

Brcrwilliams commented Apr 3, 2023

haydentherapper commented Apr 5, 2023

priyawadhwa left a comment

Choose a reason for hiding this comment

haydentherapper commented Apr 27, 2023

wlynch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brcrwilliams Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

priyawadhwa left a comment

Choose a reason for hiding this comment

cpanato commented Jan 27, 2023 •

edited by haydentherapper

Loading

codecov-commenter commented Jan 27, 2023 •

edited by codecov bot

Loading

haydentherapper commented Feb 2, 2023 •

edited

Loading

Brcrwilliams Apr 28, 2023 •

edited

Loading

haydentherapper Apr 28, 2023 •

edited

Loading