Skip to content
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

Map GitLab OIDC token claims to Fulcio OIDs #1097

Merged
merged 1 commit into from
Apr 25, 2023
Merged

Map GitLab OIDC token claims to Fulcio OIDs #1097

merged 1 commit into from
Apr 25, 2023

Conversation

aladh
Copy link
Contributor

@aladh aladh commented Apr 5, 2023

Summary

GitLab issue: https://gitlab.com/gitlab-org/gitlab/-/issues/388517

Related to #243 and #983

Note that some of the GitLab claims added in this PR do not exist in the GitLab token yet.

Release Note

Documentation

@haydentherapper
Copy link
Contributor

Thanks for working on this! If you can fill in the MUST extensions in particular, that would be very helpful!

docs/oid-info.md Outdated
| iss | iss | iss | iss | Issuer | This already exists. For example: https://token.actions.githubusercontent.com |
| exp | exp | exp | exp | N/A | Only used to validate the JWT. |
| nbf | nbf | nbf | nbf | N/A | Only used to validate the JWT. |
| iat | iat | iat | iat | N/A | Only used to validate the JWT. |
| server_url + job_workflow_ref | ?? | ?? | ?? | Build Signer URI | Reference to specific build instructions that are responsible for signing. Can be the same as Build Config URI. For example a reusable workflow in GitHub Actions or a Circle CI Orbs. |
| server_url + job_workflow_ref | server_url + project_path + /-/blob/ + ci_commit_sha + / + ci_config_path | ?? | ?? | Build Signer URI | Reference to specific build instructions that are responsible for signing. Can be the same as Build Config URI. For example a reusable workflow in GitHub Actions or a Circle CI Orbs. |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub Actions uses the job_workflow_ref, which is a ref to the workflow being used. I think the closest approximation for GitLab would be a ref for the include for the job if applicable, otherwise if it's just in the base .gitlab-ci.yml this would be the same as the build config uri.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the existence of include complicates this a lot. A single job may have elements of its configuration altered in multiple different places. For example, this is valid behavior:

include: '/templates/build.gitlab-ci.yml'

# This job originates from build.gitlab-ci.yml
build:
  # This script keyword overrides the one from `build.gitlab-ci.yml`
  script: ./my-build.sh

Additionally, include is not versioned and it could change between multiple different CI pipeline runs. The only solution I can really think of here is to persist the merged CI yaml that was used to create the pipeline, and allow that to be viewed. Ex: https://gitlab.com/:project/-/pipelines/:id/merged_yaml

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, include is not versioned and it could change between multiple different CI pipeline runs.

@Brcrwilliams the immutability problem is not unique to us and it can't be solved in this context anyway. A workflow ref in GitHub Actions has no immutability guarantee either. Folks should be version pinning their include references by SHA if they want such guarantees. For local include references (like in your example) it doesn't even matter because the ref is the same as the root .gitlab-ci.yml pipeline definition.

We do need to account for custom CI/CD config file locations which can be set to anything including arbitrary remote HTTP endpoints. As such we may need to normalize this claim value to a fully qualified URI in all cases for consistency. We may need to manually checksum references to files that are not stored in git.

cc @wlynch

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should name the new claims pipeline_ref and pipeline_sha. For the reasons noted above, ci_config_path is not always going to be .gitlab-ci.yml and the checksum is not necessarily based on ci_commit_sha.

