From 12a583d40cf90945b49f83fa5559d65d71b4df4c Mon Sep 17 00:00:00 2001 From: ShahanaFarooqui Date: Thu, 21 Sep 2023 19:05:51 -0700 Subject: [PATCH] runes: Reimplement `rate` in terms of `per` Changelog-Changed: JSON-RPC: `checkrune` `rate` restriction is slightly stricter (exact division of time like `per`) --- doc/lightning-createrune.7.md | 2 +- lightningd/runes.c | 114 +++++++++------------------------- tests/test_runes.py | 66 ++++++++------------ 3 files changed, 55 insertions(+), 127 deletions(-) diff --git a/doc/lightning-createrune.7.md b/doc/lightning-createrune.7.md index 00e711f5ccb5..ff91529c4853 100644 --- a/doc/lightning-createrune.7.md +++ b/doc/lightning-createrune.7.md @@ -35,8 +35,8 @@ 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". -* rate: the rate limit, per minute, e.g. "rate=60". * per: how often the rune can be used, with suffix "sec" (default), "min", "hour", "day" or "msec", "usec" or "nsec". e.g. "per=5sec". +* rate: the rate limit, per minute, e.g. "rate=60" is equivalent to "per=1sec". * pnum: the number of parameters. e.g. "pnum<2". * pnameX: the parameter named X (with any punctuation like `_` removed). e.g. "pnamedestination=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T". * parrN: the N'th parameter. e.g. "parr0=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T". diff --git a/lightningd/runes.c b/lightningd/runes.c index 1a677e694eec..1bd0e97a6b28 100644 --- a/lightningd/runes.c +++ b/lightningd/runes.c @@ -19,27 +19,7 @@ #include #include -struct usage { - /* If you really issue more than 2^32 runes, they'll share ratelimit buckets */ - u32 id; - u32 counter; -}; - -static u64 usage_id(const struct usage *u) -{ - return u->id; -} - -static size_t id_hash(u64 id) -{ - return siphash24(siphash_seed(), &id, sizeof(id)); -} - -static bool usage_eq_id(const struct usage *u, u64 id) -{ - return u->id == id; -} -HTABLE_DEFINE_TYPE(struct usage, usage_id, id_hash, usage_eq_id, usage_table); +static const u64 sec_per_nsec = 1000000000; struct cond_info { const struct runes *runes; @@ -49,7 +29,6 @@ struct cond_info { struct timeabs now; const jsmntok_t *params; STRMAP(const jsmntok_t *) cached_params; - struct usage *usage; }; /* This is lightningd->runes */ @@ -58,7 +37,6 @@ struct runes { struct rune *master; u64 next_unique_id; struct rune_blacklist *blacklist; - struct usage_table *usage_table; }; const char *rune_is_ours(struct lightningd *ld, const struct rune *rune) @@ -66,23 +44,6 @@ const char *rune_is_ours(struct lightningd *ld, const struct rune *rune) return rune_is_derived(ld->runes->master, rune); } -static void memleak_help_usage_table(struct htable *memtable, - struct usage_table *usage_table) -{ - memleak_scan_htable(memtable, &usage_table->raw); -} - -/* Every minute we forget entries. */ -static void flush_usage_table(struct runes *runes) -{ - tal_free(runes->usage_table); - runes->usage_table = tal(runes, struct usage_table); - usage_table_init(runes->usage_table); - memleak_add_helper(runes->usage_table, memleak_help_usage_table); - - notleak(new_reltimer(runes->ld->timers, runes, time_from_sec(60), flush_usage_table, runes)); -} - /* Convert unique_id string to u64. We only expect this to fail when we're * dealing with runes from elsewhere (i.e. param_rune) */ static bool unique_id_num(const struct rune *rune, u64 *num) @@ -114,16 +75,36 @@ u64 rune_unique_id(const struct rune *rune) return num; } -static const char *last_time_check(const tal_t *ctx, +static const char *last_time_check(const struct rune *rune, + struct cond_info *cinfo, + u64 n_sec) +{ + u64 diff; + struct timeabs last_used; + + if (!wallet_get_rune(tmpctx, cinfo->runes->ld->wallet, atol(rune->unique_id), &last_used)) { + /* FIXME: If we do not know the rune, per does not work */ + return NULL; + } + if (time_before(cinfo->now, last_used)) { + last_used = cinfo->now; + } + + diff = time_to_nsec(time_between(cinfo->now, last_used)); + if (diff < n_sec) { + return "too soon"; + } + return NULL; +} + +static const char *per_time_check(const tal_t *ctx, const struct runes *runes, const struct rune *rune, const struct rune_altern *alt, struct cond_info *cinfo) { - u64 r, multiplier, diff; - struct timeabs last_used; + u64 r, multiplier; char *endp; - const u64 sec_per_nsec = 1000000000; if (alt->condition != '=') return "per operator must be ="; @@ -151,19 +132,7 @@ static const char *last_time_check(const tal_t *ctx, if (mul_overflows_u64(r, multiplier)) { return "per overflow"; } - if (!wallet_get_rune(tmpctx, cinfo->runes->ld->wallet, atol(rune->unique_id), &last_used)) { - /* FIXME: If we do not know the rune, per does not work */ - return NULL; - } - if (time_before(cinfo->now, last_used)) { - last_used = cinfo->now; - } - diff = time_to_nsec(time_between(cinfo->now, last_used)); - - if (diff < (r * multiplier)) { - return "too soon"; - } - return NULL; + return last_time_check(rune, cinfo, r * multiplier); } static const char *rate_limit_check(const tal_t *ctx, @@ -174,8 +143,6 @@ static const char *rate_limit_check(const tal_t *ctx, { unsigned long r; char *endp; - u64 unique_id = rune_unique_id(rune); - if (alt->condition != '=') return "rate operator must be ="; @@ -183,22 +150,7 @@ static const char *rate_limit_check(const tal_t *ctx, if (endp == alt->value || *endp || r == 0 || r >= UINT32_MAX) return "malformed rate"; - /* We cache this: we only add usage counter if whole rune succeeds! */ - if (!cinfo->usage) { - cinfo->usage = usage_table_get(runes->usage_table, unique_id); - if (!cinfo->usage) { - cinfo->usage = tal(runes->usage_table, struct usage); - cinfo->usage->id = unique_id; - cinfo->usage->counter = 0; - usage_table_add(runes->usage_table, cinfo->usage); - } - } - - /* >= becuase if we allow this, counter will increment */ - if (cinfo->usage->counter >= r) - return tal_fmt(ctx, "Rate of %lu per minute exceeded", r); - - return NULL; + return last_time_check(rune, cinfo, 60 * sec_per_nsec / r); } /* We need to initialize master runes secret early, so db can use rune_is_ours */ @@ -227,10 +179,6 @@ void runes_finish_init(struct runes *runes) runes->next_unique_id = db_get_intvar(ld->wallet->db, "runes_uniqueid", 0); runes->blacklist = wallet_get_runes_blacklist(runes, ld->wallet); - - /* Initialize usage table and start flush timer. */ - runes->usage_table = NULL; - flush_usage_table(runes); } struct rune_and_string { @@ -795,7 +743,7 @@ static const char *check_condition(const tal_t *ctx, } else if (streq(alt->fieldname, "rate")) { return rate_limit_check(ctx, cinfo->runes, rune, alt, cinfo); } else if (streq(alt->fieldname, "per")) { - return last_time_check(ctx, cinfo->runes, rune, alt, cinfo); + return per_time_check(ctx, cinfo->runes, rune, alt, cinfo); } /* Rest are params looksup: generate this once! */ @@ -850,6 +798,7 @@ static const char *check_condition(const tal_t *ctx, static void update_rune_usage_time(struct runes *runes, struct rune *rune, struct timeabs now) { + /* FIXME: we could batch DB access if this is too slow */ wallet_rune_update_last_used(runes->ld->wallet, rune, now); } @@ -882,8 +831,6 @@ static struct command_result *json_checkrune(struct command *cmd, cinfo.method = method; cinfo.params = methodparams; cinfo.now = time_now(); - /* We will populate it in rate_limit_check if required. */ - cinfo.usage = NULL; strmap_init(&cinfo.cached_params); err = rune_is_ours(cmd->ld, ras->rune); @@ -900,9 +847,6 @@ static struct command_result *json_checkrune(struct command *cmd, return command_fail(cmd, RUNE_NOT_PERMITTED, "Not permitted: %s", err); } - /* If it succeeded, *now* we increment any associated usage counter. */ - if (cinfo.usage) - cinfo.usage->counter++; update_rune_usage_time(cmd->ld->runes, ras->rune, cinfo.now); js = json_stream_success(cmd); diff --git a/tests/test_runes.py b/tests/test_runes.py index a769ad9b6e53..67d3b74f4e8d 100644 --- a/tests/test_runes.py +++ b/tests/test_runes.py @@ -124,10 +124,7 @@ def test_createrune(node_factory): (rune2, "getinfo", {}), (rune3, "getinfo", {}), (rune7, "listpeers", []), - (rune7, "getinfo", {}), - (rune9, "getinfo", {}), - (rune8, "getinfo", {}), - (rune8, "getinfo", {})) + (rune7, "getinfo", {})) failures = ((rune2, "withdraw", {}), (rune2, "plugin", {'subcommand': 'list'}), @@ -136,7 +133,10 @@ def test_createrune(node_factory): (rune5, "listpeers", {'id': l1.info['id'], 'level': 'io'}), (rune6, "listpeers", [l1.info['id'], 'io']), (rune7, "listpeers", [l1.info['id']]), - (rune7, "listpeers", {'id': l1.info['id']})) + (rune7, "listpeers", {'id': l1.info['id']}), + # These are derived from rune7, so they have been recently used + (rune8, "getinfo", {}), + (rune9, "getinfo", {})) for rune, cmd, params in successes: l1.rpc.checkrune(nodeid=l1.info['id'], @@ -156,17 +156,11 @@ def test_createrune(node_factory): params=params) assert exc_info.value.error['code'] == 0x5de - # Now, this can flake if we cross a minute boundary! So wait until - # It succeeds again. - while True: - try: - l1.rpc.checkrune(nodeid=l1.info['id'], - rune=rune8['rune'], - method='getinfo') - break - except RpcError as e: - assert e.error['code'] == 0x5de - time.sleep(1) + # Rune 7 succeeded, so we need to wait for 20 seconds + time.sleep(21) + l1.rpc.checkrune(nodeid=l1.info['id'], + rune=rune8['rune'], + method='getinfo')['valid'] is True # This fails immediately, since we've done one. with pytest.raises(RpcError, match='Not permitted:') as exc_info: @@ -176,22 +170,6 @@ def test_createrune(node_factory): params={}) assert exc_info.value.error['code'] == 0x5de - # Two more succeed for rune8. - for _ in range(2): - l1.rpc.checkrune(nodeid=l1.info['id'], - rune=rune8['rune'], - method='getinfo', - params={}) - assert exc_info.value.error['code'] == 0x5de - - # Now we've had 3 in one minute, this will fail. - with pytest.raises(RpcError, match='Not permitted:') as exc_info: - l1.rpc.checkrune(nodeid=l1.info['id'], - rune=rune8['rune'], - method='getinfo', - params={}) - assert exc_info.value.error['code'] == 0x5de - # rune5 can only be used by l2: with pytest.raises(RpcError, match='Not permitted:') as exc_info: l1.rpc.checkrune(nodeid=l1.info['id'], @@ -200,16 +178,22 @@ def test_createrune(node_factory): params={}) assert exc_info.value.error['code'] == 0x5de - # Now wait for ratelimit expiry, ratelimits should reset. - time.sleep(61) + # Rune8 has rate 3 per minute (20 seconds) and rune9 has rate 1 per minute (60 seconds) + time.sleep(21) + with pytest.raises(RpcError, match='Not permitted: too soon'): + l1.rpc.checkrune(nodeid=l1.info['id'], + rune=rune9['rune'], + method="getinfo") + + assert l1.rpc.checkrune(nodeid=l1.info['id'], + rune=rune8['rune'], + method="getinfo")['valid'] is True - for rune, cmd, params in ((rune9, "getinfo", {}), - (rune8, "getinfo", {}), - (rune8, "getinfo", {})): - assert l1.rpc.checkrune(nodeid=l1.info['id'], - rune=rune['rune'], - method=cmd, - params=params)['valid'] is True + # Rune8 uses the same unique_id as rune9, so we need to wait for full 1 minute + time.sleep(61) + assert l1.rpc.checkrune(nodeid=l1.info['id'], + rune=rune9['rune'], + method="getinfo")['valid'] is True def do_test_rune_per_restriction(l1, rune_to_test, per_sec):