rpc: added getrpcwhitelist method

brakmic commented at 12:53 pm on April 30, 2020: contributor

This PR implements a new RPC, getrpcwhitelist, that returns whitelisted methods for the calling user.

Example:

0./src/bitcoin-cli -regtest getrpcwhitelist
1{
2  "methods": [
3    "getbalance",
4    "getrpcwhitelist",
5    "getwalletinfo"
6  ]
7}

A new functional test, rpc_getrpcwhitelist, and a release-notes entry are included.

Fixes https://github.com/bitcoin/bitcoin/issues/18822

fanquake added the label RPC/REST/ZMQ on Apr 30, 2020

laanwj commented at 1:40 pm on April 30, 2020: member

Agree with making this list a separate RPC call (it could potentially be long), but what about putting user on the normal getrpcinfo? It seems general enough.

brakmic commented at 1:43 pm on April 30, 2020: contributor

Agree with making this list a separate RPC call (it could potentially be long), but what about putting user on the normal getrpcinfo? It seems general enough.

Ok. Should I open a new PR or change it en passant?

MarcoFalke commented at 1:45 pm on April 30, 2020: member

getrpcinfo leaks other information, so it might not be whitelisted, see #12248 (comment) . I agree with the suggestion, but it should be done separate form introducing this new call.

brakmic commented at 1:49 pm on April 30, 2020: contributor

getrpcinfo leaks other information, so it might not be whitelisted, see #12248 (comment) . I agree with the suggestion, but it should be done separate form introducing this new call.

Ok, will remove user from this rpc and add it to getrpcinfo in a separate PR.

promag commented at 1:49 pm on April 30, 2020: member

So help and getrpcwhitelist are always available, or are there more “public” calls?

brakmic commented at 1:50 pm on April 30, 2020: contributor

So help and getrpcwhitelist are always available, or are there more “public” calls?

I did not make getrpcwhitelist always available. But I can make it, if this should be the case.

–EDIT: Sorry, I was a bit misleading: getrpcwhitelist is available by default, but when user is setting a whitelist in the config, then getrpcwhitelist must be added there as well.

DrahtBot commented at 5:57 pm on April 30, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18871 (rpc: prevent circular deps by extracting into separate include by brakmic)
#17805 (test: Add test for rpcwhitelistdefault by emilengler)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams 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.

JeremyRubin commented at 5:58 am on May 1, 2020: contributor

getrpcinfo should not be made public by default. Concept ACK on the new RPC though.

JeremyRubin commented at 5:59 am on May 1, 2020: contributor

~~The new RPC should return the permissions for the current user only; returning the permissions for other users should not be allowed.~~ nvm

in src/httprpc.cpp:328 in 4f856c7a3e outdated