Also note, we will be making some pretty significant changes to pipeline definitions in the future (https://gitlab.com/gitlab-org/gitlab/-/issues/398129#note_1323114974) and may wish to change the URI format of pipeline_ref to align with the syntax of CI pipeline components. Keeping that logic for constructing that on our side is another good reason to include fully qualified URIs in the claim.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Build Signer URI (in GitLab's case our plan is to use Job ID) would still be useful for asserting identity since it is considered a unique immutable identifier for the service - though it places trust on GitLab to do the right thing and limits a client's ability to detect if the CI service misbehaves.

I agree that having a digest that can be verified independently by clients is something that CI providers should do - but I think having this requirement as MUST is a high bar that not all CI providers may be able to do right away.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not opposed to changing it to SHOULD, but I’d like to get some more input from GH, given we have discussed this as a requirement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are fine to lower this to SHOULD. Even for GitHub supporting the Build Signer Digest is a recent addition. By lowering this to SHOULD we still allow policy makers to REQUIRE this value, which I think is a good tradeoff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wlynch Can you create a separate PR to update this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course! Will do.

docs/oid-info.md Outdated
| event_name | pipeline_source | ?? | ?? | Build Trigger | The event or action that triggered the build. |
| server_url + repository + "/actions/runs/" + run_id + "/attempts/" + run_attempt | ?? | ?? | ?? | Run Invocation URI | An immutable identifier that can uniquely identify the build execution |
| server_url + repository + "/actions/runs/" + run_id + "/attempts/" + run_attempt | server_url + project_path + /-/jobs/ + job_id | ?? | ?? | Run Invocation URI | An immutable identifier that can uniquely identify the build execution |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aladh aladh marked this pull request as ready for review April 5, 2023 19:19
@aladh
Copy link
Contributor Author

aladh commented Apr 5, 2023

@Brcrwilliams @cpanato @marshall007 @wlynch could you please take a look and add any thoughts?

docs/oid-info.md Outdated
| iss | iss | iss | iss | Issuer | This already exists. For example: https://token.actions.githubusercontent.com |
| exp | exp | exp | exp | N/A | Only used to validate the JWT. |
| nbf | nbf | nbf | nbf | N/A | Only used to validate the JWT. |
| iat | iat | iat | iat | N/A | Only used to validate the JWT. |
| server_url + job_workflow_ref | ?? | ?? | ?? | Build Signer URI | Reference to specific build instructions that are responsible for signing. Can be the same as Build Config URI. For example a reusable workflow in GitHub Actions or a Circle CI Orbs. |
| server_url + job_workflow_ref | server_url + project_path + /-/blob/ + ci_commit_sha + / + ci_config_path | ?? | ?? | Build Signer URI | Reference to specific build instructions that are responsible for signing. Can be the same as Build Config URI. For example a reusable workflow in GitHub Actions or a Circle CI Orbs. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub Actions uses the job_workflow_ref, which is a ref to the workflow being used. I think the closest approximation for GitLab would be a ref for the include for the job if applicable, otherwise if it's just in the base .gitlab-ci.yml this would be the same as the build config uri.

docs/oid-info.md Outdated
| iss | iss | iss | iss | Issuer | This already exists. For example: https://token.actions.githubusercontent.com |
| exp | exp | exp | exp | N/A | Only used to validate the JWT. |
| nbf | nbf | nbf | nbf | N/A | Only used to validate the JWT. |
| iat | iat | iat | iat | N/A | Only used to validate the JWT. |
| server_url + job_workflow_ref | ?? | ?? | ?? | Build Signer URI | Reference to specific build instructions that are responsible for signing. Can be the same as Build Config URI. For example a reusable workflow in GitHub Actions or a Circle CI Orbs. |
| job_workflow_sha | ?? | ?? | ?? | Build Signer Digest | An immutable reference to the specific version of the build instructions that is responsible for signing. Should include the digest type followed by the digest, e.g. `sha1:abc123`. |
| server_url + job_workflow_ref | server_url + project_path + /-/blob/ + ci_commit_sha + / + ci_config_path | ?? | ?? | Build Signer URI | Reference to specific build instructions that are responsible for signing. Can be the same as Build Config URI. For example a reusable workflow in GitHub Actions or a Circle CI Orbs. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment for these fields - we will want these values to be authenticated, so we'll need to make sure they're included in the JWT GitLab is producing (we can't pull them from the environment).

Based on https://gitlab.com/gitlab-org/gitlab/-/issues/388517#note_1342881271 there's commitment to make this happen, but not sure how we want to handling ordering here if some of these claims don't yet exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave this PR open until https://gitlab.com/gitlab-org/gitlab/-/merge_requests/116867 is merged.

docs/oid-info.md Outdated
| iss | iss | iss | iss | Issuer | This already exists. For example: https://token.actions.githubusercontent.com |
| exp | exp | exp | exp | N/A | Only used to validate the JWT. |
| nbf | nbf | nbf | nbf | N/A | Only used to validate the JWT. |
| iat | iat | iat | iat | N/A | Only used to validate the JWT. |
| server_url + job_workflow_ref | ?? | ?? | ?? | Build Signer URI | Reference to specific build instructions that are responsible for signing. Can be the same as Build Config URI. For example a reusable workflow in GitHub Actions or a Circle CI Orbs. |
| server_url + job_workflow_ref | server_url + project_path + /-/blob/ + ci_commit_sha + / + ci_config_path | ?? | ?? | Build Signer URI | Reference to specific build instructions that are responsible for signing. Can be the same as Build Config URI. For example a reusable workflow in GitHub Actions or a Circle CI Orbs. |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the existence of include complicates this a lot. A single job may have elements of its configuration altered in multiple different places. For example, this is valid behavior:

include: '/templates/build.gitlab-ci.yml'

# This job originates from build.gitlab-ci.yml
build:
  # This script keyword overrides the one from `build.gitlab-ci.yml`
  script: ./my-build.sh

Additionally, include is not versioned and it could change between multiple different CI pipeline runs. The only solution I can really think of here is to persist the merged CI yaml that was used to create the pipeline, and allow that to be viewed. Ex: https://gitlab.com/:project/-/pipelines/:id/merged_yaml

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Merging #1097 (bd50cd7) into main (af14408) will decrease coverage by 0.18%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1097      +/-   ##
==========================================
- Coverage   55.91%   55.73%   -0.18%     
==========================================
  Files          48       48              
  Lines        2783     2783              
==========================================
- Hits         1556     1551       -5     
- Misses       1097     1101       +4     
- Partials      130      131       +1     

see 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Alishan Ladhani <aladhani@gitlab.com>
Copy link
Member

@feelepxyz feelepxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great and very excited to see this! 😍 I was OOO for the last couple weeks so just coming around to this now.

Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work on this! Just a few nits

| runner_environment | ?? | ?? | ?? | Runner Environment | For platforms to specify whether the build took place in platform-hosted cloud infrastructure or customer-hosted infrastructure. For example: `platform-hosted` and `self-hosted`. |
| server_url + repository | project_path | ?? | ?? | Source Repository URI | Should include a fully qualified repository URL. |
| sha | ?? | ?? | build_commit | Source Repository Digest | An immutable reference to a specific version of the source code. Should include the digest type followed by the digest, e.g. `sha1:abc123`. |
| server_url + job_workflow_ref | server_url + project_path + /-/jobs/ + job_id | ?? | ?? | Build Signer URI | Reference to specific build instructions that are responsible for signing. Can be the same as Build Config URI. For example a reusable workflow in GitHub Actions or a Circle CI Orbs. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| server_url + job_workflow_ref | server_url + project_path + /-/jobs/ + job_id | ?? | ?? | Build Signer URI | Reference to specific build instructions that are responsible for signing. Can be the same as Build Config URI. For example a reusable workflow in GitHub Actions or a Circle CI Orbs. |
| server_url + job_workflow_ref | server_url + project_path + "/-/jobs/" + job_id | ?? | ?? | Build Signer URI | Reference to specific build instructions that are responsible for signing. Can be the same as Build Config URI. For example a reusable workflow in GitHub Actions or a Circle CI Orbs. |

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm, you would like to include -? This differs from the GHA example, not sure if that's more in line for a GitLab URI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aladh thinking about this a bit more. Would it make more sense to set this to the same value as Build Config URI or what you'll set the SAN uri to (essentially project path I believe)? For github actions, the signer is either the custom workflow in a repo or, if using reusable workflows, the ref to the reusable workflow.

Attempting to capture some kind of signing identity that you can could write policies against, trusting certain ones for example.

How does this work in GitLab? When running a pipeline step that eventually runs npm publish --provenance, what resource or entity ends up wrapping that step in an isolated job/run? Can it be executed by entities different to the project or is it always 1:1?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@haydentherapper - yeah the /-/ is part of GitLab's URI spec.

@feelepxyz some related discussion up above - #1097 (comment)

I don't think we want to do that because the Build Config can point to a different repo or even a remote URL. The Job ID we're including here is still useful information to encode in the cert to differentiate jobs in a pipeline, since that's what's doing the signing. However, the Job ID is far to granular to be a useful SAN since it relies on a UID.

I think ideally we would use a named CI config + named job, but that's not a concept that exists in GitLab today (though @marshall007 mentioned that there are discussions happening that may lead to this in the future).

Today there's only 1 CI config per repo, which is why for the SAN we're planning to use is the repo+ref (which also covers merge requests from forks since the identity in that case would be the forked repo).
When GitLab supports multiple CI configs per repo, we can iterate on this and take that into account.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's plans to support multiple CI configs per repo in the near future, should we handle that now? Otherwise unless we proactively account for that, there will be a period where there are multiple configs and the SAN remains the same as now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marshall007 can confirm, but I don't think there's a solid timeline / commitment at the moment, so I wouldn't design around it for now.

| event_name | pipeline_source | ?? | ?? | Build Trigger | The event or action that triggered the build. |
| server_url + repository + "/actions/runs/" + run_id + "/attempts/" + run_attempt | ?? | ?? | ?? | Run Invocation URI | An immutable identifier that can uniquely identify the build execution |
| server_url + repository + "/actions/runs/" + run_id + "/attempts/" + run_attempt | server_url + project_path + /-/pipelines/ + pipeline_id | ?? | ?? | Run Invocation URI | An immutable identifier that can uniquely identify the build execution |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| server_url + repository + "/actions/runs/" + run_id + "/attempts/" + run_attempt | server_url + project_path + /-/pipelines/ + pipeline_id | ?? | ?? | Run Invocation URI | An immutable identifier that can uniquely identify the build execution |
| server_url + repository + "/actions/runs/" + run_id + "/attempts/" + run_attempt | server_url + project_path + "/-/pipelines/" + pipeline_id | ?? | ?? | Run Invocation URI | An immutable identifier that can uniquely identify the build execution |

| repository_owner_id | namespace_id | ?? | ?? | Source Repository Owner Identifier | Stable identifier for the owner of the source repository. |
| server_url + workflow_ref | ?? | ?? | ?? | Build Config URI | A reference to the initiating build instructions. |
| workflow_sha | ?? | ?? | ?? | Build Config Digest | An immutable reference to the specific version of the top-level build instructions. Should include the digest type followed by the digest, e.g. `sha1:abc123`. |
| server_url + workflow_ref | pipeline_ref | ?? | ?? | Build Config URI | A reference to the initiating build instructions. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm, this is fully qualified with a URL?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to approve - There's just two open nits to change to ", but that's very small. Any more comments @wlynch @priyawadhwa @aladh?

Thanks so much for working on this!!

@priyawadhwa
Copy link
Contributor

This LGTM, I'll go ahead and merge. @aladh thank you for your work on this! would you mind addressing the nits in a follow-up PR? Thank you!

@priyawadhwa priyawadhwa merged commit 438cd83 into sigstore:main Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants