Proposal: TrustedMaterial Interface Refinement

[codysoyland]

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:

• Timestamp Authorities are not a type of certificate authority, they just happen to share a lot of the same fields.
• Fulcio certificate authorities don't contain a leaf certificate, so this field is always nil.
• This approach doesn't allow for flexibility of the verification interface. Currently, the verification routines in verify/certificate.go and verify/tsa.go are tightly coupled to the CertificateAuthority struct. In some instances, we may want to allow a client to provide a *x509.CertPool (for example, to cleanly migrate cosign to the new TrustedMaterial interface), to provide a custom certificate verifier function, or to provide multiple root certificates.

I propose the following changes:

• Add a new TimestampingAuthority interface.
• Add a new CertificateAuthority interface.
• Add structs FulcioCertificateAuthority and SigstoreTimestampingAuthority that implement the new interfaces.
• Verification logic for these types would largely move out of the pkg/verify package and into the pkg/root package.

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]

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 sigstore/protobuf-specs#249, which I'd really like to do before we have a 1.0 release of the protobuf spec.

[steiza]

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]

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).

[kommendorkapten]

My fear if we put all certificates into a single cert pool is that it would complicate the logic a bit, and so increase the risk of a bug causing validation to succeed when it shouldn't.

As Hayden mention, we need to verify the chain after a successful verification, to see if that chain was valid during time by comparing this against the trusted certificate chains. I feel like this is a possibly complicated operation. Keeping each ca as a single struct and then iterating over them seems less error prone to me, as the only comparison to be done is the first initial comparison of the time stamp, then verification can go forth and creating the cert pool.

If we group all certs together, my understanding is that we would then need to (perfectly) match the the returned chains against the chains in the trusted root, which is a bit more complicated. (This is a M * N * L comparison, M being number of chains returned, N the number of chains in the trusted root and L the length of the chain, which if course will vary).