Update GitLab claim mappings for build configs #1206

marshall007 · 2023-06-02T18:06:16Z

Summary

Assigns new pipeline_ref/sha claims to Build Config and Build Signer related OIDs.

Note this depends on addition of new claims upstream, which should land in GitLab v16.1: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121597

Related to discussion in #1182

Release Note

Documentation

Assigns new `pipeline_ref/sha` claims to `Build Config` and `Build Signer` related OIDs. Depends on https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121597 Related to sigstore#1182 Signed-off-by: marshall007 <[email protected]>

marshall007 · 2023-06-02T18:10:49Z

@wlynch @feelepxyz I believe this captures everything we discussed in our sync call last week:

Map pipeline_ref to Build Config URI + Build Signer URI
Map pipeline_sha to Build Config Digest + Build Signer Digest
Map job URL to Run Invocation URI (instead of the pipeline URL)

Let me know if I missed anything!

pkg/identity/gitlabcom/principal.go

aladh · 2023-06-02T19:25:57Z

Should we update the mapping table as part of this PR? https://github.com/sigstore/fulcio/blob/main/docs/oid-info.md#mapping-oidc-token-claims-to-fulcio-oids

pkg/identity/gitlabcom/principal.go

feelepxyz · 2023-06-05T13:16:29Z

@wlynch @feelepxyz I believe this captures everything we discussed in our sync call last week:

Map pipeline_ref to Build Config URI + Build Signer URI

Map pipeline_sha to Build Config Digest + Build Signer Digest

Map job URL to Run Invocation URI (instead of the pipeline URL)

Let me know if I missed anything!

@marshall007 yes this sounds right to me! Thanks for putting this together so quick. Do you have a timeline for enabling the pipeline_* feature flag once the changes are in the release?

Should we update the mapping table as part of this PR? https://github.com/sigstore/fulcio/blob/main/docs/oid-info.md#mapping-oidc-token-claims-to-fulcio-oids

👍

wlynch

Mostly LGTM pending Build Signer URI timing + doc updates.

pkg/identity/gitlabcom/principal.go

marshall007 · 2023-06-05T16:27:29Z

@marshall007 yes this sounds right to me! Thanks for putting this together so quick. Do you have a timeline for enabling the pipeline_* feature flag once the changes are in the release?

@feelepxyz I responded in #1206 (comment). Should be 16.1 which is in about two weeks.

Signed-off-by: marshall007 <[email protected]>

haydentherapper · 2023-06-05T22:16:48Z

Let's circle back in a week or two then for approving and merging this once it's in GitLab?

Signed-off-by: marshall007 <[email protected]>

marshall007 · 2023-06-26T20:14:09Z

The corresponding change on the GitLab side has been merged: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/122373

@feelepxyz @haydentherapper we started enabling the feature flag in non-production environments today and expect rollout to production gitlab.com to be completed by the end of the week.

haydentherapper · 2023-06-26T23:09:46Z

Fantastic! Can you test this against the non-production environment to confirm the certificates generated contain the expected values?

We'll get this merged once the change is out in production.

pkg/identity/gitlabcom/principal.go

docs/oid-info.md

pkg/identity/gitlabcom/principal_test.go

Signed-off-by: marshall007 <[email protected]>

feelepxyz · 2023-06-28T10:57:10Z

@haydentherapper do you have rights to approve the ci runs?

feelepxyz

LGTM 💯

aladh · 2023-06-28T20:48:32Z

@marshall007 Some changes are required in pkg/server/grpc_server_test.go to make the tests pass:

