Add crash-safe migration for legacy system secret keys

cmd/thv/main.go

@@ -1,4 +1,4 @@
-// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package main is the entry point for the ToolHive CLI.
@@ -62,6 +62,10 @@ func main() {
 		// Ensures middleware-based telemetry configs are properly migrated
 		migration.CheckAndPerformMiddlewareTelemetryMigration()
 
+		// Check and perform secret scope migration if needed
+		// Renames bare system keys (BEARER_TOKEN_, REGISTRY_OAUTH_, etc.) to __thv_<scope>_ namespace
+		migration.CheckAndPerformSecretScopeMigration()
+
 		// Ensure default group exists (creates it for fresh installs, no-op otherwise)
 		migration.EnsureDefaultGroupExists()
 	}

pkg/config/config.go

@@ -1,4 +1,4 @@
-// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package config contains the definition of the application config structure
@@ -39,6 +39,7 @@ type Config struct {
 	DefaultGroupMigration        bool                                `yaml:"default_group_migration,omitempty"`
 	TelemetryConfigMigration     bool                                `yaml:"telemetry_config_migration,omitempty"`
 	MiddlewareTelemetryMigration bool                                `yaml:"middleware_telemetry_migration,omitempty"`
+	SecretScopeMigration         bool                                `yaml:"secret_scope_migration,omitempty"`
 	DisableUsageMetrics          bool                                `yaml:"disable_usage_metrics,omitempty"`
 	BuildEnv                     map[string]string                   `yaml:"build_env,omitempty"`
 	BuildEnvFromSecrets          map[string]string                   `yaml:"build_env_from_secrets,omitempty"`

pkg/migration/secret_scope.go

@@ -0,0 +1,67 @@
+// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package migration
+
+import (
+	"context"
+	"log/slog"
+	"sync"
+
+	"github.com/stacklok/toolhive/pkg/config"
+	"github.com/stacklok/toolhive/pkg/secrets"
+)
+
+var secretScopeMigrationOnce sync.Once
+
+// CheckAndPerformSecretScopeMigration checks if secret scope migration is needed and performs it.
+// It discovers bare system keys and renames them into their __thv_<scope>_ namespace.
+// This ensures system-owned secrets are hidden from user-facing secret commands.
+func CheckAndPerformSecretScopeMigration() {
+	secretScopeMigrationOnce.Do(func() {
+		appConfig := config.NewDefaultProvider().GetConfig()
+		if appConfig.SecretScopeMigration {
+			slog.Debug("secret scope migration already completed, skipping")
+			return
+		}
+
+		if !appConfig.Secrets.SetupCompleted {
+			slog.Debug("secrets not set up, skipping secret scope migration")
+			return
+		}
+
+		providerType, err := appConfig.Secrets.GetProviderType()
+		if err != nil {
+			slog.Error("failed to get secrets provider type for migration", "error", err)
+			return
+		}
+
+		provider, err := secrets.CreateSecretProvider(providerType)
+		if err != nil {
+			slog.Error("failed to create secrets provider for migration", "error", err)
+			return
+		}
+
+		migrations, err := secrets.DiscoverMigrations(context.Background(), provider)
+		if err != nil {
+			slog.Error("failed to discover secret migrations", "error", err)
+			return
+		}
+
+		if len(migrations) == 0 {
+			slog.Debug("no secret scope migrations needed")
+		} else {
+			slog.Debug("migrating system secrets to scoped namespace", "count", len(migrations))
+			if err := secrets.MigrateSystemKeys(context.Background(), provider, migrations); err != nil {
+				slog.Error("failed to migrate system secrets", "error", err)
+				return
+			}
+		}
+
+		if err := config.UpdateConfig(func(c *config.Config) {
+			c.SecretScopeMigration = true
+		}); err != nil {
+			slog.Error("failed to update config after secret scope migration", "error", err)
+		}
+	})
+}

pkg/secrets/migration.go

@@ -0,0 +1,85 @@
+// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package secrets
+
+import (
+	"context"
+	"fmt"
+	"strings"
+)
+
+// KeyMigration describes a single key rename operation from OldKey to NewKey.
+type KeyMigration struct {
+	OldKey string
+	NewKey string
+}
+
+// SystemKeyPrefixMappings maps known bare key prefixes to their target scope.
+// Used by DiscoverMigrations to find keys that need migrating.
+var SystemKeyPrefixMappings = []struct {
+	Prefix string
+	Scope  string
+}{
+	{"BEARER_TOKEN_", ScopeWorkloads},
+	{"OAUTH_CLIENT_SECRET_", ScopeWorkloads},
+	{"OAUTH_REFRESH_TOKEN_", ScopeWorkloads},
+	{"registry-user-", ScopeWorkloads},
+	{"registry-default-", ScopeWorkloads},
+	{"BUILD_AUTH_FILE_", ScopeWorkloads},
+	{"REGISTRY_OAUTH_", ScopeRegistry},
+}
+
+// MigrateSystemKeys renames keys from OldKey to NewKey in provider.
+// Write-before-delete ordering ensures that a crash between the two operations
+// leaves the secret reachable under the new key. Keys that do not exist in
+// provider are silently skipped, making the function safe to retry.
+func MigrateSystemKeys(ctx context.Context, provider Provider, migrations []KeyMigration) error {
+	for _, m := range migrations {
+		val, err := provider.GetSecret(ctx, m.OldKey)
+		if IsNotFoundError(err) {
+			continue
+		}
+		if err != nil {
+			return fmt.Errorf("migration: reading %q: %w", m.OldKey, err)
+		}
+
+		if err := provider.SetSecret(ctx, m.NewKey, val); err != nil {
+			return fmt.Errorf("migration: writing %q: %w", m.NewKey, err)
+		}
+
+		if err := provider.DeleteSecret(ctx, m.OldKey); err != nil {
+			return fmt.Errorf("migration: deleting %q: %w", m.OldKey, err)
+		}
+	}
+	return nil
+}
+
+// DiscoverMigrations lists all secrets in provider and returns the set of
+// migrations needed to move system-owned keys into their scoped namespaces.
+// Keys that already start with SystemKeyPrefix are skipped (already migrated).
+func DiscoverMigrations(ctx context.Context, provider Provider) ([]KeyMigration, error) {
+	all, err := provider.ListSecrets(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("migration discovery: listing secrets: %w", err)
+	}
+
+	var migrations []KeyMigration
+	for _, desc := range all {
+		key := desc.Key
+		// Skip already-migrated keys.
+		if strings.HasPrefix(key, SystemKeyPrefix) {
+			continue
+		}
+		for _, mapping := range SystemKeyPrefixMappings {
+			if strings.HasPrefix(key, mapping.Prefix) {
+				migrations = append(migrations, KeyMigration{
+					OldKey: key,
+					NewKey: scopedKey(mapping.Scope, key),
+				})
+				break
+			}
+		}
+	}
+	return migrations, nil
+}

pkg/secrets/migration_test.go

@@ -0,0 +1,213 @@
+// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package secrets_test
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
+
+	"github.com/stacklok/toolhive/pkg/secrets"
+	secretsmocks "github.com/stacklok/toolhive/pkg/secrets/mocks"
+)
+
+func TestMigrateSystemKeys(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name        string
+		migrations  []secrets.KeyMigration
+		setup       func(m *secretsmocks.MockProvider)
+		wantErr     bool
+		errContains string
+	}{
+		{
+			name: "migrates all keys successfully",
+			migrations: []secrets.KeyMigration{
+				{OldKey: "BEARER_TOKEN_foo", NewKey: "__thv_workloads_BEARER_TOKEN_foo"},
+				{OldKey: "REGISTRY_OAUTH_bar", NewKey: "__thv_registry_REGISTRY_OAUTH_bar"},
+				{OldKey: "BUILD_AUTH_FILE_docker", NewKey: "__thv_workloads_BUILD_AUTH_FILE_docker"},
+			},
+			setup: func(m *secretsmocks.MockProvider) {
+				m.EXPECT().GetSecret(gomock.Any(), "BEARER_TOKEN_foo").Return("token-val", nil)
+				m.EXPECT().SetSecret(gomock.Any(), "__thv_workloads_BEARER_TOKEN_foo", "token-val").Return(nil)
+				m.EXPECT().DeleteSecret(gomock.Any(), "BEARER_TOKEN_foo").Return(nil)
+
+				m.EXPECT().GetSecret(gomock.Any(), "REGISTRY_OAUTH_bar").Return("oauth-val", nil)
+				m.EXPECT().SetSecret(gomock.Any(), "__thv_registry_REGISTRY_OAUTH_bar", "oauth-val").Return(nil)
+				m.EXPECT().DeleteSecret(gomock.Any(), "REGISTRY_OAUTH_bar").Return(nil)
+
+				m.EXPECT().GetSecret(gomock.Any(), "BUILD_AUTH_FILE_docker").Return("auth-val", nil)
+				m.EXPECT().SetSecret(gomock.Any(), "__thv_workloads_BUILD_AUTH_FILE_docker", "auth-val").Return(nil)
+				m.EXPECT().DeleteSecret(gomock.Any(), "BUILD_AUTH_FILE_docker").Return(nil)
+			},
+		},
+		{
+			name: "skips key that does not exist",
+			migrations: []secrets.KeyMigration{
+				{OldKey: "BEARER_TOKEN_missing", NewKey: "__thv_workloads_BEARER_TOKEN_missing"},
+			},
+			setup: func(m *secretsmocks.MockProvider) {
+				m.EXPECT().GetSecret(gomock.Any(), "BEARER_TOKEN_missing").Return("", errors.New("secret not found: BEARER_TOKEN_missing"))
+				// SetSecret and DeleteSecret must NOT be called.
+			},
+		},
+		{
+			name:       "empty migration list is a no-op",
+			migrations: []secrets.KeyMigration{},
+			setup:      func(_ *secretsmocks.MockProvider) {},
+		},
+		{
+			name: "returns error when GetSecret fails with unexpected error",
+			migrations: []secrets.KeyMigration{
+				{OldKey: "BEARER_TOKEN_err", NewKey: "__thv_workloads_BEARER_TOKEN_err"},
+			},
+			setup: func(m *secretsmocks.MockProvider) {
+				m.EXPECT().GetSecret(gomock.Any(), "BEARER_TOKEN_err").Return("", errors.New("backend unavailable"))
+			},
+			wantErr:     true,
+			errContains: "migration: reading",
+		},
+		{
+			name: "returns error when SetSecret fails",
+			migrations: []secrets.KeyMigration{
+				{OldKey: "BEARER_TOKEN_setfail", NewKey: "__thv_workloads_BEARER_TOKEN_setfail"},
+			},
+			setup: func(m *secretsmocks.MockProvider) {
+				m.EXPECT().GetSecret(gomock.Any(), "BEARER_TOKEN_setfail").Return("val", nil)
+				m.EXPECT().SetSecret(gomock.Any(), "__thv_workloads_BEARER_TOKEN_setfail", "val").Return(errors.New("write error"))
+			},
+			wantErr:     true,
+			errContains: "migration: writing",
+		},
+		{
+			name: "returns error when DeleteSecret fails",
+			migrations: []secrets.KeyMigration{
+				{OldKey: "BEARER_TOKEN_delfail", NewKey: "__thv_workloads_BEARER_TOKEN_delfail"},
+			},
+			setup: func(m *secretsmocks.MockProvider) {
+				m.EXPECT().GetSecret(gomock.Any(), "BEARER_TOKEN_delfail").Return("val", nil)
+				m.EXPECT().SetSecret(gomock.Any(), "__thv_workloads_BEARER_TOKEN_delfail", "val").Return(nil)
+				m.EXPECT().DeleteSecret(gomock.Any(), "BEARER_TOKEN_delfail").Return(errors.New("delete error"))
+			},
+			wantErr:     true,
+			errContains: "migration: deleting",
+		},
+		{
+			name: "idempotent when old key is already gone",
+			migrations: []secrets.KeyMigration{
+				{OldKey: "BEARER_TOKEN_gone", NewKey: "__thv_workloads_BEARER_TOKEN_gone"},
+			},
+			setup: func(m *secretsmocks.MockProvider) {
+				// Old key already deleted in a previous migration run.
+				m.EXPECT().GetSecret(gomock.Any(), "BEARER_TOKEN_gone").Return("", errors.New("secret not found: BEARER_TOKEN_gone"))
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			ctrl := gomock.NewController(t)
+
+			mock := secretsmocks.NewMockProvider(ctrl)
+			tt.setup(mock)
+
+			err := secrets.MigrateSystemKeys(context.Background(), mock, tt.migrations)
+			if tt.wantErr {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tt.errContains)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestDiscoverMigrations(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name        string
+		setup       func(m *secretsmocks.MockProvider)
+		wantCount   int
+		wantErr     bool
+		errContains string
+	}{
+		{
+			name: "discovers all system key prefixes",
+			setup: func(m *secretsmocks.MockProvider) {
+				m.EXPECT().ListSecrets(gomock.Any()).Return([]secrets.SecretDescription{
+					{Key: "BEARER_TOKEN_foo"},
+					{Key: "OAUTH_CLIENT_SECRET_bar"},
+					{Key: "OAUTH_REFRESH_TOKEN_baz"},
+					{Key: "registry-user-myrepo"},
+					{Key: "registry-default-prod"},
+					{Key: "BUILD_AUTH_FILE_docker"},
+					{Key: "REGISTRY_OAUTH_ghcr"},
+				}, nil)
+			},
+			wantCount: 7,
+		},
+		{
+			name: "skips already-migrated keys",
+			setup: func(m *secretsmocks.MockProvider) {
+				m.EXPECT().ListSecrets(gomock.Any()).Return([]secrets.SecretDescription{
+					{Key: "__thv_workloads_BEARER_TOKEN_foo"},
+					{Key: "__thv_registry_REGISTRY_OAUTH_bar"},
+					{Key: "user-secret"},
+				}, nil)
+			},
+			wantCount: 0,
+		},
+		{
+			name: "empty store returns no migrations",
+			setup: func(m *secretsmocks.MockProvider) {
+				m.EXPECT().ListSecrets(gomock.Any()).Return([]secrets.SecretDescription{}, nil)
+			},
+			wantCount: 0,
+		},
+		{
+			name: "user keys are not included in migrations",
+			setup: func(m *secretsmocks.MockProvider) {
+				m.EXPECT().ListSecrets(gomock.Any()).Return([]secrets.SecretDescription{
+					{Key: "my-api-key"},
+					{Key: "github-token"},
+					{Key: "BEARER_TOKEN_workload1"},
+				}, nil)
+			},
+			wantCount: 1, // only the system key
+		},
+		{
+			name: "returns error when ListSecrets fails",
+			setup: func(m *secretsmocks.MockProvider) {
+				m.EXPECT().ListSecrets(gomock.Any()).Return(nil, errors.New("backend unavailable"))
+			},
+			wantErr:     true,
+			errContains: "migration discovery: listing secrets",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			ctrl := gomock.NewController(t)
+
+			mock := secretsmocks.NewMockProvider(ctrl)
+			tt.setup(mock)
+
+			migs, err := secrets.DiscoverMigrations(context.Background(), mock)
+			if tt.wantErr {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tt.errContains)
+			} else {
+				require.NoError(t, err)
+				require.Len(t, migs, tt.wantCount)
+			}
+		})
+	}
+}