Switch TlsAcceptor::builder input from PKCS#12 to PKCS#8 + cert list

[briansmith]

For background, see https://unmitigatedrisk.com/?p=543 and https://www.cs.auckland.ac.nz/~pgut001/cryptlib/faq.html#Q5.

Although it seems simple for rust-native-tls to use PKCS#12 as its standard format for server key/certificate configuration, it is actually quite problematic.

First, PKCS#12 cryptography is stone-aged (3DES, RC2, etc.). This is a huge burden on any crypto library that itself wasn't forged in the stone ages of cryptography. Also, it is common for implementations to use way too few rounds of PBKDF2 (or PKBDF1), making the password-based encryption limited value unless the password itself is a strong random key.

Secondly, the interop is limited due to poor defaults and lack of following recommendations. For example, the AES-CBC encryption that the latest IETF spec recommends isn't supported by most (all?) versions of Windows. As another example, many versions of OpenSSL (and, IIRC, NSS) use RC2 as the default (in some cases only) encryption format.

Thirdly, it is pretty rare for private keys and certificate chains to be in PKCS#12 format. Usually they are in a collection of unencrypted PEM files. Even in the case where the TLS server does want to encrypt the private key key, usually that encryption is done using a key management system that (in my experience) isn't based on PKCS#12. For example, the ACME protocol (Let's Encrypt's certificate distribution protocol) defaults to PEM-based formats; see https://tools.ietf.org/html/draft-ietf-acme-acme-05#section-6.4.2.

Fourth, PKCS#12 is unnecessarily difficult to implement correctly and safely, and difficult to test and verify.

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). Then support for encrypted key/cert management, including PKCS#12, and also hopefully including better systems than PKCS#12, can be provided by separate crates.

[briansmith]

/cc @aidanhs. This is related to #26.

[sfackler]

PKCS#12 was not my first choice either, but it wasn't clear to me how to get a SecIdentity from separate key and cert files on the macOS side.

[shepmaster]

but it wasn't clear to me how to get a SecIdentity from separate key and cert files on the macOS side.

I'm sure I'm not adding anything that y'all don't already know, but just in case, it looks like SecIdentityCreateWithCertificate is useful:

    #[test]
    fn identity_pem() {
        let dir = TempDir::new("identity").unwrap();
        let keychain = keychain::CreateOptions::new()
                           .password("password")
                           .create(dir.path().join("identity.keychain"))
                           .unwrap();

        let mut items = SecItems::default();

        let data = include_bytes!("../../../test/localhost.crt");
        ImportOptions::new()
            .filename("localhost.crt")
            .items(&mut items)
            .keychain(&keychain)
            .import(data)
            .unwrap();

        let data2 = include_bytes!("../../../test/localhost.key");
        ImportOptions::new()
            .filename("localhost.key")
            .items(&mut items)
            .keychain(&keychain)
            .import(data2)
            .unwrap();

        unsafe {
            let mut x = ptr::null_mut();
            let ret = ::security_framework_sys::identity::SecIdentityCreateWithCertificate(keychain.as_concrete_TypeRef() as *const _, items.certificates[0].as_concrete_TypeRef(), &mut x);
            if ret != errSecSuccess {
                panic!("{:?}", Error::from_code(ret));
            }

            let x = SecIdentity::wrap_under_create_rule(x);
            panic!("{:?}", x);
        }
    }

Oh high-level inspection, this works:

	thread 'os::macos::import_export::test::identity_pem' panicked at 'SecIdentity { certificate: SecCertificate { subject: "Integer32, LLC" }, private_key: SecKey }', src/os/macos/import_export.rs:316

[sfackler]

Oh awesome! I somehow missed that method.

[shepmaster]

And if useful, the FFI signature (added to security-framework-sys/src/identity.rs):

    pub fn SecIdentityCreateWithCertificate(keychain_rr_array: CFTypeRef,
                                            certificate_ref: SecCertificateRef,
                                            identity_ref: *mut SecIdentityRef)
                                            -> OSStatus;

