Add the ability to contruct TrustRoot from targets #247

bkabrda · 2024-07-30T09:28:01Z

Summary

This PR:

Adds the ability to construct a TrustedRoot instance from certificate authorities for fulcio/tsa and transparency logs for ctlog and rekor
Adds JSON serialization logic for the TrustedRoot structure

This was requested in sigstore/scaffolding#1191 (review) by @haydentherapper.

Release Note

Added NewTrusted function to enable creating TrustedRoot from CertificateAuthority and TransparencyLogs instances
Added functionality to serialize TrustedRoot to JSON

Documentation

I don't think this needs additional documentation, the added public function has a godoc that I think is sufficient.

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

haydentherapper · 2024-07-30T16:26:00Z

pkg/root/trusted_root_create.go

+
+// 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


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)

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?

haydentherapper · 2024-07-30T16:31:20Z

pkg/root/trusted_root_create.go

+		}
+	}
+
+	// TODO: should this rather be the innermost cert?


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.

Sounds good, I can fix that.

Done in ab939cc

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

bkabrda · 2024-07-31T11:06:11Z

I updated the PR to add a test for deserializing and then again serializing a TrustedRoot instance and fixed couple minor issues related to that (plus linting issues). The test is failing now, only because the time instances are serialized in a different way - the provided sample has a format of "2021-03-07T03:20:29.000Z", while timestamppb serializes as "2021-03-07T03:20:29Z" (no 000Z at the end).

I don't immediately see a good way to fix the time serialization format, but I'll keep looking into it. I think this is a difference that we could potentially live with, but I also understand if you think this must be fixed before merging - let me know. Thanks!

bkabrda · 2024-08-01T11:33:20Z

FWIW, it looks like the .000 in the serialized time is intentionally stripped. I think changing that would be extremely hard and I don't think it's worth it (especially if technically we're emitting JSON that is the same, this is just a matter of formatting). I'll fix the test so that it ignores the .000 differences.

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

cmurphy

I don't think this needs additional documentation, the added public function has a godoc that I think is sufficient.

I think adding a short usage example in the docs/ folder would be great actually, just to make it a little more discoverable when people are wondering what different things sigstore-go can do.

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

bkabrda · 2024-08-02T09:31:14Z

Good point @cmurphy - I added a simple example of a binary that can construct and pretty print a trusted_root.json file - let me know what you think!

steiza · 2024-08-02T14:55:06Z

pkg/root/trusted_root_create.go

+//   - 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) {


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.

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.

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.

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.

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

bkabrda · 2024-08-09T12:47:15Z

@steiza @haydentherapper I believe I have now addressed all your concerns and that this should be acceptable. Can I ask you for another round of reviews?

FWIW I removed the example, because the only really useful added feature is the serialization, which I don't think needs an example. I will also change the description to match what the PR currently does.

steiza

This looks great - thank you!

steiza · 2024-08-09T15:03:45Z

pkg/root/trusted_root_create.go

+			return fmt.Errorf("failed converting rekor log %s to protobuf: %w", logID, err)
+		}
+		tr.trustedRoot.Tlogs = append(tr.trustedRoot.Tlogs, tlProto)
+		// ensure stable sorting of the slice


kommendorkapten · 2024-08-21T08:54:01Z

pkg/root/trusted_root_create.go

