Implement PKCS8 certificate support for all three backends. #147

Goirad · 2020-01-14T18:29:32Z

This solves #27 , although rather than removing support for PKCS12 it just adds support for PKCS8.

cc @jethrogb

jethrogb

I think there's too much use of unwrap/expect in this code, please review all uses for correctness.

src/imp/openssl.rs

src/lib.rs

src/test.rs

Cargo.toml

sfackler · 2020-01-16T02:28:20Z

src/imp/openssl.rs

@@ -265,7 +282,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() {


I do not believe this change is correct: 05fb5e5

When sending a certificate chain, extra chain certificates are sent in order following the end entity certificate.

This is from https://www.openssl.org/docs/manmaster/man3/SSL_CTX_add_extra_chain_cert.html , though looking at https://www.openssl.org/docs/manmaster/man3/PKCS12_parse.html I don't see any documentation about ordering restrictions for PKCS12_parse.
Edit: Looking again I think you might mean that the return certs from PKCS12_parse are in the opposite order, which seems awkward. Either the from_pkcs12 method or the from_pkcs8 method will need to reverse the certificate chain before creating the Identity if that is the case.

There really needs to be a cross-platform test for this behavior.

sfackler · 2020-01-16T02:29:41Z

src/imp/schannel.rs

+        let cert = CertContext::from_pem(std::str::from_utf8(leaf).map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "leaf cert contains invalid utf8"))?)?;
+
+        let mut options = AcquireOptions::new();
+        options.container("schannel");


I seem to remember there being problems on windows if the same container name is used multiple times. Probably worth a test that parses a couple of identities to confirm.

This is the test case that caused issues on my previous attempt: b0b9164

I think this test still needs to be added.

src/pem.rs

Cargo.toml

src/pem.rs

jethrogb · 2020-02-05T08:57:35Z

src/imp/openssl.rs