diff --git a/pkg/server/grpc_server_test.go b/pkg/server/grpc_server_test.go
index 854d87a..6b477b1 100644
--- a/pkg/server/grpc_server_test.go
+++ b/pkg/server/grpc_server_test.go
@@ -956,6 +956,8 @@ type gitlabClaims struct {
 	ProjectID         string `json:"project_id"`
 	PipelineSource    string `json:"pipeline_source"`
 	PipelineID        string `json:"pipeline_id"`
+	CiConfigRefURI    string `json:"ci_config_ref_uri"`
+	CiConfigSha       string `json:"ci_config_sha"`
 	NamespacePath     string `json:"namespace_path"`
 	NamespaceID       string `json:"namespace_id"`
 	JobID             string `json:"job_id"`
@@ -989,6 +991,8 @@ func TestAPIWithGitLab(t *testing.T) {
 		ProjectID:         "42831435",
 		PipelineSource:    "push",
 		PipelineID:        "757451528",
+		CiConfigRefURI:    "gitlab.com/cpanato/testing-cosign//.gitlab-ci.yml@refs/heads/main",
+		CiConfigSha:       "714a629c0b401fdce83e847fc9589983fc6f46bc",
 		NamespacePath:     "cpanato",
 		NamespaceID:       "1730270",
 		JobID:             "3659681386",
@@ -1051,7 +1055,7 @@ func TestAPIWithGitLab(t *testing.T) {
 		t.Fatalf("unexpected length of leaf certificate URIs, expected 1, got %d", len(leafCert.URIs))
 	}
 
-	gitLabURL := fmt.Sprintf("https://gitlab.com/%s@refs/heads/%s", claims.ProjectPath, claims.Ref)
+	gitLabURL := fmt.Sprintf("https://gitlab.com/%s//.gitlab-ci.yml@refs/heads/%s", claims.ProjectPath, claims.Ref)
 	gitLabURI, err := url.Parse(gitLabURL)
 	if err != nil {
 		t.Fatalf("failed to parse expected url")
@@ -1061,7 +1065,8 @@ func TestAPIWithGitLab(t *testing.T) {
 	}
 	url := "https://gitlab.com/"
 	expectedExts := map[int]string{
-		9:  url + claims.ProjectPath + "/-/jobs/" + claims.JobID,
+		9:  gitLabURL,
+		10: claims.CiConfigSha,
 		11: claims.RunnerEnvironment,
 		12: url + claims.ProjectPath,
 		13: claims.Sha,
@@ -1069,8 +1074,10 @@ func TestAPIWithGitLab(t *testing.T) {
 		15: claims.ProjectID,
 		16: url + claims.NamespacePath,
 		17: claims.NamespaceID,
+		18: gitLabURL,
+		19: claims.CiConfigSha,
 		20: claims.PipelineSource,
-		21: url + claims.ProjectPath + "/-/pipelines/" + claims.PipelineID,
+		21: url + claims.ProjectPath + "/-/jobs/" + claims.JobID,
 	}
 	for o, value := range expectedExts {
 		ext, found := findCustomExtension(leafCert, asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, o})

aladh · 2023-06-28T20:49:30Z

The corresponding change on the GitLab side has been merged: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/122373

@feelepxyz @haydentherapper we started enabling the feature flag in non-production environments today and expect rollout to production gitlab.com to be completed by the end of the week.

This change is now live on GitLab.com.

feelepxyz · 2023-06-29T14:55:04Z

This change is now live on GitLab.com.

Awesome! Excited to get this in.

haydentherapper · 2023-06-29T14:56:29Z

Can you check out the test failure @marshall007 ?

Signed-off-by: marshall007 <[email protected]>

marshall007 · 2023-07-07T15:28:28Z

@feelepxyz @haydentherapper sorry for the delay! I was out for the holiday weekend and got sick back-to-back. I've applied @aladh's patch and the tests are passing locally.

Fantastic! Can you test this against the non-production environment to confirm the certificates generated contain the expected values?

@haydentherapper I believe last time we merged first and then tested GitLab CI against the staging Fulcio instance. Is that an option this time around? I won't have the bandwidth to standup the required infrastructure and run a manual e2e test on my own this week or next.

If you insist I can see if others on my team can support that. I'm told these docs are a good starting point, is that right? https://github.com/sigstore/scaffolding

bobcallaway · 2023-07-08T10:49:24Z

pkg/server/grpc_server_test.go

@@ -1051,26 +1055,29 @@ func TestAPIWithGitLab(t *testing.T) {
 		t.Fatalf("unexpected length of leaf certificate URIs, expected 1, got %d", len(leafCert.URIs))
 	}

-	gitLabURL := fmt.Sprintf("https://gitlab.com/%s@refs/heads/%s", claims.ProjectPath, claims.Ref)
+	baseUrl := "https://gitlab.com/"


the linter isn't happy with baseUrl, perhaps just switch to baseURL?

I'm about to cut a new release and roll out to staging so if we can turn this around quickly it can be included in that.

Suggested change

baseUrl := "https://gitlab.com/"

baseURL := "https://gitlab.com/"

haydentherapper

@marshall007 Just one question and a linter failure.

If you could update the linter failure, I'll get this merged and we'll cut a new release to test in staging.

pkg/identity/gitlabcom/principal_test.go

Signed-off-by: marshall007 <[email protected]>

codecov · 2023-07-10T15:02:41Z

Codecov Report

Merging #1206 (83dcc1f) into main (d2286c6) will increase coverage by 0.15%.
The diff coverage is 73.91%.

❗ Current head 83dcc1f differs from pull request most recent head 7fe4f31. Consider uploading reports for the commit 7fe4f31 to get more accurate results

@@            Coverage Diff             @@
##             main    #1206      +/-   ##
==========================================
+ Coverage   56.18%   56.34%   +0.15%     
==========================================
  Files          50       50              
  Lines        2903     2941      +38     
==========================================
+ Hits         1631     1657      +26     
- Misses       1129     1138       +9     
- Partials      143      146       +3

Impacted Files	Coverage Δ
pkg/identity/gitlabcom/principal.go	`63.49% <73.91%> (+1.22%)`	⬆️

... and 3 files with indirect coverage changes

Update GitLab claim mappings for build configs

2e6c0aa

Assigns new `pipeline_ref/sha` claims to `Build Config` and `Build Signer` related OIDs. Depends on https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121597 Related to sigstore#1182 Signed-off-by: marshall007 <[email protected]>

marshall007 force-pushed the gitlab_pipeline_ref branch from e4ac946 to 2e6c0aa Compare June 2, 2023 18:07

aladh reviewed Jun 2, 2023

View reviewed changes

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

cpanato requested review from wlynch and haydentherapper June 5, 2023 08:15

feelepxyz reviewed Jun 5, 2023

View reviewed changes

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

wlynch mentioned this pull request Jun 5, 2023

Merge GitLab provenance generation into latest npm/cli#6525

Closed

wlynch suggested changes Jun 5, 2023

View reviewed changes

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

update docs to reflect GitLab claim mappings

b95832b

Signed-off-by: marshall007 <[email protected]>

feelepxyz approved these changes Jun 6, 2023

View reviewed changes

aladh mentioned this pull request Jun 11, 2023

Federate with Arch Linux's GitLab instance #1214

Merged

wlynch mentioned this pull request Jun 13, 2023

feat: Add GitLab CI provenance (#6375) npm/cli#6526

Merged

marshall007 added 2 commits June 26, 2023 15:09

updates to match new claim names

9793078

Signed-off-by: marshall007 <[email protected]>

fix issuer test

e7d8915

Signed-off-by: marshall007 <[email protected]>