Re: [Hipsec] Genart last call review of draft-ietf-hip-native-nat-traversal-27

[Miika Komu <[e-mail]>]

Hi Roni,

thanks for the detailed review! My comments are below.

On 02/26/2018 03:21 PM, Roni Even wrote:
> Reviewer: Roni Even
> Review result: Almost Ready
> 
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
> 
> For more information, please see the FAQ at
> 
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
> 
> Document: draft-ietf-hip-native-nat-traversal-??
> Reviewer: Roni Even
> Review Date: 2018-02-26
> IETF LC End Date: 2018-02-26
> IESG Telechat date: Not scheduled for a telechat
> 
> Summary:
> The document is almost ready for publication as a standard track RFC
> 
> Major issues:
> 
> Minor issues:
> 
> 1. in section 4.2 "Gathering of candidates MAY also be performed by other means
> than described in this section.  For example, the candidates could be
>     gathered as specified in Section 4.2 of [RFC5770] if STUN servers are
>     available, or if the host has just a single interface and no STUN orData
>     Relay Server are available." I did not see this a different ways since
>     section 3 says "The hosts use either Control Relay Servers or Data Relay
>     Servers (or other infrastructure including STUN or TURN servers) for
>     gathering the candidates." so STUN is mentioned also here.

I suggest to remove the remark in parenthesis (or other infrastructure 
including STUN or TURN servers). Does this solve the issue?

> 2. In section 4.6.2 "The connectivity check messages MUST be paced by the Ta
> value negotiated during the base exchange as described in Section 4.4.  If
> neither one of the hosts announced a minimum pacing value, a value of  20 ms
> SHOULD be used." in section 4.4 the default value is 50 ms?

Good catch! I double checked this from the ICE spec, which defaults also 
to 50 ms. So, I change the value to 50 ms also in section 4.6.2.

> 3. in section 5.4 what about "ICE-STUN-UDP         2" ;  I assume it is not
> relevant but this is also the IANA registeration

I think it makes sense to add the missing one as you suggest, but omit 
it from the IANA registration since it is already registered for RFC5770.

> 4. In section 5.5 "The TRANSACTION_PACING is a new parameter" it is not new it
> is in RFC5770

You're right, I'll change this.

> 5. In section 5.10 "SERVER_REFLEXIVE_CANDIDATE_ALLOCATION_FAILED  63" is the
> only new one. this also relates to section 7 that says that all error values in
> section 5.10 are new while the rest are in RFC5770. Also there is no mention in
> section 7 of which registry is used for the error values.

Good catch, I'll correct these and add the IANA registry.

> Nits/editorial comments:
> 1. Expand SPI and LSI when first appear in the document
> 
> 2. in section 2 "the base of an candidate" should be "a candidate"
> 
> 3. In section 3 "so it is the Initiator may also have registered to a Control
> and/or Data Relay Server" maybe "so  the Initiator may also need to register to
> a Control and/or Data Relay Server"
> 
> 4. In section 4.2 "However, it is RECOMMENDED that a Data Relay Client
> registers a new server reflexive candidate for each its peer for the reasons
> described" maybe "for each of its..."

Thanks for spotting these, will fix as suggested.

> 5. In section 4.2 I could not parse the sentence "where Ta is the value used
> for Ta is the value used for the"

Should be "where Ta is the value used for the"...

> 6. in section 4.6 "as defined in section in 6.7 in [RFC7401]:"  change to "as
> defined in section 6.7 in [RFC7401]:"

Will fix this too.

Should I post a new version with the suggested changes?