Add RPC Whitelist Feature from #12248 #12763

pull JeremyRubin wants to merge 2 commits into bitcoin:master from JeremyRubin:whitelistrpc changing 4 files +162 −3
  1. JeremyRubin commented at 4:42 am on March 23, 2018: contributor

    Summary

    This patch adds the RPC whitelisting feature requested in #12248. RPC Whitelists help enforce application policies for services being built on top of Bitcoin Core (e.g., your Lightning Node maybe shouldn’t be adding new peers). The aim of this PR is not to make it advisable to connect your Bitcoin node to arbitrary services, but to reduce risk and prevent unintended access.

    Using RPC Whitelists

    The way it works is you specify (in your bitcoin.conf) configurations such as

    0rpcauth=user1:4cc74397d6e9972e5ee7671fd241$11849357f26a5be7809c68a032bc2b16ab5dcf6348ef3ed1cf30dae47b8bcc71
1rpcauth=user2:181b4a25317bff60f3749adee7d6bca0$d9c331474f1322975fa170a2ffbcb176ba11644211746b27c1d317f265dd4ada
2rpcauth=user3:a6c8a511b53b1edcf69c36984985e$13cfba0e626db19061c9d61fa58e712d0319c11db97ad845fa84517f454f6675
3rpcwhitelist=user1:getnetworkinfo
4rpcwhitelist=user2:getnetworkinfo,getwalletinfo, getbestblockhash
5rpcwhitelistdefault=0

    Now user1 can only call getnetworkinfo, user2 can only call getnetworkinfo or getwalletinfo, while user3 can still call all RPCs.

    If any rpcwhitelist is set, act as if all users are subject to whitelists unless rpcwhitelistdefault is set to 0. If rpcwhitelistdefault is set to 1 and no rpcwhitelist is set, act as if all users are subject to whitelists.

    Review Request

    In addition to normal review, would love specific review from someone working on LN (e.g., @ roasbeef) and someone working on an infrastructure team at an exchange (e.g., @ jimpo) to check that this works well with their system.

    Notes

    The rpc list is spelling sensitive – whitespace is stripped though. Spelling errors fail towards the RPC call being blocked, which is safer.

    It was unclear to me if HTTPReq_JSONRPC is the best function to patch this functionality into, or if it would be better to place it in exec or somewhere else.

    It was also unclear to me if it would be preferred to cache the whitelists on startup or parse them on every RPC as is done with multiUserAuthorized. I opted for the cached approach as I thought it was a bit cleaner.

    Future Work

    In a future PR, I would like to add an inheritance scheme. This seemed more controversial so I didn’t want to include that here. Inheritance semantics are tricky, but it would also make these whitelists easier to read.

    It also might be good to add a getrpcwhitelist command to facilitate permission discovery.

    Tests

    Thanks to @ emilengler for adding tests for this feature. The tests cover all cases except for where rpcwhitelistdefault=1 is used, given difficulties around testing with the current test framework.

  2. fanquake added the label RPC/REST/ZMQ on Mar 23, 2018
  3. in src/httprpc.cpp:254 in 788b174c33 outdated 
    247@@ -223,6 +248,13 @@ static bool InitRPCAuthentication()
248         LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcuser for rpcauth auth generation.\n");
249         strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
250     }
251+
252+    for (const std::string& strRPCWhitelist : gArgs.GetArgs("-rpcwhitelist")) {
253+        std::string strUser = strRPCWhitelist.substr(0, strRPCWhitelist.find(':'));
254+        std::string strWhitelist = strRPCWhitelist.substr(strRPCWhitelist.find(':') + 1);
    kallewoof commented at 8:29 am on March 23, 2018:
    This behavior may be fine, but this will silently turn "foo" into strUser == strWhitelist == "foo".
    promag commented at 3:56 pm on March 23, 2018:
    Agree, it should check for correct format.
  4. kallewoof commented at 8:30 am on March 23, 2018: member
    utACK 788b174c33896065065d1ab376d6451b0f7575cd
  5. NicolasDorier commented at 9:36 am on March 23, 2018: contributor
    Concept ACK. This is very welcome. My block explorer only rely on sendrawtransaction. Would like a way to have my block explorer restrict itself at runtime to make the life of my users easier, but this might come later.
  6. instagibbs commented at 12:58 pm on March 23, 2018: member
    concept ACK
  7. instagibbs commented at 12:59 pm on March 23, 2018: member
    @RHavar you may be interested?
  8. randolf commented at 3:08 pm on March 23, 2018: contributor
    Concept ACK.
  9. RHavar commented at 3:21 pm on March 23, 2018: contributor

    @instagibbs To be honest, I don’t see it being that useful for me (but I don’t have any objections to it either)

    The main advantage I can see from this change is that you could use the same bitcoin-core instance for multiple purposes, like where one only requires access to the bitcoin-related features, and the other might need wallet access.

    That’s probably more interesting for consumer-level users, who are running core on their own computer. For more commercial users, we’d just use different instances of bitcoin-core itself.

    –

    The PRC permission feature I’m more interested in is a lot more high-level; like applying spending limits. What I would like to do is be able to store say 300 BTC in my wallet, but never allow it drop below 250 BTC (by a particular RPC user). This way I could have only 50 BTC of “risk” (e.g. in case my service was hacked) but could benefit from having 300 BTC worth of coins (so coin-selection can do a much better job)

  10. in src/httprpc.cpp:218 in 788b174c33 outdated 
    202-        } else if (valRequest.isArray())
