Skip to content

Commit

Permalink
runes: Reimplement rate in terms of per
Browse files Browse the repository at this point in the history 
Changelog-Changed: JSON-RPC: `checkrune` `rate` restriction is slightly stricter (exact division of time like `per`)
  • Loading branch information
@ShahanaFarooqui @rustyrussell
ShahanaFarooqui authored and rustyrussell committed Oct 2, 2023
1 parent d61dec8 commit 9bcabbc
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 127 deletions.
2 changes: 1 addition & 1 deletion doc/lightning-createrune.7.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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".
Expand Down
114 changes: 29 additions & 85 deletions lightningd/runes.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,27 +19,7 @@
#include <lightningd/runes.h>
#include <wallet/wallet.h>

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;
Expand All @@ -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 */
Expand All @@ -58,31 +37,13 @@ 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)
{
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)
Expand Down Expand Up @@ -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 =";
Expand Down Expand Up @@ -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,
Expand All @@ -174,31 +143,14 @@ 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 =";

r = strtoul(alt->value, &endp, 10);
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 */
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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! */
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
66 changes: 25 additions & 41 deletions tests/test_runes.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'}),
Expand All @@ -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'],
Expand All @@ -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:
Expand All @@ -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'],
Expand All @@ -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):
Expand Down

0 comments on commit 9bcabbc

Please sign in to comment.