Encrypted device sync

[riccardobl]

Description

This PR introduces a general purpose approach to end to end device sync and implements it in wallet attachments to solve #1304 .

Screenshots

Additional Context

It is developed to be a drop-in replacement to components/use-local-state.js, usage is as follows:

import useVaultStorageState from '@/components/use-user-vault-state'

const [value, setValue, clearValue] = useVaultStorageState('storage-key')
// ...
console.log(value)
// setValue(123)
// clearValue()

the useVaultStorageState hook handles everything internally and detects if the user has device sync enabled and fallbacks to localStorage if not, so there is no other code required to use this feature other than the one presented above.

useVaultStorageState needs to rely on localStorage to store the passphrase and the stored values if the user doesn't have device sync enabled. To prevent possible name conflicts it prefixs every value with "vault:" and suffixes with the userid.

Checklist

Are your changes backwards compatible? Please answer below:

yes it is backward compatible and everything that will be set using user-vault-state and everything that was set before using wallet attachments will automatically migrate to the encrypted device sync when the user enables it

in details:

• user-vault-state is backward compatible: if a key is not available with the new vault:key:userId syntax, it will try to read it normally from the localStorage
• configured wallet attachments are automatically migrated when the user enables device sync and confirms the migration dialog
• if an user doesn't have device sync activated yet and does some configurations, all the entries set using user-vault-state will be automatically migrated to device sync when the users enables it (and confirms the dialog).

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
8

For frontend changes: Tested on mobile? Please answer below:

yes

Did you introduce any new environment variables? If so, call them out explicitly here:

no

[ekzyis]

First of all, thank you for your work, this is AWESOME! #1304 is actually the most important wallet feature right now but we realized that we didn't set priority:high on it. We corrected this now so you actually get 2x more sats for #1304.

Regarding the code: I've played around with it and looked through the code and I like how you interfaced it nicely with the existing wallet code (just as you mentioned), nice work! I also tested syncing of wallets using Brave and Firefox and it works well on the happy path.

However, I noticed that it's possible to get the devices out of sync by resetting one. The other device shows disconnected now but is not actually reset (key still exists on device):

2024-09-18.22-57-01.mp4

If you now have a wallet configured that will get synced across the devices, decryption errors are thrown on the device that wasn't reset and is now "connected" with a wrong passphrase:

2024-09-18.23-00-20.mp4

Another thing I noticed is that the UI/UX could be improved. Going through the setup, I think we can simplify it. I think it's currently a bit intimidating to use since it's in a separate tab which makes it stand out and it might involve more than one step to setup because of the possible prompt for migration which sounds scary. After setup, there are three buttons in different colors.

For example, do we need this "scary" migration prompt? Does it make sense to enable this setting but not migrate? And can we express the state without three separate buttons that can be overwhelming and confusing?

I think we might be able to express everything a user might want to do with a single button on the settings page in a familiar layout and colors. The flow could be similar to how API keys are implemented. I'll give it a shot.

I've left some other suggestions but they are mostly about minor things.

[riccardobl]

Thank you for the review.

However, I noticed that it's possible to get the devices out of sync by resetting one. The other device shows disconnected now but is not actually reset (key still exists on device):

This should be fixed now, i've added some code to clear the stored key if the local and remote hashes mismatch

For example, do we need this "scary" migration prompt? Does it make sense to enable this setting but not migrate?

The only situation i could think of, where this makes sense, is when you have different conflicting settings on different devices and you want to chose which one is migrated. I think another solution is making the migration not overwrite keys, so that the first device that connects gets the precedence. I've changed the code to do that and got rid of the popup.

Also changed the ux to have a single button under general, what do you think? :

There was also some code in wallets/index.js that used localStorage to find the highest priority wallet and to check if it was "disabled locally" (used only in webln when the browser doesn't support it), so i had to change things around a bit.

[huumn]

Just a heads up: I'll be reviewing this tomorrow. I suspect it'll be merged then too.

[ekzyis]

Just realized that the wallet security disclaimer probably needs an update since what it says is no longer true if you enabled device sync.

[ekzyis]

Someone reminded me that when this is enabled, we no longer need to delete the wallets on logout.

This isn't included in this PR yet though.

[riccardobl]

Someone reminded me that when this is enabled, we no longer need to delete the wallets on logout.

This reminds me that it is probably a good practice to delete the vault key on logout.

My last two commits:

• Fix the wallet reset on logout to affect only localStorage entries
• Remove the vault key from localStorage on logout
• Make use-vault work for anonymous users (fallbacks to localOnly)

last point is to fix an error with the webln wallet that, when webln was not available, was trying to save its localOnly config to disable itself, and erroring because me was undefined

[huumn]

I'm impressed - code is clear and simple, and this problem isn't simple at all. Please lmk if you're looking for a job!

I was only able to get to this at the end of my day. The one thing we might really want to change before merging it is storing the keys as non-extractable in indexeddb rather than in localstorage. See: denoland/deno#11481 (comment). (I mentioned this to ek last week, but neither of us had done enough research to know this could be done.)

Basically, we'd generate the keys on device, store the CryptoKey object with extractable: false in indexeddb, then trash the passphrase so us or no one else can read it (or the derived key) again. This changes the device sync flow quite a bit sadly, but it seems worth it for the security advantage.

Regardless, you've more than qualified for the full bounty and I'll send that to you the moment you send me a 2m sat invoice. If you'd like, ek or I can make the remaining changes. Up to you!

I made myself notes while reviewing that I'm going to put here for convenience (don't feel like you need address these if you don't want to):

• expected copy button for device sync on initial setup - required clicking "manage device sync" to copy it afterward
• using qr to copy was surprising
• connect form error shows serverside errors in form field hint area
• reset/disconnect usage is a bit confusing - I suspect we can get away with "disconnect" then only show "reset" when there isn't already a passphrase
  • connect button should be far from reset button
• indexeddb storage for non-extractable CryptoKey object storage?
  • from https://pomcor.com/2017/06/02/keys-in-browser/ ... it should apply to symmetric keys too

    In the Web Cryptography API, generation of an RSA or ECDSA key pair produces two CryptoKey objects, one containing the private key, the other containing the public key. When the key pair is generated, the private key can be made non-extractable from its CryptoKey object. This means that it cannot later be extracted from the object by JavaScript code embedded in a Web page, even if that Web page has the same origin as the Web page containing the JavaScript code that invoked the key generation procedure. A CryptoKey object is not persistent by itself, and it is not an ordinary JavaScript object that could be encoded as a string for storage in localStorage, but it can be stored in a database accessed through the IndexedDB API.

  • also see: Enable non-extractable keys for Web Crypto denoland/deno#11481 (comment)

[riccardobl]

I'm impressed - code is clear and simple, and this problem isn't simple at all. Please lmk if you're looking for a job!

Thank you for the kind words! It is also thanks to @ekzyis improvements and suggestions.
I'll reach out to you on DevChat 🫡

I was only able to get to this at the end of my day. The one thing we might really want to change before merging it is storing the keys as non-extractable in indexeddb rather than in localstorage.

I've read the post you linked, and i am pretty sure it can be implemented easily, but for our specific use case:

• i believe we are already protected against trivial non targeted attacks (eg. an app/tab using some exploit to snoop into every localStorage looking for encrypted data and secrets), since we store only the passphrase, while all the encrypted data is queried from the server. So an attack needs to be somewhat targeted or sophisticated anyway.
• we don't need to worry about protecting the passphrase if it gets leaked, since @ekzyis changed it to be always randomly generated, so there is no risk that the user is reusing a password to some other service .
• we also need the user to be able to encrypt and decrypt, so both the non-extractable key and the passphrase give an attacker the same capabilities.

The only security improvement this would make, that i could think of is that, since we use bip39, an app (with enough permissions) could scan the filesystem for known words and should be able to find the passphrase in the format it is currently stored, while using the non extractable CryptoKey will provide some level of obfuscation, but:

• it won't help if the attack is targeted or sophisticated anyway, since it could be looking for the key patterns
• if we are looking to obfuscate the passphrase pattern for non targeted filesystem scans, (or someone opening the dev tools or an exploit for scanning the localStorage), we could just encrypt it with the user id inside localStorage

My main concerns in switching the logic to indexeddb are:

• more lines of code, since it has a more complex async interface
• indexeddb used to not work in incognito/private mode for some browsers, so i would probably write a fallback to localStorage, unless there is updated data on this, making it even more convoluted

That said, the end result shouldn't be too bad code wise imo, so we could still move forward with this. Thoughts?
Please let me know if i am missing something.

If you'd like, ek or I can make the remaining changes. Up to you!

No worries, I'll look into fixing at least some of them 🫡

[riccardobl]

reset/disconnect usage is a bit confusing - I suspect we can get away with "disconnect" then only show "reset" when there isn't already a passphrase

could something like this work?

the reset button appears only if device sync is configured but the user is disconnected

connect button should be far from reset button

I am not sure how to make this look good in the current layout (maybe an extra top margin on the reset button?)

expected copy button for device sync on initial setup - required clicking "manage device sync" to copy it afterward

added

connect form error shows serverside errors in form field hint area

is something like this preferable: 98089bd ?
or should it show in a toast?

[huumn]

we also need the user to be able to encrypt and decrypt, so both the non-extractable key and the passphrase

The non-extractable key cannot be trivially serialized and sent off device afaik, so the attacker would have to decrypt the vault on the device and send it off. That seems a step or two more targeted.

Beyond that, not having access to the key ourselves will prevent us from accidentally doing something that reveals it.

The CryptoKey API was specifically designed for this kind of thing, so I suspect it's the best tool we have for the job.

give an attacker the same capabilities

Neither method is bulletproof, but I don't think they're the same. Most security stuff is a game of inches afaict.

[huumn]

reset/disconnect usage is a bit confusing - I suspect we can get away with "disconnect" then only show "reset" when there isn't already a passphrase

could something like this work?

the reset button appears only if device sync is configured but the user is disconnected

Yep, I think that works.

connect button should be far from reset button

I am not sure how to make this look good in the current layout (maybe an extra top margin on the reset button?)

They can stay close in the current layout. We should probably create a reset prompt/barrier though if we don't already have one.

expected copy button for device sync on initial setup - required clicking "manage device sync" to copy it afterward

added

Cool! Lots of buttons, but I don't mind the "packaging" of v0 things being wip so long as the functionality is there.

connect form error shows serverside errors in form field hint area

is something like this preferable: 98089bd ? or should it show in a toast?

Toast seems to be the pattern we are using these days.

[riccardobl]

The non-extractable key cannot be trivially serialized and sent off device afaik, so the attacker would have to decrypt the vault on the device and send it off. That seems a step or two more targeted.
Beyond that, not having access to the key ourselves will prevent us from accidentally doing something that reveals it.

Makes sense, I hadn’t considered it that way.
The last commits implement the indexeddb stored non extractable object, if for some reason indexeddb is unavailable it fallsback to storing the passphrase in localStorage.

I moved the serialization/deserialization code into the set/get localkey functions, so the rest of the code needs to deal only with CryptoKey objects without worrying if it comes from the indexedDB or localStorage backends.

[huumn]

I ended up changing two things:

• I made the passphrase a textarea because it was unreadable on mobile
• I removed the QR because:
  • before device sync is enabled, it can't be used (no "connect" option yet on other devices)
  • after device sync is enabled, we can't read the passphrase

[huumn]

I ended up reverting this because given the way wallets are loaded, every time useWallet or useWallets is called, which is every where, and jankily so (side effects galor ... not this prs fault), it was making a request to the server for every sending wallet we support (both on the server and the client).

It does this mostly because we don't know the wallet priority and need to grab all of them to see the wallet priority, and the useWallet code was written assuming everything is stored locally.

To support device sync, we'll likely need a polling context that grabs the vault periodically so that we have a predictable number of API calls to support it.

This shouldn't be too difficult but it's a bit more involved. I have some work locally making progress on this.