+	return &caProto, nil
+}
+
+func transparencyLogToProtobufTL(tl TransparencyLog) (*prototrustroot.TransparencyLogInstance, error) {


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?

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

kommendorkapten · 2024-08-21T08:55:06Z

pkg/root/trusted_root_create.go

+	return nil
+}
+
+func certificateAuthorityToProtobufCA(ca CertificateAuthority) (*prototrustroot.CertificateAuthority, error) {


Suggested change

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

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

Same here on receiving via value or pointer.

kommendorkapten · 2024-08-21T08:58:10Z

pkg/root/trusted_root_create.go

+		if err != nil {
+			return fmt.Errorf("failed converting fulcio cert chain to protobuf: %w", err)
+		}
+		tr.trustedRoot.CertificateAuthorities = append(tr.trustedRoot.CertificateAuthorities, caProto)


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

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

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

bkabrda · 2024-08-21T13:29:08Z

@kommendorkapten thanks again for your review, I just pushed a commit that addresses your points. I also refactored the sorting functionality to functions to avoid copy-paste code (and I realized it was mistakenly called in every iteration of the loops, while we just need to do it after the loops, so I fixed that as well). Let me know if this looks good now!

haydentherapper · 2024-08-21T21:12:26Z

pkg/root/trusted_root.go

@@ -51,6 +51,7 @@ type CertificateAuthority struct {
 	Leaf                *x509.Certificate
 	ValidityPeriodStart time.Time
 	ValidityPeriodEnd   time.Time
+	URI                 string


FYI @cmurphy, about the URI being added for TSAs

haydentherapper

Thanks!

bkabrda force-pushed the trusted-root-from-targets branch from 2302267 to 368c2a9 Compare July 30, 2024 09:28

bkabrda mentioned this pull request Jul 30, 2024

Add functionality to generate trusted_root.json by the TUF server sigstore/scaffolding#1191

Closed

Add the ability to contruct TrustRoot from targets

09d589c

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

bkabrda force-pushed the trusted-root-from-targets branch from 368c2a9 to 09d589c Compare July 30, 2024 11:00

haydentherapper requested review from cmurphy and kommendorkapten July 30, 2024 16:19

haydentherapper requested changes Jul 30, 2024

View reviewed changes

Address review, fix linting issues

ab939cc

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

bkabrda force-pushed the trusted-root-from-targets branch from b4094db to ab939cc Compare July 31, 2024 11:02

Fix test by stripping second fraction from input

9c28c32

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

cmurphy reviewed Aug 1, 2024

View reviewed changes

Add an example, fix a public key reading error case

54d5b4a

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

bkabrda force-pushed the trusted-root-from-targets branch from 8d0a810 to 54d5b4a Compare August 2, 2024 09:30

steiza requested changes Aug 2, 2024

View reviewed changes

Address review

1a04917

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

steiza previously approved these changes Aug 9, 2024

View reviewed changes

cmurphy mentioned this pull request Aug 20, 2024

Upgrade to TUF v2 client sigstore/cosign#3844

Draft

1 task

kommendorkapten reviewed Aug 21, 2024

View reviewed changes

Address review, pull out sorting functionality in separate functions

3800b3f

Signed-off-by: Slavek Kabrda <bkabrda@redhat.com>

bkabrda dismissed steiza’s stale review via 3800b3f August 21, 2024 13:27

bkabrda requested a review from a team as a code owner August 21, 2024 13:27

haydentherapper reviewed Aug 21, 2024

View reviewed changes

haydentherapper approved these changes Aug 21, 2024

View reviewed changes

haydentherapper merged commit 2198ac3 into sigstore:main Aug 21, 2024
10 checks passed

fghanmi mentioned this pull request Aug 30, 2024

Add functions: add/update/delete Targets in TrustedRoot sigstore/sigstore-rs#385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to contruct TrustRoot from targets #247

Add the ability to contruct TrustRoot from targets #247

bkabrda commented Jul 30, 2024 •

edited

Loading

haydentherapper Jul 30, 2024

bkabrda Jul 31, 2024

haydentherapper Jul 30, 2024

bkabrda Jul 31, 2024

bkabrda Jul 31, 2024 •

edited

Loading

bkabrda commented Jul 31, 2024

bkabrda commented Aug 1, 2024

cmurphy left a comment

bkabrda commented Aug 2, 2024

steiza Aug 2, 2024

bkabrda Aug 5, 2024

steiza Aug 7, 2024

bkabrda Aug 8, 2024

bkabrda commented Aug 9, 2024

steiza left a comment

steiza Aug 9, 2024

kommendorkapten Aug 21, 2024 •

edited

Loading

bkabrda Aug 21, 2024

kommendorkapten Aug 21, 2024

kommendorkapten Aug 21, 2024

bkabrda Aug 21, 2024

bkabrda commented Aug 21, 2024

haydentherapper Aug 21, 2024

haydentherapper left a comment

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

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

Add the ability to contruct TrustRoot from targets #247

Add the ability to contruct TrustRoot from targets #247

Conversation

bkabrda commented Jul 30, 2024 • edited Loading

Summary

Release Note

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkabrda Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

bkabrda commented Jul 31, 2024

bkabrda commented Aug 1, 2024

cmurphy left a comment

Choose a reason for hiding this comment

bkabrda commented Aug 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkabrda commented Aug 9, 2024

steiza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kommendorkapten Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkabrda commented Aug 21, 2024

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

bkabrda commented Jul 30, 2024 •

edited

Loading

bkabrda Jul 31, 2024 •

edited

Loading

kommendorkapten Aug 21, 2024 •

edited

Loading