feat(webrtc): STUN-dispatch WebRTC-Direct listener (v1) + v1 dialer, /sdp harness opt-in - #1449
Conversation
7398e14 to
f761dd4
Compare
|
@seetadev @acul71 Rebased on latest main. This PR adds the spec-aligned WebRTC-Direct listener: one shared UDP port, STUN |
41968d8 to
0d24ab7
Compare
acul71
left a comment
acul71
left a comment
There was a problem hiding this comment.
Thanks for pushing this through — this is the right shape for the v1 STUN listener slice of #1437: shared UdpMux port, libp2p+webrtc+v1/ first-contact dispatch, inferred offer, Noise initiator path, and /sdp harness opt-in off by default. Tests covering STUN vs harness, concurrent dials, unknown-prefix rejection, and the in-flight cap are in good shape. CI looks green.
Please do not Fixes/Closes #1437 on merge — v2, go/js interop, and true ICE-Lite are still open on that tracking issue. Refs #1437 is correct.
Requested changes
1. Spec-path dial must not use default Google STUN (blocker)
dial() always passes self._config.ice_servers into create_peer_connection. The default is stun:stun.l.google.com:19302. That undoes the offline-dial fix from the inbound/Noise PR in this stack: gathering the public STUN server adds seconds on firewalled/offline networks, phones Google on every /webrtc-direct dial, and does not help the STUN listener path (the offer is inferred from the first BINDING REQUEST to the advertised multiaddr). Loopback tests hide this with ice_servers=[] in _transport().
For enable_sdp_http_harness=False, create the PC with ice_servers=[] (same as the listener). Keep config.ice_servers for the harness / explicit NAT-TURN opt-in. Please add a regression test that the default spec dial does not configure RTCIceServers.
2. Unknown-STUN rate limiting (ship here or park on #1437)
Spec step 6 / #1437 ask for rate-limiting first-contact STUN. Today only max_in_flight_connections bounds allocated PCs, and it runs after parse + add_ice_connection. Malformed / wrong-prefix floods still hit parse_direct_username on every datagram. Prefer a small per-source-IP token bucket before allocating ICE/PC state (go-libp2p also bounds the mux accept queue). If you want to defer, tick it explicitly on #1437 and leave the ponytail comment — but this socket is now a public STUN endpoint, so shipping the bucket here is better.
Please also
- Infer offer ICE creds from the client USERNAME half (
_client_ufrag), matching go-libp2p’s v1 recovery (client_pwd == client_ufrag). py↔py works today only because both halves are the samemake_v1_credential()string. - After detecting v1, also validate the credential as an ice-pwd (RFC 8839 min 22).
libp2p+webrtc+v1/is 17 chars, so a 4-char suffix yields a 21-char string that passes ufrag checks but is not a valid pwd. Add a reject table case for that. - Drop unused
WebRTCDirectTransport._sdp_builder(SDPBuilderstays for private transport).
Questions
- Was re-passing
config.ice_serversinto spec-pathdial()intentional, or leftover from the harness era? - Is inferred
a=setup:active(vs spec/goactpass) the long-term aiortc constraint, or can we force the DTLS server role another way before go interop? - Should
0.0.0.0listen still advertise127.0.0.1, or shouldget_addrs()prefer a non-loopback iface the way the Windows tests do?
Happy to re-review once the STUN-server dial regression is fixed.
acul71
left a comment
acul71
left a comment
There was a problem hiding this comment.
Inline notes for the changes requested above.
| # 1. Create RTCPeerConnection + Noise channel | ||
| pc = await bridge.run_coro(create_peer_connection(rtc_cert)) | ||
| pc = await bridge.run_coro( | ||
| create_peer_connection(rtc_cert, ice_servers=self._config.ice_servers) |
There was a problem hiding this comment.
Blocker: on the default (spec) path this reintroduces the Google STUN gather that the inbound/Noise work removed for offline dials. Srflx candidates never reach the STUN listener anyway — offer is inferred from the first BINDING REQUEST. Please use ice_servers=[] when enable_sdp_http_harness is false (same as the listener), and keep config.ice_servers for the harness / explicit TURN. Add a regression test so loopback helpers with ice_servers=[] cannot hide this again.
| return | ||
| # ponytail: in-flight cap only; add a per-source-IP token bucket if | ||
| # STUN floods from one address become a problem (spec: SHOULD rate-limit). | ||
| if self._in_flight >= self._config.max_in_flight_connections: |
There was a problem hiding this comment.
In-flight cap alone is not the rate-limit the spec / #1437 ask for — it only bounds allocated PCs after a successful parse + add_ice_connection. Malformed / wrong-prefix floods still pay parse_direct_username on every datagram. Prefer a per-source-IP token bucket before allocating ICE state (or explicitly park on #1437).
|
|
||
| offer_sdp = build_inferred_offer( | ||
| client_ufrag=server_ufrag, | ||
| client_pwd=server_ufrag, |
There was a problem hiding this comment.
Please use the client half of the USERNAME for the inferred offer (client_ufrag=_client_ufrag, client_pwd=_client_ufrag for v1), matching go-libp2p. Today py↔py works because both halves are the same credential; unequal halves (or a crafted v1_cred:other) would mismatch.
| server_ufrag, sep, client_ufrag = username.partition(":") | ||
| if not sep: | ||
| raise WebRTCConnectionError("STUN USERNAME is not 'server:client'") | ||
| if not (is_ice_ufrag(server_ufrag) and is_ice_ufrag(client_ufrag)): |
There was a problem hiding this comment.
For v1 the same string is also ice-pwd (RFC 8839 min 22). libp2p+webrtc+v1/ is 17 chars, so a 4-char suffix (21 total) passes ufrag checks but is not a valid pwd. After detecting v1, also require is_ice_pwd(...) and add a reject table case.
tox commands_pre installed no extras, so aiortc was absent in CI and every test under tests/core/transport/webrtc was skipped on GitHub. aiortc 1.15 is pure Python and its native deps (av, pylibsrtp) ship manylinux/win wheels, so no system packages are needed. Refs libp2p#1437
…oles The listener's inbound completion was a stub: after ICE it logged and returned, so no inbound connection ever reached the handler. dial() was broken too - get_remote_fingerprint read attributes aiortc does not have. - listener: own a trio nursery (system task, like TCP), hop asyncio->trio without blocking the loop, run Noise XX as *initiator* (spec: server initiates, dialer responds), then hand the authenticated connection to the handler; bound in-flight unauthenticated inbounds. - transport.dial(): Noise responder; verify authenticated peer ID against /p2p/ after the handshake. - noise: role-ordered prologue (dialer fingerprint, then server); PatternXX.handshake_outbound(remote_peer=None) skips only the ID equality check; DataChannelReadWriter.read(n) honours n (the Noise packet reader asks for the 2-byte prefix alone). - helpers: get_remote_fingerprint reads pc.sctp.transport._ssl peer cert; noise send waits for data channel 0 to open. - tests: transport-level dial->listen->stream echo loopback, wrong /p2p/ rejection, e2e Noise role test, buffered read test, fingerprint test. Refs libp2p#1437
- Frame Noise handshake bytes as uvarint-prefixed webrtc.pb.Message stream frames on channel 0 (spec 'Multiplexing'; what go/js do) instead of raw bytes - required for interop; py<->py was symmetric so tests did not notice. - Listener: guard the handler call - the nursery lives in a trio system task, so an escaping exception aborted the whole trio run. - dial(): bound the Noise phase (responder's first step is a read) with handshake_timeout; close the PC on every failure path, not just the two explicit mismatches; do not pass config.ice_servers (previously no STUN servers were used; the default Google STUN added ~5s to offline dials). - Tests: framing round-trip/chunking/FIN/malformed, handler-exception regression. Refs libp2p#1437
…string Review follow-up: the module docstring still described the prologue as local_fp || remote_fp; it is dialer_fp || server_fp (spec role order), matching build_noise_prologue. Also bound the dialer's wait_for_connected with config.handshake_timeout like the listener does. Refs libp2p#1437
…connection) aiortc has no injection point for an external ICE connection, so a listener that demuxes on one UDP port could not use RTCPeerConnection on top of UdpMux. Add attach_muxed_connection(pc, mux, conn): swaps the gatherer/transport aioice.Connection for the mux-backed one, rebinds the DTLS _recv/_send that RTCIceTransport captured at construction, and keeps the mux tables in sync on ICE state changes. UdpMux fixes found while validating that path with a real peer: - mark _local_candidates_start so aiortc's gather() from setLocalDescription is a no-op (it bound extra sockets and appended their candidates to the answer); - STUN responses/indications carry no USERNAME - route them by address, and learn peer addresses from inbound checks and outbound sends; - unregister(ufrag) also drops the addresses learned for that protocol; - STUN-shaped-but-malformed datagrams go straight to the connection's data path (StunProtocol only catches ValueError, so struct.error escaped). Test: mux-backed server PC vs plain aiortc client PC on loopback - answer advertises only the shared port, ICE/DTLS/SCTP connect, data flows both ways, tables empty after close. webrtc extra now aiortc>=1.15. Refs libp2p#1437
… mux test - _learn_addr: latest connection wins for a reused (ip, port) (pion behaviour) so a redial from the same socket reaches its new connection; cap learned addresses per protocol (unauthenticated STUN with a live ufrag from many source ports must not grow the table without bound). - test_pc_over_mux: bind the mux on 0.0.0.0 and advertise a real host address - a 127.0.0.1-bound socket cannot reply to a LAN-bound peer socket on Windows (WinError 1231), which Linux's weak-host model hid. Refs libp2p#1437
A hung close() after a primary failure would outlive asyncio.wait_for and be killed by pytest-timeout, reported as an xdist 'worker crashed' that hides the real error (seen on Windows CI). Refs libp2p#1437
…ection) pc.close() deadlocks on Windows CPython 3.12/3.13: DTLS queues its close_notify datagram, transport.close() runs with that write in flight and defers connection_lost to the write callback, but _ProactorDatagramTransport._loop_writing early-returns on _conn_lost - the deferred connection_lost is never delivered and aioice's StunProtocol.close() awaits its closed-future forever (confirmed via task/transport dumps on CI: closing=True, write_fut finished, closed.done=False). close_peer_connection(pc) bounds close() and, on timeout, abort()s the lingering aioice transports - _force_close delivers connection_lost unconditionally - then lets close() finish. Used in the PC-over-mux test; production call sites switch in the listener PR. Refs libp2p#1437
Writing a wrong - or upgraded-away - private/mangled attribute name is a silent no-op: setattr creates a new attribute the library never reads and the failure only surfaces far downstream (the SDP-fingerprint invariant test, an ICE that binds extra sockets). Assert hasattr at the write so it fails at the line that caused it, and so an aiortc/aioice upgrade that renames or drops a slot breaks loudly at construction. Applied to the DTLS cert pin (_RTCPeerConnection__certificates, ca8331f), the attach_muxed_connection injection points, and add_ice_connection's aioice fields. Refs libp2p#1437
…Windows Windows surfaces an ICMP port-unreachable for a datagram we sent (a keepalive to a peer that just closed) as WSAECONNRESET on the shared socket's next recv, and the proactor loop does not re-arm reading after error_received - the mux went deaf for every other peer (seen in CI as '[WinError 10054]' followed by the next dial's ICE timing out). Disable that behaviour with SIO_UDP_CONNRESET on the mux socket; no-op elsewhere. Refs libp2p#1437
Listener (spec path, default): one shared UDP socket via UdpMux. The first inbound STUN BINDING REQUEST is parsed for USERNAME = server_ufrag:client_ufrag; the libp2p+webrtc+v1/ prefix selects the flow (unknown/missing prefix -> rejected, never assumed v1; both halves validated as ice-chars, 4..256). A mux-backed ICE connection is registered for the ufrag, the packet replayed into it, and an aiortc PC attached (attach_muxed_connection). The dialer's offer is inferred from the packet: ufrag == pwd == server credential, c=/candidate at the STUN source, a=setup:active so aiortc takes the DTLS server role, placeholder fingerprint with DTLS peer verification disabled for inbound per spec (Noise authenticates). Then the existing PR1 completion path (Noise initiator -> handler). In-flight cap on unauthenticated inbounds. Dialer (v1): the same libp2p+webrtc+v1/<random> string is set as ufrag and pwd on the aioice Connection (aiortc regenerates ICE creds from it in setLocalDescription; SDP text munging is ignored) and on a synthetic ICE-Lite, setup:passive answer built from the multiaddr - aiortc pins the server's DTLS cert via the certhash fingerprint. The HTTP POST /sdp harness is now opt-in (WebRTCTransportConfig.enable_sdp_http_harness, off by default): when on, the listener also serves it on TCP and our dialer uses it. sdp.py: parse_direct_username, build_inferred_offer, build_synthetic_answer, make_v1_credential; _generate_ice_credential emits ice-chars only (token_urlsafe produced '-'/'_' which aioice rejects). UdpMux passes the full USERNAME to the unknown-STUN handler. Tests: loopback echo over STUN and over the harness, two concurrent dials on one port, unknown-prefix rejection, in-flight cap, sdp helpers. Refs libp2p#1437
- listen(): bind the UDP socket before spawning the trio nursery and map OSError to WebRTCConnectionError, so a bind failure leaves no orphaned system task. - close(): cancel in-flight inbound setup tasks so their peer connections close immediately instead of after handshake_timeout. - ice-char validation uses fullmatch ($ accepted a trailing newline). - tests: bind listeners on 0.0.0.0 (advertised as 127.0.0.1) - on Windows a 127.0.0.1-bound socket cannot answer a LAN-bound peer socket; add bind-failure and cancel-on-close assertions, newline rejection cases. Refs libp2p#1437
aiortc dialers gather LAN host candidates only (aioice skips loopback) and on Windows a LAN-bound UDP socket cannot send to 127.0.0.1 (WinError 1214), so a listener advertising 127.0.0.1 is unreachable from a Windows aiortc dialer. Bind the test listeners on the first non-loopback IPv4 interface (fallback 127.0.0.1) and target the advertised host from the STUN poke tests (which bind 0.0.0.0 for the same reason). Refs libp2p#1437
… site The Windows proactor close hang (see the close_peer_connection commit) would otherwise stall the listener's inbound failure paths, the dialer's failure path, and WebRTCConnection.close() via _close_pc_cb - each a leaked asyncio task on the bridge loop. Refs libp2p#1437
…_attr Dialer v1 ICE-credential fields on the aioice Connection and the listener's inbound _validate_peer_identity replacement now assert the slot exists before writing, matching the pattern introduced in libp2p#1448. Refs libp2p#1437
- dial(): no STUN/TURN on the spec path (config.ice_servers had been re-passed when the STUN path was added, undoing the offline-dial fix); ice_servers now only applies to the HTTP harness. Regression test uses default configs so the loopback helper's ice_servers=[] cannot hide it. - Per-source-IP token bucket on first-contact STUN, applied before any parsing or ICE/PC allocation (spec step 6). - Inferred offer takes the client half of USERNAME (client_pwd == client_ufrag for v1), matching go-libp2p. - parse_direct_username: v1 halves must also be valid ice-pwds (>= 22) - the 17-char prefix plus a 4-char suffix passed the ufrag check only. - Drop unused WebRTCDirectTransport._sdp_builder; note on ice_servers. Refs libp2p#1437
898d1da to
6b7d1b3
Compare
|
@acul71 All requested changes are in (rebased on main):
On the other questions: |
Use actpass in inferred offers and force the listener DTLS server role, advertise concrete interface IPs when listening on 0.0.0.0, and document the default STUN path vs the opt-in HTTP /sdp harness. Co-authored-by: Cursor <cursoragent@cursor.com>
acul71
left a comment
acul71
left a comment
There was a problem hiding this comment.
Round 3 addresses the deferred follow-ups:
- DTLS setup:
build_inferred_offernow usesa=setup:actpass;force_listener_dtls_server_role()sets the listener as DTLS server beforecreateAnswer. - 0.0.0.0 advertising: wildcard listens publish concrete interface IPs via
get_available_interfaces;_resolve_candidate_hostpicks a non-loopback ICE host when available. - Docs: README + module docstring note the default STUN path vs opt-in
enable_sdp_http_harness.
All prior review blockers were already fixed in round 2. LGTM to merge once CI is green.
|
Pushed round 3 (
Local: lint, typecheck, docs, 219/219 webrtc tests, full suite green (Kad DHT flakes passed on retry). |
2 participants
What
Third step for #1437 — the spec-aligned listener.
Listener (default path) — one shared UDP socket (
UdpMux):USERNAME = server_ufrag:client_ufrag;libp2p+webrtc+v1/prefix selects the flow. Unknown/missing prefix → rejected (spec: never assume v1). Both halves validated (ice-chars, 4–256).attach_muxed_connection, feat(webrtc): run aiortc RTCPeerConnection over UdpMux (attach_muxed_connection) #1448).c=/candidate at the STUN source,a=setup:active(so aiortc takes the DTLS server role — withactpassit would answer as client), placeholder fingerprint with DTLS peer verification disabled for inbound per spec step 6.2/7 (Noise authenticates).max_in_flight_connections).Dialer (v1) — sets
libp2p+webrtc+v1/<random>as ufrag and pwd on the aioiceConnection(aiortc regenerates ICE creds from it insetLocalDescription; SDP-text munging is ignored) and on a synthetic ICE-Litea=setup:passiveanswer built from the multiaddr; the certhash fingerprint in that answer makes aiortc pin the server's DTLS cert.HTTP
POST /sdpharness → opt-inWebRTCTransportConfig(enable_sdp_http_harness=True)(experimental, py↔py). Off by default; nothing else used it.sdp.py:parse_direct_username,build_inferred_offer,build_synthetic_answer,make_v1_credential; credentials generated with ice-chars only (token_urlsafeproduced-/_, which aioice rejects).UdpMuxunknown-STUN handler now receives the fullUSERNAME.Not in this PR (tracked on #1437)
libp2p+webrtc+v2/, no munging; specs#715 still open) — listener currently drops v2 first contacts with a debug log.Tests
tests/core/transport/webrtc: 204 passed; loopback file 6× no flake; mypy/pyrefly clean.Stacked on #1448 → #1447 → #1446.
Refs #1437