@@ -265,7 +282,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() {


There really needs to be a cross-platform test for this behavior.

src/lib.rs

svartalf · 2020-02-12T16:41:12Z

Hey! I'm quite interested in this PR, as it blocks me from the first release of my small app.
Is there anything I can help with to make it happen?

Goirad · 2020-02-12T22:16:58Z

@sfackler it looks like CI is failing due to an issue with the way rustdoc is testing README.md. I can't reproduce it locally.

@svartalf I wouldn't mind PR to implement the missing tests described above, the one that sfackler mentioned about reusing the container name or one for checking the certificate ordering.

sfackler · 2020-02-18T01:42:06Z

src/pem.rs

+#![allow(unused)]
+
+/// Split data by PEM guard lines
+pub struct PemBlock<'a> {


Since this is only used by the schannel backend, could you move it into that module?

sfackler · 2020-02-18T01:43:24Z

It's probably best to just remove the README test - that feature of rustdoc isn't super robust as the current failure shows.

Keruspe · 2020-08-20T07:26:33Z

@Goirad any chance of splitting the PemBlock part in its own commit on top of master (and maybe give it its own pull request) and rebase the rest on top of that?

It would be useful for #168 too

abonander · 2021-09-22T21:31:38Z

Hey folks, this PR is a bit stale but we'd like to have this functionality for SQLx: launchbadge/sqlx#1166 (comment)

If you have a certificate and private key, you have to work around this limitation by first bundling them with PKCS#12 and the only crate in Rust that you can do it with (that I'd trust, anyway) would be the openssl crate, but we don't want to require that on platforms where native-tls doesn't use it.

Any chance of getting this updated and merged?

sfackler · 2021-09-22T23:20:09Z

PRs welcomed! :D

I think this is (probably?) pretty close to being able to land with a rebase and a couple of bits of feedback resolved.

Definitely agree that this will be much easier to work with than PKCS#12.

abonander · 2021-09-22T23:28:46Z

I could probably find some time to take this to the finish line if @Goirad isn't available.

Goirad · 2021-09-23T02:00:50Z

I don't have time to pick this up again, sorry. Please go ahead and pick it up if you find the time.

kazk · 2021-11-07T12:07:11Z

If you have a certificate and private key, you have to work around this limitation by first bundling them with PKCS#12 and the only crate in Rust that you can do it with (that I'd trust, anyway) would be the openssl crate, but we don't want to require that on platforms where native-tls doesn't use it.

Yeah, we've been doing this because it's unavoidable. But requiring openssl is not enough anymore. macOS Security Framework can't import PKCS#12 with modern encryption algorithms (https://openradar.appspot.com/FB8988319), so this no longer works on macOS if a user chooses the vendored OpenSSL which was recently bumped to 3.0.0. Loading legacy provider is not possible yet sfackler/rust-openssl#1553 (blocked by openssl/openssl#16970).

I've squashed and rebased this PR, then updated the tests to use test-cert-gen (master...kazk:pkcs8), so I can open a new PR later, but I don't know what I'm doing :D

kazk · 2021-11-07T23:42:10Z

Opened #209

Implement PKCS8 certificate support for all three backends.

eb269b8

jethrogb reviewed Jan 14, 2020

View reviewed changes

src/imp/openssl.rs Outdated Show resolved Hide resolved

src/imp/openssl.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/test.rs Show resolved Hide resolved

Goirad added 3 commits January 14, 2020 12:40

added example and updated docs

c400390

revert unnecessary changes

1a6d7ba

add client auth to test

ab437ba

Goirad force-pushed the pkcs8-squashed branch from bc72aff to ab437ba Compare January 14, 2020 22:16

cleaned up unwraps and expects

beb8ebc

sfackler reviewed Jan 16, 2020

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

sfackler reviewed Jan 16, 2020

View reviewed changes

src/pem.rs Outdated Show resolved Hide resolved

steffengy mentioned this pull request Jan 23, 2020

add method to import PEM formatted PKCS8 keys steffengy/schannel-rs#64

Merged

use underlying libraries for pem parsing

28eade4

jethrogb reviewed Feb 5, 2020

View reviewed changes

svartalf mentioned this pull request Feb 7, 2020

Freeze TLS dependency version svartalf/unbound-telemetry#1

Closed

address some review comments

7e9a612

Goirad force-pushed the pkcs8-squashed branch from 177d3e8 to 7e9a612 Compare February 12, 2020 21:55

sfackler reviewed Feb 18, 2020

View reviewed changes

Goirad added 2 commits February 18, 2020 11:02

disable README tests, moved pem mod into schannel

da4cf1a

added multiple keys test

255dd54

Goirad force-pushed the pkcs8-squashed branch from 9976b62 to 255dd54 Compare February 18, 2020 19:41

abonander mentioned this pull request Sep 22, 2021

Added support for postgres certificate authentication for native tls launchbadge/sqlx#1166

Closed

kazk mentioned this pull request Nov 7, 2021

Add PKCS8 Support #209

Merged

3 tasks

sfackler closed this Mar 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PKCS8 certificate support for all three backends. #147

Implement PKCS8 certificate support for all three backends. #147

Goirad commented Jan 14, 2020 •

edited

Loading

jethrogb left a comment

sfackler Jan 16, 2020

Goirad Jan 16, 2020 •

edited

Loading

jethrogb Feb 5, 2020

sfackler Jan 16, 2020

sfackler Jan 18, 2020

sfackler Feb 18, 2020

jethrogb Feb 5, 2020

svartalf commented Feb 12, 2020

Goirad commented Feb 12, 2020

sfackler Feb 18, 2020

sfackler commented Feb 18, 2020

Keruspe commented Aug 20, 2020

abonander commented Sep 22, 2021 •

edited

Loading

sfackler commented Sep 22, 2021 •

edited

Loading

abonander commented Sep 22, 2021

Goirad commented Sep 23, 2021

kazk commented Nov 7, 2021

kazk commented Nov 7, 2021

Implement PKCS8 certificate support for all three backends. #147

Implement PKCS8 certificate support for all three backends. #147

Conversation

Goirad commented Jan 14, 2020 • edited Loading

jethrogb left a comment

Choose a reason for hiding this comment

sfackler Jan 16, 2020

Choose a reason for hiding this comment

Goirad Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

jethrogb Feb 5, 2020

Choose a reason for hiding this comment

sfackler Jan 16, 2020

Choose a reason for hiding this comment

sfackler Jan 18, 2020

Choose a reason for hiding this comment

sfackler Feb 18, 2020

Choose a reason for hiding this comment

jethrogb Feb 5, 2020

Choose a reason for hiding this comment

svartalf commented Feb 12, 2020

Goirad commented Feb 12, 2020

sfackler Feb 18, 2020

Choose a reason for hiding this comment

sfackler commented Feb 18, 2020

Keruspe commented Aug 20, 2020

abonander commented Sep 22, 2021 • edited Loading

sfackler commented Sep 22, 2021 • edited Loading

abonander commented Sep 22, 2021

Goirad commented Sep 23, 2021

kazk commented Nov 7, 2021

kazk commented Nov 7, 2021

Goirad commented Jan 14, 2020 •

edited

Loading

Goirad Jan 16, 2020 •

edited

Loading

abonander commented Sep 22, 2021 •

edited

Loading

sfackler commented Sep 22, 2021 •

edited

Loading