Subject: [Gen-art] Gen-art last call and telechat review of draft-ietf-hip-hiccups-02

From: Elwyn Davies <[email-masked]>
Date: Tue, 15 Jun 2010 15:07:19 +0100

I have been selected as the General Area Review Team (Gen-ART)
reviewer for this draft (for background on Gen-ART, please see
http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).

Please wait for direction from your document shepherd
or AD before posting a new version of the draft.

Document: draft-ietf-hip-hiccups-02
Reviewer: Elwyn Davies
Review Date: 15 June 2010
IESG Telechat date: 17 June 2010

Summary:
Almost ready for Experimental standard.  A number of minor issues (including those noted by Lars Eggert) plus a fair number of editorial nits (mostly non-use of in/definite articles).

Major issues:
None.

Minor issues:
s2: The commentary on the MIC is somewhat difficult to parse. I think the phrase 'and thus need be protected otherwise against tampering' can be misread - I presume it means that some additional means are needed to secure the MIC against tampering. e.g., 'and thus it needs additional means to ensure that it has not been tampered with during transmission.'

s3.1.2.  Length:  Specified here as the 'length of the parameter' which could be confusing.  RFC 5201 specified that 'Length' is the length of the Contents - the length of the parameter is derived from it by the algorithm in s5.2.1. of RFc 5201. Suggest 'length of contents' instead.

s3.2: the term 'half-stateless' is not used in RFC 5201 and I can't find any other definition of what it means (I can maybe guess!)  I think some additional explanation is needed to backup the term.

s4: In all the packet format specifications the HOST_ID is marked as optional, but as I understand it this is the public key for the calculation of the HIP_SIGNATURE.  Later (s5.2, item 3) we discover that the public key can be supplied out of band as an alternative to being carried in the packet.  It would be helpful to explain in the description why it is optional, and make it explicit that it is interpreted as a public key.

s4: The packet definition does not cater for the potential multiple PAYLOAD_MIC items allowed by the forllowing words.

ss4.1-4.4: Is there some logic behind the selection of the proposed Type numbers? Note that the TRANSACTION_ID parameter is taken out the range reserved for base protocol extensions whereas the others are taken from the first come, first served range, but not in sequence.  Is there a reason?

ss4.1, 4.2, 4.3: Should note explicitly that these parameters are flagged as CRITICAL. 
ss4.1, 4.2: how is the sequence number encoded? Presumably unsigned integer in network byte order.
s4.2: Why is the length variable?  The content is specified as one 32 bit sequence number - surely the length is fixed at 4.
ss4.1, 4.2: worth noting that no padding is needed (assuming there really is only one sequence number in s4.2) - see the comment on s5.1.

s4.3: sorry to be pedantic, but what happens if the payload data is less than 8 bytes/octets in length?

s4 and s4.4: S4 says the TRANSACTION_ID is a number.  If so then s4.4 needs to specify how it is encoded.  However the TRANSACTION_ID is really just a bit pattern - interpretation as a number is not necessarily our business.. but the octet order needs specifying in any case.

s5.1, para 3: 'One ACK_DATA parameter MUST contain one more sequence numbers of the HIP DATA packets being ACKed.':  I cannot parse this. I think it is trying to say that one ACK_DATA can ack multiple SEQ_DATA items by adding all their sequence numbers to the ACK_DATA parameter.  However the previous sentence implies only one SEQ_DATA is acked per ACK_DATA and the specification in s4.2 is ambiguous.  Please get your act together here! 

s5.3.2: with reference to the previous comment, s5.3.2 (initially) implies only one sequence number in a given ACK_DATA ('The sequence number reported in an ACK_DATA..'). but the last sentence seems to indicate that there might have been several.  

s5.2, item 5:  Exponential backoff MUST be used.

s10.1: Needs a normative reference for the IPv6 next header registry (definition).

Nits/editorial comments:
General: consistency between use of bytes and octets (octets preferred).
e.g., s4.4 uses octets.
s1, para 2, sentence 2: s/HIP Data packet has/The HIP Data packet has/
s1, para 2, sentence 4: s/aimed/intended/
s1, para2, sentence 5: s/ESP/the ESP/ - and ESP needs expanding.

s2, para 2: s/MIC does/The Mic does/, s/thus need be/thus needs to be/

s3.1.1, last sentence: s?there is either more extensions header or data?there are either more extension headers and/or data?

s4 needs a reference to the notation conventions used in RFC 5201 (s2.2 of RFC 5201).
s4, next to last para: s/one or more PAYLOAD_MIC parameter/one or more PAYLOAD_MIC parameters/, s/Hash algorithm used to generate MIC is same/The hash algorithm used to generate the MIC is the same/
s4, last para: s/receiver/the receiver/
s4.3, 'Next header': s/The values for are/The values for this field are/.  A specific reference for the registry is needed (from the IPv6 protocol).
s4.3, 'Payload MIC': s/points to/points/, s/used function/function used/

s5.1, para 1: s/contain SEQ_DATA or ACK_DATA parameter if both parameters are missing then packet MUST be dropped as invalid/contain at least one of a SEQ_DATA or an ACK_DATA parameter; if both parameters are missing then the packet MUST be dropped as invalid/

s5.1, para 2: s/one or more PAYLOAD_MIC parameter otherwise packet MUST be dropped/one or more PAYLOAD_MIC parameters or otherwise the packet MUST be dropped/

s5.2 items 2 and 3: There are lots of definite and indefinite articles missing.

s5.3, first sentence: s/decision/a decision/
s5.3, end of para 1: s/responsible of/responsible for/

s5.3.1, para 2: s/OK/adequate/
s5.3.1, para 3: s/verification fails/verifications fail/

s5.3.2, para 2: s/an earlier sent/a previously sent/

__________________________