Sigstore bundle #2204

kommendorkapten · 2022-08-26T13:17:30Z

Authored by @kommendorkapten and @patflynn

Summary

First iteration of the proposed new bundle format for cosign. See README.md for more details. The intent for this PR is not to be merged in this state, it is to start the discussion as decided on the design review meeting at 2022-08-22.

Before accepting, make sure these tasks are done:

Sigstore APIs/data formats: proto-first or JSON-first? community#136 concluded
Updated the binary fields to have the correct datatype.
Move shared messages (Certificate, CertificateChain, Signarture, etc) into a shared place and reuse those definitions.

Release Note

N/A as of now.

Documentation

N/A as of now.

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

…fields. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

…nto sigstore_bundle

kommendorkapten · 2022-08-26T13:19:38Z

Ping: @asraa @haydentherapper @dlorenc @bobcallaway @znewman01 @priyawadhwa

kommendorkapten · 2022-08-26T13:25:29Z

Rendered README.md
Rendered verification_flows.md

specs/dotsigstore_bundle/pb/bundle.proto

znewman01

Thanks for doing this! It's a lot easier to hash out the important details in this format.

Sorry for the million nitpicks, on the whole I think it's looking good 😄

specs/dotsigstore_bundle/pb/bundle.proto

asraa · 2022-08-30T16:53:41Z

specs/dotsigstore_bundle/verifier.go

