Allow SNI and hostname verification to be configured separately

Closes #728

2 files changed, 101 insertions, 70 deletions

diff --git a/openssl/src/ssl/connector.rs b/openssl/src/ssl/connector.rs
index a730cc49..a91b46a1 100644
--- a/openssl/src/ssl/connector.rs
+++ b/openssl/src/ssl/connector.rs
@@ -132,13 +132,9 @@ impl SslConnector {
         self.configure()?.connect(domain, stream)
     }
 
-    /// Initiates a client-side TLS session on a stream without performing hostname verification.
-    ///
-    /// # Warning
-    ///
-    /// You should think very carefully before you use this method. If hostname verification is not
-    /// used, *any* valid certificate for *any* site will be trusted for use from any other. This
-    /// introduces a significant vulnerability to man-in-the-middle attacks.
+    #[deprecated(
+        since = "0.9.24",
+        note = "use `ConnectConfiguration::verify_hostname` and `ConnectConfiguration::use_server_name_indication` instead")]
     pub fn danger_connect_without_providing_domain_for_certificate_verification_and_server_name_indication<
         S,
     >(
@@ -149,53 +145,80 @@ impl SslConnector {
         S: Read + Write,
     {
         self.configure()?
-            .danger_connect_without_providing_domain_for_certificate_verification_and_server_name_indication(stream)
+            .use_server_name_indication(false)
+            .verify_hostname(false)
+            .connect("", stream)
     }
 
     /// Returns a structure allowing for configuration of a single TLS session before connection.
     pub fn configure(&self) -> Result<ConnectConfiguration, ErrorStack> {
-        Ssl::new(&self.0).map(ConnectConfiguration)
+        Ssl::new(&self.0).map(|ssl| ConnectConfiguration { ssl, sni: true, verify_hostname: true })
     }
 }
 
 /// A type which allows for configuration of a client-side TLS session before connection.
-pub struct ConnectConfiguration(Ssl);
+pub struct ConnectConfiguration {
+    ssl: Ssl,
+    sni: bool,
+    verify_hostname: bool,
+}
 
 impl ConnectConfiguration {
     #[deprecated(since = "0.9.23",
                  note = "ConnectConfiguration now implements Deref<Target=SslRef>")]
     pub fn ssl(&self) -> &Ssl {
-        &self.0
+        &self.ssl
     }
 
     #[deprecated(since = "0.9.23",
                  note = "ConnectConfiguration now implements DerefMut<Target=SslRef>")]
     pub fn ssl_mut(&mut self) -> &mut Ssl {
-        &mut self.0
+        &mut self.ssl
     }
 
-    /// Initiates a client-side TLS session on a stream.
+    /// Configures the use of Server Name Indication (SNI) when connecting.
     ///
