From 419c7acb11cdd298bf2dec7c86ebeea3f139e796 Mon Sep 17 00:00:00 2001
From: Jarek Kowalski <jaak@jkowalski.net>
Date: Mon, 8 Aug 2022 21:45:08 -0700
Subject: [PATCH] fix(repository): fixed V1 key derivation bug from previous
 refactoring (#2286)

See https://github.com/kopia/kopia/commit/23299c34512e640d50e1ddff888758a119a09cb8
---
 internal/repotesting/repotesting.go      |  5 +-
 repo/maintenance/maintenance_schedule.go |  7 +--
 repo/repository.go                       | 15 ++---
 repo/repository_test.go                  | 77 ++++++++++++++++++++++++
 4 files changed, 86 insertions(+), 18 deletions(-)

diff --git a/internal/repotesting/repotesting.go b/internal/repotesting/repotesting.go
index 5c48899d1..2b99daeca 100644
--- a/internal/repotesting/repotesting.go
+++ b/internal/repotesting/repotesting.go
@@ -20,7 +20,8 @@
 	"github.com/kopia/kopia/snapshot"
 )
 
-const defaultPassword = "foobarbazfoobarbaz"
+// DefaultPasswordForTesting is the default password to use for all testing repositories.
+const DefaultPasswordForTesting = "foobarbazfoobarbaz"
 
 // Environment encapsulates details of a test environment.
 type Environment struct {
@@ -91,7 +92,7 @@ func (e *Environment) setup(tb testing.TB, version format.Version, opts ...Optio
 	e.st = st
 
 	if e.Password == "" {
-		e.Password = defaultPassword
+		e.Password = DefaultPasswordForTesting
 	}
 
 	if err := repo.Initialize(ctx, st, opt, e.Password); err != nil {
diff --git a/repo/maintenance/maintenance_schedule.go b/repo/maintenance/maintenance_schedule.go
index 46f0a9ec0..2e472e34a 100644
--- a/repo/maintenance/maintenance_schedule.go
+++ b/repo/maintenance/maintenance_schedule.go
@@ -62,12 +62,7 @@ func (s *Schedule) ReportRun(taskType TaskType, info RunInfo) {
 }
 
 func getAES256GCM(rep repo.DirectRepository) (cipher.AEAD, error) {
-	key, err := rep.DeriveKey(maintenanceScheduleKeyPurpose, maintenanceScheduleKeySize)
-	if err != nil {
-		return nil, errors.Wrap(err, "derive key")
-	}
-
-	c, err := aes.NewCipher(key)
+	c, err := aes.NewCipher(rep.DeriveKey(maintenanceScheduleKeyPurpose, maintenanceScheduleKeySize))
 	if err != nil {
 		return nil, errors.Wrap(err, "unable to create AES-256 cipher")
 	}
diff --git a/repo/repository.go b/repo/repository.go
index c5df08248..2513e10f6 100644
--- a/repo/repository.go
+++ b/repo/repository.go
@@ -63,7 +63,7 @@ type DirectRepository interface {
 	AlsoLogToContentLog(ctx context.Context) context.Context
 	UniqueID() []byte
 	ConfigFilename() string
-	DeriveKey(purpose []byte, keyLength int) ([]byte, error)
+	DeriveKey(purpose []byte, keyLength int) []byte
 	Token(password string) (string, error)
 	Throttler() throttling.SettableThrottler
 	RequiredFeatures() ([]feature.Required, error)
@@ -111,20 +111,15 @@ type directRepository struct {
 }
 
 // DeriveKey derives encryption key of the provided length from the master key.
-func (r *directRepository) DeriveKey(purpose []byte, keyLength int) ([]byte, error) {
-	mp, mperr := r.cmgr.ContentFormat().GetMutableParameters()
-	if mperr != nil {
-		return nil, errors.Wrap(mperr, "mutable parameters")
-	}
-
-	if mp.Version >= format.FormatVersion2 {
-		return format.DeriveKeyFromMasterKey(r.cmgr.ContentFormat().GetMasterKey(), r.uniqueID, purpose, keyLength), nil
+func (r *directRepository) DeriveKey(purpose []byte, keyLength int) []byte {
+	if r.cmgr.ContentFormat().SupportsPasswordChange() {
+		return format.DeriveKeyFromMasterKey(r.cmgr.ContentFormat().GetMasterKey(), r.uniqueID, purpose, keyLength)
 	}
 
 	// version of kopia <v0.9 had a bug where certain keys were derived directly from
 	// the password and not from the random master key. This made it impossible to change
 	// password.
-	return format.DeriveKeyFromMasterKey(r.formatEncryptionKey, r.uniqueID, purpose, keyLength), nil
+	return format.DeriveKeyFromMasterKey(r.formatEncryptionKey, r.uniqueID, purpose, keyLength)
 }
 
 // ClientOptions returns client options.
diff --git a/repo/repository_test.go b/repo/repository_test.go
index 7183cf2d0..4ea8e02d3 100644
--- a/repo/repository_test.go
+++ b/repo/repository_test.go
@@ -647,6 +647,83 @@ func (s *formatSpecificTestSuite) TestChangePassword(t *testing.T) {
 	}
 }
 
+func TestDeriveKey(t *testing.T) {
+	testPurpose := []byte{0, 0, 0, 0}
+	testKeyLength := 8
+	masterKey := []byte("01234567890123456789012345678901")
+	uniqueID := []byte("a5ba5d2da4b14b518b9501b64b5d87ca")
+
+	j := format.KopiaRepositoryJSON{
+		UniqueID:               uniqueID,
+		KeyDerivationAlgorithm: format.DefaultKeyDerivationAlgorithm,
+	}
+
+	formatEncryptionKeyFromPassword, err := j.DeriveFormatEncryptionKeyFromPassword(repotesting.DefaultPasswordForTesting)
+	require.NoError(t, err)
+
+	validV1KeyDerivedFromPassword := format.DeriveKeyFromMasterKey(formatEncryptionKeyFromPassword, uniqueID, testPurpose, testKeyLength)
+	validV2KeyDerivedFromMasterKey := format.DeriveKeyFromMasterKey(masterKey, uniqueID, testPurpose, testKeyLength)
+
+	setup := func(v format.Version) repo.DirectRepositoryWriter {
+		_, env := repotesting.NewEnvironment(t, v, repotesting.Options{
+			NewRepositoryOptions: func(nro *repo.NewRepositoryOptions) {
+				nro.BlockFormat.MasterKey = masterKey
+				nro.UniqueID = uniqueID
+			},
+		})
+
+		return env.RepositoryWriter
+	}
+
+	setupUpgraded := func(v1, v2 format.Version) repo.DirectRepositoryWriter {
+		ctx, env := repotesting.NewEnvironment(t, v1, repotesting.Options{
+			NewRepositoryOptions: func(nro *repo.NewRepositoryOptions) {
+				// do not set nro.BlockFormat.MasterKey
+				nro.UniqueID = uniqueID
+			},
+		})
+
+		// prepare upgrade
+		dw1Upgraded := env.Repository.(repo.DirectRepositoryWriter)
+		cf := dw1Upgraded.ContentReader().ContentFormat()
+
+		mp, mperr := cf.GetMutableParameters()
+		require.NoError(t, mperr)
+
+		feat, err := dw1Upgraded.RequiredFeatures()
+		require.NoError(t, err)
+
+		// perform upgrade
+		mp.Version = v2
+
+		require.NoError(t, dw1Upgraded.SetParameters(ctx, mp, dw1Upgraded.BlobCfg(), feat))
+
+		return env.MustConnectOpenAnother(t).(repo.DirectRepositoryWriter)
+	}
+
+	// we verify that repositories started on V1 will continue to derive keys from
+	// password (which can't be changed) and not from the master key.
+	cases := []struct {
+		desc       string
+		dw         repo.DirectRepositoryWriter
+		wantFormat format.Version
+		wantKey    []byte
+	}{
+		{"v1", setup(format.FormatVersion1), format.FormatVersion1, validV1KeyDerivedFromPassword},
+		{"v1-v2", setupUpgraded(format.FormatVersion1, format.FormatVersion2), format.FormatVersion2, validV1KeyDerivedFromPassword},
+		{"v1-v3", setupUpgraded(format.FormatVersion1, format.FormatVersion3), format.FormatVersion3, validV1KeyDerivedFromPassword},
+		{"v2", setup(format.FormatVersion2), format.FormatVersion2, validV2KeyDerivedFromMasterKey},
+		{"v3", setup(format.FormatVersion3), format.FormatVersion3, validV2KeyDerivedFromMasterKey},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.desc, func(t *testing.T) {
+			require.Equal(t, tc.wantFormat, tc.dw.ContentReader().ContentFormat().Struct().Version)
+			require.Equal(t, tc.wantKey, tc.dw.DeriveKey(testPurpose, testKeyLength))
+		})
+	}
+}
+
 func verifyNotFound(ctx context.Context, t *testing.T, rep repo.Repository, objectID object.ID, testCaseID string) {
 	t.Helper()