From 19af3d47a0ee100dd6b5915957c73d76a88134fb Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark (CryptoJones)" Date: Sat, 5 Sep 2026 19:47:41 -0500 Subject: [PATCH 1/6] Add nvpair-shared/ingressauth, an opt-in API-key gate for non-loopback plaintext callers Both inference proxies refuse plaintext requests that do not arrive from loopback. This package is the credential gate they will share so a LAN caller can be admitted only when the operator opts in by configuring API keys (NVPAIR_PROXY_API_KEYS_FILE, default /proxy-api-keys, or NVPAIR_PROXY_API_KEYS) and the caller presents one as Authorization: Bearer or X-Api-Key, optionally restricted by NVPAIR_PROXY_ALLOWED_CIDRS. Keys are held only as SHA-256 digests and compared in constant time across every configured digest with no early exit. Every failure fails closed: a key file readable by other users, a malformed entry, an unreadable file, or a malformed CIDR contributes no keys. The key file is re-read when its size, modification time, or mode changes, so keys can be rotated or revoked without a restart. A rejected key is logged only as an eight-hex-digit digest fingerprint. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01BwxtwuoRxP75PdR6NAmMS3 Signed-off-by: Aaron K. Clark (CryptoJones) (cherry picked from commit ef2636695d407bd4c68a3bd9c10a014e3282e9e6) --- services/shared/ingressauth/ingressauth.go | 461 ++++++++++++++++++ .../shared/ingressauth/ingressauth_test.go | 456 +++++++++++++++++ 2 files changed, 917 insertions(+) create mode 100644 services/shared/ingressauth/ingressauth.go create mode 100644 services/shared/ingressauth/ingressauth_test.go diff --git a/services/shared/ingressauth/ingressauth.go b/services/shared/ingressauth/ingressauth.go new file mode 100644 index 00000000..f6ad20a6 --- /dev/null +++ b/services/shared/ingressauth/ingressauth.go @@ -0,0 +1,461 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package ingressauth is the opt-in credential gate the inference proxies apply +// to a plaintext request that did not arrive from loopback. Both proxies share +// it for the same reason they share nvpair-shared/cors: the two must accept and +// refuse an outside caller identically, and one implementation is what keeps +// them from drifting. +// +// With nothing configured the gate is disabled and the proxies keep their +// loopback-only behavior — a LAN caller is refused before this package is +// consulted. An operator enables it by configuring at least one API key, either +// in a key file (NVPAIR_PROXY_API_KEYS_FILE, default /proxy-api-keys) or +// inline (NVPAIR_PROXY_API_KEYS). Once enabled, a non-loopback plaintext caller +// must present a configured key as "Authorization: Bearer " or, for clients +// built on the Anthropic SDK convention, "X-Api-Key: "; an optional +// NVPAIR_PROXY_ALLOWED_CIDRS narrows which source addresses may even try. +// Loopback callers are never asked for a key — the desktop application, the +// terminal interface, and local tools are unaffected by enabling the gate. +// +// Every failure fails closed. A key file that cannot be read, is readable by +// other users, or contains an entry that could never match over the wire +// contributes no keys, the reason is logged, and the LAN stays closed. Keys are +// held in memory only as SHA-256 digests and are compared in constant time; a +// rejected credential is logged as a short digest fingerprint, never as itself. +package ingressauth + +import ( + "bufio" + "crypto/sha256" + "crypto/subtle" + "encoding/hex" + "errors" + "fmt" + "io" + "io/fs" + "log/slog" + "net/http" + "net/netip" + "os" + "runtime" + "strings" + "sync" + "time" + "unicode" + + "nvpair-shared/appdir" +) + +const ( + // EnvKeys holds comma-separated API keys, for headless and container + // deployments where a file is inconvenient. Combined with the key file. + EnvKeys = "NVPAIR_PROXY_API_KEYS" + // EnvKeysFile overrides the key file path. Unset, the file is + // /DefaultKeyFileName and is consulted only if it exists. + EnvKeysFile = "NVPAIR_PROXY_API_KEYS_FILE" + // EnvAllowedCIDRs optionally lists comma-separated CIDR prefixes an + // authenticated non-loopback caller must originate from. + EnvAllowedCIDRs = "NVPAIR_PROXY_ALLOWED_CIDRS" + // DefaultKeyFileName is the key file's name inside the application data + // directory (see nvpair-shared/appdir). + DefaultKeyFileName = "proxy-api-keys" + + // MinKeyLength is the shortest key the gate accepts. A 32-character key + // drawn from hex already carries 128 bits, which puts online guessing out + // of reach without a lockout mechanism. + MinKeyLength = 32 + + // CodeUnauthorized is the ingress error code for a missing or wrong key. + CodeUnauthorized = "unauthorized" + // CodeSourceNotAllowed is the ingress error code for a caller outside the + // configured CIDR allowlist. + CodeSourceNotAllowed = "source-not-allowed" + + headerAuthorization = "Authorization" + headerAPIKey = "X-Api-Key" + bearerScheme = "bearer" + noCredential = "none" + + unauthorizedMessage = "a valid API key is required for non-loopback requests; " + + "send it as Authorization: Bearer or X-Api-Key: " +) + +// Decision is the gate's verdict on one request. When Allowed is false, Status, +// Code, and Message are what the proxy should answer with, in the same shape as +// its other ingress rejections. KeyFingerprint identifies the presented key for +// the log without revealing it; it is "none" when no credential was sent. +type Decision struct { + Allowed bool + Status int + Code string + Message string + KeyFingerprint string +} + +type digest = [sha256.Size]byte + +// fileStamp is the part of a key file's metadata that decides whether it must +// be re-read. Mode is included because fixing permissions with chmod changes +// neither size nor modification time, yet must take effect. +type fileStamp struct { + size int64 + modTime time.Time + mode fs.FileMode + exists bool +} + +// Gate holds the configured credentials and allowlist. Its zero value is a +// disabled gate; construct one with FromEnv or New. +type Gate struct { + mu sync.Mutex + + // inline keys come from the environment (or New) and never change. + inline []digest + // broken records an unrecoverable configuration error in the environment + // (a malformed inline key or CIDR). The gate then stays disabled for the + // life of the process, regardless of the key file. + broken bool + + // filePath, when non-empty, is re-checked on every Enabled call so keys can + // be rotated without restarting the proxy. explicitFile records that the + // operator named the path, so its absence is worth reporting. + filePath string + explicitFile bool + fileKeys []digest + fileStamp fileStamp + fileChecked bool + + cidrs []netip.Prefix + + // announced* remember the last state written to the log, so a change is + // reported once rather than on every request. + announcedOnce bool + announcedEnabled bool + announcedKeys int +} + +// New builds a gate from literal keys and prefixes, for tests and callers that +// resolve configuration themselves. Keys are validated like file entries; an +// invalid key panics, since a caller passing literals has a programming error +// rather than an operator mistake. +func New(keys []string, cidrs []netip.Prefix) *Gate { + g := &Gate{cidrs: cidrs} + for _, k := range keys { + if err := validateKey(k); err != nil { + panic("ingressauth.New: " + err.Error()) + } + g.inline = append(g.inline, sha256.Sum256([]byte(k))) + } + g.mu.Lock() + g.announceLocked() + g.mu.Unlock() + return g +} + +// FromEnv builds the gate from the process environment. It never fails: a +// configuration error is logged and yields a gate that stays disabled, which +// leaves the proxy in its loopback-only default. +func FromEnv() *Gate { + g := &Gate{} + + if raw := os.Getenv(EnvKeys); strings.TrimSpace(raw) != "" { + for i, k := range strings.Split(raw, ",") { + k = strings.TrimSpace(k) + if k == "" { + continue + } + if err := validateKey(k); err != nil { + slog.Error("authenticated LAN ingress disabled: invalid inline API key", + "env", EnvKeys, "entry", i+1, "err", err) + g.broken = true + break + } + g.inline = append(g.inline, sha256.Sum256([]byte(k))) + } + } + + if raw := os.Getenv(EnvAllowedCIDRs); strings.TrimSpace(raw) != "" { + for _, s := range strings.Split(raw, ",") { + s = strings.TrimSpace(s) + if s == "" { + continue + } + prefix, err := netip.ParsePrefix(s) + if err != nil { + slog.Error("authenticated LAN ingress disabled: invalid CIDR allowlist entry", + "env", EnvAllowedCIDRs, "entry", s, "err", err) + g.broken = true + break + } + g.cidrs = append(g.cidrs, prefix.Masked()) + } + } + + if p := os.Getenv(EnvKeysFile); p != "" { + g.filePath = p + g.explicitFile = true + } else if p, err := appdir.Path(DefaultKeyFileName); err == nil { + g.filePath = p + } else { + slog.Warn("authenticated LAN ingress: cannot resolve the default key file location", "err", err) + } + + g.mu.Lock() + g.refreshLocked() + g.announceLocked() + g.mu.Unlock() + return g +} + +// Enabled reports whether at least one API key is configured, re-reading the +// key file first if it changed. The proxy consults this per non-loopback +// request, so adding, rotating, or removing keys needs no restart. +func (g *Gate) Enabled() bool { + g.mu.Lock() + defer g.mu.Unlock() + g.refreshLocked() + g.announceLocked() + return g.enabledLocked() +} + +func (g *Gate) enabledLocked() bool { + return !g.broken && len(g.inline)+len(g.fileKeys) > 0 +} + +// Authorize decides whether a non-loopback plaintext request may proceed. The +// allowlist is checked before the credential, so a caller outside it learns +// nothing about whether its key is valid. Authorize does not write to the +// response; the proxy does, in its own error format. +func (g *Gate) Authorize(r *http.Request) Decision { + g.mu.Lock() + g.refreshLocked() + cidrs := g.cidrs + digests := make([]digest, 0, len(g.inline)+len(g.fileKeys)) + digests = append(digests, g.inline...) + digests = append(digests, g.fileKeys...) + enabled := g.enabledLocked() + g.mu.Unlock() + + if !enabled { + // The proxy only asks an enabled gate; answer conservatively anyway. + return Decision{Status: http.StatusForbidden, Code: CodeSourceNotAllowed, + Message: "authenticated LAN ingress is not enabled", KeyFingerprint: noCredential} + } + + if len(cidrs) > 0 { + ip, ok := remoteAddr(r) + if !ok || !anyPrefixContains(cidrs, ip) { + return Decision{Status: http.StatusForbidden, Code: CodeSourceNotAllowed, + Message: "the caller's address is outside " + EnvAllowedCIDRs, KeyFingerprint: noCredential} + } + } + + cred, ok := credentialFrom(r) + if !ok { + return Decision{Status: http.StatusUnauthorized, Code: CodeUnauthorized, + Message: unauthorizedMessage, KeyFingerprint: noCredential} + } + if !matchesAny(digests, sha256.Sum256([]byte(cred))) { + return Decision{Status: http.StatusUnauthorized, Code: CodeUnauthorized, + Message: unauthorizedMessage, KeyFingerprint: Fingerprint(cred)} + } + return Decision{Allowed: true, Status: http.StatusOK, KeyFingerprint: Fingerprint(cred)} +} + +// StripCredential removes the presented key from a request the gate admitted, +// so the proxy's credential is never forwarded to an engine or a peer. +func (g *Gate) StripCredential(h http.Header) { + h.Del(headerAuthorization) + h.Del(headerAPIKey) +} + +// Fingerprint returns the first eight hex characters of a key's SHA-256 digest: +// enough for an operator to tell repeated rejections of one misconfigured +// client apart from a scan, without the log ever holding the key. +func Fingerprint(key string) string { + sum := sha256.Sum256([]byte(key)) + return hex.EncodeToString(sum[:4]) +} + +// matchesAny compares the presented digest against every configured digest in +// constant time and without an early exit, so neither the key length nor the +// position of a match is observable through timing. +func matchesAny(configured []digest, presented digest) bool { + match := 0 + for i := range configured { + match |= subtle.ConstantTimeCompare(configured[i][:], presented[:]) + } + return match == 1 +} + +// credentialFrom extracts the client's key: a Bearer token first, then the +// X-Api-Key header. A query parameter is deliberately not accepted, because +// URLs end up in access logs and browser histories. +func credentialFrom(r *http.Request) (string, bool) { + if auth := strings.TrimSpace(r.Header.Get(headerAuthorization)); auth != "" { + scheme, token, found := strings.Cut(auth, " ") + if found && strings.EqualFold(scheme, bearerScheme) { + if token = strings.TrimSpace(token); token != "" { + return token, true + } + } + } + if key := strings.TrimSpace(r.Header.Get(headerAPIKey)); key != "" { + return key, true + } + return "", false +} + +// remoteAddr parses the transport-level peer address. Forwarding headers are +// never consulted: the gate is meant to face callers directly, and a header a +// caller sets itself is not evidence of where it is. +func remoteAddr(r *http.Request) (netip.Addr, bool) { + ap, err := netip.ParseAddrPort(r.RemoteAddr) + if err != nil { + return netip.Addr{}, false + } + return ap.Addr().Unmap(), true +} + +func anyPrefixContains(prefixes []netip.Prefix, ip netip.Addr) bool { + for _, p := range prefixes { + if p.Contains(ip) { + return true + } + } + return false +} + +// refreshLocked re-reads the key file when its metadata changed since the last +// look. Caller holds g.mu. +func (g *Gate) refreshLocked() { + if g.filePath == "" || g.broken { + return + } + info, err := os.Stat(g.filePath) + var stamp fileStamp + switch { + case err == nil: + stamp = fileStamp{size: info.Size(), modTime: info.ModTime(), mode: info.Mode(), exists: true} + case errors.Is(err, fs.ErrNotExist): + stamp = fileStamp{} + default: + // A stat failure other than absence (a parent directory's permissions, + // an I/O error) counts as absence for this request and is re-examined + // on the next; report it when it is news. + if !g.fileChecked || g.fileStamp.exists { + slog.Error("authenticated LAN ingress: cannot stat key file; no file keys are in effect", + "path", g.filePath, "err", err) + } + stamp = fileStamp{} + } + if g.fileChecked && stamp == g.fileStamp { + return + } + g.fileChecked = true + g.fileStamp = stamp + g.fileKeys = nil + + if !stamp.exists { + if g.explicitFile { + slog.Error("authenticated LAN ingress: key file does not exist; no file keys are in effect", + "env", EnvKeysFile, "path", g.filePath) + } + return + } + keys, err := loadKeyFile(g.filePath, info) + if err != nil { + slog.Error("authenticated LAN ingress: key file ignored; no file keys are in effect", + "path", g.filePath, "err", err) + return + } + g.fileKeys = keys +} + +// loadKeyFile reads and validates a key file. On Unix-like systems the file must +// not be readable or writable by group or others; on Windows the mode bits +// carry no such meaning and the check is skipped. +func loadKeyFile(path string, info fs.FileInfo) ([]digest, error) { + if !info.Mode().IsRegular() { + return nil, errors.New("not a regular file") + } + if runtime.GOOS != "windows" { + if perm := info.Mode().Perm(); perm&0o077 != 0 { + return nil, fmt.Errorf("permissions %04o allow other users to read it; chmod 600", perm) + } + } + f, err := os.Open(path) + if err != nil { + return nil, err + } + defer f.Close() + return parseKeys(f) +} + +// parseKeys reads one key per line. Blank lines and lines starting with '#' are +// ignored; surrounding whitespace, including a CR from a Windows editor, is +// trimmed. Any invalid entry fails the whole file: a key that can never match +// over the wire is a misconfiguration to surface, not to skip. +func parseKeys(r io.Reader) ([]digest, error) { + var keys []digest + sc := bufio.NewScanner(r) + line := 0 + for sc.Scan() { + line++ + entry := strings.TrimSpace(sc.Text()) + if entry == "" || strings.HasPrefix(entry, "#") { + continue + } + if err := validateKey(entry); err != nil { + return nil, fmt.Errorf("line %d: %w", line, err) + } + keys = append(keys, sha256.Sum256([]byte(entry))) + } + if err := sc.Err(); err != nil { + return nil, err + } + if len(keys) == 0 { + return nil, errors.New("contains no keys") + } + return keys, nil +} + +// validateKey enforces the shape a key must have to be usable at all: long +// enough to resist guessing, and printable ASCII with no whitespace so it +// survives an HTTP header unchanged. +func validateKey(key string) error { + if len(key) < MinKeyLength { + return fmt.Errorf("key is %d characters; at least %d are required", len(key), MinKeyLength) + } + for _, c := range key { + if c > unicode.MaxASCII || c <= ' ' || c == 0x7f { + return errors.New("key must be printable ASCII with no whitespace") + } + } + return nil +} + +// announceLocked logs a change in the gate's state — enabled with N keys, or +// back to disabled — once per change. Enabling is logged at Warn: it widens the +// proxy's exposure and an operator reading the log should see it plainly. +// Caller holds g.mu. +func (g *Gate) announceLocked() { + enabled := g.enabledLocked() + n := len(g.inline) + len(g.fileKeys) + if g.announcedOnce && enabled == g.announcedEnabled && n == g.announcedKeys { + return + } + g.announcedOnce, g.announcedEnabled, g.announcedKeys = true, enabled, n + if enabled { + cidrs := make([]string, 0, len(g.cidrs)) + for _, p := range g.cidrs { + cidrs = append(cidrs, p.String()) + } + slog.Warn("authenticated LAN ingress ENABLED: a non-loopback plaintext caller presenting a configured API key is routed", + "keys", n, "key_file", g.filePath, "allowed_cidrs", cidrs) + return + } + slog.Info("authenticated LAN ingress disabled; plaintext requests are accepted from loopback only", + "key_file", g.filePath) +} diff --git a/services/shared/ingressauth/ingressauth_test.go b/services/shared/ingressauth/ingressauth_test.go new file mode 100644 index 00000000..c7e2a70d --- /dev/null +++ b/services/shared/ingressauth/ingressauth_test.go @@ -0,0 +1,456 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package ingressauth + +import ( + "net/http" + "net/http/httptest" + "net/netip" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + "time" +) + +const ( + keyA = "0123456789abcdef0123456789abcdef" // exactly MinKeyLength + keyB = "b-key-b-key-b-key-b-key-b-key-b-key-b-key-0000" // longer, with dashes + keyC = "sk-nvpair-cccccccccccccccccccccccccccccccccccccccccccccccccccccc" // prefixed, like SDK keys +) + +func request(remote string, hdr ...string) *http.Request { + r := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil) + r.RemoteAddr = remote + for i := 0; i+1 < len(hdr); i += 2 { + r.Header.Set(hdr[i], hdr[i+1]) + } + return r +} + +func TestValidateKey(t *testing.T) { + cases := []struct { + name string + key string + ok bool + }{ + {"exactly minimum length", keyA, true}, + {"longer with punctuation", keyC, true}, + {"one short", keyA[:MinKeyLength-1], false}, + {"empty", "", false}, + {"embedded space", "0123456789abcdef 123456789abcdef0", false}, + {"embedded tab", "0123456789abcdef\t123456789abcdef0", false}, + {"non-ascii", "0123456789abcdef0123456789abcdé", false}, + {"control char", "0123456789abcdef0123456789abcde\x01", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validateKey(tc.key) + if (err == nil) != tc.ok { + t.Fatalf("validateKey(%q) err = %v, want ok=%v", tc.key, err, tc.ok) + } + }) + } +} + +func TestParseKeysSkipsCommentsBlanksAndCRLF(t *testing.T) { + in := "# leading comment\r\n\r\n " + keyA + " \r\n" + keyB + "\n\n # trailing comment\n" + keys, err := parseKeys(strings.NewReader(in)) + if err != nil { + t.Fatalf("parseKeys: %v", err) + } + if len(keys) != 2 { + t.Fatalf("parsed %d keys, want 2", len(keys)) + } + g := &Gate{inline: keys} + for _, k := range []string{keyA, keyB} { + if d := g.Authorize(request("192.0.2.9:1", "Authorization", "Bearer "+k)); !d.Allowed { + t.Errorf("key %q from file not accepted: %+v", k, d) + } + } +} + +func TestParseKeysRejectsWholeFileOnOneBadEntry(t *testing.T) { + cases := map[string]string{ + "short entry": keyA + "\nshort\n", + "whitespace": "0123456789abcdef 123456789abcdef0\n", + "only comments": "# nothing here\n\n", + "empty": "", + "non-ascii line": keyA + "\n0123456789abcdef0123456789abcdé\n", + } + for name, in := range cases { + t.Run(name, func(t *testing.T) { + if keys, err := parseKeys(strings.NewReader(in)); err == nil { + t.Fatalf("parseKeys accepted %d keys, want an error", len(keys)) + } + }) + } +} + +func TestAuthorizeCredentials(t *testing.T) { + g := New([]string{keyA, keyB}, nil) + cases := []struct { + name string + hdr []string + allow bool + status int + fp string + }{ + {"bearer first key", []string{"Authorization", "Bearer " + keyA}, true, http.StatusOK, Fingerprint(keyA)}, + {"bearer second key", []string{"Authorization", "Bearer " + keyB}, true, http.StatusOK, Fingerprint(keyB)}, + {"lowercase scheme", []string{"Authorization", "bearer " + keyA}, true, http.StatusOK, Fingerprint(keyA)}, + {"x-api-key", []string{"X-Api-Key", keyA}, true, http.StatusOK, Fingerprint(keyA)}, + {"x-api-key lowercase header name", []string{"x-api-key", keyB}, true, http.StatusOK, Fingerprint(keyB)}, + {"no credential", nil, false, http.StatusUnauthorized, noCredential}, + {"wrong key", []string{"Authorization", "Bearer " + keyC}, false, http.StatusUnauthorized, Fingerprint(keyC)}, + {"wrong scheme", []string{"Authorization", "Basic " + keyA}, false, http.StatusUnauthorized, noCredential}, + {"bearer with no token", []string{"Authorization", "Bearer "}, false, http.StatusUnauthorized, noCredential}, + {"key as prefix only", []string{"Authorization", "Bearer " + keyA + "x"}, false, http.StatusUnauthorized, Fingerprint(keyA + "x")}, + {"wrong bearer but right x-api-key", []string{"Authorization", "Bearer " + keyC, "X-Api-Key", keyA}, false, http.StatusUnauthorized, Fingerprint(keyC)}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + d := g.Authorize(request("192.0.2.9:40000", tc.hdr...)) + if d.Allowed != tc.allow || d.Status != tc.status { + t.Fatalf("decision = %+v, want allowed=%v status=%d", d, tc.allow, tc.status) + } + if d.KeyFingerprint != tc.fp { + t.Errorf("fingerprint = %q, want %q", d.KeyFingerprint, tc.fp) + } + if !tc.allow && d.Code != CodeUnauthorized { + t.Errorf("code = %q, want %q", d.Code, CodeUnauthorized) + } + if strings.Contains(d.Message, keyA) || strings.Contains(d.Message, keyC) { + t.Errorf("message echoes a key: %q", d.Message) + } + }) + } +} + +func TestAuthorizeCIDRAllowlist(t *testing.T) { + g := New([]string{keyA}, []netip.Prefix{ + netip.MustParsePrefix("192.168.1.0/24"), + netip.MustParsePrefix("fd00::/8"), + }) + auth := []string{"Authorization", "Bearer " + keyA} + cases := []struct { + name string + remote string + hdr []string + allow bool + code string + }{ + {"inside v4 with key", "192.168.1.77:5000", auth, true, ""}, + {"inside v6 with key", "[fd00::1]:5000", auth, true, ""}, + {"ipv4-mapped v6 inside", "[::ffff:192.168.1.77]:5000", auth, true, ""}, + {"outside with valid key", "192.168.2.77:5000", auth, false, CodeSourceNotAllowed}, + {"outside without key", "10.0.0.5:5000", nil, false, CodeSourceNotAllowed}, + {"inside without key", "192.168.1.77:5000", nil, false, CodeUnauthorized}, + {"unparseable remote", "garbage", auth, false, CodeSourceNotAllowed}, + {"empty remote", "", auth, false, CodeSourceNotAllowed}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + d := g.Authorize(request(tc.remote, tc.hdr...)) + if d.Allowed != tc.allow { + t.Fatalf("decision = %+v, want allowed=%v", d, tc.allow) + } + if !tc.allow && d.Code != tc.code { + t.Errorf("code = %q, want %q", d.Code, tc.code) + } + if d.Code == CodeSourceNotAllowed { + if d.Status != http.StatusForbidden { + t.Errorf("status = %d, want 403", d.Status) + } + // An out-of-policy source learns nothing about its key. + if d.KeyFingerprint != noCredential { + t.Errorf("fingerprint = %q, want %q for a source rejection", d.KeyFingerprint, noCredential) + } + } + }) + } +} + +func TestAuthorizeIgnoresForwardingHeaders(t *testing.T) { + g := New([]string{keyA}, []netip.Prefix{netip.MustParsePrefix("192.168.1.0/24")}) + d := g.Authorize(request("10.9.9.9:1", + "Authorization", "Bearer "+keyA, + "X-Forwarded-For", "192.168.1.5", + "X-Real-IP", "192.168.1.5", + "Forwarded", "for=192.168.1.5")) + if d.Allowed { + t.Fatal("a forwarding header moved the caller inside the allowlist") + } +} + +func TestDisabledGateNeverAllows(t *testing.T) { + var zero Gate + if zero.Enabled() { + t.Fatal("zero Gate reports enabled") + } + if d := zero.Authorize(request("192.0.2.9:1", "Authorization", "Bearer "+keyA)); d.Allowed { + t.Fatalf("zero Gate allowed a request: %+v", d) + } + empty := New(nil, nil) + if empty.Enabled() { + t.Fatal("New(nil, nil) reports enabled") + } +} + +func TestFingerprintIsShortStableHexAndNotTheKey(t *testing.T) { + fp := Fingerprint(keyA) + if len(fp) != 8 { + t.Fatalf("fingerprint %q has length %d, want 8", fp, len(fp)) + } + if strings.ToLower(fp) != fp || strings.Trim(fp, "0123456789abcdef") != "" { + t.Fatalf("fingerprint %q is not lowercase hex", fp) + } + if fp != Fingerprint(keyA) { + t.Fatal("fingerprint is not stable") + } + if fp == Fingerprint(keyB) { + t.Fatal("distinct keys share a fingerprint") + } + if strings.Contains(keyA, fp) { + t.Fatal("fingerprint is a substring of the key") + } +} + +func TestStripCredentialRemovesBothHeaders(t *testing.T) { + g := New([]string{keyA}, nil) + r := request("192.0.2.9:1", "Authorization", "Bearer "+keyA, "X-Api-Key", keyA, "Content-Type", "application/json") + g.StripCredential(r.Header) + if r.Header.Get("Authorization") != "" || r.Header.Get("X-Api-Key") != "" { + t.Fatalf("credential headers survived: %v", r.Header) + } + if r.Header.Get("Content-Type") != "application/json" { + t.Fatal("an unrelated header was removed") + } +} + +func TestNewPanicsOnInvalidLiteralKey(t *testing.T) { + defer func() { + if recover() == nil { + t.Fatal("New accepted an invalid literal key") + } + }() + New([]string{"short"}, nil) +} + +// writeKeyFile writes content with mode and pins the modification time so a +// rewrite is distinguishable from the previous version even on filesystems +// with coarse timestamps. +func writeKeyFile(t *testing.T, path, content string, mode os.FileMode, when time.Time) { + t.Helper() + if err := os.WriteFile(path, []byte(content), mode); err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, mode); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(path, when, when); err != nil { + t.Fatal(err) + } +} + +func TestKeyFileLoadsRotatesAndFailsClosed(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "keys") + g := &Gate{filePath: path, explicitFile: true} + base := time.Now().Add(-time.Hour).Truncate(time.Second) + + if g.Enabled() { + t.Fatal("enabled before the key file exists") + } + + writeKeyFile(t, path, "# first key\n"+keyA+"\n", 0o600, base) + if !g.Enabled() { + t.Fatal("not enabled after the key file appeared") + } + if d := g.Authorize(request("192.0.2.9:1", "Authorization", "Bearer "+keyA)); !d.Allowed { + t.Fatalf("file key rejected: %+v", d) + } + + // Rotation: replace A with B without a restart. + writeKeyFile(t, path, keyB+"\n", 0o600, base.Add(10*time.Second)) + if d := g.Authorize(request("192.0.2.9:1", "Authorization", "Bearer "+keyA)); d.Allowed { + t.Fatal("rotated-out key still accepted") + } + if d := g.Authorize(request("192.0.2.9:1", "Authorization", "Bearer "+keyB)); !d.Allowed { + t.Fatalf("rotated-in key rejected: %+v", d) + } + + // A malformed rewrite contributes nothing: the LAN closes rather than + // staying open on the previous keys. + writeKeyFile(t, path, keyB+"\nshort\n", 0o600, base.Add(20*time.Second)) + if g.Enabled() { + t.Fatal("enabled on a malformed key file") + } + if d := g.Authorize(request("192.0.2.9:1", "Authorization", "Bearer "+keyB)); d.Allowed { + t.Fatal("previous keys survived a failed reload") + } + + // Repairing the file re-enables; removing it disables. + writeKeyFile(t, path, keyB+"\n", 0o600, base.Add(30*time.Second)) + if !g.Enabled() { + t.Fatal("not re-enabled after the file was repaired") + } + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + if g.Enabled() { + t.Fatal("enabled after the key file was removed") + } +} + +func TestKeyFilePermissionsMustBePrivate(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits are not meaningful on Windows") + } + dir := t.TempDir() + path := filepath.Join(dir, "keys") + g := &Gate{filePath: path, explicitFile: true} + when := time.Now().Add(-time.Hour).Truncate(time.Second) + + writeKeyFile(t, path, keyA+"\n", 0o644, when) + if g.Enabled() { + t.Fatal("enabled on a world-readable key file") + } + // chmod alone changes neither size nor mtime; the gate must still notice. + if err := os.Chmod(path, 0o600); err != nil { + t.Fatal(err) + } + if !g.Enabled() { + t.Fatal("not enabled after permissions were tightened") + } + if err := os.Chmod(path, 0o640); err != nil { + t.Fatal(err) + } + if g.Enabled() { + t.Fatal("enabled on a group-readable key file") + } +} + +func TestKeyFileMustBeRegular(t *testing.T) { + dir := t.TempDir() + g := &Gate{filePath: dir, explicitFile: true} + if g.Enabled() { + t.Fatal("a directory was accepted as a key file") + } +} + +// clearEnv points every variable the gate reads, and every base directory +// appdir consults, at the test's own scratch space. +func clearEnv(t *testing.T) string { + t.Helper() + home := t.TempDir() + t.Setenv(EnvKeys, "") + t.Setenv(EnvKeysFile, "") + t.Setenv(EnvAllowedCIDRs, "") + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", home) + t.Setenv("LOCALAPPDATA", home) + t.Setenv("APPDATA", home) + return home +} + +func TestFromEnvUnsetIsDisabled(t *testing.T) { + clearEnv(t) + g := FromEnv() + if g.Enabled() { + t.Fatal("FromEnv with nothing configured is enabled") + } + if g.filePath == "" || filepath.Base(g.filePath) != DefaultKeyFileName { + t.Fatalf("default key file path = %q, want .../%s", g.filePath, DefaultKeyFileName) + } +} + +func TestFromEnvInlineKeysAndFileCombine(t *testing.T) { + home := clearEnv(t) + path := filepath.Join(home, "keys") + writeKeyFile(t, path, keyB+"\n", 0o600, time.Now().Add(-time.Hour)) + t.Setenv(EnvKeys, " "+keyA+" , ,"+keyC) + t.Setenv(EnvKeysFile, path) + + g := FromEnv() + if !g.Enabled() { + t.Fatal("not enabled") + } + for _, k := range []string{keyA, keyB, keyC} { + if d := g.Authorize(request("192.0.2.9:1", "X-Api-Key", k)); !d.Allowed { + t.Errorf("key %q rejected: %+v", k, d) + } + } +} + +func TestFromEnvDefaultFileInAppDir(t *testing.T) { + home := clearEnv(t) + dir, err := os.UserConfigDir() + if err != nil { + t.Skip("no user config dir:", err) + } + if !strings.HasPrefix(dir, home) { + t.Skipf("os.UserConfigDir()=%q is not under the test HOME %q on this platform", dir, home) + } + appDir := filepath.Join(dir, "Nvidia Corporation", "Personal AI Router") + if err := os.MkdirAll(appDir, 0o700); err != nil { + t.Fatal(err) + } + writeKeyFile(t, filepath.Join(appDir, DefaultKeyFileName), keyA+"\n", 0o600, time.Now().Add(-time.Hour)) + + g := FromEnv() + if !g.Enabled() { + t.Fatalf("default key file at %q not picked up", g.filePath) + } +} + +func TestFromEnvInvalidInlineKeyDisablesEverything(t *testing.T) { + home := clearEnv(t) + path := filepath.Join(home, "keys") + writeKeyFile(t, path, keyB+"\n", 0o600, time.Now().Add(-time.Hour)) + t.Setenv(EnvKeysFile, path) + t.Setenv(EnvKeys, keyA+",too-short") + + g := FromEnv() + if g.Enabled() { + t.Fatal("enabled despite an invalid inline key") + } + if d := g.Authorize(request("192.0.2.9:1", "X-Api-Key", keyB)); d.Allowed { + t.Fatal("file key accepted while the environment is misconfigured") + } +} + +func TestFromEnvInvalidCIDRDisablesEverything(t *testing.T) { + clearEnv(t) + t.Setenv(EnvKeys, keyA) + t.Setenv(EnvAllowedCIDRs, "192.168.1.0/24, not-a-cidr") + + g := FromEnv() + if g.Enabled() { + t.Fatal("enabled despite a malformed CIDR allowlist") + } +} + +func TestFromEnvCIDRsAreMaskedAndApplied(t *testing.T) { + clearEnv(t) + t.Setenv(EnvKeys, keyA) + t.Setenv(EnvAllowedCIDRs, "192.168.1.77/24") + + g := FromEnv() + if d := g.Authorize(request("192.168.1.1:1", "X-Api-Key", keyA)); !d.Allowed { + t.Fatalf("host bits in the prefix were not masked: %+v", d) + } + if d := g.Authorize(request("192.168.2.1:1", "X-Api-Key", keyA)); d.Allowed { + t.Fatal("caller outside the allowlist admitted") + } +} + +func TestFromEnvExplicitMissingFileIsDisabled(t *testing.T) { + home := clearEnv(t) + t.Setenv(EnvKeysFile, filepath.Join(home, "does-not-exist")) + if g := FromEnv(); g.Enabled() { + t.Fatal("enabled with a missing explicit key file") + } +} From 751f98e94b50a85f83b7c4acd3ac3f60d93635ac Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 22 Sep 2026 15:14:38 -0500 Subject: [PATCH 2/6] Port the authenticated LAN ingress onto the unified nvpair-proxy develop merged the Ollama and LM Studio proxies into a single nvpair-proxy process (one facade per enabled engine), so this rebases the branch's four commits onto develop and re-applies the gate there: - handlePlain on the facade consults the gate; lanAuth lives on Proxy so every enabled engine's facade enforces one identical gate. - The default stays loopback-only; an enabled gate answers the CIDR allowlist first (even for a preflight), then 401 with the RFC 6750 challenge, and strips the key before routing. - ingress_auth_test.go is adapted to soleFacade/host.lanAuth. No version-file edits: versions are now declared via the PR's release-intent block and applied by CI. Signed-off-by: Aaron K. Clark --- SECURITY.md | 51 ++- docs/architecture.mdx | 21 +- docs/getting-started.mdx | 83 ++++- docs/overview.mdx | 6 +- docs/troubleshooting.mdx | 15 + fern/openapi.yml | 17 + services/nvpair-proxy/ingress.go | 67 +++- services/nvpair-proxy/ingress_auth_test.go | 285 +++++++++++++++ services/nvpair-proxy/main.go | 5 + services/nvpair-proxy/proxy.go | 8 + services/shared/ingressauth/ingressauth.go | 331 ++++++++++++------ .../shared/ingressauth/ingressauth_test.go | 260 +++++++++++++- services/shared/ingressauth/owner_unix.go | 31 ++ .../shared/ingressauth/owner_unix_test.go | 59 ++++ services/shared/ingressauth/owner_windows.go | 13 + 15 files changed, 1089 insertions(+), 163 deletions(-) create mode 100644 services/nvpair-proxy/ingress_auth_test.go create mode 100644 services/shared/ingressauth/owner_unix.go create mode 100644 services/shared/ingressauth/owner_unix_test.go create mode 100644 services/shared/ingressauth/owner_windows.go diff --git a/SECURITY.md b/SECURITY.md index 957f9da4..eb3fff26 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -64,17 +64,58 @@ transport. These statements describe security boundaries visible in the current source. They are not claims that every deployment is secure. -### Inference Endpoints Are Loopback-Only +### Inference Endpoints Are Loopback-Only by Default A proxy's plaintext personality accepts requests from loopback only. It refuses a plaintext request from any other address. The same port also serves a mutual-TLS ingress for paired cluster members, and PAIR forwards that traffic to the node's own engine rather than routing it onward. -This is deliberate. A network-reachable plaintext endpoint would make any node an -open relay for inference to anything that can route to it. Run an application on -a node and use that node's local endpoint. Exposing an engine to the network -directly is outside PAIR and is the operator's decision and risk. +This is deliberate. A network-reachable plaintext endpoint without a credential +would make any node an open relay for inference to anything that can route to +it. Run an application on a node and use that node's local endpoint. Exposing an +engine to the network directly is outside PAIR and is the operator's decision +and risk. + +An operator can opt in to authenticated LAN access by configuring one or more +API keys (`NVPAIR_PROXY_API_KEYS_FILE`, default `proxy-api-keys` in PAIR's data +directory, or `NVPAIR_PROXY_API_KEYS`). With a key configured, a non-loopback +plaintext request is admitted only when it presents a configured key as +`Authorization: Bearer ` or `X-Api-Key: `, and, when +`NVPAIR_PROXY_ALLOWED_CIDRS` is set, only from a listed source range. An admitted +request is routed like a loopback client, and the key is stripped before the +request is forwarded, so it never reaches an engine or a peer. Loopback callers +are never asked for a key. + +A key therefore reaches everything a loopback client on that node can reach: +inference routed onward to other paired nodes, and every engine route the proxy +forwards, which for Ollama includes model management (`/api/pull`, +`/api/delete`, `/api/create`, `/api/copy`). Treat it as a credential for the +cluster, not for one machine. Enabling the gate restricts the network, not the +host: on a multi-user machine, every local account keeps loopback access to the +proxy regardless of the gate. Any process running as the proxy's own user can +also enable the gate by creating the key file, just as it could set the +environment; the proxy announces that at warning level in its log. + +The loopback exemption also means that anything terminating connections on the +node itself and re-originating them locally — a reverse proxy or TLS terminator +on the same host, or Docker Desktop on macOS forwarding container traffic to the +host — presents every one of its clients to PAIR as a loopback caller that is +never asked for a key. Such a front end must enforce its own authentication, or +run on a different host so that PAIR sees its real address. + +The gate compares keys in constant time, holds file keys in memory only as +digests (a key supplied inline through the environment also remains in the +process environment in clear, so prefer the file), never logs a presented key +(a rejection logs a short digest fingerprint), and fails closed: a key file that other users can read (judged by Unix permission +bits; on Windows the check is skipped and the per-user data directory's ACL is +the protection), that contains a malformed entry, or that cannot be read +contributes no keys and the LAN stays closed. +Enabling the gate is logged at warning level at startup and whenever the key set +changes. It adds no TLS, rate limiting, or per-key permissions: the plaintext +personality stays plaintext, so use it only on a network you trust, and pair it +with `NVPAIR_PROXY_ALLOWED_CIDRS` so a leaked key is only usable from the +networks you expect. ### Local Network Is a Trust-Relevant Boundary diff --git a/docs/architecture.mdx b/docs/architecture.mdx index b1c50e79..536f0c25 100644 --- a/docs/architecture.mdx +++ b/docs/architecture.mdx @@ -166,7 +166,7 @@ Not every surface is gated, and the exceptions are deliberate: | Surface | Transport | | --- | --- | -| Proxy, local clients | Plaintext HTTP, loopback only | +| Proxy, local clients | Plaintext HTTP, loopback only (opt-in: API-key authenticated LAN callers) | | Proxy, cluster ingress | Mutual TLS | | Model inventory, remote engine control | Mutual TLS | | Workload replication, error synchronization | Mutual TLS | @@ -192,7 +192,7 @@ port, chosen by the connection's first byte. A TLS handshake record starts with | First Byte | Personality | Who Uses It | | --- | --- | --- | -| Not `0x16` | Plaintext HTTP, loopback only | Your local applications | +| Not `0x16` | Plaintext HTTP, loopback only by default | Your local applications; API-key holders when the operator enables LAN access | | `0x16` | Mutual TLS | Paired nodes in the cluster | This is why an endpoint is `http://127.0.0.1:11434` for an application on the @@ -203,15 +203,22 @@ plaintext. The loopback restriction is enforced, not merely conventional. The listener binds all interfaces so the TLS personality can accept peers, but a plaintext request from any non-loopback address is refused with `403`. Without that check the port -would be an open relay for anything on the network. +would be an open relay for anything on the network. The one exception is opt-in: +an operator who configures API keys (refer to +[Reaching PAIR from Another Machine](getting-started.mdx#reaching-pair-from-another-machine)) +admits a non-loopback caller that presents a configured key to the same router a +loopback client uses. A caller without one is still refused, with `401`, and +loopback callers are never asked for a key. Two consequences follow, and together they define how clients are expected to reach PAIR: -- **A machine that is not a node has no way in.** It cannot use the plaintext - personality, because it is not loopback, and it cannot use the TLS personality, - because it holds no pinned cluster certificate. Pointing an application at - another machine's proxy port does not work by design. +- **A machine that is not a node has no way in unless the operator hands it a + key.** It cannot use the plaintext personality, because it is not loopback, and + it cannot use the TLS personality, because it holds no pinned cluster + certificate. Pointing an application at another machine's proxy port does not + work by design, until that machine's operator enables authenticated LAN access + and gives the application an API key. - **A peer request is served, not re-routed.** The mTLS ingress forwards straight to that node's own engine and never re-enters candidate selection, so a peer cannot chain a request onward through a third node. diff --git a/docs/getting-started.mdx b/docs/getting-started.mdx index cb7e7e34..46b43777 100644 --- a/docs/getting-started.mdx +++ b/docs/getting-started.mdx @@ -427,11 +427,12 @@ PAIR. This is the part that surprises people, so it is worth stating directly. -**An endpoint only accepts requests from the machine it is on.** A PAIR proxy -serves plaintext HTTP to loopback only. PAIR refuses a request arriving from -anywhere else on the network with `403`, and the message says so. You cannot -point an application on a fourth machine at `http://some-node:11434` and have it -work. +**By default, an endpoint only accepts requests from the machine it is on.** A +PAIR proxy serves plaintext HTTP to loopback only. PAIR refuses a request +arriving from anywhere else on the network with `403`, and the message says so. +You cannot point an application on a fourth machine at `http://some-node:11434` +and have it work, unless that node's operator has enabled authenticated access, +described below. **Run PAIR where you work.** The intended pattern is to install PAIR on the machine you use, pair it into the cluster, and point your applications at *its* @@ -443,11 +444,73 @@ PAIR makes the routing decision where you make the request. When a cluster peer reaches a node over its authenticated channel, that node's own engine serves the request and does not forward it onward. -**If you need a network-reachable inference endpoint,** that is outside -what PAIR does. You would configure an engine to listen on your network yourself -and take on the exposure that implies. PAIR does not offer it by default and there -is no plan to add it as an option, because it would turn any node into an open -relay for anything on the network. +**If you need a network-reachable inference endpoint,** PAIR can provide one, +but only when you turn it on and only to clients that hold a key. PAIR never +offers it by default, because an endpoint anything on the network can use +without a credential would turn the node into an open relay. + +### Reaching PAIR from Another Machine + +Some clients cannot run PAIR themselves: an automation host, a container, a +Kubernetes workload, an SDK on a machine you do not administer. For those, a +node can accept requests from the network when the caller presents an API key. +The default is unchanged. With no key configured, the endpoint stays local. + +1. Generate a key: at least 32 characters drawn from letters, digits, and + `- . _ ~ + / =`, produced randomly. Nothing limits how fast a caller can + guess, so a memorable passphrase is not a substitute: + + ```bash + openssl rand -hex 32 + ``` + +2. Put it in the proxy key file on the node, one key per line (`#` starts a + comment). The default location is `proxy-api-keys` in PAIR's data directory: + + | OS | Path | + | --- | --- | + | Windows | `%LOCALAPPDATA%\Nvidia Corporation\Personal AI Router\proxy-api-keys` | + | Linux | `~/.config/Nvidia Corporation/Personal AI Router/proxy-api-keys` | + | macOS | `~/Library/Application Support/Nvidia Corporation/Personal AI Router/proxy-api-keys` | + + On Linux and macOS the file must be readable by you alone (`chmod 600`). A + file other users can read is ignored, and the node's log says so. On + Windows the default location is inside your own `%LOCALAPPDATA%`, which + other accounts cannot read by default; PAIR does not check the ACL itself. + +3. Point the client at the node's address and the port shown in **Endpoints**, + for example `http://gpu-box:11434` or `http://gpu-box:1234/v1`, and send the + key as a bearer token: + + ```bash + curl http://gpu-box:1234/v1/models -H "Authorization: Bearer $KEY" + ``` + + OpenAI-compatible SDKs send this header when you give them the key as their + API key. Clients built on the Anthropic convention may send + `X-Api-Key: ` instead. + +The proxy notices the key file appearing, changing, or disappearing on its own, +so you can add, rotate, or revoke keys without restarting PAIR. When you replace +it from a script, write the new file beside the old one and rename it into place; +a rewrite that truncates the file first is briefly seen as an empty key file and +refused until the next check, about a second later. Three environment +variables refine the behavior for headless or containerized nodes: +`NVPAIR_PROXY_API_KEYS_FILE` names a different key file, `NVPAIR_PROXY_API_KEYS` +supplies keys inline (comma-separated), and `NVPAIR_PROXY_ALLOWED_CIDRS` (for +example `192.168.1.0/24,10.0.0.0/8`) additionally restricts which source networks +may use a key at all. + +Understand what you are enabling. The connection is plain HTTP, so prompts and +responses cross your network unencrypted, and anyone holding the key can use the +node, and through it every node in the cluster, exactly as a local application +could. Use it on a network you trust, keep the key private, and prefer the CIDR +allowlist. Applications on the node itself keep working over loopback without a +key, and so does anything that terminates connections on the node and forwards +them locally, such as a reverse proxy or TLS terminator on the same machine: PAIR +sees those clients as loopback and never asks them for a key, so such a front +end must do its own authentication. The node logs a warning when authenticated access is enabled and logs every +request it refuses, identifying a rejected key only by a short fingerprint. ## Verify It Is Working diff --git a/docs/overview.mdx b/docs/overview.mdx index fb519d50..997ae13c 100644 --- a/docs/overview.mdx +++ b/docs/overview.mdx @@ -125,7 +125,8 @@ and the cluster refuses a machine that is not a member. The PIN is a short convenience code for bootstrapping that exchange, not a strong authenticator, so pair only over networks and with machines you trust. Local -applications reach the proxy over loopback. +applications reach the proxy over loopback; an operator can also admit +applications on other machines by API key. For the trust boundaries in detail, refer to [Architecture](architecture.mdx) and the [security policy](../SECURITY.md). @@ -141,7 +142,8 @@ PAIR provides these capabilities: - Model-aware, workload-informed routing of independent requests. - Encrypted routing between machines: a request sent to another node travels over mutual TLS restricted to the nodes you have paired, and the cluster refuses a - machine that is not a member. Local applications reach the proxy over loopback. + machine that is not a member. Local applications reach the proxy over loopback, + and an operator can opt in to API-key access for applications on other machines. - A desktop application, plus a terminal interface for headless machines, driving the same services. - Visibility into nodes, engines, models, workloads, and service errors. diff --git a/docs/troubleshooting.mdx b/docs/troubleshooting.mdx index e4d8822c..ab44b5e7 100644 --- a/docs/troubleshooting.mdx +++ b/docs/troubleshooting.mdx @@ -98,6 +98,21 @@ needs no GPU or engine of its own, and its local endpoint routes to nodes that have them. Refer to [The Endpoint Is Local to the Machine Running PAIR](getting-started.mdx#the-endpoint-is-local-to-the-machine-running-pair). +If the application must stay where it is, the node's operator can enable +authenticated access with an API key; refer to +[Reaching PAIR from Another Machine](getting-started.mdx#reaching-pair-from-another-machine). +The refusal then says what is missing: + +- `403` `loopback-only`: the node has no usable key configured. Check the node's + log; a key file that other users can read, or that contains a malformed entry, + is ignored and the reason is logged. +- `401` `unauthorized`: the request carried no key, or a key the node does not + have. Send it as `Authorization: Bearer ` (or `X-Api-Key: `). The + node's log shows the first eight hex digits of the SHA-256 of the key it + received, so you can tell a missing key from a mistyped one. +- `403` `source-not-allowed`: the node restricts callers with + `NVPAIR_PROXY_ALLOWED_CIDRS` and the application's address is outside it. + For an application on the same machine, copy the URL from **Endpoints > API endpoints** rather than assuming a port. PAIR takes the engine's usual port for its compatible proxy and moves the engine itself to the diff --git a/fern/openapi.yml b/fern/openapi.yml index 2a9b2ac8..2fc0d088 100644 --- a/fern/openapi.yml +++ b/fern/openapi.yml @@ -195,6 +195,23 @@ components: application/json: schema: $ref: "#/components/schemas/Error" + securitySchemes: + bearerAuth: + type: http + scheme: bearer + description: > + Needed only by a caller that is not on the node itself, and only when + the node's operator has enabled authenticated LAN access by configuring + API keys. Loopback callers send no credential. Send a configured key as + `Authorization: Bearer `; the proxy strips it before forwarding. + apiKeyAuth: + type: apiKey + in: header + name: X-Api-Key + description: > + Alternative to bearerAuth for clients built on the Anthropic SDK + convention. Same scope: non-loopback callers, when the operator has + enabled LAN access. schemas: ChatRole: type: string diff --git a/services/nvpair-proxy/ingress.go b/services/nvpair-proxy/ingress.go index ba8336f5..98e79ccb 100644 --- a/services/nvpair-proxy/ingress.go +++ b/services/nvpair-proxy/ingress.go @@ -12,6 +12,9 @@ import ( "net/http/httputil" "net/url" "strconv" + + "nvpair-shared/cors" + "nvpair-shared/ingressauth" ) const engineIdentityProbeHeader = "X-NVPAIR-Engine-Identity-Probe" @@ -71,18 +74,62 @@ func (f *facade) localBackendTarget() (*url.URL, bool) { return &url.URL{Scheme: "http", Host: net.JoinHostPort(host, strconv.Itoa(b.Port))}, true } -// handlePlain is the plaintext personality: it accepts requests only from -// loopback and hands them to the full local router (handleHTTP). A non-loopback -// caller — any LAN peer — is refused; peers must use the mTLS ingress. This is -// what closes the former open-relay exposure (the listener still binds all -// interfaces for the TLS personality, but plaintext is loopback-only). +// handlePlain is the plaintext personality: it accepts requests from loopback +// and hands them to the full local router (handleHTTP). A non-loopback caller — +// any LAN peer — is refused unless the operator has enabled the API-key gate +// (nvpair-shared/ingressauth) and the caller presents a configured key, in +// which case it is routed exactly like a loopback client. Cluster peers still +// use the mTLS ingress, and loopback is never asked for a key. This is what +// closes the former open-relay exposure: the listener binds all interfaces for +// the TLS personality, but plaintext is loopback-only unless authenticated. func (f *facade) handlePlain(w http.ResponseWriter, r *http.Request) { if !isLoopbackRemote(r.RemoteAddr) { - slog.Warn("rejected non-loopback plaintext request; cluster peers must use mTLS", - "remote", r.RemoteAddr, "method", r.Method, "path", r.URL.Path) - writeIngressError(w, http.StatusForbidden, "loopback-only", - "plaintext requests are accepted only from loopback; cluster peers must use the mTLS ingress") - return + // One Authorize call refreshes the key file and judges the request from + // that single view, so enablement and the key set cannot change between + // "is the gate on?" and "is this key good?". + var d ingressauth.Decision + if p := f.host.lanAuth; p != nil { + d = p.Authorize(r) + } + // A source outside the operator's allowlist gets nothing — not even a + // CORS-answered preflight — so the allowlist means what it says for + // OPTIONS too. + if d.Enabled && d.Code == ingressauth.CodeSourceNotAllowed { + slog.Warn("rejected non-loopback plaintext request", "remote", r.RemoteAddr, + "method", r.Method, "path", r.URL.Path, "code", d.Code) + writeIngressError(w, d.Status, d.Code, d.Message) + return + } + // Answer a non-loopback preflight ahead of the credential check. It + // grants no access on its own; the request that follows still receives + // the real 401/403, and a browser sends no Authorization on a preflight + // anyway. A loopback preflight continues into handleHTTP so an engine's + // exact origin and credentials policy can be preserved (see proxy.go). + if d.Enabled && cors.IsPreflight(r) { + cors.WritePreflight(w, r) + return + } + if !d.Enabled { + slog.Warn("rejected non-loopback plaintext request; cluster peers must use mTLS", + "remote", r.RemoteAddr, "method", r.Method, "path", r.URL.Path) + writeIngressError(w, http.StatusForbidden, "loopback-only", + "plaintext requests are accepted only from loopback; cluster peers must use the mTLS ingress") + return + } + if !d.Allowed { + slog.Warn("rejected non-loopback plaintext request", "remote", r.RemoteAddr, + "method", r.Method, "path", r.URL.Path, "code", d.Code, "key_fp", d.KeyFingerprint) + if d.Challenge != "" { + w.Header().Set("WWW-Authenticate", d.Challenge) + } + writeIngressError(w, d.Status, d.Code, d.Message) + return + } + // The key is the proxy's credential, not the engine's: never forward it. + f.host.lanAuth.StripCredential(r.Header) + // At Info, not Debug: a production log must show who used a key. + slog.Info("authenticated non-loopback plaintext request", "remote", r.RemoteAddr, + "method", r.Method, "path", r.URL.Path, "key_fp", d.KeyFingerprint) } // Engine-manager marks its private identity/action requests so this // compatibility facade can never be mistaken for the local Ollama backend. diff --git a/services/nvpair-proxy/ingress_auth_test.go b/services/nvpair-proxy/ingress_auth_test.go new file mode 100644 index 00000000..71122d8f --- /dev/null +++ b/services/nvpair-proxy/ingress_auth_test.go @@ -0,0 +1,285 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/netip" + "strings" + "sync/atomic" + "testing" + + "nvpair-shared/ingressauth" +) + +const ( + lanKey = "0123456789abcdef0123456789abcdef" + lanRemote = "192.0.2.50:40000" +) + +// authEngine is an httptest engine that records the headers of the last request +// it served, so a test can prove what the proxy did and did not forward. +func authEngine(t *testing.T) (*httptest.Server, *atomic.Pointer[http.Header]) { + t.Helper() + var seen atomic.Pointer[http.Header] + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + h := r.Header.Clone() + seen.Store(&h) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ok":true}`)) + })) + t.Cleanup(srv.Close) + return srv, &seen +} + +// lanProxy is a proxy with one routable engine and the API-key gate enabled for +// lanKey, optionally restricted to cidrs. The gate lives on the Proxy so every +// facade shares it; tests reach the facade through soleFacade. +func lanProxy(t *testing.T, cidrs ...netip.Prefix) (*facade, *atomic.Pointer[http.Header]) { + t.Helper() + engine, seen := authEngine(t) + disc := NewDiscovery() + disc.AddManual(nodeForModel(t, "engine", engine.URL, "llama")) + p := testProxy(anyProfile(t), disc, 11435) + p.lanAuth = ingressauth.New([]string{lanKey}, cidrs) + return p.soleFacade(), seen +} + +func inferenceRequest(remote string, hdr ...string) *http.Request { + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", + strings.NewReader(`{"model":"llama","messages":[{"role":"user","content":"hi"}]}`)) + req.Header.Set("Content-Type", "application/json") + req.RemoteAddr = remote + for i := 0; i+1 < len(hdr); i += 2 { + req.Header.Set(hdr[i], hdr[i+1]) + } + return req +} + +func ingressCode(t *testing.T, rec *httptest.ResponseRecorder) string { + t.Helper() + var body struct { + Code string `json:"code"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("ingress error body %q is not JSON: %v", rec.Body.String(), err) + } + return body.Code +} + +// TestHandlePlainAuthenticatedNonLoopbackIsRouted: with the gate enabled, a LAN +// caller presenting a configured key is routed through the full local router +// like a loopback client, and the key is stripped before the engine sees it. +func TestHandlePlainAuthenticatedNonLoopbackIsRouted(t *testing.T) { + f, seen := lanProxy(t) + rec := httptest.NewRecorder() + f.handlePlain(rec, inferenceRequest(lanRemote, "Authorization", "Bearer "+lanKey)) + + if rec.Code != http.StatusOK { + t.Fatalf("authenticated LAN status = %d, want 200; body %s", rec.Code, rec.Body.String()) + } + h := seen.Load() + if h == nil { + t.Fatal("engine never received the routed request") + } + if got := h.Get("Authorization"); got != "" { + t.Errorf("engine received Authorization = %q, want the proxy's key stripped", got) + } + if got := h.Get("X-Api-Key"); got != "" { + t.Errorf("engine received X-Api-Key = %q, want stripped", got) + } +} + +func TestHandlePlainXApiKeyAccepted(t *testing.T) { + f, seen := lanProxy(t) + rec := httptest.NewRecorder() + f.handlePlain(rec, inferenceRequest(lanRemote, "X-Api-Key", lanKey)) + + if rec.Code != http.StatusOK { + t.Fatalf("X-Api-Key status = %d, want 200; body %s", rec.Code, rec.Body.String()) + } + if h := seen.Load(); h == nil || h.Get("X-Api-Key") != "" { + t.Fatalf("engine headers = %v, want the request forwarded without X-Api-Key", h) + } +} + +// TestHandlePlainNonLoopbackWithoutKeyIs401: an enabled gate turns the LAN +// refusal from 403 loopback-only into 401 with a challenge, still carrying CORS +// so a browser can read it, and nothing is forwarded. +func TestHandlePlainNonLoopbackWithoutKeyIs401(t *testing.T) { + f, seen := lanProxy(t) + rec := httptest.NewRecorder() + f.handlePlain(rec, inferenceRequest(lanRemote)) + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("no-key LAN status = %d, want 401", rec.Code) + } + if got := ingressCode(t, rec); got != ingressauth.CodeUnauthorized { + t.Errorf("code = %q, want %q", got, ingressauth.CodeUnauthorized) + } + if got := rec.Header().Get("WWW-Authenticate"); got != `Bearer realm="nvpair-proxy"` { + t.Errorf("WWW-Authenticate = %q, want a Bearer challenge", got) + } + if strings.Contains(rec.Body.String(), lanKey) { + t.Error("refusal body echoes a key") + } + if seen.Load() != nil { + t.Fatal("an unauthenticated LAN request reached the engine") + } +} + +func TestHandlePlainNonLoopbackWrongKeyIs401(t *testing.T) { + f, seen := lanProxy(t) + rec := httptest.NewRecorder() + f.handlePlain(rec, inferenceRequest(lanRemote, "Authorization", "Bearer "+lanKey+"-not")) + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("wrong-key LAN status = %d, want 401", rec.Code) + } + if got := ingressCode(t, rec); got != ingressauth.CodeUnauthorized { + t.Errorf("code = %q, want %q", got, ingressauth.CodeUnauthorized) + } + // RFC 6750 §3.1: a credential that was examined and rejected is told so. + if got := rec.Header().Get("WWW-Authenticate"); !strings.Contains(got, `error="invalid_token"`) { + t.Errorf("WWW-Authenticate = %q, want error=\"invalid_token\" on a rejected key", got) + } + if seen.Load() != nil { + t.Fatal("a request with a wrong key reached the engine") + } +} + +// TestHandlePlainLoopbackNeedsNoKeyWhenEnabled: enabling the gate changes +// nothing for loopback — no key is required, and a client's own Authorization +// header (an SDK placeholder, say) is forwarded untouched as it is today. +func TestHandlePlainLoopbackNeedsNoKeyWhenEnabled(t *testing.T) { + f, seen := lanProxy(t) + rec := httptest.NewRecorder() + f.handlePlain(rec, inferenceRequest("127.0.0.1:40000", "Authorization", "Bearer lm-studio")) + + if rec.Code != http.StatusOK { + t.Fatalf("loopback status = %d, want 200; body %s", rec.Code, rec.Body.String()) + } + h := seen.Load() + if h == nil { + t.Fatal("engine never received the loopback request") + } + if got := h.Get("Authorization"); got != "Bearer lm-studio" { + t.Errorf("loopback Authorization forwarded as %q, want it untouched", got) + } +} + +// TestHandlePlainOutsideAllowedCIDRIs403: with an allowlist configured, a +// caller outside it is refused before its key is examined — a valid key does not +// help, and no Bearer challenge is issued. +func TestHandlePlainOutsideAllowedCIDRIs403(t *testing.T) { + f, seen := lanProxy(t, netip.MustParsePrefix("10.0.0.0/8")) + rec := httptest.NewRecorder() + f.handlePlain(rec, inferenceRequest(lanRemote, "Authorization", "Bearer "+lanKey)) + + if rec.Code != http.StatusForbidden { + t.Fatalf("out-of-allowlist status = %d, want 403", rec.Code) + } + if got := ingressCode(t, rec); got != ingressauth.CodeSourceNotAllowed { + t.Errorf("code = %q, want %q", got, ingressauth.CodeSourceNotAllowed) + } + if got := rec.Header().Get("WWW-Authenticate"); got != "" { + t.Errorf("WWW-Authenticate = %q, want none on a source refusal", got) + } + if seen.Load() != nil { + t.Fatal("a request from outside the allowlist reached the engine") + } +} + +func TestHandlePlainInsideAllowedCIDRIsRouted(t *testing.T) { + f, seen := lanProxy(t, netip.MustParsePrefix("192.0.2.0/24")) + rec := httptest.NewRecorder() + f.handlePlain(rec, inferenceRequest(lanRemote, "Authorization", "Bearer "+lanKey)) + + if rec.Code != http.StatusOK { + t.Fatalf("in-allowlist status = %d, want 200; body %s", rec.Code, rec.Body.String()) + } + if seen.Load() == nil { + t.Fatal("engine never received the request") + } +} + +// TestHandlePlainPreflightStillAnsweredWhenEnabled: a browser sends no +// Authorization on a preflight, so the 204 must keep preceding the credential +// check once the gate is on. +func TestHandlePlainPreflightStillAnsweredWhenEnabled(t *testing.T) { + f, seen := lanProxy(t) + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) + req.RemoteAddr = lanRemote + req.Header.Set("Origin", "http://app.test") + req.Header.Set("Access-Control-Request-Method", "POST") + req.Header.Set("Access-Control-Request-Headers", "Authorization") + f.handlePlain(rec, req) + + if rec.Code != http.StatusNoContent { + t.Fatalf("LAN preflight status = %d, want 204", rec.Code) + } + if seen.Load() != nil { + t.Fatal("a preflight reached the engine") + } +} + +// TestHandlePlainGateWithoutKeysKeepsLoopbackOnly: a gate that exists but has no +// keys is the default: the LAN refusal stays the original 403 loopback-only. +func TestHandlePlainGateWithoutKeysKeepsLoopbackOnly(t *testing.T) { + f, seen := lanProxy(t) + f.host.lanAuth = ingressauth.New(nil, nil) + rec := httptest.NewRecorder() + f.handlePlain(rec, inferenceRequest(lanRemote, "Authorization", "Bearer "+lanKey)) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403", rec.Code) + } + if got := ingressCode(t, rec); got != "loopback-only" { + t.Errorf("code = %q, want loopback-only", got) + } + if seen.Load() != nil { + t.Fatal("a LAN request reached the engine with no keys configured") + } +} + +// TestHandlePlainPreflightOutsideAllowedCIDRIs403: the allowlist applies to a +// preflight too. A source the operator excluded gets no 204 that would let a +// browser proceed to the request that follows. +func TestHandlePlainPreflightOutsideAllowedCIDRIs403(t *testing.T) { + f, seen := lanProxy(t, netip.MustParsePrefix("10.0.0.0/8")) + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) + req.RemoteAddr = lanRemote + req.Header.Set("Origin", "http://app.test") + req.Header.Set("Access-Control-Request-Method", "POST") + f.handlePlain(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("out-of-allowlist preflight status = %d, want 403", rec.Code) + } + if got := ingressCode(t, rec); got != ingressauth.CodeSourceNotAllowed { + t.Errorf("code = %q, want %q", got, ingressauth.CodeSourceNotAllowed) + } + if seen.Load() != nil { + t.Fatal("a preflight reached the engine") + } +} + +// TestHandlePlainPreflightInsideAllowedCIDRIs204: inside the allowlist the +// preflight is still answered without a credential, as browsers require. +func TestHandlePlainPreflightInsideAllowedCIDRIs204(t *testing.T) { + f, _ := lanProxy(t, netip.MustParsePrefix("192.0.2.0/24")) + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) + req.RemoteAddr = lanRemote + req.Header.Set("Origin", "http://app.test") + req.Header.Set("Access-Control-Request-Method", "POST") + f.handlePlain(rec, req) + if rec.Code != http.StatusNoContent { + t.Fatalf("in-allowlist preflight status = %d, want 204", rec.Code) + } +} diff --git a/services/nvpair-proxy/main.go b/services/nvpair-proxy/main.go index ebfa902d..2bc6299d 100644 --- a/services/nvpair-proxy/main.go +++ b/services/nvpair-proxy/main.go @@ -17,6 +17,7 @@ import ( "nvpair-shared/applog" "nvpair-shared/clustertrust" "nvpair-shared/engines" + "nvpair-shared/ingressauth" ) func main() { @@ -81,6 +82,10 @@ func main() { // presence: a left/removed node keeps its keypair by design, and would // otherwise keep logging cluster_ingress with no cluster peers to serve. proxy.mesh = clustertrust.Open(*clusterDir) + // The opt-in API-key gate for non-loopback plaintext callers. Configured + // from the environment (or the default key file); with nothing configured + // it stays disabled and plaintext remains loopback-only. + proxy.lanAuth = ingressauth.FromEnv() go proxy.mesh.Watch(ctx, func(clustered bool) { slog.Info("cluster inference ingress switched personality", "cluster_ingress", clustered) diff --git a/services/nvpair-proxy/proxy.go b/services/nvpair-proxy/proxy.go index 929c3d67..ee19b59d 100644 --- a/services/nvpair-proxy/proxy.go +++ b/services/nvpair-proxy/proxy.go @@ -31,6 +31,7 @@ import ( "nvpair-shared/clustertrust" "nvpair-shared/cors" "nvpair-shared/engines" + "nvpair-shared/ingressauth" "nvpair-shared/netmon" "nvpair-shared/netpick" "nvpair-shared/nodeactivity" @@ -362,6 +363,13 @@ type Proxy struct { // only loopback-plaintext local routing. Read-only after startup. mesh *clustertrust.Mesh + // lanAuth is the opt-in API-key gate for non-loopback plaintext callers + // (nvpair-shared/ingressauth). nil or disabled: plaintext is loopback-only. + // Set once at startup; the gate re-reads its own key file on demand. It + // lives on Proxy, not on a facade: the key is one credential for the node, + // so every enabled engine's facade enforces the same gate identically. + lanAuth *ingressauth.Gate + // activity coalesces the liveness reports raised when a peer's engine streams // response bytes back through us (see reportActivity). activity *nodeactivity.Reporter diff --git a/services/shared/ingressauth/ingressauth.go b/services/shared/ingressauth/ingressauth.go index f6ad20a6..76a97f07 100644 --- a/services/shared/ingressauth/ingressauth.go +++ b/services/shared/ingressauth/ingressauth.go @@ -27,6 +27,7 @@ package ingressauth import ( "bufio" + "bytes" "crypto/sha256" "crypto/subtle" "encoding/hex" @@ -42,7 +43,6 @@ import ( "strings" "sync" "time" - "unicode" "nvpair-shared/appdir" ) @@ -62,10 +62,29 @@ const ( DefaultKeyFileName = "proxy-api-keys" // MinKeyLength is the shortest key the gate accepts. A 32-character key - // drawn from hex already carries 128 bits, which puts online guessing out - // of reach without a lockout mechanism. + // drawn at random from the allowed alphabet carries well over 128 bits, + // which puts online guessing out of reach without a lockout mechanism. A + // 32-character passphrase does not; the documentation says to generate + // keys randomly. MinKeyLength = 32 + // MaxKeyLength bounds a key, configured or presented. A credential a + // client sends is hashed before it is compared, so without a ceiling a + // caller could make the proxy digest a megabyte of header per request; + // anything longer than this is not a key and is not examined. + MaxKeyLength = 512 + + // maxKeyFileBytes bounds how much of a key file is read. A key file holds + // a handful of short lines; anything larger is not a key file. + maxKeyFileBytes = 64 << 10 + + // defaultRecheckEvery is how long a FromEnv gate trusts its last look at + // the key file before opening it again. Without a floor, a node that never + // opted in would still pay an open() for every unauthenticated LAN request, + // which is a remotely triggered cost it did not have before. One second + // keeps rotation and revocation effectively immediate. + defaultRecheckEvery = time.Second + // CodeUnauthorized is the ingress error code for a missing or wrong key. CodeUnauthorized = "unauthorized" // CodeSourceNotAllowed is the ingress error code for a caller outside the @@ -77,33 +96,40 @@ const ( bearerScheme = "bearer" noCredential = "none" + // challengeMissing and challengeInvalid are the WWW-Authenticate values for + // a 401, per RFC 6750 §3: a request with no credential gets the bare + // challenge; one whose credential was examined and rejected also carries + // error="invalid_token". + challengeMissing = `Bearer realm="nvpair-proxy"` + challengeInvalid = `Bearer realm="nvpair-proxy", error="invalid_token"` + unauthorizedMessage = "a valid API key is required for non-loopback requests; " + "send it as Authorization: Bearer or X-Api-Key: " ) -// Decision is the gate's verdict on one request. When Allowed is false, Status, -// Code, and Message are what the proxy should answer with, in the same shape as -// its other ingress rejections. KeyFingerprint identifies the presented key for -// the log without revealing it; it is "none" when no credential was sent. +// Decision is the gate's verdict on one request. Enabled reports whether any +// key is configured at the moment of the call; when it is false the proxy +// applies its loopback-only refusal and the other fields are unset. When +// Enabled is true and Allowed is false, Status, Code, and Message are what the +// proxy should answer with, in the same shape as its other ingress rejections, +// and Challenge, when non-empty, is the WWW-Authenticate value to send. +// KeyFingerprint identifies the presented key for the log without revealing it; +// it is "none" when no credential was examined. type Decision struct { + Enabled bool Allowed bool Status int Code string Message string + Challenge string KeyFingerprint string } -type digest = [sha256.Size]byte +// digest is a SHA-256 of a key. A distinct type, so a raw byte array cannot be +// mistaken for one. +type digest [sha256.Size]byte -// fileStamp is the part of a key file's metadata that decides whether it must -// be re-read. Mode is included because fixing permissions with chmod changes -// neither size nor modification time, yet must take effect. -type fileStamp struct { - size int64 - modTime time.Time - mode fs.FileMode - exists bool -} +func digestOf(key string) digest { return digest(sha256.Sum256([]byte(key))) } // Gate holds the configured credentials and allowlist. Its zero value is a // disabled gate; construct one with FromEnv or New. @@ -114,25 +140,36 @@ type Gate struct { inline []digest // broken records an unrecoverable configuration error in the environment // (a malformed inline key or CIDR). The gate then stays disabled for the - // life of the process, regardless of the key file. + // life of the process, regardless of the key file. The proxy keeps serving + // loopback clients; taking it down for an optional setting would punish + // the desktop application for an operator's typo. broken bool - // filePath, when non-empty, is re-checked on every Enabled call so keys can - // be rotated without restarting the proxy. explicitFile records that the - // operator named the path, so its absence is worth reporting. + // filePath, when non-empty, is re-read (at most once per recheckEvery) on + // Authorize so keys can be rotated without restarting the proxy. The + // cached keys are reused while the file's content hash is unchanged. + // explicitFile records that the operator named the path, so its absence + // is worth reporting. fileErr is the last error logged for the file, so a + // persisting problem is reported once rather than per request and a new + // problem is reported when it appears. filePath string explicitFile bool fileKeys []digest - fileStamp fileStamp - fileChecked bool + fileHash digest + fileLoaded bool + fileErr string + recheckEvery time.Duration + lastCheck time.Time cidrs []netip.Prefix // announced* remember the last state written to the log, so a change is - // reported once rather than on every request. + // reported once rather than on every request. The file hash is part of the + // state so a rotation that keeps the key count is still reported. announcedOnce bool announcedEnabled bool announcedKeys int + announcedHash digest } // New builds a gate from literal keys and prefixes, for tests and callers that @@ -145,7 +182,7 @@ func New(keys []string, cidrs []netip.Prefix) *Gate { if err := validateKey(k); err != nil { panic("ingressauth.New: " + err.Error()) } - g.inline = append(g.inline, sha256.Sum256([]byte(k))) + g.inline = append(g.inline, digestOf(k)) } g.mu.Lock() g.announceLocked() @@ -157,7 +194,7 @@ func New(keys []string, cidrs []netip.Prefix) *Gate { // configuration error is logged and yields a gate that stays disabled, which // leaves the proxy in its loopback-only default. func FromEnv() *Gate { - g := &Gate{} + g := &Gate{recheckEvery: defaultRecheckEvery} if raw := os.Getenv(EnvKeys); strings.TrimSpace(raw) != "" { for i, k := range strings.Split(raw, ",") { @@ -171,7 +208,7 @@ func FromEnv() *Gate { g.broken = true break } - g.inline = append(g.inline, sha256.Sum256([]byte(k))) + g.inline = append(g.inline, digestOf(k)) } } @@ -209,8 +246,9 @@ func FromEnv() *Gate { } // Enabled reports whether at least one API key is configured, re-reading the -// key file first if it changed. The proxy consults this per non-loopback -// request, so adding, rotating, or removing keys needs no restart. +// key file first if it is due. The proxy does not call this per request — +// Authorize reports the same thing in its Decision from a single refresh — but +// it is the natural question for startup logging and tests. func (g *Gate) Enabled() bool { g.mu.Lock() defer g.mu.Unlock() @@ -223,44 +261,53 @@ func (g *Gate) enabledLocked() bool { return !g.broken && len(g.inline)+len(g.fileKeys) > 0 } -// Authorize decides whether a non-loopback plaintext request may proceed. The -// allowlist is checked before the credential, so a caller outside it learns -// nothing about whether its key is valid. Authorize does not write to the -// response; the proxy does, in its own error format. +// Authorize decides whether a non-loopback plaintext request may proceed. It +// refreshes the key file once and answers from that single view, so the +// enabled/disabled state and the key set a request is judged against cannot +// change between two calls. The allowlist is checked before the credential, +// so a caller outside it learns nothing about whether its key is valid — and +// the proxy applies that source decision even to a preflight, which needs no +// credential. Authorize does not write to the response; the proxy does, in +// its own error format. func (g *Gate) Authorize(r *http.Request) Decision { g.mu.Lock() g.refreshLocked() + g.announceLocked() + enabled := g.enabledLocked() cidrs := g.cidrs digests := make([]digest, 0, len(g.inline)+len(g.fileKeys)) digests = append(digests, g.inline...) digests = append(digests, g.fileKeys...) - enabled := g.enabledLocked() g.mu.Unlock() if !enabled { - // The proxy only asks an enabled gate; answer conservatively anyway. - return Decision{Status: http.StatusForbidden, Code: CodeSourceNotAllowed, - Message: "authenticated LAN ingress is not enabled", KeyFingerprint: noCredential} + return Decision{} } if len(cidrs) > 0 { ip, ok := remoteAddr(r) if !ok || !anyPrefixContains(cidrs, ip) { - return Decision{Status: http.StatusForbidden, Code: CodeSourceNotAllowed, + return Decision{Enabled: true, Status: http.StatusForbidden, Code: CodeSourceNotAllowed, Message: "the caller's address is outside " + EnvAllowedCIDRs, KeyFingerprint: noCredential} } } - cred, ok := credentialFrom(r) - if !ok { - return Decision{Status: http.StatusUnauthorized, Code: CodeUnauthorized, - Message: unauthorizedMessage, KeyFingerprint: noCredential} + creds := credentialsFrom(r) + if len(creds) == 0 { + return Decision{Enabled: true, Status: http.StatusUnauthorized, Code: CodeUnauthorized, + Message: unauthorizedMessage, Challenge: challengeMissing, KeyFingerprint: noCredential} } - if !matchesAny(digests, sha256.Sum256([]byte(cred))) { - return Decision{Status: http.StatusUnauthorized, Code: CodeUnauthorized, - Message: unauthorizedMessage, KeyFingerprint: Fingerprint(cred)} + // Either presented credential may match. An SDK that always sends a + // placeholder Bearer token alongside the real X-Api-Key must not be locked + // out by header precedence; both headers are the caller's to set, so + // checking both costs nothing in security. + for _, cred := range creds { + if matchesAny(digests, digestOf(cred)) { + return Decision{Enabled: true, Allowed: true, KeyFingerprint: fingerprint(cred)} + } } - return Decision{Allowed: true, Status: http.StatusOK, KeyFingerprint: Fingerprint(cred)} + return Decision{Enabled: true, Status: http.StatusUnauthorized, Code: CodeUnauthorized, + Message: unauthorizedMessage, Challenge: challengeInvalid, KeyFingerprint: fingerprint(creds[0])} } // StripCredential removes the presented key from a request the gate admitted, @@ -270,17 +317,22 @@ func (g *Gate) StripCredential(h http.Header) { h.Del(headerAPIKey) } -// Fingerprint returns the first eight hex characters of a key's SHA-256 digest: +// fingerprint returns the first eight hex characters of a key's SHA-256 digest: // enough for an operator to tell repeated rejections of one misconfigured -// client apart from a scan, without the log ever holding the key. -func Fingerprint(key string) string { +// client apart from a scan, without the log ever holding the key. It is +// deliberately unsalted so an operator can compute it from the key they meant +// to configure and confirm which client is misconfigured; the price is that +// the log holds 32 bits of a truncated hash of a client's key, which is one +// more reason keys must be random rather than memorable. +func fingerprint(key string) string { sum := sha256.Sum256([]byte(key)) return hex.EncodeToString(sum[:4]) } -// matchesAny compares the presented digest against every configured digest in -// constant time and without an early exit, so neither the key length nor the -// position of a match is observable through timing. +// matchesAny compares the presented digest against every configured digest +// with a constant-time comparison and no early exit, so neither the key +// length nor the position of a match is observable through timing. (The +// number of configured keys is not a secret.) func matchesAny(configured []digest, presented digest) bool { match := 0 for i := range configured { @@ -289,22 +341,25 @@ func matchesAny(configured []digest, presented digest) bool { return match == 1 } -// credentialFrom extracts the client's key: a Bearer token first, then the -// X-Api-Key header. A query parameter is deliberately not accepted, because -// URLs end up in access logs and browser histories. -func credentialFrom(r *http.Request) (string, bool) { +// credentialsFrom extracts the client's presented keys: a Bearer token and an +// X-Api-Key header, in that order, whichever are present and no longer than +// MaxKeyLength (an over-long value cannot be a key and is not hashed). A query +// parameter is deliberately not accepted, because URLs end up in access logs +// and browser histories. +func credentialsFrom(r *http.Request) []string { + var creds []string if auth := strings.TrimSpace(r.Header.Get(headerAuthorization)); auth != "" { scheme, token, found := strings.Cut(auth, " ") if found && strings.EqualFold(scheme, bearerScheme) { - if token = strings.TrimSpace(token); token != "" { - return token, true + if token = strings.TrimSpace(token); token != "" && len(token) <= MaxKeyLength { + creds = append(creds, token) } } } - if key := strings.TrimSpace(r.Header.Get(headerAPIKey)); key != "" { - return key, true + if key := strings.TrimSpace(r.Header.Get(headerAPIKey)); key != "" && len(key) <= MaxKeyLength { + creds = append(creds, key) } - return "", false + return creds } // remoteAddr parses the transport-level peer address. Forwarding headers are @@ -327,56 +382,79 @@ func anyPrefixContains(prefixes []netip.Prefix, ip netip.Addr) bool { return false } -// refreshLocked re-reads the key file when its metadata changed since the last -// look. Caller holds g.mu. +// refreshLocked re-reads the key file, at most once per recheckEvery, and +// swaps the cached keys when its content changed. The file is read rather +// than stat-compared: a key file is a few short lines, and a stamp of size +// and modification time cannot see a same-length rewrite within the +// filesystem's timestamp granularity — exactly the rotation a compromised key +// needs. Caller holds g.mu. func (g *Gate) refreshLocked() { if g.filePath == "" || g.broken { return } - info, err := os.Stat(g.filePath) - var stamp fileStamp + now := time.Now() + if !g.lastCheck.IsZero() && now.Sub(g.lastCheck) < g.recheckEvery { + return + } + g.lastCheck = now + + content, err := readKeyFile(g.filePath) switch { case err == nil: - stamp = fileStamp{size: info.Size(), modTime: info.ModTime(), mode: info.Mode(), exists: true} + g.fileErr = "" + hash := digest(sha256.Sum256(content)) + if g.fileLoaded && hash == g.fileHash { + return + } + keys, perr := parseKeys(bytes.NewReader(content)) + if perr != nil { + g.dropFileKeysLocked() + g.logFileErrorLocked("key file ignored; no file keys are in effect", perr) + return + } + g.fileKeys, g.fileHash, g.fileLoaded = keys, hash, true case errors.Is(err, fs.ErrNotExist): - stamp = fileStamp{} - default: - // A stat failure other than absence (a parent directory's permissions, - // an I/O error) counts as absence for this request and is re-examined - // on the next; report it when it is news. - if !g.fileChecked || g.fileStamp.exists { - slog.Error("authenticated LAN ingress: cannot stat key file; no file keys are in effect", - "path", g.filePath, "err", err) + g.dropFileKeysLocked() + if g.explicitFile { + g.logFileErrorLocked("key file does not exist; no file keys are in effect", err) } - stamp = fileStamp{} + default: + g.dropFileKeysLocked() + g.logFileErrorLocked("key file ignored; no file keys are in effect", err) } - if g.fileChecked && stamp == g.fileStamp { +} + +func (g *Gate) dropFileKeysLocked() { + g.fileKeys, g.fileHash, g.fileLoaded = nil, digest{}, false +} + +// logFileErrorLocked reports a key-file problem once per distinct error, so a +// persisting misconfiguration does not write a line per request while a new +// one is still reported the moment it appears. +func (g *Gate) logFileErrorLocked(msg string, err error) { + if err.Error() == g.fileErr { return } - g.fileChecked = true - g.fileStamp = stamp - g.fileKeys = nil + g.fileErr = err.Error() + slog.Error("authenticated LAN ingress: "+msg, "path", g.filePath, "err", err) +} - if !stamp.exists { - if g.explicitFile { - slog.Error("authenticated LAN ingress: key file does not exist; no file keys are in effect", - "env", EnvKeysFile, "path", g.filePath) - } - return +// readKeyFile opens the key file and checks the opened handle — not a separate +// stat — before reading, so the permissions, type, and owner it validates +// belong to the file it reads. On Unix-like systems the file must belong to +// the proxy's user (or root) and must not be readable or writable by group or +// others; on Windows the mode bits carry no such meaning and the check is +// skipped, leaving protection to the data directory's ACL. +func readKeyFile(path string) ([]byte, error) { + f, err := os.Open(path) + if err != nil { + return nil, err } - keys, err := loadKeyFile(g.filePath, info) + defer f.Close() + info, err := f.Stat() if err != nil { - slog.Error("authenticated LAN ingress: key file ignored; no file keys are in effect", - "path", g.filePath, "err", err) - return + return nil, err } - g.fileKeys = keys -} - -// loadKeyFile reads and validates a key file. On Unix-like systems the file must -// not be readable or writable by group or others; on Windows the mode bits -// carry no such meaning and the check is skipped. -func loadKeyFile(path string, info fs.FileInfo) ([]digest, error) { if !info.Mode().IsRegular() { return nil, errors.New("not a regular file") } @@ -385,12 +463,17 @@ func loadKeyFile(path string, info fs.FileInfo) ([]digest, error) { return nil, fmt.Errorf("permissions %04o allow other users to read it; chmod 600", perm) } } - f, err := os.Open(path) + if err := ownedByProcessUser(info); err != nil { + return nil, err + } + content, err := io.ReadAll(io.LimitReader(f, maxKeyFileBytes+1)) if err != nil { return nil, err } - defer f.Close() - return parseKeys(f) + if len(content) > maxKeyFileBytes { + return nil, fmt.Errorf("larger than %d bytes; not a key file", maxKeyFileBytes) + } + return content, nil } // parseKeys reads one key per line. Blank lines and lines starting with '#' are @@ -410,7 +493,7 @@ func parseKeys(r io.Reader) ([]digest, error) { if err := validateKey(entry); err != nil { return nil, fmt.Errorf("line %d: %w", line, err) } - keys = append(keys, sha256.Sum256([]byte(entry))) + keys = append(keys, digestOf(entry)) } if err := sc.Err(); err != nil { return nil, err @@ -422,40 +505,58 @@ func parseKeys(r io.Reader) ([]digest, error) { } // validateKey enforces the shape a key must have to be usable at all: long -// enough to resist guessing, and printable ASCII with no whitespace so it -// survives an HTTP header unchanged. +// enough to resist guessing, and drawn from the RFC 6750 b64token alphabet +// (letters, digits, and - . _ ~ + / =) so it survives an Authorization header +// unchanged through any conformant intermediary. Hex and base64 output both +// qualify. func validateKey(key string) error { if len(key) < MinKeyLength { return fmt.Errorf("key is %d characters; at least %d are required", len(key), MinKeyLength) } - for _, c := range key { - if c > unicode.MaxASCII || c <= ' ' || c == 0x7f { - return errors.New("key must be printable ASCII with no whitespace") + if len(key) > MaxKeyLength { + return fmt.Errorf("key is %d characters; at most %d are allowed", len(key), MaxKeyLength) + } + for i := 0; i < len(key); i++ { + if !isTokenByte(key[i]) { + return errors.New("key must use only letters, digits, and - . _ ~ + / =") } } return nil } -// announceLocked logs a change in the gate's state — enabled with N keys, or -// back to disabled — once per change. Enabling is logged at Warn: it widens the -// proxy's exposure and an operator reading the log should see it plainly. -// Caller holds g.mu. +func isTokenByte(c byte) bool { + switch { + case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c >= '0' && c <= '9': + return true + } + return strings.IndexByte("-._~+/=", c) >= 0 +} + +// announceLocked logs a change in the gate's state — enabled with N keys, a +// rotation of the key file, or back to disabled — once per change. Enabling +// is logged at Warn: it widens the proxy's exposure and an operator reading +// the log should see it plainly. Caller holds g.mu. func (g *Gate) announceLocked() { enabled := g.enabledLocked() n := len(g.inline) + len(g.fileKeys) - if g.announcedOnce && enabled == g.announcedEnabled && n == g.announcedKeys { + if g.announcedOnce && enabled == g.announcedEnabled && n == g.announcedKeys && g.fileHash == g.announcedHash { return } - g.announcedOnce, g.announcedEnabled, g.announcedKeys = true, enabled, n - if enabled { + rotated := g.announcedOnce && enabled && g.announcedEnabled && g.fileHash != g.announcedHash + g.announcedOnce, g.announcedEnabled, g.announcedKeys, g.announcedHash = true, enabled, n, g.fileHash + switch { + case rotated: + slog.Warn("authenticated LAN ingress: key file changed; the configured key set was replaced", + "keys", n, "key_file", g.filePath) + case enabled: cidrs := make([]string, 0, len(g.cidrs)) for _, p := range g.cidrs { cidrs = append(cidrs, p.String()) } slog.Warn("authenticated LAN ingress ENABLED: a non-loopback plaintext caller presenting a configured API key is routed", "keys", n, "key_file", g.filePath, "allowed_cidrs", cidrs) - return + default: + slog.Info("authenticated LAN ingress disabled; plaintext requests are accepted from loopback only", + "key_file", g.filePath) } - slog.Info("authenticated LAN ingress disabled; plaintext requests are accepted from loopback only", - "key_file", g.filePath) } diff --git a/services/shared/ingressauth/ingressauth_test.go b/services/shared/ingressauth/ingressauth_test.go index c7e2a70d..2d0b6e86 100644 --- a/services/shared/ingressauth/ingressauth_test.go +++ b/services/shared/ingressauth/ingressauth_test.go @@ -4,6 +4,9 @@ package ingressauth import ( + "bytes" + "fmt" + "log/slog" "net/http" "net/http/httptest" "net/netip" @@ -11,6 +14,7 @@ import ( "path/filepath" "runtime" "strings" + "sync" "testing" "time" ) @@ -44,6 +48,12 @@ func TestValidateKey(t *testing.T) { {"embedded tab", "0123456789abcdef\t123456789abcdef0", false}, {"non-ascii", "0123456789abcdef0123456789abcdé", false}, {"control char", "0123456789abcdef0123456789abcde\x01", false}, + {"double quote", strings.Repeat("a", MinKeyLength-1) + `"`, false}, + {"backslash", strings.Repeat("a", MinKeyLength-1) + `\`, false}, + {"base64 alphabet", "abcd+/ABCD0123456789abcdef012345==", true}, + {"exactly maximum length", strings.Repeat("k", MaxKeyLength), true}, + {"one over maximum", strings.Repeat("k", MaxKeyLength+1), false}, + {"urlsafe alphabet", "abcd-_ABCD0123456789abcdef012345~.", true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -98,22 +108,28 @@ func TestAuthorizeCredentials(t *testing.T) { status int fp string }{ - {"bearer first key", []string{"Authorization", "Bearer " + keyA}, true, http.StatusOK, Fingerprint(keyA)}, - {"bearer second key", []string{"Authorization", "Bearer " + keyB}, true, http.StatusOK, Fingerprint(keyB)}, - {"lowercase scheme", []string{"Authorization", "bearer " + keyA}, true, http.StatusOK, Fingerprint(keyA)}, - {"x-api-key", []string{"X-Api-Key", keyA}, true, http.StatusOK, Fingerprint(keyA)}, - {"x-api-key lowercase header name", []string{"x-api-key", keyB}, true, http.StatusOK, Fingerprint(keyB)}, + {"bearer first key", []string{"Authorization", "Bearer " + keyA}, true, http.StatusOK, fingerprint(keyA)}, + {"bearer second key", []string{"Authorization", "Bearer " + keyB}, true, http.StatusOK, fingerprint(keyB)}, + {"lowercase scheme", []string{"Authorization", "bearer " + keyA}, true, http.StatusOK, fingerprint(keyA)}, + {"x-api-key", []string{"X-Api-Key", keyA}, true, http.StatusOK, fingerprint(keyA)}, + {"x-api-key lowercase header name", []string{"x-api-key", keyB}, true, http.StatusOK, fingerprint(keyB)}, {"no credential", nil, false, http.StatusUnauthorized, noCredential}, - {"wrong key", []string{"Authorization", "Bearer " + keyC}, false, http.StatusUnauthorized, Fingerprint(keyC)}, + {"wrong key", []string{"Authorization", "Bearer " + keyC}, false, http.StatusUnauthorized, fingerprint(keyC)}, {"wrong scheme", []string{"Authorization", "Basic " + keyA}, false, http.StatusUnauthorized, noCredential}, {"bearer with no token", []string{"Authorization", "Bearer "}, false, http.StatusUnauthorized, noCredential}, - {"key as prefix only", []string{"Authorization", "Bearer " + keyA + "x"}, false, http.StatusUnauthorized, Fingerprint(keyA + "x")}, - {"wrong bearer but right x-api-key", []string{"Authorization", "Bearer " + keyC, "X-Api-Key", keyA}, false, http.StatusUnauthorized, Fingerprint(keyC)}, + {"key as prefix only", []string{"Authorization", "Bearer " + keyA + "x"}, false, http.StatusUnauthorized, fingerprint(keyA + "x")}, + // An SDK placeholder Bearer beside a real X-Api-Key must not lock the client out. + {"placeholder bearer but right x-api-key", []string{"Authorization", "Bearer " + keyC, "X-Api-Key", keyA}, true, http.StatusOK, fingerprint(keyA)}, + {"right bearer but stale x-api-key", []string{"Authorization", "Bearer " + keyA, "X-Api-Key", keyC}, true, http.StatusOK, fingerprint(keyA)}, + {"both wrong", []string{"Authorization", "Bearer " + keyC, "X-Api-Key", keyC + "x"}, false, http.StatusUnauthorized, fingerprint(keyC)}, + // An over-long value is not a key: it is not hashed, so it counts as no credential. + {"over-long bearer", []string{"Authorization", "Bearer " + strings.Repeat("a", MaxKeyLength+1)}, false, http.StatusUnauthorized, noCredential}, + {"over-long x-api-key beside a valid bearer", []string{"Authorization", "Bearer " + keyA, "X-Api-Key", strings.Repeat("a", 1<<20)}, true, http.StatusOK, fingerprint(keyA)}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { d := g.Authorize(request("192.0.2.9:40000", tc.hdr...)) - if d.Allowed != tc.allow || d.Status != tc.status { + if d.Allowed != tc.allow || (!tc.allow && d.Status != tc.status) { t.Fatalf("decision = %+v, want allowed=%v status=%d", d, tc.allow, tc.status) } if d.KeyFingerprint != tc.fp { @@ -125,8 +141,22 @@ func TestAuthorizeCredentials(t *testing.T) { if strings.Contains(d.Message, keyA) || strings.Contains(d.Message, keyC) { t.Errorf("message echoes a key: %q", d.Message) } + if !d.Enabled { + t.Error("decision from an enabled gate reports Enabled=false") + } }) } + // RFC 6750 §3: a bare challenge when nothing was presented, error="invalid_token" + // when a credential was examined and rejected, no challenge on success. + if d := g.Authorize(request("192.0.2.9:1")); d.Challenge != challengeMissing { + t.Errorf("no-credential challenge = %q, want %q", d.Challenge, challengeMissing) + } + if d := g.Authorize(request("192.0.2.9:1", "X-Api-Key", keyC)); d.Challenge != challengeInvalid { + t.Errorf("wrong-key challenge = %q, want %q", d.Challenge, challengeInvalid) + } + if d := g.Authorize(request("192.0.2.9:1", "X-Api-Key", keyA)); d.Challenge != "" { + t.Errorf("challenge on success = %q, want none", d.Challenge) + } } func TestAuthorizeCIDRAllowlist(t *testing.T) { @@ -190,27 +220,56 @@ func TestDisabledGateNeverAllows(t *testing.T) { if zero.Enabled() { t.Fatal("zero Gate reports enabled") } - if d := zero.Authorize(request("192.0.2.9:1", "Authorization", "Bearer "+keyA)); d.Allowed { - t.Fatalf("zero Gate allowed a request: %+v", d) + if d := zero.Authorize(request("192.0.2.9:1", "Authorization", "Bearer "+keyA)); d.Allowed || d.Enabled { + t.Fatalf("zero Gate decision = %+v, want neither enabled nor allowed", d) } empty := New(nil, nil) if empty.Enabled() { t.Fatal("New(nil, nil) reports enabled") } + if d := empty.Authorize(request("192.0.2.9:1", "Authorization", "Bearer "+keyA)); d.Allowed || d.Enabled { + t.Fatalf("keyless Gate decision = %+v, want neither enabled nor allowed", d) + } +} + +// TestKeyFileSameSizeSameTimeRewriteIsNoticed: rotating a key for another of +// the same length, within the filesystem's timestamp granularity, must take +// effect — a stamp of size and mtime cannot see it, so the gate hashes content. +func TestKeyFileSameSizeSameTimeRewriteIsNoticed(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "keys") + g := &Gate{filePath: path, explicitFile: true} + when := time.Now().Add(-time.Hour).Truncate(time.Second) + keyA2 := strings.ToUpper(keyA) // same length, different bytes + if len(keyA2) != len(keyA) || keyA2 == keyA { + t.Fatal("test keys must differ only in content") + } + + writeKeyFile(t, path, keyA+"\n", 0o600, when) + if d := g.Authorize(request("192.0.2.9:1", "X-Api-Key", keyA)); !d.Allowed { + t.Fatalf("initial key rejected: %+v", d) + } + writeKeyFile(t, path, keyA2+"\n", 0o600, when) // identical size, mtime, and mode + if d := g.Authorize(request("192.0.2.9:1", "X-Api-Key", keyA)); d.Allowed { + t.Fatal("rotated-out key still accepted after a same-size, same-time rewrite") + } + if d := g.Authorize(request("192.0.2.9:1", "X-Api-Key", keyA2)); !d.Allowed { + t.Fatalf("rotated-in key rejected: %+v", d) + } } func TestFingerprintIsShortStableHexAndNotTheKey(t *testing.T) { - fp := Fingerprint(keyA) + fp := fingerprint(keyA) if len(fp) != 8 { t.Fatalf("fingerprint %q has length %d, want 8", fp, len(fp)) } if strings.ToLower(fp) != fp || strings.Trim(fp, "0123456789abcdef") != "" { t.Fatalf("fingerprint %q is not lowercase hex", fp) } - if fp != Fingerprint(keyA) { + if fp != fingerprint(keyA) { t.Fatal("fingerprint is not stable") } - if fp == Fingerprint(keyB) { + if fp == fingerprint(keyB) { t.Fatal("distinct keys share a fingerprint") } if strings.Contains(keyA, fp) { @@ -454,3 +513,176 @@ func TestFromEnvExplicitMissingFileIsDisabled(t *testing.T) { t.Fatal("enabled with a missing explicit key file") } } + +// The parsers face operator files and attacker-controlled headers; none of them +// may panic on arbitrary bytes, and anything validateKey accepts must be a key +// the gate can actually match over the wire. +func FuzzValidateKey(f *testing.F) { + for _, seed := range []string{"", keyA, keyB, keyC, "short", "0123456789abcdef 123456789abcdef0", "é", "\x00\xff"} { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, key string) { + if err := validateKey(key); err == nil { + if len(key) < MinKeyLength { + t.Fatalf("accepted %d-character key", len(key)) + } + for i := 0; i < len(key); i++ { + if !isTokenByte(key[i]) { + t.Fatalf("accepted key with byte %q", key[i]) + } + } + } + }) +} + +func FuzzParseKeys(f *testing.F) { + for _, seed := range []string{"", "# c\n", keyA + "\n", keyA + "\r\n" + keyB + "\n", "\x00\n\xff", strings.Repeat("a", 4096)} { + f.Add([]byte(seed)) + } + f.Fuzz(func(t *testing.T, data []byte) { + keys, err := parseKeys(bytes.NewReader(data)) + if err == nil && len(keys) == 0 { + t.Fatal("no error and no keys") + } + }) +} + +func FuzzCredentialsFrom(f *testing.F) { + for _, seed := range [][2]string{{"Bearer " + keyA, ""}, {"bearer", ""}, {"Basic x", keyA}, {"", "\x00"}, {"Bearer ", " "}} { + f.Add(seed[0], seed[1]) + } + f.Fuzz(func(t *testing.T, auth, apiKey string) { + r := request("192.0.2.9:1") + r.Header.Set("Authorization", auth) + r.Header.Set("X-Api-Key", apiKey) + for _, c := range credentialsFrom(r) { + if c == "" || c != strings.TrimSpace(c) || len(c) > MaxKeyLength { + t.Fatalf("extracted credential %q is empty, untrimmed, or over-long", c) + } + } + }) +} + +// captureLog routes slog to a buffer for the test's duration. +func captureLog(t *testing.T) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil))) + t.Cleanup(func() { slog.SetDefault(prev) }) + return &buf +} + +// TestRotationIsLogged: SECURITY.md promises a log line whenever the key set +// changes, and a rotation that keeps the key count is the case an operator +// most needs to see. +func TestRotationIsLogged(t *testing.T) { + path := filepath.Join(t.TempDir(), "keys") + g := &Gate{filePath: path, explicitFile: true} + when := time.Now().Add(-time.Hour) + buf := captureLog(t) + + writeKeyFile(t, path, keyA+"\n", 0o600, when) + g.Enabled() + if !strings.Contains(buf.String(), "ENABLED") { + t.Fatalf("enabling not logged:\n%s", buf.String()) + } + buf.Reset() + writeKeyFile(t, path, keyB+"\n", 0o600, when.Add(time.Second)) + g.Enabled() + if !strings.Contains(buf.String(), "key file changed") { + t.Fatalf("rotation with an unchanged key count not logged:\n%s", buf.String()) + } + if strings.Contains(buf.String(), keyA) || strings.Contains(buf.String(), keyB) { + t.Fatal("a key reached the log") + } + buf.Reset() + g.Enabled() + if buf.Len() != 0 { + t.Fatalf("unchanged state logged again:\n%s", buf.String()) + } +} + +// TestRecheckFloorBoundsFileReads: a FromEnv gate re-reads the key file at most +// once per recheckEvery, so a node that never opted in does not pay an open() +// for every unauthenticated LAN request. +func TestRecheckFloorBoundsFileReads(t *testing.T) { + path := filepath.Join(t.TempDir(), "keys") + g := &Gate{filePath: path, explicitFile: true, recheckEvery: time.Hour} + when := time.Now().Add(-time.Hour) + + writeKeyFile(t, path, keyA+"\n", 0o600, when) + if !g.Enabled() { + t.Fatal("first look did not load the file") + } + writeKeyFile(t, path, keyB+"\n", 0o600, when.Add(time.Second)) + if d := g.Authorize(request("192.0.2.9:1", "X-Api-Key", keyA)); !d.Allowed { + t.Fatal("the file was re-read inside the recheck window") + } + g.lastCheck = time.Time{} // window elapsed + if d := g.Authorize(request("192.0.2.9:1", "X-Api-Key", keyB)); !d.Allowed { + t.Fatalf("rotated key not picked up after the window: %+v", d) + } + clearEnv(t) + if got := FromEnv().recheckEvery; got != defaultRecheckEvery { + t.Fatalf("FromEnv recheckEvery = %v, want %v", got, defaultRecheckEvery) + } + _ = fmt.Sprint +} + +// TestConcurrentAuthorizeDuringRotation drives Authorize from many goroutines +// while the key file is rewritten underneath; run under -race. Every decision +// must be for exactly one of the two keys that were ever valid — never neither, +// never both — and a never-configured key must never pass. +func TestConcurrentAuthorizeDuringRotation(t *testing.T) { + path := filepath.Join(t.TempDir(), "keys") + g := &Gate{filePath: path, explicitFile: true} + when := time.Now().Add(-time.Hour) + writeKeyFile(t, path, keyA+"\n", 0o600, when) + + var wg sync.WaitGroup + stop := make(chan struct{}) + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + } + // Both answers must come from one view of the file, so a + // rotation between the two calls must not be confused with a + // state where neither or both keys are valid: judge a single + // request carrying both keys, which Authorize checks together. + both := g.Authorize(request("192.0.2.9:1", "Authorization", "Bearer "+keyA, "X-Api-Key", keyB)) + if !both.Allowed { + t.Error("neither of the two ever-valid keys was accepted") + return + } + if g.Authorize(request("192.0.2.9:1", "X-Api-Key", keyC)).Allowed { + t.Error("a never-configured key was accepted") + return + } + } + }() + } + // Rotate the way an operator should: write the new file beside the old one + // and rename it into place, so no reader ever sees a truncated file. (A + // truncating rewrite would be seen as an empty key file for one recheck — + // correctly fail-closed, but not what this test is about.) + for i := 1; i <= 20; i++ { + k := keyA + if i%2 == 1 { + k = keyB + } + tmp := path + ".tmp" + writeKeyFile(t, tmp, k+"\n", 0o600, when.Add(time.Duration(i)*time.Second)) + if err := os.Rename(tmp, path); err != nil { + t.Fatal(err) + } + } + close(stop) + wg.Wait() +} diff --git a/services/shared/ingressauth/owner_unix.go b/services/shared/ingressauth/owner_unix.go new file mode 100644 index 00000000..c3d50a14 --- /dev/null +++ b/services/shared/ingressauth/owner_unix.go @@ -0,0 +1,31 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package ingressauth + +import ( + "errors" + "fmt" + "io/fs" + "os" + "syscall" +) + +// ownedByProcessUser refuses a key file that belongs to another account, the +// way sshd treats authorized_keys: a private mode is not enough if someone else +// owns the file and can change its contents or mode at will. root may own the +// file, since an administrator may provision it for a service user. A +// FileInfo without Unix ownership data is refused too: on a Unix-like system +// that is not a file the gate can vouch for. +func ownedByProcessUser(info fs.FileInfo) error { + st, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return errors.New("cannot determine the key file's owner") + } + if uid := int(st.Uid); uid != os.Geteuid() && uid != 0 { + return fmt.Errorf("owned by uid %d, not by the proxy's user (uid %d)", uid, os.Geteuid()) + } + return nil +} diff --git a/services/shared/ingressauth/owner_unix_test.go b/services/shared/ingressauth/owner_unix_test.go new file mode 100644 index 00000000..200a5ee2 --- /dev/null +++ b/services/shared/ingressauth/owner_unix_test.go @@ -0,0 +1,59 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package ingressauth + +import ( + "io/fs" + "os" + "path/filepath" + "syscall" + "testing" +) + +// fakeInfo is a fs.FileInfo whose Sys() the test controls, so ownership cases +// that would otherwise need root (a file owned by someone else) can be +// exercised. +type fakeInfo struct { + fs.FileInfo + sys any +} + +func (f fakeInfo) Sys() any { return f.sys } + +func TestOwnedByProcessUser(t *testing.T) { + path := filepath.Join(t.TempDir(), "keys") + if err := os.WriteFile(path, []byte(keyA+"\n"), 0o600); err != nil { + t.Fatal(err) + } + real, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if err := ownedByProcessUser(real); err != nil { + t.Fatalf("a file this process just created is refused: %v", err) + } + + me := uint32(os.Geteuid()) + cases := []struct { + name string + sys any + ok bool + }{ + {"owned by the process user", &syscall.Stat_t{Uid: me}, true}, + {"owned by root", &syscall.Stat_t{Uid: 0}, true}, + {"owned by another user", &syscall.Stat_t{Uid: me + 1}, false}, + {"no ownership data", nil, false}, + {"foreign Sys type", struct{}{}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ownedByProcessUser(fakeInfo{FileInfo: real, sys: tc.sys}) + if (err == nil) != tc.ok { + t.Fatalf("ownedByProcessUser err = %v, want ok=%v", err, tc.ok) + } + }) + } +} diff --git a/services/shared/ingressauth/owner_windows.go b/services/shared/ingressauth/owner_windows.go new file mode 100644 index 00000000..5ec1e520 --- /dev/null +++ b/services/shared/ingressauth/owner_windows.go @@ -0,0 +1,13 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package ingressauth + +import "io/fs" + +// ownedByProcessUser is a no-op on Windows, where ownership and access are +// expressed through ACLs rather than a uid; the per-user %LOCALAPPDATA% data +// directory that holds the default key file is the protection there. +func ownedByProcessUser(fs.FileInfo) error { return nil } From 3908a9d8a5d4dd7e597d942237c2c3734a7b4e60 Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 22 Sep 2026 20:22:41 -0500 Subject: [PATCH 3/6] Answer LAN preflights from the engine's CORS policy develop replaced cors.WritePreflight with engine-backed preflights (cors.ServePreflight) when browser access began following the engine's CORS policy, so the port no longer compiled. A non-loopback preflight now skips only the credential check and continues into handleHTTP, which answers it from the engine exactly as for a loopback caller. The source allowlist still applies first, and the request that follows still receives the real 401/403. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BSXRLJk5knQd4rNnn7XH1S Signed-off-by: Aaron K. Clark --- services/nvpair-proxy/ingress.go | 25 ++++++++------- services/nvpair-proxy/ingress_auth_test.go | 36 ++++++++++++---------- 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/services/nvpair-proxy/ingress.go b/services/nvpair-proxy/ingress.go index 98e79ccb..271d1ada 100644 --- a/services/nvpair-proxy/ingress.go +++ b/services/nvpair-proxy/ingress.go @@ -100,15 +100,6 @@ func (f *facade) handlePlain(w http.ResponseWriter, r *http.Request) { writeIngressError(w, d.Status, d.Code, d.Message) return } - // Answer a non-loopback preflight ahead of the credential check. It - // grants no access on its own; the request that follows still receives - // the real 401/403, and a browser sends no Authorization on a preflight - // anyway. A loopback preflight continues into handleHTTP so an engine's - // exact origin and credentials policy can be preserved (see proxy.go). - if d.Enabled && cors.IsPreflight(r) { - cors.WritePreflight(w, r) - return - } if !d.Enabled { slog.Warn("rejected non-loopback plaintext request; cluster peers must use mTLS", "remote", r.RemoteAddr, "method", r.Method, "path", r.URL.Path) @@ -116,7 +107,13 @@ func (f *facade) handlePlain(w http.ResponseWriter, r *http.Request) { "plaintext requests are accepted only from loopback; cluster peers must use the mTLS ingress") return } - if !d.Allowed { + // A browser sends no Authorization on a preflight, so a non-loopback + // preflight skips the credential check and continues into handleHTTP, + // which answers it from the engine's own CORS policy exactly as for a + // loopback caller. It grants no access on its own: the request that + // follows still receives the real 401/403. + preflight := cors.IsPreflight(r) + if !preflight && !d.Allowed { slog.Warn("rejected non-loopback plaintext request", "remote", r.RemoteAddr, "method", r.Method, "path", r.URL.Path, "code", d.Code, "key_fp", d.KeyFingerprint) if d.Challenge != "" { @@ -127,9 +124,11 @@ func (f *facade) handlePlain(w http.ResponseWriter, r *http.Request) { } // The key is the proxy's credential, not the engine's: never forward it. f.host.lanAuth.StripCredential(r.Header) - // At Info, not Debug: a production log must show who used a key. - slog.Info("authenticated non-loopback plaintext request", "remote", r.RemoteAddr, - "method", r.Method, "path", r.URL.Path, "key_fp", d.KeyFingerprint) + if !preflight { + // At Info, not Debug: a production log must show who used a key. + slog.Info("authenticated non-loopback plaintext request", "remote", r.RemoteAddr, + "method", r.Method, "path", r.URL.Path, "key_fp", d.KeyFingerprint) + } } // Engine-manager marks its private identity/action requests so this // compatibility facade can never be mistaken for the local Ollama backend. diff --git a/services/nvpair-proxy/ingress_auth_test.go b/services/nvpair-proxy/ingress_auth_test.go index 71122d8f..f5e9d47e 100644 --- a/services/nvpair-proxy/ingress_auth_test.go +++ b/services/nvpair-proxy/ingress_auth_test.go @@ -206,10 +206,11 @@ func TestHandlePlainInsideAllowedCIDRIsRouted(t *testing.T) { } } -// TestHandlePlainPreflightStillAnsweredWhenEnabled: a browser sends no -// Authorization on a preflight, so the 204 must keep preceding the credential -// check once the gate is on. -func TestHandlePlainPreflightStillAnsweredWhenEnabled(t *testing.T) { +// TestHandlePlainPreflightReachesEngineWhenEnabled: a browser sends no +// Authorization on a preflight, so once the gate is on a keyless LAN preflight +// must not be refused with 401; it is answered from the engine's own CORS +// policy, as a loopback preflight is. +func TestHandlePlainPreflightReachesEngineWhenEnabled(t *testing.T) { f, seen := lanProxy(t) rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) @@ -219,11 +220,11 @@ func TestHandlePlainPreflightStillAnsweredWhenEnabled(t *testing.T) { req.Header.Set("Access-Control-Request-Headers", "Authorization") f.handlePlain(rec, req) - if rec.Code != http.StatusNoContent { - t.Fatalf("LAN preflight status = %d, want 204", rec.Code) + if rec.Code == http.StatusUnauthorized || rec.Code == http.StatusForbidden { + t.Fatalf("LAN preflight status = %d, want the engine's answer", rec.Code) } - if seen.Load() != nil { - t.Fatal("a preflight reached the engine") + if seen.Load() == nil { + t.Fatal("the preflight never reached the engine's CORS policy") } } @@ -247,8 +248,8 @@ func TestHandlePlainGateWithoutKeysKeepsLoopbackOnly(t *testing.T) { } // TestHandlePlainPreflightOutsideAllowedCIDRIs403: the allowlist applies to a -// preflight too. A source the operator excluded gets no 204 that would let a -// browser proceed to the request that follows. +// preflight too. A source the operator excluded gets no CORS answer that would +// let a browser proceed to the request that follows. func TestHandlePlainPreflightOutsideAllowedCIDRIs403(t *testing.T) { f, seen := lanProxy(t, netip.MustParsePrefix("10.0.0.0/8")) rec := httptest.NewRecorder() @@ -269,17 +270,20 @@ func TestHandlePlainPreflightOutsideAllowedCIDRIs403(t *testing.T) { } } -// TestHandlePlainPreflightInsideAllowedCIDRIs204: inside the allowlist the -// preflight is still answered without a credential, as browsers require. -func TestHandlePlainPreflightInsideAllowedCIDRIs204(t *testing.T) { - f, _ := lanProxy(t, netip.MustParsePrefix("192.0.2.0/24")) +// TestHandlePlainPreflightInsideAllowedCIDRReachesEngine: inside the allowlist +// the preflight is still answered without a credential, as browsers require. +func TestHandlePlainPreflightInsideAllowedCIDRReachesEngine(t *testing.T) { + f, seen := lanProxy(t, netip.MustParsePrefix("192.0.2.0/24")) rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) req.RemoteAddr = lanRemote req.Header.Set("Origin", "http://app.test") req.Header.Set("Access-Control-Request-Method", "POST") f.handlePlain(rec, req) - if rec.Code != http.StatusNoContent { - t.Fatalf("in-allowlist preflight status = %d, want 204", rec.Code) + if rec.Code == http.StatusUnauthorized || rec.Code == http.StatusForbidden { + t.Fatalf("in-allowlist preflight status = %d, want the engine's answer", rec.Code) + } + if seen.Load() == nil { + t.Fatal("the preflight never reached the engine's CORS policy") } } From f61aa4b5f819f95b5d593fc184ec0e3bec61b333 Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 22 Sep 2026 20:22:41 -0500 Subject: [PATCH 4/6] Accept a Kubernetes fsGroup Secret mount as the key file A Secret mounted under a pod fsGroup arrives root-owned and group-readable (0440), which the key-file check refused, so the gate stayed closed on OpenShift and most clusters (reported on #38). Group read is now accepted in that shape only: a root-owned file, readable and not writable by a group the proxy belongs to, with nothing granted to others. Requiring root ownership keeps a user from opening their own key to a shared login group such as macOS "staff". The permission and ownership rules move together into checkKeyFileAccess, per platform. The docs now recommend the inline variable from a Secret via valueFrom on Kubernetes, describe the accepted mount shape and the pod-CIDR allowlist, and state that a refused key file does not disable inline keys. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BSXRLJk5knQd4rNnn7XH1S Signed-off-by: Aaron K. Clark --- SECURITY.md | 7 ++- docs/getting-started.mdx | 11 ++++- services/shared/ingressauth/ingressauth.go | 14 ++---- services/shared/ingressauth/owner_unix.go | 47 +++++++++++++++---- .../shared/ingressauth/owner_unix_test.go | 42 +++++++++++------ services/shared/ingressauth/owner_windows.go | 9 ++-- 6 files changed, 90 insertions(+), 40 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index eb3fff26..70aaf47a 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -110,7 +110,12 @@ process environment in clear, so prefer the file), never logs a presented key (a rejection logs a short digest fingerprint), and fails closed: a key file that other users can read (judged by Unix permission bits; on Windows the check is skipped and the per-user data directory's ACL is the protection), that contains a malformed entry, or that cannot be read -contributes no keys and the LAN stays closed. +contributes no keys. That is judged per source: a rejected file does not +disable keys supplied through `NVPAIR_PROXY_API_KEYS`, and with neither source +yielding a key the LAN stays closed. The one group-readable shape the gate +accepts is a root-owned file readable (not writable) only by a group the proxy +itself belongs to, which is how Kubernetes mounts a Secret under a pod +`fsGroup`. Enabling the gate is logged at warning level at startup and whenever the key set changes. It adds no TLS, rate limiting, or per-key permissions: the plaintext personality stays plaintext, so use it only on a network you trust, and pair it diff --git a/docs/getting-started.mdx b/docs/getting-started.mdx index 46b43777..e6771fa5 100644 --- a/docs/getting-started.mdx +++ b/docs/getting-started.mdx @@ -499,7 +499,16 @@ variables refine the behavior for headless or containerized nodes: `NVPAIR_PROXY_API_KEYS_FILE` names a different key file, `NVPAIR_PROXY_API_KEYS` supplies keys inline (comma-separated), and `NVPAIR_PROXY_ALLOWED_CIDRS` (for example `192.168.1.0/24,10.0.0.0/8`) additionally restricts which source networks -may use a key at all. +may use a key at all. The file and inline keys are separate sources: a key +file that is refused contributes no keys, but inline keys still work. + +On Kubernetes, supply the key through `NVPAIR_PROXY_API_KEYS` from a Secret +with `valueFrom.secretKeyRef`. A Secret mounted as a file and pointed to by +`NVPAIR_PROXY_API_KEYS_FILE` also works under a pod `fsGroup`, which mounts it +root-owned and group-readable (`0440`): the proxy accepts group read on a +root-owned file whose group it belongs to. Set the allowlist to the pod +network's CIDR: on OVN-Kubernetes, a pod reaching a `hostNetwork` node on the same +host arrives from its pod IP, not the node's address. Understand what you are enabling. The connection is plain HTTP, so prompts and responses cross your network unencrypted, and anyone holding the key can use the diff --git a/services/shared/ingressauth/ingressauth.go b/services/shared/ingressauth/ingressauth.go index 76a97f07..fbb8a5e5 100644 --- a/services/shared/ingressauth/ingressauth.go +++ b/services/shared/ingressauth/ingressauth.go @@ -39,7 +39,6 @@ import ( "net/http" "net/netip" "os" - "runtime" "strings" "sync" "time" @@ -441,10 +440,8 @@ func (g *Gate) logFileErrorLocked(msg string, err error) { // readKeyFile opens the key file and checks the opened handle — not a separate // stat — before reading, so the permissions, type, and owner it validates -// belong to the file it reads. On Unix-like systems the file must belong to -// the proxy's user (or root) and must not be readable or writable by group or -// others; on Windows the mode bits carry no such meaning and the check is -// skipped, leaving protection to the data directory's ACL. +// belong to the file it reads. checkKeyFileAccess holds the per-platform +// ownership and permission rules. func readKeyFile(path string) ([]byte, error) { f, err := os.Open(path) if err != nil { @@ -458,12 +455,7 @@ func readKeyFile(path string) ([]byte, error) { if !info.Mode().IsRegular() { return nil, errors.New("not a regular file") } - if runtime.GOOS != "windows" { - if perm := info.Mode().Perm(); perm&0o077 != 0 { - return nil, fmt.Errorf("permissions %04o allow other users to read it; chmod 600", perm) - } - } - if err := ownedByProcessUser(info); err != nil { + if err := checkKeyFileAccess(info); err != nil { return nil, err } content, err := io.ReadAll(io.LimitReader(f, maxKeyFileBytes+1)) diff --git a/services/shared/ingressauth/owner_unix.go b/services/shared/ingressauth/owner_unix.go index c3d50a14..8be3d39b 100644 --- a/services/shared/ingressauth/owner_unix.go +++ b/services/shared/ingressauth/owner_unix.go @@ -10,22 +10,53 @@ import ( "fmt" "io/fs" "os" + "slices" "syscall" ) -// ownedByProcessUser refuses a key file that belongs to another account, the -// way sshd treats authorized_keys: a private mode is not enough if someone else -// owns the file and can change its contents or mode at will. root may own the -// file, since an administrator may provision it for a service user. A +// checkKeyFileAccess refuses a key file that another account could read or +// change, the way sshd treats authorized_keys: a private mode is not enough if +// someone else owns the file and can change its contents or mode at will. The +// file must belong to the proxy's user or to root, since an administrator may +// provision it for a service user, and must grant nothing to others. A // FileInfo without Unix ownership data is refused too: on a Unix-like system // that is not a file the gate can vouch for. -func ownedByProcessUser(info fs.FileInfo) error { +// +// Group read is refused except in one shape: a root-owned file, read-only to a +// group the proxy itself belongs to (0440 or 0640). That is how Kubernetes +// mounts a Secret under a pod fsGroup, where the group exists for this +// workload alone. The file must be root's so a user cannot open their own key +// to a shared login group, such as macOS "staff", on the proxy's behalf. +func checkKeyFileAccess(info fs.FileInfo) error { + return checkKeyFileAccessAs(info, os.Geteuid(), processGroups()) +} + +func checkKeyFileAccessAs(info fs.FileInfo, euid int, groups []int) error { st, ok := info.Sys().(*syscall.Stat_t) if !ok { return errors.New("cannot determine the key file's owner") } - if uid := int(st.Uid); uid != os.Geteuid() && uid != 0 { - return fmt.Errorf("owned by uid %d, not by the proxy's user (uid %d)", uid, os.Geteuid()) + uid, gid := int(st.Uid), int(st.Gid) + if uid != euid && uid != 0 { + return fmt.Errorf("owned by uid %d, not by the proxy's user (uid %d)", uid, euid) + } + perm := info.Mode().Perm() + switch { + case perm&0o077 == 0: + return nil + case perm&0o037 == 0 && uid == 0 && slices.Contains(groups, gid): + return nil + case perm&0o007 != 0: + return fmt.Errorf("permissions %04o allow other users to read it; chmod 600", perm) + default: + return fmt.Errorf("permissions %04o allow group %d to access it; chmod 600, or make it root-owned, group read-only (0440), and in a group the proxy belongs to", perm, gid) } - return nil +} + +// processGroups is the proxy's effective and supplementary group IDs. A +// failure to list the supplementary ones leaves just the effective group, +// which only narrows what checkKeyFileAccess accepts. +func processGroups() []int { + groups, _ := os.Getgroups() + return append(groups, os.Getegid()) } diff --git a/services/shared/ingressauth/owner_unix_test.go b/services/shared/ingressauth/owner_unix_test.go index 200a5ee2..e03d38bc 100644 --- a/services/shared/ingressauth/owner_unix_test.go +++ b/services/shared/ingressauth/owner_unix_test.go @@ -13,17 +13,19 @@ import ( "testing" ) -// fakeInfo is a fs.FileInfo whose Sys() the test controls, so ownership cases -// that would otherwise need root (a file owned by someone else) can be -// exercised. +// fakeInfo is a fs.FileInfo whose mode and Sys() the test controls, so +// ownership cases that would otherwise need root (a file owned by someone +// else, or by root in a Kubernetes fsGroup) can be exercised. type fakeInfo struct { fs.FileInfo - sys any + mode fs.FileMode + sys any } -func (f fakeInfo) Sys() any { return f.sys } +func (f fakeInfo) Mode() fs.FileMode { return f.mode } +func (f fakeInfo) Sys() any { return f.sys } -func TestOwnedByProcessUser(t *testing.T) { +func TestCheckKeyFileAccess(t *testing.T) { path := filepath.Join(t.TempDir(), "keys") if err := os.WriteFile(path, []byte(keyA+"\n"), 0o600); err != nil { t.Fatal(err) @@ -32,27 +34,37 @@ func TestOwnedByProcessUser(t *testing.T) { if err != nil { t.Fatal(err) } - if err := ownedByProcessUser(real); err != nil { + if err := checkKeyFileAccess(real); err != nil { t.Fatalf("a file this process just created is refused: %v", err) } - me := uint32(os.Geteuid()) + const me, other, fsGroup, shared = 1000, 1001, 2000, 20 + groups := []int{shared, fsGroup} cases := []struct { name string + mode fs.FileMode sys any ok bool }{ - {"owned by the process user", &syscall.Stat_t{Uid: me}, true}, - {"owned by root", &syscall.Stat_t{Uid: 0}, true}, - {"owned by another user", &syscall.Stat_t{Uid: me + 1}, false}, - {"no ownership data", nil, false}, - {"foreign Sys type", struct{}{}, false}, + {"owned by the process user", 0o600, &syscall.Stat_t{Uid: me}, true}, + {"owned by root", 0o400, &syscall.Stat_t{Uid: 0}, true}, + {"owned by another user", 0o600, &syscall.Stat_t{Uid: other}, false}, + {"world-readable", 0o604, &syscall.Stat_t{Uid: me}, false}, + {"Kubernetes Secret under fsGroup, 0440", 0o440, &syscall.Stat_t{Uid: 0, Gid: fsGroup}, true}, + {"Kubernetes Secret under fsGroup, 0640", 0o640, &syscall.Stat_t{Uid: 0, Gid: fsGroup}, true}, + {"root-owned, group not the proxy's", 0o440, &syscall.Stat_t{Uid: 0, Gid: 3000}, false}, + {"root-owned, group-writable", 0o460, &syscall.Stat_t{Uid: 0, Gid: fsGroup}, false}, + {"root-owned, group-executable", 0o450, &syscall.Stat_t{Uid: 0, Gid: fsGroup}, false}, + {"root-owned, group-readable and world-readable", 0o444, &syscall.Stat_t{Uid: 0, Gid: fsGroup}, false}, + {"user-owned, group-readable to a shared group", 0o640, &syscall.Stat_t{Uid: me, Gid: shared}, false}, + {"no ownership data", 0o600, nil, false}, + {"foreign Sys type", 0o600, struct{}{}, false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - err := ownedByProcessUser(fakeInfo{FileInfo: real, sys: tc.sys}) + err := checkKeyFileAccessAs(fakeInfo{FileInfo: real, mode: tc.mode, sys: tc.sys}, me, groups) if (err == nil) != tc.ok { - t.Fatalf("ownedByProcessUser err = %v, want ok=%v", err, tc.ok) + t.Fatalf("checkKeyFileAccessAs err = %v, want ok=%v", err, tc.ok) } }) } diff --git a/services/shared/ingressauth/owner_windows.go b/services/shared/ingressauth/owner_windows.go index 5ec1e520..648567e2 100644 --- a/services/shared/ingressauth/owner_windows.go +++ b/services/shared/ingressauth/owner_windows.go @@ -7,7 +7,8 @@ package ingressauth import "io/fs" -// ownedByProcessUser is a no-op on Windows, where ownership and access are -// expressed through ACLs rather than a uid; the per-user %LOCALAPPDATA% data -// directory that holds the default key file is the protection there. -func ownedByProcessUser(fs.FileInfo) error { return nil } +// checkKeyFileAccess is a no-op on Windows, where ownership and access are +// expressed through ACLs rather than a uid and mode bits; the per-user +// %LOCALAPPDATA% data directory that holds the default key file is the +// protection there. +func checkKeyFileAccess(fs.FileInfo) error { return nil } From e65ea8a664bb300368906a7fe2e80dcec84f555a Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 22 Sep 2026 21:22:11 -0500 Subject: [PATCH 5/6] Refuse keyless LAN preflights and group-readable key files An 11-model review of the port found two problems, several proved by probe tests against the branch. A keyless non-loopback preflight skipped the credential check and entered handleHTTP, which reads the whole body into memory before it recognizes a preflight (a 256 MiB probe was read in full), then fans the OPTIONS out to every cluster candidate over this node's mTLS identity and, with one candidate, relays the engine's raw reply for any path. A preflight is now judged like any other request: without a key it gets the 401. Browsers cannot send a key on a preflight, and the 401 carries no CORS, so a web page on another machine was never a working client; the docs now say so. The group-read exception for a root-owned key file in one of the proxy's groups could not tell a Kubernetes fsGroup from a shared group: root:staff, root:everyone and root:admin 0440 were all accepted on macOS, and the refusal message steered operators toward that shape. Group read is refused again. On Kubernetes the key goes in through NVPAIR_PROXY_API_KEYS from a Secret via valueFrom, as the docs say. Also: the proxy spec and README no longer say LAN plaintext is always refused, the OpenAPI document attaches its security schemes as optional, the docs state that a malformed inline key disables the gate, and a test comment no longer claims the 401 carries CORS. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BSXRLJk5knQd4rNnn7XH1S Signed-off-by: Aaron K. Clark --- SECURITY.md | 10 ++-- docs/getting-started.mdx | 22 ++++--- fern/openapi.yml | 6 ++ services/nvpair-proxy/README.md | 5 +- services/nvpair-proxy/ingress.go | 27 ++++----- services/nvpair-proxy/ingress_auth_test.go | 59 ++++++++++--------- services/nvpair-proxy/spec.md | 5 +- services/shared/ingressauth/ingressauth.go | 16 +++-- services/shared/ingressauth/owner_unix.go | 39 +++--------- .../shared/ingressauth/owner_unix_test.go | 18 +++--- 10 files changed, 98 insertions(+), 109 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 70aaf47a..20b49193 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -112,10 +112,12 @@ bits; on Windows the check is skipped and the per-user data directory's ACL is the protection), that contains a malformed entry, or that cannot be read contributes no keys. That is judged per source: a rejected file does not disable keys supplied through `NVPAIR_PROXY_API_KEYS`, and with neither source -yielding a key the LAN stays closed. The one group-readable shape the gate -accepts is a root-owned file readable (not writable) only by a group the proxy -itself belongs to, which is how Kubernetes mounts a Secret under a pod -`fsGroup`. +yielding a key the LAN stays closed. Group read is refused even for a group the +proxy belongs to, since the gate cannot tell a group made for one workload from +a shared one; a Kubernetes Secret mounted under a pod `fsGroup` is therefore +refused, and the key belongs in `NVPAIR_PROXY_API_KEYS` instead. A preflight +gets no exemption: a keyless `OPTIONS` is refused like any other request, so a +browser, which cannot send a key on a preflight, is not a supported LAN client. Enabling the gate is logged at warning level at startup and whenever the key set changes. It adds no TLS, rate limiting, or per-key permissions: the plaintext personality stays plaintext, so use it only on a network you trust, and pair it diff --git a/docs/getting-started.mdx b/docs/getting-started.mdx index e6771fa5..e0b7e244 100644 --- a/docs/getting-started.mdx +++ b/docs/getting-started.mdx @@ -499,16 +499,22 @@ variables refine the behavior for headless or containerized nodes: `NVPAIR_PROXY_API_KEYS_FILE` names a different key file, `NVPAIR_PROXY_API_KEYS` supplies keys inline (comma-separated), and `NVPAIR_PROXY_ALLOWED_CIDRS` (for example `192.168.1.0/24,10.0.0.0/8`) additionally restricts which source networks -may use a key at all. The file and inline keys are separate sources: a key -file that is refused contributes no keys, but inline keys still work. +may use a key at all. A key file that is refused contributes no keys but +leaves inline keys working. The reverse does not hold: a malformed inline key or +allowlist entry disables authenticated access entirely until it is corrected. On Kubernetes, supply the key through `NVPAIR_PROXY_API_KEYS` from a Secret -with `valueFrom.secretKeyRef`. A Secret mounted as a file and pointed to by -`NVPAIR_PROXY_API_KEYS_FILE` also works under a pod `fsGroup`, which mounts it -root-owned and group-readable (`0440`): the proxy accepts group read on a -root-owned file whose group it belongs to. Set the allowlist to the pod -network's CIDR: on OVN-Kubernetes, a pod reaching a `hostNetwork` node on the same -host arrives from its pod IP, not the node's address. +with `valueFrom.secretKeyRef`. Do not mount the Secret as a key file: under a +pod `fsGroup` it arrives group-readable, and the proxy refuses any key file a +group can read, because it cannot tell a group made for one workload from a +shared one. Set the allowlist to the pod network's CIDR: on OVN-Kubernetes, a +pod reaching a `hostNetwork` node on the same host arrives from its pod IP, not +the node's address. + +The endpoints serve applications, not web pages. A browser sends no key on the +CORS preflight that precedes an authenticated request, and a keyless request +from another machine is refused, so a web page on another machine cannot use +this access. Understand what you are enabling. The connection is plain HTTP, so prompts and responses cross your network unencrypted, and anyone holding the key can use the diff --git a/fern/openapi.yml b/fern/openapi.yml index 2fc0d088..3fee8326 100644 --- a/fern/openapi.yml +++ b/fern/openapi.yml @@ -34,6 +34,12 @@ tags: description: OpenAI-compatible inference proxied to an eligible node. - name: Node info description: Read-only node telemetry polled from discovered PAIR nodes. +# A credential is optional: loopback callers send none, and a caller on another +# machine needs one only when the node's operator has enabled LAN access. +security: + - {} + - bearerAuth: [] + - apiKeyAuth: [] paths: /api/chat: post: diff --git a/services/nvpair-proxy/README.md b/services/nvpair-proxy/README.md index c36fb0c2..6757299f 100644 --- a/services/nvpair-proxy/README.md +++ b/services/nvpair-proxy/README.md @@ -125,7 +125,10 @@ actionable warning while the primary listener stays available. **Cluster ingress.** The listener carries two personalities, demultiplexed by each connection's first byte. Plaintext HTTP is accepted only from loopback; a -LAN caller is refused. When `--cluster-dir` shows this node is a cluster member, +LAN caller is refused unless the operator has configured API keys +(`NVPAIR_PROXY_API_KEYS_FILE`, `NVPAIR_PROXY_API_KEYS`, optionally narrowed by +`NVPAIR_PROXY_ALLOWED_CIDRS`) and the caller presents one, in which case it is +routed like a loopback client with the key stripped. When `--cluster-dir` shows this node is a cluster member, the same listener also terminates cluster mTLS: a peer whose client certificate matches one of this node's pins is forwarded straight to the local engine reported by `node/set-local-backend`, and is never re-routed onward to another diff --git a/services/nvpair-proxy/ingress.go b/services/nvpair-proxy/ingress.go index 271d1ada..7641ff8e 100644 --- a/services/nvpair-proxy/ingress.go +++ b/services/nvpair-proxy/ingress.go @@ -13,7 +13,6 @@ import ( "net/url" "strconv" - "nvpair-shared/cors" "nvpair-shared/ingressauth" ) @@ -91,9 +90,8 @@ func (f *facade) handlePlain(w http.ResponseWriter, r *http.Request) { if p := f.host.lanAuth; p != nil { d = p.Authorize(r) } - // A source outside the operator's allowlist gets nothing — not even a - // CORS-answered preflight — so the allowlist means what it says for - // OPTIONS too. + // A source outside the operator's allowlist gets nothing, and its key is + // never examined. if d.Enabled && d.Code == ingressauth.CodeSourceNotAllowed { slog.Warn("rejected non-loopback plaintext request", "remote", r.RemoteAddr, "method", r.Method, "path", r.URL.Path, "code", d.Code) @@ -107,13 +105,12 @@ func (f *facade) handlePlain(w http.ResponseWriter, r *http.Request) { "plaintext requests are accepted only from loopback; cluster peers must use the mTLS ingress") return } - // A browser sends no Authorization on a preflight, so a non-loopback - // preflight skips the credential check and continues into handleHTTP, - // which answers it from the engine's own CORS policy exactly as for a - // loopback caller. It grants no access on its own: the request that - // follows still receives the real 401/403. - preflight := cors.IsPreflight(r) - if !preflight && !d.Allowed { + // A preflight is judged like any other request. Answering a keyless one + // would hand it to handleHTTP, which buffers the body, fans the OPTIONS + // out to every cluster candidate, and relays an engine's raw reply: none + // of that is for a caller who has shown no key. A browser, which cannot + // send a key on a preflight, is therefore not a supported LAN client. + if !d.Allowed { slog.Warn("rejected non-loopback plaintext request", "remote", r.RemoteAddr, "method", r.Method, "path", r.URL.Path, "code", d.Code, "key_fp", d.KeyFingerprint) if d.Challenge != "" { @@ -124,11 +121,9 @@ func (f *facade) handlePlain(w http.ResponseWriter, r *http.Request) { } // The key is the proxy's credential, not the engine's: never forward it. f.host.lanAuth.StripCredential(r.Header) - if !preflight { - // At Info, not Debug: a production log must show who used a key. - slog.Info("authenticated non-loopback plaintext request", "remote", r.RemoteAddr, - "method", r.Method, "path", r.URL.Path, "key_fp", d.KeyFingerprint) - } + // At Info, not Debug: a production log must show who used a key. + slog.Info("authenticated non-loopback plaintext request", "remote", r.RemoteAddr, + "method", r.Method, "path", r.URL.Path, "key_fp", d.KeyFingerprint) } // Engine-manager marks its private identity/action requests so this // compatibility facade can never be mistaken for the local Ollama backend. diff --git a/services/nvpair-proxy/ingress_auth_test.go b/services/nvpair-proxy/ingress_auth_test.go index f5e9d47e..20422181 100644 --- a/services/nvpair-proxy/ingress_auth_test.go +++ b/services/nvpair-proxy/ingress_auth_test.go @@ -5,6 +5,7 @@ package main import ( "encoding/json" + "io" "net/http" "net/http/httptest" "net/netip" @@ -107,8 +108,8 @@ func TestHandlePlainXApiKeyAccepted(t *testing.T) { } // TestHandlePlainNonLoopbackWithoutKeyIs401: an enabled gate turns the LAN -// refusal from 403 loopback-only into 401 with a challenge, still carrying CORS -// so a browser can read it, and nothing is forwarded. +// refusal from 403 loopback-only into 401 with a challenge, and nothing is +// forwarded. The refusal grants no CORS: a browser is not a supported LAN client. func TestHandlePlainNonLoopbackWithoutKeyIs401(t *testing.T) { f, seen := lanProxy(t) rec := httptest.NewRecorder() @@ -206,25 +207,31 @@ func TestHandlePlainInsideAllowedCIDRIsRouted(t *testing.T) { } } -// TestHandlePlainPreflightReachesEngineWhenEnabled: a browser sends no -// Authorization on a preflight, so once the gate is on a keyless LAN preflight -// must not be refused with 401; it is answered from the engine's own CORS -// policy, as a loopback preflight is. -func TestHandlePlainPreflightReachesEngineWhenEnabled(t *testing.T) { +// TestHandlePlainKeylessPreflightIs401: a preflight gets no exemption. A keyless +// LAN preflight is refused like any keyless request, reaches no engine, and has +// its body left unread. +func TestHandlePlainKeylessPreflightIs401(t *testing.T) { f, seen := lanProxy(t) + body := &countingReader{r: strings.NewReader(strings.Repeat("x", 1<<16))} rec := httptest.NewRecorder() - req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) + req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", body) req.RemoteAddr = lanRemote req.Header.Set("Origin", "http://app.test") req.Header.Set("Access-Control-Request-Method", "POST") req.Header.Set("Access-Control-Request-Headers", "Authorization") f.handlePlain(rec, req) - if rec.Code == http.StatusUnauthorized || rec.Code == http.StatusForbidden { - t.Fatalf("LAN preflight status = %d, want the engine's answer", rec.Code) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("keyless LAN preflight status = %d, want 401", rec.Code) } - if seen.Load() == nil { - t.Fatal("the preflight never reached the engine's CORS policy") + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q on a refused preflight, want none", got) + } + if seen.Load() != nil { + t.Fatal("a keyless preflight reached the engine") + } + if n := body.n.Load(); n != 0 { + t.Fatalf("proxy read %d bytes of a keyless preflight body, want 0", n) } } @@ -248,7 +255,7 @@ func TestHandlePlainGateWithoutKeysKeepsLoopbackOnly(t *testing.T) { } // TestHandlePlainPreflightOutsideAllowedCIDRIs403: the allowlist applies to a -// preflight too. A source the operator excluded gets no CORS answer that would +// preflight too. A source the operator excluded gets no answer that would // let a browser proceed to the request that follows. func TestHandlePlainPreflightOutsideAllowedCIDRIs403(t *testing.T) { f, seen := lanProxy(t, netip.MustParsePrefix("10.0.0.0/8")) @@ -270,20 +277,14 @@ func TestHandlePlainPreflightOutsideAllowedCIDRIs403(t *testing.T) { } } -// TestHandlePlainPreflightInsideAllowedCIDRReachesEngine: inside the allowlist -// the preflight is still answered without a credential, as browsers require. -func TestHandlePlainPreflightInsideAllowedCIDRReachesEngine(t *testing.T) { - f, seen := lanProxy(t, netip.MustParsePrefix("192.0.2.0/24")) - rec := httptest.NewRecorder() - req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) - req.RemoteAddr = lanRemote - req.Header.Set("Origin", "http://app.test") - req.Header.Set("Access-Control-Request-Method", "POST") - f.handlePlain(rec, req) - if rec.Code == http.StatusUnauthorized || rec.Code == http.StatusForbidden { - t.Fatalf("in-allowlist preflight status = %d, want the engine's answer", rec.Code) - } - if seen.Load() == nil { - t.Fatal("the preflight never reached the engine's CORS policy") - } +// countingReader records how many bytes the proxy pulled from the client. +type countingReader struct { + r io.Reader + n atomic.Int64 +} + +func (c *countingReader) Read(b []byte) (int, error) { + n, err := c.r.Read(b) + c.n.Add(int64(n)) + return n, err } diff --git a/services/nvpair-proxy/spec.md b/services/nvpair-proxy/spec.md index 18c59999..0dd32209 100644 --- a/services/nvpair-proxy/spec.md +++ b/services/nvpair-proxy/spec.md @@ -534,7 +534,10 @@ a redelivered snapshot must not clear live reservations. One port per facade, demultiplexed on the connection's first byte: -- **Loopback plaintext** for local clients. A LAN caller is refused. +- **Loopback plaintext** for local clients. A LAN caller is refused unless the + operator has enabled the API-key gate (`nvpair-shared/ingressauth`) and the + caller presents a configured key; a preflight gets no exemption. An admitted + caller is routed like a loopback client, with the key stripped first. - **Cluster mTLS** when `--cluster-dir` shows this node is a member: a peer whose client certificate matches a local pin is forwarded straight to the local engine reported by `node/set-local-backend`, never re-routed onward. diff --git a/services/shared/ingressauth/ingressauth.go b/services/shared/ingressauth/ingressauth.go index fbb8a5e5..9b040863 100644 --- a/services/shared/ingressauth/ingressauth.go +++ b/services/shared/ingressauth/ingressauth.go @@ -1,13 +1,12 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -// Package ingressauth is the opt-in credential gate the inference proxies apply -// to a plaintext request that did not arrive from loopback. Both proxies share -// it for the same reason they share nvpair-shared/cors: the two must accept and -// refuse an outside caller identically, and one implementation is what keeps -// them from drifting. +// Package ingressauth is the opt-in credential gate nvpair-proxy applies to a +// plaintext request that did not arrive from loopback. The proxy holds one +// gate and every engine facade consults it, so each accepts and refuses an +// outside caller identically. // -// With nothing configured the gate is disabled and the proxies keep their +// With nothing configured the gate is disabled and the proxy keeps its // loopback-only behavior — a LAN caller is refused before this package is // consulted. An operator enables it by configuring at least one API key, either // in a key file (NVPAIR_PROXY_API_KEYS_FILE, default /proxy-api-keys) or @@ -264,9 +263,8 @@ func (g *Gate) enabledLocked() bool { // refreshes the key file once and answers from that single view, so the // enabled/disabled state and the key set a request is judged against cannot // change between two calls. The allowlist is checked before the credential, -// so a caller outside it learns nothing about whether its key is valid — and -// the proxy applies that source decision even to a preflight, which needs no -// credential. Authorize does not write to the response; the proxy does, in +// so a caller outside it learns nothing about whether its key is valid. A +// preflight is judged like any other request. Authorize does not write to the response; the proxy does, in // its own error format. func (g *Gate) Authorize(r *http.Request) Decision { g.mu.Lock() diff --git a/services/shared/ingressauth/owner_unix.go b/services/shared/ingressauth/owner_unix.go index 8be3d39b..6834e140 100644 --- a/services/shared/ingressauth/owner_unix.go +++ b/services/shared/ingressauth/owner_unix.go @@ -10,7 +10,6 @@ import ( "fmt" "io/fs" "os" - "slices" "syscall" ) @@ -18,45 +17,25 @@ import ( // change, the way sshd treats authorized_keys: a private mode is not enough if // someone else owns the file and can change its contents or mode at will. The // file must belong to the proxy's user or to root, since an administrator may -// provision it for a service user, and must grant nothing to others. A -// FileInfo without Unix ownership data is refused too: on a Unix-like system +// provision it for a service user, and must grant nothing to group or others. +// Group read is refused even for a group the proxy belongs to: the gate cannot +// tell a group made for one workload from a shared one such as macOS "staff". +// A FileInfo without Unix ownership data is refused too: on a Unix-like system // that is not a file the gate can vouch for. -// -// Group read is refused except in one shape: a root-owned file, read-only to a -// group the proxy itself belongs to (0440 or 0640). That is how Kubernetes -// mounts a Secret under a pod fsGroup, where the group exists for this -// workload alone. The file must be root's so a user cannot open their own key -// to a shared login group, such as macOS "staff", on the proxy's behalf. func checkKeyFileAccess(info fs.FileInfo) error { - return checkKeyFileAccessAs(info, os.Geteuid(), processGroups()) + return checkKeyFileAccessAs(info, os.Geteuid()) } -func checkKeyFileAccessAs(info fs.FileInfo, euid int, groups []int) error { +func checkKeyFileAccessAs(info fs.FileInfo, euid int) error { st, ok := info.Sys().(*syscall.Stat_t) if !ok { return errors.New("cannot determine the key file's owner") } - uid, gid := int(st.Uid), int(st.Gid) - if uid != euid && uid != 0 { + if uid := int(st.Uid); uid != euid && uid != 0 { return fmt.Errorf("owned by uid %d, not by the proxy's user (uid %d)", uid, euid) } - perm := info.Mode().Perm() - switch { - case perm&0o077 == 0: - return nil - case perm&0o037 == 0 && uid == 0 && slices.Contains(groups, gid): - return nil - case perm&0o007 != 0: + if perm := info.Mode().Perm(); perm&0o077 != 0 { return fmt.Errorf("permissions %04o allow other users to read it; chmod 600", perm) - default: - return fmt.Errorf("permissions %04o allow group %d to access it; chmod 600, or make it root-owned, group read-only (0440), and in a group the proxy belongs to", perm, gid) } -} - -// processGroups is the proxy's effective and supplementary group IDs. A -// failure to list the supplementary ones leaves just the effective group, -// which only narrows what checkKeyFileAccess accepts. -func processGroups() []int { - groups, _ := os.Getgroups() - return append(groups, os.Getegid()) + return nil } diff --git a/services/shared/ingressauth/owner_unix_test.go b/services/shared/ingressauth/owner_unix_test.go index e03d38bc..12989e56 100644 --- a/services/shared/ingressauth/owner_unix_test.go +++ b/services/shared/ingressauth/owner_unix_test.go @@ -15,7 +15,7 @@ import ( // fakeInfo is a fs.FileInfo whose mode and Sys() the test controls, so // ownership cases that would otherwise need root (a file owned by someone -// else, or by root in a Kubernetes fsGroup) can be exercised. +// else, or a root-owned group-readable Secret mount) can be exercised. type fakeInfo struct { fs.FileInfo mode fs.FileMode @@ -38,8 +38,7 @@ func TestCheckKeyFileAccess(t *testing.T) { t.Fatalf("a file this process just created is refused: %v", err) } - const me, other, fsGroup, shared = 1000, 1001, 2000, 20 - groups := []int{shared, fsGroup} + const me, other, group = 1000, 1001, 2000 cases := []struct { name string mode fs.FileMode @@ -50,19 +49,16 @@ func TestCheckKeyFileAccess(t *testing.T) { {"owned by root", 0o400, &syscall.Stat_t{Uid: 0}, true}, {"owned by another user", 0o600, &syscall.Stat_t{Uid: other}, false}, {"world-readable", 0o604, &syscall.Stat_t{Uid: me}, false}, - {"Kubernetes Secret under fsGroup, 0440", 0o440, &syscall.Stat_t{Uid: 0, Gid: fsGroup}, true}, - {"Kubernetes Secret under fsGroup, 0640", 0o640, &syscall.Stat_t{Uid: 0, Gid: fsGroup}, true}, - {"root-owned, group not the proxy's", 0o440, &syscall.Stat_t{Uid: 0, Gid: 3000}, false}, - {"root-owned, group-writable", 0o460, &syscall.Stat_t{Uid: 0, Gid: fsGroup}, false}, - {"root-owned, group-executable", 0o450, &syscall.Stat_t{Uid: 0, Gid: fsGroup}, false}, - {"root-owned, group-readable and world-readable", 0o444, &syscall.Stat_t{Uid: 0, Gid: fsGroup}, false}, - {"user-owned, group-readable to a shared group", 0o640, &syscall.Stat_t{Uid: me, Gid: shared}, false}, + {"group-readable", 0o640, &syscall.Stat_t{Uid: me, Gid: group}, false}, + // A Kubernetes Secret under a pod fsGroup: the group might be the + // workload's alone or a shared one; the gate cannot tell, so refuses. + {"root-owned, group-readable", 0o440, &syscall.Stat_t{Uid: 0, Gid: group}, false}, {"no ownership data", 0o600, nil, false}, {"foreign Sys type", 0o600, struct{}{}, false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - err := checkKeyFileAccessAs(fakeInfo{FileInfo: real, mode: tc.mode, sys: tc.sys}, me, groups) + err := checkKeyFileAccessAs(fakeInfo{FileInfo: real, mode: tc.mode, sys: tc.sys}, me) if (err == nil) != tc.ok { t.Fatalf("checkKeyFileAccessAs err = %v, want ok=%v", err, tc.ok) } From a76b22ba444a8c8d10024a1dda3c7b5482499f82 Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 22 Sep 2026 21:44:35 -0500 Subject: [PATCH 6/6] Scope the OpenAPI credential to the proxy operations; test a keyed preflight From the verification round on e65ea8a, which found both fixes closed and no regression: - The optional credential moves from the document root onto the two proxy operations. /v1/node-info is served by nvpair-node-info, which has no key gate. - A preflight that carries a valid key is tested: routed like any authenticated request, with the key stripped. - The key-file refusal names group access and points at NVPAIR_PROXY_API_KEYS, and the package doc, SECURITY.md and the troubleshooting page say group-readable rather than only other-readable. SECURITY.md reconciles "prefer the file" with the Kubernetes guidance, and troubleshooting lists a malformed inline key or allowlist entry as a cause of 403 loopback-only. - The proxy README scopes its preflight description to loopback callers. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BSXRLJk5knQd4rNnn7XH1S Signed-off-by: Aaron K. Clark --- SECURITY.md | 6 +++-- docs/troubleshooting.mdx | 7 ++++-- fern/openapi.yml | 18 ++++++++++----- services/nvpair-proxy/README.md | 2 +- services/nvpair-proxy/ingress_auth_test.go | 26 +++++++++++++++++++++- services/shared/ingressauth/ingressauth.go | 8 +++---- services/shared/ingressauth/owner_unix.go | 2 +- 7 files changed, 52 insertions(+), 17 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 20b49193..7432456b 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -106,8 +106,10 @@ run on a different host so that PAIR sees its real address. The gate compares keys in constant time, holds file keys in memory only as digests (a key supplied inline through the environment also remains in the -process environment in clear, so prefer the file), never logs a presented key -(a rejection logs a short digest fingerprint), and fails closed: a key file that other users can read (judged by Unix permission +process environment in clear, so prefer the file where the proxy's own user or +root can own it privately; on Kubernetes that is not possible without a +group-readable mount, so use the variable there), never logs a presented key +(a rejection logs a short digest fingerprint), and fails closed: a key file that its group or other users can read (judged by Unix permission bits; on Windows the check is skipped and the per-user data directory's ACL is the protection), that contains a malformed entry, or that cannot be read contributes no keys. That is judged per source: a rejected file does not diff --git a/docs/troubleshooting.mdx b/docs/troubleshooting.mdx index ab44b5e7..315d958b 100644 --- a/docs/troubleshooting.mdx +++ b/docs/troubleshooting.mdx @@ -104,8 +104,11 @@ authenticated access with an API key; refer to The refusal then says what is missing: - `403` `loopback-only`: the node has no usable key configured. Check the node's - log; a key file that other users can read, or that contains a malformed entry, - is ignored and the reason is logged. + log; a key file that its group or other users can read, or that contains a + malformed entry, + is ignored and the reason is logged. A malformed `NVPAIR_PROXY_API_KEYS` or + `NVPAIR_PROXY_ALLOWED_CIDRS` entry turns authenticated access off entirely + until it is corrected. - `401` `unauthorized`: the request carried no key, or a key the node does not have. Send it as `Authorization: Bearer ` (or `X-Api-Key: `). The node's log shows the first eight hex digits of the SHA-256 of the key it diff --git a/fern/openapi.yml b/fern/openapi.yml index 3fee8326..8a67e07c 100644 --- a/fern/openapi.yml +++ b/fern/openapi.yml @@ -34,18 +34,18 @@ tags: description: OpenAI-compatible inference proxied to an eligible node. - name: Node info description: Read-only node telemetry polled from discovered PAIR nodes. -# A credential is optional: loopback callers send none, and a caller on another -# machine needs one only when the node's operator has enabled LAN access. -security: - - {} - - bearerAuth: [] - - apiKeyAuth: [] paths: /api/chat: post: tags: - Ollama-compatible operationId: ollamaChat + # Optional: loopback callers send no credential; a caller on another + # machine needs one only when the operator has enabled LAN access. + security: + - {} + - bearerAuth: [] + - apiKeyAuth: [] summary: Ollama-compatible chat description: > Send an Ollama-compatible chat request to the local PAIR proxy. PAIR @@ -100,6 +100,12 @@ paths: tags: - OpenAI-compatible operationId: createChatCompletion + # Optional: loopback callers send no credential; a caller on another + # machine needs one only when the operator has enabled LAN access. + security: + - {} + - bearerAuth: [] + - apiKeyAuth: [] summary: OpenAI-compatible chat completion description: > Send an OpenAI-compatible chat completion request to the local PAIR diff --git a/services/nvpair-proxy/README.md b/services/nvpair-proxy/README.md index 6757299f..e2746da4 100644 --- a/services/nvpair-proxy/README.md +++ b/services/nvpair-proxy/README.md @@ -151,7 +151,7 @@ sit on the engine's own port. The stored value predates the current default. **Browser clients (CORS).** PAIR does not enable CORS or add default browser permissions. Configure origins through the engine; its built-in defaults and user configuration remain authoritative. Ordinary forwarded responses preserve the upstream status, body, and CORS headers, including missing headers. A denial is never replaced with a successful OPTIONS response or retried to find permission elsewhere. Proxy-generated errors carry their actual status without CORS permission headers, so browser JavaScript may see a generic CORS failure while curl and diagnostics show the real error. -A browser preflight (OPTIONS with Origin and Access-Control-Request-Method) queries every currently routable target, with concurrency eight, a ten-second query deadline, and no redirects. A single target's response is relayed. Multiple responding targets must all permit the requested origin, method, and headers; PAIR grants only their shared permissions. Credentials require unanimous explicit support. Synthesized preflights allow browsers to cache the result for 60 seconds; PAIR itself does not cache decisions. A policy denial returns 403. Unavailable targets are skipped; if none can answer, the proxy returns 502. Ordinary OPTIONS requests retain normal routing. Preflights do not create inference jobs or reserve scheduler capacity. Paired ingress forwards only to its local engine. +A browser preflight (OPTIONS with Origin and Access-Control-Request-Method) from a loopback caller queries every currently routable target, with concurrency eight, a ten-second query deadline, and no redirects. A single target's response is relayed. Multiple responding targets must all permit the requested origin, method, and headers; PAIR grants only their shared permissions. Credentials require unanimous explicit support. Synthesized preflights allow browsers to cache the result for 60 seconds; PAIR itself does not cache decisions. A policy denial returns 403. Unavailable targets are skipped; if none can answer, the proxy returns 502. Ordinary OPTIONS requests retain normal routing. Preflights do not create inference jobs or reserve scheduler capacity. Paired ingress forwards only to its local engine. A preflight from another machine gets no exemption from the API-key gate: without a key it is refused with 401 before any target is queried, so a web page on another machine is not a supported client. Combined model lists forward the caller's origin and end-to-end headers, excluding Authorization and Cookie so credentials are not shared across engines. Multi-target preflights apply the same credential filtering. With an Origin header, every responding engine must return a valid list and permit sharing: one denial returns 403 and one invalid list returns 502, without partial inventory. An invalid-list error retains the combined CORS permissions when every responding engine allows the origin. Unavailable engines are skipped, with 502 returned when none can answer. Successful lists combine origin/credential permissions and Vary requirements. Requests without Origin retain partial aggregation when some inventories are unavailable. Engines without CORS support remain unavailable to cross-origin browser clients through PAIR. diff --git a/services/nvpair-proxy/ingress_auth_test.go b/services/nvpair-proxy/ingress_auth_test.go index 20422181..edfa2bf5 100644 --- a/services/nvpair-proxy/ingress_auth_test.go +++ b/services/nvpair-proxy/ingress_auth_test.go @@ -109,7 +109,7 @@ func TestHandlePlainXApiKeyAccepted(t *testing.T) { // TestHandlePlainNonLoopbackWithoutKeyIs401: an enabled gate turns the LAN // refusal from 403 loopback-only into 401 with a challenge, and nothing is -// forwarded. The refusal grants no CORS: a browser is not a supported LAN client. +// forwarded. func TestHandlePlainNonLoopbackWithoutKeyIs401(t *testing.T) { f, seen := lanProxy(t) rec := httptest.NewRecorder() @@ -288,3 +288,27 @@ func (c *countingReader) Read(b []byte) (int, error) { c.n.Add(int64(n)) return n, err } + +// TestHandlePlainKeyedPreflightIsRouted: a preflight that does carry a valid key +// is routed like any authenticated request, with the key stripped. +func TestHandlePlainKeyedPreflightIsRouted(t *testing.T) { + f, seen := lanProxy(t) + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/v1/chat/completions", nil) + req.RemoteAddr = lanRemote + req.Header.Set("Origin", "http://app.test") + req.Header.Set("Access-Control-Request-Method", "POST") + req.Header.Set("Authorization", "Bearer "+lanKey) + f.handlePlain(rec, req) + + if rec.Code == http.StatusUnauthorized || rec.Code == http.StatusForbidden { + t.Fatalf("keyed LAN preflight status = %d, want it routed", rec.Code) + } + h := seen.Load() + if h == nil { + t.Fatal("a keyed preflight never reached the engine") + } + if got := h.Get("Authorization"); got != "" { + t.Errorf("engine saw Authorization %q, want it stripped", got) + } +} diff --git a/services/shared/ingressauth/ingressauth.go b/services/shared/ingressauth/ingressauth.go index 9b040863..a32d577a 100644 --- a/services/shared/ingressauth/ingressauth.go +++ b/services/shared/ingressauth/ingressauth.go @@ -17,8 +17,8 @@ // Loopback callers are never asked for a key — the desktop application, the // terminal interface, and local tools are unaffected by enabling the gate. // -// Every failure fails closed. A key file that cannot be read, is readable by -// other users, or contains an entry that could never match over the wire +// Every failure fails closed. A key file that cannot be read, is accessible to +// its group or other users, or contains an entry that could never match over the wire // contributes no keys, the reason is logged, and the LAN stays closed. Keys are // held in memory only as SHA-256 digests and are compared in constant time; a // rejected credential is logged as a short digest fingerprint, never as itself. @@ -264,8 +264,8 @@ func (g *Gate) enabledLocked() bool { // enabled/disabled state and the key set a request is judged against cannot // change between two calls. The allowlist is checked before the credential, // so a caller outside it learns nothing about whether its key is valid. A -// preflight is judged like any other request. Authorize does not write to the response; the proxy does, in -// its own error format. +// preflight is judged like any other request. Authorize does not write to the +// response; the proxy does, in its own error format. func (g *Gate) Authorize(r *http.Request) Decision { g.mu.Lock() g.refreshLocked() diff --git a/services/shared/ingressauth/owner_unix.go b/services/shared/ingressauth/owner_unix.go index 6834e140..7744d910 100644 --- a/services/shared/ingressauth/owner_unix.go +++ b/services/shared/ingressauth/owner_unix.go @@ -35,7 +35,7 @@ func checkKeyFileAccessAs(info fs.FileInfo, euid int) error { return fmt.Errorf("owned by uid %d, not by the proxy's user (uid %d)", uid, euid) } if perm := info.Mode().Perm(); perm&0o077 != 0 { - return fmt.Errorf("permissions %04o allow other users to read it; chmod 600", perm) + return fmt.Errorf("permissions %04o grant access to its group or other users; chmod 600, or supply the key through NVPAIR_PROXY_API_KEYS", perm) } return nil }