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

cln-grpc-plugin: Secure access to your node over the network #5013

Merged
merged 15 commits into from
Mar 30, 2022

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Jan 18, 2022

This is the final PR in the cln-* series. It uses all the primitives we built in the previous 3 PRs and uses them to expose the JSON-RPC over grpc, with mTLS authentication builtin.

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

I was searching about the plugin architecture, to think about how to implement the grpc interface for my dart wrapper. In the meanwhile I left one comment (very very stupid).

Half of the code is the tls config, and I'm not the right person to review it

plugins/grpc-plugin/Cargo.toml Show resolved Hide resolved
@rustyrussell
Copy link
Contributor

rustyrussell commented Mar 19, 2022

I think experimental is too harsh: if it requires a config option to activate (grpc-port AFAICT), that's enough. IMHO.

Plugins generally don't have the same requirements as core functionality IMHO. I wonder if we should call it grpc or cln-grpc instead of grpc-plugin?

So please add a Changelog-Added: plugins: grpcXXX first class GRPC interface for controlling nodes (RUST)

Also nice to have:

  1. Rust plugins should print Something Nice when you run them on cmdline, like Python ones do.
  2. rust and rustfmt dependencies should be listed in INSTALL.md, at least as optional.

@cdecker cdecker force-pushed the cln-grpc-plugin branch 2 times, most recently from d0e0fa5 to a56eb26 Compare March 23, 2022 10:55
@cdecker
Copy link
Member Author

cdecker commented Mar 25, 2022

@rustyrussell I have your branch merged in and it compiles. Will spend today adding more types since they can be difficult to add later, and add some tests to verify the generated conversions are working ok 👍

We are about to generate the python grpc bindings, but only when we
have Rust enabled.
Currently still unencrypted, but will get its mTLS authentication in
the next commits.
If we aren't using the correct certificates we should reject the
connections during the mTLS connection setup. This test tries to
connect with the wrong client cert to the node, and the server will
reject it.
Changelog-Added: cln-grpc-plugin: The plugin can be configured to listen on a specific port using the `grpc-port` option
Suggested-by: Rusty Russell <@rustyrussell>
Changelog-Added: plugins: `cln-grpc` first class GRPC interface for remotely controlling nodes over mTLS authentication
"Which port should the grpc plugin listen for incoming connections?",
))
.start()
.await?;

let bind_port = match plugin.option("grpc-port") {
Some(options::Value::Integer(-1)) => {
log::warn!("`grpc-port` option is not configured, exiting.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably just log::info? It's a great plugin, but every non-activated plugin should probably not shout to be activated :)

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Trivial change requested... I would merge anyway, but I know more is coming...

For now we don't want to autostart.

Suggested-by: Rusty Russell <@rustyrussell>
@cdecker
Copy link
Member Author

cdecker commented Mar 28, 2022

Fixed up. The remaining grpc methods are in a separate PR, so feel free to merge this one :-)

e.g.
```
lightningd-1: 2022-03-28T11:02:12.476Z DEBUG   plugin-cln-grpc: add_pem_file processed 1 valid and 0 invalid certs
lightningd-1: 2022-03-28T11:02:12.478Z DEBUG   plugin-cln-grpc: Connecting to \"lightning-rpc\" and serving grpc on 0.0.0.0:36331
lightningd-1: 2022-03-28T11:02:12.478Z DEBUG   connectd: REPLY WIRE_CONNECTD_ACTIVATE_REPLY with 0 fds
lightningd-1: 2022-03-28T11:02:12.478Z INFO    lightningd: --------------------------------------------------
lightningd-1: 2022-03-28T11:02:12.478Z INFO    lightningd: Server started with public key
```

Which means we don't see it, since start() swallows it:

```
>               raise TimeoutError('Unable to find "{}" in logs.'.format(exs))
E               TimeoutError: Unable to find "[re.compile('serving grpc on 0.0.0.0:')]" in logs.
```

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
Copy link
Contributor

Pushed a simple test race fix.

Ack 343961d

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ack 343961d

(with just some small comment!)

plugins/grpc-plugin/Cargo.toml Show resolved Hide resolved
@rustyrussell
Copy link
Contributor

Ack a47770d

@rustyrussell rustyrussell merged commit 0cbf918 into master Mar 30, 2022
@rustyrussell rustyrussell deleted the cln-grpc-plugin branch August 15, 2022 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants