Use Argon2 for encrypted vaults #3502

jagodarybacka · 2023-06-26T14:54:27Z

Resolves #3470

What

Let's use Argon2 instead of PBKDF2 🔑

What was already done:

added required packages
configured webpack config to work with webAssembly following example from the docs
add necessary config to content_security_policy to allow webAssembly - without 'wasm-eval' we are not able to use argon implementation in the extension
migration from old vaults to new vaults
handling errors during migration and allowing users to continue with old implementation if needed
added one time analytics event after successful migration
updated Jest to allow unit tests to work with WebAssembly, updated Typescript as this was necessary to make new version of Jest work correctly and fixed a bunch of Typescript issues - most fixes are copied from Versions that go bump in the night: Bump versions across a few different core dependencies #3415

Testing

test migration from main - install extension on main, add some HD wallets, checkout this branch, reload and unlock the wallet, make sure you don't see the error about failed migration in the background console, check if analytics event has been emitted, lock and unlock more than one time
install extension on this branch, test locking and unlocking the wallet
try to make webassembly part break (throw Error here) and make sure user is able to continue with old vaults

Latest build: extension-builds-3502 (as of Sun, 02 Jul 2023 21:28:25 GMT).

- add required packages - configure webpack to work with webAssembly - add neccessary config to `content_security_policy` to allow webAssembly

background/services/internal-signer/encryption.ts

manifest/manifest.development.json

Let's set length to 32 bytes to match length expected by AES-GCM

Catch erros if vaults migration to Argon2 fails and allow to continue with old vaults encrypted with PBKDF2. Log analytics event when vaults are succesfully migrated.

jagodarybacka · 2023-06-27T14:48:50Z

For fixing tests we need to update Jest to the newest version as they apparently added support for webassembly some time ago. There is a bunch of errors after I've upgraded jest version unfortunately 🙈

Shadowfiend · 2023-06-27T15:07:04Z

I went through a lot of the jest upgrade pain in #3415 . 8ea287b is a (not so clean) commit with those changes. There's Ledger stuff in there as well, but might be straightforward to pick it apart.

hyphenized · 2023-06-28T05:29:47Z

background/services/internal-signer/index.ts

@@ -265,21 +273,29 @@ export default class InternalSignerService extends BaseService<Events> {
      return true
    }

+    const { vaults, version } = await migrateVaultsToArgon(password)


Might be nitpicking here but I think we could improve our approach if we kept the concern for migrating vaults separate from retrieving them. So, rather than returning vaults here, I propose we just return a boolean indicating success or failure and continue using getEncryptedVaults.

This should be true internally in the encryption layer, but for the caller it's the opposite--encryption layer should be completely in charge of making sure that the data is in the latest format, just like the db objects migrate before returning data.

Vault version is basically a constant across a given run of the extension. All retrievals of encrypted data should automatically reencrypt to the latest version. All encryptions should use the latest version. That fact should be transparent to almost everyone. Migrations should not be even a little bit optional (as skipping a function call might make them). This is how we ensure calling code doesn't make weird decisions about not wanting to upgrade, or weird mistakes about sometimes not doing so, etc: we give no one a choice or a hook.

To put this in terms of separation of concerns: the encryption layer is concerned with encrypting and decrypting data, and ensuring to the extent possible that all encrypted data is in the latest format.

Thought about this some more. The gap between this and how the internal signer was already implemented is not huge but it's gnarly. I don't think we should deal with it right now, unfortunately.

The way this would manifest is that we would have getEncryptedVaults transparently handle vault migration, and writeLatestEncryptedVault would be redone as a writeLatestVault and would transparently handle encryption and fallbacks.

Again, I think we can push this to later.

Shadowfiend

Left a few comments. Pondering how we can push more of the migration out of the service core and into the encryption layer... Right now the two are very entwined (really an old decision, nothing to do with the Argon2 stuff other than that it adds complexity) and it's leading to a gnarly unlock method that's starting to coordinate too much.

Not sure if it's worth refactoring further right now or not, will try to give more thoughts later today.

Shadowfiend · 2023-06-28T11:31:50Z

background/services/internal-signer/index.ts

@@ -121,6 +126,7 @@ interface Events extends ServiceLifecycleEvents {
  // TODO message was signed
  signedTx: SignedTransaction
  signedData: string
+  migratedToArgon2: never


Argon2 is an internal storage detail--it should not escape the service IMO. If we ever want to do a migration that isn't transparent to the user (eg because we want to encourage users to upgrade), we can revisit, but right now we should treat it as an implementation detail.

Shadowfiend · 2023-06-28T11:38:45Z

background/services/internal-signer/index.ts

@@ -265,21 +273,29 @@ export default class InternalSignerService extends BaseService<Events> {
      return true
    }

+    const { vaults, version } = await migrateVaultsToArgon(password)


This should be true internally in the encryption layer, but for the caller it's the opposite--encryption layer should be completely in charge of making sure that the data is in the latest format, just like the db objects migrate before returning data.

Vault version is basically a constant across a given run of the extension. All retrievals of encrypted data should automatically reencrypt to the latest version. All encryptions should use the latest version. That fact should be transparent to almost everyone. Migrations should not be even a little bit optional (as skipping a function call might make them). This is how we ensure calling code doesn't make weird decisions about not wanting to upgrade, or weird mistakes about sometimes not doing so, etc: we give no one a choice or a hook.

To put this in terms of separation of concerns: the encryption layer is concerned with encrypting and decrypting data, and ensuring to the extent possible that all encrypted data is in the latest format.

Shadowfiend · 2023-06-28T11:41:01Z

background/main.ts

@@ -1151,6 +1151,12 @@ export default class Main extends BaseService<never> {
      }
    })

