diff --git a/pkg/services/datasources/service/secrets_mig.go b/pkg/services/datasources/service/secrets_mig.go index 5075ee6367a..84fa2bcc739 100644 --- a/pkg/services/datasources/service/secrets_mig.go +++ b/pkg/services/datasources/service/secrets_mig.go @@ -2,8 +2,10 @@ package service import ( "context" + "fmt" "github.com/grafana/grafana/pkg/infra/kvstore" + "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/featuremgmt" ) @@ -21,6 +23,7 @@ type DataSourceSecretMigrationService struct { dataSourcesService datasources.DataSourceService kvStore *kvstore.NamespacedKVStore features featuremgmt.FeatureToggles + log log.Logger } func ProvideDataSourceMigrationService( @@ -32,6 +35,7 @@ func ProvideDataSourceMigrationService( dataSourcesService: dataSourcesService, kvStore: kvstore.WithNamespace(kvStore, 0, secretType), features: features, + log: log.New("secrets.migration"), } } @@ -40,7 +44,7 @@ func (s *DataSourceSecretMigrationService) Migrate(ctx context.Context) error { if err != nil { return err } - + s.log.Debug(fmt.Sprint("secret migration status is ", migrationStatus)) // If this flag is true, delete secrets from the legacy secrets store as they are migrated disableSecretsCompatibility := s.features.IsEnabled(featuremgmt.FlagDisableSecretsCompatibility) // If migration hasn't happened, migrate to unified secrets and keep copy in legacy @@ -51,6 +55,7 @@ func (s *DataSourceSecretMigrationService) Migrate(ctx context.Context) error { needMigration := migrationStatus != completeSecretMigrationValue && disableSecretsCompatibility if needCompatibility || needMigration { + s.log.Debug("performing secret migration", "needs migration", needMigration, "needs compatibility", needCompatibility) query := &datasources.GetAllDataSourcesQuery{} err := s.dataSourcesService.GetAllDataSources(ctx, query) if err != nil { @@ -85,15 +90,17 @@ func (s *DataSourceSecretMigrationService) Migrate(ctx context.Context) error { } } + var newMigStatus string if disableSecretsCompatibility { - err = s.kvStore.Set(ctx, secretMigrationStatusKey, completeSecretMigrationValue) + newMigStatus = completeSecretMigrationValue } else { - err = s.kvStore.Set(ctx, secretMigrationStatusKey, compatibleSecretMigrationValue) + newMigStatus = compatibleSecretMigrationValue } - + err = s.kvStore.Set(ctx, secretMigrationStatusKey, newMigStatus) if err != nil { return err } + s.log.Debug(fmt.Sprint("set secret migration status to ", newMigStatus)) } return nil diff --git a/pkg/services/secrets/kvstore/cache.go b/pkg/services/secrets/kvstore/cache.go index 6fd62da9302..37985c74899 100644 --- a/pkg/services/secrets/kvstore/cache.go +++ b/pkg/services/secrets/kvstore/cache.go @@ -76,3 +76,7 @@ func (kv *CachedKVStore) Rename(ctx context.Context, orgId int64, namespace stri } return nil } + +func (kv *CachedKVStore) GetUnwrappedStore() SecretsKVStore { + return kv.store +} diff --git a/pkg/services/secrets/kvstore/helpers.go b/pkg/services/secrets/kvstore/helpers.go deleted file mode 100644 index ab6089b965a..00000000000 --- a/pkg/services/secrets/kvstore/helpers.go +++ /dev/null @@ -1,81 +0,0 @@ -package kvstore - -import ( - "context" - "testing" - - "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/services/secrets/database" - "github.com/grafana/grafana/pkg/services/secrets/manager" - "github.com/grafana/grafana/pkg/services/sqlstore" -) - -func SetupTestService(t *testing.T) SecretsKVStore { - t.Helper() - - sqlStore := sqlstore.InitTestDB(t) - store := database.ProvideSecretsStore(sqlstore.InitTestDB(t)) - secretsService := manager.SetupTestService(t, store) - - kv := &secretsKVStoreSQL{ - sqlStore: sqlStore, - log: log.New("secrets.kvstore"), - secretsService: secretsService, - decryptionCache: decryptionCache{ - cache: make(map[int64]cachedDecrypted), - }, - } - - return kv -} - -// In memory kv store used for testing -type FakeSecretsKVStore struct { - store map[Key]string -} - -func NewFakeSecretsKVStore() FakeSecretsKVStore { - return FakeSecretsKVStore{store: make(map[Key]string)} -} - -func (f FakeSecretsKVStore) Get(ctx context.Context, orgId int64, namespace string, typ string) (string, bool, error) { - value := f.store[buildKey(orgId, namespace, typ)] - found := value != "" - return value, found, nil -} - -func (f FakeSecretsKVStore) Set(ctx context.Context, orgId int64, namespace string, typ string, value string) error { - f.store[buildKey(orgId, namespace, typ)] = value - return nil -} - -func (f FakeSecretsKVStore) Del(ctx context.Context, orgId int64, namespace string, typ string) error { - delete(f.store, buildKey(orgId, namespace, typ)) - return nil -} - -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 { - res = append(res, k) - } - } - return res, nil -} - -func (f FakeSecretsKVStore) Rename(ctx context.Context, orgId int64, namespace string, typ string, newNamespace string) error { - f.store[buildKey(orgId, newNamespace, typ)] = f.store[buildKey(orgId, namespace, typ)] - delete(f.store, buildKey(orgId, namespace, typ)) - return nil -} - -func buildKey(orgId int64, namespace string, typ string) Key { - return Key{ - OrgId: orgId, - Namespace: namespace, - Type: typ, - } -} - -var _ SecretsKVStore = FakeSecretsKVStore{} diff --git a/pkg/services/secrets/kvstore/kvstore.go b/pkg/services/secrets/kvstore/kvstore.go index 377e014f9d2..14444387df1 100644 --- a/pkg/services/secrets/kvstore/kvstore.go +++ b/pkg/services/secrets/kvstore/kvstore.go @@ -2,9 +2,13 @@ package kvstore import ( "context" + "errors" + "fmt" "time" + "github.com/grafana/grafana/pkg/infra/kvstore" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/secrets" "github.com/grafana/grafana/pkg/services/sqlstore" ) @@ -14,7 +18,13 @@ const ( AllOrganizations = -1 ) -func ProvideService(sqlStore sqlstore.Store, secretsService secrets.Service, remoteCheck UseRemoteSecretsPluginCheck) SecretsKVStore { +func ProvideService( + sqlStore sqlstore.Store, + secretsService secrets.Service, + remoteCheck UseRemoteSecretsPluginCheck, + kvstore kvstore.KVStore, + features featuremgmt.FeatureToggles, +) (SecretsKVStore, error) { var store SecretsKVStore logger := log.New("secrets.kvstore") store = &secretsKVStoreSQL{ @@ -26,21 +36,34 @@ func ProvideService(sqlStore sqlstore.Store, secretsService secrets.Service, rem }, } if remoteCheck.ShouldUseRemoteSecretsPlugin() { - logger.Debug("secrets kvstore is using a remote plugin for secrets management") - secretsPlugin, err := remoteCheck.GetPlugin() - if err != nil { - logger.Error("plugin client was nil, falling back to SQL implementation") + // Attempt to start the plugin + secretsPlugin, err := remoteCheck.StartAndReturnPlugin(context.Background()) + namespacedKVStore := GetNamespacedKVStore(kvstore) + if err != nil || secretsPlugin == nil { + if isFatal, err2 := isPluginStartupErrorFatal(context.Background(), namespacedKVStore); isFatal || err2 != nil { + // plugin error was fatal or there was an error determining if the error was fatal + logger.Error("secrets management plugin is required to start -- exiting app") + if err2 != nil { + // TODO decide whether an error here should actually crash the app + return nil, err2 + } + return nil, err + } + logger.Error("error starting secrets plugin, falling back to SQL implementation") } else { store = &secretsKVStorePlugin{ - secretsPlugin: secretsPlugin, - secretsService: secretsService, - log: logger, + secretsPlugin: secretsPlugin, + secretsService: secretsService, + log: logger, + kvstore: namespacedKVStore, + backwardsCompatibilityDisabled: features.IsEnabled(featuremgmt.FlagDisableSecretsCompatibility), } } } else { logger.Debug("secrets kvstore is using the default (SQL) implementation for secrets management") } - return NewCachedKVStore(store, 5*time.Second, 5*time.Minute) + + return NewCachedKVStore(store, 5*time.Second, 5*time.Minute), nil } // SecretsKVStore is an interface for k/v store. @@ -94,3 +117,23 @@ func (kv *FixedKVStore) Rename(ctx context.Context, newNamespace string) error { kv.Namespace = newNamespace return nil } + +// Helpers to determine whether plugin startup failures are fatal to the app +func GetNamespacedKVStore(kv kvstore.KVStore) *kvstore.NamespacedKVStore { + return kvstore.WithNamespace(kv, kvstore.AllOrganizations, PluginNamespace) +} + +func isPluginStartupErrorFatal(ctx context.Context, kvstore *kvstore.NamespacedKVStore) (bool, error) { + _, exists, err := kvstore.Get(ctx, QuitOnPluginStartupFailureKey) + if err != nil { + return false, errors.New(fmt.Sprint("error retrieving key ", QuitOnPluginStartupFailureKey, " from kvstore. error: ", err.Error())) + } + return exists, nil +} + +func setPluginStartupErrorFatal(ctx context.Context, kvstore *kvstore.NamespacedKVStore, isFatal bool) error { + if !isFatal { + return kvstore.Del(ctx, QuitOnPluginStartupFailureKey) + } + return kvstore.Set(ctx, QuitOnPluginStartupFailureKey, "true") +} diff --git a/pkg/services/secrets/kvstore/model.go b/pkg/services/secrets/kvstore/model.go index 6643de3a1a1..9e7db1b0c5d 100644 --- a/pkg/services/secrets/kvstore/model.go +++ b/pkg/services/secrets/kvstore/model.go @@ -4,6 +4,11 @@ import ( "time" ) +const ( + QuitOnPluginStartupFailureKey = "quit_on_secrets_plugin_startup_failure" + PluginNamespace = "secretsmanagerplugin" +) + // Item stored in k/v store. type Item struct { Id int64 diff --git a/pkg/services/secrets/kvstore/plugin_fatal_crash_test.go b/pkg/services/secrets/kvstore/plugin_fatal_crash_test.go new file mode 100644 index 00000000000..ffa4b5afd91 --- /dev/null +++ b/pkg/services/secrets/kvstore/plugin_fatal_crash_test.go @@ -0,0 +1,149 @@ +package kvstore + +import ( + "context" + "errors" + "sync" + "testing" + + "github.com/grafana/grafana/pkg/infra/kvstore" + "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/services/sqlstore/mockstore" + "github.com/grafana/grafana/pkg/setting" + "github.com/stretchr/testify/require" + "gopkg.in/ini.v1" +) + +// 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) + require.Error(t, err) + require.Nil(t, svc) +} + +// 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) + require.NoError(t, err) + require.IsType(t, &CachedKVStore{}, svc) + cachedKv, _ := svc.(*CachedKVStore) + require.IsType(t, &secretsKVStoreSQL{}, cachedKv.GetUnwrappedStore()) +} + +// 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) + require.NoError(t, err) + require.NotNil(t, svc) + err = svc.Set(context.Background(), 0, "datasource", "postgres", "my secret") + require.NoError(t, err) + isFatal, err := isPluginStartupErrorFatal(context.Background(), GetNamespacedKVStore(kvstore)) + require.NoError(t, err) + require.True(t, isFatal) +} + +// 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) + require.NoError(t, err) + require.NotNil(t, svc) + val, exists, err := svc.Get(context.Background(), 0, "datasource", "postgres") + require.NoError(t, err) + require.NotNil(t, val) + require.True(t, exists) + isFatal, err := isPluginStartupErrorFatal(context.Background(), GetNamespacedKVStore(kvstore)) + require.NoError(t, err) + require.False(t, isFatal) +} + +// 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) + require.NoError(t, err) + + migration := setupTestMigratorServiceWithDeletionError(t, svc, &mockstore.SQLStoreMock{ + ExpectedError: errors.New("random error"), + }, kvstore) + err = migration.Migrate(context.Background()) + require.Error(t, err) + isFatal, err := isPluginStartupErrorFatal(context.Background(), GetNamespacedKVStore(kvstore)) + require.NoError(t, err) + require.False(t, isFatal) +} + +func setupFatalCrashTest( + t *testing.T, + shouldRemoteCheckError bool, + isPluginErrorFatal bool, + isBackwardsCompatDisabled bool, +) (SecretsKVStore, kvstore.KVStore, *sqlstore.SQLStore, error) { + t.Helper() + sqlStore := sqlstore.InitTestDB(t) + secretService := fakes.FakeSecretsService{} + var remoteCheck *mockRemoteSecretsPluginCheck + if shouldRemoteCheckError { + remoteCheck = provideMockRemotePluginCheckWithErr() + } else { + remoteCheck = provideMockRemotePluginCheck() + } + kvstore := kvstore.ProvideService(sqlStore) + if isPluginErrorFatal { + _ = setPluginStartupErrorFatal(context.Background(), GetNamespacedKVStore(kvstore), true) + } + features := NewFakeFeatureToggles(t, isBackwardsCompatDisabled) + svc, err := ProvideService(sqlStore, secretService, remoteCheck, kvstore, features) + t.Cleanup(func() { + fatalFlagOnce = sync.Once{} + }) + return svc, kvstore, sqlStore, err +} + +func setupTestMigratorServiceWithDeletionError( + t *testing.T, + secretskv SecretsKVStore, + sqlStore sqlstore.Store, + kvstore kvstore.KVStore, +) *PluginSecretMigrationService { + t.Helper() + rawCfg := ` + [secrets] + use_plugin = true + migrate_to_plugin = true + ` + raw, err := ini.Load([]byte(rawCfg)) + require.NoError(t, err) + cfg := &setting.Cfg{Raw: raw} + remoteCheck := provideMockRemotePluginCheck() + secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore()) + getAllFuncOverride := func(ctx context.Context) ([]Item, error) { + items := make([]Item, 0) + var orgId int64 = 1 + str := "random string" + items = append(items, Item{ + Id: 1, + OrgId: &orgId, + Type: &str, + Namespace: &str, + Value: "bogus", + }) + return items, nil + } + migratorService := ProvidePluginSecretMigrationService( + secretskv, + cfg, + sqlStore, + secretsService, + remoteCheck, + kvstore, + ) + // TODO refactor Migrator to allow us to override the entire sqlstore with a mock instead + migratorService.overrideGetAllFunc(getAllFuncOverride) + return migratorService +} diff --git a/pkg/services/secrets/kvstore/plugin_mig.go b/pkg/services/secrets/kvstore/plugin_mig.go index dcf9aad066e..92694a65003 100644 --- a/pkg/services/secrets/kvstore/plugin_mig.go +++ b/pkg/services/secrets/kvstore/plugin_mig.go @@ -3,6 +3,7 @@ package kvstore import ( "context" + "github.com/grafana/grafana/pkg/infra/kvstore" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/secrets" "github.com/grafana/grafana/pkg/services/sqlstore" @@ -18,6 +19,8 @@ type PluginSecretMigrationService struct { sqlStore sqlstore.Store secretsService secrets.Service remoteCheck UseRemoteSecretsPluginCheck + kvstore kvstore.KVStore + getAllFunc func(ctx context.Context) ([]Item, error) } func ProvidePluginSecretMigrationService( @@ -26,6 +29,7 @@ func ProvidePluginSecretMigrationService( sqlStore sqlstore.Store, secretsService secrets.Service, remoteCheck UseRemoteSecretsPluginCheck, + kvstore kvstore.KVStore, ) *PluginSecretMigrationService { return &PluginSecretMigrationService{ secretsStore: secretsStore, @@ -34,13 +38,13 @@ func ProvidePluginSecretMigrationService( sqlStore: sqlStore, secretsService: secretsService, remoteCheck: remoteCheck, + kvstore: kvstore, } } func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error { // Check if we should migrate to plugin - default false - if s.cfg.SectionWithEnvOverrides("secrets").Key("migrate_to_plugin").MustBool(false) && - s.remoteCheck.ShouldUseRemoteSecretsPlugin() { + if s.cfg.SectionWithEnvOverrides("secrets").Key("migrate_to_plugin").MustBool(false) && s.remoteCheck.ShouldUseRemoteSecretsPlugin() { s.logger.Debug("starting migration of unified secrets to the plugin") // we need to instantiate the secretsKVStore as this is not on wire, and in this scenario, // the secrets store would be the plugin. @@ -51,6 +55,14 @@ func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error { decryptionCache: decryptionCache{ cache: make(map[int64]cachedDecrypted), }, + GetAllFuncOverride: s.getAllFunc, + } + + // before we start migrating, check see if plugin startup failures were already fatal + namespacedKVStore := GetNamespacedKVStore(s.kvstore) + wasFatal, err := isPluginStartupErrorFatal(ctx, namespacedKVStore) + if err != nil { + s.logger.Warn("unabled to determine whether plugin startup failures are fatal - continuing migration anyway.") } allSec, err := secretsSql.GetAll(ctx) @@ -66,9 +78,19 @@ func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error { } s.logger.Debug("migrated unified secrets to plugin", "number of secrets", len(allSec)) // as no err was returned, when we delete all the secrets from the sql store - for _, sec := range allSec { + for index, sec := range allSec { err = secretsSql.Del(ctx, *sec.OrgId, *sec.Namespace, *sec.Type) if err != nil { + s.logger.Error("plugin migrator encountered error while deleting unified secrets") + if index == 0 && !wasFatal { + // old unified secrets still exists, so plugin startup errors are still not fatal, unless they were before we started + err := setPluginStartupErrorFatal(ctx, namespacedKVStore, false) + if err != nil { + s.logger.Error("error reverting plugin failure fatal status", "error", err.Error()) + } else { + s.logger.Debug("application will continue to function without the secrets plugin") + } + } return err } } @@ -76,3 +98,10 @@ func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error { } return nil } + +// This is here to support testing and should normally not be called +// An edge case we are unit testing requires the GetAll function to return a value, but the Del function to return an error. +// This is not possible with the code as written, so this override function is a workaround. Should be refactored. +func (s *PluginSecretMigrationService) overrideGetAllFunc(getAllFunc func(ctx context.Context) ([]Item, error)) { + s.getAllFunc = getAllFunc +} diff --git a/pkg/services/secrets/kvstore/plugin_mig_test.go b/pkg/services/secrets/kvstore/plugin_mig_test.go index 32350d1e0b2..dd6ea614654 100644 --- a/pkg/services/secrets/kvstore/plugin_mig_test.go +++ b/pkg/services/secrets/kvstore/plugin_mig_test.go @@ -4,11 +4,12 @@ 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" "gopkg.in/ini.v1" @@ -82,13 +83,13 @@ func setupTestMigratorService(t *testing.T) (*PluginSecretMigrationService, Secr // this is to init the sql secret store inside the migration sqlStore := sqlstore.InitTestDB(t) secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore()) - migratorService := ProvidePluginSecretMigrationService( secretsStoreForPlugin, cfg, sqlStore, secretsService, remoteCheck, + kvstore.ProvideService(sqlStore), ) secretsSql := &secretsKVStoreSQL{ @@ -102,20 +103,3 @@ func setupTestMigratorService(t *testing.T) (*PluginSecretMigrationService, Secr return migratorService, secretsStoreForPlugin, secretsSql } - -// -type mockRemoteSecretsPluginCheck struct { - UseRemoteSecretsPluginCheck -} - -func provideMockRemotePluginCheck() *mockRemoteSecretsPluginCheck { - return &mockRemoteSecretsPluginCheck{} -} - -func (c *mockRemoteSecretsPluginCheck) ShouldUseRemoteSecretsPlugin() bool { - return true -} - -func (c *mockRemoteSecretsPluginCheck) GetPlugin() (secretsmanagerplugin.SecretsManagerPlugin, error) { - return nil, nil -} diff --git a/pkg/services/secrets/kvstore/remote_plugin.go b/pkg/services/secrets/kvstore/remote_plugin.go index a0b4a83cb80..a038ca7ce83 100644 --- a/pkg/services/secrets/kvstore/remote_plugin.go +++ b/pkg/services/secrets/kvstore/remote_plugin.go @@ -2,21 +2,30 @@ package kvstore import ( "context" + "sync" + "github.com/grafana/grafana/pkg/infra/kvstore" "github.com/grafana/grafana/pkg/infra/log" smp "github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/secrets" ) +var ( + fatalFlagOnce sync.Once +) + // secretsKVStorePlugin provides a key/value store backed by the Grafana plugin gRPC interface type secretsKVStorePlugin struct { - log log.Logger - secretsPlugin smp.SecretsManagerPlugin - secretsService secrets.Service + log log.Logger + secretsPlugin smp.SecretsManagerPlugin + secretsService secrets.Service + kvstore *kvstore.NamespacedKVStore + backwardsCompatibilityDisabled bool } // Get an item from the store +// If it is the first time a secret has been retrieved and backwards compatibility is disabled, mark plugin startup errors fatal func (kv *secretsKVStorePlugin) Get(ctx context.Context, orgId int64, namespace string, typ string) (string, bool, error) { req := &smp.GetSecretRequest{ KeyDescriptor: &smp.Key{ @@ -32,10 +41,15 @@ func (kv *secretsKVStorePlugin) Get(ctx context.Context, orgId int64, namespace err = wrapUserFriendlySecretError(res.UserFriendlyError) } + if res.Exists { + updateFatalFlag(ctx, *kv) + } + return res.DecryptedValue, res.Exists, err } // Set an item in the store +// If it is the first time a secret has been set and backwards compatibility is disabled, mark plugin startup errors fatal func (kv *secretsKVStorePlugin) Set(ctx context.Context, orgId int64, namespace string, typ string, value string) error { req := &smp.SetSecretRequest{ KeyDescriptor: &smp.Key{ @@ -51,6 +65,8 @@ func (kv *secretsKVStorePlugin) Set(ctx context.Context, orgId int64, namespace err = wrapUserFriendlySecretError(res.UserFriendlyError) } + updateFatalFlag(ctx, *kv) + return err } @@ -124,6 +140,26 @@ func parseKeys(keys []*smp.Key) []Key { return newKeys } +func updateFatalFlag(ctx context.Context, skv secretsKVStorePlugin) { + // This function makes the most sense in here because it handles all possible scenarios: + // - User changed backwards compatibility flag, so we have to migrate secrets either to or from the plugin (get or set) + // - Migration is on, so we migrate secrets to the plugin (set) + // - User doesn't migrate, but stores a new secret in the plugin (set) + // 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() { + var err error + if isFatal, _ := isPluginStartupErrorFatal(ctx, skv.kvstore); !isFatal && skv.backwardsCompatibilityDisabled { + err = setPluginStartupErrorFatal(ctx, skv.kvstore, true) + } else if isFatal && !skv.backwardsCompatibilityDisabled { + err = setPluginStartupErrorFatal(ctx, skv.kvstore, false) + } + if err != nil { + skv.log.Error("failed to set plugin error fatal flag", err.Error()) + } + }) +} + func wrapUserFriendlySecretError(ufe string) datasources.ErrDatasourceSecretsPluginUserFriendly { return datasources.ErrDatasourceSecretsPluginUserFriendly{Err: ufe} } diff --git a/pkg/services/secrets/kvstore/remote_plugin_check.go b/pkg/services/secrets/kvstore/remote_plugin_check.go index 657f705d0b0..a415bdbb68c 100644 --- a/pkg/services/secrets/kvstore/remote_plugin_check.go +++ b/pkg/services/secrets/kvstore/remote_plugin_check.go @@ -1,26 +1,34 @@ package kvstore import ( + "context" + + "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin" ) type UseRemoteSecretsPluginCheck interface { ShouldUseRemoteSecretsPlugin() bool - GetPlugin() (secretsmanagerplugin.SecretsManagerPlugin, error) + StartAndReturnPlugin(ctx context.Context) (secretsmanagerplugin.SecretsManagerPlugin, error) } type OSSRemoteSecretsPluginCheck struct { - UseRemoteSecretsPluginCheck + log log.Logger } func ProvideRemotePluginCheck() *OSSRemoteSecretsPluginCheck { - return &OSSRemoteSecretsPluginCheck{} + return &OSSRemoteSecretsPluginCheck{ + log: log.New("ossremotesecretsplugincheck"), + } } -func (c *OSSRemoteSecretsPluginCheck) ShouldUseRemoteSecretsPlugin() bool { +func (c OSSRemoteSecretsPluginCheck) ShouldUseRemoteSecretsPlugin() bool { return false } -func (c *OSSRemoteSecretsPluginCheck) GetPlugin() (secretsmanagerplugin.SecretsManagerPlugin, error) { +func (c OSSRemoteSecretsPluginCheck) StartAndReturnPlugin(ctx context.Context) (secretsmanagerplugin.SecretsManagerPlugin, error) { + c.log.Warn("OSSRemoteSecretsPluginCheck.StartAndReturnPlugin() was called by mistake. Secrets Manager plugins are enterprise only.") return nil, nil } + +var _ UseRemoteSecretsPluginCheck = OSSRemoteSecretsPluginCheck{} diff --git a/pkg/services/secrets/kvstore/sql.go b/pkg/services/secrets/kvstore/sql.go index 0c613629933..767f2604b34 100644 --- a/pkg/services/secrets/kvstore/sql.go +++ b/pkg/services/secrets/kvstore/sql.go @@ -17,6 +17,8 @@ type secretsKVStoreSQL struct { sqlStore sqlstore.Store secretsService secrets.Service decryptionCache decryptionCache + // This is here to support testing and should normally not be set + GetAllFuncOverride func(ctx context.Context) ([]Item, error) } type decryptionCache struct { @@ -227,6 +229,9 @@ func (kv *secretsKVStoreSQL) Rename(ctx context.Context, orgId int64, namespace // GetAll this returns all the secrets stored in the database. This is not part of the kvstore interface as we // only need it for migration from sql to plugin at this moment func (kv *secretsKVStoreSQL) GetAll(ctx context.Context) ([]Item, error) { + if kv.GetAllFuncOverride != nil { + return kv.GetAllFuncOverride(ctx) + } var items []Item err := kv.sqlStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error { return dbSession.Find(&items) diff --git a/pkg/services/secrets/kvstore/test_helpers.go b/pkg/services/secrets/kvstore/test_helpers.go new file mode 100644 index 00000000000..f75041563b6 --- /dev/null +++ b/pkg/services/secrets/kvstore/test_helpers.go @@ -0,0 +1,158 @@ +package kvstore + +import ( + "context" + "errors" + "testing" + + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin" + "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/secrets/database" + "github.com/grafana/grafana/pkg/services/secrets/manager" + "github.com/grafana/grafana/pkg/services/sqlstore" + "google.golang.org/grpc" +) + +func SetupTestService(t *testing.T) SecretsKVStore { + t.Helper() + + sqlStore := sqlstore.InitTestDB(t) + store := database.ProvideSecretsStore(sqlstore.InitTestDB(t)) + secretsService := manager.SetupTestService(t, store) + + kv := &secretsKVStoreSQL{ + sqlStore: sqlStore, + log: log.New("secrets.kvstore"), + secretsService: secretsService, + decryptionCache: decryptionCache{ + cache: make(map[int64]cachedDecrypted), + }, + } + + return kv +} + +// In memory kv store used for testing +type FakeSecretsKVStore struct { + store map[Key]string +} + +func NewFakeSecretsKVStore() FakeSecretsKVStore { + return FakeSecretsKVStore{store: make(map[Key]string)} +} + +func (f FakeSecretsKVStore) Get(ctx context.Context, orgId int64, namespace string, typ string) (string, bool, error) { + value := f.store[buildKey(orgId, namespace, typ)] + found := value != "" + return value, found, nil +} + +func (f FakeSecretsKVStore) Set(ctx context.Context, orgId int64, namespace string, typ string, value string) error { + f.store[buildKey(orgId, namespace, typ)] = value + return nil +} + +func (f FakeSecretsKVStore) Del(ctx context.Context, orgId int64, namespace string, typ string) error { + delete(f.store, buildKey(orgId, namespace, typ)) + return nil +} + +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 { + res = append(res, k) + } + } + return res, nil +} + +func (f FakeSecretsKVStore) Rename(ctx context.Context, orgId int64, namespace string, typ string, newNamespace string) error { + f.store[buildKey(orgId, newNamespace, typ)] = f.store[buildKey(orgId, namespace, typ)] + delete(f.store, buildKey(orgId, namespace, typ)) + return nil +} + +func buildKey(orgId int64, namespace string, typ string) Key { + return Key{ + OrgId: orgId, + Namespace: namespace, + Type: typ, + } +} + +// Fake feature toggle - only need to check the backwards compatibility disabled flag +type fakeFeatureToggles struct { + returnValue bool +} + +func NewFakeFeatureToggles(t *testing.T, returnValue bool) featuremgmt.FeatureToggles { + t.Helper() + return fakeFeatureToggles{ + returnValue: returnValue, + } +} + +func (f fakeFeatureToggles) IsEnabled(feature string) bool { + return f.returnValue +} + +// Fake Remote Secrets Plugin Check +type mockRemoteSecretsPluginCheck struct { + UseRemoteSecretsPluginCheck + shouldReturnError bool +} + +func provideMockRemotePluginCheck() *mockRemoteSecretsPluginCheck { + return &mockRemoteSecretsPluginCheck{} +} + +func provideMockRemotePluginCheckWithErr() *mockRemoteSecretsPluginCheck { + return &mockRemoteSecretsPluginCheck{ + shouldReturnError: true, + } +} + +func (c *mockRemoteSecretsPluginCheck) ShouldUseRemoteSecretsPlugin() bool { + return true +} + +func (c *mockRemoteSecretsPluginCheck) StartAndReturnPlugin(ctx context.Context) (secretsmanagerplugin.SecretsManagerPlugin, error) { + var err error + if c.shouldReturnError { + err = errors.New("bogus") + } + return &fakeGRPCSecretsPlugin{}, err +} + +// Fake grpc secrets plugin impl +type fakeGRPCSecretsPlugin struct{} + +func (c *fakeGRPCSecretsPlugin) GetSecret(ctx context.Context, in *secretsmanagerplugin.GetSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.GetSecretResponse, error) { + return &secretsmanagerplugin.GetSecretResponse{ + DecryptedValue: "bogus", + Exists: true, + }, nil +} + +func (c *fakeGRPCSecretsPlugin) SetSecret(ctx context.Context, in *secretsmanagerplugin.SetSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.SetSecretResponse, error) { + return &secretsmanagerplugin.SetSecretResponse{}, nil +} + +func (c *fakeGRPCSecretsPlugin) DeleteSecret(ctx context.Context, in *secretsmanagerplugin.DeleteSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.DeleteSecretResponse, error) { + return &secretsmanagerplugin.DeleteSecretResponse{}, nil +} + +func (c *fakeGRPCSecretsPlugin) ListSecrets(ctx context.Context, in *secretsmanagerplugin.ListSecretsRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.ListSecretsResponse, error) { + return &secretsmanagerplugin.ListSecretsResponse{ + Keys: make([]*secretsmanagerplugin.Key, 0), + }, nil +} + +func (c *fakeGRPCSecretsPlugin) RenameSecret(ctx context.Context, in *secretsmanagerplugin.RenameSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.RenameSecretResponse, error) { + return &secretsmanagerplugin.RenameSecretResponse{}, nil +} + +var _ SecretsKVStore = FakeSecretsKVStore{} +var _ secretsmanagerplugin.SecretsManagerPlugin = &fakeGRPCSecretsPlugin{}