Subject: Re: [tsvwg] draft-ietf-tsvwg-natsupp-12 - review comments

From: Michael Tuexen <[email-masked]>
Date: Sun, 17 Nov 2019 23:41:16 +0100

> On 18. Oct 2018, at 15:17, Proshin, Maksim <[email-masked]> wrote:
> 
> Hi Michael, Randall, Irene,
>  
> I’ve reviewed your work draft-ietf-tsvwg-natsupp-12 and have some comments listed below.
> My review is based on some experience of running SCTP instances (user-land) behind NAT in Linux. However it's more based on my experience with SCTP rather than with NAT.
Hi Maksim,

we discussed some item already face to face, but I'll go through the issue using in-line comments.
Thanks for a lot for the feedback!

Best regards
Michael
>  
> I list general comments first and then comments per particular sections.
>  
> ----- 
> 1.
> From my point of view it is hard to read the document due to several reasons and I think it can significantly be improved:
> - Some sections could be re-arranged and/or merged. Thus I think it's better to move Section 5 after Section 6. I would also merge Section 4.3 and 6. I also think Section 4..1 should be a part of Section 3 or separate.
> - As far as I can see the document basically describes 3 mechanisms: 1. SCTP support in NAT for SH case without the "Disable Restart" feature 2. SCTP support in NAT for MH case without the "Disable Restart" feature 3. "Disable Restart". I think it would be better to logically divide the document to 3 mechanisms.
> - In some sections, especially in subsections of Section 6, it's better to clearly state to whom it's addressed: SCTP developers, NAT developers or both. Maybe it's possible to split it in some cases, i.e. Section 6.x could consist of a subsection for SCTP and a subsection for NAT.
> - In many places when existing SCTP functionalities are mentioned, like fragmentation or restart, exact RFC number and section should be stated. 
I think this has already been addressed in the latest revision.
> -----
> 2.
> I think the mechanisms in this document are really complex for the NAT device. The document basically requires the NAT to be able to build SCTP chunks. I think it could be optional as in this case there will not be a problem rather than it may take more time. Perhaps also worse debugging. Anyway I don't see it as super critical. E.g. the NAT must send ABORT in case when INIT has the same Initial Tag. I think the NAT might drop it without ABORT. In that case SCTP will retransmit INIT several times and then abort the establishment with the same Initial Tag. Next time it will randomly choose a new VTag. 
> Or do I miss something?
We want to avoid the substantial delay introduced by the timer based retransmissions and give the end-point an indication
why the INIT dropped. That way the end-point can just start a new association right away, even without involving the SCTP user.
> Also it seems in most of the cases mapping of Int-VTag to Priv-Addr should be enough. Is it really required to keep the rest like External VTag in the table?  
Yepp. To map incoming packets with the T-bit set.
> -----
> 3.
> It seems the case when SCTP is behind the NAT and it's in the Server mode (accepts INITs) is not described clearly. There is only one paragraph about it:
>  
>    For an incoming packet containing an INIT-chunk a table lookup is
>    made only based on the addresses and port numbers.  If an entry with
>    an External-VTag of zero is found, it is considered a match and the
>    External-VTag is updated.
>  
> I think it’s not enough. What should be the behavior when an entry is not found? When it’s found but
You can't do anything...
> External VTag is not zero?
Drop it.
> As far as I understood the idea of the document is that the NAT is kind of a firewall for packets received from unknown external IP address (no entry found including incoming INIT case). Is it really so? Can it be improved in cases when the NAT trusts to some external IPs, i.e. there is a method to configure from which external IP addresses it should accept incoming INITs. In that case the NAT could also map incoming associations.
You can also have mechanisms not described in this document to handle exposed ports...
> Otherwise I see it as a serious limitation of the proposed solution and it should be clearly stated at the beginning of the document.
> -----
> 4.
> One important thing that I want to discuss is that the proposed approach basically requires the NAT to keep as many entries as the amount of associations established from/to all SCTP instances behind the NAT. It might be a very huge number – hundreds of thousands and even more in the future. It impacts performance and memory consumption in the NAT. It can also be a problem if the NAT devices need to be synchronized.  
> Have you thought about any idea that wouldn’t require to keep so many entries? One idea could be to somehow keep the Private Address (or any associated number with it) in each SCTP packet. In that case the NAT would only need to keep that information which basically will result in the number of entries be equal to the number of SCTP instances. Using that information the NAT would easily and very quickly forward packets.
As we discussed face to face, this might have some security implications. However, this document does not assume that it is
the only way to provide a NAT functionality. If you have an idea how to implement a NAT solution with possibly some different
properties, but better scalability, you can bring this up in an ID. Please note that some of the scalability impacts are
implementation dependent.
> -----
> 5.
> The document focuses on cases when entries are added into the table but doesn’t describe when and how they should be removed.
> I think processing of ABORT and SHUTDOWN COMPLETE/ACK should be added.
> The document also recommends a timer in Section 10 which I guess results in entries removal. I wonder if it should also be mentioned that if the NAT device is able to track liveness of SCTP instances associated by the private address, it should also remove entries when it’s dead..
I think the best way to remove the NAT state is to do it by a timer. You have to have one anyway. And taking
specific chunks into account makes the implementation harder in case of the packet containing that chunks is lost
after NAT processing. However, the ID does not forbid this. So if you have a setup where such processing makes sense,
you are free to add it.
> -----
> 6.
> I think it should clearly be stated that the document covers IPv4, IPv6 and dual IP cases (am I right?) but not Hostname Addresses.
They will be deprecated in RFC 4960bis... But for clarity, I added to Section 6.1:
The INIT chunk and the INIT-ACK chunk MUST NOT contain any Host Name parameters.
> -----
> 7. 
> The cases when INIT and ASCONF are retransmitted, i.e. sent from the same Priv-Addr are not covered. I think it’s better to clearly state or even have examples that in those cases there will be matching in the table also including Priv-Addr matching and the NAT should handle it like any other chunk/packet, i.e. replace the source address and send it further.
We describe where state is added, collisions are detected and so on. So we don't explicitly describe the case of retransmissions,
which means you find a matching entry. This is assumed all over the text.
> -----
> 8.
> I think the “Disable Restart” mechanism might bring issues and not all use cases are explained in the document.
> E.g. it might be the case that one of the SCTP hosts behind the NAT is lost and the server side will not discover it immediately. When the SCTP host is alive back it will try to establish the association back. In that case the NAT should drop INIT or not if it still has that entry? I guess it shouldn’t drop it or if there is no entry in the NAT already or if the association was re-established from another private address, SCTP on the server side will create a new association and will have the stale one and the new one simultaneously. If SCTP on the server side is idle and not sending HBs, it will have that stale association forever.
> I believe there might be other such cases and I think for the “Disable Restart” mechanism there should be recommendations like enabled Path Monitoring.  
> It also contradicts with the idea of RFC4960 to match the association by IP addresses and ports and I think some clarifications should be added, e.g. for OOTB processing.  
Yes, it does. I think the requirements are stated in the last sentence of Section 6.3.2.
> -----
> 9.
> Do you see any impact on PMTUD especially when the private address is IPv4 but the public one is IPv6 and vice versa?
> E.g. I wonder what should/will do the NAT when it gets ICMPv6 (Packet Too Big) but the private address is IPv4.
A NAT device changing the address family must also translated the ICMP/ICMPv6 messages. This is not specific to SCTP.
That is why we are not dealing with it explicitly.
> -----
> 10.
> There are many places where it’s said “we can see…”, “we assume…”, etc. I think it’s better to rephrase it.
This is already addressed in the current revision.
> -----
> 11.
> I think in all figures where you present how the NAT control block should look like it’s better to add “Disable Restart” as the NAT should also track it.
This parameter is not relevant for the examples. For space efficiency, I didn't include it.

