Merge pull request #674 from TheBlueMatt/2020-08-keyif-rand-names

Simplify + clarify random-bytes-fetching from KeysInterface

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -142,8 +142,7 @@ impl channelmonitor::ManyChannelMonitor for TestChannelMonitor {
 
 struct KeyProvider {
 	node_id: u8,
-	session_id: atomic::AtomicU8,
-	channel_id: atomic::AtomicU8,
+	rand_bytes_id: atomic::AtomicU8,
 }
 impl KeysInterface for KeyProvider {
 	type ChanKeySigner = EnforcingChannelKeys;
@@ -179,14 +178,8 @@ impl KeysInterface for KeyProvider {
 		))
 	}
 
-	fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) {
-		let id = self.session_id.fetch_add(1, atomic::Ordering::Relaxed);
-		(SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, id, 10, self.node_id]).unwrap(),
-		[0; 32])
-	}
-
-	fn get_channel_id(&self) -> [u8; 32] {
-		let id = self.channel_id.fetch_add(1, atomic::Ordering::Relaxed);
+	fn get_secure_random_bytes(&self) -> [u8; 32] {
+		let id = self.rand_bytes_id.fetch_add(1, atomic::Ordering::Relaxed);
 		[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, id, 11, self.node_id]
 	}
 }
@@ -202,7 +195,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 			let watch = Arc::new(ChainWatchInterfaceUtil::new(Network::Bitcoin));
 			let monitor = Arc::new(TestChannelMonitor::new(watch.clone(), broadcast.clone(), logger.clone(), fee_est.clone()));
 
-			let keys_manager = Arc::new(KeyProvider { node_id: $node_id, session_id: atomic::AtomicU8::new(0), channel_id: atomic::AtomicU8::new(0) });
+			let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU8::new(0) });
 			let mut config = UserConfig::default();
 			config.channel_options.fee_proportional_millionths = 0;
 			config.channel_options.announced_channel = true;
@@ -218,7 +211,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 			let watch = Arc::new(ChainWatchInterfaceUtil::new(Network::Bitcoin));
 			let monitor = Arc::new(TestChannelMonitor::new(watch.clone(), broadcast.clone(), logger.clone(), fee_est.clone()));
 
-			let keys_manager = Arc::new(KeyProvider { node_id: $node_id, session_id: atomic::AtomicU8::new(0), channel_id: atomic::AtomicU8::new(0) });
+			let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU8::new(0) });
 			let mut config = UserConfig::default();
 			config.channel_options.fee_proportional_millionths = 0;
 			config.channel_options.announced_channel = true;

fuzz/src/full_stack.rs

@@ -295,13 +295,7 @@ impl KeysInterface for KeyProvider {
 		})
 	}
 
