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: Add getrpcwhitelist method #19117

Closed
wants to merge 2 commits into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented May 31, 2020

Revive #18827

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

Example:

./src/bitcoin-cli -regtest getrpcwhitelist
{
  "methods": [
    "getbalance",
    "getrpcwhitelist",
    "getwalletinfo"
  ]
}

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.

@MarcoFalke
Copy link
Member

MarcoFalke commented May 31, 2020

ACK after documenting the circular dep in the linter:

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

@DrahtBot
Copy link
Contributor

DrahtBot commented May 31, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

@luke-jr
Copy link
Member Author

luke-jr commented May 31, 2020

@MarcoFalke Done

@bitcoin bitcoin locked as spam and limited conversation to collaborators May 31, 2020
@bitcoin bitcoin unlocked this conversation May 31, 2020
@promag
Copy link
Member

promag commented May 31, 2020

Uses a JSON Object for methods for future extensions

Any idea in mind?

@MarcoFalke
Copy link
Member

MarcoFalke commented May 31, 2020

Any idea in mind?

The developer notes recommend it.

@bitcoin bitcoin deleted a comment from kantornnam Jun 1, 2020
@bitcoin bitcoin deleted a comment from kantornnam Jun 1, 2020
@bitcoin bitcoin deleted a comment from kantornnam Jun 1, 2020
@bitcoin bitcoin deleted a comment from kantornnam Jun 1, 2020
@bitcoin bitcoin deleted a comment from kantornnam Jun 1, 2020
@bitcoin bitcoin deleted a comment from kantornnam Jun 1, 2020
@bitcoin bitcoin deleted a comment from kantornnam Jun 1, 2020
@bitcoin bitcoin deleted a comment from kantornnam Jun 1, 2020
@bitcoin bitcoin deleted a comment from kantornnam Jun 1, 2020
@bitcoin bitcoin deleted a comment from kantornnam Jun 1, 2020
@bitcoin bitcoin deleted a comment from kantornnam Jun 1, 2020
@promag
Copy link
Member

promag commented Jun 1, 2020

@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
Copy link
Member

MarcoFalke commented Jun 1, 2020

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.

Copy link
Member

@promag promag left a comment

I think we could name this getrpccredentials

I have pushed e8b18af with the above suggestion.


const std::set<std::string>& GetWhitelistedRpcs(const std::string& user_name)
{
return g_rpc_whitelist.at(user_name);
Copy link
Member

@promag promag Jun 2, 2020

Choose a reason for hiding this comment

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

This fails when user is not in the map.

Copy link
Member Author

@luke-jr luke-jr Jun 2, 2020

Choose a reason for hiding this comment

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

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

Copy link

@Kixunil Kixunil Jun 5, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

@MarcoFalke MarcoFalke Jun 5, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member

@promag promag Jun 5, 2020

Choose a reason for hiding this comment

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

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

Copy link

@Kixunil Kixunil Jun 5, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member

@MarcoFalke MarcoFalke Jun 5, 2020

Choose a reason for hiding this comment

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

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

Copy link

@Kixunil Kixunil Jun 5, 2020

Choose a reason for hiding this comment

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

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

@jonatack
Copy link
Member

jonatack commented Jun 3, 2020

Concept ACK. Will review.

@Kixunil
Copy link

Kixunil commented Jun 5, 2020

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
Copy link
Member

promag commented Jun 5, 2020

@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
Copy link

Kixunil commented Jun 5, 2020

@promag fair enough, thanks for clarifying!

@luke-jr
Copy link
Member Author

luke-jr commented Jun 5, 2020

@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
Copy link
Member

MarcoFalke commented Jun 5, 2020

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

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

@luke-jr
Copy link
Member Author

luke-jr commented Jun 5, 2020

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

@JeremyRubin
Copy link
Contributor

JeremyRubin commented Jun 5, 2020

@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
Copy link
Member

promag commented Jun 6, 2020

@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
Copy link
Contributor

JeremyRubin commented Jun 6, 2020

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
Copy link
Member

promag commented Jun 6, 2020

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

@Kixunil
Copy link

Kixunil commented Jun 7, 2020

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

@promag
Copy link
Member

promag commented Jun 11, 2020

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
Copy link

Kixunil commented Jun 11, 2020

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

@luke-jr
Copy link
Member Author

luke-jr commented Aug 20, 2020

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

@luke-jr luke-jr closed this Aug 20, 2020
@JeremyRubin
Copy link
Contributor

JeremyRubin commented Aug 21, 2020

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

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants