Add API functions to initialize Haveno account

[duriancrepe]

Requesting review for issue #136.

[duriancrepe]

There are a few issues that I left as TODOs, but I think the feature is at a decent break-point:

• Call restart process on delete / restore account, which may open some can of worms due to the difficulty of accurately restarting a java process. I attempted restarting the main object but ran into many issues due to the static classes and guice instantiation / tear down issues. The same process object restart via new HavenoDaemonExe.execute(args). would be the most elegant way to do things but would require a lot of time and effort.
• Desktop app login UI in password required mode
• Merge the account password with wallet password, which I kept separate since there were different rpc APIs involved
• Deduplicate ScryptUtil and CryptoUtil functions when unifying the account and wallet passwords
• Handle the various exceptions on shutdown when shutting down prior to account login. The exceptions aren't regressions and do not corrupt data but were exposed via the haveno-ui-poc tests since it exposes the ability to shutdown before fully starting. They would have been reproducible if you ctrl+c the terminal before the app fully started up.
• Stretch goal: encrypt all persistence using same mechanism as the key encryption

Additionally would need feedback and scrutiny on how the account password is applied to the KeyRing encryption and persistence, and whether that should be persisted similarly to the other persisted formats through the Persistence manager. I did not approach this yet because the original KeyRing persistence didn't follow the pattern.

[woodser]

FYI I'm starting to review this and add changes. Will submit PRs to your branches when ready.

[woodser]

I've submitted PRs to your branches:

duriancrepe#1
duriancrepe/haveno-ts#2

Please review and squash the commits after the commits which update monero-java and monero-javascript.

Also, please add me as a co-author for substantial refactoring to the tests and services with a commit message like:

Add API functions to initialize Haveno account

Co-authored-by: [email protected]

Thanks!

[duriancrepe]

Thanks for the update.

I reviewed the changes, pulled the code into my branch and ensured the "Can manage account" test passes.

The commits are squashed in both pull requests.

There are some minor static code analysis complaints that could be ignored or fixed in a subsequent commit.

[premek]

I didn't go through all of it, it's quite a lot of code :)

Just one small thing, in Java it's better to use char[] for passwords instead of String which would allow you to clear it after use so it doesn't stay in memory until the GC collects it. I don't know if its possible here, is the password coming from proto message? Not sure how that works here.

@duriancrepe To review the security more easier, could you please outline in a few simple sentences what are the key points security wise here?

e.g. I noticed the two salts in the persisted file, the payload and the pwEncrypted, would you mind explaining what each of them is used for?

[duriancrepe]

@premek The password string comes from the grpc so I don't see much of a benefit for hiding the password string in memory as we don't have full control of that path, but it could be done.

The Haveno account is represented by the user's key ring in which is used to sign messages and encrypt data. In the original implementation, the keys were persisted without encryption.

Now when the user creates the Haveno account, the key ring's key files are encrypted with a password, and must be decrypted to access the keys and thus sign messages as the account owner.

The encryption of the key file is done when saving the key file at the function savePrivateKey

The file format is as follows

• [UTF-8] The magic string HVNENC
• [INT] Version (1)
• [INT] Salt length (20)
• [INT] Secret Iterations (65536)
• [INT] Secret Length (256)
• [Bytes(20)] Password salt
• [Bytes] Password hash generated using Scrypt, used to verify the input password (salted + hashed password).
• [Bytes(20)] Encryption salt, for doing the encryption of the user's private keys.
• [Bytes] The AES encrypted payload using an encryption key generated using PBKDF2WithHmacSHA512 with the password. The encryption key is generated with the above parameters (iterations, length, and salt).

The reason there are 2 salts is because one is to verify the password is correct via Scrypt hash, and the other salt is to initialize the encryption key to encrypt the payload (user's private key). I tried to decrypt the payload without a password verification phase but that can result in incorrect cryptographic exceptions which doesn't let the user know if it was due to incorrect password or something horrible happened (file corruption?). I believe that approach could be done if the decrypted payload also contains some sort of magic and special exception handing, but that could also have security implications.

The Scrypted password hash uses the existing code that was used to verify the bitcoin wallet password. The encryption of the key uses the existing AES encryption encryptPayloadWithHmac function.

The security concern I see here is that there is that the Scrypt password hash is written to the file, and may somehow be used to decrypt the AES encrypted payload (which is why we have 2 different salts). I could not find any existing discussion on this topic.

UPDATE: After doing some investigation on how Scrypt works, it also uses PBKDF2 in some form to generate its hash, and drilling deeper I see the parameters are quite different than how its used to generate the encryption key. Overall, it's definitely safer to encrypt the keys than leaving it without encryption at all but is worth keeping in mind.

[premek]

The password string comes from the grpc so I don't see much of a benefit for hiding the password string in memory

I agree

Thanks for the summary, it helps a lot, I just have a few more questions, see my comments

[duriancrepe]

The password string comes from the grpc so I don't see much of a benefit for hiding the password string in memory

I agree

Thanks for the summary, it helps a lot, I just have a few more questions, see my comments

Thanks, I believe I've address all your comments. I think its worth attempting to simplify the key ring encryption to also work as the password verification which should simplify the encrypted key file format. I need to experiment and research the topic a bit further in detail to ensure its correctness.

[duriancrepe]

@premek @woodser I've added a commit which is a much more elegant way of validating the account password and protecting the user's keys.

The critical part was realizing that the decryption of the keys can be used to validate the password, since an incorrect password throws a padding exception, or fails the HMAC check. I had attempted this approach earlier, but failed to realize that the padding exception was to be expected, and that in addition the decryption function uses HMAC to verify that the payload was decrypted with the correct key.

I've updated the file format to:

[UTF-8] The magic string HVNENC
[INT] Version (1)
[INT] Salt length (20)
[Bytes(20)] Encryption salt
[Bytes] AES encrypted payload using the derived encryption key generated via Scrypt.

[woodser]

I added commits with more cleanup:

duriancrepe#2
duriancrepe/haveno-ts#3

They default to passwordRequired=false for default compatibility with the desktop apps and they resolve issues resolving disputes.

[duriancrepe]

I've pulled in your changes, let me know if I should squash all the commits again, left them unsquashed for easier review