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

[Michael Tuexen <[e-mail]>]

> On 20. Sep 2019, at 19:18, Gorry Fairhurst <[e-mail]> wrote:
> 
> 
> Thank you for addressing the issues raised in my last review.
> 
> This review is of rev 13, and relates to a detailed review of the languange in preparation for publication. There are a still a number of editorial issues with the current version to ensure this is sufficiently clear for a PS RFC. I think most of these are minor, but I would like you to consider a new rev to address the issues.
Hi Gorry,

thanks for the review. See my comment in-line.

Best regards
Michael
> 
> Please see below,
> best wishes,
> Gorry Fairhurst
> 
> ---
> 
> I think I understand what is intended by “so that only true NAT is available” - but for the avoidance of doubt can you please explain in the introduction what this means with a REF?
A ref is hard. But I changed the text 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.
> 
> Similarly, the English could be improves in:
> “The end result of this is that only one SCTP capable host can be behind a NAT and.. “
> - This time to perhaps say that "only one SCTP-capable host can successfully operate behind such a NAT”
Fixed.
> 
> Is the following always true: “ The only alternative for supporting legacy NAT devices is …”
> - This seems like a very bold statement - does this mean it is impossible to design other methods? that the IETF has not specified an alternate? what?
It is talking a legacy NAT, which means not changing the NAT implementation by adding
SCTP support. So UDP encapsulation is the using way of dealing with it.
> 
> “will maintain the proper state” is this better as “will maintain the appropriate state“ - or corresponding state?
Changed to “will maintain the appropriate state“.
> 
> 
> “without needing to change port numbers.” - is that the NAT or the host that does not need to change a port number?
changed to "without the NAT needing to change port numbers."
> 
> Section 2 needs to be updated to conform to the latest boilerplate text.
Fixed.
> 
> Please add reference to SCTP base spec for the definition of the Verification Tag
Added a reference to Section 3.1 of RFC 4960.
> 
> Section 4.2 please cite a reference to support calculation of the crc32c in SCTP.
Added the sentence:
See Appendix B of <xref target='RFC4960'/> for details of the CRC32c computation.
> 
> In section 4.2, please add “a” in the phrase: “is to use pre-defined table” so that this becomes: “is to use a pre-defined table”
Fixed.
> 
> I did not fully understand the test: “Other mechanisms
> could make use of NAT to NAT communication. Such mechanisms are not
> to be deployable on a wide scale base and thus not a recommended
> solution.”
> - I couldn’t work out whether this is a recommendations for some reason in this spec, or a statement of history, or what. The present text is confusing.
Changed to
Such mechanisms have not been deployabled on a wide scale base and thus are not
a recommended solution.
> 
> Please insert a hyphen in “The SCTP Specific Variant of NAT” before specific.
Fixed.
> 
> In 4.2:
> “ Using classical NAPT may result in changing one of the SCTP port
> numbers during the processing which requires the recomputation of the
> transport layer checksum. “
> - I think this is recalculation by the NAT - can you clarify which entity needs to recalculate?
Sure. Appended "by the NAPT device".
> 
> In text:
> “ One possible way of doing this is to use pre-defined table” I think “a” needs too be inserted before table.
Fixed.
> 
> In 4.2:
> “Other mechanisms
> could make use of NAT to NAT communication. Such mechanisms are not
> to be deployable on a wide scale base and thus not a recommended
> solution. Therefore the SCTP variant of NAT has been developed.”
> - I couldn’t work out whether this is a recommendations for some reason in this spec, or a statement of history, or what. The present text is confusing.
Changed to
Such mechanisms have not been deployabled on a wide scale base and thus are not
a recommended solution.
> In 4.3:
> “ In this section it is assumed that there are multiple SCTP capable
> hosts behind a NAT that has one Public-Address. “
> - Is that really the case? - Is try reader to assume there needs to be multiple SCTP hosts or that there CAN be multiple hosts. Please clarify.
Changed to
<t>In this section it is allowed that there are multiple SCTP capable hosts
behind a NAT that has one Public-Address.
> 
> in 4.3:
> “ In this case there must be no more than one entry”
> - please add in the “NAT binding table” to this phrase.
Fixed.
> 
> “ The entries in the table fulfill some uniqueness conditions. “
> should that be:
> “ The entries in the table need to fulfill some uniqueness conditions. “
Fixed.
> 
> Section 4.3 introduces the concept of a NAT control block
> - but I could not see where that was defined, nor how it relates to the table.
Replaced by NAT binding table entry.
> 
> IF I understand section 4.3 correctly it specified 3 different procedures that can be involved when an INIT chunk is received
> - do you think it is possible to highlight that in the text, e.g. labelling the different procedures as (1), (2) (3) so this is very clear?
Since these are not steps followed in a sequence I prefer not to enumerate them. But
I broke the text up in three paragraphs.
> 
> At the end of section 4, I was unclear what enabled the handling of INIT-collision through NAT - can you please add a sentence or two to make this clear?
It is the text in the paragraph before. So I removed the separation of the paragraphs.
> 
> OLD:
> a 46-bit random number which has to match
> NEW:
> a 46-bit random number that has to match
Fixed.
> 
> “In the TCP-like NAPT case “ - add citation to an RFC?
A NAPT device can control the 16-bit Natted Port and therefore avoid collisions
deterministically.</t>
> 
> In 6.2.1:
> “However, in this unlikely event the NAT device 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. “
> - why “this” and is it necessary to say this unlikely as a part of this sentence , to me it makes for a very long and complicated sentence to include an REFC2119 keywords, please consider rephrasing this and forming several sentences.
Replaced by:
<t>If the NAT device detects a collision of internal port numbers and
verification tags, it MUST send an ABORT chunk with the M-bit set if the
collision is triggered by an INIT or INIT-ACK chunk.
If such a collision is triggered by an ASCONF chunk, it MUST send an
ERROR chunk with the M-bit.
> 
> In 6.3.1:
> The considerations peak of “ its internal table”
> - which is this, please use the same term throughput to describe the entry in the table - and the name of the table.
Change to NAT binding table in the whole document.
> 
> “If the INIT is destined to an external address and port for which
> the NAT device has no outbound connection, it MUST allow the INIT”
> - what does “allow” mean, does it mean the INIT chunk is forwarded (to whom)? or does this mean something else?
The text is:
<t>If the INIT is destined to an external address and port for which
the NAT device has no outbound connection, it MUST allow the INIT creating an
NAT binding table entry.</t>

