Last Call Review of draft-ietf-rohc-ikev2-extensions-hcoipsec-
review-ietf-rohc-ikev2-extensions-hcoipsec-secdir-lc-zorn-2009-10-16-00

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."