add support for self-signed certificates #97

little-dude · 2017-05-19T02:10:51Z

This is untested, and based on my PR for hyper-native-tls (sfackler/hyper-native-tls#7), but I'd like to know if you think this is the right direction.

This is a step toward fixing #15

little-dude · 2017-05-22T15:57:23Z

hyper-native-tls now supports custom certificates.
I also updated the PR to also support hostname verification to be disabled.
It compiles, but I'll test this tonight for real.

seanmonstar

This is fantastic work! Thanks for starting it!

I have a thought, aimed at staying conservative and not exposing internal dependencies here.

What if instead of exposing NativeTlsClient and friends, at least to start with, we just added the ability to set certificates on reqwest::ClientBuilder? And by certificate, I mean also have a local reqwest::Certificate type, instead of hyper_native_tls::Certificate.

seanmonstar · 2017-05-23T01:43:19Z

Cargo.toml

@@ -11,7 +11,7 @@ categories = ["web-programming::http-client"]

 [dependencies]
 hyper = "0.10.2"
-hyper-native-tls = "0.2"
+hyper-native-tls = { git = "https://github.com/sfackler/hyper-native-tls" }


Just making a note here for myself to not merge until we can remove the git dependency.

seanmonstar · 2017-05-23T01:45:43Z

src/client.rs

+    }
+
+    /// Disable hostname verification
+    pub fn disable_hostname_verification(&mut self) {


Probably should have a scary name like the others, danger_disable_hostname_verification.

seanmonstar · 2017-05-23T01:46:02Z

src/client.rs

+        let mut tls_client = try_!(
+            self.inner.build().map_err(|e| ::hyper::Error::Ssl(Box::new(e))));
+        if ! self.hostname_verification {
+            tls_client.dannger_disable_hostname_verification(true);


dannger, with 2 ns?

Yes probably a typo, I'll make a PR to fix this.

It's been fixed already. I'll update the PR.

seanmonstar · 2017-05-23T01:51:29Z

src/client.rs

-        let mut client = try_!(new_hyper_client());
-        client.set_redirect_policy(::hyper::client::RedirectPolicy::FollowNone);
-        Ok(Client {
+impl From<NativeTlsClient> for Client {


If we did the conservative change like I suggest in the review comment, we'd probablt want to remove this impl.

little-dude · 2017-05-23T04:40:42Z

Hi @seanmonstar !

Thanks for the kind words :)

I'm not sure to understand your comment.
You'd prefer not to have a dependency to hyper_native_tls?
Or you just don't want to expose it (which means we need to re-export hyper_native_tls::Certificate as I did in 9b4edca)?

seanmonstar · 2017-05-23T05:33:09Z

reqwest already depends on hyper-native-tls, so I'm not suggesting we remove that. I'm saying let's see if we can prevent exposing that details as part of the public API. There is an interest in trying to get reqwest to a stable 1.0, so part of that requires not exporting or showing any types from other crates that aren't 1.0 stable yet.

So, to a user, all they see is a reqwest::ClientBuilder, and a reqwest::Certificate. As far as they know, it could be any TLS library underneath there. Sort of like with libflate: the user doesn't have any way of accidentally or purposefully touching something in the reqwest API that exposes that libflate is the gzip implementation used internally. Did I make that more clear?

little-dude · 2017-05-23T15:15:49Z

I think I got it. Do you thinkg the last two commits address this?

seanmonstar

Yes, exactly like that, great!

seanmonstar · 2017-05-23T17:42:57Z

src/client.rs

-        let mut client = try_!(new_hyper_client());
-        client.set_redirect_policy(::hyper::client::RedirectPolicy::FollowNone);
+/// Represent an X509 certificate.
+pub struct Certificate(hyper_native_tls::Certificate);


Yes, exactly!

seanmonstar · 2017-05-23T18:05:35Z

src/client.rs

+    /// let _ = ::std::fs::File::open("my-cert.der").unwrap().read_to_end(&mut buf).unwrap();
+    /// let cert = reqwest::Certificate::from_der(&buf).unwrap();
+    /// # }
+    /// ```


The Rust community is starting to encourage a different way of writing doc examples, that don't use unwrap and stuff. It could be written like this:

/// ``` /// # use std::fs::File; /// # fn cert() -> Result<(), Box<std::error::Error>> { /// let mut buf = Vec::new(); /// File::open("my_cert.der")? /// .read_to_end(&mut buf)?; /// let cert = reqwest::Certificate::from_der(&buf)?; /// # drop(cert); /// # Ok(()) /// # } /// ```

seanmonstar · 2017-05-23T18:07:18Z

src/client.rs

+///
+///     // [...]
+/// }
+/// ```


Similar comment about the example here: put it some sort of # fn foo() -> Result<(), Box<std::error::Error>>, and use ? instead of unwrap.

seanmonstar · 2017-05-23T18:08:10Z

src/client.rs

+/// }
+/// ```
+///
+pub struct TlsClient {


I actually liked that naming you have before, ClientBuilder. We can eventually move several of the config properties to the builder and remove the need for the Mutex and atomics...

seanmonstar · 2017-05-23T18:08:57Z

src/client.rs

+    }
+
+    /// Returns a `Client` that uses this `TlsClient` configuration.
+    pub fn into_client(self) -> ::Result<Client> {


Prefer the method name build.

seanmonstar · 2017-05-23T18:33:33Z

src/client.rs

                redirect_policy: Mutex::new(RedirectPolicy::default()),
                auto_referer: AtomicBool::new(true),
                auto_ungzip: AtomicBool::new(true),
            }),
        })
    }

