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

luke-jr · 2017-06-16T20:52:49Z

Simple rebase of current RPC stuff. No endpoints yet.

src/httprpc.cpp

ryanofsky

ACK b77b2f254cc365728790f345deedbed1204964bb.

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/rpc/server.h

ryanofsky · 2017-06-20T13:18:48Z

src/rpc/server.h

+#endif
+
+    JSONRPCRequest() : id(NullUniValue), params(NullUniValue), fHelp(false)
+#ifdef ENABLE_WALLET


In commit "RPC: Pass wallet through JSONRPCRequest"

Could drop this ifdef also.

ryanofsky · 2017-06-20T13:21:51Z

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];


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

Should update -rpcauth documentation to mention the new field.

Also add a test for the new parameter?

ryanofsky

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.

ryanofsky · 2017-06-20T14:16:18Z

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];


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.

jnewbery · 2017-06-28T13:20:35Z

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 Remaining instances of ENABLE_WALLET in libbitcoin_server.a #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.

promag · 2017-08-04T21:45:42Z

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];


Also add a test for the new parameter?

promag · 2017-08-04T21:46:22Z

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)


Just wallet_name?

promag · 2017-08-04T21:46:45Z

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;


wallet_name.

src/httprpc.cpp

…ic wallet

luke-jr · 2018-11-02T05:21:37Z

Rebased

DrahtBot · 2018-11-02T08:43:43Z

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 · 2018-11-02T17:41:57Z

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

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:

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

DrahtBot · 2019-06-19T00:00:51Z

Needs rebase

laanwj · 2019-09-30T11:57:56Z

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

luke-jr force-pushed the multiwallet_rpc branch from 4d6b4c5 to dbbdef9 Compare June 16, 2017 20:54

ryanofsky reviewed Jun 16, 2017

View reviewed changes

src/httprpc.cpp Outdated Show resolved Hide resolved

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

luke-jr changed the title ~~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) Jun 18, 2017

luke-jr force-pushed the multiwallet_rpc branch from dbbdef9 to b77b2f2 Compare June 20, 2017 04:30

ryanofsky reviewed Jun 20, 2017

View reviewed changes

This was referenced Jun 22, 2017

Simple, backwards compatible RPC multiwallet support (superseded by #10829) #10653

Closed

Multiwallet: add RPC endpoint support #10650

Closed

ryanofsky mentioned this pull request Jul 14, 2017

Simple, backwards compatible RPC multiwallet support. #10829

Closed

ryanofsky mentioned this pull request Aug 4, 2017

RPC: Restore backward compatibility, in multiwallet mode #10989

Closed

promag reviewed Aug 4, 2017

View reviewed changes

luke-jr force-pushed the multiwallet_rpc branch 2 times, most recently from 94ecb7a to 6a20988 Compare August 25, 2017 07:39

luke-jr force-pushed the multiwallet_rpc branch from 6a20988 to 370d336 Compare September 2, 2017 08:06

luke-jr mentioned this pull request Sep 21, 2017

Basic Multiwallet GUI support #11383

Closed

TheBlueMatt reviewed Sep 21, 2017

View reviewed changes

src/httprpc.cpp Outdated Show resolved Hide resolved

promag mentioned this pull request Sep 25, 2017

Use shared pointer for wallet instances #11402

Closed

ryanofsky mentioned this pull request Jan 23, 2018

RPC Whitelist Files #12248

Closed

luke-jr force-pushed the multiwallet_rpc branch from 370d336 to eef48ee Compare March 6, 2018 19:19

maflcko added the Needs rebase label Jun 6, 2018

RPC: Allow rpcauth configs to specify a 4th parameter naming a specif…

43b92a9

…ic wallet

luke-jr changed the title ~~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 Nov 2, 2018

luke-jr force-pushed the multiwallet_rpc branch from eef48ee to 43b92a9 Compare November 2, 2018 05:21

DrahtBot removed the Needs rebase label Nov 2, 2018

promag reviewed May 21, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jun 19, 2019

laanwj closed this Sep 30, 2019

laanwj removed the Needs rebase label Oct 24, 2019

jnewbery mentioned this pull request Apr 8, 2020

rpc, cli: add multiwallet balances rpc and use it in -getinfo #18453

Closed

luke-jr mentioned this pull request Sep 3, 2020

Add RPC Whitelist Feature from #12248 #12763

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

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

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

Uh oh!

Conversation

luke-jr commented Jun 16, 2017

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

promag Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 28, 2017

Uh oh!

promag Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

promag Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

promag Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

luke-jr commented Nov 2, 2018

Uh oh!

DrahtBot commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Nov 2, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 19, 2019

Uh oh!

laanwj commented Sep 30, 2019

Uh oh!

Uh oh!

DrahtBot commented Nov 2, 2018 •

edited

Loading