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

One step toward wallet locking #2925

Merged
merged 4 commits into from Nov 11, 2019

Conversation

@darosior
Copy link
Collaborator

darosior commented Aug 8, 2019

[updated 09/27]
This adds a new sensitive_command hook, which allows a plugin to take over any RPC command execution.
This was at first aimed to only restrict the use of sensitive commands, but was made a general purpose RPC hook as suggested by @ZmnSCPxj (#2925 (comment)), it now :

  • allows a plugin (who registered this hook) to return a custom result to a chosen command, hence effectively allowing a plugin to reimplement some RPC
  • allows a plugin (who registered this hook) to return a custom error to a chosen command, hence effectively allowing a plugin to restrict the use of some (sensitive?) RPC

Going further, and off topic of the wallet locking, a plugin registering for the rpc_command hook could take over the plugin start (and cie.) command and extend the plugins possibilities, such as permitting a workaround for hook chaining (#2796).
For example, the plugin registering for rpc_command could register for htlc_accepted and override plugin start, then ask to every new plugin started with plugin start and registering this same hook the permission to accept the HTLC before returning the response to lightningd.
[/updated 09/27]

Next step forward

I plan to make a (static) plugin (to be added here) which would allow users to set up a passphrase and to lock / unlock the sensitive RPC calls (a little bit like completely like bitcoin-core's walletpassphrase mechanism).
I prefered to separate this into multiple PRs.

Another step forward

If we encrypt hsm_secret then even is lightningd is stopped somehow (likely killed) it cannot be restarted without the locking plugin. I don't know exactly how to achieve this final step, or if it is even a good scheme ?

@darosior darosior requested a review from cdecker as a code owner Aug 8, 2019
@darosior darosior force-pushed the darosior:sensitive_hook_rework branch from d5e633b to 58420b4 Aug 8, 2019
@rustyrussell rustyrussell added this to the 0.7.3 milestone Aug 9, 2019
Copy link
Contributor

rustyrussell left a comment

I don't think command filtering should be done at this level: I prefer a proxy, perhaps offered by a plugin, which can do more serious enforcement.

But the idea of a command hook is fine, and offers some nice capabilities, however it should be plumbed in at the jsonrpc.c parse_request() level, before command dispatch. A security plugin should whitelist, rather than blacklist: this is safe if unknown commands are added (eg. plugins).

lightningd/plugin_hook.c Outdated Show resolved Hide resolved
lightningd/plugin_hook.c Outdated Show resolved Hide resolved
lightningd/invoice.c Outdated Show resolved Hide resolved
lightningd/plugin_hook.h Outdated Show resolved Hide resolved
@darosior

This comment has been minimized.

Copy link
Collaborator Author

darosior commented Aug 12, 2019

I don't think command filtering should be done at this level: I prefer a proxy, perhaps offered by a plugin, which can do more serious enforcement.

What more could be done ? (To get ideas)

But the idea of a command hook is fine, and offers some nice capabilities, however it should be plumbed in at the jsonrpc.c parse_request() level, before command dispatch. A security plugin should whitelist, rather than blacklist: this is safe if unknown commands are added (eg. plugins).

I'll amend this to call the hook at parse_request() and to include command alteration.

Copy link
Member

cdecker left a comment

I think we should call the hook with the RPC call payload on all calls, instead of making an arbitrary distinction between sensitive and non-sensitive. That'd give more power to the plugins, while also allowing us not to have to decide on behalf of the user. Putting the call directly in the JSON-RPC dispatch in parse_request seems like a good place.

One thing that I don't quite get is why this has to be synchronous in the first place. Since the internal dispatch will not be called until we get the result of the hook we can just call the hook asynchronously (we might even be able to call it before we start a new DB transaction, hence removing the split transaction across DB transaction issue). Don't get me wrong, I love the generalization of the sync hook call, but I'm wondering whether we should make use of it here.

If we do it asynchronously, we can also get some of the features @ZmnSCPxj wants: upon receiving a call that we'd like to rewrite, we will just call the rewritten RPC ourselves (us being the plugin) and then directly return the result as the result of the original call. No need to intercept the original call, rewrite, reinject into the dispatch, and then have the dispatch return the result for the rewritten call.

lightningd/plugin_hook.c Outdated Show resolved Hide resolved
lightningd/invoice.c Outdated Show resolved Hide resolved
lightningd/gossip_control.c Outdated Show resolved Hide resolved
@darosior

This comment has been minimized.

Copy link
Collaborator Author

darosior commented Aug 13, 2019

One thing that I don't quite get is why this has to be synchronous in the first place. Since the internal dispatch will not be called until we get the result of the hook we can just call the hook asynchronously (we might even be able to call it before we start a new DB transaction, hence removing the split transaction across DB transaction issue). Don't get me wrong, I love the generalization of the sync hook call, but I'm wondering whether we should make use of it here.

To be really honest, I first expected hooks to be asynchronous and had a hard time debugging. I then understood that I would (in the "command is restricted" case) call command_fail(cmd, ... when there was no cmd anymore. I haven't checked how other hooks handle this though.
Why aren't all hooks synchronous by default ?

If we do it asynchronously, we can also get some of the features @ZmnSCPxj wants: upon receiving a call that we'd like to rewrite, we will just call the rewritten RPC ourselves (us being the plugin) and then directly return the result as the result of the original call. No need to intercept the original call, rewrite, reinject into the dispatch, and then have the dispatch return the result for the rewritten call.

In that case, what would happen to the ongoing request that called us ? Would we return a command_fail() but actually making a new call with different parameters after ? I like the synchronous way, because it either success, fail, or be altered (I've implemented this one even if I don't see a case were we would take over an user's decision for now).

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Aug 13, 2019

To be really honest, I first expected hooks to be asynchronous and had a hard time debugging. I then understood that I would (in the "command is restricted" case) call command_fail(cmd, ... when there was no cmd anymore. I haven't checked how other hooks handle this though.
Why aren't all hooks synchronous by default ?

They were, but somewhere along the way they were made asynchronous, which is really strange and contradicts the documentation, but may be a blessing since it actually allows us to make RPC calls which would otherwise not be possible.

I am almost convinced now that only making hooks synchronous (read: lightningd makes absolutely no progress while the plugins ruminates over the hook call) if the use-case requires it (db_write for example would require the DB transaction to be split between statements and commit, which is really an edgecase).

In that case, what would happen to the ongoing request that called us ? Would we return a command_fail() but actually making a new call with different parameters after ? I like the synchronous way, because it either success, fail, or be altered (I've implemented this one even if I don't see a case were we would take over an user's decision for now).

The dispatch code in parse_request parses the JSON-RPC call, stashes it in a hook call payload, calls the hook and exits. Upon receiving a response from the plugin, the callback gets the stashed call as well as the plugin's response and can perform the actual dispatch to the registered dispatch function for that RPC method. While the plugin is thinking about what to do with the JSON-RPC call, the connection just sits idle, no intermediate result is called (hence no command_fail or command_success), and we just wait for the plugin to make up its mind.

We have a couple of similar situations (basically everywhere a JSON-RPC call results in further queries to other daemons such as listnodes below:

static struct command_result *json_listnodes(struct command *cmd,
const char *buffer,
const jsmntok_t *obj UNNEEDED,
const jsmntok_t *params)
{
u8 *req;
struct node_id *id;
if (!param(cmd, buffer, params,
p_opt("id", param_node_id, &id),
NULL))
return command_param_failed();
req = towire_gossip_getnodes_request(cmd, id);
subd_req(cmd, cmd->ld->gossip, req, -1, 0, json_getnodes_reply, cmd);
return command_still_pending(cmd);
}

We start processing the call, then issue the followup query that will eventually return the results (this is equivalent to the hook-call) and signal that the RPC call is pending using the call to command_still_pending (this just marks the call as intentionally suspended in order not to upset our sanity checks).

@darosior

This comment has been minimized.

Copy link
Collaborator Author

darosior commented Aug 13, 2019

The dispatch code in parse_request parses the JSON-RPC call, stashes it in a hook call payload, calls the hook and exits. Upon receiving a response from the plugin, the callback gets the stashed call as well as the plugin's response and can perform the actual dispatch to the registered dispatch function for that RPC method. While the plugin is thinking about what to do with the JSON-RPC call, the connection just sits idle, no intermediate result is called (hence no command_fail or command_success), and we just wait for the plugin to make up its mind.

Thank you for the detailed explanation, it makes more sense, I'll try it that way.

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Sep 6, 2019

Added the state::shelved tag to signal that we may be trying another solution, but don't want to close this PR right away 😉

@darosior

This comment has been minimized.

Copy link
Collaborator Author

darosior commented Sep 7, 2019

Added the state::shelved tag to signal that we may be trying another solution, but don't want to close this PR right away wink

I'm still working on this, I've just started something else (almost finished) that I wanted to finish before coming back to this.

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Sep 8, 2019

I'm still working on this, I've just started something else (almost finished) that I wanted to finish before coming back to this.

No problem, I'm just tagging these so I don't get the feeling that our PR stack is growing and I can stop checking this one every day 😉

@darosior darosior force-pushed the darosior:sensitive_hook_rework branch 5 times, most recently from 5bcc8e7 to 8665f3b Sep 8, 2019
@cdecker cdecker removed this from the 0.7.3 milestone Sep 26, 2019
@darosior darosior force-pushed the darosior:sensitive_hook_rework branch 5 times, most recently from 0b8a383 to 8ed91a4 Sep 26, 2019
@darosior

This comment has been minimized.

Copy link
Collaborator Author

darosior commented Sep 27, 2019

Finally got the time to fix this. Updated the OP.
(Sorry @rustyrussell I again use a timer as a workaround for a command_still_pending.. :-))

@cdecker cdecker removed the state::shelved label Oct 2, 2019
@darosior darosior mentioned this pull request Oct 7, 2019
@niftynei niftynei added this to the 0.7.4 milestone Oct 10, 2019
@darosior darosior force-pushed the darosior:sensitive_hook_rework branch from 0e98e30 to 350b294 Oct 28, 2019
@darosior

This comment has been minimized.

Copy link
Collaborator Author

darosior commented Oct 28, 2019

Thanks for the review ! Fixed the method and params serialization.

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 1, 2019

ACK 350b294

@cdecker
cdecker approved these changes Nov 1, 2019
@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 1, 2019

@rustyrussell is your request for changes still valid? If not I'll merge this 😉

@darosior darosior force-pushed the darosior:sensitive_hook_rework branch from 350b294 to 2d7c4d6 Nov 3, 2019
@darosior

This comment has been minimized.

Copy link
Collaborator Author

darosior commented Nov 3, 2019

Forgot to add documentation for the hook, documented it and rebased on top of master.

By the way @cdecker about the caller authentication, is there any drawback in making the authentication field part of the params instead of the TL object ? That way the flow of the locker plugin would be:

  • If locked
    • If there is an auth field in the params
      • Check the cookie auth
    • Else
      • Send a custom error
  • Else
    • Just response with continue

EDIT: For the Else in the first branch we could even have a restricted mode..

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 4, 2019

By the way @cdecker about the caller authentication, is there any drawback in making the authentication field part of the params instead of the TL object ?

The downside of that would be that we are mixing call parameters and authentication mechanism, whereas they really are orthogonal. It's certainly doable, by having the client inject authentication parameters and the auth plugin filtering them out again, but it feels really awkward.

@darosior

This comment has been minimized.

Copy link
Collaborator Author

darosior commented Nov 4, 2019

Ok, I think I'll go for a violation of the JSONRPC specs then :D

Copy link
Contributor

rustyrussell left a comment

Ack 2d7c4d6

This is really neat! I'll work on getting rid of that annoying timer ;)

json_add_tok(result, json_strdup(tmpctx, buffer, t), t, buffer);
json_object_end(result);
break;

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Nov 5, 2019

Contributor

OK, since tok->type should always be one of these types, change these breaks to returns, except the last JSMN_UNDEFINED. Then abort() at the bottom of the function.

This comment has been minimized.

Copy link
@darosior

darosior Nov 6, 2019

Author Collaborator

Ok

This comment has been minimized.

Copy link
@darosior

darosior Nov 6, 2019

Author Collaborator

So, a JSMN_UNDEFINED tok basically means "bad json" ?

lightningd/jsonrpc.c Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Nov 5, 2019

Ack 2d7c4d6

This is really neat! I'll work on getting rid of that annoying timer ;)

diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c
index 2562d7627..a68ffde2c 100644
--- a/lightningd/jsonrpc.c
+++ b/lightningd/jsonrpc.c
@@ -775,11 +775,12 @@ parse_request(struct json_connection *jcon, const jsmntok_t tok[])
        /* Duplicate since we might outlive the connection */
        rpc_hook->buffer = tal_dup_arr(rpc_hook, char, jcon->buffer, tal_count(jcon->buffer), 0);
        rpc_hook->request = tal_dup_arr(rpc_hook, const jsmntok_t, tok, tal_count(tok), 0);
-       /* Prevent a race between was_pending and still_pending */
-       new_reltimer(c->ld->timers, rpc_hook, time_from_msec(1),
-                    call_rpc_command_hook, rpc_hook);
+       call_rpc_command_hook(rpc_hook);
 
-       return command_still_pending(c);
+       /* Could have already completed the request, or it could be pending.
+        * Hard to plumb through the layers of callbacks, and caller doesn't
+        * care. */
+       return NULL;
 }
 
 /* Mutual recursion */
@darosior darosior force-pushed the darosior:sensitive_hook_rework branch 2 times, most recently from 8b31bda to 38d77bc Nov 6, 2019
@darosior

This comment has been minimized.

Copy link
Collaborator Author

darosior commented Nov 6, 2019

Thanks for the patch !

Replaced the cherry-picking of fields by a json_add_tok().
Made @bitcoin-bot /changelog happy :-)

@darosior darosior force-pushed the darosior:sensitive_hook_rework branch 2 times, most recently from 491a11a to 3d0dac2 Nov 6, 2019
@darosior

This comment has been minimized.

Copy link
Collaborator Author

darosior commented Nov 6, 2019

Ok so I removed your patch because otherwise valgrind tests would not pass if there is no plugin registered for the rpc_command hook.

Hope the timer isn't just hiding the stuff to both me and valgrind ?...

doc/PLUGINS.md Outdated Show resolved Hide resolved
darosior added 3 commits Sep 27, 2019
The 'rpc_command' hook allows a plugin to take over any RPC command.
It sends the complete JSONRPC request to the plugin, which can then respond
with :
- {'continue'}: executes the command normally
- {'replace': {a_jsonrpc_request}}: replaces the request made
- {'return': {'result': {}}}: send a custom response
- {'return': {'error': {}}}: send a custom error

This way, a plugin can modify (/reimplement) or restrict the usage of
any of `lightningd`'s commands.

Changelog-Added: Plugin: A new plugin hook, `rpc_command` allows a plugin to take over `lightningd` for any RPC command.
@darosior darosior force-pushed the darosior:sensitive_hook_rework branch from 3d0dac2 to 640f949 Nov 7, 2019
@darosior

This comment has been minimized.

Copy link
Collaborator Author

darosior commented Nov 7, 2019

Changed the bad json in the "continue" case. Here is the patch diff

diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c
index 4e534d242..82c445f3e 100644
--- a/lightningd/jsonrpc.c
+++ b/lightningd/jsonrpc.c
@@ -623,16 +623,23 @@ static void
 rpc_command_hook_callback(struct rpc_command_hook_payload *p,
                           const char *buffer, const jsmntok_t *resulttok)
 {
-       const jsmntok_t *tok, *method, *params, *custom_return;
+       const jsmntok_t *tok, *method, *params, *custom_return, *tok_continue;
        struct json_stream *response;
+       bool exec;
 
        params = json_get_member(p->buffer, p->request, "params");
 
        /* If no plugin registered, just continue command execution. Same if
         * the registered plugin tells us to do so. */
-       if (buffer == NULL || json_tok_streq(buffer, resulttok, "continue"))
-               return was_pending(command_exec(p->cmd->jcon, p->cmd, p->buffer,
+       if (buffer == NULL)
+           return was_pending(command_exec(p->cmd->jcon, p->cmd, p->buffer,
                                                p->request, params));
+       else {
+               tok_continue = json_get_member(buffer, resulttok, "continue");
+               if (tok_continue && json_to_bool(buffer, tok_continue, &exec) && exec)
+                       return was_pending(command_exec(p->cmd->jcon, p->cmd, p->buffer,
+                                                       p->request, params));
+       }
 
        /* If the registered plugin did not respond with continue,
         * it wants either to replace the request... */
@cdecker
cdecker approved these changes Nov 8, 2019
@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 11, 2019

ACK 640f949

@cdecker cdecker merged commit 6427ccb into ElementsProject:master Nov 11, 2019
4 checks passed
4 checks passed
bitcoin-bot/acks Acks by cdecker
bitcoin-bot/changelog This PR has at least one changelog entry
bitcoin-bot/fixups PR does not contain unsquashed fixups
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@darosior darosior deleted the darosior:sensitive_hook_rework branch Nov 25, 2019
@darosior darosior mentioned this pull request Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.