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

RPC Whitelist Files #12248

Closed
JeremyRubin opened this issue Jan 23, 2018 · 13 comments
Closed

RPC Whitelist Files #12248

JeremyRubin opened this issue Jan 23, 2018 · 13 comments

Comments

@JeremyRubin
Copy link
Contributor

JeremyRubin commented Jan 23, 2018

Recently I was discussing how to lock down access on a Bitcoin node with @jlin816 and we came up with 2 features that would be useful.

Proposing 2 features:

  1. rpc.conf file, which contains a list of enabled RPCs. No other RPCs should be callable. rpc.conf should be loaded at startup. The default file is all of them. Can also be passed in as command line args perhaps.
  2. a compile time flag file whitelist. The default file is all of them. This eases the production of custom builds where it would be impossible for the RPC to be called even if rpc.conf is not correctly set.

It's not high priority, but having this would make it easier to produce bitcoin node binaries for high security environments.

Does anyone have any thoughts or reservations on the above design before @jlin816 takes a crack at it (for her first contribution). Is this functionality already possible without external proxy?

@ryanofsky
Copy link
Contributor

Seems reasonable to me. Possible extensions:

@theuni
Copy link
Member

theuni commented Jan 26, 2018

Can you give an example use-case? As presented it sounds reasonable, though I'm a bit concerned about real-world utility.

RPC should definitely not be open to the world, no matter how locked down, so I'll assume that you're referring to a localhost or encrypted tunnel.

In that case, access to the user's profile dir should already be assumed. So I can't imagine locking down for the sake of privacy, but maybe to ensure maximum availability. So maybe you disable getblocktemplate because you don't want some jerk employee mining and wasting cpu resources from your exchange's edge node.

Or maybe you run a website that shows mempool data, so you want to unlock rpc access for a bitcoind on your subnet, with access mempool data only.

Do those examples sound about right?

