review: Non-controversial renames, nits, small fixups

Clean up small items based on review comments that are expected to be
non-controversial so they can all go in one commit.

Author: julianknutsen

fuzz/src/peer_crypt.rs

@@ -76,6 +76,9 @@ struct TestCtx {
 }
 
 impl TestCtx {
+	// At the completion of this call each handshake has the following state:
+	// initiator_handshake: HandshakeState::InitiatorStarting
+	// responder_handshake: HandshakeState::ResponderAwaitingActOne
 	fn make(generator: &mut FuzzGen) -> Result<Self, GeneratorFinishedError> {
 		let curve = secp256k1::Secp256k1::new();
 
@@ -89,6 +92,14 @@ impl TestCtx {
 
 		let mut initiator_handshake = PeerHandshake::new_outbound(&initiator_static_private_key, &responder_static_public_key, &initiator_ephemeral_private_key);
 		let act1 = initiator_handshake.set_up_outbound();
+
+		// Sanity checks around initialization to catch errors before we get to the fuzzing
+
+		// act 1 is the correct size
+		assert_eq!(act1.len(), 50);
+
+		// act 1 has the correct version
+		assert_eq!(act1[0], 0);
 		let responder_handshake = PeerHandshake::new_inbound(&responder_static_private_key, &responder_ephemeral_private_key);
 
 		Ok(TestCtx {
@@ -137,8 +148,8 @@ fn do_conduit_tests(generator: &mut FuzzGen, initiator_conduit: &mut Conduit, re
 	}
 }
 
-// This test completes a valid handshake based on "random" private keys and then sends variable
-// length encrypted messages between two conduits to validate that they can communicate.
+// This test completes a valid handshake based on fuzzer-provided private keys and then sends
+// variable length encrypted messages between two conduits to validate that they can communicate.
 #[inline]
 fn do_completed_handshake_test(generator: &mut FuzzGen) -> Result<(), GeneratorFinishedError> {
 	let mut test_ctx = TestCtx::make(generator)?;

lightning/src/ln/peers/handler.rs

@@ -140,17 +140,17 @@ impl PeerState {
 					},
 					// Otherwise, handshake may or may not be complete depending on whether or not
 					// we receive a (conduit, pubkey)
-					Ok((response_vec_opt, conduit_and_remote_pubkey_opt)) => {
+					Ok((response_vec_option, conduit_and_remote_static_public_key_option)) => {
 
 						// Any response generated by the handshake sequence is put into the response buffer
-						if let Some(response_vec) = response_vec_opt {
+						if let Some(response_vec) = response_vec_option {
 							mutable_response_buffer.push_back(response_vec);
 						}
 
-						// if process_act() returns the conduit and remote_pubkey the handshake
-						// is complete
-						if let Some((conduit, remote_pubkey)) = conduit_and_remote_pubkey_opt {
-							(Some(PeerState::Connected(conduit)), PeerDataProcessingDecision::CompleteHandshake(remote_pubkey))
+						// if process_act() returns the conduit and remote static public key (node id)
+						// the handshake is complete
+						if let Some((conduit, remote_static_public_key)) = conduit_and_remote_static_public_key_option {
+							(Some(PeerState::Connected(conduit)), PeerDataProcessingDecision::CompleteHandshake(remote_static_public_key))
 						} else {
 							(None, PeerDataProcessingDecision::Continue)
 						}

lightning/src/ln/peers/handshake/acts.rs

@@ -5,13 +5,14 @@
 
 use std::{cmp, ops};
 
-pub const ACT_ONE_TWO_LENGTH: usize = 50;
+pub const ACT_ONE_LENGTH: usize = 50;
+pub const ACT_TWO_LENGTH: usize = 50;
 pub const ACT_THREE_LENGTH: usize = 66;
-pub const EMPTY_ACT_ONE: ActOne = [0; ACT_ONE_TWO_LENGTH];
-pub const EMPTY_ACT_TWO: ActTwo = EMPTY_ACT_ONE;
+pub const EMPTY_ACT_ONE: ActOne = [0; ACT_ONE_LENGTH];
+pub const EMPTY_ACT_TWO: ActTwo = [0; ACT_TWO_LENGTH];
 pub const EMPTY_ACT_THREE: ActThree = [0; ACT_THREE_LENGTH];
-type ActOne = [u8; ACT_ONE_TWO_LENGTH];
-type ActTwo = ActOne;
+type ActOne = [u8; ACT_ONE_LENGTH];
+type ActTwo = [u8; ACT_TWO_LENGTH];
 type ActThree = [u8; ACT_THREE_LENGTH];
 
 /// Wrapper for any act message
@@ -64,19 +65,6 @@ impl AsRef<[u8]> for Act {
 	}
 }
 
-// Simple fill implementation for both almost-identical structs to deduplicate code
-// $act: Act[One|Two|Three], $input: &[u8]; returns &[u8] of remaining input that was not processed
-macro_rules! fill_impl {
-	($act:expr, $write_pos:expr, $input:expr) => {{
-		let fill_amount = cmp::min($act.len() - $write_pos, $input.len());
-
-		$act[$write_pos..$write_pos + fill_amount].copy_from_slice(&$input[..fill_amount]);
-
-		$write_pos += fill_amount;
-		&$input[fill_amount..]
-	}}
-}
-
 /// Light wrapper around an Act that allows multiple fill() calls before finally
 /// converting to an Act via Act::from(act_builder). Handles all of the bookkeeping
 /// and edge cases of the array fill
@@ -96,15 +84,28 @@ impl ActBuilder {
 
 	/// Fills the Act with bytes from input and returns the unprocessed bytes
 	pub(super) fn fill<'a>(&mut self, input: &'a [u8]) -> &'a [u8] {
+		// Simple fill implementation for both almost-identical structs to deduplicate code
+		// $act: Act[One|Two|Three], $input: &[u8]; returns &[u8] of remaining input that was not processed
+		macro_rules! fill_act_content {
+			($act:expr, $write_pos:expr, $input:expr) => {{
+				let fill_amount = cmp::min($act.len() - $write_pos, $input.len());
+
+				$act[$write_pos..$write_pos + fill_amount].copy_from_slice(&$input[..fill_amount]);
+
+				$write_pos += fill_amount;
+				&$input[fill_amount..]
+			}}
+		}
+
 		match &mut self.partial_act {
 			&mut Act::One(ref mut act) => {
-				fill_impl!(act, self.write_pos, input)
+				fill_act_content!(act, self.write_pos, input)
 			}
 			&mut Act::Two(ref mut act) => {
-				fill_impl!(act, self.write_pos, input)
+				fill_act_content!(act, self.write_pos, input)
 			}
 			&mut Act::Three(ref mut act) => {
-				fill_impl!(act, self.write_pos, input)
+				fill_act_content!(act, self.write_pos, input)
 			}
 		}
 	}
@@ -125,7 +126,7 @@ mod tests {
 		let mut builder = ActBuilder::new(Act::One(EMPTY_ACT_ONE));
 
 		let remaining = builder.fill(&[1, 2, 3]);
-		assert_eq!(builder.partial_act.len(), ACT_ONE_TWO_LENGTH);
+		assert_eq!(builder.partial_act.len(), ACT_ONE_LENGTH);
 		assert_eq!(builder.write_pos, 3);
 		assert!(!builder.is_finished());
 		assert_eq!(remaining, &[]);
@@ -138,8 +139,8 @@ mod tests {
 
 		let input = [0; 50];
 		let remaining = builder.fill(&input);
-		assert_eq!(builder.partial_act.len(), ACT_ONE_TWO_LENGTH);
-		assert_eq!(builder.write_pos, ACT_ONE_TWO_LENGTH);
+		assert_eq!(builder.partial_act.len(), ACT_ONE_LENGTH);
+		assert_eq!(builder.write_pos, ACT_ONE_LENGTH);
 		assert!(builder.is_finished());
 		assert_eq!(Act::from(builder).as_ref(), &input[..]);
 		assert_eq!(remaining, &[]);
@@ -153,16 +154,16 @@ mod tests {
 		let input = [0; 51];
 		let remaining = builder.fill(&input);
 
-		assert_eq!(builder.partial_act.len(), ACT_ONE_TWO_LENGTH);
-		assert_eq!(builder.write_pos, ACT_ONE_TWO_LENGTH);
+		assert_eq!(builder.partial_act.len(), ACT_ONE_LENGTH);
+		assert_eq!(builder.write_pos, ACT_ONE_LENGTH);
 		assert!(builder.is_finished());
 		assert_eq!(Act::from(builder).as_ref(), &input[..50]);
 		assert_eq!(remaining, &[0]);
 	}
 
 	// Converting an unfinished ActBuilder panics
 	#[test]
-	#[should_panic(expected="as")]
+	#[should_panic(expected="assertion failed: act_builder.is_finished()")]
 	fn convert_not_finished_panics() {
 		let builder = ActBuilder::new(Act::One(EMPTY_ACT_ONE));
 		assert_eq!(&Act::from(builder).as_ref(), &[]);

lightning/src/ln/peers/handshake/mod.rs

@@ -14,40 +14,39 @@ mod states;
 /// Currently requires explicit ephemeral private key specification.
 pub struct PeerHandshake {
 	state: Option<HandshakeState>,
-	ready_for_process: bool
+	ready_to_process: bool,
 }
 
 impl PeerHandshake {
-	/// Instantiate a new handshake with a node identity secret key and an ephemeral private key
+	/// Instantiate a handshake given the peer's static public key. The ephemeral private key MUST
+	/// generate a new session with strong cryptographic randomness.
 	pub fn new_outbound(initiator_static_private_key: &SecretKey, responder_static_public_key: &PublicKey, initiator_ephemeral_private_key: &SecretKey) -> Self {
 		let state = HandshakeState::new_initiator(initiator_static_private_key, responder_static_public_key, initiator_ephemeral_private_key);
 
 		Self {
 			state: Some(state),
-			ready_for_process: false
+			ready_to_process: false,
 		}
 	}
 
-	/// Instantiate a new handshake in anticipation of a peer's first handshake act
+	/// Instantiate a handshake in anticipation of a peer's first handshake act
 	pub fn new_inbound(responder_static_private_key: &SecretKey, responder_ephemeral_private_key: &SecretKey) -> Self {
 		Self {
 			state: Some(HandshakeState::new_responder(responder_static_private_key, responder_ephemeral_private_key)),
-			ready_for_process: true
+			ready_to_process: true,
 		}
 	}
 
 	/// Initializes the outbound handshake and provides the initial bytes to send to the responder
 	pub fn set_up_outbound(&mut self) -> Vec<u8> {
-		assert!(!self.ready_for_process);
-		let cur_state = self.state.take().unwrap();
+		assert!(!self.ready_to_process);
+		self.ready_to_process = true;
 
 		// This transition does not have a failure path
-		let (response_vec_opt, next_state) = cur_state.next(&[]).unwrap();
-
-		self.state = Some(next_state);
+		let (response_vec_option, conduit_and_remote_static_public_key_option) = self.process_act(&[]).unwrap();
+		assert!(conduit_and_remote_static_public_key_option.is_none());
 
-		self.ready_for_process = true;
-		response_vec_opt.unwrap().to_vec()
+		response_vec_option.unwrap()
 	}
 
 	/// Process act dynamically
@@ -59,21 +58,19 @@ impl PeerHandshake {
 	/// `.0`: Byte vector containing the next act to send back to the peer per the handshake protocol
 	/// `.1`: Some(Conduit, PublicKey) if the handshake was just processed to completion and messages can now be encrypted and decrypted
 	pub fn process_act(&mut self, input: &[u8]) -> Result<(Option<Vec<u8>>, Option<(Conduit, PublicKey)>), String> {
-		assert!(self.ready_for_process);
+		assert!(self.ready_to_process);
 		let cur_state = self.state.take().unwrap();
 
-		// Convert the Act to a Vec before passing it back. Next patch removes this in favor
-		// of passing back a slice.
-		let (act_opt, mut next_state) = match cur_state.next(input)? {
+		let (act_option, mut next_state) = match cur_state.next(input)? {
 			(Some(act), next_state) => (Some(act.to_vec()), next_state),
 			(None, next_state) => (None, next_state)
 		};
 
 		let result = match next_state {
 			HandshakeState::Complete(ref mut conduit_and_pubkey) => {
-				Ok((act_opt, conduit_and_pubkey.take()))
+				Ok((act_option, conduit_and_pubkey.take()))
 			},
-			_ => { Ok((act_opt, None)) }
+			_ => { Ok((act_option, None)) }
 		};
 
 		self.state = Some(next_state);
@@ -140,7 +137,7 @@ mod test {
 
 	// Test that the outbound needs to call set_up_outbound() before process_act()
 	#[test]
-	#[should_panic(expected = "assertion failed: self.ready_for_process")]
+	#[should_panic(expected = "assertion failed: self.ready_to_process")]
 	fn new_outbound_no_set_up_panics() {
 		let curve = secp256k1::Secp256k1::new();
 
@@ -155,7 +152,7 @@ mod test {
 
 	// Test that calling set_up_outbound() on the inbound panics
 	#[test]
-	#[should_panic(expected = "assertion failed: !self.ready_for_process")]
+	#[should_panic(expected = "assertion failed: !self.ready_to_process")]
 	fn new_inbound_calling_set_up_panics() {
 		let inbound_static_private_key = SecretKey::from_slice(&[0x_21_u8; 32]).unwrap();
 		let inbound_ephemeral_private_key = SecretKey::from_slice(&[0x_22_u8; 32]).unwrap();

lightning/src/ln/peers/handshake/states.rs

@@ -6,8 +6,9 @@ use bitcoin::secp256k1::{SecretKey, PublicKey};
 
 use ln::peers::{chacha, hkdf};
 use ln::peers::conduit::{Conduit, SymmetricKey};
-use ln::peers::handshake::acts::{Act, ActBuilder, EMPTY_ACT_ONE, EMPTY_ACT_TWO, EMPTY_ACT_THREE};
+use ln::peers::handshake::acts::{Act, ActBuilder, ACT_ONE_LENGTH, ACT_TWO_LENGTH, ACT_THREE_LENGTH, EMPTY_ACT_ONE, EMPTY_ACT_TWO, EMPTY_ACT_THREE};
 
+// Alias type to help differentiate between temporary key and chaining key when passing bytes around
 type ChainingKey = [u8; 32];
 
 // Generate a SHA-256 hash from one or more elements concatenated together
@@ -30,7 +31,8 @@ pub(super) enum HandshakeState {
 }
 
 // Trait for all individual states to implement that ensure HandshakeState::next() can
-// delegate to a common function signature.
+// delegate to a common function signature. May transition to the same state in the event there are
+// not yet enough bytes to move forward with the handshake.
 pub(super) trait IHandshakeState {
 	fn next(self, input: &[u8]) -> Result<(Option<Act>, HandshakeState), String>;
 }
@@ -101,7 +103,7 @@ pub(super) struct ResponderAwaitingActThreeState {
 impl InitiatorStartingState {
 	pub(crate) fn new(initiator_static_private_key: SecretKey, initiator_ephemeral_private_key: SecretKey, responder_static_public_key: PublicKey) -> Self {
 		let initiator_static_public_key = private_key_to_public_key(&initiator_static_private_key);
-		let (hash, chaining_key) = handshake_state_initialization(&responder_static_public_key);
+		let (hash, chaining_key) = initialize_handshake_state(&responder_static_public_key);
 		let initiator_ephemeral_public_key = private_key_to_public_key(&initiator_ephemeral_private_key);
 		InitiatorStartingState {
 			initiator_static_private_key,
@@ -160,7 +162,7 @@ impl IHandshakeState for InitiatorStartingState {
 impl ResponderAwaitingActOneState {
 	pub(crate) fn new(responder_static_private_key: SecretKey, responder_ephemeral_private_key: SecretKey) -> Self {
 		let responder_static_public_key = private_key_to_public_key(&responder_static_private_key);
-		let (hash, chaining_key) = handshake_state_initialization(&responder_static_public_key);
+		let (hash, chaining_key) = initialize_handshake_state(&responder_static_public_key);
 		let responder_ephemeral_public_key = private_key_to_public_key(&responder_ephemeral_private_key);
 
 		ResponderAwaitingActOneState {
@@ -181,7 +183,7 @@ impl IHandshakeState for ResponderAwaitingActOneState {
 		let mut act_one_builder = self.act_one_builder;
 		let remaining = act_one_builder.fill(input);
 
-		// Any payload larger than ACT_ONE_TWO_LENGTH indicates a bad peer since initiator data
+		// Any payload larger than ACT_ONE_LENGTH indicates a bad peer since initiator data
 		// is required to generate act3 (so it can't come before we transition)
 		if remaining.len() != 0 {
 			return Err("Act One too large".to_string());
@@ -247,7 +249,7 @@ impl IHandshakeState for InitiatorAwaitingActTwoState {
 		let mut act_two_builder = self.act_two_builder;
 		let remaining = act_two_builder.fill(input);
 
-		// Any payload larger than ACT_ONE_TWO_LENGTH indicates a bad peer since responder data
+		// Any payload larger than ACT_TWO_LENGTH indicates a bad peer since responder data
 		// is required to generate post-authentication messages (so it can't come before we transition)
 		if remaining.len() != 0 {
 			return Err("Act Two too large".to_string());
@@ -310,6 +312,7 @@ impl IHandshakeState for InitiatorAwaitingActTwoState {
 		// - done by Conduit
 		let conduit = Conduit::new(sending_key, receiving_key, chaining_key);
 
+		// 8. Send m = 0 || c || t
 		Ok((
 			Some(Act::Three(act_three)),
 			HandshakeState::Complete(Some((conduit, responder_static_public_key)))
@@ -345,6 +348,7 @@ impl IHandshakeState for ResponderAwaitingActThreeState {
 
 		// 1. Read exactly 66 bytes from the network buffer
 		let act_three_bytes = Act::from(act_three_builder);
+		assert_eq!(act_three_bytes.len(), ACT_THREE_LENGTH);
 
 		// 2. Parse the read message (m) into v, c, and t
 		let version = act_three_bytes[0];
@@ -396,8 +400,11 @@ impl IHandshakeState for ResponderAwaitingActThreeState {
 	}
 }
 
+// The handshake state always uses the responder's static public key. When running on the initiator,
+// the initiator provides the remote's static public key and running on the responder they provide
+// their own.
 // https://github.com/lightningnetwork/lightning-rfc/blob/master/08-transport.md#handshake-state-initialization
-fn handshake_state_initialization(responder_static_public_key: &PublicKey) -> (Sha256, Sha256) {
+fn initialize_handshake_state(responder_static_public_key: &PublicKey) -> (Sha256, Sha256) {
 	let protocol_name = b"Noise_XK_secp256k1_ChaChaPoly_SHA256";
 	let prologue = b"lightning";
 
@@ -414,6 +421,7 @@ fn handshake_state_initialization(responder_static_public_key: &PublicKey) -> (S
 	(hash, chaining_key)
 }
 
+// Due to the very high similarity of acts 1 and 2, this method is used to process both
 // https://github.com/lightningnetwork/lightning-rfc/blob/master/08-transport.md#act-one (sender)
 // https://github.com/lightningnetwork/lightning-rfc/blob/master/08-transport.md#act-two (sender)
 fn calculate_act_message(local_private_ephemeral_key: &SecretKey, local_public_ephemeral_key: &PublicKey, remote_public_key: &PublicKey, chaining_key: ChainingKey, hash: Sha256, act_out: &mut [u8]) -> (Sha256, SymmetricKey, SymmetricKey) {
@@ -448,6 +456,11 @@ fn calculate_act_message(local_private_ephemeral_key: &SecretKey, local_public_e
 // https://github.com/lightningnetwork/lightning-rfc/blob/master/08-transport.md#act-two (receiver)
 fn process_act_message(act_bytes: &[u8], local_private_key: &SecretKey, chaining_key: ChainingKey, hash: Sha256) -> Result<(PublicKey, Sha256, SymmetricKey, SymmetricKey), String> {
 	// 1. Read exactly 50 bytes from the network buffer
+	// Partial act messages are handled by the callers. By the time it gets here, it
+	// must be the correct size.
+	assert_eq!(act_bytes.len(), ACT_ONE_LENGTH);
+	assert_eq!(act_bytes.len(), ACT_TWO_LENGTH);
+
 	// 2.Parse the read message (m) into v, re, and c
 	let version = act_bytes[0];
 	let ephemeral_public_key_bytes = &act_bytes[1..34];
@@ -476,7 +489,8 @@ fn process_act_message(act_bytes: &[u8], local_private_key: &SecretKey, chaining
 	// 6. Act2: ck, temp_k2 = HKDF(ck, ee)
 	let (chaining_key, temporary_key) = hkdf::derive(&chaining_key, &ecdh);
 
-	// 7. p = decryptWithAD(temp_k1, 0, h, c)
+	// 7. Act1: p = decryptWithAD(temp_k1, 0, h, c)
+	// 7. Act2: p = decryptWithAD(temp_k2, 0, h, c)
 	chacha::decrypt(&temporary_key, 0, &hash, &chacha_tag, &mut [0; 0])?;
 
 	// 8. h = SHA-256(h || c)