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

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

Open
flavio opened this issue Apr 9, 2024 · 3 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@flavio
Copy link
Member

flavio commented Apr 9, 2024

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 flavio added the enhancement New feature or request label Apr 9, 2024
@tnytown
Copy link
Contributor

tnytown commented Apr 9, 2024

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 verify: init #311, so this should just be a matter of changing out the usages.

@flavio
Copy link
Member Author

flavio commented Apr 10, 2024

I prefer the 1st option you propose.

BTW, I've also created #348. I think we could avoid that if we were to address this issue. What do you think?

@tnytown
Copy link
Contributor

tnytown commented Apr 10, 2024

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

@flavio flavio added the good first issue Good for newcomers label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants