From 6197543b60ad85c96d4f538d7ee5549118694c34 Mon Sep 17 00:00:00 2001 From: Jesus Alejandro Sanchez Davila Date: Fri, 18 Sep 2026 08:59:54 +0200 Subject: [PATCH 1/4] Implement server-side secure_tcp key exchange for hbbs The client's secure_tcp()/key_exchange() (rustdesk/rustdesk src/rendezvous_mediator.rs, src/common.rs) waits for the ID server to proactively send a signed ephemeral public key as the first message on a new TCP-mode connection, then replies with its own ephemeral key sealing a fresh symmetric key. hbbs never implemented either half: handle_listener_inner never sent anything before entering its receive loop, and handle_tcp's message dispatch had no arm for an incoming KeyExchange reply at all -- it silently fell through the catch-all `_ => {}`. This is the actual cause of "Failed to secure tcp: deadline has elapsed": any client whose ID-server connection falls back to TCP (UDP disabled, a proxy configured, or a network that effectively blocks UDP, e.g. many corporate VPNs) hangs waiting for a reply that was never coming. The vast majority of self-hosted deployments use UDP by default and never hit this path at all, which is presumably why this has gone unaddressed since it was first reported (rustdesk-server#394, opened March 2024, zero engagement) despite a working proof of concept already being posted there. This implements the missing server side, reusing hbb_common::tcp::Encrypt (already used by FramedStream elsewhere, not reimplemented here) and the existing get_server_sk-derived signing key (self.inner.sk) -- no new dependencies, no wire format changes, and no client changes needed at all since the client already correctly implements its side. - handle_listener_inner: on a non-ws TCP accept, if self.inner.sk is set, generate an ephemeral box_ keypair, sign the public half, and send it as a KeyExchange before entering the receive loop. Skipped for ws (the client's own secure_tcp_impl treats wss:// as already encrypted and never attempts this exchange) and for servers started with an arbitrary non-crypto -k string (no secret key available to sign with, same as today). - handle_tcp: new KeyExchange arm decodes the client's two-key reply (their ephemeral pubkey + a symmetric key sealed against ours) via Encrypt::decode, and installs the derived key for this connection. - All subsequent traffic on the connection is transparently encrypted/decrypted via the derived key (send_to_sink / the receive loop), including traffic sent later through a Sink stashed in tcp_punch for an out-of-band reply (e.g. RelayResponse) -- the new EncryptState (Arc>>) travels with the stashed Sink so that path stays encrypted too, rather than silently dropping back to plaintext. - Added a unit test replicating both sides of the exchange with the same public primitives the real client and server use, confirming the derived keys match and that a message actually round-trips through Encrypt::enc/dec end to end. Verified: `cargo build` (workspace) and `cargo test --lib` (8/8 passing, including the new test) both clean. Co-Authored-By: Claude Sonnet 5 --- src/rendezvous_server.rs | 224 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 208 insertions(+), 16 deletions(-) diff --git a/src/rendezvous_server.rs b/src/rendezvous_server.rs index eaf7190f9..b8100482b 100644 --- a/src/rendezvous_server.rs +++ b/src/rendezvous_server.rs @@ -16,7 +16,7 @@ use hbb_common::{ register_pk_response::Result::{TOO_FREQUENT, UUID_MISMATCH}, *, }, - tcp::FramedStream, + tcp::{Encrypt, FramedStream}, timeout, tokio::{ self, @@ -31,7 +31,7 @@ use hbb_common::{ AddrMangle, ResultType, }; use ipnetwork::Ipv4Network; -use sodiumoxide::crypto::sign; +use sodiumoxide::crypto::{box_, sign}; use std::{ collections::HashMap, net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}, @@ -50,6 +50,14 @@ enum Data { const REG_TIMEOUT: i64 = 30_000; type TcpStreamSink = SplitSink, Bytes>; type WsSink = SplitSink, tungstenite::Message>; +// Shared, per-connection secure_tcp state: `Arc>` (not a plain `Option` +// cloned around) because the SAME logical connection's crypto state must stay reachable both +// from the still-running receive loop in `handle_listener_inner` AND from a `Sink` that gets +// `.take()`n out of that loop and stashed in `tcp_punch` for a later, out-of-band send (see +// the PunchHoleRequest/RequestRelay arms in `handle_tcp`) -- cloning `Encrypt` itself would +// give the two sides independent nonce counters and desync/reuse nonces. See +// "secure_tcp key exchange" below for why this exists at all. +type EncryptState = Arc>>; enum Sink { TcpStream(TcpStreamSink), Ws(WsSink), @@ -81,7 +89,10 @@ struct Inner { #[derive(Clone)] pub struct RendezvousServer { - tcp_punch: Arc>>, + // (Sink, EncryptState): the EncryptState travels WITH the stashed sink so a later, + // out-of-band send via this same connection (e.g. a RelayResponse) still gets encrypted + // if this connection completed a secure_tcp key exchange. + tcp_punch: Arc>>, pm: PeerMap, tx: Sender, relay_servers: Arc, @@ -508,13 +519,53 @@ impl RendezvousServer { addr: SocketAddr, key: &str, ws: bool, + encrypt: &EncryptState, + ephemeral_sk: &mut Option, ) -> bool { if let Ok(msg_in) = RendezvousMessage::parse_from_bytes(bytes) { match msg_in.union { + // secure_tcp key exchange, client's reply to the KeyExchange this connection + // was proactively sent on accept (see handle_listener_inner). Byte layout + // confirmed against the client's own create_symmetric_key_msg + // (rustdesk/rustdesk src/common.rs): keys[0] is the client's own ephemeral + // box_ public key (plain), keys[1] is a symmetric key sealed with that + // ephemeral keypair against OUR ephemeral public key -- decode() below is + // exactly the inverse of that seal. This arm previously didn't exist at all, + // which is the actual root cause of "Failed to secure tcp: deadline has + // elapsed": a TCP-mode client waits for a server-initiated KeyExchange that + // vanilla hbbs never sent (https://github.com/rustdesk/rustdesk-server/issues/394). + Some(rendezvous_message::Union::KeyExchange(ex)) => { + if ex.keys.len() != 2 { + return false; + } + let Some(sk) = ephemeral_sk.take() else { + // No handshake in flight on this connection (e.g. a stray/replayed + // message, or ws -- ws clients never attempt this exchange at all + // since they treat wss:// itself as already encrypted). Ignore rather + // than error: this arm must never be reachable before we've actually + // sent our own KeyExchange first. + return true; + }; + let their_pk_b = &ex.keys[0]; + let sealed_key = &ex.keys[1]; + match Encrypt::decode(sealed_key, their_pk_b, &sk) { + Ok(symmetric_key) => { + *encrypt.lock().await = Some(Encrypt::new(symmetric_key)); + } + Err(err) => { + log::warn!("secure_tcp key exchange failed from {}: {}", addr, err); + return false; + } + } + return true; + } Some(rendezvous_message::Union::PunchHoleRequest(ph)) => { // there maybe several attempt, so sink can be none if let Some(sink) = sink.take() { - self.tcp_punch.lock().await.insert(try_into_v4(addr), sink); + self.tcp_punch + .lock() + .await + .insert(try_into_v4(addr), (sink, encrypt.clone())); } allow_err!(self.handle_tcp_punch_hole_request(addr, ph, key, ws).await); return true; @@ -522,7 +573,10 @@ impl RendezvousServer { Some(rendezvous_message::Union::RequestRelay(mut rf)) => { // there maybe several attempt, so sink can be none if let Some(sink) = sink.take() { - self.tcp_punch.lock().await.insert(try_into_v4(addr), sink); + self.tcp_punch + .lock() + .await + .insert(try_into_v4(addr), (sink, encrypt.clone())); } if let Some(peer) = self.pm.get_in_memory(&rf.id).await { let mut msg_out = RendezvousMessage::new(); @@ -572,7 +626,7 @@ impl RendezvousServer { res.cu = MessageField::from_option(Some(cu)); } msg_out.set_test_nat_response(res); - Self::send_to_sink(sink, msg_out).await; + Self::send_to_sink(sink, msg_out, encrypt).await; } Some(rendezvous_message::Union::RegisterPk(_)) => { let res = register_pk_response::Result::NOT_SUPPORT; @@ -581,7 +635,7 @@ impl RendezvousServer { result: res.into(), ..Default::default() }); - Self::send_to_sink(sink, msg_out).await; + Self::send_to_sink(sink, msg_out, encrypt).await; } _ => {} } @@ -841,16 +895,32 @@ impl RendezvousServer { #[inline] async fn send_to_tcp(&mut self, msg: RendezvousMessage, addr: SocketAddr) { - let mut tcp = self.tcp_punch.lock().await.remove(&try_into_v4(addr)); + let stashed = self.tcp_punch.lock().await.remove(&try_into_v4(addr)); + let (mut sink, encrypt) = Self::unstash(stashed); tokio::spawn(async move { - Self::send_to_sink(&mut tcp, msg).await; + Self::send_to_sink(&mut sink, msg, &encrypt).await; }); } + // tcp_punch stores (Sink, EncryptState) pairs; callers that only care about the Sink + // still need a valid EncryptState to hand to send_to_sink, so this fills in a fresh + // (never-touched, since send_to_sink only reads it when sink is Some) empty one when + // nothing was stashed at all. + #[inline] + fn unstash(stashed: Option<(Sink, EncryptState)>) -> (Option, EncryptState) { + match stashed { + Some((sink, encrypt)) => (Some(sink), encrypt), + None => (None, Arc::new(Mutex::new(None))), + } + } + #[inline] - async fn send_to_sink(sink: &mut Option, msg: RendezvousMessage) { + async fn send_to_sink(sink: &mut Option, msg: RendezvousMessage, encrypt: &EncryptState) { if let Some(sink) = sink.as_mut() { - if let Ok(bytes) = msg.write_to_bytes() { + if let Ok(mut bytes) = msg.write_to_bytes() { + if let Some(enc) = encrypt.lock().await.as_mut() { + bytes = enc.enc(&bytes); + } match sink { Sink::TcpStream(s) => { allow_err!(s.send(Bytes::from(bytes)).await); @@ -869,8 +939,9 @@ impl RendezvousServer { msg: RendezvousMessage, addr: SocketAddr, ) -> ResultType<()> { - let mut sink = self.tcp_punch.lock().await.remove(&try_into_v4(addr)); - Self::send_to_sink(&mut sink, msg).await; + let stashed = self.tcp_punch.lock().await.remove(&try_into_v4(addr)); + let (mut sink, encrypt) = Self::unstash(stashed); + Self::send_to_sink(&mut sink, msg, &encrypt).await; Ok(()) } @@ -1178,6 +1249,14 @@ impl RendezvousServer { ws: bool, ) -> ResultType<()> { let mut sink; + // secure_tcp key exchange state for this connection -- see handle_tcp's KeyExchange + // arm and the EncryptState/Encrypt doc comments above for the full design. Deliberately + // NOT set up for the ws branch: the client's own secure_tcp_impl skips this exchange + // entirely for wss:// connections (it already gets transport-layer encryption from + // WebSocket Secure), so a ws client never waits for or sends a KeyExchange -- sending + // one here would just be a wasted, ignored message. + let encrypt: EncryptState = Arc::new(Mutex::new(None)); + let mut ephemeral_sk: Option = None; if ws { use tokio_tungstenite::tungstenite::handshake::server::{Request, Response}; let callback = |req: &Request, response: Response| { @@ -1209,7 +1288,18 @@ impl RendezvousServer { sink = Some(Sink::Ws(a)); while let Ok(Some(Ok(msg))) = timeout(30_000, b.next()).await { if let tungstenite::Message::Binary(bytes) = msg { - if !self.handle_tcp(&bytes, &mut sink, addr, key, ws).await { + if !self + .handle_tcp( + &bytes, + &mut sink, + addr, + key, + ws, + &encrypt, + &mut ephemeral_sk, + ) + .await + { break; } } @@ -1217,8 +1307,50 @@ impl RendezvousServer { } else { let (a, mut b) = Framed::new(stream, BytesCodec::new()).split(); sink = Some(Sink::TcpStream(a)); - while let Ok(Some(Ok(bytes))) = timeout(30_000, b.next()).await { - if !self.handle_tcp(&bytes, &mut sink, addr, key, ws).await { + // Proactively send the server's signed ephemeral public key as the very first + // message on this connection, before waiting to receive anything -- this is the + // half of the secure_tcp handshake vanilla hbbs never implemented (see handle_tcp's + // KeyExchange arm for the other half, and issue #394 for the original PoC this is + // based on). Sent unencrypted (encrypt is still None here), matching what the + // client's own key_exchange() expects as its first message. Skipped entirely when + // self.inner.sk is None, i.e. hbbs was started with an arbitrary non-crypto -k + // string (legacy shared-key mode) rather than a real keypair/generated key -- that + // mode has no secret key to sign with, so this improvement doesn't apply to it and + // behavior there is unchanged (a TCP-mode client against such a server still times + // out exactly as before). + if let Some(sk) = self.inner.sk.as_ref() { + let (our_pk, our_sk) = box_::gen_keypair(); + let signed = sign::sign(&our_pk.0, sk); + let mut msg_out = RendezvousMessage::new(); + msg_out.set_key_exchange(KeyExchange { + keys: vec![Bytes::from(signed)], + ..Default::default() + }); + Self::send_to_sink(&mut sink, msg_out, &encrypt).await; + ephemeral_sk = Some(our_sk); + } + while let Ok(Some(Ok(mut bytes))) = timeout(30_000, b.next()).await { + { + let mut enc = encrypt.lock().await; + if let Some(enc) = enc.as_mut() { + if enc.dec(&mut bytes).is_err() { + log::warn!("secure_tcp decryption failed from {}", addr); + break; + } + } + } + if !self + .handle_tcp( + &bytes, + &mut sink, + addr, + key, + ws, + &encrypt, + &mut ephemeral_sk, + ) + .await + { break; } } @@ -1411,6 +1543,7 @@ async fn create_tcp_listener(bind_addr: Option, port: i32) -> ResultType #[cfg(test)] mod tests { use super::*; + use sodiumoxide::crypto::secretbox; #[hbb_common::tokio::test] async fn udp_listener_uses_bind_address() { @@ -1418,4 +1551,63 @@ mod tests { let socket = create_udp_listener(Some(bind_addr), 0, 0).await.unwrap(); assert_eq!(socket.local_addr().unwrap().ip(), bind_addr); } + + // Replicates both sides of the secure_tcp key exchange (server's proactive send in + // handle_listener_inner + handle_tcp's new KeyExchange arm on one side, the client's own + // key_exchange()/create_symmetric_key_msg() in rustdesk/src/common.rs on the other) using + // only the same public primitives production code calls, without needing a full + // RendezvousServer/live TCP connection. Confirms: (1) the client can verify our signed + // ephemeral public key using only the long-term public key it already trusts, exactly as + // key_exchange() does; (2) the symmetric key our decode() derives from the client's sealed + // reply matches the plain key the client actually generated and is about to use -- i.e. + // both sides really do end up sharing the same secret, not just "some" key each. + #[test] + fn secure_tcp_key_exchange_round_trip() { + // Server's long-term identity (what get_server_sk would return for a real -k value). + let (server_pk, server_sk) = sign::gen_keypair(); + + // --- Server: what handle_listener_inner now sends first on every new connection --- + let (server_eph_pk, server_eph_sk) = box_::gen_keypair(); + let signed = sign::sign(&server_eph_pk.0, &server_sk); + + // --- Client: key_exchange()'s handling of that first message --- + // get_rs_pk(key) in the real client just base64-decodes the configured Key field into + // this same sign::PublicKey; using it directly here since decoding isn't what's under + // test. + let verified = sign::verify(&signed, &server_pk).expect("client must verify our signature"); + assert_eq!( + verified, server_eph_pk.0, + "client must recover our real ephemeral pubkey" + ); + + // --- Client: create_symmetric_key_msg(their_pk_b) --- + let their_pk_b = box_::PublicKey(verified.try_into().unwrap()); + let (client_eph_pk, client_eph_sk) = box_::gen_keypair(); + let plain_key = secretbox::gen_key(); + let nonce = box_::Nonce([0u8; box_::NONCEBYTES]); + let sealed_key = box_::seal(&plain_key.0, &nonce, &their_pk_b, &client_eph_sk); + // This is exactly the two-element KeyExchange.keys the client actually sends back. + let client_reply_keys = [Vec::from(client_eph_pk.0), sealed_key]; + + // --- Server: handle_tcp's new KeyExchange arm --- + let derived = Encrypt::decode(&client_reply_keys[1], &client_reply_keys[0], &server_eph_sk) + .expect("server must decode the client's sealed reply"); + + assert_eq!( + derived, plain_key, + "server-derived symmetric key must equal the client's own plain key" + ); + + // Both sides now construct Encrypt with the same key; confirm messages actually + // round-trip end to end, not just that the raw key bytes happen to match. + let mut server_side = Encrypt::new(derived); + let mut client_side = Encrypt::new(plain_key); + let plaintext = b"RegisterPk".to_vec(); + let ciphertext = server_side.enc(&plaintext); + let mut buf = BytesMut::from(&ciphertext[..]); + client_side + .dec(&mut buf) + .expect("client must decrypt what the server encrypted"); + assert_eq!(&buf[..], &plaintext[..]); + } } From d0c40b1209a2a0e6b2cd0ce1acd185aa615e5115 Mon Sep 17 00:00:00 2001 From: Jesus Alejandro Sanchez Davila Date: Fri, 18 Sep 2026 11:21:15 +0200 Subject: [PATCH 2/4] Trim redundant comments duplicating the same rationale in multiple spots The EncryptState-sharing rationale and the "issue #394 missing half" rationale were each explained in full at two/three separate call sites. Kept the fullest explanation at its natural definition site and reduced the others to short pointers. --- src/rendezvous_server.rs | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/rendezvous_server.rs b/src/rendezvous_server.rs index b8100482b..844e0f2cf 100644 --- a/src/rendezvous_server.rs +++ b/src/rendezvous_server.rs @@ -89,9 +89,8 @@ struct Inner { #[derive(Clone)] pub struct RendezvousServer { - // (Sink, EncryptState): the EncryptState travels WITH the stashed sink so a later, - // out-of-band send via this same connection (e.g. a RelayResponse) still gets encrypted - // if this connection completed a secure_tcp key exchange. + // (Sink, EncryptState) -- see EncryptState's own doc comment above for why the two travel + // together. tcp_punch: Arc>>, pm: PeerMap, tx: Sender, @@ -1307,17 +1306,11 @@ impl RendezvousServer { } else { let (a, mut b) = Framed::new(stream, BytesCodec::new()).split(); sink = Some(Sink::TcpStream(a)); - // Proactively send the server's signed ephemeral public key as the very first - // message on this connection, before waiting to receive anything -- this is the - // half of the secure_tcp handshake vanilla hbbs never implemented (see handle_tcp's - // KeyExchange arm for the other half, and issue #394 for the original PoC this is - // based on). Sent unencrypted (encrypt is still None here), matching what the - // client's own key_exchange() expects as its first message. Skipped entirely when - // self.inner.sk is None, i.e. hbbs was started with an arbitrary non-crypto -k - // string (legacy shared-key mode) rather than a real keypair/generated key -- that - // mode has no secret key to sign with, so this improvement doesn't apply to it and - // behavior there is unchanged (a TCP-mode client against such a server still times - // out exactly as before). + // Proactively send our half of the exchange (see handle_tcp's KeyExchange arm) + // before waiting to receive anything, unencrypted, matching what the client's own + // key_exchange() expects as its first message. Skipped when self.inner.sk is None, + // i.e. hbbs was started with an arbitrary non-crypto -k string (legacy shared-key + // mode, nothing to sign with) -- that mode's behavior is unchanged. if let Some(sk) = self.inner.sk.as_ref() { let (our_pk, our_sk) = box_::gen_keypair(); let signed = sign::sign(&our_pk.0, sk); From 5e72dcd0c0856857686ae6b312c232c06d03180d Mon Sep 17 00:00:00 2001 From: Jesus Alejandro Sanchez Davila Date: Mon, 21 Sep 2026 09:18:35 +0200 Subject: [PATCH 3/4] Add a TCP-level test for the secure_tcp handshake and stashed-sink reply Addresses a review comment on this PR: the existing secure_tcp_key_exchange_round_trip test replicates the crypto exchange directly but never exercises handle_listener_inner, handle_tcp, the plaintext-to-encrypted transition, or the stashed-sink response path (handle_tcp_punch_hole_request -> send_to_tcp_sync) - a regression in that wiring could still pass the existing test. secure_tcp_production_handshake_and_stashed_sink_response drives all of that over a real TCP socket: binds a listener, runs handle_listener_inner on an accepted connection, completes the actual handshake as a real client would, then sends an encrypted PunchHoleRequest for a nonexistent id to force handle_tcp's PunchHoleRequest arm to stash (sink, encrypt) in tcp_punch and immediately retrieve it again via send_to_tcp_sync, verifying the encrypted PunchHoleResponse{ID_NOT_EXIST} that comes back through it. Also asserts tcp_punch ends up empty, confirming the stash was actually exercised and not just some other reply path. Wire framing note: hbb_common's bytes_codec::BytesCodec (not tokio_util's identically-named, unrelated codec) is a real length-prefixed framing format (1-4 byte variable header). The test's encode_frame/ read_frame helpers replicate it so a plain TcpStream can speak the same protocol real clients use. Verified: cargo test --lib (9/9 passing, including this new test run standalone 5x and as part of the full suite), rustfmt --check confirms no new formatting drift beyond what already existed in this file before this change. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012icUrjv3vgHtovVpHUFjdp --- src/rendezvous_server.rs | 216 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 216 insertions(+) diff --git a/src/rendezvous_server.rs b/src/rendezvous_server.rs index 844e0f2cf..441d0f6b1 100644 --- a/src/rendezvous_server.rs +++ b/src/rendezvous_server.rs @@ -1603,4 +1603,220 @@ mod tests { .expect("client must decrypt what the server encrypted"); assert_eq!(&buf[..], &plaintext[..]); } + + // The wire framing `Framed` (hbb_common's own bytes_codec, NOT + // tokio_util's raw pass-through codec of the same name) actually uses: a 1-4 byte + // variable-length header (`(len << 2) | size_class`, little-endian, size_class picked by + // how many bytes `len` needs) followed by that many payload bytes. Test helpers below + // replicate encode()/decode() exactly so a plain TcpStream (no Framed wrapper) can speak + // the same protocol handle_listener_inner's real client connections use. + fn encode_frame(payload: &[u8]) -> Vec { + let len = payload.len(); + let mut out = Vec::with_capacity(len + 4); + if len <= 0x3F { + out.push((len << 2) as u8); + } else if len <= 0x3FFF { + out.extend_from_slice(&(((len << 2) as u16) | 0x1).to_le_bytes()); + } else if len <= 0x3FFFFF { + let h = ((len << 2) as u32) | 0x2; + out.extend_from_slice(&h.to_le_bytes()[..3]); + } else { + out.extend_from_slice(&(((len << 2) as u32) | 0x3).to_le_bytes()); + } + out.extend_from_slice(payload); + out + } + + async fn read_frame(stream: &mut TcpStream) -> Vec { + let mut first = [0u8; 1]; + stream + .read_exact(&mut first) + .await + .expect("must read the frame header's first byte"); + let head_len = ((first[0] & 0x3) + 1) as usize; + let mut head = vec![0u8; head_len]; + head[0] = first[0]; + if head_len > 1 { + stream + .read_exact(&mut head[1..]) + .await + .expect("must read the remaining frame header bytes"); + } + let mut n = head[0] as usize; + if head_len > 1 { + n |= (head[1] as usize) << 8; + } + if head_len > 2 { + n |= (head[2] as usize) << 16; + } + if head_len > 3 { + n |= (head[3] as usize) << 24; + } + n >>= 2; + let mut payload = vec![0u8; n]; + stream + .read_exact(&mut payload) + .await + .expect("must read the full frame payload"); + payload + } + + // Addresses the gap the round-trip test above deliberately doesn't cover: this drives the + // REAL production code path over an actual TCP socket -- handle_listener_inner's proactive + // send, handle_tcp's KeyExchange arm, the main loop's plaintext-to-encrypted transition + // (the `enc.dec(&mut bytes)` step in handle_listener_inner), and handle_tcp's + // PunchHoleRequest arm, which stashes (sink, encrypt) into tcp_punch and then immediately + // calls send_to_tcp_sync to retrieve that exact stashed pair and send an encrypted + // response back through it. A regression in any of that wiring (frame ordering, the wrong + // Encrypt instance ending up in the sink, etc.) would fail here even though the isolated + // crypto primitives above would still pass. + #[hbb_common::tokio::test] + async fn secure_tcp_production_handshake_and_stashed_sink_response() { + // PeerMap::new() has no way to select its DB backend other than this env var (its + // `map` field is private to peer.rs, so PeerMap can't be constructed directly from + // here) -- and Database::new() unconditionally pre-creates a literal file matching + // whatever string it's given (see database.rs), so "sqlite::memory:" doesn't actually + // avoid an on-disk artifact here the way it would with a bare sqlx connection. Route it + // through the OS temp dir instead and clean up explicitly at the end, rather than + // leaving a stray file in the crate's own working directory. + let db_path = std::env::temp_dir().join(format!( + "hbbs_secure_tcp_test_{}.sqlite3", + std::process::id() + )); + std::env::set_var("DB_URL", db_path.to_str().unwrap()); + + let (server_pk, server_sk) = sign::gen_keypair(); + let pm = PeerMap::new() + .await + .expect("PeerMap::new with an in-memory DB must succeed"); + let (tx, _rx) = mpsc::unbounded_channel::(); + let mut server = RendezvousServer { + tcp_punch: Arc::new(Mutex::new(HashMap::new())), + pm, + tx, + relay_servers: Default::default(), + relay_servers0: Default::default(), + rendezvous_servers: Arc::new(Vec::new()), + inner: Arc::new(Inner { + serial: 0, + version: String::new(), + software_url: String::new(), + mask: None, + local_ip: String::new(), + sk: Some(server_sk), + }), + }; + + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("must bind a local test listener"); + let server_addr = listener.local_addr().unwrap(); + + // handle_listener_inner takes &mut self and only returns when the connection closes, + // so it runs on a clone (tcp_punch is Arc-wrapped, so writes from this task are still + // visible through the original `server` handle kept in this test). + let mut server_conn = server.clone(); + tokio::spawn(async move { + let (stream, addr) = listener + .accept() + .await + .expect("must accept the test client"); + allow_err!( + server_conn + .handle_listener_inner(stream, addr, "", false) + .await + ); + }); + + let mut client = TcpStream::connect(server_addr) + .await + .expect("must connect to the test listener"); + + // --- Read the server's proactive, unencrypted KeyExchange (handle_listener_inner) --- + let frame = read_frame(&mut client).await; + let msg_in = RendezvousMessage::parse_from_bytes(&frame) + .expect("server's first message must be a valid RendezvousMessage"); + let Some(rendezvous_message::Union::KeyExchange(ex)) = msg_in.union else { + panic!( + "server's first message must be a KeyExchange, got {:?}", + msg_in.union + ); + }; + assert_eq!( + ex.keys.len(), + 1, + "server's KeyExchange must carry exactly its signed ephemeral pubkey" + ); + let server_eph_pk_bytes = sign::verify(&ex.keys[0], &server_pk) + .expect("client must be able to verify the server's signature with its trusted long-term pubkey"); + let server_eph_pk = box_::PublicKey( + server_eph_pk_bytes + .try_into() + .expect("verified payload must be exactly one box_ public key"), + ); + + // --- Client's reply, exactly as key_exchange()/create_symmetric_key_msg() build it --- + let (client_eph_pk, client_eph_sk) = box_::gen_keypair(); + let plain_key = secretbox::gen_key(); + let nonce = box_::Nonce([0u8; box_::NONCEBYTES]); + let sealed_key = box_::seal(&plain_key.0, &nonce, &server_eph_pk, &client_eph_sk); + let mut reply = RendezvousMessage::new(); + reply.set_key_exchange(KeyExchange { + keys: vec![ + Bytes::from(Vec::from(client_eph_pk.0)), + Bytes::from(sealed_key), + ], + ..Default::default() + }); + client + .write_all(&encode_frame(&reply.write_to_bytes().unwrap())) + .await + .expect("must send the client's KeyExchange reply"); + + // --- Encrypted PunchHoleRequest for a nonexistent id: exercises the plaintext-to- + // encrypted transition, handle_tcp's PunchHoleRequest arm, and the stashed-sink + // response path (handle_tcp_punch_hole_request -> send_to_tcp_sync) end to end --- + let mut client_crypto = Encrypt::new(plain_key); + let mut req = RendezvousMessage::new(); + req.set_punch_hole_request(PunchHoleRequest { + id: "secure-tcp-test-nonexistent-id".to_owned(), + ..Default::default() + }); + let ciphertext = client_crypto.enc(&req.write_to_bytes().unwrap()); + client + .write_all(&encode_frame(&ciphertext)) + .await + .expect("must send the encrypted PunchHoleRequest"); + + let frame = read_frame(&mut client).await; + let mut resp_bytes = BytesMut::from(&frame[..]); + client_crypto + .dec(&mut resp_bytes) + .expect("client must decrypt the response sent back through the stashed sink"); + let msg_resp = RendezvousMessage::parse_from_bytes(&resp_bytes) + .expect("decrypted response must be a valid RendezvousMessage"); + match msg_resp.union { + Some(rendezvous_message::Union::PunchHoleResponse(r)) => { + assert_eq!( + r.failure, + punch_hole_response::Failure::ID_NOT_EXIST.into(), + "nonexistent id must produce ID_NOT_EXIST, not some other response" + ); + } + other => panic!("expected an encrypted PunchHoleResponse, got {:?}", other), + } + + // The sink was taken out of the connection loop and stashed in tcp_punch by + // send_to_tcp_sync's own `.remove(...)` -- confirms this test actually exercised the + // stash-then-retrieve path, not some other reply mechanism that happened to look the + // same from the client's side. + assert!( + server.tcp_punch.lock().await.is_empty(), + "the stashed (sink, encrypt) pair must have been removed by send_to_tcp_sync" + ); + + let _ = std::fs::remove_file(&db_path); + let _ = std::fs::remove_file(format!("{}-shm", db_path.display())); + let _ = std::fs::remove_file(format!("{}-wal", db_path.display())); + } } From 2995573497751c162bcd511e7e231bbb825aa43c Mon Sep 17 00:00:00 2001 From: Jesus Alejandro Sanchez Davila Date: Mon, 21 Sep 2026 09:35:59 +0200 Subject: [PATCH 4/4] Avoid mutating process-global DB_URL in the new secure_tcp test CodeRabbit correctly flagged that std::env::set_var("DB_URL", ...) in the new TCP-level test had no restore/serialization - since cargo test runs tests in parallel by default within one binary, this risked another test observing this test's DB path (or vice versa) with no isolation. Adds PeerMap::new_with_db_url (test-only, peer.rs) that constructs a PeerMap from an explicit path, bypassing DB_URL/get_arg_opt entirely - the cleanest of the two alternatives the review offered, since it removes the race possibility rather than just narrowing its window with a guard. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012icUrjv3vgHtovVpHUFjdp --- src/peer.rs | 12 ++++++++++++ src/rendezvous_server.rs | 20 ++++++++++---------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/peer.rs b/src/peer.rs index 0f218dfdc..cfcfdd1dc 100644 --- a/src/peer.rs +++ b/src/peer.rs @@ -89,6 +89,18 @@ impl PeerMap { Ok(pm) } + // Test-only: construct with an explicit DB path, bypassing DB_URL/get_arg_opt entirely. + // A test that instead did `std::env::set_var("DB_URL", ...)` before calling `new()` would + // be mutating process-global state with no isolation from other tests running in the same + // (by default parallel) test binary -- this constructor exists so tests never need to. + #[cfg(test)] + pub(crate) async fn new_with_db_url(url: &str) -> ResultType { + Ok(Self { + map: Default::default(), + db: database::Database::new(url).await?, + }) + } + #[inline] pub(crate) async fn update_pk( &mut self, diff --git a/src/rendezvous_server.rs b/src/rendezvous_server.rs index 441d0f6b1..c60dc935f 100644 --- a/src/rendezvous_server.rs +++ b/src/rendezvous_server.rs @@ -1672,23 +1672,23 @@ mod tests { // crypto primitives above would still pass. #[hbb_common::tokio::test] async fn secure_tcp_production_handshake_and_stashed_sink_response() { - // PeerMap::new() has no way to select its DB backend other than this env var (its - // `map` field is private to peer.rs, so PeerMap can't be constructed directly from - // here) -- and Database::new() unconditionally pre-creates a literal file matching - // whatever string it's given (see database.rs), so "sqlite::memory:" doesn't actually - // avoid an on-disk artifact here the way it would with a bare sqlx connection. Route it - // through the OS temp dir instead and clean up explicitly at the end, rather than - // leaving a stray file in the crate's own working directory. + // PeerMap::new_with_db_url (test-only, see peer.rs) instead of PeerMap::new() + + // std::env::set_var("DB_URL", ...): DB_URL is process-global, and cargo test runs + // tests in parallel by default within one binary, so mutating it here with no + // restore/serialization would risk another test observing this test's DB path (or + // vice versa). Database::new() also unconditionally pre-creates a literal file + // matching whatever path it's given (see database.rs), so route it through the OS + // temp dir and clean up explicitly at the end, rather than leaving a stray file in + // the crate's own working directory. let db_path = std::env::temp_dir().join(format!( "hbbs_secure_tcp_test_{}.sqlite3", std::process::id() )); - std::env::set_var("DB_URL", db_path.to_str().unwrap()); let (server_pk, server_sk) = sign::gen_keypair(); - let pm = PeerMap::new() + let pm = PeerMap::new_with_db_url(db_path.to_str().unwrap()) .await - .expect("PeerMap::new with an in-memory DB must succeed"); + .expect("PeerMap::new_with_db_url must succeed"); let (tx, _rx) = mpsc::unbounded_channel::(); let mut server = RendezvousServer { tcp_punch: Arc::new(Mutex::new(HashMap::new())),