From 385383d8dedc30b2c8ad247d47d528e3604dc7dc Mon Sep 17 00:00:00 2001 From: Ken Sedgwick Date: Fri, 15 Mar 2024 12:01:43 -0700 Subject: [PATCH 1/6] hsmd: prune unreachable pre HSM_VERSION 5 code Changelog-None: shouldn't affect others HSM_MIN_VERSION is 5 which implies use of WIRE_HSMD_REVOKE_COMMITMENT_TX; prune branches that can't happen. --- channeld/channeld.c | 53 ++++++++++++++++----------------------------- 1 file changed, 19 insertions(+), 34 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index a1a12e104750..a7a45f274d0e 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -1465,23 +1465,17 @@ static u8 *make_revocation_msg(const struct peer *peer, u64 revoke_index, /* Now that the master has persisted the new commitment advance the HSMD * and fetch the revocation secret for the old one. */ - if (!hsm_is_capable(peer->hsm_capabilities, WIRE_HSMD_REVOKE_COMMITMENT_TX)) { - /* Prior to HSM_VERSION 5 we call get_per_commitment_point to - * get the old_secret and next point. - */ - get_per_commitment_point(revoke_index+2, point, &old_commit_secret); - } else { - /* After HSM_VERSION 5 we explicitly revoke the commitment in case - * the original revoke didn't complete. The hsmd_revoke_commitment_tx - * call is idempotent ... - */ - msg = towire_hsmd_revoke_commitment_tx(tmpctx, revoke_index); - msg = hsm_req(tmpctx, take(msg)); - if (!fromwire_hsmd_revoke_commitment_tx_reply(msg, &old_commit_secret, point)) - status_failed(STATUS_FAIL_HSM_IO, - "Reading revoke_commitment_tx reply: %s", - tal_hex(tmpctx, msg)); - } + + /* After HSM_VERSION 5 we explicitly revoke the commitment in case + * the original revoke didn't complete. The hsmd_revoke_commitment_tx + * call is idempotent ... + */ + msg = towire_hsmd_revoke_commitment_tx(tmpctx, revoke_index); + msg = hsm_req(tmpctx, take(msg)); + if (!fromwire_hsmd_revoke_commitment_tx_reply(msg, &old_commit_secret, point)) + status_failed(STATUS_FAIL_HSM_IO, + "Reading revoke_commitment_tx reply: %s", + tal_hex(tmpctx, msg)); return towire_revoke_and_ack(peer, &peer->channel_id, &old_commit_secret, point); @@ -1608,19 +1602,12 @@ static void send_revocation(struct peer *peer, /* Now that the master has persisted the new commitment advance the HSMD * and fetch the revocation secret for the old one. */ - if (!hsm_is_capable(peer->hsm_capabilities, WIRE_HSMD_REVOKE_COMMITMENT_TX)) { - /* Prior to HSM_VERSION 5 we use the old_secret - * received earlier from validate_commitment_tx. */ - old_secret2 = *old_secret; - next_point2 = *next_point; - } else { - msg = towire_hsmd_revoke_commitment_tx(tmpctx, peer->next_index[LOCAL] - 2); - msg = hsm_req(tmpctx, take(msg)); - if (!fromwire_hsmd_revoke_commitment_tx_reply(msg, &old_secret2, &next_point2)) - status_failed(STATUS_FAIL_HSM_IO, - "Reading revoke_commitment_tx reply: %s", - tal_hex(tmpctx, msg)); - } + msg = towire_hsmd_revoke_commitment_tx(tmpctx, peer->next_index[LOCAL] - 2); + msg = hsm_req(tmpctx, take(msg)); + if (!fromwire_hsmd_revoke_commitment_tx_reply(msg, &old_secret2, &next_point2)) + status_failed(STATUS_FAIL_HSM_IO, + "Reading revoke_commitment_tx reply: %s", + tal_hex(tmpctx, msg)); /* Revoke previous commit, get new point. */ msg = make_revocation_msg_from_secret(peer, peer->next_index[LOCAL]-2, @@ -2123,10 +2110,8 @@ static struct commitsig_info *handle_peer_commit_sig(struct peer *peer, tal_steal(commitsigs, result); } - // If the HSM doesn't support WIRE_HSMD_REVOKE_COMMITMENT_TX we'd better - // have the old_secret at this point. - if (!hsm_is_capable(peer->hsm_capabilities, WIRE_HSMD_REVOKE_COMMITMENT_TX)) - assert(old_secret); + /* We no longer receive old_secret from hsmd_validate_commitment_tx */ + assert(!old_secret); send_revocation(peer, &commit_sig, htlc_sigs, changed_htlcs, txs[0], old_secret, &next_point, commitsigs); From fcf7e9f01c9ace214fe59eb56926f89104c7c586 Mon Sep 17 00:00:00 2001 From: Ken Sedgwick Date: Fri, 15 Mar 2024 12:24:13 -0700 Subject: [PATCH 2/6] channeld: factor out unneeded make_revocation_msg_from_secret Changelog-None: internal to channeld Since we don't need a special path for early old_secrets from validate we can factor away duplicate code. --- channeld/channeld.c | 26 ++------------------------ 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index a7a45f274d0e..d955c388d50d 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -1481,17 +1481,6 @@ static u8 *make_revocation_msg(const struct peer *peer, u64 revoke_index, point); } -static u8 *make_revocation_msg_from_secret(const struct peer *peer, - u64 revoke_index, - struct pubkey *point, - const struct secret *old_commit_secret, - const struct pubkey *next_point) -{ - *point = *next_point; - return towire_revoke_and_ack(peer, &peer->channel_id, - old_commit_secret, next_point); -} - /* Convert changed htlcs into parts which lightningd expects. */ static void marshall_htlc_info(const tal_t *ctx, const struct htlc **changed_htlcs, @@ -1558,8 +1547,6 @@ static void send_revocation(struct peer *peer, struct added_htlc *added; const u8 *msg; const u8 *msg_for_master; - struct secret old_secret2; - struct pubkey next_point2; /* Marshall it now before channel_sending_revoke_and_ack changes htlcs */ /* FIXME: Make infrastructure handle state post-revoke_and_ack! */ @@ -1602,17 +1589,8 @@ static void send_revocation(struct peer *peer, /* Now that the master has persisted the new commitment advance the HSMD * and fetch the revocation secret for the old one. */ - msg = towire_hsmd_revoke_commitment_tx(tmpctx, peer->next_index[LOCAL] - 2); - msg = hsm_req(tmpctx, take(msg)); - if (!fromwire_hsmd_revoke_commitment_tx_reply(msg, &old_secret2, &next_point2)) - status_failed(STATUS_FAIL_HSM_IO, - "Reading revoke_commitment_tx reply: %s", - tal_hex(tmpctx, msg)); - - /* Revoke previous commit, get new point. */ - msg = make_revocation_msg_from_secret(peer, peer->next_index[LOCAL]-2, - &peer->next_local_per_commit, - &old_secret2, &next_point2); + msg = make_revocation_msg(peer, peer->next_index[LOCAL]-2, + &peer->next_local_per_commit); /* Now we can finally send revoke_and_ack to peer */ peer_write(peer->pps, take(msg)); From 0aa2a431a4b3295476872e7dd318af825254c039 Mon Sep 17 00:00:00 2001 From: Ken Sedgwick Date: Fri, 15 Mar 2024 13:12:53 -0700 Subject: [PATCH 3/6] channeld: split get_per_commitment_point usesinto separate functions Changelog-None: channeld internal This factoring makes it much clearer which callers need the pubkey and which need the old_secret. It does not alter the unfortunate semantic that fetching future pubkeys beyond the next commit is an error (because the signer will only release secrets for revoked commitments). Using HSM_VERSION 6 will fix this semantic in hsmd. --- channeld/channeld.c | 71 ++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 33 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index d955c388d50d..0d6374a150ae 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -1429,11 +1429,16 @@ static void start_commit_timer(struct peer *peer) send_commit_if_not_stfu, peer); } -/* If old_secret is NULL, we don't care, otherwise it is filled in. */ -static void get_per_commitment_point(u64 index, struct pubkey *point, - struct secret *old_secret) +/* Fetch the requested point. The secret is no longer returned, use + * revoke_commitment. + * + * NOTE - Because the internals of this call also release the secret + * from a revoked commitment it is an error to call this past the next + * commitment. + */ +static void get_per_commitment_point(u64 index, struct pubkey *point) { - struct secret *s; + struct secret *unused; const u8 *msg; msg = hsm_req(tmpctx, @@ -1441,41 +1446,44 @@ static void get_per_commitment_point(u64 index, struct pubkey *point, if (!fromwire_hsmd_get_per_commitment_point_reply(tmpctx, msg, point, - &s)) + &unused)) status_failed(STATUS_FAIL_HSM_IO, "Bad per_commitment_point reply %s", tal_hex(tmpctx, msg)); +} - if (old_secret) { - if (!s) - status_failed(STATUS_FAIL_HSM_IO, - "No secret in per_commitment_point_reply %" - PRIu64, - index); - *old_secret = *s; - } +/* Revoke the specified commitment, the old secret is returned and + * next commitment point are returned. This call is idempotent, it is + * fine to re-revoke a previously revoked commitment. It is an error + * to revoke a commitment beyond the next revocable commitment. + */ +static void revoke_commitment(u64 index, struct secret *old_secret, struct pubkey *point) +{ + const u8 *msg; + + msg = hsm_req(tmpctx, + take(towire_hsmd_revoke_commitment_tx(tmpctx, index))); + + if (!fromwire_hsmd_revoke_commitment_tx_reply(msg, old_secret, point)) + status_failed(STATUS_FAIL_HSM_IO, + "Reading revoke_commitment_tx reply: %s", + tal_hex(tmpctx, msg)); } /* revoke_index == current index - 1 (usually; not for retransmission) */ static u8 *make_revocation_msg(const struct peer *peer, u64 revoke_index, struct pubkey *point) { - const u8 *msg; struct secret old_commit_secret; /* Now that the master has persisted the new commitment advance the HSMD - * and fetch the revocation secret for the old one. */ - - /* After HSM_VERSION 5 we explicitly revoke the commitment in case + * and fetch the revocation secret for the old one. + * + * After HSM_VERSION 5 we explicitly revoke the commitment in case * the original revoke didn't complete. The hsmd_revoke_commitment_tx * call is idempotent ... */ - msg = towire_hsmd_revoke_commitment_tx(tmpctx, revoke_index); - msg = hsm_req(tmpctx, take(msg)); - if (!fromwire_hsmd_revoke_commitment_tx_reply(msg, &old_commit_secret, point)) - status_failed(STATUS_FAIL_HSM_IO, - "Reading revoke_commitment_tx reply: %s", - tal_hex(tmpctx, msg)); + revoke_commitment(revoke_index, &old_commit_secret, point); return towire_revoke_and_ack(peer, &peer->channel_id, &old_commit_secret, point); @@ -2026,7 +2034,7 @@ static struct commitsig_info *handle_peer_commit_sig(struct peer *peer, "error"); } - /* Validate the counterparty's signatures, returns prior per_commitment_secret. */ + /* As of HSM_VERSION 5 returned old_secret is always NULL (revoke returns it instead) */ htlcs = collect_htlcs(NULL, htlc_map); msg2 = towire_hsmd_validate_commitment_tx(NULL, txs[0], @@ -2088,7 +2096,7 @@ static struct commitsig_info *handle_peer_commit_sig(struct peer *peer, tal_steal(commitsigs, result); } - /* We no longer receive old_secret from hsmd_validate_commitment_tx */ + /* After HSM_VERSION 5 old_secret is always NULL */ assert(!old_secret); send_revocation(peer, &commit_sig, htlc_sigs, changed_htlcs, txs[0], @@ -2741,7 +2749,7 @@ static struct commitsig *interactive_send_commitments(struct peer *peer, /* Funding counts as 0th commit so we do inflight_index + 1 */ if (fromwire_peektype(msg) == WIRE_COMMITMENT_SIGNED) { get_per_commitment_point(next_index_local - 1, - &my_current_per_commitment_point, NULL); + &my_current_per_commitment_point); result = handle_peer_commit_sig(peer, msg, inflight_index + 1, @@ -4635,10 +4643,7 @@ static void check_current_dataloss_fields(struct peer *peer, memset(&old_commit_secret, 0, sizeof(old_commit_secret)); else { struct pubkey unused; - /* This gets previous revocation number, since asking for - * commitment point N gives secret for N-2 */ - get_per_commitment_point(next_revocation_number+1, - &unused, &old_commit_secret); + revoke_commitment(next_revocation_number - 1, &old_commit_secret, &unused); } if (!secret_eq_consttime(&old_commit_secret, @@ -4773,7 +4778,7 @@ static void peer_reconnect(struct peer *peer, /* Our current per-commitment point is the commitment point in the last * received signed commitment */ get_per_commitment_point(peer->next_index[LOCAL] - 1, - &my_current_per_commitment_point, NULL); + &my_current_per_commitment_point); send_tlvs = NULL; @@ -5382,7 +5387,7 @@ static void handle_funding_depth(struct peer *peer, const u8 *msg) /* Need to retrieve the first point again, even if we * moved on, as channel_ready explicitly includes the * first one. */ - get_per_commitment_point(1, &point, NULL); + get_per_commitment_point(1, &point); msg = towire_channel_ready(NULL, &peer->channel_id, &point, tlvs); @@ -5969,7 +5974,7 @@ static void init_channel(struct peer *peer) assert(peer->next_index[REMOTE] > 0); get_per_commitment_point(peer->next_index[LOCAL], - &peer->next_local_per_commit, NULL); + &peer->next_local_per_commit); peer->channel = new_full_channel(peer, &peer->channel_id, &funding, From 8eae951d81888b158a3cfc4611b6c874a98d292a Mon Sep 17 00:00:00 2001 From: Ken Sedgwick Date: Fri, 15 Mar 2024 14:16:02 -0700 Subject: [PATCH 4/6] hsmd: make the negotiated hsmd version available to libhsmd Changelog-None: hsmd internals --- hsmd/hsmd.c | 4 ++-- hsmd/libhsmd.c | 3 +++ hsmd/libhsmd.h | 3 +++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 41150da75984..f1a0e163c465 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -490,8 +490,8 @@ static struct io_plan *init_hsm(struct io_conn *conn, discard_key(take(hsm_encryption_key)); /* Define the minimum common max version for the hsmd one */ - u64 mutual_version = maxversion < our_maxversion ? maxversion : our_maxversion; - return req_reply(conn, c, hsmd_init(hsm_secret, mutual_version, + hsmd_mutual_version = maxversion < our_maxversion ? maxversion : our_maxversion; + return req_reply(conn, c, hsmd_init(hsm_secret, hsmd_mutual_version, bip32_key_version)); } diff --git a/hsmd/libhsmd.c b/hsmd/libhsmd.c index 3dfcf183876a..4db3c42c453a 100644 --- a/hsmd/libhsmd.c +++ b/hsmd/libhsmd.c @@ -16,6 +16,9 @@ #include #include +/* The negotiated protocol version ends up in here. */ +u64 hsmd_mutual_version; + /* If they specify --dev-force-privkey it ends up in here. */ struct privkey *dev_force_privkey; /* If they specify --dev-force-bip32-seed it ends up in here. */ diff --git a/hsmd/libhsmd.h b/hsmd/libhsmd.h index 756c6c2f526a..1c7e6698ead1 100644 --- a/hsmd/libhsmd.h +++ b/hsmd/libhsmd.h @@ -89,6 +89,9 @@ void hsmd_status_failed(enum status_failreason code, bool hsmd_check_client_capabilities(struct hsmd_client *client, enum hsmd_wire t); +/* The negotiated protocol version ends up in here. */ +extern u64 hsmd_mutual_version; + /* If they specify --dev-force-privkey it ends up in here. */ extern struct privkey *dev_force_privkey; /* If they specify --dev-force-bip32-seed it ends up in here. */ From b8901ac254c4d536eda9239b76d2e3294c2d0a4a Mon Sep 17 00:00:00 2001 From: Ken Sedgwick Date: Fri, 15 Mar 2024 14:07:59 -0700 Subject: [PATCH 5/6] hsmd: HSM_VERSION 6: get_per_commitment_point never returns secret Changelog-Changed: hsmd: HSM_VERSION 6: get_per_commitment_point does not imply index - 2 is revoked, makes it safe to call on any index. --- channeld/channeld.c | 7 ++----- common/hsm_version.h | 3 ++- hsmd/hsmd_wire.csv | 4 +++- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index 0d6374a150ae..4308b8c508f1 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -1430,11 +1430,8 @@ static void start_commit_timer(struct peer *peer) } /* Fetch the requested point. The secret is no longer returned, use - * revoke_commitment. - * - * NOTE - Because the internals of this call also release the secret - * from a revoked commitment it is an error to call this past the next - * commitment. + * revoke_commitment. It is leagal to call this on any commitment + * (including distant future). */ static void get_per_commitment_point(u64 index, struct pubkey *point) { diff --git a/common/hsm_version.h b/common/hsm_version.h index 7d3f8fb80da2..8464c1a78045 100644 --- a/common/hsm_version.h +++ b/common/hsm_version.h @@ -23,7 +23,8 @@ * v5 with hsmd_revoke_commitment_tx: 5742538f87ef5d5bf55b66dc19e52c8683cfeb1b887d3e64ba530ba9a4d8e638 * v5 with sign_any_cannouncement: 5fdb9068c43a21887dc03f7dce410d2e3eeff6277f0d49b4fc56595a798fd4a4 * v5 drop init v2: 5024454532fe5a78bb7558000cb344190888b9915360d3d56ddca22eaba9b872 + * v6 no secret from get_per_commitment_point: 5024454532fe5a78bb7558000cb344190888b9915360d3d56ddca22eaba9b872 */ #define HSM_MIN_VERSION 5 -#define HSM_MAX_VERSION 5 +#define HSM_MAX_VERSION 6 #endif /* LIGHTNING_COMMON_HSM_VERSION_H */ diff --git a/hsmd/hsmd_wire.csv b/hsmd/hsmd_wire.csv index cd75835622c3..6ad2ae0add0a 100644 --- a/hsmd/hsmd_wire.csv +++ b/hsmd/hsmd_wire.csv @@ -294,10 +294,12 @@ msgdata,hsmd_sign_splice_tx,input_index,u32, msgtype,hsmd_sign_tx_reply,112 msgdata,hsmd_sign_tx_reply,sig,bitcoin_signature, -# Openingd/channeld/onchaind asks for Nth per_commitment_point, if > 2, gets N-2 secret. +# Openingd/channeld/onchaind asks for Nth per_commitment_point +# Prior to HSM_VERSION 6 we will return an old_commitment_secret msgtype,hsmd_get_per_commitment_point,18 msgdata,hsmd_get_per_commitment_point,n,u64, +# IMPORTANT - Beginning HSM_VERSION 6 we never return an old_commitment_secret msgtype,hsmd_get_per_commitment_point_reply,118 msgdata,hsmd_get_per_commitment_point_reply,per_commitment_point,pubkey, msgdata,hsmd_get_per_commitment_point_reply,old_commitment_secret,?secret, From 3005799fb89e4fa40e79ecbd9b8822ba6622cd27 Mon Sep 17 00:00:00 2001 From: Ken Sedgwick Date: Fri, 15 Mar 2024 14:24:02 -0700 Subject: [PATCH 6/6] hsmd: support HSM_VERSION 6 Changelog-Changed: hsmd: the hsmd now supports HSM_VERSION 6 This is actually optional, everything should be ok leaving native hsmd support at HSM_VERSION 5. --- hsmd/hsmd.c | 2 +- hsmd/libhsmd.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index f1a0e163c465..d732d93e3ba5 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -436,7 +436,7 @@ static struct io_plan *init_hsm(struct io_conn *conn, struct secret *hsm_encryption_key; struct bip32_key_version bip32_key_version; u32 minversion, maxversion; - const u32 our_minversion = 4, our_maxversion = 5; + const u32 our_minversion = 4, our_maxversion = 6; /* This must be lightningd. */ assert(is_lightningd(c)); diff --git a/hsmd/libhsmd.c b/hsmd/libhsmd.c index 4db3c42c453a..ac79380dd45c 100644 --- a/hsmd/libhsmd.c +++ b/hsmd/libhsmd.c @@ -1188,7 +1188,7 @@ static u8 *handle_get_per_commitment_point(struct hsmd_client *c, const u8 *msg_ return hsmd_status_bad_request_fmt( c, msg_in, "bad per_commit_point %" PRIu64, n); - if (n >= 2) { + if (hsmd_mutual_version < 6 && n >= 2) { old_secret = tal(tmpctx, struct secret); if (!per_commit_secret(&shaseed, old_secret, n - 2)) { return hsmd_status_bad_request_fmt(