From fcf4c9fd63a5ae9a3c7fa0b0f0e2195c729c5e05 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 11 Aug 2026 22:17:41 +0200 Subject: [PATCH] fix(mgmt): unpair now revokes a LIVE session on both planes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unpair removed the certificate but left the revoked client's running session streaming until the client chose to leave. Now it is a complete revocation: - GameStream: when the removed certificate owns the active launch, the session is quit_session'd — the ENet control thread's ended-session arm gives the client the standard TERMINATION+disconnect. (An owner-less launch cannot be attributed and is left to the WP0 port teardown when the last pairing goes.) The endpoint docstring's long-standing caveat ('removes the client from the listing without severing its ability to reconnect') is retired: TLS handshakes complete by design, authorization is per-request, and a live session no longer survives its own revocation. - Native: session_status::stop_by_fingerprint signals the unpaired client's live session(s) to tear down deliberately (quit+stop), matched by the registry's client label — the fingerprint's 12-hex-char prefix for every pairable client; anonymous/TOFU sessions carry IP labels and are never touched (they have no pairing to revoke). (The unpair-didn't-PERSIST half of 'unpairing was broken' was already fixed in 13d57210 — save_paired was never called; this closes the other half.) Gates: Linux amd64 both flavors clippy --all-targets -D warnings clean; session_status 2/2 (new revocation test), the extended paired-clients test green in both flavors, native_pairing test green. --- CHANGELOG.md | 11 +++++ crates/punktfunk-host/src/mgmt/clients.rs | 28 +++++++++-- crates/punktfunk-host/src/mgmt/native.rs | 11 +++++ crates/punktfunk-host/src/mgmt/tests.rs | 33 +++++++++++++ crates/punktfunk-host/src/session_status.rs | 54 +++++++++++++++++++++ 5 files changed, 132 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 641a0a3e..d5edd584 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,17 @@ never touches the port, so a never-paired `--gamestream` host exposes no ENet at the management API's unpair endpoint never persisted (`save_paired` was missing), so an unpair lasted only until the next restart — fixed. `rusty_enet` is now pinned `=0.4.0`. +**Unpair is now a complete revocation, on both planes.** Beyond the persistence fix above, an +unpair used to leave the revoked client's LIVE session streaming until the client chose to +leave. Now: unpairing a GameStream client whose certificate owns the active launch ends that +session (the client gets the standard TERMINATION+disconnect, and unpair-all still closes the +ENet port); unpairing a native client deliberately stops its live punktfunk/1 session(s) +(matched by certificate fingerprint — anonymous/TOFU sessions are unaffected, they have no +pairing to revoke). The unpair endpoint's long-standing docstring caveat ("removes the client +from the listing without severing its ability to reconnect") is retired: TLS-level handshakes +still complete by design, but authorization is per-request and a live session no longer +survives its own revocation. + ### GameStream is now a cargo feature (compile-time isolation — packager-visible) The Moonlight-compat planes (nvhttp pairing, RTSP, the ENet control stream, `_nvstream` mDNS, diff --git a/crates/punktfunk-host/src/mgmt/clients.rs b/crates/punktfunk-host/src/mgmt/clients.rs index 1990692a..fc8a7656 100644 --- a/crates/punktfunk-host/src/mgmt/clients.rs +++ b/crates/punktfunk-host/src/mgmt/clients.rs @@ -79,11 +79,12 @@ pub(crate) fn client_info(der: &[u8]) -> PairedClient { /// Unpair a client /// /// Removes the client's certificate from the pairing store (persisted — the removal survives a -/// host restart). Removing the last pairing also closes the GameStream ENet control port -/// (UDP 47999), which is only bound while at least one pairing exists. Caveat: the nvhttp TLS -/// layer does not yet reject unlisted certificates (`gamestream/tls.rs` accepts any well-formed -/// client cert — a planned hardening step), so until that lands this removes the client -/// from the listing without severing its ability to reconnect. +/// host restart). Revocation is complete: a LIVE GameStream session owned by this certificate is +/// ended (the client gets the standard TERMINATION+disconnect), and removing the last pairing +/// also closes the ENet control port (UDP 47999), which is only bound while at least one pairing +/// exists. The nvhttp TLS layer still completes a handshake with any well-formed client cert BY +/// DESIGN (authorization is per-request via the paired-fingerprint check) — an unpaired client +/// that reconnects is rejected at every post-pair endpoint. #[utoipa::path( delete, path = "/clients/{fingerprint}", @@ -119,6 +120,23 @@ pub(crate) async fn unpair_client( // re-open the control port. crate::gamestream::save_paired(&paired); drop(paired); + // Revocation reaches a LIVE session too: a mid-stream client whose pairing was just + // removed must not keep streaming until it chooses to leave. Clearing the launch makes + // the ENet control thread give it the standard TERMINATION+disconnect farewell. (An + // owner-less launch — the cert was unreadable at /launch — cannot be attributed and is + // left to the port teardown below when this was the last pairing.) + let removed_fp: Option<[u8; 32]> = hex::decode(&fingerprint) + .ok() + .and_then(|v| v.try_into().ok()); + let live_owner = st + .app + .launch + .lock() + .unwrap_or_else(|e| e.into_inner()) + .and_then(|l| l.owner_fp); + if removed_fp.is_some() && removed_fp == live_owner { + st.app.quit_session("client unpaired"); + } // The last pairing going away closes the ENet control port (rust-safety WP0). A // no-op while other pairings remain — or on a native-only host, where the gate is // never armed. diff --git a/crates/punktfunk-host/src/mgmt/native.rs b/crates/punktfunk-host/src/mgmt/native.rs index e7511993..3de638a0 100644 --- a/crates/punktfunk-host/src/mgmt/native.rs +++ b/crates/punktfunk-host/src/mgmt/native.rs @@ -231,6 +231,17 @@ pub(crate) async fn unpair_native_client( }; match np.remove(&fingerprint) { Ok(true) => { + // Revocation reaches a LIVE session too: without this, a mid-stream client kept + // streaming after its pairing was removed, until it chose to disconnect. + let stopped = + crate::session_status::stop_by_fingerprint(&fingerprint.to_ascii_lowercase()); + if stopped > 0 { + tracing::info!( + fingerprint, + stopped, + "unpair: live native session(s) stopped" + ); + } tracing::info!(fingerprint, "management API: native client unpaired"); StatusCode::NO_CONTENT.into_response() } diff --git a/crates/punktfunk-host/src/mgmt/tests.rs b/crates/punktfunk-host/src/mgmt/tests.rs index 81800a71..dad4b379 100644 --- a/crates/punktfunk-host/src/mgmt/tests.rs +++ b/crates/punktfunk-host/src/mgmt/tests.rs @@ -828,6 +828,27 @@ async fn paired_clients_list_and_unpair() { .unwrap(); assert_eq!(send(&app, bad).await.0, StatusCode::BAD_REQUEST); + // A LIVE session owned by this client: unpair is a revocation, so it must END the session, + // not just delist the cert — before this, a mid-stream client kept streaming after unpair + // until it chose to leave. + { + use std::sync::atomic::Ordering; + // owner_fp is the sha256 of the cert DER — exactly the bytes `fingerprint` encodes. + let mut owner = [0u8; 32]; + owner.copy_from_slice(&hex::decode(&fingerprint).unwrap()); + state.streaming.store(true, Ordering::SeqCst); + *state.launch.lock().unwrap() = Some(LaunchSession { + gcm_key: [0; 16], + rikeyid: 0, + width: 1920, + height: 1080, + fps: 60, + appid: 1, + peer_ip: None, + owner_fp: Some(owner), + }); + } + // Unpair (uppercase hex must match too) → 204, list empties, second delete → 404. let del = |fp: String| { axum::http::Request::delete(format!("/api/v1/clients/{fp}")) @@ -838,6 +859,18 @@ async fn paired_clients_list_and_unpair() { send(&app, del(fingerprint.to_uppercase())).await.0, StatusCode::NO_CONTENT ); + { + use std::sync::atomic::Ordering; + assert!( + state.launch.lock().unwrap().is_none(), + "unpair must end the revoked client's live session" + ); + assert!(!state.streaming.load(Ordering::SeqCst)); + assert!( + state.quit.load(Ordering::SeqCst), + "the teardown is deliberate (quit), not a drop" + ); + } let (_, body) = send(&app, get_req("/api/v1/clients")).await; assert_eq!(body, serde_json::json!([])); assert_eq!(send(&app, del(fingerprint)).await.0, StatusCode::NOT_FOUND); diff --git a/crates/punktfunk-host/src/session_status.rs b/crates/punktfunk-host/src/session_status.rs index fd34b29a..bb934203 100644 --- a/crates/punktfunk-host/src/session_status.rs +++ b/crates/punktfunk-host/src/session_status.rs @@ -337,6 +337,24 @@ pub fn stop_all() { /// end-game-on-session-end policy sees an intent rather than a network drop. (Before this, a /// management stop was indistinguishable from a client vanishing, which left the display lingering /// for a session nobody was coming back to.) +/// Signals the live native sessions belonging to `fp_hex` (lowercase hex cert SHA-256) to tear +/// down **deliberately** — the unpair path's revocation reaching a mid-stream client, which must +/// not keep streaming just because it was already connected when its pairing was removed. +/// Matching is by the registry's client label, which for every pairable client is the +/// fingerprint's 12-hex-char prefix (an anonymous/TOFU session carries an IP label and never +/// matches — it has no pairing to revoke). Returns how many sessions were signalled. +pub fn stop_by_fingerprint(fp_hex: &str) -> usize { + let mut n = 0; + for s in registry().lock().unwrap().iter() { + if s.client.len() == 12 && fp_hex.starts_with(s.client.as_str()) { + s.quit.store(true, Ordering::SeqCst); + s.stop.store(true, Ordering::SeqCst); + n += 1; + } + } + n +} + pub fn stop_all_quit() { for s in registry().lock().unwrap().iter() { s.quit.store(true, Ordering::SeqCst); @@ -356,6 +374,42 @@ pub fn force_idr_all() { mod tests { use super::*; + fn fake_session(client: &str) -> (LiveSessionGuard, Arc, Arc) { + let stop = Arc::new(AtomicBool::new(false)); + let quit = Arc::new(AtomicBool::new(false)); + let guard = register(Registration { + mode: Arc::new(AtomicU64::new(0)), + bitrate_kbps: Arc::new(AtomicU32::new(20_000)), + codec: Codec::H265, + stop: stop.clone(), + quit: quit.clone(), + force_idr: Arc::new(AtomicBool::new(false)), + client: client.into(), + client_name: None, + hdr: false, + ttff_ms: Arc::new(AtomicU32::new(0)), + last_resize_ms: Arc::new(AtomicU32::new(0)), + game: None, + }); + (guard, stop, quit) + } + + /// Unpair must revoke a LIVE session — matched by the client label (the fingerprint's + /// 12-hex-char prefix), deliberately (quit + stop), and precisely: another client's session + /// and an anonymous (IP-labelled) session stay untouched. + #[test] + fn stop_by_fingerprint_revokes_exactly_the_unpaired_client() { + let fp = "aabbccddeeff00112233445566778899aabbccddeeff00112233445566778899"; + let (_g1, stop1, quit1) = fake_session(&fp[..12]); + let (_g2, stop2, _q2) = fake_session("112233445566"); // a different paired client + let (_g3, stop3, _q3) = fake_session("192.168.1.50"); // anonymous: IP label, never matches + + assert_eq!(stop_by_fingerprint(fp), 1); + assert!(stop1.load(Ordering::SeqCst) && quit1.load(Ordering::SeqCst)); + assert!(!stop2.load(Ordering::SeqCst)); + assert!(!stop3.load(Ordering::SeqCst)); + } + /// A Moonlight client's game has no live-session entry to hang off, so without the compat-plane /// slot it would be missing from `/status` entirely — the Dashboard would show a stream with no /// game while one was plainly running. Publishing must also be strictly scoped to the stream: the