From fa67ae375dae8f7114fa1903c84936f267b0ce3c Mon Sep 17 00:00:00 2001 From: Bryan Ashby Date: Fri, 2 Oct 2026 10:01:36 -0600 Subject: [PATCH 1/9] send WebSocketConfig headers, subprotocols and user agent in the upgrade request (#165) WebSocketTransport::connect built the upgrade request from a fixed set of headers and always offered Sec-WebSocket-Protocol: mqtt, so with_header, with_subprotocol(s) and with_user_agent never reached the server. Request construction moves to WebSocketConfig::build_handshake_request, which starts from tungstenite's into_client_request (fixing a Host header that dropped non-default ports) and adds the configured subprotocols, user agent and custom headers. Invalid, reserved or case-duplicated headers and non-token subprotocols are rejected before dialing. WebSocketConfig's Debug no longer prints header values. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 16 + crates/mqtt5/src/transport/websocket.rs | 362 ++++++++++++++++-- .../tests/websocket_config_property_tests.rs | 61 ++- .../tests/websocket_handshake_integration.rs | 127 ++++++ 4 files changed, 539 insertions(+), 27 deletions(-) create mode 100644 crates/mqtt5/tests/websocket_handshake_integration.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 60e8bfd5..a03486a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,22 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed + +- **The WebSocket client transport now sends its configured headers, subprotocols and user agent** (#165). `WebSocketTransport::connect` built the upgrade request from a fixed set of headers and always offered `Sec-WebSocket-Protocol: mqtt`, so anything set with `WebSocketConfig::with_header`, `with_subprotocol`, `with_subprotocols` or `with_user_agent` never reached the server, and brokers that authenticate the upgrade request through custom headers rejected the connection. The request now carries every custom header, the configured subprotocols in order (or `mqtt` when none are configured), and the user agent when one is set. The default user agent is now `mqtt5/` instead of `mqtt-v5/0.4.0`. +- **The upgrade request's `Host` header now includes a non-default port**, so `ws://broker:8080/mqtt` sends `Host: broker:8080` rather than `Host: broker`. + +### Changed + +- **`connect` rejects a WebSocket configuration it cannot send faithfully, before dialing.** It fails with `MqttError::Configuration` when a custom header has an invalid name or value, repeats another custom header's name (case-insensitively), or names a header the transport sets itself (`Host`, `Connection`, `Upgrade`, `Sec-WebSocket-Version`, `Sec-WebSocket-Key`, `Sec-WebSocket-Extensions`); when `Sec-WebSocket-Protocol` or `User-Agent` is set as a custom header instead of through its setter; or when a subprotocol is not a valid HTTP token. +- `WebSocketConfig`'s `Debug` output lists custom header names but not their values, which often carry credentials. + +### Added + +- `WebSocketConfig::build_handshake_request`, which returns the upgrade request the configuration produces. + ## [mqtt5 0.45.1] - 2026-10-01 ### Fixed diff --git a/crates/mqtt5/src/transport/websocket.rs b/crates/mqtt5/src/transport/websocket.rs index d73ec72b..664fe826 100644 --- a/crates/mqtt5/src/transport/websocket.rs +++ b/crates/mqtt5/src/transport/websocket.rs @@ -51,29 +51,68 @@ use std::collections::HashMap; use std::net::SocketAddr; use tokio::net::TcpStream; use tokio_tungstenite::{ - tungstenite::{self, http::Request, protocol::Message}, + tungstenite::{ + client::IntoClientRequest, + http::{ + header::{SEC_WEBSOCKET_PROTOCOL, USER_AGENT}, + HeaderName, HeaderValue, Request, + }, + protocol::Message, + }, MaybeTlsStream, WebSocketStream, }; use tracing::{debug, error, info, instrument}; use url::Url; +/// Subprotocol offered when none is configured. MQTT-6.0.0-3 requires the +/// client to include "mqtt" in the subprotocols it offers. +const DEFAULT_SUBPROTOCOL: &str = "mqtt"; + +/// Handshake headers the transport sets itself. A custom header with one of +/// these names would duplicate or override the handshake, so it is rejected. +/// `sec-websocket-extensions` is included because the transport negotiates no +/// extensions and cannot speak one a server accepts. +const RESERVED_HEADERS: [&str; 6] = [ + "host", + "connection", + "upgrade", + "sec-websocket-version", + "sec-websocket-key", + "sec-websocket-extensions", +]; + /// WebSocket transport configuration -#[derive(Debug)] pub struct WebSocketConfig { /// WebSocket URL (ws:// or wss://) pub url: Url, /// Connection timeout pub timeout: Duration, - /// Subprotocols to negotiate (e.g., "mqtt", "mqttv3.1", "mqttv5.0") + /// Subprotocols to offer, in order of preference (e.g., "mqtt", "mqttv5.0"). + /// "mqtt" is offered when this is empty. pub subprotocols: Vec, /// Custom HTTP headers for the WebSocket handshake pub headers: HashMap, - /// User agent string + /// User agent string, sent as the `User-Agent` header when set pub user_agent: Option, /// TLS configuration for secure WebSocket connections (wss://) pub tls_config: Option, } +impl std::fmt::Debug for WebSocketConfig { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // Header values often carry credentials, so only the names are shown. + let header_names: Vec<&str> = self.headers.keys().map(String::as_str).collect(); + f.debug_struct("WebSocketConfig") + .field("url", &self.url.as_str()) + .field("timeout", &self.timeout) + .field("subprotocols", &self.subprotocols) + .field("headers", &header_names) + .field("user_agent", &self.user_agent) + .field("tls_config", &self.tls_config) + .finish() + } +} + impl WebSocketConfig { /// Creates a new WebSocket configuration /// @@ -96,9 +135,9 @@ impl WebSocketConfig { Ok(Self { url: parsed_url, timeout: Duration::from_secs(30), - subprotocols: vec!["mqtt".to_string()], + subprotocols: vec![DEFAULT_SUBPROTOCOL.to_string()], headers: HashMap::new(), - user_agent: Some("mqtt-v5/0.4.0".to_string()), + user_agent: Some(concat!("mqtt5/", env!("CARGO_PKG_VERSION")).to_string()), tls_config: None, }) } @@ -110,7 +149,10 @@ impl WebSocketConfig { self } - /// Sets the WebSocket subprotocols to negotiate + /// Sets the WebSocket subprotocols to offer, in order of preference + /// + /// The list is sent exactly as given. MQTT-6.0.0-3 requires the client to + /// offer "mqtt", so keep it in the list unless the broker requires otherwise. #[must_use] pub fn with_subprotocols(mut self, subprotocols: &[&str]) -> Self { self.subprotocols = subprotocols @@ -120,14 +162,26 @@ impl WebSocketConfig { self } - /// Sets a single WebSocket subprotocol + /// Sets a single WebSocket subprotocol to offer + /// + /// MQTT-6.0.0-3 requires the client to offer "mqtt"; see + /// [`with_subprotocols`](Self::with_subprotocols) to offer it alongside another. #[must_use] pub fn with_subprotocol(mut self, subprotocol: &str) -> Self { self.subprotocols = vec![subprotocol.to_string()]; self } - /// Adds a custom HTTP header + /// Adds a custom HTTP header to the WebSocket handshake request + /// + /// The name and value are validated when the request is built, so an + /// invalid header makes [`connect`](WebSocketTransport::connect) fail + /// rather than this call. Names are matched case-insensitively and must not + /// repeat. The handshake headers (`Host`, `Connection`, `Upgrade`, + /// `Sec-WebSocket-Version`, `Sec-WebSocket-Key`, `Sec-WebSocket-Extensions`) + /// are rejected, as are `Sec-WebSocket-Protocol` and `User-Agent`, which are + /// set with [`with_subprotocols`](Self::with_subprotocols) and + /// [`with_user_agent`](Self::with_user_agent). #[must_use] pub fn with_header(mut self, name: &str, value: &str) -> Self { self.headers.insert(name.to_string(), value.to_string()); @@ -320,6 +374,90 @@ impl WebSocketConfig { pub fn take_tls_config(&mut self) -> Option { self.tls_config.take() } + + /// Builds the WebSocket handshake request this configuration produces + /// + /// The request carries the configured subprotocols (or "mqtt" when none + /// are configured), the user agent when one is set, and every custom + /// header. Each call generates a fresh `Sec-WebSocket-Key`. + /// + /// # Errors + /// + /// Returns an error if: + /// - A subprotocol is not a valid HTTP token + /// - The user agent is not a valid header value + /// - A custom header has an invalid name or value, names a header the + /// transport sets itself, or repeats another custom header's name + pub fn build_handshake_request(&self) -> Result> { + let mut request = self.url.as_str().into_client_request().map_err(|e| { + MqttError::ConnectionError(format!("Failed to build WebSocket request: {e}")) + })?; + let headers = request.headers_mut(); + + let subprotocols = if self.subprotocols.is_empty() { + DEFAULT_SUBPROTOCOL.to_string() + } else { + if let Some(invalid) = self.subprotocols.iter().find(|p| !is_http_token(p)) { + return Err(MqttError::Configuration(format!( + "Invalid WebSocket subprotocol {invalid:?}: must be a non-empty HTTP token" + ))); + } + self.subprotocols.join(", ") + }; + headers.insert( + SEC_WEBSOCKET_PROTOCOL, + header_value("Sec-WebSocket-Protocol", &subprotocols)?, + ); + + if let Some(user_agent) = &self.user_agent { + headers.insert(USER_AGENT, header_value("User-Agent", user_agent)?); + } + + for (name, value) in &self.headers { + let header_name = HeaderName::from_bytes(name.as_bytes()).map_err(|e| { + MqttError::Configuration(format!("Invalid WebSocket header name {name:?}: {e}")) + })?; + if RESERVED_HEADERS.contains(&header_name.as_str()) { + return Err(MqttError::Configuration(format!( + "WebSocket header {name:?} is set by the transport and cannot be overridden" + ))); + } + if header_name == SEC_WEBSOCKET_PROTOCOL { + return Err(MqttError::Configuration(format!( + "WebSocket header {name:?} cannot be set directly; use with_subprotocols" + ))); + } + if header_name == USER_AGENT { + return Err(MqttError::Configuration(format!( + "WebSocket header {name:?} cannot be set directly; use with_user_agent" + ))); + } + if headers.contains_key(&header_name) { + return Err(MqttError::Configuration(format!( + "WebSocket header {name:?} is configured more than once \ + (names are case-insensitive)" + ))); + } + headers.insert(header_name, header_value(name, value)?); + } + + Ok(request) + } +} + +/// Parses a handshake header value, naming the header in the error. +fn header_value(name: &str, value: &str) -> Result { + HeaderValue::from_str(value).map_err(|e| { + MqttError::Configuration(format!("Invalid value for WebSocket header {name:?}: {e}")) + }) +} + +/// Whether `s` is an HTTP token (RFC 9110 §5.6.2), as RFC 6455 §4.1 requires +/// of each offered subprotocol. +fn is_http_token(s: &str) -> bool { + !s.is_empty() + && s.bytes() + .all(|b| b.is_ascii_alphanumeric() || b"!#$%&'*+-.^_`|~".contains(&b)) } /// WebSocket transport implementation @@ -354,7 +492,9 @@ impl WebSocketTransport { &self.config.url } - /// Gets the negotiated subprotocol (if any) + /// Gets the most preferred configured subprotocol (if any) + /// + /// This is the first subprotocol offered, not the one the server selected. #[must_use] pub fn subprotocol(&self) -> Option<&str> { self.config.subprotocols.first().map(String::as_str) @@ -503,21 +643,7 @@ impl Transport for WebSocketTransport { return Err(MqttError::AlreadyConnected); } - let request = Request::builder() - .uri(self.config.url.as_str()) - .header("Host", self.config.url.host_str().unwrap_or("localhost")) - .header("Connection", "Upgrade") - .header("Upgrade", "websocket") - .header("Sec-WebSocket-Version", "13") - .header( - "Sec-WebSocket-Key", - tungstenite::handshake::client::generate_key(), - ) - .header("Sec-WebSocket-Protocol", "mqtt") - .body(()) - .map_err(|e| { - MqttError::ConnectionError(format!("Failed to build WebSocket request: {e}")) - })?; + let request = self.config.build_handshake_request()?; let ws_result = if self.config.is_secure() && self @@ -764,6 +890,192 @@ mod tests { assert_eq!(config.user_agent, Some("custom-client/1.0".to_string())); } + fn request_header<'a>(request: &'a Request<()>, name: &str) -> Option<&'a str> { + request + .headers() + .get(name) + .map(|v| v.to_str().expect("ascii header")) + } + + fn handshake_error(config: &WebSocketConfig) -> String { + config + .build_handshake_request() + .expect_err("handshake request should be rejected") + .to_string() + } + + #[test] + fn test_handshake_request_defaults() { + let config = WebSocketConfig::new("ws://localhost:8080/mqtt").unwrap(); + let request = config.build_handshake_request().unwrap(); + + assert_eq!(request.uri(), "ws://localhost:8080/mqtt"); + assert_eq!(request_header(&request, "Host"), Some("localhost:8080")); + assert_eq!(request_header(&request, "Connection"), Some("Upgrade")); + assert_eq!(request_header(&request, "Upgrade"), Some("websocket")); + assert_eq!( + request_header(&request, "Sec-WebSocket-Version"), + Some("13") + ); + assert!(request_header(&request, "Sec-WebSocket-Key").is_some()); + assert_eq!( + request_header(&request, "Sec-WebSocket-Protocol"), + Some("mqtt") + ); + assert_eq!( + request_header(&request, "User-Agent"), + Some(concat!("mqtt5/", env!("CARGO_PKG_VERSION"))) + ); + } + + #[test] + fn test_handshake_request_applies_configuration() { + let config = WebSocketConfig::new("wss://broker.example.com/mqtt") + .unwrap() + .with_subprotocols(&["mqttv5.0", "mqtt"]) + .with_header("Authorization", "Bearer token123") + .with_header("x-amz-customauthorizer-name", "authorizer") + .with_user_agent("custom-client/1.0"); + let request = config.build_handshake_request().unwrap(); + + assert_eq!( + request_header(&request, "Sec-WebSocket-Protocol"), + Some("mqttv5.0, mqtt") + ); + assert_eq!( + request_header(&request, "Authorization"), + Some("Bearer token123") + ); + assert_eq!( + request_header(&request, "x-amz-customauthorizer-name"), + Some("authorizer") + ); + assert_eq!( + request_header(&request, "User-Agent"), + Some("custom-client/1.0") + ); + } + + #[test] + fn test_handshake_request_offers_mqtt_when_no_subprotocols() { + let mut config = WebSocketConfig::new("ws://localhost/mqtt").unwrap(); + config.subprotocols.clear(); + let request = config.build_handshake_request().unwrap(); + + assert_eq!( + request_header(&request, "Sec-WebSocket-Protocol"), + Some("mqtt") + ); + } + + #[test] + fn test_handshake_request_omits_unset_user_agent() { + let mut config = WebSocketConfig::new("ws://localhost/mqtt").unwrap(); + config.user_agent = None; + let request = config.build_handshake_request().unwrap(); + + assert_eq!(request_header(&request, "User-Agent"), None); + } + + #[test] + fn test_handshake_request_host_header() { + let default_port = WebSocketConfig::new("wss://broker.example.com/mqtt").unwrap(); + let request = default_port.build_handshake_request().unwrap(); + assert_eq!(request_header(&request, "Host"), Some("broker.example.com")); + + let with_userinfo = WebSocketConfig::new("ws://user:pass@localhost:8080/mqtt").unwrap(); + let request = with_userinfo.build_handshake_request().unwrap(); + assert_eq!(request_header(&request, "Host"), Some("localhost:8080")); + + let ipv6 = WebSocketConfig::new("ws://[::1]:8080/mqtt").unwrap(); + let request = ipv6.build_handshake_request().unwrap(); + assert_eq!(request_header(&request, "Host"), Some("[::1]:8080")); + } + + #[test] + fn test_handshake_request_rejects_reserved_headers() { + for name in [ + "Host", + "connection", + "UPGRADE", + "Sec-WebSocket-Version", + "Sec-WebSocket-Key", + "Sec-WebSocket-Extensions", + ] { + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_header(name, "value"); + assert!( + handshake_error(&config).contains("set by the transport"), + "{name} should be rejected" + ); + } + } + + #[test] + fn test_handshake_request_rejects_headers_with_dedicated_setters() { + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_header("sec-websocket-protocol", "mqttv5.0"); + assert!(handshake_error(&config).contains("use with_subprotocols")); + + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_header("User-Agent", "other/1.0"); + assert!(handshake_error(&config).contains("use with_user_agent")); + } + + #[test] + fn test_handshake_request_rejects_case_insensitive_duplicates() { + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_header("Authorization", "Bearer a") + .with_header("authorization", "Bearer b"); + assert!(handshake_error(&config).contains("more than once")); + } + + #[test] + fn test_handshake_request_rejects_invalid_headers() { + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_header("Bad Name", "value"); + assert!(handshake_error(&config).contains("Invalid WebSocket header name")); + + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_header("X-Injected", "value\r\nX-Other: smuggled"); + assert!(handshake_error(&config).contains("Invalid value")); + + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_user_agent("agent\r\n"); + assert!(handshake_error(&config).contains("User-Agent")); + } + + #[test] + fn test_handshake_request_rejects_invalid_subprotocols() { + for subprotocol in ["", "mqtt, mqttv5.0", "mqtt v5", "mqtt\r\n"] { + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_subprotocol(subprotocol); + assert!( + handshake_error(&config).contains("Invalid WebSocket subprotocol"), + "{subprotocol:?} should be rejected" + ); + } + } + + #[test] + fn test_websocket_config_debug_redacts_header_values() { + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_header("Authorization", "Bearer secret-token"); + let debug = format!("{config:?}"); + + assert!(debug.contains("Authorization")); + assert!(!debug.contains("secret-token")); + } + #[tokio::test] async fn test_websocket_transport_creation() { let config = WebSocketConfig::new("ws://localhost:8080/mqtt").unwrap(); diff --git a/crates/mqtt5/tests/websocket_config_property_tests.rs b/crates/mqtt5/tests/websocket_config_property_tests.rs index 4a1a89aa..e8811fae 100644 --- a/crates/mqtt5/tests/websocket_config_property_tests.rs +++ b/crates/mqtt5/tests/websocket_config_property_tests.rs @@ -81,6 +81,19 @@ fn valid_http_headers() -> impl Strategy> { .prop_map(|btree| btree.into_iter().collect()) } +// Header names a custom header may not use (lowercase): the handshake headers +// the transport sets itself, plus the two that have dedicated setters. +const RESTRICTED_HEADERS: [&str; 8] = [ + "host", + "connection", + "upgrade", + "sec-websocket-version", + "sec-websocket-key", + "sec-websocket-extensions", + "sec-websocket-protocol", + "user-agent", +]; + // Generate valid subprotocols fn valid_subprotocols() -> impl Strategy> { prop::collection::vec("[a-zA-Z][a-zA-Z0-9._-]{0,20}", 0..5) @@ -189,9 +202,53 @@ proptest! { assert_eq!(config.subprotocols, subprotocols); } - if let Some(ua) = user_agent { - assert_eq!(config.user_agent, Some(ua)); + if let Some(ua) = &user_agent { + assert_eq!(config.user_agent.as_ref(), Some(ua)); } + + // The configuration must reach the handshake request, not just the struct. + let mut seen = std::collections::HashSet::new(); + let conflicting = headers.keys().any(|name| { + let lower = name.to_ascii_lowercase(); + RESTRICTED_HEADERS.contains(&lower.as_str()) || !seen.insert(lower) + }); + let request = config.build_handshake_request(); + if conflicting { + prop_assert!(request.is_err(), "conflicting headers {headers:?} were accepted"); + return Ok(()); + } + let request = request.expect("valid configuration builds a request"); + let sent = |name: &str| { + request + .headers() + .get(name) + .map(|v| v.to_str().expect("ascii header").to_string()) + }; + + for (name, value) in &headers { + prop_assert_eq!(sent(name), Some(value.clone())); + } + let expected_protocols = if subprotocols.is_empty() { + "mqtt".to_string() + } else { + subprotocols.join(", ") + }; + prop_assert_eq!(sent("Sec-WebSocket-Protocol"), Some(expected_protocols)); + prop_assert_eq!(sent("User-Agent"), config.user_agent.clone()); + } + + #[test] + fn prop_handshake_request_rejects_restricted_headers( + name in prop::sample::select(RESTRICTED_HEADERS.to_vec()), + uppercase in any::(), + value in "[A-Za-z0-9]{1,20}" + ) { + let name = if uppercase { name.to_ascii_uppercase() } else { name.to_string() }; + let config = WebSocketConfig::new("ws://localhost:8080/mqtt") + .unwrap() + .with_header(&name, &value); + + prop_assert!(config.build_handshake_request().is_err(), "{name} was accepted"); } #[test] diff --git a/crates/mqtt5/tests/websocket_handshake_integration.rs b/crates/mqtt5/tests/websocket_handshake_integration.rs new file mode 100644 index 00000000..8154c329 --- /dev/null +++ b/crates/mqtt5/tests/websocket_handshake_integration.rs @@ -0,0 +1,127 @@ +#![cfg(feature = "transport-websocket")] + +//! Checks the WebSocket upgrade request the client transport actually sends, +//! as seen by a server, rather than the configuration it was built from. + +use mqtt5::transport::websocket::{WebSocketConfig, WebSocketTransport}; +use mqtt5::Transport; +use std::sync::{Arc, Mutex}; +use std::time::Duration; +use tokio::net::TcpListener; +use tokio::time::timeout; +use tokio_tungstenite::tungstenite::handshake::server::{ + Callback, ErrorResponse, Request, Response, +}; +use tokio_tungstenite::tungstenite::http::HeaderMap; + +const WAIT: Duration = Duration::from_secs(5); + +/// Records the upgrade request's headers and selects the first offered +/// subprotocol, as a broker would. +struct CaptureRequest { + captured: Arc>>, +} + +impl Callback for CaptureRequest { + fn on_request(self, req: &Request, mut resp: Response) -> Result { + if let Some(first) = req + .headers() + .get("Sec-WebSocket-Protocol") + .and_then(|v| v.to_str().ok()) + .and_then(|v| v.split(',').next()) + { + resp.headers_mut().insert( + "Sec-WebSocket-Protocol", + first.trim().parse().expect("header"), + ); + } + *self.captured.lock().expect("lock") = Some(req.headers().clone()); + Ok(resp) + } +} + +/// Connects a transport built by `configure` to a loopback server and returns +/// the headers of the upgrade request the server received. +async fn received_headers(configure: impl FnOnce(WebSocketConfig) -> WebSocketConfig) -> HeaderMap { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let addr = listener.local_addr().expect("addr"); + let captured = Arc::new(Mutex::new(None)); + let callback = CaptureRequest { + captured: Arc::clone(&captured), + }; + let server = tokio::spawn(async move { + let (stream, _) = listener.accept().await.expect("accept"); + tokio_tungstenite::accept_hdr_async(stream, callback) + .await + .expect("server handshake") + }); + + let config = configure(WebSocketConfig::new(&format!("ws://{addr}/mqtt")).expect("config")); + let mut transport = WebSocketTransport::new(config); + timeout(WAIT, transport.connect()) + .await + .expect("connect timed out") + .expect("client handshake"); + let _server_ws = timeout(WAIT, server) + .await + .expect("server timed out") + .expect("server task"); + transport.close().await.expect("close"); + + let headers = captured.lock().expect("lock").take(); + headers.expect("server saw no upgrade request") +} + +fn header<'a>(headers: &'a HeaderMap, name: &str) -> Option<&'a str> { + headers.get(name).map(|v| v.to_str().expect("ascii header")) +} + +#[tokio::test] +async fn upgrade_request_carries_configured_headers_subprotocols_and_user_agent() { + let headers = received_headers(|config| { + config + .with_header("Authorization", "Bearer token123") + .with_header("X-Custom", "expected") + .with_subprotocols(&["mqttv5.0", "mqtt"]) + .with_user_agent("repro/1.0") + }) + .await; + + assert_eq!(header(&headers, "Authorization"), Some("Bearer token123")); + assert_eq!(header(&headers, "X-Custom"), Some("expected")); + assert_eq!( + header(&headers, "Sec-WebSocket-Protocol"), + Some("mqttv5.0, mqtt") + ); + assert_eq!(header(&headers, "User-Agent"), Some("repro/1.0")); +} + +#[tokio::test] +async fn upgrade_request_defaults_offer_mqtt_and_crate_user_agent() { + let headers = received_headers(|config| config).await; + + assert_eq!(header(&headers, "Sec-WebSocket-Protocol"), Some("mqtt")); + assert_eq!( + header(&headers, "User-Agent"), + Some(concat!("mqtt5/", env!("CARGO_PKG_VERSION"))) + ); + assert!(header(&headers, "Host").is_some_and(|host| host.starts_with("127.0.0.1:"))); +} + +#[tokio::test] +async fn invalid_header_fails_connect_before_dialing() { + let config = WebSocketConfig::new("ws://127.0.0.1:9/mqtt") + .expect("config") + .with_header("Host", "spoofed.example.com"); + let mut transport = WebSocketTransport::new(config); + + let err = transport + .connect() + .await + .expect_err("reserved header accepted"); + assert!( + err.to_string().contains("set by the transport"), + "unexpected error: {err}" + ); + assert!(!transport.is_connected()); +} From 4eaa525b84ebabb86756b2c0061447695ea5d5ab Mon Sep 17 00:00:00 2001 From: Bryan Ashby Date: Fri, 2 Oct 2026 10:30:02 -0600 Subject: [PATCH 2/9] reserve body headers, order custom headers, and note behavior changes (#165) Content-Length, Transfer-Encoding and Sec-WebSocket-Accept are now rejected as custom handshake headers alongside the existing reserved names. Custom headers are validated and sent in name order, so the request and the reported error do not depend on HashMap iteration. The integration test checks Host against the exact bound address, and the CHANGELOG calls out the changes that can affect existing configurations. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 12 +++- crates/mqtt5/src/transport/websocket.rs | 67 +++++++++++++++---- .../tests/websocket_config_property_tests.rs | 9 ++- .../tests/websocket_handshake_integration.rs | 18 +++-- 4 files changed, 83 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a03486a2..5b4d7be0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- **`connect` rejects a WebSocket configuration it cannot send faithfully, before dialing.** It fails with `MqttError::Configuration` when a custom header has an invalid name or value, repeats another custom header's name (case-insensitively), or names a header the transport sets itself (`Host`, `Connection`, `Upgrade`, `Sec-WebSocket-Version`, `Sec-WebSocket-Key`, `Sec-WebSocket-Extensions`); when `Sec-WebSocket-Protocol` or `User-Agent` is set as a custom header instead of through its setter; or when a subprotocol is not a valid HTTP token. +- **WebSocket configurations that relied on the old behavior may connect differently or fail to connect.** Because the configuration now reaches the wire: + - `with_subprotocol("mqttv5.0")` or a `with_subprotocols` list without `mqtt` now offers only what is listed. Previously `mqtt` was always offered instead, so a broker that accepts only `mqtt` will now refuse the upgrade. Include `mqtt` in the list (MQTT-6.0.0-3 requires it) to keep connecting. + - A custom header that is invalid or reserved (see the next entry) used to be silently dropped. It now makes `connect` fail. + - Every WebSocket connection, including those made by `MqttClient`, now sends a `User-Agent` header. +- **`connect` rejects a WebSocket configuration it cannot send faithfully, before dialing.** It fails with `MqttError::Configuration` in these cases: + - a custom header has an invalid name or value, or repeats another custom header's name (case-insensitively); + - a custom header names one reserved for the handshake: `Host`, `Connection`, `Upgrade`, `Sec-WebSocket-Version`, `Sec-WebSocket-Key`, `Sec-WebSocket-Extensions` or `Sec-WebSocket-Accept`; + - a custom header is `Content-Length` or `Transfer-Encoding`, which would describe a body the upgrade request does not have; + - `Sec-WebSocket-Protocol` or `User-Agent` is set as a custom header instead of through its setter; + - a subprotocol is not a valid HTTP token. +- Custom headers are sent, and validated, in name order, so the request and the error a bad configuration reports no longer depend on `HashMap` iteration order. - `WebSocketConfig`'s `Debug` output lists custom header names but not their values, which often carry credentials. ### Added diff --git a/crates/mqtt5/src/transport/websocket.rs b/crates/mqtt5/src/transport/websocket.rs index 664fe826..5632e31e 100644 --- a/crates/mqtt5/src/transport/websocket.rs +++ b/crates/mqtt5/src/transport/websocket.rs @@ -68,17 +68,23 @@ use url::Url; /// client to include "mqtt" in the subprotocols it offers. const DEFAULT_SUBPROTOCOL: &str = "mqtt"; -/// Handshake headers the transport sets itself. A custom header with one of -/// these names would duplicate or override the handshake, so it is rejected. +/// Headers a custom header may not use. The first five are set by the +/// transport, and a second copy would duplicate or override the handshake. /// `sec-websocket-extensions` is included because the transport negotiates no -/// extensions and cannot speak one a server accepts. -const RESERVED_HEADERS: [&str; 6] = [ +/// extensions and cannot speak one a server accepts, `sec-websocket-accept` +/// because it belongs in the server's response, and `content-length` and +/// `transfer-encoding` because the upgrade request has no body and a server +/// told otherwise would wait for one. +const RESERVED_HEADERS: [&str; 9] = [ "host", "connection", "upgrade", "sec-websocket-version", "sec-websocket-key", "sec-websocket-extensions", + "sec-websocket-accept", + "content-length", + "transfer-encoding", ]; /// WebSocket transport configuration @@ -177,11 +183,14 @@ impl WebSocketConfig { /// The name and value are validated when the request is built, so an /// invalid header makes [`connect`](WebSocketTransport::connect) fail /// rather than this call. Names are matched case-insensitively and must not - /// repeat. The handshake headers (`Host`, `Connection`, `Upgrade`, - /// `Sec-WebSocket-Version`, `Sec-WebSocket-Key`, `Sec-WebSocket-Extensions`) - /// are rejected, as are `Sec-WebSocket-Protocol` and `User-Agent`, which are - /// set with [`with_subprotocols`](Self::with_subprotocols) and - /// [`with_user_agent`](Self::with_user_agent). + /// repeat. Headers that belong to the handshake itself (`Host`, + /// `Connection`, `Upgrade`, `Sec-WebSocket-Version`, `Sec-WebSocket-Key`, + /// `Sec-WebSocket-Extensions`, `Sec-WebSocket-Accept`) or would describe a + /// request body (`Content-Length`, `Transfer-Encoding`) are rejected, as are + /// `Sec-WebSocket-Protocol` and `User-Agent`, which are set with + /// [`with_subprotocols`](Self::with_subprotocols) and + /// [`with_user_agent`](Self::with_user_agent). Custom headers are sent in + /// name order. #[must_use] pub fn with_header(mut self, name: &str, value: &str) -> Self { self.headers.insert(name.to_string(), value.to_string()); @@ -413,13 +422,17 @@ impl WebSocketConfig { headers.insert(USER_AGENT, header_value("User-Agent", user_agent)?); } - for (name, value) in &self.headers { + // Sorted so the request, and which error a bad configuration reports, + // do not depend on HashMap iteration order. + let mut custom: Vec<(&String, &String)> = self.headers.iter().collect(); + custom.sort_unstable(); + for (name, value) in custom { let header_name = HeaderName::from_bytes(name.as_bytes()).map_err(|e| { MqttError::Configuration(format!("Invalid WebSocket header name {name:?}: {e}")) })?; if RESERVED_HEADERS.contains(&header_name.as_str()) { return Err(MqttError::Configuration(format!( - "WebSocket header {name:?} is set by the transport and cannot be overridden" + "WebSocket header {name:?} is reserved for the handshake and cannot be set" ))); } if header_name == SEC_WEBSOCKET_PROTOCOL { @@ -1001,17 +1014,47 @@ mod tests { "Sec-WebSocket-Version", "Sec-WebSocket-Key", "Sec-WebSocket-Extensions", + "Sec-WebSocket-Accept", + "Content-Length", + "transfer-encoding", ] { let config = WebSocketConfig::new("ws://localhost/mqtt") .unwrap() .with_header(name, "value"); assert!( - handshake_error(&config).contains("set by the transport"), + handshake_error(&config).contains("reserved for the handshake"), "{name} should be rejected" ); } } + #[test] + fn test_handshake_request_header_order_is_deterministic() { + // Each config gets a freshly seeded HashMap, so any dependence on + // iteration order would show up across iterations. + for _ in 0..32 { + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_header("X-Charlie", "3") + .with_header("X-Alpha", "1") + .with_header("X-Bravo", "2"); + let request = config.build_handshake_request().unwrap(); + let custom: Vec<&str> = request + .headers() + .keys() + .map(HeaderName::as_str) + .filter(|name| name.starts_with("x-")) + .collect(); + assert_eq!(custom, ["x-alpha", "x-bravo", "x-charlie"]); + + let invalid = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_header("B-Header", "bad\r\nvalue") + .with_header("A Header", "value"); + assert!(handshake_error(&invalid).contains("Invalid WebSocket header name")); + } + } + #[test] fn test_handshake_request_rejects_headers_with_dedicated_setters() { let config = WebSocketConfig::new("ws://localhost/mqtt") diff --git a/crates/mqtt5/tests/websocket_config_property_tests.rs b/crates/mqtt5/tests/websocket_config_property_tests.rs index e8811fae..7f76f892 100644 --- a/crates/mqtt5/tests/websocket_config_property_tests.rs +++ b/crates/mqtt5/tests/websocket_config_property_tests.rs @@ -81,15 +81,18 @@ fn valid_http_headers() -> impl Strategy> { .prop_map(|btree| btree.into_iter().collect()) } -// Header names a custom header may not use (lowercase): the handshake headers -// the transport sets itself, plus the two that have dedicated setters. -const RESTRICTED_HEADERS: [&str; 8] = [ +// Header names a custom header may not use (lowercase): those reserved for +// the handshake, plus the two that have dedicated setters. +const RESTRICTED_HEADERS: [&str; 11] = [ "host", "connection", "upgrade", "sec-websocket-version", "sec-websocket-key", "sec-websocket-extensions", + "sec-websocket-accept", + "content-length", + "transfer-encoding", "sec-websocket-protocol", "user-agent", ]; diff --git a/crates/mqtt5/tests/websocket_handshake_integration.rs b/crates/mqtt5/tests/websocket_handshake_integration.rs index 8154c329..01d1e1b8 100644 --- a/crates/mqtt5/tests/websocket_handshake_integration.rs +++ b/crates/mqtt5/tests/websocket_handshake_integration.rs @@ -5,6 +5,7 @@ use mqtt5::transport::websocket::{WebSocketConfig, WebSocketTransport}; use mqtt5::Transport; +use std::net::SocketAddr; use std::sync::{Arc, Mutex}; use std::time::Duration; use tokio::net::TcpListener; @@ -41,8 +42,11 @@ impl Callback for CaptureRequest { } /// Connects a transport built by `configure` to a loopback server and returns -/// the headers of the upgrade request the server received. -async fn received_headers(configure: impl FnOnce(WebSocketConfig) -> WebSocketConfig) -> HeaderMap { +/// the headers of the upgrade request the server received, with the address +/// the server listened on. +async fn received_headers( + configure: impl FnOnce(WebSocketConfig) -> WebSocketConfig, +) -> (HeaderMap, SocketAddr) { let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); let addr = listener.local_addr().expect("addr"); let captured = Arc::new(Mutex::new(None)); @@ -69,7 +73,7 @@ async fn received_headers(configure: impl FnOnce(WebSocketConfig) -> WebSocketCo transport.close().await.expect("close"); let headers = captured.lock().expect("lock").take(); - headers.expect("server saw no upgrade request") + (headers.expect("server saw no upgrade request"), addr) } fn header<'a>(headers: &'a HeaderMap, name: &str) -> Option<&'a str> { @@ -78,7 +82,7 @@ fn header<'a>(headers: &'a HeaderMap, name: &str) -> Option<&'a str> { #[tokio::test] async fn upgrade_request_carries_configured_headers_subprotocols_and_user_agent() { - let headers = received_headers(|config| { + let (headers, _) = received_headers(|config| { config .with_header("Authorization", "Bearer token123") .with_header("X-Custom", "expected") @@ -98,14 +102,14 @@ async fn upgrade_request_carries_configured_headers_subprotocols_and_user_agent( #[tokio::test] async fn upgrade_request_defaults_offer_mqtt_and_crate_user_agent() { - let headers = received_headers(|config| config).await; + let (headers, addr) = received_headers(|config| config).await; assert_eq!(header(&headers, "Sec-WebSocket-Protocol"), Some("mqtt")); assert_eq!( header(&headers, "User-Agent"), Some(concat!("mqtt5/", env!("CARGO_PKG_VERSION"))) ); - assert!(header(&headers, "Host").is_some_and(|host| host.starts_with("127.0.0.1:"))); + assert_eq!(header(&headers, "Host"), Some(addr.to_string().as_str())); } #[tokio::test] @@ -120,7 +124,7 @@ async fn invalid_header_fails_connect_before_dialing() { .await .expect_err("reserved header accepted"); assert!( - err.to_string().contains("set by the transport"), + err.to_string().contains("reserved for the handshake"), "unexpected error: {err}" ); assert!(!transport.is_connected()); From f85f358f5d69323ef4b9a8bac7c61308b1487bd1 Mon Sep 17 00:00:00 2001 From: Bryan Ashby Date: Fri, 2 Oct 2026 10:46:11 -0600 Subject: [PATCH 3/9] use WebSocketConfig's TLS configuration for wss:// connections (#190) With server-certificate verification on, WebSocketTransport::connect used tokio-tungstenite's own client config built from the platform roots, so a configured CA, client certificate, use_system_roots and ALPN protocols were ignored. With verification off, the client certificate and ALPN were dropped. The rustls client config construction moves from TlsTransport to a public TlsConfig::client_config, and a wss:// connection with a tls_config now passes that config to tokio-tungstenite in both cases. Without a tls_config, the platform roots are still used. The WSS module's duplicate NoVerifier is replaced by the shared NoVerification. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 + crates/mqtt5/src/transport/tls.rs | 105 ++++--- crates/mqtt5/src/transport/websocket.rs | 107 ++----- .../mqtt5/tests/websocket_tls_integration.rs | 269 ++++++++++++++++++ 4 files changed, 360 insertions(+), 124 deletions(-) create mode 100644 crates/mqtt5/tests/websocket_tls_integration.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b4d7be0..f905750c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,11 +9,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`wss://` connections now use the TLS configuration on `WebSocketConfig`** (#190). With server-certificate verification on, `WebSocketTransport::connect` used tokio-tungstenite's own client configuration built from the platform's root certificates, so a CA set with `with_ca_cert_from_file` or `with_ca_cert_from_bytes` was never trusted (a server it issued failed with `UnknownIssuer`), a client certificate set with `with_client_auth_from_files` or `with_client_auth_from_bytes` was never presented, and `use_system_roots` and ALPN protocols had no effect. With verification off, the client certificate and ALPN protocols were dropped. The handshake now uses the same rustls configuration `TlsTransport` builds from a `TlsConfig`, in both cases. - **The WebSocket client transport now sends its configured headers, subprotocols and user agent** (#165). `WebSocketTransport::connect` built the upgrade request from a fixed set of headers and always offered `Sec-WebSocket-Protocol: mqtt`, so anything set with `WebSocketConfig::with_header`, `with_subprotocol`, `with_subprotocols` or `with_user_agent` never reached the server, and brokers that authenticate the upgrade request through custom headers rejected the connection. The request now carries every custom header, the configured subprotocols in order (or `mqtt` when none are configured), and the user agent when one is set. The default user agent is now `mqtt5/` instead of `mqtt-v5/0.4.0`. - **The upgrade request's `Host` header now includes a non-default port**, so `ws://broker:8080/mqtt` sends `Host: broker:8080` rather than `Host: broker`. ### Changed +- **A `wss://` connection with a TLS configuration now trusts the bundled `webpki-roots` instead of the platform's root certificates** when `use_system_roots` is set, as `TlsTransport` does. A server whose certificate chains only to a root installed on the platform, such as a corporate CA, now needs that CA added to the configuration. A `wss://` connection with no TLS configuration still uses the platform's roots. - **WebSocket configurations that relied on the old behavior may connect differently or fail to connect.** Because the configuration now reaches the wire: - `with_subprotocol("mqttv5.0")` or a `with_subprotocols` list without `mqtt` now offers only what is listed. Previously `mqtt` was always offered instead, so a broker that accepts only `mqtt` will now refuse the upgrade. Include `mqtt` in the list (MQTT-6.0.0-3 requires it) to keep connecting. - A custom header that is invalid or reserved (see the next entry) used to be silently dropped. It now makes `connect` fail. @@ -29,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `TlsConfig::client_config`, which returns the rustls client configuration a `TlsConfig` describes. - `WebSocketConfig::build_handshake_request`, which returns the upgrade request the configuration produces. ## [mqtt5 0.45.1] - 2026-10-01 diff --git a/crates/mqtt5/src/transport/tls.rs b/crates/mqtt5/src/transport/tls.rs index d818df58..94952d0c 100644 --- a/crates/mqtt5/src/transport/tls.rs +++ b/crates/mqtt5/src/transport/tls.rs @@ -307,58 +307,32 @@ impl TlsConfig { self.root_certs = Some(vec![ca_cert]); Ok(()) } -} - -/// TLS transport implementation -#[derive(Debug)] -pub struct TlsTransport { - config: TlsConfig, - stream: Option>, -} - -impl TlsTransport { - /// Creates a new TLS transport - #[must_use] - pub fn new(config: TlsConfig) -> Self { - Self { - config, - stream: None, - } - } - - /// Checks if the transport is connected - #[must_use] - pub fn is_connected(&self) -> bool { - self.stream.is_some() - } - /// Splits the TLS transport into read and write halves + /// Builds the rustls client configuration this TLS configuration describes + /// + /// The `addr` and `hostname` fields are not part of it: they select the + /// server to dial and the name to verify, which the caller supplies when + /// connecting. /// /// # Errors /// - /// Returns an error if the transport is not connected - pub fn into_split(self) -> Result<(TlsReadHalf, TlsWriteHalf)> { - match self.stream { - Some(stream) => Ok(tokio::io::split(stream)), - None => Err(MqttError::NotConnected), - } - } - - /// Builds the rustls client configuration + /// Returns an error if a root certificate cannot be added or the client + /// certificate and key cannot be used for client authentication /// - /// # Errors + /// # Panics /// - /// Returns an error if the operation fails - fn build_tls_config(&mut self) -> Result { + /// Panics if the stored client key cannot be re-parsed from its own DER + /// encoding, as [`TlsConfig`]'s `Clone` does. + pub fn client_config(&self) -> Result { let mut root_store = RootCertStore::empty(); // Add system roots if requested - if self.config.use_system_roots { + if self.use_system_roots { root_store.extend(webpki_roots::TLS_SERVER_ROOTS.to_vec()); } // Add custom root certificates - if let Some(ref root_certs) = self.config.root_certs { + if let Some(ref root_certs) = self.root_certs { for cert in root_certs { root_store.add(cert.clone()).map_err(|e| { MqttError::ProtocolError(format!("Failed to add root cert: {e}")) @@ -366,7 +340,7 @@ impl TlsTransport { } } - let config_builder = if self.config.verify_server_cert { + let config_builder = if self.verify_server_cert { ClientConfig::builder().with_root_certificates(root_store) } else { // Disable certificate verification for testing with self-signed certs @@ -376,8 +350,8 @@ impl TlsTransport { }; let mut config = if let (Some(cert), Some(key)) = ( - self.config.client_cert.clone(), - self.config.client_key.as_ref().map(|k| { + self.client_cert.clone(), + self.client_key.as_ref().map(|k| { PrivateKeyDer::try_from(k.secret_der().to_vec()) .expect("Failed to clone private key") }), @@ -392,7 +366,7 @@ impl TlsTransport { }; // Configure ALPN protocols if provided - if let Some(ref protocols) = self.config.alpn_protocols { + if let Some(ref protocols) = self.alpn_protocols { config.alpn_protocols.clone_from(protocols); } @@ -400,6 +374,51 @@ impl TlsTransport { } } +/// TLS transport implementation +#[derive(Debug)] +pub struct TlsTransport { + config: TlsConfig, + stream: Option>, +} + +impl TlsTransport { + /// Creates a new TLS transport + #[must_use] + pub fn new(config: TlsConfig) -> Self { + Self { + config, + stream: None, + } + } + + /// Checks if the transport is connected + #[must_use] + pub fn is_connected(&self) -> bool { + self.stream.is_some() + } + + /// Splits the TLS transport into read and write halves + /// + /// # Errors + /// + /// Returns an error if the transport is not connected + pub fn into_split(self) -> Result<(TlsReadHalf, TlsWriteHalf)> { + match self.stream { + Some(stream) => Ok(tokio::io::split(stream)), + None => Err(MqttError::NotConnected), + } + } + + /// Builds the rustls client configuration + /// + /// # Errors + /// + /// Returns an error if the operation fails + fn build_tls_config(&self) -> Result { + self.config.client_config() + } +} + impl Transport for TlsTransport { #[instrument(skip(self), fields(addr = %self.config.addr, server_name = %self.config.hostname, verify_certs = self.config.verify_server_cert))] async fn connect(&mut self) -> Result<()> { diff --git a/crates/mqtt5/src/transport/websocket.rs b/crates/mqtt5/src/transport/websocket.rs index 5632e31e..095ba5d0 100644 --- a/crates/mqtt5/src/transport/websocket.rs +++ b/crates/mqtt5/src/transport/websocket.rs @@ -49,6 +49,7 @@ use bytes::{Buf, Bytes, BytesMut}; use futures_util::{stream::SplitSink, stream::SplitStream, StreamExt}; use std::collections::HashMap; use std::net::SocketAddr; +use std::sync::Arc; use tokio::net::TcpStream; use tokio_tungstenite::{ tungstenite::{ @@ -59,7 +60,7 @@ use tokio_tungstenite::{ }, protocol::Message, }, - MaybeTlsStream, WebSocketStream, + Connector, MaybeTlsStream, WebSocketStream, }; use tracing::{debug, error, info, instrument}; use url::Url; @@ -205,6 +206,17 @@ impl WebSocketConfig { } /// Sets a custom TLS configuration for wss:// connections + /// + /// Its root certificates, system-roots setting, server-certificate + /// verification, client certificate and ALPN protocols are used for the + /// TLS handshake; with `use_system_roots`, the system roots are the + /// bundled `webpki-roots`, as for [`TlsTransport`](crate::transport::tls::TlsTransport). + /// Its `addr`, `hostname` and `connect_timeout` are not used: the server + /// and the name verified against its certificate come from the WebSocket + /// URL, and the timeout from [`with_timeout`](Self::with_timeout). + /// + /// Without a TLS configuration, a wss:// connection verifies the server + /// against the platform's native root certificates. #[must_use] pub fn with_tls_config(mut self, tls_config: TlsConfig) -> Self { self.tls_config = Some(tls_config); @@ -658,40 +670,21 @@ impl Transport for WebSocketTransport { let request = self.config.build_handshake_request()?; - let ws_result = if self.config.is_secure() - && self - .config - .tls_config - .as_ref() - .is_some_and(|cfg| !cfg.verify_server_cert) - { - use tokio_tungstenite::Connector; - - let tls = rustls::ClientConfig::builder() - .dangerous() - .with_custom_certificate_verifier(std::sync::Arc::new(NoVerifier)) - .with_no_client_auth(); - - let connector = Connector::Rustls(std::sync::Arc::new(tls)); - - tokio::time::timeout( - self.config.timeout, - tokio_tungstenite::connect_async_tls_with_config( - request, - None, - false, - Some(connector), - ), - ) - .await - } else { - tokio::time::timeout( - self.config.timeout, - tokio_tungstenite::connect_async(request), - ) - .await + // With no TLS configuration, tokio-tungstenite builds its own client + // config from the platform's native root certificates. + let connector = match self.config.tls_config.as_ref() { + Some(tls_config) if self.config.is_secure() => { + Some(Connector::Rustls(Arc::new(tls_config.client_config()?))) + } + _ => None, }; + let ws_result = tokio::time::timeout( + self.config.timeout, + tokio_tungstenite::connect_async_tls_with_config(request, None, false, connector), + ) + .await; + match ws_result { Ok(Ok((ws_stream, response))) => { if let Some(protocol) = response.headers().get("Sec-WebSocket-Protocol") { @@ -804,54 +797,6 @@ impl Transport for WebSocketTransport { } } -#[derive(Debug)] -struct NoVerifier; - -impl rustls::client::danger::ServerCertVerifier for NoVerifier { - fn verify_server_cert( - &self, - _end_entity: &rustls::pki_types::CertificateDer<'_>, - _intermediates: &[rustls::pki_types::CertificateDer<'_>], - _server_name: &rustls::pki_types::ServerName<'_>, - _ocsp_response: &[u8], - _now: rustls::pki_types::UnixTime, - ) -> std::result::Result { - Ok(rustls::client::danger::ServerCertVerified::assertion()) - } - - fn verify_tls12_signature( - &self, - _message: &[u8], - _cert: &rustls::pki_types::CertificateDer<'_>, - _dss: &rustls::DigitallySignedStruct, - ) -> std::result::Result { - Ok(rustls::client::danger::HandshakeSignatureValid::assertion()) - } - - fn verify_tls13_signature( - &self, - _message: &[u8], - _cert: &rustls::pki_types::CertificateDer<'_>, - _dss: &rustls::DigitallySignedStruct, - ) -> std::result::Result { - Ok(rustls::client::danger::HandshakeSignatureValid::assertion()) - } - - fn supported_verify_schemes(&self) -> Vec { - vec![ - rustls::SignatureScheme::RSA_PKCS1_SHA256, - rustls::SignatureScheme::RSA_PKCS1_SHA384, - rustls::SignatureScheme::RSA_PKCS1_SHA512, - rustls::SignatureScheme::ECDSA_NISTP256_SHA256, - rustls::SignatureScheme::ECDSA_NISTP384_SHA384, - rustls::SignatureScheme::RSA_PSS_SHA256, - rustls::SignatureScheme::RSA_PSS_SHA384, - rustls::SignatureScheme::RSA_PSS_SHA512, - rustls::SignatureScheme::ED25519, - ] - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/mqtt5/tests/websocket_tls_integration.rs b/crates/mqtt5/tests/websocket_tls_integration.rs new file mode 100644 index 00000000..69d12046 --- /dev/null +++ b/crates/mqtt5/tests/websocket_tls_integration.rs @@ -0,0 +1,269 @@ +#![cfg(feature = "transport-websocket")] + +//! Checks that a `wss://` connection uses `WebSocketConfig::tls_config`: its +//! root certificates, client certificate and ALPN protocols, as seen by a +//! TLS server, rather than only being stored on the configuration. +//! +//! Requires the certificates from `scripts/generate_test_certs.sh`. + +use mqtt5::transport::tls::TlsConfig; +use mqtt5::transport::websocket::{WebSocketConfig, WebSocketTransport}; +use mqtt5::Transport; +use rustls::pki_types::pem::PemObject; +use rustls::pki_types::{CertificateDer, PrivateKeyDer}; +use rustls::server::WebPkiClientVerifier; +use rustls::{RootCertStore, ServerConfig}; +use std::net::SocketAddr; +use std::sync::Arc; +use std::time::Duration; +use tokio::net::TcpListener; +use tokio::sync::oneshot; +use tokio::time::timeout; +use tokio_rustls::TlsAcceptor; +use tokio_tungstenite::tungstenite::handshake::server::{ + Callback, ErrorResponse, Request, Response, +}; +use tokio_tungstenite::tungstenite::http::HeaderValue; + +const WAIT: Duration = Duration::from_secs(5); +const CA: &str = "../../test_certs/ca.pem"; +const SERVER_CERT: &str = "../../test_certs/server.pem"; +const SERVER_KEY: &str = "../../test_certs/server.key"; +const CLIENT_CERT: &str = "../../test_certs/client.pem"; +const CLIENT_KEY: &str = "../../test_certs/client.key"; + +/// What the server observed about a TLS handshake that completed. +#[derive(Debug)] +struct Handshake { + alpn: Option>, + client_presented_cert: bool, +} + +fn certs(path: &str) -> Vec> { + CertificateDer::pem_file_iter(path) + .unwrap_or_else(|e| panic!("read {path}: {e}")) + .collect::>() + .unwrap_or_else(|e| panic!("parse {path}: {e}")) +} + +fn key(path: &str) -> PrivateKeyDer<'static> { + PrivateKeyDer::from_pem_file(path).unwrap_or_else(|e| panic!("read {path}: {e}")) +} + +fn server_config( + cert: Vec>, + key: PrivateKeyDer<'static>, + require_client_cert: bool, + alpn: &[&[u8]], +) -> ServerConfig { + let _ = rustls::crypto::ring::default_provider().install_default(); + let builder = ServerConfig::builder(); + let mut config = if require_client_cert { + let mut roots = RootCertStore::empty(); + for ca in certs(CA) { + roots.add(ca).expect("add CA"); + } + let verifier = WebPkiClientVerifier::builder(Arc::new(roots)) + .build() + .expect("client verifier"); + builder.with_client_cert_verifier(verifier) + } else { + builder.with_no_client_auth() + } + .with_single_cert(cert, key) + .expect("server cert"); + config.alpn_protocols = alpn.iter().map(|p| p.to_vec()).collect(); + config +} + +/// Starts a one-connection WSS server. The receiver yields the handshake +/// details if the TLS and WebSocket handshakes both complete. +async fn wss_server(config: ServerConfig) -> (SocketAddr, oneshot::Receiver) { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let addr = listener.local_addr().expect("addr"); + let acceptor = TlsAcceptor::from(Arc::new(config)); + let (tx, rx) = oneshot::channel(); + tokio::spawn(async move { + let Ok((stream, _)) = listener.accept().await else { + return; + }; + let Ok(tls) = acceptor.accept(stream).await else { + return; + }; + let (_, session) = tls.get_ref(); + let handshake = Handshake { + alpn: session.alpn_protocol().map(<[u8]>::to_vec), + client_presented_cert: session.peer_certificates().is_some(), + }; + let Ok(ws) = tokio_tungstenite::accept_hdr_async(tls, SelectMqtt).await else { + return; + }; + let _ = tx.send(handshake); + // Hold the connection open until the client closes it. + let _ws = ws; + tokio::time::sleep(WAIT).await; + }); + (addr, rx) +} + +/// Selects the "mqtt" subprotocol the client offers by default, as a broker +/// would; the client fails the handshake if the server selects none. +struct SelectMqtt; + +impl Callback for SelectMqtt { + fn on_request(self, _req: &Request, mut resp: Response) -> Result { + resp.headers_mut() + .insert("Sec-WebSocket-Protocol", HeaderValue::from_static("mqtt")); + Ok(resp) + } +} + +fn wss_config(addr: SocketAddr) -> WebSocketConfig { + WebSocketConfig::new(&format!("wss://{addr}/mqtt")).expect("config") +} + +/// Connects and returns the server's view of the handshake, or the client's +/// error if the connection failed. +async fn connect( + config: WebSocketConfig, + server: oneshot::Receiver, +) -> Result { + let mut transport = WebSocketTransport::new(config); + let result = timeout(WAIT, transport.connect()) + .await + .expect("connect timed out"); + match result { + Ok(()) => { + let handshake = timeout(WAIT, server) + .await + .expect("server timed out") + .expect("server saw no completed handshake"); + transport.close().await.expect("close"); + Ok(handshake) + } + Err(e) => Err(e.to_string()), + } +} + +#[tokio::test] +async fn custom_ca_from_tls_config_is_trusted() { + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + false, + &[], + )) + .await; + let config = wss_config(addr) + .with_ca_cert_from_file(CA) + .expect("load CA"); + + connect(config, server) + .await + .expect("server certificate issued by the configured CA was rejected"); +} + +#[tokio::test] +async fn certificate_from_another_ca_is_rejected() { + let other = rcgen::generate_simple_self_signed(vec!["127.0.0.1".to_string()]) + .expect("generate certificate"); + let (addr, server) = wss_server(server_config( + vec![other.cert.der().clone()], + PrivateKeyDer::try_from(other.signing_key.serialize_der()).expect("key"), + false, + &[], + )) + .await; + let config = wss_config(addr) + .with_ca_cert_from_file(CA) + .expect("load CA"); + + let err = connect(config, server) + .await + .expect_err("certificate not issued by the configured CA was accepted"); + assert!( + err.contains("certificate") || err.contains("Certificate"), + "unexpected error: {err}" + ); +} + +#[tokio::test] +async fn client_certificate_from_tls_config_is_presented() { + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + true, + &[], + )) + .await; + let config = wss_config(addr) + .with_client_auth_from_files(CLIENT_CERT, CLIENT_KEY) + .expect("load client auth") + .with_ca_cert_from_file(CA) + .expect("load CA"); + + let handshake = connect(config, server) + .await + .expect("server requiring a client certificate rejected the connection"); + assert!(handshake.client_presented_cert); +} + +#[tokio::test] +async fn client_certificate_is_presented_with_verification_disabled() { + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + true, + &[], + )) + .await; + let mut tls_config = TlsConfig::new(addr, "127.0.0.1").with_verify_server_cert(false); + tls_config + .load_client_cert_pem(CLIENT_CERT) + .expect("load client cert"); + tls_config + .load_client_key_pem(CLIENT_KEY) + .expect("load client key"); + let config = wss_config(addr).with_tls_config(tls_config); + + let handshake = connect(config, server) + .await + .expect("server requiring a client certificate rejected the connection"); + assert!(handshake.client_presented_cert); +} + +#[tokio::test] +async fn alpn_protocols_from_tls_config_are_offered() { + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + false, + &[b"mqtt-test", b"http/1.1"], + )) + .await; + let mut tls_config = TlsConfig::new(addr, "127.0.0.1").with_alpn_protocols(&["mqtt-test"]); + tls_config.load_ca_cert_pem(CA).expect("load CA"); + let config = wss_config(addr).with_tls_config(tls_config); + + let handshake = connect(config, server).await.expect("connect"); + assert_eq!(handshake.alpn.as_deref(), Some(&b"mqtt-test"[..])); +} + +#[tokio::test] +async fn without_tls_config_private_ca_is_not_trusted() { + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + false, + &[], + )) + .await; + + let err = connect(wss_config(addr), server) + .await + .expect_err("certificate from a private CA was accepted with no TLS configuration"); + assert!( + err.contains("certificate") || err.contains("Certificate"), + "unexpected error: {err}" + ); +} From ec7a2df82c8e197bb3241d742c09b9d87449805c Mon Sep 17 00:00:00 2001 From: Bryan Ashby Date: Fri, 2 Oct 2026 10:55:38 -0600 Subject: [PATCH 4/9] accept host name URLs in with_tls_auto and use MqttClient's stored TLS config over wss (#190) with_tls_auto parsed the URL's host and port as a socket address, so it and the with_ca_cert_* and with_client_auth_* methods that call it failed for any host name. A wss:// connection takes its server and certificate name from the URL, so a host name now gets the unspecified address with the URL's port. MqttClient's wss:// path built its WebSocketConfig from the URL alone and ignored the TLS configuration stored by set_tls_config; it now applies it as connect_tls does. New tests cover a host name URL, the URL host being verified rather than TlsConfig::hostname, and MqttClient presenting its stored client certificate, and two rejection tests now assert UnknownIssuer. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 +- crates/mqtt5/src/client/inner.rs | 17 +++- crates/mqtt5/src/transport/websocket.rs | 13 +-- .../mqtt5/tests/websocket_tls_integration.rs | 80 +++++++++++++++++-- 4 files changed, 98 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f905750c..22e45811 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,12 +10,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **`wss://` connections now use the TLS configuration on `WebSocketConfig`** (#190). With server-certificate verification on, `WebSocketTransport::connect` used tokio-tungstenite's own client configuration built from the platform's root certificates, so a CA set with `with_ca_cert_from_file` or `with_ca_cert_from_bytes` was never trusted (a server it issued failed with `UnknownIssuer`), a client certificate set with `with_client_auth_from_files` or `with_client_auth_from_bytes` was never presented, and `use_system_roots` and ALPN protocols had no effect. With verification off, the client certificate and ALPN protocols were dropped. The handshake now uses the same rustls configuration `TlsTransport` builds from a `TlsConfig`, in both cases. +- **`WebSocketConfig::with_tls_auto`, and the `with_ca_cert_*` and `with_client_auth_*` methods that call it, now accept a host name URL** (#190). They parsed the URL's host and port as a socket address, so they failed with "Invalid host/port combination" for anything but an IP address. A wss:// connection takes its server and certificate name from the URL and does not use `TlsConfig::addr`, so for a host name that is now the unspecified address with the URL's port. +- **`MqttClient` now uses its stored TLS configuration for `wss://` connections** (#190), as it already did for `mqtts://`. A CA, client certificate or key set with `set_tls_config` was ignored over WebSocket, and `set_insecure_tls(true)` replaced the stored configuration with a bare one that disables verification. - **The WebSocket client transport now sends its configured headers, subprotocols and user agent** (#165). `WebSocketTransport::connect` built the upgrade request from a fixed set of headers and always offered `Sec-WebSocket-Protocol: mqtt`, so anything set with `WebSocketConfig::with_header`, `with_subprotocol`, `with_subprotocols` or `with_user_agent` never reached the server, and brokers that authenticate the upgrade request through custom headers rejected the connection. The request now carries every custom header, the configured subprotocols in order (or `mqtt` when none are configured), and the user agent when one is set. The default user agent is now `mqtt5/` instead of `mqtt-v5/0.4.0`. - **The upgrade request's `Host` header now includes a non-default port**, so `ws://broker:8080/mqtt` sends `Host: broker:8080` rather than `Host: broker`. ### Changed -- **A `wss://` connection with a TLS configuration now trusts the bundled `webpki-roots` instead of the platform's root certificates** when `use_system_roots` is set, as `TlsTransport` does. A server whose certificate chains only to a root installed on the platform, such as a corporate CA, now needs that CA added to the configuration. A `wss://` connection with no TLS configuration still uses the platform's roots. +- **A `wss://` connection with a TLS configuration now trusts the bundled `webpki-roots` instead of the platform's root certificates** when `use_system_roots` is set, as `TlsTransport` does. A server whose certificate chains only to a root installed on the platform, such as a corporate CA, now needs that CA added to the configuration. A `wss://` connection with no TLS configuration still uses the platform's roots. The new behavior also applies to `MqttClient` `wss://` connections once a TLS configuration is set with `set_tls_config`. - **WebSocket configurations that relied on the old behavior may connect differently or fail to connect.** Because the configuration now reaches the wire: - `with_subprotocol("mqttv5.0")` or a `with_subprotocols` list without `mqtt` now offers only what is listed. Previously `mqtt` was always offered instead, so a broker that accepts only `mqtt` will now refuse the upgrade. Include `mqtt` in the list (MQTT-6.0.0-3 requires it) to keep connecting. - A custom header that is invalid or reserved (see the next entry) used to be silently dropped. It now makes `connect` fail. diff --git a/crates/mqtt5/src/client/inner.rs b/crates/mqtt5/src/client/inner.rs index 1013ecfc..7532ea13 100644 --- a/crates/mqtt5/src/client/inner.rs +++ b/crates/mqtt5/src/client/inner.rs @@ -282,8 +282,21 @@ impl MqttClient { let mut config = WebSocketConfig::new(url) .map_err(|e| MqttError::ConnectionError(format!("Invalid WebSocket URL: {e}")))?; - if insecure { - let tls_config = TlsConfig::new(addr, host).with_verify_server_cert(false); + // As in connect_tls: a stored TLS config applies to wss:// too. With + // none, an insecure connection still needs one to disable + // verification; a secure one keeps the WebSocket transport's default. + let stored = self.tls_config.read().await.clone(); + let tls_config = match stored { + Some(mut cfg) => { + cfg.addr = addr; + cfg.hostname = host.to_string(); + cfg.verify_server_cert = !insecure; + Some(cfg) + } + None if insecure => Some(TlsConfig::new(addr, host).with_verify_server_cert(false)), + None => None, + }; + if let Some(tls_config) = tls_config { config = config.with_tls_config(tls_config); } diff --git a/crates/mqtt5/src/transport/websocket.rs b/crates/mqtt5/src/transport/websocket.rs index 095ba5d0..b4e6d421 100644 --- a/crates/mqtt5/src/transport/websocket.rs +++ b/crates/mqtt5/src/transport/websocket.rs @@ -226,14 +226,16 @@ impl WebSocketConfig { /// Creates a TLS configuration automatically from the WebSocket URL /// /// This is a convenience method that creates a TLS config with the same - /// host and port as the WebSocket URL. + /// host and port as the WebSocket URL. The host may be a name or an IP + /// address. A wss:// connection does not use the config's `addr`, so for a + /// host name it is the unspecified address with the URL's port, rather + /// than a resolved one. /// /// # Errors /// /// Returns an error if: /// - The URL is not a secure WebSocket (wss://) /// - The URL does not have a valid host - /// - The host/port combination cannot be parsed as a socket address pub fn with_tls_auto(mut self) -> Result { if !self.is_secure() { return Err(MqttError::ProtocolError( @@ -245,9 +247,10 @@ impl WebSocketConfig { MqttError::ProtocolError("WebSocket URL must have a host".to_string()) })?; - let addr: SocketAddr = format!("{host}:{}", self.port()) - .parse() - .map_err(|e| MqttError::ProtocolError(format!("Invalid host/port combination: {e}")))?; + let port = self.port(); + let addr = format!("{host}:{port}").parse().unwrap_or_else(|_| { + SocketAddr::new(std::net::IpAddr::V4(std::net::Ipv4Addr::UNSPECIFIED), port) + }); let tls_config = TlsConfig::new(addr, host); self.tls_config = Some(tls_config); diff --git a/crates/mqtt5/tests/websocket_tls_integration.rs b/crates/mqtt5/tests/websocket_tls_integration.rs index 69d12046..d2db4330 100644 --- a/crates/mqtt5/tests/websocket_tls_integration.rs +++ b/crates/mqtt5/tests/websocket_tls_integration.rs @@ -8,7 +8,7 @@ use mqtt5::transport::tls::TlsConfig; use mqtt5::transport::websocket::{WebSocketConfig, WebSocketTransport}; -use mqtt5::Transport; +use mqtt5::{ConnectOptions, MqttClient, Transport}; use rustls::pki_types::pem::PemObject; use rustls::pki_types::{CertificateDer, PrivateKeyDer}; use rustls::server::WebPkiClientVerifier; @@ -181,10 +181,7 @@ async fn certificate_from_another_ca_is_rejected() { let err = connect(config, server) .await .expect_err("certificate not issued by the configured CA was accepted"); - assert!( - err.contains("certificate") || err.contains("Certificate"), - "unexpected error: {err}" - ); + assert!(err.contains("UnknownIssuer"), "unexpected error: {err}"); } #[tokio::test] @@ -262,8 +259,75 @@ async fn without_tls_config_private_ca_is_not_trusted() { let err = connect(wss_config(addr), server) .await .expect_err("certificate from a private CA was accepted with no TLS configuration"); - assert!( - err.contains("certificate") || err.contains("Certificate"), - "unexpected error: {err}" + assert!(err.contains("UnknownIssuer"), "unexpected error: {err}"); +} + +#[tokio::test] +async fn custom_ca_works_with_a_host_name_url() { + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + false, + &[], + )) + .await; + let config = WebSocketConfig::new(&format!("wss://localhost:{}/mqtt", addr.port())) + .expect("config") + .with_ca_cert_from_file(CA) + .expect("a host name URL should accept a CA"); + + connect(config, server) + .await + .expect("server certificate for localhost, issued by the configured CA, was rejected"); +} + +#[tokio::test] +async fn certificate_is_verified_against_the_url_host_not_tls_config_hostname() { + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + false, + &[], + )) + .await; + let mut tls_config = TlsConfig::new(addr, "wrong.example"); + tls_config.load_ca_cert_pem(CA).expect("load CA"); + let config = wss_config(addr).with_tls_config(tls_config); + + connect(config, server) + .await + .expect("TlsConfig::hostname was used instead of the URL host"); +} + +#[tokio::test] +async fn mqtt_client_presents_its_stored_tls_config_over_wss() { + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + true, + &[], + )) + .await; + let client = MqttClient::with_options( + ConnectOptions::new("wss-stored-tls").with_automatic_reconnect(false), ); + client + .set_tls_config( + Some(std::fs::read(CLIENT_CERT).expect("read client cert")), + Some(std::fs::read(CLIENT_KEY).expect("read client key")), + Some(std::fs::read(CA).expect("read CA")), + ) + .await; + + // The test server stops after the WebSocket handshake and never answers + // CONNECT, so only the TLS handshake it observed is checked. + let connecting = tokio::spawn(async move { + let _ = client.connect(&format!("wss://{addr}/mqtt")).await; + }); + let handshake = timeout(WAIT, server) + .await + .expect("server timed out") + .expect("server saw no completed handshake: the stored TLS config was not used"); + assert!(handshake.client_presented_cert); + connecting.abort(); } From 0f6b48faa164aa2e3a12bd5d249b6d902aac7446 Mon Sep 17 00:00:00 2001 From: Bryan Ashby Date: Thu, 8 Oct 2026 16:11:30 -0600 Subject: [PATCH 5/9] always offer mqtt, reject duplicate subprotocols, reserve hop-by-hop headers (#165) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. The configured subprotocols are now followed by "mqtt" when the list does not include it, as MQTT-6.0.0-3 requires: the broker in this crate only selects a subprotocol the client offers, so a config preferring mqttv5.0 failed with "Server sent no subprotocol". A subprotocol listed twice is rejected (RFC 6455 §4.1), and TE, Trailer, Keep-Alive, Proxy-Connection and Expect join the reserved headers. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 +- crates/mqtt5/src/transport/websocket.rs | 104 ++++++++++++++---- .../tests/websocket_config_property_tests.rs | 29 +++-- .../tests/websocket_handshake_integration.rs | 42 +++++++ 4 files changed, 147 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b4d7be0..66266a66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,21 +9,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **The WebSocket client transport now sends its configured headers, subprotocols and user agent** (#165). `WebSocketTransport::connect` built the upgrade request from a fixed set of headers and always offered `Sec-WebSocket-Protocol: mqtt`, so anything set with `WebSocketConfig::with_header`, `with_subprotocol`, `with_subprotocols` or `with_user_agent` never reached the server, and brokers that authenticate the upgrade request through custom headers rejected the connection. The request now carries every custom header, the configured subprotocols in order (or `mqtt` when none are configured), and the user agent when one is set. The default user agent is now `mqtt5/` instead of `mqtt-v5/0.4.0`. +- **The WebSocket client transport now sends its configured headers, subprotocols and user agent** (#165). `WebSocketTransport::connect` built the upgrade request from a fixed set of headers and always offered `Sec-WebSocket-Protocol: mqtt`, so anything set with `WebSocketConfig::with_header`, `with_subprotocol`, `with_subprotocols` or `with_user_agent` never reached the server, and brokers that authenticate the upgrade request through custom headers rejected the connection. The request now carries every custom header, the configured subprotocols in order, followed by `mqtt` when the list does not include it (MQTT-6.0.0-3), and the user agent when one is set. The default user agent is now `mqtt5/` instead of `mqtt-v5/0.4.0`. - **The upgrade request's `Host` header now includes a non-default port**, so `ws://broker:8080/mqtt` sends `Host: broker:8080` rather than `Host: broker`. ### Changed - **WebSocket configurations that relied on the old behavior may connect differently or fail to connect.** Because the configuration now reaches the wire: - - `with_subprotocol("mqttv5.0")` or a `with_subprotocols` list without `mqtt` now offers only what is listed. Previously `mqtt` was always offered instead, so a broker that accepts only `mqtt` will now refuse the upgrade. Include `mqtt` in the list (MQTT-6.0.0-3 requires it) to keep connecting. - A custom header that is invalid or reserved (see the next entry) used to be silently dropped. It now makes `connect` fail. - Every WebSocket connection, including those made by `MqttClient`, now sends a `User-Agent` header. - **`connect` rejects a WebSocket configuration it cannot send faithfully, before dialing.** It fails with `MqttError::Configuration` in these cases: - a custom header has an invalid name or value, or repeats another custom header's name (case-insensitively); - a custom header names one reserved for the handshake: `Host`, `Connection`, `Upgrade`, `Sec-WebSocket-Version`, `Sec-WebSocket-Key`, `Sec-WebSocket-Extensions` or `Sec-WebSocket-Accept`; - - a custom header is `Content-Length` or `Transfer-Encoding`, which would describe a body the upgrade request does not have; + - a custom header is `Content-Length`, `Transfer-Encoding` or `Trailer`, which would describe a body the upgrade request does not have, or a hop-by-hop or expectation header: `TE`, `Keep-Alive`, `Proxy-Connection` or `Expect`; - `Sec-WebSocket-Protocol` or `User-Agent` is set as a custom header instead of through its setter; - - a subprotocol is not a valid HTTP token. + - a subprotocol is not a valid HTTP token, or is listed more than once (RFC 6455 §4.1). - Custom headers are sent, and validated, in name order, so the request and the error a bad configuration reports no longer depend on `HashMap` iteration order. - `WebSocketConfig`'s `Debug` output lists custom header names but not their values, which often carry credentials. diff --git a/crates/mqtt5/src/transport/websocket.rs b/crates/mqtt5/src/transport/websocket.rs index 5632e31e..7bb7fa78 100644 --- a/crates/mqtt5/src/transport/websocket.rs +++ b/crates/mqtt5/src/transport/websocket.rs @@ -64,8 +64,9 @@ use tokio_tungstenite::{ use tracing::{debug, error, info, instrument}; use url::Url; -/// Subprotocol offered when none is configured. MQTT-6.0.0-3 requires the -/// client to include "mqtt" in the subprotocols it offers. +/// Subprotocol always offered. MQTT-6.0.0-3 requires the client to include +/// "mqtt" in the subprotocols it offers, so it is appended when the configured +/// list does not already contain it. const DEFAULT_SUBPROTOCOL: &str = "mqtt"; /// Headers a custom header may not use. The first five are set by the @@ -74,8 +75,10 @@ const DEFAULT_SUBPROTOCOL: &str = "mqtt"; /// extensions and cannot speak one a server accepts, `sec-websocket-accept` /// because it belongs in the server's response, and `content-length` and /// `transfer-encoding` because the upgrade request has no body and a server -/// told otherwise would wait for one. -const RESERVED_HEADERS: [&str; 9] = [ +/// told otherwise would wait for one. The rest are hop-by-hop or +/// expectation headers (`te`, `trailer`, `keep-alive`, `proxy-connection`, +/// `expect`) that describe the connection or a body rather than the upgrade. +const RESERVED_HEADERS: [&str; 14] = [ "host", "connection", "upgrade", @@ -85,6 +88,11 @@ const RESERVED_HEADERS: [&str; 9] = [ "sec-websocket-accept", "content-length", "transfer-encoding", + "te", + "trailer", + "keep-alive", + "proxy-connection", + "expect", ]; /// WebSocket transport configuration @@ -94,7 +102,7 @@ pub struct WebSocketConfig { /// Connection timeout pub timeout: Duration, /// Subprotocols to offer, in order of preference (e.g., "mqtt", "mqttv5.0"). - /// "mqtt" is offered when this is empty. + /// "mqtt" is appended when it is not in the list. pub subprotocols: Vec, /// Custom HTTP headers for the WebSocket handshake pub headers: HashMap, @@ -157,8 +165,11 @@ impl WebSocketConfig { /// Sets the WebSocket subprotocols to offer, in order of preference /// - /// The list is sent exactly as given. MQTT-6.0.0-3 requires the client to - /// offer "mqtt", so keep it in the list unless the broker requires otherwise. + /// "mqtt" is appended to the offer when it is not in the list, as + /// MQTT-6.0.0-3 requires; the server selects one value, so a broker that + /// wants another listed subprotocol can still choose it. Each subprotocol + /// must be an HTTP token and appear only once (RFC 6455 §4.1), or + /// [`connect`](WebSocketTransport::connect) fails. #[must_use] pub fn with_subprotocols(mut self, subprotocols: &[&str]) -> Self { self.subprotocols = subprotocols @@ -168,10 +179,10 @@ impl WebSocketConfig { self } - /// Sets a single WebSocket subprotocol to offer + /// Sets a single WebSocket subprotocol to offer, in preference to "mqtt" /// - /// MQTT-6.0.0-3 requires the client to offer "mqtt"; see - /// [`with_subprotocols`](Self::with_subprotocols) to offer it alongside another. + /// "mqtt" is still offered after it, as MQTT-6.0.0-3 requires; see + /// [`with_subprotocols`](Self::with_subprotocols). #[must_use] pub fn with_subprotocol(mut self, subprotocol: &str) -> Self { self.subprotocols = vec![subprotocol.to_string()]; @@ -185,8 +196,10 @@ impl WebSocketConfig { /// rather than this call. Names are matched case-insensitively and must not /// repeat. Headers that belong to the handshake itself (`Host`, /// `Connection`, `Upgrade`, `Sec-WebSocket-Version`, `Sec-WebSocket-Key`, - /// `Sec-WebSocket-Extensions`, `Sec-WebSocket-Accept`) or would describe a - /// request body (`Content-Length`, `Transfer-Encoding`) are rejected, as are + /// `Sec-WebSocket-Extensions`, `Sec-WebSocket-Accept`), would describe a + /// request body (`Content-Length`, `Transfer-Encoding`, `Trailer`), or are + /// hop-by-hop or expectation headers (`TE`, `Keep-Alive`, + /// `Proxy-Connection`, `Expect`) are rejected, as are /// `Sec-WebSocket-Protocol` and `User-Agent`, which are set with /// [`with_subprotocols`](Self::with_subprotocols) and /// [`with_user_agent`](Self::with_user_agent). Custom headers are sent in @@ -386,14 +399,14 @@ impl WebSocketConfig { /// Builds the WebSocket handshake request this configuration produces /// - /// The request carries the configured subprotocols (or "mqtt" when none - /// are configured), the user agent when one is set, and every custom + /// The request carries the configured subprotocols followed by "mqtt" when + /// they do not include it, the user agent when one is set, and every custom /// header. Each call generates a fresh `Sec-WebSocket-Key`. /// /// # Errors /// /// Returns an error if: - /// - A subprotocol is not a valid HTTP token + /// - A subprotocol is not a valid HTTP token or is listed more than once /// - The user agent is not a valid header value /// - A custom header has an invalid name or value, names a header the /// transport sets itself, or repeats another custom header's name @@ -403,16 +416,24 @@ impl WebSocketConfig { })?; let headers = request.headers_mut(); - let subprotocols = if self.subprotocols.is_empty() { - DEFAULT_SUBPROTOCOL.to_string() - } else { - if let Some(invalid) = self.subprotocols.iter().find(|p| !is_http_token(p)) { + let mut offered: Vec<&str> = Vec::with_capacity(self.subprotocols.len() + 1); + for subprotocol in &self.subprotocols { + if !is_http_token(subprotocol) { return Err(MqttError::Configuration(format!( - "Invalid WebSocket subprotocol {invalid:?}: must be a non-empty HTTP token" + "Invalid WebSocket subprotocol {subprotocol:?}: must be a non-empty HTTP token" ))); } - self.subprotocols.join(", ") - }; + if offered.contains(&subprotocol.as_str()) { + return Err(MqttError::Configuration(format!( + "WebSocket subprotocol {subprotocol:?} is listed more than once" + ))); + } + offered.push(subprotocol); + } + if !offered.contains(&DEFAULT_SUBPROTOCOL) { + offered.push(DEFAULT_SUBPROTOCOL); + } + let subprotocols = offered.join(", "); headers.insert( SEC_WEBSOCKET_PROTOCOL, header_value("Sec-WebSocket-Protocol", &subprotocols)?, @@ -1017,6 +1038,11 @@ mod tests { "Sec-WebSocket-Accept", "Content-Length", "transfer-encoding", + "TE", + "Trailer", + "Keep-Alive", + "proxy-connection", + "Expect", ] { let config = WebSocketConfig::new("ws://localhost/mqtt") .unwrap() @@ -1108,6 +1134,40 @@ mod tests { } } + #[test] + fn test_handshake_request_appends_mqtt_to_other_subprotocols() { + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_subprotocol("mqttv5.0"); + let request = config.build_handshake_request().unwrap(); + assert_eq!( + request_header(&request, "Sec-WebSocket-Protocol"), + Some("mqttv5.0, mqtt") + ); + + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_subprotocols(&["mqtt", "mqttv5.0"]); + let request = config.build_handshake_request().unwrap(); + assert_eq!( + request_header(&request, "Sec-WebSocket-Protocol"), + Some("mqtt, mqttv5.0") + ); + } + + #[test] + fn test_handshake_request_rejects_duplicate_subprotocols() { + for list in [&["mqtt", "mqtt"][..], &["mqttv5.0", "mqtt", "mqttv5.0"][..]] { + let config = WebSocketConfig::new("ws://localhost/mqtt") + .unwrap() + .with_subprotocols(list); + assert!( + handshake_error(&config).contains("listed more than once"), + "{list:?} should be rejected" + ); + } + } + #[test] fn test_websocket_config_debug_redacts_header_values() { let config = WebSocketConfig::new("ws://localhost/mqtt") diff --git a/crates/mqtt5/tests/websocket_config_property_tests.rs b/crates/mqtt5/tests/websocket_config_property_tests.rs index 7f76f892..67556094 100644 --- a/crates/mqtt5/tests/websocket_config_property_tests.rs +++ b/crates/mqtt5/tests/websocket_config_property_tests.rs @@ -82,8 +82,9 @@ fn valid_http_headers() -> impl Strategy> { } // Header names a custom header may not use (lowercase): those reserved for -// the handshake, plus the two that have dedicated setters. -const RESTRICTED_HEADERS: [&str; 11] = [ +// the handshake, hop-by-hop and expectation headers, plus the two that have +// dedicated setters. +const RESTRICTED_HEADERS: [&str; 16] = [ "host", "connection", "upgrade", @@ -93,6 +94,11 @@ const RESTRICTED_HEADERS: [&str; 11] = [ "sec-websocket-accept", "content-length", "transfer-encoding", + "te", + "trailer", + "keep-alive", + "proxy-connection", + "expect", "sec-websocket-protocol", "user-agent", ]; @@ -211,13 +217,17 @@ proptest! { // The configuration must reach the handshake request, not just the struct. let mut seen = std::collections::HashSet::new(); + let mut seen_protocols = std::collections::HashSet::new(); let conflicting = headers.keys().any(|name| { let lower = name.to_ascii_lowercase(); RESTRICTED_HEADERS.contains(&lower.as_str()) || !seen.insert(lower) - }); + }) || subprotocols.iter().any(|p| !seen_protocols.insert(p.as_str())); let request = config.build_handshake_request(); if conflicting { - prop_assert!(request.is_err(), "conflicting headers {headers:?} were accepted"); + prop_assert!( + request.is_err(), + "conflicting headers {headers:?} or subprotocols {subprotocols:?} were accepted" + ); return Ok(()); } let request = request.expect("valid configuration builds a request"); @@ -231,11 +241,12 @@ proptest! { for (name, value) in &headers { prop_assert_eq!(sent(name), Some(value.clone())); } - let expected_protocols = if subprotocols.is_empty() { - "mqtt".to_string() - } else { - subprotocols.join(", ") - }; + // "mqtt" is always offered, after the configured subprotocols. + let mut expected_protocols = subprotocols.clone(); + if !expected_protocols.iter().any(|p| p == "mqtt") { + expected_protocols.push("mqtt".to_string()); + } + let expected_protocols = expected_protocols.join(", "); prop_assert_eq!(sent("Sec-WebSocket-Protocol"), Some(expected_protocols)); prop_assert_eq!(sent("User-Agent"), config.user_agent.clone()); } diff --git a/crates/mqtt5/tests/websocket_handshake_integration.rs b/crates/mqtt5/tests/websocket_handshake_integration.rs index 01d1e1b8..9fca053d 100644 --- a/crates/mqtt5/tests/websocket_handshake_integration.rs +++ b/crates/mqtt5/tests/websocket_handshake_integration.rs @@ -129,3 +129,45 @@ async fn invalid_header_fails_connect_before_dialing() { ); assert!(!transport.is_connected()); } + +/// The broker in this crate only selects a subprotocol the client offers, and +/// the client fails the handshake when the server selects none, so a client +/// that preferred `mqttv5.0` could only connect because `mqtt` is still +/// offered (MQTT-6.0.0-3). +#[cfg(feature = "broker")] +#[tokio::test] +async fn preferring_another_subprotocol_still_connects_to_the_broker() { + use mqtt5::broker::config::{ + BrokerConfig, StorageBackend, StorageConfig, WebSocketConfig as BrokerWebSocketConfig, + }; + use mqtt5::broker::MqttBroker; + + let config = BrokerConfig::default() + .with_storage(StorageConfig::new().with_backend(StorageBackend::Memory)) + .with_bind_address(([127, 0, 0, 1], 0)) + .with_websocket( + BrokerWebSocketConfig::new() + .with_bind_address(([127, 0, 0, 1], 0)) + .with_path("/mqtt"), + ); + let mut broker = MqttBroker::with_config(config).await.expect("broker"); + let addr = broker.ws_local_addr().expect("websocket listener"); + let mut ready = broker.ready_receiver(); + let broker_task = tokio::spawn(async move { broker.run().await }); + timeout(WAIT, ready.wait_for(|ready| *ready)) + .await + .expect("broker did not become ready") + .expect("broker stopped"); + + let config = WebSocketConfig::new(&format!("ws://{addr}/mqtt")) + .expect("config") + .with_subprotocol("mqttv5.0"); + let mut transport = WebSocketTransport::new(config); + timeout(WAIT, transport.connect()) + .await + .expect("connect timed out") + .expect("broker refused a client preferring mqttv5.0"); + + transport.close().await.expect("close"); + broker_task.abort(); +} From 4fbeb276aba003226c21fdd78905ff3feedcb77b Mon Sep 17 00:00:00 2001 From: Bryan Ashby Date: Thu, 8 Oct 2026 16:16:30 -0600 Subject: [PATCH 6/9] use platform roots for wss with a TLS config, keep stored ALPN off wss (#190) Review follow-up. With a tls_config and use_system_roots, wss:// now loads the platform's root certificates through rustls-native-certs, as tokio-tungstenite already did without a tls_config, instead of the bundled webpki-roots, so a CA installed on the machine keeps working for an ALPN-only config or MqttClient::set_tls_config(cert, key, None). TlsTransport keeps webpki-roots; roots are only loaded when the server certificate is verified. MqttClient's wss:// path no longer offers the stored config's ALPN protocols, which select MQTT directly over TLS; a WebSocketConfig's own ALPN is still offered, and the CHANGELOG notes that a server advertising only http/1.1 rejects an MQTT one. TlsConfig::addr and with_tls_auto document the 0.0.0.0 placeholder, and the private key is copied with clone_key() rather than an expect. The wss test server moves to a shared module for the new SSL_CERT_FILE test binary. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +- crates/mqtt5/Cargo.toml | 3 +- crates/mqtt5/src/client/inner.rs | 4 + crates/mqtt5/src/transport/tls.rs | 105 +++++++--- crates/mqtt5/src/transport/websocket.rs | 25 ++- .../mqtt5/tests/websocket_tls_integration.rs | 181 +++++------------- .../mqtt5/tests/websocket_tls_native_roots.rs | 99 ++++++++++ crates/mqtt5/tests/wss_test_server/mod.rs | 143 ++++++++++++++ 8 files changed, 393 insertions(+), 173 deletions(-) create mode 100644 crates/mqtt5/tests/websocket_tls_native_roots.rs create mode 100644 crates/mqtt5/tests/wss_test_server/mod.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f63bc08..22c3f0fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,15 +9,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **`wss://` connections now use the TLS configuration on `WebSocketConfig`** (#190). With server-certificate verification on, `WebSocketTransport::connect` used tokio-tungstenite's own client configuration built from the platform's root certificates, so a CA set with `with_ca_cert_from_file` or `with_ca_cert_from_bytes` was never trusted (a server it issued failed with `UnknownIssuer`), a client certificate set with `with_client_auth_from_files` or `with_client_auth_from_bytes` was never presented, and `use_system_roots` and ALPN protocols had no effect. With verification off, the client certificate and ALPN protocols were dropped. The handshake now uses the same rustls configuration `TlsTransport` builds from a `TlsConfig`, in both cases. -- **`WebSocketConfig::with_tls_auto`, and the `with_ca_cert_*` and `with_client_auth_*` methods that call it, now accept a host name URL** (#190). They parsed the URL's host and port as a socket address, so they failed with "Invalid host/port combination" for anything but an IP address. A wss:// connection takes its server and certificate name from the URL and does not use `TlsConfig::addr`, so for a host name that is now the unspecified address with the URL's port. +- **`wss://` connections now use the TLS configuration on `WebSocketConfig`** (#190). With server-certificate verification on, `WebSocketTransport::connect` used tokio-tungstenite's own client configuration built from the platform's root certificates, so a CA set with `with_ca_cert_from_file` or `with_ca_cert_from_bytes` was never trusted (a server it issued failed with `UnknownIssuer`), a client certificate set with `with_client_auth_from_files` or `with_client_auth_from_bytes` was never presented, and `use_system_roots` and ALPN protocols had no effect. With verification off, the client certificate and ALPN protocols were dropped. The handshake now uses the rustls configuration `TlsTransport` builds from a `TlsConfig`, in both cases, except that `use_system_roots` adds the platform's root certificates, as a `wss://` connection without a TLS configuration already did, rather than the bundled `webpki-roots`. +- **`WebSocketConfig::with_tls_auto`, and the `with_ca_cert_*` and `with_client_auth_*` methods that call it, now accept a host name URL** (#190). They parsed the URL's host and port as a socket address, so they failed with "Invalid host/port combination" for anything but an IP address. A wss:// connection takes its server and certificate name from the URL and does not use `TlsConfig::addr`, so for a host name that is now the unspecified address `0.0.0.0` with the URL's port; set a real address before using such a config with `TlsTransport`. - **`MqttClient` now uses its stored TLS configuration for `wss://` connections** (#190), as it already did for `mqtts://`. A CA, client certificate or key set with `set_tls_config` was ignored over WebSocket, and `set_insecure_tls(true)` replaced the stored configuration with a bare one that disables verification. - **The WebSocket client transport now sends its configured headers, subprotocols and user agent** (#165). `WebSocketTransport::connect` built the upgrade request from a fixed set of headers and always offered `Sec-WebSocket-Protocol: mqtt`, so anything set with `WebSocketConfig::with_header`, `with_subprotocol`, `with_subprotocols` or `with_user_agent` never reached the server, and brokers that authenticate the upgrade request through custom headers rejected the connection. The request now carries every custom header, the configured subprotocols in order, followed by `mqtt` when the list does not include it (MQTT-6.0.0-3), and the user agent when one is set. The default user agent is now `mqtt5/` instead of `mqtt-v5/0.4.0`. - **The upgrade request's `Host` header now includes a non-default port**, so `ws://broker:8080/mqtt` sends `Host: broker:8080` rather than `Host: broker`. ### Changed -- **A `wss://` connection with a TLS configuration now trusts the bundled `webpki-roots` instead of the platform's root certificates** when `use_system_roots` is set, as `TlsTransport` does. A server whose certificate chains only to a root installed on the platform, such as a corporate CA, now needs that CA added to the configuration. A `wss://` connection with no TLS configuration still uses the platform's roots. The new behavior also applies to `MqttClient` `wss://` connections once a TLS configuration is set with `set_tls_config`. +- **ALPN protocols on a `WebSocketConfig`'s TLS configuration are now offered on `wss://`.** They were ignored before. A WebSocket server negotiates HTTP, so one that advertises only `http/1.1`, as this crate's `wss://` listener does, now rejects a handshake that offers only an MQTT protocol such as `mqtt` with `NoApplicationProtocol`. `MqttClient` does not offer its stored configuration's ALPN protocols on `wss://` (for example those set with `connect_with_tls`, such as AWS IoT's `x-amzn-mqtt-ca`), since they select MQTT directly over TLS. - **WebSocket configurations that relied on the old behavior may connect differently or fail to connect.** Because the configuration now reaches the wire: - A custom header that is invalid or reserved (see the next entry) used to be silently dropped. It now makes `connect` fail. - Every WebSocket connection, including those made by `MqttClient`, now sends a `User-Agent` header. diff --git a/crates/mqtt5/Cargo.toml b/crates/mqtt5/Cargo.toml index c60c4973..c8fa57d6 100644 --- a/crates/mqtt5/Cargo.toml +++ b/crates/mqtt5/Cargo.toml @@ -45,7 +45,7 @@ opentelemetry_sdk = ["dep:opentelemetry_sdk"] opentelemetry-otlp = ["dep:opentelemetry-otlp"] tracing-opentelemetry = ["dep:tracing-opentelemetry"] tokio = ["dep:tokio"] -transport-websocket = ["dep:tokio-tungstenite", "dep:futures-util"] +transport-websocket = ["dep:tokio-tungstenite", "dep:futures-util", "dep:rustls-native-certs"] transport-quic = ["dep:quinn"] codec-gzip = ["dep:flate2"] codec-deflate = ["dep:flate2"] @@ -177,6 +177,7 @@ rustls-pki-types = "1.15.0" tokio = { version = "1.47", features = ["full"] } tokio-rustls = { version = "0.26", default-features = false, features = ["ring", "tls12"] } tokio-tungstenite = { version = "0.29", default-features = false, features = ["connect", "rustls-tls-native-roots"], optional = true } +rustls-native-certs = { version = "0.8", optional = true } quinn = { version = "0.11.11", default-features = false, features = ["rustls-ring", "runtime-tokio", "platform-verifier"], optional = true } webpki-roots = "1.0.8" diff --git a/crates/mqtt5/src/client/inner.rs b/crates/mqtt5/src/client/inner.rs index 7532ea13..f560c784 100644 --- a/crates/mqtt5/src/client/inner.rs +++ b/crates/mqtt5/src/client/inner.rs @@ -285,12 +285,16 @@ impl MqttClient { // As in connect_tls: a stored TLS config applies to wss:// too. With // none, an insecure connection still needs one to disable // verification; a secure one keeps the WebSocket transport's default. + // The stored ALPN protocols are left out: they name the protocol for + // MQTT directly over TLS (mqtts://, e.g. AWS IoT's x-amzn-mqtt-ca), + // while a WebSocket server negotiates HTTP. let stored = self.tls_config.read().await.clone(); let tls_config = match stored { Some(mut cfg) => { cfg.addr = addr; cfg.hostname = host.to_string(); cfg.verify_server_cert = !insecure; + cfg.alpn_protocols = None; Some(cfg) } None if insecure => Some(TlsConfig::new(addr, host).with_verify_server_cert(false)), diff --git a/crates/mqtt5/src/transport/tls.rs b/crates/mqtt5/src/transport/tls.rs index 94952d0c..5579d658 100644 --- a/crates/mqtt5/src/transport/tls.rs +++ b/crates/mqtt5/src/transport/tls.rs @@ -21,6 +21,13 @@ pub type TlsWriteHalf = WriteHalf>; #[derive(Debug)] pub struct TlsConfig { /// Server address + /// + /// [`TlsTransport`] dials it. A wss:// connection does not use it: the + /// WebSocket URL selects the server, so a config made by + /// [`WebSocketConfig::with_tls_auto`](crate::transport::websocket::WebSocketConfig::with_tls_auto) + /// for a host name URL holds the unspecified address `0.0.0.0` with the + /// URL's port, and must be given a real address before use with + /// [`TlsTransport`]. pub addr: SocketAddr, /// Server hostname for SNI pub hostname: String, @@ -47,10 +54,7 @@ impl Clone for TlsConfig { hostname: self.hostname.clone(), connect_timeout: self.connect_timeout, client_cert: self.client_cert.clone(), - client_key: self.client_key.as_ref().map(|key| { - PrivateKeyDer::try_from(key.secret_der().to_vec()) - .expect("Failed to clone private key") - }), + client_key: self.client_key.as_ref().map(PrivateKeyDer::clone_key), root_certs: self.root_certs.clone(), use_system_roots: self.use_system_roots, verify_server_cert: self.verify_server_cert, @@ -314,33 +318,39 @@ impl TlsConfig { /// server to dial and the name to verify, which the caller supplies when /// connecting. /// + /// With `use_system_roots`, the system roots are the bundled + /// `webpki-roots`, as [`TlsTransport`] uses them. + /// /// # Errors /// /// Returns an error if a root certificate cannot be added or the client /// certificate and key cannot be used for client authentication - /// - /// # Panics - /// - /// Panics if the stored client key cannot be re-parsed from its own DER - /// encoding, as [`TlsConfig`]'s `Clone` does. pub fn client_config(&self) -> Result { - let mut root_store = RootCertStore::empty(); + self.client_config_with(SystemRoots::Bundled) + } - // Add system roots if requested - if self.use_system_roots { - root_store.extend(webpki_roots::TLS_SERVER_ROOTS.to_vec()); - } + /// Builds the rustls client configuration, taking the system roots that + /// `use_system_roots` adds from `system_roots` + /// + /// Roots are only loaded when the server certificate is verified. + pub(crate) fn client_config_with(&self, system_roots: SystemRoots) -> Result { + let config_builder = if self.verify_server_cert { + let mut root_store = RootCertStore::empty(); + + // Add custom root certificates + if let Some(ref root_certs) = self.root_certs { + for cert in root_certs { + root_store.add(cert.clone()).map_err(|e| { + MqttError::ProtocolError(format!("Failed to add root cert: {e}")) + })?; + } + } - // Add custom root certificates - if let Some(ref root_certs) = self.root_certs { - for cert in root_certs { - root_store.add(cert.clone()).map_err(|e| { - MqttError::ProtocolError(format!("Failed to add root cert: {e}")) - })?; + // Add system roots if requested + if self.use_system_roots { + system_roots.add_to(&mut root_store)?; } - } - let config_builder = if self.verify_server_cert { ClientConfig::builder().with_root_certificates(root_store) } else { // Disable certificate verification for testing with self-signed certs @@ -351,10 +361,7 @@ impl TlsConfig { let mut config = if let (Some(cert), Some(key)) = ( self.client_cert.clone(), - self.client_key.as_ref().map(|k| { - PrivateKeyDer::try_from(k.secret_der().to_vec()) - .expect("Failed to clone private key") - }), + self.client_key.as_ref().map(PrivateKeyDer::clone_key), ) { config_builder .with_client_auth_cert(cert, key) @@ -374,6 +381,52 @@ impl TlsConfig { } } +/// Where the roots added by [`TlsConfig::use_system_roots`] come from +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum SystemRoots { + /// The bundled `webpki-roots` + Bundled, + /// The platform's certificate store, loaded with `rustls-native-certs` + #[cfg(feature = "transport-websocket")] + Native, +} + +impl SystemRoots { + // Only loading native roots can fail. + #[cfg_attr(not(feature = "transport-websocket"), allow(clippy::unnecessary_wraps))] + fn add_to(self, root_store: &mut RootCertStore) -> Result<()> { + match self { + Self::Bundled => { + root_store.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); + Ok(()) + } + #[cfg(feature = "transport-websocket")] + Self::Native => add_native_roots(root_store), + } + } +} + +/// Adds the platform's root certificates, as tokio-tungstenite does for a +/// wss:// connection without a TLS configuration: load errors are logged, and +/// only finding no usable certificate at all is an error, unless custom roots +/// were already added. +#[cfg(feature = "transport-websocket")] +fn add_native_roots(root_store: &mut RootCertStore) -> Result<()> { + let loaded = rustls_native_certs::load_native_certs(); + if !loaded.errors.is_empty() { + tracing::warn!(errors = ?loaded.errors, "errors loading native root certificates"); + } + let (added, ignored) = root_store.add_parsable_certificates(loaded.certs); + debug!(added, ignored, "loaded native root certificates"); + if root_store.is_empty() { + return Err(MqttError::ConnectionError(format!( + "no native root certificates found (errors: {:?})", + loaded.errors + ))); + } + Ok(()) +} + /// TLS transport implementation #[derive(Debug)] pub struct TlsTransport { diff --git a/crates/mqtt5/src/transport/websocket.rs b/crates/mqtt5/src/transport/websocket.rs index 694aac9c..3c53fb89 100644 --- a/crates/mqtt5/src/transport/websocket.rs +++ b/crates/mqtt5/src/transport/websocket.rs @@ -43,7 +43,7 @@ use crate::error::{MqttError, Result}; use crate::packet::Packet; use crate::time::Duration; use crate::transport::packet_io::{decode_buffered_packet, PacketReader, PacketWriter}; -use crate::transport::tls::TlsConfig; +use crate::transport::tls::{SystemRoots, TlsConfig}; use crate::Transport; use bytes::{Buf, Bytes, BytesMut}; use futures_util::{stream::SplitSink, stream::SplitStream, StreamExt}; @@ -222,12 +222,19 @@ impl WebSocketConfig { /// /// Its root certificates, system-roots setting, server-certificate /// verification, client certificate and ALPN protocols are used for the - /// TLS handshake; with `use_system_roots`, the system roots are the - /// bundled `webpki-roots`, as for [`TlsTransport`](crate::transport::tls::TlsTransport). + /// TLS handshake. With `use_system_roots`, the system roots are the + /// platform's native root certificates, as for a wss:// connection with no + /// TLS configuration, rather than the bundled `webpki-roots` that + /// [`TlsTransport`](crate::transport::tls::TlsTransport) uses. /// Its `addr`, `hostname` and `connect_timeout` are not used: the server /// and the name verified against its certificate come from the WebSocket /// URL, and the timeout from [`with_timeout`](Self::with_timeout). /// + /// ALPN protocols are offered as configured. A WebSocket server negotiates + /// HTTP, so one that advertises only `http/1.1`, as this crate's broker + /// does, rejects a handshake offering only an MQTT protocol such as + /// `mqtt` with `NoApplicationProtocol`. + /// /// Without a TLS configuration, a wss:// connection verifies the server /// against the platform's native root certificates. #[must_use] @@ -241,8 +248,10 @@ impl WebSocketConfig { /// This is a convenience method that creates a TLS config with the same /// host and port as the WebSocket URL. The host may be a name or an IP /// address. A wss:// connection does not use the config's `addr`, so for a - /// host name it is the unspecified address with the URL's port, rather - /// than a resolved one. + /// host name it is the unspecified address `0.0.0.0` with the URL's port, + /// rather than a resolved one. Set a real address before using the config + /// with [`TlsTransport`](crate::transport::tls::TlsTransport), which would + /// otherwise dial the local host. /// /// # Errors /// @@ -697,9 +706,9 @@ impl Transport for WebSocketTransport { // With no TLS configuration, tokio-tungstenite builds its own client // config from the platform's native root certificates. let connector = match self.config.tls_config.as_ref() { - Some(tls_config) if self.config.is_secure() => { - Some(Connector::Rustls(Arc::new(tls_config.client_config()?))) - } + Some(tls_config) if self.config.is_secure() => Some(Connector::Rustls(Arc::new( + tls_config.client_config_with(SystemRoots::Native)?, + ))), _ => None, }; diff --git a/crates/mqtt5/tests/websocket_tls_integration.rs b/crates/mqtt5/tests/websocket_tls_integration.rs index d2db4330..297cdbf9 100644 --- a/crates/mqtt5/tests/websocket_tls_integration.rs +++ b/crates/mqtt5/tests/websocket_tls_integration.rs @@ -6,144 +6,17 @@ //! //! Requires the certificates from `scripts/generate_test_certs.sh`. +mod wss_test_server; + use mqtt5::transport::tls::TlsConfig; -use mqtt5::transport::websocket::{WebSocketConfig, WebSocketTransport}; -use mqtt5::{ConnectOptions, MqttClient, Transport}; -use rustls::pki_types::pem::PemObject; -use rustls::pki_types::{CertificateDer, PrivateKeyDer}; -use rustls::server::WebPkiClientVerifier; -use rustls::{RootCertStore, ServerConfig}; -use std::net::SocketAddr; -use std::sync::Arc; -use std::time::Duration; -use tokio::net::TcpListener; -use tokio::sync::oneshot; +use mqtt5::transport::websocket::WebSocketConfig; +use mqtt5::{ConnectOptions, MqttClient}; +use rustls::pki_types::PrivateKeyDer; use tokio::time::timeout; -use tokio_rustls::TlsAcceptor; -use tokio_tungstenite::tungstenite::handshake::server::{ - Callback, ErrorResponse, Request, Response, +use wss_test_server::{ + certs, connect, key, server_config, wss_config, wss_server, CA, CLIENT_CERT, CLIENT_KEY, + SERVER_CERT, SERVER_KEY, WAIT, }; -use tokio_tungstenite::tungstenite::http::HeaderValue; - -const WAIT: Duration = Duration::from_secs(5); -const CA: &str = "../../test_certs/ca.pem"; -const SERVER_CERT: &str = "../../test_certs/server.pem"; -const SERVER_KEY: &str = "../../test_certs/server.key"; -const CLIENT_CERT: &str = "../../test_certs/client.pem"; -const CLIENT_KEY: &str = "../../test_certs/client.key"; - -/// What the server observed about a TLS handshake that completed. -#[derive(Debug)] -struct Handshake { - alpn: Option>, - client_presented_cert: bool, -} - -fn certs(path: &str) -> Vec> { - CertificateDer::pem_file_iter(path) - .unwrap_or_else(|e| panic!("read {path}: {e}")) - .collect::>() - .unwrap_or_else(|e| panic!("parse {path}: {e}")) -} - -fn key(path: &str) -> PrivateKeyDer<'static> { - PrivateKeyDer::from_pem_file(path).unwrap_or_else(|e| panic!("read {path}: {e}")) -} - -fn server_config( - cert: Vec>, - key: PrivateKeyDer<'static>, - require_client_cert: bool, - alpn: &[&[u8]], -) -> ServerConfig { - let _ = rustls::crypto::ring::default_provider().install_default(); - let builder = ServerConfig::builder(); - let mut config = if require_client_cert { - let mut roots = RootCertStore::empty(); - for ca in certs(CA) { - roots.add(ca).expect("add CA"); - } - let verifier = WebPkiClientVerifier::builder(Arc::new(roots)) - .build() - .expect("client verifier"); - builder.with_client_cert_verifier(verifier) - } else { - builder.with_no_client_auth() - } - .with_single_cert(cert, key) - .expect("server cert"); - config.alpn_protocols = alpn.iter().map(|p| p.to_vec()).collect(); - config -} - -/// Starts a one-connection WSS server. The receiver yields the handshake -/// details if the TLS and WebSocket handshakes both complete. -async fn wss_server(config: ServerConfig) -> (SocketAddr, oneshot::Receiver) { - let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); - let addr = listener.local_addr().expect("addr"); - let acceptor = TlsAcceptor::from(Arc::new(config)); - let (tx, rx) = oneshot::channel(); - tokio::spawn(async move { - let Ok((stream, _)) = listener.accept().await else { - return; - }; - let Ok(tls) = acceptor.accept(stream).await else { - return; - }; - let (_, session) = tls.get_ref(); - let handshake = Handshake { - alpn: session.alpn_protocol().map(<[u8]>::to_vec), - client_presented_cert: session.peer_certificates().is_some(), - }; - let Ok(ws) = tokio_tungstenite::accept_hdr_async(tls, SelectMqtt).await else { - return; - }; - let _ = tx.send(handshake); - // Hold the connection open until the client closes it. - let _ws = ws; - tokio::time::sleep(WAIT).await; - }); - (addr, rx) -} - -/// Selects the "mqtt" subprotocol the client offers by default, as a broker -/// would; the client fails the handshake if the server selects none. -struct SelectMqtt; - -impl Callback for SelectMqtt { - fn on_request(self, _req: &Request, mut resp: Response) -> Result { - resp.headers_mut() - .insert("Sec-WebSocket-Protocol", HeaderValue::from_static("mqtt")); - Ok(resp) - } -} - -fn wss_config(addr: SocketAddr) -> WebSocketConfig { - WebSocketConfig::new(&format!("wss://{addr}/mqtt")).expect("config") -} - -/// Connects and returns the server's view of the handshake, or the client's -/// error if the connection failed. -async fn connect( - config: WebSocketConfig, - server: oneshot::Receiver, -) -> Result { - let mut transport = WebSocketTransport::new(config); - let result = timeout(WAIT, transport.connect()) - .await - .expect("connect timed out"); - match result { - Ok(()) => { - let handshake = timeout(WAIT, server) - .await - .expect("server timed out") - .expect("server saw no completed handshake"); - transport.close().await.expect("close"); - Ok(handshake) - } - Err(e) => Err(e.to_string()), - } -} #[tokio::test] async fn custom_ca_from_tls_config_is_trusted() { @@ -331,3 +204,41 @@ async fn mqtt_client_presents_its_stored_tls_config_over_wss() { assert!(handshake.client_presented_cert); connecting.abort(); } + +#[tokio::test] +async fn mqtt_client_does_not_offer_its_stored_alpn_over_wss() { + // Like this crate's wss:// listener, the server advertises only http/1.1, + // so a client offering an MQTT protocol would get NoApplicationProtocol. + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + false, + &[b"http/1.1"], + )) + .await; + let client = MqttClient::with_options( + ConnectOptions::new("wss-stored-alpn").with_automatic_reconnect(false), + ); + + // connect_with_tls stores its configuration before dialing; a closed port + // makes it fail at once, leaving that configuration stored. + let closed = std::net::TcpListener::bind("127.0.0.1:0") + .and_then(|listener| listener.local_addr()) + .expect("pick a port"); + let mut stored = TlsConfig::new(closed, "127.0.0.1").with_alpn_protocols(&["mqtt"]); + stored.load_ca_cert_pem(CA).expect("load CA"); + client + .connect_with_tls(stored) + .await + .expect_err("nothing listens on the closed port"); + + let connecting = tokio::spawn(async move { + let _ = client.connect(&format!("wss://{addr}/mqtt")).await; + }); + let handshake = timeout(WAIT, server) + .await + .expect("server timed out") + .expect("server saw no completed handshake: the stored ALPN was offered"); + assert_eq!(handshake.alpn, None); + connecting.abort(); +} diff --git a/crates/mqtt5/tests/websocket_tls_native_roots.rs b/crates/mqtt5/tests/websocket_tls_native_roots.rs new file mode 100644 index 00000000..d16c0126 --- /dev/null +++ b/crates/mqtt5/tests/websocket_tls_native_roots.rs @@ -0,0 +1,99 @@ +#![cfg(feature = "transport-websocket")] + +//! Checks that `use_system_roots` on a wss:// connection's TLS configuration +//! means the platform's root certificates, as it does for a wss:// connection +//! with no TLS configuration. +//! +//! `SSL_CERT_FILE` stands in for a CA installed on the platform: +//! rustls-native-certs loads that file instead of the platform store. The +//! variable is process-wide, so these tests have their own binary. +//! +//! Requires the certificates from `scripts/generate_test_certs.sh`. + +mod wss_test_server; + +use mqtt5::transport::tls::TlsConfig; +use mqtt5::{ConnectOptions, MqttClient}; +use std::sync::Once; +use tokio::time::timeout; +use wss_test_server::{ + certs, connect, key, server_config, wss_config, wss_server, CA, CLIENT_CERT, CLIENT_KEY, + SERVER_CERT, SERVER_KEY, WAIT, +}; + +/// Makes the test CA a platform root for this process. Every test calls it +/// before connecting, and it writes the variable only once. +fn install_test_ca_as_platform_root() { + static INSTALL: Once = Once::new(); + INSTALL.call_once(|| std::env::set_var("SSL_CERT_FILE", CA)); +} + +#[tokio::test] +async fn platform_roots_are_used_without_a_configured_ca() { + install_test_ca_as_platform_root(); + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + false, + &[b"mqtt-test"], + )) + .await; + let tls_config = TlsConfig::new(addr, "127.0.0.1").with_alpn_protocols(&["mqtt-test"]); + + let handshake = connect(wss_config(addr).with_tls_config(tls_config), server) + .await + .expect("server certificate from a platform-installed CA was rejected"); + assert_eq!(handshake.alpn.as_deref(), Some(&b"mqtt-test"[..])); +} + +#[tokio::test] +async fn platform_roots_are_not_used_when_system_roots_are_off() { + install_test_ca_as_platform_root(); + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + false, + &[], + )) + .await; + let tls_config = TlsConfig::new(addr, "127.0.0.1").with_system_roots(false); + + let err = connect(wss_config(addr).with_tls_config(tls_config), server) + .await + .expect_err("server certificate was trusted with no roots configured"); + assert!(err.contains("UnknownIssuer"), "unexpected error: {err}"); +} + +#[tokio::test] +async fn mqtt_client_with_cert_and_key_but_no_ca_trusts_platform_roots() { + install_test_ca_as_platform_root(); + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + true, + &[], + )) + .await; + let client = MqttClient::with_options( + ConnectOptions::new("wss-platform-roots").with_automatic_reconnect(false), + ); + client + .set_tls_config( + Some(std::fs::read(CLIENT_CERT).expect("read client cert")), + Some(std::fs::read(CLIENT_KEY).expect("read client key")), + None, + ) + .await; + + // The test server stops after the WebSocket handshake and never answers + // CONNECT, so only the TLS handshake it observed is checked. + let connecting = tokio::spawn(async move { + let _ = client.connect(&format!("wss://{addr}/mqtt")).await; + }); + let handshake = timeout(WAIT, server) + .await + .expect("server timed out") + .expect("server saw no completed handshake: the platform roots were not used"); + assert!(handshake.client_presented_cert); + connecting.abort(); +} diff --git a/crates/mqtt5/tests/wss_test_server/mod.rs b/crates/mqtt5/tests/wss_test_server/mod.rs new file mode 100644 index 00000000..a55c7c0d --- /dev/null +++ b/crates/mqtt5/tests/wss_test_server/mod.rs @@ -0,0 +1,143 @@ +//! A loopback WSS server for the wss:// TLS tests, reporting what it observed +//! about each TLS handshake. +//! +//! Requires the certificates from `scripts/generate_test_certs.sh`. +#![allow(dead_code)] + +use mqtt5::transport::websocket::{WebSocketConfig, WebSocketTransport}; +use mqtt5::Transport; +use rustls::pki_types::pem::PemObject; +use rustls::pki_types::{CertificateDer, PrivateKeyDer}; +use rustls::server::WebPkiClientVerifier; +use rustls::{RootCertStore, ServerConfig}; +use std::net::SocketAddr; +use std::sync::Arc; +use std::time::Duration; +use tokio::net::TcpListener; +use tokio::sync::oneshot; +use tokio::time::timeout; +use tokio_rustls::TlsAcceptor; +use tokio_tungstenite::tungstenite::handshake::server::{ + Callback, ErrorResponse, Request, Response, +}; +use tokio_tungstenite::tungstenite::http::HeaderValue; + +pub const WAIT: Duration = Duration::from_secs(5); +pub const CA: &str = "../../test_certs/ca.pem"; +pub const SERVER_CERT: &str = "../../test_certs/server.pem"; +pub const SERVER_KEY: &str = "../../test_certs/server.key"; +pub const CLIENT_CERT: &str = "../../test_certs/client.pem"; +pub const CLIENT_KEY: &str = "../../test_certs/client.key"; + +/// What the server observed about a TLS handshake that completed. +#[derive(Debug)] +pub struct Handshake { + pub alpn: Option>, + pub client_presented_cert: bool, +} + +pub fn certs(path: &str) -> Vec> { + CertificateDer::pem_file_iter(path) + .unwrap_or_else(|e| panic!("read {path}: {e}")) + .collect::>() + .unwrap_or_else(|e| panic!("parse {path}: {e}")) +} + +pub fn key(path: &str) -> PrivateKeyDer<'static> { + PrivateKeyDer::from_pem_file(path).unwrap_or_else(|e| panic!("read {path}: {e}")) +} + +pub fn server_config( + cert: Vec>, + key: PrivateKeyDer<'static>, + require_client_cert: bool, + alpn: &[&[u8]], +) -> ServerConfig { + let _ = rustls::crypto::ring::default_provider().install_default(); + let builder = ServerConfig::builder(); + let mut config = if require_client_cert { + let mut roots = RootCertStore::empty(); + for ca in certs(CA) { + roots.add(ca).expect("add CA"); + } + let verifier = WebPkiClientVerifier::builder(Arc::new(roots)) + .build() + .expect("client verifier"); + builder.with_client_cert_verifier(verifier) + } else { + builder.with_no_client_auth() + } + .with_single_cert(cert, key) + .expect("server cert"); + config.alpn_protocols = alpn.iter().map(|p| p.to_vec()).collect(); + config +} + +/// Starts a one-connection WSS server. The receiver yields the handshake +/// details if the TLS and WebSocket handshakes both complete. +pub async fn wss_server(config: ServerConfig) -> (SocketAddr, oneshot::Receiver) { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let addr = listener.local_addr().expect("addr"); + let acceptor = TlsAcceptor::from(Arc::new(config)); + let (tx, rx) = oneshot::channel(); + tokio::spawn(async move { + let Ok((stream, _)) = listener.accept().await else { + return; + }; + let Ok(tls) = acceptor.accept(stream).await else { + return; + }; + let (_, session) = tls.get_ref(); + let handshake = Handshake { + alpn: session.alpn_protocol().map(<[u8]>::to_vec), + client_presented_cert: session.peer_certificates().is_some(), + }; + let Ok(ws) = tokio_tungstenite::accept_hdr_async(tls, SelectMqtt).await else { + return; + }; + let _ = tx.send(handshake); + // Hold the connection open until the client closes it. + let _ws = ws; + tokio::time::sleep(WAIT).await; + }); + (addr, rx) +} + +/// Selects the "mqtt" subprotocol the client offers by default, as a broker +/// would; the client fails the handshake if the server selects none. +pub struct SelectMqtt; + +impl Callback for SelectMqtt { + fn on_request(self, _req: &Request, mut resp: Response) -> Result { + resp.headers_mut() + .insert("Sec-WebSocket-Protocol", HeaderValue::from_static("mqtt")); + Ok(resp) + } +} + +pub fn wss_config(addr: SocketAddr) -> WebSocketConfig { + WebSocketConfig::new(&format!("wss://{addr}/mqtt")).expect("config") +} + +/// Connects and returns the server's view of the handshake, or the client's +/// error if the connection failed. +pub async fn connect( + config: WebSocketConfig, + server: oneshot::Receiver, +) -> Result { + let mut transport = WebSocketTransport::new(config); + let result = timeout(WAIT, transport.connect()) + .await + .expect("connect timed out"); + match result { + Ok(()) => { + let handshake = timeout(WAIT, server) + .await + .expect("server timed out") + .expect("server saw no completed handshake"); + transport.close().await.expect("close"); + Ok(handshake) + } + Err(e) => Err(e.to_string()), + } +} From 1451548077d39b8e0f41c0da07a437aa1d037c26 Mon Sep 17 00:00:00 2001 From: Bryan Ashby Date: Thu, 8 Oct 2026 16:22:23 -0600 Subject: [PATCH 7/9] add ConnectOptions::with_websocket_config for MqttClient (#165) MqttClient built each connection's WebSocketConfig from the URL alone, so its ws:// and wss:// connections could not send custom handshake headers, subprotocols or a user agent. ConnectOptions::with_websocket_config stores a WebSocketConfig that the client reads on every connection attempt, including automatic reconnects, with the URL replaced by the address passed to connect. For wss://, its TLS configuration, when set, is used in place of the stored one; insecure_tls still disables verification. WebSocketConfig now implements Clone, and is held behind an Arc so the connect futures stay small. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 + crates/mqtt5/src/client/inner.rs | 44 +++-- crates/mqtt5/src/transport/websocket.rs | 38 ++-- crates/mqtt5/src/types.rs | 49 +++++- crates/mqtt5/tests/client_websocket_config.rs | 162 ++++++++++++++++++ 5 files changed, 267 insertions(+), 28 deletions(-) create mode 100644 crates/mqtt5/tests/client_websocket_config.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 22c3f0fd..a37eaab8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **`ConnectOptions::with_websocket_config`, so `MqttClient` can send custom WebSocket handshake headers** (#165). `MqttClient` built each connection's `WebSocketConfig` from the URL alone, so its ws:// and wss:// connections could not carry custom headers, subprotocols or a user agent. The configuration set here is applied on every connection attempt, including automatic reconnects, with the URL taken from the address passed to `connect`; for wss://, its TLS configuration, when it has one, is used in place of the one stored with `set_tls_config` or `connect_with_tls`. +- `WebSocketConfig` implements `Clone`. - `TlsConfig::client_config`, which returns the rustls client configuration a `TlsConfig` describes. - `WebSocketConfig::build_handshake_request`, which returns the upgrade request the configuration produces. diff --git a/crates/mqtt5/src/client/inner.rs b/crates/mqtt5/src/client/inner.rs index f560c784..972c2773 100644 --- a/crates/mqtt5/src/client/inner.rs +++ b/crates/mqtt5/src/client/inner.rs @@ -208,7 +208,7 @@ impl MqttClient { ClientTransportType::Tcp => Self::connect_tcp(addr).await, ClientTransportType::Tls => self.connect_tls(addr, host).await, #[cfg(feature = "transport-websocket")] - ClientTransportType::WebSocket(url) => Self::connect_websocket(&url).await, + ClientTransportType::WebSocket(url) => self.connect_websocket(&url).await, #[cfg(feature = "transport-websocket")] ClientTransportType::WebSocketSecure(url) => { self.connect_websocket_secure(addr, host, &url).await @@ -259,16 +259,23 @@ impl MqttClient { Ok(TransportType::Tls(Box::new(tls_transport))) } + /// The WebSocket configuration for a connection to `url`: the one set with + /// `ConnectOptions::with_websocket_config`, read on every attempt so + /// reconnects use it too, or a default one. #[cfg(feature = "transport-websocket")] - async fn connect_websocket(url: &str) -> Result { - let config = WebSocketConfig::new(url) - .map_err(|e| MqttError::ConnectionError(format!("Invalid WebSocket URL: {e}")))?; - let mut ws_transport = WebSocketTransport::new(config); - ws_transport - .connect() - .await - .map_err(|e| MqttError::ConnectionError(format!("WebSocket connect failed: {e}")))?; - Ok(TransportType::WebSocket(Box::new(ws_transport))) + async fn websocket_config_for(&self, url: &str) -> Result { + let configured = self.inner.read().await.options.websocket_config.clone(); + match configured { + Some(config) => WebSocketConfig::clone(&config).with_url(url), + None => WebSocketConfig::new(url), + } + .map_err(|e| MqttError::ConnectionError(format!("Invalid WebSocket URL: {e}"))) + } + + #[cfg(feature = "transport-websocket")] + async fn connect_websocket(&self, url: &str) -> Result { + let config = self.websocket_config_for(url).await?; + Self::open_websocket(config).await } #[cfg(feature = "transport-websocket")] @@ -279,8 +286,16 @@ impl MqttClient { url: &str, ) -> Result { let insecure = self.transport_config.read().await.insecure_tls; - let mut config = WebSocketConfig::new(url) - .map_err(|e| MqttError::ConnectionError(format!("Invalid WebSocket URL: {e}")))?; + let mut config = self.websocket_config_for(url).await?; + + // A TLS configuration on the WebSocket configuration is used as given, + // except that insecure_tls still disables verification. + if let Some(tls_config) = config.tls_config.as_mut() { + if insecure { + tls_config.verify_server_cert = false; + } + return Self::open_websocket(config).await; + } // As in connect_tls: a stored TLS config applies to wss:// too. With // none, an insecure connection still needs one to disable @@ -304,6 +319,11 @@ impl MqttClient { config = config.with_tls_config(tls_config); } + Self::open_websocket(config).await + } + + #[cfg(feature = "transport-websocket")] + async fn open_websocket(config: WebSocketConfig) -> Result { let mut ws_transport = WebSocketTransport::new(config); ws_transport .connect() diff --git a/crates/mqtt5/src/transport/websocket.rs b/crates/mqtt5/src/transport/websocket.rs index 3c53fb89..45b1c143 100644 --- a/crates/mqtt5/src/transport/websocket.rs +++ b/crates/mqtt5/src/transport/websocket.rs @@ -97,6 +97,7 @@ const RESERVED_HEADERS: [&str; 14] = [ ]; /// WebSocket transport configuration +#[derive(Clone)] pub struct WebSocketConfig { /// WebSocket URL (ws:// or wss://) pub url: Url, @@ -135,20 +136,8 @@ impl WebSocketConfig { /// /// Returns an error if the URL is invalid or uses an unsupported scheme pub fn new(url: &str) -> Result { - let parsed_url = Url::parse(url) - .map_err(|e| MqttError::ProtocolError(format!("Invalid WebSocket URL: {e}")))?; - - match parsed_url.scheme() { - "ws" | "wss" => {} - scheme => { - return Err(MqttError::ProtocolError(format!( - "Unsupported WebSocket scheme: {scheme}. Use 'ws' or 'wss'" - ))); - } - } - Ok(Self { - url: parsed_url, + url: parse_websocket_url(url)?, timeout: Duration::from_secs(30), subprotocols: vec![DEFAULT_SUBPROTOCOL.to_string()], headers: HashMap::new(), @@ -157,6 +146,16 @@ impl WebSocketConfig { }) } + /// Returns this configuration with its URL replaced + /// + /// Used by `MqttClient`, which connects to the address it is given with + /// the rest of the configuration from + /// [`ConnectOptions::with_websocket_config`](crate::ConnectOptions::with_websocket_config). + pub(crate) fn with_url(mut self, url: &str) -> Result { + self.url = parse_websocket_url(url)?; + Ok(self) + } + /// Sets the connection timeout #[must_use] pub fn with_timeout(mut self, timeout: Duration) -> Self { @@ -512,6 +511,19 @@ fn header_value(name: &str, value: &str) -> Result { /// Whether `s` is an HTTP token (RFC 9110 §5.6.2), as RFC 6455 §4.1 requires /// of each offered subprotocol. +/// Parses a WebSocket URL, accepting only the ws and wss schemes +fn parse_websocket_url(url: &str) -> Result { + let parsed_url = Url::parse(url) + .map_err(|e| MqttError::ProtocolError(format!("Invalid WebSocket URL: {e}")))?; + + match parsed_url.scheme() { + "ws" | "wss" => Ok(parsed_url), + scheme => Err(MqttError::ProtocolError(format!( + "Unsupported WebSocket scheme: {scheme}. Use 'ws' or 'wss'" + ))), + } +} + fn is_http_token(s: &str) -> bool { !s.is_empty() && s.bytes() diff --git a/crates/mqtt5/src/types.rs b/crates/mqtt5/src/types.rs index f613bf0e..2242fda8 100644 --- a/crates/mqtt5/src/types.rs +++ b/crates/mqtt5/src/types.rs @@ -47,6 +47,10 @@ pub struct ConnectOptions { /// Session Expiry Interval (`with_session_expiry_interval`): under MQTT v5 an absent /// Session Expiry Interval means 0, so the session ends when the connection closes. pub resume_existing_session: bool, + /// WebSocket configuration for ws:// and wss:// connections; see + /// [`ConnectOptions::with_websocket_config`]. + #[cfg(feature = "transport-websocket")] + pub websocket_config: Option>, } impl ConnectOptions { @@ -60,9 +64,45 @@ impl ConnectOptions { codec_registry: None, deferred_ack: false, resume_existing_session: false, + #[cfg(feature = "transport-websocket")] + websocket_config: None, } } + /// Sets the WebSocket configuration for ws:// and wss:// connections + /// + /// It is applied on every connection attempt, including automatic + /// reconnects: its custom headers, subprotocols, user agent and timeout go + /// into each upgrade request, and for wss:// its TLS configuration, when it + /// has one, is used in place of the one stored with `set_tls_config` or + /// `connect_with_tls` (`set_insecure_tls(true)` still disables + /// verification). The client connects to the address passed to `connect`, + /// so the configuration's own `url` is not used; build it from that + /// address or any URL with the same scheme. + /// + /// ```rust,no_run + /// # use mqtt5::{ConnectOptions, MqttClient}; + /// # use mqtt5::transport::websocket::WebSocketConfig; + /// # async fn example() -> mqtt5::Result<()> { + /// let address = "wss://broker.example.com/mqtt"; + /// let websocket = WebSocketConfig::new(address)? + /// .with_header("x-amz-customauthorizer-name", "my-authorizer"); + /// let options = ConnectOptions::new("client-1").with_websocket_config(websocket); + /// let client = MqttClient::with_options(options); + /// client.connect(address).await?; + /// # Ok(()) + /// # } + /// ``` + #[cfg(feature = "transport-websocket")] + #[must_use] + pub fn with_websocket_config( + mut self, + config: crate::transport::websocket::WebSocketConfig, + ) -> Self { + self.websocket_config = Some(Arc::new(config)); + self + } + /// Opts in to resuming a broker-held session without local session state. /// /// See [`ConnectOptions::resume_existing_session`]. @@ -248,7 +288,8 @@ impl ConnectOptions { impl std::fmt::Debug for ConnectOptions { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("ConnectOptions") + let mut debug = f.debug_struct("ConnectOptions"); + debug .field("protocol_options", &self.protocol_options) .field("session_config", &self.session_config) .field("reconnect_config", &self.reconnect_config) @@ -258,8 +299,10 @@ impl std::fmt::Debug for ConnectOptions { &self.codec_registry.as_ref().map(|_| "CodecRegistry"), ) .field("deferred_ack", &self.deferred_ack) - .field("resume_existing_session", &self.resume_existing_session) - .finish() + .field("resume_existing_session", &self.resume_existing_session); + #[cfg(feature = "transport-websocket")] + debug.field("websocket_config", &self.websocket_config); + debug.finish() } } diff --git a/crates/mqtt5/tests/client_websocket_config.rs b/crates/mqtt5/tests/client_websocket_config.rs new file mode 100644 index 00000000..3a8decab --- /dev/null +++ b/crates/mqtt5/tests/client_websocket_config.rs @@ -0,0 +1,162 @@ +#![cfg(feature = "transport-websocket")] + +//! Checks that `ConnectOptions::with_websocket_config` reaches the upgrade +//! request of every connection `MqttClient` makes, including automatic +//! reconnects, as seen by a server. +//! +//! The wss:// test requires the certificates from +//! `scripts/generate_test_certs.sh`. + +mod wss_test_server; + +use futures::{SinkExt, StreamExt}; +use mqtt5::time::Duration; +use mqtt5::transport::websocket::WebSocketConfig; +use mqtt5::{ConnectOptions, MqttClient}; +use tokio::net::TcpListener; +use tokio::sync::mpsc; +use tokio::time::timeout; +use tokio_tungstenite::tungstenite::handshake::server::{ + Callback, ErrorResponse, Request, Response, +}; +use tokio_tungstenite::tungstenite::http::HeaderMap; +use tokio_tungstenite::tungstenite::Message; +use wss_test_server::{certs, key, server_config, wss_server, CA, SERVER_CERT, SERVER_KEY, WAIT}; + +/// An MQTT v5 CONNACK accepting the connection: no session, success, no +/// properties. +const CONNACK: [u8; 5] = [0x20, 0x03, 0x00, 0x00, 0x00]; + +/// Sends each upgrade request's headers to the test and selects the first +/// offered subprotocol, as a broker would. +struct Capture(mpsc::UnboundedSender); + +impl Callback for Capture { + fn on_request(self, req: &Request, mut resp: Response) -> Result { + let _ = self.0.send(req.headers().clone()); + if let Some(first) = req + .headers() + .get("Sec-WebSocket-Protocol") + .and_then(|v| v.to_str().ok()) + .and_then(|v| v.split(',').next()) + { + let first = first.trim().parse().expect("subprotocol header value"); + resp.headers_mut().insert("Sec-WebSocket-Protocol", first); + } + Ok(resp) + } +} + +/// Starts a ws:// server that accepts each connection's CONNECT with a +/// CONNACK and then closes it, so the client reconnects. The receiver yields +/// the headers of every upgrade request. +async fn closing_mqtt_server() -> (String, mpsc::UnboundedReceiver) { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let addr = listener.local_addr().expect("addr"); + let (tx, rx) = mpsc::unbounded_channel(); + tokio::spawn(async move { + while let Ok((stream, _)) = listener.accept().await { + let capture = Capture(tx.clone()); + tokio::spawn(async move { + let Ok(mut ws) = tokio_tungstenite::accept_hdr_async(stream, capture).await else { + return; + }; + // CONNECT + if !matches!(ws.next().await, Some(Ok(Message::Binary(_)))) { + return; + } + let _ = ws.send(Message::Binary(CONNACK.to_vec().into())).await; + tokio::time::sleep(std::time::Duration::from_millis(200)).await; + }); + } + }); + (format!("ws://{addr}/mqtt"), rx) +} + +fn header<'a>(headers: &'a HeaderMap, name: &str) -> Option<&'a str> { + headers.get(name).map(|v| v.to_str().expect("ascii header")) +} + +#[tokio::test] +async fn websocket_config_is_sent_on_connect_and_on_reconnect() { + let (url, mut requests) = closing_mqtt_server().await; + // The configuration's own URL is not used: the client connects to `url`. + let websocket = WebSocketConfig::new("ws://placeholder.invalid/other") + .expect("config") + .with_header("Authorization", "Bearer token123") + .with_subprotocol("mqttv5.0") + .with_user_agent("configured/1.0"); + let options = ConnectOptions::new("ws-config-client") + .with_websocket_config(websocket) + .with_reconnect_delay(Duration::from_millis(100), Duration::from_millis(500)); + let client = MqttClient::with_options(options); + + timeout(WAIT, client.connect(&url)) + .await + .expect("connect timed out") + .expect("connect"); + + for attempt in ["connect", "reconnect"] { + let headers = timeout(WAIT, requests.recv()) + .await + .unwrap_or_else(|_| panic!("no upgrade request for the {attempt}")) + .expect("server stopped"); + assert_eq!( + header(&headers, "Authorization"), + Some("Bearer token123"), + "{attempt}" + ); + assert_eq!( + header(&headers, "Sec-WebSocket-Protocol"), + Some("mqttv5.0, mqtt"), + "{attempt}" + ); + assert_eq!( + header(&headers, "User-Agent"), + Some("configured/1.0"), + "{attempt}" + ); + assert_eq!( + header(&headers, "Host"), + url.strip_prefix("ws://") + .and_then(|r| r.strip_suffix("/mqtt")), + "{attempt}" + ); + } + + let _ = client.disconnect().await; +} + +#[tokio::test] +async fn websocket_config_tls_is_used_for_wss() { + let (addr, server) = wss_server(server_config( + certs(SERVER_CERT), + key(SERVER_KEY), + false, + &[], + )) + .await; + // No TLS configuration is stored on the client, and the platform roots do + // not include the test CA, so the handshake only succeeds if the + // WebSocket configuration's CA is used. + let websocket = WebSocketConfig::new("wss://localhost/mqtt") + .expect("config") + .with_ca_cert_from_file(CA) + .expect("load CA"); + let client = MqttClient::with_options( + ConnectOptions::new("wss-config-client") + .with_websocket_config(websocket) + .with_automatic_reconnect(false), + ); + + // The test server stops after the WebSocket handshake and never answers + // CONNECT, so only the handshake it observed is checked. + let connecting = tokio::spawn(async move { + let _ = client.connect(&format!("wss://{addr}/mqtt")).await; + }); + timeout(WAIT, server) + .await + .expect("server timed out") + .expect("server saw no completed handshake: the configured CA was not used"); + connecting.abort(); +} From 46bc5c077c4dc4ff4341fe8e63cd023c63cab7d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabr=C3=ADcio=20Bracht?= Date: Fri, 9 Oct 2026 14:15:35 -0700 Subject: [PATCH 8/9] note the new ConnectOptions field, box large test futures, move is_http_token doc --- CHANGELOG.md | 4 ++++ crates/mqtt5/src/transport/websocket.rs | 4 ++-- crates/mqtt5/tests/broker_quic_integration.rs | 8 ++++---- crates/mqtt5/tests/clean_start_prebind_subscribe.rs | 4 ++-- crates/mqtt5/tests/quic_multistream_integration.rs | 12 ++++++------ 5 files changed, 18 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fb9ee53..6411e44c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Breaking + +- **`ConnectOptions` has a new public field, `websocket_config`.** Code that builds `ConnectOptions` with a struct literal must add it. The field exists only with the `transport-websocket` feature, which is on by default. + ### Fixed - **`wss://` connections now use the TLS configuration on `WebSocketConfig`** (#190). With server-certificate verification on, `WebSocketTransport::connect` used tokio-tungstenite's own client configuration built from the platform's root certificates, so a CA set with `with_ca_cert_from_file` or `with_ca_cert_from_bytes` was never trusted (a server it issued failed with `UnknownIssuer`), a client certificate set with `with_client_auth_from_files` or `with_client_auth_from_bytes` was never presented, and `use_system_roots` and ALPN protocols had no effect. With verification off, the client certificate and ALPN protocols were dropped. The handshake now uses the rustls configuration `TlsTransport` builds from a `TlsConfig`, in both cases, except that `use_system_roots` adds the platform's root certificates, as a `wss://` connection without a TLS configuration already did, rather than the bundled `webpki-roots`. diff --git a/crates/mqtt5/src/transport/websocket.rs b/crates/mqtt5/src/transport/websocket.rs index 45b1c143..a35b51bf 100644 --- a/crates/mqtt5/src/transport/websocket.rs +++ b/crates/mqtt5/src/transport/websocket.rs @@ -509,8 +509,6 @@ fn header_value(name: &str, value: &str) -> Result { }) } -/// Whether `s` is an HTTP token (RFC 9110 §5.6.2), as RFC 6455 §4.1 requires -/// of each offered subprotocol. /// Parses a WebSocket URL, accepting only the ws and wss schemes fn parse_websocket_url(url: &str) -> Result { let parsed_url = Url::parse(url) @@ -524,6 +522,8 @@ fn parse_websocket_url(url: &str) -> Result { } } +/// Whether `s` is an HTTP token (RFC 9110 §5.6.2), as RFC 6455 §4.1 requires +/// of each offered subprotocol. fn is_http_token(s: &str) -> bool { !s.is_empty() && s.bytes() diff --git a/crates/mqtt5/tests/broker_quic_integration.rs b/crates/mqtt5/tests/broker_quic_integration.rs index 7b8d00b2..1591fd45 100644 --- a/crates/mqtt5/tests/broker_quic_integration.rs +++ b/crates/mqtt5/tests/broker_quic_integration.rs @@ -164,22 +164,22 @@ async fn test_broker_default_quic_port() { #[tokio::test] async fn test_control_only_reconnect_publish_does_not_reenter_disconnect_loop() { - assert_control_only_reconnect_publish_stable(false).await; + Box::pin(assert_control_only_reconnect_publish_stable(false)).await; } #[tokio::test] async fn test_control_only_clean_session_reconnect_publish_does_not_reenter_disconnect_loop() { - assert_control_only_reconnect_publish_stable(true).await; + Box::pin(assert_control_only_reconnect_publish_stable(true)).await; } #[tokio::test] async fn test_control_only_clean_session_qos0_burst_after_reconnect_does_not_disconnect() { - assert_control_only_reconnect_qos0_burst_stable(false).await; + Box::pin(assert_control_only_reconnect_qos0_burst_stable(false)).await; } #[tokio::test] async fn test_control_only_secure_clean_session_qos0_burst_after_reconnect_does_not_disconnect() { - assert_control_only_reconnect_qos0_burst_stable(true).await; + Box::pin(assert_control_only_reconnect_qos0_burst_stable(true)).await; } async fn assert_control_only_reconnect_publish_stable(clean_start: bool) { diff --git a/crates/mqtt5/tests/clean_start_prebind_subscribe.rs b/crates/mqtt5/tests/clean_start_prebind_subscribe.rs index 97cde09b..7b554184 100644 --- a/crates/mqtt5/tests/clean_start_prebind_subscribe.rs +++ b/crates/mqtt5/tests/clean_start_prebind_subscribe.rs @@ -124,7 +124,7 @@ async fn retained_received_after_takeover_during_hand_off(clean_start: bool) -> #[tokio::test] async fn clean_start_subscribe_during_hand_off_still_gets_retained_message() { assert_eq!( - retained_received_after_takeover_during_hand_off(true).await, + Box::pin(retained_received_after_takeover_during_hand_off(true)).await, 1 ); } @@ -132,7 +132,7 @@ async fn clean_start_subscribe_during_hand_off_still_gets_retained_message() { #[tokio::test] async fn resumed_subscribe_during_hand_off_gets_retained_message() { assert_eq!( - retained_received_after_takeover_during_hand_off(false).await, + Box::pin(retained_received_after_takeover_during_hand_off(false)).await, 1 ); } diff --git a/crates/mqtt5/tests/quic_multistream_integration.rs b/crates/mqtt5/tests/quic_multistream_integration.rs index ec36b511..301c2e1b 100644 --- a/crates/mqtt5/tests/quic_multistream_integration.rs +++ b/crates/mqtt5/tests/quic_multistream_integration.rs @@ -822,12 +822,12 @@ async fn test_discard_flow_without_flow_headers_returns_error() { #[tokio::test] async fn test_quic_per_publish_under_tight_stream_limit() { - let received = publish_and_count( + let received = Box::pin(publish_and_count( test_quic_config().with_max_concurrent_streams(2), StreamStrategy::DataPerPublish, 20, 16, - ) + )) .await; assert_eq!( received, 20, @@ -837,12 +837,12 @@ async fn test_quic_per_publish_under_tight_stream_limit() { #[tokio::test] async fn test_quic_control_only_under_small_stream_window() { - let received = publish_and_count( + let received = Box::pin(publish_and_count( test_quic_config().with_stream_receive_window(2048), StreamStrategy::ControlOnly, 50, 512, - ) + )) .await; assert_eq!( received, 50, @@ -852,12 +852,12 @@ async fn test_quic_control_only_under_small_stream_window() { #[tokio::test] async fn test_quic_delivery_with_segmentation_offload_disabled() { - let received = publish_and_count( + let received = Box::pin(publish_and_count( test_quic_config().with_disable_segmentation_offload(true), StreamStrategy::ControlOnly, 50, 512, - ) + )) .await; assert_eq!( received, 50, From 7397978a2e8f1e0d0b167197e0f81c4ce247bf08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabr=C3=ADcio=20Bracht?= Date: Sat, 10 Oct 2026 12:20:58 -0700 Subject: [PATCH 9/9] keep websocket config headers off server redirects --- CHANGELOG.md | 2 +- crates/mqtt5/src/client/inner.rs | 69 ++++++++++++++----- crates/mqtt5/src/client/mod.rs | 12 ++-- crates/mqtt5/src/client/state.rs | 23 ++++++- crates/mqtt5/src/types.rs | 4 +- crates/mqtt5/tests/client_websocket_config.rs | 61 ++++++++++++---- 6 files changed, 133 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6411e44c..9c0dfdcb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,7 +36,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- **`ConnectOptions::with_websocket_config`, so `MqttClient` can send custom WebSocket handshake headers** (#165). `MqttClient` built each connection's `WebSocketConfig` from the URL alone, so its ws:// and wss:// connections could not carry custom headers, subprotocols or a user agent. The configuration set here is applied on every connection attempt, including automatic reconnects, with the URL taken from the address passed to `connect`; for wss://, its TLS configuration, when it has one, is used in place of the one stored with `set_tls_config` or `connect_with_tls`. +- **`ConnectOptions::with_websocket_config`, so `MqttClient` can send custom WebSocket handshake headers** (#165). `MqttClient` built each connection's `WebSocketConfig` from the URL alone, so its ws:// and wss:// connections could not carry custom headers, subprotocols or a user agent. The configuration set here is applied on every connection attempt, including automatic reconnects, with the URL taken from the address passed to `connect`; for wss://, its TLS configuration, when it has one, is used in place of the one stored with `set_tls_config` or `connect_with_tls`. A connection that follows a server redirect (`UseAnotherServer` or `ServerMoved`) does not use it, so its headers are never sent to a host the broker names. - `WebSocketConfig` implements `Clone`. - `TlsConfig::client_config`, which returns the rustls client configuration a `TlsConfig` describes. - `WebSocketConfig::build_handshake_request`, which returns the upgrade request the configuration produces. diff --git a/crates/mqtt5/src/client/inner.rs b/crates/mqtt5/src/client/inner.rs index 972c2773..b037933f 100644 --- a/crates/mqtt5/src/client/inner.rs +++ b/crates/mqtt5/src/client/inner.rs @@ -20,6 +20,8 @@ use crate::transport::{TcpTransport, TlsTransport, TransportType}; use super::connection::ConnectionEvent; use super::direct::{AutomaticReconnectLifecycle, StoredSubscription}; use super::state::ClientTransportType; +#[cfg(feature = "transport-websocket")] +use super::state::WebSocketSettings; use super::MqttClient; impl MqttClient { @@ -71,7 +73,10 @@ impl MqttClient { { let (host, port) = Self::split_host_port(rest, 80)?; Ok(( - ClientTransportType::WebSocket(address.to_string()), + ClientTransportType::WebSocket( + address.to_string(), + WebSocketSettings::Configured, + ), host, port, )) @@ -88,7 +93,10 @@ impl MqttClient { { let (host, port) = Self::split_host_port(rest, 443)?; Ok(( - ClientTransportType::WebSocketSecure(address.to_string()), + ClientTransportType::WebSocketSecure( + address.to_string(), + WebSocketSettings::Configured, + ), host, port, )) @@ -208,10 +216,13 @@ impl MqttClient { ClientTransportType::Tcp => Self::connect_tcp(addr).await, ClientTransportType::Tls => self.connect_tls(addr, host).await, #[cfg(feature = "transport-websocket")] - ClientTransportType::WebSocket(url) => self.connect_websocket(&url).await, + ClientTransportType::WebSocket(url, settings) => { + self.connect_websocket(&url, settings).await + } #[cfg(feature = "transport-websocket")] - ClientTransportType::WebSocketSecure(url) => { - self.connect_websocket_secure(addr, host, &url).await + ClientTransportType::WebSocketSecure(url, settings) => { + self.connect_websocket_secure(addr, host, &url, settings) + .await } #[cfg(feature = "transport-quic")] ClientTransportType::Quic => self.connect_quic(addr, host).await, @@ -261,10 +272,20 @@ impl MqttClient { /// The WebSocket configuration for a connection to `url`: the one set with /// `ConnectOptions::with_websocket_config`, read on every attempt so - /// reconnects use it too, or a default one. + /// reconnects use it too, or a default one, which a server redirect + /// always gets. #[cfg(feature = "transport-websocket")] - async fn websocket_config_for(&self, url: &str) -> Result { - let configured = self.inner.read().await.options.websocket_config.clone(); + async fn websocket_config_for( + &self, + url: &str, + settings: WebSocketSettings, + ) -> Result { + let configured = match settings { + WebSocketSettings::Configured => { + self.inner.read().await.options.websocket_config.clone() + } + WebSocketSettings::Default => None, + }; match configured { Some(config) => WebSocketConfig::clone(&config).with_url(url), None => WebSocketConfig::new(url), @@ -273,8 +294,12 @@ impl MqttClient { } #[cfg(feature = "transport-websocket")] - async fn connect_websocket(&self, url: &str) -> Result { - let config = self.websocket_config_for(url).await?; + async fn connect_websocket( + &self, + url: &str, + settings: WebSocketSettings, + ) -> Result { + let config = self.websocket_config_for(url, settings).await?; Self::open_websocket(config).await } @@ -284,12 +309,11 @@ impl MqttClient { addr: std::net::SocketAddr, host: &str, url: &str, + settings: WebSocketSettings, ) -> Result { let insecure = self.transport_config.read().await.insecure_tls; - let mut config = self.websocket_config_for(url).await?; + let mut config = self.websocket_config_for(url, settings).await?; - // A TLS configuration on the WebSocket configuration is used as given, - // except that insecure_tls still disables verification. if let Some(tls_config) = config.tls_config.as_mut() { if insecure { tls_config.verify_server_cert = false; @@ -491,10 +515,18 @@ impl MqttClient { } pub(crate) async fn connect_internal(&self, address: &str) -> Result { + Box::pin(self.connect_following_redirects(address, 0)).await + } + + async fn connect_following_redirects( + &self, + address: &str, + redirects_followed: u8, + ) -> Result { const MAX_REDIRECTS: u8 = 3; self.inner.read().await.options.validate_deferred_ack()?; let mut current_address = address.to_string(); - let mut redirect_count: u8 = 0; + let mut redirect_count = redirects_followed; loop { let client_id = self.inner.read().await.options.client_id.clone(); @@ -505,7 +537,12 @@ impl MqttClient { "Connection attempt" ); - let (client_transport_type, host, port) = Self::parse_address(¤t_address)?; + let (requested_transport_type, host, port) = Self::parse_address(¤t_address)?; + let client_transport_type = if redirect_count == 0 { + requested_transport_type + } else { + requested_transport_type.redirected() + }; let addrs = Self::resolve_addresses(host, port).await?; let addresses_to_try = Self::select_addresses_for_connection(&addrs, host); @@ -589,7 +626,7 @@ impl MqttClient { reason = ?reason, "Following server redirect from TLS connection" ); - return Box::pin(self.connect_internal(&url)).await; + return Box::pin(self.connect_following_redirects(&url, 1)).await; } Err(MqttError::ConnectionError( "Server redirect missing server reference".to_string(), diff --git a/crates/mqtt5/src/client/mod.rs b/crates/mqtt5/src/client/mod.rs index 72a2fa56..5e346650 100644 --- a/crates/mqtt5/src/client/mod.rs +++ b/crates/mqtt5/src/client/mod.rs @@ -1344,7 +1344,7 @@ mod tests { let (transport, host, port) = MqttClient::parse_address("ws://localhost").unwrap(); assert!(matches!( transport, - state::ClientTransportType::WebSocket(_) + state::ClientTransportType::WebSocket(..) )); assert_eq!(host, "localhost"); assert_eq!(port, 80); @@ -1352,7 +1352,7 @@ mod tests { let (transport, host, port) = MqttClient::parse_address("ws://localhost:8080").unwrap(); assert!(matches!( transport, - state::ClientTransportType::WebSocket(_) + state::ClientTransportType::WebSocket(..) )); assert_eq!(host, "localhost"); assert_eq!(port, 8080); @@ -1360,7 +1360,7 @@ mod tests { let (transport, host, port) = MqttClient::parse_address("wss://secure.broker").unwrap(); assert!(matches!( transport, - state::ClientTransportType::WebSocketSecure(_) + state::ClientTransportType::WebSocketSecure(..) )); assert_eq!(host, "secure.broker"); assert_eq!(port, 443); @@ -1369,14 +1369,14 @@ mod tests { MqttClient::parse_address("wss://secure.broker:8443").unwrap(); assert!(matches!( transport, - state::ClientTransportType::WebSocketSecure(_) + state::ClientTransportType::WebSocketSecure(..) )); assert_eq!(host, "secure.broker"); assert_eq!(port, 8443); let (transport, host, port) = MqttClient::parse_address("ws://broker.emqx.io:8083/mqtt").unwrap(); - if let state::ClientTransportType::WebSocket(url) = transport { + if let state::ClientTransportType::WebSocket(url, _) = transport { assert_eq!(url, "ws://broker.emqx.io:8083/mqtt"); } else { panic!("Expected WebSocket transport type"); @@ -1386,7 +1386,7 @@ mod tests { let (transport, host, port) = MqttClient::parse_address("wss://broker.hivemq.com:8884/mqtt").unwrap(); - if let state::ClientTransportType::WebSocketSecure(url) = transport { + if let state::ClientTransportType::WebSocketSecure(url, _) = transport { assert_eq!(url, "wss://broker.hivemq.com:8884/mqtt"); } else { panic!("Expected WebSocketSecure transport type"); diff --git a/crates/mqtt5/src/client/state.rs b/crates/mqtt5/src/client/state.rs index c8353d1a..5f2c2728 100644 --- a/crates/mqtt5/src/client/state.rs +++ b/crates/mqtt5/src/client/state.rs @@ -12,20 +12,39 @@ use super::direct::AutomaticReconnectLifecycle; use super::direct::{StoredSubscription, SubscriptionPersistence}; use super::MqttClient; +#[cfg(feature = "transport-websocket")] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum WebSocketSettings { + Configured, + Default, +} + #[derive(Debug, Clone)] pub(crate) enum ClientTransportType { Tcp, Tls, #[cfg(feature = "transport-websocket")] - WebSocket(String), + WebSocket(String, WebSocketSettings), #[cfg(feature = "transport-websocket")] - WebSocketSecure(String), + WebSocketSecure(String, WebSocketSettings), #[cfg(feature = "transport-quic")] Quic, #[cfg(feature = "transport-quic")] QuicSecure, } +impl ClientTransportType { + pub(crate) fn redirected(self) -> Self { + match self { + #[cfg(feature = "transport-websocket")] + Self::WebSocket(url, _) => Self::WebSocket(url, WebSocketSettings::Default), + #[cfg(feature = "transport-websocket")] + Self::WebSocketSecure(url, _) => Self::WebSocketSecure(url, WebSocketSettings::Default), + other => other, + } + } +} + impl MqttClient { pub(crate) async fn trigger_connection_event(&self, event: ConnectionEvent) { super::fire_connection_event(&self.connection_event_callbacks, event).await; diff --git a/crates/mqtt5/src/types.rs b/crates/mqtt5/src/types.rs index 2242fda8..16e05866 100644 --- a/crates/mqtt5/src/types.rs +++ b/crates/mqtt5/src/types.rs @@ -78,7 +78,9 @@ impl ConnectOptions { /// `connect_with_tls` (`set_insecure_tls(true)` still disables /// verification). The client connects to the address passed to `connect`, /// so the configuration's own `url` is not used; build it from that - /// address or any URL with the same scheme. + /// address or any URL with the same scheme. A connection that follows a + /// server redirect (`UseAnotherServer` or `ServerMoved`) does not use it, + /// so its headers only go to the address passed to `connect`. /// /// ```rust,no_run /// # use mqtt5::{ConnectOptions, MqttClient}; diff --git a/crates/mqtt5/tests/client_websocket_config.rs b/crates/mqtt5/tests/client_websocket_config.rs index 3a8decab..22f5b1b5 100644 --- a/crates/mqtt5/tests/client_websocket_config.rs +++ b/crates/mqtt5/tests/client_websocket_config.rs @@ -47,25 +47,25 @@ impl Callback for Capture { } } -/// Starts a ws:// server that accepts each connection's CONNECT with a -/// CONNACK and then closes it, so the client reconnects. The receiver yields +/// Starts a ws:// server that answers each connection's CONNECT with +/// `connack` and then closes it, so the client reconnects. The receiver yields /// the headers of every upgrade request. -async fn closing_mqtt_server() -> (String, mpsc::UnboundedReceiver) { +async fn closing_mqtt_server(connack: Vec) -> (String, mpsc::UnboundedReceiver) { let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); let addr = listener.local_addr().expect("addr"); let (tx, rx) = mpsc::unbounded_channel(); tokio::spawn(async move { while let Ok((stream, _)) = listener.accept().await { let capture = Capture(tx.clone()); + let connack = connack.clone(); tokio::spawn(async move { let Ok(mut ws) = tokio_tungstenite::accept_hdr_async(stream, capture).await else { return; }; - // CONNECT if !matches!(ws.next().await, Some(Ok(Message::Binary(_)))) { return; } - let _ = ws.send(Message::Binary(CONNACK.to_vec().into())).await; + let _ = ws.send(Message::Binary(connack.into())).await; tokio::time::sleep(std::time::Duration::from_millis(200)).await; }); } @@ -79,8 +79,7 @@ fn header<'a>(headers: &'a HeaderMap, name: &str) -> Option<&'a str> { #[tokio::test] async fn websocket_config_is_sent_on_connect_and_on_reconnect() { - let (url, mut requests) = closing_mqtt_server().await; - // The configuration's own URL is not used: the client connects to `url`. + let (url, mut requests) = closing_mqtt_server(CONNACK.to_vec()).await; let websocket = WebSocketConfig::new("ws://placeholder.invalid/other") .expect("config") .with_header("Authorization", "Bearer token123") @@ -127,6 +126,49 @@ async fn websocket_config_is_sent_on_connect_and_on_reconnect() { let _ = client.disconnect().await; } +fn redirect_connack(server_reference: &str) -> Vec { + let reference = server_reference.as_bytes(); + let reference_len = u16::try_from(reference.len()).expect("reference length"); + let properties_len = u8::try_from(3 + reference.len()).expect("properties length"); + let mut connack = vec![0x20, properties_len + 3, 0x00, 0x9C, properties_len, 0x1C]; + connack.extend_from_slice(&reference_len.to_be_bytes()); + connack.extend_from_slice(reference); + connack +} + +#[tokio::test] +async fn websocket_config_is_not_sent_to_a_redirect_target() { + let (target_url, mut target_requests) = closing_mqtt_server(CONNACK.to_vec()).await; + let (origin_url, mut origin_requests) = + closing_mqtt_server(redirect_connack(&target_url)).await; + let websocket = WebSocketConfig::new("ws://placeholder.invalid/other") + .expect("config") + .with_header("Authorization", "Bearer token123"); + let client = MqttClient::with_options( + ConnectOptions::new("ws-redirect-client") + .with_websocket_config(websocket) + .with_automatic_reconnect(false), + ); + + timeout(WAIT, client.connect(&origin_url)) + .await + .expect("connect timed out") + .expect("connect"); + + let origin = timeout(WAIT, origin_requests.recv()) + .await + .expect("no upgrade request for the origin") + .expect("origin server stopped"); + assert_eq!(header(&origin, "Authorization"), Some("Bearer token123")); + let target = timeout(WAIT, target_requests.recv()) + .await + .expect("no upgrade request for the redirect target") + .expect("target server stopped"); + assert_eq!(header(&target, "Authorization"), None); + + let _ = client.disconnect().await; +} + #[tokio::test] async fn websocket_config_tls_is_used_for_wss() { let (addr, server) = wss_server(server_config( @@ -136,9 +178,6 @@ async fn websocket_config_tls_is_used_for_wss() { &[], )) .await; - // No TLS configuration is stored on the client, and the platform roots do - // not include the test CA, so the handshake only succeeds if the - // WebSocket configuration's CA is used. let websocket = WebSocketConfig::new("wss://localhost/mqtt") .expect("config") .with_ca_cert_from_file(CA) @@ -149,8 +188,6 @@ async fn websocket_config_tls_is_used_for_wss() { .with_automatic_reconnect(false), ); - // The test server stops after the WebSocket handshake and never answers - // CONNECT, so only the handshake it observed is checked. let connecting = tokio::spawn(async move { let _ = client.connect(&format!("wss://{addr}/mqtt")).await; });