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 <leandro.deveikis@gmail.com>

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")
+}

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))

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,

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,

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
 	})

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)

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,

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
+}

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)
+}

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.

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,

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
+	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,
+		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())
+		}
+	})
+}

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)

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)

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
+}