From 4ab09f7cfb1556ce0a59980702f0761b9d4ac7ec Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sat, 16 Jul 2022 22:48:27 +0930 Subject: [PATCH] commando: add support for parameters by array, parameter count. Awkward to filter, but they're really practical for many commands. Signed-off-by: Rusty Russell --- doc/lightning-commando-rune.7.md | 61 ++++++++++++++++++++++++++- doc/schemas/commando.request.json | 12 +++++- plugins/commando.c | 69 ++++++++++++++++++++----------- tests/test_plugin.py | 28 +++++++------ 4 files changed, 128 insertions(+), 42 deletions(-) diff --git a/doc/lightning-commando-rune.7.md b/doc/lightning-commando-rune.7.md index 23a8e9c6e..ca31e2e96 100644 --- a/doc/lightning-commando-rune.7.md +++ b/doc/lightning-commando-rune.7.md @@ -33,7 +33,9 @@ being run: * time: the current UNIX time, e.g. "time<1656759180". * id: the node_id of the peer, e.g. "id=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605". * method: the command being run, e.g. "method=withdraw". +* pnum: the number of parameters. e.g. "pnum<2". * pnameX: the parameter named X. e.g. "pnamedestination=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T". +* parrN: the N'th parameter. e.g. "parr0=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T". RESTRICTION FORMAT ------------------ @@ -56,10 +58,65 @@ a `\\`. * `!`: only passes if the *name* does *not* exist. e.g. `pnamedestination!`. Every other operator except `#` fails if *name* does not exist! -For example, the "readonly" restriction is actually two restrictions: +EXAMPLES +-------- + +This creates a fresh rune which can do anything: + + $ lightning-cli commando-rune + { + "rune": "KUhZzNlECC7pYsz3QVbF1TqjIUYi3oyESTI7n60hLMs9MA==", + "unique_id": "0" + } + +We can add restrictions to that rune, like so: + + $ lightning-cli commando-rune rune=KUhZzNlECC7pYsz3QVbF1TqjIUYi3oyESTI7n60hLMs9MA== restrictions=readonly + { + "rune": "NbL7KkXcPQsVseJ9TdJNjJK2KsPjnt_q4cE_wvc873I9MCZtZXRob2RebGlzdHxtZXRob2ReZ2V0fG1ldGhvZD1zdW1tYXJ5Jm1ldGhvZC9saXN0ZGF0YXN0b3Jl", + "unique_id": "0" + } + +The "readonly" restriction is a short-cut for two restrictions: 1. `method^list|method^get|method=summary`: You may call list, get or summary. -2. `method/listdatastore`: But not listdatastore: that contains sensitive stuff! +2. `method/listdatastore`: But not listdatastore: that contains sensitive stuff! + +We can do the same manually, like so: + + $ lightning-cli commando-rune rune=KUhZzNlECC7pYsz3QVbF1TqjIUYi3oyESTI7n60hLMs9MA== restrictions='["method^list|method^get|method=summary","method/listdatastore"]' + { + "rune": "NbL7KkXcPQsVseJ9TdJNjJK2KsPjnt_q4cE_wvc873I9MCZtZXRob2RebGlzdHxtZXRob2ReZ2V0fG1ldGhvZD1zdW1tYXJ5Jm1ldGhvZC9saXN0ZGF0YXN0b3Jl", + "unique_id": "0" + } + +Let's create a rune which lets a specific peer +(024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605) +run "listpeers" on themselves: + + $ lightning-cli commando-rune restrictions='["id=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605","method=listpeers","pnum=1","pnameid=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605|parr0=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605"]' + { + "rune": "FE8GHiGVvxcFqCQcClVRRiNE_XEeLYQzyG2jmqto4jM9MiZpZD0wMjRiOWExZmE4ZTAwNmYxZTM5MzdmNjVmNjZjNDA4ZTZkYThlMWNhNzI4ZWE0MzIyMmE3MzgxZGYxY2M0NDk2MDUmbWV0aG9kPWxpc3RwZWVycyZwbnVtPTEmcG5hbWVpZD0wMjRiOWExZmE4ZTAwNmYxZTM5MzdmNjVmNjZjNDA4ZTZkYThlMWNhNzI4ZWE0MzIyMmE3MzgxZGYxY2M0NDk2MDV8cGFycjA9MDI0YjlhMWZhOGUwMDZmMWUzOTM3ZjY1ZjY2YzQwOGU2ZGE4ZTFjYTcyOGVhNDMyMjJhNzM4MWRmMWNjNDQ5NjA1", + "unique_id": "2" + } + +This allows `listpeers` with 1 argument (`pnum=1`), which is either by name (`pnameid`), or position (`parr0`). We could shorten this in several ways: either allowing only positional or named parameters, or by testing the start of the parameters only. Here's an example which only checks the first 9 bytes of the `listpeers` parameter: + + $ lightning-cli commando-rune restrictions='["id=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605","method=listpeers","pnum=1","pnameid^024b9a1fa8e006f1e393|parr0^024b9a1fa8e006f1e393"]' + { + "rune": "fTQnfL05coEbiBO8SS0cvQwCcPLxE9c02pZCC6HRVEY9MyZpZD0wMjRiOWExZmE4ZTAwNmYxZTM5MzdmNjVmNjZjNDA4ZTZkYThlMWNhNzI4ZWE0MzIyMmE3MzgxZGYxY2M0NDk2MDUmbWV0aG9kPWxpc3RwZWVycyZwbnVtPTEmcG5hbWVpZF4wMjRiOWExZmE4ZTAwNmYxZTM5M3xwYXJyMF4wMjRiOWExZmE4ZTAwNmYxZTM5Mw==", + "unique_id": "3" + } + +Before we give this to our peer, let's add another restriction: that +it only be usable for 24 hours from now. `date +%s` can give us the +current time in seconds: + + $ lightning-cli commando-rune rune=fTQnfL05coEbiBO8SS0cvQwCcPLxE9c02pZCC6HRVEY9MyZpZD0wMjRiOWExZmE4ZTAwNmYxZTM5MzdmNjVmNjZjNDA4ZTZkYThlMWNhNzI4ZWE0MzIyMmE3MzgxZGYxY2M0NDk2MDUmbWV0aG9kPWxpc3RwZWVycyZwbnVtPTEmcG5hbWVpZF4wMjRiOWExZmE4ZTAwNmYxZTM5M3xwYXJyMF4wMjRiOWExZmE4ZTAwNmYxZTM5Mw== restrictions="t<$(($(date +%s) + 24*60*60))" + { + "rune": "Sh-jGdfO9UGByLvah2AHgc_VwgoNujckPNkxTx54ugg9MyZpZD0wMjRiOWExZmE4ZTAwNmYxZTM5MzdmNjVmNjZjNDA4ZTZkYThlMWNhNzI4ZWE0MzIyMmE3MzgxZGYxY2M0NDk2MDUmbWV0aG9kPWxpc3RwZWVycyZwbnVtPTEmcG5hbWVpZF4wMjRiOWExZmE4ZTAwNmYxZTM5M3xwYXJyMF4wMjRiOWExZmE4ZTAwNmYxZTM5MyZ0PDE2NTY4OTc5MjU=", + "unique_id": "3" + } SHARING RUNES ------------- diff --git a/doc/schemas/commando.request.json b/doc/schemas/commando.request.json index 354f83e86..52c52773f 100644 --- a/doc/schemas/commando.request.json +++ b/doc/schemas/commando.request.json @@ -16,8 +16,16 @@ "description": "method to invoke on peer" }, "params": { - "type": "object", - "description": "parameters for method" + "oneOf": [ + { + "type": "array", + "description": "array of positional parameters" + }, + { + "type": "object", + "description": "parameters for method" + } + ] }, "rune": { "type": "string", diff --git a/plugins/commando.c b/plugins/commando.c index 1fcff8d8e..b7e44f82f 100644 --- a/plugins/commando.c +++ b/plugins/commando.c @@ -201,30 +201,37 @@ static const char *check_condition(const tal_t *ctx, return rune_alt_single_str(ctx, alt, cinfo->buf + cinfo->method->start, cinfo->method->end - cinfo->method->start); + } else if (streq(alt->fieldname, "pnum")) { + return rune_alt_single_int(ctx, alt, cinfo->params->size); } /* Rest are params looksup: generate this once! */ - if (cinfo->params) { - /* Note: we require that params be an obj! */ + if (strmap_empty(&cinfo->cached_params)) { const jsmntok_t *t; size_t i; - json_for_each_obj(i, t, cinfo->params) { - char *pmemname = tal_fmt(tmpctx, - "pname%.*s", - t->end - t->start, - cinfo->buf + t->start); - size_t off = strlen("pname"); - /* Remove punctuation! */ - for (size_t n = off; pmemname[n]; n++) { - if (cispunct(pmemname[n])) - continue; - pmemname[off++] = pmemname[n]; + if (cinfo->params->type == JSMN_OBJECT) { + json_for_each_obj(i, t, cinfo->params) { + char *pmemname = tal_fmt(tmpctx, + "pname%.*s", + t->end - t->start, + cinfo->buf + t->start); + size_t off = strlen("pname"); + /* Remove punctuation! */ + for (size_t n = off; pmemname[n]; n++) { + if (cispunct(pmemname[n])) + continue; + pmemname[off++] = pmemname[n]; + } + pmemname[off++] = '\0'; + strmap_add(&cinfo->cached_params, pmemname, t+1); + } + } else if (cinfo->params->type == JSMN_ARRAY) { + json_for_each_arr(i, t, cinfo->params) { + char *pmemname = tal_fmt(tmpctx, "parr%zu", i); + strmap_add(&cinfo->cached_params, pmemname, t); } - pmemname[off++] = '\0'; - strmap_add(&cinfo->cached_params, pmemname, t+1); } - cinfo->params = NULL; } ptok = strmap_get(&cinfo->cached_params, alt->fieldname); @@ -300,9 +307,9 @@ static void try_command(struct node_id *peer, return; } params = json_get_member(buf, toks, "params"); - if (params && params->type != JSMN_OBJECT) { + if (!params || (params->type != JSMN_OBJECT && params->type != JSMN_ARRAY)) { commando_error(incoming, COMMANDO_ERROR_REMOTE, - "Params must be object"); + "Params must be object or array"); return; } rune = json_get_member(buf, toks, "rune"); @@ -323,15 +330,27 @@ static void try_command(struct node_id *peer, size_t i; const jsmntok_t *t; - json_object_start(req->js, "params"); /* FIXME: This is ugly! */ - json_for_each_obj(i, t, params) { - json_add_jsonstr(req->js, - json_strdup(tmpctx, buf, t), - json_tok_full(buf, t+1), - json_tok_full_len(t+1)); + if (params->type == JSMN_OBJECT) { + json_object_start(req->js, "params"); + json_for_each_obj(i, t, params) { + json_add_jsonstr(req->js, + json_strdup(tmpctx, buf, t), + json_tok_full(buf, t+1), + json_tok_full_len(t+1)); + } + json_object_end(req->js); + } else { + assert(params->type == JSMN_ARRAY); + json_array_start(req->js, "params"); + json_for_each_arr(i, t, params) { + json_add_jsonstr(req->js, + NULL, + json_tok_full(buf, t), + json_tok_full_len(t)); + } + json_array_end(req->js); } - json_object_end(req->js); } else { json_object_start(req->js, "params"); json_object_end(req->js); diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 1d39f13de..b7ddd0350 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -2660,6 +2660,12 @@ def test_commando_rune(node_factory): rune5 = l1.rpc.commando_rune(rune4['rune'], "pnamelevel!|pnamelevel/io") assert rune5['rune'] == 'Dw2tzGCoUojAyT0JUw7fkYJYqExpEpaDRNTkyvWKoJY9MyZpZF4wMjJkMjIzNjIwYTM1OWE0N2ZmNyZtZXRob2Q9bGlzdHBlZXJzJnBuYW1lbGV2ZWwhfHBuYW1lbGV2ZWwvaW8=' assert rune5['unique_id'] == '3' + rune6 = l1.rpc.commando_rune(rune5['rune'], "parr1!|parr1/io") + assert rune6['rune'] == '2Wh6F4R51D3esZzp-7WWG51OhzhfcYKaaI8qiIonaHE9MyZpZF4wMjJkMjIzNjIwYTM1OWE0N2ZmNyZtZXRob2Q9bGlzdHBlZXJzJnBuYW1lbGV2ZWwhfHBuYW1lbGV2ZWwvaW8mcGFycjEhfHBhcnIxL2lv' + assert rune6['unique_id'] == '3' + rune7 = l1.rpc.commando_rune(restrictions="pnum=0") + assert rune7['rune'] == 'QJonN6ySDFw-P5VnilZxlOGRs_tST1ejtd-bAYuZfjk9NCZwbnVtPTA=' + assert rune7['unique_id'] == '4' # Replace rune3 with a more useful timestamp! expiry = int(time.time()) + 15 @@ -2671,12 +2677,19 @@ def test_commando_rune(node_factory): (rune3, "getinfo", {}), (rune4, "listpeers", {}), (rune5, "listpeers", {'id': l2.info['id']}), - (rune5, "listpeers", {'id': l2.info['id'], 'level': 'broken'})) + (rune5, "listpeers", {'id': l2.info['id'], 'level': 'broken'}), + (rune6, "listpeers", [l2.info['id'], 'broken']), + (rune6, "listpeers", [l2.info['id']]), + (rune7, "listpeers", []), + (rune7, "getinfo", {})) failures = ((rune2, "withdraw", {}), (rune2, "plugin", {'subcommand': 'list'}), (rune3, "getinfo", {}), (rune4, "listnodes", {}), - (rune5, "listpeers", {'id': l2.info['id'], 'level': 'io'})) + (rune5, "listpeers", {'id': l2.info['id'], 'level': 'io'}), + (rune6, "listpeers", [l2.info['id'], 'io']), + (rune7, "listpeers", [l2.info['id']]), + (rune7, "listpeers", {'id': l2.info['id']})) for rune, cmd, params in successes: l2.rpc.call(method='commando', @@ -2708,14 +2721,3 @@ def test_commando_rune(node_factory): 'method': "listpeers", 'params': {}}) assert exc_info.value.error['code'] == 0x4c51 - - # Remote doesn't allow array parameters. - l2.rpc.check_request_schemas = False - with pytest.raises(RpcError, match='Params must be object') as exc_info: - l2.rpc.call(method='commando', - payload={'peer_id': l1.info['id'], - 'rune': rune5['rune'], - 'method': "listpeers", - 'params': [l2.info['id'], 'io']}) - assert exc_info.value.error['code'] == 0x4c50 - l2.rpc.check_request_schemas = True