Support PKCS12 identity on the ClientBuilder

[anowell]

I wasn't certain how far into the weeds of TLS features reqwest wants to go, but this seemed straight-forward to add by following the patterns set by add_root_certificate to include the TlsConnectorBuilder::identity method, so I figured I'd send a PR and see what you thought.

I started using it for an internal tool that needs TLS client authorization to connect to a kubernetes API server. (It might become the beginnings of a kubernetes client crate).

[seanmonstar]

Cool, thanks! I'd certainly want to add the ability to specify client certificates (cc #43).

I recently noticed sfackler/rust-native-tls#27 complaining that PKCS12 is not ideal. Should thoughts in there apply here? Would another format be better? (I don't actually know, I've never needed to use client certificates myself.)

[anowell]

I'm really not an expert here. This is the first time I've ever needed a client cert here in a one-night hack kubernetes client

The comments on that thread resonate with me because I didn't have a PKCS12 to begin with, rather a collection of base64-encoded PEM-encoded strings (an X509 cert and a PKey) in a YAML file as seen here. I only went through the hoop of constructing the PKCS12 (as seen in the first link) because that's how the native-tls API currently works. So an API that takes a private key and a cert (chain?) makes perfect sense for my use case.

I'm happy to modify this PR to some alternative API. I presume the ideal scenario is to help that thread settle on and land the new API for rust-native-tls and then repeat that API in reqwest::ClientBuilder?

[seanmonstar]

I presume the ideal scenario is to help that thread settle on and land the new API for rust-native-tls and then repeat that API in reqwest::ClientBuilder?

That sounds good to me.

[seanmonstar]

So, we could close this until sfackler/rust-native-tls#27 settles down... Or we could try to merge a solution that uses both? If the type weren't called Pkcs12, we could just have some constructors that can read PKCS12 and 8 or whatever, and be able to support this now?

[anowell]

Given the other thread and disdain for PKCS12, I won't push for exposing PKCS12. This comment from that thread sounds about right to me for reqwest:

I propose instead that the constructor for TlsAcceptor be changed to accept an unencrypted PKCS#8 private key and a list of certificates (maybe just a list of DER-encoded certificates in &[u8] slices).

Digging through that discussion and this prototype method, it seems like reqwest could end up with something more like this:

fn identity(&mut self, key: PrivateKey, cert_chain: Vec<Certificate>) -> ::Result<&mut ClientBuilder>

If you have a particular API in mind you'd like to see, I'm happy to wire it up using Pkcs12 as a hidden implementation detail. It is blocking me from publishing a kubernetes client to crates.io, but I'm in no rush to publish a 0.1 quality crate. So if you'd rather wait on native-tls to settle on an API, feel free to close this issue; I'll focus on that thread and start a new PR when the dust settles over there.

[seanmonstar]

Does it make sense to have some sort of Identity type, such that it can be constructed by a PKCS#12 for now, and it can be constructed by a private key and cert chain in the future?

pub struct Identity {
    fields: Something
}

impl Identity {
    pub from_pkcs12(der: &[u8]) -> Result<Identity, Error> { ... }
}

impl ClientBuilder {
    pub fn identity(&mut self, ident: Identity);
}

Is that naming too generic? Should it be something more specific, like ClientCertificate or something?

[seanmonstar]

@anowell I opened #192 that adapts this PR with what I was suggesting. Does it seem like a good way forward?

[anowell]

Looks fine to me! Sorry I wasn't more on top of this over the last week. (insert lame traveling excuse)