+		}
+	}
+
+	if b.rekorEntry != nil {


is it correct that you must also enter this path if/when a certificate (if used) is expired?

likewise, does the final cert validation wrt to timestamp need to occur here or in the outer loop?

This will be addressed in an updated coming soon, have it locally but need to run it through once more before pushing. Will add explicit logic for cert validation wrt to timestamp in verifyRekor.

patflynn · 2022-08-31T13:55:34Z

Fredrik is out for a couple of weeks, one approach to iterating on this would be to merge as is, and then we can work through all the comments on this PR, generating new PRs as we go. It's going into a temporary spot anyway, WDYAT?

znewman01 · 2022-08-31T14:13:43Z

I think I'd prefer to work on a branch/fork, I don't think the diff between "original pass" and "latest pass" is meaningful in this case. Then we don't have to worry about tests etc.

…osed during review. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

znewman01 · 2022-10-17T13:53:44Z

Verifiers should only reconstruct the Rekor entry when there is at least one Rekor entry in the bundle, as there is no requirement that the entry was ever put on any Rekor instance.

I think this is main point of disagreement. I was imagining TSAs signing off on Rekor entries, not just the signature bytes.

This just feels more consistent to me. If there are multiple TransparencyLogEntrys from multiple Rekors, I'd expect that they signed the exact same Rekor entry. Why would the thing that the TSA signs be any different?

Maybe relevant, maybe not: there are [subtle crypto attacks](https://www.bolet.org/~pornin/2005-acns-pornin+stern.pdf](can work backwards) to create a new public key that a fixed signature validates against. I don't think this is actually exploitable in our setting, but IMO the more context in the data that gets countersigned the better.

Currently there are sufficient information in the bundle to reconstruct the entry iff the Rekor kind is either intoto or hashedrecord, as those two was the only one being discussed initially. The conent in the bundle is a oneof, this can be extended if we need to capture more types.

So, to clarify the plan is "each Rekor kind/version has a corresponding content type"? For instance, for Alpine we would add a new one that included the PKGINFO data.

If that's the case, I think I'd want to be more explicit about the mapping. So the doc for content would say: "hashedrekord 0.0.1 => MessageSignature, intoto 0.0.1 => ...".

But then why not just use the Rekor types directly?

Also, what about downstream users who might want to verify other Rekor types? They're supposed to be pluggable, the bundle format should be too IMO.

This will of course make the M x N verification flow grow in each client library. But I don't see this as a huge issue, it's only a mapping from the data in the bundle to the correct entry in Rekor that can be properly canonicalized so the verification can happen. It's still code, but not that complex, it must only be written once, and is easy to test for correctness.

I would be very worried if client libraries needed to reconstruct the entry by e.g. parsing an Alpine ".apk" file (which is what we'd have to do if we weren't adding another content value). If it's just canonicalizing data, I can live with that (though if we can avoid it, we should IMO).

The reason each Rekor entry contains a KindVersion is to make the structure align better with the response from Rekor,

IMO alignment with Rekor's response as it exists today should be a non-goal; I think there are several proposals floating around right now to improve it.

Alignment with Rekor's eventual response is a good idea. However, I think the bundle format should inform the Rekor API, not the other way around.

and as I would guess the most common scenario (at least initially) is to only have a single entry, I would argue this is a bit more developer friendly, and would prefer a bit of duplication to simplify the data structure.

I don't actually think this is more dev friendly. This means that my function to reconstruct the entry for verification needs to access both the TransparencyLogEntry which may or may not exist and the content. If you stuck all the information in content, that function is much simpler.

Yes, you are correct that if no Rekor entry exist in the bundle, there is no KindVersion, so this mean that the verifier can not verify it agains any Rekor instance, and have to resort to the signed RFC3161 timestamp during verification, which would be primarily to verify that it was signed during the time the certificate was valid.

To me, this complicates the verification flow unnecessarily. "If timestamp, check that the TSA signed X, otherwise, check that Rekor signed Y" seems more complicated than "filter the entries in TimestampVerificationData to find which ones signed X."

znewman01

Thanks for the comments, super helpful context

znewman01 · 2022-10-17T13:57:48Z

specs/dotsigstore_bundle/pb/sigstore_bundle.proto

+// policies.
+// When using RFC3161 timestamps together with DSSE envelopes, the message
+// imprint (message digest) MUST be computed over the payload (octets).
+message TimestampVerificationData {


IMHO this is not a real concern, today the bundle is only possible to map to two Rekor types: inoto and hashedrecord.

How deliberate is that? I think we should try to support all of them.

For the verification to succeed an asset in the bundle must be able to correctly be put on different logs with different types, which I would say is not possible. [...]

I worry a little about different versions of the same kind in this context. Not sure whether that's a big deal or not.

But I think you're right overall.

So this is now implicitly verified, but when talking about verification flows, we can make this explicit, that the verifier MUST verify that all entries are of the same type. How about same type with different versions? I think we should start with require the same version too, until we know of any concrete use-case where it may make sense to allow that.

+1

specs/dotsigstore_bundle/pb/sigstore_common.proto

znewman01 · 2022-10-17T14:04:32Z

specs/dotsigstore_bundle/pb/sigstore_rekor.proto

+        int64 log_index = 1;
+        // The hash digest stored at the root of the merkle tree at the time
+        // the proof was generated.
+        bytes root_hash = 2;


Personal bias here: I want as many types as possible always.

Specifically, every time I see a bytes I have to think "what format is that?" If I see HashOutput I think "that's the output of a hash algorithm, I don't care what format it is."

Agreed that it feels wonky to duplicate the algorithm. So maybe there's room for three (!!) types: HashOutput { bytes }, HashAlgorithm = enum and HashData = (HashOutput, HashAlgorithm). I understand if that seems like too much.

some out-of-band data is already needed, like the public key (which may include parameters/what hash function to use). So there is already a need for certain knowledge of the transparency log, and also sharing what hash function is used for computing the Merkle tree is IMHO not a problem.

I buy that. Should we try to enumerate all these parameters in a new type? That feels like a useful exercise. It would allow creating a cross-language test suite!

(Okay if that's a TODO and we don't do it in this PR)

patflynn · 2022-10-17T14:06:22Z

could we lean into this comment: "Could we push something marked experimental pretty soon (with a self-destruct clock, to prevent it from being entrenched), then try to build client libraries with it? I think that will help us shake out the rought edges."? Feels like we're never going to merge :). We can get this in, start implementation in the various clients, and iterate on the format.

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

znewman01 · 2022-10-17T14:30:09Z

could we lean into this comment: "Could we push something marked experimental pretty soon (with a self-destruct clock, to prevent it from being entrenched), then try to build client libraries with it? I think that will help us shake out the rought edges."? Feels like we're never going to merge :). We can get this in, start implementation in the various clients, and iterate on the format.

FWIW I'm pretty happy with this as-is. I still have one high-level concern (articulated in my last comment) but doesn't need to block the PR.

That said—what's the benefit of merging the experimental version to the Cosign repo? I think we decided it makes more sense in a new repo long-term anyway. So what if we just made that (even if it's just on a personal GH account for now), then we could merge whatever we want and do some more formal process to pull it back into the sigstore/ universe.

kommendorkapten · 2022-10-17T14:41:32Z

That said—what's the benefit of merging the experimental version to the Cosign repo? I think we decided it makes more sense in a new repo long-term anyway.

This is my preference to, like a repo namedsigstore/protobuf, or sigstore/protobuf-experimental.

kommendorkapten · 2022-10-17T14:52:41Z

@znewman01

I think this is main point of disagreement. ...

There are a few important topics in that comment, and this PR is already getting quite large. When we get a new repo where the bundle goes, how about I open up some issues with your different questions as I think they can be more efficiently covered separately?

edit: s/you/your

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

znewman01 · 2022-10-17T15:35:45Z

There are a few important topics in that comment, and this PR is already getting quite large. When we get a new repo where the bundle goes, how about I open up some issues with your different questions as I think they can be more efficiently covered separately?

Works for me!

haydentherapper · 2022-10-17T16:18:33Z

Meta comment - It seems like a lot has changed since the last time I looked at it, so I'll need to do another pass. Can we avoid any more significant changes before it's checked in, as it's hard to know what's changing between each pass?

…tion data' Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

znewman01 · 2022-10-20T12:53:34Z

A use case for key IDs: #1351

Some KMSes support key rotation, and the same key has different "versions." When we use KMS, we should stick the key version as a hint in the bundle.

bobcallaway · 2022-10-21T11:13:35Z

https://github.com/sigstore/protobuf-specs is ready whenever this can be moved over.

kommendorkapten · 2022-10-21T11:15:01Z

Thanks @bobcallaway, I'll move and close this PR today.

kommendorkapten · 2022-10-21T14:00:14Z

Closing this PR in favour of sigstore/protobuf-specs#1

This library: - Is testable. - Is consistent across the CLI. Related to (does not fix) sigstore#2296 and sigstore#2204. Signed-off-by: Zachary Newman <zjn@chainguard.dev>

This library: - Is testable. - Is consistent across the CLI. Related to (does not fix) #2296 and #2204. Signed-off-by: Zachary Newman <zjn@chainguard.dev> Signed-off-by: Zachary Newman <zjn@chainguard.dev>

kommendorkapten added 12 commits August 26, 2022 08:40

Added first draft of .sigstore bundle proposal.

a644fe5

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

s/Pass/check mark/

3cda2fb

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Use UTF8 chars for Pass/Fail for easier view.

b39ab7c

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Removed generated files. Added some comments and changed name of two …

1e490d9

…fields. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Added some comments on verify commands that relates to this proposal.

fe3b80f

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Updated readme and added comments to the command line parameters.

b38719c

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Added link to pb

503e307

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Added an example verifier.

5d1f224

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Clarified verifiction column

0b7ca83

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Merge branch 'sigstore:main' into sigstore_bundle

897bf16

Corrected attribute name in JSON serialization.

d279ce2

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Merge branch 'sigstore_bundle' of github.com:kommendorkapten/cosign i…

a8ea540

…nto sigstore_bundle

patflynn mentioned this pull request Aug 26, 2022

proper signing/verification for attestations sigstore/sigstore-js#64

Merged

bdehamer reviewed Aug 27, 2022

View reviewed changes