sigstore / cosign

Code signing and transparency for containers and binaries
Apache License 2.0
4.53k stars 546 forks source link

Migration of Public API in support of sigstore-go migration #3879

Open codysoyland opened 2 months ago

codysoyland commented 2 months ago

Migration of Public API in support of sigstore-go migration

As we work toward support of the TrustedRoot in the cosign verifier, I would like to take a moment to consider our plans for the cosign public API. Beginning with verification, there are several functions that take a CheckOpts struct as an argument. This struct is used to configure the verification process for a series of functions:

CheckOpts is largely replaced by a few data structures in sigstore-go:

There are a few paths we can take with the cosign API:

  1. We can deprecate all the existing API functions and the use of CheckOpts altogether, requiring users to migrate to sigstore-go for general verification, and we would offer only a handful of container image verification utilities in cosign's public API. This could be very disruptful for existing users of cosign's public API, but ease development on cosign.
  2. We can continue supporting cosign's public API as is, and add new functions that use the new sigstore-go types. This would be less disruptive to existing users, but would result in a more complex API surface.
  3. We can deprecate most of the fields within CheckOpts, replacing them with equivalent sigstore-go types. This would be less disruptive to existing users, giving them a clear migration path, but may result in more complexity cosign's development. In this option, we would support both verifiers, and only use the deprecated fields if a TrustedMaterial is not provided. After a deprecation period, we can delete the deprecated fields and all associated verification code paths. This option would ensure API continuity for existing users.

In option 3, I would propose we modify CheckOpts like so:

Field Strategy
RegistryClientOpts Keep
Annotations Keep
ClaimVerifier Keep
RekorClient Deprecate, use TrustedMaterial
RekorPubKeys Deprecate, use TrustedMaterial
SigVerifier Keep?
PKOpts Keep?
RootCerts Deprecate, use TrustedMaterial
IntermediateCerts Deprecate, use TrustedMaterial
CertGithubWorkflowTrigger Deprecate, use CertificateIdentities
CertGithubWorkflowSha Deprecate, use CertificateIdentities
CertGithubWorkflowName Deprecate, use CertificateIdentities
CertGithubWorkflowRepository Deprecate, use CertificateIdentities
CertGithubWorkflowRef Deprecate, use CertificateIdentities
IgnoreSCT Deprecate, use VerifierOptions
SCT Deprecate, stop supporting detached SCTs
CTLogPubKeys Deprecate, use TrustedMaterial
SignatureRef Deprecate, get from Bundle
PayloadRef Deprecate, get from Bundle
Identities Deprecate, use CertificateIdentities
Offline Deprecate, use VerifierOptions
TSACertificate Deprecate, use TrustedMaterial
TSARootCertificates Deprecate, use TrustedMaterial
TSAIntermediateCertificates Deprecate, use TrustedMaterial
IgnoreTlog Deprecate, use VerifierOptions
MaxWorkers Currently unsupported by sigstore-go
ExperimentalOCI11 Deprecate, replace with Cosign Bundle Spec
TrustedMaterial New field, root.TrustedMaterial
CertificateIdentities New field, verify.CertificateIdentities
VerifierOptions New field, []verify.VerifierOption

Also note that several existing API functions receive a oci.Signature argument. We could do a similar thing to this struct, adding a field for the SigstoreBundle and deprecating other fields.

I have been somewhat in favor of option 3: migration of CheckOpts and continuity of API surface, however, I can see how this would complicate development on cosign, and outside of container image verification, it would be good to encourage users to switch to sigstore-go, so perhaps option 1 is preferable. I don't have a good sense of how much work this would put on existing users. I will say that Sigstore Policy Controller is somewhat tightly coupled with the CheckOpts and RegistryClientOpts structs. I would love to get feedback from the greater cosign community on which option makes the most sense.

codysoyland commented 2 months ago

As a small follow-on, I'd like to demonstrate how a proposed additional API surface might look with one example. If we deprecate the CheckOpts field, then the function to verify an image attestation would look like this:

old version:

func VerifyImageAttestation(ctx context.Context, atts oci.Signatures, h v1.Hash, co *CheckOpts) (checkedAttestations []oci.Signature, bundleVerified bool, err error)

new version:

func VerifyImageAttestationBundle(ctx context.Context, atts oci.Signatures, h v1.Hash, trustedMaterial *root.TrustedMaterial, certificateIdentities verify.CertificateIdentities, opts []verify.VerifierOption, registryClientOpts []ociremote.Option, annotations map[string]interface{}) (checkedAttestations []oci.Signature, bundleVerified bool, err error)

I dislike long function signatures especially in public APIs, so I would probably want to encapsulate image verification options, much like CheckOpts currently does. Perhaps an ImageVerificationOptions struct could be created for this.

steiza commented 2 months ago

I updated #3854 to make use of CheckOpts as described above, with some small tweaks.

Instead of CertificateIdentities I added IdentityPolicies []verify.PolicyOption, because the identity policy could be that you're verifying with a key instead of a certificate.

Otherwise, it seems like everything else worked pretty well! Definitely interested in feedback on the pull request.