Introduce EnforcementState, validate release of revocation secret #1039

devrandom · 2021-08-09T08:58:49Z

This helps the signer implement the following policies (from https://gitlab.com/lightning-signer/docs/-/blob/master/policy-controls.md)

Ensure that we have a validated counterparty-signed holder commitment we can broadcast before revoking a commitment. (policies policy-v2-revoke-new-commitment-signed and policy-v2-revoke-new-commitment-valid)
Ensure we sign a counterparty commitment only after a revocation - there can be a maximum of two counterparty broadcastable commitments at a time (policy-v2-commitment-previous-revoked)

codecov · 2021-08-09T09:15:45Z

Codecov Report

Merging #1039 (608ed12) into main (4368b56) will decrease coverage by 0.02%.
The diff coverage is 84.00%.

@@            Coverage Diff             @@
##             main    #1039      +/-   ##
==========================================
- Coverage   90.82%   90.80%   -0.03%     
==========================================
  Files          65       65              
  Lines       32890    32917      +27     
==========================================
+ Hits        29872    29889      +17     
- Misses       3018     3028      +10

Impacted Files	Coverage Δ
lightning/src/ln/msgs.rs	`89.10% <ø> (ø)`
lightning/src/chain/keysinterface.rs	`94.68% <25.00%> (-0.68%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.28% <80.55%> (-1.10%)`	⬇️
lightning/src/util/test_utils.rs	`82.33% <89.47%> (+0.04%)`	⬆️
lightning/src/ln/channel.rs	`88.73% <100.00%> (+0.03%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.41% <100.00%> (-0.09%)`	⬇️

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 4368b56...608ed12. Read the comment docs.

TheBlueMatt · 2021-08-09T15:14:11Z

I'm a bit confused by the security model here is - its kinda cool to have the signer receive proof that the latest counterparty commitment transaction exists, but it presumably can't broadcast it by itself (mostly cause the signer doesn't have external network access). If a tree falls in the forest....

devrandom · 2021-08-09T15:51:59Z

If the node is compromised, but the signer is intact, then the signer can be attached to a clean machine. The clean machine would have software that can broadcast the saved holder transaction. This would be a disaster recovery scenario.

TheBlueMatt · 2021-08-10T01:42:46Z

Won't our watchtower take care of the enforcement?
We'll need to see a signed ack from the watchtower before proceeding ...

Yep, this.

This would be a disaster recovery scenario.

Also this.

I'm fine with this as a reasonable belt-and-suspenders, even if its not exactly within the trust model we have to assume. Sadly needs rebase cause of large things landing for 0.0.100.

devrandom · 2021-08-10T07:32:56Z

Updated PR description to call out the policy checks this would support in the signer.

Also rebased.

devrandom · 2021-08-10T07:48:30Z

Won't our watchtower take care of the enforcement?
We'll need to see a signed ack from the watchtower before proceeding ...

Yep, this.

To be clear, I assume we are talking about policy-v2-revoke-new-commitment-* (see updated PR description).

I don't think the watchtower helps for all attacks related to this - it just helps with breaches.

If the signer doesn't implement these policies, and allows revocation of the only valid holder tx, then we can no longer force-close. If also the balance is mostly on our side, and the attacker controls the counterparty, then they can just withhold the funds indefinitely or ask for a ransom.

So we have to ensure that we have a valid holder tx at all times. And we have to connect the signer to a clean fallback node, or have a high-availability signer setup and publish that holder tx to mitigate this scenario.

lightning/src/util/enforcing_trait_impls.rs

valentinewallace

I have a few questions/suggestions but basically ACK

lightning/src/util/enforcing_trait_impls.rs

lightning/src/ln/msgs.rs

valentinewallace

LGTM given Matt's brief that EnforcingSigner is pretty much just for testing

lightning/src/util/enforcing_trait_impls.rs

TheBlueMatt

A few easy fixes but I think this is good.

TheBlueMatt · 2021-08-19T18:20:06Z

lightning/src/util/enforcing_trait_impls.rs

+	fn validate_holder_commitment(&self, holder_tx: &HolderCommitmentTransaction) {
+		let mut state = self.state.lock().unwrap();
+		let idx = holder_tx.commitment_number();
+		assert!(idx == state.last_holder_commitment || idx == state.last_holder_commitment - 1, "expecting to validate the current or next holder commitment - trying {}, current {}", idx, state.last_holder_commitment);


You can drop the == check and only keep the -1 check here (and make it an assert_eq!()).

In general, it seems advisable to allow replay of the last action, in case the signer is remote and the connection dropped during the call. However, could remove it here if we don't really care about illustrating that considering that this is mostly for internal tests.

lightning/src/ln/channel.rs

lightning/src/chain/keysinterface.rs

lightning/src/ln/channel.rs

valentinewallace · 2021-08-23T20:44:27Z

lightning/src/ln/channel.rs

+		self.holder_signer.validate_counterparty_revocation(
+			self.cur_counterparty_commitment_transaction_number + 1,
+			&secret
+		).map_err(|_| ChannelError::Close("Failed to validate your revocation".to_owned()))?;


s/your/counterparty

I'm not sure about this one. This is a message we are sending to the counterparty, so wouldn't it be confusing to them that we are referring to them as "counterparty" instead of "you"?

Actually, elsewhere the term "Peer" is used, so I'll use that.

as we add more enforcement state variables, we want to keep track of them under a single structure

…tment

…gSigner in tests

…party_commitment

devrandom force-pushed the 2021-08-more-enforcement branch from f7f300e to 3f1c6bb Compare August 9, 2021 10:18

devrandom changed the title ~~Draft: Introduce EnforcementState, validate releasing revocation secret~~ Draft: Introduce EnforcementState, validate release of revocation secret Aug 9, 2021

devrandom changed the title ~~Draft: Introduce EnforcementState, validate release of revocation secret~~ Introduce EnforcementState, validate release of revocation secret Aug 9, 2021

devrandom marked this pull request as ready for review August 9, 2021 10:21

devrandom force-pushed the 2021-08-more-enforcement branch from 3f1c6bb to 4525790 Compare August 9, 2021 15:07

devrandom force-pushed the 2021-08-more-enforcement branch from 4525790 to f738350 Compare August 10, 2021 07:27

TheBlueMatt reviewed Aug 12, 2021

View reviewed changes

lightning/src/util/enforcing_trait_impls.rs Outdated Show resolved Hide resolved

lightning/src/util/enforcing_trait_impls.rs Outdated Show resolved Hide resolved

devrandom force-pushed the 2021-08-more-enforcement branch 4 times, most recently from 7698988 to e7ab8c8 Compare August 17, 2021 11:23

valentinewallace reviewed Aug 18, 2021

View reviewed changes

devrandom force-pushed the 2021-08-more-enforcement branch 2 times, most recently from f74bb3a to f1b556a Compare August 19, 2021 07:47

valentinewallace approved these changes Aug 19, 2021

View reviewed changes

lightning/src/util/enforcing_trait_impls.rs Outdated Show resolved Hide resolved

lightning/src/util/enforcing_trait_impls.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Aug 19, 2021

View reviewed changes

devrandom force-pushed the 2021-08-more-enforcement branch from e1e69e0 to ba96aa8 Compare August 20, 2021 13:15

TheBlueMatt approved these changes Aug 20, 2021

View reviewed changes

valentinewallace approved these changes Aug 23, 2021

View reviewed changes

devrandom added 5 commits August 28, 2021 11:01

Introduce EnforcementState for EnforcingSigner

50966e2

as we add more enforcement state variables, we want to keep track of them under a single structure

Enforce that revocation can only occur after we validated a new commi…

241e448

…tment

Enforce signing counterparty commitment only after revocation

285b3fa

Always use KeysInterface.read_chan_signer for de-serializing Enforcin…

181d319

…gSigner in tests

Improve documentation for validate_holder_commitment and sign_counter…

a016bb1

…party_commitment

Allow BaseSign validation functions to return an Err

608ed12

devrandom force-pushed the 2021-08-more-enforcement branch from 152d8a6 to 608ed12 Compare August 28, 2021 09:04

TheBlueMatt merged commit 731773c into lightningdevkit:main Aug 30, 2021

Introduce EnforcementState, validate release of revocation secret #1039

Introduce EnforcementState, validate release of revocation secret #1039

Uh oh!

Conversation

devrandom commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt commented Aug 9, 2021

Uh oh!

devrandom commented Aug 9, 2021

Uh oh!

TheBlueMatt commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devrandom commented Aug 10, 2021

Uh oh!

devrandom commented Aug 10, 2021

Uh oh!

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

devrandom Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

devrandom Aug 28, 2021

Choose a reason for hiding this comment

Uh oh!

devrandom Aug 28, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

devrandom commented Aug 9, 2021 •

edited

Loading

codecov bot commented Aug 9, 2021 •

edited

Loading

TheBlueMatt commented Aug 10, 2021 •

edited

Loading