sigstore-rs icon indicating copy to clipboard operation
sigstore-rs copied to clipboard

Route away from experimental to 1.0

Open lukehinds opened this issue 2 years ago • 20 comments
trafficstars

I would like us to consider what would it look like where we are no longer experimental and can cut a release a 1.0.

Aspects to consider...

Do we have API stability? Are we performing conformance tests? Are we aligned with the protobuf client spec?

cc the codeowners:

@flavio @arsa @viccuad @Xynnn007

lukehinds avatar May 30 '23 16:05 lukehinds

cc'ing correct @asraa

+1 to integrating with the conformance suite - https://github.com/sigstore/sigstore-conformance#usage. Right now, there aren't many tests, but as we add more tests, this makes it easy to detect if a breaking change occurs for the client.

Also I would recommend producing and consuming bundles from https://github.com/sigstore/protobuf-specs. sigstore-python and sigstore-js should have examples of doing this.

haydentherapper avatar May 30 '23 21:05 haydentherapper

Would it also make sense to add FFI so other languages can interface?

domenkozar avatar Jun 02 '23 17:06 domenkozar

For the protobuf-specs: it looks like https://docs.rs/protobuf/latest/protobuf/ is the most mature protobuf implementation layer for Rust.

woodruffw avatar Jun 13 '23 16:06 woodruffw

To summarize discussions in the community meeting and Slack, here's my understanding of the current blockers for a 1.0 release of the Rust client:

  • [ ] Some degree of integration with the conformance testing suite
  • [ ] Support for the protobuf-specs formats, including the Sigstore bundle format
  • [ ] Inclusion proof verification, per https://github.com/sigstore/sigstore-rs/issues/274#issuecomment-1614939716

Are there other things I'm missing here? If not (and assuming it's okay with @lukehinds and the other maintainers here) I can begin assigning some people to work on these 🙂

woodruffw avatar Jun 13 '23 17:06 woodruffw

I have used the library quite a bit and did some internal modifications over the last months, and I have some (opinionated) feedback. I think the API can still be improved before being stabilized. I found workaround for most of my issues, but I think others might be less willing to do so.

Cosign API

  • I feel like some parts of the Cosign API are too "low level" and unclear about what is being verified.
    • trusted_signature_layers: I think this function could be removed from the public API, because it is somewhat confusing and it is not obvious what it does.
    • verify_constraints: I think this function could be moved into the the CosignCapabilities trait and replace trusted_signature_layers as part of the public API. The parameters could be a mixture of both. The name could be changed to something similar to the verify_blob* methods.
    • Is it the user's/OCI client's task to verify that image data and signature are consistent? The way I understand it, I can only assume the actual image data is authentic, if I'm fetching it with a reference that contains the digest and the OCI client ensures image data is consistent with the manifest.
  • Blob verification
    • I think the verify_blob* methods could use a variant that supports Fulcio based certificates + verification constraints.
    • Does it make sense to have verify_blob* methods for both bundle and raw signatures?
    • Does it make to use an enum for the 'verification key source' for public key, certs and Fulcio, instead of having functions for each?

Rekor API

  • I think adding a RekorClient could be useful to store the Rekor configuration, and as a wrapper for the API calls. This would not require a breaking change.
  • I think the API module structure could be flattened because the Rekor API isn't too big anyways. A RekorClient could be used to address this.
  • A lot of the Rekor data is encoded in some way during in the JSON models. In my opinion this is kind of annoying to work with. It is possible to use the serde_with crate to automatically decode a lot of these. E.g. decoding B64 signatures directly into a Vec<u8>.
  • In general, a lot of the structs can be modified to provide a better DX by using more serde features related to enum tagging an such. Though I'm not sure if this is desired because it would require some changes and be quite a change from the generated API bindings. Also I'm not sure if this is compatible with the protobuf specifications, as I'm not familiar with them.

Overall API

Does it make sense to split the crate into several subcrates? The way I understand it, this could improve development compile times, because it avoids having to recompile all the serde models for each minor. Though I'm unsure about this.

Future Features

Log proofs:

  • The way I understand it for now there's no support for inclusion proof, consistency proof and checkpoint/STH verification. Will these be added? I think adding them might be a breaking change depending on where they are added.
  • Thought, I'm not sure if running an inclusion proof in addition to verifying the SET would give a security advantage, as there is no gossiping.
  • Note: I have already implemented these for myself, and plan to create a PR for this.

vembacher avatar Jun 26 '23 08:06 vembacher

Thanks for your feedback @vembacher!

I'm in favor of stabilizing the API before moving forward with the 1.0 release.

  • Cosign changes: please, file some issues describing the changes to the API that you described. I'm in favor of them, but I need to see how that would impact the code I know about that is already using them
  • Rekor changes: I haven't been too much involved with this part of the code. I have the impression that maintaining the models is time expensive, see this comment and this issue

As for splitting the crate into subcrates, I'm not 100% sure about that. I think the changes above have a higher priority

flavio avatar Jun 27 '23 08:06 flavio

@vembacher I would like to see inclusion proof verification be a requirement of clients. The bundle format defines this as optional currently, but I've proposed making this a requirement in https://github.com/sigstore/rekor/issues/1566.

For consistency proofs and gossiping, this is not something that has been implemented in any Sigstore client yet. We could implement consistency proof verification in clients, but this requires clients to keep state (the previous checkpoints) (Note that I'm not opposed to implementing this as an optional feature for those who do want to, but I don't think it should be a requirement).

If we instead rely on a network of witnesses that compute consistency proofs, we get two benefits: Trusted witnesses verify consistency and handle state, and witnesses can distribute co-signed checkpoints to prevent split-view attacks. Witnesses would push co-signed checkpoints to a set of "distributors" - This is similar to gossiping, except witnesses do not gossip with one another, which makes witnesses far more lightweight. The operational burden is just on distributors, which Sigstore and community members can operate. See https://github.com/transparency-dev/witness/tree/main/cmd/omniwitness for an example.

The end goal is a strong offline proof of inclusion that bundles co-signed checkpoints. For now, for a 1.0 for any client, I'd like to just see inclusion proof verification implemented.

This has been my area of focus, happy to chat more over in a Rekor discussion/issue or Slack if you'd like!

haydentherapper avatar Jun 27 '23 17:06 haydentherapper

@flavio

Cosign changes: please, file some issues describing the changes to the API that you described. I'm in favor of them, but I need to see how that would impact the code I know about that is already using them

Will do as soon as I get around to it!

Rekor changes: I haven't been too much involved with this part of the code. I have the impression that maintaining the models is time expensive, see this comment and this issue

I agree, it definitely would increase required maintenance effort. I have already tried a lot of the changes for internal development, and if I remember correctly It took me a few hours to modify the models. However, in my opinion it would be worth the reduced effort on the user side, but I can see why you might want to avoid this.

Maybe this can be avoided with the protobuf specifications? Do they require Hex/B64 (de/en)coding? The way I understand it this is mostly necessary because of JSON compatibility issues.


@haydentherapper

@vembacher I would like to see inclusion proof verification be a requirement of clients. The bundle format defines this as optional currently, but I've proposed making this a requirement in sigstore/rekor#1566.

I will look into this proposal, but if there is an ongoing effort to require inclusion proofs it definitely makes a lot of sense to contribute my implementation.

For consistency proofs and gossiping, this is not something that has been implemented in any Sigstore client yet. We could implement consistency proof verification in clients, but this requires clients to keep state (the previous checkpoints) (Note that I'm not opposed to implementing this as an optional feature for those who do want to, but I don't think it should be a requirement).

If we instead rely on a network of witnesses that compute consistency proofs, we get two benefits: Trusted witnesses verify consistency and handle state, and witnesses can distribute co-signed checkpoints to prevent split-view attacks. Witnesses would push co-signed checkpoints to a set of "distributors" - This is similar to gossiping, except witnesses do not gossip with one another, which makes witnesses far more lightweight. The operational burden is just on distributors, which Sigstore and community members can operate. See https://github.com/transparency-dev/witness/tree/main/cmd/omniwitness for an example.

The end goal is a strong offline proof of inclusion that bundles co-signed checkpoints. For now, for a 1.0 for any client, I'd like to just see inclusion proof verification implemented.

I will need to look more into this concept. I think this is somewhat similar to what I implemented, but I need to check this in more-depth as soon as I have the time to do so.

My approach used what I call "federated gossiping", basically monitors can operate a gossiping endpoint to which other monitors and auditors can send checkpoints bundled with inclusion and consistency proofs. On the active gossiping side you can choose the monitors you want to gossip with. Effectively building a network that propagates all checkpoints to all federated monitors. The protocol itself however is only a proof-of-concept for now, but I can definitely at least try to contribute the consistency proof implementation.

But I think my use case is slightly different to the "default" one, because I focused on Operational Technology use cases. So scenarios where logs are operated by the publisher and potentially private needed to be covered. So there's a much higher risk of split-tree-view attacks compared to using the public instance.

This has been my area of focus, happy to chat more over in a Rekor discussion/issue or Slack if you'd like!

I'd be glad to! Should I just ping you on the Sigstore Slack as soon I get around to it?

vembacher avatar Jun 30 '23 08:06 vembacher

The design of witnesses and distributors differs in that witnesses are not addressable (distributors are) and there's not a O(N^2) fanout requiring every witnesses to gossip with other witnesses, which has been one of the major blockers for adoption of gossiping in CT.

Feel free to ping me on the Sigstore slack!

To summarize, for sigstore-rs 1.0, I'd propose inclusion proof verification be a requirement as per the about-to-be-updated bundle spec.

haydentherapper avatar Jun 30 '23 17:06 haydentherapper

To summarize, for sigstore-rs 1.0, I'd propose inclusion proof verification be a requirement as per the about-to-be-updated bundle spec.

This makes sense to me -- I'll add it to our list of tasks.

Edit: Updated the list above.

woodruffw avatar Jul 01 '23 00:07 woodruffw

For consistency proofs and gossiping, this is not something that has been implemented in any Sigstore client yet. We could implement consistency proof verification in clients, but this requires clients to keep state (the previous checkpoints) (Note that I'm not opposed to implementing this as an optional feature for those who do want to, but I don't think it should be a requirement).

FYI this is actually implemented in rekor-cli

bobcallaway avatar Jul 08 '23 10:07 bobcallaway

Would https://github.com/theupdateframework/rust-tuf be usable for the TUF integration rather than https://github.com/sigstore/sigstore-rs/tree/main/src/tuf?

haydentherapper avatar Aug 22 '23 20:08 haydentherapper

I was just curious if there's an estimated timeline for this? It's something I'm interested in using in production in the somewhat near future. Specifically looking to verify a signature bundle, including the Rekor inclusion.

jasperpatterson avatar Aug 29 '23 16:08 jasperpatterson

I was just curious if there's an estimated timeline for this? It's something I'm interested in using in production in the somewhat near future. Specifically looking to verify a signature bundle, including the Rekor inclusion.

I don't believe there's currently a firm timeline here, unfortunately -- we've recently merged some upstream work (https://github.com/sigstore/protobuf-specs/pull/118) that'll unblock bundle signing and verification here, but I can't offer a firm timeline on when that'll get integrated here.

woodruffw avatar Aug 29 '23 16:08 woodruffw

Would https://github.com/theupdateframework/rust-tuf be usable for the TUF integration rather than https://github.com/sigstore/sigstore-rs/tree/main/src/tuf?

This crate didn't exist when I added the TUF integration. The code is currently using https://github.com/awslabs/tough - which is a comparable client written and maintained by AWS (they use that as part of bottlerocket and maybe other production code).

I don't see any particular advantage in moving from the tough crate to rust-tuf

flavio avatar Aug 30 '23 08:08 flavio

Revisiting this:

To summarize discussions in the community meeting and Slack, here's my understanding of the current blockers for a 1.0 release of the Rust client:

Are there other things I'm missing here? If not (and assuming it's okay with @lukehinds and the other maintainers here) I can begin assigning some people to work on these 🙂

We've made progress towards (1) and (2) in #310, now #311, and the small soon-to-be follow-up #315. These are the features we're tracking for a 1.0 from Trail of Bits' side.

Is there anything else being tracked as a requisite feature for cutting a 1.0 release?

jleightcap avatar Jan 31 '24 16:01 jleightcap

I think it would be nice to have https://github.com/sigstore/sigstore-rs/issues/307 done.

flavio avatar Feb 01 '24 09:02 flavio

Is there DSSE support? At a quick glance, I see references to the intoto type. Ideally we would migrate to uploading DSSE rekor entries.

haydentherapper avatar Feb 01 '24 15:02 haydentherapper

@jleightcap - https://github.com/sigstore/sigstore-rs/issues/274#issuecomment-1698701976 mentioned the use of the AWS TUF library, I don't think this is a blocker unless AWS is not actively supporting the library.

haydentherapper avatar Feb 01 '24 16:02 haydentherapper