I'm not convinced that it's necessary because (other than wallet), there's no privilege separation between the commands, so only real need for black/whitelists would be: "some rpc call is an effective DoS on my server so I want to block it". I'm not sure that we should be encouraging that, rather than just not having DoSsy rpcs :(

That said, a few things to look into:

  • We already have a "safe mode" that limits usable commands when we're uneasy about the chain we're on. That's not useful here, but it may be hepful to look at how it's implemented.
  • Echoing what @ryanofsky said, tying this to auth makes sense to me, so that different rpc users can have different command access.

@jlin816
Copy link

jlin816 commented Feb 1, 2018

Thanks for the feedback! I wrote up my preliminary ideas on the design and justification for why this feature is needed -- let me know if you have any reservations before I start implementing a proof-of-concept.


Problem

There is currently no way to lock down the JSON RPC API without filtering requests through a proxy (or application). In high security situations, we want to guarantee that only necessary and expected API calls can be made by specifying this behavior at the bitcoind node level. This doesn’t preclude the use of a proxy to filter requests, but augments it with an additional layer of safety checks.

Example Use Case

We make requests to the bitcoind through a proxy, ensuring that most clients will only be able to call a limited set of RPC methods. However, the proxy itself maintains an authenticated connection to our node with full privilege, making misbehavior potentially dangerous. We would like to guarantee that only the methods required by the application are available, precluding access to critical (i.e. leaking secrets or DoS-vectors) methods not required by the application. Even if an adversary can do some dangerous things, they can’t do arbitrarily dangerous things that we can prevent up front.

In summary, this is desirable because it:

  • Adds redundant layers of security
  • Protects against programmer error (configure system once, iterate on application code without having to worry about regressing accidentally to unsafe behavior or accidentally leaking some secret as a side effect)
  • Explicitly defines expectations for privilege and performance.
    • Opportunities for future work to enforce resource management per connection/rpcauth credential. (E.g., only allowing one rescan per day)
  • Introduces concept of privilege separation between commands, helps developers build more robust software, and encourages better practices
    • Bitcoin’s RPC API was not designed with the idea that a caller could be partially trusted. As @theuni put it, there’s no explicit concept of privilege separation.
    • Adding the ability to whitelist is a step towards recognizing the implicit privilege boundaries that do exist, such as a read v.s. a write operation, a stateful operation (like addnode), and a DoS vector operation (like import with rescan)

Implementation

We use a whitelist because it is better to be explicit about what is allowed rather than what is not allowed. This ensures that even if new, unsafe, RPCs are added, the functionality is not exposed by using old conf files. My design is based off of the rpcauth feature only -- if you are in a high security setting, you likely should favor rpcauth over rpcuser anyways; this serves as a soft deprecation. I’ve written out my design using TOML, but we can use whatever Bitcoin uses, if that’s preferable.

Runtime Configuration Design

Runtime rpc.conf will allow you to specify granular user-level privileges. For example, the following demonstrates how I’m proposing to create a rpcauth user allowed to access wallet RPCs and a second user that can also call getblockchaininfo.

rpc.conf

import = [rpc_base.conf]
[rpcuser1]
	includes = [wallet]
[rpcuser2]
	includes = [rpcuser1]
	getblockchaininfo = 1

rpc_base.conf

[wallet]
	//... < list all enabled wallet methods here >

bitcoin.conf

rpcauth=rpcuser1:76cc9394b1e7e7a4b04694f24f8d59e8$e366de9398ed25150874915e7f4d2fcbc13a0df06f0e6a324515e5651f9dfa43
rpcauth=rpcuser2:69d492fe5bb73253d5adaed75987fb9e$c0b939d2ee9723bb289734c320fee83e2724af3ec681c3276df4d2ced515b1e1

Design Considerations

There are two obvious ways we can do this, either grouping by user or grouping by rpc.

By RPC:

[gennewaddress]
mode=’on’ // enabled for all users

[gettransaction]
mode=’select’ // enabled for some users
users = [
     rpcuser1,
     rpcuser2
]

// [dumpprivkey]
//     mode=’off’ // disabled, not needed explicitly because whitelist

Pros of RPC-based:

  • Succinct; most of the time most users will have most methods.

Cons of RPC-based:

  • Needs more complexity to specify argument / value-based privileges for each user down the line

By User:

[rpcuser1]
    gettransaction=1
    gennewaddress=1
[rpcuser2]
    gennewaddress=1
    gettransaction=1
    dumpprivkey=1
[rpcuser3]
    gennewaddress=1

Pros of user-based:

  • Easy to read / figure out which users have which privileges
  • Easy to specify more granular privileges at the level of arguments / values for each user down the line
  • Easy to implement

Cons of user-based:

  • If all users require most RPCs, tables are long and repetitive, therefore difficult to audit

Overall, it seems that a user-centric configuration is preferable.

To address the cons of user based paradigm, we’ve introduced a notion of inheritance and several base classes (all, networking, wallet, info, etc) as well as a method of importing files. We can then describe the privileges of a user with access to the same commands as another user and access to any of the general info commands as follows:

rpc.conf

import = [rpc_base.conf]
[rpcuser1]
	gettransaction=1
	gennewaddress=1
[rpcuser2]
	includes = [rpcuser1, info]
	dumpprivkey=1

rpc_base.conf

[info]
	gettransactions=1
	get...
[network]
	...

Conflicting definitions would be resolved to the child’s definition (e.g., allowing to disable one single command in info). Circular dependencies result in undefined behavior (we could check for it, but it seems like overkill).

On startup, we’ll load in the configuration for all users as a hashtable. On each RPC request, we can check permissions to invoke method with name (and down the line, specific options/args for that method).

Long-term considerations + extensions

To address @ryanofsky’s suggestion of filtering params at the argument or value level, we can shell out to a script with path defined in the config file. I.e., we can pipe the json of the command to a subprocess which then pipes back a new json or error message. This permits flexibility because the types of properties you might want to check are sufficiently complicated that it would be inappropriate to try to express that logic in a config file. This design is similar to what’s done with walletnotify scripts. We’ll leave that for future work to more tightly define.

Compile Time Configuration

The simplest way to do compile time configuration would be to have an autogenerated header file which defines (or doesn’t define) a macro for every RPC. Then, each handler can be wrapped with an ifdef. This is a little bit messy, so what might be nicer would be to make our class from the runtime whitelist checker read these compile time rules. This is slightly less good because the code is still compiled, but accomplishes the goal of the RPC not being able to be called independent of modifiable configuration. I think it makes the most sense to focus on the runtime configuration first, and then in future work address the compile time flags.

Compatibility

This feature will be mostly backward-compatible: if users do not have rpc.conf defined, RPC behavior will be the same as before. The default rpc.conf will allow all RPCs for all users.

@gmaxwell
Copy link
Contributor

gmaxwell commented Feb 4, 2018

This seems hard for a user to use safely. e.g. it might not be immediately obvious which RPCs are 'safe' for a given application and which aren't-- e.g. dumpwallet writing to arbitrary files.

Fancy 'config file parser' seems not so great, why not propose something simpler like rpcwhitelist=user:allowedrpc1:allowedrpc2:...

Shelling out to something to process arguments sounds like the performance and security nightmare of classical CGIs. If someone really wants arbitrary code preprocessing their RPCs please make it a proxy. At least then it wouldn't necessarily have to fork for every request.

@jlin816
Copy link

jlin816 commented Feb 6, 2018

Thanks for the comments @gmaxwell!

re: user confusion - This feature (in particular with whitelist/enable-all by default) seems strictly better than what we have currently. Typical users don't have to touch the RPC configuration, but others at least have the option to restrict access if they have more precise needs.

re: config file - You make a good point; my previous design does add some complexity for the sake of flexibility. Formatting aside (not particularly attached to TOML and I can certainly use the syntax you suggest), are you objecting specifically to inheriting roles and includes? The advantage there seems non-trivial, to avoid typing out all hundred or so allowed RPCs for every new user.

re: shelling out - My plan is to implement any checking logic I require in proxy, so I'm on board with not adding native CGI-like support.

JeremyRubin added a commit to JeremyRubin/bitcoin that referenced this issue Jun 12, 2018
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Jul 7, 2018
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Dec 24, 2018
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Nov 15, 2019
JeremyRubin added a commit to JeremyRubin/bitcoin that referenced this issue Nov 18, 2019
JeremyRubin added a commit to JeremyRubin/bitcoin that referenced this issue Nov 18, 2019
Move set include to cpp

Change RPC Whitelist failure to 403 Forbidden

Fix RPC Whitelisting to properly handle no provided whitelist (e.g., nothing following semicolon or only user name

Refactor whitelistedRPC to rpc_whitelist

implement rpc whitelist intersection semantics

strip extra whitespace from tokens in rpc whitelist parsing

Cleanup RPC Whitelist Style

Enable rpc whitelist by default if any rpcwhitelist is set, but allow disabling

Add RPC Whitelist Documentation

Add Arg type flags to rpcwhitelist argsmanager
JeremyRubin added a commit to JeremyRubin/bitcoin that referenced this issue Dec 11, 2019
JeremyRubin added a commit to JeremyRubin/bitcoin that referenced this issue Dec 11, 2019
laanwj added a commit that referenced this issue 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
sidhujag pushed a commit to syscoin/syscoin that referenced this issue 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
djp3 added a commit to djp3/bitcoin that referenced this issue Dec 16, 2019
…ocal

* 'master' of ssh://github.com/bitcoin/bitcoin: (1260 commits)
  tests: Add tests for decoding/parsing of base32, base64 and money strings containing NUL characters
  util: Don't allow DecodeBase32(...) of strings with embedded NUL characters
  util: Don't allow DecodeBase64(...) of strings with embedded NUL characters
  util: Don't allow ParseMoney(...) of strings with embedded NUL characters
  fix directory path for secp256k1 subtree in developer-notes
  tests: Add fuzzing harness for CheckBlock(...) and other CBlock related functions
  tests: Add corpora suppression (FUZZERS_MISSING_CORPORA) for fuzzers missing in https://github.com/bitcoin-core/qa-assets/tree/master/fuzz_seed_corpus
  doc: Add release note for RPC Whitelist
  depends: disable unused qt networking features
  depends: -optimized-qmake is now -optimized-tools
  depends: skip building qt proxies
  doc: update developer notes wrt unix epoch time
  qa: unify unix epoch time descriptions
  test: Add test for rpc_whitelist
  Update msvc build for Visual Studio 2019 v16.4
  build: fix typo
  util: Don't allow base58-decoding of std::string:s containing non-base58 characters
  tests: Add tests for base58-decoding of std::string:s containing non-base58 characters
  rpc: require second argument only for scantxoutset start action
  Add RPC Whitelist Feature from bitcoin#12248
  ...
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Jan 5, 2020
@maflcko
Copy link
Member

maflcko commented Apr 26, 2020

Anything left to do here after #12763?

@JeremyRubin
Copy link
Contributor Author

Yeah there's still a bit to do; but I got discouraged by it taking 2 years to merge to finish the work.

Up next for this would be:

  1. Adding some permission-discovery RPC so you can see which perms you have
  2. Adding a credential fork and dropper thing, allowing you to pass short-lived capability credentials.
  3. Adding some sort of limits system.

You can imagine in the future, I have a process which has a "master API key" to my node, with a maximal set of perms required. I can take my credential, and call the fork RPC to generate a child credential. Then i call AddPerm (one by one) on the credential with my master API key. Then I send it to the appropraite sub process. The subprocess receives the credential. The subprocess calls query_my_rpcs with the new credential. If it has the query_my_rpcs permission set (which we can default to being on safely I think as an exception...), then it learns a table of what permissions it has and if it matches a manifest for that process of what it needs to complete it's task, or if it needs to prompt the user for more credentials. You can imagine that core then has a credential manager GUI pop up and asks if it should honor the request for more credentials -- both apps can show a pin or high entropy generative art so you know it's not a MITM or something...

Is this overkill? Maybe. Does it need to be in Core? Maybe not... Is this useful. Yes, very.

Why do we need this?

As people set up more sophisticated home infrastructure, permissioning systems are critical to make easier to use and clearer. See https://twitter.com/cpc464_a500/status/1254441275773853702/photo/1 . Super cool, right? Each new service imposes additional security risk. I want people to run services safely. Right now the experience for setting up credentials is to manually add then to Bitcoin Core's conf and restart. A system that lets you hand out the cookies and short lived (perhaps even bound to a specific request, number of requests, or amount of btc) is going to help apps integrate more smoothly and decrease master-password behavior.

So I think this is actually a relatively high priority project for end user security, but think it merits some consensus on how much complexity we'd put in core and how much we should build out as e.g. a proxy server.

@maflcko
Copy link
Member

maflcko commented Apr 27, 2020

Adding some permission-discovery RPC so you can see which perms you have

This should be simple and can be filed as a "good first issue". I was thinking we could use the getrpcinfo RPC, but that one might not be whitelisted (chicken-egg-problem). So I'd say to introduce a new getrpcwhitelist.

@JeremyRubin
Copy link
Contributor Author

getrpcinfo should really be soft-renamed to gettaskstatus or something as it's not really information about about the API so much (semantically, it's correct, because the API is an RPC API and a instance of a call is an RPC, but I think in most cases it's better to disambiguate that an RPC can start a task which can be monitored with gettaskstatus.

This would have to be a new RPC to return this info as seeing current tasks could leak information undesirably.

@laanwj
Copy link
Member

laanwj commented Apr 30, 2020

To be honest, I do not like to see too much scope creep here. A simple RPC whitelist per user seems ok to me, as now, but all this complexity in RPC preprocessing does not belong in bitcoin core. It creates a ballooning of maintenance and testing overhead for something only a few people will use.

Agree with @gmaxwell. Forking out for permission checks is out of the question. If you need that level of dynamic programmable access control (does anyone? bitcoind is intended to process mostly public data) a proxy is more appropriate.

@JeremyRubin
Copy link
Contributor Author

@laanwj I don't think anyone's currently proposing shelling out FWIW.

The minimally useful set of credential features to support such a proxy are:

  1. whitelistrpc (done!)
  2. generate temp credential
  3. add permission from credential to credential
  4. restrict credential to specific wallet
  5. figure out what permissions I have

With that I think a proxy can only be needed for advanced circumstances, and we cover most use cases very nicely with what exists in core. And there's not too much complexity around those 5 features to maintain.

Ultimately I think that with @ryanofsky's work a lot of the RPC stuff can be split out anyways, so the maintenance of this stuff is less of a bitcoind concern and more of an RPC server concern. A bulk of the RPCs we care to protect are specifically the non-public data stuff in bitcoind (wallets).

With regards to whether or not people use it, I certainly hope if we build something that supports finer grained permissions that people will use it as it can help reduce attack surface, especially for messy things like setting up a config file with keys for every service that will access the node.

In the longer term, people invariably are going to be hooking up more services to bitcoinds. And do think it's our job to have a coherent story around how users are generating and integrating these credentials into the services they are allowing access. If that's a separate codebase, fine. But that user story needs to exist to narrow case where the user chooses to do something dumb (e.g., unlimited shared credentials that never get revoked or refreshed). If we want this to be something that all apps in the bitcoin ecosystem will use the same format/protocol for getting authorization and credentials, then it does become a standard so that services can integrate in a defined way.

@JeremyRubin
Copy link
Contributor Author

Summarizing some discussion in #19117

Just adding an API for returning whats in the whitelist is insufficient, as the whitelist can contain garbage info (X is allowed, but X is not an RPC). What application developers really need is a method of discovering what RPCs are available to a user, are available on the node but not whitelisted, and what version of the RPC call they are compatible with (in case there is a semantic change or new args).

This transcends just whitelist related usage, it's really useful when trying to support multiple node versions (as such, it makes it slightly time sensitive to release this soon/with backports as any feature discovery tool can only be used on nodes which have the feature discovery API).

@Kixunil
Copy link

Kixunil commented Aug 24, 2020

Since a proxy is mentioned here, FYI, I wrote such a proxy when RPC whitelist was not in Core yet. Will be happy to receive any PRs to add features proposed here. I may implement some of them myself at some point but it will take some time.

sidhujag pushed a commit to syscoin-core/syscoin that referenced this issue Nov 10, 2020
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
Stackout pushed a commit to VeriBlock/vbk-ri-btc that referenced this issue Feb 17, 2022
knst pushed a commit to knst/dash that referenced this issue Jun 7, 2022
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
knst pushed a commit to knst/dash that referenced this issue Jun 8, 2022
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
@bitcoin bitcoin locked and limited conversation to collaborators Dec 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants