From 564c259392d9bbe2e86481482e999733f6d9cf86 Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Wed, 30 Sep 2026 16:43:43 -0400 Subject: [PATCH 01/11] Reject non-loopback bind addresses for local engine starts engine:start accepted any valid IP as a bind override, silently exposing the unauthenticated engine API to the LAN. Non-loopback binds now fail with -32602, matching the remote-start path that hard-binds 127.0.0.1. Signed-off-by: Mallikh Kalkere --- .../bind_validation_test.go | 70 +++++++++++++++++++ services/nvpair-engine-manager/manager.go | 17 ++++- 2 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 services/nvpair-engine-manager/bind_validation_test.go diff --git a/services/nvpair-engine-manager/bind_validation_test.go b/services/nvpair-engine-manager/bind_validation_test.go new file mode 100644 index 000000000..7fa250bfb --- /dev/null +++ b/services/nvpair-engine-manager/bind_validation_test.go @@ -0,0 +1,70 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "context" + "encoding/json" + "io" + "strings" + "testing" +) + +// codecCapture is an io.ReadWriter that discards reads and accumulates writes +// so a test can inspect the JSON-RPC frames a Manager emits. +type codecCapture struct{ buf bytes.Buffer } + +func (c *codecCapture) Read(p []byte) (int, error) { return 0, io.EOF } +func (c *codecCapture) Write(p []byte) (int, error) { return c.buf.Write(p) } + +// runOpError drives Manager.runOp with the given method/params and returns the +// JSON-RPC error frame it emitted, or "" if it emitted none. reachedExec is +// true when validation passed and runOp proceeded to the (nil, in tests) +// executor — the test only cares that no -32602 bind rejection was emitted. +func runOpError(t *testing.T, method, params string) (out string, reachedExec bool) { + t.Helper() + cap := &codecCapture{} + m := NewManager(NewCodec(cap), nil, nil) + msg := &Message{Method: method, Params: json.RawMessage(params)} + id := json.RawMessage(`"test-1"`) + msg.ID = &id + defer func() { + // The executor is nil in this fixture: validation-passing calls panic + // when they reach it, which is exactly the signal we want. + if recover() != nil { + reachedExec = true + } + }() + m.runOp(context.Background(), msg) + return cap.buf.String(), false +} + +// TestRunOpRejectsNonLoopbackBind: engine:start (and install+start) used to +// accept any valid IP as a bind override, so a broker caller could put the +// unauthenticated engine API on 0.0.0.0. Remote starts hard-bind 127.0.0.1 +// (controllifecycle.go); local starts now require loopback too. +func TestRunOpRejectsNonLoopbackBind(t *testing.T) { + for _, bind := range []string{"0.0.0.0", "192.168.1.5", "::"} { + out, reachedExec := runOpError(t, "engine:start", `{"engine":"ollama","bind":"`+bind+`"}`) + if reachedExec || !strings.Contains(out, "bind must be a loopback address") { + t.Errorf("bind %q: response %q (reachedExec=%v), want loopback rejection", bind, out, reachedExec) + } + } + + // Invalid IPs are still rejected as invalid. + out, reachedExec := runOpError(t, "engine:start", `{"engine":"ollama","bind":"not-an-ip"}`) + if reachedExec || !strings.Contains(out, "bind must be a valid IP address") { + t.Errorf("invalid bind: response %q (reachedExec=%v), want invalid-address rejection", out, reachedExec) + } + + // Loopback binds pass validation and reach the executor (which is nil in + // this fixture — the panic-recovery reports reachedExec). + for _, bind := range []string{"127.0.0.1", "::1"} { + out, reachedExec := runOpError(t, "engine:start", `{"engine":"ollama","bind":"`+bind+`"}`) + if !reachedExec || strings.Contains(out, "bind must be") { + t.Errorf("loopback bind %q: response %q (reachedExec=%v), want validation to pass", bind, out, reachedExec) + } + } +} diff --git a/services/nvpair-engine-manager/manager.go b/services/nvpair-engine-manager/manager.go index e12f62dc6..d6e17b28f 100644 --- a/services/nvpair-engine-manager/manager.go +++ b/services/nvpair-engine-manager/manager.go @@ -297,9 +297,20 @@ func (m *Manager) runOp(ctx context.Context, msg *Message) { m.codec.RespondError(msg.ID, -32602, "port must be between 0 and 65535") return } - if p.Bind != "" && net.ParseIP(p.Bind) == nil { - m.codec.RespondError(msg.ID, -32602, "bind must be a valid IP address") - return + if p.Bind != "" { + ip := net.ParseIP(p.Bind) + if ip == nil { + m.codec.RespondError(msg.ID, -32602, "bind must be a valid IP address") + return + } + // Local starts never expose the engine beyond loopback: engine APIs + // are unauthenticated, and remote starts hard-bind 127.0.0.1 (see + // controllifecycle.go). A non-loopback override would silently put + // them on the LAN. + if !ip.IsLoopback() { + m.codec.RespondError(msg.ID, -32602, "bind must be a loopback address") + return + } } start := func() error { return m.exec.StartWith(ctx, p.Engine, startOpts{Port: p.Port, Bind: p.Bind}) From 84f0ac38e63a4670b59eec2d072c26af1b84850f Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Wed, 30 Sep 2026 16:43:44 -0400 Subject: [PATCH 02/11] Document engine bind validation Validation flow for the loopback bind requirement, plus a reading-order entry in the README. Signed-off-by: Mallikh Kalkere --- README.md | 2 ++ docs/engine-bind-validation.mdx | 64 +++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 docs/engine-bind-validation.mdx diff --git a/README.md b/README.md index 7ffc8a2d5..0c9063ef4 100644 --- a/README.md +++ b/README.md @@ -242,6 +242,8 @@ Each entry assumes the ones before it. 9. **[Developer guide](docs/developing.mdx)** — read this before contributing: where the code lives, how a change travels through the layers, and the conventions the project enforces. +9. **[Engine bind validation](docs/engine-bind-validation.mdx)** — why local + engine starts require a loopback bind address. Component references, for when you already know what you are looking for: diff --git a/docs/engine-bind-validation.mdx b/docs/engine-bind-validation.mdx new file mode 100644 index 000000000..1986d9af1 --- /dev/null +++ b/docs/engine-bind-validation.mdx @@ -0,0 +1,64 @@ +{/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 +*/} + +# Engine bind validation + +`services/nvpair-engine-manager` starts and installs inference engines on +behalf of the broker. Engine APIs (Ollama, LM Studio) are unauthenticated, so +the address they bind to is the entire access control. This document covers +the fix that keeps local engine starts on loopback. + +## The problem + +`engine:start` (and install-then-start) accepted a `bind` parameter that was +validated only as a well-formed IP address. Any valid IP passed — including +`0.0.0.0`: + +```json +{"method": "engine:start", "params": {"engine": "ollama", "bind": "0.0.0.0"}} +``` + +That silently exposed the engine's unauthenticated API to the whole LAN. The +remote-start path already hard-binds `127.0.0.1` +(`controllifecycle.go`), so local starts were the inconsistent, weaker case: +the same engine, the same API, bound to every interface on the machine. + +## The fix + +`runOp` now requires a loopback address for `bind`: + +- `""` (unset) keeps the previous default behavior — the engine binds as the + executor configures it. +- A non-loopback address (`0.0.0.0`, `192.168.1.5`, `::`, …) is rejected with + JSON-RPC `-32602` and the message `bind must be a loopback address`. + Nothing is started. +- An unparseable value is still rejected as before with `bind must be a valid + IP address`. +- Loopback addresses (`127.0.0.1`, `::1`) pass validation and reach the + executor unchanged. + +```mermaid +flowchart TD + A[engine:start / install+start] --> B{bind set?} + B -->|no| C[start with default binding] + B -->|yes| D{valid IP?} + D -->|no| E[-32602: bind must be
a valid IP address] + D -->|yes| F{IsLoopback?} + F -->|yes| G[start bound to loopback] + F -->|no| H[-32602: bind must be
a loopback address] +``` + +If a deployment genuinely needs an engine reachable beyond loopback, that is +a product decision for the engine's own configuration — not something a +broker caller should be able to opt into per start. + +## Validation + +- `bind_validation_test.go` drives `runOp` directly: `0.0.0.0`, + `192.168.1.5`, and `::` are rejected; `not-an-ip` is rejected as invalid; + `127.0.0.1` and `::1` pass validation. Run with + `go test -race ./...` from `services/nvpair-engine-manager`. +- Component version bumped per `services/VERSIONING.md`: + `nvpair-engine-manager` 0.17.4 → 0.17.5. From e1e82eadd74736fc387bbbe0e7478f6e18e79c83 Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Wed, 30 Sep 2026 16:43:46 -0400 Subject: [PATCH 03/11] Adapt bind validation test to NewManager's settings wiring NewManager now wires the executor into the settings relay at construction, so the test fixture builds it with a real Executor and nils it back out right after to keep the panic-on-reach signal. Signed-off-by: Mallikh Kalkere --- services/nvpair-engine-manager/bind_validation_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/services/nvpair-engine-manager/bind_validation_test.go b/services/nvpair-engine-manager/bind_validation_test.go index 7fa250bfb..a553a9e9b 100644 --- a/services/nvpair-engine-manager/bind_validation_test.go +++ b/services/nvpair-engine-manager/bind_validation_test.go @@ -26,7 +26,11 @@ func (c *codecCapture) Write(p []byte) (int, error) { return c.buf.Write(p) } func runOpError(t *testing.T, method, params string) (out string, reachedExec bool) { t.Helper() cap := &codecCapture{} - m := NewManager(NewCodec(cap), nil, nil) + m := NewManager(NewCodec(cap), &Executor{}, nil) + // NewManager wires the executor into the settings relay, so it needs a + // non-nil one at construction; nil it back out right after to keep the + // fixture's panic-on-reach signal for validation-passing calls. + m.exec = nil msg := &Message{Method: method, Params: json.RawMessage(params)} id := json.RawMessage(`"test-1"`) msg.ID = &id From 4ec3e2251bd1e865c5f6f078a189d4184ae7d866 Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Wed, 30 Sep 2026 16:43:48 -0400 Subject: [PATCH 04/11] docs: describe the version bump as release-intent declared The validation doc read as if versions.json was hand-bumped. Bumps are declared in the PR release-intent block and applied by automation (services/versions.json is automation-managed). Signed-off-by: Mallikh Kalkere --- docs/engine-bind-validation.mdx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/engine-bind-validation.mdx b/docs/engine-bind-validation.mdx index 1986d9af1..e2efa3b06 100644 --- a/docs/engine-bind-validation.mdx +++ b/docs/engine-bind-validation.mdx @@ -60,5 +60,6 @@ broker caller should be able to opt into per start. `192.168.1.5`, and `::` are rejected; `not-an-ip` is rejected as invalid; `127.0.0.1` and `::1` pass validation. Run with `go test -race ./...` from `services/nvpair-engine-manager`. -- Component version bumped per `services/VERSIONING.md`: - `nvpair-engine-manager` 0.17.4 → 0.17.5. +- Component bump declared in the PR's release-intent block + (`nvpair-engine-manager: patch`); `services/versions.json` is + automation-managed per `services/VERSIONING.md`. From 4e4e20b62068ffa8ab3e5f86b81c6935b03614ea Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Wed, 30 Sep 2026 16:43:49 -0400 Subject: [PATCH 05/11] nvpair-node-settings: add engine_allow_lan_bind setting New boolean node setting, default false, with settings/get-engine-allow-lan-bind and settings/set-engine-allow-lan-bind. Follows the existing bool setting pattern (copy-then-save-then-commit). The engine-manager reads it live to decide whether a non-loopback bind override is allowed. Signed-off-by: Mallikh Kalkere --- services/nvpair-node-settings/README.md | 4 +++ services/nvpair-node-settings/manager.go | 36 ++++++++++++++++++- services/nvpair-node-settings/manager_test.go | 21 +++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/services/nvpair-node-settings/README.md b/services/nvpair-node-settings/README.md index 0c733de02..dde20d3d3 100644 --- a/services/nvpair-node-settings/README.md +++ b/services/nvpair-node-settings/README.md @@ -145,6 +145,10 @@ as a simple ack so React-init code can rely on a single round-trip. // cluster-friendly-name (string) {"jsonrpc":"2.0","id":8,"method":"settings/get-cluster-friendly-name"} // -> {"value": ""} {"jsonrpc":"2.0","id":9,"method":"settings/set-cluster-friendly-name","params":{"value":"Lab 3 desks"}} // -> {"ok": true} + +// engine-allow-lan-bind (bool) +{"jsonrpc":"2.0","id":10,"method":"settings/get-engine-allow-lan-bind"} // -> {"value": false} +{"jsonrpc":"2.0","id":11,"method":"settings/set-engine-allow-lan-bind","params":{"value": true}} // -> {"ok": true} ``` `settings/get-cluster-id` returns the raw stored `cluster_id` diff --git a/services/nvpair-node-settings/manager.go b/services/nvpair-node-settings/manager.go index a30a2eaef..61ceb6196 100644 --- a/services/nvpair-node-settings/manager.go +++ b/services/nvpair-node-settings/manager.go @@ -64,6 +64,15 @@ type Settings struct { // label is purely UI sugar and a getter call on first paint is // enough. ClusterFriendlyName string `json:"cluster_friendly_name"` + + // EngineAllowLANBind lifts the loopback-only restriction on local + // engine starts. Default false: a non-loopback bind override on + // engine:start is rejected, because engine APIs are unauthenticated + // and a LAN bind is usually accidental. Flipping this on is the + // deliberate opt-in for operators who genuinely want an engine + // reachable beyond loopback. Read live by the engine-manager on + // every start, so no restart is needed after a change. + EngineAllowLANBind bool `json:"engine_allow_lan_bind"` } func defaultSettings() Settings { @@ -432,7 +441,6 @@ func (m *Manager) handleMessage(msg *Message) { v := m.settings.ClusterAutoSync m.mu.RUnlock() m.codec.Respond(msg.ID, map[string]bool{"value": v}) - case "settings/set-cluster-auto-sync": var p boolValueParams if err := json.Unmarshal(msg.Params, &p); err != nil || p.Value == nil { @@ -456,6 +464,32 @@ func (m *Manager) handleMessage(msg *Message) { slog.Warn("failed to emit cluster-auto-sync notification after set", "err", err) } + case "settings/get-engine-allow-lan-bind": + m.mu.RLock() + v := m.settings.EngineAllowLANBind + m.mu.RUnlock() + m.codec.Respond(msg.ID, map[string]bool{"value": v}) + + case "settings/set-engine-allow-lan-bind": + var p boolValueParams + if err := json.Unmarshal(msg.Params, &p); err != nil || p.Value == nil { + m.codec.RespondError(msg.ID, -32602, `invalid params: expected {"value": }`) + return + } + m.mu.RLock() + newSettings := m.settings + m.mu.RUnlock() + newSettings.EngineAllowLANBind = *p.Value + if err := m.saveSettings(newSettings); err != nil { + slog.Error("failed to persist engine-allow-lan-bind", "err", err) + m.codec.RespondError(msg.ID, -32603, "failed to persist setting: "+err.Error()) + return + } + m.mu.Lock() + m.settings = newSettings + m.mu.Unlock() + m.codec.Respond(msg.ID, map[string]bool{"ok": true}) + case "shutdown": if err := m.codec.Respond(msg.ID, nil); err != nil { log.Printf("failed to respond to shutdown: %v", err) diff --git a/services/nvpair-node-settings/manager_test.go b/services/nvpair-node-settings/manager_test.go index ab1ce0eba..1638e34f2 100644 --- a/services/nvpair-node-settings/manager_test.go +++ b/services/nvpair-node-settings/manager_test.go @@ -231,6 +231,27 @@ func TestClusterAutoSyncRoundTrip(t *testing.T) { } } +func TestEngineAllowLANBindRoundTrip(t *testing.T) { + m, rw, _ := newTestManager(t) + + got := callAndDecode[map[string]bool](t, m, rw, 1, "settings/get-engine-allow-lan-bind", nil) + if got["value"] != false { + t.Fatalf("default engine-allow-lan-bind = %v, want false", got["value"]) + } + + _ = callAndDecode[map[string]bool](t, m, rw, 2, "settings/set-engine-allow-lan-bind", map[string]bool{"value": true}) + got = callAndDecode[map[string]bool](t, m, rw, 3, "settings/get-engine-allow-lan-bind", nil) + if got["value"] != true { + t.Fatalf("after set, value = %v, want true", got["value"]) + } + + _ = callAndDecode[map[string]bool](t, m, rw, 4, "settings/set-engine-allow-lan-bind", map[string]bool{"value": false}) + got = callAndDecode[map[string]bool](t, m, rw, 5, "settings/get-engine-allow-lan-bind", nil) + if got["value"] != false { + t.Fatalf("after unset, value = %v, want false", got["value"]) + } +} + // TestClusterIDRoundTrip locks down the basic get/set contract for // the cluster identifier. Empty is the default; the round-trip is // byte-for-byte (no normalization, no trimming) so a future From 7daa3316d71563e8f8000eeeda63878cc581462b Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Wed, 30 Sep 2026 16:43:51 -0400 Subject: [PATCH 06/11] broker: relay settings/ reads from workers to node-settings handleSettingsRelay now forwards relay methods prefixed with settings/ verbatim to nvpair-node-settings, the same as inbound settings/ requests. Lets the engine-manager read node settings through its existing relay without a new wire protocol. Signed-off-by: Mallikh Kalkere --- services/nvpair-ui-broker/enginesettings.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/services/nvpair-ui-broker/enginesettings.go b/services/nvpair-ui-broker/enginesettings.go index 9f65e88bc..3c7a9afb9 100644 --- a/services/nvpair-ui-broker/enginesettings.go +++ b/services/nvpair-ui-broker/enginesettings.go @@ -660,6 +660,23 @@ func (b *Broker) handleSettingsRelay(raw json.RawMessage) { } else { err = b.rebindSettingsProxy(operation.Engine, operation.Port) } + } else if strings.HasPrefix(relay.Method, "settings/") { + // A worker reading a node setting (e.g. the engine-manager + // checking engine_allow_lan_bind). Forwarded verbatim to + // nvpair-node-settings, same as an inbound settings/ request. + st := b.getSettings() + if st == nil { + err = fmt.Errorf("node-settings not available") + } else { + var res json.RawMessage + var rpcErr *RPCError + res, rpcErr, err = st.Call(ctx, relay.Method, nil) + if rpcErr != nil { + err = fmt.Errorf("%s", rpcErr.Message) + } else { + result = res + } + } } else { result, err = b.dispatchEngineSettings(ctx, relay.Method, relay.Request, relay.Caller) } From 46cb5ebff9f03fe8f9af01eeee12e14fbf88781e Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Wed, 30 Sep 2026 16:43:52 -0400 Subject: [PATCH 07/11] engine-manager: gate non-loopback binds on the node setting runOp still rejects non-loopback bind overrides by default, but now consults the live engine_allow_lan_bind node setting first. Flipping the setting takes effect on the next start with no restart. A read failure denies (fail closed). Tests stub the setting both ways. Signed-off-by: Mallikh Kalkere --- .../bind_validation_test.go | 39 ++++++++++++++---- services/nvpair-engine-manager/manager.go | 41 ++++++++++++++++--- 2 files changed, 67 insertions(+), 13 deletions(-) diff --git a/services/nvpair-engine-manager/bind_validation_test.go b/services/nvpair-engine-manager/bind_validation_test.go index a553a9e9b..1bf8c6e34 100644 --- a/services/nvpair-engine-manager/bind_validation_test.go +++ b/services/nvpair-engine-manager/bind_validation_test.go @@ -23,7 +23,9 @@ func (c *codecCapture) Write(p []byte) (int, error) { return c.buf.Write(p) } // JSON-RPC error frame it emitted, or "" if it emitted none. reachedExec is // true when validation passed and runOp proceeded to the (nil, in tests) // executor — the test only cares that no -32602 bind rejection was emitted. -func runOpError(t *testing.T, method, params string) (out string, reachedExec bool) { +// lanBind sets the engine_allow_lan_bind node setting for the call; the relay +// is stubbed so no broker round-trip happens. +func runOpError(t *testing.T, method, params string, lanBind bool) (out string, reachedExec bool) { t.Helper() cap := &codecCapture{} m := NewManager(NewCodec(cap), &Executor{}, nil) @@ -31,6 +33,9 @@ func runOpError(t *testing.T, method, params string) (out string, reachedExec bo // non-nil one at construction; nil it back out right after to keep the // fixture's panic-on-reach signal for validation-passing calls. m.exec = nil + // Stub the node-setting read: the real path goes through the relay to + // the broker, which has no test double here. + m.lanBindForTest = func() bool { return lanBind } msg := &Message{Method: method, Params: json.RawMessage(params)} id := json.RawMessage(`"test-1"`) msg.ID = &id @@ -45,20 +50,21 @@ func runOpError(t *testing.T, method, params string) (out string, reachedExec bo return cap.buf.String(), false } -// TestRunOpRejectsNonLoopbackBind: engine:start (and install+start) used to -// accept any valid IP as a bind override, so a broker caller could put the -// unauthenticated engine API on 0.0.0.0. Remote starts hard-bind 127.0.0.1 -// (controllifecycle.go); local starts now require loopback too. +// TestRunOpRejectsNonLoopbackBind: with the node setting off (the default), +// engine:start (and install+start) reject any non-loopback bind override, so +// a broker caller cannot put the unauthenticated engine API on the LAN by +// accident. Remote starts hard-bind 127.0.0.1 (controllifecycle.go); local +// starts require loopback too unless the operator opts in. func TestRunOpRejectsNonLoopbackBind(t *testing.T) { for _, bind := range []string{"0.0.0.0", "192.168.1.5", "::"} { - out, reachedExec := runOpError(t, "engine:start", `{"engine":"ollama","bind":"`+bind+`"}`) + out, reachedExec := runOpError(t, "engine:start", `{"engine":"ollama","bind":"`+bind+`"}`, false) if reachedExec || !strings.Contains(out, "bind must be a loopback address") { t.Errorf("bind %q: response %q (reachedExec=%v), want loopback rejection", bind, out, reachedExec) } } // Invalid IPs are still rejected as invalid. - out, reachedExec := runOpError(t, "engine:start", `{"engine":"ollama","bind":"not-an-ip"}`) + out, reachedExec := runOpError(t, "engine:start", `{"engine":"ollama","bind":"not-an-ip"}`, false) if reachedExec || !strings.Contains(out, "bind must be a valid IP address") { t.Errorf("invalid bind: response %q (reachedExec=%v), want invalid-address rejection", out, reachedExec) } @@ -66,9 +72,26 @@ func TestRunOpRejectsNonLoopbackBind(t *testing.T) { // Loopback binds pass validation and reach the executor (which is nil in // this fixture — the panic-recovery reports reachedExec). for _, bind := range []string{"127.0.0.1", "::1"} { - out, reachedExec := runOpError(t, "engine:start", `{"engine":"ollama","bind":"`+bind+`"}`) + out, reachedExec := runOpError(t, "engine:start", `{"engine":"ollama","bind":"`+bind+`"}`, false) if !reachedExec || strings.Contains(out, "bind must be") { t.Errorf("loopback bind %q: response %q (reachedExec=%v), want validation to pass", bind, out, reachedExec) } } } + +// TestRunOpAllowsNonLoopbackBindWhenSettingOn: flipping +// engine_allow_lan_bind on is the deliberate opt-in. Non-loopback binds then +// pass validation; invalid IPs are still rejected as invalid. +func TestRunOpAllowsNonLoopbackBindWhenSettingOn(t *testing.T) { + for _, bind := range []string{"0.0.0.0", "192.168.1.5", "::", "127.0.0.1", "::1"} { + out, reachedExec := runOpError(t, "engine:start", `{"engine":"ollama","bind":"`+bind+`"}`, true) + if !reachedExec || strings.Contains(out, "bind must be") { + t.Errorf("bind %q with setting on: response %q (reachedExec=%v), want validation to pass", bind, out, reachedExec) + } + } + + out, reachedExec := runOpError(t, "engine:start", `{"engine":"ollama","bind":"not-an-ip"}`, true) + if reachedExec || !strings.Contains(out, "bind must be a valid IP address") { + t.Errorf("invalid bind with setting on: response %q (reachedExec=%v), want invalid-address rejection", out, reachedExec) + } +} diff --git a/services/nvpair-engine-manager/manager.go b/services/nvpair-engine-manager/manager.go index d6e17b28f..429cb6f08 100644 --- a/services/nvpair-engine-manager/manager.go +++ b/services/nvpair-engine-manager/manager.go @@ -15,6 +15,7 @@ import ( "nvpair-shared/applog" "nvpair-shared/clustertrust" + enginesettings "nvpair-shared/enginesettings" "nvpair-shared/noderec" "nvpair-shared/reach" ) @@ -67,6 +68,10 @@ type Manager struct { peers *peerDirectory addrs *reach.Chooser mesh *clustertrust.Mesh // cluster identity + pins for dialing peers' ec surfaces + // lanBindForTest overrides the engine_allow_lan_bind node-setting read + // in unit tests. Nil in production, where the live setting is read + // through the relay on every check. + lanBindForTest func() bool // remoteHTTP / readyHTTP are long-lived per-peer mTLS pools. A throwaway // Transport per engine:remote-* call leaked the idle socket. readyHTTP // uses the longer header budget for start/delete (see waitsForEngineReadiness). @@ -282,6 +287,30 @@ func (m *Manager) handleMessage(ctx context.Context, msg *Message) { } } +// lanBindAllowed reports the live value of the engine_allow_lan_bind node +// setting. Engine starts are not hot-path, so this reads through on every +// call: flipping the setting takes effect immediately, no restart needed. +// Any read failure denies (fail closed). The loopback-only default holds +// unless the setting is affirmatively on. +func (m *Manager) lanBindAllowed(ctx context.Context) bool { + if m.lanBindForTest != nil { + return m.lanBindForTest() + } + raw, err := m.settingsRelay.call(ctx, "settings/get-engine-allow-lan-bind", enginesettings.Request{}, "engine:start") + if err != nil { + slog.Warn("failed to read engine-allow-lan-bind setting; denying LAN bind", "err", err) + return false + } + var r struct { + Value bool `json:"value"` + } + if err := json.Unmarshal(raw, &r); err != nil { + slog.Warn("failed to decode engine-allow-lan-bind setting; denying LAN bind", "err", err) + return false + } + return r.Value +} + // runOp executes a lifecycle op asynchronously and responds with the // engine's resulting status (or an error). func (m *Manager) runOp(ctx context.Context, msg *Message) { @@ -303,11 +332,13 @@ func (m *Manager) runOp(ctx context.Context, msg *Message) { m.codec.RespondError(msg.ID, -32602, "bind must be a valid IP address") return } - // Local starts never expose the engine beyond loopback: engine APIs - // are unauthenticated, and remote starts hard-bind 127.0.0.1 (see - // controllifecycle.go). A non-loopback override would silently put - // them on the LAN. - if !ip.IsLoopback() { + // Local starts stay on loopback unless the operator has + // deliberately opted in: engine APIs are unauthenticated, and + // remote starts hard-bind 127.0.0.1 (see controllifecycle.go). + // A non-loopback override without the node setting would + // silently put them on the LAN. The setting is read live, so + // flipping it takes effect on the next start, no restart. + if !ip.IsLoopback() && !m.lanBindAllowed(ctx) { m.codec.RespondError(msg.ID, -32602, "bind must be a loopback address") return } From 818c1903f30f4c9621b1ad0c915d49046911e9c3 Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Wed, 30 Sep 2026 16:43:54 -0400 Subject: [PATCH 08/11] docs: describe the LAN bind opt-in Documents the engine_allow_lan_bind node setting, how to flip it at runtime, and why it is a node-level switch rather than a per-call flag. Mermaid flow updated with the setting branch. Signed-off-by: Mallikh Kalkere --- docs/engine-bind-validation.mdx | 45 ++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/docs/engine-bind-validation.mdx b/docs/engine-bind-validation.mdx index e2efa3b06..3dc3eaa0e 100644 --- a/docs/engine-bind-validation.mdx +++ b/docs/engine-bind-validation.mdx @@ -8,7 +8,8 @@ SPDX-License-Identifier: Apache-2.0 `services/nvpair-engine-manager` starts and installs inference engines on behalf of the broker. Engine APIs (Ollama, LM Studio) are unauthenticated, so the address they bind to is the entire access control. This document covers -the fix that keeps local engine starts on loopback. +the fix that keeps local engine starts on loopback by default, with a +deliberate opt-in for operators who want more. ## The problem @@ -27,7 +28,8 @@ the same engine, the same API, bound to every interface on the machine. ## The fix -`runOp` now requires a loopback address for `bind`: +`runOp` requires a loopback address for `bind`, unless the operator has +opted in through the `engine_allow_lan_bind` node setting: - `""` (unset) keeps the previous default behavior — the engine binds as the executor configures it. @@ -47,19 +49,38 @@ flowchart TD D -->|no| E[-32602: bind must be
a valid IP address] D -->|yes| F{IsLoopback?} F -->|yes| G[start bound to loopback] - F -->|no| H[-32602: bind must be
a loopback address] + F -->|no| H{engine_allow_lan_bind
setting on?} + H -->|yes| I[start with requested bind] + H -->|no| J[-32602: bind must be
a loopback address] ``` -If a deployment genuinely needs an engine reachable beyond loopback, that is -a product decision for the engine's own configuration — not something a -broker caller should be able to opt into per start. +## Opting in to LAN binds + +PAIR defaults to safe and stays out of the operator's way: if a deployment +genuinely needs an engine reachable beyond loopback, flip the +`engine_allow_lan_bind` node setting: + +```json +{"jsonrpc":"2.0","id":1,"method":"settings/set-engine-allow-lan-bind","params":{"value": true}} +``` + +The setting is read live on every start, so no restart is needed. It is +deliberately a node-level switch rather than a per-call flag: opening an +unauthenticated API to the LAN should be a conscious, persistent decision +about the machine, not something a single broker call can slip in. Turning +it back off takes effect on the next start as well. ## Validation -- `bind_validation_test.go` drives `runOp` directly: `0.0.0.0`, - `192.168.1.5`, and `::` are rejected; `not-an-ip` is rejected as invalid; - `127.0.0.1` and `::1` pass validation. Run with - `go test -race ./...` from `services/nvpair-engine-manager`. -- Component bump declared in the PR's release-intent block - (`nvpair-engine-manager: patch`); `services/versions.json` is +- `bind_validation_test.go` drives `runOp` directly with the setting stubbed + both ways: with it off, `0.0.0.0`, `192.168.1.5`, and `::` are rejected; + `not-an-ip` is rejected as invalid; `127.0.0.1` and `::1` pass. With it + on, every well-formed bind passes and only unparseable values are + rejected. Run with `go test -race ./...` from + `services/nvpair-engine-manager`. +- `nvpair-node-settings` covers the new get/set methods with a round-trip + test: default false, set true reads back true, set false reads back false. +- Component bumps declared in the PR's release-intent block + (`nvpair-engine-manager: patch`, `nvpair-node-settings: patch`, + `nvpair-ui-broker: patch`); `services/versions.json` is automation-managed per `services/VERSIONING.md`. From 62d65607d7908bfbb41b719831235cf856b15dc5 Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Wed, 30 Sep 2026 16:43:55 -0400 Subject: [PATCH 09/11] engine-manager: test the fail-closed setting read Covers the two paths the earlier tests stubbed around: a relay error (broker unreachable) and an undecodable broker reply both deny the non-loopback bind. Also exercises the live relay path end to end with a true setting value. Signed-off-by: Mallikh Kalkere --- .../bind_validation_test.go | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/services/nvpair-engine-manager/bind_validation_test.go b/services/nvpair-engine-manager/bind_validation_test.go index 1bf8c6e34..34a03ba3f 100644 --- a/services/nvpair-engine-manager/bind_validation_test.go +++ b/services/nvpair-engine-manager/bind_validation_test.go @@ -7,9 +7,12 @@ import ( "bytes" "context" "encoding/json" + "errors" "io" "strings" "testing" + + enginesettings "nvpair-shared/enginesettings" ) // codecCapture is an io.ReadWriter that discards reads and accumulates writes @@ -95,3 +98,96 @@ func TestRunOpAllowsNonLoopbackBindWhenSettingOn(t *testing.T) { t.Errorf("invalid bind with setting on: response %q (reachedExec=%v), want invalid-address rejection", out, reachedExec) } } + +// runOpWithRelaySend drives Manager.runOp like runOpError, but leaves the +// lanBindForTest stub unset so lanBindAllowed takes the real relay path, with +// send standing in for the broker round-trip. +func runOpWithRelaySend(t *testing.T, params string, send func(string, any) error) (out string, reachedExec bool) { + t.Helper() + cap := &codecCapture{} + m := NewManager(NewCodec(cap), &Executor{}, nil) + // Same fixture deal as runOpError: nil the executor out so a + // validation-passing call panics, which the recovery below reports. + m.exec = nil + m.settingsRelay.send = send + msg := &Message{Method: "engine:start", Params: json.RawMessage(params)} + id := json.RawMessage(`"test-1"`) + msg.ID = &id + defer func() { + if recover() != nil { + reachedExec = true + } + }() + m.runOp(context.Background(), msg) + return cap.buf.String(), false +} + +// replyWith returns a relay send func that answers the settings read with the +// given raw result, as the broker would. +func replyWith(m *Manager, result json.RawMessage) func(string, any) error { + return func(_ string, value any) error { + rel, ok := value.(enginesettings.Relay) + if !ok { + return errors.New("unexpected relay payload") + } + m.settingsRelay.mu.Lock() + ch := m.settingsRelay.pending[rel.ID] + m.settingsRelay.mu.Unlock() + ch <- settingsReply{Result: result} + return nil + } +} + +// TestRunOpFailsClosedWhenSettingUnreadable: the node-setting read must fail +// closed. If the relay to the broker errors, or the broker answers something +// that is not a setting value, a non-loopback bind is rejected instead of +// being let through. +func TestRunOpFailsClosedWhenSettingUnreadable(t *testing.T) { + // Broker unreachable: the relay send fails. + out, reachedExec := runOpWithRelaySend(t, `{"engine":"ollama","bind":"192.168.1.5"}`, + func(string, any) error { return errors.New("broker down") }) + if reachedExec || !strings.Contains(out, "bind must be a loopback address") { + t.Errorf("relay error: response %q (reachedExec=%v), want loopback rejection", out, reachedExec) + } + + // Broker answers garbage: the setting value cannot be decoded. + cap := &codecCapture{} + m := NewManager(NewCodec(cap), &Executor{}, nil) + m.exec = nil + m.settingsRelay.send = replyWith(m, json.RawMessage(`not-json`)) + msg := &Message{Method: "engine:start", Params: json.RawMessage(`{"engine":"ollama","bind":"192.168.1.5"}`)} + id := json.RawMessage(`"test-1"`) + msg.ID = &id + func() { + defer func() { _ = recover() }() + m.runOp(context.Background(), msg) + }() + if out := cap.buf.String(); !strings.Contains(out, "bind must be a loopback address") { + t.Errorf("garbage reply: response %q, want loopback rejection", out) + } +} + +// TestRunOpReadsLiveSettingThroughRelay: the real relay path (no test stub) +// honors the broker's answer. A true value lets the bind through; this is the +// path production takes on every start. +func TestRunOpReadsLiveSettingThroughRelay(t *testing.T) { + cap := &codecCapture{} + m := NewManager(NewCodec(cap), &Executor{}, nil) + m.exec = nil + m.settingsRelay.send = replyWith(m, json.RawMessage(`{"value": true}`)) + msg := &Message{Method: "engine:start", Params: json.RawMessage(`{"engine":"ollama","bind":"192.168.1.5"}`)} + id := json.RawMessage(`"test-1"`) + msg.ID = &id + reachedExec := false + func() { + defer func() { + if recover() != nil { + reachedExec = true + } + }() + m.runOp(context.Background(), msg) + }() + if out := cap.buf.String(); !reachedExec || strings.Contains(out, "bind must be") { + t.Errorf("live setting true: response %q (reachedExec=%v), want validation to pass", out, reachedExec) + } +} From 13a311abf6bf401aa0d6dc8b3f285e26958c094e Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Thu, 15 Oct 2026 17:13:27 -0400 Subject: [PATCH 10/11] docs: move engine-bind-validation content into service readmes Per review feedback, backend details don't belong in /docs. The LAN-bind opt-in note moves to the node-settings readme (after the setting's RPC surface), and the bind validation rules move to the engine-manager readme under Security posture. docs/engine-bind-validation.mdx is removed, along with its link from the root README. Signed-off-by: Mallikh Kalkere --- README.md | 2 - docs/engine-bind-validation.mdx | 86 ------------------------ services/nvpair-engine-manager/README.md | 4 ++ services/nvpair-node-settings/README.md | 8 +++ 4 files changed, 12 insertions(+), 88 deletions(-) delete mode 100644 docs/engine-bind-validation.mdx diff --git a/README.md b/README.md index 0c9063ef4..7ffc8a2d5 100644 --- a/README.md +++ b/README.md @@ -242,8 +242,6 @@ Each entry assumes the ones before it. 9. **[Developer guide](docs/developing.mdx)** — read this before contributing: where the code lives, how a change travels through the layers, and the conventions the project enforces. -9. **[Engine bind validation](docs/engine-bind-validation.mdx)** — why local - engine starts require a loopback bind address. Component references, for when you already know what you are looking for: diff --git a/docs/engine-bind-validation.mdx b/docs/engine-bind-validation.mdx deleted file mode 100644 index 3dc3eaa0e..000000000 --- a/docs/engine-bind-validation.mdx +++ /dev/null @@ -1,86 +0,0 @@ -{/* -SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -SPDX-License-Identifier: Apache-2.0 -*/} - -# Engine bind validation - -`services/nvpair-engine-manager` starts and installs inference engines on -behalf of the broker. Engine APIs (Ollama, LM Studio) are unauthenticated, so -the address they bind to is the entire access control. This document covers -the fix that keeps local engine starts on loopback by default, with a -deliberate opt-in for operators who want more. - -## The problem - -`engine:start` (and install-then-start) accepted a `bind` parameter that was -validated only as a well-formed IP address. Any valid IP passed — including -`0.0.0.0`: - -```json -{"method": "engine:start", "params": {"engine": "ollama", "bind": "0.0.0.0"}} -``` - -That silently exposed the engine's unauthenticated API to the whole LAN. The -remote-start path already hard-binds `127.0.0.1` -(`controllifecycle.go`), so local starts were the inconsistent, weaker case: -the same engine, the same API, bound to every interface on the machine. - -## The fix - -`runOp` requires a loopback address for `bind`, unless the operator has -opted in through the `engine_allow_lan_bind` node setting: - -- `""` (unset) keeps the previous default behavior — the engine binds as the - executor configures it. -- A non-loopback address (`0.0.0.0`, `192.168.1.5`, `::`, …) is rejected with - JSON-RPC `-32602` and the message `bind must be a loopback address`. - Nothing is started. -- An unparseable value is still rejected as before with `bind must be a valid - IP address`. -- Loopback addresses (`127.0.0.1`, `::1`) pass validation and reach the - executor unchanged. - -```mermaid -flowchart TD - A[engine:start / install+start] --> B{bind set?} - B -->|no| C[start with default binding] - B -->|yes| D{valid IP?} - D -->|no| E[-32602: bind must be
a valid IP address] - D -->|yes| F{IsLoopback?} - F -->|yes| G[start bound to loopback] - F -->|no| H{engine_allow_lan_bind
setting on?} - H -->|yes| I[start with requested bind] - H -->|no| J[-32602: bind must be
a loopback address] -``` - -## Opting in to LAN binds - -PAIR defaults to safe and stays out of the operator's way: if a deployment -genuinely needs an engine reachable beyond loopback, flip the -`engine_allow_lan_bind` node setting: - -```json -{"jsonrpc":"2.0","id":1,"method":"settings/set-engine-allow-lan-bind","params":{"value": true}} -``` - -The setting is read live on every start, so no restart is needed. It is -deliberately a node-level switch rather than a per-call flag: opening an -unauthenticated API to the LAN should be a conscious, persistent decision -about the machine, not something a single broker call can slip in. Turning -it back off takes effect on the next start as well. - -## Validation - -- `bind_validation_test.go` drives `runOp` directly with the setting stubbed - both ways: with it off, `0.0.0.0`, `192.168.1.5`, and `::` are rejected; - `not-an-ip` is rejected as invalid; `127.0.0.1` and `::1` pass. With it - on, every well-formed bind passes and only unparseable values are - rejected. Run with `go test -race ./...` from - `services/nvpair-engine-manager`. -- `nvpair-node-settings` covers the new get/set methods with a round-trip - test: default false, set true reads back true, set false reads back false. -- Component bumps declared in the PR's release-intent block - (`nvpair-engine-manager: patch`, `nvpair-node-settings: patch`, - `nvpair-ui-broker: patch`); `services/versions.json` is - automation-managed per `services/VERSIONING.md`. diff --git a/services/nvpair-engine-manager/README.md b/services/nvpair-engine-manager/README.md index b401c8833..9d32f3a22 100644 --- a/services/nvpair-engine-manager/README.md +++ b/services/nvpair-engine-manager/README.md @@ -278,6 +278,10 @@ cluster traffic. Managed engines bind **loopback by default**, but a manifest's `runtime.bind` may open an inference engine to the LAN — Ollama ships `0.0.0.0` to serve the cluster — overridable per call via `engine:start {bind}`; readiness/health probes always target loopback. +The `bind` parameter is validated before anything launches: it must be a +well-formed IP address, and a non-loopback address is rejected with +`-32602` (`bind must be a loopback address`) unless `engine_allow_lan_bind` +is on. Loopback addresses pass through to the executor unchanged. Downloads are **HTTPS-only** (plain `http` only from loopback) and verified against the manifest's `sha256` when one is pinned; an unpinned fetch runs with a loud warning. diff --git a/services/nvpair-node-settings/README.md b/services/nvpair-node-settings/README.md index dde20d3d3..ba7147569 100644 --- a/services/nvpair-node-settings/README.md +++ b/services/nvpair-node-settings/README.md @@ -159,6 +159,14 @@ non-empty is the verbatim id. Live cluster-membership changes come from `connection/cluster-identity` push notifications, not from polling this getter. +`settings/get-engine-allow-lan-bind` returns whether local engine starts +may bind to non-loopback addresses (`{"value": }`, default `false`). +PAIR keeps engine starts on loopback unless the operator opts in here: the +setting is read live on every start, so no restart is needed. It is +deliberately a node-level switch rather than a per-call flag — opening an +unauthenticated engine API to the LAN should be a conscious, persistent +decision about the machine, not something a single broker call can slip in. + ### Removed methods The schema update removed four method pairs. They return `-32601` From 2e0ab4f18253cec53b2aab3f1820eb05e9cef234 Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Thu, 15 Oct 2026 17:35:48 -0400 Subject: [PATCH 11/11] refactor: drop lanBindForTest hook, tests drive the real relay path Per review feedback, test-only code has no place in the production Manager struct. The fail-closed tests already exercised the real relay path (runOpWithRelaySend); runOpError now does the same, answering the node-setting read through settingsRelay.send the way the broker would. Same three cases covered, no hook. Signed-off-by: Mallikh Kalkere --- .../bind_validation_test.go | 20 +++++++++++-------- services/nvpair-engine-manager/manager.go | 7 ------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/services/nvpair-engine-manager/bind_validation_test.go b/services/nvpair-engine-manager/bind_validation_test.go index 34a03ba3f..71fc5e205 100644 --- a/services/nvpair-engine-manager/bind_validation_test.go +++ b/services/nvpair-engine-manager/bind_validation_test.go @@ -26,8 +26,8 @@ func (c *codecCapture) Write(p []byte) (int, error) { return c.buf.Write(p) } // JSON-RPC error frame it emitted, or "" if it emitted none. reachedExec is // true when validation passed and runOp proceeded to the (nil, in tests) // executor — the test only cares that no -32602 bind rejection was emitted. -// lanBind sets the engine_allow_lan_bind node setting for the call; the relay -// is stubbed so no broker round-trip happens. +// lanBind sets the engine_allow_lan_bind node setting for the call, answered +// through the real settings relay path — the same code production runs. func runOpError(t *testing.T, method, params string, lanBind bool) (out string, reachedExec bool) { t.Helper() cap := &codecCapture{} @@ -36,9 +36,13 @@ func runOpError(t *testing.T, method, params string, lanBind bool) (out string, // non-nil one at construction; nil it back out right after to keep the // fixture's panic-on-reach signal for validation-passing calls. m.exec = nil - // Stub the node-setting read: the real path goes through the relay to - // the broker, which has no test double here. - m.lanBindForTest = func() bool { return lanBind } + // Answer the node-setting read the way the broker would, through the + // relay — no test-only hook in the Manager struct. + setting := json.RawMessage(`{"value": false}`) + if lanBind { + setting = json.RawMessage(`{"value": true}`) + } + m.settingsRelay.send = replyWith(m, setting) msg := &Message{Method: method, Params: json.RawMessage(params)} id := json.RawMessage(`"test-1"`) msg.ID = &id @@ -99,9 +103,9 @@ func TestRunOpAllowsNonLoopbackBindWhenSettingOn(t *testing.T) { } } -// runOpWithRelaySend drives Manager.runOp like runOpError, but leaves the -// lanBindForTest stub unset so lanBindAllowed takes the real relay path, with -// send standing in for the broker round-trip. +// runOpWithRelaySend drives Manager.runOp with a custom relay send func +// standing in for the broker round-trip, for cases where the caller needs +// to control the relay behavior directly (errors, garbage replies). func runOpWithRelaySend(t *testing.T, params string, send func(string, any) error) (out string, reachedExec bool) { t.Helper() cap := &codecCapture{} diff --git a/services/nvpair-engine-manager/manager.go b/services/nvpair-engine-manager/manager.go index 429cb6f08..c985676ac 100644 --- a/services/nvpair-engine-manager/manager.go +++ b/services/nvpair-engine-manager/manager.go @@ -68,10 +68,6 @@ type Manager struct { peers *peerDirectory addrs *reach.Chooser mesh *clustertrust.Mesh // cluster identity + pins for dialing peers' ec surfaces - // lanBindForTest overrides the engine_allow_lan_bind node-setting read - // in unit tests. Nil in production, where the live setting is read - // through the relay on every check. - lanBindForTest func() bool // remoteHTTP / readyHTTP are long-lived per-peer mTLS pools. A throwaway // Transport per engine:remote-* call leaked the idle socket. readyHTTP // uses the longer header budget for start/delete (see waitsForEngineReadiness). @@ -293,9 +289,6 @@ func (m *Manager) handleMessage(ctx context.Context, msg *Message) { // Any read failure denies (fail closed). The loopback-only default holds // unless the setting is affirmatively on. func (m *Manager) lanBindAllowed(ctx context.Context) bool { - if m.lanBindForTest != nil { - return m.lanBindForTest() - } raw, err := m.settingsRelay.call(ctx, "settings/get-engine-allow-lan-bind", enginesettings.Request{}, "engine:start") if err != nil { slog.Warn("failed to read engine-allow-lan-bind setting; denying LAN bind", "err", err)