For clarifying this, I added to the beginning of section 7:

<t>The NAT binding table entries shown in the following examples do not
include the flag indicating whether the restart procedure is supported or not.
This flag is not relevant for these examples.</t>
> -----
> -----
> 12.
> Section 1, Imp:
> I propose to clearly state that this document is addressed to NAT and SCTP and that all SCTP scenarios will only work if both SCTPs (local and remote) and the NAT support this document.
There is now a table describing this.
> -----
> 13.
> Section 1, Q: 
>  
>     only true NAT is available
>  
> What is “true NAT”? I think it should be rephrased.
That has already been addressed.
> -----
> 14.
> Section 1, Imp:
>  
>    note that this document focuses on the case where the NAT maps
>    multiple private addresses to a single public address.
>  
> I think it’s better to rephrase it as mapping of a single private address to a single public one is also covered. Also it might be worth to add “and vice versa”.
Done.
> -----
> 15.
> Section 1, Q/Imp:
> What do you really mean by
>  
>    To date,
>    specialized code for SCTP has not yet been added to most NATs so that
>    only true NAT is available. The end result of this is that only one
>    SCTP capable host can be behind a NAT and this host can only be
>    single-homed.
>  
> ?
> Today there are some NAT implementations which partly support SCTP.
> I propose to rephrase this text. I think what you want to say is that if the NAT device doesn’t support SCTP, it’s only possible to run a single SCTP host with SH endpoints behind that NAT. Would it be good to explain the reason? I.e. the NAT needs to track the connection between the private address and the association to make sure that packets are received by a correct SCTP host behind the NAT.
It has been rephrased to:

