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.

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

Add the ability to contruct TrustRoot from targets #247

Merged
merged 6 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
9 changes: 9 additions & 0 deletions pkg/root/trusted_root.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,15 @@ func (tr *TrustedRoot) CTLogs() map[string]*TransparencyLog {
return tr.ctLogs
}

func (tr *TrustedRoot) MarshalJSON() ([]byte, error) {
err := tr.constructProtoTrustRoot()
if err != nil {
return nil, fmt.Errorf("failed constructing protobuf TrustRoot representation: %w", err)
}

return protojson.Marshal(tr.trustedRoot)
}

func NewTrustedRootFromProtobuf(protobufTrustedRoot *prototrustroot.TrustedRoot) (trustedRoot *TrustedRoot, err error) {
if protobufTrustedRoot.GetMediaType() != TrustedRootMediaType01 {
return nil, fmt.Errorf("unsupported TrustedRoot media type: %s", protobufTrustedRoot.GetMediaType())
Expand Down
378 changes: 378 additions & 0 deletions pkg/root/trusted_root_create.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,378 @@
// Copyright 2023 The Sigstore Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package root

import (
"crypto"
"crypto/ecdsa"
"crypto/ed25519"
"crypto/elliptic"
"crypto/rsa"
"crypto/sha256"
"crypto/x509"
"encoding/hex"
"encoding/pem"
"errors"
"fmt"
"time"

protocommon "github.com/sigstore/protobuf-specs/gen/pb-go/common/v1"
prototrustroot "github.com/sigstore/protobuf-specs/gen/pb-go/trustroot/v1"
timestamppb "google.golang.org/protobuf/types/known/timestamppb"
)

const (
FulcioTarget = "Fulcio"
RekorTarget = "Rekor"
CTFETarget = "CTFE"
TSATarget = "TSA"
)

type RawTrustedRootTarget interface {
GetType() string
GetBytes() []byte
}

type BaseTrustedRootTarget struct {
Type string
Bytes []byte
}

func (b *BaseTrustedRootTarget) GetType() string {
return b.Type
}

func (b *BaseTrustedRootTarget) GetBytes() []byte {
return b.Bytes
}

// NewTrustedRootFromTargets initializes a TrustedRoot object from a mediaType string and
// a slice of targets. These targets are expected to be PEM-encoded public keys/certificate chains.
// This method of constructing the TrustedRoot has some shortcomings which can be aided by manually
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't address these, the concern would be that one would construct a trust root with NewTrustedRootProtobuf(json), then marshal it with MarshalJSON, and get back a different trust root.

We should have a test that demonstrates JSON-input -> TrustedRoot struct -> JSON-output, where JSON-input == JSON-output.

What if the API takes in a TrustedRoot struct, fully populated, and outputs JSON? And if certain fields are missing, then we can have reasonable defaults (e.g if TransparencyLog.HashFunc is not set, then pick SHA256)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So there's couple different things to note here:

  • The only thing that this new method does is it constructs a TrustedRoot from given targets; this does not concern the JSON representation of the TrustedRoot. On the contrary this is most useful when there is no TrustedRoot yet and one needs to be constructed from public keys/certs.
  • However, I understand your concern about having the ability to marshal TrustedRoot - which was also added by this PR - I think the only problem right now is the "Uri" argument which I left unfilled for CertificateAuthority struct, so I probably need to fix that and I can also add a test that you suggest.
  • I think taking in a TrustedRoot struct fully populated defeats the purpose of this PR - that won't solve a lot of what I need in Add functionality to generate trusted_root.json by the TUF server scaffolding#1191 and most of the code will need to stay there to create the TrustedRoot struct in the first place.

So what I propose is adding the test as you suggested, and fixing any issues that pop up, but otherwise leaving this PR as is. Does that sound good?

// adjusting the TrustedRoot object after instantiation:
//
// - publicKey instances for tlogs/ctlogs will have validFor.start set to Time.now() and no validFor.end.
// - Merkle Tree hash function is hardcoded to SHA256, as this is not derivable from the public key.
// - Each certificate chain is expected to be given as a single target, where there is a newline
// between individual certificates. It is expected that the certificate chain is ordered (root last).
func NewTrustedRootFromTargets(mediaType string, targets []RawTrustedRootTarget) (*TrustedRoot, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a lot of use-cases for making it easier to construct a trusted root, and the challenge here is going to be to make an API that's flexible enough to cover those use-cases. If you're in something like cosign verify-blob with --certificate-chain and --timestamp-certificate-chain, it's totally fine to throw together something quickly to verify. But if you're the public good instance (or a private instance) you're going to want something that supports rotating key material with expiry times.

In pkg/root/trusted_root.go we already have these nice intermediate representations of CertificateAuthority and TransparencyLog, so what if we added there something like:

func NewTrustedRoot(
    certificateAuthorities []CertificateAuthorities, 
    certificateTransparencyLogs []TransparencyLog,
    timestampAuthorities []CertificateAuthorities,
    transparencyLogs []TransparencyLog
) TrustedRoot

This API would be similar to sign.BundleOptions, which is nice.

So if you're in cosign verify-blob and all you have is a PEM-encoded byte string from --certificate-authority, you could construct a minimal CertificateAuthority to pass in to NewTrustedRoot, but if you had a start and end time that certificate was valid for, you could also supply that information. But (crucially!) then we don't have to guess as to what the configuration should be, and we don't risk returning a half-configured object that the caller then has to go and fix up.

That would be enough if you just need a TrustedRoot to call into sigstore-go, but if you want to output a JSON file we might need one more step. I love the idea of adding a function like:

func (tr *TrustedRoot) MarshalJSON() ([]byte, error)

but instead of the current implementation of constructProtoTrustRoot(), maybe we could see if TrustedRoot.trustedRoot is already set up (like if it came from NewTrustedRootFromProtobuf()) or not (like if it came from the proposed NewTrustedRoot()) and only populate content in TrustedRoot.trustedRoot if it hasn't already been set up. This would ensure that if you load from a JSON file and call MarshalJSON() you get the same content back.

Does that make sense? It might make sense to get feedback on the API and approach from a few folks and settle on a direction before we move ahead with the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your input! I've been thinking about this and I have this alternative implementation proposal:

  • Instead of having the current NewTrustedRootFromTargets function, I will add NewTrustedRoot as you suggested.
  • I will also expose functions that can instantiate CertificateAuthority and TransparencyLog from certificate/public key material. These functions already exist in this PR, but are private. I'll revisit them and ensure their API is sane, name them better etc, so that they can be exposed. This will make sure that my usecase of easily instantiating the whole TrustedRoot object from the certificate/public key material is still very easy using this library.
  • In terms of the MarshalJSON call, it's important to note that we publicly allow users to access and modify the structures that form the TrustedRoot object via TimestampingAuthorities, FulcioCertificateAuthorities functions etc. This means that if we just use the current trustedRoot, we could be serializing an outdated representation of TrustedRoot.

Does that make sense? Also CC @haydentherapper, I think this should also address your original concern. Let me know if this makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

I will also expose functions that can instantiate CertificateAuthority and TransparencyLog from certificate/public key material.

I think any helper functions like this should live outside of sigstore-go. It might seem silly, but one of the design goals of sigstore-go is to have opinionated interfaces that get wrapped by things that call sigstore-go to be more user-friendly. A great example of this is cosign, where you can supply information via a URL, a path on disk, a string of encoded data, etc. We want that flexibility to live in cosign, instead of having several similar functions in sigstore-go that support slightly different data input.

In terms of the MarshalJSON call, it's important to note that we publicly allow users to access and modify the structures that form the TrustedRoot object via TimestampingAuthorities, FulcioCertificateAuthorities functions etc. This means that if we just use the current trustedRoot, we could be serializing an outdated representation of TrustedRoot.

Got it - makes sense! Like you say, we should make sure whatever we marshal reflects the current state of the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think any helper functions like this should live outside of sigstore-go. It might seem silly, but one of the design goals of sigstore-go is to have opinionated interfaces that get wrapped by things that call sigstore-go to be more user-friendly. A great example of this is cosign, where you can supply information via a URL, a path on disk, a string of encoded data, etc. We want that flexibility to live in cosign, instead of having several similar functions in sigstore-go that support slightly different data input.

I certainly don't want to go against the design goal that you folks have. TBH I wasn't even aware that this was the design goal - is there a good document that outlines design goals?

The only reason why I opened this PR in here was that @haydentherapper suggested it in sigstore/scaffolding#1191 (comment). I can definitely change the PR in the way you suggested, but I would like Hayden to comment on here to ensure I'm doing changes that everybody will agree with conceptually.

// document that we assume 1 cert chain per target and with certs already ordered from leaf to root
if mediaType != TrustedRootMediaType01 {
return nil, fmt.Errorf("unsupported TrustedRoot media type: %s", TrustedRootMediaType01)
}
tr := &TrustedRoot{
ctLogs: make(map[string]*TransparencyLog),
rekorLogs: make(map[string]*TransparencyLog),
}
now := time.Now()

var fulcioCertChains, tsaCertChains [][]byte

for _, target := range targets {
switch target.GetType() {
case FulcioTarget:
fulcioCertChains = append(fulcioCertChains, target.GetBytes())
case TSATarget:
tsaCertChains = append(tsaCertChains, target.GetBytes())
case RekorTarget:
tlInstance, keyId, err := pubkeyToTransparencyLogInstance(target.GetBytes(), now)

Check failure on line 90 in pkg/root/trusted_root_create.go

View workflow job for this annotation

GitHub Actions / lint

var-naming: var keyId should be keyID (revive)
if err != nil {
return nil, fmt.Errorf("failed to parse rekor key: %w", err)
}
tr.rekorLogs[keyId] = tlInstance
case CTFETarget:
tlInstance, keyId, err := pubkeyToTransparencyLogInstance(target.GetBytes(), now)

Check failure on line 96 in pkg/root/trusted_root_create.go

View workflow job for this annotation

GitHub Actions / lint

var-naming: var keyId should be keyID (revive)
if err != nil {
return nil, fmt.Errorf("failed to parse ctlog key: %w", err)
}
tr.ctLogs[keyId] = tlInstance
}
}

for _, fulcioCertChain := range fulcioCertChains {
fulcioCA, err := certsToAuthority(fulcioCertChain)
if err != nil {
return nil, fmt.Errorf("failed to parse Fulcio certificate chain: %w", err)
}
tr.fulcioCertAuthorities = append(tr.fulcioCertAuthorities, *fulcioCA)
}

for _, tsaCertChain := range tsaCertChains {
tsaCA, err := certsToAuthority(tsaCertChain)
if err != nil {
return nil, fmt.Errorf("failed to parse TSA certificate chain: %w", err)
}
tr.timestampingAuthorities = append(tr.timestampingAuthorities, *tsaCA)
}

return tr, nil
}

func pubkeyToTransparencyLogInstance(keyBytes []byte, tm time.Time) (*TransparencyLog, string, error) {
logId := sha256.Sum256(keyBytes)

Check failure on line 124 in pkg/root/trusted_root_create.go

View workflow job for this annotation

GitHub Actions / lint

var-naming: var logId should be logID (revive)
der, _ := pem.Decode(keyBytes)
key, keyDetails, err := getKeyWithDetails(der.Bytes)
if err != nil {
return nil, "", err
}

return &TransparencyLog{
BaseURL: "",
ID: logId[:],
ValidityPeriodStart: tm,
HashFunc: crypto.SHA256, // we can't get this from the keyBytes, assume SHA256
PublicKey: key,
SignatureHashFunc: keyDetails,
}, hex.EncodeToString(logId[:]), nil
}

func getKeyWithDetails(key []byte) (crypto.PublicKey, crypto.Hash, error) {
var k any
var hashFunc crypto.Hash
var err1, err2 error

k, err1 = x509.ParsePKCS1PublicKey(key)
if err1 != nil {
k, err2 = x509.ParsePKIXPublicKey(key)
if err2 != nil {
return 0, 0, fmt.Errorf("can't parse public key with PKCS1 or PKIX: %w, %w", err1, err2)
}
}

switch v := k.(type) {
case *ecdsa.PublicKey:
switch v.Curve {
case elliptic.P256():
hashFunc = crypto.SHA256
case elliptic.P384():
hashFunc = crypto.SHA384
case elliptic.P521():
hashFunc = crypto.SHA512
default:
return 0, 0, fmt.Errorf("unsupported elliptic curve %T", v.Curve)
}
case *rsa.PublicKey:
switch v.Size() * 8 {
case 2048, 3072, 4096:
hashFunc = crypto.SHA256
default:
return 0, 0, fmt.Errorf("unsupported public modulus %d", v.Size())
}
default:
return 0, 0, errors.New("unknown public key type")
}

return k, hashFunc, nil
}

func certsToAuthority(certChainPem []byte) (*CertificateAuthority, error) {
var cert *x509.Certificate
var err error
rest := certChainPem
certChain := []*x509.Certificate{}

// skip potential whitespace at end of file (8 is kinda random, but seems to work fine)
for len(rest) > 8 {
var derCert *pem.Block
derCert, rest = pem.Decode(rest)
if derCert == nil {
return nil, fmt.Errorf("input is left, but it is not a certificate: %+v", rest)
}
cert, err = x509.ParseCertificate(derCert.Bytes)
if err != nil {
return nil, fmt.Errorf("failed to parse certificate: %w", err)
}
certChain = append(certChain, cert)
}
if len(certChain) == 0 {
return nil, fmt.Errorf("no certificates found in input")
}

ca := CertificateAuthority{}

for i, cert := range certChain {
switch {
case i == 0 && !cert.IsCA:
ca.Leaf = cert
case i < len(certChain)-1:
ca.Intermediates = append(ca.Intermediates, cert)
case i == len(certChain)-1:
ca.Root = cert
}
}

// TODO: should this rather be the innermost cert?
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. I think it should be the most restrictive validity period of all of the CA certificates, which is likely the intermediate lowest in the chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I can fix that.

Copy link
Contributor Author

@bkabrda bkabrda Jul 31, 2024

Choose a reason for hiding this comment

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

Done in ab939cc

ca.ValidityPeriodStart = ca.Root.NotBefore
ca.ValidityPeriodEnd = ca.Root.NotAfter

return &ca, nil
}

func (tr *TrustedRoot) constructProtoTrustRoot() error {
tr.trustedRoot = &prototrustroot.TrustedRoot{}
tr.trustedRoot.MediaType = TrustedRootMediaType01

for logId, transparencyLog := range tr.rekorLogs {

Check failure on line 227 in pkg/root/trusted_root_create.go

View workflow job for this annotation

GitHub Actions / lint

var-naming: range var logId should be logID (revive)
tlProto, err := transparencyLogToProtobufTL(*transparencyLog)
if err != nil {
return fmt.Errorf("failed converting rekor log %s to protobuf: %w", logId, err)
}
tr.trustedRoot.Tlogs = append(tr.trustedRoot.Tlogs, tlProto)
}

for logId, ctLog := range tr.ctLogs {

Check failure on line 235 in pkg/root/trusted_root_create.go

View workflow job for this annotation

GitHub Actions / lint

var-naming: range var logId should be logID (revive)
ctProto, err := transparencyLogToProtobufTL(*ctLog)
if err != nil {
return fmt.Errorf("failed converting ctlog %s to protobuf: %w", logId, err)
}
tr.trustedRoot.Ctlogs = append(tr.trustedRoot.Ctlogs, ctProto)
}

for _, ca := range tr.fulcioCertAuthorities {
caProto, err := certificateAuthorityToProtobufCA(ca)
if err != nil {
return fmt.Errorf("failed converting fulcio cert chain to protobuf: %w", err)
}
tr.trustedRoot.CertificateAuthorities = append(tr.trustedRoot.CertificateAuthorities, caProto)
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to sort the certificate authorities too (and TSAs) similar to what you did for the tlogs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right, I forgot to do that. I'll submit a fix for it as well.

}

for _, ca := range tr.timestampingAuthorities {
caProto, err := certificateAuthorityToProtobufCA(ca)
if err != nil {
return fmt.Errorf("failed converting TSA cert chain to protobuf: %w", err)
}
tr.trustedRoot.TimestampAuthorities = append(tr.trustedRoot.TimestampAuthorities, caProto)
}

return nil
}

func certificateAuthorityToProtobufCA(ca CertificateAuthority) (*prototrustroot.CertificateAuthority, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func certificateAuthorityToProtobufCA(ca CertificateAuthority) (*prototrustroot.CertificateAuthority, error) {
func certificateAuthorityToProtobufCA(ca *CertificateAuthority) (*prototrustroot.CertificateAuthority, error) {

Same here on receiving via value or pointer.

org := ""
if len(ca.Root.Subject.Organization) > 0 {
org = ca.Root.Subject.Organization[0]
}
var allCerts []*protocommon.X509Certificate
if ca.Leaf != nil {
allCerts = append(allCerts, &protocommon.X509Certificate{RawBytes: ca.Leaf.Raw})
}
for _, intermed := range ca.Intermediates {
allCerts = append(allCerts, &protocommon.X509Certificate{RawBytes: intermed.Raw})
}
if ca.Root == nil {
return nil, fmt.Errorf("root certificate is nil")
}
allCerts = append(allCerts, &protocommon.X509Certificate{RawBytes: ca.Root.Raw})

caProto := prototrustroot.CertificateAuthority{
Uri: "",
Subject: &protocommon.DistinguishedName{
Organization: org,
CommonName: ca.Root.Subject.CommonName,
},
ValidFor: &protocommon.TimeRange{
Start: timestamppb.New(ca.ValidityPeriodStart),
End: timestamppb.New(ca.ValidityPeriodEnd),
},
CertChain: &protocommon.X509CertificateChain{
Certificates: allCerts,
},
}

return &caProto, nil
}

func transparencyLogToProtobufTL(tl TransparencyLog) (*prototrustroot.TransparencyLogInstance, error) {
Copy link
Member

@kommendorkapten kommendorkapten Aug 21, 2024

Choose a reason for hiding this comment

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

Suggested change
func transparencyLogToProtobufTL(tl TransparencyLog) (*prototrustroot.TransparencyLogInstance, error) {
func transparencyLogToProtobufTL(tl *TransparencyLog) (*prototrustroot.TransparencyLogInstance, error) {

Any reason not to receive this via a pointer to avoid excessive copying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right, I missed that. I will fix it.

hashAlgo, err := hashAlgorithmToProtobufHashAlgorithm(tl.HashFunc)
if err != nil {
return nil, fmt.Errorf("failed converting hash algorithm to protobuf: %w", err)
}
publicKey, err := publicKeyToProtobufPublicKey(tl.PublicKey, tl.ValidityPeriodStart)
if err != nil {
return nil, fmt.Errorf("failed converting public key to protobuf: %w", err)
}
trProto := prototrustroot.TransparencyLogInstance{
BaseUrl: tl.BaseURL,
HashAlgorithm: hashAlgo,
PublicKey: publicKey,
LogId: &protocommon.LogId{
KeyId: tl.ID,
},
}

return &trProto, nil
}

func hashAlgorithmToProtobufHashAlgorithm(hashAlgorithm crypto.Hash) (protocommon.HashAlgorithm, error) {
switch hashAlgorithm {
case crypto.SHA256:
return protocommon.HashAlgorithm_SHA2_256, nil
case crypto.SHA384:
return protocommon.HashAlgorithm_SHA2_384, nil
case crypto.SHA512:
return protocommon.HashAlgorithm_SHA2_512, nil
case crypto.SHA3_256:
return protocommon.HashAlgorithm_SHA3_256, nil
case crypto.SHA3_384:
return protocommon.HashAlgorithm_SHA3_384, nil
default:
return 0, fmt.Errorf("unsupported hash algorithm for Merkle tree: %v", hashAlgorithm)
}
}

func publicKeyToProtobufPublicKey(publicKey crypto.PublicKey, tm time.Time) (*protocommon.PublicKey, error) {
pkd := protocommon.PublicKey{
ValidFor: &protocommon.TimeRange{
Start: timestamppb.New(tm),
},
}

rawBytes, err := x509.MarshalPKIXPublicKey(publicKey)
if err != nil {
return nil, fmt.Errorf("failed marshalling public key: %w", err)
}
pkd.RawBytes = rawBytes

switch p := publicKey.(type) {
case *ecdsa.PublicKey:
switch p.Curve {
case elliptic.P256():
pkd.KeyDetails = protocommon.PublicKeyDetails_PKIX_ECDSA_P256_SHA_256
case elliptic.P384():
pkd.KeyDetails = protocommon.PublicKeyDetails_PKIX_ECDSA_P384_SHA_384
case elliptic.P521():
pkd.KeyDetails = protocommon.PublicKeyDetails_PKIX_ECDSA_P521_SHA_512
default:
return nil, fmt.Errorf("unsupported curve for ecdsa key: %T", p.Curve)
}
case *rsa.PublicKey:
switch p.Size() * 8 {
case 2048:
pkd.KeyDetails = protocommon.PublicKeyDetails_PKIX_RSA_PKCS1V15_2048_SHA256
case 3072:
pkd.KeyDetails = protocommon.PublicKeyDetails_PKIX_RSA_PKCS1V15_3072_SHA256
case 4096:
pkd.KeyDetails = protocommon.PublicKeyDetails_PKIX_RSA_PKCS1V15_4096_SHA256
default:
return nil, fmt.Errorf("unsupported public modulus for RSA key: %d", p.Size())
}
case *ed25519.PublicKey:
pkd.KeyDetails = protocommon.PublicKeyDetails_PKIX_ED25519
default:
return nil, fmt.Errorf("unknown public key type: %T", p)
}

return &pkd, nil
}
Loading
Loading