diff --git a/cmd/thv/main.go b/cmd/thv/main.go index e60095800d..7e9d37fc77 100644 --- a/cmd/thv/main.go +++ b/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__ namespace + migration.CheckAndPerformSecretScopeMigration() + // Ensure default group exists (creates it for fresh installs, no-op otherwise) migration.EnsureDefaultGroupExists() } diff --git a/pkg/config/config.go b/pkg/config/config.go index 9224b2c7b6..099b9c3dc7 100644 --- a/pkg/config/config.go +++ b/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"` diff --git a/pkg/migration/secret_scope.go b/pkg/migration/secret_scope.go new file mode 100644 index 0000000000..50739ff146 --- /dev/null +++ b/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__ 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) + } + }) +} diff --git a/pkg/secrets/migration.go b/pkg/secrets/migration.go new file mode 100644 index 0000000000..fc8810a411 --- /dev/null +++ b/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 +} diff --git a/pkg/secrets/migration_test.go b/pkg/secrets/migration_test.go new file mode 100644 index 0000000000..635562a276 --- /dev/null +++ b/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) + } + }) + } +}