Re: [tsvwg] Review of draft-ietf-tsvwg-natsupp-20

[Michael Tuexen <[e-mail]>]

> On 4. Aug 2020, at 10:19, Timo Völker <[e-mail]> wrote:
> 
> Hi,
> 
> here is my review for the SCTP NAT Support draft (draft-ietf-tsvwg-natsupp-20). I found only minor issues.
Hi Timo,

thanks for your review. See my comments in-line.

Best regards
Michael
> 
> I'll start with typos and wording, followed by suggestions to rearrange or rewrite some text. 
> 
> *Typos, wording, etc.* (syntax: /old text/new text/)
> 
> Section 1
> .... "With the widespread deployment of Network Address Translators (NAT), specialized code has been added to NAT functions for TCP that allows multiple hosts to reside behind a NAT /functions/function/ using internal addresses (see [RFC6890]) and yet share //a /single IPv4 address, even when two hosts (behind a NAT function) choose the same port numbers for their connection." ...
Fixed.
> .... "/The/This/ document also specifies a set of data formats for SCTP packets and a set of SCTP endpoint procedures to support NAT traversal." ....
Fixed.
> 
> Section 4.3
> .... "Furthermore//,/ this section focuses on the single point traversal scenario." ...
Fixed. Let's see if the RFC Editor will remove it...
> 
> Section 6 
> "/When/If/ an SCTP endpoint is behind an SCTP-aware NAT a number of problems may arise as it tries to communicate with its peer:" ...
Fixed.
> .... "/When/If/ an SCTP endpoint is a server communicating with multiple peers and the peers are behind the same NAT function, then /the two endpoints/these peers/ cannot be distinguished by the server. This case is discussed in Section 6.3." ...
Fixed.
> .... "/Each of these mechanisms requires/The mechanisms to solve these problems require/ additional chunks and parameters, defined in this document," ...
Fixed.
> 
> Section 6.2.2
> .... "upon reception of a packet /containign/containing/ an ABORT chunk with M bit set and the appropriate error cause code for colliding NAT binding table state is included," ...
Fixed (also reported by Maxim). 
> .... "The sender of// the/ packet containing the ASCONF chunk, upon reception of a packet containing an ERROR chunk with M bit set, MUST stop adding the path to the association."
Fixed.
> 
> Section 6.3.1
> .... "If the packet containing the INIT chunk is originating from an internal port to /an/a/ remote port for which the NAT function has no matching NAT binding table entry," ...
Fixed.
> 
> Section 6.4.1
> .... "If the NAT /device/function/ receives a packet// from the internal network/ for which it has no NAT binding table entry and the packet contains an ASCONF chunk with the VTags parameter," ...
Fixed.
> 
> Section 6.4.2
> .... "The peer SCTP endpoint receiving such a packet containing an ASCONF chunk SHOULD/ either// add the address and respond with an acknowledgment/,// if the address is new to the association (following all procedures defined in [RFC5061]). /Or, if/If/ the address is already part of the association, the SCTP endpoint MUST NOT respond with an error, but instead SHOULD respond with// a/ packet containing an ASCONF ACK chunk acknowledging the address and take no action (since the address is already in the association)." ...
> .... "It MUST validate that the peer of the SCTP association supports the dynamic address extension. If the peer does not support /it/this extension/, /the NAT function/it/ MUST discard the incoming packet containing the ERROR chunk." ...
Fixed.
> 
> 
> *Rearranging, rewriting, etc.*
> 
> Definition of incoming and outgoing.
> Sometimes an incoming packet for a NET function just means that the NAT function receives a packet (from any direction). Sometimes it means implicit, that the packet came from the public internet. The term outgoing packet is used for the other direction. It might makes the text clearer if either the incoming and outgoing terms are described in the Terminology section or if the direction is described explicitly in the text (e.g. /incoming packet/packet coming from the public internet/).
I think it is best to use the terms consistently. After double checking, outgoing seems to be used
consistently. 
So I changed in section 6.4.1

The source address of the packet containing the ERROR chunk MUST be the
destination address of the incoming SCTP packet.

to

The source address of the packet containing the ERROR chunk MUST be the
destination address of the packet received from the internal network.

In Section 6.4.2 I changed two times

If the validation fails, discard the incoming packet containing the ERROR chunk.

to

If the validation fails, discard the received packet containing the ERROR chunk.

and

If the peer does not support this extension, it MUST discard the incoming packet
containing the ERROR chunk.

to

If the peer does not support this extension, it MUST discard the received packet
containing the ERROR chunk.
> 
> 
> Uniqueness of NAT binding table entries (section 4.3)
> I'm not sure if I understand this correctly. Two entries with the same Internal-Port and Remote-Port are OK, if the hosts *disabled* the restart procedure and used different VTags, right? If yes, then the fifth paragraph in section 4.3 needs to be changed.
> "This rule can be relaxed, if all NAT binding table entries with the same Internal-Port and Remote-Port have the support for the restart procedure /enabled/disabled/."
Good catch! Fixed.
> 
> Motivation (section 4.3)
> Section 4.3 seems a bit overloaded for a motivation. I would remove the edge cases here and describe them in section 6.
I sort of agree. The section title 'Motivation' is misleading. I would like to keep the
description as is.
I changed the title to 'Motivation and Overview'.
> * remove everything from (incl.) "Normally a NAT binding table entry will be created." to (excl.) "The processing of outgoing SCTP packets containing no INIT chunks is described in the following figure.".
> * remove the paragraph that starts with "For an incoming packet containing an INIT chunk a table lookup is made only based"
> -> The NAT binding table collision is already described in 6.2 and 6.3.
> -> The part about NAT table entry exists when INIT or ASCONF received and the lookup when an INIT received (i.e. only by ports) could be added to 6.1 (e.g. in a subsection NAT function considerations)
> The section 4.3 should then end with a note that more details can be found in section 6.
No changes performed.
> 
> SCTP NAPT? (section 6.2)
> The last sentence of the first paragraph of section 6.2 mentions a NAPT device. This confuses me. Is there a NAPT specification for SCTP that can do better here? If not, how about removing this sentence?
Done.
> 
> ASCONF chunk integration (section 6.4.1)
> In the first paragraph of section 6.4.1 there is a sentence that says when not to send an ERROR chunk. The ASCONF chunk is not mentioned here. How about:
> .... "Such a packet containing an ERROR chunk SHOULD NOT be sent if the received packet contains// an ASCONF chunk with the VTags parameter or/ an ABORT, SHUTDOWN COMPLETE or INIT ACK chunk." ...
Fixed.
> Also, it seems better to switch the position of paragraph 2 and 3, because the following section (6.4.2) starts with a reference to a packet containing an ERROR chunk, which is described in paragraph 2. Switching the position brings it closer together.
Done.
> 
> Wildcard address? (section 6.6)
> I don't understand the last sentence of section 6.6.
> "The address to add is the wildcard address" - which address is to be added and why has it to be the wildcard address?
> "the lookup address SHOULD also contain the VTags parameter" - what is the lookup address?
This was sloppy language. I changed it to a more precise variant:
The address used in the Add IP address parameter is the wildcard address
(0.0.0.0 or ::0) and the address parameter in the ASCONF chunk SHOULD also
contain the VTags parameter and optionally the Disable Restart parameter.
> 
> Timo