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.
_______________________________________________
Gen-art mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/gen-art
