From 00f3218f6d280fd882560c8201bdd8debe5ec501 Mon Sep 17 00:00:00 2001 From: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon, 16 Dec 2024 13:22:18 -0500 Subject: [PATCH 1/4] feat(s2n-tls-hyper): Allow plain HTTP connections --- bindings/rust/s2n-tls-hyper/src/connector.rs | 38 +++++++-- bindings/rust/s2n-tls-hyper/src/stream.rs | 19 +++-- .../rust/s2n-tls-hyper/tests/common/echo.rs | 2 +- bindings/rust/s2n-tls-hyper/tests/http.rs | 77 ++++++++++++++++++- 4 files changed, 115 insertions(+), 21 deletions(-) diff --git a/bindings/rust/s2n-tls-hyper/src/connector.rs b/bindings/rust/s2n-tls-hyper/src/connector.rs index 8721bff70ee..7f3a828f604 100644 --- a/bindings/rust/s2n-tls-hyper/src/connector.rs +++ b/bindings/rust/s2n-tls-hyper/src/connector.rs @@ -27,6 +27,7 @@ use tower_service::Service; pub struct HttpsConnector { http: Http, conn_builder: ConnBuilder, + insecure_http: bool, } impl HttpsConnector @@ -101,7 +102,11 @@ where /// configured on `conn_builder` with APIs like /// `s2n_tls::config::Builder::set_application_protocol_preference()` will be ignored. pub fn builder_with_http(http: Http, conn_builder: ConnBuilder) -> Builder { - Builder { http, conn_builder } + Builder { + http, + conn_builder, + insecure_http: false, + } } } @@ -110,14 +115,22 @@ where pub struct Builder { http: Http, conn_builder: ConnBuilder, + insecure_http: bool, } impl Builder { + /// Allows communication with insecure HTTP endpoints in addition to secure HTTPS endpoints. + pub fn with_insecure_http(&mut self) -> &mut Self { + self.insecure_http = true; + self + } + /// Builds a new `HttpsConnector`. pub fn build(self) -> HttpsConnector { HttpsConnector { http: self.http, conn_builder: self.conn_builder, + insecure_http: self.insecure_http, } } } @@ -155,10 +168,18 @@ where } fn call(&mut self, req: Uri) -> Self::Future { - // Currently, the only supported stream type is TLS. If the application attempts to - // negotiate HTTP over plain TCP, return an error. - if req.scheme() == Some(&http::uri::Scheme::HTTP) { - return Box::pin(async move { Err(Error::InvalidScheme) }); + match req.scheme() { + Some(scheme) if scheme == &http::uri::Scheme::HTTPS => (), + Some(scheme) if scheme == &http::uri::Scheme::HTTP && self.insecure_http => { + let call = self.http.call(req); + return Box::pin(async move { + let tcp = call.await.map_err(|e| Error::HttpError(e.into()))?; + Ok(MaybeHttpsStream::Http(tcp)) + }); + } + _ => { + return Box::pin(async move { Err(Error::InvalidScheme) }); + } } // Attempt to negotiate HTTP/2 by including it in the ALPN extension. Other supported HTTP @@ -235,15 +256,16 @@ mod tests { } #[tokio::test] - async fn test_unsecure_http() -> Result<(), Box> { + async fn test_invalid_scheme() -> Result<(), Box> { let connector = HttpsConnector::new(Config::default()); let client: Client<_, Empty> = Client::builder(TokioExecutor::new()).build(connector); - let uri = Uri::from_str("http://www.amazon.com")?; + // Attempt to make a request with an arbitrary invalid scheme. + let uri = Uri::from_str("notascheme://www.amazon.com")?; let error = client.get(uri).await.unwrap_err(); - // Ensure that an InvalidScheme error is returned when HTTP over TCP is attempted. + // Ensure that an InvalidScheme error is returned. let error = error.source().unwrap().downcast_ref::().unwrap(); assert!(matches!(error, Error::InvalidScheme)); diff --git a/bindings/rust/s2n-tls-hyper/src/stream.rs b/bindings/rust/s2n-tls-hyper/src/stream.rs index c5440285465..532e90a0cd9 100644 --- a/bindings/rust/s2n-tls-hyper/src/stream.rs +++ b/bindings/rust/s2n-tls-hyper/src/stream.rs @@ -15,11 +15,8 @@ use std::{ }; /// `MaybeHttpsStream` is a wrapper over a hyper TCP stream, `Transport`, allowing for TLS to be -/// negotiated over the TCP stream. -/// -/// While not currently implemented, the `MaybeHttpsStream` enum will provide an `Http` type -/// corresponding to the plain TCP stream, allowing for HTTP to be negotiated in addition to HTTPS -/// when the HTTP scheme is used. +/// negotiated over the TCP stream via the `Https` type. The `Http` type bypasses TLS to optionally +/// allow for communication with HTTP endpoints over plain TCP. /// /// This struct is used to implement `tower_service::Service` for `HttpsConnector`, and shouldn't /// need to be used directly. @@ -38,6 +35,7 @@ where // traits to tokio's. This allows the `Read` and `Write` implementations for `MaybeHttpsStream` // to simply call the `TokioIo` `poll` functions. Https(TokioIo, Builder::Output>>), + Http(Transport), } impl HyperConnection for MaybeHttpsStream @@ -48,7 +46,7 @@ where { fn connected(&self) -> Connected { match self { - MaybeHttpsStream::Https(stream) => { + Self::Https(stream) => { let connected = stream.inner().get_ref().connected(); let conn = stream.inner().as_ref(); match conn.application_protocol() { @@ -57,6 +55,7 @@ where _ => connected, } } + Self::Http(stream) => stream.connected(), } } } @@ -74,6 +73,7 @@ where ) -> Poll> { match Pin::get_mut(self) { Self::Https(stream) => Pin::new(stream).poll_read(cx, buf), + Self::Http(stream) => Pin::new(stream).poll_read(cx, buf), } } } @@ -91,18 +91,21 @@ where ) -> Poll> { match Pin::get_mut(self) { Self::Https(stream) => Pin::new(stream).poll_write(cx, buf), + Self::Http(stream) => Pin::new(stream).poll_write(cx, buf), } } fn poll_flush(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { match Pin::get_mut(self) { - MaybeHttpsStream::Https(stream) => Pin::new(stream).poll_flush(cx), + Self::Https(stream) => Pin::new(stream).poll_flush(cx), + Self::Http(stream) => Pin::new(stream).poll_flush(cx), } } fn poll_shutdown(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { match Pin::get_mut(self) { - MaybeHttpsStream::Https(stream) => Pin::new(stream).poll_shutdown(cx), + Self::Https(stream) => Pin::new(stream).poll_shutdown(cx), + Self::Http(stream) => Pin::new(stream).poll_shutdown(cx), } } } diff --git a/bindings/rust/s2n-tls-hyper/tests/common/echo.rs b/bindings/rust/s2n-tls-hyper/tests/common/echo.rs index 36ce865ca7d..5961ca0c89e 100644 --- a/bindings/rust/s2n-tls-hyper/tests/common/echo.rs +++ b/bindings/rust/s2n-tls-hyper/tests/common/echo.rs @@ -11,7 +11,7 @@ use s2n_tls_tokio::TlsAcceptor; use std::{error::Error, future::Future}; use tokio::net::TcpListener; -async fn echo( +pub async fn echo( req: Request, ) -> Result>, hyper::Error> { Ok(Response::new(req.into_body().boxed())) diff --git a/bindings/rust/s2n-tls-hyper/tests/http.rs b/bindings/rust/s2n-tls-hyper/tests/http.rs index f9d9d001b69..fec820d6d82 100644 --- a/bindings/rust/s2n-tls-hyper/tests/http.rs +++ b/bindings/rust/s2n-tls-hyper/tests/http.rs @@ -1,21 +1,28 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::common::InsecureAcceptAllCertificatesHandler; +use crate::common::{echo::echo, InsecureAcceptAllCertificatesHandler}; use bytes::Bytes; use common::echo::serve_echo; use http::{Method, Request, Uri, Version}; use http_body_util::{BodyExt, Empty, Full}; -use hyper_util::{client::legacy::Client, rt::TokioExecutor}; +use hyper::service::service_fn; +use hyper_util::{ + client::legacy::Client, + rt::{TokioExecutor, TokioIo}, +}; use s2n_tls::{ callbacks::{ClientHelloCallback, ConnectionFuture}, config, connection::Connection, security::DEFAULT_TLS13, }; -use s2n_tls_hyper::connector::HttpsConnector; +use s2n_tls_hyper::{connector::HttpsConnector, error}; use std::{error::Error, pin::Pin, str::FromStr}; -use tokio::{net::TcpListener, task::JoinHandle}; +use tokio::{ + net::TcpListener, + task::{JoinHandle, JoinSet}, +}; pub mod common; @@ -330,3 +337,65 @@ async fn config_alpn_ignored() -> Result<(), Box> { Ok(()) } + +#[tokio::test] +async fn insecure_http() -> Result<(), Box> { + let listener = TcpListener::bind("127.0.0.1:0").await?; + let addr = listener.local_addr()?; + + let mut tasks: JoinSet>> = JoinSet::new(); + tasks.spawn(async move { + // Listen for HTTP requests on a plain TCP stream. + let (tcp_stream, _) = listener.accept().await.unwrap(); + let server = hyper_util::server::conn::auto::Builder::new(TokioExecutor::new()); + server + .serve_connection(TokioIo::new(tcp_stream), service_fn(echo)) + .await?; + + Ok(()) + }); + + tasks.spawn(async move { + for enable_insecure_http in [false, true] { + let connector = { + let config = common::config()?.build()?; + let mut builder = HttpsConnector::builder(config); + if enable_insecure_http { + builder.with_insecure_http(); + } + builder.build() + }; + + let client: Client<_, Empty> = + Client::builder(TokioExecutor::new()).build(connector); + let uri = Uri::from_str(format!("http://127.0.0.1:{}", addr.port()).as_str())?; + let response = client.get(uri).await; + + if enable_insecure_http { + // If insecure HTTP is enabled, the request should succeed. + let response = response.unwrap(); + assert_eq!(response.status(), 200); + } else { + // By default, insecure HTTP is disabled, and the request should error. + let error = response.unwrap_err(); + + // Ensure an InvalidScheme error is produced. + let error = error + .source() + .unwrap() + .downcast_ref::() + .unwrap(); + assert!(matches!(error, error::Error::InvalidScheme)); + assert!(!error.to_string().is_empty()); + } + } + + Ok(()) + }); + + while let Some(res) = tasks.join_next().await { + res.unwrap()?; + } + + Ok(()) +} From 3f13019387b50f5a8c6d4f4cbac06adf0bbe7b92 Mon Sep 17 00:00:00 2001 From: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon, 16 Dec 2024 16:42:44 -0500 Subject: [PATCH 2/4] add enabled argument to api --- bindings/rust/s2n-tls-hyper/src/connector.rs | 13 +++++++++++-- bindings/rust/s2n-tls-hyper/tests/http.rs | 6 ++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/bindings/rust/s2n-tls-hyper/src/connector.rs b/bindings/rust/s2n-tls-hyper/src/connector.rs index 7f3a828f604..8f55d6e07dc 100644 --- a/bindings/rust/s2n-tls-hyper/src/connector.rs +++ b/bindings/rust/s2n-tls-hyper/src/connector.rs @@ -120,8 +120,8 @@ pub struct Builder { impl Builder { /// Allows communication with insecure HTTP endpoints in addition to secure HTTPS endpoints. - pub fn with_insecure_http(&mut self) -> &mut Self { - self.insecure_http = true; + pub fn with_insecure_http(&mut self, enabled: bool) -> &mut Self { + self.insecure_http = enabled; self } @@ -274,4 +274,13 @@ mod tests { Ok(()) } + + #[tokio::test] + async fn default_builder() -> Result<(), Box> { + // Ensure that insecure HTTP is disabled by default. + let connector = HttpsConnector::builder(Config::default()).build(); + assert!(!connector.insecure_http); + + Ok(()) + } } diff --git a/bindings/rust/s2n-tls-hyper/tests/http.rs b/bindings/rust/s2n-tls-hyper/tests/http.rs index fec820d6d82..b86736f0543 100644 --- a/bindings/rust/s2n-tls-hyper/tests/http.rs +++ b/bindings/rust/s2n-tls-hyper/tests/http.rs @@ -360,9 +360,7 @@ async fn insecure_http() -> Result<(), Box> { let connector = { let config = common::config()?.build()?; let mut builder = HttpsConnector::builder(config); - if enable_insecure_http { - builder.with_insecure_http(); - } + builder.with_insecure_http(enable_insecure_http); builder.build() }; @@ -376,7 +374,7 @@ async fn insecure_http() -> Result<(), Box> { let response = response.unwrap(); assert_eq!(response.status(), 200); } else { - // By default, insecure HTTP is disabled, and the request should error. + // If insecure HTTP is disabled, the request should error. let error = response.unwrap_err(); // Ensure an InvalidScheme error is produced. From 9c998c61ea004c433a8c3160afe1f59d74c1d3c3 Mon Sep 17 00:00:00 2001 From: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon, 16 Dec 2024 17:19:03 -0500 Subject: [PATCH 3/4] document default behavior Co-authored-by: Wesley Rosenblum <55108558+WesleyRosenblum@users.noreply.github.com> --- bindings/rust/s2n-tls-hyper/src/connector.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bindings/rust/s2n-tls-hyper/src/connector.rs b/bindings/rust/s2n-tls-hyper/src/connector.rs index 8f55d6e07dc..869e56760f7 100644 --- a/bindings/rust/s2n-tls-hyper/src/connector.rs +++ b/bindings/rust/s2n-tls-hyper/src/connector.rs @@ -119,7 +119,7 @@ pub struct Builder { } impl Builder { - /// Allows communication with insecure HTTP endpoints in addition to secure HTTPS endpoints. + /// If enabled, allows communication with insecure HTTP endpoints in addition to secure HTTPS endpoints (default: false). pub fn with_insecure_http(&mut self, enabled: bool) -> &mut Self { self.insecure_http = enabled; self From 9f217b9a4bcd9872a2bd9813b6749aff0fddf9c7 Mon Sep 17 00:00:00 2001 From: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Tue, 17 Dec 2024 10:38:37 -0500 Subject: [PATCH 4/4] with_plaintext_http --- bindings/rust/s2n-tls-hyper/src/connector.rs | 21 ++++++++++---------- bindings/rust/s2n-tls-hyper/tests/http.rs | 12 +++++------ 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/bindings/rust/s2n-tls-hyper/src/connector.rs b/bindings/rust/s2n-tls-hyper/src/connector.rs index 869e56760f7..576ca93e468 100644 --- a/bindings/rust/s2n-tls-hyper/src/connector.rs +++ b/bindings/rust/s2n-tls-hyper/src/connector.rs @@ -27,7 +27,7 @@ use tower_service::Service; pub struct HttpsConnector { http: Http, conn_builder: ConnBuilder, - insecure_http: bool, + plaintext_http: bool, } impl HttpsConnector @@ -105,7 +105,7 @@ where Builder { http, conn_builder, - insecure_http: false, + plaintext_http: false, } } } @@ -115,13 +115,14 @@ where pub struct Builder { http: Http, conn_builder: ConnBuilder, - insecure_http: bool, + plaintext_http: bool, } impl Builder { - /// If enabled, allows communication with insecure HTTP endpoints in addition to secure HTTPS endpoints (default: false). - pub fn with_insecure_http(&mut self, enabled: bool) -> &mut Self { - self.insecure_http = enabled; + /// If enabled, allows communication with plaintext HTTP endpoints in addition to secure HTTPS + /// endpoints (default: false). + pub fn with_plaintext_http(&mut self, enabled: bool) -> &mut Self { + self.plaintext_http = enabled; self } @@ -130,7 +131,7 @@ impl Builder { HttpsConnector { http: self.http, conn_builder: self.conn_builder, - insecure_http: self.insecure_http, + plaintext_http: self.plaintext_http, } } } @@ -170,7 +171,7 @@ where fn call(&mut self, req: Uri) -> Self::Future { match req.scheme() { Some(scheme) if scheme == &http::uri::Scheme::HTTPS => (), - Some(scheme) if scheme == &http::uri::Scheme::HTTP && self.insecure_http => { + Some(scheme) if scheme == &http::uri::Scheme::HTTP && self.plaintext_http => { let call = self.http.call(req); return Box::pin(async move { let tcp = call.await.map_err(|e| Error::HttpError(e.into()))?; @@ -277,9 +278,9 @@ mod tests { #[tokio::test] async fn default_builder() -> Result<(), Box> { - // Ensure that insecure HTTP is disabled by default. + // Ensure that plaintext HTTP is disabled by default. let connector = HttpsConnector::builder(Config::default()).build(); - assert!(!connector.insecure_http); + assert!(!connector.plaintext_http); Ok(()) } diff --git a/bindings/rust/s2n-tls-hyper/tests/http.rs b/bindings/rust/s2n-tls-hyper/tests/http.rs index b86736f0543..9f9b87e29a0 100644 --- a/bindings/rust/s2n-tls-hyper/tests/http.rs +++ b/bindings/rust/s2n-tls-hyper/tests/http.rs @@ -339,7 +339,7 @@ async fn config_alpn_ignored() -> Result<(), Box> { } #[tokio::test] -async fn insecure_http() -> Result<(), Box> { +async fn plaintext_http() -> Result<(), Box> { let listener = TcpListener::bind("127.0.0.1:0").await?; let addr = listener.local_addr()?; @@ -356,11 +356,11 @@ async fn insecure_http() -> Result<(), Box> { }); tasks.spawn(async move { - for enable_insecure_http in [false, true] { + for enable_plaintext_http in [false, true] { let connector = { let config = common::config()?.build()?; let mut builder = HttpsConnector::builder(config); - builder.with_insecure_http(enable_insecure_http); + builder.with_plaintext_http(enable_plaintext_http); builder.build() }; @@ -369,12 +369,12 @@ async fn insecure_http() -> Result<(), Box> { let uri = Uri::from_str(format!("http://127.0.0.1:{}", addr.port()).as_str())?; let response = client.get(uri).await; - if enable_insecure_http { - // If insecure HTTP is enabled, the request should succeed. + if enable_plaintext_http { + // If plaintext HTTP is enabled, the request should succeed. let response = response.unwrap(); assert_eq!(response.status(), 200); } else { - // If insecure HTTP is disabled, the request should error. + // If plaintext HTTP is disabled, the request should error. let error = response.unwrap_err(); // Ensure an InvalidScheme error is produced.