Encryption: Cleanup and add logging (#42084)

* Encryption: Add more logs

* Add logging and checks

* Removed unused methods from secrets service

* Refactor and update tests

* Address review feedback

pkg/expr/service_test.go

@@ -13,10 +13,8 @@ import (
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/components/simplejson"
 	"github.com/grafana/grafana/pkg/models"
-	"github.com/grafana/grafana/pkg/services/encryption/ossencryption"
-	"github.com/grafana/grafana/pkg/services/kmsproviders/osskmsproviders"
 	"github.com/grafana/grafana/pkg/services/secrets/fakes"
-	"github.com/grafana/grafana/pkg/services/secrets/manager"
+	secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/stretchr/testify/require"
 )
@@ -31,16 +29,8 @@ func TestService(t *testing.T) {
 	}
 
 	cfg := setting.NewCfg()
-	encr := ossencryption.ProvideService()
-	settings := setting.ProvideProvider(cfg)
-
-	secretsService, err := manager.ProvideSecretsService(
-		fakes.NewFakeSecretsStore(),
-		osskmsproviders.ProvideService(encr, settings),
-		encr,
-		settings,
-	)
-	require.NoError(t, err)
+
+	secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore())
 
 	s := Service{
 		cfg:            cfg,

pkg/services/secrets/database/database.go

@@ -13,15 +13,15 @@ import (
 
 const dataKeysTable = "data_keys"
 
-var logger = log.New("secrets-store")
-
 type SecretsStoreImpl struct {
 	sqlStore *sqlstore.SQLStore
+	log      log.Logger
 }
 
 func ProvideSecretsStore(sqlStore *sqlstore.SQLStore) *SecretsStoreImpl {
 	return &SecretsStoreImpl{
 		sqlStore: sqlStore,
+		log:      log.New("secrets.store"),
 	}
 }
 
@@ -42,7 +42,7 @@ func (ss *SecretsStoreImpl) GetDataKey(ctx context.Context, name string) (*secre
 	}
 
 	if err != nil {
-		logger.Error("Failed getting data key", "err", err, "name", name)
+		ss.log.Error("Failed to get data key", "err", err, "name", name)
 		return nil, fmt.Errorf("failed getting data key: %w", err)
 	}
 

pkg/services/secrets/manager/manager.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/encryption"
 	"github.com/grafana/grafana/pkg/services/kmsproviders"
 	"github.com/grafana/grafana/pkg/services/secrets"
@@ -24,6 +25,7 @@ type SecretsService struct {
 	currentProvider string
 	providers       map[string]secrets.Provider
 	dataKeyCache    map[string]dataKeyCacheItem
+	log             log.Logger
 }
 
 func ProvideSecretsService(
@@ -37,8 +39,20 @@ func ProvideSecretsService(
 		return nil, err
 	}
 
+	logger := log.New("secrets")
+	enabled := settings.IsFeatureToggleEnabled(secrets.EnvelopeEncryptionFeatureToggle)
 	currentProvider := settings.KeyValue("security", "encryption_provider").MustString(kmsproviders.Default)
 
+	if _, ok := providers[currentProvider]; enabled && !ok {
+		return nil, fmt.Errorf("missing configuration for current encryption provider %s", currentProvider)
+	}
+
+	if !enabled && currentProvider != kmsproviders.Default {
+		logger.Warn("Changing encryption provider requires enabling envelope encryption feature")
+	}
+
+	logger.Debug("Envelope encryption state", "enabled", enabled, "current provider", currentProvider)
+
 	s := &SecretsService{
 		store:           store,
 		enc:             enc,
@@ -46,6 +60,7 @@ func ProvideSecretsService(
 		providers:       providers,
 		currentProvider: currentProvider,
 		dataKeyCache:    make(map[string]dataKeyCacheItem),
+		log:             logger,
 	}
 
 	return s, nil
@@ -133,6 +148,7 @@ func (s *SecretsService) Decrypt(ctx context.Context, payload []byte) ([]byte, e
 
 		dataKey, err = s.dataKey(ctx, string(key))
 		if err != nil {
+			s.log.Error("Failed to lookup data key", "name", string(key), "error", err)
 			return nil, err
 		}
 	}
@@ -274,14 +290,6 @@ func (s *SecretsService) dataKey(ctx context.Context, name string) ([]byte, erro
 	return decrypted, nil
 }
 
-func (s *SecretsService) RegisterProvider(providerID string, provider secrets.Provider) {
-	s.providers[providerID] = provider
-}
-
-func (s *SecretsService) CurrentProviderID() string {
-	return s.currentProvider
-}
-
 func (s *SecretsService) GetProviders() map[string]secrets.Provider {
 	return s.providers
 }

pkg/services/secrets/manager/manager_test.go

@@ -152,76 +152,56 @@ func TestSecretsService_UseCurrentProvider(t *testing.T) {
 		assert.Equal(t, "secretKey", svc.currentProvider)
 	})
 
-	t.Run("When encryption_provider value is set, should use it as a current provider", func(t *testing.T) {
-		rawCfg := `[security]
-			secret_key = sdDkslslld
-			encryption_provider = awskms.second_key`
-
-		raw, err := ini.Load([]byte(rawCfg))
-		require.NoError(t, err)
-
-		cfg := &setting.Cfg{Raw: raw, FeatureToggles: map[string]bool{secrets.EnvelopeEncryptionFeatureToggle: true}}
-		settings := &setting.OSSImpl{Cfg: cfg}
-
-		encr := ossencryption.ProvideService()
-		svc, err := ProvideSecretsService(
-			database.ProvideSecretsStore(sqlstore.InitTestDB(t)),
-			osskmsproviders.ProvideService(encr, settings),
-			encr,
-			settings,
-		)
-		require.NoError(t, err)
-
-		assert.Equal(t, "awskms.second_key", svc.currentProvider)
-	})
-
-	t.Run("Should use encrypt/decrypt methods of the current provider", func(t *testing.T) {
+	t.Run("Should use encrypt/decrypt methods of the current encryption provider", func(t *testing.T) {
 		rawCfg := `
 		[security]
 		secret_key = sdDkslslld
-		encryption_provider = fake-provider.some-key
+		encryption_provider = fakeProvider.v1
+		available_encryption_providers = fakeProvider.v1
 
-		[security.encryption.fake-provider.some-key]
+		[security.encryption.fakeProvider.v1]
 		`
 
 		raw, err := ini.Load([]byte(rawCfg))
 		require.NoError(t, err)
 
-		cfg := &setting.Cfg{Raw: raw, FeatureToggles: map[string]bool{secrets.EnvelopeEncryptionFeatureToggle: true}}
-		settings := &setting.OSSImpl{Cfg: cfg}
-
+		providerID := "fakeProvider.v1"
+		settings := &setting.OSSImpl{
+			Cfg: &setting.Cfg{
+				Raw:            raw,
+				FeatureToggles: map[string]bool{secrets.EnvelopeEncryptionFeatureToggle: true},
+			},
+		}
+		encr := ossencryption.ProvideService()
+		kms := newFakeKMS(osskmsproviders.ProvideService(encr, settings))
 		secretStore := database.ProvideSecretsStore(sqlstore.InitTestDB(t))
-		fake := fakeProvider{}
-		providerID := "fake-provider.some-key"
 
-		encr := ossencryption.ProvideService()
 		svcEncrypt, err := ProvideSecretsService(
 			secretStore,
-			osskmsproviders.ProvideService(encr, settings),
+			&kms,
 			encr,
 			settings,
 		)
 		require.NoError(t, err)
 
-		svcEncrypt.RegisterProvider(providerID, &fake)
-		require.NoError(t, err)
-		assert.Equal(t, providerID, svcEncrypt.CurrentProviderID())
+		assert.Equal(t, providerID, svcEncrypt.currentProvider)
 		assert.Equal(t, 2, len(svcEncrypt.GetProviders()))
+
 		encrypted, _ := svcEncrypt.Encrypt(context.Background(), []byte{}, secrets.WithoutScope())
-		assert.True(t, fake.encryptCalled)
+		assert.True(t, kms.fake.encryptCalled)
 
 		// secret service tries to find a DEK in a cache first before calling provider's decrypt
 		// to bypass the cache, we set up one more secrets service to test decrypting
 		svcDecrypt, err := ProvideSecretsService(
 			secretStore,
-			osskmsproviders.ProvideService(encr, settings),
+			&kms,
 			encr,
 			settings,
 		)
 		require.NoError(t, err)
-		svcDecrypt.RegisterProvider(providerID, &fake)
+
 		_, _ = svcDecrypt.Decrypt(context.Background(), encrypted)
-		assert.True(t, fake.decryptCalled, "fake provider's decrypt should be called")
+		assert.True(t, kms.fake.decryptCalled, "fake provider's decrypt should be called")
 	})
 }
 
@@ -239,3 +219,25 @@ func (p *fakeProvider) Decrypt(_ context.Context, _ []byte) ([]byte, error) {
 	p.decryptCalled = true
 	return []byte{}, nil
 }
+
+type fakeKMS struct {
+	kms  osskmsproviders.Service
+	fake *fakeProvider
+}
+
+func newFakeKMS(kms osskmsproviders.Service) fakeKMS {
+	return fakeKMS{
+		kms:  kms,
+		fake: &fakeProvider{},
+	}
+}
+
+func (f *fakeKMS) Provide() (map[string]secrets.Provider, error) {
+	providers, err := f.kms.Provide()
+	if err != nil {
+		return providers, err
+	}
+
+	providers["fakeProvider.v1"] = f.fake
+	return providers, nil
+}