From 39277bf3a0e093a3ae547cf5ef74b22ffa28a877 Mon Sep 17 00:00:00 2001 From: Siavash Sameni Date: Sat, 11 Apr 2026 13:37:04 +0400 Subject: [PATCH] =?UTF-8?q?feat(hole-punching):=20advertise=20peer=20refle?= =?UTF-8?q?xive=20addrs=20in=20DirectCall=20flow=20=E2=80=94=20Phase=203?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the signal-plane plumbing for P2P direct calling: both peers now learn their own server-reflexive address (Phase 1 Reflect), include it in DirectCallOffer / DirectCallAnswer, and the relay cross-wires them into each side's CallSetup so the client knows the OTHER party's direct addr. Dual-path QUIC race is scaffolded but deferred to Phase 3.5 — this commit ships the full advertising layer so real-hardware testing can confirm the addrs flow end-to-end before adding the concurrent-connect logic. Wire protocol (wzp-proto/src/packet.rs): - DirectCallOffer gains optional `caller_reflexive_addr` - DirectCallAnswer gains optional `callee_reflexive_addr` - CallSetup gains optional `peer_direct_addr` - All #[serde(default, skip_serializing_if = "Option::is_none")] so pre-Phase-3 peers and relays stay backward compatible by construction — the new fields are elided from the JSON on the wire when None, and older clients parse the JSON ignoring any fields they don't know. - 2 new roundtrip tests (Some + None cases, old-JSON parse-back). Call registry (wzp-relay/src/call_registry.rs): - DirectCall gains caller_reflexive_addr + callee_reflexive_addr. - set_caller_reflexive_addr / set_callee_reflexive_addr setters. - 2 new unit tests: stores and returns addrs, clearing works. Relay cross-wiring (wzp-relay/src/main.rs): - On DirectCallOffer: stash the caller's addr in the registry. - On DirectCallAnswer: stash the callee's addr (only set by AcceptTrusted answers — privacy-mode leaves it None). - Send two different CallSetup messages: one to the caller with peer_direct_addr=callee_addr, and one to the callee with peer_direct_addr=caller_addr. The cross-wiring means each side gets the OTHER party's direct addr, not its own. - Logs `p2p_viable=true` when both sides advertised. Client advertising (desktop/src-tauri/src/lib.rs): - New `try_reflect_own_addr` helper that reuses the Phase 1 oneshot pattern WITHOUT holding state.signal.lock() across the await (critical: the recv loop reacquires the same mutex to fire the oneshot, so holding it would deadlock). - `place_call` queries reflect first and includes the returned addr in DirectCallOffer. Falls back to None on any failure — call still proceeds via the relay path. - `answer_call` queries reflect ONLY on AcceptTrusted so AcceptGeneric keeps the callee's IP private by design. Reject and AcceptGeneric both pass None. - recv loop's CallSetup handler destructures and forwards peer_direct_addr to the JS layer in the signal-event payload. Client scaffolding for dual-path (desktop/src-tauri/src/lib.rs + desktop/src/main.ts): - `connect` Tauri command gets a new optional `peer_direct_addr` argument. Currently LOGS the addr but still uses the relay path for the media connection — Phase 3.5 will swap in a tokio::select! race between direct dial + relay dial. Scaffolding lands here so the JS wire is stable, real-hardware testing can confirm advertising works end-to-end, and Phase 3.5 is a pure Rust change with no JS touches. - JS setup handler forwards `data.peer_direct_addr` to invoke. Back-compat with the CLI client (crates/wzp-client/src/cli.rs): - CLI test harness updated for the new fields — always passes None for both reflex addrs (no hole-punching). Also destructures peer_direct_addr: _ in its CallSetup handler. Tests (8 new, all passing): - wzp-proto: hole_punching_optional_fields_roundtrip, hole_punching_backward_compat_old_json_parses - wzp-relay call_registry: call_registry_stores_reflexive_addrs, call_registry_clearing_reflex_addr_works - wzp-relay integration: crates/wzp-relay/tests/hole_punching.rs * both_peers_advertise_reflex_addrs_cross_wire_in_setup * privacy_mode_answer_omits_callee_addr_from_setup * pre_phase3_caller_leaves_both_setups_relay_only * neither_peer_advertises_both_setups_are_relay_only Full workspace test goes from 396 → 404 passing. PRD: .taskmaster/docs/prd_hole_punching.txt Tasks: 53-60 all completed (58 = scaffolding-only; 3.5 follow-up) Next up: **Phase 3.5 — dual-path QUIC connect race**. With the advertising layer live, this becomes a focused change: on CallSetup-with-peer_direct_addr, start a server-capable dual endpoint, and tokio::select! across (direct dial, relay dial, inbound accept). Whichever QUIC handshake completes first wins, the losers drop, 2s direct timeout falls back to relay. Co-Authored-By: Claude Opus 4.6 (1M context) --- crates/wzp-client/src/cli.rs | 8 +- crates/wzp-proto/src/packet.rs | 154 +++++++++++++ crates/wzp-relay/src/call_registry.rs | 82 +++++++ crates/wzp-relay/src/main.rs | 86 +++++-- crates/wzp-relay/tests/hole_punching.rs | 288 ++++++++++++++++++++++++ desktop/src-tauri/src/lib.rs | 197 ++++++++++++++-- desktop/src/main.ts | 6 +- 7 files changed, 777 insertions(+), 44 deletions(-) create mode 100644 crates/wzp-relay/tests/hole_punching.rs diff --git a/crates/wzp-client/src/cli.rs b/crates/wzp-client/src/cli.rs index 4169487..c9140ad 100644 --- a/crates/wzp-client/src/cli.rs +++ b/crates/wzp-client/src/cli.rs @@ -770,6 +770,9 @@ async fn run_signal_mode( ephemeral_pub: [0u8; 32], // Phase 1: not used for key exchange signature: vec![], supported_profiles: vec![wzp_proto::QualityProfile::GOOD], + // CLI client doesn't attempt hole-punching; always + // relay-path. + caller_reflexive_addr: None, }).await?; } @@ -799,12 +802,15 @@ async fn run_signal_mode( ephemeral_pub: None, signature: None, chosen_profile: Some(wzp_proto::QualityProfile::GOOD), + // CLI auto-accept uses generic (privacy) mode, + // so callee addr stays hidden from the caller. + callee_reflexive_addr: None, }).await; } SignalMessage::DirectCallAnswer { call_id, accept_mode, .. } => { info!(call_id = %call_id, mode = ?accept_mode, "call answered"); } - SignalMessage::CallSetup { call_id, room, relay_addr: setup_relay } => { + SignalMessage::CallSetup { call_id, room, relay_addr: setup_relay, peer_direct_addr: _ } => { info!(call_id = %call_id, room = %room, relay = %setup_relay, "call setup — connecting to media room"); // Connect to the media room diff --git a/crates/wzp-proto/src/packet.rs b/crates/wzp-proto/src/packet.rs index 5e06b48..d443087 100644 --- a/crates/wzp-proto/src/packet.rs +++ b/crates/wzp-proto/src/packet.rs @@ -736,6 +736,15 @@ pub enum SignalMessage { signature: Vec, /// Supported quality profiles. supported_profiles: Vec, + /// Phase 3 (hole-punching): caller's own server-reflexive + /// address as learned via `SignalMessage::Reflect`. The + /// relay stashes this in its call registry and later + /// injects it into the callee's `CallSetup.peer_direct_addr` + /// so the callee can try a direct QUIC handshake to the + /// caller instead of routing media through the relay. + /// `None` means "caller doesn't want P2P, use relay only". + #[serde(default, skip_serializing_if = "Option::is_none")] + caller_reflexive_addr: Option, }, /// Callee's response to a direct call. @@ -755,6 +764,13 @@ pub enum SignalMessage { /// Chosen quality profile (present when accepting). #[serde(skip_serializing_if = "Option::is_none")] chosen_profile: Option, + /// Phase 3 (hole-punching): callee's own server-reflexive + /// address, only populated on `AcceptTrusted` — privacy-mode + /// answers leave this `None` so the callee's real IP stays + /// hidden (the whole point of `AcceptGeneric`). The relay + /// carries it opaquely into the caller's `CallSetup`. + #[serde(default, skip_serializing_if = "Option::is_none")] + callee_reflexive_addr: Option, }, /// Relay tells both parties: media room is ready. @@ -764,6 +780,17 @@ pub enum SignalMessage { room: String, /// Relay address for the QUIC media connection. relay_addr: String, + /// Phase 3 (hole-punching): the OTHER party's server-reflexive + /// address as the relay learned it from the offer/answer + /// exchange. When populated, clients attempt a direct QUIC + /// handshake to this address in parallel with the existing + /// relay path and use whichever connects first. `None` + /// means the relay path is the only option — either because + /// a peer didn't advertise its addr (Phase 1/2 relay or + /// privacy-mode answer) or because the relay decided P2P + /// wasn't viable. + #[serde(default, skip_serializing_if = "Option::is_none")] + peer_direct_addr: Option, }, /// Ringing notification (relay → caller, callee received the offer). @@ -961,6 +988,133 @@ mod tests { } } + #[test] + fn hole_punching_optional_fields_roundtrip() { + // DirectCallOffer with Some(caller_reflexive_addr) + let offer = SignalMessage::DirectCallOffer { + caller_fingerprint: "alice".into(), + caller_alias: None, + target_fingerprint: "bob".into(), + call_id: "c1".into(), + identity_pub: [0; 32], + ephemeral_pub: [0; 32], + signature: vec![], + supported_profiles: vec![], + caller_reflexive_addr: Some("192.0.2.1:4433".into()), + }; + let json = serde_json::to_string(&offer).unwrap(); + assert!( + json.contains("caller_reflexive_addr"), + "Some field must serialize: {json}" + ); + let decoded: SignalMessage = serde_json::from_str(&json).unwrap(); + match decoded { + SignalMessage::DirectCallOffer { caller_reflexive_addr, .. } => { + assert_eq!(caller_reflexive_addr.as_deref(), Some("192.0.2.1:4433")); + } + _ => panic!("wrong variant"), + } + + // DirectCallOffer with None — skip_serializing_if must + // OMIT the field from the JSON so older relays that don't + // know about caller_reflexive_addr don't see it. + let offer_none = SignalMessage::DirectCallOffer { + caller_fingerprint: "alice".into(), + caller_alias: None, + target_fingerprint: "bob".into(), + call_id: "c1".into(), + identity_pub: [0; 32], + ephemeral_pub: [0; 32], + signature: vec![], + supported_profiles: vec![], + caller_reflexive_addr: None, + }; + let json_none = serde_json::to_string(&offer_none).unwrap(); + assert!( + !json_none.contains("caller_reflexive_addr"), + "None field must NOT serialize: {json_none}" + ); + + // DirectCallAnswer with callee_reflexive_addr. + let answer = SignalMessage::DirectCallAnswer { + call_id: "c1".into(), + accept_mode: CallAcceptMode::AcceptTrusted, + identity_pub: None, + ephemeral_pub: None, + signature: None, + chosen_profile: None, + callee_reflexive_addr: Some("198.51.100.9:4433".into()), + }; + let decoded: SignalMessage = + serde_json::from_str(&serde_json::to_string(&answer).unwrap()).unwrap(); + match decoded { + SignalMessage::DirectCallAnswer { callee_reflexive_addr, .. } => { + assert_eq!( + callee_reflexive_addr.as_deref(), + Some("198.51.100.9:4433") + ); + } + _ => panic!("wrong variant"), + } + + // CallSetup with peer_direct_addr. + let setup = SignalMessage::CallSetup { + call_id: "c1".into(), + room: "call-c1".into(), + relay_addr: "203.0.113.5:4433".into(), + peer_direct_addr: Some("192.0.2.1:4433".into()), + }; + let decoded: SignalMessage = + serde_json::from_str(&serde_json::to_string(&setup).unwrap()).unwrap(); + match decoded { + SignalMessage::CallSetup { peer_direct_addr, .. } => { + assert_eq!(peer_direct_addr.as_deref(), Some("192.0.2.1:4433")); + } + _ => panic!("wrong variant"), + } + } + + #[test] + fn hole_punching_backward_compat_old_json_parses() { + // An older client/relay wouldn't include the new fields at + // all — the new code must still accept that JSON because + // of #[serde(default)] on the Option. + let old_offer_json = r#"{ + "DirectCallOffer": { + "caller_fingerprint": "alice", + "caller_alias": null, + "target_fingerprint": "bob", + "call_id": "c1", + "identity_pub": [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0], + "ephemeral_pub": [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0], + "signature": [], + "supported_profiles": [] + } + }"#; + let decoded: SignalMessage = serde_json::from_str(old_offer_json).unwrap(); + match decoded { + SignalMessage::DirectCallOffer { caller_reflexive_addr, .. } => { + assert!(caller_reflexive_addr.is_none()); + } + _ => panic!("wrong variant"), + } + + let old_setup_json = r#"{ + "CallSetup": { + "call_id": "c1", + "room": "call-c1", + "relay_addr": "203.0.113.5:4433" + } + }"#; + let decoded: SignalMessage = serde_json::from_str(old_setup_json).unwrap(); + match decoded { + SignalMessage::CallSetup { peer_direct_addr, .. } => { + assert!(peer_direct_addr.is_none()); + } + _ => panic!("wrong variant"), + } + } + #[test] fn reflect_backward_compat_with_existing_variants() { // Adding Reflect/ReflectResponse at the end of the enum must diff --git a/crates/wzp-relay/src/call_registry.rs b/crates/wzp-relay/src/call_registry.rs index 56bdc81..18a19e3 100644 --- a/crates/wzp-relay/src/call_registry.rs +++ b/crates/wzp-relay/src/call_registry.rs @@ -31,6 +31,16 @@ pub struct DirectCall { pub created_at: Instant, pub answered_at: Option, pub ended_at: Option, + /// Phase 3 (hole-punching): caller's server-reflexive address + /// as carried in the `DirectCallOffer`. The relay stashes it + /// here when the offer arrives so it can later inject it as + /// `peer_direct_addr` into the callee's `CallSetup`. + pub caller_reflexive_addr: Option, + /// Phase 3 (hole-punching): callee's server-reflexive address + /// as carried in the `DirectCallAnswer`. Only populated for + /// `AcceptTrusted` answers — privacy-mode answers leave this + /// `None`. Fed into the caller's `CallSetup.peer_direct_addr`. + pub callee_reflexive_addr: Option, } /// Registry of active direct calls. @@ -57,11 +67,31 @@ impl CallRegistry { created_at: Instant::now(), answered_at: None, ended_at: None, + caller_reflexive_addr: None, + callee_reflexive_addr: None, }; self.calls.insert(call_id.clone(), call); self.calls.get(&call_id).unwrap() } + /// Phase 3: stash the caller's server-reflexive address read + /// off a `DirectCallOffer`. Safe to call on any call state; + /// a no-op if the call doesn't exist. + pub fn set_caller_reflexive_addr(&mut self, call_id: &str, addr: Option) { + if let Some(call) = self.calls.get_mut(call_id) { + call.caller_reflexive_addr = addr; + } + } + + /// Phase 3: stash the callee's server-reflexive address read + /// off a `DirectCallAnswer`. Safe to call on any call state; + /// a no-op if the call doesn't exist. + pub fn set_callee_reflexive_addr(&mut self, call_id: &str, addr: Option) { + if let Some(call) = self.calls.get_mut(call_id) { + call.callee_reflexive_addr = addr; + } + } + /// Get a call by ID. pub fn get(&self, call_id: &str) -> Option<&DirectCall> { self.calls.get(call_id) @@ -196,4 +226,56 @@ mod tests { assert_eq!(reg.peer_fingerprint("c1", "alice"), Some("bob")); assert_eq!(reg.peer_fingerprint("c1", "bob"), Some("alice")); } + + #[test] + fn call_registry_stores_reflexive_addrs() { + let mut reg = CallRegistry::new(); + reg.create_call("c1".into(), "alice".into(), "bob".into()); + + // Default: both addrs are None. + let c = reg.get("c1").unwrap(); + assert!(c.caller_reflexive_addr.is_none()); + assert!(c.callee_reflexive_addr.is_none()); + + // Caller advertises its reflex addr via DirectCallOffer. + reg.set_caller_reflexive_addr("c1", Some("192.0.2.1:4433".into())); + assert_eq!( + reg.get("c1").unwrap().caller_reflexive_addr.as_deref(), + Some("192.0.2.1:4433") + ); + + // Callee responds with AcceptTrusted + its own reflex addr. + reg.set_callee_reflexive_addr("c1", Some("198.51.100.9:4433".into())); + assert_eq!( + reg.get("c1").unwrap().callee_reflexive_addr.as_deref(), + Some("198.51.100.9:4433") + ); + + // Both addrs are independently readable — the relay uses + // them to cross-wire peer_direct_addr in CallSetup. + let c = reg.get("c1").unwrap(); + assert_eq!( + c.caller_reflexive_addr.as_deref(), + Some("192.0.2.1:4433") + ); + assert_eq!( + c.callee_reflexive_addr.as_deref(), + Some("198.51.100.9:4433") + ); + + // Setter on an unknown call is a no-op, not a panic. + reg.set_caller_reflexive_addr("does-not-exist", Some("x".into())); + } + + #[test] + fn call_registry_clearing_reflex_addr_works() { + // Passing None to the setter must clear a previously-set value + // so callers that downgrade to privacy mode mid-flow don't + // leak a stale addr into CallSetup. + let mut reg = CallRegistry::new(); + reg.create_call("c1".into(), "alice".into(), "bob".into()); + reg.set_caller_reflexive_addr("c1", Some("192.0.2.1:4433".into())); + reg.set_caller_reflexive_addr("c1", None); + assert!(reg.get("c1").unwrap().caller_reflexive_addr.is_none()); + } } diff --git a/crates/wzp-relay/src/main.rs b/crates/wzp-relay/src/main.rs index b364ffc..5f810fc 100644 --- a/crates/wzp-relay/src/main.rs +++ b/crates/wzp-relay/src/main.rs @@ -766,9 +766,15 @@ async fn main() -> anyhow::Result<()> { match transport.recv_signal().await { Ok(Some(msg)) => { match msg { - SignalMessage::DirectCallOffer { ref target_fingerprint, ref call_id, .. } => { + SignalMessage::DirectCallOffer { + ref target_fingerprint, + ref call_id, + ref caller_reflexive_addr, + .. + } => { let target_fp = target_fingerprint.clone(); let call_id = call_id.clone(); + let caller_addr_for_registry = caller_reflexive_addr.clone(); // Check if target is online let online = { @@ -783,10 +789,15 @@ async fn main() -> anyhow::Result<()> { continue; } - // Create call in registry + // Create call in registry + stash the caller's + // reflex addr (Phase 3 hole-punching). The relay + // treats the addr as opaque — no validation. + // Injected later into the callee's CallSetup as + // peer_direct_addr. { let mut reg = call_registry.lock().await; reg.create_call(call_id.clone(), client_fp.clone(), target_fp.clone()); + reg.set_caller_reflexive_addr(&call_id, caller_addr_for_registry); } // Forward offer to callee @@ -803,9 +814,15 @@ async fn main() -> anyhow::Result<()> { }).await; } - SignalMessage::DirectCallAnswer { ref call_id, ref accept_mode, .. } => { + SignalMessage::DirectCallAnswer { + ref call_id, + ref accept_mode, + ref callee_reflexive_addr, + .. + } => { let call_id = call_id.clone(); let mode = *accept_mode; + let callee_addr_for_registry = callee_reflexive_addr.clone(); let peer_fp = { let reg = call_registry.lock().await; @@ -827,13 +844,30 @@ async fn main() -> anyhow::Result<()> { reason: wzp_proto::HangupReason::Normal, }).await; } else { - // Accept — create private room + // Accept — create private room + stash the + // callee's reflex addr if it advertised one + // (AcceptTrusted only — privacy-mode answers + // leave it None by design). Then read back + // BOTH parties' addrs so we can cross-wire + // peer_direct_addr on the CallSetups below. let room = format!("call-{call_id}"); - { + let (caller_addr, callee_addr) = { let mut reg = call_registry.lock().await; reg.set_active(&call_id, mode, room.clone()); - } - info!(call_id = %call_id, room = %room, mode = ?mode, "call accepted, creating room"); + reg.set_callee_reflexive_addr(&call_id, callee_addr_for_registry); + let call = reg.get(&call_id); + ( + call.and_then(|c| c.caller_reflexive_addr.clone()), + call.and_then(|c| c.callee_reflexive_addr.clone()), + ) + }; + info!( + call_id = %call_id, + room = %room, + ?mode, + p2p_viable = caller_addr.is_some() && callee_addr.is_some(), + "call accepted, creating room" + ); // Forward answer to caller { @@ -843,25 +877,41 @@ async fn main() -> anyhow::Result<()> { // Send CallSetup to both parties. // - // BUG FIX: the previous version of this used `addr.ip()` - // which is `connection.remote_address()` — the CLIENT'S - // IP, not the relay's. So CallSetup told both parties to - // dial the answerer's own IP, which meant the caller was - // sending QUIC Initials into the callee's client (no - // server listening there) and the callee was sending to - // itself. In both cases endpoint.connect() hung forever. + // Each party's `peer_direct_addr` carries the + // OTHER party's reflex addr so they can attempt + // a direct QUIC handshake to each other in + // parallel with the relay path (Phase 3 + // hole-punching). Both sides falling back to the + // relay path is the Phase 0 behavior, so + // emitting `None` here is always safe. // - // Use the relay's precomputed advertised address instead. + // BUG FIX (pre-Phase 3): the previous version of + // this used `addr.ip()` which is the client's + // remote address, not the relay's. Use the + // precomputed advertised address. let relay_addr_for_setup = advertised_addr_str.clone(); - let setup = SignalMessage::CallSetup { + + // peer_fp identifies the caller here (the + // fingerprint currently on the other end of this + // answer flow); client_fp identifies the callee. + // So the CALLER gets the callee's addr as its + // peer_direct_addr, and vice versa. + let setup_for_caller = SignalMessage::CallSetup { + call_id: call_id.clone(), + room: room.clone(), + relay_addr: relay_addr_for_setup.clone(), + peer_direct_addr: callee_addr.clone(), + }; + let setup_for_callee = SignalMessage::CallSetup { call_id: call_id.clone(), room: room.clone(), relay_addr: relay_addr_for_setup, + peer_direct_addr: caller_addr.clone(), }; { let hub = signal_hub.lock().await; - let _ = hub.send_to(&peer_fp, &setup).await; - let _ = hub.send_to(&client_fp, &setup).await; + let _ = hub.send_to(&peer_fp, &setup_for_caller).await; + let _ = hub.send_to(&client_fp, &setup_for_callee).await; } } } diff --git a/crates/wzp-relay/tests/hole_punching.rs b/crates/wzp-relay/tests/hole_punching.rs new file mode 100644 index 0000000..58ed6f4 --- /dev/null +++ b/crates/wzp-relay/tests/hole_punching.rs @@ -0,0 +1,288 @@ +//! Phase 3 integration tests for hole-punching advertising +//! (PRD: .taskmaster/docs/prd_hole_punching.txt). +//! +//! These verify the end-to-end protocol cross-wiring: +//! caller (places offer with caller_reflexive_addr=A) +//! → relay (stashes A in registry) +//! → callee (reads A off the forwarded offer) +//! callee (sends AcceptTrusted answer with callee_reflexive_addr=B) +//! → relay (stashes B, emits CallSetup to both parties) +//! → caller receives CallSetup.peer_direct_addr = B +//! → callee receives CallSetup.peer_direct_addr = A +//! +//! The actual QUIC hole-punch race is a Phase 3.5 follow-up. +//! These tests only cover the signal-plane plumbing — that the +//! addrs make it from each peer's offer/answer through the relay +//! cross-wiring back out in CallSetup with the peer's addr. +//! +//! We drive the call registry + a minimal routing function +//! directly instead of spinning up a full relay process — easier +//! to reason about, no real network, and what we actually want to +//! test is the cross-wiring logic, not the whole signal stack. + +use wzp_proto::{CallAcceptMode, SignalMessage}; +use wzp_relay::call_registry::CallRegistry; + +/// Helper: simulate the relay's handling of a DirectCallOffer. In +/// `wzp-relay/src/main.rs` this is the match arm that creates the +/// call in the registry and stashes the caller's reflex addr. +fn handle_offer(reg: &mut CallRegistry, offer: &SignalMessage) -> String { + match offer { + SignalMessage::DirectCallOffer { + caller_fingerprint, + target_fingerprint, + call_id, + caller_reflexive_addr, + .. + } => { + reg.create_call( + call_id.clone(), + caller_fingerprint.clone(), + target_fingerprint.clone(), + ); + reg.set_caller_reflexive_addr(call_id, caller_reflexive_addr.clone()); + call_id.clone() + } + _ => panic!("not an offer"), + } +} + +/// Helper: simulate the relay's handling of a DirectCallAnswer + +/// the subsequent CallSetup emission. Returns the two CallSetup +/// messages the relay would push: (for_caller, for_callee). +fn handle_answer_and_build_setups( + reg: &mut CallRegistry, + answer: &SignalMessage, +) -> (SignalMessage, SignalMessage) { + let (call_id, mode, callee_addr) = match answer { + SignalMessage::DirectCallAnswer { + call_id, + accept_mode, + callee_reflexive_addr, + .. + } => (call_id.clone(), *accept_mode, callee_reflexive_addr.clone()), + _ => panic!("not an answer"), + }; + + reg.set_callee_reflexive_addr(&call_id, callee_addr); + let room = format!("call-{call_id}"); + reg.set_active(&call_id, mode, room.clone()); + + let (caller_addr, callee_addr) = { + let c = reg.get(&call_id).unwrap(); + ( + c.caller_reflexive_addr.clone(), + c.callee_reflexive_addr.clone(), + ) + }; + + let setup_for_caller = SignalMessage::CallSetup { + call_id: call_id.clone(), + room: room.clone(), + relay_addr: "203.0.113.5:4433".into(), + peer_direct_addr: callee_addr, + }; + let setup_for_callee = SignalMessage::CallSetup { + call_id, + room, + relay_addr: "203.0.113.5:4433".into(), + peer_direct_addr: caller_addr, + }; + (setup_for_caller, setup_for_callee) +} + +fn mk_offer(call_id: &str, caller_reflexive_addr: Option<&str>) -> SignalMessage { + SignalMessage::DirectCallOffer { + caller_fingerprint: "alice".into(), + caller_alias: None, + target_fingerprint: "bob".into(), + call_id: call_id.into(), + identity_pub: [0; 32], + ephemeral_pub: [0; 32], + signature: vec![], + supported_profiles: vec![], + caller_reflexive_addr: caller_reflexive_addr.map(String::from), + } +} + +fn mk_answer( + call_id: &str, + mode: CallAcceptMode, + callee_reflexive_addr: Option<&str>, +) -> SignalMessage { + SignalMessage::DirectCallAnswer { + call_id: call_id.into(), + accept_mode: mode, + identity_pub: None, + ephemeral_pub: None, + signature: None, + chosen_profile: None, + callee_reflexive_addr: callee_reflexive_addr.map(String::from), + } +} + +// ----------------------------------------------------------------------- +// Test 1: both peers advertise — CallSetup cross-wires correctly +// ----------------------------------------------------------------------- + +#[test] +fn both_peers_advertise_reflex_addrs_cross_wire_in_setup() { + let mut reg = CallRegistry::new(); + + let caller_addr = "192.0.2.1:4433"; + let callee_addr = "198.51.100.9:4433"; + + let offer = mk_offer("c1", Some(caller_addr)); + let call_id = handle_offer(&mut reg, &offer); + assert_eq!(call_id, "c1"); + assert_eq!( + reg.get("c1").unwrap().caller_reflexive_addr.as_deref(), + Some(caller_addr) + ); + + let answer = mk_answer("c1", CallAcceptMode::AcceptTrusted, Some(callee_addr)); + let (setup_caller, setup_callee) = + handle_answer_and_build_setups(&mut reg, &answer); + + // The CALLER's setup should carry the CALLEE's addr as peer_direct_addr. + match setup_caller { + SignalMessage::CallSetup { peer_direct_addr, .. } => { + assert_eq!( + peer_direct_addr.as_deref(), + Some(callee_addr), + "caller's CallSetup must contain callee's addr" + ); + } + _ => panic!("wrong variant"), + } + + // The CALLEE's setup should carry the CALLER's addr. + match setup_callee { + SignalMessage::CallSetup { peer_direct_addr, .. } => { + assert_eq!( + peer_direct_addr.as_deref(), + Some(caller_addr), + "callee's CallSetup must contain caller's addr" + ); + } + _ => panic!("wrong variant"), + } +} + +// ----------------------------------------------------------------------- +// Test 2: callee uses AcceptGeneric (privacy) — no addr leaks +// ----------------------------------------------------------------------- + +#[test] +fn privacy_mode_answer_omits_callee_addr_from_setup() { + let mut reg = CallRegistry::new(); + let caller_addr = "192.0.2.1:4433"; + + handle_offer(&mut reg, &mk_offer("c2", Some(caller_addr))); + + // AcceptGeneric explicitly passes None for callee_reflexive_addr — + // the whole point is to hide the callee's IP from the caller. + let answer = mk_answer("c2", CallAcceptMode::AcceptGeneric, None); + let (setup_caller, setup_callee) = + handle_answer_and_build_setups(&mut reg, &answer); + + // CALLER should see peer_direct_addr = None (privacy preserved). + match setup_caller { + SignalMessage::CallSetup { peer_direct_addr, .. } => { + assert!( + peer_direct_addr.is_none(), + "privacy mode must not leak callee addr to caller" + ); + } + _ => panic!("wrong variant"), + } + + // CALLEE still gets the caller's addr — only the callee opted for + // privacy, the caller already volunteered its addr in the offer. + match setup_callee { + SignalMessage::CallSetup { peer_direct_addr, .. } => { + assert_eq!( + peer_direct_addr.as_deref(), + Some(caller_addr), + "callee's CallSetup should still carry caller's volunteered addr" + ); + } + _ => panic!("wrong variant"), + } +} + +// ----------------------------------------------------------------------- +// Test 3: old caller (no addr) + new callee — relay path only +// ----------------------------------------------------------------------- + +#[test] +fn pre_phase3_caller_leaves_both_setups_relay_only() { + let mut reg = CallRegistry::new(); + + // Pre-Phase-3 client doesn't know about caller_reflexive_addr + // so the field is None. + handle_offer(&mut reg, &mk_offer("c3", None)); + + // New callee advertises its addr — doesn't matter because + // without caller_reflexive_addr the caller has nothing to + // attempt a direct handshake to, so the cross-wiring should + // still leave the caller's CallSetup without peer_direct_addr. + let answer = mk_answer( + "c3", + CallAcceptMode::AcceptTrusted, + Some("198.51.100.9:4433"), + ); + let (setup_caller, setup_callee) = + handle_answer_and_build_setups(&mut reg, &answer); + + match setup_caller { + SignalMessage::CallSetup { peer_direct_addr, .. } => { + // Phase 3 relay behavior: we always inject whatever + // addrs are in the registry, regardless of who + // advertised. The caller here gets the callee's addr + // because the callee did advertise. + assert_eq!(peer_direct_addr.as_deref(), Some("198.51.100.9:4433")); + } + _ => panic!("wrong variant"), + } + + // The callee's setup has no caller addr (pre-Phase-3 offer). + match setup_callee { + SignalMessage::CallSetup { peer_direct_addr, .. } => { + assert!( + peer_direct_addr.is_none(), + "callee should see no caller addr when offer was pre-Phase-3" + ); + } + _ => panic!("wrong variant"), + } +} + +// ----------------------------------------------------------------------- +// Test 4: neither side advertises — both CallSetups fall back cleanly +// ----------------------------------------------------------------------- + +#[test] +fn neither_peer_advertises_both_setups_are_relay_only() { + let mut reg = CallRegistry::new(); + + handle_offer(&mut reg, &mk_offer("c4", None)); + let answer = mk_answer("c4", CallAcceptMode::AcceptTrusted, None); + let (setup_caller, setup_callee) = + handle_answer_and_build_setups(&mut reg, &answer); + + for (label, setup) in [("caller", setup_caller), ("callee", setup_callee)] { + match setup { + SignalMessage::CallSetup { peer_direct_addr, relay_addr, .. } => { + assert!( + peer_direct_addr.is_none(), + "{label}'s CallSetup must have no peer_direct_addr" + ); + // Relay addr is always filled — that's the fallback + // path and the existing behavior. + assert!(!relay_addr.is_empty(), "{label} relay_addr must be set"); + } + _ => panic!("wrong variant"), + } + } +} diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index 0d97fce..b8cff26 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -260,12 +260,28 @@ async fn connect( alias: String, os_aec: bool, quality: String, + // Phase 3 hole-punching: peer's server-reflexive address as + // cross-wired by the relay in CallSetup.peer_direct_addr. JS + // passes it through when present. Currently LOGGED for + // observability but not yet used to race a direct QUIC + // handshake — that's the Phase 3.5 follow-up. Passing it + // through now so real-hardware testing can confirm the + // advertising layer is delivering the addrs end to end, and so + // the JS → Rust wire is stable before we add the race logic. + #[allow(non_snake_case)] + peer_direct_addr: Option, ) -> Result { let mut engine_lock = state.engine.lock().await; if engine_lock.is_some() { return Err("already connected".into()); } + if let Some(ref addr) = peer_direct_addr { + tracing::info!(%addr, %relay, %room, "connect: peer_direct_addr supplied — hole-punching candidate logged (Phase 3.5 will race direct vs relay here)"); + } else { + tracing::info!(%relay, %room, "connect: no peer_direct_addr — relay-only path"); + } + // If we previously opened a quinn::Endpoint for the signaling connection // (direct-call path), reuse it so the media connection shares the same // UDP socket. This side-steps the Android issue where a second @@ -540,10 +556,32 @@ async fn register_signal( Ok(Some(SignalMessage::DirectCallAnswer { call_id, accept_mode, .. })) => { tracing::info!(%call_id, ?accept_mode, "signal: DirectCallAnswer (forwarded by relay)"); } - Ok(Some(SignalMessage::CallSetup { call_id, room, relay_addr })) => { - tracing::info!(%call_id, %room, %relay_addr, "signal: CallSetup — emitting setup event to JS"); - let mut sig = signal_state.lock().await; sig.signal_status = "setup".into(); - let _ = app_clone.emit("signal-event", serde_json::json!({"type":"setup","call_id":call_id,"room":room,"relay_addr":relay_addr})); + Ok(Some(SignalMessage::CallSetup { call_id, room, relay_addr, peer_direct_addr })) => { + // Phase 3: peer_direct_addr carries the OTHER party's + // reflex addr when hole-punching is viable. Forwarded + // to JS alongside the relay addr so the connect flow + // can attempt a dual-path race. `null` when either + // side didn't advertise (pre-Phase-3 peer, privacy + // mode callee, or relay policy). + tracing::info!( + %call_id, + %room, + %relay_addr, + peer_direct = ?peer_direct_addr, + "signal: CallSetup — emitting setup event to JS" + ); + let mut sig = signal_state.lock().await; + sig.signal_status = "setup".into(); + let _ = app_clone.emit( + "signal-event", + serde_json::json!({ + "type": "setup", + "call_id": call_id, + "room": room, + "relay_addr": relay_addr, + "peer_direct_addr": peer_direct_addr, + }), + ); } Ok(Some(SignalMessage::Hangup { reason })) => { tracing::info!(?reason, "signal: Hangup"); @@ -609,15 +647,48 @@ async fn place_call( target_fp: String, ) -> Result<(), String> { use wzp_proto::SignalMessage; + + // Phase 3 hole-punching: query our own reflex addr BEFORE the + // offer so we can advertise it. Best-effort — a failed reflect + // (old relay, transient error) falls back to `None` which + // means the callee's CallSetup will have peer_direct_addr=None + // and the whole call goes through the relay path unchanged. + // + // Critical: this call does its own state.signal.lock() usage and + // MUST NOT be wrapped in an outer lock, or the recv loop's + // ReflectResponse handler will deadlock on the same mutex. + let state_inner: Arc = (*state).clone(); + let own_reflex = try_reflect_own_addr(&state_inner).await.ok().flatten(); + if let Some(ref a) = own_reflex { + tracing::info!(%a, "place_call: learned own reflex addr for hole-punching advertisement"); + } else { + tracing::info!("place_call: no reflex addr available, falling back to relay-only"); + } + let sig = state.signal.lock().await; let transport = sig.transport.as_ref().ok_or("not registered")?; - let call_id = format!("{:016x}", std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos()); - tracing::info!(%call_id, %target_fp, "place_call: sending DirectCallOffer"); - transport.send_signal(&SignalMessage::DirectCallOffer { - caller_fingerprint: sig.fingerprint.clone(), caller_alias: None, target_fingerprint: target_fp.clone(), - call_id: call_id.clone(), identity_pub: [0u8; 32], ephemeral_pub: [0u8; 32], signature: vec![], - supported_profiles: vec![wzp_proto::QualityProfile::GOOD], - }).await.map_err(|e| format!("{e}"))?; + let call_id = format!( + "{:016x}", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + ); + tracing::info!(%call_id, %target_fp, reflex = ?own_reflex, "place_call: sending DirectCallOffer"); + transport + .send_signal(&SignalMessage::DirectCallOffer { + caller_fingerprint: sig.fingerprint.clone(), + caller_alias: None, + target_fingerprint: target_fp.clone(), + call_id: call_id.clone(), + identity_pub: [0u8; 32], + ephemeral_pub: [0u8; 32], + signature: vec![], + supported_profiles: vec![wzp_proto::QualityProfile::GOOD], + caller_reflexive_addr: own_reflex, + }) + .await + .map_err(|e| format!("{e}"))?; history::log(call_id, target_fp, None, history::CallDirection::Placed); let _ = app.emit("history-changed", ()); Ok(()) @@ -631,31 +702,109 @@ async fn answer_call( mode: i32, ) -> Result<(), String> { use wzp_proto::SignalMessage; + let accept_mode = match mode { + 0 => wzp_proto::CallAcceptMode::Reject, + 1 => wzp_proto::CallAcceptMode::AcceptTrusted, + _ => wzp_proto::CallAcceptMode::AcceptGeneric, + }; + + // Phase 3 hole-punching: only AcceptTrusted reveals our reflex + // addr. Privacy-mode (AcceptGeneric) and Reject explicitly do + // NOT — leaking the callee's IP back to the caller in those + // modes would defeat the entire point of AcceptGeneric. + // + // Like place_call, we MUST NOT hold state.signal.lock() across + // the reflect await or the recv loop's ReflectResponse handler + // will deadlock on the same mutex. + let own_reflex = if accept_mode == wzp_proto::CallAcceptMode::AcceptTrusted { + let state_inner: Arc = (*state).clone(); + let r = try_reflect_own_addr(&state_inner).await.ok().flatten(); + if let Some(ref a) = r { + tracing::info!(%call_id, %a, "answer_call: learned own reflex addr for AcceptTrusted"); + } else { + tracing::info!(%call_id, "answer_call: no reflex addr for AcceptTrusted, falling back to relay-only"); + } + r + } else { + // Reject / AcceptGeneric: keep the IP private. + None + }; + let sig = state.signal.lock().await; let transport = sig.transport.as_ref().ok_or_else(|| { tracing::warn!("answer_call: not registered (no transport)"); "not registered".to_string() })?; - let accept_mode = match mode { 0 => wzp_proto::CallAcceptMode::Reject, 1 => wzp_proto::CallAcceptMode::AcceptTrusted, _ => wzp_proto::CallAcceptMode::AcceptGeneric }; - tracing::info!(%call_id, ?accept_mode, "answer_call: sending DirectCallAnswer"); - transport.send_signal(&SignalMessage::DirectCallAnswer { - call_id: call_id.clone(), accept_mode, identity_pub: None, ephemeral_pub: None, signature: None, - chosen_profile: Some(wzp_proto::QualityProfile::GOOD), - }).await.map_err(|e| { - tracing::error!(%call_id, error = %e, "answer_call: send_signal failed"); - format!("{e}") - })?; + tracing::info!(%call_id, ?accept_mode, reflex = ?own_reflex, "answer_call: sending DirectCallAnswer"); + transport + .send_signal(&SignalMessage::DirectCallAnswer { + call_id: call_id.clone(), + accept_mode, + identity_pub: None, + ephemeral_pub: None, + signature: None, + chosen_profile: Some(wzp_proto::QualityProfile::GOOD), + callee_reflexive_addr: own_reflex, + }) + .await + .map_err(|e| { + tracing::error!(%call_id, error = %e, "answer_call: send_signal failed"); + format!("{e}") + })?; tracing::info!(%call_id, "answer_call: DirectCallAnswer sent successfully"); // Upgrade the pending "Missed" entry to "Received" if the user // accepted (mode != Reject). Mode 0 = Reject → leave as Missed. - if mode != 0 { - if history::mark_received_if_pending(&call_id) { - let _ = app.emit("history-changed", ()); - } + if mode != 0 && history::mark_received_if_pending(&call_id) { + let _ = app.emit("history-changed", ()); } Ok(()) } +/// Internal reflect helper shared by `get_reflected_address` and the +/// hole-punching path in `place_call` / `answer_call`. +/// +/// Must be called WITHOUT holding `state.signal.lock()` — the recv +/// loop acquires the same lock to fire the oneshot, so holding it +/// across the await would deadlock. +/// +/// Returns `Ok(Some(addr))` on success, `Ok(None)` if reflect is +/// unsupported / timed out / transport failed (caller should +/// gracefully continue with a relay-only path), or `Err` on +/// "not registered" which is a hard precondition failure. +async fn try_reflect_own_addr( + state: &Arc, +) -> Result, String> { + use wzp_proto::SignalMessage; + let (tx, rx) = tokio::sync::oneshot::channel::(); + let transport = { + let mut sig = state.signal.lock().await; + sig.pending_reflect = Some(tx); + sig.transport + .as_ref() + .ok_or_else(|| "not registered".to_string())? + .clone() + }; + if let Err(e) = transport.send_signal(&SignalMessage::Reflect).await { + let mut sig = state.signal.lock().await; + sig.pending_reflect = None; + tracing::warn!(error = %e, "try_reflect_own_addr: send_signal failed, continuing without reflex addr"); + return Ok(None); + } + match tokio::time::timeout(std::time::Duration::from_millis(1000), rx).await { + Ok(Ok(addr)) => Ok(Some(addr.to_string())), + Ok(Err(_canceled)) => { + tracing::warn!("try_reflect_own_addr: oneshot canceled"); + Ok(None) + } + Err(_elapsed) => { + let mut sig = state.signal.lock().await; + sig.pending_reflect = None; + tracing::warn!("try_reflect_own_addr: 1s timeout (pre-Phase-1 relay?)"); + Ok(None) + } + } +} + /// "STUN for QUIC" — ask the relay what our own public address looks /// like from its side of the TLS-authenticated signal connection. /// diff --git a/desktop/src/main.ts b/desktop/src/main.ts index d0f54ef..5e90bbf 100644 --- a/desktop/src/main.ts +++ b/desktop/src/main.ts @@ -1138,7 +1138,10 @@ listen("signal-event", (event: any) => { break; case "setup": callStatusText.textContent = "Connecting to media..."; - // Auto-connect to the call room + // Phase 3 hole-punching: peer_direct_addr carries the OTHER + // party's reflex addr when both sides advertised one. Forward + // to Rust connect() which currently logs it + takes the relay + // path; Phase 3.5 will race direct vs relay here. (async () => { try { await invoke("connect", { @@ -1147,6 +1150,7 @@ listen("signal-event", (event: any) => { alias: aliasInput.value, osAec: osAecCheckbox.checked, quality: loadSettings().quality || "auto", + peerDirectAddr: data.peer_direct_addr ?? null, }); showCallScreen(); } catch (e: any) {