gen, protos: 0.3, single cert #191
Conversation
Signed-off-by: William Woodruff <[email protected]>
| @@ -32,8 +32,8 @@ option ruby_package = "Sigstore::Bundle::V1"; | |||
| // The primary message ('Bundle') MUST be versioned, by populating the | |||
| // 'media_type' field. Semver-ish (only major/minor versions) scheme MUST | |||
| // be used. The current version as specified by this file is: | |||
| // application/vnd.dev.sigstore.bundle+json;version=0.2 | |||
| // The semantic version is thus '0.2'. | |||
| // application/vnd.dev.sigstore.bundle+json;version=0.3 | |||
There was a problem hiding this comment.
Just pressure testing - Do we want a new version at this point? I think yes. We could add the field without a new version to avoid this being a breaking change, but then the field would have to be optional.
There was a problem hiding this comment.
Yep, I think so as well -- IMO making the field non-optional would be confusing in context, since the other two content variants are marked as required.
(But maybe there are other good reasons to not bump the version here? Other client maintainers should probably chime in 🙂)
There was a problem hiding this comment.
I would think that a breaking change is desired if we want clients to actually adopt the usage of the new certificate. If not, clients would need to write the certificate into both x509_certificate_chain and certificate, and I'm not sure that's desirable?
I think we can roll this out quite easy by describing the change more of a clarification, and as part of that moving the certificate to a new field and making it explicit. This would of course still require clients to discriminate on the version, and this is less optimal for verifiers that would need to be compliant with multiple versions.
There was a problem hiding this comment.
I would think that a breaking change is desired if we want clients to actually adopt the usage of the new
certificate. If not, clients would need to write the certificate into bothx509_certificate_chainandcertificate, and I'm not sure that's desirable?
My 0.02c would be for encouraging clients to adopt the certificate form -- the language in VerificationMaterial currently says that adopting the 0.3 bundle for keyless signing + PGI means that clients MUST use the certificate field instead of x509_certificate_chain.
But yeah, this does make things a bit of a mess for verifying clients...
There was a problem hiding this comment.
For clients that wish to only support PGI, we could say just take the first cert of cert-chain or certificate.
There was a problem hiding this comment.
I think the current wording is good 👍 . My comment was a general about clients that would need to deal with multiple versions, not how the fields are used to day. I.e that regardless what we do, clients have to pay the price :) This is of course not specific to this spec, it's a general statement of concurrently supporting multiple versions.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, maybe it's time we plan for a next release, i.e bulk more things together as you mentioned. Don't think there is any urgency around getting v0.3 out?
There was a problem hiding this comment.
No extreme urgency, although I would definitely like to get #189 merged sooner rather than later 🙂 -- there's a lot of work that isn't strictly blocked on it, but will probably become a client interop challenge unless we eventually enumerate the various sigs/hashes that can occur on the PGI.
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: Fredrik Skogman <[email protected]> Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
|
In terms of procedure, should we merge here and let |
This adds a new
oneofvariant toVerificationMaterials.contentsthat specifically encodes a single X.509 leaf certificate. This ratchets down (soft) invariants that were previously documented on thex509_certificate_chainmember.In the process, it also bumps the bundle version (
0.3) and moves some documentation around.Closes #190.