From d2e6f2ac18159f4d9b3b1cbc30bebd03030f0304 Mon Sep 17 00:00:00 2001 From: Devrandom Date: Sun, 9 Aug 2020 15:45:23 +0200 Subject: [PATCH] Make TxCreationKeys public and wrap it in PreCalculatedTxCreationKeys Allows calling of InMemoryChannelKeys methods. The wrapping makes it obvious to signer implementers that the pre-derived keys are a local cache and should not be trusted in a validating signer. --- lightning/src/chain/keysinterface.rs | 7 ++-- lightning/src/ln/chan_utils.rs | 40 ++++++++++++++++++--- lightning/src/ln/channel.rs | 11 +++--- lightning/src/ln/functional_tests.rs | 4 ++- lightning/src/util/enforcing_trait_impls.rs | 8 ++--- 5 files changed, 53 insertions(+), 17 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 26d1f07c7..63c87ad21 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -23,7 +23,7 @@ use util::byte_utils; use util::ser::{Writeable, Writer, Readable}; use ln::chan_utils; -use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, LocalCommitmentTransaction}; +use ln::chan_utils::{HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, LocalCommitmentTransaction, PreCalculatedTxCreationKeys}; use ln::msgs; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -223,7 +223,7 @@ pub trait ChannelKeys : Send+Clone { // TODO: Document the things someone using this interface should enforce before signing. // TODO: Add more input vars to enable better checking (preferably removing commitment_tx and // making the callee generate it via some util function we expose)! - fn sign_remote_commitment(&self, feerate_per_kw: u32, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; + fn sign_remote_commitment(&self, feerate_per_kw: u32, commitment_tx: &Transaction, keys: &PreCalculatedTxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; /// 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 @@ -461,8 +461,9 @@ impl ChannelKeys for InMemoryChannelKeys { fn pubkeys(&self) -> &ChannelPublicKeys { &self.local_channel_pubkeys } fn key_derivation_params(&self) -> (u64, u64) { self.key_derivation_params } - fn sign_remote_commitment(&self, feerate_per_kw: u32, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + fn sign_remote_commitment(&self, feerate_per_kw: u32, commitment_tx: &Transaction, pre_keys: &PreCalculatedTxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { if commitment_tx.input.len() != 1 { return Err(()); } + let keys = pre_keys.trust_key_derivation(); let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key); let accepted_data = self.accepted_channel_data.as_ref().expect("must accept before signing"); diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index ba175733a..1e49782b8 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -267,23 +267,52 @@ pub fn derive_public_revocation_key(secp_ctx: &Secp2 /// The set of public keys which are used in the creation of one commitment transaction. /// These are derived from the channel base keys and per-commitment data. +/// +/// These keys are assumed to be good, either because the code derived them from +/// channel basepoints via the new function, or they were obtained via +/// PreCalculatedTxCreationKeys.trust_key_derivation because we trusted the source of the +/// pre-calculated keys. #[derive(PartialEq, Clone)] pub struct TxCreationKeys { /// The per-commitment public key which was used to derive the other keys. pub per_commitment_point: PublicKey, /// The revocation key which is used to allow the owner of the commitment transaction to /// provide their counterparty the ability to punish them if they broadcast an old state. - pub(crate) revocation_key: PublicKey, + pub revocation_key: PublicKey, /// A's HTLC Key - pub(crate) a_htlc_key: PublicKey, + pub a_htlc_key: PublicKey, /// B's HTLC Key - pub(crate) b_htlc_key: PublicKey, + pub b_htlc_key: PublicKey, /// A's Payment Key (which isn't allowed to be spent from for some delay) - pub(crate) a_delayed_payment_key: PublicKey, + pub a_delayed_payment_key: PublicKey, } impl_writeable!(TxCreationKeys, 33*6, { per_commitment_point, revocation_key, a_htlc_key, b_htlc_key, a_delayed_payment_key }); +/// The per-commitment point and a set of pre-calculated public keys used for transaction creation +/// in the signer. +/// The pre-calculated keys are an optimization, because ChannelKeys has enough +/// information to re-derive them. +pub struct PreCalculatedTxCreationKeys(TxCreationKeys); + +impl PreCalculatedTxCreationKeys { + /// Create a new PreCalculatedTxCreationKeys from TxCreationKeys + pub fn new(keys: TxCreationKeys) -> Self { + PreCalculatedTxCreationKeys(keys) + } + + /// The pre-calculated transaction creation public keys. + /// An external validating signer should not trust these keys. + pub fn trust_key_derivation(&self) -> &TxCreationKeys { + &self.0 + } + + /// The transaction per-commitment point + pub fn per_comitment_point(&self) -> &PublicKey { + &self.0.per_commitment_point + } +} + /// One counterparty's public keys which do not change over the life of a channel. #[derive(Clone, PartialEq)] pub struct ChannelPublicKeys { @@ -318,7 +347,8 @@ impl_writeable!(ChannelPublicKeys, 33*5, { impl TxCreationKeys { - pub(crate) fn new(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, a_delayed_payment_base: &PublicKey, a_htlc_base: &PublicKey, b_revocation_base: &PublicKey, b_htlc_base: &PublicKey) -> Result { + /// Create a new TxCreationKeys from channel base points and the per-commitment point + pub fn new(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, a_delayed_payment_base: &PublicKey, a_htlc_base: &PublicKey, b_revocation_base: &PublicKey, b_htlc_base: &PublicKey) -> Result { Ok(TxCreationKeys { per_commitment_point: per_commitment_point.clone(), revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &b_revocation_base)?, diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 719d03c6e..1ae894f7f 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -19,7 +19,7 @@ use ln::msgs; use ln::msgs::{DecodeError, OptionalField, DataLossProtect}; use ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER}; use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash, BREAKDOWN_TIMEOUT, MAX_LOCAL_BREAKDOWN_TIMEOUT}; -use ln::chan_utils::{CounterpartyCommitmentSecrets, LocalCommitmentTransaction, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys}; +use ln::chan_utils::{CounterpartyCommitmentSecrets, LocalCommitmentTransaction, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys, PreCalculatedTxCreationKeys}; use ln::chan_utils; use chain::chaininterface::{FeeEstimator,ConfirmationTarget}; use chain::transaction::OutPoint; @@ -1484,7 +1484,8 @@ impl Channel { let remote_keys = self.build_remote_transaction_keys()?; let remote_initial_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw, logger).0; - let remote_signature = self.local_keys.sign_remote_commitment(self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), &self.secp_ctx) + let pre_remote_keys = PreCalculatedTxCreationKeys::new(remote_keys); + let remote_signature = self.local_keys.sign_remote_commitment(self.feerate_per_kw, &remote_initial_commitment_tx, &pre_remote_keys, &Vec::new(), &self.secp_ctx) .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0; // We sign the "remote" commitment transaction, allowing them to broadcast the tx if they wish. @@ -3525,7 +3526,8 @@ impl Channel { fn get_outbound_funding_created_signature(&mut self, logger: &L) -> Result where L::Target: Logger { let remote_keys = self.build_remote_transaction_keys()?; let remote_initial_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw, logger).0; - Ok(self.local_keys.sign_remote_commitment(self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), &self.secp_ctx) + let pre_remote_keys = PreCalculatedTxCreationKeys::new(remote_keys); + Ok(self.local_keys.sign_remote_commitment(self.feerate_per_kw, &remote_initial_commitment_tx, &pre_remote_keys, &Vec::new(), &self.secp_ctx) .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0) } @@ -3878,7 +3880,8 @@ impl Channel { htlcs.push(htlc); } - let res = self.local_keys.sign_remote_commitment(feerate_per_kw, &remote_commitment_tx.0, &remote_keys, &htlcs, &self.secp_ctx) + let pre_remote_keys = PreCalculatedTxCreationKeys::new(remote_keys.clone()); + let res = self.local_keys.sign_remote_commitment(feerate_per_kw, &remote_commitment_tx.0, &pre_remote_keys, &htlcs, &self.secp_ctx) .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?; signature = res.0; htlc_signatures = res.1; diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 240a5741b..e1254ee77 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -52,6 +52,7 @@ use std::sync::atomic::Ordering; use std::{mem, io}; use ln::functional_test_utils::*; +use ln::chan_utils::PreCalculatedTxCreationKeys; #[test] fn test_insane_channel_opens() { @@ -1694,7 +1695,8 @@ fn test_fee_spike_violation_fails_htlc() { let local_chan_lock = nodes[0].node.channel_state.lock().unwrap(); let local_chan = local_chan_lock.by_id.get(&chan.2).unwrap(); let local_chan_keys = local_chan.get_local_keys(); - local_chan_keys.sign_remote_commitment(feerate_per_kw, &commit_tx, &commit_tx_keys, &[&accepted_htlc_info], &secp_ctx).unwrap() + let pre_commit_tx_keys = PreCalculatedTxCreationKeys::new(commit_tx_keys); + local_chan_keys.sign_remote_commitment(feerate_per_kw, &commit_tx, &pre_commit_tx_keys, &[&accepted_htlc_info], &secp_ctx).unwrap() }; let commit_signed_msg = msgs::CommitmentSigned { diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index 1864fffbb..9f297b481 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -1,4 +1,4 @@ -use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys, LocalCommitmentTransaction}; +use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys, LocalCommitmentTransaction, PreCalculatedTxCreationKeys}; use ln::{chan_utils, msgs}; use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys}; @@ -60,9 +60,9 @@ impl ChannelKeys for EnforcingChannelKeys { fn pubkeys(&self) -> &ChannelPublicKeys { self.inner.pubkeys() } fn key_derivation_params(&self) -> (u64, u64) { self.inner.key_derivation_params() } - fn sign_remote_commitment(&self, feerate_per_kw: u32, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + fn sign_remote_commitment(&self, feerate_per_kw: u32, commitment_tx: &Transaction, pre_keys: &PreCalculatedTxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { if commitment_tx.input.len() != 1 { panic!("lightning commitment transactions have a single input"); } - self.check_keys(secp_ctx, keys); + self.check_keys(secp_ctx, pre_keys.trust_key_derivation()); let obscured_commitment_transaction_number = (commitment_tx.lock_time & 0xffffff) as u64 | ((commitment_tx.input[0].sequence as u64 & 0xffffff) << 3*8); { @@ -75,7 +75,7 @@ impl ChannelKeys for EnforcingChannelKeys { commitment_data.1 = cmp::max(commitment_number, commitment_data.1) } - Ok(self.inner.sign_remote_commitment(feerate_per_kw, commitment_tx, keys, htlcs, secp_ctx).unwrap()) + Ok(self.inner.sign_remote_commitment(feerate_per_kw, commitment_tx, pre_keys, htlcs, secp_ctx).unwrap()) } fn sign_local_commitment(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { -- 2.39.5