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

fix(tls): encode der to pem #1399

Closed
wants to merge 3 commits into from
Closed

Conversation

Direktor799
Copy link

Motivation

When using tonic with mtls, we expect the method peer_certs return PEM encoded certs since Certificate is described as a PEM cert in the doc, but it actually returns a DER encoded certs.

Solution

  • We could return the original tokio_rustls::rustls::Certificate with TlsStream::peer_certs since the TlsStream is import from tokio_rustls anyway.

  • As for Request::peer_certs, we could encode the DER certs with pem crate, so that it could be turned into a tonic::transport::Certificate properly. Now this method does more than just Arc::clone, so I removed the Arc in return value.

I doubt that anyone would want a PEM encoded cert form peer, but seems like we've been trying to get rid of rustls types, and it's not worthy to change the tonic::transport::Certificate to be compatible with DER for just this case.

@djc
Copy link
Contributor

djc commented Aug 31, 2023

I would dislike the requirement to pull in the pem crate only because the tls feature is being used. If you don't want to have rustls::Certificate in your public API, just yield a Vec<u8> or define your own trivial Certificate newtype?

(Note that rustls will in the future start using types from the new rustls-pki-types crate with a long-term stable API. Might be a decent alternative?)

@tottoto
Copy link
Collaborator

tottoto commented Jun 12, 2024

Addressed at #1707. Thank you!

@tottoto tottoto closed this Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants