From dd99a4d6a0e635a52b472e3b0ca30009a3eca5e8 Mon Sep 17 00:00:00 2001 From: licorn Date: Tue, 24 Mar 2026 21:55:38 +0100 Subject: [PATCH 1/2] fix(server): use ALPN-negotiated protocol for TLS connections The gateway server used hyper's auto-detection (byte-sniffing the HTTP/2 connection preface) for all connections, including TLS connections where ALPN already negotiated the protocol. This could misidentify h2 connections as HTTP/1.1 when the first read returned a partial preface, making gRPC unusable. Add MultiplexService::serve_h2() which starts the HTTP/2 state machine directly, bypassing auto-detection. After TLS handshake, check the ALPN-negotiated protocol and route h2 connections to serve_h2(). Plaintext connections continue using auto-detection. Also add TLS status logging (cert paths, ALPN config) on startup and per-connection ALPN protocol logging at debug level. Closes #535 --- crates/openshell-server/src/lib.rs | 37 +++++++++++++++++++++-- crates/openshell-server/src/multiplex.rs | 38 ++++++++++++++++++++++++ crates/openshell-server/src/tls.rs | 29 ++++++++++++++++++ 3 files changed, 101 insertions(+), 3 deletions(-) diff --git a/crates/openshell-server/src/lib.rs b/crates/openshell-server/src/lib.rs index e827b362..c8003cde 100644 --- a/crates/openshell-server/src/lib.rs +++ b/crates/openshell-server/src/lib.rs @@ -33,6 +33,7 @@ use tracing::{debug, error, info}; pub use grpc::OpenShellService; pub use http::{health_router, http_router}; pub use multiplex::{MultiplexService, MultiplexedService}; +pub use multiplex::ALPN_H2; use persistence::Store; use sandbox::{SandboxClient, spawn_sandbox_watcher, spawn_store_reconciler}; use sandbox_index::SandboxIndex; @@ -180,12 +181,20 @@ pub async fn run_server(config: Config, tracing_log_bus: TracingLogBus) -> Resul // Build TLS acceptor when TLS is configured; otherwise serve plaintext. let tls_acceptor = if let Some(tls) = &config.tls { - Some(TlsAcceptor::from_files( + let acceptor = TlsAcceptor::from_files( &tls.cert_path, &tls.key_path, &tls.client_ca_path, tls.allow_unauthenticated, - )?) + )?; + info!( + cert = %tls.cert_path.display(), + key = %tls.key_path.display(), + client_ca = %tls.client_ca_path.display(), + allow_unauthenticated = tls.allow_unauthenticated, + "TLS enabled — ALPN advertises h2 + http/1.1", + ); + Some(acceptor) } else { info!("TLS disabled — accepting plaintext connections"); None @@ -208,7 +217,24 @@ pub async fn run_server(config: Config, tracing_log_bus: TracingLogBus) -> Resul tokio::spawn(async move { match tls_acceptor.inner().accept(stream).await { Ok(tls_stream) => { - if let Err(e) = service.serve(tls_stream).await { + // Use ALPN-negotiated protocol when available. This + // avoids the byte-sniffing auto-detection in + // `serve()`, which can misidentify h2 connections as + // HTTP/1.1 when the first read returns a partial + // preface. + let alpn = tls_stream + .get_ref() + .1 + .alpn_protocol() + .unwrap_or_default(); + let result = if alpn == ALPN_H2 { + debug!(client = %addr, "ALPN negotiated h2 — serving HTTP/2"); + service.serve_h2(tls_stream).await + } else { + debug!(client = %addr, alpn = ?String::from_utf8_lossy(alpn), "ALPN fallback — auto-detecting protocol"); + service.serve(tls_stream).await + }; + if let Err(e) = result { error!(error = %e, client = %addr, "Connection error"); } } @@ -255,4 +281,9 @@ mod tests { assert!(!is_benign_tls_handshake_failure(&error)); } } + + #[test] + fn alpn_h2_constant_matches_standard_protocol_id() { + assert_eq!(super::ALPN_H2, b"h2"); + } } diff --git a/crates/openshell-server/src/multiplex.rs b/crates/openshell-server/src/multiplex.rs index 5ba44b1e..7dcd8669 100644 --- a/crates/openshell-server/src/multiplex.rs +++ b/crates/openshell-server/src/multiplex.rs @@ -27,6 +27,9 @@ use tower::ServiceExt; use crate::{OpenShellService, ServerState, http_router, inference::InferenceService}; +/// ALPN protocol identifier for HTTP/2. +pub const ALPN_H2: &[u8] = b"h2"; + /// Maximum inbound gRPC message size (1 MB). /// /// Replaces tonic's implicit 4 MB default with a conservative limit to @@ -49,6 +52,11 @@ impl MultiplexService { } /// Serve a connection, routing to gRPC or HTTP based on content-type. + /// + /// Uses hyper's auto-detection to determine whether the connection speaks + /// HTTP/1.1 or HTTP/2. For TLS connections where ALPN already negotiated + /// the protocol, prefer [`serve_h2`](Self::serve_h2) to skip the + /// auto-detection round-trip. pub async fn serve(&self, stream: S) -> Result<(), Box> where S: AsyncRead + AsyncWrite + Unpin + Send + 'static, @@ -68,6 +76,36 @@ impl MultiplexService { Ok(()) } + + /// Serve a connection that has already been identified as HTTP/2. + /// + /// This is the preferred path for TLS connections where ALPN negotiated + /// `h2`. It avoids the byte-sniffing auto-detection in [`serve`](Self::serve) + /// and immediately starts the HTTP/2 state machine, which eliminates a + /// class of edge-case failures (partial reads, buffering delays) that can + /// cause the auto-detector to misidentify an h2 connection as HTTP/1.1. + pub async fn serve_h2( + &self, + stream: S, + ) -> Result<(), Box> + where + S: AsyncRead + AsyncWrite + Unpin + Send + 'static, + { + let openshell = OpenShellServer::new(OpenShellService::new(self.state.clone())) + .max_decoding_message_size(MAX_GRPC_DECODE_SIZE); + let inference = InferenceServer::new(InferenceService::new(self.state.clone())) + .max_decoding_message_size(MAX_GRPC_DECODE_SIZE); + let grpc_service = GrpcRouter::new(openshell, inference); + let http_service = http_router(self.state.clone()); + + let service = MultiplexedService::new(grpc_service, http_service); + + hyper::server::conn::http2::Builder::new(TokioExecutor::new()) + .serve_connection(TokioIo::new(stream), service) + .await?; + + Ok(()) + } } /// Combined gRPC service that routes between `OpenShell` and Inference services diff --git a/crates/openshell-server/src/tls.rs b/crates/openshell-server/src/tls.rs index 95c18608..b10d5ec1 100644 --- a/crates/openshell-server/src/tls.rs +++ b/crates/openshell-server/src/tls.rs @@ -119,3 +119,32 @@ fn load_key(path: &Path) -> Result> { Err(Error::tls("no private key found in file")) } + +#[cfg(test)] +mod tests { + use super::*; + use std::path::Path; + + #[test] + fn from_files_rejects_missing_cert() { + let result = TlsAcceptor::from_files( + Path::new("/nonexistent/cert.pem"), + Path::new("/nonexistent/key.pem"), + Path::new("/nonexistent/ca.pem"), + false, + ); + assert!(result.is_err()); + } + + #[test] + fn load_certs_rejects_nonexistent_file() { + let result = load_certs(Path::new("/nonexistent/cert.pem")); + assert!(result.is_err()); + } + + #[test] + fn load_key_rejects_nonexistent_file() { + let result = load_key(Path::new("/nonexistent/key.pem")); + assert!(result.is_err()); + } +} From a97dd49425610f1d2b30a5cd4a8ccb9f55039aba Mon Sep 17 00:00:00 2001 From: licorn Date: Wed, 25 Mar 2026 16:24:53 +0100 Subject: [PATCH 2/2] style(server): apply rustfmt to TLS routing change --- crates/openshell-server/src/lib.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/crates/openshell-server/src/lib.rs b/crates/openshell-server/src/lib.rs index c8003cde..65016c38 100644 --- a/crates/openshell-server/src/lib.rs +++ b/crates/openshell-server/src/lib.rs @@ -32,8 +32,8 @@ use tracing::{debug, error, info}; pub use grpc::OpenShellService; pub use http::{health_router, http_router}; -pub use multiplex::{MultiplexService, MultiplexedService}; pub use multiplex::ALPN_H2; +pub use multiplex::{MultiplexService, MultiplexedService}; use persistence::Store; use sandbox::{SandboxClient, spawn_sandbox_watcher, spawn_store_reconciler}; use sandbox_index::SandboxIndex; @@ -222,11 +222,7 @@ pub async fn run_server(config: Config, tracing_log_bus: TracingLogBus) -> Resul // `serve()`, which can misidentify h2 connections as // HTTP/1.1 when the first read returns a partial // preface. - let alpn = tls_stream - .get_ref() - .1 - .alpn_protocol() - .unwrap_or_default(); + let alpn = tls_stream.get_ref().1.alpn_protocol().unwrap_or_default(); let result = if alpn == ALPN_H2 { debug!(client = %addr, "ALPN negotiated h2 — serving HTTP/2"); service.serve_h2(tls_stream).await