Extra address checks, with new HSM support #5708

rustyrussell · 2022-11-11T04:39:09Z

This is built on top of #5986 (first 9 commits)
Closes: #5695

cdecker · 2022-11-11T08:17:20Z

Ugh, more hsm_wire.csv breaking. Let's not create all those variants of the init message, let's rather move the HSM version up to in the init, and then use that to differentiate. Replicating the init message is just weird, and not really helping all that much. It's better to bundle changes so we break less often, but at least we do it cleanly and the hsmd can detect it based on the version in the init message.

cdecker

Replicating the init_reply for each type is weird, a simple version number in the actual init_reply followed by optional fields (TLV?) is better backwards compatible imho.

Also longer term we should try to bundle changes to the hsm wire protocol better.

cdecker · 2022-11-11T09:36:14Z

Thinking about this a bit more, I think we have two options to make the schema evolution for the hsmd more practical:

Move the hsmd_version to the first position in the init_reply message, allowing us to peek at it before decoding, and then allowing extensions to the message to be made at the end. This allows backwards compatibility for the case when hsmd_version >= node_version, since the fromwire_ methods can underread the message, ignoring trailing fields.
- One choice to be made is which part should detect and abort on version incompatibilities: the node can set a max version, however that defeats the signer backward compatibility. Alternatively the signer can abort if the node version can't be satisfied (which is in line with the signer being backward compatible)
The other option is to go with TLVs, allowing us to omit fields that have been removed from the schema without having to introduce an incompatibility because the node can no longer read the message because of missing positional fields.

I guess it boils down to whether we expect the signer or the node to be ahead, and whether we care about pruning old fields from the message.

As a quick side-note on why I don't like changing the numeric type of init_reply: when attempting to start the node without the signer present (remote signer setup), we need to cache the init_reply which is static anyway, in order to be able to start at all. Changing the type means we now need additional logic to recognize a new type and cache that instead, and on upgrades we need to bump the signer and node version in lock-step.

niftynei · 2022-11-14T16:58:08Z

Patch lgtm, utACK.

But I did have a question about the goals of API support. Mainly, how long is one's HSMD impl being out of date supported for? If a user has an HSM with a terribly outdated version from the current CLN version they're running, will it fail to start or is there supposed to be a "limp along"/backwards compatible level of functionality that is maintained?

Just trying to get a better understanding of the design goals re HSM API here.

rustyrussell · 2022-11-14T23:03:59Z

Some of this is a limitation of our wire format. It's simplest and most robust to have separate messages, but we can actually make the v3 a variant on v2 with an extra field appended since we have option logic for that (which off the top of my head, isn't used anywhere else anymore, so I would have liked to remove it, but ok).

Using TLVs here unconstrains the test matrix and is probably a bad idea. The multiple init responses is ugly, but simple. Putting the version first is a good idea, in the hope that all messages can be based on this in future.

rustyrussell · 2023-02-09T02:49:50Z

OK, I've completely reworked the HSM init revision.

@ksedgwic what do you think? We now have a version in the reply, plus capabilities which we use here to try to allow you to decouple from hsm changes (at least in cases where we can make new messages optional).

ksedgwic · 2023-02-14T02:21:54Z

Looks sensible to me, concept ACK
nit: a comment explaining the implicit v3 semantics might be helpful.

A capability which seems to be coming up is option_anchors_zero_fee_htlc_tx support; VLS would like to disallow plain anchor commitments ...
@devrandom

devrandom

utACK on the versioning scheme. a couple of small questions on the specific feature added here.

devrandom · 2023-02-17T10:27:00Z

hsmd/hsmd_wire.csv

+msgdata,hsmd_check_pubkey,index,u32,
+msgdata,hsmd_check_pubkey,pubkey,pubkey,
+
+# Reply (if it's bad, it simple aborts.)


what does this mean exactly? closes the connection? it would be difficult to distinguish from a real disconnect due to restart.

OK, this works for a local hsmd, not for remote. Will make it an explicit reply!

Thanks!

devrandom · 2023-02-17T10:28:51Z

hsmd/hsmd_wire.csv

+
+# Sanity check this pubkey derivation is correct.
+msgtype,hsmd_check_pubkey,28
+msgdata,hsmd_check_pubkey,index,u32,


are hardened indices allowed (high bit = 1)?

we plan to deny very high indices, e.g. > 1_000_000, to make recovery sane in case of database loss. is that OK?

No, we don't use hardened derivations. And the 1M keys limit seems like a sane precaution: if someone exceeds that they should tell us :)

devrandom · 2023-02-17T10:34:22Z

BTW, note that we check L1 destinations at the time of SignWithdrawal anyway. I assume that we are passed the derivation index at that time in the PSBT (@ksedgwic ?).

ksedgwic · 2023-02-17T15:40:54Z

Yes, Wallet::can_spend confirms that the index generates the correct script_pubkey

rustyrussell · 2023-03-07T00:43:22Z

Folded in fix which makes hsm explicitly reply, and fixed up hsm_versions.h definition.

done!

rustyrussell · 2023-03-20T00:46:36Z

Trivial rebase on top of master now #5986 is merged.

Ack a3a720c

