[tsvwg] draft-ietf-tsvwg-natsupp-12 - review comments

["Proshin, Maksim" <[e-mail]>]

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.

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.
-----
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?
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?
-----
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 External VTag is not zero?
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.
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.
-----
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.
-----
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.
-----
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.
-----
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.
-----
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.
-----
10.
There are many places where it's said "we can see...", "we assume...", etc. I think it's better to rephrase it.
-----
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.
-----
-----
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.
-----
13.
Section 1, Q:

    only true NAT is available

What is "true NAT"? I think it should be rephrased.
-----
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".
-----
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.
-----
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.
-----
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.
-----
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.
-----
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.
-----
20.
Section 3, Imp:
The basic scenario is when there are multiple SCTP EPs/ instances behind the NAT. I propose to extend Figure 1.
-----
21.
Section 4.1.1, Ed:
multi-home association => multi-homed association
In this single point traversal scenario => In the single point traversal scenario
-----
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.
-----
23.
Section 4.2, Imp:
I propose to clarify at the beginning of this section that SCTP packet header includes ports.
-----
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".
-----
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
-----
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!
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.
-----
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<https://tools.ietf.org/html/draft-ietf-tsvwg-natsupp-12#section-5.1.1> for the format).



I think INIT MUST be dropped in such case (not SHOULD).

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.
-----
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.
-----
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.
-----
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.
-----
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.


Also there might be additional actions in case of incoming ABORT and SHUTDOWN COMPLETE/ACK without the T-Bit set.
-----
33.
Section 4.3, Ed:
containing Local-Address => containing Private Address
-----
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.
-----
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.
-----
36.
Section 5.2, Q/Ma:
Is it really needed to include the whole packet/chunk into ABORT/ERROR?
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.
-----
37.
Section 5.2.1 and 5.2.3, Q:
When INIT ACK would cause ABORT or ERROR with any of these Error Causes?
-----
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?
-----
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<https://tools.ietf.org/html/draft-ietf-tsvwg-natsupp-12#section-6.7>.


It might be actually one NAT device which will then cover two logical NATs. Might be worth to mention it.
-----
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.
It could be also simplified if the NAT doesn't support INIT collision at all. Why is it so important?
-----
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.
-----
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.
-----
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.
-----
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.
-----
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.
-----
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.
-----
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.
-----
48.
Section 6.5, Imp:


   o  Generate a new ASCONF chunk containing the VTags parameter (see

      Section 5.3.2<https://tools.ietf.org/html/draft-ietf-tsvwg-natsupp-12#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<https://tools.ietf.org/html/rfc5061>].

It would be worth to clarify that the Disable Restart parameter should be added only if both EPs support this feature.
Also add the reference to Section 5.3.1.
-----
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
-----
50.
Section 6.5, Ed:
MUST-NOT => MUST NOT
i.  e. => i.e.
disabled restart => Disable Restart
-----
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".
-----
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)
-----
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
-----
54.
Section 7, Imp:
It would be good to extend a few examples by DATA packets (also retransmitted DATA in Section 7.2).
-----
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...
-----
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:"?
-----
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:
-----
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?
Also should it be ADD-IP=0.0.0.0 => ADD-IP= 10.1.0.1 ?
-----
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?
-----
60.
Section 7.4, Imp:
I think it's better to firstly show the empty table as the NAT lost its state.
-----
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?).
-----
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.
-----
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.
Shouldn't it be per local EP rather than per association?
-----


BR,
Maxim