diff --git a/README.md b/README.md index 0f0a7242..5e9608b0 100644 --- a/README.md +++ b/README.md @@ -239,6 +239,8 @@ Each entry assumes the ones before it. 8. **[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 00000000..1986d9af --- /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. 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 00000000..7fa250bf --- /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 80d572b4..7b1777cd 100644 --- a/services/nvpair-engine-manager/manager.go +++ b/services/nvpair-engine-manager/manager.go @@ -289,9 +289,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}) diff --git a/services/versions.json b/services/versions.json index 29d8c230..7afd844d 100644 --- a/services/versions.json +++ b/services/versions.json @@ -12,7 +12,7 @@ "nvpair-errors": "0.7.4", "nvpair-node-settings": "1.0.4", "nvpair-ui-broker": "0.40.2", - "nvpair-engine-manager": "0.17.4", + "nvpair-engine-manager": "0.17.5", "nvpair-cluster-manager": "1.1.4", "nvpair-job-scheduler": "0.4.1", "nvpair-tui": "0.7.2"