So the INIT is passed through the NAT and an entry is created.
> 
> “it MUST validate that the external server
> supports the Disable Restart feature and,”
> - is this verification using the local information in the table, or something else, please be clear?
If an entry is found, the NAT looks it up in the entry.
This is explained in Section 4.3 by stating

For the SCTP NAT processing the NAT device has to maintain a table of Internal-VTag, Internal-Port, External-VTag, External-Port, Private-Address, and whether the restart procedure is disabled or not.

> 
> The end of Section 6.3.1 describe stwo error conditions.
> - It was not clear to me which sequences of events generated these errors. Please associate the text with one of the procedures.
The first of these sentences finished the text describing the collision triggered by an INIT
chunk.

The last sentence describes the similar situation of ASCONF chunks triggering the collision.
Changed to

<t>The 'Port Number Collision' error cause (see <xref target='portcollide' />)
MUST be included in the ABORT chunk sent in response to the INIT chunk.</t>
<t>If the collision is triggered by an ASCONF chunk, a packet containing an
ERROR chunk with the 'Port Number Collision' error cause MUST be sent in
response to the ASCONF chunk.</t>

> “error cause MUST be sent back. “
> - this seems too vague to me, to which address is this sent under what conditions? (Section 6.4.1 seems clearer).
See above. Sending "in response to" means "send to the source address of the triggering packet".
> 
> Final clause of 6.3: “ Servers that support this feature will need to be capable of”
> - why isn’t that requirement expressed as a “REQUIRED to” statement at the start of the section? I am confused as to whether thus is really a prerequisite or not?
It is a prerequisite for them to announce this feature in the handshake.
> 
> “Please note that such a packet containing an ERROR chunk SHOULD NOT”
> - please remove the “Please note that” text and directly assert that this SHOULD NOT be done, to avoid any ambiguity that this could be a “note”.
Fixed.
> 
> “When sending the ERROR chunk, the new error cause ’Missing State’
> (see Section 5.2.2) MUST be included and the new M-bit of the ERROR
> chunk MUST be set (see Section 5.1.2).“
> -please remove the two occurrences of “new” from this sentence.
Fixed.
> 
> Section 6.4.2 places there requirements on the receiver, but I would expect more explicit statements about what happens if the SHOULD is not observed.
> 
> Bullet two was particularly unclear with the clause; “if it does not discard the incoming
> ERROR chunk”
> - Please specify what happens.
Changed to:
<t>It SHOULD validate that the verification tag is reflected by looking
at the VTag that would have been included in the outgoing
packet.
If the validation fails, discard the incoming ERROR chunk.</t>
<t>It SHOULD validate that the peer of the SCTP association supports
the dynamic address extension.
If the validation fails, discard the incoming ERROR chunk.</t>
> 
> “ 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).”
> - there appear to be multiple requirements here, please clarify if the MUST NOT is always needed (maybe make that separate clause) and the /should/ us actually a /SHOULD??
There is only a single requirement:

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 and take no action (since the address is
already in the association).</t>

