Allow parsing of certificates from Fulcio if ctlog is disabled #288
Conversation
Signed-off-by: Cody Soyland <[email protected]>
Signed-off-by: Cody Soyland <[email protected]>
| @@ -72,13 +72,18 @@ type publicKey struct { | |||
| } | |||
|
|
|||
| type fulcioResponse struct { | |||
| SctCertWithChain signedCertificateEmbeddedSct `json:"signedCertificateEmbeddedSct"` | |||
| SignedCertificateEmbeddedSct signedCertificateEmbeddedSct `json:"signedCertificateEmbeddedSct"` | |||
| SignedCertificateDetachedSct signedCertificateDetachedSct `json:"signedCertificateDetachedSct"` | |||
There was a problem hiding this comment.
We want to deprecate this in fulcio, no other client is using it so far.
Can we avoid adding this?
There was a problem hiding this comment.
GitHub Artifact Attestations does not use certificate transparency for private CA, using essentially fulcio serve --ct-log-url="", and when configured this way, Fulcio's response includes the certificate in signedCertificateDetachedSct. So this change is currently required to support GitHub's private CA.
Is the deprecation plan in Fulcio to disallow running Fulcio without a CT log?
There was a problem hiding this comment.
I would parse this as detached SCT is going to be deprecated, which seems fair?
There was a problem hiding this comment.
I located the Fulcio issue referencing this deprecation, and I noticed a comment from @bdehamer (with my name cc'd no less 😆 ):
One thing to note . . . we're running a private Fulcio instance w/ NO transparency log. In this mode, I noticed that the response from the create-signing-certificate API always uses the signedCertificateDetachedSct field in the response (despite the fact that there is no SCT). As the code is cleaned-up we'll want to make sure not to change this behavior so we don't break existing clients
This is the behavior I'm wanting to preserve/support in this PR -- not add support for detached SCTs in sigstore-go. I'm open to deprecating that JSON key if we want to improve naming the Fulcio response, but for now, this key is required for us to consume Fulcio certs in GitHub's Fulcio deployment.
@haydentherapper, btw, you commented in that issue that the deprecation would preserve that field for this use case.
There was a problem hiding this comment.
Sorry for taking so long to get back to you - yep, you're right, I was thinking we were writing the certificate in embedded still and that this field was only used when issuing a detached SCT. This LGTM, we'll make breaking changes as part of a 2.0 release that fixes this, but no changes now.
| @@ -72,13 +72,18 @@ type publicKey struct { | |||
| } | |||
|
|
|||
| type fulcioResponse struct { | |||
| SctCertWithChain signedCertificateEmbeddedSct `json:"signedCertificateEmbeddedSct"` | |||
| SignedCertificateEmbeddedSct signedCertificateEmbeddedSct `json:"signedCertificateEmbeddedSct"` | |||
| SignedCertificateDetachedSct signedCertificateDetachedSct `json:"signedCertificateDetachedSct"` | |||
There was a problem hiding this comment.
Sorry for taking so long to get back to you - yep, you're right, I was thinking we were writing the certificate in embedded still and that this field was only used when issuing a detached SCT. This LGTM, we'll make breaking changes as part of a 2.0 release that fixes this, but no changes now.
| @@ -56,7 +56,7 @@ func getFulcioResponse() (*http.Response, error) { | |||
| })) | |||
|
|
|||
| responseStruct := fulcioResponse{ | |||
| SctCertWithChain: signedCertificateEmbeddedSct{ | |||
| SignedCertificateEmbeddedSct: signedCertificateEmbeddedSct{ | |||
| Chain: chain{ | |||
| Certificates: []string{certPEM}, | |||
| }, | |||
There was a problem hiding this comment.
Can we add a test?
Summary
Currently, signing will fail if Fulcio has the ctlog disabled. This patch fixes the parsing of the Fulcio response so it allows certificates without an embedded SCT.
Fixes #287
Release Note
Documentation