To date, specialized code for SCTP has not yet been
added to most NAT devices so that only a translation of IP addresses is
supported.

> -----
> 16.
> Section 1, Imp:
>  
>    This document describes an SCTP specific variant NAT and specific
>    packets and procedures to help NATs provide similar features of NAPT
>    in the single-point and multi-point traversal scenario.
>  
> packets => chunks?
> Also Error Causes and parameters.
It has been changed to

<t>This document specifies procedures allowing a NAT to support SCTP
by providing similar features to those provided by a NAPT for TCP and
other supported protocols.
The document also specifies a set of data formats for SCTP packets and a set
of SCTP endpoint procedures to support NAT traversal.

> -----
> 17.
> Section 1, Imp:
> Here and in other places like this after
>  
>    implementation supporting this extension will follow these procedures
>   
> “this extension”, “these procedures”, “these changes”, “this feature”, “the features” are used and it’s not exactly clear what you mean. Propose to rephrase it.
The text has substantially been improved. Does the latest version still has ambiguities?
> -----
> 18.
> Section 1, Imp:
>  
> At each level, the Internal Host, External Host and NAT
>    may or may not support the features described in this document.
>  
> I think you actually mean “SCTP on the Internal Host” and “SCTP on the External Host”. I propose to improve it. There are other places like this.
Hmm, I think this is clear, since the "features described in this document" refers to an SCTP stack.
> -----
> 19.
> Section 1, Imp:
>  
>    From the table we can see that when a NAT does not support the
>    extension no communication can occur.
>  
> I think it’s better to say that in general case when there are multiple SCTP hosts behind the NAT we can see…
> It would be also better to clarify that “communication” actually means the successful association establishment and traffic processing.
This has already been clarified.
> -----
> 20.
> Section 3, Imp:
> The basic scenario is when there are multiple SCTP EPs/ instances behind the NAT. I propose to extend Figure 1.
We don't use that in the examples below. The idea was to keep it as simple as possible.
> -----
> 21.
> Section 4.1.1, Ed:
> multi-home association => multi-homed association
> In this single point traversal scenario => In the single point traversal scenario
Fixed.
> -----
> 22.
> Section 4.1.2, Imp:
>    
>    This case does NOT apply to a single-homed SCTP association (i.e.,
>    BOTH endpoints in the association use only one IP address).
>  
> I would say in this case SCTP EP A is always MH but EP B can be either SH or MH. Propose to rephrase it.
It could also be that EP B is dualhomed and EP is single homed, but both NATs are used (due to the
dualhoming of EP B). So I think the text says this.
> -----
> 23.
> Section 4.2, Imp:
> I propose to clarify at the beginning of this section that SCTP packet header includes ports.
We assume in this document a level of familiarity with SCTP which definitely includes the knowledge
of the common header.
> -----
> 24.
> Section 4.2, Imp:
>  
>    this rule can be relaxed, if all entries with the
>    same Internal-Port and External-Port have the support for the restart
>    procedure enabled.
>  
> I think it’s better to say “if all entries with the same Internal-Port and External-Port have the support of “Disable Restart” procedure enabled”.
We don't use quotation marks elsewhere. So I would like to keep it consistent.
> -----
> 25.
> Section 4.3, Ed:
> DISABLE_RESTART => Disable Restart
> SCTP packets containing INIT-ACK chunks => SCTP packets containing INIT-ACK chunk
> INIT-ACK => INIT ACK (all places to align with RFC 4960 terminology) 
> COOKIE-ECHO => COOKIE ECHO (all places to align with RFC 4960 terminology)
> COOKIE-ACK => COOKIE ACK (all places to align with RFC 4960 terminology)
> INIT-chunk => INIT chunk (all places)
> INIT-collision => INIT collision
Fixed.
> -----
> 26.
> Section 4.3, Ma:
>    
>    However, it is
>    possible that there is already a NAT control block with the same
>    External-Address, External-Port, Internal-Port, and Internal-VTag but
>    different Private-Address.
>  
> There is no External-Address in the block!
Fixed. It was a left-over...
> There are many other places where External-Address is used like when “Disable Restart” procedure’s status is tracked. I think it should be tracked per association/control block.
It is tracked per control block.
> -----
> 27.
> Section 4.3, Ma:
>  
> It is also possible that a connection to External-Address and
> External-Port exists without an Internal-VTag conflict but the
> External-Address does not support the DISABLE_RESTART feature (noted
> in the NAT control block when the prior connection was established).
> In such a case the INIT SHOULD be dropped by the NAT and an ABORT
> SHOULD 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).
Cleaned up by using:
<t>It is also possible that a connection to External-Address and External-Port
exists without an Internal-VTag conflict but there exists a NAT binding
table entry with the same port numbers but a different Private-Address.
>  
> I think INIT MUST be dropped in such case (not SHOULD).
Changed.
> For the second SHOULD I would agree (see my earlier comment) but there are other places where MUST is used when the same use case is described (Section 6.3).
> -----
> 28.
> Section 4.3, Imp:
> It’s not described what to do in case of INIT is sent from the same private IP with the same ports with the same or different Initiate TAG and there is the entry. 
The same initiate tag would be a retransmission, so no action required. If there is a different
initiate tag, it means that the end-point has restarted. However, to avoid attacks, no state change
should occur. That is why we haven't described it.
> -----
> 29.
> Section 4.3, Imp:
>  
> The processing of outgoing SCTP packets containing no INIT-chunk is
>    described in the following figure..
>  
> It's only valid when the lookup is successful. Also there might be additinal actions in case of ABORT and SHUTDOWN COMPLETE/ACK.
The current thinking is that we don't specify that teardown message have to result in a NAT state change.
> -----
> 30.
> Section 4.3, Imp:
>  
> In the case Lookup fails, the SCTP packet is dropped.  The Update
>    routine inserts the External-VTag (the Initiate-Tag of the INIT-ACK
>    chunk) in the NAT state control block.
>  
> Split it or rearrange as the second sentence belongs to the successful lookup. Add MUST in both sentences. 
Clarified.
> -----
> 31.
> Section 4.3, Imp:
>  
>    The processing of incoming SCTP packets containing an ABORT or
>    SHUTDOWN-COMPLETE chunk with the T-Bit set is described in the
>    following figure.
>  
> I think it should be clearly stated that in that case VTag is reflected and this is why the NAT lookup should use VTag from the packet to match External VTag instead of Internal VTag.
I think this is clear due to the definition of the T-bit.
> -----
> 32.
> Section 4.3, Imp:
> I think it's better if the following paragraph:
>  
>    The processing of other incoming SCTP packets is described in the
>    following figure.
>  
> should actually go after the following paragraph:
>  
>    For an incoming packet containing an INIT-chunk a table lookup is
>    made only based on the addresses and port numbers.  If an entry with
>    an External-VTag of zero is found, it is considered a match and the
>    External-VTag is updated.
Done.
>  
> Also there might be additional actions in case of incoming ABORT and SHUTDOWN COMPLETE/ACK without the T-Bit set..
We don't specify these...
> -----
> 33.
> Section 4.3, Ed:
> containing Local-Address => containing Private Address
Fixed.
> -----
> 34.
> Section 4.3, Imp:
> The following paragraphs should be merged:
>  
>    For an incoming packet containing an INIT-chunk a table lookup is
>    made only based on the addresses and port numbers.  If an entry with
>    an External-VTag of zero is found, it is considered a match and the
>    External-VTag is updated.
>  
>    This allows the handling of INIT-collision through NAT.
Already fixed.
> -----
> 35.
> Section 4.3, Ma:
>  
>    For an incoming packet containing an INIT-chunk a table lookup is
>    made only based on the addresses and port numbers.  If an entry with
>    an External-VTag of zero is found, it is considered a match and the
>    External-VTag is updated.
>  
> It should be clearly stated what the NAT should do when the entry is not found and when it's found but External-VTag is not zero.
I think most NAT boxes will drop the packet. Some may have something specific handling configured.
> -----
> 36.
> Section 5.2, Q/Ma:
> Is it really needed to include the whole packet/chunk into ABORT/ERROR?
We only say as much as possible.
> What SCTP should do with it? Only the case with 'Internal Port Number and Verification Tag collision' and ASCONF when it’s needed is described in the document.
Trigger retransmits with different tags if possible, for example. For doing this you might want
to do input validation.
> -----
> 37.
> Section 5.2.1 and 5.2.3, Q:
> When INIT ACK would cause ABORT or ERROR with any of these Error Causes?
Yes.
> -----
> 38.
> Section 5.3.1, Q/Ma:
>  
>    This parameter is used to indicate that the RESTART procedure is
>    requested to be disabled.  Both endpoints of an association MUST
>    include this parameter in the INIT chunk and INIT-ACK chunk when
>    establishing an association and MUST include it in the ASCONF chunk
>    when adding an address to successfully disable the restart procedure.
>  
> I don't like MUST here.. As I said above there might be issues and some SCTP EPs/applications may want actually to prevent it so SHOULD or even MAY is better for me. Section 6.2 actually uses SHOULD. Section 6.5 says "the optional Disable Restart parameter". Also I think it should be enabled by the application and I guess Section 8 is actually aimed for that. What will happen if SCTP doesn't include it? Does it break something serious so that MUST is really needed?
It says: ... MUST include ... to successfully disable the restart procedure.
I think this is correct.
> -----
> 39.
> Section 6.1, Imp:
>  
>    o  An SCTP endpoint may be behind two NATs providing redundancy.  The
>       method to set up this scenario is discussed in Section 6.7.
>  
> It might be actually one NAT device which will then cover two logical NATs. Might be worth to mention it.
I don't think we refer to physical devices, but more about the number of instances having a NAT binding table.
> -----
> 40.
> Section 6.2, Ma:
>  
>    Every association MUST initially be set up single-homed.  There MUST
>    NOT be any IPv4 Address parameter, IPv6 Address parameter, or
>    Supported Address Types parameter in the INIT-chunk.  The INIT-ACK
>    chunk MUST NOT contain any IPv4 Address parameter or IPv6 Address
>    parameter.
>  
> There is a contradiction in the document. Everywhere it's said that only SH association must be established while the document only considers the use cases when the local SCTP behind the NAT is the client so in that case only the local EP should be actually SH. The use case from Section 7.2 actually describes when the peer is MH.
> On the other hand if the document is improved by the case when the local SCTP behind the NAT is the server then it will be valid to require SH EPs on both ends.
> Then there is the third use case - INIT collision where it's also required to have both INITs sent without IPs in the body.
Good catch:
Changed to 
<t>Every association setup from a host behind a NAT MUST NOT use multiple
private addresses.
There MUST NOT be any IPv4 Address parameter, IPv6 Address parameter, or Supported
Address Types parameter in the INIT chunk. The INIT ACK chunk MUST NOT
contain any IPv4 Address parameter or IPv6 Address parameter using non-global
addresses.
The INIT chunk and the INIT ACK chunk MUST NOT contain any Host Name
parameters.</t>

> It could be also simplified if the NAT doesn't support INIT collision at all. Why is it so important?
We wanted to describe that it is possible to do it. P2P is important is some scenarios.
> -----
> 41.
> Section 6.2, Ma:
>  
>    Every association MUST initially be set up single-homed.  There MUST
>    NOT be any IPv4 Address parameter, IPv6 Address parameter, or
>    Supported Address Types parameter in the INIT-chunk.  The INIT-ACK
>    chunk MUST NOT contain any IPv4 Address parameter or IPv6 Address
>    parameter.
>  
> Also when sending INIT ACK SCTP must not include local/private IP address in the cookie.
We never specify what is in a cookie...
> -----
> 42.
> Section 6.3, Q/Ma:
> 
>    The sender of the packet containing the INIT chunk or the receiver of
>    the INIT-ACK chunk, upon reception of an ABORT chunk with M-bit set
>    and the appropriate error cause code for colliding NAT table state is
>    included, MUST reinitiate the association setup procedure after
>    choosing a new initiate tag, if the association is in COOKIE-WAIT
>    state. 
>  
> What is a reason to have MUST here? It could be better to use SHOULD as it will allow SCTP implementations to stop association setup immediately and inform the application that something is wrong if they choose such behavior. 
Changed to SHOULD.
> -----
> 43.
> Section 6.3, Im:
>  
>    In any other state, the SCTP endpoint MUST NOT respond.
>  
> I think it should be rephrased as there is no any response actually. The point is that in the other state than COOKIE-WAIT SCTP must not try to reestablish the association.
I think the statement is short and clear...
> -----
> 44.
> Section 6.4, Ed:
> Punctuation in the following paragraph should be improved:
>  
>    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.
Already improved.
> -----
> 45.
> Section 6.4, Imp:
>  
>    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.
>  
> There is no External Address in the table!
> Int-VTag should also be checked and shouldn’t match actually.
Fixed in the text.
> -----
> 46. 
> Section 6.5, Imp:
>  
>    If the NAT box receives a packet from the internal network for which
>    the lookup procedure does not find an entry in the NAT table, a
>    packet containing an ERROR chunk is sent back with the M-bit set.
>    The source address of the packet containing the ERROR chunk MUST be
>   the destination address of the incoming SCTP packet.  The
>    verification tag is reflected and the T-bit is set.  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.  An ERROR chunk MUST NOT be sent if the received packet
>    contains an ERROR chunk with the M-bit set.
>  
> Also excluding INIT chunk case.
I would not consider that a "Missing state" case. For the INIT it is not assumed to be there.
> -----
> 47. 
> Section 6.5, Q/Ma:
>  
>    If the NAT box receives a packet from the internal network for which
>    the lookup procedure does not find an entry in the NAT table, a
>    packet containing an ERROR chunk is sent back with the M-bit set.
>    The source address of the packet containing the ERROR chunk MUST be
>   the destination address of the incoming SCTP packet.  The
>    verification tag is reflected and the T-bit is set.  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.  An ERROR chunk MUST NOT be sent if the received packet
>    contains an ERROR chunk with the M-bit set.
>  
> Should the packet be sent further in that case? I believe it should clearly be stated.
No. I added text for this case.
> -----
> 48. 
> Section 6.5, Imp:
>  
>    o  Generate a new ASCONF chunk containing the VTags parameter (see
>       Section 5.3.2) and the Disable Restart parameter if the
>       association is using the disabled restart feature.  By processing
>       this packet the NAT can recover the appropriate state.  The
>       procedures for generating an ASCONF chunk can be found in
>       [RFC5061].
>  
> It would be worth to clarify that the Disable Restart parameter should be added only if both EPs support this feature.
That is the meaning of "the association is using"
> Also add the reference to Section 5.3.1.
Done.
> -----
> 49. 
> Section 6.5, Imp:
> 
>    Or, if the address is already part of the association,
>    the SCTP endpoint MUST NOT respond with an error, but instead should
>    respond with an ASCONF-ACK chunk acknowledging the address but take
>    no action (since the address is already in the association).
>  
> should => SHOULD
Already fixed.
> -----
> 50. 
> Section 6.5, Ed:
> MUST-NOT => MUST NOT
> i.  e. => i.e.
> disabled restart => Disable Restart
Fixed.
> -----
> 51. 
> Section 6.7, Imp:
>  
>    The address to add is the wildcard address and the lookup
>    address SHOULD also contain the VTags parameter and optionally the
>    Disable Restart parameter as illustrated above.
>   
> A concrete section should be referenced instead of "above".
I just removed the reference.
> -----
> 52.
> Section 6.7, Ed:
> multi-point => multi point (or vice versa, in all places)
> single-point => single point (or vice versa, in all places)
Fixed.
> -----
> 53.
> Section 6.7, Imp:
> 
>    If a multi-homed SCTP endpoint behind a NAT connects to a peer, it
>    SHOULD first set up the association single-homed with only one
>    address causing the first NAT to populate its state.
>  
> SHOULD => MUST
Fixed.
> -----
> 54. 
> Section 7, Imp:
> It would be good to extend a few examples by DATA packets (also retransmitted DATA in Section 7.2).
The only interesting thing is the manipulation of the NAT binding table. Processing of DATA chunks
does not affect it.
> -----
> 55. 
> Section 7.1 – 7.5, Imp:
>  
>    A NAT entry is created, the source address is substituted and the
>    packet is sent on:
>  
> I think it’s better to say that the NAT discovers that it’s INIT chunk…
>  
>    NAT updates entry:
>  
> Also it’s better to say that the NAT discovers that it’s INIT ACK, matches the entry by VTag, Port, … finds the Private Address, replaces the dst address by it…
I think that is clear from the figure...
> -----
> 56. 
> Section 7.2, Q/Ed:
>  
>    NAT does need to change the table for second address:
>  
> Should it be “NAT does not need to change the table for second address:”?
Fixed.
> -----
> 57.  
> Section 7.2, Ma:
> The following two paragraphs are contradicting:
>   
>    The server (Host B) includes its two addresses in the INIT-ACK chunk,
>    which results in two NAT entries.
>  
> NAT does not need to change the table for second address:
Fixed. Also a left over...
> -----
> 58.  
> Section 7.3, Q:
>  
>    ASCONF [ADD-IP=0.0.0.0, INT-VTag=1234, Ext-VTag = 5678]
>    10.1.0.1:1 --------> 203.0.113..129:2
>             Ext-VTag = 5678
>  
> Why is it sent to the second External IP? Is it a requirement?
You want to get the packet through NAT2.
> Also should it be ADD-IP=0.0.0.0 => ADD-IP= 10.1.0.1 ?
No. You add the wildcard address.
> -----
> 59.  
> Section 7.3, Q/Ma:
>  
>    Host B includes its second address in the INIT-ACK, which results in
>    two NAT entries in NAT 1.
>  
> Isn’t it wrong?
Also a left-over.
> -----
> 60.  
> Section 7.4, Imp:
> I think it’s better to firstly show the empty table as the NAT lost its state.
Fixed.
> -----
> 61.  
> Section 7.4, Imp:
> I think it would be good to clarify that SCTP should send as many ASCONF packets as many addresses it has (am I right?).
But that is the general procedure...
> -----
> 62. 
> Section 10, Ma/Imp:
>  
>    State maintenance within a NAT is always a subject of possible Denial
>    Of Service attacks.  This document recommends that at a minimum a NAT
>    runs a timer on any SCTP state so that old association state can be
>    cleaned up.
>  
> Not sure how that risk is actually possible as the NAT will not accept incoming INITs but anyway according to my experience it's not just enough to have the timer as in this case a NAT can mistakenly remove entries. It also requires SCTP to help the NAT to keep the state up to date by e.g. sending packets, e.g. HBs, which also requires timers synchronization between SCTP and the NAT.
That is correct. Since you have a default value for the HEARTBEAT.Interval, you can configure the NAT timer, Do you think the HB timer
should be configured differently?
> -----
> 63.  
> Section 8, Q/Ma:
> What do you mean by “NAT friendless”? Why should it be even enabled? Either SCTP support it or not. Or do you mean “Disable Restart”? If so, I believe it should be renamed.
It covers not only the restart feature but also the single homed setup / add additional address per ASCONF...
> Shouldn’t it be per local EP rather than per association?
When getting the state, you need an association level.
> -----
> BR,
> Maxim