-	fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) {
-		let ctr = self.counter.fetch_add(1, Ordering::Relaxed) as u8;
-		(SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 13, ctr]).unwrap(),
-		[0; 32])
-	}
-
-	fn get_channel_id(&self) -> [u8; 32] {
+	fn get_secure_random_bytes(&self) -> [u8; 32] {
 		let ctr = self.counter.fetch_add(1, Ordering::Relaxed);
 		[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 		(ctr >> 8*7) as u8, (ctr >> 8*6) as u8, (ctr >> 8*5) as u8, (ctr >> 8*4) as u8, (ctr >> 8*3) as u8, (ctr >> 8*2) as u8, (ctr >> 8*1) as u8, 14, (ctr >> 8*0) as u8]

lightning/src/chain/keysinterface.rs

@@ -341,12 +341,10 @@ pub trait KeysInterface: Send + Sync {
 	/// Get a new set of ChannelKeys for per-channel secrets. These MUST be unique even if you
 	/// restarted with some stale data!
 	fn get_channel_keys(&self, inbound: bool, channel_value_satoshis: u64) -> Self::ChanKeySigner;
-	/// Get a secret and PRNG seed for constructing an onion packet
-	fn get_onion_rand(&self) -> (SecretKey, [u8; 32]);
-	/// Get a unique temporary channel id. Channels will be referred to by this until the funding
-	/// transaction is created, at which point they will use the outpoint in the funding
-	/// transaction.
-	fn get_channel_id(&self) -> [u8; 32];
+	/// Gets a unique, cryptographically-secure, random 32 byte value. This is used for encrypting
+	/// onion packets and for temporary channel IDs. There is no requirement that these be
+	/// persisted anywhere, though they must be unique across restarts.
+	fn get_secure_random_bytes(&self) -> [u8; 32];
 }
 
 #[derive(Clone)]
@@ -666,10 +664,8 @@ pub struct KeysManager {
 	shutdown_pubkey: PublicKey,
 	channel_master_key: ExtendedPrivKey,
 	channel_child_index: AtomicUsize,
-	session_master_key: ExtendedPrivKey,
-	session_child_index: AtomicUsize,
-	channel_id_master_key: ExtendedPrivKey,
-	channel_id_child_index: AtomicUsize,
+	rand_bytes_master_key: ExtendedPrivKey,
+	rand_bytes_child_index: AtomicUsize,
 
 	seed: [u8; 32],
 	starting_time_secs: u64,
@@ -678,7 +674,7 @@ pub struct KeysManager {
 
 impl KeysManager {
 	/// Constructs a KeysManager from a 32-byte seed. If the seed is in some way biased (eg your
-	/// RNG is busted) this may panic (but more importantly, you will possibly lose funds).
+	/// CSRNG is busted) this may panic (but more importantly, you will possibly lose funds).
 	/// starting_time isn't strictly required to actually be a time, but it must absolutely,
 	/// without a doubt, be unique to this instance. ie if you start multiple times with the same
 	/// seed, starting_time must be unique to each run. Thus, the easiest way to achieve this is to
@@ -715,8 +711,7 @@ impl KeysManager {
 					Err(_) => panic!("Your RNG is busted"),
 				};
 				let channel_master_key = master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx(3).unwrap()).expect("Your RNG is busted");
-				let session_master_key = master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx(4).unwrap()).expect("Your RNG is busted");
-				let channel_id_master_key = master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx(5).unwrap()).expect("Your RNG is busted");
+				let rand_bytes_master_key = master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx(4).unwrap()).expect("Your RNG is busted");
 
 				KeysManager {
 					secp_ctx,
@@ -725,10 +720,8 @@ impl KeysManager {
 					shutdown_pubkey,
 					channel_master_key,
 					channel_child_index: AtomicUsize::new(0),
-					session_master_key,
-					session_child_index: AtomicUsize::new(0),
-					channel_id_master_key,
-					channel_id_child_index: AtomicUsize::new(0),
+					rand_bytes_master_key,
+					rand_bytes_child_index: AtomicUsize::new(0),
 
 					seed: *seed,
 					starting_time_secs,
@@ -821,29 +814,14 @@ impl KeysInterface for KeysManager {
 		self.derive_channel_keys(channel_value_satoshis, ix_and_nanos, self.starting_time_secs)
 	}
 
-	fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) {
+	fn get_secure_random_bytes(&self) -> [u8; 32] {
 		let mut sha = self.derive_unique_start();
 
-		let child_ix = self.session_child_index.fetch_add(1, Ordering::AcqRel);
-		let child_privkey = self.session_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32).expect("key space exhausted")).expect("Your RNG is busted");
-		sha.input(&child_privkey.private_key.key[..]);
-
-		let mut rng_seed = sha.clone();
-		// Not exactly the most ideal construction, but the second value will get fed into
-		// ChaCha so it is another step harder to break.
-		rng_seed.input(b"RNG Seed Salt");
-		sha.input(b"Session Key Salt");
-		(SecretKey::from_slice(&Sha256::from_engine(sha).into_inner()).expect("Your RNG is busted"),
-		Sha256::from_engine(rng_seed).into_inner())
-	}
-
-	fn get_channel_id(&self) -> [u8; 32] {
-		let mut sha = self.derive_unique_start();
-
-		let child_ix = self.channel_id_child_index.fetch_add(1, Ordering::AcqRel);
-		let child_privkey = self.channel_id_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32).expect("key space exhausted")).expect("Your RNG is busted");
+		let child_ix = self.rand_bytes_child_index.fetch_add(1, Ordering::AcqRel);
+		let child_privkey = self.rand_bytes_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32).expect("key space exhausted")).expect("Your RNG is busted");
 		sha.input(&child_privkey.private_key.key[..]);
 
