From 1b22c32ef998f17b509eb0a1edbb3611a87536c9 Mon Sep 17 00:00:00 2001 From: Leander Beernaert Date: Thu, 30 Nov 2023 08:31:14 +0100 Subject: [PATCH] fix(GODT-3102): Distinguish Vault Decryption from Serialization Errors Rather than returning whether the vault was corrupt or not return the error which caused the vault to be considered as corrupt. --- internal/app/migration_test.go | 6 +++--- internal/app/vault.go | 16 ++++++++++------ internal/user/user_test.go | 2 +- internal/vault/migrate_test.go | 2 +- internal/vault/settings_test.go | 2 +- internal/vault/types_file.go | 11 ++++++++--- internal/vault/vault.go | 28 ++++++++++++++++------------ internal/vault/vault_bench_test.go | 2 +- internal/vault/vault_test.go | 14 +++++++------- tests/ctx_bridge_test.go | 4 ++-- 10 files changed, 50 insertions(+), 37 deletions(-) diff --git a/internal/app/migration_test.go b/internal/app/migration_test.go index 17284c08..d6afb95a 100644 --- a/internal/app/migration_test.go +++ b/internal/app/migration_test.go @@ -42,7 +42,7 @@ func TestMigratePrefsToVaultWithKeys(t *testing.T) { // Create a new vault. vault, corrupt, err := vault.New(t.TempDir(), t.TempDir(), []byte("my secret key"), async.NoopPanicHandler{}) require.NoError(t, err) - require.False(t, corrupt) + require.NoError(t, corrupt) // load the old prefs file. configDir := filepath.Join("testdata", "with_keys") @@ -63,7 +63,7 @@ func TestMigratePrefsToVaultWithoutKeys(t *testing.T) { // Create a new vault. vault, corrupt, err := vault.New(t.TempDir(), t.TempDir(), []byte("my secret key"), async.NoopPanicHandler{}) require.NoError(t, err) - require.False(t, corrupt) + require.NoError(t, corrupt) // load the old prefs file. configDir := filepath.Join("testdata", "without_keys") @@ -173,7 +173,7 @@ func TestUserMigration(t *testing.T) { v, corrupt, err := vault.New(settingsFolder, settingsFolder, token, async.NoopPanicHandler{}) require.NoError(t, err) - require.False(t, corrupt) + require.NoError(t, corrupt) require.NoError(t, migrateOldAccounts(locations, kcl, v)) require.Equal(t, []string{wantCredentials.UserID}, v.GetUserIDs()) diff --git a/internal/app/vault.go b/internal/app/vault.go index 2e6d3aeb..aa93237b 100644 --- a/internal/app/vault.go +++ b/internal/app/vault.go @@ -42,21 +42,25 @@ func WithVault(locations *locations.Locations, keychains *keychain.List, panicHa logrus.WithFields(logrus.Fields{ "insecure": insecure, - "corrupt": corrupt, + "corrupt": corrupt != nil, }).Debug("Vault created") + if corrupt != nil { + logrus.WithError(corrupt).Warn("Failed to load existing vault, vault has been reset") + } + cert, _ := encVault.GetBridgeTLSCert() certs.NewInstaller().LogCertInstallStatus(cert) // GODT-1950: Add teardown actions (e.g. to close the vault). - return fn(encVault, insecure, corrupt) + return fn(encVault, insecure, corrupt != nil) } -func newVault(locations *locations.Locations, keychains *keychain.List, panicHandler async.PanicHandler) (*vault.Vault, bool, bool, error) { +func newVault(locations *locations.Locations, keychains *keychain.List, panicHandler async.PanicHandler) (*vault.Vault, bool, error, error) { vaultDir, err := locations.ProvideSettingsPath() if err != nil { - return nil, false, false, fmt.Errorf("could not get vault dir: %w", err) + return nil, false, nil, fmt.Errorf("could not get vault dir: %w", err) } logrus.WithField("vaultDir", vaultDir).Debug("Loading vault from directory") @@ -78,12 +82,12 @@ func newVault(locations *locations.Locations, keychains *keychain.List, panicHan gluonCacheDir, err := locations.ProvideGluonCachePath() if err != nil { - return nil, false, false, fmt.Errorf("could not provide gluon path: %w", err) + return nil, false, nil, fmt.Errorf("could not provide gluon path: %w", err) } vault, corrupt, err := vault.New(vaultDir, gluonCacheDir, vaultKey, panicHandler) if err != nil { - return nil, false, false, fmt.Errorf("could not create vault: %w", err) + return nil, false, corrupt, fmt.Errorf("could not create vault: %w", err) } return vault, insecure, corrupt, nil diff --git a/internal/user/user_test.go b/internal/user/user_test.go index cb7dc03b..c3fe490f 100644 --- a/internal/user/user_test.go +++ b/internal/user/user_test.go @@ -133,7 +133,7 @@ func withUser(tb testing.TB, ctx context.Context, _ *server.Server, m *proton.Ma v, corrupt, err := vault.New(tb.TempDir(), tb.TempDir(), []byte("my secret key"), nil) require.NoError(tb, err) - require.False(tb, corrupt) + require.NoError(tb, corrupt) vaultUser, err := v.AddUser(apiUser.ID, username, username+"@pm.me", apiAuth.UID, apiAuth.RefreshToken, saltedKeyPass) require.NoError(tb, err) diff --git a/internal/vault/migrate_test.go b/internal/vault/migrate_test.go index c69fe000..2cd8fbe9 100644 --- a/internal/vault/migrate_test.go +++ b/internal/vault/migrate_test.go @@ -55,7 +55,7 @@ func TestMigrate(t *testing.T) { // Migrate the vault. s, corrupt, err := New(dir, "default-gluon-dir", []byte("my secret key"), async.NoopPanicHandler{}) require.NoError(t, err) - require.False(t, corrupt) + require.NoError(t, corrupt) // Check the migrated vault. require.Equal(t, "v2.3.x-gluon-dir", s.GetGluonCacheDir()) diff --git a/internal/vault/settings_test.go b/internal/vault/settings_test.go index c48e788b..6a319987 100644 --- a/internal/vault/settings_test.go +++ b/internal/vault/settings_test.go @@ -68,7 +68,7 @@ func TestVault_Settings_GluonDir(t *testing.T) { // create a new test vault. s, corrupt, err := vault.New(t.TempDir(), "/path/to/gluon", []byte("my secret key"), async.NoopPanicHandler{}) require.NoError(t, err) - require.False(t, corrupt) + require.NoError(t, corrupt) // Check the default gluon dir. require.Equal(t, "/path/to/gluon", s.GetGluonCacheDir()) diff --git a/internal/vault/types_file.go b/internal/vault/types_file.go index 99da52e8..bd8febb7 100644 --- a/internal/vault/types_file.go +++ b/internal/vault/types_file.go @@ -19,6 +19,7 @@ package vault import ( "crypto/cipher" + "fmt" "github.com/ProtonMail/gopenpgp/v2/crypto" "github.com/vmihailenco/msgpack/v5" @@ -34,12 +35,12 @@ func unmarshalFile[T any](gcm cipher.AEAD, b []byte, data *T) error { var f File if err := msgpack.Unmarshal(b, &f); err != nil { - return err + return fmt.Errorf("%w: %v", ErrUnmarshal, err) } dec, err := gcm.Open(nil, f.Data[:gcm.NonceSize()], f.Data[gcm.NonceSize():], nil) if err != nil { - return err + return fmt.Errorf("%w: %v", ErrDecryptFailed, err) } for v := f.Version; v < Current; v++ { @@ -48,7 +49,11 @@ func unmarshalFile[T any](gcm cipher.AEAD, b []byte, data *T) error { } } - return msgpack.Unmarshal(dec, data) + if err := msgpack.Unmarshal(dec, data); err != nil { + return fmt.Errorf("%w: %v", ErrUnmarshal, err) + } + + return nil } func marshalFile[T any](gcm cipher.AEAD, t T) ([]byte, error) { diff --git a/internal/vault/vault.go b/internal/vault/vault.go index a92da785..55bbd2ca 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -49,27 +49,31 @@ type Vault struct { panicHandler async.PanicHandler } +var ErrDecryptFailed = errors.New("failed to decrypt vault") +var ErrUnmarshal = errors.New("vault contents are corrupt") + // New constructs a new encrypted data vault at the given filepath using the given encryption key. -func New(vaultDir, gluonCacheDir string, key []byte, panicHandler async.PanicHandler) (*Vault, bool, error) { +// The first error is a corruption error for an existing vault, the second errors refrain to all other errors. +func New(vaultDir, gluonCacheDir string, key []byte, panicHandler async.PanicHandler) (*Vault, error, error) { if err := os.MkdirAll(vaultDir, 0o700); err != nil { - return nil, false, err + return nil, nil, err } hash256 := sha256.Sum256(key) aes, err := aes.NewCipher(hash256[:]) if err != nil { - return nil, false, err + return nil, nil, err } gcm, err := cipher.NewGCM(aes) if err != nil { - return nil, false, err + return nil, nil, err } vault, corrupt, err := newVault(filepath.Join(vaultDir, "vault.enc"), gluonCacheDir, gcm) if err != nil { - return nil, false, err + return nil, corrupt, err } vault.panicHandler = panicHandler @@ -341,28 +345,28 @@ func (vault *Vault) detachUser(userID string) error { return nil } -func newVault(path, gluonDir string, gcm cipher.AEAD) (*Vault, bool, error) { +func newVault(path, gluonDir string, gcm cipher.AEAD) (*Vault, error, error) { if _, err := os.Stat(path); errors.Is(err, fs.ErrNotExist) { if _, err := initVault(path, gluonDir, gcm); err != nil { - return nil, false, err + return nil, nil, err } } enc, err := os.ReadFile(filepath.Clean(path)) if err != nil { - return nil, false, err + return nil, nil, err } - var corrupt bool + var corrupt error if err := unmarshalFile(gcm, enc, new(Data)); err != nil { - corrupt = true + corrupt = err } - if corrupt { + if corrupt != nil { newEnc, err := initVault(path, gluonDir, gcm) if err != nil { - return nil, false, err + return nil, corrupt, err } enc = newEnc diff --git a/internal/vault/vault_bench_test.go b/internal/vault/vault_bench_test.go index d6e96f2c..6bde9633 100644 --- a/internal/vault/vault_bench_test.go +++ b/internal/vault/vault_bench_test.go @@ -34,7 +34,7 @@ func BenchmarkVault(b *testing.B) { // Create a new vault. s, corrupt, err := vault.New(vaultDir, gluonDir, []byte("my secret key"), async.NoopPanicHandler{}) require.NoError(b, err) - require.False(b, corrupt) + require.NoError(b, corrupt) // Add 10kB of cookies to the vault. require.NoError(b, s.SetCookies(bytes.Repeat([]byte("a"), 10_000))) diff --git a/internal/vault/vault_test.go b/internal/vault/vault_test.go index 222e4a79..cba4e6ea 100644 --- a/internal/vault/vault_test.go +++ b/internal/vault/vault_test.go @@ -34,19 +34,19 @@ func TestVault_Corrupt(t *testing.T) { { _, corrupt, err := vault.New(vaultDir, gluonDir, []byte("my secret key"), async.NoopPanicHandler{}) require.NoError(t, err) - require.False(t, corrupt) + require.NoError(t, corrupt) } { _, corrupt, err := vault.New(vaultDir, gluonDir, []byte("my secret key"), async.NoopPanicHandler{}) require.NoError(t, err) - require.False(t, corrupt) + require.NoError(t, corrupt) } { _, corrupt, err := vault.New(vaultDir, gluonDir, []byte("bad key"), async.NoopPanicHandler{}) require.NoError(t, err) - require.True(t, corrupt) + require.ErrorIs(t, corrupt, vault.ErrDecryptFailed) } } @@ -56,13 +56,13 @@ func TestVault_Corrupt_JunkData(t *testing.T) { { _, corrupt, err := vault.New(vaultDir, gluonDir, []byte("my secret key"), async.NoopPanicHandler{}) require.NoError(t, err) - require.False(t, corrupt) + require.NoError(t, corrupt) } { _, corrupt, err := vault.New(vaultDir, gluonDir, []byte("my secret key"), async.NoopPanicHandler{}) require.NoError(t, err) - require.False(t, corrupt) + require.NoError(t, corrupt) } { @@ -75,7 +75,7 @@ func TestVault_Corrupt_JunkData(t *testing.T) { _, corrupt, err := vault.New(vaultDir, gluonDir, []byte("my secret key"), async.NoopPanicHandler{}) require.NoError(t, err) - require.True(t, corrupt) + require.ErrorIs(t, corrupt, vault.ErrUnmarshal) } } @@ -103,7 +103,7 @@ func newVault(t *testing.T) *vault.Vault { s, corrupt, err := vault.New(t.TempDir(), t.TempDir(), []byte("my secret key"), async.NoopPanicHandler{}) require.NoError(t, err) - require.False(t, corrupt) + require.NoError(t, corrupt) return s } diff --git a/tests/ctx_bridge_test.go b/tests/ctx_bridge_test.go index b344f446..7e848f6d 100644 --- a/tests/ctx_bridge_test.go +++ b/tests/ctx_bridge_test.go @@ -112,8 +112,8 @@ func (t *testCtx) initBridge() (<-chan events.Event, error) { vault, corrupt, err := vault.New(vaultDir, gluonCacheDir, t.storeKey, async.NoopPanicHandler{}) if err != nil { return nil, fmt.Errorf("could not create vault: %w", err) - } else if corrupt { - return nil, fmt.Errorf("vault is corrupt") + } else if corrupt != nil { + return nil, fmt.Errorf("vault is corrupt: %w", corrupt) } t.vault = vault