Add a new method `read_chan_signer` to `KeysInterface` #761

TheBlueMatt · 2020-11-25T21:43:29Z

This adds a new method to the general cross-channel KeysInterface
which requires it to handle the deserialization of per-channel
signer objects. This allows the deserialization of per-channel
signers to have more context available, which, in the case of the
C bindings, includes the actual KeysInterface information itself.

I have bindings updates to go with this as well, but will run them as a separate PR. Those bindings updates (and this) are required to get ChannelMonitor deserialization working in bindings, so tagging 0.0.13.

codecov · 2020-11-25T23:07:08Z

Codecov Report

Merging #761 (ec4c44b) into main (9c9c881) will decrease coverage by 0.01%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##             main     #761      +/-   ##
==========================================
- Coverage   91.29%   91.28%   -0.02%     
==========================================
  Files          37       37              
  Lines       22791    22836      +45     
==========================================
+ Hits        20808    20846      +38     
- Misses       1983     1990       +7

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`93.33% <0.00%> (-0.86%)`	⬇️
lightning/src/lib.rs	`100.00% <ø> (ø)`
lightning/src/util/test_utils.rs	`83.68% <66.66%> (-1.16%)`	⬇️
lightning/src/ln/channel.rs	`87.53% <93.75%> (+0.02%)`	⬆️
lightning/src/chain/channelmonitor.rs	`95.67% <100.00%> (+0.09%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.56% <100.00%> (ø)`
lightning/src/ln/channelmanager.rs	`84.94% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`95.13% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`96.96% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/onchaintx.rs	`93.92% <100.00%> (+0.14%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c9c881...990d1de. Read the comment docs.

devrandom · 2020-11-26T09:54:06Z

ChannelManager::read still seems to read the Channel and therefore the signer, directly, bypassing read_chan_signer.

devrandom · 2020-11-26T10:18:54Z

What's the intention behind the throwaway OnlyReadsKeysInterface {}? lightning-signer#1 would benefit from having the real one during the read...

TheBlueMatt · 2020-12-01T21:58:51Z

What's the intention behind the throwaway OnlyReadsKeysInterface {}?

It exists purely to be dummy KeysInterface for when we don't have the real KeysInterface handy during deserialization. Honestly, its mostly me being a bit lazy in tests which don't have a KeysInterface readily available and not wanting to bother reconstructing them. In lightning-signer, using it would likely indicate not bothering to enforce anything on the deserialized version, which seems wrong?

devrandom · 2020-12-02T18:09:04Z

I threaded TestKeysInterface in most of the remaining places in commit lightning-signer@66cf607. As a result, lightning-signer#1 now passes except for two tests.

I'll look into the two remaining issues.

devrandom · 2020-12-05T12:00:46Z

Got the two tests working and opened a PR in this repo - #764.

ariard

Pretty straightforward or do you think there are points which need more discussion?

ariard · 2020-12-09T22:12:51Z

lightning/src/chain/keysinterface.rs

+	/// Reads a `ChanKeySigner` for this `KeysInterface` from the given input stream.
+	/// This is only called during deserialization of other objects which contain
+	/// `ChannelKeys`-implementing objects (ie `ChannelMonitor`s and `ChannelManager`s) and must
+	/// read exactly the bytes that `<Self::ChanKeySigner as Writeable>::write()` writes, or return


What if you upgrade/downgrade KeysInterface between r/w ? An older implementation may discard some data chunks written by a newer implementation if version is done correctly but I think your requirement scope this ?

Maybe say instead "should validate that the length of the data is as expected".

I added a note about versioning and extra bytes.

ariard · 2020-12-09T22:44:13Z

lightning/src/chain/channelmonitor.rs

@@ -626,7 +632,8 @@ pub struct ChannelMonitor<ChanSigner: ChannelKeys> {
 	counterparty_payment_script: Script,
 	shutdown_script: Script,

-	keys: ChanSigner,
+	key_derivation_params: (u64, u64),


Wouldn't be better in a future move to replace those two values by a ChannelKeys identifier and store them behind the interface to never expose them in-memory ? Those values are consumed in ChannelKeys derivation and might it could be better to keep them private ?

Anyway it would be great to add a one-line comment about their purpose in ChannelMonitor to serve at onchain output description, without constraining the utxo signer to store ChannelKeys.

I'm not exactly sure what you're asking - are you suggesting we rename them "channel_keys_id" or similar? While our default channel keys stores actual key derivation info in them, that's certainly not a requirement, nor should key derivation information generally be particularly sensitive.

With our current KeysManager::derive_channel_keys, those two values are inputs to the commitment seed, which is used to derive per-channel private keys. Theoretically I think an attacker able to observe them in system memory will have an easier task to re-compute the commitment_seed. In practice, we're already aware of this limitation as comment in keysinterface.rs (L754) underscores it.

If we want to move from this situation, we can have the KeysManager provide an arbitrary identifier number to the derive_channel_keys caller. This identifier could be stored by ChannelMonitor and pass back in any SpendableOutputDescriptor. That way, you can have a key holder implementation on an external signing device, it consumes a SpendableOutputDescriptor, derives the channel keys and gets back the onchain output, without having ever exposed the derivation params.

I think that's a side-point to the current proposed change, so not a blocker.

These values are an arbitrary identifier. They happen to be 128 bits in size so that keys interface implementors can use them as cryptographic values if they want, but there's nothing stopping them from being just an ID. The documentation on it can be better, indeed, but I think the API is already fine.

We only actually use two of the fields in ChannelKeys inside a ChannelMonitor - the holder revocation_basepoint and the derivation parameters. Both are relatively small, so there isn't a lot of reason to hold a full copy of the ChannelKeys (with most of the interaction with it being inside the OnchainTxHandler). Further, this will avoid calling read on a `ChannelKeys` twice, which is a somewhat strange API quirk.

It doesn't make sense to ever build a lightning node which doesn't ever write ChannelMonitors to disk, so having a ChannelKeys object which doesn't implement Writeable is nonsense. Here we require Writeable for all ChannelKeys objects, simplifying code generation for C bindings somewhat.

There's no reason to have ChannelMonitor::write_for_disk instead of just using the Writeable trait anymore. Previously, it was used to differentiate with `write_for_watchtower`, but support for watchtower-mode ChannelMonitors was never completed and the partial bits were removed long ago. This has the nice benefit of hitting the custom Writeable codepaths in C bindings instead of trying to hit trait-generics paths.

This adds a new method to the general cross-channel `KeysInterface` which requires it to handle the deserialization of per-channel signer objects. This allows the deserialization of per-channel signers to have more context available, which, in the case of the C bindings, includes the actual KeysInterface information itself.

This drops any direct calls to a generic `ChannelKeys::read()` and replaces it with the new `KeysInterface::read_chan_signer()`. Still, under the hood all of our own `KeysInterface::read_chan_signer()` implementations simply call out to a `Readable::read()` implemention.

ariard

Code Review ACK 990d1de.

ariard · 2021-01-08T16:30:17Z

lightning/src/ln/channel.rs

@@ -4237,8 +4243,10 @@ impl<ChanSigner: ChannelKeys> Writeable for Channel<ChanSigner> {
 	}
 }

-impl<ChanSigner: ChannelKeys + Readable> Readable for Channel<ChanSigner> {
-	fn read<R : ::std::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+const MAX_ALLOC_SIZE: usize = 64*1024;


I know 64 kB is used across the codebase but what's the rational picking up this value ? Is it a upper bound promise made by rustc default memory allocator or an assumption on the underlying system ?

Its random, basically. Its a nice round number that is small enough that its hard to add up enough 64KB chunks to cause damage but big enough that we basically never hit it. Anything a few x higher or lower would probably work just as well.

TheBlueMatt · 2021-01-08T16:59:16Z

Gonna go ahead and merge cause there was no objection and there's a lot of work to be done on top.

TheBlueMatt added this to the 0.0.13 milestone Nov 25, 2020

TheBlueMatt force-pushed the 2020-10-chansigner-no-ser branch 2 times, most recently from bf09abd to a1aca64 Compare November 25, 2020 22:56

arik-so mentioned this pull request Nov 30, 2020

Meeting Minutes #717

Closed

TheBlueMatt force-pushed the 2020-10-chansigner-no-ser branch from a1aca64 to c9be345 Compare December 1, 2020 21:55

TheBlueMatt force-pushed the 2020-10-chansigner-no-ser branch from c9be345 to 03cb581 Compare December 1, 2020 22:00

devrandom mentioned this pull request Dec 5, 2020

Revocation enforcement #764

Merged

ariard reviewed Dec 9, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-10-chansigner-no-ser branch from 03cb581 to 2a7d5d1 Compare December 12, 2020 20:48

TheBlueMatt force-pushed the 2020-10-chansigner-no-ser branch from 2a7d5d1 to 811f548 Compare December 31, 2020 22:56

This was referenced Dec 31, 2020

Handle read_chan_signer in bindings (761 bindings updates) #767

Merged

Expose ChannelManager/Monitor read methods in bindings #768

Merged

TheBlueMatt added 6 commits January 4, 2021 12:37

Expose test_utils to fuzztarget (in addition to _test_utils feature)

c07b4de

TheBlueMatt force-pushed the 2020-10-chansigner-no-ser branch from 811f548 to 990d1de Compare January 4, 2021 17:40

ariard reviewed Jan 8, 2021

View reviewed changes

TheBlueMatt merged commit b2f1327 into lightningdevkit:main Jan 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new method `read_chan_signer` to `KeysInterface` #761

Add a new method `read_chan_signer` to `KeysInterface` #761

TheBlueMatt commented Nov 25, 2020

codecov bot commented Nov 25, 2020 •

edited

Loading

devrandom commented Nov 26, 2020

devrandom commented Nov 26, 2020

TheBlueMatt commented Dec 1, 2020

devrandom commented Dec 2, 2020 •

edited

Loading

devrandom commented Dec 5, 2020

ariard left a comment

ariard Dec 9, 2020

devrandom Dec 10, 2020

TheBlueMatt Dec 12, 2020

ariard Dec 9, 2020

TheBlueMatt Dec 11, 2020

ariard Jan 8, 2021

TheBlueMatt Jan 8, 2021

ariard left a comment

ariard Jan 8, 2021

TheBlueMatt Jan 8, 2021

TheBlueMatt commented Jan 8, 2021

Add a new method read_chan_signer to KeysInterface #761

Add a new method read_chan_signer to KeysInterface #761

Conversation

TheBlueMatt commented Nov 25, 2020

codecov bot commented Nov 25, 2020 • edited Loading

Codecov Report

devrandom commented Nov 26, 2020

devrandom commented Nov 26, 2020

TheBlueMatt commented Dec 1, 2020

devrandom commented Dec 2, 2020 • edited Loading

devrandom commented Dec 5, 2020

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jan 8, 2021

Add a new method `read_chan_signer` to `KeysInterface` #761

Add a new method `read_chan_signer` to `KeysInterface` #761

codecov bot commented Nov 25, 2020 •

edited

Loading

devrandom commented Dec 2, 2020 •

edited

Loading