From baa1400f2ad3dac67dc714cc5a3c6d946eafe8e1 Mon Sep 17 00:00:00 2001 From: james-crowley <42618816+james-crowley@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:45:33 -0400 Subject: [PATCH 1/5] feat(config): read the API key from MCPPROXY_API_KEY_FILE Docker Compose/Swarm secrets, Kubernetes secret volumes and systemd LoadCredential= deliver a secret as a file. Add the *_FILE form for the proxy's own API key so it does not have to sit in the process environment (docker inspect, /proc//environ, crash dumps) or in mcp_config.json. One helper, config.ResolveEnvAPIKey, is now the only reader of the environment's key, so the loader, EnsureAPIKey, the CLI client, status, the tray and the core it launches agree: - the file wins over MCPPROXY_API_KEY (reported, and logged as a warning); - an empty MCPPROXY_API_KEY_FILE is unset; - a named file that cannot be used (missing, unreadable, relative, over 64 KiB, empty) stops startup. No fallback to the other variable, the config file or a generated key, and nothing is saved; - the key is a process-only override and is never written back. secret.ReadFile holds the read rules (absolute path, regular file after symlinks, size cap, one trailing LF/CRLF removed, no NUL, errors never include content) for the ${file:} provider that follows. Config.EnsureAPIKey now returns an error. A tray-launched core reads the file itself instead of receiving the key in its environment. Spec 111 FR-014, FR-030 and US7 amended to cover the proxy's own key. Refs #1706 --- CHANGELOG.md | 2 + cmd/mcpproxy-tray/main.go | 35 +++- cmd/mcpproxy-tray/main_test.go | 40 ++++ cmd/mcpproxy/daemon_client.go | 16 +- cmd/mcpproxy/daemon_client_api_key_test.go | 40 ++++ cmd/mcpproxy/main.go | 14 +- cmd/mcpproxy/serve_flag_persistence_test.go | 3 +- cmd/mcpproxy/status_cmd.go | 12 +- docs/configuration.md | 3 +- docs/configuration/environment-variables.md | 33 ++++ docs/getting-started/installation.md | 3 + internal/config/api_key_env.go | 76 ++++++++ internal/config/api_key_env_test.go | 194 ++++++++++++++++++++ internal/config/config.go | 47 +++-- internal/config/process_overrides_test.go | 6 +- internal/secret/file_read.go | 90 +++++++++ internal/secret/file_read_test.go | 119 ++++++++++++ specs/111-personal-docker-image/spec.md | 6 +- 18 files changed, 705 insertions(+), 34 deletions(-) create mode 100644 cmd/mcpproxy/daemon_client_api_key_test.go create mode 100644 internal/config/api_key_env.go create mode 100644 internal/config/api_key_env_test.go create mode 100644 internal/secret/file_read.go create mode 100644 internal/secret/file_read_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index b3eb0f269..c337b5915 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,8 @@ Releases follow [Semantic Versioning](https://semver.org/). ### Features +- **config: `MCPPROXY_API_KEY_FILE` reads the API key from a file (#1706).** The `*_FILE` form of Docker Compose/Swarm secrets, Kubernetes secret volumes and systemd `LoadCredential=`: the key is not in the process environment (`docker inspect`, `/proc//environ`) and is never written to `mcp_config.json`. The path must be absolute; one trailing LF or CRLF is removed; at most 64 KiB is read. It wins over `MCPPROXY_API_KEY` (the log says so), and an empty value is treated as unset. A file that is named but cannot be used stops startup with an error naming the variable and path: mcpproxy does not fall back to `MCPPROXY_API_KEY`, the config file or a generated key. `mcpproxy status`, the other CLI commands and the tray read it too, and a tray-launched core reads the file itself instead of receiving the key in its environment. `Config.EnsureAPIKey` now also returns an error. (refs #1706, spec 111 FR-014/FR-030/US7) + - **catalog:** a catalog source whose live search times out or fails is now answered from the listing the daemon last saw from it (at most 24 hours old, kept in memory). Those results are marked `from_cache` ("From cached list" in the Web UI and macOS, `(cached)` in the CLI) and the source diff --git a/cmd/mcpproxy-tray/main.go b/cmd/mcpproxy-tray/main.go index 458037dd0..563f0005e 100644 --- a/cmd/mcpproxy-tray/main.go +++ b/cmd/mcpproxy-tray/main.go @@ -159,11 +159,24 @@ func main() { // Setup API key for secure communication between tray and core // Skip API key generation for socket/pipe connections as they're trusted by default if trayAPIKey == "" && !usingSocketCommunication { - // Check environment variable first (for consistency with core behavior) - if envAPIKey := os.Getenv("MCPPROXY_API_KEY"); envAPIKey != "" { - trayAPIKey = envAPIKey - logger.Info("Using API key from environment variable for tray-core communication", - zap.String("api_key_source", "MCPPROXY_API_KEY environment variable"), + // Check the environment first (for consistency with core behavior): + // MCPPROXY_API_KEY_FILE, else MCPPROXY_API_KEY. A named file that cannot + // be used stops the tray: the core would refuse to start with it, and a + // generated key would hide that. + envKey, envErr := config.ResolveEnvAPIKey() + if envErr != nil { + logger.Error("Cannot read the API key named by the environment", zap.Error(envErr)) + _ = logger.Sync() + os.Exit(1) + } + if envKey.Set && envKey.Key != "" { + trayAPIKey = envKey.Key + source := "MCPPROXY_API_KEY environment variable" + if envKey.FromFile { + source = config.EnvAPIKeyFile + " file" + } + logger.Info("Using API key from environment for tray-core communication", + zap.String("api_key_source", source), zap.String("api_key_prefix", maskAPIKey(trayAPIKey))) } else { trayAPIKey = generateAPIKey() @@ -1935,9 +1948,15 @@ func (cpl *CoreProcessLauncher) buildCoreEnvironment() []string { } // Add our environment variables - filtered = append(filtered, - "MCPPROXY_ENABLE_TRAY=false", - fmt.Sprintf("MCPPROXY_API_KEY=%s", trayAPIKey)) + filtered = append(filtered, "MCPPROXY_ENABLE_TRAY=false") + // When the key lives in a file (MCPPROXY_API_KEY_FILE, which the core + // inherits), let the core read it from there: injecting it as + // MCPPROXY_API_KEY would put it in the core's environment, which is what + // the file form exists to avoid. The tray read the same file, so both + // sides hold the same key. + if os.Getenv(config.EnvAPIKeyFile) == "" { + filtered = append(filtered, fmt.Sprintf("MCPPROXY_API_KEY=%s", trayAPIKey)) + } // Tell the core it was launched by the tray, so telemetry's launch_source // can say so. Without this a tray-spawned core is unclassifiable: its parent diff --git a/cmd/mcpproxy-tray/main_test.go b/cmd/mcpproxy-tray/main_test.go index ac4a3d8d1..8143479d8 100644 --- a/cmd/mcpproxy-tray/main_test.go +++ b/cmd/mcpproxy-tray/main_test.go @@ -425,3 +425,43 @@ func TestShouldTerminateCore(t *testing.T) { }) } } + +// With MCPPROXY_API_KEY_FILE the core reads the key from the file itself; the +// tray must not copy it into the core's environment. +func TestBuildCoreEnvironmentAPIKey(t *testing.T) { + prev := trayAPIKey + t.Cleanup(func() { trayAPIKey = prev }) + trayAPIKey = "tray-key" + launcher := &CoreProcessLauncher{} + + count := func(env []string, prefix string) (n int, last string) { + for _, e := range env { + if strings.HasPrefix(e, prefix) { + n++ + last = e + } + } + return n, last + } + + t.Run("key injected when no file is named", func(t *testing.T) { + t.Setenv("MCPPROXY_API_KEY_FILE", "") + t.Setenv("MCPPROXY_API_KEY", "stale") + n, last := count(launcher.buildCoreEnvironment(), "MCPPROXY_API_KEY=") + if n != 1 || last != "MCPPROXY_API_KEY=tray-key" { + t.Fatalf("want exactly the tray key injected, got %d entries, last %q", n, last) + } + }) + + t.Run("key left in the file when one is named", func(t *testing.T) { + t.Setenv("MCPPROXY_API_KEY_FILE", "/run/secrets/mcpproxy_key") + t.Setenv("MCPPROXY_API_KEY", "stale") + env := launcher.buildCoreEnvironment() + if n, _ := count(env, "MCPPROXY_API_KEY="); n != 0 { + t.Fatalf("the key must not be copied into the core's environment, found %d entries", n) + } + if n, _ := count(env, "MCPPROXY_API_KEY_FILE="); n != 1 { + t.Fatalf("the core must inherit MCPPROXY_API_KEY_FILE, found %d entries", n) + } + }) +} diff --git a/cmd/mcpproxy/daemon_client.go b/cmd/mcpproxy/daemon_client.go index 1d0a57b24..9311f34c7 100644 --- a/cmd/mcpproxy/daemon_client.go +++ b/cmd/mcpproxy/daemon_client.go @@ -12,6 +12,7 @@ package main import ( "context" + "fmt" "net" "os" "strings" @@ -30,11 +31,20 @@ import ( const daemonProbeTimeout = 2 * time.Second // resolveAPIKey returns the API key the daemon would accept, mirroring -// Config.EnsureAPIKey precedence (env > config file) WITHOUT ever generating +// Config.EnsureAPIKey precedence (MCPPROXY_API_KEY_FILE > MCPPROXY_API_KEY > +// config file) WITHOUT ever generating // a new key — a fabricated key cannot match a running daemon's. func resolveAPIKey(cfg *config.Config) string { - if envAPIKey, exists := os.LookupEnv("MCPPROXY_API_KEY"); exists && envAPIKey != "" { - return envAPIKey + envKey, err := config.ResolveEnvAPIKey() + if err != nil { + // MCPPROXY_API_KEY_FILE names a file that cannot be used. Do not fall + // back to another key (it would be the wrong one, or none): say so and + // let the command take its no-daemon path. + fmt.Fprintf(os.Stderr, "Warning: %v; not using any API key\n", err) + return "" + } + if envKey.Set && envKey.Key != "" { + return envKey.Key } if cfg != nil { return cfg.APIKey diff --git a/cmd/mcpproxy/daemon_client_api_key_test.go b/cmd/mcpproxy/daemon_client_api_key_test.go new file mode 100644 index 000000000..8a1048d98 --- /dev/null +++ b/cmd/mcpproxy/daemon_client_api_key_test.go @@ -0,0 +1,40 @@ +package main + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +func TestResolveAPIKey_Precedence(t *testing.T) { + keyFile := filepath.Join(t.TempDir(), "key") + require.NoError(t, os.WriteFile(keyFile, []byte("file-key\n"), 0o600)) + cfg := &config.Config{APIKey: "config-key"} + + t.Run("config only", func(t *testing.T) { + t.Setenv(config.EnvAPIKey, "") + require.NoError(t, os.Unsetenv(config.EnvAPIKey)) + t.Setenv(config.EnvAPIKeyFile, "") + assert.Equal(t, "config-key", resolveAPIKey(cfg)) + }) + t.Run("env beats config", func(t *testing.T) { + t.Setenv(config.EnvAPIKey, "env-key") + t.Setenv(config.EnvAPIKeyFile, "") + assert.Equal(t, "env-key", resolveAPIKey(cfg)) + }) + t.Run("file beats env and config, as the daemon resolves it", func(t *testing.T) { + t.Setenv(config.EnvAPIKey, "env-key") + t.Setenv(config.EnvAPIKeyFile, keyFile) + assert.Equal(t, "file-key", resolveAPIKey(cfg)) + }) + t.Run("an unusable file gives no key rather than a wrong one", func(t *testing.T) { + t.Setenv(config.EnvAPIKey, "env-key") + t.Setenv(config.EnvAPIKeyFile, filepath.Join(t.TempDir(), "missing")) + assert.Empty(t, resolveAPIKey(cfg)) + }) +} diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 30f03bbb8..01d0acc55 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -549,7 +549,19 @@ func runServer(cmd *cobra.Command, _ []string) error { zap.Bool("enable_prompts", cfg.EnablePrompts)) // Ensure API key is configured - apiKey, wasGenerated, source := cfg.EnsureAPIKey() + apiKey, wasGenerated, source, keyErr := cfg.EnsureAPIKey() + if keyErr != nil { + // MCPPROXY_API_KEY_FILE names a file that cannot be used. Stop here: + // generating and saving a key instead would replace the credential the + // operator meant to run with. + return keyErr + } + if source == config.APIKeySourceEnvironmentFile { + if res, resErr := config.ResolveEnvAPIKey(); resErr == nil && res.Shadowed { + logger.Warn("Both MCPPROXY_API_KEY and MCPPROXY_API_KEY_FILE are set; using the key from the file", + zap.String("api_key_file", res.Path)) + } + } // SECURITY: API key is always required - empty keys are never allowed if apiKey == "" { diff --git a/cmd/mcpproxy/serve_flag_persistence_test.go b/cmd/mcpproxy/serve_flag_persistence_test.go index c67a11277..578062dc9 100644 --- a/cmd/mcpproxy/serve_flag_persistence_test.go +++ b/cmd/mcpproxy/serve_flag_persistence_test.go @@ -200,7 +200,8 @@ func TestServeSaverDoesNotPersistEnvAPIKey(t *testing.T) { cfg, saver, err := loadConfig(newServeFlagTestCmd()) require.NoError(t, err) - apiKey, wasGenerated, _ := cfg.EnsureAPIKey() + apiKey, wasGenerated, _, keyErr := cfg.EnsureAPIKey() + require.NoError(t, keyErr) require.Equal(t, "mcp_env_secret", apiKey) require.False(t, wasGenerated) diff --git a/cmd/mcpproxy/status_cmd.go b/cmd/mcpproxy/status_cmd.go index b4f3ece13..75117d9a0 100644 --- a/cmd/mcpproxy/status_cmd.go +++ b/cmd/mcpproxy/status_cmd.go @@ -132,7 +132,9 @@ func runStatus(cmd *cobra.Command, _ []string) error { } // Ensure API key exists - cfg.EnsureAPIKey() + if _, _, _, keyErr := cfg.EnsureAPIKey(); keyErr != nil { + return keyErr + } configPath := config.GetConfigPath(cfg.DataDir) @@ -149,8 +151,12 @@ func runStatus(cmd *cobra.Command, _ []string) error { fmt.Fprintln(os.Stderr) // Check if env var overrides - if envKey, exists := os.LookupEnv("MCPPROXY_API_KEY"); exists && envKey != "" { - fmt.Fprintln(os.Stderr, "Warning: MCPPROXY_API_KEY environment variable is set and will override the config file key.") + if envKey, envErr := config.ResolveEnvAPIKey(); envErr == nil && envKey.Set && envKey.Key != "" { + if envKey.FromFile { + fmt.Fprintf(os.Stderr, "Warning: %s is set and the key in %s will override the config file key.\n", config.EnvAPIKeyFile, envKey.Path) + } else { + fmt.Fprintln(os.Stderr, "Warning: MCPPROXY_API_KEY environment variable is set and will override the config file key.") + } fmt.Fprintln(os.Stderr) } diff --git a/docs/configuration.md b/docs/configuration.md index de3443f8c..705545cff 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -827,7 +827,7 @@ With `require_mcp_auth` off, a change that would let a client bound to a profile | `allow_server_remove` | boolean | `true` | Allow removing servers via API/tools | **Security Notes:** -- **API Key**: Set via `--api-key` flag, `MCPPROXY_API_KEY` environment variable, or config file +- **API Key**: Set via `--api-key` flag, `MCPPROXY_API_KEY` environment variable, `MCPPROXY_API_KEY_FILE` (a file holding the key, for Docker/Kubernetes secrets and systemd credentials; see [Environment variables](/configuration/environment-variables#api-key-from-a-file)), or config file - **Empty API Key**: Empty values are replaced with an auto-generated key; authentication is always enforced - **Auto-Generation**: If no API key is provided, one is generated, persisted to the config file, and printed once to the terminal (stderr). It is deliberately **not** written to the log files - to recover it later, read `api_key` from `~/.mcpproxy/mcp_config.json` - **Tray Integration**: Tray app automatically manages API keys for core communication @@ -1836,6 +1836,7 @@ Many configuration options can be overridden via environment variables: |----------------------|--------------|-------------| | `MCPPROXY_LISTEN` / `MCPP_LISTEN` | `listen` | Network binding address | | `MCPPROXY_API_KEY` | `api_key` | API key for authentication (empty values trigger auto-generation; auth remains enabled) | +| `MCPPROXY_API_KEY_FILE` | `api_key` | Absolute path of a file that holds the API key; wins over `MCPPROXY_API_KEY`; an unusable file stops startup (no fallback, nothing generated) | | `MCPPROXY_TRUSTED_HOSTS` | `trusted_hosts` | Comma-separated `Host` allowlist for loopback listeners behind a reverse proxy | | `MCPPROXY_TRUSTED_PROXIES` | `trusted_proxies` | Comma-separated CIDRs/IPs whose `X-Forwarded-*` headers are honoured | | `MCPPROXY_AUDIT_LOG_ENABLED` | `audit_log.enabled` | Turn audit logging on/off | diff --git a/docs/configuration/environment-variables.md b/docs/configuration/environment-variables.md index bd7b1b433..ee00a670e 100644 --- a/docs/configuration/environment-variables.md +++ b/docs/configuration/environment-variables.md @@ -30,9 +30,42 @@ Environment variables are useful for CI/CD environments or temporary overrides d |----------|-------------|---------| | `MCPPROXY_LISTEN` | Override listen address | `127.0.0.1:8080` or `:8080` | | `MCPPROXY_API_KEY` | Set API key for authentication | `my-secret-key` | +| `MCPPROXY_API_KEY_FILE` | Read the API key from a file (absolute path). Takes precedence over `MCPPROXY_API_KEY`; see [API key from a file](#api-key-from-a-file) | `/run/secrets/mcpproxy_api_key` | | `MCPPROXY_DATA_DIR` | Override data directory | `/var/lib/mcpproxy` | | `MCPPROXY_DATA` | Override data directory (backward compatibility, prefer `MCPPROXY_DATA_DIR`) | `/var/lib/mcpproxy` | +### API key from a file + +`MCPPROXY_API_KEY_FILE` is the `*_FILE` form used by Docker Compose and Swarm secrets +(`/run/secrets/`), Kubernetes secret volumes and systemd `LoadCredential=`. The key is read +from the file at start, so it is not in the process environment (`docker inspect`, +`/proc//environ`, crash dumps) and is never written to `mcp_config.json`. + +- The path must be absolute. A symlink is followed (Kubernetes secret volumes are symlinks) but must + point at a regular file. At most 64 KiB is read. +- One trailing newline (LF or CRLF) is removed. Everything else, including inner whitespace, is kept. +- An empty `MCPPROXY_API_KEY_FILE` is treated as unset. +- If `MCPPROXY_API_KEY` is also set, the file wins and the log says so. +- A file that is named but cannot be used (missing, unreadable, not absolute, too large, empty) stops + startup with an error that names the variable and path. mcpproxy does **not** fall back to + `MCPPROXY_API_KEY`, the config file or a generated key, so a typo in a secret path cannot start the + proxy under a different credential. +- `mcpproxy status`, the other CLI commands and the tray read the same variable, so they find the key + the daemon uses. A tray-launched core reads the file itself rather than receiving the key in its + environment. + +```yaml +# docker-compose.yml +services: + mcpproxy: + environment: + MCPPROXY_API_KEY_FILE: /run/secrets/mcpproxy_api_key + secrets: [mcpproxy_api_key] +secrets: + mcpproxy_api_key: + file: ./mcpproxy_api_key.txt +``` + ### Security Settings | Variable | Description | Default | diff --git a/docs/getting-started/installation.md b/docs/getting-started/installation.md index e2e1698bb..2f7533658 100644 --- a/docs/getting-started/installation.md +++ b/docs/getting-started/installation.md @@ -397,6 +397,9 @@ docker run -d --name mcpproxy \ - **API key**: the REST API and Web UI require one. Set `MCPPROXY_API_KEY` explicitly and keep a copy — the image is distroless (no shell), so reading an auto-generated key back out of the container is awkward. + To keep the key out of `docker inspect` and the process environment, mount it as a secret and set + `MCPPROXY_API_KEY_FILE=/run/secrets/` instead (see + [API key from a file](/configuration/environment-variables#api-key-from-a-file)). - **Web UI**: `http://localhost:8080/ui/`. ### Behind an ingress with SSO diff --git a/internal/config/api_key_env.go b/internal/config/api_key_env.go new file mode 100644 index 000000000..8319bb7ac --- /dev/null +++ b/internal/config/api_key_env.go @@ -0,0 +1,76 @@ +package config + +import ( + "fmt" + "os" + "strings" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" +) + +const ( + // EnvAPIKey is the environment variable that carries the API key itself. + EnvAPIKey = "MCPPROXY_API_KEY" + // EnvAPIKeyFile names a file that holds the API key: the *_FILE convention + // of Docker Compose and Swarm secrets (/run/secrets/), Kubernetes + // secret volumes and systemd LoadCredential=. Unlike EnvAPIKey the key is + // not in the process environment, so it does not show up in + // `docker inspect`, /proc//environ or a crash dump. + EnvAPIKeyFile = "MCPPROXY_API_KEY_FILE" +) + +// EnvAPIKeyResult is the API key a process was handed through its +// environment, however it was handed over. +type EnvAPIKeyResult struct { + // Set is true when the environment carried a key variable: MCPPROXY_API_KEY + // is set, or MCPPROXY_API_KEY_FILE names a readable, non-empty file. An + // empty MCPPROXY_API_KEY is Set with an empty Key, which callers treat as + // "no key supplied" (a key is generated), the same as an unset variable. + Set bool + // Key is the value. It is never empty when FromFile is true. + Key string + // FromFile is true when Key was read from MCPPROXY_API_KEY_FILE. + FromFile bool + // Path is the file Key was read from, when FromFile. + Path string + // Shadowed is true when both variables carry a key and the file won. + Shadowed bool +} + +// ResolveEnvAPIKey is the one place the environment's API key is read. Every +// reader (the loader, EnsureAPIKey, the CLI client, `status`, the tray and the +// core it launches) goes through it so the two variables mean the same thing +// everywhere. +// +// MCPPROXY_API_KEY_FILE wins over MCPPROXY_API_KEY when both are set (an +// explicit file is the more deliberate choice and the one that keeps the key +// out of the environment); the result reports Shadowed so a caller can warn. +// An empty MCPPROXY_API_KEY_FILE is treated as unset, as Compose files that +// interpolate an optional variable produce one. +// +// A file that is named but cannot be used (missing, unreadable, not absolute, +// too large, empty after removing its trailing newline) is an error. The caller +// must stop: falling back to the other variable, or worse to a generated key, +// would start the proxy under a credential the operator did not choose. +func ResolveEnvAPIKey() (EnvAPIKeyResult, error) { + envKey, envSet := os.LookupEnv(EnvAPIKey) + + if path := os.Getenv(EnvAPIKeyFile); path != "" { + key, err := secret.ReadFile(path) + if err != nil { + return EnvAPIKeyResult{}, fmt.Errorf("%s: %w", EnvAPIKeyFile, err) + } + if strings.TrimSpace(key) == "" { + return EnvAPIKeyResult{}, fmt.Errorf("%s: secret file %q is empty", EnvAPIKeyFile, path) + } + return EnvAPIKeyResult{ + Set: true, + Key: key, + FromFile: true, + Path: path, + Shadowed: envSet && envKey != "", + }, nil + } + + return EnvAPIKeyResult{Set: envSet, Key: envKey}, nil +} diff --git a/internal/config/api_key_env_test.go b/internal/config/api_key_env_test.go new file mode 100644 index 000000000..33c1e009e --- /dev/null +++ b/internal/config/api_key_env_test.go @@ -0,0 +1,194 @@ +package config + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func writeKeyFile(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "api_key") + require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) + return path +} + +// unsetEnv removes name for the test and restores it afterwards (t.Setenv +// records the original value; the Unsetenv that follows leaves it unset). +func unsetEnv(t *testing.T, name string) { + t.Helper() + t.Setenv(name, "") + require.NoError(t, os.Unsetenv(name)) +} + +func TestResolveEnvAPIKey(t *testing.T) { + t.Run("neither variable", func(t *testing.T) { + unsetEnv(t, EnvAPIKey) + t.Setenv(EnvAPIKeyFile, "") + res, err := ResolveEnvAPIKey() + require.NoError(t, err) + assert.False(t, res.Set) + assert.Empty(t, res.Key) + }) + + t.Run("key only", func(t *testing.T) { + t.Setenv(EnvAPIKey, "env-key") + t.Setenv(EnvAPIKeyFile, "") + res, err := ResolveEnvAPIKey() + require.NoError(t, err) + assert.True(t, res.Set) + assert.Equal(t, "env-key", res.Key) + assert.False(t, res.FromFile) + assert.False(t, res.Shadowed) + }) + + t.Run("an empty key variable is still set (it disables authentication)", func(t *testing.T) { + t.Setenv(EnvAPIKey, "") + t.Setenv(EnvAPIKeyFile, "") + res, err := ResolveEnvAPIKey() + require.NoError(t, err) + assert.True(t, res.Set) + assert.Empty(t, res.Key) + }) + + t.Run("file only", func(t *testing.T) { + path := writeKeyFile(t, "file-key\n") + unsetEnv(t, EnvAPIKey) + t.Setenv(EnvAPIKeyFile, path) + res, err := ResolveEnvAPIKey() + require.NoError(t, err) + assert.True(t, res.Set) + assert.Equal(t, "file-key", res.Key, "one trailing newline is removed") + assert.True(t, res.FromFile) + assert.Equal(t, path, res.Path) + assert.False(t, res.Shadowed) + }) + + t.Run("CRLF file", func(t *testing.T) { + unsetEnv(t, EnvAPIKey) + t.Setenv(EnvAPIKeyFile, writeKeyFile(t, "file-key\r\n")) + res, err := ResolveEnvAPIKey() + require.NoError(t, err) + assert.Equal(t, "file-key", res.Key) + }) + + t.Run("both set: the file wins and the result says so", func(t *testing.T) { + t.Setenv(EnvAPIKey, "env-key") + t.Setenv(EnvAPIKeyFile, writeKeyFile(t, "file-key")) + res, err := ResolveEnvAPIKey() + require.NoError(t, err) + assert.Equal(t, "file-key", res.Key) + assert.True(t, res.FromFile) + assert.True(t, res.Shadowed) + }) + + t.Run("an empty file variable is unset", func(t *testing.T) { + t.Setenv(EnvAPIKey, "env-key") + t.Setenv(EnvAPIKeyFile, "") + res, err := ResolveEnvAPIKey() + require.NoError(t, err) + assert.Equal(t, "env-key", res.Key) + assert.False(t, res.FromFile) + }) +} + +// A named file that cannot be used is an error and never falls back to the +// other variable, nor to a generated key. +func TestResolveEnvAPIKey_UnusableFileIsAnError(t *testing.T) { + const fallback = "must-not-be-used" + cases := map[string]string{ + "missing": filepath.Join(t.TempDir(), "nope"), + "relative": "run/secrets/key", + "empty": writeKeyFile(t, ""), + "newline only": writeKeyFile(t, "\n"), + "whitespace only": writeKeyFile(t, " \t\n"), + "directory": t.TempDir(), + } + for name, path := range cases { + t.Run(name, func(t *testing.T) { + t.Setenv(EnvAPIKey, fallback) + t.Setenv(EnvAPIKeyFile, path) + res, err := ResolveEnvAPIKey() + require.Error(t, err) + assert.Contains(t, err.Error(), EnvAPIKeyFile) + assert.False(t, res.Set) + assert.NotContains(t, err.Error(), fallback) + }) + } +} + +func TestEnsureAPIKey_FileOverridesConfigAndEnv(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "api_key": "config-key", "mcpServers": []}`) + t.Setenv(EnvAPIKey, "env-key") + t.Setenv(EnvAPIKeyFile, writeKeyFile(t, "file-key\n")) + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + key, generated, source, keyErr := cfg.EnsureAPIKey() + require.NoError(t, keyErr) + assert.Equal(t, "file-key", key) + assert.False(t, generated) + assert.Equal(t, APIKeySourceEnvironmentFile, source) + + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, "config-key", readJSON(t, path)["api_key"], "the file's key never replaces the config's on save") +} + +func TestEnsureAPIKey_UnusableFileNeverGeneratesAKey(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "api_key": "config-key", "mcpServers": []}`) + cfg, err := LoadFromFile(path) + require.NoError(t, err) + unsetEnv(t, EnvAPIKey) + t.Setenv(EnvAPIKeyFile, filepath.Join(t.TempDir(), "missing")) + + key, generated, _, keyErr := cfg.EnsureAPIKey() + require.Error(t, keyErr) + assert.Empty(t, key) + assert.False(t, generated) + assert.Equal(t, "config-key", cfg.APIKey, "the config's key is untouched") + + // And with no key configured at all: still no generated key. + cfg2 := &Config{} + key, generated, _, keyErr = cfg2.EnsureAPIKey() + require.Error(t, keyErr) + assert.Empty(t, key) + assert.False(t, generated) + assert.Empty(t, cfg2.APIKey, "no key was generated") +} + +func TestLoad_APIKeyFromFileIsAProcessOverrideNeverSaved(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + unsetEnv(t, EnvAPIKey) + t.Setenv(EnvAPIKeyFile, writeKeyFile(t, "file-key\n")) + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + assert.Equal(t, "file-key", cfg.APIKey, "an empty api_key is filled from the file") + assert.Contains(t, ProcessOverrideFields(), "api_key") + + require.NoError(t, SaveConfig(cfg, path)) + raw, err := os.ReadFile(path) + require.NoError(t, err) + assert.NotContains(t, string(raw), "file-key", "the secret from the file must not be written into the config") +} + +func TestLoad_UnusableKeyFileFailsTheLoad(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + unsetEnv(t, EnvAPIKey) + t.Setenv(EnvAPIKeyFile, filepath.Join(t.TempDir(), "missing")) + + _, err := LoadFromFile(path) + require.Error(t, err, "a missing key file must stop the load, not fall through to a generated key") + assert.Contains(t, err.Error(), EnvAPIKeyFile) +} diff --git a/internal/config/config.go b/internal/config/config.go index c13770f4c..7070f5d63 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1925,6 +1925,7 @@ const ( APIKeySourceEnvironment APIKeySource = iota APIKeySourceConfig APIKeySourceGenerated + APIKeySourceEnvironmentFile ) // String returns a human-readable representation of the API key source @@ -1936,6 +1937,8 @@ func (s APIKeySource) String() string { return "configuration file" case APIKeySourceGenerated: return "auto-generated" + case APIKeySourceEnvironmentFile: + return "file named by " + EnvAPIKeyFile default: return "unknown" } @@ -2285,29 +2288,42 @@ func (sc *ServerConfig) IsToolAllowedByConfig(toolName string) bool { // EnsureAPIKey ensures the API key is set, generating one if needed // Returns the API key, whether it was auto-generated, and the source // SECURITY: Empty API keys are never allowed - always auto-generates if empty or missing -func (c *Config) EnsureAPIKey() (apiKey string, wasGenerated bool, source APIKeySource) { - // Check environment variable for API key first - this overrides config file - // Use LookupEnv to distinguish between "not set" and "set to empty string" - if envAPIKey, exists := os.LookupEnv("MCPPROXY_API_KEY"); exists && envAPIKey != "" { +// +// The error is non-nil only when MCPPROXY_API_KEY_FILE names a file that cannot +// be used. The caller must stop in that case: no key is generated and nothing +// is saved, so a typo in a secret path can never replace the key the operator +// meant to run with. +func (c *Config) EnsureAPIKey() (apiKey string, wasGenerated bool, source APIKeySource, err error) { + // The environment (MCPPROXY_API_KEY_FILE, else MCPPROXY_API_KEY) overrides + // the config file. Use the result's Set/Key to distinguish "not set" from + // "set to empty string". + envKey, err := ResolveEnvAPIKey() + if err != nil { + return "", false, APIKeySourceEnvironment, err + } + if envKey.Set && envKey.Key != "" { // A process-only override of whatever the file holds: no save path // may replace the file's key with it (see process_overrides.go). // Already equal means Validate recorded it (or the file literally // holds the env key); re-recording would lose the file value. - if c.APIKey != envAPIKey { - OverrideForProcess(c, FieldAPIKey, OverrideSourceEnv, envAPIKey) + if c.APIKey != envKey.Key { + OverrideForProcess(c, FieldAPIKey, OverrideSourceEnv, envKey.Key) } - return c.APIKey, false, APIKeySourceEnvironment + if envKey.FromFile { + return c.APIKey, false, APIKeySourceEnvironmentFile, nil + } + return c.APIKey, false, APIKeySourceEnvironment, nil } // If API key was explicitly set in config and is non-empty, use it if c.apiKeyExplicitlySet && c.APIKey != "" { - return c.APIKey, false, APIKeySourceConfig + return c.APIKey, false, APIKeySourceConfig, nil } // Generate a new API key if missing or empty (never allow empty for security) c.APIKey = generateAPIKey() c.apiKeyExplicitlySet = true - return c.APIKey, true, APIKeySourceGenerated + return c.APIKey, true, APIKeySourceGenerated, nil } // ValidationError represents a configuration validation error @@ -2952,13 +2968,18 @@ func (c *Config) Validate() error { // Handle API key generation if not configured // Empty string means authentication disabled, nil means auto-generate if c.APIKey == "" { - // Check environment variable for API key - // Use LookupEnv to distinguish between "not set" and "set to empty string" - if envAPIKey, exists := os.LookupEnv("MCPPROXY_API_KEY"); exists { + // Check the environment for an API key (MCPPROXY_API_KEY_FILE, else + // MCPPROXY_API_KEY). A file that is named but unusable fails the load: + // continuing would auto-generate a key and save it. + envKey, err := ResolveEnvAPIKey() + if err != nil { + return err + } + if envKey.Set { // Allow empty string to explicitly disable authentication. A // process-only override: no save path may write it into api_key // (see process_overrides.go). - OverrideForProcess(c, FieldAPIKey, OverrideSourceEnv, envAPIKey) + OverrideForProcess(c, FieldAPIKey, OverrideSourceEnv, envKey.Key) } } diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go index 620a4f316..bbdb8ab57 100644 --- a/internal/config/process_overrides_test.go +++ b/internal/config/process_overrides_test.go @@ -204,7 +204,8 @@ func TestEnsureAPIKey_EnvKeyOverFileKeyIsNotPersisted(t *testing.T) { require.NoError(t, err) require.Equal(t, "file-key", cfg.APIKey, "Validate only fills an EMPTY api_key from env") - key, generated, source := cfg.EnsureAPIKey() + key, generated, source, keyErr := cfg.EnsureAPIKey() + require.NoError(t, keyErr) require.Equal(t, "env-key", key) require.False(t, generated) require.Equal(t, APIKeySourceEnvironment, source) @@ -222,7 +223,8 @@ func TestEnsureAPIKey_GeneratedKeyIsPersisted(t *testing.T) { cfg, err := LoadFromFile(path) require.NoError(t, err) - key, generated, _ := cfg.EnsureAPIKey() + key, generated, _, keyErr := cfg.EnsureAPIKey() + require.NoError(t, keyErr) require.True(t, generated) require.NoError(t, SaveConfig(cfg, path)) diff --git a/internal/secret/file_read.go b/internal/secret/file_read.go new file mode 100644 index 000000000..1add4685b --- /dev/null +++ b/internal/secret/file_read.go @@ -0,0 +1,90 @@ +package secret + +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "path/filepath" +) + +// MaxSecretFileBytes caps how much of a secret file is read. A token, a +// password or an API key is a few hundred bytes; a PEM key is a few KiB. The +// cap keeps a mis-pointed path (a log, a device, a data file) from being read +// into memory and into a process's environment. +const MaxSecretFileBytes = 64 * 1024 + +// ReadFile reads a secret from an absolute file path, the way Docker Compose +// and Swarm secrets (/run/secrets/), Kubernetes secret volumes and +// systemd LoadCredential= (/run/credentials//) deliver one. +// +// Rules, shared by every caller so they behave the same: +// - the path must be absolute: a relative path would resolve against a +// working directory the operator did not choose; +// - symlinks are followed (Kubernetes secret volumes are symlinks), but the +// target must be a regular file; +// - at most MaxSecretFileBytes are accepted; a larger file is an error, not a +// truncated secret; +// - exactly one trailing newline (LF or CRLF) is removed, because most +// editors and `echo` add one and no secret should end in it; every other +// byte, including leading and inner whitespace, is kept; +// - a NUL byte is rejected, since the value ends up in an HTTP header or an +// environment variable, neither of which can carry one. +// +// An empty result is returned as "" with no error, so a caller can decide +// whether empty is acceptable for its field. Errors name the path and the +// reason and never include any of the file's content. +func ReadFile(path string) (string, error) { + if path == "" { + return "", errors.New("secret file path is empty") + } + if !filepath.IsAbs(path) { + return "", fmt.Errorf("secret file path %q must be absolute", path) + } + + f, err := os.Open(path) + if err != nil { + return "", fmt.Errorf("cannot read secret file %q: %w", path, unwrapPathError(err)) + } + defer f.Close() + + info, err := f.Stat() + if err != nil { + return "", fmt.Errorf("cannot read secret file %q: %w", path, unwrapPathError(err)) + } + if !info.Mode().IsRegular() { + return "", fmt.Errorf("secret file %q is not a regular file", path) + } + + // Read one byte past the cap so an oversized file is detected from what + // was read, not from a size that may have changed since Stat. + data, err := io.ReadAll(io.LimitReader(f, MaxSecretFileBytes+1)) + if err != nil { + return "", fmt.Errorf("cannot read secret file %q: %w", path, unwrapPathError(err)) + } + if len(data) > MaxSecretFileBytes { + return "", fmt.Errorf("secret file %q is larger than %d bytes", path, MaxSecretFileBytes) + } + if bytes.IndexByte(data, 0) >= 0 { + return "", fmt.Errorf("secret file %q contains a NUL byte", path) + } + + switch { + case bytes.HasSuffix(data, []byte("\r\n")): + data = data[:len(data)-2] + case bytes.HasSuffix(data, []byte("\n")): + data = data[:len(data)-1] + } + return string(data), nil +} + +// unwrapPathError drops the "open :" prefix of an *os.PathError so the +// path is named once, by the caller's own message. +func unwrapPathError(err error) error { + var pe *os.PathError + if errors.As(err, &pe) { + return pe.Err + } + return err +} diff --git a/internal/secret/file_read_test.go b/internal/secret/file_read_test.go new file mode 100644 index 000000000..b2eb2383f --- /dev/null +++ b/internal/secret/file_read_test.go @@ -0,0 +1,119 @@ +package secret + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func writeSecretFile(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "secret") + require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) + return path +} + +func TestReadFile_TrailingNewline(t *testing.T) { + cases := []struct { + name, content, want string + }{ + {"none", "abc", "abc"}, + {"LF", "abc\n", "abc"}, + {"CRLF", "abc\r\n", "abc"}, + {"only one LF is removed", "abc\n\n", "abc\n"}, + {"only one CRLF is removed", "abc\r\n\r\n", "abc\r\n"}, + {"a lone CR is kept", "abc\r", "abc\r"}, + {"leading and inner whitespace are kept", " a b\t\n", " a b\t"}, + {"empty", "", ""}, + {"newline only", "\n", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := ReadFile(writeSecretFile(t, tc.content)) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestReadFile_SizeCap(t *testing.T) { + atCap := strings.Repeat("a", MaxSecretFileBytes) + got, err := ReadFile(writeSecretFile(t, atCap)) + require.NoError(t, err) + assert.Len(t, got, MaxSecretFileBytes) + + _, err = ReadFile(writeSecretFile(t, atCap+"a")) + require.Error(t, err) + assert.Contains(t, err.Error(), "larger than") + + // A trailing newline does not count toward a file that is exactly at the + // cap once stripped, but the raw file is one byte over: it is refused, + // because the cap is on what is read. + _, err = ReadFile(writeSecretFile(t, atCap+"\n")) + require.Error(t, err) +} + +func TestReadFile_Refusals(t *testing.T) { + dir := t.TempDir() + + t.Run("empty path", func(t *testing.T) { + _, err := ReadFile("") + require.Error(t, err) + }) + t.Run("relative path", func(t *testing.T) { + _, err := ReadFile("secrets/key") + require.Error(t, err) + assert.Contains(t, err.Error(), "must be absolute") + }) + t.Run("missing file", func(t *testing.T) { + _, err := ReadFile(filepath.Join(dir, "nope")) + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot read secret file") + }) + t.Run("directory", func(t *testing.T) { + _, err := ReadFile(dir) + require.Error(t, err) + assert.Contains(t, err.Error(), "not a regular file") + }) + t.Run("NUL byte", func(t *testing.T) { + _, err := ReadFile(writeSecretFile(t, "ab\x00cd")) + require.Error(t, err) + assert.Contains(t, err.Error(), "NUL") + }) + t.Run("unreadable file", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root can read any file") + } + path := writeSecretFile(t, "abc") + require.NoError(t, os.Chmod(path, 0o000)) + _, err := ReadFile(path) + require.Error(t, err) + }) +} + +func TestReadFile_FollowsSymlinkToRegularFile(t *testing.T) { + // Kubernetes secret volumes expose each key as a symlink into ..data/. + target := writeSecretFile(t, "from-target\n") + link := filepath.Join(t.TempDir(), "link") + require.NoError(t, os.Symlink(target, link)) + + got, err := ReadFile(link) + require.NoError(t, err) + assert.Equal(t, "from-target", got) +} + +func TestReadFile_ErrorsNeverContainTheContent(t *testing.T) { + const sentinel = "SENTINEL-kq81ZtYw" + for name, content := range map[string]string{ + "nul": sentinel + "\x00", + "too big": sentinel + strings.Repeat("a", MaxSecretFileBytes), + } { + _, err := ReadFile(writeSecretFile(t, content)) + require.Error(t, err, name) + assert.NotContains(t, err.Error(), sentinel, name) + } +} diff --git a/specs/111-personal-docker-image/spec.md b/specs/111-personal-docker-image/spec.md index 08b44fc26..dbf5024fb 100644 --- a/specs/111-personal-docker-image/spec.md +++ b/specs/111-personal-docker-image/spec.md @@ -191,6 +191,8 @@ An operator stores upstream tokens as Docker or Kubernetes secrets mounted as fi 1. **Given** a readable file with a trailing LF newline, and separately a readable file with the same content but a trailing CRLF, **Then** both resolve to the identical value, with neither the trailing newline nor (for the CRLF file) the preceding carriage return included. 2. **Given** a missing or unreadable file, **Then** the server shows a missing-secret error naming the reference (not the value) and the proxy stays up. +3. **Given** `MCPPROXY_API_KEY_FILE` naming a readable file that holds the proxy's own API key, **When** the proxy starts, **Then** it authenticates with that key, generates and saves no key, the key is absent from the process environment and the config file, and the log names the variable and path but not the key. +4. **Given** `MCPPROXY_API_KEY_FILE` naming a missing, unreadable or empty file, **Then** startup fails with an error naming the variable and path, and no key is generated or saved. This is the one case where a missing file is fatal rather than shown as a missing-secret error: the proxy's own credential has no safe fallback. --- @@ -251,7 +253,7 @@ The maintainer wants to know how many installs use each official image and needs - **FR-011**: Both variants MUST run as a fixed non-root user (UID/GID 65532) by default and MUST work under an arbitrary non-root UID supplied by the orchestrator when the data volume is writable by that UID. - **FR-012**: Both variants MUST ship `/data` owned by the runtime user, so that a fresh named volume starts without any manual ownership step. - **FR-013**: When the data volume fails a write probe by the running user (regardless of its recorded owner, so a group-writable or `fsGroup`-adjusted volume passes), the container MUST exit with the documented permission exit code (5) and a message that names the volume path, the expected owner and the one-line fix. [Amended: the trigger is write-probe failure only, not ownership, so it does not contradict FR-011's arbitrary-UID support.] -- **FR-014**: When no API key is supplied, the first start MUST generate one, save it on the data volume, and log a message naming the FR-014 read-back command (`mcpproxy config get-api-key`) without printing the key itself, and the operator MUST be able to print the saved key by running `docker exec mcpproxy config get-api-key` (a new, no-shell CLI subcommand this spec adds; the exact form is `mcpproxy config get-api-key [--config PATH]`, matching the existing `--config`/`-c` flag convention used by `token`, `doctor` and other subcommands). That command MUST resolve and print the key through a config-only read path that does not open the BBolt database, so a read-back against a running container never contends the database lock (research R12/R13). In a detached (non-TTY) container — the documented and expected deployment mode for this spec's compose/Kubernetes examples — the container log MUST NOT contain the key. [Amended: the existing terminal-only banner behaviour (research R11) prints the key once when stderr is a TTY, e.g. under `docker run -it`, and `docker logs` captures that stream regardless of TTY allocation; this FR does not require changing that existing behaviour, and the documentation MUST say that starting the container with `-it` (or `kubectl exec`-style TTY attach at first boot) will put the key in `docker logs`/`kubectl logs` history, so operators should use the FR-014 read-back command instead of an interactive first run when they want the key kept out of log history.] The saved config file MUST be written with owner-only permissions (0600) so the key is not readable by other UIDs on a shared bind mount regardless of umask. An `MCPPROXY_API_KEY` environment variable set to an empty string MUST be treated the same as not set (a key is generated and saved), matching how Compose/Kubernetes interpolation leaves an unset variable as an empty string rather than an absent one. +- **FR-014**: When no API key is supplied, the first start MUST generate one, save it on the data volume, and log a message naming the FR-014 read-back command (`mcpproxy config get-api-key`) without printing the key itself, and the operator MUST be able to print the saved key by running `docker exec mcpproxy config get-api-key` (a new, no-shell CLI subcommand this spec adds; the exact form is `mcpproxy config get-api-key [--config PATH]`, matching the existing `--config`/`-c` flag convention used by `token`, `doctor` and other subcommands). That command MUST resolve and print the key through a config-only read path that does not open the BBolt database, so a read-back against a running container never contends the database lock (research R12/R13). In a detached (non-TTY) container — the documented and expected deployment mode for this spec's compose/Kubernetes examples — the container log MUST NOT contain the key. [Amended: the existing terminal-only banner behaviour (research R11) prints the key once when stderr is a TTY, e.g. under `docker run -it`, and `docker logs` captures that stream regardless of TTY allocation; this FR does not require changing that existing behaviour, and the documentation MUST say that starting the container with `-it` (or `kubectl exec`-style TTY attach at first boot) will put the key in `docker logs`/`kubectl logs` history, so operators should use the FR-014 read-back command instead of an interactive first run when they want the key kept out of log history.] The saved config file MUST be written with owner-only permissions (0600) so the key is not readable by other UIDs on a shared bind mount regardless of umask. An `MCPPROXY_API_KEY` environment variable set to an empty string MUST be treated the same as not set (a key is generated and saved), matching how Compose/Kubernetes interpolation leaves an unset variable as an empty string rather than an absent one. The API key MAY instead be supplied as a file: when `MCPPROXY_API_KEY_FILE` names an absolute path, the key is read from that file (read rules in FR-030) and counts as a supplied key, so none is generated or saved, and the key never enters the process environment. An empty `MCPPROXY_API_KEY_FILE` is treated as unset, for the same interpolation reason. If both variables carry a key the file wins and the log says so. A named file that cannot be used (missing, unreadable, not absolute, too large, empty) MUST stop startup with an error naming the variable and the path, never the content: the proxy MUST NOT fall back to `MCPPROXY_API_KEY`, the config file or a generated key, because that would run it under a credential the operator did not choose. The key read from the file is a process-only override and is never written back to the config file. - **FR-015**: The documentation MUST state that one data volume supports exactly one running container, and the Kubernetes example MUST use one replica with a recreate update strategy. - **FR-016**: The image entrypoint MUST run the proxy binary (behind the init process in the full variant) and the default command MUST be `serve` on `0.0.0.0:8080` with the data volume, so that `docker run [flags]` runs any subcommand without repeating the binary name, and no arguments start the server. - **FR-017**: When the first argument is the binary's own name, the image MUST treat it as absent (drop it) rather than passing it through as a subcommand. [Assumption: research D9.] @@ -277,7 +279,7 @@ The maintainer wants to know how many installs use each official image and needs **Secrets** - **FR-029**: `${env:NAME}` references MUST be documented as the primary way to pass secrets to upstream servers in containers. OS-keyring references MUST fail with a message that says the keyring is unavailable in containers and names the alternatives. -- **FR-030**: The proxy MUST support `${file:/absolute/path}` secret references that read a secret from a file, strip one trailing newline (and, when present, a preceding carriage return, so CRLF-terminated Windows-authored secret files resolve the same value as LF-terminated ones), never write the value back to config, and never show it in logs or API responses. +- **FR-030**: The proxy MUST support `${file:/absolute/path}` secret references that read a secret from a file, strip one trailing newline (and, when present, a preceding carriage return, so CRLF-terminated Windows-authored secret files resolve the same value as LF-terminated ones), never write the value back to config, and never show it in logs or API responses. The same read rules apply to every file-sourced secret, including `MCPPROXY_API_KEY_FILE` (FR-014): the path MUST be absolute, a symlink is followed but its target MUST be a regular file (Kubernetes secret volumes are symlinks), at most 64 KiB is read (a larger file is an error, not a truncated secret), a NUL byte is rejected, and an error names the path and the reason but never any of the content. **Updates and telemetry** From 721b976c62b967d20e3f2be8f0e0c1d9dfc44c37 Mon Sep 17 00:00:00 2001 From: james-crowley <42618816+james-crowley@users.noreply.github.com> Date: Tue, 20 Oct 2026 22:42:44 -0400 Subject: [PATCH 2/5] test(tray): allow the API key helper in the tray config-import guard The tray resolves MCPPROXY_API_KEY / MCPPROXY_API_KEY_FILE through config.ResolveEnvAPIKey so it matches the core. The helper reads the environment and the one key file the operator named; it never opens mcp_config.json. --- internal/tray/config_import_guard_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/tray/config_import_guard_test.go b/internal/tray/config_import_guard_test.go index 9b940138f..30dd1e719 100644 --- a/internal/tray/config_import_guard_test.go +++ b/internal/tray/config_import_guard_test.go @@ -31,8 +31,18 @@ const configPkg = "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" // LogConfig is a plain struct type describing the tray's OWN logger, built in // memory from logs.DefaultLogConfig() — see cmd/mcpproxy-tray/main.go. It reads // nothing from disk, so it is allowed. +// +// ResolveEnvAPIKey and EnvAPIKeyFile are the one helper that turns +// MCPPROXY_API_KEY / MCPPROXY_API_KEY_FILE into the proxy's key. The tray must +// resolve the key exactly as the core does (and refuse to start on an unusable +// key file instead of generating a key), so it calls the helper rather than +// copying its rules. The helper reads the process environment and, when +// MCPPROXY_API_KEY_FILE is set, that one file the operator named. It never +// opens mcp_config.json and writes nothing. var allowedConfigSymbols = map[string]bool{ - "LogConfig": true, + "LogConfig": true, + "ResolveEnvAPIKey": true, + "EnvAPIKeyFile": true, } // trayRoots are the tray-side trees the rule applies to. Both are walked From 6fb28b848d0c6861ec4243f519cbe01f41410581 Mon Sep 17 00:00:00 2001 From: james-crowley <42618816+james-crowley@users.noreply.github.com> Date: Tue, 20 Oct 2026 23:04:29 -0400 Subject: [PATCH 3/5] test(secret): skip the unreadable-file case on Windows --- internal/secret/file_read_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/secret/file_read_test.go b/internal/secret/file_read_test.go index b2eb2383f..5952f7201 100644 --- a/internal/secret/file_read_test.go +++ b/internal/secret/file_read_test.go @@ -3,6 +3,7 @@ package secret import ( "os" "path/filepath" + "runtime" "strings" "testing" @@ -85,6 +86,9 @@ func TestReadFile_Refusals(t *testing.T) { assert.Contains(t, err.Error(), "NUL") }) t.Run("unreadable file", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file permission bits are not enforced on Windows") + } if os.Geteuid() == 0 { t.Skip("root can read any file") } From 28730eff0ec0298329cb4d5827a62ee9164deb0a Mon Sep 17 00:00:00 2001 From: james-crowley <42618816+james-crowley@users.noreply.github.com> Date: Wed, 21 Oct 2026 11:24:32 -0400 Subject: [PATCH 4/5] feat(config): refuse to start when MCPPROXY_API_KEY and MCPPROXY_API_KEY_FILE are both set One rule instead of a precedence: when both variables are non-empty, ResolveEnvAPIKey returns ErrAPIKeyBothSet, whatever the file holds. The error names both variables and never contains a key or file contents. The loader (also when the config file carries its own api_key), EnsureAPIKey, the CLI client, status and the tray all stop on it; the tray now resolves the environment key even over a socket so it does not hide the conflict from the core it launches. The file-wins warning and EnvAPIKeyResult.Shadowed are removed. --- CHANGELOG.md | 2 +- cmd/mcpproxy-tray/main.go | 23 ++--- cmd/mcpproxy-tray/main_test.go | 2 +- cmd/mcpproxy/daemon_client.go | 11 +-- cmd/mcpproxy/daemon_client_api_key_test.go | 11 ++- cmd/mcpproxy/main.go | 13 +-- docs/configuration.md | 2 +- docs/configuration/environment-variables.md | 10 ++- docs/getting-started/installation.md | 2 +- internal/config/api_key_env.go | 37 ++++++-- internal/config/api_key_env_test.go | 98 ++++++++++++++++++--- internal/config/config.go | 20 +++-- specs/111-personal-docker-image/spec.md | 3 +- 13 files changed, 171 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c337b5915..f58228337 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,7 +79,7 @@ Releases follow [Semantic Versioning](https://semver.org/). ### Features -- **config: `MCPPROXY_API_KEY_FILE` reads the API key from a file (#1706).** The `*_FILE` form of Docker Compose/Swarm secrets, Kubernetes secret volumes and systemd `LoadCredential=`: the key is not in the process environment (`docker inspect`, `/proc//environ`) and is never written to `mcp_config.json`. The path must be absolute; one trailing LF or CRLF is removed; at most 64 KiB is read. It wins over `MCPPROXY_API_KEY` (the log says so), and an empty value is treated as unset. A file that is named but cannot be used stops startup with an error naming the variable and path: mcpproxy does not fall back to `MCPPROXY_API_KEY`, the config file or a generated key. `mcpproxy status`, the other CLI commands and the tray read it too, and a tray-launched core reads the file itself instead of receiving the key in its environment. `Config.EnsureAPIKey` now also returns an error. (refs #1706, spec 111 FR-014/FR-030/US7) +- **config: `MCPPROXY_API_KEY_FILE` reads the API key from a file (#1706).** The `*_FILE` form of Docker Compose/Swarm secrets, Kubernetes secret volumes and systemd `LoadCredential=`: the key is not in the process environment (`docker inspect`, `/proc//environ`) and is never written to `mcp_config.json`. The path must be absolute; one trailing LF or CRLF is removed; at most 64 KiB is read. Setting both `MCPPROXY_API_KEY` and `MCPPROXY_API_KEY_FILE` (both non-empty) is an error: mcpproxy refuses to start with a message naming both variables (never a key or file contents), and the CLI commands and the tray stop the same way. An empty value is treated as unset. A file that is named but cannot be used stops startup with an error naming the variable and path: mcpproxy does not fall back to the config file or a generated key. `mcpproxy status`, the other CLI commands and the tray read it too, and a tray-launched core reads the file itself instead of receiving the key in its environment. `Config.EnsureAPIKey` now also returns an error. (refs #1706, spec 111 FR-014/FR-030/US7) - **catalog:** a catalog source whose live search times out or fails is now answered from the listing the daemon last saw from it (at most 24 hours old, kept in memory). Those results are marked diff --git a/cmd/mcpproxy-tray/main.go b/cmd/mcpproxy-tray/main.go index 563f0005e..daa44045b 100644 --- a/cmd/mcpproxy-tray/main.go +++ b/cmd/mcpproxy-tray/main.go @@ -156,19 +156,22 @@ func main() { // Determine if we're using socket/pipe communication (which doesn't need API key) usingSocketCommunication := isSocketEndpoint(coreURL) + // Read the API key the environment names (MCPPROXY_API_KEY_FILE or + // MCPPROXY_API_KEY), for consistency with the core. Resolve it even when the + // tray talks over a socket: the core it launches inherits the same + // environment and refuses to start when both variables are set or the file + // cannot be used, so the tray stops here with the reason instead of + // generating a key that would hide it. The error never contains a key. + envKey, envErr := config.ResolveEnvAPIKey() + if envErr != nil { + logger.Error("Cannot read the API key named by the environment", zap.Error(envErr)) + _ = logger.Sync() + os.Exit(1) + } + // Setup API key for secure communication between tray and core // Skip API key generation for socket/pipe connections as they're trusted by default if trayAPIKey == "" && !usingSocketCommunication { - // Check the environment first (for consistency with core behavior): - // MCPPROXY_API_KEY_FILE, else MCPPROXY_API_KEY. A named file that cannot - // be used stops the tray: the core would refuse to start with it, and a - // generated key would hide that. - envKey, envErr := config.ResolveEnvAPIKey() - if envErr != nil { - logger.Error("Cannot read the API key named by the environment", zap.Error(envErr)) - _ = logger.Sync() - os.Exit(1) - } if envKey.Set && envKey.Key != "" { trayAPIKey = envKey.Key source := "MCPPROXY_API_KEY environment variable" diff --git a/cmd/mcpproxy-tray/main_test.go b/cmd/mcpproxy-tray/main_test.go index 8143479d8..05d0a37f4 100644 --- a/cmd/mcpproxy-tray/main_test.go +++ b/cmd/mcpproxy-tray/main_test.go @@ -455,7 +455,7 @@ func TestBuildCoreEnvironmentAPIKey(t *testing.T) { t.Run("key left in the file when one is named", func(t *testing.T) { t.Setenv("MCPPROXY_API_KEY_FILE", "/run/secrets/mcpproxy_key") - t.Setenv("MCPPROXY_API_KEY", "stale") + t.Setenv("MCPPROXY_API_KEY", "") env := launcher.buildCoreEnvironment() if n, _ := count(env, "MCPPROXY_API_KEY="); n != 0 { t.Fatalf("the key must not be copied into the core's environment, found %d entries", n) diff --git a/cmd/mcpproxy/daemon_client.go b/cmd/mcpproxy/daemon_client.go index 9311f34c7..6a99b0aac 100644 --- a/cmd/mcpproxy/daemon_client.go +++ b/cmd/mcpproxy/daemon_client.go @@ -31,15 +31,16 @@ import ( const daemonProbeTimeout = 2 * time.Second // resolveAPIKey returns the API key the daemon would accept, mirroring -// Config.EnsureAPIKey precedence (MCPPROXY_API_KEY_FILE > MCPPROXY_API_KEY > -// config file) WITHOUT ever generating +// Config.EnsureAPIKey precedence (MCPPROXY_API_KEY_FILE or MCPPROXY_API_KEY, +// which may not both be set, then the config file) WITHOUT ever generating // a new key — a fabricated key cannot match a running daemon's. func resolveAPIKey(cfg *config.Config) string { envKey, err := config.ResolveEnvAPIKey() if err != nil { - // MCPPROXY_API_KEY_FILE names a file that cannot be used. Do not fall - // back to another key (it would be the wrong one, or none): say so and - // let the command take its no-daemon path. + // Both variables are set, or MCPPROXY_API_KEY_FILE names a file that + // cannot be used. Do not fall back to another key (it would be the + // wrong one, or none): say so and let the command take its no-daemon + // path. The error text never contains a key. fmt.Fprintf(os.Stderr, "Warning: %v; not using any API key\n", err) return "" } diff --git a/cmd/mcpproxy/daemon_client_api_key_test.go b/cmd/mcpproxy/daemon_client_api_key_test.go index 8a1048d98..a503990ef 100644 --- a/cmd/mcpproxy/daemon_client_api_key_test.go +++ b/cmd/mcpproxy/daemon_client_api_key_test.go @@ -27,13 +27,18 @@ func TestResolveAPIKey_Precedence(t *testing.T) { t.Setenv(config.EnvAPIKeyFile, "") assert.Equal(t, "env-key", resolveAPIKey(cfg)) }) - t.Run("file beats env and config, as the daemon resolves it", func(t *testing.T) { - t.Setenv(config.EnvAPIKey, "env-key") + t.Run("file beats config, as the daemon resolves it", func(t *testing.T) { + t.Setenv(config.EnvAPIKey, "") t.Setenv(config.EnvAPIKeyFile, keyFile) assert.Equal(t, "file-key", resolveAPIKey(cfg)) }) - t.Run("an unusable file gives no key rather than a wrong one", func(t *testing.T) { + t.Run("both variables set gives no key, as the daemon refuses to start", func(t *testing.T) { t.Setenv(config.EnvAPIKey, "env-key") + t.Setenv(config.EnvAPIKeyFile, keyFile) + assert.Empty(t, resolveAPIKey(cfg)) + }) + t.Run("an unusable file gives no key rather than a wrong one", func(t *testing.T) { + t.Setenv(config.EnvAPIKey, "") t.Setenv(config.EnvAPIKeyFile, filepath.Join(t.TempDir(), "missing")) assert.Empty(t, resolveAPIKey(cfg)) }) diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 01d0acc55..7f7a1f0b4 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -551,18 +551,11 @@ func runServer(cmd *cobra.Command, _ []string) error { // Ensure API key is configured apiKey, wasGenerated, source, keyErr := cfg.EnsureAPIKey() if keyErr != nil { - // MCPPROXY_API_KEY_FILE names a file that cannot be used. Stop here: - // generating and saving a key instead would replace the credential the - // operator meant to run with. + // Both MCPPROXY_API_KEY and MCPPROXY_API_KEY_FILE are set, or the file + // cannot be used. Stop here: generating and saving a key instead would + // replace the credential the operator meant to run with. return keyErr } - if source == config.APIKeySourceEnvironmentFile { - if res, resErr := config.ResolveEnvAPIKey(); resErr == nil && res.Shadowed { - logger.Warn("Both MCPPROXY_API_KEY and MCPPROXY_API_KEY_FILE are set; using the key from the file", - zap.String("api_key_file", res.Path)) - } - } - // SECURITY: API key is always required - empty keys are never allowed if apiKey == "" { return fmt.Errorf("API key is required but could not be generated") diff --git a/docs/configuration.md b/docs/configuration.md index 705545cff..daa71155a 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1836,7 +1836,7 @@ Many configuration options can be overridden via environment variables: |----------------------|--------------|-------------| | `MCPPROXY_LISTEN` / `MCPP_LISTEN` | `listen` | Network binding address | | `MCPPROXY_API_KEY` | `api_key` | API key for authentication (empty values trigger auto-generation; auth remains enabled) | -| `MCPPROXY_API_KEY_FILE` | `api_key` | Absolute path of a file that holds the API key; wins over `MCPPROXY_API_KEY`; an unusable file stops startup (no fallback, nothing generated) | +| `MCPPROXY_API_KEY_FILE` | `api_key` | Absolute path of a file that holds the API key; set it or `MCPPROXY_API_KEY`, not both (both set stops startup with an error naming both); an unusable file stops startup (no fallback, nothing generated) | | `MCPPROXY_TRUSTED_HOSTS` | `trusted_hosts` | Comma-separated `Host` allowlist for loopback listeners behind a reverse proxy | | `MCPPROXY_TRUSTED_PROXIES` | `trusted_proxies` | Comma-separated CIDRs/IPs whose `X-Forwarded-*` headers are honoured | | `MCPPROXY_AUDIT_LOG_ENABLED` | `audit_log.enabled` | Turn audit logging on/off | diff --git a/docs/configuration/environment-variables.md b/docs/configuration/environment-variables.md index ee00a670e..3d06bb214 100644 --- a/docs/configuration/environment-variables.md +++ b/docs/configuration/environment-variables.md @@ -30,7 +30,7 @@ Environment variables are useful for CI/CD environments or temporary overrides d |----------|-------------|---------| | `MCPPROXY_LISTEN` | Override listen address | `127.0.0.1:8080` or `:8080` | | `MCPPROXY_API_KEY` | Set API key for authentication | `my-secret-key` | -| `MCPPROXY_API_KEY_FILE` | Read the API key from a file (absolute path). Takes precedence over `MCPPROXY_API_KEY`; see [API key from a file](#api-key-from-a-file) | `/run/secrets/mcpproxy_api_key` | +| `MCPPROXY_API_KEY_FILE` | Read the API key from a file (absolute path). Set it or `MCPPROXY_API_KEY`, not both; see [API key from a file](#api-key-from-a-file) | `/run/secrets/mcpproxy_api_key` | | `MCPPROXY_DATA_DIR` | Override data directory | `/var/lib/mcpproxy` | | `MCPPROXY_DATA` | Override data directory (backward compatibility, prefer `MCPPROXY_DATA_DIR`) | `/var/lib/mcpproxy` | @@ -45,10 +45,14 @@ from the file at start, so it is not in the process environment (`docker inspect point at a regular file. At most 64 KiB is read. - One trailing newline (LF or CRLF) is removed. Everything else, including inner whitespace, is kept. - An empty `MCPPROXY_API_KEY_FILE` is treated as unset. -- If `MCPPROXY_API_KEY` is also set, the file wins and the log says so. +- Set `MCPPROXY_API_KEY` or `MCPPROXY_API_KEY_FILE`, never both. If both are non-empty, mcpproxy + refuses to start (and the CLI commands and the tray stop) with an error that names both + variables: `MCPPROXY_API_KEY and MCPPROXY_API_KEY_FILE are both set; set only one of them`. The + check does not open the file, and the error never contains a key or file contents. An empty + variable counts as unset. - A file that is named but cannot be used (missing, unreadable, not absolute, too large, empty) stops startup with an error that names the variable and path. mcpproxy does **not** fall back to - `MCPPROXY_API_KEY`, the config file or a generated key, so a typo in a secret path cannot start the + the config file or a generated key, so a typo in a secret path cannot start the proxy under a different credential. - `mcpproxy status`, the other CLI commands and the tray read the same variable, so they find the key the daemon uses. A tray-launched core reads the file itself rather than receiving the key in its diff --git a/docs/getting-started/installation.md b/docs/getting-started/installation.md index 2f7533658..1772c7f9d 100644 --- a/docs/getting-started/installation.md +++ b/docs/getting-started/installation.md @@ -398,7 +398,7 @@ docker run -d --name mcpproxy \ copy — the image is distroless (no shell), so reading an auto-generated key back out of the container is awkward. To keep the key out of `docker inspect` and the process environment, mount it as a secret and set - `MCPPROXY_API_KEY_FILE=/run/secrets/` instead (see + `MCPPROXY_API_KEY_FILE=/run/secrets/` instead of `MCPPROXY_API_KEY` (setting both is an error; see [API key from a file](/configuration/environment-variables#api-key-from-a-file)). - **Web UI**: `http://localhost:8080/ui/`. diff --git a/internal/config/api_key_env.go b/internal/config/api_key_env.go index 8319bb7ac..8ba90b7ac 100644 --- a/internal/config/api_key_env.go +++ b/internal/config/api_key_env.go @@ -1,6 +1,7 @@ package config import ( + "errors" "fmt" "os" "strings" @@ -33,8 +34,20 @@ type EnvAPIKeyResult struct { FromFile bool // Path is the file Key was read from, when FromFile. Path string - // Shadowed is true when both variables carry a key and the file won. - Shadowed bool +} + +// ErrAPIKeyBothSet is returned when MCPPROXY_API_KEY and MCPPROXY_API_KEY_FILE +// are both set to a non-empty value. The text is fixed: it names the two +// variables and nothing else, so it can never carry a key or file contents. +var ErrAPIKeyBothSet = errors.New(EnvAPIKey + " and " + EnvAPIKeyFile + " are both set; set only one of them") + +// checkEnvAPIKeyConflict returns ErrAPIKeyBothSet when both variables are +// non-empty. It reads no file. +func checkEnvAPIKeyConflict() error { + if os.Getenv(EnvAPIKey) != "" && os.Getenv(EnvAPIKeyFile) != "" { + return ErrAPIKeyBothSet + } + return nil } // ResolveEnvAPIKey is the one place the environment's API key is read. Every @@ -42,11 +55,13 @@ type EnvAPIKeyResult struct { // core it launches) goes through it so the two variables mean the same thing // everywhere. // -// MCPPROXY_API_KEY_FILE wins over MCPPROXY_API_KEY when both are set (an -// explicit file is the more deliberate choice and the one that keeps the key -// out of the environment); the result reports Shadowed so a caller can warn. -// An empty MCPPROXY_API_KEY_FILE is treated as unset, as Compose files that -// interpolate an optional variable produce one. +// Setting both MCPPROXY_API_KEY and MCPPROXY_API_KEY_FILE to a non-empty value +// is an error (ErrAPIKeyBothSet), whatever the file holds and whether or not it +// exists: there is one rule, and no variable silently shadows the other. The +// check looks at the variables only and does not open the file. An empty +// variable counts as unset, as Compose files that interpolate an optional +// variable produce one. (An empty MCPPROXY_API_KEY alone still disables +// authentication, as before.) // // A file that is named but cannot be used (missing, unreadable, not absolute, // too large, empty after removing its trailing newline) is an error. The caller @@ -54,8 +69,13 @@ type EnvAPIKeyResult struct { // would start the proxy under a credential the operator did not choose. func ResolveEnvAPIKey() (EnvAPIKeyResult, error) { envKey, envSet := os.LookupEnv(EnvAPIKey) + path := os.Getenv(EnvAPIKeyFile) + + if err := checkEnvAPIKeyConflict(); err != nil { + return EnvAPIKeyResult{}, err + } - if path := os.Getenv(EnvAPIKeyFile); path != "" { + if path != "" { key, err := secret.ReadFile(path) if err != nil { return EnvAPIKeyResult{}, fmt.Errorf("%s: %w", EnvAPIKeyFile, err) @@ -68,7 +88,6 @@ func ResolveEnvAPIKey() (EnvAPIKeyResult, error) { Key: key, FromFile: true, Path: path, - Shadowed: envSet && envKey != "", }, nil } diff --git a/internal/config/api_key_env_test.go b/internal/config/api_key_env_test.go index 33c1e009e..2b3d9fd80 100644 --- a/internal/config/api_key_env_test.go +++ b/internal/config/api_key_env_test.go @@ -42,7 +42,6 @@ func TestResolveEnvAPIKey(t *testing.T) { assert.True(t, res.Set) assert.Equal(t, "env-key", res.Key) assert.False(t, res.FromFile) - assert.False(t, res.Shadowed) }) t.Run("an empty key variable is still set (it disables authentication)", func(t *testing.T) { @@ -64,7 +63,6 @@ func TestResolveEnvAPIKey(t *testing.T) { assert.Equal(t, "file-key", res.Key, "one trailing newline is removed") assert.True(t, res.FromFile) assert.Equal(t, path, res.Path) - assert.False(t, res.Shadowed) }) t.Run("CRLF file", func(t *testing.T) { @@ -75,14 +73,13 @@ func TestResolveEnvAPIKey(t *testing.T) { assert.Equal(t, "file-key", res.Key) }) - t.Run("both set: the file wins and the result says so", func(t *testing.T) { - t.Setenv(EnvAPIKey, "env-key") + t.Run("an empty key variable and a file is not a conflict", func(t *testing.T) { + t.Setenv(EnvAPIKey, "") t.Setenv(EnvAPIKeyFile, writeKeyFile(t, "file-key")) res, err := ResolveEnvAPIKey() require.NoError(t, err) assert.Equal(t, "file-key", res.Key) assert.True(t, res.FromFile) - assert.True(t, res.Shadowed) }) t.Run("an empty file variable is unset", func(t *testing.T) { @@ -95,10 +92,9 @@ func TestResolveEnvAPIKey(t *testing.T) { }) } -// A named file that cannot be used is an error and never falls back to the -// other variable, nor to a generated key. +// A named file that cannot be used is an error and never falls back to a +// generated key. func TestResolveEnvAPIKey_UnusableFileIsAnError(t *testing.T) { - const fallback = "must-not-be-used" cases := map[string]string{ "missing": filepath.Join(t.TempDir(), "nope"), "relative": "run/secrets/key", @@ -109,22 +105,100 @@ func TestResolveEnvAPIKey_UnusableFileIsAnError(t *testing.T) { } for name, path := range cases { t.Run(name, func(t *testing.T) { - t.Setenv(EnvAPIKey, fallback) + unsetEnv(t, EnvAPIKey) t.Setenv(EnvAPIKeyFile, path) res, err := ResolveEnvAPIKey() require.Error(t, err) + assert.NotErrorIs(t, err, ErrAPIKeyBothSet) assert.Contains(t, err.Error(), EnvAPIKeyFile) assert.False(t, res.Set) - assert.NotContains(t, err.Error(), fallback) }) } } -func TestEnsureAPIKey_FileOverridesConfigAndEnv(t *testing.T) { +// Setting both variables is one error, whatever the file holds or whether it +// exists. The text names both variables and never a key or file contents. +func TestResolveEnvAPIKey_BothSetIsAnError(t *testing.T) { + const envSecret = "env-secret-value-1234" + const fileSecret = "file-secret-value-5678" + cases := map[string]string{ + "readable file": writeKeyFile(t, fileSecret+"\n"), + "missing file": filepath.Join(t.TempDir(), "missing"), + "relative path": "run/secrets/key", + "empty file": writeKeyFile(t, ""), + "directory": t.TempDir(), + "unrelated string": "not-a-path", + } + for name, path := range cases { + t.Run(name, func(t *testing.T) { + t.Setenv(EnvAPIKey, envSecret) + t.Setenv(EnvAPIKeyFile, path) + res, err := ResolveEnvAPIKey() + require.Error(t, err) + assert.ErrorIs(t, err, ErrAPIKeyBothSet) + assert.Contains(t, err.Error(), EnvAPIKey+" ") + assert.Contains(t, err.Error(), EnvAPIKeyFile) + assert.NotContains(t, err.Error(), envSecret) + assert.NotContains(t, err.Error(), fileSecret) + assert.NotContains(t, err.Error(), path, "the conflict is reported without opening or naming the file") + assert.Equal(t, EnvAPIKeyResult{}, res, "no key is returned from either variable") + }) + } +} + +func TestEnsureAPIKey_BothSetIsAnErrorAndNeverGeneratesAKey(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + t.Setenv(EnvAPIKey, "env-secret-value-1234") + t.Setenv(EnvAPIKeyFile, writeKeyFile(t, "file-secret-value-5678\n")) + + for name, cfg := range map[string]*Config{ + "no key configured": {}, + "key configured": {APIKey: "config-key", apiKeyExplicitlySet: true}, + } { + t.Run(name, func(t *testing.T) { + before := cfg.APIKey + key, generated, _, err := cfg.EnsureAPIKey() + require.ErrorIs(t, err, ErrAPIKeyBothSet) + assert.Contains(t, err.Error(), EnvAPIKey) + assert.Contains(t, err.Error(), EnvAPIKeyFile) + assert.Empty(t, key) + assert.False(t, generated) + assert.Equal(t, before, cfg.APIKey, "no key was generated or replaced") + }) + } +} + +// The load refuses both-set even when the config file carries its own api_key, +// so no entry point that loads the config can start under an ambiguous setting. +func TestLoad_BothSetFailsTheLoad(t *testing.T) { + for name, body := range map[string]string{ + "no api_key in the config": `{"listen": "127.0.0.1:8080", "mcpServers": []}`, + "api_key in the config": `{"listen": "127.0.0.1:8080", "api_key": "config-key", "mcpServers": []}`, + } { + t.Run(name, func(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, body) + t.Setenv(EnvAPIKey, "env-secret-value-1234") + t.Setenv(EnvAPIKeyFile, writeKeyFile(t, "file-secret-value-5678\n")) + + _, err := LoadFromFile(path) + require.Error(t, err) + assert.ErrorIs(t, err, ErrAPIKeyBothSet) + assert.Contains(t, err.Error(), EnvAPIKey) + assert.Contains(t, err.Error(), EnvAPIKeyFile) + assert.NotContains(t, err.Error(), "env-secret-value") + assert.NotContains(t, err.Error(), "file-secret-value") + }) + } +} + +func TestEnsureAPIKey_FileOverridesConfig(t *testing.T) { t.Cleanup(ResetProcessOverrides) ResetProcessOverrides() path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "api_key": "config-key", "mcpServers": []}`) - t.Setenv(EnvAPIKey, "env-key") + unsetEnv(t, EnvAPIKey) t.Setenv(EnvAPIKeyFile, writeKeyFile(t, "file-key\n")) cfg, err := LoadFromFile(path) diff --git a/internal/config/config.go b/internal/config/config.go index 7070f5d63..ea073611f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2289,14 +2289,15 @@ func (sc *ServerConfig) IsToolAllowedByConfig(toolName string) bool { // Returns the API key, whether it was auto-generated, and the source // SECURITY: Empty API keys are never allowed - always auto-generates if empty or missing // -// The error is non-nil only when MCPPROXY_API_KEY_FILE names a file that cannot +// The error is non-nil only when MCPPROXY_API_KEY and MCPPROXY_API_KEY_FILE are +// both set (ErrAPIKeyBothSet), or MCPPROXY_API_KEY_FILE names a file that cannot // be used. The caller must stop in that case: no key is generated and nothing // is saved, so a typo in a secret path can never replace the key the operator // meant to run with. func (c *Config) EnsureAPIKey() (apiKey string, wasGenerated bool, source APIKeySource, err error) { - // The environment (MCPPROXY_API_KEY_FILE, else MCPPROXY_API_KEY) overrides - // the config file. Use the result's Set/Key to distinguish "not set" from - // "set to empty string". + // The environment (MCPPROXY_API_KEY_FILE or MCPPROXY_API_KEY, never both) + // overrides the config file. Use the result's Set/Key to distinguish + // "not set" from "set to empty string". envKey, err := ResolveEnvAPIKey() if err != nil { return "", false, APIKeySourceEnvironment, err @@ -2965,11 +2966,18 @@ func (c *Config) Validate() error { } c.profileWarnings = profileWarnings + // Both key variables set is an error whether or not the config file + // carries an api_key: one rule, checked on every load. + if err := checkEnvAPIKeyConflict(); err != nil { + return err + } + // Handle API key generation if not configured // Empty string means authentication disabled, nil means auto-generate if c.APIKey == "" { - // Check the environment for an API key (MCPPROXY_API_KEY_FILE, else - // MCPPROXY_API_KEY). A file that is named but unusable fails the load: + // Check the environment for an API key (MCPPROXY_API_KEY_FILE or + // MCPPROXY_API_KEY; both set is an error). A file that is named but + // unusable fails the load: // continuing would auto-generate a key and save it. envKey, err := ResolveEnvAPIKey() if err != nil { diff --git a/specs/111-personal-docker-image/spec.md b/specs/111-personal-docker-image/spec.md index dbf5024fb..0bf130158 100644 --- a/specs/111-personal-docker-image/spec.md +++ b/specs/111-personal-docker-image/spec.md @@ -193,6 +193,7 @@ An operator stores upstream tokens as Docker or Kubernetes secrets mounted as fi 2. **Given** a missing or unreadable file, **Then** the server shows a missing-secret error naming the reference (not the value) and the proxy stays up. 3. **Given** `MCPPROXY_API_KEY_FILE` naming a readable file that holds the proxy's own API key, **When** the proxy starts, **Then** it authenticates with that key, generates and saves no key, the key is absent from the process environment and the config file, and the log names the variable and path but not the key. 4. **Given** `MCPPROXY_API_KEY_FILE` naming a missing, unreadable or empty file, **Then** startup fails with an error naming the variable and path, and no key is generated or saved. This is the one case where a missing file is fatal rather than shown as a missing-secret error: the proxy's own credential has no safe fallback. +5. **Given** both `MCPPROXY_API_KEY` and `MCPPROXY_API_KEY_FILE` set to non-empty values, **Then** startup fails with an error naming both variables and containing no key or file content, whatever the file holds, and no key is generated or saved. --- @@ -253,7 +254,7 @@ The maintainer wants to know how many installs use each official image and needs - **FR-011**: Both variants MUST run as a fixed non-root user (UID/GID 65532) by default and MUST work under an arbitrary non-root UID supplied by the orchestrator when the data volume is writable by that UID. - **FR-012**: Both variants MUST ship `/data` owned by the runtime user, so that a fresh named volume starts without any manual ownership step. - **FR-013**: When the data volume fails a write probe by the running user (regardless of its recorded owner, so a group-writable or `fsGroup`-adjusted volume passes), the container MUST exit with the documented permission exit code (5) and a message that names the volume path, the expected owner and the one-line fix. [Amended: the trigger is write-probe failure only, not ownership, so it does not contradict FR-011's arbitrary-UID support.] -- **FR-014**: When no API key is supplied, the first start MUST generate one, save it on the data volume, and log a message naming the FR-014 read-back command (`mcpproxy config get-api-key`) without printing the key itself, and the operator MUST be able to print the saved key by running `docker exec mcpproxy config get-api-key` (a new, no-shell CLI subcommand this spec adds; the exact form is `mcpproxy config get-api-key [--config PATH]`, matching the existing `--config`/`-c` flag convention used by `token`, `doctor` and other subcommands). That command MUST resolve and print the key through a config-only read path that does not open the BBolt database, so a read-back against a running container never contends the database lock (research R12/R13). In a detached (non-TTY) container — the documented and expected deployment mode for this spec's compose/Kubernetes examples — the container log MUST NOT contain the key. [Amended: the existing terminal-only banner behaviour (research R11) prints the key once when stderr is a TTY, e.g. under `docker run -it`, and `docker logs` captures that stream regardless of TTY allocation; this FR does not require changing that existing behaviour, and the documentation MUST say that starting the container with `-it` (or `kubectl exec`-style TTY attach at first boot) will put the key in `docker logs`/`kubectl logs` history, so operators should use the FR-014 read-back command instead of an interactive first run when they want the key kept out of log history.] The saved config file MUST be written with owner-only permissions (0600) so the key is not readable by other UIDs on a shared bind mount regardless of umask. An `MCPPROXY_API_KEY` environment variable set to an empty string MUST be treated the same as not set (a key is generated and saved), matching how Compose/Kubernetes interpolation leaves an unset variable as an empty string rather than an absent one. The API key MAY instead be supplied as a file: when `MCPPROXY_API_KEY_FILE` names an absolute path, the key is read from that file (read rules in FR-030) and counts as a supplied key, so none is generated or saved, and the key never enters the process environment. An empty `MCPPROXY_API_KEY_FILE` is treated as unset, for the same interpolation reason. If both variables carry a key the file wins and the log says so. A named file that cannot be used (missing, unreadable, not absolute, too large, empty) MUST stop startup with an error naming the variable and the path, never the content: the proxy MUST NOT fall back to `MCPPROXY_API_KEY`, the config file or a generated key, because that would run it under a credential the operator did not choose. The key read from the file is a process-only override and is never written back to the config file. +- **FR-014**: When no API key is supplied, the first start MUST generate one, save it on the data volume, and log a message naming the FR-014 read-back command (`mcpproxy config get-api-key`) without printing the key itself, and the operator MUST be able to print the saved key by running `docker exec mcpproxy config get-api-key` (a new, no-shell CLI subcommand this spec adds; the exact form is `mcpproxy config get-api-key [--config PATH]`, matching the existing `--config`/`-c` flag convention used by `token`, `doctor` and other subcommands). That command MUST resolve and print the key through a config-only read path that does not open the BBolt database, so a read-back against a running container never contends the database lock (research R12/R13). In a detached (non-TTY) container — the documented and expected deployment mode for this spec's compose/Kubernetes examples — the container log MUST NOT contain the key. [Amended: the existing terminal-only banner behaviour (research R11) prints the key once when stderr is a TTY, e.g. under `docker run -it`, and `docker logs` captures that stream regardless of TTY allocation; this FR does not require changing that existing behaviour, and the documentation MUST say that starting the container with `-it` (or `kubectl exec`-style TTY attach at first boot) will put the key in `docker logs`/`kubectl logs` history, so operators should use the FR-014 read-back command instead of an interactive first run when they want the key kept out of log history.] The saved config file MUST be written with owner-only permissions (0600) so the key is not readable by other UIDs on a shared bind mount regardless of umask. An `MCPPROXY_API_KEY` environment variable set to an empty string MUST be treated the same as not set (a key is generated and saved), matching how Compose/Kubernetes interpolation leaves an unset variable as an empty string rather than an absent one. The API key MAY instead be supplied as a file: when `MCPPROXY_API_KEY_FILE` names an absolute path, the key is read from that file (read rules in FR-030) and counts as a supplied key, so none is generated or saved, and the key never enters the process environment. An empty `MCPPROXY_API_KEY_FILE` is treated as unset, for the same interpolation reason. If both variables are non-empty, startup MUST fail with an error naming both variables and no key or file content; there is no precedence between them. A named file that cannot be used (missing, unreadable, not absolute, too large, empty) MUST stop startup with an error naming the variable and the path, never the content: the proxy MUST NOT fall back to the config file or a generated key, because that would run it under a credential the operator did not choose. The key read from the file is a process-only override and is never written back to the config file. - **FR-015**: The documentation MUST state that one data volume supports exactly one running container, and the Kubernetes example MUST use one replica with a recreate update strategy. - **FR-016**: The image entrypoint MUST run the proxy binary (behind the init process in the full variant) and the default command MUST be `serve` on `0.0.0.0:8080` with the data volume, so that `docker run [flags]` runs any subcommand without repeating the binary name, and no arguments start the server. - **FR-017**: When the first argument is the binary's own name, the image MUST treat it as absent (drop it) rather than passing it through as a subcommand. [Assumption: research D9.] From 77ee89ee3a00bcf5a5272978436febb6c8c2a0af Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 22 Oct 2026 11:40:06 +0300 Subject: [PATCH 5/5] fix(secret): refuse a non-regular secret file before opening it open(2) on a FIFO blocks until a writer appears, so a mis-pointed MCPPROXY_API_KEY_FILE or ${file:} reference hung startup before the regular-file check could run. Stat the path first and keep the check on the open descriptor for a path swapped in between. --- internal/secret/file_read.go | 10 ++++++ internal/secret/file_read_fifo_unix_test.go | 39 +++++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 internal/secret/file_read_fifo_unix_test.go diff --git a/internal/secret/file_read.go b/internal/secret/file_read.go index 1add4685b..7db38bfc9 100644 --- a/internal/secret/file_read.go +++ b/internal/secret/file_read.go @@ -43,6 +43,16 @@ func ReadFile(path string) (string, error) { return "", fmt.Errorf("secret file path %q must be absolute", path) } + // Check the type before opening: open(2) on a FIFO blocks until a writer + // appears, and on some devices it has side effects, so a mis-pointed path + // must be refused without ever reaching open. The check is repeated on the + // open descriptor so a path swapped in between is still refused. + if info, err := os.Stat(path); err != nil { + return "", fmt.Errorf("cannot read secret file %q: %w", path, unwrapPathError(err)) + } else if !info.Mode().IsRegular() { + return "", fmt.Errorf("secret file %q is not a regular file", path) + } + f, err := os.Open(path) if err != nil { return "", fmt.Errorf("cannot read secret file %q: %w", path, unwrapPathError(err)) diff --git a/internal/secret/file_read_fifo_unix_test.go b/internal/secret/file_read_fifo_unix_test.go new file mode 100644 index 000000000..08a954c1d --- /dev/null +++ b/internal/secret/file_read_fifo_unix_test.go @@ -0,0 +1,39 @@ +//go:build unix + +package secret + +import ( + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// A FIFO with no writer makes open(2) block forever. ReadFile must refuse it +// from the path's type before opening, so a mis-pointed MCPPROXY_API_KEY_FILE +// or ${file:} reference fails fast instead of hanging startup. +func TestReadFile_RefusesFIFOWithoutBlocking(t *testing.T) { + fifo := filepath.Join(t.TempDir(), "secret.fifo") + require.NoError(t, syscall.Mkfifo(fifo, 0o600)) + + type result struct { + val string + err error + } + done := make(chan result, 1) + go func() { + v, err := ReadFile(fifo) + done <- result{v, err} + }() + + select { + case r := <-done: + require.Error(t, r.err) + require.Contains(t, r.err.Error(), "not a regular file") + require.Empty(t, r.val) + case <-time.After(5 * time.Second): + t.Fatal("ReadFile blocked on a FIFO instead of refusing it") + } +}