Merge commit from fork

* Do not persist secrets in the runconfig

Previously we expanded secrets into environment variables before writing
the run config into the state store. This led to the unfortunate
situation where the run config contained both references to secrets as
well as the raw secrets themselves. Since the state store is not
encrypted, it would be possible for someone with access to the user's
home folder to read the secrets without needing to know the decryption
password.

This code moves the point at which the secret expansion happens to after
we persist the run config, and right before we start the MCP server.

* Fix issue with detached processes

* Configure transport after writing config

* linting

* use fmt.Println instead of empty log line

Author: dmjb

cmd/thv/app/common.go

@@ -36,20 +36,6 @@ func IsOIDCEnabled(cmd *cobra.Command) bool {
 	return jwksURL != "" || issuer != ""
 }
 
-// GetSecretsProviderType returns the secrets provider type from the command flags
-func GetSecretsProviderType(_ *cobra.Command) (secrets.ProviderType, error) {
-	provider := config.GetConfig().Secrets.ProviderType
-	switch provider {
-	case string(secrets.EncryptedType):
-		return secrets.EncryptedType, nil
-	case string(secrets.OnePasswordType):
-		return secrets.OnePasswordType, nil
-	default:
-		// TODO: auto-generate the set of valid values.
-		return "", fmt.Errorf("invalid secrets provider type: %s (valid types: encrypted, 1password)", provider)
-	}
-}
-
 // SetSecretsProvider sets the secrets provider type in the configuration.
 // It validates the input and updates the configuration.
 // Choices are `encrypted` and `1password`.
@@ -83,13 +69,13 @@ func SetSecretsProvider(provider secrets.ProviderType) error {
 }
 
 // NeedSecretsPassword returns true if the secrets provider requires a password.
