feat(endpoint): added extra admin rpc endpoint for sensitive rpc calls
#386
Conversation
This should be handled by an external proxy anyways.
This currently only contains the `MadaraWriteRpc` methods but will be used to encapsulate any sensitive admin methods in the future.
Trantorian1
requested review from
shamsasari,
cchudant,
akhercha and
jbcaron
crates/node/src/cli/rpc.rs
Outdated
| @@ -185,6 +181,16 @@ impl RpcParams { | |||
| SocketAddr::new(listen_addr.into(), self.rpc_port) | |||
| } | |||
|
|
|||
| pub fn addr_admin(&self) -> SocketAddr { | |||
| let listen_addr = if self.rpc_external { | |||
There was a problem hiding this comment.
a specific bind configuration is required for the admin interface
we may want to expose the normal rpc methods externally, while restricting the admin rpc methods to local access, even if they are on a different port
however, ensure that both normal and admin RPC methods remain accessible externally when running inside Docker
There was a problem hiding this comment.
Yep, I'm assuming this is the responsibility of the node operator. For example, it might be worthwhile opening the admin methods to 0.0.0.0, but with a proxy requiring an admin api token and ip whitelist.
There was a problem hiding this comment.
I think I see more clearly what you were talking about. I have updated the rpc-methods cli arg (which is now rpc-endpoints) to handle this:
- Auto:
adminanduserare exposed onlocalhostby default. Ifrpc-externalis true,useris exposed on0.0.0.0butadminstays exposed onlocalhost. - Safe:
adminis disabled and is not exposed, even ifrpc-externalis set to false. - Unsafe:
adminis exposed on0.0.0.0ifrpc-externalis set to true.
crates/node/src/service/rpc/mod.rs
Outdated
| let (rpcs, node_operator) = match (config.rpc_methods, config.rpc_external) { | ||
| (RpcMethods::Safe, _) => (true, false), | ||
| (RpcMethods::Unsafe, _) => (true, true), | ||
| (RpcMethods::Auto, false) => (true, true), | ||
| (RpcMethods::Auto, true) => { | ||
| tracing::warn!( | ||
| "Option `--rpc-external` will hide node operator endpoints. To enable them, please pass \ | ||
| `--rpc-methods unsafe`." | ||
| ); | ||
| (true, false) | ||
| } | ||
| }; |
There was a problem hiding this comment.
ensure this logic is explicitly reflected in the CLI
There was a problem hiding this comment.
I have updated the CLI docs to more accuratly reflect how this works 👍
…edundant mp_block::BlockId
| RpcEndpoints::Auto | RpcEndpoints::Unsafe => (true, true), | ||
| }; | ||
|
|
||
| let (read, write, trace, admin, ws) = (rpcs, rpcs, rpcs, node_operator, rpcs); |
There was a problem hiding this comment.
can you remove this atrocity
(it's my fault originally so i'm allowed to say that ahah)
There was a problem hiding this comment.
would it make more sense if it worked like this:
--rpc-disableddisables the normal rpcs (unchanged)--rpc-externalmakes the normal rpcs external--node-operator-rpcenables the node operator rpcs (disabled by default, but i'm thinking in devnet it should be enabled by default tbh - but maybe for a later pr since we don't have any endpoint that specifically useful for devnet yet, like manual revert and stuff)--unsafe-node-operator-rpc-externalallows the node operator rpcs to listen to 0.0.0.0. this is very necessary in docker setups for example since your reverse proxy will have another local ip
that would mean we remove all of this --rpc-endpoints unsafe nonsense? this --rpc-endpoints thinggy is something i made by taking inspiration from substrate when we switched, that's how they do it - but contrary to them we chose to expose the node operator endpoints on another port, which i think makes this design a bit weird
Pull Request type
What is the current behavior?
Currently, all rpc methods are implemented under a single endpoint. This poses issues as we want to add privilidged methods which allow node operators and developers to manipulate their node at a distance, and currently these methods would be exposed to users as well.
What is the new behavior?
A few changes have been made, in order of importance:
adminendpoint running on port9943. See here for when this endpoint is enabled (perhaps this need to be clearer?). Currently, only theMadarRpcWriteApimethods have been moved there but we plan to add more methods soon.RPC.mdNote
The idea is that node operators and developer will (and should!) be running their own proxy for handling features such as rate limiting, api tokens and the like. Separating admin methods into their own endpoint makes it easier to limit their access in this way.
Does this introduce a breaking change?
Yes. If you were relying on the
madara_addDeclareV0Transactionrpc method, please note that this has been moved to theadminendpoint on port9943.Other information
I think it would be a good idea in the near future to bundle a sample proxy configuration with Madara which node operators and devs can use as a starting point to their own production environment. I am unfamiliar with this topic so if you are already using your own proxy with Madara for the reasons discussed above, please share!