diff --git a/CHANGELOG.md b/CHANGELOG.md index b3eb0f269..f58228337 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. 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 `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..daa44045b 100644 --- a/cmd/mcpproxy-tray/main.go +++ b/cmd/mcpproxy-tray/main.go @@ -156,14 +156,30 @@ 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 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"), + 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 +1951,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..05d0a37f4 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", "") + 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..6a99b0aac 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,21 @@ 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 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 { - if envAPIKey, exists := os.LookupEnv("MCPPROXY_API_KEY"); exists && envAPIKey != "" { - return envAPIKey + envKey, err := config.ResolveEnvAPIKey() + if err != nil { + // 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 "" + } + 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..a503990ef --- /dev/null +++ b/cmd/mcpproxy/daemon_client_api_key_test.go @@ -0,0 +1,45 @@ +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 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("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 30f03bbb8..7f7a1f0b4 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -549,8 +549,13 @@ 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 { + // 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 + } // 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/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..daa71155a 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; 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 bd7b1b433..3d06bb214 100644 --- a/docs/configuration/environment-variables.md +++ b/docs/configuration/environment-variables.md @@ -30,9 +30,46 @@ 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). 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` | +### 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. +- 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 + 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..1772c7f9d 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 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/`. ### 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..8ba90b7ac --- /dev/null +++ b/internal/config/api_key_env.go @@ -0,0 +1,95 @@ +package config + +import ( + "errors" + "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 +} + +// 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 +// 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. +// +// 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 +// 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) + path := os.Getenv(EnvAPIKeyFile) + + if err := checkEnvAPIKeyConflict(); err != nil { + return EnvAPIKeyResult{}, err + } + + if 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, + }, 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..2b3d9fd80 --- /dev/null +++ b/internal/config/api_key_env_test.go @@ -0,0 +1,268 @@ +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) + }) + + 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) + }) + + 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("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) + }) + + 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 a +// generated key. +func TestResolveEnvAPIKey_UnusableFileIsAnError(t *testing.T) { + 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) { + 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) + }) + } +} + +// 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": []}`) + unsetEnv(t, EnvAPIKey) + 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..ea073611f 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,43 @@ 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 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 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 + } + 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) + } + if envKey.FromFile { + return c.APIKey, false, APIKeySourceEnvironmentFile, nil } - return c.APIKey, false, APIKeySourceEnvironment + 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 @@ -2949,16 +2966,28 @@ 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 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 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 { + 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..7db38bfc9 --- /dev/null +++ b/internal/secret/file_read.go @@ -0,0 +1,100 @@ +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) + } + + // 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)) + } + 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_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") + } +} diff --git a/internal/secret/file_read_test.go b/internal/secret/file_read_test.go new file mode 100644 index 000000000..5952f7201 --- /dev/null +++ b/internal/secret/file_read_test.go @@ -0,0 +1,123 @@ +package secret + +import ( + "os" + "path/filepath" + "runtime" + "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 runtime.GOOS == "windows" { + t.Skip("file permission bits are not enforced on Windows") + } + 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/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 diff --git a/specs/111-personal-docker-image/spec.md b/specs/111-personal-docker-image/spec.md index 08b44fc26..0bf130158 100644 --- a/specs/111-personal-docker-image/spec.md +++ b/specs/111-personal-docker-image/spec.md @@ -191,6 +191,9 @@ 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. +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. --- @@ -251,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. +- **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.] @@ -277,7 +280,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**