Enforce https for native-tls backend #2316
Conversation
| 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())) | ||
| let mut https = hyper_tls::HttpsConnector::from((http, tls.into())); | ||
| https.https_only(true); |
There was a problem hiding this comment.
I assume we'd want the same change in HTTP_NATIVE_ROOTS as well, a few lines above.
There was a problem hiding this comment.
I was thinking about that, but the hyper_rustls::HttpsConnector doesn't seem to provide a way for customers to also allow http. So the original concern is still valid here.
There was a problem hiding this comment.
I'd be quite wary of having diverging behaviour on something like this between the two TLS backends we support - that can turn out to be quite surprising for users.
There was a problem hiding this comment.
If you think that consistency is (more) important in this case, then this PR can be scrapped.
There was a problem hiding this comment.
My suggestion here is to apply the change to both backends, rather than to native-tls only.
There was a problem hiding this comment.
So it looks like we would need a breaking change for this, either returning the builder from fn https() or taking an argument for allow_http: bool on it, right?
There was a problem hiding this comment.
Made a PR for this in hyper-rustls here.
There was a problem hiding this comment.
rustls uses a type-state builder, which requires configuration to be supplied in a specific order.
The "step" that you are currently in bleeds through the type signature as well.
Returning a partially configured rustls HTTPS connector doesn't look like an option - we need to provide one set of configuration with good defaults and then explain in the documentation section how they could configure it otherwise (linking to rustls' own docs).
| @@ -84,15 +84,18 @@ pub mod conns { | |||
| /// 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. | |||
| /// It allows only https connections. | |||
| /// To enable http connections, call `https_only(false)` on the retured NativeTls. | |||
There was a problem hiding this comment.
| /// To enable http connections, call `https_only(false)` on the retured NativeTls. | |
| /// To enable http connections, call `https_only(false)` on the retured `NativeTls`. |
There was a problem hiding this comment.
Also:
- Typo
retured->returned - Suggestion
https_only(false)->.https_only(false)
|
We're removing native-tls: #2675 I'm closing this issue |
Motivation and Context
@LukeMathWalker just set the minimum TLS version for native-tls to v.1.2: #2312
I suggest that we also enforce HTTPS connections, because this might prevent customers from accidentally supplying http URLs.
The main concern not to do this was that some customers might rely on http connections.
But given that http support can easily be re-enabled by customers as described in the added doc/comment (see diff), this should not be an issue.
The
hyper_tls::HttpsConnectorused here also doesn't sent https data to http endpoints, instead it will refuse to connect when given an http URL:Description
Forcing https connections by default for the native_tls connector.
Testing
Checklist
CHANGELOG.next.tomlif I made changes to the smithy-rs codegen or runtime cratesCHANGELOG.next.tomlif I made changes to the AWS SDK, generated SDK code, or SDK runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.