+		sha.input(b"Unique Secure Random Bytes Salt");
 		Sha256::from_engine(sha).into_inner()
 	}
 }

lightning/src/ln/channel.rs

@@ -488,7 +488,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			user_id: user_id,
 			config: config.channel_options.clone(),
 
-			channel_id: keys_provider.get_channel_id(),
+			channel_id: keys_provider.get_secure_random_bytes(),
 			channel_state: ChannelState::OurInitSent as u32,
 			channel_outbound: true,
 			secp_ctx: Secp256k1::new(),
@@ -4520,8 +4520,7 @@ mod tests {
 		fn get_channel_keys(&self, _inbound: bool, _channel_value_satoshis: u64) -> InMemoryChannelKeys {
 			self.chan_keys.clone()
 		}
-		fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) { panic!(); }
-		fn get_channel_id(&self) -> [u8; 32] { [0; 32] }
+		fn get_secure_random_bytes(&self) -> [u8; 32] { [0; 32] }
 	}
 
 	fn public_from_secret_hex(secp_ctx: &Secp256k1<All>, hex: &str) -> PublicKey {

lightning/src/ln/channelmanager.rs

@@ -1243,7 +1243,8 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 	// Only public for testing, this should otherwise never be called direcly
 	pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, total_value: u64, cur_height: u32) -> Result<(), APIError> {
 		log_trace!(self.logger, "Attempting to send payment for path with next hop {}", path.first().unwrap().short_channel_id);
-		let (session_priv, prng_seed) = self.keys_manager.get_onion_rand();
+		let prng_seed = self.keys_manager.get_secure_random_bytes();
+		let session_priv = SecretKey::from_slice(&self.keys_manager.get_secure_random_bytes()[..]).expect("RNG is busted");
 
 		let onion_keys = onion_utils::construct_onion_keys(&self.secp_ctx, &path, &session_priv)
 			.map_err(|_| APIError::RouteError{err: "Pubkey along hop was maliciously selected"})?;

lightning/src/ln/functional_tests.rs

@@ -6143,7 +6143,7 @@ fn test_onion_failure() {
 	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
 	let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 	for node in nodes.iter() {
-		*node.keys_manager.override_session_priv.lock().unwrap() = Some(SecretKey::from_slice(&[3; 32]).unwrap());
+		*node.keys_manager.override_session_priv.lock().unwrap() = Some([3; 32]);
 	}
 	let channels = [create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()), create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known())];
 	let (_, payment_hash) = get_payment_preimage_hash!(nodes[0]);

lightning/src/util/test_utils.rs

@@ -350,7 +350,7 @@ impl Logger for TestLogger {
 
 pub struct TestKeysInterface {
 	backing: keysinterface::KeysManager,
-	pub override_session_priv: Mutex<Option<SecretKey>>,
+	pub override_session_priv: Mutex<Option<[u8; 32]>>,
 	pub override_channel_id_priv: Mutex<Option<[u8; 32]>>,
 }
 
@@ -364,18 +364,19 @@ impl keysinterface::KeysInterface for TestKeysInterface {
 		EnforcingChannelKeys::new(self.backing.get_channel_keys(inbound, channel_value_satoshis))
 	}
 
-	fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) {
-		match *self.override_session_priv.lock().unwrap() {
-			Some(key) => (key.clone(), [0; 32]),
-			None => self.backing.get_onion_rand()
+	fn get_secure_random_bytes(&self) -> [u8; 32] {
+		let override_channel_id = self.override_channel_id_priv.lock().unwrap();
+		let override_session_key = self.override_session_priv.lock().unwrap();
+		if override_channel_id.is_some() && override_session_key.is_some() {
+			panic!("We don't know which override key to use!");
 		}
-	}
-
-	fn get_channel_id(&self) -> [u8; 32] {
-		match *self.override_channel_id_priv.lock().unwrap() {
-			Some(key) => key.clone(),
-			None => self.backing.get_channel_id()
+		if let Some(key) = &*override_channel_id {
+			return *key;
+		}
+		if let Some(key) = &*override_session_key {
+			return *key;
 		}
+		self.backing.get_secure_random_bytes()
 	}
 }