feature: Allow partial act messages to be received (discussion)

[julianknutsen]

The RFC mentions that any payloads smaller than the act sizes should result in an error. However, it specifically mentions network reads. Since developers of this library are implementing their own network stack, it is possible that the peer_handler code could receive partial sequential read() calls that result in a valid act.

Relevant RFC line:

https://github.com/lightningnetwork/lightning-rfc/blob/master/08-transport.md#handshake-exchange

Receiver Actions:

1. Read exactly 50 bytes from the network buffer.

Is it worth supporting users of the library that send the ACT messages in chunks?

from @ariard:

Can you point to me where the requirement is stated ? I fear that deviating from the spec would open you to some byte-counting fragmentation attack, where an infrastructure entity deliver cipher byte by byte to the decoder and such learn ciphertext boundaries. This kind of concern is really theoretical given deployment of authenticated/encrypted protocols above the standard IP stack. That said it would be nice to conserve this security property as this module minimizes assumptions on the underlying link/network layer and such could be deployed on wireless communications channels, less-prone to infrastructure manipulation.

Relevant patch:

feature: Allow partial act messages to be received

The previous implementation would error and subsequently cause
a disconnect if a partial act was sent to the state machine, even
if future calls could be added to the read buffer to create a
valid act.

The RFC mentions that an error should be generated if the act is the
incorrect size, but since this code doesn't directly read off the
network it is possible it receives a single act message over
multiple calls.

Implement this behavior and add the appropriate tests.

0001-feature-Allow-partial-act-messages-to-be-received.patch.txt

[TheBlueMatt]

Yes, its absolutely critical that we be able to read partial messages. TCP is all kinds of unreliable and only provides a stream, not "reads". I believe this is working fine on master, the above patch seems to be against #494.

[julianknutsen]

I went through master & #494 to verify the current behaviors. Matt is correct that master will just wait until it receives the full act bytes. #494 has a regression that will disconnect the peer if it receives < act bytes. I'll make these notes in the relevant lines of the review and ensure we keep the behavior the same.

A simple patch to show the behavior difference using the tokio tests:

diff --git a/lightning-net-tokio/src/lib.rs b/lightning-net-tokio/src/lib.rs
index a7818d7..b31b094 100644
--- a/lightning-net-tokio/src/lib.rs
+++ b/lightning-net-tokio/src/lib.rs
@@ -296,13 +296,15 @@ pub fn setup_outbound<CMH, RMH, L>(peer_manager: Arc<peer_handler::PeerManager<S
        #[cfg(debug_assertions)]
        let last_us = Arc::clone(&us);
 
-       let handle_opt = if let Ok(initial_send) = peer_manager.new_outbound_connection(their_node_id, SocketDescriptor::new(us.clone())) {
+       let handle_opt = if let Ok(mut initial_send) = peer_manager.new_outbound_connection(their_node_id, SocketDescriptor::new(us.clone())) {
                Some(tokio::spawn(async move {
                        // We should essentially always have enough room in a TCP socket buffer to send the
                        // initial 10s of bytes. However, tokio running in single-threaded mode will always
                        // fail writes and wake us back up later to write. Thus, we handle a single
                        // std::task::Poll::Pending but still expect to write the full set of bytes at once
                        // and use a relatively tight timeout.
+                       // initial_send.extend(&[1]);
+                       initial_send.truncate(20);
                        if let Ok(Ok(())) = tokio::time::timeout(Duration::from_millis(100), async {
                                loop {
                                        match SocketDescriptor::new(us.clone()).send_data(&initial_send, true) {

I think we can close this issue out as resolved.

[arik-so]

Thanks for the patch, @julianknutsen!

[ariard]

Yes, its absolutely critical that we be able to read partial messages. TCP is all kinds of unreliable and only provides a stream, not "reads". I believe this is working fine on master, the above patch seems to be against #494.

@julianknutsen, after skimming over Noise specs again, byte-fragmentation to discover ciphertext boundaries is only a concern during the authentication phase, not during act handshake as it does happen in clear. Matt is right that how we process data packets from unreliable stream at the net layer is a different issue that how we process ciphertexts at the authentication layer. I'm leaning to think that BOLT 8 is incomplete wrt to byte-fragmentation concern, we should pad payload and process them constantly to avoid guessing cipher content.

Anyway that's beyond the scope of this issue and we can mark it as closed.

[julianknutsen]

Thanks for the analysis @ariard. Closing this out as required.