feat: wire the OpenPlay WebRTC path end to end, behind a consent prompt - #46
Open
Developer1010x wants to merge 13 commits into
Open
feat: wire the OpenPlay WebRTC path end to end, behind a consent prompt#46Developer1010x wants to merge 13 commits into
Developer1010x wants to merge 13 commits into
Conversation
`rust-version = "1.80"` was aspirational. The locked dependency tree refuses to resolve below 1.88, and does so at resolution rather than compilation, so the declared value never described anything real: `zvariant_utils 3.3.0` (via zbus 5, reached from openplay-miracast and from ashpd) needs the edition2024 cargo feature stabilised in 1.85, and `time 0.3.47` then requires 1.88.0. Nothing caught this because every CI job runs `@stable`. A job pinned to the declared version is added separately. Raising it also un-suppresses `clippy::manual_is_multiple_of`, which is suppressed below 1.87 because the API does not exist there — hence the one-line change in casting.rs, which belongs with this commit rather than after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ilures
`SenderPipeline` and `ReceiverPipeline` built correct element graphs with
`webrtcbin` at the boundary and then stopped: no offer, no answer, no ICE, no
connection state, and no way for a caller to reach any of it except a raw
`&gst::Element`. The crate did not even depend on gstreamer-webrtc or
gstreamer-sdp, so it could not name the types involved.
`webrtc.rs` is that missing layer. It turns webrtcbin's GObject signals into a
`WebRtcEvent` channel the signaling code can await, and drives offer/answer by
role so exactly one peer offers. The channel is unbounded on purpose:
`webrtcbin` delivers promise replies and ICE candidates on GStreamer streaming
threads, where `Sender::blocking_send` panics outright for want of a runtime,
and `UnboundedSender::send` is the one primitive that is synchronous, never
blocks, and never needs a reactor.
Three failures fixed while wiring it, each of which fails silently:
- `rtph264pay`'s `aggregate-mode` is a GEnum, and `.property(_, 1i32)` panics
inside `.build()`. Nothing had ever called `SenderPipeline::new`, so nothing
had hit it.
- Without a fixed RTP capsfilter the payloader advertises `payload` as a
*range*, and webrtcbin cannot build an m-line from unfixed caps: `create-offer`
then succeeds and returns SDP with no media section at all. The session
negotiates, connects, and carries nothing.
- The pad-added handler matched `caps.to_string().contains("video")`, which both
false-positives on the base64 in sprop-parameter-sets and false-negatives to a
black screen when caps are not yet available. It matches the `media` field.
`ReceiverPipeline` gains a frame handler, because the appsink does not exist
until a pad appears and a caller polling `video_appsink()` would just get `None`
forever. Frames are repacked to a tight RGBA buffer at the only place that knows
the stride, since GStreamer pads rows and a texture uploader given the raw
mapping produces a sheared image.
`tests/webrtc_loopback.rs` negotiates two webrtcbins through this API and
asserts decoded frames arrive — the offer-with-no-m-line case in particular is
what it exists to catch. It requires the `nice` GStreamer plugin and says so
rather than skipping quietly, because webrtcbin's failure without it is to
refuse every pad request with no error.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e receiver `ReceiverAdvertiser` passed "" as the address argument, which parses to an empty address set, and `ServiceInfo::new` leaves `addr_auto` false. mdns-sd fills in interface addresses only when that flag is set, and with an empty set it suppresses everything: no unsolicited announcement on any interface, no answer to a browsing sender's PTR query, and no A/AAAA answer. The browser's own `is_ready()` also requires a non-empty address set, so even a relayed record would produce `ServiceFound` and never `ServiceResolved`. All of that happens after `register` has returned `Ok` and the advertiser has logged "mDNS service registered". A receiver looked healthy in its own log and was invisible to every sender on the network. Every test in the crate exercises `to_properties`/`from_properties` in memory, which is why a self-consistent TXT round trip passed while nothing could actually be discovered — the same blind spot `docs/crypto.md` records for the fabricated SRP group. Verified with `avahi-browse -rpt _openplay._tcp`, which now lists the receiver on every interface over both IPv4 and IPv6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Serde checks a message's shape but not its size, and every String and Vec in `SignalingMessage` arrives from an unauthenticated peer. Without a bound the ceiling is the WebSocket frame limit, which is far larger than anything this protocol legitimately carries: the biggest real message is an SDP offer of a few kilobytes. `display_name` is the one that matters most. It is rendered on the receiver's screen every frame and cloned on each read, so an unbounded one both stalls the UI thread from the network and lets a stranger put arbitrary text on someone's TV. Control characters are rejected as well as length, because a newline in a name forges lines in the receiver's structured log. Names are measured in characters rather than bytes so the limit means the same thing to the protocol and to the UI that displays it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n cost The transport accepted unlimited connections from anyone who could reach the port, with no timeouts, and handed the application a reply channel carrying no indication of which peer it belonged to. Connection identity first, because the rest depends on it. Session state — a cast in progress, a pending approval — belongs to a connection, not to the receiver as a whole. Without an identity the receiver cannot tell two senders apart, so any peer on the network could end another peer's session or relabel it, and pairing could not be built on top later. `ConnectionHandle::send` is deliberately non-blocking. The receiver drives every connection from one loop, and awaiting a full reply queue there let a single peer that stopped reading its socket freeze signaling for every other peer, permanently, for the cost of a few kilobytes. Also: an explicit 64 KiB frame limit in place of tungstenite's 64 MiB default, handshake and idle timeouts so a peer cannot hold a task and a file descriptor by connecting and saying nothing, a connection cap, backoff on accept errors so `EMFILE` does not spin a core, and `validate()` on every inbound message before it reaches any state machine. `bind` is split from `run` so a caller learns about a port clash before it tells the user the receiver has started, and can read back the real port when it asked for port 0 — which the mDNS record needs, since advertising "0" would publish an unreachable service. `tests/loopback.rs` drives a whole session over a real TLS WebSocket in one process: session negotiation, SDP both ways, trickled ICE both ways, teardown. It also pins that a client offering the wrong fingerprint is refused, and that an over-long name is dropped before the application sees it. The crate had no tests at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… afterwards `save_to_files` wrote the key with `fs::write` and chmod'd it to 0600 only afterwards. `fs::write` creates at 0666 & ~umask, so under the usual umask the key sat world-readable for the window between the two calls, and a local user watching the directory with inotify could open it in that window — after which the chmod revokes nothing, because the descriptor is already open. That matters more here than it usually would: the certificate fingerprint is the only identity anything in OpenPlay has, so whoever holds the key can impersonate the receiver completely and undetectably. The key is now created through an `OpenOptions` with the mode set, and the mode is reasserted on the descriptor before any byte is written, which also covers a file that already existed at 0644. The data directory is created 0700. On load, a group- or world-readable key is reported loudly and tightened rather than used in silence — a key left permissive by a backup restore was previously accepted forever without a word. The existing permissions test could never have caught this: the old code also reached 0600 in the end. The new ones assert the mode on the descriptor of the still-empty file, which is exactly the instant that was wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…onsent The receiver advertised nothing, listened on nothing, and displayed a static page. It now generates or loads its certificate, publishes itself over mDNS with the fingerprint in the `fp` TXT key, serves TLS signaling, builds a `ReceiverPipeline` when a sender offers, and paints the decoded frames. The consent prompt is the reason this is safe to turn on at all. Nothing here authenticates a sender: mDNS is unauthenticated so anyone on the network can advertise a fingerprint, and the protocol's pairing and auth messages have no implementation on either side. Wiring video in without a gate would have meant any device on the LAN could put pixels on someone's screen unprompted. So a session is held at `Status::PendingConsent` until a human presses Allow, the prompt names the sender and says plainly that OpenPlay cannot verify it, and there is deliberately no timeout that accepts. One session at a time, because there is one screen; a second sender is told `Busy` rather than silently replacing the first. `SessionEnd` from a connection that does not own the session is ignored, so a stranger cannot stop a cast. A liveness tick ends a session whose sender vanished, since the transport reports that only by closing the reply channel. The socket is bound before startup reports success, so a port clash is an error the user sees rather than a log line arriving after the window says "listening". It binds `[::]` with an IPv4 fallback, because mDNS advertises every interface address including IPv6 ones and an IPv4-only bind would publish addresses nothing listens on. Pong carries a real clock reading rather than echoing the sender's timestamp, which would have made every clock-offset calculation come out as exactly -rtt/2 — a plausible-looking number that is always wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `Protocol::OpenPlay` arm set a status string and cleared `is_casting`. It now connects, negotiates and streams. The receiver's fingerprint is required, not optional: it comes from the mDNS `fp` TXT key and is what makes the TLS connection mean anything, since the receiver's certificate is self-signed and any host that answers could otherwise take its place. A receiver that published no fingerprint is a hard failure rather than a fall back to unpinned TLS, so `DiscoveredReceiver` grows the accessor that was missing. The pipeline is built only after the session is accepted — there is no point touching the GPU while a human decides — and the wait has no timeout, because the far end is a person walking over to a TV. The stop flag is polled on a timer so Stop still works during that silence, which the frame-driven AirPlay loop does not do. `display_name` is taken from config rather than derived here. It is the entire basis on which somebody approves the cast, and "OpenPlay Sender" for every device would make the decision meaningless. It is sanitised to the protocol's bound so a long or odd configured name degrades to a shortened one instead of a rejection the user cannot explain. Two pre-existing bugs fixed on the way: `is_casting` was set before checking whether the receiver had a reachable address, so a receiver with none left the UI on a spinner with a live Stop button forever; and every early exit now clears the cast state, which on Linux also resumes the Wi-Fi Direct scan that `start_cast` pauses — the old OpenPlay arm paused it and never resumed, so one click stopped P2P peers appearing for the rest of the session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Miracast was documented as the one path that works. It has never worked, and
the tests could not have shown that: `rtsp_server.rs` and `session.rs` had none,
and every existing test exercised the same bitmask helpers.
Seven defects, in the order they stop a cast:
- **M1 rejected every valid response.** The check was
`!resp.status.starts_with("200")` against the status *line* `RTSP/1.0 200 OK`,
so it was always true. No negotiation could reach M2. Responses now carry a
parsed status code.
- **The session ended milliseconds after it began.** `run_session` sent
`Ended(None)` immediately after `Ready`; the casting loop read the queued
`Ended` on its first iteration and stopped the pipeline. Deleting the line
alone would not have fixed it — returning from `run_session` drops the event
sender, and the loop's `None` arm breaks just the same. The session now parks
in `serve_control_channel` for the life of the cast.
- **The RTSP control socket was closed right after M7**, because `negotiate`
took the `TcpStream` by value — immediately after promising
`Session: 1;timeout=30`. It borrows the connection now, and keep-alives go out
at a third of that interval.
- **`client_port` ranges were unparsed.** RFC 2326 sends `client_port=19000-19001`;
`parse::<u16>()` failed, and the caller substituted 1028. Video went to a port
nobody listened on, with no error. An unusable SETUP is now a hard failure
rather than a guess.
- **`disconnect()` used the root D-Bus interface**, where the method does not
exist, and swallowed the resulting error before logging success. The P2P group
was never torn down. It uses `Interface.P2PDevice` — where group teardown
actually lives — and a guard makes it run even when the session future is
cancelled.
- **No read timeouts anywhere**, so a sink that stalled hung the cast with the
UI stuck casting.
- **Pipelined messages were discarded** with the function-local buffer, losing a
PLAY that arrived in the same segment as a SETUP.
A scripted fake sink now drives the full M1-M7 exchange in tests; the crate goes
from 21 to 40. The README leads with what is still unverified, which is
everything that needs real hardware.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a source Findings from auditing the HAP implementation against pyatv, HAP-python and fast-srp-hap, which are the implementations that interoperate with real Apple hardware. - **The ChaCha20-Poly1305 nonces were built backwards**: the eight-byte label followed by four zeros, where the zeros belong in front. pyatv pads with `b"\x00" * (12 - len(nonce)) + nonce` and HAP-python with `rjust`, and this repo's own `control_channel.rs` already writes its counter to `nonce[4..]` — the two files could not both be right. Only the PIN flow and pair-verify reach these, which is why hardware testing of transient pairing never touched them. - **Pair-verify sent the accessory's identifier** where the controller's belongs, so the accessory looked up a pairing under its own ID and found none. The client's pairing ID is now carried through from setup. - **The HTTP status check matched the whole header block**, so a 500 carrying `Content-Length: 1200` was read as success and the caller wrote video into a failed connection. It reads the status line. - `methods::PAIR_VERIFY` was `0x01`, which is a different real method (`PairSetupWithAuth`); it is `0x02`. The transient flag was `0x02`, which matches no defined flag anywhere; it is `0x10`. - A HAP back-off (error 0x03) carries a retry delay and is not an authentication failure. Reading it as one silently invalidated a round of hardware testing in #27. It is now a distinct error naming the delay. The SRP round-trip test could not catch a wrong shared constant, because the reference server in the test makes the same assumptions as the client — the exact hole that let a fabricated group survive. There is now a known-answer vector generated with srptools, the SRP engine pyatv itself drives HAP with, using srptools' own prime rather than ours. Flipping one hex digit of `SRP_N_HEX` fails the new test while the old round-trip still passes. One thing deliberately not changed: our `K` and `M1` pad values that pyatv/srptools encode minimally. They differ only when a value has a leading zero byte, so roughly one pair-setup in 85 would derive a different key — an intermittent failure that needs hardware to settle rather than a guess. The known-answer vector avoids the ambiguous case rather than pretending to decide it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The WebRTC path now has a test that negotiates two webrtcbins and asserts decoded frames arrive, and none of the plugins it needs were installed. `gstreamer1.0-nice` above all: without it webrtcbin constructs happily and then refuses every pad request. Also plugins-base for `videotestsrc`/`appsink`, plugins-ugly for `x264enc`, and libav for `avdec_h264`, which is the only decoder that registers on a runner with no DRM device. `libgstreamer-plugins-bad1.0-dev` has to come back, which the old comment predicted would happen if WebRTC was ever wired up: `gstreamer-webrtc-1.0.pc` comes from that package and nothing else, and its `libopencv-dev` dependency is a hard `Depends`, so `--no-install-recommends` does not avoid the cost. The job timeouts are raised to match, with the arithmetic written down so the two stay in step. `cross-platform-check` now selects with `--workspace --exclude`, so a new crate is covered by default instead of silently uncovered — the gap that hid a broken Windows build until #25. Its comment claimed openplay-receiver was excluded for needing GStreamer, which was false when written and is true now only by accident; it says the real reason. The concurrency group cancelled master runs it claimed to protect: `cancel-in-progress: false` does not stop GitHub cancelling a *pending* run in the same group, and eight of the last twenty-five master runs were cancelled that way, leaving five merged commits never built at their own SHA. Master pushes now get a group per commit. Also `--locked`, so a PR that edits Cargo.toml without refreshing Cargo.lock fails loudly; an MSRV job pinned to the declared version, which nothing verified before; and checkout/upload-artifact moved to v7. Note upload-artifact v5 would not have helped — its manifest still says node20, and v6 was the first real switch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…policy The .deb installed cleanly and then failed to launch. eframe/winit load the whole GUI stack through `dlopen`, so none of it appears as an ELF NEEDED entry and `dpkg-shlibdeps` cannot see any of it; twelve libraries were missing from Depends. `gstreamer1.0-plugins-base` was missing too — the shipped dependency was the *library*, while `appsink` and `videoconvert` live in the plugin package, satisfied until now only because plugins-bad happens to pull it in. `gstreamer1.0-nice` and `gstreamer1.0-libav` are added for the now-live WebRTC and decode paths. The D-Bus policy shipped a `context="default"` block granting every local user full method-call access to wpa_supplicant — scan, add and remove networks, reconfigure Wi-Fi. At the path the .deb uses it was inert, because wpa_supplicant's own deny sorts later; at `/etc/dbus-1/system.d/`, which the docs told users to use, `/etc` is read last and this rule won. Removed; the `group="netdev"` policy is what does the work, and wpa_supplicant already grants that group anyway. The polkit rule is deleted rather than kept. It matched `action.id == "fi.w1.wpa_supplicant1"`, which is a D-Bus service name and not a polkit action id — wpa_supplicant registers no polkit actions at all, so the rule could never fire, while the docs presented it as a prerequisite for Wi-Fi Direct and would have misled anyone debugging a P2P failure. The Flatpak manifest asked for `--socket=pulseaudio` for an application with no audio code anywhere, and two `--talk-name` lines for portal names that are reachable by default. A `skip:` list keeps flatpak-builder from copying a 14 GB `target/`. The manifest still cannot build offline without vendored sources; that is now stated in the file rather than left to be discovered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The documentation was wrong understating and overstating, and both kinds mislead. Understated: OpenPlay/WebRTC is wired to both binaries, `CertificateManager` and the TLS builders have callers, a certificate is generated on the receiver's first launch, and the receiver is no longer a static page. Roughly thirty passages said otherwise. Overstated, and more important: "Miracast sending works" is removed everywhere. It is wired end to end and has never been run against a real sink — the M1 status check alone meant negotiation could not reach M2. `docs/protocols.md` also claimed `openplay-crypto` no longer depends on rustls, while `tls.rs` is a rustls module. The consent prompt is documented as the security model, always with its limit stated in the same breath: there is no pairing and no authentication behind it, the mDNS fingerprint is unauthenticated, and pinning is not identity. `gstreamer1.0-nice` is documented as a hard requirement wherever dependencies are listed — the Fedora package is `libnice-gstreamer1`, not the plugins-bad package one might guess. The README and install.md dependency lists disagreed with each other and both omitted plugins-ugly, which supplies the x264 fallback they promise; they are now identical. Also corrected: macOS `CaptureSession` returns a hardcoded 1920x1080 rather than querying the display, the Homebrew formulae have been folded into `gstreamer`, and the state machines in `openplay-protocol` still have no callers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wires the OpenPlay (WebRTC) path to both binaries, and fixes the defects found on the way there.
SenderPipeline,ReceiverPipeline,SignalingServer,SignalingClientandReceiverAdvertiserall existed and had no callers. The sender'sProtocol::OpenPlayarm set a status string and stopped; the receiver window was static. This connects them, and adds the session layer that was missing between "two GStreamer graphs exist" and "a session".The security decision, first
A consent prompt gates every session. Nothing here authenticates a sender — mDNS is unauthenticated so anyone on the LAN can advertise a fingerprint, and the protocol's
PairingChallenge/Auth*messages have no implementation on either side. Wiring video in without a gate would have meant any device on the network could put pixels on someone's screen unprompted.So the receiver holds a session at
PendingConsentuntil a human presses Allow, the prompt names the sender and says plainly that OpenPlay cannot verify it, and there is deliberately no timeout that accepts. Please don't weaken that into an auto-accept before real pairing exists.The docs say this in the same breath as the limitation, rather than describing certificate pinning as authentication, which it is not.
Three silent failures that made the feature impossible
Each of these fails without an error message, which is why none had been noticed:
No receiver could ever be discovered.
ReceiverAdvertiserregistered with an empty address set, and mdns-sd then refuses to announce or answer queries — afterregister()returnsOkand logs "mDNS service registered". Every test in the crate round-tripped TXT records in memory, so a self-consistent encode/decode passed while nothing was discoverable. One missing.enable_addr_auto().The sender pipeline panicked on construction.
rtph264pay'saggregate-modeis a GEnum;.property(_, 1i32)panics inside.build(). Nothing had ever calledSenderPipeline::new, so nothing had hit it.Offers would have carried no video. Without a fixed RTP capsfilter the payloader advertises
payloadas a range, andwebrtcbincannot build an m-line from unfixed caps —create-offersucceeds and returns SDP with no media section. The session connects and shows nothing.Verified on this machine
Not just "it compiles":
avahi-browse -rpt _openplay._tcplists the receiver on every interface, IPv4 and IPv6Receiver found name=E2E Receiver addr=[…192.168.1.10] port=17292fpkey, handshake throughSignalingClientSessionAcceptwebrtc_loopback.rs: two webrtcbins reachConnectedand decoded RGBA frames arrive307 tests pass, up from 218. Two are new integration tests that exercise real sockets:
openplay-signaling/tests/loopback.rs(a full session over real loopback TLS) andopenplay-pipeline/tests/webrtc_loopback.rs.Still unverified: two separate physical machines, and screen capture on macOS/Windows.
Corrections to claims the repo made about itself
!resp.status.starts_with("200")againstRTSP/1.0 200 OK— always true, so negotiation could not reach M2. Beyond that, the session sentEndedimmediately afterReady, and the RTSP control socket was dropped right after M7. Seven defects fixed, a scripted fake sink added, 21 → 40 tests. It is now described as wired but unverified against hardware, which is the honest claim.gstreamer1.0-niceis a hard requirement and appeared nowhere — not in docs, packaging, or CI. Without itwebrtcbinconstructs fine and refuses every pad request.rust-version = "1.80"was fiction. The locked tree refuses to resolve below 1.88, at resolution rather than compilation. Nothing caught it because every CI job runs@stable.Depends, invisible todpkg-shlibdeps.Security fixes beyond the consent gate
context="default"allow granted every local user full access to wpa_supplicant. Inert at the path the .deb uses; live at/etc/dbus-1/system.d/, which the docs told users to use.control_channel.rs. Only the PIN and pair-verify flows reach them, which is why hardware testing of transient pairing never touched it.Reviewing this
Thirteen commits, each scoped to one crate or concern and ordered so dependencies come first. The commit messages carry the reasoning; this description is the summary.
Two things need a human:
sudo apt install gstreamer1.0-niceon any machine that runs the WebRTC path.docs/crypto.mdstandard of evidence was applied to the new crypto work — the AirPlay SRP known-answer vector is generated with an independent implementation, and flipping one hex digit ofSRP_N_HEXfails it while the old round-trip test still passes.