+    this.internalSignerService.emitter.on("migratedToArgon2", async () => {
+      this.analyticsService.sendOneTimeAnalyticsEvent(


This is one of the few cases where we should just call this directly on the service IMO. Piping analytics through events makes sense when we're observing an existing action, but less so when we're wanting to track an action that is internal to a service.

Shadowfiend · 2023-06-28T11:46:36Z

background/services/internal-signer/index.ts

@@ -149,6 +155,8 @@ const isKeyring = (
 export default class InternalSignerService extends BaseService<Events> {
  #cachedKey: SaltedKey | null = null

+  #cachedVaultVersion: VaultVersion = VaultVersion.PBKDF2


This should never change, right? All vaults should be argon2 by the time the internal signer service sees them.

well initially for our existing users they are VaultVersion.PBKDF2, once they are migrated successfully to Argon then yeah, this should never change again but if the migration fails then I want to be able to use old vaults so these users are not stuck with locked keys - that's why I wanted to keep that info here

Ah, I see the thinking here. Here's how I think we handle that possibility:

Every time we unlock, we should try to migrate. The migration should check that once an Argon2 vault is encrypted, it can also be decrypted and produce the same data as the PBKDF2 vault had, before writing the migration to localStorage.

If the migration fails, we should log an analytics event. The unlocking process should continue normally (which will use the vault's current version, PBKDF2 if there was an error with the Argon2 encryption).

The migration process should tell us if it succeeded, i.e. we should be able to know “the internal signer data is all on the latest version of encryption”—this is just a boolean.

When we save the vault, we should use the strongest encryption we have (i.e., Argon2). If that fails, we should log an analytics event. At this point, if all vaults are on the latest version of encryption, we should fail the same way we would have failed if PBKDF2 blew up. If all vaults are not on the latest version, we should fall back to PBKDF2. In practice, this means we never want to end up with a newer vault on an older version of encryption than we have used until now—we should treat that as a full system failure.

After a set amount of time (maybe 6 months?) without errors in analytics, we should consider dropping the PBKDF2 fallbacks for encryption (but never for the migration for decryption).

This is more complex, but it never winds back security, and it doesn't leak encryption implementation details (vault version) outside of the encryption code.

Thoughts?

Sounds good, let me try to implement it and see how it will go.

Implemented:

on every unlock we try to migrate

test if the vault encrypted with Argon produces the same data as before the migration

log analytics event on migration fail

migration process should return a boolean if it was successful

What is not clear to me:

When we save the vault, we should use the strongest encryption we have (i.e., Argon2)....

So this point - if I understand correctly what you want to achieve here - is something I would rather avoid putting into code, because it should never be the case where we are writing new vault encrypted with Argon while old vaults are on the PBKDF2.
That is because to save new vault we have to have the service unlocked and we are trying to migrate during unlocking. If the migration fails then most likely writing a new vault with Argon will fail as well. I would probably go with saving the current encryption algorithm in the service as this is implemented right now and then we can use it to add new vaults without adding more complexity 🤔

A couple of notes on the reasoning, but ultimately I think we're good here ->

it should never be the case where we are writing new vault encrypted with Argon while old vaults are on the PBKDF2.

I agree this isn't ideal, but it is strictly better than having all vaults on PKDF2. The goal of having version alongside the vault is that we can have mixed versions if absolutely necessary.

If the migration fails then most likely writing a new vault with Argon will fail as well.

I don't know why argon2 would fail, so there's nothing to indicate to me whether such a failure would be transient or not. It could fail during migration due to issues loading wasm, but it could also run into transient resource limitations--again, it's unclear to me.

All that said: you are right! The piece I forgot about here is that we (correctly) don't keep the password around after the initial unlock. This means if the initial unlock fails to use argon2 to derive the key, and we want to try to encrypt with an argon2-derived key later… We would have to keep the password around/cached instead of just the key. A definite no! So let's put this one to rest.

- allowed Jest to fetch WebAssembly files - moved `crypto.subtle` mock to global setup

- for Jest to work with WebAssembly we need to update to next major version - to support dependencies for new Jest version we need to bump Typesript as well - let's fix problems found by new Typescript version

Allow destructuring objects to remove unwanted fields from the objects. This is pattern we are using often across the codebase.

.eslintrc.js

Shadowfiend · 2023-06-29T03:29:10Z

background/services/internal-signer/encryption.ts

@@ -10,6 +11,11 @@ export type EncryptedVault = {
  cipherText: string
 }

+export enum VaultVersion {


Re: my note about not leaking vault version information: that basically means we should try to be able to not export this.

I see but if we need to know and save (in the service) on which version we are then we need this exported, right?

Ultimately discussed in #3502 (comment), since what I was pushing for is precisely not saving in the service which version we're on—but the need for that has been clarified.

- return `success` boolean - make sure decrypted vaults match - send event on migration fail

Shadowfiend

Handful of final tidbits here:

Let's rename to migrateVaultsToLatestVersion.
Let's return the error message from the exception if there is a failure.
Let's bubble the error message to the migration failure analytics event.
Let's update the analytics event to VAULT_MIGRATION alongside the above.
MIGRATION_FAILED shouldn't be a one-time event, we should see it every time a migration fails. It'll help us better understand whether we're seeing transient or permanent failures.
Last but not least: I think the success flag should actually be a migrated flag, which should be true if the vaults (a) needed migration and (b) succeeded at migrating. I think the error message should be the true indicator that there was an error. So we would send the migration event if (migrated), and send the failure event if (error !== undefined). Thoughts?

Shadowfiend · 2023-07-02T19:06:34Z

Pushed the above changes to accelerate a little; we can always walk the commit back if necessary.

The main thing I noticed while adding a test for the error messages is that (and this was true before my changes) if we track migration failures, we track migration failures due to incorrect passwords as well as ones that might be due to an internal technical issue. There are a few ways we could mitigate this, but my instinctive reaction is that we roll it out as-is and see how it goes.

Vault migration is no longer tracked as Argon2 specifically, but generically for all migrations. Already-migrated vaults are not tracked, and the migration function return value reflects that no migration was performed. Additionally, error messages are bubbled out of the migration function and reported up to the caller. The main outcome here is that PostHog migration events include the migrated-to version, and PostHog migration failure events include the error message. This will leave us open to future migrations, and will let us know if there are certain failures that are happening broadly that we may be able to do something about. Notably, wrong passwords will be tracked as migration errors if a wrong password is typed with an older vault version in the mix. Mitigating this may or may not be a good idea.

jagodarybacka · 2023-07-03T15:38:42Z

if we track migration failures, we track migration failures due to incorrect passwords

ugh 😞 so what exactly do we want to measure with these events? I think info on how many users are migrated is the most important one anyway so we are good here

Shadowfiend

All righty, good to go here. Since I pushed some code, going to let @jagodarybacka do a final sanity check and then merge.

Shadowfiend · 2023-07-03T21:38:50Z

if we track migration failures, we track migration failures due to incorrect passwords

ugh 😞 so what exactly do we want to measure with these events? I think info on how many users are migrated is the most important one anyway so we are good here

Secondarily, I think it'll be useful to know potentially know what we need to look at if they aren't being migrated, or at least how many failures in migrations we're seeing. I'm comfortable shipping without being sure we can get that info though.

jagodarybacka

QA went fine, let's 🚢

@jagodarybacka

## What's Changed * Add private key onboarding flow by @jagodarybacka in #3119 * Private key JSON import by @jagodarybacka in #3177 * Allow export of private keys and mnemonics by @jagodarybacka in #3248 * Export private key form by @jagodarybacka in #3255 * Unlock screen for the account backup by @kkosiorowska in #3257 * Show mnemonic menu by @jagodarybacka in #3259 * Fix background blur issue by @jagodarybacka in #3265 * Account backup UI fixes by @jagodarybacka in #3270 * Fix unhiding removed accounts by @jagodarybacka in #3282 * New error for incorrectly decrypted JSON file by @jagodarybacka in #3293 * Export private keys from HD wallet addresses by @jagodarybacka in #3253 * Refactor keyring redux slice to remove `importing` field by @jagodarybacka in #3309 * 📚 Accounts backup by @kkosiorowska in #3252 * Catch Enter keypress on Unlock screen by @jagodarybacka in #3355 * Rename `keyring` to `internal signer` and other improvements by @jagodarybacka in #3331 * 🗝 QA - Accounts backup and private key import by @jagodarybacka in #3266 * Remove private key signers if they are replaced by accounts from HD wallet by @jagodarybacka in #3377 * RFB 4: One-Off Keyring Design by @Shadowfiend in #3372 * Copy to clipboard warning by @kkosiorowska in #3488 * Allow setting custom auto-lock timer by @hyphenized in #3477 * Use Argon2 for encrypted vaults by @jagodarybacka in #3502 * 👑 Private keys import and accounts backup by @jagodarybacka in #3089 * Untrusted assets should not block the addition of custom tokens by @kkosiorowska in #3491 * Flip updated dApp connections flag by @Shadowfiend in #3492 * v0.41.0 by @Shadowfiend in #3531 * Switch to a given network if adding a network that is already added. by @0xDaedalus in #3154 * Remove waiting for Loading Doggo component in E2E tests by @jagodarybacka in #3541 * Squeeze content to better fit on Swaps page by @jagodarybacka in #3542 * Refactor of terms for verified/unverified assets by @kkosiorowska in #3528 * Fix ChainList styling by @fulldecent in #3547 * Update release checklist by @jagodarybacka in #3548 * Fix custom asset price fetching by @hyphenized in #3508 * Sticky Defaults: Make Taho-as-default replace MetaMask in almost all cases by @Shadowfiend in #3546 ## New Contributors * @fulldecent made their first contribution in #3547 **Full Changelog**: v0.41.0...v0.42.0 Latest build: [extension-builds-3549](https://github.com/tahowallet/extension/suites/14268975651/artifacts/801826435) (as of Thu, 13 Jul 2023 09:51:56 GMT).

Rewrite vault encryption function using argon2

e789c36

- add required packages - configure webpack to work with webAssembly - add neccessary config to `content_security_policy` to allow webAssembly

jagodarybacka requested a review from Shadowfiend June 26, 2023 14:54

jagodarybacka self-assigned this Jun 26, 2023

jagodarybacka commented Jun 26, 2023

View reviewed changes

background/services/internal-signer/encryption.ts Outdated Show resolved Hide resolved

Shadowfiend reviewed Jun 26, 2023

View reviewed changes

background/services/internal-signer/encryption.ts Outdated Show resolved Hide resolved

background/services/internal-signer/encryption.ts Outdated Show resolved Hide resolved

manifest/manifest.development.json Show resolved Hide resolved

jagodarybacka added 5 commits June 27, 2023 12:28

Specify argon hash length

2884bf8

Let's set length to 32 bytes to match length expected by AES-GCM

Add migration strategy for vaults encrypted with PBKDF2

ccf69a8

Merge branch 'keyring-with-pk' into migrate-to-argon

cff31fd

Rename old.. to deprecated...

7de1fbf

Handle errors on vaults migration

9fb2145

Catch erros if vaults migration to Argon2 fails and allow to continue with old vaults encrypted with PBKDF2. Log analytics event when vaults are succesfully migrated.

hyphenized reviewed Jun 28, 2023

View reviewed changes

Shadowfiend reviewed Jun 28, 2023

View reviewed changes

jagodarybacka added 5 commits June 28, 2023 14:55

Allow Jest to work with wasm

8ce4d3b

- allowed Jest to fetch WebAssembly files - moved `crypto.subtle` mock to global setup

Move sending migration event to Internal Signer service

b04ee01

Bump Jest and Typescript versions

1d3d1a6

- for Jest to work with WebAssembly we need to update to next major version - to support dependencies for new Jest version we need to bump Typesript as well - let's fix problems found by new Typescript version

Add new unit tests to test encryption with Argon2

1acca3b

Add ignoreRestSiblings to no-unused-vars lint rule

0216036

Allow destructuring objects to remove unwanted fields from the objects. This is pattern we are using often across the codebase.

jagodarybacka marked this pull request as ready for review June 28, 2023 15:02

jagodarybacka requested review from a team as code owners June 28, 2023 15:02

jagodarybacka requested a review from Shadowfiend June 28, 2023 15:02

Shadowfiend reviewed Jun 29, 2023

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

Shadowfiend reviewed Jun 29, 2023

View reviewed changes

jagodarybacka added 2 commits June 29, 2023 11:04

Adjust no-unused-vars rule to use _ for unused elements

ac79ba4

Refactor Argon migration function

a569168

- return `success` boolean - make sure decrypted vaults match - send event on migration fail

jagodarybacka requested a review from Shadowfiend June 30, 2023 08:55

Shadowfiend requested changes Jun 30, 2023

View reviewed changes

Shadowfiend force-pushed the migrate-to-argon branch from cfa620e to d0e2908 Compare July 2, 2023 21:21

Shadowfiend approved these changes Jul 3, 2023

View reviewed changes

jagodarybacka commented Jul 4, 2023

View reviewed changes

jagodarybacka merged commit 4fc5d6a into keyring-with-pk Jul 4, 2023

jagodarybacka deleted the migrate-to-argon branch July 4, 2023 08:15

jagodarybacka mentioned this pull request Jul 4, 2023

Argon2 integration #3470

Closed

kkosiorowska mentioned this pull request Jul 13, 2023

v0.42.0 #3549

Merged

beemeeupnow mentioned this pull request Aug 5, 2023

Migrate key derivation from PBKDF to Argon2 #1328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Argon2 for encrypted vaults #3502

Use Argon2 for encrypted vaults #3502

jagodarybacka commented Jun 26, 2023 •

edited by Shadowfiend

Loading

jagodarybacka commented Jun 27, 2023

Shadowfiend commented Jun 27, 2023

hyphenized Jun 28, 2023

Shadowfiend Jun 28, 2023

Shadowfiend Jun 29, 2023 •

edited

Loading

Shadowfiend left a comment

Shadowfiend Jun 28, 2023

Shadowfiend Jun 28, 2023

Shadowfiend Jun 28, 2023

Shadowfiend Jun 28, 2023

jagodarybacka Jun 28, 2023

Shadowfiend Jun 29, 2023 •

edited

Loading

jagodarybacka Jun 29, 2023

jagodarybacka Jun 30, 2023

Shadowfiend Jun 30, 2023

Shadowfiend Jun 29, 2023

jagodarybacka Jun 30, 2023

Shadowfiend Jun 30, 2023

Shadowfiend left a comment

Shadowfiend commented Jul 2, 2023

jagodarybacka commented Jul 3, 2023

Shadowfiend left a comment

Shadowfiend commented Jul 3, 2023

jagodarybacka left a comment

Use Argon2 for encrypted vaults #3502

Use Argon2 for encrypted vaults #3502

Conversation

jagodarybacka commented Jun 26, 2023 • edited by Shadowfiend Loading

What

Testing

jagodarybacka commented Jun 27, 2023

Shadowfiend commented Jun 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Shadowfiend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend left a comment

Choose a reason for hiding this comment

Shadowfiend commented Jul 2, 2023

jagodarybacka commented Jul 3, 2023

Shadowfiend left a comment

Choose a reason for hiding this comment

Shadowfiend commented Jul 3, 2023

jagodarybacka left a comment

Choose a reason for hiding this comment

jagodarybacka commented Jun 26, 2023 •

edited by Shadowfiend

Loading

Shadowfiend Jun 29, 2023 •

edited

Loading

Shadowfiend Jun 29, 2023 •

edited

Loading