From b89fd6128b8d42433a8d7ae03e8932d52d1e696c Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 13 Dec 2019 11:25:39 +0100 Subject: [PATCH] Merge #12763: Add RPC Whitelist Feature from #12248 2081442c421cc4376e5d7839f68fbe7630e89103 test: Add test for rpc_whitelist (Emil Engler) 7414d3820c833566b4f48c6c120a18bf53978c55 Add RPC Whitelist Feature from #12248 (Jeremy Rubin) Pull request description: 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 ``` rpcauth=user1:4cc74397d6e9972e5ee7671fd241$11849357f26a5be7809c68a032bc2b16ab5dcf6348ef3ed1cf30dae47b8bcc71 rpcauth=user2:181b4a25317bff60f3749adee7d6bca0$d9c331474f1322975fa170a2ffbcb176ba11644211746b27c1d317f265dd4ada rpcauth=user3:a6c8a511b53b1edcf69c36984985e$13cfba0e626db19061c9d61fa58e712d0319c11db97ad845fa84517f454f6675 rpcwhitelist=user1:getnetworkinfo rpcwhitelist=user2:getnetworkinfo,getwalletinfo, getbestblockhash rpcwhitelistdefault=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. ACKs for top commit: laanwj: ACK 2081442c421cc4376e5d7839f68fbe7630e89103 Tree-SHA512: 0dc1ac6a6f2f4b0be9c9054d495dd17752fe7b3589aeab2c6ac4e1f91cf4e7e355deedcb5d76d707cbb5a949c2f989c871b74d6bf129351f429569a701adbcbf --- src/httprpc.cpp | 62 ++++++++++++++++++- src/init.cpp | 2 + test/functional/rpc_whitelist.py | 100 +++++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 4 files changed, 162 insertions(+), 3 deletions(-) create mode 100644 test/functional/rpc_whitelist.py diff --git a/src/httprpc.cpp b/src/httprpc.cpp index aa8ae8581f310..bb975a4ea11f4 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -15,7 +15,12 @@ #include #include +#include +#include +#include #include +#include +#include #include // boost::trim @@ -63,6 +68,9 @@ class HTTPRPCTimerInterface : public RPCTimerInterface static std::string strRPCUserColonPass; /* Stored RPC timer interface (for unregistration) */ static std::unique_ptr httpRPCTimerInterface; +/* RPC Auth Whitelist */ +static std::map> g_rpc_whitelist; +static bool g_rpc_whitelist_default = false; static void JSONErrorReply(HTTPRequest* req, const UniValue& objError, const UniValue& id) { @@ -185,18 +193,45 @@ static bool HTTPReq_JSONRPC(const util::Ref& context, HTTPRequest* req) jreq.URI = req->GetURI(); std::string strReply; + bool user_has_whitelist = g_rpc_whitelist.count(jreq.authUser); + if (!user_has_whitelist && g_rpc_whitelist_default) { + LogPrintf("RPC User %s not allowed to call any methods\n", jreq.authUser); + req->WriteReply(HTTP_FORBIDDEN); + return false; + // singleton request - if (valRequest.isObject()) { + } else if (valRequest.isObject()) { jreq.parse(valRequest); - + if (user_has_whitelist && !g_rpc_whitelist[jreq.authUser].count(jreq.strMethod)) { + LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, jreq.strMethod); + req->WriteReply(HTTP_FORBIDDEN); + return false; + } UniValue result = tableRPC.execute(jreq); // Send reply strReply = JSONRPCReply(result, NullUniValue, jreq.id); // array of requests - } else if (valRequest.isArray()) + } else if (valRequest.isArray()) { + if (user_has_whitelist) { + for (unsigned int reqIdx = 0; reqIdx < valRequest.size(); reqIdx++) { + if (!valRequest[reqIdx].isObject()) { + throw JSONRPCError(RPC_INVALID_REQUEST, "Invalid Request object"); + } else { + const UniValue& request = valRequest[reqIdx].get_obj(); + // Parse method + std::string strMethod = find_value(request, "method").get_str(); + if (!g_rpc_whitelist[jreq.authUser].count(strMethod)) { + LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, strMethod); + req->WriteReply(HTTP_FORBIDDEN); + return false; + } + } + } + } strReply = JSONRPCExecBatch(jreq, valRequest.get_array()); + } else throw JSONRPCError(RPC_PARSE_ERROR, "Top-level object parse error"); @@ -231,6 +266,27 @@ static bool InitRPCAuthentication() { LogPrintf("Using rpcauth authentication.\n"); } + + g_rpc_whitelist_default = gArgs.GetBoolArg("-rpcwhitelistdefault", gArgs.IsArgSet("-rpcwhitelist")); + for (const std::string& strRPCWhitelist : gArgs.GetArgs("-rpcwhitelist")) { + auto pos = strRPCWhitelist.find(':'); + std::string strUser = strRPCWhitelist.substr(0, pos); + bool intersect = g_rpc_whitelist.count(strUser); + std::set& whitelist = g_rpc_whitelist[strUser]; + if (pos != std::string::npos) { + std::string strWhitelist = strRPCWhitelist.substr(pos + 1); + std::set new_whitelist; + boost::split(new_whitelist, strWhitelist, boost::is_any_of(", ")); + if (intersect) { + std::set tmp_whitelist; + std::set_intersection(new_whitelist.begin(), new_whitelist.end(), + whitelist.begin(), whitelist.end(), std::inserter(tmp_whitelist, tmp_whitelist.end())); + new_whitelist = std::move(tmp_whitelist); + } + whitelist = std::move(new_whitelist); + } + } + return true; } diff --git a/src/init.cpp b/src/init.cpp index 8b104dd58fbef..12c94225e5dc1 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -754,6 +754,8 @@ void SetupServerArgs(NodeContext& node) argsman.AddArg("-rpcservertimeout=", strprintf("Timeout during HTTP requests (default: %d)", DEFAULT_HTTP_SERVER_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC); argsman.AddArg("-rpcthreads=", strprintf("Set the number of threads to service RPC calls (default: %d)", DEFAULT_HTTP_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::RPC); argsman.AddArg("-rpcuser=", "Username for JSON-RPC connections", ArgsManager::ALLOW_ANY, OptionsCategory::RPC); + argsman.AddArg("-rpcwhitelist=", "Set a whitelist to filter incoming RPC calls for a specific user. The field comes in the format: :,,...,. If multiple whitelists are set for a given user, they are set-intersected. See -rpcwhitelistdefault documentation for information on default whitelist behavior.", ArgsManager::ALLOW_ANY, OptionsCategory::RPC); + argsman.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.", ArgsManager::ALLOW_BOOL, OptionsCategory::RPC); argsman.AddArg("-rpcworkqueue=", strprintf("Set the depth of the work queue to service RPC calls (default: %d)", DEFAULT_HTTP_WORKQUEUE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC); argsman.AddArg("-server", "Accept command line and JSON-RPC commands", ArgsManager::ALLOW_ANY, OptionsCategory::RPC); diff --git a/test/functional/rpc_whitelist.py b/test/functional/rpc_whitelist.py new file mode 100644 index 0000000000000..2ba22b8a1eb45 --- /dev/null +++ b/test/functional/rpc_whitelist.py @@ -0,0 +1,100 @@ +#!/usr/bin/env python3 +# Copyright (c) 2017-2019 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +""" +A test for RPC users with restricted permissions +""" +from test_framework.test_framework import BitcoinTestFramework +import os +from test_framework.util import ( + get_datadir_path, + assert_equal, + str_to_b64str +) +import http.client +import urllib.parse + +def rpccall(node, user, method): + url = urllib.parse.urlparse(node.url) + headers = {"Authorization": "Basic " + str_to_b64str('{}:{}'.format(user[0], user[3]))} + conn = http.client.HTTPConnection(url.hostname, url.port) + conn.connect() + conn.request('POST', '/', '{"method": "' + method + '"}', headers) + resp = conn.getresponse() + conn.close() + return resp + + +class RPCWhitelistTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + + def setup_chain(self): + super().setup_chain() + # 0 => Username + # 1 => Password (Hashed) + # 2 => Permissions + # 3 => Password Plaintext + self.users = [ + ["user1", "50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e", "getbestblockhash,getblockcount,", "12345"], + ["user2", "8650ba41296f62092377a38547f361de$4620db7ba063ef4e2f7249853e9f3c5c3592a9619a759e3e6f1c63f2e22f1d21", "getblockcount", "54321"] + ] + # For exceptions + self.strange_users = [ + # Test empty + ["strangedude", "62d67dffec03836edd698314f1b2be62$c2fb4be29bb0e3646298661123cf2d8629640979cabc268ef05ea613ab54068d", ":", "s7R4nG3R7H1nGZ"], + ["strangedude2", "575c012c7fe4b1e83b9d809412da3ef7$09f448d0acfc19924dd62ecb96004d3c2d4b91f471030dfe43c6ea64a8f658c1", "", "s7R4nG3R7H1nGZ"], + # Test trailing comma + ["strangedude3", "23189c561b5975a56f4cf94030495d61$3a2f6aac26351e2257428550a553c4c1979594e36675bbd3db692442387728c0", ":getblockcount,", "s7R4nG3R7H1nGZ"], + # Test overwrite + ["strangedude4", "990c895760a70df83949e8278665e19a$8f0906f20431ff24cb9e7f5b5041e4943bdf2a5c02a19ef4960dcf45e72cde1c", ":getblockcount, getbestblockhash", "s7R4nG3R7H1nGZ"], + ["strangedude4", "990c895760a70df83949e8278665e19a$8f0906f20431ff24cb9e7f5b5041e4943bdf2a5c02a19ef4960dcf45e72cde1c", ":getblockcount", "s7R4nG3R7H1nGZ"], + # Testing the same permission twice + ["strangedude5", "d12c6e962d47a454f962eb41225e6ec8$2dd39635b155536d3c1a2e95d05feff87d5ba55f2d5ff975e6e997a836b717c9", ":getblockcount,getblockcount", "s7R4nG3R7H1nGZ"] + ] + # These commands shouldn't be allowed for any user to test failures + self.never_allowed = ["getnetworkinfo"] + with open(os.path.join(get_datadir_path(self.options.tmpdir, 0), "dash.conf"), 'a', encoding='utf8') as f: + f.write("\nrpcwhitelistdefault=0\n") + for user in self.users: + f.write("rpcauth=" + user[0] + ":" + user[1] + "\n") + f.write("rpcwhitelist=" + user[0] + ":" + user[2] + "\n") + # Special cases + for strangedude in self.strange_users: + f.write("rpcauth=" + strangedude[0] + ":" + strangedude[1] + "\n") + f.write("rpcwhitelist=" + strangedude[0] + strangedude[2] + "\n") + + + def run_test(self): + for user in self.users: + permissions = user[2].replace(" ", "").split(",") + # Pop all empty items + i = 0 + while i < len(permissions): + if permissions[i] == '': + permissions.pop(i) + + i += 1 + for permission in permissions: + self.log.info("[" + user[0] + "]: Testing a permitted permission (" + permission + ")") + assert_equal(200, rpccall(self.nodes[0], user, permission).status) + for permission in self.never_allowed: + self.log.info("[" + user[0] + "]: Testing a non permitted permission (" + permission + ")") + assert_equal(403, rpccall(self.nodes[0], user, permission).status) + # Now test the strange users + for permission in self.never_allowed: + self.log.info("Strange test 1") + assert_equal(403, rpccall(self.nodes[0], self.strange_users[0], permission).status) + for permission in self.never_allowed: + self.log.info("Strange test 2") + assert_equal(403, rpccall(self.nodes[0], self.strange_users[1], permission).status) + self.log.info("Strange test 3") + assert_equal(200, rpccall(self.nodes[0], self.strange_users[2], "getblockcount").status) + self.log.info("Strange test 4") + assert_equal(403, rpccall(self.nodes[0], self.strange_users[3], "getbestblockhash").status) + self.log.info("Strange test 5") + assert_equal(200, rpccall(self.nodes[0], self.strange_users[4], "getblockcount").status) + +if __name__ == "__main__": + RPCWhitelistTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 6cfd62803a18e..d647ae774cd7d 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -151,6 +151,7 @@ 'interface_rpc.py', 'rpc_psbt.py', 'rpc_users.py', + 'rpc_whitelist.py', 'feature_proxy.py', 'rpc_signrawtransaction.py', 'p2p_addrv2_relay.py',