We were handing 3 to hsmd (and Ken added that in 7b2c561, so I guess he's OK with that being the minimum supported version!). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Importantly, adds the version number at the *front* to help future parsing. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Header from folded patch 'fix-hsm-check-pubkey.patch': fixup! hsmd: capability addition: ability to check pubkeys.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

It's needed as the db and wallet is being set up (db migrations), so it's simpler this way to always use ld->bip32_base for the next patch. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

At the moment only lightingd needs it, and this avoids missing any places where we do bip32 derivation. This uses a hsm capability to mean we're backwards compatible with older hsmds. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Added: Protocol: we now always double-check bitcoin addresses are correct (no memory errors!) before issuing them.

`struct lightningd` is not completely initialized, so we added a "migration_context" which only had some of the fields. But we ended up handing in `struct lightningd` anyway, so I don't think this complexity is worthwhile. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2023-03-22T03:20:24Z

Ack cf04060

rustyrussell added this to the v23.02 milestone Nov 11, 2022

rustyrussell requested a review from cdecker as a code owner November 11, 2022 04:39

cdecker previously requested changes Nov 11, 2022

View reviewed changes

rustyrussell modified the milestones: v23.02, v23.05 Feb 6, 2023

rustyrussell force-pushed the guilt/extra-address-checks branch from a281242 to c77ff47 Compare February 9, 2023 03:14

rustyrussell requested a review from cdecker February 9, 2023 03:14

rustyrussell force-pushed the guilt/extra-address-checks branch from c77ff47 to 14912fd Compare February 9, 2023 03:16

rustyrussell changed the title ~~Extra address checks~~ Extra address checks, with new HSM support Feb 9, 2023

rustyrussell added the hsmd label Feb 9, 2023

rustyrussell force-pushed the guilt/extra-address-checks branch from 14912fd to 9019586 Compare February 9, 2023 03:17

devrandom reviewed Feb 17, 2023

View reviewed changes

rustyrussell force-pushed the guilt/extra-address-checks branch 2 times, most recently from be91509 to a170d37 Compare March 7, 2023 00:43

rustyrussell mentioned this pull request Mar 7, 2023

Zero fee htlc preparation: Part 1 #6073

Merged

rustyrussell force-pushed the guilt/extra-address-checks branch 2 times, most recently from 160e966 to 0bda5ad Compare March 8, 2023 04:35

rustyrussell force-pushed the guilt/extra-address-checks branch from 0bda5ad to a3a720c Compare March 20, 2023 00:44

common: update HSM_MIN_VERSION to reflect reality.

1dc0383

We were handing 3 to hsmd (and Ken added that in 7b2c561, so I guess he's OK with that being the minimum supported version!). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell added 5 commits March 21, 2023 14:28

hsmd: capability addition: ability to check pubkeys.

c4a0c3a

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: move bip32_base pointer into struct lightningd.

92c2d7c

It's needed as the db and wallet is being set up (db migrations), so it's simpler this way to always use ld->bip32_base for the next patch. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/extra-address-checks branch 2 times, most recently from defa756 to cf04060 Compare March 21, 2023 04:22

rustyrussell merged commit df085a8 into ElementsProject:master Mar 22, 2023

cdecker deleted the guilt/extra-address-checks branch March 22, 2023 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extra address checks, with new HSM support #5708

Extra address checks, with new HSM support #5708

rustyrussell commented Nov 11, 2022 •

edited

cdecker commented Nov 11, 2022

cdecker left a comment •

edited

cdecker commented Nov 11, 2022

niftynei commented Nov 14, 2022

rustyrussell commented Nov 14, 2022

rustyrussell commented Feb 9, 2023

ksedgwic commented Feb 14, 2023

devrandom left a comment

devrandom Feb 17, 2023

rustyrussell Mar 3, 2023

devrandom Feb 17, 2023

rustyrussell Mar 3, 2023

devrandom commented Feb 17, 2023

ksedgwic commented Feb 17, 2023

rustyrussell commented Mar 7, 2023

rustyrussell commented Mar 20, 2023

rustyrussell commented Mar 22, 2023

Extra address checks, with new HSM support #5708

Extra address checks, with new HSM support #5708

Conversation

rustyrussell commented Nov 11, 2022 • edited

cdecker commented Nov 11, 2022

cdecker left a comment • edited

Choose a reason for hiding this comment

cdecker commented Nov 11, 2022

niftynei commented Nov 14, 2022

rustyrussell commented Nov 14, 2022

rustyrussell commented Feb 9, 2023

ksedgwic commented Feb 14, 2023

devrandom left a comment

Choose a reason for hiding this comment

devrandom Feb 17, 2023

Choose a reason for hiding this comment

rustyrussell Mar 3, 2023

Choose a reason for hiding this comment

devrandom Feb 17, 2023

Choose a reason for hiding this comment

rustyrussell Mar 3, 2023

Choose a reason for hiding this comment

devrandom commented Feb 17, 2023

ksedgwic commented Feb 17, 2023

rustyrussell commented Mar 7, 2023

rustyrussell commented Mar 20, 2023

rustyrussell commented Mar 22, 2023

rustyrussell commented Nov 11, 2022 •

edited

cdecker left a comment •

edited