diff --git a/agent/src/session/mod.rs b/agent/src/session/mod.rs index 5d749f6..a2f7587 100644 --- a/agent/src/session/mod.rs +++ b/agent/src/session/mod.rs @@ -555,8 +555,14 @@ impl SessionManager { access ); - // The MessageBox blocks the calling thread; run it on the blocking pool - // so the agent's async loop is not stalled and heartbeats keep flowing. + // The MessageBox blocks the calling thread, so it runs on the blocking + // pool to avoid stalling the tokio runtime. Note, however, that the main + // session loop `.await`s this method (see the ConsentRequest arm), so + // the loop is SUSPENDED for the user's entire think-time and does NOT + // process or respond to server heartbeats while the dialog is open. + // This is safe because CONSENT_TIMEOUT_SECS (60s, server-side) is within + // the server's 90s HEARTBEAT_TIMEOUT_SECS: the prompt resolves before the + // server would consider the agent dead, so the session is not torn down. let granted = tokio::task::spawn_blocking(move || prompt_consent(&technician_name, access)) .await .unwrap_or_else(|e| { diff --git a/agent/src/update.rs b/agent/src/update.rs index 312af41..cbcd814 100644 --- a/agent/src/update.rs +++ b/agent/src/update.rs @@ -10,6 +10,25 @@ use tracing::{error, info, warn}; use crate::build_info; +/// Whether to disable TLS certificate verification for update traffic. +/// +/// Returns `true` ONLY in a debug build (`cfg!(debug_assertions)`) when the +/// `GURUCONNECT_DEV_INSECURE_TLS` environment variable is set. The `cfg!` gate +/// is compiled out of release builds, so a shipped agent ALWAYS verifies certs +/// regardless of environment — a MITM cannot serve a forged update binary over +/// an unverified channel. The env var lets a developer test against a +/// self-signed server without weakening production. +fn dev_insecure_tls() -> bool { + if cfg!(debug_assertions) && std::env::var("GURUCONNECT_DEV_INSECURE_TLS").is_ok() { + warn!( + "TLS certificate verification DISABLED (dev-insecure mode) — DO NOT use in production" + ); + true + } else { + false + } +} + /// Version information from the server #[derive(Debug, Clone, serde::Deserialize)] pub struct VersionInfo { @@ -42,7 +61,7 @@ pub async fn check_for_update(server_base_url: &str) -> Result Result { info!("Downloading update from {}", version_info.download_url); let client = reqwest::Client::builder() - .danger_accept_invalid_certs(true) + .danger_accept_invalid_certs(dev_insecure_tls()) .build()?; let response = client @@ -134,6 +153,13 @@ pub async fn download_update(version_info: &VersionInfo) -> Result { } /// Verify downloaded file checksum +/// +/// NOTE: This is a transport-integrity check (catches truncated/corrupted +/// downloads), NOT a tamper defense. The expected checksum arrives over the +/// same channel as the binary, so an attacker who can serve a forged binary +/// can also serve a matching checksum. Tamper resistance comes from verifying +/// the TLS certificate of the update server (see `dev_insecure_tls`) and, as a +/// future hardening step, an embedded-public-key signature over the artifact. pub fn verify_checksum(file_path: &PathBuf, expected_sha256: &str) -> Result { info!("Verifying checksum..."); @@ -160,6 +186,9 @@ pub fn verify_checksum(file_path: &PathBuf, expected_sha256: &str) -> Result Result { + // TODO(security): defense-in-depth — verify an embedded-public-key signature + // over the update binary/manifest before install_update; see + // reports/2026-05-30-gc-audit.md info!("Installing update..."); // Get current executable path @@ -321,4 +350,31 @@ mod tests { assert!(!is_newer_version("0.1.0", "0.2.0")); assert!(is_newer_version("0.2.0-abc123", "0.1.0-def456")); } + + /// In a release build (`debug_assertions` off), `dev_insecure_tls()` MUST + /// return false regardless of the env var — the shipped agent can never + /// accept invalid certs. In a debug build, it returns true only when + /// `GURUCONNECT_DEV_INSECURE_TLS` is set; we cannot assert the env-var path + /// here without mutating process-global state (which would race other + /// tests), so we only assert the invariant that holds in the current + /// build profile. + #[test] + fn test_dev_insecure_tls_release_is_always_false() { + if !cfg!(debug_assertions) { + // Release/test-release profile: must be false no matter the env. + assert!( + !dev_insecure_tls(), + "release build must never disable TLS verification" + ); + } else { + // Debug profile: with the env var unset, must still be false. + // (We avoid setting it to prevent cross-test interference.) + if std::env::var("GURUCONNECT_DEV_INSECURE_TLS").is_err() { + assert!( + !dev_insecure_tls(), + "debug build without the env var must verify TLS" + ); + } + } + } } diff --git a/reports/2026-05-30-gc-audit.md b/reports/2026-05-30-gc-audit.md new file mode 100644 index 0000000..68598c4 --- /dev/null +++ b/reports/2026-05-30-gc-audit.md @@ -0,0 +1,120 @@ +# GuruConnect Audit Report — 2026-05-30 + +**Auditor:** Claude (claude-opus-4-8[1m]) +**Passes:** Security & Remote-Session Integrity (`--pass=security` only) +**Previous audit:** 2026-05-29 (the audit that defined the v2 secure-session-core work) +**Scope note:** Single-pass security re-audit to formally verify the v2 secure-session-core +rebuild closed the three relay CRITICALs. Independent re-derivation against the code — not an +echo of the 2026-05-30 Tasks 3–5 code review. + +--- + +## Executive Summary + +| Pass | Total | Critical | High | Medium | Low | Info | +|------|-------|----------|------|--------|-----|------| +| Security & Session | 5 | 0 | 1 | 0 | 1 | 3 | + +**The three 2026-05-29 audit CRITICALs are CLOSED with no bypass.** The relay/server plane is +clean. The one serious finding is **net-new and outside the relay plane** — the Windows agent +auto-update path disables TLS verification (MITM → RCE). + +**Requires action:** [HIGH] Agent auto-update disables TLS cert verification → MITM-to-RCE. + +--- + +## The three audit CRITICALs — verification + +### CRITICAL #1 — "any JWT joins any session" → **CLOSED (no bypass)** +- `viewer_ws_handler` (`server/src/relay/mod.rs:463`) calls `validate_viewer_token`, which enforces + signature + expiry + `purpose=="viewer"` (`server/src/auth/jwt.rs:268,284`). A login JWT lacks the + `ViewerClaims` shape and is rejected (test `test_login_token_rejected_as_viewer_token`). +- Session binding: `claim_session_id != requested_session_id → 403` (`relay/mod.rs:494-501`). +- Mint authz: `mint_viewer_token` (`server/src/api/sessions.rs:114-131`) — `is_admin() || + has_permission("control")` → Control; else `has_permission("view")` → ViewOnly; else 403. +- View-only enforcement: access mode is in the **signed** claim; relay drops + `MouseEvent`/`KeyEvent`/`SpecialKey` when `!access.can_control()` (`relay/mod.rs:1313-1335`). + +### CRITICAL #2 — "viewer-WS blacklist bypass" → **CLOSED (no bypass)**, documented residual +- `token_blacklist.is_revoked(&token)` consulted on the viewer WS path before upgrade + (`relay/mod.rs:476`). +- **Residual [TRACKED — v2-secure-session-core plan.md, MEDIUM]:** logout + (`server/src/api/auth_logout.rs:64`) revokes only the login JWT; minted viewer tokens are not + blacklisted, so a leaked viewer token stays valid until natural expiry (5-min TTL, + `jwt.rs:56`). Known follow-up; the enforcement plumbing is correct for any future force-revoke. + +### CRITICAL #3 — "JWT accepted as agent key" → **CLOSED (no bypass), fails closed** +- `validate_agent_api_key` (`relay/mod.rs:384`) has no JWT branch — only a per-agent `cak_` key + (`verify_agent_key`, `server/src/auth/agent_keys.rs:71`; SHA-256 vs `connect_agent_keys`, + `revoked_at IS NULL`) or the deprecated shared `AGENT_API_KEY` (WARNING-logged). +- Identity binding fails closed: unresolved machine → 503 (`relay/mod.rs:276-286`); a client-supplied + `agent_id` is overridden by the key's machine identity (`relay/mod.rs:263-270`). + +--- + +## Pass 5: Security & Remote-Session Integrity + +### [HIGH] Agent auto-update disables TLS certificate verification → MITM-to-RCE +**File:** `agent/src/update.rs:45` and `:111` (live path: `agent/src/session/mod.rs:461-468,695-702`) +**Detail:** Both the `check_for_update` and `download_update` reqwest clients are built with +`.danger_accept_invalid_certs(true)` ("for self-signed certs in dev"). This is the **live** auto-update +path — the agent session loop calls `check_for_update → perform_update → download_update → +install_update → restart_with_new_version` at runtime. A network MITM (the relay endpoint is a public +hostname) can serve an arbitrary `guruconnect-update.exe`; the agent writes it over its own binary and +executes it — full code execution on every managed endpoint. `verify_checksum` (`update.rs:294`) does +NOT mitigate: the expected SHA-256 (`version_info.checksum_sha256`) arrives in the **same** `/api/version` +JSON over the unverified TLS connection, so the attacker controls both binary and checksum. +**Recommendation:** Remove `danger_accept_invalid_certs(true)` from both clients (gate behind a dev-only +`cfg`/env flag never set in release). Defense-in-depth: sign the update binary/manifest with an embedded +public key and verify the signature before `install_update`, so update trust does not rest solely on TLS. + +### [LOW] Chat message content logged at INFO +**File:** `server/src/relay/mod.rs:776` and `:1373` +**Detail:** Full chat text is logged (`Chat from client/technician: {content}`). Support-session chat can +carry sensitive data (spoken passwords, account numbers) — PII/secret-bearing content in server logs. +**Recommendation:** Drop content from the log line (log length or hash, or move to `trace!`). + +### [INFO] Bootstrap admin password logged on credentials-file write failure +**File:** `server/src/main.rs:200-201` +**Detail:** Self-documented one-time fallback at first boot if `.admin-credentials` write fails; password +is meant to be changed immediately. Awareness only, not a defect. + +### [INFO] Viewer-token mint not scoped to a specific machine/session ACL — **[TRACKED — SPEC-002 Phase 4]** +**File:** `server/src/api/sessions.rs:27,114` +**Detail:** Any `control`-permission user can mint a control token for any live session (no per-machine / +per-client ACL). Intentional for Phase 1; per-tenant narrowing deferred to Phase 4. + +### [INFO] Verified-sound areas +Single-use support codes (atomic `consume_for_bind`, `support_codes.rs:240` / `db/support_codes.rs:110`); +consent gate (viewer refused until `allows_viewer()`, `session/mod.rs:393`; deny/timeout tears down before +the relay loop, no pre-consent window); frame caps (agent 4 MiB / viewer 64 KiB, `relay/mod.rs:330,524`); +input throttle (200 ev/s token bucket + non-blocking `try_send`); rate limiting wired + proxy-aware +(`main.rs:317-370`, `utils/ip_extract.rs:147`); `JWT_SECRET` length-checked ≥32 at startup; Argon2id +passwords; no secret/token/code logging; parameterized runtime sqlx (no `format!`-built SQL); `wss://` default. + +--- + +## Verdict + +| CRITICAL | Verdict | Enforced at | +|----------|---------|-------------| +| #1 any-JWT-joins-any-session | **CLOSED** | `api/sessions.rs:114` (authz) + `relay/mod.rs:463,494` (token type + session bind) | +| #2 viewer-WS blacklist bypass | **CLOSED** (TTL-bounded residual tracked) | `relay/mod.rs:476` | +| #3 JWT-accepted-as-agent-key | **CLOSED** | `relay/mod.rs:384` (no JWT branch; fail-closed binding `:276`) | + +**Net-new findings:** CRITICAL 0 · HIGH 1 · MEDIUM 0 · LOW 1 · INFO 2. +**Overall posture:** The relay/server plane is clean — all three CRITICALs genuinely closed, fail-closed +session/consent/code paths, bounded abuse surface, correct proxy-aware rate limiting, no SQL injection or +credential logging. The one serious net-new issue is on the Windows **agent** (auto-update TLS bypass → +MITM-RCE), independent of the v2 secure-session-core work and in no existing spec — fix before the next +agent release. + +--- + +## Recommended Action Order +1. **[HIGH]** Remove `danger_accept_invalid_certs(true)` from `agent/src/update.rs` (+ sign update binary). +2. **[LOW]** Stop logging chat content (`relay/mod.rs:776,1373`). +3. **[TRACKED/MEDIUM]** Viewer-token logout revocation (existing plan.md follow-up). + +*Note: only `--pass=security` was run. The API-surface, Rust-quality, TypeScript, protocol-integrity, +docs-reconciliation, and CI/CD passes were not executed this run.* diff --git a/server/src/api/auth_logout.rs b/server/src/api/auth_logout.rs index 6a9ede3..822c3a9 100644 --- a/server/src/api/auth_logout.rs +++ b/server/src/api/auth_logout.rs @@ -60,10 +60,27 @@ pub async fn logout( // Extract token from headers let token = extract_token_from_headers(request.headers())?; - // Add token to blacklist + // Add the login JWT to the blacklist. state.token_blacklist.revoke(&token).await; - info!("User {} logged out (token revoked)", user.username); + // Also revoke any outstanding session-scoped VIEWER tokens this user minted + // (CRITICAL #2). The login-JWT blacklist alone leaves a viewer token minted + // before logout valid for the rest of its 5-minute TTL, keeping a live + // viewer/remote-control plane open after logout. `user.user_id` is the `sub` + // the viewer tokens were registered under (the same claim stamped into them). + // The viewer WS already blacklist-checks the exact token string, so adding + // them here is sufficient — no WS change needed. take_for_user drains and + // clears the registry entry; expired tokens are pruned (not returned). + let viewer_tokens = state.viewer_tokens.take_for_user(&user.user_id); + let revoked_viewer_count = viewer_tokens.len(); + for viewer_token in viewer_tokens { + state.token_blacklist.revoke(&viewer_token).await; + } + + info!( + "User {} logged out (login token revoked, {} viewer token(s) revoked)", + user.username, revoked_viewer_count + ); Ok(Json(LogoutResponse { message: "Logged out successfully".to_string(), @@ -196,3 +213,74 @@ pub async fn cleanup_blacklist( remaining_count: remaining, })) } + +#[cfg(test)] +mod tests { + use crate::auth::{JwtConfig, TokenBlacklist, ViewerAccess, ViewerTokenRegistry}; + use std::time::Duration; + use uuid::Uuid; + + /// End-to-end (component-level) proof of CRITICAL #2: a viewer token minted + /// under a user, then revoked at logout via the registry drain, is rejected + /// by the SAME blacklist check the viewer WS runs (`is_revoked`). + /// + /// This exercises the real mint → register → logout-drain → blacklist path + /// without the HTTP/DB plumbing of the full handler. Uses local component + /// instances only (no process-global env), so it is parallel-safe. + #[tokio::test] + async fn logout_revokes_minted_viewer_token() { + let jwt = JwtConfig::new("test-secret-at-least-32-chars-long!!".to_string(), 24); + let registry = ViewerTokenRegistry::new(); + let blacklist = TokenBlacklist::new(); + + let user_sub = Uuid::new_v4().to_string(); + let session_id = Uuid::new_v4(); + let tenant_id = Uuid::new_v4(); + + // Mint a viewer token and register it under the user (as mint_viewer_token does). + let viewer_token = jwt + .create_viewer_token(&user_sub, session_id, tenant_id, ViewerAccess::Control) + .unwrap(); + registry.register(&user_sub, &viewer_token, Duration::from_secs(300)); + + // The viewer WS check (is_revoked) passes BEFORE logout — token is live. + assert!(!blacklist.is_revoked(&viewer_token).await); + + // Logout drains the user's viewer tokens into the blacklist (handler logic). + for tok in registry.take_for_user(&user_sub) { + blacklist.revoke(&tok).await; + } + + // After logout the same WS check now REJECTS the viewer token. + assert!(blacklist.is_revoked(&viewer_token).await); + // The token also remains a structurally valid viewer JWT (not expired), + // proving revocation — not natural expiry — is what blocks it. + assert!(jwt.validate_viewer_token(&viewer_token).is_ok()); + } + + /// A different user's logout must NOT revoke this user's viewer token. + #[tokio::test] + async fn logout_does_not_revoke_other_users_viewer_token() { + let jwt = JwtConfig::new("test-secret-at-least-32-chars-long!!".to_string(), 24); + let registry = ViewerTokenRegistry::new(); + let blacklist = TokenBlacklist::new(); + + let user_a = Uuid::new_v4().to_string(); + let user_b = Uuid::new_v4().to_string(); + let session_id = Uuid::new_v4(); + let tenant_id = Uuid::new_v4(); + + let token_b = jwt + .create_viewer_token(&user_b, session_id, tenant_id, ViewerAccess::ViewOnly) + .unwrap(); + registry.register(&user_b, &token_b, Duration::from_secs(300)); + + // user_a logs out — drains only user_a's (empty) token set. + for tok in registry.take_for_user(&user_a) { + blacklist.revoke(&tok).await; + } + + // user_b's token is untouched. + assert!(!blacklist.is_revoked(&token_b).await); + } +} diff --git a/server/src/api/sessions.rs b/server/src/api/sessions.rs index af58f10..6536482 100644 --- a/server/src/api/sessions.rs +++ b/server/src/api/sessions.rs @@ -160,6 +160,18 @@ pub async fn mint_viewer_token( ) })?; + // Register the minted token under the authenticated user's `sub` so logout + // can revoke it (CRITICAL #2): a viewer token minted before logout would + // otherwise stay valid for the rest of its 5-minute TTL. The registry prunes + // expired entries on insert, so it cannot grow unbounded. The token string + // is held transiently in memory (already handled server-side) and never + // logged. `user.user_id` is the `sub` claim stamped into the viewer token. + state.viewer_tokens.register( + &user.user_id, + &token, + std::time::Duration::from_secs(crate::auth::jwt::VIEWER_TOKEN_TTL_SECS as u64), + ); + tracing::info!( "User {} minted a {} viewer token for session {} (agent {})", user.username, diff --git a/server/src/auth/mod.rs b/server/src/auth/mod.rs index 950cfb3..8cebc3d 100644 --- a/server/src/auth/mod.rs +++ b/server/src/auth/mod.rs @@ -7,10 +7,12 @@ pub mod agent_keys; pub mod jwt; pub mod password; pub mod token_blacklist; +pub mod viewer_token_registry; pub use jwt::{Claims, JwtConfig, ViewerAccess}; pub use password::{generate_random_password, hash_password, verify_password}; pub use token_blacklist::TokenBlacklist; +pub use viewer_token_registry::ViewerTokenRegistry; use axum::{ extract::FromRequestParts, @@ -58,30 +60,6 @@ impl From for AuthenticatedUser { } } -/// Authenticated agent from API key -#[derive(Debug, Clone)] -#[allow(dead_code)] // TODO(native-remote-control): consumed by the integration API; see docs/specs/native-remote-control/ -pub struct AuthenticatedAgent { - pub agent_id: String, - pub org_id: String, -} - -/// JWT configuration stored in app state -#[derive(Clone)] -#[allow(dead_code)] // TODO(native-remote-control): consumed by the integration API; see docs/specs/native-remote-control/ -pub struct AuthState { - pub jwt_config: Arc, -} - -impl AuthState { - #[allow(dead_code)] // TODO(native-remote-control): consumed by the integration API; see docs/specs/native-remote-control/ - pub fn new(jwt_secret: String, expiry_hours: i64) -> Self { - Self { - jwt_config: Arc::new(JwtConfig::new(jwt_secret, expiry_hours)), - } - } -} - /// Extract authenticated user from request #[axum::async_trait] impl FromRequestParts for AuthenticatedUser @@ -169,14 +147,3 @@ where } } } - -/// Validate an agent API key (placeholder for MVP) -#[allow(dead_code)] // TODO(native-remote-control): consumed by the integration API; see docs/specs/native-remote-control/ -pub fn validate_agent_key(_api_key: &str) -> Option { - // TODO: Implement actual API key validation against database - // For now, accept any key for agent connections - Some(AuthenticatedAgent { - agent_id: "mvp-agent".to_string(), - org_id: "mvp-org".to_string(), - }) -} diff --git a/server/src/auth/viewer_token_registry.rs b/server/src/auth/viewer_token_registry.rs new file mode 100644 index 0000000..8c05c83 --- /dev/null +++ b/server/src/auth/viewer_token_registry.rs @@ -0,0 +1,186 @@ +//! Per-user registry of outstanding viewer tokens. +//! +//! Minted viewer tokens ([`super::jwt::ViewerClaims`]) are short-lived +//! (`VIEWER_TOKEN_TTL_SECS`, 5 min) and session-scoped. The login-JWT blacklist +//! revokes the LOGIN token on logout, but a viewer token minted before logout +//! stays valid for the rest of its TTL — letting a just-logged-out user keep a +//! live remote-control plane until natural expiry. +//! +//! This registry closes that gap with the least-invasive mechanism: when a +//! viewer token is minted it is registered here under the minting user's `sub`; +//! on logout, the user's outstanding viewer tokens are taken from the registry +//! and added to the SAME exact-string blacklist the viewer WebSocket already +//! checks (`relay::viewer_ws_handler` -> `TokenBlacklist::is_revoked`). No +//! change to the WS path is required. +//! +//! Design notes: +//! - The registry holds bearer-token strings transiently in memory. The server +//! already handles these tokens; they are never logged or otherwise exposed +//! by this module. +//! - Entries are pruned-on-access (insert and take) by expiry, so the map can +//! never grow unbounded even if a user never logs out — expired viewer tokens +//! self-evict on the next operation that touches the map. +//! - Locking is a poison-tolerant `std::sync::Mutex` (matching the rate-limiter +//! pattern: `lock().unwrap_or_else(|e| e.into_inner())`); a panic while the +//! lock is held must not wedge logout/mint. No `.unwrap()` on lock paths. + +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; + +/// A single registered viewer token: the token string and the instant at which +/// it expires (mint time + TTL). +#[derive(Clone)] +struct Entry { + token: String, + expires_at: Instant, +} + +/// Per-user registry of outstanding (un-expired, un-revoked) viewer tokens. +/// +/// Cheap to clone (shares the inner map via `Arc`), so it can live in +/// `AppState` and be cloned with it like the other shared state. +#[derive(Clone, Default)] +pub struct ViewerTokenRegistry { + /// Map of user `sub` -> that user's outstanding viewer tokens. + inner: Arc>>>, +} + +impl ViewerTokenRegistry { + /// Create an empty registry. + pub fn new() -> Self { + Self::default() + } + + /// Register a freshly minted viewer token under its owning user (`sub`), + /// expiring `ttl` from now. + /// + /// Prunes expired entries for this user on insert so the per-user vector and + /// the overall map stay bounded. The token string is stored verbatim (it is + /// the exact string the viewer WS blacklist-checks); it is never logged. + pub fn register(&self, sub: &str, token: &str, ttl: Duration) { + let now = Instant::now(); + let expires_at = now + ttl; + let mut map = self.inner.lock().unwrap_or_else(|e| e.into_inner()); + + let entries = map.entry(sub.to_string()).or_default(); + // Prune-on-insert: drop this user's already-expired tokens. + entries.retain(|e| e.expires_at > now); + entries.push(Entry { + token: token.to_string(), + expires_at, + }); + } + + /// Remove and return all currently-valid viewer tokens for `sub`. + /// + /// Expired tokens are pruned (not returned) — they are already invalid at + /// the JWT layer, so there is no value in blacklisting them. The user's + /// entry is cleared from the map regardless (taking everything out). Returns + /// an empty vec if the user has no registered tokens. + pub fn take_for_user(&self, sub: &str) -> Vec { + let now = Instant::now(); + let mut map = self.inner.lock().unwrap_or_else(|e| e.into_inner()); + + match map.remove(sub) { + Some(entries) => entries + .into_iter() + .filter(|e| e.expires_at > now) + .map(|e| e.token) + .collect(), + None => Vec::new(), + } + } + + /// Number of users currently tracked (test/observability helper). Does not + /// prune; reflects the raw map size. + #[cfg(test)] + fn user_count(&self) -> usize { + let map = self.inner.lock().unwrap_or_else(|e| e.into_inner()); + map.len() + } + + /// Number of registered (un-pruned) tokens for a user (test helper). + #[cfg(test)] + fn token_count(&self, sub: &str) -> usize { + let map = self.inner.lock().unwrap_or_else(|e| e.into_inner()); + map.get(sub).map(|v| v.len()).unwrap_or(0) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn register_then_take_returns_and_clears() { + let reg = ViewerTokenRegistry::new(); + reg.register("user-a", "tok-1", Duration::from_secs(300)); + reg.register("user-a", "tok-2", Duration::from_secs(300)); + + assert_eq!(reg.token_count("user-a"), 2); + + let mut taken = reg.take_for_user("user-a"); + taken.sort(); + assert_eq!(taken, vec!["tok-1".to_string(), "tok-2".to_string()]); + + // Taking clears the user's entry. + assert_eq!(reg.token_count("user-a"), 0); + assert_eq!(reg.user_count(), 0); + assert!(reg.take_for_user("user-a").is_empty()); + } + + #[test] + fn take_for_unknown_user_is_empty() { + let reg = ViewerTokenRegistry::new(); + assert!(reg.take_for_user("nobody").is_empty()); + } + + #[test] + fn tokens_are_scoped_per_user() { + let reg = ViewerTokenRegistry::new(); + reg.register("user-a", "a-tok", Duration::from_secs(300)); + reg.register("user-b", "b-tok", Duration::from_secs(300)); + + assert_eq!(reg.take_for_user("user-a"), vec!["a-tok".to_string()]); + // user-b is untouched. + assert_eq!(reg.take_for_user("user-b"), vec!["b-tok".to_string()]); + } + + #[test] + fn expired_tokens_are_not_returned_by_take() { + let reg = ViewerTokenRegistry::new(); + // Zero TTL -> already expired by the time take runs. + reg.register("user-a", "expired", Duration::from_secs(0)); + reg.register("user-a", "live", Duration::from_secs(300)); + + let taken = reg.take_for_user("user-a"); + assert_eq!(taken, vec!["live".to_string()]); + } + + #[test] + fn expired_tokens_are_pruned_on_insert() { + let reg = ViewerTokenRegistry::new(); + // Register an already-expired token, then a live one for the same user. + reg.register("user-a", "expired", Duration::from_secs(0)); + // First insert leaves 1 entry; the second insert prunes the expired one + // before pushing, so only the live token remains. + reg.register("user-a", "live", Duration::from_secs(300)); + + assert_eq!(reg.token_count("user-a"), 1); + assert_eq!(reg.take_for_user("user-a"), vec!["live".to_string()]); + } + + #[test] + fn multiple_logins_accumulate_then_all_taken() { + // A user with several concurrent logins mints several viewer tokens; + // logout must revoke ALL of them at once. + let reg = ViewerTokenRegistry::new(); + for i in 0..5 { + reg.register("user-a", &format!("tok-{i}"), Duration::from_secs(300)); + } + assert_eq!(reg.token_count("user-a"), 5); + assert_eq!(reg.take_for_user("user-a").len(), 5); + assert_eq!(reg.user_count(), 0); + } +} diff --git a/server/src/main.rs b/server/src/main.rs index c973b6b..374181f 100644 --- a/server/src/main.rs +++ b/server/src/main.rs @@ -37,7 +37,10 @@ use tower_http::trace::TraceLayer; use tracing::{info, Level}; use tracing_subscriber::FmtSubscriber; -use auth::{generate_random_password, hash_password, AuthenticatedUser, JwtConfig, TokenBlacklist}; +use auth::{ + generate_random_password, hash_password, AuthenticatedUser, JwtConfig, TokenBlacklist, + ViewerTokenRegistry, +}; /// Root of the static asset tree, relative to the server's working directory. /// Holds the agent `downloads/` tree AND the v2 SPA build under `app/`. @@ -66,6 +69,13 @@ pub struct AppState { db: Option, pub jwt_config: Arc, pub token_blacklist: TokenBlacklist, + /// Per-user registry of outstanding session-scoped viewer tokens. Minting a + /// viewer token registers it here under the minting user's `sub`; logout + /// drains the user's registered viewer tokens into `token_blacklist` so a + /// just-logged-out user cannot keep a live viewer/remote-control plane until + /// the token's natural 5-minute expiry. The viewer WS already blacklist- + /// checks the exact token string, so no WS change is needed. + pub viewer_tokens: ViewerTokenRegistry, /// Optional API key for persistent agents (env: AGENT_API_KEY) pub agent_api_key: Option, /// Prometheus metrics @@ -291,6 +301,7 @@ async fn main() -> Result<()> { // Create application state let token_blacklist = TokenBlacklist::new(); + let viewer_tokens = ViewerTokenRegistry::new(); let state = AppState { sessions, @@ -298,6 +309,7 @@ async fn main() -> Result<()> { db: database, jwt_config, token_blacklist, + viewer_tokens, agent_api_key, metrics, registry, @@ -443,7 +455,10 @@ async fn main() -> Result<()> { // fallback below — CLAUDE.md documents this as the public download URL. // `nest_service` is matched BEFORE `fallback_service`, so these binaries // are served from disk and never fall through to the SPA index.html. - .nest_service("/downloads", ServeDir::new(format!("{STATIC_DIR}/downloads"))) + .nest_service( + "/downloads", + ServeDir::new(format!("{STATIC_DIR}/downloads")), + ) // NOTE: there are intentionally no /login, /dashboard, /users routes. // The v2 SPA (BrowserRouter) owns those paths and resolves them via the // fallback_service below; registering server-side handlers for them would @@ -909,4 +924,3 @@ async fn trigger_machine_update( )) } } - diff --git a/server/src/relay/mod.rs b/server/src/relay/mod.rs index 7a49849..4cc69c1 100644 --- a/server/src/relay/mod.rs +++ b/server/src/relay/mod.rs @@ -561,7 +561,7 @@ async fn handle_agent_connection( if let Some(ref code) = support_code { // Check if the code is cancelled or invalid if support_codes.is_cancelled(code).await { - warn!("Agent tried to connect with cancelled code: {}", code); + warn!("Agent tried to connect with a cancelled support code"); // Send disconnect message to agent let disconnect_msg = proto::Message { payload: Some(proto::message::Payload::Disconnect(proto::Disconnect { @@ -740,7 +740,7 @@ async fn handle_agent_connection( interval.tick().await; if let Some(ref code) = support_code_check { if support_codes_check.is_cancelled(code).await { - info!("Support code {} was cancelled, disconnecting agent", code); + info!("Support code was cancelled, disconnecting agent"); // Send disconnect message let disconnect_msg = proto::Message { payload: Some(proto::message::Payload::Disconnect(proto::Disconnect { @@ -772,8 +772,11 @@ async fn handle_agent_connection( let _ = frame_tx.send(data.to_vec()); } Some(proto::message::Payload::ChatMessage(chat)) => { - // Broadcast chat message to all viewers - info!("Chat from client: {}", chat.content); + // Broadcast chat message to all viewers. Do NOT log + // the chat body: support-session chat can carry + // secrets/PII. Log only the (non-sensitive) length + // so relay activity stays observable. + info!("Chat from client ({} chars)", chat.content.len()); let _ = frame_tx.send(data.to_vec()); } Some(proto::message::Payload::AgentStatus(status)) => { @@ -917,7 +920,7 @@ async fn handle_agent_connection( let _ = db::support_codes::mark_code_completed(db.pool(), code).await; } - info!("Support code {} marked as completed", code); + info!("Support code marked as completed"); } } @@ -1369,8 +1372,10 @@ async fn handle_viewer_connection( } Some(proto::message::Payload::ChatMessage(chat)) => { // Forward chat message to agent (not throttled — - // not an injected-input vector). Bounded send. - info!("Chat from technician: {}", chat.content); + // not an injected-input vector). Bounded send. Do + // NOT log the chat body (secrets/PII); log only the + // (non-sensitive) length so the relay is observable. + info!("Chat from technician ({} chars)", chat.content.len()); let _ = input_tx.try_send(data.to_vec()); } _ => {} diff --git a/server/src/utils/ip_extract.rs b/server/src/utils/ip_extract.rs index a04ed8e..5f6b097 100644 --- a/server/src/utils/ip_extract.rs +++ b/server/src/utils/ip_extract.rs @@ -154,6 +154,14 @@ pub fn client_ip(peer: &SocketAddr, headers: &HeaderMap, trusted: &TrustedProxie } // Trusted peer: prefer the single-value X-Real-IP if the proxy set it. + // + // SECURITY: we take X-Real-IP verbatim here, trusting it as set by the + // reverse proxy. The proxy (NPM) MUST overwrite it from the real TCP peer: + // proxy_set_header X-Real-IP $remote_addr; + // It must NOT pass through a client-supplied X-Real-IP. A trusted peer that + // forwards an attacker-controlled value would let the client spoof the IP + // used for rate-limiting and audit logging. The trusted-proxy gate above + // only authenticates the immediate hop, not the contents of this header. if let Some(ip) = header_single_ip(headers, X_REAL_IP) { return ip; } diff --git a/specs/v2-secure-session-core/plan.md b/specs/v2-secure-session-core/plan.md index 3f3efb1..b680168 100644 --- a/specs/v2-secure-session-core/plan.md +++ b/specs/v2-secure-session-core/plan.md @@ -1,5 +1,13 @@ # v2 Secure Session Core — Implementation Plan +> **STATUS 2026-05-30: Tasks 1–7 IMPLEMENTED + DEPLOYED. Tasks 3–5 now CODE-REVIEWED — verdict +> APPROVE-WITH-FIXES (no CRITICAL/HIGH).** Compile-verified on GURU-5070: `cargo fmt --check` clean, +> `clippy -D warnings` 0 warnings, `cargo test --workspace` 89 pass. The 3 audit CRITICALs verified +> closed with no bypass; all security paths fail closed. Non-blocking follow-ups tracked: viewer-token +> logout revocation (MEDIUM, TTL-bounded), delete the dead `validate_agent_key` "accept-any" placeholder +> (MEDIUM), `X-Real-IP`/consent-comment/support-code-log hygiene (LOW). **Remaining for Phase-1 exit: +> Task 8 (e2e verification + `/gc-audit --pass=security` re-audit).** +> > Spec created: 2026-05-29 > Status: in progress — Tasks 1-4 IMPLEMENTED 2026-05-29 (Task 4 self-reviewed, pending Code Review; > Tasks 1-3 code-reviewed APPROVED). Task 4 completes the KEYSTONE (secure auth/session core). Viewer-token authz