-func NeedSecretsPassword(cmd *cobra.Command, secretOptions []string) bool {
+func NeedSecretsPassword(secretOptions []string) bool {
 	// If the user did not ask for any secrets, then don't attempt to instantiate
 	// the secrets manager.
 	if len(secretOptions) == 0 {
 		return false
 	}
 	// Ignore err - if the flag is not set, it's not needed.
-	providerType, _ := GetSecretsProviderType(cmd)
+	providerType, _ := config.GetConfig().Secrets.GetProviderType()
 	return providerType == secrets.EncryptedType
 }

cmd/thv/app/restart.go

@@ -93,7 +93,7 @@ func restartCmdFunc(cmd *cobra.Command, args []string) error {
 
 	// Run the tooling server
 	logger.Infof("Starting tooling server %s...", containerName)
-	return RunMCPServer(ctx, cmd, mcpRunner.Config, false)
+	return RunMCPServer(ctx, mcpRunner.Config, false)
 }
 
 // isProxyRunning checks if the proxy process is running

cmd/thv/app/run.go

@@ -218,12 +218,12 @@ func runCmdFunc(cmd *cobra.Command, args []string) error {
 	}
 
 	// Configure the RunConfig with transport, ports, permissions, etc.
-	if err := configureRunConfig(cmd, config, runTransport, runPort, runTargetPort, runTargetHost, runEnv); err != nil {
+	if err := configureRunConfig(config, runTransport, runPort, runTargetPort, runTargetHost, runEnv); err != nil {
 		return err
 	}
 
 	// Run the MCP server
-	return RunMCPServer(ctx, cmd, config, runForeground)
+	return RunMCPServer(ctx, config, runForeground)
 }
 
 // pullImage pulls an image from a remote registry if it has the "latest" tag

cmd/thv/app/run_common.go

@@ -9,7 +9,6 @@ import (
 	"syscall"
 
 	"github.com/adrg/xdg"
-	"github.com/spf13/cobra"
 
 	"github.com/stacklok/toolhive/pkg/authz"
 	"github.com/stacklok/toolhive/pkg/logger"
@@ -23,10 +22,10 @@ import (
 // RunMCPServer runs an MCP server with the specified config
 //
 //nolint:gocyclo // This function is complex but manageable
-func RunMCPServer(ctx context.Context, cmd *cobra.Command, config *runner.RunConfig, foreground bool) error {
+func RunMCPServer(ctx context.Context, config *runner.RunConfig, foreground bool) error {
 	// If not running in foreground mode, start a new detached process and exit
 	if !foreground {
-		return detachProcess(cmd, config)
+		return detachProcess(config)
 	}
 
 	// Create a Runner with the RunConfig
@@ -39,7 +38,7 @@ func RunMCPServer(ctx context.Context, cmd *cobra.Command, config *runner.RunCon
 // detachProcess starts a new detached process with the same command but with the --foreground flag
 //
 //nolint:gocyclo // This function is complex but manageable
-func detachProcess(cmd *cobra.Command, options *runner.RunConfig) error {
+func detachProcess(options *runner.RunConfig) error {
 	// Get the current executable path
 	execPath, err := os.Executable()
 	if err != nil {
@@ -146,8 +145,8 @@ func detachProcess(cmd *cobra.Command, options *runner.RunConfig) error {
 	// Set environment variables for the detached process
 	detachedCmd.Env = append(os.Environ(), fmt.Sprintf("%s=%s", process.ToolHiveDetachedEnv, process.ToolHiveDetachedValue))
 
-	// If the process needs the decrypt password, pass it as an environment variable.
-	if NeedSecretsPassword(cmd, options.Secrets) {
+	// If we need the decrypt password, set it as an environment variable in the detached process.
+	if NeedSecretsPassword(options.Secrets) {
 		password, err := secrets.GetSecretsPassword()
 		if err != nil {
 			return fmt.Errorf("failed to get secrets password: %v", err)
@@ -200,7 +199,6 @@ func findEnvironmentVariableFromSecrets(secs []string, envVarName string) bool {
 
 // configureRunConfig configures a RunConfig with transport, ports, permissions, etc.
 func configureRunConfig(
-	cmd *cobra.Command,
 	config *runner.RunConfig,
 	transport string,
 	port int,
@@ -215,24 +213,6 @@ func configureRunConfig(
 		return fmt.Errorf("permission profile is required")
 	}
 
-	// Process secrets if provided
-	if len(config.Secrets) > 0 {
-		providerType, err := GetSecretsProviderType(cmd)
-		if err != nil {
-			return fmt.Errorf("error determining secrets provider type: %w", err)
-		}
-
-		secretManager, err := secrets.CreateSecretManager(providerType)
-		if err != nil {
-			return fmt.Errorf("error instantiating secret manager %v", err)
-		}
-
-		// Process secrets
-		if _, err = config.WithSecrets(secretManager); err != nil {
-			return err
-		}
-	}
-
 	// Set transport
 	if _, err = config.WithTransport(transport); err != nil {
 		return err

cmd/thv/app/secret.go

@@ -10,6 +10,7 @@ import (
 	"github.com/spf13/cobra"
 	"golang.org/x/term"
 
+	"github.com/stacklok/toolhive/pkg/config"
 	"github.com/stacklok/toolhive/pkg/secrets"
 )
 
@@ -68,7 +69,7 @@ Input Methods:
 
 The secret will be stored securely using the configured secrets provider.`,
 		Args: cobra.ExactArgs(1),
-		Run: func(cmd *cobra.Command, args []string) {
+		Run: func(_ *cobra.Command, args []string) {
 			name := args[0]
 
 			// Validate input
@@ -114,13 +115,7 @@ The secret will be stored securely using the configured secrets provider.`,
 				return
 			}
 
-			providerType, err := GetSecretsProviderType(cmd)
-			if err != nil {
-				fmt.Fprintf(os.Stderr, "Error: %v\n", err)
-				os.Exit(1)
-			}
-
-			manager, err := secrets.CreateSecretManager(providerType)
+			manager, err := getSecretsManager()
 			if err != nil {
 				fmt.Fprintf(os.Stderr, "Failed to create secrets manager: %v\n", err)
 				return
@@ -141,7 +136,7 @@ func newSecretGetCommand() *cobra.Command {
 		Use:   "get <name>",
 		Short: "Get a secret",
 		Args:  cobra.ExactArgs(1),
-		Run: func(cmd *cobra.Command, args []string) {
+		Run: func(_ *cobra.Command, args []string) {
 			name := args[0]
 
 			// Validate input
@@ -150,13 +145,7 @@ func newSecretGetCommand() *cobra.Command {
 				return
 			}
 
-			providerType, err := GetSecretsProviderType(cmd)
-			if err != nil {
-				fmt.Fprintf(os.Stderr, "Error: %v\n", err)
-				os.Exit(1)
-			}
-
-			manager, err := secrets.CreateSecretManager(providerType)
+			manager, err := getSecretsManager()
 			if err != nil {
 				fmt.Fprintf(os.Stderr, "Failed to create secrets manager: %v\n", err)
 				return
@@ -177,7 +166,7 @@ func newSecretDeleteCommand() *cobra.Command {
 		Use:   "delete <name>",
 		Short: "Delete a secret",
 		Args:  cobra.ExactArgs(1),
-		Run: func(cmd *cobra.Command, args []string) {
+		Run: func(_ *cobra.Command, args []string) {
 			name := args[0]
 
 			// Validate input
@@ -186,13 +175,7 @@ func newSecretDeleteCommand() *cobra.Command {
 				return
 			}
 
-			providerType, err := GetSecretsProviderType(cmd)
-			if err != nil {
-				fmt.Fprintf(os.Stderr, "Error: %v\n", err)
-				os.Exit(1)
-			}
-
-			manager, err := secrets.CreateSecretManager(providerType)
+			manager, err := getSecretsManager()
 			if err != nil {
 				fmt.Fprintf(os.Stderr, "Failed to create secrets manager: %v\n", err)
 				return
@@ -213,14 +196,8 @@ func newSecretListCommand() *cobra.Command {
 		Use:   "list",
 		Short: "List all available secrets",
 		Args:  cobra.NoArgs,
-		Run: func(cmd *cobra.Command, _ []string) {
-			providerType, err := GetSecretsProviderType(cmd)
-			if err != nil {
-				fmt.Fprintf(os.Stderr, "Error: %v\n", err)
-				os.Exit(1)
-			}
-
-			manager, err := secrets.CreateSecretManager(providerType)
+		Run: func(_ *cobra.Command, _ []string) {
+			manager, err := getSecretsManager()
 			if err != nil {
 				fmt.Fprintf(os.Stderr, "Failed to create secrets manager: %v\n", err)
 				return
@@ -259,3 +236,17 @@ func newSecretResetKeyringCommand() *cobra.Command {
 		},
 	}
 }
+
+func getSecretsManager() (secrets.Provider, error) {
+	providerType, err := config.GetConfig().Secrets.GetProviderType()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get secrets provider type: %w", err)
+	}
+
+	manager, err := secrets.CreateSecretProvider(providerType)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create secrets manager: %w", err)
+	}
+
+	return manager, nil
+}

pkg/config/config.go

@@ -33,6 +33,20 @@ type Secrets struct {
 	ProviderType string `yaml:"provider_type"`
 }
 
+// GetProviderType returns the secrets provider type from the application config.
+func (s *Secrets) GetProviderType() (secrets.ProviderType, error) {
+	provider := s.ProviderType
+	switch provider {
+	case string(secrets.EncryptedType):
+		return secrets.EncryptedType, nil
+	case string(secrets.OnePasswordType):
+		return secrets.OnePasswordType, nil
+	default:
+		// TODO: auto-generate the set of valid values.
+		return "", fmt.Errorf("invalid secrets provider type: %s (valid types: encrypted, 1password)", provider)
+	}
+}
+
 // Clients contains settings for client configuration.
 type Clients struct {
 	AutoDiscovery     bool     `yaml:"auto_discovery"`

pkg/runner/runner.go

@@ -11,8 +11,10 @@ import (
 
 	"github.com/stacklok/toolhive/pkg/auth"
 	"github.com/stacklok/toolhive/pkg/client"
+	"github.com/stacklok/toolhive/pkg/config"
 	"github.com/stacklok/toolhive/pkg/logger"
 	"github.com/stacklok/toolhive/pkg/process"
+	"github.com/stacklok/toolhive/pkg/secrets"
 	"github.com/stacklok/toolhive/pkg/transport"
 	"github.com/stacklok/toolhive/pkg/transport/types"
 )
@@ -24,9 +26,9 @@ type Runner struct {
 }
 
 // NewRunner creates a new Runner with the provided configuration
-func NewRunner(config *RunConfig) *Runner {
+func NewRunner(runConfig *RunConfig) *Runner {
 	return &Runner{
-		Config: config,
+		Config: runConfig,
 	}
 }
 
@@ -83,6 +85,31 @@ func (r *Runner) Run(ctx context.Context) error {
 		return fmt.Errorf("failed to create transport: %v", err)
 	}
 
+	// Save the configuration to the state store
+	if err := r.SaveState(ctx); err != nil {
+		logger.Warnf("Warning: Failed to save run configuration: %v", err)
+	}
+
+	// Process secrets if provided
+	// NOTE: This MUST happen after we save the run config to avoid storing
+	// the secrets in the state store.
+	if len(r.Config.Secrets) > 0 {
+		providerType, err := config.GetConfig().Secrets.GetProviderType()
+		if err != nil {
+			return fmt.Errorf("error determining secrets provider type: %w", err)
+		}
+
+		secretManager, err := secrets.CreateSecretProvider(providerType)
+		if err != nil {
+			return fmt.Errorf("error instantiating secret manager %v", err)
+		}
+
+		// Process secrets
+		if _, err = r.Config.WithSecrets(secretManager); err != nil {
+			return err
+		}
+	}
+
 	// Set up the transport
 	logger.Infof("Setting up %s transport...", r.Config.Transport)
 	if err := transportHandler.Setup(
@@ -100,11 +127,6 @@ func (r *Runner) Run(ctx context.Context) error {
 
 	logger.Infof("MCP server %s started successfully", r.Config.ContainerName)
 
-	// Save the configuration to the state store
-	if err := r.SaveState(ctx); err != nil {
-		logger.Warnf("Warning: Failed to save run configuration: %v", err)
-	}
-
 	// Update client configurations with the MCP server URL.
 	// Note that this function checks the configuration to determine which
 	// clients should be updated, if any.
@@ -199,12 +221,12 @@ func (r *Runner) Run(ctx context.Context) error {
 // updateClientConfigurations updates client configuration files with the MCP server URL
 func updateClientConfigurations(containerName, host string, port int) error {
 	// Find client configuration files
-	configs, err := client.FindClientConfigs()
+	clientConfigs, err := client.FindClientConfigs()
 	if err != nil {
 		return fmt.Errorf("failed to find client configurations: %w", err)
 	}
 
-	if len(configs) == 0 {
+	if len(clientConfigs) == 0 {
 		logger.Infof("No client configuration files found")
 		return nil
 	}
@@ -213,15 +235,15 @@ func updateClientConfigurations(containerName, host string, port int) error {
 	url := client.GenerateMCPServerURL(host, port, containerName)
 
 	// Update each configuration file
-	for _, config := range configs {
-		logger.Infof("Updating client configuration: %s", config.Path)
+	for _, clientConfig := range clientConfigs {
+		logger.Infof("Updating client configuration: %s", clientConfig.Path)
 
-		if err := client.Upsert(config, containerName, url); err != nil {
-			fmt.Printf("Warning: Failed to update MCP server configuration in %s: %v\n", config.Path, err)
+		if err := client.Upsert(clientConfig, containerName, url); err != nil {
+			fmt.Printf("Warning: Failed to update MCP server configuration in %s: %v\n", clientConfig.Path, err)
 			continue
 		}
 
-		logger.Infof("Successfully updated client configuration: %s", config.Path)
+		logger.Infof("Successfully updated client configuration: %s", clientConfig.Path)
 	}
 
 	return nil