From 49b5ffa2d8c63a630c6ddb450a8f0ae9942c7b82 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Mon, 24 Aug 2026 20:30:25 +0200 Subject: [PATCH 1/2] fix(web,tray,host): the console served the identity nothing pins, and the tray called it dead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Linux operator saw "Open web console (not responding)" in the tray next to a tooltip reading "idle", and the console would not load in a browser either. The host has kept two identities since the identity split (crate::identity): native-cert.pem/native-key.pem (P-256, real SANs — what the native QUIC plane, the mgmt API and every native client pin) and the legacy cert.pem/key.pem (RSA, CN=punktfunk, NO SAN, kept byte-stable for Moonlight). The web console never followed the split. Every launcher — the systemd unit, the NixOS module, the Windows service supervisor, web-run.cmd, the Steam Deck installer — still names the LEGACY pair, and none of them CAN choose: `Environment=` has no "this file, else that one". So the console served a certificate with no SAN at all, which costs twice over: * browsers reject a CN-only cert outright (ERR_CERT_COMMON_NAME_INVALID), so the console the operator was told to open does not load; * the tray's loopback liveness probe reused the agent PINNED to the mgmt identity — the native cert — so rustls refused the handshake and a perfectly healthy console was labelled "not responding". The "idle" tooltip beside it is the proof: the same agent reached mgmt fine on the very same tick. The entry is the one place every launcher routes through, so the choice is made there: prefer the native sibling pair when both files exist, as a PAIR or not at all (a native cert with the legacy key completes no handshake with anyone). A host that never took the split has no native pair on disk and falls through unchanged, as does an operator-supplied cert under any other name. This also hands the bundled bun the smaller secret: on a default build key.pem is the Moonlight pairing SIGNING key, native-key.pem is only a TLS key. The tray's console probe loses its pin rather than gaining a second one. It is a different server and there is no rule that it presents the mgmt certificate — an operator fronting the console with their own LAN-CA cert would have hit this just as squarely. The probe sends no credentials, reads no body, and decides only a menu label, so there is nothing for a pin to protect. `serve` now resolves the native identity BEFORE minting the legacy one. That closes a first-run window where the console (which waits on cert.pem) could start between the two writes and serve the SAN-less cert for the rest of the boot, and it fixes a second latent fault: with cert.pem missing but native clients paired, the old order let load_or_create mint a brand-new cert.pem that load_or_adopt then adopted while logging that it was preserving their pins. Verified against the built server: configured exactly as the shipped unit does (PUNKTFUNK_UI_TLS_CERT=.../cert.pem), it now serves the P-256 cert with DNS:localhost/IP:127.0.0.1; with the native pair removed it serves the RSA cert as before. 14/14 web tests pass, biome and rustfmt clean. --- crates/punktfunk-host/src/gamestream/mod.rs | 30 ++++++---- crates/punktfunk-host/src/windows/service.rs | 5 +- crates/punktfunk-tray/src/status.rs | 21 ++++++- packaging/nix/nixos-module.nix | 5 ++ scripts/punktfunk-web.service | 9 ++- web/nitro-entry/bun-https.mjs | 29 +++++++--- web/nitro-entry/tls-paths.mjs | 55 +++++++++++++++++++ web/nitro-entry/tls-paths.test.ts | 58 ++++++++++++++++++++ web/package.json | 2 +- web/vite.config.ts | 2 +- 10 files changed, 191 insertions(+), 25 deletions(-) create mode 100644 web/nitro-entry/tls-paths.mjs create mode 100644 web/nitro-entry/tls-paths.test.ts diff --git a/crates/punktfunk-host/src/gamestream/mod.rs b/crates/punktfunk-host/src/gamestream/mod.rs index 98db83b2a..e5506f440 100644 --- a/crates/punktfunk-host/src/gamestream/mod.rs +++ b/crates/punktfunk-host/src/gamestream/mod.rs @@ -419,6 +419,26 @@ pub fn serve( // The shared streaming-stats recorder: one handle for the mgmt API, the GameStream encode loop // (via `AppState`), and the native punktfunk/1 loops (passed to `native::serve`). let stats = crate::stats_recorder::StatsRecorder::new(crate::stats_recorder::default_dir()); + // The native plane always runs, so the shared native-pairing handle (linking the QUIC ceremony + // and the management API) always exists. + let np = Arc::new( + crate::native_pairing::NativePairing::load_with(None, None, false) + .context("native pairing store")?, + ); + // The identity the native QUIC plane and the mgmt API present (the identity split): P-256 on + // hosts no native client ever pinned, the legacy RSA cert otherwise — resolved ONCE here so + // the two planes cannot race the first-run adoption. See `crate::identity`. + // + // Resolved BEFORE the legacy GameStream identity below, and that order is load-bearing twice + // over. (1) The web console gates its start on `cert.pem` existing and then serves the native + // pair sitting next to it (web/nitro-entry/tls-paths.mjs); minting the legacy pair first leaves + // a first-run window where the console starts, finds no native pair, and serves the SAN-less + // RSA cert no browser accepts — for the rest of that boot. This way `cert.pem` existing implies + // the native pair does too. (2) In the degenerate case (native clients paired, but the cert + // they pinned is gone from disk) the old order let `load_or_create` mint a BRAND-NEW cert.pem + // that `load_or_adopt` then adopted while logging that it was preserving their pins — stranding + // them silently. Reading the dir first means that case reaches the branch written for it. + let native_ident = crate::identity::load_or_adopt(&np).context("native host identity")?; #[cfg(feature = "gamestream")] let state = { let identity = cert::ServerIdentity::load_or_create().context("host certificate")?; @@ -426,20 +446,10 @@ pub fn serve( }; #[cfg(not(feature = "gamestream"))] let state = Arc::new(AppState::new(host, stats.clone())); - // The native plane always runs, so the shared native-pairing handle (linking the QUIC ceremony - // and the management API) always exists. - let np = Arc::new( - crate::native_pairing::NativePairing::load_with(None, None, false) - .context("native pairing store")?, - ); // WP13: hand the GameStream planes the grants registry — the nvhttp launch surface and the // ENet control thread resolve a Moonlight fingerprint's mask against the same registry the // native plane enforces (design §8: it keys on fingerprint hex and serves both stores). let _ = state.access.set(np.clone()); - // The identity the native QUIC plane and the mgmt API present (the identity split): P-256 on - // hosts no native client ever pinned, the legacy RSA cert otherwise — resolved ONCE here so - // the two planes cannot race the first-run adoption. See `crate::identity`. - let native_ident = crate::identity::load_or_adopt(&np).context("native host identity")?; tracing::info!( hostname = %state.host.hostname, uniqueid = %state.host.uniqueid, diff --git a/crates/punktfunk-host/src/windows/service.rs b/crates/punktfunk-host/src/windows/service.rs index 03af1afa8..dc55ab6f6 100644 --- a/crates/punktfunk-host/src/windows/service.rs +++ b/crates/punktfunk-host/src/windows/service.rs @@ -1091,7 +1091,10 @@ fn spawn_web(cfg: &WebConfig, data: &Path, job: HANDLE) -> Result { // The /api proxy hop to the host's loopback HTTPS mgmt API. The host's self-signed cert is // accepted only inside the proxy code (per-request TLS), never process-wide. ("PUNKTFUNK_MGMT_URL", mgmt_url), - // Serve HTTPS with the host's own identity cert; mark the session cookie Secure. + // Serve HTTPS with the host's own identity cert; mark the session cookie Secure. Names the + // LEGACY pair — the console prefers the native sibling when it exists + // (web/nitro-entry/tls-paths.mjs), which is also what the gate above ends up waiting for: + // `serve` resolves the native identity before minting this one. ( "PUNKTFUNK_UI_TLS_CERT", data.join("cert.pem").to_string_lossy().into_owned(), diff --git a/crates/punktfunk-tray/src/status.rs b/crates/punktfunk-tray/src/status.rs index df0c9dec3..3fbc7f5b8 100644 --- a/crates/punktfunk-tray/src/status.rs +++ b/crates/punktfunk-tray/src/status.rs @@ -208,7 +208,22 @@ fn poll_loop( // that proves the server is answering, and the agent below refuses redirects so the probe is // exactly one round trip. (A 302 still counts as up via the `Status` arm in `probe_console`.) let console_url = format!("https://127.0.0.1:{web_port}/login"); - let agent = agent(load_pin()); + // Named, not `agent`: shadowing the fn (as this did while there was only one agent) would make + // the second call below resolve to this binding instead. + let mgmt_agent = agent(load_pin()); + // The console probe gets its OWN, UNPINNED agent. It is a different server from the mgmt API + // and there is no rule that it presents the same certificate: it served the legacy `cert.pem` + // while mgmt served the native one (the identity split), so the pinned agent refused the + // handshake and every identity-split host showed "Open web console (not responding)" over a + // perfectly healthy console — next to a tooltip reading "idle", because the same agent reached + // mgmt fine (field report 2026-08-24). An operator fronting the console with their own LAN-CA + // cert would have hit it just as squarely, so the coupling goes rather than the symptom. + // + // Nothing is lost by dropping the pin: this probe sends no credentials, reads no body, and + // decides only a menu LABEL. A local port-squatter could make that label read "up" — but the + // entry is always present and always opens the same URL regardless of the probe, so it gains + // nothing it did not already have. + let console_agent = agent(None); let mut last: Option<(TrayStatus, bool)> = None; // When the summary became unreachable while the service was running (grace anchor). // Runs for the process lifetime (the tray exits by process exit; nothing to unwind). @@ -220,7 +235,7 @@ fn poll_loop( loop { let svc = probe_service(); let summary = if svc == ServiceState::Running { - let s = fetch_summary(&agent, &summary_url()); + let s = fetch_summary(&mgmt_agent, &summary_url()); match s { Some(_) => unreachable_since = None, None if unreachable_since.is_none() => unreachable_since = Some(Instant::now()), @@ -233,7 +248,7 @@ fn poll_loop( }; let grace_expired = unreachable_since.is_some_and(|t| t.elapsed() >= START_GRACE); let status = map_status(&svc, summary, grace_expired); - let console_up = if probe_console(&agent, &console_url) { + let console_up = if probe_console(&console_agent, &console_url) { console_misses = 0; true } else { diff --git a/packaging/nix/nixos-module.nix b/packaging/nix/nixos-module.nix index 2db4b9a42..1ecf8acdf 100644 --- a/packaging/nix/nixos-module.nix +++ b/packaging/nix/nixos-module.nix @@ -667,6 +667,11 @@ in HOST = "0.0.0.0"; # Serve HTTPS with the host's own identity cert (the anchor native clients already pin) and # mark the session cookie Secure. The host's `serve` writes these PEMs. + # + # These name the LEGACY pair; the server prefers the native sibling + # (native-cert.pem/native-key.pem) when it exists, because a generated unit cannot express + # "this file, else that one" any more than the hand-written one can. The choice is made in + # web/nitro-entry/tls-paths.mjs — keep this in step with scripts/punktfunk-web.service. PUNKTFUNK_UI_TLS_CERT = "%h/.config/punktfunk/cert.pem"; PUNKTFUNK_UI_TLS_KEY = "%h/.config/punktfunk/key.pem"; PUNKTFUNK_UI_SECURE = "1"; diff --git a/scripts/punktfunk-web.service b/scripts/punktfunk-web.service index 39c4ef46f..42b2d4e0d 100644 --- a/scripts/punktfunk-web.service +++ b/scripts/punktfunk-web.service @@ -2,7 +2,8 @@ # # Installed by the punktfunk-web .deb to /usr/lib/systemd/user/. AUTO-WIRED — no env editing: # it sources the host's mgmt token + the generated login password, serves HTTPS (HTTP/1.1 over TLS) -# with the host's own identity cert (~/.config/punktfunk/{cert,key}.pem), and points the /api proxy +# with the host's own identity cert (~/.config/punktfunk/native-{cert,key}.pem, falling back to the +# legacy {cert,key}.pem — see the PUNKTFUNK_UI_TLS_CERT note below), and points the /api proxy # at the host's loopback HTTPS mgmt API. The self-signed cert is accepted only for that loopback hop, # scoped inside the proxy code (Bun per-request TLS) — no process-wide NODE_TLS_REJECT_UNAUTHORIZED. # Enable per user: @@ -39,6 +40,12 @@ Environment=HOST=0.0.0.0 # Serve HTTPS (HTTP/1.1 over TLS) with the host's own identity cert; mark the # session cookie Secure. The host's `serve` writes these PEMs; if absent at start the unit fails and # Restart retries (same as the mgmt-token wait above) rather than silently serving plain HTTP. +# +# These name the LEGACY pair and the server prefers the native sibling +# (native-cert.pem/native-key.pem) whenever it exists — `Environment=` cannot express "this file, +# else that one", so the choice is made in web/nitro-entry/tls-paths.mjs, which is the one place +# every launcher routes through. Don't "fix" these to the native names: a host that never took the +# identity split has no native pair, and the fallback lives on the other side of this handoff. Environment=PUNKTFUNK_UI_TLS_CERT=%h/.config/punktfunk/cert.pem Environment=PUNKTFUNK_UI_TLS_KEY=%h/.config/punktfunk/key.pem Environment=PUNKTFUNK_UI_SECURE=1 diff --git a/web/nitro-entry/bun-https.mjs b/web/nitro-entry/bun-https.mjs index b4384e3a2..0cf7d5501 100644 --- a/web/nitro-entry/bun-https.mjs +++ b/web/nitro-entry/bun-https.mjs @@ -8,8 +8,9 @@ // // NOTE on HTTP/2 + HTTP/3: NOT offered here, on purpose. `Bun.serve` has no HTTP/2 server, and // HTTP/3 (which Bun *can* do) is useless to a browser against this cert: QUIC refuses any cert error, -// and the host identity cert is a CN-only, no-SAN, self-signed cert (correct for native fingerprint -// PINNING, rejected by browsers). So browsers stay on HTTP/1.1 regardless — advertising h3 would just +// and the host identity is SELF-SIGNED whichever pair we serve — the native one carries real SANs, so +// a browser gets past the name check, but never past the untrusted issuer (and the legacy fallback is +// CN-only with no SAN, which fails both). So browsers stay on HTTP/1.1 regardless — advertising h3 would just // dangle an `Alt-Svc` no browser can use. Real h2/h3 would need a browser-TRUSTED, SAN-matching cert // (a local CA installed per device) fronted by a server that speaks them (e.g. Caddy) — deliberately // out of scope for a LAN console; TLS (no cleartext login/session) is the win. @@ -17,14 +18,16 @@ // TWO LISTENERS, on purpose — see `PLUGIN ORIGIN` below. // // Env (set by the launchers / the systemd unit — see web.env.example): -// PUNKTFUNK_UI_TLS_CERT / _KEY PEM file paths (the host's cert.pem / key.pem). BOTH set ⇒ HTTPS. -// Unset ⇒ plain HTTP (local dev only). +// PUNKTFUNK_UI_TLS_CERT / _KEY PEM file paths (the host's cert.pem / key.pem — the native +// sibling pair is preferred when present, see tls-paths.mjs). +// BOTH set ⇒ HTTPS. Unset ⇒ plain HTTP (local dev only). // PORT / HOST standard Nitro bind (3000 / 0.0.0.0). // PUNKTFUNK_UI_PLUGIN_PORT the plugin-UI origin's port (default: console port + 1). import "#nitro-internal-pollyfills"; import wsAdapter from "crossws/adapters/bun"; import { useNitroApp } from "nitropack/runtime"; import { startScheduleRunner } from "nitropack/runtime/internal"; +import { resolveUiTlsPaths } from "./tls-paths.mjs"; const nitroApp = useNitroApp(); const ws = import.meta._websocket @@ -75,8 +78,15 @@ const PEER_IP_HEADER = "x-pf-peer-ip"; const LISTENER_HEADER = "x-pf-listener"; // TLS from the host's identity cert (file PATHS → Bun.file, not PEM-in-env). Absent ⇒ plain HTTP. -const certPath = process.env.PUNKTFUNK_UI_TLS_CERT; -const keyPath = process.env.PUNKTFUNK_UI_TLS_KEY; +// +// The launchers all name the LEGACY cert.pem/key.pem pair and cannot express a fallback, so the +// choice between the host's two identities is made here — see tls-paths.mjs for why the native +// pair is the right one to serve (SANs a browser accepts; the cert the tray and native clients +// already pin). +const { cert: certPath, key: keyPath } = resolveUiTlsPaths( + process.env.PUNKTFUNK_UI_TLS_CERT, + process.env.PUNKTFUNK_UI_TLS_KEY, +); const tls = certPath && keyPath ? { cert: Bun.file(certPath), key: Bun.file(keyPath) } @@ -126,7 +136,8 @@ const listenerOptions = (lane) => ({ // is a hooks/library JSON edit, kilobytes. 4 MiB leaves several orders of headroom and still // makes the memory cost of an unauthenticated request negligible. maxRequestBodySize: - Number.parseInt(process.env.NITRO_BUN_MAX_BODY_BYTES, 10) || 4 * 1024 * 1024, + Number.parseInt(process.env.NITRO_BUN_MAX_BODY_BYTES, 10) || + 4 * 1024 * 1024, // `tls: undefined` ⇒ plain HTTP (dev); otherwise HTTPS over HTTP/1.1. tls, websocket: import.meta._websocket ? ws.websocket : undefined, @@ -167,7 +178,9 @@ console.log(`punktfunk web console listening on ${server.url} (tls=${!!tls})`); // this exists to close, and a security boundary that disappears when a port is busy is not one. It // degrades to "plugin UIs unavailable": the console reads the state below and renders an // explanation instead of a frame, and everything else about the console keeps working. -const pluginPort = Number(process.env.PUNKTFUNK_UI_PLUGIN_PORT || consolePort + 1); +const pluginPort = Number( + process.env.PUNKTFUNK_UI_PLUGIN_PORT || consolePort + 1, +); let pluginServer; try { pluginServer = Bun.serve({ ...listenerOptions("plugin"), port: pluginPort }); diff --git a/web/nitro-entry/tls-paths.mjs b/web/nitro-entry/tls-paths.mjs new file mode 100644 index 000000000..8826a4e24 --- /dev/null +++ b/web/nitro-entry/tls-paths.mjs @@ -0,0 +1,55 @@ +// Which of the host's two identities the console serves — resolved HERE because this entry is the +// one place every launcher routes through. +// +// The host keeps two identities side by side (crate::identity, the "identity split"): +// +// native-cert.pem / native-key.pem ECDSA P-256, with real SANs (the machine hostname, +// localhost, 127.0.0.1, ::1). This is what the native QUIC +// plane and the management API present, and what native +// clients pin. +// cert.pem / key.pem the legacy RSA GameStream identity: CN=punktfunk and NO SAN +// at all (gamestream::cert::generate passes rcgen an empty SAN +// list), kept byte-stable because Moonlight pins it and the +// pairing hashes bind its X.509 signature bytes. +// +// Every launcher names the LEGACY pair — scripts/punktfunk-web.service, the NixOS module, the +// Windows service supervisor, web-run.cmd, the Steam Deck installer — because they were written +// before the split, and none of them CAN choose: systemd `Environment=` has no "this file, else +// that one". Serving the legacy pair costs twice: +// +// * a CN-only, SAN-less cert is rejected outright by every current browser +// (ERR_CERT_COMMON_NAME_INVALID / SSL_ERROR_BAD_CERT_DOMAIN), so the console the operator was +// told to open does not load; +// * the tray's loopback liveness probe pins whatever the mgmt API serves — the NATIVE cert — so +// the handshake is refused and a perfectly healthy console is labelled "Open web console (not +// responding)" while the host beside it reads "idle" (field report 2026-08-24). +// +// So prefer the native sibling. It is also the smaller secret to hand a bundled bun: on a default +// build key.pem is the Moonlight PAIRING SIGNING key, native-key.pem is only a TLS key. +// +// Swapped as a PAIR or not at all — a native cert with the legacy key is a server that cannot +// complete a handshake with anyone. A host that never took the split (upgraded, native clients +// still pinning the RSA cert, so `load_or_adopt` keeps serving it) has no native pair on disk and +// falls through unchanged, as does a cert an operator supplied under any other name. +import { existsSync } from "node:fs"; +import { basename, dirname, join } from "node:path"; + +/** + * @param {string | undefined} cert PUNKTFUNK_UI_TLS_CERT, verbatim. + * @param {string | undefined} key PUNKTFUNK_UI_TLS_KEY, verbatim. + * @param {(p: string) => boolean} [exists] injected by the test; defaults to a real stat. + * @returns {{cert: string | undefined, key: string | undefined}} + */ +export function resolveUiTlsPaths(cert, key, exists = existsSync) { + // Half-configured TLS is the caller's error to report (it refuses to start); don't mask it by + // resolving one half of a pair that isn't there. + if (!cert || !key) return { cert, key }; + if (basename(cert) !== "cert.pem" || basename(key) !== "key.pem") { + return { cert, key }; + } + const nativeCert = join(dirname(cert), "native-cert.pem"); + const nativeKey = join(dirname(key), "native-key.pem"); + return exists(nativeCert) && exists(nativeKey) + ? { cert: nativeCert, key: nativeKey } + : { cert, key }; +} diff --git a/web/nitro-entry/tls-paths.test.ts b/web/nitro-entry/tls-paths.test.ts new file mode 100644 index 000000000..637e334ce --- /dev/null +++ b/web/nitro-entry/tls-paths.test.ts @@ -0,0 +1,58 @@ +// The pair swap is all-or-nothing, and the fallbacks are what keep legacy and custom-cert hosts +// serving. A native cert with the legacy key would be a console nobody can handshake with, so the +// mixed cases are the ones worth pinning down. +import { describe, expect, it } from "bun:test"; +import { resolveUiTlsPaths } from "./tls-paths.mjs"; + +const DIR = "/home/you/.config/punktfunk"; +const legacy = [`${DIR}/cert.pem`, `${DIR}/key.pem`] as const; +const native = [`${DIR}/native-cert.pem`, `${DIR}/native-key.pem`] as const; +/** `exists` over a fixed set of files on disk. */ +const on = + (...files: string[]) => + (p: string) => + files.includes(p); + +describe("resolveUiTlsPaths", () => { + it("prefers the native pair when both files are there", () => { + expect(resolveUiTlsPaths(...legacy, on(...legacy, ...native))).toEqual({ + cert: native[0], + key: native[1], + }); + }); + + it("keeps the legacy pair on a host that never took the identity split", () => { + expect(resolveUiTlsPaths(...legacy, on(...legacy))).toEqual({ + cert: legacy[0], + key: legacy[1], + }); + }); + + it("never mixes halves when only one native file exists", () => { + for (const half of native) { + expect(resolveUiTlsPaths(...legacy, on(...legacy, half))).toEqual({ + cert: legacy[0], + key: legacy[1], + }); + } + }); + + it("leaves an operator's own cert alone, native pair present or not", () => { + const own = [`${DIR}/lan-ca.pem`, `${DIR}/lan-ca.key`] as const; + expect(resolveUiTlsPaths(...own, on(...own, ...native))).toEqual({ + cert: own[0], + key: own[1], + }); + }); + + it("passes a half-configured pair through for the entry to refuse", () => { + expect(resolveUiTlsPaths(legacy[0], undefined, on(...native))).toEqual({ + cert: legacy[0], + key: undefined, + }); + expect(resolveUiTlsPaths(undefined, undefined, on(...native))).toEqual({ + cert: undefined, + key: undefined, + }); + }); +}); diff --git a/web/package.json b/web/package.json index bf768ed36..c0500be50 100644 --- a/web/package.json +++ b/web/package.json @@ -15,7 +15,7 @@ "start": "bun run .output/server/index.mjs", "api:gen": "orval --config orval.config.ts", "lint": "tsc --noEmit", - "test": "bun test server/", + "test": "bun test server/ nitro-entry/", "storybook": "storybook dev -p 6006", "build-storybook": "storybook build", "screenshots": "node tools/screenshots.mjs", diff --git a/web/vite.config.ts b/web/vite.config.ts index c67c82f29..1d6b12f2f 100644 --- a/web/vite.config.ts +++ b/web/vite.config.ts @@ -193,7 +193,7 @@ export default defineConfig({ // stock self-listening entry for ours (`nitro-entry/bun-https.mjs`), which calls // `Bun.serve({ tls })` so the console is served over HTTPS (HTTP/1.1 over TLS) with the // host's own identity cert. (No HTTP/2 — Bun.serve has no h2 server — and no HTTP/3, which a - // browser won't speak against this self-signed, no-SAN host cert.) Bun is the runtime + // browser won't speak against a self-signed host cert.) Bun is the runtime // everywhere now — the Windows installer already bundles it, and the punktfunk-web .deb // vendors it (it can't be `node`: `Bun.serve` is a bun API). (dev `vite dev` is unaffected.) preset: "bun", -- 2.54.0 From 1e2b956de66bbe5912080c639a25ccae1a22fd46 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Mon, 24 Aug 2026 22:45:47 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(web,tray,host):=20review=20follow-ups?= =?UTF-8?q?=20=E2=80=94=20pair=20the=20halves,=20and=20stop=20two=20commen?= =?UTF-8?q?ts=20overclaiming?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the parent commit. One real defect, the rest accuracy. The resolver could hand back a MISMATCHED pair, which is the one invariant its own header promised it never would: `nativeCert` came from `dirname(cert)` and `nativeKey` from `dirname(key)`, two independent directories, so ("/a/cert.pem", "/b/key.pem") resolved to /a/native-cert.pem + /b/native-key.pem — two unrelated files presented as a pair. No shipped launcher splits them, but the guard is one comparison and it is the whole point of the module. Rewritten off a suffix test instead of `node:path`, which fixes two more things in passing. `node:path` resolves per-RUNTIME, so a POSIX CI runner reads `C:\ProgramData\punktfunk\cert.pem` as one long filename and never swaps — and Windows, where windows/service.rs hands us exactly that, is the platform the CI job can never exercise. The suffix test gives the same answer everywhere and is now covered by a win32 case. It also leaves the prefix VERBATIM, where `join(dirname(p), …)` normalised /a/b/../cert.pem into a different directory the moment `b` was a symlink. Existence is no longer enough: `pf_paths::write_secret_file` is create+truncate+write rather than temp+rename, so a console starting mid-write could adopt a 0-byte cert and leave `Bun.serve` throwing on every restart. Not every launcher retries forever — the Steam Deck unit is `Restart=on-failure` under the default rate limit, i.e. permanently dead. The check is now a non-empty stat, mirroring the host's own `!c.trim().is_empty()`. Verified: with native-cert.pem truncated to 0 bytes the console starts and serves the legacy pair. Two comments of mine overclaimed and are corrected rather than left to mislead: * serve() said "cert.pem existing implies the native pair does too". False on an upgraded host whose native clients pinned the legacy cert — load_or_adopt returns it and writes no native files at all. The ordering claim that IS true is narrower: whenever that call writes a native pair, it does so before cert.pem appears. * the tray said the console entry "always opens the same URL regardless of the probe". True of the menu entry, but win.rs gates the tray-icon single-click on console_up. Also notes that the Windows probe was never pinned to begin with (punktfunk_config_dir is None off Linux), so that half is a no-op. Rest is doc drift the parent commit annotated in two launchers but not the other four: web.env.example, README, web-run.cmd, and the ci.yml comment that still said the web test step was "Scoped to server/". 18/18 web tests (was 14), biome and rustfmt clean, and the runtime check re-run against a fresh build: both pairs -> P-256 with SANs; 0-byte native cert -> legacy RSA, console still serving. --- .gitea/workflows/ci.yml | 8 +-- crates/punktfunk-host/src/gamestream/mod.rs | 17 +++--- crates/punktfunk-tray/src/status.rs | 8 +-- web/README.md | 2 + web/nitro-entry/tls-paths.mjs | 59 +++++++++++++++++---- web/nitro-entry/tls-paths.test.ts | 59 ++++++++++++++++++--- web/web-run.cmd | 3 ++ web/web.env.example | 4 +- 8 files changed, 128 insertions(+), 32 deletions(-) diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 3ee64f2f1..027267ba2 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -370,9 +370,11 @@ jobs: run: bun run build - name: Typecheck run: bun run lint - # Scoped to server/: the console's browser code has no test runner, but the gate that keeps a - # plugin's origin apart from the console's does — and its failure mode is a well-formed header - # that only a browser rejects, which nothing else here would catch. + # Scoped to server/ and nitro-entry/: the console's browser code has no test runner, but two + # gates here do — the one keeping a plugin's origin apart from the console's, whose failure + # mode is a well-formed header that only a browser rejects, and the one picking which of the + # host's two identities the console serves, whose failure mode is a cert no browser accepts. + # Neither would be caught anywhere else. - name: Test run: bun run test diff --git a/crates/punktfunk-host/src/gamestream/mod.rs b/crates/punktfunk-host/src/gamestream/mod.rs index e5506f440..12bde3eea 100644 --- a/crates/punktfunk-host/src/gamestream/mod.rs +++ b/crates/punktfunk-host/src/gamestream/mod.rs @@ -431,13 +431,16 @@ pub fn serve( // // Resolved BEFORE the legacy GameStream identity below, and that order is load-bearing twice // over. (1) The web console gates its start on `cert.pem` existing and then serves the native - // pair sitting next to it (web/nitro-entry/tls-paths.mjs); minting the legacy pair first leaves - // a first-run window where the console starts, finds no native pair, and serves the SAN-less - // RSA cert no browser accepts — for the rest of that boot. This way `cert.pem` existing implies - // the native pair does too. (2) In the degenerate case (native clients paired, but the cert - // they pinned is gone from disk) the old order let `load_or_create` mint a BRAND-NEW cert.pem - // that `load_or_adopt` then adopted while logging that it was preserving their pins — stranding - // them silently. Reading the dir first means that case reaches the branch written for it. + // pair sitting next to it (web/nitro-entry/tls-paths.mjs); minting the legacy pair first + // leaves a first-run window where the console starts, finds no native pair, and serves the + // SAN-less RSA cert no browser accepts — for the rest of that boot. Running first closes that + // window: whenever this call WRITES a native pair, it has done so before `cert.pem` appears. + // (It does not write one on an upgraded host whose native clients pinned the legacy cert — + // there the console correctly falls back to that same legacy pair.) (2) In the degenerate case + // (native clients paired, but the cert they pinned is gone from disk) the old order let + // `load_or_create` mint a BRAND-NEW cert.pem that `load_or_adopt` then adopted while logging + // that it was preserving their pins — stranding them silently. Reading the dir first means + // that case reaches the branch written for it. let native_ident = crate::identity::load_or_adopt(&np).context("native host identity")?; #[cfg(feature = "gamestream")] let state = { diff --git a/crates/punktfunk-tray/src/status.rs b/crates/punktfunk-tray/src/status.rs index 3fbc7f5b8..8b953bcf3 100644 --- a/crates/punktfunk-tray/src/status.rs +++ b/crates/punktfunk-tray/src/status.rs @@ -220,9 +220,11 @@ fn poll_loop( // cert would have hit it just as squarely, so the coupling goes rather than the symptom. // // Nothing is lost by dropping the pin: this probe sends no credentials, reads no body, and - // decides only a menu LABEL. A local port-squatter could make that label read "up" — but the - // entry is always present and always opens the same URL regardless of the probe, so it gains - // nothing it did not already have. + // decides only presentation — the menu entry's label, plus whether a tray-icon click opens + // the console or the menu (win.rs). A port-squatter could flip that, but the entry itself is + // unconditional and opens the same URL either way, and no browser ever pinned this cert. On + // Windows the probe was never pinned to begin with: `punktfunk_config_dir` returns None there, + // so `load_pin` was already None. let console_agent = agent(None); let mut last: Option<(TrayStatus, bool)> = None; // When the summary became unreachable while the service was running (grace anchor). diff --git a/web/README.md b/web/README.md index e45c44cb1..6f44ddd04 100644 --- a/web/README.md +++ b/web/README.md @@ -58,6 +58,8 @@ PORT=47992 HOST=0.0.0.0 \ PUNKTFUNK_UI_TLS_KEY=~/.config/punktfunk/key.pem PUNKTFUNK_UI_SECURE=1 \ bun run start # = bun run .output/server/index.mjs # PUNKTFUNK_UI_TLS_* unset ⇒ plain HTTP (local dev); both set ⇒ HTTPS (HTTP/1.1 over TLS). +# Naming cert.pem/key.pem serves native-cert.pem/native-key.pem instead when both sit beside them +# (the identity split — nitro-entry/tls-paths.mjs); the legacy pair is the fallback, not the target. # The host's self-signed mgmt cert is accepted only for the proxy's loopback hop, scoped in code # (Bun per-request TLS: server/routes/api/[...].ts) — no process-wide NODE_TLS_REJECT_UNAUTHORIZED. # See .env.example. diff --git a/web/nitro-entry/tls-paths.mjs b/web/nitro-entry/tls-paths.mjs index 8826a4e24..c786450f2 100644 --- a/web/nitro-entry/tls-paths.mjs +++ b/web/nitro-entry/tls-paths.mjs @@ -28,11 +28,48 @@ // build key.pem is the Moonlight PAIRING SIGNING key, native-key.pem is only a TLS key. // // Swapped as a PAIR or not at all — a native cert with the legacy key is a server that cannot -// complete a handshake with anyone. A host that never took the split (upgraded, native clients -// still pinning the RSA cert, so `load_or_adopt` keeps serving it) has no native pair on disk and -// falls through unchanged, as does a cert an operator supplied under any other name. -import { existsSync } from "node:fs"; -import { basename, dirname, join } from "node:path"; +// complete a handshake with anyone, so both halves must be present AND must come from the same +// directory. A host that never took the split (upgraded, native clients still pinning the RSA cert, +// so `load_or_adopt` keeps serving it) has no native pair on disk and falls through unchanged, as +// does a cert an operator supplied under any other name. +import { statSync } from "node:fs"; + +/** + * The directory prefix (separator included) of a path ending in `base`, or null if it does not. + * + * Deliberately NOT `node:path`: that resolves per-RUNTIME, so a POSIX build reads + * `C:\ProgramData\punktfunk\cert.pem` as one long filename — and Windows, where the service + * supervisor hands us exactly that (windows/service.rs), is the platform CI can never exercise. + * A suffix test gives the same answer everywhere. It also leaves the prefix VERBATIM, where + * `join(dirname(p), …)` would normalise `/a/b/../cert.pem` to a different directory than the one + * the operator named — which matters the moment `b` is a symlink. + * + * @param {string} p + * @param {string} base + * @returns {string | null} + */ +function dirPrefix(p, base) { + if (p === base) return ""; // bare relative name + if (!p.endsWith(base)) return null; + const sep = p[p.length - base.length - 1]; + return sep === "/" || sep === "\\" ? p.slice(0, -base.length) : null; +} + +/** + * A readable, NON-EMPTY file. Emptiness matters: `pf_paths::write_secret_file` is + * create+truncate+write rather than temp+rename, so a console starting mid-write could otherwise + * adopt a 0-byte cert and leave `Bun.serve` throwing on every restart — and not every launcher + * retries forever (the Steam Deck unit is `Restart=on-failure` under the default rate limit). + * + * @param {string} p + */ +function usable(p) { + try { + return statSync(p).size > 0; + } catch { + return false; + } +} /** * @param {string | undefined} cert PUNKTFUNK_UI_TLS_CERT, verbatim. @@ -40,15 +77,15 @@ import { basename, dirname, join } from "node:path"; * @param {(p: string) => boolean} [exists] injected by the test; defaults to a real stat. * @returns {{cert: string | undefined, key: string | undefined}} */ -export function resolveUiTlsPaths(cert, key, exists = existsSync) { +export function resolveUiTlsPaths(cert, key, exists = usable) { // Half-configured TLS is the caller's error to report (it refuses to start); don't mask it by // resolving one half of a pair that isn't there. if (!cert || !key) return { cert, key }; - if (basename(cert) !== "cert.pem" || basename(key) !== "key.pem") { - return { cert, key }; - } - const nativeCert = join(dirname(cert), "native-cert.pem"); - const nativeKey = join(dirname(key), "native-key.pem"); + const dir = dirPrefix(cert, "cert.pem"); + // Same directory, or we are not looking at a pair — see the PAIR note above. + if (dir === null || dir !== dirPrefix(key, "key.pem")) return { cert, key }; + const nativeCert = `${dir}native-cert.pem`; + const nativeKey = `${dir}native-key.pem`; return exists(nativeCert) && exists(nativeKey) ? { cert: nativeCert, key: nativeKey } : { cert, key }; diff --git a/web/nitro-entry/tls-paths.test.ts b/web/nitro-entry/tls-paths.test.ts index 637e334ce..34e606daa 100644 --- a/web/nitro-entry/tls-paths.test.ts +++ b/web/nitro-entry/tls-paths.test.ts @@ -1,13 +1,14 @@ // The pair swap is all-or-nothing, and the fallbacks are what keep legacy and custom-cert hosts // serving. A native cert with the legacy key would be a console nobody can handshake with, so the -// mixed cases are the ones worth pinning down. +// mixed cases are the ones worth pinning down — including the Windows shape, which the resolver +// must get right without a win32 runtime to ask (see dirPrefix in tls-paths.mjs). import { describe, expect, it } from "bun:test"; import { resolveUiTlsPaths } from "./tls-paths.mjs"; const DIR = "/home/you/.config/punktfunk"; const legacy = [`${DIR}/cert.pem`, `${DIR}/key.pem`] as const; const native = [`${DIR}/native-cert.pem`, `${DIR}/native-key.pem`] as const; -/** `exists` over a fixed set of files on disk. */ +/** `exists` over a fixed set of usable files on disk. */ const on = (...files: string[]) => (p: string) => @@ -28,7 +29,7 @@ describe("resolveUiTlsPaths", () => { }); }); - it("never mixes halves when only one native file exists", () => { + it("never mixes halves when only one native file is usable", () => { for (const half of native) { expect(resolveUiTlsPaths(...legacy, on(...legacy, half))).toEqual({ cert: legacy[0], @@ -37,11 +38,55 @@ describe("resolveUiTlsPaths", () => { } }); + // The Windows service supervisor hands us backslash paths (windows/service.rs); node:path on a + // POSIX CI runner would read the whole thing as one filename and silently never swap. + it("resolves Windows paths without a win32 runtime", () => { + const win = ["C:\\ProgramData\\punktfunk", "D:\\pf"] as const; + for (const d of win) { + expect( + resolveUiTlsPaths(`${d}\\cert.pem`, `${d}\\key.pem`, () => true), + ).toEqual({ + cert: `${d}\\native-cert.pem`, + key: `${d}\\native-key.pem`, + }); + } + }); + + it("refuses to pair halves from two different directories", () => { + expect(resolveUiTlsPaths("/a/cert.pem", "/b/key.pem", () => true)).toEqual({ + cert: "/a/cert.pem", + key: "/b/key.pem", + }); + }); + + it("leaves the prefix verbatim rather than normalising it away", () => { + // `join(dirname(p), …)` would collapse this to /a/native-cert.pem — a different directory + // the moment `b` is a symlink. + expect( + resolveUiTlsPaths("/a/b/../cert.pem", "/a/b/../key.pem", () => true), + ).toEqual({ + cert: "/a/b/../native-cert.pem", + key: "/a/b/../native-key.pem", + }); + }); + it("leaves an operator's own cert alone, native pair present or not", () => { - const own = [`${DIR}/lan-ca.pem`, `${DIR}/lan-ca.key`] as const; - expect(resolveUiTlsPaths(...own, on(...own, ...native))).toEqual({ - cert: own[0], - key: own[1], + // Also covers the endsWith trap: "mycert.pem" ends with "cert.pem" but is not one. + for (const own of [ + [`${DIR}/lan-ca.pem`, `${DIR}/lan-ca.key`], + [`${DIR}/mycert.pem`, `${DIR}/mykey.pem`], + ] as const) { + expect(resolveUiTlsPaths(...own, on(...own, ...native))).toEqual({ + cert: own[0], + key: own[1], + }); + } + }); + + it("does not re-swap a pair that already names the native files", () => { + expect(resolveUiTlsPaths(...native, () => true)).toEqual({ + cert: native[0], + key: native[1], }); }); diff --git a/web/web-run.cmd b/web/web-run.cmd index d6fb28fcf..7789ae120 100644 --- a/web/web-run.cmd +++ b/web/web-run.cmd @@ -51,6 +51,9 @@ if exist "%ENDPOINTFILE%" for /f "usebackq tokens=1* delims==" %%A in ("%ENDPOIN rem No NODE_TLS_REJECT_UNAUTHORIZED: the host's self-signed cert is accepted only for the loopback rem proxy hop, scoped inside the proxy code (Bun per-request TLS), not process-wide. rem Serve HTTPS (HTTP/1.1 over TLS) with the host's identity cert; mark the session cookie Secure. +rem These name the LEGACY pair; the server prefers native-cert.pem/native-key.pem beside them when +rem both exist (the identity split - web\nitro-entry\tls-paths.mjs). Don't "fix" them to the native +rem names: a host that never took the split has no native pair, and the fallback lives in there. set "PUNKTFUNK_UI_TLS_CERT=%CERTFILE%" set "PUNKTFUNK_UI_TLS_KEY=%KEYFILE%" set "PUNKTFUNK_UI_SECURE=1" diff --git a/web/web.env.example b/web/web.env.example index d81b7f4b2..1f0a39f4e 100644 --- a/web/web.env.example +++ b/web/web.env.example @@ -18,7 +18,9 @@ HOST=0.0.0.0 # Serve the console over HTTPS (HTTP/1.1 over TLS) with the host's own identity cert. BOTH paths # set ⇒ HTTPS. (No HTTP/2 or HTTP/3: Bun.serve has no HTTP/2 server, and a browser won't speak -# HTTP/3/QUIC against this self-signed, no-SAN host cert — so HTTP/1.1 over TLS is what's offered.) +# HTTP/3/QUIC against a self-signed host cert — so HTTP/1.1 over TLS is what's offered.) +# Name the LEGACY pair below: the server prefers native-cert.pem/native-key.pem beside it when both +# exist (nitro-entry/tls-paths.mjs), and falls back to these on a host that never took the split. PUNKTFUNK_UI_TLS_CERT=%h/.config/punktfunk/cert.pem PUNKTFUNK_UI_TLS_KEY=%h/.config/punktfunk/key.pem # Mark the session cookie Secure (required once served over TLS): -- 2.54.0