RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet

luke-jr commented at 8:52 pm on June 16, 2017: member

Simple rebase of current RPC stuff. No endpoints yet.

luke-jr force-pushed on Jun 16, 2017

in src/httprpc.cpp:201 in dbbdef9942 outdated

190@@ -184,6 +191,23 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
191         // Set the URI
192         jreq.URI = req->GetURI();
193 
194+#ifdef ENABLE_WALLET
195+        if (walletName.empty()) {

ryanofsky commented at 9:10 pm on June 16, 2017:

It seems like you could eliminate a bunch of ifdefs if you got rid of the JSONRPCRequest wallet pointer and did this mapping in GetWalletForJSONRPCRequest instead of here. This would keep core rpc code a little more independent from the wallet.

luke-jr commented at 9:45 pm on June 19, 2017:

That would be less clean when calling in from GUI, tests, etc. But maybe a void* would make sense on the class?

ryanofsky commented at 10:15 pm on June 19, 2017:

That would be less clean when calling in from GUI, tests, etc.

I can see how deriving the wallet pointer in GetWalletForJSONRPCRequest could make gui & test code messier depending on how it was written, but if you provided an inverse function that set up the request object given the wallet name or pointer, I think that would keep things clean and isolated.

Anyway, this is just a thought. It looks like if you wanted you could get rid of a lot of ifdefs even keeping the pointer member.

But maybe a void* would make sense on the class?

As long as there is a pointer member, I don’t think making it void has advantages over using the forward declaration. Changing it to void just throws away type information and requires you to add casts, without letting you do anything that wasn’t possible otherwise.

promag commented at 10:03 pm on August 4, 2017:

Agree with @ryanofsky. There has been an effort to avoid more wallet dependencies.

fanquake added the label RPC/REST/ZMQ on Jun 18, 2017

luke-jr renamed this:
~~RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet~~
RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet (multiwallet RPC support)
on Jun 18, 2017

luke-jr force-pushed on Jun 20, 2017

in src/rpc/server.h:51 in 7b73f24311 outdated

47@@ -46,8 +48,15 @@ class JSONRPCRequest
48     bool fHelp;
49     std::string URI;
50     std::string authUser;
51-
52-    JSONRPCRequest() : id(NullUniValue), params(NullUniValue), fHelp(false) {}
53+#ifdef ENABLE_WALLET

ryanofsky commented at 1:18 pm on June 20, 2017:

In commit “RPC: Pass wallet through JSONRPCRequest”

Consider dropping this ifdef. It saves an insignificant amount of memory and seems like noise.

in src/rpc/server.h:56 in 7b73f24311 outdated

53+#ifdef ENABLE_WALLET
54+    CWallet *wallet;
55+#endif
56+
57+    JSONRPCRequest() : id(NullUniValue), params(NullUniValue), fHelp(false)
58+#ifdef ENABLE_WALLET

ryanofsky commented at 1:18 pm on June 20, 2017:

In commit “RPC: Pass wallet through JSONRPCRequest”

Could drop this ifdef also.

in src/httprpc.cpp:126 in b77b2f254c outdated

121@@ -119,14 +122,17 @@ static bool multiUserAuthorized(std::string strUserPass)
122             std::string strHashFromPass = HexStr(hexvec);
123 
124             if (TimingResistantEqual(strHashFromPass, strHash)) {
125+                if (vFields.size() > 3) {
126+                    walletNameOut = vFields[3];

ryanofsky commented at 1:21 pm on June 20, 2017:

In commit “RPC: Allow rpcauth configs to specify…”

Should update -rpcauth documentation to mention the new field.

ryanofsky commented at 2:16 pm on June 20, 2017:

In commit “RPC: Allow rpcauth configs to specify…”

I think instead of interpreting the 4th rpcauth field as a wallet filename field, it might be better to treat it as a generic options field (similar to the field in fstab files for mount options). E.g. instead of:

0rpcauth=user:salt:hash:filename.dat

You would write:

0rpcauth=user:salt:hash:wallet=filename.dat

This would be more extensible, also more readable.

promag commented at 9:45 pm on August 4, 2017:

Also add a test for the new parameter?

ryanofsky commented at 1:31 pm on June 20, 2017: member

ACK b77b2f254cc365728790f345deedbed1204964bb.

Needs updated documentation, also would be good to have python tests.

Tested with:

0bitcoind -regtest -wallet=w1.dat -wallet=w2.dat -debug=1
1bitcoin-cli -regtest -rpcuser=user1 -rpcpassword=V6CGvawtTWCHzt51knRvFfTejjjfy06UzSt_FiB3Fxw= getwalletinfo
2bitcoin-cli -regtest -rpcuser=user2 -rpcpassword=V6CGvawtTWCHzt51knRvFfTejjjfy06UzSt_FiB3Fxw= getwalletinfo
3bitcoin-cli -regtest -rpcuser=user3 -rpcpassword=V6CGvawtTWCHzt51knRvFfTejjjfy06UzSt_FiB3Fxw= getwalletinfo

And $HOME/.bitcoin/bitcoin.conf:

0rpcauth=user1:51902a7be9c9911079af388a927f$22904ad1bfec659ee1e61d1b3dd73f7b552c6d2d0d1e9f71f6ee833954d062da:w1.dat
1rpcauth=user2:51902a7be9c9911079af388a927f$22904ad1bfec659ee1e61d1b3dd73f7b552c6d2d0d1e9f71f6ee833954d062da:w2.dat
2rpcauth=user3:51902a7be9c9911079af388a927f$22904ad1bfec659ee1e61d1b3dd73f7b552c6d2d0d1e9f71f6ee833954d062da:-

ryanofsky commented at 2:20 pm on June 20, 2017: member

There was a lot of objection at last IRC meeting (https://botbot.me/freenode/bitcoin-core-dev/msg/87311878/) to choosing wallet based on RPC username & password, mostly for security reasons (“securing RPC for multiple users is absolutely a nightmare”).

Personally, I don’t like the choosing wallet based on username because I think it makes for a clumsy UI. Adding support for a simple -wallet= option to bitcoin-cli and working with regular cookie authentication just seems a lot more user-friendly than having to deal with -rpcauth, the share/rpcuser script, and all of that.

ACKing this PR though because it makes multiwallet usable, and the implementation is pretty clean. If we don’t want to use rpcauth for wallet security, we could allow all users to access all wallets and just interpret the new rpcauth wallet option as the default wallet for the user.

jnewbery commented at 1:20 pm on June 28, 2017: member

Multi-user for multiwallet is definitely a very useful feature and one that we should be aiming for long-term, so this is good to see. I think the implementation some more work before its ready:

most importantly, having individual user wallet access authentication will give users the impression that it’s safe to open RPC access to multiple users, which it absolutely isn’t. Just using the standard RPC commands, a malicious user could cause mischief by stopping the node, changing consensus state using invalidateblock/preciousblock, eclipse the node using disconnectnode/addnode, etc. RPC is not a secure interface and we should be very careful to not give users the impression that it is.
this implementation adds a lot of #ifdef ENABLE_WALLETs to libbitcoin_server.a. We should be trying to remove those in order to remove circular dependency between libbitcoin_server.a and libbitcoin_wallet.a (see #7965). This PR would make future work to cleanly separate wallet from server more difficult.
this implementation needlessly binds multiwallet to multi-user. It does not allow a single user to have access to multiple wallets or select a wallet on a per-call basis.

So, definite concept ACK that we should do this, but I think it should be sequenced after wallet separation. That would make the implementation a lot cleaner and make it easier to provide an implementation that is secure and safe for users.

in src/httprpc.cpp:132 in b77b2f254c outdated

131     }
132     return false;
133 }
134 
135-static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUsernameOut)
136+static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUsernameOut, std::string& walletNameOut)

