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

certificate: accept pem format #829

Merged
merged 2 commits into from
Jul 18, 2020
Merged

certificate: accept pem format #829

merged 2 commits into from
Jul 18, 2020

Conversation

SSebo
Copy link
Contributor

@SSebo SSebo commented Jul 16, 2020

I found quinn::Certificate missing a from_pem function when I use pem format certificates, just add it.

@djc
Copy link
Member

djc commented Jul 16, 2020

Seems okay to me (modulo the formatting nit from CI -- ignore the clippy warning). @Ralith, any thoughts?

@Ralith
Copy link
Collaborator

Ralith commented Jul 16, 2020

I don't have a great grasp on the pem format itself; are there times when it'd be more appropriate to use this over CertificateChain::from_der?

@stammw
Copy link
Contributor

stammw commented Jul 17, 2020

Pem seems easier to handle from an ops point of view as most providers (i know) use this format. This PR simplify user's life by saving them the converting work IMHO.

@Ralith
Copy link
Collaborator

Ralith commented Jul 17, 2020

Er, sorry, that was a typo. I meant to ask whether there are situations where you can't use CertificateChain::from_pem. There is no CertificateChain::from_der.

@SSebo
Copy link
Contributor Author

SSebo commented Jul 17, 2020

I found der format in server.rs example is:

let key = quinn::PrivateKey::from_der(&key)?;
let cert = quinn::Certificate::from_der(&cert)?;
server_config.certificate(quinn::CertificateChain::from_certs(vec![cert]), key)?;

I followed the example, think maybe pem format should be this way:

let key = quinn::PrivateKey::from_pem(&key)?;
let cert = quinn::Certificate::from_pem(&cert)?;
server_config.certificate(quinn::CertificateChain::from_certs(vec![cert]), key)?;

after I read @Ralith 's comment, I found following way is working as well:

let key = quinn::PrivateKey::from_pem(&key)?;
server_config.certificate(quinn::CertificateChain::from_pem(&cert.to_vec())?, key)?;

In my case, client.rs need this Certificate::from_pem:

client_config.add_certificate_authority(quinn::Certificate::from_pem(&cert)?)?;

Ralith
Ralith previously approved these changes Jul 17, 2020
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

@djc
Copy link
Member

djc commented Jul 17, 2020

@SSebo please fix up the formatting, then we can merge it! Thanks!

Signed-off-by: ssebo <ssebo.zzz@gmail.com>
@djc djc merged commit 2892490 into quinn-rs:master Jul 18, 2020
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