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

Route away from experimental to 1.0 #274

Open
lukehinds opened this issue May 30, 2023 · 19 comments
Open

Route away from experimental to 1.0 #274

lukehinds opened this issue May 30, 2023 · 19 comments
Labels
bug Something isn't working

Comments

@lukehinds
Copy link
Member

lukehinds commented May 30, 2023

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 lukehinds added the bug Something isn't working label May 30, 2023
@haydentherapper
Copy link
Contributor

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.

@domenkozar
Copy link

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

@woodruffw
Copy link
Member

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

@woodruffw
Copy link
Member

woodruffw commented Jun 13, 2023

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 🙂

@vembacher
Copy link
Contributor

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.

@flavio
Copy link
Member

flavio commented Jun 27, 2023

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

@haydentherapper
Copy link
Contributor

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

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!

@vembacher
Copy link
Contributor

@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?

@haydentherapper
Copy link
Contributor

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.

@woodruffw
Copy link
Member

woodruffw commented Jul 1, 2023

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.

@bobcallaway
Copy link
Member

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

@haydentherapper
Copy link
Contributor

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?

@jasperpatterson
Copy link

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.

@woodruffw
Copy link
Member

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 (sigstore/protobuf-specs#118) that'll unblock bundle signing and verification here, but I can't offer a firm timeline on when that'll get integrated here.

@flavio
Copy link
Member

flavio commented Aug 30, 2023

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

@jleightcap
Copy link
Contributor

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?

@flavio
Copy link
Member

flavio commented Feb 1, 2024

I think it would be nice to have #307 done.

@haydentherapper
Copy link
Contributor

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
Copy link
Contributor

@jleightcap - #274 (comment) mentioned the use of the AWS TUF library, I don't think this is a blocker unless AWS is not actively supporting the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

9 participants