Support trusted root in cosign verification #3854
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3854 +/- ##
==========================================
- Coverage 40.10% 36.71% -3.39%
==========================================
Files 155 203 +48
Lines 10044 12940 +2896
==========================================
+ Hits 4028 4751 +723
- Misses 5530 7589 +2059
- Partials 486 600 +114 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Nice to see some progress made here! I would love to have some tests on this before merging, but I do have some ideas on this topic that we could perhaps postpone and iterate later after merging this. Basically, I would like to come to some agreement on how we treat cosign's public API surface, now that sigstore-go is becoming the standard Go sigstore implementation. Cosign has a number of public API functions in the main package github.com/sigstore/cosign/v2/pkg/cosign, including cosign.VerifyBlobSignature, which this PR does not affect. I kind of want to shift this new bundle logic out of the cli/verify package and into the main cosign package to centralize verification logic. Of course, we would need to decide if we add new funcs there (e.g. cosign.VerifyBlobSignatureWithBundle) or try to enhance the existing ones. I do like the idea of API continuity -- adding new fields to CheckOpts for the trusted root (and perhaps some of the verification option types from sigstore-go), and deprecating several of the existing fields. Existing API users of cosign would benefit from a smoother transition to bundles/trusted roots this way. However, I can see how we might instead deprecate these public API funcs and point people at sigstore-go. Of course, for container image verification, the set of tradeoffs is different, because as we discussed, we don't want to add container image verification to sigstore-go. I have been looking at updating cosign's container image verification to use bundles/trusted roots, and IMO, a more iterative approach to updating existing public APIs would be cleaner. Would love to hear your and @haydentherapper's thoughts especially on this.
cmd/cosign/cli/attest/attest_blob.go
Outdated
| @@ -165,7 +165,21 @@ func (c *AttestBlobCommand) Exec(ctx context.Context, artifactPath string) error | |||
| var rekorEntry *models.LogEntryAnon | |||
|
|
|||
| if c.TSAServerURL != "" { | |||
| timestampBytes, err = tsa.GetTimestampedSignature(sig, client.NewTSAClient(c.TSAServerURL)) | |||
| // sig is the entire JSON DSSE Envelope; we just want the signature for TSA | |||
There was a problem hiding this comment.
I would probably put this change in its own PR as a bug fix (would be good to have in release notes).
There was a problem hiding this comment.
I've split this out into #3877
| return sev.Verify(bundle, verify.NewPolicy(verify.WithArtifact(buf), identityPolicies...)) | ||
| } | ||
|
|
||
| func assembleNewBundle(ctx context.Context, sigBytes, signedTimestamp []byte, envelope *dsse.Envelope, artifactRef string, cert *x509.Certificate, ignoreTlog bool, sigVerifier signature.Verifier, pkOpts []signature.PublicKeyOption, rekorClient *client.Rekor) (*sgbundle.Bundle, error) { |
There was a problem hiding this comment.
I would really like to see tests for this. I would take a look at the tests in verify_blob.go -- I authored the keylessStack test helper there (it was bundled into an advisory so my name is not in git-blame) which lets you do integration-y tests without running the e2e tests. This func should be covered if we assemble a trusted root there and try to verify some blobs.
There was a problem hiding this comment.
I added a happy-path test for assembleNewBundle and verifyNewBundle.
| return err | ||
| } | ||
|
|
||
| func verifyNewBundle(ctx context.Context, bundle *sgbundle.Bundle, trustedRootPath, keyRef, slot, certOIDCIssuer, certOIDCIssuerRegex, certIdentity, certIdentityRegexp, githubWorkflowTrigger, githubWorkflowSHA, githubWorkflowName, githubWorkflowRepository, githubWorkflowRef, artifactRef string, sk, ignoreTlog, useSignedTimestamps, ignoreSCT bool) (*verify.VerificationResult, error) { |
There was a problem hiding this comment.
Would it be better to assemble a CheckOpts and pass it in here instead of continuing to grow this func signature? I have some more thoughts on this in the main review.
There was a problem hiding this comment.
I think I'm going to leave this as-is for now, but definitely open to refactoring this to whatever is decided in #3879.
|
This addresses |
|
I have written up some more thoughts on what I wrote above regarding cosign's public API. Would love to have a discussion there: #3879 |
|
Thanks for adding tests! Generally it's awesome to see the trusted root supported in verification. But I do have some concerns though that I'm not entirely sure how to address. My main concern is over the assembly/verification of a bundle with sigstore-go whenever a TrustedRoot is provided on these lines. @cmurphy is working on TUF v2 support including support for the TrustedRoot, and her approach is to assemble the fields in CheckOpts from the TrustedRoot file from TUF. I think there's a good opportunity for code reuse here, as we can support the TrustedRoot using cosign's legacy code paths. I think that's a lot safer and less prone to bugs, and I would feel good about the consistent behavior when using the trusted root via TUF and via a CLI flag. I think it's important to preserve cosign's legacy verification logic as we switch to the TrustedRoot, as some of the features in cosign are difficult or unsupported in sigstore-go right now, for example the legacy container image verification code will be pretty difficult to implement correctly in sigstore-go. Longer term, I think some variant of my proposal in #3879 of adding a fields for Would love to hear @haydentherapper and @cmurphy's thoughts on this prior to approving. Personally I would like to see the TUF v2 support and this PR share some logic, but I could be convinced otherwise. |
|
Yeah, I think @cmurphy @codysoyland @haydentherapper and I need to align on approach here. Essentially, I think the question boils down to "how should cosign perform verification when it has a trusted root file (obtained via TUF or otherwise supplied directly)?" Option 1 is to take the trusted root contents, map it to Option 2 is to use sigstore-go to perform verification when there is a trusted root, which is what I'm proposing with this pull request. The benefit of option 1 is lots of parts of cosign understand / expect A disadvantage of option 1 is that many parts of the existing cosign verification path assume there's only one set of verification material, whereas a trusted root has several sets of verification material with overlapping time ranges. I don't have a full list of places that would need to be updated, but looking through
A benefit of option 2 is that sigstore-go today supports trusted roots, and has had substantial testing and usage of those codepaths. Disadvantages of option 2 are:
|
Along these lines, there are potential pitfalls of missing commands that need a trust root, such as for signing, and needing to duplicate code if new commands are added. |
|
Chiming in on this discussion, I haven't read over the code yet, sorry in advance if I say anything inaccurate related to the PR. Part of the goal of "theseus" was to slowly replace parts of Cosign such that users should be minimally impacted with each major change, but after enough major revisions, we'll have Cosign into the state that we want [1]. With the previous PRs @steiza authored, Zach brought Cosign up to parity with the other Sigstore clients when using the public good instance. Implementing this by only using sigstore-go APIs and controlling execution of this code with a dedicated flag was the right choice, as this made it quicker to integrate and start getting users used to the "new" (new to Cosign users) bundle formats. While using sigstore-go directly would let us iterate faster, this sounds more like a rewrite of Cosign than a slow, deliberate migration that lets us make small breaking changes over a number of major releases. For some context, Cosign v2 had soooo many breaking changes and we got a lot of community feedback around this. I think we'd still have users on Cosign v1 if we hadn't had to make a breaking change with the TUF metadata that forced users onto v2 (which we also received feedback on!). I want to avoid this mistake with Cosign v3, preserving as much of the same functionality as possible while making small breaking changes that users should be fine with (e.g removing all CLI flags for individual trust root material in favor of the trust root bundle - same functionality, different format, and we can provide migration tools). One of the other benefits of #3879 is it being easier to review and know we have maintained parity. We have great e2e testing in Cosign now, and ideally our refactors will largely not touch e2e testing (except when handling a new format) which will give us confidence that we haven't broken any existing features. I also recognize that the verification logic in Cosign is not up to par with the specification - we should discuss this more. This may be an area where we need to work around Cosign's limitations and document the gaps (e.g lack of validity window verification, assumptions that aren't present in in the spec). From there we can choose to either implement these gaps, or push forward for v3 knowing we have these gaps, and then tee up those fixes (either by directly using sigstore-go or implementing the gaps) for v4. Again my goal is small, deliberate changes that give us confidence we aren't unexpectedly changing Cosign's behavior and especially aren't changing it without a migration path. [1] We need to agree on what the end goals are as well, but I roughly would summarize it as:
|
We recently added partial trusted root support to cosign when you are verifying a protobuf bundle, but this did not cover the case where you aren't using a bundle. This implements trusted root support for those cases by assembling the disparate signed material into a bundle, fixing some TODOs from when we added protobuf bundle support. Signed-off-by: Zach Steindler <[email protected]>
Also remove fix that is being handled in sigstore#3877 Signed-off-by: Zach Steindler <[email protected]>
Also align with sigstore#3879 Signed-off-by: Zach Steindler <[email protected]>
steiza
force-pushed
the
trusted-root-verify-all
branch
from
1962479
to
d3222ee
Compare
Signed-off-by: Zach Steindler <[email protected]>
|
I haven't had time for a thorough review yet but I appreciate the addition of I would definitely be interested in trying to add support for the I would be in favor of choosing the verifier to use based on following table:
The behavior in question here is what to do in the bottom-right cell. I think defaulting to the cosign legacy verifier would be safest, and adding a flag like I suggested would allow users to opt-in to the new verifier. I also want to add that the legacy cosign container image signing logic with the Simple Signing envelope is something that I personally would like to deprecate, but users are probably depending on these old signed images that I would rather not implement support for in sigstore-go (though it has been demonstrated -- it not very clean as the bundle does not inherently support this envelope type in the same way it supports DSSE). This is part of the reason I think it would be smart to add TrustedRoot support for the legacy verifier -- so we can safely deprecate it while moving to TUF v2 and thus adding support for certificate revocation via validity windows in TrustedRoot. |
Yeah, we definitely need to align on the behavior of the new TUF client. We might be allowing TUF v2 clients to choose between fetching individual files or a
I think we need someone to do a more thorough investigation on
Oh yeah - we would definitely not be making interface changes to sigstore-go to accommodate this. The pattern we've been following is writing adapter code in cosign, and then calling into sigstore-go via the same interface. I think that's what we'd also do here? I.e. we'd take the code in https://github.com/sigstore/sigstore-go/blob/main/examples/oci-image-verification/main.go and move it into cosign, not sigstore-go. Would something like that be possible? |
|
re: the chart in #3844 (comment) and Cody's chart above, my preference would be to use sigstore-go when we're using a verification bundle, and Cosign's verification in all other cases (when we are not provided a verification bundle). My reasons for this are:
For how I think this should be rolled out:
Note I haven't thought through the container story fully, so that needs to be accounted for as well.
I can help with this, though realistically it'll be two weeks til I can. If we're OK waiting til then, I'll set up some time to walk through the code and if anyone's available, I'd be happy to review it with others. Otherwise, I'm also happy to validate what others find.
What is the reason to deprecate this? I wasn't around when these decisions were first made so I can't speak to why a simple signing envelope was chosen over another structure. |
|
As another option, I think we should consider using Cosign's verification logic exclusively, especially if we add support for the validity checks for trusted root. Then we need:
The benefit with this approach is we know we will not break any clients, and it limits the Cosign v3 changes to exclusively be changes to the CLI interface (e.g. remove old bundle, remove detached trust root material). The downside is duplicated logic with sigstore-go and some additional work in Cosign, but ultimately if we are doing this work already for the non-bundle case like noted above, then we might as well just leverage Cosign everywhere. Then Cosign v4 is a transition to sigstore-go as the signing/verification API. Since we've brought Cosign up to par with sigstore-go, we can enumerate all user journeys and test for breaking changes more easily. |
Working towards #3700
Summary
We recently added partial trusted root support to cosign when you are verifying a protobuf bundle, but this did not cover the case where you are using an old bundle, or using disparate signed material on disk.
This implements trusted root support for those cases by assembling the materials into a protobuf bundle, thus fixing some TODOs from when we added protobuf bundle support.
Generally to test I would recommend signing something and then verifying it with a trusted root, like:
Or:
It's also a good idea to test with a timestamp authority, although you need to create a trusted root file that corresponds to your chosen timestamp authority. For example:
Release Note
Documentation
N/A?