Rene, thanks for this review; responses inline below: > -----Original Message----- > From: [email protected] [mailto:[email protected]] On > Behalf Of René Hummen > Sent: Sunday, September 23, 2012 8:36 AM > To: HIP WG > Subject: Re: [Hipsec] WGLCs: 4423bis and 5201bis > > Hi all, > > I had a close look at draft-ietf-hip-rfc5201-bis-09 and found some > technical as well as editorial issues that I consider worth discussing > and fixing. Please note that especially the technical issues regarding > the used packet space may not be considered problematic in case of > everyday Internet connections. However, resource-constrained > environments as targeted by RPL, 6LowPAN, CoAP, and HIP DEX have hard > payload limits. Here, a few bytes of additional packet size can > determine whether packets need to be fragmented or not. Therefore, I > think, we should also consider these scenarios in the basic HIP > specification that is shared by all HIP variants. > > I structured my feedback into different topics highlight by ###. > > > Technical issues: > ------------------------ > > ### R1 Counter ### > > 4.1.4. HIP Replay Protection > "The R1 counter SHOULD NOT roll over." > > No explanation is given what implementors should do when a roll over > occurs. I suggest to add the following text: > > "If a roll over is detected, the associated HIs SHOULD be removed and > new ones should be generated." > > Note, however, that such a change would also invalidate ACLs and HI-IP > lookup information based on the original HIs.
I might suggest instead to make the invalidation of HIs to be use case dependent, and point out the issues. "If a roll over is detected, the associated HIs may need to be replaced if there is concern that such roll over may impact potential initiators; however, such replacement would also invalidate any other state in the network that associates the HI with other identifiers." > > > ### HIP Checksum ### > > 5.1.1. Checksum > > What is the reason for using the IP-based pseudo-headers here? If I am > not overlooking something, non-HIP-aware NATs on the communication path > effectively break the HIP checksum. I know that this is the way how > TCP/IP pseudo-headers are defined. Still, I suggest to only checksum > the HIP header and the HIP parameters to avoid problems in the future > and to maintain layer separation. What happens if HIP is used in non-IP > environments? I believe that this decision was taken to be able to leverage any possible existing checksum implementations. I am neutral about changing this as suggested, but wouldn't object if others supported it for non-IP reasons. I might suggest at least that a statement be added here that checksum computation in non-IP environments is out of scope of this document. > > > ### Decreasing the per-packet overhead ### > > 4.1.2. Puzzle Exchange > "Using the Opaque data field in the PUZZLE (see Section 5.2.4), in an > ECHO_REQUEST_SIGNED (see Section 5.2.20) or in an ECHO_REQUEST_UNSIGNED > parameter (see Section 5.2.21), the Responder can include some data in > R1 that the Initiator MUST copy unmodified in the corresponding I2 > packet." > > 5.2.4. PUZZLE > "The Opaque and Random #I field are not covered by the HIP_SIGNATURE_2 > parameter." > > Especially in resource-constrained environments such as targeted by HIP > DEX, each byte that needs to be transmitted is valuable (from a power > perspective) and may lead to fragmentation at the lower layers. > Hence, I suggest removing the 2 bytes of opaque value from the PUZZLE > parameter and to add +2 to the type number (resulting in 259) for the > new PUZZLE type. If the Responder has to transfer state in an unsigned > fashion, we already provide the ECHO_REQUEST_UNSIGNED parameter for > this purpose. Furthermore, the opaque value does not seem to be > necessary, as HIPL currently does not use it. If I understand correctly, you're suggesting a new PUZZLE type that has opaque data (type code 259) but to remove opaque from PUZZLE? > > > 5.2.5. SOLUTION > > Here, I suggest removing 1 byte of reserved space and the 2 byte, > echoed opaque value for the same reason presented above. The type > number could be moved to 323 (SOLUTION + 2). If we need to modify the > puzzle parameter for some reason in the future, I suggest to design a > new parameter instead of using the reserved field. That is how new > semantics are handled for any other parameter as well (see MAC_X). > > > 5.2.3. R1_COUNTER > "It SHOULD be included in the R1 (in which case, it is covered by the > signature), and if present in the R1, it MUST be echoed (including the > Reserved field verbatim) by the Initiator in the I2 packet." > > Similar to the SOLUTION parameter, I suggest removing the 4 bytes of > reserved space from the R1 counter parameter and to add +2 to the type > number (resulting in 131) for the new R1_COUNTER type. > > > I understand that these changes break parameter compatibility with > existing v1 implementations. However, I do not consider a new parameter > layout problematic as the semantics won't change considerably allowing > for code re-use in most cases. I don't have a major problem with your proposals to shed some bytes or introduce new parameters, but won't the overhead just come back in the form of extra padding (see 5.2.1)? Do you need to revisit this padding requirement to accomplish your goals? > > > Editorial changes: > ------------------------ > > 1.3. Memo Structure > "Finally, Sections 7, 8, and 9 discuss policy, security, and IANA > considerations, respectively." > > 8 and 9 lack internal references to the respective sections in the > document. > > > 4.1.7. HIP Downgrade Protection > "In contrast to the first version of HIP [RFC5201],the version 2 of > [...]" > > There is a space missing after the comma: > "In contrast to the first version of HIP [RFC5201], the version 2 of > [...]" > > > ### HIP Header Length ### > > 5.1. Payload Format > "The Header Length field contains the combined length of the HIP Header > and HIP parameters in 8-byte units, excluding the first 8 bytes." > > 5.2.21. ECHO_REQUEST_UNSIGNED > "The creator of the ECHO_REQUEST_UNSIGNED (end-host or middlebox) has > to create the Opaque field so that it can later identify and remove the > corresponding ECHO_RESPONSE_UNSIGNED parameter." > > 5.3.2. R1 - the HIP Responder Packet > "The signature is calculated over the whole HIP packet, after setting > the Initiator's HIT, header checksum, as well as the Opaque field and > the Random #I in the PUZZLE parameter temporarily to zero, and > excluding any parameters that follow the signature, as described in > Section 5.2.15." > > These three sections indicate the following problem with the HIP header > length field: > The sender generates the packet, sets the header length, and signs/MACs > the packet. A middlebox afterwards adds an ECHO_REQUEST_UNSIGNED > parameter to the packet. However, this invalidates the HIP header > length information resulting in a broken packet. If the middlebox > modifies the HIP header length value, it invalidates the signature/MAC. > > However, following the references to: > 6.4.1. HMAC Calculation > 6.4.2. Signature Calculation > ... clarifies the issue I just described: > "In the HIP header, the Header Length field value is calculated to the > beginning of the [HIP_SIGNATURE | HIP_MAC] parameter." > > Hence, I suggest the following text in Section 5.3.2 in order to > prevent misconceptions by implementors: > "The signature is calculated over the HIP packet as described in > Section 5.2.15." > > > ### References ### > > 12. References > > References to HIP-related documents refer to old revisions (e.g., I- > D.ietf-hip-cert). > Thanks for these editorial fixes. - Tom _______________________________________________ Hipsec mailing list [email protected] https://www.ietf.org/mailman/listinfo/hipsec