promag commented at 9:46 pm on August 4, 2017:

Just wallet_name?

in src/httprpc.cpp:168 in b77b2f254c outdated

167@@ -162,7 +168,8 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
168     }
169 
170     JSONRPCRequest jreq;
171-    if (!RPCAuthorized(authHeader.second, jreq.authUser)) {
172+    std::string walletName;

promag commented at 9:46 pm on August 4, 2017:

wallet_name.

luke-jr force-pushed on Aug 25, 2017

luke-jr force-pushed on Sep 2, 2017

in src/httprpc.cpp:192 in 370d3361e8 outdated

187@@ -180,6 +188,49 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
188         // Set the URI
189         jreq.URI = req->GetURI();
190 
191+#ifdef ENABLE_WALLET
192+        static const std::string WALLET_ENDPOINT_BASE = "/wallet/";

TheBlueMatt commented at 10:15 pm on September 21, 2017:

Can we not allow an enumeration of possible users here and then have the user->wallet mapping checked in rpcwallet.cpp?

luke-jr commented at 11:17 pm on September 21, 2017:

rpcwallet.cpp is already past the point where the GUI and RPC abstractions combine.

Also, we are already enumerating the possible users here. To move it would mean two enumerations…

TheBlueMatt commented at 9:57 pm on September 22, 2017:

I was primarily echoing @jnewbery’s comments above about needing cleanups, especially not introducing a bunch more ifdef ENABLE_WALLETs, and this is maybe an obvious case where you seem to be possible-needlessly moving code from src/wallet to src/httprpc. It may be a bit more effecient, but I’m not sure its worth mucking up more wallet stuff in httprpc here.

luke-jr commented at 10:21 pm on September 22, 2017:

There is no src/wallet code that is strictly for RPC. I could make a function there, I suppose, and call it from here, but that seems just plain ugly?

TheBlueMatt commented at 9:04 pm on September 26, 2017:

Why not throw some additional registration stuff in src/wallet/rpcwallet? There’s already registration stuff there now. httprpc should be, IMO, as much as possible, a “dumb dispatcher” - wallet should tell RPC what it wants, and RPC can pass in the parameters it was given by the client and rpcwallet.cpp can handle how to deal with them.

luke-jr force-pushed on Mar 6, 2018

MarcoFalke added the label Needs rebase on Jun 6, 2018

RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet 43b92a9bb8

luke-jr renamed this:
~~RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet (multiwallet RPC support)~~
RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet
on Nov 2, 2018

luke-jr force-pushed on Nov 2, 2018

luke-jr commented at 5:21 am on November 2, 2018: member

Rebased

DrahtBot removed the label Needs rebase on Nov 2, 2018

DrahtBot commented at 8:43 am on November 2, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#13756 (wallet: “avoid_reuse” wallet flag for improved privacy by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jnewbery commented at 5:41 pm on November 2, 2018: member

Concept ACK. I still have a few concerns about RPC security and the level of coupling between RPC users and wallets. I think the first thing to do is add release notes explaining the exact model, and update the PR description to match.

promag commented at 10:54 am on May 21, 2019: member

IIUC this limits one wallet per user (auth)?

This is incomplete now that there’s dynamic support for wallets:

there should be a way to dynamically update access credentials?
rpcauth.py should be updated?
probably there’s issues with external wallets (paths) and other characters?
already mentioned, needs tests.

Alternatively it could whitelist RPC categories (not wallets) by user/auth: For instance:

0rpcauth=promag:ec94a02...$07e90e0...:blockchain,rawtransactions

DrahtBot added the label Needs rebase on Jun 19, 2019

DrahtBot commented at 0:00 am on June 19, 2019: member

laanwj commented at 11:57 am on September 30, 2019: member

This seems to have been inactive for a long time, and it was controversial in the first place, closing.

laanwj closed this on Sep 30, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet #10615

Conflicts