Subject: Re: [Hipsec] Eric Rescorla's Discuss on draft-ietf-hip-native-nat-traversal-28: (with DISCUSS and COMMENT)

From: Miika Komu <[email-masked]>
Date: Wed, 7 Nov 2018 21:37:14 +0000

Hi Eric,

apologies for the belated response, I am not working on HIP anymore, so 
it has been rather difficult to find time for this.

On 5/4/18 22:34, Eric Rescorla wrote:
> Eric Rescorla has entered the following ballot position for
> draft-ietf-hip-native-nat-traversal-28: Discuss
> 
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
> 
> 
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-hip-native-nat-traversal/
> 
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> Rich version of this review at:
> https://mozphab-ietf.devsvcdev.mozaws.net/D3099
> 
> 
> I am very familiar with ICE and yet I found this document extremely
> hard to follow. The problem is that it cherry-picks pieces of ICE and
> I'm just not sure that it's a complete specification when put all
> together. I have noted a number of places where I actually am not sure
> how to implement something, and fixing those will resolve this
> DISCUSS, but IMO you really should totally rewrite this document
> either (a) as a variant of ICE or (b) as an entirely new document not
> with a pile of new text and then references out to ICE sections.

the expected receivers of the work are the implementers of RFC5770, so 
the draft follows the sectioning of the RFC5770 (which has two 
interoperable implementations).

If I understood your comment right, the variant of ICE (a) would follow 
the ICE document structure but then the document would not serve anymore 
HIP implementers so well. What comes to option (b), I think it would 
make the the document quite long if we replicated everything in the ICE 
specification (and possibly from the HIP specifications) in the draft.

> DETAIL
> S 4.2.
>>       request type SHOULD NOT create any state at the Control Relay Server.
>>    
>>       ICE guidelines [I-D.ietf-ice-rfc5245bis] for candidate gathering are
>>       followed here.  A number of host candidates (loopback, anycast and
>>       others) should be excluded as described in the ICE specification
>>       [I-D.ietf-ice-rfc5245bis].  Relayed candidates SHOULD be gathered in
> 
> If you're going to normatively cherry-pick ICE, you need to note
> specific sections, I think.

good catch, now that the ICE specification is finally an RFC, we can add 
a reference here and to a number of other places (which I had commented 
out because the section numbering in ICE was changing).

> S 4.6.2.
>>    
>>       A host may receive a connectivity check before it has received the
>>       candidates from its peer.  In such a case, the host MUST immediately
>>       generate a response, and then continue waiting for the candidates.  A
>>       host MUST NOT select a candidate pair until it has verified the pair
>>       using a connectivity check as defined in Section 4.6.1.
> 
> Are you supposed to put this on a TODO check list as with ICE?

I believe you refer to the triggered-check queue:

https://tools.ietf.org/html/rfc8445#section-6.1.4.1

I changed the text as follows:

A host may receive a connectivity check before it has

received the candidates from its peer. In such a case, the

host MUST immediately generate a response by placing it in the 
triggered-check queue, and then continue
waiting for the candidates.

> S 5.8.
>>    
>>    5.8.  RELAY_HMAC Parameter
>>    
>>       As specified in Legacy ICE-HIP [RFC5770], the RELAY_HMAC parameter
>>       value has the TLV type 65520.  It has the same semantics as RVS_HMAC
>>       [RFC8004].
> 
> What key is used for the HMAC?

clarified this as follows:

[..] It has the same semantics as RVS_HMAC as specified in section 4.2.1 
in [RFC8004].  Similarly as with RVS_HMAC, also RELAY_HMAC is is keyed 
with the HIP integrity key (HIP-lg or HIP-gl as specified in section 6.5 
in [RFC7401]), established during the relay registration procedure as 
described in Section 4.1.

> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> S 2.
>>       Server reflexive candidate:
>>          A translated transport address of a host as observed by a Control
>>          or Data Relay Server.
>>    
>>       Peer reflexive candidate:
>>          A translated transport address of a host as observed by its peer.
> 
> You should indicate which definitions are the same as ICE/STUN

these both are the same as in ICE. I have now added references to all of 
the terms to indicate their origin.

> S 3.
>>       connected to the public Internet.  To be contacted from behind a NAT,
>>       at least the Responder must be registered with a Control Relay Server
>>       reachable on the public Internet.  The Responder may have also
>>       registered to a Data Relay Server that can forward the data plane in
>>       case NAT traversal fails.  While, strictly speaking, the Initiator
>>       does not need any Relay Servers, it may act in the other role with
> 
> Why doesn't it need Relay Servers? This isn't true for ordinary ICE.

good catch. Actually, Control Relay Server is always needed to discover 
server reflexive candidates but Data Relay Server is kind of optional. I 
changed the text as follows:

While, strictly speaking, the Initiator does not need a Data Relay 
Server, [..]

> S 4.1.
>>       for that host and close the corresponding UDP port (unless other Data
>>       Relay Clients are still using it).
>>    
>>       The Data Relay Server SHOULD offer a different relayed address and
>>       port for each Data Relay Client because this can cause problems with
>>       stateful firewalls (see Section 6.5).
> 
> by "this" I think you mean "not doing so"

thanks, corrected.

> S 4.2.
>>       parameter discovered during the Control Relay Server registration as
>>       a server reflexive candidate.  In this case, no further candidate
>>       gathering is needed.
>>    
>>       A Data Relay Client MAY register only a single relayed candidate to
>>       be used with multiple other peers.  However, it is RECOMMENDED that a
> 
> Nit: this would be clearer if you said "that it uses with"

yes, thanks!

> S 4.2.
>>       gathering is needed.
>>    
>>       A Data Relay Client MAY register only a single relayed candidate to
>>       be used with multiple other peers.  However, it is RECOMMENDED that a
>>       Data Relay Client registers a new server reflexive candidate for each
>>       of its peer for the reasons described in Section 4.12.3.  The
> 
> This sentence feels like a bit of a non-sequiter. How do you
> "register" a srflx? Or should this say "relayed"

You're right, it should say "relayed".

> S 4.2.
>>       deployments in order to enable it by software configuration update if
>>       needed at some point.  A host SHOULD employ only a single server for
>>       gathering the candidates for a single HIP association; either one
>>       server providing both Control and Data Relay Server functionality, or
>>       one Control Relay Server and also Data Relay Server if the
>>       functionality is offered by another server.  When the relay service
> 
> How does this interact with mult-layered NAT?>

No different from ICE with separated STUN and TURN servers multi-layer 
NAT scenarios. Should we mention something about the issues related to 
some specific scenario?

> S 4.3.
>>       Servers (e.g. in the case of a single server, it would be 1).  A
>>       smaller value than 500 ms for the RTO MUST NOT be used.
>>    
>>    4.3.  NAT Traversal Mode Negotiation
>>    
>>       This section describes the usage of a new non-critical parameter
> 
> Can you name the type here please?

I changed this as follows:

This section describes the usage of a non-critical parameter type called 
NAT_TRAVERSAL_MODE with a new mode called ICE-HIP-UDP.

> S 5.6.
>>         Protocol   IANA assigned, Internet Protocol number.
>>                    17 for UDP, 0 for plain IP
>>         Reserved   reserved for future use; zero when sent, ignored
>>                    when received
>>         Address    an IPv6 address or an IPv4 address in "IPv4-Mapped
>>                    IPv6 address" format
> 
> Is there a concern about NATs rewriting these, as we had for XOR-
> MAPPED-ADDRESS

we don't necessarily need XORring in HIP, we can actually encrypt 
parameters. I have requested the WG if it is ok to switch to encrypted 
parameters - should be ok I believe.

The original reason to keep the locators (and many other HIP parameters) 
in plaintext was middlebox transparency but I guess in here we have a 
good reason to break it.

> S 5.7.
>>       | Reserved  | 0        | Reserved for future extensions             |
>>       | Preferred | 0 or 1   | Set to 1 for a Locator in R1 if the        |
>>       | (P) bit   |          | Responder can use it for the rest of the   |
>>       |           |          | base exchange, otherwise set to zero       |
>>       | Locator   | Variable | Locator lifetime in seconds                |
>>       | Lifetime  |          |                                            |
> 
> What is the purpose of this? It's not an ICE parameter.

In HIP, locators have a maximum lifetime after which they become 
deprecated (RFC8046). Should add something here?

> S 5.7.
>>       | Port      |          |                                            |
>>       | Transport | Variable | IANA assigned, transport layer Internet    |
>>       | Protocol  |          | Protocol number.  Currently only UDP (17)  |
>>       |           |          | is supported.                              |
>>       | Kind      | Variable | 0 for host, 1 for server reflexive, 2 for  |
>>       |           |          | peer reflexive or 3 for relayed address    |
> 
> Why do you need to send prflx candidates?

This is residual from RFC5770 from where it wasn't used either. I added 
a note here that kind 2 is currently unused.

> S 10.2.
>>       o  If the agent is using Diffserv Codepoint markings [RFC2475] in its
>>          media packets, it SHOULD apply those same markings to its
>>          connectivity checks.
>>    
>>       o  Unlike in ICE, the addresses are not XOR-ed in Native ICE-HIP
>>          protocol in order to avoid middlebox tampering.
> 
> I noted this earlier. Why?

See my earlier comment, I would change this to:

Unlike in ICE, the addresses are not XOR-ed in Native ICE-HIP 

protocol but rather encrypted to avoid middlebox tampering.