Re: [Hipsec] Comments on draft-hip-native-nat-traversal-19

[Miika Komu <[e-mail]>]

Hi Christer,

On 03/26/2017 02:34 AM, Christer Holmberg wrote:
> Hi,
>
> As co-author for the ICEbis draft, I was asked to review
> draft-hip-native-nat-traversal-19.
>
> I have not had time to review the whole document. However, many of my
> comments are generic, and apply to the whole document.

thanks for the feedback! Your (editorial) comments should be addressed 
in this version:

https://tools.ietf.org/html/draft-ietf-hip-native-nat-traversal-20

The diff is here:

https://www.ietf.org/rfcdiff?url2=draft-ietf-hip-native-nat-traversal-20

> I have no knowledge of HIP, so I will not comment on HIP issues like
>  messages, parameters etc used.

If one implements this, some digging into the HIP documents is needed. 
 From a design perspective, the document should be readable as it is (I 
hope).

> General: =======
>
> QG0: Throughout the document, you use RFC 2119 terminology (SHOULD,
> MUST etc with capital letters) when you refer to procedures and rules
> defined elsewhere. I think that is wrong, and it also makes it very
> difficult what exactly is defined in this document, and what is
> defined in some other specification.

I am keeping the SHOULD/MUST/etc for HIP documents (since this is an 
extension of HIP) but ICE references are now with small letters (there 
actually weren't too many).

> QG1: You say that the mechanism in the draft is based on ICE. I think
> it would be good to give a name to the mechanism. "HIP-ICE", or
> something similar.

Done:

    ICE:
       Interactive Connectivity Establishment (ICE) protocol as specified
       in [I-D.ietf-ice-rfc5245bis]

    Legacy ICE-HIP:
       Refers to the "Basic Host Identity Protocol (HIP) Extensions for
       Traversal of Network Address Translators" as specified in
       [RFC5770].  The protocol specified in this document offers an
       alternative to Legacy ICE-HIP.

    Native ICE-HIP:
       The protocol specified in this document (Native NAT Traversal Mode
       for HIP).

> QG2: I would also like to have a dedicated section which on a
> high-level describes the differences/restrictions between legacy ICE
> and HIP-ICE. It helps very much when later reading the details in
> section 4. That section should at least list the different types of
> functions (HIP relays etc) are used for gathering candidates, what
> protocol (HIP messages) is used instead of STUN, what types of
> candidates are used and how they are retrieved.

The ICE comparison has been already in the previous version in Appendix 
B ("Differences with respect to ICE") and now the intro also references 
this section ("Appendix B explains the differences to ICE."). I think 
it's not good to start the document with a diff to ICE. We have to 
assume that the reader is familiar with HIP since this is an extension 
to it.

> It would also be good to give a short overview of the HIP messages
> used for the connectivity checks. It is very useful when later
> reading the details.

The intro now includes a really short intro to HIP, but I don't think we 
should introduce the messaging formats here again. This is an extension 
to HIP, so some familiarity with HIP is required.

(Btw, the ICE specification does not describe STUN/TURN messaging 
formats either, so the situation is the same for that document)

> QG3: You should use consistent terminology when you talk about
> endpoints and relays. Sometimes the text says "host", sometimes "HIP
>  relay server client", sometimes "relay client", sometimes
> "end-host". Sometimes you say "HIP relay", sometimes "HIP server
> relay", etc. Sometimes you say "non-relay host", which suggests that
> the relay is also a host.

the document has over 300 occurrences of host, could you be a bit more 
specific where this is a problem? "End-host" means "non-relay host", and 
yes, a relay is a host too.

I agree the client/server relay terminology was a bit sloppily used. In 
general, I think the terms were a bit asymmetrical:

* HIP vs. data relay, why not just "control" and "data"
* HIP relay vs HIP relay client (why not HIP relay *server*)

So I came up with a better relay terminology that is now applied 
consistently throughout the document:

    Control Relay Client:
       A requester host that registers to a Control Relay Server
       requesting it to forward control-plane traffic (i.e.  HIP control
       messages).  In the Legacy ICE-HIP specification, this is denoted
       as "HIP Relay Client".

    Data Relay Server:
       A registrar host that forwards HIP related data plane packets,
       such as Encapsulating Security Payload (ESP) [RFC7402], between

       two hosts.  This host implements similar functionality as TURN
       servers.

    Data Relay Client:
       A requester host that registers to a Data Relay Server requesting
       it to forward data-plane traffic (e.g.  ESP traffic).

> Section 3: ========
>
> Q30: The text says:
>
> "The hosts may use HIP relay servers (or even STUN or TURN servers)
> for gathering the candidates."
>
> This is confusing, as you have earlier said that HIP-ICE doesn't use
> STUN.
>
> (Implementations may of course provide both STUN- and HIP
> functionality, but that is outside the scope of the document.)

I hope this is now better:

    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 or
    Data Relay Server are available.

How the candidates are gathered does not really affect interoperability.

> Q31: The text says:
>
> "To be contacted from behind a NAT, the Responder must be registered
>  with a HIP relay server reachable on the public Internet, and we
> assume, as a starting point, that the Initiator knows both the
> Responder's Host Identity Tag (HIT) and the address of one of its
> relay servers”
>
> First, when you say "its relay servers", I assume you mean the relay
>  servers of the Responder?

yes

> Second, doesn't the Initiator need to know the address of the relay
> to which the Responder is actually registered, in case there are
> multiple relays but the Responder is not registered with all of
> them? Maybe someone thinks it's obvious, but I think it should be
> make clear.
>
> Could you simply say:
>
> "To be contacted from behind a NAT, the Responder must be registered
>
> with a HIP relay server reachable on the public Internet. It is
> assumed that
>
> the Initiator knows the address of the relay server(s) to which the
> Responder
>
> is registered."

The text is now a bit rearranged, but this is now changed as follows:

    To be contacted from behind a NAT,
    at least the Responder must be registered with a Control Relay Server
    reachable on the public Internet. [..]

    We assume, as a starting point, that the Initiator knows both the
    Responder's Host Identity Tag (HIT) and the address(es) of the
    Responder's Control Relay Server(s) (how the Initiator learns of the
    Responder's Control Relay Server is outside of the scope of this
    document, but may be through DNS or another name service).