203+        } else if (valRequest.isArray()) {
204+            if (whitelistedRPC.count(jreq.authUser)) {
205+                for (unsigned int reqIdx = 0; reqIdx < valRequest.size(); reqIdx++) {
206+                    if (!valRequest[reqIdx].isObject()) {
207+                        throw JSONRPCError(RPC_INVALID_REQUEST, "Invalid Request object");
    promag commented at 3:47 pm on March 23, 2018:
    New error, should have a test.
  11. instagibbs commented at 3:48 pm on March 23, 2018: member
    @RHavar this prevents privkey dumps, as a first step at least
  12. in src/httprpc.cpp:67 in 788b174c33 outdated 
    62@@ -63,6 +63,8 @@ class HTTPRPCTimerInterface : public RPCTimerInterface
63 static std::string strRPCUserColonPass;
64 /* Stored RPC timer interface (for unregistration) */
65 static std::unique_ptr<HTTPRPCTimerInterface> httpRPCTimerInterface;
66+/* RPC Auth Whitelist */
67+static std::map<std::string, std::set<std::string>> whitelistedRPC;
    promag commented at 3:56 pm on March 23, 2018:
    rpc_whitelist?
    MarcoFalke commented at 7:18 pm on December 11, 2019:
    style-nit in commit b411ae6500: g_rpc_whitelist, because it is a global
  13. promag commented at 3:58 pm on March 23, 2018: member

    Concept ACK. Indeed a useful feature where a daemon can be shared for multiple purposes.

    rpcwhitelist=user2:getnetworkinfo,getwalletinfo

    Is this possible?

    Doing -rpcwhitelist=user2:foo -rpcwhitelist=user2:bar will end with user2: ['bar']. Maybe it should merge or maybe we should not allow multiple methods in the same argument?

    This PR could include some tests and update code style as per developer notes.

  14. jonasschnelli commented at 1:41 pm on March 25, 2018: contributor
    Concept ACK
  15. in src/httprpc.h:10 in 788b174c33 outdated 
     6@@ -7,6 +7,7 @@
 7 
 8 #include <string>
 9 #include <map>
10+#include <set>
    jonasschnelli commented at 1:50 pm on March 25, 2018:
    Why has that to be here?
    JeremyRubin commented at 10:50 pm on March 26, 2018:
    Wasn’t sure, was just trying to follow local style. Not sure that string or map need to be there either?
    promag commented at 11:24 pm on March 26, 2018:
    None needs to be there. If you want to remove, do it in a different commit.
  16. in src/httprpc.cpp:192 in 788b174c33 outdated 
    185@@ -184,15 +186,38 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
186         // singleton request
187         if (valRequest.isObject()) {
188             jreq.parse(valRequest);
189-
190+            if (whitelistedRPC.count(jreq.authUser) && !whitelistedRPC[jreq.authUser].count(jreq.strMethod)) {
191+                LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, jreq.strMethod);
192+                req->WriteHeader("WWW-Authenticate", WWW_AUTH_HEADER_DATA);
193+                req->WriteReply(HTTP_UNAUTHORIZED);
    jimpo commented at 3:00 am on March 26, 2018:
    I think the 403 Forbidden status is more appropriate. See https://stackoverflow.com/a/6937030.
  17. jimpo commented at 3:11 am on March 26, 2018: contributor
    Concept ACK. I can’t imagine this being a problem for exchanges, given that it’s backwards compatible and uses standard bitcoind configuration logic. Definitely fine for Coinbase.
  18. in src/httprpc.cpp:274 in 86b143fad6 outdated 
    247@@ -223,6 +248,17 @@ static bool InitRPCAuthentication()
248         LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcuser for rpcauth auth generation.\n");
249         strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
250     }
251+
252+    for (const std::string& strRPCWhitelist : gArgs.GetArgs("-rpcwhitelist")) {
253+        auto pos = strRPCWhitelist.find(':');
254+        std::string strUser = strRPCWhitelist.substr(0, pos);
255+        std::set<std::string>& whitelist = rpc_whitelist[strUser];
256+        if (pos != std::string::npos) {
    kallewoof commented at 2:29 am on March 27, 2018:
    Should this maybe error on == case? It now silently ignores nocolonvalues.
    JeremyRubin commented at 8:01 pm on March 27, 2018:
    It actually doesn’t silently ignore it – it sets an empty whitelist.
  19. kallewoof commented at 2:30 am on March 27, 2018: member
    utACK
  20. in src/httprpc.cpp:258 in 86b143fad6 outdated 
    253+        auto pos = strRPCWhitelist.find(':');
254+        std::string strUser = strRPCWhitelist.substr(0, pos);
255+        std::set<std::string>& whitelist = rpc_whitelist[strUser];
256+        if (pos != std::string::npos) {
257+            std::string strWhitelist = strRPCWhitelist.substr(pos + 1);
258+            boost::split(whitelist, strWhitelist, boost::is_any_of(","));
    eklitzke commented at 6:57 am on March 27, 2018:
    You can split with a std::istream or std::getline, no need for boost::split here.
    JeremyRubin commented at 8:05 pm on March 27, 2018:

    I’d prefer to leave boost::split – this is an existing dependency throughout the codebase, at some point someone will likely clean them all up consistently with an addition to util.

    I’m happy to make such a PR if there is demand for it.

  21. eklitzke commented at 7:08 am on March 27, 2018: contributor

    I support adding ACLs and all that, so concept ACK.

    The implementation here is problematic though. In a whitelisting approach everything should be disabled by default. This does the opposite: in this PR users have access to all RPCs by default. Adding a whitelist policy for a user implicitly restricts access to other ACLs, which is very confusing. If I am $COMPANY and I add a new engineer to the team, it is going to be very bad if I create an rpcauth for the user without remembering to also set up their ACLs. The default behavior in this situation should be safe, i.e. deny by default if any policy is defined.

    There’s a huge range here from really complex authorization schemes to really simple things, but I would prefer something that has at least basic steps towards a real roles/ACL system. At the minimum I think this means there should be a way to declare a default list of ACLs, and then whitelists would expand on that.

  22. kallewoof commented at 3:09 pm on March 27, 2018: member
    Yeah it’s probably better to check if size() > 0 and to do default-no-access if so.
  23. JeremyRubin commented at 8:27 pm on March 27, 2018: contributor

    @eklitzke

    In the current scheme, a user by default has access to all RPCs to maintain backwards compatibility.

    Once a file has specified that a user should have a whitelist, it defaults to everything being off.

    I agree this is not ideal, but don’t have a great workaround.

    Here are two potential solutions:

    We add: -rpcwhitelistenable=<rpc 1>,<rpc 2> -rpcwhitelistroot=

    If rpcwhitelistenable is set, the by default any user has that whitelist (allowed to be empty)? If a user is marked rpcwhitelistroot, they have all RPCs enabled. If a user is marked rpcwhitelist=:blah, then they have blah.

    Or, we can make it such that if any rpcwhitelist is set, all users default to having an empty whitelist.

    Do you think this is a good tradeoff of complexity/dtrt? Which do you prefer? @promag The other detail (in a forthcoming patch) is that if rpcwhitelist is set multiple times for a single user, it should do the intersection of the specification (e.g., monotonically smaller whitelist) rather than the union. In cases where conflicting settings have been passed, it is safer to do less.

  24. sipa commented at 8:48 pm on March 27, 2018: member 
    0
1EDIT: nevermind, that's a concern for blacklists, not whitelists.
  25. eklitzke commented at 3:01 am on March 28, 2018: contributor

    @JeremyRubin I would add both. I guess my point is that if you need permissions at all you probably also want the ability to enable some kind of deny-by-default policy, to safeguard against a situation where you accidentally forget to lock down an account. You generally don’t want to give your software engineers root access on production machines by default, and by the same token I don’t think you would want to give people with bitcoind access root by default.

    The way I imagine a typical setup would be something like this (ignore the made up syntax):

     0# Default permissions are for a bunch of read only rpcs
 1rpcallowed = getblock,getblockchaininfo
 2
 3# Alice has the default permissions, plus stuff to admin the network
 4rpcallowed.alice = addnode,clearbanned,listbanned
 5
 6# Bob has the default permissions, plus some basic access to the wallet
 7rpcallowed.bob = getbalance,getwalletinfo,getnewaddress
 8
 9# Carol is an admin, she can access everything
10rpcallowed.carol = !all

    And semantics something like this:

    • If there are no rpcallowed lines at all then everyone can access everything, just like how bitcoin works today
    • If you set rpcallowed.alice (Alice’s list) but forget to set rpcallowed (the default list) then it should deny by default (maybe with a special error message attached to the RPC warning about the bad configuration)

    Admittedly there is an area ripe for feature creep, but I think the above would be relatively simple to implement and good enough for a lot of cases.

  26. eklitzke commented at 3:20 am on March 28, 2018: contributor

    Second idea that’s much more crazy/complex. I’ll throw it out there though.

    YAML documents have a syntax to reference other elements, which are really useful for these kinds of things. You can construct a few objects in your config and reference them elsewhere, which allows you to come up with some pseduo-role system:

     0---
 1policies:
 2  # a default policy
 3  - policy: &default
 4      - getblock
 5      - getblockchaininfo
 6      - getblockcount
 7
 8  # network admin policy
 9  - policy: &networkadmin
10      - addnode
11      - clearbanned
12      - getpeerinfo
13
14users:
15  # alice has default plus network admin
16  - name: alice
17    policies:
18      - *default
19      - *networkadmin
20
21  # bob just has default policies
22  - name: bob
23    policies:
24      - *default
25
26  # syntax idea 1 for full access
27  - name: carol
28    admin: true
29
30  # syntax idea 2 for full access, !all is implicitly defined
31  - name: dave
32    policies: [*all]

    The &obj syntax introduces a name for a reference, and then *obj means “substitute the literal value for obj here”. I’m not sure how it works in C++ YAML libraries, but the ones I’ve used in Python do the reference expansion within the YAML library. This makes it so your code can just work with dumb lists of objects, since they never see the object references.

    The obvious drawback is that this would require linking against a YAML library. You could mitigate that by only having the user ACL list be in a YAML lib, so only users who actually want these feature would need to enable the YAML parser.

  27. JeremyRubin commented at 7:12 pm on March 30, 2018: contributor

    @eklitzke did you check out the issue? The original proposal covered doing an inheritance scheme. @gmaxwell suggested that we should avoid doing any fancy config file, in favor of just a simple list.

    I do think that this could get overly verbose (especially if you want to grant network multiple times), but in general the paradigm should be to configure applications to manage multiple small credentials for specific purposes rather than one full-grant.

  28. JeremyRubin force-pushed on Apr 13, 2018
  29. JeremyRubin commented at 1:48 am on April 13, 2018: contributor

    I’ve updated this PR and rebased.

    The current version has the following changes over the previous:

    • Strip whitespace out of rpc list
    • Set-Intersect conflicting whitelists (i.e., so it is equivalent to checking multiple whitelists)
    • If any rpcwhitelist is set, act as if all users are subject to whitelists unless rpcwhitelistdefault is set to 0. If rpcwhitelistdefault is set to 1 and no rpcwhitelist is set, act as if all users are subject to whitelists.

    Please let me know if these semantics are acceptable.

  30. kallewoof commented at 6:14 am on April 13, 2018: member
    LGTM. Checked code, utACK.
  31. jimpo commented at 2:38 am on May 3, 2018: contributor
    I’m not really a fan of the rpcwhitelistdefault flag. I agree with the decision to provide backwards compatibility in the API by whitelisting everyone for everything if -rpcwhitelist is not set, but if it is, I think whitelists should be more explicit. Perhaps instead -rpcwhitelist=USER would whitelist USER for everything, whereas -rpcwhitelist=USER:ACTION1,ACTION2 would whitelist a user for specific actions.
  32. JeremyRubin commented at 6:08 am on May 7, 2018: contributor

    -rpcwhitelist=USER is currently a null list, and i think should remain that.

    We could introduce a new flag, e.g. rpcwhitelistallowall if that’s the desired ability.

  33. laanwj commented at 1:28 pm on May 30, 2018: member

    I’ve always been strongly against complex authorization schemes in bitcoind. A lot of extra security critical logic to maintain. I think the place for such things is a separate authorization proxy. This is more modular, more in line with “software should do one thing well”, and it depends on what security system is required, nothing can accommodate every possible separation of privileges (see also: linux security modules).

    That said, I like the simplicity of this scheme, simply specifying the calls that are allowed, and the small code impact, so Concept ACK.

  34. promag commented at 1:34 pm on May 30, 2018: member

    The other detail (in a forthcoming patch) is that if rpcwhitelist is set multiple times for a single user, it should do the intersection of the specification (e.g., monotonically smaller whitelist) rather than the union. In cases where conflicting settings have been passed, it is safer to do less. @JeremyRubin lgtm.

  35. JeremyRubin force-pushed on Jun 12, 2018
  36. JeremyRubin commented at 6:27 am on June 12, 2018: contributor

    Rebased and added documentation.

    Started working on tests but got a little stuck with the current test framework – the test classes assume access to the RPCs, which doesn’t let me cover all of the modes of use for rpc whitelists.

  37. JeremyRubin closed this on Jun 18, 2018
  38. JeremyRubin reopened this on Jun 18, 2018
  39. gmaxwell commented at 10:33 pm on June 18, 2018: contributor

    @RHavar The thing you’re interested in would almost just fit into this if there were RPCs to set spending velocity limits on wallets, and then just use this PR to deny the relevant clients from calling the interface to increase their limits.

    The only gap I see with that is that is that it would be a per wallet not per rpc user limit. In any case, having wallet velocity limits even that you can just bypass by calling a function would go a long way to reducing mistake surface.

  40. JeremyRubin commented at 11:00 pm on June 27, 2018: contributor

    Tagging @jnewbery for tips on best way to use the test framework here. The issue is that in order to test whitelists properly, I need to disable RPC access…

    Should I just write tests as a unit test (non-functional test)?

  41. MarcoFalke commented at 11:50 am on June 28, 2018: member

    If any rpcwhitelist is set, act as if all users are subject to whitelists unless rpcwhitelistdefault is set to 0. If rpcwhitelistdefault is set to 1 and no rpcwhitelist is set, act as if all users are subject to whitelists.

    Please update OP to reflect that.

    Should I just write tests as a unit test (non-functional test)?

    With regard to tests, I believe it is trivial to test the following [c.f. Attachment 1] in our existing functional tests:

    • No whitelist specified at all: Tested by all existing tests
    • A user withelisted
    • A user blocked because other users are whitelisted

    Am I missing a test case?

     0diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py
 1index 1ef59da5ad..864cd5977c 100755
 2--- a/test/functional/rpc_users.py
 3+++ b/test/functional/rpc_users.py
 4@@ -24,15 +24,30 @@ class HTTPBasicsTest(BitcoinTestFramework):
 5     def set_test_params(self):
 6         self.num_nodes = 2
 7 
 8+    def setup_network(self):
 9+        self.setup_nodes()
10+
11     def setup_chain(self):
12         super().setup_chain()
13         #Append rpcauth to bitcoin.conf before initialization
14+        whitelist = "rpcwhitelist=__cookie__:getblockcount,stop,getbestblockhash\n"
15+
16         rpcauth = "rpcauth=rt:93648e835a54c573682c2eb19f882535$7681e9c5b74bdd85e78166031d2058e1069b3ed7ed967c93fc63abba06f31144"
17+        whitelist += "rpcwhitelist=rt:getbestblockhash\n"
18+
19         rpcauth2 = "rpcauth=rt2:f8607b1a88861fac29dfccf9b52ff9f$ff36a0c23c8c62b4846112e50fa888416e94c17bfd4c42f88fd8f55ec6a3137e"
20+        whitelist += "rpcwhitelist=rt2:getbestblockhash\n"
21+
22+        rpcauth_blocked = "rpcauth=blocked:e66047993b1784b4dc4dddbccf9c297$fb62cda6ae6a31edf42d3d4b639a0d965e9bcea49bf1c2d61d423f373891daa1"
23+        # not whitelisted
24+
25         rpcuser = "rpcuser=rpcuser💻"
26+        whitelist += "rpcwhitelist=rpcuser💻:getbestblockhash\n"
27+
28         rpcpassword = "rpcpassword=rpcpassword🔑"
29 
30         self.user = ''.join(SystemRandom().choice(string.ascii_letters + string.digits) for _ in range(10))
31+        whitelist += "rpcwhitelist={}:getbestblockhash\n".format(self.user)
32         config = configparser.ConfigParser()
33         config.read_file(open(self.options.configfile))
34         gen_rpcauth = config['environment']['RPCAUTH']
35@@ -43,7 +58,9 @@ class HTTPBasicsTest(BitcoinTestFramework):
36 
37         with open(os.path.join(get_datadir_path(self.options.tmpdir, 0), "bitcoin.conf"), 'a', encoding='utf8') as f:
38             f.write(rpcauth+"\n")
39+            f.write(whitelist)
40             f.write(rpcauth2+"\n")
41+            f.write(rpcauth_blocked+"\n")
42             f.write(rpcauth3+"\n")
43         with open(os.path.join(get_datadir_path(self.options.tmpdir, 1), "bitcoin.conf"), 'a', encoding='utf8') as f:
44             f.write(rpcuser+"\n")
45@@ -66,6 +83,9 @@ class HTTPBasicsTest(BitcoinTestFramework):
46         password2 = "8/F3uMDw4KSEbw96U3CA1C4X05dkHDN2BPFjTgZW4KI="
47         authpairnew = "rt:"+password
48 
49+        #Third authpair (not whitelisted)
50+        password_blocked = "qpd5D9Y1mSavkBlIomhXlG4cqcRLveJvhyANVSxtk70="
51+
52         self.log.info('Correct...')
53         headers = {"Authorization": "Basic " + str_to_b64str(authpair)}
54 
55@@ -123,6 +143,29 @@ class HTTPBasicsTest(BitcoinTestFramework):
56         assert_equal(resp.status, 200)
57         conn.close()
58 
59+        self.log.info('rt2 is not whitelisted on this specific rpc...')
60+        authpairnew = "rt2:"+password2
61+        headers = {"Authorization": "Basic " + str_to_b64str(authpairnew)}
62+
63+        conn = http.client.HTTPConnection(url.hostname, url.port)
64+        conn.connect()
65+        conn.request('POST', '/', '{"method": "getnetworkinfo"}', headers)
66+        resp = conn.getresponse()
67+        assert_equal(resp.status, 403)
68+        conn.close()
69+
70+
71+        self.log.info('Calling an rpc from a user with no whitelist...')
72+        authpairnew = "blocked:" + password_blocked
73+        headers = {"Authorization": "Basic " + str_to_b64str(authpairnew)}
74+
75+        conn = http.client.HTTPConnection(url.hostname, url.port)
76+        conn.connect()
77+        conn.request('POST', '/', '{"method": "getbestblockhash"}', headers)
78+        resp = conn.getresponse()
79+        assert_equal(resp.status, 403)
80+        conn.close()
81+
82         #Wrong password for rt2
83         self.log.info('Wrong...')
84         authpairnew = "rt2:"+password2+"wrong"
  42. jnewbery commented at 1:05 pm on June 28, 2018: member
    @MarcoFalke’s test looks good. Will review fully once it’s been included in this PR.
  43. JeremyRubin commented at 7:04 pm on June 28, 2018: contributor

    Those tests look fine, and are close to where I got stuck.

    Cases missing

    • Easy to Add:

      • Multiple RPC Whitelists specified, should intersect

    • Difficult:

      • No whitelist specified for any user, rpcwhitelistdefault=1
  44. MarcoFalke commented at 11:16 am on June 30, 2018: member

    Difficult: No whitelist specified for any user, rpcwhitelistdefault=1

    Indeed not trivial. Options I see is to rework the test framework to fall back to rest if rpc is not available or, as you mentioned, write a unit test.

  45. DrahtBot added the label Needs rebase on Jul 10, 2018
  46. DrahtBot commented at 4:17 pm on July 10, 2018: member
  47. MarcoFalke commented at 2:43 pm on July 11, 2018: member
    Merge conflict can probably be solved by a simple union merge.
  48. MarcoFalke commented at 11:11 pm on November 8, 2018: member
    @JeremyRubin Are you still working on this?
  49. MarcoFalke added the label Up for grabs on Nov 8, 2018
  50. fanquake commented at 9:24 am on March 2, 2019: member
    Closing, leaving Up for grabs.
  51. fanquake closed this on Mar 2, 2019
  52. laanwj removed the label Needs rebase on Oct 24, 2019
  53. JeremyRubin commented at 6:25 pm on November 18, 2019: contributor

    Rebased – old version exists at https://github.com/JeremyRubin/bitcoin/tree/whitelistrpc-backup.

    Not sure if github tracks rebases for closed prs…

  54. ryanofsky commented at 6:35 pm on November 18, 2019: member

    Not sure if github tracks rebases for closed prs…

    IIRC after a PR is closed, the diff is no longer updated, and just will just show the changes at the time the PR was closed, no longer tracking the branch.

    Also, if you push to the branch after closing the PR, reopening the PR is disabled until you revert back to the closing commit. (So in case you need to reopen this PR, you need to temporarily force push 8c45d93b0e7187dba81e7d493bdec8f6589fbf53 to the whitelistrpc branch, then reopen the PR, and then update the branch again)

  55. JeremyRubin commented at 6:44 pm on November 18, 2019: contributor
    That’s kinda annoying because I don’t think I can (as a non-owner or whatever) re-open a closed PR?
  56. ryanofsky commented at 6:56 pm on November 18, 2019: member
    I believe you can reopen your own PRs as long as the branch tip doesn’t change or as long as you force push back to the previous branch tip. You can also request maintainer to reopen. But even maintainers can’t reopen a PR if the branch tip isn’t the same as when it was closed. At least this is my understanding.
  57. JeremyRubin commented at 6:59 pm on November 18, 2019: contributor
    hmm that’s not true here right now; it’s on 8c45d93b0e7187dba81e7d493bdec8f6589fbf53 but I can’t re-open. Maybe it’s too old?
  58. ryanofsky reopened this on Nov 18, 2019
  59. ryanofsky commented at 7:04 pm on November 18, 2019: member
    Not sure, but I just noticed the reopen button was enabled for me so I clicked it. You could be able to close this if this was just a theoretical conversation, and you don’t actually want it reopened :smiley:
  60. JeremyRubin force-pushed on Nov 18, 2019
  61. fanquake removed the label Up for grabs on Nov 18, 2019
  62. DrahtBot commented at 1:15 am on November 19, 2019: 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:

    • #16698 (Mempool: rework rebroadcast logic to improve privacy by amitiuttarwar)
    • #16673 (Relog configuration args on debug.log rotation by LarryRuane)
    • #16115 (On bitcoind startup, write config args to debug.log by LarryRuane)

    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.

  63. emilengler commented at 4:36 pm on November 20, 2019: contributor
    @JeremyRubin I’ve written a test #17536 Hope I did it correctly on with git, my first time I do a PR based on another :)
  64. in src/httprpc.h:10 in e49f644f96 outdated 
     4@@ -5,9 +5,6 @@
 5 #ifndef BITCOIN_HTTPRPC_H
 6 #define BITCOIN_HTTPRPC_H
 7 
 8-#include <string>
 9-#include <map>
10-#include <set>
    MarcoFalke commented at 7:17 pm on December 11, 2019:

    In commit e49f644f965dac09f84a7c9b0bc2004add73783d:

    Could squash?

  65. in src/httprpc.cpp:218 in 320947281b outdated 
    212@@ -214,8 +213,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
213                         std::string strMethod = find_value(request, "method").get_str();
214                         if (!whitelistedRPC[jreq.authUser].count(strMethod)) {
215                             LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, strMethod);
216-                            req->WriteHeader("WWW-Authenticate", WWW_AUTH_HEADER_DATA);
217-                            req->WriteReply(HTTP_UNAUTHORIZED);
    MarcoFalke commented at 7:20 pm on December 11, 2019:

    in commit 320947281b:

    Could squash?

  66. in src/httprpc.cpp:71 in afe5f61a9d outdated 
    67@@ -68,7 +68,7 @@ static std::string strRPCUserColonPass;
68 /* Stored RPC timer interface (for unregistration) */
69 static std::unique_ptr<HTTPRPCTimerInterface> httpRPCTimerInterface;
70 /* RPC Auth Whitelist */
71-static std::map<std::string, std::set<std::string>> whitelistedRPC;
    MarcoFalke commented at 7:24 pm on December 11, 2019:

    in commit afe5f61a9d:

    could squash?

  67. in src/init.cpp:538 in 89d9922ce7 outdated 
    533@@ -534,8 +534,8 @@ void SetupServerArgs()
534     gArgs.AddArg("-rpcservertimeout=<n>", strprintf("Timeout during HTTP requests (default: %d)", DEFAULT_HTTP_SERVER_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
535     gArgs.AddArg("-rpcthreads=<n>", strprintf("Set the number of threads to service RPC calls (default: %d)", DEFAULT_HTTP_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
536     gArgs.AddArg("-rpcuser=<user>", "Username for JSON-RPC connections", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
537-    gArgs.AddArg("-rpcwhitelist=<whitelist>", "Set a whitelist to filter incoming RPC calls for a specific user. The field <whitelist> comes in the format: <USERNAME>:<rpc 1>,<rpc 2>,...,<rpc n>. If multiple whitelists are set for a given user, they are set-intersected. See -rpcwhitelistdefault documentation for information on default whitelist behavior.", false, OptionsCategory::RPC);
538-    gArgs.AddArg("-rpcwhitelistdefault", "Sets default behavior for rpc whitelisting. Unless rpcwhitelistdefault is set to 0, if any -rpcwhitelist is set, the rpc server acts as if all rpc users are subject to empty-unless-otherwise-specified whitelists. If rpcwhitelistdefault is set to 1 and no -rpcwhitelist is set, rpc server acts as if all rpc users are subject to empty whitelists.", false, OptionsCategory::RPC);
    MarcoFalke commented at 7:39 pm on December 11, 2019:

    in commit 89d9922ce7:

    Could squash?

  68. MarcoFalke approved
  69. MarcoFalke commented at 7:40 pm on December 11, 2019: member

    ACK 89d9922ce7, could squash some of the commits 🎠

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 89d9922ce7, could squash some of the commits 🎠
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjI7gv+J/mVjsR4q8r8nIVmIKo737Xq0NNmgC58fh/Gdw3Mbvwh8t+8aC24TVCM
 8N0Zkmn8HtIsXQVQbx8/BVJDaUt6D2mJ/LYcy6XOAx7rwWlDlM5TPyCQ5nNlQGtaz
 97xUvazxHkdE9wbB+aUDGmnUTS4AAS+pY6sEX7FWvAq42vSaoRLj8jS8xIsdRrwUh
10Tu1ezvsyoQFbEp4vc3btFUaxpZcXCXDmOxGM3YDHt3qsKmGr9LwF4D83ZEOZ1vBt
11/cO+JTYioaLIGPqREXet01aSJnBoSTgu8gwvWlEGd3kWqChD4nycx1iSpL/Blnx/
12VbS+6wtdU9lWax8cJqtHocrJCvWrrPZfc2BD/1GaOGm0z0QCkmerOYJmo65Af+zn
13iMZ7GMx+f6CyCclzMM2SzByrktyNgOChsEL+mJ+ag0qdDOOAHTx/IlBqk4TCztOx
14+va0kMhgt0zq2TkePTuL9kGB2hX8kQvhh3Dq6X3z9AF2EELeY022xt1TeRn/VUp2
15f339FZxm
16=4Uvy
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 10b6cda480eee9ab8ccc67b28f8ab41d6fc050f6ba594d5fedee976626aa348a -

  70. Add RPC Whitelist Feature from #12248 7414d3820c
  71. JeremyRubin commented at 8:37 pm on December 11, 2019: contributor

    Here’s a squashed branch. I just squashed everything down; there was no need for any of the intermediate commits IMO.

    https://github.com/JeremyRubin/bitcoin/tree/whitelistrpc-squash-square

    In the branch below, I left the g_ nit un-squashed so as to make it easier to compare the hash to what you already ACKed:

    https://github.com/JeremyRubin/bitcoin/tree/whitelistrpc-squash

    Let me know your preference of opening a new PR or force-pushing.

  72. MarcoFalke commented at 8:40 pm on December 11, 2019: member
    Force push is just fine
  73. JeremyRubin force-pushed on Dec 11, 2019
  74. JeremyRubin commented at 9:15 pm on December 11, 2019: contributor
    Done :+1:
  75. MarcoFalke commented at 9:18 pm on December 11, 2019: member

    re-ACK 7414d3820c833566b4f48c6c120a18bf53978c55 only change is adding g_ prefix and squash 🌦

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 7414d3820c833566b4f48c6c120a18bf53978c55 only change is adding g_ prefix and squash 🌦
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUicqgv8CxETZpFF7NDO9EYPqAd2XbtMlKyIA3lXSXFMJZAkd7ZVhHnpXm005RfC
 86RzI8iks7Fhas2pNd26z7Jhjqm9Qzb9HbdH2YoxzV83++hvwWvdvUJnY9kHm89Y0
 9bRG/bXw3vUZkJRp+gq1Z+ZyNkkiePTARecIbcpI1Hb1ibVNagXEowjbgcY+lVbbj
10qbmF/ZDxUaA3ztOlBHQ/e99hine9ibwfpjKlgMiHb4IukwWGiui86y06UJqHYFAL
11O1EaoybQlVJ/r3rnT96zgwcGHQ7vlOD/02USzLRa/F3XrSQvrPiMlNn4UaaHXMbK
12lGlAQ58YEx7GKM80S1cBJOPnbBrIz1TGtilv7vFyLcgZrc8PfFKKXhvzxem4DFsl
13JYoaP2T46ad53TaRCiUQvAZa0EM+2srqBE/KibwOYVJQEOgSEBOYMagsKFIE6bnl
14Xed01A9z/5599ZeXlCbra8gBFEstHn6OZEJ8WxcBkc2lbU1Hnn29OB/jArFHTPg4
15B5i7SBMi
16=t/W5
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 9efdd13b36d3827d0d676b35f29a2117adc6c8e29b7a726131c675cb666639aa -

  76. test: Add test for rpc_whitelist 2081442c42
  77. laanwj commented at 10:27 am on December 13, 2019: member
    ACK 2081442c421cc4376e5d7839f68fbe7630e89103
  78. laanwj referenced this in commit 988fe5b1ad on Dec 13, 2019
  79. laanwj merged this on Dec 13, 2019
  80. laanwj closed this on Dec 13, 2019
  81. laanwj added the label Feature on Dec 13, 2019
  82. laanwj added the label Needs release note on Dec 13, 2019
  83. laanwj added this to the milestone 0.20.0 on Dec 13, 2019
  84. JeremyRubin deleted the branch on Dec 13, 2019
  85. sidhujag referenced this in commit ccff300b3c on Dec 13, 2019
  86. practicalswift commented at 5:43 pm on December 14, 2019: contributor

    Users arriving at this PR looking to use -rpcwhitelist to restrict RPC access for non-trusted RPC users should be aware of this gotcha (due to UniValue CVE-2019-18936):

     0$ share/rpcauth/rpcauth.py u p
 1String to be appended to bitcoin.conf:
 2rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5
 3Your password:
 4p
 5$ src/bitcoind -regtest -rpcwhitelist=u:help '-rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5' &
 6$ src/bitcoin-cli -regtest -rpcuser=u -rpcpassword=p help | head -1
 7== Blockchain ==
 8$ src/bitcoin-cli -regtest -rpcuser=u -rpcpassword=p getnetworkinfo
 92019-12-13T02:01:37Z RPC User u not allowed to call method getnetworkinfo
10error: server returned HTTP error 403
11$ curl -u u:p --request POST \
12     --data $(python -c 'print(50000 * "[");') http://127.0.0.1:18443
13curl: (52) Empty reply from server
14[1]+  Segmentation fault      (core dumped) src/bitcoind -regtest -rpcwhitelist=u:help '-rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5'

    See discussion in #17742.

  87. laanwj referenced this in commit c3628e4483 on Dec 16, 2019
  88. luke-jr referenced this in commit f7439b8b60 on Jan 5, 2020
  89. luke-jr referenced this in commit 1eafa07909 on Jan 5, 2020
  90. deadalnix referenced this in commit 15a5df03b0 on Apr 23, 2020
  91. fanquake removed the label Needs release note on May 23, 2020
  92. fanquake commented at 7:27 am on May 23, 2020: member
    This has a release note in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.20.0-Release-Notes-Draft.
  93. Fonta1n3 commented at 1:08 pm on June 20, 2020: none
    Is there anyway to disable specific rpc commands?
  94. JeremyRubin commented at 5:33 pm on June 20, 2020: contributor
    You have to list out all the ones you want enabled. We don’t provide specific disabling.
  95. Fonta1n3 commented at 1:36 pm on September 3, 2020: none
    @JeremyRubin It sure would be handy to be able to incorporate this with multiwalletrpc, so that I can manually add trusted others to use my node but block them from accessing specific wallets. I am sorry my c++ is not good enough to submit a PR from scratch.. I am assuming it’s not possible at the moment?
  96. JeremyRubin commented at 6:56 pm on September 3, 2020: contributor
    I think general contributor opinion on rpc whitelists is that it shouldn’t scope creep any further, but I agree a limitwallets flag would be pretty useful. I think luke does a lot of multiwallet stuff @luke-jr and might have an opinion as it does seem somewhat orthogonal. Luke what do you think of some sort of loadwalletforuser/limitwallet API?
  97. luke-jr commented at 10:43 pm on September 3, 2020: member
    See also #10615 (included in Knots)
  98. sidhujag referenced this in commit 396d9eeafc on Nov 10, 2020
  99. DrahtBot locked this on Feb 15, 2022


JeremyRubin kallewoof promag NicolasDorier instagibbs randolf RHavar MarcoFalke jonasschnelli jimpo eklitzke sipa laanwj gmaxwell jnewbery DrahtBot fanquake ryanofsky emilengler practicalswift Fonta1n3 luke-jr

Labels
Feature RPC/REST/ZMQ

Milestone
0.20.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me