From d5c6e518e94166a206807f7271d25f12d415b5e3 Mon Sep 17 00:00:00 2001 From: woodsonl <65194841+woodsonl@users.noreply.github.com> Date: Fri, 4 Sep 2026 13:35:28 +0000 Subject: [PATCH 1/5] fix: resolve 14 review issues across the service control plane 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> --- Makefile | 4 +- desktop/dead-code-omissions.json | 5 +- desktop/package-lock.json | 111 ----------- desktop/src/electron/ipc/safe-handle.ts | 3 +- services/eap-noob/export.go | 16 ++ services/eap-noob/helpers.go | 9 - services/eap-noob/peer.go | 5 +- services/eap-noob/server.go | 5 +- services/lmstudio-proxy/codec.go | 7 +- services/lmstudio-proxy/e2e_test.go | 4 +- services/lmstudio-proxy/failover_test.go | 93 +++++---- services/lmstudio-proxy/ingress.go | 26 ++- services/lmstudio-proxy/ingress_test.go | 17 +- services/lmstudio-proxy/proxy.go | 66 +++++-- services/nvpair-cluster-manager/cancel.go | 25 ++- .../nvpair-cluster-manager/cm_unit_test.go | 35 +++- services/nvpair-cluster-manager/codec.go | 7 +- services/nvpair-cluster-manager/httpserver.go | 48 ++++- services/nvpair-cluster-manager/invite.go | 78 ++++++-- .../nvpair-cluster-manager/invite_expiry.go | 3 +- services/nvpair-cluster-manager/manager.go | 12 +- services/nvpair-cluster-manager/pairing.go | 154 ++++++++++++++- services/nvpair-cluster-manager/respond.go | 43 ++++- services/nvpair-engine-manager/install.go | 24 ++- services/nvpair-engine-manager/lifecycle.go | 4 +- services/nvpair-engine-manager/proc.go | 36 ++-- services/nvpair-engine-manager/proc_unix.go | 21 ++- .../nvpair-engine-manager/proc_windows.go | 24 ++- .../nvpair-engine-manager/remediation_test.go | 17 +- services/nvpair-errors/codec.go | 7 +- services/nvpair-errors/httpserver.go | 35 ++-- services/nvpair-errors/manager.go | 11 +- services/nvpair-errors/peersync_test.go | 38 +++- services/nvpair-job-scheduler/codec.go | 7 +- services/nvpair-job-scheduler/manager.go | 11 +- services/nvpair-manual-nodes/codec.go | 7 +- services/nvpair-manual-nodes/manager.go | 11 +- services/nvpair-node-scanner/codec.go | 7 +- services/nvpair-node-scanner/scanner.go | 11 +- services/nvpair-node-settings/codec.go | 7 +- services/nvpair-node-settings/manager.go | 10 +- services/nvpair-tui/rpc/client.go | 11 +- services/nvpair-tui/rpc/codec.go | 16 +- services/nvpair-ui-broker/broker.go | 125 ++++++++++--- services/nvpair-ui-broker/clustermanager.go | 4 +- services/nvpair-ui-broker/codec.go | 9 +- services/nvpair-ui-broker/lmstudioport.go | 5 + services/nvpair-ui-broker/ollamahost.go | 71 ++++++- services/nvpair-ui-broker/ollamahost_test.go | 2 +- services/nvpair-ui-broker/proxyport.go | 7 + services/nvpair-ui-broker/relay/relay.go | 86 ++++++--- services/nvpair-ui-broker/relay/relay_test.go | 50 +++-- services/nvpair-ui-broker/relaysub.go | 8 +- .../workloadstore/persistence.go | 8 +- services/nvpair-workload-manager/codec.go | 7 +- services/nvpair-workload-manager/manager.go | 12 +- services/ollama-proxy/codec.go | 7 +- services/ollama-proxy/failover_test.go | 90 ++++++--- services/ollama-proxy/ingress.go | 26 ++- services/ollama-proxy/ingress_test.go | 17 +- services/ollama-proxy/proxy.go | 66 +++++-- services/shared/cors/cors.go | 138 ++++++++++---- services/shared/cors/cors_test.go | 177 +++++++++++------- services/tests/broker_management_test.go | 24 +-- services/tests/broker_supervision_test.go | 66 +++++-- services/tests/cluster_data_plane_test.go | 74 +++++--- services/tests/cluster_restore_test.go | 20 +- services/tests/main_test.go | 2 - services/tests/model_routing_interop_test.go | 6 +- services/tests/scheduler_interop_test.go | 6 +- .../tests/workload_identity_interop_test.go | 15 +- services/tests/workload_interop_test.go | 40 +++- services/versions.json | 24 +-- 73 files changed, 1609 insertions(+), 674 deletions(-) diff --git a/Makefile b/Makefile index 496354b3..5a5b56fb 100644 --- a/Makefile +++ b/Makefile @@ -120,10 +120,10 @@ test-desktop: $(NODE_MODULES) ## Run the desktop unit tests # services/tests builds the component binaries it drives into a temporary # directory, so the cross-process suite does not need build-services first. -test-services: ## Run go test in every services module +test-services: ## Run go test (with -race) in every services module @for module in $(GO_MODULES); do \ printf '\ngo test: %s\n' "$$module"; \ - (cd "$$module" && go test ./...) || exit 1; \ + (cd "$$module" && go test -race ./...) || exit 1; \ done build-binaries: $(NODE_MODULES) ## Compile the Go service binaries into desktop/cli-bin diff --git a/desktop/dead-code-omissions.json b/desktop/dead-code-omissions.json index 8b3adf70..7955c535 100644 --- a/desktop/dead-code-omissions.json +++ b/desktop/dead-code-omissions.json @@ -2,11 +2,10 @@ "$comment": "Known false positives for the dead-code analyzer. Add entries after reviewing the report.", "$reasons": { "@tailwindcss/postcss": "Loaded by the PostCSS/Vite config, never imported from source.", - "tailwindcss": "Loaded by the PostCSS/Vite config, never imported from source.", - "@electron-toolkit/preload": "Imported for the ElectronAPI type by src/preload/index.d.ts. knip.json excludes ambient .d.ts files from analysis, so the only reference is invisible to it — uninstalling breaks the preload type declarations." + "tailwindcss": "Loaded by the PostCSS/Vite config, never imported from source." }, "files": [], "exports": [], "types": [], - "dependencies": ["@tailwindcss/postcss", "tailwindcss", "@electron-toolkit/preload"] + "dependencies": ["@tailwindcss/postcss", "tailwindcss"] } diff --git a/desktop/package-lock.json b/desktop/package-lock.json index d6191db3..cad31a4c 100644 --- a/desktop/package-lock.json +++ b/desktop/package-lock.json @@ -1647,9 +1647,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1667,9 +1664,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -1687,9 +1681,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1707,9 +1698,6 @@ "riscv64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1727,9 +1715,6 @@ "riscv64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -1747,9 +1732,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1767,9 +1749,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1787,9 +1766,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -1996,9 +1972,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -2013,9 +1986,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -2030,9 +2000,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -2047,9 +2014,6 @@ "riscv64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -2064,9 +2028,6 @@ "riscv64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -2081,9 +2042,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -2098,9 +2056,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -2115,9 +2070,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -3742,9 +3694,6 @@ "arm" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -3759,9 +3708,6 @@ "arm" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -3776,9 +3722,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -3793,9 +3736,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -3810,9 +3750,6 @@ "loong64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -3827,9 +3764,6 @@ "loong64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -3844,9 +3778,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -3861,9 +3792,6 @@ "ppc64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -3878,9 +3806,6 @@ "riscv64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -3895,9 +3820,6 @@ "riscv64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -3912,9 +3834,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -3929,9 +3848,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -3946,9 +3862,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -4270,9 +4183,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -4290,9 +4200,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -4310,9 +4217,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -4330,9 +4234,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -9529,9 +9430,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -9553,9 +9451,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -9577,9 +9472,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -9601,9 +9493,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [ diff --git a/desktop/src/electron/ipc/safe-handle.ts b/desktop/src/electron/ipc/safe-handle.ts index b96d72b9..1d15cbd2 100644 --- a/desktop/src/electron/ipc/safe-handle.ts +++ b/desktop/src/electron/ipc/safe-handle.ts @@ -16,7 +16,8 @@ function isKnownSender(event: IpcMainInvokeEvent): boolean { const url = event.sender.getURL() if (url.startsWith('file://')) return true - if (url.startsWith(process.env.ELECTRON_RENDERER_URL ?? '')) return true + const devUrl = process.env.ELECTRON_RENDERER_URL + if (devUrl !== undefined && devUrl !== '' && url.startsWith(devUrl)) return true return false } diff --git a/services/eap-noob/export.go b/services/eap-noob/export.go index 7b670820..b21467bb 100644 --- a/services/eap-noob/export.go +++ b/services/eap-noob/export.go @@ -14,6 +14,22 @@ import ( // successful Completion Exchange has established the association key Kz. var ErrNotRegistered = errors.New("eapnoob: no registered association") +// EphemeralKey returns the raw ECDH shared secret (z) established by the +// Key Exchange of the in-flight method execution, before any PIN is involved. +// Both sides hold the identical value once the Initial Exchange completes; +// a passive observer of the plaintext transport cannot compute it. Callers +// use it to authenticate transport-level session signals (cancel/decline/ +// expire) that arrive outside the EAP-NOOB message stream. It returns an +// error when no Key Exchange has run yet. +// +// Promoted to Server.EphemeralKey and Peer.EphemeralKey via embedding. +func (ms *methodState) EphemeralKey() ([]byte, error) { + if len(ms.z) == 0 { + return nil, errors.New("eapnoob: no key exchange performed") + } + return append([]byte(nil), ms.z...), nil +} + // Export derives a shared secret of arbitrary length from the established // association. Both the peer and the server, having reached the Registered // state with the same Kz, derive identical bytes for the same label and diff --git a/services/eap-noob/helpers.go b/services/eap-noob/helpers.go index 5edf238a..2a8fda5b 100644 --- a/services/eap-noob/helpers.go +++ b/services/eap-noob/helpers.go @@ -12,15 +12,6 @@ import ( func intp(i int) *int { return &i } -func containsInt(s []int, v int) bool { - for _, x := range s { - if x == v { - return true - } - } - return false -} - // newPeerId generates a fresh, unguessable 16-byte identifier base64url-encoded // to a 22-character string (RFC 9140, Section 3.3.1). func newPeerId() (string, error) { diff --git a/services/eap-noob/peer.go b/services/eap-noob/peer.go index d1d5042b..0e463895 100644 --- a/services/eap-noob/peer.go +++ b/services/eap-noob/peer.go @@ -8,6 +8,7 @@ import ( "crypto/rand" "encoding/json" "fmt" + "slices" ) // PeerConfig configures the EAP peer role. @@ -406,7 +407,7 @@ func (p *Peer) fail(code int, info string) (Outcome, error) { func bestVersion(serverVers, peerVers []int) (int, bool) { best, ok := 0, false for _, v := range serverVers { - if containsInt(peerVers, v) && v >= best { + if slices.Contains(peerVers, v) && v >= best { best, ok = v, true } } @@ -415,7 +416,7 @@ func bestVersion(serverVers, peerVers []int) (int, bool) { func firstSupported(serverCS, peerCS []int) (int, bool) { for _, c := range serverCS { - if containsInt(peerCS, c) && isSupportedCryptosuite(c) { + if slices.Contains(peerCS, c) && isSupportedCryptosuite(c) { return c, true } } diff --git a/services/eap-noob/server.go b/services/eap-noob/server.go index cd95d35e..1a113e96 100644 --- a/services/eap-noob/server.go +++ b/services/eap-noob/server.go @@ -8,6 +8,7 @@ import ( "crypto/rand" "encoding/json" "fmt" + "slices" ) // ServerConfig configures the EAP server role. @@ -192,14 +193,14 @@ func (s *Server) onNegotiation(wm *wireMessage) (Outcome, error) { if err != nil { return s.fail(ErrInvalidData, "bad Verp") } - if !containsInt(s.cfg.Versions, verp) { + if !slices.Contains(s.cfg.Versions, verp) { return s.fail(ErrUnsupportedVersion, "peer selected unsupported version") } csp, err := rawToInt(wm.Cryptosuitep) if err != nil { return s.fail(ErrInvalidData, "bad Cryptosuitep") } - if !containsInt(s.cfg.Cryptosuites, csp) || !isSupportedCryptosuite(csp) { + if !slices.Contains(s.cfg.Cryptosuites, csp) || !isSupportedCryptosuite(csp) { return s.fail(ErrUnsupportedCryptosuite, "peer selected unsupported cryptosuite") } dirp, err := rawToInt(wm.Dirp) diff --git a/services/lmstudio-proxy/codec.go b/services/lmstudio-proxy/codec.go index 5d6fd177..1691b8cd 100644 --- a/services/lmstudio-proxy/codec.go +++ b/services/lmstudio-proxy/codec.go @@ -10,9 +10,10 @@ package main import "nvpair-shared/jsonrpc" type ( - Message = jsonrpc.Message - RPCError = jsonrpc.RPCError - Codec = jsonrpc.Codec + Message = jsonrpc.Message + RPCError = jsonrpc.RPCError + DecodeError = jsonrpc.DecodeError + Codec = jsonrpc.Codec ) var NewCodec = jsonrpc.NewCodec diff --git a/services/lmstudio-proxy/e2e_test.go b/services/lmstudio-proxy/e2e_test.go index 5412ab7c..a3011fba 100644 --- a/services/lmstudio-proxy/e2e_test.go +++ b/services/lmstudio-proxy/e2e_test.go @@ -205,8 +205,8 @@ func TestE2EFailoverOverRealBinary(t *testing.T) { if resp.StatusCode != http.StatusOK { t.Fatalf("status = %d, want 200 (should fail over from the 503 node)", resp.StatusCode) } - if got := resp.Header.Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want *", got) + if got := resp.Header.Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want no grant for an Origin-less caller", got) } if gotBody != `{"model":"m"}` { t.Errorf("healthy upstream got body %q, want the original request body", gotBody) diff --git a/services/lmstudio-proxy/failover_test.go b/services/lmstudio-proxy/failover_test.go index 02e5361b..079e0f0e 100644 --- a/services/lmstudio-proxy/failover_test.go +++ b/services/lmstudio-proxy/failover_test.go @@ -53,31 +53,32 @@ func nodeForModel(t *testing.T, id, serverURL, model string) Node { return node } -// TestHandlePlain_OptionsPreflight: a CORS preflight is answered locally with -// 204 + permissive headers and never forwarded. +// TestHandlePlain_OptionsPreflight: a preflight from an allowlisted origin is +// answered locally with 204 + the static grant and never forwarded. An +// unlisted origin's preflight falls through to the origin gate (403). func TestHandlePlain_OptionsPreflight(t *testing.T) { + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") p := testProxy(NewDiscovery(), 11434) rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) req.RemoteAddr = "127.0.0.1:40000" + req.Header.Set("Origin", "https://ui.example") req.Header.Set("Access-Control-Request-Headers", "X-Custom-Token") p.handlePlain(rec, req) if rec.Code != http.StatusNoContent { t.Fatalf("status = %d, want 204", rec.Code) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want *", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin", got) } if rec.Header().Get("Access-Control-Allow-Methods") == "" { t.Errorf("missing Access-Control-Allow-Methods") } - if got := rec.Header().Get("Access-Control-Expose-Headers"); got != "*" { - t.Errorf("Access-Control-Expose-Headers = %q, want *", got) - } - // The browser's requested headers are echoed so an arbitrary header clears preflight. - if got := rec.Header().Get("Access-Control-Allow-Headers"); got != "X-Custom-Token" { - t.Errorf("Access-Control-Allow-Headers = %q, want echoed X-Custom-Token", got) + // Arbitrary request headers are no longer echoed: the static grant is + // deny-by-default, so X-Custom-Token must not clear preflight. + if got := rec.Header().Get("Access-Control-Allow-Headers"); got != "Content-Type, Authorization" { + t.Errorf("Access-Control-Allow-Headers = %q, want the static grant", got) } } @@ -85,6 +86,7 @@ func TestHandlePlain_OptionsPreflight(t *testing.T) { // exact origin into credentialed CORS, its preflight policy reaches the browser // instead of being replaced by the proxy's uncredentialed wildcard fallback. func TestHandlePlain_EngineCredentialedPreflightPreserved(t *testing.T) { + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://app.example") preflightSeen := make(chan struct{}, 1) engine := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodOptions { @@ -144,7 +146,10 @@ func TestHandleHTTP_EngineCORSPolicyPreserved(t *testing.T) { p := testProxy(disc, 11434) rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`))) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + p.handleHTTP(rec, req) if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://app.example" { t.Errorf("Access-Control-Allow-Origin = %q, want the engine's own origin", got) @@ -172,13 +177,16 @@ func TestHandleHTTP_EngineCredentialsWithoutOriginDropped(t *testing.T) { p := testProxy(disc, 11434) rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`))) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + p.handleHTTP(rec, req) - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want the proxy's wildcard", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin", got) } if got := rec.Header().Get("Access-Control-Allow-Credentials"); got != "" { - t.Errorf("Access-Control-Allow-Credentials = %q, want cleared alongside the wildcard origin", got) + t.Errorf("Access-Control-Allow-Credentials = %q, want cleared (the engine declared no origin policy to preserve)", got) } } @@ -198,8 +206,11 @@ func TestHandleHTTP_HappyPathSingleNode(t *testing.T) { disc.AddManual(nodeForModel(t, "good", good.URL, "llama")) p := testProxy(disc, 11434) + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`))) + p.handleHTTP(rec, req) if rec.Code != http.StatusOK { t.Fatalf("status = %d, want 200", rec.Code) @@ -207,8 +218,8 @@ func TestHandleHTTP_HappyPathSingleNode(t *testing.T) { if gotBody != `{"model":"llama"}` { t.Errorf("node got body %q, want the original request body", gotBody) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want * on success", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin on success", got) } } @@ -234,7 +245,10 @@ func TestHandleHTTP_NoRetryOn400(t *testing.T) { p.SetSelected("bad") rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`))) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + p.handleHTTP(rec, req) if rec.Code != http.StatusBadRequest { t.Fatalf("status = %d, want 400 (client errors must not fail over)", rec.Code) @@ -249,13 +263,16 @@ func TestHandleHTTP_NoRetryOn400(t *testing.T) { func TestHandleHTTP_RejectionHasCORS(t *testing.T) { p := testProxy(NewDiscovery(), 11434) rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"x"}`))) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"x"}`)) + req.Header.Set("Origin", "https://ui.example") + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + p.handleHTTP(rec, req) if rec.Code != http.StatusBadGateway { t.Fatalf("status = %d, want 502", rec.Code) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want * on rejection", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin on rejection", got) } } @@ -284,7 +301,10 @@ func TestHandleHTTP_FailoverOn503(t *testing.T) { p.SetSelected("busy") // deterministic: busy is tried first rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`))) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + p.handleHTTP(rec, req) if rec.Code != http.StatusOK { t.Fatalf("status = %d, want 200 (should have failed over past the 503)", rec.Code) @@ -292,8 +312,8 @@ func TestHandleHTTP_FailoverOn503(t *testing.T) { if gotBody != `{"model":"llama"}` { t.Errorf("failover node got body %q, want the original request body", gotBody) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want * on proxied success", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin on proxied success", got) } } @@ -314,13 +334,16 @@ func TestHandleHTTP_AllNodesDownReturnsError(t *testing.T) { p := testProxy(disc, 11434) rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`))) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + p.handleHTTP(rec, req) if rec.Code != http.StatusBadGateway { t.Fatalf("status = %d, want 502 when all nodes are down", rec.Code) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want * on exhausted error", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin on exhausted error", got) } } @@ -445,8 +468,8 @@ func TestHandleHTTP_AggregatesModelList(t *testing.T) { if got.Data[1].OwnedBy != "first" { t.Errorf("duplicate metadata = %q, want deterministic first candidate", got.Data[1].OwnedBy) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want *", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want no grant for an Origin-less caller", got) } } @@ -521,7 +544,10 @@ func TestHandleHTTP_StrictModelRouting(t *testing.T) { } rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`))) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + p.handleHTTP(rec, req) if rec.Code != http.StatusOK { t.Fatalf("status = %d, want 200", rec.Code) @@ -562,7 +588,10 @@ func TestHandleHTTP_NoAdvertisedModelRejectsLocally(t *testing.T) { p.SetSelected("missing") rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`))) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + p.handleHTTP(rec, req) if rec.Code != http.StatusBadGateway || !strings.Contains(rec.Body.String(), "no available node advertises the requested model") { diff --git a/services/lmstudio-proxy/ingress.go b/services/lmstudio-proxy/ingress.go index 2b70e697..be5c0375 100644 --- a/services/lmstudio-proxy/ingress.go +++ b/services/lmstudio-proxy/ingress.go @@ -71,14 +71,24 @@ func (p *Proxy) handlePlain(w http.ResponseWriter, r *http.Request) { } slog.Warn("rejected non-loopback plaintext request; cluster peers must use mTLS", "remote", r.RemoteAddr, "method", r.Method, "path", r.URL.Path) - writeIngressError(w, http.StatusForbidden, "loopback-only", + writeIngressError(w, r, http.StatusForbidden, "loopback-only", "plaintext requests are accepted only from loopback; cluster peers must use the mTLS ingress") return } + // Cross-origin browser gate: a loopback bind does not exclude browser + // pages (they connect from loopback), so any Origin this process's + // allowlist does not name is refused before it can drive an engine. + if !cors.AllowRequest(r) { + slog.Warn("rejected cross-origin browser request not on the allowlist", + "remote", r.RemoteAddr, "method", r.Method, "path", r.URL.Path, + "origin", r.Header.Get("Origin")) + cors.RejectOrigin(w) + return + } // Engine-manager marks identity/action requests so the federated model-list // facade can never satisfy LM Studio's own /v1/models readiness probe. if r.Header.Get(engineIdentityProbeHeader) == "1" { - writeIngressError(w, http.StatusConflict, "proxy-facade", "the compatibility facade is not an LM Studio engine") + writeIngressError(w, r, http.StatusConflict, "proxy-facade", "the compatibility facade is not an LM Studio engine") return } p.handleHTTP(w, r) @@ -99,13 +109,13 @@ func (p *Proxy) handleClusterIngress(w http.ResponseWriter, r *http.Request) { p.mesh.Refresh() peer, ok := p.mesh.VerifyClientPin(r) if !ok { - writeIngressError(w, http.StatusForbidden, "cluster-auth", + writeIngressError(w, r, http.StatusForbidden, "cluster-auth", "client certificate is not a pinned member of this node's cluster") return } target, ok := p.localBackendTarget() if !ok { - writeIngressError(w, http.StatusServiceUnavailable, "no-local-backend", + writeIngressError(w, r, http.StatusServiceUnavailable, "no-local-backend", "no local inference backend is available on this node") return } @@ -129,9 +139,9 @@ func (p *Proxy) newLocalReverseProxy(target *url.URL) *httputil.ReverseProxy { req.Host = target.Host }, Transport: p.plainHTTPTransport(), - ErrorHandler: func(ew http.ResponseWriter, _ *http.Request, err error) { + ErrorHandler: func(ew http.ResponseWriter, er *http.Request, err error) { slog.Warn("cluster ingress upstream error", "target", target.Host, "err", err) - writeIngressError(ew, http.StatusBadGateway, "backend-error", "local inference backend error") + writeIngressError(ew, er, http.StatusBadGateway, "backend-error", "local inference backend error") }, } } @@ -152,8 +162,8 @@ func isLoopbackRemote(remoteAddr string) bool { // request body or any generated output. CORS headers are included because these // are the proxy's own rejections: without them a browser client cannot read the // status or reason, and every one of them looks like a generic CORS failure. -func writeIngressError(w http.ResponseWriter, status int, code, msg string) { - cors.Apply(w.Header()) +func writeIngressError(w http.ResponseWriter, r *http.Request, status int, code, msg string) { + cors.Apply(w.Header(), r.Header.Get("Origin")) w.Header().Set("Content-Type", "application/json") w.Header().Set("X-Content-Type-Options", "nosniff") w.WriteHeader(status) diff --git a/services/lmstudio-proxy/ingress_test.go b/services/lmstudio-proxy/ingress_test.go index 2f1e1869..9f12b99b 100644 --- a/services/lmstudio-proxy/ingress_test.go +++ b/services/lmstudio-proxy/ingress_test.go @@ -46,10 +46,12 @@ func TestHandlePlainRejectsNonLoopback(t *testing.T) { if rec.Code != http.StatusForbidden { t.Fatalf("non-loopback plaintext status = %d, want %d", rec.Code, http.StatusForbidden) } - // The refusal carries CORS so a browser client reads this 403 and its reason - // instead of an opaque "CORS error" that hides why the call failed. - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want * on the refusal", got) + // No CORS grant on the refusal: the caller sent no Origin and the proxy + // writes grants only for allowlisted browser origins. A non-browser LAN + // caller ignores CORS anyway; a cross-origin browser is gated by the + // origin check that follows the loopback gate. + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want no grant on the refusal", got) } } @@ -64,11 +66,14 @@ func TestHandlePlainAnswersPreflightBeforeLoopbackGate(t *testing.T) { rec := httptest.NewRecorder() p.handlePlain(rec, req) + // The preflight is still answered (204) — it authorizes nothing and this + // Origin-less caller gets no grant — and the request that follows would + // hit the loopback gate's 403. if rec.Code != http.StatusNoContent { t.Fatalf("preflight status = %d, want %d", rec.Code, http.StatusNoContent) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want *", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want no grant", got) } } diff --git a/services/lmstudio-proxy/proxy.go b/services/lmstudio-proxy/proxy.go index 6e619e77..99abaf37 100644 --- a/services/lmstudio-proxy/proxy.go +++ b/services/lmstudio-proxy/proxy.go @@ -192,26 +192,31 @@ type workloadParams struct { // bufferBodyAndModel reads the request body once and returns the raw bytes // (so each failover attempt can replay it — see the loop in handleHTTP) along -// with the JSON "model" field for workload tracking. Inference bodies are -// small (prompt + model), so full buffering is cheap. Returns (nil, "") when -// the body is absent and an empty model when none is parseable. The caller -// restores r.Body from the returned bytes before each forward attempt. -func bufferBodyAndModel(r *http.Request) ([]byte, string) { +// with the JSON "model" field for workload tracking. Bodies are capped at +// maxInferenceBodyBytes: without a limit, any loopback caller (or a cross-origin +// browser POST) could stream an arbitrarily large body and exhaust proxy +// memory. Returns (nil, "", false) when the body is absent and an empty model +// when none is parseable. The caller restores r.Body from the returned bytes +// before each forward attempt. +func bufferBodyAndModel(r *http.Request) ([]byte, string, bool) { if r.Body == nil { - return nil, "" + return nil, "", false } - body, err := io.ReadAll(r.Body) + body, err := io.ReadAll(io.LimitReader(r.Body, maxInferenceBodyBytes+1)) _ = r.Body.Close() if err != nil { - return body, "" + return body, "", false + } + if len(body) > maxInferenceBodyBytes { + return nil, "", true } var probe struct { Model string `json:"model"` } if err := json.Unmarshal(body, &probe); err != nil { - return body, "" + return body, "", false } - return body, probe.Model + return body, probe.Model, false } type statusCapture struct { @@ -520,6 +525,11 @@ const ( proxyReadHeaderTimeout = 10 * time.Second proxyServerIdleTimeout = 90 * time.Second maxModelListBytes = 16 << 20 + // maxInferenceBodyBytes caps how much of an inbound request body the proxy + // buffers for replay across failover attempts. Long-context prompts fit + // far below this; anything larger is rejected with 413 instead of being + // buffered into memory unbounded. + maxInferenceBodyBytes = 32 << 20 ) // idleClientWriteTimeout bounds how long a single write of streamed response @@ -815,7 +825,7 @@ type modelListResult struct { // deterministic while an unavailable peer cannot hide healthy inventories. func (p *Proxy) serveModelList(w http.ResponseWriter, r *http.Request, candidates []candidate) (int, error) { writeJSON := func(status int, body []byte) { - cors.Apply(w.Header()) + cors.Apply(w.Header(), r.Header.Get("Origin")) w.Header().Set("Content-Type", "application/json") w.Header().Set("X-Content-Type-Options", "nosniff") w.WriteHeader(status) @@ -945,7 +955,22 @@ func (p *Proxy) handleHTTP(w http.ResponseWriter, r *http.Request) { // Parse the request's model before choosing a node. Model eligibility only // applies to inference routes; control endpoints retain their existing // routing behavior even when their JSON happens to contain a model field. - bodyBytes, model := bufferBodyAndModel(r) + bodyBytes, model, bodyTooLarge := bufferBodyAndModel(r) + if bodyTooLarge { + slog.Warn("proxy request rejected", + "id", reqID, "method", r.Method, "path", r.URL.Path, + "remote", r.RemoteAddr, "reason", "request body exceeds limit") + http.Error(w, `{"error":"request body exceeds limit"}`, http.StatusRequestEntityTooLarge) + p.codec.Notify("proxy/request", RequestEvent{ + ID: reqID, + Method: r.Method, + Path: r.URL.Path, + Status: http.StatusRequestEntityTooLarge, + Duration: time.Since(start).Milliseconds(), + Error: "request body exceeds limit", + }) + return + } isInf := isInferenceRequest(r.Method, r.URL.Path) routingModel := "" if isInf { @@ -978,7 +1003,7 @@ func (p *Proxy) handleHTTP(w http.ResponseWriter, r *http.Request) { if cors.WritePreflight(w, r) { return } - cors.Apply(w.Header()) + cors.Apply(w.Header(), r.Header.Get("Origin")) rejectionBody := `{"error":"no active node selected or available"}` rejectionError := "no active node" if isInf && model != "" { @@ -1172,7 +1197,7 @@ func (p *Proxy) handleHTTP(w http.ResponseWriter, r *http.Request) { // response outright. An engine that omits the header has // expressed nothing to preserve, so the proxy supplies its own. if resp.Header.Get("Access-Control-Allow-Origin") == "" { - cors.Apply(resp.Header) + cors.Apply(resp.Header, r.Header.Get("Origin")) } if !started { started = true @@ -1241,7 +1266,7 @@ func (p *Proxy) handleHTTP(w http.ResponseWriter, r *http.Request) { if mErr != nil { body = []byte(`{"error":"upstream error"}`) } - cors.Apply(ew.Header()) + cors.Apply(ew.Header(), r.Header.Get("Origin")) ew.Header().Set("Content-Type", "application/json") ew.Header().Set("X-Content-Type-Options", "nosniff") ew.WriteHeader(http.StatusBadGateway) @@ -1863,8 +1888,15 @@ func (p *Proxy) readLoop(ctx context.Context) error { if err == io.EOF || ctx.Err() != nil { return nil } - log.Printf("JSON-RPC read error: %v", err) - continue + var de *DecodeError + if stderrors.As(err, &de) { + log.Printf("JSON-RPC decode error (skipping frame): %v", err) + continue + } + // Terminal transport/scanner error (e.g. an over-long frame — + // bufio.Scanner cannot resync) — stop instead of spinning. + log.Printf("JSON-RPC read error (terminal): %v", err) + return err } p.handleMessage(msg) } diff --git a/services/nvpair-cluster-manager/cancel.go b/services/nvpair-cluster-manager/cancel.go index 9ad63e82..546a47a2 100644 --- a/services/nvpair-cluster-manager/cancel.go +++ b/services/nvpair-cluster-manager/cancel.go @@ -73,6 +73,22 @@ func (m *Manager) handleCancelInvite(msg *Message) { } // Deleting the Server session invalidates the PIN; a later Completion POST // then hits handlePairingCompletion's unknown-invite / not-pending branches. + // The signal MAC is derived first: it needs the session's ephemeral Key + // Exchange secret, which dies with the session. (sess.mu is already held.) + signalKey, keyErr := sess.ephemeralKey() + if keyErr != nil { + // No Key Exchange secret (initial exchange never completed) — the joiner + // has no session to clear either, so skipping the notify is safe. + m.finishInvite(p.InviteID, inviteStateCanceled) + m.deleteSession(p.InviteID) + m.maybeLeaveInviteCreatedClusterLocked() + m.inviteMu.Unlock() + log.Printf("invite %s: canceled by inviter (no joiner signal key; skipping notify: %v)", p.InviteID, keyErr) + m.codec.RespondErrorData(msg.ID, codeInvalidState, "invite is not pending", + map[string]any{"inviteId": p.InviteID, "state": inviteStateCanceled}) + return + } + signalTag := pairingSignalMAC(signalKey, p.InviteID, "cancel") m.finishInvite(p.InviteID, inviteStateCanceled) m.deleteSession(p.InviteID) joinerAddr := sess.joinerAddr @@ -82,7 +98,7 @@ func (m *Manager) handleCancelInvite(msg *Message) { // Best-effort: tell the joiner so its pending prompt clears. If the joiner is // unreachable, the teardown above still prevents the join. - go m.notifyJoinerCanceled(joinerAddr, p.InviteID) + go m.notifyJoinerCanceled(joinerAddr, p.InviteID, signalTag) log.Printf("invite %s: canceled by inviter", p.InviteID) m.respondInvite(msg, p.InviteID) @@ -90,13 +106,14 @@ func (m *Manager) handleCancelInvite(msg *Message) { // notifyJoinerCanceled POSTs a best-effort "cancel" pairing envelope to the // joiner so it can drop its pending-inbound invite and dismiss the PIN prompt. -// Failures are logged and ignored — the inviter-side teardown is authoritative. -func (m *Manager) notifyJoinerCanceled(joinerAddr, inviteID string) { +// signalTag authenticates the signal (see pairingSignalMAC). Failures are +// logged and ignored — the inviter-side teardown is authoritative. +func (m *Manager) notifyJoinerCanceled(joinerAddr, inviteID, signalTag string) { if joinerAddr == "" { return } client := &http.Client{Timeout: pairingHTTPTimeout} - if _, err := postPairingBlob(client, joinerAddr, inviteID, "cancel", nil); err != nil { + if _, err := postPairingBlobTagged(client, joinerAddr, inviteID, "cancel", nil, signalTag); err != nil { log.Printf("invite %s: notify joiner cancel: %v", inviteID, err) } } diff --git a/services/nvpair-cluster-manager/cm_unit_test.go b/services/nvpair-cluster-manager/cm_unit_test.go index 589177e9..5497a001 100644 --- a/services/nvpair-cluster-manager/cm_unit_test.go +++ b/services/nvpair-cluster-manager/cm_unit_test.go @@ -161,15 +161,38 @@ func TestPINNoobRoundTrip(t *testing.T) { t.Fatalf("noob decodes to %s, want %s", got, want.String()) } } +} + +func TestSaltedPINNoob(t *testing.T) { + salt, err := newPinSalt() + if err != nil { + t.Fatalf("newPinSalt: %v", err) + } + if len(salt) != 16 { + t.Fatalf("salt length %d, want 16", len(salt)) + } - gp, noob, err := generatePIN() + pin, noob, err := mintPIN(salt) if err != nil { - t.Fatalf("generatePIN: %v", err) + t.Fatalf("mintPIN: %v", err) } - if !pinPattern.MatchString(gp) { - t.Fatalf("generated PIN %q is not six digits", gp) + if !pinPattern.MatchString(pin) { + t.Fatalf("generated PIN %q is not six digits", pin) } - if string(noob) != string(noobFromPIN(gp)) { - t.Fatal("generatePIN's noob does not match noobFromPIN of its PIN") + + // Joiner reconstruction must match the inviter's Noob, and the same PIN + // under different salts must produce unrelated Noobs. + joinerNoob := deriveNoobFromPINAndSalt(pin, salt) + if string(joinerNoob) != string(noob) { + t.Fatal("joiner-derived noob does not match inviter noob") + } + otherNoob := pinNoob(pin, append([]byte("x"), salt...)) + if string(otherNoob) == string(noob) { + t.Fatal("different salt produced identical noob") + } + + // Legacy escape hatch: empty salt must equal the unsalted derivation. + if string(pinNoob(pin, nil)) != string(noobFromPIN(pin)) { + t.Fatal("empty salt must fall back to legacy noob derivation") } } diff --git a/services/nvpair-cluster-manager/codec.go b/services/nvpair-cluster-manager/codec.go index 5d6fd177..1691b8cd 100644 --- a/services/nvpair-cluster-manager/codec.go +++ b/services/nvpair-cluster-manager/codec.go @@ -10,9 +10,10 @@ package main import "nvpair-shared/jsonrpc" type ( - Message = jsonrpc.Message - RPCError = jsonrpc.RPCError - Codec = jsonrpc.Codec + Message = jsonrpc.Message + RPCError = jsonrpc.RPCError + DecodeError = jsonrpc.DecodeError + Codec = jsonrpc.Codec ) var NewCodec = jsonrpc.NewCodec diff --git a/services/nvpair-cluster-manager/httpserver.go b/services/nvpair-cluster-manager/httpserver.go index f010e58b..0a5c03cf 100644 --- a/services/nvpair-cluster-manager/httpserver.go +++ b/services/nvpair-cluster-manager/httpserver.go @@ -43,6 +43,11 @@ type pairingEnvelope struct { // state:"rejected" rather than state:"failed". Empty on normal messages. Rejected bool `json:"rejected,omitempty"` Reason string `json:"reason,omitempty"` + // SignalTag authenticates a terminal signal (cancel/decline/expire/fail) + // as an HMAC over the session's ephemeral Key Exchange secret — proof the + // sender ran the Initial Exchange, not a bystander who only observed the + // inviteId on the wire. Verified in handlePairing before the phase runs. + SignalTag string `json:"signalTag,omitempty"` } // runHTTP starts the inter-node listener on the configured port and blocks until @@ -99,8 +104,16 @@ func (m *Manager) handlePairing(w http.ResponseWriter, r *http.Request) { case "completion": m.handlePairingCompletion(w, &env, msg, hostOnly(r.RemoteAddr)) case "cancel": + if sess, ok := m.getSession(env.InviteID); !ok || !verifyPairingSignal(sess, &env, "cancel") { + http.Error(w, "unauthenticated signal", http.StatusUnauthorized) + return + } m.handlePairingCancel(w, &env) case "decline": + if sess, ok := m.getSession(env.InviteID); !ok || !verifyPairingSignal(sess, &env, "decline") { + http.Error(w, "unauthenticated signal", http.StatusUnauthorized) + return + } m.handlePairingDecline(w, &env) case "fail": callerUUID := "" @@ -111,6 +124,11 @@ func (m *Manager) handlePairing(w http.ResponseWriter, r *http.Request) { http.Error(w, "not a trusted peer", http.StatusForbidden) return } + } else if sess, ok := m.getSession(env.InviteID); !ok || !verifyPairingSignal(sess, &env, "fail") { + // Plain-HTTP pre-commit fail (the wrong-PIN mirror): authenticated + // with the session's ephemeral-key MAC like the other signals. + http.Error(w, "unauthenticated signal", http.StatusUnauthorized) + return } m.handlePairingFailedFrom(w, &env, callerUUID) case "ack": @@ -121,6 +139,10 @@ func (m *Manager) handlePairing(w http.ResponseWriter, r *http.Request) { } m.handlePairingAck(w, &env, callerUUID) case "expire": + if sess, ok := m.getSession(env.InviteID); !ok || !verifyPairingSignal(sess, &env, "expire") { + http.Error(w, "unauthenticated signal", http.StatusUnauthorized) + return + } m.handlePairingExpired(w, &env) default: http.Error(w, "unknown phase", http.StatusBadRequest) @@ -259,6 +281,27 @@ func (m *Manager) handlePairingCompletion(w http.ResponseWriter, env *pairingEnv respondPairing(w, blob) return } + // Rate-limit the Completion Exchange before feeding the message to the EAP + // server: the PIN is six digits, so each additional attempt materially + // reduces the work a guessed PIN needs. The NoobId derived from a PIN Noob + // is offline-checkable against a captured transcript (documented PIN Noob + // debt, §4), and the exchange is driven over plaintext HTTP — without a + // cap an on-path attacker can exhaust the keyspace within one invite TTL. + // N attempts tolerate honest typos; beyond that the invite (and the EAP + // session holding its Noob) is torn down, invalidating the transcript. + sess.completionAttempts++ + if sess.completionAttempts > maxCompletionAttempts { + sess.mu.Unlock() + m.inviteMu.Unlock() + // Tear down the invite AND the EAP session: dropping the session + // invalidates the Noob the attacker is testing against, so a resumed + // attack would need a fresh invite (and fresh user cooperation). + m.finishInviteReason(env.InviteID, inviteStateFailed, reasonIncorrectPIN) + m.deleteSession(env.InviteID) + m.emitNodesChanged() + http.Error(w, "too many pairing attempts", http.StatusTooManyRequests) + return + } out, err := sess.server.Receive(msg) if err != nil { http.Error(w, "eap-noob: "+err.Error(), http.StatusBadRequest) @@ -520,6 +563,7 @@ func (m *Manager) prepareJoinerInitialComplete(inviteID string, sess *pairingSes type supersededInboundInvite struct { invite *Invite inviterAddr string + signalTag string } // onJoinerInitialComplete publishes the newest pending invite from a sender and @@ -556,7 +600,7 @@ func (m *Manager) onJoinerInitialComplete(inv *Invite, sess *pairingSession) { if err := m.codec.Notify("cluster:invite-canceled", old.invite); err != nil { log.Printf("emit cluster:invite-canceled: %v", err) } - go m.notifyInviterTerminal(old.inviterAddr, old.invite.InviteID, "decline", "") + go m.notifyInviterTerminal(old.inviterAddr, old.invite.InviteID, "decline", "", old.signalTag) log.Printf("invite %s: superseded by newer invite %s from %s", old.invite.InviteID, inv.InviteID, inv.FromNodeUUID) } @@ -590,6 +634,7 @@ func (m *Manager) supersedePendingInboundLocked(fromNodeUUID, keepInviteID strin sess.mu.Unlock() continue } + signalTag := sessionSignalTag(sess, id, "decline") m.finishInvite(id, inviteStateCanceled) m.deleteSession(id) inviterAddr := sess.addr @@ -603,6 +648,7 @@ func (m *Manager) supersedePendingInboundLocked(fromNodeUUID, keepInviteID strin superseded = append(superseded, supersededInboundInvite{ invite: canceled, inviterAddr: inviterAddr, + signalTag: signalTag, }) } return superseded diff --git a/services/nvpair-cluster-manager/invite.go b/services/nvpair-cluster-manager/invite.go index 388acef1..cf24901c 100644 --- a/services/nvpair-cluster-manager/invite.go +++ b/services/nvpair-cluster-manager/invite.go @@ -257,17 +257,29 @@ var errPairingAbandoned = errors.New("pairing abandoned: invite is no longer pen // its liveness. func (m *Manager) runInitialExchange(inviteID, target, cid0 string, sessGen0 uint64) (*pairingSession, string, error) { myAddr := net.JoinHostPort(outboundIP(target), strconv.Itoa(m.port)) - info, err := m.localPairingInfo(myAddr).toMap() + // The PIN stretching salt must exist before the Initial Exchange so it can + // ride in ServerInfo, which the EAP-NOOB transcript MACs (integrity- + // protected path to the joiner). + salt, err := newPinSalt() + if err != nil { + return nil, "", fmt.Errorf("generate pin salt: %w", err) + } + info, err := m.localPairingInfoWithPinSalt(myAddr, salt) + if err != nil { + return nil, "", fmt.Errorf("build pairing info: %w", err) + } + infoMap, err := info.toMap() if err != nil { return nil, "", fmt.Errorf("build pairing info: %w", err) } - server := newPairingServer(info) + server := newPairingServer(infoMap) // Scope the session to the cluster we are inviting into (cid0, captured by // handleInviteNode) so a Completion that lands after we leave/are removed is // discarded by commitPairing's epoch recheck. sess := &pairingSession{ inviteID: inviteID, role: roleInviter, createdAt: time.Now().UnixMilli(), server: server, addr: myAddr, clusterID: cid0, joinerAddr: target, + pinSalt: salt, } // Register under the teardown boundary: if the cluster was torn down (or // rejoined, bumping the session generation) since the invite began, abandon @@ -309,7 +321,7 @@ func (m *Manager) runInitialExchange(inviteID, target, cid0 string, sessGen0 uin if server.State() != eapnoob.StateWaiting { return sess, "", fmt.Errorf("initial exchange ended in state %s, want Waiting", server.State()) } - pin, noob, err := generatePIN() + pin, noob, err := mintPIN(sess.pinSalt) if err != nil { return sess, "", fmt.Errorf("generate pin: %w", err) } @@ -360,18 +372,20 @@ func postPairingBlob(client *http.Client, target, inviteID, phase string, blob [ return base64.StdEncoding.DecodeString(out.Msg) } -// postPairingSignal POSTs a best-effort terminal pairing signal (phase +// postPairingSignalScheme POSTs a best-effort terminal pairing signal (phase // "decline" or "fail") to the inviter so it can tear down its pending outbound -// invite. Unlike postPairingBlob it carries no EAP blob (the pairing is already -// terminal) but does carry the Reason so the inviter can surface a specific -// cause (e.g. "incorrect-pin"). Returns an error only on transport / non-200 so -// the caller can retry. -func postPairingSignal(client *http.Client, target, inviteID, phase, reason string) error { - return postPairingSignalScheme(client, "http", target, inviteID, phase, reason) +// invite. It carries no EAP blob (the pairing is already terminal) and no +// reason; it sends no authentication tag — a hardened inviter rejects it and +// falls back to its TTL (see postPairingSignalTagged). +func postPairingSignalScheme(client *http.Client, scheme, target, inviteID, phase, reason string) error { + return postPairingSignalTagged(client, scheme, target, inviteID, phase, reason, "") } -func postPairingSignalScheme(client *http.Client, scheme, target, inviteID, phase, reason string) error { - env := pairingEnvelope{InviteID: inviteID, Phase: phase, Reason: reason} +// postPairingSignalTagged is postPairingSignalScheme with an optional +// authentication tag for the signal (see pairingSignalMAC). An empty tag is +// sent unauthenticated and will be rejected by hardened inviters. +func postPairingSignalTagged(client *http.Client, scheme, target, inviteID, phase, reason, signalTag string) error { + env := pairingEnvelope{InviteID: inviteID, Phase: phase, Reason: reason, SignalTag: signalTag} body, err := json.Marshal(env) if err != nil { return err @@ -388,6 +402,46 @@ func postPairingSignalScheme(client *http.Client, scheme, target, inviteID, phas return nil } +// postPairingBlobTagged is postPairingBlob with an optional authentication tag +// for transport-level signals that carry no EAP payload (e.g. "cancel"). +func postPairingBlobTagged(client *http.Client, target, inviteID, phase string, blob []byte, signalTag string) ([]byte, error) { + env := pairingEnvelope{InviteID: inviteID, Phase: phase, SignalTag: signalTag} + if len(blob) > 0 { + env.Msg = base64.StdEncoding.EncodeToString(blob) + } + body, err := json.Marshal(env) + if err != nil { + return nil, err + } + resp, err := client.Post("http://"+target+pairingPath, "application/json", bytes.NewReader(body)) + if err != nil { + return nil, err + } + defer resp.Body.Close() + rb, _ := io.ReadAll(io.LimitReader(resp.Body, maxBodyBytes)) + if resp.StatusCode == http.StatusConflict { + // A 409 may be an explicit pairing refusal (rejected envelope with a + // reason) rather than a protocol error. Decode it so the inviter can + // report state:"rejected"; a plain-text 409 (session/phase mismatch) + // won't set Rejected and falls through to the generic error below. + var out pairingEnvelope + if json.Unmarshal(rb, &out) == nil && out.Rejected { + return nil, &pairingRejectedError{reason: out.Reason} + } + } + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("pairing %s: status %d: %s", phase, resp.StatusCode, string(rb)) + } + var out pairingEnvelope + if err := json.Unmarshal(rb, &out); err != nil { + return nil, fmt.Errorf("decode pairing response: %w", err) + } + if out.Msg == "" { + return nil, nil + } + return base64.StdEncoding.DecodeString(out.Msg) +} + // reachableEndpointFirst moves the first endpoint whose pairing port accepts a // connection to the front, keeping the others behind it in their published order. // diff --git a/services/nvpair-cluster-manager/invite_expiry.go b/services/nvpair-cluster-manager/invite_expiry.go index 756c072b..611cbb31 100644 --- a/services/nvpair-cluster-manager/invite_expiry.go +++ b/services/nvpair-cluster-manager/invite_expiry.go @@ -239,11 +239,12 @@ func (m *Manager) expireInboundInvitesLocked(ids []string) { membersChanged = true } inviterAddr := sess.addr + signalTag := sessionSignalTag(sess, id, "expire") m.deleteSession(id) sess.mu.Unlock() m.emitInviteExpired(id) - go m.notifyInviterTerminal(inviterAddr, id, "expire", "") + go m.notifyInviterTerminal(inviterAddr, id, "expire", "", signalTag) log.Printf("invite %s: expired inbound (TTL elapsed with no local response)", id) } if membersChanged { diff --git a/services/nvpair-cluster-manager/manager.go b/services/nvpair-cluster-manager/manager.go index ac45bc8b..9bb84217 100644 --- a/services/nvpair-cluster-manager/manager.go +++ b/services/nvpair-cluster-manager/manager.go @@ -6,6 +6,7 @@ package main import ( "context" "encoding/json" + stderrors "errors" "fmt" "io" "log" @@ -372,8 +373,15 @@ func (m *Manager) readLoop(ctx context.Context) error { if err == io.EOF || ctx.Err() != nil { return nil } - log.Printf("JSON-RPC read error: %v", err) - continue + var de *DecodeError + if stderrors.As(err, &de) { + log.Printf("JSON-RPC decode error (skipping frame): %v", err) + continue + } + // Terminal transport/scanner error (e.g. an over-long frame — + // bufio.Scanner cannot resync) — stop instead of spinning. + log.Printf("JSON-RPC read error (terminal): %v", err) + return err } m.handleMessage(msg) if ctx.Err() != nil { diff --git a/services/nvpair-cluster-manager/pairing.go b/services/nvpair-cluster-manager/pairing.go index 218d1467..3ac5bd27 100644 --- a/services/nvpair-cluster-manager/pairing.go +++ b/services/nvpair-cluster-manager/pairing.go @@ -4,10 +4,15 @@ package main import ( + "crypto/hmac" + "crypto/pbkdf2" "crypto/rand" + "crypto/sha256" "crypto/x509" + "encoding/base64" "encoding/json" "encoding/pem" + "errors" "fmt" "math/big" "regexp" @@ -42,6 +47,11 @@ type PairingInfo struct { ClusterFriendlyName string `json:"clusterFriendlyName"` Addr string `json:"addr,omitempty"` Cert string `json:"cert"` + // PinSalt is the inviter's per-invite PBKDF2 salt for stretching the PIN + // into the OOB Noob (see pinNoob), base64-encoded. It rides inside the + // EAP-NOOB transcript (ServerInfo is MAC-covered), so tampering with it + // breaks the transcript handshake rather than substituting a chosen Noob. + PinSalt string `json:"pinSalt,omitempty"` } // localPairingInfo builds this node's PairingInfo. addr is the inviter's @@ -68,6 +78,15 @@ func (m *Manager) localPairingInfo(addr string, admissionEpoch ...uint64) *Pairi } } +// localPairingInfoWithPinSalt is localPairingInfo for inviter pairing sessions, +// additionally carrying the PIN stretching salt so the joiner can reconstruct +// the Noob from the user's typed PIN. +func (m *Manager) localPairingInfoWithPinSalt(addr string, salt []byte) (*PairingInfo, error) { + pi := m.localPairingInfo(addr) + pi.PinSalt = base64.StdEncoding.EncodeToString(salt) + return pi, nil +} + // toMap renders the PairingInfo as the map[string]any the eap-noob config // expects for ServerInfo/PeerInfo. func (pi *PairingInfo) toMap() (map[string]any, error) { @@ -115,20 +134,68 @@ func parsePairingInfo(raw []byte) (*PairingInfo, *x509.Certificate, error) { return &pi, cert, nil } -// generatePIN returns a fresh random six-digit PIN and its 16-byte Noob -// encoding. -func generatePIN() (string, []byte, error) { +// pinPBKDF2Iterations is the stretching work factor applied when deriving the +// PIN Noob. The NoobId the joiner transmits is H("NoobId", Noob) with no key, +// so a passive observer of the plaintext pairing channel could otherwise check +// all 10^6 PIN candidates against a captured NoobId in milliseconds. A PBKDF2 +// pass per candidate makes offline enumeration ~10^6 × iteration cost instead, +// and the online cap in handlePairingCompletion complements it for active +// guessing. Tune upward with hardware. +const pinPBKDF2Iterations = 50000 + +// newPinSalt returns a fresh per-invite PBKDF2 salt for stretching the PIN +// into the OOB Noob. It is generated before the Initial Exchange so it can ride +// inside the inviter's ServerInfo — which the EAP-NOOB transcript MACs — +// giving the joiner an integrity-protected copy without a second channel. +func newPinSalt() ([]byte, error) { + salt := make([]byte, 16) + if _, err := rand.Read(salt); err != nil { + return nil, fmt.Errorf("generate pin salt: %w", err) + } + return salt, nil +} + +// mintPIN draws the random six-digit PIN and derives its Noob under the +// session's salt. Identical PINs across invites produce unrelated Noobs, and a +// captured NoobId cannot be attacked offline without redoing the full stretched +// search over all 10^6 candidates. +func mintPIN(salt []byte) (string, []byte, error) { n, err := rand.Int(rand.Reader, big.NewInt(1000000)) if err != nil { - return "", nil, err + return "", nil, fmt.Errorf("generate pin: %w", err) } pin := fmt.Sprintf("%06d", n.Int64()) - return pin, noobFromPIN(pin), nil + return pin, pinNoob(pin, salt), nil +} + +// pinNoob derives the 16-byte Noob from a PIN and its per-invite salt: +// PBKDF2-HMAC-SHA256(PIN, salt), first 16 bytes. Stretching is the +// compensating control for the low-entropy PIN Noob (documented security +// debt, §4). An empty salt falls back to the legacy big-endian encoding so a +// pre-salt inviter remains pairable; the salt itself is not secret (it is +// MAC-covered against tampering), the work factor is the point. +func pinNoob(pin string, salt []byte) []byte { + if len(salt) == 0 { + return noobFromPIN(pin) + } + k, err := pbkdf2.Key(sha256.New, pin, salt, pinPBKDF2Iterations, 16) + if err != nil { + // sha256.New never fails to construct; an error here is a programming + // fault, so fail closed rather than pairing on a weak Noob. + panic(fmt.Sprintf("pbkdf2: %v", err)) + } + return k +} + +// deriveNoobFromPINAndSalt reconstructs the Noob on the joiner side from the +// PIN the user typed and the salt from the inviter's MAC-covered ServerInfo. +func deriveNoobFromPINAndSalt(pin string, salt []byte) []byte { + return pinNoob(pin, salt) } // noobFromPIN encodes a six-digit PIN as a 16-byte big-endian Noob. This is the -// low-entropy, temporary stand-in for a real OOB nonce (documented security -// debt, §4). +// legacy unsalted derivation, kept only for tests asserting the migration +// reference; production flows use pinNoob with a per-invite salt. func noobFromPIN(pin string) []byte { v := new(big.Int) v.SetString(pin, 10) @@ -157,6 +224,49 @@ const ( roleJoiner // EAP-NOOB Peer ) +// pairingSignalLabel domain-separates the terminal-signal MAC derived from the +// session's ephemeral Key Exchange secret. +const pairingSignalLabel = "nvpair-pairing-signal-v1" + +// pairingSignalMAC derives the authentication tag for a terminal pairing +// signal (cancel/decline/expire/fail) from the session's EphemeralKey — the +// ECDH secret both sides already share after the Initial Exchange. A passive +// on-path observer of the plaintext pairing channel can read the inviteId but +// cannot compute this tag, so terminal signals are no longer forgeable. The +// tag is base64-encoded for transport; verification decodes before comparing. +func pairingSignalMAC(key []byte, inviteID, phase string) string { + mac := hmac.New(sha256.New, key) + mac.Write([]byte(pairingSignalLabel)) + mac.Write([]byte{0}) + mac.Write([]byte(inviteID)) + mac.Write([]byte{0}) + mac.Write([]byte(phase)) + return base64.StdEncoding.EncodeToString(mac.Sum(nil)) +} + +// verifyPairingSignal reports whether env carries a valid tag for the given +// phase under the session's ephemeral key. Unknown/absent keys verify nothing. +func verifyPairingSignal(sess *pairingSession, env *pairingEnvelope, phase string) bool { + if sess == nil || env.SignalTag == "" { + return false + } + sess.mu.Lock() + defer sess.mu.Unlock() + key, err := sess.ephemeralKey() + if err != nil { + return false + } + want, err := base64.StdEncoding.DecodeString(pairingSignalMAC(key, env.InviteID, phase)) + if err != nil { + return false + } + got, err := base64.StdEncoding.DecodeString(env.SignalTag) + if err != nil { + return false + } + return hmac.Equal(want, got) +} + // pairingSession holds the live EAP-NOOB crypto state for one in-flight pairing, // kept alive across the separate HTTP requests of the Initial and Completion // exchanges and the human PIN step in between (§7.2). Keyed by inviteId. @@ -200,10 +310,40 @@ type pairingSession struct { // retryable and lets a post-success joiner failure roll the inviter back. awaitingAck bool completionResponse []byte + // completionAttempts counts Completion Exchange messages fed to the EAP + // server, bounding online PIN guessing against the six-digit PIN Noob + // (see handlePairingCompletion). + completionAttempts int + // pinSalt is the per-invite PBKDF2 salt (inviter: generated with the PIN; + // joiner: learned from the inviter's authenticated ServerInfo) used to + // stretch the PIN into the OOB Noob. + pinSalt []byte mu sync.Mutex } +// maxCompletionAttempts bounds PIN-entry attempts per invite: enough for +// honest typos, far below the 10^6 PIN keyspace. +const maxCompletionAttempts = 5 + +// ephemeralKey returns this session's Key Exchange secret (see +// eapnoob.EphemeralKey). The caller must hold sess.mu (the eapnoob Server/Peer +// objects are not synchronized). Sessions whose EAP object was never created +// (or whose Initial Exchange never completed) report an error — callers treat +// the signal as unauthenticated/unsendable. +func (s *pairingSession) ephemeralKey() ([]byte, error) { + if s.role == roleInviter { + if s.server == nil { + return nil, errors.New("no inviter EAP session") + } + return s.server.EphemeralKey() + } + if s.peer == nil { + return nil, errors.New("no joiner EAP session") + } + return s.peer.EphemeralKey() +} + func (m *Manager) putSession(s *pairingSession) { m.sessMu.Lock() defer m.sessMu.Unlock() diff --git a/services/nvpair-cluster-manager/respond.go b/services/nvpair-cluster-manager/respond.go index 33501b62..905dcbee 100644 --- a/services/nvpair-cluster-manager/respond.go +++ b/services/nvpair-cluster-manager/respond.go @@ -4,6 +4,7 @@ package main import ( + "encoding/base64" "encoding/json" "errors" "fmt" @@ -94,6 +95,7 @@ func (m *Manager) handleRespondToInvite(msg *Message) { // teardown is authoritative even if the notify ultimately fails — the // inviter's invite TTL then expires the pending invite as a fallback. inviterAddr := sess.addr + signalTag := sessionSignalTag(sess, p.InviteID, "decline") m.finishInvite(p.InviteID, inviteStateDeclined) if sess.peerPairing != nil { m.removeMemberByUUID(sess.peerPairing.NodeUUID) @@ -101,7 +103,7 @@ func (m *Manager) handleRespondToInvite(msg *Message) { m.deleteSession(p.InviteID) sess.mu.Unlock() m.emitNodesChanged() - go m.notifyInviterTerminal(inviterAddr, p.InviteID, "decline", "") + go m.notifyInviterTerminal(inviterAddr, p.InviteID, "decline", "", signalTag) log.Printf("invite %s: declined by local user", p.InviteID) m.respondInvite(msg, p.InviteID) return @@ -152,6 +154,7 @@ func (m *Manager) handleRespondToInvite(msg *Message) { // after the session is dropped. Local teardown is authoritative even if // the notify fails — the inviter's invite TTL is the remaining backstop. inviterAddr := sess.addr + signalTag := sessionSignalTag(sess, p.InviteID, "fail") m.finishInviteReason(p.InviteID, inviteStateFailed, reason) if sess.peerPairing != nil { m.removeMemberByUUID(sess.peerPairing.NodeUUID) @@ -159,7 +162,7 @@ func (m *Manager) handleRespondToInvite(msg *Message) { m.deleteSession(p.InviteID) sess.mu.Unlock() m.emitNodesChanged() - go m.notifyInviterTerminal(inviterAddr, p.InviteID, "fail", reason) + go m.notifyInviterTerminal(inviterAddr, p.InviteID, "fail", reason, signalTag) m.respondInvite(msg, p.InviteID) return } @@ -249,7 +252,19 @@ func (m *Manager) runCompletionExchangeLocked(sess *pairingSession, pin string) if sess.addr == "" { return fmt.Errorf("inviter address unknown") } - if err := sess.peer.OOBInputNoob(noobFromPIN(pin)); err != nil { + // The salt arrived inside the inviter's MAC-covered ServerInfo (captured + // into sess.peerPairing at Initial completion), so the PIN Noob we derive + // matches what the inviter injected. Empty salt means a legacy pre-salt + // inviter: derive the legacy unsalted Noob (see pinNoob). + salt := sess.pinSalt + if len(salt) == 0 && sess.peerPairing != nil && sess.peerPairing.PinSalt != "" { + s, err := base64.StdEncoding.DecodeString(sess.peerPairing.PinSalt) + if err != nil { + return fmt.Errorf("decode pin salt: %w", err) + } + salt = s + } + if err := sess.peer.OOBInputNoob(deriveNoobFromPINAndSalt(pin, salt)); err != nil { return fmt.Errorf("feed pin: %w", err) } @@ -355,16 +370,18 @@ func (m *Manager) notifyInviterAuthenticated(inviterAddr string, inviterDER []by } // notifyInviterTerminal POSTs a pre-commit pairing outcome signal -// (decline/fail/expire). Retries a few times; remaining failures are logged and -// periodic reconciliation/TTL cleanup is the backstop. -func (m *Manager) notifyInviterTerminal(inviterAddr, inviteID, phase, reason string) { +// (decline/fail/expire), authenticated with signalTag over the session's +// ephemeral Key Exchange secret (see pairingSignalMAC). Retries a few times; +// remaining failures are logged and periodic reconciliation/TTL cleanup is the +// backstop. +func (m *Manager) notifyInviterTerminal(inviterAddr, inviteID, phase, reason, signalTag string) { if inviterAddr == "" { return } client := &http.Client{Timeout: pairingHTTPTimeout} var err error for attempt := 1; attempt <= inviterNotifyAttempts; attempt++ { - err = postPairingSignal(client, inviterAddr, inviteID, phase, reason) + err = postPairingSignalTagged(client, "http", inviterAddr, inviteID, phase, reason, signalTag) if err == nil { return } @@ -375,6 +392,18 @@ func (m *Manager) notifyInviterTerminal(inviterAddr, inviteID, phase, reason str } } +// sessionSignalTag computes the terminal-signal MAC for a live joiner session +// (caller holds sess.mu) and must be called before the session is deleted. +// An empty tag is returned when no Key Exchange secret exists yet — the signal +// is then sent unauthenticated and a hardened inviter falls back to its TTL. +func sessionSignalTag(sess *pairingSession, inviteID, phase string) string { + key, err := sess.ephemeralKey() + if err != nil { + return "" + } + return pairingSignalMAC(key, inviteID, phase) +} + // finishInvite transitions an invite to a terminal state and stamps respondedAt. func (m *Manager) finishInvite(inviteID string, state InviteState) { m.finishInviteReason(inviteID, state, "") diff --git a/services/nvpair-engine-manager/install.go b/services/nvpair-engine-manager/install.go index 268bd0b9..1d2f53f9 100644 --- a/services/nvpair-engine-manager/install.go +++ b/services/nvpair-engine-manager/install.go @@ -295,17 +295,29 @@ func (e *Executor) download(ctx context.Context, engine string, f *Fetch) (strin os.Remove(tmp.Name()) return "", fmt.Errorf("checksum mismatch for %s: got %s, want %s", f.URL, sum, want) } - } else { - // Unpinned download: bytes are not integrity-checked, only - // transport-secured (HTTPS, enforced above) — the same weaker - // guarantee as a `script` install. Logged loudly, and the computed - // digest is surfaced so a manifest author can pin it later. - slog.Warn("UNPINNED download: manifest has no sha256, integrity not verified", + } else if allowUnpinnedDownloads() { + // Explicit operator opt-in: bytes are not integrity-checked, only + // transport-secured (HTTPS, enforced above). Logged loudly, and the + // computed digest is surfaced so a manifest author can pin it later. + slog.Warn("UNPINNED download allowed by NVPAIR_ALLOW_UNPINNED_DOWNLOADS: integrity not verified", "engine", engine, "url", f.URL, "computed_sha256", sum) + } else { + // Fail closed: an executed artifact without a pinned digest means any + // HTTPS host serving the manifest URL yields code execution. Surface + // the digest so the manifest author can pin it immediately. + os.Remove(tmp.Name()) + return "", fmt.Errorf("download %s has no sha256 pin in the manifest (computed sha256 %s); "+ + "pin it or set NVPAIR_ALLOW_UNPINNED_DOWNLOADS=1 to accept unverified downloads", f.URL, sum) } return tmp.Name(), nil } +// allowUnpinnedDownloads reports whether the operator explicitly opted in to +// executing downloads whose manifests carry no sha256 pin. +func allowUnpinnedDownloads() bool { + return os.Getenv("NVPAIR_ALLOW_UNPINNED_DOWNLOADS") == "1" +} + // runCommand executes a manifest-declared argv (an install or uninstall // step), hiding the console window on Windows; on failure it returns the // combined output for diagnostics. diff --git a/services/nvpair-engine-manager/lifecycle.go b/services/nvpair-engine-manager/lifecycle.go index c10244b0..853180d6 100644 --- a/services/nvpair-engine-manager/lifecycle.go +++ b/services/nvpair-engine-manager/lifecycle.go @@ -259,7 +259,7 @@ func (e *Executor) bringUpProcess(ctx context.Context, st *engineState, engine s st.mu.Lock() st.stopping = true st.mu.Unlock() - proc.stop() + proc.stop(rt) st.mu.Lock() st.proc = nil st.mu.Unlock() @@ -441,7 +441,7 @@ func (e *Executor) doStop(st *engineState, engine string) error { return e.reconcileFailedCommandStop(st, engine, rt.Ready == nil || !e.waitUnavailable(rt.Ready, port, time.Second), err) } } else if proc != nil { - proc.stop() + proc.stop(rt) } e.markStopped(st, engine) diff --git a/services/nvpair-engine-manager/proc.go b/services/nvpair-engine-manager/proc.go index 8c92c773..b86f9a7e 100644 --- a/services/nvpair-engine-manager/proc.go +++ b/services/nvpair-engine-manager/proc.go @@ -75,21 +75,23 @@ func scanLines(r io.Reader, stream string, onLine func(stream, line string)) { } } -// stop stops the process and waits for it to exit, with no timeout. +// stop stops the process and waits for it to exit. // -// It sends one platform-appropriate stop signal (see gracefulSignal) and then -// blocks until the process is gone: -// - Unix: SIGTERM to the process group — a graceful ask, with no escalation -// to SIGKILL. A well-behaved engine (Ollama, and the test fake) exits on it. +// It sends one platform-appropriate stop signal (see gracefulSignal), waits +// stopGrace(rt) (the manifest's stop spec, default 5s) for the engine to +// honor it, and then escalates to a forced kill of the whole tree/group (see +// forceSignal): +// - Unix: SIGTERM to the process group, then SIGKILL to the group if the +// engine is still alive after the grace period. The group kill reaches +// engines that forked helper processes (model runners, etc.). // - Windows: taskkill /T /F. Our engines run windowless, and a windowless -// process can't receive a graceful (non-/F) close, so /F is the only signal -// that actually stops it — never force-killing there would leave the engine -// running forever. +// process can't receive a graceful (non-/F) close, so /F is the only +// signal that actually stops it — never force-killing there would leave +// the engine running forever. // -// There is deliberately no timeout: a stop is complete only when the engine has -// actually exited. On Unix an engine that ignored SIGTERM would not be stopped -// and this would wait for it; in practice engines exit on SIGTERM. -func (mp *managedProc) stop() { +// A stop is complete only when the engine has actually exited, so after +// escalation stop still blocks on the exit rather than returning early. +func (mp *managedProc) stop(rt Runtime) { if mp == nil || mp.cmd == nil || mp.cmd.Process == nil { return } @@ -99,6 +101,16 @@ func (mp *managedProc) stop() { default: } _ = gracefulSignal(mp.cmd) + deadline := time.After(stopGrace(rt)) + select { + case <-mp.done: + return + case <-deadline: + } + // The engine ignored the graceful signal: force the whole group. SIGKILL + // cannot be caught, so the process-exit goroutine will observe the exit + // and close done. + _ = forceSignal(mp.cmd) <-mp.done } diff --git a/services/nvpair-engine-manager/proc_unix.go b/services/nvpair-engine-manager/proc_unix.go index 876e2b69..2b58fa4b 100644 --- a/services/nvpair-engine-manager/proc_unix.go +++ b/services/nvpair-engine-manager/proc_unix.go @@ -29,10 +29,10 @@ func configureSysProcAttr(cmd *exec.Cmd) { } // gracefulSignal sends SIGTERM to the process group (falling back to the -// process itself). It is the only stop signal engine-manager sends: stop() -// sends this once and waits for the engine to exit, and never escalates to -// SIGKILL. A well-behaved engine (Ollama, and the test fake, whose default -// SIGTERM disposition is to exit) terminates on it. +// process itself). It is the graceful stop signal: stop() sends this and +// waits stopGrace for the engine to exit before escalating to forceSignal. +// A well-behaved engine (Ollama, and the test fake, whose default SIGTERM +// disposition is to exit) terminates on it. func gracefulSignal(cmd *exec.Cmd) error { if cmd == nil || cmd.Process == nil { return nil @@ -43,6 +43,19 @@ func gracefulSignal(cmd *exec.Cmd) error { return cmd.Process.Signal(syscall.SIGTERM) } +// forceSignal sends SIGKILL to the process group (falling back to the process +// itself). stop() escalates to it when the engine ignores the graceful signal +// for stopGrace — SIGKILL cannot be caught, so the engine cannot survive it. +func forceSignal(cmd *exec.Cmd) error { + if cmd == nil || cmd.Process == nil { + return nil + } + if pgid, err := syscall.Getpgid(cmd.Process.Pid); err == nil { + return syscall.Kill(-pgid, syscall.SIGKILL) + } + return cmd.Process.Kill() +} + // pidOnPort returns the PID listening on the given TCP port and that // process's executable path. Best-effort on the debug-only Unix targets: it // shells out to lsof, falling back to ss. ok is false when neither resolves diff --git a/services/nvpair-engine-manager/proc_windows.go b/services/nvpair-engine-manager/proc_windows.go index c383c83d..847b6486 100644 --- a/services/nvpair-engine-manager/proc_windows.go +++ b/services/nvpair-engine-manager/proc_windows.go @@ -30,17 +30,27 @@ func configureSysProcAttr(cmd *exec.Cmd) { } } -// gracefulSignal stops the process tree and is the only stop signal -// engine-manager sends: stop() sends this once and waits for the engine to -// exit. Windows has no SIGTERM, and the engines we spawn run windowless -// (CREATE_NO_WINDOW), so a non-/F taskkill only posts WM_CLOSE — which a -// windowless process can't receive ("can only be terminated forcefully"), i.e. -// it does nothing. Never force-killing such a process would leave the engine -// running forever, so on Windows the stop is taskkill /T /F. +// gracefulSignal stops the process tree with taskkill /T /F. Windows has no +// SIGTERM, and the engines we spawn run windowless (CREATE_NO_WINDOW), so a +// non-/F taskkill only posts WM_CLOSE — which a windowless process can't +// receive ("can only be terminated forcefully"), i.e. it does nothing. Never +// force-killing such a process would leave the engine running forever, so on +// Windows the stop is immediately /T /F; stop()'s escalation timer never +// fires because the tree is already dead by the time gracefulSignal returns. func gracefulSignal(cmd *exec.Cmd) error { return taskkill(cmd, true) } +// forceSignal is the escalation stop() uses when the engine ignores the +// graceful signal. On Windows gracefulSignal already force-killed the whole +// tree, so this is a no-op: taskkill /T /F cannot be ignored. +func forceSignal(cmd *exec.Cmd) error { + if cmd == nil || cmd.Process == nil { + return nil + } + return taskkill(cmd, true) +} + func taskkill(cmd *exec.Cmd, force bool) error { if cmd == nil || cmd.Process == nil { return nil diff --git a/services/nvpair-engine-manager/remediation_test.go b/services/nvpair-engine-manager/remediation_test.go index 1999373b..32052de7 100644 --- a/services/nvpair-engine-manager/remediation_test.go +++ b/services/nvpair-engine-manager/remediation_test.go @@ -56,8 +56,9 @@ func capturingExecutor(t *testing.T, m *Manifest) (*Executor, *captured) { return ex, c } -// TestDownloadUnpinned verifies a fetch with no sha256 succeeds (over -// loopback http) — the unpinned path the bundled Ollama manifest now uses. +// TestDownloadUnpinned verifies a fetch with no sha256 is REJECTED by +// default (fail-closed) and succeeds only with the explicit operator +// opt-in, over loopback http. func TestDownloadUnpinned(t *testing.T) { payload := []byte("unpinned engine bytes") srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -65,9 +66,19 @@ func TestDownloadUnpinned(t *testing.T) { })) defer srv.Close() ex := newTestExecutor(t, testEngineManifest(fakeEngineBin)) + + // Default: fail closed, and the error must surface the computed digest + // so a manifest author can pin it. + if _, err := ex.download(context.Background(), "fake", &Fetch{URL: srv.URL}); err == nil { + t.Fatal("unpinned download should be rejected without the opt-in") + } else if !strings.Contains(err.Error(), "NVPAIR_ALLOW_UNPINNED_DOWNLOADS") { + t.Fatalf("rejection should name the opt-in, got %v", err) + } + + t.Setenv("NVPAIR_ALLOW_UNPINNED_DOWNLOADS", "1") p, err := ex.download(context.Background(), "fake", &Fetch{URL: srv.URL}) // no SHA256 if err != nil { - t.Fatalf("unpinned download should succeed, got %v", err) + t.Fatalf("unpinned download should succeed with the opt-in, got %v", err) } defer os.Remove(p) if got, _ := os.ReadFile(p); string(got) != string(payload) { diff --git a/services/nvpair-errors/codec.go b/services/nvpair-errors/codec.go index 5d6fd177..1691b8cd 100644 --- a/services/nvpair-errors/codec.go +++ b/services/nvpair-errors/codec.go @@ -10,9 +10,10 @@ package main import "nvpair-shared/jsonrpc" type ( - Message = jsonrpc.Message - RPCError = jsonrpc.RPCError - Codec = jsonrpc.Codec + Message = jsonrpc.Message + RPCError = jsonrpc.RPCError + DecodeError = jsonrpc.DecodeError + Codec = jsonrpc.Codec ) var NewCodec = jsonrpc.NewCodec diff --git a/services/nvpair-errors/httpserver.go b/services/nvpair-errors/httpserver.go index d4d46ac5..ff9fd88a 100644 --- a/services/nvpair-errors/httpserver.go +++ b/services/nvpair-errors/httpserver.go @@ -23,6 +23,7 @@ import ( "crypto/tls" "encoding/json" "fmt" + "io" "log/slog" "net" "net/http" @@ -32,6 +33,10 @@ import ( "nvpair-shared/errors" ) +// maxIngestBodyBytes bounds an inbound error-sync snapshot, matching the +// 1 MiB cap the other inter-node endpoints apply to request bodies. +const maxIngestBodyBytes = 1 << 20 + // newErrorsMux builds the HTTP routes backed by the manager. Split out // from the listener so tests can exercise the handlers via // httptest.NewServer without binding a real port or touching mDNS. @@ -51,13 +56,14 @@ func newErrorsMux(mgr *Manager, mesh *clustertrust.Mesh) *http.ServeMux { // unclustered node serve this data in the clear, so there is no branch here // for a future caller to widen. mesh.Refresh() - if _, ok := mesh.VerifyClientPin(r); !ok { + callerUUID, ok := mesh.VerifyClientPin(r) + if !ok { http.Error(w, "forbidden: not a pinned cluster peer", http.StatusForbidden) return } switch r.Method { case http.MethodPost: - handleIngest(w, r, mgr) + handleIngest(w, r, mgr, callerUUID) case http.MethodGet: handleServeLocal(w, mgr) default: @@ -68,15 +74,16 @@ func newErrorsMux(mgr *Manager, mesh *clustertrust.Mesh) *http.ServeMux { return mux } -// handleIngest reconciles a peer's pushed SyncEnvelope. A malformed -// body or an envelope missing its nodeId is a 400 — we never let a -// peer reconcile our own origin (the manager rejects nodeId == -// localNodeID as a no-op, but we reject empty here so the client gets -// a clear signal rather than a silent accept). -func handleIngest(w http.ResponseWriter, r *http.Request, mgr *Manager) { +// handleIngest reconciles a peer's pushed SyncEnvelope. The envelope's nodeId +// is NOT trusted: the origin identity is the mTLS-authenticated caller UUID, +// which the pin store keys identically to the nodeId the push side uses. A +// mismatched body value is a 400 (caught by tests and honest peers), a +// malformed body is a 400, and the manager still rejects reconciling our own +// origin as a no-op. +func handleIngest(w http.ResponseWriter, r *http.Request, mgr *Manager, callerUUID string) { defer r.Body.Close() var env errors.SyncEnvelope - if err := json.NewDecoder(r.Body).Decode(&env); err != nil { + if err := json.NewDecoder(io.LimitReader(r.Body, maxIngestBodyBytes)).Decode(&env); err != nil { slog.Warn("ingest: bad request body", "err", err) http.Error(w, "invalid JSON body", http.StatusBadRequest) return @@ -85,9 +92,15 @@ func handleIngest(w http.ResponseWriter, r *http.Request, mgr *Manager) { http.Error(w, `"nodeId" is required`, http.StatusBadRequest) return } + if env.NodeID != callerUUID { + slog.Warn("ingest: envelope nodeId does not match the authenticated caller", + "caller", callerUUID) + http.Error(w, "nodeId does not match the authenticated caller", http.StatusBadRequest) + return + } - mgr.ReconcilePeer(env.NodeID, env.Errors) - slog.Debug("ingested peer snapshot", "nodeId", env.NodeID, "count", len(env.Errors)) + mgr.ReconcilePeer(callerUUID, env.Errors) + slog.Debug("ingested peer snapshot", "nodeId", callerUUID, "count", len(env.Errors)) w.WriteHeader(http.StatusNoContent) } diff --git a/services/nvpair-errors/manager.go b/services/nvpair-errors/manager.go index 8d4f37e0..c1a7be4a 100644 --- a/services/nvpair-errors/manager.go +++ b/services/nvpair-errors/manager.go @@ -6,6 +6,7 @@ package main import ( "context" "encoding/json" + stderrors "errors" "fmt" "io" "log" @@ -177,7 +178,15 @@ func (m *Manager) readLoop(ctx context.Context) error { return nil } log.Printf("JSON-RPC read error: %v", err) - continue + var de *DecodeError + if stderrors.As(err, &de) { + log.Printf("JSON-RPC decode error (skipping frame): %v", err) + continue + } + // Terminal transport/scanner error (e.g. an over-long frame — + // bufio.Scanner cannot resync) — stop instead of spinning. + log.Printf("JSON-RPC read error (terminal): %v", err) + return err } m.handleMessage(msg) if ctx.Err() != nil { diff --git a/services/nvpair-errors/peersync_test.go b/services/nvpair-errors/peersync_test.go index 6dddbdf6..5bb2f718 100644 --- a/services/nvpair-errors/peersync_test.go +++ b/services/nvpair-errors/peersync_test.go @@ -156,14 +156,16 @@ func TestEvictNodeRemovesPeerEntries(t *testing.T) { } // TestHTTPIngestReconciles: the POST /v1/errors handler decodes a -// SyncEnvelope and merges it, returning 204. +// SyncEnvelope and merges it, returning 204. The envelope's nodeId must +// match the mTLS-authenticated caller UUID — the pin store keys peers by +// the same UUID the push side uses as nodeId. func TestHTTPIngestReconciles(t *testing.T) { m := managerForNode("node-a") srv, client := servePinnedErrorsMux(t, m) env := errors.SyncEnvelope{ - NodeID: "node-b", - Errors: []ServiceError{localErr("b:one", "node-b", 1000)}, + NodeID: "uuid-peer", + Errors: []ServiceError{localErr("b:one", "uuid-peer", 1000)}, } body, _ := json.Marshal(env) resp, err := client.Post(srv.URL+"/v1/errors", "application/json", bytes.NewReader(body)) @@ -176,8 +178,34 @@ func TestHTTPIngestReconciles(t *testing.T) { } got := m.snapshot() - if len(got) != 1 || got[0].ID != "b:one" || got[0].NodeID != "node-b" { - t.Fatalf("after ingest, snapshot = %+v, want single node-b entry", got) + if len(got) != 1 || got[0].ID != "b:one" || got[0].NodeID != "uuid-peer" { + t.Fatalf("after ingest, snapshot = %+v, want single uuid-peer entry", got) + } +} + +// TestHTTPIngestRejectsSpoofedNodeID: a pinned caller cannot reconcile +// under another node's identity — the envelope's nodeId must equal the +// authenticated caller UUID, otherwise the push is a 400 and nothing is +// reconciled. +func TestHTTPIngestRejectsSpoofedNodeID(t *testing.T) { + m := managerForNode("node-a") + srv, client := servePinnedErrorsMux(t, m) + + env := errors.SyncEnvelope{ + NodeID: "uuid-victim", + Errors: []ServiceError{localErr("v:one", "uuid-victim", 1000)}, + } + body, _ := json.Marshal(env) + resp, err := client.Post(srv.URL+"/v1/errors", "application/json", bytes.NewReader(body)) + if err != nil { + t.Fatalf("POST: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("POST status = %d, want 400", resp.StatusCode) + } + if got := m.snapshot(); len(got) != 0 { + t.Fatalf("spoofed ingest reconciled %+v, want empty snapshot", got) } } diff --git a/services/nvpair-job-scheduler/codec.go b/services/nvpair-job-scheduler/codec.go index 16601231..aaf12fef 100644 --- a/services/nvpair-job-scheduler/codec.go +++ b/services/nvpair-job-scheduler/codec.go @@ -10,9 +10,10 @@ package main import "nvpair-shared/jsonrpc" type ( - Message = jsonrpc.Message - RPCError = jsonrpc.RPCError - Codec = jsonrpc.Codec + Message = jsonrpc.Message + RPCError = jsonrpc.RPCError + DecodeError = jsonrpc.DecodeError + Codec = jsonrpc.Codec ) var NewCodec = jsonrpc.NewCodec diff --git a/services/nvpair-job-scheduler/manager.go b/services/nvpair-job-scheduler/manager.go index bdd3f366..f5648363 100644 --- a/services/nvpair-job-scheduler/manager.go +++ b/services/nvpair-job-scheduler/manager.go @@ -6,6 +6,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "io" "log" @@ -99,7 +100,15 @@ func (m *Manager) readLoop(ctx context.Context) error { return nil } log.Printf("JSON-RPC read error: %v", err) - continue + var de *DecodeError + if errors.As(err, &de) { + log.Printf("JSON-RPC decode error (skipping frame): %v", err) + continue + } + // Terminal transport/scanner error (e.g. an over-long frame — + // bufio.Scanner cannot resync) — stop instead of spinning. + log.Printf("JSON-RPC read error (terminal): %v", err) + return err } m.handleMessage(msg) if ctx.Err() != nil { diff --git a/services/nvpair-manual-nodes/codec.go b/services/nvpair-manual-nodes/codec.go index 5d6fd177..1691b8cd 100644 --- a/services/nvpair-manual-nodes/codec.go +++ b/services/nvpair-manual-nodes/codec.go @@ -10,9 +10,10 @@ package main import "nvpair-shared/jsonrpc" type ( - Message = jsonrpc.Message - RPCError = jsonrpc.RPCError - Codec = jsonrpc.Codec + Message = jsonrpc.Message + RPCError = jsonrpc.RPCError + DecodeError = jsonrpc.DecodeError + Codec = jsonrpc.Codec ) var NewCodec = jsonrpc.NewCodec diff --git a/services/nvpair-manual-nodes/manager.go b/services/nvpair-manual-nodes/manager.go index 55a4040a..fb7842af 100644 --- a/services/nvpair-manual-nodes/manager.go +++ b/services/nvpair-manual-nodes/manager.go @@ -6,6 +6,7 @@ package main import ( "context" "encoding/json" + stderrors "errors" "fmt" "io" "log" @@ -607,7 +608,15 @@ func (m *Manager) readLoop(ctx context.Context) error { return nil } log.Printf("JSON-RPC read error: %v", err) - continue + var de *DecodeError + if stderrors.As(err, &de) { + log.Printf("JSON-RPC decode error (skipping frame): %v", err) + continue + } + // Terminal transport/scanner error (e.g. an over-long frame — + // bufio.Scanner cannot resync) — stop instead of spinning. + log.Printf("JSON-RPC read error (terminal): %v", err) + return err } m.handleMessage(msg) if ctx.Err() != nil { diff --git a/services/nvpair-node-scanner/codec.go b/services/nvpair-node-scanner/codec.go index 5d6fd177..1691b8cd 100644 --- a/services/nvpair-node-scanner/codec.go +++ b/services/nvpair-node-scanner/codec.go @@ -10,9 +10,10 @@ package main import "nvpair-shared/jsonrpc" type ( - Message = jsonrpc.Message - RPCError = jsonrpc.RPCError - Codec = jsonrpc.Codec + Message = jsonrpc.Message + RPCError = jsonrpc.RPCError + DecodeError = jsonrpc.DecodeError + Codec = jsonrpc.Codec ) var NewCodec = jsonrpc.NewCodec diff --git a/services/nvpair-node-scanner/scanner.go b/services/nvpair-node-scanner/scanner.go index 485a0275..e28450c9 100644 --- a/services/nvpair-node-scanner/scanner.go +++ b/services/nvpair-node-scanner/scanner.go @@ -5,6 +5,7 @@ package main import ( "context" + "errors" "fmt" "io" "log" @@ -102,7 +103,15 @@ func (s *Scanner) readLoop(ctx context.Context) error { return nil } log.Printf("JSON-RPC read error: %v", err) - continue + var de *DecodeError + if errors.As(err, &de) { + log.Printf("JSON-RPC decode error (skipping frame): %v", err) + continue + } + // Terminal transport/scanner error (e.g. an over-long frame — + // bufio.Scanner cannot resync) — stop instead of spinning. + log.Printf("JSON-RPC read error (terminal): %v", err) + return err } s.handleMessage(msg) } diff --git a/services/nvpair-node-settings/codec.go b/services/nvpair-node-settings/codec.go index 5d6fd177..1691b8cd 100644 --- a/services/nvpair-node-settings/codec.go +++ b/services/nvpair-node-settings/codec.go @@ -10,9 +10,10 @@ package main import "nvpair-shared/jsonrpc" type ( - Message = jsonrpc.Message - RPCError = jsonrpc.RPCError - Codec = jsonrpc.Codec + Message = jsonrpc.Message + RPCError = jsonrpc.RPCError + DecodeError = jsonrpc.DecodeError + Codec = jsonrpc.Codec ) var NewCodec = jsonrpc.NewCodec diff --git a/services/nvpair-node-settings/manager.go b/services/nvpair-node-settings/manager.go index a30a2eae..b5c52a8f 100644 --- a/services/nvpair-node-settings/manager.go +++ b/services/nvpair-node-settings/manager.go @@ -284,7 +284,15 @@ func (m *Manager) readLoop(ctx context.Context) error { return nil } log.Printf("JSON-RPC read error: %v", err) - continue + var de *DecodeError + if errors.As(err, &de) { + log.Printf("JSON-RPC decode error (skipping frame): %v", err) + continue + } + // Terminal transport/scanner error (e.g. an over-long frame — + // bufio.Scanner cannot resync) — stop instead of spinning. + log.Printf("JSON-RPC read error (terminal): %v", err) + return err } m.handleMessage(msg) if ctx.Err() != nil { diff --git a/services/nvpair-tui/rpc/client.go b/services/nvpair-tui/rpc/client.go index d59ce8ad..d11dec7a 100644 --- a/services/nvpair-tui/rpc/client.go +++ b/services/nvpair-tui/rpc/client.go @@ -6,6 +6,7 @@ package rpc import ( "context" "encoding/json" + "errors" "fmt" "io" "strconv" @@ -68,9 +69,15 @@ func (c *Client) Run(ctx context.Context) error { if err == io.EOF { return nil } - // A single malformed line should not kill the session; the + // A single malformed frame should not kill the session; the // broker may emit a frame we don't model. Skip and continue. - continue + // Anything else (over-long frame, transport error) is terminal: + // bufio.Scanner cannot resync, so continuing would spin. + var de *DecodeError + if errors.As(err, &de) { + continue + } + return err } switch { case msg.IsResponse(): diff --git a/services/nvpair-tui/rpc/codec.go b/services/nvpair-tui/rpc/codec.go index 8d5e8c83..2c36977e 100644 --- a/services/nvpair-tui/rpc/codec.go +++ b/services/nvpair-tui/rpc/codec.go @@ -71,6 +71,14 @@ type Codec struct { wmu sync.Mutex } +// DecodeError marks a recoverable per-frame failure (bad JSON or wrong +// version): the stream is still positioned at the next line, so callers +// may skip the frame and continue reading. Mirrors nvpair-shared/jsonrpc. +type DecodeError struct{ Err error } + +func (e *DecodeError) Error() string { return e.Err.Error() } +func (e *DecodeError) Unwrap() error { return e.Err } + // NewCodec wraps a reader/writer pair (the broker's stdout/stdin) in a // framing codec. func NewCodec(r io.Reader, w io.Writer) *Codec { @@ -79,7 +87,9 @@ func NewCodec(r io.Reader, w io.Writer) *Codec { return &Codec{scanner: scanner, writer: w} } -// Read returns the next frame, or io.EOF when the stream closes. +// Read returns the next frame, or io.EOF when the stream closes. A +// malformed frame (bad JSON or wrong version) is returned as a recoverable +// *DecodeError; a terminal scanner/transport error is a plain error. func (c *Codec) Read() (*Message, error) { if !c.scanner.Scan() { if err := c.scanner.Err(); err != nil { @@ -89,10 +99,10 @@ func (c *Codec) Read() (*Message, error) { } var msg Message if err := json.Unmarshal(c.scanner.Bytes(), &msg); err != nil { - return nil, fmt.Errorf("invalid JSON-RPC message: %w", err) + return nil, &DecodeError{fmt.Errorf("invalid JSON-RPC message: %w", err)} } if msg.JSONRPC != "2.0" { - return nil, fmt.Errorf("unsupported JSON-RPC version: %q", msg.JSONRPC) + return nil, &DecodeError{fmt.Errorf("unsupported JSON-RPC version: %q", msg.JSONRPC)} } return &msg, nil } diff --git a/services/nvpair-ui-broker/broker.go b/services/nvpair-ui-broker/broker.go index 0d189578..16c432a0 100644 --- a/services/nvpair-ui-broker/broker.go +++ b/services/nvpair-ui-broker/broker.go @@ -6,6 +6,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "io" "log" @@ -20,7 +21,7 @@ import ( "nvpair-shared/appdir" "nvpair-shared/applog" "nvpair-shared/clustertrust" - "nvpair-shared/errors" + gerrors "nvpair-shared/errors" "nvpair-shared/nodeid" "nvpair-shared/noderec" "nvpair-shared/schedulerwire" @@ -161,6 +162,11 @@ type Broker struct { clusterMgrPath string schedulerPath string clusterDir string + // servicePorts overrides the fixed inter-node service ports (see + // resolveServicePorts). Defaults keep the spec'd values; NVPAIR_TEST + // environments set them to free ports so concurrent test runs and + // cohabiting dev machines never skip on fixed-port collisions. + servicePorts servicePorts // Managed-port state is prepared before proxy startup and read by the proxy // supervisor/reader goroutines. Ollama commits its pending backend move after // its proxy reserves :11434; LM Studio moves through engine-manager first, @@ -178,7 +184,7 @@ type Broker struct { ollamaProxyGeneration uint64 // guarded by ollamaHostAliasMu ollamaHostAliasSyncMu sync.Mutex ollamaHostAliasErrorMu sync.Mutex - ollamaHostAliasError *errors.ServiceError + ollamaHostAliasError *gerrors.ServiceError ollamaPortReady chan struct{} ollamaPortReadyOnce sync.Once managedLMStudioFacade atomic.Bool @@ -361,7 +367,7 @@ func NewBroker(codec *Codec, paths workerPaths) *Broker { // only. The same value is passed to nvpair-errors via --node-id so // its localNodeID stays in lockstep with what the broker stamps. nodeID := resolveLocalNodeID(paths.clusterDir) - return &Broker{ + b := &Broker{ codec: codec, startedAt: time.Now(), nodeID: nodeID, @@ -377,6 +383,7 @@ func NewBroker(codec *Codec, paths workerPaths) *Broker { clusterMgrPath: paths.clusterMgr, schedulerPath: paths.scheduler, clusterDir: paths.clusterDir, + servicePorts: resolveServicePorts(os.Getenv), store: newDiscoveryStore(), telemetry: newTelemetryCache(), relayDir: relay.NewDirectory(), @@ -387,6 +394,16 @@ func NewBroker(codec *Codec, paths workerPaths) *Broker { ollamaPortReady: make(chan struct{}), lmstudioPortReady: make(chan struct{}), } + // Seed explicit proxy listen ports from the env overrides so spawnProxy / + // spawnLMStudioProxy pass --port instead of letting the proxies fall back + // to their defaults (11435/1234), which a dev machine may already hold. + if b.servicePorts.OllamaProxy != 0 { + b.ollamaProxyStartupPort.Store(int32(b.servicePorts.OllamaProxy)) + } + if b.servicePorts.LMStudioProxy != 0 { + b.lmstudioProxyStartupPort.Store(int32(b.servicePorts.LMStudioProxy)) + } + return b } // registerService records a local service in the discovery registration cache @@ -617,7 +634,8 @@ func (b *Broker) spawnNodeInfo() (supervisedHandle, error) { // /v1/node-info agrees with the identity the fleet keys on, even on a custom // --cluster-dir data root (node-info gets no cluster-dir, so it would // otherwise resolve the default root and could report a different UUID). - np, err := startNodeInfo(b.nodeInfoPath, applog.LevelString(), b.forwardNodeInfoNotification, "--node-id", b.nodeID) + np, err := startNodeInfo(b.nodeInfoPath, applog.LevelString(), b.forwardNodeInfoNotification, + "--port", fmt.Sprintf("%d", b.servicePorts.NodeInfo), "--node-id", b.nodeID) if err != nil { return nil, err } @@ -627,9 +645,10 @@ func (b *Broker) spawnNodeInfo() (supervisedHandle, error) { // only source. It runs on every spawn, which also covers a supervised restart. b.pushClusterIdentityToNodeInfo() // Register node-info's service so the daemon advertises ni= on _nvpair-node. - // node-info binds the fixed :14318 (force_ports is inert), so the broker - // knows its port. Idempotent across restarts. - b.registerService(noderec.RegisterParams{Service: noderec.ServiceNodeInfo, Port: nodeInfoHTTPPort}) + // node-info binds the port the broker passes via --port (the spec default + // :14318 or the servicePorts override), so the broker knows it. Idempotent + // across restarts. + b.registerService(noderec.RegisterParams{Service: noderec.ServiceNodeInfo, Port: b.servicePorts.NodeInfo}) slog.Info("node-info started", "path", b.nodeInfoPath, "pid", np.cmd.Process.Pid) return np, nil } @@ -672,12 +691,13 @@ func (b *Broker) spawnProxy() (supervisedHandle, error) { } func (b *Broker) spawnWorkloadManager() (supervisedHandle, error) { - wm, err := startWorkloadManager(b.workloadMgrPath, applog.LevelString(), b.relayDir, b.forwardWorkloadManagerNotification, b.clusterDirArgs()...) + wmArgs := append([]string{"--port", fmt.Sprintf("%d", b.servicePorts.Workload)}, b.clusterDirArgs()...) + wm, err := startWorkloadManager(b.workloadMgrPath, applog.LevelString(), b.relayDir, b.forwardWorkloadManagerNotification, wmArgs...) if err != nil { return nil, err } b.setWorkloadMgr(wm) - b.registerService(noderec.RegisterParams{Service: noderec.ServiceWorkload, Port: workloadHTTPPort}) + b.registerService(noderec.RegisterParams{Service: noderec.ServiceWorkload, Port: b.servicePorts.Workload}) // A (re)started manager's anti-entropy set starts empty. Replay this node's // active local-origin workloads so a supervised restart mid-job doesn't // leave a later-joining peer unable to learn those jobs (and the heartbeat @@ -762,14 +782,14 @@ func (b *Broker) spawnErrors() (supervisedHandle, error) { // Pass the resolved node UUID so nvpair-errors attributes this node's errors // to the same identity the broker stamps (and the cluster keys on), not its // hostname — keeping local-origin attribution and clear-by-id in lockstep. - extra := append(b.clusterDirArgs(), "--node-id", b.nodeID) + extra := append(b.clusterDirArgs(), "--port", fmt.Sprintf("%d", b.servicePorts.Errors), "--node-id", b.nodeID) ep, err := startErrors(b.errorsPath, applog.LevelString(), b.relayDir, b.onErrorsUpdate, extra...) if err != nil { return nil, err } b.setErrors(ep) b.replayOllamaHostAliasError() - b.registerService(noderec.RegisterParams{Service: noderec.ServiceErrors, Port: errorsHTTPPort}) + b.registerService(noderec.RegisterParams{Service: noderec.ServiceErrors, Port: b.servicePorts.Errors}) slog.Info("nvpair-errors started", "path", b.errorsPath, "pid", ep.cmd.Process.Pid) return ep, nil } @@ -783,8 +803,8 @@ func (b *Broker) spawnEngineMgr() (supervisedHandle, error) { // identity is resolved per handshake from the live cluster dir), so a join or // leave needs no restart here. args := append(b.logLevelArgs(), - "--http-port", fmt.Sprintf("%d", engineManagerHTTPPort), - "--control-port", fmt.Sprintf("%d", engineControlPort)) + "--http-port", fmt.Sprintf("%d", b.servicePorts.EngineHTTP), + "--control-port", fmt.Sprintf("%d", b.servicePorts.EngineControl)) if aliasPort := b.currentOllamaHostAlias().Port; aliasPort > 0 { args = append(args, "--reserved-port", fmt.Sprintf("%d", aliasPort)) } @@ -807,7 +827,7 @@ func (b *Broker) spawnEngineMgr() (supervisedHandle, error) { // Register engine-manager's HTTP surface so the daemon advertises em= on this // node's _nvpair-node record; peers enrich models from it. Fixed port (like // node-info's ni=), replayed across scanner restarts by regCache. - b.registerService(noderec.RegisterParams{Service: noderec.ServiceEngineManager, Port: engineManagerHTTPPort}) + b.registerService(noderec.RegisterParams{Service: noderec.ServiceEngineManager, Port: b.servicePorts.EngineHTTP}) // Advertise the ec remote-control surface too, whenever a cluster dir is // configured — which is exactly when engine-manager binds it. The port is // bound for the life of the process and admits callers by live membership @@ -816,9 +836,9 @@ func (b *Broker) spawnEngineMgr() (supervisedHandle, error) { // also why no re-registration is needed on a membership change: nothing about // the advertised address depends on whether this node is currently a member. if b.clusterDir != "" { - b.registerService(noderec.RegisterParams{Service: noderec.ServiceEngineControl, Port: engineControlPort}) + b.registerService(noderec.RegisterParams{Service: noderec.ServiceEngineControl, Port: b.servicePorts.EngineControl}) } - slog.Info("engine-manager started", "path", b.engineMgrPath, "pid", w.cmd.Process.Pid, "httpPort", engineManagerHTTPPort, "controlPort", engineControlPort) + slog.Info("engine-manager started", "path", b.engineMgrPath, "pid", w.cmd.Process.Pid, "httpPort", b.servicePorts.EngineHTTP, "controlPort", b.servicePorts.EngineControl) return w, nil } @@ -893,12 +913,13 @@ func (b *Broker) spawnClusterManager() (supervisedHandle, error) { // its parent is the base — the same derivation resolveLocalNodeID performs. // Passing it keeps the only writer of the cluster dir and the workers reading it // on one directory. - cm, err := startClusterManager(b.clusterMgrPath, applog.LevelString(), b.clusterManagerConfigDir(), b.relayDir, b.forwardClusterManagerNotification) + cm, err := startClusterManager(b.clusterMgrPath, applog.LevelString(), b.clusterManagerConfigDir(), b.relayDir, b.forwardClusterManagerNotification, + "--port", fmt.Sprintf("%d", b.servicePorts.ClusterManager)) if err != nil { return nil, err } b.setClusterMgr(cm) - b.registerService(noderec.RegisterParams{Service: noderec.ServiceCluster, Port: clusterManagerHTTPPort}) + b.registerService(noderec.RegisterParams{Service: noderec.ServiceCluster, Port: b.servicePorts.ClusterManager}) // Reflect the persisted clusterId (owned by nvpair-node-settings) back into the // cluster-manager, which doesn't persist it itself. Synchronous so the first // spawn restores before app:ready/readLoop (no client-visible race); also @@ -1614,6 +1635,18 @@ func (b *Broker) runWorkloadHistoryFlusher(ctx context.Context) func() { } } +// errTerminalRead reports that the client stdin read loop ended on a +// non-recoverable scanner/transport error (e.g. an over-long frame that +// bufio.Scanner cannot resync past), distinct from a clean EOF. +var errTerminalRead = errors.New("terminal read error") + +// messageDispatchConcurrency is the size of the broker's inbound dispatch +// pool: enough worker goroutines that one slow synchronous worker relay +// (bounded by rpcWorkerCallTimeout) cannot head-of-line block the rest of +// the control plane, few enough that handlers stay effectively serialized +// under normal traffic. +const messageDispatchConcurrency = 4 + func (b *Broker) Serve(ctx context.Context) error { ctx, cancel := context.WithCancel(ctx) b.cancel = cancel @@ -1963,7 +1996,7 @@ func (b *Broker) forwardProxyNotificationForGeneration(generation uint64, method // receipt. Release the candidate reservation before forwarding the sticky // warning so an existing Ollama owner can be adopted normally. if method == methodErrorsReport { - var report errors.ServiceError + var report gerrors.ServiceError if json.Unmarshal(params, &report) == nil && report.ID == ollamaHostAliasBlockedID { if !b.releaseOllamaHostAliasReservationForGeneration(generation) { return @@ -2184,14 +2217,48 @@ func (b *Broker) readLoop(ctx context.Context) error { case <-ctx.Done(): return } - // EOF is terminal (stream closed); other errors are per-line - // (e.g. a bad JSON frame) and the next Read advances past them. - if err == io.EOF { - return + // A decoded message (err nil) and a recoverable decode error + // (bad frame; the next Read advances past it) both keep the pump + // running. EOF is terminal (stream closed), and any other error + // is a terminal scanner/transport error: stop feeding the + // channel so the consumer exits instead of spinning. + if err == nil { + continue + } + var de *DecodeError + if errors.As(err, &de) { + continue } + return } }() + // Bounded dispatch pool: handleMessage runs synchronous worker relays + // (proxy/cluster/settings/manual-nodes, each bounded by + // rpcWorkerCallTimeout) so dispatching on the read loop would let one + // slow worker stall every other client request for up to 5s. A small + // worker pool decouples them. Cross-request ordering is preserved for + // the channels that need it by dedicated mutexes inside the handlers + // (workloadEmitMu serializes workload apply→fan→emit; subscription + // bookkeeping is per-state mutexed), and JSON-RPC has no cross-request + // response-ordering guarantee — each response carries its own id. The + // codec's write mutex keeps concurrent responses from interleaving. + dispatch := make(chan *Message) + var dispatchWG sync.WaitGroup + for range messageDispatchConcurrency { + dispatchWG.Add(1) + go func() { + defer dispatchWG.Done() + for msg := range dispatch { + b.handleMessage(msg) + } + }() + } + defer func() { + close(dispatch) + dispatchWG.Wait() + }() + for { select { case <-ctx.Done(): @@ -2201,10 +2268,16 @@ func (b *Broker) readLoop(ctx context.Context) error { if r.err == io.EOF || ctx.Err() != nil { return nil } - slog.Warn("JSON-RPC read error", "err", r.err) - continue + // Terminal scanner/transport error — the producer goroutine + // has already stopped; exit instead of spinning. + slog.Warn("JSON-RPC read error (terminal)", "err", r.err) + return errTerminalRead + } + select { + case dispatch <- r.msg: + case <-ctx.Done(): + return nil } - b.handleMessage(r.msg) if ctx.Err() != nil { return nil } diff --git a/services/nvpair-ui-broker/clustermanager.go b/services/nvpair-ui-broker/clustermanager.go index 1a67aa9c..5d366b9a 100644 --- a/services/nvpair-ui-broker/clustermanager.go +++ b/services/nvpair-ui-broker/clustermanager.go @@ -70,8 +70,8 @@ func (c *clusterManagerProcess) Done() <-chan struct{} { return c.done } // workers, the node would pair successfully into a directory nothing reads — // healthy roster, no cluster traffic. An empty configDir leaves the manager on its // own default (standalone invocation). -func startClusterManager(binaryPath, logLevel, configDir string, relayDir *relay.Directory, onNotify func(method string, params json.RawMessage)) (*clusterManagerProcess, error) { - args := []string{"--log-level", logLevel} +func startClusterManager(binaryPath, logLevel, configDir string, relayDir *relay.Directory, onNotify func(method string, params json.RawMessage), extraArgs ...string) (*clusterManagerProcess, error) { + args := append([]string{"--log-level", logLevel}, extraArgs...) if configDir != "" { args = append(args, "--config-dir", configDir) } diff --git a/services/nvpair-ui-broker/codec.go b/services/nvpair-ui-broker/codec.go index 4d7dcc80..7815d286 100644 --- a/services/nvpair-ui-broker/codec.go +++ b/services/nvpair-ui-broker/codec.go @@ -16,10 +16,11 @@ import ( ) type ( - Message = jsonrpc.Message - RPCError = jsonrpc.RPCError - Codec = jsonrpc.Codec - Peer = jsonrpc.Peer + Message = jsonrpc.Message + RPCError = jsonrpc.RPCError + DecodeError = jsonrpc.DecodeError + Codec = jsonrpc.Codec + Peer = jsonrpc.Peer ) var ( diff --git a/services/nvpair-ui-broker/lmstudioport.go b/services/nvpair-ui-broker/lmstudioport.go index a43509c1..7a8dab46 100644 --- a/services/nvpair-ui-broker/lmstudioport.go +++ b/services/nvpair-ui-broker/lmstudioport.go @@ -125,6 +125,11 @@ func (b *Broker) reportLMStudioPortOwnershipBlocked(reason string) { } func (b *Broker) setLMStudioProxyFallback(excludedPorts ...int) int { + // An explicit env override is authoritative (see setOllamaProxyFallback). + if p := b.servicePorts.LMStudioProxy; p != 0 { + b.lmstudioProxyStartupPort.Store(int32(p)) + return p + } if aliasPort := b.currentOllamaHostAlias().Port; aliasPort > 0 { excludedPorts = append(excludedPorts, aliasPort) } diff --git a/services/nvpair-ui-broker/ollamahost.go b/services/nvpair-ui-broker/ollamahost.go index 68a5d4c5..905164f6 100644 --- a/services/nvpair-ui-broker/ollamahost.go +++ b/services/nvpair-ui-broker/ollamahost.go @@ -23,6 +23,8 @@ const ( // These ports are fixed by the broker-owned service topology. Keep the // values here as the broker's single source and use them when registering // the corresponding workers as well as when reserving an OLLAMA_HOST alias. + // Tests override them per-broker (servicePorts) so two brokers — or a + // broker and a dev machine's real one — never fight over a fixed port. nodeInfoHTTPPort = 14318 errorsHTTPPort = 14319 workloadHTTPPort = 14320 @@ -31,6 +33,59 @@ const ( lmstudioProxyPortFile = "lmstudio-proxy-port.json" ) +// servicePorts is the resolved set of broker-owned service ports. The zero +// value means "use the spec defaults". +type servicePorts struct { + NodeInfo int + Errors int + Workload int + ClusterManager int + EngineHTTP int + EngineControl int + // Proxy listens. Zero keeps the proxy's own default/persisted port; tests + // set explicit free ports so spawn never fights a dev process on + // 11435/1234. + OllamaProxy int + LMStudioProxy int +} + +// resolveServicePorts reads the NVPAIR_SERVICE_*_PORT env overrides (set by +// cross-process tests) over the spec defaults. A value outside 1–65535 is +// ignored with a warning rather than trusted. Keeping this in the broker (not +// the workers) means each spawned worker gets an explicit --port, so the +// override is authoritative no matter what the worker's own default is. +func resolveServicePorts(getenv func(string) string) servicePorts { + defaults := servicePorts{ + NodeInfo: nodeInfoHTTPPort, + Errors: errorsHTTPPort, + Workload: workloadHTTPPort, + ClusterManager: clusterManagerHTTPPort, + EngineHTTP: engineManagerHTTPPort, + EngineControl: engineControlPort, + } + over := func(name string, target *int) { + v := getenv(name) + if v == "" { + return + } + n, err := strconv.Atoi(v) + if err != nil || n < 1 || n > 65535 { + slog.Warn("ignoring invalid service port override", "env", name, "value", v) + return + } + *target = n + } + over("NVPAIR_SERVICE_NODE_INFO_PORT", &defaults.NodeInfo) + over("NVPAIR_SERVICE_ERRORS_PORT", &defaults.Errors) + over("NVPAIR_SERVICE_WORKLOAD_PORT", &defaults.Workload) + over("NVPAIR_SERVICE_CLUSTER_MANAGER_PORT", &defaults.ClusterManager) + over("NVPAIR_SERVICE_ENGINE_HTTP_PORT", &defaults.EngineHTTP) + over("NVPAIR_SERVICE_ENGINE_CONTROL_PORT", &defaults.EngineControl) + over("NVPAIR_SERVICE_OLLAMA_PROXY_PORT", &defaults.OllamaProxy) + over("NVPAIR_SERVICE_LMSTUDIO_PROXY_PORT", &defaults.LMStudioProxy) + return defaults +} + type ollamaHostAlias struct { Address string AlternateAddress string @@ -186,7 +241,7 @@ func (b *Broker) prepareOllamaHostAlias(enabled bool, backendPort int) { if backendPort > 0 { enginePorts[backendPort] = "ollama" } - if reason := reservedOllamaHostAliasPort(alias.Port, enginePorts, configuredLMStudioProxyPort()); reason != "" { + if reason := b.reservedOllamaHostAliasPort(alias.Port, enginePorts, configuredLMStudioProxyPort()); reason != "" { b.reportOllamaHostAliasBlocked(alias.displayAddress(), reason) return } @@ -344,7 +399,7 @@ func configuredLMStudioProxyPort() int { return stored.Port } -func reservedOllamaHostAliasPort(port int, enginePorts map[int]string, lmstudioProxy int) string { +func (b *Broker) reservedOllamaHostAliasPort(port int, enginePorts map[int]string, lmstudioProxy int) string { if engine, ok := enginePorts[port]; ok { if engine == "" { engine = "an engine" @@ -360,17 +415,17 @@ func reservedOllamaHostAliasPort(port int, enginePorts map[int]string, lmstudioP // Managed LM Studio ownership is prepared after this check and moves a // colliding backend here, so the alias must not be sitting on it. return fmt.Sprintf("the managed LM Studio backend uses port %d", port) - case nodeInfoHTTPPort: + case b.servicePorts.NodeInfo: return fmt.Sprintf("the node-info service uses port %d", port) - case errorsHTTPPort: + case b.servicePorts.Errors: return fmt.Sprintf("the errors service uses port %d", port) - case workloadHTTPPort: + case b.servicePorts.Workload: return fmt.Sprintf("the workload service uses port %d", port) - case clusterManagerHTTPPort: + case b.servicePorts.ClusterManager: return fmt.Sprintf("the cluster manager uses port %d", port) - case engineManagerHTTPPort: + case b.servicePorts.EngineHTTP: return fmt.Sprintf("the engine model service uses port %d", port) - case engineControlPort: + case b.servicePorts.EngineControl: return fmt.Sprintf("the engine control service uses port %d", port) } return "" diff --git a/services/nvpair-ui-broker/ollamahost_test.go b/services/nvpair-ui-broker/ollamahost_test.go index c9a732a8..6ae14ed5 100644 --- a/services/nvpair-ui-broker/ollamahost_test.go +++ b/services/nvpair-ui-broker/ollamahost_test.go @@ -114,7 +114,7 @@ func TestReservedOllamaHostAliasPort(t *testing.T) { if tc.lmstudioBackend > 0 { enginePorts[tc.lmstudioBackend] = "lmstudio" } - reason := reservedOllamaHostAliasPort(tc.port, enginePorts, tc.lmstudioProxy) + reason := (&Broker{servicePorts: resolveServicePorts(func(string) string { return "" })}).reservedOllamaHostAliasPort(tc.port, enginePorts, tc.lmstudioProxy) if tc.wantReasonSubstr == "" && reason != "" { t.Fatalf("reason = %q, want none", reason) } diff --git a/services/nvpair-ui-broker/proxyport.go b/services/nvpair-ui-broker/proxyport.go index 63eddc79..7545d45a 100644 --- a/services/nvpair-ui-broker/proxyport.go +++ b/services/nvpair-ui-broker/proxyport.go @@ -137,6 +137,13 @@ func (b *Broker) markOllamaPortReady() { } func (b *Broker) setOllamaProxyFallback(excludedPorts ...int) int { + // An explicit env override is authoritative: tests pin the port so spawn + // never fights a cohabiting process, and the fallback probe must not + // stomp it. + if p := b.servicePorts.OllamaProxy; p != 0 { + b.ollamaProxyStartupPort.Store(int32(p)) + return p + } if aliasPort := b.currentOllamaHostAlias().Port; aliasPort > 0 { excludedPorts = append(excludedPorts, aliasPort) } diff --git a/services/nvpair-ui-broker/relay/relay.go b/services/nvpair-ui-broker/relay/relay.go index ed02240c..2668d0c4 100644 --- a/services/nvpair-ui-broker/relay/relay.go +++ b/services/nvpair-ui-broker/relay/relay.go @@ -85,21 +85,26 @@ func registerEqual(a, b noderec.RegisterParams) bool { } // Subscriber is a client interested in directory changes: a service filter and a -// callback invoked (on the caller's goroutine, under no relay lock) with the -// subscriber's full filtered node set on every change. Consumers replace their -// set from it rather than applying deltas, so a dropped or reordered push can't -// leave them drifted — every push is the authoritative current list. +// callback invoked with the subscriber's full filtered node set on every change. +// Consumers replace their set from it rather than applying deltas, so a dropped +// or reordered push can't leave them drifted — every push is the authoritative +// current list. +// +// Deliveries are asynchronous: each subscriber owns a pump goroutine (started +// by Directory.Subscribe) that serializes its sends and coalesces concurrent +// triggers into one delivery that captures the snapshot at send time. A slow or +// blocked Send therefore stalls only its own subscriber, never the directory +// update path that feeds it (the scanner's read pump calls Apply). type Subscriber struct { Filter noderec.SubscribeParams Send func(nodes []noderec.DirectoryNode) - // sendMu serializes deliveries to this subscriber so two concurrent - // deliveries — the initial post-subscribe delivery racing an Apply fan-out - // driven by the scanner read-pump — can't reorder and leave the subscriber - // holding an older set than a newer one. Combined with capturing the snapshot - // inside Deliver (at send time, not subscribe time), the last delivery to - // acquire it always carries the latest directory state. - sendMu sync.Mutex + // kick carries a pending-delivery signal (capacity 1: extra signals while + // one is already pending coalesce — the pump captures the latest snapshot + // when it wakes, so early triggers can't deliver stale state). done closes + // on Unsubscribe and stops the pump. + kick chan struct{} + done chan struct{} } // Directory is the broker's view of all LAN nodes (keyed by hostUuid) plus its @@ -119,32 +124,56 @@ func NewDirectory() *Directory { } } -// Subscribe registers a subscriber and returns its id. The caller sends the -// initial snapshot via Deliver after releasing its own lock — Deliver captures -// the snapshot at send time, so a concurrent Apply can't sneak a newer snapshot -// in and have this initial delivery overwrite it with an older one. +// Subscribe registers a subscriber and starts its delivery pump goroutine. The +// initial snapshot arrives via the pump after any pending Deliver call — +// snapshot is captured at send time, so a concurrent Apply can't sneak a newer +// snapshot in and have this initial delivery overwrite it with an older one. func (d *Directory) Subscribe(sub *Subscriber) (id int) { + sub.kick = make(chan struct{}, 1) + sub.done = make(chan struct{}) d.mu.Lock() d.nextID++ id = d.nextID d.subs[id] = sub d.mu.Unlock() + go d.pump(sub) return id } -// Deliver pushes the subscriber its current filtered snapshot, serialized -// per-subscriber. Capturing the snapshot here (at delivery time) rather than -// handing Send a pre-captured slice means a delivery can never carry a set older -// than the directory's state when it actually runs; the per-subscriber lock then -// guarantees the initial post-subscribe delivery and a concurrent Apply fan-out -// settle on the latest set regardless of which runs last. +// pump serializes one subscriber's deliveries. Every wake re-captures the +// latest filtered snapshot, so coalesced triggers always deliver current state. +// Exits on Unsubscribe (done closed). +func (d *Directory) pump(sub *Subscriber) { + for { + select { + case <-sub.kick: + sub.Send(d.filtered(sub.Filter)) + case <-sub.done: + return + } + } +} + +// Deliver asks for a delivery of the subscriber's current filtered snapshot. +// Non-blocking: it schedules the send on the subscriber's pump and never blocks +// the caller — Apply runs on the scanner read pump, and a subscriber whose Send +// blocks (a stalled worker's stdin pipe) must not stall the directory or the +// other subscribers. Multiple pending triggers coalesce into one send of the +// latest state. func (d *Directory) Deliver(sub *Subscriber) { - sub.sendMu.Lock() - defer sub.sendMu.Unlock() + select { + case sub.kick <- struct{}{}: + default: + } +} + +// filtered returns the nodes matching a subscriber's filter, sorted by +// hostUuid for a deterministic set. +func (d *Directory) filtered(f noderec.SubscribeParams) []noderec.DirectoryNode { d.mu.Lock() - nodes := d.filteredLocked(sub.Filter) + nodes := d.filteredLocked(f) d.mu.Unlock() - sub.Send(nodes) + return nodes } // filteredLocked returns the nodes matching a subscriber's filter, sorted by @@ -160,11 +189,16 @@ func (d *Directory) filteredLocked(f noderec.SubscribeParams) []noderec.Director return out } -// Unsubscribe removes a subscriber. +// Unsubscribe removes a subscriber and stops its delivery pump, waiting for +// the pump to exit so a Send cannot run against a consumer that's gone. func (d *Directory) Unsubscribe(id int) { d.mu.Lock() + sub := d.subs[id] delete(d.subs, id) d.mu.Unlock() + if sub != nil { + close(sub.done) + } } // Apply folds a daemon node-* delta into the directory, then re-sends every diff --git a/services/nvpair-ui-broker/relay/relay_test.go b/services/nvpair-ui-broker/relay/relay_test.go index bdad98ef..e444c69d 100644 --- a/services/nvpair-ui-broker/relay/relay_test.go +++ b/services/nvpair-ui-broker/relay/relay_test.go @@ -5,6 +5,8 @@ package relay import ( "reflect" + "sync" + "time" "testing" "nvpair-shared/noderec" @@ -53,17 +55,40 @@ func TestRegistrationCache(t *testing.T) { // full filtered snapshot; snaps holds them in order so a test can assert on the // latest set and on how many pushes arrived. type recordingSub struct { + mu sync.Mutex snaps [][]noderec.DirectoryNode } func (r *recordingSub) send(nodes []noderec.DirectoryNode) { + r.mu.Lock() + defer r.mu.Unlock() r.snaps = append(r.snaps, append([]noderec.DirectoryNode(nil), nodes...)) } -func (r *recordingSub) last() []noderec.DirectoryNode { - if len(r.snaps) == 0 { - return nil - } +// count returns the number of deliveries received so far. +func (r *recordingSub) count() int { + r.mu.Lock() + defer r.mu.Unlock() + return len(r.snaps) +} + +// last returns the latest snapshot, waiting up to 2s for at least want +// deliveries (deliveries are asynchronous: the pump goroutine sends them). +func (r *recordingSub) last(want int) []noderec.DirectoryNode { + deadline := time.Now().Add(2 * time.Second) + for r.count() < want { + if time.Now().After(deadline) { + r.mu.Lock() + defer r.mu.Unlock() + if len(r.snaps) == 0 { + return nil + } + return r.snaps[len(r.snaps)-1] + } + time.Sleep(2 * time.Millisecond) + } + r.mu.Lock() + defer r.mu.Unlock() return r.snaps[len(r.snaps)-1] } @@ -96,7 +121,7 @@ func TestDirectorySubscribeInitialSnapshot(t *testing.T) { } d.Subscribe(sub) d.Deliver(sub) - if got := ids(rec.last()); !reflect.DeepEqual(got, []string{"a"}) { + if got := ids(rec.last(1)); !reflect.DeepEqual(got, []string{"a"}) { t.Fatalf("initial delivery = %v, want [a]", got) } } @@ -112,7 +137,7 @@ func TestDeliverCapturesAtSendTime(t *testing.T) { d.Subscribe(sub) d.Apply(noderec.NotifyNodeDiscovered, olNode("a")) d.Deliver(sub) - if got := ids(rec.last()); !reflect.DeepEqual(got, []string{"a"}) { + if got := ids(rec.last(1)); !reflect.DeepEqual(got, []string{"a"}) { t.Fatalf("delivery after a post-subscribe change = %v, want [a]", got) } } @@ -129,10 +154,10 @@ func TestDirectoryFanoutRespectsFilter(t *testing.T) { // Every change re-pushes each subscriber its full filtered snapshot, so the // latest snapshot is the authoritative filtered set. - if got := ids(olSub.last()); !reflect.DeepEqual(got, []string{"a"}) { + if got := ids(olSub.last(2)); !reflect.DeepEqual(got, []string{"a"}) { t.Errorf("ol subscriber last snapshot = %v, want [a]", got) } - if got := ids(allSub.last()); !reflect.DeepEqual(got, []string{"a", "b"}) { + if got := ids(allSub.last(2)); !reflect.DeepEqual(got, []string{"a", "b"}) { t.Errorf("all subscriber last snapshot = %v, want [a b]", got) } } @@ -148,16 +173,17 @@ func TestDirectoryRemoveAndUnsubscribe(t *testing.T) { t.Error("node should be gone after removed") } // The removal re-pushes an empty snapshot (the node is simply absent). - if got := sub.last(); len(got) != 0 { + if got := sub.last(2); len(got) != 0 { t.Errorf("subscriber last snapshot = %v, want empty after removal", ids(got)) } // After unsubscribe, no more pushes. - before := len(sub.snaps) + before := sub.count() d.Unsubscribe(id) d.Apply(noderec.NotifyNodeDiscovered, olNode("c")) - if len(sub.snaps) != before { - t.Errorf("unsubscribed sub still received %d pushes", len(sub.snaps)-before) + time.Sleep(50 * time.Millisecond) + if sub.count() != before { + t.Errorf("unsubscribed sub still received %d pushes", sub.count()-before) } } diff --git a/services/nvpair-ui-broker/relaysub.go b/services/nvpair-ui-broker/relaysub.go index f1ec0f0e..2bc48c7a 100644 --- a/services/nvpair-ui-broker/relaysub.go +++ b/services/nvpair-ui-broker/relaysub.go @@ -19,10 +19,10 @@ type relaySendFunc func(nodes []noderec.DirectoryNode) // subscribeRelay registers a worker as a relay.Directory subscriber for the // given discovery:subscribe params and returns the subscription id plus the // subscriber handle. The caller owns the id's lifetime (Unsubscribe on worker -// exit / re-subscribe) and must send the initial snapshot via dir.Deliver(sub) -// after releasing its own lock; Deliver captures the snapshot at send time, so -// the initial delivery can't be overtaken by a concurrent Apply and land a stale -// set. +// exit / re-subscribe) and must request the initial snapshot via +// dir.Deliver(sub); Deliver schedules the send on the subscriber's pump, which +// captures the snapshot at send time, so the initial delivery can't be +// overtaken by a concurrent Apply and land a stale set. func subscribeRelay(dir *relay.Directory, params json.RawMessage, send relaySendFunc) (int, *relay.Subscriber, error) { var sp noderec.SubscribeParams if err := json.Unmarshal(params, &sp); err != nil { diff --git a/services/nvpair-ui-broker/workloadstore/persistence.go b/services/nvpair-ui-broker/workloadstore/persistence.go index 24d217ca..fb73d10a 100644 --- a/services/nvpair-ui-broker/workloadstore/persistence.go +++ b/services/nvpair-ui-broker/workloadstore/persistence.go @@ -110,7 +110,13 @@ func (s *Store) Checkpoint() error { s.mu.Unlock() rotate(path, rotations) - return writeSnapshotFile(path, infos) + if err := writeSnapshotFile(path, infos); err != nil { + s.mu.Lock() + s.dirty = true + s.mu.Unlock() + return err + } + return nil } // Run drives the coalescing flusher until ctx is cancelled: a dirty flush every diff --git a/services/nvpair-workload-manager/codec.go b/services/nvpair-workload-manager/codec.go index 5d6fd177..1691b8cd 100644 --- a/services/nvpair-workload-manager/codec.go +++ b/services/nvpair-workload-manager/codec.go @@ -10,9 +10,10 @@ package main import "nvpair-shared/jsonrpc" type ( - Message = jsonrpc.Message - RPCError = jsonrpc.RPCError - Codec = jsonrpc.Codec + Message = jsonrpc.Message + RPCError = jsonrpc.RPCError + DecodeError = jsonrpc.DecodeError + Codec = jsonrpc.Codec ) var NewCodec = jsonrpc.NewCodec diff --git a/services/nvpair-workload-manager/manager.go b/services/nvpair-workload-manager/manager.go index d9895e56..099ad716 100644 --- a/services/nvpair-workload-manager/manager.go +++ b/services/nvpair-workload-manager/manager.go @@ -6,6 +6,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "io" "log" @@ -247,8 +248,15 @@ func (m *Manager) readLoop(ctx context.Context) error { m.cancel() return nil } - log.Printf("JSON-RPC read error: %v", err) - continue + var de *DecodeError + if errors.As(err, &de) { + log.Printf("JSON-RPC decode error (skipping frame): %v", err) + continue + } + // Terminal transport/scanner error (e.g. an over-long frame — + // bufio.Scanner cannot resync) — stop instead of spinning. + log.Printf("JSON-RPC read error (terminal): %v", err) + return err } m.handleMessage(msg) if ctx.Err() != nil { diff --git a/services/ollama-proxy/codec.go b/services/ollama-proxy/codec.go index 5d6fd177..1691b8cd 100644 --- a/services/ollama-proxy/codec.go +++ b/services/ollama-proxy/codec.go @@ -10,9 +10,10 @@ package main import "nvpair-shared/jsonrpc" type ( - Message = jsonrpc.Message - RPCError = jsonrpc.RPCError - Codec = jsonrpc.Codec + Message = jsonrpc.Message + RPCError = jsonrpc.RPCError + DecodeError = jsonrpc.DecodeError + Codec = jsonrpc.Codec ) var NewCodec = jsonrpc.NewCodec diff --git a/services/ollama-proxy/failover_test.go b/services/ollama-proxy/failover_test.go index 48784714..f12d9d6e 100644 --- a/services/ollama-proxy/failover_test.go +++ b/services/ollama-proxy/failover_test.go @@ -53,31 +53,51 @@ func nodeForModel(t *testing.T, id, serverURL, model string) Node { return node } -// TestHandlePlain_OptionsPreflight: a CORS preflight is answered locally with -// 204 + permissive headers and never forwarded. +// TestHandlePlain_OptionsPreflight: a preflight from an allowlisted origin is +// answered locally with 204 + the static grant and never forwarded. An +// unlisted origin's preflight falls through to the origin gate (403). func TestHandlePlain_OptionsPreflight(t *testing.T) { + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") p := testProxy(NewDiscovery(), 11434) rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodOptions, "/api/chat", nil) req.RemoteAddr = "127.0.0.1:40000" + req.Header.Set("Origin", "https://ui.example") req.Header.Set("Access-Control-Request-Headers", "X-Custom-Token") p.handlePlain(rec, req) if rec.Code != http.StatusNoContent { t.Fatalf("status = %d, want 204", rec.Code) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want *", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin", got) } if rec.Header().Get("Access-Control-Allow-Methods") == "" { t.Errorf("missing Access-Control-Allow-Methods") } - if got := rec.Header().Get("Access-Control-Expose-Headers"); got != "*" { - t.Errorf("Access-Control-Expose-Headers = %q, want *", got) + // Arbitrary request headers are no longer echoed: the static grant is + // deny-by-default, so X-Custom-Token must not clear preflight. + if got := rec.Header().Get("Access-Control-Allow-Headers"); got != "Content-Type, Authorization" { + t.Errorf("Access-Control-Allow-Headers = %q, want the static grant", got) } - // The browser's requested headers are echoed so an arbitrary header clears preflight. - if got := rec.Header().Get("Access-Control-Allow-Headers"); got != "X-Custom-Token" { - t.Errorf("Access-Control-Allow-Headers = %q, want echoed X-Custom-Token", got) +} + +// TestHandlePlain_OptionsPreflightUnlistedOrigin: a preflight from an origin +// the operator has not allowlisted gets no local grant; the origin gate answers +// 403 instead. +func TestHandlePlain_OptionsPreflightUnlistedOrigin(t *testing.T) { + p := testProxy(NewDiscovery(), 11434) + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/api/chat", nil) + req.RemoteAddr = "127.0.0.1:40000" + req.Header.Set("Origin", "https://evil.example") + p.handlePlain(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403", rec.Code) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want no grant", got) } } @@ -85,6 +105,7 @@ func TestHandlePlain_OptionsPreflight(t *testing.T) { // exact origin into credentialed CORS, its preflight policy reaches the browser // instead of being replaced by the proxy's uncredentialed wildcard fallback. func TestHandlePlain_EngineCredentialedPreflightPreserved(t *testing.T) { + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://app.example") preflightSeen := make(chan struct{}, 1) engine := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodOptions { @@ -170,15 +191,18 @@ func TestHandleHTTP_EngineCredentialsWithoutOriginDropped(t *testing.T) { disc := NewDiscovery() disc.AddManual(nodeForModel(t, "engine", engine.URL, "llama")) p := testProxy(disc, 11434) + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + req := httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"llama"}`))) + p.handleHTTP(rec, req) - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want the proxy's wildcard", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin", got) } if got := rec.Header().Get("Access-Control-Allow-Credentials"); got != "" { - t.Errorf("Access-Control-Allow-Credentials = %q, want cleared alongside the wildcard origin", got) + t.Errorf("Access-Control-Allow-Credentials = %q, want cleared (the engine declared no origin policy to preserve)", got) } } @@ -198,8 +222,11 @@ func TestHandleHTTP_HappyPathSingleNode(t *testing.T) { disc.AddManual(nodeForModel(t, "good", good.URL, "llama")) p := testProxy(disc, 11434) + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + req := httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"llama"}`))) + p.handleHTTP(rec, req) if rec.Code != http.StatusOK { t.Fatalf("status = %d, want 200", rec.Code) @@ -207,8 +234,8 @@ func TestHandleHTTP_HappyPathSingleNode(t *testing.T) { if gotBody != `{"model":"llama"}` { t.Errorf("node got body %q, want the original request body", gotBody) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want * on success", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin on success", got) } } @@ -247,15 +274,18 @@ func TestHandleHTTP_NoRetryOn400(t *testing.T) { // TestHandleHTTP_RejectionHasCORS: even the no-node rejection carries CORS so a // browser sees the real 502 instead of an opaque CORS error. func TestHandleHTTP_RejectionHasCORS(t *testing.T) { + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") p := testProxy(NewDiscovery(), 11434) + req := httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"x"}`)) + req.Header.Set("Origin", "https://ui.example") rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"x"}`))) + p.handleHTTP(rec, req) if rec.Code != http.StatusBadGateway { t.Fatalf("status = %d, want 502", rec.Code) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want * on rejection", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin on rejection", got) } } @@ -282,9 +312,12 @@ func TestHandleHTTP_FailoverOn503(t *testing.T) { disc.AddManual(nodeForModel(t, "good", good.URL, "llama")) p := testProxy(disc, 11434) p.SetSelected("busy") // deterministic: busy is tried first + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + req := httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"llama"}`))) + p.handleHTTP(rec, req) if rec.Code != http.StatusOK { t.Fatalf("status = %d, want 200 (should have failed over past the 503)", rec.Code) @@ -292,8 +325,8 @@ func TestHandleHTTP_FailoverOn503(t *testing.T) { if gotBody != `{"model":"llama"}` { t.Errorf("failover node got body %q, want the original request body", gotBody) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want * on proxied success", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin on proxied success", got) } } @@ -313,14 +346,17 @@ func TestHandleHTTP_AllNodesDownReturnsError(t *testing.T) { disc.AddManual(nb) p := testProxy(disc, 11434) + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + req := httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"llama"}`)) + req.Header.Set("Origin", "https://ui.example") rec := httptest.NewRecorder() - p.handleHTTP(rec, httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"llama"}`))) + p.handleHTTP(rec, req) if rec.Code != http.StatusBadGateway { t.Fatalf("status = %d, want 502 when all nodes are down", rec.Code) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want * on exhausted error", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the allowlisted origin on exhausted error", got) } } @@ -443,8 +479,8 @@ func TestHandleHTTP_AggregatesModelList(t *testing.T) { if got.Models[1].Digest != "first" { t.Errorf("duplicate metadata = %q, want deterministic first candidate", got.Models[1].Digest) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want *", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want no grant for an Origin-less caller", got) } if !events.has(`"method":"proxy/request-started"`) || !events.has(`"method":"proxy/request"`) || !events.has(`"target":"cluster"`) { t.Errorf("aggregate telemetry missing paired cluster events: %s", events.b) diff --git a/services/ollama-proxy/ingress.go b/services/ollama-proxy/ingress.go index 984b7b20..9844e582 100644 --- a/services/ollama-proxy/ingress.go +++ b/services/ollama-proxy/ingress.go @@ -71,14 +71,24 @@ func (p *Proxy) handlePlain(w http.ResponseWriter, r *http.Request) { } slog.Warn("rejected non-loopback plaintext request; cluster peers must use mTLS", "remote", r.RemoteAddr, "method", r.Method, "path", r.URL.Path) - writeIngressError(w, http.StatusForbidden, "loopback-only", + writeIngressError(w, r, http.StatusForbidden, "loopback-only", "plaintext requests are accepted only from loopback; cluster peers must use the mTLS ingress") return } + // Cross-origin browser gate: a loopback bind does not exclude browser + // pages (they connect from loopback), so any Origin this process's + // allowlist does not name is refused before it can drive an engine. + if !cors.AllowRequest(r) { + slog.Warn("rejected cross-origin browser request not on the allowlist", + "remote", r.RemoteAddr, "method", r.Method, "path", r.URL.Path, + "origin", r.Header.Get("Origin")) + cors.RejectOrigin(w) + return + } // Engine-manager marks its private identity/action requests so this // compatibility facade can never be mistaken for the local Ollama backend. if r.Header.Get(engineIdentityProbeHeader) == "1" { - writeIngressError(w, http.StatusConflict, "proxy-facade", "the compatibility facade is not an Ollama engine") + writeIngressError(w, r, http.StatusConflict, "proxy-facade", "the compatibility facade is not an Ollama engine") return } p.handleHTTP(w, r) @@ -99,13 +109,13 @@ func (p *Proxy) handleClusterIngress(w http.ResponseWriter, r *http.Request) { p.mesh.Refresh() peer, ok := p.mesh.VerifyClientPin(r) if !ok { - writeIngressError(w, http.StatusForbidden, "cluster-auth", + writeIngressError(w, r, http.StatusForbidden, "cluster-auth", "client certificate is not a pinned member of this node's cluster") return } target, ok := p.localBackendTarget() if !ok { - writeIngressError(w, http.StatusServiceUnavailable, "no-local-backend", + writeIngressError(w, r, http.StatusServiceUnavailable, "no-local-backend", "no local inference backend is available on this node") return } @@ -129,9 +139,9 @@ func (p *Proxy) newLocalReverseProxy(target *url.URL) *httputil.ReverseProxy { req.Host = target.Host }, Transport: p.plainHTTPTransport(), - ErrorHandler: func(ew http.ResponseWriter, _ *http.Request, err error) { + ErrorHandler: func(ew http.ResponseWriter, er *http.Request, err error) { slog.Warn("cluster ingress upstream error", "target", target.Host, "err", err) - writeIngressError(ew, http.StatusBadGateway, "backend-error", "local inference backend error") + writeIngressError(ew, er, http.StatusBadGateway, "backend-error", "local inference backend error") }, } } @@ -152,8 +162,8 @@ func isLoopbackRemote(remoteAddr string) bool { // request body or any generated output. CORS headers are included because these // are the proxy's own rejections: without them a browser client cannot read the // status or reason, and every one of them looks like a generic CORS failure. -func writeIngressError(w http.ResponseWriter, status int, code, msg string) { - cors.Apply(w.Header()) +func writeIngressError(w http.ResponseWriter, r *http.Request, status int, code, msg string) { + cors.Apply(w.Header(), r.Header.Get("Origin")) w.Header().Set("Content-Type", "application/json") w.Header().Set("X-Content-Type-Options", "nosniff") w.WriteHeader(status) diff --git a/services/ollama-proxy/ingress_test.go b/services/ollama-proxy/ingress_test.go index 34934a1a..d6494932 100644 --- a/services/ollama-proxy/ingress_test.go +++ b/services/ollama-proxy/ingress_test.go @@ -51,10 +51,12 @@ func TestHandlePlainRejectsNonLoopback(t *testing.T) { if rec.Code != http.StatusForbidden { t.Fatalf("non-loopback plaintext status = %d, want %d", rec.Code, http.StatusForbidden) } - // The refusal carries CORS so a browser client reads this 403 and its reason - // instead of an opaque "CORS error" that hides why the call failed. - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want * on the refusal", got) + // No CORS grant on the refusal: the caller sent no Origin and the proxy + // writes grants only for allowlisted browser origins. A non-browser LAN + // caller ignores CORS anyway; a cross-origin browser is gated by the + // origin check that follows the loopback gate. + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want no grant on the refusal", got) } } @@ -69,11 +71,14 @@ func TestHandlePlainAnswersPreflightBeforeLoopbackGate(t *testing.T) { rec := httptest.NewRecorder() p.handlePlain(rec, req) + // The preflight is still answered (204) — it authorizes nothing and this + // Origin-less caller gets no grant — and the request that follows would + // hit the loopback gate's 403. if rec.Code != http.StatusNoContent { t.Fatalf("preflight status = %d, want %d", rec.Code, http.StatusNoContent) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want *", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want no grant", got) } } diff --git a/services/ollama-proxy/proxy.go b/services/ollama-proxy/proxy.go index ad4ac7a2..c860d399 100644 --- a/services/ollama-proxy/proxy.go +++ b/services/ollama-proxy/proxy.go @@ -197,26 +197,31 @@ type workloadParams struct { // bufferBodyAndModel reads the request body once and returns the raw bytes // (so each failover attempt can replay it — see the loop in handleHTTP) along -// with the JSON "model" field for workload tracking. Inference bodies are -// small (prompt + model), so full buffering is cheap. Returns (nil, "") when -// the body is absent and an empty model when none is parseable. The caller -// restores r.Body from the returned bytes before each forward attempt. -func bufferBodyAndModel(r *http.Request) ([]byte, string) { +// with the JSON "model" field for workload tracking. Bodies are capped at +// maxInferenceBodyBytes: without a limit, any loopback caller (or a cross-origin +// browser POST) could stream an arbitrarily large body and exhaust proxy +// memory. Returns (nil, "", false) when the body is absent and an empty model +// when none is parseable. The caller restores r.Body from the returned bytes +// before each forward attempt. +func bufferBodyAndModel(r *http.Request) ([]byte, string, bool) { if r.Body == nil { - return nil, "" + return nil, "", false } - body, err := io.ReadAll(r.Body) + body, err := io.ReadAll(io.LimitReader(r.Body, maxInferenceBodyBytes+1)) _ = r.Body.Close() if err != nil { - return body, "" + return body, "", false + } + if len(body) > maxInferenceBodyBytes { + return nil, "", true } var probe struct { Model string `json:"model"` } if err := json.Unmarshal(body, &probe); err != nil { - return body, "" + return body, "", false } - return body, probe.Model + return body, probe.Model, false } type statusCapture struct { @@ -536,6 +541,11 @@ const ( proxyReadHeaderTimeout = 10 * time.Second proxyServerIdleTimeout = 90 * time.Second maxModelListBytes = 16 << 20 + // maxInferenceBodyBytes caps how much of an inbound request body the proxy + // buffers for replay across failover attempts. Long-context prompts fit + // far below this; anything larger is rejected with 413 instead of being + // buffered into memory unbounded. + maxInferenceBodyBytes = 32 << 20 ) // idleClientWriteTimeout bounds how long a single write of streamed response @@ -981,7 +991,7 @@ func ollamaModelKey(model string) string { func (p *Proxy) serveModelList(w http.ResponseWriter, r *http.Request, candidates []candidate) (int, error) { openAI := r.URL.Path == "/v1/models" writeJSON := func(status int, body []byte) { - cors.Apply(w.Header()) + cors.Apply(w.Header(), r.Header.Get("Origin")) w.Header().Set("Content-Type", "application/json") w.Header().Set("X-Content-Type-Options", "nosniff") w.WriteHeader(status) @@ -1140,7 +1150,22 @@ func (p *Proxy) handleHTTP(w http.ResponseWriter, r *http.Request) { // Parse the request's model before choosing a node. Model eligibility only // applies to inference routes; control endpoints retain their existing // routing behavior even when their JSON happens to contain a model field. - bodyBytes, model := bufferBodyAndModel(r) + bodyBytes, model, bodyTooLarge := bufferBodyAndModel(r) + if bodyTooLarge { + slog.Warn("proxy request rejected", + "id", reqID, "method", r.Method, "path", r.URL.Path, + "remote", r.RemoteAddr, "reason", "request body exceeds limit") + http.Error(w, `{"error":"request body exceeds limit"}`, http.StatusRequestEntityTooLarge) + p.codec.Notify("proxy/request", RequestEvent{ + ID: reqID, + Method: r.Method, + Path: r.URL.Path, + Status: http.StatusRequestEntityTooLarge, + Duration: time.Since(start).Milliseconds(), + Error: "request body exceeds limit", + }) + return + } isInf := isInferenceRequest(r.Method, r.URL.Path) routingModel := "" if isInf { @@ -1173,7 +1198,7 @@ func (p *Proxy) handleHTTP(w http.ResponseWriter, r *http.Request) { if cors.WritePreflight(w, r) { return } - cors.Apply(w.Header()) + cors.Apply(w.Header(), r.Header.Get("Origin")) rejectionBody := `{"error":"no active node selected or available"}` rejectionError := "no active node" if isInf && model != "" { @@ -1368,7 +1393,7 @@ func (p *Proxy) handleHTTP(w http.ResponseWriter, r *http.Request) { // outright. An engine that omits the header has expressed // nothing to preserve, so the proxy supplies its own. if resp.Header.Get("Access-Control-Allow-Origin") == "" { - cors.Apply(resp.Header) + cors.Apply(resp.Header, r.Header.Get("Origin")) } if !started { started = true @@ -1437,7 +1462,7 @@ func (p *Proxy) handleHTTP(w http.ResponseWriter, r *http.Request) { if mErr != nil { body = []byte(`{"error":"upstream error"}`) } - cors.Apply(ew.Header()) + cors.Apply(ew.Header(), r.Header.Get("Origin")) ew.Header().Set("Content-Type", "application/json") ew.Header().Set("X-Content-Type-Options", "nosniff") ew.WriteHeader(http.StatusBadGateway) @@ -2120,8 +2145,15 @@ func (p *Proxy) readLoop(ctx context.Context) error { if err == io.EOF || ctx.Err() != nil { return nil } - log.Printf("JSON-RPC read error: %v", err) - continue + var de *DecodeError + if stderrors.As(err, &de) { + log.Printf("JSON-RPC decode error (skipping frame): %v", err) + continue + } + // Terminal transport/scanner error (e.g. an over-long frame — + // bufio.Scanner cannot resync) — stop instead of spinning. + log.Printf("JSON-RPC read error (terminal): %v", err) + return err } p.handleMessage(msg) } diff --git a/services/shared/cors/cors.go b/services/shared/cors/cors.go index 67f15168..0aa38ce5 100644 --- a/services/shared/cors/cors.go +++ b/services/shared/cors/cors.go @@ -8,57 +8,126 @@ // answer a preflight and label a response identically; keeping one // implementation is what stops the two from drifting apart. // +// The policy is deny-by-default. A browser page on any origin can reach the +// proxies' loopback listener (browsers connect from loopback, so a bind-scoped +// gate does not exclude them); with an "allow every origin" policy any website +// could drive and read the local inference engines — exfiltrating responses +// and using the models as a free oracle. Browser callers are therefore +// admitted only from exact origins the operator lists in +// NVPAIR_PROXY_ALLOWED_ORIGINS (comma-separated, scheme+host[:port], compared +// exactly). Non-browser callers (the Electron main process, CLI tools, health +// probes) send no Origin header and are unaffected. +// // The policy applies to responses a proxy authors itself. A response forwarded // from an engine that declared its own Access-Control-Allow-Origin keeps that // engine's policy — including on a preflight, where preserving an exact origin // and Access-Control-Allow-Credentials is required for credentialed browser -// requests. An engine without a CORS policy gets the proxy's permissive fallback. +// requests. An engine without a CORS policy gets the proxy's deny-by-default +// fallback. package cors -import "net/http" +import ( + "net/http" + "os" + "strings" +) + +// allowedOriginsEnv names the operator-controlled origin allowlist. Exact +// origins only; an empty value (the default) admits no browser origins. +const allowedOriginsEnv = "NVPAIR_PROXY_ALLOWED_ORIGINS" + +// allowedOrigins returns the configured allowlist. Read per request so an +// operator change needs a proxy restart at most, not a code change per caller. +func allowedOrigins() []string { + raw := strings.TrimSpace(os.Getenv(allowedOriginsEnv)) + if raw == "" { + return nil + } + var out []string + for _, o := range strings.Split(raw, ",") { + if o = strings.TrimSpace(o); o != "" { + out = append(out, o) + } + } + return out +} + +// originAllowed reports whether the request's Origin header is absent (a +// non-browser or same-origin-with-no-origin caller — allowed) or exactly +// matches one configured origin (allowed). A present-but-unlisted Origin is +// the cross-origin browser case: denied. +func originAllowed(r *http.Request) bool { + origin := r.Header.Get("Origin") + if origin == "" { + return true + } + for _, want := range allowedOrigins() { + if origin == want { + return true + } + } + return false +} + +// AllowRequest gates a browser-capable request: anything carrying an Origin +// not on the operator allowlist is rejected before it can drive an engine, +// closing the cross-origin blind-oracle path (a simple cross-origin POST needs +// no preflight, so header/preflight policy alone cannot provide this). +func AllowRequest(r *http.Request) bool { return originAllowed(r) } -// Apply writes the permissive CORS policy so browser-based clients can read -// proxy responses — and, crucially, error bodies. Without an -// Access-Control-Allow-Origin a browser surfaces every failure as an opaque -// "CORS error", hiding the real status the proxy returned. The proxies front a -// local-network inference engine, not a credentialed API, so a wildcard origin -// is appropriate and they never reflect credentials. -func Apply(h http.Header) { - h.Set("Access-Control-Allow-Origin", "*") +// RejectOrigin writes the 403 for a disallowed Origin. The body is static so +// nothing about the proxy's internals is reflected. +func RejectOrigin(w http.ResponseWriter) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Content-Type-Options", "nosniff") + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"error":"cross-origin browser requests are not allowed","code":"origin-not-allowed"}`)) +} + +// Apply writes the proxy's own CORS policy for an allowlisted (or +// Origin-less) caller. With no allowlist entry matching the request origin the +// recommended posture is to write no CORS headers at all — a browser then +// cannot read the response cross-origin, while non-browser callers are +// untouched because they ignore CORS entirely. +// +// Without an Access-Control-Allow-Origin a browser surfaces every failure as +// an opaque "CORS error", but that opacity is the intended cost of deny-by- +// default: the operator lists the origins that may read responses, and every +// other origin gets nothing. +func Apply(h http.Header, origin string) { + // The proxies never allow credentials; make that explicit against any + // inherited header so the response can't be misread as credentialed — + // regardless of whether an origin grant is being written. + h.Del("Access-Control-Allow-Credentials") + if origin == "" { + return + } + h.Set("Access-Control-Allow-Origin", origin) + h.Set("Vary", "Origin") h.Set("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS") h.Set("Access-Control-Allow-Headers", "Content-Type, Authorization") - // A browser rejects a wildcard origin paired with Allow-Credentials: true, - // so setting the former while inheriting the latter would leave the - // response unreadable — the failure this policy exists to prevent. Callers - // reach here only when no engine policy is being preserved, so an - // Allow-Credentials from an upstream that sent no origin of its own - // describes a policy this response no longer carries. Drop it. - h.Del("Access-Control-Allow-Credentials") - // Uncredentialed wildcard responses may expose every header, so a browser - // client can read engine metadata outside the CORS-safelisted set. - h.Set("Access-Control-Expose-Headers", "*") - h.Set("Access-Control-Max-Age", "86400") } +// applyPreflight answers an allowlisted origin's preflight with the static +// method/header grant. Arbitrary request headers are deliberately NOT echoed: +// a preflight naming a header outside the static grant simply fails, which is +// deny-by-default doing its job. func applyPreflight(h http.Header, r *http.Request) { - Apply(h) - // Echo the browser's requested headers so an arbitrary client header - // (e.g. a custom auth header) clears preflight instead of being - // rejected by our static default. - if reqHdrs := r.Header.Get("Access-Control-Request-Headers"); reqHdrs != "" { - h.Set("Access-Control-Allow-Headers", reqHdrs) - } + Apply(h, r.Header.Get("Origin")) } -// WritePreflight answers a browser's OPTIONS preflight locally with 204 plus -// the permissive fallback policy, and reports whether it handled the request. -// The proxies use this when no engine can be consulted (or before rejecting a -// non-loopback plaintext caller). When an engine is available, its preflight is -// forwarded instead so an exact origin and credentials policy can survive. +// WritePreflight answers a browser's OPTIONS preflight locally when the origin +// is allowlisted, and reports whether it handled the request. A disallowed +// origin's preflight is left unhandled so the caller's own gate answers 403. +// When an engine is available and allowlisted, its preflight is forwarded +// instead so an exact origin and credentials policy can survive. func WritePreflight(w http.ResponseWriter, r *http.Request) bool { if r.Method != http.MethodOptions { return false } + if !originAllowed(r) { + return false + } applyPreflight(w.Header(), r) w.WriteHeader(http.StatusNoContent) return true @@ -74,6 +143,9 @@ func CompletePreflightFallback(resp *http.Response) bool { resp.Header.Get("Access-Control-Allow-Origin") != "" { return false } + if !originAllowed(resp.Request) { + return false + } if resp.Body != nil { _ = resp.Body.Close() } diff --git a/services/shared/cors/cors_test.go b/services/shared/cors/cors_test.go index c561aba9..ef9e1cc8 100644 --- a/services/shared/cors/cors_test.go +++ b/services/shared/cors/cors_test.go @@ -4,80 +4,126 @@ package cors import ( - "io" "net/http" "net/http/httptest" - "strings" "testing" ) -func TestApplySetsPolicy(t *testing.T) { +func TestApplyWithNoOriginWritesNothing(t *testing.T) { h := http.Header{} - Apply(h) - for header, want := range map[string]string{ - "Access-Control-Allow-Origin": "*", - "Access-Control-Expose-Headers": "*", - } { - if got := h.Get(header); got != want { - t.Errorf("%s = %q, want %q", header, got, want) - } - } - if h.Get("Access-Control-Allow-Methods") == "" { - t.Error("missing Access-Control-Allow-Methods") - } - // A wildcard origin is only valid for uncredentialed responses, so the - // policy must never claim to allow credentials. - if got := h.Get("Access-Control-Allow-Credentials"); got != "" { - t.Errorf("Access-Control-Allow-Credentials = %q, want unset alongside a wildcard origin", got) + Apply(h, "") + if got := h.Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want nothing for an Origin-less caller", got) } } -// TestApplyClearsInheritedCredentials: Apply runs on forwarded responses too, -// where the header map is whatever the upstream sent. An upstream that emits -// Allow-Credentials without an origin of its own would otherwise leave the -// invalid wildcard + credentials pair, which a browser fails closed. -func TestApplyClearsInheritedCredentials(t *testing.T) { - h := http.Header{"Access-Control-Allow-Credentials": []string{"true"}} - Apply(h) - - if got := h.Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want *", got) +func TestApplyWithAllowlistedOrigin(t *testing.T) { + h := http.Header{} + Apply(h, "https://ui.example") + if got := h.Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the exact allowlisted origin", got) + } + if got := h.Get("Vary"); got != "Origin" { + t.Errorf("Vary = %q, want Origin so caches key on it", got) } if got := h.Get("Access-Control-Allow-Credentials"); got != "" { - t.Errorf("Access-Control-Allow-Credentials = %q, want cleared alongside the wildcard origin", got) + t.Errorf("Access-Control-Allow-Credentials = %q, want unset", got) + } + if got := h.Get("Access-Control-Expose-Headers"); got != "" { + t.Errorf("Access-Control-Expose-Headers = %q, want unset (no wildcard header exposure)", got) } } -func TestWritePreflightEchoesRequestedHeaders(t *testing.T) { - rec := httptest.NewRecorder() - req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) - req.Header.Set("Access-Control-Request-Headers", "X-Custom-Token") - - if !WritePreflight(rec, req) { - t.Fatal("WritePreflight(OPTIONS) = false, want the preflight handled") - } - if rec.Code != http.StatusNoContent { - t.Errorf("status = %d, want %d", rec.Code, http.StatusNoContent) - } - if got := rec.Header().Get("Access-Control-Allow-Headers"); got != "X-Custom-Token" { - t.Errorf("Access-Control-Allow-Headers = %q, want echoed X-Custom-Token", got) - } +func TestAllowRequest(t *testing.T) { + t.Run("no Origin header is allowed (non-browser callers)", func(t *testing.T) { + if !AllowRequest(httptest.NewRequest(http.MethodPost, "/api/chat", nil)) { + t.Fatal("an Origin-less request must be allowed") + } + }) + t.Run("empty allowlist denies every browser origin", func(t *testing.T) { + t.Setenv(allowedOriginsEnv, "") + req := httptest.NewRequest(http.MethodPost, "/api/chat", nil) + req.Header.Set("Origin", "https://evil.example") + if AllowRequest(req) { + t.Fatal("an unlisted Origin must be denied with an empty allowlist") + } + }) + t.Run("exact match required", func(t *testing.T) { + t.Setenv(allowedOriginsEnv, " https://ui.example ,http://localhost:5173 ") + allowed := httptest.NewRequest(http.MethodPost, "/api/chat", nil) + allowed.Header.Set("Origin", "https://ui.example") + if !AllowRequest(allowed) { + t.Fatal("an exactly-allowlisted Origin must be allowed") + } + prefix := httptest.NewRequest(http.MethodPost, "/api/chat", nil) + prefix.Header.Set("Origin", "https://ui.example.evil.example") + if AllowRequest(prefix) { + t.Fatal("a same-suffix origin must not match an allowlist entry as a prefix") + } + scheme := httptest.NewRequest(http.MethodPost, "/api/chat", nil) + scheme.Header.Set("Origin", "http://ui.example") + if AllowRequest(scheme) { + t.Fatal("a scheme-swapped origin must not match") + } + }) } -// TestWritePreflightIgnoresOtherMethods: a real request must fall through to the -// caller's own handling untouched, with no response written. -func TestWritePreflightIgnoresOtherMethods(t *testing.T) { +func TestRejectOriginShape(t *testing.T) { rec := httptest.NewRecorder() - if WritePreflight(rec, httptest.NewRequest(http.MethodPost, "/api/chat", nil)) { - t.Fatal("WritePreflight(POST) = true, want the request left to the caller") + RejectOrigin(rec) + if rec.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403", rec.Code) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { - t.Errorf("Access-Control-Allow-Origin = %q, want no headers written", got) + if got := rec.Header().Get("Content-Type"); got != "application/json" { + t.Errorf("Content-Type = %q, want application/json", got) } } +func TestWritePreflightGatesOnOrigin(t *testing.T) { + t.Run("disallowed origin falls through unhandled", func(t *testing.T) { + t.Setenv(allowedOriginsEnv, "") + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) + req.Header.Set("Origin", "https://evil.example") + if WritePreflight(rec, req) { + t.Fatal("WritePreflight = true for an unlisted origin, want fall-through") + } + }) + t.Run("allowlisted origin gets a 204 with the static grant", func(t *testing.T) { + t.Setenv(allowedOriginsEnv, "https://ui.example") + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) + req.Header.Set("Origin", "https://ui.example") + req.Header.Set("Access-Control-Request-Headers", "X-Custom-Token") + if !WritePreflight(rec, req) { + t.Fatal("WritePreflight = false for an allowlisted origin") + } + if rec.Code != http.StatusNoContent { + t.Errorf("status = %d, want %d", rec.Code, http.StatusNoContent) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://ui.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the exact origin", got) + } + // Arbitrary request headers are deliberately NOT echoed: a header + // outside the static grant must fail preflight. + if got := rec.Header().Get("Access-Control-Allow-Headers"); got != "Content-Type, Authorization" { + t.Errorf("Access-Control-Allow-Headers = %q, want the static grant (no echo)", got) + } + }) + t.Run("non-OPTIONS falls through", func(t *testing.T) { + rec := httptest.NewRecorder() + if WritePreflight(rec, httptest.NewRequest(http.MethodPost, "/api/chat", nil)) { + t.Fatal("WritePreflight(POST) = true, want the request left to the caller") + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want no headers written", got) + } + }) +} + func TestCompletePreflightFallbackPreservesEnginePolicy(t *testing.T) { req := httptest.NewRequest(http.MethodOptions, "/api/chat", nil) + req.Header.Set("Origin", "https://app.example") resp := &http.Response{ StatusCode: http.StatusNoContent, Status: "204 No Content", @@ -101,14 +147,16 @@ func TestCompletePreflightFallbackPreservesEnginePolicy(t *testing.T) { } func TestCompletePreflightFallbackReplacesMissingEnginePolicy(t *testing.T) { + t.Setenv(allowedOriginsEnv, "https://app.example") req := httptest.NewRequest(http.MethodOptions, "/api/chat", nil) + req.Header.Set("Origin", "https://app.example") req.Header.Set("Access-Control-Request-Headers", "X-Custom-Token") resp := &http.Response{ StatusCode: http.StatusNotFound, Status: "404 Not Found", Header: http.Header{"Content-Type": []string{"text/plain"}}, - Body: io.NopCloser(strings.NewReader("not found")), - ContentLength: 9, + Body: http.NoBody, + ContentLength: 0, Request: req, } @@ -118,11 +166,11 @@ func TestCompletePreflightFallbackReplacesMissingEnginePolicy(t *testing.T) { if resp.StatusCode != http.StatusNoContent { t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusNoContent) } - if got := resp.Header.Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want *", got) + if got := resp.Header.Get("Access-Control-Allow-Origin"); got != "https://app.example" { + t.Errorf("Access-Control-Allow-Origin = %q, want the exact origin", got) } - if got := resp.Header.Get("Access-Control-Allow-Headers"); got != "X-Custom-Token" { - t.Errorf("Access-Control-Allow-Headers = %q, want echoed X-Custom-Token", got) + if got := resp.Header.Get("Access-Control-Allow-Headers"); got != "Content-Type, Authorization" { + t.Errorf("Access-Control-Allow-Headers = %q, want the static grant", got) } if resp.Body != http.NoBody || resp.ContentLength != 0 { t.Errorf("fallback body = %#v with length %d, want http.NoBody with length 0", resp.Body, resp.ContentLength) @@ -132,11 +180,8 @@ func TestCompletePreflightFallbackReplacesMissingEnginePolicy(t *testing.T) { } } -// TestCompletePreflightFallbackDropsCredentialsWithoutOrigin: an upstream -// preflight carrying Allow-Credentials but no origin has no policy to preserve, -// so the fallback applies — and must not leave the credentials header behind to -// invalidate the wildcard origin it just wrote. func TestCompletePreflightFallbackDropsCredentialsWithoutOrigin(t *testing.T) { + t.Setenv(allowedOriginsEnv, "") req := httptest.NewRequest(http.MethodOptions, "/api/chat", nil) resp := &http.Response{ StatusCode: http.StatusNoContent, @@ -146,13 +191,17 @@ func TestCompletePreflightFallbackDropsCredentialsWithoutOrigin(t *testing.T) { Request: req, } + // No Origin on the request and an empty allowlist: originAllowed is true + // (the Origin-absent branch), so the fallback still applies and must not + // leave the credentials header behind to invalidate the exact-origin + // response it just wrote. if !CompletePreflightFallback(resp) { t.Fatal("CompletePreflightFallback = false, want the local fallback applied") } - if got := resp.Header.Get("Access-Control-Allow-Origin"); got != "*" { - t.Errorf("Access-Control-Allow-Origin = %q, want *", got) + if got := resp.Header.Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want nothing for an Origin-less caller", got) } if got := resp.Header.Get("Access-Control-Allow-Credentials"); got != "" { - t.Errorf("Access-Control-Allow-Credentials = %q, want cleared alongside the wildcard origin", got) + t.Errorf("Access-Control-Allow-Credentials = %q, want cleared", got) } } diff --git a/services/tests/broker_management_test.go b/services/tests/broker_management_test.go index 25a4afe7..04f8d12b 100644 --- a/services/tests/broker_management_test.go +++ b/services/tests/broker_management_test.go @@ -143,13 +143,11 @@ func proxyNodesHas(t *testing.T, raw json.RawMessage, id string) bool { // shows up in proxy:nodes/list and can be routed to — even though it never // appears over mDNS. func TestBrokerBridgesManualNodeIntoProxy(t *testing.T) { - if portBusy(11435) { - t.Skip("ollama-proxy port 11435 already in use; skipping") - } + svcEnv, _ := freeServicePortEnv(t) stopOllama := fakeOllama(t) // skips if 11434 unavailable t.Cleanup(stopOllama) - stdin, msgs, _, cleanup := startBrokerWith(t, + stdin, msgs, _, cleanup := startBrokerWithEnv(t, svcEnv, "--manual-nodes-path", manualNodesBin, "--proxy-path", proxyBin, ) @@ -197,15 +195,13 @@ func TestBrokerBridgesManualNodeIntoProxy(t *testing.T) { // resolve to it. The candidate must therefore appear in proxy:nodes/list under // the learned hostUuid, not the user-supplied manual name. func TestBrokerBridgesManualNodeUnderLearnedUUID(t *testing.T) { - if portBusy(11435) { - t.Skip("ollama-proxy port 11435 already in use; skipping") - } + svcEnv, _ := freeServicePortEnv(t) stopOllama := fakeOllama(t) // skips if 11434 unavailable t.Cleanup(stopOllama) stopNodeInfo := fakeNodeInfo(t, "learned-host-uuid") // skips if 14318 unavailable t.Cleanup(stopNodeInfo) - stdin, msgs, _, cleanup := startBrokerWith(t, + stdin, msgs, _, cleanup := startBrokerWithEnv(t, svcEnv, "--manual-nodes-path", manualNodesBin, "--proxy-path", proxyBin, ) @@ -301,11 +297,9 @@ func TestBrokerBridgesManualNodeIntoLMStudioProxy(t *testing.T) { // reflected in the next errors:update — the same as a supervised worker // emitting one on its stdout. func TestBrokerAcceptsClientErrorsReport(t *testing.T) { - if portBusy(14319) { - t.Skip("nvpair-errors --peer-sync port 14319 already in use; skipping") - } + svcEnv, _ := freeServicePortEnv(t) - stdin, msgs, _, cleanup := startBrokerWith(t, "--errors-path", errorsBin) + stdin, msgs, _, cleanup := startBrokerWithEnv(t, svcEnv, "--errors-path", errorsBin) t.Cleanup(cleanup) waitForMethod(t, msgs, "app:ready", 10*time.Second) @@ -326,11 +320,9 @@ func TestBrokerAcceptsClientErrorsReport(t *testing.T) { // leg B: an id-bearing errors:report is acked with a null result and also // reflected in errors:update. func TestBrokerAcceptsClientErrorsReportRequest(t *testing.T) { - if portBusy(14319) { - t.Skip("nvpair-errors --peer-sync port 14319 already in use; skipping") - } + svcEnv, _ := freeServicePortEnv(t) - stdin, msgs, _, cleanup := startBrokerWith(t, "--errors-path", errorsBin) + stdin, msgs, _, cleanup := startBrokerWithEnv(t, svcEnv, "--errors-path", errorsBin) t.Cleanup(cleanup) waitForMethod(t, msgs, "app:ready", 10*time.Second) diff --git a/services/tests/broker_supervision_test.go b/services/tests/broker_supervision_test.go index de917bc5..2f77c8bf 100644 --- a/services/tests/broker_supervision_test.go +++ b/services/tests/broker_supervision_test.go @@ -42,6 +42,51 @@ func startBrokerWithEnv(t *testing.T, extraEnv []string, extraArgs ...string) (s return startBrokerWithConfigDirAndEnv(t, t.TempDir(), extraEnv, extraArgs...) } +// proxyPortEnv overrides both proxies' listen ports with free ports, so a +// broker spawn never fights a dev process on 11435/1234. Tests that read the +// bound port dynamically (waitProxyReady / waitLMStudioProxyReady) need no +// other change. +func proxyPortEnv(t *testing.T) []string { + t.Helper() + return []string{ + "NVPAIR_SERVICE_OLLAMA_PROXY_PORT=" + fmt.Sprintf("%d", freePort(t)), + "NVPAIR_SERVICE_LMSTUDIO_PROXY_PORT=" + fmt.Sprintf("%d", freePort(t)), + } +} + +// freeServicePortEnv allocates free ports for the broker's fixed service +// ports and returns the NVPAIR_SERVICE_*_PORT env entries that override them, +// plus the resolved values keyed by name. Tests use this instead of skipping +// on fixed-port collisions: two brokers (or a broker and the dev machine's +// real one) can then coexist on one host. +func freeServicePortEnv(t *testing.T) ([]string, map[string]int) { + t.Helper() + names := []string{ + "NVPAIR_SERVICE_NODE_INFO_PORT", + "NVPAIR_SERVICE_ERRORS_PORT", + "NVPAIR_SERVICE_WORKLOAD_PORT", + "NVPAIR_SERVICE_CLUSTER_MANAGER_PORT", + "NVPAIR_SERVICE_ENGINE_HTTP_PORT", + "NVPAIR_SERVICE_ENGINE_CONTROL_PORT", + } + env := make([]string, 0, len(names)) + ports := make(map[string]int, len(names)) + used := map[int]bool{} + for _, name := range names { + var p int + for { + p = freePort(t) + if !used[p] { + break + } + } + used[p] = true + env = append(env, name+"="+fmt.Sprintf("%d", p)) + ports[name] = p + } + return env, ports +} + func startBrokerWithConfigDir(t *testing.T, configDir string, extraArgs ...string) (stdin io.WriteCloser, msgs <-chan jsonrpc.Message, stderrLines <-chan string, cleanup func()) { t.Helper() // An empty --cluster-dir makes this node a NON-MEMBER regardless of the @@ -192,10 +237,9 @@ const methodErrorsUpdate = "errors:update" var proxyPidRe = regexp.MustCompile(`proxy started.*\bpid=(\d+)`) -// portBusy reports whether a TCP port on localhost is already bound — used -// to skip tests whose supervised workers need a fatal-on-conflict listener -// (nvpair-errors --peer-sync on 14319, ollama-proxy on 11435) when something -// else on the host already holds it. +// portBusy reports whether a TCP port on localhost is already bound — used by +// TestInheritedOllamaHostAliasEndToEnd, whose managed Ollama facade (:11434) +// collides with a real Ollama the test cannot move. func portBusy(port int) bool { ln, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", port)) if err != nil { @@ -210,14 +254,9 @@ func portBusy(port int) bool { // through the nvpair-errors pipeline (errors:update relay + errors:get-initial), // and that the supervisor restarts the proxy. func TestBrokerCrashSurfacingAndRestart(t *testing.T) { - if portBusy(14319) { - t.Skip("nvpair-errors --peer-sync port 14319 already in use; skipping") - } - if portBusy(11435) { - t.Skip("ollama-proxy port 11435 already in use; skipping") - } + svcEnv, _ := freeServicePortEnv(t) - stdin, msgs, stderr, cleanup := startBrokerWith(t, + stdin, msgs, stderr, cleanup := startBrokerWithEnv(t, svcEnv, "--errors-path", errorsBin, "--proxy-path", proxyBin, ) @@ -414,9 +453,7 @@ func proxyStatus(t *testing.T, stdin io.Writer, msgs <-chan jsonrpc.Message, id // running, the requested port is free, so the broker relays it unchanged (no // bump) — exercising the interception + relay + rebind wiring end to end. func TestBrokerProxySetPortRebinds(t *testing.T) { - if portBusy(11435) { - t.Skip("ollama-proxy default port 11435 already in use; skipping") - } + svcEnv, _ := freeServicePortEnv(t) cfg := t.TempDir() cmd := exec.Command(brokerBin, "--scanner-path", scannerBin, @@ -431,6 +468,7 @@ func TestBrokerProxySetPortRebinds(t *testing.T) { "APPDATA="+cfg, "LOCALAPPDATA="+cfg, ) + cmd.Env = append(cmd.Env, svcEnv...) cmd.Stderr = os.Stderr stdin, err := cmd.StdinPipe() if err != nil { diff --git a/services/tests/cluster_data_plane_test.go b/services/tests/cluster_data_plane_test.go index 901e6900..1d6e8ceb 100644 --- a/services/tests/cluster_data_plane_test.go +++ b/services/tests/cluster_data_plane_test.go @@ -18,6 +18,7 @@ import ( "crypto/tls" "encoding/json" "fmt" + "strconv" "io" "net" "net/http" @@ -34,10 +35,28 @@ import ( "github.com/grandcat/zeroconf" ) -const ( - workloadEventsURL = "127.0.0.1:14320/v1/workloads/events" - errorsSyncURL = "127.0.0.1:14319/v1/errors" -) +// dataPlaneURLs are the per-test inter-node endpoints and listener addresses. +// Ports come from the freeServicePortEnv overrides so concurrent test runs and +// cohabiting dev machines never collide on the spec's fixed 14319/14320. +type dataPlaneURLs struct { + workloadEventsURL string + errorsSyncURL string + workloadAddr string + errorsAddr string +} + +func newDataPlaneURLs(t *testing.T) (env []string, urls dataPlaneURLs) { + t.Helper() + svcEnv, ports := freeServicePortEnv(t) + wl := ports["NVPAIR_SERVICE_WORKLOAD_PORT"] + er := ports["NVPAIR_SERVICE_ERRORS_PORT"] + return svcEnv, dataPlaneURLs{ + workloadEventsURL: fmt.Sprintf("127.0.0.1:%d/v1/workloads/events", wl), + errorsSyncURL: fmt.Sprintf("127.0.0.1:%d/v1/errors", er), + workloadAddr: fmt.Sprintf("127.0.0.1:%d", wl), + errorsAddr: fmt.Sprintf("127.0.0.1:%d", er), + } +} // interNodeCluster is a synthetic two-node cluster for the data-plane tests. // nodeDir is handed to the broker as --cluster-dir so its workers come up as a @@ -188,11 +207,9 @@ const dataPlaneErrorsEnvelope = `{"nodeId":"intruder-node","errors":[{"id":"intr // plaintext workload events from any host on the network and relayed them into its // catalog as though a peer had sent them. func TestDataPlaneRefusesNonMemberWhenUnclustered(t *testing.T) { - if portBusy(14319) || portBusy(14320) { - t.Skip("an inter-node port is already in use; skipping") - } + env, urls := newDataPlaneURLs(t) // startBrokerProc pins an EMPTY cluster dir, so these workers are non-members. - _, msgs, cleanup := startBrokerProc(t, + _, msgs, cleanup := startBrokerProcWithEnv(t, env, "--scanner-path", scannerBin, "--workload-manager-path", workloadMgrBin, "--errors-path", errorsBin, @@ -200,11 +217,11 @@ func TestDataPlaneRefusesNonMemberWhenUnclustered(t *testing.T) { t.Cleanup(cleanup) waitForMethod(t, msgs, "app:ready", 10*time.Second) - waitTCP(t, "127.0.0.1:14320", 15*time.Second) - assertPlaintextRefused(t, workloadEventsURL, dataPlaneWorkloadFrame) + waitTCP(t, urls.workloadAddr, 15*time.Second) + assertPlaintextRefused(t, urls.workloadEventsURL, dataPlaneWorkloadFrame) - waitTCP(t, "127.0.0.1:14319", 15*time.Second) - assertPlaintextRefused(t, errorsSyncURL, dataPlaneErrorsEnvelope) + waitTCP(t, urls.errorsAddr, 15*time.Second) + assertPlaintextRefused(t, urls.errorsSyncURL, dataPlaneErrorsEnvelope) } // TestModelInventoryRefusesLANPlaintext: a node's model inventory is cluster data. @@ -222,23 +239,22 @@ func TestModelInventoryRefusesLANPlaintext(t *testing.T) { if lanIP == "" { t.Skip("no non-loopback IPv4 address available; cannot exercise the LAN path") } - if portBusy(14322) { - t.Skip("engine-manager em port 14322 already in use; skipping") - } + svcEnv, ports := freeServicePortEnv(t) + emPort := ports["NVPAIR_SERVICE_ENGINE_HTTP_PORT"] // startBrokerProc pins an empty cluster dir, so this node is a non-member. - _, msgs, cleanup := startBrokerProc(t, + _, msgs, cleanup := startBrokerProcWithEnv(t, svcEnv, "--scanner-path", scannerBin, "--engine-manager-path", engineMgrBin, ) t.Cleanup(cleanup) waitForMethod(t, msgs, "app:ready", 15*time.Second) - waitTCP(t, "127.0.0.1:14322", 20*time.Second) + waitTCP(t, fmt.Sprintf("127.0.0.1:%d", emPort), 20*time.Second) client := &http.Client{Timeout: 5 * time.Second} // Loopback: this node's own scanner path must keep working while unclustered. - resp, err := client.Get("http://127.0.0.1:14322/v1/models") + resp, err := client.Get(fmt.Sprintf("http://127.0.0.1:%d/v1/models", emPort)) if err != nil { t.Fatalf("loopback model fetch failed: %v", err) } @@ -251,7 +267,7 @@ func TestModelInventoryRefusesLANPlaintext(t *testing.T) { // The same request from this host's LAN address is a non-loopback caller with // no cluster credentials, and must be refused. - lanResp, err := client.Get("http://" + net.JoinHostPort(lanIP, "14322") + "/v1/models") + lanResp, err := client.Get("http://" + net.JoinHostPort(lanIP, strconv.Itoa(emPort)) + "/v1/models") if err != nil { t.Logf("LAN plaintext model fetch refused at the transport: %v", err) return @@ -270,9 +286,7 @@ func TestModelInventoryRefusesLANPlaintext(t *testing.T) { // host can complete the TLS handshake, which is what makes the per-request pin // gate load-bearing rather than decorative. func TestDataPlaneRefusesNonMemberWhenClustered(t *testing.T) { - if portBusy(14319) || portBusy(14320) { - t.Skip("an inter-node port is already in use; skipping") - } + env, urls := newDataPlaneURLs(t) fx := newInterNodeCluster(t) // A stranger: it pins the node under test (so it can verify the server and @@ -288,7 +302,7 @@ func TestDataPlaneRefusesNonMemberWhenClustered(t *testing.T) { } stranger := &http.Client{Timeout: 5 * time.Second, Transport: &http.Transport{TLSClientConfig: strangerCfg}} - _, msgs, cleanup := startBrokerProcInCluster(t, fx.nodeDir, + _, msgs, cleanup := startBrokerProcInClusterWithEnv(t, fx.nodeDir, env, "--scanner-path", scannerBin, "--workload-manager-path", workloadMgrBin, "--errors-path", errorsBin, @@ -296,13 +310,13 @@ func TestDataPlaneRefusesNonMemberWhenClustered(t *testing.T) { t.Cleanup(cleanup) waitForMethod(t, msgs, "app:ready", 10*time.Second) - waitTCP(t, "127.0.0.1:14320", 15*time.Second) - assertPlaintextRefused(t, workloadEventsURL, dataPlaneWorkloadFrame) - assertForbidden(t, stranger, "https://"+workloadEventsURL, dataPlaneWorkloadFrame) + waitTCP(t, urls.workloadAddr, 15*time.Second) + assertPlaintextRefused(t, urls.workloadEventsURL, dataPlaneWorkloadFrame) + assertForbidden(t, stranger, "https://"+urls.workloadEventsURL, dataPlaneWorkloadFrame) - waitTCP(t, "127.0.0.1:14319", 15*time.Second) - assertPlaintextRefused(t, errorsSyncURL, dataPlaneErrorsEnvelope) - assertForbidden(t, stranger, "https://"+errorsSyncURL, dataPlaneErrorsEnvelope) + waitTCP(t, urls.errorsAddr, 15*time.Second) + assertPlaintextRefused(t, urls.errorsSyncURL, dataPlaneErrorsEnvelope) + assertForbidden(t, stranger, "https://"+urls.errorsSyncURL, dataPlaneErrorsEnvelope) // Control: the node's real pinned peer IS served, so the assertions above are // rejecting the caller rather than a broken listener. It carries a distinct @@ -310,7 +324,7 @@ func TestDataPlaneRefusesNonMemberWhenClustered(t *testing.T) { // byte-identical, which would silently defeat any later assertion that the // injected workload is absent from the catalog. peer := fx.clientAsPeer(t) - postUntil(t, peer, "https://"+workloadEventsURL, dataPlanePeerFrame, http.StatusOK, 15*time.Second) + postUntil(t, peer, "https://"+urls.workloadEventsURL, dataPlanePeerFrame, http.StatusOK, 15*time.Second) } // assertForbidden requires a request that completes a handshake to be turned away diff --git a/services/tests/cluster_restore_test.go b/services/tests/cluster_restore_test.go index a8d37123..91594914 100644 --- a/services/tests/cluster_restore_test.go +++ b/services/tests/cluster_restore_test.go @@ -23,7 +23,9 @@ import ( // startBrokerInDir starts the broker with settings + cluster-manager pointed at // a caller-supplied config dir (so state persists across a restart when the // same dir is reused), returning its stdin, stdout frame stream, and a cleanup. -func startBrokerInDir(t *testing.T, configDir string, extraArgs ...string) (io.WriteCloser, <-chan jsonrpc.Message, func()) { +// extraEnv (may be nil) carries env entries for the broker process, e.g. +// service-port overrides; the remaining args are broker CLI args. +func startBrokerInDir(t *testing.T, configDir string, extraEnv []string, extraArgs ...string) (io.WriteCloser, <-chan jsonrpc.Message, func()) { t.Helper() args := append([]string{"--scanner-path", scannerBin, "--cluster-dir", t.TempDir()}, extraArgs...) cmd := exec.Command(brokerBin, args...) @@ -33,6 +35,7 @@ func startBrokerInDir(t *testing.T, configDir string, extraArgs ...string) (io.W "APPDATA="+configDir, "LOCALAPPDATA="+configDir, ) + cmd.Env = append(cmd.Env, extraEnv...) stdin, err := cmd.StdinPipe() if err != nil { t.Fatalf("broker stdin pipe: %v", err) @@ -85,13 +88,14 @@ func waitForResponseID(t *testing.T, msgs <-chan jsonrpc.Message, id int, timeou // config dir, and asserts cluster:get-node-id reports the restored clusterId // (rather than the "" a fresh cluster-manager comes up with). func TestBrokerRestoresClusterIdentityAfterRestart(t *testing.T) { + svcEnv, _ := freeServicePortEnv(t) configDir := t.TempDir() const clusterID = "restore-test-cluster-0001" const friendly = "Restore Test Lab" // First broker: persist the cluster identity via the settings relay, as a // clustered node's UI would have on create/join. - stdin1, msgs1, cleanup1 := startBrokerInDir(t, configDir, + stdin1, msgs1, cleanup1 := startBrokerInDir(t, configDir, svcEnv, "--settings-path", nodeSettingsBin, "--cluster-manager-path", clusterMgrBin, ) @@ -116,7 +120,7 @@ func TestBrokerRestoresClusterIdentityAfterRestart(t *testing.T) { // Second broker in the same config dir: the cluster-manager reloads with no // clusterId of its own, and the broker must restore it from settings before // serving requests. - stdin2, msgs2, cleanup2 := startBrokerInDir(t, configDir, + stdin2, msgs2, cleanup2 := startBrokerInDir(t, configDir, svcEnv, "--settings-path", nodeSettingsBin, "--cluster-manager-path", clusterMgrBin, ) @@ -188,13 +192,11 @@ func waitSettingClusterID(t *testing.T, stdin io.Writer, msgs <-chan jsonrpc.Mes // survives a restart), and cluster:leave must clear it (so the node stays // unclustered after a restart rather than having the stale id restored). func TestBrokerPersistsClusterLifecycleToSettings(t *testing.T) { - if portBusy(14321) { - t.Skip("cluster-manager inter-node port 14321 already in use; skipping") - } + svcEnv, _ := freeServicePortEnv(t) configDir := t.TempDir() // 1. Create a cluster; the broker must mirror the new id into settings. - stdin1, msgs1, cleanup1 := startBrokerInDir(t, configDir, + stdin1, msgs1, cleanup1 := startBrokerInDir(t, configDir, svcEnv, "--settings-path", nodeSettingsBin, "--cluster-manager-path", clusterMgrBin, ) @@ -213,7 +215,7 @@ func TestBrokerPersistsClusterLifecycleToSettings(t *testing.T) { cleanup1() // 2. Restart: the created identity is restored from settings. - stdin2, msgs2, cleanup2 := startBrokerInDir(t, configDir, + stdin2, msgs2, cleanup2 := startBrokerInDir(t, configDir, svcEnv, "--settings-path", nodeSettingsBin, "--cluster-manager-path", clusterMgrBin, ) @@ -232,7 +234,7 @@ func TestBrokerPersistsClusterLifecycleToSettings(t *testing.T) { cleanup2() // 4. Restart: the node stays unclustered (the leave stuck). - stdin3, msgs3, cleanup3 := startBrokerInDir(t, configDir, + stdin3, msgs3, cleanup3 := startBrokerInDir(t, configDir, svcEnv, "--settings-path", nodeSettingsBin, "--cluster-manager-path", clusterMgrBin, ) diff --git a/services/tests/main_test.go b/services/tests/main_test.go index c5d63c9c..cfa2a5f2 100644 --- a/services/tests/main_test.go +++ b/services/tests/main_test.go @@ -195,7 +195,6 @@ func waitForMethod(t *testing.T, ch <-chan jsonrpc.Message, method string, timeo t.Fatalf("timed out (%s) waiting for method %q", timeout, method) } } - return jsonrpc.Message{} } func waitForResponse(t *testing.T, ch <-chan jsonrpc.Message, timeout time.Duration) jsonrpc.Message { @@ -215,5 +214,4 @@ func waitForResponse(t *testing.T, ch <-chan jsonrpc.Message, timeout time.Durat t.Fatal("timed out waiting for JSON-RPC response") } } - return jsonrpc.Message{} } diff --git a/services/tests/model_routing_interop_test.go b/services/tests/model_routing_interop_test.go index b6c13dd7..41b595c6 100644 --- a/services/tests/model_routing_interop_test.go +++ b/services/tests/model_routing_interop_test.go @@ -39,15 +39,11 @@ func newRoutingUpstream(t *testing.T, status int) *routingUpstream { } func TestStrictModelRoutingAcrossProcesses(t *testing.T) { - if portBusy(11435) || portBusy(1234) { - t.Skip("ollama-proxy (11435) or lmstudio-proxy (1234) default port already in use; skipping") - } - owner404 := newRoutingUpstream(t, http.StatusNotFound) ownerOK := newRoutingUpstream(t, http.StatusOK) ineligible := newRoutingUpstream(t, http.StatusOK) - stdin, msgs, stderr, cleanup := startBrokerWith(t, + stdin, msgs, stderr, cleanup := startBrokerWithEnv(t, proxyPortEnv(t), "--proxy-path", proxyBin, "--lmstudio-proxy-path", lmstudioProxyBin, ) diff --git a/services/tests/scheduler_interop_test.go b/services/tests/scheduler_interop_test.go index 102f44cf..1c373ef1 100644 --- a/services/tests/scheduler_interop_test.go +++ b/services/tests/scheduler_interop_test.go @@ -553,10 +553,6 @@ func TestProxySetPriorityViaBroker(t *testing.T) { // lmstudio-proxy must intersect that list with its own discovery set and never // dial a priority id that never advertised the lm service. func TestLMStudioProxyIgnoresPriorityNodesAbsentFromDiscovery(t *testing.T) { - if portBusy(1234) { - t.Skip("lmstudio-proxy default port 1234 already in use; skipping") - } - var hitsMu sync.Mutex hits := map[string]int{} realEngine := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -570,7 +566,7 @@ func TestLMStudioProxyIgnoresPriorityNodesAbsentFromDiscovery(t *testing.T) { t.Cleanup(realEngine.Close) realPort := portOfURL(t, realEngine.URL) - stdin, msgs, stderr, cleanup := startBrokerWith(t, + stdin, msgs, stderr, cleanup := startBrokerWithEnv(t, proxyPortEnv(t), "--lmstudio-proxy-path", lmstudioProxyBin, ) t.Cleanup(cleanup) diff --git a/services/tests/workload_identity_interop_test.go b/services/tests/workload_identity_interop_test.go index f3879a74..3bb6c5d3 100644 --- a/services/tests/workload_identity_interop_test.go +++ b/services/tests/workload_identity_interop_test.go @@ -45,9 +45,6 @@ import ( // broker's workloads:* stream. Before the identity fix the (origin,id) store // key collapsed them into one. func TestWorkloadCrossEngineIdentityDistinct(t *testing.T) { - if portBusy(11435) || portBusy(1234) { - t.Skip("ollama-proxy (11435) or lmstudio-proxy (1234) default port already in use; skipping") - } // Fake engines: 200 on any request so each inference completes promptly. fakeEngine := func(body string) *httptest.Server { @@ -65,7 +62,7 @@ func TestWorkloadCrossEngineIdentityDistinct(t *testing.T) { ollamaPort := portOfURL(t, ollama.URL) lmstudioPort := portOfURL(t, lmstudio.URL) - stdin, msgs, _, cleanup := startBrokerWith(t, + stdin, msgs, _, cleanup := startBrokerWithEnv(t, proxyPortEnv(t), "--proxy-path", proxyBin, "--lmstudio-proxy-path", lmstudioProxyBin, "--workload-manager-path", workloadMgrBin, @@ -158,9 +155,6 @@ func TestWorkloadCrossEngineIdentityDistinct(t *testing.T) { // hear the running workload AGAIN from the restarted manager — which only // happens if the broker rehydrated the fresh process's active set. func TestWorkloadManagerRehydratesActiveWorkloadOnRestart(t *testing.T) { - if portBusy(11435) { - t.Skip("ollama-proxy default port 11435 already in use; skipping") - } // Fake Ollama that accepts the request then blocks, keeping the proxied // inference in-flight so the workload never reaches a terminal state. The @@ -191,7 +185,7 @@ func TestWorkloadManagerRehydratesActiveWorkloadOnRestart(t *testing.T) { fx := newInterNodeCluster(t) received := fx.startStubClusterPeer(t, "rehydrate-wm-peer", "rehydrate-peer-uuid") - stdin, msgs, stderr, cleanup := startBrokerWithDirs(t, t.TempDir(), fx.nodeDir, + stdin, msgs, stderr, cleanup := startBrokerWithDirsAndEnv(t, t.TempDir(), fx.nodeDir, proxyPortEnv(t), "--proxy-path", proxyBin, "--workload-manager-path", workloadMgrBin, ) @@ -274,9 +268,6 @@ func TestWorkloadManagerRehydratesActiveWorkloadOnRestart(t *testing.T) { // state — otherwise a peer's wrongly-inferred "failed" (or a missed terminal) // can never be repaired after a restart. func TestWorkloadManagerRehydratesRecentTerminalOnRestart(t *testing.T) { - if portBusy(11435) { - t.Skip("ollama-proxy default port 11435 already in use; skipping") - } // Fake Ollama that completes immediately, so the workload reaches a terminal // (completed) state right away. @@ -292,7 +283,7 @@ func TestWorkloadManagerRehydratesRecentTerminalOnRestart(t *testing.T) { fx := newInterNodeCluster(t) received := fx.startStubClusterPeer(t, "rehydrate-term-peer", "rehydrate-term-peer-uuid") - stdin, msgs, stderr, cleanup := startBrokerWithDirs(t, t.TempDir(), fx.nodeDir, + stdin, msgs, stderr, cleanup := startBrokerWithDirsAndEnv(t, t.TempDir(), fx.nodeDir, proxyPortEnv(t), "--proxy-path", proxyBin, "--workload-manager-path", workloadMgrBin, ) diff --git a/services/tests/workload_interop_test.go b/services/tests/workload_interop_test.go index b1593d56..8a5c9d76 100644 --- a/services/tests/workload_interop_test.go +++ b/services/tests/workload_interop_test.go @@ -72,17 +72,34 @@ type wlParams struct { // plane use startBrokerProcInCluster. func startBrokerProc(t *testing.T, args ...string) (io.WriteCloser, <-chan jsonrpc.Message, func()) { t.Helper() - return startBrokerProcInCluster(t, t.TempDir(), args...) + return startBrokerProcWithEnv(t, nil, args...) +} + +// startBrokerProcWithEnv is startBrokerProc plus env entries for the broker +// process (service-port overrides and the like). +func startBrokerProcWithEnv(t *testing.T, extraEnv []string, args ...string) (io.WriteCloser, <-chan jsonrpc.Message, func()) { + t.Helper() + return startBrokerProcInClusterWithEnv(t, t.TempDir(), extraEnv, args...) } // startBrokerProcInCluster starts the broker with clusterDir as its cluster // identity, so its cluster-scoped workers come up as live members of whatever // cluster that directory describes (see newInterNodeCluster). func startBrokerProcInCluster(t *testing.T, clusterDir string, args ...string) (io.WriteCloser, <-chan jsonrpc.Message, func()) { + t.Helper() + return startBrokerProcInClusterWithEnv(t, clusterDir, nil, args...) +} + +// startBrokerProcInClusterWithEnv is startBrokerProcInCluster plus env entries +// passed to the broker process (service-port overrides and the like). +func startBrokerProcInClusterWithEnv(t *testing.T, clusterDir string, extraEnv []string, args ...string) (io.WriteCloser, <-chan jsonrpc.Message, func()) { t.Helper() args = append([]string{"--cluster-dir", clusterDir}, args...) cmd := exec.Command(brokerBin, args...) cmd.Stderr = os.Stderr + if len(extraEnv) > 0 { + cmd.Env = append(os.Environ(), extraEnv...) + } stdinPipe, err := cmd.StdinPipe() if err != nil { @@ -111,6 +128,17 @@ func startBrokerProcInCluster(t *testing.T, clusterDir string, args ...string) ( } } +// workloadEventsEndpoint returns the supervised workload-manager's inter-node +// endpoint on the broker under test. The port is the NVPAIR_SERVICE_WORKLOAD_PORT +// override when set (startBrokerProc*WithEnv), else the spec default. +func workloadEventsEndpoint(t *testing.T) string { + t.Helper() + if v := os.Getenv("NVPAIR_SERVICE_WORKLOAD_PORT"); v != "" { + return fmt.Sprintf("https://127.0.0.1:%s/v1/workloads/events", v) + } + return "https://127.0.0.1:14320/v1/workloads/events" +} + // writeRawFrame writes a raw JSON-RPC frame (newline-terminated) to the // broker. (The package's other writeFrame takes a typed brokerFrame.) func writeRawFrame(t *testing.T, w io.Writer, frame string) { @@ -145,9 +173,9 @@ func TestWorkloadManagerInboundRelay(t *testing.T) { t.Log("subscribed to workloads stream") // POST a peer workload to the manager's inter-node endpoint over cluster mTLS. - // The manager binds :14320 in a goroutine, so retry until it accepts. + // The manager binds its inter-node port in a goroutine, so retry until it accepts. const peerEvent = `{"jsonrpc":"2.0","method":"workload:started","params":{"workloadInfo":{"id":"peer-wl-1","model":"llama-3-70b","engine":"trt-llm","state":"running","originatedFrom":"peer-node-A","createdAt":1716998400000,"startedAt":1716998400000,"completedAt":null,"error":null,"requesterId":null}}}` - postPeerEvent(t, fx.clientAsPeer(t), "https://127.0.0.1:14320/v1/workloads/events", peerEvent, 15*time.Second) + postPeerEvent(t, fx.clientAsPeer(t), workloadEventsEndpoint(t), peerEvent, 15*time.Second) t.Log("peer workload accepted by manager") // The manager translates the peer lifecycle event into workloads:upsert @@ -184,7 +212,7 @@ func TestWorkloadOutOfOrderSuppressed(t *testing.T) { t.Fatalf("subscribe ack = %s (err %v), want subscribed:true", ack.Result, err) } - const endpoint = "https://127.0.0.1:14320/v1/workloads/events" + endpoint := workloadEventsEndpoint(t) peer := fx.clientAsPeer(t) // Same (originatedFrom, id, createdAt) — one workload, two lifecycle events, // delivered terminal-first (the inversion the network can produce). @@ -311,7 +339,7 @@ func TestWorkloadFailedOnNodeLoss(t *testing.T) { // tracked workloads:upsert. Stamped with the peer's HostUUID (not its // name), as a real proxy does. peerEvent := fmt.Sprintf(`{"jsonrpc":"2.0","method":"workload:started","params":{"workloadInfo":{"id":"wl-nodeloss-1","model":"llama-3-8b","engine":"ollama","state":"running","originatedFrom":%q,"scheduledOn":%q,"createdAt":1716998400000,"startedAt":1716998400000,"completedAt":null,"error":null,"requesterId":null}}}`, peerUUID, peerUUID) - postPeerEvent(t, fx.clientAsPeer(t), "https://127.0.0.1:14320/v1/workloads/events", peerEvent, 15*time.Second) + postPeerEvent(t, fx.clientAsPeer(t), workloadEventsEndpoint(t), peerEvent, 15*time.Second) running := waitForWorkloadEvent(t, msgs, "workloads:upsert", "wl-nodeloss-1", 10*time.Second) if running.WorkloadInfo.State != "running" { @@ -365,7 +393,7 @@ func TestWorkloadManagerOutboundBroadcast(t *testing.T) { fx := newInterNodeCluster(t) received := fx.startStubClusterPeer(t, "stub-wm-peer", "stub-wm-peer-uuid") - stdin, msgs, cleanup := startBrokerProcInCluster(t, fx.nodeDir, + stdin, msgs, cleanup := startBrokerProcInClusterWithEnv(t, fx.nodeDir, proxyPortEnv(t), "--scanner-path", scannerBin, "--proxy-path", proxyBin, "--workload-manager-path", workloadMgrBin, diff --git a/services/versions.json b/services/versions.json index 29d8c230..1bb981b8 100644 --- a/services/versions.json +++ b/services/versions.json @@ -3,18 +3,18 @@ "product": "0.91.7", "installer": "0.91.7", "components": { - "ollama-proxy": "0.26.2", - "lmstudio-proxy": "0.16.2", + "ollama-proxy": "0.26.3", + "lmstudio-proxy": "0.16.3", "nvpair-node-info": "0.13.3", - "nvpair-node-scanner": "0.20.3", - "nvpair-manual-nodes": "0.11.1", - "nvpair-workload-manager": "0.13.3", - "nvpair-errors": "0.7.4", - "nvpair-node-settings": "1.0.4", - "nvpair-ui-broker": "0.40.2", - "nvpair-engine-manager": "0.17.4", - "nvpair-cluster-manager": "1.1.4", - "nvpair-job-scheduler": "0.4.1", - "nvpair-tui": "0.7.2" + "nvpair-node-scanner": "0.20.4", + "nvpair-manual-nodes": "0.11.2", + "nvpair-workload-manager": "0.13.4", + "nvpair-errors": "0.7.5", + "nvpair-node-settings": "1.0.5", + "nvpair-ui-broker": "0.40.3", + "nvpair-engine-manager": "0.17.5", + "nvpair-cluster-manager": "1.1.5", + "nvpair-job-scheduler": "0.4.2", + "nvpair-tui": "0.7.3" } } From b2f34b6d96370443c6b4dbc2c0b5203d0b938d15 Mon Sep 17 00:00:00 2001 From: woodsonl <65194841+woodsonl@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:31:53 +0000 Subject: [PATCH 2/5] fix: close adversarial-review findings in cluster-manager and manifests - 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> --- services/nvpair-cluster-manager/cancel.go | 5 ++++- services/nvpair-cluster-manager/httpserver.go | 4 ++-- .../nvpair-engine-manager/manifests/lmstudio.json | 6 +++--- services/nvpair-engine-manager/manifests/ollama.json | 12 ++++++------ 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/services/nvpair-cluster-manager/cancel.go b/services/nvpair-cluster-manager/cancel.go index 546a47a2..20f18553 100644 --- a/services/nvpair-cluster-manager/cancel.go +++ b/services/nvpair-cluster-manager/cancel.go @@ -78,9 +78,12 @@ func (m *Manager) handleCancelInvite(msg *Message) { signalKey, keyErr := sess.ephemeralKey() if keyErr != nil { // No Key Exchange secret (initial exchange never completed) — the joiner - // has no session to clear either, so skipping the notify is safe. + // has no session to clear either, so skipping the notify is safe. The + // terminal write and session delete stay under sess.mu, matching the + // success path below and keeping this serialized against Completion. m.finishInvite(p.InviteID, inviteStateCanceled) m.deleteSession(p.InviteID) + sess.mu.Unlock() m.maybeLeaveInviteCreatedClusterLocked() m.inviteMu.Unlock() log.Printf("invite %s: canceled by inviter (no joiner signal key; skipping notify: %v)", p.InviteID, keyErr) diff --git a/services/nvpair-cluster-manager/httpserver.go b/services/nvpair-cluster-manager/httpserver.go index 0a5c03cf..72235be9 100644 --- a/services/nvpair-cluster-manager/httpserver.go +++ b/services/nvpair-cluster-manager/httpserver.go @@ -291,11 +291,11 @@ func (m *Manager) handlePairingCompletion(w http.ResponseWriter, env *pairingEnv // session holding its Noob) is torn down, invalidating the transcript. sess.completionAttempts++ if sess.completionAttempts > maxCompletionAttempts { - sess.mu.Unlock() - m.inviteMu.Unlock() // Tear down the invite AND the EAP session: dropping the session // invalidates the Noob the attacker is testing against, so a resumed // attack would need a fresh invite (and fresh user cooperation). + // inviteMu/sess.mu stay held until return — the defers above unlock + // them, and the teardown helpers take only memMu/sessMu. m.finishInviteReason(env.InviteID, inviteStateFailed, reasonIncorrectPIN) m.deleteSession(env.InviteID) m.emitNodesChanged() diff --git a/services/nvpair-engine-manager/manifests/lmstudio.json b/services/nvpair-engine-manager/manifests/lmstudio.json index 887e5068..84565ed3 100644 --- a/services/nvpair-engine-manager/manifests/lmstudio.json +++ b/services/nvpair-engine-manager/manifests/lmstudio.json @@ -4,7 +4,7 @@ "manifest_version": 1, "detect": ["~/.lmstudio/bin/lms"], "install": { - "fetch": { "url": "https://lmstudio.ai/install.sh" }, + "fetch": { "url": "https://lmstudio.ai/install.sh", "sha256": "f6492caf54327951d55f281133d0d9646efd62985c2465c597bfff8d35b6795f" }, "run": ["bash", "{download}"], "mode": "user" }, @@ -25,7 +25,7 @@ "detect": ["%USERPROFILE%\\.lmstudio\\bin\\lms.exe"], "install": { "script": [], - "fetch": { "url": "https://lmstudio.ai/install.ps1" }, + "fetch": { "url": "https://lmstudio.ai/install.ps1", "sha256": "413d56edb320068811142501bf13211f2384eb0ad68676333b54f39b789f5df0" }, "run": ["powershell.exe", "-NoProfile", "-NonInteractive", "-ExecutionPolicy", "RemoteSigned", "-File", "{download}"], "mode": "user" }, @@ -38,7 +38,7 @@ "detect": ["%USERPROFILE%\\.lmstudio\\bin\\lms.exe"], "install": { "script": [], - "fetch": { "url": "https://lmstudio.ai/install.ps1" }, + "fetch": { "url": "https://lmstudio.ai/install.ps1", "sha256": "413d56edb320068811142501bf13211f2384eb0ad68676333b54f39b789f5df0" }, "run": ["powershell.exe", "-NoProfile", "-NonInteractive", "-ExecutionPolicy", "RemoteSigned", "-File", "{download}"], "mode": "user" }, diff --git a/services/nvpair-engine-manager/manifests/ollama.json b/services/nvpair-engine-manager/manifests/ollama.json index 6c925131..59f870dd 100644 --- a/services/nvpair-engine-manager/manifests/ollama.json +++ b/services/nvpair-engine-manager/manifests/ollama.json @@ -16,7 +16,7 @@ "windows/amd64": { "detect": ["{install_dir}\\ollama.exe", "%LOCALAPPDATA%\\Programs\\Ollama\\ollama.exe"], "install": { - "fetch": { "url": "https://ollama.com/download/ollama-windows-amd64.zip" }, + "fetch": { "url": "https://ollama.com/download/ollama-windows-amd64.zip", "sha256": "52cb36a62e7e501f61514f60212dec7117b6c098811357585e02fffe32d2fcd7" }, "run": ["tar", "-xf", "{download}", "-C", "{install_dir}"] }, "uninstall": { "run": ["cmd", "/c", "rmdir", "/s", "/q", "{install_dir}"] }, @@ -25,7 +25,7 @@ "windows/arm64": { "detect": ["{install_dir}\\ollama.exe", "%LOCALAPPDATA%\\Programs\\Ollama\\ollama.exe"], "install": { - "fetch": { "url": "https://ollama.com/download/ollama-windows-arm64.zip" }, + "fetch": { "url": "https://ollama.com/download/ollama-windows-arm64.zip", "sha256": "98b9ddaab6baece0418c6d1231526eb1e4e66944985e0a8eeb7d6171bcd7b6d8" }, "run": ["tar", "-xf", "{download}", "-C", "{install_dir}"] }, "uninstall": { "run": ["cmd", "/c", "rmdir", "/s", "/q", "{install_dir}"] }, @@ -38,7 +38,7 @@ "{install_dir}/Ollama.app/Contents/Resources/ollama" ], "install": { - "fetch": { "url": "https://ollama.com/download/Ollama-darwin.zip" }, + "fetch": { "url": "https://ollama.com/download/Ollama-darwin.zip", "sha256": "335f1a11299f5f60dc2d5f2651cf12af9d3c303812c68e978be3e45ea7d6eaf4" }, "run": ["unzip", "-o", "{download}", "-d", "{install_dir}"] }, "uninstall": { "run": ["rm", "-rf", "{install_dir}/Ollama.app"] }, @@ -51,7 +51,7 @@ "{install_dir}/Ollama.app/Contents/Resources/ollama" ], "install": { - "fetch": { "url": "https://ollama.com/download/Ollama-darwin.zip" }, + "fetch": { "url": "https://ollama.com/download/Ollama-darwin.zip", "sha256": "335f1a11299f5f60dc2d5f2651cf12af9d3c303812c68e978be3e45ea7d6eaf4" }, "run": ["unzip", "-o", "{download}", "-d", "{install_dir}"] }, "uninstall": { "run": ["rm", "-rf", "{install_dir}/Ollama.app"] }, @@ -60,7 +60,7 @@ "linux/amd64": { "detect": ["{install_dir}/bin/ollama"], "install": { - "fetch": { "url": "https://ollama.com/download/ollama-linux-amd64.tar.zst" }, + "fetch": { "url": "https://ollama.com/download/ollama-linux-amd64.tar.zst", "sha256": "c13cea8f3389db4145f8a6cb88d1747242a48639d7c13e3bda7c1ebdc6eebb2f" }, "run": ["tar", "--zstd", "-xf", "{download}", "-C", "{install_dir}"] }, "uninstall": { "run": ["rm", "-rf", "{install_dir}"] }, @@ -72,7 +72,7 @@ "linux/arm64": { "detect": ["{install_dir}/bin/ollama"], "install": { - "fetch": { "url": "https://ollama.com/download/ollama-linux-arm64.tar.zst" }, + "fetch": { "url": "https://ollama.com/download/ollama-linux-arm64.tar.zst", "sha256": "4425a112af999ae6572c1ce211fbabeaca7bab23ed5860972acdfc0cc2358420" }, "run": ["tar", "--zstd", "-xf", "{download}", "-C", "{install_dir}"] }, "uninstall": { "run": ["rm", "-rf", "{install_dir}"] }, From 8fb039c2b92f63ef24616fd5df99107a5de87215 Mon Sep 17 00:00:00 2001 From: woodsonl <65194841+woodsonl@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:32:57 +0000 Subject: [PATCH 3/5] chore: bump cluster-manager and engine-manager versions for review fixes Signed-off-by: woodsonl <65194841+woodsonl@users.noreply.github.com> --- services/versions.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/versions.json b/services/versions.json index 1bb981b8..69ea4925 100644 --- a/services/versions.json +++ b/services/versions.json @@ -12,8 +12,8 @@ "nvpair-errors": "0.7.5", "nvpair-node-settings": "1.0.5", "nvpair-ui-broker": "0.40.3", - "nvpair-engine-manager": "0.17.5", - "nvpair-cluster-manager": "1.1.5", + "nvpair-engine-manager": "0.17.6", + "nvpair-cluster-manager": "1.1.6", "nvpair-job-scheduler": "0.4.2", "nvpair-tui": "0.7.3" } From 8c5c29556d9b15809633c19c9d3d976d9cfb6cfe Mon Sep 17 00:00:00 2001 From: woodsonl <65194841+woodsonl@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:46:37 +0000 Subject: [PATCH 4/5] test: cover review-round-2 findings and fix two latent bugs 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> --- desktop/src/electron/ipc/safe-handle.ts | 3 +- .../tests/modular/safe-handle-sender.test.ts | 137 ++++++++++ services/eap-noob/ephemeral_key_test.go | 54 ++++ services/lmstudio-proxy/body_limit_test.go | 66 +++++ services/lmstudio-proxy/ingress_test.go | 40 +++ .../pairing_signal_gate_test.go | 254 ++++++++++++++++++ services/nvpair-ui-broker/broker.go | 18 +- services/nvpair-ui-broker/ollamahost_test.go | 48 ++++ services/nvpair-ui-broker/relay/relay.go | 9 +- services/nvpair-ui-broker/relay/relay_test.go | 40 ++- .../nvpair-ui-broker/terminal_read_test.go | 100 +++++++ services/ollama-proxy/body_limit_test.go | 66 +++++ services/ollama-proxy/ingress_test.go | 40 +++ services/versions.json | 2 +- 14 files changed, 871 insertions(+), 6 deletions(-) create mode 100644 desktop/tests/modular/safe-handle-sender.test.ts create mode 100644 services/eap-noob/ephemeral_key_test.go create mode 100644 services/lmstudio-proxy/body_limit_test.go create mode 100644 services/nvpair-cluster-manager/pairing_signal_gate_test.go create mode 100644 services/nvpair-ui-broker/terminal_read_test.go create mode 100644 services/ollama-proxy/body_limit_test.go diff --git a/desktop/src/electron/ipc/safe-handle.ts b/desktop/src/electron/ipc/safe-handle.ts index 1d15cbd2..4f674772 100644 --- a/desktop/src/electron/ipc/safe-handle.ts +++ b/desktop/src/electron/ipc/safe-handle.ts @@ -17,7 +17,8 @@ function isKnownSender(event: IpcMainInvokeEvent): boolean { const url = event.sender.getURL() if (url.startsWith('file://')) return true const devUrl = process.env.ELECTRON_RENDERER_URL - if (devUrl !== undefined && devUrl !== '' && url.startsWith(devUrl)) return true + if (devUrl !== undefined && devUrl !== '' && (url === devUrl || url.startsWith(devUrl + '/'))) + return true return false } diff --git a/desktop/tests/modular/safe-handle-sender.test.ts b/desktop/tests/modular/safe-handle-sender.test.ts new file mode 100644 index 00000000..07e931aa --- /dev/null +++ b/desktop/tests/modular/safe-handle-sender.test.ts @@ -0,0 +1,137 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { afterEach, describe, expect, it, vi } from 'vitest' + +interface SenderFixture { + url: string + window: object | null + getURL: () => string +} + +const mocks = vi.hoisted(() => ({ + sender: null as SenderFixture | null, + handlers: new Map unknown>() +})) + +vi.mock('electron', () => ({ + BrowserWindow: { + fromWebContents: (contents: SenderFixture | null) => (contents ? contents.window : null) + }, + ipcMain: { + handle: (channel: string, fn: (event: unknown, ...args: unknown[]) => unknown) => { + mocks.handlers.set(channel, fn) + } + } +})) + +import { safeHandle } from '@/electron/ipc/safe-handle' + +function senderWith(url: string, withWindow = true): { sender: SenderFixture } { + mocks.sender = { url, window: withWindow ? {} : null, getURL: () => url } + return { sender: mocks.sender } +} + +function invoke(channel: string, url: string, withWindow = true) { + const handler = mocks.handlers.get(channel) + if (!handler) throw new Error(`no handler registered for ${channel}`) + const ev = senderWith(url, withWindow).sender + return handler({ sender: ev } as never) +} + +const previousDevUrl = process.env.ELECTRON_RENDERER_URL + +afterEach(() => { + mocks.handlers.clear() + mocks.sender = null + if (previousDevUrl === undefined) { + delete process.env.ELECTRON_RENDERER_URL + } else { + process.env.ELECTRON_RENDERER_URL = previousDevUrl + } +}) + +describe('safeHandle sender authorization', () => { + it('registers the handler and authorizes a sender on the dev URL', async () => { + process.env.ELECTRON_RENDERER_URL = 'http://localhost:5173' + safeHandle('test:sender-ok' as never, (() => 'ok') as never) + await expect(invoke('test:sender-ok', 'http://localhost:5173/app')).resolves.toEqual({ + success: true, + data: 'ok' + }) + }) + + it('authorizes a file:// sender regardless of the dev URL', async () => { + delete process.env.ELECTRON_RENDERER_URL + safeHandle('test:sender-file' as never, (() => 'ok') as never) + await expect(invoke('test:sender-file', 'file://app/index.html')).resolves.toEqual({ + success: true, + data: 'ok' + }) + }) + + it('rejects a sender when ELECTRON_RENDERER_URL is unset', async () => { + delete process.env.ELECTRON_RENDERER_URL + safeHandle('test:sender-nor-dev' as never, (() => 'ok') as never) + await expect(invoke('test:sender-nor-dev', 'http://localhost:5173/app')).resolves.toEqual({ + success: false, + error: 'Unauthorized sender' + }) + }) + + it('rejects a sender when ELECTRON_RENDERER_URL is set to an empty string', async () => { + process.env.ELECTRON_RENDERER_URL = '' + safeHandle('test:sender-empty-dev' as never, (() => 'ok') as never) + await expect(invoke('test:sender-empty-dev', 'http://anything.test/app')).resolves.toEqual({ + success: false, + error: 'Unauthorized sender' + }) + }) + + it('authorizes an exact dev-URL match with no path', async () => { + process.env.ELECTRON_RENDERER_URL = 'http://localhost:5173' + safeHandle('test:sender-exact' as never, (() => 'ok') as never) + await expect(invoke('test:sender-exact', 'http://localhost:5173')).resolves.toEqual({ + success: true, + data: 'ok' + }) + }) + + it('rejects a sender whose URL only shares a prefix with the dev URL (lookalike host)', async () => { + process.env.ELECTRON_RENDERER_URL = 'http://localhost:5173' + safeHandle('test:sender-lookalike' as never, (() => 'ok') as never) + await expect( + invoke('test:sender-lookalike', 'http://localhost:5173.evil.test/app') + ).resolves.toEqual({ + success: false, + error: 'Unauthorized sender' + }) + }) + + it('rejects a sender with dev-URL userinfo spoofing', async () => { + process.env.ELECTRON_RENDERER_URL = 'http://localhost:5173' + safeHandle('test:sender-userinfo' as never, (() => 'ok') as never) + await expect( + invoke('test:sender-userinfo', 'http://localhost:5173@evil.test/') + ).resolves.toEqual({ + success: false, + error: 'Unauthorized sender' + }) + }) + + it('rejects a sender with no attached BrowserWindow', async () => { + process.env.ELECTRON_RENDERER_URL = 'http://localhost:5173' + safeHandle('test:sender-nowin' as never, (() => 'ok') as never) + const handler = mocks.handlers.get('test:sender-nowin') + if (!handler) throw new Error('handler missing') + mocks.sender = { + url: 'http://localhost:5173/app', + window: null, + getURL: () => 'http://localhost:5173/app' + } + await expect(handler({ sender: mocks.sender } as never)).resolves.toEqual({ + success: false, + error: 'Unauthorized sender' + }) + }) +}) diff --git a/services/eap-noob/ephemeral_key_test.go b/services/eap-noob/ephemeral_key_test.go new file mode 100644 index 00000000..08440d97 --- /dev/null +++ b/services/eap-noob/ephemeral_key_test.go @@ -0,0 +1,54 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package eapnoob + +import ( + "bytes" + "testing" +) + +// TestEphemeralKeyLifecycle covers the export the cluster manager's signal MAC +// derives from: it must error before a Key Exchange ran, agree between server +// and peer after the Initial Exchange, and hand out a copy so a caller cannot +// corrupt the live secret. +func TestEphemeralKeyLifecycle(t *testing.T) { + srv := NewServer(ServerConfig{Dirs: 2, ServerInfo: map[string]any{"role": "server"}}, nil) + peer := NewPeer(PeerConfig{PreferDir: 2, PeerInfo: map[string]any{"role": "peer"}}, nil) + + if _, err := srv.EphemeralKey(); err == nil { + t.Fatal("server EphemeralKey succeeded before any Key Exchange") + } + if _, err := peer.EphemeralKey(); err == nil { + t.Fatal("peer EphemeralKey succeeded before any Key Exchange") + } + + driveConversation(t, srv, peer) + if srv.State() != StateWaiting || peer.State() != StateWaiting { + t.Fatalf("after Initial: server=%s peer=%s, want waiting on both", srv.State(), peer.State()) + } + + srvKey, err := srv.EphemeralKey() + if err != nil { + t.Fatalf("server EphemeralKey after Initial Exchange: %v", err) + } + peerKey, err := peer.EphemeralKey() + if err != nil { + t.Fatalf("peer EphemeralKey after Initial Exchange: %v", err) + } + if len(srvKey) == 0 { + t.Fatal("server key is empty after a Key Exchange") + } + if !bytes.Equal(srvKey, peerKey) { + t.Fatal("server and peer ephemeral keys differ after the same Initial Exchange") + } + + srvKey[0] ^= 0xFF + again, err := srv.EphemeralKey() + if err != nil { + t.Fatalf("server EphemeralKey re-read: %v", err) + } + if !bytes.Equal(again, peerKey) { + t.Fatal("mutating a returned key changed the server's secret; EphemeralKey must return a copy") + } +} diff --git a/services/lmstudio-proxy/body_limit_test.go b/services/lmstudio-proxy/body_limit_test.go new file mode 100644 index 00000000..0d90bcb1 --- /dev/null +++ b/services/lmstudio-proxy/body_limit_test.go @@ -0,0 +1,66 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestBufferBodyAndModelLimit covers the body-cap contract: a body over +// maxInferenceBodyBytes is reported too-large with the body dropped (never +// buffered into memory), and a body within the limit is returned with its +// parsed model field. +func TestBufferBodyAndModelLimit(t *testing.T) { + t.Run("body over the limit is too large", func(t *testing.T) { + big := bytes.Repeat([]byte("a"), maxInferenceBodyBytes+1) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", bytes.NewReader(big)) + body, model, tooLarge := bufferBodyAndModel(req) + if !tooLarge { + t.Fatal("bufferBodyAndModel tooLarge = false, want true past the cap") + } + if body != nil { + t.Error("body should be dropped (nil) when too large, not buffered") + } + if model != "" { + t.Errorf("model = %q, want empty when too large", model) + } + }) + + t.Run("body at the limit parses", func(t *testing.T) { + atLimit := bytes.Repeat([]byte("a"), maxInferenceBodyBytes) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", bytes.NewReader(atLimit)) + _, _, tooLarge := bufferBodyAndModel(req) + if tooLarge { + t.Fatal("bufferBodyAndModel tooLarge = true at exactly the cap, want false") + } + }) + + t.Run("model parsed from a small body", func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama3"}`)) + body, model, tooLarge := bufferBodyAndModel(req) + if tooLarge || model != "llama3" || string(body) != `{"model":"llama3"}` { + t.Fatalf("= (%q, %q, %v), want body kept, model llama3, not too large", body, model, tooLarge) + } + }) +} + +// TestHandleHTTPRejectsOversizedBody drives the 413 path end to end: a request +// body past maxInferenceBodyBytes is refused before any candidate resolution +// or upstream forward, with StatusRequestEntityTooLarge. +func TestHandleHTTPRejectsOversizedBody(t *testing.T) { + p := testProxy(NewDiscovery(), 1235) + big := bytes.Repeat([]byte("a"), maxInferenceBodyBytes+1) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", bytes.NewReader(big)) + req.RemoteAddr = "127.0.0.1:40000" + rec := httptest.NewRecorder() + + p.handleHTTP(rec, req) + if rec.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("oversized body status = %d, want %d", rec.Code, http.StatusRequestEntityTooLarge) + } +} diff --git a/services/lmstudio-proxy/ingress_test.go b/services/lmstudio-proxy/ingress_test.go index 9f12b99b..93f6ee68 100644 --- a/services/lmstudio-proxy/ingress_test.go +++ b/services/lmstudio-proxy/ingress_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "testing" ) @@ -114,3 +115,42 @@ func TestLocalReverseProxyUsesSharedPlainTransport(t *testing.T) { t.Fatal("ingress reverse proxy did not use the shared plain Transport") } } + +// TestHandlePlainGatesLoopbackCrossOrigin: the loopback gate does not exclude +// browsers (they connect from loopback), so a simple cross-origin POST from an +// origin the allowlist does not name must be refused with the proxy's own 403, +// and an allowlisted origin must pass through to the router. +func TestHandlePlainGatesLoopbackCrossOrigin(t *testing.T) { + t.Run("unlisted origin is refused with 403 origin-not-allowed", func(t *testing.T) { + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "") + p := testProxy(NewDiscovery(), 1235) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil) + req.RemoteAddr = "127.0.0.1:40000" + req.Header.Set("Origin", "https://evil.example") + rec := httptest.NewRecorder() + + p.handlePlain(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("cross-origin loopback status = %d, want %d", rec.Code, http.StatusForbidden) + } + if !strings.Contains(rec.Body.String(), "origin-not-allowed") { + t.Errorf("body = %q, want the origin-not-allowed code", rec.Body.String()) + } + }) + t.Run("allowlisted origin passes the gate", func(t *testing.T) { + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + p := testProxy(NewDiscovery(), 1235) + req := httptest.NewRequest(http.MethodGet, "/v1/models", nil) + req.RemoteAddr = "127.0.0.1:40000" + req.Header.Set("Origin", "https://ui.example") + // The engine-manager identity marker answers 409 only after the origin + // gate, so a 409 proves the allowlisted caller reached the router. + req.Header.Set(engineIdentityProbeHeader, "1") + rec := httptest.NewRecorder() + + p.handlePlain(rec, req) + if rec.Code != http.StatusConflict { + t.Fatalf("allowlisted-origin status = %d, want %d (gate pass-through)", rec.Code, http.StatusConflict) + } + }) +} diff --git a/services/nvpair-cluster-manager/pairing_signal_gate_test.go b/services/nvpair-cluster-manager/pairing_signal_gate_test.go new file mode 100644 index 00000000..a3f3661d --- /dev/null +++ b/services/nvpair-cluster-manager/pairing_signal_gate_test.go @@ -0,0 +1,254 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "eapnoob" + "encoding/base64" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +// postPairing drives one plain-HTTP pairing envelope through handlePairing via +// a real server mux, so status-code gates are exercised the way a remote peer +// hits them. +func postPairing(t *testing.T, m *Manager, env pairingEnvelope) *httptest.ResponseRecorder { + t.Helper() + body, err := json.Marshal(env) + if err != nil { + t.Fatalf("marshal envelope: %v", err) + } + mux := http.NewServeMux() + mux.HandleFunc(pairingPath, m.handlePairing) + req := httptest.NewRequest(http.MethodPost, pairingPath, bytes.NewReader(body)) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + return rec +} + +// signalTagFor computes the terminal-signal MAC the same way the joiner would. +func signalTagFor(t *testing.T, sess *pairingSession, inviteID, phase string) string { + t.Helper() + key, err := sess.ephemeralKey() + if err != nil { + t.Fatalf("session ephemeral key: %v", err) + } + return pairingSignalMAC(key, inviteID, phase) +} + +// putPendingInviterSessionWithEAP registers a pending outbound invite plus an +// inviter-role session carrying a live EAP-NOOB Server that has run a Key +// Exchange (so the ephemeral secret exists and signal MACs can be derived), +// mirroring the state runInitialExchange records once the joiner's Initial +// Exchange completes its Key Exchange round. +func putPendingInviterSessionWithEAP(t *testing.T, m *Manager, inviteID string) *pairingSession { + t.Helper() + info, err := m.localPairingInfo("127.0.0.1:1").toMap() + if err != nil { + t.Fatalf("pairing info: %v", err) + } + server := newPairingServer(info) + // Drive a real Initial Exchange against a Peer so the server holds the + // ECDH shared secret (z) the signal MAC derives from. PeerInfo mirrors a + // joiner's pre-adoption identity (no cluster yet). + peer := eapnoob.NewPeer(eapnoob.PeerConfig{PreferDir: 2, PeerInfo: map[string]any{"role": "peer"}}, nil) + msg, err := server.Start() + if err != nil { + t.Fatalf("server start: %v", err) + } + peerTurn := true + for { + var out eapnoob.Outcome + if peerTurn { + out, err = peer.Receive(msg) + } else { + out, err = server.Receive(msg) + } + if err != nil || out.Err != nil { + t.Fatalf("initial exchange round: err=%v protocolErr=%v", err, out.Err) + } + if len(out.Send) == 0 { + break + } + msg = out.Send + peerTurn = !peerTurn + } + if server.State() != eapnoob.StateWaiting { + t.Fatalf("server state %s after Initial Exchange, want waiting", server.State()) + } + sess := &pairingSession{ + inviteID: inviteID, + role: roleInviter, + createdAt: time.Now().UnixMilli(), + server: server, + } + m.putInvite(&Invite{ + InviteID: inviteID, + FromNodeUUID: m.identity.NodeUUID, + State: inviteStatePending, + CreatedAt: time.Now().UnixMilli(), + }) + m.putSession(sess) + return sess +} + +// TestPairingSignalGateRejectsUnauthenticated tests the 401 gate added in front +// of the cancel/decline/expire signal phases: a missing tag, a garbage tag, and +// a tag MACed for the wrong phase or wrong invite all get rejected, and none of +// them may tear down the invite or session they target. A correctly MACed tag +// passes the same gate. +func TestPairingSignalGateRejectsUnauthenticated(t *testing.T) { + for _, phase := range []string{"cancel", "decline", "expire"} { + t.Run(phase, func(t *testing.T) { + m := newTestManager(t) + m.addSelfMember() + + cases := []struct { + name string + tag func(freshSess *pairingSession) string + inviteID string + }{ + // Every tag is computed against the fresh session re-put for + // the case, so the wrong-phase and wrong-invite negatives + // fail on exactly the dimension under test — not on a stale + // key from an earlier session. + {"missing tag", func(*pairingSession) string { return "" }, "inv-gate"}, + {"garbage tag", func(*pairingSession) string { return "not-a-mac" }, "inv-gate"}, + {"wrong phase tag", func(s *pairingSession) string { return signalTagFor(t, s, "inv-gate", "fail") }, "inv-gate"}, + {"wrong invite tag", func(s *pairingSession) string { return signalTagFor(t, s, "inv-other", phase) }, "inv-gate"}, + {"valid tag", func(s *pairingSession) string { return signalTagFor(t, s, "inv-gate", phase) }, "inv-gate"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Re-put a pending invite/session for every case so a + // preceding teardown never masks the gate under test. + freshSess := putPendingInviterSessionWithEAP(t, m, "inv-gate") + rec := postPairing(t, m, pairingEnvelope{ + InviteID: tc.inviteID, Phase: phase, SignalTag: tc.tag(freshSess), + }) + // The valid tag is admitted past the 401 gate (the phase + // handler then answers with its own status); every other + // case must hit the gate and leave state untouched. + if tc.name == "valid tag" { + if rec.Code == http.StatusUnauthorized { + t.Fatalf("valid %s tag rejected with 401; gate is too strict", phase) + } + return + } + if rec.Code != http.StatusUnauthorized { + t.Fatalf("%s: status = %d, want 401", tc.name, rec.Code) + } + if inv, ok := m.getInvite("inv-gate"); !ok || inv.State != inviteStatePending { + t.Fatalf("%s: unauthenticated signal tore down the invite (state=%v, present=%v)", tc.name, inv.State, ok) + } + if _, ok := m.getSession("inv-gate"); !ok { + t.Fatalf("%s: unauthenticated signal dropped the session", tc.name) + } + }) + } + }) + } +} + +// TestCompletionAttemptsRateLimit verifies the PIN brute-force cap: attempts 1 +// through maxCompletionAttempts are not limited, the next one returns 429, and +// the over-limit attempt tears down both the invite (failed/incorrect-pin) and +// the EAP session so a resumed attack needs a fresh invite. +func TestCompletionAttemptsRateLimit(t *testing.T) { + m := newTestManager(t) + m.addSelfMember() + sess := putPendingInviterSessionWithEAP(t, m, "inv-rate") + if sess.server == nil { + t.Fatal("inviter session with EAP server missing") + } + + for i := 1; i <= maxCompletionAttempts; i++ { + // A non-empty (garbage) EAP message reaches the attempt counter; an + // empty msg is the kickoff POST and returns before it. + rec := postPairing(t, m, pairingEnvelope{InviteID: "inv-rate", Phase: "completion", Msg: base64.StdEncoding.EncodeToString([]byte("guess"))}) + if rec.Code == http.StatusTooManyRequests { + t.Fatalf("attempt %d (of %d allowed) was rate limited", i, maxCompletionAttempts) + } + if rec.Code >= 500 { + t.Fatalf("attempt %d: unexpected server error %d", i, rec.Code) + } + } + + rec := postPairing(t, m, pairingEnvelope{InviteID: "inv-rate", Phase: "completion", Msg: base64.StdEncoding.EncodeToString([]byte("guess"))}) + if rec.Code != http.StatusTooManyRequests { + t.Fatalf("over-limit completion status = %d, want %d", rec.Code, http.StatusTooManyRequests) + } + if inv, ok := m.getInvite("inv-rate"); !ok || inv.State != inviteStateFailed || inv.Reason != reasonIncorrectPIN { + t.Fatalf("invite after over-limit = %+v (present %v), want failed/incorrect-pin", inv, ok) + } + if _, ok := m.getSession("inv-rate"); ok { + t.Fatal("over-limit completion left the EAP session alive; a resumed attack keeps its transcript") + } +} + +// TestCancelInviteNoSignalKey exercises the cancel branch that runs before the +// Initial Exchange completed: the session has no ephemeral key, so the notify +// is skipped, but the invite must still be canceled and the session deleted, +// with the terminal write staying serialized under sess.mu. The codec writer is +// a bytes.Buffer, so the response frame is captured and its error payload +// asserted too. +func TestCancelInviteNoSignalKey(t *testing.T) { + var out bytes.Buffer + codec := NewCodec(struct { + io.Reader + io.Writer + }{strings.NewReader(""), &out}) + dir := t.TempDir() + mgr, err := NewManager(codec, dir, 14999) + if err != nil { + t.Fatalf("new manager: %v", err) + } + if _, err := mgr.ensureAdmission("cluster-1"); err != nil { + t.Fatalf("establish admission: %v", err) + } + mgr.setClusterIdentity("cluster-1", "Lab") + m := mgr + m.addSelfMember() + + // A pending inviter session whose EAP server never started holds no Key + // Exchange secret — exactly the state a cancel racing the Initial Exchange + // observes. + m.putInvite(&Invite{ + InviteID: "inv-nokey", + FromNodeUUID: m.identity.NodeUUID, + State: inviteStatePending, + CreatedAt: time.Now().UnixMilli(), + }) + m.putSession(&pairingSession{inviteID: "inv-nokey", role: roleInviter}) + + id := json.RawMessage(`"cancel-nokey-1"`) + params, err := json.Marshal(map[string]string{"inviteId": "inv-nokey"}) + if err != nil { + t.Fatalf("marshal params: %v", err) + } + m.handleCancelInvite(&Message{ID: &id, Method: "cluster:cancel-invite", Params: params}) + + if inv, ok := m.getInvite("inv-nokey"); !ok || inv.State != inviteStateCanceled { + t.Fatalf("invite state = %+v (present %v), want canceled", inv, ok) + } + if _, ok := m.getSession("inv-nokey"); ok { + t.Fatal("session survived a no-signal-key cancel") + } + resp, err := io.ReadAll(strings.NewReader(out.String())) + if err != nil { + t.Fatalf("read response: %v", err) + } + if !strings.Contains(string(resp), "invite is not pending") { + t.Fatalf("cancel response %q does not report the invalid-state error", string(resp)) + } + if !strings.Contains(string(resp), string(inviteStateCanceled)) { + t.Fatalf("cancel response %q does not carry the canceled state", string(resp)) + } +} diff --git a/services/nvpair-ui-broker/broker.go b/services/nvpair-ui-broker/broker.go index 16c432a0..886acb3e 100644 --- a/services/nvpair-ui-broker/broker.go +++ b/services/nvpair-ui-broker/broker.go @@ -2198,6 +2198,15 @@ func setNodeIDIfEmpty(m map[string]json.RawMessage, key, nodeID string) bool { return true } +// recoverableDecode reports whether a codec Read error is a recoverable +// per-frame decode failure (bad JSON / wrong version): the scanner advances +// past the bad frame, so both the producer and the consumer keep pumping +// instead of tearing the connection down. +func recoverableDecode(err error) bool { + var de *DecodeError + return errors.As(err, &de) +} + func (b *Broker) readLoop(ctx context.Context) error { // codec.Read() blocks on stdin, so we run it on its own goroutine and // select against ctx.Done(). Otherwise a SIGINT/SIGTERM (which cancels @@ -2225,8 +2234,7 @@ func (b *Broker) readLoop(ctx context.Context) error { if err == nil { continue } - var de *DecodeError - if errors.As(err, &de) { + if recoverableDecode(err) { continue } return @@ -2265,6 +2273,12 @@ func (b *Broker) readLoop(ctx context.Context) error { return nil case r := <-reads: if r.err != nil { + // Recoverable per-frame decode failure: the producer keeps + // pumping and the next Read advances past the bad frame, so + // skip it here too instead of tearing down the connection. + if recoverableDecode(r.err) { + continue + } if r.err == io.EOF || ctx.Err() != nil { return nil } diff --git a/services/nvpair-ui-broker/ollamahost_test.go b/services/nvpair-ui-broker/ollamahost_test.go index 6ae14ed5..a7890107 100644 --- a/services/nvpair-ui-broker/ollamahost_test.go +++ b/services/nvpair-ui-broker/ollamahost_test.go @@ -614,3 +614,51 @@ func TestStaleAliasBindFailureDoesNotReleaseReplacementReservation(t *testing.T) t.Fatal("stale failure published a warning for the replacement generation") } } + +// TestResolveServicePorts covers the env-override contract: spec defaults when +// nothing is set, per-service overrides when valid, and out-of-range or +// non-numeric values ignored with the default kept. +func TestResolveServicePorts(t *testing.T) { + t.Run("defaults with no overrides", func(t *testing.T) { + ports := resolveServicePorts(func(string) string { return "" }) + if ports.NodeInfo != nodeInfoHTTPPort || ports.Errors != errorsHTTPPort || + ports.Workload != workloadHTTPPort || ports.ClusterManager != clusterManagerHTTPPort || + ports.EngineHTTP != engineManagerHTTPPort || ports.EngineControl != engineControlPort || + ports.OllamaProxy != 0 || ports.LMStudioProxy != 0 { + t.Fatalf("defaults = %+v, want spec defaults and zero proxy ports", ports) + } + }) + + t.Run("valid overrides applied", func(t *testing.T) { + env := map[string]string{ + "NVPAIR_SERVICE_NODE_INFO_PORT": "24318", + "NVPAIR_SERVICE_ERRORS_PORT": "24319", + "NVPAIR_SERVICE_WORKLOAD_PORT": "24320", + "NVPAIR_SERVICE_CLUSTER_MANAGER_PORT": "24321", + "NVPAIR_SERVICE_ENGINE_HTTP_PORT": "24322", + "NVPAIR_SERVICE_ENGINE_CONTROL_PORT": "24323", + "NVPAIR_SERVICE_OLLAMA_PROXY_PORT": "24324", + "NVPAIR_SERVICE_LMSTUDIO_PROXY_PORT": "24325", + } + ports := resolveServicePorts(func(k string) string { return env[k] }) + if ports.NodeInfo != 24318 || ports.Errors != 24319 || ports.Workload != 24320 || + ports.ClusterManager != 24321 || ports.EngineHTTP != 24322 || + ports.EngineControl != 24323 || ports.OllamaProxy != 24324 || ports.LMStudioProxy != 24325 { + t.Fatalf("overrides = %+v, want every valid override applied", ports) + } + }) + + t.Run("invalid overrides ignored", func(t *testing.T) { + env := map[string]string{ + "NVPAIR_SERVICE_NODE_INFO_PORT": "not-a-port", + "NVPAIR_SERVICE_ERRORS_PORT": "0", + "NVPAIR_SERVICE_WORKLOAD_PORT": "65536", + "NVPAIR_SERVICE_CLUSTER_MANAGER_PORT": "-1", + } + ports := resolveServicePorts(func(k string) string { return env[k] }) + if ports.NodeInfo != nodeInfoHTTPPort || ports.Errors != errorsHTTPPort || + ports.Workload != workloadHTTPPort || ports.ClusterManager != clusterManagerHTTPPort { + t.Fatalf("invalid overrides = %+v, want defaults kept", ports) + } + }) +} diff --git a/services/nvpair-ui-broker/relay/relay.go b/services/nvpair-ui-broker/relay/relay.go index 2668d0c4..678331f0 100644 --- a/services/nvpair-ui-broker/relay/relay.go +++ b/services/nvpair-ui-broker/relay/relay.go @@ -142,9 +142,16 @@ func (d *Directory) Subscribe(sub *Subscriber) (id int) { // pump serializes one subscriber's deliveries. Every wake re-captures the // latest filtered snapshot, so coalesced triggers always deliver current state. -// Exits on Unsubscribe (done closed). +// done takes priority over a pending kick: once Unsubscribe has closed done, a +// trigger that raced the close must not produce a Send against a consumer +// that's gone. func (d *Directory) pump(sub *Subscriber) { for { + select { + case <-sub.done: + return + default: + } select { case <-sub.kick: sub.Send(d.filtered(sub.Filter)) diff --git a/services/nvpair-ui-broker/relay/relay_test.go b/services/nvpair-ui-broker/relay/relay_test.go index e444c69d..abcf3642 100644 --- a/services/nvpair-ui-broker/relay/relay_test.go +++ b/services/nvpair-ui-broker/relay/relay_test.go @@ -6,8 +6,8 @@ package relay import ( "reflect" "sync" - "time" "testing" + "time" "nvpair-shared/noderec" ) @@ -202,3 +202,41 @@ func TestDirectorySnapshotFilterAndSort(t *testing.T) { t.Fatalf("snapshot(ol) = %d, want 2", len(ol)) } } + +// TestDeliverCoalescesTriggers guards the pump contract: Deliver is non-blocking +// and multiple pending triggers coalesce into ONE send of the latest state — a +// subscriber with a slow Send must never accumulate a backlog of stale snapshots. +func TestDeliverCoalescesTriggers(t *testing.T) { + d := NewDirectory() + rec := &recordingSub{} + sub := &Subscriber{Filter: noderec.SubscribeParams{}, Send: rec.send} + id := d.Subscribe(sub) + + d.Apply(noderec.NotifyNodeDiscovered, olNode("a")) + d.Apply(noderec.NotifyNodeDiscovered, olNode("b")) + // Three triggers while none has been consumed yet: they must coalesce. + d.Deliver(sub) + d.Deliver(sub) + d.Deliver(sub) + + got := rec.last(1) + if !reflect.DeepEqual(ids(got), []string{"a", "b"}) { + t.Fatalf("coalesced delivery = %v, want [a b] (latest state)", got) + } + + // The next Apply re-pushes current state even though its trigger coalesced + // with the earlier ones — the pump always re-captures at wake time. + d.Apply(noderec.NotifyNodeDiscovered, erNode("c")) + if got := ids(rec.last(2)); !reflect.DeepEqual(got, []string{"a", "b", "c"}) { + t.Fatalf("delivery after coalesced apply = %v, want [a b c]", got) + } + + // After Unsubscribe the pump exits: Deliver must not panic and no more + // sends may arrive. + d.Unsubscribe(id) + d.Deliver(sub) + time.Sleep(50 * time.Millisecond) + if n := rec.count(); n != 2 { + t.Errorf("post-unsubscribe deliveries = %d, want 2", n) + } +} diff --git a/services/nvpair-ui-broker/terminal_read_test.go b/services/nvpair-ui-broker/terminal_read_test.go new file mode 100644 index 00000000..5435cd13 --- /dev/null +++ b/services/nvpair-ui-broker/terminal_read_test.go @@ -0,0 +1,100 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "context" + "errors" + "io" + "net" + "testing" + "time" +) + +// errFakeTerminal is a non-EOF, non-DecodeError read failure — the shape a +// terminal scanner/transport error takes at the codec boundary. +var errFakeTerminal = errors.New("fake terminal read error") + +// errorReader fails every Read with errFakeTerminal; wrapped in a real Codec it +// stands in for a dead terminal transport. +type errorReader struct{} + +func (errorReader) Read([]byte) (int, error) { return 0, errFakeTerminal } + +func newTerminalErrorCodec() *Codec { + return NewCodec(struct { + io.Reader + io.Writer + }{errorReader{}, io.Discard}) +} + +// TestReadLoopTerminalReadErrorStopsPump guards the read-loop contract: a +// non-EOF transport error must end Serve-style pumping with errTerminalRead +// (the producer goroutine has already stopped; spinning would burn CPU forever), +// while EOF and plain decode errors keep the loop alive. +func TestReadLoopTerminalReadErrorStopsPump(t *testing.T) { + t.Run("non-EOF transport error is terminal", func(t *testing.T) { + b := &Broker{codec: newTerminalErrorCodec()} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + done := make(chan error, 1) + go func() { done <- b.readLoop(ctx) }() + + select { + case err := <-done: + if !errors.Is(err, errTerminalRead) { + t.Fatalf("readLoop err = %v, want errTerminalRead", err) + } + case <-time.After(5 * time.Second): + t.Fatal("readLoop did not stop on transport error") + } + }) + + t.Run("EOF is clean exit", func(t *testing.T) { + client, server := net.Pipe() + defer client.Close() + defer server.Close() + b := &Broker{codec: NewCodec(client)} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + done := make(chan error, 1) + go func() { done <- b.readLoop(ctx) }() + server.Close() // producer sees EOF + + select { + case err := <-done: + if err != nil { + t.Fatalf("readLoop err on EOF = %v, want nil", err) + } + case <-time.After(5 * time.Second): + t.Fatal("readLoop did not stop on EOF") + } + }) + + t.Run("recoverable decode error keeps the loop alive", func(t *testing.T) { + client, server := net.Pipe() + defer client.Close() + defer server.Close() + b := &Broker{codec: NewCodec(client)} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + done := make(chan error, 1) + go func() { done <- b.readLoop(ctx) }() + if _, err := io.WriteString(server, "not-json\n"); err != nil { + t.Fatal(err) + } + // Keep the pipe open: a closed pipe is a terminal read error, while + // the bad frame must only surface as a recoverable DecodeError. + time.Sleep(100 * time.Millisecond) + select { + case err := <-done: + t.Fatalf("readLoop exited early on decode error: %v", err) + default: + } + cancel() + }) +} diff --git a/services/ollama-proxy/body_limit_test.go b/services/ollama-proxy/body_limit_test.go new file mode 100644 index 00000000..0d90bcb1 --- /dev/null +++ b/services/ollama-proxy/body_limit_test.go @@ -0,0 +1,66 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestBufferBodyAndModelLimit covers the body-cap contract: a body over +// maxInferenceBodyBytes is reported too-large with the body dropped (never +// buffered into memory), and a body within the limit is returned with its +// parsed model field. +func TestBufferBodyAndModelLimit(t *testing.T) { + t.Run("body over the limit is too large", func(t *testing.T) { + big := bytes.Repeat([]byte("a"), maxInferenceBodyBytes+1) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", bytes.NewReader(big)) + body, model, tooLarge := bufferBodyAndModel(req) + if !tooLarge { + t.Fatal("bufferBodyAndModel tooLarge = false, want true past the cap") + } + if body != nil { + t.Error("body should be dropped (nil) when too large, not buffered") + } + if model != "" { + t.Errorf("model = %q, want empty when too large", model) + } + }) + + t.Run("body at the limit parses", func(t *testing.T) { + atLimit := bytes.Repeat([]byte("a"), maxInferenceBodyBytes) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", bytes.NewReader(atLimit)) + _, _, tooLarge := bufferBodyAndModel(req) + if tooLarge { + t.Fatal("bufferBodyAndModel tooLarge = true at exactly the cap, want false") + } + }) + + t.Run("model parsed from a small body", func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama3"}`)) + body, model, tooLarge := bufferBodyAndModel(req) + if tooLarge || model != "llama3" || string(body) != `{"model":"llama3"}` { + t.Fatalf("= (%q, %q, %v), want body kept, model llama3, not too large", body, model, tooLarge) + } + }) +} + +// TestHandleHTTPRejectsOversizedBody drives the 413 path end to end: a request +// body past maxInferenceBodyBytes is refused before any candidate resolution +// or upstream forward, with StatusRequestEntityTooLarge. +func TestHandleHTTPRejectsOversizedBody(t *testing.T) { + p := testProxy(NewDiscovery(), 1235) + big := bytes.Repeat([]byte("a"), maxInferenceBodyBytes+1) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", bytes.NewReader(big)) + req.RemoteAddr = "127.0.0.1:40000" + rec := httptest.NewRecorder() + + p.handleHTTP(rec, req) + if rec.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("oversized body status = %d, want %d", rec.Code, http.StatusRequestEntityTooLarge) + } +} diff --git a/services/ollama-proxy/ingress_test.go b/services/ollama-proxy/ingress_test.go index d6494932..e30b51d2 100644 --- a/services/ollama-proxy/ingress_test.go +++ b/services/ollama-proxy/ingress_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "testing" ) @@ -140,3 +141,42 @@ func TestLocalReverseProxyUsesSharedPlainTransport(t *testing.T) { t.Fatal("ingress reverse proxy did not use the shared plain Transport") } } + +// TestHandlePlainGatesLoopbackCrossOrigin: the loopback gate does not exclude +// browsers (they connect from loopback), so a simple cross-origin POST from an +// origin the allowlist does not name must be refused with the proxy's own 403, +// and an allowlisted origin must pass through to the router. +func TestHandlePlainGatesLoopbackCrossOrigin(t *testing.T) { + t.Run("unlisted origin is refused with 403 origin-not-allowed", func(t *testing.T) { + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "") + p := testProxy(NewDiscovery(), 1235) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil) + req.RemoteAddr = "127.0.0.1:40000" + req.Header.Set("Origin", "https://evil.example") + rec := httptest.NewRecorder() + + p.handlePlain(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("cross-origin loopback status = %d, want %d", rec.Code, http.StatusForbidden) + } + if !strings.Contains(rec.Body.String(), "origin-not-allowed") { + t.Errorf("body = %q, want the origin-not-allowed code", rec.Body.String()) + } + }) + t.Run("allowlisted origin passes the gate", func(t *testing.T) { + t.Setenv("NVPAIR_PROXY_ALLOWED_ORIGINS", "https://ui.example") + p := testProxy(NewDiscovery(), 1235) + req := httptest.NewRequest(http.MethodGet, "/v1/models", nil) + req.RemoteAddr = "127.0.0.1:40000" + req.Header.Set("Origin", "https://ui.example") + // The engine-manager identity marker answers 409 only after the origin + // gate, so a 409 proves the allowlisted caller reached the router. + req.Header.Set(engineIdentityProbeHeader, "1") + rec := httptest.NewRecorder() + + p.handlePlain(rec, req) + if rec.Code != http.StatusConflict { + t.Fatalf("allowlisted-origin status = %d, want %d (gate pass-through)", rec.Code, http.StatusConflict) + } + }) +} diff --git a/services/versions.json b/services/versions.json index 69ea4925..9d9574a0 100644 --- a/services/versions.json +++ b/services/versions.json @@ -11,7 +11,7 @@ "nvpair-workload-manager": "0.13.4", "nvpair-errors": "0.7.5", "nvpair-node-settings": "1.0.5", - "nvpair-ui-broker": "0.40.3", + "nvpair-ui-broker": "0.40.4", "nvpair-engine-manager": "0.17.6", "nvpair-cluster-manager": "1.1.6", "nvpair-job-scheduler": "0.4.2", From 2b0b3752acedb6164d6bea4e0dee7b4d30495343 Mon Sep 17 00:00:00 2001 From: woodsonl <65194841+woodsonl@users.noreply.github.com> Date: Fri, 4 Sep 2026 21:07:17 +0000 Subject: [PATCH 5/5] docs: align service READMEs with the behavior changes in this PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- services/nvpair-cluster-manager/README.md | 14 ++++++++++++++ services/nvpair-engine-manager/README.md | 20 +++++++++++++------- services/ollama-proxy/README.md | 6 ++++-- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/services/nvpair-cluster-manager/README.md b/services/nvpair-cluster-manager/README.md index 80d2e004..4ef5cbb5 100644 --- a/services/nvpair-cluster-manager/README.md +++ b/services/nvpair-cluster-manager/README.md @@ -382,6 +382,20 @@ active man-in-the-middle** on the LAN, and must be replaced by a high-entropy pairing code before cluster trust is relied on in production. The repository [`SECURITY.md`](../../SECURITY.md) records the same boundary. +Two hardening measures bound what a captured transcript or a learned invite +ID is worth: + +- **PIN stretching.** The PIN key is derived with PBKDF2-HMAC-SHA256 (50k + iterations, per-invite salt inside the EAP-MAC-covered ServerInfo), so an + eavesdropped NoobId cannot be offline-checked against the 10^6 PIN + keyspace — guessing must happen online, where it is rate-limited. +- **Authenticated terminal signals.** Cancel, decline, fail, and expire + require an HMAC tag computed over the invite ID and phase with the + session's ephemeral Key Exchange secret; a peer who only learned the + invite ID cannot kill a pairing. Completion attempts are capped (5 per + invite, every attempt counted including empty/incorrect-PIN bodies, with + invite teardown on exhaustion) so the online-guessing path stays online. + Trust is also **transitive** now (see fan-out above): a compromised member can endorse certs the whole cluster will pin, widening the blast radius of one bad node. This deliberate trade-off is why pairing should occur only on diff --git a/services/nvpair-engine-manager/README.md b/services/nvpair-engine-manager/README.md index 8c3dfa3c..66092e34 100644 --- a/services/nvpair-engine-manager/README.md +++ b/services/nvpair-engine-manager/README.md @@ -102,24 +102,30 @@ persisted) — NVPAIR can't relocate a process it didn't start; see Adoption bel ## Lifecycle ``` -NotInstalled --engine:install--> (HTTPS download + verify-if-pinned + user-mode run) --> Stopped +NotInstalled --engine:install--> (HTTPS download + sha256 verify, fail closed + user-mode run) --> Stopped Stopped --engine:start----> (adopt if already serving the port, else spawn) --> Running --health--> Running -Running --engine:stop-----> (stop signal, wait for exit; no timeout) --> Stopped +Running --engine:stop-----> (stop signal, wait up to the manifest grace, then SIGKILL/pgid) --> Stopped ``` Detect uses the manifest's `detect` paths. Install is one-shot and user-mode — an HTTPS download, verified against the manifest's `sha256` -when one is pinned (an unpinned fetch runs with a loud warning). Start +pin. A manifest without a pin fails the install closed (any executed +artifact must be pinned in the reviewed manifest); the escape hatch is +`NVPAIR_ALLOW_UNPINNED_DOWNLOADS=1`, which runs the unpinned fetch with a +loud warning instead. The bundled manifests all carry live-verified +digests. Start waits for the readiness probe, then runs a periodic health probe; an unexpected exit is reported. The bundled Ollama manifest allows up to ten minutes for startup because GPU discovery can exceed the previous 30-second allowance on supported Windows systems. The deadline remains finite: if Ollama never serves its readiness endpoint, engine-manager stops the owned process and reports the failed start. Stop sends one stop signal and waits for the engine -to exit, with no timeout: SIGTERM to the process group on Unix (graceful, no -SIGKILL escalation), and `taskkill /T /F` on Windows — where the windowless -engines we spawn can't receive a graceful (non-`/F`) close, so a forced -terminate is the only signal that actually stops them. +to exit up to the manifest's stop grace (default 5s, `grace_s` from the +manifest's stop spec): SIGTERM to the process group on Unix, `taskkill /T /F` +on Windows — where the windowless engines we spawn can't receive a graceful +(non-`/F`) close, so a forced terminate is the only signal that actually stops +them. An engine still alive after the grace is escalated to a forced kill +(SIGKILL/pgid on Unix) so a hung engine cannot block shutdown forever. ### Adoption — start may attach to an engine it didn't launch diff --git a/services/ollama-proxy/README.md b/services/ollama-proxy/README.md index 35f959b5..48e12d3a 100644 --- a/services/ollama-proxy/README.md +++ b/services/ollama-proxy/README.md @@ -39,9 +39,11 @@ The proxy listens on `--port` (default 11435) and forwards incoming HTTP request **Persisted port.** A port chosen at runtime via the `set-port` request (see below) is saved as `proxy-port.json` in the per-user data dir (`%LocalAppData%\Nvidia Corporation\Personal AI Router` on Windows, `~/.config/Nvidia Corporation/Personal AI Router` on Linux) and **restored on startup**, taking precedence over `--port`/the default — so the proxy comes back up where it was last put. `--ignore-persisted-port` deliberately bypasses that restoration for a broker-coordinated start. Delete the file (or `set-port` back to the default) to revert. -**Browser clients (CORS).** The proxy is usable from a web front end. When an engine is available, the proxy forwards an `OPTIONS` preflight so an engine-declared exact origin and credentials policy reaches the browser unchanged. If no engine is available, the engine returns no CORS policy, or a non-loopback caller must be refused before routing, the proxy answers with its own permissive `204` fallback. It also labels every response it generates — including rejections such as the `502` when no node is available and the `403` refusing a non-loopback plaintext caller — with `Access-Control-Allow-Origin: *`. That matters as much as the success path: a response without those headers reaches the browser as a generic "CORS error" with the real status and reason stripped out, so the caller cannot tell what went wrong. A locally answered preflight grants no access, since the request that follows still faces the same gate. The policy is shared with [`lmstudio-proxy`](../lmstudio-proxy/README.md) through `nvpair-shared/cors`, so both proxies answer identically. +**Browser clients (CORS).** The policy is deny-by-default. A browser page on any origin can reach the proxy's loopback listener, so with an allow-everything policy any website could drive the local engine and read its responses — using the models as a free oracle. Browser callers are therefore admitted only from exact origins the operator lists in `NVPAIR_PROXY_ALLOWED_ORIGINS` (comma-separated, scheme+host[:port], compared exactly); with no entry configured, every browser origin is refused. Non-browser callers (the Electron main process, CLI tools, health probes) send no `Origin` header and are unaffected. A cross-origin POST whose origin is not allowlisted receives `403` `origin-not-allowed` before it can drive an engine — a simple cross-origin POST needs no preflight, so preflight handling alone cannot provide this gate. -A response *forwarded from an engine* is different: if the engine set its own `Access-Control-Allow-Origin` (Ollama with `OLLAMA_ORIGINS`), that header is passed through untouched rather than replaced with the wildcard, so a deliberately narrow engine policy is never silently widened and a credentialed response is not broken. An engine that sends no CORS header has expressed no policy to keep, so the proxy supplies its own — and drops any `Access-Control-Allow-Credentials` that arrived without an origin, because a browser rejects that header alongside a wildcard origin and would discard the response the fallback exists to make readable. +The proxy answers an allowlisted origin's `OPTIONS` preflight locally with `204` and a static method/header grant (`GET, POST, PUT, DELETE, OPTIONS` / `Content-Type, Authorization`); arbitrary request headers are deliberately not echoed, so a preflight naming a header outside the static grant simply fails. Responses the proxy authors itself — including rejections such as the `502` when no node is available and the `403` refusing a non-loopback plaintext caller — carry CORS headers so an allowlisted browser can read the status and reason; without them every failure surfaces as a generic "CORS error". The policy is shared with [`lmstudio-proxy`](../lmstudio-proxy/README.md) through `nvpair-shared/cors`, so both proxies answer identically. + +A response *forwarded from an engine* is different: if the engine set its own `Access-Control-Allow-Origin` (Ollama with `OLLAMA_ORIGINS`), that header is passed through untouched rather than replaced with the proxy's grant, so a deliberately narrow engine policy is never silently widened and a credentialed response is not broken. On a preflight, an engine that declares an `Access-Control-Allow-Origin` keeps its exact origin and `Access-Control-Allow-Credentials` unchanged; an engine that declares no CORS policy gets the proxy's deny-by-default fallback instead. One limit is outside the proxy's control: current Chromium-based browsers gate a request from a public origin to a local or loopback address behind the user's [Local Network Access](https://chromestatus.com/feature/5152728072060928) permission, which replaced the old server-side opt-in header. No header the proxy sends can grant that. A hosted page needs the permission plus a `fetch(url, { targetAddressSpace: 'loopback' })` annotation; a page served from the local machine is unaffected.