Document: draft-kelly-ipsec-ciph-sha2-01.txt Reviewer: Miguel Garcia Review Date: 2007-01-21 IETF LC End Date: 2007-02-08. Summary: This draft is basically ready for publication, but has nits that should be fixed before publication. Comments: 1) In Section 1, second paragraph, there are references to HAMC-SHA-PRF-256, HMAC-SHA-PRF-384, and HMAC-SHA-PRF-512. The same references appear in the first paragraph in Section 2.4. However, in the table in Section 2.6 and the test vectors in Section 2.7.1, there are references to HMAC-SHA-256-PRF, HMAC-SHA-384-PRF, AND HMAC-SHA-512-PRF. And in the IANA considerations section the references are to PRF_HMAC_SHA2_256, PRF_HMAC_SHA2_384, and PRF_HMAC_SHA2_512. The question is: are these three groups of tokens the same thing? Notice the reversal of PRF-nnn and nnn-PRF, and the different naming scheme in the IANA section. If they are the same thing, there should be only one way to refer to them. If they are different things, then the document has substantial misleading information as for to guarantee failures in implementations. 2) IANA considerations section. The document does not specify: a) The registry IANA has to operate. b) The subregistry within that registry IANA has to operate c) A differentiation between instructions to IANA and background information to the reader about already assigned values. Also, it is not clear the relation of the tokens written in this section with those written elsewhere in the draft. Common sense indicates that the digits are the key to map them, but I believe the draft should be more clear on this. For example: is AUTH_HMAC_SHA2_256_128 a token that refers to what is known elsewhere as HMAC-SHA-256-128? Since the token name is different, one ask the question of whether they are the same thing or not. This applies to all tokens defined in the IANA section. If I were to write this section, I will write something along these lines (please check if I got it right, it is not so easy): ------ 4. IANA Considerations 4.1 New assignments This memo instructs IANA to assign new values to the IKEv2 Transform Attributes Types registry, according to the following data: For Transform Type 2 (Pseudo-random Function), defined Transform IDs are: Number Name Reference ------ --------------------------------- --------- HMAC-SHA-256-PRF [RFCXXXX] HMAC-SHA-384-PRF [RFCXXXX] HMAC-SHA-512-PRF [RFCXXXX] For Transform Type 3 (Integrity Algorithm), defined Transform IDs are: Number Name Reference ------ --------------------------------- --------- HMAC-SHA-256-128 [RFCXXXX] HMAC-SHA-384-192 [RFCXXXX] HMAC-SHA-512-256 [RFCXXXX] 4.2 Existing assignments This memo uses the existing Authentication Algorithms HMAC-SHA2-256, HMAC-SHA2-384, and HMAC-SHA2-512 defined in the IPSEC Security Association Attributes subregistry of the ISAKMP registry. Please consult the online IANA ISAKMP registry for details of those values. Note: This memo does not specify the conventions for using SHA-256+ for IKE Phase 1 negotiation. --------- 3) The document does not separate the references between Normative and Informative. This is a requirement for RFCs. Some other minor comments: 4) You may want to expand acronyms before its first usage. For example, in Section 2.1.2 you may want to expand "IKE_SA". 5) As a personal matter of taste, I don't like references that do not include the RFC number. For example, I don't like text that reads "... is specified in [HMAC]". I would like to see "... is specified in RFC 2104 [HMAC]". 6) You may want to add a reference to HMAC-SHA1-96 in the first paragraph of Section 3.1 7) The idnits tools report a number of errors that should be fixed. I attach those errors here for your convenience: idnits 1.124 tmp/draft-kelly-ipsec-ciph-sha2-01.txt: tmp/draft-kelly-ipsec-ciph-sha2-01.txt(353): Line is too long: the offending characters are '-+' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(354): Line is too long: the offending characters are ' |' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(355): Line is too long: the offending characters are ' |' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(356): Line is too long: the offending characters are '=+' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(357): Line is too long: the offending characters are ' |' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(358): Line is too long: the offending characters are '-+' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(359): Line is too long: the offending characters are ' |' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(360): Line is too long: the offending characters are '-+' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(361): Line is too long: the offending characters are ' |' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(362): Line is too long: the offending characters are '-+' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(363): Line is too long: the offending characters are ' |' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(364): Line is too long: the offending characters are '-+' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(365): Line is too long: the offending characters are ' |' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(366): Line is too long: the offending characters are '-+' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(367): Line is too long: the offending characters are ' |' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(368): Line is too long: the offending characters are '-+' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(634): Line is too long: the offending characters are ')' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(635): Line is too long: the offending characters are ')' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(709): Line is too long: the offending characters are ')' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(710): Line is too long: the offending characters are ')' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(711): Line is too long: the offending characters are ')' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(807): Line is too long: the offending characters are ')' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(808): Line is too long: the offending characters are ')' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(809): Line is too long: the offending characters are ')' tmp/draft-kelly-ipsec-ciph-sha2-01.txt(810): Line is too long: the offending characters are ')' Checking nits according to http://www.ietf.org/ID-Checklist.html: Checking conformance with RFC 3978/3979 boilerplate... - This document has ISOC Copyright according to RFC 3978, instead of the newer IETF Trust Copyright according to RFC 4748. You should consider updating it; the new Copyright statement will be required from February 1st, 2007 - This document has an original RFC 3978 Section 5.5 Disclaimer, instead of the newer disclaimer which includes the IETF Trust according to RFC 4748. You should consider updating it; the new disclaimer will be required from February 1st, 2007 * The document seems to lack a both a reference to RFC 2119 and the recommended RFC 2119 boilerplate, even if it appears to use RFC 2119 keywords. RFC 2119 paragraph 2 text: "The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119." RFC 2119 keyword, line 152: '...e hash functions MUST be supported, and ...' RFC 2119 keyword, line 154: '... MUST NOT be supported....' RFC 2119 keyword, line 196: '... function MUST be used to generate the...' RFC 2119 keyword, line 248: '...m output size -- MUST be supported. No ...' RFC 2119 keyword, line 785: '... 2001 MAY,...' Checking nits according to http://www.ietf.org/ietf/1id-guidelines.txt: Nothing found here (but these checks do not cover all of 1id-guidelines.txt yet). Miscellaneous warnings: None. Experimental warnings: - Missing Reference: 'TBA-1' is mentioned on line 725, but not defined 'PRF_HMAC_SHA2_256 [TBA-1]...' - Missing Reference: 'TBA-2' is mentioned on line 727, but not defined 'PRF_HMAC_SHA2_384 [TBA-2]...' - Missing Reference: 'TBA-3' is mentioned on line 729, but not defined 'PRF_HMAC_SHA2_512 [TBA-3]...' - Missing Reference: 'TBA-4' is mentioned on line 736, but not defined 'AUTH_HMAC_SHA2_256_128 [TBA-4]...' - Missing Reference: 'TBA-5' is mentioned on line 738, but not defined 'AUTH_HMAC_SHA2_384_192 [TBA-5]...' - Missing Reference: 'TBA-6' is mentioned on line 740, but not defined 'AUTH_HMAC_SHA2_512_256 [TBA-6]...'