Re: [art] Artart last call review of draft-ietf-tsvwg-rfc4960-bis-15

[[e-mail]]

> On 4. Oct 2021, at 16:07, Eliot Lear via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Eliot Lear
> Review result: Almost Ready
> 
> 
> This review does not delve into security considerations, nor does it
> delve deeply into things that weren't changed across version (to the
> best I could determine).  There is at least one major exception to
> that below.
Hi Eliot,

thank you very much for the review. See my comments in-line.

Best regards
Michael
> 
> 
> Major
> 
> Section 3.2
> 
> Table 2 discusses processing of unknown chunks.  This sort of
> packet-level processing for unknown chunks strikes me as outdated.
I think we agree on that the handling of unknown chunks has to be
defined. They way it is done allows to select from a (small) list
of well defined options.
> Why isn't SCTP using some sort of extension negotiation?  I mean,
The SCTP extensions defined in RFCs actually negotiate support by
putting some parameters in the INIT and INIT ACK. So compliant
implementations of these extensions result in end points potentially
processing unknown parameters, but not unknown chunks.
> Telnet did that.  If you're going to update this spec, at least
> consider trying to do away with this aspect.  I also think it lends
The handling of unknown chunks and unknown parameters is part of the
ETSI test suite and therefore a well tested feature.
Removing it, would break backwards compatibility. This behaviour also
has not changed between RFC 4960 and RFC 4960bis and no issues with it
are known. So I'm hesitant to change anything here, except explicitly
being forced by the IESG.
> itself to silent protocol failures, and would fly in the face of
> draft-iab-use-it-or-lose-it.
> 
> 
> Similarly, consider the text in this note:
> 
>      Note: A robust implementation is expected to accept the chunk
>      whether or not the final padding has been included in the Chunk
>      Length.
> 
> I understand that you are attempting to adhere to Postel's Law, but
> you should discuss whether that is appropriate in this circumstance.
> Is this a protocol error?  If so, you should consider failing.
Well, it is not as expected, but you are still able to communicate.
Some implementations got is wrong initially. But they get it right now...
> 
> I'm not saying you MUST do away with this, but the WG should discuss.
This text is already present in RFC 4960 and here is the discussion of it:
https://datatracker.ietf.org/doc/html/rfc4460#section-2.3
> 
> This applies equally to Section 3.2.2.
Current protocol extensions negotiate its support by putting some
parameters in the INIT and INIT ACK. So not setting up associations
if there are unknown parameters would break interoperability.
> 
> 
> Section 6.1:
> 
>   B)  At any given time, the sender MUST NOT transmit new data to a
>       given transport address if it has cwnd + (PMDCS - 1) or more
>       bytes of data outstanding to that transport address.  If data is
>       available, the sender SHOULD exceed cwnd by up to (PMDCS - 1)
>       bytes on a new data transmission if the flightsize does not
>       currently reach cwnd.  The breach of cwnd MUST constitute one
>       packet only.
> 
> Can you explain the logic of the change?  People are now going to
Sure:
https://datatracker.ietf.org/doc/html/rfc8540#section-3.39
> start seeing packets that exceed cwnd, and that is likely to confuse
> existing receiving endpoints.  If you're going to leave this text in,
> I would go into detail somewhere as to when this should happen and
> why.  It just seems to violate the principle of least astonishment.
If you haven't does this before, you had a severe problem with CC.
> 
> Section 7.2
> 
> It seems to me that much of this section could use some pseudo-code to
> describe proper behavior.
Well, it does not specify one way how it needs to be done, but gives
some hints and constraints how to implement it in a TCP friendly way.
> 
> The TCB
> 
> I think somewhere it would be useful to show the minimal TCB.
Isn't that what Section 14 is about?
> 
> ALL of Section 10, but especially this text:
> 
>   These procedures differ from [RFC1122] and from its requirements for
>   processing of port-unreachable messages and the requirements that an
>   implementation MUST abort associations in response to a "protocol
>   unreachable" message.
> 
> Is this or any text in this section intended to modify any behavior
> specified in RFC 4443/STD89?  I'm specifically thinking of Sections
> 3.1 and 3.2.
No, I don't think so.
> 
> Minor
> 
> It would be good to do a quick rewrite of the abstract to make it more
> concise.
The first paragraph was requested during review.
I'm fine with removing the second paragraph, although it might be good
to provide some hints where SCTP is currently used.
The third paragraph list the services provided by SCTP. I think that
is useful information and I would prefer to keep it.
The last paragraph states that SCTP contains CC and protection against
blind attackers. Not sure if it is a good idea to remove this.

