Add PKCS8 Support #209

kazk · 2021-11-07T23:41:50Z

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

Closes #27

Reject other formats in openssl and security_framework. Done by simply checking the start of the key.
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.
Fix Windows failing two_servers test. Fixed by using unique container names.

sfackler · 2021-11-08T03:33:02Z

src/imp/openssl.rs

+        let pkey = PKey::private_key_from_pem(key)?;
+        let mut cert_chain = X509::stack_from_pem(buf)?.into_iter();
+        let cert = cert_chain.next();
+        let chain = cert_chain.collect();


Isn't this collecting a vec directly back into a vec?

It's collecting the rest of the chain after taking the first.

sfackler · 2021-11-08T03:33:44Z

src/imp/openssl.rs

@@ -258,7 +270,10 @@ impl TlsConnector {
        if let Some(ref identity) = builder.identity {
            connector.set_certificate(&identity.0.cert)?;
            connector.set_private_key(&identity.0.pkey)?;
-            for cert in identity.0.chain.iter().rev() {
+            for cert in identity.0.chain.iter() {


Why is this order being reversed?

I don't know the details. Previous discussion: #147 (comment)

In 05fb5e5 you wrote:

Reverse chain loading for openssl
The stack is the reverse of what you might expect due to the way
PKCS12_parse is implemented, so we need to load it backwards.
Closes #110

If PKCS12_parse is the cause, maybe it's better to reverse the chain in from_pkcs12?

rust-native-tls/src/imp/openssl.rs

Line 161 in 82ae9a3

chain: parsed.chain.into_iter().flatten().collect(),

Added 7b7afc2 (#209)

src/imp/schannel.rs

src/imp/openssl.rs

src/imp/schannel.rs

src/imp/security_framework.rs

xinyufort · 2021-12-05T22:09:32Z

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 · 2021-12-08T02:15:34Z

@sfackler Please approve running workflows.

src/imp/security_framework.rs

sfackler · 2021-12-08T02:17:26Z

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 · 2021-12-08T02:30:30Z

I will have to fix the OpenSSL issue tomorrow.

sfackler · 2021-12-08T13:05:24Z

Linux tests should be fixed if you merge in master.

kazk · 2021-12-08T21:22:19Z

Keys are generated by test_cert_gen crate.

kazk · 2021-12-08T22:05:54Z

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 · 2021-12-08T22:16:45Z

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 · 2021-12-08T23:09:54Z

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 · 2021-12-09T01:26:19Z

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 · 2021-12-09T08:00:46Z

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 🙂)

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

kazk · 2021-12-10T23:31:14Z

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

Cargo.toml

kazk · 2021-12-11T06:26:58Z

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

kazk · 2021-12-11T06:28:03Z

src/imp/schannel.rs

+fn gen_container_name() -> String {
+    use std::sync::atomic::{AtomicUsize, Ordering};
+    static COUNTER: AtomicUsize = AtomicUsize::new(0);
+    format!("native-tls-{}", COUNTER.fetch_add(1, Ordering::Relaxed))


two_servers test fails without this.

jethrogb · 2021-12-12T11:59:40Z

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 · 2021-12-13T03:29:55Z

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.

src/imp/security_framework.rs

Cargo.toml

jethrogb · 2022-02-17T15:40:31Z

What needs to be done before merge?

This was referenced Nov 7, 2021

Implement PKCS8 certificate support for all three backends. #147

Closed

macOS Security Framework fails to import modern PKCS#12 created by OpenSSL 3 kube-rs/kube#691

Closed