Implement Support for ALPN #85
Conversation
This adds support for ALPN via the newly released native-tls 0.2.7. With this hyper-tls can now automatically establish an HTTP2 connection with the server without having to force it from the client side.
| native-tls = "0.2.1" | ||
| hyper = { version = "0.14.2", default-features = false, features = ["tcp", "client"] } | ||
| native-tls = { version = "0.2.7", features = ["alpn"] } | ||
| hyper = { version = "0.14.2", default-features = false, features = ["tcp", "client", "http1", "http2"] } |
There was a problem hiding this comment.
This forces http1 and http2 back in. It's unclear to me how to proceed here. I believe we either have to always activate these or have to add new public features of hyper-tls, which if you forget to activate them, you are not getting any ALPN. Though we could make those features active by default.
There was a problem hiding this comment.
Do we need to enable them in this crate? As long as we don't send ALPN by default, the user can opt-in after enabling the features.
There was a problem hiding this comment.
Yeah we can definitely just not send ALPN by default. It could then either be a feature of this crate (which then probably should delegate it further) or be some kind of constructor / builder / configure method of the HttpsConnector (and hoping that the appropriate features are active for hyper). I'll leave that decision up to you before I proceed further.
There was a problem hiding this comment.
I was thinking about this issue and the easiest implementation that comes to mind would be to have something like this in hyper:
/// Returns `true` if the `http2` feature is enabled
pub const fn http2_feature_enabled() -> bool {
cfg!(feature = "http2")
}The advantage is that enabling http2 in hyper would also enable it automatically in the TLS libraries. The downside is that it wouldn't be possible to disable the ALPN feature in hyper-tls. Also I've never seen something like this done in other crates so maybe it's a bad pattern to do in Rust?
| let tls = s.get_ref(); | ||
| if tls | ||
| .negotiated_alpn() | ||
| .ok() |
There was a problem hiding this comment.
The error is somewhat silently ignored here, probably not great.
There was a problem hiding this comment.
Ignoring it here seems fine.
| @@ -15,8 +15,8 @@ vendored = ["native-tls/vendored"] | |||
|
|
|||
| [dependencies] | |||
| bytes = "1" | |||
| native-tls = "0.2.1" | |||
| hyper = { version = "0.14.2", default-features = false, features = ["tcp", "client"] } | |||
| native-tls = { version = "0.2.7", features = ["alpn"] } | |||
There was a problem hiding this comment.
Even if its enabled by default, ALPN support should be behind a feature, since old versions of the TLS backends don't support it.
There was a problem hiding this comment.
Say more. You mean if built on a system with an old openssl/schannel/security-framework, it will probably fail with link errors of missing symbols?
There was a problem hiding this comment.
I'm curious how likely it is that installations would be too old. I'd really wish to make it default enabled in reqwest, so people can use h2 more frequently...
There was a problem hiding this comment.
It is specifically OpenSSL 1.0.1 (CentOS 6 for example), and macOS 10.12 and older.
…round (#720) This cleans up some TLS stuff to make it work in every scenario possible. It effectively reverts #683 and replaces it with the http1 feature flag because hyper-tls is not currently in a state where we can use ALPN at all. Enabling http1 would cause it to always handle everything as http1 even when http2 ALPN is negotiated. For that we should keep an eye on hyperium/hyper-tls#85. It also fixes #703 by adding features for webpki-roots called `rustls-webpki-roots` and renaming `rustls` to `rustls-native-roots` while keeping `rustls` as an alias to preserve backwards compatibility. Approved-by: Cassandra McCarthy <[email protected]> Approved-by: Valdemar Erk <[email protected]> Merged-by: Jens Reidel <[email protected]> Signed-off-by: Jens Reidel <[email protected]>
…round (#720) This cleans up some TLS stuff to make it work in every scenario possible. It effectively reverts #683 and replaces it with the http1 feature flag because hyper-tls is not currently in a state where we can use ALPN at all. Enabling http1 would cause it to always handle everything as http1 even when http2 ALPN is negotiated. For that we should keep an eye on hyperium/hyper-tls#85. It also fixes #703 by adding features for webpki-roots called `rustls-webpki-roots` and renaming `rustls` to `rustls-native-roots` while keeping `rustls` as an alias to preserve backwards compatibility. Approved-by: Cassandra McCarthy <[email protected]> Approved-by: Valdemar Erk <[email protected]> Merged-by: Jens Reidel <[email protected]> Signed-off-by: Jens Reidel <[email protected]>
|
Hi! Is there any update on this? I'm trying to use HTTP/2 in reqwest and I found the issue seanmonstar/reqwest#857. It says that the ALPN support only works in rustls, so we can only make HTTP/2 requests with rustls. Doing this requires manually enabling the IMHO this PR adds ALPN support for the native tls, which is the default tls backend of reqwest. So if this is merged, I guess we can make |
This adds support for ALPN via the newly released native-tls 0.2.7. With this hyper-tls can now automatically establish an HTTP2 connection with the server without having to force it from the client side.