From 8544031dbd67097a63cbc454c224d8eba7fdce91 Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Wed, 30 Sep 2026 16:43:37 -0400 Subject: [PATCH 1/3] Rebase proxy request hardening onto unified nvpair-proxy Ports the request-hardening changes onto the unified proxy process (services/nvpair-proxy), one facade per engine: - 32 MiB cap on proxied request bodies (413 before routing) - Fail-closed peer transport when no certificate pin exists - Loopback-only local backend is now enforced at store time by setLocalBackend, which the unification made strictly stronger than the original read-time gate No versions.json change: release automation owns version bumps. Signed-off-by: Mallikh Kalkere --- README.md | 3 + docs/proxy-request-hardening.mdx | 137 +++++++++++++++++ services/nvpair-proxy/proxy.go | 71 +++++++-- services/nvpair-proxy/proxy_hardening_test.go | 143 ++++++++++++++++++ 4 files changed, 342 insertions(+), 12 deletions(-) create mode 100644 docs/proxy-request-hardening.mdx create mode 100644 services/nvpair-proxy/proxy_hardening_test.go diff --git a/README.md b/README.md index 757c3f390..23369ff7e 100644 --- a/README.md +++ b/README.md @@ -242,6 +242,9 @@ 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. +10. **[Proxy request hardening](docs/proxy-request-hardening.mdx)** — request + body limits, the loopback-only local engine invariant, and fail-closed + peer TLS in `nvpair-proxy`. Component references, for when you already know what you are looking for: diff --git a/docs/proxy-request-hardening.mdx b/docs/proxy-request-hardening.mdx new file mode 100644 index 000000000..e7779994c --- /dev/null +++ b/docs/proxy-request-hardening.mdx @@ -0,0 +1,137 @@ +{/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 +*/} + +# Proxy request hardening + +The unified `services/nvpair-proxy` hosts one facade per engine behind the +client-facing `ollama-proxy:` / `lmstudio-proxy:` namespaces. Every facade +shares the same request path: a loopback HTTP ingress plus a cluster mTLS +ingress on one port, ordered failover across eligible node candidates, and +model eligibility checks before routing. This document covers three hardening +fixes to that path. Nothing here changes the JSON-RPC surface or the proxy +endpoints clients use. + +## 1. Request bodies are capped at 32 MiB + +### The problem + +`handleHTTP` buffers the entire request body (`io.ReadAll(r.Body)`) so each +failover attempt can replay it. The read was uncapped: any loopback client — +and, given the proxies' permissive loopback CORS posture, any web page the +user visits — could grow the proxy process until it OOMed. No authentication +stands in front of the loopback ingress, so this was remotely triggerable by +anything able to reach the loopback port. + +### The fix + +`maxInferenceBodyBytes = 32 << 20` caps a single proxied request body. +`bufferBodyAndModel` now reads through `io.LimitReader(r.Body, maxInferenceBodyBytes+1)`: + +- Bodies over the cap return `errBodyTooLarge`. `handleHTTP` answers `413` + with a JSON error **before** candidate selection or any engine work, and + emits a `proxy/request` notification with the target marked `rejected`. +- Bodies at or under the cap behave exactly as before, including model-field + extraction for workload tracking. +- Any other read error leaves the partially read bytes in place, as before. + +```mermaid +sequenceDiagram + participant Client + participant handleHTTP + participant bufferBodyAndModel + participant Scheduler + + Client->>handleHTTP: POST /api/chat (body) + handleHTTP->>bufferBodyAndModel: read via LimitReader(cap+1) + alt body > 32 MiB + bufferBodyAndModel-->>handleHTTP: errBodyTooLarge + handleHTTP->>Client: 413 {"error":"request body exceeds 32 MiB limit"} + else body within cap + bufferBodyAndModel-->>handleHTTP: body, model + handleHTTP->>Scheduler: candidate selection, failover + end +``` + +The proxy never holds more than cap+1 bytes per in-flight request body, so a +malicious or buggy client can no longer exhaust proxy memory. + +## 2. The local backend must be loopback + +### The problem + +The proxy documents a "the host is always loopback" invariant for the local +engine, but the old per-engine `localBackendTarget()` never enforced it: it +built a dial URL from whatever host the broker supplied via +`set-local-backend`. A compromised or buggy broker could have pointed the +cluster mTLS ingress at an arbitrary LAN address, turning the proxy into a +forwarder for plaintext inference traffic to hosts the operator never +approved. + +### The fix + +The unified ingress enforces the invariant where the backend is stored: +`setLocalBackend()` in `ingress.go` refuses a non-loopback host outright and +returns an error, so a bad value never reaches the dial path at all. + +- A non-loopback host (or an unparseable one — hostnames included) is + rejected at store time. +- `127.0.0.1`, `::1`, other `127.x` addresses, and the empty default (which + resolves to `127.0.0.1`) are accepted as before. + +```mermaid +flowchart TD + A[broker: set-local-backend] --> B{host empty?} + B -->|yes| C[store, default 127.0.0.1 at dial time] + B -->|no| D{ParseIP → IsLoopback?} + D -->|yes| E[store backend] + D -->|no| F[refuse: return error
nothing stored] +``` + +The broker always advertises `127.0.0.1` (`nvpair-ui-broker/advertiser.go`), +so legitimate traffic is unaffected; the gate only bites when the supplied +host deviates from the documented invariant. + +## 3. Unpinned peers fail closed + +### The problem + +`peerHTTPTransport()` built the per-peer TLS transport from the cluster's +certificate pin store. When the pin was absent — no cluster mesh, or the pin +vanished between candidate selection and dial time — it returned an **unpinned** +transport. A request could then silently reach the peer over a connection with +no mutual-TLS authentication, exactly in the window where the operator had +reason to believe the peer was gone. + +### The fix + +The function now returns a fail-closed transport when no live pin exists: + +- The transport's `DialContext` always fails with `errPeerUnpinned`; it + carries no `TLSClientConfig` and never opens a connection. +- Dial errors flow through the existing failover path: the request tries the + next candidate, or the proxy answers `502` when none remain. + +```mermaid +flowchart TD + A[candidate selection] --> B[peerHTTPTransport] + B --> C{live pin for peer?} + C -->|yes| D[pinned mTLS transport
cached per peer] + C -->|no| E[fail-closed transport
every dial → errPeerUnpinned] + D --> F[forward to peer] + E --> G[fail over to next candidate
or 502] +``` + +Pins are still dropped from the cache when they disappear +(`dropUnpinnedPeerTransports`); the difference is that a missing pin can no +longer downgrade to an unauthenticated connection in the meantime. + +## Validation + +- `proxy_hardening_test.go` covers all three fixes: over-cap and at-cap + bodies, the 413 response shape, loopback/non-loopback backend hosts, and + the fail-closed dial error. Run with `go test -race ./...` from the + component directory. +- No JSON-RPC method or payload changed, so no contract regeneration was + needed. diff --git a/services/nvpair-proxy/proxy.go b/services/nvpair-proxy/proxy.go index 929c3d67c..cde6ae051 100644 --- a/services/nvpair-proxy/proxy.go +++ b/services/nvpair-proxy/proxy.go @@ -213,28 +213,36 @@ type workloadParams struct { WorkloadInfo Workload `json:"workloadInfo"` } +// errBodyTooLarge is returned by bufferBodyAndModel when the request body +// exceeds maxInferenceBodyBytes. +var errBodyTooLarge = stderrors.New("request body exceeds 32 MiB limit") + // 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) { +// small (prompt + model), so full buffering is cheap. Returns (nil, "", 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. +// Bodies over maxInferenceBodyBytes are rejected with errBodyTooLarge. +func bufferBodyAndModel(r *http.Request) ([]byte, string, error) { if r.Body == nil { - return nil, "" + return nil, "", nil } - 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, "", err + } + if len(body) > maxInferenceBodyBytes { + return nil, "", errBodyTooLarge } var probe struct { Model string `json:"model"` } if err := json.Unmarshal(body, &probe); err != nil { - return body, "" + return body, "", nil } - return body, probe.Model + return body, probe.Model, nil } type statusCapture struct { @@ -736,6 +744,12 @@ const ( proxyReadHeaderTimeout = 10 * time.Second proxyServerIdleTimeout = 90 * time.Second maxModelListBytes = 16 << 20 + // maxInferenceBodyBytes caps a single proxied request body. handleHTTP + // buffers the whole body to replay it across failover attempts, so an + // uncapped read lets any loopback client grow the proxy until it OOMs. + // Bodies over the cap are rejected with 413 before any routing work + // happens. + maxInferenceBodyBytes = 32 << 20 ) // idleClientWriteTimeout bounds how long a single write of streamed response @@ -860,11 +874,11 @@ func (p *Proxy) peerHTTPTransport(peerUUID string) *http.Transport { delete(p.peerTransports, peerUUID) } if p.mesh == nil { - return newProxyTransport(nil) + return unpinnedPeerTransport() } cfg, ok := p.mesh.ClientTLSConfig(peerUUID) if !ok { - return newProxyTransport(nil) + return unpinnedPeerTransport() } tr := newProxyTransport(cfg) if p.peerTransports == nil { @@ -874,6 +888,22 @@ func (p *Proxy) peerHTTPTransport(peerUUID string) *http.Transport { return tr } +// errPeerUnpinned is the dial error of a fail-closed peer transport: the +// peer's certificate pin was absent when the transport was built. +var errPeerUnpinned = stderrors.New("peer is not a pinned cluster member") + +// unpinnedPeerTransport returns a transport that fails every dial. It is the +// fail-closed answer when a peer has no live certificate pin — used instead +// of an unpinned (plaintext-auth) transport so a vanished pin can never +// silently downgrade a peer connection. +func unpinnedPeerTransport() *http.Transport { + return &http.Transport{ + DialContext: func(_ context.Context, _, _ string) (net.Conn, error) { + return nil, errPeerUnpinned + }, + } +} + // dropUnpinnedPeerTransports closes idle conns for peer Transports whose pins // are gone. Safe to call from the mesh Watch callback. func (p *Proxy) dropUnpinnedPeerTransports() { @@ -1161,7 +1191,24 @@ func (f *facade) 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, bodyErr := bufferBodyAndModel(r) + if bodyErr != nil { + // errBodyTooLarge (413) is answered here so the oversized body never + // reaches candidate selection or an engine. Any other read error + // leaves bodyBytes as whatever was read before the failure. + if stderrors.Is(bodyErr, errBodyTooLarge) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusRequestEntityTooLarge) + _, _ = w.Write([]byte(`{"error":"request body exceeds 32 MiB limit"}`)) + _ = f.notify("proxy/request", RequestEvent{ + ID: reqID, Method: r.Method, Path: r.URL.Path, Target: "rejected", + Status: http.StatusRequestEntityTooLarge, Duration: time.Since(start).Milliseconds(), + Error: bodyErr.Error(), + }) + return + } + slog.Warn("request body read error", "id", reqID, "err", bodyErr) + } isInf := isInferenceRequest(f.profile, r.Method, r.URL.Path) routingModel := "" if isInf { diff --git a/services/nvpair-proxy/proxy_hardening_test.go b/services/nvpair-proxy/proxy_hardening_test.go new file mode 100644 index 000000000..b23400134 --- /dev/null +++ b/services/nvpair-proxy/proxy_hardening_test.go @@ -0,0 +1,143 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestBufferBodyAndModelRejectsOversizedBody: the inbound body cap is the +// fix for the unauthenticated OOM — handleHTTP buffers the whole body for +// failover replay, so an uncapped read lets any loopback client grow the +// proxy until it dies. +func TestBufferBodyAndModelRejectsOversizedBody(t *testing.T) { + big := strings.NewReader(strings.Repeat("x", maxInferenceBodyBytes+1)) + req := httptest.NewRequest(http.MethodPost, "/api/chat", big) + if _, _, err := bufferBodyAndModel(req); err != errBodyTooLarge { + t.Fatalf("oversized body err = %v, want errBodyTooLarge", err) + } + + // Exactly at the cap still passes. + exact := strings.NewReader(strings.Repeat("x", maxInferenceBodyBytes)) + req = httptest.NewRequest(http.MethodPost, "/api/chat", exact) + body, _, err := bufferBodyAndModel(req) + if err != nil { + t.Fatalf("at-cap body err = %v, want nil", err) + } + if len(body) != maxInferenceBodyBytes { + t.Fatalf("at-cap body len = %d, want %d", len(body), maxInferenceBodyBytes) + } + + // Small bodies still parse the model field. + req = httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"llama3"}`)) + _, model, err := bufferBodyAndModel(req) + if err != nil || model != "llama3" { + t.Fatalf("small body model = %q, err = %v; want %q, nil", model, err, "llama3") + } +} + +// TestHandleHTTPRejectsOversizedBody: an over-cap POST is answered 413 with +// a JSON error before any routing or engine work, and the over-cap read +// consumes at most cap+1 bytes from the client. +func TestHandleHTTPRejectsOversizedBody(t *testing.T) { + profile, ok := profileFor("ollama") + if !ok { + t.Fatal("no ollama engine profile") + } + f := testProxy(profile, NewDiscovery(), profile.StandalonePort).soleFacade() + + pr, pw := io.Pipe() + go func() { + _, _ = pw.Write([]byte(strings.Repeat("x", maxInferenceBodyBytes+1024))) + _ = pw.Close() + }() + req := httptest.NewRequest(http.MethodPost, "/api/chat", pr) + rec := httptest.NewRecorder() + + f.handleHTTP(rec, req) + + if rec.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("oversized POST status = %d, want %d", rec.Code, http.StatusRequestEntityTooLarge) + } + if ct := rec.Header().Get("Content-Type"); ct != "application/json" { + t.Errorf("Content-Type = %q, want application/json", ct) + } + if !strings.Contains(rec.Body.String(), "request body exceeds") { + t.Errorf("413 body = %q, want it to name the limit", rec.Body.String()) + } +} + +// TestHandleHTTPAcceptsNormalBody: a small body is not 413'd (it takes the +// normal no-candidate rejection path in this fixture). +func TestHandleHTTPAcceptsNormalBody(t *testing.T) { + profile, ok := profileFor("ollama") + if !ok { + t.Fatal("no ollama engine profile") + } + f := testProxy(profile, NewDiscovery(), profile.StandalonePort).soleFacade() + + req := httptest.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(`{"model":"llama3"}`)) + rec := httptest.NewRecorder() + + f.handleHTTP(rec, req) + + if rec.Code == http.StatusRequestEntityTooLarge { + t.Fatalf("normal POST status = 413, want the ordinary rejection path") + } +} + +// TestSetLocalBackendRejectsNonLoopback: the loopback-only local engine +// invariant is enforced where the backend is stored — a non-loopback host is +// refused outright, so the mTLS ingress can never be turned into a forwarder +// to an arbitrary LAN address. +func TestSetLocalBackendRejectsNonLoopback(t *testing.T) { + profile, ok := profileFor("ollama") + if !ok { + t.Fatal("no ollama engine profile") + } + f := testProxy(profile, NewDiscovery(), profile.StandalonePort).soleFacade() + + for _, host := range []string{"192.168.1.5", "10.0.0.2", "example.com", "::ffff:192.168.1.5"} { + if err := f.setLocalBackend(localBackend{Engine: "ollama", Host: host, Port: 11436, Healthy: true}); err == nil { + t.Errorf("setLocalBackend accepted non-loopback host %q", host) + } + } + for _, host := range []string{"", "127.0.0.1", "127.0.0.2", "::1"} { + if err := f.setLocalBackend(localBackend{Engine: "ollama", Host: host, Port: 11436, Healthy: true}); err != nil { + t.Errorf("setLocalBackend rejected loopback host %q: %v", host, err) + } + } +} + +// TestPeerHTTPTransportFailsClosedWithoutPin: with no live certificate pin +// the transport refuses every dial instead of falling back to an unpinned +// transport — a pin that vanishes between candidate selection and dial time +// can never silently downgrade to an unauthenticated connection. +func TestPeerHTTPTransportFailsClosedWithoutPin(t *testing.T) { + profile, ok := profileFor("ollama") + if !ok { + t.Fatal("no ollama engine profile") + } + p := testProxy(profile, NewDiscovery(), profile.StandalonePort) // mesh nil => unclustered + + tr := p.peerHTTPTransport("no-such-peer") + if tr == nil { + t.Fatal("peerHTTPTransport returned nil; want a fail-closed transport") + } + if tr.TLSClientConfig != nil { + t.Error("fail-closed transport must not carry a TLS config") + } + conn, err := tr.DialContext(t.Context(), "tcp", "192.0.2.10:443") + if err != errPeerUnpinned { + t.Errorf("dial err = %v, want errPeerUnpinned", err) + } + if conn != nil { + _ = conn.Close() + t.Error("fail-closed transport dialed successfully") + } +} From 8f20d8ea41953e95a8acec54bcfb22a218d10ec4 Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Wed, 30 Sep 2026 16:43:38 -0400 Subject: [PATCH 2/3] Verify the 413 body as JSON and use engine terminology in the docs The oversized-body test now unmarshals the 413 response and asserts the exact error payload instead of substring-matching, so the response can never silently stop being valid JSON. docs/proxy-request-hardening.mdx uses engine where the prose means the engine address rather than the set-local-backend wire name, and the sequence diagram shows the 413 payload as the JSON it is. Signed-off-by: Mallikh Kalkere --- docs/proxy-request-hardening.mdx | 8 ++++---- services/nvpair-proxy/proxy_hardening_test.go | 9 +++++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/docs/proxy-request-hardening.mdx b/docs/proxy-request-hardening.mdx index e7779994c..d391f5f53 100644 --- a/docs/proxy-request-hardening.mdx +++ b/docs/proxy-request-hardening.mdx @@ -57,7 +57,7 @@ sequenceDiagram The proxy never holds more than cap+1 bytes per in-flight request body, so a malicious or buggy client can no longer exhaust proxy memory. -## 2. The local backend must be loopback +## 2. The local engine address must be loopback ### The problem @@ -71,7 +71,7 @@ approved. ### The fix -The unified ingress enforces the invariant where the backend is stored: +The unified ingress enforces the invariant where the engine address is stored: `setLocalBackend()` in `ingress.go` refuses a non-loopback host outright and returns an error, so a bad value never reaches the dial path at all. @@ -85,7 +85,7 @@ flowchart TD A[broker: set-local-backend] --> B{host empty?} B -->|yes| C[store, default 127.0.0.1 at dial time] B -->|no| D{ParseIP → IsLoopback?} - D -->|yes| E[store backend] + D -->|yes| E[store engine address] D -->|no| F[refuse: return error
nothing stored] ``` @@ -130,7 +130,7 @@ longer downgrade to an unauthenticated connection in the meantime. ## Validation - `proxy_hardening_test.go` covers all three fixes: over-cap and at-cap - bodies, the 413 response shape, loopback/non-loopback backend hosts, and + bodies, the 413 response shape, loopback/non-loopback engine hosts, and the fail-closed dial error. Run with `go test -race ./...` from the component directory. - No JSON-RPC method or payload changed, so no contract regeneration was diff --git a/services/nvpair-proxy/proxy_hardening_test.go b/services/nvpair-proxy/proxy_hardening_test.go index b23400134..e5b81583d 100644 --- a/services/nvpair-proxy/proxy_hardening_test.go +++ b/services/nvpair-proxy/proxy_hardening_test.go @@ -4,6 +4,7 @@ package main import ( + "encoding/json" "io" "net/http" "net/http/httptest" @@ -67,8 +68,12 @@ func TestHandleHTTPRejectsOversizedBody(t *testing.T) { if ct := rec.Header().Get("Content-Type"); ct != "application/json" { t.Errorf("Content-Type = %q, want application/json", ct) } - if !strings.Contains(rec.Body.String(), "request body exceeds") { - t.Errorf("413 body = %q, want it to name the limit", rec.Body.String()) + var payload map[string]string + if err := json.Unmarshal(rec.Body.Bytes(), &payload); err != nil { + t.Fatalf("413 body is not valid JSON: %v (body = %q)", err, rec.Body.String()) + } + if payload["error"] != "request body exceeds 32 MiB limit" { + t.Errorf("413 error = %q, want %q", payload["error"], "request body exceeds 32 MiB limit") } } From e10d5698e04a3b9e2f1b3393ca55f5fc55b9a2a3 Mon Sep 17 00:00:00 2001 From: Mallikh Kalkere Date: Thu, 15 Oct 2026 21:00:52 -0400 Subject: [PATCH 3/3] docs: move proxy hardening notes into the nvpair-proxy readme Per review feedback, backend details live next to the service, not in /docs. The mdx is deleted, the root link removed, and the three fixes (body cap, loopback-only local engine, fail-closed peer TLS) are now a section in services/nvpair-proxy/README.md. Signed-off-by: Mallikh Kalkere --- README.md | 3 - docs/proxy-request-hardening.mdx | 137 ------------------------------- services/nvpair-proxy/README.md | 34 ++++++++ 3 files changed, 34 insertions(+), 140 deletions(-) delete mode 100644 docs/proxy-request-hardening.mdx diff --git a/README.md b/README.md index 23369ff7e..757c3f390 100644 --- a/README.md +++ b/README.md @@ -242,9 +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. -10. **[Proxy request hardening](docs/proxy-request-hardening.mdx)** — request - body limits, the loopback-only local engine invariant, and fail-closed - peer TLS in `nvpair-proxy`. Component references, for when you already know what you are looking for: diff --git a/docs/proxy-request-hardening.mdx b/docs/proxy-request-hardening.mdx deleted file mode 100644 index d391f5f53..000000000 --- a/docs/proxy-request-hardening.mdx +++ /dev/null @@ -1,137 +0,0 @@ -{/* -SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -SPDX-License-Identifier: Apache-2.0 -*/} - -# Proxy request hardening - -The unified `services/nvpair-proxy` hosts one facade per engine behind the -client-facing `ollama-proxy:` / `lmstudio-proxy:` namespaces. Every facade -shares the same request path: a loopback HTTP ingress plus a cluster mTLS -ingress on one port, ordered failover across eligible node candidates, and -model eligibility checks before routing. This document covers three hardening -fixes to that path. Nothing here changes the JSON-RPC surface or the proxy -endpoints clients use. - -## 1. Request bodies are capped at 32 MiB - -### The problem - -`handleHTTP` buffers the entire request body (`io.ReadAll(r.Body)`) so each -failover attempt can replay it. The read was uncapped: any loopback client — -and, given the proxies' permissive loopback CORS posture, any web page the -user visits — could grow the proxy process until it OOMed. No authentication -stands in front of the loopback ingress, so this was remotely triggerable by -anything able to reach the loopback port. - -### The fix - -`maxInferenceBodyBytes = 32 << 20` caps a single proxied request body. -`bufferBodyAndModel` now reads through `io.LimitReader(r.Body, maxInferenceBodyBytes+1)`: - -- Bodies over the cap return `errBodyTooLarge`. `handleHTTP` answers `413` - with a JSON error **before** candidate selection or any engine work, and - emits a `proxy/request` notification with the target marked `rejected`. -- Bodies at or under the cap behave exactly as before, including model-field - extraction for workload tracking. -- Any other read error leaves the partially read bytes in place, as before. - -```mermaid -sequenceDiagram - participant Client - participant handleHTTP - participant bufferBodyAndModel - participant Scheduler - - Client->>handleHTTP: POST /api/chat (body) - handleHTTP->>bufferBodyAndModel: read via LimitReader(cap+1) - alt body > 32 MiB - bufferBodyAndModel-->>handleHTTP: errBodyTooLarge - handleHTTP->>Client: 413 {"error":"request body exceeds 32 MiB limit"} - else body within cap - bufferBodyAndModel-->>handleHTTP: body, model - handleHTTP->>Scheduler: candidate selection, failover - end -``` - -The proxy never holds more than cap+1 bytes per in-flight request body, so a -malicious or buggy client can no longer exhaust proxy memory. - -## 2. The local engine address must be loopback - -### The problem - -The proxy documents a "the host is always loopback" invariant for the local -engine, but the old per-engine `localBackendTarget()` never enforced it: it -built a dial URL from whatever host the broker supplied via -`set-local-backend`. A compromised or buggy broker could have pointed the -cluster mTLS ingress at an arbitrary LAN address, turning the proxy into a -forwarder for plaintext inference traffic to hosts the operator never -approved. - -### The fix - -The unified ingress enforces the invariant where the engine address is stored: -`setLocalBackend()` in `ingress.go` refuses a non-loopback host outright and -returns an error, so a bad value never reaches the dial path at all. - -- A non-loopback host (or an unparseable one — hostnames included) is - rejected at store time. -- `127.0.0.1`, `::1`, other `127.x` addresses, and the empty default (which - resolves to `127.0.0.1`) are accepted as before. - -```mermaid -flowchart TD - A[broker: set-local-backend] --> B{host empty?} - B -->|yes| C[store, default 127.0.0.1 at dial time] - B -->|no| D{ParseIP → IsLoopback?} - D -->|yes| E[store engine address] - D -->|no| F[refuse: return error
nothing stored] -``` - -The broker always advertises `127.0.0.1` (`nvpair-ui-broker/advertiser.go`), -so legitimate traffic is unaffected; the gate only bites when the supplied -host deviates from the documented invariant. - -## 3. Unpinned peers fail closed - -### The problem - -`peerHTTPTransport()` built the per-peer TLS transport from the cluster's -certificate pin store. When the pin was absent — no cluster mesh, or the pin -vanished between candidate selection and dial time — it returned an **unpinned** -transport. A request could then silently reach the peer over a connection with -no mutual-TLS authentication, exactly in the window where the operator had -reason to believe the peer was gone. - -### The fix - -The function now returns a fail-closed transport when no live pin exists: - -- The transport's `DialContext` always fails with `errPeerUnpinned`; it - carries no `TLSClientConfig` and never opens a connection. -- Dial errors flow through the existing failover path: the request tries the - next candidate, or the proxy answers `502` when none remain. - -```mermaid -flowchart TD - A[candidate selection] --> B[peerHTTPTransport] - B --> C{live pin for peer?} - C -->|yes| D[pinned mTLS transport
cached per peer] - C -->|no| E[fail-closed transport
every dial → errPeerUnpinned] - D --> F[forward to peer] - E --> G[fail over to next candidate
or 502] -``` - -Pins are still dropped from the cache when they disappear -(`dropUnpinnedPeerTransports`); the difference is that a missing pin can no -longer downgrade to an unauthenticated connection in the meantime. - -## Validation - -- `proxy_hardening_test.go` covers all three fixes: over-cap and at-cap - bodies, the 413 response shape, loopback/non-loopback engine hosts, and - the fail-closed dial error. Run with `go test -race ./...` from the - component directory. -- No JSON-RPC method or payload changed, so no contract regeneration was - needed. diff --git a/services/nvpair-proxy/README.md b/services/nvpair-proxy/README.md index c36fb0c21..f425478c2 100644 --- a/services/nvpair-proxy/README.md +++ b/services/nvpair-proxy/README.md @@ -230,3 +230,37 @@ Add one entry to `nvpair-shared/engines` for the cross-process identity, one to proxy-unification plan for the full checklist, including the one-liners that are still per-engine (a `noderec.ServiceKey`, the TUI health label, the desktop engine-type constants, and an engine-manager manifest). + +## Request hardening + +Three fixes to the request path, all in the proxy. Nothing here changes the +JSON-RPC surface or the endpoints clients use. + +**Request bodies are capped at 32 MiB.** `handleHTTP` buffers the entire +request body so each failover attempt can replay it. The read was uncapped, +so any loopback client could grow the proxy until it OOMed, and the +permissive loopback CORS posture made this reachable from any web page the +user visits. `maxInferenceBodyBytes = 32 << 20` now caps a single body. +`bufferBodyAndModel` reads through a limit reader: bodies over the cap get +a 413 with a JSON error before candidate selection or any engine work, and a +`proxy/request` notification marked rejected. Bodies at or under the cap +behave exactly as before. The proxy never holds more than cap+1 bytes per +in-flight request body. + +**The local engine address must be loopback.** The old per-engine +`localBackendTarget()` built a dial URL from whatever host the broker +supplied via `set-local-backend`, so a compromised or buggy broker could +point the cluster mTLS ingress at an arbitrary LAN address. `setLocalBackend()` +in `ingress.go` now refuses a non-loopback host outright and returns an +error, so a bad value never reaches the dial path. `127.0.0.1`, `::1`, +other `127.x` addresses, and the empty default are accepted as before. + +**Unpinned peers fail closed.** `peerHTTPTransport()` used to return an +unpinned transport when the cluster pin was absent, so a request could +silently reach a peer with no mutual-TLS authentication. It now returns a +fail-closed transport whose dial always fails with `errPeerUnpinned`. +Dial errors flow through the existing failover path: the request tries the +next candidate, or the proxy answers 502 when none remain. + +Validation: `proxy_hardening_test.go` covers all three fixes. Run with +`go test -race ./...` from the component directory.