+    /// Add a custom root certificate. This can be used to connect to a server that has a
+    /// self-signed certificate for example.
+    pub fn add_root_certificate(&mut self, cert: Certificate) -> ::Result<&mut TlsClient> {


Since we have a builder that needs to consume itself to create a final Client, we should probably follow the consuming builder guidelines.

Ugh, but at the same time, there are methods that could fail, such as this one, and a Result is needed. That'd mean that using the consuming pattern, you could lose the builder if it errors...

I'll have to think on this part.

This is how it's done in hyper-native-tls.

I've not been very consistent here, because danger_disable_hostname_verification and enable_hostname_verification don't return anything. I should probably make them return &mut ClientBuilder to be consistent with add_root_certificate, or return ClientBuilder as suggested in the guildelines.

little-dude · 2017-05-23T22:57:47Z

It's kind of unrelated to certificates, but I'm thinking that the ClientBuilder in this PR could be used to implement something similar to what @tshepang proposed in the proxy issue (#30).

let client = ClientBuilder::new()?.proxy_url(url).proxy_auth(auth).build()?;

I'd be interested in giving it a try if this get merged, what do you think?

seanmonstar · 2017-05-23T23:05:15Z

@little-dude yes, I'd expect proxy methods would be part of the ClientBuilder, that's why I liked it's name more than the specific TlsClient.

(For the actual API, I like the Proxy type spelled out in #30 personally, when we get there.)

little-dude · 2017-05-24T00:34:57Z

For some reason, this PR is not working in my case. Certificate verification still fails.

Error { code: 337047686, library: "SSL routines", function: "tls_process_server_certificate", reason: "certificate verify failed", file: "ssl/statem/statem_clnt.c", line: 1245 }

little-dude · 2017-05-24T01:19:59Z

Not sure how to debug that... both curl and wget refuse to use my der cert, but are ok with the pem version. Maybe it's because it's a bundle and I need to separate each certificate of the chain?

I'll try to find a public server with a self signed certificate to see if I have the same issue.

seanmonstar · 2017-05-26T20:22:26Z

So, this doesn't quite work?

little-dude · 2017-05-26T21:17:22Z

@seanmonstar I rebased, squashed, and added tests (poorly written).

I made it work yesterday with certificates I generated. But after making changed to the tests and automating certificates generation, it does not work anymore... but I'm still working on it. I'm not very familiar with TLS/openssl/certificates so I may be doing stupid mistakes.

Regarding the tests: I have a script that generates a root CA, and an intermediate CA. I use the intermediate CA to make two server certificates:

one with the CN localhost that I use to test the client with a custom CA and hostname verification enabled.
one with the CN wrong.hostname.comthat I use to test the client with a custom CA and hostname verification disabled.

Assuming I can make these tests pass, what are your feelings about needing a shell script to run, for the tests?

- test custom root certificate - test custom root certificate with hostname verification disabled

with this commit, tests hang sometimes. Not sure if this can be related to openssl/openssl#3505 since I also saw the tests hang when working on sfackler/hyper-native-tls#7 However, it seems to be more frequent here.

little-dude · 2017-05-27T21:41:55Z

I'm not sure I'll be able to debug the windows issue, so I'll close this for now, sorry...
To whoever wants to pick this up, there are two issues here:

in 12c71fa: there seems to be a problem with schannel. See this PR in hyper-native-tls for more details.
in eb76046: this commit is supposed to be a harmless refactoring of the tests, but it makes tests hang unpredicatbly. On my local linux machine, there's roughly 50% chance that one of the tests hang. As explained in the commit message, it might be due to an open openssl issue, but this is just a wild guess.

seanmonstar · 2017-05-31T17:10:48Z

I'm going to resurrect this, because there is a lot of good work in here! To start, we can just assume that rust-native-tls has tests of its own, and that we don't need to test that its own functionality does what it says. We can open a new issue for getting solid TLS tests running in reqwest on target platforms.

stephanbuys · 2017-05-31T18:21:01Z

I can test on MacOS, have a bit of experience with openssl and certs in general (nothing stellar), so please ping me if I can help.

seanmonstar · 2017-05-31T18:44:29Z

Re-opened in #124, without the TLS tests.

little-dude · 2017-05-31T19:44:36Z

wow thanks a lot @seanmonstar. I was kind of disappointed that I could no finish this up, so I'm glad to see this was not useless. Once openssl/openssl#3525 is merged, I'll try again to see if tests still hang on linux.

sfackler · 2017-06-02T03:29:02Z

There's a new version of openssl-sys that works around the OpenSSL issue.

little-dude · 2017-06-02T04:05:19Z

The tests still hang on my machine with openssl-sys 0.9.13.
Is there a way to debug this kind of stuff in rust? Something like pdb in python?

sfackler · 2017-06-02T04:10:54Z

gdb and lldb work.

little-dude · 2017-06-02T21:52:57Z

The hang is not directly related to openssl I think. Sometimes certificate verification fails, so the test panics, and the code that stops the server is never run:

#[test]
fn test_custom_ca_hostname_verification_disabled() {
    let listening = start_server("tests/certificates/server-wrong.hostname.com.pfx");
    let client = get_client(HostnameVerification::Disabled);
    // panic occurs here:
    // Error { kind: Http(Ssl(Failure(Ssl(ErrorStack([Error { code: 337047686, library: "SSL routines", function: "tls_process_server_certificate", reason: "certificate verify failed", file: "ssl/statem/statem_clnt.c", line: 1230 }]))))), url: Some("https://localhost:12345/") }
    let mut resp = client.get("https://localhost:12345").unwrap().send().unwrap();
    let mut body = vec![];
    resp.read_to_end(&mut body).unwrap();
    assert_eq!(body, b"ok");
    // this does not run
    mem::forget(listening);
}

After handling the error and calling listening.close() I don't see the hang anymore.
I just reopened a PR to try this against windows and mac.

The fact that sometimes certificate verification fails is not good though...

little-dude force-pushed the master branch from 610b0c9 to 0e4d51e Compare May 22, 2017 15:55

little-dude force-pushed the master branch 2 times, most recently from 4564a33 to 569d334 Compare May 22, 2017 16:27

seanmonstar reviewed May 23, 2017

View reviewed changes

little-dude force-pushed the master branch from f2c1da9 to 7905580 Compare May 23, 2017 04:36

little-dude mentioned this pull request May 23, 2017

new release sfackler/hyper-native-tls#9

Closed

seanmonstar reviewed May 23, 2017

View reviewed changes

little-dude force-pushed the master branch 2 times, most recently from 815012f to a99f168 Compare May 23, 2017 20:10

little-dude force-pushed the master branch 2 times, most recently from 226b6f5 to b9239b1 Compare May 24, 2017 04:31

little-dude added 2 commits May 26, 2017 14:07

tmp: use upstream git repo for hyper-native-tls

8bcb751

add custom certificates & disabling hostname verif

63caaff

little-dude force-pushed the master branch 2 times, most recently from 9472dae to 8e11deb Compare May 26, 2017 21:10

little-dude force-pushed the master branch 2 times, most recently from 6207e9f to edd7f71 Compare May 26, 2017 21:57

little-dude added 2 commits May 26, 2017 15:14

add certificates and script to rebuild them

44c37c7

test client with custom tls configuration

12c71fa

- test custom root certificate - test custom root certificate with hostname verification disabled

little-dude force-pushed the master branch from edd7f71 to 12c71fa Compare May 26, 2017 22:19

little-dude force-pushed the master branch 2 times, most recently from a30c012 to 12c71fa Compare May 27, 2017 00:45

cleanup ssl tests (fishy)

eb76046

with this commit, tests hang sometimes. Not sure if this can be related to openssl/openssl#3505 since I also saw the tests hang when working on sfackler/hyper-native-tls#7 However, it seems to be more frequent here.

little-dude closed this May 27, 2017

seanmonstar mentioned this pull request May 31, 2017

Fluent / Chained interface #76

Closed

seanmonstar mentioned this pull request May 31, 2017

add ClientBuilder and custom TLS certs #124

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for self-signed certificates #97

add support for self-signed certificates #97

little-dude commented May 19, 2017

little-dude commented May 22, 2017

seanmonstar left a comment

seanmonstar May 23, 2017

seanmonstar May 23, 2017

seanmonstar May 23, 2017

little-dude May 23, 2017

little-dude May 23, 2017

seanmonstar May 23, 2017

little-dude commented May 23, 2017 •

edited

seanmonstar commented May 23, 2017

little-dude commented May 23, 2017

seanmonstar left a comment

seanmonstar May 23, 2017

seanmonstar May 23, 2017

seanmonstar May 23, 2017

seanmonstar May 23, 2017

seanmonstar May 23, 2017

seanmonstar May 23, 2017

little-dude May 23, 2017

little-dude commented May 23, 2017

seanmonstar commented May 23, 2017

little-dude commented May 24, 2017

little-dude commented May 24, 2017

seanmonstar commented May 26, 2017

little-dude commented May 26, 2017

little-dude commented May 27, 2017 •

edited

seanmonstar commented May 31, 2017

stephanbuys commented May 31, 2017

seanmonstar commented May 31, 2017

little-dude commented May 31, 2017

sfackler commented Jun 2, 2017

little-dude commented Jun 2, 2017

sfackler commented Jun 2, 2017

little-dude commented Jun 2, 2017

add support for self-signed certificates #97

add support for self-signed certificates #97

Conversation

little-dude commented May 19, 2017

little-dude commented May 22, 2017

seanmonstar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

little-dude commented May 23, 2017 • edited

seanmonstar commented May 23, 2017

little-dude commented May 23, 2017

seanmonstar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

little-dude commented May 23, 2017

seanmonstar commented May 23, 2017

little-dude commented May 24, 2017

little-dude commented May 24, 2017

seanmonstar commented May 26, 2017

little-dude commented May 26, 2017

little-dude commented May 27, 2017 • edited

seanmonstar commented May 31, 2017

stephanbuys commented May 31, 2017

seanmonstar commented May 31, 2017

little-dude commented May 31, 2017

sfackler commented Jun 2, 2017

little-dude commented Jun 2, 2017

sfackler commented Jun 2, 2017

little-dude commented Jun 2, 2017

little-dude commented May 23, 2017 •

edited

little-dude commented May 27, 2017 •

edited