-    /// The domain is used for SNI and hostname verification.
-    pub fn connect<S>(mut self, domain: &str, stream: S) -> Result<SslStream<S>, HandshakeError<S>>
-    where
-        S: Read + Write,
-    {
-        self.0.set_hostname(domain)?;
-        setup_verify_hostname(&mut self.0, domain)?;
-
-        self.0.connect(stream)
+    /// Defaults to `true`.
+    pub fn use_server_name_indication(mut self, use_sni: bool) -> ConnectConfiguration {
+        self.sni = use_sni;
+        self
     }
 
-    /// Initiates a client-side TLS session on a stream without performing hostname verification.
+    /// Configures the use of hostname verification when connecting.
     ///
-    /// The verification configuration of the connector's `SslContext` is not overridden.
+    /// Defaults to `true`.
     ///
     /// # Warning
     ///
     /// You should think very carefully before you use this method. If hostname verification is not
     /// used, *any* valid certificate for *any* site will be trusted for use from any other. This
     /// introduces a significant vulnerability to man-in-the-middle attacks.
+    pub fn verify_hostname(mut self, verify_hostname: bool) -> ConnectConfiguration {
+        self.verify_hostname = verify_hostname;
+        self
+    }
+
+    /// Initiates a client-side TLS session on a stream.
+    ///
+    /// The domain is used for SNI and hostname verification if enabled.
+    pub fn connect<S>(mut self, domain: &str, stream: S) -> Result<SslStream<S>, HandshakeError<S>>
+    where
+        S: Read + Write,
+    {
+        if self.sni {
+            self.ssl.set_hostname(domain)?;
+        }
+
+        if self.verify_hostname {
+            setup_verify_hostname(&mut self.ssl, domain)?;
+        }
+
+        self.ssl.connect(stream)
+    }
+
+    #[deprecated(
+        since = "0.9.24",
+        note = "use `ConnectConfiguration::verify_hostname` and `ConnectConfiguration::use_server_name_indication` instead")]
     pub fn danger_connect_without_providing_domain_for_certificate_verification_and_server_name_indication<
         S,
     >(
@@ -205,7 +228,7 @@ impl ConnectConfiguration {
     where
         S: Read + Write,
     {
-        self.0.connect(stream)
+        self.use_server_name_indication(false).verify_hostname(false).connect("", stream)
     }
 }
 
@@ -213,13 +236,13 @@ impl Deref for ConnectConfiguration {
     type Target = SslRef;
 
     fn deref(&self) -> &SslRef {
-        &self.0
+        &self.ssl
     }
 }
 
 impl DerefMut for ConnectConfiguration {
     fn deref_mut(&mut self) -> &mut SslRef {
-        &mut self.0
+        &mut self.ssl
     }
 }
 
diff --git a/openssl/src/ssl/tests/mod.rs b/openssl/src/ssl/tests/mod.rs
index 1cc36c7f..ff1d1c86 100644
--- a/openssl/src/ssl/tests/mod.rs
+++ b/openssl/src/ssl/tests/mod.rs
@@ -6,10 +6,10 @@ use std::io::prelude::*;
 use std::io::{self, BufReader};
 use std::iter;
 use std::mem;
-use std::net::{TcpStream, TcpListener, SocketAddr};
+use std::net::{SocketAddr, TcpListener, TcpStream};
 use std::path::Path;
-use std::process::{Command, Child, Stdio, ChildStdin};
-use std::sync::atomic::{AtomicBool, ATOMIC_BOOL_INIT, Ordering};
+use std::process::{Child, ChildStdin, Command, Stdio};
+use std::sync::atomic::{AtomicBool, Ordering, ATOMIC_BOOL_INIT};
 use std::thread;
 use std::time::Duration;
 use tempdir::TempDir;
@@ -18,10 +18,9 @@ use dh::Dh;
 use hash::MessageDigest;
 use ocsp::{OcspResponse, RESPONSE_STATUS_UNAUTHORIZED};
 use ssl;
-use ssl::{SslMethod, HandshakeError, SslContext, SslStream, Ssl, ShutdownResult,
-          SslConnectorBuilder, SslAcceptorBuilder, Error, SSL_VERIFY_PEER, SSL_VERIFY_NONE,
-          STATUS_TYPE_OCSP};
-use x509::{X509StoreContext, X509, X509Name, X509_FILETYPE_PEM};
+use ssl::{Error, HandshakeError, ShutdownResult, Ssl, SslAcceptorBuilder, SslConnectorBuilder,
+          SslContext, SslMethod, SslStream, SSL_VERIFY_NONE, SSL_VERIFY_PEER, STATUS_TYPE_OCSP};
+use x509::{X509, X509Name, X509StoreContext, X509_FILETYPE_PEM};
 #[cfg(any(all(feature = "v102", ossl102), all(feature = "v110", ossl110)))]
 use x509::verify::X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS;
 use pkey::PKey;
@@ -35,7 +34,7 @@ static CERT: &'static [u8] = include_bytes!("../../../test/cert.pem");
 static KEY: &'static [u8] = include_bytes!("../../../test/key.pem");
 
 fn next_addr() -> SocketAddr {
-    use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT, Ordering};
+    use std::sync::atomic::{AtomicUsize, Ordering, ATOMIC_USIZE_INIT};
     static PORT: AtomicUsize = ATOMIC_USIZE_INIT;
     let port = 15411 + PORT.fetch_add(1, Ordering::SeqCst);
 
@@ -104,15 +103,13 @@ impl Server {
 
     #[allow(dead_code)]
     fn new_alpn() -> (Server, TcpStream) {
-        Server::new_tcp(
-            &[
-                "-www",
-                "-nextprotoneg",
-                "http/1.1,spdy/3.1",
-                "-alpn",
-                "http/1.1,spdy/3.1",
-            ],
-        )
+        Server::new_tcp(&[
+            "-www",
+            "-nextprotoneg",
+            "http/1.1,spdy/3.1",
+            "-alpn",
+            "http/1.1,spdy/3.1",
+        ])
     }
 }
 
@@ -157,10 +154,9 @@ macro_rules! run_test(
     );
 );
 
