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. 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..e5b81583d --- /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") + } +}