Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enforce the same minimum TLS version (1.2) for both TLS backends #2312

Merged
merged 5 commits into from
Feb 6, 2023
Merged

Enforce the same minimum TLS version (1.2) for both TLS backends #2312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
571fc8f
Enforce the same minimum TLS version (1.2) for both TLS backends
LukeMathWalker Feb 6, 2023
1c67cba
Add CHANGELOG entry
LukeMathWalker Feb 6, 2023
97b3d33
Add documentation for both `https` and `native_tls`.
LukeMathWalker Feb 6, 2023
568e9b8
Remove unnecessary mut
LukeMathWalker Feb 6, 2023
55d7b69
Merge branch 'main' into tls-version
LukeMathWalker Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.next.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
# meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"}
# author = "rcoh"

[[smithy-rs]]
message = "Raise the minimum TLS version from 1.0 to 1.2 when using the `native-tls` feature in `aws-smithy-client`."
references = ["smithy-rs#2312"]
meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client"}
author = "LukeMathWalker"

[[aws-sdk-rust]]
message = """
Provide a way to retrieve fallback credentials if a call to `provide_credentials` is interrupted. An interrupt can occur when a timeout future is raced against a future for `provide_credentials`, and the former wins the race. A new method, `fallback_on_interrupt` on the `ProvideCredentials` trait, can be used in that case. The following code snippet from `LazyCredentialsCache::provide_cached_credentials` has been updated like so:
Expand Down
16 changes: 15 additions & 1 deletion rust-runtime/aws-smithy-client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,27 @@ pub mod conns {
}

#[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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess returning an error here is too much of a breaking change right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also unnecessary - that should never fail since we are building the default configuration with known-good settings.

hyper_tls::HttpsConnector::new()
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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.unwrap_or_else(|e| panic!("Error while creating TLS connector: {}", e));
.unwrap_or_else(|e| panic!("error creating TLS connector: {}", e));

nit: the convention we've been following for panic and Error messages is for them to be uncapitalized.

let http = hyper::client::HttpConnector::new();
hyper_tls::HttpsConnector::from((http, tls.into()))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will still allow http connections when supplying an http URL.
Maybe a comment should be added to the native_tls() function to make users aware of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added documentation to both native_tls and https in 97b3d33 - do you think it's clear enough?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's very clear, thank you! :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the .into() necessary? Isn't tls already of type TlsConnector?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, otherwise you get the following compilation error:

error[E0277]: the trait bound `hyper_tls::HttpsConnector<_>: From<(hyper::client::HttpConnector, TlsConnector)>` is not satisfied
  --> aws-smithy-client/src/lib.rs:95:9
   |
95 |         hyper_tls::HttpsConnector::from((http, tls))
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `From<(hyper::client::HttpConnector, TlsConnector)>` is not implemented for `hyper_tls::HttpsConnector<_>`
   |
   = help: the trait `From<(T, tokio_native_tls::TlsConnector)>` is implemented for `hyper_tls::HttpsConnector<T>`

}

#[cfg(feature = "native-tls")]
Expand Down