diff --git a/internal/config/secret_config.go b/internal/config/secret_config.go index 74e66b5..71a9265 100644 --- a/internal/config/secret_config.go +++ b/internal/config/secret_config.go @@ -193,11 +193,55 @@ func (c *Config) countPlaintextSecrets() int { // with ciphertext still in place, so the unseal has to happen before the // registry (and any Save the startup path performs) sees the values. func (c *Config) NormalizeSecretsForRun(box *SecretBox) error { - c.AttachSecretBox(box) - if err := c.normalizeSecrets(box); err != nil { + hadPlaintext, err := c.UnsealSecrets(box) + if err != nil { return err } - return c.migratePlaintextSecrets() + return c.SealIfNeeded(hadPlaintext) +} + +// UnsealSecrets decrypts every sealed credential in memory and reports whether +// the config ON DISK still held plaintext (i.e. whether a sealing write is +// needed). It never writes; a failed decrypt (wrong master key) is returned so +// the process refuses to start instead of running with unusable credentials. +// +// The return value must be computed BEFORE unsealing and from the disk state, +// not from memory: after a Save the in-memory values are always plaintext, so a +// "is anything plaintext?" test run afterwards is unconditionally true and a +// caller would rewrite the file on every start. That was the actual behaviour - +// migratePlaintextSecrets() claimed to be idempotent in a comment but rewrote +// config.yaml on every boot. +// +// Callers that consume credentials (the provider registry, key seeding) must +// unseal FIRST. Seeding compares cfg.Keys[i].Key against the plaintext +// gateway_keys entries; running it while keys are still ciphertext made the +// dedupe never match, so every restart appended another copy of the same admin +// key (production accumulated four). +func (c *Config) UnsealSecrets(box *SecretBox) (bool, error) { + if box == nil { + return false, nil + } + c.AttachSecretBox(box) + hadPlaintext := c.hasPlaintextSecrets() + if err := c.normalizeSecrets(box); err != nil { + return hadPlaintext, err + } + return hadPlaintext, nil +} + +// SealIfNeeded writes the config back once if hadPlaintext reported that the +// file still held clear-text credentials. When it is false the file is left +// untouched, which is what makes startup a no-op for an already-sealed config. +func (c *Config) SealIfNeeded(hadPlaintext bool) error { + if !hadPlaintext || c.box == nil || c.Path == "" { + return nil + } + n := c.countPlaintextSecrets() + if err := c.Save(); err != nil { + return err + } + log.Printf("[config] sealed %d plaintext credential(s) in %s", n, c.Path) + return nil } // AttachSecretBox wires the encryption box into the config so Save can seal @@ -210,8 +254,9 @@ func (c *Config) AttachSecretBox(box *SecretBox) { c.box = box } func (c *Config) SecretBox() *SecretBox { return c.box } // migratePlaintextSecrets seals any credential still in the clear and writes the -// file once. It is idempotent: a config that is already sealed (or has no -// secrets) is left alone and nothing is written. +// file once. Kept for callers that attach the box themselves; it decides from +// the in-memory state, which is why UnsealSecrets + SealIfNeeded (which decide +// from the on-disk state) are preferred on the startup path. func (c *Config) migratePlaintextSecrets() error { if c.box == nil || c.Path == "" { return nil diff --git a/internal/config/secret_config_test.go b/internal/config/secret_config_test.go index b926070..cb3d025 100644 --- a/internal/config/secret_config_test.go +++ b/internal/config/secret_config_test.go @@ -119,7 +119,24 @@ sources: } } -func TestMigratePlaintextSecretsIsIdempotent(t *testing.T) { +// Sealing a config is a one-time migration: the FIRST run writes, every later +// start must leave the file alone. +// +// This test previously called migratePlaintextSecrets() twice and compared +// ModTime. That assertion was both weak and wrong: +// +// - migratePlaintextSecrets decides from IN-MEMORY state, and Save() ends by +// unsealing memory so the running process keeps working. So every call sees +// "plaintext present" and rewrites the file. Comparing ModTime hid this +// because two writes inside one filesystem timestamp tick look identical — +// the test flaked (~1 in 6) instead of failing. +// - it also exercised a function production no longer calls on the startup +// path, which now uses UnsealSecrets + SealIfNeeded (they decide from the +// ON-DISK state). +// +// So the test targets that real path and compares file CONTENT, which cannot be +// fooled by timestamp granularity. +func TestSealingIsIdempotentAcrossStarts(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "config.yaml") runtime := filepath.Join(dir, "runtime.json") @@ -132,29 +149,52 @@ func TestMigratePlaintextSecretsIsIdempotent(t *testing.T) { } cfg.AttachSecretBox(box) - if err := cfg.migratePlaintextSecrets(); err != nil { - t.Fatalf("first migrate: %v", err) + hadPlaintext, err := cfg.UnsealSecrets(box) + if err != nil { + t.Fatalf("unseal: %v", err) + } + if !hadPlaintext { + t.Fatal("a config written with a clear-text credential must report plaintext") + } + if err := cfg.SealIfNeeded(hadPlaintext); err != nil { + t.Fatalf("seal: %v", err) } first, err := os.ReadFile(path) if err != nil { t.Fatal(err) } if !strings.Contains(string(first), encPrefix) { - t.Fatal("migration did not seal the value") + t.Fatal("sealing did not encrypt the value on disk") } // In-memory must be plaintext so the running process keeps working. if cfg.Sources[0].APIKey != "sk-clear" { t.Errorf("in-memory api_key = %q, want plaintext", cfg.Sources[0].APIKey) } - // Second run: already sealed => no write. - before, _ := os.Stat(path) - if err := cfg.migratePlaintextSecrets(); err != nil { - t.Fatalf("second migrate: %v", err) + // Simulate the next start: load from disk, unseal, seal-if-needed. The file + // was already sealed, so nothing may be written. + cfg2, err := Load(path) + if err != nil { + t.Fatal(err) } - after, _ := os.Stat(path) - if before.ModTime() != after.ModTime() { - t.Error("second migrate rewrote an already-sealed config") + cfg2.AttachSecretBox(box) + hadPlaintext2, err := cfg2.UnsealSecrets(box) + if err != nil { + t.Fatalf("second unseal: %v", err) + } + if hadPlaintext2 { + t.Error("a sealed config reported plaintext — SealIfNeeded would rewrite " + + "the file on every start") + } + if err := cfg2.SealIfNeeded(hadPlaintext2); err != nil { + t.Fatalf("second seal: %v", err) + } + second, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(first) != string(second) { + t.Error("the second start rewrote an already-sealed config; startup must be a no-op") } } diff --git a/internal/core/core.go b/internal/core/core.go index 65298c2..a23c992 100644 --- a/internal/core/core.go +++ b/internal/core/core.go @@ -56,8 +56,26 @@ func NewFromConfig(cfg *config.Config) (*Core, error) { c.store = config.NewStore(cfg.RuntimeFile) // Share one box between the runtime store and config.yaml so a single // master.key seals both files. config.Load left the config holding - // ciphertext (if it was sealed); unseal it now that the box exists. + // ciphertext (if it was sealed); unseal it NOW, before anything reads a + // credential. + // + // ORDER IS LOAD-BEARING. These steps each consume secrets and must run + // after the unseal: + // - seedKeys compares cfg.Keys[i].Key against the plaintext + // gateway_keys entries; with ciphertext keys the comparison never + // matched and every restart appended another duplicate admin key + // (production had four copies of the same admin key). + // - rebuildRegistry hands cfg.Sources[i].APIKey to the providers; with + // ciphertext it built every upstream client with "enc:v1:..." as its + // bearer token. + // Previously the unseal happened at the END of this function, and things + // only appeared to work because seedKeys' Save() unsealed memory as a side + // effect. Removing the redundant saves exposed the real ordering bug. cfg.AttachSecretBox(c.store.SecretBox()) + hadPlaintextSecrets, err := cfg.UnsealSecrets(c.store.SecretBox()) + if err != nil { + return nil, fmt.Errorf("unseal secrets: %w", err) + } if err := c.store.Load(); err != nil { return nil, fmt.Errorf("runtime store: %w", err) } @@ -79,10 +97,10 @@ func NewFromConfig(cfg *config.Config) (*Core, error) { if err := c.seedPresetTemplates(); err != nil { return nil, fmt.Errorf("seed preset templates: %w", err) } - // Seal any credential still in the clear in config.yaml. Idempotent: an - // already-sealed config is not rewritten, so a normal restart writes - // nothing. This is the only place that rewrites the file on startup. - if err := cfg.NormalizeSecretsForRun(c.store.SecretBox()); err != nil { + // Seal any credential still in the clear in config.yaml. Startup writes + // nothing when the file was already sealed (hadPlaintextSecrets is decided + // from the on-disk state, before the unseal above). + if err := cfg.SealIfNeeded(hadPlaintextSecrets); err != nil { return nil, fmt.Errorf("normalize secrets: %w", err) } return c, nil diff --git a/internal/core/startup_secrets_test.go b/internal/core/startup_secrets_test.go new file mode 100644 index 0000000..8f402c6 --- /dev/null +++ b/internal/core/startup_secrets_test.go @@ -0,0 +1,178 @@ +package core + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "llmsproxy/internal/config" +) + +// Startup must be idempotent, and every credential consumer must see PLAINTEXT. +// +// Two bugs shared one root cause: NewFromConfig unsealed the config at the END, +// after the steps that read credentials had already run. +// +// 1. seedKeys() deduped cfg.Keys[i].Key against the plaintext gateway_keys +// entries. With ciphertext keys the comparison never matched, so every +// restart appended another copy of the same admin key — production reached +// four identical admin keys. +// 2. rebuildRegistry() handed cfg.Sources[i].APIKey to provider.New, so with +// ciphertext it built every upstream client with "enc:v1:..." as its bearer +// token and every upstream call would 401. +// +// They masked each other: seedKeys' Save() unsealed memory as a side effect, so +// bug 2 was invisible until the redundant saves were removed. These tests drive +// the real entry point (core.New -> config.Load off disk), because building a +// fresh in-memory config per attempt hides both bugs — that is exactly how the +// first draft of this test failed to reproduce anything. + +// writeSealedInstall creates a config on disk the way a real install looks +// after its first run: credentials already sealed, gateway_keys still plaintext. +func writeSealedInstall(t *testing.T, dir string) string { + t.Helper() + path := filepath.Join(dir, "config.yaml") + cfg := &config.Config{ + Path: path, + AdapterDir: filepath.Join(dir, "adapters"), + RuntimeFile: filepath.Join(dir, "runtime.json"), + Listen: "127.0.0.1:0", + DefaultModel: "AUTO", + GatewayKeys: []string{"sk-gw-seed-me"}, + Sources: []config.Source{{ + Name: "up", BaseURL: "http://127.0.0.1:1/v1", APIKey: "sk-upstream-secret", + Adapter: "openai", Models: []config.Model{{ID: "gpt-4o", Priority: 10}}, + }}, + } + c, err := NewFromConfig(cfg) + if err != nil { + t.Fatalf("initial install: %v", err) + } + c.Close() + + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(raw), "enc:v1:") { + t.Fatalf("fixture is wrong: on-disk config holds no sealed credential, "+ + "so this test cannot detect ciphertext leaking into consumers:\n%s", raw) + } + return path +} + +// writeSealedInstallNoSeed is writeSealedInstall without gateway_keys, so no +// key is ever seeded and no startup Save() can unseal the config as a side +// effect. That isolation is what makes the ciphertext leak observable. +func writeSealedInstallNoSeed(t *testing.T, dir string) string { + t.Helper() + path := filepath.Join(dir, "config.yaml") + cfg := &config.Config{ + Path: path, + AdapterDir: filepath.Join(dir, "adapters"), + RuntimeFile: filepath.Join(dir, "runtime.json"), + Listen: "127.0.0.1:0", + DefaultModel: "AUTO", + Sources: []config.Source{{ + Name: "up", BaseURL: "http://127.0.0.1:1/v1", APIKey: "sk-upstream-secret", + Adapter: "openai", Models: []config.Model{{ID: "gpt-4o", Priority: 10}}, + }}, + } + c, err := NewFromConfig(cfg) + if err != nil { + t.Fatalf("initial install: %v", err) + } + c.Close() + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(raw), "enc:v1:") { + t.Fatalf("fixture is wrong: no sealed credential on disk:\n%s", raw) + } + return path +} + +// countKeysWithSecret loads the config, unseals it, and counts keys whose +// plaintext equals one of the gateway_keys entries. +func countKeysWithSecret(t *testing.T, path string) (total, matching int) { + t.Helper() + cfg, err := config.Load(path) + if err != nil { + t.Fatal(err) + } + box, err := config.NewSecretBox(cfg.RuntimeFile) + if err != nil { + t.Fatal(err) + } + if _, err := cfg.UnsealSecrets(box); err != nil { + t.Fatal(err) + } + want := map[string]bool{} + for _, g := range cfg.GatewayKeys { + want[g] = true + } + for _, k := range cfg.Keys { + total++ + if want[k.Key] { + matching++ + } + } + return total, matching +} + +// Restarting must not grow the key list. Before the fix this went 2 -> 3 -> 4. +func TestRestartDoesNotDuplicateSeededKeys(t *testing.T) { + dir := t.TempDir() + path := writeSealedInstall(t, dir) + + if _, n := countKeysWithSecret(t, path); n != 1 { + t.Fatalf("after install: %d keys carry the seeded secret, want 1", n) + } + + for i := 2; i <= 4; i++ { + c, err := New(path) // production path: reads the file + if err != nil { + t.Fatalf("restart %d: %v", i, err) + } + c.Close() + total, n := countKeysWithSecret(t, path) + if n != 1 { + t.Fatalf("restart %d: %d keys carry the seeded secret (total %d), want "+ + "exactly 1 — seedKeys is comparing ciphertext against plaintext and "+ + "appending a duplicate on every start", i, n, total) + } + } +} + +// Every provider must hold the real upstream credential, not its ciphertext. +// +// This deliberately runs with gateway_keys EMPTY. The buggy order was masked +// whenever seedKeys had work to do, because its Save() unsealed memory as a +// side effect — providers then happened to see plaintext. With no key to seed, +// no Save runs, and the ciphertext reaches the registry directly. An earlier +// version of this test kept gateway_keys populated and was insensitive: it +// passed even with the bug present. +func TestProvidersGetPlaintextCredentials(t *testing.T) { + dir := t.TempDir() + path := writeSealedInstallNoSeed(t, dir) + + c, err := New(path) + if err != nil { + t.Fatalf("restart: %v", err) + } + defer c.Close() + + for _, p := range c.registry.Providers() { + key := p.Config().APIKey + if strings.HasPrefix(key, "enc:v1:") { + t.Errorf("provider %q holds CIPHERTEXT api_key %q — every upstream call "+ + "would 401", p.Name(), key) + } + if key != "sk-upstream-secret" { + t.Errorf("provider %q api_key = %q, want the plaintext upstream secret", + p.Name(), key) + } + } +}