`noise` and `secio` framing broken for large payloads

[arnetheduck]

`noise` and `secio` framing broken for large payloads

[arnetheduck]

DefaultReadSize

this looks wrong - it's a helper method to read a varint-prefixed message which seem pretty common, but being a "generic" helper it should probably take the max length as a parameter

[sinkingsugar]

I swear I had exceptions in noise.. I must have removed them when fixing the integration tests.
But yes I knew this... the thing is that there is no real spec... go implements some packets fragmentation (broken) rust does nothing about it (afaik)
Also notice that noise has 2 payload lengths, plain/cipher

[arnetheduck]

why would you have exceptions for this case? the implementation should simply split the given payload into multiple fragments, no?

[sinkingsugar]

Because there is no clear spec on how that fragmentation should happen really, like where do you write the full size of the message, or anyway if we are a chunk or not, considering that plain len includes the "noise" and encrypted len is the overflowing one?

[arnetheduck]

abstractly, it's a stream - it has no messages. you will have to account for any additional noise framing when splitting the message up. readMessage/writeMessage are poorly named, they should be called something like readFrame or readFragment. the fragmentation is basically to split the data into pieces that the framing can support - just like when you send a 1gb file over tcp, it will be fragmented into little 1.4kb packets, each with its own header.

[sinkingsugar]

I get what you mean but I don't think it fits https://github.com/libp2p/specs/tree/master/noise#wire-format
I mean what you said can be done but it would work for us and maybe others.. but maybe not cos someone might interpret the spec in a different way thinking in "message"

e.g.

The noise_message field contains a Noise Message as defined in the Noise spec, which has a maximum length of 65535 bytes.

[sinkingsugar]

Altho

All data is segmented into messages with the following structure

Might imply fragments, I wish this was more explicit honestly tho.

Anyway I was supposed to implement it this way just like go implementation, just forgot about it and had reserves from the spec really.

[arnetheduck]

I'm not sure what you mean - there are two things going on here:

• LPStream has a write method that allows you to write an arbitrary number of bytes in a single call - - there is no concept of "message" here - just bytes - 1 or many - it doesn't matter if you call write once with 2 bytes or twice with 1 byte - the end result on the other end should be the same. In our implementation, the write method effectively delegates the writing to writeMessage through a maze of abstractions but the important part is that whoever calls write has no idea about these underlying frame sizes.

• noise and secio have frames, but this is an implementation detail - these frames are limited in size - 8mb for secio and 64kb for noise - whatever we send out from these protocols has to adhere to these limits - in secio, because of an explicit limit, in noise because the wire format simply doesn't support larger frames. it follows that it's the responsibility of the noise/secio implementation to take the stream of data it gets from the application and repackage it into noise frames when writing, then reconstruct the stream of bytes when it does the reading.

[sinkingsugar]

well I get it, I guess I keep forgetting that libp2p implicitly is always "streams" anyway.
So my doubts were kinda pointless. And like you pointed anyway Connection is an LPStream.

Anyway was already on my list (altho I had those reserves)
I will check if Secio needs this too and add it if so btw

[dryajov]

I think this means that we should fragment the stream of data on frame boundaries - 8mb and 64kb respectively. When streaming, once we reach the a frame boundary it should be sent out/flushed and another should be started right after that.

This only happens for chunks that are larger than the frame size.

[sinkingsugar]

Noise now can:
1550bea
I agree with @arnetheduck and that probably was one thing that mislead-ed me too, the writeMessage/readMessage API is confusing due to actually being a stream.

[sinkingsugar]

Btw should we implement fragmentation even in mplex? @dryajov
It has a smaller frame size, I think 1mb, I didn't think about that when adding the limit

[sinkingsugar]

Btw we have another issue with big sizes (more then 0x1200000), getting a Exception message: wrong varint size
I will try to figure it out

[sinkingsugar]

Ah we have a hard limt on DefaultReadSize in Connection.nim
So implementing this for secio was pointless, I will push it anyway tho.

btw do we want this limit? I guess yeah to avoid some attack

[sinkingsugar]

I agree, some users might want to go over the limits I suppose

[sinkingsugar]

All fixed in here
#117