-
Notifications
You must be signed in to change notification settings - Fork 417
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 14 commits
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
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
use util::byte_utils; | ||
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 const TAG_SIZE: usize = 16; | ||
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. Seems like there's a few places where this can be used throughout the file. |
||
|
||
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(&byte_utils::le64_to_array(nonce)); | ||
|
||
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(&byte_utils::le64_to_array(nonce)); | ||
|
||
let length = tagged_ciphertext.len(); | ||
if length < 16 { | ||
return Err("ciphertext cannot be shorter than tag length of 16 bytes".to_string()); | ||
} | ||
let end_index = length - 16; | ||
let ciphertext = &tagged_ciphertext[0..end_index]; | ||
let authentication_tag = &tagged_ciphertext[end_index..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,200 @@ | ||
//! Handles all over the wire message encryption and decryption upon handshake completion. | ||
|
||
use ln::peers::{chacha, hkdf}; | ||
use util::byte_utils; | ||
|
||
pub(super) type SymmetricKey = [u8; 32]; | ||
|
||
const MESSAGE_LENGTH_HEADER_SIZE: usize = 2; | ||
const TAGGED_MESSAGE_LENGTH_HEADER_SIZE: usize = MESSAGE_LENGTH_HEADER_SIZE + chacha::TAG_SIZE; | ||
|
||
const KEY_ROTATION_INDEX: u32 = 1000; | ||
|
||
/// Returned after a successful handshake to encrypt and decrypt communication with peer nodes. | ||
/// It should not normally be manually instantiated. | ||
/// Automatically handles key rotation. | ||
/// For decryption, it is recommended to call `decrypt_message_stream` for automatic buffering. | ||
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 documentation is out-of-date. |
||
pub struct Conduit { | ||
Comment on lines
+13
to
+17
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. The word "conduit" is another name for "channel", which I assume you are trying not to overload. That said, nothing about this struct's behaviors fit the definition of those words. That is, the struct is not responsible for transferring (flowing) data from a source to a destination; the caller does that. Rather, it is simply responsible for encrypting and decrypting data that is transferred. Further, the fields of the struct are essentially divided into analogous "sending" and "receiving" fields used for encrypting and decrypting respectively. And these fields are disjoint. Therefore, I'd recommend dividing this struct into two structs called Then your enum PeerState {
Authenticating(PeerHandshake),
Connected(Encryptor, Decryptor),
} 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 makes perfect sense and I would love to do that, though what would you call the module? 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. How about simply "encryption.rs"? Or you could break them into "encryptor.rs" and "decryptor.rs" with key rotation utilities living elsewhere. 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. Maybe. But I'd reserve it for a separate PR 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 propose to use name |
||
pub(crate) sending_key: SymmetricKey, | ||
pub(crate) receiving_key: SymmetricKey, | ||
|
||
pub(crate) sending_chaining_key: SymmetricKey, | ||
pub(crate) receiving_chaining_key: SymmetricKey, | ||
|
||
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 { | ||
/// Encrypt data to be sent to peer | ||
pub fn encrypt(&mut self, buffer: &[u8]) -> Vec<u8> { | ||
let length = buffer.len() as u16; | ||
let length_bytes = byte_utils::be16_to_array(length); | ||
|
||
let mut ciphertext = vec![0u8; TAGGED_MESSAGE_LENGTH_HEADER_SIZE + length as usize + chacha::TAG_SIZE]; | ||
|
||
ciphertext[0..TAGGED_MESSAGE_LENGTH_HEADER_SIZE].copy_from_slice(&chacha::encrypt(&self.sending_key, self.sending_nonce as u64, &[0; 0], &length_bytes)); | ||
self.increment_sending_nonce(); | ||
|
||
ciphertext[TAGGED_MESSAGE_LENGTH_HEADER_SIZE..].copy_from_slice(&chacha::encrypt(&self.sending_key, self.sending_nonce as u64, &[0; 0], buffer)); | ||
self.increment_sending_nonce(); | ||
|
||
ciphertext | ||
} | ||
|
||
pub(super) fn read(&mut self, data: &[u8]) { | ||
let read_buffer = self.read_buffer.get_or_insert(Vec::new()); | ||
read_buffer.extend_from_slice(data); | ||
} | ||
|
||
/// Decrypt a single message. If data containing more than one message has been received, | ||
/// only the first message will be returned, and the rest stored in the internal buffer. | ||
/// If a message pending in the buffer still hasn't been decrypted, that message will be | ||
/// returned in lieu of anything new, even if new data is provided. | ||
pub fn decrypt_single_message(&mut self, new_data: Option<&[u8]>) -> Option<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 (current_message, offset) = self.decrypt(&read_buffer[..]); | ||
read_buffer.drain(0..offset); // drain the read buffer | ||
self.read_buffer = Some(read_buffer); // assign the new value to the built-in buffer | ||
|
||
if offset == 0 { | ||
return None; | ||
} | ||
|
||
current_message | ||
} | ||
|
||
/// Decrypt a message from the beginning of the provided buffer. Returns the consumed number of bytes. | ||
fn decrypt(&mut self, buffer: &[u8]) -> (Option<Vec<u8>>, usize) { | ||
if buffer.len() < TAGGED_MESSAGE_LENGTH_HEADER_SIZE { | ||
// A message must be at least 18 bytes (2 for encrypted length, 16 for the tag) | ||
return (None, 0); | ||
} | ||
|
||
let encrypted_length = &buffer[0..TAGGED_MESSAGE_LENGTH_HEADER_SIZE]; | ||
let mut length_bytes = [0u8; MESSAGE_LENGTH_HEADER_SIZE]; | ||
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 feels kinda awkward not to cache this result somehow - like, if we know we're gonna need 65KB of data to read the full message, decrypting the message header every time we get a new packet of 1.5KB seems super wasteful. |
||
length_bytes.copy_from_slice(&chacha::decrypt(&self.receiving_key, self.receiving_nonce as u64, &[0; 0], encrypted_length).unwrap()); | ||
// message_length is the length of the encrypted message excluding its trailing 16-byte tag | ||
let message_length = byte_utils::slice_to_be16(&length_bytes) as usize; | ||
|
||
let message_end_index = TAGGED_MESSAGE_LENGTH_HEADER_SIZE + message_length + chacha::TAG_SIZE; | ||
if buffer.len() < message_end_index { | ||
return (None, 0); | ||
} | ||
|
||
let encrypted_message = &buffer[TAGGED_MESSAGE_LENGTH_HEADER_SIZE..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 SymmetricKey, key: &mut SymmetricKey) { | ||
*nonce += 1; | ||
if *nonce == KEY_ROTATION_INDEX { | ||
Self::rotate_key(chaining_key, key); | ||
*nonce = 0; | ||
} | ||
} | ||
|
||
fn rotate_key(chaining_key: &mut SymmetricKey, key: &mut SymmetricKey) { | ||
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 hex; | ||
use ln::peers::conduit::Conduit; | ||
|
||
#[test] | ||
/// Based on RFC test vectors: https://github.com/lightningnetwork/lightning-rfc/blob/master/08-transport.md#message-encryption-tests | ||
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 mut remote_peer = Conduit { | ||
sending_key: receiving_key, | ||
receiving_key: sending_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. |
||
|
||
for _ in 0..501 { | ||
// read two messages at once, filling buffer | ||
let mut current_encrypted_message = encrypted_messages.remove(0); | ||
let mut next_encrypted_message = encrypted_messages.remove(0); | ||
current_encrypted_message.extend_from_slice(&next_encrypted_message); | ||
let decrypted_message = remote_peer.decrypt_single_message(Some(¤t_encrypted_message)).unwrap(); | ||
assert_eq!(decrypted_message, hex::decode("68656c6c6f").unwrap()); | ||
} | ||
|
||
for _ in 0..501 { | ||
// decrypt messages directly from buffer without adding to it | ||
let decrypted_message = remote_peer.decrypt_single_message(None).unwrap(); | ||
assert_eq!(decrypted_message, hex::decode("68656c6c6f").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 some of your files are missing a trailing newline character. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
pub const ACT_ONE_LENGTH: usize = 50; | ||
pub const ACT_TWO_LENGTH: usize = 50; | ||
pub const ACT_THREE_LENGTH: usize = 66; | ||
Comment on lines
+1
to
+3
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. There are still places in mod.rs where these aren't used. 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. In places where it's not a concrete act, but either 1 or 2, they shouldn't be used. But I found one spot to replace |
||
|
||
/// Wrapper for the first act message | ||
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'd like to get a better sense as to when separate files should be used for some of these smaller structs. In particular, handshake/acts.rs, handshake/hash.rs, and handshake/states.rs are used almost exclusively in handshake/mod.rs. And they constitute less than 100 lines total. What criteria did you used to split these out? Do they deserve separate sub-modules when the structs within them are themselves named using their sub-module name and are not independently 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. Frankly, just easier navigation through the components in the file system. 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. Given these are small and either are implementation details or form the public interface of handshake/mod.rs, I'd prefer if they were all in the same file. Otherwise, navigation between files is a bit excessive, IMHO. 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 would prefer to not stuff the handshake logic with all the accessory structs. I would be open to having one separate file where both the enums/states and the acts would live? 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 don't see them as accessories but rather core parts of the handshake module's interface (acts.rs) and implementation (states.rs). The former requires the user to include two things when using the interface. The latter requires the reader to reference another file when reading the implementation details. I'd concede that an argument can be made for keeping hash.rs as a separate file since it is encapsulating functionality. 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. For what it's worth, so does the Act enum |
||
pub struct ActOne( | ||
pub(super) [u8; ACT_ONE_LENGTH] | ||
); | ||
|
||
/// Wrapper for the second act message | ||
pub struct ActTwo( | ||
pub(super) [u8; ACT_TWO_LENGTH] | ||
); | ||
|
||
/// Wrapper for the third act message | ||
pub struct ActThree( | ||
pub(super) [u8; ACT_THREE_LENGTH] | ||
); | ||
|
||
/// Wrapper for any act message | ||
pub enum Act { | ||
One(ActOne), | ||
Two(ActTwo), | ||
Three(ActThree), | ||
} | ||
|
||
impl Act { | ||
/// Convert any act into a byte vector | ||
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(ref act) => { | ||
act.0.to_vec() | ||
} | ||
&Act::Two(ref act) => { | ||
act.0.to_vec() | ||
} | ||
&Act::Three(ref 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 :).