[bisq-network/bisq] Add rpc wallet protection endpoints (#4214)

Stan notifications at github.com
Tue May 19 01:20:52 UTC 2020

@chimp1984 ,
The full thread is tedious, so thanks for the comment.  Please do read this comment and help me clarify something I think needs to be changed?

To be direct, my question is:
Should `CoreWalletService` inject `TradeWalletService` so `CoreWalletService` can use the cached `TradeWalletService#aesKey`, instead of it's own (duplicate) copy of the `aesKey`?

Some background...  

Parts of this PR relevant to your comments are (1) an `unlockwallet` method that takes a password and caches the derived aesKey, and (2) a `lockwallet` method that clears the cached aesKey.  

Before I figured out all I needed for most encrypted wallet operations was the aesKey, I made sure kludged unlock/lock wallet methods worked for me by removing and re-setting the password (both causing a wallet re-write).  Seeing this in the thread surely caused worry for some, not to mention
> (not sure if already resolved, have not read the full thread...)

Then I changed the `unlockwallet` and `lockwallet` methods so they do not trigger wallet re-writes -- after looking more closely at bitcoin-cli and the bisq UI events that trigger `onAesKey()`.  

In the final merge, `CoreWalletService#unlockWallet()` caches the password arg derived aesKey, and `CoreWalletService#lockWallet()` clears that cached aesKey.  Right now, there are no gRPC endpoints that need that cached aesKey, but my assumption is operations such as funds withdrawal and tx-signing will need the key, and not the password used to derive it.  

Keeping that key in memory between "unlock" and "lock" commands is a security risk, but maybe less of one than typing the password in the console for every operation on an encrypted wallet?  

If you think my assumptions behind the `unlockwallet` and `lockwallet` impls are incorrect, let me know...  If not, I repeat the question:  should `CoreWalletService` inject `TradeWalletService` so `CoreWalletService` can use the cached `TradeWalletService#aesKey`, instead of it's own (duplicate) copy of the `aesKey`?

I did implement it then backed it out, waiting for an endpoint that actually needs the aesKey to work (or your recommendation, whichever came first).

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200518/92fe97f3/attachment-0001.html>

More information about the bisq-github mailing list