Subject: [Gen-art] IETF Last Call Review of draft-ietf-hip-rfc5201-bis-14

From: Tom Taylor <[email-masked]>
Date: Thu, 12 Jun 2014 20:58:34 -0400

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-hip-rfc5201-bis-14
Reviewer: Tom Taylor
Review Date: 2014-06-12
IETF LC End Date: 2014-06-11
IESG Telechat date: 2014-06-26

Summary: The draft is mostly ready, some minor issues and a number of 
editorial improvements identified to this point. Apologies for being 
late, and this is only a first installment, covering up to Section 5.3 
but not its sub-sections yet. I hope to finish the job tomorrow.

Major issues:

Minor issues:

Section 4.3 (Error Processing) final case: if the receiving host does 
not send some sort of response (e.g., ICMP) to the sender, the sender 
may have no way of knowing that the HIP session has failed. The state 
transitions from ESTABLISHED in Table 6 time out on no packet 
"sent/received" for a given period of time. If the sending application 
doesn't expect a response, it could be sending packets that are ignored 
at the other end for an indefinitely long time. At the least, this point 
should be brought out in the text of that error case, and possibly the 
ICMP response should be RECOMMENDED.

Section 5.2.7: when the host supports more than one D-H Group, is each 
group specified in a separate instance of the Diffie-Hellman parameter? 
The text does not say.

Section 5.2.18: given the strict ordering of HIP parameters, the initial 
plaintext for the Encrypted content (type and length of initial 
parameter) may be fairly easily guessed. This opens up the minor 
possibility of a known plaintext attack. [Comment to be reviewed after 
further examination.]

Section 5.3, last paragraph: forbids fragmentation of the HIP packet. 
Doesn't this contradict Section 5.1.3?

Nits/editorial comments:

Idnits generated 13 warnings. Most are spurious, but there are a few 
outdated references. I didn't check out the IPv6 addresses.
                                       ^
Inconsistent capitalization: "Base Exchange" (in the opening sections) 
vs. "base exchange" (most of the document)

Sometimes the phrase is "HIP session", sometimes "HIP association". The 
RFC Editor will probably want you to choose which one to use 
consistently. Noted that "HIP association" is defined in Section 2.3.

Section 1, second paragraph is a little brief on the additional 
specification beyond the base exchange provided in this document. The 
sentence beginning "It also defines ..." should include " and 
terminating the association".

Section 1, last paragraph, line 256 (according to the Idnits output): 
missing period.
"... later discovered to be vulnerable This update"
                                       ^

It would be nice to have a definition of the term "HIP packet" in 
Section 2.3. If I understand correctly, a HIP packet is one of the 
packet types defined in Section 5.3. After the base exchange, most of 
the packets flowing in a HIP association are ordinary user data packets 
and not HIP packets. In some places the term "HIP control packets" is 
used. Choose one or the other term for consistency.

Section 3.1, third line from end: s/a hosts/a host/

Section 4.1.4: perhaps the last and second-last paragraphs should be 
interchanged. I got lost for a moment on the association of multiple R1s 
with the Initiator rather than the Responder when reading the 
second-last paragraph. The last paragraph makes the situation clear.

Section 4.1.5, first paragraph, second sentence: assumes that the host 
is willing to carry out a base exchange with the original Initiator, 
albeit on its own terms. Should something like "and policy allows the 
establishment of a HIP association with the original Initiator" be added 
in front of the comma in that second sentence? That leads nicely to 
discussion of that case in the next paragraph.

State diagram, Figure 2, in Section 4.4.4: unlabelled line from CLOSING 
to I1-SENT.

Last line of Section 4.5.3: s/transform/transport format/

Section 5.1.1 IPv4 case refers back to Section 4 for the HIP protocol 
number. The proper reference should be to Section 5.1.

Section 5.1.3 first paragraph last sentence: probably should rephrase so 
the host doesn't get presented as a person, e.g., "If it is expected 
that a host will send HIP packets that are larger than the minimum IPv6 
MTU, the implementation MUST include IPv6 fragmentation."

Section 5.2.5, missing space at the right hand end of the line 
containing Opaque in the figure.

Section 5.2.6, immediately beneath the figure:
  - DH GROUP ID explanation:
   -- s/defines/identifies/ on first line. This comment applies to a
      number of other parameter definitions, please review.
   -- The third "sentence" is missing a verb.

  Second paragraph below list of defined group IDs:
    s/symmstric/symmetric/

Section 5.2.10, first paragraph, last sentence:
Should "which source HITs" be "which source HIT suites"? If the text is 
correct as it stands, perhaps text is needed to say what a source HIT is 
and which peer it identifies.

Section 5.2.11:

Change "transform" and "transport form" to "transport format" for 
consistency.

Middle paragraph: "... parameters and their use are defined ..."
                                                 ^^^

Last paragraph:
  - In the first line, if I understand correctly, add 
"TRANSPORT_FORMAT_LIST" before "parameter".
  - s/repective/respective/.
  - s/TRANSPORT_FORM_LIST/TRANSPORT_FORMAT_LIST/
  - The last sentence is receiver behaviour. Suggest it be rewritten as"
"The receiver MUST ignore ..."

Section 5.2.13, first paragraph, first sentence might read better as: 
"The HIP_MAC_2 is a MAC of the packet and the HI of the sender in the 
form of a HOST_ID parameter when that parameter is not actually included 
in the packet."

Section 5.2.19:

Paragraph above the list of Notify message types: perhaps clearer if 
"with status Notify Message Types" is rewritten as: "where the Notify 
Message Type is in the status range".

INVALID_SYNTAX: have "denial- of-service". Typically in XML2RFC the 
unwanted blank comes when your XML editor has split the phrase at a 
hyphen, so "denial-" comes at the end of a line and the remainder on the 
next line. Solution is to move "denial-" to the next line too.

Section 5.2.20, end of second-last sentence: s/parameter/parameters/