diff --git a/cmd/llmsproxy/main.go b/cmd/llmsproxy/main.go index f241c52..519169a 100644 --- a/cmd/llmsproxy/main.go +++ b/cmd/llmsproxy/main.go @@ -73,19 +73,26 @@ func main() { } } - gw, err := gateway.New(c, c.GatewayKeys()) + gw, err := gateway.New(c) if err != nil { log.Fatalf("[llmsproxy] gateway: %v", err) } // Ops hygiene: surface the two most common footguns instead of silently // running with them. - if keys := c.GatewayKeys(); len(keys) == 0 { - log.Printf("[llmsproxy] WARNING: gateway_keys is EMPTY — without a key every request is rejected") + // + // Read the AUTHORITATIVE source. Auth uses core.ListKeys() (cfg.Keys), not + // the legacy cfg.GatewayKeys list: seedKeys copies gateway_keys into keys[] + // on first start and the YAML list is ignored for auth afterwards. Checking + // GatewayKeys() here made the warning lie — once the legacy list was empty + // (e.g. after rotating the starter key away) it reported "every request is + // rejected" while seven working keys, one of them admin, were in service. + if keys := c.ListKeys(); len(keys) == 0 { + log.Printf("[llmsproxy] WARNING: no gateway key configured — every request will be rejected. Add one in the WebUI or set gateway_keys") } else { for _, k := range keys { - if k == "sk-gw-local-0001" || k == "sk-local-0001" { - log.Printf("[llmsproxy] WARNING: gateway key %q looks like the starter/example key — rotate it before exposing the gateway", k) + if k.Key == "sk-gw-local-0001" || k.Key == "sk-local-0001" { + log.Printf("[llmsproxy] WARNING: gateway key %q looks like the starter/example key — rotate it before exposing the gateway", k.Key) } } } diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index ba38d9b..ba48b53 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -326,3 +326,144 @@ func statusOf(resp *http.Response) int { } return resp.StatusCode } + +// The startup warning about key configuration must describe the key set that +// AUTH actually uses (cfg.Keys, via core.ListKeys), not the legacy +// gateway_keys YAML list. +// +// It used to check GatewayKeys() only. seedKeys copies gateway_keys into keys[] +// on first start and the YAML list stops being consulted, so once that list was +// emptied — e.g. after rotating the starter key away — the process logged +// "every request will be rejected" while seven working keys, one of them admin, +// were in service. Observed in production: the warning appeared and that same +// key returned HTTP 200 on /v1/models. +// +// The fixture reproduces exactly that shape: gateway_keys EMPTY, with the +// working key present only in keys[]. +func TestStartupWarningReflectsRealKeysNotLegacyList(t *testing.T) { + up := newMockUpstream(t) + dir := t.TempDir() + + // Seed a real install once so keys[] gets populated from gateway_keys, then + // rewrite the file to look like a rotated install: legacy list cleared, the + // seeded key still present in keys[]. + path := writeConfig(t, dir, "127.0.0.1:0", map[string]*mockUpstream{"good": up}) + bin := buildBinary(t) + + // First run creates the sealed config with keys[] populated. + seedPort := freePort(t) + cfg1 := writeConfig(t, dir, "127.0.0.1:"+seedPort, map[string]*mockUpstream{"good": up}) + g1 := startGateway(t, bin, "127.0.0.1:"+seedPort, cfg1) + g1.stop() + + data, err := os.ReadFile(cfg1) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(data), "keys:") { + t.Fatalf("first run did not populate keys[]:\n%s", data) + } + + // Now clear the legacy gateway_keys list, keeping keys[] intact. The + // rewritten file may use different indentation, so match loosely. + cleared, n := clearGatewayKeys(string(data)) + if n == 0 { + t.Fatalf("could not find a gateway_keys entry to clear in:\n%s", data) + } + if err := os.WriteFile(cfg1, []byte(cleared), 0o644); err != nil { + t.Fatal(err) + } + + // Start again and inspect the log. The config pins its listen address, so + // rewrite that field to the port we will actually poll. + port2 := freePort(t) + rehosted, ok := replaceListen(string(cleared), "127.0.0.1:"+port2) + if !ok { + t.Fatalf("could not rewrite listen in:\n%s", cleared) + } + cfg2 := filepath.Join(dir, "config2.yaml") + if err := os.WriteFile(cfg2, []byte(rehosted), 0o644); err != nil { + t.Fatal(err) + } + g2 := startGateway(t, bin, "127.0.0.1:"+port2, cfg2) + defer g2.stop() + + // Sanity: the key really does authenticate, so the warning would be a lie. + resp, _ := g2.do("GET", "/v1/models", "", true) + if resp == nil || resp.StatusCode != http.StatusOK { + t.Fatalf("key from keys[] did not authenticate (status %v); test fixture is wrong", + resp) + } + + logs := g2.logString() + if strings.Contains(logs, "gateway_keys is EMPTY") { + t.Errorf("startup logged 'gateway_keys is EMPTY ... every request is rejected' while "+ + "a working admin key was in service — the warning must read the authoritative "+ + "key set (core.ListKeys), not the legacy YAML list.\nlog:\n%s", logs) + } + _ = path + _ = g1 +} + +func freePort(t *testing.T) string { + t.Helper() + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer l.Close() + _, port, _ := net.SplitHostPort(l.Addr().String()) + return port +} + +// stop terminates the gateway and waits, so its log buffer is complete. +func (g *gatewayUnderTest) stop() { + if g == nil || g.cmd == nil || g.cmd.Process == nil { + return + } + _ = g.cmd.Process.Kill() + _, _ = g.cmd.Process.Wait() +} + +// logString returns everything the gateway has written so far. +func (g *gatewayUnderTest) logString() string { return g.log.String() } + +// clearGatewayKeys empties the gateway_keys list in a config, tolerating any +// indentation the YAML writer chose. Returns the new text and how many entries +// it removed. +func clearGatewayKeys(src string) (string, int) { + lines := strings.Split(src, "\n") + var out []string + removed := 0 + inList := false + for _, l := range lines { + trimmed := strings.TrimSpace(l) + if strings.HasPrefix(l, "gateway_keys:") { + out = append(out, "gateway_keys: []") + inList = true + removed++ + continue + } + if inList { + if strings.HasPrefix(trimmed, "- ") { + removed++ + continue + } + inList = false + } + out = append(out, l) + } + return strings.Join(out, "\n"), removed +} + +// replaceListen rewrites the top-level listen: value. +func replaceListen(src, addr string) (string, bool) { + lines := strings.Split(src, "\n") + for i, l := range lines { + if strings.HasPrefix(l, "listen:") { + lines[i] = "listen: " + addr + return strings.Join(lines, "\n"), true + } + } + return src, false +} diff --git a/internal/gateway/gateway_test.go b/internal/gateway/gateway_test.go index a1b9656..9e5a094 100644 --- a/internal/gateway/gateway_test.go +++ b/internal/gateway/gateway_test.go @@ -58,7 +58,7 @@ func newTestGateway(t *testing.T, srcs ...config.Source) *Gateway { t.Fatalf("core: %v", err) } t.Cleanup(c.Close) - g, err := New(c, []string{"sk-test"}) + g, err := New(c) if err != nil { t.Fatalf("gateway: %v", err) } diff --git a/internal/gateway/key_quota_api_test.go b/internal/gateway/key_quota_api_test.go index 9071a25..f5e1517 100644 --- a/internal/gateway/key_quota_api_test.go +++ b/internal/gateway/key_quota_api_test.go @@ -35,7 +35,7 @@ func adminGateway(t *testing.T, keys ...config.GWKey) *Gateway { t.Fatalf("core: %v", err) } t.Cleanup(c.Close) - g, err := New(c, []string{"sk-admin"}) + g, err := New(c) if err != nil { t.Fatalf("gateway: %v", err) } diff --git a/internal/gateway/key_quota_wiring_test.go b/internal/gateway/key_quota_wiring_test.go index 9de0870..db62184 100644 --- a/internal/gateway/key_quota_wiring_test.go +++ b/internal/gateway/key_quota_wiring_test.go @@ -47,7 +47,7 @@ func quotaGateway(t *testing.T, keyA, keyB config.GWKey) (*Gateway, *upstreamCtr t.Fatalf("core: %v", err) } t.Cleanup(c.Close) - g, err := New(c, []string{keyA.Key, keyB.Key}) + g, err := New(c) if err != nil { t.Fatalf("gateway: %v", err) } @@ -323,7 +323,7 @@ func newQuotaGW(t *testing.T, upURL string, keys ...config.GWKey) *Gateway { for _, k := range keys { secrets = append(secrets, k.Key) } - g, err := New(c, secrets) + g, err := New(c) if err != nil { t.Fatalf("gateway: %v", err) } @@ -368,7 +368,7 @@ func TestOneModelsQuotaDoesNotBlockAnother(t *testing.T) { t.Fatalf("core: %v", err) } t.Cleanup(c.Close) - g, err := New(c, []string{"sk-a"}) + g, err := New(c) if err != nil { t.Fatalf("gateway: %v", err) } diff --git a/internal/gateway/server.go b/internal/gateway/server.go index 4edc466..d017245 100644 --- a/internal/gateway/server.go +++ b/internal/gateway/server.go @@ -115,7 +115,7 @@ func (g *Gateway) loginRecord(ip string, ok bool, now int64) { } } -func New(c *core.Core, gatewayKeys []string) (*Gateway, error) { +func New(c *core.Core) (*Gateway, error) { sub, err := fs.Sub(uiFS, "ui") if err != nil { return nil, err