Add RPC Whitelist Feature from #12248 #12763

JeremyRubin · 2018-03-23T04:42:40Z

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.

kallewoof

utACK 788b174c33896065065d1ab376d6451b0f7575cd

kallewoof · 2018-03-23T08:29:59Z

src/httprpc.cpp

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

Agree, it should check for correct format.

NicolasDorier · 2018-03-23T09:36:06Z

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 · 2018-03-23T12:58:55Z

concept ACK

instagibbs · 2018-03-23T12:59:32Z

@RHavar you may be interested?

randolf · 2018-03-23T15:08:13Z

Concept ACK.

RHavar · 2018-03-23T15:21:13Z

@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 · 2018-03-23T15:48:43Z

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

promag

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.

promag · 2018-03-23T15:47:58Z

src/httprpc.cpp

New error, should have a test.

promag · 2018-03-23T15:56:18Z

src/httprpc.cpp

rpc_whitelist?

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

promag · 2018-03-23T15:56:52Z

src/httprpc.cpp

Agree, it should check for correct format.

jonasschnelli · 2018-03-25T13:41:50Z

Concept ACK

jonasschnelli · 2018-03-25T13:50:34Z

src/httprpc.h

Why has that to be here?

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

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

jimpo

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.

jimpo · 2018-03-26T03:00:07Z

src/httprpc.cpp

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

kallewoof

utACK

kallewoof · 2018-03-27T02:29:52Z

src/httprpc.cpp

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

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

eklitzke

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.

eklitzke · 2018-03-27T06:57:18Z

src/httprpc.cpp

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

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 · 2018-03-27T15:09:58Z

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

JeremyRubin · 2018-03-27T20:27:36Z

@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 · 2018-03-27T20:48:23Z


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

eklitzke · 2018-03-28T03:01:27Z

@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 · 2018-03-28T03:20:49Z

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 · 2018-03-30T19:12:02Z

@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 · 2018-04-13T01:48:41Z

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 · 2018-04-13T06:14:24Z

LGTM. Checked code, utACK.

jimpo · 2018-05-03T02:38:44Z

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 · 2018-05-07T06:08:04Z

-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 · 2018-05-30T13:28:43Z

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 · 2018-05-30T13:34:23Z

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 · 2018-06-12T06:27:31Z

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.

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

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 · 2019-12-14T17:43:05Z

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.

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

Github-Pull: bitcoin#12763 Rebased-From: 7414d38

Github-Pull: bitcoin#12763 Rebased-From: 2081442

Summary: * test: Add test for rpc_whitelist This is a backport of Core [[bitcoin/bitcoin#12763 | PR12763]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5810

fanquake · 2020-05-23T07:27:11Z

This has a release note in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.20.0-Release-Notes-Draft.

Fonta1n3 · 2020-06-20T13:08:10Z

Is there anyway to disable specific rpc commands?

JeremyRubin · 2020-06-20T17:33:34Z

You have to list out all the ones you want enabled. We don't provide specific disabling.

Fonta1n3 · 2020-09-03T13:36:53Z

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

JeremyRubin · 2020-09-03T18:56:15Z

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?

luke-jr · 2020-09-03T22:43:02Z

See also #10615 (included in Knots)

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

fanquake added the RPC/REST/ZMQ label Mar 23, 2018

kallewoof reviewed Mar 23, 2018

View reviewed changes

promag reviewed Mar 23, 2018

View reviewed changes

jonasschnelli reviewed Mar 25, 2018

View reviewed changes

jimpo reviewed Mar 26, 2018

View reviewed changes

src/httprpc.cpp Outdated

Copy link

Contributor

jimpo Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

kallewoof reviewed Mar 27, 2018

View reviewed changes

eklitzke reviewed Mar 27, 2018

View reviewed changes

JeremyRubin force-pushed the whitelistrpc branch from 43e2fe5 to e6a75fa Compare April 13, 2018 01:44

JeremyRubin force-pushed the whitelistrpc branch from e6a75fa to 8c45d93 Compare June 12, 2018 06:26

JeremyRubin closed this Jun 18, 2018

laanwj merged commit 2081442 into bitcoin:master Dec 13, 2019

laanwj added Feature Needs release note labels Dec 13, 2019

laanwj added this to the 0.20.0 milestone Dec 13, 2019

cvengler mentioned this pull request Dec 13, 2019

doc: Add release note for RPC Whitelist #17743

Merged

JeremyRubin deleted the whitelistrpc branch December 13, 2019 17:19

practicalswift mentioned this pull request Dec 13, 2019

UniValue upgrade needed to avoid UniValue CVE-2019-18936 (segfault via high object depth JSON) #17742

Closed

cvengler mentioned this pull request Dec 15, 2019

SSL/TLS support for the HTTP(S) server #17747

Closed

ryanofsky mentioned this pull request Dec 20, 2019

common: Disallow calling IsArgSet() on ALLOW_LIST options #17783

Draft

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020

Add RPC Whitelist Feature from bitcoin#12248

f7439b8

Github-Pull: bitcoin#12763 Rebased-From: 7414d38

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020

test: Add test for rpc_whitelist

1eafa07

Github-Pull: bitcoin#12763 Rebased-From: 2081442

practicalswift mentioned this pull request Jan 27, 2020

psbt: check that various indexes and amounts are within bounds #17156

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

maflcko mentioned this pull request Apr 26, 2020

RPC Whitelist Files #12248

Closed

fanquake removed the Needs release note label May 23, 2020

This was referenced Jun 1, 2020

bitcoind should use RPC cookie auth not user/password fort-nix/nix-bitcoin#183

Closed

bitcoind RPC whitelist fort-nix/nix-bitcoin#191

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Add RPC Whitelist Feature from #12248 #12763

Add RPC Whitelist Feature from #12248 #12763

Conversation

JeremyRubin commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Using RPC Whitelists

Review Request

Notes

Future Work

Tests

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

kallewoof Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Mar 23, 2018

Uh oh!

instagibbs commented Mar 23, 2018

Uh oh!

instagibbs commented Mar 23, 2018

Uh oh!

randolf commented Mar 23, 2018

Uh oh!

RHavar commented Mar 23, 2018

Uh oh!

instagibbs commented Mar 23, 2018

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Mar 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimpo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

kallewoof Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eklitzke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Mar 27, 2018

Uh oh!

JeremyRubin commented Mar 27, 2018

Uh oh!

sipa commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

JeremyRubin commented Mar 23, 2018 •

edited

Loading

kallewoof Mar 23, 2018 •

edited

Loading

promag left a comment •

edited

Loading

jimpo left a comment •

edited

Loading

kallewoof Mar 27, 2018 •

edited

Loading

sipa commented Mar 27, 2018 •

edited

Loading

eklitzke commented Mar 28, 2018 •

edited

Loading

laanwj commented May 30, 2018 •

edited

Loading

practicalswift commented Dec 14, 2019 •

edited

Loading

Fonta1n3 commented Sep 3, 2020 •

edited

Loading

luke-jr commented Sep 3, 2020 •

edited

Loading