So what should be removed?
> 
> Section 2.3
> 
> Make clearer the relationship between PMDCS and PMTU with a calculation.
> 
> PMDCS = PMTU - SCTP header
> PMTU = PMDCS + SCTP header
> 
> Right?
Correct for RFC 4960. But if you use the SCTP Authentication extension (RFC 4895),
and it is negotiated that DATA chunks need to be authenticated, then you need
to take the AUTH chunk size into consideration.
> 
> In Section 2.5.5 I would suggest "Each chunk MUST contain..."  After
> all, what else could they contain?  "Might" seems inappropriate.
What about:
Each chunk contains either user data or SCTP control information.
> 
> The next paragraph:
> 
>   During times of congestion, an SCTP implementation MAY still perform
>   bundling even if the user has requested that SCTP not bundle.  The
>   user's disabling of bundling only affects SCTP implementations that
>   might delay a small period of time before transmission (to attempt to
>   encourage bundling).  When the user layer disables bundling, this
>   small delay is prohibited but not bundling that is performed during
>   congestion or retransmission.
> 
> This needs rewording.  For one thing, the last sentence is not
> grammatically correct.  Second, we do not use "prohibited".  Please
> use active tense with normative words like "MUST [NOT]".  More
> importantly, the text seems contradictory and is difficult to interpret.
OK. What about using the following as the complete text of Section 2.5.5:

<t>As described in <xref target='sec_sctp_packet_format'/>, the SCTP packet
as delivered to the lower layer consists of a common header followed by one
or more chunks.
Each chunk contains either user data or SCTP control information.
An SCTP implementation supporting bundling on the sender side might
delay the sending of user messages to allow the corresponding DATA
chunks to be bundled.</t>
<t>The SCTP user has the option to request that the SCTP stack does not
delay the sending of a user message just for this purpose.
However, even if the SCTP user has chosen this option, the SCTP might
delay the sending due to other reasons, for example due to congestion control
or flow control, and might also bundle multiple DATA chunks, if possible.</t>
> 
> 2.5.6
> 
>   The CRC32c checksum can be set
>   by the sender of each SCTP packet to provide additional protection
>   against data corruption in the network.  The receiver of an SCTP
>   packet with an invalid CRC32c checksum silently discards the packet.
> 
> I would prefer that you use normative language here.  The CRC32c
> MAY/MUST/SHOULD be.. and then then The receiver "MUST silently
> discard".  Also, it seems good at this stage to add what an unset
> value is (0?), so that the receiver knows when NOT to discard.
In Section 2.5 we only use very limit normative language. Section 6.8
describes the CRC32c computation in a detailed way with normative
language.

