search

airgap-it / airgap-vault

The AirGap Vault is installed on a spare smartphone that has no connection to any network, thus it is air gapped. This app handles the private key.
MIT License
386 stars 109 forks source link

Feature/seed passphrase #10

Closed okipol88 closed 4 years ago

okipol88 commented 5 years ago

Hi,

I've added the possibility to add an additional passphrase that also participates in creating the accounts/keys/addresses. This is a feature I have been missing in a lot of wallets.

AndreasGassmann commented 5 years ago

Hi, thanks for another PR!

A couple of things:

  1. The package-lock should not be changed, especially if there are no dependency changes. I think you can do that by using npm ci instead of npm i, or you simply don't commit any package-lock changes.
  2. We are currently rewriting the whole Vault to Ionic 4, so this change would most likely be included once the rewrite is done.
  3. We discussed this feature a while back and decided it would be too confusing to the user to potentially have 3 different password (device password, paranoia password and bip39 password). However, this seems to be a feature a couple of people want, so we have to discuss that again.

Going forward, do you have any other changes you would like to implement? If so, please contact us/me beforehand either via telegram or open an issue here so we can discuss it before you invest too much time.

okipol88 commented 5 years ago

Hi, thanks for another PR!

A couple of things:

  1. The package-lock should not be changed, especially if there are no dependency changes. I think you can do that by using npm ci instead of npm i, or you simply don't commit any package-lock changes.
  2. We are currently rewriting the whole Vault to Ionic 4, so this change would most likely be included once the rewrite is done.
  3. We discussed this feature a while back and decided it would be too confusing to the user to potentially have 3 different password (device password, paranoia password and bip39 password). However, this seems to be a feature a couple of people want, so we have to discuss that again.

Going forward, do you have any other changes you would like to implement? If so, please contact us/me beforehand either via telegram or open an issue here so we can discuss it before you invest too much time.

1-2 Ok. Will try the ci command.

  1. Perhaps we can hide this under an "advanced" checkbox?
AndreasGassmann commented 5 years ago

We are still looking into this because it has more implications than we thought.

For example, in your current implementation, the passphrase gets stored to the "insecure" storage next to the label, etc. We have to discuss how we can solve that, either by not storing it at all or at least storing it in the secure storage.

okipol88 commented 5 years ago

I was thinking of secure storage of course. I would also like to add secure storage for "in browser" PC mode.

AndreasGassmann commented 5 years ago

We already have an implementation for the secure browser storage, but we didn't have time to test it yet. After we do some further testing, we'll make it available in a branch so you can have a look.

But the question is still if you actually want to store the bip39 password at all. Because it would in a way defeat the purpose of having a password if you store it together with your secret. Someone with access to your phone (+ device password) can get a hold of all your funds without knowing the bip39 password.

okipol88 commented 5 years ago

Valid point. Well ... Trezor does it in a "per request" manner. It is however necessary in airgap to see the balances and so on... or maybe make it @runtime only? Global or per account. It would be more confusing though... What do you think? The n+1 passphrase is always an additional word to at least minimize the unlikely collision of 24 word generation even more down.

AndreasGassmann commented 4 years ago

Thanks again for putting the work into this branch/feature. We ended up doing it slightly different, so I'm closing this PR. But the functionality is now in the latest version of the Vault.