ChannelKeys - separate commitment revocation from getting the per-commitment point #655

devrandom · 2020-07-11T10:26:28Z

The commitment secret is sensitive - it can be used by an attacker to steal funds if the node also signs the same transaction. In general, the signer should be considered more secure - it will, for example, be implemented in secure hardware. It should not give the rest of the node access to data which can be used by an attacker and it should enforce policy which prevents attacks.

valentinewallace

Nice, seems a bit cleaner than before 👍 Was there a specific motivation for this change?

lightning/src/chain/keysinterface.rs

TheBlueMatt

Cool! Val pointed out a good docs change, I think, but would be cool to do enforcement as well, no pressure if you don't want to do that right now, though.

lightning/src/chain/keysinterface.rs

lightning/src/util/enforcing_trait_impls.rs

devrandom · 2020-07-12T16:09:23Z

@valentinewallace the motivation is that the commitment secret is sensitive - it can be used by an attacker to steal funds if the node also signs the same transaction. In general, the signer should be considered more secure - it will, for example, be implemented in secure hardware. It should not give the rest of the node access to data which can be used by an attacker and it should enforce policy which prevents attacks.

In this particular case, some of the policies include: revoke commitments in order, don't sign a revoked transaction, don't revoke a signed transaction. In the third commit I implemented the first ~~two policies~~ policy.

ariard

Code Review ACK 0c98489, that's a good step forward, in case of node compromise, it will prevent to yell a revocation secret to a an attacker already owning a signed commitment and thus freely spendable with counterparty's revocation secret.

ariard · 2020-07-12T23:30:55Z

lightning/src/chain/keysinterface.rs

@@ -217,6 +225,7 @@ pub trait ChannelKeys : Send+Clone {
 	/// Create a signature for a local commitment transaction. This will only ever be called with
 	/// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees
 	/// that it will not be called multiple times.
+	/// The commitment must not have been revoked.


Did you say this policy enforcement has been implemented, namely "don't sign a revoked transaction" ? I think you need to verify the new revoked_commitment against commitment transaction number. I guess you can access it by unveiling it from LocalCommitmentTransaction 's unsigned_tx. This can be addressed later.

Oops, I misspoke above. I thought about implementing this, but didn't get to it.

Maybe since it's not implemented yet, rephrase the line to be a TODO?

Agreed, but again, I think it should be attached to EnforcingChannelKeys

I believe all of our other comments are phrased as "what rust-lightning guarantees it will do" ie, instead of "The commitment must not have been revoked" this could read "The commitment transaction contained will not have been revoked by release_commitment_secret, and external signers may wish to enforce this".

reworded, please take a look

jkczyz · 2020-07-13T14:57:25Z

Thanks for updating the PR description with the motivation. Could you also include this in the commit message as per the contributing guidelines? This would be useful for those looking through the commit log.

Generally, I see the PR description used for the broader motivation for a set of commits, though here there is only one commit.

lightning/src/chain/keysinterface.rs

valentinewallace · 2020-07-13T18:40:55Z

lightning/src/chain/keysinterface.rs

@@ -217,6 +225,7 @@ pub trait ChannelKeys : Send+Clone {
 	/// Create a signature for a local commitment transaction. This will only ever be called with
 	/// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees
 	/// that it will not be called multiple times.
+	/// The commitment must not have been revoked.


Maybe since it's not implemented yet, rephrase the line to be a TODO?

lightning/src/ln/chan_utils.rs

devrandom · 2020-07-15T11:51:03Z

I rephrased a few things and added TODOs. Please take another look.

valentinewallace

LGTM 🥇

TheBlueMatt

Sorry for the delay on this one - I was playing with it to see if I could make the asserts pass. I believe you got the issue right, so happy take this as-is, but would be nice to get the assertions in sooner or later. I commented on your PR at https://github.com/lightning-signer/rust-lightning/pull/1/files#r457548944.

TheBlueMatt · 2020-07-20T16:45:19Z

lightning/src/chain/keysinterface.rs

+	/// Gets the commitment secret for a specific commitment number as part of the revocation process
+	///
+	/// An implementation must error here if the commitment was already signed.
+	/// After this method is called, an implementation must ensure that it will refuse sign the relevant


I don't think it MUST - in theory we ensure that on the rust-lightning end, certainly it can, at least if you're in a separate memory space :).

TheBlueMatt · 2020-07-20T16:46:45Z

lightning/src/chain/keysinterface.rs

@@ -217,6 +225,7 @@ pub trait ChannelKeys : Send+Clone {
 	/// Create a signature for a local commitment transaction. This will only ever be called with
 	/// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees
 	/// that it will not be called multiple times.
+	/// The commitment must not have been revoked.


I believe all of our other comments are phrased as "what rust-lightning guarantees it will do" ie, instead of "The commitment must not have been revoked" this could read "The commitment transaction contained will not have been revoked by release_commitment_secret, and external signers may wish to enforce this".

TheBlueMatt

Needs rebase + squash, but otherwise looks good!

…mitment point The commitment secret is sensitive - it can be used by an attacker to steal funds if the node also signs the same transaction. Therefore, only release the secret from ChannelKeys when we are revoking a transaction.

devrandom · 2020-07-22T18:53:40Z

Done

devrandom force-pushed the per-commitment branch from 4202fd2 to df84dd3 Compare July 11, 2020 10:34

valentinewallace reviewed Jul 11, 2020

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Jul 11, 2020

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

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

devrandom force-pushed the per-commitment branch from 1782ec7 to 0c98489 Compare July 12, 2020 17:43

ariard reviewed Jul 12, 2020

View reviewed changes

devrandom force-pushed the per-commitment branch from 0c98489 to ebcddd9 Compare July 13, 2020 10:20

valentinewallace reviewed Jul 13, 2020

View reviewed changes

devrandom force-pushed the per-commitment branch 4 times, most recently from 950fa02 to b661164 Compare July 15, 2020 11:48

valentinewallace approved these changes Jul 15, 2020

View reviewed changes

TheBlueMatt reviewed Jul 20, 2020

View reviewed changes

TheBlueMatt approved these changes Jul 22, 2020

View reviewed changes

devrandom force-pushed the per-commitment branch from 2f5cdb7 to fefae6a Compare July 22, 2020 18:43

devrandom force-pushed the per-commitment branch from fefae6a to b19d447 Compare July 22, 2020 18:47

TheBlueMatt merged commit ed84b02 into lightningdevkit:master Jul 22, 2020

ChannelKeys - separate commitment revocation from getting the per-commitment point #655

ChannelKeys - separate commitment revocation from getting the per-commitment point #655

Uh oh!

Conversation

devrandom commented Jul 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

devrandom commented Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz commented Jul 13, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devrandom commented Jul 15, 2020

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

devrandom commented Jul 22, 2020

Uh oh!

Uh oh!

devrandom commented Jul 11, 2020 •

edited

Loading

devrandom commented Jul 12, 2020 •

edited

Loading