[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:
https://github.com/bisq-network/bisq/pull/4214#issuecomment-630519262
-------------- 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