RPC: Add getrpcwhitelist method

luke-jr commented at 0:27 am on May 31, 2020: member

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 #18822

Uses a JSON Object for methods for future extensions.

Not clear what it should do for users that don’t have a whitelist (ie, can access all methods). Currently errors I think.

fanquake added the label RPC/REST/ZMQ on May 31, 2020

luke-jr force-pushed on May 31, 2020

MarcoFalke commented at 0:37 am on May 31, 2020: member

ACK after documenting the circular dep in the linter:

0A new circular dependency in the form of "httprpc -> rpc/server -> httprpc" appears to have been introduced.

DrahtBot commented at 5:37 am on May 31, 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:

#17805 (test: Add test for rpcwhitelistdefault by emilengler)

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.

RPC: Add getrpcwhitelist method 985f0d8992

RPC: getrpcwhitelist: Return methods as a JSON Object for future expansion to sub-call permissions 94fad2edec

luke-jr force-pushed on May 31, 2020

luke-jr commented at 5:39 pm on May 31, 2020: member

@MarcoFalke Done

MarcoFalke locked this on May 31, 2020

MarcoFalke unlocked this on May 31, 2020

promag commented at 9:57 pm on May 31, 2020: member

Uses a JSON Object for methods for future extensions

Any idea in mind?

MarcoFalke commented at 10:15 pm on May 31, 2020: member

Any idea in mind?

The developer notes recommend it.

jnewbery deleted a comment on Jun 1, 2020

promag commented at 10:59 pm on June 1, 2020: member

@MarcoFalke I mean nothing comes to my mind that could be assigned to each whitelist’ed method.

I think we could name this getrpccredentials and it could return { "whitelist": ["..."] } or {} for when the user doesn’t have a whitelist. IMO this is more extensible.

MarcoFalke commented at 11:10 pm on June 1, 2020: member

Nothing comes to my mind either, but the key-value pair make the return value a bit more self-documenting. Also, there should be no downside just following the dev notes.

in src/httprpc.cpp:325 in 94fad2edec

318@@ -319,3 +319,8 @@ void StopHTTPRPC()
319         httpRPCTimerInterface.reset();
320     }
321 }
322+
323+const std::set<std::string>& GetWhitelistedRpcs(const std::string& user_name)
324+{
325+    return g_rpc_whitelist.at(user_name);

promag commented at 10:26 am on June 2, 2020:

This fails when user is not in the map.

luke-jr commented at 6:07 pm on June 2, 2020:

Not clear what it should do for users that don’t have a whitelist (ie, can access all methods). Currently errors I think.

Kixunil commented at 8:09 am on June 5, 2020:

I believe it should return the list of all available RPC methods. It’s consistent and it’d enable feature detection. See #19087 for more discussion about feature detection.

MarcoFalke commented at 11:53 am on June 5, 2020:

If the rpc whitelist is empty, one could call help to get all methods. But maybe just returning the whole set here seems appropriate. How to deal with hidden RPCs, though?

promag commented at 11:58 am on June 5, 2020:

I think it should return empty - no whitelist is defined for the user.

Kixunil commented at 2:02 pm on June 5, 2020:

Hmm, should it be designed for simplicity of bitcoind or practicality of clients? I suppose this feature will be most useful for auto-detecting available features, but maybe I’m missing something?

MarcoFalke commented at 2:15 pm on June 5, 2020:

I guess that makes sense, but I think hidden methods should not be included by default.

Kixunil commented at 2:22 pm on June 5, 2020:

Do I understand correctly that the hidden methods are used for testing only?

promag commented at 10:32 am on June 2, 2020: member

I think we could name this getrpccredentials

I have pushed e8b18af58523635cf5b27b792a14925748030488 with the above suggestion.

jonatack commented at 6:58 pm on June 3, 2020: member

Concept ACK. Will review.

Kixunil commented at 8:06 am on June 5, 2020: none

I do have an idea for extending the whitelist (I actually wanted to suggest it before I learned about whitelist being extendable!): each RPC call could be versioned using semver (two numbers should be enough) so that then the clients can very precisely detect compatibility. New options are added to various RPC methods over time, so it’d be great for the client to know what the node supports. I’d even suggest starting right away and give each method version “1.0”.

The advantage over just versioning whole RPC is higher flexibility - one breaking method doesn’t have to affect compatibility with a client that doesn’t use it. It increases maintenance burden, but hopefully not by too much? Bumping a number whenever RPC method is changed doesn’t seem too bad.

promag commented at 2:23 pm on June 5, 2020: member

@Kixunil this PR is about returning the whitelist of the current user, not about feature detection nor about interface versioning - IMO server version is enough for the client - so please lets discuss it in the right place.

Kixunil commented at 3:41 pm on June 5, 2020: none

@promag fair enough, thanks for clarifying!

luke-jr commented at 4:27 pm on June 5, 2020: member

@promag “Server versioning” is not a good method for feature discovery… Knots for example has many RPCs before they’re available in Core.

And if there are multiple ways to do something, now you’d have 2 steps: first, figure out if the version supports the “better” way, then check if the “better” way is not excluded by a whitelist.

MarcoFalke commented at 4:35 pm on June 5, 2020: member

Knots for example has many RPCs before they’re available in Core.

Then, the help RPC should already return them, no?

luke-jr commented at 5:43 pm on June 5, 2020: member

Yes, my response was to @promag’s proposal that server versioning is a good way to do it. It isn’t.

JeremyRubin commented at 11:51 pm on June 5, 2020: contributor

@promag returning the whitelist is kinda fundamentally about feature detection, and that leads to an interesting note…

Whitelists can contain RPCs that don’t exist, so it’s possible to misuse the output here if you assume the whitelist is valid rpcs.

We probably do want to figure out a robust feature detection call and filter against the whitelists…

promag commented at 0:08 am on June 6, 2020: member

@JeremyRubin IIUC this RPC exposes configuration data and, like you say, might reference inexistent RPC, especially if one goes back and forth different versions with same configuration.

We probably do want to figure out a robust feature detection call and filter against the whitelists…

Right, that’s why I suggested to discuss feature discovery elsewhere.

JeremyRubin commented at 0:51 am on June 6, 2020: contributor

I think we shouldn’t make an RPC for whitelists potentially, we should just add a feature discovery API (which is why it’s relevant to discuss here)

promag commented at 1:07 am on June 6, 2020: member

I wouldn’t consider them exclusive but in that case it makes sense to discuss here.

Kixunil commented at 7:18 am on June 7, 2020: none

What’s the use case for purely sending the whitelist anyway? How it could be useful without other things like feature detection?

promag commented at 10:56 am on June 11, 2020: member

What’s the use case for purely sending the whitelist anyway? How it could be useful without other things like feature detection? @Kixunil please see #12248 (comment) for related discussion.

Looks like we should take a different approach then? (to avoid whitelist usage and combine with feature discovery)

Kixunil commented at 5:52 pm on June 11, 2020: none

@promag so do I understand correctly the reason why write just whitelist is because it’s simple?

luke-jr commented at 11:41 pm on August 20, 2020: member

Seems like a generic feature discovery method is a better approach, so closing this

luke-jr closed this on Aug 20, 2020

JeremyRubin commented at 0:06 am on August 21, 2020: contributor

@luke-jr do you have a reference on this? I like feature discovery!

When we discussed in core dev meeting everyone was nacking any generic feature discovery feature.

DrahtBot locked this on Feb 15, 2022

RPC: Add getrpcwhitelist method #19117

Conflicts