From bd25f5e02f8331b046c0dbb37e63ebb047b68ab0 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 9 Jun 2026 22:00:22 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20M2=20=E2=80=94=20harden=20the=20manageme?= =?UTF-8?q?nt=20API=20after=20adversarial=20review?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five confirmed findings from a 46-agent review panel: - Empty --mgmt-token no longer satisfies the non-loopback token gate (critical: 'Bearer ' with an empty token authenticated; parse_serve now bails on blank tokens and mgmt::run treats blank as none) - axum's built-in body rejections (400/415/422) now wear the documented ApiError envelope via an ApiJson extractor, and the spec documents them - GET /health carries security([{}]) in the spec, matching the server's auth exemption - unpairClient's description no longer claims revocation the TLS layer doesn't enforce yet (gamestream/tls.rs accepts any cert — known gap) - CLAUDE.md/README.md no longer reference the deleted web.rs Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 14 ++++--- README.md | 9 +++-- crates/lumen-host/src/main.rs | 11 +++++- crates/lumen-host/src/mgmt.rs | 72 ++++++++++++++++++++++++++++++++--- docs/api/openapi.json | 29 ++++++++++++-- 5 files changed, 115 insertions(+), 20 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3260411..ccd98e4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -16,9 +16,11 @@ Low-latency desktop streaming stack, Linux-first, with a shared Rust protocol co round-trips every access unit through a `lumen_core` host→client session (0 mismatches). See [`docs/linux-setup.md`](docs/linux-setup.md); the code is in `crates/lumen-host/src/{m0,capture,encode}.rs` (+ `capture/linux.rs`, `encode/linux.rs`). -- **The remaining host backends are `#[cfg(target_os = "linux")]` stubs** — KWin/Mutter - virtual displays (`vdisplay.rs`), libei/uinput input (`inject.rs`), web/pairing - (`web.rs`). They compile everywhere but `bail!` until implemented. This is **M2**. +- **M2 is in flight.** The GameStream control plane lives in `gamestream/` (mDNS, + serverinfo, pairing, RTSP, ENet control, video/audio streams) and the management REST + API in `mgmt.rs`; the remaining `#[cfg(target_os = "linux")]` backends — KWin/Mutter + virtual displays (`vdisplay.rs`), libei/uinput input (`inject.rs`) — compile everywhere + and `bail!` where unimplemented. ## Build / test / run @@ -83,9 +85,9 @@ tokio runtime) + `pipewire` **0.9** (must match ashpd's; not 0.10) + `ffmpeg-nex ## Next: M2 — P1 host to a stock Moonlight client Wire M0's capture→encode pipeline (`m0.rs` / `pipeline.rs`) into a streaming host: KWin -virtual output (`vdisplay.rs`, study KRdp), `serverinfo`/pairing/RTSP (`web.rs`) enough for -a real Moonlight client, input via reis/uinput (`inject.rs`). The module seams exist and -`bail!` today. +virtual output (`vdisplay.rs`, study KRdp), `serverinfo`/pairing/RTSP +(`gamestream/{nvhttp,pairing,rtsp}.rs`) enough for a real Moonlight client, input via +reis/uinput (`inject.rs`), management/config REST API (`mgmt.rs`). ## Conventions diff --git a/README.md b/README.md index 62346ed..362b3a9 100644 --- a/README.md +++ b/README.md @@ -24,16 +24,17 @@ loopback round-trip under loss, property tests, and a **C ABI harness**) passes macOS/aarch64. **M0 is done:** `lumen-host` captures a headless wlroots output via the ScreenCast portal + PipeWire, encodes it with NVENC, writes a playable H.265 file, and round-trips every access unit through a `lumen_core` host→client session (see -`docs/linux-setup.md`). The remaining Linux host backends (KWin/Mutter virtual displays, -libei input, web/pairing) are `#[cfg(target_os = "linux")]` seams — defined and compiling, -implementations pending (M2). +`docs/linux-setup.md`). M2 is in flight: the GameStream control plane (`gamestream/`) and +the management REST API (`mgmt.rs`, OpenAPI spec in `docs/api/`) are implemented; the +remaining Linux host backends (KWin/Mutter virtual displays, libei input) are +`#[cfg(target_os = "linux")]` seams — defined and compiling, implementations pending. ## Layout ``` crates/ lumen-core/ protocol · FEC · pacing · crypto — the C ABI (lib + cdylib + staticlib) - lumen-host/ Linux host: vdisplay · capture · encode · inject · web (cfg-gated) + lumen-host/ Linux host: vdisplay · capture · encode · inject · gamestream · mgmt lumen-client-rs/ reference client (M4): VAAPI decode + wgpu present clients/{apple,android}/ native client scaffolds (import lumen_core.h) include/lumen_core.h cbindgen-generated C header (checked in) diff --git a/crates/lumen-host/src/main.rs b/crates/lumen-host/src/main.rs index 30bb687..914d32c 100644 --- a/crates/lumen-host/src/main.rs +++ b/crates/lumen-host/src/main.rs @@ -145,7 +145,16 @@ fn parse_serve(args: &[String]) -> Result { .parse() .map_err(|_| anyhow::anyhow!("bad --mgmt-bind (want IP:PORT)"))? } - "--mgmt-token" => opts.token = Some(next()?), + "--mgmt-token" => { + let token = next()?; + // An empty token would satisfy the non-loopback "token required" guard + // while authenticating nobody (or, worse, everybody) — refuse it loudly + // rather than letting `--mgmt-token "$UNSET_VAR"` ship a dead credential. + if token.trim().is_empty() { + bail!("--mgmt-token must not be empty"); + } + opts.token = Some(token); + } "-h" | "--help" => { print_usage(); std::process::exit(0); diff --git a/crates/lumen-host/src/mgmt.rs b/crates/lumen-host/src/mgmt.rs index 7970eb4..f77828f 100644 --- a/crates/lumen-host/src/mgmt.rs +++ b/crates/lumen-host/src/mgmt.rs @@ -68,7 +68,10 @@ struct MgmtState { /// Run the management API server (control plane; spawned alongside the nvhttp servers). pub async fn run(state: Arc, opts: Options) -> Result<()> { - if opts.token.is_none() && !opts.bind.ip().is_loopback() { + // A blank token is no token: it must neither satisfy the non-loopback guard below nor + // become a credential an empty `Authorization: Bearer ` header would match. + let token = opts.token.filter(|t| !t.trim().is_empty()); + if token.is_none() && !opts.bind.ip().is_loopback() { bail!( "management API bind {} is not loopback — set --mgmt-token (or LUMEN_MGMT_TOKEN) \ to expose it beyond this machine", @@ -77,10 +80,10 @@ pub async fn run(state: Arc, opts: Options) -> Result<()> { } tracing::info!( addr = %opts.bind, - auth = if opts.token.is_some() { "bearer" } else { "none (loopback)" }, + auth = if token.is_some() { "bearer" } else { "none (loopback)" }, "management API listening (docs at /api/docs, spec at /api/v1/openapi.json)" ); - let app = app(state, opts.token, opts.bind.port()); + let app = app(state, token, opts.bind.port()); axum_server::bind(opts.bind) .serve(app.into_make_service()) .await @@ -335,6 +338,25 @@ fn api_error(status: StatusCode, message: &str) -> Response { .into_response() } +/// `axum::Json` whose rejections (bad JSON → 400/422, wrong content-type → 415) are +/// rewrapped in the [`ApiError`] envelope, keeping "every non-2xx body is `ApiError`" true. +struct ApiJson(T); + +impl axum::extract::FromRequest for ApiJson +where + Json: axum::extract::FromRequest, + S: Send + Sync, +{ + type Rejection = Response; + + async fn from_request(req: Request, state: &S) -> Result { + match Json::::from_request(req, state).await { + Ok(Json(value)) => Ok(ApiJson(value)), + Err(rejection) => Err(api_error(rejection.status(), &rejection.body_text())), + } + } +} + // --------------------------------------------------------------------------------------- // Auth // --------------------------------------------------------------------------------------- @@ -377,6 +399,8 @@ fn token_eq(presented: &str, expected: &str) -> bool { path = "/health", tag = "host", operation_id = "getHealth", + // Override the document-global bearerAuth: this route is exempt in `require_auth`. + security(()), responses((status = OK, description = "Host is up", body = Health)) )] async fn get_health() -> Json { @@ -494,7 +518,10 @@ fn client_info(der: &[u8]) -> PairedClient { /// Unpair a client /// -/// Removes the pinned certificate; the client must pair again to reconnect. +/// Removes the client's certificate from the pairing store. 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. #[utoipa::path( delete, path = "/clients/{fingerprint}", @@ -566,14 +593,16 @@ async fn get_pairing_status(State(st): State>) -> Json>, - Json(req): Json, + ApiJson(req): ApiJson, ) -> Response { let pin = req.pin.trim(); if pin.is_empty() || pin.len() > 16 || !pin.bytes().all(|b| b.is_ascii_digit()) { @@ -832,6 +861,31 @@ mod tests { send(&app, post(r#"{"pin":"1234"}"#)).await.0, StatusCode::CONFLICT ); + + // axum's own body rejections must still wear the ApiError envelope (ApiJson). + let (status, body) = send(&app, post("{not json")).await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert!(body["error"].is_string(), "syntax error: {body}"); + let (status, body) = send(&app, post(r#"{"wrong":"shape"}"#)).await; + assert_eq!(status, StatusCode::UNPROCESSABLE_ENTITY); + assert!(body["error"].is_string(), "schema mismatch: {body}"); + let no_ct = axum::http::Request::post("/api/v1/pair/pin") + .body(Body::from(r#"{"pin":"1234"}"#)) + .unwrap(); + let (status, body) = send(&app, no_ct).await; + assert_eq!(status, StatusCode::UNSUPPORTED_MEDIA_TYPE); + assert!(body["error"].is_string(), "media type: {body}"); + } + + /// A blank token must not satisfy the "non-loopback requires a token" guard. + #[tokio::test] + async fn blank_token_rejected_for_public_bind() { + let opts = Options { + bind: "0.0.0.0:0".parse().unwrap(), + token: Some(" ".into()), + }; + let err = run(test_state(), opts).await.unwrap_err(); + assert!(err.to_string().contains("not loopback"), "{err}"); } #[tokio::test] @@ -905,6 +959,12 @@ mod tests { op_ids.dedup(); assert_eq!(total, op_ids.len(), "duplicate operationIds"); assert!(doc["components"]["securitySchemes"]["bearerAuth"].is_object()); + // The health probe overrides the document-global bearer requirement (the server + // exempts it in `require_auth`; the spec must agree). + assert_eq!( + doc["paths"]["/api/v1/health"]["get"]["security"], + serde_json::json!([{}]) + ); let checked_in = include_str!("../../../docs/api/openapi.json"); assert_eq!( diff --git a/docs/api/openapi.json b/docs/api/openapi.json index fcac4e5..7568cc0 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -53,7 +53,7 @@ "clients" ], "summary": "Unpair a client", - "description": "Removes the pinned certificate; the client must pair again to reconnect.", + "description": "Removes the client's certificate from the pairing store. Caveat: the nvhttp TLS layer\ndoes not yet reject unlisted certificates (`gamestream/tls.rs` accepts any well-formed\nclient cert — a planned hardening step), so until that lands this removes the client\nfrom the listing without severing its ability to reconnect.", "operationId": "unpairClient", "parameters": [ { @@ -122,7 +122,10 @@ } } } - } + }, + "security": [ + {} + ] } }, "/api/v1/host": { @@ -211,7 +214,7 @@ "description": "PIN delivered to the waiting handshake" }, "400": { - "description": "Malformed PIN", + "description": "Malformed PIN or unparseable JSON body", "content": { "application/json": { "schema": { @@ -239,6 +242,26 @@ } } } + }, + "415": { + "description": "Body is not application/json", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ApiError" + } + } + } + }, + "422": { + "description": "JSON body does not match the schema", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ApiError" + } + } + } } } }