From df409925eca0498b6713cd2f6cb5514a62c0c037 Mon Sep 17 00:00:00 2001 From: Atanas Janeshliev Date: Mon, 17 Mar 2025 12:57:47 +0100 Subject: [PATCH] fix(BRIDGE-335): store last sucessfully used keychain helper as user preference --- internal/app/migration.go | 2 +- internal/app/migration_test.go | 2 +- internal/app/vault.go | 37 +++++++++++++++++++++------------- internal/vault/helper.go | 4 ++++ pkg/keychain/keychain.go | 12 +++++------ pkg/keychain/keychain_test.go | 2 +- 6 files changed, 36 insertions(+), 23 deletions(-) diff --git a/internal/app/migration.go b/internal/app/migration.go index 43727372..51735cd9 100644 --- a/internal/app/migration.go +++ b/internal/app/migration.go @@ -138,7 +138,7 @@ func migrateOldAccounts(locations *locations.Locations, keychains *keychain.List if err != nil { return fmt.Errorf("failed to get helper: %w", err) } - keychain, err := keychain.NewKeychain(helper, "bridge", keychains.GetHelpers(), keychains.GetDefaultHelper()) + keychain, _, err := keychain.NewKeychain(helper, "bridge", keychains.GetHelpers(), keychains.GetDefaultHelper()) if err != nil { return fmt.Errorf("failed to create keychain: %w", err) } diff --git a/internal/app/migration_test.go b/internal/app/migration_test.go index 85d78c5c..7974412d 100644 --- a/internal/app/migration_test.go +++ b/internal/app/migration_test.go @@ -134,7 +134,7 @@ func TestKeychainMigration(t *testing.T) { func TestUserMigration(t *testing.T) { kcl := keychain.NewTestKeychainsList() - kc, err := keychain.NewKeychain("mock", "bridge", kcl.GetHelpers(), kcl.GetDefaultHelper()) + kc, _, err := keychain.NewKeychain("mock", "bridge", kcl.GetHelpers(), kcl.GetDefaultHelper()) require.NoError(t, err) require.NoError(t, kc.Put("brokenID", "broken")) diff --git a/internal/app/vault.go b/internal/app/vault.go index 5f69e953..3c478c8e 100644 --- a/internal/app/vault.go +++ b/internal/app/vault.go @@ -69,11 +69,12 @@ func newVault(reporter *sentry.Reporter, locations *locations.Locations, keychai logrus.WithField("vaultDir", vaultDir).Debug("Loading vault from directory") var ( - vaultKey []byte - insecure bool + vaultKey []byte + insecure bool + lastUsedHelper string ) - if key, err := loadVaultKey(vaultDir, keychains); err != nil { + if key, helper, err := loadVaultKey(vaultDir, keychains); err != nil { if reporter != nil { if rerr := reporter.ReportMessageWithContext("Could not load/create vault key", map[string]any{ "keychainDefaultHelper": keychains.GetDefaultHelper(), @@ -91,6 +92,7 @@ func newVault(reporter *sentry.Reporter, locations *locations.Locations, keychai vaultDir = path.Join(vaultDir, "insecure") } else { vaultKey = key + lastUsedHelper = helper logHashedVaultKey(vaultKey) // Log a hash of the vault key. } @@ -99,36 +101,43 @@ func newVault(reporter *sentry.Reporter, locations *locations.Locations, keychai return nil, false, nil, fmt.Errorf("could not provide gluon path: %w", err) } - vault, corrupt, err := vault.New(vaultDir, gluonCacheDir, vaultKey, panicHandler) + userVault, corrupt, err := vault.New(vaultDir, gluonCacheDir, vaultKey, panicHandler) if err != nil { return nil, false, corrupt, fmt.Errorf("could not create vault: %w", err) } - return vault, insecure, corrupt, nil + // Remember the last successfully used keychain and store that as the user preference. + if err := vault.SetHelper(vaultDir, lastUsedHelper); err != nil { + logrus.WithError(err).Error("Could not store last used keychain helper") + } + + return userVault, insecure, corrupt, nil } -func loadVaultKey(vaultDir string, keychains *keychain.List) ([]byte, error) { - helper, err := vault.GetHelper(vaultDir) +// loadVaultKey - loads the key used to encrypt the vault alongside the keychain helper used to access it. +func loadVaultKey(vaultDir string, keychains *keychain.List) (key []byte, keychainHelper string, err error) { + keychainHelper, err = vault.GetHelper(vaultDir) if err != nil { - return nil, fmt.Errorf("could not get keychain helper: %w", err) + return nil, keychainHelper, fmt.Errorf("could not get keychain helper: %w", err) } - kc, err := keychain.NewKeychain(helper, constants.KeyChainName, keychains.GetHelpers(), keychains.GetDefaultHelper()) + kc, keychainHelper, err := keychain.NewKeychain(keychainHelper, constants.KeyChainName, keychains.GetHelpers(), keychains.GetDefaultHelper()) if err != nil { - return nil, fmt.Errorf("could not create keychain: %w", err) + return nil, keychainHelper, fmt.Errorf("could not create keychain: %w", err) } - key, err := vault.GetVaultKey(kc) + key, err = vault.GetVaultKey(kc) if err != nil { if keychain.IsErrKeychainNoItem(err) { logrus.WithError(err).Warn("no vault key found, generating new") - return vault.NewVaultKey(kc) + key, err := vault.NewVaultKey(kc) + return key, keychainHelper, err } - return nil, fmt.Errorf("could not check for vault key: %w", err) + return nil, keychainHelper, fmt.Errorf("could not check for vault key: %w", err) } - return key, nil + return key, keychainHelper, nil } // logHashedVaultKey - computes a sha256 hash and encodes it to base 64. The resulting string is logged. diff --git a/internal/vault/helper.go b/internal/vault/helper.go index d4244d92..7c6eabea 100644 --- a/internal/vault/helper.go +++ b/internal/vault/helper.go @@ -63,6 +63,10 @@ func GetHelper(vaultDir string) (string, error) { } func SetHelper(vaultDir, helper string) error { + if helper == "" { + return nil + } + settings, err := LoadKeychainSettings(vaultDir) if err != nil { return err diff --git a/pkg/keychain/keychain.go b/pkg/keychain/keychain.go index 7f9dbdec..64029e05 100644 --- a/pkg/keychain/keychain.go +++ b/pkg/keychain/keychain.go @@ -82,11 +82,11 @@ func (kcl *List) GetDefaultHelper() string { return kcl.defaultHelper } -// NewKeychain creates a new native keychain. -func NewKeychain(preferred, keychainName string, helpers Helpers, defaultHelper string) (*Keychain, error) { +// NewKeychain creates a new native keychain. It also returns the keychain helper used to access the keychain. +func NewKeychain(preferred, keychainName string, helpers Helpers, defaultHelper string) (kc *Keychain, usedKeychainHelper string, err error) { // There must be at least one keychain helper available. if len(helpers) < 1 { - return nil, ErrNoKeychain + return nil, "", ErrNoKeychain } // If the preferred keychain is unsupported, fallback to the default one. @@ -97,16 +97,16 @@ func NewKeychain(preferred, keychainName string, helpers Helpers, defaultHelper // Load the user's preferred keychain helper. helperConstructor, ok := helpers[preferred] if !ok { - return nil, ErrNoKeychain + return nil, "", ErrNoKeychain } // Construct the keychain helper. helper, err := helperConstructor(hostURL(keychainName)) if err != nil { - return nil, err + return nil, preferred, err } - return newKeychain(helper, hostURL(keychainName)), nil + return newKeychain(helper, hostURL(keychainName)), preferred, nil } func newKeychain(helper credentials.Helper, url string) *Keychain { diff --git a/pkg/keychain/keychain_test.go b/pkg/keychain/keychain_test.go index 019ede2d..05f66a83 100644 --- a/pkg/keychain/keychain_test.go +++ b/pkg/keychain/keychain_test.go @@ -120,7 +120,7 @@ func TestIsErrKeychainNoItem(t *testing.T) { helpers := NewList().GetHelpers() for helperName := range helpers { - kc, err := NewKeychain(helperName, "bridge-test", helpers, helperName) + kc, _, err := NewKeychain(helperName, "bridge-test", helpers, helperName) r.NoError(err) _, _, err = kc.Get("non-existing")