mirror of
https://gitcode.com/JianFeeeee/ModelRouter.git
synced 2026-10-03 23:54:06 +00:00
fix(core): 修复启动重复播种 admin key + 配置封存非幂等
根因是 unseal 时序:NewFromConfig 把解密放在最后,而之前几步已经在读凭据。
1. seedKeys 重复播种(生产已累积 4 个同名 admin key)
seedKeys 用 cfg.Keys[i].Key 与明文 gateway_keys 比对去重,但此时内存里的
key 还是密文 enc:v1:…,比对永不命中 ⇒ 每次重启追加一个同值 admin key。
实测:core.New(path) 连续重启,seeded key 数 2→3→4 递增。
(旧测试用 NewFromConfig 构造全新内存对象,没有「盘上已有密文」这个前提,
复现不出 —— 必须走 core.New 这条读盘的生产路径。)
2. 启动恒重写 config.yaml
migratePlaintextSecrets 按内存状态判断,而 Save() 末尾会把内存恢复为明文,
于是每次调用都判定「还有明文」并重写;注释却自称幂等。
改为 UnsealSecrets 在解密前记录「盘上是否明文」,SealIfNeeded 据此决定
是否写回 ⇒ 已封存的配置启动不再落盘。
原测试 TestMigratePlaintextSecretsIsIdempotent 用 ModTime 比较,两次写落在同一
时间戳刻度内就看不出来,所以表现为 ~1/6 概率的 flake 而非稳定失败。已改为比较
文件内容并走真实启动路径(UnsealSecrets + SealIfNeeded),并顺带消除该 flake。
附带更正:先前判断「rebuildRegistry 也会拿到密文 API key」不成立 ——
mergedSources → resolveSourceKey 对每个 source 独立解密(belt-and-braces),
provider 始终拿到明文。unseal 前置仍予保留,以消除对该兜底路径的隐性依赖、
并让 seedKeys 在明文下比较。
判据:
- TestRestartDoesNotDuplicateSeededKeys(敏感:回退顺序必红)
- TestSealingIsIdempotentAcrossStarts(12/12 稳定,原先 1/6 flake)
- TestProvidersGetPlaintextCredentials(钉 provider 必须拿到明文这一不变量)
This commit is contained in:
@ -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
|
||||
|
||||
@ -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")
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -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
|
||||
|
||||
178
internal/core/startup_secrets_test.go
Normal file
178
internal/core/startup_secrets_test.go
Normal file
@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user