---

From: Maksim Proshin <[email-masked]>
Date: Tue, 19 Nov 2019 09:56:03 +0000

Hi Michael,

Thanks for your feedback and addressing my comments!

I have some follow-up below inline (with [Maxim] tag). Please take a look.

I think all major comments have been resolved, maybe 1 comment about SHOULD vs MUST still remains below but that should not stop the document to go to WGLC in my view.

BR, Maxim

________________________________
From: Michael Tuexen <[email-masked]>
Sent: 18 November 2019 01:41
To: Proshin, Maksim
Cc: Randall Stewart; Irene Rüngeler; [email-masked]
Subject: Re: [tsvwg] draft-ietf-tsvwg-natsupp-12 - review comments

Hi Maksim,

Best regards
Michael
I think this has already been addressed in the latest revision.
[Maxim] Yes, I think it's better now

We want to avoid the substantial delay introduced by the timer based retransmissions and give the end-point an indication
why the INIT dropped. That way the end-point can just start a new association right away, even without involving the SCTP user.
[Maxim] I understood that reason but I don't like MUST here. Can it be replaced by SHOULD or you have strong arguments against that?

Yepp. To map incoming packets with the T-bit set.
[Maxim] Yes, it's reasonable but not mandatory in my view compared to e.g. Int-VTag.  That was my point.  Do you think it's a bad idea to add a sentence explaining why Ext-VTag is needed? Btw, I propose to use "T bit" instead to align with RFC4960.

You can't do anything...
> External VTag is not zero?
Drop it.
[Maxim] Would be good to mention it in the draft.

You can also have mechanisms not described in this document to handle exposed ports...
[Maxim] Yes, we discussed it and I'm fine with it. Anyway, for me it would be really useful if the draft in the beginnig has a sentence explaining it.

I think the best way to remove the NAT state is to do it by a timer. You have to have one anyway. And taking
specific chunks into account makes the implementation harder in case of the packet containing that chunks is lost
after NAT processing. However, the ID does not forbid this. So if you have a setup where such processing makes sense,
you are free to add it.
[Maxim] Ok, I thought that was the idea of the draft actually but not described well. At least NAT implementations with SCTP support that I saw so far try to use ABORT/SHUTDOWN chunks and the timer, i.e. both ways. I'm actually fine if the draft describes the timer-based approach only but I thought it's logical to also use the chunks as the draft already prescribes to handle them specifically.

[Maxim] Good!

We describe where state is added, collisions are detected and so on. So we don't explicitly describe the case of retransmissions,
which means you find a matching entry. This is assumed all over the text.
[Maxim] For me it would be worth to mention it once in the beginning.

Yes, it does. I think the requirements are stated in the last sentence of Section 6.3.2.
[Maxim] Perhaps I misunderstand because I don't see how Section 6.3.2 addresses the problem that I described above. In my view there is some conflict here. In one hand, in order to work with different clients behind the NAT, Disable Restart feature should be enabled and this is what Section 6.3.2 prescribes. On the other hand, when Disable Restart is enabled, the server can't detect the restart and will always treat it as a new association and might keep the old one forever. Is it the intention of the proposed mechanism? If so, I propose to clearly describe this issue in the draft and propose some mitigation actions, at least path monitoring.  Isn't it a potential security attack when one attacker client disables restart and retriggers the same association many many times consuming only resources for that one association until the server dies due to lack of resources?

