From ff925951571df10fbc342cef932811347700777e Mon Sep 17 00:00:00 2001 From: Eric Rodrigues Pires Date: Sun, 2 Feb 2025 08:11:51 -0300 Subject: [PATCH] Make `--http-request-timeout` optional by default --- CHANGELOG.md | 10 ++ book/src/cli.md | 4 +- docker-compose-example/compose.yml | 1 + src/config.rs | 12 ++- src/http.rs | 152 +++++++++++++++++++++++------ src/lib.rs | 6 +- 6 files changed, 148 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43a0993..0d4ef5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Changelog +## Unreleased + +### Fixed + +- Further fixes to HTTPS connections lock-ups. + +### Changed + +- **BREAKING**: Make `--http-request-timeout` optional by default. + ## 0.3.2 (2025-01-12) ### Added diff --git a/book/src/cli.md b/book/src/cli.md index cf1b046..0a6d017 100644 --- a/book/src/cli.md +++ b/book/src/cli.md @@ -243,7 +243,9 @@ Expose HTTP/SSH/TCP services through SSH port forwarding. [default: 5s] --http-request-timeout <DURATION> - Time until an outgoing HTTP request is automatically canceled + Time until an outgoing HTTP request is automatically canceled. + + By default, outgoing requests are not terminated by Sandhole. [default: 30s] diff --git a/docker-compose-example/compose.yml b/docker-compose-example/compose.yml index 83f17a6..4a88228 100644 --- a/docker-compose-example/compose.yml +++ b/docker-compose-example/compose.yml @@ -29,5 +29,6 @@ services: --http-port=80 --https-port=443 --force-https + --http-request-timeout=60s network_mode: host restart: unless-stopped diff --git a/src/config.rs b/src/config.rs index 04666d5..828a33c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -295,8 +295,10 @@ pub struct ApplicationConfig { pub authentication_request_timeout: Duration, /// Time until an outgoing HTTP request is automatically canceled. - #[arg(long, default_value = "30s", value_name = "DURATION")] - pub http_request_timeout: Duration, + /// + /// By default, outgoing requests are not terminated by Sandhole. + #[arg(long, value_name = "DURATION")] + pub http_request_timeout: Option, /// How long until TCP connections (including Websockets and local forwardings) are automatically garbage-collected. /// @@ -372,7 +374,7 @@ mod application_config_tests { idle_connection_timeout: Duration::from_str("2s").unwrap(), unproxied_connection_timeout: None, authentication_request_timeout: Duration::from_str("5s").unwrap(), - http_request_timeout: Duration::from_str("30s").unwrap(), + http_request_timeout: None, tcp_connection_timeout: None } ) @@ -419,7 +421,7 @@ mod application_config_tests { "--idle-connection-timeout=3s", "--unproxied-connection-timeout=4s", "--authentication-request-timeout=6s", - "--http-request-timeout=11s", + "--http-request-timeout=15s", "--tcp-connection-timeout=30s", ]); assert_eq!( @@ -465,7 +467,7 @@ mod application_config_tests { idle_connection_timeout: Duration::from_str("3s").unwrap(), unproxied_connection_timeout: Some(Duration::from_str("4s").unwrap()), authentication_request_timeout: Duration::from_str("6s").unwrap(), - http_request_timeout: Duration::from_str("11s").unwrap(), + http_request_timeout: Some(Duration::from_str("15s").unwrap()), tcp_connection_timeout: Some(Duration::from_str("30s").unwrap()) } ) diff --git a/src/http.rs b/src/http.rs index 018773f..44d9e9b 100644 --- a/src/http.rs +++ b/src/http.rs @@ -121,8 +121,8 @@ where pub(crate) protocol: Protocol, // Configuration on which type of channel to retrieve from the handler. pub(crate) proxy_type: ProxyType, - // Duration until an outgoing request is canceled. - pub(crate) http_request_timeout: Duration, + // Optional duration until an outgoing request is canceled. + pub(crate) http_request_timeout: Option, // Optional duration until an established Websocket connection is canceled. pub(crate) websocket_timeout: Option, // If set, disables sending HTTP logs to the handler. @@ -283,13 +283,13 @@ where warn!("Connection failed: {:?}", err); } }); - // Await for a response under the given duration. - let response = timeout( - proxy_data.http_request_timeout, - sender.send_request(request), - ) - .await - .map_err(|_| ServerError::RequestTimeout)??; + let response = match proxy_data.http_request_timeout { + // Await for a response under the given duration. + Some(duration) => timeout(duration, sender.send_request(request)) + .await + .map_err(|_| ServerError::RequestTimeout)??, + None => sender.send_request(request).await?, + }; let elapsed_time = timer.elapsed(); http_log( HttpLog { @@ -317,13 +317,13 @@ where let request_type = request_upgrade.to_str()?.to_string(); // Retrieve the OnUpgrade from the incoming request let upgraded_request = hyper::upgrade::on(&mut request); - // Await for a response under the given duration. - let mut response = timeout( - proxy_data.http_request_timeout, - sender.send_request(request), - ) - .await - .map_err(|_| ServerError::RequestTimeout)??; + let mut response = match proxy_data.http_request_timeout { + // Await for a response under the given duration. + Some(duration) => timeout(duration, sender.send_request(request)) + .await + .map_err(|_| ServerError::RequestTimeout)??, + None => sender.send_request(request).await?, + }; let elapsed_time = timer.elapsed(); http_log( HttpLog { @@ -395,7 +395,7 @@ mod proxy_handler_tests { use hyper::{body::Incoming, service::service_fn, HeaderMap, Request, StatusCode}; use hyper_util::rt::TokioIo; use std::{marker::PhantomData, sync::Arc, time::Duration}; - use tokio::{io::DuplexStream, sync::mpsc}; + use tokio::{io::DuplexStream, sync::mpsc, time::sleep}; use tokio_tungstenite::client_async; use tower::Service; @@ -403,6 +403,7 @@ mod proxy_handler_tests { config::LoadBalancing, connection_handler::{ConnectionHttpData, MockConnectionHandler}, connections::ConnectionMap, + http::ServerError, quota::{DummyQuotaHandler, TokenHolder, UserIdentification}, reactor::MockConnectionMapReactor, telemetry::Telemetry, @@ -441,7 +442,7 @@ mod proxy_handler_tests { }), protocol: Protocol::Http { port: 80 }, proxy_type: ProxyType::Tunneling, - http_request_timeout: Duration::from_secs(5), + http_request_timeout: None, websocket_timeout: None, disable_http_logs: false, _phantom_data: PhantomData, @@ -483,7 +484,7 @@ mod proxy_handler_tests { }), protocol: Protocol::Http { port: 80 }, proxy_type: ProxyType::Tunneling, - http_request_timeout: Duration::from_secs(5), + http_request_timeout: None, websocket_timeout: None, disable_http_logs: false, _phantom_data: PhantomData, @@ -527,7 +528,7 @@ mod proxy_handler_tests { }), protocol: Protocol::Http { port: 80 }, proxy_type: ProxyType::Tunneling, - http_request_timeout: Duration::from_secs(5), + http_request_timeout: None, websocket_timeout: None, disable_http_logs: false, _phantom_data: PhantomData, @@ -592,7 +593,7 @@ mod proxy_handler_tests { }), protocol: Protocol::TlsRedirect { from: 80, to: 443 }, proxy_type: ProxyType::Tunneling, - http_request_timeout: Duration::from_secs(5), + http_request_timeout: None, websocket_timeout: None, disable_http_logs: false, _phantom_data: PhantomData, @@ -660,7 +661,7 @@ mod proxy_handler_tests { }), protocol: Protocol::TlsRedirect { from: 80, to: 8443 }, proxy_type: ProxyType::Tunneling, - http_request_timeout: Duration::from_secs(5), + http_request_timeout: None, websocket_timeout: None, disable_http_logs: false, _phantom_data: PhantomData, @@ -728,7 +729,7 @@ mod proxy_handler_tests { }), protocol: Protocol::Http { port: 80 }, proxy_type: ProxyType::Tunneling, - http_request_timeout: Duration::from_secs(5), + http_request_timeout: None, websocket_timeout: None, disable_http_logs: false, _phantom_data: PhantomData, @@ -796,7 +797,7 @@ mod proxy_handler_tests { }), protocol: Protocol::Http { port: 80 }, proxy_type: ProxyType::Tunneling, - http_request_timeout: Duration::from_secs(5), + http_request_timeout: None, websocket_timeout: None, disable_http_logs: false, _phantom_data: PhantomData, @@ -815,6 +816,101 @@ mod proxy_handler_tests { ); } + #[tokio::test] + async fn returns_error_for_outgoing_request_timeout() { + let conn_manager: Arc< + ConnectionMap< + String, + Arc>, + MockConnectionMapReactor, + >, + > = Arc::new(ConnectionMap::new( + LoadBalancing::Allow, + Arc::new(Box::new(DummyQuotaHandler)), + None, + )); + let (server, handler) = tokio::io::duplex(1024); + let (logging_tx, logging_rx) = mpsc::unbounded_channel::>(); + let mut mock = MockConnectionHandler::new(); + mock.expect_log_channel() + .once() + .return_once(move || Some(logging_tx)); + mock.expect_tunneling_channel() + .once() + .return_once(move |_, _| Ok(handler)); + mock.expect_http_data().once().return_once(move || { + Some(ConnectionHttpData { + redirect_http_to_https_port: None, + is_aliasing: false, + }) + }); + conn_manager + .insert( + "slow.handler".into(), + "127.0.0.1:12345".parse().unwrap(), + TokenHolder::User(UserIdentification::Username("a".into())), + Arc::new(mock), + ) + .unwrap(); + let request = Request::builder() + .method("GET") + .uri("/slow_endpoint") + .header("host", "slow.handler") + .body(Empty::::new()) + .unwrap(); + let router = axum::Router::new() + .route( + "/slow_endpoint", + axum::routing::get(|| async move { + sleep(Duration::from_secs(1)).await; + "Slow hello." + }), + ) + .into_service(); + let router_service = service_fn(move |req: Request| router.clone().call(req)); + let jh = tokio::spawn(async move { + hyper_util::server::conn::auto::Builder::new(hyper_util::rt::TokioExecutor::new()) + .serve_connection(TokioIo::new(server), router_service) + .await + .expect("Invalid request"); + }); + assert!( + logging_rx.is_empty(), + "shouldn't log before handling request" + ); + let response = proxy_handler( + request, + "127.0.0.1:12345".parse().unwrap(), + None, + Arc::new(ProxyData { + conn_manager: Arc::clone(&conn_manager), + telemetry: Arc::new(Telemetry::new()), + domain_redirect: Arc::new(DomainRedirect { + from: "main.domain".into(), + to: "https://example.com".into(), + }), + protocol: Protocol::Https { port: 443 }, + proxy_type: ProxyType::Tunneling, + http_request_timeout: Some(Duration::from_millis(500)), + websocket_timeout: None, + disable_http_logs: false, + _phantom_data: PhantomData, + }), + ) + .await; + assert!( + logging_rx.is_empty(), + "shouldn't log if failed to proxy request" + ); + assert!(response.is_err(), "should fail if timed out"); + let error = response.unwrap_err(); + assert!(matches!( + error.downcast().unwrap(), + ServerError::RequestTimeout + )); + jh.abort(); + } + #[tokio::test] async fn returns_response_for_existing_handler() { let conn_manager: Arc< @@ -896,7 +992,7 @@ mod proxy_handler_tests { }), protocol: Protocol::Https { port: 443 }, proxy_type: ProxyType::Tunneling, - http_request_timeout: Duration::from_secs(5), + http_request_timeout: None, websocket_timeout: None, disable_http_logs: false, _phantom_data: PhantomData, @@ -994,7 +1090,7 @@ mod proxy_handler_tests { }), protocol: Protocol::Http { port: 80 }, proxy_type: ProxyType::Aliasing, - http_request_timeout: Duration::from_secs(5), + http_request_timeout: None, websocket_timeout: None, disable_http_logs: false, _phantom_data: PhantomData, @@ -1092,7 +1188,7 @@ mod proxy_handler_tests { }), protocol: Protocol::Https { port: 443 }, proxy_type: ProxyType::Tunneling, - http_request_timeout: Duration::from_secs(5), + http_request_timeout: None, websocket_timeout: None, disable_http_logs: false, _phantom_data: PhantomData, @@ -1184,7 +1280,7 @@ mod proxy_handler_tests { }), protocol: Protocol::Https { port: 443 }, proxy_type: ProxyType::Tunneling, - http_request_timeout: Duration::from_secs(5), + http_request_timeout: None, websocket_timeout: None, disable_http_logs: false, _phantom_data: PhantomData, diff --git a/src/lib.rs b/src/lib.rs index d350e90..20d7d4a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -492,7 +492,7 @@ pub async fn entrypoint(config: ApplicationConfig) -> anyhow::Result<()> { }, // Always use aliasing channels instead of tunneling channels. proxy_type: ProxyType::Aliasing, - http_request_timeout: config.http_request_timeout.into(), + http_request_timeout: config.http_request_timeout.map(Into::into), websocket_timeout: config.tcp_connection_timeout.map(Into::into), disable_http_logs: config.disable_http_logs, _phantom_data: PhantomData, @@ -562,7 +562,7 @@ pub async fn entrypoint(config: ApplicationConfig) -> anyhow::Result<()> { }, // Always use tunneling channels. proxy_type: ProxyType::Tunneling, - http_request_timeout: config.http_request_timeout.into(), + http_request_timeout: config.http_request_timeout.map(Into::into), websocket_timeout: config.tcp_connection_timeout.map(Into::into), disable_http_logs: config.disable_http_logs, _phantom_data: PhantomData, @@ -626,7 +626,7 @@ pub async fn entrypoint(config: ApplicationConfig) -> anyhow::Result<()> { }, // Always use tunneling channels. proxy_type: ProxyType::Tunneling, - http_request_timeout: config.http_request_timeout.into(), + http_request_timeout: config.http_request_timeout.map(Into::into), websocket_timeout: config.tcp_connection_timeout.map(Into::into), disable_http_logs: config.disable_http_logs, _phantom_data: PhantomData,