Add secrets management support to ToolHive MCP server (#2006)

* Add secrets management support to ToolHive MCP server

Implement comprehensive secrets management functionality for the ToolHive MCP server:

- Add list_secrets tool to list available secrets from ToolHive secrets store
- Add set_secret tool to set secrets from file paths (file-based input only)
- Enhance run_server tool with secrets parameter support
- Add SecretMapping struct for structured secret name/target specification
- Include comprehensive test coverage for all new functionality
- Integrate with existing ToolHive secrets providers (encrypted, 1Password, etc.)

The run_server tool now accepts a secrets array parameter allowing users to
pass secrets to MCP servers when running them, matching the CLI --secret flag
functionality but through the MCP interface.

* Add documentation for secrets management design decisions

Clarify the following aspects of the secrets management implementation:
- Description field in SecretInfo is populated by providers that support
  it (e.g., 1Password provides "Vault :: Item :: Field" format) and
  remains empty for providers without description support
- SecretMapping intentionally excludes Description field as it's only
  relevant for listing/discovery, not for runtime secret mapping
- ListSecrets request parameter follows MCP tool handler interface
  requirements but is unused since list_secrets takes no arguments

Addresses review feedback from @yrobla on PR #2006.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>

---------

Co-authored-by: Claude <[email protected]>

Author: JAORMX

pkg/mcp/server/handler.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/stacklok/toolhive/pkg/config"
 	"github.com/stacklok/toolhive/pkg/registry"
 	"github.com/stacklok/toolhive/pkg/workloads"
 )
@@ -14,6 +15,7 @@ type Handler struct {
 	ctx              context.Context
 	workloadManager  workloads.Manager
 	registryProvider registry.Provider
+	configProvider   config.Provider
 }
 
 // NewHandler creates a new ToolHive handler
@@ -30,9 +32,13 @@ func NewHandler(ctx context.Context) (*Handler, error) {
 		return nil, fmt.Errorf("failed to get registry provider: %w", err)
 	}
 
+	// Create config provider
+	configProvider := config.NewDefaultProvider()
+
 	return &Handler{
 		ctx:              ctx,
 		workloadManager:  workloadManager,
 		registryProvider: registryProvider,
+		configProvider:   configProvider,
 	}, nil
 }

pkg/mcp/server/handler_test.go

@@ -31,6 +31,16 @@ func TestParseRunServerArgs(t *testing.T) {
 							"KEY1": "value1",
 							"KEY2": "value2",
 						},
+						"secrets": []interface{}{
+							map[string]interface{}{
+								"name":   "github-token",
+								"target": "GITHUB_TOKEN",
+							},
+							map[string]interface{}{
+								"name":   "api-key",
+								"target": "API_KEY",
+							},
+						},
 					},
 				},
 			},
@@ -42,6 +52,10 @@ func TestParseRunServerArgs(t *testing.T) {
 					"KEY1": "value1",
 					"KEY2": "value2",
 				},
+				Secrets: []SecretMapping{
+					{Name: "github-token", Target: "GITHUB_TOKEN"},
+					{Name: "api-key", Target: "API_KEY"},
+				},
 			},
 			wantErr: false,
 		},
@@ -55,10 +69,11 @@ func TestParseRunServerArgs(t *testing.T) {
 				},
 			},
 			expected: &runServerArgs{
-				Server: "test-server",
-				Name:   "test-server", // Should default to server name
-				Host:   "127.0.0.1",   // Should default to 127.0.0.1
-				Env:    nil,
+				Server:  "test-server",
+				Name:    "test-server", // Should default to server name
+				Host:    "127.0.0.1",   // Should default to 127.0.0.1
+				Env:     nil,
+				Secrets: nil,
 			},
 			wantErr: false,
 		},
@@ -73,10 +88,11 @@ func TestParseRunServerArgs(t *testing.T) {
 				},
 			},
 			expected: &runServerArgs{
-				Server: "my-server",
-				Name:   "my-server",
-				Host:   "127.0.0.1",
-				Env:    nil,
+				Server:  "my-server",
+				Name:    "my-server",
+				Host:    "127.0.0.1",
+				Env:     nil,
+				Secrets: nil,
 			},
 			wantErr: false,
 		},
@@ -91,10 +107,11 @@ func TestParseRunServerArgs(t *testing.T) {
 				},
 			},
 			expected: &runServerArgs{
-				Server: "test-server",
-				Name:   "test-server",
-				Host:   "127.0.0.1",
-				Env:    nil,
+				Server:  "test-server",
+				Name:    "test-server",
+				Host:    "127.0.0.1",
+				Env:     nil,
+				Secrets: nil,
 			},
 			wantErr: false,
 		},
