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. This is the final and complete version of the review. Lines have been drawn below to indicate where the first version left off. 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.] [Upon review: I1 packets have fixed type but variable length due to varying lengths of DH-GROUP-LISTS. The set of such lengths is limited, however, so it is worth considering whether known plain-text attacks offer any real threat.] Section 5.3, last paragraph: forbids fragmentation of the HIP packet. Doesn't this contradict Section 5.1.3? -------- Issues below here added in second version 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: 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, after the base exchange, most of the packets flowing in a HIP association are ordinary user data packets and not HIP packets, and this point should be made. 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/ -------- Material below here added in second version Sections 5.3.x: UPDATE implementation is specified to be "MANDATORY", NOTIFY implementation is specified to be "optional" (sic, lower-case), and nothing is specified for the other packet types. You probably need to specify MANDATORY implementation for each packet type except NOTIFY, and that one should be OPTIONAL. Section 5.3.5: Syntax: should "updated parameters" be added after "ACK,"? Second-last paragraph: s/forgo/forego/ Section 6.1: in processing step 3, do I understand correctly that the packet to be queued is an user data packet, not a HIP packet? The text should make this clear. Section 6.6 step 4: would "trial counter" be a more accurate term than "timeout counter", given that the counter is incremented before any timeout occurs? Section 9 (IANA) Hit Suite, start of last paragraph: "For the time being" is more usual than "At the time being". Appendix C.1 (IPv6 example): the correct documentation prefix is 2001:DB8::/32, not 2001:D88:0. C.1 through C.3: The new prefix for ORCHID as defined in rfc4843-bis is 2001:0000::/23 and no longer 2001:10::/28. Appendix E, opening sentence: ORCHID prefix is now 23 bits, not 28 bits.