[Maxim] I see. Thanks!

<t>The NAT binding table entries shown in the following examples do not
include the flag indicating whether the restart procedure is supported or not.
This flag is not relevant for these examples.</t>
[Maxim] Ok, I think it's enough.

There is now a table describing this.
[Maxim] Well, it was already in version -12 that I reviewed so I'm not sure what exactly I wanted to improve. Perhaps I wanted the draft from the beginning to clearly state that it describes mechanisms that should be implemented in NAT, SCTP client and SCTP server.

Done.

[Maxim] It's ok

It has been changed to

[Maxim] It's ok

The text has substantially been improved. Does the latest version still has ambiguities?
[Maxim] I think it's fine now.

Hmm, I think this is clear, since the "features described in this document" refers to an SCTP stack.
[Maxim] Actually I think the draft uses "host" in two different meanings. Anyway, it's not really a serious problem so up to you.

Fixed.
Fixed.
Cleaned up by using:
Changed.
The same initiate tag would be a retransmission, so no action required. If there is a different
initiate tag, it means that the end-point has restarted. However, to avoid attacks, no state change
should occur. That is why we haven't described it.
[Maxim] I think it would be good to clarify it in the draft. Is it possible?

Clarified.
Done.
Fixed.
Already fixed.
I think most NAT boxes will drop the packet. Some may have something specific handling configured.
[Maxim] Do you mean that it was not described intentionally? This is the reason I have related questions or proposals for clarification. Perhaps it would be worth to mention in the beginning that processing of SCTP chunks in use cases not described in the draft are up to implementations.

