search

sigstore / sigstore-rs

An experimental Rust crate for sigstore
https://sigstore.github.io/sigstore-rs/sigstore/
Apache License 2.0
164 stars 51 forks source link

Do not expose the `rustls_pki_types::CertificateDer` object inside of public API #347

Open flavio opened 5 months ago

flavio commented 5 months ago

Description

The ManualTrustRoot struct leaks the rustls_pki_types::CertificateDer type inside of its public API.

Should we use instead the sigstore::registry::config::Certificate type? If we were to make this change, we would have to update also the sigstore::trust::ManualTrustRoot trait, since it's leaking this type too.

Moreover, the rustls_pki_types::CertificateDer has also an explicit lifetime, which leads all the structs embedding it to have a lifetime, which introduces complexity for the end users of the library. If we end up replacing the rustls_pki_types::CertificateDer type, we might use something that doesn't have an explicit lifetime.

Another possibility would be to leave the rustls_pki_types::CertificateDer, but re-export it. Right now downstream consumers of the sigstore-rs library have to introduce an explicit dependency against the rustls_pki_types crate to be able to interact with this type.

tnytown commented 5 months ago

Agreed -- we shouldn't leak this library type 🙂

Should we use instead the sigstore::registry::config::Certificate type?

I am wary of the Certificate type as it is currently designed: it allows for multiple representations (both DER and PEM) and is externally tagged with CertificateEncoding, making it somewhat prone to misuse.

In light of this, I see two options:

  1. We can fix up our Certificate type and use it. To do this, we can make it an enum or standardize on one representation internally (I suggest DER.) Call sites that accept PEM certificates can be reworked to convert to this new type.
  2. We can create a type alias for CertificateDer with the 'static lifetime. I've made usages of CertificateDer owned in #311, so this should just be a matter of changing out the usages.
flavio commented 5 months ago

I prefer the 1st option you propose.

BTW, I've also created https://github.com/sigstore/sigstore-rs/pull/348. I think we could avoid that if we were to address this issue. What do you think?

tnytown commented 5 months ago

@flavio Yes, in fact I think I've inadvertently resolved this with the changes I've made in #311 (the CertificatePool type there is fully owned again, obviating the issue). I'll let you know when it's ready to review :)