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.

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

feat(s2n-tls-hyper): Add support for negotiating HTTP/2 #4924

Merged
merged 9 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions bindings/rust/s2n-tls-hyper/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,17 @@ default = []
s2n-tls = { version = "=0.3.7", path = "../s2n-tls" }
s2n-tls-tokio = { version = "=0.3.7", path = "../s2n-tls-tokio" }
hyper = { version = "1" }
hyper-util = { version = "0.1", features = ["client-legacy", "tokio", "http1"] }
hyper-util = { version = "0.1", features = ["client-legacy", "tokio", "http1", "http2"] }
tower-service = { version = "0.3" }
http = { version= "1" }
http = { version = "1" }

[dev-dependencies]
tokio = { version = "1", features = ["macros", "test-util"] }
http-body-util = "0.1"
hyper-util = { version = "0.1", features = ["server"] }
bytes = "1"

# Newer versions require Rust 1.65, see https://github.com/aws/s2n-tls/issues/4242.
hashbrown = { version = "=0.15.0" }
# Newer versions require Rust 1.70, see https://github.com/aws/s2n-tls/issues/4395.
tokio-util = { version = "=0.7.11" }
25 changes: 24 additions & 1 deletion bindings/rust/s2n-tls-hyper/src/connector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ where
///
/// This API creates an `HttpsConnector` using the default hyper `HttpConnector`. To use an
/// existing HTTP connector, use `HttpsConnector::new_with_http()`.
///
/// Note that s2n-tls-hyper will override the ALPN extension to negotiate HTTP. Any ALPN values
/// configured on `conn_builder` with APIs like
/// `s2n_tls::config::Builder::set_application_protocol_preference()` will be ignored.
pub fn new(conn_builder: Builder) -> HttpsConnector<HttpConnector, Builder> {
let mut http = HttpConnector::new();

Expand Down Expand Up @@ -77,6 +81,10 @@ where
/// ```
///
/// `HttpsConnector::new()` can be used to create the HTTP connector automatically.
///
/// Note that s2n-tls-hyper will override the ALPN extension to negotiate HTTP. Any ALPN values
/// configured on `conn_builder` with APIs like
/// `s2n_tls::config::Builder::set_application_protocol_preference()` will be ignored.
pub fn new_with_http(http: Http, conn_builder: Builder) -> HttpsConnector<Http, Builder> {
Self { http, conn_builder }
}
Expand Down Expand Up @@ -118,6 +126,22 @@ where
return Box::pin(async move { Err(Error::InvalidScheme) });
}

// Attempt to negotiate HTTP/2 by including it in the ALPN extension. Other supported HTTP
// versions are also included to prevent the server from rejecting the TLS connection if
// HTTP/2 isn't supported:
//
// https://datatracker.ietf.org/doc/html/rfc7301#section-3.2
// In the event that the server supports no
// protocols that the client advertises, then the server SHALL respond
// with a fatal "no_application_protocol" alert.
Comment on lines +133 to +136
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

let builder = connection::ModifiedBuilder::new(self.conn_builder.clone(), |conn| {
conn.set_application_protocol_preference([
b"h2".to_vec(),
b"http/1.1".to_vec(),
b"http/1.0".to_vec(),
])
});

// IPv6 addresses are enclosed in square brackets within the host of a URI (e.g.
// `https://[::1:2:3:4]/`). These square brackets aren't part of the domain itself, so they
// are trimmed off to provide the proper server name to s2n-tls-tokio (e.g. `::1:2:3:4`).
Expand All @@ -129,7 +153,6 @@ where
}
let domain = domain.to_owned();

let builder = self.conn_builder.clone();
let call = self.http.call(req);
Box::pin(async move {
// `HttpsConnector` wraps an HTTP connector that also implements `Service<Uri>`.
Expand Down
10 changes: 9 additions & 1 deletion bindings/rust/s2n-tls-hyper/src/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,15 @@ where
{
fn connected(&self) -> Connected {
match self {
MaybeHttpsStream::Https(stream) => stream.inner().get_ref().connected(),
MaybeHttpsStream::Https(stream) => {
let connected = stream.inner().get_ref().connected();
let conn = stream.inner().as_ref();
match conn.application_protocol() {
// Inform hyper that HTTP/2 was negotiated in the ALPN.
Some(b"h2") => connected.negotiated_h2(),
_ => connected,
Copy link
Contributor Author

@goatgoose goatgoose Dec 3, 2024

Choose a reason for hiding this comment

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

Ideally we could assert that the negotiated application protocol was something we expected here. However, we can't return an error since this function doesn't return an error. We could panic, however misbehaved servers could technically return whatever ALPN value they want, whether or not the client attempted to negotiate it. So panicking here seems wrong, since it's outside of the user's control. It seemed better to just attempt HTTP/1 if an unexpected protocol was negotiated.

}
}
}
}
}
Expand Down
68 changes: 67 additions & 1 deletion bindings/rust/s2n-tls-hyper/tests/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
use crate::common::InsecureAcceptAllCertificatesHandler;
use bytes::Bytes;
use common::echo::serve_echo;
use http::{Method, Request, Uri};
use http::{Method, Request, Uri, Version};
use http_body_util::{BodyExt, Empty, Full};
use hyper_util::{client::legacy::Client, rt::TokioExecutor};
use s2n_tls::{
Expand Down Expand Up @@ -264,3 +264,69 @@ async fn ipv6() -> Result<(), Box<dyn Error + Send + Sync>> {

Ok(())
}

#[tokio::test]
async fn http2() -> Result<(), Box<dyn Error + Send + Sync>> {
for expected_http_version in [Version::HTTP_11, Version::HTTP_2] {
let server_config = {
let mut builder = common::config()?;
if expected_http_version == Version::HTTP_2 {
builder.set_application_protocol_preference(["h2"])?;
}
builder.build()?
};

common::echo::make_echo_request(server_config.clone(), |port| async move {
let connector = HttpsConnector::new(common::config()?.build()?);
let client: Client<_, Empty<Bytes>> =
Client::builder(TokioExecutor::new()).build(connector);

let uri = Uri::from_str(format!("https://localhost:{}", port).as_str())?;
let response = client.get(uri).await?;
assert_eq!(response.status(), 200);

// Ensure that HTTP/2 is negotiated when supported by the server.
assert_eq!(response.version(), expected_http_version);

Ok(())
})
.await?;
}

Ok(())
}

/// Ensure that HTTP/2 is negotiated, regardless of any pre-configured ALPN values.
#[tokio::test]
async fn config_alpn_ignored() -> Result<(), Box<dyn Error + Send + Sync>> {
let server_config = {
let mut builder = common::config()?;
builder.set_application_protocol_preference(["h2"])?;
builder.build()?
};

common::echo::make_echo_request(server_config, |port| async move {
let client_config = {
let mut builder = common::config()?;
// Set an arbitrary non-HTTP/2 ALPN value.
builder.set_application_protocol_preference([b"http/1.1"])?;
builder.build()?
};

let connector = HttpsConnector::new(client_config);
let client: Client<_, Empty<Bytes>> =
Client::builder(TokioExecutor::new()).build(connector);

let uri = Uri::from_str(format!("https://localhost:{}", port).as_str())?;
let response = client.get(uri).await?;
assert_eq!(response.status(), 200);

// Ensure that HTTP/2 was negotiated.
assert_eq!(response.version(), Version::HTTP_2);

Ok(())
})
.await?;

Ok(())
}
Loading