Yes.
It says: ... MUST include ... to successfully disable the restart procedure.
I think this is correct.
[Maxim] Ok, I think you mean that it's not MUST to support it in general but MUST if the client wants to disable the restart procedure.

Good catch:
Changed to
[Maxim] It looks fine now

We never specify what is in a cookie...
[Maxim] Ok, I think it's fair enough.

> What is a reason to have MUST here? It could be better to use SHOULD as it will allow SCTP implementations to stop association setup immediately and inform the application that something is wrong i…
Changed to SHOULD.
[Maxim] Good

Already improved.
Fixed in the text.
Done.
Already fixed.
Fixed.
Fixed.
Fixed.
Fixed.
[Maxim] Yes, I see now.
Also a left-over.
Fixed.
But that is the general procedure...
[Maxim] Ok, I see now that the draft actually mentions it, at least in Section 6.6.
That is correct. Since you have a default value for the HEARTBEAT.Interval, you can configure the NAT timer, Do you think the HB timer
should be configured differently?
[Maxim] No, I don't think it should be different. That was one my point that it would be good if the draft recommends to configure NAT timer based on HB.Interval. It's not needed to provide a precise formula. Moreover as I wrote earlier it should also recommend/prescribe to enable Path monitoring on SCTP ends. Anyway, my initial point here was whether it's really a subject of possible DoS attacks because NAT only creates entries from locally originated INITs.