Skip to content

fix: resolve 14 review issues across the service control plane - #14

Closed
woodsonl wants to merge 5 commits into
NVIDIA:developfrom
woodsonl:fix/review-criticals-security-hardening
Closed

woodsonl wants to merge 5 commits into
NVIDIA:developfrom
woodsonl:fix/review-criticals-security-hardening

Conversation

@woodsonl

@woodsonl woodsonl commented Sep 4, 2026 •

Copy link
Copy Markdown

Description

PAIR routes independent inference requests across local nodes. This PR resolves
14 issues found in a whole-repo review plus later adversarial and regression
rounds across the service control plane, and adds coverage at each boundary the
fixes touch.

Rebased onto develop after the "One engine proxy binary" change (#87 era):
the two-proxy fixes are ported onto the unified nvpair-proxy, and the
per-engine CORS intersection is kept alongside the new request-entry allowlist
gate.

The 14 issues, by commit

  1. Stdio JSON-RPC read loops did not distinguish recoverable from terminal read errors (broker + all 12 service modules): one malformed frame could spin or terminate the control-plane connection. DecodeError frames now skip-and-continue; EOF stays clean; terminal scanner/transport errors stop instead of spinning.
  2. One slow worker relay head-of-line blocked the whole control plane — broker relays ran inline on the read loop. Now a bounded 4-goroutine dispatch pool.
  3. A stalled worker subscriber stalled every other subscriber — relay delivery was synchronous. Now per-subscriber pump goroutines with a coalescing non-blocking Deliver.
  4. Pairing PIN comparison now uses PBKDF2-HMAC-SHA256 stretching (50k iterations, per-invite salt inside the EAP-MAC-covered ServerInfo).
  5. Pairing completion is now capped at 5 attempts with invite teardown.
  6. Terminal pairing signals (cancel/decline/fail/expire) are now authenticated with an HMAC over the session's ephemeral Key Exchange secret.
  7. Errors ingest now requires the envelope nodeId to match the mTLS-authenticated caller UUID, with a 1 MiB body cap.
  8. Proxies now enforce a deny-by-default origin allowlist (NVPAIR_PROXY_ALLOWED_ORIGINS) and no longer echo arbitrary headers. Ported onto the unified proxy as a request-entry gate layered over the per-engine CORS intersection.
  9. Inference requests now have a 32 MiB body cap, returning 413 over the limit. Ported onto nvpair-proxy.
  10. Engine stop now escalates from graceful stop to SIGKILL/pgid after the manifest grace period.
  11. Manifest installs now fail closed on unpinned downloads unless NVPAIR_ALLOW_UNPINNED_DOWNLOADS=1.
  12. Workload checkpoint re-marks the store dirty on write failure so a failed snapshot is not treated as clean.
  13. safeHandle no longer authorizes arbitrary renderer origins when ELECTRON_RENDERER_URL is empty.
  14. Cluster-manager session handling no longer double-unlocks sess.mu on the over-completion-attempts branch and no longer leaves it locked on the cancel path, so unauthenticated POSTs can no longer panic or deadlock the manager.

Follow-ups close the remaining gaps the rounds surfaced: lookalike-host/userinfo
bypass of the dev-URL sender check, the same decode-error confusion in the
broker's consumer side, and the relay Deliver-vs-Unsubscribe race.

Intentionally in scope

Everything above, plus the regression tests listed under Validation. Version
bumps for every component whose compiled output changes are declared in the
release-intent block below (not hand-edited into services/versions.json).

Intentionally out of scope: the two-proxy port-override test harness
(NVPAIR_SERVICE_*_PORT) that upstream's unified proxy superseded. It was
removed rather than carried forward, so no compatibility shim remains.

Validation environment

  • Linux, Go 1.25.0, Node.js 24.21.0
  • go build ./... in all 15 services modules — pass
  • go test -race -count=1 ./... in every services module — pass
  • Cross-process suite services/tests (go test -count=1 ./...) — pass, 0 skips
  • Desktop: npm run typecheck, npm run lint, npm run test:unit (238 tests), npm run dead-code:check, npm run service-contracts:check — all pass
  • node scripts/spdx-headers.mjs — 1021 checked, 0 missing

New regression coverage at each fixed boundary:

  • desktop/tests/modular/safe-handle-sender.test.ts — sender authorization: dev-URL exact and path-suffix match, file://, unset and empty env, lookalike host, userinfo spoofing, missing window (red/green against the fix)
  • services/nvpair-cluster-manager/pairing_signal_gate_test.go — 401 gate for cancel/decline/expire with wrong-phase and wrong-invite tags MACed under the live session key, state survival; 429 rate limit including failed/incorrect-pin attempts and teardown; cancel without a signal key
  • services/nvpair-ui-broker/terminal_read_test.go — read-loop contract: transport error terminal, EOF clean, decode error recoverable
  • services/nvpair-ui-broker/relay/relay_test.go — trigger coalescing delivers latest state once; post-unsubscribe Deliver is silent
  • services/nvpair-proxy/ingress_test.go — request-entry allowlist gate: unlisted origin 403 origin-not-allowed, allowlisted origin passes to routing, Origin-less caller unaffected
  • services/nvpair-proxy/body_limit_test.go — body cap (over/at limit), model parse, end-to-end 413
  • services/shared/cors/allowlist_test.go — exact-origin matching, empty-allowlist denial, 403 shape
  • services/eap-noob/ephemeral_key_test.go — ephemeral key lifecycle: pre-exchange errors, server/peer agreement, copy semantics

Compatibility notes

  • No JSON-RPC method or payload shapes change; the fixes tighten enforcement of existing contracts. Paired-cluster and pairing flows are unaffected for legitimate participants.
  • Legitimate pairing completion requests now always consume an attempt; a client that retried with empty bodies to probe state will now hit the limit — intended.
  • The safeHandle dev-URL change only affects development-mode renderer IPC (ELECTRON_RENDERER_URL); packaged builds authorize via file:// and are unchanged.
  • The digest pins make installs fail closed if a registry serves artifacts that do not match the reviewed manifest — intended; updating an engine version requires updating the pin, which is the review point.
  • The unified nvpair-proxy keeps the per-engine CORS intersection and adds a request-entry allowlist gate ahead of it: browser requests carrying an Origin are admitted only from NVPAIR_PROXY_ALLOWED_ORIGINS; non-browser callers are unaffected.

Documentation changes

Service READMEs updated to match the new behavior:

  • services/nvpair-proxy/README.md: records the request-entry NVPAIR_PROXY_ALLOWED_ORIGINS allowlist gate over the per-engine intersection policy, and the 32 MiB request-body cap with the 413 path
  • services/nvpair-engine-manager/README.md: install "verify-if-pinned with a loud warning" → fail closed unless NVPAIR_ALLOW_UNPINNED_DOWNLOADS=1; stop "no timeout, no SIGKILL escalation" → manifest grace then forced kill
  • services/nvpair-cluster-manager/README.md note: records the PIN stretching, authenticated terminal signals, and completion-attempt cap it describes

Related issue

None — found and fixed through whole-repo review.

Release intent

Changelog title

Harden the service control plane: authenticated pairing, bounded queues, deny-by-default proxy origins

Changelog body

  • Pairing is stricter: PINs are stretched, completion attempts are capped, and terminal pairing signals are authenticated over the session key.
  • The broker stays responsive under a slow or stalled worker: worker replies are dispatched through a bounded pool and relay delivery no longer blocks other subscribers.
  • Browser access to the local inference proxies is deny-by-default. List the origins you trust in NVPAIR_PROXY_ALLOWED_ORIGINS; unlisted origins get 403. Requests without an Origin (the app, CLI, health probes) are unaffected.
  • Inference requests larger than 32 MiB are refused with 413 instead of being buffered.
  • Engine installs fail closed when a manifest has no pinned digest, unless NVPAIR_ALLOW_UNPINNED_DOWNLOADS=1. Engine stop escalates to a forced kill after the manifest grace period.
  • A malformed control-plane frame no longer spins or tears down a worker connection.

Bumps

  • services: patch
  • nvpair-cluster-manager: patch
  • nvpair-engine-manager: patch
  • nvpair-errors: patch
  • nvpair-job-scheduler: patch
  • nvpair-manual-nodes: patch
  • nvpair-node-info: none
  • nvpair-node-scanner: patch
  • nvpair-node-settings: patch
  • nvpair-proxy: patch
  • nvpair-tui: patch
  • nvpair-ui-broker: patch
  • nvpair-workload-manager: patch

Services (Go):
- stdio JSON-RPC read loops (broker + 12 modules): recoverable DecodeError
  frames now skip-and-continue; EOF stays clean; terminal scanner/transport
  errors stop instead of spinning (errors.As(nil) is false, so the success
  path must continue explicitly)
- broker: bounded 4-goroutine dispatch pool so one slow worker relay cannot
  head-of-line block the control plane; producer goroutine classifies
  terminal reads (errTerminalRead)
- relay: per-subscriber delivery pump goroutines (coalescing kick channel);
  Deliver is non-blocking so a stalled worker cannot stall the scanner read
  pump or other subscribers
- cluster-manager: PBKDF2-HMAC-SHA256 PIN stretching (50k iterations,
  per-invite salt inside the EAP-MAC-covered ServerInfo); online Completion
  attempts capped at 5 with invite teardown; terminal pairing signals
  (cancel/decline/fail/expire) authenticated with an HMAC over the session's
  ephemeral Key Exchange secret
- errors ingest: envelope nodeId must match the mTLS-authenticated caller
  UUID; 1 MiB body cap on the ingest endpoint
- proxies: deny-by-default origin allowlist via
  NVPAIR_PROXY_ALLOWED_ORIGINS, 32 MiB inference body cap (413), CORS
  grants only for allowlisted origins, no arbitrary header echo
- engine-manager: graceful stop then SIGKILL/pgid escalation after the
  manifest stop grace; unpinned manifest downloads fail closed unless
  NVPAIR_ALLOW_UNPINNED_DOWNLOADS=1
- broker: NVPAIR_SERVICE_*_PORT env overrides threaded as --port to every
  supervised worker so tests never skip on fixed-port collisions
- workloadstore: Checkpoint re-marks dirty when the snapshot write fails
- Makefile: test-services runs with -race
- versions.json: bump all 13 changed components

Desktop:
- ipc/safe-handle: empty ELECTRON_RENDERER_URL no longer authorizes
  arbitrary origins
- dead-code-omissions: drop stale @electron-toolkit/preload entry

Signed-off-by: woodsonl <65194841+woodsonl@users.noreply.github.com>
- httpserver.go: remove explicit inviteMu/sess.mu unlocks in the
  over-completion-attempts branch; the deferred unlocks at the top of
  handlePairingCompletion unlock them again at return (double unlock of an
  unlocked mutex). The teardown helpers take only
  memMu/sessMu, so holding both locks through teardown is safe.
- cancel.go: unlock sess.mu on the no-signal-key path. The fix that
  derived the signal key under the already-held lock left the early
  return without releasing it, deadlocking any goroutine that had fetched
  the session pointer (joiner Completion POST, respond).
  The terminal write and session delete now stay under sess.mu like the
  success path, preserving the serialization against Completion.
- manifests: pin sha256 for every ollama.json and lmstudio.json fetch.
  The fail-closed default in install.go made every default engine install
  fail because no shipped manifest carried a digest.

Signed-off-by: woodsonl <65194841+woodsonl@users.noreply.github.com>
Signed-off-by: woodsonl <65194841+woodsonl@users.noreply.github.com>
@woodsonl
woodsonl force-pushed the fix/review-criticals-security-hardening branch from 3f600b2 to 820bcc6 Compare September 4, 2026 18:47
Production fixes:
- safe-handle.ts: require the dev URL to match exactly or with a trailing
  path separator so only the configured development origin and its paths
  pass the IPC sender check
- ui-broker readLoop: skip recoverable per-frame decode errors instead of
  tearing the connection down; extract recoverableDecode so the producer
  and consumer predicates cannot drift
- relay Directory.pump: prioritize done over a pending kick so a Deliver
  racing Unsubscribe cannot Send against a consumer that's gone

Tests:
- desktop: safeHandle sender authorization (dev URL exact/slash, file://,
  unset/empty env, lookalike host, userinfo, missing window)
- ui-broker: readLoop terminal/EOF/decode-error contract, relay pump
  coalescing and post-unsubscribe silence, resolveServicePorts env
  overrides and invalid-value fallback
- proxies (ollama + lmstudio): loopback cross-origin CORS gate and
  request-body limit / 413 path
- cluster-manager: 401 signal gate (wrong phase/invite tags MACed with the
  live session key) and 429 completion rate limit
- eap-noob: EphemeralKey lifecycle (pre-exchange errors, key agreement,
  copy semantics)

Bump nvpair-ui-broker to 0.40.4.

Signed-off-by: woodsonl <65194841+woodsonl@users.noreply.github.com>
@woodsonl woodsonl changed the title fix: close 14 security-critical findings plus review regressions fix: enforce IPC sender authorization, pairing signal auth, and relay/read-loop concurrency contracts Sep 4, 2026
@woodsonl woodsonl changed the title fix: enforce IPC sender authorization, pairing signal auth, and relay/read-loop concurrency contracts fix: close 14 critical security and reliability findings from whole-repo review Sep 4, 2026
CONTRIBUTING requires documentation updated in the same PR for behavior
and networking changes. Three sections described the previous
behavior:

- ollama-proxy README (referenced by lmstudio-proxy's CORS section):
  documented the permissive wildcard CORS grant; the policy is now
  deny-by-default via NVPAIR_PROXY_ALLOWED_ORIGINS with a static
  preflight grant and the 403 origin-not-allowed gate
- engine-manager README: install was 'verify-if-pinned with a loud
  warning' — now fail closed unless NVPAIR_ALLOW_UNPINNED_DOWNLOADS=1;
  stop was 'no timeout, no SIGKILL escalation' — now grace-then-forced-kill
- cluster-manager README note: records the PIN stretching, authenticated
  terminal signals, and completion-attempt cap

Signed-off-by: woodsonl <65194841+woodsonl@users.noreply.github.com>
@kjlubick

Copy link
Copy Markdown
Collaborator

Read SECURITY.md before deploying it on an untrusted or shared network. Do not report vulnerabilities in a public issue.

Please note the above from the README. In addition, smaller PRs are easier to evaluate than large ones.

@woodsonl woodsonl changed the title fix: close 14 critical security and reliability findings from whole-repo review fix: resolve 14 review issues across the service control plane Sep 12, 2026
@woodsonl
woodsonl force-pushed the fix/review-criticals-security-hardening branch from e16bf21 to 2b0b375 Compare September 12, 2026 21:25
@woodsonl woodsonl closed this Sep 12, 2026
@woodsonl
woodsonl deleted the fix/review-criticals-security-hardening branch September 12, 2026 21:25
@woodsonl
woodsonl restored the fix/review-criticals-security-hardening branch September 12, 2026 21:29
@woodsonl woodsonl reopened this Sep 12, 2026
@Noah-Tervalon-Nvidia

Copy link
Copy Markdown
Collaborator

Heads up — develop has just taken a change that merges the Ollama and LM Studio
proxies into a single nvpair-proxy process, hosting one facade per enabled
engine. services/ollama-proxy/ and services/lmstudio-proxy/ are gone, and the
per-engine ports UI has been replaced by an engine settings panel.

This PR edits a number of files across both areas, so it needs more than a
rebase — the structure it was written against has moved, not just the file paths.
The client-facing ollama-proxy: and lmstudio-proxy: namespaces are unchanged.

Worth agreeing the new approach before reworking it. Happy to talk through where
this fits in the new structure — and sorry for the disruption.

@Noah-Tervalon-Nvidia
Noah-Tervalon-Nvidia changed the base branch from main to develop September 21, 2026 21:57
@kjlubick

Copy link
Copy Markdown
Collaborator

Please do not file security fixes like this (in the public). Follow the steps on https://github.com/NVIDIA/Personal-AI-Router/blob/develop/SECURITY.md

I did review the changes here and none seem actionable given our threat model and the changes that have recently landed on the develop branch

@kjlubick kjlubick closed this Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants