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

feat(tls): Use rustls_pki_types::CertificateDer to describe DER encoded certificate #1707

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

tottoto
Copy link
Collaborator

@tottoto tottoto commented May 24, 2024

Motivation

Although Certificate is for PEM encoded certificate, it is used by Request::peer_certs's return type, which is DER encoded certificate.

Solution

Adds CertificateDer type and changes Request::peer_certs to use it.

This is a breaking change.

@djc
Copy link
Contributor

djc commented May 25, 2024

So I'm guessing the name is inspired by rustls_pki_types::CertificateDer<'_>. Given that rustls-pki-types is intended to have a long-term stable API, I would like to suggest that we adopt usage of types like CertificateDer<'_> in the tonic API directly instead of copying them here. This would ensure improved type safety and clarity across crates, and should cause no public API churn due to the stability promise.

@LucioFranco I'm biased here because I created rustls-pki-types -- what do you think? It's basically a collection of very lightweight wrappers for Vec<u8> and &[u8], with optional no_std support.

@tottoto
Copy link
Collaborator Author

tottoto commented May 26, 2024

So I'm guessing the name is inspired by rustls_pki_types::CertificateDer<'_>.

Yes. introduced CertificateDer is a type for not exposing rustls-pki-types crate. I think both exposing rustls-pki-types and hiding it are reasonable, so I am not sure which is more appropriate.

@tottoto
Copy link
Collaborator Author

tottoto commented May 26, 2024

Added a commit to show how the implementation would be when using rustls_pki_types::CertificateDer directly.

@djc
Copy link
Contributor

djc commented May 27, 2024

LGTM. I think the same reasoning as in #1710 should apply here: rustls-pki-types is expected to have a more stable API than tonic so it should be okay to use rustls-pki-types API in the public tonic API.

@tottoto tottoto changed the title feat(tls): Add CertificateDer to describe DER encoded certificate feat(tls): Use rustls_pki_types::CertificateDer to describe DER encoded certificate May 27, 2024
@tottoto
Copy link
Collaborator Author

tottoto commented May 27, 2024

Updated this pull request title to reflect the latest status.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a 1.0 crate API seems fine to me.

@LucioFranco LucioFranco added this pull request to the merge queue Jun 10, 2024
Merged via the queue into hyperium:master with commit 96a8cbc Jun 10, 2024
16 checks passed
@tottoto tottoto deleted the request-peer-certs branch June 11, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants