From 277ea836b69959ec8bd67779e5065bb1d655d68f Mon Sep 17 00:00:00 2001 From: Michael Mandrus <41969079+mmandrus@users.noreply.github.com> Date: Wed, 24 Aug 2022 16:24:50 -0400 Subject: [PATCH] Secrets: Implement migration of secrets from plugin back to unified secrets (#53561) * initial cut at migration from plugin * create new migration from plugin * only migrate to or from, not both * remove cfg check from plugin migration itself * update comments, clean up secret after migration * add better error handling * hook up REST API with migrations * Minor fixes * fix wire injection issue * modify migrator to access plugin calls directly. create unit tests * change pre-migration checks in admin api * stop plugin after migrating from it * fix compile issues after merge * add comment about migration * fix linting issue * bleh, fix unit test * fix another unit test * update plugin error fatal flag after a migration from the plugin * add extra logging to migration * make linter happy Co-authored-by: Leandro Deveikis --- pkg/api/admin_encryption.go | 31 +++++ pkg/api/api.go | 2 + pkg/api/http_server.go | 6 +- pkg/cmd/grafana-cli/runner/wire.go | 2 +- pkg/infra/serverlock/serverlock.go | 8 +- pkg/infra/serverlock/serverlock_test.go | 2 +- pkg/server/wire.go | 3 +- .../secrets/kvstore/migrate_from_plugin.go | 116 ++++++++++++++++++ .../kvstore/migrate_from_plugin_test.go | 97 +++++++++++++++ .../{plugin_mig.go => migrate_to_plugin.go} | 13 +- ..._mig_test.go => migrate_to_plugin_test.go} | 27 ++-- .../secrets/kvstore/migrations/migrator.go | 55 +++++++-- .../kvstore/plugin_fatal_crash_test.go | 37 ++++-- pkg/services/secrets/kvstore/remote_plugin.go | 1 + pkg/services/secrets/kvstore/test_helpers.go | 64 ++++++++-- 15 files changed, 405 insertions(+), 59 deletions(-) create mode 100644 pkg/services/secrets/kvstore/migrate_from_plugin.go create mode 100644 pkg/services/secrets/kvstore/migrate_from_plugin_test.go rename pkg/services/secrets/kvstore/{plugin_mig.go => migrate_to_plugin.go} (89%) rename pkg/services/secrets/kvstore/{plugin_mig_test.go => migrate_to_plugin_test.go} (74%) diff --git a/pkg/api/admin_encryption.go b/pkg/api/admin_encryption.go index 1e71b67aeae..f5e5d80d8f5 100644 --- a/pkg/api/admin_encryption.go +++ b/pkg/api/admin_encryption.go @@ -5,6 +5,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/models" + skv "github.com/grafana/grafana/pkg/services/secrets/kvstore" ) func (hs *HTTPServer) AdminRotateDataEncryptionKeys(c *models.ReqContext) response.Response { @@ -48,3 +49,33 @@ func (hs *HTTPServer) AdminRollbackSecrets(c *models.ReqContext) response.Respon return response.Respond(http.StatusOK, "Secrets rolled back successfully") } + +// To migrate to the plugin, it must be installed and configured +// so as not to lose access to migrated secrets +func (hs *HTTPServer) MigrateSecretsToPlugin(c *models.ReqContext) response.Response { + if skv.EvaluateRemoteSecretsPlugin(hs.secretsPluginManager, hs.Cfg) != nil { + hs.log.Warn("Received secrets plugin migration request while plugin is not available") + return response.Respond(http.StatusBadRequest, "Secrets plugin is not available") + } + err := hs.secretsPluginMigrator.TriggerPluginMigration(c.Req.Context(), true) + if err != nil { + hs.log.Error("Failed to trigger secret migration to plugin", "error", err.Error()) + return response.Respond(http.StatusInternalServerError, "Secret migration to plugin failed") + } + return response.Respond(http.StatusOK, "Secret migration to plugin triggered successfully") +} + +// To migrate from the plugin, it must be installed only +// as it is possible the user disabled it and then wants to migrate +func (hs *HTTPServer) MigrateSecretsFromPlugin(c *models.ReqContext) response.Response { + if hs.secretsPluginManager.SecretsManager() == nil { + hs.log.Warn("Received secrets plugin migration request while plugin is not installed") + return response.Respond(http.StatusBadRequest, "Secrets plugin is not installed") + } + err := hs.secretsPluginMigrator.TriggerPluginMigration(c.Req.Context(), false) + if err != nil { + hs.log.Error("Failed to trigger secret migration from plugin", "error", err.Error()) + return response.Respond(http.StatusInternalServerError, "Secret migration from plugin failed") + } + return response.Respond(http.StatusOK, "Secret migration from plugin triggered successfully") +} diff --git a/pkg/api/api.go b/pkg/api/api.go index f9ca984503b..64e125a0625 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -599,6 +599,8 @@ func (hs *HTTPServer) registerRoutes() { adminRoute.Post("/encryption/reencrypt-data-keys", reqGrafanaAdmin, routing.Wrap(hs.AdminReEncryptEncryptionKeys)) adminRoute.Post("/encryption/reencrypt-secrets", reqGrafanaAdmin, routing.Wrap(hs.AdminReEncryptSecrets)) adminRoute.Post("/encryption/rollback-secrets", reqGrafanaAdmin, routing.Wrap(hs.AdminRollbackSecrets)) + adminRoute.Post("/encryption/migrate-secrets/to-plugin", reqGrafanaAdmin, routing.Wrap(hs.MigrateSecretsToPlugin)) + adminRoute.Post("/encryption/migrate-secrets/from-plugin", reqGrafanaAdmin, routing.Wrap(hs.MigrateSecretsFromPlugin)) adminRoute.Post("/provisioning/dashboards/reload", authorize(reqGrafanaAdmin, ac.EvalPermission(ActionProvisioningReload, ScopeProvisionersDashboards)), routing.Wrap(hs.AdminProvisioningReloadDashboards)) adminRoute.Post("/provisioning/plugins/reload", authorize(reqGrafanaAdmin, ac.EvalPermission(ActionProvisioningReload, ScopeProvisionersPlugins)), routing.Wrap(hs.AdminProvisioningReloadPlugins)) diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index f28afbb2131..7e18417e491 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -76,6 +76,7 @@ import ( "github.com/grafana/grafana/pkg/services/search" "github.com/grafana/grafana/pkg/services/searchusers" "github.com/grafana/grafana/pkg/services/secrets" + spm "github.com/grafana/grafana/pkg/services/secrets/kvstore/migrations" "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/shorturls" "github.com/grafana/grafana/pkg/services/sqlstore" @@ -178,6 +179,7 @@ type HTTPServer struct { apiKeyService apikey.Service kvStore kvstore.KVStore secretsMigrator secrets.Migrator + secretsPluginMigrator *spm.SecretMigrationServiceImpl userService user.Service tempUserService tempUser.Service loginAttemptService loginAttempt.Service @@ -217,7 +219,8 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi teamsPermissionsService accesscontrol.TeamPermissionsService, folderPermissionsService accesscontrol.FolderPermissionsService, dashboardPermissionsService accesscontrol.DashboardPermissionsService, dashboardVersionService dashver.Service, starService star.Service, csrfService csrf.Service, coremodels *registry.Base, - playlistService playlist.Service, apiKeyService apikey.Service, kvStore kvstore.KVStore, secretsMigrator secrets.Migrator, secretsPluginManager plugins.SecretsPluginManager, + playlistService playlist.Service, apiKeyService apikey.Service, kvStore kvstore.KVStore, + secretsMigrator secrets.Migrator, secretsPluginManager plugins.SecretsPluginManager, secretsPluginMigrator *spm.SecretMigrationServiceImpl, publicDashboardsApi *publicdashboardsApi.Api, userService user.Service, tempUserService tempUser.Service, loginAttemptService loginAttempt.Service, orgService org.Service, accesscontrolService accesscontrol.Service, ) (*HTTPServer, error) { @@ -307,6 +310,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi kvStore: kvStore, PublicDashboardsApi: publicDashboardsApi, secretsMigrator: secretsMigrator, + secretsPluginMigrator: secretsPluginMigrator, userService: userService, tempUserService: tempUserService, loginAttemptService: loginAttemptService, diff --git a/pkg/cmd/grafana-cli/runner/wire.go b/pkg/cmd/grafana-cli/runner/wire.go index dca2f0272d9..f8da47e9c8f 100644 --- a/pkg/cmd/grafana-cli/runner/wire.go +++ b/pkg/cmd/grafana-cli/runner/wire.go @@ -307,7 +307,7 @@ var wireSet = wire.NewSet( userimpl.ProvideService, orgimpl.ProvideService, datasourceservice.ProvideDataSourceMigrationService, - secretsStore.ProvidePluginSecretMigrationService, + secretsStore.ProvideMigrateToPluginService, secretsMigrations.ProvideSecretMigrationService, wire.Bind(new(secretsMigrations.SecretMigrationService), new(*secretsMigrations.SecretMigrationServiceImpl)), userauthimpl.ProvideService, diff --git a/pkg/infra/serverlock/serverlock.go b/pkg/infra/serverlock/serverlock.go index 837360e7f26..f02b462bfa3 100644 --- a/pkg/infra/serverlock/serverlock.go +++ b/pkg/infra/serverlock/serverlock.go @@ -147,7 +147,7 @@ func (sl *ServerLockService) acquireForRelease(ctx context.Context, actionName s if len(lockRows) > 0 { result := lockRows[0] if sl.isLockWithinInterval(result, maxInterval) { - return errors.New("there is already a lock for this operation") + return errors.New("there is already a lock for this actionName: " + actionName) } else { // lock has timeouted, so we update the timestamp result.LastExecution = time.Now().Unix() @@ -157,7 +157,7 @@ func (sl *ServerLockService) acquireForRelease(ctx context.Context, actionName s return err } if affected != 1 { - sl.log.Error("Expected rows affected to be 1 if there was no error.", "rowAffected", affected) + sl.log.Error("Expected rows affected to be 1 if there was no error.", "actionName", actionName, "rowAffected", affected) } return nil } @@ -175,7 +175,7 @@ func (sl *ServerLockService) acquireForRelease(ctx context.Context, actionName s if affected != 1 { // this means that there was no error but there is something not working correctly - sl.log.Error("Expected rows affected to be 1 if there was no error.", "rowAffected", affected) + sl.log.Error("Expected rows affected to be 1 if there was no error.", "actionName", actionName, "rowAffected", affected) } } return nil @@ -195,7 +195,7 @@ func (sl *ServerLockService) releaseLock(ctx context.Context, actionName string) } affected, err := res.RowsAffected() if affected != 1 { - sl.log.Debug("Error releasing lock ", "affected", affected) + sl.log.Debug("Error releasing lock ", "actionName", actionName, "affected", affected) } return err }) diff --git a/pkg/infra/serverlock/serverlock_test.go b/pkg/infra/serverlock/serverlock_test.go index e27c898c854..d042b59e4c6 100644 --- a/pkg/infra/serverlock/serverlock_test.go +++ b/pkg/infra/serverlock/serverlock_test.go @@ -88,7 +88,7 @@ func TestLockAndRelease(t *testing.T) { err2 := sl.acquireForRelease(context.Background(), operationUID, duration) require.Error(t, err2, "We should expect an error when trying to get the second lock") - require.Equal(t, "there is already a lock for this operation", err2.Error()) + require.Equal(t, "there is already a lock for this actionName: "+operationUID, err2.Error()) err3 := sl.releaseLock(context.Background(), operationUID) require.NoError(t, err3) diff --git a/pkg/server/wire.go b/pkg/server/wire.go index 1e311697122..ca05dff73f7 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -319,7 +319,8 @@ var wireBasicSet = wire.NewSet( tempuserimpl.ProvideService, loginattemptimpl.ProvideService, datasourceservice.ProvideDataSourceMigrationService, - secretsStore.ProvidePluginSecretMigrationService, + secretsStore.ProvideMigrateToPluginService, + secretsStore.ProvideMigrateFromPluginService, secretsMigrations.ProvideSecretMigrationService, wire.Bind(new(secretsMigrations.SecretMigrationService), new(*secretsMigrations.SecretMigrationServiceImpl)), userauthimpl.ProvideService, diff --git a/pkg/services/secrets/kvstore/migrate_from_plugin.go b/pkg/services/secrets/kvstore/migrate_from_plugin.go new file mode 100644 index 00000000000..c20db3b3a0b --- /dev/null +++ b/pkg/services/secrets/kvstore/migrate_from_plugin.go @@ -0,0 +1,116 @@ +package kvstore + +import ( + "context" + "fmt" + "sync" + + "github.com/grafana/grafana/pkg/infra/kvstore" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin" + "github.com/grafana/grafana/pkg/services/secrets" + "github.com/grafana/grafana/pkg/services/sqlstore" + "github.com/grafana/grafana/pkg/setting" +) + +// MigrateFromPluginService This migrator will handle migration of the configured plugin secrets back to Grafana unified secrets +type MigrateFromPluginService struct { + cfg *setting.Cfg + logger log.Logger + sqlStore sqlstore.Store + secretsService secrets.Service + manager plugins.SecretsPluginManager + kvstore kvstore.KVStore +} + +func ProvideMigrateFromPluginService( + cfg *setting.Cfg, + sqlStore sqlstore.Store, + secretsService secrets.Service, + manager plugins.SecretsPluginManager, + kvstore kvstore.KVStore, + +) *MigrateFromPluginService { + return &MigrateFromPluginService{ + cfg: cfg, + logger: log.New("sec-plugin-mig"), + sqlStore: sqlStore, + secretsService: secretsService, + manager: manager, + kvstore: kvstore, + } +} + +func (s *MigrateFromPluginService) Migrate(ctx context.Context) error { + s.logger.Debug("starting migration of plugin secrets to unified secrets") + // access the plugin directly + plugin, err := startAndReturnPlugin(s.manager, context.Background()) + if err != nil { + s.logger.Error("Error retrieiving plugin", "error", err.Error()) + return err + } + // Get full list of secrets from the plugin + res, err := plugin.GetAllSecrets(ctx, &secretsmanagerplugin.GetAllSecretsRequest{}) + if err != nil { + s.logger.Error("Failed to retrieve all secrets from plugin") + return err + } + totalSecrets := len(res.Items) + s.logger.Debug("retrieved all secrets from plugin", "num secrets", totalSecrets) + // create a secret sql store manually + secretsSql := &secretsKVStoreSQL{ + sqlStore: s.sqlStore, + secretsService: s.secretsService, + log: s.logger, + decryptionCache: decryptionCache{ + cache: make(map[int64]cachedDecrypted), + }, + } + for i, item := range res.Items { + s.logger.Debug(fmt.Sprintf("Migrating secret %d of %d", i+1, totalSecrets), "current", i+1, "secretCount", totalSecrets) + // Add to sql store + err = secretsSql.Set(ctx, item.Key.OrgId, item.Key.Namespace, item.Key.Type, item.Value) + if err != nil { + s.logger.Error("Error adding secret to unified secrets", "orgId", item.Key.OrgId, + "namespace", item.Key.Namespace, "type", item.Key.Type) + return err + } + } + + for i, item := range res.Items { + s.logger.Debug(fmt.Sprintf("Cleaning secret %d of %d", i+1, totalSecrets), "current", i+1, "secretCount", totalSecrets) + // Delete from the plugin + _, err := plugin.DeleteSecret(ctx, &secretsmanagerplugin.DeleteSecretRequest{ + KeyDescriptor: &secretsmanagerplugin.Key{ + OrgId: item.Key.OrgId, + Namespace: item.Key.Namespace, + Type: item.Key.Type, + }}) + if err != nil { + s.logger.Error("Error deleting secret from plugin after migration", "orgId", item.Key.OrgId, + "namespace", item.Key.Namespace, "type", item.Key.Type) + continue + } + } + s.logger.Debug("Completed migration of secrets from plugin") + + // The plugin is no longer needed at the moment + err = setPluginStartupErrorFatal(ctx, GetNamespacedKVStore(s.kvstore), false) + if err != nil { + s.logger.Error("Failed to remove plugin error fatal flag", "error", err.Error()) + } + // Reset the fatal flag setter in case another secret is created on the plugin + fatalFlagOnce = sync.Once{} + + s.logger.Debug("Shutting down secrets plugin now that migration is complete") + // if `use_plugin` wasn't set, stop the plugin after migration + if !s.cfg.SectionWithEnvOverrides("secrets").Key("use_plugin").MustBool(false) { + err := s.manager.SecretsManager().Stop(ctx) + if err != nil { + // Log a warning but don't throw an error + s.logger.Error("Error stopping secrets plugin after migration", "error", err.Error()) + } + } + return nil +} diff --git a/pkg/services/secrets/kvstore/migrate_from_plugin_test.go b/pkg/services/secrets/kvstore/migrate_from_plugin_test.go new file mode 100644 index 00000000000..6add17254f5 --- /dev/null +++ b/pkg/services/secrets/kvstore/migrate_from_plugin_test.go @@ -0,0 +1,97 @@ +package kvstore + +import ( + "context" + "testing" + + "github.com/grafana/grafana/pkg/infra/kvstore" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin" + "github.com/grafana/grafana/pkg/services/secrets/fakes" + secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager" + "github.com/grafana/grafana/pkg/services/sqlstore" + "github.com/grafana/grafana/pkg/setting" + "github.com/stretchr/testify/require" +) + +// This tests will create a mock sql database and an inmemory +// implementation of the secret manager to simulate the plugin. +func TestPluginSecretMigrationService_MigrateFromPlugin(t *testing.T) { + ctx := context.Background() + + t.Run("migrate secrets from secrets plugin to Grafana", func(t *testing.T) { + // --- SETUP + migratorService, plugin, sqlStore := setupTestMigrateFromPluginService(t) + + addSecretToPluginStore(t, plugin, ctx, 1, "secret-1", "bogus", "value-1") + addSecretToPluginStore(t, plugin, ctx, 1, "secret-2", "bogus", "value-2") + + // --- EXECUTION + err := migratorService.Migrate(ctx) + require.NoError(t, err) + + // --- VALIDATIONS + validatePluginSecretsWereDeleted(t, plugin, ctx) + + validateSecretWasStoredInSql(t, sqlStore, ctx, 1, "secret-1", "bogus", "value-1") + validateSecretWasStoredInSql(t, sqlStore, ctx, 1, "secret-2", "bogus", "value-2") + }) +} + +// Set up services used in migration +func setupTestMigrateFromPluginService(t *testing.T) (*MigrateFromPluginService, secretsmanagerplugin.SecretsManagerPlugin, *secretsKVStoreSQL) { + t.Helper() + + // this is to init the sql secret store inside the migration + sqlStore := sqlstore.InitTestDB(t) + secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore()) + manager := NewFakeSecretsPluginManager(t, false) + migratorService := ProvideMigrateFromPluginService( + setting.NewCfg(), + sqlStore, + secretsService, + manager, + kvstore.ProvideService(sqlStore), + ) + + secretsSql := &secretsKVStoreSQL{ + sqlStore: sqlStore, + secretsService: secretsService, + log: log.New("test.logger"), + decryptionCache: decryptionCache{ + cache: make(map[int64]cachedDecrypted), + }, + } + + return migratorService, manager.SecretsManager().SecretsManager, secretsSql +} + +func addSecretToPluginStore(t *testing.T, plugin secretsmanagerplugin.SecretsManagerPlugin, ctx context.Context, orgId int64, namespace string, typ string, value string) { + t.Helper() + _, err := plugin.SetSecret(ctx, &secretsmanagerplugin.SetSecretRequest{ + KeyDescriptor: &secretsmanagerplugin.Key{ + OrgId: orgId, + Namespace: namespace, + Type: typ, + }, + Value: value, + }) + require.NoError(t, err) +} + +// validates that secrets on the plugin were deleted +func validatePluginSecretsWereDeleted(t *testing.T, plugin secretsmanagerplugin.SecretsManagerPlugin, ctx context.Context) { + t.Helper() + res, err := plugin.GetAllSecrets(ctx, &secretsmanagerplugin.GetAllSecretsRequest{}) + require.NoError(t, err) + require.Equal(t, 0, len(res.Items)) +} + +// validates that secrets are in sql +func validateSecretWasStoredInSql(t *testing.T, sqlStore *secretsKVStoreSQL, ctx context.Context, orgId int64, namespace string, typ string, expectedValue string) { + t.Helper() + res, exists, err := sqlStore.Get(ctx, orgId, namespace, typ) + require.NoError(t, err) + require.True(t, exists) + require.Equal(t, expectedValue, res) +} diff --git a/pkg/services/secrets/kvstore/plugin_mig.go b/pkg/services/secrets/kvstore/migrate_to_plugin.go similarity index 89% rename from pkg/services/secrets/kvstore/plugin_mig.go rename to pkg/services/secrets/kvstore/migrate_to_plugin.go index acb6f4101aa..2c3df279d2c 100644 --- a/pkg/services/secrets/kvstore/plugin_mig.go +++ b/pkg/services/secrets/kvstore/migrate_to_plugin.go @@ -13,9 +13,9 @@ import ( "github.com/grafana/grafana/pkg/setting" ) -// PluginSecretMigrationService This migrator will handle migration of datasource secrets (aka Unified secrets) +// MigrateToPluginService This migrator will handle migration of datasource secrets (aka Unified secrets) // into the plugin secrets configured -type PluginSecretMigrationService struct { +type MigrateToPluginService struct { secretsStore SecretsKVStore cfg *setting.Cfg logger log.Logger @@ -25,15 +25,15 @@ type PluginSecretMigrationService struct { manager plugins.SecretsPluginManager } -func ProvidePluginSecretMigrationService( +func ProvideMigrateToPluginService( secretsStore SecretsKVStore, cfg *setting.Cfg, sqlStore sqlstore.Store, secretsService secrets.Service, kvstore kvstore.KVStore, manager plugins.SecretsPluginManager, -) *PluginSecretMigrationService { - return &PluginSecretMigrationService{ +) *MigrateToPluginService { + return &MigrateToPluginService{ secretsStore: secretsStore, cfg: cfg, logger: log.New("secret.migration.plugin"), @@ -44,8 +44,7 @@ func ProvidePluginSecretMigrationService( } } -func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error { - // Check if we should migrate to plugin - default false +func (s *MigrateToPluginService) Migrate(ctx context.Context) error { if err := EvaluateRemoteSecretsPlugin(s.manager, s.cfg); err == nil { s.logger.Debug("starting migration of unified secrets to the plugin") // we need to get the fallback store since in this scenario the secrets store would be the plugin. diff --git a/pkg/services/secrets/kvstore/plugin_mig_test.go b/pkg/services/secrets/kvstore/migrate_to_plugin_test.go similarity index 74% rename from pkg/services/secrets/kvstore/plugin_mig_test.go rename to pkg/services/secrets/kvstore/migrate_to_plugin_test.go index 279f5f9c618..ecf4071b2ed 100644 --- a/pkg/services/secrets/kvstore/plugin_mig_test.go +++ b/pkg/services/secrets/kvstore/migrate_to_plugin_test.go @@ -9,7 +9,6 @@ import ( "github.com/grafana/grafana/pkg/services/secrets/fakes" secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager" "github.com/grafana/grafana/pkg/services/sqlstore" - "github.com/grafana/grafana/pkg/setting" "github.com/stretchr/testify/require" "gopkg.in/ini.v1" @@ -17,12 +16,12 @@ import ( // This tests will create a mock sql database and an inmemory // implementation of the secret manager to simulate the plugin. -func TestPluginSecretMigrationService_Migrate(t *testing.T) { +func TestPluginSecretMigrationService_MigrateToPlugin(t *testing.T) { ctx := context.Background() t.Run("migration run ok - 2 secrets migrated", func(t *testing.T) { // --- SETUP - migratorService, secretsStore, sqlSecretStore := setupTestMigratorService(t) + migratorService, secretsStore, sqlSecretStore := setupTestMigrateToPluginService(t) var orgId int64 = 1 namespace1, namespace2 := "namespace-test", "namespace-test2" typ := "type-test" @@ -36,41 +35,43 @@ func TestPluginSecretMigrationService_Migrate(t *testing.T) { require.NoError(t, err) // --- VALIDATIONS - validateSecretWasDeleted(t, sqlSecretStore, ctx, orgId, namespace1, typ) - validateSecretWasDeleted(t, sqlSecretStore, ctx, orgId, namespace2, typ) + validateSqlSecretWasDeleted(t, sqlSecretStore, ctx, orgId, namespace1, typ) + validateSqlSecretWasDeleted(t, sqlSecretStore, ctx, orgId, namespace2, typ) - validateSecretWasStoreInPlugin(t, secretsStore, ctx, orgId, namespace1, typ) - validateSecretWasStoreInPlugin(t, secretsStore, ctx, orgId, namespace1, typ) + validateSecretWasStoredInPlugin(t, secretsStore, ctx, orgId, namespace1, typ) + validateSecretWasStoredInPlugin(t, secretsStore, ctx, orgId, namespace1, typ) }) } func addSecretToSqlStore(t *testing.T, sqlSecretStore *secretsKVStoreSQL, ctx context.Context, orgId int64, namespace1 string, typ string, value string) { + t.Helper() err := sqlSecretStore.Set(ctx, orgId, namespace1, typ, value) require.NoError(t, err) } // validates that secrets on the sql store were deleted. -func validateSecretWasDeleted(t *testing.T, sqlSecretStore *secretsKVStoreSQL, ctx context.Context, orgId int64, namespace1 string, typ string) { +func validateSqlSecretWasDeleted(t *testing.T, sqlSecretStore *secretsKVStoreSQL, ctx context.Context, orgId int64, namespace1 string, typ string) { + t.Helper() res, err := sqlSecretStore.Keys(ctx, orgId, namespace1, typ) require.NoError(t, err) require.Equal(t, 0, len(res)) } // validates that secrets should be on the plugin -func validateSecretWasStoreInPlugin(t *testing.T, secretsStore SecretsKVStore, ctx context.Context, orgId int64, namespace1 string, typ string) { +func validateSecretWasStoredInPlugin(t *testing.T, secretsStore SecretsKVStore, ctx context.Context, orgId int64, namespace1 string, typ string) { + t.Helper() resPlugin, err := secretsStore.Keys(ctx, orgId, namespace1, typ) require.NoError(t, err) require.Equal(t, 1, len(resPlugin)) } -// -func setupTestMigratorService(t *testing.T) (*PluginSecretMigrationService, SecretsKVStore, *secretsKVStoreSQL) { +// Set up services used in migration +func setupTestMigrateToPluginService(t *testing.T) (*MigrateToPluginService, SecretsKVStore, *secretsKVStoreSQL) { t.Helper() rawCfg := ` [secrets] use_plugin = true - migrate_to_plugin = true ` raw, err := ini.Load([]byte(rawCfg)) require.NoError(t, err) @@ -82,7 +83,7 @@ func setupTestMigratorService(t *testing.T) (*PluginSecretMigrationService, Secr sqlStore := sqlstore.InitTestDB(t) secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore()) manager := NewFakeSecretsPluginManager(t, false) - migratorService := ProvidePluginSecretMigrationService( + migratorService := ProvideMigrateToPluginService( secretsStoreForPlugin, cfg, sqlStore, diff --git a/pkg/services/secrets/kvstore/migrations/migrator.go b/pkg/services/secrets/kvstore/migrations/migrator.go index 08dc9d20ada..19934a2b449 100644 --- a/pkg/services/secrets/kvstore/migrations/migrator.go +++ b/pkg/services/secrets/kvstore/migrations/migrator.go @@ -9,41 +9,58 @@ import ( "github.com/grafana/grafana/pkg/infra/serverlock" datasources "github.com/grafana/grafana/pkg/services/datasources/service" "github.com/grafana/grafana/pkg/services/secrets/kvstore" + "github.com/grafana/grafana/pkg/setting" ) var logger = log.New("secret.migration") +const actionName = "secret migration task " + // SecretMigrationService is used to migrate legacy secrets to new unified secrets. type SecretMigrationService interface { Migrate(ctx context.Context) error } type SecretMigrationServiceImpl struct { - Services []SecretMigrationService - ServerLockService *serverlock.ServerLockService + services []SecretMigrationService + ServerLockService *serverlock.ServerLockService + migrateToPluginService *kvstore.MigrateToPluginService + migrateFromPluginService *kvstore.MigrateFromPluginService } func ProvideSecretMigrationService( + cfg *setting.Cfg, serverLockService *serverlock.ServerLockService, dataSourceSecretMigrationService *datasources.DataSourceSecretMigrationService, - pluginSecretMigrationService *kvstore.PluginSecretMigrationService, + migrateToPluginService *kvstore.MigrateToPluginService, + migrateFromPluginService *kvstore.MigrateFromPluginService, ) *SecretMigrationServiceImpl { services := make([]SecretMigrationService, 0) services = append(services, dataSourceSecretMigrationService) - // pluginMigrationService should always be the last one - services = append(services, pluginSecretMigrationService) + // Plugin migration should always be last; should either migrate to or from, not both + // This is because the migrateTo checks for use_plugin = true, in which case we should always + // migrate by default to ensure users don't lose access to secrets. If migration has + // already occurred, the migrateTo function will be called but it won't do anything + if cfg.SectionWithEnvOverrides("secrets").Key("migrate_from_plugin").MustBool(false) { + services = append(services, migrateFromPluginService) + } else { + services = append(services, migrateToPluginService) + } return &SecretMigrationServiceImpl{ - ServerLockService: serverLockService, - Services: services, + ServerLockService: serverLockService, + services: services, + migrateToPluginService: migrateToPluginService, + migrateFromPluginService: migrateFromPluginService, } } // Migrate Run migration services. This will block until all services have exited. +// This should only be called once at startup func (s *SecretMigrationServiceImpl) Migrate(ctx context.Context) error { // Start migration services. - return s.ServerLockService.LockAndExecute(ctx, "migrate secrets to unified secrets", time.Minute*10, func(context.Context) { - for _, service := range s.Services { + return s.ServerLockService.LockExecuteAndRelease(ctx, actionName, time.Minute*10, func(context.Context) { + for _, service := range s.services { serviceName := reflect.TypeOf(service).String() logger.Debug("Starting secret migration service", "service", serviceName) err := service.Migrate(ctx) @@ -54,3 +71,23 @@ func (s *SecretMigrationServiceImpl) Migrate(ctx context.Context) error { } }) } + +// TriggerPluginMigration Kick off a migration to or from the plugin. This will block until all services have exited. +func (s *SecretMigrationServiceImpl) TriggerPluginMigration(ctx context.Context, toPlugin bool) error { + // Don't migrate if there is already one happening + return s.ServerLockService.LockExecuteAndRelease(ctx, actionName, time.Minute*10, func(context.Context) { + var err error + if toPlugin { + err = s.migrateToPluginService.Migrate(ctx) + } else { + err = s.migrateFromPluginService.Migrate(ctx) + } + if err != nil { + direction := "from_plugin" + if toPlugin { + direction = "to_plugin" + } + logger.Error("Failed to migrate plugin secrets", "direction", direction, "error", err.Error()) + } + }) +} diff --git a/pkg/services/secrets/kvstore/plugin_fatal_crash_test.go b/pkg/services/secrets/kvstore/plugin_fatal_crash_test.go index f59a7371fba..64465634b68 100644 --- a/pkg/services/secrets/kvstore/plugin_fatal_crash_test.go +++ b/pkg/services/secrets/kvstore/plugin_fatal_crash_test.go @@ -3,10 +3,13 @@ package kvstore import ( "context" "errors" + "fmt" "sync" "testing" "github.com/grafana/grafana/pkg/infra/kvstore" + "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin" "github.com/grafana/grafana/pkg/services/secrets/fakes" secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager" "github.com/grafana/grafana/pkg/services/sqlstore" @@ -19,7 +22,8 @@ import ( // Set fatal flag to true, then simulate a plugin start failure // Should result in an error from the secret store provider func TestFatalPluginErr_PluginFailsToStartWithFatalFlagSet(t *testing.T) { - svc, _, _, err := setupFatalCrashTest(t, true, true, false) + svc, mgr, _, _, err := setupFatalCrashTest(t, true, true, false) + _ = fmt.Sprint(mgr) // this is here to satisfy the linter require.Error(t, err) require.Nil(t, svc) } @@ -27,7 +31,8 @@ func TestFatalPluginErr_PluginFailsToStartWithFatalFlagSet(t *testing.T) { // Set fatal flag to false, then simulate a plugin start failure // Should result in the secret store provider returning the sql impl func TestFatalPluginErr_PluginFailsToStartWithFatalFlagNotSet(t *testing.T) { - svc, _, _, err := setupFatalCrashTest(t, true, false, false) + svc, mgr, _, _, err := setupFatalCrashTest(t, true, false, false) + _ = fmt.Sprint(mgr) // this is here to satisfy the linter require.NoError(t, err) require.IsType(t, &CachedKVStore{}, svc) cachedKv, _ := svc.(*CachedKVStore) @@ -37,7 +42,7 @@ func TestFatalPluginErr_PluginFailsToStartWithFatalFlagNotSet(t *testing.T) { // With fatal flag not set, store a secret in the plugin while backwards compatibility is disabled // Should result in the fatal flag going from unset -> set to true func TestFatalPluginErr_FatalFlagGetsSetWithBackwardsCompatDisabled(t *testing.T) { - svc, kvstore, _, err := setupFatalCrashTest(t, false, false, true) + svc, _, kvstore, _, err := setupFatalCrashTest(t, false, false, true) require.NoError(t, err) require.NotNil(t, svc) err = svc.Set(context.Background(), 0, "datasource", "postgres", "my secret") @@ -50,10 +55,21 @@ func TestFatalPluginErr_FatalFlagGetsSetWithBackwardsCompatDisabled(t *testing.T // With fatal flag set, retrieve a secret from the plugin while backwards compatibility is enabled // Should result in the fatal flag going from set to true -> unset func TestFatalPluginErr_FatalFlagGetsUnSetWithBackwardsCompatEnabled(t *testing.T) { - svc, kvstore, _, err := setupFatalCrashTest(t, false, true, false) + svc, mgr, kvstore, _, err := setupFatalCrashTest(t, false, true, false) require.NoError(t, err) require.NotNil(t, svc) - val, exists, err := svc.Get(context.Background(), 0, "datasource", "postgres") + // setup - store secret and manually bypassing the remote plugin impl + _, err = mgr.SecretsManager().SecretsManager.SetSecret(context.Background(), &secretsmanagerplugin.SetSecretRequest{ + KeyDescriptor: &secretsmanagerplugin.Key{ + OrgId: 0, + Namespace: "postgres", + Type: "datasource", + }, + Value: "bogus", + }) + require.NoError(t, err) + // retrieve the secret and check values + val, exists, err := svc.Get(context.Background(), 0, "postgres", "datasource") require.NoError(t, err) require.NotNil(t, val) require.True(t, exists) @@ -65,7 +81,7 @@ func TestFatalPluginErr_FatalFlagGetsUnSetWithBackwardsCompatEnabled(t *testing. // With fatal flag unset, do a migration with backwards compatibility disabled. When unified secrets are deleted, return an error on the first deletion // Should result in the fatal flag remaining unset func TestFatalPluginErr_MigrationTestWithErrorDeletingUnifiedSecrets(t *testing.T) { - svc, kvstore, _, err := setupFatalCrashTest(t, false, false, true) + svc, _, kvstore, _, err := setupFatalCrashTest(t, false, false, true) require.NoError(t, err) migration := setupTestMigratorServiceWithDeletionError(t, svc, &mockstore.SQLStoreMock{ @@ -83,7 +99,7 @@ func setupFatalCrashTest( shouldFailOnStart bool, isPluginErrorFatal bool, isBackwardsCompatDisabled bool, -) (SecretsKVStore, kvstore.KVStore, *sqlstore.SQLStore, error) { +) (SecretsKVStore, plugins.SecretsPluginManager, kvstore.KVStore, *sqlstore.SQLStore, error) { t.Helper() fatalFlagOnce = sync.Once{} startupOnce = sync.Once{} @@ -100,7 +116,7 @@ func setupFatalCrashTest( t.Cleanup(func() { fatalFlagOnce = sync.Once{} }) - return svc, kvstore, sqlStore, err + return svc, manager, kvstore, sqlStore, err } func setupTestMigratorServiceWithDeletionError( @@ -108,14 +124,14 @@ func setupTestMigratorServiceWithDeletionError( secretskv SecretsKVStore, sqlStore sqlstore.Store, kvstore kvstore.KVStore, -) *PluginSecretMigrationService { +) *MigrateToPluginService { t.Helper() fatalFlagOnce = sync.Once{} startupOnce = sync.Once{} cfg := setupTestConfig(t) secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore()) manager := NewFakeSecretsPluginManager(t, false) - migratorService := ProvidePluginSecretMigrationService( + migratorService := ProvideMigrateToPluginService( secretskv, cfg, sqlStore, @@ -142,7 +158,6 @@ func setupTestConfig(t *testing.T) *setting.Cfg { rawCfg := ` [secrets] use_plugin = true - migrate_to_plugin = true ` raw, err := ini.Load([]byte(rawCfg)) require.NoError(t, err) diff --git a/pkg/services/secrets/kvstore/remote_plugin.go b/pkg/services/secrets/kvstore/remote_plugin.go index 4a842a4643b..2d2c39b4e7b 100644 --- a/pkg/services/secrets/kvstore/remote_plugin.go +++ b/pkg/services/secrets/kvstore/remote_plugin.go @@ -189,6 +189,7 @@ func updateFatalFlag(ctx context.Context, skv secretsKVStorePlugin) { // Rather than updating the flag in several places, it is cleaner to just do this check once // Very early on. Once backwards compatibility to legacy secrets is gone in Grafana 10, this can go away as well fatalFlagOnce.Do(func() { + skv.log.Debug("Updating plugin startup error fatal flag") var err error if isFatal, _ := isPluginStartupErrorFatal(ctx, skv.kvstore); !isFatal && skv.backwardsCompatibilityDisabled { err = setPluginStartupErrorFatal(ctx, skv.kvstore, true) diff --git a/pkg/services/secrets/kvstore/test_helpers.go b/pkg/services/secrets/kvstore/test_helpers.go index fbcc2d387f2..4381d6cdaf8 100644 --- a/pkg/services/secrets/kvstore/test_helpers.go +++ b/pkg/services/secrets/kvstore/test_helpers.go @@ -65,10 +65,13 @@ func (f *FakeSecretsKVStore) Del(ctx context.Context, orgId int64, namespace str return nil } +// List all keys with an optional filter. If default values are provided, filter is not applied. func (f *FakeSecretsKVStore) Keys(ctx context.Context, orgId int64, namespace string, typ string) ([]Key, error) { res := make([]Key, 0) for k := range f.store { - if k.OrgId == orgId && k.Namespace == namespace && k.Type == typ { + if orgId == AllOrganizations && namespace == "" && typ == "" { + res = append(res, k) + } else if k.OrgId == orgId && k.Namespace == namespace && k.Type == typ { res = append(res, k) } } @@ -114,6 +117,14 @@ func buildKey(orgId int64, namespace string, typ string) Key { } } +func internalToProtoKey(k Key) *secretsmanagerplugin.Key { + return &secretsmanagerplugin.Key{ + OrgId: k.OrgId, + Namespace: k.Namespace, + Type: k.Type, + } +} + // Fake feature toggle - only need to check the backwards compatibility disabled flag type fakeFeatureToggles struct { returnValue bool @@ -131,40 +142,60 @@ func (f fakeFeatureToggles) IsEnabled(feature string) bool { } // Fake grpc secrets plugin impl -type fakeGRPCSecretsPlugin struct{} +type fakeGRPCSecretsPlugin struct { + kv map[Key]string +} func (c *fakeGRPCSecretsPlugin) GetSecret(ctx context.Context, in *secretsmanagerplugin.GetSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.GetSecretResponse, error) { + val, ok := c.kv[buildKey(in.KeyDescriptor.OrgId, in.KeyDescriptor.Namespace, in.KeyDescriptor.Type)] return &secretsmanagerplugin.GetSecretResponse{ - DecryptedValue: "bogus", - Exists: true, + DecryptedValue: val, + Exists: ok, }, nil } func (c *fakeGRPCSecretsPlugin) SetSecret(ctx context.Context, in *secretsmanagerplugin.SetSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.SetSecretResponse, error) { + c.kv[buildKey(in.KeyDescriptor.OrgId, in.KeyDescriptor.Namespace, in.KeyDescriptor.Type)] = in.Value return &secretsmanagerplugin.SetSecretResponse{}, nil } func (c *fakeGRPCSecretsPlugin) DeleteSecret(ctx context.Context, in *secretsmanagerplugin.DeleteSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.DeleteSecretResponse, error) { + delete(c.kv, buildKey(in.KeyDescriptor.OrgId, in.KeyDescriptor.Namespace, in.KeyDescriptor.Type)) return &secretsmanagerplugin.DeleteSecretResponse{}, nil } func (c *fakeGRPCSecretsPlugin) ListSecrets(ctx context.Context, in *secretsmanagerplugin.ListSecretsRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.ListSecretsResponse, error) { + res := make([]*secretsmanagerplugin.Key, 0) + for k := range c.kv { + if in.KeyDescriptor.OrgId == AllOrganizations && in.KeyDescriptor.Namespace == "" && in.KeyDescriptor.Type == "" { + res = append(res, internalToProtoKey(k)) + } else if k.OrgId == in.KeyDescriptor.OrgId && k.Namespace == in.KeyDescriptor.Namespace && k.Type == in.KeyDescriptor.Type { + res = append(res, internalToProtoKey(k)) + } + } return &secretsmanagerplugin.ListSecretsResponse{ - Keys: make([]*secretsmanagerplugin.Key, 0), + Keys: res, }, nil } func (c *fakeGRPCSecretsPlugin) RenameSecret(ctx context.Context, in *secretsmanagerplugin.RenameSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.RenameSecretResponse, error) { + oldKey := buildKey(in.KeyDescriptor.OrgId, in.KeyDescriptor.Namespace, in.KeyDescriptor.Type) + val := c.kv[oldKey] + delete(c.kv, oldKey) + c.kv[buildKey(in.KeyDescriptor.OrgId, in.NewNamespace, in.KeyDescriptor.Type)] = val return &secretsmanagerplugin.RenameSecretResponse{}, nil } func (c *fakeGRPCSecretsPlugin) GetAllSecrets(ctx context.Context, in *secretsmanagerplugin.GetAllSecretsRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.GetAllSecretsResponse, error) { + items := make([]*secretsmanagerplugin.Item, 0) + for k, v := range c.kv { + items = append(items, &secretsmanagerplugin.Item{ + Key: internalToProtoKey(k), + Value: v, + }) + } return &secretsmanagerplugin.GetAllSecretsResponse{ - Items: []*secretsmanagerplugin.Item{ - { - Value: "bogus", - }, - }, + Items: items, }, nil } @@ -174,15 +205,22 @@ var _ secretsmanagerplugin.SecretsManagerPlugin = &fakeGRPCSecretsPlugin{} // Fake plugin manager type fakePluginManager struct { shouldFailOnStart bool + plugin *plugins.Plugin } func (mg *fakePluginManager) SecretsManager() *plugins.Plugin { + if mg.plugin != nil { + return mg.plugin + } p := &plugins.Plugin{ - SecretsManager: &fakeGRPCSecretsPlugin{}, + SecretsManager: &fakeGRPCSecretsPlugin{ + kv: make(map[Key]string), + }, } p.RegisterClient(&fakePluginClient{ shouldFailOnStart: mg.shouldFailOnStart, }) + mg.plugin = p return p } @@ -205,3 +243,7 @@ func (pc *fakePluginClient) Start(_ context.Context) error { } return nil } + +func (pc *fakePluginClient) Stop(_ context.Context) error { + return nil +}