diff --git a/pkg/gateway/activateprofile.go b/pkg/gateway/activateprofile.go index 17d83055..9c8dbb78 100644 --- a/pkg/gateway/activateprofile.go +++ b/pkg/gateway/activateprofile.go @@ -70,9 +70,10 @@ func (g *Gateway) convertWorkingSetToConfiguration(ctx context.Context, ws worki // Build secrets configs namespace := "" configs = append(configs, ServerSecretConfig{ - Secrets: server.Snapshot.Server.Secrets, - OAuth: server.Snapshot.Server.OAuth, - Namespace: namespace, + Secrets: server.Snapshot.Server.Secrets, + OAuth: server.Snapshot.Server.OAuth, + Namespace: namespace, + RequireVerifiedSecrets: localLongLivedServer(server.Snapshot.Server, g.LongLived), }) // Add tools diff --git a/pkg/gateway/clientpool.go b/pkg/gateway/clientpool.go index 3f04f40f..3370f4bb 100644 --- a/pkg/gateway/clientpool.go +++ b/pkg/gateway/clientpool.go @@ -83,6 +83,21 @@ func (cp *clientPool) longLived(serverConfig *catalog.ServerConfig, config *clie return serverConfig.Spec.LongLived || cp.LongLived } +func (cp *clientPool) missingRequiredSecrets(serverConfig *catalog.ServerConfig, config *clientConfig) []string { + if serverConfig.IsRemote() || !cp.longLived(serverConfig, config) { + return nil + } + + var missing []string + for _, secret := range serverConfig.Spec.Secrets { + if value, ok := serverConfig.Secrets[secret.Name]; !ok || value == "" { + missing = append(missing, secret.Name) + } + } + + return missing +} + func (cp *clientPool) AcquireClient(ctx context.Context, serverConfig *catalog.ServerConfig, config *clientConfig) (mcpclient.Client, error) { var getter *clientGetter c := ctx @@ -101,6 +116,10 @@ func (cp *clientPool) AcquireClient(ctx context.Context, serverConfig *catalog.S // No client found, create a new one if getter == nil { + if missing := cp.missingRequiredSecrets(serverConfig, config); len(missing) > 0 { + return nil, fmt.Errorf("required secret(s) missing for long-lived server %q: %s", serverConfig.Name, strings.Join(missing, ", ")) + } + // If the client is long running, save it for later if cp.longLived(serverConfig, config) { // Double-checked locking: re-check under write lock to avoid duplicate containers diff --git a/pkg/gateway/clientpool_test.go b/pkg/gateway/clientpool_test.go index e9eb5c1e..8de57403 100644 --- a/pkg/gateway/clientpool_test.go +++ b/pkg/gateway/clientpool_test.go @@ -119,6 +119,20 @@ secrets: assert.Equal(t, []string{"DB_PASSWORD=my-actual-db-password", "API_KEY=my-actual-api-key"}, env) } +func TestApplyConfigShortLivedMissingSecretKeepsPlaceholder(t *testing.T) { + catalogYAML := ` +secrets: + - name: short.api_key + env: API_KEY +` + + args, env := argsAndEnv(t, "short", catalogYAML, "", nil) + + assert.Contains(t, args, "-e") + assert.Contains(t, args, "API_KEY") + assert.Equal(t, []string{"API_KEY="}, env) +} + func TestApplyConfigMountAs(t *testing.T) { hostPath := t.TempDir() expectedHostPath, err := cleanDockerHostPath(hostPath) @@ -658,6 +672,104 @@ func TestLongLivedRemoteServer(t *testing.T) { assert.True(t, cp.longLived(remoteServer, cfg), "Remote server must always be long-lived") } +func TestAcquireClientRefusesLongLivedServerWithMissingSecrets(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + + session := &mcp.ServerSession{} + cp := &clientPool{ + keptClients: make(map[clientKey]keptClient), + } + + server := &catalog.ServerConfig{ + Name: "apify-mcp-server", + Spec: catalog.Server{ + Type: "server", + Image: "mcp/apify:latest", + LongLived: true, + Secrets: []catalog.Secret{ + {Name: "apify-mcp-server.apify_token", Env: "APIFY_TOKEN"}, + }, + }, + Config: map[string]any{}, + Secrets: map[string]string{}, + } + cfg := &clientConfig{serverSession: session} + + client, err := cp.AcquireClient(context.Background(), server, cfg) + + require.Error(t, err) + assert.Nil(t, client) + assert.Contains(t, err.Error(), "required secret") + assert.Contains(t, err.Error(), "apify-mcp-server.apify_token") + assert.Empty(t, cp.keptClients) +} + +func TestAcquireClientRefusesLongLivedServerWithEmptySecretValue(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + + session := &mcp.ServerSession{} + cp := &clientPool{ + keptClients: make(map[clientKey]keptClient), + } + + server := &catalog.ServerConfig{ + Name: "apify-mcp-server", + Spec: catalog.Server{ + Type: "server", + Image: "mcp/apify:latest", + LongLived: true, + Secrets: []catalog.Secret{ + {Name: "apify-mcp-server.apify_token", Env: "APIFY_TOKEN"}, + }, + }, + Config: map[string]any{}, + Secrets: map[string]string{ + "apify-mcp-server.apify_token": "", + }, + } + cfg := &clientConfig{serverSession: session} + + client, err := cp.AcquireClient(context.Background(), server, cfg) + + require.Error(t, err) + assert.Nil(t, client) + assert.Contains(t, err.Error(), "required secret") + assert.Contains(t, err.Error(), "apify-mcp-server.apify_token") + assert.Empty(t, cp.keptClients) +} + +func TestAcquireClientRefusesGlobalLongLivedServerWithMissingSecrets(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + + session := &mcp.ServerSession{} + cp := &clientPool{ + Options: Options{LongLived: true}, + keptClients: make(map[clientKey]keptClient), + } + + server := &catalog.ServerConfig{ + Name: "apify-mcp-server", + Spec: catalog.Server{ + Type: "server", + Image: "mcp/apify:latest", + Secrets: []catalog.Secret{ + {Name: "apify-mcp-server.apify_token", Env: "APIFY_TOKEN"}, + }, + }, + Config: map[string]any{}, + Secrets: map[string]string{}, + } + cfg := &clientConfig{serverSession: session} + + client, err := cp.AcquireClient(context.Background(), server, cfg) + + require.Error(t, err) + assert.Nil(t, client) + assert.Contains(t, err.Error(), "required secret") + assert.Contains(t, err.Error(), "apify-mcp-server.apify_token") + assert.Empty(t, cp.keptClients) +} + func TestReleaseClientsForSession(t *testing.T) { sess1 := &mcp.ServerSession{} sess2 := &mcp.ServerSession{} diff --git a/pkg/gateway/configuration.go b/pkg/gateway/configuration.go index 746cc00f..a364300c 100644 --- a/pkg/gateway/configuration.go +++ b/pkg/gateway/configuration.go @@ -310,6 +310,7 @@ type FileBasedConfiguration struct { OciRef []string // OCI references to fetch server definitions from MCPRegistryServers []catalog.Server // Servers fetched from MCP registries Watch bool + LongLived bool McpOAuthDcrEnabled bool docker docker.Client @@ -550,8 +551,9 @@ func (c *FileBasedConfiguration) readOnce(ctx context.Context) (Configuration, e for _, serverName := range serverNames { server := servers[serverName] configs = append(configs, ServerSecretConfig{ - Secrets: server.Secrets, - OAuth: server.OAuth, + Secrets: server.Secrets, + OAuth: server.OAuth, + RequireVerifiedSecrets: localLongLivedServer(server, c.LongLived), }) } return BuildSecretsURIs(ctx, configs) diff --git a/pkg/gateway/configuration_workingset.go b/pkg/gateway/configuration_workingset.go index b940d9ef..d699643c 100644 --- a/pkg/gateway/configuration_workingset.go +++ b/pkg/gateway/configuration_workingset.go @@ -87,9 +87,10 @@ func (c *WorkingSetConfiguration) readOnce(ctx context.Context, dao db.DAO) (Con // namespace = server.Secrets + "_" // } configs = append(configs, ServerSecretConfig{ - Secrets: server.Snapshot.Server.Secrets, - OAuth: server.Snapshot.Server.OAuth, - Namespace: namespace, + Secrets: server.Snapshot.Server.Secrets, + OAuth: server.Snapshot.Server.OAuth, + Namespace: namespace, + RequireVerifiedSecrets: localLongLivedServer(server.Snapshot.Server, c.config.LongLived), }) } secrets := BuildSecretsURIs(ctx, configs) diff --git a/pkg/gateway/mcpadd.go b/pkg/gateway/mcpadd.go index 76c75896..0d274d6f 100644 --- a/pkg/gateway/mcpadd.go +++ b/pkg/gateway/mcpadd.go @@ -89,11 +89,7 @@ func addServerHandler(g *Gateway, clientConfig *clientConfig) mcp.ToolHandler { } } - // Append the new server to the current serverNames if not already present alreadyEnabled := slices.Contains(g.configuration.serverNames, serverName) - if !alreadyEnabled { - g.configuration.serverNames = append(g.configuration.serverNames, serverName) - } // Check if all required secrets are set var missingSecrets []string @@ -101,8 +97,9 @@ func addServerHandler(g *Gateway, clientConfig *clientConfig) mcp.ToolHandler { if serverConfig != nil && len(serverConfig.Spec.Secrets) > 0 { // BuildSecretsURIs only includes secrets that exist in Secrets Engine configs := []ServerSecretConfig{{ - Secrets: serverConfig.Spec.Secrets, - OAuth: serverConfig.Spec.OAuth, + Secrets: serverConfig.Spec.Secrets, + OAuth: serverConfig.Spec.OAuth, + RequireVerifiedSecrets: localLongLivedServer(serverConfig.Spec, g.LongLived), }} availableSecrets = BuildSecretsURIs(ctx, configs) @@ -212,6 +209,12 @@ func addServerHandler(g *Gateway, clientConfig *clientConfig) mcp.ToolHandler { }, nil } + // Append the new server after validating required inputs so failed add + // attempts do not leave it enabled in memory. + if !slices.Contains(g.configuration.serverNames, serverName) { + g.configuration.serverNames = append(g.configuration.serverNames, serverName) + } + // Merge available secrets into configuration so container creation can find them. // This is needed because g.configuration.secrets is built at startup and doesn't // include secrets for dynamically added servers. diff --git a/pkg/gateway/mcpadd_test.go b/pkg/gateway/mcpadd_test.go new file mode 100644 index 00000000..fa53461d --- /dev/null +++ b/pkg/gateway/mcpadd_test.go @@ -0,0 +1,47 @@ +package gateway + +import ( + "context" + "encoding/json" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/mcp-gateway/pkg/catalog" +) + +func TestAddServerHandlerDoesNotAppendServerWhenValidationFails(t *testing.T) { + g := &Gateway{ + configuration: Configuration{ + serverNames: []string{"existing"}, + servers: map[string]catalog.Server{ + "needs-config": { + Image: "mcp/needs-config:latest", + Config: []any{ + map[string]any{"name": "url", "type": "string"}, + }, + }, + }, + config: map[string]map[string]any{}, + }, + } + handler := addServerHandler(g, nil) + args, err := json.Marshal(map[string]any{"name": "needs-config"}) + require.NoError(t, err) + + result, err := handler(context.Background(), &mcp.CallToolRequest{ + Session: &mcp.ServerSession{}, + Params: &mcp.CallToolParamsRaw{ + Arguments: args, + }, + }) + + require.NoError(t, err) + require.Len(t, result.Content, 1) + text, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.Contains(t, text.Text, "The server was not added") + assert.Equal(t, []string{"existing"}, g.configuration.serverNames) +} diff --git a/pkg/gateway/run.go b/pkg/gateway/run.go index aabf601b..7fa6f8f0 100644 --- a/pkg/gateway/run.go +++ b/pkg/gateway/run.go @@ -110,6 +110,7 @@ func NewGateway(config Config, docker docker.Client) *Gateway { OciRef: config.OciRef, MCPRegistryServers: config.MCPRegistryServers, Watch: config.Watch, + LongLived: config.LongLived, McpOAuthDcrEnabled: config.McpOAuthDcrEnabled, docker: docker, } diff --git a/pkg/gateway/secrets_uri.go b/pkg/gateway/secrets_uri.go index 12f76944..76f8dcda 100644 --- a/pkg/gateway/secrets_uri.go +++ b/pkg/gateway/secrets_uri.go @@ -10,9 +10,10 @@ import ( // ServerSecretConfig contains the secret definitions and OAuth config for a server. type ServerSecretConfig struct { - Secrets []catalog.Secret // Secret definitions from server catalog - OAuth *catalog.OAuth // OAuth config (if present, OAuth takes priority over direct secret) - Namespace string // Prefix for map keys (used by WorkingSet for namespacing) + Secrets []catalog.Secret // Secret definitions from server catalog + OAuth *catalog.OAuth // OAuth config (if present, OAuth takes priority over direct secret) + Namespace string // Prefix for map keys (used by WorkingSet for namespacing) + RequireVerifiedSecrets bool // Do not synthesize fallback URIs when existence cannot be checked. } // BuildSecretsURIs generates se:// URIs for secrets with OAuth priority handling. @@ -21,9 +22,10 @@ type ServerSecretConfig struct { // actually exist in the store (OAuth token checked first, then direct secret). // // When the secrets engine is unreachable (e.g. MSIX-sandboxed clients on Windows -// cannot follow AF_UNIX reparse points), URIs are generated for all declared secrets -// since we cannot check existence. Docker Desktop resolves se:// URIs at container -// runtime via named pipes, which are unaffected by MSIX restrictions. +// cannot follow AF_UNIX reparse points), URIs are generated for declared secrets +// unless their server requires verified secrets. Docker Desktop resolves se:// +// URIs at container runtime via named pipes, which are unaffected by MSIX +// restrictions. func BuildSecretsURIs(ctx context.Context, configs []ServerSecretConfig) map[string]string { allSecrets, err := secret.GetSecrets(ctx) if err != nil { @@ -38,13 +40,38 @@ func BuildSecretsURIs(ctx context.Context, configs []ServerSecretConfig) map[str return buildVerifiedURIs(configs, availableSecrets) } +func localLongLivedServer(server catalog.Server, globalLongLived bool) bool { + if server.Remote.URL != "" || server.SSEEndpoint != "" { + return false + } + return server.LongLived || globalLongLived +} + // buildFallbackURIs generates se:// URIs for all declared secrets without checking // existence. Used when the secrets engine is unreachable. OAuth URIs are preferred // when configured (matching the normal priority order). func buildFallbackURIs(configs []ServerSecretConfig) map[string]string { secretNameToURI := make(map[string]string) + requireVerified := make(map[string]struct{}) for _, cfg := range configs { + if !cfg.RequireVerifiedSecrets { + continue + } + for _, s := range cfg.Secrets { + if err := secret.ValidateSecretName(s.Name); err != nil { + log.Logf("Warning: skipping secret with invalid name %q: %v", s.Name, err) + continue + } + requireVerified[cfg.Namespace+s.Name] = struct{}{} + } + } + + for _, cfg := range configs { + if cfg.RequireVerifiedSecrets { + continue + } + secretToOAuthID := oauthMapping(cfg) for _, s := range cfg.Secrets { @@ -53,6 +80,9 @@ func buildFallbackURIs(configs []ServerSecretConfig) map[string]string { continue } secretName := cfg.Namespace + s.Name + if _, ok := requireVerified[secretName]; ok { + continue + } if oauthSecretID, ok := secretToOAuthID[s.Name]; ok { secretNameToURI[secretName] = "se://" + oauthSecretID } else { diff --git a/pkg/gateway/secrets_uri_test.go b/pkg/gateway/secrets_uri_test.go new file mode 100644 index 00000000..9fa6aa18 --- /dev/null +++ b/pkg/gateway/secrets_uri_test.go @@ -0,0 +1,62 @@ +package gateway + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/docker/mcp-gateway/pkg/catalog" +) + +func TestBuildFallbackURIsSkipsLongLivedLocalSecrets(t *testing.T) { + uris := buildFallbackURIs([]ServerSecretConfig{ + { + Secrets: []catalog.Secret{ + {Name: "apify-mcp-server.apify_token", Env: "APIFY_TOKEN"}, + }, + RequireVerifiedSecrets: true, + }, + }) + + assert.Empty(t, uris) +} + +func TestLocalLongLivedServerTreatsGlobalFlagAsLongLived(t *testing.T) { + assert.True(t, localLongLivedServer(catalog.Server{Image: "mcp/test:latest"}, true)) +} + +func TestLocalLongLivedServerExcludesRemoteServers(t *testing.T) { + assert.False(t, localLongLivedServer(catalog.Server{ + Remote: catalog.Remote{URL: "https://mcp.example.com/mcp"}, + }, true)) +} + +func TestBuildFallbackURIsKeepsShortLivedLocalSecrets(t *testing.T) { + uris := buildFallbackURIs([]ServerSecretConfig{ + { + Secrets: []catalog.Secret{ + {Name: "short.api_key", Env: "API_KEY"}, + }, + }, + }) + + assert.Equal(t, "se://docker/mcp/short.api_key", uris["short.api_key"]) +} + +func TestBuildFallbackURIsDoesNotBackfillSharedLongLivedSecrets(t *testing.T) { + uris := buildFallbackURIs([]ServerSecretConfig{ + { + Secrets: []catalog.Secret{ + {Name: "shared.api_key", Env: "API_KEY"}, + }, + RequireVerifiedSecrets: true, + }, + { + Secrets: []catalog.Secret{ + {Name: "shared.api_key", Env: "API_KEY"}, + }, + }, + }) + + assert.NotContains(t, uris, "shared.api_key") +}