[BUG] CA chains are not supported with native-tls

[reyk]

Describe the bug
Using a CA pem cert CertificateValidation::Full only supports a single certificate and breaks with CAs that require an intermediate CA.

To Reproduce
Steps to reproduce the behavior:

1. Create an intermediate CA
2. cat the root CA and the intermediate CA into one PEM file
3. Create a server cert for this CA
4. Try to connect an elasticsearch client to it

Expected behavior
Either accept an array of certs or split the PEM file into individual certs and call the underlying reqwest method multiple times.

Environment (please complete the following information):
native-tls

[russcam]

Thanks for opening @reyk. What OS are you running?

There seem to be differences in how native-tls works (or maybe more accurately how schannel and openssl is configured) across Windows, macOS and Linux (Ubuntu) which are captured in cert.rs.

It looks like reqwest creates a cursor over certs in a PEM formatted file with rust-tls, and adds each to the root store, but with native-tls, it simple delegates to native-tls:

    #[cfg(feature = "native-tls-crate")]
    pub(crate) fn add_to_native_tls(self, tls: &mut native_tls_crate::TlsConnectorBuilder) {
        tls.add_root_certificate(self.native);
    }

    #[cfg(feature = "rustls-tls")]
    pub(crate) fn add_to_rustls(self, tls: &mut rustls::ClientConfig) -> crate::Result<()> {
        use rustls::internal::pemfile;
        use std::io::Cursor;

        match self.original {
            Cert::Der(buf) => tls
                .root_store
                .add(&::rustls::Certificate(buf))
                .map_err(|e| crate::error::builder(TLSError::WebPKIError(e)))?,
            Cert::Pem(buf) => {
                let mut pem = Cursor::new(buf);
                let certs = pemfile::certs(&mut pem).map_err(|_| {
                    crate::error::builder(TLSError::General(String::from(
                        "No valid certificate was found",
                    )))
                })?;
                for c in certs {
                    tls.root_store
                        .add(&c)
                        .map_err(|e| crate::error::builder(TLSError::WebPKIError(e)))?;
                }
            }
        }
        Ok(())
    }

I'm wondering if this might be an issue to fix upstream?

[reyk]

I‘m using native-tls with OpenSSL (or LibreSSL) under Linux and OpenBSD.

It currently doesn’t split the CA bundle and just loads a single cert from it. Maybe this could eventually fix it one day: sfackler/rust-native-tls#168

You can also call reqwest’s add_root_certificate() multiple times as it pushes each new cert to a Vec internally. I’m using a workaround of CertificateValidation::Full(Vec<Certificate>) right now which allows me to split the PEM in advance and to call that method for each cert in it.

I wish there would be a better option but I didn’t see one without patching elasticsearch or native-tls (which is far more intrusive, so I opted for the latter).

[russcam]

It currently doesn’t split the CA bundle and just loads a single cert from it. Maybe this could eventually fix it one day: sfackler/rust-native-tls#168

That looks promising, thanks for the link!

In the meantime though, I don't see harm in splitting a PEM formatted file and calling add_root_certificate() multiple times internally when native-tls feature is used. Is this something you'd be interested in contributing, @reyk? If not, it'd probably be later next week that I can get round to looking at it.

[reyk]

The problem is that you currently embed a single reqwest Certificate in the CertificateValidation. We could fix that by turning it into a Vec as described or by turning it into a newtype. Would you be interested in a change that breaks the current API or should it be done with a new, additional variant?

[russcam]

ah, yes. Hmm.

I'm fine with breaking the API at this stage as it's still an alpha release. I'm thinking that we should introduce our own type that accepts &[u8] in either PEM or DER format and handle splitting PEM inside of that, internally holding a Vec<Certificate>. I think it's fairly trivial for the library to take care of this and not force users to have to do the splitting themselves. What do you think?

[reyk]

See PR #101