Last Call Review of draft-ietf-rohc-ikev2-extensions-hcoipsec-
|Requested rev.||no specific revision (document currently at 12)|
|Type||Last Call Review|
|Team||Security Area Directorate (secdir)|
|Draft last updated||2009-10-16|
Secdir Last Call review of -?? by Glen Zorn
Secdir Telechat review of -?? by Glen Zorn
I have reviewed this document as part of the security directorate's ongoing effort to review all IETF documents being processed by the IESG. These comments were written primarily for the benefit of the security area directors. Document editors and WG chairs should treat these comments just like any other last call comments. COMMENTS General ------- There are lots of occurrences of constructions similar to "The Notify Payload (defined in [IKEV2]) is illustrated in Figure 1." Roughly translated into English, this says "The Notify Payload (defined in the reference to RFC 4306) is illustrated in Figure 1." which, of course is nonsense: the _reference_ to RFC 4306 doesn't define, the document does. Suggest changing all such instances to something like "The Notify Payload (defined in RFC 4302 [IKEV2]) is illustrated in Figure 1." Abstract -------- I can't tell what the intended status of this draft might be (i.e., Standards Track, etc.). The I-D Tracker says that the draft wants to be a Proposed Standard, but there is no reference to RFC 2119 nor any use of 2119 keywords. It might be a good idea to fix this (under the assumption that the editor will do so, I'll not further comment upon 'must' that probably should be 'MUST', etc. There shouldn't be any references in the Abstract. The acronym "ROHC" should be expanded on first usage. Section 2.1 ----------- Paragraph 4 says: A new Notify Message Type value, denoted ROHC_SUPPORTED, indicates that the Notify payload is conveying ROHC channel parameters. The value for the ROHC_SUPPORTED message is specified in Section 4. However, that's not really true: section 4 just says that IANA needs to assign a value. Suggest changing to: A new Notify Message Type value, denoted ROHC_SUPPORTED, indicates that the Notify payload is conveying ROHC channel parameters (Section 4). The description of the Notify Payload fields doesn't include the SPI field or the Notification Data field. Since the SPI Size field is specified to be zero, I would assume that the SPI field itself must be omitted. Is that correct? If so (RFC 4306 isn't crystalline on the subject, either) & since the diagram of the payload and the description are specific to this application, I think the this should be stated, if not illustrated in the diagram itself. Also, I think that the contents of the Notification Data field should be described; maybe something like Notification Data (variable length) (2 octets) This field contains three or more ROHC Attributes (section 2.1.1). I find the headings for this section and the next misleading: this section is headed "ROHC Channel Parameters that are Signaled" when it actually seems to be talking about the "ROHC_SUPPORTED Notify Message", while the next is headed "ROHC_SUPPORTED Notify Message" when it is actually describing "ROHC Attributes". Suggest changing the headings accordingly. Section 2.1.1 ------------- The format and description of the ROHC Attribute are quite confusing: on the one hand, the ROHC Attribute Type field is stated to be 2 octets in length, but on the other hand the actual value is only 15 bits (as reflected in the IANA Considerations section); further, since the length is not reflected in the registry value itself, an implementation would need to set the AF bit (claimed to be, but not, part of the Attribute Type) according to the Attribute type. A different, perhaps more elegant, way to accomplish the same goal might be to dispense with the AF bit altogether and simply specify that fixed-length Attributes are numbered 0x8000-0xFFFF, while variable-length Attributes are allocated from the range 0x0000-0x7FFF. Section 2.1.2 ------------- The sub-headings on the attribute descriptions are disconcerting: since the first paragraph lists the attributes by name (MAX_CID, etc.), I scanned for those in the following paragraphs but was met with textual descriptions (like "Maximum Context Identifier") which might better be placed in the description itself. Suggest changing them to list the name first; it might also be nice to put the actual Attribute number in the header for quick reference. So the suggestion is to change, for example, Maximum Context Identifier (MAX_CID, AF = 1) The MAX_CID attribute is a mandatory attribute. Exactly one to MAX_CID (Maximum Context Identifier, AF = 1) The MAX_CID attribute is a mandatory attribute. Exactly one or (if you accept my numbering suggestion above) MAX_CID (0x8001) The MAX_CID (Maximum Context Identifier) attribute is a mandatory attribute. Exactly one Under ROHC_INTEG it says "The attribute contains an integrity algorithm". I'm assuming that this is actually not true (unless some _really_ amazing advances in compression have occurred recently ;-). Suggest changing to "The attribute value contains the identifier of an integrity algorithm". Under "ROHC_ICV_LEN": The acronym "ICV" should be expanded on first usage. Suggest changing "If ROHC_ICV_LEN length is zero" to "If the value of the ROHC_ICV_LEN attribute is zero" Under "MRRU" it says "If present, the attribute value is two octets in length." but this doesn't seem to make sense. Suggest changing to "The attribute value is two octets in length."