Subject: Re: [tsvwg] Review of draft-ietf-tsvwg-natsupp-16

From: Michael Tuexen <[email-masked]>
Date: Mon, 13 Jul 2020 17:44:15 +0200

> On 2. Apr 2020, at 16:54, Gorry Fairhurst <[email-masked]> wrote:
> 
> I've reviewed this ID again in preparation for a WGLC and have one question that I suggest you address and some comments that I would like to see resolved before a WGLC, especially comments that concern the clarity of the RFC2119 normative language.
> 
> My question concerns section 6.6 on the Handling of Fragmented SCTP Packets:
> 
> This says /A NAT box MUST support IP reassembly of received fragmented SCTP
>    packets.  The fragments may arrive in any order./
> 
> - I understand this, but more explanation is needed here and a reference is needed to the section of the SCTP base spec that requires this. This can not just be stated, what are the implications?
The base spec does not talk about IP level fragmentation and reassembly.

I added some explanation at the beginning of the subsection:
<t>SCTP minimizes the use of IP-level fragmentation. However, it
can happen that using IP-level fragmentation is needed to continue
an SCTP association. For example, if the path MTU is reduced and there
are still some DATA chunk in flight, which require packets larger than
the new path MTU. If IP-level fragmentation can not be used, the SCTP
association will be terminated in a non-graceful way.</t>
> 
> - I'm curious whether the next sentence actually says also what the NAT does if fragementation can not be performed, i.e. it returns an ICMP message. In fact, if that happens what does SCTP do? because
> the data being sent can not be refragmented by SCTP? Please explain what happens.
I added:
This allows for a faster recovery from this packet drop.
> 
> Best wishes,
> 
> Gorry
> 
> (as TSVWG Document Shepherd)
> 
> -------------
> 
> My editorial (hopefully) comments are:
> 
> ---
> 
> The first use of /Stream Control Transmission Protocol/ in the introduction does not define SCTP, which I suggest would be useful, although obvious from the abstract!
Done.
> 
> ---
> 
> /The Verification Tag that the host holding the External-Address/
> 
> - could you cite the SCTP base spec after /Verification Tag/ so that a stupid reader knows where this is defined?
Sure.
> 
> ---
> 
> /The public address assigned to the NAT box which it uses/
> - I think this needs to be ", which" or "that". You will know how to fix.
Done.
> 
> ---
> 
> /Whereas for UDP and TCP this can be done
> 
>    very efficiently, for SCTP the checksum (CRC32c) over the entire
>    packet needs to be recomputed. /
> 
> - could you cite an RFC to explain the CRC32c.
The next sentence did. I put it in parentheses.
> 
> ---
> 
> /The modification of SCTP packets sent to the public Internet is easy./
> 
> - Easy is often regarded as a word expressing opinion and to avoid in RFCs. Please choose another explanation, or simplify the sentence.
Hmm. Maybe you were reading on older version of the document?
Revision 12 uses "easy", Revision 13 and later use "simple". Hope that is OK.
> 
> ---
> 
> /the packet has to be delivered to/to which the packet has to be delivered/
Fixed.
> 
> ---
> 
> / In this case the INIT MUST be dropped by
> 
>    the NAT and an ABORT MUST be sent back to the SCTP host with the
>    M-Bit set and an appropriate error cause (see Section 5.1.1 for the
>    format)./
> - It may not be clear that INIT and ABORT are special terms in SCTP,
> should this say /an ABORT chunk/, and /an INIT chunk/?
Yes. And I fixed similar language in the whole document.
> - Is /the SCTP host/the SCTP host that originated the packet/ or similar?
> - What does /sent back/ mean? - Is this an /SCTP packet must be sent to the source.../
> ===
Now using
In this case the packet containing the INIT chunk MUST be dropped by the NAT
and a packet containing an ABORT chunk MUST be sent to the SCTP host that
originated the packet with the M-Bit set and an appropriate error cause
(see <xref target='mbitabort'/> for the format).
> 
> /the INIT SHOULD be dropped/
> 
> - Is this the SCTP packet containing the INIT chunk?
Yes.
> 
> ---
> 
> /and an ABORT SHOULD be sent back to the SCTP host/
> 
> - Is this /an SCTP packet containing the ABORT chunk SHOULD/?
> 
> - Is /the SCTP host/the SCTP host that originated the packet/ or similar?
Yes.
> 
> ---
> 
> In 5.1:
> 
> /that are modified by this document./
> 
> - I'd prefer to more rigorous in the language. Please say  /that include the new fields specified by this document/ or something more.
Using
<t>This section presents existing chunks defined in <xref target='RFC4960'/>
for which additional flags are specified by this document.</t>
> 
> ---
> 
> In 6.1:
> 
> /An SCTP endpoint may be behind two NATs providing redundancy/
> 
> - Replace /may/could/ because this needs top be clearly not about what is allowed. I am also concerned that this is not clear whether the two NATs are successive, or in parallel. Please add text to explain.
Now using:
An SCTP endpoint can be behind two NAT devices in parallel providing redundancy.
> 
> ---
> 
> /and possibly modified handling procedures from those specified in [RFC4960]./
> 
> - Unclear to me what was intended by /possibly/ do you mean in some instances they do require, or that they could or could not require for some viewpoint? Please remove possibly and replace the language.
Now using:
<t>Each of these mechanisms requires additional chunks and parameters,
defined in this document, and modified handling procedures
from those specified in <xref target='RFC4960'/> as described below.</t>
> 
> ---
> 
> /this can't be used/this cannot be used/
Can't find anymore...
> 
> ---
> 
> /There MUST NOT be any IPv4 Address parameter, IPv6 Address parameter, or Supported Address Types parameter in the INIT-chunk. /
> 
> - To me this was not completely clear, would you agree with:
> 
> /The INIT-chunk MUST NOT contain an IPv4 Address parameter, IPv6 Address parameter, or Supported Address Types parameter. /
Fixed.
> 
> ---
> 
> /But this is very unlikely./However, this is unlikely./
Fixed.
> 
> ---
> 
> /can control the 16-bit Natted Port/ XXX
Changed to
A NAPT device can control the Port number and therefore avoid collisions
deterministically.</t>
> 
> ---
> 
> /However, in this unlikely event the NAT box MUST send an ABORT chunk
>    with the M-bit set if the collision is triggered by an INIT or INIT-
>    ACK chunk or send an ERROR chunk with the M-bit set if the collision
>    is triggered by an ASCONF chunk./
This text is in rev-12, but not 13 and higher.

