Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow parsing of certificates from Fulcio if ctlog is disabled #288

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Allow parsing of certificates from Fulcio if ctlog is disabled #288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d39479d
Allow parsing of certificates from Fulcio if ctlog is disabled
codysoyland Sep 6, 2024
ddfd1e1
Change if/else to switch
codysoyland Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions pkg/sign/certificate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,18 @@ type publicKey struct {
}

type fulcioResponse struct {
SctCertWithChain signedCertificateEmbeddedSct `json:"signedCertificateEmbeddedSct"`
SignedCertificateEmbeddedSct signedCertificateEmbeddedSct `json:"signedCertificateEmbeddedSct"`
SignedCertificateDetachedSct signedCertificateDetachedSct `json:"signedCertificateDetachedSct"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to deprecate this in fulcio, no other client is using it so far.

Can we avoid adding this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would parse this as detached SCT is going to be deprecated, which seems fair?

Copy link
Member Author

@codysoyland codysoyland Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

type signedCertificateEmbeddedSct struct {
Chain chain `json:"chain"`
}

type signedCertificateDetachedSct struct {
Chain chain `json:"chain"`
}

type chain struct {
Certificates []string `json:"certificates"`
}
Expand Down Expand Up @@ -204,12 +209,17 @@ func (f *Fulcio) GetCertificate(ctx context.Context, keypair Keypair, opts *Cert
return nil, err
}

certs := fulcioResp.SctCertWithChain.Chain.Certificates
if len(certs) == 0 {
var cert []byte
switch {
case len(fulcioResp.SignedCertificateEmbeddedSct.Chain.Certificates) > 0:
cert = []byte(fulcioResp.SignedCertificateEmbeddedSct.Chain.Certificates[0])
case len(fulcioResp.SignedCertificateDetachedSct.Chain.Certificates) > 0:
cert = []byte(fulcioResp.SignedCertificateDetachedSct.Chain.Certificates[0])
default:
return nil, errors.New("Fulcio returned no certificates")
}

certBlock, _ := pem.Decode([]byte(certs[0]))
certBlock, _ := pem.Decode(cert)
if certBlock == nil {
return nil, errors.New("unable to parse Fulcio certificate")
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/sign/certificate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func getFulcioResponse() (*http.Response, error) {
}))

responseStruct := fulcioResponse{
SctCertWithChain: signedCertificateEmbeddedSct{
SignedCertificateEmbeddedSct: signedCertificateEmbeddedSct{
Chain: chain{
Certificates: []string{certPEM},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test?

Expand Down
Loading