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-engine-manager/bind_validation_test.go b/services/nvpair-engine-manager/bind_validation_test.go new file mode 100644 index 000000000..71fc5e205 --- /dev/null +++ b/services/nvpair-engine-manager/bind_validation_test.go @@ -0,0 +1,197 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "io" + "strings" + "testing" + + enginesettings "nvpair-shared/enginesettings" +) + +// 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. +// 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{} + 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 + // 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 + 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: 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+`"}`, 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"}`, 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) + } + + // 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+`"}`, 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) + } +} + +// 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{} + 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) + } +} diff --git a/services/nvpair-engine-manager/manager.go b/services/nvpair-engine-manager/manager.go index e12f62dc6..c985676ac 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" ) @@ -282,6 +283,27 @@ 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 { + 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) { @@ -297,9 +319,22 @@ 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 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 + } } start := func() error { return m.exec.StartWith(ctx, p.Engine, startOpts{Port: p.Port, Bind: p.Bind}) diff --git a/services/nvpair-node-settings/README.md b/services/nvpair-node-settings/README.md index 0c733de02..ba7147569 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` @@ -155,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` 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 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) }