-
Notifications
You must be signed in to change notification settings - Fork 409
Modular handshake #494
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modular handshake #494
Changes from 1 commit
eb6a371
b71b7ea
92eac9b
986f25f
ffbf5ec
19b7700
17fda75
8169b31
f1002c5
f0fc10b
eb297f9
5492717
256b6f5
b4921e9
299b6f7
944177a
0fbd895
6cf5a07
6f4e76a
c2227b6
6bae489
2df93ca
eda13bf
4e6b25a
f1940e9
4deb290
5e9c350
4b4cb98
029bb66
54b7464
fe705a9
be5e2a5
2e4e659
a4fff76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
use util::chacha20poly1305rfc::ChaCha20Poly1305RFC; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this in ln::peers? It seems to be pure crypto functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the AEAD-based encryption methods are only used for handshakes and peer message encryption IIRC, and not for the onion construction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but its also a pure-crypto primitive. I guess my preference is for such things (even if it implements a lightning protocol crypto primitive) to be in some kind of crypto module. |
||
pub fn encrypt(key: &[u8], nonce: u64, associated_data: &[u8], plaintext: &[u8]) -> Vec<u8> { | ||
let mut nonce_bytes = [0; 12]; | ||
nonce_bytes[4..].copy_from_slice(&nonce.to_le_bytes()); | ||
|
||
let mut chacha = ChaCha20Poly1305RFC::new(key, &nonce_bytes, associated_data); | ||
let mut ciphertext = vec![0u8; plaintext.len()]; | ||
let mut authentication_tag = [0u8; 16]; | ||
chacha.encrypt(plaintext, &mut ciphertext, &mut authentication_tag); | ||
|
||
let mut tagged_ciphertext = ciphertext; | ||
tagged_ciphertext.extend_from_slice(&authentication_tag); | ||
tagged_ciphertext | ||
} | ||
|
||
|
||
pub fn decrypt(key: &[u8], nonce: u64, associated_data: &[u8], tagged_ciphertext: &[u8]) -> Result<Vec<u8>, String> { | ||
let mut nonce_bytes = [0; 12]; | ||
nonce_bytes[4..].copy_from_slice(&nonce.to_le_bytes()); | ||
|
||
let length = tagged_ciphertext.len(); | ||
let ciphertext = &tagged_ciphertext[0..length - 16]; | ||
let authentication_tag = &tagged_ciphertext[length - 16..length]; | ||
|
||
let mut chacha = ChaCha20Poly1305RFC::new(key, &nonce_bytes, associated_data); | ||
let mut plaintext = vec![0u8; length - 16]; | ||
let success = chacha.decrypt(ciphertext, &mut plaintext, authentication_tag); | ||
if success { | ||
Ok(plaintext.to_vec()) | ||
} else { | ||
Err("invalid hmac".to_string()) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
use ln::peers::{chacha, hkdf}; | ||
|
||
/// Returned after a successful handshake to encrypt and decrypt communication with peer nodes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add more details about how you'd use this, what the requirements are for use, etc? Same goes for pretty much everything pub in this PR. |
||
pub struct Conduit { | ||
pub(crate) sending_key: [u8; 32], | ||
pub(crate) receiving_key: [u8; 32], | ||
|
||
pub(crate) sending_chaining_key: [u8; 32], | ||
pub(crate) receiving_chaining_key: [u8; 32], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you make constants for these? Or reuse some from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These aren't constants as much as they are types, but definitely. |
||
|
||
pub(crate) receiving_nonce: u32, | ||
pub(crate) sending_nonce: u32, | ||
|
||
pub(super) read_buffer: Option<Vec<u8>>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this an Option<>? |
||
} | ||
|
||
impl Conduit { | ||
pub fn encrypt(&mut self, buffer: &[u8]) -> Vec<u8> { | ||
let length = buffer.len() as u16; | ||
let length_bytes = length.to_be_bytes(); | ||
|
||
let encrypted_length = chacha::encrypt(&self.sending_key, self.sending_nonce as u64, &[0; 0], &length_bytes); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oof. Lets avoid allocating a Vec for two bytes somehow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure this is the right place to shave off memory usage at the expense of legibility? I'm a bit hesitant to do the whole decrypt in place approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont think it changes legibility, and its not a memory usage question, its a question of heap fragmentation and significant performance penalty to hit the heap. I'm dubious you can't make it readable without using a Vec. |
||
self.increment_sending_nonce(); | ||
|
||
let encrypted_message = chacha::encrypt(&self.sending_key, self.sending_nonce as u64, &[0; 0], buffer); | ||
self.increment_sending_nonce(); | ||
|
||
let mut ciphertext = encrypted_length; | ||
ciphertext.extend_from_slice(&encrypted_message); | ||
ciphertext | ||
} | ||
|
||
pub(super) fn read(&mut self, data: &[u8]) { | ||
let mut read_buffer = if let Some(buffer) = self.read_buffer.take() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would probably be more readable with https://doc.rust-lang.org/std/option/enum.Option.html#method.get_or_insert There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooooh, TIL. There are a bunch of places where I will replace that. |
||
buffer | ||
} else { | ||
Vec::new() | ||
}; | ||
|
||
read_buffer.extend_from_slice(data); | ||
self.read_buffer = Some(read_buffer); | ||
} | ||
|
||
/// Add newly received data from the peer node to the buffer and decrypt all possible messages | ||
pub fn decrypt_message_stream(&mut self, new_data: Option<&[u8]>) -> Vec<Vec<u8>> { | ||
let mut read_buffer = if let Some(buffer) = self.read_buffer.take() { | ||
buffer | ||
} else { | ||
Vec::new() | ||
}; | ||
|
||
if let Some(data) = new_data { | ||
read_buffer.extend_from_slice(data); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, this is kinda painful. Can we avoid copying the new data if we don't need to store it here? Maybe by returning an iterator over new messages that holds a |
||
} | ||
|
||
let mut messages = Vec::new(); | ||
|
||
loop { | ||
// todo: find way that won't require cloning the entire buffer | ||
let (current_message, offset) = self.decrypt(&read_buffer[..]); | ||
if offset == 0 { | ||
break; | ||
} | ||
|
||
read_buffer.drain(0..offset); | ||
|
||
if let Some(message) = current_message { | ||
messages.push(message); | ||
} else { | ||
break; | ||
} | ||
} | ||
|
||
self.read_buffer = Some(read_buffer); | ||
|
||
messages | ||
} | ||
|
||
/// Decrypt a single message. Buffer is an undelimited amount of bytes | ||
fn decrypt(&mut self, buffer: &[u8]) -> (Option<Vec<u8>>, usize) { // the response slice should have the same lifetime as the argument. It's the slice data is read from | ||
if buffer.len() < 18 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you constify or document here than a short read less than encrypted_message_length+MAC is invalid ? |
||
return (None, 0); | ||
} | ||
|
||
let encrypted_length = &buffer[0..18]; // todo: abort if too short | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this TODO was addressed above, right? |
||
let length_vec = chacha::decrypt(&self.receiving_key, self.receiving_nonce as u64, &[0; 0], encrypted_length).unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider naming this |
||
let mut length_bytes = [0u8; 2]; | ||
length_bytes.copy_from_slice(length_vec.as_slice()); | ||
let message_length = u16::from_be_bytes(length_bytes) as usize; | ||
|
||
let message_end_index = message_length + 18; // todo: abort if too short | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is 16-byte of MAC of the Lightning message counted in message length ? I'm not sure as l=len(m) computed before message encryption so shouldn't be 18 + message_length + 16 here? |
||
if buffer.len() < message_end_index { | ||
return (None, 0); | ||
} | ||
|
||
let encrypted_message = &buffer[18..message_end_index]; | ||
|
||
self.increment_receiving_nonce(); | ||
|
||
let message = chacha::decrypt(&self.receiving_key, self.receiving_nonce as u64, &[0; 0], encrypted_message).unwrap(); | ||
|
||
self.increment_receiving_nonce(); | ||
|
||
(Some(message), message_end_index) | ||
} | ||
|
||
fn increment_sending_nonce(&mut self) { | ||
Self::increment_nonce(&mut self.sending_nonce, &mut self.sending_chaining_key, &mut self.sending_key); | ||
} | ||
|
||
fn increment_receiving_nonce(&mut self) { | ||
Self::increment_nonce(&mut self.receiving_nonce, &mut self.receiving_chaining_key, &mut self.receiving_key); | ||
} | ||
|
||
fn increment_nonce(nonce: &mut u32, chaining_key: &mut [u8; 32], key: &mut [u8; 32]) { | ||
*nonce += 1; | ||
if *nonce == 1000 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a constant for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, you meant the 1,000 |
||
Self::rotate_key(chaining_key, key); | ||
*nonce = 0; | ||
} | ||
} | ||
|
||
fn rotate_key(chaining_key: &mut [u8; 32], key: &mut [u8; 32]) { | ||
let (new_chaining_key, new_key) = hkdf::derive(chaining_key, key); | ||
chaining_key.copy_from_slice(&new_chaining_key); | ||
key.copy_from_slice(&new_key); | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use ln::peers::conduit::Conduit; | ||
|
||
#[test] | ||
fn test_chaining() { | ||
let chaining_key_vec = hex::decode("919219dbb2920afa8db80f9a51787a840bcf111ed8d588caf9ab4be716e42b01").unwrap(); | ||
let mut chaining_key = [0u8; 32]; | ||
chaining_key.copy_from_slice(&chaining_key_vec); | ||
|
||
let sending_key_vec = hex::decode("969ab31b4d288cedf6218839b27a3e2140827047f2c0f01bf5c04435d43511a9").unwrap(); | ||
let mut sending_key = [0u8; 32]; | ||
sending_key.copy_from_slice(&sending_key_vec); | ||
|
||
let receiving_key_vec = hex::decode("bb9020b8965f4df047e07f955f3c4b88418984aadc5cdb35096b9ea8fa5c3442").unwrap(); | ||
let mut receiving_key = [0u8; 32]; | ||
receiving_key.copy_from_slice(&receiving_key_vec); | ||
arik-so marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
let mut connected_peer = Conduit { | ||
sending_key, | ||
receiving_key, | ||
sending_chaining_key: chaining_key, | ||
receiving_chaining_key: chaining_key, | ||
sending_nonce: 0, | ||
receiving_nonce: 0, | ||
read_buffer: None, | ||
}; | ||
|
||
let message = hex::decode("68656c6c6f").unwrap(); | ||
let mut encrypted_messages: Vec<Vec<u8>> = Vec::new(); | ||
|
||
for _ in 0..1002 { | ||
let encrypted_message = connected_peer.encrypt(&message); | ||
encrypted_messages.push(encrypted_message); | ||
} | ||
|
||
assert_eq!(encrypted_messages[0], hex::decode("cf2b30ddf0cf3f80e7c35a6e6730b59fe802473180f396d88a8fb0db8cbcf25d2f214cf9ea1d95").unwrap()); | ||
assert_eq!(encrypted_messages[1], hex::decode("72887022101f0b6753e0c7de21657d35a4cb2a1f5cde2650528bbc8f837d0f0d7ad833b1a256a1").unwrap()); | ||
assert_eq!(encrypted_messages[500], hex::decode("178cb9d7387190fa34db9c2d50027d21793c9bc2d40b1e14dcf30ebeeeb220f48364f7a4c68bf8").unwrap()); | ||
assert_eq!(encrypted_messages[501], hex::decode("1b186c57d44eb6de4c057c49940d79bb838a145cb528d6e8fd26dbe50a60ca2c104b56b60e45bd").unwrap()); | ||
assert_eq!(encrypted_messages[1000], hex::decode("4a2f3cc3b5e78ddb83dcb426d9863d9d9a723b0337c89dd0b005d89f8d3c05c52b76b29b740f09").unwrap()); | ||
assert_eq!(encrypted_messages[1001], hex::decode("2ecd8c8a5629d0d02ab457a0fdd0f7b90a192cd46be5ecb6ca570bfc5e268338b1a16cf4ef2d36").unwrap()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this is testing two different behaviors: chaining and key rotation? Ideally, tests should only test one behavior. Could we split this up? I realize this is the test data from BOLT 4, so happy to hear the argument if you think it should be one big test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, it is testing both. Arguably the key rotation is part of the encryption. I can split it up into two tests if you like, but in a separate commit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you test decryption in itself ? +1 for splitting into multiple tests and documenting what exactly is tested. |
||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like some of your files are missing a trailing newline character. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
pub struct ActOne( | ||
pub(super) [u8; 50] | ||
); | ||
|
||
pub struct ActTwo( | ||
pub(super) [u8; 50] | ||
); | ||
|
||
pub struct ActThree( | ||
pub(super) [u8; 66] | ||
); | ||
|
||
pub enum Act { | ||
One(ActOne), | ||
Two(ActTwo), | ||
Three(ActThree), | ||
} | ||
|
||
impl Act { | ||
pub fn serialize(&self) -> Vec<u8> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we name this something else? I almost responded with "why the hell would you ever want to serialize an Act for storage?" before I realized it meant "write act to a vec to send to a peer" (also, can we not return a slice here?). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's wrong with using lifetimes? The callsite can chose to store it in a Vec or copy to some local buffer if they wont want it. |
||
match self { | ||
Act::One(act) => { | ||
act.0.to_vec() | ||
} | ||
Act::Two(act) => { | ||
act.0.to_vec() | ||
} | ||
Act::Three(act) => { | ||
act.0.to_vec() | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
use bitcoin_hashes::{Hash, HashEngine}; | ||
use bitcoin_hashes::sha256::Hash as Sha256; | ||
|
||
pub(crate) struct HandshakeHash { | ||
pub(super) value: [u8; 32] | ||
} | ||
|
||
impl HandshakeHash { | ||
pub(super) fn new(first_input: &[u8]) -> Self { | ||
let mut hash = Self { | ||
value: [0; 32] | ||
}; | ||
let mut sha = Sha256::engine(); | ||
sha.input(first_input); | ||
hash.value = Sha256::from_engine(sha).into_inner(); | ||
hash | ||
} | ||
|
||
pub(super) fn update(&mut self, input: &[u8]) { | ||
let mut sha = Sha256::engine(); | ||
sha.input(self.value.as_ref()); | ||
sha.input(input); | ||
self.value = Sha256::from_engine(sha).into_inner(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this up one so its not in
ln
? If we're gonna put almost everything in one top-level module, it seems like we should just not have that module :).