321@@ -322,3 +322,8 @@ void StopHTTPRPC()
322         httpRPCTimerInterface.reset();
323     }
324 }
325+
326+const std::set<std::string>& GetWhitelistedRpcs(const std::string& userName)
327+{
328+    return g_rpc_whitelist[userName];

MarcoFalke commented at 10:55 am on May 1, 2020:

0    return g_rpc_whitelist.at(userName);

Otherwise there is no guarantee that the whitelist will not get corrupted

brakmic commented at 11:08 am on May 1, 2020:

Thanks!

in src/rpc/server.cpp:21 in 776d11ab43 outdated

17@@ -18,6 +18,7 @@
18 #include <memory> // for unique_ptr
19 #include <unordered_map>
20 
21+const std::set<std::string>& GetWhitelistedRpcs(const std::string&);

promag commented at 11:15 am on May 1, 2020:

Didn’t include src/httprpc.h because would add a circular dependency?

brakmic commented at 11:17 am on May 1, 2020:

Yes, because travis failed on two builds with such an error.

promag commented at 11:50 pm on May 3, 2020:

I think it’s probably better to add the #include and update test/lint/lint-circular-dependencies.sh.

promag commented at 8:38 pm on May 4, 2020:

Was this resolved by mistake?

brakmic commented at 8:43 pm on May 4, 2020:

I don’t think I should simply change the linter-checks because of a single function. Maybe someone more qualified should be asked for advice.

sipa commented at 8:46 pm on May 4, 2020:

Even nicer would be to solve the circular dependency in the first place. Using a forward declare here instead of an include isn’t a solution - it’s just hiding it from the linter.

If it’s too much work to resolve that in this PR, my suggestion is being explicit about it and adding an exception to the linter.

brakmic commented at 9:02 pm on May 4, 2020:

I’ll try to extract the circular structures into another include and then recombine everything. Maybe better in a separate PR?

brakmic commented at 9:32 pm on May 4, 2020:

So, I have extracted the following classes and functions into a new include rpc/interfaces.h

0class RPCTimerBase
1class RPCTimerInterface
2class CRPCCommand
3class CRPCTable
4std::string JSONRPCExecBatch(const JSONRPCRequest& jreq, const UniValue& vReq);
5void RPCSetTimerInterface(RPCTimerInterface *iface);
6void RPCSetTimerInterfaceIfUnset(RPCTimerInterface *iface);
7void RPCUnsetTimerInterface(RPCTimerInterface *iface);

A test run with circular-dependencies.pyscript shows no httprpc->server->httprpc anymore (but there are other unrelated circular dependencies still)

I have thought about extracting their implementations in rpc/server.cpp as well, but as this would look even more invasive, I’ll remain with this single include file for now.

I think, I should open a separate PR and let others discuss about it. And if it gets merged, I’ll rebase this PR afterwards.

brakmic commented at 3:39 pm on May 6, 2020:

I have outsourced this task into this PR (draft): https://github.com/bitcoin/bitcoin/pull/18871

brakmic commented at 3:40 pm on May 6, 2020:

Will resolve this conversation if you don’t mind. In case my other PR gets merged we can continue from there (with rebase etc.).

in src/httprpc.cpp:326 in 776d11ab43 outdated

321@@ -322,3 +322,8 @@ void StopHTTPRPC()
322         httpRPCTimerInterface.reset();
323     }
324 }
325+
326+const std::set<std::string>& GetWhitelistedRpcs(const std::string& userName)

luke-jr commented at 5:15 pm on May 4, 2020:

Nit: user_name

luke-jr commented at 5:16 pm on May 4, 2020:

Probably can use unordered_set, but maybe that’s a refactor for g_rpc_whitelist…

JeremyRubin commented at 6:03 pm on May 4, 2020:

@luke-jr I think unordered_set will be slower in most cases & use more memory, hence choosing set.

brakmic commented at 6:35 pm on May 4, 2020:

Thanks. Adapted.

in src/rpc/server.cpp:247 in 776d11ab43 outdated

242+                "\nReturns whitelisted RPCs for the current user.\n",
243+                {},
244+                RPCResult{
245+                    RPCResult::Type::OBJ, "", "",
246+                    {
247+                        {RPCResult::Type::ARR, "methods", "List of RPCs that the user is allowed to call",

luke-jr commented at 5:17 pm on May 4, 2020:

Is there a case for making it an Object with possible details of more fine-grained calls? Or is it safe to assume we won’t go beyond the method level for permissions?

JeremyRubin commented at 6:04 pm on May 4, 2020:

I think that’s a decent idea for future-proofing the return type, but there is some disagreement of the level of permission control we want to allow in core v.s. a layer on top.

luke-jr approved

luke-jr commented at 5:18 pm on May 4, 2020: member

utACK, some nits/questions

rpc: added getrpcwhitelist method 2893a5f702

brakmic closed this on May 10, 2020

MarcoFalke commented at 8:12 pm on May 10, 2020: member

I liked this and I am looking forward to seeing this reopened.

JeremyRubin commented at 4:51 am on May 11, 2020: contributor

@brakmic any context on why you closed it out? Do you want someone to pick it up or are you planning on restarting it?

Happy to help you move it along (review process in Bitcoin Core can be slow!)

DrahtBot locked this on Feb 15, 2022

rpc: added getrpcwhitelist method #18827

Conflicts