provides setup for maxInboundPayloadSize and maxFrameSize

[OlegDokuka]

closes #864

Signed-off-by: Oleh Dokuka shadowgun@i.ua

[OlegDokuka]

@rstoyanchev right now this implementation terminates the connection if the reassembly limit is overflowed. Should we consider logical stream termination instead?

The second option is a little challenging since this is a connection and not an RSocket one

[rstoyanchev]

Overall looks like a good start. Some initial thoughts below.

@rstoyanchev right now this implementation terminates the connection if the reassembly limit is overflowed. Should we consider logical stream termination instead? The second option is a little challenging since this is a connection and not an RSocket one

Could it perhaps release the accumulated fragments, send an error through the Sink, and then continue to discard further fragments received for that stream?

[rstoyanchev]

I think this should apply to any input payload size regardless of whether it is fragmented or not. In fact by default fragmentation is off and in any case once there is a limit in place for fragmented input, it would be expected to have the same for non-fragmented input. I do understand that the rest is at the transport level. Nevertheless from an application perspective, conceptually, it's one limit.

[OlegDokuka]

That is another concern. Spec specifies that Max-Frame size if 16MB, thus it should be clear that RSocket should support that size. Now the question is - if one applies a 1byte memory limit, how a client, which expects that allowed frame size is 16MB, should behave in that case.

From the fragmentation perspective, minimal frame size is 64bytes, so reassembly limit should be more than that at least, but if we wanna send something bigger than that, all tries will fail, so what is the benefit of such reassembly then?

From my point of view, the minimal storage per stream should be at least the default max frame size. Or at least minimal fragment size specified by spec

[rstoyanchev]

If the limit is reached on the very last fragment I think it should be allowed through. It is in memory at this point anyway and the purpose of the limit should be more to prevent further growth after the limit is exceeded or otherwise there would be cases where it fail for even 1 byte over. It should be lenient about that.

[OlegDokuka]

well. Disagree. This will open another spot of the attack. Since now anyone can flash anything in the pipe, it can configure fragmentation to send the very last frame == MAX FRAME SIZE (16MB) so if the limit of in-memory storage is 16KB, then the last frame will do 16MB + 16KB. Which means - one can use it as a vulnerability

[OlegDokuka]

Could it perhaps release the accumulated fragments, send an error through the Sink, and then continue to discard further fragments received for that stream?

I don't sink discarding is a good idea since we will not be able to respond back with any signal.

Also, Sink for ReassemblyDuplexConnection means connection sink so if we send error it automatically terminates transport connection and disposes of RSocket.

If we don't wanna terminate the connection, then we must do it according to the spec - Send error to the responder (depends on the stream type - if it is request-response - ignore payload and send nothing to the responder, but then send an error back to the requester saying something in the error message). If reassembly of an ordinary payload - cancel upstream and error downstream

[OlegDokuka]

moved maxFrameLength property to the transport, since it describes transport behavior and from my point of view checking that when a connection has already wired is too late

[OlegDokuka]

Also, this one is a default property, so it should not be a breaking change for anybody

[rstoyanchev]

This looks solid now. Some minor comments related to naming.