Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RPC Whitelist Feature from #12248 #12763

Merged
merged 2 commits into from Dec 13, 2019
Merged

Conversation

@JeremyRubin
Copy link
Contributor

JeremyRubin commented Mar 23, 2018

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.

Copy link
Member

kallewoof left a comment

utACK 788b174c33896065065d1ab376d6451b0f7575cd

src/httprpc.cpp Outdated

for (const std::string& strRPCWhitelist : gArgs.GetArgs("-rpcwhitelist")) {
std::string strUser = strRPCWhitelist.substr(0, strRPCWhitelist.find(':'));
std::string strWhitelist = strRPCWhitelist.substr(strRPCWhitelist.find(':') + 1);

This comment has been minimized.

Copy link
@kallewoof

kallewoof Mar 23, 2018

Member

This behavior may be fine, but this will silently turn "foo" into strUser == strWhitelist == "foo".

This comment has been minimized.

Copy link
@promag

promag Mar 23, 2018

Member

Agree, it should check for correct format.

@NicolasDorier

This comment has been minimized.

Copy link
Member

NicolasDorier commented Mar 23, 2018

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.

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Mar 23, 2018

concept ACK

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Mar 23, 2018

@RHavar you may be interested?

@randolf

This comment has been minimized.

Copy link
Contributor

randolf commented Mar 23, 2018

Concept ACK.

@RHavar

This comment has been minimized.

Copy link
Contributor

RHavar commented Mar 23, 2018

@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)

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Mar 23, 2018

@RHavar this prevents privkey dumps, as a first step at least

Copy link
Member

promag left a comment

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.