@@ -306,3 +323,51 @@ func TestBuildServerConfig(t *testing.T) {
 		})
 	}
 }
+
+func TestPrepareSecrets(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name     string
+		secrets  []SecretMapping
+		expected []string
+	}{
+		{
+			name:     "nil secrets",
+			secrets:  nil,
+			expected: nil,
+		},
+		{
+			name:     "empty secrets",
+			secrets:  []SecretMapping{},
+			expected: nil,
+		},
+		{
+			name: "single secret",
+			secrets: []SecretMapping{
+				{Name: "github-token", Target: "GITHUB_TOKEN"},
+			},
+			expected: []string{"github-token,target=GITHUB_TOKEN"},
+		},
+		{
+			name: "multiple secrets",
+			secrets: []SecretMapping{
+				{Name: "github-token", Target: "GITHUB_TOKEN"},
+				{Name: "api-key", Target: "API_KEY"},
+				{Name: "db-password", Target: "DATABASE_PASSWORD"},
+			},
+			expected: []string{
+				"github-token,target=GITHUB_TOKEN",
+				"api-key,target=API_KEY",
+				"db-password,target=DATABASE_PASSWORD",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			result := prepareSecrets(tt.secrets)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}

pkg/mcp/server/list_secrets.go

@@ -0,0 +1,73 @@
+package server
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/mark3labs/mcp-go/mcp"
+
+	"github.com/stacklok/toolhive/pkg/secrets"
+)
+
+// SecretInfo represents secret information returned by list
+type SecretInfo struct {
+	Key string `json:"key"`
+	// Description is populated by secrets providers that support it (e.g., 1Password
+	// provides "Vault :: Item :: Field" descriptions). Will be empty for providers
+	// that don't support descriptions (e.g., encrypted provider).
+	Description string `json:"description,omitempty"`
+}
+
+// ListSecretsResponse represents the response from listing secrets
+type ListSecretsResponse struct {
+	Secrets []SecretInfo `json:"secrets"`
+}
+
+// ListSecrets lists all available secrets.
+// The request parameter is required by the MCP tool handler interface but not used
+// by this handler since list_secrets takes no arguments.
+func (h *Handler) ListSecrets(ctx context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) {
+	// Get the configuration to determine the secrets provider
+	cfg := h.configProvider.GetConfig()
+
+	// Check if secrets setup has been completed
+	if !cfg.Secrets.SetupCompleted {
+		return mcp.NewToolResultError(
+			"Secrets provider not configured. Please run 'thv secret setup' to configure a secrets provider first"), nil
+	}
+
+	// Get the provider type
+	providerType, err := cfg.Secrets.GetProviderType()
+	if err != nil {
+		return mcp.NewToolResultError(fmt.Sprintf("Failed to get secrets provider type: %v", err)), nil
+	}
+
+	// Create the secrets provider
+	secretsProvider, err := secrets.CreateSecretProvider(providerType)
+	if err != nil {
+		return mcp.NewToolResultError(fmt.Sprintf("Failed to create secrets provider: %v", err)), nil
+	}
+
+	// List all secrets
+	secretDescriptions, err := secretsProvider.ListSecrets(ctx)
+	if err != nil {
+		return mcp.NewToolResultError(fmt.Sprintf("Failed to list secrets: %v", err)), nil
+	}
+
+	// Format results with structured data
+	var results []SecretInfo
+	for _, desc := range secretDescriptions {
+		info := SecretInfo{
+			Key:         desc.Key,
+			Description: desc.Description,
+		}
+		results = append(results, info)
+	}
+
+	// Create structured response
+	response := ListSecretsResponse{
+		Secrets: results,
+	}
+
+	return mcp.NewToolResultStructuredOnly(response), nil
+}

pkg/mcp/server/list_secrets_test.go

@@ -0,0 +1,88 @@
+package server
+
+import (
+	"context"
+	"testing"
+
+	"github.com/mark3labs/mcp-go/mcp"
+	"github.com/stretchr/testify/assert"
+	"go.uber.org/mock/gomock"
+
+	"github.com/stacklok/toolhive/pkg/config"
+	configmocks "github.com/stacklok/toolhive/pkg/config/mocks"
+	registrymocks "github.com/stacklok/toolhive/pkg/registry/mocks"
+	workloadsmocks "github.com/stacklok/toolhive/pkg/workloads/mocks"
+)
+
+func TestHandler_ListSecrets(t *testing.T) {
+	t.Parallel()
+	ctrl := gomock.NewController(t)
+	t.Cleanup(func() { ctrl.Finish() })
+
+	tests := []struct {
+		name        string
+		setupMocks  func(*configmocks.MockProvider)
+		wantErr     bool
+		checkResult func(*testing.T, *mcp.CallToolResult)
+	}{
+		{
+			name: "secrets not setup",
+			setupMocks: func(configProvider *configmocks.MockProvider) {
+				// Mock config setup - not completed
+				cfg := &config.Config{
+					Secrets: config.Secrets{
+						SetupCompleted: false,
+					},
+				}
+				configProvider.EXPECT().GetConfig().Return(cfg).AnyTimes()
+			},
+			wantErr: false,
+			checkResult: func(t *testing.T, result *mcp.CallToolResult) {
+				t.Helper()
+				assert.NotNil(t, result)
+				assert.True(t, result.IsError)
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			// Create mocks
+			mockRegistry := registrymocks.NewMockProvider(ctrl)
+			mockWorkloadManager := workloadsmocks.NewMockManager(ctrl)
+			mockConfigProvider := configmocks.NewMockProvider(ctrl)
+
+			// Setup mocks
+			if tt.setupMocks != nil {
+				tt.setupMocks(mockConfigProvider)
+			}
+
+			handler := &Handler{
+				ctx:              context.Background(),
+				workloadManager:  mockWorkloadManager,
+				registryProvider: mockRegistry,
+				configProvider:   mockConfigProvider,
+			}
+
+			request := mcp.CallToolRequest{
+				Params: mcp.CallToolParams{
+					Name:      "list_secrets",
+					Arguments: map[string]interface{}{},
+				},
+			}
+
+			result, err := handler.ListSecrets(context.Background(), request)
+
+			if tt.wantErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+				if tt.checkResult != nil {
+					tt.checkResult(t, result)
+				}
+			}
+		})
+	}
+}