-run_test!(
-    new_ctx,
-    |method, _| { SslContext::builder(method).unwrap(); }
-);
+run_test!(new_ctx, |method, _| {
+    SslContext::builder(method).unwrap();
+});
 
 run_test!(verify_untrusted, |method, stream| {
     let mut ctx = SslContext::builder(method).unwrap();
@@ -309,7 +305,7 @@ run_test!(verify_callback_data, |method, stream| {
 });
 
 run_test!(ssl_verify_callback, |method, stream| {
-    use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT, Ordering};
+    use std::sync::atomic::{AtomicUsize, Ordering, ATOMIC_USIZE_INIT};
 
     static CHECKED: AtomicUsize = ATOMIC_USIZE_INIT;
 
@@ -437,9 +433,9 @@ fn test_read() {
     let mut stream = Ssl::new(&ctx.build()).unwrap().connect(tcp).unwrap();
     stream.write_all("GET /\r\n\r\n".as_bytes()).unwrap();
     stream.flush().unwrap();
-    io::copy(&mut stream, &mut io::sink()).ok().expect(
-        "read error",
-    );
+    io::copy(&mut stream, &mut io::sink())
+        .ok()
+        .expect("read error");
 }
 
 #[test]
@@ -994,9 +990,8 @@ fn verify_valid_hostname() {
     ctx.set_verify(SSL_VERIFY_PEER);
 
     let mut ssl = Ssl::new(&ctx.build()).unwrap();
-    ssl.param_mut().set_hostflags(
-        X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS,
-    );
+    ssl.param_mut()
+        .set_hostflags(X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS);
     ssl.param_mut().set_host("google.com").unwrap();
 
     let s = TcpStream::connect("google.com:443").unwrap();
@@ -1019,9 +1014,8 @@ fn verify_invalid_hostname() {
     ctx.set_verify(SSL_VERIFY_PEER);
 
     let mut ssl = Ssl::new(&ctx.build()).unwrap();
-    ssl.param_mut().set_hostflags(
-        X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS,
-    );
+    ssl.param_mut()
+        .set_hostflags(X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS);
     ssl.param_mut().set_host("foobar.com").unwrap();
 
     let s = TcpStream::connect("google.com:443").unwrap();
@@ -1057,7 +1051,12 @@ fn connector_invalid_no_hostname_verification() {
     let connector = SslConnectorBuilder::new(SslMethod::tls()).unwrap().build();
 
     let s = TcpStream::connect("google.com:443").unwrap();
-    connector.danger_connect_without_providing_domain_for_certificate_verification_and_server_name_indication(s)
+    connector
+        .configure()
+        .unwrap()
+        .use_server_name_indication(false)
+        .verify_hostname(false)
+        .connect("foobar.com", s)
         .unwrap();
 }
 
@@ -1067,8 +1066,14 @@ fn connector_no_hostname_still_verifies() {
 
     let connector = SslConnectorBuilder::new(SslMethod::tls()).unwrap().build();
 
-    assert!(connector.danger_connect_without_providing_domain_for_certificate_verification_and_server_name_indication(tcp)
-        .is_err());
+    assert!(
+        connector
+            .configure()
+            .unwrap()
+            .verify_hostname(false)
+            .connect("fizzbuzz.com", tcp)
+            .is_err()
+    );
 }
 
 #[test]
@@ -1079,7 +1084,12 @@ fn connector_no_hostname_can_disable_verify() {
     connector.set_verify(SSL_VERIFY_NONE);
     let connector = connector.build();
 
-    connector.danger_connect_without_providing_domain_for_certificate_verification_and_server_name_indication(tcp).unwrap();
+    connector
+        .configure()
+        .unwrap()
+        .verify_hostname(false)
+        .connect("foobar.com", tcp)
+        .unwrap();
 }
 
 #[test]
@@ -1101,9 +1111,7 @@ fn connector_client_server_mozilla_intermediate() {
     });
 
     let mut connector = SslConnectorBuilder::new(SslMethod::tls()).unwrap();
-    connector
-        .set_ca_file("test/root-ca.pem")
-        .unwrap();
+    connector.set_ca_file("test/root-ca.pem").unwrap();
     let connector = connector.build();
 
     let stream = TcpStream::connect(("127.0.0.1", port)).unwrap();
@@ -1135,9 +1143,7 @@ fn connector_client_server_mozilla_modern() {
     });
 
     let mut connector = SslConnectorBuilder::new(SslMethod::tls()).unwrap();
-    connector
-        .set_ca_file("test/root-ca.pem")
-        .unwrap();
+    connector.set_ca_file("test/root-ca.pem").unwrap();
     let connector = connector.build();
 
     let stream = TcpStream::connect(("127.0.0.1", port)).unwrap();
@@ -1239,7 +1245,8 @@ fn tmp_dh_callback() {
 }
 
 #[test]
-#[cfg(any(all(feature = "v101", ossl101, not(any(libressl261, libressl262, libressl26x))), all(feature = "v102", ossl102)))]
+#[cfg(any(all(feature = "v101", ossl101, not(any(libressl261, libressl262, libressl26x))),
+          all(feature = "v102", ossl102)))]
 fn tmp_ecdh_callback() {
     use ec::EcKey;
     use nid;
@@ -1306,7 +1313,8 @@ fn tmp_dh_callback_ssl() {
 }
 
 #[test]
-#[cfg(any(all(feature = "v101", ossl101, not(any(libressl261, libressl262, libressl26x))), all(feature = "v102", ossl102)))]
+#[cfg(any(all(feature = "v101", ossl101, not(any(libressl261, libressl262, libressl26x))),
+          all(feature = "v102", ossl102)))]
 fn tmp_ecdh_callback_ssl() {
     use ec::EcKey;
     use nid;