-
Notifications
You must be signed in to change notification settings - Fork 139
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
Standardizing Fulcio Certificate Extensions #945
Standardizing Fulcio Certificate Extensions #945
Conversation
Proposal for standardizing Fulcio's Certificate Extensions to align with the discussion on [standardizing OIDC token claims](sigstore#754) across CI/CD systems (today GitHub Actions, in future Circle, GitLab, Buildkite etc). The aim here is to find new platform agnostic extensions (as the current ones are GitHub specific) that make sense across the different CI/CD providers that we'd like to see supported in Fulcio. I've preemptively moved the existing oid info doc to a deprecated version with little thought into how this transition should happen in practice. This should probably be scoped in a lot more detail. Signed-off-by: Philip Harrison <[email protected]>
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.
Philip, thank for summarizing everything! IMO the specification looks good and CI providers should be able to comply with ease. Left a few questions that was not clear to me.
Thanks so much for starting this proposal! I'll take a look at it in a little bit, cc'ing a few other folks who have worked on verification or CI that might have some thoughts. |
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 a little hesitant on the value of trying to massage all the various CI provider claims into to the same OID extensions. Have we considered giving each CI provider a different OID prefix (e.g 1.3.6.1.4.1.57264.1.12.xxx for all GitHub Actions, 1.3.6.1.4.1.57264.1.13.xxx for Gitlab etc) so that each provider can encode the claims that might be of value when writing policy without the need to make them fit the same abstraction?
Hello! I work with @feelepxyz and with @steiza 🙂.
On our team, we've been talking a lot about the verification and validation of different attestations. For us, these specific fields being proposed have emerged as requirements for building the provenance guarantees we're interested in providing. By providing a wider surface area of standardized claims embedded in the certs, different CI providers can immediately integrate with, well, what GitHub is going to be shipping.
I can see the utility of fulcio automatically embedding, well, every field in the token in the certificate. That would make it easier to extend in the future 🤔. |
Codecov Report
@@ Coverage Diff @@
## main #945 +/- ##
==========================================
+ Coverage 53.69% 53.96% +0.26%
==========================================
Files 37 38 +1
Lines 2326 2424 +98
==========================================
+ Hits 1249 1308 +59
- Misses 986 1018 +32
- Partials 91 98 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@nsmith5 echoing what @phillmv said. Definitely an option that's worth considering. I don't yet know how feasible it will be to align across CI/CD providers on this approach but seems worth trying to see what shakes out. From GitHub's perspective, we want these extensions in the signing certificate to support two uses-cases:
These uses-cases could be achieved by adding a few GitHub specific extensions in the Fulcio cert (Build Config URI/Digest. Build Signer URI/Digest and Runner Environment). The downside of having custom extensions for each provider would mainly fall on the validator having to know which fields each provider is using for what. Landing on some kind of standard here would simplify the work of downstream validators, but worth calling out that we would still risk leaky abstractions as each provider will encode things differently. |
Just came across this merged PR in Cosign from @developer-guy that would be affected by this change: https://github.com/sigstore/cosign/pull/1626/files These commands would have to change if we standardized these cert extensions. We'll want to write both for some period while we update Cosign and give users time to upgrade. |
@feelepxyz To avoid breaking changes, I would propose that Fulcio include duplicate extension values (same value in two OIDs) for some time before removing the older extensions. This follows the deprecation policy. This should be fine, these extensions are very small in size. We'll keep these around for 6 months or a year, then remove them in a 2.0 release of Fulcio. This lets us update Cosign in parallel without breaking current users or other clients. Edit: I’ll also use this opportunity to properly encode the extensions as nested ASN.1. |
<!-- References --> | ||
## Mapping OIDC token claims to Fulcio OIDs | ||
|
||
| GitHub [(docs)][github-oidc-doc] | GitLab | CircleCI | Buildkite | Fulcio Certificate Extension | Why / Notes / Questions | |
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.
For ref, current workload OIDC claims:
- Buildkite: Add Buildkite OIDC to Fulcio #890 (comment)
- Circle: https://circleci.com/docs/openid-connect-tokens/#format-of-the-openid-connect-id-token
- Gitlab: https://docs.gitlab.com/ee/ci/cloud_services/index.html#how-it-works
- GitHub (outdated): https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/about-security-hardening-with-openid-connect#understanding-the-oidc-token
docs/oid-info.md
Outdated
| repository_id | project_id | ?? | ?? | Source Identifier | Stable identifier for the owner of the source repository. | | ||
| repository_owner | namespace_path | ?? | ?? | Source Owner URI | Fully qualified URL for the owner of the source repository. | | ||
| repository_owner_id | namespace_id | ?? | ?? | Source Owner Identifier | Stable identifier for the owner of the source repository. | |
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.
Q: For providers like Buildkite and Circle who don't also host the source, should they forward the underlying source/source owner ids in their workload identity? How do we track the "build identity" that executed the build and distinguish this from the source identity? Is the build config URI enough?
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.
Someone more familiar with those CI services can probably answer better than me, but my expectation is that repository_id
would be the be the "project" or "pipeline" ID on Buildkite or Circle, while repository_owner_id
would be the ID of the User
or other owner model that configured the pipeline.
That's contingent on these services having some kind of underlying relational design that looks roughly like the following:
User(id, ...) from table of USERS
Project(id, user_id_roles) from table of PROJECTS
...but I think that's probably a reasonable assumption 🙂
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.
Yeah would be good to get some clarity here. I was imaging Source *
extensions would all point to the source repository. So if you use Circle to build with code on GitHub, Source URI would point to the github repo. Ref, digest would also point to the repo.
The reason we would want Source Owner Identifier and Source Identifier would be to spot transfers/resurrected repositories, in this example this would be the GitHub repo. Given this, I think we actually want Circle et al to provide forward repository_id
and repository_owner_id
(for GH) in their OIDC claims.
Build Config URI would presumably, in most cases, point to a build config file in the same repo.
The Builder Signer URI, could in Circle's case be a Circle Orb if one is used, otherwise this would point to the same above build config uri if one was not used.
Given this, there's actually no space in the current extensions for any build system identifiers like "project" or "pipeline" ID on Buildkite or Circle 🤔
Question: do we need something specific here to track separate build system/build owner identifiers when this differs from the source system?
cc @haydentherapper what are your thoughts on above?
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.
We should have a way to differentiate CI platforms, that should be a part of what’s validated by the verifier. Otherwise I could imagine a threat vector where an attacker creates a project on a different platform matching all of the project/repo details, but changing the source.
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.
We should have a way to differentiate CI platforms, that should be a part of what’s validated by the verifier.
This makes sense, is this not already covered by the OIDC issuer extension?
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.
Yes this is! OIDC issuer is sufficient
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.
but my expectation is that
repository_id
would be the be the "project" or "pipeline" ID on Buildkite or Circle, whilerepository_owner_id
would be the ID of theUser
or other owner model that configured the pipeline.
I am familiar with Buildkite. Reading this spec, I would assume any OID mentioning "Source" or "Repository" would be some pointer back to, or meta data about the GitHub (in my case) repository and commit/ref that triggered the build.
And I'd expect Run Invocation URI
to be something like https://buildkite.com/mycompany/mypipeline/builds/118
And my issuer (Buildkite) would be trusted to set the appropriate fields related to the source repository.
@feelepxyz Is there value in a hybrid approach, or would this be too confusing. Why not have standard fields that the downstream validator should always expect, and map the other claims with the appropriate CI prefix. e.g. |
@nkreiger Yeah first thought is that this would be confusing and unwieldy in the long term but I can also see some value here in case you want to write custom validators that check everything in a provenance statement against the Fulcio cert for example. Do you have a specific use case in mind? |
@feelepxyz That was pretty much my use case. No matter how many claims are added now, there will always be more that someone wants as things evolve. I see flexibility over complexity here maybe being stronger in the long-term. And simplifying it with required/guaranteed claims (the minimum to support a provider) that are well documented, might be able to keep the complexity lower. That being said, I'm no expert, and have to do more due diligence to support my claims (pun intended). :) |
@nkreiger @feelepxyz similar ideas has been discussed in this issue: #624 IMHO the set of extensions in the certificate should be fairly small, and have "stable" values that can be used to identify the signer, i.e not something like a runner id. Values that are expected to change between each run or just have a more "dynamic" nature is a much better fit for the provenance document, that is signed by the identity referred to by the certificate. |
@haydentherapper do you have any more concerns with this proposal? I think it's in a good shape and we can accept it, and so start the implementation of adding the new extensions to the certificate. |
@kommendorkapten i have not yet had a chance to review it and plan to do it either next week or the following. I’d also like some folks from SLSA to chime in, since they’ve had some experience with verifying with GH claims. |
This is a hard blocker for quickly onboarding a collection of CI providers who are keep to participate in the (rapidly approaching) beta programs surrounding npm integration, and so I feel we need to bring this to conclusion in the next few days. Unless I'm missing something, we have the contours of the solution basically set? From my POV the main outstanding item at this point is documenting examples per suggestion from @laurentsimon. Does anyone feel that there are fields missing which need to be accounted for? |
f9ae202
to
1de1653
Compare
docs/oid-info.md
Outdated
|
||
### 1.3.6.1.4.1.57264.1.17 | Build Signer Digest | ||
|
||
An immutable reference to the specific version of the build instructions that is responsible for signing. For example: `abc123` git commit 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.
@haydentherapper @asraa @kommendorkapten It just occurred to me that we won't just be dealing with Git SHA1's fairly soon. Circle has the concept of Orb's that could be used to create a trusted builder and AFAICT these are OCI bundles so probs use SHA256.
Do you see any issue with not including the algo here if it's mostly SHA1 for all Git SCM's but SHA256 for Circle Orb's? It seems ok to push this on clients to figure out or by prior knowledge based on issuer 🤷
From our chats I'm not sold on doing anything clever in Fulcio as this seems like it might just cause more problems down the road but keen to get your thoughts.
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.
@feelepxyz I think keeping the value in Fulcio as-is from the integrator is preferred. I wouldn't think it's a big problem to omit the algorithm, as you said the clients should have prior knowledge on what to expect. This I think is also consistent with the other extensions in the certificate which values are specific per integration.
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 agree with keeping the value as-is, though I'll play devils advocate here because I do see your concern. Are there any other extensions where verifiers are going to need to be aware of which CI provider the certificate came from? If not, then this is additional work for verifiers for just a single claim.
On the flip side, how will fulcio determine the algorithm? If we're just going to check digest length for example, that's something a client could do.
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 don't see any other claims where it's expected to know the CI provider.
Slightly tangential, is hash_alg:digest standardized anywhere?
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.
If we're just going to check digest length for example, that's something a client could do.
Totally agree. A better option IMHO is to push for the CI vendors to switch to alg:digest.
Also thinking how a verifier should consume the digest during policy execution. By definition the digest does not have any semantic structure, and so what a client can do is to keep an allowlist of known "good" digests for the builders. That would mean that the digest is an opaque string used to compare a against a list of approved values. In that case, the inclusion of algorithm would not make a difference.
If we compare this with the subject digest in an in-toto attestation, the hash is important, as we are mapping artifacts (where the client needs to first compute the message digest) to attestations, which will likely be of high cardinality. For builders I would expect low cardinality and a highly specialized scenrio with much tighter policies, where the client typically don't compute a digest.
Slightly tangential, is hash_alg:digest standardized anywhere?
I'm only aware of the OCI image spec: https://github.com/opencontainers/image-spec/blob/v1.0.1/descriptor.md#digests
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.
On the flip side, how will fulcio determine the algorithm? If we're just going to check digest length for example, that's something a client could do.
Yeah was thinking either check length or just hardcode expected algo for a given issuer and claim but this might break in future. But doesn't feel like a major value add?
I don't see any other claims where it's expected to know the CI provider.
You kinda need to know what to expect for each CI provider for all of them to be able to verify things. For Runner Environment
this might be set to github-hosted
and circle-hosted
etc.
I think what @kommendorkapten is is right, if the verifier is mainly checking extension values against approved values, none of this would be a problem as you're just comparing strings.
Totally agree. A better option IMHO is to push for the CI vendors to switch to alg:digest.
Yup something needs to happen here across the industry to support sha256 in git 🙈
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.
Closing on this, I believe we're all in agreement now to not include a hash algorithm and copy the value as-is, correct? We can always update during implementation if we see a need for it.
|
||
Recommended: | ||
|
||
- SHOULD include `iss` that uniquely identifies ID tokens originating from the CI/CD system, e.g. not shared with OIDC OAuth 2.0 tokens for email/username logins. |
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.
What is the reason for this requirement?
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.
Seems like good hygiene to separate OIDC issuers so you don't just rely on the audience to separate?
GitLab for example use the same issuer for email logins and workload identities at the moment but are working on separating these out. It could add some protections against very poorly configured OIDC trust relationships between CI <> Cloud where you only check the issuer.
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.
Makes sense, although you could still run into issues if the OIDC issuers are run within the same trust domain such that if one is compromised, both could be. For example a CI issuer issuing a token for an email address, or an email-based issuer issuing a token for a CI identity (we do add some checks in fulcio to make sure a subject looks like an email when it's expected, I would hesitate to always enforce that a CI identity is not an email though)
docs/oid-info.md
Outdated
|
||
The immutable identifier for the owner of the source repository that the workflow was based upon. For example: `5678` if using a primary key. | ||
|
||
### 1.3.6.1.4.1.57264.1.14 | Build Config URI |
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.
To confirm, should this include the server URL? For GH, I see that workflow_ref and job_worflow_ref don't. If so, we should add SHOULD or MUST.
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.
Updated these to clarify the url SHOULD be fully qualified.
Just to nail down the GHA details, we're now prefixing these:
Run Invocation URI
:server_url + repository + "/actions/runs/" + run_id + "/attempts/" + run_attempt
Build Signer URI
:server_url + job_workflow_ref
But not these:
Source Repository URI
:repository
(owner/repo)Source Repository Owner URI
:repository_owner
(owner)Build Config URI
:workflow_ref
.. should we be consistent across the board and prefix those with server_url
? 😅
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.
TBH seems ok as is. Should we get this in as is and noodle on specific GHA details when implementing if needed?
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 fine updating in a follow up, but my plan would be to use exactly what is in this doc when implementing for any CI provider (also makes it easier to audit). So if you want to prefix a URI with server_url, we should specify it.
docs/oid-info.md
Outdated
|
||
### 1.3.6.1.4.1.57264.1.17 | Build Signer Digest | ||
|
||
An immutable reference to the specific version of the build instructions that is responsible for signing. For example: `abc123` git commit 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.
I don't see any other claims where it's expected to know the CI provider.
Slightly tangential, is hash_alg:digest standardized anywhere?
## 1.3.6.1.4.1.57264.2 | Policy OID for Sigstore Timestamp Authority | ||
|
||
Not used by Fulcio. This specifies the policy OID for the [timestamp authority](https://github.com/sigstore/timestamp-authority) | ||
that Sigstore operates. | ||
|
||
<!-- References --> | ||
## Mapping OIDC token claims to Fulcio OIDs |
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.
Leaving this open in case anyone has any other thoughts, but I think we're all in agreement that we won't attempt to do any versioning.
1de1653
to
687c5b1
Compare
Co-authored-by: Phill MV <[email protected]> Co-authored-by: Fredrik Skogman <[email protected]> Signed-off-by: Philip Harrison <[email protected]>
687c5b1
to
ad293ad
Compare
Signed-off-by: Philip Harrison <[email protected]>
71a8dbf
to
a10950c
Compare
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.
Nice job! 🚀
Not that I know of, although I've seen it pop up all over the place. I don't think even the algorithm identifiers are really standardized, unless we were to switch to OIDs (and that seems extremely overkill). |
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.
Fantastic job on this and thanks for the many iterations!
I'll leave it open til Monday morning for any remaining comments and merge then.
A small follow-up (or feel free to do it in this PR too, I'll reapprove) is to make sure the GitHub mapping of OID value to claims is accurate (with any prefixes or concatenations noted), as I'll use this as the guide for implementation.
Signed-off-by: Philip Harrison <[email protected]>
1561d4d
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.
lgtm, thx all!
- MUST include claim to support: `Build Signer URI` that identifies the specific build instructions that are responsible for signing. | ||
- MUST include claim to support: `Build Signer Digest` which is an immutable reference to a specific version of the build instructions that are responsible for signing. |
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.
How should these attributes be supported by services like Buildkite which offer dynamic pipelines, i.e. build instructions are not immutable and change over the course of an execution? I suppose we could include a reference to the initial build instructions?
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, the directory examples suggest this could be a link to the source repository at a specific commit sha. That doesn't represent the whole signing environment for us, the instructions also involve the running pipeline, but it might be close enough.
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.
Yes, so ideally this would be an immutable reference to the Buildkite pipeline steps at the time the build ran.
How are Buildkite pipeline steps stored / versioned? If I update the pipeline steps after a build has run, is there a URL I can go to that references the previous version of the pipeline steps (e.g. the pipeline steps when the build ran)?
Summary
Proposal for standardizing Fulcio's Certificate Extensions to align with the discussion on standardizing OIDC token claims across CI/CD systems (today GitHub Actions, in future Circle, GitLab, Buildkite etc).
The aim here is to find new platform agnostic extensions (as the current ones are GitHub specific) that make sense across the different CI/CD providers that we'd like to see supported in Fulcio.
Fixes #754
Fixes #955
Signed-off-by: Philip Harrison [email protected]