pkg/mcp/server/run_server.go

@@ -14,12 +14,22 @@ import (
 	transporttypes "github.com/stacklok/toolhive/pkg/transport/types"
 )
 
+// SecretMapping represents a secret name and its target environment variable.
+// Note: Description is not included because it's only relevant for listing/discovery
+// (see SecretInfo). When mapping secrets to a running server, only the name and target
+// environment variable are needed.
+type SecretMapping struct {
+	Name   string `json:"name"`
+	Target string `json:"target"`
+}
+
 // runServerArgs holds the arguments for running a server
 type runServerArgs struct {
-	Server string            `json:"server"`
-	Name   string            `json:"name,omitempty"`
-	Host   string            `json:"host,omitempty"`
-	Env    map[string]string `json:"env,omitempty"`
+	Server  string            `json:"server"`
+	Name    string            `json:"name,omitempty"`
+	Host    string            `json:"host,omitempty"`
+	Env     map[string]string `json:"env,omitempty"`
+	Secrets []SecretMapping   `json:"secrets,omitempty"`
 }
 
 // RunServer runs an MCP server
@@ -119,6 +129,12 @@ func buildServerConfig(
 	// Prepare environment variables
 	envVars := prepareEnvironmentVariables(imageMetadata, args.Env)
 
+	// Prepare secrets
+	secrets := prepareSecrets(args.Secrets)
+	if len(secrets) > 0 {
+		opts = append(opts, runner.WithSecrets(secrets))
+	}
+
 	// Build the configuration
 	envVarValidator := &runner.DetachedEnvVarValidator{}
 	return runner.NewRunConfigBuilder(ctx, imageMetadata, envVars, envVarValidator, opts...)
@@ -159,6 +175,21 @@ func prepareEnvironmentVariables(imageMetadata *registry.ImageMetadata, userEnv
 	return envVarsMap
 }
 
+// prepareSecrets converts SecretMapping array to the string format expected by the runner
+func prepareSecrets(secretMappings []SecretMapping) []string {
+	if len(secretMappings) == 0 {
+		return nil
+	}
+
+	secrets := make([]string, len(secretMappings))
+	for i, mapping := range secretMappings {
+		// Convert to the format expected by runner: "secret_name,target=ENV_VAR_NAME"
+		secrets[i] = fmt.Sprintf("%s,target=%s", mapping.Name, mapping.Target)
+	}
+
+	return secrets
+}
+
 // saveAndRunServer saves the configuration and runs the server
 func (h *Handler) saveAndRunServer(ctx context.Context, runConfig *runner.RunConfig, name string) error {
 	// Save the run configuration state before starting