Add PKCS8 Support

[kazk]

Squashed and rebased #147, then updated the tests to use test-cert-gen like the other tests.

Closes #27

---

• [x] Reject other formats in openssl and security_framework. Done by simply checking the start of the key.
• [x] Update from_pkcs8 tests to use PKCS#8 keys. test_cert_gen doesn't support it, so generated RSA keys are converted using openssl pkcs8 -topk8 -nocrypt.
• [x] Fix Windows failing two_servers test. Fixed by using unique container names.

[xinyufort]

I noticed this PR has been sitting for a few weeks now. This is also something that I'm hoping can land soon. Is there anything I can do to help get these changes merged into master?

(And thank you @kazk @Goirad for all the work you've put in so far!)

[kazk]

@sfackler Please approve running workflows.

[sfackler]

Here's a patch to fix the macOS build:

diff --git a/src/imp/security_framework.rs b/src/imp/security_framework.rs
index ca86cd5..f133903 100644
--- a/src/imp/security_framework.rs
+++ b/src/imp/security_framework.rs
@@ -423,6 +423,7 @@ impl<S: io::Read + io::Write> TlsStream<S> {
         Ok(self.stream.context().buffered_read_size()?)
     }

+    #[allow(deprecated)]
     pub fn peer_certificate(&self) -> Result<Option<Certificate>, Error> {
         let trust = match self.stream.context().peer_trust2()? {
             Some(trust) => trust,
diff --git a/src/test.rs b/src/test.rs
index 897bb85..5fe8151 100644
--- a/src/test.rs
+++ b/src/test.rs
@@ -186,7 +186,10 @@ fn peer_certificate() {
     let socket = p!(builder.connect("localhost", socket));

     let cert = socket.peer_certificate().unwrap().unwrap();
-    assert_eq!(cert.to_der().unwrap(), keys.client.ca.get_der());
+    assert_eq!(
+        cert.to_der().unwrap(),
+        keys.server.cert_and_key.cert.get_der()
+    );

     p!(j.join());
 }

[sfackler]

I will have to fix the OpenSSL issue tomorrow.

[sfackler]

Linux tests should be fixed if you merge in master.

[kazk]

@sfackler Thanks, rebased on the latest master. I need you to approve running workflows again :|

[kazk]

schannel doesn't support importing PEM encoded RSA key at the moment. https://github.com/steffengy/schannel-rs/pull/64#issuecomment-576995977

I don't have Windows machine to test, but looks like it can be decoded with

let res = wincrypt::CryptDecodeObjectEx(wincrypt::X509_ASN_ENCODING |
                                        wincrypt::PKCS_7_ASN_ENCODING,
                                        wincrypt::PKCS_RSA_PRIVATE_KEY, // <-
                                        der.as_ptr(),
                                        der.len() as winapi::DWORD,
                                        wincrypt::CRYPT_DECODE_ALLOC_FLAG,
                                        ptr::null_mut(),
                                        &mut buf as *mut _ as winapi::LPVOID,
                                        &mut len);

[jethrogb]

I'm not sure why you say PEM is not supported and then link to a PR implementing exactly that? Are you trying a PKCS#1 or a PKCS#8 key?

[kazk]

I linked to the comment saying:

Turns out it rejects PKCS1 keys. I've added a test to that effect.

They added explicit check:

        if pem_str.starts_with("-----") {
            if !pem_str.starts_with("-----BEGIN PRIVATE KEY-----") ||
               !pem_str.ends_with("-----END PRIVATE KEY-----") {
                return Err(io::Error::new(io::ErrorKind::InvalidData,
                                          "expected '-----BEGIN PRIVATE KEY-----'\
                                          and '-----END PRIVATE KEY-----' PEM guards"));
            }
        }

Tests are failing on Windows here because we're testing with PEMs having BEGIN RSA PRIVATE KEY (PKCS1) headers.

As far as I can tell, "Turns out it rejects PKCS1 keys." is because it failed to decode DER encoded key. The snippet I posted above should be able to decode RSA key, and schannel should be able to support both by using the headers to detect the kind.

[jethrogb]

Where is the key coming from? I don't see it in the PR.

Anyway, the feature is to add PKCS8 support, the test should use a PKCS8 key.

[kazk]

Keys are generated by test_cert_gen crate.

[kazk]

Anyway, the feature is to add PKCS8 support, the test should use a PKCS8 key.

The function should probably be renamed. schannel is the only one that ignores the headers in PEM (should be possible to fix this). I think most users want a way to create an Identity from a certificate chain and a private key for client authentication, and not PKCS8 support specifically. I don't think it's a good idea to limit to PKCS8 key because users will have to parse the PEM and select a specific function to call (unless native_tls adds a wrapper function later), and native_tls will have to add separate functions wrapping each backend.

rustls supports creating config with a certificate chain, and a matching private key. They can add support for more key types without breaking changes. I think native-tls should provide something similar. The key types supported by each backend may vary, but that's already the case with PKCS12.

[jethrogb]

native-tls is about providing standard, cross-platform primitives. I strongly believe we should standardize on PKCS8 for the private key format as it supports all key types. I also think blindly accepting different formats in the same function adds to the general confusion about key formats. Adding PKCS1 support separately may be done but I don't really see the appeal. It's pretty easy to convert PKCS1 keys to PKCS8.

users will have to parse the PEM and select a specific function to call

I don't understand what you mean by this.

[kazk]

Just to be clear, only accepting PKCS#8 is fine if that's what is best for native_tls. I'm not against it. I just thought it's more ergonomic to have a function to create an Identity from a certificate chain and a PEM-encoded private key, and thought it's easier for native-tls because it can let each backend handle the PEM. This PR (#147) currently works that way, and from_pkcs8 is misleading.

If we're going strict, we'll need to reject keys in openssl and security_framework.

I don't understand what you mean by this.

If native_tls provided separate functions for each key from_pkcs8/from_rsa/from_ecdsa etc., each user (library) will have to parse the PEM to find which one to call. It's pretty common to not have control over the private key format.

Adding PKCS1 support separately may be done but I don't really see the appeal. It's pretty easy to convert PKCS1 keys to PKCS8.

If native-tls can provide a cross-platform way to convert between formats (without openssl dependency on macOS and Windows), that'll be great.

[sfackler]

I would be more than happy to also support PKCS1 (if for no other reason than to avoid all of the support issues from people with the wrong encoding :D), but whatever is implemented does definitely need to be consistent across the backends, either supporting or not supporting a format.

[xinyufort]

Oh wow... thank you all for the flurry of comments and commits!

While I'm mainly looking for PKCS 8 support (since in my case, I can require that the private keys are in PKCS 8 form), I agree with @kazk and @sfackler that if we are to limit the key format to PKCS 8, we need to do so consistently across all backends, and if we are to support something, all three backends should support it. (I'd also add that if we want to make a generic function that accepts keys of different formats (PKCS 1, PKCS 8, SEC 1, etc.), it shouldn't be named from_pkcs8, obviously :slightly_smiling_face:)

And again, do let me know if there's anything I can do to help!

[kazk]

I'll change to only support PKCS#8 format by rejecting anything else. I don't have a macOS/Windows machine to work on adding and testing other formats. Also, I found a way to convert PKCS#1 and SEC1 to PKCS#8 in pure Rust that might be good enough for my use case (started topk8).

I tried using topk8 to test Windows after converting PKCS#1 to PKCS#8, and it seems to work. But two_servers test panicked with:

• "unexpected EOF during handshake" on accept
• "The Local Security Authority cannot be contacted" on connect

Not sure what's wrong because server_pkcs8 test passes.

---

EDIT: Moved the task list to PR body

[kazk]

@sfackler I need you to approve running workflows again. I tested on my fork, and all tests should pass.

[jethrogb]

I have no experience with MacOS programming, and I see the P12 code also does this, but is there really no way to import key material without involving the filesystem?

[sfackler]

Yeah, unless things have changed since last time I looked into this, you can only get a SecIdentify out of a keychain, and keychains have to live on the filesystem.

[jethrogb]

What needs to be done before merge?