[sfackler]

Looks like that works just fine: kornelski/rust-security-framework@4ba1d68

I just need to add some similar functionality to schannel and we'll be able to get rid of the PKCS#12 silliness.

[sfackler]

The blocker is now on Windows. I have some WIP changes but can't get them to actually work: https://github.com/sfackler/schannel-rs/tree/key-import. If anyone knows how CryptoAPI works or has friends at Microsoft they can yell at about woefully insufficient documentation, help would be appreciated!

[steffengy]

@sfackler
interestingly enough switching KeySpec::signature() to KeySpec::key_exchange() gives
80090304 "The Local Security Authority Cannot Be Contacted" as error, but I also have no clue whats going on there. (the TLSStream InitializeSecurityContext call returns that error sec_e_internal_error)

[steffengy]

@sfackler
I compared your key_import branch against for example
https://github.com/zeroc-ice/ice/blob/master/cpp/src/IceSSL/SChannelEngine.cpp
and changed some stuff to match their implementation (key_exchange, provider, ...).

Still in split_cert_key during the initialization (InitializeSecurityContext) of the stream
(the client thread here) SEC_E_INTERNAL_ERROR is returned.

Feel free to take a look at it, maybe you'll spot something else.

As reference also the current state which returns that error for me as a patch:
https://gist.github.com/steffengy/245ecaa9125928cf788b7e9afdac8730

[jethrogb]

@sfackler this works for me on Windows 2016 jethrogb/schannel-rs@7c7660c (Note: I re-extracted the certificate from identity.p12, as it did not contain the right public key, which was not helpful in debugging)

NB a good strategy to get things working is to look at the CERT_KEY_PROV_INFO_PROP_ID from a key imported using ImportPfxOptions.

Note however that your code imports PKCS#1 RSAPrivateKey objects, not PKCS#8 PrivateKeyInfo objects. I haven't tried this, but I think you'd be able to parse PKCS#8 by calling CryptDecodeObjectEx twice, first with PKCS_PRIVATE_KEY_INFO and then with PKCS_RSA_PRIVATE_KEY on the PrivateKey member of CRYPT_PRIVATE_KEY_INFO.

Can we change TlsConnectorBuilder::identity too?

[sfackler]

Awesome! I'll look into the schannel side again with that help.

I imagine we would get rid of the Identity type in favor of a new PrivateKey type and the existing Certificate type.

[sfackler]

Success, thanks @jethrogb! steffengy/schannel-rs#31

[sfackler]

I have a prototype working in a branch: c7bc56e

Right now you have to provide separate DER files for all of the certificates. A much more common approach is to have a single file with a chain of PEM-formatted certificates. OpenSSL and Security.framework can parse that natively, but I don't think SChannel can, so there'll need to be some amount of extra work for that implementation.

[sfackler]

The server tests are also very flaky when running in parallel on OSX which is a bit concerning, but I believe that's an unrelated issue.

[nicklan]

So this looks much better than what's there now. Right now that branch looks like only TlsAcceptor gets the new stuff, but TlsConnector should have it too right? Any idea what the final API for this will look like?

[anowell]

I'm looking to wire this into reqwest and then consume it in a kubernetes client. It seems like using a similar API in reqwest would be ideal, so I'd also love to see the final API here.

[anowell]

And for reference, the overall changes proposed in this thread do seem right for my scenario where I'm pulling a cert and key from a kubernets conf file and currently building a Pkcs12 purely for the sake of plumbing them through to native-tls.

[Goirad]

@sfackler I am interested in completing this feature. Based on my reading of this thread and linked threads, it seems like it is almost done. What is missing to finish this?

[sfackler]

There's a (very out of date) branch with some in-progress work: https://github.com/sfackler/rust-native-tls/tree/private-key. I believe the Windows side of things was still a work in progress.