if (whitelistedRPC.count(jreq.authUser)) {
for (unsigned int reqIdx = 0; reqIdx < valRequest.size(); reqIdx++) {
if (!valRequest[reqIdx].isObject()) {
throw JSONRPCError(RPC_INVALID_REQUEST, "Invalid Request object");

This comment has been minimized.

Copy link
@promag

promag Mar 23, 2018

Member

New error, should have a test.

src/httprpc.cpp Outdated
@@ -63,6 +63,8 @@ class HTTPRPCTimerInterface : public RPCTimerInterface
static std::string strRPCUserColonPass;
/* Stored RPC timer interface (for unregistration) */
static std::unique_ptr<HTTPRPCTimerInterface> httpRPCTimerInterface;
/* RPC Auth Whitelist */
static std::map<std::string, std::set<std::string>> whitelistedRPC;

This comment has been minimized.

Copy link
@promag

promag Mar 23, 2018

Member

rpc_whitelist?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 11, 2019

Member

style-nit in commit b411ae6: g_rpc_whitelist, because it is a global

src/httprpc.cpp Outdated

for (const std::string& strRPCWhitelist : gArgs.GetArgs("-rpcwhitelist")) {
std::string strUser = strRPCWhitelist.substr(0, strRPCWhitelist.find(':'));
std::string strWhitelist = strRPCWhitelist.substr(strRPCWhitelist.find(':') + 1);

This comment has been minimized.

Copy link
@promag

promag Mar 23, 2018

Member

Agree, it should check for correct format.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 25, 2018

Concept ACK

src/httprpc.h Outdated
@@ -7,6 +7,7 @@

#include <string>
#include <map>
#include <set>

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Mar 25, 2018

Member

Why has that to be here?

This comment has been minimized.

Copy link
@JeremyRubin

JeremyRubin Mar 26, 2018

Author Contributor

Wasn't sure, was just trying to follow local style. Not sure that string or map need to be there either?

This comment has been minimized.

Copy link
@promag

promag Mar 26, 2018

Member

None needs to be there. If you want to remove, do it in a different commit.

Copy link
Contributor

jimpo left a comment

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.

src/httprpc.cpp Outdated
if (whitelistedRPC.count(jreq.authUser) && !whitelistedRPC[jreq.authUser].count(jreq.strMethod)) {
LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, jreq.strMethod);
req->WriteHeader("WWW-Authenticate", WWW_AUTH_HEADER_DATA);
req->WriteReply(HTTP_UNAUTHORIZED);

This comment has been minimized.

Copy link
@jimpo

jimpo Mar 26, 2018

Contributor

I think the 403 Forbidden status is more appropriate. See https://stackoverflow.com/a/6937030.

Copy link
Member

kallewoof left a comment

utACK

auto pos = strRPCWhitelist.find(':');
std::string strUser = strRPCWhitelist.substr(0, pos);
std::set<std::string>& whitelist = rpc_whitelist[strUser];
if (pos != std::string::npos) {

This comment has been minimized.

Copy link
@kallewoof

kallewoof Mar 27, 2018

Member

Should this maybe error on == case? It now silently ignores nocolonvalues.

This comment has been minimized.

Copy link
@JeremyRubin

JeremyRubin Mar 27, 2018

Author Contributor

It actually doesn't silently ignore it -- it sets an empty whitelist.

Copy link
Member

eklitzke left a comment

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.

src/httprpc.cpp Outdated
std::set<std::string>& whitelist = rpc_whitelist[strUser];
if (pos != std::string::npos) {
std::string strWhitelist = strRPCWhitelist.substr(pos + 1);
boost::split(whitelist, strWhitelist, boost::is_any_of(","));

This comment has been minimized.

Copy link
@eklitzke

eklitzke Mar 27, 2018

Member

You can split with a std::istream or std::getline, no need for boost::split here.

This comment has been minimized.

Copy link
@JeremyRubin

JeremyRubin Mar 27, 2018

Author Contributor

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.

@kallewoof

This comment has been minimized.

Copy link
Member

kallewoof commented Mar 27, 2018

Yeah it's probably better to check if size() > 0 and to do default-no-access if so.

@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented Mar 27, 2018

@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.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Mar 27, 2018


EDIT: nevermind, that's a concern for blacklists, not whitelists.

@eklitzke

This comment has been minimized.

Copy link
Member

eklitzke commented Mar 28, 2018

@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):

# Default permissions are for a bunch of read only rpcs
rpcallowed = getblock,getblockchaininfo

# Alice has the default permissions, plus stuff to admin the network
rpcallowed.alice = addnode,clearbanned,listbanned

# Bob has the default permissions, plus some basic access to the wallet
rpcallowed.bob = getbalance,getwalletinfo,getnewaddress

# Carol is an admin, she can access everything
rpcallowed.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.

@eklitzke

This comment has been minimized.

Copy link
Member

eklitzke commented Mar 28, 2018

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:

---
policies:
  # a default policy
  - policy: &default
      - getblock
      - getblockchaininfo
      - getblockcount

  # network admin policy
  - policy: &networkadmin
      - addnode
      - clearbanned
      - getpeerinfo

users:
  # alice has default plus network admin
  - name: alice
    policies:
      - *default
      - *networkadmin

  # bob just has default policies
  - name: bob
    policies:
      - *default

  # syntax idea 1 for full access
  - name: carol
    admin: true

  # syntax idea 2 for full access, !all is implicitly defined
  - name: dave
    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.

@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented Mar 30, 2018

@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.

@JeremyRubin JeremyRubin force-pushed the JeremyRubin:whitelistrpc branch Apr 13, 2018
@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented Apr 13, 2018

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.

@kallewoof

This comment has been minimized.

Copy link
Member

kallewoof commented Apr 13, 2018

LGTM. Checked code, utACK.

@jimpo

This comment has been minimized.

Copy link
Contributor

jimpo commented May 3, 2018

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.

@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented May 7, 2018

-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.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 30, 2018

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.

@promag

This comment has been minimized.

Copy link
Member

promag commented May 30, 2018

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.

@JeremyRubin JeremyRubin force-pushed the JeremyRubin:whitelistrpc branch to 8c45d93 Jun 12, 2018
@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented Jun 12, 2018

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.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Nov 18, 2019

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 😃

@JeremyRubin JeremyRubin force-pushed the JeremyRubin:whitelistrpc branch from 8c45d93 to b12745b Nov 18, 2019
@fanquake fanquake removed the Up for grabs label Nov 18, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 19, 2019

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.

@emilengler

This comment has been minimized.

Copy link
Contributor

emilengler commented Nov 20, 2019

@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 :)

Copy link
Member

MarcoFalke left a comment

ACK 89d9922, could squash some of the commits 🎠

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 89d9922ce7, could squash some of the commits 🎠
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjI7gv+J/mVjsR4q8r8nIVmIKo737Xq0NNmgC58fh/Gdw3Mbvwh8t+8aC24TVCM
N0Zkmn8HtIsXQVQbx8/BVJDaUt6D2mJ/LYcy6XOAx7rwWlDlM5TPyCQ5nNlQGtaz
7xUvazxHkdE9wbB+aUDGmnUTS4AAS+pY6sEX7FWvAq42vSaoRLj8jS8xIsdRrwUh
Tu1ezvsyoQFbEp4vc3btFUaxpZcXCXDmOxGM3YDHt3qsKmGr9LwF4D83ZEOZ1vBt
/cO+JTYioaLIGPqREXet01aSJnBoSTgu8gwvWlEGd3kWqChD4nycx1iSpL/Blnx/
VbS+6wtdU9lWax8cJqtHocrJCvWrrPZfc2BD/1GaOGm0z0QCkmerOYJmo65Af+zn
iMZ7GMx+f6CyCclzMM2SzByrktyNgOChsEL+mJ+ag0qdDOOAHTx/IlBqk4TCztOx
+va0kMhgt0zq2TkePTuL9kGB2hX8kQvhh3Dq6X3z9AF2EELeY022xt1TeRn/VUp2
f339FZxm
=4Uvy
-----END PGP SIGNATURE-----

Timestamp of file with hash 10b6cda480eee9ab8ccc67b28f8ab41d6fc050f6ba594d5fedee976626aa348a -

src/httprpc.h Outdated Show resolved Hide resolved
src/httprpc.cpp Outdated
@@ -63,6 +63,8 @@ class HTTPRPCTimerInterface : public RPCTimerInterface
static std::string strRPCUserColonPass;
/* Stored RPC timer interface (for unregistration) */
static std::unique_ptr<HTTPRPCTimerInterface> httpRPCTimerInterface;
/* RPC Auth Whitelist */
static std::map<std::string, std::set<std::string>> whitelistedRPC;

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 11, 2019

Member

style-nit in commit b411ae6: g_rpc_whitelist, because it is a global

src/httprpc.cpp Outdated Show resolved Hide resolved
src/httprpc.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented Dec 11, 2019

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 11, 2019

Force push is just fine

@JeremyRubin JeremyRubin force-pushed the JeremyRubin:whitelistrpc branch from 89d9922 to 7414d38 Dec 11, 2019
@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented Dec 11, 2019

Done 👍

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 11, 2019

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

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

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

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUicqgv8CxETZpFF7NDO9EYPqAd2XbtMlKyIA3lXSXFMJZAkd7ZVhHnpXm005RfC
6RzI8iks7Fhas2pNd26z7Jhjqm9Qzb9HbdH2YoxzV83++hvwWvdvUJnY9kHm89Y0
bRG/bXw3vUZkJRp+gq1Z+ZyNkkiePTARecIbcpI1Hb1ibVNagXEowjbgcY+lVbbj
qbmF/ZDxUaA3ztOlBHQ/e99hine9ibwfpjKlgMiHb4IukwWGiui86y06UJqHYFAL
O1EaoybQlVJ/r3rnT96zgwcGHQ7vlOD/02USzLRa/F3XrSQvrPiMlNn4UaaHXMbK
lGlAQ58YEx7GKM80S1cBJOPnbBrIz1TGtilv7vFyLcgZrc8PfFKKXhvzxem4DFsl
JYoaP2T46ad53TaRCiUQvAZa0EM+2srqBE/KibwOYVJQEOgSEBOYMagsKFIE6bnl
Xed01A9z/5599ZeXlCbra8gBFEstHn6OZEJ8WxcBkc2lbU1Hnn29OB/jArFHTPg4
B5i7SBMi
=t/W5
-----END PGP SIGNATURE-----

Timestamp of file with hash 9efdd13b36d3827d0d676b35f29a2117adc6c8e29b7a726131c675cb666639aa -

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 13, 2019

ACK 2081442

laanwj added a commit that referenced this pull request Dec 13, 2019
2081442 test: Add test for rpc_whitelist (Emil Engler)
7414d38 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 2081442

Tree-SHA512: 0dc1ac6a6f2f4b0be9c9054d495dd17752fe7b3589aeab2c6ac4e1f91cf4e7e355deedcb5d76d707cbb5a949c2f989c871b74d6bf129351f429569a701adbcbf
@laanwj laanwj merged commit 2081442 into bitcoin:master Dec 13, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added this to the 0.20.0 milestone Dec 13, 2019
@JeremyRubin JeremyRubin deleted the JeremyRubin:whitelistrpc branch Dec 13, 2019
sidhujag added a commit to syscoin/syscoin that referenced this pull request Dec 13, 2019
2081442 test: Add test for rpc_whitelist (Emil Engler)
7414d38 Add RPC Whitelist Feature from bitcoin#12248 (Jeremy Rubin)

Pull request description:

  Summary
  ====

  This patch adds the RPC whitelisting feature requested in bitcoin#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 2081442

Tree-SHA512: 0dc1ac6a6f2f4b0be9c9054d495dd17752fe7b3589aeab2c6ac4e1f91cf4e7e355deedcb5d76d707cbb5a949c2f989c871b74d6bf129351f429569a701adbcbf
@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Dec 14, 2019

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):

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

See discussion in #17742.

laanwj added a commit that referenced this pull request Dec 16, 2019
7965e0b doc: Add release note for RPC Whitelist (Emil Engler)

Pull request description:

  A release note for #12763

ACKs for top commit:
  laanwj:
    ACK 7965e0b

Tree-SHA512: 4ac3e62029a403e64e4cd3183433dc7aa071d42688b689d7cffb8f08dc4b26d2a586d32fa791d2b5679d6b95cd6e34c56e40a5592b9af446ad9429307f7267fe
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.