[bisq-network/bisq] Implement simple cleartext gRPC authentication (#4189)

[Chris Beams]

>  I know you don't want me to add a bunch of JUnit tests, so I am building a linux 'expect' test script to save me time. So far, the eight test cases are passing.

Just for the record, I have nothing against (and usually everything for) unit tests, but this is a special situation where it's awkward to work with them and not (yet, at least) worth the trouble. I'll be interested to see the `expect` scripts. I've also heard good things about https://github.com/sstephenson/bats as a bash-based testing system; it might be worth a look.

> I intend to push some wallet encrypt/decrypt methods Sunday or Monday, along with error handling for getbalance, which returns "Error: UNKNOWN" when the wallet is not yet avaiable or decrypted.

Perfect. I was actually going to ask you to do this next. In particular, handling the `Error: UNKNOWN` case.

How about this as a set of next steps?

 1. Handle the `Error: UNKNOWN` case in a first-class way, such that the user understands through an error message that they cannot perform the operation they're trying to (e.g. `getbalance`) because the wallet is encrypted and has not been unlocked.
 2. Implement a `setwalletpassword` method that, given a single argument, attempts to encrypt a previously unencrypted wallet using the argument as the wallet password. The method will fail with an appropriate error message if the wallet password has already been set. Given two arguments, the method will attempt to change the wallet password from its old value (1st argument) to a new value (2nd argument). In this way, the `setwalletpassword` method is like combining Bitcoin Core's https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/encryptwallet/ and https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/walletpassphrasechange/ methods into one.
 3. Implement `unlockwallet <passphrase> <timeout>` similar to the way https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/walletpassphrase/ works, and
 4. Implement `lockwallet` similar to the way https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/walletlock/ works.

And each of these would have expect/bats-based tests.

I was going to say that we should not implement a `removewalletpassword` method as a counterpart to `setwalletpassword <newpassword>`, because it's a potential security risk and there isn't a clear real-world use case for it (note that Bitcoin Core has no such method either), but, on thinking about it, perhaps we should implement it if only for the practical use case of automated testing. Ideally, we would spin up a new Bisq node on each run through the automated tests, using a new temporary data dir, (e.g. with `--appDataDir=$(mktemp -d)`), such that the wallet is always new and un-encrypted at the beginning of each run, but the downside of doing this is that it's time-consuming to spin up the node. When in a tight development loop, one would usually want to spin up a test node or complete localnet environment (see the Makefile in the root of the repo), and then be able to run the cli test suite against it very quickly as incremental code changes are being made. So I'm open to implementing a `removewalletpassword` method based on this rationale.

I just wanted to quickly type all of this up here as a comment here to get your feedback, but assuming we want to go this route, we should probably model the items above as their own issue(s). Perhaps you could transcribe this to a new issue, and make each of the above checkboxes. You can then put together a PR that will close that new issue when merged.

Another candidate next task is to implement a cookie-based alternative to explicitly providing `--apiPassword` to the daemon and `--password` to the cli when both are being run on the same machine. This would work very similarly to the way bitcoind generates a `${datadir}/.cookie` file that bitcoin-cli is aware of (see https://bitcoin.stackexchange.com/questions/46782/rpc-cookie-authentication for details if you're not familar). Doing this is actually more secure when running on the same machine because the passwords don't show up in plaintext in process lists, shell history, etc, and can be access-controlled using normal file permissions. Implementing this would mean making the cli aware of the default Bisq data directory location, and this would need to be done in a way that fully duplicates what's already in :core, keeping in mind that the :cli project is all about being a reference client, not about reusing code that other client / bot implementors could not easily reuse themselves. Again, this can and should be written up as its own issue. We can get together a project board to start managing these in a priority-ordered fashion, etc. I can help with that once we have a few issues vying for our attention.

-- 
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/4189#issuecomment-619521705
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200426/41e4aa04/attachment.html>