Mike Swanson
ded99c5882
Independent reviews of server/agent/dashboard by Gemini 3.1 Pro and Grok 4.3, each reading source via its own read_file tool, plus a Claude synthesis and a phased remediation plan. Top convergent finding: secrets/tokens in WebSocket URL query strings across agent + dashboard. See SYNTHESIS-three-way.md and REMEDIATION-PLAN.md.
111 lines
11 KiB
Markdown
111 lines
11 KiB
Markdown
# GuruConnect — Remediation Plan (from the 2026-06-05 three-way review)
|
||
|
||
Derived from `SYNTHESIS-three-way.md` (Claude + Gemini + Grok). Findings are grouped into
|
||
shippable phases. Each phase is sized to a GuruConnect spec and should go through the
|
||
standard pipeline (spec → three-way design review → build → three-way code review → deploy →
|
||
validate). Phase numbers are priority order, not strict dependency order — P0 and parts of
|
||
P4/P5 can run in parallel with the larger P1/P2/P3 efforts.
|
||
|
||
Effort key: **S** = <0.5 day, **M** = 0.5–2 days, **L** = >2 days / multi-component.
|
||
Finding IDs (C#/H#) reference the synthesis report.
|
||
|
||
---
|
||
|
||
## P0 — Emergency hardening pass (proposed SPEC-019)
|
||
Small, self-contained, high-value. No protocol or cross-component coordination needed. Ship first.
|
||
|
||
| Finding | File(s) | Action | Effort |
|
||
|---|---|---|---|
|
||
| **C2** | `server/src/api/downloads.rs` | Remove the `"managed-agent"` default API-key fallback and the hardcoded `wss://connect.azcomputerguru.com/...` URL; move both into `Config`/`AppState` and have the download handlers take `State`. Either implement real support-config embedding (MAGIC_MARKER + len + JSON, matching the permanent path) or correct the module docstring to stop claiming it. | M |
|
||
| **C3** | `server/src/api/downloads.rs` | Replace every `Response::builder()…body(…).unwrap()` with a real error path (500 JSON). Reject/percent-encode (RFC 5987 `filename*=`) `Content-Disposition` values; cap input length before sanitizing. Closes an **unauthenticated panic/DoS**. | S |
|
||
| **H1** | `server/src/api/auth.rs` (login, change_password) | Add rate-limit + failure-lockout on the login path, reusing the existing `state.rate_limits` machinery from the enroll path. Key on `(username.lower(), client_ip)`. Return 429 after threshold; record failures after constant-time work. | M |
|
||
| **H2** | `server/src/main.rs` (bootstrap ~198–225) | Stop writing the generated admin **plaintext** to `.admin-credentials`; remove the `info!` log fallback. Prefer operator-supplied initial password via env/one-time flag (error on zero-user start if absent), or print once to stdout with a "copy now, never persisted" warning. _(Server deploys on Linux so 0o600 works — the residual risk is on-disk plaintext + the log path, not the Windows-ACL angle.)_ | S |
|
||
| **H3** | `server/src/api/auth_logout.rs:116` | The `revoke_user_tokens` 501 stub claims partial behavior it doesn't have. Either delete the route until the session-tracking table exists, or implement the minimal real version and make the comment/error match reality. | S |
|
||
| **H5** | `server/src/api/users.rs` + `auth` extractor | Enforce the **self-role-demotion / self-disable guard server-side** (currently only self-_delete_ is blocked; the lockout guard is client-only in `EditUserModal.tsx`). Mirror the last-admin/self guards already shipped in GuruRMM SPEC-027. | S |
|
||
|
||
**Validation:** unauthenticated download fuzz (quotes/control chars in `company`/`site`), login brute-force returns 429, fresh-server bootstrap leaves no secret on disk/in logs, self-demote returns 4xx server-side.
|
||
|
||
---
|
||
|
||
## P1 — WebSocket auth handshake redesign (proposed SPEC-020) — the big one
|
||
**C1 — the top convergent finding.** Secrets/tokens currently travel in WS URL query strings on
|
||
both planes (agent: `api_key`/`support_code`/`machine_uid`; viewer: the session-scoped control token).
|
||
|
||
This is partly **by current design** — `guru-connect/CLAUDE.md` Security Rules mandate
|
||
"Viewer WebSocket: JWT token required in `token` query parameter." Both external models reject
|
||
that. **First action: a design decision to change that spec rule.** Server-side the viewer token
|
||
is already hardened (minted, session-bound, signature/purpose/`session_id` checked, blacklist on
|
||
the WS plane) — only the transport channel is the problem.
|
||
|
||
Workstream (L, cross-component — run a three-way *design* review before building):
|
||
1. `proto/guruconnect.proto`: add an initial `AuthHandshake` message (agent creds / viewer token + binding).
|
||
2. `server/src/relay/mod.rs`: accept the WS upgrade unauthenticated, then require the auth frame as the first message; fail-closed on timeout/missing. Keep all existing binding/blacklist/consent checks.
|
||
3. `agent/src/transport/websocket.rs` (+ call sites: `session/mod.rs`, `viewer/transport.rs`, `install.rs`): connect, then send creds in the first protobuf frame. Remove secrets from the URL.
|
||
4. `dashboard/src/features/sessions/JoinSessionModal.tsx` (`buildViewerUrl`) + `api/sessions.ts`: send the viewer token via the handshake; stop surfacing the full token-bearing URL as a copyable field.
|
||
5. Update `guru-connect/CLAUDE.md` security rules to match.
|
||
|
||
**Interim mitigation (until P1 ships):** make minted viewer tokens single-use-on-first-attach and shorten TTL; ensure the relay/NPM proxy never logs full request URLs.
|
||
|
||
---
|
||
|
||
## P2 — Update & embedded-config integrity (proposed SPEC-021)
|
||
| Finding | File(s) | Action | Effort |
|
||
|---|---|---|---|
|
||
| **C5** | `agent/src/update.rs` | SHA-256 over the same channel is not tamper defense (the code's own TODO says so). Embed an Ed25519 public key; sign the update manifest + binary at build time; **verify signature before `install_update`**. Lock `dev_insecure_tls` to debug builds and consider removing the env escape hatch. | M |
|
||
| **MED** | `agent/src/config.rs` (`read_embedded_config`) | Same crypto primitive: sign the appended `GURUCONFIG` blob (Ed25519) and verify before trusting `server_url`/`enrollment_key`. Add a strict max length, validate the marker sits after the PE end, and allowlist `server_url` scheme/host. Prevents an attacker blob from repointing the agent to a hostile relay. | M |
|
||
|
||
Bundle these because they share the signing key infrastructure and the build-pipeline signing step. The Pluto build server / Gitea webhook pipeline will need a signing key (store in SOPS vault).
|
||
|
||
---
|
||
|
||
## P3 — Agent runtime correctness (proposed SPEC-022)
|
||
| Finding | File(s) | Action | Effort |
|
||
|---|---|---|---|
|
||
| **C4** | `agent/src/transport/websocket.rs` | Remove `block_in_place(|| Handle::block_on(...))` from the main session loop and viewer recv. Use a dedicated recv task forwarding over `mpsc`, or async `timeout` directly on the locked stream. Fix the racy `connected` flag. | M |
|
||
| **H7** | `agent/src/session/mod.rs` | Drive attended-consent off the main loop (spawn a task that owns only the response send) so heartbeats/status/stop-signals keep flowing during the ~60s think-time. | M |
|
||
| **H8** | `agent/src/credential_store.rs:~309/345` | Call `icacls` by absolute path (`C:\Windows\System32\icacls.exe`) or use Win32 `SetNamedSecurityInfoW` directly; verify the resulting ACL actually denies non-admins (or log raw `icacls` output on failure) instead of silently leaving a weaker store. | S |
|
||
| **MED** | `agent/src/encoder/h264.rs`, `decoder.rs`, `capture/{dxgi,gdi}.rs`, `session/mod.rs` | Wrap the per-frame `capture()`/`encode()` in the same `catch_unwind` + "mark capturer bad, fall back/idle" recovery used at init; make COM `ReleaseFrame`/`DeleteObject` strict (scopeguard, exactly-once per Acquire); after N consecutive H.264 errors force renegotiation/hard-switch to raw instead of per-frame silent drop; remove dead `last_frame`. | L |
|
||
|
||
---
|
||
|
||
## P4 — Auth/session lifetime & dashboard hardening (proposed SPEC-023)
|
||
| Finding | File(s) | Action | Effort |
|
||
|---|---|---|---|
|
||
| **H4** | `server/src/auth/token_blacklist.rs:85` | Store `(token, exp)` and make `cleanup_expired` a pure time comparison instead of re-verifying every JWT signature. Capture `exp` at revoke time (cheap unsigned parse). | S |
|
||
| **H6** | `dashboard/src/auth/AuthProvider.tsx`, `api/client.ts` | Parse/respect token `exp`; add proactive refresh-or-logout and an idle-activity timeout. Longer-term: move the JWT to an `httpOnly; Secure; SameSite=Strict` cookie managed by the server (needs a server-side cookie/refresh endpoint — coordinate with P1). | M |
|
||
| **MED** | `dashboard/src/auth/AuthProvider.tsx` (`restore`) | Only `clearSession()` on an explicit `401`; on network (status 0) / 5xx keep the token and show an offline/retry state. Stops flaky-connection logout loops. | S |
|
||
| **MED** | `server/src/api/users.rs` + `dashboard/src/features/users/hooks.ts` | Add a transactional `PATCH /api/users/:id` that updates core fields + permissions atomically; switch the dashboard off the two-call `updateUser`→`setUserPermissions` sequence. | M |
|
||
|
||
---
|
||
|
||
## P5 — Hygiene & maintainability (rolling, no dedicated spec)
|
||
- **MED** Password policy: complexity + upper bound + breach-list (`zxcvbn`); raise min to 12. (`auth.rs`, `users.rs`, `enroll.rs`)
|
||
- **MED** Input validation: cap + control-char-strip `machine_uid`/`hostname`/`site_code`/password on the unauthenticated enroll path before DB / rate-limit keys. (`enroll.rs`)
|
||
- **MED** Audit correctness: `CONNECTION_REJECTED_*` events should insert `session_id = NULL` (like enrollment/removal), not a random UUID. Add `log_connection_rejected` or make `log_event` take `Option<Uuid>`. (`relay/mod.rs`, `db/events.rs`) — _also resolves the dangling-row issue noted in the earlier in-product audit assessment._
|
||
- **MED** Generate dashboard TS types from the Rust API (`ts-rs`/`specta`) to kill type drift; interim: a snapshot-diff test. (`dashboard/src/api/types.ts`)
|
||
- **MED** Wire the declared `VIEWER_INPUT_EVENTS_PER_SEC = 200` viewer-input rate limit (token bucket / coalesce latest mouse-move) or update the comment to match the backpressure-only reality. (`relay/mod.rs`)
|
||
- **LOW** Remove panic sites on response/header construction (`prometheus_metrics` locks/encode, `security_headers` `from_static().unwrap()`). (`main.rs`, `middleware/security_headers.rs`)
|
||
- **LOW** Document (or persist) that in-memory rate-limit/lockout state resets on restart; consider a bounded TTL table in Postgres for enroll/code-validate. (`rate_limit.rs`)
|
||
- **LOW** Redundant post-decode `exp` checks; consider a small configurable leeway. (`auth/jwt.rs`)
|
||
- **LOW** `set_client_access` should verify client UUIDs exist before storing. (`users.rs`)
|
||
- **LOW** Dashboard polling: pause `refetchInterval` when `document.hidden`, add jitter/backoff. (`features/*/hooks.ts`)
|
||
- **LOW** Remove `stubs.ts` from the barrel export; audit + delete or feature-gate the large `#[allow(dead_code)]` "native-remote-control"/Phase-4 surface as it's activated. (dashboard + server/agent)
|
||
- **LOW** Standardize on the `ApiError` envelope server-wide; centralize role/permission allow-lists. (`api/*`)
|
||
- **LOW** Dashboard `rowKey` on `m.id` while everything else keys on `agent_id` — standardize on `agent_id`. (`MachinesPage.tsx`)
|
||
- **LOW** Treat unknown/plain-text server errors as opaque in the dashboard toasts (don't dump raw bodies). (`api/client.ts`)
|
||
- **LOW** Drop the deprecated `document.execCommand("copy")` clipboard fallback. (`useClipboard.ts`)
|
||
|
||
---
|
||
|
||
## Suggested sequencing
|
||
1. **P0** immediately (one hardening spec, ~2–3 days total, all small).
|
||
2. Kick off the **P1** design review in parallel (it's the highest-impact and longest-lead).
|
||
3. **P2** and **P3** next (agent-side; can overlap once P1 design is settled, since P1 also touches the agent transport).
|
||
4. **P4** (some of it — httpOnly cookie — depends on P1's server auth changes).
|
||
5. **P5** folded into whichever spec touches the relevant file, or a periodic cleanup pass.
|
||
|
||
## Tracking
|
||
Create coord todos (project_key `guruconnect`) for P0–P4 as parent items, with the individual
|
||
findings as sub-tasks, plus Gitea issues on `azcomputerguru/guru-connect` for the CRITICAL/HIGH
|
||
items so they're referenced from commits (`Fixes #N`).
|