(Replaced should by SHOULD)
> 
> “ 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).“
> - to which address?
This doesn't need to be specified here, since it is described RFC 5061.
> 
> “ It MUST validate that the verification tag is reflected by looking
> at the VTag that would have been included in the outgoing packet.”
> …and if not verified, discard the message?
Yes.
Added
If the validation fails, it MUST discard the packet.
> OLD:
> NAT device , then
> NEW:
> NAT device, then
Fixed.
> 
> “NAT does need to change the table for second address:”
> - Insert “The”
Fixed.
> 
> OLD:
> NAT 1 does not need to update the table for second address:
> NEW:
> NAT 1 does not need to update the table for the second address:
Fixed.
> 
> OLD:
> NAT 2 creates complete entry:
> NEW:
> NAT 2 creates a complete table entry:
Fixed.
> 
> OLD:
> The NAT device cannot find entry for the association.
> NEW:
> The NAT device cannot find an entry in the table for the association.
Fixed.
> OLD:
> It sends ERROR
> message with the M-Bit set and the cause "NAT state missing".
> NEW:
> It sends an ERROR message with the M-Bit set and the cause "NAT state missing".
Fixed.
> 
> I’m unclear about the following, please rephrase:
> “ Host B adds the new source address and deletes all former entries..”
Done:
<t>Host B adds the new source address to this association and deletes
all other addresses from this association.</t>
> 
> Unclear about what was intended by:
> “If two hosts are behind NAT devices, they have to get knowledge of
> the peer’s public address. “
> - are these two SCTP hosts that try to communicate wit one another?
Yes. Changed to
<t>If two hosts are behind NAT devices and want to communicate with each other,
they have to get knowledge of the peer's public address.
> 
> “The
> naming of the verification tag in the packet flow is done from the
> sending peer’s point of view.”
> - what does “naming mean in this context?
Clarified by using
The naming (internal/external) of the verification tag in the packet
flow is done from the sending host's point of view.</t>
> 
> “Host A send INIT-ACK, which can pass through NAT B:’
> OLD
> send
> NEW
> sends
Fixed.
> 
> 
>