So what about replacing
The CRC32c checksum can be set by the sender
by
The CRC32c checksum is set by the sender
This avoids the possibility of an unset checksum.
> 
> Section 3:
> 
>   Multiple chunks can be bundled into one SCTP packet as long as the
>   size, except for the INIT, INIT ACK, and SHUTDOWN COMPLETE chunks.
> 
> Again, here you SHOULD be using normative language.
The next sentence has it:
These chunks MUST NOT be bundled with any other chunk in a packet.
> s/can/[MAY/MUST/SHOULD].  Also, I would reword this slightly as
> follows:
> 
>   INIT, INIT ACK and SHUTDOWN COMPLETE chunks MUST NOT be bundled
>   into one SCTP packet.  All other chunks MAY be bundled up to the
>   size indicated by the PMTU.  See section 6.10 for more details on
>   chunk bundling.
OK, I took your text.
> 
> 
> Section 3.2.1
> 
>     Chunk Parameter Length: 16 bits (unsigned integer)
> 
>      The Parameter Length field contains the size of the parameter in
>      bytes, including the Parameter Type, Parameter Length, and
>      Parameter Value fields.  Thus, a parameter with a zero-length
>      Parameter Value field would have a Parameter Length field of 4.
>      The Parameter Length does not include any padding bytes.
> 
> First, I don't understand why you added the word "Parameter" in the
> 4th line.
The field in the packet has the name "Parameter Length", not "Length".
I wanted to be precise.
>  Second, is the name of field Paramater Length or Chunk
> Parameter Length?  Please improve for clarity.
You are right. I just dropped the "Chunk" prefix to be consistent.
> 
> Directly below:
> 
>   The total length of a parameter (including Parameter Type, Parameter
>   Length, and Parameter Value fields) MUST be a multiple of 4 bytes.
>   If the length of the parameter is not a multiple of 4 bytes, the
>   sender pads the parameter at the end (i.e., after the Parameter Value
>   field) with all zero bytes.  The length of the padding is not
>   included in the Parameter Length field.  A sender MUST NOT pad with
>   more than 3 bytes.  The receiver MUST ignore the padding bytes.
> 
> Why is this in the definition of Chunk Parameter Value?  Doesn't it
> belong in Chunk Paramter Length?
It does not belong to any item of the list of fields. It is just a paragraph
after the list containing the fields of the parameter.
> 
> Section 3.3.1
> 
>  Res:
> 
> Please indicate the change here that these bits are reserved.
It is documented in:
https://datatracker.ietf.org/doc/html/rfc8540#section-3.45
> Changing packet field names to accommodate ASCII art should be done
> with care, as other documentation (like books) may reference them.
Please note that it is not only changing the way we state that
some bits are reserved (Reserved -> Res), but also the number of them.
But all this is documented in RFC 8540.
> 
> Section 3.3.2:
> 
>   If an INIT chunk is received with all mandatory parameters that are
>   specified for the INIT chunk, then the receiver SHOULD process the
>   INIT chunk and send back an INIT ACK.  The receiver of the INIT chunk
>   MAY bundle an ERROR chunk with the COOKIE ACK chunk later.  However,
>   restrictive implementations MAY send back an ABORT chunk in response
>   to the INIT chunk.
> 
> I don't understand the circumstances under which all but the first
> sentence apply.  Consider elaborating.  Same for 3.3.3.
This text is not new, but was added in the process RFC 2960 -> RFC 4960.
Here is a description of the change:
https://www.rfc-editor.org/rfc/rfc4460.html#section-2.49
> 
> Address type:
> 
> What should a receiver do if they see a Host Name Address parameter
> (Host name = 11)?  Specify behavior.
I added to Section 3.3.2.1.4:

The receiver of an INIT chunk or an INIT ACK containing a Host Name
Address parameter MUST send an ABORT chunk and MAY include an
"Unresolvable Address" error cause.
> 
> Section 3.3.4
> 
> 
>   The Gap Ack Blocks SHOULD be
>   isolated.  This means that the TSN just before each Gap Ack Block and
>   the TSN just after each Gap Ack Block have not been received.
> 
> and then later in that same section:
> 
>      Gap Ack Blocks SHOULD be isolated.  This means that
>      the DATA chunks with TSNs equal to (Cumulative TSN Ack + Gap Ack
>      Block Start - 1) and (Cumulative TSN Ack + Gap Ack Block End + 1)
>      have not been received.
> 
> Not ok.  You are stating a defintion in two different ways.  Pick one,
> and don't repeat the definition.
This change is
https://datatracker.ietf.org/doc/html/rfc8540#section-3.47

Not sure why it is bad to state it also in a concrete way. We duplicate
the fact the the TSN in the gap blocks have been received (instead of
are still missing) the same way. It was a try to be consistent.
But I agree, it is redundant, so I remove the concrete statement.
> 
> Section 6.8:
> 
>   Any hardware implementation SHOULD permit alternative verification of
>   the CRC in software.
> 
> What value is this statement, and why do you care?  Does it help
> interoperability?  Is this a matter of algorithm agility?  If so, I'll
> cleam there's a lot more work needing to be done.
Well, there is a big Ethernet NIC vendor which had the following bug
multiple times:

The checksum is computed from the beginning of the SCTP header to the end
of the packet. This is incorrect for short SCTP packets, since then you
have some padding at the end of packet to get the minimum ethernet frame
size. The NIC should take the length of the IP packet as indicated in the
IP header into account, but it did not.