> Q32: The section introduces the "base exchange" concept, but it is
> not clearly defined. Is it something defined in HIP, is it HIP-ICE
> specific? I think you should add a description/reference somewhere.

Defined in HIP and briefly now introduced and referenced in the 
introduction.

> Q33: The text says:
>
> "At the end of the procedure, if successful, the hosts will have
> established a
>
> UDP-based tunnel that traverses both NATs, with the data flowing
> directly from NAT to NAT or via a HIP data relay server."
>
> What if the responder has only registered to a HIP server relay? The
>  text in the section seems to suggest that it is optional to register
>  to a data relay.
>
> The text needs to be more clear on whether the endpoints need to
> register to a server relay and/or data relay.

I think this should be clear now:

The Responder may have also
    registered to a Data Relay Server that can forward the data plane in
    case NAT penetration fails. [..]

    To be contacted from behind a NAT,
    *at least* the Responder must be registered with a Control Relay
    Server reachable on the public Internet. [..]

Relayed candidates SHOULD be gathered in
    order to guarantee successful NAT traversal, and implementations
    SHOULD support this functionality even if it will not be used in
    deployments in order to enable it by software configuration update if
    needed at some point.

> Section 4.2: ==========
>
> Q421: In general, I think it would be useful to split the section
> into HIP client procedures and HIP relay server procedures.

Actually now the section just includes client procedures, so I changed 
the name.

> Q422: I think the following text should be in the beginning of the
> section:
>
> "ICE guidelines for candidate gathering MUST be followed as described
> in section 4.1.1 in [I-D.ietf-ice-rfc5245bis].  A number of host
> candidates (loopback, anycast and others) should excluded as
> described in section 4.1.1.1 of the ICE specification
> [I-D.ietf-ice-rfc5245bis]."

I think moving this text does not improve readability of the section, so 
I didn't do this. Currently, there is some intro text before this.

> Q423: The text says:
>
> "However, if no data relay is used, and the host has only a single
> local IP address to use, the host MAY use the local address as the
> only host candidate"
>
> I am not sure what is meant by "as the only host candidate".

So if the host has no clue on the server reflexive/relayed addresses, it 
should use the address as seen from the network interface as the (only) 
host candidate. I change this if needed if some other wording works better?

> Q424: The text says:
>
> "Relayed candidates SHOULD be gathered in order to guarantee
> successful NAT traversal.  It is RECOMMENDED for implementations to
> support this functionality"
>
> If you say SHOULD use, why do you then only say RECOMMEND support?
> Doesn't the support need to be stronger than the usage?

Fixed, both are now SHOULD.

> Q425: The text says:
>
> "Unlike ICE, this protocol only creates a single UDP flow between
> the two communicating hosts, so only a single component exists."
>
> This is confusing. What is meant by "unlike ICE"? You are using ICE
> :) I assume you are referring to ICE usage for non-multiplexed
> RTP/RTCP?

ICE is now defined in the terminology (I-D.ietf-ice-rfc5245bis)

> I think you simply need to say that HIP only uses a single UDP flow,
>  using a single component, with a value of 1.

I can remove the ICE comparison if this is still confusing...

    Unlike ICE, this protocol only creates a single UDP flow between the
    two communicating hosts, so only a single component exists.  Hence,
    the component ID value MUST always be set to 1.

> Section 4.3: ==========
>
> Q431: The text says:
>
> "This section describes the usage of a new non-critical parameter
> type."
>
> Shouldn't it say that the section defines a new parameter type?

No, just the usage. The actual format/definitions are in section 5.

> Q432: If I understand correctly, the NAT mode negotiation between an
>  endpoint and a relay takes place during registration.

yes. Registration = base exchange with some registration parameters.

> But, when does the NAT mode negotiation between two endpoints (shown
>  in the call flow) take place? During connectivity checks? I think
> you should clarify that.

During base exchange again, in steps 3-6 as defined in section:

4.5.  Base Exchange via Control Relay Server

I am not sure what is unclear.

> Section 4.6: ==========
>
> Q460:The text says:
>
> "but UDP encapsulated HIP control messages are used instead of ICE
> messages."
>
> What do you mean by "ICE messages"? STUN?

ICE refers to I-D.ietf-ice-rfc5245bis, so yes. I can change this if needed.

> Section 4.6.2: ============
>
> Q4620: The text says:
>
> "The HITs of the two communicating hosts MUST be used as credentials
>  in this protocol (in contrast to ICE which employs username-password
>  fragments)."
>
> Again, "contrast to ICE" sounds confusing, because you are using ICE.
> Maybe you should say "legacy ICE", or something similar, when
> referring to 5245bis procedures.

ICE refers to I-D.ietf-ice-rfc5245bis in the terminology, so let me know 
if this is still unclear...

> Section 4.7: ==========
>
> Q470: The name of the section ("NAT traversal alternatives") is
> confusing, because one may think it talks about something else than
> HIP-ICE.
>
> I would suggest to call it something like "Optimizations".

Done.

> Section 4.9: ==========
>
> Q490: I think Mobiliy should be described in a separate main
> section.

The document is structured the same way as the other HIP related RFCs, 
so all protocol interaction is described in "Protocol Description".