diff --git a/CHANGELOG.md b/CHANGELOG.md index 5501a4689..608f2a603 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,8 @@ Releases follow [Semantic Versioning](https://semver.org/). document per profile, in mcpproxy's own profile format. Also `GET /api/v1/access/suggest` and the `suggest` operation of the `profiles` MCP tool (administrators only). - **`mcp_reject_query_apikey` refuses query-parameter credentials on MCP endpoints.** Set it to `true` and every MCP endpoint (`/mcp`, `/mcp/p/`, `/mcp/all`, `/mcp/code`, `/mcp/call` and the legacy `/v1/tool_code` aliases) answers a request carrying `?apikey=` with a 401 instead of honouring it, which forces every client onto `Authorization: Bearer` or `X-API-Key`. It cannot stop a URL that was already sent from being logged by whatever sits in front of the proxy; it makes sure such a client stops working until it is migrated, so the token stops appearing in access logs, proxy logs and browser history from then on. The refusal is explicit rather than "no credential" because, with `require_mcp_auth` off, an unrecognised credential would otherwise be served as the anonymous administrator. Default `false`, so nothing changes unless you opt in; the REST API, the Web UI and its `/events` stream are not affected. Live, no restart. **Caveat:** the Connect wizard writes Codex (and the generic fallback) entries with `?apikey=` in the URL, so a client connected that way gets a 401 once the option is on: remove the query parameter and carry the credential in a header instead (`env_http_headers` or `http_headers`, see the Connect clients guide); Connect itself is not yet aware of the option. +- **secrets: `${file:/absolute/path}` references (#1706, spec 111 FR-030).** A secret can be read from a file, as Docker/Compose secrets, Kubernetes secret volumes and systemd `LoadCredential=` provide it, wherever `${env:}` and `${keyring:}` are accepted: an upstream server's `env`, `args` and `headers`, `data_dir`, and the proxy's own `api_key`. The path must be absolute, a symlink must lead to a regular file, at most 64 KiB is read, one trailing LF or CRLF is removed, and a missing, unreadable or empty file is an error that names the reference, never the content. A server shows a missing-secret error and the proxy stays up; an `api_key` reference that does not resolve stops startup (no key is generated over it). The stored config keeps the reference: the value is resolved on a copy, and the `api_key` secret is a process-only override that no save, including `PATCH /api/v1/config`, writes back. (refs #1706) + - **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 diff --git a/docs/configuration/environment-variables.md b/docs/configuration/environment-variables.md index 7d620b797..9f5a16aac 100644 --- a/docs/configuration/environment-variables.md +++ b/docs/configuration/environment-variables.md @@ -50,6 +50,8 @@ from the file at start, so it is not in the process environment (`docker inspect 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. +- The same file can instead be referenced from the config as `"api_key": "${file:/run/secrets/mcpproxy_api_key}"` + (see [File references](../features/keyring-integration.md#file-references)). - 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 diff --git a/docs/configuration/upstream-servers.md b/docs/configuration/upstream-servers.md index 6a2c6656b..a4d5296af 100644 --- a/docs/configuration/upstream-servers.md +++ b/docs/configuration/upstream-servers.md @@ -304,13 +304,14 @@ endpoint that backs the "Convert to secret" affordance. ### Secret references -`headers` and `env` values support two reference shapes that get resolved +`headers` and `env` values support three reference shapes that get resolved at request time rather than stored in plaintext: | Reference | Source | Lifetime | |---|---|---| | `${keyring:NAME}` | OS keyring entry called `NAME` | Persists across restarts; managed via the Secrets page or `mcpproxy secrets` CLI | | `${env:VAR}` | The `VAR` environment variable on the mcpproxy process | Tied to the shell/launcher that started mcpproxy | +| `${file:/absolute/path}` | The contents of a file (Docker/Kubernetes secret, systemd credential), one trailing newline removed | Read each time the server's connection is set up, so a rotated secret is picked up when the server is next restarted | When mcpproxy connects to an upstream it substitutes the reference with the actual secret. The reference itself never reaches the upstream MCP diff --git a/docs/features/keyring-integration.md b/docs/features/keyring-integration.md index 21f3b79ee..9fcc0ff99 100644 --- a/docs/features/keyring-integration.md +++ b/docs/features/keyring-integration.md @@ -40,12 +40,43 @@ All secrets are stored under the service name **`mcpproxy`**. ## Secret Reference Syntax -MCPProxy supports two types of secret references that can be used anywhere in your configuration where a string value is expected: +MCPProxy supports three types of secret references that can be used anywhere in your configuration where a string value is expected: | Syntax | Provider | Description | |--------|----------|-------------| | `${keyring:name}` | OS Keyring | Resolves from macOS Keychain, Linux Secret Service, or Windows Credential Manager | | `${env:VAR_NAME}` | Environment | Resolves from environment variables | +| `${file:/absolute/path}` | File | Reads the secret from a file, as Docker/Compose secrets (`/run/secrets/`), Kubernetes secret volumes and systemd `LoadCredential=` provide it | + +### File references + +`${file:/absolute/path}` keeps a secret out of the config file and out of the process environment +(`docker inspect`, `/proc//environ`). The same rules apply wherever it is used: + +- The path must be absolute. A symlink is followed (Kubernetes secret volumes are symlinks) but must + point at a regular file. On Windows an absolute path has a drive letter (`C:\ProgramData\mcpproxy\key` + or `C:/ProgramData/mcpproxy/key`); a path that starts with `/` and has no drive is not absolute. In + `mcp_config.json` a backslash must be escaped (`"${file:C:\\ProgramData\\mcpproxy\\key}"`), so forward + slashes are easier. +- At most 64 KiB is read. A larger file is an error, never a truncated secret. +- One trailing newline is removed, LF or CRLF, so a file written on Windows resolves to the same value + as one written on Linux. Leading and inner whitespace are kept. +- A NUL byte, an empty file, a missing file and an unreadable file are errors. The error names the + reference and the reason, never the file's content. +- The file is read each time the reference is resolved, which for an upstream server is when its + connection is set up. A rotated secret is picked up when the server is next restarted. +- The stored config keeps the reference; the value exists only in memory, on a copy. + +The proxy's own `api_key` accepts the same reference (`"api_key": "${file:/run/secrets/mcpproxy_api_key}"`), +and so does the `MCPPROXY_API_KEY_FILE` environment variable (see +[API key from a file](../configuration/environment-variables.md#api-key-from-a-file)). An `api_key` +reference that cannot be resolved stops startup: mcpproxy does not generate a key over it. The +reference is resolved when the config is loaded and the key then stays fixed while the process runs (as for +every `api_key` change), so a rotated key file takes effect on the next restart. An API key from the environment overrides the config file's, and then the reference +is not read. + +A `${file:}` reference reads any file the mcpproxy process can read, so treat who can edit server +configuration as you would for `${env:}`. References can be used in: diff --git a/internal/config/api_key_env.go b/internal/config/api_key_env.go index 8ba90b7ac..2078be534 100644 --- a/internal/config/api_key_env.go +++ b/internal/config/api_key_env.go @@ -1,6 +1,7 @@ package config import ( + "context" "errors" "fmt" "os" @@ -93,3 +94,64 @@ func ResolveEnvAPIKey() (EnvAPIKeyResult, error) { return EnvAPIKeyResult{Set: envSet, Key: envKey}, nil } + +// resolveAPIKeyRef replaces an api_key that is a secret reference with the +// value it points at, as a process-only override: the effective config runs +// with the secret, while every save writes the reference (never the secret) +// back, whatever else the save is about (see process_overrides.go). +// +// The reference is resolved here, once per load, and a reference that cannot be +// resolved fails the load. Generating a key instead would start the proxy +// under a credential the operator did not choose and save it over the +// reference. +// +// A key from the environment (MCPPROXY_API_KEY_FILE or MCPPROXY_API_KEY) +// overrides the config file's key, so with one present the reference is left +// as written, unread, for EnsureAPIKey's override to replace. +func resolveAPIKeyRef(c *Config) error { + if !secret.IsSecretRef(c.APIKey) { + return nil + } + envKey, err := ResolveEnvAPIKey() + if err != nil { + return err + } + if envKey.Set && envKey.Key != "" { + return nil + } + resolved, err := secret.NewResolver().ExpandSecretRefs(context.Background(), c.APIKey) + if err != nil { + // The error is deliberately a fixed sentence: ExpandSecretRefs' own + // text quotes the reference, and the api_key field is the one value + // no error or log line may carry. The operator knows which field it is. + return errors.New("api_key: could not resolve the secret reference; check that the referenced file, environment variable or keyring entry exists and is readable") + } + if resolved == "" { + return fmt.Errorf("api_key: the reference resolved to an empty value") + } + // api_key is fixed for the life of the process (a reload pins the running + // value), and this load may be one that never becomes the live config, such + // as the disk re-read of a server restart. Once a reference has been + // resolved, later loads carry the key the process runs with, not what the + // file holds now: recording a rotated value as the process value would make + // the running key look like an edit, and the next save would write it over + // the reference. The file is still read above, so an unusable reference + // fails the load; a rotated key takes effect on the next start. + if running, ok := runningAPIKeyFromReference(); ok { + resolved = running + } + OverrideForProcess(c, FieldAPIKey, OverrideSourceEnv, resolved) + return nil +} + +// runningAPIKeyFromReference returns the api_key this process runs with when it +// was resolved from a secret reference. +func runningAPIKeyFromReference() (string, bool) { + processOverridesMu.RLock() + defer processOverridesMu.RUnlock() + o, ok := processOverrides[overrideKey{FieldAPIKey.Name, OverrideSourceEnv}].(typedOverride[string]) + if !ok || !secret.IsSecretRef(o.loaded) { + return "", false + } + return o.process, true +} diff --git a/internal/config/api_key_env_test.go b/internal/config/api_key_env_test.go index 2b3d9fd80..3fb6aff18 100644 --- a/internal/config/api_key_env_test.go +++ b/internal/config/api_key_env_test.go @@ -1,12 +1,16 @@ package config import ( + "context" + "encoding/json" "os" "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" ) func writeKeyFile(t *testing.T, content string) string { @@ -16,6 +20,21 @@ func writeKeyFile(t *testing.T, content string) string { return path } +// writeAPIKeyConfig writes a minimal mcp_config.json whose api_key is apiKey. +// The JSON is built with encoding/json, never by formatting apiKey into a +// string: a reference such as ${file:C:\Users\x\key} carries backslashes that +// a Windows path puts into the document and that must be escaped. +func writeAPIKeyConfig(t *testing.T, apiKey string) string { + t.Helper() + raw, err := json.Marshal(map[string]any{ + "listen": "127.0.0.1:8080", + "api_key": apiKey, + "mcpServers": []any{}, + }) + require.NoError(t, err) + return writeOverrideTestFile(t, string(raw)) +} + // 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) { @@ -266,3 +285,194 @@ func TestLoad_UnusableKeyFileFailsTheLoad(t *testing.T) { 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) } + +func TestLoad_APIKeyReferenceResolvesAsProcessOverride(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + keyFile := writeKeyFile(t, "ref-secret-key\r\n") + ref := "${file:" + keyFile + "}" + path := writeAPIKeyConfig(t, ref) + unsetEnv(t, EnvAPIKey) + t.Setenv(EnvAPIKeyFile, "") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + assert.Equal(t, "ref-secret-key", cfg.APIKey, "the process runs with the resolved secret") + assert.Contains(t, ProcessOverrideFields(), "api_key") + + key, generated, source, keyErr := cfg.EnsureAPIKey() + require.NoError(t, keyErr) + assert.Equal(t, "ref-secret-key", key) + assert.False(t, generated) + assert.Equal(t, APIKeySourceConfig, source) + + // Any save, here one that is about something else, writes the reference + // and never the secret. + cfg.EnableSocket = !cfg.EnableSocket + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, ref, readJSON(t, path)["api_key"]) + raw, err := os.ReadFile(path) + require.NoError(t, err) + assert.NotContains(t, string(raw), "ref-secret-key") +} + +// PersistableConfig is what a PATCH/PUT of /api/v1/config saves through: the +// merged config carries the resolved secret, the file must keep the reference. +func TestPersistableConfig_APIKeyReferenceSurvivesAnUnrelatedEdit(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + keyFile := writeKeyFile(t, "ref-secret-key\n") + ref := "${file:" + keyFile + "}" + path := writeAPIKeyConfig(t, ref) + unsetEnv(t, EnvAPIKey) + t.Setenv(EnvAPIKeyFile, "") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + + edited := *cfg + edited.ToolsLimit = 7 + persisted := PersistableConfig(&edited, path) + assert.Equal(t, ref, persisted.APIKey) + assert.Equal(t, 7, persisted.ToolsLimit, "the edit itself is kept") +} + +// A load that never becomes the live config (a server restart re-reads the +// whole file) after the key file was rotated must not make the running key look +// like an edit: the next save would write it over the reference. +func TestPersistableConfig_APIKeyReferenceSurvivesARotationAndAnotherLoad(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + keyFile := writeKeyFile(t, "key-A-running\n") + ref := "${file:" + keyFile + "}" + path := writeAPIKeyConfig(t, ref) + unsetEnv(t, EnvAPIKey) + t.Setenv(EnvAPIKeyFile, "") + + live, err := LoadFromFile(path) + require.NoError(t, err) + require.Equal(t, "key-A-running", live.APIKey) + + require.NoError(t, os.WriteFile(keyFile, []byte("key-B-rotated\n"), 0o600)) + again, err := LoadFromFile(path) + require.NoError(t, err) + assert.Equal(t, "key-A-running", again.APIKey, "a rotated key takes effect on the next start, as the running config's does") + + edited := *live + edited.ToolsLimit = 7 + persisted := PersistableConfig(&edited, path) + assert.Equal(t, ref, persisted.APIKey, "the running key is not written over the reference") + require.NoError(t, SaveConfig(&edited, path)) + raw, err := os.ReadFile(path) + require.NoError(t, err) + assert.NotContains(t, string(raw), "key-A-running") + assert.NotContains(t, string(raw), "key-B-rotated") +} + +func TestPersistableConfig_EditingAPIKeyUnderAReferenceIsPersisted(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + keyFile := writeKeyFile(t, "ref-secret-key\n") + path := writeAPIKeyConfig(t, "${file:"+keyFile+"}") + unsetEnv(t, EnvAPIKey) + t.Setenv(EnvAPIKeyFile, "") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + + cfg.APIKey = "operator-chose-this" + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, "operator-chose-this", readJSON(t, path)["api_key"], "an edit of the field is the operator's change") +} + +func TestLoad_APIKeyReferenceThatCannotBeResolvedFailsTheLoad(t *testing.T) { + cases := map[string]string{ + "missing file": "${file:" + filepath.Join(t.TempDir(), "missing") + "}", + "relative": "${file:run/secrets/key}", + "empty file": "${file:" + writeKeyFile(t, "\n") + "}", + "unknown type": "${nope:thing}", + } + for name, ref := range cases { + t.Run(name, func(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeAPIKeyConfig(t, ref) + unsetEnv(t, EnvAPIKey) + t.Setenv(EnvAPIKeyFile, "") + + _, err := LoadFromFile(path) + require.Error(t, err, "no key may be generated over a reference that does not resolve") + assert.Contains(t, err.Error(), "api_key") + assert.NotContains(t, err.Error(), ref, "the error never quotes the reference") + assert.Equal(t, ref, readJSON(t, path)["api_key"], "the file is untouched") + }) + } +} + +func TestLoad_EnvironmentKeyWinsOverAnAPIKeyReferenceWithoutReadingIt(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + // The referenced file does not exist: with an env key the reference is not + // resolved at all, so that is not an error. + ref := "${file:" + filepath.Join(t.TempDir(), "missing") + "}" + path := writeAPIKeyConfig(t, ref) + t.Setenv(EnvAPIKey, "env-key") + t.Setenv(EnvAPIKeyFile, "") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + key, _, source, keyErr := cfg.EnsureAPIKey() + require.NoError(t, keyErr) + assert.Equal(t, "env-key", key) + assert.Equal(t, APIKeySourceEnvironment, source) + + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, ref, readJSON(t, path)["api_key"], "the reference is still what is on disk") +} + +// ${file:} works wherever ${env:} does: an upstream server's env, headers, url +// and args, resolved on a copy so the stored config keeps the reference. +func TestServerConfig_FileReferencesExpandOnACopy(t *testing.T) { + tokenFile := writeKeyFile(t, "upstream-token\n") + orig := &ServerConfig{ + Name: "gh", + URL: "https://example.test/mcp", + Headers: map[string]string{"Authorization": "Bearer ${file:" + tokenFile + "}"}, + Env: map[string]string{"GH_TOKEN": "${file:" + tokenFile + "}"}, + Args: []string{"--token", "${file:" + tokenFile + "}"}, + } + + resolved := CopyServerConfig(orig) + errs := secret.NewResolver().ExpandStructSecretsCollectErrors(context.Background(), resolved) + require.Empty(t, errs) + + assert.Equal(t, "Bearer upstream-token", resolved.Headers["Authorization"]) + assert.Equal(t, "upstream-token", resolved.Env["GH_TOKEN"]) + assert.Equal(t, []string{"--token", "upstream-token"}, resolved.Args) + assert.Contains(t, orig.Env["GH_TOKEN"], "${file:", "the stored config keeps the reference") + assert.Contains(t, orig.Headers["Authorization"], "${file:") +} + +// Both variables set stays an error when api_key is a reference: the reference +// is not read, and the error is the same fixed text. +func TestLoad_BothSetFailsTheLoadEvenWithAnAPIKeyReference(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + ref := "${file:" + writeKeyFile(t, "ref-secret-key\n") + "}" + path := writeAPIKeyConfig(t, ref) + t.Setenv(EnvAPIKey, "env-secret-value-1234") + t.Setenv(EnvAPIKeyFile, writeKeyFile(t, "file-secret-value-5678\n")) + + _, err := LoadFromFile(path) + require.ErrorIs(t, err, ErrAPIKeyBothSet) + assert.NotContains(t, err.Error(), ref) + assert.NotContains(t, err.Error(), "ref-secret-key") +} + +// The helper must produce valid JSON for a Windows-style path, which is what +// the tests above feed it when they run on Windows. +func TestWriteAPIKeyConfig_EscapesBackslashes(t *testing.T) { + ref := `${file:C:\Users\ci\AppData\Local\Temp\key}` + path := writeAPIKeyConfig(t, ref) + assert.Equal(t, ref, readJSON(t, path)["api_key"]) +} diff --git a/internal/config/config.go b/internal/config/config.go index a9cbc88e5..b8fc1dddd 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -3150,6 +3150,13 @@ func (c *Config) Validate() error { return err } + // api_key may itself be a secret reference (api_key: "${file:/run/secrets/k}"), + // so the key can come from a Docker/Kubernetes secret or a systemd credential + // without being written into this file. + if err := resolveAPIKeyRef(c); err != nil { + return err + } + // Handle API key generation if not configured // Empty string means authentication disabled, nil means auto-generate if c.APIKey == "" { diff --git a/internal/secret/file_provider.go b/internal/secret/file_provider.go new file mode 100644 index 000000000..b07882718 --- /dev/null +++ b/internal/secret/file_provider.go @@ -0,0 +1,69 @@ +package secret + +import ( + "context" + "errors" + "fmt" +) + +const ( + // SecretTypeFile is the type of a ${file:/absolute/path} reference. + SecretTypeFile = "file" +) + +// FileProvider resolves ${file:/absolute/path} references from files, the way +// Docker Compose and Swarm secrets (/run/secrets/), Kubernetes secret +// volumes and systemd LoadCredential= deliver a secret. The read rules (absolute +// path, regular file, size cap, one trailing newline removed, no NUL) are those +// of ReadFile. +// +// The file is read each time a reference is resolved; nothing is cached or logged. +type FileProvider struct{} + +// NewFileProvider creates a new file provider. +func NewFileProvider() *FileProvider { + return &FileProvider{} +} + +// CanResolve returns true if this provider can handle the given secret type. +func (p *FileProvider) CanResolve(secretType string) bool { + return secretType == SecretTypeFile +} + +// Resolve reads the secret from the file named by the reference. An empty file +// is an error, as an unset or empty variable is for the env provider: a secret +// that resolves to nothing would silently send an empty credential. +func (p *FileProvider) Resolve(_ context.Context, ref Ref) (string, error) { + if !p.CanResolve(ref.Type) { + return "", fmt.Errorf("file provider cannot resolve secret type: %s", ref.Type) + } + value, err := ReadFile(ref.Name) + if err != nil { + return "", err + } + if value == "" { + return "", fmt.Errorf("secret file %q is empty", ref.Name) + } + return value, nil +} + +// Store is not supported: mounted secret files are owned by the orchestrator. +func (p *FileProvider) Store(_ context.Context, _ Ref, _ string) error { + return errors.New("file provider does not support storing secrets") +} + +// Delete is not supported. +func (p *FileProvider) Delete(_ context.Context, _ Ref) error { + return errors.New("file provider does not support deleting secrets") +} + +// List returns nothing: there is no registry of secret files to enumerate. +func (p *FileProvider) List(_ context.Context) ([]Ref, error) { + return nil, nil +} + +// IsAvailable always returns true; whether a given file exists is reported when +// a reference to it is resolved. +func (p *FileProvider) IsAvailable() bool { + return true +} diff --git a/internal/secret/file_provider_test.go b/internal/secret/file_provider_test.go new file mode 100644 index 000000000..447abceff --- /dev/null +++ b/internal/secret/file_provider_test.go @@ -0,0 +1,96 @@ +package secret + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFileProvider_Resolve(t *testing.T) { + p := NewFileProvider() + assert.True(t, p.CanResolve("file")) + assert.False(t, p.CanResolve("env")) + assert.True(t, p.IsAvailable()) + + path := writeSecretFile(t, "tok-123\n") + got, err := p.Resolve(context.Background(), Ref{Type: "file", Name: path}) + require.NoError(t, err) + assert.Equal(t, "tok-123", got) + + _, err = p.Resolve(context.Background(), Ref{Type: "env", Name: path}) + require.Error(t, err) + + _, err = p.Resolve(context.Background(), Ref{Type: "file", Name: writeSecretFile(t, "\n")}) + require.Error(t, err, "an empty secret is an error, not an empty credential") + + require.Error(t, p.Store(context.Background(), Ref{Type: "file", Name: path}, "x")) + require.Error(t, p.Delete(context.Background(), Ref{Type: "file", Name: path})) + refs, err := p.List(context.Background()) + require.NoError(t, err) + assert.Empty(t, refs) +} + +func TestFileProvider_ReadsTheFileEachTime(t *testing.T) { + p := NewFileProvider() + path := writeSecretFile(t, "v1") + ref := Ref{Type: "file", Name: path} + + got, err := p.Resolve(context.Background(), ref) + require.NoError(t, err) + assert.Equal(t, "v1", got) + + require.NoError(t, os.WriteFile(path, []byte("v2"), 0o600)) + got, err = p.Resolve(context.Background(), ref) + require.NoError(t, err) + assert.Equal(t, "v2", got, "a rotated secret is picked up on the next resolve") +} + +func TestResolver_ExpandsFileReferences(t *testing.T) { + r := NewResolver() + lf := writeSecretFile(t, "same-value\n") + crlf := writeSecretFile(t, "same-value\r\n") + + a, err := r.ExpandSecretRefs(context.Background(), "${file:"+lf+"}") + require.NoError(t, err) + b, err := r.ExpandSecretRefs(context.Background(), "${file:"+crlf+"}") + require.NoError(t, err) + assert.Equal(t, "same-value", a) + assert.Equal(t, a, b, "an LF-terminated and a CRLF-terminated file resolve to the identical value") + + embedded, err := r.ExpandSecretRefs(context.Background(), "Bearer ${file:"+lf+"}") + require.NoError(t, err) + assert.Equal(t, "Bearer same-value", embedded) +} + +func TestResolver_FileReferenceErrorsNameTheReferenceNotTheValue(t *testing.T) { + r := NewResolver() + const sentinel = "SENTINEL-p3Wv9Qd" + + missing := filepath.Join(t.TempDir(), "missing") + _, err := r.ExpandSecretRefs(context.Background(), "${file:"+missing+"}") + require.Error(t, err) + assert.Contains(t, err.Error(), "${file:"+missing+"}") + + _, err = r.ExpandSecretRefs(context.Background(), "${file:relative/path}") + require.Error(t, err) + assert.Contains(t, err.Error(), "must be absolute") + + tooBig := writeSecretFile(t, sentinel+strings.Repeat("a", MaxSecretFileBytes)) + _, err = r.ExpandSecretRefs(context.Background(), "${file:"+tooBig+"}") + require.Error(t, err) + assert.NotContains(t, err.Error(), sentinel) +} + +func TestParseSecretRef_FilePaths(t *testing.T) { + for _, path := range []string{`/run/secrets/token`, `/run/credentials/mcpproxy.service/key`, `C:\ProgramData\mcpproxy\token`} { + ref, err := ParseSecretRef("${file:" + path + "}") + require.NoError(t, err, path) + assert.Equal(t, "file", ref.Type) + assert.Equal(t, path, ref.Name) + } +} diff --git a/internal/secret/file_read_test.go b/internal/secret/file_read_test.go index 5952f7201..08c1813cf 100644 --- a/internal/secret/file_read_test.go +++ b/internal/secret/file_read_test.go @@ -103,7 +103,12 @@ 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)) + if err := os.Symlink(target, link); err != nil { + if runtime.GOOS == "windows" { + t.Skipf("creating a symlink needs a privilege this Windows account lacks: %v", err) + } + require.NoError(t, err) + } got, err := ReadFile(link) require.NoError(t, err) diff --git a/internal/secret/resolver.go b/internal/secret/resolver.go index ca8160c43..8ed0667a8 100644 --- a/internal/secret/resolver.go +++ b/internal/secret/resolver.go @@ -15,6 +15,7 @@ func NewResolver() *Resolver { // Register default providers r.RegisterProvider("env", NewEnvProvider()) + r.RegisterProvider(SecretTypeFile, NewFileProvider()) r.RegisterProvider("keyring", NewKeyringProvider()) return r diff --git a/specs/111-personal-docker-image/spec.md b/specs/111-personal-docker-image/spec.md index 0bf130158..91b0ff36d 100644 --- a/specs/111-personal-docker-image/spec.md +++ b/specs/111-personal-docker-image/spec.md @@ -280,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. 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. +- **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. A `${file:}` reference is accepted wherever `${env:}` and `${keyring:}` are, which includes the proxy's own `api_key` (`"api_key": "${file:/run/secrets/key}"`): there it is resolved when the config is loaded, as a process-only override, so the effective config runs with the secret while every save, including the one a `PATCH /api/v1/config` triggers, writes the reference back; an `api_key` reference that does not resolve fails the load and never falls back to a generated key; and a key from the environment overrides the file's, in which case the reference is not read. 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**