From 4d87c01e1a1b173c28348e7e799a0b936d1201ab Mon Sep 17 00:00:00 2001 From: Repin Agent Date: Fri, 17 Jul 2026 03:19:07 -0600 Subject: [PATCH 1/2] feat(sip): NAT-scoped external signaling address + port on Via/Contact/From (M6 CP2, New-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit external_signaling_address was parsed but never applied to any Via/Contact/From builder. Apply it — plus a new external_signaling_port override (New-3) — to every signaling builder (build_invite/build_200_ok/build_ack/build_bye/ build_reinvite/build_reinvite_ack, and the event_handler UPDATE/re-INVITE 200 OK Contacts), transport-scoped by local_net exactly like advertised_media_ip does for SDP: a peer outside local_net sees the external address AND external port; a peer inside local_net sees the internal bind. This lets a NAT/forward map an external port to a different internal bind port without breaking the first in-dialog request (which targets the advertised Contact). New advertised_signaling_hostport(local, remote) in sdp/mod.rs mirrors advertised_media_ip's transport-coverage + local_net logic and brackets IPv6 literals. SipSession::signaling_hostport() is the per-session entry point. Receiver-side proof (e2e_signaling_address.rs): a peer establishes a call and CAPTURES rustisk's 200 OK Contact and rtptimeout BYE Via+From datagrams, asserting external addr:port for a non-local peer and internal bind for a local_net peer. RED: force signaling_hostport to the internal bind -> the external peer sees 127.0.0.1 not 203.0.113.99. Plus 7 unit tests on the scoping core (port override, local_net bypass, IPv6 bracketing, FQDN, foreign-transport no-donate). --- .../tests/e2e_media_address.rs | 1 + .../tests/e2e_signaling_address.rs | 254 ++++++++++++++++++ crates/asterisk-sip/src/event_handler.rs | 26 +- crates/asterisk-sip/src/pjsip_config.rs | 7 + crates/asterisk-sip/src/sdp/mod.rs | 207 ++++++++++++++ crates/asterisk-sip/src/session/mod.rs | 43 ++- 6 files changed, 521 insertions(+), 17 deletions(-) create mode 100644 crates/asterisk-integration-tests/tests/e2e_signaling_address.rs diff --git a/crates/asterisk-integration-tests/tests/e2e_media_address.rs b/crates/asterisk-integration-tests/tests/e2e_media_address.rs index 6751aad..f989be2 100644 --- a/crates/asterisk-integration-tests/tests/e2e_media_address.rs +++ b/crates/asterisk-integration-tests/tests/e2e_media_address.rs @@ -87,6 +87,7 @@ fn endpoint_only_config(external: Option<&str>, local_net: Vec) -> Pjsip bind: "0.0.0.0:5060".parse().unwrap(), external_media_address: external.map(|s| s.to_string()), external_signaling_address: None, + external_signaling_port: None, cert_file: None, priv_key_file: None, local_net, diff --git a/crates/asterisk-integration-tests/tests/e2e_signaling_address.rs b/crates/asterisk-integration-tests/tests/e2e_signaling_address.rs new file mode 100644 index 0000000..b6734fd --- /dev/null +++ b/crates/asterisk-integration-tests/tests/e2e_signaling_address.rs @@ -0,0 +1,254 @@ +//! End-to-end acceptance for M6 CP2: `external_signaling_address` + +//! `external_signaling_port` (New-3) applied to the Via/Contact/From builders, +//! transport-scoped by `local_net` exactly like `advertised_media_ip` is for +//! SDP. +//! +//! Receiver-side proof: a SIP peer establishes a call and CAPTURES real +//! datagrams from rustisk. It inspects: +//! * the INVITE 200 OK **Contact** (built by `SipSession::build_200_ok`), and +//! * the rtptimeout **BYE**'s **Via sent-by AND From URI** (built by +//! `SipSession::build_bye`), +//! asserting they carry the EXTERNAL address AND the EXTERNAL port for a peer +//! outside `local_net`, and the INTERNAL bind address/port for a peer inside +//! `local_net`. The BYE is still physically delivered to the peer's real +//! transport address, so the advertised (external) address is proven at the +//! receiver, independent of where the datagram was sent. +//! +//! RED control (captured in the PR body): defeat the scoping — +//! `SipSession::signaling_hostport` always returns the internal bind — and the +//! external peer sees the internal address/port -> every EXTERNAL assertion +//! goes RED. + +use std::net::SocketAddr; +use std::sync::Arc; +use std::time::{Duration, Instant}; + +use asterisk_apps::adapter::register_all_apps; +use asterisk_codecs::codecs; +use asterisk_core::channel::tech_registry::TECH_REGISTRY; +use asterisk_core::pbx::{Context, Dialplan, Extension, Priority}; +use asterisk_sip::channel_driver::SipChannelDriver; +use asterisk_sip::event_handler::SipEventHandler; +use asterisk_sip::parser::{SipMessage, SipMethod, SipUri, StartLine}; +use asterisk_sip::pjsip_config::{ + set_global_pjsip_config, EndpointConfig, PjsipConfig, TransportConfig, +}; +use asterisk_sip::sdp::SessionDescription; +use asterisk_sip::session::SipSession; +use asterisk_sip::transport::UdpTransport; +use tokio::net::UdpSocket; + +const EXTEN: &str = "100"; +const EXT_ADDR: &str = "203.0.113.99"; +const EXT_PORT: u16 = 6666; + +async fn recv_sip(sock: &UdpSocket, timeout: Duration) -> Option { + let mut buf = [0u8; 4096]; + let (len, _src) = tokio::time::timeout(timeout, sock.recv_from(&mut buf)) + .await + .ok()? + .ok()?; + SipMessage::parse(&buf[..len]).ok() +} + +async fn recv_sip_status(sock: &UdpSocket, status: u16, budget: Duration) -> Option { + let deadline = Instant::now() + budget; + while Instant::now() < deadline { + if let Some(msg) = recv_sip(sock, Duration::from_millis(300)).await { + if msg.status_code() == Some(status) { + return Some(msg); + } + } + } + None +} + +async fn recv_bye(sock: &UdpSocket, budget: Duration) -> Option { + let deadline = Instant::now() + budget; + while Instant::now() < deadline { + if let Some(msg) = recv_sip(sock, Duration::from_millis(300)).await { + if msg.method() == Some(SipMethod::Bye) { + if let StartLine::Request(_) = &msg.start_line { + return Some(msg); + } + } + } + } + None +} + +fn invite_request(call_id: &str, contact_port: u16, sdp: &str) -> SipMessage { + let raw = format!( + "INVITE sip:{EXTEN}@127.0.0.1 SIP/2.0\r\n\ + Via: SIP/2.0/UDP 127.0.0.1:{contact_port};branch=z9hG4bK{call_id}inv\r\n\ + From: \"Caller\" ;tag=caller{call_id}\r\n\ + To: \r\n\ + Call-ID: {call_id}\r\n\ + CSeq: 1 INVITE\r\n\ + Contact: \r\n\ + Content-Type: application/sdp\r\n\ + Content-Length: {len}\r\n\ + \r\n\ + {sdp}", + len = sdp.len() + ); + SipMessage::parse(raw.as_bytes()).unwrap() +} + +/// (host, port) from a ``-style header value. +fn uri_hostport(value: &str) -> (String, Option) { + let uri = asterisk_sip::parser::extract_uri(value) + .and_then(|u| SipUri::parse(&u).ok()) + .expect("header must carry a parseable URI"); + (uri.host, uri.port) +} + +/// (host, port) from a `SIP/2.0/UDP host:port;branch=...` Via value. +fn via_hostport(value: &str) -> (String, Option) { + let sent_by = value + .split_whitespace() + .nth(1) + .expect("Via must have a sent-by") + .split(';') + .next() + .unwrap(); + match sent_by.rsplit_once(':') { + Some((h, p)) => (h.to_string(), p.parse::().ok()), + None => (sent_by.to_string(), None), + } +} + +fn transport_config(local_net: Vec) -> PjsipConfig { + PjsipConfig { + endpoints: vec![EndpointConfig { + name: EXTEN.to_string(), + context: "default".to_string(), + auth: None, + ..Default::default() + }], + transports: vec![TransportConfig { + name: "transport-udp".to_string(), + protocol: "udp".to_string(), + // Concrete loopback bind; the handler's 127.0.0.1 ephemeral bind is + // matched by the exact-ip fallback in the config lookup. + bind: "127.0.0.1:5060".parse().unwrap(), + external_media_address: None, + external_signaling_address: Some(EXT_ADDR.to_string()), + external_signaling_port: Some(EXT_PORT), + cert_file: None, + priv_key_file: None, + local_net, + }], + ..Default::default() + } +} + +fn dialplan() -> Dialplan { + let mut dp = Dialplan::new(); + let mut ctx = Context::new("default"); + let mut ext = Extension::new(EXTEN); + ext.add_priority(Priority { + priority: 1, + app: "Answer".to_string(), + app_data: String::new(), + label: None, + }); + ext.add_priority(Priority { + priority: 2, + app: "Echo".to_string(), + app_data: String::new(), + label: None, + }); + ctx.add_extension(ext); + dp.add_context(ctx); + dp +} + +/// Establish a media-silent inbound call, capture its 200 OK and the subsequent +/// rtptimeout BYE. Returns (ok, bye). +async fn call_and_reap( + handler: &Arc, + sip_local: SocketAddr, + caller_sip: &UdpSocket, + caller_addr: SocketAddr, + call_id: &str, +) -> (SipMessage, SipMessage) { + let caller_rtp = UdpSocket::bind("127.0.0.1:0").await.unwrap(); + let caller_rtp_addr = caller_rtp.local_addr().unwrap(); + let offer = SessionDescription::create_offer( + &caller_rtp_addr.ip().to_string(), + caller_rtp_addr.port(), + &[codecs::pcmu()], + ); + let invite = invite_request(call_id, caller_addr.port(), &offer.to_string()); + let session = SipSession::new_inbound(&invite, sip_local, caller_addr).expect("session"); + handler + .handle_incoming_invite(&invite, caller_addr, session) + .await; + let ok = recv_sip_status(caller_sip, 200, Duration::from_secs(5)) + .await + .expect("200 OK for INVITE"); + std::mem::forget(caller_rtp); + let bye = recv_bye(caller_sip, Duration::from_secs(6)) + .await + .expect("rtptimeout must reap the silent call and send a BYE"); + (ok, bye) +} + +#[tokio::test] +async fn external_signaling_address_and_port_scoped_by_local_net() { + register_all_apps(); + + let handler_transport: Arc = Arc::new( + UdpTransport::bind("127.0.0.1:0".parse().unwrap()) + .await + .unwrap(), + ); + let sip_local: SocketAddr = handler_transport.local_addr().unwrap(); + let driver = Arc::new(SipChannelDriver::new(sip_local)); + driver.set_transport(handler_transport.clone()); + TECH_REGISTRY.register(driver.clone()); + let handler = Arc::new(SipEventHandler::new(Arc::new(dialplan()), handler_transport)); + handler.set_channel_driver(driver.clone()); + handler.set_rtp_timeout(Some(Duration::from_secs(2))); + + let caller_sip = UdpSocket::bind("127.0.0.1:0").await.unwrap(); + let caller_addr = caller_sip.local_addr().unwrap(); + + // ---- EXTERNAL: caller outside local_net -> external addr + port -------- + set_global_pjsip_config(transport_config(vec![])); + let (ok, bye) = call_and_reap(&handler, sip_local, &caller_sip, caller_addr, "sig-ext").await; + + let (c_host, c_port) = uri_hostport(ok.get_header("Contact").expect("200 OK Contact")); + assert_eq!(c_host, EXT_ADDR, "external peer must see the external signaling ADDRESS in Contact"); + assert_eq!(c_port, Some(EXT_PORT), "external peer must see the external signaling PORT in Contact (New-3)"); + + let (v_host, v_port) = via_hostport(bye.get_header("Via").expect("BYE Via")); + assert_eq!(v_host, EXT_ADDR, "external peer must see the external address in the BYE Via sent-by"); + assert_eq!(v_port, Some(EXT_PORT), "external peer must see the external port in the BYE Via sent-by"); + + let (f_host, f_port) = uri_hostport(bye.get_header("From").expect("BYE From")); + assert_eq!(f_host, EXT_ADDR, "external peer must see the external address in the BYE From URI"); + assert_eq!(f_port, Some(EXT_PORT), "external peer must see the external port in the BYE From URI"); + println!("[E2E] external peer: Contact/Via/From carry {EXT_ADDR}:{EXT_PORT}"); + + // ---- INTERNAL: caller inside local_net -> internal bind addr/port ------ + set_global_pjsip_config(transport_config(vec!["127.0.0.0/8".to_string()])); + let (ok, bye) = call_and_reap(&handler, sip_local, &caller_sip, caller_addr, "sig-int").await; + + let (c_host, c_port) = uri_hostport(ok.get_header("Contact").expect("200 OK Contact")); + assert_eq!(c_host, "127.0.0.1", "local_net peer must see the internal bind address, not the external one"); + assert_eq!( + c_port, + Some(sip_local.port()), + "local_net peer must see the internal bind port, not the external override" + ); + assert_ne!(c_port, Some(EXT_PORT), "the external port override must NOT reach a local_net peer"); + + let (v_host, v_port) = via_hostport(bye.get_header("Via").expect("BYE Via")); + assert_eq!(v_host, "127.0.0.1", "local_net peer must see the internal address in the BYE Via"); + assert_eq!(v_port, Some(sip_local.port()), "local_net peer must see the internal port in the BYE Via"); + println!("[E2E] local_net peer: Contact/Via/From carry the internal bind 127.0.0.1:{}", sip_local.port()); + + set_global_pjsip_config(PjsipConfig::default()); +} diff --git a/crates/asterisk-sip/src/event_handler.rs b/crates/asterisk-sip/src/event_handler.rs index 9729ba8..de28ec4 100644 --- a/crates/asterisk-sip/src/event_handler.rs +++ b/crates/asterisk-sip/src/event_handler.rs @@ -1629,7 +1629,13 @@ impl SipEventHandler { let Ok(mut ok) = request.create_response(200, "OK") else { return; }; - ok.add_header("Contact", &format!("", local_addr)); + ok.add_header( + "Contact", + &format!( + "", + crate::sdp::advertised_signaling_hostport(local_addr, remote_addr) + ), + ); let sdp_str = answer.to_string(); ok.add_header("Content-Type", "application/sdp"); ok.add_header("Content-Length", &sdp_str.len().to_string()); @@ -1657,7 +1663,13 @@ impl SipEventHandler { let Ok(mut ok) = request.create_response(200, "OK") else { return; }; - ok.add_header("Contact", &format!("", local_addr)); + ok.add_header( + "Contact", + &format!( + "", + crate::sdp::advertised_signaling_hostport(local_addr, remote_addr) + ), + ); if let Some((interval, refresher)) = session_timer_response(request) { ok.add_header( "Session-Expires", @@ -2116,8 +2128,14 @@ impl SipEventHandler { // Build 200 OK response let mut ok_resp = request.create_response(200, "OK").ok()?; - // Add Contact header - ok_resp.add_header("Contact", &format!("", session.local_addr)); + // Add Contact header (NAT-scoped toward the peer — New-3). + ok_resp.add_header( + "Contact", + &format!( + "", + crate::sdp::advertised_signaling_hostport(session.local_addr, remote_addr) + ), + ); // Add SDP body if let Some(ref sdp) = answer_sdp { diff --git a/crates/asterisk-sip/src/pjsip_config.rs b/crates/asterisk-sip/src/pjsip_config.rs index 1cab1d3..ac946f2 100644 --- a/crates/asterisk-sip/src/pjsip_config.rs +++ b/crates/asterisk-sip/src/pjsip_config.rs @@ -44,6 +44,12 @@ pub struct TransportConfig { pub external_media_address: Option, /// External signaling address (NAT traversal). pub external_signaling_address: Option, + /// External signaling port (New-3): the port advertised in Via/Contact/From + /// toward external peers, overriding the bind port for a NAT/forward that + /// maps an external port to the internal bind. `None` = advertise the bind + /// port. Only applied together with `external_signaling_address`, and only + /// for peers outside `local_net`. + pub external_signaling_port: Option, /// TLS certificate file. pub cert_file: Option, /// TLS private key file. @@ -517,6 +523,7 @@ fn parse_transport(cat: &asterisk_config::Category) -> Option { bind, external_media_address: get_last_variable(cat,"external_media_address").map(|s| s.to_string()), external_signaling_address: get_last_variable(cat,"external_signaling_address").map(|s| s.to_string()), + external_signaling_port: get_last_variable(cat,"external_signaling_port").and_then(|s| s.trim().parse::().ok()), cert_file: get_last_variable(cat,"cert_file").map(|s| s.to_string()), priv_key_file: get_last_variable(cat,"priv_key_file").map(|s| s.to_string()), local_net, diff --git a/crates/asterisk-sip/src/sdp/mod.rs b/crates/asterisk-sip/src/sdp/mod.rs index 3aa28e0..d59eae7 100644 --- a/crates/asterisk-sip/src/sdp/mod.rs +++ b/crates/asterisk-sip/src/sdp/mod.rs @@ -942,6 +942,96 @@ pub fn advertised_media_ip(local: std::net::SocketAddr, remote: std::net::Socket advertised_media_ip_with(external.as_deref(), &local_net, local, remote) } +/// Pick the `host:port` string to advertise in SIP Via/Contact/From toward +/// `remote` (New-3), transport-scoped by `local_net` exactly like +/// [`advertised_media_ip`]. +/// +/// Selection order: +/// 1. a transport's configured `external_signaling_address` (with its optional +/// `external_signaling_port`, else the bind port), unless the peer falls +/// inside that transport's `local_net` CIDRs; +/// 2. otherwise the concrete local bind `host:port`, unchanged. +/// +/// A peer inside `local_net` therefore sees the internal bind address/port; an +/// external peer sees the external address AND the external port. This lets a +/// NAT/forward map an external port to a different internal bind port without +/// breaking the first in-dialog request (which is targeted by the Contact this +/// produces). +pub fn advertised_signaling_hostport( + local: std::net::SocketAddr, + remote: std::net::SocketAddr, +) -> String { + let (external, external_port, local_net) = match crate::pjsip_config::get_global_pjsip_config() + { + Some(cfg) => { + // Same bind-coverage lookup as advertised_media_ip: the transport + // whose bind covers `local` — exact ip+port, then exact ip, then a + // wildcard bind. A transport bound to a DIFFERENT concrete address + // never donates its NAT config. + let with_ext = |pred: &dyn Fn(&crate::pjsip_config::TransportConfig) -> bool| { + cfg.transports + .iter() + .find(|t| t.external_signaling_address.is_some() && pred(t)) + }; + let transport = + with_ext(&|t| t.bind.ip() == local.ip() && t.bind.port() == local.port()) + .or_else(|| with_ext(&|t| t.bind.ip() == local.ip())) + .or_else(|| with_ext(&|t| t.bind.ip().is_unspecified())); + match transport { + Some(t) => ( + t.external_signaling_address.clone(), + t.external_signaling_port, + t.local_net.clone(), + ), + None => (None, None, Vec::new()), + } + } + None => (None, None, Vec::new()), + }; + advertised_signaling_hostport_with( + external.as_deref(), + external_port, + &local_net, + local, + remote, + ) +} + +/// Testable core of [`advertised_signaling_hostport`] (config plumbed in +/// explicitly). +fn advertised_signaling_hostport_with( + external: Option<&str>, + external_port: Option, + local_net: &[String], + local: std::net::SocketAddr, + remote: std::net::SocketAddr, +) -> String { + // A configured external signaling address wins for peers outside local_net. + if let Some(ext) = external.filter(|e| !e.is_empty()) { + let peer_is_local = local_net.iter().any(|cidr| { + crate::acl::AclRule::permit(cidr) + .map(|rule| rule.matches(&remote.ip())) + .unwrap_or(false) + }); + if !peer_is_local { + let port = external_port.unwrap_or_else(|| local.port()); + return format_hostport(ext, port); + } + } + // Otherwise advertise the concrete bind host:port, unchanged. + local.to_string() +} + +/// Format `host:port` for a SIP sent-by / URI host, bracketing a bare IPv6 +/// literal (`::1` -> `[::1]:5060`). An FQDN or IPv4 literal is emitted as-is. +fn format_hostport(host: &str, port: u16) -> String { + if host.contains(':') && !host.starts_with('[') { + format!("[{host}]:{port}") + } else { + format!("{host}:{port}") + } +} + /// Testable core of [`advertised_media_ip`] (config plumbed in explicitly). fn advertised_media_ip_with( external: Option<&str>, @@ -1076,6 +1166,7 @@ mod tests { bind: "192.0.2.50:5060".parse().unwrap(), external_media_address: Some("203.0.113.99".to_string()), external_signaling_address: None, + external_signaling_port: None, cert_file: None, priv_key_file: None, local_net: vec![], @@ -1091,6 +1182,122 @@ mod tests { set_global_pjsip_config(PjsipConfig::default()); } + // ---- advertised_signaling_hostport (New-3) -------------------------- + + /// No external signaling address: the bind host:port is advertised as-is. + #[test] + fn test_signaling_no_external_passes_through_bind() { + assert_eq!( + advertised_signaling_hostport_with(None, None, &[], sa("192.0.2.10:5060"), sa("198.51.100.7:5062")), + "192.0.2.10:5060" + ); + } + + /// A configured external signaling address wins for a peer outside + /// local_net; with no port override the BIND port is advertised. + #[test] + fn test_signaling_external_applies_to_nonlocal_peer_default_port() { + assert_eq!( + advertised_signaling_hostport_with( + Some("203.0.113.99"), + None, + &[], + sa("10.1.2.3:5060"), + sa("198.51.100.7:5062"), + ), + "203.0.113.99:5060" + ); + } + + /// The external signaling PORT override (New-3) replaces the bind port in + /// the advertised host:port for an external peer — independent of the bind. + #[test] + fn test_signaling_external_port_override_applies() { + assert_eq!( + advertised_signaling_hostport_with( + Some("203.0.113.99"), + Some(6666), + &[], + sa("10.1.2.3:5060"), + sa("198.51.100.7:5062"), + ), + "203.0.113.99:6666" + ); + } + + /// A peer inside local_net bypasses the external address/port and gets the + /// internal bind host:port — even when an external port override is set. + #[test] + fn test_signaling_local_net_peer_bypasses_external() { + assert_eq!( + advertised_signaling_hostport_with( + Some("203.0.113.99"), + Some(6666), + &["10.0.0.0/8".to_string()], + sa("10.1.2.3:5060"), + sa("10.9.9.9:5062"), + ), + "10.1.2.3:5060" + ); + } + + /// A bare IPv6 external literal is bracketed in the advertised host:port. + #[test] + fn test_signaling_external_ipv6_is_bracketed() { + assert_eq!( + advertised_signaling_hostport_with( + Some("2001:db8::1"), + Some(5080), + &[], + sa("192.0.2.10:5060"), + sa("198.51.100.7:5062"), + ), + "[2001:db8::1]:5080" + ); + } + + /// An FQDN external signaling address is emitted as-is (legal in a SIP + /// sent-by / URI host); resolution is the peer's job. + #[test] + fn test_signaling_external_fqdn_passes_through() { + assert_eq!( + advertised_signaling_hostport_with( + Some("pbx.example.com"), + Some(5090), + &[], + sa("192.0.2.10:5060"), + sa("198.51.100.7:5062"), + ), + "pbx.example.com:5090" + ); + } + + /// End-to-end through the config lookup: a transport bound to a DIFFERENT + /// concrete address must not donate its external signaling address. + #[test] + fn test_signaling_foreign_transport_does_not_donate_external() { + use crate::pjsip_config::{set_global_pjsip_config, PjsipConfig, TransportConfig}; + let cfg = PjsipConfig { + transports: vec![TransportConfig { + name: "other".to_string(), + protocol: "udp".to_string(), + bind: "192.0.2.50:5060".parse().unwrap(), + external_media_address: None, + external_signaling_address: Some("203.0.113.99".to_string()), + external_signaling_port: Some(6666), + cert_file: None, + priv_key_file: None, + local_net: vec![], + }], + ..Default::default() + }; + set_global_pjsip_config(cfg); + // Local bind 127.0.0.1 is NOT covered by the 192.0.2.50 transport. + let hp = advertised_signaling_hostport(sa("127.0.0.1:5060"), sa("198.51.100.7:5062")); + assert_eq!(hp, "127.0.0.1:5060"); + set_global_pjsip_config(PjsipConfig::default()); + } + #[test] fn test_parse_sdp() { let sdp_text = "v=0\r\n\ diff --git a/crates/asterisk-sip/src/session/mod.rs b/crates/asterisk-sip/src/session/mod.rs index 2c31a9e..38a71ea 100644 --- a/crates/asterisk-sip/src/session/mod.rs +++ b/crates/asterisk-sip/src/session/mod.rs @@ -206,6 +206,17 @@ impl SipSession { }) } + /// The `host:port` to advertise in Via/Contact/From toward this session's + /// remote peer, applying the transport's `external_signaling_address` + + /// optional `external_signaling_port` (New-3), scoped by `local_net` + /// exactly like `advertised_media_ip` does for SDP. A peer inside + /// `local_net` sees the internal bind; an external peer sees the external + /// address and port. With no NAT config (or a local peer) this is the + /// unchanged bind `host:port`, so non-NAT deployments are unaffected. + pub fn signaling_hostport(&self) -> String { + crate::sdp::advertised_signaling_hostport(self.local_addr, self.remote_addr) + } + /// Build an INVITE request for an outbound session. pub fn build_invite(&mut self, to_uri: &str) -> SipMessage { self.build_invite_with_uri(to_uri, to_uri) @@ -215,8 +226,9 @@ impl SipSession { /// The request_uri is used as the actual SIP Request-URI (typically the /// contact address), while to_uri is used in the To header. pub fn build_invite_with_uri(&mut self, request_uri: &str, to_uri: &str) -> SipMessage { - let from_uri = format!("sip:asterisk@{}", self.local_addr); - let contact_uri = format!("sip:asterisk@{}", self.local_addr); + let sig = self.signaling_hostport(); + let from_uri = format!("sip:asterisk@{sig}"); + let contact_uri = format!("sip:asterisk@{sig}"); let branch = format!("z9hG4bK{}", &Uuid::new_v4().to_string().replace('-', "")[..16]); let uri = SipUri::parse(request_uri).unwrap_or_else(|_| SipUri { @@ -233,7 +245,7 @@ impl SipSession { let content_length = sdp_body.len(); let mut headers = vec![ - SipHeader { name: header_names::VIA.to_string(), value: format!("SIP/2.0/UDP {};branch={}", self.local_addr, branch) }, + SipHeader { name: header_names::VIA.to_string(), value: format!("SIP/2.0/UDP {};branch={}", sig, branch) }, SipHeader { name: header_names::MAX_FORWARDS.to_string(), value: "70".to_string() }, SipHeader { name: header_names::FROM.to_string(), value: format!("<{}>;tag={}", from_uri, self.local_tag) }, SipHeader { name: header_names::TO.to_string(), value: format!("<{}>", to_uri) }, @@ -335,8 +347,9 @@ impl SipSession { let invite = self.invite.as_ref()?; let mut response = invite.create_response(200, "OK").ok()?; - // Add Contact - let contact = format!("", self.local_addr); + // Add Contact (NAT-scoped toward the peer: external addr/port for a + // peer outside local_net, internal otherwise — New-3). + let contact = format!("", self.signaling_hostport()); response.headers.push(SipHeader { name: header_names::CONTACT.to_string(), value: contact, @@ -416,9 +429,10 @@ impl SipSession { }; let branch = format!("z9hG4bK{}", &Uuid::new_v4().to_string().replace('-', "")[..16]); + let sig = self.signaling_hostport(); let headers = vec![ - SipHeader { name: header_names::VIA.to_string(), value: format!("SIP/2.0/UDP {};branch={}", self.local_addr, branch) }, + SipHeader { name: header_names::VIA.to_string(), value: format!("SIP/2.0/UDP {};branch={}", sig, branch) }, SipHeader { name: header_names::MAX_FORWARDS.to_string(), value: "70".to_string() }, SipHeader { name: header_names::FROM.to_string(), value: invite.from_header()?.to_string() }, SipHeader { @@ -443,6 +457,7 @@ impl SipSession { /// Build a BYE request. pub fn build_bye(&mut self) -> Option { + let sig = self.signaling_hostport(); let dialog = self.dialog.as_mut()?; let cseq = dialog.next_cseq(); @@ -458,12 +473,12 @@ impl SipSession { let branch = format!("z9hG4bK{}", &Uuid::new_v4().to_string().replace('-', "")[..16]); - let from_value = format!(";tag={}", self.local_addr, dialog.local_tag); + let from_value = format!(";tag={}", sig, dialog.local_tag); let to_value = format!("<{}>;tag={}", dialog.remote_uri, dialog.remote_tag); let headers = vec![ - SipHeader { name: header_names::VIA.to_string(), value: format!("SIP/2.0/UDP {};branch={}", self.local_addr, branch) }, + SipHeader { name: header_names::VIA.to_string(), value: format!("SIP/2.0/UDP {};branch={}", sig, branch) }, SipHeader { name: header_names::MAX_FORWARDS.to_string(), value: "70".to_string() }, SipHeader { name: header_names::FROM.to_string(), value: from_value }, SipHeader { name: header_names::TO.to_string(), value: to_value }, @@ -545,6 +560,7 @@ impl SipSession { /// /// Used by the SFU ConfBridge to add/remove video streams for participants. pub fn build_reinvite(&mut self, sdp: &SessionDescription) -> Option { + let sig = self.signaling_hostport(); let dialog = self.dialog.as_mut()?; let cseq = dialog.next_cseq(); @@ -562,19 +578,19 @@ impl SipSession { let branch = format!("z9hG4bK{}", &Uuid::new_v4().to_string().replace('-', "")[..16]); // For UAS (inbound call), From = our local tag, To = remote tag. - let from_value = format!(";tag={}", self.local_addr, dialog.local_tag); + let from_value = format!(";tag={}", sig, dialog.local_tag); let to_value = format!("<{}>;tag={}", dialog.remote_uri, dialog.remote_tag); let body = sdp.to_string(); let headers = vec![ - SipHeader { name: header_names::VIA.to_string(), value: format!("SIP/2.0/UDP {};branch={}", self.local_addr, branch) }, + SipHeader { name: header_names::VIA.to_string(), value: format!("SIP/2.0/UDP {};branch={}", sig, branch) }, SipHeader { name: header_names::MAX_FORWARDS.to_string(), value: "70".to_string() }, SipHeader { name: header_names::FROM.to_string(), value: from_value }, SipHeader { name: header_names::TO.to_string(), value: to_value }, SipHeader { name: header_names::CALL_ID.to_string(), value: self.call_id.clone() }, SipHeader { name: header_names::CSEQ.to_string(), value: format!("{} INVITE", cseq) }, - SipHeader { name: header_names::CONTACT.to_string(), value: format!("", self.local_addr) }, + SipHeader { name: header_names::CONTACT.to_string(), value: format!("", sig) }, SipHeader { name: header_names::CONTENT_TYPE.to_string(), value: "application/sdp".to_string() }, SipHeader { name: header_names::CONTENT_LENGTH.to_string(), value: body.len().to_string() }, ]; @@ -608,8 +624,9 @@ impl SipSession { }); let branch = format!("z9hG4bK{}", &Uuid::new_v4().to_string().replace('-', "")[..16]); + let sig = self.signaling_hostport(); - let from_value = format!(";tag={}", self.local_addr, dialog.local_tag); + let from_value = format!(";tag={}", sig, dialog.local_tag); let to_value = format!("<{}>;tag={}", dialog.remote_uri, dialog.remote_tag); // CSeq from the response we're ACKing. @@ -619,7 +636,7 @@ impl SipSession { .unwrap_or(1); let headers = vec![ - SipHeader { name: header_names::VIA.to_string(), value: format!("SIP/2.0/UDP {};branch={}", self.local_addr, branch) }, + SipHeader { name: header_names::VIA.to_string(), value: format!("SIP/2.0/UDP {};branch={}", sig, branch) }, SipHeader { name: header_names::MAX_FORWARDS.to_string(), value: "70".to_string() }, SipHeader { name: header_names::FROM.to_string(), value: from_value }, SipHeader { name: header_names::TO.to_string(), value: to_value }, From 05afed76dd24294bf9377f3a97f5499d7f274a6d Mon Sep 17 00:00:00 2001 From: Repin Agent Date: Fri, 17 Jul 2026 03:31:44 -0600 Subject: [PATCH 2/2] =?UTF-8?q?fix(sip):=20address=20codex=20CP2=20review?= =?UTF-8?q?=20=E2=80=94=20transport=20selection,=20port-0,=20stronger=20e2?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - F1 (High): select the transport that COVERS local_addr first, then read its NAT config, instead of filtering for transports that HAVE an external address. Stops a same-ip/wildcard transport from donating its external addr/port to a covering transport that deliberately set none. Unit test added. - F6 (Low): reject external_signaling_port=0 and malformed values (warn, fall back to the bind port) rather than silently advertising an unusable port 0. - Strengthen e2e: assert the internal-peer BYE From host:port (was logged but not asserted) and that the external port override never reaches it. --- .../tests/e2e_signaling_address.rs | 5 ++ crates/asterisk-sip/src/pjsip_config.rs | 17 ++++- crates/asterisk-sip/src/sdp/mod.rs | 64 +++++++++++++++---- 3 files changed, 72 insertions(+), 14 deletions(-) diff --git a/crates/asterisk-integration-tests/tests/e2e_signaling_address.rs b/crates/asterisk-integration-tests/tests/e2e_signaling_address.rs index b6734fd..724f83b 100644 --- a/crates/asterisk-integration-tests/tests/e2e_signaling_address.rs +++ b/crates/asterisk-integration-tests/tests/e2e_signaling_address.rs @@ -248,6 +248,11 @@ async fn external_signaling_address_and_port_scoped_by_local_net() { let (v_host, v_port) = via_hostport(bye.get_header("Via").expect("BYE Via")); assert_eq!(v_host, "127.0.0.1", "local_net peer must see the internal address in the BYE Via"); assert_eq!(v_port, Some(sip_local.port()), "local_net peer must see the internal port in the BYE Via"); + + let (f_host, f_port) = uri_hostport(bye.get_header("From").expect("BYE From")); + assert_eq!(f_host, "127.0.0.1", "local_net peer must see the internal address in the BYE From URI"); + assert_eq!(f_port, Some(sip_local.port()), "local_net peer must see the internal port in the BYE From URI"); + assert_ne!(f_port, Some(EXT_PORT), "the external port override must NOT reach a local_net peer's From"); println!("[E2E] local_net peer: Contact/Via/From carry the internal bind 127.0.0.1:{}", sip_local.port()); set_global_pjsip_config(PjsipConfig::default()); diff --git a/crates/asterisk-sip/src/pjsip_config.rs b/crates/asterisk-sip/src/pjsip_config.rs index ac946f2..6f91fb1 100644 --- a/crates/asterisk-sip/src/pjsip_config.rs +++ b/crates/asterisk-sip/src/pjsip_config.rs @@ -523,7 +523,22 @@ fn parse_transport(cat: &asterisk_config::Category) -> Option { bind, external_media_address: get_last_variable(cat,"external_media_address").map(|s| s.to_string()), external_signaling_address: get_last_variable(cat,"external_signaling_address").map(|s| s.to_string()), - external_signaling_port: get_last_variable(cat,"external_signaling_port").and_then(|s| s.trim().parse::().ok()), + external_signaling_port: get_last_variable(cat,"external_signaling_port").and_then(|s| { + // Reject 0 and malformed values (codex CP2 F6): silently advertising + // port 0 would map the NAT target to an unusable port. Fall back to + // the bind port (None) and warn, rather than break the mapping. + match s.trim().parse::() { + Ok(0) => { + warn!(name = %cat.name, "external_signaling_port=0 is invalid; ignoring (advertising the bind port)"); + None + } + Ok(p) => Some(p), + Err(_) => { + warn!(name = %cat.name, value = %s.trim(), "invalid external_signaling_port; ignoring (advertising the bind port)"); + None + } + } + }), cert_file: get_last_variable(cat,"cert_file").map(|s| s.to_string()), priv_key_file: get_last_variable(cat,"priv_key_file").map(|s| s.to_string()), local_net, diff --git a/crates/asterisk-sip/src/sdp/mod.rs b/crates/asterisk-sip/src/sdp/mod.rs index d59eae7..a839db3 100644 --- a/crates/asterisk-sip/src/sdp/mod.rs +++ b/crates/asterisk-sip/src/sdp/mod.rs @@ -964,19 +964,22 @@ pub fn advertised_signaling_hostport( let (external, external_port, local_net) = match crate::pjsip_config::get_global_pjsip_config() { Some(cfg) => { - // Same bind-coverage lookup as advertised_media_ip: the transport - // whose bind covers `local` — exact ip+port, then exact ip, then a - // wildcard bind. A transport bound to a DIFFERENT concrete address - // never donates its NAT config. - let with_ext = |pred: &dyn Fn(&crate::pjsip_config::TransportConfig) -> bool| { - cfg.transports - .iter() - .find(|t| t.external_signaling_address.is_some() && pred(t)) - }; - let transport = - with_ext(&|t| t.bind.ip() == local.ip() && t.bind.port() == local.port()) - .or_else(|| with_ext(&|t| t.bind.ip() == local.ip())) - .or_else(|| with_ext(&|t| t.bind.ip().is_unspecified())); + // Select the transport whose bind COVERS `local` first — exact + // ip+port, then exact ip, then a wildcard bind — and only THEN read + // its NAT config. Selecting by bind coverage (not by "has an + // external address") stops a same-ip/wildcard transport that + // happens to set an external address from donating it to a covering + // transport that deliberately set none (codex CP2 F1). A transport + // bound to a DIFFERENT concrete address never covers `local`. + // (Transport identity is not yet carried on the dialog; when it is, + // that binding should replace this bind-coverage lookup — protocol + // is likewise not distinguished here because only UDP is bound.) + let transport = cfg + .transports + .iter() + .find(|t| t.bind == local) + .or_else(|| cfg.transports.iter().find(|t| t.bind.ip() == local.ip())) + .or_else(|| cfg.transports.iter().find(|t| t.bind.ip().is_unspecified())); match transport { Some(t) => ( t.external_signaling_address.clone(), @@ -1298,6 +1301,41 @@ mod tests { set_global_pjsip_config(PjsipConfig::default()); } + /// codex CP2 F1: the transport that COVERS `local` is selected first, even + /// when it sets no external address. A same-ip / wildcard transport that + /// happens to configure an external address must NOT donate it to the + /// covering transport that deliberately set none. + #[test] + fn test_signaling_covering_transport_without_external_wins() { + use crate::pjsip_config::{set_global_pjsip_config, PjsipConfig, TransportConfig}; + let base = |bind: &str, ext: Option<&str>| TransportConfig { + name: format!("t-{bind}"), + protocol: "udp".to_string(), + bind: bind.parse().unwrap(), + external_media_address: None, + external_signaling_address: ext.map(|s| s.to_string()), + external_signaling_port: ext.map(|_| 6666), + cert_file: None, + priv_key_file: None, + local_net: vec![], + }; + let cfg = PjsipConfig { + transports: vec![ + // The covering transport (exact ip 192.0.2.10) sets NO external. + base("192.0.2.10:5060", None), + // A wildcard transport DOES set an external address. + base("0.0.0.0:5062", Some("203.0.113.99")), + ], + ..Default::default() + }; + set_global_pjsip_config(cfg); + // local is covered by the exact-ip transport (no external) -> internal + // bind must be advertised, NOT the wildcard transport's external. + let hp = advertised_signaling_hostport(sa("192.0.2.10:5060"), sa("198.51.100.7:5062")); + assert_eq!(hp, "192.0.2.10:5060", "covering transport (no external) must not inherit the wildcard's external"); + set_global_pjsip_config(PjsipConfig::default()); + } + #[test] fn test_parse_sdp() { let sdp_text = "v=0\r\n\