/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
from
+82 −31

Conversation

Reviewers

@TheBlueMatt TheBlueMatt

@promag promag

@ryanofsky ryanofsky

Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
6 participants
@luke-jr @jnewbery @TheBlueMatt @promag @ryanofsky @fanquake
@luke-jr
Member

luke-jr commented Jun 16, 2017

Simple rebase of current RPC stuff. No endpoints yet.
src/httprpc.cpp
@@ -184,6 +191,23 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
// Set the URI
jreq.URI = req->GetURI();
+#ifdef ENABLE_WALLET
+ if (walletName.empty()) {
@ryanofsky

ryanofsky Jun 16, 2017

Contributor

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

luke-jr Jun 19, 2017

Member

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

@ryanofsky

ryanofsky Jun 19, 2017

Contributor

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

promag Aug 4, 2017

Contributor

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

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

@luke-jr luke-jr changed the title from RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet to RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet (multiwallet RPC support) Jun 18, 2017

@ryanofsky

ryanofsky reviewed Jun 20, 2017
View changes

ACK b77b2f2.

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

Tested with:

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

And $HOME/.bitcoin/bitcoin.conf:

rpcauth=user1:51902a7be9c9911079af388a927f$22904ad1bfec659ee1e61d1b3dd73f7b552c6d2d0d1e9f71f6ee833954d062da:w1.dat
rpcauth=user2:51902a7be9c9911079af388a927f$22904ad1bfec659ee1e61d1b3dd73f7b552c6d2d0d1e9f71f6ee833954d062da:w2.dat
rpcauth=user3:51902a7be9c9911079af388a927f$22904ad1bfec659ee1e61d1b3dd73f7b552c6d2d0d1e9f71f6ee833954d062da:-
src/httprpc.cpp
@@ -119,14 +122,17 @@ static bool multiUserAuthorized(std::string strUserPass)
std::string strHashFromPass = HexStr(hexvec);
if (TimingResistantEqual(strHashFromPass, strHash)) {
+ if (vFields.size() > 3) {
+ walletNameOut = vFields[3];
@ryanofsky

ryanofsky Jun 20, 2017

Contributor

In commit "RPC: Allow rpcauth configs to specify..."

Should update -rpcauth documentation to mention the new field.

@promag

promag Aug 4, 2017

Contributor

Also add a test for the new parameter?

src/rpc/server.h
@@ -46,8 +48,15 @@ class JSONRPCRequest
bool fHelp;
std::string URI;
std::string authUser;
-
- JSONRPCRequest() : id(NullUniValue), params(NullUniValue), fHelp(false) {}
+#ifdef ENABLE_WALLET
@ryanofsky

ryanofsky Jun 20, 2017

Contributor

In commit "RPC: Pass wallet through JSONRPCRequest"

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

src/rpc/server.h
+#endif
+
+ JSONRPCRequest() : id(NullUniValue), params(NullUniValue), fHelp(false)
+#ifdef ENABLE_WALLET
@ryanofsky

ryanofsky Jun 20, 2017

Contributor

In commit "RPC: Pass wallet through JSONRPCRequest"

Could drop this ifdef also.

@ryanofsky

ryanofsky reviewed Jun 20, 2017
View changes

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.

src/httprpc.cpp
@@ -119,14 +122,17 @@ static bool multiUserAuthorized(std::string strUserPass)
std::string strHashFromPass = HexStr(hexvec);
if (TimingResistantEqual(strHashFromPass, strHash)) {
+ if (vFields.size() > 3) {
+ walletNameOut = vFields[3];
@ryanofsky

ryanofsky Jun 20, 2017

Contributor

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:

rpcauth=user:salt:hash:filename.dat

You would write:

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

This would be more extensible, also more readable.

This was referenced Jun 22, 2017

Closed
Closed
@jnewbery
Member

jnewbery commented Jun 28, 2017

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.

This was referenced Jul 14, 2017

Closed
Closed
src/httprpc.cpp
@@ -119,14 +122,17 @@ static bool multiUserAuthorized(std::string strUserPass)
std::string strHashFromPass = HexStr(hexvec);
if (TimingResistantEqual(strHashFromPass, strHash)) {
+ if (vFields.size() > 3) {
+ walletNameOut = vFields[3];
@promag

promag Aug 4, 2017

Contributor

Also add a test for the new parameter?

src/httprpc.cpp
return true;
}
}
}
return false;
}
-static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUsernameOut)
+static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUsernameOut, std::string& walletNameOut)
@promag

promag Aug 4, 2017

Contributor

Just wallet_name?

src/httprpc.cpp
@@ -162,7 +168,8 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
}
JSONRPCRequest jreq;
- if (!RPCAuthorized(authHeader.second, jreq.authUser)) {
+ std::string walletName;
@promag

promag Aug 4, 2017

Contributor

wallet_name.

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

promag Aug 4, 2017

Contributor

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

@luke-jr luke-jr referenced this pull request Sep 21, 2017

Open

Basic Multiwallet GUI support #11383

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

TheBlueMatt Sep 21, 2017

Contributor

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

@luke-jr

luke-jr Sep 21, 2017

Member

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

TheBlueMatt Sep 22, 2017

Contributor

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

luke-jr Sep 22, 2017

Member

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

TheBlueMatt Sep 26, 2017

Contributor

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.

@promag promag referenced this pull request Sep 25, 2017

Open

[wallet] Use shared pointer for wallet instances #11402

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment