Skip to content

Commit

Permalink
Fix: native-tls client creation to support HTTPS again (#2360)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
Velfi authored Feb 13, 2023
1 parent f7c550e commit 10520d7
Show file tree
Hide file tree
Showing 5 changed files with 168 additions and 79 deletions.
12 changes: 12 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
24 changes: 14 additions & 10 deletions rust-runtime/aws-smithy-client/src/bounds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//!
Expand All @@ -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> =
Expand Down Expand Up @@ -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,
>
Expand All @@ -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,
Expand Down Expand Up @@ -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.
///
Expand All @@ -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;
Expand Down
129 changes: 129 additions & 0 deletions rust-runtime/aws-smithy-client/src/conns.rs
Original file line number Diff line number Diff line change
@@ -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;
}
}
}
78 changes: 11 additions & 67 deletions rust-runtime/aws-smithy-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand All @@ -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;

Expand All @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions rust-runtime/aws-smithy-client/src/static_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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")
Expand Down

0 comments on commit 10520d7

Please sign in to comment.