Fix: native-tls client creation to support HTTPS again (#2360)

Zelda Hessler · jjant · commit c819dbe4e444 · 2023-02-14T12:30:03.000Z
* update: use `enforce_http = false` when creating native-tls hyper connector
refactor: move smithy conns module to its own file
add: sanity tests ensuring we can make HTTP and HTTPS requests with the rustls and native-tls connectors
remove: `use crate::*` imports in favor of explicit imports

* update: CHANGELOG.next.toml

* add: feature gate to conns tests
diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml
@@ -34,3 +34,15 @@ message = "Fix inconsistent casing in services re-export."
 references = ["smithy-rs#2349"]
 meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "server" }
 author = "hlbarber"
+
+[[aws-sdk-rust]]
+message = "Fix issue where clients using native-tls connector were prevented from making HTTPS requests."
+references = ["aws-sdk-rust#736"]
+meta = { "breaking" = false, "tada" = false, "bug" = true }
+author = "Velfi"
+
+[[smithy-rs]]
+message = "Fix issue where clients using native-tls connector were prevented from making HTTPS requests."
+references = ["aws-sdk-rust#736"]
+meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "client" }
+author = "Velfi"
diff --git a/rust-runtime/aws-smithy-client/src/bounds.rs b/rust-runtime/aws-smithy-client/src/bounds.rs
@@ -7,7 +7,7 @@
 //! required for `call` and friends.
 //!
 //! The short-hands will one day be true [trait aliases], but for now they are traits with blanket
-//! implementations. Also, due to [compiler limitations], the bounds repeat a nubmer of associated
+//! implementations. Also, due to [compiler limitations], the bounds repeat a number of associated
 //! types with bounds so that those bounds [do not need to be repeated] at the call site. It's a
 //! bit of a mess to define, but _should_ be invisible to callers.
 //!
@@ -17,8 +17,12 @@
 
 use crate::erase::DynConnector;
 use crate::http_connector::HttpConnector;
-use crate::*;
-use aws_smithy_http::result::ConnectorError;
+use aws_smithy_http::body::SdkBody;
+use aws_smithy_http::operation::{self, Operation};
+use aws_smithy_http::response::ParseHttpResponse;
+use aws_smithy_http::result::{ConnectorError, SdkError, SdkSuccess};
+use aws_smithy_http::retry::ClassifyRetry;
+use tower::{Layer, Service};
 
 /// A service that has parsed a raw Smithy response.
 pub type Parsed<S, O, Retry> =
@@ -75,14 +79,14 @@ where
     }
 }
 
-/// A Smithy middleware service that adjusts [`aws_smithy_http::operation::Request`]s.
+/// A Smithy middleware service that adjusts [`aws_smithy_http::operation::Request`](operation::Request)s.
 ///
 /// This trait has a blanket implementation for all compatible types, and should never be
 /// implemented.
 pub trait SmithyMiddlewareService:
     Service<
-    aws_smithy_http::operation::Request,
-    Response = aws_smithy_http::operation::Response,
+    operation::Request,
+    Response = operation::Response,
     Error = aws_smithy_http_tower::SendOperationError,
     Future = <Self as SmithyMiddlewareService>::Future,
 >
@@ -96,8 +100,8 @@ pub trait SmithyMiddlewareService:
 impl<T> SmithyMiddlewareService for T
 where
     T: Service<
-        aws_smithy_http::operation::Request,
-        Response = aws_smithy_http::operation::Response,
+        operation::Request,
+        Response = operation::Response,
         Error = aws_smithy_http_tower::SendOperationError,
     >,
     T::Future: Send + 'static,
@@ -143,7 +147,7 @@ pub trait SmithyRetryPolicy<O, T, E, Retry>:
     /// Forwarding type to `E` for bound inference.
     ///
     /// See module-level docs for details.
-    type E: Error;
+    type E: std::error::Error;
 
     /// Forwarding type to `Retry` for bound inference.
     ///
@@ -155,7 +159,7 @@ impl<R, O, T, E, Retry> SmithyRetryPolicy<O, T, E, Retry> for R
 where
     R: tower::retry::Policy<Operation<O, Retry>, SdkSuccess<T>, SdkError<E>> + Clone,
     O: ParseHttpResponse<Output = Result<T, E>> + Send + Sync + Clone + 'static,
-    E: Error,
+    E: std::error::Error,
     Retry: ClassifyRetry<SdkSuccess<T>, SdkError<E>>,
 {
     type O = O;
diff --git a/rust-runtime/aws-smithy-client/src/conns.rs b/rust-runtime/aws-smithy-client/src/conns.rs
@@ -0,0 +1,129 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+//! Type aliases for standard connection types.
+
+#[cfg(feature = "rustls")]
+/// A `hyper` connector that uses the `rustls` crate for TLS. To use this in a smithy client,
+/// wrap it in a [hyper_ext::Adapter](crate::hyper_ext::Adapter).
+pub type Https = hyper_rustls::HttpsConnector<hyper::client::HttpConnector>;
+
+#[cfg(feature = "native-tls")]
+/// A `hyper` connector that uses the `native-tls` crate for TLS. To use this in a smithy client,
+/// wrap it in a [hyper_ext::Adapter](crate::hyper_ext::Adapter).
+pub type NativeTls = hyper_tls::HttpsConnector<hyper::client::HttpConnector>;
+
+#[cfg(feature = "rustls")]
+/// A smithy connector that uses the `rustls` crate for TLS.
+pub type Rustls = crate::hyper_ext::Adapter<Https>;
+
+// Creating a `with_native_roots` HTTP client takes 300ms on OS X. Cache this so that we
+// don't need to repeatedly incur that cost.
+#[cfg(feature = "rustls")]
+lazy_static::lazy_static! {
+    static ref HTTPS_NATIVE_ROOTS: Https = {
+        hyper_rustls::HttpsConnectorBuilder::new()
+            .with_native_roots()
+            .https_or_http()
+            .enable_http1()
+            .enable_http2()
+            .build()
+    };
+}
+
+#[cfg(feature = "rustls")]
+/// Return a default HTTPS connector backed by the `rustls` crate.
+///
+/// It requires a minimum TLS version of 1.2.
+/// It allows you to connect to both `http` and `https` URLs.
+pub fn https() -> Https {
+    HTTPS_NATIVE_ROOTS.clone()
+}
+
+#[cfg(feature = "native-tls")]
+/// Return a default HTTPS connector backed by the `hyper_tls` crate.
+///
+/// It requires a minimum TLS version of 1.2.
+/// It allows you to connect to both `http` and `https` URLs.
+pub fn native_tls() -> NativeTls {
+    // `TlsConnector` actually comes for here: https://docs.rs/native-tls/latest/native_tls/
+    // hyper_tls just re-exports the crate for convenience.
+    let mut tls = hyper_tls::native_tls::TlsConnector::builder();
+    let tls = tls
+        .min_protocol_version(Some(hyper_tls::native_tls::Protocol::Tlsv12))
+        .build()
+        .unwrap_or_else(|e| panic!("Error while creating TLS connector: {}", e));
+    let mut http = hyper::client::HttpConnector::new();
+    http.enforce_http(false);
+    hyper_tls::HttpsConnector::from((http, tls.into()))
+}
+
+#[cfg(all(test, any(feature = "native-tls", feature = "rustls")))]
+mod tests {
+    use crate::erase::DynConnector;
+    use crate::hyper_ext::Adapter;
+    use aws_smithy_http::body::SdkBody;
+    use http::{Method, Request, Uri};
+    use tower::{Service, ServiceBuilder};
+
+    async fn send_request_and_assert_success(conn: DynConnector, uri: &Uri) {
+        let mut svc = ServiceBuilder::new().service(conn);
+        let req = Request::builder()
+            .uri(uri)
+            .method(Method::GET)
+            .body(SdkBody::empty())
+            .unwrap();
+        let res = svc.call(req).await.unwrap();
+        assert!(res.status().is_success());
+    }
+
+    #[cfg(feature = "native-tls")]
+    mod native_tls_tests {
+        use super::super::native_tls;
+        use super::*;
+
+        #[tokio::test]
+        async fn test_native_tls_connector_can_make_http_requests() {
+            let conn = Adapter::builder().build(native_tls());
+            let conn = DynConnector::new(conn);
+            let http_uri: Uri = "http://example.com/".parse().unwrap();
+
+            send_request_and_assert_success(conn, &http_uri).await;
+        }
+
+        #[tokio::test]
+        async fn test_native_tls_connector_can_make_https_requests() {
+            let conn = Adapter::builder().build(native_tls());
+            let conn = DynConnector::new(conn);
+            let https_uri: Uri = "https://example.com/".parse().unwrap();
+
+            send_request_and_assert_success(conn, &https_uri).await;
+        }
+    }
+
+    #[cfg(feature = "rustls")]
+    mod rustls_tests {
+        use super::super::https;
+        use super::*;
+
+        #[tokio::test]
+        async fn test_rustls_connector_can_make_http_requests() {
+            let conn = Adapter::builder().build(https());
+            let conn = DynConnector::new(conn);
+            let http_uri: Uri = "http://example.com/".parse().unwrap();
+
+            send_request_and_assert_success(conn, &http_uri).await;
+        }
+
+        #[tokio::test]
+        async fn test_rustls_connector_can_make_https_requests() {
+            let conn = Adapter::builder().build(https());
+            let conn = DynConnector::new(conn);
+            let https_uri: Uri = "https://example.com/".parse().unwrap();
+
+            send_request_and_assert_success(conn, &https_uri).await;
+        }
+    }
+}
diff --git a/rust-runtime/aws-smithy-client/src/lib.rs b/rust-runtime/aws-smithy-client/src/lib.rs
@@ -24,7 +24,10 @@
 
 pub mod bounds;
 pub mod erase;
+pub mod http_connector;
+pub mod never;
 pub mod retry;
+pub mod timeout;
 
 // https://github.com/rust-lang/rust/issues/72081
 #[allow(rustdoc::private_doc_tests)]
@@ -36,8 +39,8 @@ pub mod dvr;
 #[cfg(feature = "test-util")]
 pub mod test_connection;
 
-pub mod http_connector;
-
+#[cfg(feature = "client-hyper")]
+pub mod conns;
 #[cfg(feature = "client-hyper")]
 pub mod hyper_ext;
 
@@ -47,78 +50,19 @@ pub mod hyper_ext;
 #[doc(hidden)]
 pub mod static_tests;
 
-pub mod never;
-pub mod timeout;
-pub use timeout::TimeoutLayer;
-
-/// Type aliases for standard connection types.
-#[cfg(feature = "client-hyper")]
-#[allow(missing_docs)]
-pub mod conns {
-    #[cfg(feature = "rustls")]
-    pub type Https = hyper_rustls::HttpsConnector<hyper::client::HttpConnector>;
-
-    // Creating a `with_native_roots` HTTP client takes 300ms on OS X. Cache this so that we
-    // don't need to repeatedly incur that cost.
-    #[cfg(feature = "rustls")]
-    lazy_static::lazy_static! {
-        static ref HTTPS_NATIVE_ROOTS: Https = {
-            hyper_rustls::HttpsConnectorBuilder::new()
-                .with_native_roots()
-                .https_or_http()
-                .enable_http1()
-                .enable_http2()
-                .build()
-        };
-    }
-
-    #[cfg(feature = "rustls")]
-    /// Return a default HTTPS connector backed by the `rustls` crate.
-    ///
-    /// It requires a minimum TLS version of 1.2.
-    /// It allows you to connect to both `http` and `https` URLs.
-    pub fn https() -> Https {
-        HTTPS_NATIVE_ROOTS.clone()
-    }
-
-    #[cfg(feature = "native-tls")]
-    /// Return a default HTTPS connector backed by the `hyper_tls` crate.
-    ///
-    /// It requires a minimum TLS version of 1.2.
-    /// It allows you to connect to both `http` and `https` URLs.
-    pub fn native_tls() -> NativeTls {
-        let mut tls = hyper_tls::native_tls::TlsConnector::builder();
-        let tls = tls
-            .min_protocol_version(Some(hyper_tls::native_tls::Protocol::Tlsv12))
-            .build()
-            .unwrap_or_else(|e| panic!("Error while creating TLS connector: {}", e));
-        let http = hyper::client::HttpConnector::new();
-        hyper_tls::HttpsConnector::from((http, tls.into()))
-    }
-
-    #[cfg(feature = "native-tls")]
-    pub type NativeTls = hyper_tls::HttpsConnector<hyper::client::HttpConnector>;
-
-    #[cfg(feature = "rustls")]
-    pub type Rustls =
-        crate::hyper_ext::Adapter<hyper_rustls::HttpsConnector<hyper::client::HttpConnector>>;
-}
-
 use aws_smithy_async::rt::sleep::AsyncSleep;
-use aws_smithy_http::body::SdkBody;
 use aws_smithy_http::operation::Operation;
 use aws_smithy_http::response::ParseHttpResponse;
 pub use aws_smithy_http::result::{SdkError, SdkSuccess};
-use aws_smithy_http::retry::ClassifyRetry;
 use aws_smithy_http_tower::dispatch::DispatchLayer;
 use aws_smithy_http_tower::parse_response::ParseResponseLayer;
 use aws_smithy_types::error::display::DisplayErrorContext;
 use aws_smithy_types::retry::ProvideErrorKind;
 use aws_smithy_types::timeout::OperationTimeoutConfig;
-use std::error::Error;
 use std::sync::Arc;
 use timeout::ClientTimeoutParams;
-use tower::{Layer, Service, ServiceBuilder, ServiceExt};
+pub use timeout::TimeoutLayer;
+use tower::{Service, ServiceBuilder, ServiceExt};
 use tracing::{debug_span, field, field::display, Instrument};
 
 /// Smithy service client.
@@ -131,7 +75,7 @@ use tracing::{debug_span, field, field::display, Instrument};
 /// such as those used for routing (like the URL), authentication, and authorization.
 ///
 /// The middleware takes the form of a [`tower::Layer`] that wraps the actual connection for each
-/// request. The [`tower::Service`] that the middleware produces must accept requests of the type
+/// request. The [`tower::Service`](Service) that the middleware produces must accept requests of the type
 /// [`aws_smithy_http::operation::Request`] and return responses of the type
 /// [`http::Response<SdkBody>`], most likely by modifying the provided request in place, passing it
 /// to the inner service, and then ultimately returning the inner service's response.
@@ -165,9 +109,9 @@ impl<C, M> Client<C, M>
 where
     M: Default,
 {
-    /// Create a Smithy client from the given `connector`, a middleware default, the [standard
-    /// retry policy](crate::retry::Standard), and the [`default_async_sleep`](aws_smithy_async::rt::sleep::default_async_sleep)
-    /// sleep implementation.
+    /// Create a Smithy client from the given `connector`, a middleware default, the
+    /// [standard retry policy](retry::Standard), and the
+    /// [`default_async_sleep`](aws_smithy_async::rt::sleep::default_async_sleep) sleep implementation.
     pub fn new(connector: C) -> Self {
         Builder::new()
             .connector(connector)
diff --git a/rust-runtime/aws-smithy-client/src/static_tests.rs b/rust-runtime/aws-smithy-client/src/static_tests.rs
@@ -5,7 +5,7 @@
 //! This module provides types useful for static tests.
 #![allow(missing_docs, missing_debug_implementations)]
 
-use crate::{Builder, Error, Operation, ParseHttpResponse, ProvideErrorKind};
+use crate::{Builder, Operation, ParseHttpResponse, ProvideErrorKind};
 use aws_smithy_http::operation;
 use aws_smithy_http::retry::DefaultResponseRetryClassifier;
 
@@ -17,7 +17,7 @@ impl std::fmt::Display for TestOperationError {
         unreachable!("only used for static tests")
     }
 }
-impl Error for TestOperationError {}
+impl std::error::Error for TestOperationError {}
 impl ProvideErrorKind for TestOperationError {
     fn retryable_error_kind(&self) -> Option<aws_smithy_types::retry::ErrorKind> {
         unreachable!("only used for static tests")
