[v9.3.x] Encryption: Cache new DEKs (only) after a caution period (#62177)

Encryption: Cache new DEKs (only) after a caution period (#60664)

* Encryption: Cache new DEKs (only) after commit

* Fix typo

* Update secrets manager tests with new failing case

* Update secrets manager tests with new clarifications (comments)

* Correct broken method calls

* Unify methods

* Cache data keys only after a caution period

* Caution period for data keys caching only for encrypt ops

(cherry picked from commit c4e067d49d)

Co-authored-by: Joan López de la Franca Beltran <5459617+joanlopez@users.noreply.github.com>

pkg/services/secrets/manager/cache.go

@@ -8,10 +8,6 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 )
 
-var (
-	now = time.Now
-)
-
 type dataKeyCacheEntry struct {
 	id         string
 	label      string
@@ -75,13 +71,21 @@ func (c *dataKeyCache) getByLabel(label string) (*dataKeyCacheEntry, bool) {
 	return entry, true
 }
 
-func (c *dataKeyCache) add(entry *dataKeyCacheEntry) {
+func (c *dataKeyCache) addById(entry *dataKeyCacheEntry) {
 	c.mtx.Lock()
 	defer c.mtx.Unlock()
 
 	entry.expiration = now().Add(c.cacheTTL)
 
 	c.byId[entry.id] = entry
+}
+
+func (c *dataKeyCache) addByLabel(entry *dataKeyCacheEntry) {
+	c.mtx.Lock()
+	defer c.mtx.Unlock()
+
+	entry.expiration = now().Add(c.cacheTTL)
+
 	c.byLabel[entry.label] = entry
 }
 

pkg/services/secrets/manager/manager.go

@@ -19,15 +19,21 @@ import (
 	"github.com/grafana/grafana/pkg/services/secrets"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/util"
+
 	"github.com/prometheus/client_golang/prometheus"
 	"golang.org/x/sync/errgroup"
-	"xorm.io/xorm"
 )
 
 const (
 	keyIdDelimiter = '#'
 )
 
+var (
+	// now is used for testing purposes,
+	// as a way to fake time.Now function.
+	now = time.Now
+)
+
 type SecretsService struct {
 	store      secrets.Store
 	enc        encryption.Internal
@@ -152,10 +158,6 @@ func (s *SecretsService) encryptedWithEnvelopeEncryption(payload []byte) bool {
 var b64 = base64.RawStdEncoding
 
 func (s *SecretsService) Encrypt(ctx context.Context, payload []byte, opt secrets.EncryptionOptions) ([]byte, error) {
-	return s.EncryptWithDBSession(ctx, payload, opt, nil)
-}
-
-func (s *SecretsService) EncryptWithDBSession(ctx context.Context, payload []byte, opt secrets.EncryptionOptions, sess *xorm.Session) ([]byte, error) {
 	// Use legacy encryption service if featuremgmt.FlagDisableEnvelopeEncryption toggle is on
 	if s.features.IsEnabled(featuremgmt.FlagDisableEnvelopeEncryption) {
 		return s.enc.Encrypt(ctx, payload, setting.SecretKey)
@@ -175,7 +177,7 @@ func (s *SecretsService) EncryptWithDBSession(ctx context.Context, payload []byt
 
 	var id string
 	var dataKey []byte
-	id, dataKey, err = s.currentDataKey(ctx, label, scope, sess)
+	id, dataKey, err = s.currentDataKey(ctx, label, scope)
 	if err != nil {
 		s.log.Error("Failed to get current data key", "error", err, "label", label)
 		return nil, err
@@ -203,7 +205,7 @@ func (s *SecretsService) EncryptWithDBSession(ctx context.Context, payload []byt
 // currentDataKey looks up for current data key in cache or database by name, and decrypts it.
 // If there's no current data key in cache nor in database it generates a new random data key,
 // and stores it into both the in-memory cache and database (encrypted by the encryption provider).
-func (s *SecretsService) currentDataKey(ctx context.Context, label string, scope string, sess *xorm.Session) (string, []byte, error) {
+func (s *SecretsService) currentDataKey(ctx context.Context, label string, scope string) (string, []byte, error) {
 	// We want only one request fetching current data key at time to
 	// avoid the creation of multiple ones in case there's no one existing.
 	s.mtx.Lock()
@@ -217,7 +219,7 @@ func (s *SecretsService) currentDataKey(ctx context.Context, label string, scope
 
 	// If no existing data key was found, create a new one
 	if dataKey == nil {
-		id, dataKey, err = s.newDataKey(ctx, label, scope, sess)
+		id, dataKey, err = s.newDataKey(ctx, label, scope)
 		if err != nil {
 			return "", nil, err
 		}
@@ -226,7 +228,7 @@ func (s *SecretsService) currentDataKey(ctx context.Context, label string, scope
 	return id, dataKey, nil
 }
 
-// dataKeyByLabel looks up for data key in cache.
+// dataKeyByLabel looks up for data key in cache by label.
 // Otherwise, it fetches it from database, decrypts it and caches it decrypted.
 func (s *SecretsService) dataKeyByLabel(ctx context.Context, label string) (string, []byte, error) {
 	// 0. Get data key from in-memory cache.
@@ -256,18 +258,13 @@ func (s *SecretsService) dataKeyByLabel(ctx context.Context, label string) (stri
 	}
 
 	// 3. Store the decrypted data key into the in-memory cache.
-	s.dataKeyCache.add(&dataKeyCacheEntry{
-		id:      dataKey.Id,
-		label:   dataKey.Label,
-		dataKey: decrypted,
-		active:  dataKey.Active,
-	})
+	s.cacheDataKey(dataKey, decrypted)
 
 	return dataKey.Id, decrypted, nil
 }
 
 // newDataKey creates a new random data key, encrypts it and stores it into the database and cache.
-func (s *SecretsService) newDataKey(ctx context.Context, label string, scope string, sess *xorm.Session) (string, []byte, error) {
+func (s *SecretsService) newDataKey(ctx context.Context, label string, scope string) (string, []byte, error) {
 	// 1. Create new data key.
 	dataKey, err := newRandomDataKey()
 	if err != nil {
@@ -288,6 +285,7 @@ func (s *SecretsService) newDataKey(ctx context.Context, label string, scope str
 
 	// 3. Store its encrypted value into the DB.
 	id := util.GenerateShortUID()
+
 	dbDataKey := secrets.DataKey{
 		Active:        true,
 		Id:            id,
@@ -297,24 +295,11 @@ func (s *SecretsService) newDataKey(ctx context.Context, label string, scope str
 		Scope:         scope,
 	}
 
-	if sess == nil {
-		err = s.store.CreateDataKey(ctx, &dbDataKey)
-	} else {
-		err = s.store.CreateDataKeyWithDBSession(ctx, &dbDataKey, sess)
-	}
-
+	err = s.store.CreateDataKey(ctx, &dbDataKey)
 	if err != nil {
 		return "", nil, err
 	}
 
-	// 4. Store the decrypted data key into the in-memory cache.
-	s.dataKeyCache.add(&dataKeyCacheEntry{
-		id:      id,
-		label:   label,
-		dataKey: dataKey,
-		active:  true,
-	})
-
 	return id, dataKey, nil
 }
 
@@ -388,13 +373,9 @@ func (s *SecretsService) Decrypt(ctx context.Context, payload []byte) ([]byte, e
 }
 
 func (s *SecretsService) EncryptJsonData(ctx context.Context, kv map[string]string, opt secrets.EncryptionOptions) (map[string][]byte, error) {
-	return s.EncryptJsonDataWithDBSession(ctx, kv, opt, nil)
-}
-
-func (s *SecretsService) EncryptJsonDataWithDBSession(ctx context.Context, kv map[string]string, opt secrets.EncryptionOptions, sess *xorm.Session) (map[string][]byte, error) {
 	encrypted := make(map[string][]byte)
 	for key, value := range kv {
-		encryptedData, err := s.EncryptWithDBSession(ctx, []byte(value), opt, sess)
+		encryptedData, err := s.Encrypt(ctx, []byte(value), opt)
 		if err != nil {
 			return nil, err
 		}
@@ -457,12 +438,7 @@ func (s *SecretsService) dataKeyById(ctx context.Context, id string) ([]byte, er
 	}
 
 	// 3. Store the decrypted data key into the in-memory cache.
-	s.dataKeyCache.add(&dataKeyCacheEntry{
-		id:      dataKey.Id,
-		label:   dataKey.Label,
-		dataKey: decrypted,
-		active:  dataKey.Active,
-	})
+	s.cacheDataKey(dataKey, decrypted)
 
 	return decrypted, nil
 }
@@ -547,3 +523,51 @@ func (s *SecretsService) Run(ctx context.Context) error {
 		}
 	}
 }
+
+// Caching a data key is tricky, because at SecretsService level we cannot guarantee
+// that a newly created data key has actually been persisted, depending on the different
+// use cases that rely on SecretsService encryption and different database engines that
+// we have support for, because the data key creation may have happened within a DB TX,
+// that may fail afterwards.
+//
+// Therefore, if we cache a data key that hasn't been persisted with success (and won't),
+// and later that one is used for a encryption operation (aside from the DB TX that created
+// it), we may end up with data encrypted by a non-persisted data key, which could end up
+// in (unrecoverable) data corruption.
+//
+// So, we cache the data key by id and/or by label, depending on the data key's lifetime,
+// assuming that a data key older than a "caution period" should have been persisted.
+//
+// Look at the comments inline for further details.
+// You can also take a look at the issue below for more context:
+// https://github.com/grafana/grafana-enterprise/issues/4252
+func (s *SecretsService) cacheDataKey(dataKey *secrets.DataKey, decrypted []byte) {
+	// First, we cache the data key by id, because cache "by id" is
+	// only used by decrypt operations, so no risk of corrupting data.
+	entry := &dataKeyCacheEntry{
+		id:      dataKey.Id,
+		label:   dataKey.Label,
+		dataKey: decrypted,
+		active:  dataKey.Active,
+	}
+
+	s.dataKeyCache.addById(entry)
+
+	// Then, we cache the data key by label, ONLY if data key's lifetime
+	// is longer than a certain "caution period", because cache "by label"
+	// is used (only) by encrypt operations, and we want to ensure that
+	// no data key is cached for encryption ops before being persisted.
+
+	const cautionPeriod = 10 * time.Minute
+	// We consider a "caution period" of 10m to be long enough for any database
+	// transaction that implied a data key creation to have finished successfully.
+	//
+	// Therefore, we consider that if we fetch a data key from the database,
+	// more than 10m later than its creation, it should have been actually
+	// persisted - i.e. the transaction that created it is no longer running.
+
+	nowMinusCautionPeriod := now().Add(-cautionPeriod)
+	if dataKey.Created.Before(nowMinusCautionPeriod) {
+		s.dataKeyCache.addByLabel(entry)
+	}
+}

pkg/services/secrets/manager/manager_test.go

@@ -2,6 +2,7 @@ package manager
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
@@ -17,12 +18,14 @@ import (
 	"github.com/grafana/grafana/pkg/services/kmsproviders/osskmsproviders"
 	"github.com/grafana/grafana/pkg/services/secrets"
 	"github.com/grafana/grafana/pkg/services/secrets/database"
+	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/util"
 )
 
 func TestSecretsService_EnvelopeEncryption(t *testing.T) {
-	store := database.ProvideSecretsStore(db.InitTestDB(t))
+	testDB := db.InitTestDB(t)
+	store := database.ProvideSecretsStore(testDB)
 	svc := SetupTestService(t, store)
 	ctx := context.Background()
 
@@ -82,7 +85,8 @@ func TestSecretsService_EnvelopeEncryption(t *testing.T) {
 }
 
 func TestSecretsService_DataKeys(t *testing.T) {
-	store := database.ProvideSecretsStore(db.InitTestDB(t))
+	testDB := db.InitTestDB(t)
+	store := database.ProvideSecretsStore(testDB)
 	ctx := context.Background()
 
 	dataKey := &secrets.DataKey{
@@ -160,7 +164,8 @@ func TestSecretsService_DataKeys(t *testing.T) {
 
 func TestSecretsService_UseCurrentProvider(t *testing.T) {
 	t.Run("When encryption_provider is not specified explicitly, should use 'secretKey' as a current provider", func(t *testing.T) {
-		svc := SetupTestService(t, database.ProvideSecretsStore(db.InitTestDB(t)))
+		testDB := db.InitTestDB(t)
+		svc := SetupTestService(t, database.ProvideSecretsStore(testDB))
 		assert.Equal(t, secrets.ProviderID("secretKey.v1"), svc.currentProviderID)
 	})
 
@@ -187,7 +192,8 @@ func TestSecretsService_UseCurrentProvider(t *testing.T) {
 
 		features := featuremgmt.WithFeatures()
 		kms := newFakeKMS(osskmsproviders.ProvideService(encryptionService, settings, features))
-		secretStore := database.ProvideSecretsStore(db.InitTestDB(t))
+		testDB := db.InitTestDB(t)
+		secretStore := database.ProvideSecretsStore(testDB)
 
 		secretsService, err := ProvideSecretsService(
 			secretStore,
@@ -261,8 +267,8 @@ func (f *fakeKMS) Provide() (map[secrets.ProviderID]secrets.Provider, error) {
 
 func TestSecretsService_Run(t *testing.T) {
 	ctx := context.Background()
-	sql := db.InitTestDB(t)
-	store := database.ProvideSecretsStore(sql)
+	testDB := db.InitTestDB(t)
+	store := database.ProvideSecretsStore(testDB)
 	svc := SetupTestService(t, store)
 
 	t.Run("should stop with no error once the context's finished", func(t *testing.T) {
@@ -274,16 +280,26 @@ func TestSecretsService_Run(t *testing.T) {
 	})
 
 	t.Run("should trigger cache clean up", func(t *testing.T) {
-		// Encrypt to ensure there's a data encryption key generated
-		_, err := svc.Encrypt(ctx, []byte("grafana"), secrets.WithoutScope())
+		restoreTimeNowAfterTestExec(t)
+
+		// Encrypt to force data encryption key generation
+		encrypted, err := svc.Encrypt(ctx, []byte("grafana"), secrets.WithoutScope())
+		require.NoError(t, err)
+
+		// Ten minutes later (after caution period)
+		// Look SecretsService.cacheDataKey for more details.
+		now = func() time.Time { return time.Now().Add(10 * time.Minute) }
+
+		// Decrypt to ensure data encryption key is cached
+		_, err = svc.Decrypt(ctx, encrypted)
 		require.NoError(t, err)
 
 		// Data encryption key cache should contain one element
 		require.Len(t, svc.dataKeyCache.byId, 1)
 		require.Len(t, svc.dataKeyCache.byLabel, 1)
 
-		t.Cleanup(func() { now = time.Now })
-		now = func() time.Time { return time.Now().Add(10 * time.Minute) }
+		// Twenty minutes later (after caution period + cache ttl)
+		now = func() time.Time { return time.Now().Add(20 * time.Minute) }
 
 		ctx, cancel := context.WithTimeout(ctx, 1*time.Second)
 		defer cancel()
@@ -301,8 +317,8 @@ func TestSecretsService_Run(t *testing.T) {
 
 func TestSecretsService_ReEncryptDataKeys(t *testing.T) {
 	ctx := context.Background()
-	sql := db.InitTestDB(t)
-	store := database.ProvideSecretsStore(sql)
+	testDB := db.InitTestDB(t)
+	store := database.ProvideSecretsStore(testDB)
 	svc := SetupTestService(t, store)
 
 	// Encrypt to generate data encryption key
@@ -326,6 +342,12 @@ func TestSecretsService_ReEncryptDataKeys(t *testing.T) {
 	})
 
 	t.Run("data keys cache should be invalidated", func(t *testing.T) {
+		restoreTimeNowAfterTestExec(t)
+
+		// Ten minutes later (after caution period)
+		// Look SecretsService.cacheDataKey for more details.
+		now = func() time.Time { return time.Now().Add(10 * time.Minute) }
+
 		// Decrypt to ensure data key is cached
 		_, err := svc.Decrypt(ctx, ciphertext)
 		require.NoError(t, err)
@@ -342,7 +364,8 @@ func TestSecretsService_ReEncryptDataKeys(t *testing.T) {
 
 func TestSecretsService_Decrypt(t *testing.T) {
 	ctx := context.Background()
-	store := database.ProvideSecretsStore(db.InitTestDB(t))
+	testDB := db.InitTestDB(t)
+	store := database.ProvideSecretsStore(testDB)
 
 	t.Run("empty payload should fail", func(t *testing.T) {
 		svc := SetupTestService(t, store)
@@ -401,3 +424,137 @@ func TestSecretsService_Decrypt(t *testing.T) {
 		assert.Equal(t, []byte("grafana"), decrypted)
 	})
 }
+
+func TestIntegration_SecretsService(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test")
+	}
+
+	ctx := context.Background()
+	someData := []byte(`some-data`)
+
+	tcs := map[string]func(*testing.T, *sqlstore.SQLStore, *SecretsService){
+		"regular": func(t *testing.T, _ *sqlstore.SQLStore, svc *SecretsService) {
+			// We encrypt some data normally, no transactions implied.
+			_, err := svc.Encrypt(ctx, someData, secrets.WithoutScope())
+			require.NoError(t, err)
+		},
+		"within successful InTransaction": func(t *testing.T, store *sqlstore.SQLStore, svc *SecretsService) {
+			require.NoError(t, store.InTransaction(ctx, func(ctx context.Context) error {
+				// We encrypt some data within a transaction that shares the db session.
+				_, err := svc.Encrypt(ctx, someData, secrets.WithoutScope())
+				require.NoError(t, err)
+
+				// And the transition succeeds.
+				return nil
+			}))
+		},
+		"within unsuccessful InTransaction": func(t *testing.T, store *sqlstore.SQLStore, svc *SecretsService) {
+			require.NotNil(t, store.InTransaction(ctx, func(ctx context.Context) error {
+				// We encrypt some data within a transaction that shares the db session.
+				_, err := svc.Encrypt(ctx, someData, secrets.WithoutScope())
+				require.NoError(t, err)
+
+				// But the transaction fails.
+				return errors.New("error")
+			}))
+		},
+		"within unsuccessful InTransaction (plus forced db fetch)": func(t *testing.T, store *sqlstore.SQLStore, svc *SecretsService) {
+			require.NotNil(t, store.InTransaction(ctx, func(ctx context.Context) error {
+				// We encrypt some data within a transaction that shares the db session.
+				encrypted, err := svc.Encrypt(ctx, someData, secrets.WithoutScope())
+				require.NoError(t, err)
+
+				// At this point the data key is not cached yet because
+				// the transaction haven't been committed yet,
+				// and won't, so we do a decrypt operation within the
+				// transaction to force the data key to be
+				// (potentially) cached (it shouldn't to prevent issues).
+				decrypted, err := svc.Decrypt(ctx, encrypted)
+				require.NoError(t, err)
+				assert.Equal(t, someData, decrypted)
+
+				// But the transaction fails.
+				return errors.New("error")
+			}))
+		},
+		"within successful WithTransactionalDbSession": func(t *testing.T, store *sqlstore.SQLStore, svc *SecretsService) {
+			require.NoError(t, store.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error {
+				// We encrypt some data within a transaction that does not share the db session.
+				_, err := svc.Encrypt(ctx, someData, secrets.WithoutScope())
+				require.NoError(t, err)
+
+				// And the transition succeeds.
+				return nil
+			}))
+		},
+		"within unsuccessful WithTransactionalDbSession": func(t *testing.T, store *sqlstore.SQLStore, svc *SecretsService) {
+			require.NotNil(t, store.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error {
+				// We encrypt some data within a transaction that does not share the db session.
+				_, err := svc.Encrypt(ctx, someData, secrets.WithoutScope())
+				require.NoError(t, err)
+
+				// But the transaction fails.
+				return errors.New("error")
+			}))
+		},
+		"within unsuccessful WithTransactionalDbSession (plus forced db fetch)": func(t *testing.T, store *sqlstore.SQLStore, svc *SecretsService) {
+			require.NotNil(t, store.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error {
+				// We encrypt some data within a transaction that does not share the db session.
+				encrypted, err := svc.Encrypt(ctx, someData, secrets.WithoutScope())
+				require.NoError(t, err)
+
+				// At this point the data key is not cached yet because
+				// the transaction haven't been committed yet,
+				// and won't, so we do a decrypt operation within the
+				// transaction to force the data key to be
+				// (potentially) cached (it shouldn't to prevent issues).
+				decrypted, err := svc.Decrypt(ctx, encrypted)
+				require.NoError(t, err)
+				assert.Equal(t, someData, decrypted)
+
+				// But the transaction fails.
+				return errors.New("error")
+			}))
+		},
+	}
+
+	for name, tc := range tcs {
+		t.Run(name, func(t *testing.T) {
+			testDB := db.InitTestDB(t)
+			svc := SetupTestService(t, database.ProvideSecretsStore(testDB))
+
+			// Here's what actually matters and varies on each test: look at the test case name.
+			//
+			// For historical reasons, and in an old implementation, when a successful encryption
+			// operation happened within an unsuccessful transaction, the data key was used to be
+			// cached in memory for the next encryption operations, which caused some data to be
+			// encrypted with a data key that haven't actually been persisted into the database.
+			tc(t, testDB, svc)
+			// Therefore, the data encrypted after this point, become unrecoverable after a restart.
+			// So, the different test cases here are there to prevent that from happening again
+			// in the future, whatever it is what happens.
+
+			// So, we proceed with an encryption operation:
+			toEncrypt := []byte(`data-to-encrypt`)
+			encrypted, err := svc.Encrypt(ctx, toEncrypt, secrets.WithoutScope())
+			require.NoError(t, err)
+
+			// We simulate an instance restart. So, there's no data in the in-memory cache.
+			svc.dataKeyCache.flush()
+
+			// And then, we MUST still be able to decrypt the previously encrypted data:
+			decrypted, err := svc.Decrypt(ctx, encrypted)
+			require.NoError(t, err)
+			assert.Equal(t, toEncrypt, decrypted)
+		})
+	}
+}
+
+// Use this function at the beginning of those tests
+// that manipulates 'now', so it'll leave it in a
+// correct state once test execution finishes.
+func restoreTimeNowAfterTestExec(t *testing.T) {
+	t.Helper()
+	t.Cleanup(func() { now = time.Now })
+}