Skip to content

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

Closed
wants to merge 34 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
eb6a371
build scaffold for handshake module substitution support
arik-so Feb 12, 2020
b71b7ea
change peer_handler.rs to use modular handshake and encryption handle…
arik-so Feb 12, 2020
92eac9b
make ephemeral private key explicit for handshake (todo: remove it fr…
arik-so Feb 12, 2020
986f25f
remove import of rand
arik-so Feb 12, 2020
ffbf5ec
make linter complain less about docs
arik-so Feb 12, 2020
19b7700
allocate act messages without vector prevarication
arik-so Feb 13, 2020
17fda75
reduce vector allocations for message encryption
arik-so Feb 13, 2020
8169b31
address some of Jeff's comments pertaining to message decryption, con…
arik-so Feb 19, 2020
f1002c5
Use type standin for remaining act lengths when parsing. Use the same…
arik-so Feb 19, 2020
f0fc10b
Improve comments and type aliasing for handshake module.
arik-so Feb 20, 2020
eb297f9
Merge branch 'master' into modular_handshake
arik-so Feb 20, 2020
5492717
Merge branch 'master' into modular_handshake
arik-so Feb 20, 2020
256b6f5
Reflect new modular encryption mechanism in tock ping message creation.
arik-so Feb 20, 2020
b4921e9
Elaborate on lightning codec in conduit's decrypt method.
arik-so Feb 21, 2020
299b6f7
Split up conduit unit tests by tested functionality.
arik-so Feb 21, 2020
944177a
Make handshake store the remote public key instead of passing an opti…
arik-so Feb 21, 2020
0fbd895
Panic when attempting invalid state transitions.
arik-so Feb 27, 2020
6cf5a07
Merge branch 'master' into modular_handshake
arik-so Mar 12, 2020
6f4e76a
Group peer handler's connected state checks instead of repeating them…
arik-so Mar 12, 2020
c2227b6
Fix missing init message send upon connection initiation.
arik-so Mar 13, 2020
6bae489
Merge branch 'master' into modular_handshake
arik-so Apr 9, 2020
2df93ca
fix some unit tests
arik-so Apr 9, 2020
eda13bf
Disconnect peer if act message is too short.
arik-so Apr 9, 2020
4e6b25a
Replace unwrapping public keys with handleable errors in handshake mo…
arik-so Apr 9, 2020
f1940e9
Merge branch 'master' into modular_handshake
arik-so Apr 11, 2020
4deb290
Split conduit into encryptor and decryptor components (to allow for b…
arik-so Apr 30, 2020
5e9c350
Fix conduit constructor bugs and revert indentation for message proce…
arik-so Apr 30, 2020
4b4cb98
Merge remote-tracking branch 'upstream/master' into modular_handshake
arik-so Apr 30, 2020
029bb66
Replace hashing and secp256k1 dependencies with components of the bit…
arik-so Apr 30, 2020
54b7464
Restrict conduit borrow scope for compatibility with Rust 1.22.0.
arik-so Apr 30, 2020
fe705a9
Fix lightning-net-tokio peer handler import.
arik-so Apr 30, 2020
be5e2a5
Apply message handling extraction to relocated peer handler.
arik-so Jun 12, 2020
2e4e659
Merge branch 'master' into modular_handshake
arik-so Jun 12, 2020
a4fff76
Fix unit tests
arik-so Jun 13, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lightning/src/ln/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ pub mod channelmanager;
pub mod channelmonitor;
pub mod msgs;
pub mod router;
pub mod peers;
Copy link
Collaborator

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 :).

pub mod peer_handler;
pub mod chan_utils;
pub mod features;
Expand All @@ -27,7 +28,8 @@ mod onion_utils;
mod wire;

#[cfg(test)]
#[macro_use] mod functional_test_utils;
#[macro_use]
mod functional_test_utils;
#[cfg(test)]
mod functional_tests;
#[cfg(test)]
Expand Down
34 changes: 34 additions & 0 deletions lightning/src/ln/peers/chacha.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use util::chacha20poly1305rfc::ChaCha20Poly1305RFC;

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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())
}
}
173 changes: 173 additions & 0 deletions lightning/src/ln/peers/conduit.rs
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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make constants for these? Or reuse some from secp256k1::constants?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>>,
Copy link
Collaborator

Choose a reason for hiding this comment

The 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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof. Lets avoid allocating a Vec for two bytes somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 &mut self to to this Conduit we can structure it so that it reads from the existing data first, then from the new data, then when the Iterator is dropped pushes the still-pending data into the buffer.

}

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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eb6a371

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider naming this decrypted_length rather than encoding the type in the name. It makes the relationship between it and encrypted_length above explicit. The actual types aren't important in understanding the code conceptually.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eb6a371

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a constant for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);

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());
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eb6a371

Do you test decryption in itself ? +1 for splitting into multiple tests and documenting what exactly is tested.

}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some of your files are missing a trailing newline character.

33 changes: 33 additions & 0 deletions lightning/src/ln/peers/handshake/acts.rs
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> {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps to_vec? We could return a slice, but would have to mark its lifetime as linked to the act object's.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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()
}
}
}
}
25 changes: 25 additions & 0 deletions lightning/src/ln/peers/handshake/hash.rs
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();
}
}
Loading