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..4f674772 100644 --- a/desktop/src/electron/ipc/safe-handle.ts +++ b/desktop/src/electron/ipc/safe-handle.ts @@ -16,7 +16,9 @@ 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 === 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/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/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/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..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" ) @@ -46,10 +47,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 +67,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) } } @@ -109,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/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/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-cluster-manager/cancel.go b/services/nvpair-cluster-manager/cancel.go index 9ad63e82..20f18553 100644 --- a/services/nvpair-cluster-manager/cancel.go +++ b/services/nvpair-cluster-manager/cancel.go @@ -73,6 +73,25 @@ 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. 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) + 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 +101,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 +109,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..72235be9 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 { + // 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() + 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/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-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/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/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/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}"] }, 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..886acb3e 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 @@ -2165,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 @@ -2184,27 +2226,72 @@ 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 + } + if recoverableDecode(err) { + 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(): 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 } - 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..a7890107 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) } @@ -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/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..678331f0 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,63 @@ 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. +// 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)) + 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 +196,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..abcf3642 100644 --- a/services/nvpair-ui-broker/relay/relay_test.go +++ b/services/nvpair-ui-broker/relay/relay_test.go @@ -5,7 +5,9 @@ package relay import ( "reflect" + "sync" "testing" + "time" "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) } } @@ -176,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/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/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/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/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. 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/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..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" ) @@ -51,10 +52,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 +72,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) } } @@ -135,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/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..9d9574a0 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.4", + "nvpair-engine-manager": "0.17.6", + "nvpair-cluster-manager": "1.1.6", + "nvpair-job-scheduler": "0.4.2", + "nvpair-tui": "0.7.3" } }