From 10520d7fd7bdaff447482f28fba0b50acd7183c9 Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Mon, 13 Feb 2023 16:45:50 -0600 Subject: [PATCH] Fix: native-tls client creation to support HTTPS again (#2360) * 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 --- CHANGELOG.next.toml | 12 ++ rust-runtime/aws-smithy-client/src/bounds.rs | 24 ++-- rust-runtime/aws-smithy-client/src/conns.rs | 129 ++++++++++++++++++ rust-runtime/aws-smithy-client/src/lib.rs | 78 ++--------- .../aws-smithy-client/src/static_tests.rs | 4 +- 5 files changed, 168 insertions(+), 79 deletions(-) create mode 100644 rust-runtime/aws-smithy-client/src/conns.rs diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index d1d98ba8ae..b206c8c663 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -198,3 +198,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 index 0cddf86096..e0abf9e4df 100644 --- 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 = @@ -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 = ::Future, > @@ -96,8 +100,8 @@ pub trait SmithyMiddlewareService: impl 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: /// 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 SmithyRetryPolicy for R where R: tower::retry::Policy, SdkSuccess, SdkError> + Clone, O: ParseHttpResponse> + Send + Sync + Clone + 'static, - E: Error, + E: std::error::Error, Retry: ClassifyRetry, SdkError>, { type O = O; diff --git a/rust-runtime/aws-smithy-client/src/conns.rs b/rust-runtime/aws-smithy-client/src/conns.rs new file mode 100644 index 0000000000..1aac78cf46 --- /dev/null +++ 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; + +#[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; + +#[cfg(feature = "rustls")] +/// A smithy connector that uses the `rustls` crate for TLS. +pub type Rustls = crate::hyper_ext::Adapter; + +// 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 index f83c9d895e..6e5e5ba9ee 100644 --- 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; - - // 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; - - #[cfg(feature = "rustls")] - pub type Rustls = - crate::hyper_ext::Adapter>; -} - 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`], 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 Client 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 index 4c9ef91bad..a8cd503022 100644 --- 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 { unreachable!("only used for static tests")