From cf52b7161f164e96376794862bdb3b14cd4177ec Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 17 Sep 2018 03:45:01 +0200 Subject: [PATCH 01/20] json-rpc: Remove upper limit for percentage The `json_tok_percentage` parser is used for the `fuzzpercent` in `getroute` and `maxfeepercent` in `pay`. In both cases it seems reasonable to allow values larger than 100%. This has bitten users in the past when they transferred single satoshis to things like satoshis.place over a route longer than 2 hops. --- lightningd/gossip_control.c | 2 +- lightningd/json.c | 7 +++---- lightningd/test/run-param.c | 4 ++-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index 83357b59e71a..39e91ad4cc1c 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -327,7 +327,7 @@ static const struct json_command getroute_command = { json_getroute, "Show route to {id} for {msatoshi}, using {riskfactor} and optional {cltv} (default 9). " "If specified search from {fromid} otherwise use this node as source. " - "Randomize the route with up to {fuzzpercent} (0.0 -> 100.0, default 5.0) " + "Randomize the route with up to {fuzzpercent} (default 5.0) " "using {seed} as an arbitrary-size string seed." }; AUTODATA(json_command, &getroute_command); diff --git a/lightningd/json.c b/lightningd/json.c index d99ab9ec10a6..b0ac53246925 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -238,12 +238,11 @@ bool json_tok_percent(struct command *cmd, const char *name, double **num) { *num = tal(cmd, double); - if (json_to_double(buffer, tok, *num)) - if (**num >= 0.0 && **num <= 100.0) - return true; + if (json_to_double(buffer, tok, *num) && **num >= 0.0) + return true; command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%s' should be a double in range [0.0, 100.0], not '%.*s'", + "'%s' should be a positive double, not '%.*s'", name, tok->end - tok->start, buffer + tok->start); return false; } diff --git a/lightningd/test/run-param.c b/lightningd/test/run-param.c index 6b6576ec52f0..033ec56dd870 100644 --- a/lightningd/test/run-param.c +++ b/lightningd/test/run-param.c @@ -502,8 +502,8 @@ static void json_tok_tests(void) test_cb(json_tok_percent, double, "[ 1.01 ]", 1.01, true); test_cb(json_tok_percent, double, "[ 99.99 ]", 99.99, true); test_cb(json_tok_percent, double, "[ 100.0 ]", 100, true); - test_cb(json_tok_percent, double, "[ 100.001 ]", 0, false); - test_cb(json_tok_percent, double, "[ 1000 ]", 0, false); + test_cb(json_tok_percent, double, "[ 100.001 ]", 100.001, true); + test_cb(json_tok_percent, double, "[ 1000 ]", 1000, true); test_cb(json_tok_percent, double, "[ 'wow' ]", 0, false); } From 674d17608718e714a5591f4c88b4f8fc5a5566f5 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 17 Sep 2018 03:51:37 +0200 Subject: [PATCH 02/20] doc: Update docs to remove 100% upper bound Actual change is in the previous commit. --- doc/lightning-getroute.7 | 6 +++--- doc/lightning-getroute.7.txt | 3 +-- doc/lightning-listfunds.7 | 4 ++-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/doc/lightning-getroute.7 b/doc/lightning-getroute.7 index 28565c536be3..fa218b6133d4 100644 --- a/doc/lightning-getroute.7 +++ b/doc/lightning-getroute.7 @@ -2,12 +2,12 @@ .\" Title: lightning-getroute .\" Author: [see the "AUTHOR" section] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" Date: 04/26/2018 +.\" Date: 09/17/2018 .\" Manual: \ \& .\" Source: \ \& .\" Language: English .\" -.TH "LIGHTNING\-GETROUTE" "7" "04/26/2018" "\ \&" "\ \&" +.TH "LIGHTNING\-GETROUTE" "7" "09/17/2018" "\ \&" "\ \&" .\" ----------------------------------------------------------------- .\" * Define some portability stuff .\" ----------------------------------------------------------------- @@ -42,7 +42,7 @@ For example, if you thought there was a 1% chance that a node would fail, and it .sp If you didn\(cqt care about risk, \fIriskfactor\fR would be zero\&. .sp -The \fIfuzzpercent\fR is a floating\-point number, between 0\&.0 to 100\&.0, unit of percent\&. The \fIfuzzpercent\fR is used to distort computed fees along each channel, to provide some randomization to the route generated\&. 0\&.0 means the exact fee of that channel is used, while 100\&.0 means the fee used might be from 0 to twice the actual fee\&. The default is 5\&.0, or up to 5% fee distortion\&. +The \fIfuzzpercent\fR is a positive floating\-point number, representing a percentage of the actual fee\&. The \fIfuzzpercent\fR is used to distort computed fees along each channel, to provide some randomization to the route generated\&. 0\&.0 means the exact fee of that channel is used, while 100\&.0 means the fee used might be from 0 to twice the actual fee\&. The default is 5\&.0, or up to 5% fee distortion\&. .sp The \fIseed\fR is a string whose bytes are used to seed the RNG for the route randomization\&. If not specified, a random string is used\&. .SH "RISKFACTOR EFFECT ON ROUTING" diff --git a/doc/lightning-getroute.7.txt b/doc/lightning-getroute.7.txt index 3bd7f5f5bc89..8a62a9d1bd3c 100644 --- a/doc/lightning-getroute.7.txt +++ b/doc/lightning-getroute.7.txt @@ -29,8 +29,7 @@ fail, and it would cost you 20% per annum if that happened, If you didn't care about risk, 'riskfactor' would be zero. -The 'fuzzpercent' is a floating-point number, between 0.0 to 100.0, -unit of percent. +The 'fuzzpercent' is a positive floating-point number, representing a percentage of the actual fee. The 'fuzzpercent' is used to distort computed fees along each channel, to provide some randomization to the route generated. 0.0 means the exact fee of that channel is used, diff --git a/doc/lightning-listfunds.7 b/doc/lightning-listfunds.7 index a3f014a4bbeb..31a31dcfc951 100644 --- a/doc/lightning-listfunds.7 +++ b/doc/lightning-listfunds.7 @@ -2,12 +2,12 @@ .\" Title: lightning-listfunds .\" Author: [see the "AUTHOR" section] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" Date: 09/07/2018 +.\" Date: 09/18/2018 .\" Manual: \ \& .\" Source: \ \& .\" Language: English .\" -.TH "LIGHTNING\-LISTFUNDS" "7" "09/07/2018" "\ \&" "\ \&" +.TH "LIGHTNING\-LISTFUNDS" "7" "09/18/2018" "\ \&" "\ \&" .\" ----------------------------------------------------------------- .\" * Define some portability stuff .\" ----------------------------------------------------------------- From 94e42f2384d8e24ce8e181d05da97d795a03c3d6 Mon Sep 17 00:00:00 2001 From: Simon Vrouwe Date: Mon, 17 Sep 2018 08:55:30 +0300 Subject: [PATCH 03/20] openingd: prioritize incoming peer traffic over handling (and sending out) gossip - reduces probability for a deadlock where we block on sending data because the other peer cannot receive because it blocks on sending data etc. - when either side sends so much data that it fills up the kernel/network buffer - however sending out gossip can still block when (malicious) peer never receives --- openingd/openingd.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openingd/openingd.c b/openingd/openingd.c index b17e0eea3b89..dbcca169232a 100644 --- a/openingd/openingd.c +++ b/openingd/openingd.c @@ -1157,10 +1157,11 @@ int main(int argc, char *argv[]) * don't try to service more than one fd per loop. */ if (pollfd[0].revents & POLLIN) msg = handle_master_in(state); - else if (pollfd[1].revents & POLLIN) - handle_gossip_in(state); else if (pollfd[2].revents & POLLIN) msg = handle_peer_in(state); + else if (pollfd[1].revents & POLLIN) + handle_gossip_in(state); + clean_tmpctx(); } From 4198cb34a28523f7a5559c5b839270a72b1f5413 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 13:40:37 +0930 Subject: [PATCH 04/20] CHANGELOG.md: catchup with changes so far. Signed-off-by: Rusty Russell --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index db1a501cbebc..42634719073e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added - JSON API: `listpeers` has new field `scratch_txid`: the latest tx in channel. +- Bitcoind: more parallelism in requests, for very slow nodes. +- Testing: fixed logging, cleaner interception of bitcoind, minor fixes. ### Changed @@ -21,6 +23,12 @@ changes. ### Fixed +- Startup: more coherent complaint if daemon already running. +- JSON RPC: `getinfo` now shows correct Tor port. +- JSON RPC: `ping` now works even after one peer fails to respond. +- JSON RPC: `getroute` `fuzzpercent` and `pay` `maxfeepercent` can now be > 100. +- Protocol: fix occasional deadlock when both peers flood with gossip. + ### Security ## [0.6.1] - 2018-09-11: "Principled Opposition To Segwit" From d5c3626fa73967ff59d60b36c8ec21394f357f9d Mon Sep 17 00:00:00 2001 From: Saibato Date: Wed, 19 Sep 2018 17:09:56 +0000 Subject: [PATCH 05/20] parse autotor: address before separate_address_and_port this enables addr like --addr=autotor:127.0.0.1 or --addr=autotor:localhost to just use the default tor service port Signed-off-by: Saibato --- common/wireaddr.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/common/wireaddr.c b/common/wireaddr.c index 8f2f2819bc00..1ff8ca13235c 100644 --- a/common/wireaddr.c +++ b/common/wireaddr.c @@ -430,6 +430,16 @@ bool parse_wireaddr_internal(const char *arg, struct wireaddr_internal *addr, return true; } + /* 'autotor:' is a special prefix meaning talk to Tor to create + * an onion address. */ + if (strstarts(arg, "autotor:")) { + addr->itype = ADDR_INTERNAL_AUTOTOR; + return parse_wireaddr(arg + strlen("autotor:"), + &addr->u.torservice, 9051, + dns_ok ? NULL : &needed_dns, + err_msg); + } + splitport = port; if (!separate_address_and_port(tmpctx, arg, &ip, &splitport)) { if (err_msg) { @@ -446,16 +456,6 @@ bool parse_wireaddr_internal(const char *arg, struct wireaddr_internal *addr, return true; } - /* 'autotor:' is a special prefix meaning talk to Tor to create - * an onion address. */ - if (strstarts(arg, "autotor:")) { - addr->itype = ADDR_INTERNAL_AUTOTOR; - return parse_wireaddr(arg + strlen("autotor:"), - &addr->u.torservice, 9051, - dns_ok ? NULL : &needed_dns, - err_msg); - } - addr->itype = ADDR_INTERNAL_WIREADDR; if (parse_wireaddr(arg, &addr->u.wireaddr, port, dns_ok ? NULL : &needed_dns, err_msg)) From e41e1a177ebf1dde139e096f5450e5b633928ee1 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 13:52:15 +0930 Subject: [PATCH 06/20] pytest: wait until mock is called for set_feerates. Got a spurious failure in test_no_fee_estimate; we fired too soon from the logs (presumably we raced in on the first response, but estimatesmartfee gets called 3 times). Signed-off-by: Rusty Russell --- tests/btcproxy.py | 4 ++++ tests/utils.py | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/btcproxy.py b/tests/btcproxy.py index 9f7d3d348d58..8b0f64018692 100644 --- a/tests/btcproxy.py +++ b/tests/btcproxy.py @@ -29,6 +29,7 @@ def __init__(self, bitcoind, rpcport=0): self.app.add_url_rule("/", "API entrypoint", self.proxy, methods=['POST']) self.rpcport = rpcport self.mocks = {} + self.mock_counts = {} self.bitcoind = bitcoind self.request_count = 0 @@ -40,8 +41,10 @@ def _handle_request(self, r): # If we have set a mock for this method reply with that instead of # forwarding the request. if method in self.mocks and type(method) == dict: + self.mock_counts[method] += 1 return self.mocks[method] elif method in self.mocks and callable(self.mocks[method]): + self.mock_counts[method] += 1 return self.mocks[method](r) try: @@ -100,5 +103,6 @@ def mock_rpc(self, method, response=None): """ if response is not None: self.mocks[method] = response + self.mock_counts[method] = 0 elif method in self.mocks: del self.mocks[method] diff --git a/tests/utils.py b/tests/utils.py index 565d306e6c72..7ec68a812f17 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -627,8 +627,10 @@ def mock_estimatesmartfee(r): } self.daemon.rpcproxy.mock_rpc('estimatesmartfee', mock_estimatesmartfee) + # Technically, this waits until it's called, not until it's processed. + # We wait until all three levels have been called. if wait_for_effect: - self.daemon.wait_for_log('Feerate estimate for .* set to') + wait_for(lambda: self.daemon.rpcproxy.mock_counts['estimatesmartfee'] >= 3) class NodeFactory(object): From 72e7856bf38fb77aab98de0a41cca8a9b2d83ee9 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 12:36:42 +0930 Subject: [PATCH 07/20] hsmd: reorder functions (MOVEONLY). Signed-off-by: Rusty Russell --- hsmd/hsmd.c | 822 ++++++++++++++++++++++++++-------------------------- 1 file changed, 407 insertions(+), 415 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 085f77a2beb8..59c29a2c2205 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -69,15 +69,16 @@ static UINTMAP(struct client *) clients; static struct client *dbid_zero_clients[3]; static size_t num_dbid_zero_clients; -/* Function declarations for later */ -static struct io_plan *handle_client(struct io_conn *conn, - struct daemon_conn *dc); -static void init_hsm(struct daemon_conn *master, const u8 *msg); -static void pass_client_hsmfd(struct daemon_conn *master, const u8 *msg); -static void sign_funding_tx(struct daemon_conn *master, const u8 *msg); -static void sign_invoice(struct daemon_conn *master, const u8 *msg); -static void sign_node_announcement(struct daemon_conn *master, const u8 *msg); -static void sign_withdrawal_tx(struct daemon_conn *master, const u8 *msg); +/* FIXME: This is used by debug.c, but doesn't apply to us. */ +extern void dev_disconnect_init(int fd); +void dev_disconnect_init(int fd UNUSED) { } + +/* Pre-declare this, due to mutual recursion */ +static struct client *new_client(struct daemon_conn *master, + const struct pubkey *id, + u64 dbid, + const u64 capabilities, + int fd); static void node_key(struct privkey *node_privkey, struct pubkey *node_id) { @@ -101,56 +102,6 @@ static void node_key(struct privkey *node_privkey, struct pubkey *node_id) node_privkey->secret.data)); } -static void destroy_client(struct client *c) -{ - if (!uintmap_del(&clients, c->dbid)) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Failed to remove client dbid %"PRIu64, c->dbid); -} - -static struct client *new_client(struct daemon_conn *master, - const struct pubkey *id, - u64 dbid, - const u64 capabilities, - int fd) -{ - struct client *c = tal(master, struct client); - - if (id) { - c->id = *id; - } else { - memset(&c->id, 0, sizeof(c->id)); - } - c->dbid = dbid; - - c->master = master; - c->capabilities = capabilities; - daemon_conn_init(c, &c->dc, fd, handle_client, NULL); - - /* Free the connection if we exit everything. */ - tal_steal(master, c->dc.conn); - /* Free client when connection freed. */ - tal_steal(c->dc.conn, c); - - if (dbid == 0) { - assert(num_dbid_zero_clients < ARRAY_SIZE(dbid_zero_clients)); - dbid_zero_clients[num_dbid_zero_clients++] = c; - } else { - struct client *old_client = uintmap_get(&clients, dbid); - - /* Close conn and free any old client of this dbid. */ - if (old_client) - io_close(old_client->dc.conn); - - if (!uintmap_add(&clients, dbid, c)) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Failed inserting dbid %"PRIu64, dbid); - tal_add_destructor(c, destroy_client); - } - - return c; -} - /** * hsm_peer_secret_base -- Derive the base secret seed for per-peer seeds * @@ -182,6 +133,173 @@ static void get_channel_seed(const struct pubkey *peer_id, u64 dbid, info, strlen(info)); } +static void send_init_response(struct daemon_conn *master) +{ + struct pubkey node_id; + u8 *msg; + + node_key(NULL, &node_id); + + msg = towire_hsm_init_reply(NULL, &node_id, &secretstuff.bip32); + daemon_conn_send(master, take(msg)); +} + +static void populate_secretstuff(void) +{ + u8 bip32_seed[BIP32_ENTROPY_LEN_256]; + u32 salt = 0; + struct ext_key master_extkey, child_extkey; + + /* Fill in the BIP32 tree for bitcoin addresses. */ + do { + hkdf_sha256(bip32_seed, sizeof(bip32_seed), + &salt, sizeof(salt), + &secretstuff.hsm_secret, + sizeof(secretstuff.hsm_secret), + "bip32 seed", strlen("bip32 seed")); + salt++; + } while (bip32_key_from_seed(bip32_seed, sizeof(bip32_seed), + BIP32_VER_TEST_PRIVATE, + 0, &master_extkey) != WALLY_OK); + + /* BIP 32: + * + * The default wallet layout + * + * An HDW is organized as several 'accounts'. Accounts are numbered, + * the default account ("") being number 0. Clients are not required + * to support more than one account - if not, they only use the + * default account. + * + * Each account is composed of two keypair chains: an internal and an + * external one. The external keychain is used to generate new public + * addresses, while the internal keychain is used for all other + * operations (change addresses, generation addresses, ..., anything + * that doesn't need to be communicated). Clients that do not support + * separate keychains for these should use the external one for + * everything. + * + * - m/iH/0/k corresponds to the k'th keypair of the external chain of account number i of the HDW derived from master m. + */ + /* Hence child 0, then child 0 again to get extkey to derive from. */ + if (bip32_key_from_parent(&master_extkey, 0, BIP32_FLAG_KEY_PRIVATE, + &child_extkey) != WALLY_OK) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "Can't derive child bip32 key"); + + if (bip32_key_from_parent(&child_extkey, 0, BIP32_FLAG_KEY_PRIVATE, + &secretstuff.bip32) != WALLY_OK) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "Can't derive private bip32 key"); +} + +static void bitcoin_pubkey(struct pubkey *pubkey, u32 index) +{ + struct ext_key ext; + + if (index >= BIP32_INITIAL_HARDENED_CHILD) + status_failed(STATUS_FAIL_MASTER_IO, + "Index %u too great", index); + + if (bip32_key_from_parent(&secretstuff.bip32, index, + BIP32_FLAG_KEY_PUBLIC, &ext) != WALLY_OK) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "BIP32 of %u failed", index); + + if (!secp256k1_ec_pubkey_parse(secp256k1_ctx, &pubkey->pubkey, + ext.pub_key, sizeof(ext.pub_key))) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "Parse of BIP32 child %u pubkey failed", index); +} + +static void bitcoin_keypair(struct privkey *privkey, + struct pubkey *pubkey, + u32 index) +{ + struct ext_key ext; + + if (index >= BIP32_INITIAL_HARDENED_CHILD) + status_failed(STATUS_FAIL_MASTER_IO, + "Index %u too great", index); + + if (bip32_key_from_parent(&secretstuff.bip32, index, + BIP32_FLAG_KEY_PRIVATE, &ext) != WALLY_OK) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "BIP32 of %u failed", index); + + /* libwally says: The private key with prefix byte 0 */ + memcpy(privkey->secret.data, ext.priv_key+1, 32); + if (!secp256k1_ec_pubkey_create(secp256k1_ctx, &pubkey->pubkey, + privkey->secret.data)) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "BIP32 pubkey %u create failed", index); +} + +static void maybe_create_new_hsm(void) +{ + int fd = open("hsm_secret", O_CREAT|O_EXCL|O_WRONLY, 0400); + if (fd < 0) { + if (errno == EEXIST) + return; + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "creating: %s", strerror(errno)); + } + + randombytes_buf(&secretstuff.hsm_secret, sizeof(secretstuff.hsm_secret)); + if (!write_all(fd, &secretstuff.hsm_secret, sizeof(secretstuff.hsm_secret))) { + unlink_noerr("hsm_secret"); + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "writing: %s", strerror(errno)); + } + if (fsync(fd) != 0) { + unlink_noerr("hsm_secret"); + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "fsync: %s", strerror(errno)); + } + if (close(fd) != 0) { + unlink_noerr("hsm_secret"); + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "closing: %s", strerror(errno)); + } + fd = open(".", O_RDONLY); + if (fd < 0) { + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "opening: %s", strerror(errno)); + } + if (fsync(fd) != 0) { + unlink_noerr("hsm_secret"); + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "fsyncdir: %s", strerror(errno)); + } + close(fd); + status_unusual("HSM: created new hsm_secret file"); +} + +static void load_hsm(void) +{ + int fd = open("hsm_secret", O_RDONLY); + if (fd < 0) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "opening: %s", strerror(errno)); + if (!read_all(fd, &secretstuff.hsm_secret, sizeof(secretstuff.hsm_secret))) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "reading: %s", strerror(errno)); + close(fd); + + populate_secretstuff(); +} + +static void init_hsm(struct daemon_conn *master, const u8 *msg) +{ + if (!fromwire_hsm_init(msg)) + master_badmsg(WIRE_HSM_INIT, msg); + + maybe_create_new_hsm(); + load_hsm(); + + send_init_response(master); +} + static struct io_plan *handle_ecdh(struct io_conn *conn, struct daemon_conn *dc) { struct client *c = container_of(dc, struct client, dc); @@ -833,361 +951,23 @@ static struct io_plan *handle_sign_mutual_close_tx(struct io_conn *conn, return io_close(conn); } -static bool check_client_capabilities(struct client *client, - enum hsm_client_wire_type t) +static void pass_client_hsmfd(struct daemon_conn *master, const u8 *msg) { - switch (t) { - case WIRE_HSM_ECDH_REQ: - return (client->capabilities & HSM_CAP_ECDH) != 0; + int fds[2]; + u64 dbid, capabilities; + struct pubkey id; - case WIRE_HSM_CANNOUNCEMENT_SIG_REQ: - case WIRE_HSM_CUPDATE_SIG_REQ: - case WIRE_HSM_NODE_ANNOUNCEMENT_SIG_REQ: - return (client->capabilities & HSM_CAP_SIGN_GOSSIP) != 0; + if (!fromwire_hsm_client_hsmfd(msg, &id, &dbid, &capabilities)) + master_badmsg(WIRE_HSM_CLIENT_HSMFD, msg); - case WIRE_HSM_SIGN_DELAYED_PAYMENT_TO_US: - case WIRE_HSM_SIGN_REMOTE_HTLC_TO_US: - case WIRE_HSM_SIGN_PENALTY_TO_US: - case WIRE_HSM_SIGN_LOCAL_HTLC_TX: - return (client->capabilities & HSM_CAP_SIGN_ONCHAIN_TX) != 0; + if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) != 0) + status_failed(STATUS_FAIL_INTERNAL_ERROR, "creating fds: %s", strerror(errno)); - case WIRE_HSM_GET_PER_COMMITMENT_POINT: - case WIRE_HSM_CHECK_FUTURE_SECRET: - return (client->capabilities & HSM_CAP_COMMITMENT_POINT) != 0; - - case WIRE_HSM_SIGN_REMOTE_COMMITMENT_TX: - case WIRE_HSM_SIGN_REMOTE_HTLC_TX: - return (client->capabilities & HSM_CAP_SIGN_REMOTE_TX) != 0; - - case WIRE_HSM_SIGN_MUTUAL_CLOSE_TX: - return (client->capabilities & HSM_CAP_SIGN_CLOSING_TX) != 0; - - case WIRE_HSM_INIT: - case WIRE_HSM_CLIENT_HSMFD: - case WIRE_HSM_SIGN_FUNDING: - case WIRE_HSM_SIGN_WITHDRAWAL: - case WIRE_HSM_SIGN_INVOICE: - case WIRE_HSM_SIGN_COMMITMENT_TX: - case WIRE_HSM_GET_CHANNEL_BASEPOINTS: - return (client->capabilities & HSM_CAP_MASTER) != 0; - - /* These are messages sent by the HSM so we should never receive them */ - case WIRE_HSM_ECDH_RESP: - case WIRE_HSM_CANNOUNCEMENT_SIG_REPLY: - case WIRE_HSM_CUPDATE_SIG_REPLY: - case WIRE_HSM_CLIENT_HSMFD_REPLY: - case WIRE_HSM_SIGN_FUNDING_REPLY: - case WIRE_HSM_NODE_ANNOUNCEMENT_SIG_REPLY: - case WIRE_HSM_SIGN_WITHDRAWAL_REPLY: - case WIRE_HSM_SIGN_INVOICE_REPLY: - case WIRE_HSM_INIT_REPLY: - case WIRE_HSMSTATUS_CLIENT_BAD_REQUEST: - case WIRE_HSM_SIGN_COMMITMENT_TX_REPLY: - case WIRE_HSM_SIGN_TX_REPLY: - case WIRE_HSM_GET_PER_COMMITMENT_POINT_REPLY: - case WIRE_HSM_CHECK_FUTURE_SECRET_REPLY: - case WIRE_HSM_GET_CHANNEL_BASEPOINTS_REPLY: - break; - } - return false; -} - -static struct io_plan *handle_client(struct io_conn *conn, - struct daemon_conn *dc) -{ - struct client *c = container_of(dc, struct client, dc); - enum hsm_client_wire_type t = fromwire_peektype(dc->msg_in); - - status_debug("Client: Received message %d from client", t); - - /* Before we do anything else, is this client allowed to do - * what he asks for? */ - if (!check_client_capabilities(c, t)) { - status_broken("Client does not have the required capability to run %d", t); - daemon_conn_send(c->master, - take(towire_hsmstatus_client_bad_request( - NULL, &c->id, dc->msg_in))); - return io_close(conn); - } - - /* Now actually go and do what the client asked for */ - switch (t) { - case WIRE_HSM_INIT: - init_hsm(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); - - case WIRE_HSM_CLIENT_HSMFD: - pass_client_hsmfd(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); - - case WIRE_HSM_GET_CHANNEL_BASEPOINTS: - return handle_get_channel_basepoints(conn, dc); - - case WIRE_HSM_ECDH_REQ: - return handle_ecdh(conn, dc); - - case WIRE_HSM_CANNOUNCEMENT_SIG_REQ: - return handle_cannouncement_sig(conn, c); - - case WIRE_HSM_CUPDATE_SIG_REQ: - return handle_channel_update_sig(conn, dc); - - case WIRE_HSM_SIGN_FUNDING: - sign_funding_tx(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); - - case WIRE_HSM_NODE_ANNOUNCEMENT_SIG_REQ: - sign_node_announcement(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); - - case WIRE_HSM_SIGN_INVOICE: - sign_invoice(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); - - case WIRE_HSM_SIGN_WITHDRAWAL: - sign_withdrawal_tx(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); - - case WIRE_HSM_SIGN_COMMITMENT_TX: - return handle_sign_commitment_tx(conn, dc); - - case WIRE_HSM_SIGN_DELAYED_PAYMENT_TO_US: - return handle_sign_delayed_payment_to_us(conn, c); - - case WIRE_HSM_SIGN_REMOTE_HTLC_TO_US: - return handle_sign_remote_htlc_to_us(conn, c); - - case WIRE_HSM_SIGN_PENALTY_TO_US: - return handle_sign_penalty_to_us(conn, c); - - case WIRE_HSM_SIGN_LOCAL_HTLC_TX: - return handle_sign_local_htlc_tx(conn, c); - - case WIRE_HSM_GET_PER_COMMITMENT_POINT: - return handle_get_per_commitment_point(conn, c); - - case WIRE_HSM_CHECK_FUTURE_SECRET: - return handle_check_future_secret(conn, c); - - case WIRE_HSM_SIGN_REMOTE_COMMITMENT_TX: - return handle_sign_remote_commitment_tx(conn, c); - - case WIRE_HSM_SIGN_REMOTE_HTLC_TX: - return handle_sign_remote_htlc_tx(conn, c); - - case WIRE_HSM_SIGN_MUTUAL_CLOSE_TX: - return handle_sign_mutual_close_tx(conn, c); - - case WIRE_HSM_ECDH_RESP: - case WIRE_HSM_CANNOUNCEMENT_SIG_REPLY: - case WIRE_HSM_CUPDATE_SIG_REPLY: - case WIRE_HSM_CLIENT_HSMFD_REPLY: - case WIRE_HSM_SIGN_FUNDING_REPLY: - case WIRE_HSM_NODE_ANNOUNCEMENT_SIG_REPLY: - case WIRE_HSM_SIGN_WITHDRAWAL_REPLY: - case WIRE_HSM_SIGN_INVOICE_REPLY: - case WIRE_HSM_INIT_REPLY: - case WIRE_HSMSTATUS_CLIENT_BAD_REQUEST: - case WIRE_HSM_SIGN_COMMITMENT_TX_REPLY: - case WIRE_HSM_SIGN_TX_REPLY: - case WIRE_HSM_GET_PER_COMMITMENT_POINT_REPLY: - case WIRE_HSM_CHECK_FUTURE_SECRET_REPLY: - case WIRE_HSM_GET_CHANNEL_BASEPOINTS_REPLY: - break; - } - - daemon_conn_send(c->master, - take(towire_hsmstatus_client_bad_request(NULL, - &c->id, - dc->msg_in))); - return io_close(conn); -} - -static void send_init_response(struct daemon_conn *master) -{ - struct pubkey node_id; - u8 *msg; - - node_key(NULL, &node_id); - - msg = towire_hsm_init_reply(NULL, &node_id, &secretstuff.bip32); - daemon_conn_send(master, take(msg)); -} - -static void populate_secretstuff(void) -{ - u8 bip32_seed[BIP32_ENTROPY_LEN_256]; - u32 salt = 0; - struct ext_key master_extkey, child_extkey; - - /* Fill in the BIP32 tree for bitcoin addresses. */ - do { - hkdf_sha256(bip32_seed, sizeof(bip32_seed), - &salt, sizeof(salt), - &secretstuff.hsm_secret, - sizeof(secretstuff.hsm_secret), - "bip32 seed", strlen("bip32 seed")); - salt++; - } while (bip32_key_from_seed(bip32_seed, sizeof(bip32_seed), - BIP32_VER_TEST_PRIVATE, - 0, &master_extkey) != WALLY_OK); - - /* BIP 32: - * - * The default wallet layout - * - * An HDW is organized as several 'accounts'. Accounts are numbered, - * the default account ("") being number 0. Clients are not required - * to support more than one account - if not, they only use the - * default account. - * - * Each account is composed of two keypair chains: an internal and an - * external one. The external keychain is used to generate new public - * addresses, while the internal keychain is used for all other - * operations (change addresses, generation addresses, ..., anything - * that doesn't need to be communicated). Clients that do not support - * separate keychains for these should use the external one for - * everything. - * - * - m/iH/0/k corresponds to the k'th keypair of the external chain of account number i of the HDW derived from master m. - */ - /* Hence child 0, then child 0 again to get extkey to derive from. */ - if (bip32_key_from_parent(&master_extkey, 0, BIP32_FLAG_KEY_PRIVATE, - &child_extkey) != WALLY_OK) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Can't derive child bip32 key"); - - if (bip32_key_from_parent(&child_extkey, 0, BIP32_FLAG_KEY_PRIVATE, - &secretstuff.bip32) != WALLY_OK) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Can't derive private bip32 key"); -} - -static void bitcoin_pubkey(struct pubkey *pubkey, u32 index) -{ - struct ext_key ext; - - if (index >= BIP32_INITIAL_HARDENED_CHILD) - status_failed(STATUS_FAIL_MASTER_IO, - "Index %u too great", index); - - if (bip32_key_from_parent(&secretstuff.bip32, index, - BIP32_FLAG_KEY_PUBLIC, &ext) != WALLY_OK) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "BIP32 of %u failed", index); - - if (!secp256k1_ec_pubkey_parse(secp256k1_ctx, &pubkey->pubkey, - ext.pub_key, sizeof(ext.pub_key))) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Parse of BIP32 child %u pubkey failed", index); -} - -static void bitcoin_keypair(struct privkey *privkey, - struct pubkey *pubkey, - u32 index) -{ - struct ext_key ext; - - if (index >= BIP32_INITIAL_HARDENED_CHILD) - status_failed(STATUS_FAIL_MASTER_IO, - "Index %u too great", index); - - if (bip32_key_from_parent(&secretstuff.bip32, index, - BIP32_FLAG_KEY_PRIVATE, &ext) != WALLY_OK) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "BIP32 of %u failed", index); - - /* libwally says: The private key with prefix byte 0 */ - memcpy(privkey->secret.data, ext.priv_key+1, 32); - if (!secp256k1_ec_pubkey_create(secp256k1_ctx, &pubkey->pubkey, - privkey->secret.data)) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "BIP32 pubkey %u create failed", index); -} - -static void maybe_create_new_hsm(void) -{ - int fd = open("hsm_secret", O_CREAT|O_EXCL|O_WRONLY, 0400); - if (fd < 0) { - if (errno == EEXIST) - return; - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "creating: %s", strerror(errno)); - } - - randombytes_buf(&secretstuff.hsm_secret, sizeof(secretstuff.hsm_secret)); - if (!write_all(fd, &secretstuff.hsm_secret, sizeof(secretstuff.hsm_secret))) { - unlink_noerr("hsm_secret"); - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "writing: %s", strerror(errno)); - } - if (fsync(fd) != 0) { - unlink_noerr("hsm_secret"); - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "fsync: %s", strerror(errno)); - } - if (close(fd) != 0) { - unlink_noerr("hsm_secret"); - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "closing: %s", strerror(errno)); - } - fd = open(".", O_RDONLY); - if (fd < 0) { - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "opening: %s", strerror(errno)); - } - if (fsync(fd) != 0) { - unlink_noerr("hsm_secret"); - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "fsyncdir: %s", strerror(errno)); - } - close(fd); - status_unusual("HSM: created new hsm_secret file"); -} - -static void load_hsm(void) -{ - int fd = open("hsm_secret", O_RDONLY); - if (fd < 0) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "opening: %s", strerror(errno)); - if (!read_all(fd, &secretstuff.hsm_secret, sizeof(secretstuff.hsm_secret))) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "reading: %s", strerror(errno)); - close(fd); - - populate_secretstuff(); -} - -static void init_hsm(struct daemon_conn *master, const u8 *msg) -{ - - if (!fromwire_hsm_init(msg)) - master_badmsg(WIRE_HSM_INIT, msg); - - maybe_create_new_hsm(); - load_hsm(); - - send_init_response(master); -} - -static void pass_client_hsmfd(struct daemon_conn *master, const u8 *msg) -{ - int fds[2]; - u64 dbid, capabilities; - struct pubkey id; - - if (!fromwire_hsm_client_hsmfd(msg, &id, &dbid, &capabilities)) - master_badmsg(WIRE_HSM_CLIENT_HSMFD, msg); - - if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) != 0) - status_failed(STATUS_FAIL_INTERNAL_ERROR, "creating fds: %s", strerror(errno)); - - new_client(master, &id, dbid, capabilities, fds[0]); - daemon_conn_send(master, - take(towire_hsm_client_hsmfd_reply(NULL))); - daemon_conn_send_fd(master, fds[1]); -} + new_client(master, &id, dbid, capabilities, fds[0]); + daemon_conn_send(master, + take(towire_hsm_client_hsmfd_reply(NULL))); + daemon_conn_send_fd(master, fds[1]); +} static void derive_peer_seed(struct secret *peer_seed, struct secret *peer_seed_base, @@ -1453,11 +1233,224 @@ static void sign_node_announcement(struct daemon_conn *master, const u8 *msg) daemon_conn_send(master, take(reply)); } -#ifndef TESTING -/* FIXME: This is used by debug.c, but doesn't apply to us. */ -extern void dev_disconnect_init(int fd); -void dev_disconnect_init(int fd UNUSED) +static bool check_client_capabilities(struct client *client, + enum hsm_client_wire_type t) +{ + switch (t) { + case WIRE_HSM_ECDH_REQ: + return (client->capabilities & HSM_CAP_ECDH) != 0; + + case WIRE_HSM_CANNOUNCEMENT_SIG_REQ: + case WIRE_HSM_CUPDATE_SIG_REQ: + case WIRE_HSM_NODE_ANNOUNCEMENT_SIG_REQ: + return (client->capabilities & HSM_CAP_SIGN_GOSSIP) != 0; + + case WIRE_HSM_SIGN_DELAYED_PAYMENT_TO_US: + case WIRE_HSM_SIGN_REMOTE_HTLC_TO_US: + case WIRE_HSM_SIGN_PENALTY_TO_US: + case WIRE_HSM_SIGN_LOCAL_HTLC_TX: + return (client->capabilities & HSM_CAP_SIGN_ONCHAIN_TX) != 0; + + case WIRE_HSM_GET_PER_COMMITMENT_POINT: + case WIRE_HSM_CHECK_FUTURE_SECRET: + return (client->capabilities & HSM_CAP_COMMITMENT_POINT) != 0; + + case WIRE_HSM_SIGN_REMOTE_COMMITMENT_TX: + case WIRE_HSM_SIGN_REMOTE_HTLC_TX: + return (client->capabilities & HSM_CAP_SIGN_REMOTE_TX) != 0; + + case WIRE_HSM_SIGN_MUTUAL_CLOSE_TX: + return (client->capabilities & HSM_CAP_SIGN_CLOSING_TX) != 0; + + case WIRE_HSM_INIT: + case WIRE_HSM_CLIENT_HSMFD: + case WIRE_HSM_SIGN_FUNDING: + case WIRE_HSM_SIGN_WITHDRAWAL: + case WIRE_HSM_SIGN_INVOICE: + case WIRE_HSM_SIGN_COMMITMENT_TX: + case WIRE_HSM_GET_CHANNEL_BASEPOINTS: + return (client->capabilities & HSM_CAP_MASTER) != 0; + + /* These are messages sent by the HSM so we should never receive them */ + case WIRE_HSM_ECDH_RESP: + case WIRE_HSM_CANNOUNCEMENT_SIG_REPLY: + case WIRE_HSM_CUPDATE_SIG_REPLY: + case WIRE_HSM_CLIENT_HSMFD_REPLY: + case WIRE_HSM_SIGN_FUNDING_REPLY: + case WIRE_HSM_NODE_ANNOUNCEMENT_SIG_REPLY: + case WIRE_HSM_SIGN_WITHDRAWAL_REPLY: + case WIRE_HSM_SIGN_INVOICE_REPLY: + case WIRE_HSM_INIT_REPLY: + case WIRE_HSMSTATUS_CLIENT_BAD_REQUEST: + case WIRE_HSM_SIGN_COMMITMENT_TX_REPLY: + case WIRE_HSM_SIGN_TX_REPLY: + case WIRE_HSM_GET_PER_COMMITMENT_POINT_REPLY: + case WIRE_HSM_CHECK_FUTURE_SECRET_REPLY: + case WIRE_HSM_GET_CHANNEL_BASEPOINTS_REPLY: + break; + } + return false; +} + +static struct io_plan *handle_client(struct io_conn *conn, + struct daemon_conn *dc) +{ + struct client *c = container_of(dc, struct client, dc); + enum hsm_client_wire_type t = fromwire_peektype(dc->msg_in); + + status_debug("Client: Received message %d from client", t); + + /* Before we do anything else, is this client allowed to do + * what he asks for? */ + if (!check_client_capabilities(c, t)) { + status_broken("Client does not have the required capability to run %d", t); + daemon_conn_send(c->master, + take(towire_hsmstatus_client_bad_request( + NULL, &c->id, dc->msg_in))); + return io_close(conn); + } + + /* Now actually go and do what the client asked for */ + switch (t) { + case WIRE_HSM_INIT: + init_hsm(dc, dc->msg_in); + return daemon_conn_read_next(conn, dc); + + case WIRE_HSM_CLIENT_HSMFD: + pass_client_hsmfd(dc, dc->msg_in); + return daemon_conn_read_next(conn, dc); + + case WIRE_HSM_GET_CHANNEL_BASEPOINTS: + return handle_get_channel_basepoints(conn, dc); + + case WIRE_HSM_ECDH_REQ: + return handle_ecdh(conn, dc); + + case WIRE_HSM_CANNOUNCEMENT_SIG_REQ: + return handle_cannouncement_sig(conn, c); + + case WIRE_HSM_CUPDATE_SIG_REQ: + return handle_channel_update_sig(conn, dc); + + case WIRE_HSM_SIGN_FUNDING: + sign_funding_tx(dc, dc->msg_in); + return daemon_conn_read_next(conn, dc); + + case WIRE_HSM_NODE_ANNOUNCEMENT_SIG_REQ: + sign_node_announcement(dc, dc->msg_in); + return daemon_conn_read_next(conn, dc); + + case WIRE_HSM_SIGN_INVOICE: + sign_invoice(dc, dc->msg_in); + return daemon_conn_read_next(conn, dc); + + case WIRE_HSM_SIGN_WITHDRAWAL: + sign_withdrawal_tx(dc, dc->msg_in); + return daemon_conn_read_next(conn, dc); + + case WIRE_HSM_SIGN_COMMITMENT_TX: + return handle_sign_commitment_tx(conn, dc); + + case WIRE_HSM_SIGN_DELAYED_PAYMENT_TO_US: + return handle_sign_delayed_payment_to_us(conn, c); + + case WIRE_HSM_SIGN_REMOTE_HTLC_TO_US: + return handle_sign_remote_htlc_to_us(conn, c); + + case WIRE_HSM_SIGN_PENALTY_TO_US: + return handle_sign_penalty_to_us(conn, c); + + case WIRE_HSM_SIGN_LOCAL_HTLC_TX: + return handle_sign_local_htlc_tx(conn, c); + + case WIRE_HSM_GET_PER_COMMITMENT_POINT: + return handle_get_per_commitment_point(conn, c); + + case WIRE_HSM_CHECK_FUTURE_SECRET: + return handle_check_future_secret(conn, c); + + case WIRE_HSM_SIGN_REMOTE_COMMITMENT_TX: + return handle_sign_remote_commitment_tx(conn, c); + + case WIRE_HSM_SIGN_REMOTE_HTLC_TX: + return handle_sign_remote_htlc_tx(conn, c); + + case WIRE_HSM_SIGN_MUTUAL_CLOSE_TX: + return handle_sign_mutual_close_tx(conn, c); + + case WIRE_HSM_ECDH_RESP: + case WIRE_HSM_CANNOUNCEMENT_SIG_REPLY: + case WIRE_HSM_CUPDATE_SIG_REPLY: + case WIRE_HSM_CLIENT_HSMFD_REPLY: + case WIRE_HSM_SIGN_FUNDING_REPLY: + case WIRE_HSM_NODE_ANNOUNCEMENT_SIG_REPLY: + case WIRE_HSM_SIGN_WITHDRAWAL_REPLY: + case WIRE_HSM_SIGN_INVOICE_REPLY: + case WIRE_HSM_INIT_REPLY: + case WIRE_HSMSTATUS_CLIENT_BAD_REQUEST: + case WIRE_HSM_SIGN_COMMITMENT_TX_REPLY: + case WIRE_HSM_SIGN_TX_REPLY: + case WIRE_HSM_GET_PER_COMMITMENT_POINT_REPLY: + case WIRE_HSM_CHECK_FUTURE_SECRET_REPLY: + case WIRE_HSM_GET_CHANNEL_BASEPOINTS_REPLY: + break; + } + + daemon_conn_send(c->master, + take(towire_hsmstatus_client_bad_request(NULL, + &c->id, + dc->msg_in))); + return io_close(conn); +} + +static void destroy_client(struct client *c) +{ + if (!uintmap_del(&clients, c->dbid)) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "Failed to remove client dbid %"PRIu64, c->dbid); +} + +static struct client *new_client(struct daemon_conn *master, + const struct pubkey *id, + u64 dbid, + const u64 capabilities, + int fd) { + struct client *c = tal(master, struct client); + + if (id) { + c->id = *id; + } else { + memset(&c->id, 0, sizeof(c->id)); + } + c->dbid = dbid; + + c->master = master; + c->capabilities = capabilities; + daemon_conn_init(c, &c->dc, fd, handle_client, NULL); + + /* Free the connection if we exit everything. */ + tal_steal(master, c->dc.conn); + /* Free client when connection freed. */ + tal_steal(c->dc.conn, c); + + if (dbid == 0) { + assert(num_dbid_zero_clients < ARRAY_SIZE(dbid_zero_clients)); + dbid_zero_clients[num_dbid_zero_clients++] = c; + } else { + struct client *old_client = uintmap_get(&clients, dbid); + + /* Close conn and free any old client of this dbid. */ + if (old_client) + io_close(old_client->dc.conn); + + if (!uintmap_add(&clients, dbid, c)) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "Failed inserting dbid %"PRIu64, dbid); + tal_add_destructor(c, destroy_client); + } + + return c; } static void master_gone(struct io_conn *unused UNUSED, struct daemon_conn *dc UNUSED) @@ -1494,4 +1487,3 @@ int main(int argc, char *argv[]) io_loop(NULL, NULL); abort(); } -#endif From 3e63d88ad156f11b625b939b8e8e3395c1d56e37 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 12:36:42 +0930 Subject: [PATCH 08/20] hsmd: rename per-peer to per-channel. And remove cut&paste of derive_peer_seed. Signed-off-by: Rusty Russell --- hsmd/hsmd.c | 39 ++++++++++++--------------------------- 1 file changed, 12 insertions(+), 27 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 59c29a2c2205..7734600d7c83 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -103,15 +103,14 @@ static void node_key(struct privkey *node_privkey, struct pubkey *node_id) } /** - * hsm_peer_secret_base -- Derive the base secret seed for per-peer seeds + * hsm_channel_secret_base -- Derive the base secret seed for per-channel seeds * - * This secret is shared by all channels/peers for the client. The - * per-peer seeds will be generated from it by mixing in the - * channel_id and the peer node_id. + * This secret is the basis for all per-channel secrets: the per-channel seeds + * will be generated mixing in the channel_id and the peer node_id. */ -static void hsm_peer_secret_base(struct secret *peer_seed_base) +static void hsm_channel_secret_base(struct secret *channel_seed_base) { - hkdf_sha256(peer_seed_base, sizeof(struct secret), NULL, 0, + hkdf_sha256(channel_seed_base, sizeof(struct secret), NULL, 0, &secretstuff.hsm_secret, sizeof(secretstuff.hsm_secret), "peer seed", strlen("peer seed")); } @@ -119,17 +118,17 @@ static void hsm_peer_secret_base(struct secret *peer_seed_base) static void get_channel_seed(const struct pubkey *peer_id, u64 dbid, struct secret *channel_seed) { - struct secret peer_base; + struct secret channel_base; u8 input[PUBKEY_DER_LEN + sizeof(dbid)]; const char *info = "per-peer seed"; - hsm_peer_secret_base(&peer_base); + hsm_channel_secret_base(&channel_base); pubkey_to_der(input, peer_id); memcpy(input + PUBKEY_DER_LEN, &dbid, sizeof(dbid)); hkdf_sha256(channel_seed, sizeof(*channel_seed), input, sizeof(input), - &peer_base, sizeof(peer_base), + &channel_base, sizeof(channel_base), info, strlen(info)); } @@ -970,29 +969,15 @@ static void pass_client_hsmfd(struct daemon_conn *master, const u8 *msg) } -static void derive_peer_seed(struct secret *peer_seed, struct secret *peer_seed_base, - const struct pubkey *peer_id, const u64 channel_id) -{ - u8 input[PUBKEY_DER_LEN + sizeof(channel_id)]; - char *info = "per-peer seed"; - pubkey_to_der(input, peer_id); - memcpy(input + PUBKEY_DER_LEN, &channel_id, sizeof(channel_id)); - - hkdf_sha256(peer_seed, sizeof(*peer_seed), - input, sizeof(input), - peer_seed_base, sizeof(*peer_seed_base), - info, strlen(info)); -} - static void hsm_unilateral_close_privkey(struct privkey *dst, struct unilateral_close_info *info) { - struct secret peer_seed, peer_seed_base; + struct secret channel_seed; struct basepoints basepoints; struct secrets secrets; - hsm_peer_secret_base(&peer_seed_base); - derive_peer_seed(&peer_seed, &peer_seed_base, &info->peer_id, info->channel_id); - derive_basepoints(&peer_seed, NULL, &basepoints, &secrets, NULL); + + get_channel_seed(&info->peer_id, info->channel_id, &channel_seed); + derive_basepoints(&channel_seed, NULL, &basepoints, &secrets, NULL); if (!derive_simple_privkey(&secrets.payment_basepoint_secret, &basepoints.payment, &info->commitment_point, From 1e4e476c9e16865ab3625c9b4b6f107e83e2c0bc Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 12:36:42 +0930 Subject: [PATCH 09/20] hsmd: implement bitcoin_key() to subsume bitcoin_pubkey and bitcoin_keypair. This mirrors the node_key() interface we already have. Signed-off-by: Rusty Russell --- hsmd/hsmd.c | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 7734600d7c83..62c783cbe5b2 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -192,30 +192,15 @@ static void populate_secretstuff(void) "Can't derive private bip32 key"); } -static void bitcoin_pubkey(struct pubkey *pubkey, u32 index) +/* If privkey is NULL, we don't fill it in */ +static void bitcoin_key(struct privkey *privkey, struct pubkey *pubkey, + u32 index) { struct ext_key ext; + struct privkey unused_priv; - if (index >= BIP32_INITIAL_HARDENED_CHILD) - status_failed(STATUS_FAIL_MASTER_IO, - "Index %u too great", index); - - if (bip32_key_from_parent(&secretstuff.bip32, index, - BIP32_FLAG_KEY_PUBLIC, &ext) != WALLY_OK) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "BIP32 of %u failed", index); - - if (!secp256k1_ec_pubkey_parse(secp256k1_ctx, &pubkey->pubkey, - ext.pub_key, sizeof(ext.pub_key))) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Parse of BIP32 child %u pubkey failed", index); -} - -static void bitcoin_keypair(struct privkey *privkey, - struct pubkey *pubkey, - u32 index) -{ - struct ext_key ext; + if (privkey == NULL) + privkey = &unused_priv; if (index >= BIP32_INITIAL_HARDENED_CHILD) status_failed(STATUS_FAIL_MASTER_IO, @@ -1002,7 +987,7 @@ static void hsm_key_for_utxo(struct privkey *privkey, struct pubkey *pubkey, status_debug("Derived public key %s from unilateral close", type_to_string(tmpctx, struct pubkey, pubkey)); } else { /* Simple case: just get derive via HD-derivation */ - bitcoin_keypair(privkey, pubkey, utxo->keyindex); + bitcoin_key(privkey, pubkey, utxo->keyindex); } } @@ -1029,7 +1014,7 @@ static void sign_funding_tx(struct daemon_conn *master, const u8 *msg) if (change_out) { changekey = tal(tmpctx, struct pubkey); - bitcoin_pubkey(changekey, change_keyindex); + bitcoin_key(NULL, changekey, change_keyindex); } else changekey = NULL; From da9d92960d5eaab83794ce7ecc5fded15e13d7a0 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 12:36:42 +0930 Subject: [PATCH 10/20] lightningd: accept hsmstatus_client_bad_request messages (and log!) We currently just ignore them. This is one reason the hsm (in some places) explicitly calls log_broken so we get some idea. This was the only subdaemon which had a NULL msgcb and msgname, so eliminate those checks in subd.c. Signed-off-by: Rusty Russell --- lightningd/hsm_control.c | 22 +++++++++++++++++++++- lightningd/subd.c | 38 +++++++++++++++++++------------------- lightningd/subd.h | 2 +- tests/fixtures.py | 11 +++++++++++ 4 files changed, 52 insertions(+), 21 deletions(-) diff --git a/lightningd/hsm_control.c b/lightningd/hsm_control.c index 5d67e3bfac44..b0c313d54d87 100644 --- a/lightningd/hsm_control.c +++ b/lightningd/hsm_control.c @@ -42,6 +42,24 @@ int hsm_get_client_fd(struct lightningd *ld, return hsm_fd; } +static unsigned int hsm_msg(struct subd *hsmd, + const u8 *msg, const int *fds UNUSED) +{ + /* We only expect one thing from the HSM that's not a STATUS message */ + struct pubkey client_id; + u8 *bad_msg; + + if (!fromwire_hsmstatus_client_bad_request(tmpctx, msg, &client_id, + &bad_msg)) + fatal("Bad status message from hsmd: %s", tal_hex(tmpctx, msg)); + + /* This should, of course, never happen. */ + log_broken(hsmd->log, "client %s sent bad hsm request %s", + type_to_string(tmpctx, struct pubkey, &client_id), + tal_hex(tmpctx, bad_msg)); + return 0; +} + void hsm_init(struct lightningd *ld) { u8 *msg; @@ -51,7 +69,9 @@ void hsm_init(struct lightningd *ld) if (socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) != 0) err(1, "Could not create hsm socketpair"); - ld->hsm = new_global_subd(ld, "lightning_hsmd", NULL, NULL, + ld->hsm = new_global_subd(ld, "lightning_hsmd", + hsm_client_wire_type_name, + hsm_msg, take(&fds[1]), NULL); if (!ld->hsm) err(1, "Could not subd hsm"); diff --git a/lightningd/subd.c b/lightningd/subd.c index 589f1a412461..c6a667879233 100644 --- a/lightningd/subd.c +++ b/lightningd/subd.c @@ -400,6 +400,8 @@ static struct io_plan *sd_msg_read(struct io_conn *conn, struct subd *sd) struct subd_req *sr; struct db *db = sd->ld->wallet->db; struct io_plan *plan; + unsigned int i; + bool freed = false; /* Everything we do, we wrap in a database transaction */ db_begin_transaction(db); @@ -464,29 +466,25 @@ static struct io_plan *sd_msg_read(struct io_conn *conn, struct subd *sd) } log_debug(sd->log, "UPDATE %s", sd->msgname(type)); - if (sd->msgcb) { - unsigned int i; - bool freed = false; - /* Might free sd (if returns negative); save/restore sd->conn */ - sd->conn = NULL; - tal_add_destructor2(sd, mark_freed, &freed); + /* Might free sd (if returns negative); save/restore sd->conn */ + sd->conn = NULL; + tal_add_destructor2(sd, mark_freed, &freed); - i = sd->msgcb(sd, sd->msg_in, sd->fds_in); - if (freed) - goto close; - tal_del_destructor2(sd, mark_freed, &freed); + i = sd->msgcb(sd, sd->msg_in, sd->fds_in); + if (freed) + goto close; + tal_del_destructor2(sd, mark_freed, &freed); - sd->conn = conn; + sd->conn = conn; - if (i != 0) { - /* Don't ask for fds twice! */ - assert(!sd->fds_in); - /* Don't free msg_in: we go around again. */ - tal_steal(sd, sd->msg_in); - plan = sd_collect_fds(conn, sd, i); - goto out; - } + if (i != 0) { + /* Don't ask for fds twice! */ + assert(!sd->fds_in); + /* Don't free msg_in: we go around again. */ + tal_steal(sd, sd->msg_in); + plan = sd_collect_fds(conn, sd, i); + goto out; } next: @@ -651,7 +649,9 @@ static struct subd *new_subd(struct lightningd *ld, sd->must_not_exit = false; sd->talks_to_peer = talks_to_peer; sd->msgname = msgname; + assert(msgname); sd->msgcb = msgcb; + assert(msgcb); sd->errcb = errcb; sd->billboardcb = billboardcb; sd->fds_in = NULL; diff --git a/lightningd/subd.h b/lightningd/subd.h index 72e929eab20a..a0ea87a36fd4 100644 --- a/lightningd/subd.h +++ b/lightningd/subd.h @@ -76,7 +76,7 @@ struct subd { * @ld: global state * @name: basename of daemon * @msgname: function to get name from messages - * @msgcb: function to call (inside db transaction) when non-fatal message received (or NULL) + * @msgcb: function to call (inside db transaction) when non-fatal message received * @...: NULL-terminated list of pointers to fds to hand as fd 3, 4... * (can be take, if so, set to -1) * diff --git a/tests/fixtures.py b/tests/fixtures.py index a097e249895d..b9a2e07a4ee1 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -134,6 +134,11 @@ def check_errors(request, err_count, msg): err_count += checkBadReestablish(node) check_errors(request, err_count, "{} nodes had bad reestablish") + for node in nf.nodes: + err_count += checkBadHSMRequest(node) + if err_count: + raise ValueError("{} nodes had bad hsm requests".format(err_count)) + if not ok: request.node.has_errors = True raise Exception("At least one lightning exited with unexpected non-zero return code") @@ -201,6 +206,12 @@ def checkBadReestablish(node): return 0 +def checkBadHSMRequest(node): + if node.daemon.is_in_log('bad hsm request'): + return 1 + return 0 + + @pytest.fixture def executor(): ex = futures.ThreadPoolExecutor(max_workers=20) From 6b6b7eac61491c8a7ed4c6e939570221f1286f7e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 12:36:42 +0930 Subject: [PATCH 11/20] hsmd: use status_conn to send bad_request messages, make handlers uniform. The current code sends hsmstatus_client_bad_request via the req fd; this won't work, since lightningd uses that synchronously and only expects a reply to its commands. So send it via status_conn. We also enhance hsmstatus_client_bad_request to include details, and create convenience functions for it. Our previous handling was ad-hoc; we sometimes just closed on the client without telling lightningd, and sometimes we didn't tell lightningd *which* client was broken. Also make every handler the exact same prototype, so they now use the exact same patterns (hsmd *only* handles requests, makes replies). I tested this manually by corrupting a request to hsmd. Signed-off-by: Rusty Russell --- hsmd/hsm_client_wire_csv | 1 + hsmd/hsmd.c | 614 +++++++++++++++++---------------------- lightningd/hsm_control.c | 7 +- 3 files changed, 269 insertions(+), 353 deletions(-) diff --git a/hsmd/hsm_client_wire_csv b/hsmd/hsm_client_wire_csv index 2103d0b0fe36..44a58b186638 100644 --- a/hsmd/hsm_client_wire_csv +++ b/hsmd/hsm_client_wire_csv @@ -1,6 +1,7 @@ # Clients should not give a bad request but not the HSM's decision to crash. hsmstatus_client_bad_request,1000 hsmstatus_client_bad_request,,id,struct pubkey +hsmstatus_client_bad_request,,description,wirestring hsmstatus_client_bad_request,,len,u16 hsmstatus_client_bad_request,,msg,len*u8 diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 62c783cbe5b2..0c1b2b4e0b0e 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -69,6 +69,9 @@ static UINTMAP(struct client *) clients; static struct client *dbid_zero_clients[3]; static size_t num_dbid_zero_clients; +/* For reporting issues. */ +static struct daemon_conn *status_conn; + /* FIXME: This is used by debug.c, but doesn't apply to us. */ extern void dev_disconnect_init(int fd); void dev_disconnect_init(int fd UNUSED) { } @@ -80,6 +83,48 @@ static struct client *new_client(struct daemon_conn *master, const u64 capabilities, int fd); +static PRINTF_FMT(4,5) + struct io_plan *bad_req_fmt(struct io_conn *conn, + struct client *c, + const u8 *msg_in, + const char *fmt, ...) +{ + va_list ap; + char *str; + + va_start(ap, fmt); + str = tal_fmt(tmpctx, fmt, ap); + va_end(ap); + + /* If the client was actually lightningd, it's Game Over. */ + if (&c->dc == c->master) { + status_broken("%s", str); + master_badmsg(fromwire_peektype(msg_in), msg_in); + } + + daemon_conn_send(status_conn, + take(towire_hsmstatus_client_bad_request(NULL, + &c->id, + str, + msg_in))); + return io_close(conn); +} + +static struct io_plan *bad_req(struct io_conn *conn, + struct client *c, + const u8 *msg_in) +{ + return bad_req_fmt(conn, c, msg_in, "could not parse request"); +} + +static struct io_plan *req_reply(struct io_conn *conn, + struct client *c, + const u8 *msg_out TAKES) +{ + daemon_conn_send(&c->dc, msg_out); + return daemon_conn_read_next(conn, &c->dc); +} + static void node_key(struct privkey *node_privkey, struct pubkey *node_id) { u32 salt = 0; @@ -132,17 +177,6 @@ static void get_channel_seed(const struct pubkey *peer_id, u64 dbid, info, strlen(info)); } -static void send_init_response(struct daemon_conn *master) -{ - struct pubkey node_id; - u8 *msg; - - node_key(NULL, &node_id); - - msg = towire_hsm_init_reply(NULL, &node_id, &secretstuff.bip32); - daemon_conn_send(master, take(msg)); -} - static void populate_secretstuff(void) { u8 bip32_seed[BIP32_ENTROPY_LEN_256]; @@ -273,52 +307,51 @@ static void load_hsm(void) populate_secretstuff(); } -static void init_hsm(struct daemon_conn *master, const u8 *msg) +static struct io_plan *init_hsm(struct io_conn *conn, + struct client *c, + const u8 *msg_in) { - if (!fromwire_hsm_init(msg)) - master_badmsg(WIRE_HSM_INIT, msg); + struct pubkey node_id; + + /* This must be the master. */ + assert(&c->dc == c->master); + + if (!fromwire_hsm_init(msg_in)) + return bad_req(conn, c, msg_in); maybe_create_new_hsm(); load_hsm(); - send_init_response(master); + node_key(NULL, &node_id); + return req_reply(conn, c, + take(towire_hsm_init_reply(NULL, &node_id, + &secretstuff.bip32))); } -static struct io_plan *handle_ecdh(struct io_conn *conn, struct daemon_conn *dc) +static struct io_plan *handle_ecdh(struct io_conn *conn, + struct client *c, + const u8 *msg_in) { - struct client *c = container_of(dc, struct client, dc); struct privkey privkey; struct pubkey point; struct secret ss; - if (!fromwire_hsm_ecdh_req(dc->msg_in, &point)) { - daemon_conn_send(c->master, - take(towire_hsmstatus_client_bad_request(NULL, - &c->id, - dc->msg_in))); - return io_close(conn); - } + if (!fromwire_hsm_ecdh_req(msg_in, &point)) + return bad_req(conn, c, msg_in); node_key(&privkey, NULL); if (secp256k1_ecdh(secp256k1_ctx, ss.data, &point.pubkey, privkey.secret.data) != 1) { - status_broken("secp256k1_ecdh fail for client %s", - type_to_string(tmpctx, struct pubkey, &c->id)); - daemon_conn_send(c->master, - take(towire_hsmstatus_client_bad_request(NULL, - &c->id, - dc->msg_in))); - return io_close(conn); + return bad_req_fmt(conn, c, msg_in, "secp256k1_ecdh fail"); } - daemon_conn_send(dc, take(towire_hsm_ecdh_resp(NULL, &ss))); - return daemon_conn_read_next(conn, dc); + return req_reply(conn, c, take(towire_hsm_ecdh_resp(NULL, &ss))); } static struct io_plan *handle_cannouncement_sig(struct io_conn *conn, - struct client *c) + struct client *c, + const u8 *msg_in) { - struct daemon_conn *dc = &c->dc; /* First 2 + 256 byte are the signatures and msg type, skip them */ size_t offset = 258; struct privkey node_pkey; @@ -334,16 +367,13 @@ static struct io_plan *handle_cannouncement_sig(struct io_conn *conn, get_channel_seed(&c->id, c->dbid, &channel_seed); derive_funding_key(&channel_seed, &funding_pubkey, &funding_privkey); - if (!fromwire_hsm_cannouncement_sig_req(tmpctx, dc->msg_in, &ca)) { - status_broken("Failed to parse cannouncement_sig_req: %s", - tal_hex(tmpctx, dc->msg_in)); - return io_close(conn); - } + if (!fromwire_hsm_cannouncement_sig_req(tmpctx, msg_in, &ca)) + return bad_req(conn, c, msg_in); - if (tal_count(ca) < offset) { - status_broken("bad cannounce length %zu", tal_count(ca)); - return io_close(conn); - } + if (tal_count(ca) < offset) + return bad_req_fmt(conn, c, msg_in, + "bad cannounce length %zu", + tal_count(ca)); /* TODO(cdecker) Check that this is actually a valid * channel_announcement */ @@ -355,13 +385,12 @@ static struct io_plan *handle_cannouncement_sig(struct io_conn *conn, reply = towire_hsm_cannouncement_sig_reply(NULL, &node_sig, &bitcoin_sig); - daemon_conn_send(dc, take(reply)); - - return daemon_conn_read_next(conn, dc); + return req_reply(conn, c, take(reply)); } static struct io_plan *handle_channel_update_sig(struct io_conn *conn, - struct daemon_conn *dc) + struct client *c, + const u8 *msg_in) { /* 2 bytes msg type + 64 bytes signature */ size_t offset = 66; @@ -375,26 +404,18 @@ static struct io_plan *handle_channel_update_sig(struct io_conn *conn, struct bitcoin_blkid chain_hash; u8 *cu; - if (!fromwire_hsm_cupdate_sig_req(tmpctx, dc->msg_in, &cu)) { - status_broken("Failed to parse %s: %s", - hsm_client_wire_type_name(fromwire_peektype(dc->msg_in)), - tal_hex(tmpctx, dc->msg_in)); - return io_close(conn); - } + if (!fromwire_hsm_cupdate_sig_req(tmpctx, msg_in, &cu)) + return bad_req(conn, c, msg_in); if (!fromwire_channel_update(cu, &sig, &chain_hash, &scid, ×tamp, &flags, &cltv_expiry_delta, &htlc_minimum_msat, &fee_base_msat, &fee_proportional_mill)) { - status_broken("Failed to parse inner channel_update: %s", - tal_hex(tmpctx, dc->msg_in)); - return io_close(conn); - } - if (tal_count(cu) < offset) { - status_broken("inner channel_update too short: %s", - tal_hex(tmpctx, dc->msg_in)); - return io_close(conn); + return bad_req_fmt(conn, c, msg_in, "Bad inner channel_update"); } + if (tal_count(cu) < offset) + return bad_req_fmt(conn, c, msg_in, + "inner channel_update too short"); node_key(&node_pkey, NULL); sha256_double(&hash, cu + offset, tal_count(cu) - offset); @@ -405,13 +426,12 @@ static struct io_plan *handle_channel_update_sig(struct io_conn *conn, &scid, timestamp, flags, cltv_expiry_delta, htlc_minimum_msat, fee_base_msat, fee_proportional_mill); - - daemon_conn_send(dc, take(towire_hsm_cupdate_sig_reply(NULL, cu))); - return daemon_conn_read_next(conn, dc); + return req_reply(conn, c, take(towire_hsm_cupdate_sig_reply(NULL, cu))); } static struct io_plan *handle_get_channel_basepoints(struct io_conn *conn, - struct daemon_conn *dc) + struct client *c, + const u8 *msg_in) { struct pubkey peer_id; u64 dbid; @@ -419,22 +439,22 @@ static struct io_plan *handle_get_channel_basepoints(struct io_conn *conn, struct basepoints basepoints; struct pubkey funding_pubkey; - if (!fromwire_hsm_get_channel_basepoints(dc->msg_in, &peer_id, &dbid)) - master_badmsg(WIRE_HSM_GET_CHANNEL_BASEPOINTS, dc->msg_in); + if (!fromwire_hsm_get_channel_basepoints(msg_in, &peer_id, &dbid)) + return bad_req(conn, c, msg_in); get_channel_seed(&peer_id, dbid, &seed); derive_basepoints(&seed, &funding_pubkey, &basepoints, NULL, NULL); - daemon_conn_send(dc, + return req_reply(conn, c, take(towire_hsm_get_channel_basepoints_reply(NULL, &basepoints, &funding_pubkey))); - return daemon_conn_read_next(conn, dc); } /* FIXME: Ensure HSM never does this twice for same dbid! */ static struct io_plan *handle_sign_commitment_tx(struct io_conn *conn, - struct daemon_conn *dc) + struct client *c, + const u8 *msg_in) { struct pubkey peer_id, remote_funding_pubkey, local_funding_pubkey; u64 dbid, funding_amount; @@ -444,12 +464,12 @@ static struct io_plan *handle_sign_commitment_tx(struct io_conn *conn, struct secrets secrets; const u8 *funding_wscript; - if (!fromwire_hsm_sign_commitment_tx(tmpctx, dc->msg_in, + if (!fromwire_hsm_sign_commitment_tx(tmpctx, msg_in, &peer_id, &dbid, &tx, &remote_funding_pubkey, &funding_amount)) - master_badmsg(WIRE_HSM_SIGN_COMMITMENT_TX, dc->msg_in); + return bad_req(conn, c, msg_in); get_channel_seed(&peer_id, dbid, &channel_seed); derive_basepoints(&channel_seed, @@ -465,37 +485,15 @@ static struct io_plan *handle_sign_commitment_tx(struct io_conn *conn, &local_funding_pubkey, &sig); - daemon_conn_send(dc, + return req_reply(conn, c, take(towire_hsm_sign_commitment_tx_reply(NULL, &sig))); - return daemon_conn_read_next(conn, dc); -} - -static PRINTF_FMT(3,4) - struct io_plan *bad_sign_request(struct io_conn *conn, - struct client *c, - const char *fmt, ...) -{ - va_list ap; - char *str; - - va_start(ap, fmt); - str = tal_fmt(tmpctx, fmt, ap); - status_broken("Client %s bad sign request: %s", - type_to_string(tmpctx, struct pubkey, &c->id), str); - va_end(ap); - - daemon_conn_send(c->master, - take(towire_hsmstatus_client_bad_request(NULL, - &c->id, - c->dc.msg_in))); - return io_close(conn); } /* FIXME: make sure it meets some criteria? */ static struct io_plan *handle_sign_remote_commitment_tx(struct io_conn *conn, - struct client *c) + struct client *c, + const u8 *msg_in) { - struct daemon_conn *dc = &c->dc; struct pubkey remote_funding_pubkey, local_funding_pubkey; u64 funding_amount; struct secret channel_seed; @@ -504,11 +502,11 @@ static struct io_plan *handle_sign_remote_commitment_tx(struct io_conn *conn, struct secrets secrets; const u8 *funding_wscript; - if (!fromwire_hsm_sign_remote_commitment_tx(tmpctx, dc->msg_in, + if (!fromwire_hsm_sign_remote_commitment_tx(tmpctx, msg_in, &tx, &remote_funding_pubkey, &funding_amount)) - master_badmsg(WIRE_HSM_SIGN_REMOTE_COMMITMENT_TX, dc->msg_in); + bad_req(conn, c, msg_in); get_channel_seed(&c->id, c->dbid, &channel_seed); derive_basepoints(&channel_seed, @@ -524,13 +522,13 @@ static struct io_plan *handle_sign_remote_commitment_tx(struct io_conn *conn, &local_funding_pubkey, &sig); - daemon_conn_send(dc, take(towire_hsm_sign_tx_reply(NULL, &sig))); - return daemon_conn_read_next(conn, dc); + return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } /* FIXME: Derive output address for this client, and check it here! */ static struct io_plan *handle_sign_to_us_tx(struct io_conn *conn, struct client *c, + const u8 *msg_in, struct bitcoin_tx *tx, const struct privkey *privkey, const u8 *wscript, @@ -540,20 +538,20 @@ static struct io_plan *handle_sign_to_us_tx(struct io_conn *conn, struct pubkey pubkey; if (!pubkey_from_privkey(privkey, &pubkey)) - return bad_sign_request(conn, c, "bad pubkey_from_privkey"); + return bad_req_fmt(conn, c, msg_in, "bad pubkey_from_privkey"); if (tal_count(tx->input) != 1) - return bad_sign_request(conn, c, "bad txinput count"); + return bad_req_fmt(conn, c, msg_in, "bad txinput count"); tx->input[0].amount = tal_dup(tx->input, u64, &input_amount); sign_tx_input(tx, 0, NULL, wscript, privkey, &pubkey, &sig); - daemon_conn_send(&c->dc, take(towire_hsm_sign_tx_reply(NULL, &sig))); - return daemon_conn_read_next(conn, &c->dc); + return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } static struct io_plan *handle_sign_delayed_payment_to_us(struct io_conn *conn, - struct client *c) + struct client *c, + const u8 *msg_in) { u64 commit_num, input_amount; struct secret channel_seed, basepoint_secret; @@ -564,40 +562,40 @@ static struct io_plan *handle_sign_delayed_payment_to_us(struct io_conn *conn, struct privkey privkey; u8 *wscript; - if (!fromwire_hsm_sign_delayed_payment_to_us(tmpctx, c->dc.msg_in, + if (!fromwire_hsm_sign_delayed_payment_to_us(tmpctx, msg_in, &commit_num, &tx, &wscript, &input_amount)) - return bad_sign_request(conn, c, - "malformed hsm_sign_delayed_payment"); + return bad_req(conn, c, msg_in); get_channel_seed(&c->id, c->dbid, &channel_seed); if (!derive_shaseed(&channel_seed, &shaseed)) - return bad_sign_request(conn, c, "bad derive_shaseed"); + return bad_req_fmt(conn, c, msg_in, "bad derive_shaseed"); if (!per_commit_point(&shaseed, &per_commitment_point, commit_num)) - return bad_sign_request(conn, c, - "bad per_commitment_point %"PRIu64, - commit_num); + return bad_req_fmt(conn, c, msg_in, + "bad per_commitment_point %"PRIu64, + commit_num); if (!derive_delayed_payment_basepoint(&channel_seed, &basepoint, &basepoint_secret)) - return bad_sign_request(conn, c, "failed deriving basepoint"); + return bad_req_fmt(conn, c, msg_in, "failed deriving basepoint"); if (!derive_simple_privkey(&basepoint_secret, &basepoint, &per_commitment_point, &privkey)) - return bad_sign_request(conn, c, "failed deriving privkey"); + return bad_req_fmt(conn, c, msg_in, "failed deriving privkey"); - return handle_sign_to_us_tx(conn, c, tx, &privkey, wscript, - input_amount); + return handle_sign_to_us_tx(conn, c, msg_in, + tx, &privkey, wscript, input_amount); } static struct io_plan *handle_sign_remote_htlc_to_us(struct io_conn *conn, - struct client *c) + struct client *c, + const u8 *msg_in) { u64 input_amount; struct secret channel_seed, htlc_basepoint_secret; @@ -607,33 +605,33 @@ static struct io_plan *handle_sign_remote_htlc_to_us(struct io_conn *conn, struct privkey privkey; u8 *wscript; - if (!fromwire_hsm_sign_remote_htlc_to_us(tmpctx, c->dc.msg_in, + if (!fromwire_hsm_sign_remote_htlc_to_us(tmpctx, msg_in, &remote_per_commitment_point, &tx, &wscript, &input_amount)) - return bad_sign_request(conn, c, - "malformed hsm_sign_remote_htlc_to_us"); + return bad_req(conn, c, msg_in); get_channel_seed(&c->id, c->dbid, &channel_seed); if (!derive_htlc_basepoint(&channel_seed, &htlc_basepoint, &htlc_basepoint_secret)) - return bad_sign_request(conn, c, - "Failed derive_htlc_basepoint"); + return bad_req_fmt(conn, c, msg_in, + "Failed derive_htlc_basepoint"); if (!derive_simple_privkey(&htlc_basepoint_secret, &htlc_basepoint, &remote_per_commitment_point, &privkey)) - return bad_sign_request(conn, c, - "Failed deriving htlc privkey"); + return bad_req_fmt(conn, c, msg_in, + "Failed deriving htlc privkey"); - return handle_sign_to_us_tx(conn, c, tx, &privkey, wscript, - input_amount); + return handle_sign_to_us_tx(conn, c, msg_in, + tx, &privkey, wscript, input_amount); } static struct io_plan *handle_sign_penalty_to_us(struct io_conn *conn, - struct client *c) + struct client *c, + const u8 *msg_in) { u64 input_amount; struct secret channel_seed, revocation_secret, revocation_basepoint_secret; @@ -643,38 +641,37 @@ static struct io_plan *handle_sign_penalty_to_us(struct io_conn *conn, struct privkey privkey; u8 *wscript; - if (!fromwire_hsm_sign_penalty_to_us(tmpctx, c->dc.msg_in, + if (!fromwire_hsm_sign_penalty_to_us(tmpctx, msg_in, &revocation_secret, &tx, &wscript, &input_amount)) - return bad_sign_request(conn, c, - "malformed hsm_sign_penalty_to_us"); + return bad_req(conn, c, msg_in); if (!pubkey_from_secret(&revocation_secret, &point)) - return bad_sign_request(conn, c, - "Failed deriving pubkey"); + return bad_req_fmt(conn, c, msg_in, "Failed deriving pubkey"); get_channel_seed(&c->id, c->dbid, &channel_seed); if (!derive_revocation_basepoint(&channel_seed, &revocation_basepoint, &revocation_basepoint_secret)) - return bad_sign_request(conn, c, - "Failed deriving revocation basepoint"); + return bad_req_fmt(conn, c, msg_in, + "Failed deriving revocation basepoint"); if (!derive_revocation_privkey(&revocation_basepoint_secret, &revocation_secret, &revocation_basepoint, &point, &privkey)) - return bad_sign_request(conn, c, - "Failed deriving revocation privkey"); + return bad_req_fmt(conn, c, msg_in, + "Failed deriving revocation privkey"); - return handle_sign_to_us_tx(conn, c, tx, &privkey, wscript, - input_amount); + return handle_sign_to_us_tx(conn, c, msg_in, + tx, &privkey, wscript, input_amount); } static struct io_plan *handle_sign_local_htlc_tx(struct io_conn *conn, - struct client *c) + struct client *c, + const u8 *msg_in) { u64 commit_num, input_amount; struct secret channel_seed, htlc_basepoint_secret; @@ -686,151 +683,113 @@ static struct io_plan *handle_sign_local_htlc_tx(struct io_conn *conn, struct privkey htlc_privkey; struct pubkey htlc_pubkey; - if (!fromwire_hsm_sign_local_htlc_tx(tmpctx, c->dc.msg_in, + if (!fromwire_hsm_sign_local_htlc_tx(tmpctx, msg_in, &commit_num, &tx, &wscript, &input_amount)) - return bad_sign_request(conn, c, - "malformed hsm_sign_local_htlc_tx"); + return bad_req(conn, c, msg_in); get_channel_seed(&c->id, c->dbid, &channel_seed); if (!derive_shaseed(&channel_seed, &shaseed)) - return bad_sign_request(conn, c, "bad derive_shaseed"); + return bad_req_fmt(conn, c, msg_in, "bad derive_shaseed"); if (!per_commit_point(&shaseed, &per_commitment_point, commit_num)) - return bad_sign_request(conn, c, - "bad per_commitment_point %"PRIu64, - commit_num); + return bad_req_fmt(conn, c, msg_in, + "bad per_commitment_point %"PRIu64, + commit_num); if (!derive_htlc_basepoint(&channel_seed, &htlc_basepoint, &htlc_basepoint_secret)) - return bad_sign_request(conn, c, - "Failed deriving htlc basepoint"); + return bad_req_fmt(conn, c, msg_in, + "Failed deriving htlc basepoint"); if (!derive_simple_privkey(&htlc_basepoint_secret, &htlc_basepoint, &per_commitment_point, &htlc_privkey)) - return bad_sign_request(conn, c, - "Failed deriving htlc privkey"); + return bad_req_fmt(conn, c, msg_in, + "Failed deriving htlc privkey"); if (!pubkey_from_privkey(&htlc_privkey, &htlc_pubkey)) - return bad_sign_request(conn, c, "bad pubkey_from_privkey"); + return bad_req_fmt(conn, c, msg_in, "bad pubkey_from_privkey"); if (tal_count(tx->input) != 1) - return bad_sign_request(conn, c, "bad txinput count"); + return bad_req_fmt(conn, c, msg_in, "bad txinput count"); /* FIXME: Check that output script is correct! */ tx->input[0].amount = tal_dup(tx->input, u64, &input_amount); sign_tx_input(tx, 0, NULL, wscript, &htlc_privkey, &htlc_pubkey, &sig); - daemon_conn_send(&c->dc, take(towire_hsm_sign_tx_reply(NULL, &sig))); - return daemon_conn_read_next(conn, &c->dc); + return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } -static struct io_plan * -handle_get_per_commitment_point(struct io_conn *conn, struct client *c) +static struct io_plan *handle_get_per_commitment_point(struct io_conn *conn, + struct client *c, + const u8 *msg_in) { - struct daemon_conn *dc = &c->dc; struct secret channel_seed; struct sha256 shaseed; struct pubkey per_commitment_point; u64 n; struct secret *old_secret; - if (!fromwire_hsm_get_per_commitment_point(dc->msg_in, &n)) { - status_broken("bad get_per_commitment_point for client %s", - type_to_string(tmpctx, struct pubkey, &c->id)); - goto fail; - } + if (!fromwire_hsm_get_per_commitment_point(msg_in, &n)) + return bad_req(conn, c, msg_in); get_channel_seed(&c->id, c->dbid, &channel_seed); - if (!derive_shaseed(&channel_seed, &shaseed)) { - status_broken("bad derive_shaseed for client %s", - type_to_string(tmpctx, struct pubkey, &c->id)); - goto fail; - } + if (!derive_shaseed(&channel_seed, &shaseed)) + return bad_req_fmt(conn, c, msg_in, "bad derive_shaseed"); - if (!per_commit_point(&shaseed, &per_commitment_point, n)) { - status_broken("bad per_commit_point %"PRIu64" for client %s", - n, type_to_string(tmpctx, struct pubkey, &c->id)); - goto fail; - } + if (!per_commit_point(&shaseed, &per_commitment_point, n)) + return bad_req_fmt(conn, c, msg_in, + "bad per_commit_point %"PRIu64, n); if (n >= 2) { old_secret = tal(tmpctx, struct secret); if (!per_commit_secret(&shaseed, old_secret, n - 2)) { - status_broken("Cannot derive secret %"PRIu64 - " for client %s", - n - 1, - type_to_string(tmpctx, - struct pubkey, &c->id)); - goto fail; + return bad_req_fmt(conn, c, msg_in, + "Cannot derive secret %"PRIu64, + n - 2); } } else old_secret = NULL; - daemon_conn_send(&c->dc, + return req_reply(conn, c, take(towire_hsm_get_per_commitment_point_reply(NULL, &per_commitment_point, old_secret))); - return daemon_conn_read_next(conn, &c->dc); - -fail: - daemon_conn_send(c->master, - take(towire_hsmstatus_client_bad_request(NULL, - &c->id, - c->dc.msg_in))); - return io_close(conn); } static struct io_plan *handle_check_future_secret(struct io_conn *conn, - struct client *c) + struct client *c, + const u8 *msg_in) { - struct daemon_conn *dc = &c->dc; struct secret channel_seed; struct sha256 shaseed; u64 n; struct secret secret, suggested; - if (!fromwire_hsm_check_future_secret(dc->msg_in, &n, - &suggested)) { - status_broken("bad check_future_secret for client %s", - type_to_string(tmpctx, struct pubkey, &c->id)); - goto fail; - } + if (!fromwire_hsm_check_future_secret(msg_in, &n, &suggested)) + return bad_req(conn, c, msg_in); get_channel_seed(&c->id, c->dbid, &channel_seed); - if (!derive_shaseed(&channel_seed, &shaseed)) { - status_broken("bad derive_shaseed for client %s", - type_to_string(tmpctx, struct pubkey, &c->id)); - goto fail; - } + if (!derive_shaseed(&channel_seed, &shaseed)) + return bad_req_fmt(conn, c, msg_in, "bad derive_shaseed"); - if (!per_commit_secret(&shaseed, &secret, n)) { - status_broken("bad commit secret #%"PRIu64" for client %s", - n, type_to_string(tmpctx, struct pubkey, &c->id)); - goto fail; - } + if (!per_commit_secret(&shaseed, &secret, n)) + return bad_req_fmt(conn, c, msg_in, + "bad commit secret #%"PRIu64, n); - daemon_conn_send(&c->dc, + return req_reply(conn, c, take(towire_hsm_check_future_secret_reply(NULL, secret_eq_consttime(&secret, &suggested)))); - return daemon_conn_read_next(conn, &c->dc); - -fail: - daemon_conn_send(c->master, - take(towire_hsmstatus_client_bad_request(NULL, - &c->id, - c->dc.msg_in))); - return io_close(conn); } static struct io_plan *handle_sign_remote_htlc_tx(struct io_conn *conn, - struct client *c) + struct client *c, + const u8 *msg_in) { - struct daemon_conn *dc = &c->dc; struct secret channel_seed; struct bitcoin_tx *tx; secp256k1_ecdsa_signature sig; @@ -842,13 +801,10 @@ static struct io_plan *handle_sign_remote_htlc_tx(struct io_conn *conn, struct privkey htlc_privkey; struct pubkey htlc_pubkey; - if (!fromwire_hsm_sign_remote_htlc_tx(tmpctx, dc->msg_in, - &tx, &wscript, &amount, - &remote_per_commit_point)) { - status_broken("bad hsm_sign_remote_htlc_tx for client %s", - type_to_string(tmpctx, struct pubkey, &c->id)); - goto fail; - } + if (!fromwire_hsm_sign_remote_htlc_tx(tmpctx, msg_in, + &tx, &wscript, &amount, + &remote_per_commit_point)) + return bad_req(conn, c, msg_in); get_channel_seed(&c->id, c->dbid, &channel_seed); derive_basepoints(&channel_seed, NULL, &basepoints, &secrets, NULL); @@ -856,40 +812,27 @@ static struct io_plan *handle_sign_remote_htlc_tx(struct io_conn *conn, if (!derive_simple_privkey(&secrets.htlc_basepoint_secret, &basepoints.htlc, &remote_per_commit_point, - &htlc_privkey)) { - status_broken("Failed deriving htlc privkey for client %s", - type_to_string(tmpctx, struct pubkey, &c->id)); - goto fail; - } + &htlc_privkey)) + return bad_req_fmt(conn, c, msg_in, + "Failed deriving htlc privkey"); if (!derive_simple_key(&basepoints.htlc, &remote_per_commit_point, - &htlc_pubkey)) { - status_broken("Failed deriving htlc pubkey for client %s", - type_to_string(tmpctx, struct pubkey, &c->id)); - goto fail; - } + &htlc_pubkey)) + return bad_req_fmt(conn, c, msg_in, + "Failed deriving htlc pubkey"); /* Need input amount for signing */ tx->input[0].amount = tal_dup(tx->input, u64, &amount); - sign_tx_input(tx, 0, NULL, wscript, &htlc_privkey, &htlc_pubkey, - &sig); - - daemon_conn_send(dc, take(towire_hsm_sign_tx_reply(NULL, &sig))); - return daemon_conn_read_next(conn, dc); + sign_tx_input(tx, 0, NULL, wscript, &htlc_privkey, &htlc_pubkey, &sig); -fail: - daemon_conn_send(c->master, - take(towire_hsmstatus_client_bad_request(NULL, - &c->id, - c->dc.msg_in))); - return io_close(conn); + return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } static struct io_plan *handle_sign_mutual_close_tx(struct io_conn *conn, - struct client *c) + struct client *c, + const u8 *msg_in) { - struct daemon_conn *dc = &c->dc; struct secret channel_seed; struct bitcoin_tx *tx; struct pubkey remote_funding_pubkey, local_funding_pubkey; @@ -898,14 +841,11 @@ static struct io_plan *handle_sign_mutual_close_tx(struct io_conn *conn, u64 funding_amount; const u8 *funding_wscript; - if (!fromwire_hsm_sign_mutual_close_tx(tmpctx, dc->msg_in, + if (!fromwire_hsm_sign_mutual_close_tx(tmpctx, msg_in, &tx, &remote_funding_pubkey, - &funding_amount)) { - status_broken("bad hsm_sign_htlc_mutual_close_tx for client %s", - type_to_string(tmpctx, struct pubkey, &c->id)); - goto fail; - } + &funding_amount)) + return bad_req(conn, c, msg_in); /* FIXME: We should know dust level, decent fee range and * balances, and final_keyindex, and thus be able to check tx @@ -924,36 +864,32 @@ static struct io_plan *handle_sign_mutual_close_tx(struct io_conn *conn, &local_funding_pubkey, &sig); - daemon_conn_send(dc, take(towire_hsm_sign_tx_reply(NULL, &sig))); - return daemon_conn_read_next(conn, dc); - -fail: - daemon_conn_send(c->master, - take(towire_hsmstatus_client_bad_request(NULL, - &c->id, - dc->msg_in))); - return io_close(conn); + return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } -static void pass_client_hsmfd(struct daemon_conn *master, const u8 *msg) +static struct io_plan *pass_client_hsmfd(struct io_conn *conn, + struct client *c, + const u8 *msg_in) { int fds[2]; u64 dbid, capabilities; struct pubkey id; - if (!fromwire_hsm_client_hsmfd(msg, &id, &dbid, &capabilities)) - master_badmsg(WIRE_HSM_CLIENT_HSMFD, msg); + /* This must be the master. */ + assert(&c->dc == c->master); + + if (!fromwire_hsm_client_hsmfd(msg_in, &id, &dbid, &capabilities)) + return bad_req(conn, c, msg_in); if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) != 0) status_failed(STATUS_FAIL_INTERNAL_ERROR, "creating fds: %s", strerror(errno)); - new_client(master, &id, dbid, capabilities, fds[0]); - daemon_conn_send(master, - take(towire_hsm_client_hsmfd_reply(NULL))); - daemon_conn_send_fd(master, fds[1]); + new_client(&c->dc, &id, dbid, capabilities, fds[0]); + daemon_conn_send(&c->dc, take(towire_hsm_client_hsmfd_reply(NULL))); + daemon_conn_send_fd(&c->dc, fds[1]); + return daemon_conn_read_next(conn, &c->dc); } - static void hsm_unilateral_close_privkey(struct privkey *dst, struct unilateral_close_info *info) { @@ -993,7 +929,9 @@ static void hsm_key_for_utxo(struct privkey *privkey, struct pubkey *pubkey, /* Note that it's the main daemon that asks for the funding signature so it * can broadcast it. */ -static void sign_funding_tx(struct daemon_conn *master, const u8 *msg) +static struct io_plan *handle_sign_funding_tx(struct io_conn *conn, + struct client *c, + const u8 *msg_in) { u64 satoshi_out, change_out; u32 change_keyindex; @@ -1006,11 +944,11 @@ static void sign_funding_tx(struct daemon_conn *master, const u8 *msg) u8 **scriptSigs; /* FIXME: Check fee is "reasonable" */ - if (!fromwire_hsm_sign_funding(tmpctx, msg, + if (!fromwire_hsm_sign_funding(tmpctx, msg_in, &satoshi_out, &change_out, &change_keyindex, &local_pubkey, &remote_pubkey, &utxomap)) - master_badmsg(WIRE_HSM_SIGN_FUNDING, msg); + return bad_req(conn, c, msg_in); if (change_out) { changekey = tal(tmpctx, struct pubkey); @@ -1055,14 +993,15 @@ static void sign_funding_tx(struct daemon_conn *master, const u8 *msg) for (size_t i=0; iinput[i].script = scriptSigs[i]; - daemon_conn_send(master, - take(towire_hsm_sign_funding_reply(NULL, tx))); + return req_reply(conn, c, take(towire_hsm_sign_funding_reply(NULL, tx))); } /** * sign_withdrawal_tx - Generate and sign a withdrawal transaction from the master */ -static void sign_withdrawal_tx(struct daemon_conn *master, const u8 *msg) +static struct io_plan *handle_sign_withdrawal_tx(struct io_conn *conn, + struct client *c, + const u8 *msg_in) { u64 satoshi_out, change_out; u32 change_keyindex; @@ -1073,20 +1012,15 @@ static void sign_withdrawal_tx(struct daemon_conn *master, const u8 *msg) struct pubkey changekey; u8 *scriptpubkey; - if (!fromwire_hsm_sign_withdrawal(tmpctx, msg, &satoshi_out, + if (!fromwire_hsm_sign_withdrawal(tmpctx, msg_in, &satoshi_out, &change_out, &change_keyindex, - &scriptpubkey, &utxos)) { - status_broken("Failed to parse sign_withdrawal: %s", - tal_hex(tmpctx, msg)); - return; - } + &scriptpubkey, &utxos)) + return bad_req(conn, c, msg_in); if (bip32_key_from_parent(&secretstuff.bip32, change_keyindex, - BIP32_FLAG_KEY_PUBLIC, &ext) != WALLY_OK) { - status_broken("Failed to parse sign_withdrawal: %s", - tal_hex(tmpctx, msg)); - return; - } + BIP32_FLAG_KEY_PUBLIC, &ext) != WALLY_OK) + return bad_req_fmt(conn, c, msg_in, + "Failed to get key %u", change_keyindex); pubkey_from_der(ext.pub_key, sizeof(ext.pub_key), &changekey); tx = withdraw_tx( @@ -1125,14 +1059,16 @@ static void sign_withdrawal_tx(struct daemon_conn *master, const u8 *msg) for (size_t i=0; iinput[i].script = scriptSigs[i]; - daemon_conn_send(master, + return req_reply(conn, c, take(towire_hsm_sign_withdrawal_reply(NULL, tx))); } /** * sign_invoice - Sign an invoice with our key. */ -static void sign_invoice(struct daemon_conn *master, const u8 *msg) +static struct io_plan *handle_sign_invoice(struct io_conn *conn, + struct client *c, + const u8 *msg_in) { u5 *u5bytes; u8 *hrpu8; @@ -1142,11 +1078,8 @@ static void sign_invoice(struct daemon_conn *master, const u8 *msg) struct hash_u5 hu5; struct privkey node_pkey; - if (!fromwire_hsm_sign_invoice(tmpctx, msg, &u5bytes, &hrpu8)) { - status_broken("Failed to parse sign_invoice: %s", - tal_hex(tmpctx, msg)); - return; - } + if (!fromwire_hsm_sign_invoice(tmpctx, msg_in, &u5bytes, &hrpu8)) + return bad_req(conn, c, msg_in); /* FIXME: Check invoice! */ @@ -1162,16 +1095,16 @@ static void sign_invoice(struct daemon_conn *master, const u8 *msg) (const u8 *)&sha, node_pkey.secret.data, NULL, NULL)) { - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Failed to sign invoice: %s", - tal_hex(tmpctx, msg)); + return bad_req_fmt(conn, c, msg_in, "Failed to sign invoice"); } - daemon_conn_send(master, + return req_reply(conn, c, take(towire_hsm_sign_invoice_reply(NULL, &rsig))); } -static void sign_node_announcement(struct daemon_conn *master, const u8 *msg) +static struct io_plan *handle_sign_node_announcement(struct io_conn *conn, + struct client *c, + const u8 *msg_in) { /* 2 bytes msg type + 64 bytes signature */ size_t offset = 66; @@ -1181,17 +1114,12 @@ static void sign_node_announcement(struct daemon_conn *master, const u8 *msg) u8 *reply; u8 *ann; - if (!fromwire_hsm_node_announcement_sig_req(msg, msg, &ann)) { - status_failed(STATUS_FAIL_GOSSIP_IO, - "Failed to parse node_announcement_sig_req: %s", - tal_hex(tmpctx, msg)); - } + if (!fromwire_hsm_node_announcement_sig_req(tmpctx, msg_in, &ann)) + return bad_req(conn, c, msg_in); - if (tal_count(ann) < offset) { - status_failed(STATUS_FAIL_GOSSIP_IO, - "Node announcement too short: %s", - tal_hex(tmpctx, msg)); - } + if (tal_count(ann) < offset) + return bad_req_fmt(conn, c, msg_in, + "Node announcement too short"); /* FIXME(cdecker) Check the node announcement's content */ node_key(&node_pkey, NULL); @@ -1200,7 +1128,7 @@ static void sign_node_announcement(struct daemon_conn *master, const u8 *msg) sign_hash(&node_pkey, &hash, &sig); reply = towire_hsm_node_announcement_sig_reply(NULL, &sig); - daemon_conn_send(master, take(reply)); + return req_reply(conn, c, take(reply)); } static bool check_client_capabilities(struct client *client, @@ -1272,81 +1200,71 @@ static struct io_plan *handle_client(struct io_conn *conn, /* Before we do anything else, is this client allowed to do * what he asks for? */ - if (!check_client_capabilities(c, t)) { - status_broken("Client does not have the required capability to run %d", t); - daemon_conn_send(c->master, - take(towire_hsmstatus_client_bad_request( - NULL, &c->id, dc->msg_in))); - return io_close(conn); - } + if (!check_client_capabilities(c, t)) + return bad_req_fmt(conn, c, dc->msg_in, + "does not have capability to run %d", t); /* Now actually go and do what the client asked for */ switch (t) { case WIRE_HSM_INIT: - init_hsm(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); + return init_hsm(conn, c, dc->msg_in); case WIRE_HSM_CLIENT_HSMFD: - pass_client_hsmfd(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); + return pass_client_hsmfd(conn, c, dc->msg_in); case WIRE_HSM_GET_CHANNEL_BASEPOINTS: - return handle_get_channel_basepoints(conn, dc); + return handle_get_channel_basepoints(conn, c, dc->msg_in); case WIRE_HSM_ECDH_REQ: - return handle_ecdh(conn, dc); + return handle_ecdh(conn, c, dc->msg_in); case WIRE_HSM_CANNOUNCEMENT_SIG_REQ: - return handle_cannouncement_sig(conn, c); + return handle_cannouncement_sig(conn, c, dc->msg_in); case WIRE_HSM_CUPDATE_SIG_REQ: - return handle_channel_update_sig(conn, dc); + return handle_channel_update_sig(conn, c, dc->msg_in); case WIRE_HSM_SIGN_FUNDING: - sign_funding_tx(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); + return handle_sign_funding_tx(conn, c, dc->msg_in); case WIRE_HSM_NODE_ANNOUNCEMENT_SIG_REQ: - sign_node_announcement(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); + return handle_sign_node_announcement(conn, c, dc->msg_in); case WIRE_HSM_SIGN_INVOICE: - sign_invoice(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); + return handle_sign_invoice(conn, c, dc->msg_in); case WIRE_HSM_SIGN_WITHDRAWAL: - sign_withdrawal_tx(dc, dc->msg_in); - return daemon_conn_read_next(conn, dc); + return handle_sign_withdrawal_tx(conn, c, dc->msg_in); case WIRE_HSM_SIGN_COMMITMENT_TX: - return handle_sign_commitment_tx(conn, dc); + return handle_sign_commitment_tx(conn, c, dc->msg_in); case WIRE_HSM_SIGN_DELAYED_PAYMENT_TO_US: - return handle_sign_delayed_payment_to_us(conn, c); + return handle_sign_delayed_payment_to_us(conn, c, dc->msg_in); case WIRE_HSM_SIGN_REMOTE_HTLC_TO_US: - return handle_sign_remote_htlc_to_us(conn, c); + return handle_sign_remote_htlc_to_us(conn, c, dc->msg_in); case WIRE_HSM_SIGN_PENALTY_TO_US: - return handle_sign_penalty_to_us(conn, c); + return handle_sign_penalty_to_us(conn, c, dc->msg_in); case WIRE_HSM_SIGN_LOCAL_HTLC_TX: - return handle_sign_local_htlc_tx(conn, c); + return handle_sign_local_htlc_tx(conn, c, dc->msg_in); case WIRE_HSM_GET_PER_COMMITMENT_POINT: - return handle_get_per_commitment_point(conn, c); + return handle_get_per_commitment_point(conn, c, dc->msg_in); case WIRE_HSM_CHECK_FUTURE_SECRET: - return handle_check_future_secret(conn, c); + return handle_check_future_secret(conn, c, dc->msg_in); case WIRE_HSM_SIGN_REMOTE_COMMITMENT_TX: - return handle_sign_remote_commitment_tx(conn, c); + return handle_sign_remote_commitment_tx(conn, c, dc->msg_in); case WIRE_HSM_SIGN_REMOTE_HTLC_TX: - return handle_sign_remote_htlc_tx(conn, c); + return handle_sign_remote_htlc_tx(conn, c, dc->msg_in); case WIRE_HSM_SIGN_MUTUAL_CLOSE_TX: - return handle_sign_mutual_close_tx(conn, c); + return handle_sign_mutual_close_tx(conn, c, dc->msg_in); case WIRE_HSM_ECDH_RESP: case WIRE_HSM_CANNOUNCEMENT_SIG_REPLY: @@ -1366,11 +1284,7 @@ static struct io_plan *handle_client(struct io_conn *conn, break; } - daemon_conn_send(c->master, - take(towire_hsmstatus_client_bad_request(NULL, - &c->id, - dc->msg_in))); - return io_close(conn); + return bad_req_fmt(conn, c, dc->msg_in, "Unknown request"); } static void destroy_client(struct client *c) @@ -1433,13 +1347,13 @@ static void master_gone(struct io_conn *unused UNUSED, struct daemon_conn *dc UN int main(int argc, char *argv[]) { struct client *master; - struct daemon_conn *status_conn = tal(NULL, struct daemon_conn); setup_locale(); subdaemon_setup(argc, argv); /* A trivial daemon_conn just for writing. */ + status_conn = tal(NULL, struct daemon_conn); daemon_conn_init(status_conn, status_conn, STDIN_FILENO, (void *)io_never, NULL); status_setup_async(status_conn); diff --git a/lightningd/hsm_control.c b/lightningd/hsm_control.c index b0c313d54d87..96b1f112a6ab 100644 --- a/lightningd/hsm_control.c +++ b/lightningd/hsm_control.c @@ -48,15 +48,16 @@ static unsigned int hsm_msg(struct subd *hsmd, /* We only expect one thing from the HSM that's not a STATUS message */ struct pubkey client_id; u8 *bad_msg; + char *desc; if (!fromwire_hsmstatus_client_bad_request(tmpctx, msg, &client_id, - &bad_msg)) + &desc, &bad_msg)) fatal("Bad status message from hsmd: %s", tal_hex(tmpctx, msg)); /* This should, of course, never happen. */ - log_broken(hsmd->log, "client %s sent bad hsm request %s", + log_broken(hsmd->log, "client %s %s (request %s)", type_to_string(tmpctx, struct pubkey, &client_id), - tal_hex(tmpctx, bad_msg)); + desc, tal_hex(tmpctx, bad_msg)); return 0; } From f8df069536a9320369bbdf7b61bc919e267d8c20 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 12:36:42 +0930 Subject: [PATCH 12/20] hsmd: move HTLC TX signing next to commitment TX signing. Signed-off-by: Rusty Russell --- hsmd/hsmd.c | 86 ++++++++++++++++++++++++++--------------------------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 0c1b2b4e0b0e..9acb0edb521c 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -525,6 +525,49 @@ static struct io_plan *handle_sign_remote_commitment_tx(struct io_conn *conn, return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } +static struct io_plan *handle_sign_remote_htlc_tx(struct io_conn *conn, + struct client *c, + const u8 *msg_in) +{ + struct secret channel_seed; + struct bitcoin_tx *tx; + secp256k1_ecdsa_signature sig; + struct secrets secrets; + struct basepoints basepoints; + struct pubkey remote_per_commit_point; + u64 amount; + u8 *wscript; + struct privkey htlc_privkey; + struct pubkey htlc_pubkey; + + if (!fromwire_hsm_sign_remote_htlc_tx(tmpctx, msg_in, + &tx, &wscript, &amount, + &remote_per_commit_point)) + return bad_req(conn, c, msg_in); + + get_channel_seed(&c->id, c->dbid, &channel_seed); + derive_basepoints(&channel_seed, NULL, &basepoints, &secrets, NULL); + + if (!derive_simple_privkey(&secrets.htlc_basepoint_secret, + &basepoints.htlc, + &remote_per_commit_point, + &htlc_privkey)) + return bad_req_fmt(conn, c, msg_in, + "Failed deriving htlc privkey"); + + if (!derive_simple_key(&basepoints.htlc, + &remote_per_commit_point, + &htlc_pubkey)) + return bad_req_fmt(conn, c, msg_in, + "Failed deriving htlc pubkey"); + + /* Need input amount for signing */ + tx->input[0].amount = tal_dup(tx->input, u64, &amount); + sign_tx_input(tx, 0, NULL, wscript, &htlc_privkey, &htlc_pubkey, &sig); + + return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); +} + /* FIXME: Derive output address for this client, and check it here! */ static struct io_plan *handle_sign_to_us_tx(struct io_conn *conn, struct client *c, @@ -786,49 +829,6 @@ static struct io_plan *handle_check_future_secret(struct io_conn *conn, secret_eq_consttime(&secret, &suggested)))); } -static struct io_plan *handle_sign_remote_htlc_tx(struct io_conn *conn, - struct client *c, - const u8 *msg_in) -{ - struct secret channel_seed; - struct bitcoin_tx *tx; - secp256k1_ecdsa_signature sig; - struct secrets secrets; - struct basepoints basepoints; - struct pubkey remote_per_commit_point; - u64 amount; - u8 *wscript; - struct privkey htlc_privkey; - struct pubkey htlc_pubkey; - - if (!fromwire_hsm_sign_remote_htlc_tx(tmpctx, msg_in, - &tx, &wscript, &amount, - &remote_per_commit_point)) - return bad_req(conn, c, msg_in); - - get_channel_seed(&c->id, c->dbid, &channel_seed); - derive_basepoints(&channel_seed, NULL, &basepoints, &secrets, NULL); - - if (!derive_simple_privkey(&secrets.htlc_basepoint_secret, - &basepoints.htlc, - &remote_per_commit_point, - &htlc_privkey)) - return bad_req_fmt(conn, c, msg_in, - "Failed deriving htlc privkey"); - - if (!derive_simple_key(&basepoints.htlc, - &remote_per_commit_point, - &htlc_pubkey)) - return bad_req_fmt(conn, c, msg_in, - "Failed deriving htlc pubkey"); - - /* Need input amount for signing */ - tx->input[0].amount = tal_dup(tx->input, u64, &amount); - sign_tx_input(tx, 0, NULL, wscript, &htlc_privkey, &htlc_pubkey, &sig); - - return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); -} - static struct io_plan *handle_sign_mutual_close_tx(struct io_conn *conn, struct client *c, const u8 *msg_in) From 8f1f1784b3d5b9da50e4c494d2040f4062b629d2 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 12:36:42 +0930 Subject: [PATCH 13/20] hsmd: remove hsmd/client.c It was only used by handshake.c. Signed-off-by: Rusty Russell --- channeld/Makefile | 1 + closingd/Makefile | 1 + connectd/Makefile | 1 - connectd/connectd.c | 17 +++++++++++++++-- connectd/handshake.c | 1 - connectd/handshake.h | 2 ++ gossipd/Makefile | 1 - gossipd/gossipd.c | 2 -- hsmd/Makefile | 28 +++++++++------------------- hsmd/client.c | 29 ----------------------------- hsmd/client.h | 17 ----------------- hsmd/hsmd.c | 1 - lightningd/Makefile | 5 +++-- onchaind/Makefile | 3 ++- openingd/Makefile | 1 + 15 files changed, 34 insertions(+), 76 deletions(-) delete mode 100644 hsmd/client.c delete mode 100644 hsmd/client.h diff --git a/channeld/Makefile b/channeld/Makefile index 1c15d2ef8040..d68f6a18fa6e 100644 --- a/channeld/Makefile +++ b/channeld/Makefile @@ -72,6 +72,7 @@ CHANNELD_COMMON_OBJS := \ common/wire_error.o \ common/wireaddr.o \ gossipd/gen_gossip_wire.o \ + hsmd/gen_hsm_client_wire.o \ lightningd/gossip_msg.o \ wire/fromwire.o \ wire/towire.o diff --git a/closingd/Makefile b/closingd/Makefile index 0eedf7af8ade..32279bbd46cb 100644 --- a/closingd/Makefile +++ b/closingd/Makefile @@ -72,6 +72,7 @@ CLOSINGD_COMMON_OBJS := \ common/wire_error.o \ common/wireaddr.o \ gossipd/gen_gossip_wire.o \ + hsmd/gen_hsm_client_wire.o \ lightningd/gossip_msg.o closingd/gen_closing_wire.h: $(WIRE_GEN) closingd/closing_wire.csv diff --git a/connectd/Makefile b/connectd/Makefile index f32dc7bce597..d3eacb9bb80e 100644 --- a/connectd/Makefile +++ b/connectd/Makefile @@ -61,7 +61,6 @@ CONNECTD_COMMON_OBJS := \ common/wireaddr.o \ common/wire_error.o \ gossipd/gen_gossip_wire.o \ - hsmd/client.o \ hsmd/gen_hsm_client_wire.o \ lightningd/gossip_msg.o \ wire/gen_onion_wire.o diff --git a/connectd/connectd.c b/connectd/connectd.c index 192dae73a17a..fc97bab8b85a 100644 --- a/connectd/connectd.c +++ b/connectd/connectd.c @@ -39,7 +39,6 @@ #include #include #include -#include #include #include #include @@ -1286,6 +1285,21 @@ static struct io_plan *recv_req(struct io_conn *conn, struct daemon_conn *master t, tal_hex(tmpctx, master->msg_in)); } +/* Helper for handshake.c */ +bool hsm_do_ecdh(struct secret *ss, const struct pubkey *point) +{ + u8 *req = towire_hsm_ecdh_req(tmpctx, point), *resp; + + if (!wire_sync_write(HSM_FD, req)) + return false; + resp = wire_sync_read(req, HSM_FD); + if (!resp) + return false; + if (!fromwire_hsm_ecdh_resp(resp, ss)) + return false; + return true; +} + #ifndef TESTING static void master_gone(struct io_conn *unused UNUSED, struct daemon_conn *dc UNUSED) { @@ -1312,7 +1326,6 @@ int main(int argc, char *argv[]) daemon_conn_init(daemon, &daemon->master, STDIN_FILENO, recv_req, master_gone); status_setup_async(&daemon->master); - hsm_setup(HSM_FD); /* When conn closes, everything is freed. */ tal_steal(daemon->master.conn, daemon); diff --git a/connectd/handshake.c b/connectd/handshake.c index 296e0a4faf08..d861e2424c05 100644 --- a/connectd/handshake.c +++ b/connectd/handshake.c @@ -13,7 +13,6 @@ #include #include #include -#include #include #include #include diff --git a/connectd/handshake.h b/connectd/handshake.h index 2e501dd8f381..20c51ecb9b44 100644 --- a/connectd/handshake.h +++ b/connectd/handshake.h @@ -51,4 +51,6 @@ struct io_plan *responder_handshake_(struct io_conn *conn, void *cbarg), void *cbarg); +/* helper which is defined in connect.c */ +bool hsm_do_ecdh(struct secret *ss, const struct pubkey *point); #endif /* LIGHTNING_CONNECTD_HANDSHAKE_H */ diff --git a/gossipd/Makefile b/gossipd/Makefile index 264fe33aee93..cdb1e122cedb 100644 --- a/gossipd/Makefile +++ b/gossipd/Makefile @@ -61,7 +61,6 @@ GOSSIPD_COMMON_OBJS := \ common/wireaddr.o \ common/wire_error.o \ connectd/gen_connect_gossip_wire.o \ - hsmd/client.o \ hsmd/gen_hsm_client_wire.o \ lightningd/gossip_msg.o \ wire/gen_onion_wire.o diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index 45c00be6a4c9..a05025303a44 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -36,7 +36,6 @@ #include #include #include -#include #include #include #include @@ -2258,7 +2257,6 @@ int main(int argc, char *argv[]) daemon_conn_init(daemon, &daemon->master, STDIN_FILENO, recv_req, master_gone); status_setup_async(&daemon->master); - hsm_setup(HSM_FD); daemon_conn_init(daemon, &daemon->connectd, CONNECTD_FD, connectd_req, NULL); diff --git a/hsmd/Makefile b/hsmd/Makefile index 5147f21f3dc4..dbc5a17c95aa 100644 --- a/hsmd/Makefile +++ b/hsmd/Makefile @@ -6,16 +6,9 @@ hsmd-wrongdir: default: hsmd-all -# Clients use this: -LIGHTNINGD_HSM_CLIENT_HEADERS := hsmd/client.h -LIGHTNINGD_HSM_CLIENT_SRC := hsmd/client.c hsmd/gen_hsm_client_wire.c -LIGHTNINGD_HSM_CLIENT_OBJS := $(LIGHTNINGD_HSM_CLIENT_SRC:.c=.o) - -# lightningd/hsm needs these: -LIGHTNINGD_HSM_HEADERS := hsmd/gen_hsm_client_wire.h - LIGHTNINGD_HSM_SRC := hsmd/hsmd.c \ - $(LIGHTNINGD_HSM_HEADERS:.h=.c) + hsmd/gen_hsm_client_wire.c +LIGHTNINGD_HSM_HEADERS := hsmd/gen_hsm_client_wire.h LIGHTNINGD_HSM_OBJS := $(LIGHTNINGD_HSM_SRC:.c=.o) # Common source we use. @@ -40,20 +33,17 @@ HSMD_COMMON_OBJS := \ common/withdraw_tx.o # For checking -LIGHTNINGD_HSM_ALLSRC_NOGEN := $(filter-out hsmd/gen_%, $(LIGHTNINGD_HSM_CLIENT_SRC) $(LIGHTNINGD_HSM_SRC)) -LIGHTNINGD_HSM_ALLHEADERS_NOGEN := $(filter-out hsmd/gen_%, $(LIGHTNINGD_HSM_CLIENT_HEADERS) $(LIGHTNINGD_HSM_HEADERS)) - -# Add to headers which any object might need. -LIGHTNINGD_HEADERS_GEN += $(LIGHTNINGD_HSM_HEADERS) $(LIGHTNINGD_HSM_CLIENT_HEADERS) +LIGHTNINGD_HSM_ALLSRC_NOGEN := $(filter-out hsmd/gen_%, $(LIGHTNINGD_HSM_SRC) $(LIGHTNINGD_HSM_SRC)) +LIGHTNINGD_HSM_ALLHEADERS_NOGEN := $(filter-out hsmd/gen_%, $(LIGHTNINGD_HSM_HEADERS)) -$(LIGHTNINGD_HSM_OBJS) $(LIGHTNINGD_HSM_CLIENT_OBJS): $(LIGHTNINGD_HEADERS) +$(LIGHTNINGD_HSM_OBJS): $(LIGHTNINGD_HEADERS) # Make sure these depend on everything. -ALL_OBJS += $(LIGHTNINGD_HSM_OBJS) $(LIGHTNINGD_HSM_CLIENT_OBJS) +ALL_OBJS += $(LIGHTNINGD_HSM_OBJS) ALL_PROGRAMS += lightningd/lightning_hsmd -ALL_GEN_HEADERS += $(LIGHTNINGD_HSM_HEADERS) +ALL_GEN_HEADERS += hsmd/gen_hsm_client_wire.h -hsmd-all: lightningd/lightning_hsmd $(LIGHTNINGD_HSM_CLIENT_OBJS) +hsmd-all: lightningd/lightning_hsmd lightningd/lightning_hsmd: $(LIGHTNINGD_HSM_OBJS) $(LIGHTNINGD_LIB_OBJS) $(HSMD_COMMON_OBJS) $(BITCOIN_OBJS) $(WIRE_OBJS) @@ -70,7 +60,7 @@ hsmd/gen_hsm_wire.c: $(WIRE_GEN) hsmd/hsm_wire.csv $(WIRE_GEN) ${@:.c=.h} hsm_wire_type < hsmd/hsm_wire.csv > $@ check-source: $(LIGHTNINGD_HSM_ALLSRC_NOGEN:%=check-src-include-order/%) $(LIGHTNINGD_HSM_ALLHEADERS_NOGEN:%=check-hdr-include-order/%) -check-source-bolt: $(LIGHTNINGD_HSM_SRC:%=bolt-check/%) $(LIGHTNINGD_HSM_HEADERS:%=bolt-check/%) +check-source-bolt: $(LIGHTNINGD_HSM_SRC:%=bolt-check/%) check-whitespace: $(LIGHTNINGD_HSM_ALLSRC_NOGEN:%=check-whitespace/%) $(LIGHTNINGD_HSM_ALLHEADERS_NOGEN:%=check-whitespace/%) diff --git a/hsmd/client.c b/hsmd/client.c deleted file mode 100644 index c631f826fe73..000000000000 --- a/hsmd/client.c +++ /dev/null @@ -1,29 +0,0 @@ -#include -#include -#include - -static int hsm_fd = -1; - -void hsm_setup(int fd) -{ - hsm_fd = fd; -} - -bool hsm_do_ecdh(struct secret *ss, const struct pubkey *point) -{ - u8 *req = towire_hsm_ecdh_req(NULL, point), *resp; - - if (!wire_sync_write(hsm_fd, req)) - goto fail; - resp = wire_sync_read(req, hsm_fd); - if (!resp) - goto fail; - if (!fromwire_hsm_ecdh_resp(resp, ss)) - goto fail; - tal_free(req); - return true; - -fail: - tal_free(req); - return false; -} diff --git a/hsmd/client.h b/hsmd/client.h deleted file mode 100644 index 9adfb28fbf49..000000000000 --- a/hsmd/client.h +++ /dev/null @@ -1,17 +0,0 @@ -/* API to ask the HSM for things. */ -#ifndef LIGHTNING_HSMD_CLIENT_H -#define LIGHTNING_HSMD_CLIENT_H -#include "config.h" -#include -#include -#include - -struct pubkey; -struct secret; - -/* Setup communication to the HSM */ -void hsm_setup(int fd); - -/* Do ECDH using this node id secret. */ -bool hsm_do_ecdh(struct secret *ss, const struct pubkey *point); -#endif /* LIGHTNING_HSMD_CLIENT_H */ diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 9acb0edb521c..3b187536da3f 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -31,7 +31,6 @@ #include #include #include -#include #include #include #include diff --git a/lightningd/Makefile b/lightningd/Makefile index 682fe33a724f..9508dfe62ce2 100644 --- a/lightningd/Makefile +++ b/lightningd/Makefile @@ -48,7 +48,8 @@ LIGHTNINGD_COMMON_OBJS := \ common/wallet_tx.o \ common/wire_error.o \ common/wireaddr.o \ - common/withdraw_tx.o + common/withdraw_tx.o \ + hsmd/gen_hsm_client_wire.o LIGHTNINGD_SRC := \ lightningd/bitcoind.c \ @@ -120,7 +121,7 @@ check-makefile: check-lightningd-makefile check-lightningd-makefile: @for f in lightningd/*.h lightningd/*/*.h; do if ! echo $(LIGHTNINGD_HEADERS_NOGEN) $(LIGHTNINGD_HEADERS_GEN) "" | grep -q "$$f "; then echo $$f not mentioned in LIGHTNINGD_HEADERS_NOGEN or LIGHTNINGD_HEADERS_GEN >&2; exit 1; fi; done -lightningd/lightningd: $(LIGHTNINGD_OBJS) $(LIGHTNINGD_COMMON_OBJS) $(BITCOIN_OBJS) $(WIRE_OBJS) $(WIRE_ONION_OBJS) $(LIGHTNINGD_HSM_CLIENT_OBJS) $(LIGHTNINGD_HANDSHAKE_CONTROL_OBJS) $(LIGHTNINGD_GOSSIP_CONTROL_OBJS) $(LIGHTNINGD_OPENING_CONTROL_OBJS) $(LIGHTNINGD_CHANNEL_CONTROL_OBJS) $(LIGHTNINGD_CLOSING_CONTROL_OBJS) $(LIGHTNINGD_ONCHAIN_CONTROL_OBJS) $(WALLET_LIB_OBJS) $(LIGHTNINGD_CONNECT_CONTROL_OBJS) +lightningd/lightningd: $(LIGHTNINGD_OBJS) $(LIGHTNINGD_COMMON_OBJS) $(BITCOIN_OBJS) $(WIRE_OBJS) $(WIRE_ONION_OBJS) $(LIGHTNINGD_HANDSHAKE_CONTROL_OBJS) $(LIGHTNINGD_GOSSIP_CONTROL_OBJS) $(LIGHTNINGD_OPENING_CONTROL_OBJS) $(LIGHTNINGD_CHANNEL_CONTROL_OBJS) $(LIGHTNINGD_CLOSING_CONTROL_OBJS) $(LIGHTNINGD_ONCHAIN_CONTROL_OBJS) $(WALLET_LIB_OBJS) $(LIGHTNINGD_CONNECT_CONTROL_OBJS) clean: lightningd-clean diff --git a/onchaind/Makefile b/onchaind/Makefile index 1ea8b7bbcaf5..e2d76695be33 100644 --- a/onchaind/Makefile +++ b/onchaind/Makefile @@ -68,7 +68,8 @@ ONCHAIND_COMMON_OBJS := \ common/type_to_string.o \ common/utils.o \ common/utxo.o \ - common/version.o + common/version.o \ + hsmd/gen_hsm_client_wire.o onchaind/gen_onchain_wire.h: $(WIRE_GEN) onchaind/onchain_wire.csv $(WIRE_GEN) --header $@ onchain_wire_type < onchaind/onchain_wire.csv > $@ diff --git a/openingd/Makefile b/openingd/Makefile index 92fb90a0ac67..33cbea1c040a 100644 --- a/openingd/Makefile +++ b/openingd/Makefile @@ -70,6 +70,7 @@ OPENINGD_COMMON_OBJS := \ common/wire_error.o \ common/wireaddr.o \ gossipd/gen_gossip_wire.o \ + hsmd/gen_hsm_client_wire.o \ lightningd/gossip_msg.o $(LIGHTNINGD_OPENING_OBJS): $(LIGHTNINGD_HEADERS) From e012e94ab2940c4622e44d0e97d958ec6ce708b4 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 12:36:42 +0930 Subject: [PATCH 14/20] hsmd: rename hsm_client_wire_csv to hsm_wire.csv That matches the other CSV names (HSM was the first, so it was written before the pattern emerged). Signed-off-by: Rusty Russell --- channeld/Makefile | 2 +- channeld/channeld.c | 6 +++--- closingd/Makefile | 2 +- closingd/closingd.c | 2 +- common/bolt11.c | 2 +- connectd/Makefile | 2 +- connectd/connectd.c | 2 +- gossipd/Makefile | 2 +- gossipd/gossipd.c | 2 +- hsmd/Makefile | 12 +++--------- hsmd/{hsm_client_wire_csv => hsm_wire.csv} | 0 hsmd/hsmd.c | 6 +++--- lightningd/Makefile | 2 +- lightningd/channel.c | 2 +- lightningd/channel_control.c | 2 +- lightningd/connect_control.c | 2 +- lightningd/gossip_control.c | 2 +- lightningd/hsm_control.c | 4 ++-- lightningd/invoice.c | 2 +- lightningd/onchain_control.c | 2 +- lightningd/opening_control.c | 2 +- lightningd/peer_control.c | 2 +- onchaind/Makefile | 2 +- onchaind/onchaind.c | 2 +- openingd/Makefile | 2 +- openingd/openingd.c | 2 +- wallet/walletrpc.c | 2 +- 27 files changed, 33 insertions(+), 39 deletions(-) rename hsmd/{hsm_client_wire_csv => hsm_wire.csv} (100%) diff --git a/channeld/Makefile b/channeld/Makefile index d68f6a18fa6e..1739a1cf0637 100644 --- a/channeld/Makefile +++ b/channeld/Makefile @@ -72,7 +72,7 @@ CHANNELD_COMMON_OBJS := \ common/wire_error.o \ common/wireaddr.o \ gossipd/gen_gossip_wire.o \ - hsmd/gen_hsm_client_wire.o \ + hsmd/gen_hsm_wire.o \ lightningd/gossip_msg.o \ wire/fromwire.o \ wire/towire.o diff --git a/channeld/channeld.c b/channeld/channeld.c index 028228ac4ba0..3cdd3b180fba 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -45,7 +45,7 @@ #include #include #include -#include +#include #include #include #include @@ -223,14 +223,14 @@ static const u8 *hsm_req(const tal_t *ctx, const u8 *req TAKES) if (!wire_sync_write(HSM_FD, req)) status_failed(STATUS_FAIL_HSM_IO, "Writing %s to HSM: %s", - hsm_client_wire_type_name(type), + hsm_wire_type_name(type), strerror(errno)); msg = wire_sync_read(ctx, HSM_FD); if (!msg) status_failed(STATUS_FAIL_HSM_IO, "Reading resp to %s: %s", - hsm_client_wire_type_name(type), + hsm_wire_type_name(type), strerror(errno)); return msg; diff --git a/closingd/Makefile b/closingd/Makefile index 32279bbd46cb..d3a0ab666d82 100644 --- a/closingd/Makefile +++ b/closingd/Makefile @@ -72,7 +72,7 @@ CLOSINGD_COMMON_OBJS := \ common/wire_error.o \ common/wireaddr.o \ gossipd/gen_gossip_wire.o \ - hsmd/gen_hsm_client_wire.o \ + hsmd/gen_hsm_wire.o \ lightningd/gossip_msg.o closingd/gen_closing_wire.h: $(WIRE_GEN) closingd/closing_wire.csv diff --git a/closingd/closingd.c b/closingd/closingd.c index bef2e9886c60..eb19badae749 100644 --- a/closingd/closingd.c +++ b/closingd/closingd.c @@ -15,7 +15,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/common/bolt11.c b/common/bolt11.c index c3e42be5c1f5..3bb23ab3e07b 100644 --- a/common/bolt11.c +++ b/common/bolt11.c @@ -11,7 +11,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/connectd/Makefile b/connectd/Makefile index d3eacb9bb80e..516863f19ae5 100644 --- a/connectd/Makefile +++ b/connectd/Makefile @@ -61,7 +61,7 @@ CONNECTD_COMMON_OBJS := \ common/wireaddr.o \ common/wire_error.o \ gossipd/gen_gossip_wire.o \ - hsmd/gen_hsm_client_wire.o \ + hsmd/gen_hsm_wire.o \ lightningd/gossip_msg.o \ wire/gen_onion_wire.o diff --git a/connectd/connectd.c b/connectd/connectd.c index fc97bab8b85a..a6c6c46bd8ac 100644 --- a/connectd/connectd.c +++ b/connectd/connectd.c @@ -39,7 +39,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/gossipd/Makefile b/gossipd/Makefile index cdb1e122cedb..2855eb0b21db 100644 --- a/gossipd/Makefile +++ b/gossipd/Makefile @@ -61,7 +61,7 @@ GOSSIPD_COMMON_OBJS := \ common/wireaddr.o \ common/wire_error.o \ connectd/gen_connect_gossip_wire.o \ - hsmd/gen_hsm_client_wire.o \ + hsmd/gen_hsm_wire.o \ lightningd/gossip_msg.o \ wire/gen_onion_wire.o diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index a05025303a44..7d1d6247d70a 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -36,7 +36,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/hsmd/Makefile b/hsmd/Makefile index dbc5a17c95aa..ad3eff3b850a 100644 --- a/hsmd/Makefile +++ b/hsmd/Makefile @@ -7,8 +7,8 @@ hsmd-wrongdir: default: hsmd-all LIGHTNINGD_HSM_SRC := hsmd/hsmd.c \ - hsmd/gen_hsm_client_wire.c -LIGHTNINGD_HSM_HEADERS := hsmd/gen_hsm_client_wire.h + hsmd/gen_hsm_wire.c +LIGHTNINGD_HSM_HEADERS := hsmd/gen_hsm_wire.h LIGHTNINGD_HSM_OBJS := $(LIGHTNINGD_HSM_SRC:.c=.o) # Common source we use. @@ -41,18 +41,12 @@ $(LIGHTNINGD_HSM_OBJS): $(LIGHTNINGD_HEADERS) # Make sure these depend on everything. ALL_OBJS += $(LIGHTNINGD_HSM_OBJS) ALL_PROGRAMS += lightningd/lightning_hsmd -ALL_GEN_HEADERS += hsmd/gen_hsm_client_wire.h +ALL_GEN_HEADERS += hsmd/gen_hsm_wire.h hsmd-all: lightningd/lightning_hsmd lightningd/lightning_hsmd: $(LIGHTNINGD_HSM_OBJS) $(LIGHTNINGD_LIB_OBJS) $(HSMD_COMMON_OBJS) $(BITCOIN_OBJS) $(WIRE_OBJS) -hsmd/gen_hsm_client_wire.h: $(WIRE_GEN) hsmd/hsm_client_wire_csv - $(WIRE_GEN) --header $@ hsm_client_wire_type < hsmd/hsm_client_wire_csv > $@ - -hsmd/gen_hsm_client_wire.c: $(WIRE_GEN) hsmd/hsm_client_wire_csv - $(WIRE_GEN) ${@:.c=.h} hsm_client_wire_type< hsmd/hsm_client_wire_csv > $@ - hsmd/gen_hsm_wire.h: $(WIRE_GEN) hsmd/hsm_wire.csv $(WIRE_GEN) --header $@ hsm_wire_type < hsmd/hsm_wire.csv > $@ diff --git a/hsmd/hsm_client_wire_csv b/hsmd/hsm_wire.csv similarity index 100% rename from hsmd/hsm_client_wire_csv rename to hsmd/hsm_wire.csv diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 3b187536da3f..8dea14dd215d 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -31,7 +31,7 @@ #include #include #include -#include +#include #include #include #include @@ -1131,7 +1131,7 @@ static struct io_plan *handle_sign_node_announcement(struct io_conn *conn, } static bool check_client_capabilities(struct client *client, - enum hsm_client_wire_type t) + enum hsm_wire_type t) { switch (t) { case WIRE_HSM_ECDH_REQ: @@ -1193,7 +1193,7 @@ static struct io_plan *handle_client(struct io_conn *conn, struct daemon_conn *dc) { struct client *c = container_of(dc, struct client, dc); - enum hsm_client_wire_type t = fromwire_peektype(dc->msg_in); + enum hsm_wire_type t = fromwire_peektype(dc->msg_in); status_debug("Client: Received message %d from client", t); diff --git a/lightningd/Makefile b/lightningd/Makefile index 9508dfe62ce2..eb37128f453c 100644 --- a/lightningd/Makefile +++ b/lightningd/Makefile @@ -49,7 +49,7 @@ LIGHTNINGD_COMMON_OBJS := \ common/wire_error.o \ common/wireaddr.o \ common/withdraw_tx.o \ - hsmd/gen_hsm_client_wire.o + hsmd/gen_hsm_wire.o LIGHTNINGD_SRC := \ lightningd/bitcoind.c \ diff --git a/lightningd/channel.c b/lightningd/channel.c index 0b792d802e6d..a0f6443b21d6 100644 --- a/lightningd/channel.c +++ b/lightningd/channel.c @@ -4,7 +4,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index 6bcb636b5a5b..3f68317af719 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -7,7 +7,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/lightningd/connect_control.c b/lightningd/connect_control.c index 31b5a05a8200..1c6c38489b06 100644 --- a/lightningd/connect_control.c +++ b/lightningd/connect_control.c @@ -11,7 +11,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index 39e91ad4cc1c..88644231c775 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -17,7 +17,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/lightningd/hsm_control.c b/lightningd/hsm_control.c index 96b1f112a6ab..e609de96f63c 100644 --- a/lightningd/hsm_control.c +++ b/lightningd/hsm_control.c @@ -8,7 +8,7 @@ #include #include #include -#include +#include #include #include #include @@ -71,7 +71,7 @@ void hsm_init(struct lightningd *ld) err(1, "Could not create hsm socketpair"); ld->hsm = new_global_subd(ld, "lightning_hsmd", - hsm_client_wire_type_name, + hsm_wire_type_name, hsm_msg, take(&fds[1]), NULL); if (!ld->hsm) diff --git a/lightningd/invoice.c b/lightningd/invoice.c index c5e229347d6a..31d746b1b114 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -13,7 +13,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index 3f29f4b45c81..d8d8ab553c76 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -2,7 +2,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index 8d43890843b8..433d4a3cf712 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -8,7 +8,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 4dc9fa945b97..033c746d413d 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -23,7 +23,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/onchaind/Makefile b/onchaind/Makefile index e2d76695be33..d9146f394d8b 100644 --- a/onchaind/Makefile +++ b/onchaind/Makefile @@ -69,7 +69,7 @@ ONCHAIND_COMMON_OBJS := \ common/utils.o \ common/utxo.o \ common/version.o \ - hsmd/gen_hsm_client_wire.o + hsmd/gen_hsm_wire.o onchaind/gen_onchain_wire.h: $(WIRE_GEN) onchaind/onchain_wire.csv $(WIRE_GEN) --header $@ onchain_wire_type < onchaind/onchain_wire.csv > $@ diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c index ce7801a130a1..af1e3d73354a 100644 --- a/onchaind/onchaind.c +++ b/onchaind/onchaind.c @@ -15,7 +15,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/openingd/Makefile b/openingd/Makefile index 33cbea1c040a..893efb1566d1 100644 --- a/openingd/Makefile +++ b/openingd/Makefile @@ -70,7 +70,7 @@ OPENINGD_COMMON_OBJS := \ common/wire_error.o \ common/wireaddr.o \ gossipd/gen_gossip_wire.o \ - hsmd/gen_hsm_client_wire.o \ + hsmd/gen_hsm_wire.o \ lightningd/gossip_msg.o $(LIGHTNINGD_OPENING_OBJS): $(LIGHTNINGD_HEADERS) diff --git a/openingd/openingd.c b/openingd/openingd.c index dbcca169232a..4478f545d453 100644 --- a/openingd/openingd.c +++ b/openingd/openingd.c @@ -23,7 +23,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index 32bcbc562dc3..2e0ad1c1f5e0 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -9,7 +9,7 @@ #include #include #include -#include +#include #include #include #include From cc48e794c9cdaca2f2631f1c52cdcc30914b3730 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 12:36:42 +0930 Subject: [PATCH 15/20] hsmd: extract and use common sign_all_inputs() helper. Signed-off-by: Rusty Russell --- hsmd/hsmd.c | 112 +++++++++++++++++++--------------------------------- 1 file changed, 41 insertions(+), 71 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 8dea14dd215d..2c637176fbde 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -926,6 +926,39 @@ static void hsm_key_for_utxo(struct privkey *privkey, struct pubkey *pubkey, } } +static void sign_all_inputs(struct bitcoin_tx *tx, struct utxo **utxos) +{ + u8 **scriptSigs = tal_arr(tmpctx, u8 *, tal_count(utxos)); + + assert(tal_count(tx->input) == tal_count(utxos)); + for (size_t i = 0; i < tal_count(utxos); i++) { + struct pubkey inkey; + struct privkey inprivkey; + const struct utxo *in = utxos[i]; + u8 *subscript, *wscript; + secp256k1_ecdsa_signature sig; + + hsm_key_for_utxo(&inprivkey, &inkey, in); + + wscript = p2wpkh_scriptcode(tmpctx, &inkey); + if (in->is_p2sh) { + subscript = bitcoin_redeem_p2sh_p2wpkh(tmpctx, &inkey); + scriptSigs[i] = bitcoin_scriptsig_p2sh_p2wpkh(tx, &inkey); + } else { + subscript = NULL; + scriptSigs[i] = NULL; + } + sign_tx_input(tx, i, subscript, wscript, &inprivkey, &inkey, + &sig); + + tx->input[i].witness = bitcoin_witness_p2wpkh(tx, &sig, &inkey); + } + + /* Now complete the transaction by attaching the scriptSigs where necessary */ + for (size_t i = 0; i < tal_count(utxos); i++) + tx->input[i].script = scriptSigs[i]; +} + /* Note that it's the main daemon that asks for the funding signature so it * can broadcast it. */ static struct io_plan *handle_sign_funding_tx(struct io_conn *conn, @@ -935,18 +968,16 @@ static struct io_plan *handle_sign_funding_tx(struct io_conn *conn, u64 satoshi_out, change_out; u32 change_keyindex; struct pubkey local_pubkey, remote_pubkey; - struct utxo **utxomap; + struct utxo **utxos; struct bitcoin_tx *tx; u16 outnum; - size_t i; struct pubkey *changekey; - u8 **scriptSigs; /* FIXME: Check fee is "reasonable" */ if (!fromwire_hsm_sign_funding(tmpctx, msg_in, &satoshi_out, &change_out, &change_keyindex, &local_pubkey, - &remote_pubkey, &utxomap)) + &remote_pubkey, &utxos)) return bad_req(conn, c, msg_in); if (change_out) { @@ -956,42 +987,12 @@ static struct io_plan *handle_sign_funding_tx(struct io_conn *conn, changekey = NULL; tx = funding_tx(tmpctx, &outnum, - cast_const2(const struct utxo **, utxomap), + cast_const2(const struct utxo **, utxos), satoshi_out, &local_pubkey, &remote_pubkey, change_out, changekey, NULL); - scriptSigs = tal_arr(tmpctx, u8*, tal_count(utxomap)); - for (i = 0; i < tal_count(utxomap); i++) { - struct pubkey inkey; - struct privkey inprivkey; - const struct utxo *in = utxomap[i]; - u8 *subscript; - secp256k1_ecdsa_signature sig; - - hsm_key_for_utxo(&inprivkey, &inkey, in); - - if (in->is_p2sh) - subscript = bitcoin_redeem_p2sh_p2wpkh(tmpctx, &inkey); - else - subscript = NULL; - u8 *wscript = p2wpkh_scriptcode(tmpctx, &inkey); - - sign_tx_input(tx, i, subscript, wscript, &inprivkey, &inkey, - &sig); - - tx->input[i].witness = bitcoin_witness_p2wpkh(tx, &sig, &inkey); - - if (utxomap[i]->is_p2sh) - scriptSigs[i] = bitcoin_scriptsig_p2sh_p2wpkh(tx, &inkey); - else - scriptSigs[i] = NULL; - } - - /* Now complete the transaction by attaching the scriptSigs where necessary */ - for (size_t i=0; iinput[i].script = scriptSigs[i]; - + sign_all_inputs(tx, utxos); return req_reply(conn, c, take(towire_hsm_sign_funding_reply(NULL, tx))); } @@ -1005,7 +1006,6 @@ static struct io_plan *handle_sign_withdrawal_tx(struct io_conn *conn, u64 satoshi_out, change_out; u32 change_keyindex; struct utxo **utxos; - u8 **scriptSigs; struct bitcoin_tx *tx; struct ext_key ext; struct pubkey changekey; @@ -1022,41 +1022,11 @@ static struct io_plan *handle_sign_withdrawal_tx(struct io_conn *conn, "Failed to get key %u", change_keyindex); pubkey_from_der(ext.pub_key, sizeof(ext.pub_key), &changekey); - tx = withdraw_tx( - tmpctx, cast_const2(const struct utxo **, utxos), - scriptpubkey, satoshi_out, - &changekey, change_out, NULL); - - scriptSigs = tal_arr(tmpctx, u8*, tal_count(utxos)); - for (size_t i = 0; i < tal_count(utxos); i++) { - struct pubkey inkey; - struct privkey inprivkey; - const struct utxo *in = utxos[i]; - u8 *subscript; - secp256k1_ecdsa_signature sig; - - hsm_key_for_utxo(&inprivkey, &inkey, in); - - if (in->is_p2sh || in->close_info != NULL) - subscript = bitcoin_redeem_p2sh_p2wpkh(tmpctx, &inkey); - else - subscript = NULL; - u8 *wscript = p2wpkh_scriptcode(tmpctx, &inkey); - - sign_tx_input(tx, i, subscript, wscript, &inprivkey, &inkey, - &sig); + tx = withdraw_tx(tmpctx, cast_const2(const struct utxo **, utxos), + scriptpubkey, satoshi_out, + &changekey, change_out, NULL); - tx->input[i].witness = bitcoin_witness_p2wpkh(tx, &sig, &inkey); - - if (utxos[i]->is_p2sh) - scriptSigs[i] = bitcoin_scriptsig_p2sh_p2wpkh(tx, &inkey); - else - scriptSigs[i] = NULL; - } - - /* Now complete the transaction by attaching the scriptSigs where necessary */ - for (size_t i=0; iinput[i].script = scriptSigs[i]; + sign_all_inputs(tx, utxos); return req_reply(conn, c, take(towire_hsm_sign_withdrawal_reply(NULL, tx))); From 04c77f4853ae8e4ee0e27632ef9b6ebb0e2cd680 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 12:37:41 +0930 Subject: [PATCH 16/20] lightningd: use hsm_get_client_fd() helper for global daemons too. We couldn't use it before because it asserted dbid was non-zero. Remove assert and save some code. Signed-off-by: Rusty Russell Header from folded patch 'fixup!_lightningd__use_hsm_get_client_fd()_helper_for_global_daemons_too.patch': fixup! lightningd: use hsm_get_client_fd() helper for global daemons too. Suggested-by: @ZmnSCPxj --- lightningd/connect_control.c | 14 ++------------ lightningd/gossip_control.c | 13 +------------ lightningd/hsm_control.c | 18 ++++++++++++++++-- lightningd/hsm_control.h | 3 +++ 4 files changed, 22 insertions(+), 26 deletions(-) diff --git a/lightningd/connect_control.c b/lightningd/connect_control.c index 1c6c38489b06..f55b9897fe11 100644 --- a/lightningd/connect_control.c +++ b/lightningd/connect_control.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -333,7 +334,6 @@ int connectd_init(struct lightningd *ld) int fds[2]; u8 *msg; int hsmfd; - u64 capabilities = HSM_CAP_ECDH; struct wireaddr_internal *wireaddrs = ld->proposed_wireaddr; enum addr_listen_announce *listen_announce = ld->proposed_listen_announce; bool allow_localhost = false; @@ -345,17 +345,7 @@ int connectd_init(struct lightningd *ld) if (socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) != 0) fatal("Could not socketpair for connectd<->gossipd"); - msg = towire_hsm_client_hsmfd(tmpctx, &ld->id, 0, capabilities); - if (!wire_sync_write(ld->hsm_fd, msg)) - fatal("Could not write to HSM: %s", strerror(errno)); - - msg = wire_sync_read(tmpctx, ld->hsm_fd); - if (!fromwire_hsm_client_hsmfd_reply(msg)) - fatal("Malformed hsmfd response: %s", tal_hex(msg, msg)); - - hsmfd = fdpass_recv(ld->hsm_fd); - if (hsmfd < 0) - fatal("Could not read fd from HSM: %s", strerror(errno)); + hsmfd = hsm_get_global_fd(ld, HSM_CAP_ECDH); ld->connectd = new_global_subd(ld, "lightning_connectd", connect_wire_type_name, connectd_msg, diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index 88644231c775..5ebf00b7b09a 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -148,19 +148,8 @@ void gossip_init(struct lightningd *ld, int connectd_fd) { u8 *msg; int hsmfd; - u64 capabilities = HSM_CAP_SIGN_GOSSIP; - msg = towire_hsm_client_hsmfd(tmpctx, &ld->id, 0, capabilities); - if (!wire_sync_write(ld->hsm_fd, msg)) - fatal("Could not write to HSM: %s", strerror(errno)); - - msg = wire_sync_read(tmpctx, ld->hsm_fd); - if (!fromwire_hsm_client_hsmfd_reply(msg)) - fatal("Malformed hsmfd response: %s", tal_hex(msg, msg)); - - hsmfd = fdpass_recv(ld->hsm_fd); - if (hsmfd < 0) - fatal("Could not read fd from HSM: %s", strerror(errno)); + hsmfd = hsm_get_global_fd(ld, HSM_CAP_SIGN_GOSSIP); ld->gossip = new_global_subd(ld, "lightning_gossipd", gossip_wire_type_name, gossip_msg, diff --git a/lightningd/hsm_control.c b/lightningd/hsm_control.c index e609de96f63c..030f165ea901 100644 --- a/lightningd/hsm_control.c +++ b/lightningd/hsm_control.c @@ -19,7 +19,7 @@ #include #include -int hsm_get_client_fd(struct lightningd *ld, +static int hsm_get_fd(struct lightningd *ld, const struct pubkey *id, u64 dbid, int capabilities) @@ -27,7 +27,6 @@ int hsm_get_client_fd(struct lightningd *ld, int hsm_fd; u8 *msg; - assert(dbid); msg = towire_hsm_client_hsmfd(NULL, id, dbid, capabilities); if (!wire_sync_write(ld->hsm_fd, take(msg))) fatal("Could not write to HSM: %s", strerror(errno)); @@ -42,6 +41,21 @@ int hsm_get_client_fd(struct lightningd *ld, return hsm_fd; } +int hsm_get_client_fd(struct lightningd *ld, + const struct pubkey *id, + u64 dbid, + int capabilities) +{ + assert(dbid); + + return hsm_get_fd(ld, id, dbid, capabilities); +} + +int hsm_get_global_fd(struct lightningd *ld, int capabilities) +{ + return hsm_get_fd(ld, &ld->id, 0, capabilities); +} + static unsigned int hsm_msg(struct subd *hsmd, const u8 *msg, const int *fds UNUSED) { diff --git a/lightningd/hsm_control.h b/lightningd/hsm_control.h index 2a5ddd9f87d4..5e80a47369df 100644 --- a/lightningd/hsm_control.h +++ b/lightningd/hsm_control.h @@ -15,5 +15,8 @@ int hsm_get_client_fd(struct lightningd *ld, u64 dbid, int capabilities); +/* Ask HSM for an fd for a global subdaemon to use (gossipd, connectd) */ +int hsm_get_global_fd(struct lightningd *ld, int capabilities); + void hsm_init(struct lightningd *ld); #endif /* LIGHTNING_LIGHTNINGD_HSM_CONTROL_H */ From 573f2f065a473eb93cb51680349d668db1031432 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 14:01:09 +0930 Subject: [PATCH 17/20] hsmd: document as part II of our journey. Thanks greatly to the four people who I *know* have read this: @wythe, @ZmnSCPxj, @SimonVrouwe, and @cdecker Your feedback will help future developers seeking enlightenment! Signed-off-by: Rusty Russell --- hsmd/hsmd.c | 425 +++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 391 insertions(+), 34 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 2c637176fbde..21cb1cc97b2d 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -1,3 +1,11 @@ +/*~ Welcome to the hsm daemon: keeper of our secrets! + * + * This is a separate daemon which keeps a root secret from which all others + * are generated. It starts with one client: lightningd, which can ask for + * new sockets for other clients. Each client has a simple capability map + * which indicates what it's allowed to ask for. We're entirely driven + * by request, response. + */ #include #include #include @@ -31,6 +39,7 @@ #include #include #include +/*~ All gen_ files are autogenerated; in this case by tools/generate-wire.py */ #include #include #include @@ -43,35 +52,55 @@ #include #include +/*~ Each subdaemon is started with stdin connected to lightningd (for status + * messages), and stderr untouched (for emergency printing). File descriptors + * 3 and beyond are set up on other sockets: for hsmd, fd 3 is the request + * stream from lightningd. */ #define REQ_FD 3 -/* Nobody will ever find it here! */ +/*~ Nobody will ever find it here! hsm_secret is our root secret, the bip32 + * tree is derived from that, and cached here. */ static struct { struct secret hsm_secret; struct ext_key bip32; } secretstuff; +/*~ We keep track of clients, but there's not much to keep. */ struct client { struct daemon_conn dc; struct daemon_conn *master; + /* ~Useful for logging, but also used to derive the per-channel seed. */ struct pubkey id; + + /* ~This is a unique value handed to us from lightningd, used for + * per-channel seed generation (a single id may have multiple channels + * over time). + * + * It's actually zero for the initial lightningd client connection and + * the ones for gossipd and connectd, which don't have channels + * associated. */ u64 dbid; /* What is this client allowed to ask for? */ u64 capabilities; }; -/* We keep a map of nonzero dbid -> clients */ +/*~ We keep a map of nonzero dbid -> clients, mainly for leak detection. + * This is ccan/uintmap, which maps u64 to some (non-NULL) pointer. + * I really dislike these kinds of declaration-via-magic macro things, as + * tags can't find them without special hacks, but the payoff here is that + * the map is typesafe: the compiler won't let you put anything in but a + * struct client pointer. */ static UINTMAP(struct client *) clients; -/* We get three zero-dbid clients: master, gossipd and connnectd. */ +/*~ Plus the three zero-dbid clients: master, gossipd and connnectd. */ static struct client *dbid_zero_clients[3]; static size_t num_dbid_zero_clients; -/* For reporting issues. */ +/*~ We need this deep inside bad_req_fmt, so we make it a global. */ static struct daemon_conn *status_conn; -/* FIXME: This is used by debug.c, but doesn't apply to us. */ +/*~ FIXME: This is used by debug.c. Doesn't apply to us, but lets us link. */ extern void dev_disconnect_init(int fd); void dev_disconnect_init(int fd UNUSED) { } @@ -82,6 +111,13 @@ static struct client *new_client(struct daemon_conn *master, const u64 capabilities, int fd); +/*~ ccan/compiler.h defines PRINTF_FMT as the gcc compiler hint so it will + * check that fmt and other trailing arguments really are the correct type. + * + * This is a convenient helper to tell lightningd we've received a bad request + * and closes the client connection. This should never happen, of course, but + * we definitely want to log if it does. + */ static PRINTF_FMT(4,5) struct io_plan *bad_req_fmt(struct io_conn *conn, struct client *c, @@ -95,20 +131,32 @@ static PRINTF_FMT(4,5) str = tal_fmt(tmpctx, fmt, ap); va_end(ap); - /* If the client was actually lightningd, it's Game Over. */ + /*~ If the client was actually lightningd, it's Game Over; we actually + * fail in this case, and it will too. */ if (&c->dc == c->master) { status_broken("%s", str); master_badmsg(fromwire_peektype(msg_in), msg_in); } + /*~ Note the use of NULL as the ctx arg to towire_hsmstatus_: only + * use NULL as the allocation when we're about to immediately free it + * or hand it off with take(), as here. That makes it clear we don't + * expect it to linger, and in fact our memleak detection will + * complain if it does (unlike using the deliberately-transient + * tmpctx). */ daemon_conn_send(status_conn, take(towire_hsmstatus_client_bad_request(NULL, &c->id, str, msg_in))); + + /*~ The way ccan/io works is that you return the "plan" for what to do + * next (eg. io_read). io_close() is special: it means to close the + * connection. */ return io_close(conn); } +/* Convenience wrapper for when we simply can't parse. */ static struct io_plan *bad_req(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -116,6 +164,7 @@ static struct io_plan *bad_req(struct io_conn *conn, return bad_req_fmt(conn, c, msg_in, "could not parse request"); } +/* This is the common pattern for the tail of each handler in this file. */ static struct io_plan *req_reply(struct io_conn *conn, struct client *c, const u8 *msg_out TAKES) @@ -124,18 +173,25 @@ static struct io_plan *req_reply(struct io_conn *conn, return daemon_conn_read_next(conn, &c->dc); } +/*~ This returns the secret and/or public key for this node. */ static void node_key(struct privkey *node_privkey, struct pubkey *node_id) { u32 salt = 0; struct privkey unused_s; struct pubkey unused_k; + /* If caller specifies NULL, they don't want the results. */ if (node_privkey == NULL) node_privkey = &unused_s; else if (node_id == NULL) node_id = &unused_k; + /*~ So, there is apparently a 1 in 2^127 chance that a random value is + * not a valid private key, so this never actually loops. */ do { + /*~ ccan/crypto/hkdf_sha256 implements RFC5869 "Hardened Key + * Derivation Functions". That means that if a derived key + * leaks somehow, the other keys are not compromised. */ hkdf_sha256(node_privkey, sizeof(*node_privkey), &salt, sizeof(salt), &secretstuff.hsm_secret, @@ -146,28 +202,40 @@ static void node_key(struct privkey *node_privkey, struct pubkey *node_id) node_privkey->secret.data)); } -/** - * hsm_channel_secret_base -- Derive the base secret seed for per-channel seeds - * - * This secret is the basis for all per-channel secrets: the per-channel seeds - * will be generated mixing in the channel_id and the peer node_id. - */ +/*~ This secret is the basis for all per-channel secrets: the per-channel seeds + * will be generated by mixing in the dbid and the peer node_id. */ static void hsm_channel_secret_base(struct secret *channel_seed_base) { hkdf_sha256(channel_seed_base, sizeof(struct secret), NULL, 0, &secretstuff.hsm_secret, sizeof(secretstuff.hsm_secret), + /*~ Initially, we didn't support multiple channels per + * peer at all: a channel had to be completely forgotten + * before another could exist. That was slightly relaxed, + * but the phrase "peer seed" is wired into the seed + * generation here, so we need to keep it that way for + * existing clients, rather than using "channel seed". */ "peer seed", strlen("peer seed")); } +/*~ This gets the seed for this particular channel. */ static void get_channel_seed(const struct pubkey *peer_id, u64 dbid, struct secret *channel_seed) { struct secret channel_base; u8 input[PUBKEY_DER_LEN + sizeof(dbid)]; + /*~ Again, "per-peer" should be "per-channel", but Hysterical Raisins */ const char *info = "per-peer seed"; + /*~ We use the DER encoding of the pubkey, because it's platform + * independent. Since the dbid is unique, however, it's completely + * unnecessary, but again, existing users can't be broken. */ + /* FIXME: lnd has a nicer BIP32 method for deriving secrets which we + * should migrate to. */ hsm_channel_secret_base(&channel_base); pubkey_to_der(input, peer_id); + /*~ For all that talk about platform-independence, note that this + * field is endian-dependent! But let's face it, little-endian won. + * In related news, we don't support EBCDIC or middle-endian. */ memcpy(input + PUBKEY_DER_LEN, &dbid, sizeof(dbid)); hkdf_sha256(channel_seed, sizeof(*channel_seed), @@ -176,6 +244,7 @@ static void get_channel_seed(const struct pubkey *peer_id, u64 dbid, info, strlen(info)); } +/*~ Called at startup to derive the bip32 field. */ static void populate_secretstuff(void) { u8 bip32_seed[BIP32_ENTROPY_LEN_256]; @@ -216,6 +285,9 @@ static void populate_secretstuff(void) /* Hence child 0, then child 0 again to get extkey to derive from. */ if (bip32_key_from_parent(&master_extkey, 0, BIP32_FLAG_KEY_PRIVATE, &child_extkey) != WALLY_OK) + /*~ status_failed() is a helper which exits and sends lightningd + * a message about what happened. For hsmd, that's fatal to + * lightningd. */ status_failed(STATUS_FAIL_INTERNAL_ERROR, "Can't derive child bip32 key"); @@ -225,7 +297,8 @@ static void populate_secretstuff(void) "Can't derive private bip32 key"); } -/* If privkey is NULL, we don't fill it in */ +/*~ Get the keys for this given BIP32 index: if privkey is NULL, we + * don't fill it in. */ static void bitcoin_key(struct privkey *privkey, struct pubkey *pubkey, u32 index) { @@ -239,12 +312,15 @@ static void bitcoin_key(struct privkey *privkey, struct pubkey *pubkey, status_failed(STATUS_FAIL_MASTER_IO, "Index %u too great", index); + /*~ This uses libwally, which doesn't dovetail directly with + * libsecp256k1 even though it, too, uses it internally. */ if (bip32_key_from_parent(&secretstuff.bip32, index, BIP32_FLAG_KEY_PRIVATE, &ext) != WALLY_OK) status_failed(STATUS_FAIL_INTERNAL_ERROR, "BIP32 of %u failed", index); - /* libwally says: The private key with prefix byte 0 */ + /* libwally says: The private key with prefix byte 0; remove it + * for libsecp256k1. */ memcpy(privkey->secret.data, ext.priv_key+1, 32); if (!secp256k1_ec_pubkey_create(secp256k1_ctx, &pubkey->pubkey, privkey->secret.data)) @@ -252,32 +328,50 @@ static void bitcoin_key(struct privkey *privkey, struct pubkey *pubkey, "BIP32 pubkey %u create failed", index); } +/*~ We store our root secret in a "hsm_secret" file (like all of c-lightning, + * we run in the user's .lightningd directory). */ static void maybe_create_new_hsm(void) { + /*~ Note that this is opened for write-only, even though the permissions + * are set to read-only. That's perfectly valid! */ int fd = open("hsm_secret", O_CREAT|O_EXCL|O_WRONLY, 0400); if (fd < 0) { + /* If this is not the first time we've run, it will exist. */ if (errno == EEXIST) return; status_failed(STATUS_FAIL_INTERNAL_ERROR, "creating: %s", strerror(errno)); } + /*~ This is libsodium's cryptographic randomness routine: we assume + * it's doing a good job. */ randombytes_buf(&secretstuff.hsm_secret, sizeof(secretstuff.hsm_secret)); + /*~ ccan/read_write_all has a more convenient return than write() where + * we'd have to check the return value == the length we gave: write() + * can return short on normal files if we run out of disk space. */ if (!write_all(fd, &secretstuff.hsm_secret, sizeof(secretstuff.hsm_secret))) { + /* ccan/noerr contains useful routines like this, which don't + * clobber errno, so we can use it in our error report. */ unlink_noerr("hsm_secret"); status_failed(STATUS_FAIL_INTERNAL_ERROR, "writing: %s", strerror(errno)); } + /*~ fsync (mostly!) ensures that the file has reached the disk. */ if (fsync(fd) != 0) { unlink_noerr("hsm_secret"); status_failed(STATUS_FAIL_INTERNAL_ERROR, "fsync: %s", strerror(errno)); } + /*~ This should never fail if fsync succeeded. But paranoia good, and + * bugs exist. */ if (close(fd) != 0) { unlink_noerr("hsm_secret"); status_failed(STATUS_FAIL_INTERNAL_ERROR, "closing: %s", strerror(errno)); } + /*~ We actually need to sync the *directory itself* to make sure the + * file exists! You're only allowed to open directories read-only in + * modern Unix though. */ fd = open(".", O_RDONLY); if (fd < 0) { status_failed(STATUS_FAIL_INTERNAL_ERROR, @@ -289,9 +383,16 @@ static void maybe_create_new_hsm(void) "fsyncdir: %s", strerror(errno)); } close(fd); + /*~ status_unusual() is good for things which are interesting and + * definitely won't spam the logs. Only status_broken() is higher; + * status_info() is lower, then status_debug() and finally + * status_io(). */ status_unusual("HSM: created new hsm_secret file"); } +/*~ We always load the HSM file, even if we just created it above. This + * both unifies the code paths, and provides a nice sanity check that the + * file contents are as they will be for future invocations. */ static void load_hsm(void) { int fd = open("hsm_secret", O_RDONLY); @@ -306,6 +407,8 @@ static void load_hsm(void) populate_secretstuff(); } +/*~ This is the response to lightningd's HSM_INIT request, which is the first + * thing it sends. */ static struct io_plan *init_hsm(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -315,18 +418,31 @@ static struct io_plan *init_hsm(struct io_conn *conn, /* This must be the master. */ assert(&c->dc == c->master); + /*~ The fromwire_* routines are autogenerated, based on the message + * definitions in hsm_client_wire.csv. The format of those files is + * an extension of the simple comma-separated format output by the + * BOLT tools/extract-formats.py tool. */ if (!fromwire_hsm_init(msg_in)) return bad_req(conn, c, msg_in); maybe_create_new_hsm(); load_hsm(); + /*~ We tell lightning our node id and (public) bip32 seed. */ node_key(NULL, &node_id); + + /*~ Note: marshalling a bip32 tree only marshals the public side, + * not the secrets! So we're not actually handing them out here! + */ return req_reply(conn, c, take(towire_hsm_init_reply(NULL, &node_id, &secretstuff.bip32))); } +/*~ The client has asked us to extract the shared secret from an EC Diffie + * Hellman token. This doesn't leak any information, but requires the private + * key, so the hsmd performs it. It's used to set up an encryption key for the + * connection handshaking (BOLT #8) and for the onion wrapping (BOLT #4). */ static struct io_plan *handle_ecdh(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -338,21 +454,44 @@ static struct io_plan *handle_ecdh(struct io_conn *conn, if (!fromwire_hsm_ecdh_req(msg_in, &point)) return bad_req(conn, c, msg_in); + /*~ We simply use the secp256k1_ecdh function, which really shouldn't + * fail (iff the point is invalid). */ node_key(&privkey, NULL); if (secp256k1_ecdh(secp256k1_ctx, ss.data, &point.pubkey, privkey.secret.data) != 1) { return bad_req_fmt(conn, c, msg_in, "secp256k1_ecdh fail"); } + /*~ In the normal case, we return the shared secret, and then read + * the next msg. */ return req_reply(conn, c, take(towire_hsm_ecdh_resp(NULL, &ss))); } +/*~ The specific routine to sign the channel_announcement message. This is + * defined in BOLT #7, and requires *two* signatures: one from this node's key + * (to prove it's from us), and one from the bitcoin key used to create the + * funding transaction (to prove we own the output). */ static struct io_plan *handle_cannouncement_sig(struct io_conn *conn, struct client *c, const u8 *msg_in) { - /* First 2 + 256 byte are the signatures and msg type, skip them */ - size_t offset = 258; + /*~ Our autogeneration code doesn't define field offsets, so we just + * copy this from the spec itself. + * + * Note that 'check-source' will actually find and check this quote + * against the spec (if available); whitespace is ignored and + * ... means some content is skipped, but it works remarkably well to + * track spec changes. */ + + /* BOLT #7: + * + * - MUST compute the double-SHA256 hash `h` of the message, beginning + * at offset 256, up to the end of the message. + * - Note: the hash skips the 4 signatures but hashes the rest of the + * message, including any future fields appended to the end. + */ + /* First type bytes are the msg type */ + size_t offset = 2 + 256; struct privkey node_pkey; secp256k1_ecdsa_signature node_sig, bitcoin_sig; struct sha256_double hash; @@ -362,10 +501,18 @@ static struct io_plan *handle_cannouncement_sig(struct io_conn *conn, struct privkey funding_privkey; struct secret channel_seed; + /*~ You'll find FIXMEs like this scattered through the code. + * Sometimes they suggest simple improvements which someone like + * yourself should go ahead an implement. Sometimes they're deceptive + * quagmires which will cause you nothing but grief. You decide! */ + /* FIXME: We should cache these. */ get_channel_seed(&c->id, c->dbid, &channel_seed); derive_funding_key(&channel_seed, &funding_pubkey, &funding_privkey); + /*~ fromwire_ routines which need to do allocation take a tal context + * as their first field; tmpctx is good here since we won't need it + * after this function. */ if (!fromwire_hsm_cannouncement_sig_req(tmpctx, msg_in, &ca)) return bad_req(conn, c, msg_in); @@ -374,6 +521,8 @@ static struct io_plan *handle_cannouncement_sig(struct io_conn *conn, "bad cannounce length %zu", tal_count(ca)); + /*~ Christian uses TODO(cdecker), but I'm sure he won't mind if you fix + * this for him! */ /* TODO(cdecker) Check that this is actually a valid * channel_announcement */ node_key(&node_pkey, NULL); @@ -387,10 +536,17 @@ static struct io_plan *handle_cannouncement_sig(struct io_conn *conn, return req_reply(conn, c, take(reply)); } +/*~ The specific routine to sign the channel_update message. */ static struct io_plan *handle_channel_update_sig(struct io_conn *conn, struct client *c, const u8 *msg_in) { + /* BOLT #7: + * + * - MUST set `signature` to the signature of the double-SHA256 of the + * entire remaining packet after `signature`, using its own + * `node_id`. + */ /* 2 bytes msg type + 64 bytes signature */ size_t offset = 66; struct privkey node_pkey; @@ -428,6 +584,12 @@ static struct io_plan *handle_channel_update_sig(struct io_conn *conn, return req_reply(conn, c, take(towire_hsm_cupdate_sig_reply(NULL, cu))); } +/*~ This gets the basepoints for a channel; it's not privite information really + * (we tell the peer this to establish a channel, as it sets up the keys used + * for each transaction). + * + * Note that this is asked by lightningd, so it tells us what channels it wants. + */ static struct io_plan *handle_get_channel_basepoints(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -450,6 +612,12 @@ static struct io_plan *handle_get_channel_basepoints(struct io_conn *conn, &funding_pubkey))); } +/*~ This is another lightningd-only interface; signing a commit transaction. + * This is dangerous, since if we sign a revoked commitment tx we'll lose + * funds, thus it's only available to lightningd. + * + * + * Oh look, another FIXME! */ /* FIXME: Ensure HSM never does this twice for same dbid! */ static struct io_plan *handle_sign_commitment_tx(struct io_conn *conn, struct client *c, @@ -474,10 +642,18 @@ static struct io_plan *handle_sign_commitment_tx(struct io_conn *conn, derive_basepoints(&channel_seed, &local_funding_pubkey, NULL, &secrets, NULL); + /*~ Bitcoin signatures cover the (part of) the script they're + * executing; the rules are a bit complex in general, but for + * Segregated Witness it's simply the current script. */ funding_wscript = bitcoin_redeem_2of2(tmpctx, &local_funding_pubkey, &remote_funding_pubkey); - /* Need input amount for signing */ + /*~ Segregated Witness also added the input amount to the signing + * algorithm; it's only part of the input implicitly (it's part of the + * output it's spending), so in our 'bitcoin_tx' structure it's a + * pointer, as we don't always know it (and zero is a valid amount, so + * NULL is better to mean 'unknown' and has the nice property that + * you'll crash if you assume it's there and you're wrong. */ tx->input[0].amount = tal_dup(tx->input, u64, &funding_amount); sign_tx_input(tx, 0, NULL, funding_wscript, &secrets.funding_privkey, @@ -488,6 +664,13 @@ static struct io_plan *handle_sign_commitment_tx(struct io_conn *conn, take(towire_hsm_sign_commitment_tx_reply(NULL, &sig))); } +/*~ This is used by channeld to create signatures for the remote peer's + * commitment transaction. It's functionally identical to signing our own, + * but we expect to do this repeatedly as commitment transactions are + * updated. + * + * The HSM almost certainly *should* do more checks before signing! + */ /* FIXME: make sure it meets some criteria? */ static struct io_plan *handle_sign_remote_commitment_tx(struct io_conn *conn, struct client *c, @@ -524,6 +707,8 @@ static struct io_plan *handle_sign_remote_commitment_tx(struct io_conn *conn, return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } +/*~ This is used by channeld to create signatures for the remote peer's + * HTLC transactions. */ static struct io_plan *handle_sign_remote_htlc_tx(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -567,6 +752,8 @@ static struct io_plan *handle_sign_remote_htlc_tx(struct io_conn *conn, return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } +/*~ This covers several cases where onchaind is creating a transaction which + * sends funds to our internal wallet. */ /* FIXME: Derive output address for this client, and check it here! */ static struct io_plan *handle_sign_to_us_tx(struct io_conn *conn, struct client *c, @@ -591,6 +778,10 @@ static struct io_plan *handle_sign_to_us_tx(struct io_conn *conn, return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } +/*~ When we send a commitment transaction onchain (unilateral close), there's + * a delay before we can spend it. onchaind does an explicit transaction to + * transfer it to the wallet so that doesn't need to remember how to spend + * this complex transaction. */ static struct io_plan *handle_sign_delayed_payment_to_us(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -604,6 +795,7 @@ static struct io_plan *handle_sign_delayed_payment_to_us(struct io_conn *conn, struct privkey privkey; u8 *wscript; + /*~ We don't derive the wscript ourselves, but perhaps we should? */ if (!fromwire_hsm_sign_delayed_payment_to_us(tmpctx, msg_in, &commit_num, &tx, &wscript, @@ -612,14 +804,22 @@ static struct io_plan *handle_sign_delayed_payment_to_us(struct io_conn *conn, get_channel_seed(&c->id, c->dbid, &channel_seed); + /*~ ccan/crypto/shachain how we efficiently derive 2^48 ordered + * preimages from a single seed; the twist is that as the preimages + * are revealed, you can generate the previous ones yourself, needing + * to only keep log(N) of them at any time. */ if (!derive_shaseed(&channel_seed, &shaseed)) return bad_req_fmt(conn, c, msg_in, "bad derive_shaseed"); + /*~ BOLT #3 describes exactly how this is used to generate the Nth + * per-commitment point. */ if (!per_commit_point(&shaseed, &per_commitment_point, commit_num)) return bad_req_fmt(conn, c, msg_in, "bad per_commitment_point %"PRIu64, commit_num); + /*~ ... which is combined with the basepoint to generate then N'th key. + */ if (!derive_delayed_payment_basepoint(&channel_seed, &basepoint, &basepoint_secret)) @@ -635,6 +835,9 @@ static struct io_plan *handle_sign_delayed_payment_to_us(struct io_conn *conn, tx, &privkey, wscript, input_amount); } +/*~ This is used when the a commitment transaction is onchain, and has an HTLC + * output paying to us (because we have the preimage); this signs that + * transaction, which lightningd will broadcast to collect the funds. */ static struct io_plan *handle_sign_remote_htlc_to_us(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -671,6 +874,9 @@ static struct io_plan *handle_sign_remote_htlc_to_us(struct io_conn *conn, tx, &privkey, wscript, input_amount); } +/*~ This is used when the remote peer's commitment transaction is revoked; + * we can use the revocation secret to spend the outputs. For simplicity, + * we do them one at a time, though. */ static struct io_plan *handle_sign_penalty_to_us(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -711,6 +917,9 @@ static struct io_plan *handle_sign_penalty_to_us(struct io_conn *conn, tx, &privkey, wscript, input_amount); } +/*~ This is used when the a commitment transaction is onchain, and has an HTLC + * output paying to them, which has timed out; this signs that transaction, + * which lightningd will broadcast to collect the funds. */ static struct io_plan *handle_sign_local_htlc_tx(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -766,6 +975,11 @@ static struct io_plan *handle_sign_local_htlc_tx(struct io_conn *conn, return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } +/*~ This get the Nth a per-commitment point, and for N > 2, returns the + * grandparent per-commitment secret. This pattern is because after + * negotiating commitment N-1, we send them the next per-commitment point, + * and reveal the previous per-commitment secret as a promise not to spend + * the previous commitment transaction. */ static struct io_plan *handle_get_per_commitment_point(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -797,12 +1011,19 @@ static struct io_plan *handle_get_per_commitment_point(struct io_conn *conn, } else old_secret = NULL; + /*~ hsm_client_wire.csv marks the secret field here optional, so it only + * gets included if the parameter is non-NULL. We violate 80 columns + * pretty badly here, but it's a recommendation not a religion. */ return req_reply(conn, c, take(towire_hsm_get_per_commitment_point_reply(NULL, &per_commitment_point, old_secret))); } +/*~ This is used when the remote peer claims to have knowledge of future + * commitment states (option_data_loss_protect in the spec) which means we've + * been restored from backup or something, and may have already revealed + * secrets. We carefully check that this is true, here. */ static struct io_plan *handle_check_future_secret(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -823,11 +1044,16 @@ static struct io_plan *handle_check_future_secret(struct io_conn *conn, return bad_req_fmt(conn, c, msg_in, "bad commit secret #%"PRIu64, n); + /*~ Note the special secret_eq_consttime: we generate foo_eq for many + * types using ccan/structeq, but not 'struct secret' because any + * comparison risks leaking information about the secret if it is + * timing dependent. */ return req_reply(conn, c, take(towire_hsm_check_future_secret_reply(NULL, secret_eq_consttime(&secret, &suggested)))); } +/* This is used by closingd to sign off on a mutual close tx. */ static struct io_plan *handle_sign_mutual_close_tx(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -866,6 +1092,8 @@ static struct io_plan *handle_sign_mutual_close_tx(struct io_conn *conn, return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } +/* This is used by by the master to create a new client connection (which + * becomes the HSM_FD for the subdaemon after forking). */ static struct io_plan *pass_client_hsmfd(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -880,15 +1108,30 @@ static struct io_plan *pass_client_hsmfd(struct io_conn *conn, if (!fromwire_hsm_client_hsmfd(msg_in, &id, &dbid, &capabilities)) return bad_req(conn, c, msg_in); + /* socketpair is a bi-directional pipe, which is what we want. */ if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) != 0) - status_failed(STATUS_FAIL_INTERNAL_ERROR, "creating fds: %s", strerror(errno)); + status_failed(STATUS_FAIL_INTERNAL_ERROR, "creating fds: %s", + strerror(errno)); new_client(&c->dc, &id, dbid, capabilities, fds[0]); daemon_conn_send(&c->dc, take(towire_hsm_client_hsmfd_reply(NULL))); + /* There's arcane UNIX magic to send an open file descriptor over a + * UNIX domain socket. There's no great way to autogenerate this + * though; especially for the receive side, so we always pass these + * manually immediately following the message. */ daemon_conn_send_fd(&c->dc, fds[1]); return daemon_conn_read_next(conn, &c->dc); } +/*~ For almost every wallet tx we use the BIP32 seed, but not for onchain + * unilateral closes from a peer: they (may) have an output to us using a + * public key based on the channel basepoints. It's a bit spammy to spend + * those immediately just to make the wallet simpler, and we didn't appreciate + * the problem when we designed the protocol for commitment transaction keys. + * + * So we store just enough about the channel it came from (which may be + * long-gone) to regenerate the keys here. That has the added advantage that + * the secrets themselves stay within the HSM. */ static void hsm_unilateral_close_privkey(struct privkey *dst, struct unilateral_close_info *info) { @@ -907,9 +1150,7 @@ static void hsm_unilateral_close_privkey(struct privkey *dst, } } -/** - * hsm_key_for_utxo - generate the keypair matching the utxo - */ +/* This gets the bitcoin private key needed to spend from our wallet. */ static void hsm_key_for_utxo(struct privkey *privkey, struct pubkey *pubkey, const struct utxo *utxo) { @@ -919,17 +1160,32 @@ static void hsm_key_for_utxo(struct privkey *privkey, struct pubkey *pubkey, status_debug("Unilateral close output, deriving secrets"); hsm_unilateral_close_privkey(privkey, utxo->close_info); pubkey_from_privkey(privkey, pubkey); - status_debug("Derived public key %s from unilateral close", type_to_string(tmpctx, struct pubkey, pubkey)); + status_debug("Derived public key %s from unilateral close", + type_to_string(tmpctx, struct pubkey, pubkey)); } else { /* Simple case: just get derive via HD-derivation */ bitcoin_key(privkey, pubkey, utxo->keyindex); } } +/* This completes the tx by filling in the input scripts with signatures. */ static void sign_all_inputs(struct bitcoin_tx *tx, struct utxo **utxos) { + /* FIXME: sign_tx_input is dumb and needs all input->script to be + * NULL, so we gather these here and assign them at the end */ u8 **scriptSigs = tal_arr(tmpctx, u8 *, tal_count(utxos)); + /*~ Deep in my mind there's a continuous battle: should arrays be + * named as singular or plural? Is consistency the sign of a weak + * mind? + * + * ZmnSCPxj answers thusly: One must make peace with the fact, that + * the array itself is singular, yet its contents are plural. Do you + * name the array, or do you name its contents? Is the array itself + * the thing and the whole of the thing, or is it its contents that + * define what it is? + * + *... I'm not sure that helps! */ assert(tal_count(tx->input) == tal_count(utxos)); for (size_t i = 0; i < tal_count(utxos); i++) { struct pubkey inkey; @@ -938,29 +1194,38 @@ static void sign_all_inputs(struct bitcoin_tx *tx, struct utxo **utxos) u8 *subscript, *wscript; secp256k1_ecdsa_signature sig; + /* Figure out keys to spend this. */ hsm_key_for_utxo(&inprivkey, &inkey, in); + /* It's either a p2wpkh or p2sh (we support that so people from + * the last bitcoin era can put funds into the wallet) */ wscript = p2wpkh_scriptcode(tmpctx, &inkey); if (in->is_p2sh) { + /* For P2SH-wrapped Segwit, the (implied) redeemScript + * is defined in BIP141 */ subscript = bitcoin_redeem_p2sh_p2wpkh(tmpctx, &inkey); scriptSigs[i] = bitcoin_scriptsig_p2sh_p2wpkh(tx, &inkey); } else { + /* Pure segwit uses an empty inputScript; NULL has + * tal_count() == 0, so it works great here. */ subscript = NULL; scriptSigs[i] = NULL; } + /* This is the core crypto magic. */ sign_tx_input(tx, i, subscript, wscript, &inprivkey, &inkey, &sig); + /* The witness is [sig] [key] */ tx->input[i].witness = bitcoin_witness_p2wpkh(tx, &sig, &inkey); } - /* Now complete the transaction by attaching the scriptSigs where necessary */ + /* Now complete the transaction by attaching the scriptSigs */ for (size_t i = 0; i < tal_count(utxos); i++) tx->input[i].script = scriptSigs[i]; } -/* Note that it's the main daemon that asks for the funding signature so it - * can broadcast it. */ +/*~ lightningd asks us to sign the transaction to fund a channel; it feeds us + * the set of inputs and the local and remote pubkeys, and we sign it. */ static struct io_plan *handle_sign_funding_tx(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -987,6 +1252,11 @@ static struct io_plan *handle_sign_funding_tx(struct io_conn *conn, changekey = NULL; tx = funding_tx(tmpctx, &outnum, + /*~ For simplicity, our generated code is not const + * correct. The C rules around const and + * pointer-to-pointer are a bit weird, so we use + * ccan/cast which ensures the type is correct and + * we're not casting something random */ cast_const2(const struct utxo **, utxos), satoshi_out, &local_pubkey, &remote_pubkey, change_out, changekey, @@ -996,9 +1266,8 @@ static struct io_plan *handle_sign_funding_tx(struct io_conn *conn, return req_reply(conn, c, take(towire_hsm_sign_funding_reply(NULL, tx))); } -/** - * sign_withdrawal_tx - Generate and sign a withdrawal transaction from the master - */ +/*~ lightningd asks us to sign a withdrawal; same as above but we in theory + * we can do more to check the previous case is valid. */ static struct io_plan *handle_sign_withdrawal_tx(struct io_conn *conn, struct client *c, const u8 *msg_in) @@ -1032,13 +1301,18 @@ static struct io_plan *handle_sign_withdrawal_tx(struct io_conn *conn, take(towire_hsm_sign_withdrawal_reply(NULL, tx))); } -/** - * sign_invoice - Sign an invoice with our key. - */ +/*~ Lightning invoices, defined by BOLT 11, are signed. This has been + * surprisingly controversial; it means a node needs to be online to create + * invoices. However, it seems clear to me that in a world without + * intermedaries you need proof that you have received an offer (the + * signature), as well as proof that you've paid it (the preimage). */ static struct io_plan *handle_sign_invoice(struct io_conn *conn, struct client *c, const u8 *msg_in) { + /*~ We make up a 'u5' type to represent BOLT11's 5-bits-per-byte + * format: it's only for human consumption, as typedefs are almost + * entirely transparent to the C compiler. */ u5 *u5bytes; u8 *hrpu8; char *hrp; @@ -1050,8 +1324,25 @@ static struct io_plan *handle_sign_invoice(struct io_conn *conn, if (!fromwire_hsm_sign_invoice(tmpctx, msg_in, &u5bytes, &hrpu8)) return bad_req(conn, c, msg_in); + /* BOLT #11: + * + * A writer MUST set `signature` to a valid 512-bit secp256k1 + * signature of the SHA2 256-bit hash of the human-readable part, + * represented as UTF-8 bytes, concatenated with the data part + * (excluding the signature) with zero bits appended to pad the data + * to the next byte boundary, with a trailing byte containing the + * recovery ID (0, 1, 2 or 3). + */ + /* FIXME: Check invoice! */ + /* tal_dup_arr() does what you'd expect: allocate an array by copying + * another; the cast is needed because the hrp is a 'char' array, not + * a 'u8' (unsigned char) as it's the "human readable" part. + * + * The final arg of tal_dup_arr() is how many extra bytes to allocate: + * it's so often zero that I've thought about dropping the argument, but + * in cases like this (adding a NUL terminator) it's perfect. */ hrp = tal_dup_arr(tmpctx, char, (char *)hrpu8, tal_count(hrpu8), 1); hrp[tal_count(hrpu8)] = '\0'; @@ -1060,6 +1351,8 @@ static struct io_plan *handle_sign_invoice(struct io_conn *conn, hash_u5_done(&hu5, &sha); node_key(&node_pkey, NULL); + /*~ By no small coincidence, this libsecp routine uses the exact + * recovery signature format mandated by BOLT 11. */ if (!secp256k1_ecdsa_sign_recoverable(secp256k1_ctx, &rsig, (const u8 *)&sha, node_pkey.secret.data, @@ -1071,10 +1364,21 @@ static struct io_plan *handle_sign_invoice(struct io_conn *conn, take(towire_hsm_sign_invoice_reply(NULL, &rsig))); } +/*~ It's optional for nodes to send node_announcement, but it lets us set our + * favourite color and cool alias! Plus other minor details like how to + * connect to us. */ static struct io_plan *handle_sign_node_announcement(struct io_conn *conn, struct client *c, const u8 *msg_in) { + /* BOLT #7: + * + * The origin node: + *... + * - MUST set `signature` to the signature of the double-SHA256 of the + * entire remaining packet after `signature` (using the key given by + * `node_id`). + */ /* 2 bytes msg type + 64 bytes signature */ size_t offset = 66; struct sha256_double hash; @@ -1100,9 +1404,24 @@ static struct io_plan *handle_sign_node_announcement(struct io_conn *conn, return req_reply(conn, c, take(reply)); } +/*~ This routine checks that a client is allowed to call the handler. */ static bool check_client_capabilities(struct client *client, enum hsm_wire_type t) { + /*~ Here's a useful trick: enums in C are not real types, they're + * semantic sugar sprinkled over an int, bascally (in fact, older + * versions of gcc used to convert the values ints in the parser!). + * + * But GCC will do one thing for us: if we have a switch statement + * with a controlling expression which is an enum, it will warn us + * if a declared enum value is *not* handled in the switch, eg: + * enumeration value ‘FOOBAR’ not handled in switch [-Werror=switch] + * + * This only works if there's no 'default' label, which is sometimes + * hard, as we *can* have non-enum values in our enum. But the tradeoff + * is worth it so the compiler tells us everywhere we have to fix when + * we add a new enum identifier! + */ switch (t) { case WIRE_HSM_ECDH_REQ: return (client->capabilities & HSM_CAP_ECDH) != 0; @@ -1138,7 +1457,9 @@ static bool check_client_capabilities(struct client *client, case WIRE_HSM_GET_CHANNEL_BASEPOINTS: return (client->capabilities & HSM_CAP_MASTER) != 0; - /* These are messages sent by the HSM so we should never receive them */ + /*~ These are messages sent by the HSM so we should never receive them. + * FIXME: Since we autogenerate these, we should really generate separate + * enums for replies to avoid this kind of clutter! */ case WIRE_HSM_ECDH_RESP: case WIRE_HSM_CANNOUNCEMENT_SIG_REPLY: case WIRE_HSM_CUPDATE_SIG_REPLY: @@ -1159,9 +1480,16 @@ static bool check_client_capabilities(struct client *client, return false; } +/*~ This is the core of the HSM daemon: handling requests. */ static struct io_plan *handle_client(struct io_conn *conn, struct daemon_conn *dc) { + /*~ Note the use of container_of here: this is the Linux kernel way of + * doing callbacks. Rather than have struct daemon_conn contain a + * void * pointer to the structure for this use, we simply embed the + * daemon_conn in the structure; container_of is a fancy way of doing + * pointer arithmetic to get the containing structure, saving a + * pointer. */ struct client *c = container_of(dc, struct client, dc); enum hsm_wire_type t = fromwire_peektype(dc->msg_in); @@ -1256,6 +1584,9 @@ static struct io_plan *handle_client(struct io_conn *conn, return bad_req_fmt(conn, c, dc->msg_in, "Unknown request"); } +/*~ This is the destructor on our client: we may call it manually, but + * generally it's called because the io_conn associated with the client is + * closed by the other end. */ static void destroy_client(struct client *c) { if (!uintmap_del(&clients, c->dbid)) @@ -1271,6 +1602,7 @@ static struct client *new_client(struct daemon_conn *master, { struct client *c = tal(master, struct client); + /*~ All-zero pubkey is used for the initial master connection */ if (id) { c->id = *id; } else { @@ -1280,13 +1612,25 @@ static struct client *new_client(struct daemon_conn *master, c->master = master; c->capabilities = capabilities; + /*~ This is our daemon_conn infrastructure, which does the queueing for + * us; we just tell it what our handler function is. */ daemon_conn_init(c, &c->dc, fd, handle_client, NULL); - /* Free the connection if we exit everything. */ + /*~ tal_steal() moves a pointer to a new parent. At this point, the + * hierarchy is: + * + * master -> c -> daemon_conn.conn + * + * We want to invert the bottom two, so that if the io_conn closes, + * the client is freed: + * + * master -> c->conn -> c. + */ tal_steal(master, c->dc.conn); - /* Free client when connection freed. */ tal_steal(c->dc.conn, c); + /* We put the special zero-db HSM connections into an array, the rest + * go into the map. */ if (dbid == 0) { assert(num_dbid_zero_clients < ARRAY_SIZE(dbid_zero_clients)); dbid_zero_clients[num_dbid_zero_clients++] = c; @@ -1319,6 +1663,7 @@ int main(int argc, char *argv[]) setup_locale(); + /* This sets up tmpctx, various DEVELOPER options, backtraces, etc. */ subdaemon_setup(argc, argv); /* A trivial daemon_conn just for writing. */ @@ -1337,6 +1682,18 @@ int main(int argc, char *argv[]) /* When conn closes, everything is freed. */ io_set_finish(master->dc.conn, master_gone, &master->dc); + /*~ The two NULL args a list of timers, and the timer which expired: + * we don't have any timers. */ io_loop(NULL, NULL); + + /*~ This should never be reached: io_loop only exits on io_break which + * we don't call, a timer expiry which we don't have, or all connections + * being closed, and closing the master calls master_gone. */ abort(); } + +/*~ Congratulations on making it through the first of the seven dwarves! + * (And Christian wondered why I'm so fond of having separate daemons!). + * + * We continue our story in the next-more-complex daemon: connectd/connectd.c + */ From b0769d9c0cd77877cf1fa98fc27bc062d620de2d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 14:01:26 +0930 Subject: [PATCH 18/20] hsmd: don't use daemon_conn for clients. It offers them a DoS vector, if they don't read the replies. We really want to use raw ccan/io so we can avoid buffering for this. It makes the handing of fds for new clients a bit more complex (callback based), but it's not too bad. Signed-off-by: Rusty Russell --- hsmd/hsmd.c | 182 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 120 insertions(+), 62 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 21cb1cc97b2d..1f227acdb21c 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -67,8 +67,15 @@ static struct { /*~ We keep track of clients, but there's not much to keep. */ struct client { - struct daemon_conn dc; - struct daemon_conn *master; + /* The connection to lightningd itself */ + struct client *master; + + /* The ccan/io async io connection for this client: it closes, we die. */ + struct io_conn *conn; + + /*~ io_read_wire needs a pointer to store incoming messages until + * it has the complete thing; this is it. */ + u8 *msg_in; /* ~Useful for logging, but also used to derive the per-channel seed. */ struct pubkey id; @@ -105,11 +112,12 @@ extern void dev_disconnect_init(int fd); void dev_disconnect_init(int fd UNUSED) { } /* Pre-declare this, due to mutual recursion */ -static struct client *new_client(struct daemon_conn *master, +static struct client *new_client(struct client *master, const struct pubkey *id, u64 dbid, const u64 capabilities, int fd); +static struct io_plan *handle_client(struct io_conn *conn, struct client *c); /*~ ccan/compiler.h defines PRINTF_FMT as the gcc compiler hint so it will * check that fmt and other trailing arguments really are the correct type. @@ -133,7 +141,7 @@ static PRINTF_FMT(4,5) /*~ If the client was actually lightningd, it's Game Over; we actually * fail in this case, and it will too. */ - if (&c->dc == c->master) { + if (c == c->master) { status_broken("%s", str); master_badmsg(fromwire_peektype(msg_in), msg_in); } @@ -164,13 +172,39 @@ static struct io_plan *bad_req(struct io_conn *conn, return bad_req_fmt(conn, c, msg_in, "could not parse request"); } +/*~ This plan simply says: read the next packet into 'c->msg_in' (parent 'c'), + * and then call handle_client with argument 'c' */ +static struct io_plan *client_read_next(struct io_conn *conn, struct client *c) +{ + return io_read_wire(conn, c, &c->msg_in, handle_client, c); +} + /* This is the common pattern for the tail of each handler in this file. */ static struct io_plan *req_reply(struct io_conn *conn, struct client *c, const u8 *msg_out TAKES) { - daemon_conn_send(&c->dc, msg_out); - return daemon_conn_read_next(conn, &c->dc); + /*~ Write this out, then read the next one. This works perfectly for + * a simple request/response system like this. + * + * Internally, the ccan/io subsystem gathers all the file descriptors, + * figures out which want to write and read, asks the OS which ones + * are available, and for those file descriptors, tries to do the + * reads/writes we've asked it. It handles retry in the case where a + * read or write is done partially. + * + * Since the OS does buffering internally (on my system, over 100k + * worth) writes will normally succeed immediately. However, if the + * client is slow or malicious, and doesn't read from the socket as + * fast as we're writing, eventually the socket buffer will fill up; + * we don't care, because ccan/io will wait until there's room to + * write this reply before it will read again. The client just hurts + * themselves, and there's no Denial of Service on us. + * + * If we were to queue outgoing messages ourselves, we *would* have to + * consider such scenarios; this is why our daemons generally avoid + * buffering from untrusted parties. */ + return io_write_wire(conn, msg_out, client_read_next, c); } /*~ This returns the secret and/or public key for this node. */ @@ -416,7 +450,7 @@ static struct io_plan *init_hsm(struct io_conn *conn, struct pubkey node_id; /* This must be the master. */ - assert(&c->dc == c->master); + assert(c == c->master); /*~ The fromwire_* routines are autogenerated, based on the message * definitions in hsm_client_wire.csv. The format of those files is @@ -1092,7 +1126,37 @@ static struct io_plan *handle_sign_mutual_close_tx(struct io_conn *conn, return req_reply(conn, c, take(towire_hsm_sign_tx_reply(NULL, &sig))); } -/* This is used by by the master to create a new client connection (which +/*~ Since we process requests then service them in strict order, and because + * only lightningd can request a new client fd, we can get away with a global + * here! But because we are being tricky, I set it to an invalid value when + * not in use, and sprinkle assertions around. */ +static int pending_client_fd = -1; + +/*~ This is the callback from below: having sent the reply, we now send the + * fd for the client end of the new socketpair. */ +static struct io_plan *send_pending_client_fd(struct io_conn *conn, + struct client *master) +{ + int fd = pending_client_fd; + /* This must be the master. */ + assert(master == master->master); + assert(fd != -1); + + /* This sanity check shouldn't be necessary, but it's cheap. */ + pending_client_fd = -1; + + /*~There's arcane UNIX magic to send an open file descriptor over a + * UNIX domain socket. There's no great way to autogenerate this + * though; especially for the receive side, so we always pass these + * manually immediately following the message. + * + * io_send_fd()'s third parameter is whether to close the local one + * after sending; that saves us YA callback. + */ + return io_send_fd(conn, fd, true, client_read_next, master); +} + +/*~ This is used by by the master to create a new client connection (which * becomes the HSM_FD for the subdaemon after forking). */ static struct io_plan *pass_client_hsmfd(struct io_conn *conn, struct client *c, @@ -1102,8 +1166,8 @@ static struct io_plan *pass_client_hsmfd(struct io_conn *conn, u64 dbid, capabilities; struct pubkey id; - /* This must be the master. */ - assert(&c->dc == c->master); + /* This must be lightningd itself. */ + assert(c == c->master); if (!fromwire_hsm_client_hsmfd(msg_in, &id, &dbid, &capabilities)) return bad_req(conn, c, msg_in); @@ -1113,14 +1177,15 @@ static struct io_plan *pass_client_hsmfd(struct io_conn *conn, status_failed(STATUS_FAIL_INTERNAL_ERROR, "creating fds: %s", strerror(errno)); - new_client(&c->dc, &id, dbid, capabilities, fds[0]); - daemon_conn_send(&c->dc, take(towire_hsm_client_hsmfd_reply(NULL))); - /* There's arcane UNIX magic to send an open file descriptor over a - * UNIX domain socket. There's no great way to autogenerate this - * though; especially for the receive side, so we always pass these - * manually immediately following the message. */ - daemon_conn_send_fd(&c->dc, fds[1]); - return daemon_conn_read_next(conn, &c->dc); + status_trace("new_client: %"PRIu64, dbid); + new_client(c, &id, dbid, capabilities, fds[0]); + + /*~ We stash this in a global, because we need to get both the fd and + * the client pointer to the callback. The other way would be to + * create a boutique structure and hand that, but we don't need to. */ + pending_client_fd = fds[1]; + return io_write_wire(conn, take(towire_hsm_client_hsmfd_reply(NULL)), + send_pending_client_fd, c); } /*~ For almost every wallet tx we use the BIP32 seed, but not for onchain @@ -1481,87 +1546,79 @@ static bool check_client_capabilities(struct client *client, } /*~ This is the core of the HSM daemon: handling requests. */ -static struct io_plan *handle_client(struct io_conn *conn, - struct daemon_conn *dc) +static struct io_plan *handle_client(struct io_conn *conn, struct client *c) { - /*~ Note the use of container_of here: this is the Linux kernel way of - * doing callbacks. Rather than have struct daemon_conn contain a - * void * pointer to the structure for this use, we simply embed the - * daemon_conn in the structure; container_of is a fancy way of doing - * pointer arithmetic to get the containing structure, saving a - * pointer. */ - struct client *c = container_of(dc, struct client, dc); - enum hsm_wire_type t = fromwire_peektype(dc->msg_in); + enum hsm_wire_type t = fromwire_peektype(c->msg_in); status_debug("Client: Received message %d from client", t); /* Before we do anything else, is this client allowed to do * what he asks for? */ if (!check_client_capabilities(c, t)) - return bad_req_fmt(conn, c, dc->msg_in, + return bad_req_fmt(conn, c, c->msg_in, "does not have capability to run %d", t); /* Now actually go and do what the client asked for */ switch (t) { case WIRE_HSM_INIT: - return init_hsm(conn, c, dc->msg_in); + return init_hsm(conn, c, c->msg_in); case WIRE_HSM_CLIENT_HSMFD: - return pass_client_hsmfd(conn, c, dc->msg_in); + return pass_client_hsmfd(conn, c, c->msg_in); case WIRE_HSM_GET_CHANNEL_BASEPOINTS: - return handle_get_channel_basepoints(conn, c, dc->msg_in); + return handle_get_channel_basepoints(conn, c, c->msg_in); case WIRE_HSM_ECDH_REQ: - return handle_ecdh(conn, c, dc->msg_in); + return handle_ecdh(conn, c, c->msg_in); case WIRE_HSM_CANNOUNCEMENT_SIG_REQ: - return handle_cannouncement_sig(conn, c, dc->msg_in); + return handle_cannouncement_sig(conn, c, c->msg_in); case WIRE_HSM_CUPDATE_SIG_REQ: - return handle_channel_update_sig(conn, c, dc->msg_in); + return handle_channel_update_sig(conn, c, c->msg_in); case WIRE_HSM_SIGN_FUNDING: - return handle_sign_funding_tx(conn, c, dc->msg_in); + return handle_sign_funding_tx(conn, c, c->msg_in); case WIRE_HSM_NODE_ANNOUNCEMENT_SIG_REQ: - return handle_sign_node_announcement(conn, c, dc->msg_in); + return handle_sign_node_announcement(conn, c, c->msg_in); case WIRE_HSM_SIGN_INVOICE: - return handle_sign_invoice(conn, c, dc->msg_in); + return handle_sign_invoice(conn, c, c->msg_in); case WIRE_HSM_SIGN_WITHDRAWAL: - return handle_sign_withdrawal_tx(conn, c, dc->msg_in); + return handle_sign_withdrawal_tx(conn, c, c->msg_in); case WIRE_HSM_SIGN_COMMITMENT_TX: - return handle_sign_commitment_tx(conn, c, dc->msg_in); + return handle_sign_commitment_tx(conn, c, c->msg_in); case WIRE_HSM_SIGN_DELAYED_PAYMENT_TO_US: - return handle_sign_delayed_payment_to_us(conn, c, dc->msg_in); + return handle_sign_delayed_payment_to_us(conn, c, c->msg_in); case WIRE_HSM_SIGN_REMOTE_HTLC_TO_US: - return handle_sign_remote_htlc_to_us(conn, c, dc->msg_in); + return handle_sign_remote_htlc_to_us(conn, c, c->msg_in); case WIRE_HSM_SIGN_PENALTY_TO_US: - return handle_sign_penalty_to_us(conn, c, dc->msg_in); + return handle_sign_penalty_to_us(conn, c, c->msg_in); case WIRE_HSM_SIGN_LOCAL_HTLC_TX: - return handle_sign_local_htlc_tx(conn, c, dc->msg_in); + return handle_sign_local_htlc_tx(conn, c, c->msg_in); case WIRE_HSM_GET_PER_COMMITMENT_POINT: - return handle_get_per_commitment_point(conn, c, dc->msg_in); + return handle_get_per_commitment_point(conn, c, c->msg_in); case WIRE_HSM_CHECK_FUTURE_SECRET: - return handle_check_future_secret(conn, c, dc->msg_in); + return handle_check_future_secret(conn, c, c->msg_in); case WIRE_HSM_SIGN_REMOTE_COMMITMENT_TX: - return handle_sign_remote_commitment_tx(conn, c, dc->msg_in); + return handle_sign_remote_commitment_tx(conn, c, c->msg_in); case WIRE_HSM_SIGN_REMOTE_HTLC_TX: - return handle_sign_remote_htlc_tx(conn, c, dc->msg_in); + return handle_sign_remote_htlc_tx(conn, c, c->msg_in); case WIRE_HSM_SIGN_MUTUAL_CLOSE_TX: - return handle_sign_mutual_close_tx(conn, c, dc->msg_in); + return handle_sign_mutual_close_tx(conn, c, c->msg_in); case WIRE_HSM_ECDH_RESP: case WIRE_HSM_CANNOUNCEMENT_SIG_REPLY: @@ -1581,7 +1638,7 @@ static struct io_plan *handle_client(struct io_conn *conn, break; } - return bad_req_fmt(conn, c, dc->msg_in, "Unknown request"); + return bad_req_fmt(conn, c, c->msg_in, "Unknown request"); } /*~ This is the destructor on our client: we may call it manually, but @@ -1594,7 +1651,7 @@ static void destroy_client(struct client *c) "Failed to remove client dbid %"PRIu64, c->dbid); } -static struct client *new_client(struct daemon_conn *master, +static struct client *new_client(struct client *master, const struct pubkey *id, u64 dbid, const u64 capabilities, @@ -1612,22 +1669,23 @@ static struct client *new_client(struct daemon_conn *master, c->master = master; c->capabilities = capabilities; - /*~ This is our daemon_conn infrastructure, which does the queueing for - * us; we just tell it what our handler function is. */ - daemon_conn_init(c, &c->dc, fd, handle_client, NULL); + /*~ This is the core of ccan/io: the connection creation calls a + * callback which returns the initial plan to execute: in our case, + * read a message.*/ + c->conn = io_new_conn(master, fd, client_read_next, c); /*~ tal_steal() moves a pointer to a new parent. At this point, the * hierarchy is: * - * master -> c -> daemon_conn.conn + * master -> c + * master -> c->conn * - * We want to invert the bottom two, so that if the io_conn closes, + * We want to the c->conn to own 'c', so that if the io_conn closes, * the client is freed: * * master -> c->conn -> c. */ - tal_steal(master, c->dc.conn); - tal_steal(c->dc.conn, c); + tal_steal(c->conn, c); /* We put the special zero-db HSM connections into an array, the rest * go into the map. */ @@ -1639,7 +1697,7 @@ static struct client *new_client(struct daemon_conn *master, /* Close conn and free any old client of this dbid. */ if (old_client) - io_close(old_client->dc.conn); + io_close(old_client->conn); if (!uintmap_add(&clients, dbid, c)) status_failed(STATUS_FAIL_INTERNAL_ERROR, @@ -1650,7 +1708,7 @@ static struct client *new_client(struct daemon_conn *master, return c; } -static void master_gone(struct io_conn *unused UNUSED, struct daemon_conn *dc UNUSED) +static void master_gone(struct io_conn *unused UNUSED, struct client *c UNUSED) { daemon_shutdown(); /* Can't tell master, it's gone. */ @@ -1677,10 +1735,10 @@ int main(int argc, char *argv[]) REQ_FD); /* We're our own master! */ - master->master = &master->dc; + master->master = master; /* When conn closes, everything is freed. */ - io_set_finish(master->dc.conn, master_gone, &master->dc); + io_set_finish(master->conn, master_gone, master); /*~ The two NULL args a list of timers, and the timer which expired: * we don't have any timers. */ From 019bc4fcd7ca24fe8b50554aaffd468dd7775543 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 14:01:26 +0930 Subject: [PATCH 19/20] hsmd: reorder functions (MOVEONLY). We don't need to pre-declare any more, but I left it in the previous patch for review simplicity. Signed-off-by: Rusty Russell --- hsmd/hsmd.c | 139 +++++++++++++++++++++++++--------------------------- 1 file changed, 67 insertions(+), 72 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 1f227acdb21c..4027052273d7 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -112,11 +112,6 @@ extern void dev_disconnect_init(int fd); void dev_disconnect_init(int fd UNUSED) { } /* Pre-declare this, due to mutual recursion */ -static struct client *new_client(struct client *master, - const struct pubkey *id, - u64 dbid, - const u64 capabilities, - int fd); static struct io_plan *handle_client(struct io_conn *conn, struct client *c); /*~ ccan/compiler.h defines PRINTF_FMT as the gcc compiler hint so it will @@ -179,6 +174,73 @@ static struct io_plan *client_read_next(struct io_conn *conn, struct client *c) return io_read_wire(conn, c, &c->msg_in, handle_client, c); } +/*~ This is the destructor on our client: we may call it manually, but + * generally it's called because the io_conn associated with the client is + * closed by the other end. */ +static void destroy_client(struct client *c) +{ + if (!uintmap_del(&clients, c->dbid)) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "Failed to remove client dbid %"PRIu64, c->dbid); +} + +static struct client *new_client(struct client *master, + const struct pubkey *id, + u64 dbid, + const u64 capabilities, + int fd) +{ + struct client *c = tal(master, struct client); + + /*~ All-zero pubkey is used for the initial master connection */ + if (id) { + c->id = *id; + } else { + memset(&c->id, 0, sizeof(c->id)); + } + c->dbid = dbid; + + c->master = master; + c->capabilities = capabilities; + /*~ This is the core of ccan/io: the connection creation calls a + * callback which returns the initial plan to execute: in our case, + * read a message.*/ + c->conn = io_new_conn(master, fd, client_read_next, c); + + /*~ tal_steal() moves a pointer to a new parent. At this point, the + * hierarchy is: + * + * master -> c + * master -> c->conn + * + * We want to the c->conn to own 'c', so that if the io_conn closes, + * the client is freed: + * + * master -> c->conn -> c. + */ + tal_steal(c->conn, c); + + /* We put the special zero-db HSM connections into an array, the rest + * go into the map. */ + if (dbid == 0) { + assert(num_dbid_zero_clients < ARRAY_SIZE(dbid_zero_clients)); + dbid_zero_clients[num_dbid_zero_clients++] = c; + } else { + struct client *old_client = uintmap_get(&clients, dbid); + + /* Close conn and free any old client of this dbid. */ + if (old_client) + io_close(old_client->conn); + + if (!uintmap_add(&clients, dbid, c)) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "Failed inserting dbid %"PRIu64, dbid); + tal_add_destructor(c, destroy_client); + } + + return c; +} + /* This is the common pattern for the tail of each handler in this file. */ static struct io_plan *req_reply(struct io_conn *conn, struct client *c, @@ -1641,73 +1703,6 @@ static struct io_plan *handle_client(struct io_conn *conn, struct client *c) return bad_req_fmt(conn, c, c->msg_in, "Unknown request"); } -/*~ This is the destructor on our client: we may call it manually, but - * generally it's called because the io_conn associated with the client is - * closed by the other end. */ -static void destroy_client(struct client *c) -{ - if (!uintmap_del(&clients, c->dbid)) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Failed to remove client dbid %"PRIu64, c->dbid); -} - -static struct client *new_client(struct client *master, - const struct pubkey *id, - u64 dbid, - const u64 capabilities, - int fd) -{ - struct client *c = tal(master, struct client); - - /*~ All-zero pubkey is used for the initial master connection */ - if (id) { - c->id = *id; - } else { - memset(&c->id, 0, sizeof(c->id)); - } - c->dbid = dbid; - - c->master = master; - c->capabilities = capabilities; - /*~ This is the core of ccan/io: the connection creation calls a - * callback which returns the initial plan to execute: in our case, - * read a message.*/ - c->conn = io_new_conn(master, fd, client_read_next, c); - - /*~ tal_steal() moves a pointer to a new parent. At this point, the - * hierarchy is: - * - * master -> c - * master -> c->conn - * - * We want to the c->conn to own 'c', so that if the io_conn closes, - * the client is freed: - * - * master -> c->conn -> c. - */ - tal_steal(c->conn, c); - - /* We put the special zero-db HSM connections into an array, the rest - * go into the map. */ - if (dbid == 0) { - assert(num_dbid_zero_clients < ARRAY_SIZE(dbid_zero_clients)); - dbid_zero_clients[num_dbid_zero_clients++] = c; - } else { - struct client *old_client = uintmap_get(&clients, dbid); - - /* Close conn and free any old client of this dbid. */ - if (old_client) - io_close(old_client->conn); - - if (!uintmap_add(&clients, dbid, c)) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Failed inserting dbid %"PRIu64, dbid); - tal_add_destructor(c, destroy_client); - } - - return c; -} - static void master_gone(struct io_conn *unused UNUSED, struct client *c UNUSED) { daemon_shutdown(); From 5b2e829b4f582375198b2a0035fc16778adc149a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Sep 2018 14:01:27 +0930 Subject: [PATCH 20/20] hsmd: remove master pointer. We used to use it to complain about bad requests, but we use the status conn now, so it's unused except for tests and asserts. Signed-off-by: Rusty Russell --- hsmd/hsmd.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 4027052273d7..d047e8e917b5 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -67,9 +67,6 @@ static struct { /*~ We keep track of clients, but there's not much to keep. */ struct client { - /* The connection to lightningd itself */ - struct client *master; - /* The ccan/io async io connection for this client: it closes, we die. */ struct io_conn *conn; @@ -107,6 +104,12 @@ static size_t num_dbid_zero_clients; /*~ We need this deep inside bad_req_fmt, so we make it a global. */ static struct daemon_conn *status_conn; +/* This is used for various assertions and error cases. */ +static bool is_lightningd(const struct client *client) +{ + return client == dbid_zero_clients[0]; +} + /*~ FIXME: This is used by debug.c. Doesn't apply to us, but lets us link. */ extern void dev_disconnect_init(int fd); void dev_disconnect_init(int fd UNUSED) { } @@ -136,7 +139,7 @@ static PRINTF_FMT(4,5) /*~ If the client was actually lightningd, it's Game Over; we actually * fail in this case, and it will too. */ - if (c == c->master) { + if (is_lightningd(c)) { status_broken("%s", str); master_badmsg(fromwire_peektype(msg_in), msg_in); } @@ -184,13 +187,13 @@ static void destroy_client(struct client *c) "Failed to remove client dbid %"PRIu64, c->dbid); } -static struct client *new_client(struct client *master, +static struct client *new_client(const tal_t *ctx, const struct pubkey *id, u64 dbid, const u64 capabilities, int fd) { - struct client *c = tal(master, struct client); + struct client *c = tal(ctx, struct client); /*~ All-zero pubkey is used for the initial master connection */ if (id) { @@ -200,23 +203,22 @@ static struct client *new_client(struct client *master, } c->dbid = dbid; - c->master = master; c->capabilities = capabilities; /*~ This is the core of ccan/io: the connection creation calls a * callback which returns the initial plan to execute: in our case, * read a message.*/ - c->conn = io_new_conn(master, fd, client_read_next, c); + c->conn = io_new_conn(ctx, fd, client_read_next, c); /*~ tal_steal() moves a pointer to a new parent. At this point, the * hierarchy is: * - * master -> c - * master -> c->conn + * ctx -> c + * ctx -> c->conn * * We want to the c->conn to own 'c', so that if the io_conn closes, * the client is freed: * - * master -> c->conn -> c. + * ctx -> c->conn -> c. */ tal_steal(c->conn, c); @@ -511,8 +513,8 @@ static struct io_plan *init_hsm(struct io_conn *conn, { struct pubkey node_id; - /* This must be the master. */ - assert(c == c->master); + /* This must be lightningd. */ + assert(is_lightningd(c)); /*~ The fromwire_* routines are autogenerated, based on the message * definitions in hsm_client_wire.csv. The format of those files is @@ -1201,7 +1203,7 @@ static struct io_plan *send_pending_client_fd(struct io_conn *conn, { int fd = pending_client_fd; /* This must be the master. */ - assert(master == master->master); + assert(is_lightningd(master)); assert(fd != -1); /* This sanity check shouldn't be necessary, but it's cheap. */ @@ -1229,7 +1231,7 @@ static struct io_plan *pass_client_hsmfd(struct io_conn *conn, struct pubkey id; /* This must be lightningd itself. */ - assert(c == c->master); + assert(is_lightningd(c)); if (!fromwire_hsm_client_hsmfd(msg_in, &id, &dbid, &capabilities)) return bad_req(conn, c, msg_in); @@ -1729,8 +1731,8 @@ int main(int argc, char *argv[]) master = new_client(NULL, NULL, 0, HSM_CAP_MASTER | HSM_CAP_SIGN_GOSSIP, REQ_FD); - /* We're our own master! */ - master->master = master; + /* First client == lightningd. */ + assert(is_lightningd(master)); /* When conn closes, everything is freed. */ io_set_finish(master->conn, master_gone, master);