diff --git a/README.md b/README.md index 757c3f39..23369ff7 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 00000000..d391f5f5 --- /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 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/proxy.go b/services/nvpair-proxy/proxy.go index 929c3d67..cde6ae05 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 00000000..e5b81583 --- /dev/null +++ b/services/nvpair-proxy/proxy_hardening_test.go @@ -0,0 +1,148 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "encoding/json" + "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) + } + 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") + } +} + +// 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") + } +}