sigstore / sigstore-go

Go library for Sigstore signing and verification
Apache License 2.0
43 stars 21 forks source link

Proposal: TrustedMaterial Interface Refinement #293

Open codysoyland opened 3 days ago

codysoyland commented 3 days ago

Description

The TrustedMaterial interface currently includes these methods:

type TrustedMaterial interface {
    TimestampingAuthorities() []CertificateAuthority
    FulcioCertificateAuthorities() []CertificateAuthority
    RekorLogs() map[string]*TransparencyLog
    CTLogs() map[string]*TransparencyLog
    PublicKeyVerifier(string) (TimeConstrainedVerifier, error)
}

The CertificateAuthority interface is defined as:

type CertificateAuthority struct {
    Root                *x509.Certificate
    Intermediates       []*x509.Certificate
    Leaf                *x509.Certificate
    ValidityPeriodStart time.Time
    ValidityPeriodEnd   time.Time
    URI                 string
}

There are a couple of issues with this interface:

I propose the following changes:

These would be defined as follows:

type TimestampingAuthority interface {
    Verify(signedTimestamp []byte, signatureBytes []byte) error
}

type SigstoreTimestampingAuthority struct {
    Root                *x509.Certificate
    Intermediates       []*x509.Certificate
    Leaf                *x509.Certificate
    ValidityPeriodStart time.Time
    ValidityPeriodEnd   time.Time
    URI                 string
}

func (sta *SigstoreTimestampingAuthority) Verify(signedTimestamp []byte, signatureBytes []byte) error {
    // verification logic here
}
type CertificateAuthority interface {
    Verify(cert *x509.Certificate, observerTimestamp time.Time) error
}

type FulcioCertificateAuthority struct {
    Root                *x509.Certificate
    Intermediates       []*x509.Certificate
    ValidityPeriodStart time.Time
    ValidityPeriodEnd   time.Time
    URI                 string
}

func (fca *FulcioCertificateAuthority) Verify(cert *x509.Certificate, observerTimestamp time.Time) error {
    // verification logic here
}

The TrustedMaterial interface would then be updated to look like this:

type TrustedMaterial interface {
    TimestampingAuthorities() []TimestampingAuthority
    FulcioCertificateAuthorities() []CertificateAuthority
    RekorLogs() map[string]*TransparencyLog
    CTLogs() map[string]*TransparencyLog
    PublicKeyVerifier(string) (TimeConstrainedVerifier, error)
}

One of the benefits of this approach is a smoother transition for cosign. This would allow cosign's CheckOpts to implement the TrustedMaterial interface, as it currently is impossible due to the use of x509.CertPool in cosign, which is not compatible with the current TrustedMaterial interface.

haydentherapper commented 2 days ago

I'm +1 to all of these changes, thanks for putting this together!

In some instances, we may want to allow a client to provide a *x509.CertPool

This also will come up with https://github.com/sigstore/protobuf-specs/issues/249, which I'd really like to do before we have a 1.0 release of the protobuf spec.

steiza commented 2 days ago

I have some clarifying questions, one general and the other more specific.

First the general question, with a disclaimer that I haven't thought much about certificate pools until this week. Let's say in our trusted root there's 3 different certificateAuthorities. Today in sigstore-go we iterate through that list and create a separate certificate pool for each certificate authority we're verifying against. I think what we're proposing here is to allow one certificate pool that contains all the content from the different certificateAuthorities at once. Is that the case? And if so, is there a meaningful difference / does that allow malicious use that wouldn't be allowed if they were separate? If not, should we change sigstore-go's implementation to create one big certificate pool for all the certificate authorities?

Assuming we get that ironed out : ) today in sigstore-go the TrustedMaterials interface mirrors the private members of TrustedRoot and the various function signatures in trusted_root.go. Would we update those all to match? Or are these changes just scoped to TrustedMaterials?

haydentherapper commented 1 day ago

And if so, is there a meaningful difference / does that allow malicious use that wouldn't be allowed if they were separate?

It shouldn't allow any malicious behavior. x509 libraries implement path finding, using the subject key IDs and authority key IDs in each certificate to determine the path from a leaf certificate down to a CA certificate from the root pool. There is no meaningful difference between the two when it comes to certificate validation.

If not, should we change sigstore-go's implementation to create one big certificate pool for all the certificate authorities?

Yea, I think that would be reasonable. With the current logic, we would lose the validity window check however if we merge all CA certs together, so we'd need a mapping from CA cert to trust root entry to check the validity window after the x509 library verifies the certificate and finds the certificate chain (which is returned here, we currently ignore this).