Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WALLET] Updated the encrypt wallet logic to not replace master key #697

Conversation

mxaddict
Copy link
Contributor

@mxaddict mxaddict commented Jun 4, 2020

closes #695

I've updated the wallet to not replace master key with new key on encryption.

Test scenarios:

  • Scenario 1
    • Create wallet
    • Dumped mnemonic
    • Encrypted wallet
    • Dumped mnemonic again
    • Made sure all dumped mnemonic match
  • Scenario 2
    • Create wallet
    • Dumped mnemonic 1
    • Encrypted wallet
    • Dumped mnemonic 2
    • Changed password
    • Dumped mnemonic 3
    • Made sure all dumped mnemonic match
  • Scenario 3
    • Restored wallet with mnemonic
    • Dumped mnemonic 1
    • Encrypted wallet
    • Dumped mnemonic 2
    • Changed password
    • Dumped mnemonic 3
    • Made sure all dumped mnemonic match

@mxaddict mxaddict changed the title Updated the encrypt wallet logic to not replace master key [WALLET] Updated the encrypt wallet logic to not replace master key Jun 4, 2020
@navbuilder
Copy link

A new build of 25c173d has completed succesfully!
Binaries available at https://build.nav.community/binaries/disabled-regeneration-of-master-key-on-encryption

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/wallet/wallet.cpp Show resolved Hide resolved
@mxaddict
Copy link
Contributor Author

mxaddict commented Jun 4, 2020

I'll try it out with those lines reverted.

@mxaddict mxaddict force-pushed the disabled-regeneration-of-master-key-on-encryption branch from 25c173d to d76bfbf Compare June 4, 2020 20:31
@mxaddict
Copy link
Contributor Author

mxaddict commented Jun 4, 2020

Hmmm, you were right, it does work without those 2 changes, I don't understand why though, I thought that new masterkey was the key used for the mnemonic...

So the HD master key is the one used?

@aguycalled
Copy link
Member

/**

  • Private key encryption is done based on a CMasterKey,
  • which holds a salt and random encryption key.
  • CMasterKeys are encrypted using AES-256-CBC using a key
  • derived using derivation method nDerivationMethod
  • (0 == EVP_sha512()) and derivation iterations nDeriveIterations.
  • vchOtherDerivationParameters is provided for alternative algorithms
  • which may require more parameters (such as scrypt).
  • Wallet Private Keys are then encrypted using AES-256-CBC
  • with the double-sha256 of the public key as the IV, and the
  • master key's key as the encryption key (see keystore.[ch]).
    */

@aguycalled
Copy link
Member

master key here is more related to the password used for encrypting

@mxaddict
Copy link
Contributor Author

mxaddict commented Jun 4, 2020

master key here is more related to the password used for encrypting

Gotcha, the name was confusing me I guess 😸

@navbuilder
Copy link

A new build of d76bfbf has completed succesfully!
Binaries available at https://build.nav.community/binaries/disabled-regeneration-of-master-key-on-encryption

@Xa5r
Copy link

Xa5r commented Jun 5, 2020

Test scenarios:

* [x]  Scenario 1
  
  * [x]  Create wallet
  * [x]  Dumped mnemonic
  * [x]  Encrypted wallet
  * [x]  Dumped mnemonic again
  * [x]  Made sure all dumped mnemonic match

* [x]  Scenario 2
  
  * [x]  Create wallet
  * [x]  Dumped mnemonic 1
  * [x]  Encrypted wallet
  * [x]  Dumped mnemonic 2
  * [x]  Changed password
  * [x]  Dumped mnemonic 3
  * [x]  Made sure all dumped mnemonic match

* [x]  Scenario 3
  
  * [x]  Restored wallet with mnemonic
  * [x]  Dumped mnemonic 1
  * [x]  Encrypted wallet
  * [x]  Dumped mnemonic 2
  * [x]  Changed password
  * [x]  Dumped mnemonic 3
  * [x]  Made sure all dumped mnemonic match

Tested all this scenarios including master private key, the mnemonic never changes.

@chasingkirkjufell
Copy link
Contributor

chasingkirkjufell commented Jun 5, 2020

  • Scenario 1
    • Create wallet
    • Dumped mnemonic
    • Encrypted wallet
    • Dumped mnemonic again
    • Made sure all dumped mnemonic match
  • Scenario 2
    • Create wallet
    • Dumped mnemonic 1
    • Encrypted wallet
    • Dumped mnemonic 2
    • Changed password
    • Dumped mnemonic 3
    • Made sure all dumped mnemonic match
  • Scenario 3
    • Restored wallet with mnemonic
    • Dumped mnemonic 1
    • Encrypted wallet
    • Dumped mnemonic 2
    • Changed password
    • Dumped mnemonic 3
    • Made sure all dumped mnemonic match

Copy link
Member

@aguycalled aguycalled left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested gitian osx

@aguycalled aguycalled merged commit 83bb19d into navcoin:master Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding encryption to non encrypted wallet.dat changes master private key
5 participants