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

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

Open flavio opened this issue 1 year ago • 3 comments
trafficstars

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.

flavio avatar Apr 09 '24 11:04 flavio

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.

tnytown avatar Apr 09 '24 16:04 tnytown

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?

flavio avatar Apr 10 '24 07:04 flavio

@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 :)

tnytown avatar Apr 10 '24 17:04 tnytown