Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement PKCS8 certificate support for all three backends. #147

Closed
wants to merge 9 commits into from

Conversation

Goirad
Copy link
Contributor

@Goirad Goirad commented Jan 14, 2020

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

cc @jethrogb

Copy link

@jethrogb jethrogb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Cargo.toml Outdated Show resolved Hide resolved
@@ -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() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not believe this change is correct: 05fb5e5

Copy link
Contributor Author

@Goirad Goirad Jan 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test still needs to be added.

src/pem.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/pem.rs Outdated Show resolved Hide resolved
@@ -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() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/lib.rs Outdated Show resolved Hide resolved
@svartalf
Copy link

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
Copy link
Contributor Author

Goirad commented Feb 12, 2020

@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.

src/pem.rs Outdated
#![allow(unused)]

/// Split data by PEM guard lines
pub struct PemBlock<'a> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sfackler
Copy link
Owner

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

@Keruspe
Copy link

Keruspe commented Aug 20, 2020

@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
Copy link

abonander commented Sep 22, 2021

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
Copy link
Owner

sfackler commented Sep 22, 2021

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
Copy link

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

@Goirad
Copy link
Contributor Author

Goirad commented Sep 23, 2021

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

@kazk
Copy link
Contributor

kazk commented Nov 7, 2021

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 kazk mentioned this pull request Nov 7, 2021
3 tasks
@kazk
Copy link
Contributor

kazk commented Nov 7, 2021

Opened #209

@sfackler sfackler closed this Mar 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants