coapjs / node-coap

CoAP - Node.js style
MIT License
528 stars 154 forks source link

Fix blockwise response logic #376

Closed edrose closed 5 months ago

edrose commented 8 months ago

Overview

This is a fix for #375 where the server will attempt to generate a packet greater than the maximum allowable, which was set to 1280 bytes. This would occur if the size of the payload was less than 1280 bytes, but the size of the resulting CoAP message was greater than 1280 bytes after the headers were added.

The bug was introduced 9 years ago in d727d430d7c744a516d4c68d156ed6bf8262f7b7 and has some incorrect logic for dealing with message sizes. It would be very easy to simply revert those changes and call it a fix, but I wanted to try and understand what the author was trying to implement and to fix the implementation, rather than simply removing functionality.

In addition I've updated the tests to be more robust and to catch errors like this in the future.

Motivation and Context

The intent of the above commit, I believe, was to allow the maximum packet size to be adjusted for cases where the server is running over a network stack that has an MTU different to that of ethernet. A 6LoWPAN/Thread network stack, for example, will have an MTU significantly less than that of ethernet so the server can be configured to perform blockwise transfers earlier to avoid message fragmentation at the IP layer. This is done with an IP_MTU constant and a maxPacketSize parameter that control when blockwise transfers are applied.

Unfortunately the phrase 'packet' and 'MTU' are used all over the place and don't really refer to anything meaningful in the context of the application layer. The IP MTU is the size of the whole IP packet, including all headers and payload. The IP_MTU constant defaults to 1280 and is used only to check the CoAP payload, missing the CoAP headers, UDP header, and IP header when performing the check.

The CoAP specification does not ever refer to anything as a 'packet' - it refers to 'messages'. A 'packet' is the IP layer 3 data unit, and I think this ambiguity of naming in the library has caused confusion in the past when features are implemented.

Changes Introduced

Even though the maximum CoAP message size is based on the IP MTU, it's not practical to try and determine the MTU for every response and apply blockwise logic accordingly. Furthermore the header size of the IP layer is not fixed, and when using 6LoWPAN neither is the UDP header size. So instead the changes here follow the recommendations set out in RFC 7252 Section 4.6 which state:

If the Path MTU is not known for a destination, an IP MTU of 1280 bytes SHOULD be assumed; if nothing is known about the size of the headers, good upper bounds are 1152 bytes for the message size and 1024 bytes for the payload size.

Those recommendations are reflected in the defaults that are implemented, and can be adjusted from the server parameters.

Tests

Update tests in test/blockwise.ts to check the response code of the server when checking responses and to move the test payload size to within the bounds of the edge case that this is fixing.

Server Code

Change the parameters file to remove the IP_MTU constant. This was replaced with two constants: MAX_PAYLOAD_SIZE and MAX_MESSAGE_SIZE. Those changes are reflected in the parameters by removing the maxPacketSize, replacing it with maxPayloadSize and maxMessageSize respectively.

The blockwise logic has been updated to be more efficient and understandable, using the provided maxPayloadSize parameter. The check for whether a response needs to be split into blocks is now correct, fixing the initial bug.

The maxMessageSize parameter is used when calling coap-packet:generate() and is passed as the maxLength argument. This argument, when not explicitly passed, defaults to 1280 which (as discussed) does not represent the maximum allowable message size. Now an error will be returned from that function when the total CoAP message size is greater than the max allowable set in the server. This error should now only occur if the server is misconfigured.

Potentially Breaking Changes

The changes to the parameters might be breaking changes, depending on how they are used in other projects. I'm happy to rename them if required in order to maintain some form of backwards compatibility, at the expense of the names being less clear.