So, at least in FreeBSD, we have code like
https://cgit.freebsd.org/src/tree/sys/netinet/sctp_input.c#n5715
> 
> Section 6.9:
> 
>   Once a user message is fragmented, it cannot be re-fragmented
> 
> s/cannot/MUST NOT/?
It is not that it is not allowed (MUST NOT), there is no way to do so.
That is why cannot is used.
> 
> 
> Section 7 1st paragraph:
> 
>   For some applications, it might be likely that adequate resources
>   will be allocated to SCTP traffic to ensure prompt delivery of
>   time-critical data -- thus, it would appear to be unlikely, during
>   normal operations, that transmissions encounter severe congestion
>   conditions.  However,...
> 
> The entire 1st paragraph could be replaced with one sentence:
> 
>   To manage congestion, the mechanisms and algorithms in this section
>   are to be employed.
> 
> The rest is just waffle, and best left for researchers to
> characterize.
When SCTP was initially deployed in signalling networks, the impact of
deploying CC was an important issue. The protocols it was replacing had
a completely different concept of load control. The links that were used
had a fixed bandwidth.

Since your comment indicates that this might not be important anymore, I
replace the text as you suggest. Since you are not citing the first
sentence "Congestion control is one of the basic functions in SCTP." of
section 7, I guess you want it to stay there.
> 
> In Section 8.2:
> 
>   When an outstanding TSN is acknowledged or a HEARTBEAT chunk sent to
>   that address is acknowledged with a HEARTBEAT ACK chunk, the endpoint
>   SHOULD clear the error counter of the destination transport address
>   to which the DATA chunk was last sent (or HEARTBEAT chunk was sent)
>   and SHOULD also report to the upper layer when an inactive
>   destination address is marked as active.
> 
> Do you have an example of how this notification should occur if the
> upper layer is the application?
The notification (the NETWORK STATUS CHANGE) is described in Section 11.2.3.
When using the socket API, this is described in
https://datatracker.ietf.org/doc/html/rfc6458#section-6.1.2
and if you want to see how this is used, see the example program in
https://datatracker.ietf.org/doc/html/rfc6458#appendix-B
> 
> 
> 
> Nits
> 
> A bit too aggressive with may->might.  In Section 2.3, the Bundling
> definition probably should be "can" instead of "might".  "Might"
> introduces some element of uncertainty.
Changed to "can".
> 
> Section 3.2, Chunk length
> 
> s/, if the Chunk/If the Chunk/
There was a 'Therefore' missing. So it reads now: Therefore, if the chunk
> 
> Section 3.3.1 U bit
> 
> s/,/Therefore,/
Fixed.
> 
> Oddly, same with Payload Protocol Identifier...
> 
> s/,/,therefore/
At some point of time, several 'therefore' got lost. My fault...
> 
> 
> Section 5.4
> 
> Redundant:
> 
>   However, it is possible that a
>   misbehaving peer might supply addresses that it does not own. 
> 
> Try:
> 
> However, a misbehaving peer might supply addresses that it does not
>   own.
Taken.
> 
> Section 6.9:
> 
>   If an implementation that supports fragmentation makes available to
>   its upper layer a mechanism to turn off fragmentation, it might do
>   so.
> 
> Well, yes ;-)  I would suggest the following rewrite:
> 
>  An implementation MAY make available its upper layer a mechanism to
>  turn off (disable?) fragmentation.  When in such a state, it MUST
>  behave as an implementation that does not support fragmentation.
Done (with using "disable").
> 
> Further down in Fragmentation:
>  The sender SHOULD choose the size of the DATA chunks is
>  smaller than or equal to the AMDCS.
> 
> s/chunks is/chunks that is/
Done.
> 
> 
> Section 7.2:
> 
>   When doing accounting for a DATA chunk related to one of these
>   variables
> 
> I suggest the following:
> 
>   When calculating one of these variables...
> 
> Let's leave accounting out of this ;-)
Done.
> 
> Section 10:
> 
> If you're not going to reference ICMP1-9, there's no point in labeling
> them as such, and you should just use a normal list.
I can do that. However, the references have been used in discussions and
references have been used in comments in SCTP implementations.
But if you prefer, I use be bullet list. Just let me know.
> 
> 
>