diff --git a/internal/agent/engines/engine.go b/internal/agent/engines/engine.go index 0e582c46..6bc5f712 100644 --- a/internal/agent/engines/engine.go +++ b/internal/agent/engines/engine.go @@ -249,6 +249,11 @@ func baseEnv(s ShardSpec) map[string]string { // Profile overrides win: they are the escape hatch for the per-cluster // tuning no adapter can guess. for k, v := range s.Profile.Env { + // Checked again here, not only where the model was defined: this is + // the process about to run as root, whatever sent the profile. + if bad, _ := UnsafeEnv(k); bad { + continue + } env[k] = v } return env diff --git a/internal/agent/engines/unsafeenv.go b/internal/agent/engines/unsafeenv.go new file mode 100644 index 00000000..f60bc53e --- /dev/null +++ b/internal/agent/engines/unsafeenv.go @@ -0,0 +1,30 @@ +package engines + +import "strings" + +// UnsafeEnv reports whether a profile may not set an environment variable, +// and why. +// +// A profile's env is the escape hatch for tuning no adapter can guess — +// CUDA_*, NCCL_*, VLLM_*, HF_* — and it reaches an engine the agent starts as +// root. Some variables are not tuning but a way to run other code in that +// process: the dynamic loader's LD_PRELOAD, Python's PYTHONSTARTUP, a shell's +// BASH_ENV. Set from a model definition, which an admin API or a config file +// supplies, they would turn "add a model" into "run this as root on every +// node". They are refused where a model is defined and again where an engine +// starts. +func UnsafeEnv(key string) (bool, string) { + k := strings.ToUpper(key) + switch { + case strings.HasPrefix(k, "LD_"), strings.HasPrefix(k, "DYLD_"): + return true, "it changes what the dynamic loader loads" + case k == "PATH": + return true, "it changes which binaries run" + case strings.HasPrefix(k, "PYTHON") && k != "PYTHONUNBUFFERED" && k != "PYTHONHASHSEED": + return true, "it changes what Python imports or runs at start" + case k == "BASH_ENV" || k == "ENV" || k == "PERL5OPT" || k == "PERL5LIB" || + k == "NODE_OPTIONS" || k == "RUBYOPT" || k == "GCONV_PATH" || k == "OPENSSL_CONF": + return true, "it makes an interpreter or library load code" + } + return false, "" +} diff --git a/internal/cli/ctl.go b/internal/cli/ctl.go index 4ebecea6..158056cf 100644 --- a/internal/cli/ctl.go +++ b/internal/cli/ctl.go @@ -67,7 +67,9 @@ func (c *ctlClient) do(ctx context.Context, method, path string, body, out any) if err != nil { return err } - if body != nil { + // On every change, body or not: the admin listener refuses anything else, + // because a browser can send a bodiless POST across origins unasked. + if method != http.MethodGet { req.Header.Set("Content-Type", "application/json") } // Who made the change, kept with a stored model so a catalog that grew diff --git a/internal/cli/server.go b/internal/cli/server.go index 82a0f3a8..927fb062 100644 --- a/internal/cli/server.go +++ b/internal/cli/server.go @@ -246,7 +246,7 @@ func newServerCmd() *cobra.Command { // enforces its own per-phase deadlines. } adminSrv := &http.Server{ - Handler: adminMux, + Handler: router.AdminGuard(adminMux), ReadHeaderTimeout: 10 * time.Second, } diff --git a/internal/control/catalog/catalog.go b/internal/control/catalog/catalog.go index 15afa763..b64277c8 100644 --- a/internal/control/catalog/catalog.go +++ b/internal/control/catalog/catalog.go @@ -302,6 +302,13 @@ func (m Model) validate() error { } seen[p.Name] = struct{}{} + for k := range p.Env { + if bad, why := engines.UnsafeEnv(k); bad { + return fmt.Errorf("model %q profile %q: env %s is not allowed: %s", + m.Name, p.Name, k, why) + } + } + if p.GPUClass == "" && !cpuCapable(p.Engine) { return fmt.Errorf("model %q profile %q: gpu_class is required for engine %q", m.Name, p.Name, p.Engine) diff --git a/internal/control/catalog/catalog_test.go b/internal/control/catalog/catalog_test.go index eafec55d..619a1c6d 100644 --- a/internal/control/catalog/catalog_test.go +++ b/internal/control/catalog/catalog_test.go @@ -302,3 +302,22 @@ func TestSimulatedNeedsEveryProfileToBeSimulated(t *testing.T) { }) } } + +func TestAProfileCannotSetWhatRunsCodeInTheEngine(t *testing.T) { + // A profile's env reaches an engine started as root. LD_PRELOAD from a + // model definition would make "add a model" into "run this everywhere". + for _, k := range []string{"LD_PRELOAD", "ld_library_path", "PYTHONSTARTUP", "PATH", "BASH_ENV"} { + m := Model{Name: "m", Source: "http://x/m.gguf", Profiles: []Profile{{ + Name: "cpu", Engine: "llamacpp", Env: map[string]string{k: "/tmp/x"}, + }}} + if _, err := New([]Model{m}); err == nil || !strings.Contains(err.Error(), "not allowed") { + t.Errorf("%s: %v", k, err) + } + } + ok := Model{Name: "m", Source: "http://x/m.gguf", Profiles: []Profile{{ + Name: "cpu", Engine: "llamacpp", Env: map[string]string{"OMP_NUM_THREADS": "4", "PYTHONUNBUFFERED": "1"}, + }}} + if _, err := New([]Model{ok}); err != nil { + t.Errorf("ordinary tuning refused: %v", err) + } +} diff --git a/internal/router/adminguard.go b/internal/router/adminguard.go new file mode 100644 index 00000000..b0749554 --- /dev/null +++ b/internal/router/adminguard.go @@ -0,0 +1,75 @@ +package router + +import ( + "net" + "net/http" + "net/netip" + "net/url" + "strings" +) + +// AdminGuard keeps a browser from being used against the admin listener. +// +// The listener has no login: reaching it is meant to mean holding the machine, +// so it binds to loopback and an operator reaches it over an ssh tunnel. But a +// browser on that operator's machine reaches it too, from any page it has +// open. A page could POST a model whose profile runs as root on every node, +// or — by pointing its own domain at 127.0.0.1 — read pprof's heap, which +// holds API keys in the clear. Three checks close that without touching what +// ctl and curl do: +// +// - Host must be an address or localhost. A page that rebinds its own name +// to 127.0.0.1 still sends that name. +// - A request carrying Origin or Sec-Fetch-Site from another site is +// refused. Browsers send them; ctl and curl do not. +// - Anything but GET and HEAD must be application/json. A page can send a +// form or text/plain anywhere without asking; JSON across origins needs a +// preflight this listener never grants. +func AdminGuard(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !addressHost(r.Host) { + http.Error(w, "the admin listener answers to an address or localhost, not a name", http.StatusMisdirectedRequest) + return + } + if crossSite(r) { + http.Error(w, "cross-site request refused", http.StatusForbidden) + return + } + if r.Method != http.MethodGet && r.Method != http.MethodHead { + ct := r.Header.Get("Content-Type") + if mt, _, _ := strings.Cut(ct, ";"); strings.TrimSpace(strings.ToLower(mt)) != "application/json" { + http.Error(w, "a change to the admin API must be sent as application/json", http.StatusUnsupportedMediaType) + return + } + } + next.ServeHTTP(w, r) + }) +} + +func addressHost(hostport string) bool { + host := hostport + if h, _, err := net.SplitHostPort(hostport); err == nil { + host = h + } + host = strings.Trim(host, "[]") + if strings.EqualFold(host, "localhost") { + return true + } + _, err := netip.ParseAddr(host) + return err == nil +} + +func crossSite(r *http.Request) bool { + switch r.Header.Get("Sec-Fetch-Site") { + case "", "same-origin", "none": + default: + return true + } + if o := r.Header.Get("Origin"); o != "" { + u, err := url.Parse(o) + if err != nil || !strings.EqualFold(u.Host, r.Host) { + return true + } + } + return false +} diff --git a/internal/router/adminguard_test.go b/internal/router/adminguard_test.go new file mode 100644 index 00000000..8727435d --- /dev/null +++ b/internal/router/adminguard_test.go @@ -0,0 +1,57 @@ +package router + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestTheAdminListenerRefusesWhatABrowserPageCouldSend(t *testing.T) { + h := AdminGuard(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {})) + do := func(method, host, ct string, hdr map[string]string) int { + r := httptest.NewRequest(method, "/api/v1/models", strings.NewReader("{}")) + r.Host = host + if ct != "" { + r.Header.Set("Content-Type", ct) + } + for k, v := range hdr { + r.Header.Set(k, v) + } + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + return w.Code + } + + // What ctl and curl send. + if got := do("POST", "127.0.0.1:9090", "application/json", nil); got != 200 { + t.Errorf("ctl's request was refused: %d", got) + } + if got := do("GET", "localhost:9090", "", nil); got != 200 { + t.Errorf("a GET through an ssh tunnel was refused: %d", got) + } + if got := do("DELETE", "[::1]:9090", "application/json", nil); got != 200 { + t.Errorf("IPv6 loopback refused: %d", got) + } + + // A page posting a form, or text/plain, needs no preflight. + if got := do("POST", "127.0.0.1:9090", "text/plain", nil); got != http.StatusUnsupportedMediaType { + t.Errorf("text/plain POST accepted: %d", got) + } + if got := do("POST", "127.0.0.1:9090", "", nil); got != http.StatusUnsupportedMediaType { + t.Errorf("a bodiless POST accepted: %d", got) + } + // DNS rebinding: the page's own name, now resolving to 127.0.0.1. + if got := do("GET", "evil.example:9090", "", nil); got != http.StatusMisdirectedRequest { + t.Errorf("a rebound name was answered: %d", got) + } + // And anything a browser marks as coming from elsewhere. + if got := do("POST", "127.0.0.1:9090", "application/json", + map[string]string{"Sec-Fetch-Site": "cross-site"}); got != http.StatusForbidden { + t.Errorf("cross-site fetch accepted: %d", got) + } + if got := do("POST", "127.0.0.1:9090", "application/json", + map[string]string{"Origin": "https://evil.example"}); got != http.StatusForbidden { + t.Errorf("foreign Origin accepted: %d", got) + } +}