> 
> - I have a strong preference for every RFC2119 declaration to be self-describing. /This/ is not clear, please restructure to explicitly say the unlikely event in the same sentence.
> 
> ---
> 
> /In the other two cases, the source and destination address and port numbers MUST be swapped./
> 
> - I have a strong preference for every RFC2119 declaration to be self-describing. /other cases/ is not clear, please restructure to explicitly state what.
Now using:
If a packet containing an INIT chunk or an ASCONF chunk,
> 
> ---
> 
> /The NAT MUST do the following when processing an
> 
>    INIT:
> 
>    o  If the INIT is destined to an external address and port for which
>       the NAT has no outbound connection, allow the INIT creating an
>       internal mapping table.
> 
>    o  If the INIT matches the external address and port of an already
>       existing connection, validate that the external server supports
>       the Disable Restart feature, if it does allow the INIT to be
>       forwarded.
> 
>    o  If the external server does not support the Disable Restart
>       extension the NAT MUST send an ABORT with the M-bit set./
> 
> - I have a strong preference to clarify the RFC2119 declaration.
> Please restructure as somnething like this:
The above text is only in rev12, not in later revisions. however, I think
the issue is fixed in new versions.
> 
> /The NAT performs following when processing an INIT:
> 
> 
>    o  If the INIT is destined to an external address and port for which
>       the NAT has no outbound connection, the NAT MUST allow the INIT creating an
>       internal mapping table.
> 
>    o  If the INIT matches the external address and port of an already
>       existing connection, the NAT MUST validate that the external server supports
>       the Disable Restart feature, if it does allow the INIT to be
>       forwarded.
> 
>    o  If the external server does not support the Disable Restart
>       extension the NAT MUST send an ABORT with the M-bit set./
> ---
> 
> /... error cause MUST be sent back./
> .... error cause MUST be sent to the SCTP host that originated the packet./
> - or similar.
Now using
error cause MUST be sent in response to the packet containing the ASCONF chunk
> ----
> /Please note that
>    such a packet containing an ERROR chunk SHOULD NOT be sent if the
>    received packet contains an ABORT, SHUTDOWN-COMPLETE or INIT-ACK
>    chunk. /
> - Please remove /Please not that/ from an RFC2119 clause, this can not be a note.
This text is not there anymore.
> 
> ---
> /In such a
>    case the NAT MUST send an ERROR chunk with the error cause code set
>    to 'VTag and Port Number Collision' (see Section 5.2.1)./
> - I have a strong preference for every RFC2119 declaration to be
> self-describing. /in such a case/If the NAT discovers an Internal Port Number and Verification
> Tag collision.../
But it is defined in the sentence before. If you don't want to have such
references, the sentences get very long...
> ---
> /The endpoint MUST do the
>    following:
> 
>    o  Validate that the verification tag is reflected by looking at the
>       VTag that would have been included in the outgoing packet.
>    o  Validate that the peer of the SCTP association supports the
>       dynamic address extension, if it does not discard the incoming
>       ERROR chunk.
>    o  If the association is attempting to add an address (i.e. following
>       the procedures in Section 6.7) then the endpoint MUST-NOT consider
>       the address part of the association and SHOULD make no further
>       attempt to add the address (i.e. cancel any ASCONF timers and
>       remove any record of the path), since the NAT has a VTag collision
>       and the association cannot easily create a new VTag (as it would
>       if the error occurred when sending an INIT).
>    o  If the endpoint has no other path, i.  e. the procedure was
>       executed due to missing a state in the NAT, then the endpoint MUST
>       abort the association.  This would occur only if the local NAT
>       restarted and accepted a new association before attempting to
>       repair the missing state (Note that this is no different than what
>       happens to all TCP connections when a NAT looses its state)./
> - Is not yet clear what is required, suggest:
> /
> /The endpoint processing is:
> 
>    o  The endpoint MUST validate that the verification tag is reflected by looking at the
>       VTag that would have been included in the outgoing packet.
>    o  The endpoint MUST validate that the peer of the SCTP association supports the
>       dynamic address extension, if it does not it MUST discard the incoming
>       ERROR chunk.
>    o  If the association is attempting to add an address (i.e. following
>       the procedures in Section 6.7) then the endpoint MUST NOT consider
>       the address part of the association and SHOULD make no further
>       attempt to add the address (i.e. cancel any ASCONF timers and
>       remove any record of the path), since the NAT has a VTag collision
>       and the association cannot easily create a new VTag (as it would
>       if the error occurred when sending an INIT).
>    o  If the endpoint has no other path, i.e. the procedure was
>       executed due to missing a state in the NAT, then the endpoint MUST
>       abort the association.  This would occur only if the local NAT
>       restarted and accepted a new association before attempting to
>       repair the missing state (Note that this is no different than what
>       happens to all TCP connections when a NAT looses its state)./
> /
Again, the above text is not in rev16...
> 
> ----
> /7.5.  Peer-to-Peer Communication
> 
>    If two hosts are behind NATs,/
> - Is that each behind a NAT or two behind the same NAT, I assume the former.
OK, using:
<t>If two hosts, each of them behind a NAT device, want to communicate with
each other, they have to get knowledge of the peer's external address.
> ---
> 
> Please note the recommendations in the yang review.
Done.