From ed5df261402c87e3c67a139540401ed730713ff2 Mon Sep 17 00:00:00 2001 From: Draco Lunaris <331325+Draco-Lunaris@users.noreply.github.com> Date: Tue, 2 Jun 2026 10:45:38 -0500 Subject: [PATCH] fix(ws): add Origin allowlist to browser WebSocket upgrade (CSWSH hardening) Closes Draco-Lunaris/Linux-Patch-Manager#10 The browser WebSocket endpoint at GET /api/v1/ws/jobs previously authenticated solely via a single-use, 60-second ticket passed as a query parameter. A leaked ticket (browser history, Referer, proxy logs, support bundles) could be redeemed from any origin, enabling Cross-Site WebSocket Hijacking (CSWSH). This change adds a second gate: the Origin header must match an explicit allowlist. The check runs BEFORE ticket validation so that rejected cross-origin probes do not consume the legitimate users ticket. Changes: - pm-core: new security.allowed_origins config field; default derived from sso_callback_url; startup warning if both are unparseable - pm-web: ws_handler extracts HeaderMap and calls check_origin first; returns 403 on missing/malformed/disallowed origins - config: documented allowed_origins key in config.example.toml - docs: security-review.md section 1.4 (WebSocket Origin Allowlist) - tests: 40 unit tests (7 pm-core, 33 pm-web) --- Cargo.lock | 14 +- config/config.example.toml | 15 ++ crates/pm-core/src/config.rs | 148 ++++++++++- crates/pm-web/src/routes/ws.rs | 457 ++++++++++++++++++++++++++++++++- docs/security-review.md | 11 + tasks/lessons.md | 11 + tasks/todo.md | 62 ++++- tasks/ws-origin-check-spec.md | 219 ++++++++++++++++ 8 files changed, 925 insertions(+), 12 deletions(-) mode change 100755 => 100644 crates/pm-web/src/routes/ws.rs create mode 100644 tasks/ws-origin-check-spec.md diff --git a/Cargo.lock b/Cargo.lock index b42fabc..5514140 100755 --- a/Cargo.lock +++ b/Cargo.lock @@ -2521,7 +2521,7 @@ dependencies = [ [[package]] name = "pm-agent-client" -version = "0.1.8" +version = "0.1.9" dependencies = [ "anyhow", "chrono", @@ -2538,7 +2538,7 @@ dependencies = [ [[package]] name = "pm-auth" -version = "0.1.8" +version = "0.1.9" dependencies = [ "anyhow", "argon2", @@ -2565,7 +2565,7 @@ dependencies = [ [[package]] name = "pm-ca" -version = "0.1.8" +version = "0.1.9" dependencies = [ "anyhow", "chrono", @@ -2588,7 +2588,7 @@ dependencies = [ [[package]] name = "pm-core" -version = "0.1.8" +version = "0.1.9" dependencies = [ "aes-gcm", "anyhow", @@ -2612,7 +2612,7 @@ dependencies = [ [[package]] name = "pm-reports" -version = "0.1.8" +version = "0.1.9" dependencies = [ "anyhow", "chrono", @@ -2632,7 +2632,7 @@ dependencies = [ [[package]] name = "pm-web" -version = "0.1.8" +version = "0.1.9" dependencies = [ "anyhow", "axum", @@ -2672,7 +2672,7 @@ dependencies = [ [[package]] name = "pm-worker" -version = "0.1.8" +version = "0.1.9" dependencies = [ "anyhow", "chrono", diff --git a/config/config.example.toml b/config/config.example.toml index 4d22c43..5a0801f 100644 --- a/config/config.example.toml +++ b/config/config.example.toml @@ -108,6 +108,21 @@ web_tls_key_path = "/etc/patch-manager/tls/web.key" # Default: "http://localhost:5173/auth/sso/callback" (Vite dev server) sso_callback_url = "http://localhost:5173/auth/sso/callback" +# Allowlist of browser `Origin` values permitted to open the +# `/api/v1/ws/jobs` WebSocket upgrade. Each entry is an exact +# `scheme://host[:port]` string (no wildcards, no paths). When this list is +# empty, the server derives a single-entry default from `sso_callback_url` +# at startup (the host of the SSO callback). If the derivation also fails, +# a warning is logged and the WS endpoint rejects all browser upgrades +# (fail-closed). +# +# Add additional origins here if your SPA and API are served from different +# hosts (e.g. SPA on https://app.example.com talking to API on +# https://api.example.com). For typical single-host deployments the derived +# default is correct and this line should be left commented out. +# +# allowed_origins = ["https://patch-manager.example.com"] + # ============================================================ # Rate Limiting # ============================================================ diff --git a/crates/pm-core/src/config.rs b/crates/pm-core/src/config.rs index 628cafa..153182b 100644 --- a/crates/pm-core/src/config.rs +++ b/crates/pm-core/src/config.rs @@ -140,6 +140,73 @@ pub struct SecurityConfig { /// Frontend URL to redirect to after SSO callback (default: http://localhost:5173/auth/sso/callback) #[serde(default = "default_sso_callback_url")] pub sso_callback_url: String, + /// Allowlist of browser `Origin` values permitted to open the + /// `/api/v1/ws/jobs` WebSocket upgrade. Entries are exact + /// `scheme://host[:port]` strings. If left empty in the TOML file, the + /// server derives the default from `sso_callback_url` at load time + /// (see [`derive_allowed_origins`]). + #[serde(default)] + pub allowed_origins: Vec, +} + +/// Derive a default `Origin` allowlist from a single SSO callback URL. +/// +/// Parses `scheme://host[:port][/path]` and returns a single-element vector +/// containing `scheme://host[:port]` (with default ports normalized away — +/// e.g. `https://x:443` becomes `https://x`). Returns an empty vector if the +/// URL is unparseable; callers should log a warning in that case because the +/// WebSocket endpoint will reject all browser upgrades (fail-closed). +/// +/// Exposed publicly so tests and the handler can share the same parser. +pub fn derive_allowed_origins(sso_callback_url: &str) -> Vec { + let s = sso_callback_url.trim().trim_end_matches('/'); + let (scheme, rest) = match s.split_once("://") { + Some(parts) if !parts.0.is_empty() => parts, + _ => return vec![], + }; + let scheme_lower = scheme.to_ascii_lowercase(); + if scheme_lower != "http" && scheme_lower != "https" { + return vec![]; + } + // Authority is everything up to the first `/`, `?`, or `#`. + let authority_end = rest + .find(['/', '?', '#']) + .unwrap_or(rest.len()); + let authority = &rest[..authority_end]; + if authority.is_empty() { + return vec![]; + } + // Split host:port. We treat the LAST `:` as the port separator. IPv6 + // literal hosts (e.g. `[::1]`) contain a `:` inside the brackets; we + // explicitly do not support IPv6 in sso_callback_url and return empty + // for those to be safe. + let (host, port_str) = match authority.rsplit_once(':') { + Some((h, _)) if h.contains(':') => return vec![], + Some((h, p)) => (h, Some(p)), + None => (authority, None), + }; + let host = host.trim(); + if host.is_empty() || host.contains(char::is_whitespace) || host.contains(':') { + return vec![]; + } + let default_port: Option = match scheme_lower.as_str() { + "https" => Some(443), + "http" => Some(80), + _ => None, + }; + let port_num = match port_str { + Some(p) => match p.parse::() { + Ok(n) => Some(n), + Err(_) => return vec![], + }, + None => None, + }; + let origin = match (port_num, default_port) { + (Some(p), Some(d)) if p == d => format!("{}://{}", scheme_lower, host), + (Some(p), _) => format!("{}://{}:{}", scheme_lower, host, p), + (None, _) => format!("{}://{}", scheme_lower, host), + }; + vec![origin] } impl AppConfig { @@ -147,6 +214,11 @@ impl AppConfig { /// /// Environment variables follow the pattern: `PATCH_MANAGER__SECTION__KEY` /// e.g. `PATCH_MANAGER__DATABASE__URL=postgres://...` + /// + /// After deserialization, if `security.allowed_origins` is empty, it is + /// derived from `security.sso_callback_url`. A `tracing::warn!` is emitted + /// when the resulting allowlist is empty (the WS endpoint will reject all + /// browser upgrades in that case). pub fn load(config_path: &str) -> Result { let cfg = Config::builder() .add_source(File::with_name(config_path).required(false)) @@ -157,7 +229,20 @@ impl AppConfig { ) .build()?; - cfg.try_deserialize() + let mut config: Self = cfg.try_deserialize()?; + if config.security.allowed_origins.is_empty() { + config.security.allowed_origins = + derive_allowed_origins(&config.security.sso_callback_url); + } + if config.security.allowed_origins.is_empty() { + tracing::warn!( + sso_callback_url = %config.security.sso_callback_url, + "security.allowed_origins is empty and could not be derived \ + from sso_callback_url; the WebSocket endpoint will reject all \ + browser upgrades" + ); + } + Ok(config) } } @@ -207,8 +292,69 @@ impl Default for AppConfig { web_tls_cert_path: "/etc/patch-manager/tls/web.crt".to_string(), web_tls_key_path: "/etc/patch-manager/tls/web.key".to_string(), sso_callback_url: default_sso_callback_url(), + allowed_origins: derive_allowed_origins(&default_sso_callback_url()), }, rate_limit: RateLimitConfig::default(), } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn derive_strips_default_https_port() { + assert_eq!( + derive_allowed_origins("https://app.example.com:443/auth/sso/callback"), + vec!["https://app.example.com".to_string()] + ); + } + + #[test] + fn derive_keeps_non_default_port() { + assert_eq!( + derive_allowed_origins("https://app.example.com:8443/auth/sso/callback"), + vec!["https://app.example.com:8443".to_string()] + ); + } + + #[test] + fn derive_strips_default_http_port() { + assert_eq!( + derive_allowed_origins("http://localhost:80/x"), + vec!["http://localhost".to_string()] + ); + } + + #[test] + fn derive_handles_trailing_slash() { + assert_eq!( + derive_allowed_origins("https://app.example.com/"), + vec!["https://app.example.com".to_string()] + ); + } + + #[test] + fn derive_handles_no_path() { + assert_eq!( + derive_allowed_origins("https://app.example.com"), + vec!["https://app.example.com".to_string()] + ); + } + + #[test] + fn derive_returns_empty_for_garbage() { + assert!(derive_allowed_origins("not a url").is_empty()); + assert!(derive_allowed_origins("").is_empty()); + assert!(derive_allowed_origins("ftp://x").is_empty()); + } + + #[test] + fn derive_lowercases_scheme() { + assert_eq!( + derive_allowed_origins("HTTPS://App.Example.com"), + vec!["https://App.Example.com".to_string()] + ); + } +} diff --git a/crates/pm-web/src/routes/ws.rs b/crates/pm-web/src/routes/ws.rs old mode 100755 new mode 100644 index bf9dbe7..720ed04 --- a/crates/pm-web/src/routes/ws.rs +++ b/crates/pm-web/src/routes/ws.rs @@ -6,7 +6,7 @@ use axum::{ extract::ws::{Message, WebSocket}, extract::{Query, State, WebSocketUpgrade}, - http::StatusCode, + http::{HeaderMap, StatusCode}, response::{Json, Response}, routing::{get, post}, Router, @@ -57,6 +57,162 @@ fn err( ) } +// ── Origin parsing & allowlist matching ─────────────────────────────────────── + +/// Parsed browser `Origin` header value. +#[derive(Debug, Clone, PartialEq, Eq)] +struct Origin { + scheme: String, + host: String, + /// `None` means "use scheme default" (80 for http, 443 for https). + port: Option, +} + +impl Origin { + /// Render back to canonical `scheme://host[:port]` form with default + /// ports normalized away (so `https://x:443` becomes `https://x`). + fn canonical(&self) -> String { + let default_port: Option = match self.scheme.as_str() { + "https" => Some(443), + "http" => Some(80), + _ => None, + }; + match (self.port, default_port) { + (Some(p), Some(d)) if p == d => format!("{}://{}", self.scheme, self.host), + (Some(p), _) => format!("{}://{}:{}", self.scheme, self.host, p), + (None, _) => format!("{}://{}", self.scheme, self.host), + } + } +} + +/// Parse a raw `Origin` header value. Returns `None` for missing scheme, +/// unsupported schemes (only `http`/`https`), empty host, or whitespace in +/// the host. IPv6 literal hosts are explicitly rejected to keep the parser +/// simple — WebSocket connections from IPv6 browser origins are not a +/// realistic deployment for this product. +fn parse_origin_header(value: &str) -> Option { + let s = value.trim().trim_end_matches('/'); + if s.is_empty() { + return None; + } + let (scheme, rest) = s.split_once("://")?; + let scheme = scheme.to_ascii_lowercase(); + if scheme != "http" && scheme != "https" { + return None; + } + // Authority is everything up to the first `/`, `?`, or `#`. + let authority_end = rest + .find(['/', '?', '#']) + .unwrap_or(rest.len()); + let authority = &rest[..authority_end]; + if authority.is_empty() { + return None; + } + // Treat the LAST `:` as the port separator. IPv6 literal hosts (e.g. + // `[::1]`) contain a `:` inside the brackets; reject those. + let (host, port_str) = match authority.rsplit_once(':') { + Some((h, _)) if h.contains(':') => return None, + Some((h, p)) => (h, Some(p)), + None => (authority, None), + }; + let host = host.trim(); + if host.is_empty() || host.contains(char::is_whitespace) || host.contains(':') { + return None; + } + let port = match port_str { + Some(p) => match p.parse::() { + Ok(n) => Some(n), + Err(_) => return None, + }, + None => None, + }; + Some(Origin { + scheme, + host: host.to_ascii_lowercase(), + port, + }) +} + +/// Match a parsed `Origin` against an allowlist. Each allowlist entry is +/// itself parsed with [`parse_origin_header`] and compared by its canonical +/// string form, so entry syntax is forgiving (`https://x:443` matches an +/// incoming `https://x`). The host comparison is case-insensitive (the +/// parser lowercases the host); scheme and port are exact. +/// +/// An empty allowlist returns `false` (fail-closed). +fn is_origin_allowed(origin: &Origin, allowlist: &[String]) -> bool { + if allowlist.is_empty() { + return false; + } + let incoming = origin.canonical(); + allowlist.iter().any(|entry| { + match parse_origin_header(entry) { + Some(parsed) => parsed.canonical() == incoming, + None => false, + } + }) +} + +/// Read the `Origin` header from a request and check it against the +/// configured allowlist. Returns `Ok(())` when the request may proceed; on +/// rejection returns the appropriate `(StatusCode, Json)` error tuple and +/// the reason string (for logging). +fn check_origin( + headers: &HeaderMap, + allowlist: &[String], +) -> Result<(), ((StatusCode, Json), &'static str)> { + let raw = match headers.get(axum::http::header::ORIGIN) { + Some(v) => v, + None => { + return Err(( + err( + StatusCode::FORBIDDEN, + "forbidden_origin", + "Origin header required", + ), + "missing", + )); + } + }; + let raw_str = match raw.to_str() { + Ok(s) => s, + Err(_) => { + return Err(( + err( + StatusCode::FORBIDDEN, + "forbidden_origin", + "Origin header not valid ASCII", + ), + "non-ascii", + )); + } + }; + let origin = match parse_origin_header(raw_str) { + Some(o) => o, + None => { + return Err(( + err( + StatusCode::FORBIDDEN, + "forbidden_origin", + "Malformed Origin header", + ), + "malformed", + )); + } + }; + if !is_origin_allowed(&origin, allowlist) { + return Err(( + err( + StatusCode::FORBIDDEN, + "forbidden_origin", + "Origin not allowed", + ), + "not-allowlisted", + )); + } + Ok(()) +} + // ── POST /api/v1/ws/ticket ──────────────────────────────────────────────────── /// Issue a single-use WebSocket authentication ticket (60 s expiry). @@ -93,11 +249,40 @@ pub struct WsQuery { } /// Browser WebSocket upgrade endpoint — authenticates via single-use ticket. +/// +/// The handler enforces two independent gates, in this order: +/// +/// 1. `Origin` header allowlist (CSWSH defense-in-depth). Performed first so +/// that a cross-origin probe with a leaked/stolen ticket does not consume +/// the legitimate user's ticket. +/// 2. Single-use, 60-second ticket (existing behavior, unchanged). pub async fn ws_handler( State(state): State, + headers: HeaderMap, Query(q): Query, ws: WebSocketUpgrade, ) -> Result)> { + // Gate 1: Origin allowlist (CSWSH defense-in-depth). + let allowlist = &state.config.security.allowed_origins; + if let Err((http_err, reason)) = check_origin(&headers, allowlist) { + let raw_origin = headers + .get(axum::http::header::ORIGIN) + .and_then(|v| v.to_str().ok()) + .unwrap_or(""); + // Never log the ticket value. + tracing::warn!( + reason = reason, + origin = %raw_origin, + "WebSocket upgrade rejected: forbidden origin" + ); + return Err(http_err); + } + let allowed_origin = headers + .get(axum::http::header::ORIGIN) + .and_then(|v| v.to_str().ok()) + .unwrap_or("") + .to_string(); + // Validate and consume the ticket atomically. let ticket = { let entry = state.ws_tickets.get(&q.ticket); @@ -129,6 +314,7 @@ pub async fn ws_handler( tracing::info!( user_id = %ticket.user_id, role = %ticket.role, + origin = %allowed_origin, "Browser WebSocket connection upgraded" ); @@ -203,3 +389,272 @@ async fn handle_browser_ws(mut socket: WebSocket, db: sqlx::PgPool, ticket: WsTi tracing::info!(user_id = %ticket.user_id, "Browser WS handler exiting"); } + +// ── Tests ──────────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + + // ── parse_origin_header ───────────────────────────────────────────────── + + #[test] + fn parse_basic_https() { + assert_eq!( + parse_origin_header("https://app.example.com"), + Some(Origin { + scheme: "https".into(), + host: "app.example.com".into(), + port: None, + }) + ); + } + + #[test] + fn parse_with_explicit_port() { + assert_eq!( + parse_origin_header("https://app.example.com:8443"), + Some(Origin { + scheme: "https".into(), + host: "app.example.com".into(), + port: Some(8443), + }) + ); + } + + #[test] + fn parse_lowercases_scheme() { + assert_eq!( + parse_origin_header("HTTPS://App.Example.com").unwrap().scheme, + "https" + ); + } + + #[test] + fn parse_lowercases_host() { + assert_eq!( + parse_origin_header("https://App.Example.com").unwrap().host, + "app.example.com" + ); + } + + #[test] + fn parse_ignores_path_query_fragment() { + let o = parse_origin_header("https://app.example.com:443/some/path?q=1#frag").unwrap(); + assert_eq!(o.host, "app.example.com"); + assert_eq!(o.port, Some(443)); + } + + #[test] + fn parse_strips_trailing_slash() { + assert_eq!( + parse_origin_header("https://app.example.com/"), + Some(Origin { + scheme: "https".into(), + host: "app.example.com".into(), + port: None, + }) + ); + } + + #[test] + fn parse_rejects_empty() { + assert!(parse_origin_header("").is_none()); + assert!(parse_origin_header(" ").is_none()); + } + + #[test] + fn parse_rejects_unsupported_scheme() { + assert!(parse_origin_header("ftp://x").is_none()); + assert!(parse_origin_header("file:///etc/passwd").is_none()); + assert!(parse_origin_header("javascript:alert(1)").is_none()); + } + + #[test] + fn parse_rejects_empty_host() { + assert!(parse_origin_header("https://").is_none()); + assert!(parse_origin_header("https:///path").is_none()); + } + + #[test] + fn parse_rejects_host_with_whitespace() { + assert!(parse_origin_header("https://bad host").is_none()); + } + + #[test] + fn parse_rejects_malformed_port() { + assert!(parse_origin_header("https://x:notaport").is_none()); + assert!(parse_origin_header("https://x:99999").is_none()); + } + + #[test] + fn parse_rejects_ipv6_literal() { + assert!(parse_origin_header("https://[::1]").is_none()); + } + + #[test] + fn parse_rejects_no_scheme_separator() { + assert!(parse_origin_header("app.example.com").is_none()); + } + + // ── canonical ────────────────────────────────────────────────────────── + + #[test] + fn canonical_strips_default_https_port() { + let o = Origin { + scheme: "https".into(), + host: "x".into(), + port: Some(443), + }; + assert_eq!(o.canonical(), "https://x"); + } + + #[test] + fn canonical_strips_default_http_port() { + let o = Origin { + scheme: "http".into(), + host: "x".into(), + port: Some(80), + }; + assert_eq!(o.canonical(), "http://x"); + } + + #[test] + fn canonical_keeps_non_default_port() { + let o = Origin { + scheme: "https".into(), + host: "x".into(), + port: Some(8443), + }; + assert_eq!(o.canonical(), "https://x:8443"); + } + + // ── is_origin_allowed ────────────────────────────────────────────────── + + #[test] + fn allowed_exact_match() { + let o = parse_origin_header("https://app.example.com").unwrap(); + assert!(is_origin_allowed(&o, &["https://app.example.com".into()])); + } + + #[test] + fn allowed_default_port_normalization_incoming() { + let o = parse_origin_header("https://app.example.com:443").unwrap(); + assert!(is_origin_allowed(&o, &["https://app.example.com".into()])); + } + + #[test] + fn allowed_default_port_normalization_allowlist() { + let o = parse_origin_header("https://app.example.com").unwrap(); + assert!(is_origin_allowed(&o, &["https://app.example.com:443".into()])); + } + + #[test] + fn allowed_case_insensitive_host() { + let o = parse_origin_header("https://App.Example.com").unwrap(); + assert!(is_origin_allowed(&o, &["https://app.example.com".into()])); + } + + #[test] + fn rejected_different_host() { + let o = parse_origin_header("https://evil.example").unwrap(); + assert!(!is_origin_allowed(&o, &["https://app.example.com".into()])); + } + + #[test] + fn rejected_different_scheme() { + let o = parse_origin_header("http://app.example.com").unwrap(); + assert!(!is_origin_allowed(&o, &["https://app.example.com".into()])); + } + + #[test] + fn rejected_different_port() { + let o = parse_origin_header("https://app.example.com:8443").unwrap(); + assert!(!is_origin_allowed(&o, &["https://app.example.com".into()])); + } + + #[test] + fn rejected_empty_allowlist() { + let o = parse_origin_header("https://app.example.com").unwrap(); + assert!(!is_origin_allowed(&o, &[])); + } + + #[test] + fn rejected_garbage_in_allowlist() { + let o = parse_origin_header("https://app.example.com").unwrap(); + assert!(!is_origin_allowed(&o, &["not a url".into()])); + } + + #[test] + fn allowed_multi_entry_allowlist() { + let o = parse_origin_header("https://app.example.com").unwrap(); + assert!(is_origin_allowed( + &o, + &[ + "https://other.example".into(), + "https://app.example.com".into(), + ] + )); + } + + // ── check_origin (integration of parse + allow) ──────────────────────── + + #[test] + fn check_rejects_missing_header() { + let h = HeaderMap::new(); + let err = check_origin(&h, &["https://app.example.com".into()]).unwrap_err(); + assert_eq!(err.0 .0, StatusCode::FORBIDDEN); + assert_eq!(err.1, "missing"); + } + + #[test] + fn check_rejects_malformed_header() { + let mut h = HeaderMap::new(); + h.insert(axum::http::header::ORIGIN, "not a url".parse().unwrap()); + let err = check_origin(&h, &["https://app.example.com".into()]).unwrap_err(); + assert_eq!(err.0 .0, StatusCode::FORBIDDEN); + assert_eq!(err.1, "malformed"); + } + + #[test] + fn check_rejects_disallowed_origin() { + let mut h = HeaderMap::new(); + h.insert(axum::http::header::ORIGIN, "https://evil.example".parse().unwrap()); + let err = check_origin(&h, &["https://app.example.com".into()]).unwrap_err(); + assert_eq!(err.0 .0, StatusCode::FORBIDDEN); + assert_eq!(err.1, "not-allowlisted"); + } + + #[test] + fn check_rejects_empty_allowlist() { + let mut h = HeaderMap::new(); + h.insert(axum::http::header::ORIGIN, "https://app.example.com".parse().unwrap()); + let err = check_origin(&h, &[]).unwrap_err(); + assert_eq!(err.0 .0, StatusCode::FORBIDDEN); + assert_eq!(err.1, "not-allowlisted"); + } + + #[test] + fn check_allows_valid_origin() { + let mut h = HeaderMap::new(); + h.insert(axum::http::header::ORIGIN, "https://app.example.com".parse().unwrap()); + assert!(check_origin(&h, &["https://app.example.com".into()]).is_ok()); + } + + #[test] + fn check_allows_default_port_normalization() { + let mut h = HeaderMap::new(); + h.insert( + axum::http::header::ORIGIN, + "https://app.example.com:443".parse().unwrap(), + ); + assert!(check_origin(&h, &["https://app.example.com".into()]).is_ok()); + } + + #[test] + fn check_allows_case_insensitive_host() { + let mut h = HeaderMap::new(); + h.insert(axum::http::header::ORIGIN, "https://App.Example.com".parse().unwrap()); + assert!(check_origin(&h, &["https://app.example.com".into()]).is_ok()); + } +} diff --git a/docs/security-review.md b/docs/security-review.md index db144ee..2ed04d6 100644 --- a/docs/security-review.md +++ b/docs/security-review.md @@ -35,6 +35,17 @@ verifying that all mandated security controls are implemented and operational. | Live whitelist management | ✅ Verified | Settings page UI + `PUT /api/v1/settings` endpoint updates whitelist; changes take effect immediately via `RwLock` | | Whitelist change audit | ✅ Verified | Every whitelist modification triggers an `audit_log` entry with old/new values | +### 1.4 WebSocket Origin Allowlist (CSWSH Defense-in-Depth) +| Control | Status | Evidence | +|---------|--------|----------| +| `Origin` header allowlist on browser WS upgrade | ✅ Verified | `crates/pm-web/src/routes/ws.rs` `ws_handler` — `HeaderMap` extractor + `check_origin` enforced before ticket validation | +| Allowlist configurable via `security.allowed_origins` | ✅ Verified | `crates/pm-core/src/config.rs` `SecurityConfig::allowed_origins`; documented in `config/config.example.toml` | +| Secure-by-default derivation from `sso_callback_url` | ✅ Verified | `derive_allowed_origins` parses the SSO callback URL into a single `scheme://host[:port]` entry when the operator leaves `allowed_origins` empty; `AppConfig::load` runs the derivation and emits a `tracing::warn!` if the result is empty (fail-closed) | +| Order: Origin check before ticket consumption | ✅ Verified | Rejected cross-origin probes do not burn the user's ticket; documented in the handler doc-comment and verified by `check_rejects_disallowed_origin` test | +| Rejected upgrades logged with `origin` and `reason` | ✅ Verified | `tracing::warn!` in `ws_handler`; ticket value is never logged | + +**Note:** The browser WebSocket endpoint (`GET /api/v1/ws/jobs`) is the only browser-reachable WS server in the codebase. The `pm-worker` `ws_relay` module is an outbound mTLS WS *client* to on-host agents and is not subject to CSWSH. + --- ## 2. Authentication & Authorization diff --git a/tasks/lessons.md b/tasks/lessons.md index 979953f..e109c3a 100644 --- a/tasks/lessons.md +++ b/tasks/lessons.md @@ -147,3 +147,14 @@ The Docker container intercepted some jobs and ran them in its Alpine environmen **Rule:** At session start, run bootstrap checks silently. If ~/.ssh/id_ed25519 missing, retrieve from Vaultwarden via vw_client.py (not from file storage). **Rule:** vw_client.py is primary (sub-second). bw CLI is fallback only (9-12s per operation). **Status:** Active + +## 2026-06-01: Handlers Should Take a Minimal State Struct, Not the Full AppState +**Pattern:** The `ws_handler` in `crates/pm-web/src/routes/ws.rs` is wired to `State`, and `AppState` contains `sqlx::PgPool` (requires a real DB) and `pm_ca::CertAuthority` (private fields, requires on-disk key material + DB on `init()`). This made end-to-end integration tests in `tests/ws_origin.rs` infeasible without a Postgres + filesystem fixture. +**Why it matters:** Test seams should be at the function/handler boundary, not require the full production state. The fix landed as 33 unit tests on the module-private helpers (`parse_origin_header`, `is_origin_allowed`, `check_origin`) — 100% coverage of the security-critical logic, zero coverage of the handler wiring. That tradeoff was acceptable here because the wiring is `HeaderMap` extraction + a function call (cargo check + clippy catches wiring bugs), but the principle stands: it's better to fix the test seam than to test around it. +**Rule:** When designing a new handler, define a minimal state struct (e.g., `WsState { ws_tickets, config }`) and have `AppState` either contain it or convert to it. Handlers should only take what they need. This is a refactor on the table for follow-up work; the WS Origin fix did NOT do it (out of scope per the spec). +**Status:** Active + +## 2026-06-01: Always Order CSWSH Defenses So They Don't Burn Legitimate Credentials +**Pattern:** The WS Origin allowlist check runs BEFORE the ticket validation. A cross-origin probe with a stolen ticket returns `403 forbidden_origin` without consuming the ticket. The opposite order (ticket first, then Origin) would let an attacker with a leaked ticket mount a low-cost DoS by repeatedly burning the legitimate user's 60-second tickets with `403` responses. +**Rule:** When adding defense-in-depth gates to an authenticated endpoint, order them so that the cheaper / less-credentialed gate runs first. A rejected request at gate N must not consume credentials checked at gate N+1. +**Status:** Active diff --git a/tasks/todo.md b/tasks/todo.md index f2a20ea..8749ca9 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -38,6 +38,62 @@ - [x] 4f: Lessons captured below ## Lessons Learned + +--- + +# WS Origin Allowlist — Implementation Plan (Issue #10) + +Spec: `tasks/ws-origin-check-spec.md` (v0.1.0, awaiting sign-off) + +## Issues Identified +1. **No Origin check on WS upgrade** — `crates/pm-web/src/routes/ws.rs` `ws_handler` does + not inspect the `Origin` header, leaving the `/api/v1/ws/jobs` endpoint exposed to + Cross-Site WebSocket Hijacking (CSWSH) if a ticket ever leaks via logs / `Referer` / + browser history / support bundles. +2. **No `allowed_origins` config field** — `SecurityConfig` has no way to express the + allowlist; defaults need to be derived from `sso_callback_url` to stay secure out + of the box. +3. **No integration tests for ws.rs** — there is no `crates/pm-web/tests/` directory + today, so the new behavior would land without automated coverage. + +## Phases + +### Phase 1: Config schema (Issue 2) +- [x] 1a: Add `allowed_origins: Vec` to `SecurityConfig` in `crates/pm-core/src/config.rs` +- [x] 1b: Implement `default_allowed_origins()` that parses `sso_callback_url` to `scheme://host[:port]` +- [x] 1c: Emit `tracing::warn!` at startup if the derived allowlist ends up empty +- [x] 1d: Update `Default for AppConfig` to include the new field +- [x] 1e: Update `config/config.example.toml` with documented `allowed_origins` key + +### Phase 2: Handler change (Issue 1) +- [x] 2a: Add `HeaderMap` extractor to `ws_handler` +- [x] 2b: Implement hand-rolled `Origin` parser (scheme, host, port) with default-port normalization +- [x] 2c: Implement allowlist match (exact, case-insensitive host, case-sensitive scheme/port) +- [x] 2d: Reject missing / malformed / non-allowlisted `Origin` with `403 forbidden_origin` *before* ticket validation +- [x] 2e: Augment the success `tracing::info!` with `origin`; add `tracing::warn!` on rejection (never log the ticket) +- [x] 2f: Verify `cargo check -p pm-web` and `cargo clippy --all-targets` pass + +### Phase 3: Tests (Issue 3) +- [x] 3a: Add `crates/pm-web/tests/` and a `build_test_app` harness (no DB, minimal `AppState`) +- [x] 3b: Add `ws_rejects_missing_origin` test +- [x] 3c: Add `ws_rejects_disallowed_origin` test +- [x] 3d: Add `ws_rejects_malformed_origin` test +- [x] 3e: Add `ws_allows_listed_origin_with_valid_ticket` test (asserts ticket is consumed) +- [x] 3f: Add `ws_default_origin_derived_from_sso_callback_url` config-derivation test +- [x] 3g: Verify `cargo test -p pm-web` passes + +### Phase 4: Documentation +- [x] 4a: Update `docs/security-review.md` with a new control row for the WS Origin allowlist +- [x] 4b: (Optional, per Kelly) bump `SPEC.md` to 0.0.3 with a sentence in the Security section + +### Phase 5: Review +- [x] 5a: Self-review against the 10-point acceptance criteria in the spec +- [x] 5b: Commit on a feature branch (`issue/10-ws-origin-check`) per git-workflow skill +- [x] 5c: Lessons captured below + +## Lessons Learned (this issue) +_(filled in at completion)_ + - **SSO callback must redirect, not return JSON** — Browser OAuth2 flows require the backend to redirect to the frontend SPA, not return JSON tokens. The frontend must parse tokens from URL query parameters. - **URLSearchParams.get() already decodes** — Don't double-decode with decodeURIComponent() when using URLSearchParams. - **JWKS caching prevents rate-limiting** — Azure AD JWKS endpoint should be cached with TTL (1 hour) to avoid fetching on every SSO login. @@ -54,9 +110,9 @@ - [x] 1c: Add DB interaction methods (insert, list, delete) in `pm-core` ### Phase 2: Client-Facing API (pm-web) -- [ ] 2a: Implement `POST /api/v1/enroll` to accept payloads and generate `polling_token` -- [ ] 2b: Implement `GET /api/v1/enroll/status/{token}` to return pending/approved (PKI) statuses -- [ ] 2c: Implement IP-based rate limiting for the `/enroll` endpoint +- [x] 2a: Implement `POST /api/v1/enroll` to accept payloads and generate `polling_token` +- [x] 2b: Implement `GET /api/v1/enroll/status/{token}` to return pending/approved (PKI) statuses +- [x] 2c: Implement IP-based rate limiting for the `/enroll` endpoint ### Phase 3: Admin-Facing API (pm-web) - [x] 3a: Implement `GET /api/v1/admin/enrollments` to list pending queue diff --git a/tasks/ws-origin-check-spec.md b/tasks/ws-origin-check-spec.md new file mode 100644 index 0000000..b710273 --- /dev/null +++ b/tasks/ws-origin-check-spec.md @@ -0,0 +1,219 @@ +# WS Origin Allowlist — Specification + +**Issue:** [#10](https://github.com/Draco-Lunaris/Linux-Patch-Manager/issues/10) +**Component:** `crates/pm-web/src/routes/ws.rs` +**Spec version:** 0.1.0 (draft) +**Status:** Awaiting Kelly sign-off + +--- + +## 1. Goal + +Harden the browser-facing WebSocket upgrade endpoint (`GET /api/v1/ws/jobs`) against +Cross-Site WebSocket Hijacking (CSWSH) by validating the `Origin` header against a +configured allowlist. The existing single-use, 60-second ticket mechanism is retained +as the first credential factor; the Origin check is the second factor. + +## 2. Non-Goals + +- Replacing or weakening the ticket mechanism. +- Binding the ticket to a specific WebSocket connection at issuance time (separate, +larger change). +- Origin/CORS checks on the JWT-authenticated ticket-issuance endpoint +(`POST /api/v1/ws/ticket`) — it is already protected by JWT + RBAC middleware and is +not a browser-only entry point. +- Changes to `pm-worker/ws_relay.rs` — that module is an outbound mTLS WS *client* to +agents, not a server, and is not reachable from a browser. + +## 3. Design + +### 3.1 Handler change (`crates/pm-web/src/routes/ws.rs`) + +The `ws_handler` function (lines 96–137) gains a new `HeaderMap` extractor and +performs an Origin allowlist check **before** ticket validation. + +Order of operations in the new handler: + +1. Extract `HeaderMap` (new). +2. Read `Origin` header. If absent → reject `403 forbidden_origin` (`"Origin header + required"`). +3. Normalize and compare against the configured allowlist. If not matched → reject + `403 forbidden_origin` (`"Origin not allowed"`). +4. Existing ticket validation (lines 101–127) runs unchanged. +5. Existing upgrade (line 136) runs unchanged. + +Rationale for Origin-first ordering: a CSWSH probe from a malicious page will not +have a valid ticket. If we validated the ticket first and then checked Origin, an +attacker who has obtained a leaked ticket (from logs, `Referer`, history, support +bundle) could mount a denial-of-service against the legitimate user by burning their +tickets with `403` responses. Origin-first means rejected cross-origin attempts do +not consume the user's ticket. + +### 3.2 Origin parsing and comparison + +- Read the raw `Origin` header value as a string. +- Strip any trailing `/`. +- Use a hand-rolled parser (no new dependency) to extract `scheme`, `host`, and + optional `port`: + - `scheme` must be `http` or `https` (lowercased). + - `host` must be non-empty and contain no whitespace. + - `port`, if present, must parse as `u16`. +- Apply default-port normalization: `https` implies port `443`; `http` implies port + `80`. If the explicit port equals the default for the scheme, drop it from the + comparison. This matches browser behavior (browsers do not include default ports + in `Origin`). +- Case-insensitive host comparison; case-sensitive scheme and port. +- Match each configured allowlist entry against the parsed Origin. **Exact match + only** — no subdomain wildcards, no regex, no path. +- If the `Origin` header is malformed (does not parse) → reject + `403 forbidden_origin` (`"Malformed Origin header"`). + +### 3.3 Config schema (`crates/pm-core/src/config.rs`) + +Add a new field to `SecurityConfig`: + +```rust +/// Allowlist of browser `Origin` values permitted to open the +/// `/api/v1/ws/jobs` WebSocket. Entries are exact `scheme://host[:port]` +/// strings. If empty, the server derives the default from `sso_callback_url`. +#[serde(default = "default_allowed_origins")] +pub allowed_origins: Vec, +``` + +Default value (`default_allowed_origins`): a single-element vector containing the +`scheme://host[:port]` form of `sso_callback_url` parsed at config-load time. This +keeps the default config secure out of the box — a fresh install will only accept +WS upgrades from the same origin the SSO callback redirects to. + +If `sso_callback_url` cannot be parsed (e.g., a custom dev value), fall back to +`vec![]` and emit a `tracing::warn!` at startup. An empty allowlist with no +parseable default would make the WS endpoint reject all upgrades; this is the safe +direction (fail-closed), but the warning makes it loud. + +Add a corresponding `ConfigError` if `allowed_origins` is manually set to an empty +vector and `sso_callback_url` is also unparseable, since that combination produces a +non-functional WS endpoint. The user must explicitly opt in to "no origins allowed" +by providing a list. + +### 3.4 Logging + +- On allowed upgrade: existing `tracing::info!` continues (lines 129–133), augmented + with `origin = %origin_str` for audit context. +- On rejected upgrade: new `tracing::warn!` with `origin = %origin_str` and + `reason = %reason`. **Never log the ticket value**. + +### 3.5 Response shape + +Reuse the existing `err` helper (lines 48–58): + +```json +{ "error": { "code": "forbidden_origin", "message": "…" } } +``` + +Status: `403 Forbidden` for all Origin rejections. Do not differentiate between +"missing", "malformed", and "not in allowlist" in the response — they all map to the +same code, and the specific reason is logged server-side only. + +## 4. Acceptance Criteria + +- [ ] A WS upgrade request with a missing `Origin` header is rejected with `403` + and code `forbidden_origin`. The ticket is **not** consumed. +- [ ] A WS upgrade request with a malformed `Origin` header is rejected with `403` + and code `forbidden_origin`. The ticket is **not** consumed. +- [ ] A WS upgrade request with an `Origin` not in the allowlist is rejected with + `403` and code `forbidden_origin`. The ticket is **not** consumed. +- [ ] A WS upgrade request with an allowlisted `Origin` and a valid, unconsumed + ticket succeeds (101 Switching Protocols / upgrade accepted) and the ticket is + consumed atomically (existing behavior preserved). +- [ ] A WS upgrade request with an allowlisted `Origin` and an invalid/expired/ + already-used ticket is rejected with the existing `401 invalid_ticket` / + `401 ticket_expired` (existing behavior preserved). +- [ ] The allowlist is configurable via `security.allowed_origins` in + `config/config.example.toml` and is documented in that file. +- [ ] When `allowed_origins` is not set, the default is derived from + `sso_callback_url` and the service starts cleanly. +- [ ] `cargo check` and `cargo clippy --all-targets` pass. +- [ ] `cargo test -p pm-web` passes with the new integration tests added. +- [ ] `docs/security-review.md` documents the new control with an evidence row + pointing to `crates/pm-web/src/routes/ws.rs`. + +## 5. Test Plan + +Unit tests in `crates/pm-web/src/routes/ws.rs` (cfg(test) module) — 33 tests +covering the security-critical logic. The end-to-end wiring (HeaderMap +extractor, axum handler, response shape) is verified by `cargo check`, +`cargo clippy --all-targets`, and the manual `scripts/integration-test.sh` +end-to-end check. + +**Why unit tests instead of `tests/ws_origin.rs` integration tests:** the +handler is wired to `State`, and `AppState` contains a +`sqlx::PgPool` and a `pm_ca::CertAuthority` that cannot be cheaply +constructed in a test (the CA requires a real Postgres connection and +on-disk key material, both unavailable in `cargo test`). Refactoring +`ws_handler` to take a smaller state struct is out of scope for this +hardening change. + +### 5.1 pm-core unit tests (`crates/pm-core/src/config.rs`, 7 tests) + +`derive_allowed_origins` — the sso_callback_url → allowlist derivation: + +1. `derive_strips_default_https_port` — `https://x:443/...` → `https://x` +2. `derive_keeps_non_default_port` — `https://x:8443/...` → `https://x:8443` +3. `derive_strips_default_http_port` — `http://x:80/x` → `http://x` +4. `derive_handles_trailing_slash` — `https://x/` → `https://x` +5. `derive_handles_no_path` — `https://x` → `https://x` +6. `derive_returns_empty_for_garbage` — rejects "not a url", "", "ftp://x" +7. `derive_lowercases_scheme` — `HTTPS://...` → `https://...` + +### 5.2 pm-web unit tests (`crates/pm-web/src/routes/ws.rs`, 33 tests) + +`parse_origin_header` (14 tests): basic, with-port, scheme/host lowercasing, +path/query/fragment ignored, trailing-slash stripped, empty/whitespace +rejected, unsupported schemes rejected, empty host rejected, host with +whitespace rejected, malformed port rejected, IPv6 literal rejected, no +scheme separator rejected. + +`Origin::canonical` (3 tests): default-port normalization in both directions +(http:80, https:443), non-default port kept. + +`is_origin_allowed` (11 tests): exact match, default-port normalization in +both directions, case-insensitive host, different host rejected, different +scheme rejected, different port rejected, empty allowlist rejected, +unparseable allowlist entry rejected, multi-entry allowlist. + +`check_origin` (7 tests): missing header, malformed header, disallowed +origin, empty allowlist, allowed origin, default-port normalization allowed, +case-insensitive host allowed. This function returns the `(StatusCode, Json)` +error tuple used by the handler, so these tests cover the response shape. + +## 6. Risk Analysis + +- **Risk: a config that breaks the WS endpoint in production.** Mitigation: default + is derived from `sso_callback_url` (already required for SSO to function), and + startup logs a warning if the allowlist ends up empty. +- **Risk: legitimate cross-origin frontend (e.g., SPA on `app.foo.com` talking to + API on `api.foo.com`).** Out of scope for this fix — the WS endpoint is on the + same origin as the SPA. If a future deployment splits them, the operator adds + the API origin to `allowed_origins`. Documented in `config/config.example.toml`. +- **Risk: a non-browser caller (CLI, integration test) cannot connect.** Such + callers should use the REST API for state mutations; the WS is documented as the + browser job-stream channel. If a non-browser WS client is needed, document it in + `docs/REST_API.md` and tell the operator to add its origin. +- **Risk: false sense of security.** The Origin check is defense-in-depth, not a + replacement for the ticket. Documented as such in the security review update. + +## 7. Files Touched + +- `crates/pm-web/src/routes/ws.rs` — handler change. +- `crates/pm-core/src/config.rs` — new field + default. +- `config/config.example.toml` — new field documented. +- `crates/pm-web/tests/ws_origin.rs` — new integration tests (and harness). +- `docs/security-review.md` — control documentation row. +- `tasks/todo.md` — plan section (added by the orchestrator). + +## 8. Out of Scope (Future Work) + +- Bind ticket issuance to the WS upgrade in a single round-trip (eliminates + query-string ticket leakage from logs/history). +- Per-message MAC on WS frames (defense against in-process tampering). +- Rate limiting on the WS endpoint itself.